Merge pull request #124 from zcash/merge_shardtree_hotfix

Merge `shardtree` hotfixes back to `main` & update `shardtree` to `incrementalmerkletree 0.8.0`

Cargo.toml

@@ -18,7 +18,7 @@ categories = ["algorithms", "data-structures"]
 
 [workspace.dependencies]
 # Intra-workspace dependencies
-incrementalmerkletree = { version = "0.7", path = "incrementalmerkletree" }
+incrementalmerkletree = { version = "0.8", path = "incrementalmerkletree" }
 incrementalmerkletree-testing = { version = "0.2", path = "incrementalmerkletree-testing" }
 
 # Testing

incrementalmerkletree-testing/Cargo.toml

@@ -18,6 +18,6 @@ all-features = true
 rustdoc-args = ["--cfg", "docsrs"]
 
 [dependencies]
-incrementalmerkletree = { version = "0.7", features = ["test-dependencies"] }
+incrementalmerkletree = { workspace = true, features = ["test-dependencies"] }
 
 proptest.workspace = true

shardtree/CHANGELOG.md

@@ -132,6 +132,9 @@ pruning in the presence of inserted `Frontier` nodes. See the `Removed` and
   annotation data being discarded when pruning a `Parent` node having
   `Nil` nodes for both its left and right children.
 
+## [0.3.2] - 2024-12-09
+- Replaces `unwrap` calls with `expect` calls & documents panics.
+
 ## [0.3.1] - 2024-04-03
 
 ### Fixed

shardtree/Cargo.toml

@@ -20,14 +20,14 @@ rustdoc-args = ["--cfg", "docsrs"]
 assert_matches = { version = "1.5", optional = true }
 bitflags = "2"
 either = "1.8"
-incrementalmerkletree = "0.7"
+incrementalmerkletree.workspace = true
 proptest = { workspace = true, optional = true }
 incrementalmerkletree-testing = { workspace = true, optional = true }
 tracing = "0.1"
 
 [dev-dependencies]
 assert_matches = "1.5"
-incrementalmerkletree = { version = "0.7", features = ["test-dependencies"] }
+incrementalmerkletree = { workspace = true, features = ["test-dependencies"] }
 incrementalmerkletree-testing.workspace = true
 proptest.workspace = true
 

shardtree/src/legacy.rs

