starknet_committer,starknet_patricia: layout-dependent FilledTree serialization

[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 👈 (View in Graphite)
• 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
• 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 9 files and all commit messages, made 2 comments, and resolved 5 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArielElp and @nimrod-starkware).

---

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

/// While the same underlying type as `FactDbFilledNode`, this alias is not used in the context of
/// the DB-represnentation of the node.
pub type HashFilledNode<L> = FilledNode<L, HashOutput>;

Consider

Suggestion:

/// A node in an updated trie, where all the hashes were commputed. Used in the `FilledTree` trait.
///
/// While the same underlying type as `FactDbFilledNode`, this alias is not used in the context of
/// the DB-represnentation of the node.
pub type HashFilledNode<L> = FilledNode<L, HashOutput>;

pub type FactDbFilledNode<L> = HashFilledNode<L>;

[nimrod-starkware]

@nimrod-starkware made 2 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArielElp).

---

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

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

/// A node in an updated trie, where all the hashes were commputed. Used in the `FilledTree` trait.

In our terminology updated can be updated skeleton and can confuse.

Suggestion:

/// A node in a filled trie, where all the hashes were commputed. Used in the `FilledTree` trait.

---

crates/starknet_committer/src/db/external_test_utils.rs line 43 at r6 (raw file):

        sorted_leaf_indices,
        &config,
        &leaf_modifications.iter().map(|(k, v)| (*k, v.clone().into())).collect(),

works?

Suggestion:

&leaf_modifications.into_iter().map(|(k, v)| (k, v.into())).collect(),

[ArielElp]

@ArielElp made 2 comments.
Reviewable status: 8 of 9 files reviewed, 3 unresolved discussions (waiting on @nimrod-starkware and @yoavGrs).

---

crates/starknet_committer/src/db/external_test_utils.rs line 43 at r6 (raw file):

Previously, nimrod-starkware wrote…

works?

Here yes but it consumes leaf_modifications which is needed later.

---

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

Previously, nimrod-starkware wrote…

In our terminology updated can be updated skeleton and can confuse.

Done.

[nimrod-starkware]

@nimrod-starkware made 1 comment and resolved 2 discussions.
Reviewable status: 8 of 9 files reviewed, 1 unresolved discussion (waiting on @ArielElp and @yoavGrs).

[ArielElp]

@ArielElp resolved 1 discussion.
Reviewable status: 8 of 9 files reviewed, all discussions resolved (waiting on @dorimedini-starkware and @yoavGrs).

[dorimedini-starkware]

@dorimedini-starkware reviewed 9 files and all commit messages, and made 6 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArielElp).

---

crates/starknet_patricia/src/patricia_merkle_tree/filled_tree/tree.rs line 21 at r8 (raw file):

use crate::patricia_merkle_tree::updated_skeleton_tree::node::UpdatedSkeletonNode;
use crate::patricia_merkle_tree::updated_skeleton_tree::tree::UpdatedSkeletonTree;
#[cfg(test)]

revert line deletion

Suggestion:

use crate::patricia_merkle_tree::updated_skeleton_tree::tree::UpdatedSkeletonTree;

#[cfg(test)]

---

crates/starknet_patricia/src/patricia_merkle_tree/filled_tree/tree.rs line 57 at r8 (raw file):

pub struct FilledTreeImpl<L>
where
    L: Leaf,

why this diff?

Code quote:

pub struct FilledTreeImpl<L>
where
    L: Leaf,

---

crates/starknet_patricia/src/patricia_merkle_tree/filled_tree/tree.rs line 390 at r8 (raw file):

            })
            .collect::<Result<_, _>>()
    }

I don't like the cloning here
in a separate PR, can you check if serialize can be consuming?
if you add a get_all_nodes_owned method?

intuitively I'd say after serializing a trie you won't want to keep using the trie

