Merge #6057: backport: merge bitcoin#21571, #22284, #20541, #22735, #23614, #23758, #23801, #24078, #20196, #24141, #25591, #25174 (networking backports: part 6)

71d14528ca refactor: enumerate each CNode argument on a separate line (Kittywhiskers Van Gogh)
f8d1e5b3ec merge bitcoin#25174: Add thread safety related annotations for CNode and Peer (Kittywhiskers Van Gogh)
4847f6e96f refactor: move `m_initial_sync_finished` out of header (Kittywhiskers Van Gogh)
dba4cf056b merge bitcoin#25591: Version handshake to libtest_util (Kittywhiskers Van Gogh)
b9b13bd8ec merge bitcoin#24141: Rename message_command variables in src/net* and src/rpc/net.cpp (Kittywhiskers Van Gogh)
a6aa3735be merge bitcoin#20196: fix GetListenPort() to derive the proper port (Kittywhiskers Van Gogh)
c443cf4825 merge bitcoin#24078: Rename CNetMessage::m_command with CNetMessage::m_type (Kittywhiskers Van Gogh)
182e31d04c merge bitcoin#23801: Change time variable type from int64_t to std::chrono::seconds in net_processing.cpp (Kittywhiskers Van Gogh)
6e6c9442fa merge bitcoin#23758: Use type-safe mockable time for peer connection time (Kittywhiskers Van Gogh)
7beeae77b9 merge bitcoin#23614: add unit test for block-relay-only eviction (Kittywhiskers Van Gogh)
cf8f17e423 merge bitcoin#22735: Don't return an optional from TransportDeserializer::GetMessage() (Kittywhiskers Van Gogh)
224fb687c8 merge bitcoin#20541: Move special CAddress-without-nTime logic to net_processing (Kittywhiskers Van Gogh)
30ac41e068 merge bitcoin#22284: performance improvements to ProtectEvictionCandidatesByRatio() (Kittywhiskers Van Gogh)
ad4369fd83 merge bitcoin#21571: make sure non-IP peers get discouraged and disconnected (Kittywhiskers Van Gogh)

Pull request description:

  ## Breaking Changes

  None expected.

  ## Checklist:

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  PastaPastaPasta:
    utACK 71d14528ca
  UdjinM6:
    utACK 71d14528ca

Tree-SHA512: b214d50fd87a046f22a9b3baf676483b4eee45ad267a4e501b221bbc77ef1e4532f0ad1fc8931be66761a9c50f2033ceeabbaae2bdb42f7c9edf7708bac8a9eb

src/Makefile.bench.include

@@ -40,6 +40,7 @@ bench_bench_dash_SOURCES = \
   bench/mempool_stress.cpp \
   bench/nanobench.h \
   bench/nanobench.cpp \
+  bench/peer_eviction.cpp \
   bench/rpc_blockchain.cpp \
   bench/rpc_mempool.cpp \
   bench/util_time.cpp \

src/bench/peer_eviction.cpp

@@ -0,0 +1,156 @@
+// Copyright (c) 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.
+
+#include <bench/bench.h>
+#include <net.h>
+#include <netaddress.h>
+#include <random.h>
+#include <test/util/net.h>
+#include <test/util/setup_common.h>
+
+#include <algorithm>
+#include <functional>
+#include <vector>
+
+static void EvictionProtectionCommon(
+    benchmark::Bench& bench,
+    int num_candidates,
+    std::function<void(NodeEvictionCandidate&)> candidate_setup_fn)
+{
+    using Candidates = std::vector<NodeEvictionCandidate>;
+    FastRandomContext random_context{true};
+    bench.warmup(100).epochIterations(1100);
+
+    Candidates candidates{GetRandomNodeEvictionCandidates(num_candidates, random_context)};
+    for (auto& c : candidates) {
+        candidate_setup_fn(c);
+    }
+
+    std::vector<Candidates> copies{bench.epochs() * bench.epochIterations(), candidates};
+    size_t i{0};
+    bench.run([&] {
+        ProtectEvictionCandidatesByRatio(copies.at(i));
+        ++i;
+    });
+}
+
+/* Benchmarks */
+
+static void EvictionProtection0Networks250Candidates(benchmark::Bench& bench)
+{
+    EvictionProtectionCommon(
+        bench,
+        250 /* num_candidates */,
+        [](NodeEvictionCandidate& c) {
+            c.m_connected = std::chrono::seconds{c.id};
+            c.m_network = NET_IPV4;
+        });
+}
+
+static void EvictionProtection1Networks250Candidates(benchmark::Bench& bench)
+{
+    EvictionProtectionCommon(
+        bench,
+        250 /* num_candidates */,
+        [](NodeEvictionCandidate& c) {
+            c.m_connected = std::chrono::seconds{c.id};
+            c.m_is_local = false;
+            if (c.id >= 130 && c.id < 240) { // 110 Tor
+                c.m_network = NET_ONION;
+            } else {
+                c.m_network = NET_IPV4;
+            }
+        });
+}
+
+static void EvictionProtection2Networks250Candidates(benchmark::Bench& bench)
+{
+    EvictionProtectionCommon(
+        bench,
+        250 /* num_candidates */,
+        [](NodeEvictionCandidate& c) {
+            c.m_connected = std::chrono::seconds{c.id};
+            c.m_is_local = false;
+            if (c.id >= 90 && c.id < 160) { // 70 Tor
+                c.m_network = NET_ONION;
+            } else if (c.id >= 170 && c.id < 250) { // 80 I2P
+                c.m_network = NET_I2P;
+            } else {
+                c.m_network = NET_IPV4;
+            }
+        });
+}
+
+static void EvictionProtection3Networks050Candidates(benchmark::Bench& bench)
+{
+    EvictionProtectionCommon(
+        bench,
+        50 /* num_candidates */,
+        [](NodeEvictionCandidate& c) {
+            c.m_connected = std::chrono::seconds{c.id};
+            c.m_is_local = (c.id == 28 || c.id == 47); //  2 localhost
+            if (c.id >= 30 && c.id < 47) {             // 17 I2P
+                c.m_network = NET_I2P;
+            } else if (c.id >= 24 && c.id < 28) { //  4 Tor
+                c.m_network = NET_ONION;
+            } else {
+                c.m_network = NET_IPV4;
+            }
+        });
+}
+
+static void EvictionProtection3Networks100Candidates(benchmark::Bench& bench)
+{
+    EvictionProtectionCommon(
+        bench,
+        100 /* num_candidates */,
+        [](NodeEvictionCandidate& c) {
+            c.m_connected = std::chrono::seconds{c.id};
+            c.m_is_local = (c.id >= 55 && c.id < 60); //  5 localhost
+            if (c.id >= 70 && c.id < 80) {            // 10 I2P
+                c.m_network = NET_I2P;
+            } else if (c.id >= 80 && c.id < 96) { // 16 Tor
+                c.m_network = NET_ONION;
+            } else {
+                c.m_network = NET_IPV4;
+            }
+        });
+}
+
+static void EvictionProtection3Networks250Candidates(benchmark::Bench& bench)
+{
+    EvictionProtectionCommon(
+        bench,
+        250 /* num_candidates */,
+        [](NodeEvictionCandidate& c) {
+            c.m_connected = std::chrono::seconds{c.id};
+            c.m_is_local = (c.id >= 140 && c.id < 160); // 20 localhost
+            if (c.id >= 170 && c.id < 180) {            // 10 I2P
+                c.m_network = NET_I2P;
+            } else if (c.id >= 190 && c.id < 240) { // 50 Tor
+                c.m_network = NET_ONION;
+            } else {
+                c.m_network = NET_IPV4;
+            }
+        });
+}
+
+// Candidate numbers used for the benchmarks:
+// -  50 candidates simulates a possible use of -maxconnections
+// - 100 candidates approximates an average node with default settings
+// - 250 candidates is the number of peers reported by operators of busy nodes
+
+// No disadvantaged networks, with 250 eviction candidates.
+BENCHMARK(EvictionProtection0Networks250Candidates);
+
+// 1 disadvantaged network (Tor) with 250 eviction candidates.
+BENCHMARK(EvictionProtection1Networks250Candidates);
+
+// 2 disadvantaged networks (I2P, Tor) with 250 eviction candidates.
+BENCHMARK(EvictionProtection2Networks250Candidates);
+
+// 3 disadvantaged networks (I2P/localhost/Tor) with 50/100/250 eviction candidates.
+BENCHMARK(EvictionProtection3Networks050Candidates);
+BENCHMARK(EvictionProtection3Networks100Candidates);
+BENCHMARK(EvictionProtection3Networks250Candidates);

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<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, /*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;

src/masternode/utils.cpp

@@ -49,7 +49,7 @@ void CMasternodeUtils::DoMaintenance(CConnman& connman, CDeterministicMNManager&
     connman.ForEachNode(CConnman::AllNodes, [&](CNode* pnode) {
         if (pnode->m_masternode_probe_connection) {
             // we're not disconnecting masternode probes for at least PROBE_WAIT_INTERVAL seconds
-            if (GetTimeSeconds() - pnode->nTimeConnected < PROBE_WAIT_INTERVAL) return;
+            if (GetTime<std::chrono::seconds>() - pnode->m_connected < PROBE_WAIT_INTERVAL) return;
         } else {
             // we're only disconnecting m_masternode_connection connections
             if (!pnode->m_masternode_connection) return;
@@ -67,7 +67,7 @@ void CMasternodeUtils::DoMaintenance(CConnman& connman, CDeterministicMNManager&
                 if (pnode->IsInboundConn()) {
                     return;
                 }
-            } else if (GetTimeSeconds() - pnode->nTimeConnected < 5) {
+            } else if (GetTime<std::chrono::seconds>() - pnode->m_connected < 5s) {
                 // non-verified, give it some time to verify itself
                 return;
             } else if (pnode->qwatch) {

src/net.cpp

@@ -125,7 +125,7 @@ static const uint64_t SELECT_TIMEOUT_MILLISECONDS = 50;
 static const uint64_t SELECT_TIMEOUT_MILLISECONDS = 500;
 #endif /* USE_WAKEUP_PIPE */
 
-const std::string NET_MESSAGE_COMMAND_OTHER = "*other*";
+const std::string NET_MESSAGE_TYPE_OTHER = "*other*";
 
 constexpr const CConnman::CFullyConnectedOnly CConnman::FullyConnectedOnly;
 constexpr const CConnman::CAllNodes CConnman::AllNodes;
@@ -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<uint16_t>(gArgs.GetArg("-port", Params().GetDefaultPort()));
 }
 
@@ -246,7 +271,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))
     {
@@ -704,9 +739,9 @@ void CNode::CopyStats(CNodeStats& stats)
     stats.m_network = ConnectedThroughNetwork();
     X(m_last_send);
     X(m_last_recv);
-    X(nLastTXTime);
+    X(m_last_tx_time);
-    X(nLastBlockTime);
+    X(m_last_block_time);
-    X(nTimeConnected);
+    X(m_connected);
     X(nTimeOffset);
     X(m_addr_name);
     X(nVersion);
@@ -720,12 +755,12 @@ void CNode::CopyStats(CNodeStats& stats)
     X(m_bip152_highbandwidth_from);
     {
         LOCK(cs_vSend);
-        X(mapSendBytesPerMsgCmd);
+        X(mapSendBytesPerMsgType);
         X(nSendBytes);
     }
     {
         LOCK(cs_vRecv);
-        X(mapRecvBytesPerMsgCmd);
+        X(mapRecvBytesPerMsgType);
         X(nRecvBytes);
     }
     X(m_legacyWhitelisted);
@@ -766,26 +801,27 @@ bool CNode::ReceiveMsgBytes(Span<const uint8_t> msg_bytes, bool& complete)
 
         if (m_deserializer->Complete()) {
             // decompose a transport agnostic CNetMessage from the deserializer
-            uint32_t out_err_raw_size{0};
+            bool reject_message{false};
-            std::optional<CNetMessage> result{m_deserializer->GetMessage(time, out_err_raw_size)};
+            CNetMessage msg = m_deserializer->GetMessage(time, reject_message);
-            if (!result) {
+            if (reject_message) {
-                // Message deserialization failed.  Drop the message but don't disconnect the peer.
+                // Message deserialization failed. Drop the message but don't disconnect the peer.
                 // store the size of the corrupt message
-                mapRecvBytesPerMsgCmd.find(NET_MESSAGE_COMMAND_OTHER)->second += out_err_raw_size;
+                mapRecvBytesPerMsgType.at(NET_MESSAGE_TYPE_OTHER) += msg.m_raw_message_size;
                 continue;
             }
 
-            //store received bytes per message command
+            // Store received bytes per message type.
-            //to prevent a memory DOS, only allow valid commands
+            // To prevent a memory DOS, only allow known message types.
-            mapMsgCmdSize::iterator i = mapRecvBytesPerMsgCmd.find(result->m_command);
+            auto i = mapRecvBytesPerMsgType.find(msg.m_type);
-            if (i == mapRecvBytesPerMsgCmd.end())
+            if (i == mapRecvBytesPerMsgType.end()) {
-                i = mapRecvBytesPerMsgCmd.find(NET_MESSAGE_COMMAND_OTHER);
+                i = mapRecvBytesPerMsgType.find(NET_MESSAGE_TYPE_OTHER);
-            assert(i != mapRecvBytesPerMsgCmd.end());
+            }
-            i->second += result->m_raw_message_size;
+            assert(i != mapRecvBytesPerMsgType.end());
-            statsClient.count("bandwidth.message." + std::string(result->m_command) + ".bytesReceived", result->m_raw_message_size, 1.0f);
+            i->second += msg.m_raw_message_size;
+            statsClient.count("bandwidth.message." + std::string(msg.m_type) + ".bytesReceived", msg.m_raw_message_size, 1.0f);
 
             // push the message to the process queue,
-            vRecvMsg.push_back(std::move(*result));
+            vRecvMsg.push_back(std::move(msg));
 
             complete = true;
         }
@@ -859,36 +895,36 @@ const uint256& V1TransportDeserializer::GetMessageHash() const
     return data_hash;
 }
 
-std::optional<CNetMessage> V1TransportDeserializer::GetMessage(const std::chrono::microseconds time, uint32_t& out_err_raw_size)
+CNetMessage V1TransportDeserializer::GetMessage(const std::chrono::microseconds time, bool& reject_message)
 {
+    // Initialize out parameter
+    reject_message = false;
     // decompose a single CNetMessage from the TransportDeserializer
-    std::optional<CNetMessage> msg(std::move(vRecv));
+    CNetMessage msg(std::move(vRecv));
 
-    // store command string, time, and sizes
+    // store message type string, time, and sizes
-    msg->m_command = hdr.GetCommand();
+    msg.m_type = hdr.GetCommand();
-    msg->m_time = time;
+    msg.m_time = time;
-    msg->m_message_size = hdr.nMessageSize;
+    msg.m_message_size = hdr.nMessageSize;
-    msg->m_raw_message_size = hdr.nMessageSize + CMessageHeader::HEADER_SIZE;
+    msg.m_raw_message_size = hdr.nMessageSize + CMessageHeader::HEADER_SIZE;
 
     uint256 hash = GetMessageHash();
 
     // We just received a message off the wire, harvest entropy from the time (and the message checksum)
     RandAddEvent(ReadLE32(hash.begin()));
 
-    // Check checksum and header command string
+    // Check checksum and header message type string
     if (memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) != 0) {
         LogPrint(BCLog::NET, "Header error: Wrong checksum (%s, %u bytes), expected %s was %s, peer=%d\n",
-                 SanitizeString(msg->m_command), msg->m_message_size,
+                 SanitizeString(msg.m_type), msg.m_message_size,
                  HexStr(Span{hash}.first(CMessageHeader::CHECKSUM_SIZE)),
                  HexStr(hdr.pchChecksum),
                  m_node_id);
-        out_err_raw_size = msg->m_raw_message_size;
+        reject_message = true;
-        msg.reset();
     } else if (!hdr.IsCommandValid()) {
         LogPrint(BCLog::NET, "Header error: Invalid message type (%s, %u bytes), peer=%d\n",
-                 SanitizeString(hdr.GetCommand()), msg->m_message_size, m_node_id);
+                 SanitizeString(hdr.GetCommand()), msg.m_message_size, m_node_id);
-        out_err_raw_size = msg->m_raw_message_size;
+        reject_message = true;
-        msg.reset();
     }
 
     // Always reset the network deserializer (prepare for the next message)
