Make it possible to disable Tor binds and abort startup on bind failure #22729

vasild · 2021-08-17T14:16:34Z

Make it possible to disable the Tor binding on 127.0.0.1:8334 and stop startup if any P2P bind fails instead of "if all P2P binds fail".

Fixes #22726
Fixes #22727

vasild · 2021-08-17T14:16:58Z

cc @hebasto

jonatack · 2021-08-17T17:06:54Z

The test changes seems sporadic (and often pass on master for me locally)
Would 1597069 disable our inbound onion detection? (CNode::m_inbound_onion in net.h)

vasild · 2021-08-17T17:47:24Z

Would 1597069 disable our inbound onion detection? (CNode::m_inbound_onion in net.h)

If both -bind=... and -bind=...=onion are given -- no
If none of -bind=... or -bind=...=onion are given -- no
If only -bind=... is given, without -bind=...=onion -- yes
If only -bind=...=onion is given, without -bind=... -- no

fanquake · 2021-08-18T01:05:04Z

CI failure https://cirrus-ci.com/task/5600680695037952:

89/217 - feature_bind_extra.py failed, Duration: 31 s
stdout:
2021-08-17T14:41:23.064000Z TestFramework (INFO): Initializing test directory /tmp/cirrus-ci-build/ci/scratch/test_runner/test_runner_₿_🏃_20210817_143957/feature_bind_extra_170
2021-08-17T14:41:23.064000Z TestFramework (INFO): Checking for Linux
2021-08-17T14:41:53.701000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 530, in start_nodes
    node.wait_for_rpc_connection()
  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_node.py", line 225, in wait_for_rpc_connection
    'bitcoind exited with status {} during initialization'.format(self.process.returncode)))
test_framework.test_node.FailedToStartError: [node 0] bitcoind exited with status 1 during initialization
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 130, in main
    self.setup()
  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 285, in setup
    self.setup_network()
  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/feature_bind_extra.py", line 95, in setup_network
    self.setup_nodes()
  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 404, in setup_nodes
    self.start_nodes()
  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 533, in start_nodes
    self.stop_nodes()
  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 548, in stop_nodes
    node.stop_node(wait=wait, wait_until_stopped=False)
  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_node.py", line 323, in stop_node
    self.stop(wait=wait)
  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_node.py", line 183, in __getattr__
    assert self.rpc_connected and self.rpc is not None, self._node_msg("Error: no RPC connection")
AssertionError: [node 0] Error: no RPC connection
2021-08-17T14:41:53.753000Z TestFramework (INFO): Stopping nodes

Rspigler · 2021-08-18T02:10:04Z

#20234 For reference for related changes.

I might propose some Tor doc clarifications in a separate PR. There has been a lot of confusion around onlynet, and users thinking that that affects outgoing and incoming connections.

laanwj · 2021-09-15T09:34:06Z

Concept ACK (will review in more detail). It would be better if any error in binding configuration stopped the process, not just onion-related ones, as it can lead to surprising outcomes. But historically there were some issues with libevent in getting this right see #14968.

src/net.cpp

vasild · 2021-09-16T12:31:51Z

f89f80ef2d...57e0c5d32e: rebase and stop startup if any^* of the P2P binds fail, instead of all.

^*except the bind on IPv6 any address :: when the user did not explicitly ask for it because the machine may not have IPv6 support.

It would be better if any error in binding configuration stopped the process, not just onion-related ones, as it can lead to surprising outcomes.

I agree. Changed. If we want to also do the same for ^* then we would have to check whether socket(2) failed due to EAFNOSUPPORT or bind(2) failed due to EADDRNOTAVAIL. I think that would be an overkill.

vasild · 2021-09-16T13:06:02Z

The first commit ceeb317 net: don't extra bind for Tor if binds are restricted was part of #20234 but was removed so that PR could proceed quickly (it was deemed controversial).

After #22727 (comment) I am even more convinced that this is the right approach as it reduces user surprises (and those surprises can have highly undesirable outcomes). It must be possible to disable the binding on 127.0.0.1:8334 somehow. Otherwise, if we consider those binds fatal, two bitcoinds would collide on that port.