Suggestion:

    fn serialize<Layout: NodeLayoutFor<L>>(
        self,
        key_context: &<Layout::DbLeaf as HasStaticPrefix>::KeyContext,
    ) -> SerializationResult<DbHashMap> {
        // This function iterates over each node in the tree, using the node's `db_key` as the
        // hashmap key and the result of the node's `serialize` method as the value.
        self.get_all_nodes_owned()
            .into_iter()
            .map(|(index, node)| {
                let (db_key, node_db_object) =
                    Layout::get_db_object(index, key_context, node);
                let db_value = node_db_object.serialize()?;
                Ok((db_key, db_value))
            })
            .collect::<Result<_, _>>()
    }

---

crates/starknet_committer/src/db/forest_trait.rs line 134 at r8 (raw file):

    <Layout as NodeLayoutFor<ContractState>>::DbLeaf: HasStaticPrefix<KeyContext = EmptyKeyContext>,
    <Layout as NodeLayoutFor<CompiledClassHash>>::DbLeaf:
        HasStaticPrefix<KeyContext = EmptyKeyContext>,

these trait bounds can (and should) be defined on the trait definition of Layout, right? this bound is always needed for every layout?

Code quote:

    Layout: NodeLayoutFor<StarknetStorageValue>
        + NodeLayoutFor<ContractState>
        + NodeLayoutFor<CompiledClassHash>,
    <Layout as NodeLayoutFor<StarknetStorageValue>>::DbLeaf:
        HasStaticPrefix<KeyContext = ContractAddress>,
    <Layout as NodeLayoutFor<ContractState>>::DbLeaf: HasStaticPrefix<KeyContext = EmptyKeyContext>,
    <Layout as NodeLayoutFor<CompiledClassHash>>::DbLeaf:
        HasStaticPrefix<KeyContext = EmptyKeyContext>,

---

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

/// While the same underlying type as `FactDbFilledNode`, this alias is not used in the context of
/// the DB-representation of the node.
pub type HashFilledNode<L> = FilledNode<L, HashOutput>;

compiler won't block you from sending a HashFilledNode<L> to a FactDbFilledNode<L> API or vice-versa, right?
since that's the case, I think instead of another name for the same type, we just need a better name (more general?)

Code quote:

/// A node in a filled tree, where all the hashes were computed. Used in the `FilledTree` trait.
///
/// While the same underlying type as `FactDbFilledNode`, this alias is not used in the context of
/// the DB-representation of the node.
pub type HashFilledNode<L> = FilledNode<L, HashOutput>;

---

crates/starknet_committer_and_os_cli/benches/main.rs line 52 at r8 (raw file):

                runtime.block_on(tree_computation_flow::<
                    StarknetStorageValue,
                    FactsNodeLayout,

maybe add a TODO to make this benchmark generic...?
we want to be able to benchmark both layouts (we will use both in production).
I'm imagining something like fn single_tree_flow_benchmark<Layout>(..)

Code quote:

FactsNodeLayout,

[dorimedini-starkware]

@dorimedini-starkware made 1 comment and resolved 1 discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ArielElp).

---

crates/starknet_committer/src/db/forest_trait.rs line 134 at r8 (raw file):

Previously, dorimedini-starkware wrote…

these trait bounds can (and should) be defined on the trait definition of Layout, right? this bound is always needed for every layout?

ah... next PR, I see

[ArielElp]

@ArielElp made 2 comments.
Reviewable status: 8 of 9 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware and @yoavGrs).

---

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

Previously, dorimedini-starkware wrote…

compiler won't block you from sending a HashFilledNode<L> to a FactDbFilledNode<L> API or vice-versa, right?
since that's the case, I think instead of another name for the same type, we just need a better name (more general?)

It won't, alias only helps you understand which type should be used where, but it doesn't stop you from using the "wrong" one.

If it helps, this PR https://reviewable.io/reviews/starkware-libs/sequencer/11644#- (last in the stack, after tests) uses a new wrapper of FilledNode for the DB type (not for naming reasons, only due to orphan rules, but this also helps your cause).

---

crates/starknet_patricia/src/patricia_merkle_tree/filled_tree/tree.rs line 21 at r8 (raw file):

Previously, dorimedini-starkware wrote…

revert line deletion

Done.

[dorimedini-starkware]

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