@@ -896,7 +932,8 @@ std::optional<CNetMessage> V1TransportDeserializer::GetMessage(const std::chrono
     return msg;
 }
 
-void V1TransportSerializer::prepareForTransport(CSerializedNetMsg& msg, std::vector<unsigned char>& header) {
+void V1TransportSerializer::prepareForTransport(CSerializedNetMsg& msg, std::vector<unsigned char>& header) const
+{
     // create dbl-sha256 checksum
     uint256 hash = Hash(msg.data);
 
@@ -971,7 +1008,7 @@ static bool ReverseCompareNodeMinPingTime(const NodeEvictionCandidate& a, const
 
 static bool ReverseCompareNodeTimeConnected(const NodeEvictionCandidate& a, const NodeEvictionCandidate& b)
 {
-    return a.nTimeConnected > b.nTimeConnected;
+    return a.m_connected > b.m_connected;
 }
 
 static bool CompareNetGroupKeyed(const NodeEvictionCandidate &a, const NodeEvictionCandidate &b) {
@@ -981,27 +1018,27 @@ static bool CompareNetGroupKeyed(const NodeEvictionCandidate &a, const NodeEvict
 static bool CompareNodeBlockTime(const NodeEvictionCandidate &a, const NodeEvictionCandidate &b)
 {
     // There is a fall-through here because it is common for a node to have many peers which have not yet relayed a block.
-    if (a.nLastBlockTime != b.nLastBlockTime) return a.nLastBlockTime < b.nLastBlockTime;
+    if (a.m_last_block_time != b.m_last_block_time) return a.m_last_block_time < b.m_last_block_time;
     if (a.fRelevantServices != b.fRelevantServices) return b.fRelevantServices;
-    return a.nTimeConnected > b.nTimeConnected;
+    return a.m_connected > b.m_connected;
 }
 
 static bool CompareNodeTXTime(const NodeEvictionCandidate &a, const NodeEvictionCandidate &b)
 {
     // There is a fall-through here because it is common for a node to have more than a few peers that have not yet relayed txn.
-    if (a.nLastTXTime != b.nLastTXTime) return a.nLastTXTime < b.nLastTXTime;
+    if (a.m_last_tx_time != b.m_last_tx_time) return a.m_last_tx_time < b.m_last_tx_time;
     if (a.m_relay_txs != b.m_relay_txs) return b.m_relay_txs;
     if (a.fBloomFilter != b.fBloomFilter) return a.fBloomFilter;
-    return a.nTimeConnected > b.nTimeConnected;
+    return a.m_connected > b.m_connected;
 }
 
 // Pick out the potential block-relay only peers, and sort them by last block time.
 static bool CompareNodeBlockRelayOnlyTime(const NodeEvictionCandidate &a, const NodeEvictionCandidate &b)
 {
     if (a.m_relay_txs != b.m_relay_txs) return a.m_relay_txs;
-    if (a.nLastBlockTime != b.nLastBlockTime) return a.nLastBlockTime < b.nLastBlockTime;
+    if (a.m_last_block_time != b.m_last_block_time) return a.m_last_block_time < b.m_last_block_time;
     if (a.fRelevantServices != b.fRelevantServices) return b.fRelevantServices;
-    return a.nTimeConnected > b.nTimeConnected;
+    return a.m_connected > b.m_connected;
 }
 
 /**
@@ -1020,7 +1057,7 @@ struct CompareNodeNetworkTime {
     {
         if (m_is_local && a.m_is_local != b.m_is_local) return b.m_is_local;
         if ((a.m_network == m_network) != (b.m_network == m_network)) return b.m_network == m_network;
-        return a.nTimeConnected > b.nTimeConnected;
+        return a.m_connected > b.m_connected;
     };
 };
 
@@ -1068,14 +1105,17 @@ void ProtectEvictionCandidatesByRatio(std::vector<NodeEvictionCandidate>& evicti
     size_t num_protected{0};
 
     while (num_protected < max_protect_by_network) {
+        // Count the number of disadvantaged networks from which we have peers to protect.
+        auto num_networks = std::count_if(networks.begin(), networks.end(), [](const Net& n) { return n.count; });
+        if (num_networks == 0) {
+            break;
+        }
         const size_t disadvantaged_to_protect{max_protect_by_network - num_protected};
-        const size_t protect_per_network{
+        const size_t protect_per_network{std::max(disadvantaged_to_protect / num_networks, static_cast<size_t>(1))};
-            std::max(disadvantaged_to_protect / networks.size(), static_cast<size_t>(1))};
-
         // Early exit flag if there are no remaining candidates by disadvantaged network.
         bool protected_at_least_one{false};
 
-        for (const Net& n : networks) {
+        for (Net& n : networks) {
             if (n.count == 0) continue;
             const size_t before = eviction_candidates.size();
             EraseLastKElements(eviction_candidates, CompareNodeNetworkTime(n.is_local, n.id),
@@ -1085,10 +1125,12 @@ void ProtectEvictionCandidatesByRatio(std::vector<NodeEvictionCandidate>& evicti
             const size_t after = eviction_candidates.size();
             if (before > after) {
                 protected_at_least_one = true;
-                num_protected += before - after;
+                const size_t delta{before - after};
+                num_protected += delta;
                 if (num_protected >= max_protect_by_network) {
                     break;
                 }
+                n.count -= delta;
             }
         }
         if (!protected_at_least_one) {
@@ -1142,12 +1184,12 @@ void ProtectEvictionCandidatesByRatio(std::vector<NodeEvictionCandidate>& evicti
     // (vEvictionCandidates is already sorted by reverse connect time)
     uint64_t naMostConnections;
     unsigned int nMostConnections = 0;
-    int64_t nMostConnectionsTime = 0;
+    std::chrono::seconds nMostConnectionsTime{0};
     std::map<uint64_t, std::vector<NodeEvictionCandidate> > mapNetGroupNodes;
     for (const NodeEvictionCandidate &node : vEvictionCandidates) {
         std::vector<NodeEvictionCandidate> &group = mapNetGroupNodes[node.nKeyedNetGroup];
         group.push_back(node);
-        const int64_t grouptime = group[0].nTimeConnected;
+        const auto grouptime{group[0].m_connected};
 
         if (group.size() > nMostConnections || (group.size() == nMostConnections && grouptime > nMostConnectionsTime)) {
             nMostConnections = group.size();
@@ -1190,7 +1232,7 @@ bool CConnman::AttemptToEvictConnection()
                 // was accepted. This short time is meant for the VERSION/VERACK exchange and the possible MNAUTH that might
                 // follow when the incoming connection is from another masternode. When a message other than MNAUTH
                 // is received after VERSION/VERACK, the protection is lifted immediately.
-                bool isProtected = GetTimeSeconds() - node->nTimeConnected < INBOUND_EVICTION_PROTECTION_TIME;
+                bool isProtected = GetTime<std::chrono::seconds>() - node->m_connected < INBOUND_EVICTION_PROTECTION_TIME;
                 if (node->nTimeFirstMessageReceived != 0 && !node->fFirstMessageIsMNAUTH) {
                     isProtected = false;
                 }
@@ -1204,8 +1246,8 @@ bool CConnman::AttemptToEvictConnection()
                 }
             }
 
-            NodeEvictionCandidate candidate = {node->GetId(), node->nTimeConnected, node->m_min_ping_time,
+            NodeEvictionCandidate candidate = {node->GetId(), node->m_connected, node->m_min_ping_time,
-                                               node->nLastBlockTime, node->nLastTXTime,
+                                               node->m_last_block_time, node->m_last_tx_time,
                                                HasAllDesirableServiceFlags(node->nServices),
                                                node->m_relays_txs.load(), node->m_bloom_filter_loaded.load(),
                                                node->nKeyedNetGroup, node->m_prefer_evict, node->addr.IsLocal(),
@@ -1584,24 +1626,24 @@ void CConnman::CalculateNumConnectionsChangedStats()
     int ipv4Nodes = 0;
     int ipv6Nodes = 0;
     int torNodes = 0;
-    mapMsgCmdSize mapRecvBytesMsgStats;
+    mapMsgTypeSize mapRecvBytesMsgStats;
-    mapMsgCmdSize mapSentBytesMsgStats;
+    mapMsgTypeSize mapSentBytesMsgStats;
     for (const std::string &msg : getAllNetMessageTypes()) {
         mapRecvBytesMsgStats[msg] = 0;
         mapSentBytesMsgStats[msg] = 0;
     }
-    mapRecvBytesMsgStats[NET_MESSAGE_COMMAND_OTHER] = 0;
+    mapRecvBytesMsgStats[NET_MESSAGE_TYPE_OTHER] = 0;
-    mapSentBytesMsgStats[NET_MESSAGE_COMMAND_OTHER] = 0;
+    mapSentBytesMsgStats[NET_MESSAGE_TYPE_OTHER] = 0;
     const NodesSnapshot snap{*this, /* filter = */ CConnman::FullyConnectedOnly};
     for (auto pnode : snap.Nodes()) {
         {
             LOCK(pnode->cs_vRecv);
-            for (const mapMsgCmdSize::value_type &i : pnode->mapRecvBytesPerMsgCmd)
+            for (const mapMsgTypeSize::value_type &i : pnode->mapRecvBytesPerMsgType)
                 mapRecvBytesMsgStats[i.first] += i.second;
         }
         {
             LOCK(pnode->cs_vSend);
-            for (const mapMsgCmdSize::value_type &i : pnode->mapSendBytesPerMsgCmd)
+            for (const mapMsgTypeSize::value_type &i : pnode->mapSendBytesPerMsgType)
                 mapSentBytesMsgStats[i.first] += i.second;
         }
         if(pnode->fClient)
@@ -1638,7 +1680,7 @@ void CConnman::CalculateNumConnectionsChangedStats()
 
 bool CConnman::ShouldRunInactivityChecks(const CNode& node, std::chrono::seconds now) const
 {
-    return std::chrono::seconds{node.nTimeConnected} + m_peer_connect_timeout < now;
+    return node.m_connected + m_peer_connect_timeout < now;
 }
 
 bool CConnman::InactivityCheck(const CNode& node) const
@@ -4084,9 +4126,21 @@ ServiceFlags CConnman::GetLocalServices() const
 
 unsigned int CConnman::GetReceiveFloodSize() const { return nReceiveFloodSize; }
 
-CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, std::shared_ptr<Sock> sock, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress& addrBindIn, const std::string& addrNameIn, ConnectionType conn_type_in, bool inbound_onion, std::unique_ptr<i2p::sam::Session>&& i2p_sam_session)
+CNode::CNode(NodeId idIn,
-    : m_sock{sock},
+             ServiceFlags nLocalServicesIn,
-      nTimeConnected{GetTimeSeconds()},
+             std::shared_ptr<Sock> sock,
+             const CAddress& addrIn,
+             uint64_t nKeyedNetGroupIn,
+             uint64_t nLocalHostNonceIn,
+             const CAddress& addrBindIn,
+             const std::string& addrNameIn,
+             ConnectionType conn_type_in,
+             bool inbound_onion,
+             std::unique_ptr<i2p::sam::Session>&& i2p_sam_session)
+    : m_deserializer{std::make_unique<V1TransportDeserializer>(V1TransportDeserializer(Params(), idIn, SER_NETWORK, INIT_PROTO_VERSION))},
+      m_serializer{std::make_unique<V1TransportSerializer>(V1TransportSerializer())},
+      m_sock{sock},
+      m_connected{GetTime<std::chrono::seconds>()},
       addr{addrIn},
       addrBind{addrBindIn},
       m_addr_name{addrNameIn.empty() ? addr.ToStringIPPort() : addrNameIn},
@@ -4101,17 +4155,14 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, std::shared_ptr<Sock> s
     if (inbound_onion) assert(conn_type_in == ConnectionType::INBOUND);
 
     for (const std::string &msg : getAllNetMessageTypes())
-        mapRecvBytesPerMsgCmd[msg] = 0;
+        mapRecvBytesPerMsgType[msg] = 0;
-    mapRecvBytesPerMsgCmd[NET_MESSAGE_COMMAND_OTHER] = 0;
+    mapRecvBytesPerMsgType[NET_MESSAGE_TYPE_OTHER] = 0;
 
     if (fLogIPs) {
         LogPrint(BCLog::NET, "Added connection to %s peer=%d\n", m_addr_name, id);
     } else {
         LogPrint(BCLog::NET, "Added connection peer=%d\n", id);
     }
-
-    m_deserializer = std::make_unique<V1TransportDeserializer>(V1TransportDeserializer(Params(), GetId(), SER_NETWORK, INIT_PROTO_VERSION));
-    m_serializer = std::make_unique<V1TransportSerializer>(V1TransportSerializer());
 }
 
 bool CConnman::NodeFullyConnected(const CNode* pnode)
@@ -4141,7 +4192,7 @@ void CConnman::PushMessage(CNode* pnode, CSerializedNetMsg&& msg)
         bool hasPendingData = !pnode->vSendMsg.empty();
 
         //log total amount of bytes per message type
-        pnode->mapSendBytesPerMsgCmd[msg.m_type] += nTotalSize;
+        pnode->mapSendBytesPerMsgType[msg.m_type] += nTotalSize;
         pnode->nSendSize += nTotalSize;
 
         if (pnode->nSendSize > nSendBufferMaxSize) pnode->fPauseSend = true;

src/net.h

@@ -63,8 +63,8 @@ static const bool DEFAULT_WHITELISTFORCERELAY = false;
 
 /** Time after which to disconnect, after waiting for a ping response (or inactivity). */
 static constexpr std::chrono::minutes TIMEOUT_INTERVAL{20};
-/** Time to wait since nTimeConnected before disconnecting a probe node. **/
+/** Time to wait since m_connected before disconnecting a probe node. */
-static const int PROBE_WAIT_INTERVAL = 5;
+static const auto PROBE_WAIT_INTERVAL{5s};
 /** Minimum time between warnings printed to log. */
 static const int WARNING_INTERVAL = 10 * 60;
 /** Run the feeler connection loop once every 2 minutes. **/
@@ -82,7 +82,7 @@ static const int MAX_OUTBOUND_FULL_RELAY_CONNECTIONS = 8;
 /** Maximum number of addnode outgoing nodes */
 static const int MAX_ADDNODE_CONNECTIONS = 8;
 /** Eviction protection time for incoming connections  */
-static const int INBOUND_EVICTION_PROTECTION_TIME = 1;
+static const auto INBOUND_EVICTION_PROTECTION_TIME{1s};
 /** Maximum number of block-relay-only outgoing connections */
 static const int MAX_BLOCK_RELAY_ONLY_CONNECTIONS = 2;
 /** Maximum number of feeler connections */
@@ -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
@@ -266,8 +274,8 @@ struct LocalServiceInfo {
 extern Mutex g_maplocalhost_mutex;
 extern std::map<CNetAddr, LocalServiceInfo> mapLocalHost GUARDED_BY(g_maplocalhost_mutex);
 
-extern const std::string NET_MESSAGE_COMMAND_OTHER;
+extern const std::string NET_MESSAGE_TYPE_OTHER;
-typedef std::map<std::string, uint64_t> mapMsgCmdSize; //command, total bytes
+using mapMsgTypeSize = std::map</* message type */ std::string, /* total bytes */ uint64_t>;
 
 class CNodeStats
 {
@@ -276,9 +284,9 @@ public:
     ServiceFlags nServices;
     std::chrono::seconds m_last_send;
     std::chrono::seconds m_last_recv;
-    int64_t nLastTXTime;
+    std::chrono::seconds m_last_tx_time;
-    int64_t nLastBlockTime;
+    std::chrono::seconds m_last_block_time;
-    int64_t nTimeConnected;
+    std::chrono::seconds m_connected;
     int64_t nTimeOffset;
     std::string m_addr_name;
     int nVersion;
@@ -289,9 +297,9 @@ public:
     bool m_bip152_highbandwidth_from;
     int m_starting_height;
     uint64_t nSendBytes;
-    mapMsgCmdSize mapSendBytesPerMsgCmd;
+    mapMsgTypeSize mapSendBytesPerMsgType;
     uint64_t nRecvBytes;
-    mapMsgCmdSize mapRecvBytesPerMsgCmd;
+    mapMsgTypeSize mapRecvBytesPerMsgType;
     NetPermissionFlags m_permissionFlags;
     bool m_legacyWhitelisted;
     std::chrono::microseconds m_last_ping_time;
@@ -316,7 +324,7 @@ public:
 
 /** Transport protocol agnostic message container.
  * Ideally it should only contain receive time, payload,
- * command and size.
+ * type and size.
  */
 class CNetMessage {
 public:
@@ -324,7 +332,7 @@ public:
     std::chrono::microseconds m_time{0}; //!< time of message receipt
     uint32_t m_message_size{0};     //!< size of the payload
     uint32_t m_raw_message_size{0}; //!< used wire size of the message (including header/checksum)
-    std::string m_command;
+    std::string m_type;
 
     CNetMessage(CDataStream&& recv_in) : m_recv(std::move(recv_in)) {}
 
@@ -336,7 +344,7 @@ public:
 
 /** The TransportDeserializer takes care of holding and deserializing the
  * network receive buffer. It can deserialize the network buffer into a
- * transport protocol agnostic CNetMessage (command & payload)
+ * transport protocol agnostic CNetMessage (message type & payload)
  */
 class TransportDeserializer {
 public:
@@ -347,7 +355,7 @@ public:
     /** read and deserialize data, advances msg_bytes data pointer */
     virtual int Read(Span<const uint8_t>& msg_bytes) = 0;
     // decomposes a message from the context
-    virtual std::optional<CNetMessage> GetMessage(std::chrono::microseconds time, uint32_t& out_err) = 0;
+    virtual CNetMessage GetMessage(std::chrono::microseconds time, bool& reject_message) = 0;
     virtual ~TransportDeserializer() {}
 };
 
@@ -411,7 +419,7 @@ public:
         }
         return ret;
     }
-    std::optional<CNetMessage> GetMessage(std::chrono::microseconds time, uint32_t& out_err_raw_size) override;
+    CNetMessage GetMessage(std::chrono::microseconds time, bool& reject_message) override;
 };
 
 /** The TransportSerializer prepares messages for the network transport
@@ -419,13 +427,13 @@ public:
 class TransportSerializer {
 public:
     // prepare message for transport (header construction, error-correction computation, payload encryption, etc.)
-    virtual void prepareForTransport(CSerializedNetMsg& msg, std::vector<unsigned char>& header) = 0;
+    virtual void prepareForTransport(CSerializedNetMsg& msg, std::vector<unsigned char>& header) const = 0;
     virtual ~TransportSerializer() {}
 };
 
-class V1TransportSerializer  : public TransportSerializer {
+class V1TransportSerializer : public TransportSerializer {
 public:
-    void prepareForTransport(CSerializedNetMsg& msg, std::vector<unsigned char>& header) override;
+    void prepareForTransport(CSerializedNetMsg& msg, std::vector<unsigned char>& header) const override;
 };
 
 /** Information about a peer */
@@ -435,10 +443,10 @@ class CNode
     friend struct ConnmanTestMsg;
 
 public:
-    std::unique_ptr<TransportDeserializer> m_deserializer;
+    const std::unique_ptr<TransportDeserializer> m_deserializer; // Used only by SocketHandler thread
-    std::unique_ptr<TransportSerializer> m_serializer;
+    const std::unique_ptr<const TransportSerializer> m_serializer;
 
-    NetPermissionFlags m_permissionFlags{ NetPermissionFlags::None };
+    NetPermissionFlags m_permissionFlags{NetPermissionFlags::None}; // treated as const outside of fuzz tester
     std::atomic<ServiceFlags> nServices{NODE_NONE};
 
     /**
@@ -464,7 +472,7 @@ public:
 
     RecursiveMutex cs_vProcessMsg;
     std::list<CNetMessage> vProcessMsg GUARDED_BY(cs_vProcessMsg);
-    size_t nProcessQueueSize{0};
+    size_t nProcessQueueSize GUARDED_BY(cs_vProcessMsg){0};
 
     RecursiveMutex cs_sendProcessing;
 
@@ -472,8 +480,8 @@ public:
 
     std::atomic<std::chrono::seconds> m_last_send{0s};
     std::atomic<std::chrono::seconds> m_last_recv{0s};
-    //! Unix epoch time at peer connection, in seconds.
+    //! Unix epoch time at peer connection
-    const int64_t nTimeConnected;
+    const std::chrono::seconds m_connected;
     std::atomic<int64_t> nTimeOffset{0};
     std::atomic<int64_t> nLastWarningTime{0};
     std::atomic<int64_t> nTimeFirstMessageReceived{0};
@@ -493,7 +501,7 @@ public:
      * from the wire. This cleaned string can safely be logged or displayed.
      */
     std::string cleanSubVer GUARDED_BY(m_subver_mutex){};
-    bool m_prefer_evict{false}; // This peer is preferred for eviction.
+    bool m_prefer_evict{false}; // This peer is preferred for eviction. (treated as const)
     bool HasPermission(NetPermissionFlags permission) const {
         return NetPermissions::HasFlag(m_permissionFlags, permission);
     }
@@ -513,7 +521,7 @@ public:
     std::atomic<bool> m_masternode_connection{false};
     /**
      * If 'true' this node will be disconnected after MNAUTH (outbound only) or
-     * after PROBE_WAIT_INTERVAL seconds since nTimeConnected
+     * after PROBE_WAIT_INTERVAL seconds since m_connected
      */
     std::atomic<bool> m_masternode_probe_connection{false};
     // If 'true', we identified it as an intra-quorum relay connection
@@ -615,13 +623,13 @@ public:
      * preliminary validity checks and was saved to disk, even if we don't
      * connect the block or it eventually fails connection. Used as an inbound
      * peer eviction criterium in CConnman::AttemptToEvictConnection. */
-    std::atomic<int64_t> nLastBlockTime{0};
+    std::atomic<std::chrono::seconds> m_last_block_time{0s};
 
     /** UNIX epoch time of the last transaction received from this peer that we
      * had not yet seen (e.g. not already received from another peer) and that
      * was accepted into our mempool. Used as an inbound peer eviction criterium
      * in CConnman::AttemptToEvictConnection. */
-    std::atomic<int64_t> nLastTXTime{0};
+    std::atomic<std::chrono::seconds> m_last_tx_time{0s};
 
     /** Last measured round-trip time. Used only for RPC/GUI stats/debugging.*/
     std::atomic<std::chrono::microseconds> m_last_ping_time{0us};
@@ -640,7 +648,17 @@ public:
 
     bool IsBlockRelayOnly() const;
 
-    CNode(NodeId id, ServiceFlags nLocalServicesIn, std::shared_ptr<Sock> sock, const CAddress &addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress &addrBindIn, const std::string &addrNameIn, ConnectionType conn_type_in, bool inbound_onion, std::unique_ptr<i2p::sam::Session>&& i2p_sam_session = nullptr);
+    CNode(NodeId id,
+          ServiceFlags nLocalServicesIn,
+          std::shared_ptr<Sock> sock,
+          const CAddress &addrIn,
+          uint64_t nKeyedNetGroupIn,
+          uint64_t nLocalHostNonceIn,
+          const CAddress &addrBindIn,
+          const std::string &addrNameIn,
+          ConnectionType conn_type_in,
+          bool inbound_onion,
+          std::unique_ptr<i2p::sam::Session>&& i2p_sam_session = nullptr);
     CNode(const CNode&) = delete;
     CNode& operator=(const CNode&) = delete;
 
@@ -791,8 +809,8 @@ private:
     uint256 verifiedProRegTxHash GUARDED_BY(cs_mnauth);
     uint256 verifiedPubKeyHash GUARDED_BY(cs_mnauth);
 
-    mapMsgCmdSize mapSendBytesPerMsgCmd GUARDED_BY(cs_vSend);
+    mapMsgTypeSize mapSendBytesPerMsgType GUARDED_BY(cs_vSend);
-    mapMsgCmdSize mapRecvBytesPerMsgCmd GUARDED_BY(cs_vRecv);
+    mapMsgTypeSize mapRecvBytesPerMsgType GUARDED_BY(cs_vRecv);
 
     /**
      * If an I2P session is created per connection (for outbound transient I2P
@@ -1628,10 +1646,10 @@ extern std::function<void(const CAddress& addr,
 struct NodeEvictionCandidate
 {
     NodeId id;
-    int64_t nTimeConnected;
+    std::chrono::seconds m_connected;
     std::chrono::microseconds m_min_ping_time;
-    int64_t nLastBlockTime;
+    std::chrono::seconds m_last_block_time;
-    int64_t nLastTXTime;
+    std::chrono::seconds m_last_tx_time;
     bool fRelevantServices;
     bool m_relay_txs;
     bool fBloomFilter;

src/net_processing.cpp

@@ -34,6 +34,8 @@
 #include <util/strencodings.h>
 
 #include <algorithm>
+#include <atomic>
+#include <chrono>
 #include <list>
 #include <memory>
 #include <optional>
@@ -101,12 +103,12 @@ static constexpr auto HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER = 1ms;
 static constexpr int32_t MAX_OUTBOUND_PEERS_TO_PROTECT_FROM_DISCONNECT = 4;
 /** Timeout for (unprotected) outbound peers to sync to our chainwork, in seconds */
 static constexpr int64_t CHAIN_SYNC_TIMEOUT = 20 * 60; // 20 minutes
-/** How frequently to check for stale tips, in seconds */
+/** How frequently to check for stale tips */
-static constexpr int64_t STALE_CHECK_INTERVAL = 2.5 * 60; // 2.5 minutes (~block interval)
+static constexpr auto STALE_CHECK_INTERVAL{150s}; // 2.5 minutes (~block interval)
-/** How frequently to check for extra outbound peers and disconnect, in seconds */
+/** How frequently to check for extra outbound peers and disconnect */
-static constexpr int64_t EXTRA_PEER_CHECK_INTERVAL = 45;
+static constexpr auto EXTRA_PEER_CHECK_INTERVAL{45s};
-/** Minimum time an outbound-peer-eviction candidate must be connected for, in order to evict, in seconds */
+/** Minimum time an outbound-peer-eviction candidate must be connected for, in order to evict */
-static constexpr int64_t MINIMUM_CONNECT_TIME = 30;
+static constexpr std::chrono::seconds MINIMUM_CONNECT_TIME{30};
 /** SHA256("main address relay")[0:8] */
 static constexpr uint64_t RANDOMIZER_ID_ADDRESS_RELAY = 0x3cac0035b5866b90ULL;
 /// Age after which a stale block will no longer be served if requested as
@@ -420,7 +422,7 @@ private:
     void ConsiderEviction(CNode& pto, int64_t time_in_seconds) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
     /** If we have extra outbound peers, try to disconnect the one with the oldest block announcement */
-    void EvictExtraOutboundPeers(int64_t time_in_seconds) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+    void EvictExtraOutboundPeers(std::chrono::seconds now) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
     /** Retrieve unbroadcast transactions from the mempool and reattempt sending to peers */
     void ReattemptInitialBroadcast(CScheduler& scheduler) EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
@@ -517,11 +519,15 @@ private:
     std::atomic<int> m_best_height{-1};
 
     /** Next time to check for stale tip */
-    int64_t m_stale_tip_check_time{0};
+    std::chrono::seconds m_stale_tip_check_time GUARDED_BY(cs_main){0s};
 
     /** Whether this node is running in blocks only mode */
     const bool m_ignore_incoming_txs;
 
+    /** Whether we've completed initial sync yet, for determining when to turn
+      * on extra block-relay-only peers. */
+    bool m_initial_sync_finished GUARDED_BY(cs_main){false};
+
     /** Protects m_peer_map. This mutex must not be locked while holding a lock
      *  on any of the mutexes inside a Peer object. */
     mutable Mutex m_peer_mutex;
@@ -685,7 +691,7 @@ private:
     std::map<uint256, std::pair<NodeId, std::list<QueuedBlock>::iterator> > mapBlocksInFlight GUARDED_BY(cs_main);
 
     /** When our tip was last updated. */
-    std::atomic<int64_t> m_last_tip_update{0};
+    std::atomic<std::chrono::seconds> m_last_tip_update{0s};
 
     /** Determine whether or not a peer can request a transaction, and return it (or nullptr if not found or not allowed). */
     CTransactionRef FindTxForGetData(const CNode* peer, const uint256& txid, const std::chrono::seconds mempool_req, const std::chrono::seconds now) LOCKS_EXCLUDED(cs_main);
@@ -1108,10 +1114,10 @@ bool PeerManagerImpl::TipMayBeStale()
 {
     AssertLockHeld(cs_main);
     const Consensus::Params& consensusParams = m_chainparams.GetConsensus();
-    if (m_last_tip_update == 0) {
+    if (count_seconds(m_last_tip_update) == 0) {
-        m_last_tip_update = GetTime();
+        m_last_tip_update = GetTime<std::chrono::seconds>();
     }
-    return m_last_tip_update < GetTime() - consensusParams.nPowTargetSpacing * 3 && mapBlocksInFlight.empty();
+    return count_seconds(m_last_tip_update) < GetTime() - consensusParams.nPowTargetSpacing * 3 && mapBlocksInFlight.empty();
 }
 
 bool PeerManagerImpl::CanDirectFetch()
@@ -1254,17 +1260,15 @@ void PeerManagerImpl::PushNodeVersion(CNode& pnode, const Peer& peer)
     // Note that pnode->GetLocalServices() is a reflection of the local
     // services we were offering when the CNode object was created for this
     // peer.
-    ServiceFlags nLocalNodeServices = pnode.GetLocalServices();
+    uint64_t my_services{pnode.GetLocalServices()};
     const int64_t nTime{count_seconds(GetTime<std::chrono::seconds>())};
     uint64_t nonce = pnode.GetLocalNonce();
     const int nNodeStartingHeight{m_best_height};
     NodeId nodeid = pnode.GetId();
     CAddress addr = pnode.addr;
 
-    CAddress addrYou = addr.IsRoutable() && !IsProxy(addr) && addr.IsAddrV1Compatible() ?
+    CService addr_you = addr.IsRoutable() && !IsProxy(addr) && addr.IsAddrV1Compatible() ? addr : CService();
-                           addr :
+    uint64_t your_services{addr.nServices};
-                           CAddress(CService(), addr.nServices);
-    CAddress addrMe = CAddress(CService(), nLocalNodeServices);
 
     uint256 mnauthChallenge;
     GetRandBytes({mnauthChallenge.begin(), mnauthChallenge.size()});
@@ -1276,13 +1280,15 @@ void PeerManagerImpl::PushNodeVersion(CNode& pnode, const Peer& peer)
     }
 
     const bool tx_relay = !m_ignore_incoming_txs && !pnode.IsBlockOnlyConn();
-    m_connman.PushMessage(&pnode, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERSION, nProtocolVersion, (uint64_t)nLocalNodeServices, nTime, addrYou, addrMe,
+    m_connman.PushMessage(&pnode, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERSION, nProtocolVersion, my_services, nTime,
-            nonce, strSubVersion, nNodeStartingHeight, tx_relay, mnauthChallenge, pnode.m_masternode_connection.load()));
+                          your_services, addr_you, // Together the pre-version-31402 serialization of CAddress "addrYou" (without nTime)
+                          my_services, CService(), // Together the pre-version-31402 serialization of CAddress "addrMe" (without nTime)
+                          nonce, strSubVersion, nNodeStartingHeight, tx_relay, mnauthChallenge, pnode.m_masternode_connection.load()));
 
     if (fLogIPs) {
-        LogPrint(BCLog::NET, "send version message: version %d, blocks=%d, us=%s, them=%s, txrelay=%d, peer=%d\n", nProtocolVersion, nNodeStartingHeight, addrMe.ToString(), addrYou.ToString(), tx_relay, nodeid);
+        LogPrint(BCLog::NET, "send version message: version %d, blocks=%d, them=%s, txrelay=%d, peer=%d\n", nProtocolVersion, nNodeStartingHeight, addr_you.ToString(), tx_relay, nodeid);
     } else {
-        LogPrint(BCLog::NET, "send version message: version %d, blocks=%d, us=%s, txrelay=%d, peer=%d\n", nProtocolVersion, nNodeStartingHeight, addrMe.ToString(), tx_relay, nodeid);
+        LogPrint(BCLog::NET, "send version message: version %d, blocks=%d, txrelay=%d, peer=%d\n", nProtocolVersion, nNodeStartingHeight, tx_relay, nodeid);
     }
 }
 
