[Merged by Bors] - Retrospective invalidation of exec. payloads for opt. sync

Makefile

@@ -15,7 +15,7 @@ PINNED_NIGHTLY ?= nightly
 
 # List of all hard forks. This list is used to set env variables for several tests so that
 # they run for different forks.
-FORKS=phase0 altair
+FORKS=phase0 altair merge
 
 # Builds the Lighthouse binary in release (optimized).
 #

beacon_node/beacon_chain/src/block_verification.rs

@@ -54,7 +54,7 @@ use crate::{
     metrics, BeaconChain, BeaconChainError, BeaconChainTypes,
 };
 use eth2::types::EventKind;
-use execution_layer::PayloadStatusV1Status;
+use execution_layer::PayloadStatus;
 use fork_choice::{ForkChoice, ForkChoiceStore, PayloadVerificationStatus};
 use parking_lot::RwLockReadGuard;
 use proto_array::Block as ProtoBlock;
@@ -76,9 +76,9 @@ use std::time::Duration;
 use store::{Error as DBError, HotColdDB, HotStateSummary, KeyValueStore, StoreOp};
 use tree_hash::TreeHash;
 use types::{
-    BeaconBlockRef, BeaconState, BeaconStateError, ChainSpec, CloneConfig, Epoch, EthSpec, Hash256,
-    InconsistentFork, PublicKey, PublicKeyBytes, RelativeEpoch, SignedBeaconBlock,
-    SignedBeaconBlockHeader, Slot,
+    BeaconBlockRef, BeaconState, BeaconStateError, ChainSpec, CloneConfig, Epoch, EthSpec,
+    ExecutionBlockHash, Hash256, InconsistentFork, PublicKey, PublicKeyBytes, RelativeEpoch,
+    SignedBeaconBlock, SignedBeaconBlockHeader, Slot,
 };
 
 /// Maximum block slot number. Block with slots bigger than this constant will NOT be processed.
