From 150f5544ba73d651a9091e736006123882184659 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sat, 23 Nov 2024 00:51:30 +0000 Subject: [PATCH 1/2] shardtree: Justify `unwrap`s due to an `Infallible` error type Rust 1.82 adds support for omitting empty types in pattern matching, which would make these much clearer. --- shardtree/src/store/caching.rs | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/shardtree/src/store/caching.rs b/shardtree/src/store/caching.rs index ab898fe..74ce3b2 100644 --- a/shardtree/src/store/caching.rs +++ b/shardtree/src/store/caching.rs @@ -46,9 +46,11 @@ where let _ = cache.put_cap(backend.get_cap()?); backend.with_checkpoints(backend.checkpoint_count()?, |checkpoint_id, checkpoint| { + // TODO: Once MSRV is at least 1.82, replace this (and similar `expect()`s below) with: + // `let Ok(_) = cache.add_checkpoint(checkpoint_id.clone(), checkpoint.clone());` cache .add_checkpoint(checkpoint_id.clone(), checkpoint.clone()) - .unwrap(); + .expect("error type is Infallible"); Ok(()) })?; @@ -74,26 +76,37 @@ where } self.deferred_actions.clear(); - for shard_root in self.cache.get_shard_roots().unwrap() { + for shard_root in self + .cache + .get_shard_roots() + .expect("error type is Infallible") + { self.backend.put_shard( self.cache .get_shard(shard_root) - .unwrap() + .expect("error type is Infallible") .expect("known address"), )?; } - self.backend.put_cap(self.cache.get_cap().unwrap())?; + self.backend + .put_cap(self.cache.get_cap().expect("error type is Infallible"))?; - let mut checkpoints = Vec::with_capacity(self.cache.checkpoint_count().unwrap()); + let mut checkpoints = Vec::with_capacity( + self.cache + .checkpoint_count() + .expect("error type is Infallible"), + ); self.cache .with_checkpoints( - self.cache.checkpoint_count().unwrap(), + self.cache + .checkpoint_count() + .expect("error type is Infallible"), |checkpoint_id, checkpoint| { checkpoints.push((checkpoint_id.clone(), checkpoint.clone())); Ok(()) }, ) - .unwrap(); + .expect("error type is Infallible"); for (checkpoint_id, checkpoint) in checkpoints { self.backend.add_checkpoint(checkpoint_id, checkpoint)?; } From 766a00eb660b10c9e03b7634d22f651c68e647ec Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sat, 23 Nov 2024 00:54:32 +0000 Subject: [PATCH 2/2] shardtree: Justify `unwraps` due to upheld pre-conditions --- shardtree/src/legacy.rs | 6 ++++- shardtree/src/lib.rs | 13 +++++++--- shardtree/src/prunable.rs | 51 ++++++++++++++++++++++++++++++--------- shardtree/src/tree.rs | 15 +++++++++--- 4 files changed, 66 insertions(+), 19 deletions(-) diff --git a/shardtree/src/legacy.rs b/shardtree/src/legacy.rs index 3957061..e151ae7 100644 --- a/shardtree/src/legacy.rs +++ b/shardtree/src/legacy.rs @@ -177,7 +177,11 @@ impl LocatedPrunableTree { // construct the subtree and cap based on the frontier containing the // witnessed position let (past_subtree, past_supertree) = self.insert_frontier_nodes::( - witness.tree().to_frontier().take().unwrap(), + witness + .tree() + .to_frontier() + .take() + .expect("IncrementalWitness must not be created from the empty tree."), &Retention::Marked, )?; diff --git a/shardtree/src/lib.rs b/shardtree/src/lib.rs index 1f4510a..bb60eef 100644 --- a/shardtree/src/lib.rs +++ b/shardtree/src/lib.rs @@ -392,13 +392,16 @@ impl< /// Adds a checkpoint at the rightmost leaf state of the tree. pub fn checkpoint(&mut self, checkpoint_id: C) -> Result> { + /// Pre-condition: `root_addr` must be the address of `root`. fn go( root_addr: Address, root: &PrunableTree, ) -> Option<(PrunableTree, Position)> { match root { Tree(Node::Parent { ann, left, right }) => { - let (l_addr, r_addr) = root_addr.children().unwrap(); + let (l_addr, r_addr) = root_addr + .children() + .expect("has children because we checked `root` is a parent"); go(r_addr, right).map_or_else( || { go(l_addr, left).map(|(new_left, pos)| { @@ -750,7 +753,10 @@ impl< // Compute the roots of the left and right children and hash them together. // We skip computation in any subtrees that will not have data included in // the final result. - let (l_addr, r_addr) = cap.root_addr.children().unwrap(); + let (l_addr, r_addr) = cap + .root_addr + .children() + .expect("has children because we checked `cap.root` is a parent"); let l_result = if r_addr.contains(&target_addr) { None } else { @@ -1103,7 +1109,8 @@ impl< cur_addr = cur_addr.parent(); } - Ok(MerklePath::from_parts(witness, position).unwrap()) + Ok(MerklePath::from_parts(witness, position) + .expect("witness has length DEPTH because we extended it to the root")) } fn witness_internal( diff --git a/shardtree/src/prunable.rs b/shardtree/src/prunable.rs index 673a801..31d56fb 100644 --- a/shardtree/src/prunable.rs +++ b/shardtree/src/prunable.rs @@ -120,7 +120,9 @@ impl PrunableTree { || { // 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), @@ -207,6 +209,7 @@ impl PrunableTree { /// 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. pub fn merge_checked(self, root_addr: Address, other: Self) -> Result { + /// Pre-condition: `root_addr` must be the address of `t0` and `t1`. #[allow(clippy::type_complexity)] fn go( addr: Address, @@ -261,7 +264,9 @@ impl PrunableTree { }), ) = (lparent, rparent) { - let (l_addr, r_addr) = addr.children().unwrap(); + let (l_addr, r_addr) = addr + .children() + .expect("The root address of a parent node must have children."); Ok(Tree::unite( addr.level() - 1, lann.or(rann), @@ -357,6 +362,7 @@ impl LocatedPrunableTree { /// Returns the positions of marked leaves in the tree. pub fn marked_positions(&self) -> BTreeSet { + /// Pre-condition: `root_addr` must be the address of `root`. fn go( root_addr: Address, root: &PrunableTree, @@ -364,7 +370,9 @@ impl LocatedPrunableTree { ) { match &root.0 { Node::Parent { left, right, .. } => { - let (l_addr, r_addr) = root_addr.children().unwrap(); + let (l_addr, r_addr) = root_addr + .children() + .expect("has children because we checked `root` is a parent"); go(l_addr, left.as_ref(), acc); go(r_addr, right.as_ref(), acc); } @@ -391,8 +399,10 @@ impl LocatedPrunableTree { /// Returns either the witness for the leaf at the specified position, or an error that /// describes the causes of failure. pub fn witness(&self, position: Position, truncate_at: Position) -> Result, QueryError> { - // traverse down to the desired leaf position, and then construct - // the authentication path on the way back up. + /// Traverse down to the desired leaf position, and then construct + /// the authentication path on the way back up. + // + /// Pre-condition: `root_addr` must be the address of `root`. fn go( root: &PrunableTree, root_addr: Address, @@ -401,7 +411,9 @@ impl LocatedPrunableTree { ) -> Result, Vec
> { match &root.0 { Node::Parent { left, right, .. } => { - let (l_addr, r_addr) = root_addr.children().unwrap(); + let (l_addr, r_addr) = root_addr + .children() + .expect("has children because we checked `root` is a parent"); if root_addr.level() > 1.into() { let r_start = r_addr.position_range_start(); if position < r_start { @@ -476,6 +488,7 @@ impl LocatedPrunableTree { /// subtree root with the specified position as its maximum position exists, or `None` /// otherwise. pub fn truncate_to_position(&self, position: Position) -> Option { + /// Pre-condition: `root_addr` must be the address of `root`. fn go( position: Position, root_addr: Address, @@ -483,7 +496,9 @@ impl LocatedPrunableTree { ) -> Option> { match &root.0 { Node::Parent { ann, left, right } => { - let (l_child, r_child) = root_addr.children().unwrap(); + let (l_child, r_child) = root_addr + .children() + .expect("has children because we checked `root` is a parent"); if position < r_child.position_range_start() { // we are truncating within the range of the left node, so recurse // to the left to truncate the left child and then reconstruct the @@ -537,8 +552,10 @@ impl LocatedPrunableTree { subtree: Self, contains_marked: bool, ) -> Result<(Self, Vec), InsertionError> { - // A function to recursively dig into the tree, creating a path downward and introducing - // empty nodes as necessary until we can insert the provided subtree. + /// A function to recursively dig into the tree, creating a path downward and introducing + /// empty nodes as necessary until we can insert the provided subtree. + /// + /// Pre-condition: `root_addr` must be the address of `into`. #[allow(clippy::type_complexity)] fn go( root_addr: Address, @@ -621,7 +638,9 @@ impl LocatedPrunableTree { Tree(Node::Parent { ann, left, right }) => { // In this case, we have an existing parent but we need to dig down farther // before we can insert the subtree that we're carrying for insertion. - let (l_addr, r_addr) = root_addr.children().unwrap(); + let (l_addr, r_addr) = root_addr + .children() + .expect("has children because we checked `into` is a parent"); if l_addr.contains(&subtree.root_addr) { let (new_left, incomplete) = go(l_addr, left.as_ref(), subtree, is_complete, contains_marked)?; @@ -696,7 +715,12 @@ impl LocatedPrunableTree { if r.remainder.next().is_some() { Err(InsertionError::TreeFull) } else { - Ok((r.subtree, r.max_insert_position.unwrap(), checkpoint_id)) + Ok(( + r.subtree, + r.max_insert_position + .expect("Batch insertion result position is never initialized to None"), + checkpoint_id, + )) } }) } @@ -820,6 +844,7 @@ impl LocatedPrunableTree { /// Clears the specified retention flags at all positions specified, pruning any branches /// that no longer need to be retained. pub fn clear_flags(&self, to_clear: BTreeMap) -> Self { + /// Pre-condition: `root_addr` must be the address of `root`. fn go( to_clear: &[(Position, RetentionFlags)], root_addr: Address, @@ -831,7 +856,9 @@ impl LocatedPrunableTree { } else { match root { Tree(Node::Parent { ann, left, right }) => { - let (l_addr, r_addr) = root_addr.children().unwrap(); + let (l_addr, r_addr) = root_addr + .children() + .expect("has children because we checked `root` is a parent"); let p = to_clear.partition_point(|(p, _)| p < &l_addr.position_range_end()); trace!( diff --git a/shardtree/src/tree.rs b/shardtree/src/tree.rs index 5a87eae..e873ba0 100644 --- a/shardtree/src/tree.rs +++ b/shardtree/src/tree.rs @@ -218,10 +218,13 @@ impl LocatedTree { /// Returns the value at the specified position, if any. pub fn value_at_position(&self, position: Position) -> Option<&V> { + /// Pre-condition: `addr` must be the address of `root`. fn go(pos: Position, addr: Address, root: &Tree) -> Option<&V> { match &root.0 { Node::Parent { left, right, .. } => { - let (l_addr, r_addr) = addr.children().unwrap(); + let (l_addr, r_addr) = addr + .children() + .expect("has children because we checked `root` is a parent"); if l_addr.position_range().contains(&pos) { go(pos, l_addr, left) } else { @@ -265,6 +268,7 @@ impl LocatedTree { /// if the tree is terminated by a [`Node::Nil`] or leaf node before the specified address can /// be reached. pub fn subtree(&self, addr: Address) -> Option { + /// Pre-condition: `root_addr` must be the address of `root`. fn go( root_addr: Address, root: &Tree, @@ -278,7 +282,9 @@ impl LocatedTree { } else { match &root.0 { Node::Parent { left, right, .. } => { - let (l_addr, r_addr) = root_addr.children().unwrap(); + let (l_addr, r_addr) = root_addr + .children() + .expect("has children because we checked `root` is a parent"); if l_addr.contains(&addr) { go(l_addr, left.as_ref(), addr) } else { @@ -302,6 +308,7 @@ impl LocatedTree { /// If this root address of this tree is lower down in the tree than the level specified, /// the entire tree is returned as the sole element of the result vector. pub fn decompose_to_level(self, level: Level) -> Vec { + /// Pre-condition: `root_addr` must be the address of `root`. fn go( level: Level, root_addr: Address, @@ -312,7 +319,9 @@ impl LocatedTree { } else { match root.0 { Node::Parent { left, right, .. } => { - let (l_addr, r_addr) = root_addr.children().unwrap(); + let (l_addr, r_addr) = root_addr + .children() + .expect("has children because we checked `root` is a parent"); let mut l_decomposed = go( level, l_addr,