Deny clippy::arithmetic_side_effects for fuel-merkle

CHANGELOG.md

@@ -10,13 +10,15 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 ### Changed
 
 - [#725](https://github.com/FuelLabs/fuel-vm/pull/725): Adds more clippy lints to catch possible integer overflow and casting bugs on compile time.
+- [#729](https://github.com/FuelLabs/fuel-vm/pull/729): Adds more clippy lints to `fuel-merkle` to catch possible integer overflow and casting bugs on compile time. It also does some internal refactoring.
 
 ### Added
 
 #### Breaking
 
 - [#725](https://github.com/FuelLabs/fuel-vm/pull/725): `UtxoId::from_str` now rejects inputs with multiple `0x` prefixes. Many `::from_str` implementations also reject extra data in the end of the input, instead of silently ignoring it. `UtxoId::from_str` allows a single `:` between the fields. Unused `GasUnit` struct removed.
 - [#726](https://github.com/FuelLabs/fuel-vm/pull/726): Removed code related to Binary Merkle Sum Trees (BMSTs). The BMST is deprecated and not used in production environments. 
+- [#729](https://github.com/FuelLabs/fuel-vm/pull/729): Removed default implementation of `Node::key_size_bits`, implementors must now define it themselves. Also some helper traits have been merged together, or their types changed.
 
 ## [Version 0.49.0]
 

fuel-merkle/src/binary/merkle_tree.rs

@@ -24,7 +24,7 @@ use crate::{
 use alloc::vec::Vec;
 use core::marker::PhantomData;
 
-#[derive(Debug, Clone, derive_more::Display)]
+#[derive(Debug, Clone, derive_more::Display, PartialEq, Eq)]
 pub enum MerkleTreeError<StorageError> {
     #[display(fmt = "proof index {_0} is not valid")]
     InvalidProofIndex(u64),
@@ -34,6 +34,9 @@ pub enum MerkleTreeError<StorageError> {
 
     #[display(fmt = "{}", _0)]
     StorageError(StorageError),
+
+    #[display(fmt = "the tree is too large and contains {_0} leaves")]
+    TooLargeTree(u64),
 }
 
 impl<StorageError> From<StorageError> for MerkleTreeError<StorageError> {
@@ -96,31 +99,34 @@ impl<TableType, StorageType> MerkleTree<TableType, StorageType> {
             .map(|head| build_root_node(head, scratch_storage))
     }
 
-    fn peak_positions(&self) -> Vec<Position> {
-        // Define a new tree with a leaf count 1 greater than the current leaf
-        // count.
-        let leaves_count = self.leaves_count + 1;
-
-        // The rightmost leaf position of a tree will always have a leaf index
-        // N - 1, where N is the number of leaves.
-        let leaf_position = Position::from_leaf_index(leaves_count - 1);
+    fn peak_positions<StorageError>(
+        &self,
+    ) -> Result<Vec<Position>, MerkleTreeError<StorageError>> {
+        let leaf_position = Position::from_leaf_index(self.leaves_count)
+            .ok_or(MerkleTreeError::TooLargeTree(self.leaves_count))?;
         let root_position = self.root_position();
-        let mut peaks_itr = root_position.path(&leaf_position, leaves_count).iter();
+        // u64 cannot overflow, as memory is finite
+        #[allow(clippy::arithmetic_side_effects)]
+        let next_leaves_count = self.leaves_count + 1;
+        let mut peaks_itr = root_position.path(&leaf_position, next_leaves_count).iter();
         peaks_itr.next(); // Omit the root
 
         let (_, peaks): (Vec<_>, Vec<_>) = peaks_itr.unzip();
 
-        peaks
+        Ok(peaks)
     }
 
     fn root_position(&self) -> Position {
         // Define a new tree with a leaf count 1 greater than the current leaf
         // count.
+        // u64 cannot overflow, as memory is finite
+        #[allow(clippy::arithmetic_side_effects)]
         let leaves_count = self.leaves_count + 1;
 
         // The root position of a tree will always have an in-order index equal
         // to N' - 1, where N is the leaves count and N' is N rounded (or equal)
         // to the next power of 2.
+        #[allow(clippy::arithmetic_side_effects)] // next_power_of_two > 0
         let root_index = leaves_count.next_power_of_two() - 1;
         Position::from_in_order_index(root_index)
     }
@@ -160,14 +166,15 @@ where
         &self,
         proof_index: u64,
     ) -> Result<(Bytes32, ProofSet), MerkleTreeError<StorageError>> {
-        if proof_index + 1 > self.leaves_count {
+        if proof_index >= self.leaves_count {
             return Err(MerkleTreeError::InvalidProofIndex(proof_index))
         }
 
         let mut proof_set = ProofSet::new();
 
         let root_position = self.root_position();
-        let leaf_position = Position::from_leaf_index(proof_index);
+        let leaf_position = Position::from_leaf_index(proof_index)
+            .ok_or(MerkleTreeError::TooLargeTree(proof_index))?;
         let (_, mut side_positions): (Vec<_>, Vec<_>) = root_position
             .path(&leaf_position, self.leaves_count)
             .iter()
@@ -289,7 +296,7 @@ where
     /// side positions `03`, `09`, and `12`, matching our set of MMR peaks.
     fn build(&mut self) -> Result<(), MerkleTreeError<StorageError>> {
         let mut current_head = None;
-        let peaks = &self.peak_positions();
+        let peaks = &self.peak_positions()?;
         for peak in peaks.iter() {
             let key = peak.in_order_index();
             let node = self
@@ -313,23 +320,30 @@ where
     TableType: Mappable<Key = u64, Value = Primitive, OwnedValue = Primitive>,
     StorageType: StorageMutate<TableType, Error = StorageError>,
 {
-    pub fn push(&mut self, data: &[u8]) -> Result<(), StorageError> {
-        let node = Node::create_leaf(self.leaves_count, data);
-        self.storage.insert(&node.key(), &node.as_ref().into())?;
+    pub fn push(&mut self, data: &[u8]) -> Result<(), TreeExtendError<StorageError>> {
+        let node = Node::create_leaf(self.leaves_count, data)
+            .ok_or(TreeExtendError::TooLarge)?;
+        self.storage
+            .insert(&node.key(), &node.as_ref().into())
+            .map_err(TreeExtendError::Storage)?;
         let next = self.head.take();
         let head = Subtree::new(node, next);
         self.head = Some(head);
         self.join_all_subtrees()?;
 
-        self.leaves_count += 1;
+        // u64 cannot overflow, as memory is finite
+        #[allow(clippy::arithmetic_side_effects)]
+        {
+            self.leaves_count += 1;
+        }
 
         Ok(())
     }
 
     // PRIVATE
     //
 
-    fn join_all_subtrees(&mut self) -> Result<(), StorageError> {
+    fn join_all_subtrees(&mut self) -> Result<(), TreeExtendError<StorageError>> {
         while {
             // Iterate through all subtrees in the tree to see which subtrees
             // can be merged. Two consecutive subtrees will be merged if, and
@@ -348,19 +362,25 @@ where
             // Merge the two front heads of the list into a single head
             let mut head = self.head.take().expect("Expected head to be present");
             let mut head_next = head.take_next().expect("Expected next to be present");
-            let joined_head = join_subtrees(&mut head_next, &mut head);
+            let joined_head = join_subtrees(&mut head_next, &mut head)
+                .ok_or(TreeExtendError::TooLarge)?;
             self.storage
-                .insert(&joined_head.node().key(), &joined_head.node().into())?;
+                .insert(&joined_head.node().key(), &joined_head.node().into())
+                .map_err(TreeExtendError::Storage)?;
             self.head = Some(joined_head);
         }
 
         Ok(())
     }
 }
 
-fn join_subtrees(lhs: &mut Subtree<Node>, rhs: &mut Subtree<Node>) -> Subtree<Node> {
-    let joined_node = Node::create_node(lhs.node(), rhs.node());
-    Subtree::new(joined_node, lhs.take_next())
+/// Returns `None` if the new node cannot be created.
+fn join_subtrees(
+    lhs: &mut Subtree<Node>,
+    rhs: &mut Subtree<Node>,
+) -> Option<Subtree<Node>> {
+    let joined_node = Node::create_node(lhs.node(), rhs.node())?;
+    Some(Subtree::new(joined_node, lhs.take_next()))
 }
 
 fn build_root_node<Table, Storage>(subtree: &Subtree<Node>, storage: &mut Storage) -> Node
@@ -370,12 +390,21 @@ where
 {
     let mut head = subtree.clone();
     while let Some(mut head_next) = head.take_next() {
-        head = join_subtrees(&mut head_next, &mut head);
+        head = join_subtrees(&mut head_next, &mut head).expect("Failed to join subtrees");
         storage.insert(&head.node().key(), &head.node().into());
     }
     head.node().clone()
 }
 
+/// Error when attempting to extend a tree.
+#[derive(Debug, Clone, PartialEq)]
+pub enum TreeExtendError<StorageError> {
+    /// Max tree size exceeded
+    TooLarge,
+    /// Storage write failed
+    Storage(StorageError),
+}
+
 #[cfg(test)]
 mod test {
     use super::{
@@ -386,6 +415,7 @@ mod test {
         binary::{
             empty_sum,
             leaf_sum,
+            merkle_tree::TreeExtendError,
             node_sum,
             Node,
             Primitive,
@@ -396,8 +426,10 @@ mod test {
     use fuel_storage::{
         Mappable,
         StorageInspect,
+        StorageMutate,
     };
 
+    use crate::binary::MerkleTreeError::TooLargeTree;
     use alloc::vec::Vec;
 
     #[derive(Debug)]
@@ -891,4 +923,42 @@ mod test {
         let expected_root = node_3;
         assert_eq!(root, expected_root);
     }
+
+    #[test]
+    fn load_overflows() {
+        // Given
+        let storage_map = StorageMap::<TestTable>::new();
+        const LEAVES_COUNT: u64 = u64::MAX;
+
+        // When
+        let result = MerkleTree::load(storage_map, LEAVES_COUNT).map(|_| ());
+
+        // Then
+        assert_eq!(result, Err(TooLargeTree(LEAVES_COUNT)));
+    }
+
+    #[test]
+    fn push_overflows() {
+        // Given
+        let mut storage_map = StorageMap::<TestTable>::new();
+        const LEAVES_COUNT: u64 = u64::MAX / 2;
+        loop {
+            let result = MerkleTree::load(&mut storage_map, LEAVES_COUNT).map(|_| ());
+
+            if let Err(MerkleTreeError::LoadError(index)) = result {
+                storage_map.insert(&index, &Primitive::default()).unwrap();
+            } else {
+                break;
+            }
+        }
+
+        // When
+        let mut tree = MerkleTree::load(storage_map, LEAVES_COUNT)
+            .expect("Expected `load()` to succeed");
+        let _ = tree.push(&[]);
+        let result = tree.push(&[]);
+
+        // Then
+        assert_eq!(result, Err(TreeExtendError::TooLarge));
+    }
 }

fuel-merkle/src/binary/node.rs

@@ -22,20 +22,23 @@ impl Node {
         Self { position, hash }
     }
 
-    pub fn create_leaf(index: u64, data: &[u8]) -> Self {
-        let position = Position::from_leaf_index(index);
+    /// Returns `None` if the leaf cannot be created due to incorrect position.
+    pub fn create_leaf(index: u64, data: &[u8]) -> Option<Self> {
+        let position = Position::from_leaf_index(index)?;
         let hash = leaf_sum(data);
-        Self { position, hash }
+        Some(Self { position, hash })
     }
 
-    pub fn create_node(left_child: &Self, right_child: &Self) -> Self {
-        let position = left_child.position().parent();
+    /// Creates a new node under the parent of the left_child.
+    /// Returns `None` if the leaf cannot be created due to incorrect position.
+    pub fn create_node(left_child: &Self, right_child: &Self) -> Option<Self> {
+        let position = left_child.position().parent().ok()?;
         let hash = node_sum(left_child.hash(), right_child.hash());
-        Self { position, hash }
+        Some(Self { position, hash })
     }
 
-    pub fn position(&self) -> Position {
-        self.position
+    pub fn position(&self) -> &Position {
+        &self.position
     }
 
     pub fn key(&self) -> u64 {

fuel-merkle/src/binary/root_calculator.rs

@@ -25,14 +25,16 @@ impl MerkleRootCalculator {
     }
 
     pub fn push(&mut self, data: &[u8]) {
-        let node = Node::create_leaf(0, data);
+        let node = Node::create_leaf(0, data).expect("Zero is a valid index for a leaf");
         self.stack.push(node);
 
+        #[allow(clippy::arithmetic_side_effects)] // ensured by loop condition
         while self.stack.len() > 1 {
             let right_node = &self.stack[self.stack.len() - 1];
             let left_node = &self.stack[self.stack.len() - 2];
             if right_node.height() == left_node.height() {
-                let merged_node = Node::create_node(left_node, right_node);
+                let merged_node = Node::create_node(left_node, right_node)
+                    .expect("Unable to create a node");
                 self.stack.pop();
                 self.stack.pop();
                 self.stack.push(merged_node);
@@ -47,9 +49,10 @@ impl MerkleRootCalculator {
             return empty_sum().to_owned()
         }
         while self.stack.len() > 1 {
-            let right_child = self.stack.pop().expect("Unable to pop element from stack");
-            let left_child = self.stack.pop().expect("Unable to pop element from stack");
-            let merged_node = Node::create_node(&left_child, &right_child);
+            let right_child = self.stack.pop().expect("Checked in loop bound");
+            let left_child = self.stack.pop().expect("Checked in loop bound");
+            let merged_node = Node::create_node(&left_child, &right_child)
+                .expect("Unable to create a node");
             self.stack.push(merged_node);
         }
         self.stack.pop().unwrap().hash().to_owned()