feat: support p2p-only followers without DA#3386
Merged
Merged
Conversation
Contributor
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughDA initialization is now conditional on node mode and address configuration. Followers without DA sync through P2P, aggregators require a DA address, and DA-dependent sync components are omitted when unavailable. ChangesConditional DA runtime
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RunCmd
participant NodeConfig
participant DAClient
participant Syncer
RunCmd->>NodeConfig: ParseStartConfig()
RunCmd->>NodeConfig: DAEnabled()
RunCmd->>DAClient: create client when enabled
RunCmd->>Syncer: pass DA client or nil
Syncer->>Syncer: start DA workers only when client exists
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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
|
tac0turtle
changed the title
tac0turtle
changed the title
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cmd/run_node.go (1)
161-163: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the DA-disabled log accurate for light nodes.
DAEnabled()is also false for light nodes that do have a configured address, so"DA address is not configured"is misleading. Use a neutral message such as"DA disabled; syncing through P2P only"or distinguish the two cases.🤖 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/cmd/run_node.go` around lines 161 - 163, Update the else branch around DAEnabled() in the node startup flow to use a neutral log message that remains accurate for both light nodes with a configured DA address and nodes without one, such as indicating that DA is disabled and syncing continues through P2P only.
🤖 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 `@pkg/cmd/run_node.go`:
- Around line 154-155: Trim the DA address before passing it to blob client
construction in the node startup flow, using the value checked by
nodeConfig.DAEnabled(). Apply the same normalization at pkg/cmd/run_node.go
lines 154-155 and apps/testapp/cmd/run.go line 128; also update
apps/evm/cmd/run.go lines 63-64 as an affected site, ensuring NewWSClient
receives the trimmed address.
---
Outside diff comments:
In `@pkg/cmd/run_node.go`:
- Around line 161-163: Update the else branch around DAEnabled() in the node
startup flow to use a neutral log message that remains accurate for both light
nodes with a configured DA address and nodes without one, such as indicating
that DA is disabled and syncing continues through P2P only.
🪄 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: b3ebb279-8aca-4987-87b0-cf4e85db0153
📒 Files selected for processing (11)
apps/evm/cmd/run.goapps/testapp/cmd/init_test.goapps/testapp/cmd/run.godocs/learn/config.mddocs/learn/specs/da.mddocs/reference/specs/da.mdpkg/cmd/run_node.gopkg/cmd/run_node_test.gopkg/config/config.gopkg/config/config_test.gopkg/config/defaults.go
💤 Files with no reviewable changes (1)
- pkg/config/defaults.go
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/reference/specs/da.md
- pkg/config/config_test.go
- docs/learn/config.md
- apps/evm/cmd/run.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3386 +/- ##
=======================================
Coverage 62.33% 62.33%
=======================================
Files 120 120
Lines 13425 13444 +19
=======================================
+ Hits 8368 8380 +12
- Misses 4118 4124 +6
- Partials 939 940 +1
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:
|
tac0turtle
added a commit
that referenced
this pull request
Jul 16, 2026
…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>
tac0turtle
added a commit
that referenced
this pull request
Jul 16, 2026
* fix(evm): don't create single sequencer for non-aggregator nodes 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> * fix: keep sequencer for promotable followers and require DA for promotable 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> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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
Summary by CodeRabbit