starknet_committer,starknet_os,starknet_patricia: make node data generic

[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 👈 (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 {

[ArielElp]

Reviewable status: 15 of 16 files reviewed, 8 unresolved discussions (waiting on @yoavGrs)

---

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

Previously, yoavGrs wrote…

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

The hashing logic assumes a node carries the hash of its children, otherwise it can't compute the hash of the node. Even if this were generic you'd need some way to do T --> Hash.

This isn't really layout-dependent code.

---

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

Previously, yoavGrs wrote…

Same question

See above

---

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

Previously, yoavGrs wrote…

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

Sort of, the next step is to make it generic, FilledNode<L, ChildData>

---

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

Previously, yoavGrs wrote…

Is it correct?

The second part is no longer correct, removing

---

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

Previously, yoavGrs wrote…

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

ATM it's still there. There's some issues with moving it:

1. It's used in Preimage and in the starknet_os crate. That's not really a blocker since we can move the entire Preimage logic, and starknet_os already depends on the committer
2. We need BinaryData next to NodeData, so we'll need to add a flatten trait in starknet_patricia and make Preimage work on things that implement this trait (we can't impl directly for the type since the type is foreign in starknet_committer)

---

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

Previously, yoavGrs wrote…

Same question.

See above

---

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

Previously, yoavGrs wrote…

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

Nope, we can move it modulo slight refactor to preimage. Added TODO

---

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

Previously, yoavGrs wrote…

Same question in this file.

See above

[nimrod-starkware]

@nimrod-starkware reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @yoavGrs)