Skip to content

Commit

Permalink
net: fix GetListenPort() to derive the proper port
Browse files Browse the repository at this point in the history
`GetListenPort()` uses a simple logic: "if `-port=P` is given, then we
must be listening on `P`, otherwise we must be listening on `8333`".
This is however not true if `-bind=` has been provided with `:port` part
or if `-whitebind=` has been provided. Thus, extend `GetListenPort()` to
return the port from `-bind=` or `-whitebind=`, if any.

Fixes bitcoin#20184 (cases 1. 2. 3. 5.)
  • Loading branch information
vasild committed Mar 17, 2021
1 parent 950c9f6 commit 39b4a0c
Show file tree
Hide file tree
Showing 6 changed files with 263 additions and 7 deletions.
6 changes: 5 additions & 1 deletion src/init.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1941,11 +1941,15 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
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<uint16_t>(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, false)) {
connOptions.vBinds.push_back(bind_addr);
continue;
}
Expand Down
38 changes: 37 additions & 1 deletion src/net.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,32 @@ 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")) {
constexpr uint16_t dummy_port = 0;
constexpr bool allow_lookup = false;
CService bind_addr;

if (Lookup(bind_arg, bind_addr, dummy_port, allow_lookup)) {
if (bind_addr.GetPort() != dummy_port) {
return bind_addr.GetPort();
}
}
}

// Otherwise, if -whitebind= without PF_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 (!(whitebind.m_flags & PF_NOBAN)) {
return whitebind.m_service.GetPort();
}
}
}

// Otherwise, if -port= is provided, use that. Otherwise use the default port.
return (uint16_t)(gArgs.GetArg("-port", Params().GetDefaultPort()));
}

Expand Down Expand Up @@ -207,7 +233,17 @@ std::optional<CAddress> 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))
{
Expand Down
4 changes: 4 additions & 0 deletions src/net_processing.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2465,6 +2465,10 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
LogPrint(BCLog::NET, "ProcessMessages: advertising address %s\n", addr.ToString());
pfrom.PushAddress(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 because this is outbound connection and the peer cannot
// observe our listening port.
addr.SetIP(addrMe);
LogPrint(BCLog::NET, "ProcessMessages: advertising address %s\n", addr.ToString());
pfrom.PushAddress(addr, insecure_rand);
Expand Down
146 changes: 141 additions & 5 deletions src/test/net_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,22 @@
#include <addrman.h>
#include <chainparams.h>
#include <clientversion.h>
#include <compat.h>
#include <cstdint>
#include <net.h>
#include <net_processing.h>
#include <netbase.h>
#include <netmessagemaker.h>
#include <serialize.h>
#include <span.h>
#include <streams.h>
#include <test/util/setup_common.h>
#include <test/util/validation.h>
#include <timedata.h>
#include <util/strencodings.h>
#include <util/string.h>
#include <util/system.h>
#include <validation.h>
#include <version.h>

#include <boost/test/unit_test.hpp>
Expand Down Expand Up @@ -86,7 +92,7 @@ static CDataStream AddrmanToStream(const CAddrManSerializationMock& _addrman)
return CDataStream(vchData, SER_DISK, CLIENT_VERSION);
}

BOOST_FIXTURE_TEST_SUITE(net_tests, BasicTestingSetup)
BOOST_FIXTURE_TEST_SUITE(net_tests, RegTestingSetup)

BOOST_AUTO_TEST_CASE(cnode_listen_port)
{
Expand Down Expand Up @@ -695,15 +701,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 addr_in_mapLocalHost_in_addr;
addr_in_mapLocalHost_in_addr.s_addr = htonl(0x7f000001);
const CNetAddr addr_in_mapLocalHost = CNetAddr(addr_in_mapLocalHost_in_addr);
{
LOCK(cs_mapLocalHost);
in_addr ipv4AddrLocal;
ipv4AddrLocal.s_addr = 0x0100007f;
CNetAddr addr = CNetAddr(ipv4AddrLocal);
LocalServiceInfo lsi;
lsi.nScore = 23;
lsi.nPort = 42;
mapLocalHost[addr] = lsi;
mapLocalHost[addr_in_mapLocalHost] = lsi;
}

// create a peer with an IPv4 address
Expand All @@ -725,8 +731,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(cs_mapLocalHost);
mapLocalHost.erase(addr_in_mapLocalHost);
}
}

