Merge bitcoin/bitcoin#20196: net: fix GetListenPort() to derive the proper port

7d64ea4a01920bb55bc6de0de6766712ec792a11 net: only assume all local addresses if listening on any (Vasil Dimov)
0cfc0cd32239d3c08d2121e028b297022450b320 net: fix GetListenPort() to derive the proper port (Vasil Dimov)
f98cdcb3574ee661223e1a09e1762b2cc85fab2f net: pass Span by value to CaptureMessage() (Vasil Dimov)
3cb9d9c861710c0cff018bee90b1969193d3ed68 net: make CaptureMessage() mockable (Vasil Dimov)
43868ba416727effd515a471e2a337c3b8cacc37 timedata: rename variables to match the coding style (Vasil Dimov)
60da1eaa1113e7318e273144e7ef9c8895d7ed54 timedata: make it possible to reset the state (Vasil Dimov)

Pull request description:

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

  Also, if `-bind=` is provided then we would bind only to a particular address
  and should not add all the other addresses of the machine to the list of
  local addresses.

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

ACKs for top commit:
  sipa:
    utACK 7d64ea4a01920bb55bc6de0de6766712ec792a11. I didn't review the tests in detail.
  jonatack:
    re-ACK 7d64ea4a01920bb55bc6de0de6766712ec792a11 per `git range-diff 08bcfa27 35ec977 7d64ea4`, changes are rebase-only, light re-review, re-ran the new tests locally

Tree-SHA512: 45135ab9c0ec3cc8c83e3b3e58a1c1f77eaeaba00618d54f1010db1d23d6db7d9c0dc7807e72ebc34e8b2d0e91f1e0d0e9239d13b90f1bdce8be84459e7837f0

12 files changed, 454 insertions, 27 deletions

diff --git a/src/init.cpp b/src/init.cpp
index 9813a16563..a3d53c3fae 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1668,8 +1668,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     LogPrintf("nBestHeight = %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), gArgs.GetBoolArg("-natpmp", DEFAULT_NATPMP));
 
@@ -1689,6 +1687,10 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     connOptions.nMaxOutboundLimit = *opt_max_upload;
     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.GetIntArg("-port", Params().GetDefaultPort()));
+
     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 Bitcoin Core peers connect to it. See "
@@ -1701,7 +1703,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         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);
                 if (IsBadPort(bind_addr.GetPort())) {
                     InitWarning(BadPortWarning("-bind", bind_addr.GetPort()));
@@ -1758,6 +1760,12 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         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 9bb264a38a..955eec46e3 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -126,6 +126,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<uint16_t>(gArgs.GetIntArg("-port", Params().GetDefaultPort()));
 }
 
@@ -221,7 +246,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))
     {
@@ -3085,7 +3120,10 @@ uint64_t CConnman::CalculateKeyedNetGroup(const CAddress& ad) const
     return GetDeterministicRandomizer(RANDOMIZER_ID_NETGROUP).Write(vchNetGroup.data(), vchNetGroup.size()).Finalize();
 }
 
-void CaptureMessage(const CAddress& addr, const std::string& msg_type, const Span<const unsigned char>& data, bool is_incoming)
+void CaptureMessageToFile(const CAddress& addr,
+                          const std::string& msg_type,
+                          Span<const unsigned char> data,
+                          bool is_incoming)
 {
     // Note: This function captures the message at the time of processing,
     // not at socket receive/send time.
@@ -3112,3 +3150,9 @@ void CaptureMessage(const CAddress& addr, const std::string& msg_type, const Spa
     ser_writedata32(f, size);
     f.write(AsBytes(data));
 }
+
+std::function<void(const CAddress& addr,
+                   const std::string& msg_type,
+                   Span<const unsigned char> data,
+                   bool is_incoming)>
+    CaptureMessage = CaptureMessageToFile;
diff --git a/src/net.h b/src/net.h
index bbc253e7ff..a38310938b 100644
--- a/src/net.h
+++ b/src/net.h
@@ -31,6 +31,7 @@
 #include <condition_variable>
 #include <cstdint>
 #include <deque>
+#include <functional>
 #include <map>
 #include <memory>
 #include <optional>
@@ -182,7 +183,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
@@ -1272,7 +1281,17 @@ private:
 };
 
 /** Dump binary message to file, with timestamp */
