feat(committer): use FilledTree::create() in filled forest #495

amosStarkware · 2024-08-18T14:09:55Z

This change is

amosStarkware

+reviewer:@dorimedini-starkware
second half of this PR:
https://reviewable.io/reviews/starkware-libs/sequencer/471

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

codecov · 2024-08-18T14:20:26Z

Codecov Report

Attention: Patch coverage is 10.34483% with 78 lines in your changes missing coverage. Please review.

Project coverage is 76.41%. Comparing base (091490f) to head (eb44e1b).
Report is 1 commits behind head on main-v0.13.2.

Files	Patch %	Lines
...tes/starknet_committer/src/forest/filled_forest.rs	0.00%	74 Missing ⚠️
...es/starknet_committer/src/block_committer/input.rs	66.66%	4 Missing ⚠️

Additional details and impacted files

@@               Coverage Diff                @@
##           main-v0.13.2     #495      +/-   ##
================================================
- Coverage         76.45%   76.41%   -0.05%     
================================================
  Files               314      314              
  Lines             34205    34240      +35     
  Branches          34205    34240      +35     
================================================
+ Hits              26152    26164      +12     
- Misses             5775     5798      +23     
  Partials           2278     2278

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dorimedini-starkware

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @amosStarkware)

crates/starknet_committer/src/block_committer/input.rs line 19 at r1 (raw file):

impl From<&NodeIndex> for ContractAddress {
    fn from(node_index: &NodeIndex) -> ContractAddress {
        assert!(node_index.is_leaf(), "NodeIndex {:?} is not a leaf.", node_index);

if you need to assert, maybe you should consider implementing TryFrom instead of From...?

Code quote:

impl From<&NodeIndex> for ContractAddress {
    fn from(node_index: &NodeIndex) -> ContractAddress {
        assert!(node_index.is_leaf(), "NodeIndex {:?} is not a leaf.", node_index);

crates/starknet_committer/src/block_committer/input.rs line 23 at r1 (raw file):

            Felt::try_from(*node_index - NodeIndex::FIRST_LEAF)
                .expect("Unable to convert node index to felt."),
        )

there is a computation here... please add a unit test for some valid values and edge cases.
if you convert this to a TryFrom implementation, also test some invalid cases please

Code quote:

        ContractAddress(
            Felt::try_from(*node_index - NodeIndex::FIRST_LEAF)
                .expect("Unable to convert node index to felt."),
        )

crates/starknet_committer/src/forest/filled_forest.rs line 81 at r1 (raw file):

        let (contracts_trie, storage_tries) =
            contracts_trie_task.await?.map_err(ForestError::ContractsTrie)?;
        let classes_trie = classes_trie_task.await?.map_err(ForestError::ClassesTrie)?;

there are already running (in parallel) and it doesn't matter that you await each one separately, right?

Code quote:

        let (contracts_trie, storage_tries) =
            contracts_trie_task.await?.map_err(ForestError::ContractsTrie)?;
        let classes_trie = classes_trie_task.await?.map_err(ForestError::ClassesTrie)?;

crates/starknet_committer/src/forest/filled_forest.rs line 81 at r1 (raw file):

        let (contracts_trie, storage_tries) =
            contracts_trie_task.await?.map_err(ForestError::ContractsTrie)?;
        let classes_trie = classes_trie_task.await?.map_err(ForestError::ClassesTrie)?;

consider:

swapping the order of awaits
adding logs

the classes trie will probably finish before the contracts trie (height 251 vs 502), so better to await it first

Suggestion:

        let classes_trie = classes_trie_task.await?.map_err(ForestError::ClassesTrie)?;
        log::???!("Classes trie update complete; {x} new facts computed.");
        let (contracts_trie, storage_tries) =
            contracts_trie_task.await?.map_err(ForestError::ContractsTrie)?;
        log::???!("Contracts trie update complete; {x} new facts computed.");

crates/starknet_committer/src/forest/filled_forest.rs line 109 at r1 (raw file):

            contract_address_to_storage_updates.len(),
            contract_address_to_storage_skeleton.len()
        );

please add an informative message if this assert fails

Code quote:

        assert_eq!(
            contract_address_to_storage_updates.len(),
            contract_address_to_storage_skeleton.len()
        );

crates/starknet_committer/src/forest/filled_forest.rs line 131 at r1 (raw file):

                        .ok_or(ForestError::MissingUpdatedSkeleton(contract_address))?,
                    storage_updates,
                ),

this 4-tuple is IMO not a very nice way to describe a type...
can you open another PR on top of the current tip of your stack, to convert this 4-tuple to a struct with named fields?

Code quote:

                (
                    node_index,
                    *(address_to_nonce
                        .get(&contract_address)
                        .unwrap_or(&original_contract_state.nonce)),
                    *(address_to_class_hash
                        .get(&contract_address)
                        .unwrap_or(&original_contract_state.class_hash)),
                    contract_address_to_storage_skeleton
                        .remove(&contract_address)
                        .ok_or(ForestError::MissingUpdatedSkeleton(contract_address))?,
                    storage_updates,
                ),

amosStarkware

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

crates/starknet_committer/src/block_committer/input.rs line 19 at r1 (raw file):