addrman, net: filter during address selection via AddrPolicy to avoid underfill

waketraindev commented at 6:03 pm on October 20, 2025: none

This PR introduces AddrMan::AddrPolicy, a predicate that allows callers to exclude addresses during selection. The policy returns true to skip a given entry and is evaluated while holding AddrMan::cs.

The mechanism is used in CConnman::GetAddressesUnsafe() to filter out banned and discouraged peers during address selection instead of removing them afterward. This prevents P2P responses (GETADDR) and RPCs (getnodeaddresses) from returning fewer results than requested when large portions of the address space are filtered.

Additional improvements:

Logs the number of filtered entries for diagnostics
Avoids redundant filtering passes in higher-level callers
Keeps locking behavior unchanged (the policy runs under AddrMan::cs)

Testing

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.

No behavior change for unaffected callers.

DrahtBot commented at 6:03 pm on October 20, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33663.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	mzumsande

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

waketraindev marked this as ready for review on Oct 20, 2025

in src/addrman.cpp:843 in c8cb2ea114

839@@ -839,14 +840,17 @@ std::vector<CAddress> AddrManImpl::GetAddr_(size_t max_addresses, size_t max_pct
840         const AddrInfo& ai{it->second};
841 
842         // Filter by network (optional)
843-        if (network != std::nullopt && ai.GetNetClass() != network) continue;
844+        if (network != std::nullopt && ai.GetNetClass() != network) { ++filtered_addresses; continue; }

mzumsande commented at 1:38 pm on October 21, 2025:

here and below: would be good to expand this over multiple lines (clang-format)

mzumsande commented at 8:18 am on October 22, 2025: contributor

Concept ACK

The main function of this is p2p (answering GetAddr requests from peers), RPCs like getnodeaddresses are of secondary importance. Since the p2p use is also affected by this, the PR description should be changed to reflect this.

That said, it seems good to always try to return 1000 addresses in a GetAddr answer, so that peers don’t know whether we have a long list of banned addresses or not.

waketraindev force-pushed on Oct 22, 2025

waketraindev commented at 1:02 pm on October 22, 2025: none

updated PR description and commit message to highlight P2p getaddr responses
corrected formatting

waketraindev requested review from mzumsande on Oct 24, 2025

in src/addrman.cpp:854 in 70e2204b61 outdated

851+        if (ai.IsTerrible(now) && filtered) {
852+            ++filtered_addresses;
853+            continue;
854+        }
855+
856+        // Filter by policy

brunoerg commented at 2:49 pm on October 24, 2025:

You should add a functional/unit test for this. See uncovered new code: https://corecheck.dev/bitcoin/bitcoin/pulls/33663

waketraindev commented at 2:51 pm on October 24, 2025:

Will do, looking into coverege and sonarcloud right now

waketraindev force-pushed on Oct 28, 2025

waketraindev commented at 6:55 am on October 28, 2025: none

Added AddrPolicy coverage in addrman_tests

waketraindev force-pushed on Oct 28, 2025

waketraindev requested review from brunoerg on Oct 28, 2025

waketraindev force-pushed on Oct 28, 2025

addrman, net: filter during selection via AddrPolicy to avoid underfill

Add AddrMan::AddrPolicy, a predicate returning true to exclude an address
during selection. This allows callers to apply custom filtering directly in
AddrMan rather than post-processing results.

Use the new policy in CConnman::GetAddressesUnsafe to exclude banned and
discouraged peers while filling address requests. This fixes underfilled
results observed when banned peers were removed after selection, causing
P2P responses from `GETADDR` to return fewer entries than requested.

Also log the number of filtered entries for debugging and diagnostics.

4f39b9255b

in src/addrman.cpp:857 in d7c5cbb7f9 outdated

854+        }
855+
856+        // Filter by policy
857+        if (policy && policy(ai)) {
858+            ++filtered_addresses;
859+            continue;

brunoerg commented at 1:59 pm on October 28, 2025:

Thanks for adding test coverage. By doing mutation testing for this PR, I noticed that the following mutant has not been killed (it means tests were not able to detect this change):

 0diff --git a/src/addrman.cpp b/src/addrman.cpp
 1index c06fbb0673..4cc7073c3b 100644
 2--- a/src/addrman.cpp
 3+++ b/src/addrman.cpp
 4@@ -854,7 +854,7 @@ std::vector<CAddress> AddrManImpl::GetAddr_(size_t max_addresses, size_t max_pct
 5         // Filter by policy
 6         if (policy && policy(ai)) {
 7             ++filtered_addresses;
 8-            continue;
 9+            break;
10         }

This is because you’re checking that the addresses that are returned when filtering by port are - de facto - according to the policy. However, you could also check that all the proper addresses are being returned, I think just adding the following check would be enough:

0diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp
1index 987249104d..3073656ed5 100644
2--- a/src/test/addrman_tests.cpp
3+++ b/src/test/addrman_tests.cpp
4@@ -421,6 +421,7 @@ BOOST_AUTO_TEST_CASE(addrman_getaddr)
5     BOOST_CHECK_EQUAL(vAddrNoPolicy.size(), addedAddresses);
6     BOOST_CHECK_EQUAL(vAddrPolicyNoSkip.size(), vAddrNoPolicy.size());
7     BOOST_CHECK_EQUAL(vAddrPolicySkipAll.size(), 0);
8+    BOOST_CHECK_EQUAL(vAddrPolicyPort.size(), 4U);
9     BOOST_CHECK(std::none_of(vAddrPolicyPort.begin(), vAddrPolicyPort.end(), policyPortPolicy));

waketraindev commented at 2:29 pm on October 28, 2025:

Thanks for running mutation testing and for review!

I’m thinking:

0    const std::set<CAddress> vExpectedAddresses = {addr1, addr3, addr4, addr5};
1    BOOST_CHECK_EQUAL(vAddrPolicyPort.size(), vExpectedAddresses.size());
2    BOOST_CHECK(std::all_of(vAddrPolicyPort.begin(), vAddrPolicyPort.end(), [&](const CAddress& a){ return vExpectedAddresses.contains(a); }));
3    BOOST_CHECK(std::none_of(vAddrPolicyPort.begin(), vAddrPolicyPort.end(), policyPortPolicy));

waketraindev force-pushed on Oct 28, 2025

waketraindev commented at 2:39 pm on October 28, 2025: none

Added test verifying only expected addresses are returned and filtered ones excluded.

waketraindev requested review from brunoerg on Oct 29, 2025

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

Code Coverage & Benchmarks

Reviews