Narrowing: edge case fixes, documentation, cleanup

 - reverted the tendermint version bump
 - fixed edge cases on trees of total size 0 and 1
 - fixed incorrect implementation in nmt_proof.rs (now just wraps the simple proof.rs)
 - fixed missing test cases for the above two issues
 - cleaned up inconsistent parameter naming to be more clear
 - cleaned up large argument sets in a few places to make data flow easier to follow
 - documented all assertions with a description of why they should never break
 - added documentation to the functions and descriptions of the narrowing logic

Author: theodorebugnet

Cargo.toml

@@ -21,7 +21,7 @@ nmt-rs = { path = ".", features = ["borsh", "serde"] }
 borsh = { version = "1" }
 serde_json = "1.0.96"
 postcard = { version = "1.0.4", features = ["use-std"] }
-tendermint = { version = "0.39.1" }
+tendermint = { version = "0.35.0" }
 
 [features]
 default = ["std"]

src/lib.rs

@@ -422,6 +422,7 @@ pub enum RangeProofType {
 #[cfg(test)]
 mod tests {
     use crate::maybestd::vec::Vec;
+    use crate::simple_merkle::error::RangeProofError;
     use crate::NamespaceMerkleHasher;
     use crate::{
         namespaced_hash::{NamespaceId, NamespacedSha2Hasher},
@@ -458,6 +459,20 @@ mod tests {
         tree
     }
 
+    fn tree_from_one_namespace<const NS_ID_SIZE: usize>(
+        leaves: u64,
+        namespace: u64,
+    ) -> DefaultNmt<NS_ID_SIZE> {
+        let mut tree = DefaultNmt::new();
+        let namespace = ns_id_from_u64(namespace);
+        for i in 0..leaves {
+            let data = format!("leaf_{i}");
+            tree.push_leaf(data.as_bytes(), namespace)
+                .expect("Failed to push the leaf");
+        }
+        tree
+    }
+
     /// Builds a tree with N leaves
     fn tree_with_n_leaves<const NS_ID_SIZE: usize>(n: usize) -> DefaultNmt<NS_ID_SIZE> {
         tree_from_namespace_ids((0..n as u64).collect::<Vec<_>>())
@@ -587,6 +602,63 @@ mod tests {
         }
     }
 
+    fn test_range_proof_narrowing_within_namespace<const NS_ID_SIZE: usize>(n: usize) {
+        let ns_id = 4;
+        let mut tree = tree_from_one_namespace::<NS_ID_SIZE>(n as u64, ns_id); // since there's a single namespace, the actual ID shouldn't matter
+        let root = tree.root();
+        for i in 1..=n {
+            for j in 0..=i {
+                let proof_nmt = NamespaceProof::PresenceProof {
+                    proof: tree.build_range_proof(j..i),
+                    ignore_max_ns: tree.ignore_max_ns,
+                };
+                for k in (j + 1)..=i {
+                    for l in j..=k {
+                        let left_leaf_datas: Vec<_> =
+                            tree.leaves()[j..l].iter().map(|l| l.data()).collect();
+                        let right_leaf_datas: Vec<_> =
+                            tree.leaves()[k..i].iter().map(|l| l.data()).collect();
+                        let narrowed_proof_nmt = proof_nmt.narrow_range(
+                            &left_leaf_datas,
+                            &right_leaf_datas,
+                            ns_id_from_u64(ns_id),
+                        );
+                        if k == l {
+                            // Cannot prove the empty range!
+                            assert!(narrowed_proof_nmt.is_err());
+                            assert_eq!(
+                                narrowed_proof_nmt.unwrap_err(),
+                                RangeProofError::NoLeavesProvided
+                            );
+                            continue;
+                        } else {
+                            assert!(narrowed_proof_nmt.is_ok());
+                        }
+                        let narrowed_proof = narrowed_proof_nmt.unwrap();
+                        let new_leaves: Vec<_> = tree.leaves()[l..k]
+                            .iter()
+                            .map(|l| l.hash().clone())
+                            .collect();
+                        tree.check_range_proof(&root, &new_leaves, narrowed_proof.siblings(), l)
+                            .unwrap();
+                    }
+                }
+            }
+        }
+        test_min_and_max_ns_against(&mut tree)
+    }
+
+    #[test]
+    fn test_range_proof_narrowing_nmt() {
+        for x in 0..20 {
+            test_range_proof_narrowing_within_namespace::<8>(x);
+            test_range_proof_narrowing_within_namespace::<17>(x);
+            test_range_proof_narrowing_within_namespace::<24>(x);
+            test_range_proof_narrowing_within_namespace::<CELESTIA_NS_ID_SIZE>(x);
+            test_range_proof_narrowing_within_namespace::<32>(x);
+        }
+    }
+
     /// Builds a tree with n leaves, and then creates and checks proofs of all valid
     /// ranges, and attempts to narrow every proof and re-check it for the narrowed range
     fn test_range_proof_narrowing_with_n_leaves<const NS_ID_SIZE: usize>(n: usize) {
@@ -595,12 +667,8 @@ mod tests {
         for i in 1..=n {
             for j in 0..=i {
                 let proof = tree.build_range_proof(j..i);
-                let leaf_hashes: Vec<_> = tree.leaves()[j..i]
-                    .iter()
-                    .map(|l| l.hash().clone())
-                    .collect();
-                for k in (j + 1)..i {
-                    for l in j..k {
+                for k in (j + 1)..=i {
+                    for l in j..=k {
                         let left_hashes: Vec<_> = tree.leaves()[j..l]
                             .iter()
                             .map(|l| l.hash().clone())
@@ -609,47 +677,38 @@ mod tests {
                             .iter()
                             .map(|l| l.hash().clone())
                             .collect();
-                        let narrowed_proof = proof
-                            .narrow_range_with_hasher(
-                                &left_hashes,
-                                &right_hashes,
-                                NamespacedSha2Hasher::with_ignore_max_ns(tree.ignore_max_ns),
-                            )
-                            .unwrap();
+                        let narrowed_proof_simple = proof.narrow_range_with_hasher(
+                            &left_hashes,
+                            &right_hashes,
+                            NamespacedSha2Hasher::with_ignore_max_ns(tree.ignore_max_ns),
+                        );
+                        if k == l {
+                            // Cannot prove the empty range!
+                            assert!(narrowed_proof_simple.is_err());
+                            assert_eq!(
+                                narrowed_proof_simple.unwrap_err(),
+                                RangeProofError::NoLeavesProvided
+                            );
+                            continue;
+                        } else {
+                            assert!(narrowed_proof_simple.is_ok());
+                        }
+                        let narrowed_proof = narrowed_proof_simple.unwrap();
                         let new_leaves: Vec<_> = tree.leaves()[l..k]
                             .iter()
                             .map(|l| l.hash().clone())
                             .collect();
-                        let res = tree.check_range_proof(
-                            &root,
-                            &new_leaves,
-                            narrowed_proof.siblings(),
-                            l,
-                        );
-                        if l != k {
-                            assert!(res.is_ok());
-                            assert_eq!(res.unwrap(), RangeProofType::Complete)
-                        } else {
-                            // Cannot prove the empty range!
-                            assert!(res.is_err())
-                        }
+                        tree.check_range_proof(&root, &new_leaves, narrowed_proof.siblings(), l)
+                            .unwrap();
                     }
                 }
-                let res = tree.check_range_proof(&root, &leaf_hashes, proof.siblings(), j);
-                if i != j {
-                    assert!(res.is_ok());
-                    assert_eq!(res.unwrap(), RangeProofType::Complete)
-                } else {
-                    // Cannot prove the empty range!
-                    assert!(res.is_err())
-                }
             }
         }
         test_min_and_max_ns_against(&mut tree)
     }
 
     #[test]
-    fn test_range_proof_narrowing() {
+    fn test_range_proof_narrowing_simple() {
         for x in 0..20 {
             test_range_proof_narrowing_with_n_leaves::<8>(x);
             test_range_proof_narrowing_with_n_leaves::<17>(x);

src/nmt_proof.rs

@@ -100,6 +100,13 @@ where
 
     /// Narrows the proof range: uses an existing proof to create
     /// a new proof for a subrange of the original proof's range
+    ///
+    /// # Arguments
+    ///  - left_extra_raw_leaves: The data for the leaves that will narrow the range from the left
+    ///    side (i.e. all the leaves from the left edge of the currently proven range, to the left
+    ///    edge of the new desired shrunk range)
+    ///  - right_extra_raw_leaves: Analogously, data for all the leaves between the right edge of
+    ///    the desired shrunken range, and the right edge of the current proof's range
     pub fn narrow_range<L: AsRef<[u8]>>(
         &self,
         left_extra_raw_leaves: &[L],
@@ -112,11 +119,6 @@ where
             ));
         }
 
-        let new_leaf_len = left_extra_raw_leaves.len() + right_extra_raw_leaves.len();
-        if new_leaf_len >= self.range_len() {
-            return Err(RangeProofError::WrongAmountOfLeavesProvided);
-        }
-
         let leaves_to_hashes = |l: &[L]| -> Vec<NamespacedHash<NS_ID_SIZE>> {
             l.iter()
                 .map(|data| {
@@ -128,16 +130,10 @@ where
         let left_extra_hashes = leaves_to_hashes(left_extra_raw_leaves);
         let right_extra_hashes = leaves_to_hashes(right_extra_raw_leaves);
 
-        let mut tree = NamespaceMerkleTree::<NoopDb, M, NS_ID_SIZE>::with_hasher(
-            M::with_ignore_max_ns(self.ignores_max_ns()),
-        );
-
-        let proof = tree.inner.narrow_range_proof(
+        let proof = self.merkle_proof().narrow_range_with_hasher(
             &left_extra_hashes,
-            self.start_idx() as usize..(self.range_len() - new_leaf_len),
             &right_extra_hashes,
-            &mut self.siblings(),
-            self.start_idx() as usize,
+            M::with_ignore_max_ns(self.ignores_max_ns()),
         )?;
 
         Ok(Self::PresenceProof {

src/simple_merkle/proof.rs

@@ -1,4 +1,4 @@
-use core::ops::Range;
+use core::{cmp::Ordering, ops::Range};
 
 use super::{
     db::NoopDb,
@@ -84,6 +84,13 @@ where
 
     /// Narrows the proof range: uses an existing proof to create
     /// a new proof for a subrange of the original proof's range
+    ///
+    /// # Arguments
+    ///  - left_extra_leaves: The hashes of the leaves that will narrow the range from the left
+    ///    side (i.e. all the leaves from the left edge of the currently proven range, to the left
+    ///    edge of the new desired shrunk range)
+    ///  - right_extra_leaves: Analogously, hashes of all the leaves between the right edge of
+    ///    the desired shrunken range, and the right edge of the current proof's range
     pub fn narrow_range_with_hasher(
         &self,
         left_extra_leaves: &[M::Output],
@@ -94,9 +101,16 @@ where
             .len()
             .checked_add(right_extra_leaves.len())
             .ok_or(RangeProofError::TreeTooLarge)?;
-        if new_leaf_len >= self.range_len() {
-            return Err(RangeProofError::WrongAmountOfLeavesProvided);
+        match new_leaf_len.cmp(&self.range_len()) {
+            Ordering::Equal => {
+                // We cannot prove the empty range!
+                return Err(RangeProofError::NoLeavesProvided);
+            }
+            Ordering::Greater => return Err(RangeProofError::WrongAmountOfLeavesProvided),
+            Ordering::Less => { /* Ok! */ }
         }
+
+        // Indices relative to the leaves of the entire tree
         let new_start_idx = (self.start_idx() as usize)
             .checked_add(left_extra_leaves.len())
             .ok_or(RangeProofError::TreeTooLarge)?;