feat(skeleton): add_node_from_binary_data

crates/committer/src/patricia_merkle_tree/original_skeleton_tree/node.rs

@@ -3,7 +3,7 @@ use crate::patricia_merkle_tree::node_data::inner_node::{EdgeData, PathToBottom}
 use crate::patricia_merkle_tree::node_data::leaf::SkeletonLeaf;
 
 #[allow(dead_code)]
-#[derive(Debug, PartialEq, Eq)]
+#[derive(Copy, Clone, Debug, PartialEq, Eq)]
 /// A node in the structure of a Patricia-Merkle tree, before the update.
 pub(crate) enum OriginalSkeletonNode {
     Binary,

crates/committer/src/patricia_merkle_tree/test_utils.rs

@@ -3,6 +3,17 @@ use rand::Rng;
 use rstest::rstest;
 
 use crate::patricia_merkle_tree::node_data::inner_node::{EdgePathLength, PathToBottom};
+use crate::patricia_merkle_tree::node_data::leaf::SkeletonLeaf;
+
+impl From<u8> for SkeletonLeaf {
+    fn from(value: u8) -> Self {
+        if value == 0 {
+            Self::Zero
+        } else {
+            Self::NonZero
+        }
+    }
+}
 
 impl From<&str> for PathToBottom {
     fn from(value: &str) -> Self {
@@ -18,6 +29,7 @@ impl From<&str> for PathToBottom {
         }
     }
 }
+
 /// Generates a random U256 number between low and high (exclusive).
 /// Panics if low > high.
 pub(crate) fn get_random_u256(low: U256, high: U256) -> U256 {

crates/committer/src/patricia_merkle_tree/types.rs

@@ -67,6 +67,11 @@ impl NodeIndex {
         (index << length.0) + Self::new(path.into())
     }
 
+    pub(crate) fn get_children_indices(&self) -> [Self; 2] {
+        let left_child = *self << 1;
+        [left_child, left_child + 1.into()]
+    }
+
     /// Returns the number of leading zeroes when represented with Self::BITS bits.
     pub(crate) fn leading_zeros(&self) -> u8 {
         (self.0.leading_zeros() - (U256::BITS - u32::from(Self::BITS)))

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree.rs

@@ -1,7 +1,5 @@
 use crate::patricia_merkle_tree::node_data::inner_node::EdgeData;
 use crate::patricia_merkle_tree::node_data::inner_node::PathToBottom;
-use crate::patricia_merkle_tree::node_data::leaf::LeafModifications;
-use crate::patricia_merkle_tree::node_data::leaf::SkeletonLeaf;
 use crate::patricia_merkle_tree::original_skeleton_tree::node::OriginalSkeletonNode;
 use crate::patricia_merkle_tree::original_skeleton_tree::tree::OriginalSkeletonTreeImpl;
 use crate::patricia_merkle_tree::original_skeleton_tree::utils::split_leaves;
@@ -21,6 +19,12 @@ enum TempSkeletonNode {
     Original(OriginalSkeletonNode),
 }
 
+impl TempSkeletonNode {
+    fn is_empty(&self) -> bool {
+        *self == Self::Empty
+    }
+}
+
 impl OriginalSkeletonTreeImpl {
     #[allow(dead_code)]
     /// Returns the path from the given root_index to the LCA of the leaves. Assumes the leaves are:
@@ -55,53 +59,109 @@ impl OriginalSkeletonTreeImpl {
 
 impl UpdatedSkeletonTreeImpl {
     #[allow(dead_code)]
-    /// 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.
+    /// 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.
+    fn node_from_binary_data(
+        &mut self,
+        root_index: &NodeIndex,
+        left: &TempSkeletonNode,
+        right: &TempSkeletonNode,
+    ) -> TempSkeletonNode {
+        let [left_index, right_index] = root_index.get_children_indices();
+
+        if !left.is_empty() && !right.is_empty() {
+            // Both children are non-empty - a binary node.
+            // Finalize children, as a binary node cannot change form.
+            for (index, node) in [(left_index, left), (right_index, right)] {
+                let TempSkeletonNode::Original(original_node) = node else {
+                    unreachable!("Unexpected empty node.");
+                };
+                let updated = match original_node {
+                    // Leaf is finalized upon updated skeleton creation.
+                    OriginalSkeletonNode::Leaf(_) => continue,
+                    OriginalSkeletonNode::Binary => UpdatedSkeletonNode::Binary,
+                    OriginalSkeletonNode::Edge { path_to_bottom } => UpdatedSkeletonNode::Edge {
+                        path_to_bottom: *path_to_bottom,
+                    },
+                    OriginalSkeletonNode::LeafOrBinarySibling(hash) => {
+                        UpdatedSkeletonNode::Sibling(*hash)
+                    }
+                    OriginalSkeletonNode::EdgeSibling(EdgeData {
+                        path_to_bottom,
+                        bottom_hash,
+                    }) => {
+                        // Finalize bottom to allow the edge hash computation.
+                        // TODO(Tzahi, 1/6/2024): Consider moving this to the create function, or
+                        // even to the OriginalSkeleton creation.
+                        self.skeleton_tree.insert(
+                            path_to_bottom.bottom_index(index),
+                            UpdatedSkeletonNode::Sibling(*bottom_hash),
+                        );
+                        UpdatedSkeletonNode::Edge {
+                            path_to_bottom: *path_to_bottom,
+                        }
+                    }
+                };
+                self.skeleton_tree.insert(index, updated);
+            }
+
+            return TempSkeletonNode::Original(OriginalSkeletonNode::Binary);
+        }
+
+        // At least one of the children is empty.
+        let (child_node, child_index, child_direction) = if *right == TempSkeletonNode::Empty {
+            (left, left_index, PathToBottom::LEFT_CHILD)
+        } else {
+            (right, right_index, PathToBottom::RIGHT_CHILD)
+        };
+        self.node_from_edge_data(&child_direction, &child_index, child_node)
+    }
+
+    /// Builds a (probably edge) node from its given updated 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.
     fn node_from_edge_data(
         &mut self,
         path: &PathToBottom,
         bottom_index: &NodeIndex,
         bottom: &TempSkeletonNode,
-        leaf_modifications: &LeafModifications<SkeletonLeaf>,
     ) -> TempSkeletonNode {
-        TempSkeletonNode::Original(match bottom {
-            TempSkeletonNode::Empty => return TempSkeletonNode::Empty,
-            TempSkeletonNode::Original(OriginalSkeletonNode::Leaf(_)) => {
-                let leaf = leaf_modifications
-                    .get(bottom_index)
-                    .unwrap_or_else(|| panic!("Leaf modification {bottom_index:?} not found"));
-                if leaf.is_zero() {
-                    return TempSkeletonNode::Empty;
-                };
+        let TempSkeletonNode::Original(original_node) = bottom else {
+            return TempSkeletonNode::Empty;
+        };
+        TempSkeletonNode::Original(match original_node {
+            OriginalSkeletonNode::Leaf(_) => {
+                // Leaf is finalized upon updated skeleton creation.
+                // bottom_index is in the updated 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."
+                );
                 OriginalSkeletonNode::Edge {
                     path_to_bottom: *path,
                 }
             }
-            TempSkeletonNode::Original(OriginalSkeletonNode::Edge { path_to_bottom }) => {
-                OriginalSkeletonNode::Edge {
-                    path_to_bottom: path.concat_paths(*path_to_bottom),
-                }
-            }
-            TempSkeletonNode::Original(OriginalSkeletonNode::EdgeSibling(edge_data)) => {
+            OriginalSkeletonNode::Edge { path_to_bottom } => OriginalSkeletonNode::Edge {
+                path_to_bottom: path.concat_paths(*path_to_bottom),
+            },
+            OriginalSkeletonNode::EdgeSibling(edge_data) => {
                 OriginalSkeletonNode::EdgeSibling(EdgeData {
                     bottom_hash: edge_data.bottom_hash,
                     path_to_bottom: path.concat_paths(edge_data.path_to_bottom),
                 })
             }
-            TempSkeletonNode::Original(OriginalSkeletonNode::Binary) => {
+            OriginalSkeletonNode::Binary => {
                 // Finalize bottom - a binary descendant cannot change form.
                 self.skeleton_tree
                     .insert(*bottom_index, UpdatedSkeletonNode::Binary);
                 OriginalSkeletonNode::Edge {
                     path_to_bottom: *path,
                 }
             }
-            TempSkeletonNode::Original(OriginalSkeletonNode::LeafOrBinarySibling(_)) => {
-                OriginalSkeletonNode::Edge {
-                    path_to_bottom: *path,
-                }
-            }
+            OriginalSkeletonNode::LeafOrBinarySibling(_) => OriginalSkeletonNode::Edge {
+                path_to_bottom: *path,
+            },
         })
     }
 }

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/compute_updated_skeleton_tree_test.rs

@@ -22,15 +22,19 @@ fn empty_skeleton(height: u8) -> OriginalSkeletonTreeImpl {
 }
 
 #[fixture]
-fn updated_skeleton(
-    #[default(&[])] leaf_modifications: &[(NodeIndex, SkeletonLeaf)],
-) -> UpdatedSkeletonTreeImpl {
-    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 }
+fn updated_skeleton(#[default(&[])] leaf_modifications: &[(u128, u8)]) -> UpdatedSkeletonTreeImpl {
+    UpdatedSkeletonTreeImpl {
+        skeleton_tree: leaf_modifications
+            .iter()
+            .filter(|(_, leaf_val)| *leaf_val != 0)
+            .map(|(index, leaf_val)| {
+                (
+                    NodeIndex::from(*index),
+                    UpdatedSkeletonNode::Leaf(SkeletonLeaf::from(*leaf_val)),
+                )
+            })
+            .collect(),
+    }
 }
 
 #[rstest]
@@ -116,6 +120,81 @@ fn test_get_path_to_lca(
     );
 }
 
+#[rstest]
+#[case::two_deleted_leaves(
+    &NodeIndex::from(1),
+    &TempSkeletonNode::Empty,
+    &TempSkeletonNode::Empty,
+    &[(2,0), (3,0)],
+    TempSkeletonNode::Empty,
+    &[]
+)]
+#[case::one_deleted_leaf(
+    &NodeIndex::from(1),
+    &TempSkeletonNode::Original(OriginalSkeletonNode::Leaf(SkeletonLeaf::NonZero)),
+    &TempSkeletonNode::Empty,
+    &[(2, 1), (3, 0)],
+    TempSkeletonNode::Original(
+        OriginalSkeletonNode::Edge {path_to_bottom: PathToBottom::LEFT_CHILD}
+    ),
+    &[]
+)]
+#[case::two_leaves(
+    &NodeIndex::from(5),
+    &TempSkeletonNode::Original(OriginalSkeletonNode::Leaf(SkeletonLeaf::NonZero)),
+    &TempSkeletonNode::Original(OriginalSkeletonNode::Leaf(SkeletonLeaf::NonZero)),
+    &[(10,1), (11,1)],
+    TempSkeletonNode::Original(OriginalSkeletonNode::Binary),
+    &[]
+)]
+#[case::two_nodes(
+    &NodeIndex::from(5),
+    &TempSkeletonNode::Original(OriginalSkeletonNode::Binary),
+    &TempSkeletonNode::Original(OriginalSkeletonNode::Binary),
+    &[],
+    TempSkeletonNode::Original(OriginalSkeletonNode::Binary),
+    &[(NodeIndex::from(10),UpdatedSkeletonNode::Binary), (NodeIndex::from(11), UpdatedSkeletonNode::Binary)]
+)]
+#[case::deleted_left_child(
+    &NodeIndex::from(5),
+    &TempSkeletonNode::Empty,
+    &TempSkeletonNode::Original(OriginalSkeletonNode::Binary),
+    &[(20, 0)],
+    TempSkeletonNode::Original(OriginalSkeletonNode::Edge { path_to_bottom: PathToBottom::RIGHT_CHILD }),
+    &[(NodeIndex::from(11),UpdatedSkeletonNode::Binary)]
+)]
+#[case::deleted_two_children(
+    &NodeIndex::from(5),
+    &TempSkeletonNode::Empty,
+    &TempSkeletonNode::Empty,
+    &[(20, 0), (22, 0)],
+    TempSkeletonNode::Empty,
+    &[]
+)]
+#[case::left_edge_right_deleted(
+    &NodeIndex::from(5),
+    &TempSkeletonNode::Original(OriginalSkeletonNode::Edge { path_to_bottom: PathToBottom::RIGHT_CHILD }),
+    &TempSkeletonNode::Empty,
+    &[(22, 0)],
+    TempSkeletonNode::Original(OriginalSkeletonNode::Edge { path_to_bottom: PathToBottom::from("01") }),
+    &[]
+)]
+fn test_node_from_binary_data(
+    #[case] root_index: &NodeIndex,
+    #[case] left: &TempSkeletonNode,
+    #[case] right: &TempSkeletonNode,
+    #[case] _leaf_modifications: &[(u128, u8)],
+    #[case] expected_node: TempSkeletonNode,
+    #[case] expected_skeleton_additions: &[(NodeIndex, UpdatedSkeletonNode)],
+    #[with(_leaf_modifications)] mut updated_skeleton: UpdatedSkeletonTreeImpl,
+) {
+    let mut expected_skeleton_tree = updated_skeleton.skeleton_tree.clone();
+    expected_skeleton_tree.extend(expected_skeleton_additions.iter().cloned());
+    let temp_node = updated_skeleton.node_from_binary_data(root_index, left, right);
+    assert_eq!(temp_node, expected_node);
+    assert_eq!(updated_skeleton.skeleton_tree, expected_skeleton_tree);
+}
+
 #[rstest]
 #[case::to_empty(
     &PathToBottom::LEFT_CHILD,
