From c1de1127153259b88f9d26cae983f9036673be90 Mon Sep 17 00:00:00 2001 From: Andre Alves Date: Wed, 26 Feb 2025 04:01:12 -0300 Subject: [PATCH 1/2] net: execute Discover() when bind=0.0.0.0 or :: is set --- src/init.cpp | 11 ++++++----- src/net.cpp | 14 ++++++++++++-- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 3cfd301fbab..bf8393e45a2 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1902,6 +1902,10 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) const uint16_t default_bind_port_onion = default_bind_port + 1; + // If the user did not specify -bind= or -whitebind= then we bind + // on any address - 0.0.0.0 (IPv4) and :: (IPv6). + connOptions.bind_on_any = args.GetArgs("-bind").empty() && args.GetArgs("-whitebind").empty(); + const auto BadPortWarning = [](const char* prefix, uint16_t port) { return strprintf(_("%s request to listen on port %u. This port is considered \"bad\" and " "thus it is unlikely that any peer will connect to it. See " @@ -1916,6 +1920,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) if (index == std::string::npos) { bind_addr = Lookup(bind_arg, default_bind_port, /*fAllowLookup=*/false); if (bind_addr.has_value()) { + connOptions.bind_on_any |= bind_addr.value().IsBindAny(); connOptions.vBinds.push_back(bind_addr.value()); if (IsBadPort(bind_addr.value().GetPort())) { InitWarning(BadPortWarning("-bind", bind_addr.value().GetPort())); @@ -1928,6 +1933,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) const std::string truncated_bind_arg = bind_arg.substr(0, index); bind_addr = Lookup(truncated_bind_arg, default_bind_port_onion, false); if (bind_addr.has_value()) { + connOptions.bind_on_any |= bind_addr.value().IsBindAny(); connOptions.onion_binds.push_back(bind_addr.value()); continue; } @@ -1943,10 +1949,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) connOptions.vWhiteBinds.push_back(whitebind); } - // If the user did not specify -bind= or -whitebind= then we bind - // on any address - 0.0.0.0 (IPv4) and :: (IPv6). - connOptions.bind_on_any = args.GetArgs("-bind").empty() && args.GetArgs("-whitebind").empty(); - // Emit a warning if a bad port is given to -port= but only if -bind and -whitebind are not // given, because if they are, then -port= is ignored. if (connOptions.bind_on_any && args.IsArgSet("-port")) { @@ -1963,7 +1965,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) onion_service_target = connOptions.vBinds.front(); } else { onion_service_target = DefaultOnionServiceTarget(default_bind_port_onion); - connOptions.onion_binds.push_back(onion_service_target); } if (args.GetBoolArg("-listenonion", DEFAULT_LISTEN_ONION)) { diff --git a/src/net.cpp b/src/net.cpp index 735985a8414..55a3fa485a5 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -146,8 +146,10 @@ uint16_t GetListenPort() // If -bind= is provided with ":port" part, use that (first one if multiple are provided). for (const std::string& bind_arg : gArgs.GetArgs("-bind")) { constexpr uint16_t dummy_port = 0; + // maybe bind_arg has "=onion" + const std::string truncated_bind_arg = bind_arg.substr(0, bind_arg.rfind('=')); - const std::optional bind_addr{Lookup(bind_arg, dummy_port, /*fAllowLookup=*/false)}; + const std::optional bind_addr{Lookup(truncated_bind_arg, dummy_port, /*fAllowLookup=*/false)}; if (bind_addr.has_value() && bind_addr->GetPort() != dummy_port) return bind_addr->GetPort(); } @@ -3273,7 +3275,7 @@ bool CConnman::InitBinds(const Options& options) return false; } } - if (options.bind_on_any) { + if (options.bind_on_any && options.vBinds.empty() && options.onion_binds.empty()) { // 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. @@ -3286,6 +3288,14 @@ bool CConnman::InitBinds(const Options& options) if (!Bind(ipv4_any, BF_REPORT_ERROR, NetPermissionFlags::None)) { return false; } + + struct in_addr onion_service_target; + onion_service_target.s_addr = htonl(INADDR_LOOPBACK); + const uint16_t onion_port = GetListenPort() + 1; + const CService onion_addr = {onion_service_target, onion_port}; // 127.0.0.1 + if (!Bind(onion_addr, BF_REPORT_ERROR | BF_DONT_ADVERTISE, NetPermissionFlags::None)) { + return false; + } } return true; } From 1561575c2d984fd94c9679de3d1de207ef6c7c06 Mon Sep 17 00:00:00 2001 From: Andre Alves Date: Wed, 26 Feb 2025 04:01:27 -0300 Subject: [PATCH 2/2] Fix feature_bind_port_discover.py and add new test cases --- test/functional/feature_bind_port_discover.py | 78 ++++++++++++++----- 1 file changed, 59 insertions(+), 19 deletions(-) diff --git a/test/functional/feature_bind_port_discover.py b/test/functional/feature_bind_port_discover.py index 568c88bcbea..beccc3d7090 100755 --- a/test/functional/feature_bind_port_discover.py +++ b/test/functional/feature_bind_port_discover.py @@ -27,14 +27,36 @@ BIND_PORT = 31001 class BindPortDiscoverTest(BitcoinTestFramework): def set_test_params(self): # Avoid any -bind= on the command line. Force the framework to avoid adding -bind=127.0.0.1. - self.setup_clean_chain = True self.bind_to_localhost_only = False self.extra_args = [ ['-discover', f'-port={BIND_PORT}'], # bind on any ['-discover', f'-bind={ADDR1}:{BIND_PORT}'], + ['-discover', f'-bind=0.0.0.0:{BIND_PORT+1}'], + ['-discover', f'-bind=[::]:{BIND_PORT+2}'], + ['-discover', '-bind=::', f'-port={BIND_PORT+3}'], + ['-discover', f'-bind=0.0.0.0:{BIND_PORT+4}=onion'], ] self.num_nodes = len(self.extra_args) + def setup_network(self): + """ + BitcoinTestFramework.setup_network() without connecting node0 to node1, + we don't need that and avoiding it makes the test faster. + """ + self.setup_nodes() + + def setup_nodes(self): + """ + BitcoinTestFramework.setup_nodes() with overridden has_explicit_bind=True + for each node and without the chain setup. + """ + self.add_nodes(self.num_nodes, self.extra_args) + # TestNode.start() will add -bind= to extra_args if has_explicit_bind is + # False. We do not want any -bind= thus set has_explicit_bind to True. + for node in self.nodes: + node.has_explicit_bind = True + self.start_nodes() + def add_options(self, parser): parser.add_argument( "--ihave1111and2222", action='store_true', dest="ihave1111and2222", @@ -47,32 +69,50 @@ class BindPortDiscoverTest(BitcoinTestFramework): f"To run this test make sure that {ADDR1} and {ADDR2} (routable addresses) are " "assigned to the interfaces on this machine and rerun with --ihave1111and2222") + def verify_addrs(self, found_addrs, expected_addr1, expected_addr2, port): + print(found_addrs, port) + found_addr1 = False + found_addr2 = False + for local in found_addrs: + if local['address'] == ADDR1: + found_addr1 = True + assert_equal(local['port'], port) + if local['address'] == ADDR2: + found_addr2 = True + assert_equal(local['port'], port) + assert found_addr1 == expected_addr1 + assert found_addr2 == expected_addr2 + def run_test(self): self.log.info( "Test that if -bind= is not passed then all addresses are " "added to localaddresses") - found_addr1 = False - found_addr2 = False - for local in self.nodes[0].getnetworkinfo()['localaddresses']: - if local['address'] == ADDR1: - found_addr1 = True - assert_equal(local['port'], BIND_PORT) - if local['address'] == ADDR2: - found_addr2 = True - assert_equal(local['port'], BIND_PORT) - assert found_addr1 - assert found_addr2 + self.verify_addrs(self.nodes[0].getnetworkinfo()['localaddresses'], True, True, BIND_PORT) self.log.info( "Test that if -bind= is passed then only that address is " "added to localaddresses") - found_addr1 = False - for local in self.nodes[1].getnetworkinfo()['localaddresses']: - if local['address'] == ADDR1: - found_addr1 = True - assert_equal(local['port'], BIND_PORT) - assert local['address'] != ADDR2 - assert found_addr1 + self.verify_addrs(self.nodes[1].getnetworkinfo()['localaddresses'], True, False, BIND_PORT) + + self.log.info( + "Test that if -bind=0.0.0.0:port is passed then all addresses are " + "added to localaddresses") + self.verify_addrs(self.nodes[2].getnetworkinfo()['localaddresses'], True, True, BIND_PORT+1) + + self.log.info( + "Test that if -bind=[::]:port is passed then all addresses are " + "added to localaddresses") + self.verify_addrs(self.nodes[3].getnetworkinfo()['localaddresses'], True, True, BIND_PORT+2) + + self.log.info( + "Test that if -bind=:: is passed then all addresses are " + "added to localaddresses") + self.verify_addrs(self.nodes[4].getnetworkinfo()['localaddresses'], True, True, BIND_PORT+3) + + self.log.info( + "Test that if -bind=0.0.0.0:port=onion is passed then all addresses are " + "added to localaddresses") + self.verify_addrs(self.nodes[5].getnetworkinfo()['localaddresses'], True, True, BIND_PORT+4) if __name__ == '__main__': BindPortDiscoverTest(__file__).main()