starknet_committer: fetch patricia paths tests

[ArielElp]

No description provided.

[reviewable-StarkWare]

This change is 

[ArielElp]

• apollo_committer_types: add patricia proofs request and response types #13996 : 2 dependent PRs (#13997 , #13998 )
• 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
Test-only refactor and a small pub(crate) visibility change; no production committer logic changes.

Overview
Extends fetch patricia paths coverage so the same scenarios run against both facts and index DB layouts, not only facts storage.

Hand-written rstest cases and Python JSON fixtures are split into rstest_reuse templates (fetch_patricia_paths_inner_cases, fetch_patricia_paths_inner_from_json_cases) with separate *_facts_layout and *_index_layout tests. Shared helpers (fetch_patricia_paths_inner_tester, compute_expected_leaves, parse_json_test_input, convert_trie_to_index_storage) drive a generic NodeLayout path; index runs build storage via traverse_and_convert, now pub(crate) for test reuse.

MockTreeHashFunction gains a TreeHashFunction<MockLeaf> impl (moved out of create_index_tree_test.rs). Tests import fetch_patricia_paths_inner from crate::db::trie_traversal and assert fetched leaves explicitly.

^{Reviewed by Cursor Bugbot for commit e15f135. 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.

[dorimedini-starkware]

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

---

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

        &EmptyKeyContext,
        &mut None,
        false,

why not panic on missing node?

Code quote:

false,

---

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

    let root_index = small_tree_index_to_full(U256::ONE, height);

    // map indices from a small tree to a height 251 tree

Suggestion:

// Map indices from a small tree to a height 251 tree.

[ArielElp]

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

---

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

Previously, dorimedini-starkware wrote…

why not panic on missing node?

I don't think the trees in the example are full (e.g. binary node with no children just because they're not needed for the scan towards the agreed upon leaves)

[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 756c396. Configure here.}

[dorimedini-starkware]

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

[ArielElp]

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

[dorimedini-starkware]

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