Transition Block Proposer Preparation (#3088)

## Issue Addressed

- #3058 

Co-authored-by: Paul Hauner <paul@paulhauner.com>

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -3481,9 +3481,6 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
             .beacon_state
             .attester_shuffling_decision_root(self.genesis_block_root, RelativeEpoch::Current);
 
-        // Used later for the execution engine.
-        let is_merge_transition_complete = is_merge_transition_complete(&new_head.beacon_state);
-
         drop(lag_timer);
 
         // Clear the early attester cache in case it conflicts with `self.canonical_head`.
@@ -3690,32 +3687,28 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
             }
         }
 
-        // If this is a post-merge block, update the execution layer.
+        // Update the execution layer.
-        if is_merge_transition_complete {
+        if let Err(e) = self.update_execution_engine_forkchoice_blocking(self.slot()?) {
-            let current_slot = self.slot()?;
+            crit!(
+                self.log,
+                "Failed to update execution head";
+                "error" => ?e
+            );
+        }
 
-            if let Err(e) = self.update_execution_engine_forkchoice_blocking(current_slot) {
+        // Performing this call immediately after
-                crit!(
+        // `update_execution_engine_forkchoice_blocking` might result in two calls to fork
-                    self.log,
+        // choice updated, one *without* payload attributes and then a second *with*
-                    "Failed to update execution head";
+        // payload attributes.
-                    "error" => ?e
+        //
-                );
+        // This seems OK. It's not a significant waste of EL<>CL bandwidth or resources, as
-            }
+        // far as I know.
-
+        if let Err(e) = self.prepare_beacon_proposer_blocking() {
-            // Performing this call immediately after
+            crit!(
-            // `update_execution_engine_forkchoice_blocking` might result in two calls to fork
+                self.log,
-            // choice updated, one *without* payload attributes and then a second *with*
+                "Failed to prepare proposers after fork choice";
-            // payload attributes.
+                "error" => ?e
-            //
+            );
-            // This seems OK. It's not a significant waste of EL<>CL bandwidth or resources, as
-            // far as I know.
-            if let Err(e) = self.prepare_beacon_proposer_blocking() {
-                crit!(
-                    self.log,
-                    "Failed to prepare proposers after fork choice";
-                    "error" => ?e
-                );
-            }
         }
 
         Ok(())
@@ -3745,6 +3738,19 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
     /// 2. The head block is one slot (or less) behind the prepare slot (e.g., we're preparing for
     ///    the next slot and the block at the current slot is already known).
     pub async fn prepare_beacon_proposer_async(&self) -> Result<(), Error> {
+        let current_slot = self.slot()?;
+        let prepare_slot = current_slot + 1;
+        let prepare_epoch = prepare_slot.epoch(T::EthSpec::slots_per_epoch());
+
+        // There's no need to run the proposer preparation routine before the bellatrix fork.
+        if self
+            .spec
+            .bellatrix_fork_epoch
+            .map_or(true, |bellatrix| prepare_epoch < bellatrix)
+        {
+            return Ok(());
+        }
+
         let execution_layer = self
             .execution_layer
             .clone()
@@ -3757,7 +3763,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         }
 
         let head = self.head_info()?;
-        let current_slot = self.slot()?;
+        let head_epoch = head.slot.epoch(T::EthSpec::slots_per_epoch());
 
         // Don't bother with proposer prep if the head is more than
         // `PREPARE_PROPOSER_HISTORIC_EPOCHS` prior to the current slot.
@@ -3775,19 +3781,6 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
             return Ok(());
         }
 