For example our functional testing framework starts >1 bitcoind and all of them collide on 127.0.0.1:8334 but the bind errors are ignored 👀. If we are to consider bind errors fatal (I think we should) and ceeb317 net: don't extra bind for Tor if binds are restricted is not desirable, then the testing framework would need to be adjusted to avoid the collisions. I don't object this, but I am not going to implement it because I think the approach in this PR is a better one. I would review other approaches if somebody bothers to go in another direction.

laanwj · 2021-09-27T11:58:59Z

For example our functional testing framework starts >1 bitcoind and all of them collide on 127.0.0.1:8334 but the bind errors are ignored eyes

There is no reason for the test framework to be creating Tor binds at all. It's kind of surprising to me that it does by default but I understand why.

I think the long term solution would be to only create the Tor bind when either requested through a command line flag, or when it succesfully connects to Tor (e.g. in torcontrol). But this would require dynamically changing (at least adding) bindings so isn't very straightforward.

vasild · 2022-06-13T13:20:13Z

57e0c5d32e...3d54f24f02: rebase due to conflicts

DrahtBot · 2022-07-30T07:56:40Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	cbergqvist, pinheadmz, achow101
Concept ACK	laanwj, stratospher, jonatack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28710 (Remove the legacy wallet and BDB dependency by achow101)

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.

vasild · 2022-10-25T09:58:18Z

3d54f24f02...7180859e7e: rebase due to conflicts

vasild · 2022-11-14T09:42:45Z

I think the long term solution would be to only create the Tor bind when either requested through a command line flag, or when it succesfully connects to Tor (e.g. in torcontrol)

This would also help binding on the proper address and announcing it to the tor daemon. If tor and bitcoind run on the same machine the default 127.0.0.1 works smoothly.

But it gets more involved in setups where tor is on another machine. Then the default -bind=127.0.0.1:8334=onion is not ok anymore and the operator has to use something like -torcontrol=192.168.0.5:9051 -bind=192.168.0.6:8334=onion. Looks easy enough, but is further complicated in environments where IP addresses are dynamically set up (DHCP, Docker) and the fact that we don't resolve the argument to -bind= so a hostname can't be used there.

There is also the special case -bind=0.0.0.0:8334=onion - then we would tell the tor daemon that the service is listening on 0.0.0.0:8334 and it will not be able to connect to it.

If we postpone the binding to after we have connected to tor control, then we can automatically figure out the best address to bind to and announce that (if one is not explicitly configured).

#26484 is related, thanks @schildbach!

In the Tor case, this prevents us from telling the Tor daemon to send our incoming connections from the Tor network to an address where we do not listen (we tried to listen but failed probably because another application is already listening). In the other cases (IPv4/IPv6 binds) this also prevents unpleasant surprises caused by continuing operations even on bind failure. For example, another application may be listening on portX, bitcoind tries to bind on portX and portY, only succeeds with portY and continues operation leaving the user thinking that his bitcoind is listening on portX whereas another application is listening (the error message in the log could easily be missed). Avoid having the functional testing framework start multiple `bitcoind`s that try to listen on the same `127.0.0.1:18445` (Tor listen for regtest) if `bind_to_localhost_only` is set to `False`. Also fix a typo in `test-shell.md` related to `bind_to_localhost_only`. Fixes bitcoin#22727

vasild · 2024-07-02T12:20:57Z

ffe9b27498...bca346a970: take minor suggestion which I forgot earlier: use expected_services instead of self.expected[i][1] in the test.

cbergqvist

ACK bca346a

The current solution means most test nodes in the functional test suite will now bind to an onion port even though the vast majority are not testing Tor

Only tests that use bind_to_localhost_only will do. We have 5 such tests.

We have 5 tests that disable bind_to_localhost_only, all the others will bind=127.0.0.1. Having that will result in your new has_explicit_bind being true for the majority of tests, which will lead to your later added binds not being set for the majority of tests.
=> So only the 5 tests that disable bind_to_localhost_only will get into this Tor exception. Good, thanks for correcting me on how many tests are impacted!

The previous incarnation of this PR (8c30871) had the tests not bind on Tor, because it is not needed (the vast majority are not testing Tor), but you did not like that.

What I do not like is the special casing inside of test_node.py that feels like a dirty workaround to me instead of having individual tests pass in the arguments they require (such as -listenonion), which in turn leads to binding of otherwise inactive Tor ports. But looking back at the comment history I understand how bitcoind behaves by default might be the underlying reason preventing this cleaner proposal from being implemented.

