starknet_committer: underlying logic of the new read witnesses and commit endpoint

[ArielElp]

No description provided.

[reviewable-StarkWare]

This change is 

[ArielElp]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• apollo_committer,starknet_committer: use blockifier's AccessedKeys #14252
• apollo_committer: add new request handler #14003
• apollo_committer: add request_paths_and_commit_block tests #14208
• starknet_committer: delete patricia paths on reverts #14002
• starknet_committer,apollo_committer: add timers for fetch patricia paths #14189
• starknet_committer: underlying logic of the new read witnesses and commit endpoint #14001 👈 (View in Graphite)
• starknet_committer: extract metadata construction to a function #14000
• starknet_committer: extract commit plan to commit_or_load #13999
• starknet_committer: read and write patricia paths to index db #13998
• apollo_committer_types: add patricia proofs request and response types #13996 : 1 other dependent PR (#13997 )
• starknet_committer: add patricia paths forest reader/writer traits #13995
• starknet_committer: define StarknetForestProofs serde #13994
• starknet_committer: compute and store accessed keys digest #13993
• starknet_committer: add utility that returns sorted leaf indices #13992
• starknet_committer: abstract fetch_all_patricia_paths #13991
• starknet_patricia_storage: two layer storage #13990
• starknet_committer: move fetch_patricia_paths tests #13960
• starknet_committer: fetch patricia paths tests #13959
• starknet_committer: support both layouts in fetch_patricia_paths #13958
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[ArielElp]

@ArielElp made 1 comment and resolved 1 discussion.
Reviewable status: 2 of 5 files reviewed, all discussions resolved (waiting on yoavGrs).

---

crates/apollo_committer/src/committer.rs line 626 at r6 (raw file):

Previously, yoavGrs wrote…

This is my suggestion - break it with a let in the middle.

Done

[yoavGrs]

@yoavGrs reviewed 2 files and all commit messages.
Reviewable status: 4 of 5 files reviewed, all discussions resolved.

[ArielElp]

@ArielElp dismissed @yoavGrs from a discussion.
Reviewable status: 4 of 5 files reviewed, all discussions resolved (waiting on yoavGrs).

[yoavGrs]

@yoavGrs partially reviewed 4 files and all commit messages.
Reviewable status: 4 of 5 files reviewed, all discussions resolved.

[yoavGrs]

Reviewable status: 4 of 5 files reviewed, all discussions resolved.

[dorimedini-starkware]

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

---

crates/apollo_committer/Cargo.toml line 11 at r8 (raw file):

[features]
testing = []
os_input = ["apollo_committer_types/os_input", "starknet_committer/os_input"]

this is already in the os-inputs workflow right?

Code quote:

os_input = ["apollo_committer_types/os_input", "starknet_committer/os_input"]

---

crates/apollo_committer/src/committer.rs line 511 at r8 (raw file):

            class_hashes.len(),
            contract_addresses.len()
        );

it would be nice to log

1. state diff "size" (how many diffs in total, classes + storage leaves)
2. "read set size"
3. height
   currently you have height + partial read set size (no state diff size or number of storage leaves needed to be read)

Code quote:

        info!(
            "read_paths_and_commit_block: block {height} with {} class hashes, {} contract \
             addresses",
            class_hashes.len(),
            contract_addresses.len()
        );

---

crates/apollo_committer/src/committer.rs line 532 at r8 (raw file):

            }
            // Flow overview:
            // 1. Fetch patricia paths for the accessed keys

Suggestion:

// 1. Fetch patricia paths for the accessed keys.

---

crates/apollo_committer/src/committer.rs line 537 at r8 (raw file):

            // 3. Fetch patricia paths for the post-commit tries, via running step 1 against a two
            //    layer storage composed from the underlying storage and the modifications from 2.
            // 4. Merge the two sets of patricia paths and write the result to the storage.

this phase is done in "fact layout" mode, right? if we have two versions of the same node we will want both of them in the result

Code quote:

4. Merge the two sets of patricia paths and write the result to the storage.

---

crates/apollo_committer/src/committer.rs line 647 at r8 (raw file):

                })
            })
            .transpose()

what's this?

Code quote:

.transpose()

[ArielElp]

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

---

crates/apollo_committer/Cargo.toml line 11 at r8 (raw file):

Previously, dorimedini-starkware wrote…

this is already in the os-inputs workflow right?

These are, just added apollo_committer too

---

crates/apollo_committer/src/committer.rs line 511 at r8 (raw file):

