starknet_committer,starknet_patricia: add subtree trait

[ArielElp]

No description provided.

[reviewable-StarkWare]

This change is 

[ArielElp]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• starknet_committer: add index db #10685
• starknet_committer: abstract original tree creation #10684
• starknet_committer,starknet_patricia: remove generic config #10683
• starknet_committer: add index layout nodes #10682
• starknet_committer: add index layout leaves #10681
• starknet_committer,starknet_committer_and_os_cli,starknet_patricia: make fetch_nodes layout dependent #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 and introduce the FactsFilledNode alias #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.

[nimrod-starkware]

@nimrod-starkware reviewed all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @ArielElp)

---

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

pub type TraversalResult<T> = Result<T, TraversalError>;

// The SubTreeTrait allows traversing a trie without knowledge of the concrete node types and data.

Docs are with triple slashes, please apply for all methods

Suggestion:

/// The SubTreeTrait allows traversing a trie without knowledge of the concrete node types and data.

---

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

pub type TraversalResult<T> = Result<T, TraversalError>;

// The SubTreeTrait allows traversing a trie without knowledge of the concrete node types and data.

Suggestion:

/// Trait that allows traversing a trie without knowledge of the concrete node types, data and storage layout.

---

crates/starknet_patricia/src/patricia_merkle_tree/traversal.rs line 28 at r5 (raw file):

// The SubTreeTrait allows traversing a trie without knowledge of the concrete node types and data.
pub trait SubTreeTrait<'a>: Sized {
    // A node can carry data about its children (e.g. their hashes).

Suggestion:

/// Extra data a node can carry for it's children (e.g their hashes).

---

crates/starknet_patricia/src/patricia_merkle_tree/traversal.rs line 103 at r5 (raw file):

    // Indicates whether unmodified children should be traversed during the construction of the
    // skeleton tree.

Suggestion:

    /// Indicates whether unmodified children should be traversed during the construction of the
    /// original skeleton tree.

---

crates/starknet_patricia/src/patricia_merkle_tree/traversal.rs line 106 at r5 (raw file):

    fn should_traverse_unmodified_children() -> bool;

    // Returns the db key prefix of the root node.

Suggestion:

/// Returns the [DbKeyPrefix] of the root node.

---

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

use crate::patricia_merkle_tree::types::{NodeIndex, SortedLeafIndices, SubTreeHeight};

// A test implementation of the SubTreeTrait. It should only be used for chilld creation purposes,

typo

Suggestion:

child

---

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

// A test implementation of the SubTreeTrait. It should only be used for chilld creation purposes,
// e.g. by using get_bottom_subtree or get_children_subtrees.

Suggestion:

/// Test implementation for [SubTreeTrait]. It should only be used for chilld creation purposes,
/// e.g. by using `get_bottom_subtree` or `get_children_subtrees`.

---

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

    fn get_root_prefix<L: Leaf>(&self) -> DbKeyPrefix {
        DbKeyPrefix::new(&[0])
    }

This prefix is dummy right?
If so, add a comment that's it's dummy and explain why it's ok to use dummy prefixes in this impl.

Suggestion:

    fn get_root_prefix<L: Leaf>(&self) -> DbKeyPrefix {
        DbKeyPrefix::new(&[0]) // 
    }

[ArielElp]

Reviewable status: 1 of 7 files reviewed, 10 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…

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

Changed to create

---

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

Previously, nimrod-starkware wrote…

Docs are with triple slashes, please apply for all methods

Done.

---

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

Previously, nimrod-starkware wrote…

This prefix is dummy right?
If so, add a comment that's it's dummy and explain why it's ok to use dummy prefixes in this impl.

Done.

---

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

// A test implementation of the SubTreeTrait. It should only be used for chilld creation purposes,
// e.g. by using get_bottom_subtree or get_children_subtrees.

Done.

---

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

pub type TraversalResult<T> = Result<T, TraversalError>;

// The SubTreeTrait allows traversing a trie without knowledge of the concrete node types and data.

Done.

---

crates/starknet_patricia/src/patricia_merkle_tree/traversal.rs line 28 at r5 (raw file):

// The SubTreeTrait allows traversing a trie without knowledge of the concrete node types and data.
pub trait SubTreeTrait<'a>: Sized {
    // A node can carry data about its children (e.g. their hashes).

Done.

---

crates/starknet_patricia/src/patricia_merkle_tree/traversal.rs line 103 at r5 (raw file):

    // Indicates whether unmodified children should be traversed during the construction of the
    // skeleton tree.

Done.

---

crates/starknet_patricia/src/patricia_merkle_tree/traversal.rs line 106 at r5 (raw file):

    fn should_traverse_unmodified_children() -> bool;

    // Returns the db key prefix of the root node.

Done.

[nimrod-starkware]

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

[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)