net: Favor peers from addrman over fetching seednodes

[sr-gi]

This is a follow-up of #28016 motivated by #28016 (review) and #28016 (comment).

The current behavior of seednode fetching is pretty eager: we do it as the first step under ThreadOpenNetworkConnections even if some peers may be queryable from our addrman. This poses two potential issues:

• First, if permanently set (e.g. running with seednode in a config file) we'd be signaling such seed every time we restart our node
• Second, we will be giving the seed node way too much influence over our addrman, populating the latter with data from the former even when unnecessary

This changes the behavior to only add seednodes to m_addr_fetch if our addrman is empty, or little by little after we've spent some time trying addresses from our addrman. Also, seednodes are added to m_addr_fetch in random order, to avoid signaling the same node in case more than one seed is added and we happen to try them over multiple restarts

[DrahtBot]

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
ACK	cbergqvist

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)
• #26114 (net: Make AddrFetch connections to fixed seeds by mzumsande)

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.

[sr-gi]

This is currently dependent on #28016, given it redefines a local constexpr into a global one, but it could be reworked to add that functionality here if the previous PR doesn't get merged

[DrahtBot]

🚧 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/22454758070}

[sr-gi]

Addressed @brunoerg comment, plus slightly reworked the approach to make sure seednodes are not queried if we reach an acceptable number of peers.

Before this, we would have queried them after the timer went off anyway

[sr-gi]

Rebased to fix CI due to #29610

[cbergqvist]

Concept ACK f809817.

Glad you agreed something like this was worth doing!

[sr-gi]

Rebased to fix merge conflicts

[sr-gi]

Moving this to draft given it is dependant on #28016

[sr-gi]

Moving out from drat given #28016 was recently merged.

This comment is still outstanding, and I'd love some feedback on it: #29605 (comment)

[DrahtBot]

🚧 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/24455108062}

[cbergqvist]

Re-tested 33ddd1b. Bumping of timeout seems blocking.

[cbergqvist]

re-ACK f3c4c4b

Ran test/functional/p2p_seednode.py 15 times to confirm robustness against timeouts.
Also ran functional tests with -extended --exclude=feature_dbcrash with no failures.

Still urge you to do something like f9bfc58 from #29605 (comment) (still cherry-picks cleanly). Narrowing state carried around to when it is actually used is cleaner and more respectful to others reading through CConnman, so they don't have to figure out where that extra m_seed_nodes-member is accessed (+ to RAM that no longer needs to make room for unused state). Maybe it looks a bit more messy around your current change, but it reduces cognitive load/complexity for the rest of the code by reducing "live state". It's a bit like leaving a local variable mutable instead of const at the top of a long function even though it is never changed after initialization.

[sr-gi]

@cbergqvist I took your suggestion from #29605 (comment)

[cbergqvist]

re ACK b41bbd5

Passed functional tests (with --extended --exclude=feature_dbcrash).