-        // We only start to push preparation data for some chain *after* the transition block
-        // has been imported.
-        //
-        // There is no payload preparation for the transition block (i.e., the first block with
-        // execution enabled in some chain).
-        if head.execution_payload_block_hash.is_none() {
-            return Ok(());
-        };
-
-        let head_epoch = head.slot.epoch(T::EthSpec::slots_per_epoch());
-        let prepare_slot = current_slot + 1;
-        let prepare_epoch = prepare_slot.epoch(T::EthSpec::slots_per_epoch());
-
         // Ensure that the shuffling decision root is correct relative to the epoch we wish to
         // query.
         let shuffling_decision_root = if head_epoch == prepare_epoch {
@@ -3968,6 +3961,23 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         &self,
         current_slot: Slot,
     ) -> Result<(), Error> {
+        let next_slot = current_slot + 1;
+
+        // There is no need to issue a `forkchoiceUpdated` (fcU) message unless the Bellatrix fork
+        // has:
+        //
+        // 1. Already happened.
+        // 2. Will happen in the next slot.
+        //
+        // The reason for a fcU message in the slot prior to the Bellatrix fork is in case the
+        // terminal difficulty has already been reached and a payload preparation message needs to
+        // be issued.
+        if self.spec.bellatrix_fork_epoch.map_or(true, |bellatrix| {
+            next_slot.epoch(T::EthSpec::slots_per_epoch()) < bellatrix
+        }) {
+            return Ok(());
+        }
+
         let execution_layer = self
             .execution_layer
             .as_ref()
@@ -3994,29 +4004,69 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         // We are taking the `self.fork_choice` lock whilst holding the `forkchoice_lock`. This
         // is intentional, since it allows us to ensure a consistent ordering of messages to the
         // execution layer.
-        let (head_block_root, head_hash, finalized_hash) =
+        let forkchoice_update_parameters =
-            if let Some(params) = self.fork_choice.read().get_forkchoice_update_parameters() {
+            self.fork_choice.read().get_forkchoice_update_parameters();
-                if let Some(head_hash) = params.head_hash {
+        let (head_block_root, head_hash, finalized_hash) = if let Some(params) =
-                    (
+            forkchoice_update_parameters
-                        params.head_root,
+        {
-                        head_hash,
+            if let Some(head_hash) = params.head_hash {
-                        params
+                (
-                            .finalized_hash
+                    params.head_root,
-                            .unwrap_or_else(ExecutionBlockHash::zero),
+                    head_hash,
-                    )
+                    params
-                } else {
+                        .finalized_hash
-                    // The head block does not have an execution block hash, there is no need to
+                        .unwrap_or_else(ExecutionBlockHash::zero),
-                    // send an update to the EL.
+                )
-                    return Ok(());
-                }
             } else {
-                warn!(
+                // The head block does not have an execution block hash. We must check to see if we
-                    self.log,
+                // happen to be the proposer of the transition block, in which case we still need to
-                    "Missing forkchoice params";
+                // send forkchoice_updated.
-                    "msg" => "please report this non-critical bug"
+                match self.spec.fork_name_at_slot::<T::EthSpec>(next_slot) {
-                );
+                    // We are pre-bellatrix; no need to update the EL.
-                return Ok(());
+                    ForkName::Base | ForkName::Altair => return Ok(()),
-            };
+                    _ => {
+                        // We are post-bellatrix
+                        if 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)
+                                .await
+                                .map_err(Error::ForkchoiceUpdate)?
+                            {
+                                info!(
+                                    self.log,
+                                    "Prepared POS transition block proposer"; "slot" => next_slot
+                                );
+                                (
+                                    params.head_root,
+                                    terminal_pow_block_hash,
+                                    params
+                                        .finalized_hash
+                                        .unwrap_or_else(ExecutionBlockHash::zero),
+                                )
+                            } else {
+                                // TTD hasn't been reached yet, no need to update the EL.
+                                return Ok(());
+                            }
+                        } else {
+                            // We are not a proposer, no need to update the EL.
+                            return Ok(());
+                        }
+                    }
+                }
+            }
+        } else {
+            warn!(
+                self.log,
+                "Missing forkchoice params";
+                "msg" => "please report this non-critical bug"
+            );
+            return Ok(());
+        };
 
         let forkchoice_updated_response = self
             .execution_layer

beacon_node/beacon_chain/tests/payload_invalidation.rs

@@ -6,7 +6,8 @@ use beacon_chain::{
     WhenSlotSkipped, INVALID_JUSTIFIED_PAYLOAD_SHUTDOWN_REASON,
 };
 use execution_layer::{
-    json_structures::JsonPayloadAttributesV1, ExecutionLayer, PayloadAttributes,
+    json_structures::{JsonForkChoiceStateV1, JsonPayloadAttributesV1},
+    ExecutionLayer, ForkChoiceState, PayloadAttributes,
 };
 use fork_choice::{Error as ForkChoiceError, InvalidationOperation, PayloadVerificationStatus};
 use proto_array::{Error as ProtoArrayError, ExecutionStatus};
@@ -95,17 +96,28 @@ impl InvalidPayloadRig {
         self.harness.chain.head_info().unwrap()
     }
 
