net: avoid extra dns query per seed #10446

theuni · 2017-05-24T01:32:18Z

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?

laanwj · 2017-05-24T10:19:42Z

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 · 2017-05-25T08:54:53Z

Concept ACK.

TheBlueMatt · 2017-06-01T20:53:27Z

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 · 2017-06-02T20:24:02Z

@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 · 2017-06-03T18:37:38Z

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 · 2017-06-07T15:19:52Z

@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 · 2017-06-13T22:33:29Z

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 · 2017-06-13T22:40:22Z

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

sipa

utACK

sipa · 2017-06-13T23:21:00Z

src/netaddress.h

@@ -45,6 +46,12 @@ class CNetAddr
         */
        void SetRaw(Network network, const uint8_t *data);

+        /**
+          * Transform an arbitrary string into a non-routable ipv6 address.
+          * Useful for mapping resolved addresses back to thir source.


VERY BIG ERROR WOW: 'thir'

What, no responsible disclosure? Jeez!

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

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.

This addresss the TODO to avoid resolving twice.

laanwj · 2017-06-22T18:00:13Z

src/chainparams.cpp

-        vSeeds.push_back(CDNSSeedData("petertodd.org", "seed.btc.petertodd.org", true)); // Peter Todd, only supports x1, x5, x9, and xd
+        vSeeds.emplace_back("seed.bitcoin.sipa.be", true); // Pieter Wuille, only supports x1, x5, x9, and xd
+        vSeeds.emplace_back("dnsseed.bluematt.me", true); // Matt Corallo, only supports x9
+        vSeeds.emplace_back("dnsseed.bitcoin.dashjr.org"); // Luke Dashjr


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.

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.

Pushed a small commit for this to avoid invalidating review.

theuni · 2017-06-22T20:10:45Z

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

TheBlueMatt · 2017-06-23T22:11:24Z

utACK c1be285

laanwj · 2017-06-24T10:24:41Z

utACK c1be285

c1be285 chainparams: make supported service bits option explicit (Cory Fields) d5c7c1c net: use an internal address for fixed seeds (Cory Fields) 6cdc488 net: switch to dummy internal ip for dns seed source (Cory Fields) 6d0bd5b net: do not allow resolving to an internal address (Cory Fields) 7f31762 net: add an internal subnet for representing unresolved hostnames (Cory Fields) Tree-SHA512: 9bf1042bef546ac3ef0e0d3a9a5555eb21628ff2674a0cf8c6367194b22bfdab477adf452c0e7c56f44e0fb37debc5e14bdb623452e076fb9c492c7702601d7a

fcdf87a Populate services in GetLocalAddress (Alex Morcos) a0a079f Return early in IsBanned. (Gregory Maxwell) 7772138 Remove redundant nullptr checks before deallocation (practicalswift) 5390862 net: remove unimplemented functions. (furszy) 74d0482 Limit setAskFor and retire requested entries only when a getdata returns. (Gregory Maxwell) 3b3bf63 prevent peer flooding request queue for an inv (kazcw) c814967 Fix subscript[0] potential bugs in key.cpp (Jeremy Rubin) 8e2e79e Remove unnecessary branches in utilstrencodings string constructors. (Jeremy Rubin) 25a16b3 Fix subscript[0] in utilstrencodings.cpp (Jeremy Rubin) df0584e Fix subscript[0] in streams.h (Jeremy Rubin) b7e64a5 Fix subscript[0] in validation.cpp (Jeremy Rubin) 2f7b73b Fix subscript[0] in torcontrol (Jeremy Rubin) 3b883eb Fix subscript[0] in netaddress.cpp (Jeremy Rubin) e5f5401 Fix subscript[0] in base58.cpp (Jeremy Rubin) 7d4ec87 Cleanup (safe, it was checked) subscript[0] in MurmurHash3 (and cleanup MurmurHash3 to be more clear). (Jeremy Rubin) 5e14f54 Fix subscript[0] in compressor.cpp (Jeremy Rubin) 48a622d Fix subscript[0] bug in net.cpp if GetGroup returns a 0-sized vector (Jeremy Rubin) af4cba3 net: use an internal address for fixed seeds (Cory Fields) 36368c5 net: switch to dummy internal ip for dns seed source (Cory Fields) 8f31295 net: do not allow resolving to an internal address (Cory Fields) 8ba3a40 net: add an internal subnet for representing unresolved hostnames (Cory Fields) c60b875 fix uninitialized read when stringifying an addrLocal (Kaz Wesley) 3d36540 add test demonstrating addrLocal UB (Kaz Wesley) Pull request description: Grouped some net updates (plus a miscellaneous one) coming from upstream. Part of another deep rabbit hole that i'm doing in parallel of the tier two work. PRs List: * bitcoin#7079. * bitcoin#9804. * bitcoin#10424 * bitcoin#10446. * bitcoin#10564. * bitcoin#14728. ACKs for top commit: random-zebra: Code ACK fcdf87a Tree-SHA512: ee81c834641aa6fdb9ca4396657457358a4b32f7862d60716e914dcfc2d355970937bd0fb4e164faaa0f4ea26d263ca8a2af4d8d5d6615b2db5bf89ec70d15f3

fanquake added the P2P label May 24, 2017

theuni mentioned this pull request Jun 2, 2017

UNIX sockets support for RPC #9919

Closed

theuni force-pushed the no-double-resolve branch from 89a98f0 to b366b2f Compare June 13, 2017 22:26

sipa reviewed Jun 13, 2017

View reviewed changes

theuni added 3 commits June 14, 2017 18:05

net: do not allow resolving to an internal address

6d0bd5b

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.

net: switch to dummy internal ip for dns seed source

6cdc488

This addresss the TODO to avoid resolving twice.

theuni force-pushed the no-double-resolve branch from b366b2f to 6cdc488 Compare June 14, 2017 22:05

laanwj reviewed Jun 22, 2017

View reviewed changes

theuni added 2 commits June 22, 2017 15:21

net: use an internal address for fixed seeds

d5c7c1c

chainparams: make supported service bits option explicit

c1be285

laanwj merged commit c1be285 into bitcoin:master Jun 24, 2017

furszy mentioned this pull request May 3, 2021

Net code updates [Step 2] PIVX-Project/PIVX#2359

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: avoid extra dns query per seed #10446

net: avoid extra dns query per seed #10446

theuni commented May 24, 2017

laanwj commented May 24, 2017 •

edited

Loading

gmaxwell commented May 25, 2017

TheBlueMatt commented Jun 1, 2017

theuni commented Jun 2, 2017

sipa commented Jun 3, 2017

TheBlueMatt commented Jun 7, 2017

theuni commented Jun 13, 2017

theuni commented Jun 13, 2017

sipa left a comment

sipa Jun 13, 2017

theuni Jun 14, 2017

laanwj Jun 22, 2017

theuni Jun 22, 2017

theuni Jun 22, 2017

theuni commented Jun 22, 2017

TheBlueMatt commented Jun 23, 2017

laanwj commented Jun 24, 2017

net: avoid extra dns query per seed #10446

net: avoid extra dns query per seed #10446

Conversation

theuni commented May 24, 2017

laanwj commented May 24, 2017 • edited Loading

gmaxwell commented May 25, 2017

TheBlueMatt commented Jun 1, 2017

theuni commented Jun 2, 2017

sipa commented Jun 3, 2017

TheBlueMatt commented Jun 7, 2017

theuni commented Jun 13, 2017

theuni commented Jun 13, 2017

sipa left a comment

Choose a reason for hiding this comment

sipa Jun 13, 2017

Choose a reason for hiding this comment

theuni Jun 14, 2017

Choose a reason for hiding this comment

laanwj Jun 22, 2017

Choose a reason for hiding this comment

theuni Jun 22, 2017

Choose a reason for hiding this comment

theuni Jun 22, 2017

Choose a reason for hiding this comment

theuni commented Jun 22, 2017

TheBlueMatt commented Jun 23, 2017

laanwj commented Jun 24, 2017

laanwj commented May 24, 2017 •

edited

Loading