Use checked arithmetic in types and state proc

Author: michaelsproul

.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.lock

@@ -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",

Cargo.toml

@@ -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 allow 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

Makefile

@@ -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/Cargo.toml

@@ -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/beacon_chain/src/eth1_chain.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!(

beacon_node/genesis/src/eth1_genesis_service.rs

@@ -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/Cargo.toml

@@ -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/deposit_data_tree.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)?)
     }
 }