p2p: No delay in adding fixed seeds if -dnsseed=0 and peers.dat is empty

dhruv commented at 4:46 pm on September 5, 2020: member

Closes #19795

Before PR: If peers.dat is empty and -dnsseed=0, bitcoind will fallback on to fixed seeds but only after a 60 seconds delay. After PR: There’s no 60 second delay.

To reproduce: rm ~/.bitcoin/peers.dat && src/bitcoind -dnsseed=0 without and with patch code

Other changes in the PR:

-fixedseeds command line argument added: -dnsseed=0 -fixedseeds=0 -addnode=X provides a trusted peer only setup. -dnsseed=0 -fixedseeds=0 allows for a addnode RPC to add a trusted peer without falling back to hardcoded seeds.

dhruv force-pushed on Sep 5, 2020

dhruv renamed this:
~~No delay in adding fixed seeds if -dnsseed=0 and peers.dat is empty~~
p2p: No delay in adding fixed seeds if -dnsseed=0 and peers.dat is empty
on Sep 5, 2020

n-thumann approved

n-thumann commented at 5:11 pm on September 5, 2020: contributor

tACK bcbd637 on master at 3ba25e3, was able to reproduce the issue and confirm that it´s fixed with this PR ✌️

dhruv commented at 5:23 pm on September 5, 2020: member

@practicalswift I’m sorry this took so long. Ready for review.

dhruv commented at 5:24 pm on September 5, 2020: member

@n-thumann Thank you, for the prompt review.

DrahtBot added the label P2P on Sep 5, 2020

in src/net.cpp:1848 in bcbd637bcf outdated

