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

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

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, frankomosh

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: contributor

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:855 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: contributor

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

in src/addrman.cpp:858 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: contributor

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

waketraindev requested review from brunoerg on Oct 29, 2025

waketraindev renamed this:
~~addrman, net: filter during address selection via AddrPolicy to avoid underfill~~
addrman, net: Filter during address selection via AddrPolicy to avoid underfill
on Nov 3, 2025

waketraindev renamed this:
~~addrman, net: Filter during address selection via AddrPolicy to avoid underfill~~
net: Filter during address selection via AddrPolicy to avoid underfill
on Nov 8, 2025

DrahtBot added the label P2P on Nov 8, 2025

waketraindev renamed this:
~~net: Filter during address selection via AddrPolicy to avoid underfill~~
net: Filter addrman during address selection via AddrPolicy to avoid underfill
on Nov 8, 2025

in src/net.cpp:3532 in 4f39b9255b outdated

3532-        addresses.erase(std::remove_if(addresses.begin(), addresses.end(),
3533-                        [this](const CAddress& addr){return m_banman->IsDiscouraged(addr) || m_banman->IsBanned(addr);}),
3534-                        addresses.end());
3535-    }
3536-    return addresses;
3537+        const AddrMan::AddrPolicy policy = [this](const CAddress& addr) {

frankomosh commented at 10:46 am on November 14, 2025:

Is there a way/test to verify that the BanMan methods here are safe to call while AddrMan’s lock is held?

Can see a comment in addrman.h says the policy “must not acquire locks that can conflict with AddrMan::cs”. So it would make sense to ensure deadlock scenario is not accidentally created.

waketraindev commented at 1:01 pm on November 14, 2025:

The BanMan methods used here take m_banned_mutex and never call back into AddrMan, so the lock order in this policy is currently AddrMan::cs>BanMan::m_banned_mutex with no reverse lock order in the codebase.

Using the predicate here also enforces that AddrMan::cs is always taken first, before any BanMan locks which helps maintain a consistent lock order.

GetAddressesUnsafe() is invoked on startup to load peers.dat with the responses being cached for CConnman::GetAddresses(), every 24h for CConnman::ASMapHealthCheck(), when a GETADDR message is called and the peer has permission NetPermissionFlags::Addr and finally by RPC getnodeaddresses. I tested each of these manually and checked the lock ordering and lock wait times.

Regarding ways to test for lock ordering, a debug build will enable DEBUG_LOCKORDER and DEBUG_LOCKCONTENTION which will log issues with lock ordering or lock waits (--debug=lock). (see: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#debug_lockorder)

frankomosh commented at 8:13 am on November 17, 2025:

The BanMan methods used here take m_banned_mutex and never call back into AddrMan, so the lock order in this policy is currently AddrMan::cs>BanMan::m_banned_mutex with no reverse lock order in the codebase.

Thanks. Might be worth a comment somewhere in the code documenting this ordering requirement so someone does not accidentally introduce the reverse path in future ?

waketraindev commented at 11:27 am on November 17, 2025:

Updated comments for more clarity in addrman.h:

0    /** Predicate used to exclude addresses during selection.
1     *  Return true to skip the given address.
2     *
3     *  Runs while holding AddrMan::cs, so it must be non-blocking, must not
4     *  attempt to reacquire AddrMan::cs, and must preserve lock ordering to
5     *  avoid deadlocks.
6     */

in net.cpp -> GetAddressesUnsafe()

0        // This runs under AddrMan::cs, and BanMan checks take m_banned_mutex.
1        // The lock order here is AddrMan::cs -> m_banned_mutex.

frankomosh commented at 4:01 pm on November 17, 2025:

Thanks

in src/addrman.cpp:844 in 4f39b9255b

839@@ -839,14 +840,26 @@ 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) {
845+            ++filtered_addresses;

frankomosh commented at 10:53 am on November 14, 2025:

Would network mismatches also count as filtered i this case?

waketraindev commented at 1:06 pm on November 14, 2025:

Yes; Currently all skipped addresses in AddrMan::GetAddr are counted as filtered; If you’d prefer (or find it useful) separate counters for each reason (Network, Quality or Policy), I can add that.

frankomosh commented at 8:07 am on November 17, 2025:

Maybe separate would be clearer for diagnostics. Up to you though,.. if you think the current logging is clear enough then that’s fine too.

waketraindev commented at 11:25 am on November 17, 2025:

Went with separate counters and also added the selected network to the log output for clarity. Example log entry:

0[addrman] GetAddr returned 62823 random addresses from ipv4; 1104 filtered (0 network, 0 quality, 1104 policy)
1[addrman] GetAddr returned 0 random addresses from ipv6; 63927 filtered (63927 network, 0 quality, 0 policy)
2[addrman] GetAddr returned 0 random addresses from onion; 63927 filtered (66927 network, 0 quality, 0 policy)

frankomosh commented at 4:08 pm on November 17, 2025:

Awesome

frankomosh commented at 10:54 am on November 14, 2025: contributor

Concept ACK

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.

fb3efdb3d2

waketraindev force-pushed on Nov 17, 2025

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

Code Coverage & Benchmarks

Reviews