net: Avoid undershooting in GetAddressesUnsafe #34162

1. 
   fjahr commented at 5:01 pm on December 28, 2025: contributor

   This is demonstrating an alternative approach to #33663. The author didn’t seem to like by suggestion so I am putting it here to be considered by reviewers. I personally prefer this approach due to it’s simplicity.

   GetAddressesUnsafe filters banned and discouraged addresses if those exist. If addresses are filtered the returned list of addresses can be shorter than originally requested because the list isn’t backfilled with more valid addresses. These results end up in GETADDR responses as well as the getnodeaddresses RPC.

   Fix this by requesting all available addresses and filter them, then only trim them to the requested max_addresses afterwards. Also adds a functional test which checks the new behavior via getnodeaddresses RPC.

2. net: Avoid undershooting in GetAddressesUnsafe

   When addresses are filtered by banman, the final return value could be lower than requested. Fix this by requesting and filtering all addresses before returning a trimmed list.

   54f5add2f1
3. test: Check getnodeaddresses doesn't undershoot if some addresses are banned b4394c8048
4. DrahtBot added the label P2P on Dec 28, 2025
5. 
   DrahtBot commented at 5:02 pm on December 28, 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/34162.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	Bicaru20
   Concept ACK	danielabrozzoni

   If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #33663 (net: Filter addrman during address selection via AddrPolicy to avoid underfill by waketraindev)
   • #29418 (rpc: provide per message stats for global traffic via new RPC ‘getnetmsgstats’ by vasild)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

6. 
   Bicaru20 commented at 0:14 am on December 29, 2025: none

   Code review ACK b4394c8048bcb790d4d8e583f311040d6d20e4f2. I prefer this approach instead of the proposed in #33663 as this one is much simpler. I tested the code an it works as expected.

   My only concern is the impact on efficiency, since now we are passing the filters to all available addresses instead of limiting them to max_addresses. However, I believe this will probably not be an issue, as the number should never be large enough to cause problems,

7. in test/functional/rpc_net.py:607 in b4394c8048

   602+        addresses = node.getnodeaddresses(count=count)
   603+        assert_equal(len(addresses), count)
   604+
   605+        # Verify none of the returned addresses are banned
   606+        for addr in addresses:
   607+            assert addr["address"] not in banned
   

   ---

   ---

   

   Bicaru20 commented at 0:15 am on December 29, 2025:

   Why make this verification? I can’t think why getnodeaddresses would return some addresses that are banned.

   ---

   

   fjahr commented at 5:04 pm on December 29, 2025:

   It’s just a protection against a regression if some refactoring of the banned filtering causes it to not work properly anymore. That may seem a bit far fetched but it also doesn’t cost much.
8. 
   danielabrozzoni commented at 3:04 pm on December 29, 2025: member

   Concept ACK

   I prefer this solution over #33663, as it’s much simpler, and I share the concern that the other one seemed a bit overengineered. (I agree that if we decided to have a AddrPolicy instead, it should also include network and filtered).

   I ran test_getnodeaddresses_with_banned on master and it fails, as expected.

   My only concern is the impact on efficiency, since now we are passing the filters to all available addresses instead of limiting them to max_addresses.

   I think this is a valid concern, and something I wonder too. However, it seems to me that GetAddressesUnsafe is never called in places where performance is critical. These are the call sites:

   • GetAddresses: to recreate the cache when it’s expired (happens about once a day)
   • ASMapHealthCheck: health check for asmap, it’s run on startup and once a day
   • When responding to a GETADDR request from a node with NetPermissionFlags::Addr permission
   • getnodeaddresses RPC

   I think it would be interesting to try and see the performance difference… I will try to look into it, and report back if I succeed :)

Contributors
fjahr DrahtBot Bicaru20 danielabrozzoni

Labels
P2P