refactor: no need to deserialize leaf siblings - all we need is the hash

[nimrod-starkware]

This change is 

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [33.795 ms 33.854 ms 33.926 ms]
change: [-3.5482% -2.6427% -1.9420%] (p = 0.00 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
2 (2.00%) high mild
1 (1.00%) high severe

[nimrod-starkware]

python side PR

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @dorimedini-starkware and @TzahiTaub)

[dorimedini-starkware]

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

---

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 209 at r1 (raw file):

                        !subtree.is_unmodified(),
                        "Unexpectedly deserialized leaf sibling."
                    );

I understand why this would be unexpected, but is it an unrecoverable error?

Code quote:

                    assert!(
                        !subtree.is_unmodified(),
                        "Unexpectedly deserialized leaf sibling."
                    );

---

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 219 at r1 (raw file):

                    if let Some(ref mut leaves) = previous_leaves {
                        leaves.insert(subtree.root_index, previous_leaf);
                    }

is the else case here an unrecoverable error?
if so please add a TODO to panic here on else (maybe even just call expect on previous_leaves)

Code quote:

                    if let Some(ref mut leaves) = previous_leaves {
                        leaves.insert(subtree.root_index, previous_leaf);
                    }

---

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 347 at r1 (raw file):

        should_fetch_modified_leaves: bool,
    ) {
        if !subtree.is_leaf() || (should_fetch_modified_leaves && !subtree.is_unmodified()) {

can you explain what is the logical change here?

Code quote:

if !subtree.is_leaf() || (should_fetch_modified_leaves && !subtree.is_unmodified()) {

[TzahiTaub]

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

---

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 219 at r1 (raw file):

Previously, dorimedini-starkware wrote…

is the else case here an unrecoverable error?
if so please add a TODO to panic here on else (maybe even just call expect on previous_leaves)

It's not an error, for storage trees without the trivial modifications flag, we don't want the previous leaves of modifications. It's only an error if config.compare_modified_leaves() is True right? Maybe we should assert this at the beginning of the function.

[dorimedini-starkware]

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

---

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 219 at r1 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

It's not an error, for storage trees without the trivial modifications flag, we don't want the previous leaves of modifications. It's only an error if config.compare_modified_leaves() is True right? Maybe we should assert this at the beginning of the function.

OK, so not a business logic error anyway? I wouldn't want this config to cause new panics, so I'm unblocking

[TzahiTaub]

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

[nimrod-starkware]

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

---

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 209 at r1 (raw file):

Previously, dorimedini-starkware wrote…

I understand why this would be unexpected, but is it an unrecoverable error?

I added this as a sanity check to make sure that we don't deserialize a leaf sibling...
We assume that leaf siblings do not exist in storage, so if we actually get a leaf sibling, we can't know if the deserialization has been done correctly.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 219 at r1 (raw file):

Previously, dorimedini-starkware wrote…

OK, so not a business logic error anyway? I wouldn't want this config to cause new panics, so I'm unblocking

not exactly.
leaves is not none in the case that we want also to return the previous leaves at the modified indices. It's independent of config.compare_modified_leaves() configuration.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 347 at r1 (raw file):

Previously, dorimedini-starkware wrote…

can you explain what is the logical change here?

Now we no longer assume that we can deserialize leaf siblings. (they do not exist in storage)
This function decides whether we should deserialize the next subtree or not.
If it's an inner node, we always have to deserialize.
If it's a leaf sibling, we don't really need (and can't - since we don't have it in storage) - the hash is enough.
If it's a modified leaf, then it depends on should_fetch_modified_leaves flag.

The change is that previously we deserialized a leaf sibling iff we deserialized a modified leaf.
Now, in any case, we don't deserialize a leaf sibling.

[dorimedini-starkware]

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

---

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 209 at r1 (raw file):

so if we actually get a leaf sibling, we can't know if the deserialization has been done correctly.

how can we incorrectly deserialize a leaf?
if it's not in storage we will get a missing key error from storage (in the future, everything should exist in storage anyway, just a matter of deciding to read or not);
but if we don't get a missing key error, how will the deserialization be "done incorrectly"?

---

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 217 at r1 (raw file):

                        log_trivial_modification!(subtree.root_index, previous_leaf);
                    }
                    if let Some(ref mut leaves) = previous_leaves {

is this comment correct? if so please add it

Suggestion:

                    }

                    // If previous values of modified leaves are required, add this leaf.
                    if let Some(ref mut leaves) = previous_leaves {

[dorimedini-starkware]

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

---

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 217 at r1 (raw file):

Previously, dorimedini-starkware wrote…

is this comment correct? if so please add it

required requested

[nimrod-starkware]

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

---

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 209 at r1 (raw file):

Previously, dorimedini-starkware wrote…

so if we actually get a leaf sibling, we can't know if the deserialization has been done correctly.

how can we incorrectly deserialize a leaf?
if it's not in storage we will get a missing key error from storage (in the future, everything should exist in storage anyway, just a matter of deciding to read or not);
but if we don't get a missing key error, how will the deserialization be "done incorrectly"?

you are right.
i added this assert to make sure that if we deserialize a leaf it's not a sibling. do you want me to remove this check?

---

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 217 at r1 (raw file):

Previously, dorimedini-starkware wrote…

required requested

Done.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 95.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 70.21%. Comparing base (94d0be9) to head (8e0b459).
Report is 4 commits behind head on main.

Files	Patch %	Lines
..._merkle_tree/original_skeleton_tree/create_tree.rs	95.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #310      +/-   ##
==========================================
- Coverage   70.65%   70.21%   -0.44%     
==========================================
  Files          38       38              
  Lines        2082     2122      +40     
  Branches     2082     2122      +40     
==========================================
+ Hits         1471     1490      +19     
- Misses        541      557      +16     
- Partials       70       75       +5     

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

[github-actions]

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [34.461 ms 34.870 ms 35.355 ms]
change: [+2.6875% +3.9781% +5.4283%] (p = 0.00 < 0.05)
Performance has regressed.
Found 15 outliers among 100 measurements (15.00%)
4 (4.00%) high mild
11 (11.00%) high severe

full_committer_flow performance regressed!
full_committer_flow time: [28.907 ms 28.949 ms 28.991 ms]
change: [+1.1846% +1.4489% +1.6709%] (p = 0.00 < 0.05)
Performance has regressed.

[dorimedini-starkware]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware)

---

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 209 at r1 (raw file):

Previously, nimrod-starkware wrote…

you are right.
i added this assert to make sure that if we deserialize a leaf it's not a sibling. do you want me to remove this check?

keep the check but just log a warning if the subtree is modified

[TzahiTaub]

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware)

---

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 219 at r1 (raw file):

Previously, nimrod-starkware wrote…

not exactly.
leaves is not none in the case that we want also to return the previous leaves at the modified indices. It's independent of config.compare_modified_leaves() configuration.

It's not indeoendent. If config.compare_modified_leaves() is True, then previous_leaves shouldn't be None in any case (and if it is, it's an error), doesn't it? If it's False, then it should be None for one Tree and False for another.

[nimrod-starkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware)

---

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 219 at r1 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

It's not indeoendent. If config.compare_modified_leaves() is True, then previous_leaves shouldn't be None in any case (and if it is, it's an error), doesn't it? If it's False, then it should be None for one Tree and False for another.

Let's take the storage tries for example: config.compare_modified_leaves() is True, but previous_leaves is none.
the variable previous_leaves is used and returned when create_and_get_previous_leaves is called

[nimrod-starkware]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)

---

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/create_tree.rs line 209 at r1 (raw file):

Previously, dorimedini-starkware wrote…

keep the check but just log a warning if the subtree is modified

Done.

[dorimedini-starkware]

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