@@ -1956,7 +1962,7 @@ void PeerManagerImpl::BlockConnected(const std::shared_ptr<const CBlock>& pblock
             ProcessOrphanTx(orphanWorkSet);
         }
 
-        m_last_tip_update = GetTime();
+        m_last_tip_update = GetTime<std::chrono::seconds>();
     }
     {
         LOCK(m_recent_confirmed_transactions_mutex);
@@ -3240,7 +3246,7 @@ void PeerManagerImpl::ProcessBlock(CNode& pfrom, const std::shared_ptr<const CBl
     bool fNewBlock = false;
     m_chainman.ProcessNewBlock(m_chainparams, pblock, fForceProcessing, &fNewBlock);
     if (fNewBlock) {
-        pfrom.nLastBlockTime = GetTime();
+        pfrom.m_last_block_time = GetTime<std::chrono::seconds>();
     } else {
         LOCK(cs_main);
         mapBlockSource.erase(pblock->GetHash());
@@ -3276,21 +3282,20 @@ void PeerManagerImpl::ProcessMessage(
         }
 
         int64_t nTime;
-        CAddress addrMe;
+        CService addrMe;
-        CAddress addrFrom;
         uint64_t nNonce = 1;
-        uint64_t nServiceInt;
         ServiceFlags nServices;
         int nVersion;
         std::string cleanSubVer;
         int starting_height = -1;
         bool fRelay = true;
 
-        vRecv >> nVersion >> nServiceInt >> nTime >> addrMe;
+        vRecv >> nVersion >> Using<CustomUintFormatter<8>>(nServices) >> nTime;
         if (nTime < 0) {
             nTime = 0;
         }
-        nServices = ServiceFlags(nServiceInt);
+        vRecv.ignore(8); // Ignore the addrMe service bits sent by the peer
+        vRecv >> addrMe;
         if (!pfrom.IsInboundConn())
         {
             m_addrman.SetServices(pfrom.addr, nServices);
@@ -3309,8 +3314,14 @@ void PeerManagerImpl::ProcessMessage(
             return;
         }
 
-        if (!vRecv.empty())
+        if (!vRecv.empty()) {
-            vRecv >> addrFrom >> nNonce;
+            // The version message includes information about the sending node which we don't use:
+            //   - 8 bytes (service bits)
+            //   - 16 bytes (ipv6 address)
+            //   - 2 bytes (port)
+            vRecv.ignore(26);
+            vRecv >> nNonce;
+        }
         if (!vRecv.empty()) {
             std::string strSubVer;
             vRecv >> LIMITED_STRING(strSubVer, MAX_SUBVERSION_LENGTH);
@@ -3437,6 +3448,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);
@@ -4131,7 +4146,7 @@ void PeerManagerImpl::ProcessMessage(
                 }
             }
 
-            pfrom.nLastTXTime = GetTime();
+            pfrom.m_last_tx_time = GetTime<std::chrono::seconds>();
 
             LogPrint(BCLog::MEMPOOL, "AcceptToMemoryPool: peer=%d: accepted %s (poolsz %u txn, %u kB)\n",
                      pfrom.GetId(),
@@ -4999,26 +5014,22 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic<bool>& interrupt
     CNetMessage& msg(msgs.front());
 
     if (gArgs.GetBoolArg("-capturemessages", false)) {
-        CaptureMessage(pfrom->addr, msg.m_command, MakeUCharSpan(msg.m_recv), /* incoming */ true);
+        CaptureMessage(pfrom->addr, msg.m_type, MakeUCharSpan(msg.m_recv), /* incoming */ true);
     }
 
     msg.SetVersion(pfrom->GetCommonVersion());
-    const std::string& msg_type = msg.m_command;
-
-    // Message size
-    unsigned int nMessageSize = msg.m_message_size;
 
     try {
-        ProcessMessage(*pfrom, msg_type, msg.m_recv, msg.m_time, interruptMsgProc);
+        ProcessMessage(*pfrom, msg.m_type, msg.m_recv, msg.m_time, interruptMsgProc);
         if (interruptMsgProc) return false;
         {
             LOCK(peer->m_getdata_requests_mutex);
             if (!peer->m_getdata_requests.empty()) fMoreWork = true;
         }
     } catch (const std::exception& e) {
-        LogPrint(BCLog::NET, "%s(%s, %u bytes): Exception '%s' (%s) caught\n", __func__, SanitizeString(msg_type), nMessageSize, e.what(), typeid(e).name());
+        LogPrint(BCLog::NET, "%s(%s, %u bytes): Exception '%s' (%s) caught\n", __func__, SanitizeString(msg.m_type), msg.m_message_size, e.what(), typeid(e).name());
     } catch (...) {
-        LogPrint(BCLog::NET, "%s(%s, %u bytes): Unknown exception caught\n", __func__, SanitizeString(msg_type), nMessageSize);
+        LogPrint(BCLog::NET, "%s(%s, %u bytes): Unknown exception caught\n", __func__, SanitizeString(msg.m_type), msg.m_message_size);
     }
 
     return fMoreWork;
@@ -5082,7 +5093,7 @@ void PeerManagerImpl::ConsiderEviction(CNode& pto, int64_t time_in_seconds)
     }
 }
 
-void PeerManagerImpl::EvictExtraOutboundPeers(int64_t time_in_seconds)
+void PeerManagerImpl::EvictExtraOutboundPeers(std::chrono::seconds now)
 {
     // If we have any extra block-relay-only peers, disconnect the youngest unless
     // it's given us a block -- in which case, compare with the second-youngest, and
@@ -5091,14 +5102,14 @@ void PeerManagerImpl::EvictExtraOutboundPeers(int64_t time_in_seconds)
     // to temporarily in order to sync our tip; see net.cpp.
     // Note that we use higher nodeid as a measure for most recent connection.
     if (m_connman.GetExtraBlockRelayCount() > 0) {
-        std::pair<NodeId, int64_t> youngest_peer{-1, 0}, next_youngest_peer{-1, 0};
+        std::pair<NodeId, std::chrono::seconds> youngest_peer{-1, 0}, next_youngest_peer{-1, 0};
 
         m_connman.ForEachNode([&](CNode* pnode) {
             if (!pnode->IsBlockOnlyConn() || pnode->fDisconnect) return;
             if (pnode->GetId() > youngest_peer.first) {
                 next_youngest_peer = youngest_peer;
                 youngest_peer.first = pnode->GetId();
-                youngest_peer.second = pnode->nLastBlockTime;
+                youngest_peer.second = pnode->m_last_block_time;
             }
         });
         NodeId to_disconnect = youngest_peer.first;
@@ -5116,13 +5127,14 @@ void PeerManagerImpl::EvictExtraOutboundPeers(int64_t time_in_seconds)
             // valid headers chain with at least as much work as our tip.
             CNodeState *node_state = State(pnode->GetId());
             if (node_state == nullptr ||
-                (time_in_seconds - pnode->nTimeConnected >= MINIMUM_CONNECT_TIME && node_state->nBlocksInFlight == 0)) {
+                (now - pnode->m_connected >= MINIMUM_CONNECT_TIME && node_state->nBlocksInFlight == 0)) {
                 pnode->fDisconnect = true;
-                LogPrint(BCLog::NET, "disconnecting extra block-relay-only peer=%d (last block received at time %d)\n", pnode->GetId(), pnode->nLastBlockTime);
+                LogPrint(BCLog::NET, "disconnecting extra block-relay-only peer=%d (last block received at time %d)\n",
+                         pnode->GetId(), count_seconds(pnode->m_last_block_time));
                 return true;
             } else {
                 LogPrint(BCLog::NET, "keeping block-relay-only peer=%d chosen for eviction (connect time: %d, blocks_in_flight: %d)\n",
-                    pnode->GetId(), pnode->nTimeConnected, node_state->nBlocksInFlight);
+                         pnode->GetId(), count_seconds(pnode->m_connected), node_state->nBlocksInFlight);
             }
             return false;
         });
@@ -5164,12 +5176,13 @@ void PeerManagerImpl::EvictExtraOutboundPeers(int64_t time_in_seconds)
                 // Also don't disconnect any peer we're trying to download a
                 // block from.
                 CNodeState &state = *State(pnode->GetId());
-                if (time_in_seconds - pnode->nTimeConnected > MINIMUM_CONNECT_TIME && state.nBlocksInFlight == 0) {
+                if (now - pnode->m_connected > MINIMUM_CONNECT_TIME && state.nBlocksInFlight == 0) {
                     LogPrint(BCLog::NET, "disconnecting extra outbound peer=%d (last block announcement received at time %d)\n", pnode->GetId(), oldest_block_announcement);
                     pnode->fDisconnect = true;
                     return true;
                 } else {
-                    LogPrint(BCLog::NET, "keeping outbound peer=%d chosen for eviction (connect time: %d, blocks_in_flight: %d)\n", pnode->GetId(), pnode->nTimeConnected, state.nBlocksInFlight);
+                    LogPrint(BCLog::NET, "keeping outbound peer=%d chosen for eviction (connect time: %d, blocks_in_flight: %d)\n",
+                             pnode->GetId(), count_seconds(pnode->m_connected), state.nBlocksInFlight);
                     return false;
                 }
             });
