starknet_committer,starknet_os,starknet_patricia: make node data generic

[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
• starknet_committer: 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 👈 (View in Graphite)
• 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
• 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.

[yoavGrs]

@yoavGrs reviewed 16 of 16 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ArielElp)

---

crates/starknet_patricia/src/patricia_merkle_tree/filled_tree/node.rs line 8 at r1 (raw file):

#[derive(Clone, Debug, PartialEq, Eq)]
/// A node in a Patricia-Merkle tree, complete with its hash and data.
pub struct FilledNode<L: Leaf> {

I'm guessing that this will be the next step here, right?

Suggestion:

pub struct FilledNode<L: Leaf, HashOutput>

---

crates/starknet_patricia/src/patricia_merkle_tree/updated_skeleton_tree/hash_function.rs line 23 at r1 (raw file):

    /// Computes the hash for the given node data.
    fn compute_node_hash(node_data: &NodeData<L, HashOutput>) -> HashOutput;

Same question in this file.

Suggestion:

 fn compute_node_hash<T>(node_data: &NodeData<L, T>) -> HashOutput;

---

crates/starknet_patricia/src/patricia_merkle_tree/internal_test_utils.rs line 27 at r1 (raw file):

    }

    fn compute_node_hash(node_data: &NodeData<MockLeaf, HashOutput>) -> HashOutput {

Same question

Suggestion:

 fn compute_node_hash<T>(node_data: &NodeData<MockLeaf, T>) -> HashOutput {

---

crates/starknet_committer/src/hash_function/hash.rs line 54 at r1 (raw file):

        ))
    }
    fn compute_node_hash(node_data: &NodeData<ContractState, HashOutput>) -> HashOutput {

Why does it work only for HashOutput type?
Same question below.

Suggestion:

fn compute_node_hash<T>(node_data: &NodeData<ContractState, T>) -> HashOutput

---

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

#[cfg_attr(any(test, feature = "testing"), derive(strum_macros::EnumDiscriminants))]
#[cfg_attr(any(test, feature = "testing"), strum_discriminants(derive(strum_macros::EnumIter)))]
// A Patricia-Merkle tree node's data, i.e., the pre-image of its hash.

Is it correct?

Code quote:

// A Patricia-Merkle tree node's data, i.e., the pre-image of its hash.

---

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

        vec![self.left_data.0, self.right_data.0]
    }
}

Are you going to move it under facts_db?
If it is not in the next PR, please add TODO.

Code quote:

impl BinaryData<HashOutput> {
    pub fn flatten(&self) -> Vec<Felt> {
        vec![self.left_data.0, self.right_data.0]
    }
}

---

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

        ]
    }
}

Same question.

Code quote:

impl EdgeData<HashOutput> {
    pub fn flatten(&self) -> Vec<Felt> {
        vec![
            self.path_to_bottom.length.into(),
            (&self.path_to_bottom.path).into(),
            self.bottom_data.0,
        ]
    }
}

---

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

#[derive(Clone, Debug, PartialEq)]
pub enum Preimage {

Is it relevant for both layers?
If not, please add a TODO for moving it under facts_db.

Code quote:

pub enum Preimage {

[nimrod-starkware]

@nimrod-starkware reviewed 2 of 3 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArielElp)

[dorimedini-starkware]

@dorimedini-starkware reviewed 11 of 16 files at r1, 1 of 1 files at r2, 1 of 5 files at r3, 1 of 3 files at r4, 1 of 1 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ArielElp)

---

crates/starknet_committer/src/db/facts_db/traversal.rs line 124 at r6 (raw file):

                    if !right_subtree.is_unmodified() {
                        next_subtrees.push(right_subtree);
                    }

what is this change?
we really do want to only conditionally add these subtrees!

Code quote:

                    if !left_subtree.is_unmodified() {
                        next_subtrees.push(left_subtree);
                    }
                    if !right_subtree.is_unmodified() {
                        next_subtrees.push(right_subtree);
                    }

---

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

    fn compute_node_hash_with_inner_hash_function<H: HashFunction>(
        node_data: &NodeData<L, HashOutput>,
    ) -> HashOutput {

will you be changing the signatures here?
how will we compute that hash of NodeData instances in index layout?

Code quote:

    fn compute_node_hash(node_data: &NodeData<L, HashOutput>) -> HashOutput;

    /// The default implementation for internal nodes is based on the following reference:
    /// <https://docs.starknet.io/documentation/architecture_and_concepts/Network_Architecture/starknet-state/#trie_construction>
    fn compute_node_hash_with_inner_hash_function<H: HashFunction>(
        node_data: &NodeData<L, HashOutput>,
    ) -> HashOutput {

---

crates/starknet_patricia/src/patricia_merkle_tree/node_data/inner_node.rs line 39 at r6 (raw file):

        vec![self.left_data.0, self.right_data.0]
    }
}

