Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Oct 7, 2018

Fixes #13378

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 fanquake added the P2P label Oct 7, 2018
@ghost
Copy link
Author

ghost commented Oct 8, 2018

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
Copy link
Contributor

DrahtBot commented Jan 10, 2019

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

Conflicts

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.

@laanwj
Copy link
Member

laanwj commented Jan 16, 2019

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

@practicalswift
Copy link
Contributor

Could be closed?

@ghost
Copy link
Author

ghost commented Mar 21, 2019

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
Copy link
Contributor

practicalswift commented Mar 21, 2019

@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 :-)

@ghost
Copy link
Author

ghost commented Mar 23, 2019

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
Copy link
Contributor

DrahtBot commented Mar 9, 2020

The last travis run for this pull request was 352 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

@Saibato
Copy link
Contributor

Saibato commented Jun 25, 2020

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

diff --git a/src/init.cpp b/src/init.cpp
index 8d9566edc..00354c90f 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1397,6 +1397,9 @@ bool AppInitMain(const util::Ref& context, NodeContext& node)
             strSubVersion.size(), MAX_SUBVERSION_LENGTH));
     }
 
+    // Future devs please note: the underlying structure is default all nets are reachable
+    // So here if onlynet defined we set all routeable false and then mask in all desired nets to *true*.
+    // Use IsReachable(netname) to determine later this user settings.
     if (gArgs.IsArgSet("-onlynet")) {
         std::set<enum Network> nets;
         for (const std::string& snet : gArgs.GetArgs("-onlynet")) {
@@ -1419,7 +1422,6 @@ bool AppInitMain(const util::Ref& context, NodeContext& node)
     // -proxy sets a proxy for all outgoing network traffic
     // -noproxy (or -proxy=0) as well as the empty string can be used to not set a proxy, this is the default
     std::string proxyArg = gArgs.GetArg("-proxy", "");
-    SetReachable(NET_ONION, false);
     if (proxyArg != "" && proxyArg != "0") {
         CService proxyAddr;
         if (!Lookup(proxyArg, proxyAddr, 9050, fNameLookup)) {
@@ -1430,16 +1432,18 @@ bool AppInitMain(const util::Ref& context, NodeContext& node)
         if (!addrProxy.IsValid())
             return InitError(strprintf(_("Invalid -proxy address or hostname: '%s'"), proxyArg));
 
-        SetProxy(NET_IPV4, addrProxy);
-        SetProxy(NET_IPV6, addrProxy);
-        SetProxy(NET_ONION, addrProxy);
+        if (IsReachable(NET_IPV4)) SetProxy(NET_IPV4, addrProxy);
+        if (IsReachable(NET_IPV6)) SetProxy(NET_IPV6, addrProxy);
+        if (IsReachable(NET_ONION)) SetProxy(NET_ONION, addrProxy);
+        // -onlynet will at least leave one allowed, so we now set the name proxy
         SetNameProxy(addrProxy);
-        SetReachable(NET_ONION, true); // by default, -proxy sets onion as reachable, unless -noonion later
+        // by default, -proxy sets also an onion proxy, if allowed by -netonly flags, unless -noonion later
     }
 
     // -onion can be used to set only a proxy for .onion, or override normal proxy for .onion addresses
     // -noonion (or -onion=0) disables connecting to .onion entirely
     // An empty string is used to not override the onion proxy (in which case it defaults to -proxy set above, or none)
+
     std::string onionArg = gArgs.GetArg("-onion", "");
     if (onionArg != "") {
         if (onionArg == "0") { // Handle -noonion/-onion=0
@@ -1452,10 +1456,15 @@ bool AppInitMain(const util::Ref& context, NodeContext& node)
             proxyType addrOnion = proxyType(onionProxy, proxyRandomize);
             if (!addrOnion.IsValid())
                 return InitError(strprintf(_("Invalid -onion address or hostname: '%s'"), onionArg));
-            SetProxy(NET_ONION, addrOnion);
-            SetReachable(NET_ONION, true);
+            if (IsReachable(NET_ONION)) {
+                SetProxy(NET_ONION, addrOnion);
+            } else return InitError(strprintf(_("Invalid request network not allowed:  '%s'"), onionArg));
         }
     }
+    for (int n = 0; n < NET_MAX; n++) {
+       Network net = (enum Network)n;
+       if (net != NET_UNROUTABLE) LogPrintf("Net [%s] outbound allowed %d\n", GetNetworkName(net), IsReachable(net));
+    }
 
     // see Step 2: parameter interactions for more information about these
     fListen = gArgs.GetBoolArg("-listen", DEFAULT_LISTEN);
diff --git a/src/torcontrol.cpp b/src/torcontrol.cpp
index 84118b36e..7bcb96d67 100644
--- a/src/torcontrol.cpp
+++ b/src/torcontrol.cpp
@@ -522,15 +522,21 @@ void TorController::auth_cb(TorControlConnection& _conn, const TorControlReply&
     if (reply.code == 250) {
         LogPrint(BCLog::TOR, "tor: Authentication successful\n");
 
-        // Now that we know Tor is running setup the proxy for onion addresses
+         // Now that we know Tor is running setup the proxy for onion addresses
         // if -onion isn't set to something else.
-        if (gArgs.GetArg("-onion", "") == "") {
-            CService resolved(LookupNumeric("127.0.0.1", 9050));
-            proxyType addrOnion = proxyType(resolved, true);
-            SetProxy(NET_ONION, addrOnion);
-            SetReachable(NET_ONION, true);
+        // Or if we have already an outbound proxy defined. we skip this footgun gracefully.
+        proxyType addrOnion;
+        if (IsReachable(NET_ONION)) {
+            if (gArgs.GetArg("-onion", "") == "" && !GetProxy(NET_ONION, addrOnion)) {
+                CService resolved(LookupNumeric("127.0.0.1", 9050));
+                addrOnion = proxyType(resolved, gArgs.GetBoolArg("-proxyrandomize", true));
+                if (SetProxy(NET_ONION, addrOnion)) {
+                    // Here the 'do not default dox the service' line?
+                    SetNameProxy(addrOnion);
+                    LogPrintf("tor: Default outbound tor .onion address proxy is set to 127.0.0.1:9050\n");
+                }
+            }
         }
-
         // Finally - now create the service
         if (private_key.empty()) // No private key, generate one
             private_key = "NEW:RSA1024"; // Explicitly request RSA1024 - see issue #9214

@ghost
Copy link
Author

ghost commented Jun 26, 2020

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
Copy link
Contributor

Saibato commented Jun 26, 2020

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. 👍
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
Copy link
Contributor

🐙 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".

@fanquake
Copy link
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 fanquake closed this Sep 15, 2020
@ghost ghost deleted the do-not-reenable-Tor-when-disabled-via-onlynet branch February 1, 2021 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

P2P: Node with onlynet=ipv6 connects outgoing to Onion
6 participants