net: Split DNS resolving functionality out of net structures #7868

theuni · 2016-04-13T02:54:44Z

First PR of many in the p2p refactor.

This brings CNetAddr/CSubNet/CService one step closer to being dumb storage structures. By forcing addresses to be resolved elsewhere, the implementation details are free to change. In particular, this is necessary for making the resolves fully async, which is necessary in a model in which the entire connection process is asynchronous.

The DNS seed TODO could be fixed more properly with something like this: theuni@792b0f5 (an ipv6 range would probably make more sense, though), but I'll leave that for another PR.

dcousens · 2016-04-13T02:58:23Z

src/netbase.cpp

+    CService addrResolved;
+    if (Lookup(strDest.c_str(), addrResolved, port, fNameLookup && !HaveNameProxy()))
+    {
+        if (addrResolved.IsValid()) {


nit: inconsistent { indentation

dcousens · 2016-04-13T02:59:14Z

concept ACK, light utACK 825de38

jonasschnelli · 2016-04-13T06:46:30Z

Concept ACK.

instagibbs · 2016-04-13T20:09:06Z

meta-comment: Is there an issue open with all the various things to be done for p2p refactor, for a higher-level view? I'd like to review but my knowledge of p2p part of Core is weak. I think a higher-level view may help me review.

theuni · 2016-04-14T17:33:16Z

@instagibbs good point. I have another RFC PR coming up today, that will be a good place to discuss general scope/goals. I'll begin writing something up.

sipa · 2016-04-14T17:40:46Z

src/init.cpp

-                return InitError(ResolveErrMsg("externalip", strAddr));
-            AddLocal(CService(strAddr, GetListenPort(), fNameLookup), LOCAL_MANUAL);
+            CService addrLocal;
+            if (Lookup(strAddr.c_str(), addrLocal, GetListenPort(), fNameLookup)) {


If the Lookup fails, shouldn't the InitError below trigger as well?

Yes, good catch.

sipa · 2016-04-14T18:10:58Z

Concept ACK; utACK apart from one comment

laanwj · 2016-04-15T06:45:20Z

utACK, good to see DNS lookup being decoupled from connecting.

To make it clear where DNS resolves are happening

theuni · 2016-04-18T01:51:41Z

Updated for @sipa's comment.

@instagibbs I didn't manage to get it ready last week as hoped, but a PR that looks similar to https://github.com/theuni/bitcoin/tree/net-refactor12 plus a lengthy explanation is hopefully coming up in the next day or two. That work aims to segregate the p2p functionality from the rest of the code so that it can be replaced in chunks without interfering with other subsystems.

instagibbs · 2016-04-18T12:16:01Z

concept ACK, lightly tACK

sipa · 2016-04-20T09:53:28Z

src/net.cpp

+            // TODO: It doesn't make sense to define an arbitrary IP as the source of others.
+            // Instead, a dummy range should be created to identify seed resolves.
+            if (!vIPs.empty())
+                addrman.Add(vAdd, vIPs[0]);


That's not actually correct. The "source IP" the addresses get assigned to should be stable over multiple runs, as the DNS seed is seen as a potential attacker who is only allowed to modify a certain subset of entries. If the IP changes over time, that protection disappears.

@sipa To clarify, I meant that each dns seed should have a corresponding stable "source" dummy ip. Something like this: theuni@792b0f5

I see now that the second resolve is happening on seed.name rather than seed.host, so the resolve isn't actually arbitrary. That was just poor reading on my account.

I'll add back the second resolve, and we can revisit the dummy later.

Note: Some seeds aren't actually returning an IP for their name entries, so they're being added to addrman with a source of [::]. This commit shouldn't change that behavior, for better or worse.

Rather than allowing CNetAddr/CService/CSubNet to launch DNS queries, require that addresses are already resolved. This greatly simplifies async resolve logic, and makes it harder to accidentally leak DNS queries.

CNetAddr/CService/CSubnet can no longer resolve DNS.

theuni · 2016-04-20T17:14:56Z

Addressed @sipa's concern above (I hope). For reference, the following mainnet seeds don't currently resolve their names properly (for me, anyway), so they end up grouped together with sources of [::]:

xf2.org
bluematt.me

I've confirmed that seed results are added to addrman that way before and after this change. I'll work on fixing that in a follow-up pull-request.

sipa · 2016-04-21T05:14:16Z

utACK d39f5b4

Meta question: I am right to expect that there will be follow-ups that split the Lookup functions in resolving ones and non-resolving ones, and make the CNetAddr constructors only depend in the non-resolving ones?

theuni · 2016-04-21T05:17:10Z

@sipa yep, exactly.

…ures d39f5b4 net: disable resolving from storage structures (Cory Fields) 3675699 net: resolve outside of storage structures (Cory Fields) a98cd1f net: manually resolve dns seed sources (Cory Fields) e9fc71e net: require lookup functions to specify all arguments (Cory Fields)

…tures ccf4902 net: disable resolving from storage structures (Cory Fields) 424722f net: resolve outside of storage structures (Cory Fields) deb76ab net: manually resolve dns seed sources (Cory Fields) f3bd785 net: require lookup functions to specify all arguments (Cory Fields) Pull request description: Port of bitcoin#7868 This brings CNetAddr/CSubNet/CService one step closer to being dumb storage structures. By forcing addresses to be resolved elsewhere, the implementation details are free to change. In particular, this is necessary for making the resolves fully async, which is necessary in a model in which the entire connection process is asynchronous. ACKs for top commit: random-zebra: ACK ccf4902 furszy: good initial back port, utACK ccf4902 Tree-SHA512: 560c0212901474a1de8540595cf957af73960355a64840fbdb40b728b0d4a320475b77368dadb2221cf5ec7a3c2a06088ce6bb31d69fa5b05c2e02a54b21ad31

…tures ccf49028439469248e402d6da748f3515ea82adf net: disable resolving from storage structures (Cory Fields) 424722f26d80b9d602fcb16e2f5ad22416e38a84 net: resolve outside of storage structures (Cory Fields) deb76abafd2cf47619d3eccd5f8993942a0ad9e1 net: manually resolve dns seed sources (Cory Fields) f3bd785389181bb2f286e6c80c2ce5b85df52186 net: require lookup functions to specify all arguments (Cory Fields) Pull request description: Port of bitcoin/bitcoin#7868 This brings CNetAddr/CSubNet/CService one step closer to being dumb storage structures. By forcing addresses to be resolved elsewhere, the implementation details are free to change. In particular, this is necessary for making the resolves fully async, which is necessary in a model in which the entire connection process is asynchronous. ACKs for top commit: random-zebra: ACK ccf49028439469248e402d6da748f3515ea82adf furszy: good initial back port, utACK ccf4902 Tree-SHA512: 560c0212901474a1de8540595cf957af73960355a64840fbdb40b728b0d4a320475b77368dadb2221cf5ec7a3c2a06088ce6bb31d69fa5b05c2e02a54b21ad31

dcousens reviewed Apr 13, 2016
View reviewed changes

theuni force-pushed the net-cleanup-resolve branch from 825de38 to 6088311 Compare April 13, 2016 03:58

jonasschnelli added the P2P label Apr 13, 2016

sipa reviewed Apr 14, 2016
View reviewed changes

theuni force-pushed the net-cleanup-resolve branch from 6088311 to 1955067 Compare April 17, 2016 21:35

net: require lookup functions to specify all arguments

e9fc71e

To make it clear where DNS resolves are happening

theuni mentioned this pull request Apr 18, 2016

net: prerequisites for p2p encapsulation changes #7906

Merged

sipa reviewed Apr 20, 2016
View reviewed changes

theuni added 3 commits April 20, 2016 13:07

net: manually resolve dns seed sources

a98cd1f

Note: Some seeds aren't actually returning an IP for their name entries, so they're being added to addrman with a source of [::]. This commit shouldn't change that behavior, for better or worse.

net: resolve outside of storage structures

3675699

Rather than allowing CNetAddr/CService/CSubNet to launch DNS queries, require that addresses are already resolved. This greatly simplifies async resolve logic, and makes it harder to accidentally leak DNS queries.

net: disable resolving from storage structures

d39f5b4

CNetAddr/CService/CSubnet can no longer resolve DNS.

theuni force-pushed the net-cleanup-resolve branch from 1955067 to d39f5b4 Compare April 20, 2016 17:10

sipa merged commit d39f5b4 into bitcoin:master Apr 21, 2016

theuni mentioned this pull request May 31, 2016

Net: Turn net structures into dumb storage classes #8128

Merged

kyuupichan mentioned this pull request Apr 9, 2017

Net backports BitcoinUnlimited/BitcoinUnlimited#435

Merged

OlegGirko mentioned this pull request Jul 11, 2017

Backport Bitcoin PR#7906: net: prerequisites for p2p encapsulation changes dashpay/dash#1521

Merged

This was referenced Sep 2, 2017

Backport Bitcoin PR#7868: net: Split DNS resolving functionality out of net structures dashpay/dash#1601

Merged

Backport Bitcoin PR#8128: Net: Turn net structures into dumb storage classes dashpay/dash#1604

Merged

Fuzzbawls mentioned this pull request Feb 27, 2020

[Net] Split DNS resolving functionality out of net structures PIVX-Project/PIVX#1360

Merged

Fuzzbawls mentioned this pull request Mar 4, 2020

Tracking: Backport upstream network encapsulation PIVX-Project/PIVX#1374

Closed

21 tasks

PRCYDev mentioned this pull request Jan 25, 2021

Tracking: Backport upstream network encapsulation PRCYCoin/PRCYCoin#4

Open

21 tasks

This was referenced Feb 23, 2021

net: Split DNS resolving functionality out of net structures zcash/zcash#5005

Draft

Rework AddNode behaviour zcash/zcash#5006

Draft

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

net: Split DNS resolving functionality out of net structures #7868

net: Split DNS resolving functionality out of net structures #7868

Uh oh!

Conversation

theuni commented Apr 13, 2016

Uh oh!

dcousens Apr 13, 2016

Choose a reason for hiding this comment

Uh oh!

dcousens commented Apr 13, 2016

Uh oh!

jonasschnelli commented Apr 13, 2016

Uh oh!

instagibbs commented Apr 13, 2016

Uh oh!

theuni commented Apr 14, 2016

Uh oh!

sipa Apr 14, 2016

Choose a reason for hiding this comment

Uh oh!

theuni Apr 15, 2016

Choose a reason for hiding this comment

Uh oh!

sipa commented Apr 14, 2016

Uh oh!

laanwj commented Apr 15, 2016

Uh oh!

theuni commented Apr 18, 2016

Uh oh!

instagibbs commented Apr 18, 2016

Uh oh!

sipa Apr 20, 2016

Choose a reason for hiding this comment

Uh oh!

theuni Apr 20, 2016

Choose a reason for hiding this comment

Uh oh!

theuni commented Apr 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Apr 21, 2016

Uh oh!

theuni commented Apr 21, 2016

Uh oh!

Uh oh!

theuni commented Apr 20, 2016 •

edited

Loading