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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
• #28960 (kernel: Remove dependency on CScheduler by TheCharlatan)

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.

cc: @mzumsande

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/20916324600}

Having these records around might be useful too, if you go back to those networks.

Do we have any practical problems with these Select calls? Too much CPU time, orz disk reads, or memory use?

Not only this would motivate this code change and review use better, but also it would help us understand under which circumstances a node operator might actually want to do this....And then maybe the right thing would be to trigger this cleaning automatically rather than thinking a user will trigger it.

Or, what i think is even more likely, we may rather change Addrman architecture to facilitate this better in the first place.

+1 to the questions asked by @naumenkogs. I agree with the statement in the OP that this change would "avoid a lot of unnecessary addrman Select calls (especially when...", but would like additional context as to how that would tangibly impact the node operations or end user.

in general I am hesitant to add init params since it increases the combinatoric space of startup possibilities & they are hard to deprecate over time. maybe one idea (if we want to support this feature) could be to add a param to -onlynet instead of a whole new startup option.

@naumenkogs and @amitiuttarwar I just updated the description with more infos. Thanks!

Or, what i think is even more likely, we may rather change Addrman architecture to facilitate this better in the first place.

@naumenkogs: What do you mean by changing addrman architecture?

Deleting addresses that do not belong to the supported networks can avoid a lot of unnecessary addrman Select calls

It would be good to have some intution for how big this effect is. For example, if you have an addrman with mostly IPv4/IPv6 addresses and a few onion ones then restart with -onlynet=onion, will it take noticeably longer to find peers? The worst case scenario would be if you had just a single onion address, but that doesn't seem realistic, so I'd be more interested about the case where you have between 5% and 10% onion addresses.

What about setting preferred_net

if -onlynet is used? Surely it is a waste to select an unreachable address:

That would skew the outcome though. If addrman has 60% IPv4, 25% Tor and 15% I2P addresses right now it is more likely to select Tor over I2P. If we run with -onlynet=tor -onlynet=i2p and set preferred_net to Tor or I2P randomly (50% chance) to avoid the IPv4 addresses, then it will not be more likely to pick Tor. It would be relatively easy to extend that parameter to preferred_nets -- meaning to return an address from one of the given networks.

4. Specially turning from clearnet + Tor/I2P/CJDNS to Tor/I2P/CJDNS, this feature it will ensure we will relay more addresses from these networks to other peers.

clearnet addresses might be useful to Tor peers if they are multi-homed. I think it is good that we send any address type to any peer. Instead of e.g. only Tor addresses to Tor peers.

What about setting preferred_net

Yes, I previously discussed this options with @mzumsande. It might be an alternative, sure.

That would skew the outcome though. If addrman has 60% IPv4, 25% Tor and 15% I2P addresses right now it is more likely to select Tor over I2P. If we run with -onlynet=tor -onlynet=i2p and set preferred_net to Tor or I2P randomly (50% chance) to avoid the IPv4 addresses, then it will not be more likely to pick Tor. It would be relatively easy to extend that parameter to preferred_nets -- meaning to return an address from one of the given networks.

Sure, we can extend addrman's Select to deal with more than one network and return an address from one of the given networks.

clearnet addresses might be useful to Tor peers if they are multi-homed. I think it is good that we send any address type to any peer. Instead of e.g. only Tor addresses to Tor peers.

Sure, this is something we can dicuss because not ensuring we're sending Tor addresses to Tor peers is also not positive. But I agree on relaying clearnet addresses for those nodes. I do not have this data, but I suppose that most Tor peers also run on clearnet?

not ensuring we're sending Tor addresses to Tor peers is also not positive

Right, if we sent solely IPv4 addresses to a Tor peer while we have Tor addresses, that seems like something that can be improved.

I'll close this PR to work on a "less aggressive" approach (extending Select to support multiple networks). I will provide my benchmarks on that new PR.

Thanks, @vasild @mzumsande @naumenkogs @amitiuttarwar.