Ensure we don't optimistically verify

beacon_node/beacon_chain/src/block_verification.rs

@@ -44,7 +44,7 @@
 //! ```
 use crate::execution_payload::{
     is_optimistic_candidate_block, validate_execution_payload_for_gossip, validate_merge_block,
-    PayloadNotifier,
+    AllowOptimisticImport, PayloadNotifier,
 };
 use crate::snapshot_cache::PreProcessingSnapshot;
 use crate::validator_monitor::HISTORIC_EPOCHS as VALIDATOR_MONITOR_HISTORIC_EPOCHS;
@@ -1199,7 +1199,7 @@ impl<T: BeaconChainTypes> ExecutionPendingBlock<T> {
             // - Doing the check here means we can keep our fork-choice implementation "pure". I.e., no
             //   calls to remote servers.
             if is_valid_merge_transition_block {
-                validate_merge_block(&chain, block.message()).await?;
+                validate_merge_block(&chain, block.message(), AllowOptimisticImport::Yes).await?;
             };
 
             // The specification declares that this should be run *inside* `per_block_processing`,

beacon_node/beacon_chain/src/execution_payload.rs

@@ -28,6 +28,12 @@ use types::*;
 pub type PreparePayloadResult<Payload> = Result<Payload, BlockProductionError>;
 pub type PreparePayloadHandle<Payload> = JoinHandle<Option<PreparePayloadResult<Payload>>>;
 
+#[derive(PartialEq)]
+pub enum AllowOptimisticImport {
+    Yes,
+    No,
+}
+
 /// Used to await the result of executing payload with a remote EE.
 pub struct PayloadNotifier<T: BeaconChainTypes> {
     pub chain: Arc<BeaconChain<T>>,
@@ -147,6 +153,7 @@ async fn notify_new_payload<'a, T: BeaconChainTypes>(
 pub async fn validate_merge_block<'a, T: BeaconChainTypes>(
     chain: &Arc<BeaconChain<T>>,
     block: BeaconBlockRef<'a, T::EthSpec>,
+    allow_optimistic_import: AllowOptimisticImport,
 ) -> Result<(), BlockError<T::EthSpec>> {
     let spec = &chain.spec;
     let block_epoch = block.slot().epoch(T::EthSpec::slots_per_epoch());
@@ -189,7 +196,9 @@ pub async fn validate_merge_block<'a, T: BeaconChainTypes>(
         }
         .into()),
         None => {
-            if is_optimistic_candidate_block(chain, block.slot(), block.parent_root()).await? {
+            if allow_optimistic_import == AllowOptimisticImport::Yes
+                && is_optimistic_candidate_block(chain, block.slot(), block.parent_root()).await?
+            {
                 debug!(
                     chain.log,
                     "Optimistically importing merge transition block";

beacon_node/beacon_chain/src/otb_verification_service.rs

@@ -1,4 +1,4 @@
-use crate::execution_payload::validate_merge_block;
+use crate::execution_payload::{validate_merge_block, AllowOptimisticImport};
 use crate::{
     BeaconChain, BeaconChainError, BeaconChainTypes, BlockError, ExecutionPayloadError,
     INVALID_FINALIZED_MERGE_TRANSITION_BLOCK_SHUTDOWN_REASON,
@@ -181,7 +181,8 @@ pub async fn validate_optimistic_transition_blocks<T: BeaconChainTypes>(
     for otb in finalized_canonical_otbs {
         match chain.get_block(otb.root()).await {
             Ok(Some(block)) => {
-                match validate_merge_block(chain, block.message()).await {
+                match validate_merge_block(chain, block.message(), AllowOptimisticImport::No).await
+                {
                     Ok(()) => {
                         // merge transition block is valid, remove it from OTB
                         otb.remove_from_store::<T, _>(&chain.store)
@@ -193,6 +194,12 @@ pub async fn validate_optimistic_transition_blocks<T: BeaconChainTypes>(
                             "type" => "finalized"
                         );
                     }
+                    // The block was not able to be verified by the EL. Leave the OTB in the
+                    // database since the EL is likely still syncing and may verify the block
+                    // later.
+                    Err(BlockError::ExecutionPayloadError(
+                        ExecutionPayloadError::UnverifiedNonOptimisticCandidate,
+                    )) => (),
                     Err(BlockError::ExecutionPayloadError(
                         ExecutionPayloadError::InvalidTerminalPoWBlock { .. },
                     )) => {
@@ -231,7 +238,8 @@ pub async fn validate_optimistic_transition_blocks<T: BeaconChainTypes>(
     for otb in unfinalized_canonical_otbs {
         match chain.get_block(otb.root()).await {
             Ok(Some(block)) => {
-                match validate_merge_block(chain, block.message()).await {
+                match validate_merge_block(chain, block.message(), AllowOptimisticImport::No).await
+                {
                     Ok(()) => {
                         // merge transition block is valid, remove it from OTB
                         otb.remove_from_store::<T, _>(&chain.store)
@@ -243,6 +251,12 @@ pub async fn validate_optimistic_transition_blocks<T: BeaconChainTypes>(
                             "type" => "not finalized"
                         );
                     }
+                    // The block was not able to be verified by the EL. Leave the OTB in the
+                    // database since the EL is likely still syncing and may verify the block
+                    // later.
+                    Err(BlockError::ExecutionPayloadError(
+                        ExecutionPayloadError::UnverifiedNonOptimisticCandidate,
+                    )) => (),
                     Err(BlockError::ExecutionPayloadError(
                         ExecutionPayloadError::InvalidTerminalPoWBlock { .. },
                     )) => {

beacon_node/beacon_chain/tests/payload_invalidation.rs

@@ -1540,6 +1540,111 @@ async fn optimistic_transition_block_invalid_unfinalized() {
     );
 }
 
+#[tokio::test]
+async fn optimistic_transition_block_invalid_unfinalized_syncing_ee() {
+    let block_ttd = 42;
+    let rig_ttd = 1337;
+    let num_blocks = 22 as usize;
+    let rig = build_optimistic_chain(block_ttd, rig_ttd, num_blocks).await;
+
+    // In theory, you should be able to retrospectively validate the transition block now.
+    let post_transition_block_root = rig
+        .harness
+        .chain
+        .block_root_at_slot(Slot::new(1), WhenSlotSkipped::None)
+        .unwrap()
+        .unwrap();
+    let post_transition_block = rig
+        .harness
+        .chain
+        .get_block(&post_transition_block_root)
+        .await
+        .unwrap()
+        .unwrap();
+
+    let otbs = load_optimistic_transition_blocks(&rig.harness.chain)
+        .expect("should load optimistic transition block from db");
+    assert_eq!(
+        otbs.len(),
+        1,
+        "There should be one optimistic transition block"
+    );
+
+    let invalid_otb = OptimisticTransitionBlock::from_block(post_transition_block.message());
+    assert_eq!(
+        invalid_otb, otbs[0],
+        "The optimistic transition block stored in the database should be what we expect",
+    );
+
+    // No shutdown should've been triggered.
+    assert_eq!(rig.harness.shutdown_reasons(), vec![]);
+    // It shouldn't be known as invalid yet
+    assert!(!rig
+        .execution_status(post_transition_block_root)
+        .is_invalid());
+
+    // Make the execution layer respond `None` to all `getBlockByHash` requests to simulate a
+    // syncing EE.
+    let mock_execution_layer = rig.harness.mock_execution_layer.as_ref().unwrap();
+    mock_execution_layer
+        .server
+        .all_get_block_by_hash_requests_return_none();
+
+    validate_optimistic_transition_blocks(&rig.harness.chain, otbs)
+        .await
+        .unwrap();
+
+    // Still no shutdown should've been triggered.
+    assert_eq!(rig.harness.shutdown_reasons(), vec![]);
+
+    // It should still be marked as optimistic.
+    assert!(rig
+        .execution_status(post_transition_block_root)
+        .is_optimistic());
+
+    // the optimistic merge transition block should NOT have been removed from the database
+    let otbs = load_optimistic_transition_blocks(&rig.harness.chain)
+        .expect("should load optimistic transition block from db");
+    assert_eq!(
+        otbs.len(),
+        1,
+        "The optimistic merge transition block should still be in the database",
+    );
+    assert_eq!(
+        invalid_otb, otbs[0],
+        "The optimistic transition block stored in the database should be what we expect",
+    );
+
+    // Allow the EL to respond to `getBlockByHash`, as if it has finished syncing.
+    mock_execution_layer
+        .server
+        .all_get_block_by_hash_requests_return_natural_value();
+
+    validate_optimistic_transition_blocks(&rig.harness.chain, otbs)
+        .await
+        .expect("should invalidate merge transition block and shutdown the client");
+
+    // Still no shutdown should've been triggered.
+    assert_eq!(rig.harness.shutdown_reasons(), vec![]);
+    // It should be marked invalid now
+    assert!(rig
+        .execution_status(post_transition_block_root)
+        .is_invalid());
+
+    // the invalid merge transition block should NOT have been removed from the database
+    let otbs = load_optimistic_transition_blocks(&rig.harness.chain)
+        .expect("should load optimistic transition block from db");
+    assert_eq!(
+        otbs.len(),
+        1,
+        "The invalid merge transition block should still be in the database",
+    );
+    assert_eq!(
+        invalid_otb, otbs[0],
+        "The optimistic transition block stored in the database should be what we expect",
+    );
+}
+
 #[tokio::test]
 async fn optimistic_transition_block_invalid_finalized() {
     let block_ttd = 42;