Kintsugi on_merge_block tests (#2811)

* Start v1.1.5 updates

* Implement new payload creation logic

* Tidy, add comments

* Remove unused error enums

* Add validate payload for gossip

* Refactor validate_merge_block

* Split payload verification in per block processing

* Add execute_payload

* Tidy

* Tidy

* Start working on new fork choice tests

* Fix failing merge block test

* Skip block_lookup_failed test

* Fix failing terminal block test

* Fixes from self-review

* Address review comments

Author: paulhauner

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -15,6 +15,7 @@ use crate::chain_config::ChainConfig;
 use crate::errors::{BeaconChainError as Error, BlockProductionError};
 use crate::eth1_chain::{Eth1Chain, Eth1ChainBackend};
 use crate::events::ServerSentEventHandler;
+use crate::execution_payload::get_execution_payload;
 use crate::head_tracker::HeadTracker;
 use crate::historical_blocks::HistoricalBlockError;
 use crate::migrate::BackgroundMigrator;
@@ -65,9 +66,7 @@ use ssz::Encode;
 use state_processing::{
     common::get_indexed_attestation,
     per_block_processing,
-    per_block_processing::{
-        compute_timestamp_at_slot, errors::AttestationValidationError, is_merge_complete,
-    },
+    per_block_processing::{errors::AttestationValidationError, is_merge_complete},
     per_slot_processing,
     state_advance::{complete_state_advance, partial_state_advance},
     BlockSignatureStrategy, SigVerifiedOp,
@@ -2881,63 +2880,6 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
                     SyncAggregate::new()
                 }))
         };
-        // Closure to fetch a sync aggregate in cases where it is required.
-        let get_execution_payload = |latest_execution_payload_header: &ExecutionPayloadHeader<
-            T::EthSpec,
-        >|
-         -> Result<ExecutionPayload<_>, BlockProductionError> {
-            let execution_layer = self
-                .execution_layer
-                .as_ref()
-                .ok_or(BlockProductionError::ExecutionLayerMissing)?;
-
-            let parent_hash;
-            if !is_merge_complete(&state) {
-                let terminal_pow_block_hash = execution_layer
-                    .block_on(|execution_layer| {
-                        execution_layer.get_terminal_pow_block_hash(&self.spec)
-                    })
-                    .map_err(BlockProductionError::TerminalPoWBlockLookupFailed)?;
-
-                if let Some(terminal_pow_block_hash) = terminal_pow_block_hash {
-                    parent_hash = terminal_pow_block_hash;
-                } else {
-                    return Ok(<_>::default());
-                }
-            } else {
-                parent_hash = latest_execution_payload_header.block_hash;
-            }
-
-            let timestamp =
-                compute_timestamp_at_slot(&state, &self.spec).map_err(BeaconStateError::from)?;
-            let random = *state.get_randao_mix(state.current_epoch())?;
-            let finalized_root = state.finalized_checkpoint().root;
-
-            let finalized_block_hash =
-                if let Some(block) = self.fork_choice.read().get_block(&finalized_root) {
-                    block.execution_status.block_hash()
-                } else {
-                    self.store
-                        .get_block(&finalized_root)
-                        .map_err(BlockProductionError::FailedToReadFinalizedBlock)?
-                        .ok_or(BlockProductionError::MissingFinalizedBlock(finalized_root))?
-                        .message()
-                        .body()
-                        .execution_payload()
-                        .map(|ep| ep.block_hash)
-                };
-
-            execution_layer
-                .block_on(|execution_layer| {
-                    execution_layer.get_payload(
-                        parent_hash,
-                        timestamp,
-                        random,
-                        finalized_block_hash.unwrap_or_else(Hash256::zero),
-                    )
-                })
-                .map_err(BlockProductionError::GetPayloadFailed)
-        };
 
         let inner_block = match &state {
             BeaconState::Base(_) => BeaconBlock::Base(BeaconBlockBase {
@@ -2976,10 +2918,9 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
                     },
                 })
             }
