starknet_committer,starknet_patricia: enrich subtree trait to support fetch nodes

[ArielElp]

No description provided.

[reviewable-StarkWare]

This change is 

[ArielElp]

• 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,starknet_patricia: layout-dependent FilledTree serialization #10988
• 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 👈 (View in Graphite)
• 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
• 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 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ArielElp and @yoavGrs)

---

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

    /// When should_traverse_unmodified_children is false, this function is used to get the hash
    /// of the unmodified child (in this case NodeData will be HashOutput).
    fn unmodified_child_hash(data: Self::NodeData) -> Option<HashOutput>;

Does that make sense?

Suggestion:

    /// When should_traverse_unmodified_children is false, this function is used to get the hash
    /// of the unmodified child (in this case NodeData will be HashOutput).
    fn unmodified_child_data(data: Self::NodeData) -> Option<Self::NodeData>;

---

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

    /// When should_traverse_unmodified_children is false, this function is used to get the hash
    /// of the unmodified child (in this case NodeData will be HashOutput).
    fn unmodified_child_hash(data: Self::NodeData) -> Option<HashOutput>;

These two methods are coupled.
WDYT of this:

1. Delete should_traverse_unmodified_children.
2. Change the signature to unmodified_child_data(data: Self::NodeData) -> Option<Self::NodeData>.

Where if the return value is Some(...) then it means we should traverse the children, otherwise we should not.
If that works and makes sense please update the doc

Code quote:

    /// Indicates whether unmodified children should be traversed during the construction of the
    /// original skeleton tree.
    fn should_traverse_unmodified_children() -> bool;

    /// When should_traverse_unmodified_children is false, this function is used to get the hash
    /// of the unmodified child (in this case NodeData will be HashOutput).
    fn unmodified_child_hash(data: Self::NodeData) -> Option<HashOutput>;

---

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

    ) -> DbKeyPrefix;

    fn get_root_suffix(&self) -> Vec<u8>;

please doc

Suggestion:

fn get_root_suffix(&self) -> Vec<u8>;

---

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

    // Need when deserializing the root node from a raw DbValue.
    fn get_root_context(&self) -> Self::NodeContext;

doc

Suggestion:

    /// Returns [Self::NodeContext] needed to deserialize the root node from a raw [DbValue].
    fn get_root_context(&self) -> Self::NodeContext;

---

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

    // Need when deserializing the root node from a raw DbValue.
    fn get_root_context(&self) -> Self::NodeContext;

Can this be static?

Suggestion:

fn get_root_context() -> Self::NodeContext;

[nimrod-starkware]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ArielElp and @yoavGrs)

---

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

    /// Extra data a node can carry (e.g. its hash).
    type NodeData: Copy;
    type NodeContext;

doc plz

Suggestion:

/// ....
type NodeContext;

---

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

}

pub type FactDbFilledNode<L> = FilledNode<L, HashOutput>;

see comment in prev PR, keep this def in delete the others please

Code quote:

pub type FactDbFilledNode<L> = FilledNode<L, HashOutput>;

[dorimedini-starkware]

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

---

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

    // tree node.
    Traverse,
    // Indicates that the child should be skipped as its unmodified and we have its hash.

not sure about the second its
but it is == it's for sure

Suggestion:

as it's unmodified and we have its hash

[dorimedini-starkware]

@dorimedini-starkware 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 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArielElp)

---

crates/starknet_patricia/src/patricia_merkle_tree/traversal.rs line 27 at r8 (raw file):

/// An enum that specifies how to treat unmodified children during the construction of the
/// `OriginalSkeletonTree`.

failed the doctest?
why note like this?

Suggestion:

[crate::patricia_merkle_tree::original_skeleton_tree::tree::OriginalSkeletonTree].

[ArielElp]

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

---

crates/starknet_patricia/src/patricia_merkle_tree/traversal.rs line 27 at r8 (raw file):

Previously, dorimedini-starkware wrote…

failed the doctest?
why note like this?

Yep, it just seemed too long, changed

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

[dorimedini-starkware]

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