p2p: Delay querying DNS seeds #16939

ajtowns commented at 10:51 AM on September 23, 2019: member

Changes the logic for querying DNS seeds: after this PR, if there's less than 1000 entries in addrman, it will still usually query DNS seeds after 11s (unless the first few peers tried mostly succeed), but if there's more than 1000 entries it won't try DNS seeds until 5 minutes have passed without getting multiple outbound peers. (If there's 0 entries in addrman, it will still immediately query the DNS seeds). Additionally, delays querying DNS seeds while the p2p network is not active.

Fixes #15434

ajtowns commented at 10:53 AM on September 23, 2019: member

Somewhat follows the suggestions of @luke-jr and @jgarzik in #15558

ajtowns commented at 11:02 AM on September 23, 2019: member

Behaviour looks like:

2019-09-23T10:54:49Z dnsseed thread start
2019-09-23T10:54:49Z Waiting 300 seconds before falling back to DNS seeds.
2019-09-23T10:54:50Z trying connection XXX lastseen=662.4hrs
2019-09-23T10:54:55Z connection to XXX timeout
2019-09-23T10:54:55Z trying connection XXX lastseen=197.4hrs
2019-09-23T10:54:56Z Added connection peer=0
2019-09-23T10:54:56Z New outbound peer connected: ...
2019-09-23T10:54:56Z trying connection XXX lastseen=469.4hrs

DNS thread would previously have decided to contact seeds here.

2019-09-23T10:55:01Z connection to XXX timeout
2019-09-23T10:55:02Z trying connection XXX lastseen=685.1hrs
2019-09-23T10:55:07Z connection to XXX timeout
2019-09-23T10:55:07Z trying connection XXX lastseen=232.5hrs
2019-09-23T10:55:12Z connection to XXX timeout
2019-09-23T10:55:13Z trying connection XXX lastseen=388.0hrs

DNS thread checks for early exit here, but doesn't need one so keeps waiting.

2019-09-23T10:55:18Z connection to XXX timeout
2019-09-23T10:55:18Z trying connection XXX lastseen=15.6hrs
2019-09-23T10:55:19Z Added connection peer=1
2019-09-23T10:55:19Z New outbound peer connected: ...
2019-09-23T10:55:19Z trying connection XXX lastseen=36.8hrs
2019-09-23T10:55:24Z connection to XXX timeout
2019-09-23T10:55:25Z trying connection XXX lastseen=214.8hrs
2019-09-23T10:55:30Z connection to XXX timeout
2019-09-23T10:55:30Z trying connection XXX lastseen=524.4hrs
2019-09-23T10:55:35Z connection to XXX timeout
2019-09-23T10:55:36Z trying connection XXX lastseen=1705.3hrs

DNS thread checks again for early exit, and since there's two connections now, does:

2019-09-23T10:55:39Z P2P peers available. Skipped DNS seeding.
2019-09-23T10:55:39Z dnsseed thread exit

fanquake added the label P2P on Sep 23, 2019

fanquake renamed this:
~~Delay querying DNS seeds if addrman is populated~~
p2p: Delay querying DNS seeds if addrman is populated
on Sep 23, 2019

practicalswift commented at 2:02 PM on September 23, 2019: contributor

Concept ACK: seed querying should be kept to a minimum for obvious reasons

naumenkogs commented at 3:53 PM on September 23, 2019: member

Concept ACK.

This doesn't make us depend on a single seed, does it? Also, perhaps move some of the magic numbers to constants?

ajtowns force-pushed on Sep 23, 2019

ajtowns commented at 6:32 PM on September 23, 2019: member

