Finished Gossip Block Validation Conditions (#2640)

* Gossip Block Validation is Much More Efficient

Co-authored-by: realbigsean <seananderson33@gmail.com>

beacon_node/beacon_chain/src/block_verification.rs

@@ -53,10 +53,11 @@ use crate::{
 use fork_choice::{ForkChoice, ForkChoiceStore};
 use parking_lot::RwLockReadGuard;
 use proto_array::Block as ProtoBlock;
+use safe_arith::ArithError;
 use slog::{debug, error, Logger};
 use slot_clock::SlotClock;
 use ssz::Encode;
-use state_processing::per_block_processing::{is_execution_enabled, is_merge_complete};
+use state_processing::per_block_processing::is_execution_enabled;
 use state_processing::{
     block_signature_verifier::{BlockSignatureVerifier, Error as BlockSignatureVerifierError},
     per_block_processing, per_slot_processing,
@@ -254,18 +255,12 @@ pub enum ExecutionPayloadError {
     ///
     /// The block is invalid and the peer is faulty
     RejectedByExecutionEngine,
-    /// The execution payload is empty when is shouldn't be
-    ///
-    /// ## Peer scoring
-    ///
-    /// The block is invalid and the peer is faulty
-    PayloadEmpty,
     /// The execution payload timestamp does not match the slot
     ///
     /// ## Peer scoring
     ///
     /// The block is invalid and the peer is faulty
-    InvalidPayloadTimestamp,
+    InvalidPayloadTimestamp { expected: u64, found: u64 },
     /// The gas used in the block exceeds the gas limit
     ///
     /// ## Peer scoring
@@ -338,6 +333,12 @@ impl<T: EthSpec> From<DBError> for BlockError<T> {
     }
 }
 
+impl<T: EthSpec> From<ArithError> for BlockError<T> {
+    fn from(e: ArithError) -> Self {
+        BlockError::BeaconChainError(BeaconChainError::ArithError(e))
+    }
+}
+
 /// Information about invalid blocks which might still be slashable despite being invalid.
 #[allow(clippy::enum_variant_names)]
 pub enum BlockSlashInfo<TErr> {
@@ -729,17 +730,8 @@ impl<T: BeaconChainTypes> GossipVerifiedBlock<T> {
             });
         }
 
-        // TODO: avoid this by adding field to fork-choice to determine if merge-block has been imported
-        let (parent, block) = if let Some(snapshot) = parent {
-            (Some(snapshot), block)
-        } else {
-            let (snapshot, block) = load_parent(block, chain)?;
-            (Some(snapshot), block)
-        };
-        let state = &parent.as_ref().unwrap().pre_state;
-
         // validate the block's execution_payload
-        validate_execution_payload(block.message(), state)?;
+        validate_execution_payload(&parent_block, block.message(), chain)?;
 
         Ok(Self {
             block,
@@ -1201,33 +1193,57 @@ impl<'a, T: BeaconChainTypes> FullyVerifiedBlock<'a, T> {
 
 /// Validate the gossip block's execution_payload according to the checks described here:
 /// https://github.com/ethereum/consensus-specs/blob/dev/specs/merge/p2p-interface.md#beacon_block
-fn validate_execution_payload<E: EthSpec>(
+fn validate_execution_payload<T: BeaconChainTypes>(
-    block: BeaconBlockRef<'_, E>,
+    parent_block: &ProtoBlock,
-    state: &BeaconState<E>,
+    block: BeaconBlockRef<'_, T::EthSpec>,
-) -> Result<(), BlockError<E>> {
+    chain: &BeaconChain<T>,
-    if !is_execution_enabled(state, block.body()) {
+) -> Result<(), BlockError<T::EthSpec>> {
-        return Ok(());
+    // Only apply this validation if this is a merge beacon block.
-    }
+    if let Some(execution_payload) = block.body().execution_payload() {
-    let execution_payload = block
+        // This logic should match `is_execution_enabled`. We use only the execution block hash of
-        .body()
+        // the parent here in order to avoid loading the parent state during gossip verification.
-        .execution_payload()
+        let is_merge_complete = parent_block.execution_block_hash != Hash256::zero();
-        // TODO: this really should never error so maybe
+        let is_merge_block =
-        //       we should make this simpler..
+            !is_merge_complete && *execution_payload != <ExecutionPayload<T::EthSpec>>::default();
-        .ok_or_else(|| {
+        if !is_merge_block && !is_merge_complete {
-            BlockError::InconsistentFork(InconsistentFork {
+            return Ok(());
-                fork_at_slot: eth2::types::ForkName::Merge,
+        }
-                object_fork: block.body().fork_name(),
-            })
-        })?;
 
-    if is_merge_complete(state) && *execution_payload == <ExecutionPayload<E>>::default() {
+        let expected_timestamp = chain
-        return Err(BlockError::ExecutionPayloadError(
+            .slot_clock
-            ExecutionPayloadError::PayloadEmpty,
+            .compute_timestamp_at_slot(block.slot())
-        ));
+            .ok_or(BlockError::BeaconChainError(
+                BeaconChainError::UnableToComputeTimeAtSlot,
+            ))?;
+        // The block's execution payload timestamp is correct with respect to the slot
+        if execution_payload.timestamp != expected_timestamp {
+            return Err(BlockError::ExecutionPayloadError(
+                ExecutionPayloadError::InvalidPayloadTimestamp {
+                    expected: expected_timestamp,
+                    found: execution_payload.timestamp,
+                },
+            ));
+        }
+        // Gas used is less than the gas limit
+        if execution_payload.gas_used > execution_payload.gas_limit {
+            return Err(BlockError::ExecutionPayloadError(
+                ExecutionPayloadError::GasUsedExceedsLimit,
+            ));
+        }
+        // The execution payload block hash is not equal to the parent hash
+        if execution_payload.block_hash == execution_payload.parent_hash {
+            return Err(BlockError::ExecutionPayloadError(
+                ExecutionPayloadError::BlockHashEqualsParentHash,
+            ));
+        }
+        // The execution payload transaction list data is within expected size limits
+        if execution_payload.transactions.len() > T::EthSpec::max_transactions_per_payload() {
+            return Err(BlockError::ExecutionPayloadError(
+                ExecutionPayloadError::TransactionDataExceedsSizeLimit,
+            ));
+        }
     }
 
-    // TODO: finish these
-
     Ok(())
 }
 

beacon_node/beacon_chain/src/errors.rs

@@ -40,6 +40,7 @@ macro_rules! easy_from_to {
 pub enum BeaconChainError {
     InsufficientValidators,
     UnableToReadSlot,
+    UnableToComputeTimeAtSlot,
     RevertedFinalizedEpoch {
         previous_epoch: Epoch,
         new_epoch: Epoch,

common/slot_clock/src/lib.rs

@@ -65,6 +65,9 @@ pub trait SlotClock: Send + Sync + Sized + Clone {
     /// Returns the first slot to be returned at the genesis time.
     fn genesis_slot(&self) -> Slot;
 
+    /// Returns the `Duration` from `UNIX_EPOCH` to the genesis time.
+    fn genesis_duration(&self) -> Duration;
+
     /// Returns the slot if the internal clock were advanced by `duration`.
     fn now_with_future_tolerance(&self, tolerance: Duration) -> Option<Slot> {
         self.slot_of(self.now_duration()?.checked_add(tolerance)?)
@@ -99,4 +102,14 @@ pub trait SlotClock: Send + Sync + Sized + Clone {
     fn sync_committee_contribution_production_delay(&self) -> Duration {
         self.slot_duration() * 2 / 3
     }
+
+    /// An implementation of the method described in the consensus spec here:
+    ///
+    /// https://github.com/ethereum/consensus-specs/blob/dev/specs/merge/beacon-chain.md#compute_timestamp_at_slot
+    fn compute_timestamp_at_slot(&self, slot: Slot) -> Option<u64> {
+        let slots_since_genesis = slot.as_u64().checked_sub(self.genesis_slot().as_u64())?;
+        slots_since_genesis
+            .checked_mul(self.slot_duration().as_secs())
+            .and_then(|since_genesis| self.genesis_duration().as_secs().checked_add(since_genesis))
+    }
 }

common/slot_clock/src/manual_slot_clock.rs

@@ -154,6 +154,10 @@ impl SlotClock for ManualSlotClock {
     fn genesis_slot(&self) -> Slot {
         self.genesis_slot
     }
+
+    fn genesis_duration(&self) -> Duration {
+        self.genesis_duration
+    }
 }
 
 #[cfg(test)]

common/slot_clock/src/system_time_slot_clock.rs

@@ -61,6 +61,10 @@ impl SlotClock for SystemTimeSlotClock {
     fn genesis_slot(&self) -> Slot {
         self.clock.genesis_slot()
     }
+
+    fn genesis_duration(&self) -> Duration {
+        *self.clock.genesis_duration()
+    }
 }
 
 #[cfg(test)]