1850-            if (!done) {
1851+        // If we started with an empty peers.dat, we will query DNS seeds immediately.
1852+        // Here we allow 60 seconds for the DNS seeds to return results. If they do not
1853+        // return any results, or if they were disabled with -dnsseed=0, we add
1854+        // the fixed seeds.
1855+        bool dnsseed = gArgs.GetBoolArg("-dnsseed", DEFAULT_DNSSEED);

practicalswift commented at 5:52 pm on September 5, 2020:

Nit:

0        const bool dnsseed = gArgs.GetBoolArg("-dnsseed", DEFAULT_DNSSEED);

dhruv commented at 6:10 pm on September 5, 2020:

Done. Thanks for the catch.

in src/net.cpp:1851 in bcbd637bcf outdated

1853+        // return any results, or if they were disabled with -dnsseed=0, we add
1854+        // the fixed seeds.
1855+        bool dnsseed = gArgs.GetBoolArg("-dnsseed", DEFAULT_DNSSEED);
1856+        if (addrman.size() == 0 &&
1857+            ((GetTime() - nStart > 60) || !dnsseed)) {
1858+            if (!dnsseed) {

practicalswift commented at 5:54 pm on September 5, 2020:

Nit: Personally I find if-else logic easier to read if starting with the truthy case. In this specific case: starting with the if (dnsseed) { case and handle the !dnsseed case in the else block :)

dhruv commented at 6:10 pm on September 5, 2020:

Done

in src/net.cpp:1850 in bcbd637bcf outdated

1852+        // Here we allow 60 seconds for the DNS seeds to return results. If they do not
1853+        // return any results, or if they were disabled with -dnsseed=0, we add
1854+        // the fixed seeds.
1855+        bool dnsseed = gArgs.GetBoolArg("-dnsseed", DEFAULT_DNSSEED);
1856+        if (addrman.size() == 0 &&
1857+            ((GetTime() - nStart > 60) || !dnsseed)) {

jonatack commented at 6:02 pm on September 5, 2020:

Might be outside the scope of this PR, but ISTMGetTime() is deprecated; see src/util/time.h.

dhruv commented at 10:49 pm on September 5, 2020:

Addressed.

in src/net.cpp:1849 in bcbd637bcf outdated

1851+        // If we started with an empty peers.dat, we will query DNS seeds immediately.
1852+        // Here we allow 60 seconds for the DNS seeds to return results. If they do not
1853+        // return any results, or if they were disabled with -dnsseed=0, we add
1854+        // the fixed seeds.
1855+        bool dnsseed = gArgs.GetBoolArg("-dnsseed", DEFAULT_DNSSEED);
1856+        if (addrman.size() == 0 &&

jonatack commented at 6:06 pm on September 5, 2020:

0        if (addrman.empty() &&

dhruv commented at 10:23 pm on September 5, 2020:

no member named empty in CAddrMan.

Happy to create one if that’s what you are suggesting.

jonatack commented at 3:22 pm on September 10, 2020:

Oops, you are right. Probably out of scope to create it unless you want to.

dhruv force-pushed on Sep 5, 2020

jonatack commented at 6:24 pm on September 5, 2020: member

Concept ACK. Maybe add a regression test in feature_config_args.py to cover these cases.

practicalswift commented at 6:49 pm on September 5, 2020: contributor

@dhruv I saw you removed the done logic: are you sure that !addrman.empty() is guaranteed after addrman.Add(convertSeed6(Params().FixedSeeds()), local);? AFAICT that condition must be hold for the new code to be equivalent to the old code.

dhruv force-pushed on Sep 6, 2020

dhruv commented at 0:20 am on September 6, 2020: member

@jonatack Added a regression test in feature_config_args.py to cover the cases. It’s a slow test though because we have to wait 60 seconds. Is there anything I can do to reduce that interval for tests? I thought about making it configurable via cli flags but I will look to your guidance on whether that is standard practice.

dhruv commented at 0:37 am on September 6, 2020: member

@dhruv I saw you removed the done logic: are you sure that !addrman.empty() is guaranteed after addrman.Add(convertSeed6(Params().FixedSeeds()), local);? AFAICT that condition must be hold for the new code to be equivalent to the old code. @practicalswift Thank you for catching that. I overlooked the implication of the static qualifier on done and mistakenly thought the code was redundant. It is now added back in.

It seems that the done logic is used to make sure that addrman.Add(convertSeed6(Params().FixedSeeds()), local); gets called no more than once regardless of the outcome. When I followed the code in CAddrMan, it seems that the invocation just adds the fixed seeds to the address tables so long as the seeds are well-formed. So invoking the deterministic function more than once is unnecessary. I’m new around here, so I’m hoping you can confirm my understanding.

dhruv force-pushed on Sep 6, 2020

dhruv commented at 5:38 am on September 7, 2020: member

Appveyor has passed on e7de5b5, Github is just not reflecting it.

dhruv requested review from jonatack on Sep 10, 2020

dhruv requested review from practicalswift on Sep 10, 2020

in src/net.cpp:1832 in e7de5b53e7 outdated

1825@@ -1826,10 +1826,10 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
1826     }
1827 
1828     // Initiate network connections
1829-    int64_t nStart = GetTime();
1830+    auto nStart = GetTime<std::chrono::seconds>();
1831 
1832     // Minimum time before next feeler connection (in microseconds).
1833-    int64_t nNextFeeler = PoissonNextSend(nStart*1000*1000, FEELER_INTERVAL);
1834+    int64_t nNextFeeler = PoissonNextSend(nStart.count()*1000*1000, FEELER_INTERVAL);

jonatack commented at 3:33 pm on September 10, 2020:

nit: clang format while touching this

0-    int64_t nNextFeeler = PoissonNextSend(nStart.count()*1000*1000, FEELER_INTERVAL);
1-    while (!interruptNet)
2-    {
3+    int64_t nNextFeeler = PoissonNextSend(nStart.count() * 1000 * 1000, FEELER_INTERVAL);
4+    while (!interruptNet) {

in test/functional/feature_config_args.py:152 in e7de5b53e7 outdated

147+        # We expect the node will use DNS Seeds, but Regtest mode has 0 DNS seeds
148+        # So after 60 seconds, the node should fallback to fixed seeds (this is a slow test)
149+        assert not os.path.exists(os.path.join(default_data_dir, "peers.dat"))
150+        start = time.time()
151+        with self.nodes[0].assert_debug_log(expected_msgs=['Loaded 0 addresses from peers.dat',
152+                '0 addresses found from DNS seeds', 'Adding fixed seed nodes as DNS doesn\'t seem to be available.'],

jonatack commented at 5:54 pm on September 10, 2020:

0                '0 addresses found from DNS seeds', "Adding fixed seed nodes as DNS doesn't seem to be available."],

jonatack commented at 6:01 pm on September 10, 2020: member

ACK e7de5b53e7b4433d41b519ca04d4c3aad0b6e186

Thanks for adding the tests. No strong opinion whether it’s better to drop the slow first test or keep it. There is a setmocktime RPC often used in the functional tests, and a -mocktime=<n> config option that could be passed an extra_arg to start_node, but AFAICT neither one will have any effect and I’m not sure it’s worth changing the code to be more testable unless you have an elegant way to do it or it could be generalized and useful elsewhere.

A couple of nit comments follow; you can ignore them unless you need to retouch for other reasons.

ajtowns commented at 11:24 pm on September 10, 2020: member

If peers.dat is empty and -dnsseed=0

Is this a use case worth optimising for? The linked issue doesn’t really provide a reason why this use case is worthwhile, as far as I can see, just that the filer thought it was surprising.

The only case I can see where you’d want to set -dnsseed=0 is if you somehow don’t have DNS despite being connected to the real internet, or if you’re really worried about privacy. But if you’re really worried about privacy, connecting to fixed seeds is probably worse than doing a dns query, and if you were doing something clever, like triggering an outbound/inbound connection to your own node, losing the 60 second delay might also drop privacy.

dhruv commented at 4:20 am on September 11, 2020: member

Is this a use case worth optimising for? The linked issue doesn’t really provide a reason why this use case is worthwhile, as far as I can see, just that the filer thought it was surprising.

Fair question, @ajtowns. Let’s page @practicalswift and see if he can shed light on his original use case.

practicalswift commented at 6:01 am on September 11, 2020: contributor

The only case I can see where you’d want to set -dnsseed=0 is if you somehow don’t have DNS despite being connected to the real internet, or if you’re really worried about privacy. But if you’re really worried about privacy, connecting to fixed seeds is probably worse than doing a dns query, and if you were doing something clever, like triggering an outbound/inbound connection to your own node, losing the 60 second delay might also drop privacy.

There are multiple important use cases for -dnsseed=0:

You don’t want to use clearnet at all (-dnsseed=0 -onlynet=onion -listen=0)
You don’t want to broadcast to the DNS seed operators (or anyone listening on the path to those DNS seed operators) that you’re starting a new node (-dnsseed=0 -listen=0)
You want to test how Bitcoin Core behaves if the DNS seeds are unavailable

I hope you’re not advocating the removal of -dnsseed=0 :)

Is this a use case worth optimising for? The linked issue doesn’t really provide a reason why this use case is worthwhile, as far as I can see, just that the filer thought it was surprising.

I’m not sure I would call removal of a 60 second sleep an optimization. To me it is clearly a bug fix :)

Also, please note that the behaviour is unchanged if -dnsseed=0 is not set.

The only effect of this change is that users of -dnsseed=0 won’t have to wait an extra 60 seconds for their node to start.

Given the above clarification: do your skepticism to this bug fix remain? :)

sipa commented at 6:16 am on September 11, 2020: member

You don’t want to use clearnet at all (-dnsseed=0 -onlynet=onion -listen=0)

FWIW, if you use -proxy, DNS seeding happens over the proxy (which in the case of Tor means the exit node will do the resolving for you).

practicalswift commented at 6:21 am on September 11, 2020: contributor

You don’t want to use clearnet at all (-dnsseed=0 -onlynet=onion -listen=0)

FWIW, if you use -proxy, DNS seeding happens over the proxy (which in the case of Tor means the exit node will do the resolving for you).

Yes I know, but then you’ll have to trust the exit node operator not to tamper with the DNS responses :)

-dnsseed=0 -onlynet=onion -listen=0 avoids that.

sipa commented at 6:23 am on September 11, 2020: member

Of course, and I’m not commenting on the validity of the use case in general. Just that “You don’t want to use clearnet at all” on its own is not a reason to use -dnsseed=0.

practicalswift commented at 6:25 am on September 11, 2020: contributor

Of course, and I’m not commenting on the validity of the use case in general. Just that “You don’t want to use clearnet at all” on its own is not a reason to use -dnsseed=0.

Fair point. I should have written “You don’t want to use clearnet at all and you don’t trust exit node operators” :)

practicalswift commented at 9:07 am on September 11, 2020: contributor

FWIW, if you use -proxy, DNS seeding happens over the proxy (which in the case of Tor means the exit node will do the resolving for you).

By “DNS seeding” you’re in this context referring to the “establish ADDR_FETCH connections via proxy to seed nodes”-logic, right?

AFAICT we’ve never supported sending actual DNS queries to the DNS seed nodes via proxy.

From the SetNameProxy documentation: “SOCKS5’s support for UDP-over-SOCKS5 has been considered, but no SOCK5 server in common use (most notably Tor) actually implements UDP support, and a DNS resolver is beyond the scope of this project.”

practicalswift approved

practicalswift commented at 1:42 pm on September 11, 2020: contributor

Tested ACK e7de5b53e7b4433d41b519ca04d4c3aad0b6e186

This fixes the bug which makes -dnsseed=0 users have to wait out a 60 second sleep intended only for -dnsseed=1 users :)

sipa commented at 5:20 pm on September 11, 2020: member

@practicalswift Yes, when -proxy is enabled, no actual DNS seeding is done, but ADDR_FETCH connections are established to the seeder’s names instead. That was the reason for introducing ADDR_FETCH connections (formerly called oneshot connections) initially.

ajtowns commented at 10:20 pm on September 12, 2020: member

Fair point. I should have written “You don’t want to use clearnet at all and you don’t trust exit node operators” :)