@@ -5189,20 +5202,20 @@ void PeerManagerImpl::CheckForStaleTipAndEvictPeers()
 {
     LOCK(cs_main);
 
-    int64_t time_in_seconds = GetTime();
+    auto now{GetTime<std::chrono::seconds>()};
 
-    EvictExtraOutboundPeers(time_in_seconds);
+    EvictExtraOutboundPeers(now);
 
-    if (time_in_seconds > m_stale_tip_check_time) {
+    if (now > m_stale_tip_check_time) {
         // Check whether our tip is stale, and if so, allow using an extra
         // outbound peer
         if (!fImporting && !fReindex && m_connman.GetNetworkActive() && m_connman.GetUseAddrmanOutgoing() && TipMayBeStale()) {
-            LogPrintf("Potential stale tip detected, will try using extra outbound peer (last tip update: %d seconds ago)\n", time_in_seconds - m_last_tip_update);
+            LogPrintf("Potential stale tip detected, will try using extra outbound peer (last tip update: %d seconds ago)\n", count_seconds(now) - count_seconds(m_last_tip_update));
             m_connman.SetTryNewOutboundPeer(true);
         } else if (m_connman.GetTryNewOutboundPeer()) {
             m_connman.SetTryNewOutboundPeer(false);
         }
-        m_stale_tip_check_time = time_in_seconds + STALE_CHECK_INTERVAL;
+        m_stale_tip_check_time = now + STALE_CHECK_INTERVAL;
     }
 
     if (!m_initial_sync_finished && CanDirectFetch()) {
@@ -5374,7 +5387,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
 
     const auto current_time = GetTime<std::chrono::microseconds>();
 
-    if (pto->IsAddrFetchConn() && current_time - std::chrono::seconds(pto->nTimeConnected) > 10 * AVG_ADDRESS_BROADCAST_INTERVAL) {
+    if (pto->IsAddrFetchConn() && current_time - pto->m_connected > 10 * AVG_ADDRESS_BROADCAST_INTERVAL) {
         LogPrint(BCLog::NET_NETCONN, "addrfetch connection timeout; disconnecting peer=%d\n", pto->GetId());
         pto->fDisconnect = true;
         return true;

src/net_processing.h

@@ -116,11 +116,6 @@ public:
                                 const std::chrono::microseconds time_received, const std::atomic<bool>& interruptMsgProc) = 0;
 
     virtual bool IsBanned(NodeId pnode) = 0;
-
-    /** Whether we've completed initial sync yet, for determining when to turn
-      * on extra block-relay-only peers. */
-    bool m_initial_sync_finished{false};
-
 };
 
 #endif // BITCOIN_NET_PROCESSING_H

src/protocol.h

@@ -431,7 +431,6 @@ public:
         // ambiguous what that would mean. Make sure no code relying on that is introduced:
         assert(!(s.GetType() & SER_GETHASH));
         bool use_v2;
-        bool store_time;
         if (s.GetType() & SER_DISK) {
             // In the disk serialization format, the encoding (v1 or v2) is determined by a flag version
             // that's part of the serialization itself. ADDRV2_FORMAT in the stream version only determines
@@ -448,24 +447,16 @@ public:
             } else {
                 throw std::ios_base::failure("Unsupported CAddress disk format version");
             }
-            store_time = true;
         } else {
             // In the network serialization format, the encoding (v1 or v2) is determined directly by
             // the value of ADDRV2_FORMAT in the stream version, as no explicitly encoded version
             // exists in the stream.
             assert(s.GetType() & SER_NETWORK);
             use_v2 = s.GetVersion() & ADDRV2_FORMAT;
-            // The only time we serialize a CAddress object without nTime is in
-            // the initial VERSION messages which contain two CAddress records.
-            // At that point, the serialization version is INIT_PROTO_VERSION.
-            // After the version handshake, serialization version is >=
-            // MIN_PEER_PROTO_VERSION and all ADDR messages are serialized with
-            // nTime.
-            store_time = s.GetVersion() != INIT_PROTO_VERSION;
         }
 
         SER_READ(obj, obj.nTime = TIME_INIT);
-        if (store_time) READWRITE(obj.nTime);
+        READWRITE(obj.nTime);
         // nServices is serialized as CompactSize in V2; as uint64_t in V1.
         if (use_v2) {
             uint64_t services_tmp;

src/qt/rpcconsole.cpp

@@ -1285,9 +1285,9 @@ void RPCConsole::updateDetailWidget()
     ui->peerHeading->setText(peerAddrDetails);
     ui->peerServices->setText(GUIUtil::formatServicesStr(stats->nodeStats.nServices));
     const auto time_now{GetTime<std::chrono::seconds>()};
-    ui->peerConnTime->setText(GUIUtil::formatDurationStr(time_now - std::chrono::seconds{stats->nodeStats.nTimeConnected}));
+    ui->peerConnTime->setText(GUIUtil::formatDurationStr(time_now - stats->nodeStats.m_connected));
-    ui->peerLastBlock->setText(TimeDurationField(time_now, std::chrono::seconds{stats->nodeStats.nLastBlockTime}));
+    ui->peerLastBlock->setText(TimeDurationField(time_now, stats->nodeStats.m_last_block_time));
-    ui->peerLastTx->setText(TimeDurationField(time_now, std::chrono::seconds{stats->nodeStats.nLastTXTime}));
+    ui->peerLastTx->setText(TimeDurationField(time_now, stats->nodeStats.m_last_tx_time));
     QString bip152_hb_settings;
     if (stats->nodeStats.m_bip152_highbandwidth_to) bip152_hb_settings += "To";
     if (stats->nodeStats.m_bip152_highbandwidth_from) bip152_hb_settings += (bip152_hb_settings == "" ? "From" : "/From");

src/rpc/net.cpp

@@ -177,7 +177,7 @@ static RPCHelpMan getpeerinfo()
                     {
                         {RPCResult::Type::NUM, "msg", "The total bytes received aggregated by message type\n"
                                                       "When a message type is not listed in this json object, the bytes received are 0.\n"
-                                                      "Only known message types can appear as keys in the object and all bytes received of unknown message types are listed under '"+NET_MESSAGE_COMMAND_OTHER+"'."}
+                                                      "Only known message types can appear as keys in the object and all bytes received of unknown message types are listed under '"+NET_MESSAGE_TYPE_OTHER+"'."}
                     }},
                     {RPCResult::Type::STR, "connection_type", "Type of connection: \n" + Join(CONNECTION_TYPE_DOC, ",\n") + ".\n"
                                                                "Please note this output is unlikely to be stable in upcoming releases as we iterate to\n"
@@ -225,11 +225,11 @@ static RPCHelpMan getpeerinfo()
         obj.pushKV("servicesnames", GetServicesNames(stats.nServices));
         obj.pushKV("lastsend", count_seconds(stats.m_last_send));
         obj.pushKV("lastrecv", count_seconds(stats.m_last_recv));
-        obj.pushKV("last_transaction", stats.nLastTXTime);
+        obj.pushKV("last_transaction", count_seconds(stats.m_last_tx_time));
-        obj.pushKV("last_block", stats.nLastBlockTime);
+        obj.pushKV("last_block", count_seconds(stats.m_last_block_time));
         obj.pushKV("bytessent", stats.nSendBytes);
         obj.pushKV("bytesrecv", stats.nRecvBytes);
-        obj.pushKV("conntime", stats.nTimeConnected);
+        obj.pushKV("conntime", count_seconds(stats.m_connected));
         obj.pushKV("timeoffset", stats.nTimeOffset);
         if (stats.m_last_ping_time > 0us) {
             obj.pushKV("pingtime", CountSecondsDouble(stats.m_last_ping_time));
@@ -281,19 +281,19 @@ static RPCHelpMan getpeerinfo()
         }
         obj.pushKV("permissions", permissions);
 
-        UniValue sendPerMsgCmd(UniValue::VOBJ);
+        UniValue sendPerMsgType(UniValue::VOBJ);
-        for (const auto& i : stats.mapSendBytesPerMsgCmd) {
+        for (const auto& i : stats.mapSendBytesPerMsgType) {
             if (i.second > 0)
-                sendPerMsgCmd.pushKV(i.first, i.second);
+                sendPerMsgType.pushKV(i.first, i.second);
         }
-        obj.pushKV("bytessent_per_msg", sendPerMsgCmd);
+        obj.pushKV("bytessent_per_msg", sendPerMsgType);
 
-        UniValue recvPerMsgCmd(UniValue::VOBJ);
+        UniValue recvPerMsgType(UniValue::VOBJ);
-        for (const auto& i : stats.mapRecvBytesPerMsgCmd) {
+        for (const auto& i : stats.mapRecvBytesPerMsgType) {
             if (i.second > 0)
-                recvPerMsgCmd.pushKV(i.first, i.second);
+                recvPerMsgType.pushKV(i.first, i.second);
         }
-        obj.pushKV("bytesrecv_per_msg", recvPerMsgCmd);
+        obj.pushKV("bytesrecv_per_msg", recvPerMsgType);
         obj.pushKV("connection_type", ConnectionTypeAsString(stats.m_conn_type));
 
         ret.push_back(obj);

src/test/denialofservice_tests.cpp

@@ -130,7 +130,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
     peerLogic->FinalizeNode(dummyNode1);
 }
 
-static void AddRandomOutboundPeer(std::vector<CNode*>& vNodes, PeerManager& peerLogic, ConnmanTestMsg& connman)
+static void AddRandomOutboundPeer(std::vector<CNode*>& vNodes, PeerManager& peerLogic, ConnmanTestMsg& connman, ConnectionType connType)
 {
     CAddress addr(ip(g_insecure_rand_ctx.randbits(32)), NODE_NONE);
     vNodes.emplace_back(new CNode{id++,
@@ -141,7 +141,7 @@ static void AddRandomOutboundPeer(std::vector<CNode*>& vNodes, PeerManager& peer
                                   /*nLocalHostNonceIn=*/0,
                                   CAddress(),
                                   /*addrNameIn=*/"",
-                                  ConnectionType::OUTBOUND_FULL_RELAY,
+                                  connType,
                                   /*inbound_onion=*/false});
     CNode &node = *vNodes.back();
     node.SetCommonVersion(PROTOCOL_VERSION);
@@ -167,12 +167,15 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management)
     options.m_max_outbound_full_relay = max_outbound_full_relay;
     options.nMaxFeeler = MAX_FEELER_CONNECTIONS;
 
+    const auto time_init{GetTime<std::chrono::seconds>()};
+    SetMockTime(time_init);
+    const auto time_later{time_init + 3 * std::chrono::seconds{chainparams.GetConsensus().nPowTargetSpacing} + 1s};
     connman->Init(options);
     std::vector<CNode *> vNodes;
 
     // Mock some outbound peers
     for (int i = 0; i < max_outbound_full_relay; ++i) {
-        AddRandomOutboundPeer(vNodes, *peerLogic, *connman);
+        AddRandomOutboundPeer(vNodes, *peerLogic, *connman, ConnectionType::OUTBOUND_FULL_RELAY);
     }
 
     peerLogic->CheckForStaleTipAndEvictPeers();
@@ -182,7 +185,7 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management)
         BOOST_CHECK(node->fDisconnect == false);
     }
 
-    SetMockTime(GetTime() + 3 * chainparams.GetConsensus().nPowTargetSpacing + 1);
+    SetMockTime(time_later);
 
     // Now tip should definitely be stale, and we should look for an extra
     // outbound peer
@@ -197,7 +200,9 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management)
     // If we add one more peer, something should get marked for eviction
     // on the next check (since we're mocking the time to be in the future, the
     // required time connected check should be satisfied).
-    AddRandomOutboundPeer(vNodes, *peerLogic, *connman);
+    SetMockTime(time_init);
+    AddRandomOutboundPeer(vNodes, *peerLogic, *connman, ConnectionType::OUTBOUND_FULL_RELAY);
+    SetMockTime(time_later);
 
     peerLogic->CheckForStaleTipAndEvictPeers();
     for (int i = 0; i < max_outbound_full_relay; ++i) {
@@ -226,70 +231,185 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management)
     connman->ClearTestNodes();
 }
 
