apollo_committer: test new edge bottom in witnesses

[ArielElp]

No description provided.

[reviewable-StarkWare]

This change is 

[ArielElp]

• apollo_committer: reduce tests boilerplate #14425
• apollo_committer: test new edge bottom in witnesses #14388 👈 (View in Graphite)
• apollo_committer: add request_paths_and_commit_block tests #14208
• starknet_patricia: add storage proof verification #14296
• apollo_committer: add new request handler #14003
• 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 with no production logic modified; low risk to runtime behavior.

Overview
Adds two integration tests in request_paths_and_commit_block_tests.rs that exercise read_paths_and_commit_block when a block removes a class leaf and the classes Patricia trie is reshaped with new edge nodes. Each test seeds block 0 with three class hashes in a specific trie layout, then commits block 1 with a deletion (compiled class hash zeroed) while requesting witnesses only for the deleted key.

The first scenario checks that the returned classes_trie_proof includes the hash of inner node G (bottom of a new edge into an unchanged subtree). The second checks inclusion of node E when that node was not previously the bottom of an old edge. Supporting imports were added (PATRICIA_KEY_UPPER_BOUND_FELT, BinaryData, NodeData) to place leaves and compute expected node hashes. One test documents a TODO that preimage G may be stricter than the OS needs once witness selection is tightened.

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

[yoavGrs]

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

---

a discussion (no related file):
Can you reuse utils in crates/starknet_committer/src/forest/deleted_nodes_test.rs?

---

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

        response.patricia_proofs.classes_trie_proof.contains_key(&edge_tree_node_e_hash),
        "missing bottom of a new edge node in a proof",
    );

Use your new storage proof verification?

Code quote:

    assert!(
        response.patricia_proofs.classes_trie_proof.contains_key(&edge_tree_node_e_hash),
        "missing bottom of a new edge node in a proof",
    );

[ArielElp]

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

---

a discussion (no related file):

Previously, yoavGrs wrote…

Can you reuse utils in crates/starknet_committer/src/forest/deleted_nodes_test.rs?

Hhhmm doesn't look like it, in addition to being a bit higher level on ApolloCommitter it's testing different things. I don't really care about the deleted nodes on the right side.

---

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

Previously, yoavGrs wrote…

Use your new storage proof verification?

To what leaf? I think the point here is that this case does not add "full paths" to the proof.

[yoavGrs]

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

---

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

    let class_hash_a = ClassHash(0_u64.into());
    let class_hash_b = ClassHash(1_u64.into());
    let class_hash_d = ClassHash(PATRICIA_KEY_UPPER_BOUND_FELT - 1_u64);

Please add another case where this key is 3 or 4:

1. To match the illustration.
2. In this case, the proof should contain a and b, right?

Code quote:

PATRICIA_KEY_UPPER_BOUND_FELT - 1_u64

[dorimedini-starkware]

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

---

crates/apollo_committer/src/request_paths_and_commit_block_tests.rs line 383 at r3 (raw file):

/// 2. Commit block 1 via [crate::committer::Committer::read_paths_and_commit_block], deleting `D`
///    and requesting witnesses only for the deleted key.
/// 3. Assert the returned classes-trie proof contains node `E`.

we actually need A and B here as well, to prove E is binary when converting R to an edge from R to E

Code quote:

Assert the returned classes-trie proof contains node `E`.

---

crates/apollo_committer/src/request_paths_and_commit_block_tests.rs line 452 at r3 (raw file):

        response.patricia_proofs.classes_trie_proof.contains_key(&edge_tree_node_e_hash),
        "missing bottom of a new edge node in a proof",
    );

also assert leaf_a_hash and leaf_b_hash

Code quote:

    assert!(
        response.patricia_proofs.classes_trie_proof.contains_key(&edge_tree_node_e_hash),
        "missing bottom of a new edge node in a proof",
    );

[ArielElp]

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

---

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

Previously, yoavGrs wrote…

Please add another case where this key is 3 or 4:

1. To match the illustration.
2. In this case, the proof should contain a and b, right?

Done.

---

crates/apollo_committer/src/request_paths_and_commit_block_tests.rs line 383 at r3 (raw file):

Previously, dorimedini-starkware wrote…

we actually need A and B here as well, to prove E is binary when converting R to an edge from R to E

It was all messed up, see the new drawing which is hopefully accurate. I think we only need E, and before I was checking G, which was also present...

[dorimedini-starkware]

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

---

crates/apollo_committer/src/request_paths_and_commit_block_tests.rs line 383 at r3 (raw file):

Previously, ArielElp wrote…

It was all messed up, see the new drawing which is hopefully accurate. I think we only need E, and before I was checking G, which was also present...

