chore: add clippy::arithmetic-side-effects lint and fix resulting w…

…arnings (#1081)

## Summary
Added a clippy lint to CI to check for arithmetic side-effects (e.g.
overflows).

## Background
This is to harden the production codebase.

## Changes
- Added `--warn clippy::arithmetic-side-effects` to the clippy CI
target.
- Fixed resulting warnings.

## Testing
No new tests added.

## Related Issues
Closes #981.

.github/workflows/test.yml

@@ -153,7 +153,7 @@ jobs:
       - name: run pedantic clippy on workspace crates
         run: |
           cargo clippy --all-targets --all-features \
-          -- --warn clippy::pedantic --deny warnings
+          -- --warn clippy::pedantic --warn clippy::arithmetic-side-effects --deny warnings
       - name: run pedantic clippy on tools/protobuf-compiler
         run: |
           cargo clippy --manifest-path tools/protobuf-compiler/Cargo.toml \

crates/astria-cli/src/commands/rollup.rs

@@ -74,7 +74,11 @@ fn update_yaml_value(
 
     let keys: Vec<&str> = key.split('.').collect();
 
-    for &key in keys.iter().take(keys.len() - 1) {
+    let keys_len_minus_one = keys
+        .len()
+        .checked_sub(1)
+        .expect("`key.split()` should always return at least one value");
+    for &key in keys.iter().take(keys_len_minus_one) {
         target = target
             .get_mut(key)
             .ok_or_else(|| eyre::eyre!("Invalid key path: {}", key))?;

crates/astria-composer/src/executor/bundle_factory/mod.rs

@@ -87,16 +87,18 @@ impl SizedBundle {
             return Err(SizedBundleError::SequenceActionTooLarge(seq_action));
         }
 
-        if self.curr_size + seq_action_size > self.max_size {
+        let new_size = self.curr_size.saturating_add(seq_action_size);
+
+        if new_size > self.max_size {
             return Err(SizedBundleError::NotEnoughSpace(seq_action));
         }
 
         self.rollup_counts
             .entry(seq_action.rollup_id)
-            .and_modify(|count| *count += 1)
+            .and_modify(|count| *count = count.saturating_add(1))
             .or_insert(1);
         self.buffer.push(Action::Sequence(seq_action));
-        self.curr_size += seq_action_size;
+        self.curr_size = new_size;
 
         Ok(())
     }

crates/astria-composer/src/executor/mod.rs

@@ -206,7 +206,11 @@ impl Executor {
         let mut bundle_factory =
             BundleFactory::new(self.max_bytes_per_bundle, self.bundle_queue_capacity);
 
-        let reset_time = || Instant::now() + self.block_time;
+        let reset_time = || {
+            Instant::now()
+                .checked_add(self.block_time)
+                .expect("block_time should not be large enough to cause an overflow")
+        };
 
         let reason = loop {
             select! {
@@ -288,7 +292,7 @@ impl Executor {
         };
 
         let mut bundles_to_drain: VecDeque<SizedBundle> = VecDeque::new();
-        let mut bundles_drained: u64 = 0;
+        let mut bundles_drained: Option<u64> = Some(0);
 
         info!("draining already received transactions");
 
@@ -360,7 +364,7 @@ impl Executor {
                         );
 
                         nonce = new_nonce;
-                        bundles_drained += 1;
+                        bundles_drained = bundles_drained.and_then(|value| value.checked_add(1));
                     }
                     Err(error) => {
                         error!(
@@ -386,9 +390,14 @@ impl Executor {
             Err(error) => error!(%error, "executor shutdown tasks failed to complete in time"),
         }
 
+        let number_of_submitted_bundles = if let Some(value) = bundles_drained {
+            value.to_string()
+        } else {
+            format!("more than {}", u64::MAX)
+        };
         if bundles_to_drain.is_empty() {
             info!(
-                number_of_submitted_bundles = bundles_drained,
+                %number_of_submitted_bundles,
                 "submitted all outstanding bundles to sequencer during shutdown"
             );
         } else {
@@ -397,7 +406,7 @@ impl Executor {
                 bundles_to_drain.iter().map(SizedBundleReport).collect();
 
             warn!(
-                number_of_bundles_submitted = bundles_drained,
+                %number_of_submitted_bundles,
                 number_of_missing_bundles = report.len(),
                 missing_bundles = %telemetry::display::json(&report),
                 "unable to drain all bundles within the allocated time"
@@ -598,7 +607,10 @@ impl Future for SubmitFut {
                             metrics::histogram!(crate::metrics_init::TRANSACTIONS_PER_SUBMISSION)
                                 .record(this.bundle.actions_count() as f64);
 
-                            return Poll::Ready(Ok(*this.nonce + 1));
+                            return Poll::Ready(Ok(this
+                                .nonce
+                                .checked_add(1)
+                                .expect("nonce should not overflow")));
                         };
                         match AbciErrorCode::from(code) {
                             AbciErrorCode::INVALID_NONCE => {

crates/astria-conductor/src/block_cache.rs

@@ -49,7 +49,10 @@ impl<T> BlockCache<T> {
     /// Returns the next sequential block if it exists in the cache.
     pub(crate) fn pop(&mut self) -> Option<T> {
         let block = self.inner.remove(&self.next_height)?;
-        self.next_height += 1;
+        self.next_height = self
+            .next_height
+            .checked_add(1)
+            .expect("block height must not exceed `u64::MAX`");
         Some(block)
     }
 

crates/astria-conductor/src/celestia/block_verifier.rs

@@ -311,7 +311,7 @@ mod test {
         };
 
         (
-            validators::Response::new((height - 1).into(), vec![validator], 1),
+            validators::Response::new(height.checked_sub(1).unwrap().into(), vec![validator], 1),
             address,
             commit,
         )

crates/astria-conductor/src/executor/mod.rs

@@ -708,7 +708,7 @@ fn convert_tendermint_time_to_protobuf_timestamp(value: TendermintTime) -> pbjso
     }
 }
 
-#[derive(Debug)]
+#[derive(Copy, Clone, Debug)]
 enum ExecutionKind {
     Firm,
     Soft,
@@ -725,15 +725,19 @@ impl std::fmt::Display for ExecutionKind {
 }
 
 #[derive(Debug, thiserror::Error)]
-#[error(
-    "contract violated: execution kind: {kind}, current block number {current}, expected \
-     {expected}, received {actual}"
-)]
-struct ContractViolation {
-    kind: ExecutionKind,
-    current: u32,
-    expected: u32,
-    actual: u32,
+enum ContractViolation {
+    #[error(
+        "contract violated: execution kind: {kind}, current block number {current}, expected \
+         {expected}, received {actual}"
+    )]
+    WrongBlock {
+        kind: ExecutionKind,
+        current: u32,
+        expected: u32,
+        actual: u32,
+    },
+    #[error("contract violated: current height cannot be incremented")]
+    CurrentBlockNumberIsMax { kind: ExecutionKind, actual: u32 },
 }
 
 fn does_block_response_fulfill_contract(
@@ -745,12 +749,17 @@ fn does_block_response_fulfill_contract(
         ExecutionKind::Firm => state.firm_number(),
         ExecutionKind::Soft => state.soft_number(),
     };
-    let expected = current + 1;
     let actual = block.number();
+    let expected = current
+        .checked_add(1)
+        .ok_or(ContractViolation::CurrentBlockNumberIsMax {
+            kind,
+            actual,
+        })?;
     if actual == expected {
         Ok(())
     } else {
-        Err(ContractViolation {
+        Err(ContractViolation::WrongBlock {
             kind,
             current,
             expected,

crates/astria-eyre/src/lib.rs

@@ -56,7 +56,7 @@ fn display(
     let mut level = 0;
     write_layer(level, error, f)?;
     while let Some(cause) = error.source() {
-        level += 1;
+        level = level.saturating_add(1);
         f.write_str(", ")?;
         write_layer(level, cause, f)?;
         error = cause;

crates/astria-grpc-mock-test/src/lib.rs

@@ -1,4 +1,4 @@
-#![allow(unreachable_pub, clippy::pedantic)]
+#![allow(unreachable_pub, clippy::pedantic, clippy::arithmetic_side_effects)]
 
 pub mod health {
     include!("generated/grpc.health.v1.rs");

crates/astria-grpc-mock/src/lib.rs

@@ -1,7 +1,10 @@
 // allow: to be fixed in future PRs. This is used for testing and is not in a critical path.
-#![allow(clippy::module_name_repetitions)]
-#![allow(clippy::missing_errors_doc)]
-#![allow(clippy::missing_panics_doc)]
+#![allow(
+    clippy::module_name_repetitions,
+    clippy::missing_errors_doc,
+    clippy::missing_panics_doc,
+    clippy::arithmetic_side_effects
+)]
 
 use std::any::Any;
 

crates/astria-merkle/src/lib.rs

@@ -247,8 +247,9 @@ impl<'a> Drop for LeafBuilder<'a> {
         // append 2 * 32 = 64 zeros
         tree.nodes.extend_from_slice(&[0; 64]);
         let size = tree.len();
-        tree.set_node(size - 1, leaf_hash);
-        let mut idx = tree.len() - 1;
+        // safe to unwrap as we already exited above if `tree.nodes.is_empty()`
+        let mut idx = size.checked_sub(1).expect("tree len must be at least 1");
+        tree.set_node(idx, leaf_hash);
         let root = complete_root(tree.len());
         loop {
             idx = complete_parent(idx, size);
@@ -284,7 +285,9 @@ impl Tree {
     #[inline]
     fn get_node(&self, i: usize) -> [u8; 32] {
         assert!(self.is_in_tree(i));
-        self.nodes[i * 32..(i + 1) * 32].try_into().unwrap()
+        let low = i.checked_mul(32).unwrap();
+        let high = i.checked_add(1).unwrap().checked_mul(32).unwrap();
+        self.nodes[low..high].try_into().unwrap()
     }
 
     /// Returns `true` if the index `i` falls inside the Merkle tree.
@@ -306,7 +309,9 @@ impl Tree {
     #[inline]
     fn set_node(&mut self, i: usize, val: [u8; 32]) {
         assert!(self.is_in_tree(i));
-        self.nodes[i * 32..(i + 1) * 32].copy_from_slice(&val);
+        let low = i.checked_mul(32).unwrap();
+        let high = i.checked_add(1).unwrap().checked_mul(32).unwrap();
+        self.nodes[low..high].copy_from_slice(&val);
     }
 
     /// Constructs the inclusion proof for the i-th leaf of the tree.
@@ -504,21 +509,32 @@ impl Default for Tree {
 ///
 /// Since leaves are always indexed with even numbers and branches with
 /// odd numbers, this is just the formula `i = 2 * j`.
+///
+/// # Panics
+/// Panics if `j` is greater than `usize::MAX / 2`.
 #[inline]
 fn leaf_index_to_tree_index(j: usize) -> usize {
-    j * 2
+    j.checked_mul(2).unwrap()
 }
 
 /// Isolates last set bit of an unsigned integer `x` as a mask.
+///
+/// # Panics
+/// Panics if `x` is 0.
 #[inline]
 fn last_set_bit(x: usize) -> usize {
-    x - ((x - 1) & x)
+    // x - ((x - 1) & x)
+    let x_minus_one = x.checked_sub(1).unwrap();
+    x.checked_sub(x_minus_one & x).unwrap()
 }
 
 /// Isolates the last unset bit of an unsigned integer `x` as a mask.
+///
+/// # Panics
+/// Panics if `x` is `usize::MAX`.
 #[inline]
 fn last_zero_bit(x: usize) -> usize {
-    last_set_bit(x + 1)
+    last_set_bit(x.checked_add(1).unwrap())
 }
 
 /// Returns the parent index of a node at index `i` in a perfect binary tree.
@@ -645,7 +661,10 @@ fn complete_right_child(i: usize, n: usize) -> usize {
     if right_child < n {
         right_child
     } else {
-        i + 1 + complete_root(n - i - 1)
+        // i + 1 + complete_root(n - (i + 1))
+        let i_plus_one = i.checked_add(1).unwrap();
+        let root = complete_root(n.checked_sub(i_plus_one).unwrap());
+        i_plus_one.checked_add(root).unwrap()
     }
 }
 

crates/astria-sequencer-client/src/extension_trait.rs

@@ -526,7 +526,15 @@ pub(super) fn make_path_from_prefix_and_address(
     prefix: &'static [u8],
     address: [u8; 20],
 ) -> String {
-    let mut path = vec![0u8; prefix.len() + address.len() * 2];
+    let address_hex_len = address
+        .len()
+        .checked_mul(2)
+        .expect("`20 * 2` should not overflow");
+    let path_len = prefix
+        .len()
+        .checked_add(address_hex_len)
+        .expect("`prefix` should not be greater than `usize::MAX - 40`");
+    let mut path = vec![0u8; path_len];
     path[..prefix.len()].copy_from_slice(prefix);
     hex::encode_to_slice(address, &mut path[prefix.len()..]).expect(
         "this is a bug: a buffer of sufficient size must have been allocated to hold 20 hex \

crates/astria-sequencer-relayer/src/relayer/read.rs

@@ -158,14 +158,14 @@ impl Stream for BlockStream {
 
         // A new future will be spawned as long as next_height <= latest_height. So
         // 10 - 10 = 0, but there 1 future still to be spawned at next height = 10.
-        lower_limit += Into::<u64>::into(next_height <= latest_height);
+        lower_limit = lower_limit.saturating_add(u64::from(next_height <= latest_height));
 
         // Add 1 if a fetch is in-flight. Example: if requested and latest heights are
         // both 10, then (latest - requested) = 0. But that is only true if the future already
         // resolved and its result was returned. If requested height is 9 and latest is 10,
         // then `(latest - requested) = 1`, meaning there is one more height to be fetched plus
         // the one that's currently in-flight.
-        lower_limit += Into::<u64>::into(self.future.is_some());
+        lower_limit = lower_limit.saturating_add(u64::from(self.future.is_some()));
 
         let lower_limit = lower_limit.try_into().expect(
             "height differences should convert to usize on all reasonable architectures; 64 bit: \