Ah, and are thus only connecting to the ~89 fixed seeds with onion addresses, and not going via exit nodes at all? That makes more sense.

I think that currently, if you do -onlynet=onion -listen=0 -dnsseed=0 -addnode=mytrustedpeer.onion, and you successfully connect to mytrustedpeer.onion within 60 seconds, you won’t add the fixed seeds (your addrman will contain mypeer.onion rather than being empty), whereas with this change you will add the fixed seeds. Not sure that’s a change for the better?

Perhaps rather than a time check, it would be better to check for something like “if addrman is empty, there are no ADDR_FETCH nodes left to try, no open connections and no added nodes to keep trying” ?

OTOH, if it is a change for the better, it might make sense to move the add fixed nodes code to its own function, and call it from ThreadDNSAddressSeed directly (after trying all the dns seeds, with addrman still empty, and only after the opencon thread as processed any ADDR_FETCH nodes that got queued up), and from CConnman::Start when -dnsseed=0 and addrman is empty. That way you avoid unnecessary waiting in all cases, I think.

(edit: ADDR_FETCH not oneshot)

DrahtBot commented at 1:53 pm on September 19, 2020: 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:

#21015 (Make all of net_processing (and some of net) use std::chrono types by dhruv)
#20729 (p2p: standardize outbound full/block relay connection type naming by jonatack)
#20018 (p2p: ProcessAddrFetch(-seednode) is unnecessary if -connect is specified by dhruv)

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.

