starknet_committer: add index db

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

---

a discussion (no related file):
These read & write implementations are similar to the facts layout.
What are the next steps?

Do you plan to define a default implementation with generic types?
Made some changes to justify the code duplication?

---

crates/starknet_committer/src/db/index_db/db.rs line 51 at r2 (raw file):

impl<'a, L: Leaf> NodeLayout<'a, L> for IndexNodeLayout
where
    L: HasStaticPrefix<KeyContext = TrieType>,

non-blocking

Suggestion:

impl<'a, L> NodeLayout<'a, L> for IndexNodeLayout
where
    L: Leaf + HasStaticPrefix<KeyContext = TrieType>,

---

crates/starknet_committer/src/db/index_db/db.rs line 59 at r2 (raw file):

    type SubTree = IndexLayoutSubTree<'a>;

    fn generate_key_context(trie_type: TrieType) -> <L as HasStaticPrefix>::KeyContext {

In both implementations, you do not generate (or calculate). WDYT?

Suggestion:

get_key_context

---

crates/starknet_committer/src/db/index_db/db.rs line 85 at r2 (raw file):

                forest_sorted_indices.contracts_trie_sorted_indices,
            )
            .await?;

By adding such a get_state_roots function to the reader trait, can we set the read function as the default implementation?

Suggestion:

        let state_roots = self.get_state_roots(context);
        let (contracts_trie, original_contracts_trie_leaves) =
            create_contracts_trie::<IndexLayoutContractState, IndexNodeLayout>(
                &mut self.storage,
                state_roots.contracts_trie_root_hash,
                forest_sorted_indices.contracts_trie_sorted_indices,
            )
            .await?;

[nimrod-starkware]

@nimrod-starkware reviewed all commit messages and made 1 comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ArielElp).

---

crates/starknet_committer/src/db/index_db/db.rs line 109 at r2 (raw file):

        ))
    }
}

This looks almost like the ForestReader impl for FactsDb.
IIUC the only difference are the types.
I thought to associate the concrete leaf types with the ForestReader trait and add (and use) default implementation for fn read. WDYT?

Code quote:

// TODO(Ariel): define an IndexDbInitialRead empty type, and check whether each tree is empty inside
// create_xxx_trie.
#[async_trait]
impl<S: Storage> ForestReader<FactsDbInitialRead> for IndexDb<S> {
    /// Creates an original skeleton forest that includes the storage tries of the modified
    /// contracts, the classes trie and the contracts trie. Additionally, returns the original
    /// contract states that are needed to compute the contract state tree.
    async fn read<'a>(
        &mut self,
        context: FactsDbInitialRead,
        storage_updates: &'a HashMap<ContractAddress, LeafModifications<StarknetStorageValue>>,
        classes_updates: &'a LeafModifications<CompiledClassHash>,
        forest_sorted_indices: &'a ForestSortedIndices<'a>,
        config: ReaderConfig,
    ) -> ForestResult<(OriginalSkeletonForest<'a>, HashMap<NodeIndex, ContractState>)> {
        let (contracts_trie, original_contracts_trie_leaves) =
            create_contracts_trie::<IndexLayoutContractState, IndexNodeLayout>(
                &mut self.storage,
                context.0.contracts_trie_root_hash,
                forest_sorted_indices.contracts_trie_sorted_indices,
            )
            .await?;
        let storage_tries =
            create_storage_tries::<IndexLayoutStarknetStorageValue, IndexNodeLayout>(
                &mut self.storage,
                storage_updates,
                &original_contracts_trie_leaves,
                &config,
                &forest_sorted_indices.storage_tries_sorted_indices,
            )
            .await?;
        let classes_trie = create_classes_trie::<IndexLayoutCompiledClassHash, IndexNodeLayout>(
            &mut self.storage,
            classes_updates,
            context.0.classes_trie_root_hash,
            &config,
            forest_sorted_indices.classes_trie_sorted_indices,
        )
        .await?;

        Ok((
            OriginalSkeletonForest { classes_trie, contracts_trie, storage_tries },
            original_contracts_trie_leaves,
        ))
    }
}

[nimrod-starkware]

@nimrod-starkware made 1 comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ArielElp).

---

crates/starknet_committer/src/db/index_db/db.rs line 109 at r2 (raw file):

Previously, nimrod-starkware wrote…

This looks almost like the ForestReader impl for FactsDb.
IIUC the only difference are the types.
I thought to associate the concrete leaf types with the ForestReader trait and add (and use) default implementation for fn read. WDYT?

