Repository navigation
feat(p2p): allowlist-only limits connection only to allowed nodes - #1248
Conversation
|
Warning Rate limit exceeded
⌛ 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. 📝 WalkthroughWalkthroughAdds an "allowlist-only" P2P mode that restricts inbound/outbound connections to peers listed in PersistentPeers or BootstrapPeers, with config field, TOML/docs entry, validation, router filtering logic, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Node as Node (startup)
participant Conf as Config (P2P)
participant Builder as buildAllowlist()
participant App as AppClient (ABCI)
participant Router as Router (constructor)
Node->>Conf: read P2P config (AllowlistOnly=true)
Conf->>Builder: parse PersistentPeers & BootstrapPeers
Builder-->>Conf: allowed NodeID set (or error)
alt AppClient present
Node->>App: request ABCI peer filter
App-->>Node: abciFilterByID
Node->>Router: chainFilterByID(allowlist, abciFilter)
else no AppClient
Node->>Router: set FilterPeerByID = allowlist
end
Router-->>Node: constructed with FilterPeerByID
(Note: rectangle styling omitted for clarity.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@node/node.go`:
- Around line 808-837: buildAllowlist currently returns an empty map when peers
lack NodeIDs, which combined with AllowlistOnly=true effectively isolates the
node; update buildAllowlist to detect when allowedIDs is empty after processing
conf.P2P.PersistentPeers and conf.P2P.BootstrapPeers and return an explicit
error (or at minimum log a clear warning) explaining that no NodeIDs were found
and advising to include NodeID@host:port entries or disable AllowlistOnly;
reference the buildAllowlist function and the
conf.P2P.PersistentPeers/conf.P2P.BootstrapPeers inputs so the caller can handle
the error at startup.
🧹 Nitpick comments (1)
node/node_test.go (1)
143-161: Misleading test name: no IP filtering is tested or implemented.
TestGetRouterConfigAllowlistOnlyFiltersByIDAndIPsuggests IP-based filtering is covered, but the test and the implementation only filter byNodeID. Consider renaming to something likeTestGetRouterConfigAllowlistOnlyFiltersByIDto accurately reflect what is tested.
#1364) PR #1248 rewrote getRouterConfig to add allowlist-only composition via chainFilterByID, but silently omitted opts.FilterPeerByIP. The router still calls filterPeersIP when that option is non-nil, so ABCI apps that ban peers by IP address (/p2p/filter/addr/<ip:port>) lost that control without any observable error on upgrade. Restore the FilterPeerByIP closure inside the `conf.FilterPeers && appClient != nil` branch, matching v1.5.4 semantics: issue a Query for /p2p/filter/addr/<ip:port>, reject the peer on any non-OK response or transport error. The allowlist / chainFilterByID logic introduced by #1248 is left intact. Add TestGetRouterConfigFilterPeerByIP with six subtests covering: - non-nil assertion when filtering is enabled with an app client - nil when filtering is disabled or app client is absent - closure accepts a peer on OK ABCI response - closure rejects a peer on non-OK ABCI response - closure propagates ABCI transport errors as rejections Refs: #1248, audit finding GO-004 / SEC MEDIUM Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Issue being fixed or feature implemented
Introduce a
p2p.allowlist-onlyconfiguration option that restricts P2P connections to peers listed inpersistent-peersandbootstrap-peers, enforced byNodeID.What was done?
allowlist-onlytoP2PConfigwith validation (requires at least one peer source).config.tomltemplate with the new option.NodeIDin the P2P router configuration.How Has This Been Tested?
TestGetRouterConfigAllowlistOnlyFiltersByIDAndIPandTestGetRouterConfigAllowlistOnlyAcceptsOnlyNodeIDsinnode/node_test.go.Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Documentation
Tests