refactor(pkg/p2p): swap GossipSub by FloodSub#3263
Conversation
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @julienrbrt's task in 1m 22s —— View job Code Review
SummaryClean, minimal change — the actual implementation is a single line swap in Two issues found — one is a missed stale reference (minor bug), the other is an inaccurate description that could mislead future contributors. Issues1. Stale reference in README mermaid diagram — The top-level architecture diagram was not updated: The second diagram at line 72 was correctly updated to 2. Inaccurate description for FloodSub — This description was carried over from GossipSub, but it's factually wrong for FloodSub. GossipSub was specifically designed to reduce bandwidth overhead via a mesh topology (it only forwards to D peers instead of all peers). FloodSub broadcasts to all directly connected peers — it has higher bandwidth overhead, which is the tradeoff you accept in exchange for simplicity. The description should be corrected to something like: Suggestions (non-blocking)ADR-003 missing rationale — The ADR was updated to replace GossipSub with FloodSub but no explanation for why was added. ADRs are most valuable when they capture the reasoning behind decisions. Consider adding a note (even just a sentence) under the relevant section explaining the tradeoff: FloodSub is simpler, has no misconfiguration risk (no mesh scoring/pruning), and is sufficient for small-topology rollup networks. Code Change AssessmentThe implementation change itself ( |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 53 minutes and 31 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe pubsub backend initialization in Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3263 +/- ##
==========================================
+ Coverage 62.33% 62.35% +0.02%
==========================================
Files 122 122
Lines 12877 12877
==========================================
+ Hits 8027 8030 +3
+ Misses 3969 3967 -2
+ Partials 881 880 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/p2p/client.go`:
- Line 453: Switching c.ps from pubsub.NewGossipSub to pubsub.NewFloodSub (the
change at pubsub.NewFloodSub(ctx, c.host)) increases per-message egress and
removes GossipSub's mesh/gossip and scoring protections; update the PR by either
reverting to GossipSub or adding explicit justification and measurements: state
intended lifetime (temporary debug branch p2p-debug vs permanent), expected
production peer fan-out per node, and benchmark results showing FloodSub
bandwidth/duplicate delivery behavior for worst-case message sizes (blocks/data
blobs). Also mention impacts on downstream consumers such as
pkg/sync/sync_service.go which use goheaderp2p.NewSubscriber with
pubsub.DefaultMsgIdFn and explain how deduplication/delivery semantics will
change under FloodSub. Ensure this rationale is added to the PR description
and/or a comment near the pubsub.NewFloodSub call (c.ps) so reviewers can verify
the tradeoff.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| func (c *Client) setupGossiping(ctx context.Context) error { | ||
| var err error | ||
| c.ps, err = pubsub.NewGossipSub(ctx, c.host) | ||
| c.ps, err = pubsub.NewFloodSub(ctx, c.host) |
There was a problem hiding this comment.
Confirm the scalability tradeoff of switching to FloodSub.
FloodSub forwards every message to every connected peer with no mesh or gossip optimization, so per-message bandwidth scales linearly with peer fan-out. GossipSub was specifically designed to bound this cost via mesh degree (D) + gossip. For a blockchain P2P network propagating headers/blocks/data, this change can materially increase egress bandwidth and duplicate deliveries as the network grows, and it removes score-based peer protection against spam/amplification that GossipSub provides.
Since the PR description doesn't include rationale or benchmarks, could you confirm:
- Expected peer count per node in production — is fan-out bounded enough that flooding is acceptable?
- Whether this is intended as a temporary debug aid (the source branch is
p2p-debug) or a permanent change. - Any measurements showing FloodSub behaves acceptably for the largest expected message sizes (e.g., blocks/data blobs).
Also note that downstream consumers such as pkg/sync/sync_service.go pass pubsub.DefaultMsgIdFn to goheaderp2p.NewSubscriber; that should still work (it's a generic PubSub helper, not GossipSub-specific), but message deduplication characteristics will differ in practice under FloodSub's flood semantics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/p2p/client.go` at line 453, Switching c.ps from pubsub.NewGossipSub to
pubsub.NewFloodSub (the change at pubsub.NewFloodSub(ctx, c.host)) increases
per-message egress and removes GossipSub's mesh/gossip and scoring protections;
update the PR by either reverting to GossipSub or adding explicit justification
and measurements: state intended lifetime (temporary debug branch p2p-debug vs
permanent), expected production peer fan-out per node, and benchmark results
showing FloodSub bandwidth/duplicate delivery behavior for worst-case message
sizes (blocks/data blobs). Also mention impacts on downstream consumers such as
pkg/sync/sync_service.go which use goheaderp2p.NewSubscriber with
pubsub.DefaultMsgIdFn and explain how deduplication/delivery semantics will
change under FloodSub. Ensure this rationale is added to the PR description
and/or a comment near the pubsub.NewFloodSub call (c.ps) so reviewers can verify
the tradeoff.
There was a problem hiding this comment.
This is expected and something I suggested. It does indeed increase bandwidth requirements; however, it simplifies the system substantially as well when it comes to propagation issue debugging. The gossipsub is an overkill for the scale evolve networks are deployed at and if the network ever grows to hundreds of peers we can always migrated back to gossipsub in a backwards compatible manner
There was a problem hiding this comment.
@Wondertan, thanks for the clear rationale — that makes complete sense. FloodSub is a reasonable fit for small, bounded networks where simplicity and debuggability outweigh bandwidth efficiency, and the backwards-compatible migration path to GossipSub if the network scales is a pragmatic safety net. I'll note this for future reviews.
ʕ •ᴥ•ʔ
✏️ Learnings added
Learnt from: Wondertan
Repo: evstack/ev-node PR: 3263
File: pkg/p2p/client.go:453-453
Timestamp: 2026-04-17T10:00:53.638Z
Learning: In evstack/ev-node, the p2p layer intentionally uses FloodSub (pubsub.NewFloodSub) instead of GossipSub. This is a deliberate design decision suggested by Wondertan: FloodSub simplifies propagation debugging, and GossipSub is considered overkill for the small peer counts (well below hundreds) at which evolve networks are deployed. Migration back to GossipSub is considered feasible in a backwards-compatible manner if the network ever grows significantly. Do not flag this as a scalability concern.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: alpe
Repo: evstack/ev-node PR: 3146
File: block/internal/syncing/da_follower.go:128-156
Timestamp: 2026-03-12T14:41:30.304Z
Learning: In evstack/ev-node, DA priority heights (queued via `QueuePriorityHeight` in `block/internal/syncing/da_follower.go`) are untrusted, best-effort optimizations sourced from P2P hints. Dropping a hint on a transient fetch failure is intentional — the sequential catchup loop in `da.Subscriber` will cover every height eventually. Do not flag missing retry/re-queue logic for priority hints as a bug.
Learnt from: alpe
Repo: evstack/ev-node PR: 3131
File: block/internal/syncing/syncer_backoff_test.go:241-245
Timestamp: 2026-03-06T09:40:36.029Z
Learning: In evstack/ev-node, the module declares go 1.25.6. Since Go 1.22, loop variables are per-iteration by default, so loop variable capture is not a concern. Do not flag or fix loop variable capture in this codebase for any Go files; this rule applies to all Go files in the repository, not just the specific test file.
3 participants
Thanks @Wondertan for suggesting this.
Summary by CodeRabbit