tor: respect non-onion -onlynet= for outgoing Tor connections

vasild commented at 3:17 pm on August 6, 2021: member

If

-onlynet= is given one or more times but none of them includes onion and
-proxy is not set and
-onion is not set,

then we should not be making outbound connections to the Tor network.

Fixes https://github.com/bitcoin/bitcoin/issues/22647

vasild commented at 3:17 pm on August 6, 2021: member

See also https://en.wikipedia.org/wiki/Principle_of_least_astonishment

jonatack commented at 3:32 pm on August 6, 2021: member

Concept/Approach ACK. It may be good to have functional test coverage (or make it a separate function with unit tests).

Edit: updated the -onlynet documentation in bebcf785c080df9273e03b854832ba3dbd4320ec.

DrahtBot added the label P2P on Aug 6, 2021

in src/torcontrol.cpp:387 in 4f4c7d8b11 outdated

385+                    allow_outbound_onion = true;
386+                    break;
387+                }
388+            }
389+        }
390+        if (gArgs.GetArg("-onion", "") == "" && allow_outbound_onion) {

jonatack commented at 6:12 pm on August 6, 2021:

A function seems nicer than a localvar here (optionally declare in header file for unit testing)

 0+bool AllowOutboundOnion()
 1+{
 2+    if (!gArgs.IsArgSet("-onlynet")) return true;
 3+    for (const auto& net : gArgs.GetArgs("-onlynet")) {
 4+        if (ParseNetwork(net) == NET_ONION) return true;
 5+    }
 6+    return false;
 7+}
 8          // Now that we know Tor is running, possibly set the proxy for onion addresses.
 9-        bool allow_outbound_onion{true};
10-        if (gArgs.IsArgSet("-onlynet")) {
11-            allow_outbound_onion = false;
12-            for (const auto& net : gArgs.GetArgs("-onlynet")) {
13-                if (ParseNetwork(net) == NET_ONION) {
14-                    allow_outbound_onion = true;
15-                    break;
16-                }
17-            }
18-        }
19-        if (gArgs.GetArg("-onion", "") == "" && allow_outbound_onion) {
20+        if (gArgs.GetArg("-onion", "").empty() && AllowOutboundOnion()) {

jonatack commented at 6:31 pm on August 6, 2021:

Tested with this patch on mainnet:

onlynet=i2p: only I2P outbound connections are made (i2pd 2.23)
onlynet=ipv4: only IPv4 outbound connections are made
onlynet=onion: only onion outbound connections are made (tor 0.4.6)

jonatack commented at 6:40 pm on August 6, 2021:

I did more testing with combinations (all using the proposed function version above ;)

onlynet=i2p and onlynet=onion: only I2P and onion outbound connections are made
onlynet=ipv4 and onlynet=onion: only IPv4 and onion outbound connections are made
onlynet=ipv4 and onlynet=i2p: only IPv4 and I2P outbound connections are made

(Test coverage for these cases–single onlynet and combinations–would be great.)

unknown commented at 8:57 pm on August 6, 2021:

Test coverage for these cases–single onlynet and combinations–would be great

Agree

vasild commented at 7:27 am on August 11, 2021:

I think it is fine either way. A function would make more sense if this is used in more than one place. The code is now covered by a new functional test feature_onlynet.py. Left it as is.

jonatack commented at 7:37 am on August 11, 2021:

I think the function is not only shorter but also clearer and easier to reason about.

vasild commented at 7:47 am on August 11, 2021:

Alright, changed to a function.

jonatack commented at 9:34 am on August 11, 2021:

Thanks!

ghost commented at 6:34 pm on August 6, 2021: none

See also https://en.wikipedia.org/wiki/Principle_of_least_astonishment

This link says “The behavior should not astonish or surprise users”

I am surprised by lot of things in Bitcoin Core 😄

Concept ACK. Doesn’t make sense to create outbound connections with onion peers if onlynet=i2p is used.

in src/torcontrol.cpp:390 in 4f4c7d8b11 outdated

386+                    break;
387+                }
388+            }
389+        }
390+        if (gArgs.GetArg("-onion", "") == "" && allow_outbound_onion) {
391             CService resolved(LookupNumeric("127.0.0.1", 9050));

unknown commented at 7:30 pm on August 6, 2021:

This is normally 9150 on Windows so can we use #ifdef here? Although not sure if it is out of scope for this PR

0#ifdef WIN32 
1            CService resolved(LookupNumeric("127.0.0.1", 9150));
2#else
3            CService resolved(LookupNumeric("127.0.0.1", 9050));
4#endif

vasild commented at 9:01 am on August 9, 2021:

Yes, ouf of scope of this PR. This belongs to #15423, I hope that PR moves forward.

ghost commented at 9:27 pm on August 6, 2021: none

It may be good to have functional test coverage (or make it a separate function with unit tests).

Agree. Do you think these steps are correct for test?

Run Node 1:

0bitcoind -port=18333 -rpcport=18222 -datadir="/home/user/node1" -regtest=1 -listen=1 -server=1 -debug=net -rpcuser=user1 -rpcpassword=password1 -torcontrol='127.0.0.1:9051' -proxy='127.0.0.1:9050' -onlynet=onion

Run Node 2:

0bitcoind -port=18777 -rpcport=18666 -datadir="/home/user/node2" -regtest=1 -listen=1 -server=1 -debug=net -rpcuser=user2 -rpcpassword=password2 -addnode='127.0.0.1:18333' -torcontrol='127.0.0.1:9051' -proxy='127.0.0.1:9050' -onlynet=onion

Stop Node 1 and 2:

0bitcoin-cli -rpcport=18222 -rpcuser=user1 -rpcpassword=password1 stop
1bitcoin-cli -rpcport=18666 -rpcuser=user2 -rpcpassword=password2 stop

Restart Node 2 with different config (onlynet=i2p and no proxy):

0bitcoind -port=18777 -rpcport=18666 -datadir="/home/user/node2" -regtest=1 -listen=1 -server=1 -debug=net -rpcuser=user2 -rpcpassword=password2 -i2psam=127.0.0.1:7656 -onlynet=i2p

Check debug.log:

0trying connection 3llpwlxkisrm2iu5oayh2hd6df7q36wdmt6nounenzqcxkasxfk34eid.onion

Master branch should print such connection attempts and PR branch should not

jonatack commented at 9:47 pm on August 6, 2021: member

Checking the debug log output is a bit of a last resort when there’s nothing else to assert on… getnetworkinfo limited/reachable (and maybe getpeerinfo) would probably be good.

DrahtBot commented at 5:05 am on August 7, 2021: 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:

#19358 (torcontrol : avoid to set wrong outbound proxy and network settings when creating an inbound onion 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.

ghost commented at 6:37 am on August 7, 2021: none

Would this fix also #13378 ?

ghost commented at 9:20 pm on August 7, 2021: none

Checking the debug log output is a bit of a last resort when there’s nothing else to assert on… getnetworkinfo limited/reachable (and maybe getpeerinfo) would probably be good.

Also the steps I mentioned above don’t work. They worked on one of my VMs. Maybe it already had some onion address in peers.dat. I was expecting it will add onion peer in peers.dat but it didn’t. addpeeraddress with some onion address and port followed by getpeerinfo should work.

ghost commented at 8:12 am on August 8, 2021: none

Wrote a PowerShell script to test this. Tried on Windows 10 and Pop!_OS. Node 2 is connected to Node 1 on Linux and fails on Windows (Master branch).

TL;DR

Run Node 1 and get onion URL for it.
Sleep 10 seconds
Get onion address for Node 1 from getnetworkinfo -> localaddresses -> address ending with .onion and its port
Run Node 2 with onlynet=i2p (No Tor proxy)
Sleep 10 seconds
Add Node 1 in peers.dat for Node 2 using hidden RPC addpeeraddress
Sleep 30 seconds
Check if Node 2 is connected to Node 1 with getpeerinfo

0Peer info:
1
2xejoddwvi35krze3546qtx6dfmednjs2ccsociyezkyhzdxtemp5v5ad.onion:18444
3outbound-full-relay

vasild commented at 9:15 am on August 9, 2021: member

Would this fix also #13378 ?

To my understanding, yes, but only if none of -proxy or -onion is set.

Providing both -onlynet=ipv4 -onion=127.0.0.1:9050 is kind of contradictory - why provide an outgoing tor proxy if you only want to connect to IPv4? I think Bitcoin Core should print an error and refuse to start in this case, but instead it treats it as “onlynet ipv4 or tor”. Changing/fixing that is out of the scope of this PR which only aims to fix the behavior if none of -proxy or -onion is set.

ghost commented at 11:22 am on August 9, 2021: none

Also of course out of this PR, just to mention: I am sure you are already aware that to have multiple “only” nets is paradox, the option should better be called e.g. “net” instead of “onlynet”..

vasild commented at 7:20 am on August 11, 2021: member

4f4c7d8b11..4f3020d524: append a functional test to exercise the modified behavior

vasild force-pushed on Aug 11, 2021

vasild commented at 7:40 am on August 11, 2021: member

4f3020d524...00b7ba51eb: hush test/lint/lint-python.sh

vasild force-pushed on Aug 11, 2021

vasild commented at 7:46 am on August 11, 2021: member

00b7ba51eb...25f9e113db: change allow_outbound_onion to a function, as per suggestion

in test/functional/feature_onlynet.py:6 in 25f9e113db outdated

0@@ -0,0 +1,54 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2021-2021 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+"""
6+Test -onlynet

jonatack commented at 8:07 am on August 11, 2021:

0Test -onlynet configuration option.

vasild commented at 12:49 pm on August 11, 2021:

Done.

ghost commented at 8:17 am on August 11, 2021: none

Introduce a basic TCP server in the functional testing framework and use it to simulate a Tor Control daemon, which is needed in order for the code in src/torcontrol.cpp to execute the code that is relevant for -onlynet.

Interesting. Will try this today.

in test/functional/feature_onlynet.py:24 in 25f9e113db outdated

16+)
17+
18+
19+class OnlynetTest(BitcoinTestFramework):
20+    def set_test_params(self):
21+        self.extra_args = [

jonatack commented at 9:23 am on August 11, 2021:

I needed this line for the test to run:

0 class OnlynetTest(BitcoinTestFramework):
1     def set_test_params(self):
2+        self.setup_clean_chain = True
3         self.extra_args = [

Otherwise, was seeing the following error:

 0$ test/functional/feature_onlynet.py 
 12021-08-11T09:22:40.305000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_p1tdzv8m
 22021-08-11T09:22:41.628000Z TestFramework (ERROR): Assertion failed
 3Traceback (most recent call last):
 4  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 130, in main
 5    self.setup()
 6  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 285, in setup
 7    self.setup_network()
 8  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 379, in setup_network
 9    self.setup_nodes()
10  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 409, in setup_nodes
11    assert_equal(n.getblockchaininfo()["blocks"], 199)
12  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/util.py", line 49, in assert_equal
13    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
14AssertionError: not(0 == 199)
152021-08-11T09:22:41.680000Z TestFramework (INFO): Stopping nodes
162021-08-11T09:22:41.860000Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test_p1tdzv8m
172021-08-11T09:22:41.860000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/bitcoin_func_test_p1tdzv8m/test_framework.log
182021-08-11T09:22:41.861000Z TestFramework (ERROR): 
192021-08-11T09:22:41.861000Z TestFramework (ERROR): Hint: Call /home/jon/projects/bitcoin/bitcoin/test/functional/combine_logs.py '/tmp/bitcoin_func_test_p1tdzv8m' to consolidate all logs
202021-08-11T09:22:41.862000Z TestFramework (ERROR): 
212021-08-11T09:22:41.862000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
222021-08-11T09:22:41.862000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
232021-08-11T09:22:41.862000Z TestFramework (ERROR):

vasild commented at 12:50 pm on August 11, 2021:

Added. Any idea why that failure is not on CI? I don’t see it either locally.

jonatack commented at 2:11 pm on August 11, 2021:

Hm, after coming back here and rebuilding, the test now runs for me without it. Strange. Sorry for the noise.

in test/functional/feature_onlynet.py:50 in 25f9e113db outdated

45+        assert_equal(n1['onion']['reachable'], True)
46+
47+        # Passes with or without https://github.com/bitcoin/bitcoin/pull/22651,
48+        # just to ensure behavior does not change unintended.
49+        n2 = networks_dict(self.nodes[2].getnetworkinfo())
50+        assert_equal(n2['onion']['reachable'], True)

jonatack commented at 9:24 am on August 11, 2021:

Here is a proposed simplification using a dict comprehension in networks_dict and a loop for the assertions:

 0+    def networks_dict(self, node):
 1+        info = self.nodes[node].getnetworkinfo()
 2+        return {network["name"]: network for network in info["networks"]}
 3 
 4     def run_test(self):
 5-        def networks_dict(d):
 6-            r = {}
 7-            for x in d['networks']:
 8-                r[x['name']] = x
 9-            return r
10-
11-        # Would fail without [#22651](/bitcoin-bitcoin/22651/)
12-        n0 = networks_dict(self.nodes[0].getnetworkinfo())
13-        assert_equal(n0['onion']['reachable'], False)
14-
15-        # Passes with or without [#22651](/bitcoin-bitcoin/22651/),
16-        # just to ensure behavior does not change unintended.
17-        n1 = networks_dict(self.nodes[1].getnetworkinfo())
18-        assert_equal(n1['onion']['reachable'], True)
19-
20-        # Passes with or without [#22651](/bitcoin-bitcoin/22651/),
21-        # just to ensure behavior does not change unintended.
22-        n2 = networks_dict(self.nodes[2].getnetworkinfo())
23-        assert_equal(n2['onion']['reachable'], True)
24+        for node, reachable in [[0, False], [1, True], [2, True]]:
25+            assert_equal(self.networks_dict(node)['onion']['reachable'], reachable)

Could perhaps add a comment describing why we expect the first node to not be reachable.

vasild commented at 12:50 pm on August 11, 2021:

Did something similar. I find return {network["name"]: network for network in info["networks"]} extremely hard to read.

jonatack commented at 2:05 pm on August 11, 2021:

Comprehensions seem idiomatic/pythonic and the most-used way to do it in this codebase too (though I agree that a shorter n or net for the one line would probably be better than network), but I like your new version too. Nice and simple.

in test/functional/test_framework/basic_server.py:27 in 25f9e113db outdated

22+        a string or as an array which is joined into a single string. If it
23+        returns None, then the connection is closed by the server.
24+        """
25+        # create_server() is only in python 3.8
26+        # https://docs.python.org/3/library/socket.html#socket.create_server
27+        #self.listen_socket = socket.create_server(address=bind, reuse_port=True)

jonatack commented at 9:30 am on August 11, 2021:

0        # self.listen_socket = socket.create_server(address=bind, reuse_port=True)

vasild commented at 12:52 pm on August 11, 2021:

Done.

in test/functional/test_framework/basic_server.py:32 in 25f9e113db outdated

27+        #self.listen_socket = socket.create_server(address=bind, reuse_port=True)
28+        self.listen_socket = socket.socket(socket.AF_INET)
29+        self.listen_socket.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
30+        self.listen_socket.bind(bind)
31+        self.listen_socket.listen()
32+        #

jonatack commented at 9:30 am on August 11, 2021:

?

vasild commented at 12:51 pm on August 11, 2021:

I wanted to mark the end of the snippet that is equivalent to create_server(). Removed.

jonatack commented at 9:32 am on August 11, 2021: member

Nice! A few suggestions for the test. Only skimmed the basic server so far.

in test/functional/feature_onlynet.py:11 in 25f9e113db outdated

 6+Test -onlynet
 7+"""
 8+
 9+from test_framework.basic_server import (
10+    BasicServer,
11+    tor_control

jonatack commented at 9:39 am on August 11, 2021:

nit, either add a comma at the end of this line, or one-line it:

0from test_framework.basic_server import BasicServer, tor_control

vasild commented at 12:51 pm on August 11, 2021:

Added ,.

vasild commented at 12:22 pm on August 11, 2021: member

test_framework/basic_server.py can also be used to add some functional tests for I2P. We can simulate an I2P router at least as long as the text/line-based SAM is being talked on the socket. Later, when the socket switches from SAM to the binary bitcoin p2p protocol we would have to close the socket.

vasild force-pushed on Aug 11, 2021

vasild commented at 12:49 pm on August 11, 2021: member

25f9e113db...16bf93e136: address suggestions

in test/functional/feature_onlynet.py:41 in 16bf93e136 outdated

36+        return False
37+
38+    def run_test(self):
39+        # Node 0 would fail the check without https://github.com/bitcoin/bitcoin/pull/22651.
40+        # Nodes 1 and 2 pass with or without https://github.com/bitcoin/bitcoin/pull/22651,
41+        # they are here just to ensure behavior does not change unintended.

jonatack commented at 2:14 pm on August 11, 2021:

nits:

I think it’s more robust to describe why than to refer to a GitHub PR or depend on GitHub in general
s/unintended/unintentionally/

vasild commented at 9:52 am on August 12, 2021:

I agree, removed the github reference.

I think “uninternationally” is even better, changed.

in src/torcontrol.cpp:389 in 16bf93e136 outdated

388 
389-        // Now that we know Tor is running setup the proxy for onion addresses
390-        // if -onion isn't set to something else.
391-        if (gArgs.GetArg("-onion", "") == "") {
392+        // Now that we know Tor is running, possibly set the proxy for onion addresses.
393+        if (gArgs.GetArg("-onion", "") == "" && AllowOutboundOnion()) {

jonatack commented at 2:17 pm on August 11, 2021:

0        if (gArgs.GetArg("-onion", "").empty() && AllowOutboundOnion()) {

vasild commented at 9:51 am on August 12, 2021:

You suggested this one earlier and I deliberately skipped it but did not explain why.

Here we are testing whether the result of GetArg() equals to its second argument, so it is better to keep that obvious, e.g. GetArg(..., X) == X.

Talkless commented at 12:55 pm on August 15, 2021:

Because == will generate/call string comparison code, instead of just checking string length using empty(). Better just use specialized member functions instead of some kind “generic” comparison.

Another example is using clear() vs = "": https://www.youtube.com/watch?v=3X9qK7HWxjk

vasild commented at 1:36 pm on August 19, 2021:

I think readability trumps micro-optimization here, leaving it as is.

Talkless commented at 1:25 pm on August 26, 2021:

This is not micro-optimization, but avoiding unnecessary possible premature pessimization, see: https://stackoverflow.com/questions/15875252/premature-optimization-and-premature-pessimization-related-to-c-coding-standar/32269630#32269630

It’s not optimization as it’s not something more complex, harder to write or to read, not something uncommon.

in test/functional/feature_onlynet.py:28 in 16bf93e136 outdated

20+    def set_test_params(self):
21+        self.extra_args = [
22+            ['-onlynet=ipv4', '-torcontrol=127.0.0.1:19051', '-listenonion=1'],
23+            ['-onlynet=ipv4', '-torcontrol=127.0.0.1:19051', '-listenonion=1', '-onion=127.0.0.1:9050'],
24+            ['-onlynet=ipv4', '-torcontrol=127.0.0.1:19051', '-listenonion=1', '-proxy=127.0.0.1:9050'],
25+        ]

jonatack commented at 2:25 pm on August 11, 2021:

Would it make sense to test with multiple onlynet settings? (this is only a quick example, not a good one)

 0diff --git a/test/functional/feature_onlynet.py b/test/functional/feature_onlynet.py
 1index 60e4caf33d..c43a541196 100755
 2--- a/test/functional/feature_onlynet.py
 3+++ b/test/functional/feature_onlynet.py
 4@@ -22,6 +22,8 @@ class OnlynetTest(BitcoinTestFramework):
 5             ['-onlynet=ipv4', '-torcontrol=127.0.0.1:19051', '-listenonion=1'],
 6             ['-onlynet=ipv4', '-torcontrol=127.0.0.1:19051', '-listenonion=1', '-onion=127.0.0.1:9050'],
 7             ['-onlynet=ipv4', '-torcontrol=127.0.0.1:19051', '-listenonion=1', '-proxy=127.0.0.1:9050'],
 8+            ['-onlynet=ipv4', '-onlynet=ipv6', '-torcontrol=127.0.0.1:19051', '-listenonion=1'],
 9         ]
10         self.num_nodes = len(self.extra_args)
11
12@@ -39,7 +41,7 @@ class OnlynetTest(BitcoinTestFramework):
13-        for node, expected_reachable in [[0, False], [1, True], [2, True]]:
14+        for node, expected_reachable in [[0, False], [1, True], [2, True], [3, False]]:
15             self.log.info(f'Verifying node {node} which uses {self.extra_args[node]}')
16             assert_equal(self.onion_is_reachable(node), expected_reachable)

vasild commented at 9:46 am on August 12, 2021:

I think it is fine as it is. The point is to have onlynet=... but not onlynet=onion.

jonatack commented at 2:25 pm on August 11, 2021: member

ACK 16bf93e1361fa9a1503cadd96e5121d92937bfb5 rebased to master, debug build, ran new tests, verified that the test fails where expected on master, and previously manually verified the behavior extensively as described in #22651 (review) and #22651 (review)

Happy to re-ACK for further improvements.

Talkless commented at 2:45 pm on August 11, 2021: none

Concept ACK

unknown approved

unknown commented at 11:08 pm on August 11, 2021: none

tACK https://github.com/bitcoin/bitcoin/pull/22651/commits/16bf93e1361fa9a1503cadd96e5121d92937bfb5

Tested with test/functional/feature_onlynet.py and my PowerShell Script

Master Branch:

Python test fails

02021-08-11T22:47:24.271000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_tfz0x_fp
12021-08-11T22:47:26.786000Z TestFramework (INFO): Verifying node 0 which uses ['-onlynet=ipv4', '-torcontrol=127.0.0.1:19051', '-listenonion=1']
22021-08-11T22:47:26.790000Z TestFramework (ERROR): Assertion failed

PS script returns : xejoddwvi35krze3546qtx6dfmednjs2ccsociyezkyhzdxtemp5v5ad.onion:18444 outbound-full-relay

PR Branch:

Python test successful:

011T22:44:25.360000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_6vo22aqz
12021-08-11T22:44:26.622000Z TestFramework (INFO): Verifying node 0 which uses ['-onlynet=ipv4', '-torcontrol=127.0.0.1:19051', '-listenonion=1']
22021-08-11T22:44:26.623000Z TestFramework (INFO): Verifying node 1 which uses ['-onlynet=ipv4', '-torcontrol=127.0.0.1:19051', '-listenonion=1', '-onion=127.0.0.1:9050']
32021-08-11T22:44:26.625000Z TestFramework (INFO): Verifying node 2 which uses ['-onlynet=ipv4', '-torcontrol=127.0.0.1:19051', '-listenonion=1', '-proxy=127.0.0.1:9050']
42021-08-11T22:44:26.629000Z TestFramework (INFO): Stopping nodes
52021-08-11T22:44:26.791000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_6vo22aqz on exit
62021-08-11T22:44:26.791000Z TestFramework (INFO): Tests successful
7

PS script returns nothing (no onion peer)

Although both tests use different approach and a basic TCP server is added in test framework for this test, most important commit is https://github.com/bitcoin/bitcoin/pull/22651/commits/c591379eb03381145c85ee5760ce829be337a749 which looks good and fixes the issue.

Thanks for fixing this issue and improving privacy in Bitcoin Core. Hope this is merged soon and tests can always be improved in future if required.

vasild force-pushed on Aug 12, 2021

vasild commented at 9:45 am on August 12, 2021: member

16bf93e136...5959ece29e: address minor suggestions and avoid possibility of colliding listening ports by binding to port 0 (let the OS choose an available port, thanks @darosior for the idea!)

in test/functional/feature_onlynet.py:40 in 5959ece29e outdated

35+        return False
36+
37+    def run_test(self):
38+        # Node 0 would fail the check without c591379eb03381145c85ee5760ce829be337a749.
39+        # Nodes 1 and 2 pass with or without that commit, they are here just to ensure
40+        # behavior does not change unintentionally.

jonatack commented at 3:38 pm on August 12, 2021:

maybe use a docstring and describe the commit, feel free to ignore

0-        # Node 0 would fail the check without c591379eb03381145c85ee5760ce829be337a749.
1-        # Nodes 1 and 2 pass with or without that commit, they are here just to ensure
2-        # behavior does not change unintentionally.
3+        """
4+        - The test of node 0 would fail without commit c591379eb0,
5+          "tor: respect non-onion -onlynet= for outgoing Tor connections."
6+
7+        - The tests of nodes 1 and 2 pass with or without that commit;
8+          they ensure that behavior does not change unintentionally.
9+        """

vasild commented at 1:37 pm on August 19, 2021:

Leaving it as is.

in test/functional/feature_onlynet.py:42 in 5959ece29e outdated

37+    def run_test(self):
38+        # Node 0 would fail the check without c591379eb03381145c85ee5760ce829be337a749.
39+        # Nodes 1 and 2 pass with or without that commit, they are here just to ensure
40+        # behavior does not change unintentionally.
41+        for node, expected_reachable in [[0, False], [1, True], [2, True]]:
42+            self.log.info(f'Verifying node {node} which uses {self.extra_args[node]}')

jonatack commented at 3:39 pm on August 12, 2021:

output is fairly long, could shorten without loss of signal, feel free to ignore

0            self.log.info(f'Test node {node} {self.extra_args[node]}')

vasild commented at 1:38 pm on August 19, 2021:

Done.

in test/functional/test_framework/basic_server.py:17 in 5959ece29e outdated

12+
13+class BasicServer:
14+    def __init__(self, *, bind, response_generator):
15+        """
16+        :param bind: Listen address-port tuple, for example ("127.0.0.1", 80).
17+        If the port is 0 then a random available one will be chosen and it

jonatack commented at 3:44 pm on August 12, 2021:

indeed, seeing a different random port on each run as described here

jonatack commented at 3:44 pm on August 12, 2021: member

ACK 5959ece29e5b6b7ce0d8be60ae64a3312861f841 tested rebased to master, verified that the test fails where expected on master, and previously manually verified the behavior extensively as described in #22651 (review) and #22651 (review)

in src/torcontrol.cpp:376 in 5959ece29e outdated

367@@ -368,14 +368,26 @@ void TorController::add_onion_cb(TorControlConnection& _conn, const TorControlRe
368     }
369 }
370 
371+static bool AllowOutboundOnion()
372+{
373+    if (!gArgs.IsArgSet("-onlynet")) {
374+        return true;
375+    }
376+    for (const auto& net : gArgs.GetArgs("-onlynet")) {

Talkless commented at 1:16 pm on August 15, 2021:

Please consider using standard algorithms instead of hand-rolled loop:

0#include <algorithm>
1...
2
3    const auto only_nets{gArgs.GetArgs("-onlynet")};
4
5    //TODO: C++20: use std::ranges::any_of() instead
6    return std::any_of(only_nets.cbegin(), only_nets.cend(), [](const auto &net) {
7        return ParseNetwork(net) == NET_ONION;
8    });

vasild commented at 1:49 pm on August 19, 2021:

Done.

jonatack commented at 9:56 am on August 19, 2021: member

This has a couple of ACKs and would be very good to have in 22.0 to alleviate user confusion over an impactful user-facing change in the release. @vasild, do you plan to update? Anyone else like to review here?

vasild force-pushed on Aug 19, 2021

vasild commented at 1:49 pm on August 19, 2021: member

5959ece29e...a2ebcda19a: address some suggestions

@vasild, do you plan to update?

Done! :)

jonatack commented at 2:53 pm on August 19, 2021: member

Code review re-ACK a2ebcda19a25067cb8183f25f6ffd0a26065fcc5 per git diff 5959ece a2ebcda, rebase on master, debug build, re-ran test a few times, and previously manually verified the behavior extensively as described in #22651 (review) and #22651 (review)

Rspigler commented at 10:45 pm on August 19, 2021: contributor

Concept ACK

ghost commented at 4:08 pm on August 20, 2021: none

reACK https://github.com/bitcoin/bitcoin/pull/22651/commits/a2ebcda19a25067cb8183f25f6ffd0a26065fcc5

Major changes since last review:

laanwj referenced this in commit 7740ebcb02 on Aug 26, 2021

laanwj commented at 11:28 am on August 26, 2021: member

See also https://en.wikipedia.org/wiki/Principle_of_least_astonishment

To be honest this whole situation around onlynet gives me a headache: #22648#pullrequestreview-739349992

tor: respect non-onion -onlynet= for outgoing Tor connections

If
* `-onlynet=` is given one or more times but none of them includes
  `onion` and
* `-proxy` is not set and
* `-onion` is not set,
then we should not be making outbound connections to the Tor network.

Fixes https://github.com/bitcoin/bitcoin/issues/22647

5384c98993

test: ensure -onlynet behaves as expected

Introduce a basic TCP server in the functional testing framework and use
it to simulate a Tor Control daemon, which is needed in order for the
code in `src/torcontrol.cpp` to execute the code that is relevant for
`-onlynet`.

baa6cb12ac

in src/torcontrol.cpp:377 in a2ebcda19a outdated

368@@ -368,14 +369,24 @@ void TorController::add_onion_cb(TorControlConnection& _conn, const TorControlRe
369     }
370 }
371 
372+static bool AllowOutboundOnion()
373+{
374+    if (!gArgs.IsArgSet("-onlynet")) {
375+        return true;
376+    }
377+    const auto& onlynets = gArgs.GetArgs("-onlynet");

Talkless commented at 2:50 pm on August 26, 2021:

const auto& onlynets should not be a reference (it was not in my suggestion). It does extend lifetime of temporary, so no undefined horrors here, but since GetArgs does not return const&, and returned object does not use polymorphism, & seems out of place here.

vasild commented at 1:15 pm on August 30, 2021:

Removed.

vasild referenced this in commit 7b821b9d4c on Aug 30, 2021

vasild force-pushed on Aug 30, 2021

vasild commented at 1:16 pm on August 30, 2021: member

a2ebcda19a...baa6cb12ac: address minor suggestion and fix commit id in a comment.

vasild commented at 1:18 pm on August 30, 2021: member

@laanwj, I agree with your comments at #22648 (review). Opened #22834 which if merged will make this PR unnecessary.

IMO #22834 should be merged and this PR (https://github.com/bitcoin/bitcoin/pull/22651) closed without merge. But lets see what reviewers think.

jonatack commented at 5:00 pm on August 30, 2021: member

Code review re-ACK baa6cb12ac5071bb68eb5ec375be73df4f4ae28f per git diff a2ebcda baa6cb1, rebase on master, debug build, ran the new test test/functional/feature_onlynet.py a few times, previously manually verified the behavior extensively as described in #22651 (review) and #22651 (review)

Will review #22834 soon. Perhaps the functional test work here can be ~ported over to it if that change is preferred.

vasild referenced this in commit 0ea0de6438 on Sep 1, 2021

vasild referenced this in commit 46a9a797f1 on Sep 3, 2021

luke-jr commented at 4:44 pm on September 14, 2021: member

#22834 looks like a better solution IMO

luke-jr referenced this in commit 93c87541f6 on Oct 10, 2021

Talkless commented at 2:00 pm on November 6, 2021: none

#22834 looks like a better solution IMO

So this PR should be closed? @vasild I’m confused :)

vasild commented at 8:47 am on November 8, 2021: member

Closing this in favor of #22834 which has more (concept) ACKs.

Thanks, @Talkless!

vasild closed this on Nov 8, 2021

vasild referenced this in commit 8dea7fcf72 on Nov 8, 2021

vasild referenced this in commit e53a8505db on Nov 24, 2021

luke-jr referenced this in commit 06fc482a73 on Dec 14, 2021

laanwj referenced this in commit 848b11615b on Mar 1, 2022

janus referenced this in commit 11617f9a40 on Jul 3, 2022

DrahtBot locked this on Nov 8, 2022

tor: respect non-onion -onlynet= for outgoing Tor connections #22651

Conflicts