starknet_committer,starknet_patricia: add subtree trait

[ArielElp]

No description provided.

[reviewable-StarkWare]

This change is 

[ArielElp]

• starknet_committer,starknet_patricia: move node_serde to facts db #11644
• starknet_committer: reorganize db modules #11640
• starknet_committer: add commit e2e tests #11331
• index layout forest creation tests #11286
• starknet_committer,starknet_patricia: layout-based original tree creation tests #11236
• starknet_committer: add original tree creation test templates #11715
• starknet_committer,starknet_patricia: layout-dependent db-key separator #11641
• starknet_committer,starknet_patricia: remove FilledNode impl #11637
• starknet_committer: db layout trait #11621
• starknet_committer,starknet_patricia: layout-dependent FilledTree serialization #10988
• starknet_committer,starknet_patricia: refactor key context #11592
• starknet_committer,starknet_patricia: add layout for base trait #11566
• starknet_committer,starknet_patricia: add serialization utilities to node layout #10987
• starknet_committer,starknet_patricia: move db_layout #10986
• starknet_committer: add index db #10685
• starknet_committer: abstract original tree creation #10684
• starknet_committer: enrich layout trait to abstract skeleton creation #11182
• starknet_committer: move trie creation logic to trie_traversal #11180
• starknet_committer: group generic layout logic #10852
• starknet_committer,starknet_patricia: remove generic config #10683
• starknet_committer: add index layout nodes #10682
• starknet_committer: add index layout subtree #10919
• starknet_committer: hash leaves of both layouts #10918
• starknet_committer: add index layout leaves #10681
• starknet_committer,starknet_patricia,starknet_patricia_storage: support contract address prefix #10858
• starknet_committer,starknet_patricia: add serde errors #10810
• starknet_committer,starknet_committer_and_os_cli,starknet_patricia: layout dependent fetch_nodes #10680
• starknet_committer,starknet_patricia: enrich subtree trait to support fetch nodes #10751
• starknet_committer,starknet_committer_and_os_cli,starknet_patricia: make FilledNode generic #10750
• starknet_committer,starknet_os,starknet_patricia: make node data generic #10679
• starknet_committer,starknet_patricia: add deserialization context #10704
• starknet_committer,starknet_patricia,starknet_patricia_storage: add context to prefix and serde #10678
• starknet_committer,starknet_patricia: add subtree trait #10694 👈 (View in Graphite)
• starknet_committer: reorganize facts db files #10677
• main-v0.14.1-committer

This stack of pull requests is managed by Graphite. Learn more about stacking.

[nimrod-starkware]

@nimrod-starkware reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ArielElp)

---

crates/starknet_patricia/src/patricia_merkle_tree/traversal_test.rs line 11 at r1 (raw file):

#[derive(Debug, PartialEq)]
struct TestSubTree<'a> {

Can you think of a more informative name for that?
Seems like the test here can be changed in a separate PR

Code quote:

TestSubTree<'a>

---

crates/starknet_patricia/src/patricia_merkle_tree/traversal_test.rs line 33 at r1 (raw file):

    fn should_traverse_unmodified_children() -> bool {
        false
    }

empty lines between methods

Suggestion:

impl<'a> SubTreeTrait<'a> for TestSubTree<'a> {
    type ChildData = ();
    
    fn create_child(
        sorted_leaf_indices: SortedLeafIndices<'a>,
        root_index: NodeIndex,
        _child_data: Self::ChildData,
    ) -> Self {
        Self { sorted_leaf_indices, root_index }
    }
    
    fn get_root_index(&self) -> NodeIndex {
        self.root_index
    }
    
    fn get_sorted_leaf_indices(&self) -> &SortedLeafIndices<'a> {
        &self.sorted_leaf_indices
    }
    
    fn should_traverse_unmodified_children() -> bool {
        false
    }

---

crates/starknet_patricia/src/patricia_merkle_tree/traversal.rs line 46 at r1 (raw file):

    fn is_unmodified(&self) -> bool {
        self.get_sorted_leaf_indices().is_empty()
    }

Please add doc for the trait, for the associated type and for each method

Code quote:

pub trait SubTreeTrait<'a>: Sized {
    type ChildData: Copy;

    fn create_child(
        sorted_leaf_indices: SortedLeafIndices<'a>,
        root_index: NodeIndex,
        child_data: Self::ChildData,
    ) -> Self;
    fn get_root_index(&self) -> NodeIndex;
    fn get_sorted_leaf_indices(&self) -> &SortedLeafIndices<'a>;

    fn get_height(&self) -> SubTreeHeight {
        SubTreeHeight::new(
            SubTreeHeight::ACTUAL_HEIGHT.0 - (self.get_root_index().bit_length() - 1),
        )
    }
    fn split_leaves(&self) -> [SortedLeafIndices<'a>; 2] {
        split_leaves(&self.get_root_index(), self.get_sorted_leaf_indices())
    }
    fn is_unmodified(&self) -> bool {
        self.get_sorted_leaf_indices().is_empty()
    }

