#33663 net: Filter addrman during address selection via AddrPolicy to avoid underfill
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #33663 net: Filter addrman during address selection via AddrPolicy to avoid underfill | Waiting on author | 0 (+1) | Mild | 47 + 39 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Introduces an AddrPolicy predicate callback to AddrMan::GetAddr() to filter out addresses during selection rather than post-processing. CConnman::GetAddressesUnsafe() uses this callback to skip banned and discouraged peers while populating the requested address quota. AddrMan also logs the breakdown of addresses filtered by network, quality, and policy.
Problem: Previously, CConnman::GetAddressesUnsafe() fetched a target number of addresses from AddrMan and then erased banned and discouraged addresses from the result. When many banned or discouraged addresses were selected, callers like getnodeaddresses and P2P GETADDR handlers received fewer addresses than requested or none at all, even if AddrMan held plenty of valid alternatives.
Evidence of importance:
- PR description: Repro: ban or discourage a large set of addresses, then call getnodeaddresses 1, previously underfilled. After this change, the RPC returns up to the requested count as expected.
Scope: The callback is executed inside AddrManImpl::GetAddr_ while holding AddrMan::cs, introducing a lock acquisition order of AddrMan::cs followed by BanMan::m_banned_mutex.
Reviewability
Paused · Waiting on author
Open objections from ajtowns, frankomosh, sipa with no author reply for 47 days; review would repeat what has been asked.
Agreement
Mild
objection that needs a change was answered, no reply from the objector (fjahr: introducing AddrPolicy alongside existing network and quality filtering parameters creates redundancy and complexity unl; Bicaru20: introducing a predicate adds unnecessary complexity compared to oversampling or fully unifying all AddrMan filtering; fjahr: AddrPolicy introduces overengineering when simpler oversampling or requesting more addresses and trimming would fix unde); objection that needs an answer has none yet (ajtowns, sipa, frankomosh)
Thread notes: Initial record. While mzumsande, frankomosh, and sipa Concept ACK filtering during selection, fjahr and Bicaru20 oppose the predicate design as overengineering in favor of oversampling (#34162). Unanswered suggestions remain from ajtowns on evicting banned addresses from AddrMan, sipa on moving all selection parameters into the callback, and frankomosh on unit test coverage for underfill.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| brunoerg (member) | objection, correctness | unit tests do not catch a mutant where AddrPolicy breaks out of the loop instead of continuing on filtered addresses | fixed · fix pushed · v1, not yet re-checked | answered | yes | 2025-10-28: 'the following mutant has not been killed (it means tests were not able to detect this change)' Status from: 2025-10-28: waketraindev: 'Added test verifying only expected addresses are returned and filtered ones excluded.' |
| fjahr (member) | objection, approach | introducing AddrPolicy alongside existing network and quality filtering parameters creates redundancy and complexity unless all filtering is unified under the policy | answered · reply on record; standing not assessed yet · v1, not yet re-checked | changed | yes | 2025-12-25: 'if we want to add such a generalized concept for filtering, we should implement the current filtering with it as well. E.g. network and filtered params go away and we only have AddrPolicy as param to GetAddr.' |
| Bicaru20 (contributor) | objection, approach | introducing a predicate adds unnecessary complexity compared to oversampling or fully unifying all AddrMan filtering | answered · reply on record; standing not assessed yet · v1, not yet re-checked | changed | yes | 2025-12-28: 'introducing a predicate feels like adding unnecessary complexity. I believe his solution is cleaner.' |
| fjahr (member) | objection, approach | AddrPolicy introduces overengineering when simpler oversampling or requesting more addresses and trimming would fix underfill | answered · reply on record; standing not assessed yet · v1, not yet re-checked | changed | yes | 2025-12-28: 'If preventing underfilling is the only goal of this PR I would suggest a simpler approach: For example request more addresses than max_addresses and then trim at the end... The current approach doesn't seem minimal and if there is no plan to use it more broadly it seems like overengineering.' |
| ajtowns (member) | objection, approach | retaining banned addresses in AddrMan causes them to compete for slots with un-banned addresses and leads to behavioral differences between fresh and running nodes | open · v1, not yet re-checked | answered | no | 2026-03-20: 'Should we be clearing banned addresses out of addrman? We already skip adding them if they're already banned, so I think it would be consistent.' |
| sipa (member) | objection, interface | partial policy filtering leaves redundant selection parameters and logic split between AddrMan and the caller callback | open · v1, not yet re-checked | answered | no | 2026-05-18: 'if we're going this direction, I think we could reasonably move all selection logic into to function argument?' |
| frankomosh (contributor) | objection, correctness | unit tests run with uncapped limits and do not verify that a filtered request still returns the requested number of addresses | open · v1, not yet re-checked | answered | no | 2026-09-15: 'These calls all pass max_addresses=0, max_pct=0, so both caps are off, and nNodes is the whole table. With no count to fill, they only check that the policy excludes the right addresses, and not that a filtered request still returns what was asked for' |
Support:
- mzumsande (member) (Concept ACK, 2025-10-22): Good to always try to return 1000 addresses in GetAddr responses so peers cannot infer the size of the banlist
- frankomosh (contributor) (Concept ACK, 2025-11-14): (no reason given) [not substantive]
- mzumsande (member) (ACK, 2025-12-22): AddrPolicy avoids coupling AddrMan to BanMan despite an implied lock ordering risk
- sipa (member) (Concept ACK, 2026-05-18): Filtering during selection is a good direction, with suggestion to move all selection logic into the policy argument
- frankomosh (contributor) (Concept ACK, 2026-09-15): Filtering during selection is a good approach
Participants:
- mzumsande (member, support, 7 statements 2025-10-21 to 2026-03-20): Concept ACK and code review ACK; noted returning full address lists avoids leaking banlist size and AddrPolicy avoids coupling AddrMan to BanMan
- brunoerg (member, objection, 2 statements 2025-10-24 to 2025-10-28): flagged unkilled mutant in unit tests where policy filter breaks instead of continuing, resolved with added tests
- frankomosh (contributor, objection, 11 statements 2025-11-14 to 2026-09-15): Concept ACK, but pointed out that existing unit tests run with uncapped limits and do not catch a regression in underfill prevention
- fjahr (member, objection, 13 statements 2025-12-25 to 2026-01-05): opposed AddrPolicy as overengineering unless all filtering is unified under it, preferring a simpler oversampling approach in #34162
- Bicaru20 (contributor, objection, 1 statements 2025-12-28 to 2025-12-28): supported fjahr's simpler oversampling approach and argued introducing a predicate adds unnecessary complexity
- ajtowns (member, objection, 2 statements 2026-03-20 to 2026-03-20): suggested removing banned addresses from AddrMan directly to prevent them competing for slots with un-banned addresses
- sipa (member, objection, 1 statements 2026-05-18 to 2026-05-18): Concept ACK, but suggested moving all selection parameters into the policy callback
- sedited (member, neutral, 1 statements 2026-09-09 to 2026-09-09): pinged frankomosh for follow-up review
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
The short text the ranking pass compares across PRs.
This pull request adds an AddrPolicy callback to AddrMan::GetAddr so callers can filter addresses during selection instead of removing them afterward. CConnman::GetAddressesUnsafe previously fetched a batch of addresses and then stripped banned and discouraged nodes, causing P2P GETADDR responses and the getnodeaddresses RPC to return fewer addresses than requested when many banned peers were selected. By filtering during selection, AddrMan continues searching until the requested quota is filled. The author demonstrates that banning a large set of addresses caused getnodeaddresses 1 to return empty results before this change.
Data
thread read through 2026-09-15 (52 statements), head a02f911ef4
Discussion read under schema 1 rules: suggestions and questions may be missing, and an objection may combine several harms (schema history).
labels: P2P, Needs rebase
Categories
P2P
P3 · bug fix
Prevents GETADDR and getnodeaddresses underfill when nodes have large numbers of banned or discouraged peers. While mzumsande notes that returning full address sets prevents peers from inferring banlist size, the issue only manifests under heavy ban or discouragement states and is reasonably deferrable.
Membership: Labeled P2P and changes AddrMan address selection in src/addrman.cpp and src/net.cpp for GETADDR responses and RPC address retrieval.
Factors: security/stability 1, bug 1, performance 0, user value 1, leverage 0