From 4ceef0fd95386e298980a31d525348e69907f3ce Mon Sep 17 00:00:00 2001 From: op-will <232669456+op-will@users.noreply.github.com> Date: Wed, 5 Nov 2025 10:08:49 -0600 Subject: [PATCH 01/19] Splitting build and seal into separate engine tasks The main goal of this change is to allow engine processing to continue after block building has been initiated but before it has been sealed. This is accomplished by - Updating the EngineActor to - Make the BuildTask only initiate block building - Add a new SealTask to seal and insert the initiated block - Updating the SequencerActor loop to seal the last block and start the next block. Importantly the build_block function no longer starts and completes block building in a single call but rather seals the last block if there was one, and starts the next. This means that the time spent waiting for the next tick will be _during block building_. --- crates/node/engine/Cargo.toml | 2 +- crates/node/engine/src/lib.rs | 6 +- crates/node/engine/src/metrics/mod.rs | 2 + .../src/task_queue/tasks/build/error.rs | 5 +- .../engine/src/task_queue/tasks/build/task.rs | 236 ++------------- .../src/task_queue/tasks/consolidate/error.rs | 17 +- .../src/task_queue/tasks/consolidate/task.rs | 23 +- .../node/engine/src/task_queue/tasks/mod.rs | 6 + .../engine/src/task_queue/tasks/seal/error.rs | 117 ++++++++ .../engine/src/task_queue/tasks/seal/mod.rs | 7 + .../engine/src/task_queue/tasks/seal/task.rs | 268 ++++++++++++++++++ .../src/task_queue/tasks/synchronize/task.rs | 2 +- .../node/engine/src/task_queue/tasks/task.rs | 21 +- .../node/engine/src/task_queue/tasks/util.rs | 88 ++++++ .../node/service/src/actors/engine/actor.rs | 67 +++-- .../service/src/actors/sequencer/actor.rs | 216 +++++++++----- crates/node/service/src/metrics/mod.rs | 20 +- crates/node/service/src/service/core.rs | 4 + .../grafana/dashboards/overview.json | 6 +- 19 files changed, 780 insertions(+), 333 deletions(-) create mode 100644 crates/node/engine/src/task_queue/tasks/seal/error.rs create mode 100644 crates/node/engine/src/task_queue/tasks/seal/mod.rs create mode 100644 crates/node/engine/src/task_queue/tasks/seal/task.rs create mode 100644 crates/node/engine/src/task_queue/tasks/util.rs diff --git a/crates/node/engine/Cargo.toml b/crates/node/engine/Cargo.toml index 66ec2bd4bb..a6f0e878c6 100644 --- a/crates/node/engine/Cargo.toml +++ b/crates/node/engine/Cargo.toml @@ -48,7 +48,7 @@ thiserror.workspace = true url.workspace = true tower.workspace = true http-body-util.workspace = true -derive_more = { workspace = true, features = ["display", "deref", "from_str"] } +derive_more = { workspace = true, features = ["display", "deref", "from_str", "constructor"] } serde_json.workspace = true # metrics diff --git a/crates/node/engine/src/lib.rs b/crates/node/engine/src/lib.rs index b18b8135c7..f730c9ebc0 100644 --- a/crates/node/engine/src/lib.rs +++ b/crates/node/engine/src/lib.rs @@ -41,9 +41,9 @@ extern crate tracing; mod task_queue; pub use task_queue::{ BuildTask, BuildTaskError, ConsolidateTask, ConsolidateTaskError, Engine, EngineBuildError, - EngineResetError, EngineTask, EngineTaskError, EngineTaskErrorSeverity, EngineTaskErrors, - EngineTaskExt, FinalizeTask, FinalizeTaskError, InsertTask, InsertTaskError, SynchronizeTask, - SynchronizeTaskError, + EngineResetError, EngineSealError, EngineTask, EngineTaskError, EngineTaskErrorSeverity, + EngineTaskErrors, EngineTaskExt, FinalizeTask, FinalizeTaskError, InsertTask, InsertTaskError, + SealTask, SealTaskError, SynchronizeTask, SynchronizeTaskError, }; mod attributes; diff --git a/crates/node/engine/src/metrics/mod.rs b/crates/node/engine/src/metrics/mod.rs index 4ede3e14eb..87a65e372e 100644 --- a/crates/node/engine/src/metrics/mod.rs +++ b/crates/node/engine/src/metrics/mod.rs @@ -57,6 +57,8 @@ impl Metrics { pub const FORKCHOICE_TASK_LABEL: &str = "forkchoice-update"; /// Build task label. pub const BUILD_TASK_LABEL: &str = "build"; + /// Seal task label. + pub const SEAL_TASK_LABEL: &str = "seal"; /// Finalize task label. pub const FINALIZE_TASK_LABEL: &str = "finalize"; diff --git a/crates/node/engine/src/task_queue/tasks/build/error.rs b/crates/node/engine/src/task_queue/tasks/build/error.rs index a33b53b5af..a74639adad 100644 --- a/crates/node/engine/src/task_queue/tasks/build/error.rs +++ b/crates/node/engine/src/task_queue/tasks/build/error.rs @@ -4,10 +4,9 @@ use crate::{ EngineTaskError, InsertTaskError, SynchronizeTaskError, task_queue::tasks::task::EngineTaskErrorSeverity, }; -use alloy_rpc_types_engine::PayloadStatusEnum; +use alloy_rpc_types_engine::{PayloadId, PayloadStatusEnum}; use alloy_transport::{RpcError, TransportErrorKind}; use kona_protocol::FromBlockError; -use op_alloy_rpc_types_engine::OpExecutionPayloadEnvelope; use thiserror::Error; use tokio::sync::mpsc; @@ -80,7 +79,7 @@ pub enum BuildTaskError { FromBlock(#[from] FromBlockError), /// Error sending the built payload envelope. #[error(transparent)] - MpscSend(#[from] Box>), + MpscSend(#[from] Box>), /// The clock went backwards. #[error("The clock went backwards")] ClockWentBackwards, diff --git a/crates/node/engine/src/task_queue/tasks/build/task.rs b/crates/node/engine/src/task_queue/tasks/build/task.rs index 1e71bb239b..45e87fec62 100644 --- a/crates/node/engine/src/task_queue/tasks/build/task.rs +++ b/crates/node/engine/src/task_queue/tasks/build/task.rs @@ -1,49 +1,30 @@ //! A task for building a new block and importing it. use super::BuildTaskError; -use crate::{ - EngineClient, EngineForkchoiceVersion, EngineGetPayloadVersion, EngineState, EngineTaskExt, - InsertTask, - InsertTaskError::{self}, - state::EngineSyncStateUpdate, - task_queue::tasks::build::error::EngineBuildError, -}; -use alloy_rpc_types_engine::{ExecutionPayload, PayloadId, PayloadStatusEnum}; +use crate::{EngineClient, EngineForkchoiceVersion, EngineState, EngineTaskExt, state::EngineSyncStateUpdate, task_queue::tasks::build::error::EngineBuildError}; +use alloy_rpc_types_engine::{PayloadId, PayloadStatusEnum}; use async_trait::async_trait; +use derive_more::{Constructor}; use kona_genesis::RollupConfig; -use kona_protocol::{L2BlockInfo, OpAttributesWithParent}; +use kona_protocol::{OpAttributesWithParent}; use op_alloy_provider::ext::engine::OpEngineApi; -use op_alloy_rpc_types_engine::{OpExecutionPayload, OpExecutionPayloadEnvelope}; use std::{ sync::Arc, - time::{Duration, Instant, SystemTime}, + time::{Instant}, }; -use tokio::{sync::mpsc, time::sleep}; +use tokio::{sync::mpsc}; /// Task for building new blocks with automatic forkchoice synchronization. /// -/// The [`BuildTask`] handles the complete block building workflow, including: -/// -/// 1. **Automatic Forkchoice Updates**: Performs initial `engine_forkchoiceUpdated` call with -/// payload attributes to initiate block building on the execution layer -/// 2. **Payload Construction**: Retrieves the built payload using `engine_getPayload` -/// 3. **Block Import**: Imports the payload using [`InsertTask`] for canonicalization -/// -/// ## Forkchoice Integration -/// -/// Unlike previous versions where forkchoice updates required separate tasks, -/// `BuildTask` now handles forkchoice synchronization automatically as part of -/// the block building process. This eliminates the need for explicit forkchoice -/// management and ensures atomic block building operations. +/// The [`BuildTask`] only performs the `engine_forkchoiceUpdated` call within the block building +/// workflow. It makes this call with the provided attributes to initiate block building on the +/// execution layer and, if successful, sends the new [`PayloadId`] via the configured sender. /// /// ## Error Handling /// -/// The task uses [`EngineBuildError`] for build-specific failures during the forkchoice -/// update phase, and delegates to [`InsertTaskError`] for payload import failures. +/// The task uses [`EngineBuildError`] for build-specific failures during the forkchoice update phase. /// -/// [`InsertTask`]: crate::InsertTask /// [`EngineBuildError`]: crate::EngineBuildError -/// [`InsertTaskError`]: crate::InsertTaskError -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Constructor)] pub struct BuildTask { /// The engine API client. pub engine: Arc, @@ -51,25 +32,11 @@ pub struct BuildTask { pub cfg: Arc, /// The [`OpAttributesWithParent`] to instruct the execution layer to build. pub attributes: OpAttributesWithParent, - /// Whether or not the payload was derived, or created by the sequencer. - pub is_attributes_derived: bool, - /// An optional channel to send the built [`OpExecutionPayloadEnvelope`] to, after the block - /// has been built, imported, and canonicalized. - pub payload_tx: Option>, + /// The optional sender to which [`PayloadId`] will be sent after the block build has been started. + pub payload_id_tx: Option>, } impl BuildTask { - /// Creates a new block building task. - pub const fn new( - engine: Arc, - cfg: Arc, - attributes: OpAttributesWithParent, - is_attributes_derived: bool, - payload_tx: Option>, - ) -> Self { - Self { engine, cfg, attributes, is_attributes_derived, payload_tx } - } - /// Starts the block building process by sending an initial `engine_forkchoiceUpdate` call with /// the payload attributes to build. /// @@ -169,84 +136,15 @@ impl BuildTask { .payload_id .ok_or(BuildTaskError::EngineBuildError(EngineBuildError::MissingPayloadId)) } - - /// Fetches the execution payload from the EL. - /// - /// ## Engine Method Selection - /// The method used to fetch the payload from the EL is determined by the payload timestamp. The - /// method used to import the payload into the engine is determined by the payload version. - /// - /// - `engine_getPayloadV2` is used for payloads with a timestamp before the Ecotone fork. - /// - `engine_getPayloadV3` is used for payloads with a timestamp after the Ecotone fork. - /// - `engine_getPayloadV4` is used for payloads with a timestamp after the Isthmus fork. - async fn fetch_payload( - &self, - cfg: &RollupConfig, - engine: &EngineClient, - payload_id: PayloadId, - payload_attrs: OpAttributesWithParent, - ) -> Result { - let payload_timestamp = payload_attrs.inner().payload_attributes.timestamp; - - debug!( - target: "engine_builder", - payload_id = payload_id.to_string(), - l2_time = payload_timestamp, - "Inserting payload" - ); - - let get_payload_version = EngineGetPayloadVersion::from_cfg(cfg, payload_timestamp); - let payload_envelope = match get_payload_version { - EngineGetPayloadVersion::V4 => { - let payload = engine.get_payload_v4(payload_id).await.map_err(|e| { - error!(target: "engine_builder", "Payload fetch failed: {e}"); - BuildTaskError::GetPayloadFailed(e) - })?; - - OpExecutionPayloadEnvelope { - parent_beacon_block_root: Some(payload.parent_beacon_block_root), - execution_payload: OpExecutionPayload::V4(payload.execution_payload), - } - } - EngineGetPayloadVersion::V3 => { - let payload = engine.get_payload_v3(payload_id).await.map_err(|e| { - error!(target: "engine_builder", "Payload fetch failed: {e}"); - BuildTaskError::GetPayloadFailed(e) - })?; - - OpExecutionPayloadEnvelope { - parent_beacon_block_root: Some(payload.parent_beacon_block_root), - execution_payload: OpExecutionPayload::V3(payload.execution_payload), - } - } - EngineGetPayloadVersion::V2 => { - let payload = engine.get_payload_v2(payload_id).await.map_err(|e| { - error!(target: "engine_builder", "Payload fetch failed: {e}"); - BuildTaskError::GetPayloadFailed(e) - })?; - - OpExecutionPayloadEnvelope { - parent_beacon_block_root: None, - execution_payload: match payload.execution_payload.into_payload() { - ExecutionPayload::V1(payload) => OpExecutionPayload::V1(payload), - ExecutionPayload::V2(payload) => OpExecutionPayload::V2(payload), - _ => unreachable!("the response should be a V1 or V2 payload"), - }, - } - } - }; - - Ok(payload_envelope) - } } #[async_trait] impl EngineTaskExt for BuildTask { - type Output = (); + type Output = PayloadId; type Error = BuildTaskError; - async fn execute(&self, state: &mut EngineState) -> Result<(), BuildTaskError> { + async fn execute(&self, state: &mut EngineState) -> Result { debug!( target: "engine_builder", txs = self.attributes.inner().transactions.as_ref().map_or(0, |txs| txs.len()), @@ -258,109 +156,19 @@ impl EngineTaskExt for BuildTask { // payload attributes. let fcu_start_time = Instant::now(); let payload_id = self.start_build(state, &self.engine, self.attributes.clone()).await?; - let fcu_duration = fcu_start_time.elapsed(); - // Compute the time of the next block. - let next_block = Duration::from_secs( - self.attributes.parent().block_info.timestamp.saturating_add(self.cfg.block_time), - ); - - // Compute the time left to seal the next block. - let now = SystemTime::now() - .duration_since(SystemTime::UNIX_EPOCH) - .map_err(|_| BuildTaskError::ClockWentBackwards)?; - - // Add a buffer to the time left to seal the next block. - const SEALING_BUFFER: Duration = Duration::from_millis(50); - - let time_left_to_seal = next_block.saturating_sub(now).saturating_sub(SEALING_BUFFER); - - // Wait for the time left to seal the next block. - if !time_left_to_seal.is_zero() { - sleep(time_left_to_seal).await; - } - - // Fetch the payload just inserted from the EL and import it into the engine. - let block_import_start_time = Instant::now(); - let new_payload = self - .fetch_payload(&self.cfg, &self.engine, payload_id, self.attributes.clone()) - .await?; - - let new_block_ref = L2BlockInfo::from_payload_and_genesis( - new_payload.execution_payload.clone(), - self.attributes.inner().payload_attributes.parent_beacon_block_root, - &self.cfg.genesis, - ) - .map_err(BuildTaskError::FromBlock)?; - - // Insert the new block into the engine. - match InsertTask::new( - Arc::clone(&self.engine), - self.cfg.clone(), - new_payload.clone(), - self.is_attributes_derived, - ) - .execute(state) - .await - { - Err(InsertTaskError::UnexpectedPayloadStatus(e)) - if self.attributes.is_deposits_only() => - { - error!(target: "engine_builder", error = ?e, "Critical: Deposit-only payload import failed"); - return Err(BuildTaskError::DepositOnlyPayloadFailed); - } - // HOLOCENE: Re-attempt payload import with deposits only - Err(InsertTaskError::UnexpectedPayloadStatus(e)) - if self - .cfg - .is_holocene_active(self.attributes.inner().payload_attributes.timestamp) => - { - warn!(target: "engine_builder", error = ?e, "Re-attempting payload import with deposits only."); - // HOLOCENE: Re-attempt payload import with deposits only - match Self::new( - self.engine.clone(), - self.cfg.clone(), - self.attributes.as_deposits_only(), - self.is_attributes_derived, - self.payload_tx.clone(), - ) - .execute(state) - .await - { - Ok(_) => { - info!(target: "engine_builder", "Successfully imported deposits-only payload") - } - Err(_) => return Err(BuildTaskError::DepositOnlyPayloadReattemptFailed), - } - return Err(BuildTaskError::HoloceneInvalidFlush); - } - Err(e) => { - error!(target: "engine_builder", "Payload import failed: {e}"); - return Err(Box::new(e).into()); - } - Ok(_) => { - info!(target: "engine_builder", "Successfully imported payload") - } - } - - let block_import_duration = block_import_start_time.elapsed(); - - // If a channel was provided, send the built payload envelope to it. - if let Some(tx) = &self.payload_tx { - tx.send(new_payload).await.map_err(Box::new).map_err(BuildTaskError::MpscSend)?; - } - info!( target: "engine_builder", - l2_number = new_block_ref.block_info.number, - l2_time = new_block_ref.block_info.timestamp, fcu_duration = ?fcu_duration, - block_import_duration = ?block_import_duration, - "Built and imported new {} block", - if self.is_attributes_derived { "safe" } else { "unsafe" }, + "block build started" ); - Ok(()) + // If a channel was provided, send the payload ID to it. + if let Some(tx) = &self.payload_id_tx { + tx.send(payload_id).await.map_err(Box::new).map_err(BuildTaskError::MpscSend)?; + } + + Ok(payload_id) } } diff --git a/crates/node/engine/src/task_queue/tasks/consolidate/error.rs b/crates/node/engine/src/task_queue/tasks/consolidate/error.rs index 7eb9a41097..935ffbfa8b 100644 --- a/crates/node/engine/src/task_queue/tasks/consolidate/error.rs +++ b/crates/node/engine/src/task_queue/tasks/consolidate/error.rs @@ -1,8 +1,8 @@ //! Contains error types for the [`crate::ConsolidateTask`]. use crate::{ - BuildTaskError, EngineTaskError, SynchronizeTaskError, - task_queue::tasks::task::EngineTaskErrorSeverity, + BuildTaskError, EngineTaskError, SealTaskError, SynchronizeTaskError, + task_queue::tasks::{task::EngineTaskErrorSeverity, BuildAndSealError}, }; use thiserror::Error; @@ -18,17 +18,30 @@ pub enum ConsolidateTaskError { /// The build task failed. #[error(transparent)] BuildTaskFailed(#[from] BuildTaskError), + /// The seal task failed. + #[error(transparent)] + SealTaskFailed(#[from] SealTaskError), /// The consolidation forkchoice update call to the engine api failed. #[error(transparent)] ForkchoiceUpdateFailed(#[from] SynchronizeTaskError), } +impl From for ConsolidateTaskError { + fn from(err: BuildAndSealError) -> Self { + match err { + BuildAndSealError::Build(e) => Self::BuildTaskFailed(e), + BuildAndSealError::Seal(e) => Self::SealTaskFailed(e), + } + } +} + impl EngineTaskError for ConsolidateTaskError { fn severity(&self) -> EngineTaskErrorSeverity { match self { Self::MissingUnsafeL2Block(_) => EngineTaskErrorSeverity::Reset, Self::FailedToFetchUnsafeL2Block => EngineTaskErrorSeverity::Temporary, Self::BuildTaskFailed(inner) => inner.severity(), + Self::SealTaskFailed(inner) => inner.severity(), Self::ForkchoiceUpdateFailed(inner) => inner.severity(), } } diff --git a/crates/node/engine/src/task_queue/tasks/consolidate/task.rs b/crates/node/engine/src/task_queue/tasks/consolidate/task.rs index 3c76eeb8c3..44d853afd9 100644 --- a/crates/node/engine/src/task_queue/tasks/consolidate/task.rs +++ b/crates/node/engine/src/task_queue/tasks/consolidate/task.rs @@ -1,8 +1,7 @@ //! A task to consolidate the engine state. use crate::{ - BuildTask, ConsolidateTaskError, EngineClient, EngineState, EngineTaskExt, SynchronizeTask, - state::EngineSyncStateUpdate, + state::EngineSyncStateUpdate, task_queue::build_and_seal, ConsolidateTaskError, EngineClient, EngineState, EngineTaskExt, SynchronizeTask }; use async_trait::async_trait; use kona_genesis::RollupConfig; @@ -12,7 +11,8 @@ use std::{sync::Arc, time::Instant}; /// The [`ConsolidateTask`] attempts to consolidate the engine state /// using the specified payload attributes and the oldest unsafe head. /// -/// If consolidation fails, payload attributes processing is attempted using the [`BuildTask`]. +/// If consolidation fails, payload attributes processing is attempted using the [`BuildTask`] +/// followed by the [`SealTask`]. #[derive(Debug, Clone)] pub struct ConsolidateTask { /// The engine client. @@ -36,20 +36,13 @@ impl ConsolidateTask { Self { client, cfg: config, attributes, is_attributes_derived } } - /// Executes a new [`BuildTask`]. + /// Executes a new [`BuildTask`] followed by a [`SealTask`]. /// This is used when the [`ConsolidateTask`] fails to consolidate the engine state. - async fn execute_build_task( + async fn execute_build_and_seal_tasks( &self, state: &mut EngineState, ) -> Result<(), ConsolidateTaskError> { - let build_task = BuildTask::new( - self.client.clone(), - self.cfg.clone(), - self.attributes.clone(), - self.is_attributes_derived, - None, - ); - Ok(build_task.execute(state).await?) + build_and_seal(state, self.client.clone(), self.cfg.clone(), self.attributes.clone(), self.is_attributes_derived, None).await.map_err(Into::into) } /// Attempts consolidation on the engine state. @@ -158,7 +151,7 @@ impl ConsolidateTask { block_hash = %block_hash, "Attributes mismatch! Executing build task to initiate reorg", ); - self.execute_build_task(state).await + self.execute_build_and_seal_tasks(state).await } } @@ -175,7 +168,7 @@ impl EngineTaskExt for ConsolidateTask { { self.consolidate(state).await } else { - self.execute_build_task(state).await + self.execute_build_and_seal_tasks(state).await } } } diff --git a/crates/node/engine/src/task_queue/tasks/mod.rs b/crates/node/engine/src/task_queue/tasks/mod.rs index c59a9bba38..99b7d47620 100644 --- a/crates/node/engine/src/task_queue/tasks/mod.rs +++ b/crates/node/engine/src/task_queue/tasks/mod.rs @@ -14,8 +14,14 @@ pub use insert::{InsertTask, InsertTaskError}; mod build; pub use build::{BuildTask, BuildTaskError, EngineBuildError}; +mod seal; +pub use seal::{SealTask, SealTaskError, EngineSealError}; + mod consolidate; pub use consolidate::{ConsolidateTask, ConsolidateTaskError}; mod finalize; pub use finalize::{FinalizeTask, FinalizeTaskError}; + +mod util; +pub(super) use util::{build_and_seal, BuildAndSealError}; diff --git a/crates/node/engine/src/task_queue/tasks/seal/error.rs b/crates/node/engine/src/task_queue/tasks/seal/error.rs new file mode 100644 index 0000000000..2d1c41427e --- /dev/null +++ b/crates/node/engine/src/task_queue/tasks/seal/error.rs @@ -0,0 +1,117 @@ +//! Contains error types for the [crate::SynchronizeTask]. + +use crate::{ + EngineTaskError, InsertTaskError, + task_queue::tasks::task::EngineTaskErrorSeverity, +}; +use alloy_rpc_types_engine::PayloadStatusEnum; +use alloy_transport::{RpcError, TransportErrorKind}; +use kona_protocol::FromBlockError; +use op_alloy_rpc_types_engine::OpExecutionPayloadEnvelope; +use thiserror::Error; +use tokio::sync::mpsc; + +/// An error that occurs during payload building within the engine. +/// +/// This error type is specific to the block building process and represents failures +/// that can occur during the automatic forkchoice update phase of [`SealTask`]. +/// Unlike [`SealTaskError`], which handles higher-level build orchestration errors, +/// `EngineSealError` focuses on low-level engine API communication failures. +/// +/// ## Error Categories +/// +/// - **State Validation**: Errors related to inconsistent chain state +/// - **Engine Communication**: RPC failures during forkchoice updates +/// - **Payload Validation**: Invalid payload status responses from the execution layer +/// +/// [`SealTask`]: crate::SealTask +#[derive(Debug, Error)] +pub enum EngineSealError { + /// The finalized head is ahead of the unsafe head. + #[error("Finalized head is ahead of unsafe head")] + FinalizedAheadOfUnsafe(u64, u64), + /// The forkchoice update call to the engine api failed. + #[error("Failed to build payload attributes in the engine. Forkchoice RPC error: {0}")] + AttributesInsertionFailed(#[from] RpcError), + /// The inserted payload is invalid. + #[error("The inserted payload is invalid: {0}")] + InvalidPayload(String), + /// The inserted payload status is unexpected. + #[error("The inserted payload status is unexpected: {0}")] + UnexpectedPayloadStatus(PayloadStatusEnum), + /// The payload ID is missing. + #[error("The inserted payload ID is missing")] + MissingPayloadId, + /// The engine is syncing. + #[error("The engine is syncing")] + EngineSyncing, +} + +/// An error that occurs when running the [crate::SynchronizeTask]. +#[derive(Debug, Error)] +pub enum SealTaskError { + /// An error occurred when sealing the payload attributes in the engine. + #[error("An error occurred when sealing the payload attributes in the engine.")] + EngineSealError(EngineSealError), + /// Impossible to insert the payload into the engine. + #[error(transparent)] + PayloadInsertionFailed(#[from] Box), + /// The get payload call to the engine api failed. + #[error(transparent)] + GetPayloadFailed(RpcError), + /// A deposit-only payload failed to import. + #[error("Deposit-only payload failed to import")] + DepositOnlyPayloadFailed, + /// Failed to re-attempt payload import with deposit-only payload. + #[error("Failed to re-attempt payload import with deposit-only payload")] + DepositOnlyPayloadReattemptFailed, + /// The payload is invalid, and the derivation pipeline must + /// be flushed post-holocene. + #[error("Invalid payload, must flush post-holocene")] + HoloceneInvalidFlush, + /// Failed to convert a [`OpExecutionPayload`] to a [`L2BlockInfo`]. + /// + /// [`OpExecutionPayload`]: op_alloy_rpc_types_engine::OpExecutionPayload + /// [`L2BlockInfo`]: kona_protocol::L2BlockInfo + #[error(transparent)] + FromBlock(#[from] FromBlockError), + /// Error sending the built payload envelope. + #[error(transparent)] + MpscSend(#[from] Box>), + /// The clock went backwards. + #[error("The clock went backwards")] + ClockWentBackwards, +} + +impl EngineTaskError for SealTaskError { + fn severity(&self) -> EngineTaskErrorSeverity { + match self { + Self::PayloadInsertionFailed(inner) => inner.severity(), + Self::EngineSealError(EngineSealError::FinalizedAheadOfUnsafe(_, _)) => { + EngineTaskErrorSeverity::Critical + } + Self::EngineSealError(EngineSealError::AttributesInsertionFailed(_)) => { + EngineTaskErrorSeverity::Temporary + } + Self::EngineSealError(EngineSealError::InvalidPayload(_)) => { + EngineTaskErrorSeverity::Temporary + } + Self::EngineSealError(EngineSealError::UnexpectedPayloadStatus(_)) => { + EngineTaskErrorSeverity::Temporary + } + Self::EngineSealError(EngineSealError::MissingPayloadId) => { + EngineTaskErrorSeverity::Temporary + } + Self::EngineSealError(EngineSealError::EngineSyncing) => { + EngineTaskErrorSeverity::Temporary + } + Self::GetPayloadFailed(_) => EngineTaskErrorSeverity::Temporary, + Self::HoloceneInvalidFlush => EngineTaskErrorSeverity::Flush, + Self::DepositOnlyPayloadReattemptFailed => EngineTaskErrorSeverity::Critical, + Self::DepositOnlyPayloadFailed => EngineTaskErrorSeverity::Critical, + Self::FromBlock(_) => EngineTaskErrorSeverity::Critical, + Self::MpscSend(_) => EngineTaskErrorSeverity::Critical, + Self::ClockWentBackwards => EngineTaskErrorSeverity::Critical, + } + } +} diff --git a/crates/node/engine/src/task_queue/tasks/seal/mod.rs b/crates/node/engine/src/task_queue/tasks/seal/mod.rs new file mode 100644 index 0000000000..e7927e7419 --- /dev/null +++ b/crates/node/engine/src/task_queue/tasks/seal/mod.rs @@ -0,0 +1,7 @@ +//! Task and its associated types for importing a block that has been started. + +mod task; +pub use task::SealTask; + +mod error; +pub use error::{SealTaskError, EngineSealError}; diff --git a/crates/node/engine/src/task_queue/tasks/seal/task.rs b/crates/node/engine/src/task_queue/tasks/seal/task.rs new file mode 100644 index 0000000000..c701291ba6 --- /dev/null +++ b/crates/node/engine/src/task_queue/tasks/seal/task.rs @@ -0,0 +1,268 @@ +//! A task for importing a block that has already been started. +use super::SealTaskError; +use crate::{ + task_queue::build_and_seal, EngineClient, EngineGetPayloadVersion, EngineState, EngineTaskExt, InsertTask, InsertTaskError::{self} +}; +use alloy_rpc_types_engine::{ExecutionPayload, PayloadId}; +use async_trait::async_trait; +use kona_genesis::RollupConfig; +use kona_protocol::{L2BlockInfo, OpAttributesWithParent}; +use op_alloy_provider::ext::engine::OpEngineApi; +use op_alloy_rpc_types_engine::{OpExecutionPayload, OpExecutionPayloadEnvelope}; +use std::{ + sync::Arc, + time::{Duration, Instant, SystemTime}, +}; +use tokio::{sync::mpsc, time::sleep}; + +/// Task for sealing blocks. +/// +/// The [`SealTask`] handles the following parts of the block building workflow: +/// +/// 1. **Payload Construction**: Retrieves the built payload using `engine_getPayload` +/// 2. **Block Import**: Imports the payload using [`InsertTask`] for canonicalization +/// +/// ## Error Handling +/// +/// The task delegates to [`InsertTaskError`] for payload import failures. +/// +/// [`InsertTask`]: crate::InsertTask +/// [`InsertTaskError`]: crate::InsertTaskError +#[derive(Debug, Clone)] +pub struct SealTask { + /// The engine API client. + pub engine: Arc, + /// The [`RollupConfig`]. + pub cfg: Arc, + /// The [`PayloadId`] being sealed. + pub payload_id: PayloadId, + /// The [`OpAttributesWithParent`] to instruct the execution layer to build. + pub attributes: OpAttributesWithParent, + /// Whether or not the payload was derived, or created by the sequencer. + pub is_attributes_derived: bool, + /// An optional channel to send the built [`OpExecutionPayloadEnvelope`] to, after the block + /// has been built, imported, and canonicalized. + pub payload_tx: Option>, +} + +impl SealTask { + /// Creates a new block building task. + pub const fn new( + engine: Arc, + cfg: Arc, + payload_id: PayloadId, + attributes: OpAttributesWithParent, + is_attributes_derived: bool, + payload_tx: Option>, + ) -> Self { + Self { engine, cfg, payload_id, attributes, is_attributes_derived, payload_tx } + } + + /// Fetches the execution payload from the EL. + /// + /// ## Engine Method Selection + /// The method used to fetch the payload from the EL is determined by the payload timestamp. The + /// method used to import the payload into the engine is determined by the payload version. + /// + /// - `engine_getPayloadV2` is used for payloads with a timestamp before the Ecotone fork. + /// - `engine_getPayloadV3` is used for payloads with a timestamp after the Ecotone fork. + /// - `engine_getPayloadV4` is used for payloads with a timestamp after the Isthmus fork. + async fn fetch_payload( + &self, + cfg: &RollupConfig, + engine: &EngineClient, + payload_id: PayloadId, + payload_attrs: OpAttributesWithParent, + ) -> Result { + let payload_timestamp = payload_attrs.inner().payload_attributes.timestamp; + + debug!( + target: "engine_builder", + payload_id = payload_id.to_string(), + l2_time = payload_timestamp, + "Inserting payload" + ); + + let get_payload_version = EngineGetPayloadVersion::from_cfg(cfg, payload_timestamp); + let payload_envelope = match get_payload_version { + EngineGetPayloadVersion::V4 => { + let payload = engine.get_payload_v4(payload_id).await.map_err(|e| { + error!(target: "engine_builder", "Payload fetch failed: {e}"); + SealTaskError::GetPayloadFailed(e) + })?; + + OpExecutionPayloadEnvelope { + parent_beacon_block_root: Some(payload.parent_beacon_block_root), + execution_payload: OpExecutionPayload::V4(payload.execution_payload), + } + } + EngineGetPayloadVersion::V3 => { + let payload = engine.get_payload_v3(payload_id).await.map_err(|e| { + error!(target: "engine_builder", "Payload fetch failed: {e}"); + SealTaskError::GetPayloadFailed(e) + })?; + + OpExecutionPayloadEnvelope { + parent_beacon_block_root: Some(payload.parent_beacon_block_root), + execution_payload: OpExecutionPayload::V3(payload.execution_payload), + } + } + EngineGetPayloadVersion::V2 => { + let payload = engine.get_payload_v2(payload_id).await.map_err(|e| { + error!(target: "engine_builder", "Payload fetch failed: {e}"); + SealTaskError::GetPayloadFailed(e) + })?; + + OpExecutionPayloadEnvelope { + parent_beacon_block_root: None, + execution_payload: match payload.execution_payload.into_payload() { + ExecutionPayload::V1(payload) => OpExecutionPayload::V1(payload), + ExecutionPayload::V2(payload) => OpExecutionPayload::V2(payload), + _ => unreachable!("the response should be a V1 or V2 payload"), + }, + } + } + }; + + Ok(payload_envelope) + } + + + /// Seals the block by waiting for the appropriate time, fetching the payload, and importing it. + /// + /// This function handles: + /// 1. Computing and waiting for the seal time (with a buffer) + /// 2. Fetching the execution payload from the EL + /// 3. Importing the payload into the engine with Holocene fallback support + /// 4. Sending the payload to the optional channel + /// + /// Returns the imported block info and the duration taken to import the block. + async fn seal_block( + &self, + state: &mut EngineState, + ) -> Result<(L2BlockInfo, Duration), SealTaskError> { + // Compute the time of the next block. + let next_block = Duration::from_secs( + self.attributes.parent().block_info.timestamp.saturating_add(self.cfg.block_time), + ); + + // Compute the time left to seal the next block. + let now = SystemTime::now() + .duration_since(SystemTime::UNIX_EPOCH) + .map_err(|_| SealTaskError::ClockWentBackwards)?; + + // Add a buffer to the time left to seal the next block. + const SEALING_BUFFER: Duration = Duration::from_millis(50); + + let time_left_to_seal = next_block.saturating_sub(now).saturating_sub(SEALING_BUFFER); + + // Wait for the time left to seal the next block. + if !time_left_to_seal.is_zero() { + sleep(time_left_to_seal).await; + } + + // Fetch the payload just inserted from the EL and import it into the engine. + let block_import_start_time = Instant::now(); + let new_payload = self + .fetch_payload(&self.cfg, &self.engine, self.payload_id, self.attributes.clone()) + .await?; + + let new_block_ref = L2BlockInfo::from_payload_and_genesis( + new_payload.execution_payload.clone(), + self.attributes.inner().payload_attributes.parent_beacon_block_root, + &self.cfg.genesis, + ) + .map_err(SealTaskError::FromBlock)?; + + // Insert the new block into the engine. + match InsertTask::new( + Arc::clone(&self.engine), + self.cfg.clone(), + new_payload.clone(), + self.is_attributes_derived, + ) + .execute(state) + .await + { + Err(InsertTaskError::UnexpectedPayloadStatus(e)) + if self.attributes.is_deposits_only() => + { + error!(target: "engine_sealer", error = ?e, "Critical: Deposit-only payload import failed"); + return Err(SealTaskError::DepositOnlyPayloadFailed); + } + // HOLOCENE: Re-attempt payload import with deposits only + Err(InsertTaskError::UnexpectedPayloadStatus(e)) + if self + .cfg + .is_holocene_active(self.attributes.inner().payload_attributes.timestamp) => + { + warn!(target: "engine_sealer", error = ?e, "Re-attempting payload import with deposits only."); + + // HOLOCENE: Re-attempt payload import with deposits only + // First build the deposits-only payload, then seal it + let deposits_only_attrs = self.attributes.as_deposits_only(); + + return match build_and_seal( + state, + self.engine.clone(), + self.cfg.clone(), + deposits_only_attrs.clone(), + self.is_attributes_derived, + None, + ).await { + Ok(_) => { + info!(target: "engine_sealer", "Successfully imported deposits-only payload"); + Err(SealTaskError::HoloceneInvalidFlush) + }, + Err(_) => return Err(SealTaskError::DepositOnlyPayloadReattemptFailed), + } + } + Err(e) => { + error!(target: "engine_sealer", "Payload import failed: {e}"); + return Err(Box::new(e).into()); + } + Ok(_) => { + info!(target: "engine_sealer", "Successfully imported payload") + } + } + + let block_import_duration = block_import_start_time.elapsed(); + + // If a channel was provided, send the built payload envelope to it. + if let Some(tx) = &self.payload_tx { + tx.send(new_payload).await.map_err(Box::new).map_err(SealTaskError::MpscSend)?; + } + + Ok((new_block_ref, block_import_duration)) + } +} + +#[async_trait] +impl EngineTaskExt for SealTask { + type Output = (); + + type Error = SealTaskError; + + async fn execute(&self, state: &mut EngineState) -> Result<(), SealTaskError> { + debug!( + target: "engine_sealer", + txs = self.attributes.inner().transactions.as_ref().map_or(0, |txs| txs.len()), + is_deposits = self.attributes.is_deposits_only(), + "Starting new seal job" + ); + + // Seal the block and import it into the engine. + let (new_block_ref, block_import_duration) = self.seal_block(state).await?; + + info!( + target: "engine_sealer", + l2_number = new_block_ref.block_info.number, + l2_time = new_block_ref.block_info.timestamp, + block_import_duration = ?block_import_duration, + "Built and imported new {} block", + if self.is_attributes_derived { "safe" } else { "unsafe" }, + ); + + Ok(()) + } +} diff --git a/crates/node/engine/src/task_queue/tasks/synchronize/task.rs b/crates/node/engine/src/task_queue/tasks/synchronize/task.rs index 08818852e4..c0576765fb 100644 --- a/crates/node/engine/src/task_queue/tasks/synchronize/task.rs +++ b/crates/node/engine/src/task_queue/tasks/synchronize/task.rs @@ -27,7 +27,7 @@ use tokio::time::Instant; /// ## Automatic Integration /// /// Unlike the legacy `ForkchoiceTask`, forkchoice updates during block building are now -/// handled automatically within [`BuildTask`], eliminating the need for explicit +/// explicitly handled within [`BuildTask`], eliminating the need for explicit /// forkchoice management in most user scenarios. /// /// [`InsertTask`]: crate::InsertTask diff --git a/crates/node/engine/src/task_queue/tasks/task.rs b/crates/node/engine/src/task_queue/tasks/task.rs index a9120a5afb..c9c1437745 100644 --- a/crates/node/engine/src/task_queue/tasks/task.rs +++ b/crates/node/engine/src/task_queue/tasks/task.rs @@ -11,6 +11,7 @@ use derive_more::Display; use std::cmp::Ordering; use thiserror::Error; use tokio::task::yield_now; +use crate::task_queue::{SealTask, SealTaskError}; /// The severity of an engine task error. /// @@ -62,6 +63,9 @@ pub enum EngineTaskErrors { /// An error that occurred while building a block. #[error(transparent)] Build(#[from] BuildTaskError), + /// An error that occurred while sealing a block. + #[error(transparent)] + Seal(#[from] SealTaskError), /// An error that occurred while consolidating the engine state. #[error(transparent)] Consolidate(#[from] ConsolidateTaskError), @@ -75,6 +79,7 @@ impl EngineTaskError for EngineTaskErrors { match self { Self::Insert(inner) => inner.severity(), Self::Build(inner) => inner.severity(), + Self::Seal(inner) => inner.severity(), Self::Consolidate(inner) => inner.severity(), Self::Finalize(inner) => inner.severity(), } @@ -88,8 +93,10 @@ impl EngineTaskError for EngineTaskErrors { pub enum EngineTask { /// Inserts a payload into the execution engine. Insert(Box), - /// Builds a new block with the given attributes, and inserts it into the execution engine. + /// Begins building a new block with the given attributes. Build(Box), + /// Seals the block with the given payload ID and attributes, inserting it into the execution engine. + Seal(Box), /// Performs consolidation on the engine state, reverting to payload attribute processing /// via the [`BuildTask`] if consolidation fails. Consolidate(Box), @@ -102,9 +109,12 @@ impl EngineTask { async fn execute_inner(&self, state: &mut EngineState) -> Result<(), EngineTaskErrors> { match self.clone() { Self::Insert(task) => task.execute(state).await?, - Self::Build(task) => task.execute(state).await?, + Self::Seal(task) => task.execute(state).await?, Self::Consolidate(task) => task.execute(state).await?, Self::Finalize(task) => task.execute(state).await?, + Self::Build(task) => { + task.execute(state).await?; + } }; Ok(()) @@ -115,6 +125,7 @@ impl EngineTask { Self::Insert(_) => crate::Metrics::INSERT_TASK_LABEL, Self::Consolidate(_) => crate::Metrics::CONSOLIDATE_TASK_LABEL, Self::Build(_) => crate::Metrics::BUILD_TASK_LABEL, + Self::Seal(_) => crate::Metrics::SEAL_TASK_LABEL, Self::Finalize(_) => crate::Metrics::FINALIZE_TASK_LABEL, } } @@ -126,6 +137,7 @@ impl PartialEq for EngineTask { (self, other), (Self::Insert(_), Self::Insert(_)) | (Self::Build(_), Self::Build(_)) | + (Self::Seal(_), Self::Seal(_)) | (Self::Consolidate(_), Self::Consolidate(_)) | (Self::Finalize(_), Self::Finalize(_)) ) @@ -158,8 +170,13 @@ impl Ord for EngineTask { (Self::Insert(_), Self::Insert(_)) => Ordering::Equal, (Self::Consolidate(_), Self::Consolidate(_)) => Ordering::Equal, (Self::Build(_), Self::Build(_)) => Ordering::Equal, + (Self::Seal(_), Self::Seal(_)) => Ordering::Equal, (Self::Finalize(_), Self::Finalize(_)) => Ordering::Equal, + // SealBlock tasks are prioritized over all others + (Self::Seal(_), _) => Ordering::Greater, + (_, Self::Seal(_)) => Ordering::Less, + // BuildBlock tasks are prioritized over InsertUnsafe and Consolidate tasks (Self::Build(_), _) => Ordering::Greater, (_, Self::Build(_)) => Ordering::Less, diff --git a/crates/node/engine/src/task_queue/tasks/util.rs b/crates/node/engine/src/task_queue/tasks/util.rs new file mode 100644 index 0000000000..f3b407be29 --- /dev/null +++ b/crates/node/engine/src/task_queue/tasks/util.rs @@ -0,0 +1,88 @@ +//! Utility functions for task execution. + +use super::{BuildTask, BuildTaskError, EngineTaskExt, SealTask, SealTaskError}; +use crate::EngineState; +use kona_protocol::OpAttributesWithParent; +use std::sync::Arc; +use tokio::sync::mpsc; +use crate::EngineClient; +use kona_genesis::RollupConfig; +use op_alloy_rpc_types_engine::OpExecutionPayloadEnvelope; + +/// Error type for build and seal operations. +#[derive(Debug, thiserror::Error)] +pub(in crate::task_queue) enum BuildAndSealError { + /// An error occurred during the build phase. + #[error(transparent)] + Build(#[from] BuildTaskError), + /// An error occurred during the seal phase. + #[error(transparent)] + Seal(#[from] SealTaskError), +} + +/// Builds and seals a payload in sequence. +/// +/// This is a utility function that: +/// 1. Creates and executes a [`BuildTask`] to initiate block building +/// 2. Creates and executes a [`SealTask`] with the resulting [`PayloadId`] to seal the block +/// +/// This pattern is commonly used for Holocene deposits-only fallback and other scenarios +/// where a build-then-seal workflow is needed. +/// +/// # Arguments +/// +/// * `state` - Mutable reference to the engine state +/// * `engine` - The engine client +/// * `cfg` - The rollup configuration +/// * `attributes` - The payload attributes to build +/// * `is_attributes_derived` - Whether the attributes were derived or created by the sequencer +/// * `payload_tx` - Optional channel to send the built payload envelope +/// +/// # Returns +/// +/// Returns `Ok(())` if both build and seal operations succeed, or an error if either fails. +/// +/// # Examples +/// +/// ```ignore +/// let result = build_and_seal( +/// engine.clone(), +/// cfg.clone(), +/// deposits_only_attrs, +/// true, +/// Some(tx), +/// &mut state, +/// ).await?; +/// ``` +pub(in crate::task_queue) async fn build_and_seal( + state: &mut EngineState, + engine: Arc, + cfg: Arc, + attributes: OpAttributesWithParent, + is_attributes_derived: bool, + payload_tx: Option>, +) -> Result<(), BuildAndSealError> { + // Execute the build task + let payload_id = BuildTask::new( + engine.clone(), + cfg.clone(), + attributes.clone(), + None, // Build task doesn't send the payload yet + ) + .execute(state) + .await?; + + // Execute the seal task with the payload ID from the build + SealTask::new( + engine, + cfg, + payload_id, + attributes, + is_attributes_derived, + payload_tx, + ) + .execute(state) + .await?; + + Ok(()) +} \ No newline at end of file diff --git a/crates/node/service/src/actors/engine/actor.rs b/crates/node/service/src/actors/engine/actor.rs index ac860dc97a..32ae8494e8 100644 --- a/crates/node/service/src/actors/engine/actor.rs +++ b/crates/node/service/src/actors/engine/actor.rs @@ -1,15 +1,11 @@ //! The [`EngineActor`]. use super::{EngineError, L2Finalizer}; -use alloy_rpc_types_engine::JwtSecret; +use alloy_rpc_types_engine::{JwtSecret, PayloadId}; use async_trait::async_trait; use futures::future::OptionFuture; use kona_derive::{ResetSignal, Signal}; -use kona_engine::{ - BuildTask, ConsolidateTask, Engine, EngineClient, EngineQueries, - EngineState as InnerEngineState, EngineTask, EngineTaskError, EngineTaskErrorSeverity, - InsertTask, -}; +use kona_engine::{BuildTask, ConsolidateTask, Engine, EngineClient, EngineQueries, EngineState as InnerEngineState, EngineTask, EngineTaskError, EngineTaskErrorSeverity, InsertTask, SealTask}; use kona_genesis::RollupConfig; use kona_protocol::{BlockInfo, L2BlockInfo, OpAttributesWithParent}; use op_alloy_rpc_types_engine::OpExecutionPayloadEnvelope; @@ -39,12 +35,21 @@ pub struct EngineActor { /// Handler for inbound queries to the engine. inbound_queries: mpsc::Receiver, /// A channel to receive build requests from the sequencer actor. - /// + /// Upon successful processing of the provided attributes, a `PayloadId` will be sent via the + /// provided sender. /// ## Note /// This is `Some` when the node is in sequencer mode, and `None` when the node is in validator /// mode. build_request_rx: - Option)>>, + Option)>>, + /// A channel to receive seal requests from the sequencer actor. + /// Upon successful seal of the provided attributes, the resulting `OpExecutionPayloadEnvelope` + /// will be sent via provided sender. + /// ## Note + /// This is `Some` when the node is in sequencer mode, and `None` when the node is in validator + /// mode. + seal_request_rx: + Option)>>, /// The [`L2Finalizer`], used to finalize L2 blocks. finalizer: L2Finalizer, } @@ -52,13 +57,22 @@ pub struct EngineActor { /// The outbound data for the [`EngineActor`]. #[derive(Debug)] pub struct EngineInboundData { - /// The channel used by the sequencer actor to send build requests to the engine actor. - /// + /// A channel to send build requests from the sequencer actor. + /// Upon successful processing of the provided attributes, a `PayloadId` will be sent via the + /// provided sender. /// ## Note /// This is `Some` when the node is in sequencer mode, and `None` when the node is in validator /// mode. pub build_request_tx: - Option)>>, + Option)>>, + /// A channel to send seal requests from the sequencer actor. + /// Upon successful seal of the provided attributes, the resulting `OpExecutionPayloadEnvelope` + /// will be sent via provided sender. + /// ## Note + /// This is `Some` when the node is in sequencer mode, and `None` when the node is in validator + /// mode. + pub seal_request_tx: + Option)>>, /// A channel to send [`OpAttributesWithParent`] to the engine actor. pub attributes_tx: mpsc::Sender, /// A channel to send [`OpExecutionPayloadEnvelope`] to the engine actor. @@ -167,11 +181,12 @@ impl EngineActor { let (unsafe_block_tx, unsafe_block_rx) = mpsc::channel(1024); let (reset_request_tx, reset_request_rx) = mpsc::channel(1024); - let (build_request_tx, build_request_rx) = if config.mode.is_sequencer() { - let (tx, rx) = mpsc::channel(1024); - (Some(tx), Some(rx)) + let (build_request_tx, build_request_rx, seal_request_tx, seal_request_rx) = if config.mode.is_sequencer() { + let (build_tx, build_rx) = mpsc::channel(1024); + let (seal_tx, seal_rx) = mpsc::channel(1024); + (Some(build_tx), Some(build_rx), Some(seal_tx), Some(seal_rx)) } else { - (None, None) + (None, None, None, None) }; let actor = Self { @@ -181,11 +196,13 @@ impl EngineActor { reset_request_rx, inbound_queries: inbound_queries_rx, build_request_rx, + seal_request_rx, finalizer: L2Finalizer::new(finalized_l1_block_rx), }; let outbound_data = EngineInboundData { build_request_tx, + seal_request_tx, finalized_l1_block_tx, inbound_queries_tx, attributes_tx, @@ -442,6 +459,24 @@ impl NodeActor for EngineActor { .reset(&derivation_signal_tx, &engine_l2_safe_head_tx, &mut self.finalizer) .await?; } + Some(req) = OptionFuture::from(self.seal_request_rx.as_mut().map(|rx| rx.recv())), if self.seal_request_rx.is_some() => { + let Some((payload_id, attributes, response_tx)) = req else { + error!(target: "engine", "Seal request receiver closed unexpectedly while in sequencer mode"); + cancellation.cancel(); + return Err(EngineError::ChannelClosed); + }; + + let task = EngineTask::Seal(Box::new(SealTask::new( + state.client.clone(), + state.rollup.clone(), + payload_id, + attributes, + // The payload is not derived in this case. + false, + Some(response_tx), + ))); + state.engine.enqueue(task); + } Some(res) = OptionFuture::from(self.build_request_rx.as_mut().map(|rx| rx.recv())), if self.build_request_rx.is_some() => { let Some((attributes, response_tx)) = res else { error!(target: "engine", "Build request receiver closed unexpectedly while in sequencer mode"); @@ -453,8 +488,6 @@ impl NodeActor for EngineActor { state.client.clone(), state.rollup.clone(), attributes, - // The payload is not derived in this case. - false, Some(response_tx), ))); state.engine.enqueue(task); diff --git a/crates/node/service/src/actors/sequencer/actor.rs b/crates/node/service/src/actors/sequencer/actor.rs index 1fd1d162b4..5d7107c9b3 100644 --- a/crates/node/service/src/actors/sequencer/actor.rs +++ b/crates/node/service/src/actors/sequencer/actor.rs @@ -5,7 +5,9 @@ use super::{ }; use crate::{CancellableContext, NodeActor, actors::sequencer::conductor::ConductorClient}; use alloy_provider::RootProvider; +use alloy_rpc_types_engine::{PayloadId}; use async_trait::async_trait; +use derive_more::Constructor; use kona_derive::{AttributesBuilder, PipelineErrorKind, StatefulAttributesBuilder}; use kona_genesis::{L1ChainConfig, RollupConfig}; use kona_protocol::{BlockInfo, L2BlockInfo, OpAttributesWithParent}; @@ -14,8 +16,7 @@ use kona_rpc::SequencerAdminQuery; use op_alloy_network::Optimism; use op_alloy_rpc_types_engine::OpExecutionPayloadEnvelope; use std::{ - sync::Arc, - time::{Duration, Instant, SystemTime, UNIX_EPOCH}, + sync::Arc, time::{Duration, Instant} }; use tokio::{ select, @@ -36,6 +37,15 @@ pub struct SequencerActor { pub admin_query_rx: mpsc::Receiver, } +/// The handle to a block that has been started but not sealed. +#[derive(Debug, Constructor)] +pub(super) struct UnsealedPayloadData { + /// The [`PayloadId`] of the unsealed payload. + pub payload_id: PayloadId, + /// The [`OpAttributesWithParent`] used to start block building. + pub attributes_with_parent: OpAttributesWithParent +} + /// The state of the [`SequencerActor`]. #[derive(Debug)] pub(super) struct SequencerActorState { @@ -172,10 +182,14 @@ pub struct SequencerContext { pub l1_head_rx: watch::Receiver>, /// Sender to request the engine to reset. pub reset_request_tx: mpsc::Sender<()>, - /// Sender to request the execution layer to build a payload attributes on top of the + /// Sender to request the execution layer to start building a payload attributes on top of the /// current unsafe head. pub build_request_tx: - mpsc::Sender<(OpAttributesWithParent, mpsc::Sender)>, + mpsc::Sender<(OpAttributesWithParent, mpsc::Sender)>, + /// Sender to request the execution layer to seal the payload ID and attributes that + /// resulted from a previous build call. + pub seal_request_tx: + mpsc::Sender<(PayloadId, OpAttributesWithParent, mpsc::Sender)>, /// A sender to asynchronously sign and gossip built [`OpExecutionPayloadEnvelope`]s to the /// network actor. pub gossip_payload_tx: mpsc::Sender, @@ -213,13 +227,71 @@ impl SequencerActor { } impl SequencerActorState { - /// Starts the build job for the next L2 block, on top of the current unsafe head. - async fn build_block( + /// Seals and commits the last pending block, if one exists and starts the build job for the + /// next L2 block, on top of the current unsafe head. + /// + /// In the success case, this function stores the [`UnsealedPayloadData`] for the block it began + /// in state so that it may be sealed and committed the next time this function is called. + async fn seal_last_and_start_next( &mut self, ctx: &mut SequencerContext, unsafe_head_rx: &mut watch::Receiver, in_recovery_mode: bool, - ) -> Result<(), SequencerActorError> { + payload_to_seal: Option + ) -> Result, SequencerActorError> { + + if let Some(to_seal) = payload_to_seal { + self.seal_and_commit_payload_if_applicable(ctx, to_seal).await?; + } + + let unsealed_payload = self.build_unsealed_payload(ctx, unsafe_head_rx, in_recovery_mode).await?; + + Ok(unsealed_payload) + } + + /// Sends a seal request to seal the provided [`UnsealedPayloadData`], committing and gossiping + /// the resulting block, if one is built. + async fn seal_and_commit_payload_if_applicable(&mut self, ctx: &mut SequencerContext, unsealed_payload_data: UnsealedPayloadData) -> Result<(), SequencerActorError> { + let UnsealedPayloadData {payload_id, attributes_with_parent} = unsealed_payload_data; + + // Create a new channel to receive the built payload. + let (payload_tx, payload_rx) = mpsc::channel(1); + + // Send the seal request to the engine to seal the unsealed block. + let _seal_request_start = Instant::now(); + if let Err(err) = ctx.seal_request_tx.send((payload_id, attributes_with_parent, payload_tx)).await { + error!(target: "sequencer", ?err, "Failed to send seal request to engine, payload id {},", payload_id); + ctx.cancellation.cancel(); + return Err(SequencerActorError::ChannelClosed); + } + + let payload = self.try_wait_for_payload(ctx, payload_rx).await?; + + // Log the block building seal task duration, if metrics are enabled. + kona_macros::set!( + gauge, + crate::Metrics::SEQUENCER_BLOCK_BUILDING_SEAL_TASK_DURATION, + _seal_request_start.elapsed() + ); + + // If the conductor is available, commit the payload to it. + if let Some(conductor) = &self.conductor { + let _conductor_commitment_start = Instant::now(); + if let Err(err) = conductor.commit_unsafe_payload(&payload).await { + error!(target: "sequencer", ?err, "Failed to commit unsafe payload to conductor"); + } + + kona_macros::set!( + gauge, + crate::Metrics::SEQUENCER_CONDUCTOR_COMMITMENT_DURATION, + _conductor_commitment_start.elapsed() + ); + } + + self.schedule_gossip(ctx, payload).await + } + + async fn build_unsealed_payload(&mut self, ctx: &mut SequencerContext, unsafe_head_rx: &mut watch::Receiver, in_recovery_mode: bool) -> Result, SequencerActorError> { let unsafe_head = *unsafe_head_rx.borrow(); let l1_origin = match self .origin_selector @@ -233,7 +305,7 @@ impl SequencerActorState { ?err, "Temporary error occurred while selecting next L1 origin. Re-attempting on next tick." ); - return Ok(()); + return Ok(None); } }; @@ -252,7 +324,7 @@ impl SequencerActorState { ctx.cancellation.cancel(); return Err(SequencerActorError::ChannelClosed); } - return Ok(()); + return Ok(None); } info!( @@ -264,12 +336,53 @@ impl SequencerActorState { // Build the payload attributes for the next block. let _attributes_build_start = Instant::now(); + + let attrs_with_parent = match self.build_attributes(ctx, in_recovery_mode, unsafe_head, l1_origin).await? { + Some(attrs) => attrs, + None => { + // Temporary error or reset - retry on next tick. + return Ok(None); + } + }; + + // Log the attributes build duration, if metrics are enabled. + kona_macros::set!( + gauge, + crate::Metrics::SEQUENCER_ATTRIBUTES_BUILDER_DURATION, + _attributes_build_start.elapsed() + ); + + // Create a new channel to receive the built payload. + let (payload_id_tx, payload_id_rx) = mpsc::channel(1); + + // Send the built attributes to the engine to be built. + let _build_request_start = Instant::now(); + if let Err(err) = ctx.build_request_tx.send((attrs_with_parent.clone(), payload_id_tx)).await { + error!(target: "sequencer", ?err, "Failed to send built attributes to engine"); + ctx.cancellation.cancel(); + return Err(SequencerActorError::ChannelClosed); + } + + let payload_id = self.try_wait_for_payload_id(ctx, payload_id_rx).await?; + + Ok(Some(UnsealedPayloadData::new(payload_id, attrs_with_parent))) + } + + /// Builds the OpAttributesWithParent for the next block to build. If None is returned, it + /// indicates that no attributes could be built at this time but future attempts may be made. + async fn build_attributes( + &mut self, + ctx: &mut SequencerContext, + in_recovery_mode: bool, + unsafe_head: L2BlockInfo, + l1_origin: BlockInfo, + ) -> Result, SequencerActorError> { let mut attributes = match self.builder.prepare_payload_attributes(unsafe_head, l1_origin.id()).await { Ok(attrs) => attrs, Err(PipelineErrorKind::Temporary(_)) => { - return Ok(()); - // Do nothing and allow a retry. + // Temporary error - retry on next tick. + return Ok(None); } Err(PipelineErrorKind::Reset(_)) => { if let Err(err) = ctx.reset_request_tx.send(()).await { @@ -282,7 +395,7 @@ impl SequencerActorState { target: "sequencer", "Resetting engine due to pipeline error while preparing payload attributes" ); - return Ok(()); + return Ok(None); } Err(err @ PipelineErrorKind::Critical(_)) => { error!(target: "sequencer", ?err, "Failed to prepare payload attributes"); @@ -351,60 +464,7 @@ impl SequencerActorState { } let attrs_with_parent = OpAttributesWithParent::new(attributes, unsafe_head, None, false); - - // Log the attributes build duration, if metrics are enabled. - kona_macros::set!( - gauge, - crate::Metrics::SEQUENCER_ATTRIBUTES_BUILDER_DURATION, - _attributes_build_start.elapsed() - ); - - // Create a new channel to receive the built payload. - let (payload_tx, payload_rx) = mpsc::channel(1); - - // Send the built attributes to the engine to be built. - let _build_request_start = Instant::now(); - if let Err(err) = ctx.build_request_tx.send((attrs_with_parent, payload_tx)).await { - error!(target: "sequencer", ?err, "Failed to send built attributes to engine"); - ctx.cancellation.cancel(); - return Err(SequencerActorError::ChannelClosed); - } - - let payload = self.try_wait_for_payload(ctx, payload_rx).await?; - - // Log the block building job duration, if metrics are enabled. - kona_macros::set!( - gauge, - crate::Metrics::SEQUENCER_BLOCK_BUILDING_JOB_DURATION, - _build_request_start.elapsed() - ); - - // If the conductor is available, commit the payload to it. - if let Some(conductor) = &self.conductor { - let _conductor_commitment_start = Instant::now(); - if let Err(err) = conductor.commit_unsafe_payload(&payload).await { - error!(target: "sequencer", ?err, "Failed to commit unsafe payload to conductor"); - } - - kona_macros::set!( - gauge, - crate::Metrics::SEQUENCER_CONDUCTOR_COMMITMENT_DURATION, - _conductor_commitment_start.elapsed() - ); - } - - let now = - SystemTime::now().duration_since(UNIX_EPOCH).expect("Time went backwards").as_secs(); - let then = payload.execution_payload.timestamp() + self.cfg.block_time; - if then.saturating_sub(now) <= self.cfg.block_time { - warn!( - target: "sequencer", - "Next block timestamp is more than a block time away from now, building immediately" - ); - self.build_ticker.reset_immediately(); - } - - self.schedule_gossip(ctx, payload).await + Ok(Some(attrs_with_parent)) } /// Waits for the next payload to be built and returns it, if there is a payload receiver @@ -421,6 +481,20 @@ impl SequencerActorState { }) } + /// Waits for the next payload to be built and returns it, if there is a payload receiver + /// present. + async fn try_wait_for_payload_id( + &mut self, + ctx: &mut SequencerContext, + mut payload_id_rx: mpsc::Receiver, + ) -> Result { + payload_id_rx.recv().await.ok_or_else(|| { + error!(target: "sequencer", "Failed to receive payload for initiated block build"); + ctx.cancellation.cancel(); + SequencerActorError::ChannelClosed + }) + } + /// Schedules a built [`OpExecutionPayloadEnvelope`] to be signed and gossipped. async fn schedule_gossip( &mut self, @@ -496,6 +570,8 @@ impl NodeActor for SequencerActor { // Reset the engine state prior to beginning block building. state.schedule_initial_reset(&mut ctx, &mut self.unsafe_head_rx).await?; + let mut next_block_to_seal = None; + loop { select! { // We are using a biased select here to ensure that the admin queries are given priority over the block building task. @@ -527,7 +603,13 @@ impl NodeActor for SequencerActor { } // The sequencer must be active to build new blocks. _ = state.build_ticker.tick(), if state.is_active => { - state.build_block(&mut ctx, &mut self.unsafe_head_rx, state.is_recovery_mode).await?; + + let start_time = Instant::now(); + + next_block_to_seal = state.seal_last_and_start_next(&mut ctx, &mut self.unsafe_head_rx, state.is_recovery_mode, next_block_to_seal).await?; + + // Set the next tick time to the configured blocktime less the time it takes to seal last and start next. + state.build_ticker.reset_after(Duration::from_secs(state.cfg.block_time).saturating_sub(start_time.elapsed())); } } } diff --git a/crates/node/service/src/metrics/mod.rs b/crates/node/service/src/metrics/mod.rs index 973db32eac..02b84b3b07 100644 --- a/crates/node/service/src/metrics/mod.rs +++ b/crates/node/service/src/metrics/mod.rs @@ -21,9 +21,13 @@ impl Metrics { pub const SEQUENCER_ATTRIBUTES_BUILDER_DURATION: &str = "kona_node_sequencer_attributes_build_duration"; - /// Gauge for the sequencer's block building job duration. - pub const SEQUENCER_BLOCK_BUILDING_JOB_DURATION: &str = - "kona_node_sequencer_block_building_duration"; + /// Gauge for the sequencer's block building start task duration. + pub const SEQUENCER_BLOCK_BUILDING_START_TASK_DURATION: &str = + "kona_node_sequencer_block_building_start_task_duration"; + + /// Gauge for the sequencer's block building seal task duration. + pub const SEQUENCER_BLOCK_BUILDING_SEAL_TASK_DURATION: &str = + "kona_node_sequencer_block_building_seal_task_duration"; /// Gauge for the sequencer's conductor commitment duration. pub const SEQUENCER_CONDUCTOR_COMMITMENT_DURATION: &str = @@ -66,8 +70,14 @@ impl Metrics { // Sequencer block building job duration metrics::describe_gauge!( - Self::SEQUENCER_BLOCK_BUILDING_JOB_DURATION, - "Duration of the sequencer block building job" + Self::SEQUENCER_BLOCK_BUILDING_START_TASK_DURATION, + "Duration of the sequencer block building start task" + ); + + // Sequencer block building job duration + metrics::describe_gauge!( + Self::SEQUENCER_BLOCK_BUILDING_SEAL_TASK_DURATION, + "Duration of the sequencer block building seal task" ); // Sequencer conductor commitment duration diff --git a/crates/node/service/src/service/core.rs b/crates/node/service/src/service/core.rs index 9f28fd6b76..eb6d51f1c5 100644 --- a/crates/node/service/src/service/core.rs +++ b/crates/node/service/src/service/core.rs @@ -125,6 +125,7 @@ pub trait RollupNodeService { let ( EngineInboundData { build_request_tx, + seal_request_tx, attributes_tx, unsafe_block_tx, reset_request_tx, @@ -176,6 +177,9 @@ pub trait RollupNodeService { build_request_tx: build_request_tx.expect( "`build_request_tx` not set while in sequencer mode. This should never happen.", ), + seal_request_tx: seal_request_tx.expect( + "`build_request_tx` not set while in sequencer mode. This should never happen.", + ), gossip_payload_tx, cancellation: cancellation.clone(), }) diff --git a/docker/recipes/kona-node/grafana/dashboards/overview.json b/docker/recipes/kona-node/grafana/dashboards/overview.json index a4eb0a89c9..13e480f04a 100644 --- a/docker/recipes/kona-node/grafana/dashboards/overview.json +++ b/docker/recipes/kona-node/grafana/dashboards/overview.json @@ -6073,16 +6073,16 @@ }, "disableTextWrap": false, "editorMode": "builder", - "expr": "kona_node_sequencer_block_building_duration{instance=~\"$instance\"}", + "expr": "kona_node_sequencer_block_building_seal_task_duration{instance=~\"$instance\"}", "fullMetaSearch": false, "includeNullMetadata": true, - "legendFormat": "Block Building Duration", + "legendFormat": "Block Building Seal Task Duration", "range": true, "refId": "A", "useBackend": false } ], - "title": "Block Building Duration", + "title": "Block Building Seal Task Duration", "transparent": true, "type": "timeseries" }, From ba52c32659dfc0057c50598077334ea465823afa Mon Sep 17 00:00:00 2001 From: op-will <232669456+op-will@users.noreply.github.com> Date: Wed, 5 Nov 2025 15:16:34 -0600 Subject: [PATCH 02/19] Fixing linting --- .../engine/src/task_queue/tasks/build/task.rs | 22 ++++---- .../src/task_queue/tasks/consolidate/error.rs | 2 +- .../src/task_queue/tasks/consolidate/task.rs | 18 +++++-- .../node/engine/src/task_queue/tasks/mod.rs | 4 +- .../engine/src/task_queue/tasks/seal/error.rs | 5 +- .../engine/src/task_queue/tasks/seal/mod.rs | 2 +- .../engine/src/task_queue/tasks/seal/task.rs | 35 ++++-------- .../node/engine/src/task_queue/tasks/task.rs | 7 +-- .../node/engine/src/task_queue/tasks/util.rs | 24 +++------ .../node/service/src/actors/engine/actor.rs | 41 ++++++++------ .../service/src/actors/sequencer/actor.rs | 54 ++++++++++++------- 11 files changed, 110 insertions(+), 104 deletions(-) diff --git a/crates/node/engine/src/task_queue/tasks/build/task.rs b/crates/node/engine/src/task_queue/tasks/build/task.rs index 45e87fec62..8da56cdbd4 100644 --- a/crates/node/engine/src/task_queue/tasks/build/task.rs +++ b/crates/node/engine/src/task_queue/tasks/build/task.rs @@ -1,17 +1,17 @@ //! A task for building a new block and importing it. use super::BuildTaskError; -use crate::{EngineClient, EngineForkchoiceVersion, EngineState, EngineTaskExt, state::EngineSyncStateUpdate, task_queue::tasks::build::error::EngineBuildError}; +use crate::{ + EngineClient, EngineForkchoiceVersion, EngineState, EngineTaskExt, + state::EngineSyncStateUpdate, task_queue::tasks::build::error::EngineBuildError, +}; use alloy_rpc_types_engine::{PayloadId, PayloadStatusEnum}; use async_trait::async_trait; -use derive_more::{Constructor}; +use derive_more::Constructor; use kona_genesis::RollupConfig; -use kona_protocol::{OpAttributesWithParent}; +use kona_protocol::OpAttributesWithParent; use op_alloy_provider::ext::engine::OpEngineApi; -use std::{ - sync::Arc, - time::{Instant}, -}; -use tokio::{sync::mpsc}; +use std::{sync::Arc, time::Instant}; +use tokio::sync::mpsc; /// Task for building new blocks with automatic forkchoice synchronization. /// @@ -21,7 +21,8 @@ use tokio::{sync::mpsc}; /// /// ## Error Handling /// -/// The task uses [`EngineBuildError`] for build-specific failures during the forkchoice update phase. +/// The task uses [`EngineBuildError`] for build-specific failures during the forkchoice update +/// phase. /// /// [`EngineBuildError`]: crate::EngineBuildError #[derive(Debug, Clone, Constructor)] @@ -32,7 +33,8 @@ pub struct BuildTask { pub cfg: Arc, /// The [`OpAttributesWithParent`] to instruct the execution layer to build. pub attributes: OpAttributesWithParent, - /// The optional sender to which [`PayloadId`] will be sent after the block build has been started. + /// The optional sender to which [`PayloadId`] will be sent after the block build has been + /// started. pub payload_id_tx: Option>, } diff --git a/crates/node/engine/src/task_queue/tasks/consolidate/error.rs b/crates/node/engine/src/task_queue/tasks/consolidate/error.rs index 935ffbfa8b..b4210ce22a 100644 --- a/crates/node/engine/src/task_queue/tasks/consolidate/error.rs +++ b/crates/node/engine/src/task_queue/tasks/consolidate/error.rs @@ -2,7 +2,7 @@ use crate::{ BuildTaskError, EngineTaskError, SealTaskError, SynchronizeTaskError, - task_queue::tasks::{task::EngineTaskErrorSeverity, BuildAndSealError}, + task_queue::tasks::{BuildAndSealError, task::EngineTaskErrorSeverity}, }; use thiserror::Error; diff --git a/crates/node/engine/src/task_queue/tasks/consolidate/task.rs b/crates/node/engine/src/task_queue/tasks/consolidate/task.rs index 44d853afd9..246d3e9518 100644 --- a/crates/node/engine/src/task_queue/tasks/consolidate/task.rs +++ b/crates/node/engine/src/task_queue/tasks/consolidate/task.rs @@ -1,7 +1,8 @@ //! A task to consolidate the engine state. use crate::{ - state::EngineSyncStateUpdate, task_queue::build_and_seal, ConsolidateTaskError, EngineClient, EngineState, EngineTaskExt, SynchronizeTask + ConsolidateTaskError, EngineClient, EngineState, EngineTaskExt, SynchronizeTask, + state::EngineSyncStateUpdate, task_queue::build_and_seal, }; use async_trait::async_trait; use kona_genesis::RollupConfig; @@ -11,8 +12,7 @@ use std::{sync::Arc, time::Instant}; /// The [`ConsolidateTask`] attempts to consolidate the engine state /// using the specified payload attributes and the oldest unsafe head. /// -/// If consolidation fails, payload attributes processing is attempted using the [`BuildTask`] -/// followed by the [`SealTask`]. +/// If consolidation fails, payload attributes processing is attempted using `build_and_seal`. #[derive(Debug, Clone)] pub struct ConsolidateTask { /// The engine client. @@ -36,13 +36,21 @@ impl ConsolidateTask { Self { client, cfg: config, attributes, is_attributes_derived } } - /// Executes a new [`BuildTask`] followed by a [`SealTask`]. /// This is used when the [`ConsolidateTask`] fails to consolidate the engine state. async fn execute_build_and_seal_tasks( &self, state: &mut EngineState, ) -> Result<(), ConsolidateTaskError> { - build_and_seal(state, self.client.clone(), self.cfg.clone(), self.attributes.clone(), self.is_attributes_derived, None).await.map_err(Into::into) + build_and_seal( + state, + self.client.clone(), + self.cfg.clone(), + self.attributes.clone(), + self.is_attributes_derived, + None, + ) + .await + .map_err(Into::into) } /// Attempts consolidation on the engine state. diff --git a/crates/node/engine/src/task_queue/tasks/mod.rs b/crates/node/engine/src/task_queue/tasks/mod.rs index 99b7d47620..562c1d5637 100644 --- a/crates/node/engine/src/task_queue/tasks/mod.rs +++ b/crates/node/engine/src/task_queue/tasks/mod.rs @@ -15,7 +15,7 @@ mod build; pub use build::{BuildTask, BuildTaskError, EngineBuildError}; mod seal; -pub use seal::{SealTask, SealTaskError, EngineSealError}; +pub use seal::{EngineSealError, SealTask, SealTaskError}; mod consolidate; pub use consolidate::{ConsolidateTask, ConsolidateTaskError}; @@ -24,4 +24,4 @@ mod finalize; pub use finalize::{FinalizeTask, FinalizeTaskError}; mod util; -pub(super) use util::{build_and_seal, BuildAndSealError}; +pub(super) use util::{BuildAndSealError, build_and_seal}; diff --git a/crates/node/engine/src/task_queue/tasks/seal/error.rs b/crates/node/engine/src/task_queue/tasks/seal/error.rs index 2d1c41427e..553f969d91 100644 --- a/crates/node/engine/src/task_queue/tasks/seal/error.rs +++ b/crates/node/engine/src/task_queue/tasks/seal/error.rs @@ -1,9 +1,6 @@ //! Contains error types for the [crate::SynchronizeTask]. -use crate::{ - EngineTaskError, InsertTaskError, - task_queue::tasks::task::EngineTaskErrorSeverity, -}; +use crate::{EngineTaskError, InsertTaskError, task_queue::tasks::task::EngineTaskErrorSeverity}; use alloy_rpc_types_engine::PayloadStatusEnum; use alloy_transport::{RpcError, TransportErrorKind}; use kona_protocol::FromBlockError; diff --git a/crates/node/engine/src/task_queue/tasks/seal/mod.rs b/crates/node/engine/src/task_queue/tasks/seal/mod.rs index e7927e7419..d91dee604d 100644 --- a/crates/node/engine/src/task_queue/tasks/seal/mod.rs +++ b/crates/node/engine/src/task_queue/tasks/seal/mod.rs @@ -4,4 +4,4 @@ mod task; pub use task::SealTask; mod error; -pub use error::{SealTaskError, EngineSealError}; +pub use error::{EngineSealError, SealTaskError}; diff --git a/crates/node/engine/src/task_queue/tasks/seal/task.rs b/crates/node/engine/src/task_queue/tasks/seal/task.rs index c701291ba6..553457d62e 100644 --- a/crates/node/engine/src/task_queue/tasks/seal/task.rs +++ b/crates/node/engine/src/task_queue/tasks/seal/task.rs @@ -1,7 +1,9 @@ //! A task for importing a block that has already been started. use super::SealTaskError; use crate::{ - task_queue::build_and_seal, EngineClient, EngineGetPayloadVersion, EngineState, EngineTaskExt, InsertTask, InsertTaskError::{self} + EngineClient, EngineGetPayloadVersion, EngineState, EngineTaskExt, InsertTask, + InsertTaskError::{self}, + task_queue::build_and_seal, }; use alloy_rpc_types_engine::{ExecutionPayload, PayloadId}; use async_trait::async_trait; @@ -11,9 +13,9 @@ use op_alloy_provider::ext::engine::OpEngineApi; use op_alloy_rpc_types_engine::{OpExecutionPayload, OpExecutionPayloadEnvelope}; use std::{ sync::Arc, - time::{Duration, Instant, SystemTime}, + time::{Duration, Instant}, }; -use tokio::{sync::mpsc, time::sleep}; +use tokio::sync::mpsc; /// Task for sealing blocks. /// @@ -127,7 +129,6 @@ impl SealTask { Ok(payload_envelope) } - /// Seals the block by waiting for the appropriate time, fetching the payload, and importing it. /// /// This function handles: @@ -141,26 +142,6 @@ impl SealTask { &self, state: &mut EngineState, ) -> Result<(L2BlockInfo, Duration), SealTaskError> { - // Compute the time of the next block. - let next_block = Duration::from_secs( - self.attributes.parent().block_info.timestamp.saturating_add(self.cfg.block_time), - ); - - // Compute the time left to seal the next block. - let now = SystemTime::now() - .duration_since(SystemTime::UNIX_EPOCH) - .map_err(|_| SealTaskError::ClockWentBackwards)?; - - // Add a buffer to the time left to seal the next block. - const SEALING_BUFFER: Duration = Duration::from_millis(50); - - let time_left_to_seal = next_block.saturating_sub(now).saturating_sub(SEALING_BUFFER); - - // Wait for the time left to seal the next block. - if !time_left_to_seal.is_zero() { - sleep(time_left_to_seal).await; - } - // Fetch the payload just inserted from the EL and import it into the engine. let block_import_start_time = Instant::now(); let new_payload = self @@ -209,11 +190,13 @@ impl SealTask { deposits_only_attrs.clone(), self.is_attributes_derived, None, - ).await { + ) + .await + { Ok(_) => { info!(target: "engine_sealer", "Successfully imported deposits-only payload"); Err(SealTaskError::HoloceneInvalidFlush) - }, + } Err(_) => return Err(SealTaskError::DepositOnlyPayloadReattemptFailed), } } diff --git a/crates/node/engine/src/task_queue/tasks/task.rs b/crates/node/engine/src/task_queue/tasks/task.rs index c9c1437745..de4b0e24f4 100644 --- a/crates/node/engine/src/task_queue/tasks/task.rs +++ b/crates/node/engine/src/task_queue/tasks/task.rs @@ -5,13 +5,13 @@ use super::{BuildTask, ConsolidateTask, FinalizeTask, InsertTask}; use crate::{ BuildTaskError, ConsolidateTaskError, EngineState, FinalizeTaskError, InsertTaskError, + task_queue::{SealTask, SealTaskError}, }; use async_trait::async_trait; use derive_more::Display; use std::cmp::Ordering; use thiserror::Error; use tokio::task::yield_now; -use crate::task_queue::{SealTask, SealTaskError}; /// The severity of an engine task error. /// @@ -95,7 +95,8 @@ pub enum EngineTask { Insert(Box), /// Begins building a new block with the given attributes. Build(Box), - /// Seals the block with the given payload ID and attributes, inserting it into the execution engine. + /// Seals the block with the given payload ID and attributes, inserting it into the execution + /// engine. Seal(Box), /// Performs consolidation on the engine state, reverting to payload attribute processing /// via the [`BuildTask`] if consolidation fails. @@ -176,7 +177,7 @@ impl Ord for EngineTask { // SealBlock tasks are prioritized over all others (Self::Seal(_), _) => Ordering::Greater, (_, Self::Seal(_)) => Ordering::Less, - + // BuildBlock tasks are prioritized over InsertUnsafe and Consolidate tasks (Self::Build(_), _) => Ordering::Greater, (_, Self::Build(_)) => Ordering::Less, diff --git a/crates/node/engine/src/task_queue/tasks/util.rs b/crates/node/engine/src/task_queue/tasks/util.rs index f3b407be29..c7dd523850 100644 --- a/crates/node/engine/src/task_queue/tasks/util.rs +++ b/crates/node/engine/src/task_queue/tasks/util.rs @@ -1,13 +1,12 @@ //! Utility functions for task execution. use super::{BuildTask, BuildTaskError, EngineTaskExt, SealTask, SealTaskError}; -use crate::EngineState; +use crate::{EngineClient, EngineState}; +use kona_genesis::RollupConfig; use kona_protocol::OpAttributesWithParent; +use op_alloy_rpc_types_engine::OpExecutionPayloadEnvelope; use std::sync::Arc; use tokio::sync::mpsc; -use crate::EngineClient; -use kona_genesis::RollupConfig; -use op_alloy_rpc_types_engine::OpExecutionPayloadEnvelope; /// Error type for build and seal operations. #[derive(Debug, thiserror::Error)] @@ -24,7 +23,7 @@ pub(in crate::task_queue) enum BuildAndSealError { /// /// This is a utility function that: /// 1. Creates and executes a [`BuildTask`] to initiate block building -/// 2. Creates and executes a [`SealTask`] with the resulting [`PayloadId`] to seal the block +/// 2. Creates and executes a [`SealTask`] to seal the block, referencing the initiated payload /// /// This pattern is commonly used for Holocene deposits-only fallback and other scenarios /// where a build-then-seal workflow is needed. @@ -73,16 +72,9 @@ pub(in crate::task_queue) async fn build_and_seal( .await?; // Execute the seal task with the payload ID from the build - SealTask::new( - engine, - cfg, - payload_id, - attributes, - is_attributes_derived, - payload_tx, - ) - .execute(state) - .await?; + SealTask::new(engine, cfg, payload_id, attributes, is_attributes_derived, payload_tx) + .execute(state) + .await?; Ok(()) -} \ No newline at end of file +} diff --git a/crates/node/service/src/actors/engine/actor.rs b/crates/node/service/src/actors/engine/actor.rs index 32ae8494e8..0e264aa90d 100644 --- a/crates/node/service/src/actors/engine/actor.rs +++ b/crates/node/service/src/actors/engine/actor.rs @@ -5,7 +5,11 @@ use alloy_rpc_types_engine::{JwtSecret, PayloadId}; use async_trait::async_trait; use futures::future::OptionFuture; use kona_derive::{ResetSignal, Signal}; -use kona_engine::{BuildTask, ConsolidateTask, Engine, EngineClient, EngineQueries, EngineState as InnerEngineState, EngineTask, EngineTaskError, EngineTaskErrorSeverity, InsertTask, SealTask}; +use kona_engine::{ + BuildTask, ConsolidateTask, Engine, EngineClient, EngineQueries, + EngineState as InnerEngineState, EngineTask, EngineTaskError, EngineTaskErrorSeverity, + InsertTask, SealTask, +}; use kona_genesis::RollupConfig; use kona_protocol::{BlockInfo, L2BlockInfo, OpAttributesWithParent}; use op_alloy_rpc_types_engine::OpExecutionPayloadEnvelope; @@ -40,16 +44,20 @@ pub struct EngineActor { /// ## Note /// This is `Some` when the node is in sequencer mode, and `None` when the node is in validator /// mode. - build_request_rx: - Option)>>, + build_request_rx: Option)>>, /// A channel to receive seal requests from the sequencer actor. /// Upon successful seal of the provided attributes, the resulting `OpExecutionPayloadEnvelope` /// will be sent via provided sender. /// ## Note /// This is `Some` when the node is in sequencer mode, and `None` when the node is in validator /// mode. - seal_request_rx: - Option)>>, + seal_request_rx: Option< + mpsc::Receiver<( + PayloadId, + OpAttributesWithParent, + mpsc::Sender, + )>, + >, /// The [`L2Finalizer`], used to finalize L2 blocks. finalizer: L2Finalizer, } @@ -63,16 +71,16 @@ pub struct EngineInboundData { /// ## Note /// This is `Some` when the node is in sequencer mode, and `None` when the node is in validator /// mode. - pub build_request_tx: - Option)>>, + pub build_request_tx: Option)>>, /// A channel to send seal requests from the sequencer actor. /// Upon successful seal of the provided attributes, the resulting `OpExecutionPayloadEnvelope` /// will be sent via provided sender. /// ## Note /// This is `Some` when the node is in sequencer mode, and `None` when the node is in validator /// mode. - pub seal_request_tx: - Option)>>, + pub seal_request_tx: Option< + mpsc::Sender<(PayloadId, OpAttributesWithParent, mpsc::Sender)>, + >, /// A channel to send [`OpAttributesWithParent`] to the engine actor. pub attributes_tx: mpsc::Sender, /// A channel to send [`OpExecutionPayloadEnvelope`] to the engine actor. @@ -181,13 +189,14 @@ impl EngineActor { let (unsafe_block_tx, unsafe_block_rx) = mpsc::channel(1024); let (reset_request_tx, reset_request_rx) = mpsc::channel(1024); - let (build_request_tx, build_request_rx, seal_request_tx, seal_request_rx) = if config.mode.is_sequencer() { - let (build_tx, build_rx) = mpsc::channel(1024); - let (seal_tx, seal_rx) = mpsc::channel(1024); - (Some(build_tx), Some(build_rx), Some(seal_tx), Some(seal_rx)) - } else { - (None, None, None, None) - }; + let (build_request_tx, build_request_rx, seal_request_tx, seal_request_rx) = + if config.mode.is_sequencer() { + let (build_tx, build_rx) = mpsc::channel(1024); + let (seal_tx, seal_rx) = mpsc::channel(1024); + (Some(build_tx), Some(build_rx), Some(seal_tx), Some(seal_rx)) + } else { + (None, None, None, None) + }; let actor = Self { builder: config, diff --git a/crates/node/service/src/actors/sequencer/actor.rs b/crates/node/service/src/actors/sequencer/actor.rs index 5d7107c9b3..207bb49587 100644 --- a/crates/node/service/src/actors/sequencer/actor.rs +++ b/crates/node/service/src/actors/sequencer/actor.rs @@ -5,7 +5,7 @@ use super::{ }; use crate::{CancellableContext, NodeActor, actors::sequencer::conductor::ConductorClient}; use alloy_provider::RootProvider; -use alloy_rpc_types_engine::{PayloadId}; +use alloy_rpc_types_engine::PayloadId; use async_trait::async_trait; use derive_more::Constructor; use kona_derive::{AttributesBuilder, PipelineErrorKind, StatefulAttributesBuilder}; @@ -16,7 +16,8 @@ use kona_rpc::SequencerAdminQuery; use op_alloy_network::Optimism; use op_alloy_rpc_types_engine::OpExecutionPayloadEnvelope; use std::{ - sync::Arc, time::{Duration, Instant} + sync::Arc, + time::{Duration, Instant}, }; use tokio::{ select, @@ -43,7 +44,7 @@ pub(super) struct UnsealedPayloadData { /// The [`PayloadId`] of the unsealed payload. pub payload_id: PayloadId, /// The [`OpAttributesWithParent`] used to start block building. - pub attributes_with_parent: OpAttributesWithParent + pub attributes_with_parent: OpAttributesWithParent, } /// The state of the [`SequencerActor`]. @@ -184,8 +185,7 @@ pub struct SequencerContext { pub reset_request_tx: mpsc::Sender<()>, /// Sender to request the execution layer to start building a payload attributes on top of the /// current unsafe head. - pub build_request_tx: - mpsc::Sender<(OpAttributesWithParent, mpsc::Sender)>, + pub build_request_tx: mpsc::Sender<(OpAttributesWithParent, mpsc::Sender)>, /// Sender to request the execution layer to seal the payload ID and attributes that /// resulted from a previous build call. pub seal_request_tx: @@ -237,29 +237,35 @@ impl SequencerActorState { ctx: &mut SequencerContext, unsafe_head_rx: &mut watch::Receiver, in_recovery_mode: bool, - payload_to_seal: Option + payload_to_seal: Option, ) -> Result, SequencerActorError> { - if let Some(to_seal) = payload_to_seal { self.seal_and_commit_payload_if_applicable(ctx, to_seal).await?; } - let unsealed_payload = self.build_unsealed_payload(ctx, unsafe_head_rx, in_recovery_mode).await?; + let unsealed_payload = + self.build_unsealed_payload(ctx, unsafe_head_rx, in_recovery_mode).await?; Ok(unsealed_payload) } /// Sends a seal request to seal the provided [`UnsealedPayloadData`], committing and gossiping /// the resulting block, if one is built. - async fn seal_and_commit_payload_if_applicable(&mut self, ctx: &mut SequencerContext, unsealed_payload_data: UnsealedPayloadData) -> Result<(), SequencerActorError> { - let UnsealedPayloadData {payload_id, attributes_with_parent} = unsealed_payload_data; + async fn seal_and_commit_payload_if_applicable( + &mut self, + ctx: &mut SequencerContext, + unsealed_payload_data: UnsealedPayloadData, + ) -> Result<(), SequencerActorError> { + let UnsealedPayloadData { payload_id, attributes_with_parent } = unsealed_payload_data; // Create a new channel to receive the built payload. let (payload_tx, payload_rx) = mpsc::channel(1); // Send the seal request to the engine to seal the unsealed block. let _seal_request_start = Instant::now(); - if let Err(err) = ctx.seal_request_tx.send((payload_id, attributes_with_parent, payload_tx)).await { + if let Err(err) = + ctx.seal_request_tx.send((payload_id, attributes_with_parent, payload_tx)).await + { error!(target: "sequencer", ?err, "Failed to send seal request to engine, payload id {},", payload_id); ctx.cancellation.cancel(); return Err(SequencerActorError::ChannelClosed); @@ -291,7 +297,12 @@ impl SequencerActorState { self.schedule_gossip(ctx, payload).await } - async fn build_unsealed_payload(&mut self, ctx: &mut SequencerContext, unsafe_head_rx: &mut watch::Receiver, in_recovery_mode: bool) -> Result, SequencerActorError> { + async fn build_unsealed_payload( + &mut self, + ctx: &mut SequencerContext, + unsafe_head_rx: &mut watch::Receiver, + in_recovery_mode: bool, + ) -> Result, SequencerActorError> { let unsafe_head = *unsafe_head_rx.borrow(); let l1_origin = match self .origin_selector @@ -337,13 +348,14 @@ impl SequencerActorState { // Build the payload attributes for the next block. let _attributes_build_start = Instant::now(); - let attrs_with_parent = match self.build_attributes(ctx, in_recovery_mode, unsafe_head, l1_origin).await? { - Some(attrs) => attrs, - None => { - // Temporary error or reset - retry on next tick. - return Ok(None); - } - }; + let attrs_with_parent = + match self.build_attributes(ctx, in_recovery_mode, unsafe_head, l1_origin).await? { + Some(attrs) => attrs, + None => { + // Temporary error or reset - retry on next tick. + return Ok(None); + } + }; // Log the attributes build duration, if metrics are enabled. kona_macros::set!( @@ -357,7 +369,9 @@ impl SequencerActorState { // Send the built attributes to the engine to be built. let _build_request_start = Instant::now(); - if let Err(err) = ctx.build_request_tx.send((attrs_with_parent.clone(), payload_id_tx)).await { + if let Err(err) = + ctx.build_request_tx.send((attrs_with_parent.clone(), payload_id_tx)).await + { error!(target: "sequencer", ?err, "Failed to send built attributes to engine"); ctx.cancellation.cancel(); return Err(SequencerActorError::ChannelClosed); From dc0c11d70b4805b930cd9ac154cea173a6f4a090 Mon Sep 17 00:00:00 2001 From: op-will <232669456+op-will@users.noreply.github.com> Date: Wed, 5 Nov 2025 16:34:42 -0600 Subject: [PATCH 03/19] Cleaning up code - Removing unused errors from Engine Tasks - Renaming structs/variables to be clearer - Updating documentation - Extracting helper functions where useful --- crates/node/engine/src/lib.rs | 6 +- .../src/task_queue/tasks/build/error.rs | 38 +----- .../engine/src/task_queue/tasks/build/task.rs | 80 +++++++---- .../node/engine/src/task_queue/tasks/mod.rs | 2 +- .../engine/src/task_queue/tasks/seal/error.rs | 60 +------- .../engine/src/task_queue/tasks/seal/mod.rs | 2 +- .../engine/src/task_queue/tasks/seal/task.rs | 129 +++++++++--------- .../node/engine/src/task_queue/tasks/task.rs | 2 +- .../node/engine/src/task_queue/tasks/util.rs | 17 --- .../node/service/src/actors/engine/actor.rs | 12 +- .../service/src/actors/sequencer/actor.rs | 32 ++--- 11 files changed, 147 insertions(+), 233 deletions(-) diff --git a/crates/node/engine/src/lib.rs b/crates/node/engine/src/lib.rs index f730c9ebc0..291f45d578 100644 --- a/crates/node/engine/src/lib.rs +++ b/crates/node/engine/src/lib.rs @@ -41,9 +41,9 @@ extern crate tracing; mod task_queue; pub use task_queue::{ BuildTask, BuildTaskError, ConsolidateTask, ConsolidateTaskError, Engine, EngineBuildError, - EngineResetError, EngineSealError, EngineTask, EngineTaskError, EngineTaskErrorSeverity, - EngineTaskErrors, EngineTaskExt, FinalizeTask, FinalizeTaskError, InsertTask, InsertTaskError, - SealTask, SealTaskError, SynchronizeTask, SynchronizeTaskError, + EngineResetError, EngineTask, EngineTaskError, EngineTaskErrorSeverity, EngineTaskErrors, + EngineTaskExt, FinalizeTask, FinalizeTaskError, InsertTask, InsertTaskError, SealTask, + SealTaskError, SynchronizeTask, SynchronizeTaskError, }; mod attributes; diff --git a/crates/node/engine/src/task_queue/tasks/build/error.rs b/crates/node/engine/src/task_queue/tasks/build/error.rs index a74639adad..8cb07f7371 100644 --- a/crates/node/engine/src/task_queue/tasks/build/error.rs +++ b/crates/node/engine/src/task_queue/tasks/build/error.rs @@ -1,12 +1,10 @@ //! Contains error types for the [crate::SynchronizeTask]. use crate::{ - EngineTaskError, InsertTaskError, SynchronizeTaskError, - task_queue::tasks::task::EngineTaskErrorSeverity, + EngineTaskError, SynchronizeTaskError, task_queue::tasks::task::EngineTaskErrorSeverity, }; use alloy_rpc_types_engine::{PayloadId, PayloadStatusEnum}; use alloy_transport::{RpcError, TransportErrorKind}; -use kona_protocol::FromBlockError; use thiserror::Error; use tokio::sync::mpsc; @@ -46,7 +44,7 @@ pub enum EngineBuildError { EngineSyncing, } -/// An error that occurs when running the [crate::SynchronizeTask]. +/// An error that occurs when running the [crate::BuildTask]. #[derive(Debug, Error)] pub enum BuildTaskError { /// An error occurred when building the payload attributes in the engine. @@ -55,41 +53,15 @@ pub enum BuildTaskError { /// The initial forkchoice update call to the engine api failed. #[error(transparent)] ForkchoiceUpdateFailed(#[from] SynchronizeTaskError), - /// Impossible to insert the payload into the engine. - #[error(transparent)] - PayloadInsertionFailed(#[from] Box), - /// The get payload call to the engine api failed. - #[error(transparent)] - GetPayloadFailed(RpcError), - /// A deposit-only payload failed to import. - #[error("Deposit-only payload failed to import")] - DepositOnlyPayloadFailed, - /// Failed to re-attempt payload import with deposit-only payload. - #[error("Failed to re-attempt payload import with deposit-only payload")] - DepositOnlyPayloadReattemptFailed, - /// The payload is invalid, and the derivation pipeline must - /// be flushed post-holocene. - #[error("Invalid payload, must flush post-holocene")] - HoloceneInvalidFlush, - /// Failed to convert a [`OpExecutionPayload`] to a [`L2BlockInfo`]. - /// - /// [`OpExecutionPayload`]: op_alloy_rpc_types_engine::OpExecutionPayload - /// [`L2BlockInfo`]: kona_protocol::L2BlockInfo - #[error(transparent)] - FromBlock(#[from] FromBlockError), /// Error sending the built payload envelope. #[error(transparent)] MpscSend(#[from] Box>), - /// The clock went backwards. - #[error("The clock went backwards")] - ClockWentBackwards, } impl EngineTaskError for BuildTaskError { fn severity(&self) -> EngineTaskErrorSeverity { match self { Self::ForkchoiceUpdateFailed(inner) => inner.severity(), - Self::PayloadInsertionFailed(inner) => inner.severity(), Self::EngineBuildError(EngineBuildError::FinalizedAheadOfUnsafe(_, _)) => { EngineTaskErrorSeverity::Critical } @@ -108,13 +80,7 @@ impl EngineTaskError for BuildTaskError { Self::EngineBuildError(EngineBuildError::EngineSyncing) => { EngineTaskErrorSeverity::Temporary } - Self::GetPayloadFailed(_) => EngineTaskErrorSeverity::Temporary, - Self::HoloceneInvalidFlush => EngineTaskErrorSeverity::Flush, - Self::DepositOnlyPayloadReattemptFailed => EngineTaskErrorSeverity::Critical, - Self::DepositOnlyPayloadFailed => EngineTaskErrorSeverity::Critical, - Self::FromBlock(_) => EngineTaskErrorSeverity::Critical, Self::MpscSend(_) => EngineTaskErrorSeverity::Critical, - Self::ClockWentBackwards => EngineTaskErrorSeverity::Critical, } } } diff --git a/crates/node/engine/src/task_queue/tasks/build/task.rs b/crates/node/engine/src/task_queue/tasks/build/task.rs index 8da56cdbd4..dd3594ff62 100644 --- a/crates/node/engine/src/task_queue/tasks/build/task.rs +++ b/crates/node/engine/src/task_queue/tasks/build/task.rs @@ -33,12 +33,52 @@ pub struct BuildTask { pub cfg: Arc, /// The [`OpAttributesWithParent`] to instruct the execution layer to build. pub attributes: OpAttributesWithParent, - /// The optional sender to which [`PayloadId`] will be sent after the block build has been - /// started. + /// The optional sender through which [`PayloadId`] will be sent after the + /// block build has been started. pub payload_id_tx: Option>, } impl BuildTask { + /// Validates the provided [PayloadStatusEnum] according to the rules listed below. + /// + /// ## Observed [PayloadStatusEnum] Variants + /// The `engine_forkchoiceUpdate` payload statuses that this function observes are below. Any + /// other [PayloadStatusEnum] variant is considered a failure. + /// + /// ### Success (`VALID`) + /// If the build is successful, the [PayloadId] is returned for sealing and the external + /// actor is notified of the successful forkchoice update. + /// + /// ### Failure (`INVALID`) + /// If the forkchoice update fails, the external actor is notified of the failure. + /// + /// Validates the payload status from a forkchoice update response. + /// + /// ## Observed [PayloadStatusEnum] Variants + /// - `VALID`: Returns Ok(()) - forkchoice update was successful + /// - `INVALID`: Returns error with validation details + /// - `SYNCING`: Returns temporary error - EL is syncing + /// - Other: Returns error for unexpected status codes + fn validate_forkchoice_status(status: PayloadStatusEnum) -> Result<(), BuildTaskError> { + match status { + PayloadStatusEnum::Valid => Ok(()), + PayloadStatusEnum::Invalid { validation_error } => { + error!(target: "engine_builder", "Forkchoice update failed: {}", validation_error); + Err(BuildTaskError::EngineBuildError(EngineBuildError::InvalidPayload( + validation_error, + ))) + } + PayloadStatusEnum::Syncing => { + warn!(target: "engine_builder", "Forkchoice update failed temporarily: EL is syncing"); + Err(BuildTaskError::EngineBuildError(EngineBuildError::EngineSyncing)) + } + s => { + // Other codes are never returned by `engine_forkchoiceUpdate` + Err(BuildTaskError::EngineBuildError(EngineBuildError::UnexpectedPayloadStatus(s))) + } + } + } + /// Starts the block building process by sending an initial `engine_forkchoiceUpdate` call with /// the payload attributes to build. /// @@ -104,33 +144,15 @@ impl BuildTask { BuildTaskError::EngineBuildError(EngineBuildError::AttributesInsertionFailed(e)) })?; - match update.payload_status.status { - PayloadStatusEnum::Valid => { - debug!( - target: "engine_builder", - unsafe_hash = new_forkchoice.head_block_hash.to_string(), - safe_hash = new_forkchoice.safe_block_hash.to_string(), - finalized_hash = new_forkchoice.finalized_block_hash.to_string(), - "Forkchoice update with attributes successful" - ); - } - PayloadStatusEnum::Invalid { validation_error } => { - error!(target: "engine_builder", "Forkchoice update failed: {}", validation_error); - return Err(BuildTaskError::EngineBuildError(EngineBuildError::InvalidPayload( - validation_error, - ))); - } - PayloadStatusEnum::Syncing => { - warn!(target: "engine_builder", "Forkchoice update failed temporarily: EL is syncing"); - return Err(BuildTaskError::EngineBuildError(EngineBuildError::EngineSyncing)); - } - s => { - // Other codes are never returned by `engine_forkchoiceUpdate` - return Err(BuildTaskError::EngineBuildError( - EngineBuildError::UnexpectedPayloadStatus(s), - )); - } - } + Self::validate_forkchoice_status(update.payload_status.status)?; + + debug!( + target: "engine_builder", + unsafe_hash = new_forkchoice.head_block_hash.to_string(), + safe_hash = new_forkchoice.safe_block_hash.to_string(), + finalized_hash = new_forkchoice.finalized_block_hash.to_string(), + "Forkchoice update with attributes successful" + ); // Fetch the payload ID from the FCU. If no payload ID was returned, something went wrong - // the block building job on the EL should have been initiated. diff --git a/crates/node/engine/src/task_queue/tasks/mod.rs b/crates/node/engine/src/task_queue/tasks/mod.rs index 562c1d5637..d24d75fdf3 100644 --- a/crates/node/engine/src/task_queue/tasks/mod.rs +++ b/crates/node/engine/src/task_queue/tasks/mod.rs @@ -15,7 +15,7 @@ mod build; pub use build::{BuildTask, BuildTaskError, EngineBuildError}; mod seal; -pub use seal::{EngineSealError, SealTask, SealTaskError}; +pub use seal::{SealTask, SealTaskError}; mod consolidate; pub use consolidate::{ConsolidateTask, ConsolidateTaskError}; diff --git a/crates/node/engine/src/task_queue/tasks/seal/error.rs b/crates/node/engine/src/task_queue/tasks/seal/error.rs index 553f969d91..288dfe7599 100644 --- a/crates/node/engine/src/task_queue/tasks/seal/error.rs +++ b/crates/node/engine/src/task_queue/tasks/seal/error.rs @@ -1,55 +1,15 @@ //! Contains error types for the [crate::SynchronizeTask]. use crate::{EngineTaskError, InsertTaskError, task_queue::tasks::task::EngineTaskErrorSeverity}; -use alloy_rpc_types_engine::PayloadStatusEnum; use alloy_transport::{RpcError, TransportErrorKind}; use kona_protocol::FromBlockError; use op_alloy_rpc_types_engine::OpExecutionPayloadEnvelope; use thiserror::Error; use tokio::sync::mpsc; -/// An error that occurs during payload building within the engine. -/// -/// This error type is specific to the block building process and represents failures -/// that can occur during the automatic forkchoice update phase of [`SealTask`]. -/// Unlike [`SealTaskError`], which handles higher-level build orchestration errors, -/// `EngineSealError` focuses on low-level engine API communication failures. -/// -/// ## Error Categories -/// -/// - **State Validation**: Errors related to inconsistent chain state -/// - **Engine Communication**: RPC failures during forkchoice updates -/// - **Payload Validation**: Invalid payload status responses from the execution layer -/// -/// [`SealTask`]: crate::SealTask -#[derive(Debug, Error)] -pub enum EngineSealError { - /// The finalized head is ahead of the unsafe head. - #[error("Finalized head is ahead of unsafe head")] - FinalizedAheadOfUnsafe(u64, u64), - /// The forkchoice update call to the engine api failed. - #[error("Failed to build payload attributes in the engine. Forkchoice RPC error: {0}")] - AttributesInsertionFailed(#[from] RpcError), - /// The inserted payload is invalid. - #[error("The inserted payload is invalid: {0}")] - InvalidPayload(String), - /// The inserted payload status is unexpected. - #[error("The inserted payload status is unexpected: {0}")] - UnexpectedPayloadStatus(PayloadStatusEnum), - /// The payload ID is missing. - #[error("The inserted payload ID is missing")] - MissingPayloadId, - /// The engine is syncing. - #[error("The engine is syncing")] - EngineSyncing, -} - -/// An error that occurs when running the [crate::SynchronizeTask]. +/// An error that occurs when running the [crate::SealTask]. #[derive(Debug, Error)] pub enum SealTaskError { - /// An error occurred when sealing the payload attributes in the engine. - #[error("An error occurred when sealing the payload attributes in the engine.")] - EngineSealError(EngineSealError), /// Impossible to insert the payload into the engine. #[error(transparent)] PayloadInsertionFailed(#[from] Box), @@ -84,24 +44,6 @@ impl EngineTaskError for SealTaskError { fn severity(&self) -> EngineTaskErrorSeverity { match self { Self::PayloadInsertionFailed(inner) => inner.severity(), - Self::EngineSealError(EngineSealError::FinalizedAheadOfUnsafe(_, _)) => { - EngineTaskErrorSeverity::Critical - } - Self::EngineSealError(EngineSealError::AttributesInsertionFailed(_)) => { - EngineTaskErrorSeverity::Temporary - } - Self::EngineSealError(EngineSealError::InvalidPayload(_)) => { - EngineTaskErrorSeverity::Temporary - } - Self::EngineSealError(EngineSealError::UnexpectedPayloadStatus(_)) => { - EngineTaskErrorSeverity::Temporary - } - Self::EngineSealError(EngineSealError::MissingPayloadId) => { - EngineTaskErrorSeverity::Temporary - } - Self::EngineSealError(EngineSealError::EngineSyncing) => { - EngineTaskErrorSeverity::Temporary - } Self::GetPayloadFailed(_) => EngineTaskErrorSeverity::Temporary, Self::HoloceneInvalidFlush => EngineTaskErrorSeverity::Flush, Self::DepositOnlyPayloadReattemptFailed => EngineTaskErrorSeverity::Critical, diff --git a/crates/node/engine/src/task_queue/tasks/seal/mod.rs b/crates/node/engine/src/task_queue/tasks/seal/mod.rs index d91dee604d..45d94e0013 100644 --- a/crates/node/engine/src/task_queue/tasks/seal/mod.rs +++ b/crates/node/engine/src/task_queue/tasks/seal/mod.rs @@ -4,4 +4,4 @@ mod task; pub use task::SealTask; mod error; -pub use error::{EngineSealError, SealTaskError}; +pub use error::SealTaskError; diff --git a/crates/node/engine/src/task_queue/tasks/seal/task.rs b/crates/node/engine/src/task_queue/tasks/seal/task.rs index 553457d62e..9c1966565e 100644 --- a/crates/node/engine/src/task_queue/tasks/seal/task.rs +++ b/crates/node/engine/src/task_queue/tasks/seal/task.rs @@ -7,17 +7,15 @@ use crate::{ }; use alloy_rpc_types_engine::{ExecutionPayload, PayloadId}; use async_trait::async_trait; +use derive_more::Constructor; use kona_genesis::RollupConfig; use kona_protocol::{L2BlockInfo, OpAttributesWithParent}; use op_alloy_provider::ext::engine::OpEngineApi; use op_alloy_rpc_types_engine::{OpExecutionPayload, OpExecutionPayloadEnvelope}; -use std::{ - sync::Arc, - time::{Duration, Instant}, -}; +use std::{sync::Arc, time::Instant}; use tokio::sync::mpsc; -/// Task for sealing blocks. +/// Task for block sealing and canonicalization. /// /// The [`SealTask`] handles the following parts of the block building workflow: /// @@ -30,7 +28,7 @@ use tokio::sync::mpsc; /// /// [`InsertTask`]: crate::InsertTask /// [`InsertTaskError`]: crate::InsertTaskError -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Constructor)] pub struct SealTask { /// The engine API client. pub engine: Arc, @@ -42,24 +40,12 @@ pub struct SealTask { pub attributes: OpAttributesWithParent, /// Whether or not the payload was derived, or created by the sequencer. pub is_attributes_derived: bool, - /// An optional channel to send the built [`OpExecutionPayloadEnvelope`] to, after the block - /// has been built, imported, and canonicalized. + /// An optional sender through which the built [`OpExecutionPayloadEnvelope`] will be sent + /// after the block has been built, imported, and canonicalized. pub payload_tx: Option>, } impl SealTask { - /// Creates a new block building task. - pub const fn new( - engine: Arc, - cfg: Arc, - payload_id: PayloadId, - attributes: OpAttributesWithParent, - is_attributes_derived: bool, - payload_tx: Option>, - ) -> Self { - Self { engine, cfg, payload_id, attributes, is_attributes_derived, payload_tx } - } - /// Fetches the execution payload from the EL. /// /// ## Engine Method Selection @@ -79,7 +65,7 @@ impl SealTask { let payload_timestamp = payload_attrs.inner().payload_attributes.timestamp; debug!( - target: "engine_builder", + target: "engine", payload_id = payload_id.to_string(), l2_time = payload_timestamp, "Inserting payload" @@ -89,7 +75,7 @@ impl SealTask { let payload_envelope = match get_payload_version { EngineGetPayloadVersion::V4 => { let payload = engine.get_payload_v4(payload_id).await.map_err(|e| { - error!(target: "engine_builder", "Payload fetch failed: {e}"); + error!(target: "engine", "Payload fetch failed: {e}"); SealTaskError::GetPayloadFailed(e) })?; @@ -100,7 +86,7 @@ impl SealTask { } EngineGetPayloadVersion::V3 => { let payload = engine.get_payload_v3(payload_id).await.map_err(|e| { - error!(target: "engine_builder", "Payload fetch failed: {e}"); + error!(target: "engine", "Payload fetch failed: {e}"); SealTaskError::GetPayloadFailed(e) })?; @@ -111,7 +97,7 @@ impl SealTask { } EngineGetPayloadVersion::V2 => { let payload = engine.get_payload_v2(payload_id).await.map_err(|e| { - error!(target: "engine_builder", "Payload fetch failed: {e}"); + error!(target: "engine", "Payload fetch failed: {e}"); SealTaskError::GetPayloadFailed(e) })?; @@ -129,32 +115,19 @@ impl SealTask { Ok(payload_envelope) } - /// Seals the block by waiting for the appropriate time, fetching the payload, and importing it. + /// Inserts a payload into the engine with Holocene fallback support. /// /// This function handles: - /// 1. Computing and waiting for the seal time (with a buffer) - /// 2. Fetching the execution payload from the EL - /// 3. Importing the payload into the engine with Holocene fallback support - /// 4. Sending the payload to the optional channel + /// 1. Executing the InsertTask to import the payload + /// 2. Handling deposits-only payload failures + /// 3. Holocene fallback via build_and_seal if needed /// - /// Returns the imported block info and the duration taken to import the block. - async fn seal_block( + /// Returns Ok(()) if the payload is successfully inserted, or an error if insertion fails. + async fn insert_payload( &self, state: &mut EngineState, - ) -> Result<(L2BlockInfo, Duration), SealTaskError> { - // Fetch the payload just inserted from the EL and import it into the engine. - let block_import_start_time = Instant::now(); - let new_payload = self - .fetch_payload(&self.cfg, &self.engine, self.payload_id, self.attributes.clone()) - .await?; - - let new_block_ref = L2BlockInfo::from_payload_and_genesis( - new_payload.execution_payload.clone(), - self.attributes.inner().payload_attributes.parent_beacon_block_root, - &self.cfg.genesis, - ) - .map_err(SealTaskError::FromBlock)?; - + new_payload: OpExecutionPayloadEnvelope, + ) -> Result<(), SealTaskError> { // Insert the new block into the engine. match InsertTask::new( Arc::clone(&self.engine), @@ -168,16 +141,15 @@ impl SealTask { Err(InsertTaskError::UnexpectedPayloadStatus(e)) if self.attributes.is_deposits_only() => { - error!(target: "engine_sealer", error = ?e, "Critical: Deposit-only payload import failed"); + error!(target: "engine", error = ?e, "Critical: Deposit-only payload import failed"); return Err(SealTaskError::DepositOnlyPayloadFailed); } - // HOLOCENE: Re-attempt payload import with deposits only Err(InsertTaskError::UnexpectedPayloadStatus(e)) if self .cfg .is_holocene_active(self.attributes.inner().payload_attributes.timestamp) => { - warn!(target: "engine_sealer", error = ?e, "Re-attempting payload import with deposits only."); + warn!(target: "engine", error = ?e, "Re-attempting payload import with deposits only."); // HOLOCENE: Re-attempt payload import with deposits only // First build the deposits-only payload, then seal it @@ -194,21 +166,50 @@ impl SealTask { .await { Ok(_) => { - info!(target: "engine_sealer", "Successfully imported deposits-only payload"); + info!(target: "engine", "Successfully imported deposits-only payload"); Err(SealTaskError::HoloceneInvalidFlush) } - Err(_) => return Err(SealTaskError::DepositOnlyPayloadReattemptFailed), + Err(_) => Err(SealTaskError::DepositOnlyPayloadReattemptFailed), } } Err(e) => { - error!(target: "engine_sealer", "Payload import failed: {e}"); + error!(target: "engine", "Payload import failed: {e}"); return Err(Box::new(e).into()); } Ok(_) => { - info!(target: "engine_sealer", "Successfully imported payload") + info!(target: "engine", "Successfully imported payload") } } + Ok(()) + } + + /// Seals and canonicalizes the block by fetching the payload and importing it. + /// + /// This function handles: + /// 1. Fetching the execution payload from the EL + /// 2. Importing the payload into the engine with Holocene fallback support + /// 3. Sending the payload to the optional channel + async fn seal_and_canonicalize_block( + &self, + state: &mut EngineState, + ) -> Result<(), SealTaskError> { + // Fetch the payload just inserted from the EL and import it into the engine. + let block_import_start_time = Instant::now(); + let new_payload = self + .fetch_payload(&self.cfg, &self.engine, self.payload_id, self.attributes.clone()) + .await?; + + let new_block_ref = L2BlockInfo::from_payload_and_genesis( + new_payload.execution_payload.clone(), + self.attributes.inner().payload_attributes.parent_beacon_block_root, + &self.cfg.genesis, + ) + .map_err(SealTaskError::FromBlock)?; + + // Insert the payload into the engine. + self.insert_payload(state, new_payload.clone()).await?; + let block_import_duration = block_import_start_time.elapsed(); // If a channel was provided, send the built payload envelope to it. @@ -216,7 +217,16 @@ impl SealTask { tx.send(new_payload).await.map_err(Box::new).map_err(SealTaskError::MpscSend)?; } - Ok((new_block_ref, block_import_duration)) + info!( + target: "engine", + l2_number = new_block_ref.block_info.number, + l2_time = new_block_ref.block_info.timestamp, + block_import_duration = ?block_import_duration, + "Built and imported new {} block", + if self.is_attributes_derived { "safe" } else { "unsafe" }, + ); + + Ok(()) } } @@ -228,23 +238,14 @@ impl EngineTaskExt for SealTask { async fn execute(&self, state: &mut EngineState) -> Result<(), SealTaskError> { debug!( - target: "engine_sealer", + target: "engine", txs = self.attributes.inner().transactions.as_ref().map_or(0, |txs| txs.len()), is_deposits = self.attributes.is_deposits_only(), "Starting new seal job" ); // Seal the block and import it into the engine. - let (new_block_ref, block_import_duration) = self.seal_block(state).await?; - - info!( - target: "engine_sealer", - l2_number = new_block_ref.block_info.number, - l2_time = new_block_ref.block_info.timestamp, - block_import_duration = ?block_import_duration, - "Built and imported new {} block", - if self.is_attributes_derived { "safe" } else { "unsafe" }, - ); + self.seal_and_canonicalize_block(state).await?; Ok(()) } diff --git a/crates/node/engine/src/task_queue/tasks/task.rs b/crates/node/engine/src/task_queue/tasks/task.rs index de4b0e24f4..c1852a9b04 100644 --- a/crates/node/engine/src/task_queue/tasks/task.rs +++ b/crates/node/engine/src/task_queue/tasks/task.rs @@ -93,7 +93,7 @@ impl EngineTaskError for EngineTaskErrors { pub enum EngineTask { /// Inserts a payload into the execution engine. Insert(Box), - /// Begins building a new block with the given attributes. + /// Begins building a new block with the given attributes, producing a new payload ID. Build(Box), /// Seals the block with the given payload ID and attributes, inserting it into the execution /// engine. diff --git a/crates/node/engine/src/task_queue/tasks/util.rs b/crates/node/engine/src/task_queue/tasks/util.rs index c7dd523850..534bc7988c 100644 --- a/crates/node/engine/src/task_queue/tasks/util.rs +++ b/crates/node/engine/src/task_queue/tasks/util.rs @@ -36,23 +36,6 @@ pub(in crate::task_queue) enum BuildAndSealError { /// * `attributes` - The payload attributes to build /// * `is_attributes_derived` - Whether the attributes were derived or created by the sequencer /// * `payload_tx` - Optional channel to send the built payload envelope -/// -/// # Returns -/// -/// Returns `Ok(())` if both build and seal operations succeed, or an error if either fails. -/// -/// # Examples -/// -/// ```ignore -/// let result = build_and_seal( -/// engine.clone(), -/// cfg.clone(), -/// deposits_only_attrs, -/// true, -/// Some(tx), -/// &mut state, -/// ).await?; -/// ``` pub(in crate::task_queue) async fn build_and_seal( state: &mut EngineState, engine: Arc, diff --git a/crates/node/service/src/actors/engine/actor.rs b/crates/node/service/src/actors/engine/actor.rs index 0e264aa90d..bc58a57de4 100644 --- a/crates/node/service/src/actors/engine/actor.rs +++ b/crates/node/service/src/actors/engine/actor.rs @@ -38,14 +38,14 @@ pub struct EngineActor { reset_request_rx: mpsc::Receiver<()>, /// Handler for inbound queries to the engine. inbound_queries: mpsc::Receiver, - /// A channel to receive build requests from the sequencer actor. + /// A channel to receive build requests. /// Upon successful processing of the provided attributes, a `PayloadId` will be sent via the /// provided sender. /// ## Note /// This is `Some` when the node is in sequencer mode, and `None` when the node is in validator /// mode. build_request_rx: Option)>>, - /// A channel to receive seal requests from the sequencer actor. + /// A channel to receive seal requests. /// Upon successful seal of the provided attributes, the resulting `OpExecutionPayloadEnvelope` /// will be sent via provided sender. /// ## Note @@ -65,14 +65,14 @@ pub struct EngineActor { /// The outbound data for the [`EngineActor`]. #[derive(Debug)] pub struct EngineInboundData { - /// A channel to send build requests from the sequencer actor. + /// A channel to use to send build requests to the [`EngineActor`]. /// Upon successful processing of the provided attributes, a `PayloadId` will be sent via the /// provided sender. /// ## Note /// This is `Some` when the node is in sequencer mode, and `None` when the node is in validator /// mode. pub build_request_tx: Option)>>, - /// A channel to send seal requests from the sequencer actor. + /// A channel to send seal requests to the [`EngineActor`]. /// Upon successful seal of the provided attributes, the resulting `OpExecutionPayloadEnvelope` /// will be sent via provided sender. /// ## Note @@ -486,8 +486,8 @@ impl NodeActor for EngineActor { ))); state.engine.enqueue(task); } - Some(res) = OptionFuture::from(self.build_request_rx.as_mut().map(|rx| rx.recv())), if self.build_request_rx.is_some() => { - let Some((attributes, response_tx)) = res else { + Some(req) = OptionFuture::from(self.build_request_rx.as_mut().map(|rx| rx.recv())), if self.build_request_rx.is_some() => { + let Some((attributes, response_tx)) = req else { error!(target: "engine", "Build request receiver closed unexpectedly while in sequencer mode"); cancellation.cancel(); return Err(EngineError::ChannelClosed); diff --git a/crates/node/service/src/actors/sequencer/actor.rs b/crates/node/service/src/actors/sequencer/actor.rs index 207bb49587..7accda6439 100644 --- a/crates/node/service/src/actors/sequencer/actor.rs +++ b/crates/node/service/src/actors/sequencer/actor.rs @@ -40,7 +40,7 @@ pub struct SequencerActor { /// The handle to a block that has been started but not sealed. #[derive(Debug, Constructor)] -pub(super) struct UnsealedPayloadData { +pub(super) struct UnsealedPayloadHandle { /// The [`PayloadId`] of the unsealed payload. pub payload_id: PayloadId, /// The [`OpAttributesWithParent`] used to start block building. @@ -183,11 +183,11 @@ pub struct SequencerContext { pub l1_head_rx: watch::Receiver>, /// Sender to request the engine to reset. pub reset_request_tx: mpsc::Sender<()>, - /// Sender to request the execution layer to start building a payload attributes on top of the + /// Sender to request the execution layer to start building a payload on top of the /// current unsafe head. pub build_request_tx: mpsc::Sender<(OpAttributesWithParent, mpsc::Sender)>, - /// Sender to request the execution layer to seal the payload ID and attributes that - /// resulted from a previous build call. + /// Sender to request the execution layer to seal and commit the payload ID and + /// attributes that resulted from a previous build call. pub seal_request_tx: mpsc::Sender<(PayloadId, OpAttributesWithParent, mpsc::Sender)>, /// A sender to asynchronously sign and gossip built [`OpExecutionPayloadEnvelope`]s to the @@ -230,15 +230,15 @@ impl SequencerActorState { /// Seals and commits the last pending block, if one exists and starts the build job for the /// next L2 block, on top of the current unsafe head. /// - /// In the success case, this function stores the [`UnsealedPayloadData`] for the block it began - /// in state so that it may be sealed and committed the next time this function is called. + /// If a new block was started, it will return the associated [`UnsealedPayloadHandle`] so + /// that it may be sealed and committed in a future call to this function. async fn seal_last_and_start_next( &mut self, ctx: &mut SequencerContext, unsafe_head_rx: &mut watch::Receiver, in_recovery_mode: bool, - payload_to_seal: Option, - ) -> Result, SequencerActorError> { + payload_to_seal: Option, + ) -> Result, SequencerActorError> { if let Some(to_seal) = payload_to_seal { self.seal_and_commit_payload_if_applicable(ctx, to_seal).await?; } @@ -249,14 +249,14 @@ impl SequencerActorState { Ok(unsealed_payload) } - /// Sends a seal request to seal the provided [`UnsealedPayloadData`], committing and gossiping - /// the resulting block, if one is built. + /// Sends a seal request to seal the provided [`UnsealedPayloadHandle`], committing and + /// gossiping the resulting block, if one is built. async fn seal_and_commit_payload_if_applicable( &mut self, ctx: &mut SequencerContext, - unsealed_payload_data: UnsealedPayloadData, + unsealed_payload_handle: UnsealedPayloadHandle, ) -> Result<(), SequencerActorError> { - let UnsealedPayloadData { payload_id, attributes_with_parent } = unsealed_payload_data; + let UnsealedPayloadHandle { payload_id, attributes_with_parent } = unsealed_payload_handle; // Create a new channel to receive the built payload. let (payload_tx, payload_rx) = mpsc::channel(1); @@ -302,7 +302,7 @@ impl SequencerActorState { ctx: &mut SequencerContext, unsafe_head_rx: &mut watch::Receiver, in_recovery_mode: bool, - ) -> Result, SequencerActorError> { + ) -> Result, SequencerActorError> { let unsafe_head = *unsafe_head_rx.borrow(); let l1_origin = match self .origin_selector @@ -379,7 +379,7 @@ impl SequencerActorState { let payload_id = self.try_wait_for_payload_id(ctx, payload_id_rx).await?; - Ok(Some(UnsealedPayloadData::new(payload_id, attrs_with_parent))) + Ok(Some(UnsealedPayloadHandle::new(payload_id, attrs_with_parent))) } /// Builds the OpAttributesWithParent for the next block to build. If None is returned, it @@ -584,7 +584,7 @@ impl NodeActor for SequencerActor { // Reset the engine state prior to beginning block building. state.schedule_initial_reset(&mut ctx, &mut self.unsafe_head_rx).await?; - let mut next_block_to_seal = None; + let mut next_payload_to_seal = None; loop { select! { @@ -620,7 +620,7 @@ impl NodeActor for SequencerActor { let start_time = Instant::now(); - next_block_to_seal = state.seal_last_and_start_next(&mut ctx, &mut self.unsafe_head_rx, state.is_recovery_mode, next_block_to_seal).await?; + next_payload_to_seal = state.seal_last_and_start_next(&mut ctx, &mut self.unsafe_head_rx, state.is_recovery_mode, next_payload_to_seal).await?; // Set the next tick time to the configured blocktime less the time it takes to seal last and start next. state.build_ticker.reset_after(Duration::from_secs(state.cfg.block_time).saturating_sub(start_time.elapsed())); From 5ec6c42e28faf0bf1c74ce49517fc53cb8460944 Mon Sep 17 00:00:00 2001 From: op-will <232669456+op-will@users.noreply.github.com> Date: Thu, 6 Nov 2025 14:39:21 -0600 Subject: [PATCH 04/19] Relaying seal errors from engine to sequencer This allows the sequencer to branch on error type/severity rather than fail if any error happens --- .../src/task_queue/tasks/consolidate/task.rs | 1 - .../engine/src/task_queue/tasks/seal/error.rs | 4 +- .../engine/src/task_queue/tasks/seal/task.rs | 27 +++---- .../node/engine/src/task_queue/tasks/util.rs | 6 +- .../node/service/src/actors/engine/actor.rs | 17 +++-- crates/node/service/src/actors/engine/mod.rs | 1 + .../service/src/actors/sequencer/actor.rs | 70 ++++++++++++++----- crates/node/service/src/service/core.rs | 2 +- 8 files changed, 83 insertions(+), 45 deletions(-) diff --git a/crates/node/engine/src/task_queue/tasks/consolidate/task.rs b/crates/node/engine/src/task_queue/tasks/consolidate/task.rs index 246d3e9518..8722d59d58 100644 --- a/crates/node/engine/src/task_queue/tasks/consolidate/task.rs +++ b/crates/node/engine/src/task_queue/tasks/consolidate/task.rs @@ -47,7 +47,6 @@ impl ConsolidateTask { self.cfg.clone(), self.attributes.clone(), self.is_attributes_derived, - None, ) .await .map_err(Into::into) diff --git a/crates/node/engine/src/task_queue/tasks/seal/error.rs b/crates/node/engine/src/task_queue/tasks/seal/error.rs index 288dfe7599..d740fd79d3 100644 --- a/crates/node/engine/src/task_queue/tasks/seal/error.rs +++ b/crates/node/engine/src/task_queue/tasks/seal/error.rs @@ -34,7 +34,9 @@ pub enum SealTaskError { FromBlock(#[from] FromBlockError), /// Error sending the built payload envelope. #[error(transparent)] - MpscSend(#[from] Box>), + MpscSend( + #[from] Box>>, + ), /// The clock went backwards. #[error("The clock went backwards")] ClockWentBackwards, diff --git a/crates/node/engine/src/task_queue/tasks/seal/task.rs b/crates/node/engine/src/task_queue/tasks/seal/task.rs index 9c1966565e..a89fe08079 100644 --- a/crates/node/engine/src/task_queue/tasks/seal/task.rs +++ b/crates/node/engine/src/task_queue/tasks/seal/task.rs @@ -40,9 +40,10 @@ pub struct SealTask { pub attributes: OpAttributesWithParent, /// Whether or not the payload was derived, or created by the sequencer. pub is_attributes_derived: bool, - /// An optional sender through which the built [`OpExecutionPayloadEnvelope`] will be sent - /// after the block has been built, imported, and canonicalized. - pub payload_tx: Option>, + /// An optional sender to convey success/failure result of the built + /// [`OpExecutionPayloadEnvelope`] after the block has been built, imported, and canonicalized + /// or the [`SealTaskError`] that occurred during processing. + pub result_tx: Option>>, } impl SealTask { @@ -161,7 +162,6 @@ impl SealTask { self.cfg.clone(), deposits_only_attrs.clone(), self.is_attributes_derived, - None, ) .await { @@ -193,7 +193,7 @@ impl SealTask { async fn seal_and_canonicalize_block( &self, state: &mut EngineState, - ) -> Result<(), SealTaskError> { + ) -> Result { // Fetch the payload just inserted from the EL and import it into the engine. let block_import_start_time = Instant::now(); let new_payload = self @@ -212,11 +212,6 @@ impl SealTask { let block_import_duration = block_import_start_time.elapsed(); - // If a channel was provided, send the built payload envelope to it. - if let Some(tx) = &self.payload_tx { - tx.send(new_payload).await.map_err(Box::new).map_err(SealTaskError::MpscSend)?; - } - info!( target: "engine", l2_number = new_block_ref.block_info.number, @@ -226,7 +221,7 @@ impl SealTask { if self.is_attributes_derived { "safe" } else { "unsafe" }, ); - Ok(()) + Ok(new_payload) } } @@ -245,7 +240,15 @@ impl EngineTaskExt for SealTask { ); // Seal the block and import it into the engine. - self.seal_and_canonicalize_block(state).await?; + let res = self.seal_and_canonicalize_block(state).await; + + // NB: If a response channel is provided, that channel will receive success/failure info, + // and this task will always succeed. If not, task failure will be relayed to the caller. + if let Some(tx) = &self.result_tx { + tx.send(res).await.map_err(Box::new).map_err(SealTaskError::MpscSend)?; + } else if let Err(x) = res { + return Err(x) + } Ok(()) } diff --git a/crates/node/engine/src/task_queue/tasks/util.rs b/crates/node/engine/src/task_queue/tasks/util.rs index 534bc7988c..4ddb41eca7 100644 --- a/crates/node/engine/src/task_queue/tasks/util.rs +++ b/crates/node/engine/src/task_queue/tasks/util.rs @@ -4,9 +4,7 @@ use super::{BuildTask, BuildTaskError, EngineTaskExt, SealTask, SealTaskError}; use crate::{EngineClient, EngineState}; use kona_genesis::RollupConfig; use kona_protocol::OpAttributesWithParent; -use op_alloy_rpc_types_engine::OpExecutionPayloadEnvelope; use std::sync::Arc; -use tokio::sync::mpsc; /// Error type for build and seal operations. #[derive(Debug, thiserror::Error)] @@ -35,14 +33,12 @@ pub(in crate::task_queue) enum BuildAndSealError { /// * `cfg` - The rollup configuration /// * `attributes` - The payload attributes to build /// * `is_attributes_derived` - Whether the attributes were derived or created by the sequencer -/// * `payload_tx` - Optional channel to send the built payload envelope pub(in crate::task_queue) async fn build_and_seal( state: &mut EngineState, engine: Arc, cfg: Arc, attributes: OpAttributesWithParent, is_attributes_derived: bool, - payload_tx: Option>, ) -> Result<(), BuildAndSealError> { // Execute the build task let payload_id = BuildTask::new( @@ -55,7 +51,7 @@ pub(in crate::task_queue) async fn build_and_seal( .await?; // Execute the seal task with the payload ID from the build - SealTask::new(engine, cfg, payload_id, attributes, is_attributes_derived, payload_tx) + SealTask::new(engine, cfg, payload_id, attributes, is_attributes_derived, None) .execute(state) .await?; diff --git a/crates/node/service/src/actors/engine/actor.rs b/crates/node/service/src/actors/engine/actor.rs index bc58a57de4..ed213ff6d1 100644 --- a/crates/node/service/src/actors/engine/actor.rs +++ b/crates/node/service/src/actors/engine/actor.rs @@ -8,7 +8,7 @@ use kona_derive::{ResetSignal, Signal}; use kona_engine::{ BuildTask, ConsolidateTask, Engine, EngineClient, EngineQueries, EngineState as InnerEngineState, EngineTask, EngineTaskError, EngineTaskErrorSeverity, - InsertTask, SealTask, + InsertTask, SealTask, SealTaskError, }; use kona_genesis::RollupConfig; use kona_protocol::{BlockInfo, L2BlockInfo, OpAttributesWithParent}; @@ -46,8 +46,7 @@ pub struct EngineActor { /// mode. build_request_rx: Option)>>, /// A channel to receive seal requests. - /// Upon successful seal of the provided attributes, the resulting `OpExecutionPayloadEnvelope` - /// will be sent via provided sender. + /// The success/fail result of the sealing operation will be sent via the provided sender. /// ## Note /// This is `Some` when the node is in sequencer mode, and `None` when the node is in validator /// mode. @@ -55,7 +54,7 @@ pub struct EngineActor { mpsc::Receiver<( PayloadId, OpAttributesWithParent, - mpsc::Sender, + mpsc::Sender>, )>, >, /// The [`L2Finalizer`], used to finalize L2 blocks. @@ -73,13 +72,17 @@ pub struct EngineInboundData { /// mode. pub build_request_tx: Option)>>, /// A channel to send seal requests to the [`EngineActor`]. - /// Upon successful seal of the provided attributes, the resulting `OpExecutionPayloadEnvelope` - /// will be sent via provided sender. + /// If provided, the success/fail result of the sealing operation will be sent via the provided + /// sender. /// ## Note /// This is `Some` when the node is in sequencer mode, and `None` when the node is in validator /// mode. pub seal_request_tx: Option< - mpsc::Sender<(PayloadId, OpAttributesWithParent, mpsc::Sender)>, + mpsc::Sender<( + PayloadId, + OpAttributesWithParent, + mpsc::Sender>, + )>, >, /// A channel to send [`OpAttributesWithParent`] to the engine actor. pub attributes_tx: mpsc::Sender, diff --git a/crates/node/service/src/actors/engine/mod.rs b/crates/node/service/src/actors/engine/mod.rs index 22896cf654..42a13ec040 100644 --- a/crates/node/service/src/actors/engine/mod.rs +++ b/crates/node/service/src/actors/engine/mod.rs @@ -7,4 +7,5 @@ mod error; pub use error::EngineError; mod finalizer; + pub use finalizer::L2Finalizer; diff --git a/crates/node/service/src/actors/sequencer/actor.rs b/crates/node/service/src/actors/sequencer/actor.rs index 7accda6439..d50c91b5f0 100644 --- a/crates/node/service/src/actors/sequencer/actor.rs +++ b/crates/node/service/src/actors/sequencer/actor.rs @@ -9,6 +9,7 @@ use alloy_rpc_types_engine::PayloadId; use async_trait::async_trait; use derive_more::Constructor; use kona_derive::{AttributesBuilder, PipelineErrorKind, StatefulAttributesBuilder}; +use kona_engine::{EngineTaskError, EngineTaskErrorSeverity, SealTaskError}; use kona_genesis::{L1ChainConfig, RollupConfig}; use kona_protocol::{BlockInfo, L2BlockInfo, OpAttributesWithParent}; use kona_providers_alloy::{AlloyChainProvider, AlloyL2ChainProvider}; @@ -188,8 +189,11 @@ pub struct SequencerContext { pub build_request_tx: mpsc::Sender<(OpAttributesWithParent, mpsc::Sender)>, /// Sender to request the execution layer to seal and commit the payload ID and /// attributes that resulted from a previous build call. - pub seal_request_tx: - mpsc::Sender<(PayloadId, OpAttributesWithParent, mpsc::Sender)>, + pub seal_request_tx: mpsc::Sender<( + PayloadId, + OpAttributesWithParent, + mpsc::Sender>, + )>, /// A sender to asynchronously sign and gossip built [`OpExecutionPayloadEnvelope`]s to the /// network actor. pub gossip_payload_tx: mpsc::Sender, @@ -207,12 +211,15 @@ pub enum SequencerActorError { /// An error occurred while building payload attributes. #[error(transparent)] AttributesBuilder(#[from] PipelineErrorKind), - /// An error occurred while selecting the next L1 origin. - #[error(transparent)] - L1OriginSelector(#[from] L1OriginSelectorError), /// A channel was unexpectedly closed. #[error("Channel closed unexpectedly")] ChannelClosed, + /// An error occurred while selecting the next L1 origin. + #[error(transparent)] + L1OriginSelector(#[from] L1OriginSelectorError), + /// An error occurred while attempting to seal a payload. + #[error(transparent)] + SealError(#[from] SealTaskError), } impl SequencerActor { @@ -259,19 +266,19 @@ impl SequencerActorState { let UnsealedPayloadHandle { payload_id, attributes_with_parent } = unsealed_payload_handle; // Create a new channel to receive the built payload. - let (payload_tx, payload_rx) = mpsc::channel(1); + let (seal_result_tx, seal_result_rx) = mpsc::channel(1); // Send the seal request to the engine to seal the unsealed block. let _seal_request_start = Instant::now(); if let Err(err) = - ctx.seal_request_tx.send((payload_id, attributes_with_parent, payload_tx)).await + ctx.seal_request_tx.send((payload_id, attributes_with_parent, seal_result_tx)).await { error!(target: "sequencer", ?err, "Failed to send seal request to engine, payload id {},", payload_id); ctx.cancellation.cancel(); return Err(SequencerActorError::ChannelClosed); } - let payload = self.try_wait_for_payload(ctx, payload_rx).await?; + let payload = self.try_wait_for_payload(ctx, seal_result_rx).await?; // Log the block building seal task duration, if metrics are enabled. kona_macros::set!( @@ -486,13 +493,17 @@ impl SequencerActorState { async fn try_wait_for_payload( &mut self, ctx: &mut SequencerContext, - mut payload_rx: mpsc::Receiver, + mut payload_rx: mpsc::Receiver>, ) -> Result { - payload_rx.recv().await.ok_or_else(|| { - error!(target: "sequencer", "Failed to receive built payload"); - ctx.cancellation.cancel(); - SequencerActorError::ChannelClosed - }) + match payload_rx.recv().await { + Some(Ok(x)) => Ok(x), + Some(Err(x)) => Err(SequencerActorError::SealError(x)), + None => { + error!(target: "sequencer", "Failed to receive built payload"); + ctx.cancellation.cancel(); + Err(SequencerActorError::ChannelClosed) + } + } } /// Waits for the next payload to be built and returns it, if there is a payload receiver @@ -620,10 +631,33 @@ impl NodeActor for SequencerActor { let start_time = Instant::now(); - next_payload_to_seal = state.seal_last_and_start_next(&mut ctx, &mut self.unsafe_head_rx, state.is_recovery_mode, next_payload_to_seal).await?; - - // Set the next tick time to the configured blocktime less the time it takes to seal last and start next. - state.build_ticker.reset_after(Duration::from_secs(state.cfg.block_time).saturating_sub(start_time.elapsed())); + match state.seal_last_and_start_next(&mut ctx, &mut self.unsafe_head_rx, state.is_recovery_mode, next_payload_to_seal).await { + Ok(res) => { + next_payload_to_seal = res; + // Set the next tick time to the configured blocktime less the time it takes to seal last and start next. + state.build_ticker.reset_after(Duration::from_secs(state.cfg.block_time).saturating_sub(start_time.elapsed())); + }, + Err(SequencerActorError::SealError(seal_err)) => { + match seal_err.severity(){ + EngineTaskErrorSeverity::Flush => { + next_payload_to_seal = None; + state.build_ticker.reset_after(Duration::from_secs(state.cfg.block_time).saturating_sub(start_time.elapsed())) + } + EngineTaskErrorSeverity::Reset => { + next_payload_to_seal = None; + state.build_ticker.reset_immediately(); + } + _ => { + error!(target: "sequencer", err = ?seal_err, "Unrecoverable seal error"); + return Err(SequencerActorError::SealError(seal_err)); + } + } + }, + Err(other_err) => { + error!(target: "sequencer", err = ?other_err, "Unexpected error sealing payload"); + return Err(other_err); + } + } } } } diff --git a/crates/node/service/src/service/core.rs b/crates/node/service/src/service/core.rs index eb6d51f1c5..79ba800f69 100644 --- a/crates/node/service/src/service/core.rs +++ b/crates/node/service/src/service/core.rs @@ -178,7 +178,7 @@ pub trait RollupNodeService { "`build_request_tx` not set while in sequencer mode. This should never happen.", ), seal_request_tx: seal_request_tx.expect( - "`build_request_tx` not set while in sequencer mode. This should never happen.", + "`seal_request_tx` not set while in sequencer mode. This should never happen.", ), gossip_payload_tx, cancellation: cancellation.clone(), From e6b356732691373c338b5cf0646d8bf52b055a3c Mon Sep 17 00:00:00 2001 From: op-will <232669456+op-will@users.noreply.github.com> Date: Thu, 6 Nov 2025 15:23:54 -0600 Subject: [PATCH 05/19] Also handling temporary errors in sequencer --- .../service/src/actors/sequencer/actor.rs | 30 ++++++++++++------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/crates/node/service/src/actors/sequencer/actor.rs b/crates/node/service/src/actors/sequencer/actor.rs index d50c91b5f0..ef19df3260 100644 --- a/crates/node/service/src/actors/sequencer/actor.rs +++ b/crates/node/service/src/actors/sequencer/actor.rs @@ -244,7 +244,7 @@ impl SequencerActorState { ctx: &mut SequencerContext, unsafe_head_rx: &mut watch::Receiver, in_recovery_mode: bool, - payload_to_seal: Option, + payload_to_seal: Option<&UnsealedPayloadHandle>, ) -> Result, SequencerActorError> { if let Some(to_seal) = payload_to_seal { self.seal_and_commit_payload_if_applicable(ctx, to_seal).await?; @@ -261,19 +261,23 @@ impl SequencerActorState { async fn seal_and_commit_payload_if_applicable( &mut self, ctx: &mut SequencerContext, - unsealed_payload_handle: UnsealedPayloadHandle, + unsealed_payload_handle: &UnsealedPayloadHandle, ) -> Result<(), SequencerActorError> { - let UnsealedPayloadHandle { payload_id, attributes_with_parent } = unsealed_payload_handle; - // Create a new channel to receive the built payload. let (seal_result_tx, seal_result_rx) = mpsc::channel(1); // Send the seal request to the engine to seal the unsealed block. let _seal_request_start = Instant::now(); - if let Err(err) = - ctx.seal_request_tx.send((payload_id, attributes_with_parent, seal_result_tx)).await + if let Err(err) = ctx + .seal_request_tx + .send(( + unsealed_payload_handle.payload_id, + unsealed_payload_handle.attributes_with_parent.clone(), + seal_result_tx, + )) + .await { - error!(target: "sequencer", ?err, "Failed to send seal request to engine, payload id {},", payload_id); + error!(target: "sequencer", ?err, "Failed to send seal request to engine, payload id {},", unsealed_payload_handle.payload_id); ctx.cancellation.cancel(); return Err(SequencerActorError::ChannelClosed); } @@ -631,7 +635,7 @@ impl NodeActor for SequencerActor { let start_time = Instant::now(); - match state.seal_last_and_start_next(&mut ctx, &mut self.unsafe_head_rx, state.is_recovery_mode, next_payload_to_seal).await { + match state.seal_last_and_start_next(&mut ctx, &mut self.unsafe_head_rx, state.is_recovery_mode, next_payload_to_seal.as_ref()).await { Ok(res) => { next_payload_to_seal = res; // Set the next tick time to the configured blocktime less the time it takes to seal last and start next. @@ -641,14 +645,20 @@ impl NodeActor for SequencerActor { match seal_err.severity(){ EngineTaskErrorSeverity::Flush => { next_payload_to_seal = None; + // this means that a block was inserted, so wait blocktime to build. state.build_ticker.reset_after(Duration::from_secs(state.cfg.block_time).saturating_sub(start_time.elapsed())) } + EngineTaskErrorSeverity::Temporary => { + // this means that the task can be retried immediately + state.build_ticker.reset_immediately(); + } EngineTaskErrorSeverity::Reset => { next_payload_to_seal = None; + // this means that a block was not inserted, so retry building immediately. state.build_ticker.reset_immediately(); } - _ => { - error!(target: "sequencer", err = ?seal_err, "Unrecoverable seal error"); + EngineTaskErrorSeverity::Critical => { + error!(target: "sequencer", err = ?seal_err, "Critical seal error"); return Err(SequencerActorError::SealError(seal_err)); } } From e09906e266fc4d9674547dceb14dd84543acdc8d Mon Sep 17 00:00:00 2001 From: op-will <232669456+op-will@users.noreply.github.com> Date: Fri, 7 Nov 2025 18:23:07 -0600 Subject: [PATCH 06/19] Reworking Engine seal interface to detect and relay errors Specifically, this catches the error in which a Build was performed on a previous block because it had an out of date view of the chain state --- crates/node/engine/src/lib.rs | 4 +- .../node/engine/src/task_queue/tasks/mod.rs | 2 +- .../engine/src/task_queue/tasks/seal/error.rs | 27 +++++++++-- .../engine/src/task_queue/tasks/seal/mod.rs | 2 +- .../engine/src/task_queue/tasks/seal/task.rs | 46 ++++++++++++++++--- .../node/service/src/actors/engine/actor.rs | 6 +-- .../service/src/actors/sequencer/actor.rs | 42 +++++++---------- 7 files changed, 87 insertions(+), 42 deletions(-) diff --git a/crates/node/engine/src/lib.rs b/crates/node/engine/src/lib.rs index 291f45d578..ba8ab2a423 100644 --- a/crates/node/engine/src/lib.rs +++ b/crates/node/engine/src/lib.rs @@ -42,8 +42,8 @@ mod task_queue; pub use task_queue::{ BuildTask, BuildTaskError, ConsolidateTask, ConsolidateTaskError, Engine, EngineBuildError, EngineResetError, EngineTask, EngineTaskError, EngineTaskErrorSeverity, EngineTaskErrors, - EngineTaskExt, FinalizeTask, FinalizeTaskError, InsertTask, InsertTaskError, SealTask, - SealTaskError, SynchronizeTask, SynchronizeTaskError, + EngineTaskExt, FinalizeTask, FinalizeTaskError, InsertTask, InsertTaskError, SealError, + SealTask, SealTaskError, SynchronizeTask, SynchronizeTaskError, }; mod attributes; diff --git a/crates/node/engine/src/task_queue/tasks/mod.rs b/crates/node/engine/src/task_queue/tasks/mod.rs index d24d75fdf3..73608cd701 100644 --- a/crates/node/engine/src/task_queue/tasks/mod.rs +++ b/crates/node/engine/src/task_queue/tasks/mod.rs @@ -15,7 +15,7 @@ mod build; pub use build::{BuildTask, BuildTaskError, EngineBuildError}; mod seal; -pub use seal::{SealTask, SealTaskError}; +pub use seal::{SealError, SealTask, SealTaskError}; mod consolidate; pub use consolidate::{ConsolidateTask, ConsolidateTaskError}; diff --git a/crates/node/engine/src/task_queue/tasks/seal/error.rs b/crates/node/engine/src/task_queue/tasks/seal/error.rs index d740fd79d3..9b9b56da32 100644 --- a/crates/node/engine/src/task_queue/tasks/seal/error.rs +++ b/crates/node/engine/src/task_queue/tasks/seal/error.rs @@ -34,12 +34,18 @@ pub enum SealTaskError { FromBlock(#[from] FromBlockError), /// Error sending the built payload envelope. #[error(transparent)] - MpscSend( - #[from] Box>>, - ), + MpscSend(#[from] Box>>), /// The clock went backwards. #[error("The clock went backwards")] ClockWentBackwards, + /// Unsafe head changed between build and seal. This likely means that there was some race + /// condition between the previous seal updating the unsafe head and the build attributes + /// being created. This build has been invalidated. + /// + /// If not propagated to the original caller for handling (i.e. there was no original caller), + /// this should not happen and is a critical error. + #[error("Unsafe head changed between build and seal")] + UnsafeHeadChangedSinceBuild, } impl EngineTaskError for SealTaskError { @@ -53,6 +59,21 @@ impl EngineTaskError for SealTaskError { Self::FromBlock(_) => EngineTaskErrorSeverity::Critical, Self::MpscSend(_) => EngineTaskErrorSeverity::Critical, Self::ClockWentBackwards => EngineTaskErrorSeverity::Critical, + Self::UnsafeHeadChangedSinceBuild => EngineTaskErrorSeverity::Critical, } } } + +/// The inter-actor error returned to the initial requestor of a seal. +#[derive(Debug, Error)] +pub enum SealError { + /// Holocene retry occurred. A deposit-only block was built and sealed. + #[error("Holocene retry occurred. A deposit-only block was built and sealed.")] + HoloceneRetry, + /// Some state mismatch invalidated this seal. Consider rebuilding. + #[error("Some state mismatch invalidated this seal. Consider rebuilding.")] + ConsiderRebuild, + /// A critical engine error occurred. + #[error("A critical engine error occurred.")] + EngineError, +} diff --git a/crates/node/engine/src/task_queue/tasks/seal/mod.rs b/crates/node/engine/src/task_queue/tasks/seal/mod.rs index 45d94e0013..88282b778e 100644 --- a/crates/node/engine/src/task_queue/tasks/seal/mod.rs +++ b/crates/node/engine/src/task_queue/tasks/seal/mod.rs @@ -4,4 +4,4 @@ mod task; pub use task::SealTask; mod error; -pub use error::SealTaskError; +pub use error::{SealError, SealTaskError}; diff --git a/crates/node/engine/src/task_queue/tasks/seal/task.rs b/crates/node/engine/src/task_queue/tasks/seal/task.rs index a89fe08079..10bce4da63 100644 --- a/crates/node/engine/src/task_queue/tasks/seal/task.rs +++ b/crates/node/engine/src/task_queue/tasks/seal/task.rs @@ -1,9 +1,10 @@ //! A task for importing a block that has already been started. use super::SealTaskError; use crate::{ - EngineClient, EngineGetPayloadVersion, EngineState, EngineTaskExt, InsertTask, + EngineClient, EngineGetPayloadVersion, EngineState, EngineTaskError, EngineTaskErrorSeverity, + EngineTaskExt, InsertTask, InsertTaskError::{self}, - task_queue::build_and_seal, + task_queue::{build_and_seal, tasks::seal::error::SealError}, }; use alloy_rpc_types_engine::{ExecutionPayload, PayloadId}; use async_trait::async_trait; @@ -43,7 +44,7 @@ pub struct SealTask { /// An optional sender to convey success/failure result of the built /// [`OpExecutionPayloadEnvelope`] after the block has been built, imported, and canonicalized /// or the [`SealTaskError`] that occurred during processing. - pub result_tx: Option>>, + pub result_tx: Option>>, } impl SealTask { @@ -239,13 +240,44 @@ impl EngineTaskExt for SealTask { "Starting new seal job" ); - // Seal the block and import it into the engine. - let res = self.seal_and_canonicalize_block(state).await; + let unsafe_block_info = state.sync_state.unsafe_head().block_info; + let parent_block_info = self.attributes.parent.block_info; - // NB: If a response channel is provided, that channel will receive success/failure info, + let res = { + if unsafe_block_info.hash != parent_block_info.hash || + unsafe_block_info.number != parent_block_info.number + { + info!( + target: "engine", + unsafe_block_info = ?unsafe_block_info, + parent_block_info = ?parent_block_info, + "Seal attributes parent does not match unsafe head, returning rebuild error" + ); + Err(SealTaskError::UnsafeHeadChangedSinceBuild) + } else { + // Seal the block and import it into the engine. + self.seal_and_canonicalize_block(state).await + } + }; + + // NB: If a response channel was provided, that channel will receive success/failure info, // and this task will always succeed. If not, task failure will be relayed to the caller. if let Some(tx) = &self.result_tx { - tx.send(res).await.map_err(Box::new).map_err(SealTaskError::MpscSend)?; + let to_send = match res { + Ok(x) => Ok(x), + // NB: This error is critical IFF not relayed back to a caller, so we can't rely on + // the severity below. + Err(SealTaskError::UnsafeHeadChangedSinceBuild) => Err(SealError::ConsiderRebuild), + Err(err) => match err.severity() { + EngineTaskErrorSeverity::Temporary | EngineTaskErrorSeverity::Reset => { + Err(SealError::ConsiderRebuild) + } + EngineTaskErrorSeverity::Flush => Err(SealError::HoloceneRetry), + EngineTaskErrorSeverity::Critical => Err(SealError::EngineError), + }, + }; + + tx.send(to_send).await.map_err(Box::new).map_err(SealTaskError::MpscSend)?; } else if let Err(x) = res { return Err(x) } diff --git a/crates/node/service/src/actors/engine/actor.rs b/crates/node/service/src/actors/engine/actor.rs index ed213ff6d1..bd3d11e172 100644 --- a/crates/node/service/src/actors/engine/actor.rs +++ b/crates/node/service/src/actors/engine/actor.rs @@ -8,7 +8,7 @@ use kona_derive::{ResetSignal, Signal}; use kona_engine::{ BuildTask, ConsolidateTask, Engine, EngineClient, EngineQueries, EngineState as InnerEngineState, EngineTask, EngineTaskError, EngineTaskErrorSeverity, - InsertTask, SealTask, SealTaskError, + InsertTask, SealError, SealTask, }; use kona_genesis::RollupConfig; use kona_protocol::{BlockInfo, L2BlockInfo, OpAttributesWithParent}; @@ -54,7 +54,7 @@ pub struct EngineActor { mpsc::Receiver<( PayloadId, OpAttributesWithParent, - mpsc::Sender>, + mpsc::Sender>, )>, >, /// The [`L2Finalizer`], used to finalize L2 blocks. @@ -81,7 +81,7 @@ pub struct EngineInboundData { mpsc::Sender<( PayloadId, OpAttributesWithParent, - mpsc::Sender>, + mpsc::Sender>, )>, >, /// A channel to send [`OpAttributesWithParent`] to the engine actor. diff --git a/crates/node/service/src/actors/sequencer/actor.rs b/crates/node/service/src/actors/sequencer/actor.rs index ef19df3260..5d7b0ef449 100644 --- a/crates/node/service/src/actors/sequencer/actor.rs +++ b/crates/node/service/src/actors/sequencer/actor.rs @@ -9,7 +9,7 @@ use alloy_rpc_types_engine::PayloadId; use async_trait::async_trait; use derive_more::Constructor; use kona_derive::{AttributesBuilder, PipelineErrorKind, StatefulAttributesBuilder}; -use kona_engine::{EngineTaskError, EngineTaskErrorSeverity, SealTaskError}; +use kona_engine::SealError; use kona_genesis::{L1ChainConfig, RollupConfig}; use kona_protocol::{BlockInfo, L2BlockInfo, OpAttributesWithParent}; use kona_providers_alloy::{AlloyChainProvider, AlloyL2ChainProvider}; @@ -192,7 +192,7 @@ pub struct SequencerContext { pub seal_request_tx: mpsc::Sender<( PayloadId, OpAttributesWithParent, - mpsc::Sender>, + mpsc::Sender>, )>, /// A sender to asynchronously sign and gossip built [`OpExecutionPayloadEnvelope`]s to the /// network actor. @@ -219,7 +219,7 @@ pub enum SequencerActorError { L1OriginSelector(#[from] L1OriginSelectorError), /// An error occurred while attempting to seal a payload. #[error(transparent)] - SealError(#[from] SealTaskError), + SealError(#[from] SealError), } impl SequencerActor { @@ -497,7 +497,7 @@ impl SequencerActorState { async fn try_wait_for_payload( &mut self, ctx: &mut SequencerContext, - mut payload_rx: mpsc::Receiver>, + mut payload_rx: mpsc::Receiver>, ) -> Result { match payload_rx.recv().await { Some(Ok(x)) => Ok(x), @@ -641,28 +641,20 @@ impl NodeActor for SequencerActor { // Set the next tick time to the configured blocktime less the time it takes to seal last and start next. state.build_ticker.reset_after(Duration::from_secs(state.cfg.block_time).saturating_sub(start_time.elapsed())); }, - Err(SequencerActorError::SealError(seal_err)) => { - match seal_err.severity(){ - EngineTaskErrorSeverity::Flush => { - next_payload_to_seal = None; - // this means that a block was inserted, so wait blocktime to build. - state.build_ticker.reset_after(Duration::from_secs(state.cfg.block_time).saturating_sub(start_time.elapsed())) - } - EngineTaskErrorSeverity::Temporary => { - // this means that the task can be retried immediately - state.build_ticker.reset_immediately(); - } - EngineTaskErrorSeverity::Reset => { - next_payload_to_seal = None; - // this means that a block was not inserted, so retry building immediately. - state.build_ticker.reset_immediately(); - } - EngineTaskErrorSeverity::Critical => { - error!(target: "sequencer", err = ?seal_err, "Critical seal error"); - return Err(SequencerActorError::SealError(seal_err)); - } - } + Err(SequencerActorError::SealError(SealError::HoloceneRetry)) => { + next_payload_to_seal = None; + // this means that a block was inserted, so wait blocktime to build. + state.build_ticker.reset_after(Duration::from_secs(state.cfg.block_time).saturating_sub(start_time.elapsed())) }, + Err(SequencerActorError::SealError(SealError::ConsiderRebuild)) => { + next_payload_to_seal = None; + // this means that a block was not inserted, so retry building immediately. + state.build_ticker.reset_immediately(); + }, + Err(SequencerActorError::SealError(SealError::EngineError)) => { + error!(target: "sequencer", "Critical engine error occurred"); + return Err(SequencerActorError::SealError(SealError::EngineError)); + } Err(other_err) => { error!(target: "sequencer", err = ?other_err, "Unexpected error sealing payload"); return Err(other_err); From 9eb566ea8be2560efa17789956d0b114f3d6fe8f Mon Sep 17 00:00:00 2001 From: op-will <232669456+op-will@users.noreply.github.com> Date: Sat, 8 Nov 2025 11:53:24 -0600 Subject: [PATCH 07/19] Fix lint --- .../node/service/src/actors/engine/actor.rs | 32 +++++++++---------- crates/node/service/src/actors/engine/mod.rs | 1 + .../service/src/actors/sequencer/actor.rs | 16 ++++++---- 3 files changed, 26 insertions(+), 23 deletions(-) diff --git a/crates/node/service/src/actors/engine/actor.rs b/crates/node/service/src/actors/engine/actor.rs index bd3d11e172..332fb7cab4 100644 --- a/crates/node/service/src/actors/engine/actor.rs +++ b/crates/node/service/src/actors/engine/actor.rs @@ -23,6 +23,18 @@ use url::Url; use crate::{NodeActor, NodeMode, actors::CancellableContext}; +/// A request to build a payload. +/// Contains the attributes to build and a channel to send back the resulting `PayloadId`. +pub(crate) type BuildRequest = (OpAttributesWithParent, mpsc::Sender); + +/// A request to seal a payload. +/// Contains the `PayloadId`, attributes, and a channel to send back the result. +pub(crate) type SealRequest = ( + PayloadId, + OpAttributesWithParent, + mpsc::Sender>, +); + /// The [`EngineActor`] is responsible for managing the operations sent to the execution layer's /// Engine API. To accomplish this, it uses the [`Engine`] task queue to order Engine API /// interactions based off of the [`Ord`] implementation of [`EngineTask`]. @@ -44,19 +56,13 @@ pub struct EngineActor { /// ## Note /// This is `Some` when the node is in sequencer mode, and `None` when the node is in validator /// mode. - build_request_rx: Option)>>, + build_request_rx: Option>, /// A channel to receive seal requests. /// The success/fail result of the sealing operation will be sent via the provided sender. /// ## Note /// This is `Some` when the node is in sequencer mode, and `None` when the node is in validator /// mode. - seal_request_rx: Option< - mpsc::Receiver<( - PayloadId, - OpAttributesWithParent, - mpsc::Sender>, - )>, - >, + seal_request_rx: Option>, /// The [`L2Finalizer`], used to finalize L2 blocks. finalizer: L2Finalizer, } @@ -70,20 +76,14 @@ pub struct EngineInboundData { /// ## Note /// This is `Some` when the node is in sequencer mode, and `None` when the node is in validator /// mode. - pub build_request_tx: Option)>>, + pub build_request_tx: Option>, /// A channel to send seal requests to the [`EngineActor`]. /// If provided, the success/fail result of the sealing operation will be sent via the provided /// sender. /// ## Note /// This is `Some` when the node is in sequencer mode, and `None` when the node is in validator /// mode. - pub seal_request_tx: Option< - mpsc::Sender<( - PayloadId, - OpAttributesWithParent, - mpsc::Sender>, - )>, - >, + pub seal_request_tx: Option>, /// A channel to send [`OpAttributesWithParent`] to the engine actor. pub attributes_tx: mpsc::Sender, /// A channel to send [`OpExecutionPayloadEnvelope`] to the engine actor. diff --git a/crates/node/service/src/actors/engine/mod.rs b/crates/node/service/src/actors/engine/mod.rs index 42a13ec040..5a587b4e77 100644 --- a/crates/node/service/src/actors/engine/mod.rs +++ b/crates/node/service/src/actors/engine/mod.rs @@ -1,6 +1,7 @@ //! The [`EngineActor`] and its components. mod actor; +pub(crate) use actor::{BuildRequest, SealRequest}; pub use actor::{EngineActor, EngineBuilder, EngineContext, EngineInboundData}; mod error; diff --git a/crates/node/service/src/actors/sequencer/actor.rs b/crates/node/service/src/actors/sequencer/actor.rs index 5d7b0ef449..47ff6da41b 100644 --- a/crates/node/service/src/actors/sequencer/actor.rs +++ b/crates/node/service/src/actors/sequencer/actor.rs @@ -3,7 +3,13 @@ use super::{ DelayedL1OriginSelectorProvider, L1OriginSelector, L1OriginSelectorError, SequencerConfig, }; -use crate::{CancellableContext, NodeActor, actors::sequencer::conductor::ConductorClient}; +use crate::{ + CancellableContext, NodeActor, + actors::{ + engine::{BuildRequest, SealRequest}, + sequencer::conductor::ConductorClient, + }, +}; use alloy_provider::RootProvider; use alloy_rpc_types_engine::PayloadId; use async_trait::async_trait; @@ -186,14 +192,10 @@ pub struct SequencerContext { pub reset_request_tx: mpsc::Sender<()>, /// Sender to request the execution layer to start building a payload on top of the /// current unsafe head. - pub build_request_tx: mpsc::Sender<(OpAttributesWithParent, mpsc::Sender)>, + pub build_request_tx: mpsc::Sender, /// Sender to request the execution layer to seal and commit the payload ID and /// attributes that resulted from a previous build call. - pub seal_request_tx: mpsc::Sender<( - PayloadId, - OpAttributesWithParent, - mpsc::Sender>, - )>, + pub seal_request_tx: mpsc::Sender, /// A sender to asynchronously sign and gossip built [`OpExecutionPayloadEnvelope`]s to the /// network actor. pub gossip_payload_tx: mpsc::Sender, From c54213785507e093e4abbb62ae7514ecf3390923 Mon Sep 17 00:00:00 2001 From: op-will <232669456+op-will@users.noreply.github.com> Date: Tue, 11 Nov 2025 10:26:21 -0600 Subject: [PATCH 08/19] Fix duplicate documentation. --- .../engine/src/task_queue/tasks/build/task.rs | 23 +++---------------- 1 file changed, 3 insertions(+), 20 deletions(-) diff --git a/crates/node/engine/src/task_queue/tasks/build/task.rs b/crates/node/engine/src/task_queue/tasks/build/task.rs index dd3594ff62..6a69bd5e0f 100644 --- a/crates/node/engine/src/task_queue/tasks/build/task.rs +++ b/crates/node/engine/src/task_queue/tasks/build/task.rs @@ -42,19 +42,6 @@ impl BuildTask { /// Validates the provided [PayloadStatusEnum] according to the rules listed below. /// /// ## Observed [PayloadStatusEnum] Variants - /// The `engine_forkchoiceUpdate` payload statuses that this function observes are below. Any - /// other [PayloadStatusEnum] variant is considered a failure. - /// - /// ### Success (`VALID`) - /// If the build is successful, the [PayloadId] is returned for sealing and the external - /// actor is notified of the successful forkchoice update. - /// - /// ### Failure (`INVALID`) - /// If the forkchoice update fails, the external actor is notified of the failure. - /// - /// Validates the payload status from a forkchoice update response. - /// - /// ## Observed [PayloadStatusEnum] Variants /// - `VALID`: Returns Ok(()) - forkchoice update was successful /// - `INVALID`: Returns error with validation details /// - `SYNCING`: Returns temporary error - EL is syncing @@ -82,16 +69,12 @@ impl BuildTask { /// Starts the block building process by sending an initial `engine_forkchoiceUpdate` call with /// the payload attributes to build. /// - /// ## Observed [PayloadStatusEnum] Variants - /// The `engine_forkchoiceUpdate` payload statuses that this function observes are below. Any - /// other [PayloadStatusEnum] variant is considered a failure. - /// /// ### Success (`VALID`) - /// If the build is successful, the [PayloadId] is returned for sealing and the external - /// actor is notified of the successful forkchoice update. + /// If the build is successful, the [PayloadId] is returned for sealing and the successful + /// forkchoice update identifier is relayed via the stored `payload_id_tx` sender. /// /// ### Failure (`INVALID`) - /// If the forkchoice update fails, the external actor is notified of the failure. + /// If the forkchoice update fails, the [BuildTaskError]. /// /// ### Syncing (`SYNCING`) /// If the EL is syncing, the payload attributes are buffered and the function returns early. From d6b99c578a9fc2131a34ae8a80877afc255edf08 Mon Sep 17 00:00:00 2001 From: op-will <232669456+op-will@users.noreply.github.com> Date: Tue, 11 Nov 2025 10:28:02 -0600 Subject: [PATCH 09/19] Adding explicit match in BuildTask Wildcard match only had one possibility, so this is good future-proofing --- crates/node/engine/src/task_queue/tasks/build/task.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/node/engine/src/task_queue/tasks/build/task.rs b/crates/node/engine/src/task_queue/tasks/build/task.rs index 6a69bd5e0f..9507164098 100644 --- a/crates/node/engine/src/task_queue/tasks/build/task.rs +++ b/crates/node/engine/src/task_queue/tasks/build/task.rs @@ -59,9 +59,11 @@ impl BuildTask { warn!(target: "engine_builder", "Forkchoice update failed temporarily: EL is syncing"); Err(BuildTaskError::EngineBuildError(EngineBuildError::EngineSyncing)) } - s => { + PayloadStatusEnum::Accepted => { // Other codes are never returned by `engine_forkchoiceUpdate` - Err(BuildTaskError::EngineBuildError(EngineBuildError::UnexpectedPayloadStatus(s))) + Err(BuildTaskError::EngineBuildError(EngineBuildError::UnexpectedPayloadStatus( + status, + ))) } } } From 02f7c61310f2ce89dbed69d803968b7476f2b605 Mon Sep 17 00:00:00 2001 From: op-will <232669456+op-will@users.noreply.github.com> Date: Tue, 11 Nov 2025 10:29:34 -0600 Subject: [PATCH 10/19] Fix map_err nit in build/task.rs --- crates/node/engine/src/task_queue/tasks/build/task.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/node/engine/src/task_queue/tasks/build/task.rs b/crates/node/engine/src/task_queue/tasks/build/task.rs index 9507164098..8474c7488e 100644 --- a/crates/node/engine/src/task_queue/tasks/build/task.rs +++ b/crates/node/engine/src/task_queue/tasks/build/task.rs @@ -175,7 +175,7 @@ impl EngineTaskExt for BuildTask { // If a channel was provided, send the payload ID to it. if let Some(tx) = &self.payload_id_tx { - tx.send(payload_id).await.map_err(Box::new).map_err(BuildTaskError::MpscSend)?; + tx.send(payload_id).await.map_err(Box::new)?; } Ok(payload_id) From f131428d21ef6774aadd338165835bdfab9d6bc2 Mon Sep 17 00:00:00 2001 From: op-will <232669456+op-will@users.noreply.github.com> Date: Tue, 11 Nov 2025 10:34:54 -0600 Subject: [PATCH 11/19] Propagate error nit in consolidate/task.rs --- crates/node/engine/src/task_queue/tasks/consolidate/task.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/node/engine/src/task_queue/tasks/consolidate/task.rs b/crates/node/engine/src/task_queue/tasks/consolidate/task.rs index 8722d59d58..59a4b0179c 100644 --- a/crates/node/engine/src/task_queue/tasks/consolidate/task.rs +++ b/crates/node/engine/src/task_queue/tasks/consolidate/task.rs @@ -48,8 +48,9 @@ impl ConsolidateTask { self.attributes.clone(), self.is_attributes_derived, ) - .await - .map_err(Into::into) + .await?; + + Ok(()) } /// Attempts consolidation on the engine state. From 48ff01bf4ec110faf58e88ccdba619666728cf96 Mon Sep 17 00:00:00 2001 From: op-will <232669456+op-will@users.noreply.github.com> Date: Tue, 11 Nov 2025 10:49:12 -0600 Subject: [PATCH 12/19] Adding ConsiderReseal error to SealError enum Otherwise block production might take 2x block_time. Consider: 1. Build 2. Seal block_time later 3. Err => ConsiderRebuild 4. Build 5. Seal block_time later Result: 1 block in 2*block_time --- crates/node/engine/src/task_queue/tasks/seal/error.rs | 3 +++ crates/node/engine/src/task_queue/tasks/seal/task.rs | 5 ++--- crates/node/service/src/actors/sequencer/actor.rs | 5 +++++ 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/crates/node/engine/src/task_queue/tasks/seal/error.rs b/crates/node/engine/src/task_queue/tasks/seal/error.rs index 9b9b56da32..cf9787c4df 100644 --- a/crates/node/engine/src/task_queue/tasks/seal/error.rs +++ b/crates/node/engine/src/task_queue/tasks/seal/error.rs @@ -73,6 +73,9 @@ pub enum SealError { /// Some state mismatch invalidated this seal. Consider rebuilding. #[error("Some state mismatch invalidated this seal. Consider rebuilding.")] ConsiderRebuild, + /// A temporary error occurred when sealing. Consider retrying the seal operation. + #[error("A temporary error occurred when sealing. Consider retrying the seal operation.")] + ConsiderReseal, /// A critical engine error occurred. #[error("A critical engine error occurred.")] EngineError, diff --git a/crates/node/engine/src/task_queue/tasks/seal/task.rs b/crates/node/engine/src/task_queue/tasks/seal/task.rs index 10bce4da63..ce64f64e30 100644 --- a/crates/node/engine/src/task_queue/tasks/seal/task.rs +++ b/crates/node/engine/src/task_queue/tasks/seal/task.rs @@ -269,9 +269,8 @@ impl EngineTaskExt for SealTask { // the severity below. Err(SealTaskError::UnsafeHeadChangedSinceBuild) => Err(SealError::ConsiderRebuild), Err(err) => match err.severity() { - EngineTaskErrorSeverity::Temporary | EngineTaskErrorSeverity::Reset => { - Err(SealError::ConsiderRebuild) - } + EngineTaskErrorSeverity::Temporary => Err(SealError::ConsiderReseal), + EngineTaskErrorSeverity::Reset => Err(SealError::ConsiderRebuild), EngineTaskErrorSeverity::Flush => Err(SealError::HoloceneRetry), EngineTaskErrorSeverity::Critical => Err(SealError::EngineError), }, diff --git a/crates/node/service/src/actors/sequencer/actor.rs b/crates/node/service/src/actors/sequencer/actor.rs index 47ff6da41b..6717bc23f7 100644 --- a/crates/node/service/src/actors/sequencer/actor.rs +++ b/crates/node/service/src/actors/sequencer/actor.rs @@ -648,6 +648,11 @@ impl NodeActor for SequencerActor { // this means that a block was inserted, so wait blocktime to build. state.build_ticker.reset_after(Duration::from_secs(state.cfg.block_time).saturating_sub(start_time.elapsed())) }, + Err(SequencerActorError::SealError(SealError::ConsiderReseal)) => { + // Note: we don't reset next_payload_to_seal, so we'll retry the seal. + // this means that a block was not inserted, so retry sealing immediately. + state.build_ticker.reset_immediately(); + }, Err(SequencerActorError::SealError(SealError::ConsiderRebuild)) => { next_payload_to_seal = None; // this means that a block was not inserted, so retry building immediately. From 5f9c4f25848da6f4d608a1041a9299ff18c3b720 Mon Sep 17 00:00:00 2001 From: op-will <232669456+op-will@users.noreply.github.com> Date: Tue, 11 Nov 2025 10:55:15 -0600 Subject: [PATCH 13/19] Updating seal task logging and fn naming --- crates/node/engine/src/task_queue/tasks/seal/task.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/node/engine/src/task_queue/tasks/seal/task.rs b/crates/node/engine/src/task_queue/tasks/seal/task.rs index ce64f64e30..83916ae998 100644 --- a/crates/node/engine/src/task_queue/tasks/seal/task.rs +++ b/crates/node/engine/src/task_queue/tasks/seal/task.rs @@ -48,7 +48,7 @@ pub struct SealTask { } impl SealTask { - /// Fetches the execution payload from the EL. + /// Seals the execution payload in the EL, returning the execution envelope. /// /// ## Engine Method Selection /// The method used to fetch the payload from the EL is determined by the payload timestamp. The @@ -57,7 +57,7 @@ impl SealTask { /// - `engine_getPayloadV2` is used for payloads with a timestamp before the Ecotone fork. /// - `engine_getPayloadV3` is used for payloads with a timestamp after the Ecotone fork. /// - `engine_getPayloadV4` is used for payloads with a timestamp after the Isthmus fork. - async fn fetch_payload( + async fn seal_payload( &self, cfg: &RollupConfig, engine: &EngineClient, @@ -70,7 +70,7 @@ impl SealTask { target: "engine", payload_id = payload_id.to_string(), l2_time = payload_timestamp, - "Inserting payload" + "Sealing payload" ); let get_payload_version = EngineGetPayloadVersion::from_cfg(cfg, payload_timestamp); @@ -198,7 +198,7 @@ impl SealTask { // Fetch the payload just inserted from the EL and import it into the engine. let block_import_start_time = Instant::now(); let new_payload = self - .fetch_payload(&self.cfg, &self.engine, self.payload_id, self.attributes.clone()) + .seal_payload(&self.cfg, &self.engine, self.payload_id, self.attributes.clone()) .await?; let new_block_ref = L2BlockInfo::from_payload_and_genesis( From c555409211143a5559e008b8baa70c1e42ee2862 Mon Sep 17 00:00:00 2001 From: op-will <232669456+op-will@users.noreply.github.com> Date: Tue, 11 Nov 2025 10:56:10 -0600 Subject: [PATCH 14/19] Braces nit in SealTask --- .../engine/src/task_queue/tasks/seal/task.rs | 28 +++++++++---------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/crates/node/engine/src/task_queue/tasks/seal/task.rs b/crates/node/engine/src/task_queue/tasks/seal/task.rs index 83916ae998..ff069c6149 100644 --- a/crates/node/engine/src/task_queue/tasks/seal/task.rs +++ b/crates/node/engine/src/task_queue/tasks/seal/task.rs @@ -243,21 +243,19 @@ impl EngineTaskExt for SealTask { let unsafe_block_info = state.sync_state.unsafe_head().block_info; let parent_block_info = self.attributes.parent.block_info; - let res = { - if unsafe_block_info.hash != parent_block_info.hash || - unsafe_block_info.number != parent_block_info.number - { - info!( - target: "engine", - unsafe_block_info = ?unsafe_block_info, - parent_block_info = ?parent_block_info, - "Seal attributes parent does not match unsafe head, returning rebuild error" - ); - Err(SealTaskError::UnsafeHeadChangedSinceBuild) - } else { - // Seal the block and import it into the engine. - self.seal_and_canonicalize_block(state).await - } + let res = if unsafe_block_info.hash != parent_block_info.hash || + unsafe_block_info.number != parent_block_info.number + { + info!( + target: "engine", + unsafe_block_info = ?unsafe_block_info, + parent_block_info = ?parent_block_info, + "Seal attributes parent does not match unsafe head, returning rebuild error" + ); + Err(SealTaskError::UnsafeHeadChangedSinceBuild) + } else { + // Seal the block and import it into the engine. + self.seal_and_canonicalize_block(state).await }; // NB: If a response channel was provided, that channel will receive success/failure info, From 0489b18a4416e5c05a7010cb109196289ccbd541 Mon Sep 17 00:00:00 2001 From: op-will <232669456+op-will@users.noreply.github.com> Date: Tue, 11 Nov 2025 11:27:40 -0600 Subject: [PATCH 15/19] Extracting method for SealTask return logic --- .../engine/src/task_queue/tasks/seal/task.rs | 56 ++++++++++++------- 1 file changed, 36 insertions(+), 20 deletions(-) diff --git a/crates/node/engine/src/task_queue/tasks/seal/task.rs b/crates/node/engine/src/task_queue/tasks/seal/task.rs index ff069c6149..5490275187 100644 --- a/crates/node/engine/src/task_queue/tasks/seal/task.rs +++ b/crates/node/engine/src/task_queue/tasks/seal/task.rs @@ -224,6 +224,41 @@ impl SealTask { Ok(new_payload) } + + /// Sends the provided result via the `result_tx` sender if one exists, returning the + /// appropriate error if it does not. + /// + /// This allows the original caller to handle errors, removing that burden from the engine, + /// which may not know the caller's intent or retry preferences. If the original caller did not + /// provide a mechanism to get notified of updates, handle the error in the default manner in + /// the task queue logic. + async fn send_channel_result_or_get_error( + &self, + res: Result, + ) -> Result<(), SealTaskError> { + // NB: If a response channel was provided, that channel will receive success/failure info, + // and this task will always succeed. If not, task failure will be relayed to the caller. + if let Some(tx) = &self.result_tx { + let to_send = match res { + Ok(x) => Ok(x), + // NB: This error is critical IFF not relayed back to a caller, so we can't rely on + // the severity below. + Err(SealTaskError::UnsafeHeadChangedSinceBuild) => Err(SealError::ConsiderRebuild), + Err(err) => match err.severity() { + EngineTaskErrorSeverity::Temporary => Err(SealError::ConsiderReseal), + EngineTaskErrorSeverity::Reset => Err(SealError::ConsiderRebuild), + EngineTaskErrorSeverity::Flush => Err(SealError::HoloceneRetry), + EngineTaskErrorSeverity::Critical => Err(SealError::EngineError), + }, + }; + + tx.send(to_send).await.map_err(Box::new).map_err(SealTaskError::MpscSend)?; + } else if let Err(x) = res { + return Err(x) + } + + Ok(()) + } } #[async_trait] @@ -258,26 +293,7 @@ impl EngineTaskExt for SealTask { self.seal_and_canonicalize_block(state).await }; - // NB: If a response channel was provided, that channel will receive success/failure info, - // and this task will always succeed. If not, task failure will be relayed to the caller. - if let Some(tx) = &self.result_tx { - let to_send = match res { - Ok(x) => Ok(x), - // NB: This error is critical IFF not relayed back to a caller, so we can't rely on - // the severity below. - Err(SealTaskError::UnsafeHeadChangedSinceBuild) => Err(SealError::ConsiderRebuild), - Err(err) => match err.severity() { - EngineTaskErrorSeverity::Temporary => Err(SealError::ConsiderReseal), - EngineTaskErrorSeverity::Reset => Err(SealError::ConsiderRebuild), - EngineTaskErrorSeverity::Flush => Err(SealError::HoloceneRetry), - EngineTaskErrorSeverity::Critical => Err(SealError::EngineError), - }, - }; - - tx.send(to_send).await.map_err(Box::new).map_err(SealTaskError::MpscSend)?; - } else if let Err(x) = res { - return Err(x) - } + self.send_channel_result_or_get_error(res).await?; Ok(()) } From bd229064a72e15285617866c462fcdee8c62833d Mon Sep 17 00:00:00 2001 From: op-will <232669456+op-will@users.noreply.github.com> Date: Tue, 11 Nov 2025 11:34:24 -0600 Subject: [PATCH 16/19] Fixing formatting in SequencerActor main loop match --- crates/node/service/src/actors/sequencer/actor.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/node/service/src/actors/sequencer/actor.rs b/crates/node/service/src/actors/sequencer/actor.rs index 6717bc23f7..eaa833bfe7 100644 --- a/crates/node/service/src/actors/sequencer/actor.rs +++ b/crates/node/service/src/actors/sequencer/actor.rs @@ -659,8 +659,8 @@ impl NodeActor for SequencerActor { state.build_ticker.reset_immediately(); }, Err(SequencerActorError::SealError(SealError::EngineError)) => { - error!(target: "sequencer", "Critical engine error occurred"); - return Err(SequencerActorError::SealError(SealError::EngineError)); + error!(target: "sequencer", "Critical engine error occurred"); + return Err(SequencerActorError::SealError(SealError::EngineError)); } Err(other_err) => { error!(target: "sequencer", err = ?other_err, "Unexpected error sealing payload"); From 28f2f5c7d691660a6f89b3d4c7f19a59a52322db Mon Sep 17 00:00:00 2001 From: op-will <232669456+op-will@users.noreply.github.com> Date: Tue, 11 Nov 2025 11:38:27 -0600 Subject: [PATCH 17/19] Handling ctx.cancellation.cancel() in one place instead of many --- .../node/service/src/actors/sequencer/actor.rs | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/crates/node/service/src/actors/sequencer/actor.rs b/crates/node/service/src/actors/sequencer/actor.rs index eaa833bfe7..23e9dc0746 100644 --- a/crates/node/service/src/actors/sequencer/actor.rs +++ b/crates/node/service/src/actors/sequencer/actor.rs @@ -280,11 +280,10 @@ impl SequencerActorState { .await { error!(target: "sequencer", ?err, "Failed to send seal request to engine, payload id {},", unsealed_payload_handle.payload_id); - ctx.cancellation.cancel(); return Err(SequencerActorError::ChannelClosed); } - let payload = self.try_wait_for_payload(ctx, seal_result_rx).await?; + let payload = self.try_wait_for_payload(seal_result_rx).await?; // Log the block building seal task duration, if metrics are enabled. kona_macros::set!( @@ -345,7 +344,6 @@ impl SequencerActorState { ); if let Err(err) = ctx.reset_request_tx.send(()).await { error!(target: "sequencer", ?err, "Failed to reset engine"); - ctx.cancellation.cancel(); return Err(SequencerActorError::ChannelClosed); } return Ok(None); @@ -386,11 +384,10 @@ impl SequencerActorState { ctx.build_request_tx.send((attrs_with_parent.clone(), payload_id_tx)).await { error!(target: "sequencer", ?err, "Failed to send built attributes to engine"); - ctx.cancellation.cancel(); return Err(SequencerActorError::ChannelClosed); } - let payload_id = self.try_wait_for_payload_id(ctx, payload_id_rx).await?; + let payload_id = self.try_wait_for_payload_id(payload_id_rx).await?; Ok(Some(UnsealedPayloadHandle::new(payload_id, attrs_with_parent))) } @@ -414,7 +411,6 @@ impl SequencerActorState { Err(PipelineErrorKind::Reset(_)) => { if let Err(err) = ctx.reset_request_tx.send(()).await { error!(target: "sequencer", ?err, "Failed to reset engine"); - ctx.cancellation.cancel(); return Err(SequencerActorError::ChannelClosed); } @@ -426,7 +422,6 @@ impl SequencerActorState { } Err(err @ PipelineErrorKind::Critical(_)) => { error!(target: "sequencer", ?err, "Failed to prepare payload attributes"); - ctx.cancellation.cancel(); return Err(err.into()); } }; @@ -498,7 +493,6 @@ impl SequencerActorState { /// present. async fn try_wait_for_payload( &mut self, - ctx: &mut SequencerContext, mut payload_rx: mpsc::Receiver>, ) -> Result { match payload_rx.recv().await { @@ -506,7 +500,6 @@ impl SequencerActorState { Some(Err(x)) => Err(SequencerActorError::SealError(x)), None => { error!(target: "sequencer", "Failed to receive built payload"); - ctx.cancellation.cancel(); Err(SequencerActorError::ChannelClosed) } } @@ -516,12 +509,10 @@ impl SequencerActorState { /// present. async fn try_wait_for_payload_id( &mut self, - ctx: &mut SequencerContext, mut payload_id_rx: mpsc::Receiver, ) -> Result { payload_id_rx.recv().await.ok_or_else(|| { error!(target: "sequencer", "Failed to receive payload for initiated block build"); - ctx.cancellation.cancel(); SequencerActorError::ChannelClosed }) } @@ -535,7 +526,6 @@ impl SequencerActorState { // Send the payload to the P2P layer to be signed and gossipped. if let Err(err) = ctx.gossip_payload_tx.send(payload).await { error!(target: "sequencer", ?err, "Failed to send payload to be signed and gossipped"); - ctx.cancellation.cancel(); return Err(SequencerActorError::ChannelClosed); } @@ -551,7 +541,6 @@ impl SequencerActorState { // Schedule a reset of the engine, in order to initialize the engine state. if let Err(err) = ctx.reset_request_tx.send(()).await { error!(target: "sequencer", ?err, "Failed to send reset request to engine"); - ctx.cancellation.cancel(); return Err(SequencerActorError::ChannelClosed); } @@ -560,7 +549,6 @@ impl SequencerActorState { // We know that the reset has concluded when the unsafe head watch channel is updated. if unsafe_head_rx.changed().await.is_err() { error!(target: "sequencer", "Failed to receive unsafe head update after reset request"); - ctx.cancellation.cancel(); return Err(SequencerActorError::ChannelClosed); } @@ -660,10 +648,12 @@ impl NodeActor for SequencerActor { }, Err(SequencerActorError::SealError(SealError::EngineError)) => { error!(target: "sequencer", "Critical engine error occurred"); + ctx.cancellation.cancel(); return Err(SequencerActorError::SealError(SealError::EngineError)); } Err(other_err) => { error!(target: "sequencer", err = ?other_err, "Unexpected error sealing payload"); + ctx.cancellation.cancel(); return Err(other_err); } } From 37925c498c347f702d3a152b7c924357dfbb78e9 Mon Sep 17 00:00:00 2001 From: op-will <232669456+op-will@users.noreply.github.com> Date: Tue, 11 Nov 2025 12:49:52 -0600 Subject: [PATCH 18/19] Reverting 8ff01bf4ec110faf58e88ccdba619666728cf96 --- crates/node/engine/src/task_queue/tasks/seal/error.rs | 3 --- crates/node/engine/src/task_queue/tasks/seal/task.rs | 5 +++-- crates/node/service/src/actors/sequencer/actor.rs | 5 ----- 3 files changed, 3 insertions(+), 10 deletions(-) diff --git a/crates/node/engine/src/task_queue/tasks/seal/error.rs b/crates/node/engine/src/task_queue/tasks/seal/error.rs index cf9787c4df..9b9b56da32 100644 --- a/crates/node/engine/src/task_queue/tasks/seal/error.rs +++ b/crates/node/engine/src/task_queue/tasks/seal/error.rs @@ -73,9 +73,6 @@ pub enum SealError { /// Some state mismatch invalidated this seal. Consider rebuilding. #[error("Some state mismatch invalidated this seal. Consider rebuilding.")] ConsiderRebuild, - /// A temporary error occurred when sealing. Consider retrying the seal operation. - #[error("A temporary error occurred when sealing. Consider retrying the seal operation.")] - ConsiderReseal, /// A critical engine error occurred. #[error("A critical engine error occurred.")] EngineError, diff --git a/crates/node/engine/src/task_queue/tasks/seal/task.rs b/crates/node/engine/src/task_queue/tasks/seal/task.rs index 5490275187..2bd41d19a1 100644 --- a/crates/node/engine/src/task_queue/tasks/seal/task.rs +++ b/crates/node/engine/src/task_queue/tasks/seal/task.rs @@ -245,8 +245,9 @@ impl SealTask { // the severity below. Err(SealTaskError::UnsafeHeadChangedSinceBuild) => Err(SealError::ConsiderRebuild), Err(err) => match err.severity() { - EngineTaskErrorSeverity::Temporary => Err(SealError::ConsiderReseal), - EngineTaskErrorSeverity::Reset => Err(SealError::ConsiderRebuild), + EngineTaskErrorSeverity::Temporary | EngineTaskErrorSeverity::Reset => { + Err(SealError::ConsiderRebuild) + } EngineTaskErrorSeverity::Flush => Err(SealError::HoloceneRetry), EngineTaskErrorSeverity::Critical => Err(SealError::EngineError), }, diff --git a/crates/node/service/src/actors/sequencer/actor.rs b/crates/node/service/src/actors/sequencer/actor.rs index 23e9dc0746..aa7bf8f2cb 100644 --- a/crates/node/service/src/actors/sequencer/actor.rs +++ b/crates/node/service/src/actors/sequencer/actor.rs @@ -636,11 +636,6 @@ impl NodeActor for SequencerActor { // this means that a block was inserted, so wait blocktime to build. state.build_ticker.reset_after(Duration::from_secs(state.cfg.block_time).saturating_sub(start_time.elapsed())) }, - Err(SequencerActorError::SealError(SealError::ConsiderReseal)) => { - // Note: we don't reset next_payload_to_seal, so we'll retry the seal. - // this means that a block was not inserted, so retry sealing immediately. - state.build_ticker.reset_immediately(); - }, Err(SequencerActorError::SealError(SealError::ConsiderRebuild)) => { next_payload_to_seal = None; // this means that a block was not inserted, so retry building immediately. From 2ac3bfaa3f2bba2ed30b6527baa4364a45a7cbc0 Mon Sep 17 00:00:00 2001 From: op-will <232669456+op-will@users.noreply.github.com> Date: Tue, 11 Nov 2025 14:35:54 -0600 Subject: [PATCH 19/19] Adjusting next tick logic to match previous BuildTask logic --- .../service/src/actors/sequencer/actor.rs | 44 ++++++++++++++----- 1 file changed, 32 insertions(+), 12 deletions(-) diff --git a/crates/node/service/src/actors/sequencer/actor.rs b/crates/node/service/src/actors/sequencer/actor.rs index aa7bf8f2cb..2a67936e3f 100644 --- a/crates/node/service/src/actors/sequencer/actor.rs +++ b/crates/node/service/src/actors/sequencer/actor.rs @@ -24,7 +24,7 @@ use op_alloy_network::Optimism; use op_alloy_rpc_types_engine::OpExecutionPayloadEnvelope; use std::{ sync::Arc, - time::{Duration, Instant}, + time::{Duration, Instant, SystemTime, UNIX_EPOCH}, }; use tokio::{ select, @@ -54,6 +54,16 @@ pub(super) struct UnsealedPayloadHandle { pub attributes_with_parent: OpAttributesWithParent, } +/// The return payload of the `seal_last_and_start_next` function. This allows the sequencer +/// to make an informed decision about when to seal and build the next block. +#[derive(Debug, Constructor)] +pub(super) struct SealLastStartNextResult { + /// The [`UnsealedPayloadHandle`] that was built. + pub unsealed_payload_handle: Option, + /// How long it took to execute the seal operation. + pub seconds_to_seal: u64, +} + /// The state of the [`SequencerActor`]. #[derive(Debug)] pub(super) struct SequencerActorState { @@ -247,15 +257,18 @@ impl SequencerActorState { unsafe_head_rx: &mut watch::Receiver, in_recovery_mode: bool, payload_to_seal: Option<&UnsealedPayloadHandle>, - ) -> Result, SequencerActorError> { + ) -> Result { + let mut seconds_to_seal = 0u64; if let Some(to_seal) = payload_to_seal { + let seal_start = Instant::now(); self.seal_and_commit_payload_if_applicable(ctx, to_seal).await?; + seconds_to_seal = seal_start.elapsed().as_secs(); } let unsealed_payload = self.build_unsealed_payload(ctx, unsafe_head_rx, in_recovery_mode).await?; - Ok(unsealed_payload) + Ok(SealLastStartNextResult::new(unsealed_payload, seconds_to_seal)) } /// Sends a seal request to seal the provided [`UnsealedPayloadHandle`], committing and @@ -589,8 +602,8 @@ impl NodeActor for SequencerActor { // Reset the engine state prior to beginning block building. state.schedule_initial_reset(&mut ctx, &mut self.unsafe_head_rx).await?; - let mut next_payload_to_seal = None; - + let mut next_payload_to_seal: Option = None; + let mut last_seconds_to_seal = 0u64; loop { select! { // We are using a biased select here to ensure that the admin queries are given priority over the block building task. @@ -623,18 +636,13 @@ impl NodeActor for SequencerActor { // The sequencer must be active to build new blocks. _ = state.build_ticker.tick(), if state.is_active => { - let start_time = Instant::now(); - match state.seal_last_and_start_next(&mut ctx, &mut self.unsafe_head_rx, state.is_recovery_mode, next_payload_to_seal.as_ref()).await { Ok(res) => { - next_payload_to_seal = res; - // Set the next tick time to the configured blocktime less the time it takes to seal last and start next. - state.build_ticker.reset_after(Duration::from_secs(state.cfg.block_time).saturating_sub(start_time.elapsed())); + next_payload_to_seal = res.unsealed_payload_handle; + last_seconds_to_seal = res.seconds_to_seal; }, Err(SequencerActorError::SealError(SealError::HoloceneRetry)) => { next_payload_to_seal = None; - // this means that a block was inserted, so wait blocktime to build. - state.build_ticker.reset_after(Duration::from_secs(state.cfg.block_time).saturating_sub(start_time.elapsed())) }, Err(SequencerActorError::SealError(SealError::ConsiderRebuild)) => { next_payload_to_seal = None; @@ -652,6 +660,18 @@ impl NodeActor for SequencerActor { return Err(other_err); } } + + if let Some(ref payload) = next_payload_to_seal { + let next_block_seconds = payload.attributes_with_parent.parent().block_info.timestamp.saturating_add(state.cfg.block_time); + // next block time is last + block_time - time it takes to seal. + let next_block_time = UNIX_EPOCH + Duration::from_secs(next_block_seconds) - Duration::from_secs(last_seconds_to_seal); + match next_block_time.duration_since(SystemTime::now()) { + Ok(duration) => state.build_ticker.reset_after(duration), + Err(_) => state.build_ticker.reset_immediately(), + }; + } else { + state.build_ticker.reset_immediately(); + } } } }