+BOOST_AUTO_TEST_CASE(block_relay_only_eviction)
+{
+    const CChainParams& chainparams = Params();
+    auto connman = std::make_unique<ConnmanTestMsg>(0x1337, 0x1337, *m_node.addrman);
+    auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, nullptr, *m_node.scheduler,
+                                       *m_node.chainman, *m_node.mempool, *m_node.mn_metaman, *m_node.mn_sync,
+                                       *m_node.govman, *m_node.sporkman, /* mn_activeman = */ nullptr, m_node.dmnman,
+                                       m_node.cj_ctx, m_node.llmq_ctx, /* ignore_incoming_txs = */ false);
+
+    constexpr int max_outbound_block_relay{MAX_BLOCK_RELAY_ONLY_CONNECTIONS};
+    constexpr int64_t MINIMUM_CONNECT_TIME{30};
+    CConnman::Options options;
+    options.nMaxConnections = DEFAULT_MAX_PEER_CONNECTIONS;
+    options.m_max_outbound_full_relay = MAX_OUTBOUND_FULL_RELAY_CONNECTIONS;
+    options.m_max_outbound_block_relay = max_outbound_block_relay;
+
+    connman->Init(options);
+    std::vector<CNode*> vNodes;
+
+    // Add block-relay-only peers up to the limit
+    for (int i = 0; i < max_outbound_block_relay; ++i) {
+        AddRandomOutboundPeer(vNodes, *peerLogic, *connman, ConnectionType::BLOCK_RELAY);
+    }
+    peerLogic->CheckForStaleTipAndEvictPeers();
+
+    for (int i = 0; i < max_outbound_block_relay; ++i) {
+        BOOST_CHECK(vNodes[i]->fDisconnect == false);
+    }
+
+    // Add an extra block-relay-only peer breaking the limit (mocks logic in ThreadOpenConnections)
+    AddRandomOutboundPeer(vNodes, *peerLogic, *connman, ConnectionType::BLOCK_RELAY);
+    peerLogic->CheckForStaleTipAndEvictPeers();
+
+    // The extra peer should only get marked for eviction after MINIMUM_CONNECT_TIME
+    for (int i = 0; i < max_outbound_block_relay; ++i) {
+        BOOST_CHECK(vNodes[i]->fDisconnect == false);
+    }
+    BOOST_CHECK(vNodes.back()->fDisconnect == false);
+
+    SetMockTime(GetTime() + MINIMUM_CONNECT_TIME + 1);
+    peerLogic->CheckForStaleTipAndEvictPeers();
+    for (int i = 0; i < max_outbound_block_relay; ++i) {
+        BOOST_CHECK(vNodes[i]->fDisconnect == false);
+    }
+    BOOST_CHECK(vNodes.back()->fDisconnect == true);
+
+    // Update the last block time for the extra peer,
+    // and check that the next youngest peer gets evicted.
+    vNodes.back()->fDisconnect = false;
+    vNodes.back()->m_last_block_time = GetTime<std::chrono::seconds>();
+
+    peerLogic->CheckForStaleTipAndEvictPeers();
+    for (int i = 0; i < max_outbound_block_relay - 1; ++i) {
+        BOOST_CHECK(vNodes[i]->fDisconnect == false);
+    }
+    BOOST_CHECK(vNodes[max_outbound_block_relay - 1]->fDisconnect == true);
+    BOOST_CHECK(vNodes.back()->fDisconnect == false);
+
+    for (const CNode* node : vNodes) {
+        peerLogic->FinalizeNode(*node);
+    }
+    connman->ClearTestNodes();
+}
+
 BOOST_AUTO_TEST_CASE(peer_discouragement)
 {
     const CChainParams& chainparams = Params();
     auto banman = std::make_unique<BanMan>(m_args.GetDataDirPath() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME);
-    auto connman = std::make_unique<CConnman>(0x1337, 0x1337, *m_node.addrman);
+    auto connman = std::make_unique<ConnmanTestMsg>(0x1337, 0x1337, *m_node.addrman);
     auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, banman.get(), *m_node.scheduler,
                                        *m_node.chainman, *m_node.mempool, *m_node.mn_metaman, *m_node.mn_sync,
                                        *m_node.govman, *m_node.sporkman, /* mn_activeman = */ nullptr, m_node.dmnman,
                                        m_node.cj_ctx, m_node.llmq_ctx, /* ignore_incoming_txs = */ false);
 
