From eb9565d22dae8f3c279002695b9bb72fc8abd6a9 Mon Sep 17 00:00:00 2001 From: Matthias Seitz Date: Thu, 10 Oct 2024 11:39:40 +0200 Subject: [PATCH] fix: always handle payload building for opstack (#11629) --- crates/chainspec/src/api.rs | 9 +++++++++ crates/engine/service/Cargo.toml | 1 + crates/engine/service/src/service.rs | 7 ++++++- crates/engine/tree/src/engine.rs | 17 +++++++++++++++-- crates/engine/tree/src/tree/mod.rs | 24 +++++++++++++++++++++--- crates/optimism/chainspec/src/lib.rs | 4 ++++ 6 files changed, 56 insertions(+), 6 deletions(-) diff --git a/crates/chainspec/src/api.rs b/crates/chainspec/src/api.rs index fb64e08ae1..e481fbede7 100644 --- a/crates/chainspec/src/api.rs +++ b/crates/chainspec/src/api.rs @@ -46,6 +46,11 @@ pub trait EthChainSpec: Send + Sync + Unpin + Debug { /// The bootnodes for the chain, if any. fn bootnodes(&self) -> Option>; + + /// Returns `true` if this chain contains Optimism configuration. + fn is_optimism(&self) -> bool { + self.chain().is_optimism() + } } impl EthChainSpec for ChainSpec { @@ -92,4 +97,8 @@ impl EthChainSpec for ChainSpec { fn bootnodes(&self) -> Option> { self.bootnodes() } + + fn is_optimism(&self) -> bool { + Self::is_optimism(self) + } } diff --git a/crates/engine/service/Cargo.toml b/crates/engine/service/Cargo.toml index 63d5f1fb97..c6098bfe66 100644 --- a/crates/engine/service/Cargo.toml +++ b/crates/engine/service/Cargo.toml @@ -24,6 +24,7 @@ reth-prune.workspace = true reth-stages-api.workspace = true reth-tasks.workspace = true reth-node-types.workspace = true +reth-chainspec.workspace = true # async futures.workspace = true diff --git a/crates/engine/service/src/service.rs b/crates/engine/service/src/service.rs index 880be5c026..ed9c1aa1c2 100644 --- a/crates/engine/service/src/service.rs +++ b/crates/engine/service/src/service.rs @@ -1,11 +1,12 @@ use futures::{Stream, StreamExt}; use pin_project::pin_project; use reth_beacon_consensus::{BeaconConsensusEngineEvent, BeaconEngineMessage, EngineNodeTypes}; +use reth_chainspec::EthChainSpec; use reth_consensus::Consensus; use reth_engine_tree::{ backfill::PipelineSync, download::BasicBlockDownloader, - engine::{EngineApiRequest, EngineApiRequestHandler, EngineHandler}, + engine::{EngineApiKind, EngineApiRequest, EngineApiRequestHandler, EngineHandler}, persistence::PersistenceHandle, tree::{EngineApiTreeHandler, InvalidBlockHook, TreeConfig}, }; @@ -79,6 +80,9 @@ where invalid_block_hook: Box, sync_metrics_tx: MetricEventsSender, ) -> Self { + let engine_kind = + if chain_spec.is_optimism() { EngineApiKind::OpStack } else { EngineApiKind::Ethereum }; + let downloader = BasicBlockDownloader::new(client, consensus.clone()); let persistence_handle = @@ -97,6 +101,7 @@ where canonical_in_memory_state, tree_config, invalid_block_hook, + engine_kind, ); let engine_handler = EngineApiRequestHandler::new(to_tree_tx, from_tree); diff --git a/crates/engine/tree/src/engine.rs b/crates/engine/tree/src/engine.rs index 8172a44691..c1571ed821 100644 --- a/crates/engine/tree/src/engine.rs +++ b/crates/engine/tree/src/engine.rs @@ -212,15 +212,28 @@ where } } -/// The type for specifying the kind of engine api -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +/// The type for specifying the kind of engine api. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] pub enum EngineApiKind { /// The chain contains Ethereum configuration. + #[default] Ethereum, /// The chain contains Optimism configuration. OpStack, } +impl EngineApiKind { + /// Returns true if this is the ethereum variant + pub const fn is_ethereum(&self) -> bool { + matches!(self, Self::Ethereum) + } + + /// Returns true if this is the ethereum variant + pub const fn is_opstack(&self) -> bool { + matches!(self, Self::OpStack) + } +} + /// The request variants that the engine API handler can receive. #[derive(Debug)] pub enum EngineApiRequest { diff --git a/crates/engine/tree/src/tree/mod.rs b/crates/engine/tree/src/tree/mod.rs index a4491fde29..bc1da63694 100644 --- a/crates/engine/tree/src/tree/mod.rs +++ b/crates/engine/tree/src/tree/mod.rs @@ -66,7 +66,10 @@ pub mod config; mod invalid_block_hook; mod metrics; mod persistence_state; -use crate::{engine::EngineApiRequest, tree::metrics::EngineApiMetrics}; +use crate::{ + engine::{EngineApiKind, EngineApiRequest}, + tree::metrics::EngineApiMetrics, +}; pub use config::TreeConfig; pub use invalid_block_hook::{InvalidBlockHooks, NoopInvalidBlockHook}; pub use persistence_state::PersistenceState; @@ -498,6 +501,8 @@ pub struct EngineApiTreeHandler { metrics: EngineApiMetrics, /// An invalid block hook. invalid_block_hook: Box, + /// The engine API variant of this handler + engine_kind: EngineApiKind, } impl std::fmt::Debug @@ -519,6 +524,7 @@ impl std::fmt::Debug .field("config", &self.config) .field("metrics", &self.metrics) .field("invalid_block_hook", &format!("{:p}", self.invalid_block_hook)) + .field("engine_kind", &self.engine_kind) .finish() } } @@ -545,6 +551,7 @@ where persistence_state: PersistenceState, payload_builder: PayloadBuilderHandle, config: TreeConfig, + engine_kind: EngineApiKind, ) -> Self { let (incoming_tx, incoming) = std::sync::mpsc::channel(); @@ -565,6 +572,7 @@ where metrics: Default::default(), incoming_tx, invalid_block_hook: Box::new(NoopInvalidBlockHook), + engine_kind, } } @@ -589,6 +597,7 @@ where canonical_in_memory_state: CanonicalInMemoryState, config: TreeConfig, invalid_block_hook: Box, + kind: EngineApiKind, ) -> (Sender>>, UnboundedReceiver) { let best_block_number = provider.best_block_number().unwrap_or(0); let header = provider.sealed_header(best_block_number).ok().flatten().unwrap_or_default(); @@ -618,6 +627,7 @@ where persistence_state, payload_builder, config, + kind, ); task.set_invalid_block_hook(invalid_block_hook); let incoming = task.incoming_tx.clone(); @@ -1041,8 +1051,15 @@ where if let Ok(Some(canonical_header)) = self.find_canonical_header(state.head_block_hash) { debug!(target: "engine::tree", head = canonical_header.number, "fcu head block is already canonical"); - // TODO(mattsse): for optimism we'd need to trigger a build job as well because on - // Optimism, the proposers are allowed to reorg their own chain at will. + // For OpStack the proposers are allowed to reorg their own chain at will, so we need to + // always trigger a new payload job if requested. + if self.engine_kind.is_opstack() { + if let Some(attr) = attrs { + debug!(target: "engine::tree", head = canonical_header.number, "handling payload attributes for canonical head"); + let updated = self.process_payload_attributes(attr, &canonical_header, state); + return Ok(TreeOutcome::new(updated)) + } + } // 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` @@ -2680,6 +2697,7 @@ mod tests { PersistenceState::default(), payload_builder, TreeConfig::default(), + EngineApiKind::Ethereum, ); let block_builder = TestBlockBuilder::default().with_chain_spec((*chain_spec).clone()); diff --git a/crates/optimism/chainspec/src/lib.rs b/crates/optimism/chainspec/src/lib.rs index ebd9392b53..9b2d1c8c11 100644 --- a/crates/optimism/chainspec/src/lib.rs +++ b/crates/optimism/chainspec/src/lib.rs @@ -93,6 +93,10 @@ impl EthChainSpec for OpChainSpec { fn bootnodes(&self) -> Option> { self.inner.bootnodes() } + + fn is_optimism(&self) -> bool { + true + } } impl Hardforks for OpChainSpec {