Ancestry proof #1
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
More opaque wrt. code duplication, but less cognitive overhead for review.
acatangiu
approved these changes
Mar 26, 2024
serban300
approved these changes
Mar 27, 2024
serban300
reviewed
Apr 5, 2024
Comment on lines
+196
to
+206
| // handle special if next queue item is descendant of sibling | ||
| else if let Some(&(front_pos, ..)) = queue.front() { | ||
| if height > 0 && is_descendant_pos(sib_pos, front_pos) { | ||
| queue.push_back((pos, item, height)); | ||
| continue; | ||
| } else { | ||
| return Err(Error::CorruptedProof); | ||
| } | ||
| } else { | ||
| return Err(Error::CorruptedProof); | ||
| }; |
There was a problem hiding this comment.
Is this needed ? Do you have an example input where this branch would be reached ?
Comment on lines
+335
to
+342
| fn take_while_vec<T, P: Fn(&T) -> bool>(v: &mut Vec<T>, p: P) -> Vec<T> { | ||
| for i in 0..v.len() { | ||
| if !p(&v[i]) { | ||
| return v.drain(..i).collect(); | ||
| } | ||
| } | ||
| v.drain(..).collect() | ||
| } |
There was a problem hiding this comment.
Nit: I would just use crate::mmr::take_while_vec() instead of duplicating it.
Comment on lines
+306
to
+315
| pub fn bagging_peaks_hashes<T, M: Merge<Item = T>>(mut peaks_hashes: Vec<T>) -> Result<T> { | ||
| // bagging peaks | ||
| // bagging from right to left via hash(right, left). | ||
| while peaks_hashes.len() > 1 { | ||
| let right_peak = peaks_hashes.pop().expect("pop"); | ||
| let left_peak = peaks_hashes.pop().expect("pop"); | ||
| peaks_hashes.push(M::merge_peaks(&right_peak, &left_peak)?); | ||
| } | ||
| peaks_hashes.pop().ok_or(Error::CorruptedProof) | ||
| } |
There was a problem hiding this comment.
Nit: I would just use crate::mmr::bagging_peaks_hashes() instead of duplicating it.
Merged
serban300
reviewed
May 10, 2024
There was a problem hiding this comment.
Took a more in-depth look. Overall the approach seems sane. Just opened a PR with some small improvements here: #2
@Lederstrumpf could you take a look please ?
This was referenced May 20, 2024
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Implements ancestry proofs for MMRs:
For a given MMR with size
nand rootr, an ancestry proof proves that some other rootr'was the root of the MMR when it had sizen' < n. If this is the case, we say rootr'ancestors rootr.These ancestry proofs are used in paritytech/polkadot-sdk#1903 to prove a correct ancestor of the current mmr root to the runtime.
Closes paritytech/polkadot-sdk#1441
Implementation notes
While root
r'is not a node in themmr(n), all of its associated peaksp': peaks(n')are, see below illustration:Hence, to prove
r'ancestorsr, we can:p' ∈ peaks(n')in mmr(n)calculate_root(peaks(n')) = r'To perform 1. we adjust the proof structure to also permit membership proofs for nodes, not just strictly leaves.
To support this the proof items also include the position of the nodes:
and the proof verification is adjusted to support this.
While this was previously performed inline for
mmr::MerkleProof(https://github.com/Lederstrumpf/merkle-mountain-range/tree/linearize-proof) and therefore also affected standard leaf proofs, this PR employs dedicated node merkle proofs implemented inancestry_proof::NodeMerkleProof: https://github.com/lederstrumpf/merkle-mountain-range/tree/14edd7ceecccfec3118114f988d7dc9ef925ac20/src/ancestry_proof.rs.While this leads to greater code duplication than adjusting
mmr::MerkleProofto support proving node membership, they are (currently) less performant than the unadjusted leaf proofs and also unaudited. Hence, having a dedicated implementation avoids having a reaudit on areas of the code that use standard membership proofs.