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

mzumsande · 2022-07-22T21:30:56Z

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

DrahtBot · 2022-07-23T01:04:36Z

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

Strong concept ACK

test/functional/feature_config_args.py

michaelfolkson · 2022-08-08T10:25:07Z

Concept ACK, Approach ACK.

This was asked about in this StackExchange question.

sipa · 2022-08-10T15:22:20Z

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.

mzumsande · 2022-08-10T21:38:12Z

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 · 2022-08-12T17:10:12Z

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:

seed.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 · 2022-08-15T18:47:26Z

ACK 1e039d1

src/net.cpp

naumenkogs · 2022-08-29T07:58:00Z

utACK 1e039d1

vasild

ACK 1e039d1

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

Would require something like this in InitParameterInteraction().

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

vasild · 2022-08-30T15:38:31Z

src/init.cpp

+    if (args.GetBoolArg("-dnsseed") == true && !IsReachable(NET_IPV4) && !IsReachable(NET_IPV6)) {
+        return InitError(strprintf(_("Incompatible options: -dnsseed was explicitly specified, but -onlynet forbids connections to IPv4/IPv6")));
+    };


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?

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.

src/net.h

mzumsande · 2022-09-01T18:59:57Z

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

src/net.cpp

src/init.cpp

naumenkogs · 2022-09-02T06:50:04Z

ACK 7dab719

vasild

ACK 7dab719

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

jonatack

Approach ACK, agree with @naumenkogs' suggestions as well

src/init.cpp

jonatack · 2022-09-02T11:42:22Z

src/net.cpp

