p2p: give seednodes time before falling back to fixed seeds #27577

mzumsande · 2023-05-04T22:21:15Z

-seednode is an alternative bootstrap mechanism - when choosing it, we make a AddrFetch connection to the specified peer, gather addresses from them, and then disconnect. Presumably, if users specify a seednode they prefer addresses from that node over fixed seeds.

However, when disabling dns seeds and specifiying -seednode, CConnman::ProcessAddrFetch() immediately removes the entry from m_addr_fetches (before the seednode could give us addresses) - and once m_addr_fetches is empty, ThreadOpenConnections will add fixed seeds, resulting in a "race" between the fixed seeds and seednodes filling up AddrMan.

This PR suggests to check for any provided -seednode arg instead of using the size of m_addr_fetches, thus delaying the querying of fixed seeds for 1 minute when specifying any seednode (as we already do for addnode peers).
That way, we actually give the seednodes a chance for to provide us with addresses before falling back to fixed seeds.

This can be tested with bitcoind -debug=net -dnsseed=0 -seednode=(...) on a node without peers.dat and observing the debug log.

DrahtBot · 2023-05-04T22:21:17Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	dergoegge, sr-gi, ajtowns, achow101

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:

#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

tACK 01c7fb6

I think it makes sense to prioritize nodes provided via seednode over fixed seeds. However, this makes me think whether we'd like to do the same over dnsseed.

The proposed changes LGTM. Replacing the m_addr_fetch.empty() by use_seednodes doesn't seem to have any side effects, given m_addr_fetch is only populated by addresses provided via seednode and by ThreadDNSAddressSeed. In the former, it means that it cannot be the case that m_addr_fetch is empty if use_seednodes is set. Whereas for the latter, this only triggers provided dnsseed is enabled, so it won't matterfor our case.

There are a couple of things I realized that may be simplifiable even though they are not directly modified by this PR, but are part of the logic around it, check comments inline.

src/net.cpp

sr-gi · 2023-06-20T20:45:45Z

src/net.cpp

Cannot add_fixed_seeds_now be defined here, outside the while loop?

As this is currently defined it could be the case that the flag is re-defined multiple times while waiting for start + std::chrono::minutes{1}) to trigger, while there shouldn't be a need for that. The flag should never go from false to true, so I miss the point of having it within the loop.

Having variables defined over a shorter scope is better than a longer one; it gives the compiler better opportunities to optimise the code and to reuse the variable's memory.

I think I'd prefer to leave add_fixed_seeds_now within the current scope, especially because my plan is to follow up by moving the entire fixed seed logic out of ThreadOpenConnections into what is currently ThreadDNSAddressSeed (#26114). One reason for opening this PR is so that the move into another thread can be more of a refactor and doesn't change timing behavior noticeably.

ajtowns

Approach ACK 01c7fb639f72f12e3d1ed3aa3da9b86ce1279e81

src/net.cpp

Before, we'd remove a seednode from the list right after connecting to it, leading to a race with loading the fixed seed and connecting to them.

mzumsande · 2023-06-21T19:20:18Z

01c7fb6 to 3077812: Addressed suggestions by @sr-gi and @ajtowns

However, this makes me think whether we'd like to do the same over dnsseed.

I agree that it could make sense to not query -dnsseeds if the user specified a -seednode, so that we don't do both at the same time - whether that's by waiting with querying the dns seeds, or soft-setting -dnsseed to 0 I'm not sure. But that should be a seperate PR imo.

dergoegge

Code review ACK 3077812

sr-gi

ACK 3077812 with a tiny nit, feel free to ignore it

sr-gi · 2023-06-22T14:54:00Z

src/net.cpp

                add_fixed_seeds_now = true;
                LogPrintf("Adding fixed seeds as 60 seconds have passed and addrman is empty for at least one reachable network\n");
            }



nit: maybe remove this line between the if/else?

I think I'll remove it in a follow-up #26114 unless I need retouch for another reason.

sr-gi · 2023-06-22T14:56:08Z

01c7fb6 to 3077812: Addressed suggestions by @sr-gi and @ajtowns

However, this makes me think whether we'd like to do the same over dnsseed.

I agree that it could make sense to not query -dnsseeds if the user specified a -seednode, so that we don't do both at the same time - whether that's by waiting with querying the dns seeds, or soft-setting -dnsseed to 0 I'm not sure. But that should be a seperate PR imo.

Agreed that this should be part of a separate PR.

ajtowns · 2023-06-23T05:58:20Z

utACK 3077812

achow101 · 2023-06-23T21:33:27Z

ACK 3077812

Code looks good and a manual test showed that there is now a 1 minute wait after attempting a seednode before the fixed seeds are added.

