From 25242e26d9d560f1522634b7168ffd4070d2d41d Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 27 Jun 2024 15:48:20 -0600 Subject: [PATCH] shardtree: Remove `Node::Pruned` In 7e48886fd326481f80dbfb0860ad46db05f4bd05, `Node::Pruned` was introduced in order to fix problems with `LocatedTree::max_position` resulting from reinsertion of subtree information as children of a previously pruned subtree represented by a `Node::Leaf` at its root. This fix introduced a large amount of complexity that is better resolved by fixing the `max_position` function in a different way and generally minimizing its usage. --- shardtree/CHANGELOG.md | 15 ++--- shardtree/src/lib.rs | 4 +- shardtree/src/prunable.rs | 112 +++++++++++++++++++++----------------- shardtree/src/tree.rs | 57 +------------------ 4 files changed, 75 insertions(+), 113 deletions(-) diff --git a/shardtree/CHANGELOG.md b/shardtree/CHANGELOG.md index d00ecde..16d9e6e 100644 --- a/shardtree/CHANGELOG.md +++ b/shardtree/CHANGELOG.md @@ -10,15 +10,16 @@ and this project adheres to Rust's notion of ### Added - `shardtree::tree::Tree::{is_leaf, map, try_map, empty_pruned}` - `shardtree::tree::LocatedTree::{map, try_map}` -- `shardtree::prunable::PrunableTree::{has_computable_root}` - -### Changed -- `shardtree::tree::Node` has additional variant `Node::Pruned`. +- `shardtree::prunable::PrunableTree::{has_computable_root, is_full}` +- `shardtree::prunable::LocatedPrunableTree::{max_position}` ### Removed -- `shardtree::tree::Tree::is_complete` as it is no longer well-defined in the - presence of `Pruned` nodes. Use `PrunableTree::has_computable_root` to - determine whether it is possible to compute the root of a tree. +- `shardtree::tree::LocatedTree::max_position` did not behave correctly regarding + annotated parent nodes. Use `LocatedPrunableTree::max_position` instead. +- `shardtree::tree::Tree::is_complete` was somewhat misleadingly named, as `Nil` + nodes that were inserted as a consequence of insertion after pruning could be + interpreted as rendering the tree incomplete. Use `PrunableTree::has_computable_root` + instead to determine whether it is possible to compute the root of a tree. ### Fixed - Fixes an error that could occur if an inserted `Frontier` node was diff --git a/shardtree/src/lib.rs b/shardtree/src/lib.rs index fff21b4..48ee472 100644 --- a/shardtree/src/lib.rs +++ b/shardtree/src/lib.rs @@ -439,7 +439,7 @@ impl< Tree::leaf((h.clone(), *r | RetentionFlags::CHECKPOINT)), root_addr.max_position(), )), - Node::Nil | Node::Pruned => None, + Node::Nil => None, } } @@ -853,7 +853,7 @@ impl< )) } } - Node::Nil | Node::Pruned => { + Node::Nil => { if cap.root_addr == target_addr || cap.root_addr.level() == ShardTree::::subtree_level() { diff --git a/shardtree/src/prunable.rs b/shardtree/src/prunable.rs index 28d73ab..e823e87 100644 --- a/shardtree/src/prunable.rs +++ b/shardtree/src/prunable.rs @@ -109,7 +109,17 @@ impl PrunableTree { || (left.as_ref().has_computable_root() && right.as_ref().has_computable_root()) } Node::Leaf { .. } => true, - Node::Nil | Node::Pruned => false, + Node::Nil => false, + } + } + + /// Returns `true` if no additional leaves can be appended to the right hand side of this tree + /// without over-filling it given its current depth. + pub fn is_full(&self) -> bool { + match &self.0 { + Node::Nil => false, + Node::Leaf { .. } => true, + Node::Parent { ann, right, .. } => ann.is_some() || right.is_full(), } } @@ -118,7 +128,7 @@ impl PrunableTree { match &self.0 { Node::Parent { left, right, .. } => left.contains_marked() || right.contains_marked(), Node::Leaf { value: (_, r) } => r.is_marked(), - Node::Nil | Node::Pruned => false, + Node::Nil => false, } } @@ -170,7 +180,7 @@ impl PrunableTree { Err(vec![root_addr]) } } - Node::Nil | Node::Pruned => Err(vec![root_addr]), + Node::Nil => Err(vec![root_addr]), } } } @@ -205,7 +215,7 @@ impl PrunableTree { } result } - Node::Nil | Node::Pruned => BTreeSet::new(), + Node::Nil => BTreeSet::new(), } } @@ -242,7 +252,6 @@ impl PrunableTree { let no_default_fill = addr.position_range_end(); match (t0, t1) { (Tree(Node::Nil), other) | (other, Tree(Node::Nil)) => Ok(other), - (Tree(Node::Pruned), other) | (other, Tree(Node::Pruned)) => Ok(other), (Tree(Node::Leaf { value: vl }), Tree(Node::Leaf { value: vr })) => { if vl.0 == vr.0 { // Merge the flags together. @@ -344,6 +353,32 @@ pub struct IncompleteAt { } impl LocatedPrunableTree { + /// Returns the maximum position at which a non-`Nil` leaf has been observed in the tree. + /// + /// Note that no actual leaf value may exist at this position, as it may have previously been + /// pruned. + pub fn max_position(&self) -> Option { + fn go( + addr: Address, + root: &Tree>, (H, RetentionFlags)>, + ) -> Option { + match &root.0 { + Node::Nil => None, + Node::Leaf { .. } => Some(addr.position_range_end() - 1), + Node::Parent { ann, left, right } => { + if ann.is_some() { + Some(addr.max_position()) + } else { + let (l_addr, r_addr) = addr.children().unwrap(); + go(r_addr, right.as_ref()).or_else(|| go(l_addr, left.as_ref())) + } + } + } + } + + go(self.root_addr, &self.root) + } + /// Computes the root hash of this tree, truncated to the given position. /// /// If the tree contains any [`Node::Nil`] nodes corresponding to positions less than @@ -521,7 +556,7 @@ impl LocatedPrunableTree { None } } - Node::Nil | Node::Pruned => None, + Node::Nil => None, } } @@ -567,36 +602,29 @@ impl LocatedPrunableTree { // In the case that we are replacing a node entirely, we need to extend the // subtree up to the level of the node being replaced, adding Nil siblings // and recording the presence of those incomplete nodes when necessary - let replacement = - |ann: Option>, mut node: LocatedPrunableTree, pruned: bool| { - // construct the replacement node bottom-up - let mut incomplete = vec![]; - while node.root_addr.level() < root_addr.level() { - incomplete.push(IncompleteAt { - address: node.root_addr.sibling(), - required_for_witness: contains_marked, - }); - let empty = if pruned { - Tree::empty_pruned() + let replacement = |ann: Option>, mut node: LocatedPrunableTree| { + // construct the replacement node bottom-up + let mut incomplete = vec![]; + while node.root_addr.level() < root_addr.level() { + incomplete.push(IncompleteAt { + address: node.root_addr.sibling(), + required_for_witness: contains_marked, + }); + let full = node.root; + node = LocatedTree { + root_addr: node.root_addr.parent(), + root: if node.root_addr.is_right_child() { + Tree::parent(None, Tree::empty(), full) } else { - Tree::empty() - }; - let full = node.root; - node = LocatedTree { - root_addr: node.root_addr.parent(), - root: if node.root_addr.is_right_child() { - Tree::parent(None, empty, full) - } else { - Tree::parent(None, full, empty) - }, - }; - } - (node.root.reannotate_root(ann), incomplete) - }; + Tree::parent(None, full, Tree::empty()) + }, + }; + } + (node.root.reannotate_root(ann), incomplete) + }; match into { - Tree(Node::Nil) => Ok(replacement(None, subtree, false)), - Tree(Node::Pruned) => Ok(replacement(None, subtree, true)), + Tree(Node::Nil) => Ok(replacement(None, subtree)), Tree(Node::Leaf { value: (value, retention), }) => { @@ -651,22 +679,7 @@ impl LocatedPrunableTree { Err(InsertionError::Conflict(root_addr)) } } else { - Ok(replacement( - Some(Arc::new(value.clone())), - subtree, - // The subtree being inserted may have its root at some level lower - // than the next level down. The siblings of nodes that will be - // generated while descending to the subtree root level will be - // `Nil` nodes (indicating that the value of these nodes have never - // been observed) if the leaf being replaced has `REFERENCE` - // retention. Any other leaf without `REFERENCE` retention will - // have been produced by pruning of previously observed node - // values, so in those cases we use `Pruned` nodes for the absent - // siblings. This allows us to retain the distinction between what - // parts of the tree have been directly observed and what parts - // have not. - !retention.contains(RetentionFlags::REFERENCE), - )) + Ok(replacement(Some(Arc::new(value.clone())), subtree)) } } parent if root_addr == subtree.root_addr => { @@ -939,7 +952,6 @@ impl LocatedPrunableTree { } } Node::Nil => Tree::empty(), - Node::Pruned => Tree::empty_pruned(), } } } diff --git a/shardtree/src/tree.rs b/shardtree/src/tree.rs index 30abb7c..1156942 100644 --- a/shardtree/src/tree.rs +++ b/shardtree/src/tree.rs @@ -19,9 +19,6 @@ pub enum Node { Leaf { value: V }, /// The empty tree; a subtree or leaf for which no information is available. Nil, - /// An empty node in the tree created as a consequence of partial reinserion of data into a - /// subtree after the subtree was previously pruned. - Pruned, } impl Node { @@ -39,7 +36,6 @@ impl Node { Node::Parent { .. } => None, Node::Leaf { value } => Some(value), Node::Nil => None, - Node::Pruned => None, } } @@ -49,7 +45,6 @@ impl Node { Node::Parent { ann, .. } => Some(ann), Node::Leaf { .. } => None, Node::Nil => None, - Node::Pruned => None, } } @@ -76,7 +71,6 @@ impl<'a, C: Clone, A: Clone, V: Clone> Node { value: (*value).clone(), }, Node::Nil => Node::Nil, - Node::Pruned => Node::Pruned, } } } @@ -106,19 +100,10 @@ impl Tree { Self::parent(ann, Tree::empty(), Tree::empty()) } - /// Constructs the empty tree consisting of a single pruned node. - /// - /// This represents a tree for which we have previously observed all of the - /// children of its root, and have pruned away all of the data of this tree - /// as irrelevant. - pub const fn empty_pruned() -> Self { - Tree(Node::Pruned) - } - /// Constructs a tree containing a single leaf. /// /// This represents either leaf of the tree, or an internal parent node of the - /// tree having all [`Node::Pruned`] children. + /// tree whose children have all been pruned. pub fn leaf(value: V) -> Self { Tree(Node::Leaf { value }) } @@ -148,21 +133,7 @@ impl Tree { matches!(&self.0, Node::Leaf { .. }) } - /// Returns `true` if no additional leaves can be appended to this tree. - /// - /// The tree is considered full if no `Nil` node exists along the right-hand - /// path in a depth-first traversal of this tree. In this case, no additional - /// nodes can be added to the right-hand side of this tree without introducing - /// a new `Parent` node having this tree as its left-hand child. - pub fn is_full(&self) -> bool { - match &self.0 { - Node::Nil => false, - Node::Leaf { .. } | Node::Pruned => true, - Node::Parent { right, .. } => right.is_full(), - } - } - - /// Returns a vector of the addresses of [`Node::Nil`] and [`Node::Pruned`] subtree roots + /// Returns a vector of the addresses of [`Node::Nil`] subtree roots /// within this tree. /// /// The given address must correspond to the root of this tree, or this method will @@ -184,7 +155,7 @@ impl Tree { left_incomplete } Node::Leaf { .. } => vec![], - Node::Nil | Node::Pruned => vec![root_addr], + Node::Nil => vec![root_addr], } } @@ -202,7 +173,6 @@ impl Tree { }, Node::Leaf { value } => Node::Leaf { value: f(value) }, Node::Nil => Node::Nil, - Node::Pruned => Node::Pruned, }) } @@ -221,7 +191,6 @@ impl Tree { }, Node::Leaf { value } => Node::Leaf { value: f(value)? }, Node::Nil => Node::Nil, - Node::Pruned => Node::Pruned, })) } } @@ -269,25 +238,6 @@ impl LocatedTree { self.root.incomplete_nodes(self.root_addr) } - /// Returns the maximum position at which a non-`Nil` leaf has been observed in the tree. - /// - /// Note that no actual leaf value may exist at this position, as it may have previously been - /// pruned. - pub fn max_position(&self) -> Option { - fn go(addr: Address, root: &Tree) -> Option { - match &root.0 { - Node::Nil => None, - Node::Leaf { .. } | Node::Pruned => Some(addr.position_range_end() - 1), - Node::Parent { left, right, .. } => { - let (l_addr, r_addr) = addr.children().unwrap(); - go(r_addr, right.as_ref()).or_else(|| go(l_addr, left.as_ref())) - } - } - } - - go(self.root_addr, &self.root) - } - /// Returns the value at the specified position, if any. pub fn value_at_position(&self, position: Position) -> Option<&V> { fn go(pos: Position, addr: Address, root: &Tree) -> Option<&V> { @@ -492,7 +442,6 @@ pub(crate) mod tests { root: parent(l.clone(), r.clone()), }; - assert_eq!(t.max_position(), Some(7.into())); assert_eq!(t.value_at_position(5.into()), Some(&"b".to_string())); assert_eq!(t.value_at_position(8.into()), None); assert_eq!(t.subtree(Address::from_parts(0.into(), 1)), None);