#33663 net: Filter addrman during address selection via AddrPolicy to avoid underfill

full analysis

https://github.com/bitcoin/bitcoin/pull/33663 · waketraindev · +86/-18 in 6 files, 1 commits · labels: P2P

Goal

  • Return full address responses to peers even when a node maintains many banned addresses
  • Prevent peers from inferring node ban list sizes through short address responses

This pull request introduces an `AddrPolicy` predicate callback to `AddrMan::GetAddr` so callers can filter addresses during selection rather than post-processing them. In `CConnman::GetAddressesUnsafe`, this policy is used to skip banned and discouraged addresses while scanning `AddrMan`. This ensures that `GETADDR` P2P messages and the `getnodeaddresses` RPC return up to the requested number of addresses instead of underfilling.

Problem: When responding to address requests, `GetAddressesUnsafe` previously selected addresses up to the requested limit and then filtered out banned or discouraged nodes afterward. If a node has many banned or discouraged addresses, the returned list contains fewer entries than requested, which degrades address discovery and reveals that the node maintains a large ban list.

Category: P2P (#26 of 65)

P3 · bug fix

  • P3 because fixing address underfill prevents ban list leakage without affecting core network connectivity
  • The privacy and discovery improvement is minor and non-critical for routine operation

The change fixes an address selection shortfall where banned peers caused GETADDR responses to return fewer addresses than expected. As mzumsande noted, returning full sets prevents peers from learning whether a node has a large ban list. The issue is minor in severity and does not disrupt connectivity, making it a worthwhile but non-critical P2P improvement.

Membership: Modifies address selection and filtering in AddrMan and CConnman used for P2P GETADDR responses.

Factors: security/stability 1, bug 1, performance 0, user value 1, leverage 0

Reviewability: Stale: Author silent

  • Author inactive for over two months with open questions on test coverage

The author has been inactive for 83 days, exceeding the 60-day threshold, and recent reviewer feedback on test coverage remains unanswered.

Author status: silent since 2026-06-26

Open concerns:

  • fjahr and Bicaru20 argued that passing an AddrPolicy predicate adds unnecessary complexity compared to oversampling or fully unifying all AddrMan filtering under the policy mechanism.
  • sipa suggested moving all filtering arguments (network, quality, caps) into the policy callback if this direction is chosen.
  • ajtowns suggested removing banned addresses from AddrMan directly rather than filtering them upon selection.
  • frankomosh pointed out that the current unit tests run with unlimited caps and do not actually verify that underfill is avoided when a policy is active.

Resolved concerns:

  • mzumsande noted that answering peer GETADDR queries is the primary use case, prompting updates to the PR title and description.
  • brunoerg and frankomosh identified missing mutation test coverage and requested diagnostics breakdown by filter type, both of which the author added.

Agreement: Positive w/ caveats

  • Support for ensuring full address responses to hide ban list sizes (mzumsande, frankomosh)
  • Prefers simpler oversampling instead of adding a new selection callback (fjahr)
  • Suggested purging banned addresses directly instead of filtering during selection (ajtowns)
  • Suggests moving all selection criteria into the callback if this design is kept (sipa)

Concept supported, but reviewers debated architectural complexity and alternative approaches

Reviewers broadly agree on the value of returning full address quotas (mzumsande, frankomosh, sipa), but fjahr and sipa questioned the piecemeal policy design and ajtowns suggested purging banned addresses from addrman instead.

  • mzumsande and frankomosh gave Concept ACKs, noting it is good to always return full address sets so peers cannot deduce ban list sizes.
  • sipa gave Concept ACK but suggested that all selection logic could reasonably move into the function argument if pursuing this path.
  • fjahr preferred a simpler oversampling approach (#34162) and argued the policy mechanism was overengineering unless generalized.
  • ajtowns questioned whether banned addresses should simply be removed from addrman upon banning.

Review verdicts (DrahtBot): 0 (+1)

Files

39 lines under test/bench/ci.

  • src/addrman.cpp +27/-8
  • src/test/addrman_tests.cpp +21/-0
  • src/test/fuzz/addrman.cpp +18/-0
  • src/addrman.h +12/-1
  • src/net.cpp +6/-7
  • src/addrman_impl.h +2/-2

Card

This PR adds an AddrPolicy callback predicate to AddrMan::GetAddr so CConnman can filter out banned and discouraged peers during selection rather than post-selection. This ensures GETADDR P2P responses and getnodeaddresses RPC calls return the full requested count instead of underfilling, preventing peers from inferring ban list sizes. While multiple reviewers Concept ACKed the intent, discussion remains divided between this approach, generalized filter refactoring, oversampling, and evicting banned peers entirely from AddrMan. Review is currently stale due to author inactivity of over 80 days.

Data

dossier JSON · extract JSON · model openrouter/google/gemini-3.8-flash, generated 2026-09-17T16:16, confidence high, input hash 9cd7855a4250b232