@@ -159,19 +238,11 @@ fn test_get_path_to_lca(
     ),
     &[(NodeIndex::from(7), UpdatedSkeletonNode::Binary)]
 )]
-#[case::to_empty_leaf(
-    &PathToBottom::RIGHT_CHILD,
-    &NodeIndex::from(7),
-    &TempSkeletonNode::Original(OriginalSkeletonNode::Leaf(SkeletonLeaf::NonZero)),
-    &[(NodeIndex::from(7),SkeletonLeaf::Zero)],
-    TempSkeletonNode::Empty,
-    &[],
-)]
 #[case::to_non_empty_leaf(
     &PathToBottom::RIGHT_CHILD,
     &NodeIndex::from(7),
     &TempSkeletonNode::Original(OriginalSkeletonNode::Leaf(SkeletonLeaf::NonZero)),
-    &[(NodeIndex::from(7), SkeletonLeaf::NonZero)],
+    &[(7, 1)],
     TempSkeletonNode::Original(
         OriginalSkeletonNode::Edge {path_to_bottom: PathToBottom::RIGHT_CHILD}
     ),
@@ -181,17 +252,14 @@ fn test_node_from_edge_data(
     #[case] path: &PathToBottom,
     #[case] bottom_index: &NodeIndex,
     #[case] bottom: &TempSkeletonNode,
-    #[case] leaf_modifications: &[(NodeIndex, SkeletonLeaf)],
+    #[case] _leaf_modifications: &[(u128, u8)],
     #[case] expected_node: TempSkeletonNode,
     #[case] expected_skeleton_additions: &[(NodeIndex, UpdatedSkeletonNode)],
-    #[with(leaf_modifications)] mut updated_skeleton: UpdatedSkeletonTreeImpl,
+    #[with(_leaf_modifications)] mut updated_skeleton: UpdatedSkeletonTreeImpl,
 ) {
     let mut expected_skeleton_tree = updated_skeleton.skeleton_tree.clone();
     expected_skeleton_tree.extend(expected_skeleton_additions.iter().cloned());
-    let leaf_modifications: HashMap<NodeIndex, SkeletonLeaf> =
-        leaf_modifications.iter().cloned().collect();
-    let temp_node =
-        updated_skeleton.node_from_edge_data(path, bottom_index, bottom, &leaf_modifications);
+    let temp_node = updated_skeleton.node_from_edge_data(path, bottom_index, bottom);
     assert_eq!(temp_node, expected_node);
     assert_eq!(updated_skeleton.skeleton_tree, expected_skeleton_tree);
 }

crates/committer/src/patricia_merkle_tree/updated_skeleton_tree/node.rs

@@ -10,5 +10,7 @@ pub(crate) enum UpdatedSkeletonNode {
     Edge { path_to_bottom: PathToBottom },
     // All unmodified nodes on the merkle paths of modified leaves.
     Sibling(HashOutput),
+    // TODO(Tzahi, 1/6/2024): Remove the internal data - a leaf in the UpdatedSkeletonTree must be
+    // NonZero.
     Leaf(SkeletonLeaf),
 }