p2p: gives seednode priority over dnsseed if both are provided

sr-gi commented at 2:23 pm on June 30, 2023: member

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

DrahtBot commented at 2:23 pm on June 30, 2023: 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
ACK	davidgumberg, cbergqvist, itornaza, achow101
Stale ACK	tdb3

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:

#29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)
#29605 (net: Favor peers from addrman over fetching seednodes by sr-gi)
#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 Jun 30, 2023

sr-gi commented at 2:31 pm on June 30, 2023: member

I wanted to have an exit early strategy that will hand the logic back to dnsseed if all seednodes have been tried and no data has been added to the addrman. However, looks like checking if both m_nodes and m_addr_fetch are empty may not be sufficient (i.e. a node can be removed from m_addr_fetch, tried but not yet put at m_nodes. This seems to be specifically sensitive for privacy networks where the latency may be higher)

sr-gi force-pushed on Jun 30, 2023

in src/net.cpp:1413 in f86cb3c668 outdated

1409+                return;
1410+
1411+            // Abort if we have spent enough time without reaching our target.
1412+            // Giving seed nodes 30 seconds so this does not become a race against fixedseeds (which
1413+            // triggers after a minute)
1414+            if (GetTime<std::chrono::seconds>() > start + std::chrono::seconds{30}) {

maflcko commented at 2:33 pm on June 30, 2023:

nit: Could use NodeClock::now() for new code? Also 30s should work for the constant.

sr-gi commented at 3:32 pm on June 30, 2023:

Should be fixed in ac7ce24

sr-gi force-pushed on Jun 30, 2023

DrahtBot added the label CI failed on Jun 30, 2023

DrahtBot removed the label CI failed on Jun 30, 2023

luke-jr commented at 3:17 pm on July 4, 2023: member

If both are provided, I would expect both to be active in parallel, and complete even if the other succeeds…

DrahtBot added the label CI failed on Jul 30, 2023

DrahtBot removed the label CI failed on Aug 1, 2023

DrahtBot added the label Needs rebase on Sep 14, 2023

sr-gi commented at 3:51 pm on September 14, 2023: member

If both are provided, I would expect both to be active in parallel, and complete even if the other succeeds…

Kindly add some rationale for that?

As I see it, if the user is changing a default (adding a seednode in this case) I’d make sense to prioritize that, especially in this case where the dnsseeds are more likely to provide a higher number of addresses for the node to pick up. Hence, the odds of the user selecting a peer from the seednode(s) that he has manually specified are lower

sr-gi force-pushed on Sep 14, 2023

DrahtBot removed the label Needs rebase on Sep 14, 2023

luke-jr commented at 5:47 pm on September 15, 2023: member

If both are provided, I would expect both to be active in parallel, and complete even if the other succeeds…

Kindly add some rationale for that?

If the user has specified both, presumably he wants both to be used…

sr-gi commented at 6:23 pm on September 15, 2023: member

If both are provided, I would expect both to be active in parallel, and complete even if the other succeeds…

Kindly add some rationale for that?

If the user has specified both, presumably he wants both to be used…

And they are, just not at the same time.

The same rationale applies to #27577, yet seednodes are prioritized over fixed seeds

DrahtBot added the label CI failed on Sep 29, 2023

DrahtBot removed the label CI failed on Oct 4, 2023

sr-gi force-pushed on Oct 4, 2023

DrahtBot added the label CI failed on Oct 8, 2023

DrahtBot removed the label CI failed on Oct 16, 2023

DrahtBot added the label CI failed on Jan 13, 2024

sr-gi force-pushed on Jan 29, 2024

sr-gi commented at 10:07 pm on January 29, 2024: member

Rebased to fix CI

DrahtBot removed the label CI failed on Jan 30, 2024

DrahtBot added the label Needs rebase on Jan 31, 2024

sr-gi force-pushed on Feb 2, 2024

DrahtBot removed the label Needs rebase on Feb 2, 2024

in src/net.cpp:2192 in 206094a73e outdated

2188@@ -2189,7 +2189,30 @@ void CConnman::ThreadDNSAddressSeed()
2189     std::vector<std::string> seeds = m_params.DNSSeeds();
2190     Shuffle(seeds.begin(), seeds.end(), rng);
2191     int seeds_right_now = 0; // Number of seeds left before testing if we have enough connections
2192-    int found = 0;
2193+    int target_outbound_connections = 2;

tdb3 commented at 3:54 pm on February 23, 2024:

Is there a particular rationale for this chosen target of 2? nit: Recommend adding comment with rationale (perhaps I overlooked this).

sr-gi commented at 9:21 pm on February 28, 2024:

We are just keeping it consistent with what nRelevant used to be

cbergqvist commented at 9:38 am on February 29, 2024:

Suggestion to change to:

0constexpr int TARGET_OUTBOUND_CONNECTIONS = 2;

tdb3 commented at 3:55 pm on February 23, 2024: contributor

Brief Code Review ACK 206094a73e23c3e6c09f21a4888b1a8edb217aab Great tweak. Seems reasonable (and unsurprising) from a user perspective that explicitly defined seeds would be prioritized over DNS seeds.

nit: What are your thoughts on informing the user of seednode prioritization behavior in help output? Welcome to cherry pick the commit here: https://github.com/tdb3/bitcoin/commit/c83d32e86cf40ceec1a0cf53b435e0fd8a3dabfe

sr-gi commented at 9:22 pm on February 28, 2024: member

nit: What are your thoughts on informing the user of seednode prioritization behavior in help output? Welcome to cherry pick the commit here: tdb3@c83d32e

That makes sense to me

cbergqvist commented at 12:51 pm on February 29, 2024: contributor

If the user has specified both, presumably he wants both to be used…

And they are, just not at the same time.

The same rationale applies to #27577, yet seednodes are prioritized over fixed seeds

Attempt to clarify: #27577 postpones the possible usage of fixed seeds if -seednode is specified. It does not prioritize -seednode over -dnsseed.

I started out agreeing with @luke-jr that if one specifies both -seednode and -dnsseed one probably wants both. But net.h states:

0static constexpr bool DEFAULT_DNSSEED{true};

So most nodes have it enabled by default. Makes sense to prioritize -seednode if specified.

in src/net.cpp:2191 in 87be4abd33 outdated

2188@@ -2189,7 +2189,30 @@ void CConnman::ThreadDNSAddressSeed()
2189     std::vector<std::string> seeds = m_params.DNSSeeds();
2190     Shuffle(seeds.begin(), seeds.end(), rng);
2191     int seeds_right_now = 0; // Number of seeds left before testing if we have enough connections

cbergqvist commented at 1:10 pm on February 29, 2024:

Consider moving these variable declarations (including rng) after the new if (gArgs.IsArgSet("-seednode"))-block since they aren’t used until later.

sr-gi commented at 2:40 pm on February 29, 2024: member

Attempt to clarify: #27577 postpones the possible usage of fixed seeds if -seednode is specified. It does not prioritize -seednode over -dnsseed.

From where I see it that is literally prioritizing one over the other. Before the aforementioned change, both would race, after the change, seednode was given a 1 min headstart before defaulting to dnsseed

cbergqvist commented at 5:11 pm on February 29, 2024: contributor

From where I see it that is literally prioritizing one over the other. Before the aforementioned change, both would race, after the change, seednode was given a 1 min headstart before defaulting to dnsseed

Unless I’ve misunderstood, #27577 is concerned about what happens to -seednode when -dnsseed is turned off:

However, when disabling dns seeds and specifiying -seednode …

That’s when the race condition that PR is concerned about occurs. Both before and after #27577, ThreadOpenConnections() had an if-condition with std::chrono::minutes{1}, but that’s literally to set add_fixed_seeds_now = true;, so CChainParams::vFixedSeeds, not for DNS seeds CChainParams::vSeeds.

sr-gi commented at 5:24 pm on February 29, 2024: member

From where I see it that is literally prioritizing one over the other. Before the aforementioned change, both would race, after the change, seednode was given a 1 min headstart before defaulting to dnsseed

Unless I’ve misunderstood, #27577 is concerned about what happens to -seednode when -dnsseed is turned off:

However, when disabling dns seeds and specifiying -seednode …

That’s when the race condition that PR is concerned about occurs. Both before and after #27577, ThreadOpenConnections() had an if-condition with std::chrono::minutes{1}, but that’s literally to set add_fixed_seeds_now = true;, so CChainParams::vFixedSeeds, not for DNS seeds CChainParams::vSeeds.

Sorry, I meant fixed seeds, not dnsseeds

in src/net.cpp:2192 in 87be4abd33 outdated

2208+            }
2209+
2210+            outbound_connection_count = GetFullOutboundConnCount();
2211+            if (outbound_connection_count >= target_outbound_connections) {
2212+                    LogPrintf("P2P peers available. Finished fetching data from seed nodes.\n");
2213+                break;

cbergqvist commented at 2:55 pm on March 2, 2024:

nit: indentation

tdb3 commented at 10:58 pm on March 2, 2024: contributor

re ACK for 87be4abd33eb6c539a5f953db2f4be3248a86dd0

cbergqvist commented at 9:54 pm on March 3, 2024: contributor

Concept ACK 87be4ab.

My initial impression was that it’s a bit too loosely coupled having CConnman::ThreadDNSAddressSeed() just wait around 30s with the actual work of fetching peers happening on another thread. Later I noticed this in the log:

0Waiting 300 seconds before querying DNS seeds.

That’s the pre-existing code waiting for other threads to populate AddrMan (maybe through peers.dat and/or anchors.dat), before compromising privacy by reaching out to the hardcoded DNS seed nodes that could be monitored to a higher extent. So I agree with the approach in this PR.

Future PR idea: -seednode does actually have a similar privacy issue to the hardcoded DNS seed nodes in that if a bitcoin.conf was shared around containing a fixed set of seednodes, that could make nodes easier to monitor. Another PR should probably actually de-prioritize -seednode in a similar way to increase privacy through preferring peers.dat and anchors.dat.

Still have some more testing to do, but unless that uncovers something there probably won’t be more feedback.

in src/net.cpp:2197 in 87be4abd33 outdated

2193+    int target_outbound_connections = 2;
2194+    int outbound_connection_count = 0;
2195+
2196+    auto start = NodeClock::now();
2197+    if (gArgs.IsArgSet("-seednode")) {
2198+        LogPrintf("-seednode enabled. Trying the provided seeds before defaulting to the dnsseeds.\n");

cbergqvist commented at 10:49 pm on March 3, 2024:

Only additional nit is that DNS seeds and fixed seeds output the number of seconds, might be nice & consistent to do so here too. Consider breaking out a constexpr std::chrono::seconds SEEDNODE_TIMEOUT = 30s; and %d-ing it into the message.

Compare with DNS seeds:

0Waiting 300 seconds before querying DNS seeds.

And fixed seeds:

0Adding fixed seeds as 60 seconds have passed and addrman is empty for at least one reachable network

cbergqvist approved

cbergqvist commented at 10:56 pm on March 3, 2024: contributor

ACK 87be4ab (regardless of nits).

Test methodology

Modified code in node 0 worktree

Edited CMainParams() in src/kernel/chainparams.cpp to keep vSeeds (hardcoded DNS seeds) and vFixedSeeds empty.

Based on the condition in CConnman::GetFullOutboundConnCount() I added

0    if (pfrom.m_conn_type == ConnectionType::OUTBOUND_FULL_RELAY) {
1        LogPrintf("@@@@@@@@@@@@@@@@@@: successfully connected to %d!\n", pfrom.GetId());
2    }

right before

0    pfrom.fSuccessfullyConnected = true;

in PeerManagerImpl::ProcessMessage() (net_processing.cpp).

Also added

0Node::~CNode() {
1    LogPrintf("@@@@@@@@@@@@@@@@@@: Destroying %d!\n", this->id);
2}

Ran

Node 1

0$ ./src/bitcoind

This node has been run a few times, so it has a bunch of peers to share with node 0 later.

Node 0

Remove possible cached node data and use node 1 as the only -seednode.

0$ rm sidedir/peers.dat sidedir/anchors.dat
1$ ./src/bitcoind --datadir=sidedir -rpcport=32123 -bind=127.0.0.1:31212 -prune=600 -seednode=127.0.0.1

Confirmed log shows -seednode successfully providing enough nodes within 30s to skip DNS seed step without possible interference from fixed nodes, peers.dat or anchors.dat:

 02024-03-03T22:06:31Z dnsseed thread start
 12024-03-03T22:06:31Z addcon thread start
 22024-03-03T22:06:31Z -seednode enabled. Trying the provided seeds before defaulting to the dnsseeds.
 3...
 42024-03-03T22:06:31Z New addr-fetch v2 peer connected: version: 70016, blocks=833031, peer=0
 52024-03-03T22:06:31Z @@@@@@@@@@@@@@@@@@: Destroying 0!
 62024-03-03T22:06:31Z New outbound-full-relay v1 peer connected: version: 70016, blocks=833031, peer=1
 72024-03-03T22:06:31Z @@@@@@@@@@@@@@@@@@: successfully connected to 1!
 82024-03-03T22:06:32Z @@@@@@@@@@@@@@@@@@: Destroying 2!
 9...
102024-03-03T22:06:39Z New outbound-full-relay v1 peer connected: version: 70016, blocks=833031, peer=3
112024-03-03T22:06:39Z @@@@@@@@@@@@@@@@@@: successfully connected to 3!
12...
132024-03-03T22:06:39Z P2P peers available. Finished fetching data from seed nodes.
142024-03-03T22:06:39Z Skipping DNS seeds. Enough peers have been found
152024-03-03T22:06:39Z dnsseed thread exit

– So it took only 8s to get enough peers.

As a sanity-check, ran without -seednode:

0$ rm sidedir/peers.dat sidedir/anchors.dat
1$ ./src/bitcoind --datadir=sidedir -rpcport=32123 -bind=127.0.0.1:31212 -prune=600

And checked log to confirm that node 0 doesn’t somehow have yet another way to find peers.

 0...
 12024-03-03T22:38:26Z dnsseed thread start
 22024-03-03T22:38:26Z 0 addresses found from DNS seeds
 32024-03-03T22:38:26Z dnsseed thread exit
 42024-03-03T22:38:26Z addcon thread start
 52024-03-03T22:38:26Z net thread start
 62024-03-03T22:38:26Z msghand thread start
 72024-03-03T22:38:26Z opencon thread start
 82024-03-03T22:38:26Z init message: Done loading
 92024-03-03T22:39:27Z Adding fixed seeds as 60 seconds have passed and addrman is empty for at least one reachable network
102024-03-03T22:39:27Z Added 0 fixed seeds from reachable networks.

sr-gi force-pushed on Mar 7, 2024

sr-gi commented at 8:58 pm on March 7, 2024: member

Addressed comments from @cbergqvist

Future PR idea: -seednode does actually have a similar privacy issue to the hardcoded DNS seed nodes in that if a bitcoin.conf was shared around containing a fixed set of seednodes, that could make nodes easier to monitor. Another PR should probably actually de-prioritize -seednode in a similar way to increase privacy through preferring peers.dat and anchors.dat.

I like this idea. Actually, it’s not just if a config file is shared, in the current version of ThreadOpenConnections we will query all provided seednodes even if we don’t need to further populate our AddrMan, that could even signal when we have restarted our node, which is not good.

I’ll work in a follow-up to query seednodes only if we don’t have peers to connect to.

cbergqvist approved

cbergqvist commented at 3:56 pm on March 11, 2024: contributor

ACK 78482a09e06beb841e70781eb509c2b2fdea8bd9

Testing

Changed the fixed and DNS seeds to be empty in CMainParams() and re-added @@@-messages as described in comment above - #28016#pullrequestreview-1913140932.

Test 1

Passed (invalid) -seednode parameter to ensure that path is taken in the code.

 0$ rm sidedir/peers.dat sidedir/anchors.dat
 1$ ./src/bitcoind --datadir=sidedir -rpcport=32123 -bind=127.0.0.1:31212 -prune=600 -seednode=192.168.1.1
 2...
 32024-03-08T13:05:19Z dnsseed thread start
 42024-03-08T13:05:19Z addcon thread start
 52024-03-08T13:05:19Z -seednode enabled. Trying the provided seeds for 30 seconds before defaulting to the dnsseeds.
 62024-03-08T13:05:19Z net thread start
 72024-03-08T13:05:19Z msghand thread start
 82024-03-08T13:05:19Z init message: Done loading
 92024-03-08T13:05:19Z opencon thread start
102024-03-08T13:05:49Z Couldn't connect to enough peers via seed nodes. Handing fetch logic to the DNS seeds.
112024-03-08T13:05:49Z 0 addresses found from DNS seeds
122024-03-08T13:05:49Z dnsseed thread exit
132024-03-08T13:06:20Z Adding fixed seeds as 60 seconds have passed and addrman is empty for at least one reachable network
142024-03-08T13:06:20Z Added 0 fixed seeds from reachable networks.

Got expected output and node unable to find outbound peers.

Test 2

Passed valid -seednode to connect to and fetch addresses from local peer.

 0$ rm sidedir/peers.dat sidedir/anchors.dat
 1$ ./src/bitcoind --datadir=sidedir -rpcport=32123 -bind=127.0.0.1:31212 -prune=600 -seednode=127.0.0.1
 2...
 32024-03-11T15:34:55Z dnsseed thread start
 42024-03-11T15:34:55Z -seednode enabled. Trying the provided seeds for 30 seconds before defaulting to the dnsseeds.
 52024-03-11T15:34:55Z addcon thread start
 62024-03-11T15:34:55Z msghand thread start
 72024-03-11T15:34:55Z New addr-fetch v2 peer connected: version: 70016, blocks=834231, peer=0
 82024-03-11T15:34:55Z @@@@@@@@@@@@@@@@@@: Destroying 0!
 92024-03-11T15:35:24Z @@@@@@@@@@@@@@@@@@: Destroying 1!
102024-03-11T15:35:25Z Couldn't connect to enough peers via seed nodes. Handing fetch logic to the DNS seeds.
112024-03-11T15:35:25Z 0 addresses found from DNS seeds
122024-03-11T15:35:25Z dnsseed thread exit
132024-03-11T15:35:26Z @@@@@@@@@@@@@@@@@@: Destroying 2!
142024-03-11T15:35:32Z @@@@@@@@@@@@@@@@@@: Destroying 3!
152024-03-11T15:35:32Z New outbound-full-relay v1 peer connected: version: 70015, blocks=834232, peer=4
162024-03-11T15:35:32Z @@@@@@@@@@@@@@@@@@: successfully connected to 4!
172024-03-11T15:35:32Z Synchronizing blockheaders, height: 834232 (~100.00%)
182024-03-11T15:35:33Z UpdateTip: new best=000000000000000004740090b8fad58dbf02a554214464afbb96ed7f085276c2 height=404083 version=0x00000004 log2_work=84.357992 tx=118152631 date='2016-03-24T14:52:27Z' progress=0.120640 cache=1.1MiB(8915txo)

Did ~7 runs of this, not sure why it’s usually NOT able to connect to 2 valid peers before the 30 second timeout. Maybe it gets blocked because of disconnecting and reconnecting to nodes within a certain time-interval.

DrahtBot requested review from tdb3 on Mar 11, 2024

sr-gi commented at 7:55 pm on March 11, 2024: member

Did ~7 runs of this, not sure why it’s usually NOT able to connect to 2 valid peers before the 30 second timeout. Maybe it gets blocked because of disconnecting and reconnecting to nodes within a certain time-interval.

According to your logs, it doesn’t even look like any connection is attempted after the addr-fetch and defaulting to DNS seed. Are you sure your fetched node has any data to provide to the requester?

tdb3 commented at 10:50 pm on March 12, 2024: contributor

re ACK 78482a09e06beb841e70781eb509c2b2fdea8bd9 Re-built locally and ran all functional tests. All passed.

cbergqvist commented at 10:41 am on March 14, 2024: contributor

According to your logs, it doesn’t even look like any connection is attempted after the addr-fetch and defaulting to DNS seed. Are you sure your fetched node has any data to provide to the requester?

Ran with -debug=net and some extra logging around connection failure/success. Got a bunch of timing out (offline?) peers + one “No route to host (113)” overlapping with the 30s timeout for -seednode.

We only try one new connection at a time, waiting 5 seconds for timeout before moving on to the next. Might be good to at least try 2 connections in parallel?

Maybe there would be some way for the second node to better prioritize which addresses it propagates so they’re a bit less likely to fail. But also, maybe failures are to be expected if my two nodes are running behind the same external IP.

  02024-03-14T10:16:22Z init message: Starting network threads…
  12024-03-14T10:16:22Z dnsseed thread start
  22024-03-14T10:16:22Z -seednode enabled. Trying the provided seeds for 30 seconds before defaulting to the dnsseeds.
  32024-03-14T10:16:22Z opencon thread start
  42024-03-14T10:16:22Z net thread start
  52024-03-14T10:16:22Z addcon thread start
  62024-03-14T10:16:22Z init message: Done loading
  72024-03-14T10:16:22Z msghand thread start
  82024-03-14T10:16:22Z [net] trying v2 connection 127.0.0.1 lastseen=0.0hrs
  92024-03-14T10:16:22Z [net] Added connection peer=0
 102024-03-14T10:16:22Z @@@@@@@@@@@@@@@@@@: OpenNetworkConnection succeeded connecting to ::! id: 0
 112024-03-14T10:16:22Z [net] sending version (103 bytes) peer=0
 122024-03-14T10:16:22Z [net] send version message: version 70016, blocks=0, txrelay=1, peer=0
 132024-03-14T10:16:23Z [net] start sending v2 handshake to peer=0
 142024-03-14T10:16:23Z [net] received: version (103 bytes) peer=0
 152024-03-14T10:16:23Z [net] sending wtxidrelay (0 bytes) peer=0
 162024-03-14T10:16:23Z [net] sending sendaddrv2 (0 bytes) peer=0
 172024-03-14T10:16:23Z [net] sending verack (0 bytes) peer=0
 182024-03-14T10:16:23Z [net] sending getaddr (0 bytes) peer=0
 192024-03-14T10:16:23Z [net] receive version message: /Satoshi:26.99.0/: version 70016, blocks=834648, us=[::]:0, txrelay=1, peer=0
 202024-03-14T10:16:23Z [net] added time data, samples 2, offset +0 (+0 minutes)
 212024-03-14T10:16:23Z [net] received: wtxidrelay (0 bytes) peer=0
 222024-03-14T10:16:23Z [net] received: sendaddrv2 (0 bytes) peer=0
 232024-03-14T10:16:23Z [net] received: verack (0 bytes) peer=0
 242024-03-14T10:16:23Z New addr-fetch v2 peer connected: version: 70016, blocks=834648, peer=0
 252024-03-14T10:16:23Z [net] sending sendcmpct (9 bytes) peer=0
 262024-03-14T10:16:23Z [net] sending ping (8 bytes) peer=0
 272024-03-14T10:16:23Z [net] sending feefilter (8 bytes) peer=0
 282024-03-14T10:16:23Z [net] received: sendcmpct (9 bytes) peer=0
 292024-03-14T10:16:23Z [net] received: ping (8 bytes) peer=0
 302024-03-14T10:16:23Z [net] sending pong (8 bytes) peer=0
 312024-03-14T10:16:23Z [net] received: getheaders (1029 bytes) peer=0
 322024-03-14T10:16:23Z [net] Ignoring getheaders from peer=0 because active chain has too little work; sending empty response
 332024-03-14T10:16:23Z [net] sending headers (1 bytes) peer=0
 342024-03-14T10:16:23Z [net] received: feefilter (8 bytes) peer=0
 352024-03-14T10:16:23Z [net] received: feefilter of 0.00001000 BTC/kvB from peer=0
 362024-03-14T10:16:23Z [net] received: addrv2 (17493 bytes) peer=0
 372024-03-14T10:16:23Z [net] Received addr: 1000 addresses (1000 processed, 0 rate-limited) from peer=0
 382024-03-14T10:16:23Z [net] addrfetch connection completed peer=0; disconnecting
 392024-03-14T10:16:23Z [net] disconnecting peer=0
 402024-03-14T10:16:23Z [net] Cleared nodestate for peer=0
 412024-03-14T10:16:23Z @@@@@@@@@@@@@@@@@@: Destroying 0!
 422024-03-14T10:16:23Z [net] trying v1 connection 207.136.119.136:8333 lastseen=88.5hrs
 432024-03-14T10:16:28Z [net] connection attempt to 207.136.119.136:8333 timed out
 442024-03-14T10:16:28Z @@@@@@@@@@@@@@@@@@: Failed connecting to 207.136.119.136!
 452024-03-14T10:16:28Z [net] trying v1 connection 114.226.60.193:8333 lastseen=510.2hrs
 462024-03-14T10:16:33Z [net] connection attempt to 114.226.60.193:8333 timed out
 472024-03-14T10:16:33Z @@@@@@@@@@@@@@@@@@: Failed connecting to 114.226.60.193!
 482024-03-14T10:16:34Z [net] trying v1 connection 20.48.9.203:8333 lastseen=401.2hrs
 492024-03-14T10:16:39Z [net] connection attempt to 20.48.9.203:8333 timed out
 502024-03-14T10:16:39Z @@@@@@@@@@@@@@@@@@: Failed connecting to 20.48.9.203!
 512024-03-14T10:16:39Z [net] trying v1 connection 47.24.20.126:8333 lastseen=169.5hrs
 522024-03-14T10:16:44Z [net] connection attempt to 47.24.20.126:8333 timed out
 532024-03-14T10:16:44Z @@@@@@@@@@@@@@@@@@: Failed connecting to 47.24.20.126!
 542024-03-14T10:16:45Z [net] trying v1 connection 46.114.181.32:8333 lastseen=139.3hrs
 552024-03-14T10:16:50Z [net] connection attempt to 46.114.181.32:8333 timed out
 562024-03-14T10:16:50Z @@@@@@@@@@@@@@@@@@: Failed connecting to 46.114.181.32!
 572024-03-14T10:16:50Z [net] trying v1 connection 217.195.164.4:8333 lastseen=142.8hrs
 582024-03-14T10:16:52Z Couldn't connect to enough peers via seed nodes. Handing fetch logic to the DNS seeds.
 592024-03-14T10:16:52Z 0 addresses found from DNS seeds
 602024-03-14T10:16:52Z dnsseed thread exit
 612024-03-14T10:16:54Z [net] connect() to 217.195.164.4:8333 failed after wait: No route to host (113)
 622024-03-14T10:16:54Z @@@@@@@@@@@@@@@@@@: Failed connecting to 217.195.164.4!
 632024-03-14T10:16:54Z [net] trying v1 connection 82.135.80.45:8333 lastseen=467.3hrs
 642024-03-14T10:16:59Z [net] connection attempt to 82.135.80.45:8333 timed out
 652024-03-14T10:16:59Z @@@@@@@@@@@@@@@@@@: Failed connecting to 82.135.80.45!
 662024-03-14T10:17:00Z [net] trying v1 connection 78.94.29.85:8333 lastseen=164.0hrs
 672024-03-14T10:17:00Z [net] Added connection peer=1
 682024-03-14T10:17:00Z @@@@@@@@@@@@@@@@@@: OpenNetworkConnection succeeded connecting to 78.94.29.85! id: 1
 692024-03-14T10:17:00Z [net] sending version (103 bytes) peer=1
 702024-03-14T10:17:00Z [net] send version message: version 70016, blocks=0, txrelay=1, peer=1
 712024-03-14T10:17:00Z [net] trying v1 connection 174.51.168.163:8333 lastseen=644.8hrs
 722024-03-14T10:17:03Z [net] socket closed for peer=1
 732024-03-14T10:17:03Z [net] disconnecting peer=1
 742024-03-14T10:17:03Z [net] Cleared nodestate for peer=1
 752024-03-14T10:17:03Z @@@@@@@@@@@@@@@@@@: Destroying 1!
 762024-03-14T10:17:05Z [net] connection attempt to 174.51.168.163:8333 timed out
 772024-03-14T10:17:05Z @@@@@@@@@@@@@@@@@@: Failed connecting to 174.51.168.163!
 782024-03-14T10:17:06Z [net] trying v1 connection 185.130.44.59:8333 lastseen=136.9hrs
 792024-03-14T10:17:06Z [net] connect() to 185.130.44.59:8333 failed after wait: Connection refused (111)
 802024-03-14T10:17:06Z @@@@@@@@@@@@@@@@@@: Failed connecting to 185.130.44.59!
 812024-03-14T10:17:06Z [net] trying v1 connection 101.44.81.22:8333 lastseen=248.6hrs
 822024-03-14T10:17:11Z [net] connection attempt to 101.44.81.22:8333 timed out
 832024-03-14T10:17:11Z @@@@@@@@@@@@@@@@@@: Failed connecting to 101.44.81.22!
 842024-03-14T10:17:12Z [net] trying v1 connection 177.42.111.146:8333 lastseen=72.0hrs
 852024-03-14T10:17:17Z [net] connection attempt to 177.42.111.146:8333 timed out
 862024-03-14T10:17:17Z @@@@@@@@@@@@@@@@@@: Failed connecting to 177.42.111.146!
 872024-03-14T10:17:17Z [net] trying v1 connection 149.50.101.18:8333 lastseen=271.1hrs
 882024-03-14T10:17:17Z [net] Added connection peer=2
 892024-03-14T10:17:17Z @@@@@@@@@@@@@@@@@@: OpenNetworkConnection succeeded connecting to 149.50.101.18! id: 2
 902024-03-14T10:17:17Z [net] sending version (103 bytes) peer=2
 912024-03-14T10:17:17Z [net] send version message: version 70016, blocks=0, txrelay=1, peer=2
 922024-03-14T10:17:17Z [net] received: version (102 bytes) peer=2
 932024-03-14T10:17:17Z [net] sending wtxidrelay (0 bytes) peer=2
 942024-03-14T10:17:17Z [net] sending sendaddrv2 (0 bytes) peer=2
 952024-03-14T10:17:17Z [net] sending verack (0 bytes) peer=2
 962024-03-14T10:17:17Z [net] sending getaddr (0 bytes) peer=2
 972024-03-14T10:17:17Z [net] receive version message: /Satoshi:26.0.0/: version 70016, blocks=834648, us=84.216.185.21:51030, txrelay=1, peer=2
 982024-03-14T10:17:17Z [net] added time data, samples 3, offset +0 (+0 minutes)
 992024-03-14T10:17:17Z [net] received: wtxidrelay (0 bytes) peer=2
1002024-03-14T10:17:17Z [net] received: sendaddrv2 (0 bytes) peer=2
1012024-03-14T10:17:17Z [net] received: verack (0 bytes) peer=2
1022024-03-14T10:17:17Z New outbound-full-relay v1 peer connected: version: 70016, blocks=834648, peer=2
1032024-03-14T10:17:17Z [net] sending sendcmpct (9 bytes) peer=2
1042024-03-14T10:17:17Z @@@@@@@@@@@@@@@@@@: successfully connected to 2!
1052024-03-14T10:17:17Z [net] sending ping (8 bytes) peer=2
1062024-03-14T10:17:17Z [net] sending getheaders (69 bytes) peer=2
1072024-03-14T10:17:17Z [net] initial getheaders (0) to peer=2 (startheight:834648)
1082024-03-14T10:17:17Z [net] sending feefilter (8 bytes) peer=2
1092024-03-14T10:17:17Z [net] received: sendcmpct (9 bytes) peer=2
1102024-03-14T10:17:17Z [net] received: ping (8 bytes) peer=2
1112024-03-14T10:17:17Z [net] sending pong (8 bytes) peer=2
1122024-03-14T10:17:17Z [net] received: getheaders (1029 bytes) peer=2
1132024-03-14T10:17:17Z [net] Ignoring getheaders from peer=2 because active chain has too little work; sending empty response
1142024-03-14T10:17:17Z [net] sending headers (1 bytes) peer=2
1152024-03-14T10:17:17Z [net] received: feefilter (8 bytes) peer=2
1162024-03-14T10:17:17Z [net] received: feefilter of 0.00001000 BTC/kvB from peer=2
1172024-03-14T10:17:17Z [net] received: addrv2 (17699 bytes) peer=2
1182024-03-14T10:17:17Z [net] Received addr: 1000 addresses (1000 processed, 0 rate-limited) from peer=2
1192024-03-14T10:17:17Z [net] received: pong (8 bytes) peer=2
1202024-03-14T10:17:17Z [net] received: headers (162003 bytes) peer=2
1212024-03-14T10:17:17Z [net] Initial headers sync started with peer=2: height=0, max_commitments=4746654, min_work=000000000000000000000000000000000000000052b2559353df4117b7348b64
1222024-03-14T10:17:17Z [net] sending getheaders (101 bytes) peer=2
1232024-03-14T10:17:17Z [net] more getheaders (from 00000000dfd5d65c9d8561b4b8f60a63018fe3933ecb131fb37f905f87da951a) to peer=2
1242024-03-14T10:17:17Z Pre-synchronizing blockheaders, height: 2000 (~0.25%)

in src/net.cpp:2174 in 78482a09e0 outdated

2184@@ -2185,11 +2185,36 @@ void CConnman::WakeMessageHandler()
2185 
2186 void CConnman::ThreadDNSAddressSeed()
2187 {
2188+    constexpr int TARGET_OUTBOUND_CONNECTIONS = 2;
2189+    int outbound_connection_count = 0;
2190+
2191+    auto start = NodeClock::now();
2192+    if (gArgs.IsArgSet("-seednode")) {

davidgumberg commented at 8:13 pm on March 15, 2024:

Nit:

0    if (gArgs.IsArgSet("-seednode")) {
1        auto start = NodeClock::now();

davidgumberg commented at 9:07 pm on March 15, 2024:

I’m open to being wrong about this if you think start may be useful outside of the -seednode logic. If we refactored the dns seed seeds_wait_time sleep loop to resemble this part, maybe we’d want to reinitialize the variable anyways:

0  const std::chrono::seconds seeds_wait_time = (addrman.Size() >= DNSSEEDS_DELAY_PEER_THRESHOLD ? DNSSEEDS_DELAY_MANY_PEERS : DNSSEEDS_DELAY_FEW_PEERS);
1+ auto start = NodeClock::now();

sr-gi commented at 3:38 pm on March 18, 2024:

I think this makes sense. I wouldn’t repurpose seeds_wait_time though. Despite both being chrono::seconds, they are semantically different (one is supposed to be a timestamp, while the other is a delta to be added to a timestamp, so even though we may simplify one variable, the code may be harder to read)

DrahtBot requested review from davidgumberg on Mar 15, 2024

davidgumberg commented at 10:14 pm on March 15, 2024: contributor

ACK https://github.com/bitcoin/bitcoin/commit/78482a09e06beb841e70781eb509c2b2fdea8bd9

I ran into this issue with the existing behavior when testing -seednode over v2. To state more explicitly the problem with allowing both to run in parallel, it seems to me that populating from the dns seeds will almost always be faster than the time it takes to handshake and getaddr from a -seednode

Testing with a fresh signet node:

0$ bitcoind -debug=net -signet -seednode=bitcoin.achow101.com:38333 -datadir=/tmp/28016/

On master

0$ bitcoind -debug=net -signet -seednode=bitcoin.achow101.com:38333 -datadir=/tmp/28016/

 02024-03-15T21:22:57Z init message: Starting network threads…
 12024-03-15T21:22:57Z net thread start
 22024-03-15T21:22:57Z addcon thread start
 32024-03-15T21:22:57Z dnsseed thread start
 42024-03-15T21:22:57Z Loading addresses from DNS seed seed.signet.bitcoin.sprovoost.nl.
 52024-03-15T21:22:57Z opencon thread start
 62024-03-15T21:22:57Z [net] trying v2 connection bitcoin.achow101.com:38333 lastseen=0.0hrs
 72024-03-15T21:22:57Z init message: Done loading
 82024-03-15T21:22:57Z msghand thread start
 92024-03-15T21:22:57Z Loading addresses from DNS seed 178.128.221.177
102024-03-15T21:22:57Z Loading addresses from DNS seed v7ajjeirttkbnt32wpy3c6w3emwnfr3fkla7hpxcfokr3ysd3kqtzmqd.onion:38333
112024-03-15T21:22:57Z 34 addresses found from DNS seeds
122024-03-15T21:22:57Z dnsseed thread exit
132024-03-15T21:22:57Z [net] Added connection peer=0
142024-03-15T21:22:57Z [net] sending version (103 bytes) peer=0
152024-03-15T21:22:57Z [net] send version message: version 70016, blocks=0, txrelay=1, peer=0
162024-03-15T21:22:57Z [net] start sending v2 handshake to peer=0
172024-03-15T21:22:57Z [net] received: version (103 bytes) peer=0
182024-03-15T21:22:57Z [net] sending wtxidrelay (0 bytes) peer=0
192024-03-15T21:22:57Z [net] sending sendaddrv2 (0 bytes) peer=0
202024-03-15T21:22:57Z [net] sending verack (0 bytes) peer=0
212024-03-15T21:22:57Z [net] sending getaddr (0 bytes) peer=0
222024-03-15T21:22:57Z [net] receive version message: /Satoshi:27.99.0/: version 70016, blocks=186938, us=192.12.14.3:16482, txrelay=1, peer=0
232024-03-15T21:22:57Z [net] added time data, samples 2, offset +0 (+0 minutes)
242024-03-15T21:22:57Z [net] received: wtxidrelay (0 bytes) peer=0
252024-03-15T21:22:57Z [net] received: sendaddrv2 (0 bytes) peer=0
262024-03-15T21:22:57Z [net] received: verack (0 bytes) peer=0
272024-03-15T21:22:57Z New addr-fetch v2 peer connected: version: 70016, blocks=186938, peer=0
282024-03-15T21:22:57Z [net] sending sendcmpct (9 bytes) peer=0
292024-03-15T21:22:57Z [net] sending ping (8 bytes) peer=0
302024-03-15T21:22:57Z [net] sending feefilter (8 bytes) peer=0
312024-03-15T21:22:57Z [net] received: sendcmpct (9 bytes) peer=0
322024-03-15T21:22:57Z [net] received: ping (8 bytes) peer=0
332024-03-15T21:22:57Z [net] sending pong (8 bytes) peer=0
342024-03-15T21:22:57Z [net] received: getheaders (965 bytes) peer=0
352024-03-15T21:22:57Z [net] Ignoring getheaders from peer=0 because active chain has too little work; sending empty response
362024-03-15T21:22:57Z [net] sending headers (1 bytes) peer=0
372024-03-15T21:22:57Z [net] received: feefilter (8 bytes) peer=0
382024-03-15T21:22:57Z [net] received: feefilter of 0.00001000 BTC/kvB from peer=0
392024-03-15T21:22:57Z [net] received: addrv2 (19609 bytes) peer=0
402024-03-15T21:22:57Z [net] Received addr: 1000 addresses (1000 processed, 0 rate-limited) from peer=0
412024-03-15T21:22:57Z [net] addrfetch connection completed peer=0; disconnecting
422024-03-15T21:22:57Z [net] disconnecting peer=0
432024-03-15T21:22:57Z [net] Cleared nodestate for peer=0

On this branch:

0$ bitcoind -signet -seednode=bitcoin.achow101.com:38333 -datadir=/tmp/28016/

 02024-03-15T21:31:46Z -seednode enabled. Trying the provided seeds for 30 seconds before defaulting to the dnsseeds.
 12024-03-15T21:31:46Z addcon thread start
 22024-03-15T21:31:46Z opencon thread start
 32024-03-15T21:31:46Z init message: Done loading
 42024-03-15T21:31:46Z [net] trying v2 connection bitcoin.achow101.com:38333 lastseen=0.0hrs
 52024-03-15T21:31:46Z msghand thread start
 62024-03-15T21:31:46Z [net] Added connection peer=0
 72024-03-15T21:31:46Z [net] sending version (103 bytes) peer=0
 82024-03-15T21:31:46Z [net] send version message: version 70016, blocks=0, txrelay=1, peer=0
 92024-03-15T21:31:46Z [net] start sending v2 handshake to peer=0
102024-03-15T21:31:46Z [net] received: version (103 bytes) peer=0
112024-03-15T21:31:46Z [net] sending wtxidrelay (0 bytes) peer=0
122024-03-15T21:31:46Z [net] sending sendaddrv2 (0 bytes) peer=0
132024-03-15T21:31:46Z [net] sending verack (0 bytes) peer=0
142024-03-15T21:31:46Z [net] sending getaddr (0 bytes) peer=0
152024-03-15T21:31:46Z [net] receive version message: /Satoshi:27.99.0/: version 70016, blocks=186940, us=192.12.14.3:44748, txrelay=1, peer=0
162024-03-15T21:31:46Z [net] added time data, samples 2, offset +0 (+0 minutes)
172024-03-15T21:31:46Z [net] received: wtxidrelay (0 bytes) peer=0
182024-03-15T21:31:46Z [net] received: sendaddrv2 (0 bytes) peer=0
192024-03-15T21:31:46Z [net] received: verack (0 bytes) peer=0
202024-03-15T21:31:46Z New addr-fetch v2 peer connected: version: 70016, blocks=186940, peer=0
212024-03-15T21:31:46Z [net] sending sendcmpct (9 bytes) peer=0
222024-03-15T21:31:46Z [net] sending ping (8 bytes) peer=0
232024-03-15T21:31:46Z [net] sending feefilter (8 bytes) peer=0
242024-03-15T21:31:46Z [net] received: sendcmpct (9 bytes) peer=0
252024-03-15T21:31:46Z [net] received: ping (8 bytes) peer=0
262024-03-15T21:31:46Z [net] sending pong (8 bytes) peer=0
272024-03-15T21:31:46Z [net] received: getheaders (965 bytes) peer=0
282024-03-15T21:31:46Z [net] Ignoring getheaders from peer=0 because active chain has too little work; sending empty response
292024-03-15T21:31:46Z [net] sending headers (1 bytes) peer=0
302024-03-15T21:31:46Z [net] received: feefilter (8 bytes) peer=0
312024-03-15T21:31:46Z [net] received: feefilter of 0.00001000 BTC/kvB from peer=0
322024-03-15T21:31:46Z [net] received: addrv2 (19594 bytes) peer=0
332024-03-15T21:31:46Z [net] Received addr: 999 addresses (999 processed, 0 rate-limited) from peer=0
342024-03-15T21:31:46Z [net] addrfetch connection completed peer=0; disconnecting
352024-03-15T21:31:46Z [net] disconnecting peer=0
362024-03-15T21:31:46Z [net] Cleared nodestate for peer=0
372024-03-15T21:31:47Z [net] trying v1 connection [2620:0:e00:400f::2c5]:38333 lastseen=196.1hrs
382024-03-15T21:31:47Z [net] connect() to [2620:0:e00:400f::2c5]:38333 failed: Network is unreachable (101)
392024-03-15T21:31:47Z [net] trying v1 connection [2a01:4f9:3a:2dd2::2]:38333 lastseen=15.0hrs
40# [Later...]
412024-03-15T21:31:55Z P2P peers available. Finished fetching data from seed nodes.
422024-03-15T21:31:55Z Skipping DNS seeds. Enough peers have been found
432024-03-15T21:31:55Z dnsseed thread exit

sr-gi commented at 3:36 pm on March 18, 2024: member

We only try one new connection at a time, waiting 5 seconds for timeout before moving on to the next. Might be good to at least try 2 connections in parallel?

I’m not too familiar with the underlying logic for making the actual connection, but the higher level logic is designed around always having a max number of connections, that is: there is logic for filling the connections slots, and the logic to maintain, evict, and favor different kind of connections once the limit is reached. I don’t think there is a real rush to fill the outbound connection slots, it happens rather quickly under normal conditions (with a populated addrman), so the case that it may take slightly longer on the first bootstrap shouldn’t be an issue.

Maybe there would be some way for the second node to better prioritize which addresses it propagates so they’re a bit less likely to fail. But also, maybe failures are to be expected if my two nodes are running behind the same external IP.

I don’t think this would be a good idea. The collection of addresses that other peers feed you when requested is purposely “untested” (as in, not giving you information of whether they are good, or not giving you only good addresses is a feature). The reason for that is that if a seed only provides valid/tested addresses, you could tell what peers the node is connected to/has been connected to, which is undesirable

sr-gi force-pushed on Mar 18, 2024

sr-gi commented at 3:40 pm on March 18, 2024: member

Addresses review comments by @davidgumberg

in src/net.cpp:2171 in 2842e51a24 outdated

2184@@ -2185,11 +2185,36 @@ void CConnman::WakeMessageHandler()
2185 
2186 void CConnman::ThreadDNSAddressSeed()
2187 {
2188+    constexpr int TARGET_OUTBOUND_CONNECTIONS = 2;

cbergqvist commented at 9:36 pm on March 18, 2024:

Sorry for the late realization, but maybe this should be

0    const int target_outbound_connections = std::clamp(args.GetIntArg("-maxconnections", DEFAULT_MAX_PEER_CONNECTIONS), 1, 2);

This would be done in order to minimize unnecessarily waiting for 30s if we don’t allow more than 1 connection, but simultaneously wait for 2.

Make it at least 1 since we shouldn’t get here if maxconnections is <= 0 as that would turn off -dnsseed and ThreadDNSAddressSeed().

sr-gi commented at 2:58 pm on March 19, 2024:

I kind of agree, even though that was already the case before this PR.

I’d like to hear your thoughts on this @mzumsande

mzumsande commented at 7:33 pm on March 28, 2024:

I’d like to hear your thoughts on this @mzumsande

I think it doesn’t really make sense to try and improve the user experience of -maxconnections=1 - doing that is stupid and dangerous, it’s basically begging to be eclipsed. If someone wants just one peer they should use -connect to at least pick one that they trust. It has been suggested before to disable a very low -maxconnections or at least print out a big warning.

cbergqvist commented at 10:58 pm on March 28, 2024:

@mzumsande thanks for your informed opinion! @sr-gi will try to go through the latest version in the next few days.

itornaza commented at 5:13 pm on March 23, 2024: none

tested ACK 2842e51a246b162a586941184b7694f187d7aee7

Standard tests

Did a code review
Configured with --with-incompatible-bdb and --enable-suppress-external-warnings
Run unit tests with make check and all tests pass
Run all functional tests with no extra flags and all tests pass
Run all functional tests with --extended and all tests pass

Manual functional test

Repeated a similar manual functional testing methodology to @davidgumberg and @cbergqvist on both testnet and signet, and again the expected behavior described in this PR is confirmed. For brevity I only include the testnet examples.

Test case 1: Valid seed node

Removed the peers.dat, anchors.dat and debug.log to start on a clean state with no known peers.
Run bitcoind with the following parameters set in bitcoin.conf for a run with a valid seed node.

0testnet=1
1seednode=testnet-seed.bitcoin.jonasschnelli.ch
2debug=net

 02024-03-23T13:30:08Z Bitcoin Core version v26.99.0-2842e51a246b (release build)
 12024-03-23T13:30:08Z Config file arg: debug="net"
 22024-03-23T13:30:08Z Config file arg: loglevel="debug"
 32024-03-23T13:30:08Z Config file arg: seednode="testnet-seed.bitcoin.jonasschnelli.ch"
 42024-03-23T13:30:08Z Config file arg: testnet="1"
 5...
 62024-03-23T13:30:15Z net thread start
 72024-03-23T13:30:15Z dnsseed thread start
 82024-03-23T13:30:15Z opencon thread start
 9**2024-03-23T13:30:15Z -seednode enabled. Trying the provided seeds for 30 seconds before defaulting to the dnsseeds.**
102024-03-23T13:30:15Z init message: Done loading
112024-03-23T13:30:15Z Progress loading mempool transactions from disk: 71% (tried 5, 2 remaining)
12**2024-03-23T13:30:15Z [net] trying v2 connection testnet-seed.bitcoin.jonasschnelli.ch lastseen=0.0hrs**
132024-03-23T13:30:15Z addcon thread start
142024-03-23T13:30:15Z msghand thread start
152024-03-23T13:30:15Z Progress loading mempool transactions from disk: 85% (tried 6, 1 remaining)
162024-03-23T13:30:15Z Imported mempool transactions from disk: 7 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
172024-03-23T13:30:15Z initload thread exit
182024-03-23T13:30:15Z [net] Added connection peer=0
192024-03-23T13:30:15Z [net] sending version (103 bytes) peer=0
202024-03-23T13:30:15Z [net] send version message: version 70016, blocks=2582929, txrelay=1, peer=0
212024-03-23T13:30:15Z [net] start sending v2 handshake to peer=0
222024-03-23T13:30:15Z [net] socket closed for peer=0
232024-03-23T13:30:15Z [net] disconnecting peer=0
242024-03-23T13:30:15Z [net] retrying with v1 transport protocol for peer=0
252024-03-23T13:30:15Z [net] Cleared nodestate for peer=0
262024-03-23T13:30:16Z [net] trying v1 connection testnet-seed.bitcoin.jonasschnelli.ch lastseen=0.0hrs
272024-03-23T13:30:16Z [net] Added connection peer=1
282024-03-23T13:30:16Z [net] sending version (103 bytes) peer=1
292024-03-23T13:30:16Z [net] send version message: version 70016, blocks=2582929, txrelay=1, peer=1
302024-03-23T13:30:16Z [net] received: version (102 bytes) peer=1
312024-03-23T13:30:16Z [net] sending wtxidrelay (0 bytes) peer=1
322024-03-23T13:30:16Z [net] sending sendaddrv2 (0 bytes) peer=1
332024-03-23T13:30:16Z [net] sending verack (0 bytes) peer=1
342024-03-23T13:30:16Z [net] sending getaddr (0 bytes) peer=1
352024-03-23T13:30:16Z [net] receive version message: /Satoshi:25.1.0/: version 70016, blocks=2583188, us=185.51.134.196:54427, txrelay=1, peer=1
362024-03-23T13:30:16Z [net] added time data, samples 2, offset +0 (+0 minutes)
372024-03-23T13:30:16Z [net] received: wtxidrelay (0 bytes) peer=1
382024-03-23T13:30:16Z [net] received: sendaddrv2 (0 bytes) peer=1
392024-03-23T13:30:16Z [net] received: verack (0 bytes) peer=1
402024-03-23T13:30:16Z New addr-fetch v1 peer connected: version: 70016, blocks=2583188, peer=1
41...

Test case 2: Invalid seed node

Again, removed the peers.dat, anchors.dat and debug.log to start on a clean state with no known peers.
Run bitcoind with the following parameters set in bitcoin.conf for a run with an invalid seed node.

0testnet=1
1seednode=192.168.1.46
2debug=net

 02024-03-23T13:33:59Z Bitcoin Core version v26.99.0-2842e51a246b (release build)
 1...
 22024-03-23T13:33:59Z Config file arg: debug="net"
 32024-03-23T13:33:59Z Config file arg: loglevel="debug"
 42024-03-23T13:33:59Z Config file arg: seednode="192.168.1.46"
 52024-03-23T13:33:59Z Config file arg: testnet="1"
 6...
 72024-03-23T13:34:06Z net thread start
 82024-03-23T13:34:06Z addcon thread start
 92024-03-23T13:34:06Z dnsseed thread start
102024-03-23T13:34:06Z opencon thread start
112024-03-23T13:34:06Z init message: Done loading
122024-03-23T13:34:06Z -seednode enabled. Trying the provided seeds for 30 seconds before defaulting to the dnsseeds.
132024-03-23T13:34:06Z [net] trying v2 connection 192.168.1.46 lastseen=0.0hrs
142024-03-23T13:34:06Z msghand thread start
152024-03-23T13:34:06Z [net] connect() to 192.168.1.46:18333 failed after wait: Connection refused (61)
16...
17...           ***  the process waits for 30s until handing over to DNS seed ***
18...           ***  as is evident by the previous and next timestamps     ***
19...
202024-03-23T13:34:36Z Couldn't connect to enough peers via seed nodes. Handing fetch logic to the DNS seeds.
21...
222024-03-23T13:34:36Z Loading addresses from DNS seed testnet-seed.bitcoin.jonasschnelli.ch.
232024-03-23T13:34:36Z Loading addresses from DNS seed testnet-seed.bluematt.me.
242024-03-23T13:34:36Z Loading addresses from DNS seed seed.testnet.bitcoin.sprovoost.nl.
252024-03-23T13:34:36Z Loading addresses from DNS seed seed.tbtc.petertodd.net.
262024-03-23T13:34:36Z 46 addresses found from DNS seeds
272024-03-23T13:34:36Z dnsseed thread exit
28...

DrahtBot requested review from cbergqvist on Mar 23, 2024

DrahtBot requested review from tdb3 on Mar 23, 2024

mzumsande commented at 7:38 pm on March 28, 2024: contributor

I wonder if it wouldn’t be simpler to just disable dns seeds in InitParameterInteraction if -seednode was specified:

My main use case for -seenode would be as a backup option to use if I thought that there was something wrong with the dns seeds, e.g. if I thought they might be compromised or not working. In that case, having the DNS seeds as a backup option for seednode wouldn’t really make sense: I’d rather not find any peers and be able to manually select another seednode if the -seednode peer I specified wasn’t available.

In general, results from working dns seeds will be of better quality (in terms of percentage of peers you can actually connect to), because those specifically return peers that were reachable during the last minutes / hours, whereas GETADDR / ADDR does not filter for that.

I’m not sure about other people’s use case though, so I wonder if there is a common use case for DNS seeds being backup to -seednode?

sr-gi commented at 7:42 am on April 2, 2024: member

I wonder if it wouldn’t be simpler to just disable dns seeds in InitParameterInteraction if -seednode was specified

I’m not sure about other people’s use case though, so I wonder if there is a common use case for DNS seeds being backup to -seednode?

Yeah, me neither to be honest. My intuition is that if a non-default config is provided (and a default is not disabled), the user may want to give priority over the manual default, but still use the default as a last resort.

Given dns seeds are usually faster feeding addresses than seednodes are (due to a dns query vs a p2p oneshot) I leaned towards the current approach.

davidgumberg commented at 5:18 pm on April 4, 2024: contributor

reACK https://github.com/bitcoin/bitcoin/commit/2842e51a246b162a586941184b7694f187d7aee7

I think the 30 second dns seed fallback condition here is more likely than I assumed because of the low quality of P2P gossip addresses mentioned by mzumsande above. I experienced it on my first test of this branch on signet and decided to investigate:

In my testing, 6 out of 12 attempts to bootstrap a signet node with a -seednode fell back to DNS seeds, and 7 out of 12 attempts to bootstrap a mainnet node fell back to DNS seeds.

It’s a bit difficult for me to imagine a use case where someone would want to use -seednode without -dnsseed=0, but I feel disabling dns seeds / fixed seeds when the seednode argument is used would be surprising behavior.

Balancing the surprise of using -seednode and your node completely failing to bootstrap against using -seednode and having your node fall back to dns seeds / fixed seeds, I think this PR has it right since if our seed node is up, we at least populate our addrman with addresses from the seed node, mitigating some of the risks someone concerned about the DNS seeds would have, but I don’t feel strongly one way or the other.

tdb3 commented at 1:29 pm on April 6, 2024: contributor

re-ACK for 2842e51a246b162a586941184b7694f187d7aee7

The code adjustments made to bound scope and increase declaration/usage proximity look great to me (cleaner, more readable). Overall, I think the approach of this PR (prioritize seednodes before dnsseeds, then use dnsseeds if seednode usage was unsuccessful) is appropriate. Although subjective, overall I would consider node network reachability/health a bit more important than privacy. The parameter descriptions seem clear enough to enable a user to enhance privacy (e.g. disable dnsseed).

Built and ran all functionals (all passed).

Cleared datadir (with the exception of bitcoin.conf, chainstate, and blocks). Performed >1000 block download on signet to a reachable public node specified as a seednode. dnsseeds seemed to be avoided as expected.

 02024-04-06T13:15:52Z dnsseed thread start
 12024-04-06T13:15:52Z -seednode enabled. Trying the provided seeds for 30 seconds before defaulting to the dnsseeds.
 2...
 32024-04-06T13:15:52Z New addr-fetch v2 peer connected: version: 70016, blocks=190136, peer=0
 42024-04-06T13:15:59Z New outbound-full-relay v1 peer connected: version: 70016, blocks=190136, peer=1
 52024-04-06T13:16:00Z Synchronizing blockheaders, height: 190136 (~100.00%)
 62024-04-06T13:16:00Z UpdateTip: new best=00000104b1e879664a5b1706ef903e9647f6d156b67b2949bd17a7cc4e32c535 height=189083 version=0x20000000 log2_work=41.093842 tx=3272864 date='2024-03-30T12:13:51Z' progress=0.999366 cache=0.3MiB(1314txo)
 7...
 82024-04-06T13:16:17Z UpdateTip: new best=0000001d2abdd48d80cb96fd4b2d36625af36ea3abb441069b05cb8c22d9ea6c height=190136 version=0x20000000 log2_work=41.102275 tx=3313628 date='2024-04-06T13:14:43Z' progress=1.000000 cache=13.0MiB(95511txo)
 92024-04-06T13:16:17Z New outbound-full-relay v1 peer connected: version: 70016, blocks=190136, peer=3
102024-04-06T13:16:17Z P2P peers available. Finished fetching data from seed nodes.
112024-04-06T13:16:17Z Skipping DNS seeds. Enough peers have been found
122024-04-06T13:16:17Z dnsseed thread exit

Cleared datadir again and performed test with a known invalid seednode specified (i.e. purposefully specified an invalid port node for the previous public node used). The invalid seednode was tried for 30 seconds, then dnsseeds were used (as expected).

 02024-04-06T13:23:50Z dnsseed thread start
 12024-04-06T13:23:50Z addcon thread start
 22024-04-06T13:23:50Z -seednode enabled. Trying the provided seeds for 30 seconds before defaulting to the dnsseeds.
 32024-04-06T13:23:50Z opencon thread start
 42024-04-06T13:23:50Z msghand thread start
 52024-04-06T13:23:50Z init message: Done loading
 62024-04-06T13:24:20Z Couldn't connect to enough peers via seed nodes. Handing fetch logic to the DNS seeds.
 72024-04-06T13:24:20Z Loading addresses from DNS seed v7ajjeirttkbnt32wpy3c6w3emwnfr3fkla7hpxcfokr3ysd3kqtzmqd.onion:38333
 82024-04-06T13:24:20Z Loading addresses from DNS seed seed.signet.bitcoin.sprovoost.nl.
 92024-04-06T13:24:20Z Cannot create socket for v7ajjeirttkbnt32wpy3c6w3emwnfr3fkla7hpxcfokr3ysd3kqtzmqd.onion:38333: unsupported network
102024-04-06T13:24:21Z Loading addresses from DNS seed 178.128.221.177
112024-04-06T13:24:21Z 34 addresses found from DNS seeds
122024-04-06T13:24:21Z dnsseed thread exit
132024-04-06T13:24:22Z New outbound-full-relay v1 peer connected: version: 70016, blocks=190138, peer=0
142024-04-06T13:24:22Z Leaving InitialBlockDownload (latching to false)
152024-04-06T13:24:44Z New outbound-full-relay v1 peer connected: version: 70016, blocks=190138, peer=1
162024-04-06T13:24:49Z New outbound-full-relay v1 peer connected: version: 70016, blocks=190138, peer=2
172024-04-06T13:24:55Z New outbound-full-relay v1 peer connected: version: 70016, blocks=190138, peer=3
182024-04-06T13:24:56Z New outbound-full-relay v1 peer connected: version: 70016, blocks=190138, peer=4
192024-04-06T13:24:56Z New outbound-full-relay v1 peer connected: version: 70016, blocks=190138, peer=5

cbergqvist commented at 1:11 pm on April 8, 2024: contributor

ACK 2842e51a246b162a586941184b7694f187d7aee7

Diffed top 2 commits in that and 78482a09e06beb841e70781eb509c2b2fdea8bd9 which I previously tested & acked. Only scope of start-variable was narrowed as suggested by @davidgumberg in #28016 (review).

DrahtBot added the label Needs rebase on Apr 22, 2024

Gives seednode priority over dnsseed if both are provided 3120a4678a

Added seednode prioritization message to help output 82f41d76f1

sr-gi force-pushed on Apr 22, 2024

sr-gi commented at 6:08 pm on April 22, 2024: member

Rebased to address merge conflicts

DrahtBot removed the label Needs rebase on Apr 22, 2024

DrahtBot added the label CI failed on Apr 22, 2024

DrahtBot removed the label CI failed on Apr 23, 2024

davidgumberg commented at 5:13 pm on April 28, 2024: contributor

untested reACK https://github.com/bitcoin/bitcoin/commit/82f41d76f1c6ad38290917dad5499ffbe6b3974d

cbergqvist approved

cbergqvist commented at 10:30 pm on April 28, 2024: contributor

ACK 82f41d76f1c6ad38290917dad5499ffbe6b3974d

After checking out ran: git range-diff 2842e51~2..2842e51 HEAD~2..HEAD Functional including --extended tests passed (skipped unrelated feature_dbcrash).

itornaza commented at 4:05 pm on April 29, 2024: none

tested re-ACK 82f41d76f1c6ad38290917dad5499ffbe6b3974d

This PR not only adds meaningful functionality by enabling seeds that are provided by the user to take precedence over the default DNS seeds, but also enhances user experience with relevant reporting on what exactly happens through the process of connecting to the P2P network and which type of nodes are effectively used.

Built this PR from a cleaned up environment using make distclean and checked out the latest commit to test upon. Manually examined the differences using git difftool to inspect the code two commits back and it looks good to me.

I also run all unit, functional, extended and manual tests exactly as I described them in my previous #28016 (comment) and all pass.

achow101 commented at 10:51 pm on April 30, 2024: member

ACK 82f41d76f1c6ad38290917dad5499ffbe6b3974d

achow101 merged this on Apr 30, 2024

achow101 closed this on Apr 30, 2024

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

Code Coverage

Reviews

Conflicts

Test methodology

Modified code in node 0 worktree

Ran

Node 1

Node 0

Testing

Test 1

Test 2

Standard tests

Manual functional test

Test case 1: Valid seed node

Test case 2: Invalid seed node