Net: Do not re-enable Onion network when it was disabled via onlynet

ghost commented at 3:37 pm on October 7, 2018: none

If set onlynet=ipv4 or onlynet=ipv6 (implicating the attempt to disable onion network) without this patch, Bitcoin Core would re-enable onion network (setting onion network to status “reachable”, when it was initially set to “unreachable” (aka “limited” in the code). This happened when proxy or onion parameters where set, or when listenonion was set to 1 (which is the default).

I think, the users initial choice of disabling onion network should be honored instead of quietly being overwritten.

I have testet with onlynet=ipv4 onlynet=ipv6 onlynet=onion (each for itself, no multiple onlynet settings) and without specifying onlynet With this patch, the node only connects to the specified network (or to all networks in the last test case)

fanquake added the label P2P on Oct 7, 2018

ghost commented at 4:36 am on October 8, 2018: none

I tested also the following case: When Tor is running on 127.0.0.1:9050, and the following two options are set together: proxy=127.0.0.1:9050 onlynet=ipv4 then a Onion Hidden Service will be started and announced automatically via Tor ControlPort as usual, but the node will not connect outgoing to Onion addresses, but outgoing connections to IPv4 addresses will go through Tor.

DrahtBot commented at 7:27 am on January 10, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19779 (Remove gArgs global from init by MarcoFalke)
#19358 (net: Make sure we do not override proxy settings in hidden service. by Saibato)
#15423 (torcontrol: Query Tor for correct -onion configuration by luke-jr)

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.

DrahtBot added the label Needs rebase on Jan 14, 2019

laanwj commented at 5:33 pm on January 16, 2019: member

Needs rebase, but also I don’t think simply removing SetLimited on NET_ONION everywhere is the right solution.

practicalswift commented at 5:46 pm on March 20, 2019: contributor

Could be closed?

ghost commented at 5:52 pm on March 21, 2019: none

Could be closed?

Thanks for reaction. What’s Your objective for closing? Is the issue solved, or do You think there is no issue, or do You think the PR is not so good, in that case any other idea?

practicalswift commented at 6:00 pm on March 21, 2019: contributor

@wodry I noticed that this PR hasn’t been rebased in a long time so I simply assumed it was abandoned :-)

Please rebase and respond to @laanwj:s feedback to make sure this PR won’t be closed due to staleness :-)

Do not reenable Tor when it was disabled via onlynet 1b07f17a7e

ghost commented at 9:30 am on March 23, 2019: none

Rebased (I hope I made it right, was my first stale PR rebase). @laanwj Maybe You could give more details about why You think the PR wouldn’t be the right solution.

DrahtBot removed the label Needs rebase on Mar 23, 2019

DrahtBot closed this on Mar 9, 2020

DrahtBot commented at 8:35 pm on March 9, 2020: member

DrahtBot reopened this on Mar 9, 2020

Saibato commented at 7:10 pm on June 25, 2020: contributor

Rebased (I hope I made it right, was my first stale PR rebase). @laanwj Maybe You could give more details about why You think the PR wouldn’t be the right solution.

Hi @wodry You are almost correct, those settings are total meaningless since they are true by default, and in #7553 the mess was introduced that will prevent, onlynet selections to work correctly with onion proxys. since then it was and is possible that an onion proxy will be created later in torcontrol, that does not care about the onlynet flags, according to doku a bug since then not much noticed.

How do you like the diff, this will fix both your issue and the problem with torcontrol and different proxy settings other than 127.0.0.1:9050? Please feel free to take and copy paste what you like, i will then review …

 0diff --git a/src/init.cpp b/src/init.cpp
 1index 8d9566edc..00354c90f 100644
 2--- a/src/init.cpp
 3+++ b/src/init.cpp
 4@@ -1397,6 +1397,9 @@ bool AppInitMain(const util::Ref& context, NodeContext& node)
 5             strSubVersion.size(), MAX_SUBVERSION_LENGTH));
 6     }
 7 
 8+    // Future devs please note: the underlying structure is default all nets are reachable
 9+    // So here if onlynet defined we set all routeable false and then mask in all desired nets to *true*.
