starknet_patricia: add storage proof verification#14296
Conversation
d64f472 to
a75f780
Compare
a430dcd to
8f90e28
Compare
PR SummaryMedium Risk Overview The new
Reviewed by Cursor Bugbot for commit 53780cb. Bugbot is set up for automated code reviews on this repo. Configure here. |
8f90e28 to
f76df99
Compare
278e79e to
2ca5a5e
Compare
2b73b72 to
11dc0c3
Compare
2ca5a5e to
dc8b161
Compare
11dc0c3 to
65ddea4
Compare
83d5cd7 to
e1435cb
Compare
647af45 to
c26c606
Compare
e1435cb to
6340321
Compare
yoavGrs
left a comment
There was a problem hiding this comment.
@yoavGrs reviewed 4 files and all commit messages, and made 7 comments.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on ArielElp).
crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 24 at r2 (raw file):
#[derive(Debug, Error, PartialEq, Eq)] pub enum ProofVerificationError {
Usually, in tests, we panic on invalid cases. WDYT?
Code quote:
enum ProofVerificationErrorcrates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 57 at r2 (raw file):
) -> Result<ProofIndexMaps, ProofVerificationError> { let mut hash_by_index = HashMap::from([(NodeIndex::ROOT, root_hash)]); let mut parent_by_index = HashMap::new();
If you want:
impl ProofIndexMaps {
fn insert(index, hash, parent_hash
}
Suggestion, non-blocking.
Suggestion:
let mut index_maps = ProofIndexMaps{
hash_by_index: HashMap::from([(NodeIndex::ROOT, root_hash)]),
parent_by_index: HashMap::new(),
}crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 115 at r2 (raw file):
} /// Verifies that `preimages` contains a path from the root to each of the leaves in `leaf_indices`.
The nodes in the maps are validated,
Suggestion:
`maps`crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 121 at r2 (raw file):
root_hash: HashOutput, leaf_indices: &[NodeIndex], leaf_hashes: &HashMap<NodeIndex, HashOutput>,
Please extract the validation that this hash-map (:smile:) is contained in the hash-by-index map, and remove this arg.
Code quote:
leaf_hashes: &HashMap<NodeIndex, HashOutput>,crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 155 at r2 (raw file):
.ok_or(ProofVerificationError::MissingNode { index: current_index })?; if !is_valid_child(current_index, parent_index, preimage) { return Err(ProofVerificationError::MissingNode { index: current_index });
You are validating it in build_proof_index_maps.
Code quote:
if !is_valid_child(current_index, parent_index, preimage) {
return Err(ProofVerificationError::MissingNode { index: current_index });crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 169 at r2 (raw file):
}), None => Err(ProofVerificationError::MissingNode { index: NodeIndex::ROOT }), }
- It's not part of the loop.
- You inserted the root value in
build_proof_index_maps. You don't need to check it.
Code quote:
match maps.hash_by_index.get(&NodeIndex::ROOT) {
Some(proof_value) if *proof_value == root_hash => Ok(()),
Some(proof_value) => Err(ProofVerificationError::HashMismatch {
index: NodeIndex::ROOT,
proof_value: *proof_value,
actual: root_hash,
}),
None => Err(ProofVerificationError::MissingNode { index: NodeIndex::ROOT }),
}
yoavGrs
left a comment
There was a problem hiding this comment.
@yoavGrs reviewed 4 files and all commit messages, and made 7 comments.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on ArielElp).
crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 24 at r2 (raw file):
#[derive(Debug, Error, PartialEq, Eq)] pub enum ProofVerificationError {
Usually, in tests, we panic on invalid cases. WDYT?
Code quote:
enum ProofVerificationErrorcrates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 57 at r2 (raw file):
) -> Result<ProofIndexMaps, ProofVerificationError> { let mut hash_by_index = HashMap::from([(NodeIndex::ROOT, root_hash)]); let mut parent_by_index = HashMap::new();
If you want:
impl ProofIndexMaps {
fn insert(index, hash, parent_hash
}
Suggestion, non-blocking.
Suggestion:
let mut index_maps = ProofIndexMaps{
hash_by_index: HashMap::from([(NodeIndex::ROOT, root_hash)]),
parent_by_index: HashMap::new(),
}crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 115 at r2 (raw file):
} /// Verifies that `preimages` contains a path from the root to each of the leaves in `leaf_indices`.
The nodes in the maps are validated,
Suggestion:
`maps`crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 121 at r2 (raw file):
root_hash: HashOutput, leaf_indices: &[NodeIndex], leaf_hashes: &HashMap<NodeIndex, HashOutput>,
Please extract the validation that this hash-map (:smile:) is contained in the hash-by-index map, and remove this arg.
Code quote:
leaf_hashes: &HashMap<NodeIndex, HashOutput>,crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 155 at r2 (raw file):
.ok_or(ProofVerificationError::MissingNode { index: current_index })?; if !is_valid_child(current_index, parent_index, preimage) { return Err(ProofVerificationError::MissingNode { index: current_index });
You are validating it in build_proof_index_maps.
Code quote:
if !is_valid_child(current_index, parent_index, preimage) {
return Err(ProofVerificationError::MissingNode { index: current_index });crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 169 at r2 (raw file):
}), None => Err(ProofVerificationError::MissingNode { index: NodeIndex::ROOT }), }
- It's not part of the loop.
- You inserted the root value in
build_proof_index_maps. You don't need to check it.
Code quote:
match maps.hash_by_index.get(&NodeIndex::ROOT) {
Some(proof_value) if *proof_value == root_hash => Ok(()),
Some(proof_value) => Err(ProofVerificationError::HashMismatch {
index: NodeIndex::ROOT,
proof_value: *proof_value,
actual: root_hash,
}),
None => Err(ProofVerificationError::MissingNode { index: NodeIndex::ROOT }),
}
yoavGrs
left a comment
There was a problem hiding this comment.
@yoavGrs reviewed 4 files and all commit messages, and made 7 comments.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on ArielElp).
crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 24 at r2 (raw file):
#[derive(Debug, Error, PartialEq, Eq)] pub enum ProofVerificationError {
Usually, in tests, we panic on invalid cases. WDYT?
Code quote:
enum ProofVerificationErrorcrates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 57 at r2 (raw file):
) -> Result<ProofIndexMaps, ProofVerificationError> { let mut hash_by_index = HashMap::from([(NodeIndex::ROOT, root_hash)]); let mut parent_by_index = HashMap::new();
If you want:
impl ProofIndexMaps {
fn insert(index, hash, parent_hash
}
Suggestion, non-blocking.
Suggestion:
let mut index_maps = ProofIndexMaps{
hash_by_index: HashMap::from([(NodeIndex::ROOT, root_hash)]),
parent_by_index: HashMap::new(),
}crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 115 at r2 (raw file):
} /// Verifies that `preimages` contains a path from the root to each of the leaves in `leaf_indices`.
The nodes in the maps are validated,
Suggestion:
`maps`crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 121 at r2 (raw file):
root_hash: HashOutput, leaf_indices: &[NodeIndex], leaf_hashes: &HashMap<NodeIndex, HashOutput>,
Please extract the validation that this hash-map (:smile:) is contained in the hash-by-index map, and remove this arg.
Code quote:
leaf_hashes: &HashMap<NodeIndex, HashOutput>,crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 155 at r2 (raw file):
.ok_or(ProofVerificationError::MissingNode { index: current_index })?; if !is_valid_child(current_index, parent_index, preimage) { return Err(ProofVerificationError::MissingNode { index: current_index });
You are validating it in build_proof_index_maps.
Code quote:
if !is_valid_child(current_index, parent_index, preimage) {
return Err(ProofVerificationError::MissingNode { index: current_index });crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 169 at r2 (raw file):
}), None => Err(ProofVerificationError::MissingNode { index: NodeIndex::ROOT }), }
- It's not part of the loop.
- You inserted the root value in
build_proof_index_maps. You don't need to check it.
Code quote:
match maps.hash_by_index.get(&NodeIndex::ROOT) {
Some(proof_value) if *proof_value == root_hash => Ok(()),
Some(proof_value) => Err(ProofVerificationError::HashMismatch {
index: NodeIndex::ROOT,
proof_value: *proof_value,
actual: root_hash,
}),
None => Err(ProofVerificationError::MissingNode { index: NodeIndex::ROOT }),
}
ArielElp
left a comment
There was a problem hiding this comment.
@ArielElp made 6 comments and resolved 3 discussions.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on yoavGrs).
crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 24 at r2 (raw file):
Previously, yoavGrs wrote…
Usually, in tests, we panic on invalid cases. WDYT?
I prefer to pinpoint what goes wrong in the proof. May also allow easier transition of this into prod code if we ever want it in the future.
crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 57 at r2 (raw file):
Previously, yoavGrs wrote…
If you want:
impl ProofIndexMaps { fn insert(index, hash, parent_hash }Suggestion, non-blocking.
Resolved given that there's only one map now
crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 115 at r2 (raw file):
Previously, yoavGrs wrote…
The nodes in the maps are validated,
maps is just a byproduct that helps me write the traversal more conveniently, the source of truth is preimages (every step "claimed" by the maps is verified w.r.t preimages).
crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 121 at r2 (raw file):
Previously, yoavGrs wrote…
Please extract the validation that this hash-map (:smile:) is contained in the hash-by-index map, and remove this arg.
Done.
crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 155 at r2 (raw file):
Previously, yoavGrs wrote…
You are validating it in
build_proof_index_maps.
Right, removed.
crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 169 at r2 (raw file):
Previously, yoavGrs wrote…
- It's not part of the loop.
- You inserted the root value in
build_proof_index_maps. You don't need to check it.
Right...
I realize now after separating hash verification from the traversal, that there is no need for a bottom-up traversal at all. It suffices to:
- Build an index-->hash map from
preimages, setting the children indices along the way (i.e. proof doesn't control them), and verify hashes - Verify that the expected leaf hashes are in the map from 1 (leaves get to 1 from their parent's preimage)
So I can trim much more, no need for the index-->parent mapping
221ec08 to
c96290d
Compare
There was a problem hiding this comment.
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 c96290d. Configure here.
yoavGrs
left a comment
There was a problem hiding this comment.
@yoavGrs reviewed 2 files and all commit messages, made 5 comments, and resolved 4 discussions.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on ArielElp).
crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 32 at r4 (raw file):
preimages: &PreimageMap, leaf_indices: &[NodeIndex], leaf_hashes: &HashMap<NodeIndex, HashOutput>,
It seems to be enough.
Suggestion:
leaf_hashes: &HashMap<NodeIndex, HashOutput>,crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 36 at r4 (raw file):
if leaf_indices.is_empty() { return Ok(()); }
Can you remove it?
Code quote:
if leaf_indices.is_empty() {
return Ok(());
}crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 70 at r4 (raw file):
root_hash: HashOutput, preimages: &PreimageMap, ) -> Result<HashMap<NodeIndex, HashOutput>, ProofVerificationError> {
Doc the returned object.
Code quote:
HashMap<NodeIndex, HashOutput>crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 78 at r4 (raw file):
let Some(preimage) = preimages.get(&hash) else { continue;
Please add a comment about the state at that point.
Code quote:
continue;crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 78 at r4 (raw file):
let Some(preimage) = preimages.get(&hash) else { continue;
Can you collect only these nodes into hash_by_index? So you return leaves + unmodified subtree roots - the nodes are not in preimages?
Code quote:
continue;c96290d to
293201d
Compare
ArielElp
left a comment
There was a problem hiding this comment.
@ArielElp made 5 comments and resolved 1 discussion.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on yoavGrs).
crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 32 at r4 (raw file):
Previously, yoavGrs wrote…
It seems to be enough.
Done.
crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 36 at r4 (raw file):
Previously, yoavGrs wrote…
Can you remove it?
Yes, this is not important for soundness or anything, but it makes sense IMO. No need to check preimages if you don't care about any leaf.
crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 70 at r4 (raw file):
Previously, yoavGrs wrote…
Doc the returned object.
There is already doc over this function that starts with:
/// Builds an `index -> hash` map by expanding a Patricia proof from the root.
I think the mapping itself is clear from the types (index --> hash).
crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 78 at r4 (raw file):
Previously, yoavGrs wrote…
Please add a comment about the state at that point.
Done.
crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 78 at r4 (raw file):
Previously, yoavGrs wrote…
Can you collect only these nodes into
hash_by_index? So you return leaves + unmodified subtree roots - the nodes are not inpreimages?
Why only these nodes? More intuitive to just build the entire mapping IMO
yoavGrs
left a comment
There was a problem hiding this comment.
@yoavGrs reviewed 1 file and all commit messages, made 2 comments, and resolved 5 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on ArielElp).
crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 78 at r4 (raw file):
Previously, ArielElp wrote…
Why only these nodes? More intuitive to just build the entire mapping IMO
Because you already have the preimages as input. It makes sense to return the new nodes.
I suggest using a queue of hashes (instead of indices), and appending to ' hash_by_index' only the nodes that enter the continue step.
crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 74 at r5 (raw file):
let Some(preimage) = preimages.get(&hash) else { // We reach a child-node of a node in `preimages` that is supposedly not required in // the proof (e.g. sibling of a node with no requested leaves in its subtree).
Or a leaf?
Code quote:
sibling of a node with no requested leaves in its subtree
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware reviewed 4 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on ArielElp).
crates/starknet_patricia/src/patricia_merkle_tree.rs line 5 at r5 (raw file):
pub mod node_data; pub mod original_skeleton_tree; #[cfg(feature = "testing")]
non-blocking, but these usually come together; if there are no tests in the patricia crate that will use it then it's ok to keep it as is
Suggestion:
#[cfg(any(test, feature = "testing"))]crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 74 at r5 (raw file):
let Some(preimage) = preimages.get(&hash) else { // We reach a child-node of a node in `preimages` that is supposedly not required in // the proof (e.g. sibling of a node with no requested leaves in its subtree).
I'm confused, isn't this what you mean?
Suggestion:
// We reach a child-node of a node in `preimages` that is supposedly not required in
// the proof (e.g. child of a sibling of a node with requested leaves in its subtree).293201d to
53780cb
Compare
ArielElp
left a comment
There was a problem hiding this comment.
@ArielElp made 3 comments.
Reviewable status: 3 of 4 files reviewed, 4 unresolved discussions (waiting on dorimedini-starkware and yoavGrs).
crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 78 at r4 (raw file):
Previously, yoavGrs wrote…
Because you already have the preimages as input. It makes sense to return the new nodes.
I suggest using a queue of hashes (instead of indices), and appending to ' hash_by_index' only the nodes that enter thecontinuestep.
It feels somewhat random to return dead ends of preimages rather than the validated nodes in a nice tree structure.
crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 74 at r5 (raw file):
Previously, yoavGrs wrote…
Or a leaf?
Done.
crates/starknet_patricia/src/patricia_merkle_tree/storage_proof_verification.rs line 74 at r5 (raw file):
Previously, dorimedini-starkware wrote…
I'm confused, isn't this what you mean?
No, but maybe I need to rephrase to make it clear. I mean "dead ends" referrenced in preimages, which if honest, are unmodified children of modified nodes.
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware reviewed 1 file and all commit messages, and resolved 1 discussion.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on ArielElp and yoavGrs).
ArielElp
left a comment
There was a problem hiding this comment.
@ArielElp made 1 comment and resolved 1 discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on yoavGrs).
crates/starknet_patricia/src/patricia_merkle_tree.rs line 5 at r5 (raw file):
Previously, dorimedini-starkware wrote…
non-blocking, but these usually come together; if there are no tests in the patricia crate that will use it then it's ok to keep it as is
At the moment it's only used in apollo_committer
yoavGrs
left a comment
There was a problem hiding this comment.
@yoavGrs reviewed 1 file and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on ArielElp).
yoavGrs
left a comment
There was a problem hiding this comment.
@yoavGrs made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on ArielElp).
dorimedini-starkware
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on ArielElp).


No description provided.