From 0a0deb73e38fc2d10ef45c76f150314a6543db25 Mon Sep 17 00:00:00 2001 From: ethDreamer <37123614+ethDreamer@users.noreply.github.com> Date: Tue, 28 Sep 2021 18:36:03 -0500 Subject: [PATCH] Finished Gossip Block Validation Conditions (#2640) * Gossip Block Validation is Much More Efficient Co-authored-by: realbigsean --- .../beacon_chain/src/block_verification.rs | 100 ++++++++++-------- beacon_node/beacon_chain/src/errors.rs | 1 + common/slot_clock/src/lib.rs | 13 +++ common/slot_clock/src/manual_slot_clock.rs | 4 + .../slot_clock/src/system_time_slot_clock.rs | 4 + 5 files changed, 80 insertions(+), 42 deletions(-) diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 9c933d0210..91ba04d8e1 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/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 From for BlockError { } } +impl From for BlockError { + 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 { @@ -729,17 +730,8 @@ impl GossipVerifiedBlock { }); } - // 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( - block: BeaconBlockRef<'_, E>, - state: &BeaconState, -) -> Result<(), BlockError> { - if !is_execution_enabled(state, block.body()) { - return Ok(()); - } - let execution_payload = block - .body() - .execution_payload() - // TODO: this really should never error so maybe - // we should make this simpler.. - .ok_or_else(|| { - BlockError::InconsistentFork(InconsistentFork { - fork_at_slot: eth2::types::ForkName::Merge, - object_fork: block.body().fork_name(), - }) - })?; +fn validate_execution_payload( + parent_block: &ProtoBlock, + block: BeaconBlockRef<'_, T::EthSpec>, + chain: &BeaconChain, +) -> Result<(), BlockError> { + // Only apply this validation if this is a merge beacon block. + if let Some(execution_payload) = block.body().execution_payload() { + // This logic should match `is_execution_enabled`. We use only the execution block hash of + // the parent here in order to avoid loading the parent state during gossip verification. + let is_merge_complete = parent_block.execution_block_hash != Hash256::zero(); + let is_merge_block = + !is_merge_complete && *execution_payload != >::default(); + if !is_merge_block && !is_merge_complete { + return Ok(()); + } - if is_merge_complete(state) && *execution_payload == >::default() { - return Err(BlockError::ExecutionPayloadError( - ExecutionPayloadError::PayloadEmpty, - )); + let expected_timestamp = chain + .slot_clock + .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(()) } diff --git a/beacon_node/beacon_chain/src/errors.rs b/beacon_node/beacon_chain/src/errors.rs index 65b07d87f1..972e701815 100644 --- a/beacon_node/beacon_chain/src/errors.rs +++ b/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, diff --git a/common/slot_clock/src/lib.rs b/common/slot_clock/src/lib.rs index 2d14abb55a..18b7fd322b 100644 --- a/common/slot_clock/src/lib.rs +++ b/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 { 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 { + 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)) + } } diff --git a/common/slot_clock/src/manual_slot_clock.rs b/common/slot_clock/src/manual_slot_clock.rs index 567a6b4cd9..296247fe93 100644 --- a/common/slot_clock/src/manual_slot_clock.rs +++ b/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)] diff --git a/common/slot_clock/src/system_time_slot_clock.rs b/common/slot_clock/src/system_time_slot_clock.rs index c5d6dedc9b..c54646fbc6 100644 --- a/common/slot_clock/src/system_time_slot_clock.rs +++ b/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)]