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

pull brunoerg wants to merge 5 commits into bitcoin:master from brunoerg:2024-01-addrman-delete changing 8 files +126 −0
  1. 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).

    1. This flag is not enabled by default, if user intends to go back to the previous network, just do not set it.
    2. 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).
    3. 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;
    
    1. 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.
  2. 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.

  3. DrahtBot added the label P2P on Jan 26, 2024
  4. brunoerg commented at 9:08 pm on January 26, 2024: contributor
  5. brunoerg referenced this in commit c4dec94503 on Jan 26, 2024
  6. brunoerg force-pushed on Jan 26, 2024
  7. brunoerg referenced this in commit cf44d431f2 on Jan 26, 2024
  8. DrahtBot added the label CI failed on Jan 26, 2024
  9. 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

  10. addrman: add `DeleteAddresses`
    This function deletes addresses from
    addrman that are not from the specified
    networks.
    b8d4119c0e
  11. 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
  12. bench: addrman: add `DeleteAddresses` e245d2de66
  13. test: add functional test for `-cleanupaddrman` 4973e04850
  14. brunoerg referenced this in commit 9019237465 on Jan 26, 2024
  15. brunoerg force-pushed on Jan 26, 2024
  16. docs: add release notes for #29330 36fd7046a0
  17. brunoerg force-pushed on Jan 27, 2024
  18. DrahtBot removed the label CI failed on Jan 27, 2024
  19. 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.

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

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

  22. 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?

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

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

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

  25. 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?

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

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

  28. brunoerg closed this on Feb 13, 2024


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-11 03:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me