dhruv force-pushed on Oct 1, 2020

dhruv commented at 5:59 pm on October 1, 2020: member

My apologies for the delay in response. I needed to study all the ways seeding is done before I felt like I could have an opinion. I audited the code and found 6 ways addrman grows. I want to document them here for future readers. Reviewers well-versed in seeding can skip to the next section.

Peer seeding sources

-connect=node: If the user runs the node in this configuration, bitcoind will only ever make outbound connections with nodes specified in -connect. It will still send getaddr messages to them and addrman will grow, but no other outbound connections will be made. -dnsseed and -listen are disabled unless overridden by the user.
peers.dat: addrman flushes to peers.dat. When not in -connect mode, the node will use it’s pre-existing knowledge of possible peers from peers.dat. Each peer will be queried using getaddr and addrman will grow further.
-dnsseed: defaults to true. Uses some domain names owned by Bitcoin contributors to expand the peers in addrman. Domains can be found in CMainParams. If -proxy is set, or if the domain does not support x9.domain (which tests for NODE_NETWORK | NODE_WITNESS), getaddr messages are sent to the dns seed itself as a peer. i.e. connections are of type ADDR_FETCH to an address like dnsseed.emzy.de. Otherwise (and this is mostly the case), DNS resolved addresses are used as peer candidates in addrman. So, sometimes, DNS seeds act as seednodes (eg. -proxy) requiring a further getaddr message, and sometimes, they directly dns resolve to peers which support NODE_NETWORK | NODE_WITNESS.
-seednode=node: User-specified ADDR_FETCH connections.
-addnode=node: Like -connect, except when addrman grows, new outbound peers will be established. (This won’t happen if -connect is also specified).
Fixed seeds: If all else fails, there are hardcoded seeds in chainparamsseeds.h that the node can try to seed peers from. It’s important to have a good definition of “when all else fails”.

Continued PR discussion

-connect is not relevant as seeding does not really matter in that case.
We are solving for the case when peers.dat is empty
We are solving for the case when -dnsseed=0
That leaves us to deal with -seednode and -addnode @ajtowns has made a great observation that in the case that -seednode or -addnode are provided, removing the 60 second delay before trying the fixed seeds makes it so that we are relying on a last-resort mechanism before giving the nodes in -seednode and -addnode a chance to help us grow addrman. I’ve updated the code to reflect this. We will now try to use fixed seeds when:

0-connect is not provided
1
2AND peers.dat is empty
3
4AND ((-dnsseed=0 AND all_ADDR_FETCH_processed AND -addnode=[]) OR 60 seconds have passed)

This gives DNS seeds(ADDR_FETCH), -seednode connections(also ADDR_FETCH) and -addnode peers 60 seconds to help us find peers IF they are specified. Note that we explicitly need to check if -dnsseed=0 even though it may seem like all_ADDR_FETCH_processed covers that case. This is because when -dnsseed=1, ThreadDNSAddressSeed will add ADDR_FETCH connections almost instantaneously, but ThreadOpenConnections may check this condition before the DNS seeds are added. @ajtowns Please note that I was not able to do this:

OTOH, if it is a change for the better, it might make sense to move the add fixed nodes code to its own function, and call it from ThreadDNSAddressSeed directly (after trying all the dns seeds, with addrman still empty, and only after the opencon thread as processed any ADDR_FETCH nodes that got queued up), and from CConnman::Start when -dnsseed=0 and addrman is empty. That way you avoid unnecessary waiting in all cases, I think.

because I don’t understand how we decide we are giving up on -addnode manual connections being able to grow addrman without a timeout (in this case, 60 seconds).

This PR is ready for re-review.

dhruv requested review from practicalswift on Oct 3, 2020

dhruv requested review from jonatack on Oct 3, 2020

luke-jr commented at 2:58 pm on October 24, 2020: member

addnode is also a RPC method. It is quite plausible that an external peer discovery mechanism might launch bitcoind and use it to get connections started within that 60 seconds…

ajtowns commented at 3:24 am on October 26, 2020: member

it might make sense to move the add fixed nodes code to its own function, …

I was thinking something like https://github.com/ajtowns/bitcoin/commits/202010-fixed-seeds

I don’t understand how we decide we are giving up on -addnode manual connections being able to grow addrman without a timeout (in this case, 60 seconds).

The above makes adding fixed seeds always be an immediate decision – either because it’s the only way of connecting to the network at startup, or because you tried DNS seeds and they all failed. If you want to disable that because you’re doing -addnode later via RPC, you could specify -dnsseed=0 -seednode=127.0.0.1, though perhaps adding a -fixedseeds=0 param would be better.

I think the argument for removing the timeout is just that if you end up adding fixed seeds, the entire time before you did that is wasted because you weren’t able to connect to the p2p network (if you were able to, you wouldn’t have ended up adding the fixed seeds), so rather than waste people’s time, just make the decision earlier.

I think that’s probably better behaviour for the edge cases too – if you’re doing -addnode to a trusted peer in order to avoid untrusted seeds, then you probably don’t want to fall back to the untrusted peers if you happen to not be able to connect to your trusted peer in the first minute of trying.

in src/net.cpp:1832 in c731275ed0 outdated

1825@@ -1826,10 +1826,10 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
1826     }
1827 
1828     // Initiate network connections
1829-    int64_t nStart = GetTime();
1830+    auto nStart = GetTime<std::chrono::seconds>();
1831 
1832     // Minimum time before next feeler connection (in microseconds).
1833-    int64_t nNextFeeler = PoissonNextSend(nStart*1000*1000, FEELER_INTERVAL);
1834+    int64_t nNextFeeler = PoissonNextSend(nStart.count() * 1000 * 1000, FEELER_INTERVAL);

ajtowns commented at 3:25 am on October 26, 2020:

Use count_microseconds

dhruv commented at 7:33 pm on November 28, 2020:

Done

in src/net.cpp:1808 in c731275ed0 outdated

1862+                addFixedSeeds = true;
1863+                LogPrintf("Adding fixed seeds as 60 seconds have passed and addrman is empty.\n");
1864+            }
1865+
1866+            const bool dnsseed = gArgs.GetBoolArg("-dnsseed", DEFAULT_DNSSEED);
1867+            // Checking !dnsseed is cheaper before locking 2 mutexes.

ajtowns commented at 3:28 am on October 26, 2020:

GetBoolArg takes the cs_args lock, so probably better to move this check outside of the loop if you care about that

dhruv commented at 7:33 pm on November 28, 2020:

Done

in src/net.cpp:1862 in c731275ed0 outdated