pinheadmz

ACK bca346a

Changes since last ACK extremely minimal, cleaning up one line in the test

Show Signature

pinheadmz's public key is on keybase

achow101 · 2024-07-16T20:26:58Z

ACK bca346a

After \bitcoin#22729, if a `-bind` is set, we no longer bind `127.0.0.1:8334` as an onion port by default.

After bitcoin#22729 if `-bind=127.0.0.1` is used, then bitcoind will not automatically also bind on `127.0.0.1:8334`.

6d33e13 doc: tor.md: use -bind=127.0.0.1:8334=onion for the Tor bind (David Gumberg) a7f5d18 doc: add release notes for #22729 (Vasil Dimov) Pull request description: Add release notes for #22729. ACKs for top commit: davidgumberg: reACK 6d33e13 willcl-ark: ACK 6d33e13 Tree-SHA512: 9d7e66ee1d0bb1d75b8273707d30f20915d5040a768c2c5cd47c84997df2645c8bec35db6c09dc77ab917836622411b924373816cbc83c4be38e2e9156a139d8

When the bind option is replaced in the bitcoin.conf, bitcoind will attempd to bind to the default tor listening port. If another bitcoind is running that is already bound to that port, the bind will fail which, since bitcoin#22729, causes the test to fail.

When the bind option is replaced in the bitcoin.conf, bitcoind will attempd to bind to the default tor listening port. If another bitcoind is running that is already bound to that port, the bind will fail which, since bitcoin#22729, causes the test to fail. This failure can be avoided by explicitly binding the tor port when the bind is removed.

082779d test: Add explicit onion bind to p2p_permissions (Ava Chow) Pull request description: When the bind option is replaced in the bitcoin.conf, bitcoind will attempd to bind to the default tor listening port. If another bitcoind is running that is already bound to that port, the bind will fail which, since #22729, causes the test to fail. This failure can be avoided by explicitly binding the tor port when the bind is removed. ACKs for top commit: tdb3: ACK 082779d theStack: re-ACK 082779d glozow: ACK 082779d Tree-SHA512: 4acb69ea2e00aeacf9e7c9ab9595ceaf0e0d2adbd795602034b2184197d9bad54c7bc9f3da43ef9c52a71869fe96ba8c87fc5b7c37880f258f5a2aaab2b4046c

After bitcoin/bitcoin#22729 if `-bind=127.0.0.1` is used, then bitcoind will not automatically also bind on `127.0.0.1:8334`.

