net: Avoid undershooting in GetAddressesUnsafe

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.

DrahtBot added the label P2P on Dec 28, 2025

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
Concept ACK	danielabrozzoni, naiyoma
Stale ACK	Bicaru20, bensig

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.

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,

in test/functional/rpc_net.py:610 in b4394c8048 outdated

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.

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

bensig commented at 8:08 pm on January 8, 2026: contributor

ACK b4394c8048bcb790d4d8e583f311040d6d20e4f2 tested

DrahtBot requested review from danielabrozzoni on Jan 8, 2026

DrahtBot added the label Needs rebase on Jan 20, 2026

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.

54896f3c94

test: Check getnodeaddresses doesn't undershoot if some addresses are banned 3f8fe85fae

fjahr force-pushed on Jan 21, 2026

fjahr commented at 8:54 am on January 21, 2026: contributor

Rebased

DrahtBot removed the label Needs rebase on Jan 21, 2026

naiyoma commented at 8:44 am on January 26, 2026: contributor

Concept ACK, I do agree that this approach is simpler, but could be further optimized to improve performance.

For GETADDR, this is my understanding of how we filter addresses. During address selection, and then twice again after selection.

Filter for banned and discouraged addresses in GetAddressesUnsafe.
Filter for IsValid, IsAddrCompatible, and m_addr_known in PushAddress.

If GetAddr returns 1000 addresses and a user has 100 banned and 100 invalid addresses:

0GetAddr() returns:           1000 addresses
1After GetAddressesUnsafe():  900 addresses (100 banned removed)
2After PushAddress():         800 addresses (100 invalid/incompatible removed)
3Final sent to peer:          800 addresses

By fetching all addresses upfront if banman is true and filtering out banned/discouraged addresses and then resizing to max_addresses, we increase the chances of returning 1000 addresses.

I tested https://github.com/bitcoin/bitcoin/pull/34162/commits/54896f3c940738914793283e29101b059c659342 by sending a getaddr to my node

And then logged timestamps: start time right before getting all addresses, after Getaddr(), and end time before returning all addresses.

Before change

02026-01-25T15:45:12Z [net] GetAddressesUnsafe Performance:
12026-01-25T15:45:12Z [net]   max_addresses=1000, max_pct=23
22026-01-25T15:45:12Z [net]   GetAddr() returned: 1000 addresses
32026-01-25T15:45:12Z [net]   Actually banned/discouraged: 0
42026-01-25T15:45:12Z [net]   Trimmed by resize: 0
52026-01-25T15:45:12Z [net]   Final count: 1000
62026-01-25T15:45:12Z [net]   Time - GetAddr=1583μs, ban_filter=682μs, total=2265μs
72026-01-25T15:45:12Z [net] GETADDR: GetAddressesUnsafe returned 1000 addresses in 2573μs

After change

02026-01-25T15:13:32Z [net] received: getaddr (0 bytes) peer=5
12026-01-25T15:13:32Z [net] GetAddressesUnsafe Perfomance:
22026-01-25T15:13:32Z [net] GetAddressesUnsafe: Trimmed by resize: 13540
32026-01-25T15:13:32Z [net] GetAddressesUnsafe: Actually banned/discouraged: 0
42026-01-25T15:13:32Z [net] GetAddressesUnsafe: max_addresses=1000, max_pct=23
52026-01-25T15:13:32Z [net] GetAddressesUnsafe: GetAddr() returned 14540 addresses
62026-01-25T15:13:32Z [net] GetAddressesUnsafe: Final count: 1000
72026-01-25T15:13:32Z [net] GetAddressesUnsafe: Time - GetAddr=17960μs, ban_filter=8781μs, total=26741μs
82026-01-25T15:13:32Z [net] GETADDR: GetAddressesUnsafe returned 1000 addresses in 27012μs

This difference isn’t significant to me (0.024439 seconds), since it’s just a few seconds. But I only have a few addresses in my AddrMan. I’d expect this to be slower if I had more banned addresses and more addresses (40k+). Though performance issue doesn’t bother me as much, since I think it can be improved by not fetching all the addresses, instead max_address * 2 or even max_address * 4 is still sufficient.

net: Avoid undershooting in GetAddressesUnsafe #34162

Code Coverage & Benchmarks

Reviews

Conflicts