From 017d1b40e3239287aa9c5ff928373eba89ea956d Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Tue, 26 Mar 2024 17:39:35 +0000 Subject: [PATCH 01/19] merge bitcoin#19763: don't relay to the address' originator --- src/net_processing.cpp | 25 +++++++++++++++++---- test/functional/p2p_addr_relay.py | 36 +++++++++++++++++++++++-------- 2 files changed, 48 insertions(+), 13 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 90bcc8e785e9..eafc5b11ef18 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2006,7 +2006,23 @@ void PeerManagerImpl::RelayTransaction(const uint256& txid) }); } -static void RelayAddress(const CAddress& addr, bool fReachable, const CConnman& connman) +/** + * Relay (gossip) an address to a few randomly chosen nodes. + * We choose the same nodes within a given 24h window (if the list of connected + * nodes does not change) and we don't relay to nodes that already know an + * address. So within 24h we will likely relay a given address once. This is to + * prevent a peer from unjustly giving their address better propagation by sending + * it to us repeatedly. + * @param[in] originator The peer that sent us the address. We don't want to relay it back. + * @param[in] addr Address to relay. + * @param[in] fReachable Whether the address' network is reachable. We relay unreachable + * addresses less. + * @param[in] connman Connection manager to choose nodes to relay to. + */ +static void RelayAddress(const CNode& originator, + const CAddress& addr, + bool fReachable, + const CConnman& connman) { if (!fReachable && !addr.IsRelayable()) return; @@ -2023,8 +2039,8 @@ static void RelayAddress(const CAddress& addr, bool fReachable, const CConnman& std::array,2> best{{{0, nullptr}, {0, nullptr}}}; assert(nRelayNodes <= best.size()); - auto sortfunc = [&best, &hasher, nRelayNodes, addr](CNode* pnode) { - if (pnode->RelayAddrsWithConn() && pnode->IsAddrCompatible(addr)) { + auto sortfunc = [&best, &hasher, nRelayNodes, &originator, &addr](CNode* pnode) { + if (pnode->RelayAddrsWithConn() && pnode != &originator && pnode->IsAddrCompatible(addr)) { uint64_t hashKey = CSipHasher(hasher).Write(pnode->GetId()).Finalize(); for (unsigned int i = 0; i < nRelayNodes; i++) { if (hashKey > best[i].first) { @@ -3329,7 +3345,8 @@ void PeerManagerImpl::ProcessMessage( bool fReachable = IsReachable(addr); if (addr.nTime > nSince && !pfrom.fGetAddr && vAddr.size() <= 10 && addr.IsRoutable()) { - RelayAddress(addr, fReachable, m_connman); + // Relay to a limited number of other nodes + RelayAddress(pfrom, addr, fReachable, m_connman); } // Do not store addresses outside our network if (fReachable) diff --git a/test/functional/p2p_addr_relay.py b/test/functional/p2p_addr_relay.py index e113545ab449..bc10a7c3c862 100755 --- a/test/functional/p2p_addr_relay.py +++ b/test/functional/p2p_addr_relay.py @@ -17,14 +17,20 @@ assert_equal, ) +# Keep this with length <= 10. Addresses from larger messages are not relayed. ADDRS = [] +num_ipv4_addrs = 10 class AddrReceiver(P2PInterface): + num_ipv4_received = 0 + def on_addr(self, message): for addr in message.addrs: assert_equal(addr.nServices, 1) + if not 8333 <= addr.port < 8343: + raise AssertionError("Invalid addr.port of {} (8333-8342 expected)".format(addr.port)) assert addr.ip.startswith('123.123.123.') - assert (8333 <= addr.port < 8343) + self.num_ipv4_received += 1 class AddrTest(BitcoinTestFramework): @@ -32,7 +38,7 @@ def set_test_params(self): self.num_nodes = 1 def run_test(self): - for i in range(10): + for i in range(num_ipv4_addrs): addr = CAddress() addr.time = int(self.mocktime) + i addr.nServices = NODE_NETWORK @@ -45,21 +51,33 @@ def run_test(self): msg = msg_addr() self.log.info('Send too-large addr message') - msg.addrs = ADDRS * 101 + msg.addrs = ADDRS * 101 # more than 1000 addresses in one message with self.nodes[0].assert_debug_log(['addr message size = 1010']): addr_source.send_and_ping(msg) self.log.info('Check that addr message content is relayed and added to addrman') - addr_receiver = self.nodes[0].add_p2p_connection(AddrReceiver()) + num_receivers = 7 + receivers = [] + for _ in range(num_receivers): + receivers.append(self.nodes[0].add_p2p_connection(AddrReceiver())) msg.addrs = ADDRS - with self.nodes[0].assert_debug_log([ - 'Added 10 addresses from 127.0.0.1: 0 tried', + with self.nodes[0].assert_debug_log( + [ + 'Added {} addresses from 127.0.0.1: 0 tried'.format(num_ipv4_addrs), 'received: addr (301 bytes) peer=0', - 'sending addr (301 bytes) peer=1', - ]): + ] + ): addr_source.send_and_ping(msg) self.bump_mocktime(30 * 60) - addr_receiver.sync_with_ping() + for receiver in receivers: + receiver.sync_with_ping() + + total_ipv4_received = sum(r.num_ipv4_received for r in receivers) + + # Every IPv4 address must be relayed to two peers, other than the + # originating node (addr_source). + ipv4_branching_factor = 2 + assert_equal(total_ipv4_received, num_ipv4_addrs * ipv4_branching_factor) if __name__ == '__main__': From d0c596e91d235248e7304a23ab8d4b23f3dabc92 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Mon, 14 Dec 2020 18:25:20 +0100 Subject: [PATCH 02/19] merge bitcoin#20653: Move addr relay comment in net to correct place comment was moved to net.h in 678df631 (#4888) and removed entirely in 796353ad (#5771). the comment is being restored back to where it is upstream, in CNode::RelayAddrsWithConn. --- src/net.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/net.h b/src/net.h index 9f486a49ad01..67e5e93cf70d 100644 --- a/src/net.h +++ b/src/net.h @@ -568,6 +568,9 @@ class CNode /* Whether we send addr messages over this connection */ bool RelayAddrsWithConn() const { + // Don't relay addr messages to peers that we connect to as block-relay-only + // peers (to prevent adversaries from inferring these links from addr + // traffic). return m_conn_type != ConnectionType::BLOCK_RELAY; } From b76e029e44ceda194802900f3ddb471e13fc05a4 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Wed, 23 Dec 2020 11:38:59 -0800 Subject: [PATCH 03/19] merge bitcoin#20756: Add missing field (permissions) to the getpeerinfo help --- src/rpc/net.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 7a68deb474d1..de1e1f14affa 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -145,6 +145,10 @@ static RPCHelpMan getpeerinfo() {RPCResult::Type::NUM, "n", "The heights of blocks we're currently asking from this peer"}, }}, {RPCResult::Type::BOOL, "whitelisted", "Whether the peer is whitelisted"}, + {RPCResult::Type::ARR, "permissions", "Any special permissions that have been granted to this peer", + { + {RPCResult::Type::STR, "permission_type", Join(NET_PERMISSIONS_DOC, ",\n") + ".\n"}, + }}, {RPCResult::Type::OBJ_DYN, "bytessent_per_msg", "", { {RPCResult::Type::NUM, "msg", "The total bytes sent aggregated by message type\n" From 1d4f10a378a8bee70cdb2f4392eb33a493029a77 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 28 Mar 2024 03:21:36 +0000 Subject: [PATCH 04/19] merge bitcoin#19315: Allow outbound & block-relay-only connections in functional tests --- src/net.cpp | 21 ++++ src/net.h | 13 +++ src/rpc/net.cpp | 58 +++++++++++ src/rpc/protocol.h | 1 + test/functional/p2p_add_connections.py | 97 ++++++++++++++++++ test/functional/p2p_blocksonly.py | 104 +++++++++++++------- test/functional/test_framework/p2p.py | 98 +++++++++++++++--- test/functional/test_framework/test_node.py | 27 ++++- test/functional/test_runner.py | 1 + 9 files changed, 366 insertions(+), 54 deletions(-) create mode 100755 test/functional/p2p_add_connections.py diff --git a/src/net.cpp b/src/net.cpp index fd7c1189bfe3..536061463864 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1274,6 +1274,27 @@ void CConnman::CreateNodeFromAcceptedSocket(SOCKET hSocket, RandAddEvent((uint32_t)id); } +bool CConnman::AddConnection(const std::string& address, ConnectionType conn_type) +{ + if (conn_type != ConnectionType::OUTBOUND_FULL_RELAY && conn_type != ConnectionType::BLOCK_RELAY) return false; + + const int max_connections = conn_type == ConnectionType::OUTBOUND_FULL_RELAY ? m_max_outbound_full_relay : m_max_outbound_block_relay; + + // Count existing connections + int existing_connections = WITH_LOCK(cs_vNodes, + return std::count_if(vNodes.begin(), vNodes.end(), [conn_type](CNode* node) { return node->m_conn_type == conn_type; });); + + // Max connections of specified type already exist + if (existing_connections >= max_connections) return false; + + // Max total outbound connections already exist + CSemaphoreGrant grant(*semOutbound, true); + if (!grant) return false; + + OpenNetworkConnection(CAddress(), false, &grant, address.c_str(), conn_type); + return true; +} + void CConnman::DisconnectNodes() { { diff --git a/src/net.h b/src/net.h index 67e5e93cf70d..3ce61943ca33 100644 --- a/src/net.h +++ b/src/net.h @@ -1227,6 +1227,19 @@ friend class CNode; bool RemoveAddedNode(const std::string& node); std::vector GetAddedNodeInfo(); + /** + * Attempts to open a connection. Currently only used from tests. + * + * @param[in] address Address of node to try connecting to + * @param[in] conn_type ConnectionType::OUTBOUND or ConnectionType::BLOCK_RELAY + * @return bool Returns false if there are no available + * slots for this connection: + * - conn_type not a supported ConnectionType + * - Max total outbound connection capacity filled + * - Max connection capacity for type is filled + */ + bool AddConnection(const std::string& address, ConnectionType conn_type); + bool AddPendingMasternode(const uint256& proTxHash); void SetMasternodeQuorumNodes(Consensus::LLMQType llmqType, const uint256& quorumHash, const std::set& proTxHashes); void SetMasternodeQuorumRelayMembers(Consensus::LLMQType llmqType, const uint256& quorumHash, const std::set& proTxHashes); diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index de1e1f14affa..ca6e3fef30e3 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -6,6 +6,7 @@ #include #include +#include #include #include #include @@ -324,6 +325,61 @@ static RPCHelpMan addnode() }; } +static RPCHelpMan addconnection() +{ + return RPCHelpMan{"addconnection", + "\nOpen an outbound connection to a specified node. This RPC is for testing only.\n", + { + {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The IP address and port to attempt connecting to."}, + {"connection_type", RPCArg::Type::STR, RPCArg::Optional::NO, "Type of connection to open, either \"outbound-full-relay\" or \"block-relay-only\"."}, + }, + RPCResult{ + RPCResult::Type::OBJ, "", "", + { + { RPCResult::Type::STR, "address", "Address of newly added connection." }, + { RPCResult::Type::STR, "connection_type", "Type of connection opened." }, + }}, + RPCExamples{ + HelpExampleCli("addconnection", "\"192.168.0.6:8333\" \"outbound-full-relay\"") + + HelpExampleRpc("addconnection", "\"192.168.0.6:8333\" \"outbound-full-relay\"") + }, + [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue +{ + if (Params().NetworkIDString() != CBaseChainParams::REGTEST) { + throw std::runtime_error("addconnection is for regression testing (-regtest mode) only."); + } + + RPCTypeCheck(request.params, {UniValue::VSTR, UniValue::VSTR}); + const std::string address = request.params[0].get_str(); + const std::string conn_type_in{TrimString(request.params[1].get_str())}; + ConnectionType conn_type{}; + if (conn_type_in == "outbound-full-relay") { + conn_type = ConnectionType::OUTBOUND_FULL_RELAY; + } else if (conn_type_in == "block-relay-only") { + conn_type = ConnectionType::BLOCK_RELAY; + } else { + throw JSONRPCError(RPC_INVALID_PARAMETER, self.ToString()); + } + + NodeContext& node = EnsureAnyNodeContext(request.context); + if (!node.connman) { + throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled."); + } + + const bool success = node.connman->AddConnection(address, conn_type); + if (!success) { + throw JSONRPCError(RPC_CLIENT_NODE_CAPACITY_REACHED, "Error: Already at capacity for specified connection type."); + } + + UniValue info(UniValue::VOBJ); + info.pushKV("address", address); + info.pushKV("connection_type", conn_type_in); + + return info; +}, + }; +} + static RPCHelpMan disconnectnode() { return RPCHelpMan{"disconnectnode", @@ -967,6 +1023,8 @@ static const CRPCCommand commands[] = { "network", "cleardiscouraged", &cleardiscouraged, {} }, { "network", "setnetworkactive", &setnetworkactive, {"state"} }, { "network", "getnodeaddresses", &getnodeaddresses, {"count"} }, + + { "hidden", "addconnection", &addconnection, {"address", "connection_type"} }, { "hidden", "addpeeraddress", &addpeeraddress, {"address", "port"} }, }; // clang-format on diff --git a/src/rpc/protocol.h b/src/rpc/protocol.h index 0e1b28ff1e5f..6a567a9ae12c 100644 --- a/src/rpc/protocol.h +++ b/src/rpc/protocol.h @@ -63,6 +63,7 @@ enum RPCErrorCode RPC_CLIENT_NODE_NOT_CONNECTED = -29, //!< Node to disconnect not found in connected nodes RPC_CLIENT_INVALID_IP_OR_SUBNET = -30, //!< Invalid IP/Subnet RPC_CLIENT_P2P_DISABLED = -31, //!< No valid connection manager instance found + RPC_CLIENT_NODE_CAPACITY_REACHED= -34, //!< Max number of outbound or block-relay connections already open //! Chain errors RPC_CLIENT_MEMPOOL_DISABLED = -33, //!< No mempool instance found diff --git a/test/functional/p2p_add_connections.py b/test/functional/p2p_add_connections.py new file mode 100755 index 000000000000..a63c3a3287c8 --- /dev/null +++ b/test/functional/p2p_add_connections.py @@ -0,0 +1,97 @@ +#!/usr/bin/env python3 +# Copyright (c) 2020 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. +"""Test add_outbound_p2p_connection test framework functionality""" + +from test_framework.p2p import P2PInterface +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import assert_equal + + +def check_node_connections(*, node, num_in, num_out): + info = node.getnetworkinfo() + assert_equal(info["connections_in"], num_in) + assert_equal(info["connections_out"], num_out) + + +class P2PAddConnections(BitcoinTestFramework): + def set_test_params(self): + self.setup_clean_chain = False + self.num_nodes = 2 + + def setup_network(self): + self.setup_nodes() + # Don't connect the nodes + + def run_test(self): + self.log.info("Add 8 outbounds to node 0") + for i in range(8): + self.log.info(f"outbound: {i}") + self.nodes[0].add_outbound_p2p_connection(P2PInterface(), p2p_idx=i, connection_type="outbound-full-relay") + + self.log.info("Add 2 block-relay-only connections to node 0") + for i in range(2): + self.log.info(f"block-relay-only: {i}") + # set p2p_idx based on the outbound connections already open to the + # node, so add 8 to account for the previous full-relay connections + self.nodes[0].add_outbound_p2p_connection(P2PInterface(), p2p_idx=i + 8, connection_type="block-relay-only") + + self.log.info("Add 2 block-relay-only connections to node 1") + for i in range(2): + self.log.info(f"block-relay-only: {i}") + self.nodes[1].add_outbound_p2p_connection(P2PInterface(), p2p_idx=i, connection_type="block-relay-only") + + self.log.info("Add 5 inbound connections to node 1") + for i in range(5): + self.log.info(f"inbound: {i}") + self.nodes[1].add_p2p_connection(P2PInterface()) + + self.log.info("Add 8 outbounds to node 1") + for i in range(8): + self.log.info(f"outbound: {i}") + # bump p2p_idx to account for the 2 existing outbounds on node 1 + self.nodes[1].add_outbound_p2p_connection(P2PInterface(), p2p_idx=i + 2) + + self.log.info("Check the connections opened as expected") + check_node_connections(node=self.nodes[0], num_in=0, num_out=10) + check_node_connections(node=self.nodes[1], num_in=5, num_out=10) + + self.log.info("Disconnect p2p connections & try to re-open") + self.nodes[0].disconnect_p2ps() + check_node_connections(node=self.nodes[0], num_in=0, num_out=0) + + self.log.info("Add 8 outbounds to node 0") + for i in range(8): + self.log.info(f"outbound: {i}") + self.nodes[0].add_outbound_p2p_connection(P2PInterface(), p2p_idx=i) + check_node_connections(node=self.nodes[0], num_in=0, num_out=8) + + self.log.info("Add 2 block-relay-only connections to node 0") + for i in range(2): + self.log.info(f"block-relay-only: {i}") + # bump p2p_idx to account for the 8 existing outbounds on node 0 + self.nodes[0].add_outbound_p2p_connection(P2PInterface(), p2p_idx=i + 8, connection_type="block-relay-only") + check_node_connections(node=self.nodes[0], num_in=0, num_out=10) + + self.log.info("Restart node 0 and try to reconnect to p2ps") + self.restart_node(0) + + self.log.info("Add 4 outbounds to node 0") + for i in range(4): + self.log.info(f"outbound: {i}") + self.nodes[0].add_outbound_p2p_connection(P2PInterface(), p2p_idx=i) + check_node_connections(node=self.nodes[0], num_in=0, num_out=4) + + self.log.info("Add 2 block-relay-only connections to node 0") + for i in range(2): + self.log.info(f"block-relay-only: {i}") + # bump p2p_idx to account for the 4 existing outbounds on node 0 + self.nodes[0].add_outbound_p2p_connection(P2PInterface(), p2p_idx=i + 4, connection_type="block-relay-only") + check_node_connections(node=self.nodes[0], num_in=0, num_out=6) + + check_node_connections(node=self.nodes[1], num_in=5, num_out=10) + + +if __name__ == '__main__': + P2PAddConnections().main() diff --git a/test/functional/p2p_blocksonly.py b/test/functional/p2p_blocksonly.py index da520aa349e1..5f25edf42497 100755 --- a/test/functional/p2p_blocksonly.py +++ b/test/functional/p2p_blocksonly.py @@ -2,10 +2,13 @@ # Copyright (c) 2019 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 p2p blocksonly""" +"""Test p2p blocksonly mode & block-relay-only connections.""" -from test_framework.messages import msg_tx, tx_from_hex -from test_framework.p2p import P2PInterface +import time + +from test_framework.blocktools import create_transaction +from test_framework.messages import msg_tx +from test_framework.p2p import P2PInterface, P2PTxInvStore from test_framework.test_framework import BitcoinTestFramework from test_framework.util import assert_equal @@ -15,49 +18,30 @@ def set_test_params(self): self.num_nodes = 1 self.extra_args = [["-blocksonly"]] + def skip_test_if_missing_module(self): + self.skip_if_no_wallet() + def run_test(self): - block_relay_peer = self.nodes[0].add_p2p_connection(P2PInterface()) - - self.log.info('Check that txs from p2p are rejected and result in disconnect') - prevtx = self.nodes[0].getblock(self.nodes[0].getblockhash(1), 2)['tx'][0] - rawtx = self.nodes[0].createrawtransaction( - inputs=[{ - 'txid': prevtx['txid'], - 'vout': 0 - }], - outputs=[{ - self.nodes[0].get_deterministic_priv_key()[0]: 500 - 0.00125 - }], - ) - sigtx = self.nodes[0].signrawtransactionwithkey( - hexstring=rawtx, - privkeys=[self.nodes[0].get_deterministic_priv_key()[1]], - prevtxs=[{ - 'txid': prevtx['txid'], - 'vout': 0, - 'scriptPubKey': prevtx['vout'][0]['scriptPubKey']['hex'], - }], - )['hex'] + self.blocksonly_mode_tests() + self.blocks_relay_conn_tests() + + def blocksonly_mode_tests(self): + self.log.info("Tests with node running in -blocksonly mode") assert_equal(self.nodes[0].getnetworkinfo()['localrelay'], False) - with self.nodes[0].assert_debug_log(['tx sent in violation of protocol peer=0']): - block_relay_peer.send_message(msg_tx(tx_from_hex(sigtx))) - block_relay_peer.wait_for_disconnect() - assert_equal(self.nodes[0].getmempoolinfo()['size'], 0) - # Remove the disconnected peer and add a new one. - del self.nodes[0].p2ps[0] - tx_relay_peer = self.nodes[0].add_p2p_connection(P2PInterface()) + self.nodes[0].add_p2p_connection(P2PInterface()) + tx, txid, tx_hex = self.check_p2p_tx_violation() self.log.info('Check that txs from rpc are not rejected and relayed to other peers') + tx_relay_peer = self.nodes[0].add_p2p_connection(P2PInterface()) assert_equal(self.nodes[0].getpeerinfo()[0]['relaytxes'], True) - txid = self.nodes[0].testmempoolaccept([sigtx])[0]['txid'] + assert_equal(self.nodes[0].testmempoolaccept([tx_hex])[0]['allowed'], True) with self.nodes[0].assert_debug_log(['received getdata for: tx {} peer=1'.format(txid)]): - self.nodes[0].sendrawtransaction(sigtx) + self.nodes[0].sendrawtransaction(tx_hex) self.bump_mocktime(60) tx_relay_peer.wait_for_tx(txid) assert_equal(self.nodes[0].getmempoolinfo()['size'], 1) - self.log.info('Check that txs from peers with relay-permission are not rejected and relayed to others') self.log.info("Restarting node 0 with relay permission and blocksonly") self.restart_node(0, ["-persistmempool=0", "-whitelist=relay@127.0.0.1", "-blocksonly"]) assert_equal(self.nodes[0].getrawmempool(), []) @@ -67,8 +51,7 @@ def run_test(self): assert_equal(peer_1_info['permissions'], ['relay']) peer_2_info = self.nodes[0].getpeerinfo()[1] assert_equal(peer_2_info['permissions'], ['relay']) - assert_equal(self.nodes[0].testmempoolaccept([sigtx])[0]['allowed'], True) - txid = self.nodes[0].testmempoolaccept([sigtx])[0]['txid'] + assert_equal(self.nodes[0].testmempoolaccept([tx_hex])[0]['allowed'], True) self.log.info('Check that the tx from first_peer with relay-permission is relayed to others (ie.second_peer)') with self.nodes[0].assert_debug_log(["received getdata"]): @@ -78,7 +61,7 @@ def run_test(self): # But if, for some reason, first_peer decides to relay transactions to us anyway, we should relay them to # second_peer since we gave relay permission to first_peer. # See https://github.com/bitcoin/bitcoin/issues/19943 for details. - first_peer.send_message(msg_tx(tx_from_hex(sigtx))) + first_peer.send_message(msg_tx(tx)) self.log.info('Check that the peer with relay-permission is still connected after sending the transaction') assert_equal(first_peer.is_connected, True) self.bump_mocktime(60) @@ -86,6 +69,51 @@ def run_test(self): assert_equal(self.nodes[0].getmempoolinfo()['size'], 1) self.log.info("Relay-permission peer's transaction is accepted and relayed") + self.nodes[0].disconnect_p2ps() + self.nodes[0].generate(1) + + def blocks_relay_conn_tests(self): + self.log.info('Tests with node in normal mode with block-relay-only connections') + self.restart_node(0, ["-noblocksonly"]) # disables blocks only mode + assert_equal(self.nodes[0].getnetworkinfo()['localrelay'], True) + + # Ensure we disconnect if a block-relay-only connection sends us a transaction + self.nodes[0].add_outbound_p2p_connection(P2PInterface(), p2p_idx=0, connection_type="block-relay-only") + assert_equal(self.nodes[0].getpeerinfo()[0]['relaytxes'], False) + _, txid, tx_hex = self.check_p2p_tx_violation(index=2) + + self.log.info("Check that txs from RPC are not sent to blockrelay connection") + conn = self.nodes[0].add_outbound_p2p_connection(P2PTxInvStore(), p2p_idx=1, connection_type="block-relay-only") + + self.nodes[0].sendrawtransaction(tx_hex) + + # Bump time forward to ensure nNextInvSend timer pops + self.nodes[0].setmocktime(int(time.time()) + 60) + + # Calling sync_with_ping twice requires that the node calls + # `ProcessMessage` twice, and thus ensures `SendMessages` must have + # been called at least once + conn.sync_with_ping() + conn.sync_with_ping() + assert(int(txid, 16) not in conn.get_invs()) + + def check_p2p_tx_violation(self, index=1): + self.log.info('Check that txs from P2P are rejected and result in disconnect') + input_txid = self.nodes[0].getblock(self.nodes[0].getblockhash(index), 2)['tx'][0]['txid'] + tx = create_transaction(self.nodes[0], input_txid, self.nodes[0].getnewaddress(), amount=(500 - 0.001)) + txid = tx.rehash() + tx_hex = tx.serialize().hex() + + with self.nodes[0].assert_debug_log(['tx sent in violation of protocol peer=0']): + self.nodes[0].p2ps[0].send_message(msg_tx(tx)) + self.nodes[0].p2ps[0].wait_for_disconnect() + assert_equal(self.nodes[0].getmempoolinfo()['size'], 0) + + # Remove the disconnected peer + del self.nodes[0].p2ps[0] + + return tx, txid, tx_hex + if __name__ == '__main__': P2PBlocksOnly().main() diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py index c00a3c2986df..103ea0420c4b 100755 --- a/test/functional/test_framework/p2p.py +++ b/test/functional/test_framework/p2p.py @@ -82,7 +82,11 @@ NODE_NETWORK, sha256, ) -from test_framework.util import wait_until_helper +from test_framework.util import ( + MAX_NODES, + p2p_port, + wait_until_helper, +) logger = logging.getLogger("TestFramework.p2p") @@ -168,7 +172,7 @@ def __init__(self): def is_connected(self): return self._transport is not None - def peer_connect(self, dstaddr, dstport, *, net, timeout_factor, uacomment=None): + def peer_connect_helper(self, dstaddr, dstport, net, timeout_factor, uacomment): assert not self.is_connected self.timeout_factor = timeout_factor self.dstaddr = dstaddr @@ -190,12 +194,19 @@ def peer_connect(self, dstaddr, dstport, *, net, timeout_factor, uacomment=None) else: self.strSubVer = MY_SUBVERSION % b"" - logger.debug('Connecting to Dash Node: %s:%d' % (self.dstaddr, self.dstport)) + def peer_connect(self, dstaddr, dstport, *, net, timeout_factor, uacomment=None): + self.peer_connect_helper(dstaddr, dstport, net, timeout_factor, uacomment) loop = NetworkThread.network_event_loop - conn_gen_unsafe = loop.create_connection(lambda: self, host=self.dstaddr, port=self.dstport) - conn_gen = lambda: loop.call_soon_threadsafe(loop.create_task, conn_gen_unsafe) - return conn_gen + logger.debug('Connecting to Dash Node: %s:%d' % (self.dstaddr, self.dstport)) + coroutine = loop.create_connection(lambda: self, host=self.dstaddr, port=self.dstport) + return lambda: loop.call_soon_threadsafe(loop.create_task, coroutine) + + def peer_accept_connection(self, connect_id, connect_cb=lambda: None, *, net, timeout_factor, uacomment=None): + self.peer_connect_helper('0', 0, net, timeout_factor, uacomment) + + logger.debug('Listening for Dash Node with id: {}'.format(connect_id)) + return lambda: NetworkThread.listen(self, connect_cb, idx=connect_id) def peer_disconnect(self): # Connection could have already been closed by other end. @@ -354,19 +365,28 @@ def __init__(self, support_addrv2=False): self.support_addrv2 = support_addrv2 + def peer_connect_send_version(self, services): + # Send a version msg + vt = msg_version() + vt.nServices = services + vt.addrTo.ip = self.dstaddr + vt.addrTo.port = self.dstport + vt.addrFrom.ip = "0.0.0.0" + vt.addrFrom.port = 0 + vt.strSubVer = self.strSubVer + self.on_connection_send_msg = vt # Will be sent soon after connection_made + def peer_connect(self, *args, services=NODE_NETWORK | NODE_HEADERS_COMPRESSED, send_version=True, **kwargs): create_conn = super().peer_connect(*args, **kwargs) if send_version: - # Send a version msg - vt = msg_version() - vt.nServices = services - vt.addrTo.ip = self.dstaddr - vt.addrTo.port = self.dstport - vt.addrFrom.ip = "0.0.0.0" - vt.addrFrom.port = 0 - vt.strSubVer = self.strSubVer - self.on_connection_send_msg = vt # Will be sent soon after connection_made + self.peer_connect_send_version(services) + + return create_conn + + def peer_accept_connection(self, *args, services=NODE_NETWORK | NODE_HEADERS_COMPRESSED, **kwargs): + create_conn = super().peer_accept_connection(*args, **kwargs) + self.peer_connect_send_version(services) return create_conn @@ -465,6 +485,10 @@ def test_function(): wait_until_helper(test_function, timeout=timeout, lock=p2p_lock, timeout_factor=self.timeout_factor) + def wait_for_connect(self, timeout=60): + test_function = lambda: self.is_connected + wait_until_helper(test_function, timeout=timeout, lock=p2p_lock) + def wait_for_disconnect(self, timeout=60): test_function = lambda: not self.is_connected self.wait_until(test_function, timeout=timeout, check_connected=False) @@ -580,6 +604,8 @@ def __init__(self): # There is only one event loop and no more than one thread must be created assert not self.network_event_loop + NetworkThread.listeners = {} + NetworkThread.protos = {} NetworkThread.network_event_loop = asyncio.new_event_loop() def run(self): @@ -595,6 +621,48 @@ def close(self, timeout=10): # Safe to remove event loop. NetworkThread.network_event_loop = None + @classmethod + def listen(cls, p2p, callback, port=None, addr=None, idx=1): + """ Ensure a listening server is running on the given port, and run the + protocol specified by `p2p` on the next connection to it. Once ready + for connections, call `callback`.""" + + if port is None: + assert 0 < idx <= MAX_NODES + port = p2p_port(MAX_NODES - idx) + if addr is None: + addr = '127.0.0.1' + + coroutine = cls.create_listen_server(addr, port, callback, p2p) + cls.network_event_loop.call_soon_threadsafe(cls.network_event_loop.create_task, coroutine) + + @classmethod + async def create_listen_server(cls, addr, port, callback, proto): + def peer_protocol(): + """Returns a function that does the protocol handling for a new + connection. To allow different connections to have different + behaviors, the protocol function is first put in the cls.protos + dict. When the connection is made, the function removes the + protocol function from that dict, and returns it so the event loop + can start executing it.""" + response = cls.protos.get((addr, port)) + cls.protos[(addr, port)] = None + return response + + if (addr, port) not in cls.listeners: + # When creating a listener on a given (addr, port) we only need to + # do it once. If we want different behaviors for different + # connections, we can accomplish this by providing different + # `proto` functions + + listener = await cls.network_event_loop.create_server(peer_protocol, addr, port) + logger.debug("Listening server on %s:%d should be started" % (addr, port)) + cls.listeners[(addr, port)] = listener + + cls.protos[(addr, port)] = proto + callback(addr, port) + + class P2PDataStore(P2PInterface): """A P2P data store class. diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 6e27f6fd303a..d86205ef2832 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -72,6 +72,7 @@ def __init__(self, i, datadir, extra_args_from_options, *, chain, rpchost, timew """ self.index = i + self.p2p_conn_index = 1 self.datadir = datadir self.chain = chain self.bitcoinconf = os.path.join(self.datadir, "dash.conf") @@ -537,7 +538,7 @@ def assert_start_raises_init_error(self, extra_args=None, expected_msg=None, mat self._raise_assertion_error(assert_msg) def add_p2p_connection(self, p2p_conn, *, wait_for_verack=True, **kwargs): - """Add a p2p connection to the node. + """Add an inbound p2p connection to the node. This method adds the p2p connection to the self.p2ps list and also returns the connection to the caller.""" @@ -566,6 +567,29 @@ def add_p2p_connection(self, p2p_conn, *, wait_for_verack=True, **kwargs): return p2p_conn + def add_outbound_p2p_connection(self, p2p_conn, *, p2p_idx, connection_type="outbound-full-relay", **kwargs): + """Add an outbound p2p connection from node. Either + full-relay("outbound-full-relay") or + block-relay-only("block-relay-only") connection. + + This method adds the p2p connection to the self.p2ps list and returns + the connection to the caller. + """ + + def addconnection_callback(address, port): + self.log.debug("Connecting to %s:%d %s" % (address, port, connection_type)) + self.addconnection('%s:%d' % (address, port), connection_type) + + p2p_conn.peer_accept_connection(connect_cb=addconnection_callback, connect_id=p2p_idx + 1, net=self.chain, timeout_factor=self.timeout_factor, **kwargs)() + + p2p_conn.wait_for_connect() + self.p2ps.append(p2p_conn) + + p2p_conn.wait_for_verack() + p2p_conn.sync_with_ping() + + return p2p_conn + def num_test_p2p_connections(self): """Return number of test framework p2p connections to the node.""" return len([peer for peer in self.getpeerinfo() if peer['subver'] == MY_SUBVERSION.decode("utf-8")]) @@ -585,6 +609,7 @@ def check_peers(): wait_until_helper(check_peers, timeout=5) del self.p2ps[:] + wait_until_helper(lambda: self.num_test_p2p_connections() == 0, timeout_factor=self.timeout_factor) diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 8af724feb73c..2d3818609b73 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -306,6 +306,7 @@ 'feature_filelock.py', 'feature_loadblock.py', 'p2p_dos_header_tree.py', + 'p2p_add_connections.py', 'p2p_blockfilters.py', 'p2p_message_capture.py', 'feature_asmap.py', From e109c0042a45093db9acb44f509c534740e2e504 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Mon, 14 Dec 2020 15:51:26 +0100 Subject: [PATCH 05/19] merge bitcoin#20646: refer to BIPs 339/155 in feature negotiation --- src/net.h | 1 + src/net_processing.cpp | 5 +++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/net.h b/src/net.h index 3ce61943ca33..86c7e82a3e25 100644 --- a/src/net.h +++ b/src/net.h @@ -487,6 +487,7 @@ class CNode * messages, implying a preference to receive ADDRv2 instead of ADDR ones. */ std::atomic_bool m_wants_addrv2{false}; + /** fSuccessfullyConnected is set to true on receiving VERACK from the peer. */ std::atomic_bool fSuccessfullyConnected{false}; // Setting fDisconnect to true will cause the node to be disconnected the // next time DisconnectNodes() runs diff --git a/src/net_processing.cpp b/src/net_processing.cpp index eafc5b11ef18..df03622009eb 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3256,14 +3256,15 @@ void PeerManagerImpl::ProcessMessage( return; } + // BIP155 defines feature negotiation of addrv2 and sendaddrv2, which must happen + // between VERSION and VERACK. if (msg_type == NetMsgType::SENDADDRV2) { if (pfrom.GetCommonVersion() < ADDRV2_PROTO_VERSION) { // Ignore previous implementations return; } if (pfrom.fSuccessfullyConnected) { - // Disconnect peers that send SENDADDRV2 message after VERACK; this - // must be negotiated between VERSION and VERACK. + // Disconnect peers that send a SENDADDRV2 message after VERACK. LogPrint(BCLog::NET_NETCONN, "sendaddrv2 received after verack from peer=%d; disconnecting\n", pfrom.GetId()); pfrom.fDisconnect = true; return; From 3e8ba24c87e32c2669c7be6924e750e29805a30e Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sat, 30 Jan 2021 22:09:40 +0100 Subject: [PATCH 06/19] partial bitcoin-core/gui#206: Display fRelayTxes and bip152_highbandwidth_{to, from} in peer details excludes: - 142807af8b82e2372a03df893c50df4f4a96aca4 --- src/qt/forms/debugwindow.ui | 78 ++++++++++++++++++++++++------------- src/qt/rpcconsole.cpp | 1 + 2 files changed, 53 insertions(+), 26 deletions(-) diff --git a/src/qt/forms/debugwindow.ui b/src/qt/forms/debugwindow.ui index 96df63629679..753c9ba4855b 100644 --- a/src/qt/forms/debugwindow.ui +++ b/src/qt/forms/debugwindow.ui @@ -1121,13 +1121,39 @@ + + + Whether the peer requested us to relay transactions. + + + Wants Tx Relay + + + + + + + IBeamCursor + + + N/A + + + Qt::PlainText + + + Qt::LinksAccessibleByMouse|Qt::TextSelectableByKeyboard|Qt::TextSelectableByMouse + + + + Starting Block - + IBeamCursor @@ -1143,14 +1169,14 @@ - + Synced Headers - + IBeamCursor @@ -1166,14 +1192,14 @@ - + Synced Blocks - + IBeamCursor @@ -1189,14 +1215,14 @@ - + Connection Time - + IBeamCursor @@ -1212,14 +1238,14 @@ - + Last Send - + IBeamCursor @@ -1235,14 +1261,14 @@ - + Last Receive - + IBeamCursor @@ -1258,14 +1284,14 @@ - + Sent - + IBeamCursor @@ -1281,14 +1307,14 @@ - + Received - + IBeamCursor @@ -1304,14 +1330,14 @@ - + Ping Time - + IBeamCursor @@ -1327,7 +1353,7 @@ - + The duration of a currently outstanding ping. @@ -1337,7 +1363,7 @@ - + IBeamCursor @@ -1353,14 +1379,14 @@ - + Min Ping - + IBeamCursor @@ -1376,14 +1402,14 @@ - + Time Offset - + IBeamCursor @@ -1399,7 +1425,7 @@ - + The mapped Autonomous System used for diversifying peer selection. @@ -1409,7 +1435,7 @@ - + IBeamCursor @@ -1425,7 +1451,7 @@ - + Qt::Vertical diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp index 18ebb08ec7ba..d852c9291a96 100644 --- a/src/qt/rpcconsole.cpp +++ b/src/qt/rpcconsole.cpp @@ -1243,6 +1243,7 @@ void RPCConsole::updateNodeDetail(const CNodeCombinedStats *stats) peerAddrDetails += "
" + tr("via %1").arg(QString::fromStdString(stats->nodeStats.addrLocal)); ui->peerHeading->setText(peerAddrDetails); ui->peerServices->setText(GUIUtil::formatServicesStr(stats->nodeStats.nServices)); + ui->peerRelayTxes->setText(stats->nodeStats.fRelayTxes ? "Yes" : "No"); ui->peerLastSend->setText(stats->nodeStats.nLastSend ? GUIUtil::formatDurationStr(GetSystemTimeInSeconds() - stats->nodeStats.nLastSend) : tr("never")); ui->peerLastRecv->setText(stats->nodeStats.nLastRecv ? GUIUtil::formatDurationStr(GetSystemTimeInSeconds() - stats->nodeStats.nLastRecv) : tr("never")); ui->peerBytesSent->setText(GUIUtil::formatBytes(stats->nodeStats.nSendBytes)); From 8b204c4c8233f74163468323117efdf76d75c4d8 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Mon, 25 Mar 2024 16:06:49 +0000 Subject: [PATCH 07/19] merge bitcoin-core/gui#226: Add "Last Block" and "Last Tx" rows to peer details area --- src/qt/forms/debugwindow.ui | 88 +++++++++++++++++++++++++++++-------- src/qt/rpcconsole.cpp | 9 ++-- src/qt/rpcconsole.h | 5 +++ 3 files changed, 81 insertions(+), 21 deletions(-) diff --git a/src/qt/forms/debugwindow.ui b/src/qt/forms/debugwindow.ui index 753c9ba4855b..7faa1d25d387 100644 --- a/src/qt/forms/debugwindow.ui +++ b/src/qt/forms/debugwindow.ui @@ -1239,13 +1239,65 @@ + + + Elapsed time since a novel block passing initial validity checks was received from this peer. + + + Last Block + + + + + + + IBeamCursor + + + N/A + + + Qt::PlainText + + + Qt::LinksAccessibleByMouse|Qt::TextSelectableByKeyboard|Qt::TextSelectableByMouse + + + + + + + Elapsed time since a novel transaction accepted into our mempool was received from this peer. + + + Last Tx + + + + + + + IBeamCursor + + + N/A + + + Qt::PlainText + + + Qt::LinksAccessibleByMouse|Qt::TextSelectableByKeyboard|Qt::TextSelectableByMouse + + + + Last Send - + IBeamCursor @@ -1261,14 +1313,14 @@ - + Last Receive - + IBeamCursor @@ -1284,14 +1336,14 @@ - + Sent - + IBeamCursor @@ -1307,14 +1359,14 @@ - + Received - + IBeamCursor @@ -1330,14 +1382,14 @@ - + Ping Time - + IBeamCursor @@ -1353,7 +1405,7 @@ - + The duration of a currently outstanding ping. @@ -1363,7 +1415,7 @@ - + IBeamCursor @@ -1379,14 +1431,14 @@ - + Min Ping - + IBeamCursor @@ -1402,14 +1454,14 @@ - + Time Offset - + IBeamCursor @@ -1425,7 +1477,7 @@ - + The mapped Autonomous System used for diversifying peer selection. @@ -1435,7 +1487,7 @@ - + IBeamCursor @@ -1451,7 +1503,7 @@ - + Qt::Vertical diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp index d852c9291a96..c1953e1273d5 100644 --- a/src/qt/rpcconsole.cpp +++ b/src/qt/rpcconsole.cpp @@ -1244,11 +1244,14 @@ void RPCConsole::updateNodeDetail(const CNodeCombinedStats *stats) ui->peerHeading->setText(peerAddrDetails); ui->peerServices->setText(GUIUtil::formatServicesStr(stats->nodeStats.nServices)); ui->peerRelayTxes->setText(stats->nodeStats.fRelayTxes ? "Yes" : "No"); - ui->peerLastSend->setText(stats->nodeStats.nLastSend ? GUIUtil::formatDurationStr(GetSystemTimeInSeconds() - stats->nodeStats.nLastSend) : tr("never")); - ui->peerLastRecv->setText(stats->nodeStats.nLastRecv ? GUIUtil::formatDurationStr(GetSystemTimeInSeconds() - stats->nodeStats.nLastRecv) : tr("never")); + const int64_t time_now{GetSystemTimeInSeconds()}; + ui->peerConnTime->setText(GUIUtil::formatDurationStr(time_now - stats->nodeStats.nTimeConnected)); + ui->peerLastBlock->setText(TimeDurationField(time_now, stats->nodeStats.nLastBlockTime)); + ui->peerLastTx->setText(TimeDurationField(time_now, stats->nodeStats.nLastTXTime)); + ui->peerLastSend->setText(TimeDurationField(time_now, stats->nodeStats.nLastSend)); + ui->peerLastRecv->setText(TimeDurationField(time_now, stats->nodeStats.nLastRecv)); ui->peerBytesSent->setText(GUIUtil::formatBytes(stats->nodeStats.nSendBytes)); ui->peerBytesRecv->setText(GUIUtil::formatBytes(stats->nodeStats.nRecvBytes)); - ui->peerConnTime->setText(GUIUtil::formatDurationStr(GetSystemTimeInSeconds() - stats->nodeStats.nTimeConnected)); ui->peerPingTime->setText(GUIUtil::formatPingTime(stats->nodeStats.m_ping_usec)); ui->peerMinPing->setText(GUIUtil::formatPingTime(stats->nodeStats.m_min_ping_usec)); ui->timeoffset->setText(GUIUtil::formatTimeOffset(stats->nodeStats.nTimeOffset)); diff --git a/src/qt/rpcconsole.h b/src/qt/rpcconsole.h index febc46c9b17e..79af6df9a378 100644 --- a/src/qt/rpcconsole.h +++ b/src/qt/rpcconsole.h @@ -194,6 +194,11 @@ public Q_SLOTS: /** Update UI with latest network info from model. */ void updateNetworkState(); + /** Helper for the output of a time duration field. Inputs are UNIX epoch times. */ + QString TimeDurationField(uint64_t time_now, uint64_t time_at_event) const { + return time_at_event ? GUIUtil::formatDurationStr(time_now - time_at_event) : tr("Never"); + } + private Q_SLOTS: void updateAlerts(const QString& warnings); }; From 62a7311fe4d007d03ec5e608f3858c7ef51bb0db Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Wed, 3 Apr 2024 16:10:14 +0000 Subject: [PATCH 08/19] merge bitcoin#21015: Make all of net_processing (and some of net) use std::chrono types --- src/net.cpp | 39 +++++------ src/net.h | 32 ++++----- src/net_processing.cpp | 113 ++++++++++++++++++-------------- src/net_processing.h | 2 +- src/qt/guiutil.cpp | 8 ++- src/qt/guiutil.h | 6 +- src/qt/peertablemodel.cpp | 4 +- src/qt/rpcconsole.cpp | 6 +- src/rpc/net.cpp | 12 ++-- src/test/fuzz/connman.cpp | 4 +- src/test/fuzz/node_eviction.cpp | 22 +++---- src/test/net_tests.cpp | 26 ++------ src/util/time.h | 13 +++- 13 files changed, 148 insertions(+), 139 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 536061463864..adb21b1908fa 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -684,8 +684,8 @@ void CNode::copyStats(CNodeStats &stats, const std::vector &m_asmap) X(m_legacyWhitelisted); X(m_permissionFlags); - stats.m_ping_usec = m_last_ping_time; - stats.m_min_ping_usec = m_min_ping_time; + X(m_last_ping_time); + X(m_min_ping_time); // Leave string empty if addrLocal invalid (not filled in yet) CService addrLocalUnlocked = GetAddrLocal(); @@ -1469,8 +1469,9 @@ void CConnman::CalculateNumConnectionsChangedStats() ipv6Nodes++; if(pnode->addr.IsTor()) torNodes++; - if(pnode->m_last_ping_time > 0) - statsClient.timing("peers.ping_us", pnode->m_last_ping_time, 1.0f); + const auto last_ping_time = count_microseconds(pnode->m_last_ping_time); + if (last_ping_time > 0) + statsClient.timing("peers.ping_us", last_ping_time, 1.0f); } ReleaseNodeVector(vNodesCopy); for (const std::string &msg : getAllNetMessageTypes()) { @@ -2283,12 +2284,11 @@ void CConnman::ThreadOpenConnections(const std::vector connect) } // Initiate network connections - auto start = GetTime(); + auto start = GetTime(); // Minimum time before next feeler connection (in microseconds). - - int64_t nNextFeeler = PoissonNextSend(count_microseconds(start), FEELER_INTERVAL); - int64_t nNextExtraBlockRelay = PoissonNextSend(count_microseconds(start), EXTRA_BLOCK_RELAY_ONLY_PEER_INTERVAL); + auto next_feeler = PoissonNextSend(start, FEELER_INTERVAL); + auto next_extra_block_relay = PoissonNextSend(start, EXTRA_BLOCK_RELAY_ONLY_PEER_INTERVAL); const bool dnsseed = gArgs.GetBoolArg("-dnsseed", DEFAULT_DNSSEED); bool add_fixed_seeds = gArgs.GetBoolArg("-fixedseeds", DEFAULT_FIXEDSEEDS); @@ -2382,7 +2382,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect) } ConnectionType conn_type = ConnectionType::OUTBOUND_FULL_RELAY; - int64_t nTime = GetTimeMicros(); + auto now = GetTime(); bool anchor = false; bool fFeeler = false; @@ -2394,7 +2394,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect) // GetTryNewOutboundPeer() gets set when a stale tip is detected, so we // try opening an additional OUTBOUND_FULL_RELAY connection. If none of // these conditions are met, check to see if it's time to try an extra - // block-relay-only peer (to confirm our tip is current, see below) or the nNextFeeler + // block-relay-only peer (to confirm our tip is current, see below) or the next_feeler // timer to decide if we should open a FEELER. if (!m_anchors.empty() && (nOutboundBlockRelay < m_max_outbound_block_relay)) { @@ -2406,7 +2406,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect) conn_type = ConnectionType::BLOCK_RELAY; } else if (GetTryNewOutboundPeer()) { // OUTBOUND_FULL_RELAY - } else if (nTime > nNextExtraBlockRelay && m_start_extra_block_relay_peers) { + } else if (now > next_extra_block_relay && m_start_extra_block_relay_peers) { // Periodically connect to a peer (using regular outbound selection // methodology from addrman) and stay connected long enough to sync // headers, but not much else. @@ -2428,10 +2428,10 @@ void CConnman::ThreadOpenConnections(const std::vector connect) // Because we can promote these connections to block-relay-only // connections, they do not get their own ConnectionType enum // (similar to how we deal with extra outbound peers). - nNextExtraBlockRelay = PoissonNextSend(nTime, EXTRA_BLOCK_RELAY_ONLY_PEER_INTERVAL); + next_extra_block_relay = PoissonNextSend(now, EXTRA_BLOCK_RELAY_ONLY_PEER_INTERVAL); conn_type = ConnectionType::BLOCK_RELAY; - } else if (nTime > nNextFeeler) { - nNextFeeler = PoissonNextSend(nTime, FEELER_INTERVAL); + } else if (now > next_feeler) { + next_feeler = PoissonNextSend(now, FEELER_INTERVAL); conn_type = ConnectionType::FEELER; fFeeler = true; } else { @@ -4150,20 +4150,21 @@ bool CConnman::IsMasternodeOrDisconnectRequested(const CService& addr) { }); } -int64_t CConnman::PoissonNextSendInbound(int64_t now, int average_interval_seconds) +std::chrono::microseconds CConnman::PoissonNextSendInbound(std::chrono::microseconds now, std::chrono::seconds average_interval) { - if (m_next_send_inv_to_incoming < now) { + if (m_next_send_inv_to_incoming.load() < now) { // If this function were called from multiple threads simultaneously // it would possible that both update the next send variable, and return a different result to their caller. // This is not possible in practice as only the net processing thread invokes this function. - m_next_send_inv_to_incoming = PoissonNextSend(now, average_interval_seconds); + m_next_send_inv_to_incoming = PoissonNextSend(now, average_interval); } return m_next_send_inv_to_incoming; } -int64_t PoissonNextSend(int64_t now, int average_interval_seconds) +std::chrono::microseconds PoissonNextSend(std::chrono::microseconds now, std::chrono::seconds average_interval) { - return now + (int64_t)(log1p(GetRand(1ULL << 48) * -0.0000000000000035527136788 /* -1/2^48 */) * average_interval_seconds * -1000000.0 + 0.5); + double unscaled = -log1p(GetRand(1ULL << 48) * -0.0000000000000035527136788 /* -1/2^48 */); + return now + std::chrono::duration_cast(unscaled * average_interval + 0.5us); } std::vector CConnman::CopyNodeVector(std::function cond) diff --git a/src/net.h b/src/net.h index 86c7e82a3e25..68116bba9c04 100644 --- a/src/net.h +++ b/src/net.h @@ -63,12 +63,12 @@ static const int TIMEOUT_INTERVAL = 20 * 60; static const int PROBE_WAIT_INTERVAL = 5; /** Minimum time between warnings printed to log. */ static const int WARNING_INTERVAL = 10 * 60; -/** Run the feeler connection loop once every 2 minutes or 120 seconds. **/ -static const int FEELER_INTERVAL = 120; +/** Run the feeler connection loop once every 2 minutes. **/ +static constexpr auto FEELER_INTERVAL = 2min; /** The maximum number of entries in an 'inv' protocol message */ static const unsigned int MAX_INV_SZ = 50000; /** Run the extra block-relay-only connection loop once every 5 minutes. **/ -static const int EXTRA_BLOCK_RELAY_ONLY_PEER_INTERVAL = 300; +static constexpr auto EXTRA_BLOCK_RELAY_ONLY_PEER_INTERVAL = 5min; /** The maximum number of addresses from our addrman to return in response to a getaddr message. */ static constexpr size_t MAX_ADDR_TO_SEND = 1000; /** Maximum length of incoming protocol messages (no message over 3 MiB is currently acceptable). */ @@ -288,8 +288,8 @@ class CNodeStats mapMsgCmdSize mapRecvBytesPerMsgCmd; NetPermissionFlags m_permissionFlags; bool m_legacyWhitelisted; - int64_t m_ping_usec; - int64_t m_min_ping_usec; + std::chrono::microseconds m_last_ping_time; + std::chrono::microseconds m_min_ping_time; // Our address, as reported by the peer std::string addrLocal; // Address of this peer @@ -646,11 +646,11 @@ class CNode std::atomic nLastTXTime{0}; /** Last measured round-trip time. Used only for RPC/GUI stats/debugging.*/ - std::atomic m_last_ping_time{0}; + std::atomic m_last_ping_time{0us}; /** Lowest measured round-trip time. Used as an inbound peer eviction * criterium in CConnman::AttemptToEvictConnection. */ - std::atomic m_min_ping_time{std::numeric_limits::max()}; + std::atomic m_min_ping_time{std::chrono::microseconds::max()}; // If true, we will send him CoinJoin queue messages std::atomic fSendDSQueue{false}; @@ -838,8 +838,8 @@ class CNode /** A ping-pong round trip has completed successfully. Update latest and minimum ping times. */ void PongReceived(std::chrono::microseconds ping_time) { - m_last_ping_time = count_microseconds(ping_time); - m_min_ping_time = std::min(m_min_ping_time.load(), count_microseconds(ping_time)); + m_last_ping_time = ping_time; + m_min_ping_time = std::min(m_min_ping_time.load(), ping_time); } /** Whether this peer is an inbound onion, e.g. connected via our Tor onion service. */ @@ -1300,7 +1300,7 @@ friend class CNode; Works assuming that a single interval is used. Variable intervals will result in privacy decrease. */ - int64_t PoissonNextSendInbound(int64_t now, int average_interval_seconds); + std::chrono::microseconds PoissonNextSendInbound(std::chrono::microseconds now, std::chrono::seconds average_interval); void SetAsmap(std::vector asmap) { addrman.m_asmap = std::move(asmap); } @@ -1579,7 +1579,7 @@ friend class CNode; */ std::atomic_bool m_start_extra_block_relay_peers{false}; - std::atomic m_next_send_inv_to_incoming{0}; + std::atomic m_next_send_inv_to_incoming{0us}; /** * A vector of -bind=
:=onion arguments each of which is @@ -1592,13 +1592,7 @@ friend class CNode; }; /** Return a timestamp in the future (in microseconds) for exponentially distributed events. */ -int64_t PoissonNextSend(int64_t now, int average_interval_seconds); - -/** Wrapper to return mockable type */ -inline std::chrono::microseconds PoissonNextSend(std::chrono::microseconds now, std::chrono::seconds average_interval) -{ - return std::chrono::microseconds{PoissonNextSend(now.count(), average_interval.count())}; -} +std::chrono::microseconds PoissonNextSend(std::chrono::microseconds now, std::chrono::seconds average_interval); /** Dump binary message to file, with timestamp */ void CaptureMessage(const CAddress& addr, const std::string& msg_type, const Span& data, bool is_incoming); @@ -1607,7 +1601,7 @@ struct NodeEvictionCandidate { NodeId id; int64_t nTimeConnected; - int64_t m_min_ping_time; + std::chrono::microseconds m_min_ping_time; int64_t nLastBlockTime; int64_t nLastTXTime; bool fRelevantServices; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index df03622009eb..39a0483552e4 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -87,13 +87,13 @@ static constexpr int64_t ORPHAN_TX_EXPIRE_TIME = 20 * 60; /** Minimum time between orphan transactions expire time checks in seconds */ static constexpr int64_t ORPHAN_TX_EXPIRE_INTERVAL = 5 * 60; /** How long to cache transactions in mapRelay for normal relay */ -static constexpr std::chrono::seconds RELAY_TX_CACHE_TIME = std::chrono::minutes{15}; +static constexpr auto RELAY_TX_CACHE_TIME = 15min; /** How long a transaction has to be in the mempool before it can unconditionally be relayed (even when not in mapRelay). */ -static constexpr std::chrono::seconds UNCONDITIONAL_RELAY_DELAY = std::chrono::minutes{2}; -/** Headers download timeout expressed in microseconds +static constexpr auto UNCONDITIONAL_RELAY_DELAY = 2min; +/** Headers download timeout. * Timeout = base + per_header * (expected number of headers) */ -static constexpr int64_t HEADERS_DOWNLOAD_TIMEOUT_BASE = 15 * 60 * 1000000; // 15 minutes -static constexpr int64_t HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER = 1000; // 1ms/header +static constexpr auto HEADERS_DOWNLOAD_TIMEOUT_BASE = 15min; +static constexpr auto HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER = 1ms; /** Protect at least this many outbound peers from disconnection due to slow/ * behind headers chain. */ @@ -120,8 +120,8 @@ static constexpr std::chrono::minutes PING_INTERVAL{2}; static const unsigned int MAX_LOCATOR_SZ = 101; /** Number of blocks that can be requested at any given time from a single peer. */ static const int MAX_BLOCKS_IN_TRANSIT_PER_PEER = 16; -/** Timeout in seconds during which a peer must stall block download progress before being disconnected. */ -static const unsigned int BLOCK_STALLING_TIMEOUT = 2; +/** Time during which a peer must stall block download progress before being disconnected. */ +static constexpr auto BLOCK_STALLING_TIMEOUT = 2s; /** Maximum depth of blocks we're willing to serve as compact blocks to peers * when requested. For older blocks, a regular BLOCK response will be sent. */ static const int MAX_CMPCTBLOCK_DEPTH = 5; @@ -132,31 +132,34 @@ static const int MAX_BLOCKTXN_DEPTH = 10; * degree of disordering of blocks on disk (which make reindexing and pruning harder). We'll probably * want to make this a per-peer adaptive value at some point. */ static const unsigned int BLOCK_DOWNLOAD_WINDOW = 1024; -/** Block download timeout base, expressed in millionths of the block interval (i.e. 10 min) */ -static const int64_t BLOCK_DOWNLOAD_TIMEOUT_BASE = 1000000; +/** Block download timeout base, expressed in multiples of the block interval (i.e. 10 min) */ +static constexpr double BLOCK_DOWNLOAD_TIMEOUT_BASE = 1; /** Additional block download timeout per parallel downloading peer (i.e. 5 min) */ -static const int64_t BLOCK_DOWNLOAD_TIMEOUT_PER_PEER = 500000; +static constexpr double BLOCK_DOWNLOAD_TIMEOUT_PER_PEER = 0.5; /** Maximum number of headers to announce when relaying blocks with headers message.*/ static const unsigned int MAX_BLOCKS_TO_ANNOUNCE = 8; /** Maximum number of unconnecting headers announcements before DoS score */ static const int MAX_UNCONNECTING_HEADERS = 10; /** Minimum blocks required to signal NODE_NETWORK_LIMITED */ static const unsigned int NODE_NETWORK_LIMITED_MIN_BLOCKS = 288; - -/** Average delay between local address broadcasts in seconds. */ -static constexpr std::chrono::hours AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL{24}; -/** Average delay between peer address broadcasts in seconds. */ -static constexpr std::chrono::seconds AVG_ADDRESS_BROADCAST_INTERVAL{30}; -/** Average delay between trickled inventory transmissions in seconds. - * Blocks and peers with noban permission bypass this, regular outbound peers get half this delay, - * Masternode outbound peers get quarter this delay. */ -static const unsigned int INVENTORY_BROADCAST_INTERVAL = 5; +/** Average delay between local address broadcasts */ +static constexpr auto AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL = 24h; +/** Average delay between peer address broadcasts */ +static constexpr auto AVG_ADDRESS_BROADCAST_INTERVAL = 30s; +/** Average delay between trickled inventory transmissions for inbound peers. + * Blocks and peers with noban permission bypass this. */ +static constexpr auto INBOUND_INVENTORY_BROADCAST_INTERVAL = 5s; +/** Average delay between trickled inventory transmissions for outbound peers. + * Use a smaller delay as there is less privacy concern for them. + * Blocks and peers with noban permission bypass this. + * Masternode outbound peers get half this delay. */ +static constexpr auto OUTBOUND_INVENTORY_BROADCAST_INTERVAL = 2s; /** Maximum rate of inventory items to send per second. * Limits the impact of low-fee transaction floods. * We have 4 times smaller block times in Dash, so we need to push 4 times more invs per 1MB. */ static constexpr unsigned int INVENTORY_BROADCAST_PER_SECOND = 7; /** Maximum number of inventory items to send per transmission. */ -static constexpr unsigned int INVENTORY_BROADCAST_MAX_PER_1MB_BLOCK = 4 * INVENTORY_BROADCAST_PER_SECOND * INVENTORY_BROADCAST_INTERVAL; +static constexpr unsigned int INVENTORY_BROADCAST_MAX_PER_1MB_BLOCK = 4 * INVENTORY_BROADCAST_PER_SECOND * count_seconds(INBOUND_INVENTORY_BROADCAST_INTERVAL); /** The number of most recently announced transactions a peer can request. */ static constexpr unsigned int INVENTORY_MAX_RECENT_RELAY = 3500; /** Verify that INVENTORY_MAX_RECENT_RELAY is enough to cache everything typically @@ -564,7 +567,7 @@ class PeerManagerImpl final : public PeerManager typedef std::map MapRelay; MapRelay mapRelay GUARDED_BY(cs_main); /** Expiration-time ordered list of (expire time, relay map entry) pairs. */ - std::deque> vRelayExpiration GUARDED_BY(cs_main); + std::deque> g_relay_expiration GUARDED_BY(cs_main); /** * When a peer sends us a valid block, instruct it to announce blocks to us @@ -633,12 +636,12 @@ struct CNodeState { //! Whether we've started headers synchronization with this peer. bool fSyncStarted; //! When to potentially disconnect peer for stalling headers download - int64_t nHeadersSyncTimeout; + std::chrono::microseconds m_headers_sync_timeout{0us}; //! Since when we're stalling block download progress (in microseconds), or 0. - int64_t nStallingSince; + std::chrono::microseconds m_stalling_since{0us}; std::list vBlocksInFlight; //! When the first entry in vBlocksInFlight started downloading. Don't care when vBlocksInFlight is empty. - int64_t nDownloadingSince; + std::chrono::microseconds m_downloading_since{0us}; int nBlocksInFlight; int nBlocksInFlightValidHeaders; //! Whether we consider this a preferred download peer. @@ -775,9 +778,6 @@ struct CNodeState { pindexBestHeaderSent = nullptr; nUnconnectingHeaders = 0; fSyncStarted = false; - nHeadersSyncTimeout = 0; - nStallingSince = 0; - nDownloadingSince = 0; nBlocksInFlight = 0; nBlocksInFlightValidHeaders = 0; fPreferredDownload = false; @@ -829,11 +829,11 @@ bool PeerManagerImpl::MarkBlockAsReceived(const uint256& hash) } if (state->vBlocksInFlight.begin() == itInFlight->second.second) { // First block on the queue was received, update the start download time for the next one - state->nDownloadingSince = std::max(state->nDownloadingSince, count_microseconds(GetTime())); + state->m_downloading_since = std::max(state->m_downloading_since, GetTime()); } state->vBlocksInFlight.erase(itInFlight->second.second); state->nBlocksInFlight--; - state->nStallingSince = 0; + state->m_stalling_since = 0us; mapBlocksInFlight.erase(itInFlight); return true; } @@ -863,7 +863,7 @@ bool PeerManagerImpl::MarkBlockAsInFlight(NodeId nodeid, const uint256& hash, co state->nBlocksInFlightValidHeaders += it->fValidatedHeaders; if (state->nBlocksInFlight == 1) { // We're starting a block download (batch) from this peer. - state->nDownloadingSince = GetTime().count(); + state->m_downloading_since = GetTime(); } if (state->nBlocksInFlightValidHeaders == 1 && pindex != nullptr) { nPeersWithValidatedDownloads++; @@ -1378,7 +1378,7 @@ bool PeerManagerImpl::GetNodeStateStats(NodeId nodeid, CNodeStateStats &stats) ping_wait = GetTime() - peer->m_ping_start.load(); } - stats.m_ping_wait_usec = count_microseconds(ping_wait); + stats.m_ping_wait = ping_wait; return true; } @@ -3449,7 +3449,13 @@ void PeerManagerImpl::ProcessMessage( if ((nSyncStarted == 0 && fFetch) || pindexBestHeader->GetBlockTime() > GetAdjustedTime() - nMaxTipAge) { // Make sure to mark this peer as the one we are currently syncing with etc. state->fSyncStarted = true; - state->nHeadersSyncTimeout = GetTimeMicros() + HEADERS_DOWNLOAD_TIMEOUT_BASE + HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER * (GetAdjustedTime() - pindexBestHeader->GetBlockTime())/(m_chainparams.GetConsensus().nPowTargetSpacing); + state->m_headers_sync_timeout = current_time + HEADERS_DOWNLOAD_TIMEOUT_BASE + + ( + // Convert HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER to microseconds before scaling + // to maintain precision + std::chrono::microseconds{HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER} * + (GetAdjustedTime() - pindexBestHeader->GetBlockTime()) / m_chainparams.GetConsensus().nPowTargetSpacing + ); nSyncStarted++; // Headers-first is the primary method of announcement on // the network. If a node fell back to sending blocks by inv, @@ -5025,7 +5031,13 @@ bool PeerManagerImpl::SendMessages(CNode* pto) // Only actively request headers from a single peer, unless we're close to end of initial download. if ((nSyncStarted == 0 && fFetch) || pindexBestHeader->GetBlockTime() > GetAdjustedTime() - nMaxTipAge) { state.fSyncStarted = true; - state.nHeadersSyncTimeout = count_microseconds(current_time) + HEADERS_DOWNLOAD_TIMEOUT_BASE + HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER * (GetAdjustedTime() - pindexBestHeader->GetBlockTime())/(consensusParams.nPowTargetSpacing); + state.m_headers_sync_timeout = current_time + HEADERS_DOWNLOAD_TIMEOUT_BASE + + ( + // Convert HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER to microseconds before scaling + // to maintain precision + std::chrono::microseconds{HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER} * + (GetAdjustedTime() - pindexBestHeader->GetBlockTime()) / consensusParams.nPowTargetSpacing + ); nSyncStarted++; const CBlockIndex *pindexStart = pindexBestHeader; /* If possible, start at the block preceding the currently @@ -5246,11 +5258,12 @@ bool PeerManagerImpl::SendMessages(CNode* pto) if (pto->m_tx_relay->nNextInvSend < current_time) { fSendTrickle = true; if (pto->IsInboundConn()) { - pto->m_tx_relay->nNextInvSend = std::chrono::microseconds{m_connman.PoissonNextSendInbound(count_microseconds(current_time), INVENTORY_BROADCAST_INTERVAL)}; + pto->m_tx_relay->nNextInvSend = m_connman.PoissonNextSendInbound(current_time, INBOUND_INVENTORY_BROADCAST_INTERVAL); } else { - // Use half the delay for regular outbound peers, as there is less privacy concern for them. - // and quarter the delay for Masternode outbound peers, as there is even less privacy concern in this case. - pto->m_tx_relay->nNextInvSend = PoissonNextSend(current_time, std::chrono::seconds{INVENTORY_BROADCAST_INTERVAL >> 1 >> !pto->GetVerifiedProRegTxHash().IsNull()}); + // Use half the delay for Masternode outbound peers, as there is less privacy concern for them. + pto->m_tx_relay->nNextInvSend = pto->GetVerifiedProRegTxHash().IsNull() ? + PoissonNextSend(current_time, OUTBOUND_INVENTORY_BROADCAST_INTERVAL) : + PoissonNextSend(current_time, OUTBOUND_INVENTORY_BROADCAST_INTERVAL / 2); } } @@ -5332,15 +5345,15 @@ bool PeerManagerImpl::SendMessages(CNode* pto) nRelayedTransactions++; { // Expire old relay messages - while (!vRelayExpiration.empty() && vRelayExpiration.front().first < count_microseconds(current_time)) + while (!g_relay_expiration.empty() && g_relay_expiration.front().first < current_time) { - mapRelay.erase(vRelayExpiration.front().second); - vRelayExpiration.pop_front(); + mapRelay.erase(g_relay_expiration.front().second); + g_relay_expiration.pop_front(); } auto ret = mapRelay.emplace(hash, std::move(txinfo.tx)); if (ret.second) { - vRelayExpiration.emplace_back(count_microseconds(current_time + std::chrono::microseconds{RELAY_TX_CACHE_TIME}), ret.first); + g_relay_expiration.emplace_back(current_time + RELAY_TX_CACHE_TIME, ret.first); } } int nInvType = m_cj_ctx->dstxman->GetDSTX(hash) ? MSG_DSTX : MSG_TX; @@ -5375,7 +5388,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) // Detect whether we're stalling current_time = GetTime(); - if (state.nStallingSince && state.nStallingSince < count_microseconds(current_time) - 1000000 * BLOCK_STALLING_TIMEOUT) { + if (state.m_stalling_since.count() && state.m_stalling_since < current_time - BLOCK_STALLING_TIMEOUT) { // Stalling only triggers when the block download window cannot move. During normal steady state, // the download window should be much larger than the to-be-downloaded set of blocks, so disconnection // should only happen during initial block download. @@ -5383,7 +5396,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) pto->fDisconnect = true; return true; } - // In case there is a block that has been in flight from this peer for 2 + 0.5 * N times the block interval + // In case there is a block that has been in flight from this peer for block_interval * (1 + 0.5 * N) // (with N the number of peers from which we're downloading validated blocks), disconnect due to timeout. // We compensate for other peers to prevent killing off peers due to our own downstream link // being saturated. We only count validated in-flight blocks so peers can't advertise non-existing block hashes @@ -5391,17 +5404,17 @@ bool PeerManagerImpl::SendMessages(CNode* pto) if (state.vBlocksInFlight.size() > 0) { QueuedBlock &queuedBlock = state.vBlocksInFlight.front(); int nOtherPeersWithValidatedDownloads = nPeersWithValidatedDownloads - (state.nBlocksInFlightValidHeaders > 0); - if (count_microseconds(current_time) > state.nDownloadingSince + consensusParams.nPowTargetSpacing * (BLOCK_DOWNLOAD_TIMEOUT_BASE + BLOCK_DOWNLOAD_TIMEOUT_PER_PEER * nOtherPeersWithValidatedDownloads)) { + if (current_time > state.m_downloading_since + std::chrono::seconds{consensusParams.nPowTargetSpacing} * (BLOCK_DOWNLOAD_TIMEOUT_BASE + BLOCK_DOWNLOAD_TIMEOUT_PER_PEER * nOtherPeersWithValidatedDownloads)) { LogPrintf("Timeout downloading block %s from peer=%d, disconnecting\n", queuedBlock.hash.ToString(), pto->GetId()); pto->fDisconnect = true; return true; } } // Check for headers sync timeouts - if (state.fSyncStarted && state.nHeadersSyncTimeout < std::numeric_limits::max()) { + if (state.fSyncStarted && state.m_headers_sync_timeout < std::chrono::microseconds::max()) { // Detect whether this is a stalling initial-headers-sync peer if (pindexBestHeader->GetBlockTime() <= GetAdjustedTime() - nMaxTipAge) { - if (count_microseconds(current_time) > state.nHeadersSyncTimeout && nSyncStarted == 1 && (nPreferredDownload - state.fPreferredDownload >= 1)) { + if (current_time > state.m_headers_sync_timeout && nSyncStarted == 1 && (nPreferredDownload - state.fPreferredDownload >= 1)) { // Disconnect a peer (without the noban permission) if it is our only sync peer, // and we have others we could be using instead. // Note: If all our peers are inbound, then we won't @@ -5420,13 +5433,13 @@ bool PeerManagerImpl::SendMessages(CNode* pto) // this peer (eventually). state.fSyncStarted = false; nSyncStarted--; - state.nHeadersSyncTimeout = 0; + state.m_headers_sync_timeout = 0us; } } } else { // After we've caught up once, reset the timeout so we can't trigger // disconnect later. - state.nHeadersSyncTimeout = std::numeric_limits::max(); + state.m_headers_sync_timeout = std::chrono::microseconds::max(); } } @@ -5449,8 +5462,8 @@ bool PeerManagerImpl::SendMessages(CNode* pto) pindex->nHeight, pto->GetId()); } if (state.nBlocksInFlight == 0 && staller != -1) { - if (State(staller)->nStallingSince == 0) { - State(staller)->nStallingSince = count_microseconds(current_time); + if (State(staller)->m_stalling_since == 0us) { + State(staller)->m_stalling_since = current_time; LogPrint(BCLog::NET, "Stall started peer=%d\n", staller); } } diff --git a/src/net_processing.h b/src/net_processing.h index 7605c3eebc77..d868d1025676 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -39,7 +39,7 @@ struct CNodeStateStats { int nSyncHeight = -1; int nCommonHeight = -1; int m_starting_height = -1; - int64_t m_ping_wait_usec; + std::chrono::microseconds m_ping_wait; std::vector vHeightInFlight; }; diff --git a/src/qt/guiutil.cpp b/src/qt/guiutil.cpp index 45abd5c220d3..705e3a952a2d 100644 --- a/src/qt/guiutil.cpp +++ b/src/qt/guiutil.cpp @@ -70,6 +70,8 @@ #include #include +#include + #if defined(Q_OS_MAC) #include @@ -1687,9 +1689,11 @@ QString formatServicesStr(quint64 mask) return QObject::tr("None"); } -QString formatPingTime(int64_t ping_usec) +QString formatPingTime(std::chrono::microseconds ping_time) { - return (ping_usec == std::numeric_limits::max() || ping_usec == 0) ? QObject::tr("N/A") : QString(QObject::tr("%1 ms")).arg(QString::number((int)(ping_usec / 1000), 10)); + return (ping_time == std::chrono::microseconds::max() || ping_time == 0us) ? + QObject::tr("N/A") : + QString(QObject::tr("%1 ms")).arg(QString::number((int)(count_microseconds(ping_time) / 1000), 10)); } QString formatTimeOffset(int64_t nTimeOffset) diff --git a/src/qt/guiutil.h b/src/qt/guiutil.h index 7db77634ea69..b5095becd26d 100644 --- a/src/qt/guiutil.h +++ b/src/qt/guiutil.h @@ -20,6 +20,8 @@ #include #include +#include + class QValidatedLineEdit; class OptionsModel; class SendCoinsRecipient; @@ -395,8 +397,8 @@ namespace GUIUtil /** Format CNodeStats.nServices bitmask into a user-readable string */ QString formatServicesStr(quint64 mask); - /** Format a CNodeStats.m_ping_usec into a user-readable string or display N/A, if 0 */ - QString formatPingTime(int64_t ping_usec); + /** Format a CNodeStats.m_last_ping_time into a user-readable string or display N/A, if 0 */ + QString formatPingTime(std::chrono::microseconds ping_time); /** Format a CNodeCombinedStats.nTimeOffset into a user-readable string */ QString formatTimeOffset(int64_t nTimeOffset); diff --git a/src/qt/peertablemodel.cpp b/src/qt/peertablemodel.cpp index 0d2a4ea9ce86..e1a4a1f46fdb 100644 --- a/src/qt/peertablemodel.cpp +++ b/src/qt/peertablemodel.cpp @@ -32,7 +32,7 @@ bool NodeLessThan::operator()(const CNodeCombinedStats &left, const CNodeCombine case PeerTableModel::Network: return pLeft->m_network < pRight->m_network; case PeerTableModel::Ping: - return pLeft->m_min_ping_usec < pRight->m_min_ping_usec; + return pLeft->m_min_ping_time < pRight->m_min_ping_time; case PeerTableModel::Sent: return pLeft->nSendBytes < pRight->nSendBytes; case PeerTableModel::Received: @@ -167,7 +167,7 @@ QVariant PeerTableModel::data(const QModelIndex &index, int role) const case Network: return GUIUtil::NetworkToQString(rec->nodeStats.m_network); case Ping: - return GUIUtil::formatPingTime(rec->nodeStats.m_min_ping_usec); + return GUIUtil::formatPingTime(rec->nodeStats.m_min_ping_time); case Sent: return GUIUtil::formatBytes(rec->nodeStats.nSendBytes); case Received: diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp index c1953e1273d5..a51f2f8c7019 100644 --- a/src/qt/rpcconsole.cpp +++ b/src/qt/rpcconsole.cpp @@ -1252,8 +1252,8 @@ void RPCConsole::updateNodeDetail(const CNodeCombinedStats *stats) ui->peerLastRecv->setText(TimeDurationField(time_now, stats->nodeStats.nLastRecv)); ui->peerBytesSent->setText(GUIUtil::formatBytes(stats->nodeStats.nSendBytes)); ui->peerBytesRecv->setText(GUIUtil::formatBytes(stats->nodeStats.nRecvBytes)); - ui->peerPingTime->setText(GUIUtil::formatPingTime(stats->nodeStats.m_ping_usec)); - ui->peerMinPing->setText(GUIUtil::formatPingTime(stats->nodeStats.m_min_ping_usec)); + ui->peerPingTime->setText(GUIUtil::formatPingTime(stats->nodeStats.m_last_ping_time)); + ui->peerMinPing->setText(GUIUtil::formatPingTime(stats->nodeStats.m_min_ping_time)); ui->timeoffset->setText(GUIUtil::formatTimeOffset(stats->nodeStats.nTimeOffset)); ui->peerVersion->setText(QString::number(stats->nodeStats.nVersion)); ui->peerSubversion->setText(QString::fromStdString(stats->nodeStats.cleanSubVer)); @@ -1302,7 +1302,7 @@ void RPCConsole::updateNodeDetail(const CNodeCombinedStats *stats) ui->peerCommonHeight->setText(tr("Unknown")); ui->peerHeight->setText(QString::number(stats->nodeStateStats.m_starting_height)); - ui->peerPingWait->setText(GUIUtil::formatPingTime(stats->nodeStateStats.m_ping_wait_usec)); + ui->peerPingWait->setText(GUIUtil::formatPingTime(stats->nodeStateStats.m_ping_wait)); } ui->detailWidget->show(); diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index ca6e3fef30e3..c80851e77bb6 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -213,14 +213,14 @@ static RPCHelpMan getpeerinfo() obj.pushKV("bytesrecv", stats.nRecvBytes); obj.pushKV("conntime", stats.nTimeConnected); obj.pushKV("timeoffset", stats.nTimeOffset); - if (stats.m_ping_usec > 0) { - obj.pushKV("pingtime", ((double)stats.m_ping_usec) / 1e6); + if (stats.m_last_ping_time > 0us) { + obj.pushKV("pingtime", CountSecondsDouble(stats.m_last_ping_time)); } - if (stats.m_min_ping_usec < std::numeric_limits::max()) { - obj.pushKV("minping", ((double)stats.m_min_ping_usec) / 1e6); + if (stats.m_min_ping_time < std::chrono::microseconds::max()) { + obj.pushKV("minping", CountSecondsDouble(stats.m_min_ping_time)); } - if (fStateStats && statestats.m_ping_wait_usec > 0) { - obj.pushKV("pingwait", ((double)statestats.m_ping_wait_usec) / 1e6); + if (fStateStats && statestats.m_ping_wait > 0s) { + obj.pushKV("pingwait", CountSecondsDouble(statestats.m_ping_wait)); } obj.pushKV("version", stats.nVersion); // Use the sanitized form of subver here, to avoid tricksy remote peers from diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index c5c399bba7b6..d7b0cde1be02 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -93,7 +93,9 @@ FUZZ_TARGET_INIT(connman, initialize_connman) }, [&] { // Limit now to int32_t to avoid signed integer overflow - (void)connman.PoissonNextSendInbound(fuzzed_data_provider.ConsumeIntegral(), fuzzed_data_provider.ConsumeIntegral()); + (void)connman.PoissonNextSendInbound( + std::chrono::microseconds{fuzzed_data_provider.ConsumeIntegral()}, + std::chrono::seconds{fuzzed_data_provider.ConsumeIntegral()}); }, [&] { CSerializedNetMsg serialized_net_msg; diff --git a/src/test/fuzz/node_eviction.cpp b/src/test/fuzz/node_eviction.cpp index 74b8e638eb54..603d520cf5b0 100644 --- a/src/test/fuzz/node_eviction.cpp +++ b/src/test/fuzz/node_eviction.cpp @@ -20,17 +20,17 @@ FUZZ_TARGET(node_eviction) std::vector eviction_candidates; while (fuzzed_data_provider.ConsumeBool()) { eviction_candidates.push_back({ - fuzzed_data_provider.ConsumeIntegral(), - fuzzed_data_provider.ConsumeIntegral(), - fuzzed_data_provider.ConsumeIntegral(), - fuzzed_data_provider.ConsumeIntegral(), - fuzzed_data_provider.ConsumeIntegral(), - fuzzed_data_provider.ConsumeBool(), - fuzzed_data_provider.ConsumeBool(), - fuzzed_data_provider.ConsumeBool(), - fuzzed_data_provider.ConsumeIntegral(), - fuzzed_data_provider.ConsumeBool(), - fuzzed_data_provider.ConsumeBool(), + /* id */ fuzzed_data_provider.ConsumeIntegral(), + /* nTimeConnected */ fuzzed_data_provider.ConsumeIntegral(), + /* m_min_ping_time */ std::chrono::microseconds{fuzzed_data_provider.ConsumeIntegral()}, + /* nLastBlockTime */ fuzzed_data_provider.ConsumeIntegral(), + /* nLastTXTime */ fuzzed_data_provider.ConsumeIntegral(), + /* fRelevantServices */ fuzzed_data_provider.ConsumeBool(), + /* fRelayTxes */ fuzzed_data_provider.ConsumeBool(), + /* fBloomFilter */ fuzzed_data_provider.ConsumeBool(), + /* nKeyedNetGroup */ fuzzed_data_provider.ConsumeIntegral(), + /* prefer_evict */ fuzzed_data_provider.ConsumeBool(), + /* m_is_local */ fuzzed_data_provider.ConsumeBool(), }); } // Make a copy since eviction_candidates may be in some valid but otherwise diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index a102228ab8aa..0090a4dae978 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -250,20 +250,6 @@ BOOST_AUTO_TEST_CASE(cnode_simple_test) BOOST_CHECK_EQUAL(pnode4->ConnectedThroughNetwork(), Network::NET_ONION); } -BOOST_AUTO_TEST_CASE(PoissonNextSend) -{ - g_mock_deterministic_tests = true; - int64_t now = 5000; - int average_interval_seconds = 600; - - auto poisson = ::PoissonNextSend(now, average_interval_seconds); - std::chrono::microseconds poisson_chrono = ::PoissonNextSend(std::chrono::microseconds{now}, std::chrono::seconds{average_interval_seconds}); - - BOOST_CHECK_EQUAL(poisson, poisson_chrono.count()); - - g_mock_deterministic_tests = false; -} - BOOST_AUTO_TEST_CASE(cnetaddr_basic) { CNetAddr addr; @@ -854,7 +840,7 @@ std::vector GetRandomNodeEvictionCandidates(const int n_c candidates.push_back({ /* id */ id, /* nTimeConnected */ static_cast(random_context.randrange(100)), - /* m_min_ping_time */ static_cast(random_context.randrange(100)), + /* m_min_ping_time */ std::chrono::microseconds{random_context.randrange(100)}, /* nLastBlockTime */ static_cast(random_context.randrange(100)), /* nLastTXTime */ static_cast(random_context.randrange(100)), /* fRelevantServices */ random_context.randbool(), @@ -914,7 +900,7 @@ BOOST_AUTO_TEST_CASE(node_eviction_test) // from eviction. BOOST_CHECK(!IsEvicted( number_of_nodes, [](NodeEvictionCandidate& candidate) { - candidate.m_min_ping_time = candidate.id; + candidate.m_min_ping_time = std::chrono::microseconds{candidate.id}; }, {0, 1, 2, 3, 4, 5, 6, 7}, random_context)); @@ -960,10 +946,10 @@ BOOST_AUTO_TEST_CASE(node_eviction_test) // Combination of all tests above. BOOST_CHECK(!IsEvicted( number_of_nodes, [number_of_nodes](NodeEvictionCandidate& candidate) { - candidate.nKeyedNetGroup = number_of_nodes - candidate.id; // 4 protected - candidate.m_min_ping_time = candidate.id; // 8 protected - candidate.nLastTXTime = number_of_nodes - candidate.id; // 4 protected - candidate.nLastBlockTime = number_of_nodes - candidate.id; // 4 protected + 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.nLastBlockTime = 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)); diff --git a/src/util/time.h b/src/util/time.h index 3e0f7d9771cb..3eefa353ae62 100644 --- a/src/util/time.h +++ b/src/util/time.h @@ -31,9 +31,16 @@ void UninterruptibleSleep(const std::chrono::microseconds& n); * This helper is used to convert durations before passing them over an * interface that doesn't support std::chrono (e.g. RPC, debug log, or the GUI) */ -inline int64_t count_seconds(std::chrono::seconds t) { return t.count(); } -inline int64_t count_milliseconds(std::chrono::milliseconds t) { return t.count(); } -inline int64_t count_microseconds(std::chrono::microseconds t) { return t.count(); } +constexpr int64_t count_seconds(std::chrono::seconds t) { return t.count(); } +constexpr int64_t count_milliseconds(std::chrono::milliseconds t) { return t.count(); } +constexpr int64_t count_microseconds(std::chrono::microseconds t) { return t.count(); } + +using SecondsDouble = std::chrono::duration; + +/** + * Helper to count the seconds in any std::chrono::duration type + */ +inline double CountSecondsDouble(SecondsDouble t) { return t.count(); } /** * DEPRECATED From 5c4c7c55f8ced43c2217378c185c7b0e1e0f4910 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Mon, 25 Mar 2024 16:46:31 +0000 Subject: [PATCH 09/19] merge bitcoin#19771: Replace enum CConnMan::NumConnections with enum class ConnectionDirection --- src/interfaces/node.h | 5 +++-- src/llmq/dkgsession.cpp | 2 +- src/net.cpp | 8 ++++---- src/net.h | 14 ++------------ src/netbase.h | 20 ++++++++++++++++++++ src/node/interfaces.cpp | 2 +- src/qt/clientmodel.cpp | 8 ++++---- src/rpc/mining.cpp | 2 +- src/rpc/net.cpp | 14 +++++++------- src/test/fuzz/connman.cpp | 2 +- 10 files changed, 44 insertions(+), 33 deletions(-) diff --git a/src/interfaces/node.h b/src/interfaces/node.h index 18dc638984dc..cc48d8f82859 100644 --- a/src/interfaces/node.h +++ b/src/interfaces/node.h @@ -6,9 +6,10 @@ #define BITCOIN_INTERFACES_NODE_H #include // For CAmount -#include // For CConnman::NumConnections +#include // For NodeId #include // For banmap_t #include // For Network +#include // For ConnectionDirection #include // For SecureString #include #include @@ -175,7 +176,7 @@ class Node virtual bool getProxy(Network net, proxyType& proxy_info) = 0; //! Get number of connections. - virtual size_t getNodeCount(CConnman::NumConnections flags) = 0; + virtual size_t getNodeCount(ConnectionDirection flags) = 0; //! Get stats for connected nodes. using NodesStats = std::vector>; diff --git a/src/llmq/dkgsession.cpp b/src/llmq/dkgsession.cpp index b819a593474f..82437a5c64bb 100644 --- a/src/llmq/dkgsession.cpp +++ b/src/llmq/dkgsession.cpp @@ -1316,7 +1316,7 @@ void CDKGSession::RelayInvToParticipants(const CInv& inv) const logger.Batch("RelayInvToParticipants inv[%s] relayMembers[%d] GetNodeCount[%d] GetNetworkActive[%d] HasMasternodeQuorumNodes[%d] for quorumHash[%s] forMember[%s] relayMembers[%s]", inv.ToString(), relayMembers.size(), - connman.GetNodeCount(CConnman::CONNECTIONS_ALL), + connman.GetNodeCount(ConnectionDirection::Both), connman.GetNetworkActive(), connman.HasMasternodeQuorumNodes(params.type, m_quorum_base_block_index->GetBlockHash()), m_quorum_base_block_index->GetBlockHash().ToString(), diff --git a/src/net.cpp b/src/net.cpp index adb21b1908fa..cbe857ca3712 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3790,7 +3790,7 @@ void CConnman::AddPendingProbeConnections(const std::set &proTxHashes) masternodePendingProbes.insert(proTxHashes.begin(), proTxHashes.end()); } -size_t CConnman::GetNodeCount(NumConnections flags) +size_t CConnman::GetNodeCount(ConnectionDirection flags) { LOCK(cs_vNodes); @@ -3799,12 +3799,12 @@ size_t CConnman::GetNodeCount(NumConnections flags) if (pnode->fDisconnect) { continue; } - if ((flags & CONNECTIONS_VERIFIED) && pnode->GetVerifiedProRegTxHash().IsNull()) { + if ((flags & ConnectionDirection::Verified) && pnode->GetVerifiedProRegTxHash().IsNull()) { continue; } - if (flags & (pnode->IsInboundConn() ? CONNECTIONS_IN : CONNECTIONS_OUT)) { + if (flags & (pnode->IsInboundConn() ? ConnectionDirection::In : ConnectionDirection::Out)) { nNum++; - } else if (flags == CONNECTIONS_VERIFIED) { + } else if (flags == ConnectionDirection::Verified) { nNum++; } } diff --git a/src/net.h b/src/net.h index 68116bba9c04..a9e53d25070c 100644 --- a/src/net.h +++ b/src/net.h @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -931,17 +932,6 @@ class CConnman { friend class CNode; public: - - enum NumConnections { - CONNECTIONS_NONE = 0, - CONNECTIONS_IN = (1U << 0), - CONNECTIONS_OUT = (1U << 1), - CONNECTIONS_ALL = (CONNECTIONS_IN | CONNECTIONS_OUT), - CONNECTIONS_VERIFIED = (1U << 2), - CONNECTIONS_VERIFIED_IN = (CONNECTIONS_VERIFIED | CONNECTIONS_IN), - CONNECTIONS_VERIFIED_OUT = (CONNECTIONS_VERIFIED | CONNECTIONS_OUT), - }; - enum SocketEventsMode { SOCKETEVENTS_SELECT = 0, SOCKETEVENTS_POLL = 1, @@ -1253,7 +1243,7 @@ friend class CNode; bool IsMasternodeQuorumRelayMember(const uint256& protxHash); void AddPendingProbeConnections(const std::set& proTxHashes); - size_t GetNodeCount(NumConnections num); + size_t GetNodeCount(ConnectionDirection); size_t GetMaxOutboundNodeCount(); void GetNodeStats(std::vector& vstats); bool DisconnectNode(const std::string& node); diff --git a/src/netbase.h b/src/netbase.h index 36c08f3d60aa..f40f0a5b3126 100644 --- a/src/netbase.h +++ b/src/netbase.h @@ -18,6 +18,7 @@ #include #include #include +#include #include extern int nConnectTimeout; @@ -29,6 +30,25 @@ static const int DEFAULT_CONNECT_TIMEOUT = 5000; static const int DEFAULT_NAME_LOOKUP = true; static const bool DEFAULT_ALLOWPRIVATENET = false; +enum class ConnectionDirection { + None = 0, + In = (1U << 0), + Out = (1U << 1), + Both = (In | Out), + Verified = (1U << 2), + VerifiedIn = (Verified | In), + VerifiedOut = (Verified | Out), +}; +static inline ConnectionDirection& operator|=(ConnectionDirection& a, ConnectionDirection b) { + using underlying = typename std::underlying_type::type; + a = ConnectionDirection(underlying(a) | underlying(b)); + return a; +} +static inline bool operator&(ConnectionDirection a, ConnectionDirection b) { + using underlying = typename std::underlying_type::type; + return (underlying(a) & underlying(b)); +} + class proxyType { public: diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 8f329543855d..406932ef55f3 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -335,7 +335,7 @@ class NodeImpl : public Node bool shutdownRequested() override { return ShutdownRequested(); } void mapPort(bool use_upnp, bool use_natpmp) override { StartMapPort(use_upnp, use_natpmp); } bool getProxy(Network net, proxyType& proxy_info) override { return GetProxy(net, proxy_info); } - size_t getNodeCount(CConnman::NumConnections flags) override + size_t getNodeCount(ConnectionDirection flags) override { return m_context->connman ? m_context->connman->GetNodeCount(flags) : 0; } diff --git a/src/qt/clientmodel.cpp b/src/qt/clientmodel.cpp index 143440bb3f02..a7485b1e713b 100644 --- a/src/qt/clientmodel.cpp +++ b/src/qt/clientmodel.cpp @@ -76,14 +76,14 @@ ClientModel::~ClientModel() int ClientModel::getNumConnections(unsigned int flags) const { - CConnman::NumConnections connections = CConnman::CONNECTIONS_NONE; + ConnectionDirection connections = ConnectionDirection::None; if(flags == CONNECTIONS_IN) - connections = CConnman::CONNECTIONS_IN; + connections = ConnectionDirection::In; else if (flags == CONNECTIONS_OUT) - connections = CConnman::CONNECTIONS_OUT; + connections = ConnectionDirection::Out; else if (flags == CONNECTIONS_ALL) - connections = CConnman::CONNECTIONS_ALL; + connections = ConnectionDirection::Both; return m_node.getNodeCount(connections); } diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index bb1d06c87d0a..ead13eddf3d1 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -734,7 +734,7 @@ static RPCHelpMan getblocktemplate() if(!node.connman) throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled"); - if (node.connman->GetNodeCount(CConnman::CONNECTIONS_ALL) == 0) + if (node.connman->GetNodeCount(ConnectionDirection::Both) == 0) throw JSONRPCError(RPC_CLIENT_NOT_CONNECTED, PACKAGE_NAME " is not connected!"); if (active_chainstate.IsInitialBlockDownload()) diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index c80851e77bb6..e906e4759af4 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -58,7 +58,7 @@ static RPCHelpMan getconnectioncount() if(!node.connman) throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled"); - return (int)node.connman->GetNodeCount(CConnman::CONNECTIONS_ALL); + return (int)node.connman->GetNodeCount(ConnectionDirection::Both); }, }; } @@ -649,12 +649,12 @@ static RPCHelpMan getnetworkinfo() obj.pushKV("timeoffset", GetTimeOffset()); if (node.connman) { obj.pushKV("networkactive", node.connman->GetNetworkActive()); - obj.pushKV("connections", (int)node.connman->GetNodeCount(CConnman::CONNECTIONS_ALL)); - obj.pushKV("connections_in", (int)node.connman->GetNodeCount(CConnman::CONNECTIONS_IN)); - obj.pushKV("connections_out", (int)node.connman->GetNodeCount(CConnman::CONNECTIONS_OUT)); - obj.pushKV("connections_mn", (int)node.connman->GetNodeCount(CConnman::CONNECTIONS_VERIFIED)); - obj.pushKV("connections_mn_in", (int)node.connman->GetNodeCount(CConnman::CONNECTIONS_VERIFIED_IN)); - obj.pushKV("connections_mn_out", (int)node.connman->GetNodeCount(CConnman::CONNECTIONS_VERIFIED_OUT)); + obj.pushKV("connections", (int)node.connman->GetNodeCount(ConnectionDirection::Both)); + obj.pushKV("connections_in", (int)node.connman->GetNodeCount(ConnectionDirection::In)); + obj.pushKV("connections_out", (int)node.connman->GetNodeCount(ConnectionDirection::Out)); + obj.pushKV("connections_mn", (int)node.connman->GetNodeCount(ConnectionDirection::Verified)); + obj.pushKV("connections_mn_in", (int)node.connman->GetNodeCount(ConnectionDirection::VerifiedIn)); + obj.pushKV("connections_mn_out", (int)node.connman->GetNodeCount(ConnectionDirection::VerifiedOut)); std::string strSocketEvents; switch (node.connman->GetSocketEventsMode()) { case CConnman::SOCKETEVENTS_SELECT: diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index d7b0cde1be02..5923b06afe23 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -86,7 +86,7 @@ FUZZ_TARGET_INIT(connman, initialize_connman) (void)connman.GetDeterministicRandomizer(fuzzed_data_provider.ConsumeIntegral()); }, [&] { - (void)connman.GetNodeCount(fuzzed_data_provider.PickValueInArray({CConnman::CONNECTIONS_NONE, CConnman::CONNECTIONS_IN, CConnman::CONNECTIONS_OUT, CConnman::CONNECTIONS_ALL})); + (void)connman.GetNodeCount(fuzzed_data_provider.PickValueInArray({ConnectionDirection::None, ConnectionDirection::In, ConnectionDirection::Out, ConnectionDirection::Both})); }, [&] { (void)connman.OutboundTargetReached(fuzzed_data_provider.ConsumeBool()); From ba1df91d8d0ef6261e45eef9b7ab3ba52e30def1 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Mon, 25 Mar 2024 15:41:22 +0000 Subject: [PATCH 10/19] merge bitcoin#21425: Pass PeerManagerImpl members only once --- src/net_processing.cpp | 102 ++++++++++++++++++----------------------- 1 file changed, 44 insertions(+), 58 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 39a0483552e4..b13f1460dbad 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -404,8 +404,8 @@ class PeerManagerImpl final : public PeerManager /** Check whether the last unknown block a peer advertised is not yet known. */ void ProcessBlockAvailability(NodeId nodeid) EXCLUSIVE_LOCKS_REQUIRED(cs_main); /** Update tracking information about which blocks a peer is assumed to have. */ - void UpdateBlockAvailability(NodeId nodeid, const uint256 &hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main); - bool CanDirectFetch(const Consensus::Params &consensusParams) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + void UpdateBlockAvailability(NodeId nodeid, const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + bool CanDirectFetch() EXCLUSIVE_LOCKS_REQUIRED(cs_main); /** * To prevent fingerprinting attacks, only send blocks/headers outside of the @@ -413,9 +413,9 @@ class PeerManagerImpl final : public PeerManager * best equivalent proof of work) than the best header chain we know about and * we fully-validated them at some point. */ - bool BlockRequestAllowed(const CBlockIndex* pindex, const Consensus::Params& consensusParams) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + bool BlockRequestAllowed(const CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); bool AlreadyHaveBlock(const uint256& block_hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main); - void ProcessGetBlockData(CNode& pfrom, Peer& peer, const CChainParams& chainparams, const CInv& inv, CConnman& connman, llmq::CInstantSendManager& isman); + void ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& inv, llmq::CInstantSendManager& isman); /** * Validation logic for compact filters request handling. @@ -423,7 +423,6 @@ class PeerManagerImpl final : public PeerManager * May disconnect from the peer in the case of a bad request. * * @param[in] peer The peer that we received the request from - * @param[in] chain_params Chain parameters * @param[in] filter_type The filter type the request is for. Must be basic filters. * @param[in] start_height The start height for the request * @param[in] stop_hash The stop_hash for the request @@ -432,7 +431,7 @@ class PeerManagerImpl final : public PeerManager * @param[out] filter_index The filter index, if the request can be serviced. * @return True if the request can be serviced. */ - bool PrepareBlockFilterRequest(CNode& peer, const CChainParams& chain_params, + bool PrepareBlockFilterRequest(CNode& peer, BlockFilterType filter_type, uint32_t start_height, const uint256& stop_hash, uint32_t max_height_diff, const CBlockIndex*& stop_index, @@ -445,11 +444,8 @@ class PeerManagerImpl final : public PeerManager * * @param[in] peer The peer that we received the request from * @param[in] vRecv The raw message received - * @param[in] chain_params Chain parameters - * @param[in] connman Pointer to the connection manager */ - void ProcessGetCFilters(CNode& peer, CDataStream& vRecv, const CChainParams& chain_params, - CConnman& connman); + void ProcessGetCFilters(CNode& peer, CDataStream& vRecv); /** * Handle a cfheaders request. @@ -458,11 +454,8 @@ class PeerManagerImpl final : public PeerManager * * @param[in] peer The peer that we received the request from * @param[in] vRecv The raw message received - * @param[in] chain_params Chain parameters - * @param[in] connman Pointer to the connection manager */ - void ProcessGetCFHeaders(CNode& peer, CDataStream& vRecv, const CChainParams& chain_params, - CConnman& connman); + void ProcessGetCFHeaders(CNode& peer, CDataStream& vRecv); /** * Handle a getcfcheckpt request. @@ -471,11 +464,8 @@ class PeerManagerImpl final : public PeerManager * * @param[in] peer The peer that we received the request from * @param[in] vRecv The raw message received - * @param[in] chain_params Chain parameters - * @param[in] connman Pointer to the connection manager */ - void ProcessGetCFCheckPt(CNode& peer, CDataStream& vRecv, const CChainParams& chain_params, - CConnman& connman); + void ProcessGetCFCheckPt(CNode& peer, CDataStream& vRecv); /** Number of nodes with fSyncStarted. */ int nSyncStarted GUARDED_BY(cs_main) = 0; @@ -919,9 +909,9 @@ bool PeerManagerImpl::TipMayBeStale() return m_last_tip_update < GetTime() - consensusParams.nPowTargetSpacing * 3 && mapBlocksInFlight.empty(); } -bool PeerManagerImpl::CanDirectFetch(const Consensus::Params &consensusParams) +bool PeerManagerImpl::CanDirectFetch() { - return m_chainman.ActiveChain().Tip()->GetBlockTime() > GetAdjustedTime() - consensusParams.nPowTargetSpacing * 20; + return m_chainman.ActiveChain().Tip()->GetBlockTime() > GetAdjustedTime() - m_chainparams.GetConsensus().nPowTargetSpacing * 20; } static bool PeerHasHeader(CNodeState *state, const CBlockIndex *pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main) @@ -1643,13 +1633,13 @@ bool PeerManagerImpl::MaybePunishNodeForTx(NodeId nodeid, const TxValidationStat return false; } -bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex* pindex, const Consensus::Params& consensusParams) +bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex* pindex) { AssertLockHeld(cs_main); if (m_chainman.ActiveChain().Contains(pindex)) return true; return pindex->IsValid(BLOCK_VALID_SCRIPTS) && (pindexBestHeader != nullptr) && (pindexBestHeader->GetBlockTime() - pindex->GetBlockTime() < STALE_RELAY_AGE_LIMIT) && - (GetBlockProofEquivalentTime(*pindexBestHeader, *pindex, *pindexBestHeader, consensusParams) < STALE_RELAY_AGE_LIMIT); + (GetBlockProofEquivalentTime(*pindexBestHeader, *pindex, *pindexBestHeader, m_chainparams.GetConsensus()) < STALE_RELAY_AGE_LIMIT); } std::unique_ptr PeerManager::make(const CChainParams& chainparams, CConnman& connman, CAddrMan& addrman, BanMan* banman, @@ -2061,12 +2051,11 @@ static void RelayAddress(const CNode& originator, connman.ForEachNodeThen(std::move(sortfunc), std::move(pushfunc)); } -void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CChainParams& chainparams, const CInv& inv, CConnman& connman, llmq::CInstantSendManager& isman) +void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& inv, llmq::CInstantSendManager& isman) { bool send = false; std::shared_ptr a_recent_block; std::shared_ptr a_recent_compact_block; - const Consensus::Params& consensusParams = chainparams.GetConsensus(); { LOCK(cs_most_recent_block); a_recent_block = most_recent_block; @@ -2099,7 +2088,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CChain LOCK(cs_main); const CBlockIndex* pindex = m_chainman.m_blockman.LookupBlockIndex(inv.hash); if (pindex) { - send = BlockRequestAllowed(pindex, consensusParams); + send = BlockRequestAllowed(pindex); if (!send) { LogPrint(BCLog::NET,"%s: ignoring request from peer=%i for old block that isn't in the main chain\n", __func__, pfrom.GetId()); } @@ -2107,7 +2096,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CChain const CNetMsgMaker msgMaker(pfrom.GetCommonVersion()); // disconnect node in case we have reached the outbound limit for serving historical blocks if (send && - connman.OutboundTargetReached(true) && + m_connman.OutboundTargetReached(true) && (((pindexBestHeader != nullptr) && (pindexBestHeader->GetBlockTime() - pindex->GetBlockTime() > HISTORICAL_BLOCK_AGE)) || inv.IsMsgFilteredBlk()) && !pfrom.HasPermission(PF_DOWNLOAD) // nodes with the download permission may exceed target ) { @@ -2137,13 +2126,13 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CChain } else { // Send block from disk std::shared_ptr pblockRead = std::make_shared(); - if (!ReadBlockFromDisk(*pblockRead, pindex, consensusParams)) + if (!ReadBlockFromDisk(*pblockRead, pindex, m_chainparams.GetConsensus())) assert(!"cannot load block from disk"); pblock = pblockRead; } if (pblock) { if (inv.IsMsgBlk()) { - connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::BLOCK, *pblock)); + m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::BLOCK, *pblock)); } else if (inv.IsMsgFilteredBlk()) { bool sendMerkleBlock = false; CMerkleBlock merkleBlock; @@ -2155,7 +2144,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CChain } } if (sendMerkleBlock) { - connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::MERKLEBLOCK, merkleBlock)); + m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::MERKLEBLOCK, merkleBlock)); // CMerkleBlock just contains hashes, so also push any transactions in the block the client did not see // This avoids hurting performance by pointlessly requiring a round-trip // Note that there is currently no way for a node to request any single transactions we didn't send here - @@ -2164,12 +2153,12 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CChain // however we MUST always provide at least what the remote peer needs typedef std::pair PairType; for (PairType &pair : merkleBlock.vMatchedTxn) { - connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::TX, *pblock->vtx[pair.first])); + m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::TX, *pblock->vtx[pair.first])); } for (PairType &pair : merkleBlock.vMatchedTxn) { auto islock = isman.GetInstantSendLockByTxid(pair.second); if (islock != nullptr) { - connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::ISDLOCK, *islock)); + m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::ISDLOCK, *islock)); } } } @@ -2180,17 +2169,17 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CChain // they won't have a useful mempool to match against a compact block, // and we don't feel like constructing the object for them, so // instead we respond with the full, non-compact block. - if (CanDirectFetch(consensusParams) && + if (CanDirectFetch() && pindex->nHeight >= m_chainman.ActiveChain().Height() - MAX_CMPCTBLOCK_DEPTH) { if (a_recent_compact_block && a_recent_compact_block->header.GetHash() == pindex->GetBlockHash()) { - connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::CMPCTBLOCK, *a_recent_compact_block)); + m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::CMPCTBLOCK, *a_recent_compact_block)); } else { CBlockHeaderAndShortTxIDs cmpctblock(*pblock); - connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::CMPCTBLOCK, cmpctblock)); + m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::CMPCTBLOCK, cmpctblock)); } } else { - connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::BLOCK, *pblock)); + m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::BLOCK, *pblock)); } } } @@ -2204,7 +2193,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CChain // wait for other stuff first. std::vector vInv; vInv.push_back(CInv(MSG_BLOCK, m_chainman.ActiveChain().Tip()->GetBlockHash())); - connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::INV, vInv)); + m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::INV, vInv)); peer.m_continuation_block.SetNull(); } } @@ -2432,7 +2421,7 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic if (it != peer.m_getdata_requests.end() && !pfrom.fPauseSend) { const CInv &inv = *it++; if (inv.IsGenBlkMsg()) { - ProcessGetBlockData(pfrom, peer, m_chainparams, inv, m_connman, *m_llmq_ctx->isman); + ProcessGetBlockData(pfrom, peer, inv, *m_llmq_ctx->isman); } // else: If the first item on the queue is an unknown type, we erase it // and continue processing the queue on the next call. @@ -2573,7 +2562,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, const Peer& peer, m_connman.PushMessage(&pfrom, msgMaker.Make(msg_type, m_chainman.ActiveChain().GetLocator(pindexLast), uint256())); } - bool fCanDirectFetch = CanDirectFetch(m_chainparams.GetConsensus()); + bool fCanDirectFetch = CanDirectFetch(); // If this set of headers is valid and ends in a block with at least as // much work as our tip, download as much as possible. if (fCanDirectFetch && pindexLast->IsValid(BLOCK_VALID_TREE) && m_chainman.ActiveChain().Tip()->nChainWork <= pindexLast->nChainWork) { @@ -2716,7 +2705,7 @@ void PeerManagerImpl::ProcessOrphanTx(std::set& orphan_work_set) m_mempool.check(m_chainman.ActiveChainstate()); } -bool PeerManagerImpl::PrepareBlockFilterRequest(CNode& peer, const CChainParams& chain_params, +bool PeerManagerImpl::PrepareBlockFilterRequest(CNode& peer, BlockFilterType filter_type, uint32_t start_height, const uint256& stop_hash, uint32_t max_height_diff, const CBlockIndex*& stop_index, @@ -2737,7 +2726,7 @@ bool PeerManagerImpl::PrepareBlockFilterRequest(CNode& peer, const CChainParams& stop_index = m_chainman.m_blockman.LookupBlockIndex(stop_hash); // Check that the stop block exists and the peer would be allowed to fetch it. - if (!stop_index || !BlockRequestAllowed(stop_index, chain_params.GetConsensus())) { + if (!stop_index || !BlockRequestAllowed(stop_index)) { LogPrint(BCLog::NET, "peer %d requested invalid block hash: %s\n", peer.GetId(), stop_hash.ToString()); peer.fDisconnect = true; @@ -2769,8 +2758,7 @@ bool PeerManagerImpl::PrepareBlockFilterRequest(CNode& peer, const CChainParams& return true; } -void PeerManagerImpl::ProcessGetCFilters(CNode& peer, CDataStream& vRecv, const CChainParams& chain_params, - CConnman& connman) +void PeerManagerImpl::ProcessGetCFilters(CNode& peer, CDataStream& vRecv) { uint8_t filter_type_ser; uint32_t start_height; @@ -2782,7 +2770,7 @@ void PeerManagerImpl::ProcessGetCFilters(CNode& peer, CDataStream& vRecv, const const CBlockIndex* stop_index; BlockFilterIndex* filter_index; - if (!PrepareBlockFilterRequest(peer, chain_params, filter_type, start_height, stop_hash, + if (!PrepareBlockFilterRequest(peer, filter_type, start_height, stop_hash, MAX_GETCFILTERS_SIZE, stop_index, filter_index)) { return; } @@ -2797,12 +2785,11 @@ void PeerManagerImpl::ProcessGetCFilters(CNode& peer, CDataStream& vRecv, const for (const auto& filter : filters) { CSerializedNetMsg msg = CNetMsgMaker(peer.GetCommonVersion()) .Make(NetMsgType::CFILTER, filter); - connman.PushMessage(&peer, std::move(msg)); + m_connman.PushMessage(&peer, std::move(msg)); } } -void PeerManagerImpl::ProcessGetCFHeaders(CNode& peer, CDataStream& vRecv, const CChainParams& chain_params, - CConnman& connman) +void PeerManagerImpl::ProcessGetCFHeaders(CNode& peer, CDataStream& vRecv) { uint8_t filter_type_ser; uint32_t start_height; @@ -2814,7 +2801,7 @@ void PeerManagerImpl::ProcessGetCFHeaders(CNode& peer, CDataStream& vRecv, const const CBlockIndex* stop_index; BlockFilterIndex* filter_index; - if (!PrepareBlockFilterRequest(peer, chain_params, filter_type, start_height, stop_hash, + if (!PrepareBlockFilterRequest(peer, filter_type, start_height, stop_hash, MAX_GETCFHEADERS_SIZE, stop_index, filter_index)) { return; } @@ -2843,11 +2830,10 @@ void PeerManagerImpl::ProcessGetCFHeaders(CNode& peer, CDataStream& vRecv, const stop_index->GetBlockHash(), prev_header, filter_hashes); - connman.PushMessage(&peer, std::move(msg)); + m_connman.PushMessage(&peer, std::move(msg)); } -void PeerManagerImpl::ProcessGetCFCheckPt(CNode& peer, CDataStream& vRecv, const CChainParams& chain_params, - CConnman& connman) +void PeerManagerImpl::ProcessGetCFCheckPt(CNode& peer, CDataStream& vRecv) { uint8_t filter_type_ser; uint256 stop_hash; @@ -2858,7 +2844,7 @@ void PeerManagerImpl::ProcessGetCFCheckPt(CNode& peer, CDataStream& vRecv, const const CBlockIndex* stop_index; BlockFilterIndex* filter_index; - if (!PrepareBlockFilterRequest(peer, chain_params, filter_type, /*start_height=*/0, stop_hash, + if (!PrepareBlockFilterRequest(peer, filter_type, /*start_height=*/0, stop_hash, /*max_height_diff=*/std::numeric_limits::max(), stop_index, filter_index)) { return; @@ -2884,7 +2870,7 @@ void PeerManagerImpl::ProcessGetCFCheckPt(CNode& peer, CDataStream& vRecv, const filter_type_ser, stop_index->GetBlockHash(), headers); - connman.PushMessage(&peer, std::move(msg)); + m_connman.PushMessage(&peer, std::move(msg)); } std::pair static ValidateDSTX(CDeterministicMNManager& dmnman, CDSTXManager& dstxman, ChainstateManager& chainman, @@ -3677,7 +3663,7 @@ void PeerManagerImpl::ProcessMessage( return; } - if (!BlockRequestAllowed(pindex, m_chainparams.GetConsensus())) { + if (!BlockRequestAllowed(pindex)) { LogPrint(BCLog::NET, "%s: ignoring request from peer=%i for old block header that isn't in the main chain\n", __func__, pfrom.GetId()); return; } @@ -3985,7 +3971,7 @@ void PeerManagerImpl::ProcessMessage( } // If we're not close to tip yet, give up and let parallel block fetch work its magic - if (!fAlreadyInFlight && !CanDirectFetch(m_chainparams.GetConsensus())) + if (!fAlreadyInFlight && !CanDirectFetch()) return; // We want to be a bit conservative just to be extra careful about DoS @@ -4464,17 +4450,17 @@ void PeerManagerImpl::ProcessMessage( } if (msg_type == NetMsgType::GETCFILTERS) { - ProcessGetCFilters(pfrom, vRecv, m_chainparams, m_connman); + ProcessGetCFilters(pfrom, vRecv); return; } if (msg_type == NetMsgType::GETCFHEADERS) { - ProcessGetCFHeaders(pfrom, vRecv, m_chainparams, m_connman); + ProcessGetCFHeaders(pfrom, vRecv); return; } if (msg_type == NetMsgType::GETCFCHECKPT) { - ProcessGetCFCheckPt(pfrom, vRecv, m_chainparams, m_connman); + ProcessGetCFCheckPt(pfrom, vRecv); return; } @@ -4868,7 +4854,7 @@ void PeerManagerImpl::CheckForStaleTipAndEvictPeers() m_stale_tip_check_time = time_in_seconds + STALE_CHECK_INTERVAL; } - if (!m_initial_sync_finished && CanDirectFetch(m_chainparams.GetConsensus())) { + if (!m_initial_sync_finished && CanDirectFetch()) { m_connman.StartExtraBlockRelayPeers(); m_initial_sync_finished = true; } From d34d2c4efbb5d5414a8aa8472325d1f0c6a66345 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Wed, 3 Mar 2021 08:46:55 +0000 Subject: [PATCH 11/19] merge bitcoin#21236: Extract addr send functionality into MaybeSendAddr() --- src/net.h | 5 +- src/net_processing.cpp | 158 ++++++++++++++++++++++------------------- 2 files changed, 87 insertions(+), 76 deletions(-) diff --git a/src/net.h b/src/net.h index a9e53d25070c..f8faf00d7876 100644 --- a/src/net.h +++ b/src/net.h @@ -600,8 +600,9 @@ class CNode std::vector vAddrToSend; std::unique_ptr m_addr_known{nullptr}; bool fGetAddr{false}; - std::chrono::microseconds m_next_addr_send GUARDED_BY(cs_sendProcessing){0}; - std::chrono::microseconds m_next_local_addr_send GUARDED_BY(cs_sendProcessing){0}; + Mutex m_addr_send_times_mutex; + std::chrono::microseconds m_next_addr_send GUARDED_BY(m_addr_send_times_mutex){0}; + std::chrono::microseconds m_next_local_addr_send GUARDED_BY(m_addr_send_times_mutex){0}; bool IsBlockRelayOnly() const; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index b13f1460dbad..b074ff1ea962 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -33,6 +33,7 @@ #include #include +#include #include #include #include @@ -365,8 +366,13 @@ class PeerManagerImpl final : public PeerManager void PushNodeVersion(CNode& pnode, int64_t nTime); /** Send a ping message every PING_INTERVAL or if requested via RPC. May - * mark the peer to be disconnected if a ping has timed out. */ - void MaybeSendPing(CNode& node_to, Peer& peer); + * mark the peer to be disconnected if a ping has timed out. + * We use mockable time for ping timeouts, so setmocktime may cause pings + * to time out. */ + void MaybeSendPing(CNode& node_to, Peer& peer, std::chrono::microseconds now); + + /** Send `addr` messages on a regular schedule. */ + void MaybeSendAddr(CNode& node, std::chrono::microseconds current_time); const CChainParams& m_chainparams; CConnman& m_connman; @@ -4860,12 +4866,8 @@ void PeerManagerImpl::CheckForStaleTipAndEvictPeers() } } -void PeerManagerImpl::MaybeSendPing(CNode& node_to, Peer& peer) +void PeerManagerImpl::MaybeSendPing(CNode& node_to, Peer& peer, std::chrono::microseconds now) { - // Use mockable time for ping timeouts. - // This means that setmocktime may cause pings to time out. - auto now = GetTime(); - if (m_connman.RunInactivityChecks(node_to) && peer.m_ping_nonce_sent && now > peer.m_ping_start.load() + std::chrono::seconds{TIMEOUT_INTERVAL}) { LogPrint(BCLog::NET, "ping timeout: %fs peer=%d\n", 0.000001 * count_microseconds(now - peer.m_ping_start.load()), peer.m_id); @@ -4898,6 +4900,75 @@ void PeerManagerImpl::MaybeSendPing(CNode& node_to, Peer& peer) } } +void PeerManagerImpl::MaybeSendAddr(CNode& node, std::chrono::microseconds current_time) +{ + // Nothing to do for non-address-relay peers + if (!node.RelayAddrsWithConn()) return; + + assert(node.m_addr_known); + + LOCK(node.m_addr_send_times_mutex); + // Periodically advertise our local address to the peer. + if (fListen && !m_chainman.ActiveChainstate().IsInitialBlockDownload() && + node.m_next_local_addr_send < current_time) { + // If we've sent before, clear the bloom filter for the peer, so that our + // self-announcement will actually go out. + // This might be unnecessary if the bloom filter has already rolled + // over since our last self-announcement, but there is only a small + // bandwidth cost that we can incur by doing this (which happens + // once a day on average). + if (node.m_next_local_addr_send != 0us) { + node.m_addr_known->reset(); + } + if (std::optional local_addr = GetLocalAddrForPeer(&node)) { + FastRandomContext insecure_rand; + node.PushAddress(*local_addr, insecure_rand); + } + node.m_next_local_addr_send = PoissonNextSend(current_time, AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL); + } + + // We sent an `addr` message to this peer recently. Nothing more to do. + if (current_time <= node.m_next_addr_send) return; + + node.m_next_addr_send = PoissonNextSend(current_time, AVG_ADDRESS_BROADCAST_INTERVAL); + + if (!Assume(node.vAddrToSend.size() <= MAX_ADDR_TO_SEND)) { + // Should be impossible since we always check size before adding to + // vAddrToSend. Recover by trimming the vector. + node.vAddrToSend.resize(MAX_ADDR_TO_SEND); + } + + // Remove addr records that the peer already knows about, and add new + // addrs to the m_addr_known filter on the same pass. + auto addr_already_known = [&node](const CAddress& addr) { + bool ret = node.m_addr_known->contains(addr.GetKey()); + if (!ret) node.m_addr_known->insert(addr.GetKey()); + return ret; + }; + node.vAddrToSend.erase(std::remove_if(node.vAddrToSend.begin(), node.vAddrToSend.end(), addr_already_known), + node.vAddrToSend.end()); + + // No addr messages to send + if (node.vAddrToSend.empty()) return; + + const char* msg_type; + int make_flags; + if (node.m_wants_addrv2) { + msg_type = NetMsgType::ADDRV2; + make_flags = ADDRV2_FORMAT; + } else { + msg_type = NetMsgType::ADDR; + make_flags = 0; + } + m_connman.PushMessage(&node, CNetMsgMaker(node.GetCommonVersion()).Make(make_flags, msg_type, node.vAddrToSend)); + node.vAddrToSend.clear(); + + // we only send the big addr message once + if (node.vAddrToSend.capacity() > 40) { + node.vAddrToSend.shrink_to_fit(); + } +} + namespace { class CompareInvMempoolOrder { @@ -4936,79 +5007,20 @@ bool PeerManagerImpl::SendMessages(CNode* pto) // If we get here, the outgoing message serialization version is set and can't change. const CNetMsgMaker msgMaker(pto->GetCommonVersion()); - MaybeSendPing(*pto, *peer); + const auto current_time = GetTime(); + + MaybeSendPing(*pto, *peer, current_time); // MaybeSendPing may have marked peer for disconnection if (pto->fDisconnect) return true; + MaybeSendAddr(*pto, current_time); + { LOCK(cs_main); CNodeState &state = *State(pto->GetId()); - // Address refresh broadcast - auto current_time = GetTime(); - - if (fListen && pto->RelayAddrsWithConn() && - !m_chainman.ActiveChainstate().IsInitialBlockDownload() && - pto->m_next_local_addr_send < current_time) { - // If we've sent before, clear the bloom filter for the peer, so that our - // self-announcement will actually go out. - // This might be unnecessary if the bloom filter has already rolled - // over since our last self-announcement, but there is only a small - // bandwidth cost that we can incur by doing this (which happens - // once a day on average). - if (pto->m_next_local_addr_send != std::chrono::microseconds::zero()) { - pto->m_addr_known->reset(); - } - if (std::optional local_addr = GetLocalAddrForPeer(pto)) { - FastRandomContext insecure_rand; - pto->PushAddress(*local_addr, insecure_rand); - } - pto->m_next_local_addr_send = PoissonNextSend(current_time, AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL); - } - - // - // Message: addr - // - if (pto->RelayAddrsWithConn() && pto->m_next_addr_send < current_time) { - pto->m_next_addr_send = PoissonNextSend(current_time, AVG_ADDRESS_BROADCAST_INTERVAL); - std::vector vAddr; - vAddr.reserve(pto->vAddrToSend.size()); - - const char* msg_type; - int make_flags; - if (pto->m_wants_addrv2) { - msg_type = NetMsgType::ADDRV2; - make_flags = ADDRV2_FORMAT; - } else { - msg_type = NetMsgType::ADDR; - make_flags = 0; - } - - assert(pto->m_addr_known); - for (const CAddress& addr : pto->vAddrToSend) - { - if (!pto->m_addr_known->contains(addr.GetKey())) - { - pto->m_addr_known->insert(addr.GetKey()); - vAddr.push_back(addr); - // receiver rejects addr messages larger than MAX_ADDR_TO_SEND - if (vAddr.size() >= MAX_ADDR_TO_SEND) - { - m_connman.PushMessage(pto, msgMaker.Make(make_flags, msg_type, vAddr)); - vAddr.clear(); - } - } - } - pto->vAddrToSend.clear(); - if (!vAddr.empty()) - m_connman.PushMessage(pto, msgMaker.Make(make_flags, msg_type, vAddr)); - // we only send the big addr message once - if (pto->vAddrToSend.capacity() > 40) - pto->vAddrToSend.shrink_to_fit(); - } - // Start block sync if (pindexBestHeader == nullptr) pindexBestHeader = m_chainman.ActiveChain().Tip(); @@ -5287,8 +5299,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) uint256 chainlockHash = ::SerializeHash(clsig); queueAndMaybePushInv(CInv(MSG_CLSIG, chainlockHash)); } - - pto->m_tx_relay->m_last_mempool_req = GetTime(); + pto->m_tx_relay->m_last_mempool_req = std::chrono::duration_cast(current_time); } // Determine transactions to relay @@ -5373,7 +5384,6 @@ bool PeerManagerImpl::SendMessages(CNode* pto) m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::INV, vInv)); // Detect whether we're stalling - current_time = GetTime(); if (state.m_stalling_since.count() && state.m_stalling_since < current_time - BLOCK_STALLING_TIMEOUT) { // Stalling only triggers when the block download window cannot move. During normal steady state, // the download window should be much larger than the to-be-downloaded set of blocks, so disconnection From 39384ba461b73768120fad1961c99004793ac60d Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sun, 24 Mar 2024 20:57:05 +0000 Subject: [PATCH 12/19] merge bitcoin#21198: Address outstanding review comments from PR20721 --- src/net.cpp | 9 ++++++--- src/net.h | 4 ++-- src/net_processing.cpp | 2 +- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index cbe857ca3712..c01020e25fb6 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1488,9 +1488,10 @@ void CConnman::CalculateNumConnectionsChangedStats() statsClient.gauge("peers.torConnections", torNodes, 1.0f); } -bool CConnman::RunInactivityChecks(const CNode& node) const +bool CConnman::ShouldRunInactivityChecks(const CNode& node, std::optional now_in) const { - return GetSystemTimeInSeconds() > node.nTimeConnected + m_peer_connect_timeout; + const int64_t now = now_in ? now_in.value() : GetSystemTimeInSeconds(); + return node.nTimeConnected + m_peer_connect_timeout < now; } bool CConnman::InactivityCheck(const CNode& node) const @@ -1499,6 +1500,8 @@ bool CConnman::InactivityCheck(const CNode& node) const // use setmocktime in the tests). int64_t now = GetSystemTimeInSeconds(); + if (!ShouldRunInactivityChecks(node, now)) return false; + if (node.nLastRecv == 0 || node.nLastSend == 0) { LogPrint(BCLog::NET, "socket no message in first %i seconds, %d %d peer=%d\n", m_peer_connect_timeout, node.nLastRecv != 0, node.nLastSend != 0, node.GetId()); return true; @@ -2038,7 +2041,7 @@ void CConnman::ThreadSocketHandler() SocketHandler(); if (GetTimeMillis() - nLastCleanupNodes > 1000) { ForEachNode(AllNodes, [&](CNode* pnode) { - if (RunInactivityChecks(*pnode) && InactivityCheck(*pnode)) pnode->fDisconnect = true; + if (InactivityCheck(*pnode)) pnode->fDisconnect = true; }); nLastCleanupNodes = GetTimeMillis(); } diff --git a/src/net.h b/src/net.h index f8faf00d7876..cb2eec975f37 100644 --- a/src/net.h +++ b/src/net.h @@ -1295,8 +1295,8 @@ friend class CNode; void SetAsmap(std::vector asmap) { addrman.m_asmap = std::move(asmap); } - /** Return true if the peer has been connected for long enough to do inactivity checks. */ - bool RunInactivityChecks(const CNode& node) const; + /** Return true if we should disconnect the peer for failing an inactivity check. */ + bool ShouldRunInactivityChecks(const CNode& node, std::optional now=std::nullopt) const; private: struct ListenSocket { diff --git a/src/net_processing.cpp b/src/net_processing.cpp index b074ff1ea962..189438c3ddf0 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -4868,7 +4868,7 @@ void PeerManagerImpl::CheckForStaleTipAndEvictPeers() void PeerManagerImpl::MaybeSendPing(CNode& node_to, Peer& peer, std::chrono::microseconds now) { - if (m_connman.RunInactivityChecks(node_to) && peer.m_ping_nonce_sent && + if (m_connman.ShouldRunInactivityChecks(node_to) && peer.m_ping_nonce_sent && now > peer.m_ping_start.load() + std::chrono::seconds{TIMEOUT_INTERVAL}) { LogPrint(BCLog::NET, "ping timeout: %fs peer=%d\n", 0.000001 * count_microseconds(now - peer.m_ping_start.load()), peer.m_id); node_to.fDisconnect = true; From 6d27db58d134e1b007b345f725d5af21d21e3a92 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Tue, 2 Apr 2024 16:15:27 +0000 Subject: [PATCH 13/19] merge bitcoin#21707: Extend functional tests for addr relay --- test/functional/p2p_addr_relay.py | 159 ++++++++++++++++++++++++++---- 1 file changed, 142 insertions(+), 17 deletions(-) diff --git a/test/functional/p2p_addr_relay.py b/test/functional/p2p_addr_relay.py index bc10a7c3c862..53f5b5da1399 100755 --- a/test/functional/p2p_addr_relay.py +++ b/test/functional/p2p_addr_relay.py @@ -10,6 +10,7 @@ CAddress, NODE_NETWORK, msg_addr, + msg_getaddr ) from test_framework.p2p import P2PInterface from test_framework.test_framework import BitcoinTestFramework @@ -17,9 +18,6 @@ assert_equal, ) -# Keep this with length <= 10. Addresses from larger messages are not relayed. -ADDRS = [] -num_ipv4_addrs = 10 class AddrReceiver(P2PInterface): num_ipv4_received = 0 @@ -33,44 +31,85 @@ def on_addr(self, message): self.num_ipv4_received += 1 +class GetAddrStore(P2PInterface): + getaddr_received = False + num_ipv4_received = 0 + + def on_getaddr(self, message): + self.getaddr_received = True + + def on_addr(self, message): + for addr in message.addrs: + self.num_ipv4_received += 1 + + def addr_received(self): + return self.num_ipv4_received != 0 + + class AddrTest(BitcoinTestFramework): + counter = 0 + def set_test_params(self): self.num_nodes = 1 def run_test(self): - for i in range(num_ipv4_addrs): + self.oversized_addr_test() + self.relay_tests() + self.getaddr_tests() + self.blocksonly_mode_tests() + + def setup_addr_msg(self, num): + addrs = [] + for i in range(num): addr = CAddress() - addr.time = int(self.mocktime) + i + addr.time = self.mocktime + i addr.nServices = NODE_NETWORK - addr.ip = "123.123.123.{}".format(i % 256) + addr.ip = f"123.123.123.{self.counter % 256}" addr.port = 8333 + i - ADDRS.append(addr) + addrs.append(addr) + self.counter += 1 - self.log.info('Create connection that sends addr messages') - addr_source = self.nodes[0].add_p2p_connection(P2PInterface()) msg = msg_addr() + msg.addrs = addrs + return msg + + def send_addr_msg(self, source, msg, receivers): + source.send_and_ping(msg) + # pop m_next_addr_send timer + self.bump_mocktime(5 * 60) + for peer in receivers: + peer.sync_with_ping() + + def oversized_addr_test(self): + self.log.info('Send an addr message that is too large') + addr_source = self.nodes[0].add_p2p_connection(P2PInterface()) - self.log.info('Send too-large addr message') - msg.addrs = ADDRS * 101 # more than 1000 addresses in one message + msg = self.setup_addr_msg(1010) with self.nodes[0].assert_debug_log(['addr message size = 1010']): addr_source.send_and_ping(msg) + self.nodes[0].disconnect_p2ps() + + def relay_tests(self): + self.log.info('Test address relay') self.log.info('Check that addr message content is relayed and added to addrman') + addr_source = self.nodes[0].add_p2p_connection(P2PInterface()) num_receivers = 7 receivers = [] for _ in range(num_receivers): receivers.append(self.nodes[0].add_p2p_connection(AddrReceiver())) - msg.addrs = ADDRS + + # Keep this with length <= 10. Addresses from larger messages are not + # relayed. + num_ipv4_addrs = 10 + msg = self.setup_addr_msg(num_ipv4_addrs) with self.nodes[0].assert_debug_log( [ 'Added {} addresses from 127.0.0.1: 0 tried'.format(num_ipv4_addrs), - 'received: addr (301 bytes) peer=0', + 'received: addr (301 bytes) peer=1', ] ): - addr_source.send_and_ping(msg) - self.bump_mocktime(30 * 60) - for receiver in receivers: - receiver.sync_with_ping() + self.send_addr_msg(addr_source, msg, receivers) total_ipv4_received = sum(r.num_ipv4_received for r in receivers) @@ -79,6 +118,92 @@ def run_test(self): ipv4_branching_factor = 2 assert_equal(total_ipv4_received, num_ipv4_addrs * ipv4_branching_factor) + self.nodes[0].disconnect_p2ps() + + self.log.info('Check relay of addresses received from outbound peers') + inbound_peer = self.nodes[0].add_p2p_connection(AddrReceiver()) + full_outbound_peer = self.nodes[0].add_outbound_p2p_connection(GetAddrStore(), p2p_idx=0, connection_type="outbound-full-relay") + msg = self.setup_addr_msg(2) + self.send_addr_msg(full_outbound_peer, msg, [inbound_peer]) + self.log.info('Check that the first addr message received from an outbound peer is not relayed') + # Currently, there is a flag that prevents the first addr message received + # from a new outbound peer to be relayed to others. Originally meant to prevent + # large GETADDR responses from being relayed, it now typically affects the self-announcement + # of the outbound peer which is often sent before the GETADDR response. + assert_equal(inbound_peer.num_ipv4_received, 0) + + self.log.info('Check that subsequent addr messages sent from an outbound peer are relayed') + msg2 = self.setup_addr_msg(2) + self.send_addr_msg(full_outbound_peer, msg2, [inbound_peer]) + assert_equal(inbound_peer.num_ipv4_received, 2) + + self.log.info('Check address relay to outbound peers') + block_relay_peer = self.nodes[0].add_outbound_p2p_connection(GetAddrStore(), p2p_idx=1, connection_type="block-relay-only") + msg3 = self.setup_addr_msg(2) + self.send_addr_msg(inbound_peer, msg3, [full_outbound_peer, block_relay_peer]) + + self.log.info('Check that addresses are relayed to full outbound peers') + assert_equal(full_outbound_peer.num_ipv4_received, 2) + self.log.info('Check that addresses are not relayed to block-relay-only outbound peers') + assert_equal(block_relay_peer.num_ipv4_received, 0) + + self.nodes[0].disconnect_p2ps() + + def getaddr_tests(self): + self.log.info('Test getaddr behavior') + self.log.info('Check that we send a getaddr message upon connecting to an outbound-full-relay peer') + full_outbound_peer = self.nodes[0].add_outbound_p2p_connection(GetAddrStore(), p2p_idx=0, connection_type="outbound-full-relay") + full_outbound_peer.sync_with_ping() + assert full_outbound_peer.getaddr_received + + self.log.info('Check that we do not send a getaddr message upon connecting to a block-relay-only peer') + block_relay_peer = self.nodes[0].add_outbound_p2p_connection(GetAddrStore(), p2p_idx=1, connection_type="block-relay-only") + block_relay_peer.sync_with_ping() + assert_equal(block_relay_peer.getaddr_received, False) + + self.log.info('Check that we answer getaddr messages only from inbound peers') + inbound_peer = self.nodes[0].add_p2p_connection(GetAddrStore()) + inbound_peer.sync_with_ping() + + # Add some addresses to addrman + for i in range(1000): + first_octet = i >> 8 + second_octet = i % 256 + a = f"{first_octet}.{second_octet}.1.1" + self.nodes[0].addpeeraddress(a, 8333) + + full_outbound_peer.send_and_ping(msg_getaddr()) + block_relay_peer.send_and_ping(msg_getaddr()) + inbound_peer.send_and_ping(msg_getaddr()) + + self.bump_mocktime(5 * 60) + inbound_peer.wait_until(inbound_peer.addr_received) + + assert_equal(full_outbound_peer.num_ipv4_received, 0) + assert_equal(block_relay_peer.num_ipv4_received, 0) + assert inbound_peer.num_ipv4_received > 100 + + self.nodes[0].disconnect_p2ps() + + def blocksonly_mode_tests(self): + self.log.info('Test addr relay in -blocksonly mode') + self.restart_node(0, ["-blocksonly"]) + + self.log.info('Check that we send getaddr messages') + full_outbound_peer = self.nodes[0].add_outbound_p2p_connection(GetAddrStore(), p2p_idx=0, connection_type="outbound-full-relay") + full_outbound_peer.sync_with_ping() + assert full_outbound_peer.getaddr_received + + self.log.info('Check that we relay address messages') + addr_source = self.nodes[0].add_p2p_connection(P2PInterface()) + msg = self.setup_addr_msg(2) + addr_source.send_and_ping(msg) + self.bump_mocktime(5 * 60) + full_outbound_peer.sync_with_ping() + assert_equal(full_outbound_peer.num_ipv4_received, 2) + + self.nodes[0].disconnect_p2ps() + if __name__ == '__main__': AddrTest().main() From 03ab144b8fe72381c4ad1f8e04748d2107d6eab7 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Wed, 28 Apr 2021 08:14:22 +0200 Subject: [PATCH 14/19] merge bitcoin#21785: Fix intermittent issue in p2p_addr_relay.py --- test/functional/p2p_addr_relay.py | 6 ++---- test/functional/p2p_blocksonly.py | 6 +----- test/functional/p2p_filter.py | 3 +-- test/functional/test_framework/p2p.py | 10 +++++++++- 4 files changed, 13 insertions(+), 12 deletions(-) diff --git a/test/functional/p2p_addr_relay.py b/test/functional/p2p_addr_relay.py index 53f5b5da1399..f9c727d60776 100755 --- a/test/functional/p2p_addr_relay.py +++ b/test/functional/p2p_addr_relay.py @@ -78,7 +78,7 @@ def send_addr_msg(self, source, msg, receivers): # pop m_next_addr_send timer self.bump_mocktime(5 * 60) for peer in receivers: - peer.sync_with_ping() + peer.sync_send_with_ping() def oversized_addr_test(self): self.log.info('Send an addr message that is too large') @@ -197,9 +197,7 @@ def blocksonly_mode_tests(self): self.log.info('Check that we relay address messages') addr_source = self.nodes[0].add_p2p_connection(P2PInterface()) msg = self.setup_addr_msg(2) - addr_source.send_and_ping(msg) - self.bump_mocktime(5 * 60) - full_outbound_peer.sync_with_ping() + self.send_addr_msg(addr_source, msg, [full_outbound_peer]) assert_equal(full_outbound_peer.num_ipv4_received, 2) self.nodes[0].disconnect_p2ps() diff --git a/test/functional/p2p_blocksonly.py b/test/functional/p2p_blocksonly.py index 5f25edf42497..f40d406fa2ea 100755 --- a/test/functional/p2p_blocksonly.py +++ b/test/functional/p2p_blocksonly.py @@ -90,11 +90,7 @@ def blocks_relay_conn_tests(self): # Bump time forward to ensure nNextInvSend timer pops self.nodes[0].setmocktime(int(time.time()) + 60) - # Calling sync_with_ping twice requires that the node calls - # `ProcessMessage` twice, and thus ensures `SendMessages` must have - # been called at least once - conn.sync_with_ping() - conn.sync_with_ping() + conn.sync_send_with_ping() assert(int(txid, 16) not in conn.get_invs()) def check_p2p_tx_violation(self, index=1): diff --git a/test/functional/p2p_filter.py b/test/functional/p2p_filter.py index 458e5235b6c7..4d4b69391619 100755 --- a/test/functional/p2p_filter.py +++ b/test/functional/p2p_filter.py @@ -168,8 +168,7 @@ def test_filter(self, filter_peer): filter_peer.merkleblock_received = False filter_peer.tx_received = False self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 90) - filter_peer.sync_with_ping() - filter_peer.sync_with_ping() + filter_peer.sync_send_with_ping() assert not filter_peer.merkleblock_received assert not filter_peer.tx_received diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py index 103ea0420c4b..4e94149a4870 100755 --- a/test/functional/test_framework/p2p.py +++ b/test/functional/test_framework/p2p.py @@ -577,8 +577,16 @@ def send_and_ping(self, message, timeout=60): self.send_message(message) self.sync_with_ping(timeout=timeout) - # Sync up with the node + def sync_send_with_ping(self, timeout=60): + """Ensure SendMessages is called on this connection""" + # Calling sync_with_ping twice requires that the node calls + # `ProcessMessage` twice, and thus ensures `SendMessages` must have + # been called at least once + self.sync_with_ping() + self.sync_with_ping() + def sync_with_ping(self, timeout=60): + """Ensure ProcessMessages is called on this connection""" self.send_message(msg_ping(nonce=self.ping_counter)) def test_function(): From 4844e729e25b83e815b560e5739654710dc1d8b8 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Mon, 25 Mar 2024 14:55:43 +0000 Subject: [PATCH 15/19] merge bitcoin#21506: make NetPermissionFlags an enum class --- src/masternode/sync.cpp | 2 +- src/net.cpp | 36 ++++++++++----------- src/net.h | 2 +- src/net_permissions.cpp | 34 ++++++++++---------- src/net_permissions.h | 46 ++++++++++++++++----------- src/net_processing.cpp | 36 ++++++++++----------- src/qt/rpcconsole.cpp | 2 +- src/test/fuzz/net_permissions.cpp | 4 +-- src/test/netbase_tests.cpp | 52 +++++++++++++++---------------- src/test/util/net.h | 20 ++++++------ 10 files changed, 121 insertions(+), 113 deletions(-) diff --git a/src/masternode/sync.cpp b/src/masternode/sync.cpp index abf9b93a15c5..ad83eaf49fe1 100644 --- a/src/masternode/sync.cpp +++ b/src/masternode/sync.cpp @@ -165,7 +165,7 @@ void CMasternodeSync::ProcessTick() if (!pnode->CanRelay() || (fMasternodeMode && pnode->IsInboundConn())) continue; { - if ((pnode->HasPermission(PF_NOBAN) || pnode->IsManualConn()) && !m_netfulfilledman.HasFulfilledRequest(pnode->addr, strAllow)) { + if ((pnode->HasPermission(NetPermissionFlags::NoBan) || pnode->IsManualConn()) && !m_netfulfilledman.HasFulfilledRequest(pnode->addr, strAllow)) { m_netfulfilledman.RemoveAllFulfilledRequests(pnode->addr); m_netfulfilledman.AddFulfilledRequest(pnode->addr, strAllow); LogPrintf("CMasternodeSync::ProcessTick -- skipping mnsync restrictions for peer=%d\n", pnode->GetId()); diff --git a/src/net.cpp b/src/net.cpp index c01020e25fb6..3c922bc62be1 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -582,7 +582,7 @@ bool CNode::IsBlockRelayOnly() const { // Stop processing non-block data early if // 1) We are in blocks only mode and peer has no relay permission // 2) This peer is a block-relay-only peer - return (ignores_incoming_txs && !HasPermission(PF_RELAY)) || !RelayAddrsWithConn(); + return (ignores_incoming_txs && !HasPermission(NetPermissionFlags::Relay)) || !RelayAddrsWithConn(); } std::string CNode::ConnectionTypeAsString() const @@ -1060,7 +1060,7 @@ bool CConnman::AttemptToEvictConnection() LOCK(cs_vNodes); for (const CNode* node : vNodes) { - if (node->HasPermission(PF_NOBAN)) + if (node->HasPermission(NetPermissionFlags::NoBan)) continue; if (!node->IsInboundConn()) continue; @@ -1135,7 +1135,7 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) { const CAddress addr_bind = GetBindAddress(hSocket); - NetPermissionFlags permissionFlags = NetPermissionFlags::PF_NONE; + NetPermissionFlags permissionFlags = NetPermissionFlags::None; hListenSocket.AddSocketPermissionFlags(permissionFlags); CreateNodeFromAcceptedSocket(hSocket, permissionFlags, addr_bind, addr); @@ -1152,12 +1152,12 @@ void CConnman::CreateNodeFromAcceptedSocket(SOCKET hSocket, AddWhitelistPermissionFlags(permissionFlags, addr); bool legacyWhitelisted = false; - if (NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_ISIMPLICIT)) { - NetPermissions::ClearFlag(permissionFlags, PF_ISIMPLICIT); - if (gArgs.GetBoolArg("-whitelistforcerelay", DEFAULT_WHITELISTFORCERELAY)) NetPermissions::AddFlag(permissionFlags, PF_FORCERELAY); - if (gArgs.GetBoolArg("-whitelistrelay", DEFAULT_WHITELISTRELAY)) NetPermissions::AddFlag(permissionFlags, PF_RELAY); - NetPermissions::AddFlag(permissionFlags, PF_MEMPOOL); - NetPermissions::AddFlag(permissionFlags, PF_NOBAN); + if (NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::Implicit)) { + NetPermissions::ClearFlag(permissionFlags, NetPermissionFlags::Implicit); + if (gArgs.GetBoolArg("-whitelistforcerelay", DEFAULT_WHITELISTFORCERELAY)) NetPermissions::AddFlag(permissionFlags, NetPermissionFlags::ForceRelay); + if (gArgs.GetBoolArg("-whitelistrelay", DEFAULT_WHITELISTRELAY)) NetPermissions::AddFlag(permissionFlags, NetPermissionFlags::Relay); + NetPermissions::AddFlag(permissionFlags, NetPermissionFlags::Mempool); + NetPermissions::AddFlag(permissionFlags, NetPermissionFlags::NoBan); legacyWhitelisted = true; } @@ -1200,7 +1200,7 @@ void CConnman::CreateNodeFromAcceptedSocket(SOCKET hSocket, // Don't accept connections from banned peers. bool banned = m_banman && m_banman->IsBanned(addr); - if (!NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_NOBAN) && banned) + if (!NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::NoBan) && banned) { LogPrint(BCLog::NET, "%s (banned)\n", strDropped); CloseSocket(hSocket); @@ -1209,7 +1209,7 @@ void CConnman::CreateNodeFromAcceptedSocket(SOCKET hSocket, // Only accept connections from discouraged peers if our inbound slots aren't (almost) full. bool discouraged = m_banman && m_banman->IsDiscouraged(addr); - if (!NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_NOBAN) && nInbound + 1 >= nMaxInbound && discouraged) + if (!NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::NoBan) && nInbound + 1 >= nMaxInbound && discouraged) { LogPrint(BCLog::NET, "connection from %s dropped (discouraged)\n", addr.ToString()); CloseSocket(hSocket); @@ -1243,7 +1243,7 @@ void CConnman::CreateNodeFromAcceptedSocket(SOCKET hSocket, uint64_t nonce = GetDeterministicRandomizer(RANDOMIZER_ID_LOCALHOSTNONCE).Write(id).Finalize(); ServiceFlags nodeServices = nLocalServices; - if (NetPermissions::HasFlag(permissionFlags, PF_BLOOMFILTER)) { + if (NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::BloomFilter)) { nodeServices = static_cast(nodeServices | NODE_BLOOM); } @@ -2999,7 +2999,7 @@ void CConnman::ThreadI2PAcceptIncoming() continue; } - CreateNodeFromAcceptedSocket(conn.sock->Release(), NetPermissionFlags::PF_NONE, + CreateNodeFromAcceptedSocket(conn.sock->Release(), NetPermissionFlags::None, CAddress{conn.me, NODE_NONE}, CAddress{conn.peer, NODE_NONE}); } } @@ -3190,7 +3190,7 @@ bool CConnman::Bind(const CService &addr, unsigned int flags, NetPermissionFlags return false; } - if (addr.IsRoutable() && fDiscover && !(flags & BF_DONT_ADVERTISE) && !NetPermissions::HasFlag(permissions, NetPermissionFlags::PF_NOBAN)) { + if (addr.IsRoutable() && fDiscover && !(flags & BF_DONT_ADVERTISE) && !NetPermissions::HasFlag(permissions, NetPermissionFlags::NoBan)) { AddLocal(addr, LOCAL_BIND); } @@ -3204,7 +3204,7 @@ bool CConnman::InitBinds( { bool fBound = false; for (const auto& addrBind : binds) { - fBound |= Bind(addrBind, (BF_EXPLICIT | BF_REPORT_ERROR), NetPermissionFlags::PF_NONE); + fBound |= Bind(addrBind, (BF_EXPLICIT | BF_REPORT_ERROR), NetPermissionFlags::None); } for (const auto& addrBind : whiteBinds) { fBound |= Bind(addrBind.m_service, (BF_EXPLICIT | BF_REPORT_ERROR), addrBind.m_flags); @@ -3213,12 +3213,12 @@ bool CConnman::InitBinds( struct in_addr inaddr_any; inaddr_any.s_addr = htonl(INADDR_ANY); struct in6_addr inaddr6_any = IN6ADDR_ANY_INIT; - fBound |= Bind(CService(inaddr6_any, GetListenPort()), BF_NONE, NetPermissionFlags::PF_NONE); - fBound |= Bind(CService(inaddr_any, GetListenPort()), !fBound ? BF_REPORT_ERROR : BF_NONE, NetPermissionFlags::PF_NONE); + fBound |= Bind(CService(inaddr6_any, GetListenPort()), BF_NONE, NetPermissionFlags::None); + fBound |= Bind(CService(inaddr_any, GetListenPort()), !fBound ? BF_REPORT_ERROR : BF_NONE, NetPermissionFlags::None); } for (const auto& addr_bind : onion_binds) { - fBound |= Bind(addr_bind, BF_EXPLICIT | BF_DONT_ADVERTISE, NetPermissionFlags::PF_NONE); + fBound |= Bind(addr_bind, BF_EXPLICIT | BF_DONT_ADVERTISE, NetPermissionFlags::None); } return fBound; diff --git a/src/net.h b/src/net.h index cb2eec975f37..0725560b0efb 100644 --- a/src/net.h +++ b/src/net.h @@ -433,7 +433,7 @@ class CNode std::unique_ptr m_deserializer; std::unique_ptr m_serializer; - NetPermissionFlags m_permissionFlags{ PF_NONE }; + NetPermissionFlags m_permissionFlags{ NetPermissionFlags::None }; std::atomic nServices{NODE_NONE}; SOCKET hSocket GUARDED_BY(cs_hSocket); /** Total size of all vSendMsg entries */ diff --git a/src/net_permissions.cpp b/src/net_permissions.cpp index 0015a627f1b0..0e9bcf725d62 100644 --- a/src/net_permissions.cpp +++ b/src/net_permissions.cpp @@ -23,12 +23,12 @@ namespace { // Parse the following format: "perm1,perm2@xxxxxx" bool TryParsePermissionFlags(const std::string& str, NetPermissionFlags& output, size_t& readen, bilingual_str& error) { - NetPermissionFlags flags = PF_NONE; + NetPermissionFlags flags = NetPermissionFlags::None; const auto atSeparator = str.find('@'); // if '@' is not found (ie, "xxxxx"), the caller should apply implicit permissions if (atSeparator == std::string::npos) { - NetPermissions::AddFlag(flags, PF_ISIMPLICIT); + NetPermissions::AddFlag(flags, NetPermissionFlags::Implicit); readen = 0; } // else (ie, "perm1,perm2@xxxxx"), let's enumerate the permissions by splitting by ',' and calculate the flags @@ -44,14 +44,14 @@ bool TryParsePermissionFlags(const std::string& str, NetPermissionFlags& output, readen += len; // We read "perm1" if (commaSeparator != std::string::npos) readen++; // We read "," - if (permission == "bloomfilter" || permission == "bloom") NetPermissions::AddFlag(flags, PF_BLOOMFILTER); - else if (permission == "noban") NetPermissions::AddFlag(flags, PF_NOBAN); - else if (permission == "forcerelay") NetPermissions::AddFlag(flags, PF_FORCERELAY); - else if (permission == "mempool") NetPermissions::AddFlag(flags, PF_MEMPOOL); - else if (permission == "download") NetPermissions::AddFlag(flags, PF_DOWNLOAD); - else if (permission == "all") NetPermissions::AddFlag(flags, PF_ALL); - else if (permission == "relay") NetPermissions::AddFlag(flags, PF_RELAY); - else if (permission == "addr") NetPermissions::AddFlag(flags, PF_ADDR); + if (permission == "bloomfilter" || permission == "bloom") NetPermissions::AddFlag(flags, NetPermissionFlags::BloomFilter); + else if (permission == "noban") NetPermissions::AddFlag(flags, NetPermissionFlags::NoBan); + else if (permission == "forcerelay") NetPermissions::AddFlag(flags, NetPermissionFlags::ForceRelay); + else if (permission == "mempool") NetPermissions::AddFlag(flags, NetPermissionFlags::Mempool); + else if (permission == "download") NetPermissions::AddFlag(flags, NetPermissionFlags::Download); + else if (permission == "all") NetPermissions::AddFlag(flags, NetPermissionFlags::All); + else if (permission == "relay") NetPermissions::AddFlag(flags, NetPermissionFlags::Relay); + else if (permission == "addr") NetPermissions::AddFlag(flags, NetPermissionFlags::Addr); else if (permission.length() == 0); // Allow empty entries else { error = strprintf(_("Invalid P2P permission: '%s'"), permission); @@ -71,13 +71,13 @@ bool TryParsePermissionFlags(const std::string& str, NetPermissionFlags& output, std::vector NetPermissions::ToStrings(NetPermissionFlags flags) { std::vector strings; - if (NetPermissions::HasFlag(flags, PF_BLOOMFILTER)) strings.push_back("bloomfilter"); - if (NetPermissions::HasFlag(flags, PF_NOBAN)) strings.push_back("noban"); - if (NetPermissions::HasFlag(flags, PF_FORCERELAY)) strings.push_back("forcerelay"); - if (NetPermissions::HasFlag(flags, PF_RELAY)) strings.push_back("relay"); - if (NetPermissions::HasFlag(flags, PF_MEMPOOL)) strings.push_back("mempool"); - if (NetPermissions::HasFlag(flags, PF_DOWNLOAD)) strings.push_back("download"); - if (NetPermissions::HasFlag(flags, PF_ADDR)) strings.push_back("addr"); + if (NetPermissions::HasFlag(flags, NetPermissionFlags::BloomFilter)) strings.push_back("bloomfilter"); + if (NetPermissions::HasFlag(flags, NetPermissionFlags::NoBan)) strings.push_back("noban"); + if (NetPermissions::HasFlag(flags, NetPermissionFlags::ForceRelay)) strings.push_back("forcerelay"); + if (NetPermissions::HasFlag(flags, NetPermissionFlags::Relay)) strings.push_back("relay"); + if (NetPermissions::HasFlag(flags, NetPermissionFlags::Mempool)) strings.push_back("mempool"); + if (NetPermissions::HasFlag(flags, NetPermissionFlags::Download)) strings.push_back("download"); + if (NetPermissions::HasFlag(flags, NetPermissionFlags::Addr)) strings.push_back("addr"); return strings; } diff --git a/src/net_permissions.h b/src/net_permissions.h index b1c11a330850..ed75562738d7 100644 --- a/src/net_permissions.h +++ b/src/net_permissions.h @@ -5,6 +5,7 @@ #include #include +#include #include #ifndef BITCOIN_NET_PERMISSIONS_H @@ -14,51 +15,58 @@ struct bilingual_str; extern const std::vector NET_PERMISSIONS_DOC; -enum NetPermissionFlags { - PF_NONE = 0, +enum class NetPermissionFlags : uint32_t { + None = 0, // Can query bloomfilter even if -peerbloomfilters is false - PF_BLOOMFILTER = (1U << 1), + BloomFilter = (1U << 1), // Relay and accept transactions from this peer, even if -blocksonly is true - PF_RELAY = (1U << 3), + Relay = (1U << 3), // Always relay transactions from this peer, even if already in mempool // Keep parameter interaction: forcerelay implies relay - PF_FORCERELAY = (1U << 2) | PF_RELAY, + ForceRelay = (1U << 2) | Relay, // Allow getheaders during IBD and block-download after maxuploadtarget limit - PF_DOWNLOAD = (1U << 6), + Download = (1U << 6), // Can't be banned/disconnected/discouraged for misbehavior - PF_NOBAN = (1U << 4) | PF_DOWNLOAD, + NoBan = (1U << 4) | Download, // Can query the mempool - PF_MEMPOOL = (1U << 5), + Mempool = (1U << 5), // Can request addrs without hitting a privacy-preserving cache - PF_ADDR = (1U << 7), + Addr = (1U << 7), // True if the user did not specifically set fine grained permissions - PF_ISIMPLICIT = (1U << 31), - PF_ALL = PF_BLOOMFILTER | PF_FORCERELAY | PF_RELAY | PF_NOBAN | PF_MEMPOOL | PF_DOWNLOAD | PF_ADDR, + Implicit = (1U << 31), + All = BloomFilter | ForceRelay | Relay | NoBan | Mempool | Download | Addr, }; +static inline constexpr NetPermissionFlags operator|(NetPermissionFlags a, NetPermissionFlags b) +{ + using t = typename std::underlying_type::type; + return static_cast(static_cast(a) | static_cast(b)); +} class NetPermissions { public: NetPermissionFlags m_flags; static std::vector ToStrings(NetPermissionFlags flags); - static inline bool HasFlag(const NetPermissionFlags& flags, NetPermissionFlags f) + static inline bool HasFlag(NetPermissionFlags flags, NetPermissionFlags f) { - return (flags & f) == f; + using t = typename std::underlying_type::type; + return (static_cast(flags) & static_cast(f)) == static_cast(f); } static inline void AddFlag(NetPermissionFlags& flags, NetPermissionFlags f) { - flags = static_cast(flags | f); + flags = flags | f; } - //! ClearFlag is only called with `f` == NetPermissionFlags::PF_ISIMPLICIT. + //! ClearFlag is only called with `f` == NetPermissionFlags::Implicit. //! If that should change in the future, be aware that ClearFlag should not - //! be called with a subflag of a multiflag, e.g. NetPermissionFlags::PF_RELAY - //! or NetPermissionFlags::PF_DOWNLOAD, as that would leave `flags` in an + //! be called with a subflag of a multiflag, e.g. NetPermissionFlags::Relay + //! or NetPermissionFlags::Download, as that would leave `flags` in an //! invalid state corresponding to none of the existing flags. static inline void ClearFlag(NetPermissionFlags& flags, NetPermissionFlags f) { - assert(f == NetPermissionFlags::PF_ISIMPLICIT); - flags = static_cast(flags & ~f); + assert(f == NetPermissionFlags::Implicit); + using t = typename std::underlying_type::type; + flags = static_cast(static_cast(flags) & ~static_cast(f)); } }; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 189438c3ddf0..2e9b18ff83eb 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -148,11 +148,11 @@ static constexpr auto AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL = 24h; /** Average delay between peer address broadcasts */ static constexpr auto AVG_ADDRESS_BROADCAST_INTERVAL = 30s; /** Average delay between trickled inventory transmissions for inbound peers. - * Blocks and peers with noban permission bypass this. */ + * Blocks and peers with NetPermissionFlags::NoBan permission bypass this. */ static constexpr auto INBOUND_INVENTORY_BROADCAST_INTERVAL = 5s; /** Average delay between trickled inventory transmissions for outbound peers. * Use a smaller delay as there is less privacy concern for them. - * Blocks and peers with noban permission bypass this. + * Blocks and peers with NetPermissionFlags::NoBan permission bypass this. * Masternode outbound peers get half this delay. */ static constexpr auto OUTBOUND_INVENTORY_BROADCAST_INTERVAL = 2s; /** Maximum rate of inventory items to send per second. @@ -223,7 +223,7 @@ struct Peer { Mutex m_misbehavior_mutex; /** Accumulated misbehavior score for this peer */ int m_misbehavior_score GUARDED_BY(m_misbehavior_mutex){0}; - /** Whether this peer should be disconnected and marked as discouraged (unless it has the noban permission). */ + /** Whether this peer should be disconnected and marked as discouraged (unless it has NetPermissionFlags::NoBan permission). */ bool m_should_discourage GUARDED_BY(m_misbehavior_mutex){false}; /** Protects block inventory data members */ @@ -807,7 +807,7 @@ static void UpdatePreferredDownload(const CNode& node, CNodeState* state) EXCLUS nPreferredDownload -= state->fPreferredDownload; // Whether this node should be marked as a preferred download node. - state->fPreferredDownload = (!node.IsInboundConn() || node.HasPermission(PF_NOBAN)) && !node.IsAddrFetchConn() && !node.fClient; + state->fPreferredDownload = (!node.IsInboundConn() || node.HasPermission(NetPermissionFlags::NoBan)) && !node.IsAddrFetchConn() && !node.fClient; nPreferredDownload += state->fPreferredDownload; } @@ -2104,7 +2104,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& if (send && m_connman.OutboundTargetReached(true) && (((pindexBestHeader != nullptr) && (pindexBestHeader->GetBlockTime() - pindex->GetBlockTime() > HISTORICAL_BLOCK_AGE)) || inv.IsMsgFilteredBlk()) && - !pfrom.HasPermission(PF_DOWNLOAD) // nodes with the download permission may exceed target + !pfrom.HasPermission(NetPermissionFlags::Download) // nodes with the download permission may exceed target ) { LogPrint(BCLog::NET, "historical block serving limit reached, disconnect peer=%d\n", pfrom.GetId()); @@ -2113,7 +2113,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& send = false; } // Avoid leaking prune-height by never sending blocks below the NODE_NETWORK_LIMITED threshold - if (send && !pfrom.HasPermission(PF_NOBAN) && ( + if (send && !pfrom.HasPermission(NetPermissionFlags::NoBan) && ( (((pfrom.GetLocalServices() & NODE_NETWORK_LIMITED) == NODE_NETWORK_LIMITED) && ((pfrom.GetLocalServices() & NODE_NETWORK) != NODE_NETWORK) && (m_chainman.ActiveChain().Tip()->nHeight - pindex->nHeight > (int)NODE_NETWORK_LIMITED_MIN_BLOCKS + 2 /* add two blocks buffer extension for possible races */) ) )) { LogPrint(BCLog::NET, "Ignore block request below NODE_NETWORK_LIMITED threshold from peer=%d\n", pfrom.GetId()); @@ -3654,7 +3654,7 @@ void PeerManagerImpl::ProcessMessage( } LOCK(cs_main); - if (m_chainman.ActiveChainstate().IsInitialBlockDownload() && !pfrom.HasPermission(PF_DOWNLOAD)) { + if (m_chainman.ActiveChainstate().IsInitialBlockDownload() && !pfrom.HasPermission(NetPermissionFlags::Download)) { LogPrint(BCLog::NET, "Ignoring %s from peer=%d because node is in initial block download\n", msg_type, pfrom.GetId()); return; } @@ -3762,7 +3762,7 @@ void PeerManagerImpl::ProcessMessage( LOCK2(cs_main, g_cs_orphans); if (AlreadyHave(inv)) { - if (pfrom.HasPermission(PF_FORCERELAY)) { + if (pfrom.HasPermission(NetPermissionFlags::ForceRelay)) { // Always relay transactions received from peers with forcerelay permission, even // if they were already in the mempool, // allowing the node to function as a gateway for @@ -4259,7 +4259,7 @@ void PeerManagerImpl::ProcessMessage( pfrom.vAddrToSend.clear(); std::vector vAddr; - if (pfrom.HasPermission(PF_ADDR)) { + if (pfrom.HasPermission(NetPermissionFlags::Addr)) { vAddr = m_connman.GetAddresses(MAX_ADDR_TO_SEND, MAX_PCT_ADDR_TO_SEND, /* network */ std::nullopt); } else { vAddr = m_connman.GetAddresses(pfrom, MAX_ADDR_TO_SEND, MAX_PCT_ADDR_TO_SEND); @@ -4272,9 +4272,9 @@ void PeerManagerImpl::ProcessMessage( } if (msg_type == NetMsgType::MEMPOOL) { - if (!(pfrom.GetLocalServices() & NODE_BLOOM) && !pfrom.HasPermission(PF_MEMPOOL)) + if (!(pfrom.GetLocalServices() & NODE_BLOOM) && !pfrom.HasPermission(NetPermissionFlags::Mempool)) { - if (!pfrom.HasPermission(PF_NOBAN)) + if (!pfrom.HasPermission(NetPermissionFlags::NoBan)) { LogPrint(BCLog::NET, "mempool request with bloom filters disabled, disconnect peer=%d\n", pfrom.GetId()); pfrom.fDisconnect = true; @@ -4282,9 +4282,9 @@ void PeerManagerImpl::ProcessMessage( return; } - if (m_connman.OutboundTargetReached(false) && !pfrom.HasPermission(PF_MEMPOOL)) + if (m_connman.OutboundTargetReached(false) && !pfrom.HasPermission(NetPermissionFlags::Mempool)) { - if (!pfrom.HasPermission(PF_NOBAN)) + if (!pfrom.HasPermission(NetPermissionFlags::NoBan)) { LogPrint(BCLog::NET, "mempool request with bandwidth limit reached, disconnect peer=%d\n", pfrom.GetId()); pfrom.fDisconnect = true; @@ -4575,8 +4575,8 @@ bool PeerManagerImpl::MaybeDiscourageAndDisconnect(CNode& pnode, Peer& peer) peer.m_should_discourage = false; } // peer.m_misbehavior_mutex - if (pnode.HasPermission(PF_NOBAN)) { - // We never disconnect or discourage peers for bad behavior if they have the NOBAN permission flag + if (pnode.HasPermission(NetPermissionFlags::NoBan)) { + // We never disconnect or discourage peers for bad behavior if they have NetPermissionFlags::NoBan permission LogPrintf("Warning: not punishing noban peer %d!\n", peer.m_id); return false; } @@ -5252,7 +5252,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) // Check whether periodic sends should happen // Note: If this node is running in a Masternode mode, it makes no sense to delay outgoing txes // because we never produce any txes ourselves i.e. no privacy is lost in this case. - bool fSendTrickle = pto->HasPermission(PF_NOBAN) || fMasternodeMode; + bool fSendTrickle = pto->HasPermission(NetPermissionFlags::NoBan) || fMasternodeMode; if (pto->m_tx_relay->nNextInvSend < current_time) { fSendTrickle = true; if (pto->IsInboundConn()) { @@ -5411,12 +5411,12 @@ bool PeerManagerImpl::SendMessages(CNode* pto) // Detect whether this is a stalling initial-headers-sync peer if (pindexBestHeader->GetBlockTime() <= GetAdjustedTime() - nMaxTipAge) { if (current_time > state.m_headers_sync_timeout && nSyncStarted == 1 && (nPreferredDownload - state.fPreferredDownload >= 1)) { - // Disconnect a peer (without the noban permission) if it is our only sync peer, + // Disconnect a peer (without NetPermissionFlags::NoBan permission) if it is our only sync peer, // and we have others we could be using instead. // Note: If all our peers are inbound, then we won't // disconnect our sync peer for stalling; we have bigger // problems if we can't get any outbound peers. - if (!pto->HasPermission(PF_NOBAN)) { + if (!pto->HasPermission(NetPermissionFlags::NoBan)) { LogPrintf("Timeout downloading headers from peer=%d, disconnecting\n", pto->GetId()); pto->fDisconnect = true; return true; diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp index a51f2f8c7019..30f393ba60d1 100644 --- a/src/qt/rpcconsole.cpp +++ b/src/qt/rpcconsole.cpp @@ -1263,7 +1263,7 @@ void RPCConsole::updateNodeDetail(const CNodeCombinedStats *stats) ? tr("Outbound") : tr("Outbound block-relay")); ui->peerNetwork->setText(GUIUtil::NetworkToQString(stats->nodeStats.m_network)); - if (stats->nodeStats.m_permissionFlags == PF_NONE) { + if (stats->nodeStats.m_permissionFlags == NetPermissionFlags::None) { ui->peerPermissions->setText(tr("N/A")); } else { QStringList permissions; diff --git a/src/test/fuzz/net_permissions.cpp b/src/test/fuzz/net_permissions.cpp index 6fdf4b653cc3..6ea79464d019 100644 --- a/src/test/fuzz/net_permissions.cpp +++ b/src/test/fuzz/net_permissions.cpp @@ -25,7 +25,7 @@ FUZZ_TARGET(net_permissions) (void)NetPermissions::ToStrings(net_whitebind_permissions.m_flags); (void)NetPermissions::AddFlag(net_whitebind_permissions.m_flags, net_permission_flags); assert(NetPermissions::HasFlag(net_whitebind_permissions.m_flags, net_permission_flags)); - (void)NetPermissions::ClearFlag(net_whitebind_permissions.m_flags, NetPermissionFlags::PF_ISIMPLICIT); + (void)NetPermissions::ClearFlag(net_whitebind_permissions.m_flags, NetPermissionFlags::Implicit); (void)NetPermissions::ToStrings(net_whitebind_permissions.m_flags); } @@ -35,7 +35,7 @@ FUZZ_TARGET(net_permissions) (void)NetPermissions::ToStrings(net_whitelist_permissions.m_flags); (void)NetPermissions::AddFlag(net_whitelist_permissions.m_flags, net_permission_flags); assert(NetPermissions::HasFlag(net_whitelist_permissions.m_flags, net_permission_flags)); - (void)NetPermissions::ClearFlag(net_whitelist_permissions.m_flags, NetPermissionFlags::PF_ISIMPLICIT); + (void)NetPermissions::ClearFlag(net_whitelist_permissions.m_flags, NetPermissionFlags::Implicit); (void)NetPermissions::ToStrings(net_whitelist_permissions.m_flags); } } diff --git a/src/test/netbase_tests.cpp b/src/test/netbase_tests.cpp index c304acaf48c6..cae1e3c06730 100644 --- a/src/test/netbase_tests.cpp +++ b/src/test/netbase_tests.cpp @@ -469,27 +469,27 @@ BOOST_AUTO_TEST_CASE(netpermissions_test) // If no permission flags, assume backward compatibility BOOST_CHECK(NetWhitebindPermissions::TryParse("1.2.3.4:32", whitebindPermissions, error)); BOOST_CHECK(error.empty()); - BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, PF_ISIMPLICIT); - BOOST_CHECK(NetPermissions::HasFlag(whitebindPermissions.m_flags, PF_ISIMPLICIT)); - NetPermissions::ClearFlag(whitebindPermissions.m_flags, PF_ISIMPLICIT); - BOOST_CHECK(!NetPermissions::HasFlag(whitebindPermissions.m_flags, PF_ISIMPLICIT)); - BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, PF_NONE); - NetPermissions::AddFlag(whitebindPermissions.m_flags, PF_ISIMPLICIT); - BOOST_CHECK(NetPermissions::HasFlag(whitebindPermissions.m_flags, PF_ISIMPLICIT)); + BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, NetPermissionFlags::Implicit); + BOOST_CHECK(NetPermissions::HasFlag(whitebindPermissions.m_flags, NetPermissionFlags::Implicit)); + NetPermissions::ClearFlag(whitebindPermissions.m_flags, NetPermissionFlags::Implicit); + BOOST_CHECK(!NetPermissions::HasFlag(whitebindPermissions.m_flags, NetPermissionFlags::Implicit)); + BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, NetPermissionFlags::None); + NetPermissions::AddFlag(whitebindPermissions.m_flags, NetPermissionFlags::Implicit); + BOOST_CHECK(NetPermissions::HasFlag(whitebindPermissions.m_flags, NetPermissionFlags::Implicit)); // Can set one permission BOOST_CHECK(NetWhitebindPermissions::TryParse("bloom@1.2.3.4:32", whitebindPermissions, error)); - BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, PF_BLOOMFILTER); + BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, NetPermissionFlags::BloomFilter); BOOST_CHECK(NetWhitebindPermissions::TryParse("@1.2.3.4:32", whitebindPermissions, error)); - BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, PF_NONE); + BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, NetPermissionFlags::None); NetWhitebindPermissions noban, noban_download, download_noban, download; // "noban" implies "download" BOOST_REQUIRE(NetWhitebindPermissions::TryParse("noban@1.2.3.4:32", noban, error)); - BOOST_CHECK_EQUAL(noban.m_flags, NetPermissionFlags::PF_NOBAN); - BOOST_CHECK(NetPermissions::HasFlag(noban.m_flags, NetPermissionFlags::PF_DOWNLOAD)); - BOOST_CHECK(NetPermissions::HasFlag(noban.m_flags, NetPermissionFlags::PF_NOBAN)); + BOOST_CHECK_EQUAL(noban.m_flags, NetPermissionFlags::NoBan); + BOOST_CHECK(NetPermissions::HasFlag(noban.m_flags, NetPermissionFlags::Download)); + BOOST_CHECK(NetPermissions::HasFlag(noban.m_flags, NetPermissionFlags::NoBan)); // "noban,download" is equivalent to "noban" BOOST_REQUIRE(NetWhitebindPermissions::TryParse("noban,download@1.2.3.4:32", noban_download, error)); @@ -501,31 +501,31 @@ BOOST_AUTO_TEST_CASE(netpermissions_test) // "download" excludes (does not imply) "noban" BOOST_REQUIRE(NetWhitebindPermissions::TryParse("download@1.2.3.4:32", download, error)); - BOOST_CHECK_EQUAL(download.m_flags, NetPermissionFlags::PF_DOWNLOAD); - BOOST_CHECK(NetPermissions::HasFlag(download.m_flags, NetPermissionFlags::PF_DOWNLOAD)); - BOOST_CHECK(!NetPermissions::HasFlag(download.m_flags, NetPermissionFlags::PF_NOBAN)); + BOOST_CHECK_EQUAL(download.m_flags, NetPermissionFlags::Download); + BOOST_CHECK(NetPermissions::HasFlag(download.m_flags, NetPermissionFlags::Download)); + BOOST_CHECK(!NetPermissions::HasFlag(download.m_flags, NetPermissionFlags::NoBan)); // Happy path, can parse flags BOOST_CHECK(NetWhitebindPermissions::TryParse("bloom,forcerelay@1.2.3.4:32", whitebindPermissions, error)); // forcerelay should also activate the relay permission - BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, PF_BLOOMFILTER | PF_FORCERELAY | PF_RELAY); + BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, NetPermissionFlags::BloomFilter | NetPermissionFlags::ForceRelay | NetPermissionFlags::Relay); BOOST_CHECK(NetWhitebindPermissions::TryParse("bloom,relay,noban@1.2.3.4:32", whitebindPermissions, error)); - BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, PF_BLOOMFILTER | PF_RELAY | PF_NOBAN); + BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, NetPermissionFlags::BloomFilter | NetPermissionFlags::Relay | NetPermissionFlags::NoBan); BOOST_CHECK(NetWhitebindPermissions::TryParse("bloom,forcerelay,noban@1.2.3.4:32", whitebindPermissions, error)); BOOST_CHECK(NetWhitebindPermissions::TryParse("all@1.2.3.4:32", whitebindPermissions, error)); - BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, PF_ALL); + BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, NetPermissionFlags::All); // Allow dups BOOST_CHECK(NetWhitebindPermissions::TryParse("bloom,relay,noban,noban@1.2.3.4:32", whitebindPermissions, error)); - BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, PF_BLOOMFILTER | PF_RELAY | PF_NOBAN | PF_DOWNLOAD); // "noban" implies "download" + BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, NetPermissionFlags::BloomFilter | NetPermissionFlags::Relay | NetPermissionFlags::NoBan | NetPermissionFlags::Download); // "noban" implies "download" // Allow empty BOOST_CHECK(NetWhitebindPermissions::TryParse("bloom,relay,,noban@1.2.3.4:32", whitebindPermissions, error)); - BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, PF_BLOOMFILTER | PF_RELAY | PF_NOBAN); + BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, NetPermissionFlags::BloomFilter | NetPermissionFlags::Relay | NetPermissionFlags::NoBan); BOOST_CHECK(NetWhitebindPermissions::TryParse(",@1.2.3.4:32", whitebindPermissions, error)); - BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, PF_NONE); + BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, NetPermissionFlags::None); BOOST_CHECK(NetWhitebindPermissions::TryParse(",,@1.2.3.4:32", whitebindPermissions, error)); - BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, PF_NONE); + BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, NetPermissionFlags::None); // Detect invalid flag BOOST_CHECK(!NetWhitebindPermissions::TryParse("bloom,forcerelay,oopsie@1.2.3.4:32", whitebindPermissions, error)); @@ -537,16 +537,16 @@ BOOST_AUTO_TEST_CASE(netpermissions_test) // Happy path for whitelist parsing BOOST_CHECK(NetWhitelistPermissions::TryParse("noban@1.2.3.4", whitelistPermissions, error)); - BOOST_CHECK_EQUAL(whitelistPermissions.m_flags, PF_NOBAN); - BOOST_CHECK(NetPermissions::HasFlag(whitelistPermissions.m_flags, NetPermissionFlags::PF_NOBAN)); + BOOST_CHECK_EQUAL(whitelistPermissions.m_flags, NetPermissionFlags::NoBan); + BOOST_CHECK(NetPermissions::HasFlag(whitelistPermissions.m_flags, NetPermissionFlags::NoBan)); BOOST_CHECK(NetWhitelistPermissions::TryParse("bloom,forcerelay,noban,relay@1.2.3.4/32", whitelistPermissions, error)); - BOOST_CHECK_EQUAL(whitelistPermissions.m_flags, PF_BLOOMFILTER | PF_FORCERELAY | PF_NOBAN | PF_RELAY); + BOOST_CHECK_EQUAL(whitelistPermissions.m_flags, NetPermissionFlags::BloomFilter | NetPermissionFlags::ForceRelay | NetPermissionFlags::NoBan | NetPermissionFlags::Relay); BOOST_CHECK(error.empty()); BOOST_CHECK_EQUAL(whitelistPermissions.m_subnet.ToString(), "1.2.3.4/32"); BOOST_CHECK(NetWhitelistPermissions::TryParse("bloom,forcerelay,noban,relay,mempool@1.2.3.4/32", whitelistPermissions, error)); - const auto strings = NetPermissions::ToStrings(PF_ALL); + const auto strings = NetPermissions::ToStrings(NetPermissionFlags::All); BOOST_CHECK_EQUAL(strings.size(), 7U); BOOST_CHECK(std::find(strings.begin(), strings.end(), "bloomfilter") != strings.end()); BOOST_CHECK(std::find(strings.begin(), strings.end(), "forcerelay") != strings.end()); diff --git a/src/test/util/net.h b/src/test/util/net.h index da0eec8be275..c6cebed8def1 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -46,16 +46,16 @@ constexpr ServiceFlags ALL_SERVICE_FLAGS[]{ }; constexpr NetPermissionFlags ALL_NET_PERMISSION_FLAGS[]{ - NetPermissionFlags::PF_NONE, - NetPermissionFlags::PF_BLOOMFILTER, - NetPermissionFlags::PF_RELAY, - NetPermissionFlags::PF_FORCERELAY, - NetPermissionFlags::PF_NOBAN, - NetPermissionFlags::PF_MEMPOOL, - NetPermissionFlags::PF_ADDR, - NetPermissionFlags::PF_DOWNLOAD, - NetPermissionFlags::PF_ISIMPLICIT, - NetPermissionFlags::PF_ALL, + NetPermissionFlags::None, + NetPermissionFlags::BloomFilter, + NetPermissionFlags::Relay, + NetPermissionFlags::ForceRelay, + NetPermissionFlags::NoBan, + NetPermissionFlags::Mempool, + NetPermissionFlags::Addr, + NetPermissionFlags::Download, + NetPermissionFlags::Implicit, + NetPermissionFlags::All, }; /** From 26c39f5b929edda6a0dea8d6e6bd58b3d4a69e7c Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 28 Mar 2024 19:07:00 +0000 Subject: [PATCH 16/19] net: replace RelayAddrsWithConn check with !IsBlockOnlyConn Dash uses a lot more CNode::RelayAddrsWithConn checks than Bitcoin (esp. since a483122f (#4888)), so bitcoin#21186 will not adequately cover the removal of RelayAddrsWithConn usages. When possible to query with RelayAddrsWithPeer, that should be used, as that value is the most reliable, else we rely on the former mutual exclusivity of IsBlockOnlyConn and RelayAddrsWithConn to fill in the blanks where a more reliable query isn't available. Note: To prevent builds from breaking, a change has been made in InstantSend code despite it breaking functionality. A commit later will repair it by creating a way to access RelayAddrsWithPeer. --- src/llmq/instantsend.cpp | 2 +- src/net.cpp | 10 +++++----- src/net.h | 2 +- src/net_processing.cpp | 4 ++-- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/llmq/instantsend.cpp b/src/llmq/instantsend.cpp index a48e254181ef..927a8088ca08 100644 --- a/src/llmq/instantsend.cpp +++ b/src/llmq/instantsend.cpp @@ -1458,7 +1458,7 @@ void CInstantSendManager::AskNodesForLockedTx(const uint256& txid, const CConnma if (nodesToAskFor.size() >= 4) { return; } - if (pnode->RelayAddrsWithConn()) { + if (!pnode->IsBlockOnlyConn()) { LOCK(pnode->m_tx_relay->cs_tx_inventory); if (pnode->m_tx_relay->filterInventoryKnown.contains(txid)) { pnode->AddRef(); diff --git a/src/net.cpp b/src/net.cpp index 3c922bc62be1..1e1c2bf3bf5a 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -582,7 +582,7 @@ bool CNode::IsBlockRelayOnly() const { // Stop processing non-block data early if // 1) We are in blocks only mode and peer has no relay permission // 2) This peer is a block-relay-only peer - return (ignores_incoming_txs && !HasPermission(NetPermissionFlags::Relay)) || !RelayAddrsWithConn(); + return (ignores_incoming_txs && !HasPermission(NetPermissionFlags::Relay)) || IsBlockOnlyConn(); } std::string CNode::ConnectionTypeAsString() const @@ -651,7 +651,7 @@ void CNode::copyStats(CNodeStats &stats, const std::vector &m_asmap) X(addrBind); stats.m_network = ConnectedThroughNetwork(); stats.m_mapped_as = addr.GetMappedAS(m_asmap); - if (RelayAddrsWithConn()) { + if (!IsBlockOnlyConn()) { LOCK(m_tx_relay->cs_filter); stats.fRelayTxes = m_tx_relay->fRelayTxes; } else { @@ -1088,7 +1088,7 @@ bool CConnman::AttemptToEvictConnection() bool peer_relay_txes = false; bool peer_filter_not_null = false; - if (node->RelayAddrsWithConn()) { + if (!node->IsBlockOnlyConn()) { LOCK(node->m_tx_relay->cs_filter); peer_relay_txes = node->m_tx_relay->fRelayTxes; peer_filter_not_null = node->m_tx_relay->pfilter != nullptr; @@ -3897,7 +3897,7 @@ void CConnman::RelayInvFiltered(CInv &inv, const CTransaction& relatedTx, const { LOCK(cs_vNodes); for (const auto& pnode : vNodes) { - if (pnode->nVersion < minProtoVersion || !pnode->CanRelay() || !pnode->RelayAddrsWithConn()) { + if (pnode->nVersion < minProtoVersion || !pnode->CanRelay() || pnode->IsBlockOnlyConn()) { continue; } { @@ -3917,7 +3917,7 @@ void CConnman::RelayInvFiltered(CInv &inv, const uint256& relatedTxHash, const i { LOCK(cs_vNodes); for (const auto& pnode : vNodes) { - if (pnode->nVersion < minProtoVersion || !pnode->CanRelay() || !pnode->RelayAddrsWithConn()) { + if (pnode->nVersion < minProtoVersion || !pnode->CanRelay() || pnode->IsBlockOnlyConn()) { continue; } { diff --git a/src/net.h b/src/net.h index 0725560b0efb..0be08ba7aab9 100644 --- a/src/net.h +++ b/src/net.h @@ -631,7 +631,7 @@ class CNode }; // in bitcoin: m_tx_relay == nullptr if we're not relaying transactions with this peer - // in dash: m_tx_relay should never be nullptr, use `RelayAddrsWithConn() == false` instead + // in dash: m_tx_relay should never be nullptr, use `!IsBlockOnlyConn() == false` instead std::unique_ptr m_tx_relay{std::make_unique()}; /** UNIX epoch time of the last block received from this peer that we had diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 2e9b18ff83eb..9bb5b0ef6b76 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1075,7 +1075,7 @@ void PeerManagerImpl::PushNodeVersion(CNode& pnode, int64_t nTime) nProtocolVersion = gArgs.GetArg("-pushversion", PROTOCOL_VERSION); } - const bool tx_relay = !m_ignore_incoming_txs && pnode.RelayAddrsWithConn(); + 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, nonce, strSubVersion, nNodeStartingHeight, tx_relay, mnauthChallenge, pnode.m_masternode_connection.load())); @@ -1315,7 +1315,7 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) { } } // cs_main - if (node.fSuccessfullyConnected && misbehavior == 0 && node.RelayAddrsWithConn() && !node.IsInboundConn()) { + if (node.fSuccessfullyConnected && misbehavior == 0 && !node.IsBlockOnlyConn() && !node.IsInboundConn()) { // Only change visible addrman state for full outbound peers. We don't // call Connected() for feeler connections since they don't have // fSuccessfullyConnected set. From 5478001a810bb7cf8232a90226e3235ace670dd0 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sun, 24 Mar 2024 20:35:04 +0000 Subject: [PATCH 17/19] partial bitcoin#21186: Move addr data into net_processing excludes: - 0829516d1f3868c1c2ba507feee718325d81e329 --- src/net.cpp | 4 - src/net.h | 64 +----------- src/net_processing.cpp | 231 +++++++++++++++++++++++++---------------- src/test/fuzz/net.cpp | 22 ---- 4 files changed, 146 insertions(+), 175 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 1e1c2bf3bf5a..b114a4b1ae99 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -4047,10 +4047,6 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, SOCKET hSocketIn, const hSocket = hSocketIn; addrName = addrNameIn == "" ? addr.ToStringIPPort() : addrNameIn; - if (conn_type_in != ConnectionType::BLOCK_RELAY) { - m_addr_known = std::make_unique(5000, 0.001); - } - for (const std::string &msg : getAllNetMessageTypes()) mapRecvBytesPerMsgCmd[msg] = 0; mapRecvBytesPerMsgCmd[NET_MESSAGE_COMMAND_OTHER] = 0; diff --git a/src/net.h b/src/net.h index 0be08ba7aab9..8ce40a902df6 100644 --- a/src/net.h +++ b/src/net.h @@ -70,8 +70,6 @@ static constexpr auto FEELER_INTERVAL = 2min; static const unsigned int MAX_INV_SZ = 50000; /** Run the extra block-relay-only connection loop once every 5 minutes. **/ static constexpr auto EXTRA_BLOCK_RELAY_ONLY_PEER_INTERVAL = 5min; -/** The maximum number of addresses from our addrman to return in response to a getaddr message. */ -static constexpr size_t MAX_ADDR_TO_SEND = 1000; /** Maximum length of incoming protocol messages (no message over 3 MiB is currently acceptable). */ static const unsigned int MAX_PROTOCOL_MESSAGE_LENGTH = 3 * 1024 * 1024; /** Maximum length of the user agent string in `version` message */ @@ -482,12 +480,6 @@ class CNode bool m_legacyWhitelisted{false}; bool fClient{false}; // set by version message bool m_limited_node{false}; //after BIP159, set by version message - - /** - * Whether the peer has signaled support for receiving ADDRv2 (BIP155) - * messages, implying a preference to receive ADDRv2 instead of ADDR ones. - */ - std::atomic_bool m_wants_addrv2{false}; /** fSuccessfullyConnected is set to true on receiving VERACK from the peer. */ std::atomic_bool fSuccessfullyConnected{false}; // Setting fDisconnect to true will cause the node to be disconnected the @@ -496,7 +488,6 @@ class CNode std::atomic nDisconnectLingerTime{0}; std::atomic_bool fSocketShutdown{false}; std::atomic_bool fOtherSideDisconnected { false }; - bool fSentAddr{false}; // If 'true' this node will be disconnected on CMasternodeMan::ProcessMasternodeConnections() std::atomic m_masternode_connection{false}; /** @@ -567,15 +558,6 @@ class CNode return m_conn_type == ConnectionType::INBOUND; } - /* Whether we send addr messages over this connection */ - bool RelayAddrsWithConn() const - { - // Don't relay addr messages to peers that we connect to as block-relay-only - // peers (to prevent adversaries from inferring these links from addr - // traffic). - return m_conn_type != ConnectionType::BLOCK_RELAY; - } - bool ExpectServicesFromConn() const { switch (m_conn_type) { case ConnectionType::INBOUND: @@ -596,16 +578,6 @@ class CNode mapMsgCmdSize mapRecvBytesPerMsgCmd GUARDED_BY(cs_vRecv); public: - // flood relay - std::vector vAddrToSend; - std::unique_ptr m_addr_known{nullptr}; - bool fGetAddr{false}; - Mutex m_addr_send_times_mutex; - std::chrono::microseconds m_next_addr_send GUARDED_BY(m_addr_send_times_mutex){0}; - std::chrono::microseconds m_next_local_addr_send GUARDED_BY(m_addr_send_times_mutex){0}; - - bool IsBlockRelayOnly() const; - struct TxRelay { mutable RecursiveMutex cs_filter; // We use fRelayTxes for two purposes - @@ -662,6 +634,8 @@ class CNode // If true, we will send him all quorum related messages, even if he is not a member of our quorums std::atomic qwatch{false}; + bool IsBlockRelayOnly() const; + CNode(NodeId id, ServiceFlags nLocalServicesIn, SOCKET hSocketIn, const CAddress &addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress &addrBindIn, const std::string &addrNameIn, ConnectionType conn_type_in, bool inbound_onion = false); ~CNode(); CNode(const CNode&) = delete; @@ -761,40 +735,6 @@ class CNode nRefCount--; } - - - void AddAddressKnown(const CAddress& _addr) - { - assert(m_addr_known); - m_addr_known->insert(_addr.GetKey()); - } - - /** - * Whether the peer supports the address. For example, a peer that does not - * implement BIP155 cannot receive Tor v3 addresses because it requires - * ADDRv2 (BIP155) encoding. - */ - bool IsAddrCompatible(const CAddress& addr) const - { - return m_wants_addrv2 || addr.IsAddrV1Compatible(); - } - - void PushAddress(const CAddress& _addr, FastRandomContext &insecure_rand) - { - // Known checking here is only to save space from duplicates. - // SendMessages will filter it again for knowns that were added - // after addresses were pushed. - assert(m_addr_known); - if (_addr.IsValid() && !m_addr_known->contains(_addr.GetKey()) && IsAddrCompatible(_addr)) { - if (vAddrToSend.size() >= MAX_ADDR_TO_SEND) { - vAddrToSend[insecure_rand.randrange(vAddrToSend.size())] = _addr; - } else { - vAddrToSend.push_back(_addr); - } - } - } - - void AddKnownInventory(const uint256& hash) { LOCK(m_tx_relay->cs_tx_inventory); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 9bb5b0ef6b76..34330318eefa 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -174,6 +174,8 @@ static constexpr uint32_t MAX_GETCFILTERS_SIZE = 1000; static constexpr uint32_t MAX_GETCFHEADERS_SIZE = 2000; /** the maximum percentage of addresses from our addrman to return in response to a getaddr message. */ static constexpr size_t MAX_PCT_ADDR_TO_SEND = 23; +/** The maximum number of address records permitted in an ADDR message. */ +static constexpr size_t MAX_ADDR_TO_SEND{1000}; struct COrphanTx { // When modifying, adapt the copy of this definition in tests/DoS_tests. @@ -253,6 +255,25 @@ struct Peer { /** Whether a ping has been requested by the user */ std::atomic m_ping_queued{false}; + /** A vector of addresses to send to the peer, limited to MAX_ADDR_TO_SEND. */ + std::vector m_addrs_to_send; + /** Probabilistic filter of addresses that this peer already knows. + * Used to avoid relaying addresses to this peer more than once. */ + const std::unique_ptr m_addr_known; + /** Whether a getaddr request to this peer is outstanding. */ + bool m_getaddr_sent{false}; + /** Guards address sending timers. */ + mutable Mutex m_addr_send_times_mutex; + /** Time point to send the next ADDR message to this peer. */ + std::chrono::microseconds m_next_addr_send GUARDED_BY(m_addr_send_times_mutex){0}; + /** Time point to possibly re-announce our local address to this peer. */ + std::chrono::microseconds m_next_local_addr_send GUARDED_BY(m_addr_send_times_mutex){0}; + /** Whether the peer has signaled support for receiving ADDRv2 (BIP155) + * messages, indicating a preference to receive ADDRv2 instead of ADDR ones. */ + std::atomic_bool m_wants_addrv2{false}; + /** Whether this peer has already sent us a getaddr message. */ + bool m_getaddr_recvd{false}; + /** Set of txids to reconsider once their parent transactions have been accepted **/ std::set m_orphan_work_set GUARDED_BY(g_cs_orphans); @@ -261,7 +282,10 @@ struct Peer { /** Work queue of items requested by this peer **/ std::deque m_getdata_requests GUARDED_BY(m_getdata_requests_mutex); - explicit Peer(NodeId id) : m_id(id) {} + explicit Peer(NodeId id, bool addr_relay) + : m_id(id) + , m_addr_known{addr_relay ? std::make_unique(5000, 0.001) : nullptr} + {} }; using PeerRef = std::shared_ptr; @@ -372,7 +396,16 @@ class PeerManagerImpl final : public PeerManager void MaybeSendPing(CNode& node_to, Peer& peer, std::chrono::microseconds now); /** Send `addr` messages on a regular schedule. */ - void MaybeSendAddr(CNode& node, std::chrono::microseconds current_time); + void MaybeSendAddr(CNode& node, Peer& peer, std::chrono::microseconds current_time); + + /** Relay (gossip) an address to a few randomly chosen nodes. + * + * @param[in] originator The id of the peer that sent us the address. We don't want to relay it back. + * @param[in] addr Address to relay. + * @param[in] fReachable Whether the address' network is reachable. We relay unreachable + * addresses less. + */ + void RelayAddress(NodeId originator, const CAddress& addr, bool fReachable); const CChainParams& m_chainparams; CConnman& m_connman; @@ -802,6 +835,42 @@ static CNodeState *State(NodeId pnode) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return &it->second; } +static bool RelayAddrsWithPeer(const Peer& peer) +{ + return peer.m_addr_known != nullptr; +} + +/** + * Whether the peer supports the address. For example, a peer that does not + * implement BIP155 cannot receive Tor v3 addresses because it requires + * ADDRv2 (BIP155) encoding. + */ +static bool IsAddrCompatible(const Peer& peer, const CAddress& addr) +{ + return peer.m_wants_addrv2 || addr.IsAddrV1Compatible(); +} + +static void AddAddressKnown(Peer& peer, const CAddress& addr) +{ + assert(peer.m_addr_known); + peer.m_addr_known->insert(addr.GetKey()); +} + +static void PushAddress(Peer& peer, const CAddress& addr, FastRandomContext& insecure_rand) +{ + // Known checking here is only to save space from duplicates. + // Before sending, we'll filter it again for known addresses that were + // added after addresses were pushed. + assert(peer.m_addr_known); + if (addr.IsValid() && !peer.m_addr_known->contains(addr.GetKey()) && IsAddrCompatible(peer, addr)) { + if (peer.m_addrs_to_send.size() >= MAX_ADDR_TO_SEND) { + peer.m_addrs_to_send[insecure_rand.randrange(peer.m_addrs_to_send.size())] = addr; + } else { + peer.m_addrs_to_send.push_back(addr); + } + } +} + static void UpdatePreferredDownload(const CNode& node, CNodeState* state) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { nPreferredDownload -= state->fPreferredDownload; @@ -1244,7 +1313,9 @@ void PeerManagerImpl::InitializeNode(CNode *pnode) { mapNodeState.emplace_hint(mapNodeState.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(addr, pnode->IsInboundConn())); } { - PeerRef peer = std::make_shared(nodeid); + // Addr relay is disabled for outbound block-relay-only peers to + // prevent adversaries from inferring these links from addr traffic. + PeerRef peer = std::make_shared(nodeid, /* addr_relay = */ !pnode->IsBlockOnlyConn()); LOCK(m_peer_mutex); m_peer_map.emplace_hint(m_peer_map.end(), nodeid, std::move(peer)); } @@ -2002,59 +2073,49 @@ void PeerManagerImpl::RelayTransaction(const uint256& txid) }); } -/** - * Relay (gossip) an address to a few randomly chosen nodes. - * We choose the same nodes within a given 24h window (if the list of connected - * nodes does not change) and we don't relay to nodes that already know an - * address. So within 24h we will likely relay a given address once. This is to - * prevent a peer from unjustly giving their address better propagation by sending - * it to us repeatedly. - * @param[in] originator The peer that sent us the address. We don't want to relay it back. - * @param[in] addr Address to relay. - * @param[in] fReachable Whether the address' network is reachable. We relay unreachable - * addresses less. - * @param[in] connman Connection manager to choose nodes to relay to. - */ -static void RelayAddress(const CNode& originator, - const CAddress& addr, - bool fReachable, - const CConnman& connman) +void PeerManagerImpl::RelayAddress(NodeId originator, + const CAddress& addr, + bool fReachable) { + // We choose the same nodes within a given 24h window (if the list of connected + // nodes does not change) and we don't relay to nodes that already know an + // address. So within 24h we will likely relay a given address once. This is to + // prevent a peer from unjustly giving their address better propagation by sending + // it to us repeatedly. + if (!fReachable && !addr.IsRelayable()) return; // Relay to a limited number of other nodes // Use deterministic randomness to send to the same nodes for 24 hours // at a time so the m_addr_knowns of the chosen nodes prevent repeats uint64_t hashAddr = addr.GetHash(); - const CSipHasher hasher = connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY).Write(hashAddr << 32).Write((GetTime() + hashAddr) / (24 * 60 * 60)); + const CSipHasher hasher = m_connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY).Write(hashAddr << 32).Write((GetTime() + hashAddr) / (24 * 60 * 60)); FastRandomContext insecure_rand; // Relay reachable addresses to 2 peers. Unreachable addresses are relayed randomly to 1 or 2 peers. unsigned int nRelayNodes = (fReachable || (hasher.Finalize() & 1)) ? 2 : 1; - std::array,2> best{{{0, nullptr}, {0, nullptr}}}; + std::array, 2> best{{{0, nullptr}, {0, nullptr}}}; assert(nRelayNodes <= best.size()); - auto sortfunc = [&best, &hasher, nRelayNodes, &originator, &addr](CNode* pnode) { - if (pnode->RelayAddrsWithConn() && pnode != &originator && pnode->IsAddrCompatible(addr)) { - uint64_t hashKey = CSipHasher(hasher).Write(pnode->GetId()).Finalize(); + LOCK(m_peer_mutex); + + for (auto& [id, peer] : m_peer_map) { + if (RelayAddrsWithPeer(*peer) && id != originator && IsAddrCompatible(*peer, addr)) { + uint64_t hashKey = CSipHasher(hasher).Write(id).Finalize(); for (unsigned int i = 0; i < nRelayNodes; i++) { if (hashKey > best[i].first) { std::copy(best.begin() + i, best.begin() + nRelayNodes - 1, best.begin() + i + 1); - best[i] = std::make_pair(hashKey, pnode); + best[i] = std::make_pair(hashKey, peer.get()); break; } } } }; - auto pushfunc = [&addr, &best, nRelayNodes, &insecure_rand] { - for (unsigned int i = 0; i < nRelayNodes && best[i].first != 0; i++) { - best[i].second->PushAddress(addr, insecure_rand); - } - }; - - connman.ForEachNodeThen(std::move(sortfunc), std::move(pushfunc)); + for (unsigned int i = 0; i < nRelayNodes && best[i].first != 0; i++) { + PushAddress(*best[i].second, addr, insecure_rand); + } } void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& inv, llmq::CInstantSendManager& isman) @@ -2142,7 +2203,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& } else if (inv.IsMsgFilteredBlk()) { bool sendMerkleBlock = false; CMerkleBlock merkleBlock; - if (pfrom.RelayAddrsWithConn()) { + if (RelayAddrsWithPeer(peer)) { LOCK(pfrom.m_tx_relay->cs_filter); if (pfrom.m_tx_relay->pfilter) { sendMerkleBlock = true; @@ -2245,8 +2306,8 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic const std::chrono::seconds now = GetTime(); // Get last mempool request time - const std::chrono::seconds mempool_req = pfrom.RelayAddrsWithConn() ? pfrom.m_tx_relay->m_last_mempool_req.load() - : std::chrono::seconds::min(); + const std::chrono::seconds mempool_req = RelayAddrsWithPeer(peer) ? pfrom.m_tx_relay->m_last_mempool_req.load() + : std::chrono::seconds::min(); // Process as many TX items from the front of the getdata queue as // possible, since they're common and it's efficient to batch process @@ -2266,7 +2327,7 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic } ++it; - if (!pfrom.RelayAddrsWithConn() && NetMessageViolatesBlocksOnly(inv.GetCommand())) { + if (!RelayAddrsWithPeer(peer) && NetMessageViolatesBlocksOnly(inv.GetCommand())) { // Note that if we receive a getdata for non-block messages // from a block-relay-only outbound peer that violate the policy, // we skip such getdata messages from this peer @@ -3109,7 +3170,7 @@ void PeerManagerImpl::ProcessMessage( // set nodes not capable of serving the complete blockchain history as "limited nodes" pfrom.m_limited_node = (!(nServices & NODE_NETWORK) && (nServices & NODE_NETWORK_LIMITED)); - if (pfrom.RelayAddrsWithConn()) { + if (RelayAddrsWithPeer(*peer)) { LOCK(pfrom.m_tx_relay->cs_filter); pfrom.m_tx_relay->fRelayTxes = fRelay; // set to true after we get the first filter* message } @@ -3138,17 +3199,17 @@ void PeerManagerImpl::ProcessMessage( if (addr.IsRoutable()) { LogPrint(BCLog::NET, "ProcessMessages: advertising address %s\n", addr.ToString()); - pfrom.PushAddress(addr, insecure_rand); + PushAddress(*peer, addr, insecure_rand); } else if (IsPeerAddrLocalGood(&pfrom)) { addr.SetIP(addrMe); LogPrint(BCLog::NET, "ProcessMessages: advertising address %s\n", addr.ToString()); - pfrom.PushAddress(addr, insecure_rand); + PushAddress(*peer, addr, insecure_rand); } } // Get recent addresses m_connman.PushMessage(&pfrom, CNetMsgMaker(greatest_common_version).Make(NetMsgType::GETADDR)); - pfrom.fGetAddr = true; + peer->m_getaddr_sent = true; } if (!pfrom.IsInboundConn()) { @@ -3261,7 +3322,7 @@ void PeerManagerImpl::ProcessMessage( pfrom.fDisconnect = true; return; } - pfrom.m_wants_addrv2 = true; + peer->m_wants_addrv2 = true; return; } @@ -3303,7 +3364,7 @@ void PeerManagerImpl::ProcessMessage( s >> vAddr; - if (!pfrom.RelayAddrsWithConn()) { + if (!RelayAddrsWithPeer(*peer)) { LogPrint(BCLog::NET, "ignoring %s message from %s peer=%d\n", msg_type, pfrom.ConnectionTypeAsString(), pfrom.GetId()); return; } @@ -3330,24 +3391,22 @@ void PeerManagerImpl::ProcessMessage( if (addr.nTime <= 100000000 || addr.nTime > nNow + 10 * 60) addr.nTime = nNow - 5 * 24 * 60 * 60; - pfrom.AddAddressKnown(addr); + AddAddressKnown(*peer, addr); if (m_banman && (m_banman->IsDiscouraged(addr) || m_banman->IsBanned(addr))) { // Do not process banned/discouraged addresses beyond remembering we received them continue; } bool fReachable = IsReachable(addr); - if (addr.nTime > nSince && !pfrom.fGetAddr && vAddr.size() <= 10 && addr.IsRoutable()) - { + if (addr.nTime > nSince && !peer->m_getaddr_sent && vAddr.size() <= 10 && addr.IsRoutable()) { // Relay to a limited number of other nodes - RelayAddress(pfrom, addr, fReachable, m_connman); + RelayAddress(pfrom.GetId(), addr, fReachable); } // Do not store addresses outside our network if (fReachable) vAddrOk.push_back(addr); } m_addrman.Add(vAddrOk, pfrom.addr, 2 * 60 * 60); - if (vAddr.size() < 1000) - pfrom.fGetAddr = false; + if (vAddr.size() < 1000) peer->m_getaddr_sent = false; if (pfrom.IsAddrFetchConn()) { LogPrint(BCLog::NET_NETCONN, "addrfetch connection completed peer=%d; disconnecting\n", pfrom.GetId()); pfrom.fDisconnect = true; @@ -4250,14 +4309,14 @@ void PeerManagerImpl::ProcessMessage( } // Only send one GetAddr response per connection to reduce resource waste - // and discourage addr stamping of INV announcements. - if (pfrom.fSentAddr) { + // and discourage addr stamping of INV announcements. + if (peer->m_getaddr_recvd) { LogPrint(BCLog::NET, "Ignoring repeated \"getaddr\". peer=%d\n", pfrom.GetId()); return; } - pfrom.fSentAddr = true; + peer->m_getaddr_recvd = true; - pfrom.vAddrToSend.clear(); + peer->m_addrs_to_send.clear(); std::vector vAddr; if (pfrom.HasPermission(NetPermissionFlags::Addr)) { vAddr = m_connman.GetAddresses(MAX_ADDR_TO_SEND, MAX_PCT_ADDR_TO_SEND, /* network */ std::nullopt); @@ -4266,7 +4325,7 @@ void PeerManagerImpl::ProcessMessage( } FastRandomContext insecure_rand; for (const CAddress &addr : vAddr) { - pfrom.PushAddress(addr, insecure_rand); + PushAddress(*peer, addr, insecure_rand); } return; } @@ -4292,7 +4351,7 @@ void PeerManagerImpl::ProcessMessage( return; } - if (pfrom.RelayAddrsWithConn()) { + if (RelayAddrsWithPeer(*peer)) { LOCK(pfrom.m_tx_relay->cs_tx_inventory); pfrom.m_tx_relay->fSendMempool = true; } @@ -4386,7 +4445,7 @@ void PeerManagerImpl::ProcessMessage( // There is no excuse for sending a too-large filter Misbehaving(pfrom.GetId(), 100, "too-large bloom filter"); } - else if (pfrom.RelayAddrsWithConn()) + else if (RelayAddrsWithPeer(*peer)) { LOCK(pfrom.m_tx_relay->cs_filter); pfrom.m_tx_relay->pfilter.reset(new CBloomFilter(filter)); @@ -4409,7 +4468,7 @@ void PeerManagerImpl::ProcessMessage( bool bad = false; if (vData.size() > MAX_SCRIPT_ELEMENT_SIZE) { bad = true; - } else if (pfrom.RelayAddrsWithConn()) { + } else if (RelayAddrsWithPeer(*peer)) { LOCK(pfrom.m_tx_relay->cs_filter); if (pfrom.m_tx_relay->pfilter) { pfrom.m_tx_relay->pfilter->insert(vData); @@ -4429,7 +4488,7 @@ void PeerManagerImpl::ProcessMessage( pfrom.fDisconnect = true; return; } - if (!pfrom.RelayAddrsWithConn()) { + if (!RelayAddrsWithPeer(*peer)) { return; } LOCK(pfrom.m_tx_relay->cs_filter); @@ -4900,72 +4959,70 @@ void PeerManagerImpl::MaybeSendPing(CNode& node_to, Peer& peer, std::chrono::mic } } -void PeerManagerImpl::MaybeSendAddr(CNode& node, std::chrono::microseconds current_time) +void PeerManagerImpl::MaybeSendAddr(CNode& node, Peer& peer, std::chrono::microseconds current_time) { // Nothing to do for non-address-relay peers - if (!node.RelayAddrsWithConn()) return; - - assert(node.m_addr_known); + if (!RelayAddrsWithPeer(peer)) return; - LOCK(node.m_addr_send_times_mutex); + LOCK(peer.m_addr_send_times_mutex); // Periodically advertise our local address to the peer. if (fListen && !m_chainman.ActiveChainstate().IsInitialBlockDownload() && - node.m_next_local_addr_send < current_time) { + peer.m_next_local_addr_send < current_time) { // If we've sent before, clear the bloom filter for the peer, so that our // self-announcement will actually go out. // This might be unnecessary if the bloom filter has already rolled // over since our last self-announcement, but there is only a small // bandwidth cost that we can incur by doing this (which happens // once a day on average). - if (node.m_next_local_addr_send != 0us) { - node.m_addr_known->reset(); + if (peer.m_next_local_addr_send != 0us) { + peer.m_addr_known->reset(); } if (std::optional local_addr = GetLocalAddrForPeer(&node)) { FastRandomContext insecure_rand; - node.PushAddress(*local_addr, insecure_rand); + PushAddress(peer, *local_addr, insecure_rand); } - node.m_next_local_addr_send = PoissonNextSend(current_time, AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL); + peer.m_next_local_addr_send = PoissonNextSend(current_time, AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL); } // We sent an `addr` message to this peer recently. Nothing more to do. - if (current_time <= node.m_next_addr_send) return; + if (current_time <= peer.m_next_addr_send) return; - node.m_next_addr_send = PoissonNextSend(current_time, AVG_ADDRESS_BROADCAST_INTERVAL); + peer.m_next_addr_send = PoissonNextSend(current_time, AVG_ADDRESS_BROADCAST_INTERVAL); - if (!Assume(node.vAddrToSend.size() <= MAX_ADDR_TO_SEND)) { + if (!Assume(peer.m_addrs_to_send.size() <= MAX_ADDR_TO_SEND)) { // Should be impossible since we always check size before adding to - // vAddrToSend. Recover by trimming the vector. - node.vAddrToSend.resize(MAX_ADDR_TO_SEND); + // m_addrs_to_send. Recover by trimming the vector. + peer.m_addrs_to_send.resize(MAX_ADDR_TO_SEND); } // Remove addr records that the peer already knows about, and add new // addrs to the m_addr_known filter on the same pass. - auto addr_already_known = [&node](const CAddress& addr) { - bool ret = node.m_addr_known->contains(addr.GetKey()); - if (!ret) node.m_addr_known->insert(addr.GetKey()); + auto addr_already_known = [&peer](const CAddress& addr) { + bool ret = peer.m_addr_known->contains(addr.GetKey()); + if (!ret) peer.m_addr_known->insert(addr.GetKey()); return ret; }; - node.vAddrToSend.erase(std::remove_if(node.vAddrToSend.begin(), node.vAddrToSend.end(), addr_already_known), - node.vAddrToSend.end()); + peer.m_addrs_to_send.erase(std::remove_if(peer.m_addrs_to_send.begin(), peer.m_addrs_to_send.end(), addr_already_known), + peer.m_addrs_to_send.end()); // No addr messages to send - if (node.vAddrToSend.empty()) return; + if (peer.m_addrs_to_send.empty()) return; const char* msg_type; int make_flags; - if (node.m_wants_addrv2) { + if (peer.m_wants_addrv2) { msg_type = NetMsgType::ADDRV2; make_flags = ADDRV2_FORMAT; } else { msg_type = NetMsgType::ADDR; make_flags = 0; } - m_connman.PushMessage(&node, CNetMsgMaker(node.GetCommonVersion()).Make(make_flags, msg_type, node.vAddrToSend)); - node.vAddrToSend.clear(); + m_connman.PushMessage(&node, CNetMsgMaker(node.GetCommonVersion()).Make(make_flags, msg_type, peer.m_addrs_to_send)); + peer.m_addrs_to_send.clear(); // we only send the big addr message once - if (node.vAddrToSend.capacity() > 40) { - node.vAddrToSend.shrink_to_fit(); + if (peer.m_addrs_to_send.capacity() > 40) { + peer.m_addrs_to_send.shrink_to_fit(); } } @@ -5014,7 +5071,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) // MaybeSendPing may have marked peer for disconnection if (pto->fDisconnect) return true; - MaybeSendAddr(*pto, current_time); + MaybeSendAddr(*pto, *peer, current_time); { LOCK(cs_main); @@ -5216,7 +5273,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) LOCK2(m_mempool.cs, peer->m_block_inv_mutex); size_t reserve = INVENTORY_BROADCAST_MAX_PER_1MB_BLOCK * MaxBlockSize() / 1000000; - if (pto->RelayAddrsWithConn()) { + if (RelayAddrsWithPeer(*peer)) { LOCK(pto->m_tx_relay->cs_tx_inventory); reserve = std::min(pto->m_tx_relay->setInventoryTxToSend.size(), reserve); } @@ -5247,7 +5304,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) } }; - if (pto->RelayAddrsWithConn()) { + if (RelayAddrsWithPeer(*peer)) { LOCK(pto->m_tx_relay->cs_tx_inventory); // Check whether periodic sends should happen // Note: If this node is running in a Masternode mode, it makes no sense to delay outgoing txes diff --git a/src/test/fuzz/net.cpp b/src/test/fuzz/net.cpp index cb8ae2b41c41..8370ac8aa034 100644 --- a/src/test/fuzz/net.cpp +++ b/src/test/fuzz/net.cpp @@ -60,27 +60,6 @@ FUZZ_TARGET_INIT(net, initialize_net) node.Release(); } }, - [&] { - if (node.m_addr_known == nullptr) { - return; - } - const std::optional addr_opt = ConsumeDeserializable(fuzzed_data_provider); - if (!addr_opt) { - return; - } - node.AddAddressKnown(*addr_opt); - }, - [&] { - if (node.m_addr_known == nullptr) { - return; - } - const std::optional addr_opt = ConsumeDeserializable(fuzzed_data_provider); - if (!addr_opt) { - return; - } - FastRandomContext fast_random_context{ConsumeUInt256(fuzzed_data_provider)}; - node.PushAddress(*addr_opt, fast_random_context); - }, [&] { const std::optional inv_opt = ConsumeDeserializable(fuzzed_data_provider); if (!inv_opt) { @@ -117,7 +96,6 @@ FUZZ_TARGET_INIT(net, initialize_net) const int ref_count = node.GetRefCount(); assert(ref_count >= 0); (void)node.GetCommonVersion(); - (void)node.RelayAddrsWithConn(); const NetPermissionFlags net_permission_flags = ConsumeWeakEnum(fuzzed_data_provider, ALL_NET_PERMISSION_FLAGS); (void)node.HasPermission(net_permission_flags); From 2e55327f55312ad53ff04709753d5094cd6bcfc4 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 28 Mar 2024 18:50:04 +0000 Subject: [PATCH 18/19] net: introduce CanRelayAddrs as RelayAddrsWithConn substitute Since bitcoin#21186, mutual exclusivity is not a given (i.e. RelayAddrsWithConn != !IsBlockOnlyConn), we should use RelayPeersWithConn for a definitive answer and since relying on a no-longer-true property breaks InstantSend, let's fetch the right answer instead. --- src/llmq/instantsend.cpp | 10 +++++----- src/llmq/instantsend.h | 2 +- src/net_processing.cpp | 9 +++++++++ src/net_processing.h | 3 +++ 4 files changed, 18 insertions(+), 6 deletions(-) diff --git a/src/llmq/instantsend.cpp b/src/llmq/instantsend.cpp index 927a8088ca08..c0ba51d3bd61 100644 --- a/src/llmq/instantsend.cpp +++ b/src/llmq/instantsend.cpp @@ -1068,7 +1068,7 @@ void CInstantSendManager::ProcessInstantSendLock(NodeId from, const uint256& has // bump mempool counter to make sure newly locked txes are picked up by getblocktemplate mempool.AddTransactionsUpdated(1); } else { - AskNodesForLockedTx(islock->txid, connman); + AskNodesForLockedTx(islock->txid, connman, *m_peerman.load()); } } @@ -1344,7 +1344,7 @@ void CInstantSendManager::RemoveMempoolConflictsForLock(const uint256& hash, con for (const auto& p : toDelete) { RemoveConflictedTx(*p.second); } - AskNodesForLockedTx(islock.txid, connman); + AskNodesForLockedTx(islock.txid, connman, *m_peerman.load()); } } @@ -1449,16 +1449,16 @@ void CInstantSendManager::RemoveConflictingLock(const uint256& islockHash, const } } -void CInstantSendManager::AskNodesForLockedTx(const uint256& txid, const CConnman& connman) +void CInstantSendManager::AskNodesForLockedTx(const uint256& txid, const CConnman& connman, const PeerManager& peerman) { std::vector nodesToAskFor; nodesToAskFor.reserve(4); - auto maybe_add_to_nodesToAskFor = [&nodesToAskFor, &txid](CNode* pnode) { + auto maybe_add_to_nodesToAskFor = [&peerman, &nodesToAskFor, &txid](CNode* pnode) { if (nodesToAskFor.size() >= 4) { return; } - if (!pnode->IsBlockOnlyConn()) { + if (peerman.CanRelayAddrs(pnode->GetId())) { LOCK(pnode->m_tx_relay->cs_tx_inventory); if (pnode->m_tx_relay->filterInventoryKnown.contains(txid)) { pnode->AddRef(); diff --git a/src/llmq/instantsend.h b/src/llmq/instantsend.h index bddf057f6acd..cd141b65b586 100644 --- a/src/llmq/instantsend.h +++ b/src/llmq/instantsend.h @@ -299,7 +299,7 @@ class CInstantSendManager : public CRecoveredSigsListener void RemoveMempoolConflictsForLock(const uint256& hash, const CInstantSendLock& islock); void ResolveBlockConflicts(const uint256& islockHash, const CInstantSendLock& islock) LOCKS_EXCLUDED(cs_pendingLocks, cs_nonLocked); - static void AskNodesForLockedTx(const uint256& txid, const CConnman& connman); + static void AskNodesForLockedTx(const uint256& txid, const CConnman& connman, const PeerManager& peerman); void ProcessPendingRetryLockTxs() LOCKS_EXCLUDED(cs_creating, cs_nonLocked, cs_pendingRetry); void WorkThreadMain(); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 34330318eefa..47ba9c613dbf 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -325,6 +325,7 @@ class PeerManagerImpl final : public PeerManager void ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv, const std::chrono::microseconds time_received, const std::atomic& interruptMsgProc) override; bool IsBanned(NodeId pnode) override EXCLUSIVE_LOCKS_REQUIRED(cs_main); + bool CanRelayAddrs(NodeId pnode) const override; private: /** Helper to process result of external handlers of message */ @@ -1630,6 +1631,14 @@ bool PeerManagerImpl::IsBanned(NodeId pnode) return false; } +bool PeerManagerImpl::CanRelayAddrs(NodeId pnode) const +{ + PeerRef peer = GetPeerRef(pnode); + if (peer == nullptr) + return false; + return RelayAddrsWithPeer(*peer); +} + bool PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidationState& state, bool via_compact_block, const std::string& message) { diff --git a/src/net_processing.h b/src/net_processing.h index d868d1025676..680dd2cae56b 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -87,6 +87,9 @@ class PeerManager : public CValidationInterface, public NetEventsInterface virtual bool IsBanned(NodeId pnode) = 0; + /* Can we send addr messages to a peer. Used by InstantSend. */ + virtual bool CanRelayAddrs(NodeId pnode) const = 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}; From e89c555b0f7ade0d2e2e599a15414bd3129a5a59 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Tue, 26 Mar 2024 17:22:54 +0000 Subject: [PATCH 19/19] merge bitcoin#22107: rename GetSystemTimeInSeconds to GetTimeSeconds --- src/bitcoin-cli.cpp | 2 +- src/masternode/utils.cpp | 4 ++-- src/net.cpp | 12 ++++++------ src/net_processing.cpp | 2 +- src/qt/rpcconsole.cpp | 2 +- src/test/util_tests.cpp | 2 +- src/util/time.cpp | 2 +- src/util/time.h | 4 ++-- 8 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp index b1631e410bb2..92334ba28b06 100644 --- a/src/bitcoin-cli.cpp +++ b/src/bitcoin-cli.cpp @@ -426,7 +426,7 @@ class NetinfoRequestHandler : public BaseRequestHandler "See this help\n" "> dash-cli -netinfo help\n"}; } - const int64_t m_time_now{GetSystemTimeInSeconds()}; + const int64_t m_time_now{GetTimeSeconds()}; public: static constexpr int ID_PEERINFO = 0; diff --git a/src/masternode/utils.cpp b/src/masternode/utils.cpp index 651264d3f8a3..29469139558e 100644 --- a/src/masternode/utils.cpp +++ b/src/masternode/utils.cpp @@ -48,7 +48,7 @@ void CMasternodeUtils::DoMaintenance(CConnman& connman, const CMasternodeSync& m 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 (GetSystemTimeInSeconds() - pnode->nTimeConnected < PROBE_WAIT_INTERVAL) return; + if (GetTimeSeconds() - pnode->nTimeConnected < PROBE_WAIT_INTERVAL) return; } else { // we're only disconnecting m_masternode_connection connections if (!pnode->m_masternode_connection) return; @@ -65,7 +65,7 @@ void CMasternodeUtils::DoMaintenance(CConnman& connman, const CMasternodeSync& m if (pnode->IsInboundConn()) { return; } - } else if (GetSystemTimeInSeconds() - pnode->nTimeConnected < 5) { + } else if (GetTimeSeconds() - pnode->nTimeConnected < 5) { // non-verified, give it some time to verify itself return; } else if (pnode->qwatch) { diff --git a/src/net.cpp b/src/net.cpp index b114a4b1ae99..49324f14120c 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -878,7 +878,7 @@ size_t CConnman::SocketSendData(CNode& node) nBytes = send(node.hSocket, reinterpret_cast(data.data()) + node.nSendOffset, data.size() - node.nSendOffset, MSG_NOSIGNAL | MSG_DONTWAIT); } if (nBytes > 0) { - node.nLastSend = GetSystemTimeInSeconds(); + node.nLastSend = GetTimeSeconds(); node.nSendBytes += nBytes; node.nSendOffset += nBytes; nSentSize += nBytes; @@ -1072,7 +1072,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 = GetSystemTimeInSeconds() - node->nTimeConnected < INBOUND_EVICTION_PROTECTION_TIME; + bool isProtected = GetTimeSeconds() - node->nTimeConnected < INBOUND_EVICTION_PROTECTION_TIME; if (node->nTimeFirstMessageReceived != 0 && !node->fFirstMessageIsMNAUTH) { isProtected = false; } @@ -1490,7 +1490,7 @@ void CConnman::CalculateNumConnectionsChangedStats() bool CConnman::ShouldRunInactivityChecks(const CNode& node, std::optional now_in) const { - const int64_t now = now_in ? now_in.value() : GetSystemTimeInSeconds(); + const int64_t now = now_in ? now_in.value() : GetTimeSeconds(); return node.nTimeConnected + m_peer_connect_timeout < now; } @@ -1498,7 +1498,7 @@ bool CConnman::InactivityCheck(const CNode& node) const { // Use non-mockable system time (otherwise these timers will pop when we // use setmocktime in the tests). - int64_t now = GetSystemTimeInSeconds(); + int64_t now = GetTimeSeconds(); if (!ShouldRunInactivityChecks(node, now)) return false; @@ -2718,7 +2718,7 @@ void CConnman::ThreadOpenMasternodeConnections() // we probably connected to it before it became a masternode // or maybe we are still waiting for mnauth (void)ForNode(addr2, [&](CNode* pnode) { - if (pnode->nTimeFirstMessageReceived != 0 && GetSystemTimeInSeconds() - pnode->nTimeFirstMessageReceived > 5) { + if (pnode->nTimeFirstMessageReceived != 0 && GetTimeSeconds() - pnode->nTimeFirstMessageReceived > 5) { // clearly not expecting mnauth to take that long even if it wasn't the first message // we received (as it should normally), disconnect LogPrint(BCLog::NET_NETCONN, "CConnman::%s -- dropping non-mnauth connection to %s, service=%s\n", _func_, proRegTxHash.ToString(), addr2.ToString(false)); @@ -4033,7 +4033,7 @@ ServiceFlags CConnman::GetLocalServices() const unsigned int CConnman::GetReceiveFloodSize() const { return nReceiveFloodSize; } CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, SOCKET hSocketIn, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress& addrBindIn, const std::string& addrNameIn, ConnectionType conn_type_in, bool inbound_onion) - : nTimeConnected(GetSystemTimeInSeconds()), + : nTimeConnected(GetTimeSeconds()), addr(addrIn), addrBind(addrBindIn), nKeyedNetGroup(nKeyedNetGroupIn), diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 47ba9c613dbf..23cd76cc7e93 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3342,7 +3342,7 @@ void PeerManagerImpl::ProcessMessage( if (pfrom.nTimeFirstMessageReceived == 0) { // First message after VERSION/VERACK - pfrom.nTimeFirstMessageReceived = GetSystemTimeInSeconds(); + pfrom.nTimeFirstMessageReceived = GetTimeSeconds(); pfrom.fFirstMessageIsMNAUTH = msg_type == NetMsgType::MNAUTH; // Note: do not break the flow here diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp index 30f393ba60d1..9a05ddb62ba9 100644 --- a/src/qt/rpcconsole.cpp +++ b/src/qt/rpcconsole.cpp @@ -1244,7 +1244,7 @@ void RPCConsole::updateNodeDetail(const CNodeCombinedStats *stats) ui->peerHeading->setText(peerAddrDetails); ui->peerServices->setText(GUIUtil::formatServicesStr(stats->nodeStats.nServices)); ui->peerRelayTxes->setText(stats->nodeStats.fRelayTxes ? "Yes" : "No"); - const int64_t time_now{GetSystemTimeInSeconds()}; + const int64_t time_now{GetTimeSeconds()}; ui->peerConnTime->setText(GUIUtil::formatDurationStr(time_now - stats->nodeStats.nTimeConnected)); ui->peerLastBlock->setText(TimeDurationField(time_now, stats->nodeStats.nLastBlockTime)); ui->peerLastTx->setText(TimeDurationField(time_now, stats->nodeStats.nLastTXTime)); diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index a0f743a6e764..d5eafe282409 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -268,7 +268,7 @@ BOOST_AUTO_TEST_CASE(util_FormatParseISO8601DateTime) BOOST_CHECK_EQUAL(ParseISO8601DateTime("1960-01-01T00:00:00Z"), 0); BOOST_CHECK_EQUAL(ParseISO8601DateTime("2011-09-30T23:36:17Z"), 1317425777); - auto time = GetSystemTimeInSeconds(); + auto time = GetTimeSeconds(); BOOST_CHECK_EQUAL(ParseISO8601DateTime(FormatISO8601DateTime(time)), time); } diff --git a/src/util/time.cpp b/src/util/time.cpp index d8f37f5843c1..14183d540d8b 100644 --- a/src/util/time.cpp +++ b/src/util/time.cpp @@ -115,7 +115,7 @@ int64_t GetTimeMicros() return int64_t{GetSystemTime().count()}; } -int64_t GetSystemTimeInSeconds() +int64_t GetTimeSeconds() { return int64_t{GetSystemTime().count()}; } diff --git a/src/util/time.h b/src/util/time.h index 3eefa353ae62..36300b29fd11 100644 --- a/src/util/time.h +++ b/src/util/time.h @@ -44,7 +44,7 @@ inline double CountSecondsDouble(SecondsDouble t) { return t.count(); } /** * DEPRECATED - * Use either GetSystemTimeInSeconds (not mockable) or GetTime (mockable) + * Use either GetTimeSeconds (not mockable) or GetTime (mockable) */ int64_t GetTime(); @@ -53,7 +53,7 @@ int64_t GetTimeMillis(); /** Returns the system time (not mockable) */ int64_t GetTimeMicros(); /** Returns the system time (not mockable) */ -int64_t GetSystemTimeInSeconds(); // Like GetTime(), but not mockable +int64_t GetTimeSeconds(); // Like GetTime(), but not mockable /** * DEPRECATED