feat(skeleton): add_node_from_binary_data

[TzahiTaub]

This change is 

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 62.29508% with 23 lines in your changes are missing coverage. Please review.

Project coverage is 69.05%. Comparing base (9c049ae) to head (4d11982).
Report is 1 commits behind head on main.

Files	Patch %	Lines
...ted_skeleton_tree/compute_updated_skeleton_tree.rs	59.64%	23 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #137      +/-   ##
==========================================
- Coverage   70.80%   69.05%   -1.76%     
==========================================
  Files          32       34       +2     
  Lines        1175     1241      +66     
  Branches     1175     1241      +66     
==========================================
+ Hits          832      857      +25     
- Misses        302      344      +42     
+ Partials       41       40       -1     

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

[TzahiTaub]

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

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs line 86 at r1 (raw file):

                        }
                        OriginalSkeletonNode::Binary => UpdatedSkeletonNode::Binary,
                        OriginalSkeletonNode::Leaf(_) => {

Updating this due to the changes in the UpdateSkeletonTree::create

[TzahiTaub]

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

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs line 86 at r1 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Updating this due to the changes in the UpdateSkeletonTree::create

Done

[Yoni-Starkware]

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

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs line 60 at r1 (raw file):

    /// Builds a (probably binary) node from its two updated children. Returns the TempSkeletonNode
    /// matching the given root for the subtree it is the root of. if one or more children are
    /// empty the resulting node will not be binary .

Suggestion:

he resulting node will not be binary.

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs line 70 at r1 (raw file):

        let [left_index, right_index] = root_index.get_children_indices();

        if *left != TempSkeletonNode::Empty && *right != TempSkeletonNode::Empty {

Implement is_empty() for TempSkeletonNode

Code quote:

if *left != TempSkeletonNode::Empty && *right != TempSkeletonNode::Empty {

[Yoni-Starkware]

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

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs line 72 at r1 (raw file):

        if *left != TempSkeletonNode::Empty && *right != TempSkeletonNode::Empty {
            // Both children are non-empty - a binary node.
            // finalize children as a binary node cannot change form.

.

Suggestion:

// finalize children, as a binary node cannot change form

[dorimedini-starkware]

Reviewed 3 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs line 60 at r2 (raw file):

    /// Builds a (probably binary) node from its two updated children. Returns the TempSkeletonNode
    /// matching the given root for the subtree it is the root of. if one or more children are
    /// empty the resulting node will not be binary .

non-blocking

Suggestion:

    /// Builds a (probably binary) node from its two updated children. Returns the TempSkeletonNode
    /// matching the given root for the subtree it is the root of. If one or more children are
    /// empty, the resulting node will not be binary.

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs line 72 at r2 (raw file):

        if *left != TempSkeletonNode::Empty && *right != TempSkeletonNode::Empty {
            // Both children are non-empty - a binary node.
            // finalize children as a binary node cannot change form.

non-blocking

Suggestion:

Finalize children, as a binary node cannot change form.

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs line 73 at r2 (raw file):

            // Both children are non-empty - a binary node.
            // finalize children as a binary node cannot change form.
            let mut insert_updated = |index: NodeIndex, node: &TempSkeletonNode| {

I think it's clearer to just loop, and not define a lambda function

Suggestion:

for (index, node) in [(left_index, left), (right_index, right)] {

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs line 75 at r2 (raw file):

            let mut insert_updated = |index: NodeIndex, node: &TempSkeletonNode| {
                if let TempSkeletonNode::Original(original_node) = node {
                    let updated = match original_node {

save a bit of indentation by flattening the match and explicitly panicking on the Empty case. no need for if let .. else

Suggestion:

                match node {
                    TempSkeletonNode::Empty => panic!("bla"),
                    TempSkeletonNode::Original(OriginalSkeletonNode::EdgeSibling(edge_data)) => {
                        ...
                    }
                    ...
                }

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs line 76 at r2 (raw file):

                if let TempSkeletonNode::Original(original_node) = node {
                    let updated = match original_node {
                        OriginalSkeletonNode::EdgeSibling(edge_data) => {

destructure for the data you use

Suggestion:

(EdgeData { path_to_bottom, bottom_hash })

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree_test.rs line 127 at r2 (raw file):

    &TempSkeletonNode::Empty,
    &TempSkeletonNode::Empty,
    &[(NodeIndex::from(2), SkeletonLeaf::Zero), (NodeIndex::from(3), SkeletonLeaf::Zero)],

change the test inputs to integer types and convert them in the test body; makes cases much easier to read.
SkeletonLeaf should have a from_felt function or something that you can use

Suggestion:

    1,
    &TempSkeletonNode::Empty,
    &TempSkeletonNode::Empty,
    &[(2, 0), (3, 0)],

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree_test.rs line 162 at r2 (raw file):

    &TempSkeletonNode::Original(OriginalSkeletonNode::Binary),
    &[(NodeIndex::from(20), SkeletonLeaf::Zero)],
    TempSkeletonNode::Original(OriginalSkeletonNode::Edge { path_to_bottom: PathToBottom::from("1") }),

Suggestion:

PathToBottom::RIGHT

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree_test.rs line 172 at r2 (raw file):

    TempSkeletonNode::Empty,
    &[]
)]

please add a test case where the right child's subtree is deleted, and the left child was an edge. the expected node should be an edge with +1 length and a path prefixed with an extra 0, right?

[TzahiTaub]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs line 70 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Implement is_empty() for TempSkeletonNode

Done.

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs line 73 at r2 (raw file):

Previously, dorimedini-starkware wrote…

I think it's clearer to just loop, and not define a lambda function

Done.

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs line 75 at r2 (raw file):

Previously, dorimedini-starkware wrote…

save a bit of indentation by flattening the match and explicitly panicking on the Empty case. no need for if let .. else

Done.

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs line 76 at r2 (raw file):

Previously, dorimedini-starkware wrote…

destructure for the data you use

Done.

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree_test.rs line 127 at r2 (raw file):

Previously, dorimedini-starkware wrote…

change the test inputs to integer types and convert them in the test body; makes cases much easier to read.
SkeletonLeaf should have a from_felt function or something that you can use

If you want the rest of the args, let's do it in another PR please.

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree_test.rs line 162 at r2 (raw file):

    &TempSkeletonNode::Original(OriginalSkeletonNode::Binary),
    &[(NodeIndex::from(20), SkeletonLeaf::Zero)],
    TempSkeletonNode::Original(OriginalSkeletonNode::Edge { path_to_bottom: PathToBottom::from("1") }),

Done.

[Yoni-Starkware]

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

[TzahiTaub]

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

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree_test.rs line 172 at r2 (raw file):

Previously, dorimedini-starkware wrote…

please add a test case where the right child's subtree is deleted, and the left child was an edge. the expected node should be an edge with +1 length and a path prefixed with an extra 0, right?

Done.

[Yoni-Starkware]

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

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs line 75 at r2 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Done.

Not sure the TempSkeletonNode::Original( prefix is better.
To avoid the indentation, you can add some kind of unwrap method to the temp node. Maybe original().

Also, consider using the native Some/None Enum

type TempSkeletonNode = Option(OriginalSkeletonNode);

[TzahiTaub]

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

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs line 75 at r2 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Not sure the TempSkeletonNode::Original( prefix is better.
To avoid the indentation, you can add some kind of unwrap method to the temp node. Maybe original().

Also, consider using the native Some/None Enum

type TempSkeletonNode = Option(OriginalSkeletonNode);

Not sure either what is better, told that to @dorimedini-starkware . Uding Optional to mark an empty valid value seems to me like a bad practice as it's like using null as a value.

[Yoni-Starkware]

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

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs line 75 at r2 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Not sure either what is better, told that to @dorimedini-starkware . Uding Optional to mark an empty valid value seems to me like a bad practice as it's like using null as a value.

Ok, still consider getting rid of TempSkeletonNode::Original using something similar to unwrap()/some() etc

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs line 106 at r5 (raw file):

                            path_to_bottom: *path_to_bottom,
                        }
                    }

Can you document this special case?

Code quote:

                        self.skeleton_tree.insert(
                            path_to_bottom.bottom_index(index),
                            UpdatedSkeletonNode::Sibling(*bottom_hash),
                        );
                        UpdatedSkeletonNode::Edge {
                            path_to_bottom: *path_to_bottom,
                        }
                    }

[dorimedini-starkware]

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

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs line 26 at r5 (raw file):

impl TempSkeletonNode {
    fn is_empty(&self) -> bool {
        *self == TempSkeletonNode::Empty

non-blocking

Suggestion:

*self == Self::Empty

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree_test.rs line 43 at r5 (raw file):

        leaf_modifications,
    )
}

can you make a separate updated_skeleton_tree fixture that depends on the leaf_modifications fixture and just use both fixtures? why is the tuple fixture needed?
don't you get the leaf_modifications as input to the test anyway?

Code quote:

type UpdatedSkeletonAndModifications = (UpdatedSkeletonTreeImpl, LeafModifications<SkeletonLeaf>);

#[fixture]
fn updated_skeleton_and_modifications(
    #[default(&[])] leaf_modifications: &[(u128, u8)],
) -> UpdatedSkeletonAndModifications {
    let leaf_modifications: LeafModifications<SkeletonLeaf> = leaf_modifications
        .iter()
        .map(|(index, leaf_val)| (NodeIndex::from(*index), SkeletonLeaf::from(*leaf_val)))
        .collect();
    let skeleton_tree: HashMap<NodeIndex, UpdatedSkeletonNode> = leaf_modifications
        .iter()
        .filter(|(_, leaf)| !leaf.is_zero())
        .map(|(index, leaf)| (*index, UpdatedSkeletonNode::Leaf(*leaf)))
        .collect();
    (
        UpdatedSkeletonTreeImpl { skeleton_tree },
        leaf_modifications,
    )
}

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree_test.rs line 133 at r5 (raw file):

    &TempSkeletonNode::Empty,
    &TempSkeletonNode::Empty,
    &[(2,0), (3,1)],

second skeleton leaf should also be zero, no? why did the test pass?

Suggestion:

&[(2,0), (3,0)],

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree_test.rs line 197 at r5 (raw file):

    updated_skeleton_and_modifications: UpdatedSkeletonAndModifications,
) {
    let (mut updated_skeleton, leaf_modifications) = updated_skeleton_and_modifications;

why was this change needed?
can't you send the leaf_modifications as fixture input and get them as input to the test?

Code quote:

    #[case] _leaf_modifications: &[(u128, u8)],
    #[case] expected_node: TempSkeletonNode,
    #[case] expected_skeleton_additions: &[(NodeIndex, UpdatedSkeletonNode)],
    #[with(_leaf_modifications)]
    updated_skeleton_and_modifications: UpdatedSkeletonAndModifications,
) {
    let (mut updated_skeleton, leaf_modifications) = updated_skeleton_and_modifications;

[Yoni-Starkware]

I'm OOO next week, so you can dismiss my comments if they block you somehow (they should not)

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @TzahiTaub)

[TzahiTaub]

Reviewable status: 3 of 6 files reviewed, 5 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs line 75 at r2 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Ok, still consider getting rid of TempSkeletonNode::Original using something similar to unwrap()/some() etc

Dori to the rescue. Got a nice idea.

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs line 106 at r5 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Can you document this special case?

Is this OK? Made me think this EdgeSibling variant should be removed entirely from the OriginalSkeleton.

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree_test.rs line 43 at r5 (raw file):

Previously, dorimedini-starkware wrote…

can you make a separate updated_skeleton_tree fixture that depends on the leaf_modifications fixture and just use both fixtures? why is the tuple fixture needed?
don't you get the leaf_modifications as input to the test anyway?

Done.

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree_test.rs line 133 at r5 (raw file):

Previously, dorimedini-starkware wrote…

second skeleton leaf should also be zero, no? why did the test pass?

You are right, but the node_from functions don't check consistency with the leaf_modifications. They only use it when they reach a leaf node. Got me thinking I can rely on the first pass we make on the modifications at the beginning of the create function and not pass as an argument at all. So I changed everything a bit. Sorry.

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree_test.rs line 197 at r5 (raw file):

Previously, dorimedini-starkware wrote…

why was this change needed?
can't you send the leaf_modifications as fixture input and get them as input to the test?

I think I'm done. Is this different from the previous comment about this? 😐

[TzahiTaub]

Reviewable status: 3 of 6 files reviewed, 6 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree_test.rs line 252 at r5 (raw file):

    &PathToBottom::RIGHT_CHILD,
    &NodeIndex::from(7),
    &TempSkeletonNode::Original(OriginalSkeletonNode::Leaf(SkeletonLeaf::NonZero)),

Not a possible scenario - if bottom_index is of a deleted leaf, when reaching node_from_edge_data as bottom was already updated, it should be Empty and this case is the same as the first case.

Code quote:

TempSkeletonNode::Original(OriginalSkeletonNode::Leaf(SkeletonLeaf::NonZero)),

[dorimedini-starkware]

Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @TzahiTaub and @Yoni-Starkware)

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs line 140 at r6 (raw file):

                    self.skeleton_tree.contains_key(bottom_index),
                    "bottom is a non-empty leaf but doesn't appear in the skeleton."
                );

there can be no case where the original node is a leaf that has changed to zero? what's going on here?
if so, please add a TODO to get the set of nonzero leaves as input, instead of modifications (which were already deleted), so the assert will no longer be on skeleton_tree.contains(index), but rather on nonzer_leaves.contains(index), seems cleaner

Code quote:

                // Leaf is finalized upon updated skeleton creation.
                // bottom_index is in the skeleton iff it wasn't deleted from the tree.
                assert!(
                    self.skeleton_tree.contains_key(bottom_index),
                    "bottom is a non-empty leaf but doesn't appear in the skeleton."
                );

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree_test.rs line 32 at r6 (raw file):

            .filter(|(_, leaf)| !leaf.is_zero())
            .map(|(index, leaf)| (index, UpdatedSkeletonNode::Leaf(leaf)))
            .collect(),

no need to map twice

Suggestion:

        skeleton_tree: leaf_modifications
            .iter()
            .filter(|(_, leaf_val)| leaf_val != Felt::ZERO)
            .map(|(index, leaf_val)| (
               NodeIndex::from(*index),
               UpdatedSkeletonNode::Leaf(SkeletonLeaf::from(*leaf_val))
            ))
            .collect(),

[TzahiTaub]

Reviewable status: 4 of 6 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs line 140 at r6 (raw file):

Previously, dorimedini-starkware wrote…

there can be no case where the original node is a leaf that has changed to zero? what's going on here?
if so, please add a TODO to get the set of nonzero leaves as input, instead of modifications (which were already deleted), so the assert will no longer be on skeleton_tree.contains(index), but rather on nonzer_leaves.contains(index), seems cleaner

self.skeleton_tree is the updated skeleton (added to the comment). Deleted leaves/nodes do not appear in it (we don't need it to compute any hashes, this becomes an empty subtree). No reason to get another input, we only need the fact that all modified non-zero leaves already appear in the updated skeleton.

---

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree_test.rs line 32 at r6 (raw file):

Previously, dorimedini-starkware wrote…

no need to map twice

Done.

[dorimedini-starkware]

Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yoni-Starkware)