From 9a7e5f4d68dadc64a675f32d1e91199d6b1aa095 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Mon, 2 Aug 2021 18:54:06 +0200 Subject: [PATCH 1/3] 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 --- src/init.cpp | 2 ++ test/functional/feature_bind_extra.py | 23 ++++++++++++++--------- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index c9405153b47be..cf908c505825b 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1890,6 +1890,8 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) CService onion_service_target; if (!connOptions.onion_binds.empty()) { onion_service_target = connOptions.onion_binds.front(); + } else if (!connOptions.vBinds.empty()) { + onion_service_target = connOptions.vBinds.front(); } else { onion_service_target = DefaultOnionServiceTarget(); connOptions.onion_binds.push_back(onion_service_target); diff --git a/test/functional/feature_bind_extra.py b/test/functional/feature_bind_extra.py index 5cd031f852bec..ed2328b76f7f5 100755 --- a/test/functional/feature_bind_extra.py +++ b/test/functional/feature_bind_extra.py @@ -27,7 +27,7 @@ def set_test_params(self): # Avoid any -bind= on the command line. Force the framework to avoid # adding -bind=127.0.0.1. self.bind_to_localhost_only = False - self.num_nodes = 2 + self.num_nodes = 3 def skip_test_if_missing_module(self): # Due to OS-specific network stats queries, we only run on Linux. @@ -60,14 +60,21 @@ def setup_network(self): ) port += 2 + # Node2, no -bind=...=onion, thus no extra port for Tor target. + self.expected.append( + [ + [f"-bind=127.0.0.1:{port}"], + [(loopback_ipv4, port)] + ], + ) + port += 1 + self.extra_args = list(map(lambda e: e[0], self.expected)) - self.add_nodes(self.num_nodes, self.extra_args) - # Don't start the nodes, as some of them would collide trying to bind on the same port. + self.setup_nodes() def run_test(self): - for i in range(len(self.expected)): - self.log.info(f"Starting node {i} with {self.expected[i][0]}") - self.start_node(i) + for i, (args, expected_services) in enumerate(self.expected): + self.log.info(f"Checking listening ports of node {i} with {args}") pid = self.nodes[i].process.pid binds = set(get_bind_addrs(pid)) # Remove IPv6 addresses because on some CI environments "::1" is not configured @@ -78,9 +85,7 @@ def run_test(self): binds = set(filter(lambda e: len(e[0]) != ipv6_addr_len_bytes, binds)) # Remove RPC ports. They are not relevant for this test. binds = set(filter(lambda e: e[1] != rpc_port(i), binds)) - assert_equal(binds, set(self.expected[i][1])) - self.stop_node(i) - self.log.info(f"Stopped node {i}") + assert_equal(binds, set(expected_services)) if __name__ == '__main__': BindExtraTest().main() From af552534ab83c572d3bc3f93ccaee5c1961ccab5 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Mon, 2 Aug 2021 19:05:35 +0200 Subject: [PATCH 2/3] net: report an error if unable to bind on the Tor listening addr:port --- src/net.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/net.cpp b/src/net.cpp index 990c58ee3d88f..34972d814f15d 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3206,7 +3206,7 @@ bool CConnman::InitBinds(const Options& options) fBound |= Bind(addrBind.m_service, BF_REPORT_ERROR, addrBind.m_flags); } for (const auto& addr_bind : options.onion_binds) { - fBound |= Bind(addr_bind, BF_DONT_ADVERTISE, NetPermissionFlags::None); + fBound |= Bind(addr_bind, BF_REPORT_ERROR | BF_DONT_ADVERTISE, NetPermissionFlags::None); } if (options.bind_on_any) { struct in_addr inaddr_any; From bca346a97056748f1e3fb899f336d56d9fd45a64 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Fri, 13 Aug 2021 10:33:24 +0200 Subject: [PATCH 3/3] 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 --- src/net.cpp | 28 +++++++++++++++------ test/functional/test-shell.md | 2 +- test/functional/test_framework/test_node.py | 15 +++++++++++ test/functional/test_framework/util.py | 10 +++++--- 4 files changed, 43 insertions(+), 12 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 34972d814f15d..dbb338d54c33a 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3198,24 +3198,36 @@ bool CConnman::Bind(const CService& addr_, unsigned int flags, NetPermissionFlag bool CConnman::InitBinds(const Options& options) { - bool fBound = false; for (const auto& addrBind : options.vBinds) { - fBound |= Bind(addrBind, BF_REPORT_ERROR, NetPermissionFlags::None); + if (!Bind(addrBind, BF_REPORT_ERROR, NetPermissionFlags::None)) { + return false; + } } for (const auto& addrBind : options.vWhiteBinds) { - fBound |= Bind(addrBind.m_service, BF_REPORT_ERROR, addrBind.m_flags); + if (!Bind(addrBind.m_service, BF_REPORT_ERROR, addrBind.m_flags)) { + return false; + } } for (const auto& addr_bind : options.onion_binds) { - fBound |= Bind(addr_bind, BF_REPORT_ERROR | BF_DONT_ADVERTISE, NetPermissionFlags::None); + if (!Bind(addr_bind, BF_REPORT_ERROR | BF_DONT_ADVERTISE, NetPermissionFlags::None)) { + return false; + } } if (options.bind_on_any) { + // Don't consider errors to bind on IPv6 "::" fatal because the host OS + // may not have IPv6 support and the user did not explicitly ask us to + // bind on that. + const CService ipv6_any{in6_addr(IN6ADDR_ANY_INIT), GetListenPort()}; // :: + Bind(ipv6_any, BF_NONE, NetPermissionFlags::None); + struct in_addr inaddr_any; inaddr_any.s_addr = htonl(INADDR_ANY); - struct in6_addr inaddr6_any = IN6ADDR_ANY_INIT; - fBound |= Bind(CService(inaddr6_any, GetListenPort()), BF_NONE, NetPermissionFlags::None); - fBound |= Bind(CService(inaddr_any, GetListenPort()), !fBound ? BF_REPORT_ERROR : BF_NONE, NetPermissionFlags::None); + const CService ipv4_any{inaddr_any, GetListenPort()}; // 0.0.0.0 + if (!Bind(ipv4_any, BF_REPORT_ERROR, NetPermissionFlags::None)) { + return false; + } } - return fBound; + return true; } bool CConnman::Start(CScheduler& scheduler, const Options& connOptions) diff --git a/test/functional/test-shell.md b/test/functional/test-shell.md index 4cd62c4ef340e..d79c4a0ab691e 100644 --- a/test/functional/test-shell.md +++ b/test/functional/test-shell.md @@ -169,7 +169,7 @@ can be called after the TestShell is shut down. | Test parameter key | Default Value | Description | |---|---|---| -| `bind_to_localhost_only` | `True` | Binds bitcoind RPC services to `127.0.0.1` if set to `True`.| +| `bind_to_localhost_only` | `True` | Binds bitcoind P2P services to `127.0.0.1` if set to `True`.| | `cachedir` | `"/path/to/bitcoin/test/cache"` | Sets the bitcoind datadir directory. | | `chain` | `"regtest"` | Sets the chain-type for the underlying test bitcoind processes. | | `configfile` | `"/path/to/bitcoin/test/config.ini"` | Sets the location of the test framework config file. | diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 0ac0af27d5bf5..216de9c309079 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -39,6 +39,7 @@ rpc_url, wait_until_helper_internal, p2p_port, + tor_port, ) BITCOIND_PROC_WAIT_TIMEOUT = 60 @@ -88,8 +89,11 @@ def __init__(self, i, datadir_path, *, chain, rpchost, timewait, timeout_factor, self.coverage_dir = coverage_dir self.cwd = cwd self.descriptors = descriptors + self.has_explicit_bind = False if extra_conf is not None: append_config(self.datadir_path, extra_conf) + # Remember if there is bind=... in the config file. + self.has_explicit_bind = any(e.startswith("bind=") for e in extra_conf) # Most callers will just need to add extra args to the standard list below. # For those callers that need more flexibility, they can just set the args property directly. # Note that common args are set in the config file (see initialize_datadir) @@ -210,6 +214,17 @@ def start(self, extra_args=None, *, cwd=None, stdout=None, stderr=None, env=None if extra_args is None: extra_args = self.extra_args + # If listening and no -bind is given, then bitcoind would bind P2P ports on + # 0.0.0.0:P and 127.0.0.1:18445 (for incoming Tor connections), where P is + # a unique port chosen by the test framework and configured as port=P in + # bitcoin.conf. To avoid collisions on 127.0.0.1:18445, change it to + # 127.0.0.1:tor_port(). + will_listen = all(e != "-nolisten" and e != "-listen=0" for e in extra_args) + has_explicit_bind = self.has_explicit_bind or any(e.startswith("-bind=") for e in extra_args) + if will_listen and not has_explicit_bind: + extra_args.append(f"-bind=0.0.0.0:{p2p_port(self.index)}") + extra_args.append(f"-bind=127.0.0.1:{tor_port(self.index)}=onion") + self.use_v2transport = "-v2transport=1" in extra_args or (self.default_to_v2 and "-v2transport=0" not in extra_args) # Add a new stdout and stderr file each time bitcoind is started diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py index c5b69a39549ac..7be1a35d4d2eb 100644 --- a/test/functional/test_framework/util.py +++ b/test/functional/test_framework/util.py @@ -309,9 +309,9 @@ def sha256sum_file(filename): # The maximum number of nodes a single test can spawn MAX_NODES = 12 -# Don't assign rpc or p2p ports lower than this +# Don't assign p2p, rpc or tor ports lower than this PORT_MIN = int(os.getenv('TEST_RUNNER_PORT_MIN', default=11000)) -# The number of ports to "reserve" for p2p and rpc, each +# The number of ports to "reserve" for p2p, rpc and tor, each PORT_RANGE = 5000 @@ -351,7 +351,11 @@ def p2p_port(n): def rpc_port(n): - return PORT_MIN + PORT_RANGE + n + (MAX_NODES * PortSeed.n) % (PORT_RANGE - 1 - MAX_NODES) + return p2p_port(n) + PORT_RANGE + + +def tor_port(n): + return p2p_port(n) + PORT_RANGE * 2 def rpc_url(datadir, i, chain, rpchost):