From f5889dffa5ccb59a66583c72e410e442875e1151 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 16 Jun 2023 17:04:05 -0600 Subject: [PATCH] Address comments from code review. --- incrementalmerkletree/Cargo.toml | 5 ++ incrementalmerkletree/src/frontier.rs | 8 +-- incrementalmerkletree/src/lib.rs | 10 +++- incrementalmerkletree/src/testing.rs | 8 ++- shardtree/Cargo.toml | 5 ++ shardtree/src/lib.rs | 75 ++++++++++++--------------- 6 files changed, 60 insertions(+), 51 deletions(-) diff --git a/incrementalmerkletree/Cargo.toml b/incrementalmerkletree/Cargo.toml index 48080a9..9a8dfb1 100644 --- a/incrementalmerkletree/Cargo.toml +++ b/incrementalmerkletree/Cargo.toml @@ -21,5 +21,10 @@ proptest = { version = "1.0.0", optional = true } proptest = "1.0.0" [features] +# The legacy-api feature guards types and functions that were previously +# part of the `zcash_primitives` crate. Those types were removed in the +# `zcash_primitives` 0.12 release and are now maintained here. legacy-api = [] +# The test-depenencies feature guards types and functions that are +# useful for testing incremental Merkle trees and Merkle tree frontiers. test-dependencies = ["proptest"] diff --git a/incrementalmerkletree/src/frontier.rs b/incrementalmerkletree/src/frontier.rs index 37d6d7a..f942018 100644 --- a/incrementalmerkletree/src/frontier.rs +++ b/incrementalmerkletree/src/frontier.rs @@ -84,9 +84,9 @@ impl NonEmptyFrontier { let prior_leaf = self.leaf.clone(); self.position += 1; self.leaf = leaf; - if self.position.is_odd() { - // if the new position is odd, the current leaf will directly become - // an ommer at level 0, and there is no other mutation made to the tree. + if self.position.is_right_child() { + // if the new position is a right-hand leaf, the current leaf will directly become an + // ommer at level 0, and there is no other mutation made to the tree. self.ommers.insert(0, prior_leaf); } else { // if the new position is even, then the current leaf will be hashed @@ -411,7 +411,7 @@ impl CommitmentTree { pub fn from_frontier(frontier: &Frontier) -> Self { frontier.value().map_or_else(Self::empty, |f| { let mut ommers_iter = f.ommers().iter().cloned(); - let (left, right) = if f.position().is_odd() { + let (left, right) = if f.position().is_right_child() { ( ommers_iter .next() diff --git a/incrementalmerkletree/src/lib.rs b/incrementalmerkletree/src/lib.rs index e76d810..de40a02 100644 --- a/incrementalmerkletree/src/lib.rs +++ b/incrementalmerkletree/src/lib.rs @@ -94,8 +94,9 @@ impl Iterator for WitnessAddrsIter { pub struct Position(u64); impl Position { - /// Return whether the position is odd-valued. - pub fn is_odd(&self) -> bool { + /// Return whether the position refers to the right-hand child of a subtree with + /// its root at level 1. + pub fn is_right_child(&self) -> bool { self.0 & 0x1 == 1 } @@ -424,6 +425,11 @@ impl Address { } } + /// Returns whether this address is the right-hand child of its parent + pub fn is_left_child(&self) -> bool { + self.index & 0x1 == 0 + } + /// Returns whether this address is the right-hand child of its parent pub fn is_right_child(&self) -> bool { self.index & 0x1 == 1 diff --git a/incrementalmerkletree/src/testing.rs b/incrementalmerkletree/src/testing.rs index d098446..f1232e8 100644 --- a/incrementalmerkletree/src/testing.rs +++ b/incrementalmerkletree/src/testing.rs @@ -1061,7 +1061,9 @@ pub fn check_remove_mark, F: Fn(usize) -> } } -pub fn check_rewind_remove_mark, F: Fn(usize) -> T>(new_tree: F) { +pub fn check_rewind_remove_mark, F: Fn(usize) -> T>( + new_tree: F, +) { // rewinding doesn't remove a mark let mut tree = new_tree(100); tree.append("e".to_string(), Retention::Marked); @@ -1139,7 +1141,9 @@ pub fn check_rewind_remove_mark, F: Fn(usi } } -pub fn check_witness_consistency, F: Fn(usize) -> T>(new_tree: F) { +pub fn check_witness_consistency, F: Fn(usize) -> T>( + new_tree: F, +) { let samples = vec![ // Reduced examples vec![ diff --git a/shardtree/Cargo.toml b/shardtree/Cargo.toml index 2d40ad1..2e4e4b3 100644 --- a/shardtree/Cargo.toml +++ b/shardtree/Cargo.toml @@ -24,7 +24,12 @@ incrementalmerkletree = { version = "0.4", path = "../incrementalmerkletree", fe proptest = "1.0.0" [features] +# The legacy-api feature guards types and functions that are useful for +# migrating data previously managed using `incrementalmerkletree/legacy-api` +# types into the `ShardTree` data structure. legacy-api = ["incrementalmerkletree/legacy-api"] +# The test-depenencies feature can be enabled to expose types and functions +# that are useful for testing `shardtree` functionality. test-dependencies = ["proptest"] [target.'cfg(unix)'.dev-dependencies] diff --git a/shardtree/src/lib.rs b/shardtree/src/lib.rs index f37fa38..fea3c9b 100644 --- a/shardtree/src/lib.rs +++ b/shardtree/src/lib.rs @@ -1,6 +1,6 @@ use bitflags::bitflags; use core::convert::TryFrom; -use core::fmt::{self, Debug}; +use core::fmt::{self, Debug, Display}; use core::ops::{Deref, Range}; use either::Either; use std::collections::{BTreeMap, BTreeSet}; @@ -722,11 +722,7 @@ impl fmt::Display for InsertionError { } } -impl std::error::Error for InsertionError { - fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - None - } -} +impl std::error::Error for InsertionError {} /// Errors that may be returned in the process of querying a [`ShardTree`] #[derive(Clone, Debug, PartialEq, Eq)] @@ -765,11 +761,7 @@ impl fmt::Display for QueryError { } } -impl std::error::Error for QueryError { - fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - None - } -} +impl std::error::Error for QueryError {} /// Operations on [`LocatedTree`]s that are annotated with Merkle hashes. impl LocatedPrunableTree { @@ -871,7 +863,7 @@ impl LocatedPrunableTree { } else { // we handle the level 0 leaves here by adding the sibling of our desired // leaf to the witness - if position.is_odd() { + if position.is_right_child() { if right.is_marked_leaf() { left.leaf_value() .map(|v| vec![v.clone()]) @@ -1345,9 +1337,9 @@ impl LocatedPrunableTree { }), }; - if position.is_odd() { - // At odd positions, we are completing a subtree and so we unite fragments - // up the stack until we get the largest possible subtree + if position.is_right_child() { + // At right-hand positions, we are completing a subtree and so we unite + // fragments up the stack until we get the largest possible subtree while let Some((potential_sibling, marked)) = fragments.pop() { if potential_sibling.root_addr.parent() == subtree.root_addr.parent() { subtree = unite(potential_sibling, subtree, prune_below); @@ -1416,11 +1408,10 @@ impl LocatedPrunableTree { } } - // Constructs a pair of trees contains the leaf and ommers of the given frontier. - // The first element of the result is a tree with its root at a level less than - // or equal to `split_at`; the second element is a tree with its leaves at level - // `split_at` that is only returned if the frontier contains sufficient data to - // fill the first tree to the `split_at` level. + // Constructs a pair of trees that contain the leaf and ommers of the given frontier. The first + // element of the result is a tree with its root at a level less than or equal to `split_at`; + // the second element is a tree with its leaves at level `split_at` that is only returned if + // the frontier contains sufficient data to fill the first tree to the `split_at` level. fn from_frontier( frontier: NonEmptyFrontier, leaf_retention: &Retention, @@ -1446,7 +1437,7 @@ impl LocatedPrunableTree { }); while addr.level() < split_at { - if addr.index() & 0x1 == 0 { + if addr.is_left_child() { // the current address is a left child, so create a parent with // an empty right-hand tree subtree = Tree::parent(None, subtree, Tree::empty()); @@ -1472,7 +1463,7 @@ impl LocatedPrunableTree { let mut supertree = None; for left in ommers { // build up the left-biased tree until we get a right-hand node - while addr.index() & 0x1 == 0 { + while addr.is_left_child() { supertree = supertree.map(|t| Tree::parent(None, t, Tree::empty())); addr = addr.parent(); } @@ -1511,7 +1502,7 @@ impl LocatedPrunableTree { let mut addr = leaf_addr; let mut subtree = Tree::empty(); while addr.level() < split_at { - if addr.index() & 0x1 == 0 { + if addr.is_left_child() { // the current root is a left child, so take the right sibling from the // filled iterator if let Some(right) = filled.next() { @@ -1543,7 +1534,7 @@ impl LocatedPrunableTree { let mut supertree = None; for right in filled { // build up the right-biased tree until we get a left-hand node - while addr.index() & 0x1 == 1 { + while addr.is_right_child() { supertree = supertree.map(|t| Tree::parent(None, Tree::empty(), t)); addr = addr.parent(); } @@ -1818,12 +1809,10 @@ pub trait ShardStore { /// provided has level `SHARD_HEIGHT`. fn truncate(&mut self, from: Address) -> Result<(), Self::Error>; - /// A tree that is used to cache the known roots of subtrees in the "cap" of nodes between - /// `SHARD_HEIGHT` and `DEPTH` that are otherwise not directly represented in the tree. - /// - /// This tree only contains data that is expected to be stable; i.e. any node whose value - /// might be altered as a consequence of the discovery of additional information from - /// branches of the tree will be `Nil` in this cache. + /// A tree that is used to cache the known roots of subtrees in the "cap" - the top part of the + /// tree, which contains parent nodes produced by hashing the roots of the individual shards. + /// Nodes in the cap have levels in the range `SHARD_HEIGHT..DEPTH`. Note that the cap may be + /// sparse, in the same way that individual shards may be sparse. fn get_cap(&self) -> Result, Self::Error>; /// Persists the provided cap to the data store. @@ -2176,12 +2165,8 @@ impl From for ShardTreeError { impl fmt::Display for ShardTreeError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match &self { - ShardTreeError::Query(q) => { - write!(f, "{}", q) - } - ShardTreeError::Insert(i) => { - write!(f, "{}", i) - } + ShardTreeError::Query(q) => Display::fmt(&q, f), + ShardTreeError::Insert(i) => Display::fmt(&i, f), ShardTreeError::Storage(s) => { write!( f, @@ -2749,16 +2734,15 @@ impl< &mut self, checkpoint_id: &C, ) -> Result> { - match self + if let Some(c) = self .store .get_checkpoint(checkpoint_id) .map_err(ShardTreeError::Storage)? { - Some(c) => { - self.truncate_removing_checkpoint_internal(checkpoint_id, &c)?; - Ok(true) - } - None => Ok(false), + self.truncate_removing_checkpoint_internal(checkpoint_id, &c)?; + Ok(true) + } else { + Ok(false) } } @@ -3221,6 +3205,11 @@ impl< } } + /// Computes the witness for the leaf at the specified position. + /// + /// This implementation will mutate the tree to cache intermediate root (ommer) values that are + /// computed in the process of constructing the witness, so as to avoid the need to recompute + /// those values from potentially large numbers of subtree roots in the future. pub fn witness_caching( &mut self, position: Position, @@ -3499,7 +3488,7 @@ pub mod testing { } fn marked_positions(&self) -> BTreeSet { - match ShardTree::marked_positions(&self) { + match ShardTree::marked_positions(self) { Ok(v) => v, Err(err) => panic!("marked positions query failed: {:?}", err), }