From e4fa7d906f999b82510ad0eab791bd092947107b Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 10 Mar 2022 06:05:24 +0000 Subject: [PATCH] Fix post-merge checkpoint sync (#3065) ## Issue Addressed This address an issue which was preventing checkpoint-sync. When the node starts from checkpoint sync, the head block and the finalized block are the same value. We did not respect this when sending a `forkchoiceUpdated` (fcU) call to the EL and were expecting fork choice to hold the *finalized ancestor of the head* and returning an error when it didn't. This PR uses *only fork choice* for sending fcU updates. This is actually quite nice and avoids some atomicity issues between `chain.canonical_head` and `chain.fork_choice`. Now, whenever `chain.fork_choice.get_head` returns a value we also cache the values required for the next fcU call. ## TODO - [x] ~~Blocked on #3043~~ - [x] Ensure there isn't a warn message at startup. --- beacon_node/beacon_chain/src/beacon_chain.rs | 65 ++++++++------------ consensus/fork_choice/src/fork_choice.rs | 54 +++++++++++++--- 2 files changed, 72 insertions(+), 47 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 77900d82c..443b44ffa 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -3952,56 +3952,45 @@ impl BeaconChain { // of the head. I.e., we will never communicate a past head after communicating a later // one. // - // There are two "deadlock warnings" in this function. The downside of this nice ordering - // is the potential for deadlock. I would advise against any other use of + // There is a "deadlock warning" in this function. The downside of this nice ordering is the + // potential for deadlock. I would advise against any other use of // `execution_engine_forkchoice_lock` apart from the one here. let forkchoice_lock = execution_layer.execution_engine_forkchoice_lock().await; // Deadlock warning: // - // We are taking the `self.canonical_head` lock whilst holding the `forkchoice_lock`. This + // 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 = self.head_info()?; - let head_block_root = head.block_root; - let head_execution_block_hash = if let Some(hash) = head - .execution_payload_block_hash - .filter(|h| *h != ExecutionBlockHash::zero()) - { - hash - } else { - // Don't send fork choice updates to the execution layer before the transition block. - return Ok(()); - }; - - let finalized_root = if head.finalized_checkpoint.root == Hash256::zero() { - // De-alias `0x00..00` to the genesis block root. - self.genesis_block_root - } else { - head.finalized_checkpoint.root - }; - // Deadlock warning: - // - // The same as above, but the lock on `self.fork_choice`. - let finalized_execution_block_hash = self - .fork_choice - .read() - .get_block(&finalized_root) - .ok_or(Error::FinalizedBlockMissingFromForkChoice(finalized_root))? - .execution_status - .block_hash() - .unwrap_or_else(ExecutionBlockHash::zero); + 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(()); + } + } else { + warn!( + self.log, + "Missing forkchoice params"; + "msg" => "please report this non-critical bug" + ); + return Ok(()); + }; let forkchoice_updated_response = self .execution_layer .as_ref() .ok_or(Error::ExecutionLayerMissing)? - .notify_forkchoice_updated( - head_execution_block_hash, - finalized_execution_block_hash, - current_slot, - head_block_root, - ) + .notify_forkchoice_updated(head_hash, finalized_hash, current_slot, head_block_root) .await .map_err(Error::ExecutionForkChoiceUpdateFailed); diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index b7fd67eca..dfa922e5d 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -241,6 +241,14 @@ pub enum AttestationFromBlock { False, } +/// Parameters which are cached between calls to `Self::get_head`. +#[derive(Clone, Copy)] +pub struct ForkchoiceUpdateParameters { + pub head_root: Hash256, + pub head_hash: Option, + pub finalized_hash: Option, +} + /// Provides an implementation of "Ethereum 2.0 Phase 0 -- Beacon Chain Fork Choice": /// /// https://github.com/ethereum/eth2.0-specs/blob/v0.12.1/specs/phase0/fork-choice.md#ethereum-20-phase-0----beacon-chain-fork-choice @@ -258,6 +266,8 @@ pub struct ForkChoice { proto_array: ProtoArrayForkChoice, /// Attestations that arrived at the current slot and must be queued for later processing. queued_attestations: Vec, + /// Stores a cache of the values required to be sent to the execution layer. + forkchoice_update_parameters: Option, _phantom: PhantomData, } @@ -332,6 +342,7 @@ where fc_store, proto_array, queued_attestations: vec![], + forkchoice_update_parameters: None, _phantom: PhantomData, }) } @@ -349,10 +360,20 @@ where fc_store, proto_array, queued_attestations, + forkchoice_update_parameters: None, _phantom: PhantomData, } } + /// Returns cached information that can be used to issue a `forkchoiceUpdated` message to an + /// execution engine. + /// + /// These values are updated each time `Self::get_head` is called. May return `None` if + /// `Self::get_head` has not yet been called. + pub fn get_forkchoice_update_parameters(&self) -> Option { + self.forkchoice_update_parameters + } + /// Returns the block root of an ancestor of `block_root` at the given `slot`. (Note: `slot` refers /// to the block that is *returned*, not the one that is supplied.) /// @@ -414,15 +435,29 @@ where let store = &mut self.fc_store; - self.proto_array - .find_head::( - *store.justified_checkpoint(), - *store.finalized_checkpoint(), - store.justified_balances(), - store.proposer_boost_root(), - spec, - ) - .map_err(Into::into) + let head_root = self.proto_array.find_head::( + *store.justified_checkpoint(), + *store.finalized_checkpoint(), + store.justified_balances(), + store.proposer_boost_root(), + spec, + )?; + + // Cache some values for the next forkchoiceUpdate call to the execution layer. + let head_hash = self + .get_block(&head_root) + .and_then(|b| b.execution_status.block_hash()); + let finalized_root = self.finalized_checkpoint().root; + let finalized_hash = self + .get_block(&finalized_root) + .and_then(|b| b.execution_status.block_hash()); + self.forkchoice_update_parameters = Some(ForkchoiceUpdateParameters { + head_root, + head_hash, + finalized_hash, + }); + + Ok(head_root) } /// Returns `true` if the given `store` should be updated to set @@ -1049,6 +1084,7 @@ where fc_store, proto_array, queued_attestations: persisted.queued_attestations, + forkchoice_update_parameters: None, _phantom: PhantomData, }) }