@@ -270,10 +270,7 @@ pub enum ExecutionPayloadError {
     /// ## Peer scoring
     ///
     /// The block is invalid and the peer is faulty
-    RejectedByExecutionEngine {
-        status: PayloadStatusV1Status,
-        latest_valid_hash: Option<Vec<Hash256>>,
-    },
+    RejectedByExecutionEngine { status: PayloadStatus },
     /// The execution payload timestamp does not match the slot
     ///
     /// ## Peer scoring
@@ -286,7 +283,7 @@ pub enum ExecutionPayloadError {
     ///
     /// 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 },
+    InvalidTerminalPoWBlock { parent_hash: ExecutionBlockHash },
     /// The `TERMINAL_BLOCK_HASH` is set, but the block has not reached the
     /// `TERMINAL_BLOCK_HASH_ACTIVATION_EPOCH`.
     ///
@@ -305,16 +302,16 @@ pub enum ExecutionPayloadError {
     /// 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,
+        terminal_block_hash: ExecutionBlockHash,
+        payload_parent_hash: ExecutionBlockHash,
     },
     /// 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),
+    PoWParentMissing(ExecutionBlockHash),
 }
 
 impl From<execution_layer::Error> for ExecutionPayloadError {

beacon_node/beacon_chain/src/errors.rs

@@ -8,7 +8,7 @@ use crate::naive_aggregation_pool::Error as NaiveAggregationError;
 use crate::observed_aggregates::Error as ObservedAttestationsError;
 use crate::observed_attesters::Error as ObservedAttestersError;
 use crate::observed_block_producers::Error as ObservedBlockProducersError;
-use execution_layer::PayloadStatusV1Status;
+use execution_layer::PayloadStatus;
 use futures::channel::mpsc::TrySendError;
 use operation_pool::OpPoolError;
 use safe_arith::ArithError;
@@ -139,15 +139,27 @@ pub enum BeaconChainError {
     ExecutionLayerMissing,
     ExecutionForkChoiceUpdateFailed(execution_layer::Error),
     ExecutionForkChoiceUpdateInvalid {
-        status: PayloadStatusV1Status,
-        latest_valid_hash: Option<Vec<Hash256>>,
+        status: PayloadStatus,
     },
     BlockRewardSlotError,
     BlockRewardAttestationError,
     BlockRewardSyncError,
     HeadMissingFromForkChoice(Hash256),
     FinalizedBlockMissingFromForkChoice(Hash256),
+    InvalidFinalizedPayload {
+        finalized_root: Hash256,
+        execution_block_hash: ExecutionBlockHash,
+    },
     InvalidFinalizedPayloadShutdownError(TrySendError<ShutdownReason>),
+    JustifiedPayloadInvalid {
+        justified_root: Hash256,
+        execution_block_hash: Option<ExecutionBlockHash>,
+    },
+    ForkchoiceUpdate(execution_layer::Error),
+    FinalizedCheckpointMismatch {
+        head_state: Checkpoint,
+        fork_choice: Hash256,
+    },
 }
 
 easy_from_to!(SlotProcessingError, BeaconChainError);

beacon_node/beacon_chain/src/execution_payload.rs

@@ -11,7 +11,7 @@ use crate::{
     BeaconChain, BeaconChainError, BeaconChainTypes, BlockError, BlockProductionError,
     ExecutionPayloadError,
 };
-use execution_layer::PayloadStatusV1Status;
+use execution_layer::PayloadStatus;
 use fork_choice::PayloadVerificationStatus;
 use proto_array::{Block as ProtoBlock, ExecutionStatus};
 use slog::debug;
@@ -57,22 +57,26 @@ pub fn notify_new_payload<T: BeaconChainTypes>(
         .block_on(|execution_layer| execution_layer.notify_new_payload(execution_payload));
 
     match new_payload_response {
-        Ok((status, latest_valid_hash)) => match status {
-            PayloadStatusV1Status::Valid => Ok(PayloadVerificationStatus::Verified),
-            PayloadStatusV1Status::Syncing | PayloadStatusV1Status::Accepted => {
+        Ok(status) => match status {
+            PayloadStatus::Valid => Ok(PayloadVerificationStatus::Verified),
+            PayloadStatus::Syncing | PayloadStatus::Accepted => {
                 Ok(PayloadVerificationStatus::NotVerified)
             }
-            PayloadStatusV1Status::Invalid
-            | PayloadStatusV1Status::InvalidTerminalBlock
-            | PayloadStatusV1Status::InvalidBlockHash => {
-                // TODO(bellatrix): process the invalid payload.
-                //
-                // See: https://github.com/sigp/lighthouse/pull/2837
-                Err(ExecutionPayloadError::RejectedByExecutionEngine {
-                    status,
-                    latest_valid_hash,
-                }
-                .into())
+            PayloadStatus::Invalid {
+                latest_valid_hash, ..
+            } => {
+                // This block has not yet been applied to fork choice, so the latest block that was
+                // imported to fork choice was the parent.
+                let latest_root = block.parent_root();
+                chain.process_invalid_execution_payload(latest_root, Some(latest_valid_hash))?;
+
+                Err(ExecutionPayloadError::RejectedByExecutionEngine { status }.into())
+            }
+            PayloadStatus::InvalidTerminalBlock { .. } | PayloadStatus::InvalidBlockHash { .. } => {
+                // Returning an error here should be sufficient to invalidate the block. We have no
+                // information to indicate its parent is invalid, so no need to run
+                // `BeaconChain::process_invalid_execution_payload`.
+                Err(ExecutionPayloadError::RejectedByExecutionEngine { status }.into())
             }
         },
         Err(e) => Err(ExecutionPayloadError::RequestFailed(e).into()),
@@ -99,7 +103,7 @@ pub fn validate_merge_block<T: BeaconChainTypes>(
     let block_epoch = block.slot().epoch(T::EthSpec::slots_per_epoch());
     let execution_payload = block.execution_payload()?;
 
-    if spec.terminal_block_hash != Hash256::zero() {
+    if spec.terminal_block_hash != ExecutionBlockHash::zero() {
         if block_epoch < spec.terminal_block_hash_activation_epoch {
             return Err(ExecutionPayloadError::InvalidActivationEpoch {
                 activation_epoch: spec.terminal_block_hash_activation_epoch,
@@ -263,7 +267,7 @@ pub async fn prepare_execution_payload<T: BeaconChainTypes>(
         .ok_or(BlockProductionError::ExecutionLayerMissing)?;
 
     let parent_hash = if !is_merge_transition_complete(state) {
-        let is_terminal_block_hash_set = spec.terminal_block_hash != Hash256::zero();
+        let is_terminal_block_hash_set = spec.terminal_block_hash != ExecutionBlockHash::zero();
         let is_activation_epoch_reached =
             state.current_epoch() >= spec.terminal_block_hash_activation_epoch;
 
@@ -314,7 +318,7 @@ pub async fn prepare_execution_payload<T: BeaconChainTypes>(
             parent_hash,
             timestamp,
             random,
-            finalized_block_hash.unwrap_or_else(Hash256::zero),
+            finalized_block_hash.unwrap_or_else(ExecutionBlockHash::zero),
             proposer_index,
         )
         .await

beacon_node/beacon_chain/src/lib.rs

@@ -41,7 +41,7 @@ mod validator_pubkey_cache;
 pub use self::beacon_chain::{
     AttestationProcessingOutcome, BeaconChain, BeaconChainTypes, BeaconStore, ChainSegmentResult,
     ForkChoiceError, HeadInfo, HeadSafetyStatus, StateSkipConfig, WhenSlotSkipped,
-    MAXIMUM_GOSSIP_CLOCK_DISPARITY,
+    INVALID_JUSTIFIED_PAYLOAD_SHUTDOWN_REASON, MAXIMUM_GOSSIP_CLOCK_DISPARITY,
 };
 pub use self::beacon_snapshot::BeaconSnapshot;
 pub use self::chain_config::ChainConfig;

beacon_node/beacon_chain/src/test_utils.rs

@@ -432,7 +432,7 @@ where
             spec: chain.spec.clone(),
             chain: Arc::new(chain),
             validator_keypairs,
-            shutdown_receiver,
+            shutdown_receiver: Arc::new(Mutex::new(shutdown_receiver)),
             mock_execution_layer: self.mock_execution_layer,
             execution_layer_runtime: self.execution_layer_runtime,
             rng: make_rng(),
@@ -449,7 +449,7 @@ pub struct BeaconChainHarness<T: BeaconChainTypes> {
 
     pub chain: Arc<BeaconChain<T>>,
     pub spec: ChainSpec,
-    pub shutdown_receiver: Receiver<ShutdownReason>,
+    pub shutdown_receiver: Arc<Mutex<Receiver<ShutdownReason>>>,
 
     pub mock_execution_layer: Option<MockExecutionLayer<T::EthSpec>>,
     pub execution_layer_runtime: Option<ExecutionLayerRuntime>,
@@ -502,6 +502,17 @@ where
         epoch.start_slot(E::slots_per_epoch()).into()
     }
 
+    pub fn shutdown_reasons(&self) -> Vec<ShutdownReason> {
+        let mutex = self.shutdown_receiver.clone();
+        let mut receiver = mutex.lock();
+        std::iter::from_fn(move || match receiver.try_next() {
+            Ok(Some(s)) => Some(s),
+            Ok(None) => panic!("shutdown sender dropped"),
+            Err(_) => None,
+        })
+        .collect()
+    }
+
     pub fn get_current_state(&self) -> BeaconState<E> {
         self.chain.head().unwrap().beacon_state
     }

beacon_node/beacon_chain/tests/attestation_production.rs

@@ -26,6 +26,7 @@ fn produces_attestations() {
         .default_spec()
         .keypairs(KEYPAIRS[..].to_vec())
         .fresh_ephemeral_store()
+        .mock_execution_layer()
         .build();
 
     let chain = &harness.chain;

beacon_node/beacon_chain/tests/attestation_verification.rs

@@ -42,6 +42,7 @@ fn get_harness(validator_count: usize) -> BeaconChainHarness<EphemeralHarnessTyp
         .spec(spec)
         .keypairs(KEYPAIRS[0..validator_count].to_vec())
         .fresh_ephemeral_store()
+        .mock_execution_layer()
         .build();
 
     harness.advance_slot();

beacon_node/beacon_chain/tests/block_verification.rs

@@ -54,6 +54,7 @@ fn get_harness(validator_count: usize) -> BeaconChainHarness<EphemeralHarnessTyp
         .default_spec()
         .keypairs(KEYPAIRS[0..validator_count].to_vec())
         .fresh_ephemeral_store()
+        .mock_execution_layer()
         .build();
 
     harness.advance_slot();
@@ -839,6 +840,7 @@ fn verify_block_for_gossip_slashing_detection() {
         .keypairs(KEYPAIRS.to_vec())
         .fresh_ephemeral_store()
         .initial_mutator(Box::new(move |builder| builder.slasher(inner_slasher)))
+        .mock_execution_layer()
         .build();
     harness.advance_slot();
 
@@ -918,6 +920,7 @@ fn add_base_block_to_altair_chain() {
         .spec(spec)
         .keypairs(KEYPAIRS[..].to_vec())
         .fresh_ephemeral_store()
+        .mock_execution_layer()
         .build();
 
     // Move out of the genesis slot.
@@ -1036,6 +1039,7 @@ fn add_altair_block_to_base_chain() {
         .spec(spec)
         .keypairs(KEYPAIRS[..].to_vec())
         .fresh_ephemeral_store()
+        .mock_execution_layer()
         .build();
 
     // Move out of the genesis slot.

beacon_node/beacon_chain/tests/main.rs

@@ -3,6 +3,7 @@ mod attestation_verification;
 mod block_verification;
 mod merge;
 mod op_verification;
+mod payload_invalidation;
 mod store_tests;
 mod sync_committee_verification;
 mod tests;

beacon_node/beacon_chain/tests/merge.rs

@@ -13,7 +13,7 @@ fn verify_execution_payload_chain<T: EthSpec>(chain: &[ExecutionPayload<T>]) {
 
     for ep in chain {
         assert!(*ep != ExecutionPayload::default());
-        assert!(ep.block_hash != Hash256::zero());
+        assert!(ep.block_hash != ExecutionBlockHash::zero());
 
         // Check against previous `ExecutionPayload`.
         if let Some(prev_ep) = prev_ep {
@@ -40,7 +40,7 @@ fn merge_with_terminal_block_hash_override() {
         spec.terminal_total_difficulty,
         DEFAULT_TERMINAL_BLOCK,
         0,
-        Hash256::zero(),
+        ExecutionBlockHash::zero(),
     )
     .unwrap()
     .block_hash;

beacon_node/beacon_chain/tests/op_verification.rs

@@ -40,6 +40,7 @@ fn get_harness(store: Arc<HotColdDB>, validator_count: usize) -> TestHarness {
         .default_spec()
         .keypairs(KEYPAIRS[0..validator_count].to_vec())
         .fresh_disk_store(store)
+        .mock_execution_layer()
         .build();
     harness.advance_slot();
     harness