Skip to content

refactor(pkg/p2p): swap GossipSub by FloodSub #3263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
julienrbrt merged 2 commits into from
Apr 17, 2026
Merged

refactor(pkg/p2p): swap GossipSub by FloodSub #3263

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
28ec4f9
refactor(pkg/p2p): swap GossipSub by FloodSub
julienrbrt Apr 17, 2026
c599936
docs + cl
julienrbrt Apr 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/p2p/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,7 +450,7 @@ func (c *Client) tryConnect(ctx context.Context, peer peer.AddrInfo) {

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)
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 17, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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:

  1. Expected peer count per node in production — is fan-out bounded enough that flooding is acceptable?
  2. Whether this is intended as a temporary debug aid (the source branch is p2p-debug) or a permanent change.
  3. 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.

Copy link
Copy Markdown
Contributor

@Wondertan Wondertan Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

if err != nil {
return err
}
Expand Down
Loading