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

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)

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)

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;
                 {

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

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

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

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

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

Considering

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?

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.

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.

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

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 :)

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

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.

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.

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.

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

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