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

@@ -97,15 +97,12 @@ impl<TableType, StorageType> MerkleTree<TableType, StorageType> {
     }
 
     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);
+        let leaf_position = Position::from_leaf_index(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();
@@ -114,13 +111,14 @@ impl<TableType, StorageType> MerkleTree<TableType, StorageType> {
     }
 
     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,7 +158,7 @@ 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))
         }
 
@@ -321,7 +319,11 @@ where
         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(())
     }

fuel-merkle/src/binary/root_calculator.rs

@@ -28,9 +28,13 @@ impl MerkleRootCalculator {
         let node = Node::create_leaf(0, data);
         self.stack.push(node);
 
-        while self.stack.len() > 1 {
-            let right_node = &self.stack[self.stack.len() - 1];
-            let left_node = &self.stack[self.stack.len() - 2];
+        loop {
+            let Some(i) = self.stack.len().checked_sub(2) else {
+                break;
+            };
+            let left_node = &self.stack[i];
+            #[allow(clippy::arithmetic_side_effects)] // stack.len()-2+1 is valid
+            let right_node = &self.stack[i + 1];
             if right_node.height() == left_node.height() {
                 let merged_node = Node::create_node(left_node, right_node);
                 self.stack.pop();

fuel-merkle/src/binary/verify.rs

@@ -9,32 +9,34 @@ use crate::{
     },
 };
 
-fn path_length_from_key(key: u64, num_leaves: u64) -> usize {
-    let mut path_length = 0;
-    while (1u64 << path_length) < num_leaves {
-        path_length += 1;
-
-        if path_length >= 64 {
-            break;
-        }
+/// Returns None if:
+/// - `num_leaves` is 0
+/// - the result wouldn't fit into an usize
+fn path_length_from_key(key: u64, num_leaves: u64) -> Option<usize> {
+    if num_leaves == 0 {
+        return None;
     }
+
+    let path_length =
+        usize::try_from(num_leaves.checked_next_power_of_two()?.ilog2()).ok()?;
+
+    #[allow(clippy::arithmetic_side_effects)] // ilog2(..) > 0
     let num_leaves_left_subtree = 1 << (path_length - 1);
 
-    // If leaf is in left subtree, path length is full height of left subtree
-    if key < num_leaves_left_subtree {
-        path_length
-    }
+    let subtree_leaves = num_leaves.saturating_sub(num_leaves_left_subtree);
+
+    let Some(subtree_key) = key.checked_sub(num_leaves_left_subtree) else {
+        // If leaf is in left subtree, path length is full height of left subtree
+        return Some(path_length);
+    };
+
     // Otherwise, if left or right subtree has only one leaf, path has one additional step
-    else if num_leaves_left_subtree == 1 || num_leaves - num_leaves_left_subtree <= 1 {
-        1
+    if num_leaves_left_subtree == 1 || subtree_leaves <= 1 {
+        return Some(1);
     }
+
     // Otherwise, add 1 to height and recurse into right subtree
-    else {
-        1 + path_length_from_key(
-            key - num_leaves_left_subtree,
-            num_leaves - num_leaves_left_subtree,
-        )
-    }
+    path_length_from_key(subtree_key, subtree_leaves)?.checked_add(1)
 }
 
 pub fn verify<T: AsRef<[u8]>>(
@@ -48,25 +50,33 @@ pub fn verify<T: AsRef<[u8]>>(
         if !proof_set.is_empty() {
             return false;
         }
-    } else if proof_set.len() != path_length_from_key(proof_index, num_leaves) {
+    } else if Some(proof_set.len()) != path_length_from_key(proof_index, num_leaves) {
         return false;
     }
 
     if proof_index >= num_leaves {
-        return false
+        return false;
     }
 
     let mut sum = leaf_sum(data.as_ref());
     if proof_set.is_empty() {
         return if num_leaves == 1 { *root == sum } else { false }
     }
+    #[allow(clippy::arithmetic_side_effects)] // checked above
+    let last_leaf = num_leaves - 1;
 
-    let mut height = 1usize;
+    let mut parent = 0usize;
     let mut stable_end = proof_index;
 
     loop {
-        let subtree_start_index = proof_index / (1 << height) * (1 << height);
-        let subtree_end_index = subtree_start_index + (1 << height) - 1;
+        #[allow(clippy::arithmetic_side_effects)] // path_length_from_key checks
+        let height = parent + 1;
+
+        let subtree_size = 1u64 << height;
+        #[allow(clippy::arithmetic_side_effects)] // floor(a / b) * b <= a
+        let subtree_start_index = proof_index / subtree_size * subtree_size;
+        #[allow(clippy::arithmetic_side_effects)]
+        let subtree_end_index = subtree_start_index + subtree_size - 1;
 
         if subtree_end_index >= num_leaves {
             break
@@ -78,29 +88,39 @@ pub fn verify<T: AsRef<[u8]>>(
             return false
         }
 
-        let proof_data = proof_set[height - 1];
-        if proof_index - subtree_start_index < 1 << (height - 1) {
+        let proof_data = proof_set[parent];
+        #[allow(clippy::arithmetic_side_effects)] // proof_index > subtree_start_index
+        if proof_index - subtree_start_index < (1 << parent) {
             sum = node_sum(&sum, &proof_data);
         } else {
             sum = node_sum(&proof_data, &sum);
         }
 
-        height += 1;
+        #[allow(clippy::arithmetic_side_effects)] // path_length_from_key checks
+        {
+            parent += 1;
+        }
     }
 
-    if stable_end != num_leaves - 1 {
-        if proof_set.len() < height {
+    if stable_end != last_leaf {
+        if proof_set.len() <= parent {
             return false
         }
-        let proof_data = proof_set[height - 1];
+        let proof_data = proof_set[parent];
         sum = node_sum(&sum, &proof_data);
-        height += 1;
+        #[allow(clippy::arithmetic_side_effects)] // path_length_from_key checks
+        {
+            parent += 1;
+        }
     }
 
-    while height - 1 < proof_set.len() {
-        let proof_data = proof_set[height - 1];
+    while parent < proof_set.len() {
+        let proof_data = proof_set[parent];
         sum = node_sum(&proof_data, &sum);
-        height += 1;
+        #[allow(clippy::arithmetic_side_effects)] // path_length_from_key checks
+        {
+            parent += 1;
+        }
     }
 
     sum == *root

fuel-merkle/src/common.rs

@@ -16,10 +16,7 @@ pub use position::Position;
 pub use storage_map::StorageMap;
 pub use subtree::Subtree;
 
-pub(crate) use msb::{
-    Bit,
-    Msb,
-};
+pub(crate) use msb::Msb;
 pub(crate) use position_path::PositionPath;
 pub(crate) use prefix::{
     Prefix,

fuel-merkle/src/common/msb.rs

@@ -1,50 +1,32 @@
-#[derive(Debug, Eq, PartialEq)]
-pub enum Bit {
-    _0 = 0,
-    _1 = 1,
-}
-
-trait GetBit {
-    fn get_bit(&self, bit_index: u32) -> Option<Bit>;
-}
-
-impl GetBit for u8 {
-    fn get_bit(&self, bit_index: u32) -> Option<Bit> {
-        if bit_index < 8 {
-            let mask = 1 << (7 - bit_index);
-            let bit = self & mask;
-            match bit {
-                0 => Some(Bit::_0),
-                _ => Some(Bit::_1),
-            }
-        } else {
-            None
-        }
-    }
-}
-
 pub trait Msb {
-    fn get_bit_at_index_from_msb(&self, index: u32) -> Option<Bit>;
-    fn common_prefix_count(&self, other: &[u8]) -> u32;
+    fn get_bit_at_index_from_msb(&self, index: u32) -> Option<bool>;
+    fn common_prefix_count(&self, other: &[u8]) -> u64;
 }
 
 impl<const N: usize> Msb for [u8; N] {
-    fn get_bit_at_index_from_msb(&self, index: u32) -> Option<Bit> {
+    fn get_bit_at_index_from_msb(&self, index: u32) -> Option<bool> {
         // The byte that contains the bit
         let byte_index = index / 8;
         // The bit within the containing byte
         let byte_bit_index = index % 8;
-        self.get(byte_index as usize)
-            .and_then(|byte| byte.get_bit(byte_bit_index))
+        self.get(byte_index as usize).map(|byte| {
+            #[allow(clippy::arithmetic_side_effects)] // checked above
+            let mask = 1 << (7 - byte_bit_index);
+            byte & mask != 0
+        })
     }
 
-    fn common_prefix_count(&self, other: &[u8]) -> u32 {
+    fn common_prefix_count(&self, other: &[u8]) -> u64 {
         let mut count = 0;
         for (byte1, byte2) in self.iter().zip(other.iter()) {
             // For each pair of bytes, compute the similarity of each byte using
             // exclusive or (XOR). The leading zeros measures the number of
             // similar bits from left to right. For equal bytes, this will be 8.
-            count += (byte1 ^ byte2).leading_zeros();
+            let common_bits = (byte1 ^ byte2).leading_zeros();
+            #[allow(clippy::arithmetic_side_effects)] // u64 is always large enough
+            {
+                count += common_bits as u64;
+            }
             if byte1 != byte2 {
                 break
             }

fuel-merkle/src/common/node.rs

@@ -4,10 +4,7 @@ use crate::common::{
 };
 
 use alloc::string::String;
-use core::{
-    fmt,
-    mem,
-};
+use core::fmt;
 
 pub trait KeyFormatting {
     type PrettyType: fmt::Display;
@@ -18,11 +15,7 @@ pub trait KeyFormatting {
 pub trait Node {
     type Key: KeyFormatting;
 
-    fn key_size_in_bits() -> u32 {
-        u32::try_from(mem::size_of::<Self::Key>() * 8)
-            .expect("The key usually is several bytes")
-    }
-
+    fn key_size_bits() -> u32;
     fn height(&self) -> u32;
     fn leaf_key(&self) -> Self::Key;
     fn is_leaf(&self) -> bool;

fuel-merkle/src/common/path.rs

@@ -1,44 +1,36 @@
-use crate::common::{
-    Bit,
-    Msb,
-};
+use crate::common::Msb;
 
-pub enum Instruction {
+/// The side of a child node in a binary tree.
+pub enum Side {
     Left,
     Right,
 }
 
-impl From<Bit> for Instruction {
-    fn from(bit: Bit) -> Self {
+impl From<bool> for Side {
+    fn from(bit: bool) -> Self {
         match bit {
-            Bit::_0 => Instruction::Left,
-            Bit::_1 => Instruction::Right,
+            false => Side::Left,
+            true => Side::Right,
         }
     }
 }
 
 pub trait Path {
-    fn get_instruction(&self, index: u32) -> Option<Instruction>;
-}
+    /// Which child node to follow at the given index.
+    fn get_instruction(&self, index: u32) -> Option<Side>;
 
-pub trait ComparablePath {
-    fn common_path_length(&self, other: &[u8]) -> u32;
+    fn common_path_length(&self, other: &[u8]) -> u64;
 }
 
 impl<T> Path for T
 where
     T: Msb,
 {
-    fn get_instruction(&self, index: u32) -> Option<Instruction> {
+    fn get_instruction(&self, index: u32) -> Option<Side> {
         self.get_bit_at_index_from_msb(index).map(Into::into)
     }
-}
 
-impl<T> ComparablePath for T
-where
-    T: Msb,
-{
-    fn common_path_length(&self, other: &[u8]) -> u32 {
+    fn common_path_length(&self, other: &[u8]) -> u64 {
         self.common_prefix_count(other)
     }
 }