…fixed seeds 3077812 net: Give seednodes time before falling back to fixed seeds (Martin Zumsande) Pull request description: `-seednode` is an alternative bootstrap mechanism - when choosing it, we make a `AddrFetch` connection to the specified peer, gather addresses from them, and then disconnect. Presumably, if users specify a seednode they prefer addresses from that node over fixed seeds. However, when disabling dns seeds and specifiying `-seednode`, `CConnman::ProcessAddrFetch()` immediately removes the entry from `m_addr_fetches` (before the seednode could give us addresses) - and once `m_addr_fetches` is empty, `ThreadOpenConnections` will add fixed seeds, resulting in a "race" between the fixed seeds and seednodes filling up AddrMan. This PR suggests to check for any provided `-seednode` arg instead of using the size of `m_addr_fetches`, thus delaying the querying of fixed seeds for 1 minute when specifying any seednode (as we already do for `addnode` peers). That way, we actually give the seednodes a chance for to provide us with addresses before falling back to fixed seeds. This can be tested with `bitcoind -debug=net -dnsseed=0 -seednode=(...)` on a node without `peers.dat` and observing the debug log. ACKs for top commit: ajtowns: utACK 3077812 achow101: ACK 3077812 dergoegge: Code review ACK 3077812 sr-gi: ACK [3077812](bitcoin@3077812) with a tiny nit, feel free to ignore it Tree-SHA512: 96446eb34c0805f10ee158a00a3001a07029e795ac40ad5638228d426e30e9bb836c64ac05d145f2f9ab23ec5a528f3a416e3d52ecfdfb0b813bd4b1ebab3c01

Before, we'd remove a seednode from the list right after connecting to it, leading to a race with loading the fixed seed and connecting to them. Github-Pull: bitcoin#27577 Rebased-From: 3077812

…rovided 82f41d7 Added seednode prioritization message to help output (tdb3) 3120a46 Gives seednode priority over dnsseed if both are provided (Sergi Delgado Segura) Pull request description: This is a follow-up of #27577 If both `seednode` and `dnsseed` are provided, the node will start a race between them in order to fetch data to feed the `addrman`. This PR gives priority to `seednode` over `dnsseed` so if some nodes are provided as seeds, they can be tried before defaulting to the `dnsseeds` ACKs for top commit: davidgumberg: untested reACK 82f41d7 itornaza: tested re-ACK 82f41d7 achow101: ACK 82f41d7 cbergqvist: ACK 82f41d7 Tree-SHA512: 4e39e10a7449af6cd9b8f9f6878f846b94bca11baf89ff2d4fbcd4f28293978a6ed71a3a86cea36d49eca891314c834e32af93f37a09c2cc698a878f84d31c62

DrahtBot added the P2P label May 4, 2023

DrahtBot mentioned this pull request May 5, 2023

net: Make AddrFetch connections to fixed seeds #26114

Closed

sr-gi reviewed Jun 20, 2023

View reviewed changes

fanquake requested review from ajtowns and dergoegge June 21, 2023 10:30

ajtowns reviewed Jun 21, 2023

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

net: Give seednodes time before falling back to fixed seeds

3077812

Before, we'd remove a seednode from the list right after connecting to it, leading to a race with loading the fixed seed and connecting to them.

mzumsande force-pushed the 202304_seednode_fixedseed_interaction branch from 01c7fb6 to 3077812 Compare June 21, 2023 19:12

DrahtBot added the CI failed label Jun 21, 2023

dergoegge approved these changes Jun 22, 2023

View reviewed changes

DrahtBot requested a review from sr-gi June 22, 2023 09:23

DrahtBot removed the CI failed label Jun 22, 2023

sr-gi reviewed Jun 22, 2023

View reviewed changes

achow101 merged commit 035ae61 into bitcoin:master Jun 23, 2023

sr-gi mentioned this pull request Jun 30, 2023

p2p: gives seednode priority over dnsseed if both are provided #28016

Merged

davidgumberg mentioned this pull request Mar 14, 2024

p2p: gives seednode priority over dnsseed if both are provided davidgumberg/bitcoin#6

Closed

bitcoin locked and limited conversation to collaborators Jun 22, 2024

p2p: give seednodes time before falling back to fixed seeds #27577

p2p: give seednodes time before falling back to fixed seeds #27577

Uh oh!

Conversation

mzumsande commented May 4, 2023

Uh oh!

DrahtBot commented May 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

sr-gi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sr-gi Jun 20, 2023

Choose a reason for hiding this comment

Uh oh!

ajtowns Jun 21, 2023

Choose a reason for hiding this comment

Uh oh!

mzumsande Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mzumsande commented Jun 21, 2023

Uh oh!

dergoegge left a comment

Choose a reason for hiding this comment

Uh oh!

sr-gi left a comment

Choose a reason for hiding this comment

Uh oh!

sr-gi Jun 22, 2023

Choose a reason for hiding this comment

Uh oh!

mzumsande Jun 23, 2023

Choose a reason for hiding this comment

Uh oh!

sr-gi commented Jun 22, 2023

Uh oh!

ajtowns commented Jun 23, 2023

Uh oh!

achow101 commented Jun 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DrahtBot commented May 4, 2023 •

edited

Loading

mzumsande Jun 21, 2023 •

edited

Loading