10+    // Use IsReachable(netname) to determine later this user settings.
11     if (gArgs.IsArgSet("-onlynet")) {
12         std::set<enum Network> nets;
13         for (const std::string& snet : gArgs.GetArgs("-onlynet")) {
14@@ -1419,7 +1422,6 @@ bool AppInitMain(const util::Ref& context, NodeContext& node)
15     // -proxy sets a proxy for all outgoing network traffic
16     // -noproxy (or -proxy=0) as well as the empty string can be used to not set a proxy, this is the default
17     std::string proxyArg = gArgs.GetArg("-proxy", "");
18-    SetReachable(NET_ONION, false);
19     if (proxyArg != "" && proxyArg != "0") {
20         CService proxyAddr;
21         if (!Lookup(proxyArg, proxyAddr, 9050, fNameLookup)) {
22@@ -1430,16 +1432,18 @@ bool AppInitMain(const util::Ref& context, NodeContext& node)
23         if (!addrProxy.IsValid())
24             return InitError(strprintf(_("Invalid -proxy address or hostname: '%s'"), proxyArg));
25 
26-        SetProxy(NET_IPV4, addrProxy);
27-        SetProxy(NET_IPV6, addrProxy);
28-        SetProxy(NET_ONION, addrProxy);
29+        if (IsReachable(NET_IPV4)) SetProxy(NET_IPV4, addrProxy);
30+        if (IsReachable(NET_IPV6)) SetProxy(NET_IPV6, addrProxy);
31+        if (IsReachable(NET_ONION)) SetProxy(NET_ONION, addrProxy);
32+        // -onlynet will at least leave one allowed, so we now set the name proxy
33         SetNameProxy(addrProxy);
34-        SetReachable(NET_ONION, true); // by default, -proxy sets onion as reachable, unless -noonion later
35+        // by default, -proxy sets also an onion proxy, if allowed by -netonly flags, unless -noonion later
36     }
37 
38     // -onion can be used to set only a proxy for .onion, or override normal proxy for .onion addresses
39     // -noonion (or -onion=0) disables connecting to .onion entirely
40     // An empty string is used to not override the onion proxy (in which case it defaults to -proxy set above, or none)
41+
42     std::string onionArg = gArgs.GetArg("-onion", "");
43     if (onionArg != "") {
44         if (onionArg == "0") { // Handle -noonion/-onion=0
45@@ -1452,10 +1456,15 @@ bool AppInitMain(const util::Ref& context, NodeContext& node)
46             proxyType addrOnion = proxyType(onionProxy, proxyRandomize);
47             if (!addrOnion.IsValid())
48                 return InitError(strprintf(_("Invalid -onion address or hostname: '%s'"), onionArg));
49-            SetProxy(NET_ONION, addrOnion);
50-            SetReachable(NET_ONION, true);
51+            if (IsReachable(NET_ONION)) {
52+                SetProxy(NET_ONION, addrOnion);
53+            } else return InitError(strprintf(_("Invalid request network not allowed:  '%s'"), onionArg));
54         }
55     }
56+    for (int n = 0; n < NET_MAX; n++) {
57+       Network net = (enum Network)n;
58+       if (net != NET_UNROUTABLE) LogPrintf("Net [%s] outbound allowed %d\n", GetNetworkName(net), IsReachable(net));
59+    }
60 
61     // see Step 2: parameter interactions for more information about these
62     fListen = gArgs.GetBoolArg("-listen", DEFAULT_LISTEN);
63diff --git a/src/torcontrol.cpp b/src/torcontrol.cpp
64index 84118b36e..7bcb96d67 100644
65--- a/src/torcontrol.cpp
66+++ b/src/torcontrol.cpp
67@@ -522,15 +522,21 @@ void TorController::auth_cb(TorControlConnection& _conn, const TorControlReply&
68     if (reply.code == 250) {
69         LogPrint(BCLog::TOR, "tor: Authentication successful\n");
70 
71-        // Now that we know Tor is running setup the proxy for onion addresses
72+         // Now that we know Tor is running setup the proxy for onion addresses
73         // if -onion isn't set to something else.
74-        if (gArgs.GetArg("-onion", "") == "") {
75-            CService resolved(LookupNumeric("127.0.0.1", 9050));
76-            proxyType addrOnion = proxyType(resolved, true);
77-            SetProxy(NET_ONION, addrOnion);
78-            SetReachable(NET_ONION, true);
79+        // Or if we have already an outbound proxy defined. we skip this footgun gracefully.
80+        proxyType addrOnion;
81+        if (IsReachable(NET_ONION)) {
82+            if (gArgs.GetArg("-onion", "") == "" && !GetProxy(NET_ONION, addrOnion)) {
83+                CService resolved(LookupNumeric("127.0.0.1", 9050));
84+                addrOnion = proxyType(resolved, gArgs.GetBoolArg("-proxyrandomize", true));
85+                if (SetProxy(NET_ONION, addrOnion)) {
86+                    // Here the 'do not default dox the service' line?
87+                    SetNameProxy(addrOnion);
88+                    LogPrintf("tor: Default outbound tor .onion address proxy is set to 127.0.0.1:9050\n");
89+                }
90+            }
91         }
92-
93         // Finally - now create the service
94         if (private_key.empty()) // No private key, generate one
95             private_key = "NEW:RSA1024"; // Explicitly request RSA1024 - see issue [#9214](/bitcoin-bitcoin/9214/)

ghost commented at 7:46 am on June 26, 2020: none

Please feel free to take and copy paste what you like, i will then review … @Saibato I had created this PR quite long ago and have forgotten what this is all about in detail, and it looks according to You that things changed also. So, please feel free to create a new PR, You could mention this PR there, and this PR could be closed then.

Saibato commented at 11:34 am on June 26, 2020: contributor

Please feel free to take and copy paste what you like, i will then review …

@Saibato I had created this PR quite long ago and have forgotten what this is all about in detail, and it looks according to You that things changed also. So, please feel free to create a new PR, You could mention this PR there, and this PR could be closed then.

Yup, will mention this. you clearly found and fixed a missbehave of the node that is not fixed since #7553 and your fix would have worked then. Things change a bit and I will add a fix to the PR #19358. Thx. :+1: EDIT: Hmm, maybe this needs really its own PR or this as is, because when Luke accepts my review in #15423, #19358 might be obsolete. And your fix is the the initial right step and does the job. In combination with patched by #19358 #15423 and yours #14425 as is, merged first, we would fix #13378 and #14722 in one sweep. ;-) So stay tuned…

DrahtBot commented at 9:04 am on August 26, 2020: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot added the label Needs rebase on Aug 26, 2020

fanquake added the label Up for grabs on Sep 15, 2020

fanquake commented at 6:31 am on September 15, 2020: member

See above:

Saibato I had created this PR quite long ago and have forgotten what this is all about in detail, and it looks according to You that things changed also. So, please feel free to create a new PR, You could mention this PR there, and this PR could be closed then.

fanquake closed this on Sep 15, 2020

unknown deleted the branch on Feb 1, 2021

MarcoFalke removed the label Up for grabs on Mar 22, 2022

MarcoFalke removed the label Needs rebase on Mar 22, 2022

Net: Do not re-enable Onion network when it was disabled via onlynet #14425

Conflicts