feat(preconfs): minimal working sequencer #186
Conversation
Update rpc.rs progress-failing compile v1 Update payload_service.rs Update payload_service.rs Update payload_service.rs hmm Update payload_service.rs compiles but errs Update payload_service.rs working sequencer Update payload_service.rs
WalkthroughThis PR introduces sequencer infrastructure to the Berachain Reth implementation by adding new binaries ( Changes
Sequence Diagram(s)sequenceDiagram
participant Main as bera-sequencer Binary
participant CLI as CliRunner
participant Node as SequencerNode
participant Builder as ComponentsBuilder
participant Pool as BerachainPoolBuilder
participant Payload as SequencerPayloadServiceBuilder
participant Executor as BerachainExecutorBuilder
participant Consensus as BerachainConsensusBuilder
Main->>CLI: Initialize with component builder
CLI->>Node: Create SequencerNode
Node->>Builder: components_builder()
Builder->>Pool: Initialize pool builder
Builder->>Payload: Initialize payload service builder
Builder->>Executor: Initialize executor builder
Builder->>Consensus: Initialize consensus builder
Builder-->>Node: Return composed ComponentsBuilder
CLI->>Node: Launch with DebugNode
Node-->>CLI: Return node handle
CLI->>CLI: Await node exit
Note over CLI: Node runs indefinitely until signal
sequenceDiagram
participant Cmd as Command Receiver
participant PS as SequencerPayloadService
participant Task as Blocking Build Task
participant Result as OneShot Channel
Cmd->>PS: SendCommand (Subscribe/BuildNewPayload)
PS->>PS: transition(cmd)
alt State: Waiting
PS->>PS: WaitingForForkchoiceUpdate
else State: Building
PS->>Task: Spawn build_payload_blocking task
Task->>Task: Construct PayloadConfig
Task->>Task: Execute build_payload (CPU-intensive)
Task->>Result: Send BuildOutcome
Result-->>PS: Receive result
PS->>PS: Transition to WaitingForForkchoiceUpdate
end
Note over PS: Poll loop drives state machine
Cmd->>PS: SendCommand (Resolve)
PS->>PS: Cancel ongoing build via CancellationToken
PS->>PS: Return to WaitingForForkchoiceUpdate
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
src/sequencer/node.rs
Outdated
| .node_types() | ||
| .pool(BerachainPoolBuilder) | ||
| .executor(BerachainExecutorBuilder) | ||
| .payload(SequencerPayloadServiceBuilder::default()) |
There was a problem hiding this comment.
Notice the different SequencerPayloadServiceBuilder
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (10)
src/node/evm/mod.rs (2)
23-25: Defensively clamp extra_data to 32 bytes (header limit)extra_data in Ethereum headers is limited to 32 bytes; while the current string likely fits, future changes could overflow. Clamp to avoid invalid headers.
Apply this diff:
-pub fn default_extra_data_bytes() -> Bytes { - Bytes::from(default_extra_data().as_bytes().to_vec()) -} +pub fn default_extra_data_bytes() -> Bytes { + const MAX_EXTRA_DATA: usize = 32; + let mut bytes = default_extra_data().into_bytes(); + if bytes.len() > MAX_EXTRA_DATA { + bytes.truncate(MAX_EXTRA_DATA); + } + Bytes::from(bytes) +}Optional: de-duplicate with version::init_bera_version’s extra_data source to keep a single definition of the format.
4-4: Consider narrowing visibility for block_context until stabilizedIf block_context is internal for now, prefer pub(crate) to avoid committing to public API prematurely.
- pub mod block_context; + pub(crate) mod block_context;src/bin/bera-sequencer.rs (3)
33-36: Remove unnecessary unsafe around set_varstd::env::set_var is safe; the unsafe block is misleading.
Apply this diff:
- if std::env::var_os("RUST_BACKTRACE").is_none() { - unsafe { std::env::set_var("RUST_BACKTRACE", "1") }; - } + if std::env::var_os("RUST_BACKTRACE").is_none() { + std::env::set_var("RUST_BACKTRACE", "1"); + }
58-60: Use structured logging for errors instead of eprintln!Prefer tracing::error! to keep logs consistent with info!.
Apply this diff:
- use tracing::info; + use tracing::{error, info}; @@ - eprintln!("Error: {err:?}"); + error!(target: "reth::cli", error = ?err, "Sequencer node terminated with error"); std::process::exit(1);Also applies to: 24-25
10-17: Optional: use the public re-export path for EVM configSince BerachainEvmConfig is re-exported, you can shorten the import path for consistency.
Apply this diff:
- evm::BerachainEvmFactory, - node::evm::config::BerachainEvmConfig, + evm::{BerachainEvmConfig, BerachainEvmFactory},Cargo.toml (1)
7-14: Bins added: consider feature‑gating and CI builds for both.If “preconfs” is experimental, gate
bera-sequencerbehind a feature to avoid shipping it by default and ensure CI builds both bins. Also verify these paths exist.src/sequencer/payload_service_builder.rs (2)
27-33: Tighten trait bounds for clearer compile errors.
SequencerPayloadService::newrequiresBlockReaderIdExt<Header = BerachainHeader>on the client. Make it explicit onN::Providerhere to surface better diagnostics.N::Provider: reth_provider::StateProviderFactory + reth_chainspec::ChainSpecProvider<ChainSpec = crate::chainspec::BerachainChainSpec> + + reth::providers::BlockReaderIdExt<Header = crate::primitives::BerachainHeader> + reth_provider::BlockReader + Clone + Unpin + 'static,
59-62: Background task lifecycle/observability.Optionally keep the JoinHandle and add an instrumented span for easier shutdown and tracing.
- tokio::spawn(async move { + let _jh = tokio::spawn(async move { service.await; info!(target: "sequencer", "Payload service terminated"); });src/sequencer/payload_service.rs (2)
86-88: Extract magic number for events channel capacity.Make the channel size a named constant per guidelines.
- let (events_tx, _) = broadcast::channel(100); + let (events_tx, _) = broadcast::channel(EVENTS_CHANNEL_CAP);Add near the top of the module:
const EVENTS_CHANNEL_CAP: usize = 100;
33-58: Comment volume vs. guidelines.The extensive doc comments are helpful now, but our guideline prefers minimal comments; consider condensing to “why” and moving detailed state docs to module‑level docs or a brief ADR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.toml(3 hunks)src/bin/bera-sequencer.rs(1 hunks)src/lib.rs(1 hunks)src/node/evm/mod.rs(4 hunks)src/node/mod.rs(1 hunks)src/sequencer/mod.rs(1 hunks)src/sequencer/node.rs(1 hunks)src/sequencer/payload_service.rs(1 hunks)src/sequencer/payload_service_builder.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.rs: Avoid comments unless explicitly requested
Prefer custom error types over generic errors
Write succinct documentation that focuses on explaining the "why"
Extract magic numbers into named, documented constants
Files:
src/sequencer/mod.rssrc/sequencer/payload_service.rssrc/bin/bera-sequencer.rssrc/node/evm/mod.rssrc/sequencer/payload_service_builder.rssrc/node/mod.rssrc/lib.rssrc/sequencer/node.rs
src/node/mod.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Integrate the node using Reth SDK patterns in the Node Builder
Files:
src/node/mod.rs
🧬 Code graph analysis (4)
src/sequencer/payload_service.rs (2)
src/engine/builder.rs (1)
default_berachain_payload(180-416)src/engine/payload.rs (2)
payload_id(104-106)parent(108-110)
src/bin/bera-sequencer.rs (4)
src/version/mod.rs (1)
init_bera_version(10-37)src/node/evm/config.rs (1)
new_with_evm_factory(55-65)src/sequencer/payload_service.rs (1)
new(80-101)src/chainspec/mod.rs (1)
parse(381-383)
src/node/evm/mod.rs (1)
src/node/evm/executor.rs (1)
evm(333-335)
src/sequencer/payload_service_builder.rs (2)
src/chainspec/mod.rs (1)
chain(229-237)src/sequencer/payload_service.rs (1)
new(80-101)
🔇 Additional comments (12)
src/node/evm/mod.rs (3)
10-11: Re-export of BerachainEvmConfig looks goodPublicly exposing BerachainEvmConfig simplifies downstream usage. LGTM.
47-49: Factory path refactor is correctUsing evm::BerachainEvmFactory with the crate::evm import is clearer and avoids path ambiguities.
12-15: The premise is factually incorrect: src/node/mod.rs does not exist in the repository. Additionally, bothreth::api::NodeTypesandreth_node_api::NodeTypesrefer to the same trait (the latter is re-exported by the former), so no trait mismatch risk exists. The current import in src/node/evm/mod.rs (line 14) aligns with the consistent codebase pattern used across src/sequencer/node.rs, src/rpc/mod.rs, src/engine/validator.rs, and other modules.Likely an incorrect or invalid review comment.
src/node/mod.rs (2)
17-17: Import adjustment for BerachainEthApiBuilder is consistentMatches the updated AddOns type in this module. LGTM.
5-9: Re-exports are properly structured with no naming collisionsVerification confirms each builder has a single definition with no duplicates or conflicts. All four types are correctly re-exported at the crate root in
src/node/mod.rs(lines 5-8), improving downstream ergonomics as intended.src/sequencer/mod.rs (1)
1-10: Review comment based on non-existent featureThe "preconfs" feature referenced in the suggestion does not exist in Cargo.toml. The codebase currently defines only: jemalloc, asm-keccak, min-debug-logs, and client. Additionally, the sequencer module is already publicly exposed in lib.rs (line 15) without conditional compilation. The suggestion cannot be implemented as proposed.
Likely an incorrect or invalid review comment.
src/bin/bera-sequencer.rs (1)
38-44: Binary gating verification inconclusiveThe
bera-sequencerbinary is defined but lacks feature gating. More importantly, the "preconfs" feature referenced in the verification command does not exist in Cargo.toml. The available features are:jemalloc,asm-keccak,min-debug-logs, andclient.Clarification needed:
- Should
bera-sequencerbe optional/gated by a feature?- If yes, which feature should gate it (does "preconfs" need to be added, or use an existing feature)?
src/lib.rs (1)
15-15: The preconfs feature does not exist in the codebase, and sequencer is intentionally public.The review's premise is invalid. A search of the entire codebase reveals zero references to "preconfs," and Cargo.toml defines no such feature. The sequencer module is unconditionally public (line 15 of src/lib.rs), and the dedicated
bera-sequencerbinary directly imports it without feature constraints, indicating this is intended public API, not WIP code requiring gating. The reordering of public modules is unremarkable and does not require verification of downstream crate compatibility.Likely an incorrect or invalid review comment.
Cargo.toml (2)
79-79: Tokio/Tokio‑util compatibility check.You’re on tokio 1.46.0 and tokio‑util 0.7. Verify compatibility with reth v1.8.2 (which pins specific tokio ranges).
65-65: Git‑tag pinning looks good; align all reth crates.All reth crates are pinned to v1.8.2, including the newly added ones. LGTM.
Also applies to: 69-69
src/sequencer/node.rs (2)
52-60: Components wiring looks correct for the sequencer stack.
BerachainPoolBuilder+SequencerPayloadServiceBuilder+EthereumNetworkBuilder+BerachainExecutorBuilder+BerachainConsensusBuilderis coherent.Please confirm
bera-sequenceruses thisSequencerNodepath end‑to‑end (CLI wiring) so the custom payload service is actually active.
77-82: Local payload attributes builder matches chain spec usage.This matches reth’s
LocalPayloadAttributesBuilderpattern. LGTM.
| /// Custom payload service builder that spawns a NoopPayloadBuilder for now | ||
| #[derive(Debug, Clone, Default)] | ||
| pub struct SequencerPayloadServiceBuilder; |
There was a problem hiding this comment.
Docstring is inaccurate (mentions Noop).
Update to reflect that we spawn SequencerPayloadService, not a Noop builder.
-/// Custom payload service builder that spawns a NoopPayloadBuilder for now
+/// Custom payload service builder that spawns the minimal `SequencerPayloadService`.🤖 Prompt for AI Agents
In src/sequencer/payload_service_builder.rs around lines 14 to 16, the struct
doc comment incorrectly says it spawns a NoopPayloadBuilder; update the
docstring to state that this builder spawns a SequencerPayloadService (or the
correct service name used in code). Replace the mention of "Noop" with
"SequencerPayloadService" and adjust wording to accurately describe the
builder's purpose and behavior.
| cmd => { | ||
| warn!(target: "sequencer", ?cmd, "Received unexpected PayloadServiceCommand while waiting for forkchoice update"); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Always reply to commands even in an unexpected state.
Dropping Resolve/PayloadTimestamp here can hang callers (RPC timeouts). Reply deterministically.
- cmd => {
- warn!(target: "sequencer", ?cmd, "Received unexpected PayloadServiceCommand while waiting for forkchoice update");
- }
+ cmd => {
+ match cmd {
+ PayloadServiceCommand::Resolve(_, _, tx) => { let _ = tx.send(None); }
+ PayloadServiceCommand::PayloadTimestamp(_, tx) => { let _ = tx.send(None); }
+ other => warn!(target: "sequencer", ?other, "Unexpected command while waiting"),
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/sequencer/payload_service.rs around lines 174 to 177, the match arm
currently logs unexpected PayloadServiceCommand and drops the command, which can
hang callers (e.g., Resolve / PayloadTimestamp). Change this to always send a
deterministic reply instead of dropping: match on the command variants and for
Resolve/PayloadTimestamp (and any other request/ones expecting a response) send
back an explicit error/timeout/ignored response via the command's reply channel
so callers receive a deterministic failure; keep the warn log but ensure the
reply channel is triggered for every command path.
| let result = tokio::task::block_in_place(|| { | ||
| tokio::runtime::Handle::current().block_on(receiver) | ||
| }); | ||
|
|
||
| let response = match result { | ||
| Ok(Ok(payload)) => { | ||
| info!(target: "sequencer", payload_id = ?payload_id, "Payload successfully retrieved"); | ||
| Some(Box::pin(async move { Ok(payload) }) as _) | ||
| } | ||
| Ok(Err(err)) => { | ||
| error!(target: "sequencer", payload_id = ?payload_id, ?err, "Payload build failed"); | ||
| Some(Box::pin(async move { Err(err) }) as _) | ||
| } | ||
| Err(_) => { | ||
| warn!(target: "sequencer", payload_id = ?payload_id, "Build task dropped without result"); | ||
| None | ||
| } | ||
| }; | ||
|
|
||
| let _ = tx.send(response); |
There was a problem hiding this comment.
Avoid blocking inside poll; asynchronously await the oneshot.
block_in_place + block_on(receiver) risks prolonged blocking on a runtime thread. Return a future that awaits the oneshot instead.
- let result = tokio::task::block_in_place(|| {
- tokio::runtime::Handle::current().block_on(receiver)
- });
-
- let response = match result {
- Ok(Ok(payload)) => {
- info!(target: "sequencer", payload_id = ?payload_id, "Payload successfully retrieved");
- Some(Box::pin(async move { Ok(payload) }) as _)
- }
- Ok(Err(err)) => {
- error!(target: "sequencer", payload_id = ?payload_id, ?err, "Payload build failed");
- Some(Box::pin(async move { Err(err) }) as _)
- }
- Err(_) => {
- warn!(target: "sequencer", payload_id = ?payload_id, "Build task dropped without result");
- None
- }
- };
+ let rx = receiver;
+ let response = Some(Box::pin(async move {
+ match rx.await {
+ Ok(res) => res,
+ Err(e) => Err(PayloadBuilderError::other(e)),
+ }
+ }) as _);
let _ = tx.send(response);🤖 Prompt for AI Agents
In src/sequencer/payload_service.rs around lines 203–222, remove the synchronous
block_in_place + block_on(receiver) and instead create and send a boxed async
future that awaits the oneshot receiver; specifically, construct response =
Some(Box::pin(async move { match receiver.await { Ok(Ok(payload)) => {
info!(...payload_id..., "Payload successfully retrieved"); Ok(payload) }
Ok(Err(err)) => { error!(...payload_id..., ?err, "Payload build failed");
Err(err) } Err(_) => { warn!(...payload_id..., "Build task dropped without
result"); /* return an Err or a suitable cancellation error value */ Err(/*
cancellation error */) } } })) and then tx.send(response); this avoids blocking
the runtime thread and preserves the same logging and result mapping (replace
the inline cancellation error with your crate's appropriate error type).
There was a problem hiding this comment.
done intentionally for now to capture block sealing time
There was a problem hiding this comment.
@rezbera, understood! That makes sense for capturing accurate block sealing metrics. The synchronous wait gives you precise timing from the resolve request through to payload completion. 👍
🐇
_ /)
mo / )
|/)
^ ^ (noting that down...)
| cmd => { | ||
| warn!(target: "sequencer", ?cmd, "Received unexpected PayloadServiceCommand while BuildingPayload"); | ||
| } |
There was a problem hiding this comment.
Reply to BuildNewPayload while already building.
Respond with the current payload_id instead of only logging, to avoid caller hangs.
- cmd => {
- warn!(target: "sequencer", ?cmd, "Received unexpected PayloadServiceCommand while BuildingPayload");
- }
+ cmd => {
+ match cmd {
+ PayloadServiceCommand::BuildNewPayload(_, tx) => {
+ let _ = tx.send(Ok(*payload_id));
+ }
+ other => warn!(target: "sequencer", ?other, "Unexpected command while BuildingPayload"),
+ }
+ }🤖 Prompt for AI Agents
In src/sequencer/payload_service.rs around lines 231 to 233, the match arm that
currently only logs unexpected PayloadServiceCommand while BuildingPayload
should also send a reply with the current payload_id so callers don't hang;
replace the warn-only branch with code that (1) preserves the warning log, and
(2) sends the current payload_id back to the caller via the command's reply
channel (e.g., call cmd.reply(Ok(current_payload_id.clone())) or the equivalent
response method in this codebase), handling any send errors gracefully.
| // TODO: The cancel token should be passed to default_berachain_payload and checked | ||
| // after each transaction is processed. When cancelled, it should stop processing | ||
| // more transactions and immediately finalize the block with transactions processed so far. | ||
| // This will enable getPayload to signal "stop adding txs and return what you have". | ||
| // For now, we don't pass the cancel token to avoid complexity. | ||
| let args = BuildArguments::new( | ||
| Default::default(), // cached_reads | ||
| config, | ||
| Default::default(), // cancel token - TODO: integrate with our cancel_token | ||
| None, // best_payload | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
Cancellation token is unused; Resolve can block until full build completes.
Wire cancel_token into BuildArguments::new so default_berachain_payload can early‑exit on cancel. Without this, Resolve can stall.
Would you like me to draft a small adapter between tokio_util::sync::CancellationToken and the cancel handle expected by BuildArguments for reth v1.8.2?
I'll verify this review comment by examining the code context and understanding the cancellation token integration issue.
---
🏁 Script executed:
```shell
#!/bin/bash
# Get full context around lines 268-278 in payload_service.rs
cat -n src/sequencer/payload_service.rs | sed -n '250,290p'
Length of output: 2220
🏁 Script executed:
#!/bin/bash
# Search for cancel_token usage and type in the file
rg -n 'cancel_token' src/sequencer/payload_service.rs -B2 -A2Length of output: 2007
🏁 Script executed:
#!/bin/bash
# Search for BuildArguments imports and usage
rg -n 'BuildArguments' src/sequencer/payload_service.rs -B2 -A2Length of output: 575
🏁 Script executed:
#!/bin/bash
# Search for default_berachain_payload calls and definition
rg -n 'default_berachain_payload' src/sequencer/payload_service.rs -B2 -A2Length of output: 901
🏁 Script executed:
#!/bin/bash
# Check if there's a Cargo.toml to understand reth version
fd -e Cargo.toml | head -5 | xargs grep -l reth | head -1 | xargs cat | grep -A5 '\[dependencies\]' | grep rethLength of output: 0
🏁 Script executed:
#!/bin/bash
# Find and inspect Cargo.toml files
find . -name Cargo.toml -type f | head -3Length of output: 12
🏁 Script executed:
#!/bin/bash
# Look for Cargo.toml in root and check dependencies
cat Cargo.toml 2>/dev/null | grep -A 20 '\[dependencies\]' | head -30Length of output: 474
🏁 Script executed:
#!/bin/bash
# Check what cancel token type is expected - look for CancellationToken import
rg -n 'use.*CancellationToken' src/sequencer/payload_service.rsLength of output: 43
🏁 Script executed:
#!/bin/bash
# Search for the function signature of build_payload_internal to see full context
rg -n 'async fn build_payload_internal' src/sequencer/payload_service.rs -A 10Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there are other usages of BuildArguments with non-default cancel tokens in codebase
rg -n 'BuildArguments::new' --type rust -B 3 -A 3 | head -40Length of output: 1231
🏁 Script executed:
#!/bin/bash
# Look at PayloadServiceCommand::PayloadTimestamp handling to understand the blocking concern
rg -n 'PayloadServiceCommand::PayloadTimestamp' src/sequencer/payload_service.rs -B 5 -A 15Length of output: 1137
🏁 Script executed:
#!/bin/bash
# Check the run method to understand how Resolve blocks and how cancel_token is used
rg -n 'fn run' src/sequencer/payload_service.rs -A 30 | head -50Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for spawn_blocking or tokio::spawn to see if build task runs on different thread/task
rg -n 'spawn' src/sequencer/payload_service.rs -B 2 -A 2Length of output: 268
🏁 Script executed:
#!/bin/bash
# Check what the actual reth version is
cat Cargo.toml | grep -E 'reth|berachain' | head -20Length of output: 1534
---
🌐 Web query:
```web
reth v1.8.2 BuildArguments cancel token parameter support
💡 Result:
Short answer: Not in upstream Reth v1.8.2 — the cancel token (cancel / Cancelled) appears in the op-reth (Optimism) fork's payload-builder changes, not in the official reth v1.8.2 release.
Evidence:
- Official Reth v1.8.2 release (no mention of a cancel field in BuildArguments). [1]
- op‑reth / payload builder diffs show BuildArguments usages with a cancel: Cancelled and checks like cancel.is_cancelled(). This is in the Optimism fork changes, not the upstream reth crate. [2][3]
If you want, I can point to the exact files/lines in the op‑reth diff or check the upstream reth repo for the BuildArguments struct definition in tag v1.8.2.
Sources:
- Reth v1.8.2 release notes (paradigmxyz/reth). [1]
- op-reth payload builder diffs showing cancel usage (optimism additions). [2]
- reth_basic_payload_builder docs / BuildArguments references. [3]
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for any custom BuildArguments implementation in codebase
rg -n 'struct BuildArguments' --type rust
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there's a custom engine/builder.rs that might have cancel support
rg -n 'cancel' src/engine/builder.rs -B 3 -A 3Length of output: 640
🏁 Script executed:
#!/bin/bash
# Look at the full context of build_payload_internal function
rg -n 'fn build_payload_internal' src/sequencer/payload_service.rs -B 5 -A 50Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check imports and see if BuildArguments comes from reth or is custom
head -50 src/sequencer/payload_service.rs | grep -E 'use|import'Length of output: 661
Wire _cancel_token parameter to BuildArguments::new to enable cancellation.
The cancellation infrastructure exists—default_berachain_payload checks cancel.is_cancelled() at line 262 of src/engine/builder.rs—but the _cancel_token parameter passed to build_payload_internal is unused. Currently, BuildArguments::new receives Default::default() for the cancel token (line 276), so the signal from cancel_token.cancel() (triggered by Resolve at line 199) never reaches the build task. This allows the spawn_blocking task to continue indefinitely, causing Resolve to stall.
Fix: Change line 276 from:
Default::default(), // cancel token - TODO: integrate with our cancel_tokento:
_cancel_token.token(), // wired cancel token for early exit on Resolve(Remove the underscore prefix from the parameter name at line 244.)
🤖 Prompt for AI Agents
In src/sequencer/payload_service.rs around lines 268 to 278, the cancel token
passed into build_payload_internal is unused and BuildArguments::new currently
receives Default::default() for the cancel token so cancel signals never reach
the builder; wire the actual cancel token by replacing the Default::default()
argument with the token from the function parameter (use _cancel_token.token()),
and rename the parameter at its declaration (around line 244) to remove the
leading underscore so it is used (e.g., change _cancel_token to cancel_token) so
the cancel_token.token() can be passed into BuildArguments::new enabling early
exit on Resolve.
| _ => { | ||
| warn!(target: "sequencer", payload_id = ?payload_id, "Build did not produce a better payload"); | ||
| panic!("cannot happen as payloads will always be better"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Remove panic on non‑Better outcome; return an error instead.
A panic in the blocking builder thread can starve Resolve and degrade resilience. Return a typed error.
- _ => {
- warn!(target: "sequencer", payload_id = ?payload_id, "Build did not produce a better payload");
- panic!("cannot happen as payloads will always be better");
- }
+ _ => {
+ warn!(target: "sequencer", payload_id = ?payload_id, "Build did not produce a better payload; returning error");
+ return Err(PayloadBuilderError::other(BuildNotBetterError));
+ }Add this small error type somewhere in this module:
#[derive(Debug, thiserror::Error)]
#[error("build did not produce a better payload")]
pub struct BuildNotBetterError;🤖 Prompt for AI Agents
In src/sequencer/payload_service.rs around lines 305 to 309, replace the panic
in the `_` match arm with returning a typed error: add the suggested
BuildNotBetterError type to this module (the #[derive(Debug, thiserror::Error)]
struct with the "build did not produce a better payload" message) and change the
match arm to return Err(BuildNotBetterError) (or map to that error) instead of
calling panic!; also update the enclosing function signature to return a Result
type (propagating the BuildNotBetterError) if it currently returns plain values
so callers can handle the error.
Only caters to the happy path