Skip to content

starknet_transaction_prover: add compute_missing_sibling_keys helper#14118

Open
AvivYossef-starkware wants to merge 1 commit into
mainfrom
05-20-starknet_transaction_prover_add_compute_missing_sibling_keys_helper
Open

starknet_transaction_prover: add compute_missing_sibling_keys helper#14118
AvivYossef-starkware wants to merge 1 commit into
mainfrom
05-20-starknet_transaction_prover_add_compute_missing_sibling_keys_helper

Conversation

@AvivYossef-starkware
Copy link
Copy Markdown
Contributor

No description provided.

@reviewable-StarkWare
Copy link
Copy Markdown

This change is Reviewable

Copy link
Copy Markdown
Contributor Author

AvivYossef-starkware commented May 20, 2026

@cursor
Copy link
Copy Markdown

cursor Bot commented May 20, 2026

PR Summary

Medium Risk
Adds new trie-walking bit-level logic for deriving crafted storage keys from RPC proofs; correctness depends on path/bit-offset calculations and proof node shape. Not yet wired into production flow, but could affect future proof fetching/canonicalization for delete diffs.

Overview
Adds compute_missing_sibling_keys to derive additional crafted storage keys for deleted entries in StateMaps, by walking each contract’s storage-trie proof and identifying missing inner-node sibling preimages needed for canonicalization after deletions.

Introduces supporting helpers to map contract addresses to storage roots/proof nodes, traverse BinaryNode/EdgeNode paths using a KEY_BIT_OFFSET, and construct sibling keys at specific depths; adds unit tests building minimal tries to validate crafted-key output and the no-delete empty case.

Reviewed by Cursor Bugbot for commit faa7f62. Bugbot is set up for automated code reviews on this repo. Configure here.

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Fix All in Cursor

❌ 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 faa7f62. Configure here.

(0..edge_len).all(|i| edge_bits[edge_start + i] == key_bits[key_start + i]);
if !edge_matches {
// Phantom delete: the key doesn't exist in this trie.
return crafted;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Phantom delete returns non-empty crafted keys unnecessarily

Low Severity

When an edge path doesn't match the key (phantom delete — the key doesn't exist in the trie), collect_missing_siblings_for_key returns crafted which may already contain entries accumulated from binary nodes traversed earlier on the path. Since the deletion is a no-op, no sibling preimages are needed. The outer function compute_missing_sibling_keys documents it "returns an empty vec when…the deleted keys don't exist in the trie," but the implementation violates this contract. Returning Vec::new() instead of crafted on the phantom-delete branch would match the documented behavior.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit faa7f62. Configure here.

contract_address: *addr.0.key(),
storage_keys: keys.into_iter().collect(),
})
.collect()
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-deterministic contract ordering in output vector

Low Severity

keys_by_contract is a HashMap, so into_iter() yields contracts in non-deterministic order across runs. The codebase explicitly cares about deterministic ordering — prepare_query sorts contract addresses "for deterministic ordering (for offline replay mode)." When this helper is wired into get_storage_proofs in the follow-up PR, the non-deterministic Vec<ContractStorageKeys> output could break replay reproducibility or cause mismatched positional pairing with RPC responses. Using a BTreeMap (or sorting the output) would match the codebase convention.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit faa7f62. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants