Do not make outbound connections to hosts which belong to a network
which is restricted by -onlynet.
This applies to hosts that are automatically chosen to connect to and to anchors.
This does not apply to hosts given to -connect, -addnode,
addnode RPC, dns seeds, -seednode.
Fixes #13378 Fixes #22647 Supersedes https://github.com/bitcoin/bitcoin/pull/22651
-onlynet=ipv6 -onion=127.0.0.1:9050 and Bitcoin Core will not automatically connect to .onion hosts. However one could later use the addnode RPC to manually connect to an .onion host. This matches the current behavior in master wrt other networks, e.g. -onlynet=ipv6 and later addnode to an ipv4 host.
56@@ -57,11 +57,7 @@ outgoing connections, but more is possible.
57 -onlynet=onion Make outgoing connections only to .onion addresses. Incoming
58 connections are not affected by this option. This option can be
59 specified multiple times to allow multiple network types, e.g.
60- onlynet=ipv4, onlynet=ipv6, onlynet=onion, onlynet=i2p.
61- Warning: if you use -onlynet with values other than onion, and
62- the -onion or -proxy option is set, then outgoing onion
63- connections will still be made; use -noonion or -onion=0 to
64- disable outbound onion connections in this case.
65+ onlynet=ipv6, onlynet=onion.
(Initial drive-by review.) Every onlynet question I’ve received has been about using -onlynet=onion, -onlynet=i2p, or both together, so it may be reasonable to address what node operators seem most interested in and minimize confusion, by using the hidden networks for the onlynet examples in both doc/tor.md and doc/i2p.md (unless you use all of them in the examples to illustrate that it is possible, which is what I did in #22648, but no strong opinion on whether that is helpful)
0 onlynet=onion, onlynet=i2p.
122+{
123+ if (!gArgs.IsArgSet("-onlynet")) {
124+ return true;
125+ }
126+ const auto onlynets = gArgs.GetArgs("-onlynet");
127+ return std::any_of(onlynets.begin(), onlynets.end(), [&addr](const auto& net) {
naming nit, onlynet or maybe onlynet_str might be a clearer iterator name for onlynets, and net is often used for passing a Network enum, e.g. in src/net.h::IsReachable() and SetReachable())
0 return std::any_of(onlynets.begin(), onlynets.end(), [&addr](const auto& onlynet) {
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
Do you plan to add functional tests like in #22651?
It would be good. I wonder how to test this deterministically. I guess I would want to have an addrman with just addresses from one network and use onlynet another network and check that a connection is never attempted, or something like that.
Concept ACK
Started testing today and need to test lot of combinations with onlynet. Sharing things that I observed during basic tests:
onlynet=i2p with proxy=127.0.0.1:9050 does not create any outbound connections to onion nodes using PR branch however it connects to onion node with Master branch. ✅
1.1 Run node 1:
0bitcoind -regtest=1 -port=18333 -rpcport=18222 -rpcuser=user1 -rpcpassword=pass1 -datadir=/home/prayank/node1 -debug=net
1.2 Run node 2:
0bitcoind -regtest=1 -port=18777 -rpcport=18666 -rpcuser=user2 -rpcpassword=pass2 -onlynet=i2p -i2psam=127.0.0.1:7656 -proxy=127.0.0.1:9050 -datadir=/home/prayank/node2 -debug=net
1.3 Copy onion address for node 1 and add it in peers.dat for node2:
0bitcoin-cli -rpcport=18666 -rpcuser=user2 -rpcpassword=pass2 addpeeraddress "6w724ckwacu2sdfitjr6otzfi4hfutxqqcc5mju2gc2ufguysb447gqd.onion" 18444
1.4 Wait for few seconds and node 2 will create an outbound connection with node 1 on Master branch. It should fail on PR branch.
Master branch: https://pastebin.com/raw/krvahBf7 PR branch: https://pastebin.com/raw/P6uwLtxf
Onion service and i2p service are not created when proxy=127.0.0.1:9050 is used. (Master and PR branch) :warning:
7b821b9d4c...0ea0de6438: optimize OutboundConnectionAllowedTo() for a fast lookup coz it is called frequently, plus some rewording in the docs.
@prayank23, thanks for looking into this! So all is working as expected. -logips=1 will improve your log output a tiny bit. I also tested this manually by adding a printout inside OutboundConnectionAllowedTo(), with -onlynet=i2p -onion=127.0.0.1:9050 it is called many times with *.onion addresses and returns false as expected.
A room for improvement (out of the scope of this PR): with -onlynet=X if addrman contains a small percentage of X addresses, CConnman::ThreadOpenConnections() will loop many times trying non-X addresses which get rejected either by the existent IsReachable() check or by the newly added OutboundConnectionAllowedTo(). It takes seconds to select an I2P address for example. CAddrMan::Select() could be smarter and take a onlynets argument and somehow return addresses that only belong to the requested networks fast.
Day 2 of testing onlynet
| ipv4 | ipv6 | onion | i2p | logs | Comments |
|---|---|---|---|---|---|
| :large_blue_circle: | https://pastebin.com/raw/nq5kXqYK | No issues :white_check_mark: | |||
| :large_blue_circle: | https://pastebin.com/raw/Hqnz0nAP | No issues :white_check_mark: | |||
| :large_blue_circle: | https://pastebin.com/raw/9bwY0Nbw | No issues :white_check_mark: | |||
| :large_blue_circle: | https://pastebin.com/raw/mhW371kx | No issues :white_check_mark: |
So all is working as expected
Need to test more things. onlynet=ipv6 did not work as expected today.
-logips=1 will improve your log output a tiny bit
Thanks. Will try this tomorrow.
CAddrMan::Select() could be smarter and take a onlynets argument and somehow return addresses that only belong to the requested networks fast.
Yes we can try this.
I wonder about the interactions of the new OutboundConnectionAllowedTo() with IsReachable(), which is how -onlynet also influences things - it seems complicated to have both OutboundConnectionAllowedTo() and IsReachable() doing similar things with subtle differences, and depending on the situation one or the other is effective.
My understanding is that IsReachable() will still be able to violate the -onlynet option sometimes (if -proxy , -onion in init.cpp or torcontrol.cpp call SetReachable() after the -onlynet arg was evaluated).
Because addrman acceptance is gated by IsReachable(), I think we could still accept addrs from networks excluded by -onlynet into addrman that we no longer would make outgoing connections to - leading to futile connection attempts.
Would it be possible as an alternative approach to get Reachable in line with -onlynet expectations (e.g. by moving the -onlynet block in init.cpp below the -onion and -proxy blocks and also not calling SetReachable() from torcontrol if -onlynet excludes onions?) Or would there be negative side effects with other callers of IsReachable()?
-onlynet= is not used? The text Network is unreachable is an actual system error, it does not come from Bitcoin Core.
Yes it was ipv6 connectivity issue. There were 2 issues: 1. One of my ISP didn’t support ipv6 2. Had to change network adapter from NAT to bridged in VM. Checked connectivity on http://test-ipv6.com/
Updated logs above as ipv6 works fine. Will test onion-ipv4, onion-ipv6, i2p-ipv4, i2p-ipv6, i2p-onion and ipv6-ipv4 today.
@mzumsande, good insight! I think there are two distinct properties of a network:
IsReachable() function). That is - do we have the means to connect to it if we wanted?-onlynet=, or the newly added OutboundConnectionAllowedTo() function)For example, a setup like -onlynet=ipv6 -onion=127.0.0.1:9050 should make the Tor network “reachable”, but avoid automatically opening connections to it. However, manual connections using -connect, -addnode and -seednode to Tor hosts should be still possible, using the proxy given with -onion=. Makes sense?
Your comments make me think - should we only add addresses to addrman if they are both 1. and 2. (right now it is just 1.)? E.g.
0 // Do not store addresses outside our network
1- if (fReachable)
2+ if (fReachable && OutboundConnectionAllowedTo(addr))
3 vAddrOk.push_back(addr);
or also limit relay of 1. but not 2. addresses?
What about changing the limited property in the getnetworkinfo RPC reply?
-onlynet (see how vfLimited is filled with onlynet).
It tried few combinations with onlynet and everything works as expected. Although I am still not sure about trade-offs involved in each of these. Things get even more complicated if user has incoming connections on multiple networks as discussed in IRC: https://www.erisian.com.au/bitcoin-core-dev/log-2021-09-01.html (260 2021-09-01T20:26:30)
This PR has limited scope and it does fix the issues with onlynet and proxy usage.
ACK https://github.com/bitcoin/bitcoin/pull/22834/commits/0ea0de64385c0150e179885a792d615005e20841
Will reACK if suggestions discussed above are implemented and functional tests are added:
Also not sure why addpeeraddress returns false in this case: https://bitcoin.stackexchange.com/q/109465/
@mzumsande, @naumenkogs, if we need not make a distinction between 1. and 2. (from #22834 (comment)), then an alternative approach is possible, similar to the one described above by @mzumsande (but not quite the same).
What do you think about this: https://github.com/vasild/bitcoin/commit/46a9a797f1692bc8ceefea1bee49caba5ca87127? Note that even with that, IsReachable() does not represent exactly -onlynet.
I didn’t find a place we would need to make the distinction between the ability (1) and the willingness (2) to make outbound connections to networks. Also the status quo for IsReachable() before this PR, despite the name, is a mix of 1. and 2. because it is initially set by -onlynet user preference (2) and then possibly overruled based on the means to connect to a network (1):
For example, a setup like -onlynet=ipv6 -onion=127.0.0.1:9050 should make the Tor network “reachable”, but avoid automatically opening connections to it.
Is this a relevant scenario, i.e. what should be the practical consequence of a network being “reachable” in this scenario when we wouldn’t do automatic connections to it?
However, manual connections using -connect, -addnode and -seednode to Tor hosts should be still possible, using the proxy given with -onion=. Makes sense?
Yes, I agree. As far as I can see, all of the manual connection code is completely independent from IsReachable, so this shouldn’t be affected by any of the discussed suggestions.
Your comments make me think - should we only add addresses to addrman if they are both 1. and 2. (right now it is just 1.)
I tend to think that this would make sense. I don’t see the point of storing addresses in addrman when we won’t connect to them - whether this is because we cannot or because we don’t want to doesn’t seem crucial to me.
What do you think about this: vasild@46a9a79? Note that even with that, IsReachable() does not represent exactly -onlynet.
On first sight, I like this approach. I don’t quite understand why it is possible to just drop the SetReachable() in torcontrol.cpp.
I have been looking into the other call sites of IsReachable() besides connection opening and addrman acceptance to see how they would be affected by possible changes:
IsPeerAddrLocalGood() and AddLocal() both call IsReachable() and therefore might be affected. Considering that they deal with the address used for self-advertising (and thus for getting incoming connections) I don’t really understand why they make use of IsReachable() in the first place. Shouldn’t this affect only outbound connection logic?
I didn’t find a place we would need to make the distinction between the ability (1) and the willingness (2) to make outbound connections to networks.
The only one I can think of is this - it would be strange to display onion as not reachable in getnetworkinfo RPC, but to be able to open manual connections to it using addnode.
Also the status quo for
IsReachable()before this PR, despite the name, is a mix of 1. and 2. because it is initially set by-onlynetuser preference (2) and then possibly overruled based on the means to connect to a network (1):
Yes.
For example, a setup like -onlynet=ipv6 -onion=127.0.0.1:9050 should make the Tor network “reachable”, but avoid automatically opening connections to it.
Is this a relevant scenario, i.e. what should be the practical consequence of a network being “reachable” in this scenario when we wouldn’t do automatic connections to it?
It would be possible to reach the network by opening a manual connection. I am not sure if this is “relevant”. Maybe it is exotic use case.
What do you think about this: vasild/bitcoin@46a9a79? Note that even with that, IsReachable() does not represent exactly -onlynet.
On first sight, I like this approach. I don’t quite understand why it is possible to just drop the
SetReachable()in torcontrol.cpp.
Hmm, maybe that is not a good idea - if -torcontrol=... -torpassword=... are given (without any of -onlynet, -proxy or -onion) then that code would have flipped the Tor network from not reachable to reachable. Is this desirable? If yes, then instead of removing SetReachable() from torcontrol.cpp it should be made conditional: if (!onion_restricted) {. However that variable onion_restricted is local in init.cpp so it would either have to be made global (:disappointed:) or OutboundConnectionAllowedTo() used instead of it. But we are looking into that potentially more invasive patch only because the co-existence of IsReachable() and OutboundConnectionAllowedTo() is confusing…
IsPeerAddrLocalGood()andAddLocal()both callIsReachable()and therefore might be affected. Considering that they deal with the address used for self-advertising (and thus for getting incoming connections) I don’t really understand why they make use ofIsReachable()in the first place. Shouldn’t this affect only outbound connection logic?
I think the logic is this - if a network is not reachable then we cannot accept connections from it. “Reachable” in the broader English word meaning, not the stricter -onlynet meaning. For example, a host/OS may have support for both IPv4 and IPv6 but its ISP may only support IPv4. So, an operator would run -onlynet=ipv4 to avoid attempting connections to IPv6 hosts and from advertising an IPv6 address to which the rest of the world cannot connect.
119+ * Could be restricted by the `-onlynet` configuration option.
120+ * @return true if allowed
121+ */
122+static bool OutboundConnectionAllowedTo(const CNetAddr& addr)
123+{
124+ static const std::unordered_set<Network> onlynets = [](){
This seems fragile. Can we guarantee onlynets is initialised only after the config files get loaded?
At the very least, I think it should be tested.
Init functions, or called from there. We’ve historiically had many initialization bugs due to hidden gotchas like this.
0ea0de6438...051c2554ca: rebase and refactor to address concerns:
IsReachable() and OutboundConnectionAllowedTo() (@mzumsande, @naumenkogs)OutboundConnectionAllowedTo() - this function was removed (@luke-jr, @laanwj)Rename proxyType to Proxy in a second commit (scripted diff), which is optional.
Invalidates ACK from @prayank23
Would it be possible as an alternative approach to get Reachable in line with
-onlynetexpectations …
Almost! The only exception now is that we SetReachable(NET_ONION, false) is we don’t have a proxy to reach the Tor network.
Comparison of IsReachable(NET_ONION) before and after this PR:
reACK https://github.com/bitcoin/bitcoin/pull/22834/commits/051c2554ca1eb9f9f2109c586f7b53c5b1e83ce9
Tested only one case with steps mentioned here: #22834 (comment)
Could also see the error mentioned in the table above:
02021-11-10T19:34:34Z Error: Outbound connections restricted to Tor (-onlynet=onion) but the proxy for reaching the Tor network is not provided (no -proxy= and no -onion= given) or it is explicitly forbidden (-onion=0)
Concept/ Approach ACK.
I like the reworked approach, the handling of -onlynet in connection with other parameters and IsReachable() makes sense to me and is clearer than before - so my points are addressed.
Having very little experience with Tor in bitcoin, I’ll need to do more testing/exploration before actually ACKing.
Do not make outbound connections to hosts which belong to a network
which is restricted by `-onlynet`.
This applies to hosts that are automatically chosen to connect to and to
anchors.
This does not apply to hosts given to `-connect`, `-addnode`,
`addnode` RPC, dns seeds, `-seednodes`.
Fixes https://github.com/bitcoin/bitcoin/issues/13378
Fixes https://github.com/bitcoin/bitcoin/issues/22647
Supersedes https://github.com/bitcoin/bitcoin/pull/22651
-BEGIN VERIFY SCRIPT-
sed -i 's/\<proxyType\>/Proxy/g' $(git grep -l proxyType)
-END VERIFY SCRIPT-
051c2554ca...0eea83a85e: rebase due to conflicts
Invalidates ACK from @prayank23
1850@@ -1843,7 +1851,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1851 if (!Lookup(i2psam_arg, addr, 7656, fNameLookup) || !addr.IsValid()) {
1852 return InitError(strprintf(_("Invalid -i2psam address or hostname: '%s'"), i2psam_arg));
1853 }
1854- SetReachable(NET_I2P, true);
Same logic as with Tor - avoid explicitly calling SetReachable(..., true) because:
vfLimited[] are false by default), so if the network is reachable anyway, SetReachable(..., true) is a noop.-onlynet= was specified and the network in question is not among the lucky ones being listed. In this case, respect the -onlynet= setting.
This does not apply to hosts given to `-connect`, `-addnode`,
`addnode` RPC, dns seeds, `-seednodes`.
1319+
1320 bool proxyRandomize = args.GetBoolArg("-proxyrandomize", DEFAULT_PROXYRANDOMIZE);
1321 // -proxy sets a proxy for all outgoing network traffic
1322 // -noproxy (or -proxy=0) as well as the empty string can be used to not set a proxy, this is the default
1323 std::string proxyArg = args.GetArg("-proxy", "");
1324- SetReachable(NET_ONION, false);
The default network reachability values are implicitly set in this line in net.cpp:
0static bool vfLimited[NET_MAX] GUARDED_BY(g_maplocalhost_mutex) = {};
Maybe assert that each network is reachable in this first loop through them during init. Edit: proposed in #24205.
0diff --git a/src/init.cpp b/src/init.cpp
1index 72b86daf21..81c145870b 100644
2--- a/src/init.cpp
3+++ b/src/init.cpp
4@@ -1317,6 +1317,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
5 }
6 for (int n = 0; n < NET_MAX; n++) {
7 enum Network net = (enum Network)n;
8+ assert(IsReachable(net));
9 if (!nets.count(net))
10 SetReachable(net, false);
11 }
1367+ } else {
1368+ if (args.IsArgSet("-onlynet") && IsReachable(NET_ONION)) {
1369+ return InitError(
1370+ _("Outbound connections restricted to Tor (-onlynet=onion) but the proxy for "
1371+ "reaching the Tor network is not provided (no -proxy= and no -onion= given) or "
1372+ "it is explicitly forbidden (-onion=0)"));
ACK 0eea83a85ec6b215d44facc2b16ee1b035275a6b code review, rebased to master, debug built, and did some manual testing with various config options on signet
Came across the onlynet issue again recently that this pull fixes.
This pull has been through quite a bit of review and has several ACKs. It may be good to have it in v23.
440@@ -441,7 +441,7 @@ void SetupServerArgs(ArgsManager& argsman)
441 argsman.AddArg("-onion=<ip:port>", "Use separate SOCKS5 proxy to reach peers via Tor onion services, set -noonion to disable (default: -proxy)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
442 argsman.AddArg("-i2psam=<ip:port>", "I2P SAM proxy to reach I2P peers and accept I2P connections (default: none)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
443 argsman.AddArg("-i2pacceptincoming", "If set and -i2psam is also set then incoming I2P connections are accepted via the SAM proxy. If this is not set but -i2psam is set then only outgoing connections will be made to the I2P network. Ignored if -i2psam is not set. Listening for incoming I2P connections is done through the SAM proxy, not by binding to a local address and port (default: 1)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
444- argsman.AddArg("-onlynet=<net>", "Make outgoing connections only through network <net> (" + Join(GetNetworkNames(), ", ") + "). Incoming connections are not affected by this option. This option can be specified multiple times to allow multiple networks. Warning: if it is used with non-onion networks and the -onion or -proxy option is set, then outbound onion connections will still be made; use -noonion or -onion=0 to disable outbound onion connections in this case.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
445+ argsman.AddArg("-onlynet=<net>", "Make automatic outgoing connections only through network <net> (" + Join(GetNetworkNames(), ", ") + "). Incoming connections are not affected by this option. This option can be specified multiple times to allow multiple networks.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
Maybe s/outgoing connections/automatic outbound connections and s/incoming connections/inbound and manual connections/ in these three lines:
0₿ git grep "Incoming connections\|. Incoming"
1doc/i2p.md:68:Make outgoing connections only to I2P addresses. Incoming connections are not
2doc/tor.md:56: -onlynet=onion Make outgoing connections only to .onion addresses. Incoming
3src/init.cpp:444: argsman.AddArg("-onlynet=<net>", "Make automatic outgoing connections only through network <net> (" + Join(GetNetworkNames(), ", ") + "). Incoming connections are not affected by this option. This option can be specified multiple times to allow multiple networks.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);