Fixed the logic to make it query in batches of 3 more sensibly, and bumped the delay for adding fixed seeds to 6 minutes from startup (fixed nodes are only added if addrman is empty, which will only happen if DNS seeds aren't replying -- but this may be due to the first one we try hanging, rather than all of them failing, so 6m is a compromise).

Behaviour for empty peers.dat, but no ability to actually connect anywhere is to immediately query 3 random dns seeds, then wait 11 seconds and try the next 3, etc:

2019-09-23T18:02:46Z Loaded 0 addresses from peers.dat  0ms
2019-09-23T18:02:46Z dnsseed thread start
2019-09-23T18:02:46Z Loading addresses from DNS seed dnsseed.bluematt.me
2019-09-23T18:02:47Z Loading addresses from DNS seed dnsseed.bitcoin.dashjr.org
2019-09-23T18:02:52Z Loading addresses from DNS seed seed.bitcoin.jonasschnelli.ch
2019-09-23T18:02:52Z Waiting 11 seconds before querying DNS seeds.
2019-09-23T18:03:03Z Loading addresses from DNS seed dnsseed.emzy.de
2019-09-23T18:03:03Z Loading addresses from DNS seed seed.bitcoin.sipa.be
2019-09-23T18:03:03Z Loading addresses from DNS seed seed.bitcoin.sprovoost.nl
2019-09-23T18:03:03Z Waiting 11 seconds before querying DNS seeds.
2019-09-23T18:03:14Z Loading addresses from DNS seed seed.btc.petertodd.org
2019-09-23T18:03:14Z 33 addresses found from DNS seeds
2019-09-23T18:03:14Z dnsseed thread exit

Same deal if there's some but less than 1k entries in addrman, except it'll wait 11s first:

2019-09-23T18:23:15Z Loaded 33 addresses from peers.dat  0ms
2019-09-23T18:23:15Z dnsseed thread start
2019-09-23T18:23:15Z Waiting 11 seconds before querying DNS seeds.
2019-09-23T18:23:26Z Loading addresses from DNS seed dnsseed.bitcoin.dashjr.org
2019-09-23T18:23:26Z Loading addresses from DNS seed dnsseed.emzy.de
2019-09-23T18:23:26Z Loading addresses from DNS seed dnsseed.bluematt.me
2019-09-23T18:23:27Z Waiting 11 seconds before querying DNS seeds.
2019-09-23T18:23:38Z Loading addresses from DNS seed seed.btc.petertodd.org
2019-09-23T18:23:38Z Loading addresses from DNS seed seed.bitcoin.sipa.be
2019-09-23T18:23:38Z Loading addresses from DNS seed seed.bitcoin.jonasschnelli.ch
2019-09-23T18:23:38Z Waiting 11 seconds before querying DNS seeds.
2019-09-23T18:23:49Z Loading addresses from DNS seed seed.bitcoin.sprovoost.nl
2019-09-23T18:23:49Z 0 addresses found from DNS seeds
2019-09-23T18:23:49Z dnsseed thread exit

(seed.bitcoinstats.com seems to hang for me, so I've dropped it from my chainparams, but that change isn't in this patch)

DrahtBot commented at 8:03 PM on September 23, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19029 (net: Fix CThreadInterrupt::sleep_for TSan issues by hebasto)
#15502 (p2p: Speed up initial connection to p2p network by ajtowns)

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.

fanquake added this to the milestone 0.19.0 on Sep 24, 2019

fanquake requested review from sdaftuar on Sep 24, 2019

laanwj removed this from the milestone 0.19.0 on Sep 26, 2019

laanwj added the label Bug on Sep 30, 2019

in src/net.cpp:59 in 899d9f4097 outdated

  52 | @@ -53,6 +53,15 @@ static constexpr int DUMP_PEERS_INTERVAL = 15 * 60;
  53 |  /** Number of DNS seeds to query when the number of connections is low. */
  54 |  static constexpr int DNSSEEDS_TO_QUERY_AT_ONCE = 3;
  55 |  
  56 | +/** How long to delay before querying DNS seeds
  57 | + */
  58 | +static constexpr std::chrono::seconds DNSSEEDS_DELAY_FEW_PEERS{11};
  59 | +static constexpr std::chrono::seconds DNSSEEDS_DELAY_MANY_PEERS{300};

ariard commented at 9:23 PM on January 9, 2020:

nit: // 5 minutes, also move them to net.h ?

ajtowns commented at 2:16 AM on February 11, 2020:

They're not needed from other modules, so doesn't make sense to have them in net.h

in src/net.cpp:1595 in 899d9f4097 outdated

1565 | +    //   creating fewer identifying DNS requests, reduces trust by
1566 | +    //   giving seeds less influence on the network topology, and
1567 | +    //   reduces traffic to the seeds.
1568 | +    // * When querying DNS seeds query a few at once, this ensures
1569 | +    //   that we don't give DNS seeds the ability to eclipse nodes
1570 | +    //   that query them.

ariard commented at 9:25 PM on January 9, 2020:

You can lay out trade-off further: "Every new DNS seed queried increases the bar for eclipsing node but leaks more privacy."

in src/net.cpp:1555 in 899d9f4097 outdated

1551 | @@ -1543,30 +1552,55 @@ void CConnman::ThreadDNSAddressSeed()
1552 |      Shuffle(seeds.begin(), seeds.end(), rng);
1553 |      int seeds_right_now = 0; // Number of seeds left before testing if we have enough connections
1554 |      int found = 0;
1555 | +    const std::chrono::seconds seeds_wait_time = (addrman.size() >= 1000 ? DNSSEEDS_DELAY_MANY_PEERS : DNSSEEDS_DELAY_FEW_PEERS);

ariard commented at 9:33 PM on January 9, 2020:

What the logic for picking 1000 ? If we assume uniform address propagation, what's the ratio of reliable public nodes on others in addrman and what the probability of not connecting successfully to at least 3 outbound peers ? Based on that, I would say intuitively we may scale down number a bit to increase number of nodes who delay DNS query.

ajtowns commented at 2:52 AM on February 11, 2020:

1000 is the maximum number of entries the addr message can include, so seems a good threshold to indicate you've managed to pick up some live data from the p2p network about potential peers, rather than just some results from DNS lookups.

ariard commented at 12:44 AM on March 16, 2020:

Okay, make sense given we send GETADDR at VERSION reception which means if you have at only one reliable outbound connection you should be able to bootstrap your addrman without seed assistance.

in src/net.cpp:1604 in 899d9f4097 outdated

1586 | -            }
1587 | -            if (nRelevant >= 2) {
1588 | -                LogPrintf("P2P peers available. Skipped DNS seeding.\n");
1589 | -                return;
1590 | +            LogPrintf("Waiting %d seconds before querying DNS seeds.\n", seeds_wait_time.count());
1591 | +            std::chrono::seconds to_wait = seeds_wait_time;

ariard commented at 9:36 PM on January 9, 2020:

For fresh nodes, addrman can't be bigger than 1000 after querying 3 dns (because nMaxIPs == 256), but if we change DNSSEEDS_TO_QUERY_AT_ONCE, we may assign seeds_wait_time before every time evaluation instead

ajtowns commented at 4:43 PM on February 11, 2020:

With the current code:

if addrman starts off at 0, it'll query all DNS seeds with no delay
if addrman starts off under 1000, it'll query all DNS seeds in groups of 3 after waiting 11 seconds before each group
if addrman starts off over 1000, it'll query all DNS seeds in groups of 3 after waiting 5 minutes before each group

That seems mostly fine to me?

(The timeout for connecting to a peer is nConnectTimeout which is 5 seconds; so 11 seconds will pass if you happen to try three peers that timeout before getting two that succeed)

ariard commented at 1:05 AM on March 16, 2020:

Right, I think I forget we set seeds_right_now to number of seeds if if we don't have any known peers (IIRC my comment motivation)

in src/net.cpp:1629 in 899d9f4097 outdated

1611 | +                    }
1612 | +                    return;
1613 | +                }
1614 |              }
1615 |              seeds_right_now += DNSSEEDS_TO_QUERY_AT_ONCE;
1616 | +        } else if (seeds_right_now == 0) {

ariard commented at 9:39 PM on January 9, 2020:

Not sure about this, I would say that's a behavior change. Previously node with an empty addrman would have query all DNS seeds due to seeds_right_now == -1 as you noticed in #15558. Now you may exit early, if among DNSSEEDS_TO_QUERY_AT_ONCE you find at least 2 outbound peers but with final result of having less address origin diversity in your addrman..

in src/net.cpp:1752 in 899d9f4097 outdated

1748 | @@ -1715,7 +1749,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
1749 |              return;
1750 |  
1751 |          // Add seed nodes if DNS seeds are all down (an infrastructure attack?).
1752 | -        if (addrman.size() == 0 && (GetTime() - nStart > 60)) {
1753 | +        if (addrman.size() == 0 && (GetTime() - nStart > FIXEDSEEDS_DELAY)) {

ariard commented at 9:41 PM on January 9, 2020:

This is a behavior change too, I do understand the logic to increase privacy for fresh nodes if DNS are busy but how did you pick up this number ?

ajtowns commented at 4:10 AM on February 11, 2020:

This change isn't needed -- if addrman is going to end up empty, it will have been empty for each dns seed which means there would have been no delay in querying the seed.

ariard commented at 9:47 PM on January 9, 2020: member

@ajtowns what's the state of this PR ?

I think that's interesting work but title/description should be updated to inform about the 3 behavior changes:

delay querying DNS seeds for 5min if addrman is less than 1000 (concept ACK)
reduce set of queried DNS for fresh nodes to 3 instead of all (need to think more)
increase fallback to hardcoded seeds for fresh nodes to 6 min instead of 60s (concept ACK)

gmaxwell commented at 10:14 PM on January 9, 2020: contributor

This shouldn't run if the node is not currently making automatic connections (network disable or -noconnect).

laanwj added the label Waiting for author on Feb 5, 2020

ajtowns commented at 3:06 AM on February 11, 2020: member

@gmaxwell

This shouldn't run if the node is not currently making automatic connections (network disable or -noconnect).

For -noconnect we already have "parameter interaction: -connect set -> setting -dnsseed=0" which prevents the dns thread from ever starting. Not sure what network disable means, should we not do dns seeding if -onlynet=onion or something?

ajtowns force-pushed on Feb 11, 2020

ajtowns commented at 4:39 AM on February 11, 2020: member

Rebased, reverted delay for fixed seeds, and changed to explicitly query all dns seeds if we start up with an empty addrman.

ajtowns removed the label Waiting for author on Feb 11, 2020

MarcoFalke commented at 1:09 PM on February 11, 2020: member

Not sure what network disable means

There is a setnetworkactive RPC, which can disable the connman

ajtowns renamed this:
~~p2p: Delay querying DNS seeds if addrman is populated~~
p2p: Delay querying DNS seeds
on Feb 11, 2020

ajtowns commented at 4:35 PM on February 11, 2020: member

Added patch to avoid querying DNS seeds while setnetworkactive is set to false.

hebasto commented at 9:58 AM on March 11, 2020: member

Concept ACK.

in src/net.cpp:1606 in 5bf1665316 outdated

1615 | -                LogPrintf("P2P peers available. Skipped DNS seeding.\n");
1616 | -                return;
1617 | +            LogPrintf("Waiting %d seconds before querying DNS seeds.\n", seeds_wait_time.count());
1618 | +            std::chrono::seconds to_wait = seeds_wait_time;
1619 | +            while (to_wait.count() > 0) {
1620 | +                std::chrono::seconds w{25}; // check for early abort every 25 seconds

hebasto commented at 1:26 PM on March 11, 2020:

As 25 seconds is a timeout for the early exit from dnsseed thread, we could do not introduce a new heuristic constant at all:

--- a/src/net.cpp
+++ b/src/net.cpp
@@ -41,9 +41,9 @@
 static_assert(MINIUPNPC_API_VERSION >= 10, "miniUPnPc API version >= 10 assumed");
 #endif
 
-#include <unordered_map>
-
+#include <chrono>
 #include <math.h>
+#include <unordered_map>
 
 // Dump addresses to peers.dat every 15 minutes (900s)
 static constexpr int DUMP_PEERS_INTERVAL = 15 * 60;
@@ -1601,12 +1601,12 @@ void CConnman::ThreadDNSAddressSeed()
     for (const std::string& seed : seeds) {
         if (addrman.size() > 0 && seeds_right_now == 0) {
             LogPrintf("Waiting %d seconds before querying DNS seeds.\n", seeds_wait_time.count());
-            std::chrono::seconds to_wait = seeds_wait_time;
+            std::chrono::milliseconds to_wait{seeds_wait_time};
             while (to_wait.count() > 0) {
-                std::chrono::seconds w{25}; // check for early abort every 25 seconds
-                if (w > to_wait) w = to_wait;
-                if (!interruptNet.sleep_for(w)) return;
-                to_wait -= w;
+                std::chrono::milliseconds recheck_period{nConnectTimeout};
+                if (recheck_period > to_wait) recheck_period = to_wait;
+                if (!interruptNet.sleep_for(recheck_period)) return;
+                to_wait -= recheck_period;
 
                 int nRelevant = 0;
                 {

ajtowns commented at 12:22 AM on March 12, 2020:

It'd make sense for DNSSEEDS_DELAY_FEW_PEERS to be calculated from that rather than a constant, but the early exit ought to be based on how long it takes for a connection to succeed rather than the timeout time

ajtowns commented at 6:21 AM on March 13, 2020:

On the other hand, can easily reuse DNSSEEDS_DELAY_FEW_PEERS instead of a new constant, so will just do that.

jgarzik commented at 1:55 PM on March 11, 2020: contributor

concept ACK

When DNS seeding was introduced, the idea was to be a backup, for when P2P peer addr exchange is not working for whatever reason. It seems preferable to make the polling closer to "24 hours" or "never" in the case where AddrMan is healthy and happy.

Regardless of that preference, concept ACK as stated above, as this appears to be a strict improvement & resolve an issue.

in src/net.cpp:1630 in 5bf1665316 outdated

1639 | +                    return;
1640 | +                }
1641 |              }
1642 |              seeds_right_now += DNSSEEDS_TO_QUERY_AT_ONCE;
1643 | +        } else if (seeds_right_now == 0) {
1644 | +            seeds_right_now += DNSSEEDS_TO_QUERY_AT_ONCE;

hebasto commented at 2:17 PM on March 11, 2020:

It seems this branch will never be executed since all of the dns seeds are explicitly queried if we start up with an empty addrman: https://github.com/bitcoin/bitcoin/blob/5bf16653168cbc3c8bd0814cfa937d0dc8a879ff/src/net.cpp#L1582-L1585

ajtowns commented at 6:22 AM on March 13, 2020:

Rearranged to if (s_r_n == 0) { s_r_n += AT_ONCE; if (addrman.size() == 0) { wait(); } }

in src/net.cpp:1618 in 5bf1665316 outdated

1627 | +                    LOCK(cs_vNodes);
1628 | +                    for (const CNode* pnode : vNodes) {
1629 | +                        nRelevant += pnode->fSuccessfullyConnected && !pnode->fFeeler && !pnode->fOneShot && !pnode->m_manual_connection && !pnode->fInbound;
1630 | +                    }
1631 | +                }
1632 | +                if (nRelevant >= 2) {

hebasto commented at 2:24 PM on March 11, 2020:

While here mind replacing a magic number 2 with a named constant?

ajtowns force-pushed on Mar 13, 2020

ariard commented at 1:18 AM on March 16, 2020: member

@ajtowns I think you need to update commit message, it's still mentioning the 25s delays while you now rely on DNSSEED_DELAY_FEW_PEERS?

in src/net.cpp:1609 in b1338cc25c outdated

1618 | -                return;
1619 | +            if (addrman.size() > 0) {
1620 | +                LogPrintf("Waiting %d seconds before querying DNS seeds.\n", seeds_wait_time.count());
1621 | +                std::chrono::seconds to_wait = seeds_wait_time;
1622 | +                while (to_wait.count() > 0) {
1623 | +                    std::chrono::seconds w = DNSSEEDS_DELAY_FEW_PEERS;

sipa commented at 4:35 AM on March 16, 2020:

                    std::chrono::seconds w = std::min(DNSSEEDS_DELAY_FEW_PEERS, to_wait);

ajtowns commented at 6:36 PM on March 17, 2020:

Took this since I rebased to update the commit description

sipa commented at 4:49 AM on March 16, 2020: member

utACK b1338cc25c0d66a73aea760117e488b6c91d0d43

DNS seeds: wait for 5m instead of 11s if 1000+ peers are known

If 1000 potential peers are known, wait for 5m before querying DNS seeds
for more peers, since eventually the addresses we already know should
get us connected. Also check every 11s whether we've got enough active
outbounds that DNS seeds aren't worth querying, and exit the dnsseed
thread early if so.

fa5894f7f5

DNS seeds: don't query DNS while network is inactive 96954d1794

ajtowns force-pushed on Mar 17, 2020

in src/net.cpp:1599 in 96954d1794

1594 | +    //   that we don't give DNS seeds the ability to eclipse nodes
1595 | +    //   that query them.
1596 | +    // * If we continue having problems, eventually query all the
1597 | +    //   DNS seeds, and if that fails too, also try the fixed seeds.
1598 | +    //   (done in ThreadOpenConnections)
1599 | +    const std::chrono::seconds seeds_wait_time = (addrman.size() >= DNSSEEDS_DELAY_PEER_THRESHOLD ? DNSSEEDS_DELAY_MANY_PEERS : DNSSEEDS_DELAY_FEW_PEERS);

hebasto commented at 11:34 AM on March 20, 2020:

Could add #include <chrono>? See Source code organization.

ajtowns commented at 4:40 PM on May 26, 2020:

We already use std::chrono in net.cpp; I think it's included via net.h via addrman.h via util/system.h via util/time.h. So yeah, could see the value in cleaning that up, but don't think it needs to be in this patch.

in src/net.cpp:1605 in 96954d1794

1614 | -                nRelevant += pnode->fSuccessfullyConnected && !pnode->fFeeler && !pnode->fOneShot && !pnode->m_manual_connection && !pnode->fInbound;
1615 | -            }
1616 | -            if (nRelevant >= 2) {
1617 | -                LogPrintf("P2P peers available. Skipped DNS seeding.\n");
1618 | -                return;
1619 | +            if (addrman.size() > 0) {

hebasto commented at 11:40 AM on March 20, 2020:

Considering https://github.com/bitcoin/bitcoin/blob/96954d17948662672cababc940e453dff08e8cbb/src/net.cpp#L1582-L1585 and https://github.com/bitcoin/bitcoin/blob/96954d17948662672cababc940e453dff08e8cbb/src/net.cpp#L1602

this point is not reachable when addrman.size() == 0, and addrman.size() > 0 will always be evaluated to true, so it could be dropped, no?

naumenkogs commented at 5:33 PM on May 19, 2020:

I think you are right @hebasto. Although it took 5 minutes for me to confirm. I think if we remove this redundant check, a comment should be added.

ajtowns commented at 4:00 PM on May 26, 2020:

If we did reach it with addrman.size() == 0, waiting would be pointless, so I don't think the logic is wrong; there's no else so there's no unreachable code, just a conditional branch that always happens to go one way. I think double checking is a bit more robust in case we change the code to querying some subset of dns seeds when addrman is empty in future, eg if we had dozens of dns seeds available.

Sjors commented at 5:18 PM on May 28, 2020:

in case we change the code to querying some subset of dns seeds when addrman is empty in future

That's worth explaining in a comment. It may also be more readable to have a bool skip_wait

in src/net.cpp:1620 in 96954d1794

1629 | +                        LOCK(cs_vNodes);
1630 | +                        for (const CNode* pnode : vNodes) {
1631 | +                            nRelevant += pnode->fSuccessfullyConnected && !pnode->fFeeler && !pnode->fOneShot && !pnode->m_manual_connection && !pnode->fInbound;
1632 | +                        }
1633 | +                    }
1634 | +                    if (nRelevant >= 2) {

hebasto commented at 11:41 AM on March 20, 2020:

                    if (nRelevant >= DNSSEEDS_ENOUGH_PEERS) {

ajtowns commented at 4:43 PM on May 26, 2020:

I don't really think this adds anything -- the logs in the body of the if seems like it documents the value better than giving it a name would, and we don't reuse it anywhere else.

hebasto changes_requested

hebasto commented at 11:44 AM on March 20, 2020: member

Tested 96954d17948662672cababc940e453dff08e8cbb on Linux Mint 19.3: works as expected.

ariard approved

ariard commented at 10:41 PM on April 22, 2020: member

Tested ACK 96954d1, on Debian 9.1. Both MANY_PEERS/FEW_PEERS cases work.

2020-04-22T22:13:40Z Loaded 3579 addresses from peers.dat  10ms
2020-04-22T22:13:40Z init message: Starting network threads...
2020-04-22T22:13:40Z net thread start
2020-04-22T22:13:40Z addcon thread start
2020-04-22T22:13:40Z Using /16 prefix for IP bucketing.
2020-04-22T22:13:40Z init message: Done loading
2020-04-22T22:13:40Z msghand thread start
2020-04-22T22:13:40Z dnsseed thread start
2020-04-22T22:13:40Z Waiting 300 seconds before querying DNS seeds.
2020-04-22T22:18:40Z Loading addresses from DNS seed dnsseed.bitcoin.dashjr.org
2020-04-22T22:18:40Z Loading addresses from DNS seed seed.bitcoinstats.com
2020-04-22T22:18:41Z Loading addresses from DNS seed seed.bitcoin.sipa.be

in src/net.cpp:58 in 96954d1794

  50 | @@ -51,6 +51,12 @@ static constexpr int DUMP_PEERS_INTERVAL = 15 * 60;
  51 |  /** Number of DNS seeds to query when the number of connections is low. */
  52 |  static constexpr int DNSSEEDS_TO_QUERY_AT_ONCE = 3;
  53 |  
  54 | +/** How long to delay before querying DNS seeds
  55 | + */
  56 | +static constexpr std::chrono::seconds DNSSEEDS_DELAY_FEW_PEERS{11}; // 11sec
  57 | +static constexpr std::chrono::seconds DNSSEEDS_DELAY_MANY_PEERS{300}; // 5min
  58 | +static constexpr int DNSSEEDS_DELAY_PEER_THRESHOLD = 1000; // "many" vs "few" peers -- you should only get this many if you've been on the live network

naumenkogs commented at 4:49 PM on May 19, 2020:

I think we should expand this comment into couple sentences. What's "live network" is unclear. What if we have 500 peers, is that not a live network? Would be also nice to briefly rationalize this distinction (yes, one more time). Also, long inline comments like this are hard to read :)

in src/net.cpp:1582 in 96954d1794

1578 | @@ -1573,31 +1579,67 @@ void CConnman::ThreadDNSAddressSeed()
1579 |      if (gArgs.GetBoolArg("-forcednsseed", DEFAULT_FORCEDNSSEED)) {
1580 |          // When -forcednsseed is provided, query all.
1581 |          seeds_right_now = seeds.size();
1582 | +    } else if (addrman.size() == 0) {

naumenkogs commented at 5:16 PM on May 19, 2020:

Comment here would be nice too. What exactly this tells us? For example, I spent time thinking "what if a node was connected to 1 node via --connect". But then they'd still probably have a big addrman (all learned from that peer)...

ajtowns commented at 4:22 PM on May 26, 2020:

It tells us we don't have any known peers (there's already a comment to that effect) -- perhaps this is the first time you've started bitcoind or you've just restarted after deleting peers.dat. If you previously queried the dns seeds, or have at some point connected to some other peers that aren't in IBD, this shouldn't be true any more.

naumenkogs approved

naumenkogs commented at 6:01 PM on May 19, 2020: member

utACK 96954d17948662672cababc940e453dff08e8cbb Feel free to ignore comment suggestions if you’re not force-pushing any other fixes.

MarcoFalke commented at 5:06 PM on May 26, 2020: member

@ajtowns Would you prefer to add a new commit with documentation or instead have this merged first or something else?

ajtowns commented at 12:15 AM on May 28, 2020: member

@ajtowns Would you prefer to add a new commit with documentation or instead have this merged first or something else?

Maybe a separate PR for updating the comments? See #19084

in src/net.cpp:1584 in fa5894f7f5 outdated

1578 | @@ -1573,31 +1579,61 @@ void CConnman::ThreadDNSAddressSeed()
1579 |      if (gArgs.GetBoolArg("-forcednsseed", DEFAULT_FORCEDNSSEED)) {
1580 |          // When -forcednsseed is provided, query all.
1581 |          seeds_right_now = seeds.size();
1582 | +    } else if (addrman.size() == 0) {
1583 | +        // If we have no known peers, query all.
1584 | +        seeds_right_now = seeds.size();

Sjors commented at 4:55 PM on May 28, 2020:

This undoes #15558 for the first run. No strong opinion on the matter, but it should probably be in the PR and commit description. It's also not mentioned in the comments below.

Also, both here and in the if branch it's useful to comment that seeds_right_now > 0 causes the wait to be skipped.

ajtowns commented at 10:05 PM on May 28, 2020:

In #15558 the first run will set seeds_right_now = 0 then see that addrman.size() > 0 && seeds_right_now == 0 is false (since addrman.size() == 0) so will not sleep nor bump seeds_right_now by DNSSEEDS_TO_QUERY_AT_ONCE, it will then query a seed, and decrement seeds_right_now so that it's -1, at which point addrman.size() > 0 && seeds_right_now == 0 will continue being false (because seeds_right_now < 0) and will query all seeds without waiting between them.

This retains that behaviour, but is explicit about it. See suhas's comment on 15558

in src/net.cpp:1609 in fa5894f7f5 outdated

1618 | -                return;
1619 | +            if (addrman.size() > 0) {
1620 | +                LogPrintf("Waiting %d seconds before querying DNS seeds.\n", seeds_wait_time.count());
1621 | +                std::chrono::seconds to_wait = seeds_wait_time;
1622 | +                while (to_wait.count() > 0) {
1623 | +                    std::chrono::seconds w = std::min(DNSSEEDS_DELAY_FEW_PEERS, to_wait);

Sjors commented at 5:25 PM on May 28, 2020:

Could use a comment of why you're sleeping in shorter intervals: because we can exit this thread once we have enough nodes.

Sjors approved

Sjors commented at 5:39 PM on May 28, 2020: member

tACK 96954d1 (rebased on master) on macOS 10.15.4. It found it useful to run with -debug=addrman and change DNSSEEDS_DELAY_MANY_PEERS to something lower to test the behaviour, as well as renaming peers.dat to test the peer threshold.

The second commit can be tested by calling bitcoin-cli setnetworkactive false within DNSSEEDS_DELAY_MANY_PEERS seconds of the DNS thread starting.

Consider adding the improved documentation as a new commit on top of this PR. That doesn't break any reviews, but it makes it easier to squash if you need to change anything else.

fanquake approved

fanquake commented at 10:23 AM on May 29, 2020: member

ACK 96954d17948662672cababc940e453dff08e8cbb - Ran some tests of different scenarios. More documentation is being added in #19084.

# already had a large peers.dat
src/bitcoind | rg dnsseed
2020-05-29T10:00:29.745627Z [dnsseed] dnsseed thread start
2020-05-29T10:00:29.745657Z [dnsseed] Waiting 300 seconds before querying DNS seeds.
2020-05-29T10:00:40.745989Z [dnsseed] P2P peers available. Skipped DNS seeding.
2020-05-29T10:00:40.746071Z [dnsseed] dnsseed thread exit

# deleted peers.dat
src/bitcoind | rg dnsseed
2020-05-29T10:01:38.673439Z [dnsseed] dnsseed thread start
2020-05-29T10:01:38.673508Z [dnsseed] Loading addresses from DNS seed dnsseed.bitcoin.dashjr.org
2020-05-29T10:01:38.674948Z [dnsseed] Loading addresses from DNS seed dnsseed.emzy.de
2020-05-29T10:01:39.681687Z [dnsseed] Loading addresses from DNS seed dnsseed.bluematt.me
2020-05-29T10:01:40.081659Z [dnsseed] Loading addresses from DNS seed seed.bitcoinstats.com
2020-05-29T10:01:40.705362Z [dnsseed] Loading addresses from DNS seed seed.bitcoin.jonasschnelli.ch
2020-05-29T10:01:41.014162Z [dnsseed] Loading addresses from DNS seed seed.bitcoin.sprovoost.nl
2020-05-29T10:01:42.539538Z [dnsseed] Loading addresses from DNS seed seed.btc.petertodd.org
2020-05-29T10:01:45.568217Z [dnsseed] Loading addresses from DNS seed seed.bitcoin.sipa.be
2020-05-29T10:01:45.570067Z [dnsseed] 305 addresses found from DNS seeds
2020-05-29T10:01:45.570103Z [dnsseed] dnsseed thread exit

# peers.dat < 1000 addresses
src/bitcoind | rg dnsseed
2020-05-29T10:04:03.566819Z [dnsseed] dnsseed thread start
2020-05-29T10:04:03.566983Z [dnsseed] Waiting 11 seconds before querying DNS seeds.
2020-05-29T10:04:14.567539Z [dnsseed] P2P peers available. Skipped DNS seeding.
2020-05-29T10:04:14.567643Z [dnsseed] dnsseed thread exit

# -connect=0
src/bitcoind -connect=0 | rg dnsseed
2020-05-29T10:05:19.064542Z [init] InitParameterInteraction: parameter interaction: -connect set -> setting -dnsseed=0

# called setnetworkactive false early
src/bitcoind | rg dnsseed
2020-05-29T10:07:31.399566Z [dnsseed] dnsseed thread start
2020-05-29T10:07:31.399595Z [dnsseed] Waiting 300 seconds before querying DNS seeds.
2020-05-29T10:12:31.466326Z [dnsseed] Waiting for network to be reactivated before querying DNS seeds.
2020-05-29T10:14:20.753947Z [dnsseed] Loading addresses from DNS seed dnsseed.emzy.de
2020-05-29T10:14:20.762381Z [dnsseed] Loading addresses from DNS seed dnsseed.bitcoin.dashjr.org
2020-05-29T10:14:20.763729Z [dnsseed] Loading addresses from DNS seed seed.bitcoin.jonasschnelli.ch
2020-05-29T10:14:20.765102Z [dnsseed] Waiting 300 seconds before querying DNS seeds.
2020-05-29T10:14:42.766527Z [dnsseed] 113 addresses found from DNS seeds
2020-05-29T10:14:42.766629Z [dnsseed] P2P peers available. Finished DNS seeding.
2020-05-29T10:14:42.766673Z [dnsseed] dnsseed thread exit

src/bitcoind | rg dnsseed
2020-05-29T10:15:02.325759Z [dnsseed] dnsseed thread start
2020-05-29T10:15:02.325804Z [dnsseed] Waiting 300 seconds before querying DNS seeds.
2020-05-29T10:15:13.326011Z [dnsseed] P2P peers available. Skipped DNS seeding.
2020-05-29T10:15:13.326123Z [dnsseed] dnsseed thread exit

fanquake merged this on May 29, 2020

fanquake closed this on May 29, 2020

sidhujag referenced this in commit 4f7ea77c81 on May 31, 2020

fanquake referenced this in commit 657b82cef0 on Jun 3, 2020

sidhujag referenced this in commit dd925a31c5 on Jun 3, 2020

deadalnix referenced this in commit ebbaf0f25c on Nov 24, 2020

DrahtBot locked this on Feb 15, 2022