starknet_transaction_prover,starknet_patricia: add compute_missing_sibling_keys helper

[Yoni-Starkware]

What

When a storage leaf is deleted, the Patricia storage trie changes shape: the deepest binary node on the path to the deleted key collapses into an edge pointing at its sibling. To produce the canonical post-deletion tree (checked by patricia.cairo), the committer must merge that collapse edge with the sibling — which requires the sibling's preimage. A plain get_storage_proof does not return it: siblings on the path appear only as orphan hashes.

compute_missing_sibling_keys scans the storage deletes in a state_diff, walks each deleted key's proof to the deepest binary node, and returns one crafted storage key per contract. Queried on a follow-up get_storage_proof, each crafted key routes into the sibling's subtree and forces the RPC to expose exactly that sibling's preimage.

It is #[allow(dead_code)] for now and wired into get_storage_proofs in a follow-up.

Why only the deepest sibling

For a single-leaf deletion, only the deepest binary node on the path collapses; every shallower binary node keeps a non-empty on-path child (the collapsed edge) and is merely rehashed, for which the committer already has the sibling's hash (carried by the parent / the dummy node from add_dummy_nodes_for_orphan_hashes, exactly as for non-deletion updates). The sibling's structure is only consumed when it is an edge (node_from_edge_data → concat_paths); when it is binary or a leaf, the existing dummy-node path already yields the correct node. We can't tell an orphan sibling's type from the base proof, so we fetch the deepest one whenever its preimage is absent and the committer ignores it harmlessly if it turns out non-edge.

Edge paths are validated against the key, so a zero-write to a key that isn't in the trie (a no-op) fetches nothing.

Implementation notes

The walk is expressed over NodeIndex. To support it, starknet_patricia exposes get_children_indices and from_felt_value as pub and adds a non-panicking is_descendant_of.

Tests

• orphan sibling on the delete path → the one crafted key;
• two binary nodes on the path → only the deepest sibling is fetched (not the shallow one);
• phantom delete (edge diverges from the key) → empty;
• non-delete writes → empty.

🤖 Generated with Claude Code

[reviewable-StarkWare]

This change is 

[cursor]

PR Summary

Medium Risk
New proof-walking logic affects post-deletion commitment correctness once integrated; behavior is isolated behind dead code until the follow-up wires it into RPC fetching.

Overview
Adds compute_missing_sibling_keys (not yet wired into get_storage_proofs) so storage deletes in a state_diff can drive follow-up get_storage_proof queries: it walks each deleted key’s RPC storage proof and returns crafted storage keys that pull in the deepest collapsing binary sibling’s preimage when that sibling is only an orphan hash.

starknet_patricia exposes NodeIndex::is_descendant_of and makes compute_bottom_index, get_children_indices, and leading_zeros public to support the proof walk.

Tests cover orphan siblings, deepest-only fetch, phantom deletes, empty tries, non-deletes, and incomplete proofs.

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

[cursor]

Cursor Bugbot has reviewed your changes and found 2 potential issues.

^{❌ 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 cf7dc99. Configure here.}

[Yoni-Starkware]

@Yoni-Starkware made 6 comments.
Reviewable status: 0 of 3 files reviewed, 8 unresolved discussions.

---

crates/starknet_transaction_prover/src/running/storage_proofs.rs line 175 at r1 (raw file):

        // `rpc_proof.contracts_storage_proofs` are built together by `prepare_query` and share
        // index order.
        let Some(idx) = query.contract_addresses.iter().position(|a| a == addr) else { continue };

build the addr -> index map outside the for loop

---

crates/starknet_transaction_prover/src/running/storage_proofs.rs line 231 at r1 (raw file):

    while !current_index.is_leaf() {
        let Some(node) = proof_nodes.get(&current_hash) else { break };

Is this reachable? If not, please return an error. If yes, comment.

Code quote:

 else { break };

---

crates/starknet_transaction_prover/src/running/storage_proofs.rs line 232 at r1 (raw file):

    while !current_index.is_leaf() {
        let Some(node) = proof_nodes.get(&current_hash) else { break };
        match node {

Are you sure this code works? you only document the sibling in binary nodes. What if I have a tree with only edges? (i.e., one edge)

Also, I'm not sure the sibling you're taking is the right one. The ancestor of a deleted node in our case is a middle of an edge, and we need the preimage of that edge's bottom. Please revise your solution

---

crates/starknet_transaction_prover/src/running/storage_proofs.rs line 270 at r1 (raw file):

                    )));
                }
                let bottom_index = (current_index << edge_len) + path_index;

This whole code is just to extract the bottom index. Please try to use an existing util from the Patricia crate. If there isn't, extract this part to a function in this file and make the sanity checks more concise

