fix: bug in lca

[aner-starkware]

This change is 

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.49%. Comparing base (027c5b4) to head (ebfa309).
Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #194      +/-   ##
==========================================
+ Coverage   61.86%   65.49%   +3.62%     
==========================================
  Files          36       36              
  Lines        1631     1788     +157     
  Branches     1631     1788     +157     
==========================================
+ Hits         1009     1171     +162     
+ Misses        571      564       -7     
- Partials       51       53       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dorimedini-starkware]

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @aner-starkware and @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/types.rs line 112 at r1 (raw file):

        if post_common_prefix_len == 0 && self < other {
            return *self;
        }

please add a short comment above this block explaining what does this if indicate (self is an ancestor of other?)

Code quote:

        if post_common_prefix_len == 0 && self < other {
            return *self;
        }

---

crates/committer/src/patricia_merkle_tree/types_test.rs line 55 at r1 (raw file):

#[case(U256([9,0]), U256([12,0]), U256([1,0]))]
#[case(U256([1,0]), U256([2,0]), U256([1,0]))]
#[case(U256([2,0]), U256([1,0]), U256([1,0]))]

The order of the u128s may depend on the architecture of the system.
Use U256::new(x) instead of U256([x, 0])

Code quote:

#[case(U256([1,0]), U256([1,0]), U256([1,0]))]
#[case(U256([2,0]), U256([5,0]), U256([2,0]))]
#[case(U256([5,0]), U256([2,0]), U256([2,0]))]
#[case(U256([8,0]), U256([10,0]), U256([2,0]))]
#[case(U256([9,0]), U256([12,0]), U256([1,0]))]
#[case(U256([1,0]), U256([2,0]), U256([1,0]))]
#[case(U256([2,0]), U256([1,0]), U256([1,0]))]

---

crates/committer/src/patricia_merkle_tree/types_test.rs line 59 at r1 (raw file):

    U256::from_str_hex("0x200000000000000000000000000000000000000000000000000000000000000").unwrap(),
    U256::from_str_hex("0x800000000000000000000000000000000000000000000000000000000000000").unwrap(),
    U256::from_str_hex("0x200000000000000000000000000000000000000000000000000000000000000").unwrap()

this case is a bit difficult to read; can you use U256::from_words and use bit-shifting to describe the high value?

Code quote:

    U256::from_str_hex("0x200000000000000000000000000000000000000000000000000000000000000").unwrap(),
    U256::from_str_hex("0x800000000000000000000000000000000000000000000000000000000000000").unwrap(),
    U256::from_str_hex("0x200000000000000000000000000000000000000000000000000000000000000").unwrap()

[aner-starkware]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/types.rs line 112 at r1 (raw file):

Previously, dorimedini-starkware wrote…

please add a short comment above this block explaining what does this if indicate (self is an ancestor of other?)

Done.

---

crates/committer/src/patricia_merkle_tree/types_test.rs line 55 at r1 (raw file):

Previously, dorimedini-starkware wrote…

The order of the u128s may depend on the architecture of the system.
Use U256::new(x) instead of U256([x, 0])

Done.

---

crates/committer/src/patricia_merkle_tree/types_test.rs line 59 at r1 (raw file):

Previously, dorimedini-starkware wrote…

this case is a bit difficult to read; can you use U256::from_words and use bit-shifting to describe the high value?

Done.

[dorimedini-starkware]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @aner-starkware and @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/types_test.rs line 51 at r2 (raw file):

#[case(U256::new(1), U256::new(1), U256::new(1))]
#[case(U256::new(2), U256::new(5), U256::new(2))]
#[case(U256::new(5), U256([2,0]), U256([2,0]))]

still need new here

Code quote:

U256([2,0]), U256([2,0]))]

---

crates/committer/src/patricia_merkle_tree/types_test.rs line 63 at r2 (raw file):

#[case(
    U256::from_words(6<<121, 12109832645278165874326859176438297),
    U256::from_words(7<<121, 34269583569287659876592876529763453),

what are these low values?

Code quote:

    U256::from_words(6<<121, 12109832645278165874326859176438297),
    U256::from_words(7<<121, 34269583569287659876592876529763453),

---

crates/committer/src/patricia_merkle_tree/types_test.rs line 64 at r2 (raw file):

    U256::from_words(6<<121, 12109832645278165874326859176438297),
    U256::from_words(7<<121, 34269583569287659876592876529763453),
    U256([3,0])