+    CNetAddr tor_netaddr;
+    BOOST_REQUIRE(
+        tor_netaddr.SetSpecial("pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion"));
+    const CService tor_service{tor_netaddr, Params().GetDefaultPort()};
+
+    const std::array<CAddress, 3> addr{CAddress{ip(0xa0b0c001), NODE_NONE},
+                                       CAddress{ip(0xa0b0c002), NODE_NONE},
+                                       CAddress{tor_service, NODE_NONE}};
+
+    const CNetAddr other_addr{ip(0xa0b0ff01)}; // Not any of addr[].
+
+    std::array<CNode*, 3> nodes;
+
     banman->ClearBanned();
-    CAddress addr1(ip(0xa0b0c001), NODE_NONE);
+    nodes[0] = new CNode{id++,
-    CNode dummyNode1{id++,
+                         NODE_NETWORK,
-                     NODE_NETWORK,
+                         /*sock=*/nullptr,
-                     /*sock=*/nullptr,
+                         addr[0],
-                     addr1,
+                         /*nKeyedNetGroupIn=*/0,
-                     /*nKeyedNetGroupIn=*/0,
+                         /*nLocalHostNonceIn=*/0,
-                     /*nLocalHostNonceIn=*/0,
+                         CAddress(),
-                     CAddress(),
+                         /*addrNameIn=*/"",
-                     /*addrNameIn=*/"",
+                         ConnectionType::INBOUND,
-                     ConnectionType::INBOUND,
+                         /*inbound_onion=*/false};
-                     /*inbound_onion=*/false};
+    nodes[0]->SetCommonVersion(PROTOCOL_VERSION);
-    dummyNode1.SetCommonVersion(PROTOCOL_VERSION);
+    peerLogic->InitializeNode(nodes[0]);
-    peerLogic->InitializeNode(&dummyNode1);
+    nodes[0]->fSuccessfullyConnected = true;
-    dummyNode1.fSuccessfullyConnected = true;
+    connman->AddTestNode(*nodes[0]);
-    peerLogic->Misbehaving(dummyNode1.GetId(), DISCOURAGEMENT_THRESHOLD); // Should be discouraged
+    peerLogic->Misbehaving(nodes[0]->GetId(), DISCOURAGEMENT_THRESHOLD); // Should be discouraged
     {
-        LOCK(dummyNode1.cs_sendProcessing);
+        LOCK(nodes[0]->cs_sendProcessing);
-        BOOST_CHECK(peerLogic->SendMessages(&dummyNode1));
+        BOOST_CHECK(peerLogic->SendMessages(nodes[0]));
     }
-    BOOST_CHECK(banman->IsDiscouraged(addr1));
+    BOOST_CHECK(banman->IsDiscouraged(addr[0]));
-    BOOST_CHECK(!banman->IsDiscouraged(ip(0xa0b0c001|0x0000ff00))); // Different IP, not discouraged
+    BOOST_CHECK(nodes[0]->fDisconnect);
+    BOOST_CHECK(!banman->IsDiscouraged(other_addr)); // Different address, not discouraged
 
-    CAddress addr2(ip(0xa0b0c002), NODE_NONE);
+    nodes[1] = new CNode{id++,
-    CNode dummyNode2{id++,
+                         NODE_NETWORK,
-                     NODE_NETWORK,
+                         /*sock=*/nullptr,
-                     /*sock=*/nullptr,
+                         addr[1],
-                     addr2,
+                         /*nKeyedNetGroupIn=*/1,
-                     /*nKeyedNetGroupIn=*/1,
+                         /*nLocalHostNonceIn=*/1,
-                     /*nLocalHostNonceIn=*/1,
+                         CAddress(),
-                     CAddress(),
+                         /*pszDest=*/"",
-                     /*pszDest=*/"",
+                         ConnectionType::INBOUND,
-                     ConnectionType::INBOUND,
+                         /*inbound_onion=*/false};
-                     /*inbound_onion=*/false};
+    nodes[1]->SetCommonVersion(PROTOCOL_VERSION);
-    dummyNode2.SetCommonVersion(PROTOCOL_VERSION);
+    peerLogic->InitializeNode(nodes[1]);
-    peerLogic->InitializeNode(&dummyNode2);
+    nodes[1]->fSuccessfullyConnected = true;
-    dummyNode2.fSuccessfullyConnected = true;
+    connman->AddTestNode(*nodes[1]);
-    peerLogic->Misbehaving(dummyNode2.GetId(), DISCOURAGEMENT_THRESHOLD - 1);
+    peerLogic->Misbehaving(nodes[1]->GetId(), DISCOURAGEMENT_THRESHOLD - 1);
     {
-        LOCK(dummyNode2.cs_sendProcessing);
+        LOCK(nodes[1]->cs_sendProcessing);
-        BOOST_CHECK(peerLogic->SendMessages(&dummyNode2));
+        BOOST_CHECK(peerLogic->SendMessages(nodes[1]));
     }
-    BOOST_CHECK(!banman->IsDiscouraged(addr2)); // 2 not discouraged yet...
+    // [0] is still discouraged/disconnected.
-    BOOST_CHECK(banman->IsDiscouraged(addr1));  // ... but 1 still should be
+    BOOST_CHECK(banman->IsDiscouraged(addr[0]));
-    peerLogic->Misbehaving(dummyNode2.GetId(), 1); // 2 reaches discouragement threshold
+    BOOST_CHECK(nodes[0]->fDisconnect);
+    // [1] is not discouraged/disconnected yet.
+    BOOST_CHECK(!banman->IsDiscouraged(addr[1]));
+    BOOST_CHECK(!nodes[1]->fDisconnect);
+    peerLogic->Misbehaving(nodes[1]->GetId(), 1); // [1] reaches discouragement threshold
     {
-        LOCK(dummyNode2.cs_sendProcessing);
+        LOCK(nodes[1]->cs_sendProcessing);
-        BOOST_CHECK(peerLogic->SendMessages(&dummyNode2));
+        BOOST_CHECK(peerLogic->SendMessages(nodes[1]));
     }
-    BOOST_CHECK(banman->IsDiscouraged(addr1));  // Expect both 1 and 2
+    // Expect both [0] and [1] to be discouraged/disconnected now.
-    BOOST_CHECK(banman->IsDiscouraged(addr2));  // to be discouraged now
+    BOOST_CHECK(banman->IsDiscouraged(addr[0]));
+    BOOST_CHECK(nodes[0]->fDisconnect);
+    BOOST_CHECK(banman->IsDiscouraged(addr[1]));
+    BOOST_CHECK(nodes[1]->fDisconnect);
 
-    peerLogic->FinalizeNode(dummyNode1);
+    // Make sure non-IP peers are discouraged and disconnected properly.
-    peerLogic->FinalizeNode(dummyNode2);
+
+    nodes[2] = new CNode{id++,
+                         NODE_NETWORK,
+                         /*sock=*/nullptr,
+                         addr[2],
+                         /*nKeyedNetGroupIn=*/1,
+                         /*nLocalHostNonceIn=*/1,
+                         CAddress(),
+                         /*pszDest=*/"",
+                         ConnectionType::OUTBOUND_FULL_RELAY,
+                         /*inbound_onion=*/false};
+    nodes[2]->SetCommonVersion(PROTOCOL_VERSION);
+    peerLogic->InitializeNode(nodes[2]);
+    nodes[2]->fSuccessfullyConnected = true;
+    connman->AddTestNode(*nodes[2]);
+    peerLogic->Misbehaving(nodes[2]->GetId(), DISCOURAGEMENT_THRESHOLD, /* message */ "");
+    {
+        LOCK(nodes[2]->cs_sendProcessing);
+        BOOST_CHECK(peerLogic->SendMessages(nodes[2]));
+    }
+    BOOST_CHECK(banman->IsDiscouraged(addr[0]));
+    BOOST_CHECK(banman->IsDiscouraged(addr[1]));
+    BOOST_CHECK(banman->IsDiscouraged(addr[2]));
+    BOOST_CHECK(nodes[0]->fDisconnect);
+    BOOST_CHECK(nodes[1]->fDisconnect);
+    BOOST_CHECK(nodes[2]->fDisconnect);
+
+    for (CNode* node : nodes) {
+        peerLogic->FinalizeNode(*node);
+    }
+    connman->ClearTestNodes();
 }
 
 BOOST_AUTO_TEST_CASE(DoS_bantime)

src/test/fuzz/node_eviction.cpp

@@ -21,10 +21,10 @@ FUZZ_TARGET(node_eviction)
     while (fuzzed_data_provider.ConsumeBool()) {
         eviction_candidates.push_back({
             /* id */ fuzzed_data_provider.ConsumeIntegral<NodeId>(),
-            /* nTimeConnected */ fuzzed_data_provider.ConsumeIntegral<int64_t>(),
+            /* m_connected */ std::chrono::seconds{fuzzed_data_provider.ConsumeIntegral<int64_t>()},
             /* m_min_ping_time */ std::chrono::microseconds{fuzzed_data_provider.ConsumeIntegral<int64_t>()},
-            /* nLastBlockTime */ fuzzed_data_provider.ConsumeIntegral<int64_t>(),
+            /* m_last_block_time */ std::chrono::seconds{fuzzed_data_provider.ConsumeIntegral<int64_t>()},
-            /* nLastTXTime */ fuzzed_data_provider.ConsumeIntegral<int64_t>(),
+            /* m_last_tx_time */ std::chrono::seconds{fuzzed_data_provider.ConsumeIntegral<int64_t>()},
             /* fRelevantServices */ fuzzed_data_provider.ConsumeBool(),
             /* m_relay_txs */ fuzzed_data_provider.ConsumeBool(),
             /* fBloomFilter */ fuzzed_data_provider.ConsumeBool(),

src/test/fuzz/p2p_transport_serialization.cpp

@@ -68,18 +68,16 @@ FUZZ_TARGET_INIT(p2p_transport_serialization, initialize_p2p_transport_serializa
         }
         if (deserializer.Complete()) {
             const std::chrono::microseconds m_time{std::numeric_limits<int64_t>::max()};
-            uint32_t out_err_raw_size{0};
+            bool reject_message{false};
-            std::optional<CNetMessage> result{deserializer.GetMessage(m_time, out_err_raw_size)};
+            CNetMessage msg = deserializer.GetMessage(m_time, reject_message);
-            if (result) {
+            assert(msg.m_type.size() <= CMessageHeader::COMMAND_SIZE);
-                assert(result->m_command.size() <= CMessageHeader::COMMAND_SIZE);
+            assert(msg.m_raw_message_size <= mutable_msg_bytes.size());
-                assert(result->m_raw_message_size <= mutable_msg_bytes.size());
+            assert(msg.m_raw_message_size == CMessageHeader::HEADER_SIZE + msg.m_message_size);
-                assert(result->m_raw_message_size == CMessageHeader::HEADER_SIZE + result->m_message_size);
+            assert(msg.m_time == m_time);
-                assert(result->m_time == m_time);
 
-                std::vector<unsigned char> header;
+            std::vector<unsigned char> header;
-                auto msg = CNetMsgMaker{result->m_recv.GetVersion()}.Make(result->m_command, MakeUCharSpan(result->m_recv));
+            auto msg2 = CNetMsgMaker{msg.m_recv.GetVersion()}.Make(msg.m_type, MakeUCharSpan(msg.m_recv));
-                serializer.prepareForTransport(msg, header);
+            serializer.prepareForTransport(msg2, header);
-            }
         }
     }
 }

src/test/fuzz/process_message.cpp

@@ -87,8 +87,7 @@ void fuzz_target(FuzzBufferType buffer, const std::string& LIMIT_TO_MESSAGE_TYPE
     CNode& p2p_node = *ConsumeNodeAsUniquePtr(fuzzed_data_provider).release();
 
     connman.AddTestNode(p2p_node);
-    g_setup->m_node.peerman->InitializeNode(&p2p_node);
+    FillNode(fuzzed_data_provider, connman, p2p_node);
-    FillNode(fuzzed_data_provider, connman, *g_setup->m_node.peerman, p2p_node);
 
     const auto mock_time = ConsumeTime(fuzzed_data_provider);
     SetMockTime(mock_time);

src/test/fuzz/process_messages.cpp

@@ -45,8 +45,7 @@ FUZZ_TARGET_INIT(process_messages, initialize_process_messages)
         peers.push_back(ConsumeNodeAsUniquePtr(fuzzed_data_provider, i).release());
         CNode& p2p_node = *peers.back();
 
-        g_setup->m_node.peerman->InitializeNode(&p2p_node);
+        FillNode(fuzzed_data_provider, connman, p2p_node);
-        FillNode(fuzzed_data_provider, connman, *g_setup->m_node.peerman, p2p_node);
 
         connman.AddTestNode(p2p_node);
     }

src/test/fuzz/util.cpp

@@ -282,57 +282,14 @@ bool FuzzedSock::IsConnected(std::string& errmsg) const
     return false;
 }
 
