More powerful clippy lints

Cargo.toml

@@ -35,6 +35,11 @@ members = [
 
 resolver = "2"
 
+[workspace.lints.clippy]
+#unwrap_used = "warn"
+arithmetic_side_effects = "warn"
+indexing_slicing = "warn"
+
 [workspace.dependencies]
 mina-p2p-messages = { path = "mina-p2p-messages" }
 ledger = { path = "ledger", package = "mina-tree" }
@@ -48,7 +53,7 @@ poly-commitment = {git = "https://github.com/openmina/proof-systems", rev = "2fd
 libp2p = { git = "https://github.com/openmina/rust-libp2p", rev = "5c44c7d9", default-features = false }
 vrf = { path = "vrf" }
 openmina-node-account = { path = "node/account" }
-redux = { git = "https://github.com/openmina/redux-rs.git", rev = "741a01d", features = ["serde"] }
+redux = { git = "https://github.com/openmina/redux-rs.git", rev = "ab14890c", features = ["serde"] }
 serde = "1.0.190"
 serde_json = "1.0.107"
 serde_with = { version = "3.7.0", features = ["hex"] }

clippy.toml

@@ -0,0 +1 @@
+allow-unwrap-in-tests=true

node/Cargo.toml

@@ -4,6 +4,9 @@ version = "0.11.0"
 edition = "2021"
 license = "Apache-2.0"
 
+[lints]
+workspace = true
+
 [dependencies]
 rand = "0.8.0"
 serde = "1.0.147"

node/build.rs

@@ -126,7 +126,7 @@ fn main() -> Result<(), Box<dyn Error>> {
 
     visit_dirs(&node_dir, &mut |file| {
         let mut path = file.path();
-        let path_str = path.to_str().unwrap();
+        let path_str = path.to_str().expect("path");
         if !path_str.ends_with("_actions.rs") && !path_str.ends_with("action.rs") {
             return;
         }
@@ -156,7 +156,8 @@ fn main() -> Result<(), Box<dyn Error>> {
                     if let Some(action_name) = matches.get(2) {
                         let action_name = action_name.as_str().to_owned();
                         // Without 'Action' suffix
-                        let action_name_base = action_name[..(action_name.len() - 6)].to_string();
+                        let action_name_base =
+                            action_name[..(action_name.len().saturating_sub(6))].to_string();
                         let mut variant_lines = vec![];
                         loop {
                             let Some(line) = lines.next() else { break };
@@ -252,8 +253,7 @@ fn main() -> Result<(), Box<dyn Error>> {
         .map(|(k, v)| {
             let mut s = "use crate::".to_owned();
             if !k.is_empty() {
-                s += &k.join("::");
-                s += "::";
+                s.push_str(&format!("{}::", k.join("::")));
             }
             s += &format!("{{{}}};", v.join(", "));
             s
@@ -268,8 +268,7 @@ fn main() -> Result<(), Box<dyn Error>> {
             if meta.is_inlined() {
                 meta.action_kinds()
             } else {
-                // Remove suffix `Action` from action name.
-                vec![name[..(name.len() - 6)].to_string()]
+                vec![name[..name.len().saturating_sub(6)].to_string()]
             }
         });
     let action_kinds = std::iter::once("None".to_owned())
@@ -304,7 +303,7 @@ fn main() -> Result<(), Box<dyn Error>> {
         while let Some(action_name) = queue.pop_front() {
             let fn_body = match actions.get(dbg!(&action_name)).unwrap() {
                 ActionMeta::Struct => {
-                    let action_kind = &action_name[..(action_name.len() - 6)];
+                    let action_kind = &action_name[..(action_name.len().saturating_sub(6))];
                     format!("ActionKind::{action_kind}")
                 }
                 ActionMeta::Enum(variants) => {

node/common/src/service/p2p.rs

@@ -44,7 +44,7 @@ impl webrtc::P2pServiceWebrtc for NodeService {
         &mut self,
         other_pk: &PublicKey,
         message: &T,
-    ) -> Result<T::Encrypted, ()> {
+    ) -> Result<T::Encrypted, Box<dyn std::error::Error>> {
         let rng = &mut self.rng;
         self.p2p.sec_key.encrypt(other_pk, rng, message)
     }
@@ -53,7 +53,7 @@ impl webrtc::P2pServiceWebrtc for NodeService {
         &mut self,
         other_pk: &PublicKey,
         encrypted: &T::Encrypted,
-    ) -> Result<T, ()> {
+    ) -> Result<T, Box<dyn std::error::Error>> {
         self.p2p.sec_key.decrypt(other_pk, encrypted)
     }
 

node/src/block_producer/block_producer_reducer.rs

@@ -145,9 +145,10 @@ impl BlockProducerEnabled {
                 if let Some(won_slot) = state.current.won_slot() {
                     if let Some(chain) = best_chain.last().map(|best_tip| {
                         if best_tip.global_slot() == won_slot.global_slot() {
-                            // We are producing block which replaces current best tip
-                            // instead of extending it.
-                            best_chain[..(best_chain.len() - 1)].to_vec()
+                            best_chain
+                                .get(..best_chain.len().saturating_sub(1))
+                                .unwrap_or(&[])
+                                .to_vec()
                         } else {
                             best_chain.to_vec()
                         }
@@ -430,14 +431,24 @@ impl BlockProducerEnabled {
                         epoch_length: v2::UnsignedExtendedUInt32StableV1(1.into()),
                     };
                 let epoch_count = v2::UnsignedExtendedUInt32StableV1(
-                    (pred_consensus_state.epoch_count.as_u32() + 1).into(),
+                    (pred_consensus_state
+                        .epoch_count
+                        .as_u32()
+                        .checked_add(1)
+                        .expect("overflow"))
+                    .into(),
                 );
                 (staking_data, next_data, epoch_count)
             } else {
                 assert_eq!(pred_epoch, next_epoch);
                 let mut next_data = pred_consensus_state.next_epoch_data.clone();
                 next_data.epoch_length = v2::UnsignedExtendedUInt32StableV1(
-                    (next_data.epoch_length.as_u32() + 1).into(),
+                    (next_data
+                        .epoch_length
+                        .as_u32()
+                        .checked_add(1)
+                        .expect("overflow"))
+                    .into(),
                 );
                 (
                     pred_consensus_state.staking_epoch_data.clone(),
@@ -475,7 +486,8 @@ impl BlockProducerEnabled {
 
             let is_same_global_sub_window = pred_global_sub_window == next_global_sub_window;
             let are_windows_overlapping = pred_global_sub_window
-                + constraint_constants().sub_windows_per_window as u32
+                .checked_add(constraint_constants().sub_windows_per_window as u32)
+                .expect("overflow")
                 >= next_global_sub_window;
 
             let current_sub_window_densities = pred_sub_window_densities
@@ -525,7 +537,7 @@ impl BlockProducerEnabled {
                             0
                         };
                         if incr_window {
-                            density + 1
+                            density.saturating_add(1)
                         } else {
                             density
                         }
@@ -545,7 +557,9 @@ impl BlockProducerEnabled {
             &global_slot_since_genesis,
         );
         let consensus_state = v2::ConsensusProofOfStakeDataConsensusStateValueStableV2 {
-            blockchain_length: v2::UnsignedExtendedUInt32StableV1((pred_block.height() + 1).into()),
+            blockchain_length: v2::UnsignedExtendedUInt32StableV1(
+                (pred_block.height().checked_add(1).expect("overflow")).into(),
+            ),
             epoch_count,
             min_window_density,
             sub_window_densities,
@@ -592,7 +606,11 @@ impl BlockProducerEnabled {
             0 => (pred_block.hash().clone(), List::new()),
             chain_proof_len => {
                 // TODO(binier): test
-                let mut iter = chain.iter().rev().take(chain_proof_len + 1).rev();
+                let mut iter = chain
+                    .iter()
+                    .rev()
+                    .take(chain_proof_len.saturating_add(1))
+                    .rev();
                 if let Some(first_block) = iter.next() {
                     let first_hash = first_block.hash().clone();
                     let body_hashes = iter

node/src/block_producer/block_producer_state.rs

@@ -302,8 +302,13 @@ impl BlockProducerCurrentState {
         match self {
             Self::WonSlot { won_slot, .. } | Self::WonSlotWait { won_slot, .. } => {
                 // Make sure to only producer blocks when in the slot interval
-                let slot_upper_bound = won_slot.slot_time + slot_interval;
-                let estimated_produced_time = now + BLOCK_PRODUCTION_ESTIMATE;
+                let slot_upper_bound = won_slot
+                    .slot_time
+                    .checked_add(slot_interval)
+                    .expect("overflow");
+                let estimated_produced_time = now
+                    .checked_add(BLOCK_PRODUCTION_ESTIMATE)
+                    .expect("overflow");
                 estimated_produced_time >= won_slot.slot_time && now < slot_upper_bound
             }
             _ => false,

node/src/block_producer/mod.rs

@@ -78,22 +78,30 @@ impl BlockProducerWonSlot {
     fn calculate_slot_time(genesis_timestamp: redux::Timestamp, slot: u32) -> redux::Timestamp {
         // FIXME: this calculation must use values from the protocol constants,
         // now it assumes 3 minutes blocks.
-        genesis_timestamp + (slot as u64) * 3 * 60 * 1_000_000_000_u64
+        genesis_timestamp
+            .checked_add(
+                (slot as u64)
+                    .checked_mul(3u64.saturating_mul(60).saturating_mul(1_000_000_000_u64))
+                    .expect("overflow"),
+            )
+            .expect("overflow")
     }
 
     pub fn global_slot(&self) -> u32 {
         self.global_slot.slot_number.as_u32()
     }
 
     pub fn epoch(&self) -> u32 {
-        self.global_slot() / self.global_slot.slots_per_epoch.as_u32()
+        self.global_slot()
+            .checked_div(self.global_slot.slots_per_epoch.as_u32())
+            .expect("division by 0")
     }
 
     pub fn global_slot_since_genesis(
         &self,
         slot_diff: u32,
     ) -> v2::MinaNumbersGlobalSlotSinceGenesisMStableV1 {
-        let slot = self.global_slot() + slot_diff;
+        let slot = self.global_slot().checked_add(slot_diff).expect("overflow");
         v2::MinaNumbersGlobalSlotSinceGenesisMStableV1::SinceGenesis(slot.into())
     }
 
@@ -105,7 +113,9 @@ impl BlockProducerWonSlot {
     }
 
     pub fn next_slot_time(&self) -> redux::Timestamp {
-        self.slot_time + 3 * 60 * 1_000_000_000_u64
+        self.slot_time
+            .checked_add(3u64.saturating_mul(60).saturating_mul(1_000_000_000_u64))
+            .expect("overflow")
     }
 }
 
@@ -145,14 +155,24 @@ impl PartialOrd<ArcBlockWithHash> for BlockProducerWonSlot {
 }
 
 pub fn to_epoch_and_slot(global_slot: &v2::ConsensusGlobalSlotStableV1) -> (u32, u32) {
-    let epoch = global_slot.slot_number.as_u32() / global_slot.slots_per_epoch.as_u32();
-    let slot = global_slot.slot_number.as_u32() % global_slot.slots_per_epoch.as_u32();
+    let epoch = global_slot
+        .slot_number
+        .as_u32()
+        .checked_div(global_slot.slots_per_epoch.as_u32())
+        .expect("division by 0");
+    let slot = global_slot
+        .slot_number
+        .as_u32()
+        .checked_rem(global_slot.slots_per_epoch.as_u32())
+        .expect("division by 0");
     (epoch, slot)
 }
 
 pub fn next_epoch_first_slot(global_slot: &v2::ConsensusGlobalSlotStableV1) -> u32 {
     let (epoch, _) = to_epoch_and_slot(global_slot);
-    (epoch + 1) * global_slot.slots_per_epoch.as_u32()
+    (epoch.saturating_add(1))
+        .checked_mul(global_slot.slots_per_epoch.as_u32())
+        .expect("overflow")
 }
 
 // Returns the epoch number and whether it is the last slot of the epoch

node/src/block_producer/vrf_evaluator/block_producer_vrf_evaluator_actions.rs

@@ -133,7 +133,11 @@ impl redux::EnablingCondition<crate::State> for BlockProducerVrfEvaluatorAction
             } => state.block_producer.with(false, |this| {
                 if this.vrf_evaluator.is_slot_requested() {
                     if let Some(current_evaluation) = this.vrf_evaluator.current_evaluation() {
-                        current_evaluation.latest_evaluated_slot + 1 == vrf_output.global_slot()
+                        current_evaluation
+                            .latest_evaluated_slot
+                            .checked_add(1)
+                            .expect("overflow")
+                            == vrf_output.global_slot()
                             && current_evaluation.epoch_data.ledger == *staking_ledger_hash
                     } else {
                         false

node/src/block_producer/vrf_evaluator/block_producer_vrf_evaluator_reducer.rs

@@ -191,7 +191,8 @@ impl BlockProducerVrfEvaluatorState {
                     best_tip_epoch,
                     root_block_epoch: *root_block_epoch,
                     is_current_epoch_evaluated: state.is_epoch_evaluated(best_tip_epoch),
-                    is_next_epoch_evaluated: state.is_epoch_evaluated(best_tip_epoch + 1),
+                    is_next_epoch_evaluated: state
+                        .is_epoch_evaluated(best_tip_epoch.checked_add(1).expect("overflow")),
                     last_evaluated_epoch: state.last_evaluated_epoch(),
                     staking_epoch_data: staking_epoch_data.clone(),
                     next_epoch_data: next_epoch_data.clone(),
@@ -235,7 +236,8 @@ impl BlockProducerVrfEvaluatorState {
                     time: meta.time(),
                     best_tip_epoch: *best_tip_epoch,
                     is_current_epoch_evaluated: state.is_epoch_evaluated(*best_tip_epoch),
-                    is_next_epoch_evaluated: state.is_epoch_evaluated(best_tip_epoch + 1),
+                    is_next_epoch_evaluated: state
+                        .is_epoch_evaluated(best_tip_epoch.checked_add(1).expect("overflow")),
                     best_tip_slot: *best_tip_slot,
                     best_tip_global_slot: *best_tip_global_slot,
                     next_epoch_first_slot: *next_epoch_first_slot,
@@ -425,7 +427,10 @@ impl BlockProducerVrfEvaluatorState {
             } => {
                 let (epoch_number, initial_slot) = match state.epoch_context() {
                     super::EpochContext::Current(_) => (*best_tip_epoch, *current_global_slot),
-                    super::EpochContext::Next(_) => (best_tip_epoch + 1, next_epoch_first_slot - 1),
+                    super::EpochContext::Next(_) => (
+                        best_tip_epoch.checked_add(1).expect("overflow"),
+                        next_epoch_first_slot.checked_sub(1).expect("underflow"),
+                    ),
                     super::EpochContext::Waiting => todo!(),
                 };
                 state.status = BlockProducerVrfEvaluatorStatus::InitialSlotSelection {

node/src/block_producer/vrf_evaluator/block_producer_vrf_evaluator_state.rs

@@ -46,7 +46,7 @@ impl BlockProducerVrfEvaluatorState {
     pub fn evaluate_epoch_bounds(global_slot: &u32) -> SlotPositionInEpoch {
         if global_slot % SLOTS_PER_EPOCH == 0 {
             SlotPositionInEpoch::Beginning
-        } else if (global_slot + 1) % SLOTS_PER_EPOCH == 0 {
+        } else if (global_slot.checked_add(1).expect("overflow")) % SLOTS_PER_EPOCH == 0 {
             SlotPositionInEpoch::End
         } else {
             SlotPositionInEpoch::Within
@@ -111,7 +111,7 @@ impl BlockProducerVrfEvaluatorState {
 
             if !self.is_epoch_evaluated(best_tip_epoch) {
                 self.epoch_context = EpochContext::Current((*staking_epoch_data).into())
-            } else if !self.is_epoch_evaluated(best_tip_epoch + 1) {
+            } else if !self.is_epoch_evaluated(best_tip_epoch.checked_add(1).expect("overflow")) {
                 if root_block_epoch == best_tip_epoch && is_next_epoch_seed_finalized {
                     self.epoch_context = EpochContext::Next((*next_epoch_data).into())
                 } else {
@@ -284,7 +284,9 @@ impl BlockProducerVrfEvaluatorState {
         {
             match self.epoch_context {
                 EpochContext::Current(_) => self.last_evaluated_epoch = Some(*epoch_number),
-                EpochContext::Next(_) => self.last_evaluated_epoch = Some(epoch_number + 1),
+                EpochContext::Next(_) => {
+                    self.last_evaluated_epoch = Some(epoch_number.checked_add(1).expect("overflow"))
+                }
                 EpochContext::Waiting => {}
             }
         }
@@ -333,7 +335,10 @@ impl BlockProducerVrfEvaluatorState {
             Some(VrfEvaluatorInput::new(
                 pending_evaluation.epoch_data.seed,
                 pending_evaluation.epoch_data.delegator_table,
-                pending_evaluation.latest_evaluated_slot + 1,
+                pending_evaluation
+                    .latest_evaluated_slot
+                    .checked_add(1)
+                    .expect("overflow"),
                 pending_evaluation.epoch_data.total_currency,
                 pending_evaluation.epoch_data.ledger,
             ))
@@ -345,7 +350,7 @@ impl BlockProducerVrfEvaluatorState {
     pub fn retention_slot(&self, current_epoch_number: &u32) -> u32 {
         const PAST_EPOCHS_TO_KEEP: u32 = 2;
         let cutoff_epoch = current_epoch_number.saturating_sub(PAST_EPOCHS_TO_KEEP);
-        (cutoff_epoch * SLOTS_PER_EPOCH).saturating_sub(1)
+        (cutoff_epoch.saturating_mul(SLOTS_PER_EPOCH)).saturating_sub(1)
     }
 
     pub fn cleanup_old_won_slots(&mut self, current_epoch_number: &u32) {

node/src/ledger/ledger_service.rs

@@ -856,7 +856,7 @@ impl LedgerCtx {
 
         let num_accounts = mask.num_accounts() as u64;
         let first_node_addr = ledger::Address::first(
-            LEDGER_DEPTH - super::tree_height_for_num_accounts(num_accounts),
+            LEDGER_DEPTH.saturating_sub(super::tree_height_for_num_accounts(num_accounts)),
         );
         let hash = LedgerHash::from_fp(mask.get_hash(first_node_addr)?);
         Some((num_accounts, hash))