Skip to content

starknet_committer: define fetch_contract_storage_paths_concurrently#14282

Merged
ArielElp merged 1 commit into
mainfrom
ariel/split_read_paths_to_sequential_and_concurrent
Jun 8, 2026
Merged

starknet_committer: define fetch_contract_storage_paths_concurrently#14282
ArielElp merged 1 commit into
mainfrom
ariel/split_read_paths_to_sequential_and_concurrent

Conversation

@ArielElp

@ArielElp ArielElp commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@reviewable-StarkWare

Copy link
Copy Markdown

This change is Reviewable

@ArielElp ArielElp force-pushed the ariel/split_read_paths_to_sequential_and_concurrent branch from 9c26b8a to a4fcb4e Compare June 1, 2026 08:41
@ArielElp ArielElp force-pushed the ariel/define_new_storage_task branch from 5d66469 to 3dd02d1 Compare June 1, 2026 08:41
@ArielElp ArielElp marked this pull request as ready for review June 1, 2026 11:34
@cursor

cursor Bot commented Jun 1, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Refactor plus new dead-code helpers; behavior in fetch_all_patricia_paths is unchanged aside from shared address/root lookup.

Overview
Adds fetch_contract_storage_paths in trie_traversal.rs, mirroring create_storage_tries: when storage implements GatherableStorage, per-contract storage Patricia proofs are fetched via storage.gather and StoragePathsReadTask; otherwise the same work runs in a sequential loop.

Extracts get_address_and_storage_root so contract address + storage root resolution (including skipping missing/new/deleted contracts) is shared. fetch_all_patricia_paths in tree.rs now calls that helper instead of inlined logic.

The new entry points are #[allow(dead_code)] for now—the concurrent storage-path API is defined but not yet switched in at the main proof-fetch call site.

Reviewed by Cursor Bugbot for commit 72f6809. Bugbot is set up for automated code reviews on this repo. Configure here.

@ArielElp ArielElp requested a review from yoavGrs June 1, 2026 12:39
@ArielElp ArielElp force-pushed the ariel/split_read_paths_to_sequential_and_concurrent branch from a4fcb4e to 2cabcf1 Compare June 2, 2026 11:57
@ArielElp ArielElp force-pushed the ariel/define_new_storage_task branch from 3dd02d1 to 1bb4a46 Compare June 2, 2026 11:57
@ArielElp ArielElp force-pushed the ariel/define_new_storage_task branch from 1bb4a46 to c0762db Compare June 2, 2026 12:08
@ArielElp ArielElp force-pushed the ariel/split_read_paths_to_sequential_and_concurrent branch 2 times, most recently from 98cfa3b to f629d46 Compare June 2, 2026 13:54
@ArielElp ArielElp force-pushed the ariel/define_new_storage_task branch from c0762db to 74043bf Compare June 2, 2026 13:54
@ArielElp ArielElp force-pushed the ariel/split_read_paths_to_sequential_and_concurrent branch from f629d46 to 6475f21 Compare June 2, 2026 16:02
@ArielElp ArielElp force-pushed the ariel/define_new_storage_task branch 2 times, most recently from 227a428 to 9950a6c Compare June 2, 2026 17:21
@ArielElp ArielElp force-pushed the ariel/split_read_paths_to_sequential_and_concurrent branch from 6475f21 to 7b965ad Compare June 2, 2026 17:21
@ArielElp ArielElp force-pushed the ariel/define_new_storage_task branch from f36ecf8 to 0f499ff Compare June 3, 2026 15:52

@yoavGrs yoavGrs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yoavGrs reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on ArielElp).


crates/starknet_committer/src/db/trie_traversal.rs line 1001 at r1 (raw file):

        .await?;
        contracts_trie_storage_proofs.insert(contract_address, proof);
    }

Now I see that it's not new code.

Code quote:

    let mut contracts_trie_storage_proofs =
        HashMap::with_capacity(contract_storage_sorted_leaf_indices.len());

    for (idx, sorted_leaf_indices) in contract_storage_sorted_leaf_indices {
        let contract_address = try_node_index_into_contract_address(idx).unwrap_or_else(|_| {
            panic!(
                "Converting leaf NodeIndex to ContractAddress should succeed; failed to convert \
                 {idx:?}."
            )
        });

        // The contract address might not exist in the contracts trie in the following cases:
        // 1. We are looking at the previous tree and the contract is new.
        // 2. We are looking at the new tree and the contract is deleted (revert).
        // In either case, the storage trie of this contract is empty, so there is nothing to
        // prove regarding the contract storage.
        let Some(storage_root_hash) =
            contract_leaves.get(idx).map(|leaf| leaf.as_ref().storage_root_hash)
        else {
            continue;
        };

        let leaves = None;
        let proof = fetch_patricia_paths::<StorageLayout::DbLeaf, StorageLayout>(
            storage,
            storage_root_hash,
            *sorted_leaf_indices,
            leaves,
            &contract_address,
        )
        .await?;
        contracts_trie_storage_proofs.insert(contract_address, proof);
    }

@yoavGrs yoavGrs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yoavGrs made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on ArielElp).


