Before this PR, when we don’t have enough connections after 11 seconds, we proceed to query all DNS seeds in a fixed order, loading responses from all of them.
Change this to to only query three randomly-selected DNS seed. If 11 seconds later we still don’t have enough connections, try again with another one, and so on.
This reduces the amount of information DNS seeds can observe about the requesters by spreading the load over all of them.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
Wouldn’t this mean if one DNS seed is compromised, it could potentially feed sybil addresses to nodes and its victims wouldn’t have any honest nodes (eg, from other DNS seeds)?
Does the issue it intends to address actually exist? I would think DNS queries would get cached and proxied by ISP DNS servers sufficiently to deny any information to the real DNS servers, no?
3 seems like an OK number to query for at once, but a reasonable thing to bikeshed a bit.
Code looks good to me.
Yeah, my comment in IRC was that one creates an immediate eclipse vulnerability. Three sounds reasonable.
We also really should look at why this is triggered as much as it is, locally I see it run on every restart these days.
Concept ACK.
Yeah, my comment in IRC was that one creates an immediate eclipse vulnerability. Three sounds reasonable.
But only for new nodes, I suppose? If it already has gossiped peers before, then ‘query one DNS seed’ seems more reasonable. But okay, 3 is a safer option in any case.
We also really should look at why this is triggered as much as it is, locally I see it run on every restart these days.
My guess would be that the peers it tries initially to connect to might not be selected for connectability. Maybe 11 seconds is also too little. I think it would be fine to increase this to say, 30 or even 60 seconds.
When the code was added circa 2014 in 2e7009d67b862cf822a1c70e181de6af659a3096, 11 seconds was completely arbitrary. It was always the intent to adjust that number based on field experience. It sounds like it needs to increase.
The driving theory is to make DNS seeds a fallback and “give it a good try” to avoid querying DNS seeds at all.
The code already skips the delay for an empty AddrMan – ie. fresh bitcoind install – thus narrowing the user impact of a longer delay to existing nodes that presumably have addrman data to sort through.
It boils down to a question of how much dependence should a node have on DNS seeds, in general? 300 seconds is a reasonable answer for the new delay, if we want to give AddrMan time to “warm up” and preserve privacy a bit more.
It boils down to a question of how much dependence should a node have on DNS seeds, in general?
IMO nodes should only consult DNS seeds at first run, and then never again.
Concept ACK with > 1 seed per round.
utACK 9f36b04fa039d4ddbd9a4b559a62a5d6e1a873e5 (3 seeds)
The frequent DNS queries recently have hopefully been addressed with #15486.
IMO nodes should only consult DNS seeds at first run, and then never again.
I like that idea as well, but for another PR, as it requires much more thought than this change.
11 seconds was completely arbitrary. It was always the intent to adjust that number based on field experience. It sounds like it needs to increase
As a GUI use I’d get antsy if “nothing happens” for 15+ seconds, but as long as there’s some indicator that we’re trying various new peers that should be fine. In that case we can wait much longer before trying the DNS again.
1546- if (nRelevant >= 2) {
1547- LogPrintf("P2P peers available. Skipped DNS seeding.\n");
1548- return;
1549- }
1550+ if (gArgs.GetBoolArg("-forcednsseed", DEFAULT_FORCEDNSSEED)) {
1551+ seeds_right_now += DNSSEEDS_TO_QUERY_AT_ONCE;
-forcednsseed, they might want/expect all of them to be queried?
1551+ seeds_right_now += DNSSEEDS_TO_QUERY_AT_ONCE;
1552 }
1553
1554- const std::vector<std::string> &vSeeds = Params().DNSSeeds();
1555- int found = 0;
1556+ for (const auto& seed : seeds) {
@naumenkogs Did you want to take a look here?
This has a number of Concept ACKs, have tagged with 0.19.0.
First of all, could we update the initial message of the PR so that it’s clear that it suggests connecting to 3 (not one) without reading full discussion? Maybe strike-through would work.
The choice of 3 seeds and the general idea seems reasonable to me. utACK
Instead, when necessary, query 3. If that leads to a sufficient number
of connects, stop. If not, query 3 more, and so on.
ACK 6170ec5d3
With -forcednsseed=1 it now fetches all (testnet) seeds.
When I restart it sometimes fetches (3) seeds, about 10 seconds after dnsseed thread start. Other times it finds several outbound nodes and the DNS seed exited.
When I delete peers.dat it uses all DNS seeds.
1540 }
1541
1542- const std::vector<std::string> &vSeeds = Params().DNSSeeds();
1543- int found = 0;
1544+ for (const std::string& seed : seeds) {
1545+ // goal: only query DNS seed if address need is acute
goal: only query DNS seed if address need is acute
From my understanding, “acute” here is essentially on first run, or if you’ve got a peers.dat with low quality/unlikely to connect quickly peers?
ACK 6170ec5d3ac2bc206068b270e5722a7ecd3a8f26 - Agree with the reasoning behind the change. Did some testing with and without -forcednsseed and/or a peers.dat and monitored the DNS activity.
0src/bitcoind -debug=net | grep -i -E 'dns|net|p2p|peers'
12019-09-23T04:36:24Z Loaded 0 banned node ips/subnets from banlist.dat 0ms
22019-09-23T04:36:24Z net: setting try another outbound peer=false
32019-09-23T04:36:29Z init message: Loading P2P addresses...
42019-09-23T04:36:29Z Loaded 6640 addresses from peers.dat 18ms
52019-09-23T04:36:29Z init message: Starting network threads...
62019-09-23T04:36:29Z net thread start
72019-09-23T04:36:29Z dnsseed thread start
82019-09-23T04:36:40Z P2P peers available. Skipped DNS seeding.
92019-09-23T04:36:40Z dnsseed thread exit
0src/bitcoind -debug=net | grep -i -E 'dns|net|p2p|peers'
12019-09-23T04:20:17Z Loaded 0 banned node ips/subnets from banlist.dat 0ms
22019-09-23T04:20:17Z net: setting try another outbound peer=false
32019-09-23T04:20:22Z init message: Loading P2P addresses...
42019-09-23T04:20:22Z Loaded 2863 addresses from peers.dat 7ms
52019-09-23T04:20:22Z init message: Starting network threads...
62019-09-23T04:20:22Z net thread start
72019-09-23T04:20:22Z dnsseed thread start
82019-09-23T04:20:33Z Loading addresses from DNS seed seed.btc.petertodd.org
92019-09-23T04:20:33Z Loading addresses from DNS seed dnsseed.emzy.de
102019-09-23T04:20:33Z Loading addresses from DNS seed dnsseed.bluematt.me
112019-09-23T04:20:44Z P2P peers available. Skipped DNS seeding.
122019-09-23T04:20:44Z dnsseed thread exit
0src/bitcoind -forcednsseed -debug=net | grep -i -E 'dns|net|p2p|peers'
12019-09-23T04:09:52Z Loaded 0 banned node ips/subnets from banlist.dat 0ms
22019-09-23T04:09:52Z net: setting try another outbound peer=false
32019-09-23T04:09:58Z init message: Loading P2P addresses...
42019-09-23T04:09:58Z Loaded 9738 addresses from peers.dat 23ms
52019-09-23T04:09:58Z init message: Starting network threads...
62019-09-23T04:09:58Z net thread start
72019-09-23T04:09:58Z dnsseed thread start
82019-09-23T04:09:58Z Loading addresses from DNS seed seed.bitcoin.sipa.be
92019-09-23T04:09:58Z Loading addresses from DNS seed seed.btc.petertodd.org
102019-09-23T04:09:58Z Loading addresses from DNS seed seed.bitcoinstats.com
112019-09-23T04:09:58Z Loading addresses from DNS seed dnsseed.bitcoin.dashjr.org
122019-09-23T04:09:58Z Loading addresses from DNS seed seed.bitcoin.jonasschnelli.ch
132019-09-23T04:09:58Z Loading addresses from DNS seed seed.bitcoin.sprovoost.nl
142019-09-23T04:09:58Z Loading addresses from DNS seed dnsseed.bluematt.me
152019-09-23T04:09:59Z Loading addresses from DNS seed dnsseed.emzy.de
162019-09-23T04:09:59Z 294 addresses found from DNS seeds
172019-09-23T04:09:59Z dnsseed thread exit
0src/bitcoind -forcednsseed -debug=net | grep -i -E 'dns|net|p2p|peers'
12019-09-23T04:11:34Z Loaded 0 banned node ips/subnets from banlist.dat 0ms
22019-09-23T04:11:34Z net: setting try another outbound peer=false
32019-09-23T04:11:39Z init message: Loading P2P addresses...
42019-09-23T04:11:39Z ERROR: DeserializeFileDB: Failed to open file /Users/michael/Library/Application Support/Bitcoin/peers.dat
52019-09-23T04:11:39Z Invalid or missing peers.dat; recreating
62019-09-23T04:11:39Z Flushed 0 addresses to peers.dat 4ms
72019-09-23T04:11:39Z init message: Starting network threads...
82019-09-23T04:11:39Z dnsseed thread start
92019-09-23T04:11:39Z net thread start
102019-09-23T04:11:39Z Loading addresses from DNS seed seed.bitcoin.sprovoost.nl
112019-09-23T04:11:39Z Loading addresses from DNS seed dnsseed.emzy.de
122019-09-23T04:11:39Z Loading addresses from DNS seed seed.bitcoin.jonasschnelli.ch
132019-09-23T04:11:39Z Loading addresses from DNS seed seed.bitcoin.sipa.be
142019-09-23T04:11:39Z Loading addresses from DNS seed seed.bitcoinstats.com
152019-09-23T04:11:40Z Loading addresses from DNS seed dnsseed.bluematt.me
162019-09-23T04:11:40Z Loading addresses from DNS seed dnsseed.bitcoin.dashjr.org
172019-09-23T04:11:40Z Loading addresses from DNS seed seed.btc.petertodd.org
182019-09-23T04:11:40Z 294 addresses found from DNS seeds
192019-09-23T04:11:40Z dnsseed thread exit
One high-level concern with this is the monoculture of DNS seed software we have, if you select three at random you’re almost certain to get three seeds serving from sipa’s seeder implementation, whereas we should really be trying to diversify a little bit (not to mention get things like dnssec going, which I dont believe sipa’s seeder support).
Opened #16938 if anyone wants to brainstorm / discuss DNS seeder software diversity. This PR is also related to #15434.
Change this to to only query three randomly-selected DNS seed. If 11 seconds later we still don’t have enough connections, try again with another one, and so on
I guess this should say “try again with another three”
0
12019-09-23T14:06:29Z Loading addresses from DNS seed seed.bitcoin.sipa.be
22019-09-23T14:06:29Z Loading addresses from DNS seed seed.bitcoin.sprovoost.nl
32019-09-23T14:06:29Z Loading addresses from DNS seed dnsseed.bluematt.me
4
5
6
72019-09-23T14:06:40Z Loading addresses from DNS seed seed.bitcoinstats.com
82019-09-23T14:06:40Z Loading addresses from DNS seed seed.bitcoin.jonasschnelli.ch
92019-09-23T14:06:40Z Loading addresses from DNS seed seed.btc.petertodd.org
10
11
12
132019-09-23T14:06:51Z Loading addresses from DNS seed dnsseed.bitcoin.dashjr.org
142019-09-23T14:06:51Z Loading addresses from DNS seed dnsseed.emzy.de
152019-09-23T14:06:51Z 0 addresses found from DNS seeds
162019-09-23T14:06:51Z dnsseed thread exit
1544+ for (const std::string& seed : seeds) {
1545+ // goal: only query DNS seed if address need is acute
1546+ // Avoiding DNS seeds when we don't need them improves user privacy by
1547+ // creating fewer identifying DNS requests, reduces trust by giving seeds
1548+ // less influence on the network topology, and reduces traffic to the seeds.
1549+ if (addrman.size() > 0 && seeds_right_now == 0) {
addrman.size()==0 and seeds_right_now==0 it will then query the first seed, and set seeds_right_now==-1, at which point this condition will keep failing on future loops, preventing a delay between batches of 3 seeds, and preventing early exit. I think this means that new nodes with no peers.dat will still always immediately query all dns seeds. In #16939 I’ve made the logic if (addrman==0 && seeds_right_now==0) { current code } else if (seeds_right_now == 0) { seeds_right_now += 3; } which should give more sensible behaviour.
My understanding was that this was intentional behavior (eg @Sjors points this out in #15558 (comment)), but perhaps this should be better commented.
It made sense to me that we would query all dns seeds when starting up for the first time, which is consistent with prior behavior, and that the purpose of this PR was to limit dns seed usage only in the case of nodes that have been online before.
Seems surprising to me. If it’s intentional, could simplify the logic a bit, and make it:
0if (forcednsseed || addrman.size() == 0) seeds_right_now = seeds.size();
1for (seed : seeds) {
2 if (seeds_right_now == 0) { sleep(); check(); seeds_right_now += 3; }
3 lookup();
4 --seeds_right_now;
5}
Aligning with the code comments that are quoted above + @luke-jr ’s comments in related PR, this seems like an adequate summary?
The latter point seems to maximally align with the code comment & user privacy
Labels
P2P
Milestone
0.19.0