@@ -1630,15 +1633,20 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
                LOCK2(m_addr_fetches_mutex, m_added_nodes_mutex);
                if (m_addr_fetches.empty() && m_added_nodes.empty()) {
                    add_fixed_seeds_now = true;
-                    LogPrintf("Adding fixed seeds as -dnsseed=0, -addnode is not provided and all -seednode(s) attempted\n");
+                    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");


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

Suggested change

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");

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");

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.

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

mzumsande · 2022-09-02T15:24:55Z

7dab719 to 37b2b5b: Addressed review feedback.

vasild

ACK 37b2b5b

Sjors · 2022-09-06T16:11:58Z

Concept ACK

37b2b5b ("don't query seeds...") makes sense to me, but e91e3ef ("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.

Sjors · 2022-09-06T16:03:25Z

src/net.cpp

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


e91e3ef: 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.

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.

mzumsande · 2022-09-06T19:05:18Z

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?

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.

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>

Sjors · 2022-09-07T07:27:10Z

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

Sjors

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

src/net.cpp

naumenkogs · 2022-09-07T10:07:34Z

utACK 385f5a4

vasild

ACK 385f5a4

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!

…s IPv…

Sjors · 2022-09-08T15:31:32Z

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

…es to addrman" ce42570 doc: comment "add only reachable addresses to addrman" (Kristaps Kaupe) Pull request description: Proposed by Sjors during review of #25678, was likely just missed, as it also for me looks a code where comment will not hurt. bitcoin/bitcoin#25678 (comment) ACKs for top commit: mzumsande: ACK ce42570 vasild: ACK ce42570 Zero-1729: re-ACK ce42570 Tree-SHA512: ef085d527349de07c1b43ed39e55e34b29cb0137c9509bd14a1af88206f7d4aa7dfec1dca53a9deaed67a2d0f32fa21e0b1a04d4d5d7f8a265dfab3b62bf8c54

…drman" ce42570 doc: comment "add only reachable addresses to addrman" (Kristaps Kaupe) Pull request description: Proposed by Sjors during review of bitcoin#25678, was likely just missed, as it also for me looks a code where comment will not hurt. bitcoin#25678 (comment) ACKs for top commit: mzumsande: ACK ce42570 vasild: ACK ce42570 Zero-1729: re-ACK ce42570 Tree-SHA512: ef085d527349de07c1b43ed39e55e34b29cb0137c9509bd14a1af88206f7d4aa7dfec1dca53a9deaed67a2d0f32fa21e0b1a04d4d5d7f8a265dfab3b62bf8c54

fanquake added the P2P label Jul 22, 2022

mzumsande force-pushed the 202207_onlynet_dns branch from 45ed3f7 to 09384fc Compare July 22, 2022 21:40

DrahtBot mentioned this pull request Jul 23, 2022

Severity-based logging -- parent PR #25203

Closed

6 tasks

brunoerg reviewed Jul 26, 2022

View reviewed changes

luke-jr reviewed Jul 27, 2022

View reviewed changes

test/functional/feature_config_args.py Outdated Show resolved Hide resolved

fanquake added this to the 24.0 milestone Aug 10, 2022

mzumsande force-pushed the 202207_onlynet_dns branch from 09384fc to 1e039d1 Compare August 10, 2022 21:16

fanquake requested a review from dergoegge August 15, 2022 20:51

DrahtBot mentioned this pull request Aug 21, 2022

Add Yggdrasil support #23531

Closed

naumenkogs reviewed Aug 29, 2022

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

vasild approved these changes Aug 30, 2022

View reviewed changes

mzumsande force-pushed the 202207_onlynet_dns branch from 1e039d1 to c21bd3d Compare September 1, 2022 18:48

mzumsande force-pushed the 202207_onlynet_dns branch from c21bd3d to 7dab719 Compare September 1, 2022 19:39

naumenkogs reviewed Sep 2, 2022

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

naumenkogs reviewed Sep 2, 2022

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

vasild approved these changes Sep 2, 2022

View reviewed changes

jonatack reviewed Sep 2, 2022

View reviewed changes

mzumsande force-pushed the 202207_onlynet_dns branch from 7dab719 to 626ad77 Compare September 2, 2022 15:23

mzumsande force-pushed the 202207_onlynet_dns branch from 626ad77 to 37b2b5b Compare September 2, 2022 15:40

vasild approved these changes Sep 6, 2022

View reviewed changes

Sjors reviewed Sep 6, 2022

View reviewed changes

mzumsande and others added 2 commits September 6, 2022 15:16

mzumsande force-pushed the 202207_onlynet_dns branch from 37b2b5b to 385f5a4 Compare September 6, 2022 19:17

Sjors reviewed Sep 7, 2022

View reviewed changes

src/net.cpp Show resolved Hide resolved

vasild mentioned this pull request Sep 7, 2022

Finding peers to connect to after -onlynet changes may be problematic #26035

Closed

vasild approved these changes Sep 7, 2022

View reviewed changes

fanquake merged commit 37095c7 into bitcoin:master Sep 7, 2022

kristapsk mentioned this pull request Sep 7, 2022

doc: comment "add only reachable addresses to addrman" #26040

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 7, 2022

Merge bitcoin#25678: p2p: skip querying dns seeds if -onlynet disable…

a09dc28

…s IPv…

mzumsande mentioned this pull request Sep 16, 2022

net: Make AddrFetch connections to fixed seeds #26114

Closed

mzumsande deleted the 202207_onlynet_dns branch November 3, 2022 01:10

willcl-ark mentioned this pull request Dec 16, 2022

Setting onlynet=onion still makes some IPv4 connections. #12344

Closed

bitcoin locked and limited conversation to collaborators Nov 3, 2023

	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");
	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");

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

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

Uh oh!

Conversation

mzumsande commented Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

brunoerg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michaelfolkson commented Aug 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mzumsande commented Aug 10, 2022

Uh oh!

vasild commented Aug 12, 2022

Uh oh!

achow101 commented Aug 15, 2022

Uh oh!

Uh oh!

naumenkogs commented Aug 29, 2022

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

vasild Aug 30, 2022

Choose a reason for hiding this comment

Uh oh!

mzumsande Sep 1, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mzumsande commented Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

naumenkogs commented Sep 2, 2022

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack Sep 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mzumsande Sep 2, 2022

Choose a reason for hiding this comment

Uh oh!

jonatack Sep 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mzumsande commented Sep 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors commented Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors Sep 6, 2022

Choose a reason for hiding this comment

Uh oh!

mzumsande Sep 6, 2022

Choose a reason for hiding this comment

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

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

mzumsande commented Jul 22, 2022 •

edited

Loading

DrahtBot commented Jul 23, 2022 •

edited

Loading

michaelfolkson commented Aug 8, 2022 •

edited

Loading

sipa commented Aug 10, 2022 •

edited

Loading

mzumsande commented Sep 1, 2022 •

edited

Loading

jonatack left a comment •

edited

Loading

jonatack Sep 2, 2022 •

edited

Loading

jonatack Sep 2, 2022 •

edited

Loading

mzumsande commented Sep 2, 2022 •

edited

Loading

Sjors commented Sep 6, 2022 •

edited

Loading

Sjors commented Sep 8, 2022 •

edited

Loading