Repository navigation
fix(p2p): restore ABCI IP-based peer filtering dropped in #1248 - #1364
Conversation
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>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
|
✅ Review complete (commit c292278) |
There was a problem hiding this comment.
Pull request overview
This PR restores the ABCI-based IP/addr peer filtering hook (opts.FilterPeerByIP) in the node’s P2P router options, which was inadvertently removed in PR #1248. This re-enables operators/apps to reject peers based on their IP address (and port) via ABCI query endpoints.
Changes:
- Reintroduced wiring for
RouterOptions.FilterPeerByIPwhenFilterPeersis enabled and an ABCI client is available. - Implemented the ABCI query path
/p2p/filter/addr/<ip:port>for the IP filter closure. - Added a regression unit test suite (
TestGetRouterConfigFilterPeerByIP) covering enabled/disabled behavior and OK/non-OK/error ABCI responses.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
node/node.go |
Restores FilterPeerByIP by querying the ABCI app on incoming connections using the /p2p/filter/addr/... path. |
node/node_test.go |
Adds regression tests verifying the filter is wired correctly and queries the expected ABCI path, including error cases. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR #1364 narrowly restores the opts.FilterPeerByIP closure dropped by #1248, re-wiring the ABCI /p2p/filter/addr/<ip:port> peer rejection path with byte-equivalent semantics to v1.5.4 (IPv6-safe net.JoinHostPort, fail-closed on transport error and non-OK ABCI response, properly gated on conf.FilterPeers && appClient != nil). Both independent agents converged on no in-scope findings; the diff at node/node.go:781-797 matches the stated intent and is exercised by six focused subtests. Approving.
Summary
Restores the ABCI
/p2p/filter/addr/%sIP-based peer filter (opts.FilterPeerByIP) that was inadvertently dropped in #1248, re-enabling operator address-based peer rejection via the ABCI app.Tests
TestGetRouterConfigFilterPeerByIP(6 subtests)🤖 Generated with Claude Code