p2p: skip querying dns seeds if `-onlynet` disables IPv4 and IPv6

mzumsande commented at 9:30 pm on July 22, 2022: contributor

Currently, -onlynet does not work well in connection with initial peer discovery, because DNS seeds only resolve to IPv6 and IPv4 adresses: With -onlynet=i2p, we would load clearnet addresses from DNS seeds into addrman, be content our addrman isn’t empty so we don’t try to query hardcoded seeds (although these exist for i2p!), and never attempt to make an automatic outbound connection. With -onlynet=onion and -proxy set, we wouldn’t load addresses via DNS, but will make AddrFetch connections (through a tor exit node) to a random clearnet peer the DNS seed resolves to (see #6808 (comment)), thus breaching the -onlynet preference of the user - this has been reported in the two issues listed below.

This PR proposes two changes: 1.) Don’t load addresses that are unreachable (so that we wouldn’t connect to them) into addrman. This is already the case for addresses received via p2p addr messages, this PR implements the same for addresses received from DNS seeds and fixed seeds. This means that in the case of -onlynet=onion, we wouldn’t load fixed seed IPv4 addresses into addrman, only the onion ones. 2.) Skip trying the DNS seeds if neither IPv4 nor IPv6 are reachable and move directly to adding the hardcoded seeds from networks we can connect to. This is done by soft-setting -dnsseed to 0 in this case, unless -dnsseed=1 was explicitly specified, in which case we abort with an InitError.

Fixes #6808 Fixes #12344

fanquake added the label P2P on Jul 22, 2022

mzumsande force-pushed on Jul 22, 2022

DrahtBot commented at 1:04 am on July 23, 2022: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23531 (Add Yggdrasil support by prusnak)

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.

brunoerg commented at 7:48 pm on July 26, 2022: contributor

Strong concept ACK

in test/functional/feature_config_args.py:195 in 09384fc611 outdated

191         ]):
192             self.start_node(0, extra_args=['-dnsseed=0', '-fixedseeds=1'])
193         assert time.time() - start < 60
194         self.stop_node(0)
195 
196+        # No peers.dat exists, -dnsseed=1 but -onlynet=i2p

luke-jr commented at 2:26 am on July 27, 2022:

If dnsseed=1 is set, I would expect it to use DNS seeds no matter what the other options are…

mzumsande commented at 3:30 am on July 27, 2022:

It’s the default value, so many users will have it set without explicitely choosing it. Or do you suggest to have behavior conditional on whether it is set as a default value, or explicitly specified by the user?

But I can’t see a point in -dnsseed=1 -onlynet=i2p - for me, it is essentially like -noconnect: no attempts for outgoing connections are made, because we’ll load address from DNS seeds that we then refuse to connect to.

luke-jr commented at 4:42 am on July 27, 2022:

It’s the default value, so many users will have it set without explicitely choosing it. Or do you suggest to have behavior conditional on whether it is set as a default value, or explicitly specified by the user?

Yes, like SoftSetArg already does for other similar situations

But I can’t see a point in -dnsseed=1 -onlynet=i2p - for me, it is essentially like -noconnect: no attempts for outgoing connections are made, because we’ll load address from DNS seeds that we then refuse to connect to.

Perhaps not, but for tor it does make some sense.

mzumsande commented at 2:44 pm on August 10, 2022:

Yes, like SoftSetArg already does for other similar situations

Ok, I understand it would have to be separate from the other SoftSetArg interactions, because the precondition onlynet is only evaluated during AppInitMain

Perhaps not, but for tor it does make some sense.

Why? I tend to see the current behavior for Tor (adding the dns seed as an addr fetch, and making a clearnet-connection to a random node it resolves to) as a privacy bug and not something a user who specified -onlynet=onion could possibly want to do on purpose.

sipa commented at 3:19 pm on August 10, 2022:

It’s undesirable to just ignore the user’s request if they explicitly specify -dnsseed=1. Perhaps it makes sense to give an InitError, or even just a warning?

mzumsande commented at 3:41 pm on August 10, 2022:

