init: allow startup with -onlynet=onion -listenonion=1

vasild commented at 9:21 am on April 26, 2022: contributor

It does not make sense to specify -onlynet=onion without providing a Tor proxy (even if other -onlynet=... are given). This is checked during startup. However, it was forgotten that a Tor proxy can also be retrieved from “Tor control” to which we connect if -listenonion=1.

So, the full Tor proxy retrieval logic is:

get it from -onion
get it from -proxy
if -listenonion=1, then connect to “Tor control” and get the proxy from there (was forgotten before this change)

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

vasild force-pushed on Apr 26, 2022

vasild commented at 9:48 am on April 26, 2022: contributor

111bec2fdc...bbbb1755da: remove ; from python code 🤦

111b, bbbb1, wow! today is my lucky day!

DrahtBot added the label Tests on Apr 26, 2022

in src/init.cpp:1374 in bbbb1755da outdated

1367@@ -1361,11 +1368,14 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1368     if (onion_proxy.IsValid()) {
1369         SetProxy(NET_ONION, onion_proxy);
1370     } else {
1371-        if (args.IsArgSet("-onlynet") && IsReachable(NET_ONION)) {
1372+        // If -listenonion is set, then we will (try to) connect to the Tor control port
1373+        // later from the torcontrol thread and may retrieve the onion proxy from there.
1374+        const bool maybe_later = args.GetBoolArg("-listenonion", DEFAULT_LISTEN_ONION);
1375+        if (onlynet_used_with_onion && !maybe_later) {

jonatack commented at 12:09 pm on April 26, 2022:

naming suggestion (or just inline it without a local variable, as a comment is also added here)

0-        const bool maybe_later = args.GetBoolArg("-listenonion", DEFAULT_LISTEN_ONION);
1-        if (onlynet_used_with_onion && !maybe_later) {
2+        const bool listen_onion_disabled{!args.GetBoolArg("-listenonion", DEFAULT_LISTEN_ONION)};
3+        if (onlynet_used_with_onion && listen_onion_disabled) {

vasild commented at 1:21 pm on April 29, 2022:

Done.

in src/init.cpp:1343 in bbbb1755da outdated

1376             return InitError(
1377                 _("Outbound connections restricted to Tor (-onlynet=onion) but the proxy for "
1378-                  "reaching the Tor network is not provided (no -proxy= and no -onion= given) or "
1379-                  "it is explicitly forbidden (-onion=0)"));
1380+                  "reaching the Tor network is not provided: none of -proxy, -onion or "
1381+                  "-listenonion is given"));

jonatack commented at 12:18 pm on April 26, 2022:

Maybe consider something like this as a more helpful error message:

0-                _("Outbound connections restricted to Tor (-onlynet=onion) but the proxy for "
1-                  "reaching the Tor network is not provided: none of -proxy, -onion or "
2-                  "-listenonion is given"));
3+                _("The -onlynet=onion configuration option was passed to restrict outbound"
4+                  " connections to Tor, but the proxy for reaching the Tor network was not"
5+                  " provided: none of -proxy, -onion or -listenonion was given"));

vasild commented at 1:21 pm on April 29, 2022:

Done.

in test/functional/feature_proxy.py:350 in bbbb1755da outdated

356+        msg = (
357+            "Error: Outbound connections restricted to Tor (-onlynet=onion) but "
358+            "the proxy for reaching the Tor network is not provided: "
359+            "none of -proxy, -onion or -listenonion is given"
360+        )
361+        self.nodes[1].assert_start_raises_init_error(expected_msg=msg)

jonatack commented at 12:19 pm on April 26, 2022:

maybe keep the test without explicit listenonion=0 (not sure, didn’t review deeply yet)

 0-        self.log.info("Test passing -onlynet=onion without -proxy, -onion or -listenonion raises expected init error")
 1-        self.nodes[1].extra_args = ["-onlynet=onion", "-listenonion=0"]
 2         msg = (
 3             "Error: Outbound connections restricted to Tor (-onlynet=onion) but "
 4             "the proxy for reaching the Tor network is not provided: "
 5             "none of -proxy, -onion or -listenonion is given"
 6         )
 7+
 8+        self.log.info("Test passing -onlynet=onion without -proxy or -onion raises expected init error")
 9+        self.nodes[1].extra_args = ["-onlynet=onion", "-listen"]
10+        self.nodes[1].assert_start_raises_init_error(expected_msg=msg)
11+
12+        self.log.info("Test passing -onlynet=onion and -listen, without -proxy or -onion, and with -listenonion disabled raises expected init error")
13+        self.nodes[1].extra_args = ["-onlynet=onion", "-listen", "-listenonion=0"]
14         self.nodes[1].assert_start_raises_init_error(expected_msg=msg)

vasild commented at 1:24 pm on April 29, 2022:

Since now the behavior depends on the -listenonion value, I added two tests - one with 1 and one with 0. Leaving the test without an explicit -listenonion would mean relying on some parameter interaction or that the testing framework uses a specific value. This could result in a test failure if it is changed.

jonatack commented at 12:20 pm on April 26, 2022: contributor

Light Approach ACK (looked quickly, in seminar ATM)

0₿ bitcoind -signet -onlynet=onion -listenonion=0
12022-04-26T12:11:00Z [init] Error: Outbound connections restricted to Tor (-onlynet=onion) but the proxy for reaching the Tor network is not provided: none of -proxy, -onion or -listenonion is given
2Error: Outbound connections restricted to Tor (-onlynet=onion) but the proxy for reaching the Tor network is not provided: none of -proxy, -onion or -listenonion is given
32022-04-26T12:11:00Z [init] Shutdown: In progress...
42022-04-26T12:11:00Z [scheduler] scheduler thread exit
52022-04-26T12:11:00Z [shutoff] Shutdown: done

laanwj added the label P2P on Apr 26, 2022

dunxen commented at 4:51 pm on April 26, 2022: contributor

Concept ACK

111bec2fdc...bbbb1755da: remove ; from python code 🤦

111b, bbbb1, wow! today is my lucky day!

Nice!

dunxen commented at 7:10 am on April 28, 2022: contributor

tACK bbbb175

./src/bitcoind -signet -onlynet=onion -listenonion=1 will not fail to start up. Node starts connecting to onion peers and runs normally.

./src/bitcoind -signet -onlynet=onion -listenonion=0 will kill init with:

02022-04-28T07:09:12Z Error: Outbound connections restricted to Tor (-onlynet=onion) but the proxy for reaching the Tor network is not provided: none of -proxy, -onion or -listenonion is given
1Error: Outbound connections restricted to Tor (-onlynet=onion) but the proxy for reaching the Tor network is not provided: none of -proxy, -onion or -listenonion is given

laanwj commented at 8:33 am on April 28, 2022: member

@midnightmagic Can you confirm this solves your issue?

vasild commented at 1:21 pm on April 29, 2022: contributor

bbbb1755da...c313568a97: address suggestions

vasild force-pushed on Apr 29, 2022

schildbach commented at 9:38 pm on May 1, 2022: contributor

Does this issue prevent incoming connections via Tor, if --torcontrol is used in combination with --listenonion=1? I’ve been running version 23.0 for days, and I’ve not got a single incoming connection via Tor. All are outgoing.

vasild commented at 7:25 am on May 2, 2022: contributor

@schildbach, no, the issue itself (described in #24980) is that startup would abort with an error if -onlynet=onion is given without -proxy and without -onion. Those options specify the proxy for connecting to the Tor network. However there is a 3rd way to provide a Tor proxy - if -listenonion=1 is given, then we would connect to the “Tor control center” and retrieve the IP address/port of the Tor proxy from there.

This PR aims to fix that problem by relaxing the startup check to allow startup with -onlynet=onion without -proxy, without -onion if -listenonion is given.

jonatack commented at 4:13 pm on May 5, 2022: contributor

Diff LGTM in latest push per git diff bbbb175 c313568, will re-review.

I’ve been running version 23.0 for days, and I’ve not got a single incoming connection via Tor. @schildbach If you like, send me your tor address over IRC (nick jonatack on #bitcoin-core-dev) and I’ll try to manually connect to you.

DrahtBot added the label Needs rebase on May 20, 2022

luke-jr referenced this in commit c9c2818619 on May 21, 2022

vasild force-pushed on May 23, 2022

vasild commented at 9:23 am on May 23, 2022: contributor

c313568a97...28ae912f50: rebase due to conflicts

Previously invalidated ACK from @dunxen

DrahtBot removed the label Needs rebase on May 23, 2022

dunxen commented at 11:33 am on May 23, 2022: contributor

re-ACK 28ae912

mzumsande commented at 8:52 pm on September 2, 2022: contributor

Concept ACK, will review closely soon. I think it would be nice to get this fixed for 24.0.

DrahtBot commented at 8:30 am on September 4, 2022: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25989 (init: abort if i2p/cjdns are chosen via -onlynet but are unreachable by mzumsande)

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.

in src/init.cpp:1322 in 28ae912f50 outdated

1358         if (onionArg == "0") { // Handle -noonion/-onion=0
1359             onion_proxy = Proxy{};
1360+            if (onlynet_used_with_onion) {
1361+                return InitError(
1362+                    _("Outbound connections restricted to Tor (-onlynet=onion) but the proxy for "
1363+                      "reaching the Tor network is explicitly forbidden: -onion=0"));

jonatack commented at 10:02 am on September 5, 2022:

nit, use the same (more explicit) phrasing here as for the error message below, and consistent verb tense

0-                    _("Outbound connections restricted to Tor (-onlynet=onion) but the proxy for "
1-                      "reaching the Tor network is explicitly forbidden: -onion=0"));
2+                    _("The -onlynet=onion configuration option was passed to restrict outbound"
3+                      " connections to Tor, but the proxy for reaching the Tor network was explicitly forbidden: -onion=0"));

vasild commented at 3:18 pm on September 5, 2022:

Would be good to have consistent wording here and for I2P and CJDNS in #25989.

master:

Outbound connections restricted to Tor (-onlynet=onion) but the proxy for reaching the Tor network is not provided …

this PR:

Outbound connections restricted to Tor (-onlynet=onion) but the proxy for reaching the Tor network is explicitly forbidden: -onion=0

The -onlynet=onion configuration option was passed to restrict outbound connections to Tor, but the proxy for reaching the Tor network was not provided …

#25989

Outbound connections restricted to CJDNS (-onlynet=cjdns) but -cjdnsreachable is not provided

Outbound connections restricted to i2p (-onlynet=i2p) but -i2psam is not provided

vasild commented at 4:05 pm on September 5, 2022:

I changed the second message in this PR to be consistent with the rest:

Outbound connections restricted to Tor (-onlynet=onion) but the proxy for reaching the Tor network is not provided: none of -proxy, -onion or -listenonion is given

jonatack commented at 2:32 pm on September 8, 2022:

I think the explicit version is clearer.

in test/functional/feature_proxy.py:359 in 28ae912f50 outdated

361+        )
362+        self.nodes[1].assert_start_raises_init_error(expected_msg=msg)
363+
364+        self.log.info("Test passing -onlynet=onion without -proxy or -onion but with -listenonion=1 is ok")
365+        self.nodes[1].extra_args = ["-onlynet=onion", "-listenonion=1"]
366+        self.start_node(1)

jonatack commented at 10:33 am on September 5, 2022:

these two lines can be combined to one, i.e. self.start_node(1, extra_args=["-onlynet=onion", "-listenonion=1"])
maybe check that Tor is successfully up with assert_debug_log

0         self.log.info("Test passing -onlynet=onion without -proxy or -onion but with -listenonion=1 is ok")
1-        self.nodes[1].extra_args = ["-onlynet=onion", "-listenonion=1"]
2-        self.start_node(1)
3+        with self.nodes[1].assert_debug_log(["[tor] Authentication successful"]):
4+            self.start_node(1, extra_args=["-onlynet=onion", "-listenonion=1"])
5         self.stop_node(1)

vasild commented at 4:04 pm on September 5, 2022:

I combined the two lines to one, as suggested.

However, expecting “[tor] Authentication successful” in the log failed for me because it tried to talk to a Tor daemon on 127.0.0.1:9051, not to the Python one from the testing framework at self.conf2.addr, because when overriding extra_args the setting -onion=... which was in extra_args is lost.

jonatack commented at 10:33 am on September 5, 2022: contributor

ACK 28ae912f501e1a09bd050dcf35ecf712374f743f

I agree that this should be tagged for the v24.0 milestone, have run across this issue myself.

MarcoFalke added this to the milestone 24.0 on Sep 5, 2022

init: allow startup with -onlynet=onion -listenonion=1

It does not make sense to specify `-onlynet=onion` without providing a
Tor proxy (even if other `-onlynet=...` are given). This is checked
during startup. However, it was forgotten that a Tor proxy can also be
retrieved from "Tor control" to which we connect if `-listenonion=1`.

So, the full Tor proxy retrieval logic is:
1. get it from `-onion`
2. get it from `-proxy`
3. if `-listenonion=1`, then connect to "Tor control" and get the proxy
   from there (was forgotten before this change)

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

2d0b4e4ff6

vasild force-pushed on Sep 5, 2022

vasild commented at 3:56 pm on September 5, 2022: contributor

28ae912f50...2d0b4e4ff6: rebase because this was old (no clear conflicts though) + pick suggestion + reword error message for consistency with the other message in this PR and with #25989.

Invalidates ACKs from @dunxen, @jonatack

in src/init.cpp:1338 in 2d0b4e4ff6

1332@@ -1326,11 +1333,14 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1333     if (onion_proxy.IsValid()) {
1334         SetProxy(NET_ONION, onion_proxy);
1335     } else {
1336-        if (args.IsArgSet("-onlynet") && IsReachable(NET_ONION)) {
1337+        // If -listenonion is set, then we will (try to) connect to the Tor control port
1338+        // later from the torcontrol thread and may retrieve the onion proxy from there.
1339+        const bool listenonion_disabled{!args.GetBoolArg("-listenonion", DEFAULT_LISTEN_ONION)};

mzumsande commented at 3:39 pm on September 6, 2022:

Since DEFAULT_LISTEN_ONION=true, this makes the InitError go away in some cases where it would be useful - in particular in the (probably very typical) case where a user just starts ./bitcoind -onlynet=onion without providing any additional configuration and hopes everything works together out of the box. The user would have to actively set -listenonion=0 or listen=0 to still get this error.

I wonder if it might be more helpful to change the InitError to a info/warning (something along the lines of “Did you maybe forgot to provide a tor proxy? Disregard this if you use torcontrol”) that is not conditional on -listenonion Not a strong suggestion though, I am not sure either way.

vasild commented at 10:53 am on September 7, 2022:

this makes the InitError go away in some cases where it would be useful … ./bitcoind -onlynet=onion

That is exactly the case where we do not want an init error – the problem described in #24980 which this PR aims to fix. Because it could as well work out of the box, giving an init error is too strict and is relaxed by this PR.

Now, it is true that we do not know for sure, at this stage, at startup, whether we would be able to connect to torcontrol and retrieve the proxy address from it. I think it is proper to allow startup and later, if the tor control thread fails to retrieve the proxy it will log the relevant error. That is similar (but not identical) to the case where -proxy= is given and at startup we do not know if it will work and actually proxy connections. Maybe later we would find out that nobody is listening on the given proxy address and will log an error then.

That is, we give init errors only if we are sure, guaranteed that it will not work.

I wonder if it might be more helpful to change the InitError to a info/warning

I am a bit confused. Do you mean to keep the init error as it is in this PR, but in the case where we silently allow startup (the new relaxation in this PR) to log a warning? I think it would not be necessary because it could be annoying for the cases when all is ok. For the cases when it is not, then the tor control thread will log a proper message if/when the problem occurs. Also, “Disregard this if you use torcontrol” – -torcontrol is irrelevant here – even if not specified it has a reasonable default. The relevant setting is -listenonion=1.

Off topic rant: I find the name listenonion confusing, it should be something like -auto_create_tor_listening_service_via_torcontrol because -listenonion does not control whether or not we listen on 127.0.0.1:8334 and the tor service could be setup in tor.conf (the “static” way in which we should have -listenonion=0, but would still be listening and accepting incoming tor connections).

mzumsande commented at 7:13 pm on September 7, 2022:

I was just thinking about the scenario where a user installs and starts tor, then ./bitcoind -onlynet=onion, but didn’t setup a Onion Service as described in tor.md because they didn’t read the doc. In that case they wouldn’t make outbound connections and I think also get no error or warning from the torcontrol Thread unless they have debug=tor enabled.

vasild commented at 8:47 am on September 8, 2022:

but didn’t setup a Onion Service as described in tor.md

There are two ways - automatic and manual, which one do you mean?

What you describe above could work with the automatic way:

bitcoind finds the tor control at 127.0.0.1:9051 (no need to specify -torcontrol=127.0.0.1:9051 since that is the default).
The tor daemon tells bitcoind where is the tor cookie file and bitcoind is able to read it. Authentication is successful.
bitcoind automatically creates the onion service and starts listening and accepting incoming tor connections (locally at 127.0.0.1:8334)
bitcoind retrieves the address of the socks5 tor proxy from tor control and sets -onion to it, for making outbound connections to tor peers.

If 2. fails then there will be some error in the log, either from TorController::auth_cb() or from TorController::protocolinfo_cb().

If 4. fails, there will be an error from TorController::get_socks_cb().

mzumsande commented at 6:04 pm on September 8, 2022:

I mean the automatic way. If step 1) is not successful, there will be repeated messages:

0[tor] Error connecting to Tor control socket
1[tor] Not connected to Tor control port 127.0.0.1:9051, trying to reconnect

that are usually hidden in the tor debug category, because with -listenonion=1 being the default, we probably don’t even intend to use anything but clearnet and don’t need to see those. These messages would become kind of relevant though if we used -onlynet=onion because we’d not find any peers without being able to find tor control, so it might be helpful to log something unconditionally in this case. But these are logging nits that don’t have to be discussed in this PR and certainly shouldn’t hold it off, so I think you should just go ahead and resolve this thread.

vasild commented at 1:12 pm on September 9, 2022:

… a user installs and starts tor, then ./bitcoind -onlynet=onion …

For 1. to fail, then tor is not listening on 127.0.0.1:9051 which it does by default. So, maybe the user did not install and start tor or did so, but fiddled with its config and changed the listening address.

Anyway, maybe this change makes sense:

 0--- i/src/torcontrol.cpp
 1+++ w/src/torcontrol.cpp
 2@@ -612,13 +612,26 @@ void TorController::disconnected_cb(TorControlConnection& _conn)
 3     if (service.IsValid())
 4         RemoveLocal(service);
 5     service = CService();
 6     if (!reconnect)
 7         return;
 8
 9-    LogPrint(BCLog::TOR, "Not connected to Tor control port %s, trying to reconnect\n", m_tor_control_
10+    const auto onlynets = gArgs.GetArgs("-onlynet");
11+    if (gArgs.IsArgSet("-onlynet") &&
12+        std::any_of(onlynets.begin(), onlynets.end(), [](const auto& n) {
13+            return ParseNetwork(n) == NET_ONION;
14+        })) {
15+        LogPrintf("Not connected to Tor control port %s, trying to reconnect. "
16+                  "Use -listenonion=0 to disable the tor control thread.\n",
17+                  m_tor_control_center);
18+    } else {
19+        LogPrint(BCLog::TOR,
20+                 "Not connected to Tor control port %s, trying to reconnect. "
21+                 "Use -listenonion=0 to disable the tor control thread.\n",
22+                 m_tor_control_center);
23+    }
24
25     // Single-shot timer for reconnect. Use exponential backoff.

Keep it in the debug log only if -onlynet=onion is not given, because for users that did not install tor and do not care about tor and just run bitcoind this would be just noise.

kristapsk commented at 7:01 pm on September 6, 2022: contributor

Concept ACK

mzumsande commented at 7:13 pm on September 7, 2022: contributor

Tested ACK 2d0b4e4ff66e60c85f86c526a53f8fb242ebb7d0

MarcoFalke commented at 8:59 am on September 12, 2022: member

ACK 2d0b4e4ff66e60c85f86c526a53f8fb242ebb7d0 🕸

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 2d0b4e4ff66e60c85f86c526a53f8fb242ebb7d0 🕸
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgf6gv+JHLZSJ81Lalmx6JwgWyyjItQKB/aJMLaEK32S1zCAgLEEWK0RLm6L9XK
 8agQsA3ShTx7s25Lpa50vESdibiK/gGARpd2Wt8k0KsSdq4swZQiA5GnH2MIVGCY/
 9tS6H0bNnZUxNtq5yY9i0bT7MTPbjd0QTeQUs3KZHlmZBgigEkMXfIYyW24LtgTiE
10rDo5Q70SXeQde2tbxKAvUET3TIdVCRj8sZ6+qyqP2VS9w54DfKA4w/q8jnv5RC8q
112glKU/0RMrQ7M23EH1vFH6Qp9TWV6/PQhKFB9LgyciSGGcBfG8IwF4Elhsc34pxV
12TgIISzdWBhjrWG6ql78eXMT2vJAfmPCD+pThkBc9nxzbIeU/CBSQMwYCjyhDOyN0
13mX0xpeDdjuCb3rIHgG2qDgrvBs9FkL2zNOAbKYyqzkIwCYfDNMDDYHDrYpqiGd7B
14CKTL0X9zUB7+aIao36dM7QkcT8Q8V8eL9nEY8wZ4JwAgF+8ujZ9yp0rJ3uB7tOZx
15vz9IevYu
16=4zdT
17-----END PGP SIGNATURE-----

fanquake merged this on Sep 13, 2022

fanquake closed this on Sep 13, 2022

sidhujag referenced this in commit 58d242900d on Sep 13, 2022

vasild deleted the branch on Sep 14, 2022

bitcoin locked this on Sep 15, 2023

init: allow startup with -onlynet=onion -listenonion=1 #24991

Conflicts