crates/starknet_committer/src/db/trie_traversal.rs line 1000 at r1 (raw file):

        )
        .await?;
        contracts_trie_storage_proofs.insert(contract_address, proof);

Share code with impl StorageTask for StoragePathsReadTask.

Code quote:

        let leaves = None;
        let proof = fetch_patricia_paths::<StorageLayout::DbLeaf, StorageLayout>(
            storage,
            storage_root_hash,
            *sorted_leaf_indices,
            leaves,
            &contract_address,
        )
        .await?;
        contracts_trie_storage_proofs.insert(contract_address, proof);

@yoavGrs yoavGrs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yoavGrs reviewed 1 file.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on ArielElp).

@ArielElp ArielElp left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ArielElp made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on yoavGrs).


crates/starknet_committer/src/db/trie_traversal.rs line 1000 at r1 (raw file):

Previously, yoavGrs wrote…

Share code with impl StorageTask for StoragePathsReadTask.

There isn't much code to share, it only ends up increasing LOC. StoragePathsReadTask just calls fetch_patricia_paths, this function loops + does some input preparation.

@ArielElp ArielElp force-pushed the ariel/define_new_storage_task branch from 0f499ff to 7403c44 Compare June 7, 2026 07:57
@ArielElp ArielElp force-pushed the ariel/split_read_paths_to_sequential_and_concurrent branch from b1d7747 to 101a813 Compare June 7, 2026 07:57

@yoavGrs yoavGrs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yoavGrs resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on ArielElp).

@dorimedini-starkware dorimedini-starkware left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


crates/starknet_committer/src/db/trie_traversal.rs line 1001 at r1 (raw file):

Previously, yoavGrs wrote…

Now I see that it's not new code.

where is it from?


crates/starknet_committer/src/db/trie_traversal.rs line 989 at r1 (raw file):

        else {
            continue;
        };

to reduce boilerplate and keep the same docstring in one place, extract to a function and use twice

Suggestion:

        let Some((contract_address, storage_root_hash)) = get_address_and_storage_root(
            idx, contract_leaves
        ) else {
            continue;
        };

@dorimedini-starkware dorimedini-starkware left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


crates/starknet_committer/src/db/trie_traversal.rs line 1001 at r1 (raw file):

Previously, dorimedini-starkware wrote…

where is it from?

ah, from fetch_all_patricia_paths?

@ArielElp ArielElp force-pushed the ariel/define_new_storage_task branch from 7403c44 to d0cde67 Compare June 7, 2026 12:09
@ArielElp ArielElp force-pushed the ariel/split_read_paths_to_sequential_and_concurrent branch from 101a813 to 376f97c Compare June 7, 2026 12:09

@ArielElp ArielElp left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


crates/starknet_committer/src/db/trie_traversal.rs line 989 at r1 (raw file):

Previously, dorimedini-starkware wrote…

to reduce boilerplate and keep the same docstring in one place, extract to a function and use twice

Done.


crates/starknet_committer/src/db/trie_traversal.rs line 1001 at r1 (raw file):

Previously, dorimedini-starkware wrote…

ah, from fetch_all_patricia_paths?

Ah now I see what you meant, this logic is changed a bit in the next PR anyway (I accidentally "fixed" it in this one, doesn't matter much)

@dorimedini-starkware dorimedini-starkware left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ArielElp ArielElp force-pushed the ariel/split_read_paths_to_sequential_and_concurrent branch from 376f97c to 8f1abbf Compare June 7, 2026 12:17
@ArielElp ArielElp force-pushed the ariel/define_new_storage_task branch from d0cde67 to 437c43d Compare June 7, 2026 12:17
@ArielElp ArielElp changed the base branch from ariel/define_new_storage_task to graphite-base/14282 June 8, 2026 11:58
@graphite-app

graphite-app Bot commented Jun 8, 2026

Copy link
Copy Markdown

Merge activity

  • Jun 8, 12:40 PM UTC: This pull request can not be added to the Graphite merge queue. Please try rebasing and resubmitting to merge when ready.
  • Jun 8, 12:40 PM UTC: Graphite disabled "merge when ready" on this PR due to: a merge conflict with the target branch; resolve the conflict and try again..

@dorimedini-starkware dorimedini-starkware left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ArielElp ArielElp force-pushed the ariel/split_read_paths_to_sequential_and_concurrent branch from 8f1abbf to 3cbe5fb Compare June 8, 2026 12:49
@ArielElp ArielElp force-pushed the graphite-base/14282 branch from 437c43d to e0b7076 Compare June 8, 2026 12:49
@ArielElp ArielElp changed the base branch from graphite-base/14282 to main June 8, 2026 12:49

@dorimedini-starkware dorimedini-starkware left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ArielElp ArielElp force-pushed the ariel/split_read_paths_to_sequential_and_concurrent branch from 3cbe5fb to 72f6809 Compare June 8, 2026 13:05

@dorimedini-starkware dorimedini-starkware left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ArielElp ArielElp added this pull request to the merge queue Jun 8, 2026
Merged via the queue into main with commit b61348b Jun 8, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants