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

vasild commented at 2:16 pm on August 17, 2021: contributor

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 https://github.com/bitcoin/bitcoin/issues/22727

vasild commented at 2:16 pm on August 17, 2021: contributor

cc @hebasto

DrahtBot added the label P2P on Aug 17, 2021

jonatack commented at 5:06 pm on August 17, 2021: contributor

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

vasild commented at 5:47 pm on August 17, 2021: contributor

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 requested review from laanwj on Aug 18, 2021

fanquake commented at 1:05 am on August 18, 2021: member

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

 089/217 - feature_bind_extra.py failed, Duration: 31 s
 1stdout:
 22021-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
 32021-08-17T14:41:23.064000Z TestFramework (INFO): Checking for Linux
 42021-08-17T14:41:53.701000Z TestFramework (ERROR): Assertion failed
 5Traceback (most recent call last):
 6  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
 7    node.wait_for_rpc_connection()
 8  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
 9    'bitcoind exited with status {} during initialization'.format(self.process.returncode)))
10test_framework.test_node.FailedToStartError: [node 0] bitcoind exited with status 1 during initialization
11During handling of the above exception, another exception occurred:
12Traceback (most recent call last):
13  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
14    self.setup()
15  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
16    self.setup_network()
17  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
18    self.setup_nodes()
19  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
20    self.start_nodes()
21  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
22    self.stop_nodes()
23  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
24    node.stop_node(wait=wait, wait_until_stopped=False)
25  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
26    self.stop(wait=wait)
27  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__
28    assert self.rpc_connected and self.rpc is not None, self._node_msg("Error: no RPC connection")
29AssertionError: [node 0] Error: no RPC connection
302021-08-17T14:41:53.753000Z TestFramework (INFO): Stopping nodes

Rspigler commented at 2:10 am on August 18, 2021: contributor

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

vasild force-pushed on Aug 18, 2021

laanwj commented at 9:34 am on September 15, 2021: member

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.

in src/net.cpp:2510 in f89f80ef2d outdated

2506@@ -2505,7 +2507,7 @@ bool CConnman::InitBinds(const Options& options)
2507         fBound |= Bind(CService(inaddr6_any, GetListenPort()), BF_NONE, NetPermissionFlags::None);
2508         fBound |= Bind(CService(inaddr_any, GetListenPort()), !fBound ? BF_REPORT_ERROR : BF_NONE, NetPermissionFlags::None);
2509     }
2510-    return fBound;
2511+    return fBound && (bound_onion || options.onion_binds.empty());

laanwj commented at 9:40 am on September 15, 2021:

I dislike making this logic more and more complex, special-casing for onion binds. I’d prefer something more general instead that will solve this and surrounding issues.

vasild commented at 12:33 pm on September 16, 2021:

Changed the code to consider bind failures fatal.

in src/net.cpp:2501 in f89f80ef2d outdated

2494@@ -2495,8 +2495,10 @@ bool CConnman::InitBinds(const Options& options)
2495     for (const auto& addrBind : options.vWhiteBinds) {
2496         fBound |= Bind(addrBind.m_service, (BF_EXPLICIT | BF_REPORT_ERROR), addrBind.m_flags);
2497     }
2498+    bool bound_onion{false};
2499     for (const auto& addr_bind : options.onion_binds) {
2500-        fBound |= Bind(addr_bind, BF_EXPLICIT | BF_DONT_ADVERTISE, NetPermissionFlags::None);
2501+        bound_onion |= Bind(addr_bind, BF_EXPLICIT | BF_REPORT_ERROR | BF_DONT_ADVERTISE, NetPermissionFlags::None);
2502+        fBound |= bound_onion;

laanwj commented at 9:41 am on September 15, 2021:

Please move the fBound |= bound_onion; out of the loop. No point in doing it every iteration, and it (imo) makes it sightly easier to reason about.

vasild commented at 12:32 pm on September 16, 2021:

Not relevant with the latest push.

vasild force-pushed on Sep 16, 2021

vasild commented at 12:31 pm on September 16, 2021: contributor

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 renamed this:
~~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
on Sep 16, 2021

vasild renamed this:
~~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
on Sep 16, 2021

vasild commented at 1:06 pm on September 16, 2021: contributor

The first commit ceeb317cde 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 :eyes:. If we are to consider bind errors fatal (I think we should) and ceeb317cde 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 commented at 11:58 am on September 27, 2021: member

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.

DrahtBot added the label Needs rebase on Jun 10, 2022

vasild force-pushed on Jun 13, 2022

vasild commented at 1:20 pm on June 13, 2022: contributor

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

DrahtBot removed the label Needs rebase on Jun 13, 2022

DrahtBot commented at 7:56 am on July 30, 2022: contributor

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.

DrahtBot added the label Needs rebase on Oct 3, 2022

vasild force-pushed on Oct 25, 2022

vasild commented at 9:58 am on October 25, 2022: contributor

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

DrahtBot removed the label Needs rebase on Oct 25, 2022

vasild commented at 9:42 am on November 14, 2022: contributor

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!

stratospher commented at 8:50 pm on March 20, 2023: contributor

Concept ACK. This feature would be useful to have! Observed how before this PR, specifying -bind=addr:port(without =onion) led to vBinds and onion_binds filling up and both of them getting used in InitBinds(). After this PR, we don’t fill onion_binds with the default value; vBinds is filled which gets used in InitBinds(). Also liked the specificity in error messages obtained when serially checking whether we need to abort or not during startup.

only concern is if there’s a way around #22729 (comment) - the need for =onion to accept inbound connections wouldn’t be intuitive to users and might break existing configs?

jonatack commented at 2:47 pm on April 18, 2023: contributor

Concept ACK

vasild force-pushed on Aug 16, 2023

vasild commented at 1:42 pm on August 16, 2023: contributor

7180859e7e...6a51eaf4a9: rebase, did not lose any relevance through time

An alternative to this is described in: #22729 (comment) that would be more complicated, but maybe it would be more appealing to reviewers?

only concern is if there’s a way around #22729 (comment) - the need for =onion to accept inbound connections wouldn’t be intuitive to users and might break existing configs?

Maybe refuse to start with an error if -bind= is used, -bind=...=onion is not and we will be accepting tor connections (e.g. -listenonion=1)?

DrahtBot added the label CI failed on Oct 30, 2023

DrahtBot removed the label CI failed on Oct 31, 2023

DrahtBot added the label Needs rebase on Jan 11, 2024

vasild force-pushed on Jan 18, 2024

vasild commented at 11:54 am on January 18, 2024: contributor

6a51eaf4a9...d0c8109dd0: rebase due to conflicts

DrahtBot removed the label Needs rebase on Jan 18, 2024

DrahtBot added the label CI failed on Jan 24, 2024

DrahtBot removed the label CI failed on Jan 29, 2024

achow101 requested review from laanwj on Apr 9, 2024

achow101 requested review from achow101 on Apr 9, 2024

achow101 requested review from sr-gi on Apr 9, 2024

achow101 commented at 9:21 pm on April 15, 2024: member

ACK d0c8109dd0f2c8ca3ee742f0a0f11911553e0a62

DrahtBot requested review from stratospher on Apr 15, 2024

DrahtBot requested review from jonatack on Apr 15, 2024

fanquake requested review from pinheadmz on Apr 16, 2024

achow101 removed review request from achow101 on Apr 16, 2024

pinheadmz approved

pinheadmz commented at 8:01 pm on April 19, 2024: member

ACK d0c8109dd0f2c8ca3ee742f0a0f11911553e0a62

Reviewed each commit. Ran all unit and functional tests on arm/macos and then AGAIN on arm/linux (docker) when I noticed the relevant test requires it :-P

Confirmed the tests fail on master without the patch.

Also confirmed the “crash on port collision” behavior with my own test, @vasild take it or leave it: https://gist.github.com/pinheadmz/0bea4bf8d8bf9af85f8ae326286d3082

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK d0c8109dd0f2c8ca3ee742f0a0f11911553e0a62
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmYizSsACgkQ5+KYS2KJ
 7yTrBYhAAuZaC4Hq1+3ezG1rcwv5MKKrhVNu+hLZ4NVKd7w9vZkVtinL9mMwZ0xsC
 8NmB6A1uC7kfGY+lVPzh4ifv1BK2h1VcBpN5+Ly90N9pyqx4mmsn+oj4mANThxiVF
 9FURLVQpWLtugPY21xBKDZoyf9AIITpu45UxQWFVIxYWgvBkAfWtE/Wu0OpraWCxs
10O8jD0mFODTYrk9pYZ8Igv8/6cIHx/a5oyB1keAuNzqec7kWHjOaY3+c3YzPqyh33
113lCDvAm/PEvr+S5CIqMnnVQnduacClCvd8xeHBdjO2xSA///drWPoPgFywOy8YB3
12Tly2rE8zzZRNaKWkosi6YNN+9ckEs1i9UqDQN115lxh6JfK3FSDPsQocqmWdNaMH
13Jy19EXohRMaLDQwMgkQWdOQYW8AKzr/sLOMcMqfapafL4627DND52nk3ABmoxNbP
1417HD2RnEyxIIxra7/NHTw3eX+eUEQcXtvedTaaa5gwVOAbgxkqWcqhWXpdmaUMxh
15HQNcABnVmg8C73hKCOi/TtuQJFwjIylE7TbrinvNePoih+dWYc9kWuhoHoapn9QG
16BphV/+Ywhi/oZa3qSBMdtzu/al5Uw6bAkRLW0O4stPFyS27pHrYFtbqS5+Z3FKiv
17nVo8FbiJoahUSIyKrOQSCv5dI5+X28eIjs03MTvxXkDSlBOsQZw=
18=REWG
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on keybase

DrahtBot added the label CI failed on Apr 22, 2024

DrahtBot removed the label CI failed on Apr 23, 2024

in test/functional/feature_bind_extra.py:37 in 5f5ec7daf3 outdated

37         # Due to OS-specific network stats queries, we only run on Linux.
38         self.skip_if_platform_not_linux()
39 
40     def setup_network(self):
41+        any_ipv4 = addr_to_hex('0.0.0.0')
42         loopback_ipv4 = addr_to_hex("127.0.0.1")

cbergqvist commented at 1:24 pm on May 20, 2024:

nit: Introducing style mismatch on adjacent lines - mixing single and double quotes in the two addr_to_hex() calls.

vasild commented at 2:42 pm on June 1, 2024:

Dropped this line altogether.

in test/functional/feature_bind_extra.py:78 in 5f5ec7daf3 outdated

73+        )
74+
75+        # Node3, no -bind=...=onion, thus no extra port for Tor target.
76+        self.expected.append(
77+            [
78+                ['-bind=127.0.0.1:{}'.format(port)],

cbergqvist commented at 7:02 pm on May 20, 2024:

nit: Using ''.format() instead of f'' as recommended in https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines

Use f’{x}’ for string formatting in preference to ‘{}’.format(x) or ‘%s’ % x.

Prior entries use the recommended style apart from "-quotes.

vasild commented at 2:43 pm on June 1, 2024:

Changed to use F-strings.

in test/functional/feature_bind_extra.py:77 in d0c8109dd0 outdated

88 
89     def run_test(self):
90         for i in range(len(self.expected)):
91-            self.log.info(f"Starting node {i} with {self.expected[i][0]}")
92-            self.start_node(i)
93+            self.log.info(f"Checking listening ports of node {i} with {self.expected[i][0]}")

cbergqvist commented at 9:15 pm on May 20, 2024:

The self.expected variable name really confused me since it includes command line inputs used to steer the test, but it was named prior to this PR. Maybe improve the situation with some slightly clearer names in the loop?

0        for i, (args, expected_services) in enumerate(self.expected):
1            self.log.info(f"Checking listening ports of node {i} with {args}")

vasild commented at 2:58 pm on June 1, 2024:

Done, thanks!

cbergqvist approved

cbergqvist commented at 10:25 pm on May 20, 2024: contributor

ACK d0c8109dd0f2c8ca3ee742f0a0f11911553e0a62

Was worried that the UI might fail to show the MessageBoxes about bind errors in CConnman::Bind() now that CConnman::InitBinds() fails on bind errors instead of continuing. But MessageBoxes are modal due to MSG_ERROR = (ICON_ERROR | BTN_OK | MODAL) and I manually verified it works fine in bitcoin-qt by attempting to bind to a port occupied by another process on my machine.

Ran functional --extended tests with one single expected test timeout. make check passed.

achow101 commented at 3:04 pm on May 23, 2024: member

There seems to be an intermittent failure with feature_discover.py. 2 of 4 functional test suite runs that I just did failed there with:

 0175/307 - feature_discover.py failed, Duration: 1 s
 1
 2stdout:
 32024-05-23T15:01:57.868000Z TestFramework (INFO): PRNG seed is: 6285884694359884871
 42024-05-23T15:01:57.869000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20240523_110117/feature_discover_78
 52024-05-23T15:01:58.127000Z TestFramework (INFO): Restart node with ['-listen', '-discover']
 62024-05-23T15:01:58.428000Z TestFramework (ERROR): Unexpected exception caught during testing
 7Traceback (most recent call last):
 8  File "/home/ava/bitcoin/bitcoin/master/test/functional/test_framework/test_framework.py", line 132, in main
 9    self.run_test()
10  File "/home/ava/bitcoin/bitcoin/master/gcc_build/enable-debug/../../test/functional/feature_discover.py", line 68, in run_test
11    self.test_local_addresses(test_case, expect_empty=False)
12  File "/home/ava/bitcoin/bitcoin/master/gcc_build/enable-debug/../../test/functional/feature_discover.py", line 45, in test_local_addresses
13    self.restart_node(0, test_case)
14  File "/home/ava/bitcoin/bitcoin/master/test/functional/test_framework/test_framework.py", line 598, in restart_node
15    self.start_node(i, extra_args)
16  File "/home/ava/bitcoin/bitcoin/master/test/functional/test_framework/test_framework.py", line 550, in start_node
17    node.wait_for_rpc_connection()
18  File "/home/ava/bitcoin/bitcoin/master/test/functional/test_framework/test_node.py", line 255, in wait_for_rpc_connection
19    raise FailedToStartError(self._node_msg(
20test_framework.test_node.FailedToStartError: [node 0] bitcoind exited with status 1 during initialization. Error: Unable to bind to 127.0.0.1:18445 on this computer. Bitcoin Core is probably already running.
21Error: Failed to listen on any port. Use -listen=0 if you want this.
22************************
23
242024-05-23T15:01:58.479000Z TestFramework (INFO): Stopping nodes
25[node 0] Cleaning up leftover process
26
27
28stderr:
29Traceback (most recent call last):
30  File "/home/ava/bitcoin/bitcoin/master/gcc_build/enable-debug/../../test/functional/feature_discover.py", line 75, in <module>
31    DiscoverTest().main()
32  File "/home/ava/bitcoin/bitcoin/master/test/functional/test_framework/test_framework.py", line 155, in main
33    exit_code = self.shutdown()
34                ^^^^^^^^^^^^^^^
35  File "/home/ava/bitcoin/bitcoin/master/test/functional/test_framework/test_framework.py", line 318, in shutdown
36    self.stop_nodes()
37  File "/home/ava/bitcoin/bitcoin/master/test/functional/test_framework/test_framework.py", line 583, in stop_nodes
38    node.stop_node(wait=wait, wait_until_stopped=False)
39  File "/home/ava/bitcoin/bitcoin/master/test/functional/test_framework/test_node.py", line 375, in stop_node
40    self.stop(wait=wait)
41    ^^^^^^^^^
42  File "/home/ava/bitcoin/bitcoin/master/test/functional/test_framework/test_node.py", line 205, in __getattr__
43    assert self.rpc_connected and self.rpc is not None, self._node_msg("Error: no RPC connection")
44           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
45AssertionError: [node 0] Error: no RPC connection

vasild force-pushed on Jun 1, 2024

vasild commented at 2:41 pm on June 1, 2024: contributor

d0c8109dd0...2e5e69a2d6: rebase, address suggestions and most importantly - @achow101, wow! thank you for catching this! The problem was that when a functional test requests bind_to_localhost_only=False then the framework would not use bind=127.0.0.1 and it will start bitcoind without any bind=... and thus would still try to bind on 127.0.0.1:18445 causing collisions. The solution is to use bind=0.0.0.0 in that case.

vasild force-pushed on Jun 1, 2024

vasild commented at 2:58 pm on June 1, 2024: contributor

2e5e69a2d6...8c3087150c: forgot to address one suggestion, sorry for the noise

in test/functional/feature_bind_extra.py:88 in 8c3087150c outdated

85@@ -79,8 +86,6 @@ def run_test(self):
86             # Remove RPC ports. They are not relevant for this test.
87             binds = set(filter(lambda e: e[1] != rpc_port(i), binds))
88             assert_equal(binds, set(self.expected[i][1]))

cbergqvist commented at 8:49 pm on June 8, 2024:

Thanks for taking my suggestion in #22729 (review)! To increase readability one can also use the named loop-variable here, sorry I didn’t mention it before:

0            assert_equal(binds, set(expected_services))

vasild commented at 5:06 am on June 11, 2024:

Will do if I retouch.

cbergqvist commented at 10:29 pm on July 1, 2024:

Did you skip this in the ffe9b274984a351716348a6c99df19c391bfdc8e force-push because you weren’t modifying this specific file or did you forget?

vasild commented at 12:19 pm on July 2, 2024:

I forgot. Applied it now.

in test/functional/feature_bind_extra.py:70 in 8c3087150c outdated

65+            [
66+                [f"-bind=127.0.0.1:{port}"],
67+                [(loopback_ipv4, port)]
68+            ],
69+        )
70+        port += 1

cbergqvist commented at 9:29 pm on June 8, 2024:

(Would be nice to add something like

0        # Node3, -bind=... and -listenonion.
1        self.expected.append(
2            [
3                [f"-bind=127.0.0.1:{port}", "-listenonion"],
4                [(loopback_ipv4, port)]
5            ],
6        )
7        port += 1

But would then also be nice to verify whether a port was being used for onion traffic or not, to distinguish Node3 from Node2, which I don’t know how to do).

vasild commented at 5:08 am on June 11, 2024:

I think that would be too complicated. Feel free to consider a followup, but I would restrain bloating this PR further.

in test/functional/test_framework/test_node.py:224 in 8c3087150c outdated

219+        # bitcoin.conf. Don't bind on 127.0.0.1:18445 because it is not needed and
220+        # to avoid collisions.
221+        will_listen = all(e != "-nolisten" and e != "-listen=0" for e in extra_args)
222+        has_explicit_bind = self.has_explicit_bind or any(e.startswith("-bind=") for e in extra_args)
223+        if will_listen and not has_explicit_bind:
224+            extra_args.append("-bind=0.0.0.0")

cbergqvist commented at 9:38 pm on June 8, 2024:

Feels confusing to have the test framework diverge in behavior this much from default bitcoind behavior.

Would be less surprising if the test framework avoided Tor port collisions through spreading out the ports among the nodes as is done for (P2P)Port and RPCPort. A suggested alternative to your current workaround is in 6b785873557696cc611d58fdcac5a3d54622082c.

vasild commented at 5:18 am on June 11, 2024:

All these

https://github.com/bitcoin/bitcoin/blob/b1ba1b178f501daa1afdd91f9efec34e5ec1e294/test/functional/test_framework/util.py#L393-L423

can be considered as divergence from the default bitcoind behavior.

The suggested change adds a new bitcoind config option just for the test framework, I do not like that. I guess it may be possible to avoid Tor collisions by spreading the ports without using a new config option, by using -bind=127.0.0.1:torport=onion, but then, I do not think that will be simpler than the current approach and and also, as stated in the comment above “Don’t bind on 127.0.0.1:18445 because it is not needed and to avoid collisions.” avoiding collisions is part of the reason why not to bind. The other reason is that “it is not needed”. I would rather avoid the bind if is not needed, instead of trying to accommodate it.

What do other reviewers think? Leave it like now, or try to bind on the tor port in a non-conflicting way even if not needed? @achow101, @pinheadmz, @laanwj, @stratospher, @jonatack?

pinheadmz commented at 3:54 pm on June 26, 2024:

I’m actually in favor of adding tor_port() to util.py and binding every bitcoind in the functional tests to a unique port by default. It is consistent with how we set the other two ports and feels like an oversight not to assign tor port the same way, as long as it doesn’t interfere with anything else.

I know what you mean that its not needed but it also default behavior to bind a tor port so I feel like we should do that in the functional tests to prevent some future regression.

vasild commented at 10:57 am on July 1, 2024:

I’m actually in favor of adding tor_port() to util.py and binding every bitcoind in the functional tests to a unique port by default.

Done, see if you like it. No new bitcoind config options introduced.

cbergqvist changes_requested

vasild force-pushed on Jul 1, 2024

vasild commented at 10:56 am on July 1, 2024: contributor

8c3087150c...ffe9b27498: spread the tor ports in the test framework, see #22729 (review)

pinheadmz approved

pinheadmz commented at 6:17 pm on July 1, 2024: member

ACK ffe9b274984a351716348a6c99df19c391bfdc8e

Re-reviewed changes since last ACK, including addition of tor_port() in tests which I like.

Also ran my extra test again from https://gist.github.com/pinheadmz/0bea4bf8d8bf9af85f8ae326286d3082

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK ffe9b274984a351716348a6c99df19c391bfdc8e
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmaC8jkACgkQ5+KYS2KJ
 7yTq+AQ//SckV94PZnCpRlkcKpw5FVG2dv+gdS39mtSmheodKmwZQKvUeagoeliFf
 8/GfPWqwnh+zP7eEIAy2HpDLkDwpUNfGoUn39gEEXxHTJ635arYk+lgX6I95IihqK
 9THvOjQ7pKH/KB04misMQMBuqEs8JaCVbZp0u2pDiryzPuL5eRl8dU94pVXvFrCla
10Q4fPXRXXxN4RSoKUbnvcPU9nVv8tHV4+nGY4D3tJROhBUdzjbDpJ7M6W8Q67WR9Z
11lELsg4EOh8P61hqPIJuljsNHylSh93viK01QaHu57TW7aC4SCo6OczHZQ3OFZgdx
12JuXofhZIMCf56+Ret6tAkDxhHMEcmQhoxTgLxl+rkIQdyOf9Fum6vozASMBvNp/p
13LPqIYheMzCV73xzmtxoy6CR3nTz90jzFGwz3Qf9HyDHtRYW6KlQI/mFDCdx/sHrw
14NEZ+5JNO2etTHWJdaHwdFwbO2E2ONczWII/9xCZTCusiuGOVq0iazyFjU43yFECG
15aQxBR4pGSF3mNQUZ4PPioqL6Or+N+pxTc4tZYXaMTx8pP4xxHY9tK73/rit5AN/W
16HBdCWNtyxoc+Uv/zGNtteQ7dnul3A8i+Lc6M7xNfX6BhHYd18uaMkGAViJvWwpUY
178S3aykSqsgKIhfOzzSemKAavdQ4kfMBiACqoiSg2scsRGHZPWLA=
18=XCgS
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on keybase

DrahtBot requested review from cbergqvist on Jul 1, 2024

DrahtBot requested review from achow101 on Jul 1, 2024

cbergqvist changes_requested

cbergqvist commented at 10:25 pm on July 1, 2024: contributor

Approach NACK ffe9b274984a351716348a6c99df19c391bfdc8e

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. My proposed solution in 6b785873557696cc611d58fdcac5a3d54622082c did not have that behavior. Maybe you could add a custom flag on the Python test framework level that can be opted in to by tests that require it?

vasild commented at 12:00 pm on July 2, 2024: contributor

@cbergqvist,

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.

The previous incarnation of this PR (8c3087150ce4afe6d6831b9c0d4da6e6e1155d55) 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.

net: don't extra bind for Tor if binds are restricted

If only `-bind=addr:port` is given (without `-bind=...=onion`) then we
would bind to `addr:port` _and_ to `127.0.0.1:8334` in addition which
may be unexpected, assuming the semantic of `-bind=addr:port` is
"bind _only_ to `addr:port`".

Change the above to not do the additional bind: if only
`-bind=addr:port` is given (without `-bind=...=onion`) then bind to
`addr:port` (only). If we are creating a Tor hidden service then use
`addr:port` as target (same behavior as before
https://github.com/bitcoin/bitcoin/pull/19991).

This allows disabling binding on the onion port.

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

9a7e5f4d68

net: report an error if unable to bind on the Tor listening addr:port af552534ab

net: require P2P binds to succeed

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 https://github.com/bitcoin/bitcoin/issues/22727

bca346a970

vasild force-pushed on Jul 2, 2024

vasild commented at 12:20 pm on July 2, 2024: contributor

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

cbergqvist approved

cbergqvist commented at 11:17 pm on July 3, 2024: contributor

ACK bca346a97056748f1e3fb899f336d56d9fd45a64

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.

DrahtBot requested review from pinheadmz on Jul 3, 2024

pinheadmz approved

pinheadmz commented at 7:27 pm on July 12, 2024: member

ACK bca346a97056748f1e3fb899f336d56d9fd45a64

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

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK bca346a97056748f1e3fb899f336d56d9fd45a64
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmaRhAIACgkQ5+KYS2KJ
 7yTr7nA//Sq7hxrI2I5/n/paZ+F6aMbZSxIkv7ynizzr3WfzBimoFmh+RcXkpyg5I
 8OK32DfJbKod4U1EgkTXY6RkjF8uYftCqLugF9qwDKC4NhkWQxEjepxX75Vrs2qtr
 9mZkg49HMpxb6Kj3xm/Mulj0cEgKcIyxlrNPN/AeQq/M+iBEymkBTNBMVowipAh9Z
10b/B74kPiA5/D2Lk8Xp4ffd2JFpVyScDXa9CgJv7kg6mS/Jr3V/kQJ6EaIBJlcCPZ
11iag0HdTXkrXOjOKDhLPgMn4sENOKBPQtpTvlZTmkyVFEAdiDo2aFaZGUwQrbRILk
12GDeuQqLKWHk7ypgW3oevUzdEtfKTO2vf53m2VuzM4opQTgyx9F5OzQbYPHDc+oKQ
13K3Lsort48CIReEkXnnVdtkifmJLUnDyV3qpBB2r7TMoT144089kjPbbkPafjVTzL
1405l4QeAnNLS5ERrm9wTBKiSzxassRNSMOS2gH4RgWmbixchSrq0PCTJc+TBYIrRi
15uLUaqZDJZTsIHw+c789nmfG08PT6r9i5LDWBEGMF176lffkuKaXD1eB7nn0wHzWQ
16nS8wHmGLND4NXNJiM5j1jPnuBuHwB3ZYVab6eqzaKO3ovTfh1xzHPr5K8OksxRHP
17jwwUhWJ9t6VEUZM6g/NnfA8wmGYb9F//45o3YiPol6+kNW4QKrA=
18=m0K0
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on keybase

achow101 commented at 8:26 pm on July 16, 2024: member

ACK bca346a97056748f1e3fb899f336d56d9fd45a64

achow101 merged this on Jul 16, 2024

achow101 closed this on Jul 16, 2024

vasild deleted the branch on Jul 17, 2024

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

Code Coverage

Reviews

Conflicts