feat(skeleton): add_construct_from_dege_data

[TzahiTaub]

This change is 

[codecov-commenter]

Codecov Report

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

Project coverage is 74.30%. Comparing base (b59276e) to head (dab06e2).
Report is 1 commits behind head on main.

Files	Patch %	Lines
...ted_skeleton_tree/compute_updated_skeleton_tree.rs	87.17%	4 Missing and 1 partial ⚠️
...itter/src/patricia_merkle_tree/filled_tree/tree.rs	0.00%	0 Missing and 1 partial ⚠️
...patricia_merkle_tree/updated_skeleton_tree/tree.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #129      +/-   ##
==========================================
+ Coverage   73.10%   74.30%   +1.19%     
==========================================
  Files          30       30              
  Lines        1071     1117      +46     
  Branches     1071     1117      +46     
==========================================
+ Hits          783      830      +47     
+ Misses        251      249       -2     
- Partials       37       38       +1     

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

[dorimedini-starkware]

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

---

crates/committer/src/patricia_merkle_tree/node_data/inner_node.rs line 32 at r1 (raw file):

        Self(self.0 + rhs.0)
    }
}

shorter

Suggestion:

#[derive(Clone, Copy, Debug, Default, derive_more::Add, PartialEq, Eq, Hash)]
pub struct EdgePathLength(pub u8);

---

crates/committer/src/patricia_merkle_tree/node_data/inner_node.rs line 56 at r1 (raw file):

        path: EdgePath(Felt::ONE),
        length: EdgePathLength(1),
    };

please move these definitions to the test utils area (you should be able to add impl PathToBottom blocks as many times as you want).
when you do, remove the cfg(test) attribute.
also, add a newline between the functions

Suggestion:

    pub(crate) const LEFT_CHILD: Self = PathToBottom {
        path: EdgePath(Felt::ZERO),
        length: EdgePathLength(1),
    };
    
    pub(crate) const RIGHT_CHILD: Self = PathToBottom {
        path: EdgePath(Felt::ONE),
        length: EdgePathLength(1),
    };

---

crates/committer/src/patricia_merkle_tree/node_data/inner_node.rs line 67 at r1 (raw file):

            length: self.length + other.length,
        }
    }

non-blocking

Suggestion:

    pub(crate) fn concat_paths(&self, other: PathToBottom) -> Self {
        Self {
            path: EdgePath((self.path.0 * Felt::TWO.pow(other.length.0)) + other.path.0),
            length: self.length + other.length,
        }
    }

---

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

// Builds a (probably edge) node from its given descendant. Returns the TempSkeletonNode matching
// the given root (the source for the path to bottom) for the subtree it is the root of. If bottom
// is empty, returns an empty node.

triple slash

Suggestion:

/// Builds a (probably edge) node from its given descendant. Returns the TempSkeletonNode matching
/// the given root (the source for the path to bottom) for the subtree it is the root of. If bottom
/// is empty, returns an empty node.

[dorimedini-starkware]

Reviewable status: 3 of 6 files reviewed, 5 unresolved discussions (waiting on @TzahiTaub)

---

-- commits line 2 at r1:
in general these commit messages will appear in release notes. branch names can be "neighborhood", commit messages - less

Suggestion:

add node_from_edge_data

[TzahiTaub]

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

---

crates/committer/src/patricia_merkle_tree/node_data/inner_node.rs line 32 at r1 (raw file):

Previously, dorimedini-starkware wrote…

shorter

Done.

---

crates/committer/src/patricia_merkle_tree/node_data/inner_node.rs line 56 at r1 (raw file):

Previously, dorimedini-starkware wrote…

please move these definitions to the test utils area (you should be able to add impl PathToBottom blocks as many times as you want).
when you do, remove the cfg(test) attribute.
also, add a newline between the functions

It's going to be in the actual code in the next PR. Replaced with allow dead code. A new line between the consts?

---

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

Previously, dorimedini-starkware wrote…

triple slash

Done.

[TzahiTaub]

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

---

-- commits line 2 at r1:

Previously, dorimedini-starkware wrote…

in general these commit messages will appear in release notes. branch names can be "neighborhood", commit messages - less

Done.

[dorimedini-starkware]

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

---

crates/committer/src/patricia_merkle_tree/test_utils.rs line 23 at r2 (raw file):

        PathToBottom {
            path: EdgePath(Felt::from(path)),
            length: EdgePathLength(length),

Suggestion:

        for c in value.chars() {
            path <<= 1;
            path |= match c {
                '0' => 0,
                '1' => 1,
                _ => panic!("Invalid character in path: {}", c),
            };
        }
        PathToBottom {
            path: EdgePath(Felt::from(path)),
            length: EdgePathLength(value.len()),

---

crates/committer/src/patricia_merkle_tree/test_utils.rs line 25 at r2 (raw file):

            length: EdgePathLength(length),
        }
    }

the wheel has been invented previously

Suggestion:

    fn from(value: &str) -> Self {
        Self {
            path: EdgePath(Felt::from(u128::from_str_radix(value, 2))),
            length: EdgePathLength(value.len()),
        }
    }

---

crates/committer/src/patricia_merkle_tree/node_data/inner_node.rs line 56 at r1 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

It's going to be in the actual code in the next PR. Replaced with allow dead code. A new line between the consts?

ok, so good that you removed the cfg(test).
ah... right, these aren't functions... but they are multiline definitions so I still prefer newlines.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/node.rs line 16 at r2 (raw file):

    EdgeSibling(EdgeData),
    Leaf(SkeletonLeaf),
    Empty,

rebase next time

Code quote:

Empty,