(obviously should not be done in this PR, if you think it's a good idea add a todo please)

[ArielElp]

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

---

crates/starknet_committer/src/db/index_db/db.rs line 59 at r2 (raw file):

Previously, yoavGrs wrote…

In both implementations, you do not generate (or calculate). WDYT?

I prefer generate since this is THE place where we get it for the first time and then it's passed between all the internal functions

---

crates/starknet_committer/src/db/index_db/db.rs line 85 at r2 (raw file):

Previously, yoavGrs wrote…

By adding such a get_state_roots function to the reader trait, can we set the read function as the default implementation?

IMO this makes the trait more complicated and the benefit is not that large. I prefer the read_inner method with all the generics, but prefer keeping it as is compared to both. @dorimedini-starkware will be the judge

---

crates/starknet_committer/src/db/index_db/db.rs line 109 at r2 (raw file):

Previously, nimrod-starkware wrote…

(obviously should not be done in this PR, if you think it's a good idea add a todo please)

See comment above, relayed to @dorimedini-starkware's judgement

[dorimedini-starkware]

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

---

crates/starknet_committer/src/db/index_db/db.rs line 85 at r2 (raw file):

Previously, ArielElp wrote…

IMO this makes the trait more complicated and the benefit is not that large. I prefer the read_inner method with all the generics, but prefer keeping it as is compared to both. @dorimedini-starkware will be the judge

1. indeed seems identical except for generics; why does it complicate the trait? generics list is longer?
2. If we go for this change, I think this actually should be part of this Pr, so it's easy to see behavior change is minimal, rather than adding logic here and deleting it in a later PR

---

crates/starknet_committer/src/db/index_db/db.rs line 109 at r2 (raw file):

Previously, ArielElp wrote…

See comment above, relayed to @dorimedini-starkware's judgement

gave my two cents above

---

crates/starknet_committer/src/db/index_db/db.rs line 38 at r2 (raw file):

pub struct IndexDb<S: Storage> {
    pub storage: S,

better to block direct access in advance

Suggestion:

storage: S,

---

crates/starknet_committer/src/db/index_db/db.rs line 135 at r2 (raw file):

            .unwrap_or_else(|_| panic!("Write of {n_updates} new updates to storage failed"));
        n_updates
    }

these impls are also identical to the FactsDb implementation, why are specific impls needed?

Code quote:

    fn serialize_forest(filled_forest: &FilledForest) -> SerializationResult<DbHashMap> {
        let mut serialized_forest = DbHashMap::new();

        // TODO(Ariel): use a different key context when FilledForest is generic over leaf types.
        for tree in filled_forest.storage_tries.values() {
            serialized_forest.extend(tree.serialize(&EmptyKeyContext)?);
        }

        // Contracts and classes tries.
        serialized_forest.extend(filled_forest.contracts_trie.serialize(&EmptyKeyContext)?);
        serialized_forest.extend(filled_forest.classes_trie.serialize(&EmptyKeyContext)?);

        Ok(serialized_forest)
    }

    async fn write_updates(&mut self, updates: DbHashMap) -> usize {
        let n_updates = updates.len();
        self.storage
            .mset(updates)
            .await
            .unwrap_or_else(|_| panic!("Write of {n_updates} new updates to storage failed"));
        n_updates
    }

[dorimedini-starkware]

@dorimedini-starkware reviewed 1 file and all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ArielElp, @nimrod-starkware, and @yoavGrs).

[ArielElp]

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

---

a discussion (no related file):

Previously, yoavGrs wrote…

These read & write implementations are similar to the facts layout.
What are the next steps?

Do you plan to define a default implementation with generic types?
Made some changes to justify the code duplication?

Done.

---

crates/starknet_committer/src/db/index_db/db.rs line 38 at r2 (raw file):

Previously, dorimedini-starkware wrote…

better to block direct access in advance

Done.

---

crates/starknet_committer/src/db/index_db/db.rs line 85 at r2 (raw file):

Previously, dorimedini-starkware wrote…

1. indeed seems identical except for generics; why does it complicate the trait? generics list is longer?
2. If we go for this change, I think this actually should be part of this Pr, so it's easy to see behavior change is minimal, rather than adding logic here and deleting it in a later PR

I went for not changing the trait and adding the generic forest_reader function, which is called from both reads. PTAL, looks good IMO. Relying on a helper inside the trait requires extracting storage from the reader which we probably don't want to do.

---

crates/starknet_committer/src/db/index_db/db.rs line 109 at r2 (raw file):

Previously, dorimedini-starkware wrote…

gave my two cents above

Done.

---

crates/starknet_committer/src/db/index_db/db.rs line 135 at r2 (raw file):

Previously, dorimedini-starkware wrote…

these impls are also identical to the FactsDb implementation, why are specific impls needed?

True, in the serialization PRs they will become different (in a similar way to reads, via the generics). Can we leave it as-is and tackle the serialization shared code in the serialization PR?

[dorimedini-starkware]

@dorimedini-starkware reviewed 3 files and all commit messages, and resolved 2 discussions.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @nimrod-starkware and @yoavGrs).

[nimrod-starkware]

@nimrod-starkware made 1 comment and resolved 1 discussion.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @yoavGrs).

[dorimedini-starkware]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @yoavGrs).

[yoavGrs]

@yoavGrs reviewed all commit messages, made 1 comment, and resolved 3 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArielElp).

[dorimedini-starkware]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArielElp).