diff --git a/durable-storage/benches/avl_tree.rs b/durable-storage/benches/avl_tree.rs index 4d5e58421d3..148d14a98d5 100644 --- a/durable-storage/benches/avl_tree.rs +++ b/durable-storage/benches/avl_tree.rs @@ -54,7 +54,8 @@ fn bench_avl_tree_operations(c: &mut Criterion) { let mut tree = Tree::default(); for key in &keys[..keys.len() / 2] { let random_data = generate_random_bytes_in_range(&mut rng, 1..20); - tree.set(key, &random_data, &mut resolver); + tree.set(key, &random_data, &mut resolver) + .expect("Hit error on setting a value in setup."); } c.bench_function("Bench AVL tree with operations", |b| { @@ -64,10 +65,12 @@ fn bench_avl_tree_operations(c: &mut Criterion) { for operation in operations { match operation { Operation::Upsert(key, value) => { - tree.set(&key, &value, &mut resolver); + tree.set(&key, &value, &mut resolver) + .expect("Hit error on setting a value in bench."); } Operation::Delete(key) => { - tree.delete(&key, &mut resolver); + tree.delete(&key, &mut resolver) + .expect("Hit error on deleting a value in bench."); } } } diff --git a/durable-storage/src/avl/node.rs b/durable-storage/src/avl/node.rs index 49e9587c2b1..fc6f8515875 100644 --- a/durable-storage/src/avl/node.rs +++ b/durable-storage/src/avl/node.rs @@ -16,11 +16,24 @@ use octez_riscv_data::mode::Normal; use super::tree::Tree; use crate::avl::resolver::Resolver; +use crate::errors::OperationalError; use crate::key::Key; /// Value stored in a node pub type Value = Bytes; +#[derive(Encode)] +/// A serialisable representation of [`Node`]. +struct NodeHashRepresentation<'a, Value> { + key: &'a Key, + data: Value, + // The bytes of the hash of an optional left child + left: Option<&'a Hash>, + // The bytes of the hash of an optional right child + right: Option<&'a Hash>, + balance_factor: i64, +} + /// A node that supports rebalancing and Merklisation. #[derive(Clone, Default, Debug)] pub struct Node { @@ -39,18 +52,6 @@ pub struct Node { balance_factor: i64, } -#[derive(Encode)] -/// A serialisable representation of [`Node`]. -struct NodeHashRepresentation<'a, Value> { - key: &'a Key, - data: Value, - // The bytes of the hash of an optional left child - left: Option<&'a Hash>, - // The bytes of the hash of an optional right child - right: Option<&'a Hash>, - balance_factor: i64, -} - impl Node { /// Create a new leaf [`Node`] from the given key and data. pub(crate) fn new(key: Key, data: impl Into) -> Self { @@ -64,21 +65,38 @@ impl Node { /// Converts the [`Node`] to an encoded, serialisable representation, /// [`NodeHashRepresentation`], potentially re-hashing uncached [`Node`]s. - pub(crate) fn to_encode(&self, resolver: &impl Resolver, Node>) -> impl Encode + '_ { + pub(crate) fn to_encode<'a>( + &'a self, + resolver: &impl Resolver, Node>, + ) -> impl Encode + 'a { + // Recursively hashes any left child and its children. Stops when a hash was cached or a + // node is blinded. + let left = self.left_ref().root().map(|node| resolver.hash(node)); + + // Recursively hashes any right child and its children. Stops when a hash was cached or a + // node is blinded. + let right = self.right_ref().root().map(|node| resolver.hash(node)); + NodeHashRepresentation { key: &self.key, data: &self.data, - - // Recursively hashes any left child and its children - left: self.left_ref().root().map(|node| hash(node, resolver)), - - // Recursively hashes any right child and its children - right: self.right_ref().root().map(|node| hash(node, resolver)), - + left, + right, balance_factor: self.balance_factor, } } + /// Returns the hash of this node. + /// + /// If the hash has been cached, the memo is returned. Otherwise, the hash is calculated and + /// cached. + pub(crate) fn hash(&self, resolver: &impl Resolver, Self>) -> &Hash { + self.hash.get_or_init(|| { + let data = self.to_encode(resolver); + Hash::hash_encodable(data).expect("The hashing should not fail") + }) + } + #[inline] /// The difference in heights between child branches. pub(super) fn balance_factor(&self) -> i64 { @@ -115,16 +133,19 @@ impl Node { /// /// The subtree of the [`Node`] must already have balance factor in the range of -2..=2, else /// it is an invalid AVL tree. - pub(super) fn rebalance(node: &mut Arc, resolver: &mut impl Resolver, Node>) { - let resolved_node = resolver.resolve(node); + pub(super) fn rebalance( + node: &mut Arc, + resolver: &mut impl Resolver, Node>, + ) -> Result<(), OperationalError> { + let resolved_node = resolver.resolve(node)?; let balance_factor = resolved_node.balance_factor(); match balance_factor { 2 => { - let right_balance = resolved_node.right_ref().balance_factor(resolver); + let right_balance = resolved_node.right_ref().balance_factor(resolver)?; match right_balance { - 1 | 0 => Self::rotate_left(node, resolver), - -1 => Self::rotate_right_left(node, resolver), + 1 | 0 => Self::rotate_left(node, resolver)?, + -1 => Self::rotate_right_left(node, resolver)?, _ => panic!( "Rebalancing an invalid AVL tree. The balance factor of the right node is {right_balance:?}, but it should be in the range of -1..=1" ), @@ -132,11 +153,11 @@ impl Node { } -1..=1 => (), -2 => { - let left_balance = resolved_node.left_ref().balance_factor(resolver); + let left_balance = resolved_node.left_ref().balance_factor(resolver)?; match left_balance { - 1 => Self::rotate_left_right(node, resolver), - -1 | 0 => Self::rotate_right(node, resolver), + 1 => Self::rotate_left_right(node, resolver)?, + -1 | 0 => Self::rotate_right(node, resolver)?, _ => panic!( "Rebalancing an invalid AVL tree. The balance factor of the left node is {left_balance:?}, but it should be in the range of -1..=1" ), @@ -146,7 +167,8 @@ impl Node { "Rebalancing an invalid AVL tree. The balance factor is {:?}, but it should be in the range of -2..=2", resolved_node.balance_factor() ), - } + }; + Ok(()) } /// Remove the successor of the [`Node`] from its subtree and replace the original [`Node`] @@ -155,24 +177,26 @@ impl Node { /// Returns a tuple of: /// - The [`Node`] at the root of the new subtree. /// - `true` if the [`Tree`] has shrunk in size. - #[must_use] pub(super) fn replace_with_successor( node: &mut Arc, resolver: &mut impl Resolver, Node>, - ) -> (Arc, bool) { - let node_mut = resolver.resolve_mut(node); + ) -> Result<(Arc, bool), OperationalError> { + let node_mut = resolver.resolve_mut(node)?; // If the right child has a left child, the successor is the min of the left child's subtree. - let (mut successor, shrank) = if node_mut - .right_ref() - .root() - .expect("A node with a successor must have a right child") + let (mut successor, shrank) = if resolver + .resolve( + node_mut + .right_ref() + .root() + .expect("A node with a successor must have a right child"), + )? .left_ref() .root() .is_some() { let right = node_mut.right_mut(); - let (mut min, _, shrank) = Tree::take_min(right, resolver); + let (mut min, _, shrank) = Tree::take_min(right, resolver)?; ( min.take() .expect("A node with a successor must have a right child"), @@ -184,23 +208,23 @@ impl Node { .right_mut() .take() .expect("A node with a successor must have a right child"); - let successor_mut = resolver.resolve_mut(&mut successor); + let successor_mut = resolver.resolve_mut(&mut successor)?; // Bump up the (optional) right child of the right child, causing the subtree to shrink. node_mut.right = successor_mut.right.take().into(); (successor, true) }; - let successor_mut = resolver.resolve_mut(&mut successor); + let successor_mut = resolver.resolve_mut(&mut successor)?; successor_mut.balance_factor = node_mut.balance_factor() - if shrank { 1 } else { 0 }; successor_mut.left = std::mem::take(&mut node_mut.left); successor_mut.right = std::mem::take(&mut node_mut.right); - Self::rebalance(&mut successor, resolver); + Self::rebalance(&mut successor, resolver)?; let shrank = node_mut.balance_factor().abs() == 1 && successor.balance_factor == 0; - (successor, shrank) + Ok((successor, shrank)) } #[inline] @@ -223,16 +247,15 @@ impl Node { /// - The occupied [`Tree`] with the minimum [`Key`]. /// - The minimum [`Tree`]'s right child, if it hasn't been moved to its new position. /// - True if this [`Node`]'s subtree has shrunk in size. - #[must_use] pub(super) fn take_min( node: &mut Arc, resolver: &mut impl Resolver, Node>, - ) -> (Tree, Tree, bool) { - let node_mut = resolver.resolve_mut(node); + ) -> Result<(Tree, Tree, bool), OperationalError> { + let node_mut = resolver.resolve_mut(node)?; let old_node_bf = node_mut.balance_factor(); let left = node_mut.left_mut(); - let (min, right, left_shrank) = Tree::take_min(left, resolver); + let (min, right, left_shrank) = Tree::take_min(left, resolver)?; if right.root().is_some() { *node_mut.left_mut() = right; @@ -241,12 +264,12 @@ impl Node { node_mut.balance_factor += 1; }; - Node::rebalance(node, resolver); - ( + Node::rebalance(node, resolver)?; + Ok(( min, None.into(), - old_node_bf.abs() == 1 && resolver.resolve(node).balance_factor() == 0, - ) + old_node_bf.abs() == 1 && resolver.resolve(node)?.balance_factor() == 0, + )) } /// Set or update the value of a [`Node`] in this [`Node`]'s subtree with a given key and given @@ -261,7 +284,7 @@ impl Node { offset: usize, data: impl FnOnce(&mut Value), resolver: &mut impl Resolver, Node>, - ) -> bool { + ) -> Result { // SAFETY: The default recursion limit in Rust is 128 // see: // @@ -284,21 +307,21 @@ impl Node { Ordering::Equal => { data(&mut self.data); self.invalidate_hash(); - false + Ok(false) } Ordering::Greater => { - let grew = self.left_mut().upsert(key, offset, data, resolver); + let grew = self.left_mut().upsert(key, offset, data, resolver)?; if grew { self.balance_factor -= 1; } - grew + Ok(grew) } Ordering::Less => { - let grew = self.right_mut().upsert(key, offset, data, resolver); + let grew = self.right_mut().upsert(key, offset, data, resolver)?; if grew { self.balance_factor += 1; } - grew + Ok(grew) } } } @@ -323,13 +346,16 @@ impl Node { /// /// Assumes this [`Node`]'s balance factor is 2 and the right [`Node`]'s balance factor is +1 /// or 0. - fn rotate_left(node: &mut Arc, resolver: &mut impl Resolver, Node>) { - let node_mut = resolver.resolve_mut(node); + fn rotate_left( + node: &mut Arc, + resolver: &mut impl Resolver, Node>, + ) -> Result<(), OperationalError> { + let node_mut = resolver.resolve_mut(node)?; let mut right = node_mut .right_mut() .take() .expect("There should be a right node to rotate left"); - let right_mut = resolver.resolve_mut(&mut right); + let right_mut = resolver.resolve_mut(&mut right)?; *node_mut.right_mut() = right_mut.left_mut().take().into(); @@ -364,7 +390,8 @@ impl Node { right_mut.balance_factor = right_mut.balance_factor - 1 + std::cmp::min(new_node_bf, 0); *right_mut.left_mut() = Some(node.clone()).into(); - *node = right + *node = right; + Ok(()) } /// Rotate the left child of this [`Node`] left, then this [`Node`]'s subtree right. @@ -383,14 +410,17 @@ impl Node { /// ``` /// /// Assumes this [`Node`]'s balance factor is -2 and the left [Node]'s balance factor is +1. - fn rotate_left_right(node: &mut Arc, resolver: &mut impl Resolver, Node>) { - let node_mut = resolver.resolve_mut(node); + fn rotate_left_right( + node: &mut Arc, + resolver: &mut impl Resolver, Node>, + ) -> Result<(), OperationalError> { + let node_mut = resolver.resolve_mut(node)?; let mut left = node_mut .left_mut() .take() .expect("Left child must exist for the right rotation of the node"); - let left_mut = resolver.resolve_mut(&mut left); + let left_mut = resolver.resolve_mut(&mut left)?; let mut left_right = left_mut .right_mut() @@ -404,7 +434,7 @@ impl Node { // The second rotation doesn't mutate A's subtree, so the final balance factor is: left_mut.balance_factor = std::cmp::min(-left_right.balance_factor, 0); - let left_right_mut = resolver.resolve_mut(&mut left_right); + let left_right_mut = resolver.resolve_mut(&mut left_right)?; // B's right child is between B and B, it's moved to node's left node_mut.left = left_right_mut.right.take().into(); @@ -424,7 +454,8 @@ impl Node { // The new root will always be balanced left_right_mut.balance_factor = 0; - *node = left_right + *node = left_right; + Ok(()) } /// Rotate this [`Node`]'s subtree right. @@ -442,13 +473,16 @@ impl Node { /// /// Assumes this [`Node`]'s balance factor is -2 and the left [`Node`]'s balance factor is -1 /// or 0. - fn rotate_right(node: &mut Arc, resolver: &mut impl Resolver, Node>) { - let node_mut = resolver.resolve_mut(node); + fn rotate_right( + node: &mut Arc, + resolver: &mut impl Resolver, Node>, + ) -> Result<(), OperationalError> { + let node_mut = resolver.resolve_mut(node)?; let mut left = node_mut .left_mut() .take() .expect("There should be a left node to rotate right"); - let left_mut = resolver.resolve_mut(&mut left); + let left_mut = resolver.resolve_mut(&mut left)?; *node_mut.left_mut() = left_mut.right_mut().take().into(); @@ -483,7 +517,8 @@ impl Node { left_mut.balance_factor = left_mut.balance_factor + 1 + std::cmp::max(new_node_bf, 0); *left_mut.right_mut() = Some(node.clone()).into(); - *node = left + *node = left; + Ok(()) } /// Rotate the right child of this [`Node`] right, then this [`Node`]'s subtree left. @@ -502,13 +537,16 @@ impl Node { /// ``` /// /// Assumes this [`Node`]'s balance factor is +2 and the left [Node]'s balance factor is -1. - fn rotate_right_left(node: &mut Arc, resolver: &mut impl Resolver, Node>) { - let node_mut = resolver.resolve_mut(node); + fn rotate_right_left( + node: &mut Arc, + resolver: &mut impl Resolver, Node>, + ) -> Result<(), OperationalError> { + let node_mut = resolver.resolve_mut(node)?; let mut right = node_mut .right_mut() .take() .expect("Right child must exist for the left rotation of the node"); - let right_mut = resolver.resolve_mut(&mut right); + let right_mut = resolver.resolve_mut(&mut right)?; let mut right_left = right_mut .left_mut() @@ -522,7 +560,7 @@ impl Node { // The second rotation doesn't mutate A's subtree, so the final balance factor is: right_mut.balance_factor = std::cmp::max(0, -right_left.balance_factor); - let right_left_mut = resolver.resolve_mut(&mut right_left); + let right_left_mut = resolver.resolve_mut(&mut right_left)?; // B's left child is between node and B, it's moved to node's right node_mut.right = right_left_mut.left.take().into(); @@ -542,20 +580,11 @@ impl Node { // The new root will always be balanced right_left_mut.balance_factor = 0; - *node = right_left + *node = right_left; + Ok(()) } } -/// Returns the hash of this node, including recursively hashing any child nodes. -/// -/// If the hash has been cached, the memo is returned. Otherwise, the hash is calculated and -/// cached. -pub(crate) fn hash<'a>(node: &'a Arc, resolver: &impl Resolver, Node>) -> &'a Hash { - resolver.resolve(node).hash.get_or_init(|| { - Hash::hash_encodable(node.to_encode(resolver)).expect("The hashing should not fail") - }) -} - #[cfg(test)] impl Node { #[inline] @@ -569,13 +598,23 @@ impl Node { mut node: &'a Arc, key: &Key, resolver: &impl Resolver, Node>, - ) -> Option<&'a Value> { + ) -> Result, OperationalError> { loop { - let resolved_node = resolver.resolve(node); + let resolved_node = resolver.resolve(node)?; match resolved_node.key().cmp(key) { - Ordering::Equal => return Some(resolved_node.data()), - Ordering::Greater => node = resolved_node.left_ref().root()?, - Ordering::Less => node = resolved_node.right_ref().root()?, + Ordering::Equal => return Ok(Some(resolved_node.data())), + Ordering::Greater => { + let Some(left) = resolved_node.left_ref().root() else { + return Ok(None); + }; + node = left; + } + Ordering::Less => { + let Some(right) = resolved_node.right_ref().root() else { + return Ok(None); + }; + node = right; + } } } } @@ -584,36 +623,45 @@ impl Node { pub(super) fn has_correct_balance_factors( &self, resolver: &impl Resolver, Node>, - ) -> bool { - let left_height = self.left_ref().height(resolver); - let right_height = self.right_ref().height(resolver); + ) -> Result { + let left_height = self.left_ref().height(resolver)?; + let right_height = self.right_ref().height(resolver)?; let calculated_balance_factor = right_height as i64 - left_height as i64; if self.balance_factor() != calculated_balance_factor { eprintln!( "Node has balance_factor {:?}, should be {calculated_balance_factor:?}\nnode: {self:?}", self.balance_factor() ); - return false; + return Ok(false); } - self.left_ref().has_correct_balance_factors(resolver) - && self.right_ref().has_correct_balance_factors(resolver) + let res = self.left_ref().has_correct_balance_factors(resolver)? + && self.right_ref().has_correct_balance_factors(resolver)?; + Ok(res) } /// Returns the height of this [`Node`]'s subtree. - pub(super) fn height(&self, resolver: &impl Resolver, Node>) -> u32 { - let left_height = self.left_ref().height(resolver); - let right_height = self.right_ref().height(resolver); - 1 + std::cmp::max(left_height, right_height) + pub(super) fn height( + &self, + resolver: &impl Resolver, Node>, + ) -> Result { + let left_height = self.left_ref().height(resolver)?; + let right_height = self.right_ref().height(resolver)?; + Ok(1 + std::cmp::max(left_height, right_height)) } /// Returns true if this [`Node`]'s subtree is balanced. - pub(super) fn is_balanced(&self, resolver: &impl Resolver, Node>) -> bool { + pub(super) fn is_balanced( + &self, + resolver: &impl Resolver, Node>, + ) -> Result { let balance_factor = self.balance_factor(); if balance_factor.abs() > 1 { eprintln!("Balance factor not in -1..=1: {balance_factor:?}"); - return false; + return Ok(false); } - self.left_ref().is_balanced(resolver) && self.right_ref().is_balanced(resolver) + let res = + self.left_ref().is_balanced(resolver)? && self.right_ref().is_balanced(resolver)?; + Ok(res) } /// Returns true if this [`Node`]'s subtree is in-order. @@ -622,11 +670,16 @@ impl Node { min: &Key, max: &Key, resolver: &impl Resolver, Node>, - ) -> bool { + ) -> Result { if self.key() < min || self.key() > max { - return false; + return Ok(false); } - self.left_ref().is_inorder_inner(min, self.key(), resolver) - && self.right_ref().is_inorder_inner(self.key(), max, resolver) + let res = self + .left_ref() + .is_inorder_inner(min, self.key(), resolver)? + && self + .right_ref() + .is_inorder_inner(self.key(), max, resolver)?; + Ok(res) } } diff --git a/durable-storage/src/avl/resolver.rs b/durable-storage/src/avl/resolver.rs index d60d080fc56..39897b396be 100644 --- a/durable-storage/src/avl/resolver.rs +++ b/durable-storage/src/avl/resolver.rs @@ -9,25 +9,41 @@ use std::sync::Arc; +use octez_riscv_data::hash::Hash; + +use super::node::Node; +use crate::errors::OperationalError; + /// Trait for resolving identifiers to values. pub trait Resolver { + /// Retrieve the hash using the identifier. + /// + /// This will not perform any resolution. If the identifier is for a blinded node, then the + /// identity is the hash. In case the node is not blinded, there is no resolution needed + /// anyway. + fn hash<'a>(&self, id: &'a Id) -> &'a Hash; + /// Resolve an identifier to a value. - fn resolve<'a>(&self, id: &'a Id) -> &'a Value; + fn resolve<'a>(&self, id: &'a Id) -> Result<&'a Value, OperationalError>; /// Resolve an identifier to a mutable value. - fn resolve_mut<'a>(&mut self, id: &'a mut Id) -> &'a mut Value; + fn resolve_mut<'a>(&mut self, id: &'a mut Id) -> Result<&'a mut Value, OperationalError>; } /// Provide values identified by an [`Arc`]. #[derive(Clone, Debug)] pub struct ArcResolver; -impl Resolver, T> for ArcResolver { - fn resolve<'a>(&self, id: &'a Arc) -> &'a T { - id.as_ref() +impl Resolver, Node> for ArcResolver { + fn hash<'a>(&self, id: &'a Arc) -> &'a Hash { + id.hash(self) + } + + fn resolve<'a>(&self, id: &'a Arc) -> Result<&'a Node, OperationalError> { + Ok(id.as_ref()) } - fn resolve_mut<'a>(&mut self, id: &'a mut Arc) -> &'a mut T { - Arc::make_mut(id) + fn resolve_mut<'a>(&mut self, id: &'a mut Arc) -> Result<&'a mut Node, OperationalError> { + Ok(Arc::make_mut(id)) } } diff --git a/durable-storage/src/avl/tree.rs b/durable-storage/src/avl/tree.rs index 23d5b46bd29..604a1673fb6 100644 --- a/durable-storage/src/avl/tree.rs +++ b/durable-storage/src/avl/tree.rs @@ -13,6 +13,7 @@ use octez_riscv_data::hash::Hash; use super::node::Node; use super::node::Value; use crate::avl::resolver::Resolver; +use crate::errors::OperationalError; #[cfg(test)] use crate::key::KEY_MAX_SIZE; use crate::key::Key; @@ -25,14 +26,18 @@ impl Tree { /// Delete the [`Node`] in the [`Tree`] with a given key. /// /// Returns true if the [`Tree`] has shrunk in size. - pub fn delete(&mut self, key: &Key, resolver: &mut impl Resolver, Node>) -> bool { - let old_balance_factor = self.balance_factor(resolver); + pub fn delete( + &mut self, + key: &Key, + resolver: &mut impl Resolver, Node>, + ) -> Result { + let old_balance_factor = self.balance_factor(resolver)?; let Some(node) = self.root_mut() else { // The key does not exist so nothing will happen. - return false; + return Ok(false); }; - let resolved_node = resolver.resolve(node); + let resolved_node = resolver.resolve(node)?; match resolved_node.key().cmp(key) { Ordering::Equal => match ( resolved_node.left_ref().root(), @@ -40,35 +45,35 @@ impl Tree { ) { (None, None) => { self.take(); - true + Ok(true) } (Some(left), None) => { *node = left.clone(); - true + Ok(true) } (None, Some(right)) => { *node = right.clone(); - true + Ok(true) } (Some(_), Some(_)) => { - let (new_node, shrank) = Node::replace_with_successor(node, resolver); + let (new_node, shrank) = Node::replace_with_successor(node, resolver)?; *node = new_node; - shrank + Ok(shrank) } }, Ordering::Greater => { - let node_mut = resolver.resolve_mut(node); - let left_shrank = node_mut.left_mut().delete(key, resolver); + let node_mut = resolver.resolve_mut(node)?; + let left_shrank = node_mut.left_mut().delete(key, resolver)?; *node_mut.balance_factor_mut() += if left_shrank { 1 } else { 0 }; - self.rebalance(resolver); - old_balance_factor.abs() == 1 && self.balance_factor(resolver) == 0 + self.rebalance(resolver)?; + Ok(old_balance_factor.abs() == 1 && self.balance_factor(resolver)? == 0) } Ordering::Less => { - let node_mut = resolver.resolve_mut(node); - let right_shrank = node_mut.right_mut().delete(key, resolver); + let node_mut = resolver.resolve_mut(node)?; + let right_shrank = node_mut.right_mut().delete(key, resolver)?; *node_mut.balance_factor_mut() -= if right_shrank { 1 } else { 0 }; - self.rebalance(resolver); - old_balance_factor.abs() == 1 && self.balance_factor(resolver) == 0 + self.rebalance(resolver)?; + Ok(old_balance_factor.abs() == 1 && self.balance_factor(resolver)? == 0) } } } @@ -82,7 +87,7 @@ impl Tree { key: &Key, data: &[u8], resolver: &mut impl Resolver, Node>, - ) -> bool { + ) -> Result { self.upsert(key, 0, |old_data| old_data.set(data), resolver) } @@ -90,9 +95,12 @@ impl Tree { /// /// If the [`struct@Hash`] has been cached, the memo is returned. Otherwise, the /// [`struct@Hash`] is calculated and cached. - pub(crate) fn hash(&self, resolver: &impl Resolver, Node>) -> Hash { + pub(crate) fn hash( + &self, + resolver: &impl Resolver, Node>, + ) -> Result { let encodable = self.0.as_deref().map(|node| node.to_encode(resolver)); - Hash::hash_encodable(encodable).expect("Should be hashable") + Ok(Hash::hash_encodable(encodable).expect("Should be hashable")) } /// Creates an in-order iterator for the [`Node`]s in the [`Tree`] @@ -110,10 +118,17 @@ impl Tree { #[inline] /// The difference in heights between any child branches in the [`Tree`]. - pub(super) fn balance_factor(&self, resolver: &impl Resolver, Node>) -> i64 { - self.0 - .as_ref() - .map_or(0, |n| resolver.resolve(n).balance_factor()) + pub(super) fn balance_factor( + &self, + resolver: &impl Resolver, Node>, + ) -> Result { + let Some(node) = self.root() else { + return Ok(0); + }; + + let resolved_node = resolver.resolve(node)?; + let balance_factor = resolved_node.balance_factor(); + Ok(balance_factor) } #[inline] @@ -135,27 +150,23 @@ impl Tree { /// - The occupied [`Tree`] with the minimum [`Key`]. /// - The minimum [`Tree`]'s right child, if it hasn't been moved to its new position. /// - True if the [`Tree`] has shrunk in size. - #[must_use] pub(super) fn take_min( &mut self, resolver: &mut impl Resolver, Node>, - ) -> (Tree, Tree, bool) { + ) -> Result<(Tree, Tree, bool), OperationalError> { let Some(node_arc) = self.root_mut() else { - return (None.into(), None.into(), false); + return Ok((None.into(), None.into(), false)); }; - let node_mut = resolver.resolve_mut(node_arc); + let node_mut = resolver.resolve_mut(node_arc)?; // Base case if node_mut.left_ref().root().is_none() { - let mut min = self.take().expect("Already checked"); - let min_mut = resolver.resolve_mut(&mut min); - let right = min_mut.right_mut().take(); - - (Some(min).into(), right.into(), true) + let right = node_mut.right_mut().take(); + Ok((self.take().into(), right.into(), true)) // Recursive } else { - Node::take_min(node_arc, resolver) + Ok(Node::take_min(node_arc, resolver)?) } } @@ -170,7 +181,7 @@ impl Tree { offset: usize, data: impl FnOnce(&mut Value), resolver: &mut impl Resolver, Node>, - ) -> bool { + ) -> Result { let node = self.root_mut(); let Some(node) = node else { // We can't create a new `Node` with a non-zero offset. @@ -185,16 +196,16 @@ impl Tree { // The key does not exist and a new `Node` shall be created. self.0 = Some(Arc::new(Node::new(key.clone(), new_data))); - return true; + return Ok(true); }; let grew = resolver - .resolve_mut(node) - .upsert(key, offset, data, resolver); + .resolve_mut(node)? + .upsert(key, offset, data, resolver)?; if grew { - self.rebalance(resolver); - self.balance_factor(resolver) != 0 + self.rebalance(resolver)?; + Ok(self.balance_factor(resolver)? != 0) } else { - false + Ok(false) } } @@ -208,7 +219,7 @@ impl Tree { offset: usize, data: &[u8], resolver: &mut impl Resolver, Node>, - ) -> bool { + ) -> Result { self.upsert( key, offset, @@ -238,9 +249,13 @@ impl Tree { /// /// The [`Tree`] must already have balance factor in the range of -2..=2, else it is an invalid /// AVL tree. - fn rebalance(&mut self, resolver: &mut impl Resolver, Node>) { - if let Some(node) = self.root_mut() { - Node::rebalance(node, resolver) + fn rebalance( + &mut self, + resolver: &mut impl Resolver, Node>, + ) -> Result<(), OperationalError> { + match self.root_mut() { + Some(node) => Node::rebalance(node, resolver), + None => Ok(()), } } } @@ -276,16 +291,25 @@ impl<'a> Iterator for TreeIterator<'a> { impl Tree { #[inline] /// The data stored in a [`Node`] in the [`Tree`] with a given [`Key`]. - pub fn get(&self, key: &Key, resolver: &impl Resolver, Node>) -> Option<&Value> { - let node = self.root()?; + pub fn get( + &self, + key: &Key, + resolver: &impl Resolver, Node>, + ) -> Result, OperationalError> { + let Some(node) = self.root() else { + return Ok(None); + }; Node::get(node, key, resolver) } /// Asserts that the [`Tree`] is a valid AVL tree - pub(crate) fn check(&self, resolver: &impl Resolver, Node>) { - let inorder = self.is_inorder(resolver); - let is_balanced = self.is_balanced(resolver); - let has_correct_balance_factors = self.has_correct_balance_factors(resolver); + pub(crate) fn check( + &self, + resolver: &impl Resolver, Node>, + ) -> Result<(), OperationalError> { + let inorder = self.is_inorder(resolver)?; + let is_balanced = self.is_balanced(resolver)?; + let has_correct_balance_factors = self.has_correct_balance_factors(resolver)?; if !inorder || !is_balanced || !has_correct_balance_factors { eprintln!("{self:?}"); } @@ -295,10 +319,14 @@ impl Tree { has_correct_balance_factors, "AVL tree balance factors are incorrect" ); + Ok(()) } /// Returns true if the [`Tree`] is in-order. - pub(crate) fn is_inorder(&self, resolver: &impl Resolver, Node>) -> bool { + pub(crate) fn is_inorder( + &self, + resolver: &impl Resolver, Node>, + ) -> Result { self.is_inorder_inner( &Key::new(&[u8::MIN]).expect("Size less than KEY_MAX_SIZE"), &Key::new(&[u8::MAX; KEY_MAX_SIZE]).expect("Size less than KEY_MAX_SIZE"), @@ -310,19 +338,37 @@ impl Tree { pub(super) fn has_correct_balance_factors( &self, resolver: &impl Resolver, Node>, - ) -> bool { - self.root() - .is_none_or(|node| resolver.resolve(node).has_correct_balance_factors(resolver)) + ) -> Result { + match self.root() { + None => Ok(true), + Some(node) => resolver + .resolve(node) + .map(|res| res.has_correct_balance_factors(resolver))?, + } } /// Returns the height of the [`Tree`]. - pub(super) fn height(&self, resolver: &impl Resolver, Node>) -> u32 { - self.root().map_or(0, |node| node.height(resolver)) + pub(super) fn height( + &self, + resolver: &impl Resolver, Node>, + ) -> Result { + match self.root() { + None => Ok(0), + Some(node) => resolver.resolve(node).map(|res| res.height(resolver))?, + } } /// Returns true if the [`Tree`] is balanced. - pub(super) fn is_balanced(&self, resolver: &impl Resolver, Node>) -> bool { - self.root().is_none_or(|node| node.is_balanced(resolver)) + pub(super) fn is_balanced( + &self, + resolver: &impl Resolver, Node>, + ) -> Result { + match self.root() { + None => Ok(true), + Some(node) => resolver + .resolve(node) + .map(|res| res.is_balanced(resolver))?, + } } /// Returns true if the [`Tree`] is in-order and all values lie between the `min` and `max`. @@ -331,9 +377,13 @@ impl Tree { min: &Key, max: &Key, resolver: &impl Resolver, Node>, - ) -> bool { - self.root() - .is_none_or(|node| node.is_inorder(min, max, resolver)) + ) -> Result { + match self.root() { + None => Ok(true), + Some(node) => resolver + .resolve(node) + .map(|res| res.is_inorder(min, max, resolver))?, + } } } @@ -418,7 +468,7 @@ mod tests { for operation in operations { match operation { Operation::Get(key) => { - let tree_value = tree.get(&key, &resolver); + let tree_value = tree.get(&key, &resolver)?; // The values in the Options are comparable, but the Options themselves are // not. So we need to awkwardly match on both Options to compare them. @@ -437,16 +487,16 @@ mod tests { continue; } Operation::Upsert(key, value) => { - tree.set(&key, &value, &mut resolver); + tree.set(&key, &value, &mut resolver)?; reference.insert(key, value); } Operation::Delete(key) => { - tree.delete(&key, &mut resolver); + tree.delete(&key, &mut resolver)?; reference.remove(&key); } } compare_tree_to_reference(&tree, &reference); - tree.check(&resolver); + tree.check(&resolver)?; } } } @@ -470,13 +520,19 @@ mod tests { .iter() .zip(data) .map(|(key, data)| -> Hash { - let digest = tree.hash(&resolver); - tree.set(key, data.as_bytes(), &mut resolver); + let digest = tree + .hash(&resolver) + .expect("Resolving the tree should succeed."); + tree.set(key, data.as_bytes(), &mut resolver) + .expect("Set should succeed"); digest }) .collect(); - digests.push(tree.hash(&resolver)); + digests.push( + tree.hash(&resolver) + .expect("Resolving the tree should succeed."), + ); digests }; diff --git a/durable-storage/src/merkle_layer.rs b/durable-storage/src/merkle_layer.rs index 6b75690d0f7..9d3672d4bd8 100644 --- a/durable-storage/src/merkle_layer.rs +++ b/durable-storage/src/merkle_layer.rs @@ -55,33 +55,37 @@ impl MerkleLayer { // calculated during the encoding of the node // if necessary. for node in self.tree.iter() { - let value = serialisation::serialise(node.to_encode(&self.resolver)) + let encoded = node.to_encode(&self.resolver); + let value = serialisation::serialise(encoded) .expect("Serialisation of node data should not fail"); let blob = HashedData::from_value(value.as_slice()); self.persistence.blob_set(&blob)?; } - Ok(CommitId::from(self.hash())) + Ok(CommitId::from(self.hash()?)) } /// Delete the data associated with a given [Key]. - pub fn delete(&mut self, key: &Key) { - self.tree.delete(key, &mut self.resolver); + pub fn delete(&mut self, key: &Key) -> Result<(), OperationalError> { + self.tree.delete(key, &mut self.resolver)?; + Ok(()) } /// Returns the root hash, potentially re-hashing uncached nodes. - pub fn hash(&mut self) -> Hash { + pub fn hash(&mut self) -> Result { self.tree.hash(&self.resolver) } /// Sets the data associated with a given [Key]. - pub fn set(&mut self, key: &Key, data: &[u8]) { - self.tree.set(key, data, &mut self.resolver); + pub fn set(&mut self, key: &Key, data: &[u8]) -> Result<(), OperationalError> { + self.tree.set(key, data, &mut self.resolver)?; + Ok(()) } /// Writes the data to the node associated with a given [Key] with the given offset. - pub fn write(&mut self, key: &Key, offset: usize, data: &[u8]) { - self.tree.write(key, offset, data, &mut self.resolver); + pub fn write(&mut self, key: &Key, offset: usize, data: &[u8]) -> Result<(), OperationalError> { + self.tree.write(key, offset, data, &mut self.resolver)?; + Ok(()) } } @@ -97,6 +101,7 @@ mod tests { use crate::avl::node::Node; use crate::avl::node::Value; use crate::avl::tree::Tree; + use crate::errors::OperationalError; use crate::key::Key; use crate::persistence_layer::PersistenceLayer; use crate::repo::DirectoryManager; @@ -112,7 +117,7 @@ mod tests { } /// Returns an immutable reference to the data stored for a given [Key]. - fn get(&mut self, key: &Key) -> Option<&Value> { + pub fn get(&mut self, key: &Key) -> Result, OperationalError> { self.tree.get(key, &self.resolver) } } @@ -140,18 +145,31 @@ mod tests { let mut ml = new_merkle_layer(); for i in 0..keys.len() { - ml.set(&keys[i], &data[i]); - ml.tree().check(&ml.resolver); + ml.set(&keys[i], &data[i]) + .expect("setting node should succeed"); + ml.tree() + .check(&ml.resolver) + .expect("the tree should be retrieved successfully."); } let mut ml2 = ml.clone(); - let original_hash = ml.hash(); - assert_eq!(original_hash, ml2.hash()); + let original_hash = ml.hash().expect("hash operation should succeed."); + assert_eq!( + original_hash, + ml2.hash().expect("hash operation should succeed.") + ); let cow_data = "🐮<(moo!)"; - ml2.set(&keys[0], cow_data.as_bytes()); - assert_ne!(original_hash, ml2.hash()); - assert_eq!(original_hash, ml.hash()); + ml2.set(&keys[0], cow_data.as_bytes()) + .expect("setting node should succeed"); + assert_ne!( + original_hash, + ml2.hash().expect("hash operation should succeed.") + ); + assert_eq!( + original_hash, + ml.hash().expect("hash operation should succeed.") + ); let old_node1 = Node::new(keys[0].clone(), Bytes::copy_from_slice(&data[0])); let new_node1 = Node::new(keys[0].clone(), cow_data.as_bytes()); @@ -163,37 +181,47 @@ mod tests { &old_node1.data(), &ml.get(&keys[0]) .expect("The node should be retrieved successfully. Merkle layer: {ml:?}") + .expect("The data should exist.") ); assert_eq!( &new_node1.data(), &ml2.get(&keys[0]) .expect("The node should be retrieved successfully. Merkle layer: {ml2:?}") + .expect("The data should exist.") ); assert_eq!( &node2.data(), &ml.get(&keys[1]) .expect("The node should be retrieved successfully. Merkle layer: {ml:?}") + .expect("The data should exist.") ); assert_eq!( &node2.data(), &ml2.get(&keys[1]) .expect("The node should be retrieved successfully. Merkle layer: {ml2:?}") + .expect("The data should exist.") ); assert_eq!( &node3.data(), &ml.get(&keys[2]) .expect("The node should be retrieved successfully. Merkle layer: {ml:?}") + .expect("The data should exist.") ); assert_eq!( &node3.data(), &ml2.get(&keys[2]) .expect("The node should be retrieved successfully. Merkle layer: {ml2:?}") + .expect("The data should exist.") ); - ml.tree().check(&ml.resolver); - ml2.tree().check(&ml.resolver); + ml.tree() + .check(&ml.resolver) + .expect("the tree should be retrieved successfully."); + ml2.tree() + .check(&ml.resolver) + .expect("the tree should be retrieved successfully."); } proptest! { @@ -206,46 +234,46 @@ mod tests { // Set all the keys in the tree for bytes in &keys1 { let key = Key::new(bytes).expect("Sizes less than KEY_MAX_SIZE"); - ml.set(&key, &data1); + ml.set(&key, &data1).expect("setting node should succeed"); } // Create a cheap copy - let original_hash = ml.hash(); + let original_hash = ml.hash().expect("hash operation should succeed."); let mut ml2 = ml.clone(); - prop_assert_eq!(original_hash, ml2.hash()); + prop_assert_eq!(original_hash, ml2.hash().expect("hash operation should succeed.")); // Delete all the keys in the copy for bytes in &keys1 { let key = Key::new(bytes).expect("Sizes less than KEY_MAX_SIZE"); - ml2.delete(&key); - prop_assert_eq!(ml2.get(&key), None); + ml2.delete(&key).expect("deleting node should succeed."); + prop_assert_eq!(ml2.get(&key).expect(""), None); } // Set new keys in the copy for bytes in &keys2 { let key = Key::new(bytes).expect("Sizes less than KEY_MAX_SIZE"); - ml2.set(&key, &data2); + ml2.set(&key, &data2).expect("setting node should succeed"); } if keys1.is_empty() && keys2.is_empty() { - prop_assert_eq!(original_hash, ml2.hash()); + prop_assert_eq!(original_hash, ml2.hash().expect("hash operation should succeed.")); } else { - prop_assert_ne!(original_hash, ml2.hash()); + prop_assert_ne!(original_hash, ml2.hash().expect("hash operation should succeed.")); } - prop_assert_eq!(original_hash, ml.hash()); + prop_assert_eq!(original_hash, ml.hash().expect("hash operation should succeed.")); // Check both trees are still correct for bytes in &keys1 { let key = Key::new(bytes).expect("Sizes less than KEY_MAX_SIZE"); - prop_assert_eq!(ml.get(&key).expect("The node should be retrieved successfully"), &data1); + prop_assert_eq!(ml.get(&key).expect("The node should be retrieved successfully").expect("The data should exist."), &data1); } for bytes in &keys2 { let key = Key::new(bytes).expect("Sizes less than KEY_MAX_SIZE"); - prop_assert_eq!(ml2.get(&key).expect("The node should be retrieved successfully"), &data2); + prop_assert_eq!(ml2.get(&key).expect("The node should be retrieved successfully").expect("The data should exist."), &data2); } - ml.tree().check(&ml.resolver); - ml2.tree().check(&ml2.resolver); + ml.tree().check(&ml.resolver).expect("the tree should be retrieved successfully."); + ml2.tree().check(&ml2.resolver).expect("the tree should be retrieved successfully."); } } @@ -254,17 +282,23 @@ mod tests { let key = Key::new(&[1]).expect("Size less than KEY_MAX_SIZE"); let data = Bytes::from("create"); let mut ml = new_merkle_layer(); - let empty_hash = ml.hash(); - ml.set(&key, &data); - assert_ne!(empty_hash, ml.hash()); + let empty_hash = ml.hash().expect("hash operation should succeed."); + ml.set(&key, &data).expect("setting node should succeed"); + assert_ne!( + empty_hash, + ml.hash().expect("hash operation should succeed.") + ); let node = Node::new(key.clone(), data); let get_node = ml .get(&key) - .expect("The node should be retrieved successfully"); + .expect("The node should be retrieved successfully") + .expect("The data should exist."); assert_eq!(&get_node, &node.data()); - ml.tree().check(&ml.resolver); + ml.tree() + .check(&ml.resolver) + .expect("the tree should be retrieved successfully."); } #[test] @@ -273,29 +307,35 @@ mod tests { let data = Bytes::from("old"); let data2 = Bytes::from("new"); let mut ml = new_merkle_layer(); - ml.set(&key, &data); - let old_hash = ml.hash(); + ml.set(&key, &data).expect("setting node should succeed"); + let old_hash = ml.hash().expect("hash operation should succeed."); let node = Node::new(key.clone(), data); let get_node = ml .get(&key) - .expect("The node should be retrieved successfully"); + .expect("The node should be retrieved successfully") + .expect("The data should exist."); assert_eq!(&get_node, &node.data()); - ml.set(&key, &data2); - assert_ne!(old_hash, ml.hash()); + ml.set(&key, &data2).expect("setting node should succeed"); + assert_ne!(old_hash, ml.hash().expect("hash operation should succeed.")); assert!( - ml.tree.is_inorder(&ml.resolver), + ml.tree + .is_inorder(&ml.resolver) + .expect("The tree should be retrieved successfully."), "AVL isn't in order: {ml:?}" ); let node = Node::new(key.clone(), data2); let get_node = ml .get(&key) - .expect("The node should be retrieved successfully"); + .expect("The node should be retrieved successfully") + .expect("The data should exist."); assert_eq!(&get_node, &node.data()); - ml.tree().check(&ml.resolver); + ml.tree() + .check(&ml.resolver) + .expect("the tree should be retrieved successfully."); } #[test] @@ -318,13 +358,16 @@ mod tests { let mut ml = new_merkle_layer(); for (key, data) in keys.iter().zip(data.iter()) { - let old_hash = ml.hash(); - ml.set(key, data); - assert_ne!(old_hash, ml.hash()); - ml.tree().check(&ml.resolver); + let old_hash = ml.hash().expect("hash operation should succeed."); + ml.set(key, data).expect("setting node should succeed"); + assert_ne!(old_hash, ml.hash().expect("hash operation should succeed.")); + ml.tree() + .check(&ml.resolver) + .expect("the tree should be retrieved successfully."); assert_eq!( ml.get(key) - .expect("The node should be retrieved successfully"), + .expect("The node should be retrieved successfully") + .expect("The data should exist."), data ); } @@ -344,19 +387,24 @@ mod tests { .map(|r| r.expect("Sizes less than KEY_MAX_SIZE")); let mut ml = new_merkle_layer(); - let empty_hash = ml.hash(); + let empty_hash = ml.hash().expect("hash operation should succeed."); // Left imbalance let data = Bytes::from("imbalanced left"); for key in keys.iter() { - let old_hash = ml.hash(); - ml.set(key, &data); - assert_ne!(old_hash, ml.hash()); - ml.tree().check(&ml.resolver); + let old_hash = ml.hash().expect("hash operation should succeed."); + ml.set(key, &data).expect("setting node should succeed"); + assert_ne!(old_hash, ml.hash().expect("hash operation should succeed.")); + ml.tree() + .check(&ml.resolver) + .expect("the tree should be retrieved successfully."); } ml.clear(); - assert_eq!(empty_hash, ml.hash()); + assert_eq!( + empty_hash, + ml.hash().expect("hash operation should succeed.") + ); let keys = { let mut keys = keys; @@ -367,10 +415,12 @@ mod tests { // Right imbalance let data = Bytes::from("imbalanced right"); for key in keys.iter() { - let old_hash = ml.hash(); - ml.set(key, &data); - assert_ne!(old_hash, ml.hash()); - ml.tree().check(&ml.resolver); + let old_hash = ml.hash().expect("hash operation should succeed."); + ml.set(key, &data).expect("setting node should succeed"); + assert_ne!(old_hash, ml.hash().expect("hash operation should succeed.")); + ml.tree() + .check(&ml.resolver) + .expect("the tree should be retrieved successfully."); } } @@ -384,13 +434,16 @@ mod tests { let mut ml = new_merkle_layer(); for key in keys.iter() { - let old_hash = ml.hash(); - ml.set(key, &data); - assert_ne!(old_hash, ml.hash()); - ml.tree().check(&ml.resolver); + let old_hash = ml.hash().expect("hash operation should succeed."); + ml.set(key, &data).expect("setting node should succeed"); + assert_ne!(old_hash, ml.hash().expect("hash operation should succeed.")); + ml.tree() + .check(&ml.resolver) + .expect("the tree should be retrieved successfully."); assert_eq!( ml.get(key) - .expect("The node should be retrieved successfully"), + .expect("The node should be retrieved successfully") + .expect("The data should exist."), &data ); } @@ -406,13 +459,16 @@ mod tests { let mut ml = new_merkle_layer(); for key in keys.iter() { - let old_hash = ml.hash(); - ml.set(key, &data); - assert_ne!(old_hash, ml.hash()); - ml.tree().check(&ml.resolver); + let old_hash = ml.hash().expect("hash operation should succeed."); + ml.set(key, &data).expect("setting node should succeed"); + assert_ne!(old_hash, ml.hash().expect("hash operation should succeed.")); + ml.tree() + .check(&ml.resolver) + .expect("the tree should be retrieved successfully."); assert_eq!( ml.get(key) - .expect("The node should be retrieved successfully"), + .expect("The node should be retrieved successfully") + .expect("The data should exist."), &data ); } @@ -439,13 +495,16 @@ mod tests { let mut ml = new_merkle_layer(); for key in keys.iter() { - let old_hash = ml.hash(); - ml.set(key, &data); - assert_ne!(old_hash, ml.hash()); - ml.tree().check(&ml.resolver); + let old_hash = ml.hash().expect("hash operation should succeed."); + ml.set(key, &data).expect("setting node should succeed"); + assert_ne!(old_hash, ml.hash().expect("hash operation should succeed.")); + ml.tree() + .check(&ml.resolver) + .expect("the tree should be retrieved successfully."); assert_eq!( ml.get(key) - .expect("The node should be retrieved successfully"), + .expect("The node should be retrieved successfully") + .expect("The data should exist."), &data ); } @@ -472,13 +531,16 @@ mod tests { let mut ml = new_merkle_layer(); for key in keys.iter() { - let old_hash = ml.hash(); - ml.set(key, &data); - assert_ne!(old_hash, ml.hash()); - ml.tree().check(&ml.resolver); + let old_hash = ml.hash().expect("hash operation should succeed."); + ml.set(key, &data).expect("setting node should succeed"); + assert_ne!(old_hash, ml.hash().expect("hash operation should succeed.")); + ml.tree() + .check(&ml.resolver) + .expect("the tree should be retrieved successfully."); assert_eq!( ml.get(key) - .expect("The node should be retrieved successfully"), + .expect("The node should be retrieved successfully") + .expect("The data should exist."), &data ); } @@ -489,23 +551,23 @@ mod tests { fn test_mavl_create_prop(keys in prop::collection::vec(any::<[u8; 2]>(), 0..500)) { let data = Bytes::from("property"); let mut ml = new_merkle_layer(); - let old_hash = ml.hash(); + let old_hash = ml.hash().expect("hash operation should succeed."); for bytes in &keys { let key = Key::new(bytes).expect("Sizes less than KEY_MAX_SIZE"); - ml.set(&key, &data); + ml.set(&key, &data).expect("setting node should succeed"); } if !keys.is_empty() { - assert_ne!(old_hash, ml.hash()); + assert_ne!(old_hash, ml.hash().expect("hash operation should succeed.")); } for bytes in &keys { let key = Key::new(bytes).expect("Sizes less than KEY_MAX_SIZE"); - prop_assert_eq!(ml.get(&key).expect("The node should be retrieved successfully"), &data); + prop_assert_eq!(ml.get(&key).expect("The node should be retrieved successfully").expect("The data should exist."), &data); } - ml.tree().check(&ml.resolver); + ml.tree().check(&ml.resolver).expect("the tree should be retrieved successfully."); } } @@ -514,19 +576,29 @@ mod tests { let key = Key::new(&[1]).expect("Sizes less than KEY_MAX_SIZE"); let data = Bytes::from("delete"); let mut ml = new_merkle_layer(); - let empty_hash = ml.hash(); - ml.set(&key, &data); - let full_hash = ml.hash(); + let empty_hash = ml.hash().expect("hash operation should succeed."); + ml.set(&key, &data).expect("setting node should succeed"); + let full_hash = ml.hash().expect("hash operation should succeed."); assert_ne!(empty_hash, full_hash); - ml.delete(&key); - assert_ne!(full_hash, ml.hash()); - assert_eq!(empty_hash, ml.hash()); + ml.delete(&key).expect("deleting node should succeed."); + assert_ne!( + full_hash, + ml.hash().expect("hash operation should succeed.") + ); + assert_eq!( + empty_hash, + ml.hash().expect("hash operation should succeed.") + ); - let get_node = ml.get(&key); + let get_node = ml + .get(&key) + .expect("The node should be retrieved successfully."); assert_eq!(get_node, None); - ml.tree().check(&ml.resolver); + ml.tree() + .check(&ml.resolver) + .expect("the tree should be retrieved successfully."); } proptest! { @@ -534,26 +606,26 @@ mod tests { fn test_mavl_delete_prop(keys in prop::collection::vec(any::<[u8; 2]>(), 0..500)) { let data = Bytes::from("delete_prop"); let mut ml = new_merkle_layer(); - let empty_hash = ml.hash(); + let empty_hash = ml.hash().expect("hash operation should succeed."); for bytes in &keys { let key = Key::new(bytes).expect("Sizes less than KEY_MAX_SIZE"); - ml.set(&key, &data); + ml.set(&key, &data).expect("setting node should succeed"); } if !keys.is_empty() { - prop_assert_ne!(empty_hash, ml.hash()); + prop_assert_ne!(empty_hash, ml.hash().expect("hash operation should succeed.")); } for bytes in &keys { let key = Key::new(bytes).expect("Sizes less than KEY_MAX_SIZE"); - ml.delete(&key); - prop_assert_eq!(ml.get(&key), None); + ml.delete(&key).expect("delete should succeed."); + prop_assert_eq!(ml.get(&key).expect("The node should be retrieved successfully."), None); } - prop_assert_eq!(empty_hash, ml.hash()); + prop_assert_eq!(empty_hash, ml.hash().expect("hash operation should succeed.")); - ml.tree().check(&ml.resolver); + ml.tree().check(&ml.resolver).expect("the tree should be retrieved successfully."); } } @@ -561,30 +633,41 @@ mod tests { let data = Bytes::from("delete"); let mut ml = new_merkle_layer(); - let empty_hash = ml.hash(); + let empty_hash = ml.hash().expect("hash operation should succeed."); for key in keys.iter() { - ml.set(key, &data); - ml.tree().check(&ml.resolver); + ml.set(key, &data).expect("setting node should succeed"); + ml.tree() + .check(&ml.resolver) + .expect("the tree should be retrieved successfully."); assert_eq!( ml.get(key) - .expect("The node should be retrieved successfully"), + .expect("The node should be retrieved successfully") + .expect("The data should exist."), &data, ); } if !keys.is_empty() { - assert_ne!(empty_hash, ml.hash()); + assert_ne!( + empty_hash, + ml.hash().expect("hash operation should succeed.") + ); } for key in keys.iter() { - ml.delete(key); - ml.tree().check(&ml.resolver); - ml.delete(key); - assert_eq!(ml.get(key), None); + ml.delete(key).expect("deleting node should succeed."); + ml.tree() + .check(&ml.resolver) + .expect("the tree should be retrieved successfully."); + ml.delete(key).expect("deleting node should succeed."); + assert_eq!(ml.get(key).expect("The data should exist."), None); } - assert_eq!(empty_hash, ml.hash()); + assert_eq!( + empty_hash, + ml.hash().expect("hash operation should succeed.") + ); } // Requires replacing a node with its successor while rebalancing a node on the return path. @@ -726,17 +809,20 @@ mod tests { let key = Key::new(&[1]).expect("Size less than KEY_MAX_SIZE"); let data = Bytes::from("write_new_value"); let mut ml = new_merkle_layer(); - let old_hash = ml.hash(); - ml.write(&key, 0, &data); + let old_hash = ml.hash().expect("hash operation should succeed."); + ml.write(&key, 0, &data).expect("write should succeed."); let node = Node::new(key.clone(), data); let get_node = ml .get(&key) - .expect("The node should be retrieved successfully"); + .expect("The node should be retrieved successfully") + .expect("The data should exist."); assert_eq!(&get_node, &node.data()); - assert_ne!(old_hash, ml.hash()); - ml.tree().check(&ml.resolver); + assert_ne!(old_hash, ml.hash().expect("hash operation should succeed.")); + ml.tree() + .check(&ml.resolver) + .expect("The tree should be retrieved successfully."); } #[test] @@ -745,31 +831,37 @@ mod tests { let data = Bytes::from("a long value"); let data2 = Bytes::from("good"); let mut ml = new_merkle_layer(); - ml.set(&key, &data); - let old_hash = ml.hash(); + ml.set(&key, &data).expect("setting node should succeed"); + let old_hash = ml.hash().expect("hash operation should succeed."); let data_len = data.len(); let node = Node::new(key.clone(), data); let get_node = ml .get(&key) - .expect("The node should be retrieved successfully"); + .expect("The node should be retrieved successfully") + .expect("The data should exist."); assert_eq!(&get_node, &node.data()); - ml.write(&key, 2, &data2); - assert_ne!(old_hash, ml.hash()); + ml.write(&key, 2, &data2).expect("write should succeed."); + assert_ne!(old_hash, ml.hash().expect("hash operation should succeed.")); assert!( - ml.tree.is_inorder(&ml.resolver), + ml.tree + .is_inorder(&ml.resolver) + .expect("The tree should be retrieved successfully."), "AVL isn't in order: {ml:?}" ); let node = Node::new(key.clone(), Bytes::from("a good value")); let get_node = ml .get(&key) - .expect("The node should be retrieved successfully"); + .expect("The node should be retrieved successfully") + .expect("The data should exist."); assert_eq!(get_node.len(), data_len); assert_eq!(&get_node, &node.data()); - ml.tree().check(&ml.resolver); + ml.tree() + .check(&ml.resolver) + .expect("the tree should be retrieved successfully."); } proptest! { @@ -784,26 +876,29 @@ mod tests { .collect::>()); let mut ml = new_merkle_layer(); - let old_hash = ml.hash(); + let old_hash = ml.hash().expect("hash operation should succeed."); for bytes in &keys { let key = Key::new(bytes).expect("Sizes less than KEY_MAX_SIZE"); - ml.set(&key, &data); + ml.set(&key, &data).expect("setting node should succeed"); for offset in 0..250 { - ml.write(&key, offset * 2, &[1]); + ml.write(&key, offset * 2, &[1]).expect("write should succeed."); } } if !keys.is_empty() { - assert_ne!(old_hash, ml.hash()); + assert_ne!(old_hash, ml.hash().expect("hash operation should succeed.")); } for bytes in &keys { let key = Key::new(bytes).expect("Sizes less than KEY_MAX_SIZE"); - prop_assert_eq!(ml.get(&key).expect("The node should be retrieved successfully"), &alternating); + prop_assert_eq!(ml.get(&key) + .expect("The node should be retrieved successfully") + .expect("The data should exist."), + &alternating); } - ml.tree().check(&ml.resolver); + ml.tree().check(&ml.resolver).expect("the tree should be retrieved successfully."); } } @@ -837,7 +932,9 @@ mod tests { ]; for (key, data_elem) in keys.iter().zip(data.iter()) { - merkle_layer.set(key, data_elem); + merkle_layer + .set(key, data_elem) + .expect("setting node should succeed"); } let commit_id = merkle_layer @@ -845,10 +942,10 @@ mod tests { .expect("The commit operation should not fail"); for node in merkle_layer.tree.iter() { - let serialised = - octez_riscv_data::serialisation::serialise(node.to_encode(&merkle_layer.resolver)) - .expect("We should be able to serialise the node"); - let node_hash = crate::avl::node::hash(node, &merkle_layer.resolver); + let encoded = node.to_encode(&merkle_layer.resolver); + let serialised = octez_riscv_data::serialisation::serialise(encoded) + .expect("We should be able to serialise the node"); + let node_hash = node.hash(&merkle_layer.resolver); let blob = merkle_layer .persistence .blob_get(node_hash) @@ -856,7 +953,10 @@ mod tests { assert_eq!(serialised, blob.as_ref()); } - let root_hash = merkle_layer.tree.hash(&merkle_layer.resolver); + let root_hash = merkle_layer + .tree + .hash(&merkle_layer.resolver) + .expect("Resolving the node should succeed."); assert_eq!(*commit_id.as_hash(), root_hash); } } diff --git a/durable-storage/src/merkle_worker.rs b/durable-storage/src/merkle_worker.rs index 84e4fc4fa34..1f2e9229505 100644 --- a/durable-storage/src/merkle_worker.rs +++ b/durable-storage/src/merkle_worker.rs @@ -128,14 +128,26 @@ impl MerkleWorker { while let Some(cmd) = receiver.recv().await { match cmd { - Command::Write { key, offset, value } => layer.write(&key, offset, &value), + Command::Write { key, offset, value } => { + layer + .write(&key, offset, &value) + .expect("Writing to the Merkle layer should succeed."); + } - Command::Set { key, value } => layer.set(&key, &value), + Command::Set { key, value } => { + layer + .set(&key, &value) + .expect("Set on the Merkle layer should succeed."); + } - Command::Delete { key } => layer.delete(&key), + Command::Delete { key } => { + layer + .delete(&key) + .expect("Delete on the Merkle layer should succeed."); + } Command::Hash { response } => { - let hash = layer.hash(); + let hash = layer.hash().expect("Resolving the tree should succeed."); let _ = response.send(hash); } @@ -269,23 +281,27 @@ mod tests { ) { match self { Self::Write { key, offset, value } => { - layer.write(&key, offset, &value); + layer + .write(&key, offset, &value) + .expect("Write should succeed."); worker.write(key, offset, value).unwrap(); } Self::Set { key, value } => { - layer.set(&key, &value); + layer.set(&key, &value).expect("Set should succeed."); worker.set(key, value).unwrap(); } Self::Delete { key } => { - layer.delete(&key); + layer.delete(&key).expect("Delete should succeed."); worker.delete(key).unwrap(); } Self::Hash => { let hash1 = worker.hash().unwrap(); - let hash2 = layer.hash(); + let hash2 = layer + .hash() + .expect("Resolving the tree for the hash should succeed."); assert_eq!(hash1, hash2); } @@ -370,7 +386,7 @@ mod tests { command.run(handle, &dir_manager, &mut merkle_worker, &mut merkle_layer); } - let layer_hash = merkle_layer.hash(); + let layer_hash = merkle_layer.hash().expect("Resolving the tree should succeed."); let worker_hash = merkle_worker.hash().unwrap(); prop_assert_eq!(layer_hash, worker_hash); });