BOOST_AUTO_TEST_CASE(get_local_addr_for_peer_port)
{
// Test that GetLocalAddrForPeer() properly selects the address for 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{0,
NODE_NETWORK,
INVALID_SOCKET,
CAddress{CService{peer_out_in_addr, 8333}, NODE_NETWORK},
0,
0,
CAddress{},
std::string{},
ConnectionType::OUTBOUND_FULL_RELAY,
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 CAddress expected{CService{peer_us_addr, bind_port}, NODE_NETWORK};
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{0,
NODE_NETWORK,
INVALID_SOCKET,
CAddress{CService{peer_in_in_addr, 8333}, NODE_NETWORK},
0,
0,
CAddress{},
std::string{},
ConnectionType::INBOUND,
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)
{
Expand Down Expand Up @@ -945,4 +1022,63 @@ BOOST_AUTO_TEST_CASE(node_eviction_test)
}
}

BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
{
// 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 - 2.3.4.5:20002 (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.
in_addr peer_in_addr;
peer_in_addr.s_addr = htonl(0x01020304);
CNode peer{0,
NODE_NETWORK,
INVALID_SOCKET,
CAddress{CService{peer_in_addr, 8333}, NODE_NETWORK},
0,
0,
CAddress{},
std::string{},
ConnectionType::OUTBOUND_FULL_RELAY,
false};

const uint64_t services{NODE_NETWORK | NODE_WITNESS};
const int64_t time{0};
// Our address, as seen from the peer.
const CNetMsgMaker msg_maker{PROTOCOL_VERSION};
const auto msg = msg_maker.Make(NetMsgType::VERSION, PROTOCOL_VERSION, services, time, peer_us);

CDataStream recv_stream{msg.data, SER_NETWORK, PROTOCOL_VERSION};

// Force CChainState::IsInitialBlockDownload() to return false.
// Otherwise PushAddress() isn't called by PeerManager::ProcessMessage().
TestChainState& chainstate = *(TestChainState*)&m_node.chainman->ActiveChainstate();
chainstate.JumpOutOfIbd();

m_node.peerman->InitializeNode(&peer);

std::atomic<bool> interrupt_dummy{false};
std::chrono::microseconds time_received_dummy{0};

m_node.peerman->ProcessMessage(peer, NetMsgType::VERSION, recv_stream, time_received_dummy,
interrupt_dummy);

// Ensure that peer_us:bind_port is to be sent to the peer.
const CAddress expected{CService{peer_us_addr, bind_port}, NODE_NETWORK};
BOOST_CHECK(std::any_of(peer.vAddrToSend.begin(), peer.vAddrToSend.end(),
[&expected](const CAddress& cur) { return cur == expected; }));

chainstate.ResetIbd();
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()
75 changes: 75 additions & 0 deletions test/functional/feature_bind_port_externalip.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
#!/usr/bin/env python3
# Copyright (c) 2020 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', '-bind={}'.format(ADDR)], 30002],
[['-externalip=2.2.2.2', '-bind={}'.format(ADDR)], 'default_p2p_port'],
[['-externalip=2.2.2.2', '-port=30003', '-bind={}:30004'.format(ADDR)], 30004],
[['-externalip=2.2.2.2', '-bind={}:30005'.format(ADDR)], 30005],
[['-externalip=2.2.2.2:30006', '-port=30007' ], 30006],
[['-externalip=2.2.2.2:30008', '-port=30009', '-bind={}'.format(ADDR)], 30008],
[['-externalip=2.2.2.2:30010', '-bind={}'.format(ADDR)], 30010],
[['-externalip=2.2.2.2:30011', '-port=30012', '-bind={}:30013'.format(ADDR)], 30011],
[['-externalip=2.2.2.2:30014', '-bind={}:30015'.format(ADDR)], 30014],
[['-externalip=2.2.2.2', '-port=30016', '-bind={}:30017'.format(ADDR),
'-whitebind={}:30018'.format(ADDR)], 30017],
[['-externalip=2.2.2.2', '-port=30019',
'-whitebind={}:30020'.format(ADDR)], 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="Run the test, assuming {} is configured on the machine".format(ADDR),
default=False)

def skip_test_if_missing_module(self):
if not self.options.ihave1111:
raise SkipTest(
"To run this test make sure that {} (a routable address) is assigned "
"to one of the interfaces on this machine and rerun with --ihave1111".format(ADDR))

def run_test(self):
for i in range(len(EXPECTED)):
if EXPECTED[i][1] == 'default_p2p_port':
expected_port = p2p_port(i)
else:
expected_port = EXPECTED[i][1]
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()
1 change: 1 addition & 0 deletions test/functional/test_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,7 @@
'feature_minchainwork.py',
'rpc_estimatefee.py',
'rpc_getblockstats.py',
'feature_bind_port_externalip.py',
'wallet_create_tx.py --legacy-wallet',
'wallet_send.py --legacy-wallet',
'wallet_send.py --descriptors',
Expand Down

0 comments on commit 39b4a0c

Please sign in to comment.