fix(evm): don't create single sequencer for non-aggregator nodes#3389
Merged
Conversation
P2P-only followers (no DA address) crashed on startup with a nil pointer dereference: the EVM app's createSequencer unconditionally built a single sequencer, whose forced-inclusion retriever calls GetForcedInclusionNamespace on the nil DA client. Mirror the testapp guard from #3386 by skipping sequencer creation for non-aggregators, and fail fast in single.NewSequencer when the DA client is nil so future regressions surface as a clean error instead of a SIGSEGV. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughSequencer creation now supports aggregator and promotable nodes while skipping followers. Startup validation requires a DA address for either mode, and single sequencer construction rejects nil DA clients with test coverage. ChangesSequencer guard behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/sequencers/single/sequencer_test.go (1)
117-122: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the descriptive error message.
require.Errorverifies only that some error occurred; it would still pass if the clear DA-client error were replaced by an unrelated generic failure. Assert the expected message withrequire.EqualError(orrequire.ErrorContains) to protect the new contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/sequencers/single/sequencer_test.go` around lines 117 - 122, Update TestNewSequencer_NilDAClient to assert the descriptive error returned by NewSequencer using require.EqualError or require.ErrorContains, while preserving the existing nil sequencer assertion. Match the expected DA-client validation message to protect the intended error contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/evm/cmd/run.go`:
- Around line 163-165: Update the early-return logic in the node setup flow so
the BasedSequencer-and-non-Aggregator combination is validated before the
ordinary non-aggregator return. Preserve the existing configuration error from
the check near the BasedSequencer validation, while still returning nil for
valid non-aggregator nodes.
---
Nitpick comments:
In `@pkg/sequencers/single/sequencer_test.go`:
- Around line 117-122: Update TestNewSequencer_NilDAClient to assert the
descriptive error returned by NewSequencer using require.EqualError or
require.ErrorContains, while preserving the existing nil sequencer assertion.
Match the expected DA-client validation message to protect the intended error
contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4db8d811-7e4f-4521-8f7e-f1fc52594f67
📒 Files selected for processing (3)
apps/evm/cmd/run.gopkg/sequencers/single/sequencer.gopkg/sequencers/single/sequencer_test.go
…table mode Promotable nodes start as followers but can be promoted to proposer at runtime, which hands the sequencer to the aggregator components. The non-aggregator guard (added to testapp in #3386 and mirrored here for the EVM app) would leave promotable nodes with a nil sequencer and break promotion. Create the sequencer for promotable nodes too, and require a DA address for promotable mode at config validation, same as aggregator mode. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3389 +/- ##
==========================================
+ Coverage 62.31% 62.32% +0.01%
==========================================
Files 120 120
Lines 13452 13454 +2
==========================================
+ Hits 8382 8385 +3
+ Misses 4128 4127 -1
Partials 942 942
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
P2P-only followers (no DA address configured) crash on startup with a nil pointer dereference:
#3386 made the DA client nil when no DA address is set and guarded
block/components.goand testapp'screateSequencer, but the EVM app'screateSequencerwas missed: it unconditionally builds a single sequencer, whose forced-inclusion retriever constructor callsGetForcedInclusionNamespace()on the nil client.While verifying the follower paths, a second gap surfaced: the sequencer is only consumed via
leaderFactory→newAggregatorMode, but promotable nodes start as followers (Aggregator=false) and are handed to the aggregator components on promotion. A plain!Aggregatorguard (as added to testapp in #3386) leaves promotable nodes with a nil sequencer and breaks promotion.Changes
apps/evm/cmd/run.go: skip sequencer creation for nodes that are neither aggregator nor promotable. Plain followers runnewSyncMode, which never touches the sequencer.apps/testapp/cmd/run.go: extend the feat: support p2p-only followers without DA #3386 guard to keep the sequencer for promotable followers, fixing the promotion path there too.pkg/cmd/run_node.go: require a DA address for promotable mode inParseStartConfig, same as aggregator mode — a promoted proposer needs DA, so fail at config validation instead of later.pkg/sequencers/single/sequencer.go: fail fast with a clear error whenNewSequencerreceives a nil DA client — the sequencer dereferences it in several paths, so any future regression surfaces as an error instead of a SIGSEGV.🤖 Generated with Claude Code
Summary by CodeRabbit