---

crates/starknet_patricia/src/patricia_merkle_tree/traversal.rs line 90 at r1 (raw file):

        self.get_root_index().is_leaf()
    }
    fn should_traverse_unmodified_children() -> bool;

Please add empty line between trait methods

Code quote:

pub trait SubTreeTrait<'a>: Sized {
    type ChildData: Copy;

    fn create_child(
        sorted_leaf_indices: SortedLeafIndices<'a>,
        root_index: NodeIndex,
        child_data: Self::ChildData,
    ) -> Self;
    fn get_root_index(&self) -> NodeIndex;
    fn get_sorted_leaf_indices(&self) -> &SortedLeafIndices<'a>;

    fn get_height(&self) -> SubTreeHeight {
        SubTreeHeight::new(
            SubTreeHeight::ACTUAL_HEIGHT.0 - (self.get_root_index().bit_length() - 1),
        )
    }
    fn split_leaves(&self) -> [SortedLeafIndices<'a>; 2] {
        split_leaves(&self.get_root_index(), self.get_sorted_leaf_indices())
    }
    fn is_unmodified(&self) -> bool {
        self.get_sorted_leaf_indices().is_empty()
    }
    /// Returns the bottom subtree which is referred from `self` by the given path. When creating
    /// the bottom subtree some indices that were modified under `self` are not modified under the
    /// bottom subtree (leaves that were previously empty). These indices are returned as well.
    fn get_bottom_subtree(
        &self,
        path_to_bottom: &PathToBottom,
        bottom_data: Self::ChildData,
    ) -> (Self, Vec<NodeIndex>) {
        let sorted_leaf_indices = self.get_sorted_leaf_indices();
        let bottom_index = path_to_bottom.bottom_index(self.get_root_index());
        let bottom_height = self.get_height() - SubTreeHeight::new(path_to_bottom.length.into());
        let leftmost_in_subtree = bottom_index << bottom_height.into();
        let rightmost_in_subtree =
            leftmost_in_subtree - NodeIndex::ROOT + (NodeIndex::ROOT << bottom_height.into());
        let leftmost_index = sorted_leaf_indices.bisect_left(&leftmost_in_subtree);
        let rightmost_index = sorted_leaf_indices.bisect_right(&rightmost_in_subtree);
        let bottom_leaves = sorted_leaf_indices.subslice(leftmost_index, rightmost_index);
        let previously_empty_leaf_indices = sorted_leaf_indices.get_indices()[..leftmost_index]
            .iter()
            .chain(sorted_leaf_indices.get_indices()[rightmost_index..].iter())
            .cloned()
            .collect();

        (
            Self::create_child(bottom_leaves, bottom_index, bottom_data),
            previously_empty_leaf_indices,
        )
    }
    fn get_children_subtrees(
        &self,
        left_data: Self::ChildData,
        right_data: Self::ChildData,
    ) -> (Self, Self) {
        let [left_leaves, right_leaves] = self.split_leaves();
        let left_root_index = self.get_root_index() * 2.into();
        (
            Self::create_child(left_leaves, left_root_index, left_data),
            Self::create_child(right_leaves, left_root_index + NodeIndex::ROOT, right_data),
        )
    }
    fn is_leaf(&self) -> bool {
        self.get_root_index().is_leaf()
    }
    fn should_traverse_unmodified_children() -> bool;

---

crates/starknet_committer/src/db/facts_db/types.rs line 32 at r1 (raw file):

        false
    }
}

new lines between methods

Suggestion:

impl<'a> SubTreeTrait<'a> for FactsSubTree<'a> {
    type ChildData = HashOutput;
    
    fn create_child(
        sorted_leaf_indices: SortedLeafIndices<'a>,
        root_index: NodeIndex,
        child_data: Self::ChildData,
    ) -> Self {
        Self { sorted_leaf_indices, root_index, root_hash: child_data }
    }
    
    fn get_root_index(&self) -> NodeIndex {
        self.root_index
    }
    
    fn get_sorted_leaf_indices(&self) -> &SortedLeafIndices<'a> {
        &self.sorted_leaf_indices
    }
    
    fn should_traverse_unmodified_children() -> bool {
        false
    }
}

