Skip to content

Commit

Permalink
shardtree: Remove Node::Pruned
Browse files Browse the repository at this point in the history 
In 7e48886, `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.
  • Loading branch information
@nuttycom
nuttycom committed Jun 27, 2024
1 parent d36fd44 commit 25242e2
Show file tree
Hide file tree
Showing 4 changed files with 75 additions and 113 deletions.
15 changes: 8 additions & 7 deletions shardtree/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions shardtree/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -439,7 +439,7 @@ impl<
Tree::leaf((h.clone(), *r | RetentionFlags::CHECKPOINT)),
root_addr.max_position(),
)),
Node::Nil | Node::Pruned => None,
Node::Nil => None,
}
}

Expand Down Expand Up @@ -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()
{
Expand Down
112 changes: 62 additions & 50 deletions shardtree/src/prunable.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
}
}

Expand All @@ -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,
}
}

Expand Down Expand Up @@ -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]),
}
}
}
Expand Down Expand Up @@ -205,7 +215,7 @@ impl<H: Hashable + Clone + PartialEq> PrunableTree<H> {
}
result
}
Node::Nil | Node::Pruned => BTreeSet::new(),
Node::Nil => BTreeSet::new(),
}
}

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -521,7 +556,7 @@ impl<H: Hashable + Clone + PartialEq> LocatedPrunableTree<H> {
None
}
}
Node::Nil | Node::Pruned => None,
Node::Nil => None,
}
}

Expand Down Expand Up @@ -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 =
|ann: Option<Arc<H>>, mut node: LocatedPrunableTree<H>, 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<Arc<H>>, mut node: LocatedPrunableTree<H>| {
// 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),
}) => {
Expand Down Expand Up @@ -651,22 +679,7 @@ impl<H: Hashable + Clone + PartialEq> LocatedPrunableTree<H> {
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 => {
Expand Down Expand Up @@ -939,7 +952,6 @@ impl<H: Hashable + Clone + PartialEq> LocatedPrunableTree<H> {
}
}
Node::Nil => Tree::empty(),
Node::Pruned => Tree::empty_pruned(),
}
}
}
Expand Down
57 changes: 3 additions & 54 deletions shardtree/src/tree.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand All @@ -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,
}
}

Expand All @@ -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,
}
}

Expand All @@ -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,
}
}
}
Expand Down Expand Up @@ -106,19 +100,10 @@ impl<A, V> Tree<A, V> {
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 })
}
Expand Down Expand Up @@ -148,21 +133,7 @@ impl<A, V> Tree<A, V> {
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
Expand All @@ -184,7 +155,7 @@ impl<A, V> Tree<A, V> {
left_incomplete
}
Node::Leaf { .. } => vec![],
Node::Nil | Node::Pruned => vec![root_addr],
Node::Nil => vec![root_addr],
}
}

Expand All @@ -202,7 +173,6 @@ impl<A, V> Tree<A, V> {
},
Node::Leaf { value } => Node::Leaf { value: f(value) },
Node::Nil => Node::Nil,
Node::Pruned => Node::Pruned,
})
}

Expand All @@ -221,7 +191,6 @@ impl<A, V> Tree<A, V> {
},
Node::Leaf { value } => Node::Leaf { value: f(value)? },
Node::Nil => Node::Nil,
Node::Pruned => Node::Pruned,
}))
}
}
Expand Down Expand Up @@ -269,25 +238,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> {
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit 25242e2

Please sign in to comment.