Code quote:

                let edge_len = usize::try_from(en.length).map_err(|_| {
                    ProofProviderError::InvalidProofResponse(format!(
                        "edge node {current_hash:#x} has length {} that doesn't fit in usize",
                        en.length
                    ))
                })?;
                // An edge can't extend past the leaf level in a valid trie; reject malformed proofs
                // here so the index shifts below can't overflow `NodeIndex::MAX`.
                if depth + edge_len > storage_tree_height {
                    return Err(ProofProviderError::InvalidProofResponse(format!(
                        "edge node {current_hash:#x} of length {edge_len} at depth {depth} extends \
                         past the leaf level (tree height {storage_tree_height})"
                    )));
                }
                let edge_len = u8::try_from(edge_len).expect("edge_len <= tree height fits in u8");
                let path_index = NodeIndex::from_felt_value(&en.path);
                // A path must fit in its declared length; stray higher bits mean a malformed proof.
                if path_index >= NodeIndex::ROOT << edge_len {
                    return Err(ProofProviderError::InvalidProofResponse(format!(
                        "edge node {current_hash:#x} path {:#x} exceeds its length {edge_len}",
                        en.path
                    )));
                }
                let bottom_index = (current_index << edge_len) + path_index;

---

crates/starknet_transaction_prover/src/running/storage_proofs.rs line 285 at r1 (raw file):

    let Some((sibling_index, sibling_hash)) = collapse_sibling else { return Ok(None) };
    // A leaf-level sibling is a storage value merged by hash; an already-present sibling needs no
    // follow-up. Only an orphan internal sibling requires a crafted key.

Move the comment below the if

Code quote:

    // A leaf-level sibling is a storage value merged by hash; an already-present sibling needs no
    // follow-up. Only an orphan internal sibling requires a crafted key.

---

crates/starknet_transaction_prover/src/running/storage_proofs.rs line 294 at r1 (raw file):

    while !crafted_leaf_index.is_leaf() {
        crafted_leaf_index = crafted_leaf_index << 1;
    }

Do it in a single line, without looping

Code quote:

    // Any leaf under the sibling exposes its preimage on a follow-up query; descend to the
    // left-most one and convert it back to a storage key (strip the leading `FIRST_LEAF` bit).
    let mut crafted_leaf_index = sibling_index;
    while !crafted_leaf_index.is_leaf() {
        crafted_leaf_index = crafted_leaf_index << 1;
    }

[Yoni-Starkware]

Addressed all comments in the latest push (single amended commit). Verified locally: running::storage_proofs tests pass (13 ok, 1 ignored = the live-RPC test), nightly rustfmt clean, clippy clean on starknet_patricia + starknet_transaction_prover.

Per comment:

• line 175 — build addr→index map outside the loop: done. A HashMap<&ContractAddress, usize> is built once before the loop; the per-delete .position() scan is gone.

• line 231 — is the break reachable?: yes — an incomplete proof, or a zero-write to a key absent from the trie (a no-op delete). Added a comment saying so, and it's now handled: after the walk, if current_index != leaf_index (we didn't reach the deleted leaf) the helper fetches nothing rather than using an unverified collapse_sibling. New test ..._incomplete_walk_returns_empty.

• line 232 — edge-only tree / is the sibling the right one?: The needed node is the deepest binary node's sibling, and that's exactly your "bottom of the edge": when the deepest binary collapses, node_from_edge_data does path.concat_paths(sibling_edge.path), i.e. the committer takes the sibling edge and extends to its bottom. We can't read that node from the base proof (it's an orphan hash there), so we craft a key into its subtree to fetch its preimage; the committer then does the merge. A trie that is a single edge has no binary node, so deleting its only leaf empties the trie and needs nothing — new test ..._single_edge_trie_returns_empty covers it (returns empty).

• line 270 — use a Patricia util for the bottom index: done. Replaced the manual shift/validation with NodeIndex::compute_bottom_index (made pub) fed by the crate's existing Preimage::from(&MerkleNode) conversion. Only one concise guard remains (reject an edge that would extend past the leaf level, to keep the shift from overflowing NodeIndex::MAX).

• line 285 — move the comment below the if: done.

• line 294 — craft the key in a single line, no loop: done — sibling_index << sibling_index.leading_zeros() (made leading_zeros pub), then strip FIRST_LEAF.

Cursor Bugbot:

• None storage root rejects deletes: fixed — a missing storage_root (empty trie) is now treated as a no-op delete (skip), matching build_storage_commitment_infos. New test ..._empty_storage_trie_returns_empty.
• Incomplete walk still crafts keys: fixed by the current_index != leaf_index guard above.

Patricia pub changes are now: get_children_indices, compute_bottom_index, leading_zeros, and the new is_descendant_of (from_felt_value is no longer needed and stays pub(crate)).

[Yoni-Starkware]

Re L231 — agreed, it's an RPC bug. Changed the missing-node case from "return nothing" to a hard InvalidProofResponse error.

Reasoning it's safe to error there: a deleted key is always read before deletion, so it's queried and a complete proof contains its full path → the walk reaches its leaf. A no-op delete of an absent key diverges at an edge (binary nodes always have both children present), which the is_descendant_of(bottom_index) check catches and returns None before any node is missing. So the only way to hit a missing inner node is an incomplete/malformed proof — now surfaced as an error instead of being masked (and turning into a worse "missing preimage" failure later in the committer).

This also made the post-loop "didn't reach the leaf" guard dead code (the loop now only exits at the deleted leaf), so I removed it. The test is updated to assert an error (..._incomplete_proof_errors). Tests + fmt + clippy green.

[Yoni-Starkware]

@Yoni-Starkware resolved 6 discussions.
Reviewable status: 0 of 3 files reviewed, all discussions resolved.