shardtree: Remove `Node::Pruned`

In 7e48886fd3, `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

@@ -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}`
+- `shardtree::prunable::PrunableTree::{has_computable_root, is_full}`
-
+- `shardtree::prunable::LocatedPrunableTree::{max_position}`
-### Changed
-- `shardtree::tree::Node` has additional variant `Node::Pruned`.
 
 ### Removed
-- `shardtree::tree::Tree::is_complete` as it is no longer well-defined in the
+- `shardtree::tree::LocatedTree::max_position` did not behave correctly regarding
-  presence of `Pruned` nodes. Use `PrunableTree::has_computable_root` to
+  annotated parent nodes. Use `LocatedPrunableTree::max_position` instead.
-  determine whether it is possible to compute the root of a tree.
+- `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

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::<S, DEPTH, SHARD_HEIGHT>::subtree_level()
                 {

shardtree/src/prunable.rs

@@ -109,7 +109,17 @@ impl<H: Hashable + Clone + PartialEq> PrunableTree<H> {
                     || (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<H: Hashable + Clone + PartialEq> PrunableTree<H> {
         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<H: Hashable + Clone + PartialEq> PrunableTree<H> {
                         Err(vec![root_addr])
                     }
                 }
-                Node::Nil | Node::Pruned => Err(vec![root_addr]),
+                Node::Nil => Err(vec![root_addr]),
             }
         }
     }
@@ -205,7 +215,7 @@ impl<H: Hashable + Clone + PartialEq> PrunableTree<H> {
                 }
                 result
             }
-            Node::Nil | Node::Pruned => BTreeSet::new(),
+            Node::Nil => BTreeSet::new(),
         }
     }
 
@@ -242,7 +252,6 @@ impl<H: Hashable + Clone + PartialEq> PrunableTree<H> {
             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<H: Hashable + Clone + PartialEq> LocatedPrunableTree<H> {
+    /// 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<Position> {
+        fn go<H>(
+            addr: Address,
+            root: &Tree<Option<Arc<H>>, (H, RetentionFlags)>,
+        ) -> Option<Position> {
+            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<H: Hashable + Clone + PartialEq> LocatedPrunableTree<H> {
                         None
                     }
                 }
-                Node::Nil | Node::Pruned => None,
+                Node::Nil => None,
             }
         }
 
@@ -567,36 +602,29 @@ impl<H: Hashable + Clone + PartialEq> LocatedPrunableTree<H> {
                 // 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 =
+                let replacement = |ann: Option<Arc<H>>, mut node: LocatedPrunableTree<H>| {
-                    |ann: Option<Arc<H>>, mut node: LocatedPrunableTree<H>, pruned: bool| {
+                    // construct the replacement node bottom-up
-                        // construct the replacement node bottom-up
+                    let mut incomplete = vec![];
-                        let mut incomplete = vec![];
+                    while node.root_addr.level() < root_addr.level() {
-                        while node.root_addr.level() < root_addr.level() {
+                        incomplete.push(IncompleteAt {
-                            incomplete.push(IncompleteAt {
+                            address: node.root_addr.sibling(),
-                                address: node.root_addr.sibling(),
+                            required_for_witness: contains_marked,
-                                required_for_witness: contains_marked,
+                        });
-                            });
+                        let full = node.root;
-                            let empty = if pruned {
+                        node = LocatedTree {
-                                Tree::empty_pruned()
+                            root_addr: node.root_addr.parent(),
+                            root: if node.root_addr.is_right_child() {
+                                Tree::parent(None, Tree::empty(), full)
                             } else {
-                                Tree::empty()
+                                Tree::parent(None, full, Tree::empty())
-                            };
+                            },
-                            let full = node.root;
+                        };
-                            node = LocatedTree {
+                    }
-                                root_addr: node.root_addr.parent(),
+                    (node.root.reannotate_root(ann), incomplete)
-                                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)
-                    };
 
                 match into {
-                    Tree(Node::Nil) => Ok(replacement(None, subtree, false)),
+                    Tree(Node::Nil) => Ok(replacement(None, subtree)),
-                    Tree(Node::Pruned) => Ok(replacement(None, subtree, true)),
                     Tree(Node::Leaf {
                         value: (value, retention),
                     }) => {
@@ -651,22 +679,7 @@ impl<H: Hashable + Clone + PartialEq> LocatedPrunableTree<H> {
                                 Err(InsertionError::Conflict(root_addr))
                             }
                         } else {
-                            Ok(replacement(
+                            Ok(replacement(Some(Arc::new(value.clone())), subtree))
-                                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),
-                            ))
                         }
                     }
                     parent if root_addr == subtree.root_addr => {
@@ -921,7 +934,6 @@ impl<H: Hashable + Clone + PartialEq> LocatedPrunableTree<H> {
                         }
                     }
                     Node::Nil => Tree::empty(),
-                    Node::Pruned => Tree::empty_pruned(),
                 }
             }
         }

shardtree/src/tree.rs

@@ -19,9 +19,6 @@ pub enum Node<C, A, V> {
     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<C, A, V> Node<C, A, V> {
@@ -39,7 +36,6 @@ impl<C, A, V> Node<C, A, V> {
             Node::Parent { .. } => None,
             Node::Leaf { value } => Some(value),
             Node::Nil => None,
-            Node::Pruned => None,
         }
     }
 
@@ -49,7 +45,6 @@ impl<C, A, V> Node<C, A, V> {
             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<C, &'a A, &'a V> {
                 value: (*value).clone(),
             },
             Node::Nil => Node::Nil,
-            Node::Pruned => Node::Pruned,
         }
     }
 }
@@ -100,15 +94,10 @@ impl<A, V> Tree<A, V> {
         Tree(Node::Nil)
     }
 
-    /// Constructs the empty tree consisting of a single pruned node.
-    pub 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 })
     }
@@ -138,21 +127,7 @@ impl<A, V> Tree<A, V> {
         matches!(&self.0, Node::Leaf { .. })
     }
 
-    /// Returns `true` if no additional leaves can be appended to this tree.
+    /// Returns a vector of the addresses of [`Node::Nil`] subtree roots
-    ///
-    /// 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
     /// within this tree.
     ///
     /// The given address must correspond to the root of this tree, or this method will
@@ -174,7 +149,7 @@ impl<A, V> Tree<A, V> {
                 left_incomplete
             }
             Node::Leaf { .. } => vec![],
-            Node::Nil | Node::Pruned => vec![root_addr],
+            Node::Nil => vec![root_addr],
         }
     }
 
@@ -192,7 +167,6 @@ impl<A, V> Tree<A, V> {
             },
             Node::Leaf { value } => Node::Leaf { value: f(value) },
             Node::Nil => Node::Nil,
-            Node::Pruned => Node::Pruned,
         })
     }
 
@@ -211,7 +185,6 @@ impl<A, V> Tree<A, V> {
             },
             Node::Leaf { value } => Node::Leaf { value: f(value)? },
             Node::Nil => Node::Nil,
-            Node::Pruned => Node::Pruned,
         }))
     }
 }
@@ -259,25 +232,6 @@ impl<A, V> LocatedTree<A, V> {
         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<Position> {
-        fn go<A, V>(addr: Address, root: &Tree<A, V>) -> Option<Position> {
-            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<A, V>(pos: Position, addr: Address, root: &Tree<A, V>) -> Option<&V> {
@@ -482,7 +436,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);