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

pull sr-gi wants to merge 2 commits into bitcoin:master from sr-gi:dnsseed-priority changing 3 files +126 −87
  1. 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

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

  3. DrahtBot added the label P2P on Jun 30, 2023
  4. 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)
  5. sr-gi force-pushed on Jun 30, 2023
  6. 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
  7. sr-gi force-pushed on Jun 30, 2023
  8. DrahtBot added the label CI failed on Jun 30, 2023
  9. DrahtBot removed the label CI failed on Jun 30, 2023
  10. 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…
  11. DrahtBot added the label CI failed on Jul 30, 2023
  12. DrahtBot removed the label CI failed on Aug 1, 2023
  13. DrahtBot added the label Needs rebase on Sep 14, 2023
  14. 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

  15. sr-gi force-pushed on Sep 14, 2023
  16. DrahtBot removed the label Needs rebase on Sep 14, 2023
  17. 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…

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

  19. DrahtBot added the label CI failed on Sep 29, 2023
  20. DrahtBot removed the label CI failed on Oct 4, 2023
  21. sr-gi force-pushed on Oct 4, 2023
  22. DrahtBot added the label CI failed on Oct 8, 2023
  23. DrahtBot removed the label CI failed on Oct 16, 2023
  24. DrahtBot added the label CI failed on Jan 13, 2024
  25. sr-gi force-pushed on Jan 29, 2024
  26. sr-gi commented at 10:07 pm on January 29, 2024: member
    Rebased to fix CI
  27. DrahtBot removed the label CI failed on Jan 30, 2024
  28. DrahtBot added the label Needs rebase on Jan 31, 2024
  29. sr-gi force-pushed on Feb 2, 2024
  30. DrahtBot removed the label Needs rebase on Feb 2, 2024
  31. 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;
    
  32. 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

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

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

  35. 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.
  36. 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

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

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

  39. 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
  40. tdb3 commented at 10:58 pm on March 2, 2024: contributor
    re ACK for 87be4abd33eb6c539a5f953db2f4be3248a86dd0
  41. 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.

  42. 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
    
  43. cbergqvist approved
  44. 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.
    
  45. sr-gi force-pushed on Mar 7, 2024
  46. 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.

  47. cbergqvist approved
  48. 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.

  49. DrahtBot requested review from tdb3 on Mar 11, 2024
  50. 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?

  51. tdb3 commented at 10:50 pm on March 12, 2024: contributor
    re ACK 78482a09e06beb841e70781eb509c2b2fdea8bd9 Re-built locally and ran all functional tests. All passed.
  52. 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%)
    
  53. 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)
  54. DrahtBot requested review from davidgumberg on Mar 15, 2024
  55. 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
    
  56. 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

  57. sr-gi force-pushed on Mar 18, 2024
  58. sr-gi commented at 3:40 pm on March 18, 2024: member
    Addresses review comments by @davidgumberg
  59. 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.
  60. 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...
    
  61. DrahtBot requested review from cbergqvist on Mar 23, 2024
  62. DrahtBot requested review from tdb3 on Mar 23, 2024
  63. 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?

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

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

  66. 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
    
  67. 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).

  68. DrahtBot added the label Needs rebase on Apr 22, 2024
  69. Gives seednode priority over dnsseed if both are provided 3120a4678a
  70. Added seednode prioritization message to help output 82f41d76f1
  71. sr-gi force-pushed on Apr 22, 2024
  72. sr-gi commented at 6:08 pm on April 22, 2024: member
    Rebased to address merge conflicts
  73. DrahtBot removed the label Needs rebase on Apr 22, 2024
  74. DrahtBot added the label CI failed on Apr 22, 2024
  75. DrahtBot removed the label CI failed on Apr 23, 2024
  76. davidgumberg commented at 5:13 pm on April 28, 2024: contributor
  77. cbergqvist approved
  78. 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).

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

  80. achow101 commented at 10:51 pm on April 30, 2024: member
    ACK 82f41d76f1c6ad38290917dad5499ffbe6b3974d
  81. achow101 merged this on Apr 30, 2024
  82. achow101 closed this on Apr 30, 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 06:12 UTC

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