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 463fec6
Show file tree
Hide file tree
Showing 4 changed files with 214 additions and 148 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> {
// 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
17 changes: 9 additions & 8 deletions shardtree/src/legacy.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ 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)),
Expand All @@ -123,7 +123,7 @@ impl<H: Hashable + Clone + PartialEq> LocatedPrunableTree<H> {
}
} 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);

Check failure on line 126 in shardtree/src/legacy.rs

View workflow job for this annotation

GitHub Actions / Clippy (MSRV)

mismatched types 

error[E0308]: mismatched types
   --> shardtree/src/legacy.rs:126:27
    |
108 |         let mut subtree = Tree::empty();
    |                           ------------- expected due to this value
...
126 |                 subtree = PrunableTree::parent_checked(None, Tree::empty(), subtree);
    |                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `tree::Tree`, found enum `std::result::Result`
    |
    = note: expected struct `tree::Tree<_, _>`
                 found enum `std::result::Result<tree::Tree<_, _>, ()>`

Check failure on line 126 in shardtree/src/legacy.rs

View workflow job for this annotation

GitHub Actions / Clippy (MSRV)

mismatched types 

error[E0308]: mismatched types
   --> shardtree/src/legacy.rs:126:27
    |
108 |         let mut subtree = Tree::empty();
    |                           ------------- expected due to this value
...
126 |                 subtree = PrunableTree::parent_checked(None, Tree::empty(), subtree);
    |                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `tree::Tree`, found enum `std::result::Result`
    |
    = note: expected struct `tree::Tree<_, _>`
                 found enum `std::result::Result<tree::Tree<_, _>, ()>`
}

addr = addr.parent();
Expand All @@ -140,12 +140,13 @@ 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));

Check failure on line 144 in shardtree/src/legacy.rs

View workflow job for this annotation

GitHub Actions / Clippy (MSRV)

mismatched types 

error[E0308]: mismatched types
   --> shardtree/src/legacy.rs:144:25
    |
139 |             let mut supertree = None;
    |                                 ---- expected due to this value
...
144 |                         supertree.map(|t| PrunableTree::parent_checked(None, Tree::empty(), t));
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `tree::Tree`, found enum `std::result::Result`
    |
    = note: expected enum `std::option::Option<tree::Tree<std::option::Option<std::sync::Arc<_>>, (_, prunable::RetentionFlags)>>`
               found enum `std::option::Option<std::result::Result<tree::Tree<std::option::Option<std::sync::Arc<_>>, (_, prunable::RetentionFlags)>, ()>>`

Check failure on line 144 in shardtree/src/legacy.rs

View workflow job for this annotation

GitHub Actions / Clippy (MSRV)

mismatched types 

error[E0308]: mismatched types
   --> shardtree/src/legacy.rs:144:25
    |
139 |             let mut supertree = None;
    |                                 ---- expected due to this value
...
144 |                         supertree.map(|t| PrunableTree::parent_checked(None, Tree::empty(), t));
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `tree::Tree`, found enum `std::result::Result`
    |
    = note: expected enum `std::option::Option<tree::Tree<std::option::Option<std::sync::Arc<_>>, (_, prunable::RetentionFlags)>>`
               found enum `std::option::Option<std::result::Result<tree::Tree<std::option::Option<std::sync::Arc<_>>, (_, prunable::RetentionFlags)>, ()>>`
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(
supertree = Some(PrunableTree::parent_checked(
None,
supertree.unwrap_or_else(PrunableTree::empty),
Tree::leaf((right.clone(), RetentionFlags::EPHEMERAL)),
Expand Down Expand Up @@ -253,7 +254,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,14 +281,14 @@ mod tests {

assert_eq!(
c.root,
Tree::parent(
PrunableTree::parent_checked(

Check failure on line 284 in shardtree/src/legacy.rs

View workflow job for this annotation

GitHub Actions / Clippy (MSRV)

arguments to this function are incorrect 

error[E0308]: arguments to this function are incorrect
   --> shardtree/src/legacy.rs:284:17
    |
284 |                   PrunableTree::parent_checked(
    |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
285 |                       None,
286 | /                     PrunableTree::parent_checked(
287 | |                         None,
288 | |                         Tree::empty(),
289 | |                         Tree::leaf(("ijklmnop".to_string(), RetentionFlags::EPHEMERAL)),
290 | |                     ),
    | |_____________________- expected struct `tree::Tree`, found enum `std::result::Result`
291 | /                     PrunableTree::parent_checked(
292 | |                         None,
293 | |                         Tree::leaf(("qrstuvwx".to_string(), RetentionFlags::EPHEMERAL)),
294 | |                         Tree::empty()
295 | |                     )
    | |_____________________- expected struct `tree::Tree`, found enum `std::result::Result`
    |
    = note: expected struct `tree::Tree<std::option::Option<std::sync::Arc<_>>, (_, prunable::RetentionFlags)>`
                 found enum `std::result::Result<tree::Tree<std::option::Option<std::sync::Arc<std::string::String>>, (std::string::String, prunable::RetentionFlags)>, ()>`
    = note: expected struct `tree::Tree<std::option::Option<std::sync::Arc<_>>, (_, prunable::RetentionFlags)>`
                 found enum `std::result::Result<tree::Tree<std::option::Option<std::sync::Arc<std::string::String>>, (std::string::String, prunable::RetentionFlags)>, ()>`
note: associated function defined here
   --> shardtree/src/prunable.rs:272:19
    |
272 |     pub(crate) fn parent_checked(ann: Option<Arc<H>>, left: Self, right: Self) -> Result<Self, ()> {
    |                   ^^^^^^^^^^^^^^ -------------------  ----------  -----------
None,
Tree::parent(
PrunableTree::parent_checked(
None,
Tree::empty(),
Tree::leaf(("ijklmnop".to_string(), RetentionFlags::EPHEMERAL)),
),
Tree::parent(
PrunableTree::parent_checked(
None,
Tree::leaf(("qrstuvwx".to_string(), RetentionFlags::EPHEMERAL)),
Tree::empty()
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 463fec6

Please sign in to comment.