Summary: ``` Make it possible to disable the Tor binding on 127.0.0.1:8334 and stop startup if any P2P bind fails instead of "if all P2P binds fail". ``` Backport of [[bitcoin/bitcoin#22729 | core#22729]]. Depends on D17661. Test Plan: ninja check-functional Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Subscribers: PiRK Differential Revision: https://reviews.bitcoinabc.org/D17662

DrahtBot added the P2P label Aug 17, 2021

fanquake requested a review from laanwj August 18, 2021 01:03

vasild force-pushed the torbind branch from 05db6b4 to f89f80e Compare August 18, 2021 10:58

vasild mentioned this pull request Sep 15, 2021

No check is done whether the binding for Tor succeeded before using it for ADD_ONION #22727

Closed

laanwj reviewed Sep 15, 2021

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

laanwj reviewed Sep 15, 2021

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

vasild force-pushed the torbind branch from f89f80e to 57e0c5d Compare September 16, 2021 12:27

vasild changed the title ~~Make it possible to disable and check for error in -bind=...=onion~~ Make it possible to disable onion binds and abort startup on bind failure Sep 16, 2021

vasild changed the title ~~Make it possible to disable onion binds and abort startup on bind failure~~ Make it possible to disable Tor binds and abort startup on bind failure Sep 16, 2021

DrahtBot added the Needs rebase label Jun 10, 2022

vasild force-pushed the torbind branch from 57e0c5d to 3d54f24 Compare June 13, 2022 13:20

DrahtBot removed the Needs rebase label Jun 13, 2022

DrahtBot mentioned this pull request Jul 29, 2022

net: remove useless call to IsReachable() from CConnman::Bind() #25735

Merged

DrahtBot added the Needs rebase label Oct 3, 2022

vasild force-pushed the torbind branch from 3d54f24 to 7180859 Compare October 25, 2022 09:57

DrahtBot removed the Needs rebase label Oct 25, 2022

vasild mentioned this pull request Nov 17, 2022

Error: Cannot resolve -bind address: 'bitcoind:8334=onion' #26484

Open

vasild force-pushed the torbind branch from ffe9b27 to bca346a Compare July 2, 2024 12:19

cbergqvist approved these changes Jul 3, 2024

View reviewed changes

DrahtBot requested a review from pinheadmz July 3, 2024 23:17

pinheadmz approved these changes Jul 12, 2024

View reviewed changes

achow101 merged commit 45750f6 into bitcoin:master Jul 16, 2024

vasild deleted the torbind branch July 17, 2024 07:48

vasild added a commit to vasild/bitcoin that referenced this pull request Jul 23, 2024

doc: add release notes for bitcoin#22729

620bbc7

vasild mentioned this pull request Jul 23, 2024

doc: add release notes for #22729 #30502

Merged

vasild added a commit to vasild/bitcoin that referenced this pull request Jul 23, 2024

doc: add release notes for bitcoin#22729

3076597

vasild added a commit to vasild/bitcoin that referenced this pull request Jul 31, 2024

doc: add release notes for bitcoin#22729

a7f5d18

davidgumberg added a commit to davidgumberg/bitcoin that referenced this pull request Aug 1, 2024

doc: reflect default tor binding changes in bitcoin#22729

39afcd0

After \bitcoin#22729, if a `-bind` is set, we no longer bind `127.0.0.1:8334` as an onion port by default.

vasild pushed a commit to vasild/bitcoin that referenced this pull request Aug 2, 2024

doc: tor.md: use -bind=127.0.0.1:8334=onion for the Tor bind

6d33e13

After bitcoin#22729 if `-bind=127.0.0.1` is used, then bitcoind will not automatically also bind on `127.0.0.1:8334`.

achow101 mentioned this pull request Sep 3, 2024

test: Add explicit onion bind to p2p_permissions #30805

Merged

laanwj mentioned this pull request Oct 22, 2024

net: Tor service target port collides when running multiple nodes, making bitcoind error out #31133

Closed

mzumsande mentioned this pull request Oct 22, 2024

net: multiple-node local setups with different -port don't work anymore #31134

Closed

This comment was marked as off-topic.

Sign in to view

fanquake mentioned this pull request Nov 25, 2024

Discover() will not run if listening on any address with an explicit bind=0.0.0.0 #31293

Open

janus pushed a commit to BitgesellOfficial/bitgesell that referenced this pull request Jan 6, 2025

doc: tor.md: use -bind=127.0.0.1:8334=onion for the Tor bind

f223969

After bitcoin/bitcoin#22729 if `-bind=127.0.0.1` is used, then bitcoind will not automatically also bind on `127.0.0.1:8334`.

xyephy mentioned this pull request Feb 26, 2025

Upgrade to bitcoin v28 whiveio/whive#12

Open

Make it possible to disable Tor binds and abort startup on bind failure #22729

Make it possible to disable Tor binds and abort startup on bind failure #22729

Uh oh!

Conversation

vasild commented Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasild commented Aug 17, 2021

Uh oh!

jonatack commented Aug 17, 2021

Uh oh!

vasild commented Aug 17, 2021

Uh oh!

fanquake commented Aug 18, 2021

Uh oh!

Rspigler commented Aug 18, 2021

Uh oh!

laanwj commented Sep 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vasild commented Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasild commented Sep 16, 2021

Uh oh!

laanwj commented Sep 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasild commented Jun 13, 2022

Uh oh!

DrahtBot commented Jul 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

vasild commented Oct 25, 2022

Uh oh!

vasild commented Nov 14, 2022

Uh oh!

vasild commented Jul 2, 2024

Uh oh!

cbergqvist left a comment

Choose a reason for hiding this comment

Uh oh!

pinheadmz left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jul 16, 2024

Uh oh!

This comment was marked as off-topic.

Uh oh!

Uh oh!

vasild commented Aug 17, 2021 •

edited

Loading

laanwj commented Sep 15, 2021 •

edited

Loading

vasild commented Sep 16, 2021 •

edited

Loading

laanwj commented Sep 27, 2021 •

edited

Loading

DrahtBot commented Jul 30, 2022 •

edited

Loading