Suggestion:

U256::new(3)

[aner-starkware]

Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/types_test.rs line 63 at r2 (raw file):

Previously, dorimedini-starkware wrote…

what are these low values?

Just some random values, the idea is that it doesn't matter

[aner-starkware]

Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/types_test.rs line 51 at r2 (raw file):

Previously, dorimedini-starkware wrote…

still need new here

Done.

[dorimedini-starkware]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)

[dorimedini-starkware]

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @aner-starkware and @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/types.rs line 93 at r4 (raw file):

    pub(crate) fn is_ancestor_of(&self, descendant: &NodeIndex) -> bool {
        self.bit_length() < descendant.bit_length()

question: is self not a descendant of self?
please rename function to is_strict_ancestor_of just for clarity.

On the other hand, the only use case I see would benefit from a weak inequality... maybe change to <= and rename function to is_ancestor_or_equal?

Code quote:

self.bit_length() < descendant.bit_length()

---

crates/committer/src/patricia_merkle_tree/types.rs line 94 at r4 (raw file):

    pub(crate) fn is_ancestor_of(&self, descendant: &NodeIndex) -> bool {
        self.bit_length() < descendant.bit_length()
            && *self == *descendant >> (descendant.bit_length() - self.bit_length())

Suggestion:

        let (self_bits, descendant_bits) = (self.bit_length(), descendant.bit_length());
        self_bits < descendent_bits && *self == *descendant >> (descendant_bits - self_bits)

---

crates/committer/src/patricia_merkle_tree/types.rs line 105 at r4 (raw file):

        if self.is_ancestor_of(other) {
            return *self;
        }

Suggestion:

        // If self equal to / is an ancestor of other, return self.
        if self.is_ancestor_or_equal(other) {
            return *self;
        }

[TzahiTaub]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @aner-starkware)

---

crates/committer/src/patricia_merkle_tree/types_test.rs line 49 at r4 (raw file):

#[rstest]
#[case(U256::new(1), U256::new(1), U256::new(1))]

Shorter macro

Suggestion:

uint!(

[TzahiTaub]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @aner-starkware)

---

crates/committer/src/patricia_merkle_tree/types.rs line 105 at r4 (raw file):

        } else {
            *self >> (bit_length - other_bit_length)
        };

This is the only fix needed. Please remove the rest of the additions.

Suggestion:

        // Bring self and other to a common (low) height.
        let (adapted_self, adapter_other) = if self < other {
            (*self, *other >> (other_bit_length - bit_length))
        } else {
            (*self >> (bit_length - other_bit_length), *other)
        };

[aner-starkware]

Reviewable status: 1 of 2 files reviewed, 5 unresolved discussions (waiting on @dorimedini-starkware and @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/types.rs line 93 at r4 (raw file):

Previously, dorimedini-starkware wrote…

question: is self not a descendant of self?
please rename function to is_strict_ancestor_of just for clarity.

On the other hand, the only use case I see would benefit from a weak inequality... maybe change to <= and rename function to is_ancestor_or_equal?

Not relevant anymore.

---

crates/committer/src/patricia_merkle_tree/types.rs line 94 at r4 (raw file):

    pub(crate) fn is_ancestor_of(&self, descendant: &NodeIndex) -> bool {
        self.bit_length() < descendant.bit_length()
            && *self == *descendant >> (descendant.bit_length() - self.bit_length())

Not relevant anymore.

---

crates/committer/src/patricia_merkle_tree/types.rs line 105 at r4 (raw file):

        if self.is_ancestor_of(other) {
            return *self;
        }

Not relevant anymore.

---

crates/committer/src/patricia_merkle_tree/types.rs line 105 at r4 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

This is the only fix needed. Please remove the rest of the additions.

Done.

[aner-starkware]

crates/committer/src/patricia_merkle_tree/types_test.rs line 49 at r4 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Shorter macro

This macro works from strings, it's less clear imo.

[dorimedini-starkware]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @TzahiTaub)

[TzahiTaub]

Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @aner-starkware)

---

crates/committer/src/patricia_merkle_tree/types.rs line 101 at r5 (raw file):

        let other_bit_length = other.bit_length();
        // Bring self and other to a common (low) height.
        let (adapted_self, adapter_other) = if self < other {

Suggestion:

adapted_other

[TzahiTaub]

Reviewed 2 of 2 files at r6.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @aner-starkware)