WDYT of this type of thing?
will it be useful for other ChildData implementations?

Suggestion:

pub trait Flatten {
    fn flatten(&self) -> Vec<Felt>;
}

#[derive(Clone, Debug, PartialEq, Eq)]
#[cfg_attr(any(test, feature = "testing"), derive(strum_macros::EnumDiscriminants))]
#[cfg_attr(any(test, feature = "testing"), strum_discriminants(derive(strum_macros::EnumIter)))]
// A Patricia-Merkle tree node's data.
pub enum NodeData<L: Leaf, ChildData: Flatten> {
    Binary(BinaryData<ChildData>),
    Edge(EdgeData<ChildData>),
    Leaf(L),
}

#[derive(Clone, Debug, Default, PartialEq, Eq)]
pub struct BinaryData<ChildData: Flatten> {
    pub left_data: ChildData,
    pub right_data: ChildData,
}

impl<C: ChildData> Flatten for BinaryData<C> {
    pub fn flatten(&self) -> Vec<Felt> {
        [self.left_data.flatten(), self.right_data.flatten()].concat()
    }
}

---

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

            match filled_root.data {
                // Binary node.
                NodeData::<L, HashOutput>::Binary(BinaryData { left_data, right_data }) => {

is the turbofish required (here and below)? can't the compiler infer the ChildData type?

Code quote:

NodeData::<L, HashOutput>::Binary

[ArielElp]

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

---

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

Previously, dorimedini-starkware wrote…

is the turbofish required (here and below)? can't the compiler infer the ChildData type?

Not needed indeed, removed.

---

crates/starknet_committer/src/db/facts_db/traversal.rs line 124 at r6 (raw file):

Previously, dorimedini-starkware wrote…

what is this change?
we really do want to only conditionally add these subtrees!

Good catch. Not intentional, best guess is that at one point I thought I was editing fetch_nodes but was actually editing this function instead.

---

crates/starknet_patricia/src/patricia_merkle_tree/node_data/inner_node.rs line 39 at r6 (raw file):

Previously, dorimedini-starkware wrote…

WDYT of this type of thing?
will it be useful for other ChildData implementations?

Probably won't be useful for other child data, it's used in preimage which needs children hashes.

Note this related todo on preimage:

// TODO(Ariel): Move Preimage to the fact_db module in starknet\committer (add a flatten
// trait to be implemented in starknet_committer for BinaryData and EdgeData).

(in the TODO, the trait is only introduced so that I can implement it in starknet_committer on the foreign NodeData type)

---

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

Previously, dorimedini-starkware wrote…

will you be changing the signatures here?
how will we compute that hash of NodeData instances in index layout?

The signature works for index layout too. The trickery going on is that NodeData is used with a layout-dependent ChildData during the construction of the original skeleton, but when computing the filled tree, we always use HashOutput. You compute the node data there, get hash output of right and left, and construct the node data of the current node before feeding it to the hasher.

[dorimedini-starkware]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ArielElp)

---

crates/starknet_committer/src/db/facts_db/traversal.rs line 124 at r6 (raw file):

Previously, ArielElp wrote…

Good catch. Not intentional, best guess is that at one point I thought I was editing fetch_nodes but was actually editing this function instead.

please apply the change in the correct PR (here, not here :) )

[dorimedini-starkware]

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

---

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

Previously, ArielElp wrote…

Not needed indeed, removed.

the fix is in the next PR but nvm, not a problem (just style)

---

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

Previously, ArielElp wrote…

The signature works for index layout too. The trickery going on is that NodeData is used with a layout-dependent ChildData during the construction of the original skeleton, but when computing the filled tree, we always use HashOutput. You compute the node data there, get hash output of right and left, and construct the node data of the current node before feeding it to the hasher.

can you add a remark in the docstring about this? it's confusing that this case is layout-independent

[ArielElp]

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

---

crates/starknet_committer/src/db/facts_db/traversal.rs line 124 at r6 (raw file):

Previously, dorimedini-starkware wrote…

please apply the change in the correct PR (here, not here :) )

ups, done

---

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

Previously, dorimedini-starkware wrote…

can you add a remark in the docstring about this? it's confusing that this case is layout-independent

Added, WDYT?

[dorimedini-starkware]

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

---

crates/starknet_patricia/src/patricia_merkle_tree/updated_skeleton_tree/hash_function.rs line 28 at r7 (raw file):

    /// <https://docs.starknet.io/documentation/architecture_and_concepts/Network_Architecture/starknet-state/#trie_construction>
    /// This code is layout independent. After computing the children hashes, NodeData can be
    /// insantiated with the concrete HashOutput for ChildData, regardless of what's stored in the

typo
looks good otherwise

Suggestion:

instantiated

[dorimedini-starkware]

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

[dorimedini-starkware]

@dorimedini-starkware reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArielElp)