From e82baa35bb9e0eb0ca160e431aa5e9af33afaf8f Mon Sep 17 00:00:00 2001 From: Andrey Khmuro Date: Thu, 17 Oct 2024 17:23:29 +0300 Subject: [PATCH] feat: return error instead of panic during MMR verification (#335) --- CHANGELOG.md | 4 ++- src/merkle/mmr/error.rs | 8 ++++- src/merkle/mmr/peaks.rs | 26 +++++++++++++-- src/merkle/mmr/tests.rs | 68 +++++++++++++++++++++++---------------- src/merkle/path.rs | 15 ++++++--- src/merkle/store/tests.rs | 4 +-- 6 files changed, 85 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a652ffc..bd17037 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,10 +4,12 @@ - Added `Mmr::open()` and `Mmr::peaks()` which rely on `Mmr::open_at()` and `Mmr::peaks()` respectively (#234). - Standardised CI and Makefile across Miden repos (#323). - Added `Smt::compute_mutations()` and `Smt::apply_mutations()` for validation-checked insertions (#327). +- [BREAKING] Changed return value of the `Mmr::verify()` and `MerklePath::verify()` from `bool` to + `Result<>` (#). ## 0.10.1 (2024-09-13) -* Added `Serializable` and `Deserializable` implementations for `PartialMmr` and `InOrderIndex` (#329). +- Added `Serializable` and `Deserializable` implementations for `PartialMmr` and `InOrderIndex` (#329). ## 0.10.0 (2024-08-06) diff --git a/src/merkle/mmr/error.rs b/src/merkle/mmr/error.rs index 33b43c0..5bede2d 100644 --- a/src/merkle/mmr/error.rs +++ b/src/merkle/mmr/error.rs @@ -9,6 +9,7 @@ pub enum MmrError { InvalidPosition(usize), InvalidPeaks, InvalidPeak, + PeakOutOfBounds(usize, usize), InvalidUpdate, UnknownPeak, MerkleError(MerkleError), @@ -22,7 +23,12 @@ impl Display for MmrError { MmrError::InvalidPeak => { write!(fmt, "Peak values does not match merkle path computed root") }, - MmrError::InvalidUpdate => write!(fmt, "Invalid mmr update"), + MmrError::PeakOutOfBounds(peak_idx, peaks_len) => write!( + fmt, + "Requested peak index is {} but the number of peaks is {}", + peak_idx, peaks_len + ), + MmrError::InvalidUpdate => write!(fmt, "Invalid Mmr update"), MmrError::UnknownPeak => { write!(fmt, "Peak not in Mmr") }, diff --git a/src/merkle/mmr/peaks.rs b/src/merkle/mmr/peaks.rs index 16f3e96..ae39aaf 100644 --- a/src/merkle/mmr/peaks.rs +++ b/src/merkle/mmr/peaks.rs @@ -69,6 +69,17 @@ impl MmrPeaks { &self.peaks } + /// Returns the peak by the provided index. + /// + /// # Errors + /// Returns an error if the provided peak index is greater or equal to the current number of + /// peaks in the Mmr. + pub fn get_peak(&self, peak_idx: usize) -> Result<&RpoDigest, MmrError> { + self.peaks + .get(peak_idx) + .ok_or(MmrError::PeakOutOfBounds(peak_idx, self.peaks.len())) + } + /// Converts this [MmrPeaks] into its components: number of leaves and a vector of peaks of /// the underlying MMR. pub fn into_parts(self) -> (usize, Vec) { @@ -84,9 +95,18 @@ impl MmrPeaks { Rpo256::hash_elements(&self.flatten_and_pad_peaks()) } - pub fn verify(&self, value: RpoDigest, opening: MmrProof) -> bool { - let root = &self.peaks[opening.peak_index()]; - opening.merkle_path.verify(opening.relative_pos() as u64, value, root) + /// Verifies the Merkle opening proof. + /// + /// # Errors + /// Returns an error if: + /// - provided opening proof is invalid. + /// - Mmr root value computed using the provided leaf value differs from the actual one. + pub fn verify(&self, value: RpoDigest, opening: MmrProof) -> Result<(), MmrError> { + let root = self.get_peak(opening.peak_index())?; + opening + .merkle_path + .verify(opening.relative_pos() as u64, value, root) + .map_err(MmrError::MerkleError) } /// Flattens and pads the peaks to make hashing inside of the Miden VM easier. diff --git a/src/merkle/mmr/tests.rs b/src/merkle/mmr/tests.rs index 6bf5685..93ea638 100644 --- a/src/merkle/mmr/tests.rs +++ b/src/merkle/mmr/tests.rs @@ -215,10 +215,7 @@ fn test_mmr_open() { assert_eq!(opening.merkle_path, empty); assert_eq!(opening.forest, mmr.forest); assert_eq!(opening.position, 6); - assert!( - mmr.peaks().verify(LEAVES[6], opening), - "MmrProof should be valid for the current accumulator." - ); + mmr.peaks().verify(LEAVES[6], opening).unwrap(); // nodes 4,5 are depth 1 let root_to_path = MerklePath::new(vec![LEAVES[4]]); @@ -228,10 +225,7 @@ fn test_mmr_open() { assert_eq!(opening.merkle_path, root_to_path); assert_eq!(opening.forest, mmr.forest); assert_eq!(opening.position, 5); - assert!( - mmr.peaks().verify(LEAVES[5], opening), - "MmrProof should be valid for the current accumulator." - ); + mmr.peaks().verify(LEAVES[5], opening).unwrap(); let root_to_path = MerklePath::new(vec![LEAVES[5]]); let opening = mmr @@ -240,10 +234,7 @@ fn test_mmr_open() { assert_eq!(opening.merkle_path, root_to_path); assert_eq!(opening.forest, mmr.forest); assert_eq!(opening.position, 4); - assert!( - mmr.peaks().verify(LEAVES[4], opening), - "MmrProof should be valid for the current accumulator." - ); + mmr.peaks().verify(LEAVES[4], opening).unwrap(); // nodes 0,1,2,3 are detph 2 let root_to_path = MerklePath::new(vec![LEAVES[2], h01]); @@ -253,10 +244,7 @@ fn test_mmr_open() { assert_eq!(opening.merkle_path, root_to_path); assert_eq!(opening.forest, mmr.forest); assert_eq!(opening.position, 3); - assert!( - mmr.peaks().verify(LEAVES[3], opening), - "MmrProof should be valid for the current accumulator." - ); + mmr.peaks().verify(LEAVES[3], opening).unwrap(); let root_to_path = MerklePath::new(vec![LEAVES[3], h01]); let opening = mmr @@ -265,10 +253,7 @@ fn test_mmr_open() { assert_eq!(opening.merkle_path, root_to_path); assert_eq!(opening.forest, mmr.forest); assert_eq!(opening.position, 2); - assert!( - mmr.peaks().verify(LEAVES[2], opening), - "MmrProof should be valid for the current accumulator." - ); + mmr.peaks().verify(LEAVES[2], opening).unwrap(); let root_to_path = MerklePath::new(vec![LEAVES[0], h23]); let opening = mmr @@ -277,10 +262,7 @@ fn test_mmr_open() { assert_eq!(opening.merkle_path, root_to_path); assert_eq!(opening.forest, mmr.forest); assert_eq!(opening.position, 1); - assert!( - mmr.peaks().verify(LEAVES[1], opening), - "MmrProof should be valid for the current accumulator." - ); + mmr.peaks().verify(LEAVES[1], opening).unwrap(); let root_to_path = MerklePath::new(vec![LEAVES[1], h23]); let opening = mmr @@ -289,10 +271,7 @@ fn test_mmr_open() { assert_eq!(opening.merkle_path, root_to_path); assert_eq!(opening.forest, mmr.forest); assert_eq!(opening.position, 0); - assert!( - mmr.peaks().verify(LEAVES[0], opening), - "MmrProof should be valid for the current accumulator." - ); + mmr.peaks().verify(LEAVES[0], opening).unwrap(); } #[test] @@ -835,6 +814,39 @@ fn test_mmr_add_invalid_odd_leaf() { assert!(result.is_ok()); } +/// Tests that a proof whose peak count exceeds the peak count of the MMR returns an error. +/// +/// Here we manipulate the proof to return a peak index of 1 while the MMR only has 1 peak (with +/// index 0). +#[test] +#[should_panic] +fn test_mmr_proof_num_peaks_exceeds_current_num_peaks() { + let mmr: Mmr = LEAVES[0..4].iter().cloned().into(); + let mut proof = mmr.open(3).unwrap(); + proof.forest = 5; + proof.position = 4; + mmr.peaks().verify(LEAVES[3], proof).unwrap(); +} + +/// Tests that a proof whose peak count exceeds the peak count of the MMR returns an error. +/// +/// We create an MmrProof for a leaf whose peak index to verify against is 1. +/// Then we add another leaf which results in an Mmr with just one peak due to trees +/// being merged. If we try to use the old proof against the new Mmr, we should get an error. +#[test] +#[should_panic] +fn test_mmr_old_proof_num_peaks_exceeds_current_num_peaks() { + let leaves_len = 3; + let mut mmr = Mmr::from(LEAVES[0..leaves_len].iter().cloned()); + + let leaf_idx = leaves_len - 1; + let proof = mmr.open(leaf_idx).unwrap(); + assert!(mmr.peaks().verify(LEAVES[leaf_idx], proof.clone()).is_ok()); + + mmr.add(LEAVES[leaves_len]); + mmr.peaks().verify(LEAVES[leaf_idx], proof).unwrap(); +} + mod property_tests { use proptest::prelude::*; diff --git a/src/merkle/path.rs b/src/merkle/path.rs index 417e1ac..1800bc1 100644 --- a/src/merkle/path.rs +++ b/src/merkle/path.rs @@ -54,12 +54,17 @@ impl MerklePath { /// Verifies the Merkle opening proof towards the provided root. /// - /// Returns `true` if `node` exists at `index` in a Merkle tree with `root`. - pub fn verify(&self, index: u64, node: RpoDigest, root: &RpoDigest) -> bool { - match self.compute_root(index, node) { - Ok(computed_root) => root == &computed_root, - Err(_) => false, + /// # Errors + /// Returns an error if: + /// - provided node index is invalid. + /// - root calculated during the verification differs from the provided one. + pub fn verify(&self, index: u64, node: RpoDigest, root: &RpoDigest) -> Result<(), MerkleError> { + let computed_root = self.compute_root(index, node)?; + if &computed_root != root { + return Err(MerkleError::ConflictingRoots(vec![computed_root, root.clone()])); } + + Ok(()) } /// Returns an iterator over every inner node of this [MerklePath]. diff --git a/src/merkle/store/tests.rs b/src/merkle/store/tests.rs index f158dc0..fe9ee0f 100644 --- a/src/merkle/store/tests.rs +++ b/src/merkle/store/tests.rs @@ -613,7 +613,7 @@ fn node_path_should_be_truncated_by_midtier_insert() { let path = store.get_path(root, index).unwrap().path; assert_eq!(node, result); assert_eq!(path.depth(), depth); - assert!(path.verify(index.value(), result, &root)); + assert!(path.verify(index.value(), result, &root).is_ok()); // flip the first bit of the key and insert the second node on a different depth let key = key ^ (1 << 63); @@ -626,7 +626,7 @@ fn node_path_should_be_truncated_by_midtier_insert() { let path = store.get_path(root, index).unwrap().path; assert_eq!(node, result); assert_eq!(path.depth(), depth); - assert!(path.verify(index.value(), result, &root)); + assert!(path.verify(index.value(), result, &root).is_ok()); // attempt to fetch a path of the second node to depth 64 // should fail because the previously inserted node will remove its sub-tree from the set