[ArielElp]

Reviewable status: 4 of 7 files reviewed, 5 unresolved discussions (waiting on @nimrod-starkware)

---

crates/starknet_committer/src/db/facts_db/types.rs line 32 at r1 (raw file):

Previously, nimrod-starkware wrote…

new lines between methods

Done.

---

crates/starknet_patricia/src/patricia_merkle_tree/traversal.rs line 46 at r1 (raw file):

Previously, nimrod-starkware wrote…

Please add doc for the trait, for the associated type and for each method

Done.

---

crates/starknet_patricia/src/patricia_merkle_tree/traversal.rs line 90 at r1 (raw file):

Previously, nimrod-starkware wrote…

Please add empty line between trait methods

Done.

---

crates/starknet_patricia/src/patricia_merkle_tree/traversal_test.rs line 11 at r1 (raw file):

Previously, nimrod-starkware wrote…

Can you think of a more informative name for that?
Seems like the test here can be changed in a separate PR

The tests themselves are checking traversal, e.g. that create_bottom_subtree indeed returns the expected subtree (index-wise). It needs a subtree type to work with, I don't think the tests themselves should change.

Regarding the name, NoDataSubTree? But IMO it should be clear that this is only used for test, e.g. the response to should_traverse_unmodified_children is meaningless, these tests only check children creation.

---

crates/starknet_patricia/src/patricia_merkle_tree/traversal_test.rs line 33 at r1 (raw file):

Previously, nimrod-starkware wrote…

empty lines between methods

Done.

[yoavGrs]

@yoavGrs reviewed 4 of 7 files at r1, 2 of 3 files at r2.
Reviewable status: 3 of 7 files reviewed, 7 unresolved discussions (waiting on @ArielElp and @nimrod-starkware)

---

crates/starknet_patricia/src/patricia_merkle_tree/traversal_test.rs line 11 at r1 (raw file):

Previously, ArielElp wrote…

The tests themselves are checking traversal, e.g. that create_bottom_subtree indeed returns the expected subtree (index-wise). It needs a subtree type to work with, I don't think the tests themselves should change.

Regarding the name, NoDataSubTree? But IMO it should be clear that this is only used for test, e.g. the response to should_traverse_unmodified_children is meaningless, these tests only check children creation.

Add this explanation to a doc of the struct (it stands for testing the structure of the sub-tree only, not the data)

---

crates/starknet_patricia/src/patricia_merkle_tree/traversal.rs line 26 at r2 (raw file):

// The SubTreeTrait allows traversing a trie without knowledge of the concrete node types and data.
pub trait SubTreeTrait<'a>: Sized {

Suggestion, non-blocking.

Suggestion:

SubTreeStructureTrait

---

crates/starknet_patricia/src/patricia_merkle_tree/traversal.rs line 35 at r2 (raw file):

        root_index: NodeIndex,
        child_data: Self::ChildData,
    ) -> Self;

It's a general creation of a sub-tree, right?
Same question about the name ChildData.

Suggestion:

    // Creates a concrete child node given its index and data.
    fn create(
        sorted_leaf_indices: SortedLeafIndices<'a>,
        root_index: NodeIndex,
        child_data: Self::ChildData,
    ) -> Self;

[ArielElp]

Reviewable status: 3 of 7 files reviewed, 7 unresolved discussions (waiting on @nimrod-starkware and @yoavGrs)

---

crates/starknet_patricia/src/patricia_merkle_tree/traversal.rs line 35 at r2 (raw file):

Previously, yoavGrs wrote…

It's a general creation of a sub-tree, right?
Same question about the name ChildData.

It not analogus to xxx::new, an existing SubTree can, given data for the child, create its child.

---

crates/starknet_patricia/src/patricia_merkle_tree/traversal_test.rs line 11 at r1 (raw file):

Previously, yoavGrs wrote…

Add this explanation to a doc of the struct (it stands for testing the structure of the sub-tree only, not the data)

Added comment

[yoavGrs]

@yoavGrs reviewed 2 of 3 files at r3, 1 of 2 files at r4, 4 of 4 files at r5, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ArielElp and @nimrod-starkware)

---

crates/starknet_patricia/src/patricia_merkle_tree/traversal.rs line 35 at r2 (raw file):

Previously, ArielElp wrote…

It not analogus to xxx::new, an existing SubTree can, given data for the child, create its child.

What is this reflected in? There is no &self argument.
It indeed seems like a signature of fn new.

[yoavGrs]

@yoavGrs reviewed 4 of 6 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ArielElp)

