From a6aa3735be49eb34b46f6eed6ebd52361adf8256 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Wed, 12 Jun 2024 16:22:21 +0000 Subject: [PATCH] merge bitcoin#20196: fix GetListenPort() to derive the proper port continuation of 24205d94fe51093067cf9dd64c29d78147e4619c from dash#5982 includes: - 0cfc0cd32239d3c08d2121e028b297022450b320 - 7d64ea4a01920bb55bc6de0de6766712ec792a11 --- src/init.cpp | 15 +- src/net.cpp | 37 +++- src/net.h | 8 + src/net_processing.cpp | 4 + src/test/net_tests.cpp | 191 +++++++++++++++++- test/functional/feature_bind_port_discover.py | 78 +++++++ .../feature_bind_port_externalip.py | 75 +++++++ test/functional/test_runner.py | 2 + 8 files changed, 400 insertions(+), 10 deletions(-) create mode 100755 test/functional/feature_bind_port_discover.py create mode 100755 test/functional/feature_bind_port_externalip.py diff --git a/src/init.cpp b/src/init.cpp index 04442f2db2..3fc7eef7c8 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -2477,8 +2477,6 @@ bool AppInitMain(const CoreContext& context, NodeContext& node, interfaces::Bloc LogPrintf("::ChainActive().Height() = %d\n", chain_active_height); if (node.peerman) node.peerman->SetBestHeight(chain_active_height); - Discover(); - // Map ports with UPnP or NAT-PMP. StartMapPort(args.GetBoolArg("-upnp", DEFAULT_UPNP), args.GetBoolArg("-natpmp", DEFAULT_NATPMP)); @@ -2495,15 +2493,18 @@ bool AppInitMain(const CoreContext& context, NodeContext& node, interfaces::Bloc connOptions.nSendBufferMaxSize = 1000 * args.GetArg("-maxsendbuffer", DEFAULT_MAXSENDBUFFER); connOptions.nReceiveFloodSize = 1000 * args.GetArg("-maxreceivebuffer", DEFAULT_MAXRECEIVEBUFFER); connOptions.m_added_nodes = args.GetArgs("-addnode"); - connOptions.nMaxOutboundLimit = 1024 * 1024 * args.GetArg("-maxuploadtarget", DEFAULT_MAX_UPLOAD_TARGET); connOptions.m_peer_connect_timeout = peer_connect_timeout; + // Port to bind to if `-bind=addr` is provided without a `:port` suffix. + const uint16_t default_bind_port = + static_cast(args.GetArg("-port", Params().GetDefaultPort())); + for (const std::string& bind_arg : args.GetArgs("-bind")) { CService bind_addr; const size_t index = bind_arg.rfind('='); if (index == std::string::npos) { - if (Lookup(bind_arg, bind_addr, GetListenPort(), false)) { + if (Lookup(bind_arg, bind_addr, default_bind_port, /*fAllowLookup=*/false)) { connOptions.vBinds.push_back(bind_addr); continue; } @@ -2548,6 +2549,12 @@ bool AppInitMain(const CoreContext& context, NodeContext& node, interfaces::Bloc StartTorControl(onion_service_target); } + if (connOptions.bind_on_any) { + // Only add all IP addresses of the machine if we would be listening on + // any address - 0.0.0.0 (IPv4) and :: (IPv6). + Discover(); + } + for (const auto& net : args.GetArgs("-whitelist")) { NetWhitelistPermissions subnet; bilingual_str error; diff --git a/src/net.cpp b/src/net.cpp index 501fef5a84..8824b606f5 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -151,6 +151,31 @@ void CConnman::AddAddrFetch(const std::string& strDest) 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")) { + CService bind_addr; + constexpr uint16_t dummy_port = 0; + + if (Lookup(bind_arg, bind_addr, dummy_port, /*fAllowLookup=*/false)) { + if (bind_addr.GetPort() != dummy_port) { + return bind_addr.GetPort(); + } + } + } + + // Otherwise, if -whitebind= without NetPermissionFlags::NoBan is provided, use that + // (-whitebind= is required to have ":port"). + for (const std::string& whitebind_arg : gArgs.GetArgs("-whitebind")) { + NetWhitebindPermissions whitebind; + bilingual_str error; + if (NetWhitebindPermissions::TryParse(whitebind_arg, whitebind, error)) { + if (!NetPermissions::HasFlag(whitebind.m_flags, NetPermissionFlags::NoBan)) { + return whitebind.m_service.GetPort(); + } + } + } + + // Otherwise, if -port= is provided, use that. Otherwise use the default port. return static_cast(gArgs.GetArg("-port", Params().GetDefaultPort())); } @@ -246,7 +271,17 @@ std::optional GetLocalAddrForPeer(CNode *pnode) if (IsPeerAddrLocalGood(pnode) && (!addrLocal.IsRoutable() || rng.randbits((GetnScore(addrLocal) > LOCAL_MANUAL) ? 3 : 1) == 0)) { - addrLocal.SetIP(pnode->GetAddrLocal()); + if (pnode->IsInboundConn()) { + // For inbound connections, assume both the address and the port + // as seen from the peer. + addrLocal = CAddress{pnode->GetAddrLocal(), addrLocal.nServices}; + } else { + // For outbound connections, assume just the address as seen from + // the peer and leave the port in `addrLocal` as returned by + // `GetLocalAddress()` above. The peer has no way to observe our + // listening port when we have initiated the connection. + addrLocal.SetIP(pnode->GetAddrLocal()); + } } if (addrLocal.IsRoutable() || gArgs.GetBoolArg("-addrmantest", false)) { diff --git a/src/net.h b/src/net.h index 1adf20b296..9ec6199638 100644 --- a/src/net.h +++ b/src/net.h @@ -215,7 +215,15 @@ enum class ConnectionType { /** Convert ConnectionType enum to a string value */ std::string ConnectionTypeAsString(ConnectionType conn_type); + +/** + * Look up IP addresses from all interfaces on the machine and add them to the + * list of local addresses to self-advertise. + * The loopback interface is skipped and only the first address from each + * interface is used. + */ void Discover(); + uint16_t GetListenPort(); enum diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 77dbcfaeeb..a8c4f52de1 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3444,6 +3444,10 @@ void PeerManagerImpl::ProcessMessage( LogPrint(BCLog::NET, "ProcessMessages: advertising address %s\n", addr.ToString()); PushAddress(*peer, addr, insecure_rand); } else if (IsPeerAddrLocalGood(&pfrom)) { + // Override just the address with whatever the peer sees us as. + // Leave the port in addr as it was returned by GetLocalAddress() + // above, as this is an outbound connection and the peer cannot + // observe our listening port. addr.SetIP(addrMe); LogPrint(BCLog::NET, "ProcessMessages: advertising address %s\n", addr.ToString()); PushAddress(*peer, addr, insecure_rand); diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index 06f37a1468..050cb3e4b4 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -6,15 +6,21 @@ #include #include +#include #include +#include #include #include +#include #include #include #include +#include +#include #include #include #include +#include #include #include @@ -28,7 +34,7 @@ using namespace std::literals; -BOOST_FIXTURE_TEST_SUITE(net_tests, BasicTestingSetup) +BOOST_FIXTURE_TEST_SUITE(net_tests, RegTestingSetup) BOOST_AUTO_TEST_CASE(cnode_listen_port) { @@ -608,15 +614,15 @@ BOOST_AUTO_TEST_CASE(ipv4_peer_with_ipv6_addrMe_test) // set up local addresses; all that's necessary to reproduce the bug is // that a normal IPv4 address is among the entries, but if this address is // !IsRoutable the undefined behavior is easier to trigger deterministically + in_addr raw_addr; + raw_addr.s_addr = htonl(0x7f000001); + const CNetAddr mapLocalHost_entry = CNetAddr(raw_addr); { LOCK(g_maplocalhost_mutex); - in_addr ipv4AddrLocal; - ipv4AddrLocal.s_addr = 0x0100007f; - CNetAddr addr = CNetAddr(ipv4AddrLocal); LocalServiceInfo lsi; lsi.nScore = 23; lsi.nPort = 42; - mapLocalHost[addr] = lsi; + mapLocalHost[mapLocalHost_entry] = lsi; } // create a peer with an IPv4 address @@ -647,8 +653,79 @@ BOOST_AUTO_TEST_CASE(ipv4_peer_with_ipv6_addrMe_test) // suppress no-checks-run warning; if this test fails, it's by triggering a sanitizer BOOST_CHECK(1); + + // Cleanup, so that we don't confuse other tests. + { + LOCK(g_maplocalhost_mutex); + mapLocalHost.erase(mapLocalHost_entry); + } } +BOOST_AUTO_TEST_CASE(get_local_addr_for_peer_port) +{ + // Test that GetLocalAddrForPeer() properly selects the address to self-advertise: + // + // 1. GetLocalAddrForPeer() calls GetLocalAddress() which returns an address that is + // not routable. + // 2. GetLocalAddrForPeer() overrides the address with whatever the peer has told us + // he sees us as. + // 2.1. For inbound connections we must override both the address and the port. + // 2.2. For outbound connections we must override only the address. + + // Pretend that we bound to this port. + const uint16_t bind_port = 20001; + m_node.args->ForceSetArg("-bind", strprintf("3.4.5.6:%u", bind_port)); + + // Our address:port as seen from the peer, completely different from the above. + in_addr peer_us_addr; + peer_us_addr.s_addr = htonl(0x02030405); + const CAddress peer_us{CService{peer_us_addr, 20002}, NODE_NETWORK}; + + // Create a peer with a routable IPv4 address (outbound). + in_addr peer_out_in_addr; + peer_out_in_addr.s_addr = htonl(0x01020304); + CNode peer_out{/*id=*/0, + /*nLocalServicesIn=*/NODE_NETWORK, + /*sock=*/nullptr, + /*addrIn=*/CAddress{CService{peer_out_in_addr, 8333}, NODE_NETWORK}, + /*nKeyedNetGroupIn=*/0, + /*nLocalHostNonceIn=*/0, + /*addrBindIn=*/CAddress{}, + /*addrNameIn=*/std::string{}, + /*conn_type_in=*/ConnectionType::OUTBOUND_FULL_RELAY, + /*inbound_onion=*/false}; + peer_out.fSuccessfullyConnected = true; + peer_out.SetAddrLocal(peer_us); + + // Without the fix peer_us:8333 is chosen instead of the proper peer_us:bind_port. + auto chosen_local_addr = GetLocalAddrForPeer(&peer_out); + BOOST_REQUIRE(chosen_local_addr); + const CService expected{peer_us_addr, bind_port}; + BOOST_CHECK(*chosen_local_addr == expected); + + // Create a peer with a routable IPv4 address (inbound). + in_addr peer_in_in_addr; + peer_in_in_addr.s_addr = htonl(0x05060708); + CNode peer_in{/*id=*/0, + /*nLocalServicesIn=*/NODE_NETWORK, + /*sock=*/nullptr, + /*addrIn=*/CAddress{CService{peer_in_in_addr, 8333}, NODE_NETWORK}, + /*nKeyedNetGroupIn=*/0, + /*nLocalHostNonceIn=*/0, + /*addrBindIn=*/CAddress{}, + /*addrNameIn=*/std::string{}, + /*conn_type_in=*/ConnectionType::INBOUND, + /*inbound_onion=*/false}; + peer_in.fSuccessfullyConnected = true; + peer_in.SetAddrLocal(peer_us); + + // Without the fix peer_us:8333 is chosen instead of the proper peer_us:peer_us.GetPort(). + chosen_local_addr = GetLocalAddrForPeer(&peer_in); + BOOST_REQUIRE(chosen_local_addr); + BOOST_CHECK(*chosen_local_addr == peer_us); + + m_node.args->ForceSetArg("-bind", ""); +} BOOST_AUTO_TEST_CASE(LimitedAndReachable_Network) { @@ -734,4 +811,108 @@ BOOST_AUTO_TEST_CASE(LocalAddress_BasicLifecycle) BOOST_CHECK(!IsLocal(addr)); } +BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message) +{ + // Tests the following scenario: + // * -bind=3.4.5.6:20001 is specified + // * we make an outbound connection to a peer + // * the peer reports he sees us as 2.3.4.5:20002 in the version message + // (20002 is a random port assigned by our OS for the outgoing TCP connection, + // we cannot accept connections to it) + // * we should self-advertise to that peer as 2.3.4.5:20001 + + // Pretend that we bound to this port. + const uint16_t bind_port = 20001; + m_node.args->ForceSetArg("-bind", strprintf("3.4.5.6:%u", bind_port)); + m_node.args->ForceSetArg("-capturemessages", "1"); + + // Our address:port as seen from the peer - 2.3.4.5:20002 (different from the above). + in_addr peer_us_addr; + peer_us_addr.s_addr = htonl(0x02030405); + const CService peer_us{peer_us_addr, 20002}; + + // Create a peer with a routable IPv4 address. + in_addr peer_in_addr; + peer_in_addr.s_addr = htonl(0x01020304); + CNode peer{/*id=*/0, + /*nLocalServicesIn=*/NODE_NETWORK, + /*sock=*/nullptr, + /*addrIn=*/CAddress{CService{peer_in_addr, 8333}, NODE_NETWORK}, + /*nKeyedNetGroupIn=*/0, + /*nLocalHostNonceIn=*/0, + /*addrBindIn=*/CAddress{}, + /*addrNameIn=*/std::string{}, + /*conn_type_in=*/ConnectionType::OUTBOUND_FULL_RELAY, + /*inbound_onion=*/false}; + + const uint64_t services{NODE_NETWORK}; + const int64_t time{0}; + const CNetMsgMaker msg_maker{PROTOCOL_VERSION}; + + // Force CChainState::IsInitialBlockDownload() to return false. + // Otherwise PushAddress() isn't called by PeerManager::ProcessMessage(). + TestChainState& chainstate = + *static_cast(&m_node.chainman->ActiveChainstate()); + chainstate.JumpOutOfIbd(); + + m_node.peerman->InitializeNode(&peer); + + std::atomic interrupt_dummy{false}; + std::chrono::microseconds time_received_dummy{0}; + + const auto msg_version = + msg_maker.Make(NetMsgType::VERSION, PROTOCOL_VERSION, services, time, services, peer_us); + CDataStream msg_version_stream{msg_version.data, SER_NETWORK, PROTOCOL_VERSION}; + + m_node.peerman->ProcessMessage( + peer, NetMsgType::VERSION, msg_version_stream, time_received_dummy, interrupt_dummy); + + const auto msg_verack = msg_maker.Make(NetMsgType::VERACK); + CDataStream msg_verack_stream{msg_verack.data, SER_NETWORK, PROTOCOL_VERSION}; + + // Will set peer.fSuccessfullyConnected to true (necessary in SendMessages()). + m_node.peerman->ProcessMessage( + peer, NetMsgType::VERACK, msg_verack_stream, time_received_dummy, interrupt_dummy); + + // Ensure that peer_us_addr:bind_port is sent to the peer. + const CService expected{peer_us_addr, bind_port}; + bool sent{false}; + + const auto CaptureMessageOrig = CaptureMessage; + CaptureMessage = [&sent, &expected](const CAddress& addr, + const std::string& msg_type, + Span data, + bool is_incoming) -> void { + if (!is_incoming && msg_type == "addr") { + CDataStream s(data, SER_NETWORK, PROTOCOL_VERSION); + std::vector addresses; + + s >> addresses; + + for (const auto& addr : addresses) { + if (addr == expected) { + sent = true; + return; + } + } + } + }; + + { + LOCK(peer.cs_sendProcessing); + m_node.peerman->SendMessages(&peer); + } + + BOOST_CHECK(sent); + + CaptureMessage = CaptureMessageOrig; + chainstate.ResetIbd(); + m_node.args->ForceSetArg("-capturemessages", "0"); + m_node.args->ForceSetArg("-bind", ""); + // PeerManager::ProcessMessage() calls AddTimeData() which changes the internal state + // in timedata.cpp and later confuses the test "timedata_tests/addtimedata". Thus reset + // that state as it was before our test was run. + TestOnlyResetTimeData(); +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/test/functional/feature_bind_port_discover.py b/test/functional/feature_bind_port_discover.py new file mode 100755 index 0000000000..6e07f2f16c --- /dev/null +++ b/test/functional/feature_bind_port_discover.py @@ -0,0 +1,78 @@ +#!/usr/bin/env python3 +# Copyright (c) 2020-2021 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. +""" +Test that -discover does not add all interfaces' addresses if we listen on only some of them +""" + +from test_framework.test_framework import BitcoinTestFramework, SkipTest +from test_framework.util import assert_equal + +# We need to bind to a routable address for this test to exercise the relevant code +# and also must have another routable address on another interface which must not +# be named "lo" or "lo0". +# To set these routable addresses on the machine, use: +# Linux: +# ifconfig lo:0 1.1.1.1/32 up && ifconfig lo:1 2.2.2.2/32 up # to set up +# ifconfig lo:0 down && ifconfig lo:1 down # to remove it, after the test +# FreeBSD: +# ifconfig em0 1.1.1.1/32 alias && ifconfig wlan0 2.2.2.2/32 alias # to set up +# ifconfig em0 1.1.1.1 -alias && ifconfig wlan0 2.2.2.2 -alias # to remove it, after the test +ADDR1 = '1.1.1.1' +ADDR2 = '2.2.2.2' + +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}'], + ] + self.num_nodes = len(self.extra_args) + + def add_options(self, parser): + parser.add_argument( + "--ihave1111and2222", action='store_true', dest="ihave1111and2222", + help=f"Run the test, assuming {ADDR1} and {ADDR2} are configured on the machine", + default=False) + + def skip_test_if_missing_module(self): + if not self.options.ihave1111and2222: + raise SkipTest( + 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 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.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 + +if __name__ == '__main__': + BindPortDiscoverTest().main() diff --git a/test/functional/feature_bind_port_externalip.py b/test/functional/feature_bind_port_externalip.py new file mode 100755 index 0000000000..6a74ce5738 --- /dev/null +++ b/test/functional/feature_bind_port_externalip.py @@ -0,0 +1,75 @@ +#!/usr/bin/env python3 +# Copyright (c) 2020-2021 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. +""" +Test that the proper port is used for -externalip= +""" + +from test_framework.test_framework import BitcoinTestFramework, SkipTest +from test_framework.util import assert_equal, p2p_port + +# We need to bind to a routable address for this test to exercise the relevant code. +# To set a routable address on the machine use: +# Linux: +# ifconfig lo:0 1.1.1.1/32 up # to set up +# ifconfig lo:0 down # to remove it, after the test +# FreeBSD: +# ifconfig lo0 1.1.1.1/32 alias # to set up +# ifconfig lo0 1.1.1.1 -alias # to remove it, after the test +ADDR = '1.1.1.1' + +# array of tuples [arguments, expected port in localaddresses] +EXPECTED = [ + [['-externalip=2.2.2.2', '-port=30001'], 30001], + [['-externalip=2.2.2.2', '-port=30002', f'-bind={ADDR}'], 30002], + [['-externalip=2.2.2.2', f'-bind={ADDR}'], 'default_p2p_port'], + [['-externalip=2.2.2.2', '-port=30003', f'-bind={ADDR}:30004'], 30004], + [['-externalip=2.2.2.2', f'-bind={ADDR}:30005'], 30005], + [['-externalip=2.2.2.2:30006', '-port=30007'], 30006], + [['-externalip=2.2.2.2:30008', '-port=30009', f'-bind={ADDR}'], 30008], + [['-externalip=2.2.2.2:30010', f'-bind={ADDR}'], 30010], + [['-externalip=2.2.2.2:30011', '-port=30012', f'-bind={ADDR}:30013'], 30011], + [['-externalip=2.2.2.2:30014', f'-bind={ADDR}:30015'], 30014], + [['-externalip=2.2.2.2', '-port=30016', f'-bind={ADDR}:30017', + f'-whitebind={ADDR}:30018'], 30017], + [['-externalip=2.2.2.2', '-port=30019', + f'-whitebind={ADDR}:30020'], 30020], +] + +class BindPortExternalIPTest(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.num_nodes = len(EXPECTED) + self.extra_args = list(map(lambda e: e[0], EXPECTED)) + + def add_options(self, parser): + parser.add_argument( + "--ihave1111", action='store_true', dest="ihave1111", + help=f"Run the test, assuming {ADDR} is configured on the machine", + default=False) + + def skip_test_if_missing_module(self): + if not self.options.ihave1111: + raise SkipTest( + f"To run this test make sure that {ADDR} (a routable address) is assigned " + "to one of the interfaces on this machine and rerun with --ihave1111") + + def run_test(self): + self.log.info("Test the proper port is used for -externalip=") + for i in range(len(EXPECTED)): + expected_port = EXPECTED[i][1] + if expected_port == 'default_p2p_port': + expected_port = p2p_port(i) + found = False + for local in self.nodes[i].getnetworkinfo()['localaddresses']: + if local['address'] == '2.2.2.2': + assert_equal(local['port'], expected_port) + found = True + break + assert found + +if __name__ == '__main__': + BindPortExternalIPTest().main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 193dee7c18..bcf26c453e 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -277,6 +277,7 @@ BASE_SCRIPTS = [ 'p2p_connect_to_devnet.py', 'feature_sporks.py', 'rpc_getblockstats.py', + 'feature_bind_port_externalip.py', 'wallet_encryption.py --legacy-wallet', 'wallet_encryption.py --descriptors', 'wallet_upgradetohd.py --legacy-wallet', @@ -316,6 +317,7 @@ BASE_SCRIPTS = [ 'feature_loadblock.py', 'p2p_dos_header_tree.py', 'p2p_add_connections.py', + 'feature_bind_port_discover.py', 'p2p_blockfilters.py', 'p2p_message_capture.py', 'feature_addrman.py',