net: Split DNS resolving functionality out of net structures

theuni commented at 2:54 am on April 13, 2016: member

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: https://github.com/theuni/bitcoin/commit/792b0f5da618ea51ecd7b21db633faa6743c1e68 (an ipv6 range would probably make more sense, though), but I’ll leave that for another PR.

in src/netbase.cpp: in 825de38f5f outdated

619-        addr = addrResolved;
620-        return ConnectSocket(addr, hSocketRet, nTimeout);
621+    CService addrResolved;
622+    if (Lookup(strDest.c_str(), addrResolved, port, fNameLookup && !HaveNameProxy()))
623+    {
624+        if (addrResolved.IsValid()) {

dcousens commented at 2:58 am on April 13, 2016:

nit: inconsistent { indentation

dcousens commented at 2:59 am on April 13, 2016: contributor

concept ACK, light utACK 825de38

theuni force-pushed on Apr 13, 2016

jonasschnelli added the label P2P on Apr 13, 2016

jonasschnelli commented at 6:46 am on April 13, 2016: contributor

Concept ACK.

instagibbs commented at 8:09 pm on April 13, 2016: member

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 commented at 5:33 pm on April 14, 2016: member

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

in src/init.cpp: in 6088311aa5 outdated

1155@@ -1156,10 +1156,12 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
1156 
1157     if (mapArgs.count("-externalip")) {
1158         BOOST_FOREACH(const std::string& strAddr, mapMultiArgs["-externalip"]) {
1159-            CService addrLocal(strAddr, GetListenPort(), fNameLookup);
1160-            if (!addrLocal.IsValid())
1161-                return InitError(ResolveErrMsg("externalip", strAddr));
1162-            AddLocal(CService(strAddr, GetListenPort(), fNameLookup), LOCAL_MANUAL);
1163+            CService addrLocal;
1164+            if (Lookup(strAddr.c_str(), addrLocal, GetListenPort(), fNameLookup)) {

sipa commented at 5:40 pm on April 14, 2016:

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

theuni commented at 11:55 pm on April 15, 2016:

Yes, good catch.

sipa commented at 6:10 pm on April 14, 2016: member

Concept ACK; utACK apart from one comment

laanwj commented at 6:45 am on April 15, 2016: member

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

theuni force-pushed on Apr 17, 2016

net: require lookup functions to specify all arguments

To make it clear where DNS resolves are happening

e9fc71e5fa

theuni commented at 1:51 am on April 18, 2016: member

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 commented at 12:16 pm on April 18, 2016: member

concept ACK, lightly tACK

in src/net.cpp: in afcd44488e outdated

1453@@ -1454,7 +1454,11 @@ void ThreadDNSAddressSeed()
1454                     found++;
1455                 }
1456             }
1457-            addrman.Add(vAdd, CNetAddr(seed.name, true));
1458+
1459+            // TODO: It doesn't make sense to define an arbitrary IP as the source of others.
1460+            // Instead, a dummy range should be created to identify seed resolves.
1461+            if (!vIPs.empty())
1462+                addrman.Add(vAdd, vIPs[0]);

sipa commented at 9:53 am on April 20, 2016:

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.

theuni commented at 3:57 pm on April 20, 2016:

@sipa To clarify, I meant that each dns seed should have a corresponding stable “source” dummy ip. Something like this: https://github.com/theuni/bitcoin/commit/792b0f5da618ea51ecd7b21db633faa6743c1e68

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.

net: manually resolve dns seed sources

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.

a98cd1fc86

net: resolve outside of storage structures

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.

367569926a

net: disable resolving from storage structures

CNetAddr/CService/CSubnet can no longer resolve DNS.

d39f5b425d

theuni force-pushed on Apr 20, 2016

theuni commented at 5:14 pm on April 20, 2016: member

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 [::]:

0xf2.org
1bluematt.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 commented at 5:14 am on April 21, 2016: member

utACK d39f5b425d8fc1bf3b7f33d35625ffd8d7a3cd77

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 commented at 5:17 am on April 21, 2016: member

@sipa yep, exactly.

sipa merged this on Apr 21, 2016

sipa closed this on Apr 21, 2016

sipa referenced this in commit 7daa3adb24 on Apr 21, 2016

furszy referenced this in commit aee230135f on Mar 4, 2020

wqking referenced this in commit c2d420af61 on Aug 5, 2020

DrahtBot locked this on Sep 8, 2021

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