---

crates/starknet_patricia/src/patricia_merkle_tree/traversal.rs line 29 at r6 (raw file):

/// storage layout.
pub trait SubTreeTrait<'a>: Sized {
    /// Extra data a node can carry about its children (e.g. their hashes).

Rephrase that too, please.

Code quote:

/// Extra data a node can carry about its children (e.g. their hashes).

[ArielElp]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @yoavGrs)

---

crates/starknet_patricia/src/patricia_merkle_tree/traversal.rs line 29 at r6 (raw file):

Previously, yoavGrs wrote…

Rephrase that too, please.

Done.

[yoavGrs]

@yoavGrs reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArielElp)

[dorimedini-starkware]

@dorimedini-starkware reviewed 1 of 3 files at r3, 5 of 6 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArielElp)

---

crates/starknet_patricia/src/patricia_merkle_tree/traversal_test.rs line 0 at r7 (raw file):
we will want this test to be generic - i.e. test traversal with both layouts.
to this end, I suggest you split out the body of the test into a separate function that is generic in the subtree type.
no need for TestSubTree I think - for now, you will only have one instantiation of the test, but you will have another

#[rstest]
..
fn test_get_bottom_subtree(
    #[case] height: SubTreeHeight,
    #[case] sorted_leaf_indices: &[U256],
    #[case] path_to_bottom: PathToBottom,
    #[case] expected_sorted_leaf_indices: &[U256],
    #[case] expected_root_index: U256,
) {
    test_get_bottom_subtree_body::<FactsSubTree>(height, ..);
}

---

crates/starknet_patricia/src/patricia_merkle_tree/traversal.rs line 30 at r7 (raw file):

pub trait SubTreeTrait<'a>: Sized {
    /// Extra data a node can carry (e.g. its hash).
    type NodeData: Copy;

why Copy and not Clone? Copy is a stronger constraint

Code quote:

type NodeData: Copy;

---

crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 196 at r7 (raw file):

        ));
    }
    let main_subtree = FactsSubTree { sorted_leaf_indices, root_index: NodeIndex::ROOT, root_hash };

you use create above, but construct explicitly here. why?
if there is no reason - please make at least one of the fields in FactsSubTree private, so the compiler will catch any attempt of manual construction

Code quote:

let main_subtree = FactsSubTree { sorted_leaf_indices, root_index: NodeIndex::ROOT, root_hash };

[dorimedini-starkware]

@dorimedini-starkware reviewed 4 of 4 files at r8, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArielElp)

[ArielElp]

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

---

crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 196 at r7 (raw file):

Previously, dorimedini-starkware wrote…

you use create above, but construct explicitly here. why?
if there is no reason - please make at least one of the fields in FactsSubTree private, so the compiler will catch any attempt of manual construction

Done.

---

crates/starknet_patricia/src/patricia_merkle_tree/traversal.rs line 30 at r7 (raw file):

Previously, dorimedini-starkware wrote…

why Copy and not Clone? Copy is a stronger constraint

Apparently I no longer need any constraint, probably related to code that is now gone.

---

crates/starknet_patricia/src/patricia_merkle_tree/traversal_test.rs line at r7 (raw file):

Previously, dorimedini-starkware wrote…

we will want this test to be generic - i.e. test traversal with both layouts.
to this end, I suggest you split out the body of the test into a separate function that is generic in the subtree type.
no need for TestSubTree I think - for now, you will only have one instantiation of the test, but you will have another

#[rstest]
..
fn test_get_bottom_subtree(
    #[case] height: SubTreeHeight,
    #[case] sorted_leaf_indices: &[U256],
    #[case] path_to_bottom: PathToBottom,
    #[case] expected_sorted_leaf_indices: &[U256],
    #[case] expected_root_index: U256,
) {
    test_get_bottom_subtree_body::<FactsSubTree>(height, ..);
}

This test is not layout dependent, it only uses functions implemented on the trait itself. It checks correct manipulation of indices and leaves when creating bottom/child subtrees.

[dorimedini-starkware]

@dorimedini-starkware reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArielElp)

---

crates/starknet_patricia/src/patricia_merkle_tree/traversal.rs line 30 at r7 (raw file):

Previously, ArielElp wrote…

Apparently I no longer need any constraint, probably related to code that is now gone.

so can you delete the constraint?

[dorimedini-starkware]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArielElp)

---

crates/starknet_patricia/src/patricia_merkle_tree/traversal.rs line 30 at r7 (raw file):

Previously, dorimedini-starkware wrote…

so can you delete the constraint?

ah, next PR, ok