apollo_committer: add request_paths_and_commit_block tests

[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

• starknet_committer: fetch patricia paths concurrently #14283
• starknet_committer: define fetch_contract_storage_paths_concurrently #14282
• starknet_committer: define StoragePathsReadTask #14281
• starknet_patricia_storage: move is_gatherable to ReadOnlyStorage #14280
• 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 👈 (View in Graphite)
• 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
• 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.

[cursor]

PR Summary

Low Risk
Test-only changes plus a gated test helper; no production committer or storage behavior changes.

Overview
Adds integration tests for read_paths_and_commit_block (behind the os_input feature), wired in from committer_test with new dev-dependencies on starknet_committer and starknet_patricia.

The new suite checks that Patricia witnesses for accessed class/contract/storage keys are valid paths, that replaying the same request after stripping trie nodes still returns identical proofs (persisted witnesses), and that revert_block clears the per-block accessed-keys digest and stored witness payload.

IndexDb::clear_patricia_trie_nodes_for_test is added under os_input + testing so tests can drop trie node keys while keeping commitment metadata and witness blobs.

^{Reviewed by Cursor Bugbot for commit efc7f18. Bugbot is set up for automated code reviews on this repo. Configure here.}

[ArielElp]

@ArielElp made 5 comments.
Reviewable status: 1 of 5 files reviewed, 5 unresolved discussions (waiting on yoavGrs).

---

a discussion (no related file):

Previously, yoavGrs wrote…

Please split it into settings (block structure) and logic (verify functions) PRs.

The block setup is ~100 lines of arbitrary vlaues, IMO it's weird to review them without knowing the test flow.

---

crates/apollo_committer/src/request_paths_and_commit_block_tests.rs line 81 at r1 (raw file):

Previously, yoavGrs wrote…

Do the naming clearer - what are the block 0 settings, and what are the general constants.

I added docs for the two tests, PTAL and let me know whether the flow makes sense.

---

crates/apollo_committer/src/request_paths_and_commit_block_tests.rs line 111 at r1 (raw file):

Previously, yoavGrs wrote…

Share the values with block_0_state_diff.
For testing theaccessed_storage_leaves, they should be connected to the committer input.

I specifically want to test fetching paths not included in the state diff, see comments below.

---

crates/apollo_committer/src/request_paths_and_commit_block_tests.rs line 131 at r1 (raw file):

Previously, yoavGrs wrote…

The story of block 0 is broken - unacessed key in the state diff.

unaccessed = not later requested for witnesses (a la "read set")

we never use the committer in that way (we always request paths for the state diff too), but the API supports it

---

crates/apollo_committer/src/request_paths_and_commit_block_tests.rs line 324 at r1 (raw file):

Previously, yoavGrs wrote…

This needs some explanation. Just to make sure I understand, are you verifying that fetch_patricia_paths for indexes returns the same result for the facts layout?

If that's the core of the test, consider converting it to a more focused unit test.

No, it's not about testing multiple storage layouts. Changed the comment on verify_preimage_map_paths_exist.

TL;DR: I'm abusing the current code because there is no Rust to verify all the paths given a PreimageMap. Modulo hash calculation though, this is actually fetch_patricia_paths, which starts at the root towards the requested leaves, and attempts to get all the witnesses. So the trick to test the result is to feed it back to fetch_patricia_paths and see that the traversal succeeds. To do that, you need storage, and PreImageMap is more easily converted to a facts layout than to an index layout. An index layout storage would have served as well for this test.

[yoavGrs]

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

---

a discussion (no related file):

Previously, ArielElp wrote…

The block setup is ~100 lines of arbitrary vlaues, IMO it's weird to review them without knowing the test flow.

It's too long for me :(
Please include in the first PR the stateless validations verify_preimage_map_paths_exist and verify_witness_patricia_paths, along with all related utils.

[yoavGrs]

@yoavGrs made 2 comments.
Reviewable status: 1 of 5 files reviewed, 6 unresolved discussions (waiting on ArielElp).

---

crates/apollo_committer/src/request_paths_and_commit_block_tests.rs line 324 at r1 (raw file):

Previously, ArielElp wrote…

No, it's not about testing multiple storage layouts. Changed the comment on verify_preimage_map_paths_exist.

TL;DR: I'm abusing the current code because there is no Rust to verify all the paths given a PreimageMap. Modulo hash calculation though, this is actually fetch_patricia_paths, which starts at the root towards the requested leaves, and attempts to get all the witnesses. So the trick to test the result is to feed it back to fetch_patricia_paths and see that the traversal succeeds. To do that, you need storage, and PreImageMap is more easily converted to a facts layout than to an index layout. An index layout storage would have served as well for this test.

You are testing fetch_patricia_witnesses that calls fetch_patricia_paths by calling fetch_patricia_paths.

---

crates/apollo_committer/src/request_paths_and_commit_block_tests.rs line 429 at r4 (raw file):

) {
    let class_root_hashes = [HashOutput::ROOT_OF_EMPTY_TREE, classes_trie_root];
    let contract_root_hashes = [HashOutput::ROOT_OF_EMPTY_TREE, contracts_trie_root];

You're filtering out empty roots in verify_preimage_map_paths_exist

Suggestion:

    let class_root_hashes = [classes_trie_root];
    let contract_root_hashes = [contracts_trie_root];

[yoavGrs]

@yoavGrs made 1 comment.
Reviewable status: 1 of 5 files reviewed, 6 unresolved discussions (waiting on ArielElp).

---

crates/apollo_committer/src/request_paths_and_commit_block_tests.rs line 324 at r1 (raw file):

Previously, yoavGrs wrote…

You are testing fetch_patricia_witnesses that calls fetch_patricia_paths by calling fetch_patricia_paths.

On second thought, this is enough, but please make sure the function name reflects it, maybe assert_witness_nodes_on_traversal_paths?

[yoavGrs]

@yoavGrs made 8 comments.
Reviewable status: 1 of 5 files reviewed, 12 unresolved discussions (waiting on ArielElp).

---

crates/apollo_committer/src/request_paths_and_commit_block_tests.rs line 111 at r1 (raw file):

Previously, ArielElp wrote…

I specifically want to test fetching paths not included in the state diff, see comments below.

The values 100 and 200 are reused in fn block_0_state_diff(). Define them as constants.

---

crates/apollo_committer/src/request_paths_and_commit_block_tests.rs line 131 at r1 (raw file):

Previously, ArielElp wrote…

unaccessed = not later requested for witnesses (a la "read set")

we never use the committer in that way (we always request paths for the state diff too), but the API supports it

OK, it confused me.
More importantly, we do use the committer with accessed keys that are not in the state diff. Can you test this case?

---

a discussion (no related file):
Are there tests in crates/apollo_committer/src/committer_test.rs that we want to share with the new endpoint?

---

crates/apollo_committer/src/request_paths_and_commit_block_tests.rs line 161 at r4 (raw file):

            accessed_contract_1() => indexmap! {
                accessed_storage_key_1().0 => 100_u128.into(),
                UNACCESSED_STORAGE_KEY.into() => 300_u128.into(),

Same comment about the values here. Define constatnts.

Code quote:

                accessed_storage_key_1().0 => 100_u128.into(),
                UNACCESSED_STORAGE_KEY.into() => 300_u128.into(),

---

crates/apollo_committer/src/request_paths_and_commit_block_tests.rs line 218 at r4 (raw file):

    assert_eq!(response.global_root, replay_response.global_root);
    assert_eq!(response.patricia_proofs, replay_response.patricia_proofs);
    assert_witnesses_and_digest_present(&mut committer, BlockNumber(height)).await;

I prefer a more realistic scenario.
What are you trying to verify?

Code quote:

    // Historical replay should load persisted witnesses, removing trie nodes to assert this.
    committer.forest_storage.clear_patricia_trie_nodes_for_test();

    let replay_response = committer.read_paths_and_commit_block(request).await.unwrap();
    assert_eq!(response.global_root, replay_response.global_root);
    assert_eq!(response.patricia_proofs, replay_response.patricia_proofs);
    assert_witnesses_and_digest_present(&mut committer, BlockNumber(height)).await;

---

crates/apollo_committer/src/request_paths_and_commit_block_tests.rs line 218 at r4 (raw file):

    assert_eq!(response.global_root, replay_response.global_root);
    assert_eq!(response.patricia_proofs, replay_response.patricia_proofs);
    assert_witnesses_and_digest_present(&mut committer, BlockNumber(height)).await;

Can you verify the exact value, not just is_some?

Code quote:

assert_witnesses_and_digest_present

---

crates/apollo_committer/src/request_paths_and_commit_block_tests.rs line 257 at r4 (raw file):

        .await
        .unwrap();
    assert_witnesses_and_digest_absent(&mut committer, BlockNumber(height_1)).await;

Verify the offset as well.

Code quote:

 assert_witnesses_and_digest_absent(&mut committer, BlockNumber(height_1)).await;

---

crates/starknet_committer/src/db/index_db/db.rs line 509 at r4 (raw file):

                || prefix == &*STATE_ROOT_METADATA_PREFIX
                || prefix == &*ACCESSED_KEYS_DIGEST_METADATA_PREFIX
                || prefix == &*PATRICIA_PATHS_PREFIX

Suggestion:

  prefix != &*CONTRACTS_TREE_PREFIX
      && prefix != &*CLASSES_TREE_PREFIX
      && prefix >= &*FIRST_AVAILABLE_PREFIX_FELT

[ArielElp]

@ArielElp made 9 comments.
Reviewable status: 1 of 5 files reviewed, 12 unresolved discussions (waiting on yoavGrs).

---

crates/apollo_committer/src/request_paths_and_commit_block_tests.rs line 111 at r1 (raw file):

Previously, yoavGrs wrote…

The values 100 and 200 are reused in fn block_0_state_diff(). Define them as constants.

Done.

---

crates/apollo_committer/src/request_paths_and_commit_block_tests.rs line 131 at r1 (raw file):

Previously, yoavGrs wrote…

OK, it confused me.
More importantly, we do use the committer with accessed keys that are not in the state diff. Can you test this case?

Accessed_keys is the same between block 0 and block 1, but in block 1 it is not a subset of the statediff, so already happening in the second test.

---

crates/apollo_committer/src/request_paths_and_commit_block_tests.rs line 324 at r1 (raw file):

You are testing fetch_patricia_witnesses that calls fetch_patricia_paths by calling fetch_patricia_paths.

Yes, sounds a bit stupid, but assuming I'm not testing fetch_patricia_paths here, I think there's some sense. Maybe it's too roundabout, and I (Claude) should just implement path verification. The problem is that it would need a top-down scan very similar to fetch_patricia_paths, just with hashing along the way.

---

crates/apollo_committer/src/request_paths_and_commit_block_tests.rs line 161 at r4 (raw file):

Previously, yoavGrs wrote…

Same comment about the values here. Define constatnts.

Done.

---

crates/apollo_committer/src/request_paths_and_commit_block_tests.rs line 218 at r4 (raw file):

Previously, yoavGrs wrote…

I prefer a more realistic scenario.
What are you trying to verify?

Doc was not pushed before, PTAL now. TLDR is that I'm just testing the batcher submitting the same request to the committer (which is a scenario we have a specific handling flow in both os_input and the regular flow)

---

crates/apollo_committer/src/request_paths_and_commit_block_tests.rs line 218 at r4 (raw file):

Previously, yoavGrs wrote…

Can you verify the exact value, not just is_some?

Done.

---

crates/apollo_committer/src/request_paths_and_commit_block_tests.rs line 257 at r4 (raw file):

Previously, yoavGrs wrote…

Verify the offset as well.

Done.

---

crates/apollo_committer/src/request_paths_and_commit_block_tests.rs line 429 at r4 (raw file):

Previously, yoavGrs wrote…

You're filtering out empty roots in verify_preimage_map_paths_exist

AI fluke, my bad. Simplified.

---

crates/starknet_committer/src/db/index_db/db.rs line 509 at r4 (raw file):

                || prefix == &*STATE_ROOT_METADATA_PREFIX
                || prefix == &*ACCESSED_KEYS_DIGEST_METADATA_PREFIX
                || prefix == &*PATRICIA_PATHS_PREFIX

isn't this equivalent to

prefix >= &*COMMITMENT_OFFSET_METADATA_PREFIX

doing this with a comment

[yoavGrs]

@yoavGrs reviewed 4 files and all commit messages, made 2 comments, and resolved 10 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on ArielElp).

---

crates/apollo_committer/src/request_paths_and_commit_block_tests.rs line 324 at r1 (raw file):

Previously, ArielElp wrote…

You are testing fetch_patricia_witnesses that calls fetch_patricia_paths by calling fetch_patricia_paths.

Yes, sounds a bit stupid, but assuming I'm not testing fetch_patricia_paths here, I think there's some sense. Maybe it's too roundabout, and I (Claude) should just implement path verification. The problem is that it would need a top-down scan very similar to fetch_patricia_paths, just with hashing along the way.

The question is, what is the scope of what you want to test here?
As you wrote, we have a unit test that verifies fetch_patricia_paths and there is no need to duplicate it here.

---

crates/starknet_committer/src/db/index_db/db.rs line 509 at r4 (raw file):

Previously, ArielElp wrote…

isn't this equivalent to

prefix >= &*COMMITMENT_OFFSET_METADATA_PREFIX

doing this with a comment

It is