fix(pkg/p2p): replace persistent gater with no-op gater#3273
fix(pkg/p2p): replace persistent gater with no-op gater#3273
Conversation
Defines the full TTLConnectionGater feature: TTL-based peer blocklist with config/dynamic block sources, startup reconciliation, background sweep, and Prometheus metrics for blocklist observability. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Stale blocklist entries in the Badger datastore were causing the edennet-2 incident (#3267): fullnodes rejected every binary builder peer, header sync never initialized, and nodes fell back to DA-only sync. Replace the persistent BasicConnectionGater with a no-op variant: - Nil datastore → purely in-memory, nothing survives restart - Removed from libp2p host → no connection-level filtering - Removed setupBlockedPeers / setupAllowedPeers → nothing ever blocks - Instance kept only to satisfy go-header's Exchange API requirement Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The no-op gater implementation resolves the edennet-2 incident (#3267) without the TTL wrapper described in the ADR. The ADR is no longer relevant. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
📝 WalkthroughWalkthroughA new P2P configuration flag Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3273 +/- ##
==========================================
- Coverage 62.60% 62.59% -0.01%
==========================================
Files 122 122
Lines 13020 13029 +9
==========================================
+ Hits 8151 8156 +5
- Misses 3984 3987 +3
- Partials 885 886 +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:
|
julienrbrt
left a comment
julienrbrt
left a comment
There was a problem hiding this comment.
Can we make this behind a evnode.p2p flag?
Like --evnode.p2p.disable_blacklist so we can get why they get blacklisted
Adds p2p.disable_connection_gater (default: true) so operators can re-enable peer-level connection filtering without redeploying a patched binary. When set to false, the connection gater is registered with the libp2p host and blocked_peers / allowed_peers config entries are enforced — useful when experiencing P2P flooding. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![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`:
- Around line 169-178: If DisableConnectionGater is true but conf.BlockedPeers
or conf.AllowedPeers are non-empty, add a warning log so operators know their
settings are ignored; in the method containing the shown block (the code that
currently checks c.conf.DisableConnectionGater and calls
setupBlockedPeers/setupAllowedPeers), detect when c.conf.DisableConnectionGater
== true and (len(c.conf.BlockedPeers) > 0 || len(c.conf.AllowedPeers) > 0) and
emit a clear warning via c.logger.Warn() referencing the fields (e.g.,
"BlockedPeers configured but connection gater disabled") before
returning/continuing, rather than silently dropping the values.
🪄 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: 7dceaf50-9e0c-43bd-bff6-124290699e7b
📒 Files selected for processing (5)
CHANGELOG.mdpkg/config/config.gopkg/config/config_test.gopkg/config/defaults.gopkg/p2p/client.go
| if !c.conf.DisableConnectionGater { | ||
| c.logger.Debug().Str("blacklist", c.conf.BlockedPeers).Msg("blocking blacklisted peers") | ||
| if err := c.setupBlockedPeers(c.parseAddrInfoList(c.conf.BlockedPeers)); err != nil { | ||
| return err | ||
| } | ||
| c.logger.Debug().Str("whitelist", c.conf.AllowedPeers).Msg("allowing whitelisted peers") | ||
| if err := c.setupAllowedPeers(c.parseAddrInfoList(c.conf.AllowedPeers)); err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
Warn when BlockedPeers/AllowedPeers are configured but ignored.
When DisableConnectionGater is true (the default), any values set in conf.BlockedPeers or conf.AllowedPeers are silently dropped — the flags remain wired in AddFlags and the struct fields still exist, so operators can easily set them without realizing they have no effect. Consider emitting a warning log here (or at startup) if either list is non-empty while the gater is disabled, so misconfiguration is surfaced instead of hidden.
Proposed fix
if !c.conf.DisableConnectionGater {
c.logger.Debug().Str("blacklist", c.conf.BlockedPeers).Msg("blocking blacklisted peers")
if err := c.setupBlockedPeers(c.parseAddrInfoList(c.conf.BlockedPeers)); err != nil {
return err
}
c.logger.Debug().Str("whitelist", c.conf.AllowedPeers).Msg("allowing whitelisted peers")
if err := c.setupAllowedPeers(c.parseAddrInfoList(c.conf.AllowedPeers)); err != nil {
return err
}
+ } else if c.conf.BlockedPeers != "" || c.conf.AllowedPeers != "" {
+ c.logger.Warn().
+ Str("blocked_peers", c.conf.BlockedPeers).
+ Str("allowed_peers", c.conf.AllowedPeers).
+ Msg("p2p.blocked_peers / p2p.allowed_peers are set but ignored because p2p.disable_connection_gater=true")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if !c.conf.DisableConnectionGater { | |
| c.logger.Debug().Str("blacklist", c.conf.BlockedPeers).Msg("blocking blacklisted peers") | |
| if err := c.setupBlockedPeers(c.parseAddrInfoList(c.conf.BlockedPeers)); err != nil { | |
| return err | |
| } | |
| c.logger.Debug().Str("whitelist", c.conf.AllowedPeers).Msg("allowing whitelisted peers") | |
| if err := c.setupAllowedPeers(c.parseAddrInfoList(c.conf.AllowedPeers)); err != nil { | |
| return err | |
| } | |
| } | |
| if !c.conf.DisableConnectionGater { | |
| c.logger.Debug().Str("blacklist", c.conf.BlockedPeers).Msg("blocking blacklisted peers") | |
| if err := c.setupBlockedPeers(c.parseAddrInfoList(c.conf.BlockedPeers)); err != nil { | |
| return err | |
| } | |
| c.logger.Debug().Str("whitelist", c.conf.AllowedPeers).Msg("allowing whitelisted peers") | |
| if err := c.setupAllowedPeers(c.parseAddrInfoList(c.conf.AllowedPeers)); err != nil { | |
| return err | |
| } | |
| } else if c.conf.BlockedPeers != "" || c.conf.AllowedPeers != "" { | |
| c.logger.Warn(). | |
| Str("blocked_peers", c.conf.BlockedPeers). | |
| Str("allowed_peers", c.conf.AllowedPeers). | |
| Msg("p2p.blocked_peers / p2p.allowed_peers are set but ignored because p2p.disable_connection_gater=true") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/p2p/client.go` around lines 169 - 178, If DisableConnectionGater is true
but conf.BlockedPeers or conf.AllowedPeers are non-empty, add a warning log so
operators know their settings are ignored; in the method containing the shown
block (the code that currently checks c.conf.DisableConnectionGater and calls
setupBlockedPeers/setupAllowedPeers), detect when c.conf.DisableConnectionGater
== true and (len(c.conf.BlockedPeers) > 0 || len(c.conf.AllowedPeers) > 0) and
emit a clear warning via c.logger.Warn() referencing the fields (e.g.,
"BlockedPeers configured but connection gater disabled") before
returning/continuing, rather than silently dropping the values.
2 participants
Summary
BasicConnectionGater(backed by Badger) with a no-op variant that never persists or blocks any peerlibp2p.ConnectionGaterregistration from the host — no connection-level filtering at allsetupBlockedPeers/setupAllowedPeers— nothing ever blocksgoheaderp2p.NewExchange's concrete type requirementFixes the root cause of the edennet-2 incident (#3267): stale blocklist entries in Badger were causing fullnodes to reject every binary builder peer, preventing header sync from initializing and forcing DA-only sync.
Test plan
go test ./pkg/p2p/...passes🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
p2p.disable_connection_gaterconfiguration flag (enabled by default) to manage P2P connection filtering behavior. Users can disable it to restore peer filtering when experiencing P2P network flooding issues.Bug Fixes