1865+
1866+            const bool dnsseed = gArgs.GetBoolArg("-dnsseed", DEFAULT_DNSSEED);
1867+            // Checking !dnsseed is cheaper before locking 2 mutexes.
1868+            if (!addFixedSeeds && !dnsseed) {
1869+                LOCK2(m_addr_fetches_mutex, cs_vAddedNodes);
1870+                if(!dnsseed && m_addr_fetches.empty() && vAddedNodes.empty()) {

ajtowns commented at 3:28 am on October 26, 2020:

if ( – needs a space

laanwj commented at 10:00 am on October 27, 2020: member

Concept ACK.

dhruv force-pushed on Nov 28, 2020

dhruv commented at 7:53 pm on November 28, 2020: member

Thank you, for the review and for taking the time to teach me via your branch, @ajtowns.

I’ve added a new command line flag: -fixedseeds. A user can run -dnsseed=0 -fixedseeds=0 and then issue a addnode RPC to satisfy the use case @luke-jr mentioned. -dnsseed=0 -fixedseeds=0 -addnode=X will provide the trusted peer only behavior @ajtowns mentioned. I also added a functional test.

I definitely see the appeal of not having any wait, but I chose not to split up adding fixed seeds into CConnman::Start and ThreadDNSAddressSeed yet. If the user runs -dnsseed=0 -seednode=X or -dnsseed=0 -addnode=X, CConnMan::Start will not add fixed seeds, and ThreadDNSAddressSeed will not run at all. Then, in case the node at X is unreachable/censored, our node would never fall back to fixed seeds. This would be a departure from how things work today as fallback to untrusted peers is the default behavior. Perhaps we can allow for the trusted-peer-only behavior using -dnsseed=0 -fixedseeds=0 -addnode=X, but not impose it? In addition, -connect already provides a similar and more limited option. If other reviewers also agree that we should default to the trusted-peer-only behavior, I am open to changing it.

The changes to use chrono for feelers are included in #20044 and nicely bundled with other chrono types, so I did not include it.

Ready for further review.

dhruv force-pushed on Nov 28, 2020

DrahtBot added the label Needs rebase on Dec 11, 2020

dhruv force-pushed on Dec 11, 2020

dhruv commented at 3:57 pm on December 11, 2020: member

Rebased

DrahtBot removed the label Needs rebase on Dec 11, 2020

in src/init.cpp:438 in eb1eef4e21 outdated

434@@ -435,8 +435,9 @@ void SetupServerArgs(NodeContext& node)
435     argsman.AddArg("-connect=<ip>", "Connect only to the specified node; -noconnect disables automatic connections (the rules for this peer are the same as for -addnode). This option can be specified multiple times to connect to multiple nodes.", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
436     argsman.AddArg("-discover", "Discover own IP addresses (default: 1 when listening and no -externalip or -proxy)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
437     argsman.AddArg("-dns", strprintf("Allow DNS lookups for -addnode, -seednode and -connect (default: %u)", DEFAULT_NAME_LOOKUP), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
438-    argsman.AddArg("-dnsseed", "Query for peer addresses via DNS lookup, if low on addresses (default: 1 unless -connect used)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
439+    argsman.AddArg("-dnsseed", strprintf("Query for peer addresses via DNS lookup, if low on addresses (default: %u unless -connect used)", DEFAULT_DNSSEED), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);

LarryRuane commented at 11:20 pm on January 4, 2021:

0    argsman.AddArg("-dnsseed", strprintf("Query for peer addresses via DNS lookup, if low on addresses (default: %u unless -connect used)", DEFAULT_DNSSEED), ArgsManager::ALLOW_BOOL, OptionsCategory::CONNECTION);

dhruv commented at 1:33 am on February 6, 2021:

Done. Thanks!

in src/init.cpp:440 in eb1eef4e21 outdated

434@@ -435,8 +435,9 @@ void SetupServerArgs(NodeContext& node)
435     argsman.AddArg("-connect=<ip>", "Connect only to the specified node; -noconnect disables automatic connections (the rules for this peer are the same as for -addnode). This option can be specified multiple times to connect to multiple nodes.", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
436     argsman.AddArg("-discover", "Discover own IP addresses (default: 1 when listening and no -externalip or -proxy)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
437     argsman.AddArg("-dns", strprintf("Allow DNS lookups for -addnode, -seednode and -connect (default: %u)", DEFAULT_NAME_LOOKUP), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
438-    argsman.AddArg("-dnsseed", "Query for peer addresses via DNS lookup, if low on addresses (default: 1 unless -connect used)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
439+    argsman.AddArg("-dnsseed", strprintf("Query for peer addresses via DNS lookup, if low on addresses (default: %u unless -connect used)", DEFAULT_DNSSEED), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
440     argsman.AddArg("-externalip=<ip>", "Specify your own public address", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
441+    argsman.AddArg("-fixedseeds", strprintf("Allow fixed seeds if DNS seeds don't provide peers (default: %u)", DEFAULT_FIXEDSEEDS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);

LarryRuane commented at 11:20 pm on January 4, 2021:

0    argsman.AddArg("-fixedseeds", strprintf("Allow fixed seeds if DNS seeds don't provide peers (default: %u)", DEFAULT_FIXEDSEEDS), ArgsManager::ALLOW_BOOL, OptionsCategory::CONNECTION);

dhruv commented at 1:33 am on February 6, 2021:

Done

in src/net.cpp:1907 in eb1eef4e21 outdated

1910-        if (addrman.size() == 0 && (GetTime() - nStart > 60)) {
1911-            static bool done = false;
1912-            if (!done) {
1913-                LogPrintf("Adding fixed seed nodes as DNS doesn't seem to be available.\n");
1914+        // Static bool makes sure we only add fixed seeds once in binary lifetime.
1915+        static bool fixedSeedsDone = disableFixedSeeds;

LarryRuane commented at 1:04 am on January 5, 2021:

I like this renaming of the variable done to fixedSeedsDone, but maybe we can take the opportunity to remove this static? Static variables are scope-limited global variables (in general, globals should be avoided).

 0-    const bool disableFixedSeeds = !gArgs.GetBoolArg("-fixedseeds", DEFAULT_FIXEDSEEDS);
 1+    bool addFixedSeeds = gArgs.GetBoolArg("-fixedseeds", DEFAULT_FIXEDSEEDS);
 2 
 3-    if (disableFixedSeeds) {
 4+    if (!addFixedSeeds) {
 5         LogPrintf("Fixed seeds are disabled\n");
 6     }
 7 
 8     while (!interruptNet)
 9     {
10         ProcessAddrFetch();
11 
12         if (!interruptNet.sleep_for(std::chrono::milliseconds(500)))
13             return;
14 
15         CSemaphoreGrant grant(*semOutbound);
16         if (interruptNet)
17             return;
18 
19         // Static bool makes sure we only add fixed seeds once in binary lifetime.
20-        static bool fixedSeedsDone = disableFixedSeeds;
21-        if (!fixedSeedsDone && addrman.size() == 0) {
22+        if (addFixedSeeds && addrman.size() == 0) {
23             // When the node starts with an empty peers.dat, there are a few other sources of peers before
24             // we fallback on to fixed seeds: -dnsseed, -seednode, -addnode
25             // If none of those are available, we fallback on to fixed seeds immediately, else we allow
26             // 60 seconds for any of those sources to populate addrman.
27-            bool addFixedSeeds = false;
28+            bool addFixedSeedsNow = false;
29             // It is cheapest to check if enough time has passed first.
30             if (GetTime<std::chrono::seconds>() > nStart + std::chrono::minutes{1}) {
31-                addFixedSeeds = true;
32+                addFixedSeedsNow = true;
33                 LogPrintf("Adding fixed seeds as 60 seconds have passed and addrman is empty\n");
34             }
35 
36             // Checking !dnsseed is cheaper before locking 2 mutexes.
37-            if (!addFixedSeeds && !dnsseed) {
38+            if (!addFixedSeedsNow && !dnsseed) {
39                 LOCK2(m_addr_fetches_mutex, cs_vAddedNodes);
40                 if (m_addr_fetches.empty() && vAddedNodes.empty()) {
41-                    addFixedSeeds = true;
42+                    addFixedSeedsNow = true;
43                     LogPrintf("Adding fixed seeds as -dnsseed=0, -addnode is not provided and and all -seednode(s) attempted\n");
44                 }
45             }
46 
47-            if (addFixedSeeds) {
48+            if (addFixedSeedsNow) {
49                 CNetAddr local;
50                 local.SetInternal("fixedseeds");
51                 addrman.Add(convertSeed6(Params().FixedSeeds()), local);
52-                fixedSeedsDone = true;
53+                addFixedSeeds = false;
54             }
55         }

Should new variables be written in snake_case?

dhruv commented at 1:34 am on February 6, 2021:

Done! I too prefer the elimination of the static variable. Wasn’t sure reviewers would be keen on it, but I agree it is cleaner.

in test/functional/feature_config_args.py:160 in eb1eef4e21 outdated

155+        # So after 60 seconds, the node should fallback to fixed seeds (this is a slow test)
156+        assert not os.path.exists(os.path.join(default_data_dir, "peers.dat"))
157+        start = time.time()
158+        with self.nodes[0].assert_debug_log(expected_msgs=["Loaded 0 addresses from peers.dat",
159+                "0 addresses found from DNS seeds", "Adding fixed seeds as 60 seconds have passed and addrman is empty"],
160+                timeout=80):

LarryRuane commented at 1:16 am on January 5, 2021:

0        with self.nodes[0].assert_debug_log(expected_msgs=[
1                "Loaded 0 addresses from peers.dat",
2                "0 addresses found from DNS seeds",
3                "Adding fixed seeds as 60 seconds have passed and addrman is empty",
4            ],
5                timeout=80):

(and similarly below)

dhruv commented at 1:34 am on February 6, 2021:

Done.

in test/functional/feature_config_args.py:150 in eb1eef4e21 outdated

145@@ -145,11 +146,61 @@ def test_networkactive(self):
146             self.start_node(0, extra_args=['-nonetworkactive=1'])
147         self.stop_node(0)
148 
149+    def test_seed_peers(self):
150+        self.log.info('Test seed peers')

LarryRuane commented at 1:23 am on January 5, 2021:

0        self.log.info('Test seed peers, this will take about 2 minutes')

in test/functional/feature_config_args.py:175 in eb1eef4e21 outdated

166+        # We expect the node will fallback immediately to fixed seeds
167+        assert not os.path.exists(os.path.join(default_data_dir, "peers.dat"))
168+        start = time.time()
169+        with self.nodes[0].assert_debug_log(expected_msgs=["Loaded 0 addresses from peers.dat",
170+                "DNS seeding disabled",
171+                "Adding fixed seeds as -dnsseed=0, -addnode is not provided and and all -seednode(s) attempted\n"]):

LarryRuane commented at 1:27 am on January 5, 2021:

You can also check for the absence of debug.log messages (I’m not sure if it’s worth it in this case):

0        with self.nodes[0].assert_debug_log(expected_msgs=[
1                "Loaded 0 addresses from peers.dat",
2                "DNS seeding disabled",
3                "Adding fixed seeds as -dnsseed=0, -addnode is not provided and and all -seednode(s) attempted\n"
4            ], unexpected_msgs=["60 seconds"]):

dhruv commented at 1:35 am on February 6, 2021:

Changed the indentation. I did not add the unexpected_msgs=["60 seconds"] as it is implicit in the 2 second timeout for assert_debug_log.

LarryRuane commented at 1:39 am on January 5, 2021: contributor

As suggested in the description, I tested:

0rm ~/.bitcoin/peers.dat && src/bitcoind -dnsseed=0

without the patch (note the timestamps):

02021-01-02T00:00:49Z msghand thread start
12021-01-02T00:01:50Z Adding fixed seed nodes as DNS doesn't seem to be available.

With the patch:

02021-01-02T00:05:38Z msghand thread start
12021-01-02T00:05:38Z Adding fixed seeds as -dnsseed=0, -addnode is not provided and and all -seednode(s) attempted

Ran clang-format-diff.py (it found nothing). Nice PR! Suggestions are minor and optional. ACK eb1eef4e21fda019a9d309656aac1ffaf04b262d (but take with a grain of salt; I don’t know this area of the code well.)

dhruv force-pushed on Feb 6, 2021

dhruv commented at 1:36 am on February 6, 2021: member

Thanks for the review @LarryRuane, and my apologies for the long wait.

Comments addressed. Rebased. Ready for further review.

dhruv force-pushed on Feb 6, 2021

dhruv commented at 1:46 am on February 6, 2021: member

Fixed failing linter.

LarryRuane commented at 2:39 am on February 11, 2021: contributor

re-reviewed, re-tested ACK 3fdea1f77b7afa07789139f17c0e03ee55f3ee02

laanwj commented at 6:23 pm on February 11, 2021: member

Code review ACK 3fdea1f77b7afa07789139f17c0e03ee55f3ee02

[p2p] No delay in adding fixed seeds if -dnsseed=0 and peers.dat is empty. Add -fixedseeds arg. fe3e993968

in test/functional/feature_config_args.py:177 in 3fdea1f77b outdated

172+        with self.nodes[0].assert_debug_log(expected_msgs=[
173+                "Loaded 0 addresses from peers.dat",
174+                "DNS seeding disabled",
175+                "Adding fixed seeds as -dnsseed=0, -addnode is not provided and and all -seednode(s) attempted\n"]):
176+            self.start_node(0, extra_args=['-dnsseed=0'])
177+        assert time.time() - start < 5

laanwj commented at 6:25 pm on February 11, 2021:

It’s slightly risky to have a test that depends on real time instead of mocked times. CI environments can be extremely slow, so this bound might introduce random failures.

dhruv commented at 0:12 am on February 12, 2021:

You’re right - that could make for a flaky test. I picked 5 seconds as an arbitrary bound. What I really wanted to check was that the node did not wait 60 seconds to use fixed seeds.

0        assert time.time() - start < 60

accomplished that. And if CI is so slow that 60 seconds pass, the test will fail anyway because the logs will say “Adding fixed seeds as 60 seconds have passed and addrman is empty” instead of “Adding fixed seeds as -dnsseed=0, -addnode is not provided and and all -seednode(s) attempted”.

laanwj commented at 10:37 am on February 12, 2021:

Thanks, good idea!

In the longer term I think we should change this to use mock time but for this PR that would be too much of a hassle.

dhruv force-pushed on Feb 12, 2021

LarryRuane commented at 0:23 am on February 12, 2021: contributor

re-ACK fe3e993968d6b46777d5a16a662cd22790ddf5bb

dhruv commented at 2:42 am on February 12, 2021: member

Updated tests for CI stability. Ready for further review.

laanwj commented at 10:48 am on February 12, 2021: member

re-ACK fe3e993968d6b46777d5a16a662cd22790ddf5bb checked that the only change since my last ACK is the < time bounds for the test

laanwj merged this on Feb 12, 2021

laanwj closed this on Feb 12, 2021

sidhujag referenced this in commit 255abc22a9 on Feb 12, 2021

in test/functional/feature_config_args.py:175 in fe3e993968

170+        assert not os.path.exists(os.path.join(default_data_dir, "peers.dat"))
171+        start = time.time()
172+        with self.nodes[0].assert_debug_log(expected_msgs=[
173+                "Loaded 0 addresses from peers.dat",
174+                "DNS seeding disabled",
175+                "Adding fixed seeds as -dnsseed=0, -addnode is not provided and and all -seednode(s) attempted\n"]):

MarcoFalke commented at 4:13 pm on February 12, 2021:

and and

dhruv commented at 5:40 pm on February 12, 2021:

Please see #21165

in test/functional/feature_config_args.py:165 in fe3e993968

160+        with self.nodes[0].assert_debug_log(expected_msgs=[
161+                "Loaded 0 addresses from peers.dat",
162+                "0 addresses found from DNS seeds",
163+                "Adding fixed seeds as 60 seconds have passed and addrman is empty"], timeout=80):
164+            self.start_node(0, extra_args=['-dnsseed=1'])
165+        assert time.time() - start >= 60

MarcoFalke commented at 4:14 pm on February 12, 2021:

wouldn’t it work to use setmocktime inside the context manager above and check here: node.uptime() >= 60 to speed up the test?

jonatack commented at 4:20 pm on February 12, 2021:

I looked at this last September (https://github.com/bitcoin/bitcoin/pull/19884#pullrequestreview-486061184) and it didn’t look feasible, interesting if it is.

MarcoFalke commented at 5:03 pm on February 12, 2021:

 0$ time test/functional/feature_config_args.py 
 12021-02-12T17:02:39.647000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_kuphxowf
 22021-02-12T17:02:45.294000Z TestFramework (INFO): Test config args logging
 32021-02-12T17:02:45.605000Z TestFramework (INFO): Test seed peers, this will take about 2 minutes
 42021-02-12T17:02:48.587000Z TestFramework (INFO): Test -networkactive option
 52021-02-12T17:02:54.023000Z TestFramework (INFO): Stopping nodes
 62021-02-12T17:02:54.127000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_kuphxowf on exit
 72021-02-12T17:02:54.127000Z TestFramework (INFO): Tests successful
 8
 9real	0m14.682s
10user	0m3.842s
11sys	0m0.745s

dhruv commented at 5:40 pm on February 12, 2021:

Please see #21165

MarcoFalke referenced this in commit c0e44ee8e4 on Feb 25, 2021

sidhujag referenced this in commit bcf12e4943 on Feb 25, 2021

luke-jr referenced this in commit 0169da9fee on Jun 27, 2021

Fabcien referenced this in commit 6974a58c04 on Jan 27, 2022

DrahtBot locked this on Aug 16, 2022

p2p: No delay in adding fixed seeds if -dnsseed=0 and peers.dat is empty #19884

Conflicts

Peer seeding sources

Continued PR discussion