From 58c7fe22108fd66dd6199b9058d5f0104b60f506 Mon Sep 17 00:00:00 2001 From: Roman Krasiuk Date: Mon, 5 Feb 2024 10:15:32 +0100 Subject: [PATCH] fix(engine): skip payload building on already canonical --- crates/consensus/beacon/src/engine/mod.rs | 79 +++++++++++++---------- 1 file changed, 45 insertions(+), 34 deletions(-) diff --git a/crates/consensus/beacon/src/engine/mod.rs b/crates/consensus/beacon/src/engine/mod.rs index 91c26861e8ec..d2643767e2d8 100644 --- a/crates/consensus/beacon/src/engine/mod.rs +++ b/crates/consensus/beacon/src/engine/mod.rs @@ -335,7 +335,7 @@ where fn forkchoice_updated( &mut self, state: ForkchoiceState, - attrs: Option, + mut attrs: Option, ) -> RethResult { trace!(target: "consensus::engine", ?state, "Received new forkchoice state update"); if state.head_block_hash.is_zero() { @@ -376,39 +376,14 @@ where Ok(outcome) => { match &outcome { CanonicalOutcome::AlreadyCanonical { header } => { - // On Optimism, the proposers are allowed to reorg their own chain at will. - cfg_if::cfg_if! { - if #[cfg(feature = "optimism")] { - if self.blockchain.chain_spec().is_optimism() { - debug!( - target: "consensus::engine", - fcu_head_num=?header.number, - current_head_num=?self.blockchain.canonical_tip().number, - "[Optimism] Allowing beacon reorg to old head" - ); - let _ = self.update_head(header.clone()); - self.listeners.notify( - BeaconConsensusEngineEvent::CanonicalChainCommitted( - Box::new(header.clone()), - elapsed, - ), - ); - } else { - debug!( - target: "consensus::engine", - fcu_head_num=?header.number, - current_head_num=?self.blockchain.canonical_tip().number, - "Ignoring beacon update to old head" - ); - } - } else { - debug!( - target: "consensus::engine", - fcu_head_num=?header.number, - current_head_num=?self.blockchain.canonical_tip().number, - "Ignoring beacon update to old head" - ); - } + if self.on_head_already_canonical(header, &mut attrs) { + let _ = self.update_head(header.clone()); + self.listeners.notify( + BeaconConsensusEngineEvent::CanonicalChainCommitted( + Box::new(header.clone()), + elapsed, + ), + ); } } CanonicalOutcome::Committed { head } => { @@ -473,6 +448,42 @@ where Ok(OnForkChoiceUpdated::valid(status)) } + /// Invoked when head hash references a `VALID` block that is already canonical. + /// + /// Returns `true` if the head needs to be updated. + fn on_head_already_canonical( + &mut self, + header: &SealedHeader, + attrs: &mut Option, + ) -> bool { + // On Optimism, the proposers are allowed to reorg their own chain at will. + #[cfg(feature = "optimism")] + if self.blockchain.chain_spec().is_optimism() { + debug!( + target: "consensus::engine", + fcu_head_num=?header.number, + current_head_num=?self.blockchain.canonical_tip().number, + "[Optimism] Allowing beacon reorg to old head" + ); + return true + } + + // 2. Client software MAY skip an update of the forkchoice state and MUST NOT begin a + // payload build process if `forkchoiceState.headBlockHash` references a `VALID` ancestor + // of the head of canonical chain, i.e. the ancestor passed payload validation process + // and deemed `VALID`. In the case of such an event, client software MUST return + // `{payloadStatus: {status: VALID, latestValidHash: forkchoiceState.headBlockHash, + // validationError: null}, payloadId: null}` + attrs.take(); + debug!( + target: "consensus::engine", + fcu_head_num=?header.number, + current_head_num=?self.blockchain.canonical_tip().number, + "Ignoring beacon update to old head" + ); + false + } + /// Invoked when we receive a new forkchoice update message. /// /// Returns `true` if the engine now reached its maximum block number, See