net: Avoid undershooting in GetAddressesUnsafe #34162
Conversation
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.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34162. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Code review ACK b4394c8. 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 |
| for addr in addresses: | ||
| assert addr["address"] not in banned |
There was a problem hiding this comment.
Why make this verification? I can't think why getnodeaddresses would return some addresses that are banned.
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
GETADDRrequest from a node withNetPermissionFlags::Addrpermission getnodeaddressesRPC
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 :)
|
ACK b4394c8 |
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.
GetAddressesUnsafefilters 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 inGETADDRresponses as well as thegetnodeaddressesRPC.Fix this by requesting all available addresses and filter them, then only trim them to the requested
max_addressesafterwards. Also adds a functional test which checks the new behavior viagetnodeaddressesRPC.