-void FillNode(FuzzedDataProvider& fuzzed_data_provider, ConnmanTestMsg& connman, PeerManager& peerman, CNode& node) noexcept
+void FillNode(FuzzedDataProvider& fuzzed_data_provider, ConnmanTestMsg& connman, CNode& node) noexcept
 {
-    const bool successfully_connected{fuzzed_data_provider.ConsumeBool()};
+    connman.Handshake(node,
-    const ServiceFlags remote_services = ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS);
+                      /*successfully_connected=*/fuzzed_data_provider.ConsumeBool(),
-    const NetPermissionFlags permission_flags = ConsumeWeakEnum(fuzzed_data_provider, ALL_NET_PERMISSION_FLAGS);
+                      /*remote_services=*/ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS),
-    const int32_t version = fuzzed_data_provider.ConsumeIntegralInRange<int32_t>(MIN_PEER_PROTO_VERSION, std::numeric_limits<int32_t>::max());
+                      /*permission_flags=*/ConsumeWeakEnum(fuzzed_data_provider, ALL_NET_PERMISSION_FLAGS),
-    const bool relay_txs{fuzzed_data_provider.ConsumeBool()};
+                      /*version=*/fuzzed_data_provider.ConsumeIntegralInRange<int32_t>(MIN_PEER_PROTO_VERSION, std::numeric_limits<int32_t>::max()),
-
+                      /*relay_txs=*/fuzzed_data_provider.ConsumeBool());
-    const CNetMsgMaker mm{0};
-
-    CSerializedNetMsg msg_version{
-        mm.Make(NetMsgType::VERSION,
-                version,                                        //
-                Using<CustomUintFormatter<8>>(remote_services), //
-                int64_t{},                                      // dummy time
-                int64_t{},                                      // ignored service bits
-                CService{},                                     // dummy
-                int64_t{},                                      // ignored service bits
-                CService{},                                     // ignored
-                uint64_t{1},                                    // dummy nonce
-                std::string{},                                  // dummy subver
-                int32_t{},                                      // dummy starting_height
-                relay_txs),
-    };
-
-    (void)connman.ReceiveMsgFrom(node, msg_version);
-    node.fPauseSend = false;
-    connman.ProcessMessagesOnce(node);
-    {
-        LOCK(node.cs_sendProcessing);
-        peerman.SendMessages(&node);
-    }
-    if (node.fDisconnect) return;
-    assert(node.nVersion == version);
-    assert(node.GetCommonVersion() == std::min(version, PROTOCOL_VERSION));
-    assert(node.nServices == remote_services);
-    CNodeStateStats statestats;
-    assert(peerman.GetNodeStateStats(node.GetId(), statestats));
-    assert(statestats.m_relay_txs == (relay_txs && !node.IsBlockOnlyConn()));
-    node.m_permissionFlags = permission_flags;
-    if (successfully_connected) {
-        CSerializedNetMsg msg_verack{mm.Make(NetMsgType::VERACK)};
-        (void)connman.ReceiveMsgFrom(node, msg_verack);
-        node.fPauseSend = false;
-        connman.ProcessMessagesOnce(node);
-        {
-            LOCK(node.cs_sendProcessing);
-            peerman.SendMessages(&node);
-        }
-        assert(node.fSuccessfullyConnected == true);
-    }
 }
 
 int64_t ConsumeTime(FuzzedDataProvider& fuzzed_data_provider, const std::optional<int64_t>& min, const std::optional<int64_t>& max) noexcept

src/test/fuzz/util.h

@@ -396,7 +396,7 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional<N
 }
 inline std::unique_ptr<CNode> ConsumeNodeAsUniquePtr(FuzzedDataProvider& fdp, const std::optional<NodeId>& node_id_in = std::nullopt) { return ConsumeNode<true>(fdp, node_id_in); }
 