Since we’d ignore a user’s request either way in this situation (either -dnseed=1 or -onlynet=onion) I like the suggestion of returning an InitError and not ignoring either of the two conflicting requests. Will adjust the PR to implement that.

mzumsande commented at 9:17 pm on August 10, 2022:

Done with the latest push.

michaelfolkson commented at 10:25 am on August 8, 2022: contributor

Concept ACK, Approach ACK.

This was asked about in this StackExchange question.

sipa commented at 3:22 pm on August 10, 2022: member

Concept ACK on (at least by default) not using DNS seed lookups (incl. addrfetch based ones) with -onlynet=onion. This is really a contradiction. Onlynet=onion means only connections to hidden services, and DNS seeds fundamentally requires making a connection to the IPv4IPv6 world (either by asking DNS servers, or by asking a Tor exit node to make a connection to what a DNS result resolves to).

Orthogonal idea (not for this PR): would it make sense, when using the hardcoded seeds (for whatever reason, onlynot or not), to only make addrfetch connections to them? Their point is getting us an entrypoint to the network, and using them directly as fully-featured nodes probably places an undue burden on them. This would get worse with a change like this PR.

fanquake added this to the milestone 24.0 on Aug 10, 2022

mzumsande force-pushed on Aug 10, 2022

mzumsande commented at 9:38 pm on August 10, 2022: contributor

09384fc to 1e039d1: Addressed feedback, some refactoring/rewording.

The current PR is not specific to -onlynet=onion but affects all alternative networks. While I discussed Tor and I2P in the OP, is there anything special to CJDNS that would require another approach (ping @vasild)?

Orthogonal idea (not for this PR): would it make sense, when using the hardcoded seeds (for whatever reason, onlynot or not), to only make addrfetch connections to them? Their point is getting us an entrypoint to the network, and using them directly as fully-featured nodes probably places an undue burden on them. This would get worse with a change like this PR.

Yes, I agree that this could makes sense. While in principle, the maximum inbound limit together with the inbound eviction logic should keep the number of connections under control, hardcoded peers will receive connections from a lot of new nodes which require IBD, so it would be nice to reduce their traffic burden.

vasild commented at 5:10 pm on August 12, 2022: contributor

Concept ACK

is there anything special to CJDNS that would require another approach

Well, unlike Tor and I2P addresses, CJDNS addresses could be returned by the DNS seeds, e.g. this could happen:

0seed.bitcoin.sipa.be has IPv6 address fc:fb1:4:387e:9248:9aff:febe:1c39

But we would still need to open a connection to the DNS server itself. If that is from the CJDNS network, then we are perfectly good (e.g. /etc/resolv.conf contains something like nameserver fc:123...). But if the DNS server is IPv4 or non-cjdns-IPv6, then opening a connection to it violates onlynet=cjdns, right? My understanding is that there is no easy and reliable way to check the address(es) of the DNS server(s) in use by the machine. They could also be changed after we have checked.

I guess, given the above, it is best and easiest to treat CJDNS like Tor and I2P and have onlynet=cjdns incompatible with dnsseed=1.

achow101 commented at 6:47 pm on August 15, 2022: member

ACK 1e039d14b86a7c3c014f9302c159a21c302c6ccb

fanquake requested review from dergoegge on Aug 15, 2022

in src/net.cpp:1642 in c1ef9a20b7 outdated