-            BeaconState::Merge(state) => {
+            BeaconState::Merge(_) => {
                 let sync_aggregate = get_sync_aggregate()?;
-                let execution_payload =
-                    get_execution_payload(&state.latest_execution_payload_header)?;
+                let execution_payload = get_execution_payload(self, &state)?;
                 BeaconBlock::Merge(BeaconBlockMerge {
                     slot,
                     proposer_index,

beacon_node/beacon_chain/src/block_verification.rs

@@ -40,6 +40,9 @@
 //!            END
 //!
 //! ```
+use crate::execution_payload::{
+    execute_payload, validate_execution_payload_for_gossip, validate_merge_block,
+};
 use crate::snapshot_cache::PreProcessingSnapshot;
 use crate::validator_monitor::HISTORIC_EPOCHS as VALIDATOR_MONITOR_HISTORIC_EPOCHS;
 use crate::validator_pubkey_cache::ValidatorPubkeyCache;
@@ -50,15 +53,14 @@ use crate::{
     },
     metrics, BeaconChain, BeaconChainError, BeaconChainTypes,
 };
-use execution_layer::ExecutePayloadResponseStatus;
 use fork_choice::{ForkChoice, ForkChoiceStore, PayloadVerificationStatus};
 use parking_lot::RwLockReadGuard;
-use proto_array::{Block as ProtoBlock, ExecutionStatus};
+use proto_array::Block as ProtoBlock;
 use safe_arith::ArithError;
-use slog::{debug, error, info, Logger};
+use slog::{debug, error, Logger};
 use slot_clock::SlotClock;
 use ssz::Encode;
-use state_processing::per_block_processing::{is_execution_enabled, is_merge_block};
+use state_processing::per_block_processing::is_merge_block;
 use state_processing::{
     block_signature_verifier::{BlockSignatureVerifier, Error as BlockSignatureVerifierError},
     per_block_processing, per_slot_processing,
@@ -71,9 +73,9 @@ use std::io::Write;
 use store::{Error as DBError, HotColdDB, HotStateSummary, KeyValueStore, StoreOp};
 use tree_hash::TreeHash;
 use types::{
-    BeaconBlockRef, BeaconState, BeaconStateError, ChainSpec, CloneConfig, Epoch, EthSpec,
-    ExecutionPayload, Hash256, InconsistentFork, PublicKey, PublicKeyBytes, RelativeEpoch,
-    SignedBeaconBlock, SignedBeaconBlockHeader, Slot,
+    BeaconBlockRef, BeaconState, BeaconStateError, ChainSpec, CloneConfig, Epoch, EthSpec, Hash256,
+    InconsistentFork, PublicKey, PublicKeyBytes, RelativeEpoch, SignedBeaconBlock,
+    SignedBeaconBlockHeader, Slot,
 };
 
 /// Maximum block slot number. Block with slots bigger than this constant will NOT be processed.
@@ -266,38 +268,47 @@ pub enum ExecutionPayloadError {
     ///
     /// The block is invalid and the peer is faulty
     RejectedByExecutionEngine,
-    /// The execution engine returned SYNCING for the payload
-    ///
-    /// ## Peer scoring
-    ///
-    /// It is not known if the block is valid or invalid.
-    ExecutionEngineIsSyncing,
     /// The execution payload timestamp does not match the slot
     ///
     /// ## Peer scoring
     ///
     /// The block is invalid and the peer is faulty
     InvalidPayloadTimestamp { expected: u64, found: u64 },
-    /// The execution payload transaction list data exceeds size limits
+    /// The execution payload references an execution block that cannot trigger the merge.
     ///
     /// ## Peer scoring
     ///
-    /// The block is invalid and the peer is faulty
-    TransactionDataExceedsSizeLimit,
-    /// The execution payload references an execution block that cannot trigger the merge.
+    /// The block is invalid and the peer sent us a block that passes gossip propagation conditions,
+    /// but is invalid upon further verification.
+    InvalidTerminalPoWBlock { parent_hash: Hash256 },
+    /// The `TERMINAL_BLOCK_HASH` is set, but the block has not reached the
+    /// `TERMINAL_BLOCK_HASH_ACTIVATION_EPOCH`.
     ///
     /// ## Peer scoring
     ///
     /// The block is invalid and the peer sent us a block that passes gossip propagation conditions,
     /// but is invalid upon further verification.
-    InvalidTerminalPoWBlock,
-    /// The execution payload references execution blocks that are unavailable on our execution
-    /// nodes.
+    InvalidActivationEpoch {
+        activation_epoch: Epoch,
+        epoch: Epoch,
+    },
+    /// The `TERMINAL_BLOCK_HASH` is set, but does not match the value specified by the block.
     ///
     /// ## Peer scoring
     ///
-    /// It's not clear if the peer is invalid or if it's on a different execution fork to us.
-    TerminalPoWBlockNotFound,
+    /// The block is invalid and the peer sent us a block that passes gossip propagation conditions,
+    /// but is invalid upon further verification.
+    InvalidTerminalBlockHash {
+        terminal_block_hash: Hash256,
+        payload_parent_hash: Hash256,
+    },
+    /// The execution node failed to provide a parent block to a known block. This indicates an
+    /// issue with the execution node.
+    ///
+    /// ## Peer scoring
+    ///
+    /// The peer is not necessarily invalid.
+    PoWParentMissing(Hash256),
 }
 
 impl From<execution_layer::Error> for ExecutionPayloadError {
@@ -768,8 +779,8 @@ impl<T: BeaconChainTypes> GossipVerifiedBlock<T> {
             });
         }
 
-        // validate the block's execution_payload
-        validate_execution_payload(&parent_block, block.message(), chain)?;
+        // Validate the block's execution_payload (if any).
+        validate_execution_payload_for_gossip(&parent_block, block.message(), chain)?;
 
         Ok(Self {
             block,
@@ -1103,83 +1114,16 @@ impl<'a, T: BeaconChainTypes> FullyVerifiedBlock<'a, T> {
         // - Doing the check here means we can keep our fork-choice implementation "pure". I.e., no
         //   calls to remote servers.
         if is_merge_block(&state, block.message().body()) {
-            let execution_layer = chain
-                .execution_layer
-                .as_ref()
-                .ok_or(ExecutionPayloadError::NoExecutionConnection)?;
-            let execution_payload =
-                block
-                    .message()
-                    .body()
-                    .execution_payload()
-                    .ok_or_else(|| InconsistentFork {
-                        fork_at_slot: eth2::types::ForkName::Merge,
-                        object_fork: block.message().body().fork_name(),
-                    })?;
-
-            let is_valid_terminal_pow_block = execution_layer
-                .block_on(|execution_layer| {
-                    execution_layer.is_valid_terminal_pow_block_hash(
-                        execution_payload.parent_hash,
-                        &chain.spec,
-                    )
-                })
-                .map_err(ExecutionPayloadError::from)?;
-
-            match is_valid_terminal_pow_block {
-                Some(true) => Ok(()),
-                Some(false) => Err(ExecutionPayloadError::InvalidTerminalPoWBlock),
-                None => {
-                    info!(
-                        chain.log,
-                        "Optimistically accepting terminal block";
-                        "block_hash" => ?execution_payload.parent_hash,
-                        "msg" => "the terminal block/parent was unavailable"
-                    );
-                    Ok(())
-                }
-            }?;
+            validate_merge_block(chain, block.message())?
         }
 
-        // This is the soonest we can run these checks as they must be called AFTER per_slot_processing
+        // The specification declares that this should be run *inside* `per_block_processing`,
+        // however we run it here to keep `per_block_processing` pure (i.e., no calls to external
+        // servers).
         //
-        // TODO(merge): handle the latest_valid_hash of an invalid payload.
-        let (_latest_valid_hash, payload_verification_status) =
-            if is_execution_enabled(&state, block.message().body()) {
-                let execution_layer = chain
-                    .execution_layer
-                    .as_ref()
-                    .ok_or(ExecutionPayloadError::NoExecutionConnection)?;
-                let execution_payload =
-                    block
-                        .message()
-                        .body()
-                        .execution_payload()
-                        .ok_or_else(|| InconsistentFork {
-                            fork_at_slot: eth2::types::ForkName::Merge,
-                            object_fork: block.message().body().fork_name(),
-                        })?;
-
-                let execute_payload_response = execution_layer
-                    .block_on(|execution_layer| execution_layer.execute_payload(execution_payload));
-
-                match execute_payload_response {
-                    Ok((status, latest_valid_hash)) => match status {
-                        ExecutePayloadResponseStatus::Valid => {
-                            (latest_valid_hash, PayloadVerificationStatus::Verified)
-                        }
-                        ExecutePayloadResponseStatus::Invalid => {
-                            return Err(ExecutionPayloadError::RejectedByExecutionEngine.into());
-                        }
-                        ExecutePayloadResponseStatus::Syncing => {
-                            (latest_valid_hash, PayloadVerificationStatus::NotVerified)
-                        }
-                    },
-                    Err(_) => (None, PayloadVerificationStatus::NotVerified),
-                }
-            } else {
-                (None, PayloadVerificationStatus::Irrelevant)
-            };
+        // It is important that this function is called *after* `per_slot_processing`, since the
+        // `randao` may change.
+        let payload_verification_status = execute_payload(chain, &state, block.message())?;
 
         // If the block is sufficiently recent, notify the validator monitor.
         if let Some(slot) = chain.slot_clock.now() {
@@ -1290,64 +1234,6 @@ 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<T: BeaconChainTypes>(
-    parent_block: &ProtoBlock,
-    block: BeaconBlockRef<'_, T::EthSpec>,
-    chain: &BeaconChain<T>,
-) -> Result<(), BlockError<T::EthSpec>> {
-    // 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 = match parent_block.execution_status {
-            // Optimistically declare that an "unknown" status block has completed the merge.
-            ExecutionStatus::Valid(_) | ExecutionStatus::Unknown(_) => true,
-            // It's impossible for an irrelevant block to have completed the merge. It is pre-merge
-            // by definition.
-            ExecutionStatus::Irrelevant(_) => false,
-            // If the parent has an invalid payload then it's impossible to build a valid block upon
-            // it. Reject the block.
-            ExecutionStatus::Invalid(_) => {
-                return Err(BlockError::ParentExecutionPayloadInvalid {
-                    parent_root: parent_block.root,
-                })
-            }
-        };
-        let is_merge_block =
-            !is_merge_complete && *execution_payload != <ExecutionPayload<T::EthSpec>>::default();
-        if !is_merge_block && !is_merge_complete {
-            return Ok(());
-        }
-
-        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,
-                },
-            ));
-        }
-        // 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,
-            ));
-        }
-    }
-
-    Ok(())
-}
-
 /// Check that the count of skip slots between the block and its parent does not exceed our maximum
 /// value.
 ///

beacon_node/beacon_chain/src/errors.rs

@@ -181,6 +181,7 @@ pub enum BlockProductionError {
         state_slot: Slot,
     },
     ExecutionLayerMissing,
+    BlockingFailed(execution_layer::Error),
     TerminalPoWBlockLookupFailed(execution_layer::Error),
     GetPayloadFailed(execution_layer::Error),
     FailedToReadFinalizedBlock(store::Error),