diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 6599bb69f..b1d67e5d9 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -3481,9 +3481,6 @@ impl BeaconChain { .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 BeaconChain { } } - // If this is a post-merge block, update the execution layer. - if is_merge_transition_complete { - let current_slot = self.slot()?; + // Update the execution layer. + if let Err(e) = self.update_execution_engine_forkchoice_blocking(self.slot()?) { + crit!( + self.log, + "Failed to update execution head"; + "error" => ?e + ); + } - if let Err(e) = self.update_execution_engine_forkchoice_blocking(current_slot) { - crit!( - self.log, - "Failed to update execution head"; - "error" => ?e - ); - } - - // Performing this call immediately after - // `update_execution_engine_forkchoice_blocking` might result in two calls to fork - // choice updated, one *without* payload attributes and then a second *with* - // payload attributes. - // - // 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 - ); - } + // Performing this call immediately after + // `update_execution_engine_forkchoice_blocking` might result in two calls to fork + // choice updated, one *without* payload attributes and then a second *with* + // payload attributes. + // + // 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 BeaconChain { /// 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 BeaconChain { } 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 BeaconChain { 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 BeaconChain { &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 BeaconChain { // 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) = - if let Some(params) = self.fork_choice.read().get_forkchoice_update_parameters() { - if let Some(head_hash) = params.head_hash { - ( - params.head_root, - head_hash, - params - .finalized_hash - .unwrap_or_else(ExecutionBlockHash::zero), - ) - } else { - // The head block does not have an execution block hash, there is no need to - // send an update to the EL. - return Ok(()); - } + let forkchoice_update_parameters = + self.fork_choice.read().get_forkchoice_update_parameters(); + let (head_block_root, head_hash, finalized_hash) = if let Some(params) = + forkchoice_update_parameters + { + if let Some(head_hash) = params.head_hash { + ( + params.head_root, + head_hash, + params + .finalized_hash + .unwrap_or_else(ExecutionBlockHash::zero), + ) } else { - warn!( - self.log, - "Missing forkchoice params"; - "msg" => "please report this non-critical bug" - ); - return Ok(()); - }; + // The head block does not have an execution block hash. We must check to see if we + // happen to be the proposer of the transition block, in which case we still need to + // send forkchoice_updated. + match self.spec.fork_name_at_slot::(next_slot) { + // We are pre-bellatrix; no need to update the EL. + 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 diff --git a/beacon_node/beacon_chain/tests/payload_invalidation.rs b/beacon_node/beacon_chain/tests/payload_invalidation.rs index e1c082a88..84317196b 100644 --- a/beacon_node/beacon_chain/tests/payload_invalidation.rs +++ b/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 { (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); +} diff --git a/beacon_node/execution_layer/src/lib.rs b/beacon_node/execution_layer/src/lib.rs index 82baec4b0..442c5b48d 100644 --- a/beacon_node/execution_layer/src/lib.rs +++ b/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 engines::{Engine, EngineError, Engines, ForkChoiceState, Logging}; +use engine_api::Error as ApiError; +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;