feat: return error instead of panic during MMR verification (#335)

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)
 

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")
             },

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<RpoDigest>) {
@@ -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.

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::*;
 

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].

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