-void CaptureMessage(const CAddress& addr, const std::string& msg_type, const Span<const unsigned char>& data, bool is_incoming);
+void CaptureMessageToFile(const CAddress& addr,
+                          const std::string& msg_type,
+                          Span<const unsigned char> data,
+                          bool is_incoming);
+
+/** Defaults to `CaptureMessageToFile()`, but can be overridden by unit tests. */
+extern std::function<void(const CAddress& addr,
+                          const std::string& msg_type,
+                          Span<const unsigned char> data,
+                          bool is_incoming)>
+    CaptureMessage;
 
 struct NodeEvictionCandidate
 {
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index f05b4fd8e2..aee1e4c30c 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2711,6 +2711,10 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
                     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 908b030eea..fcb1a80765 100644
--- a/src/test/net_tests.cpp
+++ b/src/test/net_tests.cpp
@@ -4,17 +4,23 @@
 
 #include <chainparams.h>
 #include <clientversion.h>
+#include <compat.h>
 #include <cstdint>
 #include <net.h>
+#include <net_processing.h>
 #include <netaddress.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>
@@ -27,7 +33,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)
 {
@@ -607,15 +613,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
@@ -646,8 +652,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)
 {
@@ -728,4 +805,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 | NODE_WITNESS};
+    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<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};
+
+    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<const unsigned char> data,
+                                        bool is_incoming) -> void {
+        if (!is_incoming && msg_type == "addr") {
+            CDataStream s(data, SER_NETWORK, PROTOCOL_VERSION);
+            std::vector<CAddress> 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/src/test/timedata_tests.cpp b/src/test/timedata_tests.cpp
index 1dcee23bbb..478d61d5e2 100644
--- a/src/test/timedata_tests.cpp
+++ b/src/test/timedata_tests.cpp
@@ -96,9 +96,10 @@ BOOST_AUTO_TEST_CASE(addtimedata)
     // not to fix this because it prevents possible attacks. See the comment in AddTimeData() or issue #4521
     // for a more detailed explanation.
     MultiAddTimeData(2, 100); // filter median is 100 now, but nTimeOffset will not change
+    // We want this test to end with nTimeOffset==0, otherwise subsequent tests of the suite will fail.
     BOOST_CHECK_EQUAL(GetTimeOffset(), 0);
 
-    // We want this test to end with nTimeOffset==0, otherwise subsequent tests of the suite will fail.
+    TestOnlyResetTimeData();
 }
 
 BOOST_AUTO_TEST_SUITE_END()
diff --git a/src/timedata.cpp b/src/timedata.cpp
index 541580b3ff..06659871e5 100644
--- a/src/timedata.cpp
+++ b/src/timedata.cpp
@@ -39,29 +39,31 @@ int64_t GetAdjustedTime()
 
 #define BITCOIN_TIMEDATA_MAX_SAMPLES 200
 
+static std::set<CNetAddr> g_sources;
+static CMedianFilter<int64_t> g_time_offsets{BITCOIN_TIMEDATA_MAX_SAMPLES, 0};
+static bool g_warning_emitted;
+
 void AddTimeData(const CNetAddr& ip, int64_t nOffsetSample)
 {
     LOCK(g_timeoffset_mutex);
     // Ignore duplicates
-    static std::set<CNetAddr> setKnown;
-    if (setKnown.size() == BITCOIN_TIMEDATA_MAX_SAMPLES)
+    if (g_sources.size() == BITCOIN_TIMEDATA_MAX_SAMPLES)
         return;
-    if (!setKnown.insert(ip).second)
+    if (!g_sources.insert(ip).second)
         return;
 
     // Add data
-    static CMedianFilter<int64_t> vTimeOffsets(BITCOIN_TIMEDATA_MAX_SAMPLES, 0);
-    vTimeOffsets.input(nOffsetSample);
-    LogPrint(BCLog::NET, "added time data, samples %d, offset %+d (%+d minutes)\n", vTimeOffsets.size(), nOffsetSample, nOffsetSample / 60);
+    g_time_offsets.input(nOffsetSample);
+    LogPrint(BCLog::NET, "added time data, samples %d, offset %+d (%+d minutes)\n", g_time_offsets.size(), nOffsetSample, nOffsetSample / 60);
 
     // There is a known issue here (see issue #4521):
     //
-    // - The structure vTimeOffsets contains up to 200 elements, after which
+    // - The structure g_time_offsets contains up to 200 elements, after which
     // any new element added to it will not increase its size, replacing the
     // oldest element.
     //
     // - The condition to update nTimeOffset includes checking whether the
-    // number of elements in vTimeOffsets is odd, which will never happen after
+    // number of elements in g_time_offsets is odd, which will never happen after
     // there are 200 elements.
     //
     // But in this case the 'bug' is protective against some attacks, and may
@@ -71,9 +73,9 @@ void AddTimeData(const CNetAddr& ip, int64_t nOffsetSample)
     // So we should hold off on fixing this and clean it up as part of
     // a timing cleanup that strengthens it in a number of other ways.
     //
-    if (vTimeOffsets.size() >= 5 && vTimeOffsets.size() % 2 == 1) {
-        int64_t nMedian = vTimeOffsets.median();
-        std::vector<int64_t> vSorted = vTimeOffsets.sorted();
+    if (g_time_offsets.size() >= 5 && g_time_offsets.size() % 2 == 1) {
+        int64_t nMedian = g_time_offsets.median();
+        std::vector<int64_t> vSorted = g_time_offsets.sorted();
         // Only let other nodes change our time by so much
         int64_t max_adjustment = std::max<int64_t>(0, gArgs.GetIntArg("-maxtimeadjustment", DEFAULT_MAX_TIME_ADJUSTMENT));
         if (nMedian >= -max_adjustment && nMedian <= max_adjustment) {
@@ -81,8 +83,7 @@ void AddTimeData(const CNetAddr& ip, int64_t nOffsetSample)
         } else {
             nTimeOffset = 0;
 
-            static bool fDone;
-            if (!fDone) {
+            if (!g_warning_emitted) {
                 // If nobody has a time different than ours but within 5 minutes of ours, give a warning
                 bool fMatch = false;
                 for (const int64_t nOffset : vSorted) {
@@ -90,7 +91,7 @@ void AddTimeData(const CNetAddr& ip, int64_t nOffsetSample)
                 }
 
                 if (!fMatch) {
-                    fDone = true;
+                    g_warning_emitted = true;
                     bilingual_str strMessage = strprintf(_("Please check that your computer's date and time are correct! If your clock is wrong, %s will not work properly."), PACKAGE_NAME);
                     SetMiscWarning(strMessage);
                     uiInterface.ThreadSafeMessageBox(strMessage, "", CClientUIInterface::MSG_WARNING);
@@ -108,3 +109,12 @@ void AddTimeData(const CNetAddr& ip, int64_t nOffsetSample)
         }
     }
 }
+
+void TestOnlyResetTimeData()
+{
+    LOCK(g_timeoffset_mutex);
+    nTimeOffset = 0;
+    g_sources.clear();
+    g_time_offsets = CMedianFilter<int64_t>{BITCOIN_TIMEDATA_MAX_SAMPLES, 0};
+    g_warning_emitted = false;
+}
diff --git a/src/timedata.h b/src/timedata.h
index b165ecde26..2f039d5465 100644
--- a/src/timedata.h
+++ b/src/timedata.h
@@ -75,4 +75,9 @@ int64_t GetTimeOffset();
 int64_t GetAdjustedTime();
 void AddTimeData(const CNetAddr& ip, int64_t nTime);
 
+/**
+ * Reset the internal state of GetTimeOffset(), GetAdjustedTime() and AddTimeData().
+ */
+void TestOnlyResetTimeData();
+
 #endif // BITCOIN_TIMEDATA_H
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/p2p_message_capture.py b/test/functional/p2p_message_capture.py
index edde9a6ecf..0a7ae44de4 100755
--- a/test/functional/p2p_message_capture.py
+++ b/test/functional/p2p_message_capture.py
@@ -20,7 +20,7 @@ LENGTH_SIZE = 4
 MSGTYPE_SIZE = 12
 
 def mini_parser(dat_file):
-    """Parse a data file created by CaptureMessage.
+    """Parse a data file created by CaptureMessageToFile.
 
     From the data file we'll only check the structure.
 
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index d15edcedd2..b0f24e3b97 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -276,6 +276,7 @@ BASE_SCRIPTS = [
     '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',
@@ -291,6 +292,7 @@ BASE_SCRIPTS = [
     'feature_loadblock.py',
     'p2p_dos_header_tree.py',
     'p2p_add_connections.py',
+    'feature_bind_port_discover.py',
     'p2p_unrequested_blocks.py',
     'p2p_blockfilters.py',
     'p2p_message_capture.py',