Previously, dorimedini-starkware wrote…

it would be nice to log

1. state diff "size" (how many diffs in total, classes + storage leaves)
2. "read set size"
3. height
   currently you have height + partial read set size (no state diff size or number of storage leaves needed to be read)

Done.

---

crates/apollo_committer/src/committer.rs line 537 at r8 (raw file):

Previously, dorimedini-starkware wrote…

this phase is done in "fact layout" mode, right? if we have two versions of the same node we will want both of them in the result

yes, it's a merge of two maps from hash to preimage

---

crates/apollo_committer/src/committer.rs line 647 at r8 (raw file):

Previously, dorimedini-starkware wrote…

what's this?

Option -->Result

---

crates/apollo_committer/src/committer.rs line 511 at r9 (raw file):

                accumulator
            },
        );

@dorimedini-starkware @yoavGrs the blockifier's accessed keys don't sit too well here, my signatures are induced from fetch_all_patricia_paths/fetch_patricia_paths, so I need another conversion here. I can either extract this to a function or revert to a different style of AccessedKeys.

Code quote:

        let class_hashes: Vec<_> = accessed_class_hashes.iter().copied().collect();
        let contract_addresses: Vec<_> = accessed_contracts.iter().copied().collect();
        let contract_storage_keys = storage_keys.iter().fold(
            HashMap::<ContractAddress, Vec<StarknetStorageKey>>::new(),
            |mut accumulator, (address, key)| {
                accumulator.entry(*address).or_default().push(StarknetStorageKey(*key));
                accumulator
            },
        );

[ArielElp]

@ArielElp made 1 comment.
Reviewable status: 4 of 7 files reviewed, 5 unresolved discussions (waiting on dorimedini-starkware and yoavGrs).

---

crates/apollo_committer/src/committer.rs line 647 at r8 (raw file):

Previously, ArielElp wrote…

Option -->Result

`Option<Result<T, E>>` → `Result<Option, E>`

[ArielElp]

@ArielElp made 1 comment.
Reviewable status: 4 of 7 files reviewed, 5 unresolved discussions (waiting on dorimedini-starkware and yoavGrs).

---

crates/apollo_committer/src/committer.rs line 647 at r8 (raw file):

Previously, ArielElp wrote…

`Option<Result<T, E>>` → `Result<Option, E>`

`Option<Result<T, E>>` → `Result<Option, E>`

[dorimedini-starkware]

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

---

crates/apollo_committer/src/committer.rs line 647 at r8 (raw file):

Previously, ArielElp wrote…

`Option<Result<T, E>>` → `Result<Option, E>`

lol
my turn to try
Option<Result<T, E>> → Result<Option<T>, E>

---

crates/apollo_committer/src/committer.rs line 511 at r9 (raw file):

Previously, ArielElp wrote…

@dorimedini-starkware @yoavGrs the blockifier's accessed keys don't sit too well here, my signatures are induced from fetch_all_patricia_paths/fetch_patricia_paths, so I need another conversion here. I can either extract this to a function or revert to a different style of AccessedKeys.

@yoavGrs is AccessedKeys used anywhere else, other than DB serialization logic? Maybe better to change the types at the source

[yoavGrs]

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

---

crates/apollo_committer/src/committer.rs line 511 at r9 (raw file):

Previously, dorimedini-starkware wrote…

@yoavGrs is AccessedKeys used anywhere else, other than DB serialization logic? Maybe better to change the types at the source

It is used in the flow tests.
You can change the type, but a B-tree is sorted and does not allow duplicates.

[dorimedini-starkware]

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

---

crates/apollo_committer/src/committer.rs line 511 at r9 (raw file):

Previously, yoavGrs wrote…

It is used in the flow tests.
You can change the type, but a B-tree is sorted and does not allow duplicates.

@ArielElp we already talked about using BTrees in the fetch-paths signatures, which will help your sorted-indices struct as well, right?
I suggest leaving the conversions as they are, and migrating to BTrees in a PR at the top of the stack. This will solve it, correct?

[ArielElp]

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

---

crates/apollo_committer/src/committer.rs line 511 at r9 (raw file):

Previously, dorimedini-starkware wrote…

@ArielElp we already talked about using BTrees in the fetch-paths signatures, which will help your sorted-indices struct as well, right?
I suggest leaving the conversions as they are, and migrating to BTrees in a PR at the top of the stack. This will solve it, correct?

Looks doable, will add a PR.

[ArielElp]

@ArielElp resolved 1 discussion.
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).