in the case you drew, I think you really only need E, because E is an edge so it cannot be followed by an edge; thus, connecting the new R to the bottom of E must be legal because the bottom of E is not an edge.
however, the interesting case you should be testing is the previous one, where E is binary with children A,B. In this case you need to prove E was binary originally, so you need A and B...
strange to assert that E is here in this case, when it should be here anyway, even without the edge case

---

crates/apollo_committer/src/request_paths_and_commit_block_tests.rs line 452 at r3 (raw file):

Previously, dorimedini-starkware wrote…

also assert leaf_a_hash and leaf_b_hash

still relevant, see above

---

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

/// 3. Assert the returned classes-trie proof contains node `E`. In this test, E is not the bottom
///    of an existing edge, so it must be included in the proof to become convinced of the new
///    tree's validity.

1. E is needed in the proof anyway, just to be able to compute the new node R after deletion (E is a sibling of a node on the path from a diff to the root)
2. if you don't fetch A and B as well, then E could be the top of an edge node in the original tree; how would you prove the new edge R->E is a valid edge without A and B?

Code quote:

/// 3. Assert the returned classes-trie proof contains node `E`. In this test, E is not the bottom
///    of an existing edge, so it must be included in the proof to become convinced of the new
///    tree's validity.

[dorimedini-starkware]

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

[dorimedini-starkware]

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

[ArielElp]

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

---

crates/apollo_committer/src/request_paths_and_commit_block_tests.rs line 383 at r3 (raw file):

Previously, dorimedini-starkware wrote…

in the case you drew, I think you really only need E, because E is an edge so it cannot be followed by an edge; thus, connecting the new R to the bottom of E must be legal because the bottom of E is not an edge.
however, the interesting case you should be testing is the previous one, where E is binary with children A,B. In this case you need to prove E was binary originally, so you need A and B...
strange to assert that E is here in this case, when it should be here anyway, even without the edge case

I agree that we only need E, I am now asserting both E and G since today we also fetch G, I want it to trigger a failure after Yoav's future changes (see new comments).

---

crates/apollo_committer/src/request_paths_and_commit_block_tests.rs line 452 at r3 (raw file):

Previously, dorimedini-starkware wrote…

actually, we need only the preimage of E in this case, so if the preimage map contains E as a key then it contains A and B as values...? is that enough for the OS?

Yes, we never need A and B, the preimage of E tells us that it is binary.

---

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

Previously, dorimedini-starkware wrote…

1. E is needed in the proof anyway, just to be able to compute the new node R after deletion (E is a sibling of a node on the path from a diff to the root)
2. if you don't fetch A and B as well, then E could be the top of an edge node in the original tree; how would you prove the new edge R->E is a valid edge without A and B?

1. not true, the hash of E appears in T, when I say "appears" I mean exists in the preimages map, i.e. we load its preimages
2. see below

[dorimedini-starkware]

@dorimedini-starkware resolved 3 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on ArielElp).

[ArielElp]

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

[dorimedini-starkware]

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

---

crates/apollo_committer/src/request_paths_and_commit_block_tests.rs line 391 at r7 (raw file):

///    and requesting witnesses only for the deleted key.
/// 3. Assert the returned classes-trie proof contains node `E` (this will allow verifying that G,
///    which appears in the new edge A--G, indeed exists in the old tree).

update the doc?
and can you explain why the OS doesn't need a key E? you need G (preimage of E) to compute the new preimage of R'

Code quote:

/// 3. Assert the returned classes-trie proof contains node `E` (this will allow verifying that G,
///    which appears in the new edge A--G, indeed exists in the old tree).

[ArielElp]

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

---

crates/apollo_committer/src/request_paths_and_commit_block_tests.rs line 391 at r7 (raw file):

Previously, dorimedini-starkware wrote…

update the doc?
and can you explain why the OS doesn't need a key E? you need G (preimage of E) to compute the new preimage of R'

Don't need G, the preimage of R' has the hash of G, so R' can be computed. To bind hash(G) to the old tree, it suffices to take E's hash from the opening of R, modify the path, and check that you can reproduce hash(E) with hash(G) from R' (AFAIU this is happening in patricia.cairo, a bit of me verifying but mostly relying on AI to TAL, information theoretically this is possible though)

[ArielElp]

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

---

a discussion (no related file):

Previously, yoavGrs wrote…

Please define a common util function for the tests that returns the committer's response.

See subsequent PR

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 11ad4ad. Configure here.}

[dorimedini-starkware]

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

[ArielElp]

@ArielElp resolved 2 discussions.
Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on dorimedini-starkware).

[dorimedini-starkware]

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

[ArielElp]

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

[ArielElp]

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

[yoavGrs]

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

[yoavGrs]

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