Fix clippy lints on merge-f2f (#2626)

* Remove unchecked arith from ssz_derive * Address clippy lints in block_verfication * Use safe math for is_valid_gas_limit
sigp · Oct 1, 2021 · 82a1780 · 82a1780
1 parent 6da6453
commit 82a1780
Show file tree

Hide file tree

Showing 2 changed files with 35 additions and 26 deletions.
diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs
@@ -1071,19 +1071,20 @@ impl<'a, T: BeaconChainTypes> FullyVerifiedBlock<'a, T> {
                     ExecutionPayloadError::NoEth1Connection,
                 ))?;
 
-            if !eth1_chain
-                .on_payload(block.message().body().execution_payload().ok_or(
+            let payload_valid = eth1_chain
+                .on_payload(block.message().body().execution_payload().ok_or_else(|| {
                     BlockError::InconsistentFork(InconsistentFork {
                         fork_at_slot: eth2::types::ForkName::Merge,
                         object_fork: block.message().body().fork_name(),
-                    }),
-                )?)
+                    })
+                })?)
                 .map_err(|e| {
                     BlockError::ExecutionPayloadError(ExecutionPayloadError::Eth1VerificationError(
                         e,
                     ))
-                })?
-            {
+                })?;
+
+            if !payload_valid {
                 return Err(BlockError::ExecutionPayloadError(
                     ExecutionPayloadError::RejectedByExecutionEngine,
                 ));
@@ -1212,17 +1213,17 @@ fn validate_execution_payload<E: EthSpec>(
         .execution_payload()
         // TODO: this really should never error so maybe
         //       we should make this simpler..
-        .ok_or(BlockError::InconsistentFork(InconsistentFork {
-            fork_at_slot: eth2::types::ForkName::Merge,
-            object_fork: block.body().fork_name(),
-        }))?;
-
-    if is_merge_complete(state) {
-        if *execution_payload == <ExecutionPayload<E>>::default() {
-            return Err(BlockError::ExecutionPayloadError(
-                ExecutionPayloadError::PayloadEmpty,
-            ));
-        }
+        .ok_or_else(|| {
+            BlockError::InconsistentFork(InconsistentFork {
+                fork_at_slot: eth2::types::ForkName::Merge,
+                object_fork: block.body().fork_name(),
+            })
+        })?;
+
+    if is_merge_complete(state) && *execution_payload == <ExecutionPayload<E>>::default() {
+        return Err(BlockError::ExecutionPayloadError(
+            ExecutionPayloadError::PayloadEmpty,
+        ));
     }
 
     // TODO: finish these

diff --git a/consensus/state_processing/src/per_block_processing.rs b/consensus/state_processing/src/per_block_processing.rs
@@ -288,24 +288,32 @@ pub fn get_new_eth1_data<T: EthSpec>(
 pub fn is_valid_gas_limit<T: EthSpec>(
     payload: &ExecutionPayload<T>,
     parent: &ExecutionPayloadHeader<T>,
-) -> bool {
+) -> Result<bool, ArithError> {
     // check if payload used too much gas
     if payload.gas_used > payload.gas_limit {
-        return false;
+        return Ok(false);
     }
     // check if payload changed the gas limit too much
-    if payload.gas_limit >= parent.gas_limit + parent.gas_limit / T::gas_limit_denominator() {
-        return false;
+    if payload.gas_limit
+        >= parent
+            .gas_limit
+            .safe_add(parent.gas_limit.safe_div(T::gas_limit_denominator())?)?
+    {
+        return Ok(false);
     }
-    if payload.gas_limit <= parent.gas_limit - parent.gas_limit / T::gas_limit_denominator() {
-        return false;
+    if payload.gas_limit
+        <= parent
+            .gas_limit
+            .safe_sub(parent.gas_limit.safe_div(T::gas_limit_denominator())?)?
+    {
+        return Ok(false);
     }
     // check if the gas limit is at least the minimum gas limit
     if payload.gas_limit < T::min_gas_limit() {
-        return false;
+        return Ok(false);
     }
 
-    return true;
+    Ok(true)
 }
 
 /// https://github.com/ethereum/consensus-specs/blob/dev/specs/merge/beacon-chain.md#process_execution_payload
@@ -344,7 +352,7 @@ pub fn process_execution_payload<T: EthSpec>(
             }
         );
         block_verify!(
-            is_valid_gas_limit(payload, state.latest_execution_payload_header()?),
+            is_valid_gas_limit(payload, state.latest_execution_payload_header()?)?,
             BlockProcessingError::ExecutionInvalidGasLimit {
                 used: payload.gas_used,
                 limit: payload.gas_limit,