1637@@ -1635,9 +1638,13 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
1638             }
1639 
1640             if (add_fixed_seeds_now) {
1641+                std::vector<CAddress> seed_addr{ConvertSeeds(Params().FixedSeeds())};
1642+                seed_addr.erase(std::remove_if(seed_addr.begin(), seed_addr.end(),

naumenkogs commented at 7:52 am on August 29, 2022:

nit: Wondering if this deserves a comment? For example, for a confused users which had this working previously.

naumenkogs commented at 7:30 am on August 31, 2022:

Oops, I meant a Log statement, not a comment.

mzumsande commented at 6:50 pm on September 1, 2022:

I added a log “Added XXX fixed seeds from reachable networks”, that would make it clear, I also think it might be of interest how many addresses were loaded (there is a similar log for the DNS seeds).

naumenkogs commented at 7:58 am on August 29, 2022: member

utACK 1e039d14b86a7c3c014f9302c159a21c302c6ccb

in src/init.cpp:1340 in 1e039d14b8 outdated

1321@@ -1322,6 +1322,13 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1322     // 2.1. -onlynet is not given or
1323     // 2.2. -onlynet=cjdns is given
1324 
1325+    // Requesting DNS seeds requires connecting to IPv4/IPv6, which -onlynet options may prohibit:
1326+    // If -dnsseed=1 is explicitly specified, abort. If it's unspecified by the user, we don't request
1327+    // the DNS seeds per default but skip to the fixed seeds.
1328+    if (args.GetBoolArg("-dnsseed") == true && !IsReachable(NET_IPV4) && !IsReachable(NET_IPV6)) {
1329+        return InitError(strprintf(_("Incompatible options: -dnsseed was explicitly specified, but -onlynet forbids connections to IPv4/IPv6")));
1330+    };

vasild commented at 3:38 pm on August 30, 2022:

Should this also cover -forcednsseed? Its help reads: “Always query for peer addresses via DNS lookup”, seems to contradict with -onlynet=onion, just like -dnsseed does. In other words, should this give an init error too: bitcoind -onlynet=onion -forcednsseed=1?

mzumsande commented at 6:55 pm on September 1, 2022:

There is already an interaction between -forcednsseed and -dnsseed which gives an InitError if -dnsseed is not set. With the latest push (taking your suggestion to use InitParameterInteraction()) that should hit.

in src/net.h:1064 in 1e039d14b8 outdated

1060@@ -1061,6 +1061,7 @@ class CConnman
1061     std::condition_variable condMsgProc;
1062     Mutex mutexMsgProc;
1063     std::atomic<bool> flagInterruptMsgProc{false};
1064+    std::atomic<bool> skip_dns_no_clearnet{false};

vasild commented at 3:50 pm on August 30, 2022:

nit: m_ prefix

mzumsande commented at 6:56 pm on September 1, 2022:

Obsolete because skip_dns_no_clearnet was removed with the latest push.

vasild approved

vasild commented at 4:07 pm on August 30, 2022: contributor

ACK 1e039d14b86a7c3c014f9302c159a21c302c6ccb

This would leave gArgs.GetBoolArg("-dnsseed") to true but would effectively disable DNS seeds if e.g. -onlynet=onion is given. The existent code is already capable of disabling DNS seeds - it does so if dnsseed=0.

Given that we want to disable DNS seeds, would it be simpler to flip (soft set) -dnsseed to false in InitParameterInteraction() and not touch any of the code in net.{cpp.h} (no need to add new member to CConnman)?

0bool clearnet_reachable = true;
1if (args.IsArgSet("-onlynet")) {
2    const auto onlynets = args.GetArgs("-onlynet");
3    clearnet_reachable = std::any_of(onlynets.begin(), onlynets.end(), [](const auto& net) {
4        const auto n = ParseNetwork(net);
5        return n == NET_IPV4 || n == NET_IPV6;
6    });
7}

mzumsande force-pushed on Sep 1, 2022

mzumsande commented at 6:59 pm on September 1, 2022: contributor

Given that we want to disable DNS seeds, would it be simpler to flip (soft set) -dnsseed to false in InitParameterInteraction() and not touch any of the code in net.{cpp.h} (no need to add new member to CConnman)?

Thanks - I agree that it would be simpler and more similar to other parameter interactions to do that and took your suggestion with the latest push (added you as coauthor).

mzumsande force-pushed on Sep 1, 2022

in src/net.cpp:1641 in 4d9d26ecb3 outdated

1637@@ -1635,10 +1638,15 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
1638             }
1639 
1640             if (add_fixed_seeds_now) {
1641+                std::vector<CAddress> seed_addr{ConvertSeeds(Params().FixedSeeds())};

naumenkogs commented at 6:46 am on September 2, 2022:

nit: i think plural (seed_addrs) would make more sense, in the latter calls to this vector.

mzumsande commented at 3:22 pm on September 2, 2022:

done

in src/init.cpp:1339 in 7dab71932c outdated

1331@@ -1322,6 +1332,13 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1332     // 2.1. -onlynet is not given or
1333     // 2.2. -onlynet=cjdns is given
1334 
1335+    // Requesting DNS seeds entails connecting to IPv4/IPv6, which -onlynet options may prohibit:
1336+    // If -dnsseed=1 is explicitly specified, abort. If it's left unspecified by the user, we skip
1337+    // the DNS seeds by adjusting -dnsseed in InitParameterInteraction.
1338+    if (args.GetBoolArg("-dnsseed") == true && !IsReachable(NET_IPV4) && !IsReachable(NET_IPV6)) {
1339+        return InitError(strprintf(_("Incompatible options: -dnsseed was explicitly specified, but -onlynet forbids connections to IPv4/IPv6")));

naumenkogs commented at 6:49 am on September 2, 2022:

nit: I think specified might as well mean setting to false, while you want to talk about setting to true

vasild commented at 9:34 am on September 2, 2022:

Maybe just

0        return InitError(strprintf(_("Incompatible options: -dnsseed=1 was explicitly specified, but -onlynet forbids connections to IPv4/IPv6")));

mzumsande commented at 3:22 pm on September 2, 2022:

Changed to -dnsseed=1

naumenkogs commented at 6:50 am on September 2, 2022: member

ACK 7dab71932c070896b68d2c780909c9ca817710f4

vasild approved

vasild commented at 9:33 am on September 2, 2022: contributor

ACK 7dab71932c070896b68d2c780909c9ca817710f4 @naumenkogs’s suggestions above make sense, would be happy to re-ACK.

in src/init.cpp:729 in 7dab71932c outdated

724+        bool clearnet_reachable = std::any_of(onlynets.begin(), onlynets.end(), [](const auto& net) {
725+            const auto n = ParseNetwork(net);
726+            return n == NET_IPV4 || n == NET_IPV6;
727+        });
728+        if (!clearnet_reachable && args.SoftSetBoolArg("-dnsseed", false)) {
729+            LogPrintf("%s: parameter interaction: -onlynet exludes IPv4 and IPv6 -> setting -dnsseed=0\n", __func__);

jonatack commented at 11:30 am on September 2, 2022:

spelling: “excludes”

0            LogPrintLevel(BCLog::ADDRMAN, BCLog::Level::Info, "%s: parameter interaction: -onlynet excludes IPv4 and IPv6 -> setting -dnsseed=0\n", __func__);

mzumsande commented at 3:22 pm on September 2, 2022:

fixed, thanks

in src/net.cpp:1633 in 7dab71932c outdated

1632@@ -1630,15 +1633,20 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
1633                 LOCK2(m_addr_fetches_mutex, m_added_nodes_mutex);
1634                 if (m_addr_fetches.empty() && m_added_nodes.empty()) {
1635                     add_fixed_seeds_now = true;
1636-                    LogPrintf("Adding fixed seeds as -dnsseed=0, -addnode is not provided and all -seednode(s) attempted\n");
1637+                    LogPrintf("Adding fixed seeds as -dnsseed=0 (or IPv4/IPv6 connections are disabled via -onlynet), -addnode is not provided and all -seednode(s) attempted\n");

jonatack commented at 11:42 am on September 2, 2022:

while touching this, could use severity-based logging (info is always logged, like LogPrintf), feel free to ignore

0                    LogPrintLevel(BCLog::ADDRMAN, BCLog::Level::Info, "Adding fixed seeds as -dnsseed=0 (or IPv4/IPv6 connections are disabled via -onlynet), -addnode is not provided and all -seednode(s) attempted\n");

mzumsande commented at 3:21 pm on September 2, 2022:

I think I would prefer to have this done in a dedicated PR that switches it everywhere per module (for 25.0 - no idea whether this qualifies as a bugfix and should still go into 24.0, at least it still has the label), so that hopefully there wouldn’t be too much of a mix for an extended time.

Also, how about a short heads-up in the weekly IRC meeting or so summarizing the recent changes? While logging is used by / affects everyone, I think most contributors who didn’t follow the recent developments have internalized something like “With LogPrintf be careful about disk-filling attacks - with category-based logging no need to think about that much” which is going to change.

jonatack commented at 3:44 pm on September 2, 2022:

Yes agree, was about to update my comment to say never mind. (Am about to propose renaming the LogPrintLevel macro to simply Log or something before converting to more of them, as it is likely to subsume most of the other logging.)

jonatack commented at 11:43 am on September 2, 2022: contributor

Approach ACK, agree with @naumenkogs’ suggestions as well

mzumsande force-pushed on Sep 2, 2022

mzumsande commented at 3:24 pm on September 2, 2022: contributor

7dab719 to 37b2b5b: Addressed review feedback.

mzumsande force-pushed on Sep 2, 2022

vasild approved

vasild commented at 7:37 am on September 6, 2022: contributor

ACK 37b2b5bee84e1ae9678620fe2cdbdc3dd6b8608b

in src/net.cpp:1479 in e91e3efac9 outdated

1474@@ -1475,6 +1475,9 @@ void CConnman::ThreadDNSAddressSeed()
1475                     vAdd.push_back(addr);
1476                     found++;
1477                 }
1478+                vAdd.erase(std::remove_if(vAdd.begin(), vAdd.end(),
1479+                                          [](const CAddress& addr) { return !IsReachable(addr); }),

Sjors commented at 4:03 pm on September 6, 2022:

e91e3efac9c7b845fbabe30f23a1981706194ccb: why not just check this condition before calling vAdd.push_back(addr)? We should probably not increment found either when we don’t use an address.

mzumsande commented at 7:18 pm on September 6, 2022:

Good point. On second thought, the filtering seems unnecessary in the DNS seed case, because with the second commit, we will skip querying the DNS seeds if IPv4 and IPv6 are unreachable, while DNS seeds only return IPv4 and IPv6 addresses. So I removed it.

Sjors commented at 4:11 pm on September 6, 2022: member

Concept ACK

37b2b5bee84e1ae9678620fe2cdbdc3dd6b8608b (“don’t query seeds…”) makes sense to me, but e91e3efac9c7b845fbabe30f23a1981706194ccb (“add only reachable addresses to addrman”) less so.

This commit seems to assume that the node is always started with the same -onlynet parameter, but what if the user decides to enable IPv4 / IPv6 later? peers.dat would be full of onion nodes in that case. Then again, eventually we’d query the DNS seeds again, so maybe it’s not a big deal.

IIUC we only ever load the fixed seed nodes if addrman.size() == 0.

mzumsande commented at 7:05 pm on September 6, 2022: contributor

IIUC we only ever load the fixed seed nodes if addrman.size() == 0.

Yes, so we will never load them more than once for a given peers.dat.

This commit seems to assume that the node is always started with the same -onlynet parameter, but what if the user decides to enable IPv4 / IPv6 later? peers.dat would be full of onion nodes in that case. Then again, eventually we’d query the DNS seeds again, so maybe it’s not a big deal.

The general idea is to make the behavior consistent with the behavior towards peers (don’t accept addrs into our addrman we can’t or don’t want to connect to).

I think if a user decides to just more additional networks (at least for a transitional period), there shouldn’t be a problem: Both GetAddr answers and rumoured addresses from existing peer will likely contain a few entries from other networks over time, and once these are no longer unreachable due to -onlynet parameters, they will be accepted to addrman.

It’s more complicated if the user makes an abrupt change from -onlynet=X to -onlynet=Y: As you mentioned, switching to -onlynet=IPv4/IPv6 should not be a problem, because we’ll ask the DNS seeds even if addrman.size() != 0 after 5 minutes.

I think that switching from -onlynet=IPv4/IPv6 to e.g. -onlynet=onion or another network will probably not work as a general rule (irrespective of this PR): Since we likely used the DNS seeds to bootstrap, we have no IPs from previously unreachable networks in AddrMan, and we won’t attempt the hardcoded seeds because addrman isn’t empty.

What this PR may make more difficult is a situation where we switch between privacy networks, e.g. from -onlynet=onion to -onlynet=i2p because we may have queried the fixed seed and still have some random i2p fixed seeds in our addrman if we are lucky (and didn’t overwrite these with others, which would likely happen after a while). This seems like a rather exotic situation to me, less likely than the previous one.

Maybe it would be better to change the criterion for querying hardcoded seeds from addrman.size() == 0 to # of reachable addresses == 0, so that we would query the hardcoded seeds again if the existing entries all became unreachable due to a sudden change in -onlynet parameters?

p2p: add only reachable addresses to addrman

We will not make outgoing connection to peers that are unreachable
(e.g. because of -onlynet configuration).
Therefore, it makes no sense to add them to addrman in the first place.
While this is already the case for addresses received via p2p addr
messages, this commit does the same for addresses received
from fixed seeds.

91f0a7fbb7

p2p: Don't query DNS seeds when both IPv4 and IPv6 are unreachable

This happens, for example, if the user specified -onlynet=onion or
-onlynet=i2p. DNS seeds only resolve to IPv4 / IPv6 addresses,
making their answers useless to us, since we don't want to make
connections to these.
If, within the DNS seed thread, we'd instead do fallback AddrFetch
connections to one of the clearnet addresses the DNS seed resolves to,
we might get usable addresses from other networks
if lucky, but would be violating our -onlynet user preference
in doing so.

Therefore, in this case it is better to rely on fixed seeds for networks we
want to connect to.

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

385f5a4c3f

mzumsande force-pushed on Sep 6, 2022

Sjors commented at 7:27 am on September 7, 2022: member

addrman.size() == 0 || reachable addresses == 0 sounds good to me

in src/net.cpp:1638 in 91f0a7fbb7 outdated

1634@@ -1635,10 +1635,15 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
1635             }
1636 
1637             if (add_fixed_seeds_now) {
1638+                std::vector<CAddress> seed_addrs{ConvertSeeds(Params().FixedSeeds())};

Sjors commented at 7:28 am on September 7, 2022:

91f0a7fbb79fe81a75370a4b60dcdd2e55edfa81: what you say in the commit message, would make a good source comment.

kristapsk commented at 1:18 pm on September 7, 2022:

I agree with @Sjors on this.

fanquake commented at 8:46 am on September 8, 2022:

Done in #26040.

Sjors commented at 7:33 am on September 7, 2022: member

utACK 91f0a7fbb79fe81a75370a4b60dcdd2e55edfa81 modulo using hardcoded seeds when we have nothing reachable

naumenkogs commented at 10:07 am on September 7, 2022: member

utACK 385f5a4c3feb716fcf3f2b4823535df6da6bb67b

vasild approved

vasild commented at 12:22 pm on September 7, 2022: contributor

ACK 385f5a4c3feb716fcf3f2b4823535df6da6bb67b

I experienced the problem discussed above (finding peers after onlynet changes) but forgot to fix or even report it back then. This discussion prompted me to report it at #26035 so that it does not get forgotten again.

Thanks!

fanquake merged this on Sep 7, 2022

fanquake closed this on Sep 7, 2022

sidhujag referenced this in commit a09dc28eb8 on Sep 7, 2022

Sjors commented at 3:31 pm on September 8, 2022: member

Ok, so now the “modulo” part of my review is at least documented in #26035.

maflcko referenced this in commit ef5bb742f0 on Sep 9, 2022

sidhujag referenced this in commit bdc177ef80 on Sep 11, 2022

mzumsande deleted the branch on Nov 3, 2022

bitcoin locked this on Nov 3, 2023

p2p: skip querying dns seeds if -onlynet disables IPv4 and IPv6 #25678

Conflicts

p2p: skip querying dns seeds if `-onlynet` disables IPv4 and IPv6 #25678