Don't create a execution payload with same timestamp as terminal block (

#3331)

## Issue Addressed

Resolves #3316 

## Proposed Changes

This PR fixes an issue where lighthouse created a transition block with `block.execution_payload().timestamp == terminal_block.timestamp` if the terminal block was created at the slot boundary.

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -3908,14 +3908,13 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
                 ForkName::Base | ForkName::Altair => return Ok(()),
                 _ => {
                     // We are post-bellatrix
-                    if execution_layer
+                    if let Some(payload_attributes) = execution_layer
                         .payload_attributes(next_slot, params.head_root)
                         .await
-                        .is_some()
                     {
                         // We are a proposer, check for terminal_pow_block_hash
                         if let Some(terminal_pow_block_hash) = execution_layer
-                            .get_terminal_pow_block_hash(&self.spec)
+                            .get_terminal_pow_block_hash(&self.spec, payload_attributes.timestamp)
                             .await
                             .map_err(Error::ForkchoiceUpdate)?
                         {

beacon_node/beacon_chain/src/execution_payload.rs

@@ -393,7 +393,7 @@ where
         }
 
         let terminal_pow_block_hash = execution_layer
-            .get_terminal_pow_block_hash(spec)
+            .get_terminal_pow_block_hash(spec, timestamp)
             .await
             .map_err(BlockProductionError::TerminalPoWBlockLookupFailed)?;
 

beacon_node/beacon_chain/src/test_utils.rs

@@ -30,6 +30,7 @@ use rayon::prelude::*;
 use sensitive_url::SensitiveUrl;
 use slog::Logger;
 use slot_clock::TestingSlotClock;
+use state_processing::per_block_processing::compute_timestamp_at_slot;
 use state_processing::{
     state_advance::{complete_state_advance, partial_state_advance},
     StateRootStrategy,
@@ -521,6 +522,11 @@ where
         self.chain.head_beacon_state_cloned()
     }
 
+    pub fn get_timestamp_at_slot(&self) -> u64 {
+        let state = self.get_current_state();
+        compute_timestamp_at_slot(&state, &self.spec).unwrap()
+    }
+
     pub fn get_current_state_and_root(&self) -> (BeaconState<E>, Hash256) {
         let head = self.chain.head_snapshot();
         let state_root = head.beacon_state_root();

beacon_node/beacon_chain/tests/merge.rs

@@ -1,7 +1,7 @@
 #![cfg(not(debug_assertions))] // Tests run too slow in debug.
 
 use beacon_chain::test_utils::BeaconChainHarness;
-use execution_layer::test_utils::{generate_pow_block, DEFAULT_TERMINAL_BLOCK};
+use execution_layer::test_utils::{generate_pow_block, Block, DEFAULT_TERMINAL_BLOCK};
 use types::*;
 
 const VALIDATOR_COUNT: usize = 32;
@@ -22,6 +22,7 @@ fn verify_execution_payload_chain<T: EthSpec>(chain: &[FullPayload<T>]) {
                 prev_ep.execution_payload.block_number + 1,
                 ep.execution_payload.block_number
             );
+            assert!(ep.execution_payload.timestamp > prev_ep.execution_payload.timestamp);
         }
         prev_ep = Some(ep.clone());
     }
@@ -169,6 +170,30 @@ async fn base_altair_merge_with_terminal_block_after_fork() {
         .move_to_terminal_block()
         .unwrap();
 
+    // Add a slot duration to get to the next slot
+    let timestamp = harness.get_timestamp_at_slot() + harness.spec.seconds_per_slot;
+
+    harness
+        .execution_block_generator()
+        .modify_last_block(|block| {
+            if let Block::PoW(terminal_block) = block {
+                terminal_block.timestamp = timestamp;
+            }
+        });
+
+    harness.extend_slots(1).await;
+
+    let one_after_merge_head = &harness.chain.head_snapshot().beacon_block;
+    assert_eq!(
+        *one_after_merge_head
+            .message()
+            .body()
+            .execution_payload()
+            .unwrap(),
+        FullPayload::default()
+    );
+    assert_eq!(one_after_merge_head.slot(), merge_fork_slot + 2);
+
     /*
      * Next merge block should include an exec payload.
      */

beacon_node/execution_layer/src/engine_api.rs

@@ -106,6 +106,8 @@ pub struct ExecutionBlock {
     pub block_number: u64,
     pub parent_hash: ExecutionBlockHash,
     pub total_difficulty: Uint256,
+    #[serde(with = "eth2_serde_utils::u64_hex_be")]
+    pub timestamp: u64,
 }
 
 /// Representation of an exection block with enough detail to reconstruct a payload.

beacon_node/execution_layer/src/lib.rs

@@ -902,6 +902,7 @@ impl<T: EthSpec> ExecutionLayer<T> {
     pub async fn get_terminal_pow_block_hash(
         &self,
         spec: &ChainSpec,
+        timestamp: u64,
     ) -> Result<Option<ExecutionBlockHash>, Error> {
         let _timer = metrics::start_timer_vec(
             &metrics::EXECUTION_LAYER_REQUEST_TIMES,
@@ -924,8 +925,19 @@ impl<T: EthSpec> ExecutionLayer<T> {
                     }
                 }
 
-                self.get_pow_block_hash_at_total_difficulty(engine, spec)
-                    .await
+                let block = self.get_pow_block_at_total_difficulty(engine, spec).await?;
+                if let Some(pow_block) = block {
+                    // If `terminal_block.timestamp == transition_block.timestamp`,
+                    // we violate the invariant that a block's timestamp must be
+                    // strictly greater than its parent's timestamp.
+                    // The execution layer will reject a fcu call with such payload
+                    // attributes leading to a missed block.
+                    // Hence, we return `None` in such a case.
+                    if pow_block.timestamp >= timestamp {
+                        return Ok(None);
+                    }
+                }
+                Ok(block.map(|b| b.block_hash))
             })
             .await
             .map_err(Box::new)
@@ -953,11 +965,11 @@ impl<T: EthSpec> ExecutionLayer<T> {
     /// `get_pow_block_at_terminal_total_difficulty`
     ///
     /// https://github.com/ethereum/consensus-specs/blob/v1.1.5/specs/merge/validator.md
-    async fn get_pow_block_hash_at_total_difficulty(
+    async fn get_pow_block_at_total_difficulty(
         &self,
         engine: &Engine,
         spec: &ChainSpec,
-    ) -> Result<Option<ExecutionBlockHash>, ApiError> {
+    ) -> Result<Option<ExecutionBlock>, ApiError> {
         let mut block = engine
             .api
             .get_block_by_number(BlockByNumberQuery::Tag(LATEST_TAG))
@@ -970,7 +982,7 @@ impl<T: EthSpec> ExecutionLayer<T> {
             let block_reached_ttd = block.total_difficulty >= spec.terminal_total_difficulty;
             if block_reached_ttd {
                 if block.parent_hash == ExecutionBlockHash::zero() {
-                    return Ok(Some(block.block_hash));
+                    return Ok(Some(block));
                 }
                 let parent = self
                     .get_pow_block(engine, block.parent_hash)
@@ -979,7 +991,7 @@ impl<T: EthSpec> ExecutionLayer<T> {
                 let parent_reached_ttd = parent.total_difficulty >= spec.terminal_total_difficulty;
 
                 if block_reached_ttd && !parent_reached_ttd {
-                    return Ok(Some(block.block_hash));
+                    return Ok(Some(block));
                 } else {
                     block = parent;
                 }
@@ -1197,19 +1209,54 @@ mod test {
             .move_to_block_prior_to_terminal_block()
             .with_terminal_block(|spec, el, _| async move {
                 el.engine().upcheck().await;
-                assert_eq!(el.get_terminal_pow_block_hash(&spec).await.unwrap(), None)
+                assert_eq!(
+                    el.get_terminal_pow_block_hash(&spec, timestamp_now())
+                        .await
+                        .unwrap(),
+                    None
+                )
             })
             .await
             .move_to_terminal_block()
             .with_terminal_block(|spec, el, terminal_block| async move {
                 assert_eq!(
-                    el.get_terminal_pow_block_hash(&spec).await.unwrap(),
+                    el.get_terminal_pow_block_hash(&spec, timestamp_now())
+                        .await
+                        .unwrap(),
                     Some(terminal_block.unwrap().block_hash)
                 )
             })
             .await;
     }
 
+    #[tokio::test]
+    async fn rejects_terminal_block_with_equal_timestamp() {
+        let runtime = TestRuntime::default();
+        MockExecutionLayer::default_params(runtime.task_executor.clone())
+            .move_to_block_prior_to_terminal_block()
+            .with_terminal_block(|spec, el, _| async move {
+                el.engine().upcheck().await;
+                assert_eq!(
+                    el.get_terminal_pow_block_hash(&spec, timestamp_now())
+                        .await
+                        .unwrap(),
+                    None
+                )
+            })
+            .await
+            .move_to_terminal_block()
+            .with_terminal_block(|spec, el, terminal_block| async move {
+                let timestamp = terminal_block.as_ref().map(|b| b.timestamp).unwrap();
+                assert_eq!(
+                    el.get_terminal_pow_block_hash(&spec, timestamp)
+                        .await
+                        .unwrap(),
+                    None
+                )
+            })
+            .await;
+    }
+
     #[tokio::test]
     async fn verifies_valid_terminal_block_hash() {
         let runtime = TestRuntime::default();
@@ -1269,3 +1316,12 @@ mod test {
 fn noop<T: EthSpec>(_: &ExecutionLayer<T>, _: &ExecutionPayload<T>) -> Option<ExecutionPayload<T>> {
     None
 }
+
+#[cfg(test)]
+/// Returns the duration since the unix epoch.
+fn timestamp_now() -> u64 {
+    SystemTime::now()
+        .duration_since(UNIX_EPOCH)
+        .unwrap_or_else(|_| Duration::from_secs(0))
+        .as_secs()
+}

beacon_node/execution_layer/src/test_utils/execution_block_generator.rs

@@ -60,12 +60,14 @@ impl<T: EthSpec> Block<T> {
                 block_number: block.block_number,
                 parent_hash: block.parent_hash,
                 total_difficulty: block.total_difficulty,
+                timestamp: block.timestamp,
             },
             Block::PoS(payload) => ExecutionBlock {
                 block_hash: payload.block_hash,
                 block_number: payload.block_number,
                 parent_hash: payload.parent_hash,
                 total_difficulty,
+                timestamp: payload.timestamp,
             },
         }
     }
@@ -100,6 +102,7 @@ pub struct PoWBlock {
     pub block_hash: ExecutionBlockHash,
     pub parent_hash: ExecutionBlockHash,
     pub total_difficulty: Uint256,
+    pub timestamp: u64,
 }
 
 pub struct ExecutionBlockGenerator<T: EthSpec> {
@@ -266,6 +269,26 @@ impl<T: EthSpec> ExecutionBlockGenerator<T> {
         Ok(())
     }
 
+    pub fn modify_last_block(&mut self, block_modifier: impl FnOnce(&mut Block<T>)) {
+        if let Some((last_block_hash, block_number)) =
+            self.block_hashes.keys().max().and_then(|block_number| {
+                self.block_hashes
+                    .get(block_number)
+                    .map(|block| (block, *block_number))
+            })
+        {
+            let mut block = self.blocks.remove(last_block_hash).unwrap();
+            block_modifier(&mut block);
+            // Update the block hash after modifying the block
+            match &mut block {
+                Block::PoW(b) => b.block_hash = ExecutionBlockHash::from_root(b.tree_hash_root()),
+                Block::PoS(b) => b.block_hash = ExecutionBlockHash::from_root(b.tree_hash_root()),
+            }
+            self.block_hashes.insert(block_number, block.block_hash());
+            self.blocks.insert(block.block_hash(), block);
+        }
+    }
+
     pub fn get_payload(&mut self, id: &PayloadId) -> Option<ExecutionPayload<T>> {
         self.payload_ids.get(id).cloned()
     }
@@ -423,6 +446,7 @@ pub fn generate_pow_block(
         block_hash: ExecutionBlockHash::zero(),
         parent_hash,
         total_difficulty,
+        timestamp: block_number,
     };
 
     block.block_hash = ExecutionBlockHash::from_root(block.tree_hash_root());

beacon_node/execution_layer/src/test_utils/mod.rs

@@ -6,7 +6,7 @@ use crate::engine_api::{
 };
 use bytes::Bytes;
 use environment::null_logger;
-use execution_block_generator::{Block, PoWBlock};
+use execution_block_generator::PoWBlock;
 use handle_rpc::handle_rpc;
 use parking_lot::{Mutex, RwLock, RwLockWriteGuard};
 use serde::{Deserialize, Serialize};
@@ -21,7 +21,7 @@ use tokio::{runtime, sync::oneshot};
 use types::{EthSpec, ExecutionBlockHash, Uint256};
 use warp::{http::StatusCode, Filter, Rejection};
 
-pub use execution_block_generator::{generate_pow_block, ExecutionBlockGenerator};
+pub use execution_block_generator::{generate_pow_block, Block, ExecutionBlockGenerator};
 pub use mock_execution_layer::MockExecutionLayer;
 
 pub const DEFAULT_TERMINAL_DIFFICULTY: u64 = 6400;
@@ -334,6 +334,7 @@ impl<T: EthSpec> MockServer<T> {
             block_hash,
             parent_hash,
             total_difficulty,
+            timestamp: block_number,
         });
 
         self.ctx

testing/execution_engine_integration/src/test_rig.rs

@@ -219,7 +219,7 @@ impl<E: GenericExecutionEngine> TestRig<E> {
         let terminal_pow_block_hash = self
             .ee_a
             .execution_layer
-            .get_terminal_pow_block_hash(&self.spec)
+            .get_terminal_pow_block_hash(&self.spec, timestamp_now())
             .await
             .unwrap()
             .unwrap();
@@ -228,7 +228,7 @@ impl<E: GenericExecutionEngine> TestRig<E> {
             terminal_pow_block_hash,
             self.ee_b
                 .execution_layer
-                .get_terminal_pow_block_hash(&self.spec)
+                .get_terminal_pow_block_hash(&self.spec, timestamp_now())
                 .await
                 .unwrap()
                 .unwrap()