Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't query all DNS seeds at once #15558

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
10 participants
@sipa
Copy link
Member

commented Mar 7, 2019

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

@fanquake fanquake added the P2P label Mar 7, 2019

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #14856 (net: remove more CConnman globals (theuni) by dongcarl)

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.

Do not query all DNS seed at once
Instead, when necessary, query 3. If that leads to a sufficient number
of connects, stop. If not, query 3 more, and so on.

@sipa sipa force-pushed the sipa:201903_dnsoneatatime branch from 70d24bd to 9f36b04 Mar 7, 2019

@sipa sipa changed the title Query DNS seeds one by one Don't query all DNS seeds at on ce Mar 8, 2019

@fanquake fanquake changed the title Don't query all DNS seeds at on ce Don't query all DNS seeds at once Mar 8, 2019

@luke-jr

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

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?

@jimpo

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

3 seems like an OK number to query for at once, but a reasonable thing to bikeshed a bit.

Code looks good to me.

@gmaxwell

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

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.

@laanwj

This comment has been minimized.

Copy link
Member

commented Mar 9, 2019

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.

@gmaxwell

This comment has been minimized.

Copy link
Member

commented Mar 9, 2019

It may just be that we need to try connections faster initially. ... I dunno I really have the impression that it was frequently not using dns seeds in the past but is now, but I don't have hard data to support that. In theory feelers should mean that our tried table is better than ever before, but it might just be that there has been a lot of node churn lately.

@jgarzik

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2019

When the code was added circa 2014 in 2e7009d, 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.

@luke-jr

This comment has been minimized.

Copy link
Member

commented Mar 10, 2019

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.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

Concept ACK

@Sjors

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

Concept ACK with > 1 seed per round.

utACK 9f36b04 (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.

@luke-jr
Copy link
Member

left a comment

utACK, with a couple of minor suggestions

return;
}
if (gArgs.GetBoolArg("-forcednsseed", DEFAULT_FORCEDNSSEED)) {
seeds_right_now += DNSSEEDS_TO_QUERY_AT_ONCE;

This comment has been minimized.

Copy link
@luke-jr

luke-jr Apr 17, 2019

Member

If the user specifies -forcednsseed, they might want/expect all of them to be queried?

}

const std::vector<std::string> &vSeeds = Params().DNSSeeds();
int found = 0;
for (const auto& seed : seeds) {

This comment has been minimized.

Copy link
@luke-jr

luke-jr Apr 17, 2019

Member

I think this might be more readable if the std::string type remained explicit here (since we LogPrintf it below).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.