Skip to content

Commit

Permalink
shardtree: make construction of fully-empty Parent nodes an error
Browse files Browse the repository at this point in the history 
Ideally the errors here would all be panics, as construction of such a
node represents a programming error; however, it is preferable to return
extra contextual information about the circumstance that led to this
error where possible, so we model this as an error where possible
without altering the public API.
  • Loading branch information
@nuttycom
nuttycom committed Feb 17, 2025
1 parent 561a6dc commit 1d7cd42
Show file tree
Hide file tree
Showing 4 changed files with 254 additions and 171 deletions.
47 changes: 26 additions & 21 deletions shardtree/src/batch.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use tracing::trace;
use crate::{
error::{InsertionError, ShardTreeError},
store::{Checkpoint, ShardStore},
IncompleteAt, LocatedPrunableTree, LocatedTree, RetentionFlags, ShardTree, Tree,
IncompleteAt, LocatedPrunableTree, LocatedTree, PrunableTree, RetentionFlags, ShardTree, Tree,
};

impl<
Expand Down Expand Up @@ -215,7 +215,8 @@ impl<H: Hashable + Clone + PartialEq> LocatedPrunableTree<H> {
// fragments up the stack until we get the largest possible subtree
while let Some((potential_sibling, marked)) = fragments.pop() {
if potential_sibling.root_addr.parent() == subtree.root_addr.parent() {
subtree = unite(potential_sibling, subtree, prune_below);
subtree = unite(potential_sibling, subtree, prune_below)
.expect("subtree is non-Nil");
} else {
// this is not a sibling node, so we push it back on to the stack
// and are done
Expand All @@ -238,7 +239,9 @@ impl<H: Hashable + Clone + PartialEq> LocatedPrunableTree<H> {
let minimal_tree_addr =
Address::from(position_range.start).common_ancestor(&last_position.into());
trace!("Building minimal tree at {:?}", minimal_tree_addr);
build_minimal_tree(fragments, minimal_tree_addr, prune_below).map(
build_minimal_tree(fragments, minimal_tree_addr, prune_below)
.expect("construction of minimal tree does not result in creation of invalid parent nodes")
.map(
|(to_insert, contains_marked, incomplete)| BatchInsertionResult {
subtree: to_insert,
contains_marked,
Expand All @@ -263,16 +266,18 @@ fn unite<H: Hashable + Clone + PartialEq>(
lroot: LocatedPrunableTree<H>,
rroot: LocatedPrunableTree<H>,
prune_below: Level,
) -> LocatedPrunableTree<H> {
) -> Result<LocatedPrunableTree<H>, Address> {
assert_eq!(lroot.root_addr.parent(), rroot.root_addr.parent());
LocatedTree {
Ok(LocatedTree {
root_addr: lroot.root_addr.parent(),
root: if lroot.root_addr.level() < prune_below {
Tree::unite(lroot.root_addr.level(), None, lroot.root, rroot.root)
PrunableTree::unite(lroot.root_addr.level(), None, lroot.root, rroot.root)
.map_err(|_| lroot.root_addr)?
} else {
Tree::parent(None, lroot.root, rroot.root)
PrunableTree::parent_checked(None, lroot.root, rroot.root)
.map_err(|_| lroot.root_addr)?
},
}
})
}

/// Combines the given subtree with an empty sibling node to obtain the next level
Expand All @@ -286,7 +291,7 @@ fn combine_with_empty<H: Hashable + Clone + PartialEq>(
incomplete: &mut Vec<IncompleteAt>,
contains_marked: bool,
prune_below: Level,
) -> LocatedPrunableTree<H> {
) -> Result<LocatedPrunableTree<H>, Address> {
assert_eq!(expect_left_child, root.root_addr.is_left_child());
let sibling_addr = root.root_addr.sibling();
incomplete.push(IncompleteAt {
Expand All @@ -313,27 +318,27 @@ fn build_minimal_tree<H: Hashable + Clone + PartialEq>(
mut xs: Vec<(LocatedPrunableTree<H>, bool)>,
root_addr: Address,
prune_below: Level,
) -> Option<(LocatedPrunableTree<H>, bool, Vec<IncompleteAt>)> {
) -> Result<Option<(LocatedPrunableTree<H>, bool, Vec<IncompleteAt>)>, Address> {

Check failure on line 321 in shardtree/src/batch.rs

View workflow job for this annotation

GitHub Actions / Clippy (MSRV)

very complex type used. Consider factoring parts into `type` definitions 

error: very complex type used. Consider factoring parts into `type` definitions
   --> shardtree/src/batch.rs:321:6
    |
321 | ) -> Result<Option<(LocatedPrunableTree<H>, bool, Vec<IncompleteAt>)>, Address> {
    |      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `-D clippy::type-complexity` implied by `-D warnings`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#type_complexity

Check failure on line 321 in shardtree/src/batch.rs

View workflow job for this annotation

GitHub Actions / Clippy (MSRV)

very complex type used. Consider factoring parts into `type` definitions 

error: very complex type used. Consider factoring parts into `type` definitions
   --> shardtree/src/batch.rs:321:6
    |
321 | ) -> Result<Option<(LocatedPrunableTree<H>, bool, Vec<IncompleteAt>)>, Address> {
    |      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `-D clippy::type-complexity` implied by `-D warnings`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#type_complexity
// First, consume the stack from the right, building up a single tree
// until we can't combine any more.
if let Some((mut cur, mut contains_marked)) = xs.pop() {
let mut incomplete = vec![];
while let Some((top, top_marked)) = xs.pop() {
while cur.root_addr.level() < top.root_addr.level() {
cur = combine_with_empty(cur, true, &mut incomplete, top_marked, prune_below);
cur = combine_with_empty(cur, true, &mut incomplete, top_marked, prune_below)?;
}

if cur.root_addr.level() == top.root_addr.level() {
contains_marked = contains_marked || top_marked;
if cur.root_addr.is_right_child() {
// We have a left child and a right child, so unite them.
cur = unite(top, cur, prune_below);
cur = unite(top, cur, prune_below)?;
} else {
// This is a left child, so we build it up one more level and then
// we've merged as much as we can from the right and need to work from
// the left
xs.push((top, top_marked));
cur = combine_with_empty(cur, true, &mut incomplete, top_marked, prune_below);
cur = combine_with_empty(cur, true, &mut incomplete, top_marked, prune_below)?;
break;
}
} else {
Expand All @@ -346,15 +351,15 @@ fn build_minimal_tree<H: Hashable + Clone + PartialEq>(

// Ensure we can work from the left in a single pass by making this right-most subtree
while cur.root_addr.level() + 1 < root_addr.level() {
cur = combine_with_empty(cur, true, &mut incomplete, contains_marked, prune_below);
cur = combine_with_empty(cur, true, &mut incomplete, contains_marked, prune_below)?;
}

// push our accumulated max-height right hand node back on to the stack.
xs.push((cur, contains_marked));

// From the stack of subtrees, construct a single sparse tree that can be
// inserted/merged into the existing tree
let res_tree = xs.into_iter().fold(
let res_tree = xs.into_iter().try_fold(
None,
|acc: Option<LocatedPrunableTree<H>>, (next_tree, next_marked)| {
if let Some(mut prev_tree) = acc {
Expand All @@ -368,19 +373,19 @@ fn build_minimal_tree<H: Hashable + Clone + PartialEq>(
&mut incomplete,
next_marked,
prune_below,
);
)?;
}

Some(unite(prev_tree, next_tree, prune_below))
Ok::<_, Address>(Some(unite(prev_tree, next_tree, prune_below)?))
} else {
Some(next_tree)
Ok::<_, Address>(Some(next_tree))
}
},
);
)?;

res_tree.map(|t| (t, contains_marked, incomplete))
Ok(res_tree.map(|t| (t, contains_marked, incomplete)))
} else {
None
Ok(None)
}
}

Expand Down
80 changes: 49 additions & 31 deletions shardtree/src/legacy.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ impl<H: Hashable + Clone + PartialEq> LocatedPrunableTree<H> {
leaf_addr: Address,
mut filled: impl Iterator<Item = H>,
split_at: Level,
) -> (Self, Option<Self>) {
) -> Result<(Self, Option<Self>), Address> {
// add filled nodes to the subtree; here, we do not need to worry about
// whether or not these nodes can be invalidated by a rewind
let mut addr = leaf_addr;
Expand All @@ -113,17 +113,19 @@ impl<H: Hashable + Clone + PartialEq> LocatedPrunableTree<H> {
if let Some(right) = filled.next() {
// once we have a right-hand node, add a parent with the current tree
// as the left-hand sibling
subtree = Tree::parent(
subtree = PrunableTree::parent_checked(
None,
subtree,
Tree::leaf((right.clone(), RetentionFlags::EPHEMERAL)),
);
)
.map_err(|_| addr)?;
} else {
break;
}
} else {
// the current address is for a right child, so add an empty left sibling
subtree = Tree::parent(None, Tree::empty(), subtree);
subtree =
PrunableTree::parent_checked(None, Tree::empty(), subtree).map_err(|_| addr)?;
}

addr = addr.parent();
Expand All @@ -140,16 +142,22 @@ impl<H: Hashable + Clone + PartialEq> LocatedPrunableTree<H> {
for right in filled {
// build up the right-biased tree until we get a left-hand node
while addr.is_right_child() {
supertree = supertree.map(|t| Tree::parent(None, Tree::empty(), t));
supertree = supertree
.map(|t| PrunableTree::parent_checked(None, Tree::empty(), t))
.transpose()
.map_err(|_| addr)?;
addr = addr.parent();
}

// once we have a left-hand root, add a parent with the current ommer as the right-hand sibling
supertree = Some(Tree::parent(
None,
supertree.unwrap_or_else(PrunableTree::empty),
Tree::leaf((right.clone(), RetentionFlags::EPHEMERAL)),
));
supertree = Some(
PrunableTree::parent_checked(
None,
supertree.unwrap_or_else(PrunableTree::empty),
Tree::leaf((right.clone(), RetentionFlags::EPHEMERAL)),
)
.map_err(|_| addr)?,
);
addr = addr.parent();
}

Expand All @@ -161,7 +169,7 @@ impl<H: Hashable + Clone + PartialEq> LocatedPrunableTree<H> {
None
};

(subtree, supertree)
Ok((subtree, supertree))
}

/// Insert the nodes belonging to the given incremental witness to this tree, truncating the
Expand Down Expand Up @@ -196,23 +204,30 @@ impl<H: Hashable + Clone + PartialEq> LocatedPrunableTree<H> {
Address::from(witness.witnessed_position()),
witness.filled().iter().cloned(),
self.root_addr.level(),
);
)
.map_err(InsertionError::InputMalformed)?;

// construct subtrees from the `cursor` part of the witness
let cursor_trees = witness.cursor().as_ref().filter(|c| c.size() > 0).map(|c| {
Self::from_frontier_parts(
witness.tip_position(),
c.leaf()
.cloned()
.expect("Cannot have an empty leaf for a non-empty tree"),
c.ommers_iter().cloned(),
&Retention::Checkpoint {
id: checkpoint_id,
marking: Marking::None,
},
self.root_addr.level(),
)
});
let cursor_trees = witness
.cursor()
.as_ref()
.filter(|c| c.size() > 0)
.map(|c| {
Self::from_frontier_parts(
witness.tip_position(),
c.leaf()
.cloned()
.expect("Cannot have an empty leaf for a non-empty tree"),
c.ommers_iter().cloned(),
&Retention::Checkpoint {
id: checkpoint_id,
marking: Marking::None,
},
self.root_addr.level(),
)
})
.transpose()
.map_err(InsertionError::InputMalformed)?;

let (subtree, _) = past_subtree.insert_subtree(future_subtree, true)?;

Expand Down Expand Up @@ -253,7 +268,7 @@ mod tests {
frontier::CommitmentTree, witness::IncrementalWitness, Address, Level, Position,
};

use crate::{LocatedPrunableTree, RetentionFlags, Tree};
use crate::{LocatedPrunableTree, PrunableTree, RetentionFlags, Tree};

#[test]
fn insert_witness_nodes() {
Expand All @@ -280,19 +295,22 @@ mod tests {

assert_eq!(
c.root,
Tree::parent(
PrunableTree::parent_checked(
None,
Tree::parent(
PrunableTree::parent_checked(
None,
Tree::empty(),
Tree::leaf(("ijklmnop".to_string(), RetentionFlags::EPHEMERAL)),
),
Tree::parent(
)
.unwrap(),
PrunableTree::parent_checked(
None,
Tree::leaf(("qrstuvwx".to_string(), RetentionFlags::EPHEMERAL)),
Tree::empty()
)
.unwrap()
)
.unwrap()
);

assert_eq!(
Expand Down
46 changes: 24 additions & 22 deletions shardtree/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -414,44 +414,44 @@ impl<
fn go<H: Hashable + Clone + PartialEq>(
root_addr: Address,
root: &PrunableTree<H>,
) -> Option<(PrunableTree<H>, Position)> {
) -> Result<Option<(PrunableTree<H>, Position)>, Address> {
match &root.0 {
Node::Parent { ann, left, right } => {
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(r_addr, right)?.map_or_else(
|| {
go(l_addr, left).map(|(new_left, pos)| {
(
Tree::unite(
go(l_addr, left)?
.map(|(new_left, pos)| {
PrunableTree::unite(
l_addr.level(),
ann.clone(),
new_left,
Tree::empty(),
),
pos,
)
})
)
.map_err(|_| l_addr)
.map(|t| (t, pos))
})
.transpose()
},
|(new_right, pos)| {
Some((
Tree::unite(
l_addr.level(),
ann.clone(),
left.as_ref().clone(),
new_right,
),
pos,
))
Tree::unite(
l_addr.level(),
ann.clone(),
left.as_ref().clone(),
new_right,
)
.map_err(|_| l_addr)
.map(|t| Some((t, pos)))
},
)
}
Node::Leaf { value: (h, r) } => Some((
Node::Leaf { value: (h, r) } => Ok(Some((
Tree::leaf((h.clone(), *r | RetentionFlags::CHECKPOINT)),
root_addr.max_position(),
)),
Node::Nil => None,
))),
Node::Nil => Ok(None),
}
}

Expand All @@ -469,7 +469,9 @@ impl<
// Update the rightmost subtree to add the `CHECKPOINT` flag to the right-most leaf (which
// need not be a level-0 leaf; it's fine to rewind to a pruned state).
if let Some(subtree) = self.store.last_shard().map_err(ShardTreeError::Storage)? {
if let Some((replacement, pos)) = go(subtree.root_addr, &subtree.root) {
if let Some((replacement, pos)) = go(subtree.root_addr, &subtree.root)
.map_err(|addr| ShardTreeError::Insert(InsertionError::InputMalformed(addr)))?
{
self.store
.put_shard(LocatedTree {
root_addr: subtree.root_addr,
Expand Down
Loading

0 comments on commit 1d7cd42

Please sign in to comment.