@@ -169,6 +169,10 @@ impl<H: Hashable + Clone + PartialEq> LocatedPrunableTree<H> {
     ///
     /// Returns a copy of this tree updated to include the witness nodes, any partial supertree that is
     /// produced from nodes "higher" in the witness tree
+    ///
+    /// # Panics
+    ///
+    /// Panics if `witness` corresponds to the empty tree.
     pub fn insert_witness_nodes<C, const DEPTH: u8>(
         &self,
         witness: IncrementalWitness<H, DEPTH>,
@@ -179,7 +183,11 @@ impl<H: Hashable + Clone + PartialEq> LocatedPrunableTree<H> {
             // construct the subtree and cap based on the frontier containing the
             // witnessed position
             let (past_subtree, past_supertree) = self.insert_frontier_nodes::<C>(
-                witness.tree().to_frontier().take().unwrap(),
+                witness
+                    .tree()
+                    .to_frontier()
+                    .take()
+                    .expect("IncrementalWitness cannot be constructed for the empty tree."),
                 &Retention::Marked,
             )?;
 
@@ -253,7 +261,7 @@ mod tests {
         for c in 'a'..'h' {
             base_tree.append(c.to_string()).unwrap();
         }
-        let mut witness = IncrementalWitness::from_tree(base_tree);
+        let mut witness = IncrementalWitness::from_tree(base_tree).unwrap();
         for c in 'h'..'z' {
             witness.append(c.to_string()).unwrap();
         }
@@ -301,7 +309,7 @@ mod tests {
         for c in 'a'..='c' {
             base_tree.append(c.to_string()).unwrap();
         }
-        let mut witness = IncrementalWitness::from_tree(base_tree);
+        let mut witness = IncrementalWitness::from_tree(base_tree).unwrap();
         witness.append("d".to_string()).unwrap();
 
         let root_addr = Address::from_parts(Level::from(3), 0);

shardtree/src/lib.rs

@@ -405,6 +405,10 @@ impl<
     }
 
     /// Adds a checkpoint at the rightmost leaf state of the tree.
+    ///
+    /// # Panics
+    ///
+    /// Panics if `root` represents a parent node but `root_addr` is a depth-0 leaf address.
     pub fn checkpoint(&mut self, checkpoint_id: C) -> Result<bool, ShardTreeError<S::Error>> {
         /// Pre-condition: `root_addr` must be the address of `root`.
         fn go<H: Hashable + Clone + PartialEq>(

shardtree/src/prunable.rs

@@ -1,6 +1,7 @@
 //! Helpers for working with trees that support pruning unneeded leaves and branches.
 
 use std::collections::{BTreeMap, BTreeSet};
+use std::fmt;
 use std::sync::Arc;
 
 use bitflags::bitflags;
@@ -78,6 +79,37 @@ impl<C> From<Retention<C>> for RetentionFlags {
 /// A [`Tree`] annotated with Merkle hashes.
 pub type PrunableTree<H> = Tree<Option<Arc<H>>, (H, RetentionFlags)>;
 
+/// Errors that can occur when merging trees.
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub enum MergeError {
+    Conflict(Address),
+    TreeMalformed(Address),
+}
+
+impl From<MergeError> for InsertionError {
+    fn from(value: MergeError) -> Self {
+        match value {
+            MergeError::Conflict(addr) => InsertionError::Conflict(addr),
+            MergeError::TreeMalformed(addr) => InsertionError::InputMalformed(addr),
+        }
+    }
+}
+
+impl fmt::Display for MergeError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match &self {
+            MergeError::Conflict(addr) => write!(
+                f,
+                "Inserted root conflicts with existing root at address {:?}",
+                addr
+            ),
+            MergeError::TreeMalformed(addr) => {
+                write!(f, "Merge input malformed at address {:?}", addr)
+            }
+        }
+    }
+}
+
 impl<H: Hashable + Clone + PartialEq> PrunableTree<H> {
     /// Returns the the value if this is a leaf.
     pub fn leaf_value(&self) -> Option<&H> {
@@ -153,7 +185,9 @@ impl<H: Hashable + Clone + PartialEq> PrunableTree<H> {
                         || {
                             // Compute the roots of the left and right children and hash them
                             // together.
-                            let (l_addr, r_addr) = root_addr.children().unwrap();
+                            let (l_addr, r_addr) = root_addr
+                                .children()
+                                .expect("The root address of a parent node must have children.");
                             accumulate_result_with(
                                 left.root_hash(l_addr, truncate_at),
                                 right.root_hash(r_addr, truncate_at),
@@ -240,13 +274,14 @@ impl<H: Hashable + Clone + PartialEq> PrunableTree<H> {
     /// would cause information loss or if a conflict between root hashes occurs at a node. The
     /// returned error contains the address of the node where such a conflict occurred.
     #[tracing::instrument()]
-    pub fn merge_checked(self, root_addr: Address, other: Self) -> Result<Self, Address> {
+    pub fn merge_checked(self, root_addr: Address, other: Self) -> Result<Self, MergeError> {
+        /// Pre-condition: `root_addr` must be the address of `t0` and `t1`.
         #[allow(clippy::type_complexity)]
         fn go<H: Hashable + Clone + PartialEq>(
             addr: Address,
             t0: PrunableTree<H>,
             t1: PrunableTree<H>,
-        ) -> Result<PrunableTree<H>, Address> {
+        ) -> Result<PrunableTree<H>, MergeError> {
             // Require that any roots the we compute will not be default-filled by picking
             // a starting valid fill point that is outside the range of leaf positions.
             let no_default_fill = addr.position_range_end();
@@ -258,7 +293,7 @@ impl<H: Hashable + Clone + PartialEq> PrunableTree<H> {
                         Ok(Tree::leaf((vl.0, vl.1 | vr.1)))
                     } else {
                         trace!(left = ?vl.0, right = ?vr.0, "Merge conflict for leaves");
-                        Err(addr)
+                        Err(MergeError::Conflict(addr))
                     }
                 }
                 (Tree(Node::Leaf { value }), parent @ Tree(Node::Parent { .. }))
@@ -268,7 +303,7 @@ impl<H: Hashable + Clone + PartialEq> PrunableTree<H> {
                         Ok(parent.reannotate_root(Some(Arc::new(value.0))))
                     } else {
                         trace!(leaf = ?value, node = ?parent_hash, "Merge conflict for leaf into node");
-                        Err(addr)
+                        Err(MergeError::Conflict(addr))
                     }
                 }
                 (lparent, rparent) => {
@@ -293,7 +328,8 @@ impl<H: Hashable + Clone + PartialEq> PrunableTree<H> {
                             }),
                         ) = (lparent, rparent)
                         {
-                            let (l_addr, r_addr) = addr.children().unwrap();
+                            let (l_addr, r_addr) =
+                                addr.children().ok_or(MergeError::TreeMalformed(addr))?;
                             Ok(Tree::unite(
                                 addr.level() - 1,
                                 lann.or(rann),
@@ -305,7 +341,7 @@ impl<H: Hashable + Clone + PartialEq> PrunableTree<H> {
                         }
                     } else {
                         trace!(left = ?lroot, right = ?rroot, "Merge conflict for nodes");
-                        Err(addr)
+                        Err(MergeError::Conflict(addr))
                     }
                 }
             }
@@ -703,7 +739,7 @@ impl<H: Hashable + Clone + PartialEq> LocatedPrunableTree<H> {
                         parent
                             .clone()
                             .merge_checked(root_addr, subtree.root)
-                            .map_err(InsertionError::Conflict)
+                            .map_err(InsertionError::from)
                             .map(|tree| (tree, vec![]))
                     }
                     Tree(Node::Parent { ann, left, right }) => {
@@ -1021,6 +1057,7 @@ mod tests {
     use super::{LocatedPrunableTree, PrunableTree, RetentionFlags};
     use crate::{
         error::{InsertionError, QueryError},
+        prunable::MergeError,
         testing::{arb_char_str, arb_prunable_tree},
         tree::{
             tests::{leaf, nil, parent},
@@ -1117,7 +1154,7 @@ mod tests {
         assert_eq!(
             t0.clone()
                 .merge_checked(Address::from_parts(1.into(), 0), t2.clone()),
-            Err(Address::from_parts(0.into(), 0))
+            Err(MergeError::Conflict(Address::from_parts(0.into(), 0)))
         );
 
         let t3: PrunableTree<String> = parent(t0, t2);

shardtree/src/tree.rs

@@ -199,33 +199,35 @@ pub struct LocatedTree<A, V> {
 impl<A, V> LocatedTree<A, V> {
     /// Constructs a new LocatedTree from its constituent parts.
     ///
-    /// Returns `None` if `root_addr` is inconsistent with `root` (in particular, if the
-    /// level of `root_addr` is too small to contain `tree`).
-    pub fn from_parts(root_addr: Address, root: Tree<A, V>) -> Option<Self> {
+    /// Returns the newly constructed error, or the address at which the provided tree extends
+    /// beyond the position range of the provided root address.
+    pub fn from_parts(root_addr: Address, root: Tree<A, V>) -> Result<Self, Address> {
         // In order to meet various pre-conditions throughout the crate, we require that
         // no `Node::Parent` in `root` has a level of 0 relative to `root_addr`.
-        fn is_consistent<A, V>(addr: Address, root: &Tree<A, V>) -> bool {
+        fn check<A, V>(addr: Address, root: &Tree<A, V>) -> Result<(), Address> {
             match (&root.0, addr.children()) {
                 // Found an inconsistency!
-                (Node::Parent { .. }, None) => false,
+                (Node::Parent { .. }, None) => Err(addr),
                 // Check consistency of children recursively.
                 (Node::Parent { left, right, .. }, Some((l_addr, r_addr))) => {
-                    is_consistent(l_addr, left) && is_consistent(r_addr, right)
+                    check(l_addr, left)?;
+                    check(r_addr, right)?;
+                    Ok(())
                 }
 
                 // Leaves are technically allowed to occur at any level, so we do not
                 // require `addr` to have no children.
-                (Node::Leaf { .. }, _) => true,
+                (Node::Leaf { .. }, _) => Ok(()),
 
                 // Nil nodes have no information, so we cannot verify that the data it
                 // represents is consistent with `root_addr`. Instead we rely on methods
                 // that mutate `LocatedTree` to verify that the insertion address is not
                 // inconsistent with `root_addr`.
-                (Node::Nil, _) => true,
+                (Node::Nil, _) => Ok(()),
             }
         }
 
-        is_consistent(root_addr, &root).then_some(LocatedTree { root_addr, root })
+        check(root_addr, &root).map(|_| LocatedTree { root_addr, root })
     }
 
     /// Returns the root address of this tree.