Use checked arithmetic in types and state proc

.github/workflows/test-suite.yml

@@ -65,3 +65,10 @@ jobs:
     - uses: actions/checkout@v1
     - name: Typecheck benchmark code without running it
       run: make check-benches
+  clippy:
+    runs-on: ubuntu-latest
+    needs: cargo-fmt
+    steps:
+    - uses: actions/checkout@v1
+    - name: Lint code for quality and style with Clippy
+      run: make lint

Cargo.toml

@@ -17,6 +17,7 @@ members = [
     "eth2/utils/lighthouse_bootstrap",
     "eth2/utils/merkle_proof",
     "eth2/utils/int_to_bytes",
+    "eth2/utils/safe_arith",
     "eth2/utils/serde_hex",
     "eth2/utils/slot_clock",
     "eth2/utils/ssz",
@@ -56,3 +57,9 @@ eth2_ssz = { path = "eth2/utils/ssz" }
 eth2_ssz_derive = { path = "eth2/utils/ssz_derive" }
 eth2_ssz_types = { path = "eth2/utils/ssz_types" }
 eth2_hashing = { path = "eth2/utils/eth2_hashing" }
+
+# [profile.release]
+# overflow-checks = true
+
+# [profile.bench]
+# overflow-checks = true

Makefile

@@ -41,6 +41,11 @@ test: test-release
 # Runs the entire test suite, downloading test vectors if required.
 test-full: cargo-fmt test-release test-debug test-ef
 
+# Lints the code for bad style and potentially unsafe arithmetic using Clippy.
+# Clippy lints are opt-in per-crate for now, which is why we all all by default.
+lint:
+	cargo clippy --all -- -A clippy::all
+
 # Runs the makefile in the `ef_tests` repo.
 #
 # May download and extract an archive of test vectors from the ethereum

beacon_node/beacon_chain/Cargo.toml

@@ -43,6 +43,7 @@ rand = "0.7.2"
 proto_array_fork_choice = { path = "../../eth2/proto_array_fork_choice" }
 lru = "0.4.3"
 tempfile = "3.1.0"
+safe_arith = { path = "../../eth2/utils/safe_arith" }
 
 [dev-dependencies]
 lazy_static = "1.4.0"

beacon_node/beacon_chain/src/eth1_chain.rs

@@ -45,6 +45,14 @@ pub enum Error {
     ///
     /// The eth1 caches are stale, or a junk value was voted into the chain.
     UnknownPreviousEth1BlockHash,
+    /// An arithmetic error occurred.
+    ArithError(safe_arith::ArithError),
+}
+
+impl From<safe_arith::ArithError> for Error {
+    fn from(e: safe_arith::ArithError) -> Self {
+        Self::ArithError(e)
+    }
 }
 
 #[derive(Encode, Decode, Clone)]
@@ -369,7 +377,7 @@ impl<T: EthSpec, S: Store<T>> Eth1ChainBackend<T, S> for CachingEth1Backend<T, S
         _spec: &ChainSpec,
     ) -> Result<Vec<Deposit>, Error> {
         let deposit_index = state.eth1_deposit_index;
-        let deposit_count = if let Some(new_eth1_data) = get_new_eth1_data(state, eth1_data_vote) {
+        let deposit_count = if let Some(new_eth1_data) = get_new_eth1_data(state, eth1_data_vote)? {
             new_eth1_data.deposit_count
         } else {
             state.eth1_data.deposit_count

beacon_node/genesis/src/eth1_genesis_service.rs

@@ -383,7 +383,8 @@ impl Eth1GenesisService {
                         .map_err(|e| format!("Error whilst processing deposit: {:?}", e))
                 })?;
 
-            process_activations(&mut local_state, spec);
+            process_activations(&mut local_state, spec)
+                .map_err(|e| format!("Error whilst processing activations: {:?}", e))?;
             let is_valid = is_valid_genesis_state(&local_state, spec);
 
             trace!(

eth2/state_processing/Cargo.toml

@@ -27,6 +27,7 @@ eth2_ssz = "0.1.2"
 eth2_ssz_types = { path = "../utils/ssz_types" }
 merkle_proof = { path = "../utils/merkle_proof" }
 log = "0.4.8"
+safe_arith = { path = "../utils/safe_arith" }
 tree_hash = "0.1.0"
 tree_hash_derive = "0.2"
 types = { path = "../types" }

eth2/state_processing/src/common/deposit_data_tree.rs

@@ -1,6 +1,7 @@
 use eth2_hashing::hash;
 use int_to_bytes::int_to_bytes32;
 use merkle_proof::{MerkleTree, MerkleTreeError};
+use safe_arith::SafeArith;
 use types::Hash256;
 
 /// Emulates the eth1 deposit contract merkle tree.
@@ -46,7 +47,7 @@ impl DepositDataTree {
     /// Add a deposit to the merkle tree.
     pub fn push_leaf(&mut self, leaf: Hash256) -> Result<(), MerkleTreeError> {
         self.tree.push_leaf(leaf, self.depth)?;
-        self.mix_in_length += 1;
+        self.mix_in_length.increment()?;
         Ok(())
     }
 }

eth2/state_processing/src/common/get_base_reward.rs

@@ -1,4 +1,5 @@
 use integer_sqrt::IntegerSquareRoot;
+use safe_arith::SafeArith;
 use types::*;
 
 /// Returns the base reward for some validator.
@@ -14,10 +15,10 @@ pub fn get_base_reward<T: EthSpec>(
     if total_active_balance == 0 {
         Ok(0)
     } else {
-        Ok(
-            state.get_effective_balance(index, spec)? * spec.base_reward_factor
-                / total_active_balance.integer_sqrt()
-                / spec.base_rewards_per_epoch,
-        )
+        Ok(state
+            .get_effective_balance(index, spec)?
+            .safe_mul(spec.base_reward_factor)?
+            .safe_div(total_active_balance.integer_sqrt())?
+            .safe_div(spec.base_rewards_per_epoch)?)
     }
 }

eth2/state_processing/src/common/slash_validator.rs

@@ -1,4 +1,5 @@
 use crate::common::initiate_validator_exit;
+use safe_arith::SafeArith;
 use std::cmp;
 use types::{BeaconStateError as Error, *};
 
@@ -27,18 +28,21 @@ pub fn slash_validator<T: EthSpec>(
     let validator_effective_balance = state.get_effective_balance(slashed_index, spec)?;
     state.set_slashings(
         epoch,
-        state.get_slashings(epoch)? + validator_effective_balance,
+        state
+            .get_slashings(epoch)?
+            .safe_add(validator_effective_balance)?,
     )?;
     safe_sub_assign!(
         state.balances[slashed_index],
-        validator_effective_balance / spec.min_slashing_penalty_quotient
+        validator_effective_balance.safe_div(spec.min_slashing_penalty_quotient)?
     );
 
     // Apply proposer and whistleblower rewards
     let proposer_index = state.get_beacon_proposer_index(state.slot, spec)?;
     let whistleblower_index = opt_whistleblower_index.unwrap_or(proposer_index);
-    let whistleblower_reward = validator_effective_balance / spec.whistleblower_reward_quotient;
-    let proposer_reward = whistleblower_reward / spec.proposer_reward_quotient;
+    let whistleblower_reward =
+        validator_effective_balance.safe_div(spec.whistleblower_reward_quotient)?;
+    let proposer_reward = whistleblower_reward.safe_div(spec.proposer_reward_quotient)?;
 
     safe_add_assign!(state.balances[proposer_index], proposer_reward);
     safe_add_assign!(

eth2/state_processing/src/genesis.rs

@@ -1,5 +1,6 @@
 use super::per_block_processing::{errors::BlockProcessingError, process_deposit};
 use crate::common::DepositDataTree;
+use safe_arith::SafeArith;
 use tree_hash::TreeHash;
 use types::DEPOSIT_TREE_DEPTH;
 use types::*;
@@ -14,8 +15,9 @@ pub fn initialize_beacon_state_from_eth1<T: EthSpec>(
     deposits: Vec<Deposit>,
     spec: &ChainSpec,
 ) -> Result<BeaconState<T>, BlockProcessingError> {
-    let genesis_time =
-        eth1_timestamp - eth1_timestamp % spec.min_genesis_delay + 2 * spec.min_genesis_delay;
+    let genesis_time = eth1_timestamp
+        .safe_sub(eth1_timestamp.safe_rem(spec.min_genesis_delay)?)?
+        .safe_add(2.safe_mul(spec.min_genesis_delay)?)?;
     let eth1_data = Eth1Data {
         // Temporary deposit root
         deposit_root: Hash256::zero(),
@@ -37,7 +39,7 @@ pub fn initialize_beacon_state_from_eth1<T: EthSpec>(
         process_deposit(&mut state, &deposit, spec, true)?;
     }
 
-    process_activations(&mut state, spec);
+    process_activations(&mut state, spec)?;
 
     // Now that we have our validators, initialize the caches (including the committees)
     state.build_all_caches(spec)?;
@@ -60,16 +62,20 @@ pub fn is_valid_genesis_state<T: EthSpec>(state: &BeaconState<T>, spec: &ChainSp
 /// Activate genesis validators, if their balance is acceptable.
 ///
 /// Spec v0.11.1
-pub fn process_activations<T: EthSpec>(state: &mut BeaconState<T>, spec: &ChainSpec) {
+pub fn process_activations<T: EthSpec>(
+    state: &mut BeaconState<T>,
+    spec: &ChainSpec,
+) -> Result<(), Error> {
     for (index, validator) in state.validators.iter_mut().enumerate() {
         let balance = state.balances[index];
         validator.effective_balance = std::cmp::min(
-            balance - balance % spec.effective_balance_increment,
+            balance.safe_sub(balance.safe_rem(spec.effective_balance_increment)?)?,
             spec.max_effective_balance,
         );
         if validator.effective_balance == spec.max_effective_balance {
             validator.activation_eligibility_epoch = T::genesis_epoch();
             validator.activation_epoch = T::genesis_epoch();
         }
     }
+    Ok(())
 }

eth2/state_processing/src/lib.rs

@@ -1,3 +1,5 @@
+#![deny(clippy::integer_arithmetic)]
+
 #[macro_use]
 mod macros;
 

eth2/state_processing/src/per_block_processing.rs

@@ -1,6 +1,7 @@
 use crate::common::{initiate_validator_exit, slash_validator};
 use errors::{BlockOperationError, BlockProcessingError, HeaderInvalid, IntoWithIndex};
 use rayon::prelude::*;
+use safe_arith::{ArithError, SafeArith};
 use signature_sets::{block_proposal_signature_set, get_pubkey_from_state, randao_signature_set};
 use std::convert::TryInto;
 use tree_hash::TreeHash;
@@ -239,7 +240,7 @@ pub fn process_eth1_data<T: EthSpec>(
     state: &mut BeaconState<T>,
     eth1_data: &Eth1Data,
 ) -> Result<(), Error> {
-    if let Some(new_eth1_data) = get_new_eth1_data(state, eth1_data) {
+    if let Some(new_eth1_data) = get_new_eth1_data(state, eth1_data)? {
         state.eth1_data = new_eth1_data;
     }
 
@@ -248,25 +249,25 @@ pub fn process_eth1_data<T: EthSpec>(
     Ok(())
 }
 
-/// Returns `Some(eth1_data)` if adding the given `eth1_data` to `state.eth1_data_votes` would
+/// Returns `Ok(Some(eth1_data))` if adding the given `eth1_data` to `state.eth1_data_votes` would
 /// result in a change to `state.eth1_data`.
 ///
 /// Spec v0.11.1
 pub fn get_new_eth1_data<T: EthSpec>(
     state: &BeaconState<T>,
     eth1_data: &Eth1Data,
-) -> Option<Eth1Data> {
+) -> Result<Option<Eth1Data>, ArithError> {
     let num_votes = state
         .eth1_data_votes
         .iter()
         .filter(|vote| *vote == eth1_data)
         .count();
 
     // The +1 is to account for the `eth1_data` supplied to the function.
-    if 2 * (num_votes + 1) > T::SlotsPerEth1VotingPeriod::to_usize() {
-        Some(eth1_data.clone())
+    if num_votes.safe_add(1)?.safe_mul(2)? > T::SlotsPerEth1VotingPeriod::to_usize() {
+        Ok(Some(eth1_data.clone()))
     } else {
-        None
+        Ok(None)
     }
 }
 
@@ -318,7 +319,8 @@ pub fn process_attester_slashings<T: EthSpec>(
 ) -> Result<(), BlockProcessingError> {
     // Verify the `IndexedAttestation`s in parallel (these are the resource-consuming objects, not
     // the `AttesterSlashing`s themselves).
-    let mut indexed_attestations: Vec<&_> = Vec::with_capacity(attester_slashings.len() * 2);
+    let mut indexed_attestations: Vec<&_> =
+        Vec::with_capacity(attester_slashings.len().safe_mul(2)?);
     for attester_slashing in attester_slashings {
         indexed_attestations.push(&attester_slashing.attestation_1);
         indexed_attestations.push(&attester_slashing.attestation_2);
@@ -432,8 +434,13 @@ pub fn process_deposits<T: EthSpec>(
         .par_iter()
         .enumerate()
         .try_for_each(|(i, deposit)| {
-            verify_deposit_merkle_proof(state, deposit, state.eth1_deposit_index + i as u64, spec)
-                .map_err(|e| e.into_with_index(i))
+            verify_deposit_merkle_proof(
+                state,
+                deposit,
+                state.eth1_deposit_index.safe_add(i as u64)?,
+                spec,
+            )
+            .map_err(|e| e.into_with_index(i))
         })?;
 
     // Update the state in series.
@@ -459,7 +466,7 @@ pub fn process_deposit<T: EthSpec>(
             .map_err(|e| e.into_with_index(deposit_index))?;
     }
 
-    state.eth1_deposit_index += 1;
+    state.eth1_deposit_index.increment()?;
 
     // Ensure the state's pubkey cache is fully up-to-date, it will be used to check to see if the
     // depositing validator already exists in the registry.
@@ -495,7 +502,7 @@ pub fn process_deposit<T: EthSpec>(
             exit_epoch: spec.far_future_epoch,
             withdrawable_epoch: spec.far_future_epoch,
             effective_balance: std::cmp::min(
-                amount - amount % spec.effective_balance_increment,
+                amount.safe_sub(amount.safe_rem(spec.effective_balance_increment)?)?,
                 spec.max_effective_balance,
             ),
             slashed: false,

eth2/state_processing/src/per_block_processing/block_signature_verifier.rs

@@ -1,3 +1,5 @@
+#![allow(clippy::integer_arithmetic)]
+
 use super::signature_sets::{Error as SignatureSetError, Result as SignatureSetResult, *};
 
 use crate::common::get_indexed_attestation;