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

vasild · 2022-04-26T09:21:55Z

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 #24980

vasild · 2022-04-26T09:48:59Z

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

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

jonatack

Light Approach ACK (looked quickly, in seminar ATM)

₿ bitcoind -signet -onlynet=onion -listenonion=0
2022-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
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
2022-04-26T12:11:00Z [init] Shutdown: In progress...
2022-04-26T12:11:00Z [scheduler] scheduler thread exit
2022-04-26T12:11:00Z [shutoff] Shutdown: done

src/init.cpp

jonatack · 2022-04-26T12:19:50Z

test/functional/feature_proxy.py

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

- self.log.info("Test passing -onlynet=onion without -proxy, -onion or -listenonion raises expected init error") - self.nodes[1].extra_args = ["-onlynet=onion", "-listenonion=0"] msg = ( "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" ) + + self.log.info("Test passing -onlynet=onion without -proxy or -onion raises expected init error") + self.nodes[1].extra_args = ["-onlynet=onion", "-listen"] + self.nodes[1].assert_start_raises_init_error(expected_msg=msg) + + self.log.info("Test passing -onlynet=onion and -listen, without -proxy or -onion, and with -listenonion disabled raises expected init error") + self.nodes[1].extra_args = ["-onlynet=onion", "-listen", "-listenonion=0"] self.nodes[1].assert_start_raises_init_error(expected_msg=msg)

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.

dunxen · 2022-04-26T16:51:51Z

Concept ACK

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

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

Nice!

dunxen · 2022-04-28T07:10:36Z

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:

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

laanwj · 2022-04-28T08:33:38Z

@midnightmagic Can you confirm this solves your issue?

vasild · 2022-04-29T13:21:06Z

bbbb1755da...c313568a97: address suggestions

schildbach · 2022-05-01T21:38:59Z

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 · 2022-05-02T07:25:56Z

@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

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.

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 bitcoin#24980 Github-Pull: bitcoin#24991 Rebased-From: c313568a97bbac41d7f372f62594e440a515e1f5

vasild · 2022-05-23T09:23:52Z

c313568a97...28ae912f50: rebase due to conflicts

Previously invalidated ACK from @dunxen

dunxen · 2022-05-23T11:33:30Z

re-ACK 28ae912

mzumsande · 2022-09-02T20:52:33Z

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

DrahtBot · 2022-09-04T08:30:07Z

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.

jonatack

ACK 28ae912f501e1a09bd050dcf35ecf712374f743f

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

jonatack · 2022-09-05T10:02:21Z

src/init.cpp

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

- _("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 explicitly forbidden: -onion=0"));

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

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

I think the explicit version is clearer.

jonatack · 2022-09-05T10:33:04Z

test/functional/feature_proxy.py

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

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

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.

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 bitcoin#24980

vasild · 2022-09-05T15:56:50Z

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

src/init.cpp

kristapsk · 2022-09-06T19:01:28Z

Concept ACK

mzumsande · 2022-09-07T19:13:29Z

Tested ACK 2d0b4e4

maflcko · 2022-09-12T08:59:41Z

ACK 2d0b4e4 🕸

Show signature

Signature:

…nion=1 2d0b4e4 init: allow startup with -onlynet=onion -listenonion=1 (Vasil Dimov) Pull request description: 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 bitcoin#24980 ACKs for top commit: mzumsande: Tested ACK 2d0b4e4 MarcoFalke: ACK 2d0b4e4 🕸 Tree-SHA512: d1d18e07a8a40a47b7f00c31cb291a3d3a9b24eeb28c5e4720d5df4997f488583a3a010d46902b4b600d2ed1136a368e1051c133847ae165e0748b8167603dc3

vasild mentioned this pull request Apr 26, 2022

-onlynet=onion plus -onlynet=ipv4/etc, without -onion=, in spite of -torcontrol= specified, incorrectly kills startup #24980

Closed

vasild force-pushed the onlynet_onion_with_listenonion_is_ok branch from 111bec2 to bbbb175 Compare April 26, 2022 09:45

DrahtBot added the Tests label Apr 26, 2022

jonatack reviewed Apr 26, 2022

View reviewed changes

laanwj added the P2P label Apr 26, 2022

vasild force-pushed the onlynet_onion_with_listenonion_is_ok branch from bbbb175 to c313568 Compare April 29, 2022 13:21

jonatack reviewed May 5, 2022

View reviewed changes

DrahtBot added the Needs rebase label May 20, 2022

vasild force-pushed the onlynet_onion_with_listenonion_is_ok branch from c313568 to 28ae912 Compare May 23, 2022 09:22

DrahtBot removed the Needs rebase label May 23, 2022

DrahtBot mentioned this pull request Sep 3, 2022

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

Merged

jonatack reviewed Sep 5, 2022

View reviewed changes

maflcko added this to the 24.0 milestone Sep 5, 2022

vasild force-pushed the onlynet_onion_with_listenonion_is_ok branch from 28ae912 to 2d0b4e4 Compare September 5, 2022 15:54

mzumsande reviewed Sep 6, 2022

View reviewed changes

src/init.cpp Show resolved Hide resolved

fanquake merged commit 94d1784 into bitcoin:master Sep 13, 2022

vasild deleted the onlynet_onion_with_listenonion_is_ok branch September 14, 2022 09:38

ghost mentioned this pull request Sep 29, 2022

contrib: add vasild to trusted keys #25871

Closed

bitcoin locked and limited conversation to collaborators Sep 15, 2023

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

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

Uh oh!

Conversation

vasild commented Apr 26, 2022 • edited by fanquake Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasild commented Apr 26, 2022

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jonatack Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vasild Apr 29, 2022

Choose a reason for hiding this comment

Uh oh!

dunxen commented Apr 26, 2022

Uh oh!

dunxen commented Apr 28, 2022

Uh oh!

laanwj commented Apr 28, 2022

Uh oh!

vasild commented Apr 29, 2022

Uh oh!

schildbach commented May 1, 2022

Uh oh!

vasild commented May 2, 2022

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

vasild commented May 23, 2022

Uh oh!

dunxen commented May 23, 2022

Uh oh!

mzumsande commented Sep 2, 2022

Uh oh!

DrahtBot commented Sep 4, 2022

Conflicts

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Sep 5, 2022

Choose a reason for hiding this comment

Uh oh!

vasild Sep 5, 2022

Choose a reason for hiding this comment

Uh oh!

vasild Sep 5, 2022

Choose a reason for hiding this comment

Uh oh!

jonatack Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

jonatack Sep 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vasild Sep 5, 2022

Choose a reason for hiding this comment

Uh oh!

vasild commented Sep 5, 2022

Uh oh!

Uh oh!

kristapsk commented Sep 6, 2022

Uh oh!

mzumsande commented Sep 7, 2022

Uh oh!

maflcko commented Sep 12, 2022

Uh oh!

Reviewers

Assignees

Labels

vasild commented Apr 26, 2022 •

edited by fanquake

Loading

jonatack left a comment •

edited

Loading

jonatack Apr 26, 2022 •

edited

Loading

jonatack Sep 5, 2022 •

edited

Loading