Merge pull request #107 from nuttycom/shardtree/fix_inserted_parents

shardtree: Use annotated parent nodes with empty children for inserted frontier ommers.

incrementalmerkletree/CHANGELOG.md

@@ -9,6 +9,7 @@ and this project adheres to Rust's notion of
 
 ### Added
 - `incrementalmerkletree::Marking`
+- `incrementalmerkletree::Level::ZERO`
 
 ### Changed
 - `incrementalmerkletree::Retention`

incrementalmerkletree/src/lib.rs

@@ -280,6 +280,9 @@ impl TryFrom<Position> for usize {
 pub struct Level(u8);
 
 impl Level {
+    /// Level 0 corresponds to a leaf of the tree.
+    pub const ZERO: Self = Level(0);
+
     pub const fn new(value: u8) -> Self {
         Self(value)
     }

shardtree/CHANGELOG.md

@@ -10,20 +10,24 @@ 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
   interpreted as a node that had actually had its value observed as though it
   had been inserted using the ordinary tree insertion methods.
+- Fixes an error in an internal method that could result in subtree root
+  annotation data being discarded when pruning a `Parent` node having
+  `Nil` nodes for both its left and right children.
 
 ## [0.3.1] - 2024-04-03
 

shardtree/src/lib.rs

@@ -210,7 +210,8 @@ impl<
         Ok(())
     }
 
-    /// Append a single value at the first available position in the tree.
+    /// Append a single value at the first unfilled position greater than the maximum position of
+    /// any previously inserted leaf.
     ///
     /// Prefer to use [`Self::batch_insert`] when appending multiple values, as these operations
     /// require fewer traversals of the tree than are necessary when performing multiple sequential
@@ -234,17 +235,17 @@ impl<
 
         let (append_result, position, checkpoint_id) =
             if let Some(subtree) = self.store.last_shard().map_err(ShardTreeError::Storage)? {
-                match subtree.max_position() {
-                    // If the subtree is full, then construct a successor tree.
-                    Some(pos) if pos == subtree.root_addr.max_position() => {
-                        let addr = subtree.root_addr;
-                        if subtree.root_addr.index() < Self::max_subtree_index() {
-                            LocatedTree::empty(addr.next_at_level()).append(value, retention)?
-                        } else {
-                            return Err(InsertionError::TreeFull.into());
-                        }
+                if subtree.root().is_full() {
+                    // If the shard is full, then construct a successor tree.
+                    let addr = subtree.root_addr;
+                    if subtree.root_addr.index() < Self::max_subtree_index() {
+                        LocatedTree::empty(addr.next_at_level()).append(value, retention)?
+                    } else {
+                        return Err(InsertionError::TreeFull.into());
                     }
-                    _ => subtree.append(value, retention)?,
+                } else {
+                    // Otherwise, just append to the shard.
+                    subtree.append(value, retention)?
                 }
             } else {
                 let root_addr = Address::from_parts(Self::subtree_level(), 0);
@@ -320,17 +321,8 @@ impl<
             .map_err(ShardTreeError::Storage)?
             .unwrap_or_else(|| LocatedTree::empty(subtree_root_addr));
 
-        trace!(
-            max_position = ?current_shard.max_position(),
-            subtree = ?current_shard,
-            "Current shard");
         let (updated_subtree, supertree) =
             current_shard.insert_frontier_nodes(frontier, &leaf_retention)?;
-        trace!(
-            max_position = ?updated_subtree.max_position(),
-            subtree = ?updated_subtree,
-            "Replacement shard"
-        );
         self.store
             .put_shard(updated_subtree)
             .map_err(ShardTreeError::Storage)?;
@@ -447,7 +439,7 @@ impl<
                     Tree::leaf((h.clone(), *r | RetentionFlags::CHECKPOINT)),
                     root_addr.max_position(),
                 )),
-                Node::Nil | Node::Pruned => None,
+                Node::Nil => None,
             }
         }
 
@@ -581,13 +573,7 @@ impl<
                     .map_err(ShardTreeError::Storage)?;
 
                 if let Some(to_clear) = to_clear {
-                    let pre_clearing_max_position = to_clear.max_position();
                     let cleared = to_clear.clear_flags(positions);
-
-                    // Clearing flags should not modify the max position of leaves represented
-                    // in the shard.
-                    assert!(cleared.max_position() == pre_clearing_max_position);
-
                     self.store
                         .put_shard(cleared)
                         .map_err(ShardTreeError::Storage)?;
@@ -867,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()
                 {
@@ -1464,7 +1450,8 @@ mod tests {
                     id: 1,
                     marking: frontier_marking,
                 },
-            )?;
+            )
+            .unwrap();
 
             // Insert a few leaves beginning at the subsequent position, so as to cross the shard
             // boundary.
@@ -1473,7 +1460,8 @@ mod tests {
                 ('g'..='j')
                     .into_iter()
                     .map(|c| (c.to_string(), Retention::Ephemeral)),
-            )?;
+            )
+            .unwrap();
 
             // Trigger pruning by adding 5 more checkpoints
             for i in 2..7 {
@@ -1486,7 +1474,8 @@ mod tests {
                 ('e'..='f')
                     .into_iter()
                     .map(|c| (c.to_string(), Retention::Marked)),
-            )?;
+            )
+            .unwrap();
 
             // Compute the witness
             tree.witness_at_checkpoint_id(frontier_end, &6)