---

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

            TempSkeletonNode::Original(original_node) => {
                let res = match original_node {
                    OriginalSkeletonNode::Leaf(_) => {

to save indentation, please flatten these match arms like so:

Suggestion:

            TempSkeletonNode::Original(OriginalSkeletonNode::Leaf(_)) => {

---

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

                        let leaf = leaf_modifications.get(bottom_index).unwrap_or_else(|| {
                            panic!("Leaf modification {bottom_index:?} not found")
                        });

I think you should return an error in this case
my reasoning is: if modification data is missing, then it's the user of the committer library's fault, not the committer.

Suggestion:

                        let leaf = leaf_modifications.get(bottom_index).ok_or(
                            UpdatedTreeError::MissingLeafData(bottom_index)
                        );

---

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

    );
}

suggestion: instead of the two HashMap<NodeIndex, SkeletonLeaf> inputs, use &[(NodeIndex, SkeletonLeaf)] and do the HashMap::from() in the test body (to reduce some boilerplate in the #[case]s).

---

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

        (NodeIndex::from(7),
        UpdatedSkeletonNode::Binary)]
))]

wierd formatting here

Suggestion:

    HashMap::from([
        (NodeIndex::from(7), UpdatedSkeletonNode::Binary)
    ]))]

---

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

        SkeletonLeaf::Zero)]),
    TempSkeletonNode::Empty,
        HashMap::new()

formatting

Suggestion:

    &HashMap::from([
        (NodeIndex::from(7), SkeletonLeaf::Zero)
    ]),
    TempSkeletonNode::Empty,
    HashMap::new()

---

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

        UpdatedSkeletonNode::Leaf(SkeletonLeaf::NonZero))]
)
)]

formatting

Suggestion:

    &HashMap::from([
        (NodeIndex::from(7), SkeletonLeaf::NonZero)
    ]),
    TempSkeletonNode::Original(
        OriginalSkeletonNode::Edge {path_to_bottom: PathToBottom::RIGHT_CHILD}
    ),
    HashMap::from([
        (NodeIndex::from(7), UpdatedSkeletonNode::Leaf(SkeletonLeaf::NonZero))
    ]))]

---

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

    #[case] bottom: &TempSkeletonNode,
    #[case] leaf_modifications: &LeafModifications<SkeletonLeaf>,
    #[case] expected: TempSkeletonNode,

Suggestion:

expected_node

[TzahiTaub]

Reviewable status: 3 of 9 files reviewed, 9 unresolved discussions (waiting on @dorimedini-starkware)

---

crates/committer/src/patricia_merkle_tree/test_utils.rs line 23 at r2 (raw file):

        PathToBottom {
            path: EdgePath(Felt::from(path)),
            length: EdgePathLength(length),

Done.

---

crates/committer/src/patricia_merkle_tree/test_utils.rs line 25 at r2 (raw file):

Previously, dorimedini-starkware wrote…

the wheel has been invented previously

Done.

---

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

Previously, dorimedini-starkware wrote…

to save indentation, please flatten these match arms like so:

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…

I think you should return an error in this case
my reasoning is: if modification data is missing, then it's the user of the committer library's fault, not the committer.

But AFAIU this is based on the OriginalSkeletonNode - so if the user of the lib missed data - it should be caught when building the original skeleton, and if we're here after it was created, we can assume the skeleton and modification list should match each other.

---

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

Previously, dorimedini-starkware wrote…

wierd formatting here

Done.

---

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

Previously, dorimedini-starkware wrote…

formatting

Done.

---

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

Previously, dorimedini-starkware wrote…

formatting

Done.

---

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

    #[case] bottom: &TempSkeletonNode,
    #[case] leaf_modifications: &LeafModifications<SkeletonLeaf>,
    #[case] expected: TempSkeletonNode,

Done.

---

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/node.rs line 16 at r2 (raw file):

Previously, dorimedini-starkware wrote…

rebase next time

Done.

[dorimedini-starkware]

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

---

crates/committer/src/patricia_merkle_tree/test_utils.rs line 17 at r5 (raw file):

                (value.len() - if value.starts_with('+') { 1 } else { 0 })
                    .try_into()
                    .expect("String is too large"),

if the character + is not in the string you are always non-negative; and if + is in the string, the length is at least 1, so still non-negative.
test util though, so non-blocking

Suggestion:

"Unreachable."

---

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

Previously, TzahiTaub (Tzahi) wrote…

But AFAIU this is based on the OriginalSkeletonNode - so if the user of the lib missed data - it should be caught when building the original skeleton, and if we're here after it was created, we can assume the skeleton and modification list should match each other.

good point, unblocking

[TzahiTaub]

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

---

crates/committer/src/patricia_merkle_tree/test_utils.rs line 17 at r5 (raw file):

Previously, dorimedini-starkware wrote…

if the character + is not in the string you are always non-negative; and if + is in the string, the length is at least 1, so still non-negative.
test util though, so non-blocking

It's not a negativity issue; it's converting a usize into u8. If your string is longer than 255 chars, this will fail.

[Yoni-Starkware]

Reviewed 3 of 6 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/node_data/inner_node.rs line 57 at r5 (raw file):

    pub(crate) fn concat_paths(&self, other: Self) -> Self {
        Self {
            path: EdgePath((self.path.0 * Felt::TWO.pow(other.length.0)) + other.path.0),

This might be expensive (finite field)
@dorimedini-starkware, can we switch the type to U256 and use a simple bit shift here?

Code quote:

Felt::TWO.pow(other.length.0)

---

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

#[derive(Debug, PartialEq, Eq)]
/// A temporary skeleton node used to during the computation of the updated skeleton tree.

Suggestion:

n node used during the computation of the updated skeleton tree.