-void FillNode(FuzzedDataProvider& fuzzed_data_provider, ConnmanTestMsg& connman, PeerManager& peerman, CNode& node) noexcept;
+void FillNode(FuzzedDataProvider& fuzzed_data_provider, ConnmanTestMsg& connman, CNode& node) noexcept;
 
 class FuzzedFileProvider
 {

src/test/net_peer_eviction_tests.cpp

@@ -17,28 +17,6 @@
 
 BOOST_FIXTURE_TEST_SUITE(net_peer_eviction_tests, BasicTestingSetup)
 
-std::vector<NodeEvictionCandidate> GetRandomNodeEvictionCandidates(const int n_candidates, FastRandomContext& random_context)
-{
-    std::vector<NodeEvictionCandidate> candidates;
-    for (int id = 0; id < n_candidates; ++id) {
-        candidates.push_back({
-            /* id */ id,
-            /* nTimeConnected */ static_cast<int64_t>(random_context.randrange(100)),
-            /* m_min_ping_time */ std::chrono::microseconds{random_context.randrange(100)},
-            /* nLastBlockTime */ static_cast<int64_t>(random_context.randrange(100)),
-            /* nLastTXTime */ static_cast<int64_t>(random_context.randrange(100)),
-            /* fRelevantServices */ random_context.randbool(),
-            /* m_relay_txs */ random_context.randbool(),
-            /* fBloomFilter */ random_context.randbool(),
-            /* nKeyedNetGroup */ random_context.randrange(100),
-            /* prefer_evict */ random_context.randbool(),
-            /* m_is_local */ random_context.randbool(),
-            /* m_network */ ALL_NETWORKS[random_context.randrange(ALL_NETWORKS.size())],
-        });
-    }
-    return candidates;
-}
-
 // Create `num_peers` random nodes, apply setup function `candidate_setup_fn`,
 // call ProtectEvictionCandidatesByRatio() to apply protection logic, and then
 // return true if all of `protected_peer_ids` and none of `unprotected_peer_ids`
@@ -86,11 +64,11 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
     FastRandomContext random_context{true};
     int num_peers{12};
 
-    // Expect half of the peers with greatest uptime (the lowest nTimeConnected)
+    // Expect half of the peers with greatest uptime (the lowest m_connected)
     // to be protected from eviction.
     BOOST_CHECK(IsProtected(
         num_peers, [](NodeEvictionCandidate& c) {
-            c.nTimeConnected = c.id;
+            c.m_connected = std::chrono::seconds{c.id};
             c.m_is_local = false;
             c.m_network = NET_IPV4;
         },
@@ -101,7 +79,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
     // Verify in the opposite direction.
     BOOST_CHECK(IsProtected(
         num_peers, [num_peers](NodeEvictionCandidate& c) {
-            c.nTimeConnected = num_peers - c.id;
+            c.m_connected = std::chrono::seconds{num_peers - c.id};
             c.m_is_local = false;
             c.m_network = NET_IPV6;
         },
@@ -123,10 +101,10 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
         random_context));
 
     // Expect 1/4 onion peers and 1/4 of the other peers to be protected,
-    // sorted by longest uptime (lowest nTimeConnected), if no localhost, I2P or CJDNS peers.
+    // sorted by longest uptime (lowest m_connected), if no localhost, I2P or CJDNS peers.
     BOOST_CHECK(IsProtected(
         num_peers, [](NodeEvictionCandidate& c) {
-            c.nTimeConnected = c.id;
+            c.m_connected = std::chrono::seconds{c.id};
             c.m_is_local = false;
             c.m_network = (c.id == 3 || c.id > 7) ? NET_ONION : NET_IPV6;
         },
@@ -146,10 +124,10 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
         random_context));
 
     // Expect 1/4 localhost peers and 1/4 of the other peers to be protected,
-    // sorted by longest uptime (lowest nTimeConnected), if no onion, I2P, or CJDNS peers.
+    // sorted by longest uptime (lowest m_connected), if no onion, I2P, or CJDNS peers.
     BOOST_CHECK(IsProtected(
         num_peers, [](NodeEvictionCandidate& c) {
-            c.nTimeConnected = c.id;
+            c.m_connected = std::chrono::seconds{c.id};
             c.m_is_local = (c.id > 6);
             c.m_network = NET_IPV6;
         },
@@ -169,10 +147,10 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
         random_context));
 
     // Expect 1/4 I2P peers and 1/4 of the other peers to be protected, sorted
-    // by longest uptime (lowest nTimeConnected), if no onion, localhost, or CJDNS peers.
+    // by longest uptime (lowest m_connected), if no onion, localhost, or CJDNS peers.
     BOOST_CHECK(IsProtected(
         num_peers, [](NodeEvictionCandidate& c) {
-            c.nTimeConnected = c.id;
+            c.m_connected = std::chrono::seconds{c.id};
             c.m_is_local = false;
             c.m_network = (c.id == 4 || c.id > 8) ? NET_I2P : NET_IPV6;
         },
@@ -192,10 +170,10 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
         random_context));
 
     // Expect 1/4 CJDNS peers and 1/4 of the other peers to be protected, sorted
-    // by longest uptime (lowest nTimeConnected), if no onion, localhost, or I2P peers.
+    // by longest uptime (lowest m_connected), if no onion, localhost, or I2P peers.
     BOOST_CHECK(IsProtected(
         num_peers, [](NodeEvictionCandidate& c) {
-            c.nTimeConnected = c.id;
+            c.m_connected = std::chrono::seconds{c.id};
             c.m_is_local = false;
             c.m_network = (c.id == 4 || c.id > 8) ? NET_CJDNS : NET_IPV6;
         },
@@ -210,7 +188,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
     // stable sort breaks tie with array order of localhost first.
     BOOST_CHECK(IsProtected(
         4, [](NodeEvictionCandidate& c) {
-            c.nTimeConnected = c.id;
+            c.m_connected = std::chrono::seconds{c.id};
             c.m_is_local = (c.id == 4);
             c.m_network = (c.id == 3) ? NET_ONION : NET_IPV4;
         },
@@ -223,7 +201,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
     // uptime; stable sort breaks tie with array order of localhost first.
     BOOST_CHECK(IsProtected(
         7, [](NodeEvictionCandidate& c) {
-            c.nTimeConnected = c.id;
+            c.m_connected = std::chrono::seconds{c.id};
             c.m_is_local = (c.id == 6);
             c.m_network = (c.id == 5) ? NET_ONION : NET_IPV4;
         },
@@ -236,7 +214,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
     // by uptime; stable sort breaks tie with array order of localhost first.
     BOOST_CHECK(IsProtected(
         8, [](NodeEvictionCandidate& c) {
-            c.nTimeConnected = c.id;
+            c.m_connected = std::chrono::seconds{c.id};
             c.m_is_local = (c.id == 6);
             c.m_network = (c.id == 5) ? NET_ONION : NET_IPV4;
         },
@@ -249,7 +227,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
     // uptime; stable sort breaks ties with the array order of localhost first.
     BOOST_CHECK(IsProtected(
         num_peers, [](NodeEvictionCandidate& c) {
-            c.nTimeConnected = c.id;
+            c.m_connected = std::chrono::seconds{c.id};
             c.m_is_local = (c.id == 6 || c.id == 9 || c.id == 11);
             c.m_network = (c.id == 7 || c.id == 8 || c.id == 10) ? NET_ONION : NET_IPV6;
         },
@@ -261,7 +239,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
     // protect 2 localhost and 1 onion, plus 3 other peers, sorted by longest uptime.
     BOOST_CHECK(IsProtected(
         num_peers, [](NodeEvictionCandidate& c) {
-            c.nTimeConnected = c.id;
+            c.m_connected = std::chrono::seconds{c.id};
             c.m_is_local = (c.id > 4 && c.id < 9);
             c.m_network = (c.id == 10) ? NET_ONION : NET_IPV4;
         },
@@ -273,7 +251,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
     // protect 2 localhost and 2 onions, plus 4 other peers, sorted by longest uptime.
     BOOST_CHECK(IsProtected(
         16, [](NodeEvictionCandidate& c) {
-            c.nTimeConnected = c.id;
+            c.m_connected = std::chrono::seconds{c.id};
             c.m_is_local = (c.id == 6 || c.id == 9 || c.id == 11 || c.id == 12);
             c.m_network = (c.id == 8 || c.id == 10) ? NET_ONION : NET_IPV6;
         },
@@ -286,7 +264,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
     // others, sorted by longest uptime.
     BOOST_CHECK(IsProtected(
         16, [](NodeEvictionCandidate& c) {
-            c.nTimeConnected = c.id;
+            c.m_connected = std::chrono::seconds{c.id};
             c.m_is_local = (c.id > 10);
             c.m_network = (c.id == 10) ? NET_ONION : NET_IPV4;
         },
@@ -299,7 +277,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
     // plus 4 others, sorted by longest uptime.
     BOOST_CHECK(IsProtected(
         16, [](NodeEvictionCandidate& c) {
-            c.nTimeConnected = c.id;
+            c.m_connected = std::chrono::seconds{c.id};
             c.m_is_local = (c.id == 15);
             c.m_network = (c.id > 6 && c.id < 11) ? NET_ONION : NET_IPV6;
         },
@@ -312,7 +290,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
     // others, sorted by longest uptime.
     BOOST_CHECK(IsProtected(
         num_peers, [](NodeEvictionCandidate& c) {
-            c.nTimeConnected = c.id;
+            c.m_connected = std::chrono::seconds{c.id};
             c.m_is_local = false;
             if (c.id == 8 || c.id == 10) {
                 c.m_network = NET_ONION;
@@ -333,7 +311,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
     // by longest uptime; stable sort breaks tie with array order of I2P first.
     BOOST_CHECK(IsProtected(
         4, [](NodeEvictionCandidate& c) {
-            c.nTimeConnected = c.id;
+            c.m_connected = std::chrono::seconds{c.id};
             c.m_is_local = (c.id == 2);
             if (c.id == 3) {
                 c.m_network = NET_I2P;
@@ -352,7 +330,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
     // by longest uptime; stable sort breaks tie with array order of I2P first.
     BOOST_CHECK(IsProtected(
         7, [](NodeEvictionCandidate& c) {
-            c.nTimeConnected = c.id;
+            c.m_connected = std::chrono::seconds{c.id};
             c.m_is_local = (c.id == 4);
             if (c.id == 6) {
                 c.m_network = NET_I2P;
@@ -371,7 +349,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
     // by uptime; stable sort breaks tie with array order of I2P then localhost.
     BOOST_CHECK(IsProtected(
         8, [](NodeEvictionCandidate& c) {
-            c.nTimeConnected = c.id;
+            c.m_connected = std::chrono::seconds{c.id};
             c.m_is_local = (c.id == 6);
             if (c.id == 5) {
                 c.m_network = NET_I2P;
@@ -390,7 +368,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
     // for 8 total, sorted by longest uptime.
     BOOST_CHECK(IsProtected(
         16, [](NodeEvictionCandidate& c) {
-            c.nTimeConnected = c.id;
+            c.m_connected = std::chrono::seconds{c.id};
             c.m_is_local = (c.id == 6 || c.id > 11);
             if (c.id == 7 || c.id == 11) {
                 c.m_network = NET_I2P;
@@ -409,7 +387,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
     // sorted by longest uptime.
     BOOST_CHECK(IsProtected(
         24, [](NodeEvictionCandidate& c) {
-            c.nTimeConnected = c.id;
+            c.m_connected = std::chrono::seconds{c.id};
             c.m_is_local = (c.id == 12);
             if (c.id > 14 && c.id < 23) { // 4 protected instead of usual 2
                 c.m_network = NET_I2P;
@@ -428,7 +406,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
     // unused localhost slot), plus 6 others for 12/24 total, sorted by longest uptime.
     BOOST_CHECK(IsProtected(
         24, [](NodeEvictionCandidate& c) {
-            c.nTimeConnected = c.id;
+            c.m_connected = std::chrono::seconds{c.id};
             c.m_is_local = (c.id == 15);
             if (c.id == 12 || c.id == 14 || c.id == 17) {
                 c.m_network = NET_I2P;
@@ -447,7 +425,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
     // for 12/24 total, sorted by longest uptime.
     BOOST_CHECK(IsProtected(
         24, [](NodeEvictionCandidate& c) {
-            c.nTimeConnected = c.id;
+            c.m_connected = std::chrono::seconds{c.id};
             c.m_is_local = (c.id == 13);
             if (c.id > 16) {
                 c.m_network = NET_I2P;
@@ -466,7 +444,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
     // sorted by longest uptime.
     BOOST_CHECK(IsProtected(
         24, [](NodeEvictionCandidate& c) {
-            c.nTimeConnected = c.id;
+            c.m_connected = std::chrono::seconds{c.id};
             c.m_is_local = (c.id > 15);
             if (c.id > 10 && c.id < 15) {
                 c.m_network = NET_CJDNS;
@@ -488,7 +466,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
     // order of CJDNS first.
     BOOST_CHECK(IsProtected(
         5, [](NodeEvictionCandidate& c) {
-            c.nTimeConnected = c.id;
+            c.m_connected = std::chrono::seconds{c.id};
             c.m_is_local = (c.id == 3);
             if (c.id == 4) {
                 c.m_network = NET_CJDNS;
@@ -510,7 +488,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
     // array order of CJDNS first.
     BOOST_CHECK(IsProtected(
         7, [](NodeEvictionCandidate& c) {
-            c.nTimeConnected = c.id;
+            c.m_connected = std::chrono::seconds{c.id};
             c.m_is_local = (c.id == 4);
             if (c.id == 6) {
                 c.m_network = NET_CJDNS;
@@ -532,7 +510,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
     // array order of CJDNS first.
     BOOST_CHECK(IsProtected(
         8, [](NodeEvictionCandidate& c) {
-            c.nTimeConnected = c.id;
+            c.m_connected = std::chrono::seconds{c.id};
             c.m_is_local = (c.id == 3);
             if (c.id == 5) {
                 c.m_network = NET_CJDNS;
@@ -553,7 +531,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
     // total), plus 4 others for 8 total, sorted by longest uptime.
     BOOST_CHECK(IsProtected(
         16, [](NodeEvictionCandidate& c) {
-            c.nTimeConnected = c.id;
+            c.m_connected = std::chrono::seconds{c.id};
             c.m_is_local = (c.id > 5);
             if (c.id == 11 || c.id == 15) {
                 c.m_network = NET_CJDNS;
@@ -574,7 +552,7 @@ BOOST_AUTO_TEST_CASE(peer_protection_test)
     // total), plus 6 others for 12/24 total, sorted by longest uptime.
     BOOST_CHECK(IsProtected(
         24, [](NodeEvictionCandidate& c) {
-            c.nTimeConnected = c.id;
+            c.m_connected = std::chrono::seconds{c.id};
             c.m_is_local = (c.id == 13);
             if (c.id > 17) {
                 c.m_network = NET_CJDNS;
@@ -639,7 +617,7 @@ BOOST_AUTO_TEST_CASE(peer_eviction_test)
         // into our mempool should be protected from eviction.
         BOOST_CHECK(!IsEvicted(
                         number_of_nodes, [number_of_nodes](NodeEvictionCandidate& candidate) {
-                            candidate.nLastTXTime = number_of_nodes - candidate.id;
+                            candidate.m_last_tx_time = std::chrono::seconds{number_of_nodes - candidate.id};
                         },
                         {0, 1, 2, 3}, random_context));
 
@@ -647,7 +625,7 @@ BOOST_AUTO_TEST_CASE(peer_eviction_test)
         // blocks should be protected from eviction.
         BOOST_CHECK(!IsEvicted(
                         number_of_nodes, [number_of_nodes](NodeEvictionCandidate& candidate) {
-                            candidate.nLastBlockTime = number_of_nodes - candidate.id;
+                            candidate.m_last_block_time = std::chrono::seconds{number_of_nodes - candidate.id};
                             if (candidate.id <= 7) {
                                 candidate.m_relay_txs = false;
                                 candidate.fRelevantServices = true;
@@ -659,14 +637,14 @@ BOOST_AUTO_TEST_CASE(peer_eviction_test)
         // protected from eviction.
         BOOST_CHECK(!IsEvicted(
                         number_of_nodes, [number_of_nodes](NodeEvictionCandidate& candidate) {
-                            candidate.nLastBlockTime = number_of_nodes - candidate.id;
+                            candidate.m_last_block_time = std::chrono::seconds{number_of_nodes - candidate.id};
                         },
                         {0, 1, 2, 3}, random_context));
 
         // Combination of the previous two tests.
         BOOST_CHECK(!IsEvicted(
                         number_of_nodes, [number_of_nodes](NodeEvictionCandidate& candidate) {
-                            candidate.nLastBlockTime = number_of_nodes - candidate.id;
+                            candidate.m_last_block_time = std::chrono::seconds{number_of_nodes - candidate.id};
                             if (candidate.id <= 7) {
                                 candidate.m_relay_txs = false;
                                 candidate.fRelevantServices = true;
@@ -679,8 +657,8 @@ BOOST_AUTO_TEST_CASE(peer_eviction_test)
                         number_of_nodes, [number_of_nodes](NodeEvictionCandidate& candidate) {
                             candidate.nKeyedNetGroup = number_of_nodes - candidate.id;           // 4 protected
                             candidate.m_min_ping_time = std::chrono::microseconds{candidate.id}; // 8 protected
-                            candidate.nLastTXTime = number_of_nodes - candidate.id;              // 4 protected
+                            candidate.m_last_tx_time = std::chrono::seconds{number_of_nodes - candidate.id};    // 4 protected
-                            candidate.nLastBlockTime = number_of_nodes - candidate.id;           // 4 protected
+                            candidate.m_last_block_time = std::chrono::seconds{number_of_nodes - candidate.id}; // 4 protected
                         },
                         {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19}, random_context));
 

src/test/net_tests.cpp

@@ -6,15 +6,21 @@
 
 #include <chainparams.h>
 #include <clientversion.h>
+#include <compat.h>
 #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/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>
@@ -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<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()

src/test/util/net.cpp

@@ -6,6 +6,67 @@
 
 #include <chainparams.h>
 #include <net.h>
+#include <net_processing.h>
+#include <netmessagemaker.h>
+#include <span.h>
+
+#include <vector>
+
+void ConnmanTestMsg::Handshake(CNode& node,
+                               bool successfully_connected,
+                               ServiceFlags remote_services,
+                               NetPermissionFlags permission_flags,
+                               int32_t version,
+                               bool relay_txs)
+{
+    auto& peerman{static_cast<PeerManager&>(*m_msgproc)};
+    auto& connman{*this};
+    const CNetMsgMaker mm{0};
+
+    peerman.InitializeNode(&node);
+
+    CSerializedNetMsg msg_version{
+        mm.Make(NetMsgType::VERSION,
+                version,                                        //
+                Using<CustomUintFormatter<8>>(remote_services), //
+                int64_t{},                                      // dummy time
+                int64_t{},                                      // ignored service bits
+                CService{},                                     // dummy
+                int64_t{},                                      // ignored service bits
+                CService{},                                     // ignored
+                uint64_t{1},                                    // dummy nonce
+                std::string{},                                  // dummy subver
+                int32_t{},                                      // dummy starting_height
+                relay_txs),
+    };
+
+    (void)connman.ReceiveMsgFrom(node, msg_version);
+    node.fPauseSend = false;
+    connman.ProcessMessagesOnce(node);
+    {
+        LOCK(node.cs_sendProcessing);
+        peerman.SendMessages(&node);
+    }
+    if (node.fDisconnect) return;
+    assert(node.nVersion == version);
+    assert(node.GetCommonVersion() == std::min(version, PROTOCOL_VERSION));
+    assert(node.nServices == remote_services);
+    CNodeStateStats statestats;
+    assert(peerman.GetNodeStateStats(node.GetId(), statestats));
+    assert(statestats.m_relay_txs == (relay_txs && !node.IsBlockOnlyConn()));
+    node.m_permissionFlags = permission_flags;
+    if (successfully_connected) {
+        CSerializedNetMsg msg_verack{mm.Make(NetMsgType::VERACK)};
+        (void)connman.ReceiveMsgFrom(node, msg_verack);
+        node.fPauseSend = false;
+        connman.ProcessMessagesOnce(node);
+        {
+            LOCK(node.cs_sendProcessing);
+            peerman.SendMessages(&node);
+        }
+        assert(node.fSuccessfullyConnected == true);
+    }
+}
 
 void ConnmanTestMsg::NodeReceiveMsgBytes(CNode& node, Span<const uint8_t> msg_bytes, bool& complete) const
 {
@@ -37,3 +98,25 @@ bool ConnmanTestMsg::ReceiveMsgFrom(CNode& node, CSerializedNetMsg& ser_msg) con
     NodeReceiveMsgBytes(node, ser_msg.data, complete);
     return complete;
 }
+
+std::vector<NodeEvictionCandidate> GetRandomNodeEvictionCandidates(int n_candidates, FastRandomContext& random_context)
+{
+    std::vector<NodeEvictionCandidate> candidates;
+    for (int id = 0; id < n_candidates; ++id) {
+        candidates.push_back({
+            /* id */ id,
+            /* m_connected */ std::chrono::seconds{random_context.randrange(100)},
+            /* m_min_ping_time */ std::chrono::microseconds{random_context.randrange(100)},
+            /* m_last_block_time */ std::chrono::seconds{random_context.randrange(100)},
+            /* m_last_tx_time */ std::chrono::seconds{random_context.randrange(100)},
+            /* fRelevantServices */ random_context.randbool(),
+            /* m_relay_txs */ random_context.randbool(),
+            /* fBloomFilter */ random_context.randbool(),
+            /* nKeyedNetGroup */ random_context.randrange(100),
+            /* prefer_evict */ random_context.randbool(),
+            /* m_is_local */ random_context.randbool(),
+            /* m_network */ ALL_NETWORKS[random_context.randrange(ALL_NETWORKS.size())],
+        });
+    }
+    return candidates;
+}

src/test/util/net.h

@@ -38,6 +38,13 @@ struct ConnmanTestMsg : public CConnman {
         m_nodes.clear();
     }
 
+    void Handshake(CNode& node,
+                   bool successfully_connected,
+                   ServiceFlags remote_services,
+                   NetPermissionFlags permission_flags,
+                   int32_t version,
+                   bool relay_txs);
+
     void ProcessMessagesOnce(CNode& node) { m_msgproc->ProcessMessages(&node, flagInterruptMsgProc); }
 
     void NodeReceiveMsgBytes(CNode& node, Span<const uint8_t> msg_bytes, bool& complete) const;
@@ -177,4 +184,6 @@ private:
     mutable size_t m_consumed;
 };
 
+std::vector<NodeEvictionCandidate> GetRandomNodeEvictionCandidates(int n_candidates, FastRandomContext& random_context);
+
 #endif // BITCOIN_TEST_UTIL_NET_H

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()

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()

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',