p2p: give seednodes time before falling back to fixed seeds

mzumsande commented at 10:21 pm on May 4, 2023: contributor

-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 commented at 10:21 pm on May 4, 2023: contributor

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.

DrahtBot added the label P2P on May 4, 2023

in src/net.cpp:1675 in 01c7fb639f outdated

1670@@ -1670,12 +1671,12 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
1671                 LogPrintf("Adding fixed seeds as 60 seconds have passed and addrman is empty for at least one reachable network\n");
1672             }
1673 
1674-            // Checking !dnsseed is cheaper before locking 2 mutexes.
1675+            // Checking !dnsseed is cheaper before locking a mutex.
1676             if (!add_fixed_seeds_now && !dnsseed) {

sr-gi commented at 8:36 pm on June 20, 2023:

Can we not drop !add_fixed_seeds_now here and make this an else if?

0if (GetTime<std::chrono::seconds>() > start + std::chrono::minutes{1}) {
1                add_fixed_seeds_now = true;
2                LogPrintf("Adding fixed seeds as 60 seconds have passed and addrman is empty for at least one reachable network\n");
3            } else if (!dnsseed) {
4            // Checking !dnsseed is cheaper before locking a mutex.

mzumsande commented at 7:07 pm on June 21, 2023:

good suggestion, I took it, that should make the code easier to read.

in src/net.cpp:1644 in 01c7fb639f outdated

1640@@ -1641,6 +1641,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
1641     auto next_extra_block_relay = GetExponentialRand(start, EXTRA_BLOCK_RELAY_ONLY_PEER_INTERVAL);
1642     const bool dnsseed = gArgs.GetBoolArg("-dnsseed", DEFAULT_DNSSEED);
1643     bool add_fixed_seeds = gArgs.GetBoolArg("-fixedseeds", DEFAULT_FIXEDSEEDS);
1644+    const bool use_seednodes{gArgs.IsArgSet("-seednode")};

sr-gi commented at 8:45 pm on June 20, 2023:

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.

ajtowns commented at 1:27 pm on June 21, 2023:

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.

mzumsande commented at 7:08 pm on June 21, 2023:

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.

sr-gi commented at 9:04 pm on June 20, 2023: member

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.

fanquake requested review from ajtowns on Jun 21, 2023

fanquake requested review from dergoegge on Jun 21, 2023

in src/net.cpp:1674 in 01c7fb639f outdated

1670@@ -1670,12 +1671,12 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
1671                 LogPrintf("Adding fixed seeds as 60 seconds have passed and addrman is empty for at least one reachable network\n");
1672             }
1673 
1674-            // Checking !dnsseed is cheaper before locking 2 mutexes.
1675+            // Checking !dnsseed is cheaper before locking a mutex.

ajtowns commented at 1:16 pm on June 21, 2023:

Checking use_seednodes is cheap too, so should also be checked before acquiring the mutex rather than after.

mzumsande commented at 7:09 pm on June 21, 2023:

fixed

ajtowns commented at 1:21 pm on June 21, 2023: contributor

Approach ACK 01c7fb639f72f12e3d1ed3aa3da9b86ce1279e81

net: Give seednodes time before falling back to fixed seeds

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.

30778124b8

mzumsande force-pushed on Jun 21, 2023

mzumsande commented at 7:20 pm on June 21, 2023: contributor

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.

DrahtBot added the label CI failed on Jun 21, 2023

dergoegge approved

dergoegge commented at 9:23 am on June 22, 2023: member

Code review ACK 30778124b82791abdc6e930373460ef1dd587cb2

DrahtBot requested review from sr-gi on Jun 22, 2023

DrahtBot removed the label CI failed on Jun 22, 2023

in src/net.cpp:1673 in 30778124b8

1670@@ -1670,12 +1671,12 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
1671                 LogPrintf("Adding fixed seeds as 60 seconds have passed and addrman is empty for at least one reachable network\n");
1672             }
1673

sr-gi commented at 2:54 pm on June 22, 2023:

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

mzumsande commented at 3:43 pm on June 23, 2023:

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

sr-gi commented at 2:54 pm on June 22, 2023: member

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

sr-gi commented at 2:56 pm on June 22, 2023: member

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 commented at 5:58 am on June 23, 2023: contributor

utACK 30778124b82791abdc6e930373460ef1dd587cb2

achow101 commented at 9:33 pm on June 23, 2023: member

ACK 30778124b82791abdc6e930373460ef1dd587cb2

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.

achow101 merged this on Jun 23, 2023

achow101 closed this on Jun 23, 2023

sidhujag referenced this in commit 3620db7a46 on Jun 25, 2023

luke-jr referenced this in commit 5705172d21 on Aug 16, 2023

achow101 referenced this in commit 326e563360 on Apr 30, 2024

bitcoin locked this on Jun 22, 2024

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

Reviews

Conflicts