From f43d3522af5949a2411f21fb42e994abaa559686 Mon Sep 17 00:00:00 2001 From: varun-doshi Date: Wed, 26 Nov 2025 13:42:54 +0530 Subject: [PATCH 1/4] chore(node/actor): Remove .expect(...) usage --- .../node/service/src/actors/engine/actor.rs | 36 ++++++++++++++++- .../service/src/actors/sequencer/builder.rs | 40 +++++++++++++------ .../service/src/actors/sequencer/error.rs | 3 ++ crates/node/service/src/service/node.rs | 14 ++----- 4 files changed, 69 insertions(+), 24 deletions(-) diff --git a/crates/node/service/src/actors/engine/actor.rs b/crates/node/service/src/actors/engine/actor.rs index 942661f080..358e4f6568 100644 --- a/crates/node/service/src/actors/engine/actor.rs +++ b/crates/node/service/src/actors/engine/actor.rs @@ -141,6 +141,29 @@ pub struct EngineInboundData { pub unsafe_head_rx: Option>, } +/// Validates that the sequencer fields are set. +fn validate_sequencer_fields( + build_request_tx: &Option>, + seal_request_tx: &Option>, + unsafe_head_rx: &Option>, +) -> Result<(), String> { + if build_request_tx.is_none() { + return Err( + "build_request_tx is None in sequencer mode. This should never happen.".to_string() + ); + } + if seal_request_tx.is_none() { + return Err( + "seal_request_tx is None in sequencer mode. This should never happen.".to_string() + ); + } + if unsafe_head_rx.is_none() { + return Err("unsafe_head_rx is None in sequencer mode. This should never happen".to_string()); + } + + Ok(()) +} + /// Configuration for the Engine Actor. #[derive(Debug, Clone)] pub struct EngineConfig { @@ -249,7 +272,7 @@ struct SequencerChannels { impl EngineActor { /// Constructs a new [`EngineActor`] from the params. - pub fn new(config: EngineConfig) -> (EngineInboundData, Self) { + pub fn new(config: EngineConfig) -> Result<(EngineInboundData, Self), String> { let (finalized_l1_block_tx, finalized_l1_block_rx) = watch::channel(None); let (inbound_queries_tx, inbound_queries_rx) = mpsc::channel(1024); let (attributes_tx, attributes_rx) = mpsc::channel(1024); @@ -297,6 +320,15 @@ impl EngineActor { rollup_boost_health_query_rx, }; + // Validate sequencer fields if node is in sequencer mode. + if actor.builder.mode.is_sequencer() { + validate_sequencer_fields( + &sequencer_channels.build_request_tx, + &sequencer_channels.seal_request_tx, + &sequencer_channels.unsafe_head_rx, + )?; + } + let outbound_data = EngineInboundData { attributes_tx, build_request_tx: sequencer_channels.build_request_tx, @@ -310,7 +342,7 @@ impl EngineActor { unsafe_head_rx: sequencer_channels.unsafe_head_rx, }; - (outbound_data, actor) + Ok((outbound_data, actor)) } } diff --git a/crates/node/service/src/actors/sequencer/builder.rs b/crates/node/service/src/actors/sequencer/builder.rs index 58f504f364..9ad2702477 100644 --- a/crates/node/service/src/actors/sequencer/builder.rs +++ b/crates/node/service/src/actors/sequencer/builder.rs @@ -13,6 +13,8 @@ use std::sync::Arc; use tokio::sync::mpsc; use tokio_util::sync::CancellationToken; +use super::SequencerActorError; + /// Builder for constructing a [`SequencerActor`]. #[derive(Debug, Default)] pub struct SequencerActorBuilder< @@ -171,23 +173,37 @@ where OriginSelector_, UnsafePayloadGossipClient_, >, - String, + SequencerActorError, > { Ok(SequencerActor { - admin_api_rx: self.admin_api_rx.expect("admin_api_rx is required"), - attributes_builder: self.attributes_builder.expect("attributes_builder is required"), + admin_api_rx: self + .admin_api_rx + .ok_or(SequencerActorError::MissingField("admin_api_rx".to_string()))?, + attributes_builder: self + .attributes_builder + .ok_or(SequencerActorError::MissingField("attributes_builder".to_string()))?, block_building_client: self .block_building_client - .expect("block_building_client is required"), - cancellation_token: self.cancellation_token.expect("cancellation is required"), + .ok_or(SequencerActorError::MissingField("block_building_client".to_string()))?, + cancellation_token: self + .cancellation_token + .ok_or(SequencerActorError::MissingField("cancellation_token".to_string()))?, conductor: self.conductor, - is_active: self.is_active.expect("initial active status not set"), - in_recovery_mode: self.in_recovery_mode.expect("initial recovery mode status not set"), - origin_selector: self.origin_selector.expect("origin_selector is required"), - rollup_config: self.rollup_config.expect("rollup_config is required"), - unsafe_payload_gossip_client: self - .unsafe_payload_gossip_client - .expect("unsafe_payload_gossip_client is required"), + is_active: self + .is_active + .ok_or(SequencerActorError::MissingField("is_active".to_string()))?, + in_recovery_mode: self + .in_recovery_mode + .ok_or(SequencerActorError::MissingField("in_recovery_mode".to_string()))?, + origin_selector: self + .origin_selector + .ok_or(SequencerActorError::MissingField("origin_selector".to_string()))?, + rollup_config: self + .rollup_config + .ok_or(SequencerActorError::MissingField("rollup_config".to_string()))?, + unsafe_payload_gossip_client: self.unsafe_payload_gossip_client.ok_or( + SequencerActorError::MissingField("unsafe_payload_gossip_client".to_string()), + )?, }) } } diff --git a/crates/node/service/src/actors/sequencer/error.rs b/crates/node/service/src/actors/sequencer/error.rs index 70f1cbf5c4..b300c4eafb 100644 --- a/crates/node/service/src/actors/sequencer/error.rs +++ b/crates/node/service/src/actors/sequencer/error.rs @@ -25,4 +25,7 @@ pub enum SequencerActorError { /// An error occurred while attempting to schedule unsafe payload gossip. #[error("An error occurred while attempting to schedule unsafe payload gossip: {0}")] PayloadGossip(#[from] UnsafePayloadGossipClientError), + /// Payload was missing a required field. + #[error("Missing field: {0} required")] + MissingField(String), } diff --git a/crates/node/service/src/service/node.rs b/crates/node/service/src/service/node.rs index 57d0f0838b..fdbdbe4f6d 100644 --- a/crates/node/service/src/service/node.rs +++ b/crates/node/service/src/service/node.rs @@ -171,7 +171,7 @@ impl RollupNode { unsafe_head_rx, }, engine, - ) = EngineActor::new(self.engine_config()); + ) = EngineActor::new(self.engine_config())?; // Create the p2p actor. let ( @@ -220,16 +220,10 @@ impl RollupNode { let origin_selector = L1OriginSelector::new(self.config.clone(), l1_provider); let block_building_client = QueuedBlockBuildingClient { - build_request_tx: build_request_tx.expect( - "build_request_tx is None in sequencer mode. This should never happen.", - ), + build_request_tx: build_request_tx.unwrap(), reset_request_tx: reset_request_tx.clone(), - seal_request_tx: seal_request_tx.expect( - "seal_request_tx is None in sequencer mode. This should never happen.", - ), - unsafe_head_rx: unsafe_head_rx.expect( - "unsafe_head_rx is None in sequencer mode. This should never happen.", - ), + seal_request_tx: seal_request_tx.unwrap(), + unsafe_head_rx: unsafe_head_rx.unwrap(), }; // Conditionally add conductor if configured From 72642fb6ed91f776dda28163aefd31c18c2604cc Mon Sep 17 00:00:00 2001 From: varun-doshi Date: Fri, 28 Nov 2025 20:14:33 +0530 Subject: [PATCH 2/4] fix: apply review changes --- .../node/service/src/actors/engine/actor.rs | 15 +++---------- crates/node/service/src/actors/engine/mod.rs | 2 +- crates/node/service/src/actors/mod.rs | 4 ++-- .../service/src/actors/sequencer/builder.rs | 22 +++++++++---------- .../service/src/actors/sequencer/error.rs | 7 +++++- .../node/service/src/actors/sequencer/mod.rs | 2 +- crates/node/service/src/lib.rs | 5 +++-- crates/node/service/src/service/node.rs | 8 ++++++- 8 files changed, 34 insertions(+), 31 deletions(-) diff --git a/crates/node/service/src/actors/engine/actor.rs b/crates/node/service/src/actors/engine/actor.rs index 358e4f6568..a1040c5565 100644 --- a/crates/node/service/src/actors/engine/actor.rs +++ b/crates/node/service/src/actors/engine/actor.rs @@ -142,7 +142,7 @@ pub struct EngineInboundData { } /// Validates that the sequencer fields are set. -fn validate_sequencer_fields( +pub fn validate_sequencer_fields( build_request_tx: &Option>, seal_request_tx: &Option>, unsafe_head_rx: &Option>, @@ -272,7 +272,7 @@ struct SequencerChannels { impl EngineActor { /// Constructs a new [`EngineActor`] from the params. - pub fn new(config: EngineConfig) -> Result<(EngineInboundData, Self), String> { + pub fn new(config: EngineConfig) -> (EngineInboundData, Self) { let (finalized_l1_block_tx, finalized_l1_block_rx) = watch::channel(None); let (inbound_queries_tx, inbound_queries_rx) = mpsc::channel(1024); let (attributes_tx, attributes_rx) = mpsc::channel(1024); @@ -320,15 +320,6 @@ impl EngineActor { rollup_boost_health_query_rx, }; - // Validate sequencer fields if node is in sequencer mode. - if actor.builder.mode.is_sequencer() { - validate_sequencer_fields( - &sequencer_channels.build_request_tx, - &sequencer_channels.seal_request_tx, - &sequencer_channels.unsafe_head_rx, - )?; - } - let outbound_data = EngineInboundData { attributes_tx, build_request_tx: sequencer_channels.build_request_tx, @@ -342,7 +333,7 @@ impl EngineActor { unsafe_head_rx: sequencer_channels.unsafe_head_rx, }; - Ok((outbound_data, actor)) + (outbound_data, actor) } } diff --git a/crates/node/service/src/actors/engine/mod.rs b/crates/node/service/src/actors/engine/mod.rs index 4ee4032408..c6440db959 100644 --- a/crates/node/service/src/actors/engine/mod.rs +++ b/crates/node/service/src/actors/engine/mod.rs @@ -3,7 +3,7 @@ mod actor; pub use actor::{ BuildRequest, EngineActor, EngineConfig, EngineContext, EngineInboundData, ResetRequest, - SealRequest, + SealRequest, validate_sequencer_fields, }; mod error; diff --git a/crates/node/service/src/actors/mod.rs b/crates/node/service/src/actors/mod.rs index a29f4aa5b2..6f043a9b2b 100644 --- a/crates/node/service/src/actors/mod.rs +++ b/crates/node/service/src/actors/mod.rs @@ -9,7 +9,7 @@ mod engine; pub use engine::{ BlockBuildingClient, BlockEngineError, BlockEngineResult, BuildRequest, EngineActor, EngineConfig, EngineContext, EngineError, EngineInboundData, L2Finalizer, - QueuedBlockBuildingClient, ResetRequest, SealRequest, + QueuedBlockBuildingClient, ResetRequest, SealRequest, validate_sequencer_fields, }; mod rpc; @@ -39,5 +39,5 @@ pub use sequencer::{ Conductor, ConductorClient, ConductorError, DelayedL1OriginSelectorProvider, L1OriginSelector, L1OriginSelectorError, L1OriginSelectorProvider, OriginSelector, QueuedSequencerAdminAPIClient, SequencerActor, SequencerActorBuilder, SequencerActorError, SequencerAdminQuery, - SequencerConfig, + SequencerBuilderError, SequencerConfig, }; diff --git a/crates/node/service/src/actors/sequencer/builder.rs b/crates/node/service/src/actors/sequencer/builder.rs index 9ad2702477..3396b7e473 100644 --- a/crates/node/service/src/actors/sequencer/builder.rs +++ b/crates/node/service/src/actors/sequencer/builder.rs @@ -13,7 +13,7 @@ use std::sync::Arc; use tokio::sync::mpsc; use tokio_util::sync::CancellationToken; -use super::SequencerActorError; +use super::error::SequencerBuilderError; /// Builder for constructing a [`SequencerActor`]. #[derive(Debug, Default)] @@ -173,36 +173,36 @@ where OriginSelector_, UnsafePayloadGossipClient_, >, - SequencerActorError, + SequencerBuilderError, > { Ok(SequencerActor { admin_api_rx: self .admin_api_rx - .ok_or(SequencerActorError::MissingField("admin_api_rx".to_string()))?, + .ok_or(SequencerBuilderError::MissingField("admin_api_rx".to_string()))?, attributes_builder: self .attributes_builder - .ok_or(SequencerActorError::MissingField("attributes_builder".to_string()))?, + .ok_or(SequencerBuilderError::MissingField("attributes_builder".to_string()))?, block_building_client: self .block_building_client - .ok_or(SequencerActorError::MissingField("block_building_client".to_string()))?, + .ok_or(SequencerBuilderError::MissingField("block_building_client".to_string()))?, cancellation_token: self .cancellation_token - .ok_or(SequencerActorError::MissingField("cancellation_token".to_string()))?, + .ok_or(SequencerBuilderError::MissingField("cancellation_token".to_string()))?, conductor: self.conductor, is_active: self .is_active - .ok_or(SequencerActorError::MissingField("is_active".to_string()))?, + .ok_or(SequencerBuilderError::MissingField("is_active".to_string()))?, in_recovery_mode: self .in_recovery_mode - .ok_or(SequencerActorError::MissingField("in_recovery_mode".to_string()))?, + .ok_or(SequencerBuilderError::MissingField("in_recovery_mode".to_string()))?, origin_selector: self .origin_selector - .ok_or(SequencerActorError::MissingField("origin_selector".to_string()))?, + .ok_or(SequencerBuilderError::MissingField("origin_selector".to_string()))?, rollup_config: self .rollup_config - .ok_or(SequencerActorError::MissingField("rollup_config".to_string()))?, + .ok_or(SequencerBuilderError::MissingField("rollup_config".to_string()))?, unsafe_payload_gossip_client: self.unsafe_payload_gossip_client.ok_or( - SequencerActorError::MissingField("unsafe_payload_gossip_client".to_string()), + SequencerBuilderError::MissingField("unsafe_payload_gossip_client".to_string()), )?, }) } diff --git a/crates/node/service/src/actors/sequencer/error.rs b/crates/node/service/src/actors/sequencer/error.rs index b300c4eafb..caa00ba79c 100644 --- a/crates/node/service/src/actors/sequencer/error.rs +++ b/crates/node/service/src/actors/sequencer/error.rs @@ -25,7 +25,12 @@ pub enum SequencerActorError { /// An error occurred while attempting to schedule unsafe payload gossip. #[error("An error occurred while attempting to schedule unsafe payload gossip: {0}")] PayloadGossip(#[from] UnsafePayloadGossipClientError), - /// Payload was missing a required field. +} + +/// An error produced by the [`crate::SequencerActorBuilder`]. +#[derive(Debug, thiserror::Error)] +pub enum SequencerBuilderError { + /// Builder was missing a required field. #[error("Missing field: {0} required")] MissingField(String), } diff --git a/crates/node/service/src/actors/sequencer/mod.rs b/crates/node/service/src/actors/sequencer/mod.rs index dde8f2cc7c..3bae49957b 100644 --- a/crates/node/service/src/actors/sequencer/mod.rs +++ b/crates/node/service/src/actors/sequencer/mod.rs @@ -22,7 +22,7 @@ pub use builder::SequencerActorBuilder; mod metrics; mod error; -pub use error::SequencerActorError; +pub use error::{SequencerActorError, SequencerBuilderError}; mod conductor; diff --git a/crates/node/service/src/lib.rs b/crates/node/service/src/lib.rs index 4675bb9e29..542c11ad64 100644 --- a/crates/node/service/src/lib.rs +++ b/crates/node/service/src/lib.rs @@ -25,8 +25,9 @@ pub use actors::{ NetworkHandler, NetworkInboundData, NodeActor, OriginSelector, PipelineBuilder, QueuedBlockBuildingClient, QueuedSequencerAdminAPIClient, QueuedUnsafePayloadGossipClient, ResetRequest, RpcActor, RpcActorError, RpcContext, SealRequest, SequencerActor, - SequencerActorBuilder, SequencerActorError, SequencerAdminQuery, SequencerConfig, - UnsafePayloadGossipClient, UnsafePayloadGossipClientError, + SequencerActorBuilder, SequencerActorError, SequencerAdminQuery, SequencerBuilderError, + SequencerConfig, UnsafePayloadGossipClient, UnsafePayloadGossipClientError, + validate_sequencer_fields, }; mod metrics; diff --git a/crates/node/service/src/service/node.rs b/crates/node/service/src/service/node.rs index fdbdbe4f6d..a6d78dc248 100644 --- a/crates/node/service/src/service/node.rs +++ b/crates/node/service/src/service/node.rs @@ -9,6 +9,7 @@ use crate::{ DerivationInboundChannels, EngineInboundData, L1WatcherRpcInboundChannels, NetworkInboundData, QueuedUnsafePayloadGossipClient, SequencerActorBuilder, }, + validate_sequencer_fields, }; use alloy_provider::RootProvider; use kona_derive::StatefulAttributesBuilder; @@ -171,7 +172,7 @@ impl RollupNode { unsafe_head_rx, }, engine, - ) = EngineActor::new(self.engine_config())?; + ) = EngineActor::new(self.engine_config()); // Create the p2p actor. let ( @@ -219,6 +220,11 @@ impl RollupNode { let origin_selector = L1OriginSelector::new(self.config.clone(), l1_provider); + // Validate sequencer fields if node is in sequencer mode. + if self.mode().is_sequencer() { + validate_sequencer_fields(&build_request_tx, &seal_request_tx, &unsafe_head_rx) + .ok()?; + } let block_building_client = QueuedBlockBuildingClient { build_request_tx: build_request_tx.unwrap(), reset_request_tx: reset_request_tx.clone(), From b6154223b4be5c6a5cf1726abb311ec268c62f6a Mon Sep 17 00:00:00 2001 From: varun-doshi Date: Fri, 5 Dec 2025 12:36:05 +0530 Subject: [PATCH 3/4] fix: better builder pattern --- .../node/service/src/actors/engine/actor.rs | 23 --- crates/node/service/src/actors/engine/mod.rs | 2 +- crates/node/service/src/actors/mod.rs | 4 +- .../service/src/actors/sequencer/builder.rs | 131 ++++++++---------- .../service/src/actors/sequencer/error.rs | 8 -- .../node/service/src/actors/sequencer/mod.rs | 2 +- crates/node/service/src/lib.rs | 5 +- crates/node/service/src/service/node.rs | 88 +++++------- 8 files changed, 97 insertions(+), 166 deletions(-) diff --git a/crates/node/service/src/actors/engine/actor.rs b/crates/node/service/src/actors/engine/actor.rs index a1040c5565..942661f080 100644 --- a/crates/node/service/src/actors/engine/actor.rs +++ b/crates/node/service/src/actors/engine/actor.rs @@ -141,29 +141,6 @@ pub struct EngineInboundData { pub unsafe_head_rx: Option>, } -/// Validates that the sequencer fields are set. -pub fn validate_sequencer_fields( - build_request_tx: &Option>, - seal_request_tx: &Option>, - unsafe_head_rx: &Option>, -) -> Result<(), String> { - if build_request_tx.is_none() { - return Err( - "build_request_tx is None in sequencer mode. This should never happen.".to_string() - ); - } - if seal_request_tx.is_none() { - return Err( - "seal_request_tx is None in sequencer mode. This should never happen.".to_string() - ); - } - if unsafe_head_rx.is_none() { - return Err("unsafe_head_rx is None in sequencer mode. This should never happen".to_string()); - } - - Ok(()) -} - /// Configuration for the Engine Actor. #[derive(Debug, Clone)] pub struct EngineConfig { diff --git a/crates/node/service/src/actors/engine/mod.rs b/crates/node/service/src/actors/engine/mod.rs index c6440db959..4ee4032408 100644 --- a/crates/node/service/src/actors/engine/mod.rs +++ b/crates/node/service/src/actors/engine/mod.rs @@ -3,7 +3,7 @@ mod actor; pub use actor::{ BuildRequest, EngineActor, EngineConfig, EngineContext, EngineInboundData, ResetRequest, - SealRequest, validate_sequencer_fields, + SealRequest, }; mod error; diff --git a/crates/node/service/src/actors/mod.rs b/crates/node/service/src/actors/mod.rs index 6f043a9b2b..a29f4aa5b2 100644 --- a/crates/node/service/src/actors/mod.rs +++ b/crates/node/service/src/actors/mod.rs @@ -9,7 +9,7 @@ mod engine; pub use engine::{ BlockBuildingClient, BlockEngineError, BlockEngineResult, BuildRequest, EngineActor, EngineConfig, EngineContext, EngineError, EngineInboundData, L2Finalizer, - QueuedBlockBuildingClient, ResetRequest, SealRequest, validate_sequencer_fields, + QueuedBlockBuildingClient, ResetRequest, SealRequest, }; mod rpc; @@ -39,5 +39,5 @@ pub use sequencer::{ Conductor, ConductorClient, ConductorError, DelayedL1OriginSelectorProvider, L1OriginSelector, L1OriginSelectorError, L1OriginSelectorProvider, OriginSelector, QueuedSequencerAdminAPIClient, SequencerActor, SequencerActorBuilder, SequencerActorError, SequencerAdminQuery, - SequencerBuilderError, SequencerConfig, + SequencerConfig, }; diff --git a/crates/node/service/src/actors/sequencer/builder.rs b/crates/node/service/src/actors/sequencer/builder.rs index 3396b7e473..b9d58db930 100644 --- a/crates/node/service/src/actors/sequencer/builder.rs +++ b/crates/node/service/src/actors/sequencer/builder.rs @@ -13,10 +13,8 @@ use std::sync::Arc; use tokio::sync::mpsc; use tokio_util::sync::CancellationToken; -use super::error::SequencerBuilderError; - /// Builder for constructing a [`SequencerActor`]. -#[derive(Debug, Default)] +#[derive(Debug)] pub struct SequencerActorBuilder< AttributesBuilder_, BlockBuildingClient_, @@ -31,25 +29,25 @@ pub struct SequencerActorBuilder< UnsafePayloadGossipClient_: UnsafePayloadGossipClient, { /// Receiver for admin API requests. - pub admin_api_rx: Option>, + pub admin_api_rx: mpsc::Receiver, /// The attributes builder used for block building. - pub attributes_builder: Option, + pub attributes_builder: AttributesBuilder_, /// The struct used to build blocks. - pub block_building_client: Option, + pub block_building_client: BlockBuildingClient_, /// The cancellation token, shared between all tasks. - pub cancellation_token: Option, + pub cancellation_token: CancellationToken, /// The optional conductor RPC client. pub conductor: Option, /// Whether the sequencer is active. - pub is_active: Option, + pub is_active: bool, /// Whether the sequencer is in recovery mode. - pub in_recovery_mode: Option, + pub in_recovery_mode: bool, /// The struct used to determine the next L1 origin. - pub origin_selector: Option, + pub origin_selector: OriginSelector_, /// The rollup configuration. - pub rollup_config: Option>, + pub rollup_config: Arc, /// A client to asynchronously sign and gossip built payloads to the network actor. - pub unsafe_payload_gossip_client: Option, + pub unsafe_payload_gossip_client: UnsafePayloadGossipClient_, } impl< @@ -74,36 +72,43 @@ where UnsafePayloadGossipClient_: UnsafePayloadGossipClient, { /// Creates a new empty [`SequencerActorBuilder`]. - pub const fn new() -> Self { + pub fn new( + admin_api_rx: mpsc::Receiver, + attributes_builder: AttributesBuilder_, + block_building_client: BlockBuildingClient_, + origin_selector: OriginSelector_, + rollup_config: Arc, + unsafe_payload_gossip_client: UnsafePayloadGossipClient_, + ) -> Self { Self { - admin_api_rx: None, - attributes_builder: None, - block_building_client: None, - cancellation_token: None, + admin_api_rx, + attributes_builder, + block_building_client, + cancellation_token: CancellationToken::new(), conductor: None, - unsafe_payload_gossip_client: None, - is_active: None, - in_recovery_mode: None, - origin_selector: None, - rollup_config: None, + is_active: false, + in_recovery_mode: false, + origin_selector, + rollup_config, + unsafe_payload_gossip_client, } } /// Sets whether the sequencer is active. pub const fn with_active_status(mut self, is_active: bool) -> Self { - self.is_active = Some(is_active); + self.is_active = is_active; self } /// Sets whether the sequencer is in recovery mode. pub const fn with_recovery_mode_status(mut self, is_recovery_mode: bool) -> Self { - self.in_recovery_mode = Some(is_recovery_mode); + self.in_recovery_mode = is_recovery_mode; self } /// Sets the rollup configuration. pub fn with_rollup_config(mut self, rollup_config: Arc) -> Self { - self.rollup_config = Some(rollup_config); + self.rollup_config = rollup_config; self } @@ -112,13 +117,13 @@ where mut self, admin_api_rx: mpsc::Receiver, ) -> Self { - self.admin_api_rx = Some(admin_api_rx); + self.admin_api_rx = admin_api_rx; self } /// Sets the attributes builder. pub fn with_attributes_builder(mut self, attributes_builder: AttributesBuilder_) -> Self { - self.attributes_builder = Some(attributes_builder); + self.attributes_builder = attributes_builder; self } @@ -130,7 +135,7 @@ where /// Sets the origin selector. pub fn with_origin_selector(mut self, origin_selector: OriginSelector_) -> Self { - self.origin_selector = Some(origin_selector); + self.origin_selector = origin_selector; self } @@ -139,13 +144,13 @@ where mut self, block_building_client: BlockBuildingClient_, ) -> Self { - self.block_building_client = Some(block_building_client); + self.block_building_client = block_building_client; self } /// Sets the cancellation token. pub fn with_cancellation_token(mut self, token: CancellationToken) -> Self { - self.cancellation_token = Some(token); + self.cancellation_token = token; self } @@ -154,56 +159,34 @@ where mut self, gossip_client: UnsafePayloadGossipClient_, ) -> Self { - self.unsafe_payload_gossip_client = Some(gossip_client); + self.unsafe_payload_gossip_client = gossip_client; self } - /// Builds the [`SequencerActor`]. - /// - /// # Panics - /// - /// Panics if any required field is not set. + /// Builds the [`SequencerActor`]. + /// + /// This builder cannot fail because all required fields are provided + /// through `new()`. Optional fields (like `conductor`) may be unset. pub fn build( self, - ) -> Result< - SequencerActor< - AttributesBuilder_, - BlockBuildingClient_, - Conductor_, - OriginSelector_, - UnsafePayloadGossipClient_, - >, - SequencerBuilderError, + ) -> SequencerActor< + AttributesBuilder_, + BlockBuildingClient_, + Conductor_, + OriginSelector_, + UnsafePayloadGossipClient_, > { - Ok(SequencerActor { - admin_api_rx: self - .admin_api_rx - .ok_or(SequencerBuilderError::MissingField("admin_api_rx".to_string()))?, - attributes_builder: self - .attributes_builder - .ok_or(SequencerBuilderError::MissingField("attributes_builder".to_string()))?, - block_building_client: self - .block_building_client - .ok_or(SequencerBuilderError::MissingField("block_building_client".to_string()))?, - cancellation_token: self - .cancellation_token - .ok_or(SequencerBuilderError::MissingField("cancellation_token".to_string()))?, + SequencerActor { + admin_api_rx: self.admin_api_rx, + attributes_builder: self.attributes_builder, + block_building_client: self.block_building_client, + cancellation_token: self.cancellation_token, conductor: self.conductor, - is_active: self - .is_active - .ok_or(SequencerBuilderError::MissingField("is_active".to_string()))?, - in_recovery_mode: self - .in_recovery_mode - .ok_or(SequencerBuilderError::MissingField("in_recovery_mode".to_string()))?, - origin_selector: self - .origin_selector - .ok_or(SequencerBuilderError::MissingField("origin_selector".to_string()))?, - rollup_config: self - .rollup_config - .ok_or(SequencerBuilderError::MissingField("rollup_config".to_string()))?, - unsafe_payload_gossip_client: self.unsafe_payload_gossip_client.ok_or( - SequencerBuilderError::MissingField("unsafe_payload_gossip_client".to_string()), - )?, - }) + is_active: self.is_active, + in_recovery_mode: self.in_recovery_mode, + origin_selector: self.origin_selector, + rollup_config: self.rollup_config, + unsafe_payload_gossip_client: self.unsafe_payload_gossip_client, + } } } diff --git a/crates/node/service/src/actors/sequencer/error.rs b/crates/node/service/src/actors/sequencer/error.rs index caa00ba79c..70f1cbf5c4 100644 --- a/crates/node/service/src/actors/sequencer/error.rs +++ b/crates/node/service/src/actors/sequencer/error.rs @@ -26,11 +26,3 @@ pub enum SequencerActorError { #[error("An error occurred while attempting to schedule unsafe payload gossip: {0}")] PayloadGossip(#[from] UnsafePayloadGossipClientError), } - -/// An error produced by the [`crate::SequencerActorBuilder`]. -#[derive(Debug, thiserror::Error)] -pub enum SequencerBuilderError { - /// Builder was missing a required field. - #[error("Missing field: {0} required")] - MissingField(String), -} diff --git a/crates/node/service/src/actors/sequencer/mod.rs b/crates/node/service/src/actors/sequencer/mod.rs index 3bae49957b..dde8f2cc7c 100644 --- a/crates/node/service/src/actors/sequencer/mod.rs +++ b/crates/node/service/src/actors/sequencer/mod.rs @@ -22,7 +22,7 @@ pub use builder::SequencerActorBuilder; mod metrics; mod error; -pub use error::{SequencerActorError, SequencerBuilderError}; +pub use error::SequencerActorError; mod conductor; diff --git a/crates/node/service/src/lib.rs b/crates/node/service/src/lib.rs index 542c11ad64..4675bb9e29 100644 --- a/crates/node/service/src/lib.rs +++ b/crates/node/service/src/lib.rs @@ -25,9 +25,8 @@ pub use actors::{ NetworkHandler, NetworkInboundData, NodeActor, OriginSelector, PipelineBuilder, QueuedBlockBuildingClient, QueuedSequencerAdminAPIClient, QueuedUnsafePayloadGossipClient, ResetRequest, RpcActor, RpcActorError, RpcContext, SealRequest, SequencerActor, - SequencerActorBuilder, SequencerActorError, SequencerAdminQuery, SequencerBuilderError, - SequencerConfig, UnsafePayloadGossipClient, UnsafePayloadGossipClientError, - validate_sequencer_fields, + SequencerActorBuilder, SequencerActorError, SequencerAdminQuery, SequencerConfig, + UnsafePayloadGossipClient, UnsafePayloadGossipClientError, }; mod metrics; diff --git a/crates/node/service/src/service/node.rs b/crates/node/service/src/service/node.rs index a6d78dc248..f31a9acd54 100644 --- a/crates/node/service/src/service/node.rs +++ b/crates/node/service/src/service/node.rs @@ -9,7 +9,6 @@ use crate::{ DerivationInboundChannels, EngineInboundData, L1WatcherRpcInboundChannels, NetworkInboundData, QueuedUnsafePayloadGossipClient, SequencerActorBuilder, }, - validate_sequencer_fields, }; use alloy_provider::RootProvider; use kona_derive::StatefulAttributesBuilder; @@ -188,69 +187,50 @@ impl RollupNode { // Create the RPC server actor. let rpc = self.rpc_builder().map(RpcActor::new); - let (sequencer_actor_builder, sequencer_admin_api_client) = if self.mode().is_sequencer() { + let (sequencer_actor, sequencer_admin_api_client) = if self.mode().is_sequencer() { // Create the admin API channel let (admin_api_tx, admin_api_rx) = mpsc::channel(1024); + // 2. CONFIGURE DEPENDENCIES let cfg = self.sequencer_config.clone(); - let builder = SequencerActorBuilder::new() - .with_active_status(!cfg.sequencer_stopped) - .with_recovery_mode_status(cfg.sequencer_recovery_mode) - .with_rollup_config(self.config.clone()) - .with_admin_api_receiver(admin_api_rx); + // will never panic if `mode().is_sequencer()` + let block_building_client = QueuedBlockBuildingClient { + build_request_tx: build_request_tx.unwrap(), + reset_request_tx: reset_request_tx.clone(), + seal_request_tx: seal_request_tx.unwrap(), + unsafe_head_rx: unsafe_head_rx.unwrap(), + }; - (Some(builder), Some(QueuedSequencerAdminAPIClient::new(admin_api_tx))) - } else { - (None, None) - }; - - // 2. CONFIGURE DEPENDENCIES - - let sequencer_actor = sequencer_actor_builder.map_or_else( - || None, - |mut builder| { - let cfg = self.sequencer_config.clone(); - - let l1_provider = DelayedL1OriginSelectorProvider::new( - self.l1_provider.clone(), - l1_head_updates_tx.subscribe(), - cfg.l1_conf_delay, - ); + let l1_provider = DelayedL1OriginSelectorProvider::new( + self.l1_provider.clone(), + l1_head_updates_tx.subscribe(), + cfg.l1_conf_delay, + ); - let origin_selector = L1OriginSelector::new(self.config.clone(), l1_provider); + let origin_selector = L1OriginSelector::new(self.config.clone(), l1_provider); - // Validate sequencer fields if node is in sequencer mode. - if self.mode().is_sequencer() { - validate_sequencer_fields(&build_request_tx, &seal_request_tx, &unsafe_head_rx) - .ok()?; - } - let block_building_client = QueuedBlockBuildingClient { - build_request_tx: build_request_tx.unwrap(), - reset_request_tx: reset_request_tx.clone(), - seal_request_tx: seal_request_tx.unwrap(), - unsafe_head_rx: unsafe_head_rx.unwrap(), - }; + let mut builder = SequencerActorBuilder::new( + admin_api_rx, + self.create_attributes_builder(), + block_building_client, + origin_selector, + self.config.clone(), + QueuedUnsafePayloadGossipClient::new(gossip_payload_tx.clone()), + ) + .with_active_status(!cfg.sequencer_stopped) + .with_recovery_mode_status(cfg.sequencer_recovery_mode) + .with_cancellation_token(cancellation.clone()); - // Conditionally add conductor if configured - if let Some(conductor_url) = cfg.conductor_rpc_url { - builder = builder.with_conductor(ConductorClient::new_http(conductor_url)); - } + // Conditionally add conductor if configured + if let Some(conductor_url) = cfg.conductor_rpc_url { + builder = builder.with_conductor(ConductorClient::new_http(conductor_url)); + } - Some( - builder - .with_attributes_builder(self.create_attributes_builder()) - .with_block_building_client(block_building_client) - .with_cancellation_token(cancellation.clone()) - .with_unsafe_payload_gossip_client(QueuedUnsafePayloadGossipClient::new( - gossip_payload_tx.clone(), - )) - .with_origin_selector(origin_selector) - .build() - .expect("Failed to build SequencerActor"), - ) - }, - ); + (Some(builder.build()), Some(QueuedSequencerAdminAPIClient::new(admin_api_tx))) + } else { + (None, None) + }; crate::service::spawn_and_wait!( cancellation, From decdc15af8097751da930e4a344d2a77f318fa5a Mon Sep 17 00:00:00 2001 From: varun-doshi Date: Fri, 5 Dec 2025 12:53:35 +0530 Subject: [PATCH 4/4] fix: tests --- .../actors/sequencer/admin_api_impl_test.rs | 51 +++++++++---------- 1 file changed, 23 insertions(+), 28 deletions(-) diff --git a/crates/node/service/src/actors/sequencer/admin_api_impl_test.rs b/crates/node/service/src/actors/sequencer/admin_api_impl_test.rs index e25a6316ce..ffc8a5c557 100644 --- a/crates/node/service/src/actors/sequencer/admin_api_impl_test.rs +++ b/crates/node/service/src/actors/sequencer/admin_api_impl_test.rs @@ -24,16 +24,17 @@ fn test_builder() -> SequencerActorBuilder< MockUnsafePayloadGossipClient, > { let (_admin_api_tx, admin_api_rx) = mpsc::channel(20); - SequencerActorBuilder::new() - .with_active_status(true) - .with_admin_api_receiver(admin_api_rx) - .with_attributes_builder(TestAttributesBuilder { attributes: vec![] }) - .with_block_building_client(MockBlockBuildingClient::new()) - .with_cancellation_token(CancellationToken::new()) - .with_origin_selector(MockOriginSelector::new()) - .with_recovery_mode_status(false) - .with_rollup_config(Arc::new(RollupConfig::default())) - .with_unsafe_payload_gossip_client(MockUnsafePayloadGossipClient::new()) + SequencerActorBuilder::new( + admin_api_rx, + TestAttributesBuilder { attributes: vec![] }, + MockBlockBuildingClient::new(), + MockOriginSelector::new(), + Arc::new(RollupConfig::default()), + MockUnsafePayloadGossipClient::new(), + ) + .with_active_status(true) + .with_cancellation_token(CancellationToken::new()) + .with_recovery_mode_status(false) } #[rstest] @@ -42,7 +43,7 @@ async fn test_is_sequencer_active( #[values(true, false)] active: bool, #[values(true, false)] via_channel: bool, ) { - let mut actor = test_builder().with_active_status(active).build().unwrap(); + let mut actor = test_builder().with_active_status(active).build(); let result = async { match via_channel { @@ -73,8 +74,7 @@ async fn test_is_conductor_enabled( test_builder() } } - .build() - .unwrap(); + .build(); let result = async { match via_channel { @@ -98,7 +98,7 @@ async fn test_in_recovery_mode( #[values(true, false)] recovery_mode: bool, #[values(true, false)] via_channel: bool, ) { - let mut actor = test_builder().with_recovery_mode_status(recovery_mode).build().unwrap(); + let mut actor = test_builder().with_recovery_mode_status(recovery_mode).build(); let result = async { match via_channel { @@ -122,7 +122,7 @@ async fn test_start_sequencer( #[values(true, false)] already_started: bool, #[values(true, false)] via_channel: bool, ) { - let mut actor = test_builder().with_active_status(already_started).build().unwrap(); + let mut actor = test_builder().with_active_status(already_started).build(); // verify starting state let result = actor.is_sequencer_active().await; @@ -167,8 +167,7 @@ async fn test_stop_sequencer_success( let mut actor = test_builder() .with_block_building_client(client) .with_active_status(!already_stopped) - .build() - .unwrap(); + .build(); // verify starting state let result = actor.is_sequencer_active().await; @@ -205,7 +204,7 @@ async fn test_stop_sequencer_error_fetching_unsafe_head(#[values(true, false)] v .times(1) .return_once(|| Err(BlockEngineError::RequestError("whoops!".to_string()))); - let mut actor = test_builder().with_block_building_client(client).build().unwrap(); + let mut actor = test_builder().with_block_building_client(client).build(); let result = async { match via_channel { @@ -234,7 +233,7 @@ async fn test_set_recovery_mode( #[values(true, false)] mode_to_set: bool, #[values(true, false)] via_channel: bool, ) { - let mut actor = test_builder().with_recovery_mode_status(starting_mode).build().unwrap(); + let mut actor = test_builder().with_recovery_mode_status(starting_mode).build(); // verify starting state let result = actor.in_recovery_mode().await; @@ -290,8 +289,7 @@ async fn test_override_leader( test_builder().with_conductor(conductor) } } - .build() - .unwrap(); + .build(); // call to override leader let result = async { @@ -324,7 +322,7 @@ async fn test_reset_derivation_pipeline_success(#[values(true, false)] via_chann let mut client = MockBlockBuildingClient::new(); client.expect_reset_engine_forkchoice().times(1).return_once(|| Ok(())); - let mut actor = test_builder().with_block_building_client(client).build().unwrap(); + let mut actor = test_builder().with_block_building_client(client).build(); let result = async { match via_channel { @@ -350,7 +348,7 @@ async fn test_reset_derivation_pipeline_error(#[values(true, false)] via_channel .times(1) .return_once(|| Err(BlockEngineError::RequestError("reset failed".to_string()))); - let mut actor = test_builder().with_block_building_client(client).build().unwrap(); + let mut actor = test_builder().with_block_building_client(client).build(); let result = async { match via_channel { @@ -382,11 +380,8 @@ async fn test_handle_admin_query_resilient_to_dropped_receiver() { client.expect_get_unsafe_head().times(1).returning(move || Ok(unsafe_head)); client.expect_reset_engine_forkchoice().times(1).returning(|| Ok(())); - let mut actor = test_builder() - .with_conductor(conductor) - .with_block_building_client(client) - .build() - .unwrap(); + let mut actor = + test_builder().with_conductor(conductor).with_block_building_client(client).build(); let mut queries: Vec = Vec::new(); {