net: avoid extra dns query per seed

theuni commented at 1:32 am on May 24, 2017: member

This addresses the TODO here: https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L1604 and is necessary before introducing async dns queries.

Right now, we resolve an extra hostname for each dns seed, so that we have some source ip to identify them with in addrman. It’s entirely arbitrary and unnecessary.

Instead, create a dummy “ip” for this association with a prefix of fd6b:88c0:8724::/48 (fd + sha256(bitcoin)[0:5]).

I’ve also changed the “source ip” to match a hash of the host with the subdomain included, rather than having all results mapped to the top-level domain.

I’m not sure whether we need to bother with addrman forwards-compatibility for graceful downgrades. Thoughts on that?

fanquake added the label P2P on May 24, 2017

laanwj commented at 10:19 am on May 24, 2017: member

Concept ACK. The six-byte hash is a good idea; I thought of replacing it with simply the fd6b:88c0:8724::<seqnr> of the DNS seed, but it needs to be compatible over time and those seeds could be added and removed so this solution is better.

gmaxwell commented at 8:54 am on May 25, 2017: contributor

Concept ACK.

TheBlueMatt commented at 8:53 pm on June 1, 2017: member

It would be kinda nice if we blocked DNS names from resolving to internal IPs, since that should never happen, and unlike local/tor/whatever, there is no reason you’d ever want to -connect to them.

theuni commented at 8:24 pm on June 2, 2017: member

@TheBlueMatt Agreed in principle. Though they do fail the IsValid test, so we’d never actually attempt a connection to them: https://github.com/bitcoin/bitcoin/pull/10446/files#diff-b64569708508232923e5fe3059396334R216

I can add explicit filtering to resolving if that would make you more comfortable?

sipa commented at 6:37 pm on June 3, 2017: member

Concept ACK, but can you adapt the touched code to the style guide?

Agree with @TheBlueMatt, but maybe we should also do that for onion addresses?

TheBlueMatt commented at 3:19 pm on June 7, 2017: member

@sipa well dns seeds and other dns queries are allowed to return bitcoin-onion-encoded results (I’ve done this as -addnode/-connect arguments in the past). @theuni Yea, I would feel a bit more comfortable doing that, thanks :).

theuni force-pushed on Jun 13, 2017

theuni commented at 10:33 pm on June 13, 2017: member

Since there have been no (ut)ACKs and there were several things to change, I went ahead and made changes, squashed, and rebased. I believe this is good to go now.

changes since the first round:

rebased/squashed
adapted code style
use sha256 rather than sha256d for name mapping, for easier sanity checking
filter out internal addresses from the resolver
minor fixes

theuni commented at 10:40 pm on June 13, 2017: member

@sipa by filtering onion addresses the same way that internal ones are done here, we’d prevent dns seeds from handing out onion addresses. I’m not sure if the current seeders include those (I think they do), but I see no reason to filter them either way.

in src/netaddress.h:51 in c992103559 outdated

45@@ -45,6 +46,12 @@ class CNetAddr
46          */
47         void SetRaw(Network network, const uint8_t *data);
48 
49+        /**
50+          * Transform an arbitrary string into a non-routable ipv6 address.
51+          * Useful for mapping resolved addresses back to thir source.

sipa commented at 11:21 pm on June 13, 2017:

VERY BIG ERROR WOW: ’thir'

theuni commented at 10:08 pm on June 14, 2017:

What, no responsible disclosure? Jeez!

sipa commented at 11:24 pm on June 13, 2017: member

utACK

net: add an internal subnet for representing unresolved hostnames

We currently do two resolves for dns seeds: one for the results, and one to
serve in addrman as the source for those addresses.

There's no requirement that the source hostname resolves to the stored
identifier, only that the mapping is unique. So rather than incurring the
second lookup, combine a private subnet with a hash of the hostname.

The resulting v6 ip is guaranteed not to be publicy routable, and has only a
negligible chance of colliding with a user's internal network (which would be
of no consequence anyway).

7f31762cb6

net: do not allow resolving to an internal address

In order to prevent mixups, our internal range is never allowed as a resolve
result. This means that no user-provided string will ever be confused with an
internal address.

6d0bd5b73d

net: switch to dummy internal ip for dns seed source

This addresss the TODO to avoid resolving twice.

6cdc488e36

theuni force-pushed on Jun 14, 2017

in src/chainparams.cpp:129 in 6cdc488e36 outdated

130-        vSeeds.push_back(CDNSSeedData("bitcoinstats.com", "seed.bitcoinstats.com", true)); // Christian Decker, supports x1 - xf
131-        vSeeds.push_back(CDNSSeedData("bitcoin.jonasschnelli.ch", "seed.bitcoin.jonasschnelli.ch", true)); // Jonas Schnelli, only supports x1, x5, x9, and xd
132-        vSeeds.push_back(CDNSSeedData("petertodd.org", "seed.btc.petertodd.org", true)); // Peter Todd, only supports x1, x5, x9, and xd
133+        vSeeds.emplace_back("seed.bitcoin.sipa.be", true); // Pieter Wuille, only supports x1, x5, x9, and xd
134+        vSeeds.emplace_back("dnsseed.bluematt.me", true); // Matt Corallo, only supports x9
135+        vSeeds.emplace_back("dnsseed.bitcoin.dashjr.org"); // Luke Dashjr

laanwj commented at 6:00 pm on June 22, 2017:

Sorry for the last minute comment, feel free to say you don’t want to do it in this PR. But as all these lines are updated I think this is a nice opportunity to remove the default argument for supportsServiceBitsFilteringIn and add explicit , false here.

theuni commented at 6:59 pm on June 22, 2017:

No problem, I found one tiny additional thing to change on this today, I’ll go ahead with your suggestion while I’m at it.

theuni commented at 8:07 pm on June 22, 2017:

Pushed a small commit for this to avoid invalidating review.

net: use an internal address for fixed seeds d5c7c1cfe3

chainparams: make supported service bits option explicit c1be285364

theuni commented at 8:10 pm on June 22, 2017: member

Added a very small commit to use an internal address for seed nodes. I believe this is ready for merge.

TheBlueMatt commented at 10:11 pm on June 23, 2017: member

utACK c1be28536467e90ce75eaa7d8c338f6485c4bee5

laanwj commented at 10:24 am on June 24, 2017: member

utACK c1be285

laanwj merged this on Jun 24, 2017

laanwj closed this on Jun 24, 2017

laanwj referenced this in commit 2772dc9f21 on Jun 24, 2017

PastaPastaPasta referenced this in commit be3d6ed4da on Jul 6, 2019

PastaPastaPasta referenced this in commit c957149b93 on Jul 8, 2019

PastaPastaPasta referenced this in commit 9a6535e299 on Jul 9, 2019

PastaPastaPasta referenced this in commit 64ef42c797 on Jul 11, 2019

barrystyle referenced this in commit d035e0c877 on Jan 22, 2020

random-zebra referenced this in commit 71275c1896 on Jun 9, 2021

DrahtBot locked this on Sep 8, 2021

net: avoid extra dns query per seed #10446