-    fn previous_payload_attributes(&self) -> PayloadAttributes {
+    fn previous_forkchoice_update_params(&self) -> (ForkChoiceState, PayloadAttributes) {
         let mock_execution_layer = self.harness.mock_execution_layer.as_ref().unwrap();
         let json = mock_execution_layer
             .server
             .take_previous_request()
             .expect("no previous request");
         let params = json.get("params").expect("no params");
+
+        let fork_choice_state_json = params.get(0).expect("no payload param");
+        let fork_choice_state: JsonForkChoiceStateV1 =
+            serde_json::from_value(fork_choice_state_json.clone()).unwrap();
+
         let payload_param_json = params.get(1).expect("no payload param");
         let attributes: JsonPayloadAttributesV1 =
             serde_json::from_value(payload_param_json.clone()).unwrap();
-        attributes.into()
+
+        (fork_choice_state.into(), attributes.into())
+    }
+
+    fn previous_payload_attributes(&self) -> PayloadAttributes {
+        let (_, payload_attributes) = self.previous_forkchoice_update_params();
+        payload_attributes
     }
 
     fn move_to_terminal_block(&self) {
@@ -117,6 +129,16 @@ impl InvalidPayloadRig {
             .unwrap();
     }
 
+    fn latest_execution_block_hash(&self) -> ExecutionBlockHash {
+        let mock_execution_layer = self.harness.mock_execution_layer.as_ref().unwrap();
+        mock_execution_layer
+            .server
+            .execution_block_generator()
+            .latest_execution_block()
+            .unwrap()
+            .block_hash
+    }
+
     fn build_blocks(&mut self, num_blocks: u64, is_valid: Payload) -> Vec<Hash256> {
         (0..num_blocks)
             .map(|_| self.import_block(is_valid.clone()))
@@ -810,3 +832,56 @@ fn invalid_parent() {
         ))
     ));
 }
+
+/// Tests to ensure that we will still send a proposer preparation
+#[test]
+fn payload_preparation_before_transition_block() {
+    let rig = InvalidPayloadRig::new();
+    let el = rig.execution_layer();
+
+    let head = rig.harness.chain.head().unwrap();
+    let head_info = rig.head_info();
+    assert!(
+        !head_info.is_merge_transition_complete,
+        "the head block is pre-transition"
+    );
+    assert_eq!(
+        head_info.execution_payload_block_hash,
+        Some(ExecutionBlockHash::zero()),
+        "the head block is post-bellatrix"
+    );
+
+    let current_slot = rig.harness.chain.slot().unwrap();
+    let next_slot = current_slot + 1;
+    let proposer = head
+        .beacon_state
+        .get_beacon_proposer_index(next_slot, &rig.harness.chain.spec)
+        .unwrap();
+    let fee_recipient = Address::repeat_byte(99);
+
+    // Provide preparation data to the EL for `proposer`.
+    el.update_proposer_preparation_blocking(
+        Epoch::new(0),
+        &[ProposerPreparationData {
+            validator_index: proposer as u64,
+            fee_recipient,
+        }],
+    )
+    .unwrap();
+
+    rig.move_to_terminal_block();
+
+    rig.harness
+        .chain
+        .prepare_beacon_proposer_blocking()
+        .unwrap();
+    rig.harness
+        .chain
+        .update_execution_engine_forkchoice_blocking(current_slot)
+        .unwrap();
+
+    let (fork_choice_state, payload_attributes) = rig.previous_forkchoice_update_params();
+    let latest_block_hash = rig.latest_execution_block_hash();
+    assert_eq!(payload_attributes.suggested_fee_recipient, fee_recipient);
+    assert_eq!(fork_choice_state.head_block_hash, latest_block_hash);
+}

beacon_node/execution_layer/src/lib.rs

@@ -4,11 +4,17 @@
 //! This crate only provides useful functionality for "The Merge", it does not provide any of the
 //! deposit-contract functionality that the `beacon_node/eth1` crate already provides.
 
+use crate::engine_api::Builder;
+use crate::engines::Builders;
 use auth::{Auth, JwtKey};
-use engine_api::{Error as ApiError, *};
+use engine_api::Error as ApiError;
-use engines::{Engine, EngineError, Engines, ForkChoiceState, Logging};
+pub use engine_api::*;
+pub use engine_api::{http, http::HttpJsonRpc};
+pub use engines::ForkChoiceState;
+use engines::{Engine, EngineError, Engines, Logging};
 use lru::LruCache;
 use payload_status::process_multiple_payload_statuses;
+pub use payload_status::PayloadStatus;
 use sensitive_url::SensitiveUrl;
 use serde::{Deserialize, Serialize};
 use slog::{crit, debug, error, info, trace, Logger};
@@ -30,12 +36,6 @@ use types::{
     ProposerPreparationData, SignedBeaconBlock, Slot,
 };
 
-use crate::engine_api::Builder;
-use crate::engines::Builders;
-pub use engine_api::*;
-pub use engine_api::{http, http::HttpJsonRpc};
-pub use payload_status::PayloadStatus;
-
 mod engine_api;
 mod engines;
 mod metrics;