starknet_committer: fetch patricia paths 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

• apollo_committer: add new request handler #14003
• starknet_committer: delete patricia paths on reverts #14002
• 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
• starknet_committer: db keys for proofs and accessed keys digest #13997
• apollo_committer_types: add patricia proofs request and response types #13996
• 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 👈 (View in Graphite)
• 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
Changes are limited to test refactors and test-only utilities/hash implementations; production traversal logic is not modified, so risk is mainly around test correctness and brittleness.

Overview
Refactors fetch_patricia_paths_inner traversal tests to be layout-agnostic by introducing a generic test harness (fetch_patricia_paths_inner_tester) and reusing the same case set for both FactsNodeLayout and IndexNodeLayout.

Adds helpers to compute expected fetched leaves and to convert a Facts-layout trie into Index-layout storage via traverse_and_convert (now pub(crate) for test reuse). Python JSON fixture parsing is extracted into parse_json_test_input, and the JSON-based tests are duplicated to validate both layouts.

Moves the MockTreeHashFunction implementation for MockLeaf into hash_function/mock_hash.rs (removing the local impl in create_index_tree_test.rs) so Index-layout tests can hash MockLeaf consistently.

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

[yoavGrs]

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

---

crates/starknet_committer/src/db/facts_db/traversal_test.rs line 6 at r1 (raw file):

use pretty_assertions::assert_eq;
use rstest::rstest;
use rstest_reuse::{apply, template};

Nice :)

Code quote:

use rstest_reuse::{apply, template};

---

crates/starknet_committer/src/db/facts_db/traversal_test.rs line 79 at r1 (raw file):

async fn convert_trie_to_index_storage(
    storage: &mut MapStorage,

Suggestion:

facts_storage: &mut MapStorage,

---

crates/starknet_committer/src/db/facts_db/traversal_test.rs line 97 at r1 (raw file):

}

async fn test_fetch_patricia_paths_inner_impl<Layout>(

Tests should start with a test_ prefix, not helper functions.
(I know it's not your code)

Code quote:

test_fetch_patricia_paths_inner_impl

---

crates/starknet_committer/src/db/facts_db/traversal_test.rs line 107 at r1 (raw file):

    for<'a> Layout: NodeLayout<'a, MockLeaf>,
{
    let root_index = small_tree_index_to_full(U256::ONE, height);

It's always 1, no?

Suggestion:

let root_index = NodeIndex::ROOT;

---

crates/starknet_committer/src/db/facts_db/traversal_test.rs line 108 at r1 (raw file):

{
    let root_index = small_tree_index_to_full(U256::ONE, height);
    let mut leaf_indices_full = leaf_indices

Better?

Suggestion:

leaf_full_indices

---

crates/starknet_committer/src/db/facts_db/traversal_test.rs line 142 at r1 (raw file):

}

#[template]

I'm not blocking, but I think this is one of the cases where parameterization makes it difficult to understand.

Code quality:

Code snippet:

Using parametrization in a test (using rstest)
can make tests harder to reason about and debug in some cases,
when the parametrization is very complicated and involves complex types.
In those cases one should consider a free-function that performs the test,
and several one-line tests that call it with different args,
rather than forcing a parameterized solution.

---

crates/starknet_committer/src/db/facts_db/traversal_test.rs line 625 at r1 (raw file):

) {
    let expected_fetched_leaves = compute_expected_leaves(&storage, &leaf_indices, height);
    let root_index = small_tree_index_to_full(U256::ONE, height);

As above

Suggestion:

let root_index = NodeIndex::ROOT;

---

crates/starknet_committer/src/db/facts_db/traversal_test.rs line 648 at r1 (raw file):

}

fn parse_json_test_input(

Document the output.

Code quote:

parse_json_test_input(

---

crates/starknet_committer/src/db/facts_db/traversal_test.rs line 747 at r1 (raw file):

        parse_json_test_input(input_data);
    let expected_fetched_leaves = compute_expected_leaves(&storage, &leaf_indices, height);
    let root_index = small_tree_index_to_full(U256::ONE, height);

Same here.

Code quote:

small_tree_index_to_full(U256::ONE, height);

[ArielElp]

@ArielElp made 8 comments.
Reviewable status: 3 of 4 files reviewed, 9 unresolved discussions (waiting on yoavGrs).

---

crates/starknet_committer/src/db/facts_db/traversal_test.rs line 97 at r1 (raw file):

Previously, yoavGrs wrote…

Tests should start with a test_ prefix, not helper functions.
(I know it's not your code)

Renamed and added some docs

---

crates/starknet_committer/src/db/facts_db/traversal_test.rs line 107 at r1 (raw file):

Previously, yoavGrs wrote…

It's always 1, no?

No, we're submitting small subtrees to this function

---

crates/starknet_committer/src/db/facts_db/traversal_test.rs line 108 at r1 (raw file):

Previously, yoavGrs wrote…

Better?

Given the new comment I think it's better as-is

---

crates/starknet_committer/src/db/facts_db/traversal_test.rs line 142 at r1 (raw file):

Previously, yoavGrs wrote…

I'm not blocking, but I think this is one of the cases where parameterization makes it difficult to understand.

Code quality:

Why though? IMO it screams template, you have your trees (worked hard to come up with the examples once), and want to test them with two storage layouts.

---

crates/starknet_committer/src/db/facts_db/traversal_test.rs line 625 at r1 (raw file):

Previously, yoavGrs wrote…

As above

Same

---

crates/starknet_committer/src/db/facts_db/traversal_test.rs line 648 at r1 (raw file):

Previously, yoavGrs wrote…

Document the output.

Done.

---

crates/starknet_committer/src/db/facts_db/traversal_test.rs line 747 at r1 (raw file):

Previously, yoavGrs wrote…

Same here.

Same

---

crates/starknet_committer/src/db/facts_db/traversal_test.rs line 79 at r1 (raw file):

async fn convert_trie_to_index_storage(
    storage: &mut MapStorage,

Done.

[ArielElp]

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

---

crates/starknet_committer/src/db/facts_db/traversal_test.rs line 6 at r1 (raw file):

Previously, yoavGrs wrote…

Nice :)

toda!

[yoavGrs]

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

---

crates/starknet_committer/src/db/facts_db/traversal_test.rs line 142 at r1 (raw file):

Previously, ArielElp wrote…

Why though? IMO it screams template, you have your trees (worked hard to come up with the examples once), and want to test them with two storage layouts.

The template concept fits here, sure.
My issue is with complicated Rust objects wrapped by a macro.