addrman: delete addresses that don't belong to the supported networks

brunoerg commented at 8:58 pm on January 26, 2024: contributor

This PR adds a new flag -cleanupaddrman. When initializing a node with this flag, it will delete addresses from addrman (both new and tried tables) that do not belong to the supported networks (e.g. -onlynet). Deleting addresses that do not belong to the supported networks can avoid a lot of unnecessary addrman Select calls (especially when turning from clearnet + Tor/I2P/CJDNS to Tor/I2P/CJDNS).

This flag is not enabled by default, if user intends to go back to the previous network, just do not set it.
Addresses from non-supported networks will naturely be replaced in addrman since we only store addresses from networks we support (cleaning up them on init is a way to avoid spending resources on it).
Avoiding these addresses in addrman can avoid exceeding the maximum number of tries in ThreadOpenConnections.

0// If we didn't find an appropriate destination after trying 100 addresses fetched from addrman,
1// stop this loop, and let the outer loop run again (which sleeps, adds seed nodes, recalculates
2// already-connected network ranges, ...) before trying new addrman addresses.
3nTries++;
4if (nTries > 100)
5    break;

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.

DrahtBot commented at 8:58 pm on January 26, 2024: contributor

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.

Type	Reviewers
Concept ACK	1440000bytes

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.

DrahtBot added the label P2P on Jan 26, 2024

brunoerg commented at 9:08 pm on January 26, 2024: contributor

cc: @mzumsande

brunoerg referenced this in commit c4dec94503 on Jan 26, 2024

brunoerg force-pushed on Jan 26, 2024

brunoerg referenced this in commit cf44d431f2 on Jan 26, 2024

DrahtBot added the label CI failed on Jan 26, 2024

DrahtBot commented at 10:22 pm on January 26, 2024: contributor

🚧 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

addrman: add `DeleteAddresses`

This function deletes addresses from
addrman that are not from the specified
networks.

b8d4119c0e

init: add `-cleanupaddrman`

This flag controls whether the user wants to
delete addresses from addrman that don't belong
to the supported networks in initialization.

b72b948a49

bench: addrman: add `DeleteAddresses` e245d2de66

test: add functional test for `-cleanupaddrman` 4973e04850

brunoerg referenced this in commit 9019237465 on Jan 26, 2024

brunoerg force-pushed on Jan 26, 2024

docs: add release notes for #29330 36fd7046a0

brunoerg force-pushed on Jan 27, 2024

DrahtBot removed the label CI failed on Jan 27, 2024

naumenkogs commented at 9:57 am on January 29, 2024: member

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.

amitiuttarwar commented at 10:53 pm on January 31, 2024: contributor

+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.

1440000bytes commented at 1:25 am on February 1, 2024: none

This PR adds a new flag -cleanupaddrman. When initializing a node with this flag, it will delete addresses from addrman (both new and tried tables) that do not belong to the supported networks (e.g. -onlynet).

I don’t think this is a good idea

This flag is not enabled by default.

Concept ACK because it’s not enabled by default and could be useful for testing.

brunoerg commented at 8:59 pm on February 2, 2024: contributor

@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?

mzumsande commented at 6:19 pm on February 6, 2024: contributor

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.

vasild commented at 6:01 pm on February 11, 2024: contributor

What about setting preferred_net

https://github.com/bitcoin/bitcoin/blob/7143d4388407ab3d12005e55a02d5e8f334e4dc9/src/net.cpp#L2682

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

https://github.com/bitcoin/bitcoin/blob/7143d4388407ab3d12005e55a02d5e8f334e4dc9/src/net.cpp#L2695-L2697

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.

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.

brunoerg commented at 7:36 pm on February 12, 2024: contributor

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?

vasild commented at 11:44 am on February 13, 2024: contributor

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.

brunoerg commented at 9:34 pm on February 13, 2024: contributor

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.

brunoerg closed this on Feb 13, 2024

addrman: delete addresses that don’t belong to the supported networks #29330

Code Coverage

Reviews

Conflicts