From 1f52c47d5c09b59fd3153700751c74e63edc7d7e Mon Sep 17 00:00:00 2001 From: John Newbery Date: Mon, 20 Jul 2020 14:01:05 +0100 Subject: [PATCH 1/7] [net processing] Add m_our_services and m_their_services to Peer Track services offered by us and the peer in the Peer object. --- src/net.cpp | 4 +-- src/net.h | 16 +----------- src/net_processing.cpp | 39 ++++++++++++++++++++++++------ src/test/denialofservice_tests.cpp | 12 ++++----- src/test/net_tests.cpp | 2 +- src/test/util/net.cpp | 2 +- 6 files changed, 42 insertions(+), 33 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 90c73d583e5..5082d6eb685 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1026,7 +1026,7 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr&& sock, pnode->AddRef(); pnode->m_permissionFlags = permissionFlags; pnode->m_prefer_evict = discouraged; - m_msgproc->InitializeNode(pnode); + m_msgproc->InitializeNode(*pnode, nodeServices); LogPrint(BCLog::NET, "connection from %s accepted\n", addr.ToString()); @@ -1964,7 +1964,7 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai if (grantOutbound) grantOutbound->MoveTo(pnode->grantOutbound); - m_msgproc->InitializeNode(pnode); + m_msgproc->InitializeNode(*pnode, nLocalServices); { LOCK(m_nodes_mutex); m_nodes.push_back(pnode); diff --git a/src/net.h b/src/net.h index 6453ad1dc7c..00e8a780200 100644 --- a/src/net.h +++ b/src/net.h @@ -592,20 +592,6 @@ private: std::atomic m_greatest_common_version{INIT_PROTO_VERSION}; //! Services offered to this peer. - //! - //! This is supplied by the parent CConnman during peer connection - //! (CConnman::ConnectNode()) from its attribute of the same name. - //! - //! This is const because there is no protocol defined for renegotiating - //! services initially offered to a peer. The set of local services we - //! offer should not change after initialization. - //! - //! An interesting example of this is NODE_NETWORK and initial block - //! download: a node which starts up from scratch doesn't have any blocks - //! to serve, but still advertises NODE_NETWORK because it will eventually - //! fulfill this role after IBD completes. P2P code is written in such a - //! way that it can gracefully handle peers who don't make good on their - //! service advertisements. const ServiceFlags nLocalServices; std::list vRecvMsg; // Used only by SocketHandler thread @@ -625,7 +611,7 @@ class NetEventsInterface { public: /** Initialize a peer (setup state, queue any initial messages) */ - virtual void InitializeNode(CNode* pnode) = 0; + virtual void InitializeNode(CNode& node, ServiceFlags our_services) = 0; /** Handle removal of a peer (clear state) */ virtual void FinalizeNode(const CNode& node) = 0; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index c33dd299233..7c403d73db4 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -207,6 +207,27 @@ struct Peer { /** Same id as the CNode object for this peer */ const NodeId m_id{0}; + /** Services we offered to this peer. + * + * This is supplied by CConnman during peer initialization. It's const + * because there is no protocol defined for renegotiating services + * initially offered to a peer. The set of local services we offer should + * not change after initialization. + * + * An interesting example of this is NODE_NETWORK and initial block + * download: a node which starts up from scratch doesn't have any blocks + * to serve, but still advertises NODE_NETWORK because it will eventually + * fulfill this role after IBD completes. P2P code is written in such a + * way that it can gracefully handle peers who don't make good on their + * service advertisements. + * + * TODO: remove redundant CNode::nLocalServices*/ + const ServiceFlags m_our_services; + /** Services this peer offered to us. + * + * TODO: remove redundant CNode::nServices */ + std::atomic m_their_services{NODE_NONE}; + /** Protects misbehavior data members */ Mutex m_misbehavior_mutex; /** Accumulated misbehavior score for this peer */ @@ -360,8 +381,9 @@ struct Peer { /** Time of the last getheaders message to this peer */ std::atomic m_last_getheaders_timestamp{NodeSeconds{}}; - Peer(NodeId id) + explicit Peer(NodeId id, ServiceFlags our_services) : m_id{id} + , m_our_services{our_services} {} private: @@ -482,7 +504,7 @@ public: EXCLUSIVE_LOCKS_REQUIRED(!m_most_recent_block_mutex); /** Implement NetEventsInterface */ - void InitializeNode(CNode* pnode) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); + void InitializeNode(CNode& node, ServiceFlags our_services) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); void FinalizeNode(const CNode& node) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); bool ProcessMessages(CNode* pfrom, std::atomic& interrupt) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex, !m_most_recent_block_mutex); @@ -1299,21 +1321,21 @@ void PeerManagerImpl::UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_s if (state) state->m_last_block_announcement = time_in_seconds; } -void PeerManagerImpl::InitializeNode(CNode *pnode) +void PeerManagerImpl::InitializeNode(CNode& node, ServiceFlags our_services) { - NodeId nodeid = pnode->GetId(); + NodeId nodeid = node.GetId(); { LOCK(cs_main); - m_node_states.emplace_hint(m_node_states.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(pnode->IsInboundConn())); + m_node_states.emplace_hint(m_node_states.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(node.IsInboundConn())); assert(m_txrequest.Count(nodeid) == 0); } - PeerRef peer = std::make_shared(nodeid); + PeerRef peer = std::make_shared(nodeid, our_services); { LOCK(m_peer_mutex); m_peer_map.emplace_hint(m_peer_map.end(), nodeid, peer); } - if (!pnode->IsInboundConn()) { - PushNodeVersion(*pnode, *peer); + if (!node.IsInboundConn()) { + PushNodeVersion(node, *peer); } } @@ -2843,6 +2865,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::VERACK)); pfrom.nServices = nServices; + peer->m_their_services = nServices; pfrom.SetAddrLocal(addrMe); { LOCK(pfrom.m_subver_mutex); diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index c87ed82c88d..d77099f921e 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -65,7 +65,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) /*inbound_onion=*/false}; dummyNode1.SetCommonVersion(PROTOCOL_VERSION); - peerLogic->InitializeNode(&dummyNode1); + peerLogic->InitializeNode(dummyNode1, dummyNode1.GetLocalServices()); dummyNode1.fSuccessfullyConnected = true; // This test requires that we have a chain with non-zero work. @@ -124,7 +124,7 @@ static void AddRandomOutboundPeer(NodeId& id, std::vector& vNodes, PeerM CNode &node = *vNodes.back(); node.SetCommonVersion(PROTOCOL_VERSION); - peerLogic.InitializeNode(&node); + peerLogic.InitializeNode(node, node.GetLocalServices()); node.fSuccessfullyConnected = true; connman.AddTestNode(node); @@ -302,7 +302,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) ConnectionType::INBOUND, /*inbound_onion=*/false}; nodes[0]->SetCommonVersion(PROTOCOL_VERSION); - peerLogic->InitializeNode(nodes[0]); + peerLogic->InitializeNode(*nodes[0], nodes[0]->GetLocalServices()); nodes[0]->fSuccessfullyConnected = true; connman->AddTestNode(*nodes[0]); peerLogic->UnitTestMisbehaving(nodes[0]->GetId(), DISCOURAGEMENT_THRESHOLD); // Should be discouraged @@ -325,7 +325,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) ConnectionType::INBOUND, /*inbound_onion=*/false}; nodes[1]->SetCommonVersion(PROTOCOL_VERSION); - peerLogic->InitializeNode(nodes[1]); + peerLogic->InitializeNode(*nodes[1], nodes[1]->GetLocalServices()); nodes[1]->fSuccessfullyConnected = true; connman->AddTestNode(*nodes[1]); peerLogic->UnitTestMisbehaving(nodes[1]->GetId(), DISCOURAGEMENT_THRESHOLD - 1); @@ -363,7 +363,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) ConnectionType::OUTBOUND_FULL_RELAY, /*inbound_onion=*/false}; nodes[2]->SetCommonVersion(PROTOCOL_VERSION); - peerLogic->InitializeNode(nodes[2]); + peerLogic->InitializeNode(*nodes[2], nodes[2]->GetLocalServices()); nodes[2]->fSuccessfullyConnected = true; connman->AddTestNode(*nodes[2]); peerLogic->UnitTestMisbehaving(nodes[2]->GetId(), DISCOURAGEMENT_THRESHOLD); @@ -408,7 +408,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime) ConnectionType::INBOUND, /*inbound_onion=*/false}; dummyNode.SetCommonVersion(PROTOCOL_VERSION); - peerLogic->InitializeNode(&dummyNode); + peerLogic->InitializeNode(dummyNode, dummyNode.GetLocalServices()); dummyNode.fSuccessfullyConnected = true; peerLogic->UnitTestMisbehaving(dummyNode.GetId(), DISCOURAGEMENT_THRESHOLD); diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index 115c4b9b240..0ac508bea95 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -857,7 +857,7 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message) *static_cast(&m_node.chainman->ActiveChainstate()); chainstate.JumpOutOfIbd(); - m_node.peerman->InitializeNode(&peer); + m_node.peerman->InitializeNode(peer, peer.GetLocalServices()); std::atomic interrupt_dummy{false}; std::chrono::microseconds time_received_dummy{0}; diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index e70ad036bd9..90a49d52d7c 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -24,7 +24,7 @@ void ConnmanTestMsg::Handshake(CNode& node, auto& connman{*this}; const CNetMsgMaker mm{0}; - peerman.InitializeNode(&node); + peerman.InitializeNode(node, node.GetLocalServices()); CSerializedNetMsg msg_version{ mm.Make(NetMsgType::VERSION, From fc5eb528f7d7b33e2f2e443c5610a1551c7f099b Mon Sep 17 00:00:00 2001 From: John Newbery Date: Mon, 7 Feb 2022 17:13:54 +0000 Subject: [PATCH 2/7] [tests] Connect peer in outbound_slow_chain_eviction by sending p2p messages Prior to this commit, the peer was connected, and then the services and connectivity fields in the CNode object were manually set. Instead, send p2p `version` and `verack` messages, and have net_processing's internal logic set the state of the node. This ensures that the node's internal state is consistent with how it would be set in the live code. Prior to this commit, `dummyNode1.nServices` was set to `NODE_NONE` which was not a problem since `CNode::fClient` and `CNode::m_limited_node` are default initialised to false. Now that we are doing the actual version handshake, the values of `fClient` and `m_limited_node` are set during the handshake and cause the test to fail if we do not set `dummyNode1.nServices` to a reasonable value (NODE_NETWORK | NODE_WITNESS). --- src/test/denialofservice_tests.cpp | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index d77099f921e..f34ddf50abe 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -44,11 +45,10 @@ BOOST_FIXTURE_TEST_SUITE(denialofservice_tests, TestingSetup) // work. BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) { - auto connman = std::make_unique(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman); + ConnmanTestMsg& connman = static_cast(*m_node.connman); // Disable inactivity checks for this test to avoid interference - static_cast(connman.get())->SetPeerConnectTimeout(99999s); - auto peerLogic = PeerManager::make(*connman, *m_node.addrman, nullptr, - *m_node.chainman, *m_node.mempool, false); + connman.SetPeerConnectTimeout(99999s); + PeerManager& peerman = *m_node.peerman; // Mock an outbound peer CAddress addr1(ip(0xa0b0c001), NODE_NONE); @@ -63,10 +63,15 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) /*addrNameIn=*/"", ConnectionType::OUTBOUND_FULL_RELAY, /*inbound_onion=*/false}; - dummyNode1.SetCommonVersion(PROTOCOL_VERSION); - peerLogic->InitializeNode(dummyNode1, dummyNode1.GetLocalServices()); - dummyNode1.fSuccessfullyConnected = true; + connman.Handshake( + /*node=*/dummyNode1, + /*successfully_connected=*/true, + /*remote_services=*/ServiceFlags(NODE_NETWORK | NODE_WITNESS), + /*permission_flags=*/NetPermissionFlags::None, + /*version=*/PROTOCOL_VERSION, + /*relay_txs=*/true); + TestOnlyResetTimeData(); // This test requires that we have a chain with non-zero work. { @@ -78,7 +83,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) // Test starts here { LOCK(dummyNode1.cs_sendProcessing); - BOOST_CHECK(peerLogic->SendMessages(&dummyNode1)); // should result in getheaders + BOOST_CHECK(peerman.SendMessages(&dummyNode1)); // should result in getheaders } { LOCK(dummyNode1.cs_vSend); @@ -91,7 +96,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) SetMockTime(nStartTime+21*60); { LOCK(dummyNode1.cs_sendProcessing); - BOOST_CHECK(peerLogic->SendMessages(&dummyNode1)); // should result in getheaders + BOOST_CHECK(peerman.SendMessages(&dummyNode1)); // should result in getheaders } { LOCK(dummyNode1.cs_vSend); @@ -101,11 +106,11 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) SetMockTime(nStartTime+24*60); { LOCK(dummyNode1.cs_sendProcessing); - BOOST_CHECK(peerLogic->SendMessages(&dummyNode1)); // should result in disconnect + BOOST_CHECK(peerman.SendMessages(&dummyNode1)); // should result in disconnect } BOOST_CHECK(dummyNode1.fDisconnect == true); - peerLogic->FinalizeNode(dummyNode1); + peerman.FinalizeNode(dummyNode1); } static void AddRandomOutboundPeer(NodeId& id, std::vector& vNodes, PeerManager& peerLogic, ConnmanTestMsg& connman, ConnectionType connType) From f65e83d51bfb6a34f1d5efccfb3d8786a51a4534 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Mon, 20 Jul 2020 14:45:20 +0100 Subject: [PATCH 3/7] [net processing] Remove fClient and m_limited_node fClient is replaced by CanServeBlocks(), and m_limited_node is replaced by IsLimitedPeer(). --- src/net.h | 2 -- src/net_processing.cpp | 28 ++++++++++++++++++---------- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/src/net.h b/src/net.h index 00e8a780200..a986226d861 100644 --- a/src/net.h +++ b/src/net.h @@ -399,8 +399,6 @@ public: bool HasPermission(NetPermissionFlags permission) const { return NetPermissions::HasFlag(m_permissionFlags, permission); } - bool fClient{false}; // set by version message - bool m_limited_node{false}; //after BIP159, set by version message /** 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 diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 7c403d73db4..293ed36fb71 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -977,6 +977,20 @@ static void AddKnownTx(Peer& peer, const uint256& hash) tx_relay->m_tx_inventory_known_filter.insert(hash); } +/** Whether this peer can serve us blocks. */ +static bool CanServeBlocks(const Peer& peer) +{ + return peer.m_their_services & (NODE_NETWORK|NODE_NETWORK_LIMITED); +} + +/** Whether this peer can only serve limited recent blocks (e.g. because + * it prunes old blocks) */ +static bool IsLimitedPeer(const Peer& peer) +{ + return (!(peer.m_their_services & NODE_NETWORK) && + (peer.m_their_services & NODE_NETWORK_LIMITED)); +} + std::chrono::microseconds PeerManagerImpl::NextInvToInbounds(std::chrono::microseconds now, std::chrono::seconds average_interval) { @@ -2873,12 +2887,6 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, } peer->m_starting_height = starting_height; - // set nodes not relaying blocks and tx and not serving (parts) of the historical blockchain as "clients" - pfrom.fClient = (!(nServices & NODE_NETWORK) && !(nServices & NODE_NETWORK_LIMITED)); - - // set nodes not capable of serving the complete blockchain history as "limited nodes" - pfrom.m_limited_node = (!(nServices & NODE_NETWORK) && (nServices & NODE_NETWORK_LIMITED)); - // We only initialize the m_tx_relay data structure if: // - this isn't an outbound block-relay-only connection; and // - fRelay=true or we're offering NODE_BLOOM to this peer @@ -2903,7 +2911,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, { LOCK(cs_main); CNodeState* state = State(pfrom.GetId()); - state->fPreferredDownload = (!pfrom.IsInboundConn() || pfrom.HasPermission(NetPermissionFlags::NoBan)) && !pfrom.IsAddrFetchConn() && !pfrom.fClient; + state->fPreferredDownload = (!pfrom.IsInboundConn() || pfrom.HasPermission(NetPermissionFlags::NoBan)) && !pfrom.IsAddrFetchConn() && CanServeBlocks(*peer); m_num_preferred_download_peers += state->fPreferredDownload; } @@ -4863,7 +4871,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) bool sync_blocks_and_headers_from_peer = false; if (state.fPreferredDownload) { sync_blocks_and_headers_from_peer = true; - } else if (!pto->fClient && !pto->IsAddrFetchConn()) { + } else if (CanServeBlocks(*peer) && !pto->IsAddrFetchConn()) { // Typically this is an inbound peer. If we don't have any outbound // peers, or if we aren't downloading any blocks from such peers, // then allow block downloads from this peer, too. @@ -4878,7 +4886,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) } } - if (!state.fSyncStarted && !pto->fClient && !fImporting && !fReindex) { + if (!state.fSyncStarted && CanServeBlocks(*peer) && !fImporting && !fReindex) { // Only actively request headers from a single peer, unless we're close to today. if ((nSyncStarted == 0 && sync_blocks_and_headers_from_peer) || m_chainman.m_best_header->GetBlockTime() > GetAdjustedTime() - 24 * 60 * 60) { const CBlockIndex* pindexStart = m_chainman.m_best_header; @@ -5255,7 +5263,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) // Message: getdata (blocks) // std::vector vGetData; - if (!pto->fClient && ((sync_blocks_and_headers_from_peer && !pto->m_limited_node) || !m_chainman.ActiveChainstate().IsInitialBlockDownload()) && state.nBlocksInFlight < MAX_BLOCKS_IN_TRANSIT_PER_PEER) { + if (CanServeBlocks(*peer) && ((sync_blocks_and_headers_from_peer && !IsLimitedPeer(*peer)) || !m_chainman.ActiveChainstate().IsInitialBlockDownload()) && state.nBlocksInFlight < MAX_BLOCKS_IN_TRANSIT_PER_PEER) { std::vector vToDownload; NodeId staller = -1; FindNextBlocksToDownload(pto->GetId(), MAX_BLOCKS_IN_TRANSIT_PER_PEER - state.nBlocksInFlight, vToDownload, staller); From 7d1c0369340cb752f0d78e24f4251595534bf5e9 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Mon, 20 Jul 2020 13:06:03 -0400 Subject: [PATCH 4/7] [net processing] Replace fHaveWitness with CanServeWitnesses() --- src/net_processing.cpp | 69 +++++++++++++++++++++--------------------- 1 file changed, 35 insertions(+), 34 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 293ed36fb71..8889266a020 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -432,8 +432,6 @@ struct CNodeState { bool m_requested_hb_cmpctblocks{false}; /** Whether this peer will send us cmpctblocks if we request them. */ bool m_provides_cmpctblocks{false}; - //! Whether this peer can give us witnesses - bool fHaveWitness{false}; /** State used to enforce CHAIN_SYNC_TIMEOUT and EXTRA_PEER_CHECK_INTERVAL logic. * @@ -514,7 +512,8 @@ public: /** Implement PeerManager */ void StartScheduledTasks(CScheduler& scheduler) override; void CheckForStaleTipAndEvictPeers() override; - std::optional FetchBlock(NodeId peer_id, const CBlockIndex& block_index) override; + std::optional FetchBlock(NodeId peer_id, const CBlockIndex& block_index) override + EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); bool IgnoresIncomingTxs() override { return m_ignore_incoming_txs; } void SendPings() override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); @@ -600,7 +599,7 @@ private: */ bool MaybeSendGetHeaders(CNode& pfrom, const CBlockLocator& locator, Peer& peer); /** Potentially fetch blocks from this peer upon receipt of a new headers tip */ - void HeadersDirectFetchBlocks(CNode& pfrom, const CBlockIndex* pindexLast); + void HeadersDirectFetchBlocks(CNode& pfrom, const Peer& peer, const CBlockIndex* pindexLast); /** Update peer state based on received headers message */ void UpdatePeerStateForReceivedHeaders(CNode& pfrom, const CBlockIndex *pindexLast, bool received_new_header, bool may_have_more_headers); @@ -679,7 +678,7 @@ private: /** Get a pointer to a mutable CNodeState. */ CNodeState* State(NodeId pnode) EXCLUSIVE_LOCKS_REQUIRED(cs_main); - uint32_t GetFetchFlags(const CNode& pfrom) const EXCLUSIVE_LOCKS_REQUIRED(cs_main); + uint32_t GetFetchFlags(const Peer& peer) const; std::atomic m_next_inv_to_inbounds{0us}; @@ -800,7 +799,7 @@ private: /** Update pindexLastCommonBlock and add not-in-flight missing successors to vBlocks, until it has * at most count entries. */ - void FindNextBlocksToDownload(NodeId nodeid, unsigned int count, std::vector& vBlocks, NodeId& nodeStaller) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + void FindNextBlocksToDownload(const Peer& peer, unsigned int count, std::vector& vBlocks, NodeId& nodeStaller) EXCLUSIVE_LOCKS_REQUIRED(cs_main); std::map::iterator> > mapBlocksInFlight GUARDED_BY(cs_main); @@ -991,6 +990,12 @@ static bool IsLimitedPeer(const Peer& peer) (peer.m_their_services & NODE_NETWORK_LIMITED)); } +/** Whether this peer can serve us witness data */ +static bool CanServeWitnesses(const Peer& peer) +{ + return peer.m_their_services & NODE_WITNESS; +} + std::chrono::microseconds PeerManagerImpl::NextInvToInbounds(std::chrono::microseconds now, std::chrono::seconds average_interval) { @@ -1184,17 +1189,17 @@ void PeerManagerImpl::UpdateBlockAvailability(NodeId nodeid, const uint256 &hash } } -void PeerManagerImpl::FindNextBlocksToDownload(NodeId nodeid, unsigned int count, std::vector& vBlocks, NodeId& nodeStaller) +void PeerManagerImpl::FindNextBlocksToDownload(const Peer& peer, unsigned int count, std::vector& vBlocks, NodeId& nodeStaller) { if (count == 0) return; vBlocks.reserve(vBlocks.size() + count); - CNodeState *state = State(nodeid); + CNodeState *state = State(peer.m_id); assert(state != nullptr); // Make sure pindexBestKnownBlock is up to date, we'll need it. - ProcessBlockAvailability(nodeid); + ProcessBlockAvailability(peer.m_id); if (state->pindexBestKnownBlock == nullptr || state->pindexBestKnownBlock->nChainWork < m_chainman.ActiveChain().Tip()->nChainWork || state->pindexBestKnownBlock->nChainWork < nMinimumChainWork) { // This peer has nothing interesting. @@ -1242,7 +1247,7 @@ void PeerManagerImpl::FindNextBlocksToDownload(NodeId nodeid, unsigned int count // We consider the chain that this peer is on invalid. return; } - if (!State(nodeid)->fHaveWitness && DeploymentActiveAt(*pindex, m_chainman, Consensus::DEPLOYMENT_SEGWIT)) { + if (!CanServeWitnesses(peer) && DeploymentActiveAt(*pindex, m_chainman, Consensus::DEPLOYMENT_SEGWIT)) { // We wouldn't download this block or its descendants from this peer. return; } @@ -1253,7 +1258,7 @@ void PeerManagerImpl::FindNextBlocksToDownload(NodeId nodeid, unsigned int count // The block is not already downloaded, and not yet in flight. if (pindex->nHeight > nWindowEnd) { // We reached the end of the window. - if (vBlocks.size() == 0 && waitingfor != nodeid) { + if (vBlocks.size() == 0 && waitingfor != peer.m_id) { // We aren't able to fetch anything, but we would be if the download window was one larger. nodeStaller = waitingfor; } @@ -1621,12 +1626,14 @@ std::optional PeerManagerImpl::FetchBlock(NodeId peer_id, const CBl if (fImporting) return "Importing..."; if (fReindex) return "Reindexing..."; - LOCK(cs_main); // Ensure this peer exists and hasn't been disconnected - CNodeState* state = State(peer_id); - if (state == nullptr) return "Peer does not exist"; + PeerRef peer = GetPeerRef(peer_id); + if (peer == nullptr) return "Peer does not exist"; + // Ignore pre-segwit peers - if (!state->fHaveWitness) return "Pre-SegWit peer"; + if (!CanServeWitnesses(*peer)) return "Pre-SegWit peer"; + + LOCK(cs_main); // Mark block as in-flight unless it already is (for this peer). // If a block was already in-flight for a different peer, its BLOCKTXN @@ -2227,10 +2234,10 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic } } -uint32_t PeerManagerImpl::GetFetchFlags(const CNode& pfrom) const EXCLUSIVE_LOCKS_REQUIRED(cs_main) +uint32_t PeerManagerImpl::GetFetchFlags(const Peer& peer) const { uint32_t nFetchFlags = 0; - if (State(pfrom.GetId())->fHaveWitness) { + if (CanServeWitnesses(peer)) { nFetchFlags |= MSG_WITNESS_FLAG; } return nFetchFlags; @@ -2325,7 +2332,7 @@ bool PeerManagerImpl::MaybeSendGetHeaders(CNode& pfrom, const CBlockLocator& loc * We require that the given tip have at least as much work as our tip, and for * our current tip to be "close to synced" (see CanDirectFetch()). */ -void PeerManagerImpl::HeadersDirectFetchBlocks(CNode& pfrom, const CBlockIndex* pindexLast) +void PeerManagerImpl::HeadersDirectFetchBlocks(CNode& pfrom, const Peer& peer, const CBlockIndex* pindexLast) { const CNetMsgMaker msgMaker(pfrom.GetCommonVersion()); @@ -2340,7 +2347,7 @@ void PeerManagerImpl::HeadersDirectFetchBlocks(CNode& pfrom, const CBlockIndex* while (pindexWalk && !m_chainman.ActiveChain().Contains(pindexWalk) && vToFetch.size() <= MAX_BLOCKS_IN_TRANSIT_PER_PEER) { if (!(pindexWalk->nStatus & BLOCK_HAVE_DATA) && !IsBlockRequested(pindexWalk->GetBlockHash()) && - (!DeploymentActiveAt(*pindexWalk, m_chainman, Consensus::DEPLOYMENT_SEGWIT) || State(pfrom.GetId())->fHaveWitness)) { + (!DeploymentActiveAt(*pindexWalk, m_chainman, Consensus::DEPLOYMENT_SEGWIT) || CanServeWitnesses(peer))) { // We don't have this block, and it's not yet in flight. vToFetch.push_back(pindexWalk); } @@ -2362,7 +2369,7 @@ void PeerManagerImpl::HeadersDirectFetchBlocks(CNode& pfrom, const CBlockIndex* // Can't download any more from this peer break; } - uint32_t nFetchFlags = GetFetchFlags(pfrom); + uint32_t nFetchFlags = GetFetchFlags(peer); vGetData.push_back(CInv(MSG_BLOCK | nFetchFlags, pindex->GetBlockHash())); BlockRequested(pfrom.GetId(), *pindex); LogPrint(BCLog::NET, "Requesting block %s from peer=%d\n", @@ -2507,7 +2514,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer, UpdatePeerStateForReceivedHeaders(pfrom, pindexLast, received_new_header, nCount == MAX_HEADERS_RESULTS); // Consider immediately downloading blocks. - HeadersDirectFetchBlocks(pfrom, pindexLast); + HeadersDirectFetchBlocks(pfrom, peer, pindexLast); return; } @@ -2901,12 +2908,6 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, if (fRelay) pfrom.m_relays_txs = true; } - if((nServices & NODE_WITNESS)) - { - LOCK(cs_main); - State(pfrom.GetId())->fHaveWitness = true; - } - // Potentially mark this peer as a preferred download peer. { LOCK(cs_main); @@ -3789,7 +3790,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // We requested this block for some reason, but our mempool will probably be useless // so we just grab the block via normal getdata std::vector vInv(1); - vInv[0] = CInv(MSG_BLOCK | GetFetchFlags(pfrom), cmpctblock.header.GetHash()); + vInv[0] = CInv(MSG_BLOCK | GetFetchFlags(*peer), cmpctblock.header.GetHash()); m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, vInv)); } return; @@ -3825,7 +3826,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, } else if (status == READ_STATUS_FAILED) { // Duplicate txindexes, the block is now in-flight, so just request it std::vector vInv(1); - vInv[0] = CInv(MSG_BLOCK | GetFetchFlags(pfrom), cmpctblock.header.GetHash()); + vInv[0] = CInv(MSG_BLOCK | GetFetchFlags(*peer), cmpctblock.header.GetHash()); m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, vInv)); return; } @@ -3868,7 +3869,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // We requested this block, but its far into the future, so our // mempool will probably be useless - request the block normally std::vector vInv(1); - vInv[0] = CInv(MSG_BLOCK | GetFetchFlags(pfrom), cmpctblock.header.GetHash()); + vInv[0] = CInv(MSG_BLOCK | GetFetchFlags(*peer), cmpctblock.header.GetHash()); m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, vInv)); return; } else { @@ -3952,7 +3953,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, } else if (status == READ_STATUS_FAILED) { // Might have collided, fall back to getdata now :( std::vector invs; - invs.push_back(CInv(MSG_BLOCK | GetFetchFlags(pfrom), resp.blockhash)); + invs.push_back(CInv(MSG_BLOCK | GetFetchFlags(*peer), resp.blockhash)); m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, invs)); } else { // Block is either okay, or possibly we received @@ -5266,9 +5267,9 @@ bool PeerManagerImpl::SendMessages(CNode* pto) if (CanServeBlocks(*peer) && ((sync_blocks_and_headers_from_peer && !IsLimitedPeer(*peer)) || !m_chainman.ActiveChainstate().IsInitialBlockDownload()) && state.nBlocksInFlight < MAX_BLOCKS_IN_TRANSIT_PER_PEER) { std::vector vToDownload; NodeId staller = -1; - FindNextBlocksToDownload(pto->GetId(), MAX_BLOCKS_IN_TRANSIT_PER_PEER - state.nBlocksInFlight, vToDownload, staller); + FindNextBlocksToDownload(*peer, MAX_BLOCKS_IN_TRANSIT_PER_PEER - state.nBlocksInFlight, vToDownload, staller); for (const CBlockIndex *pindex : vToDownload) { - uint32_t nFetchFlags = GetFetchFlags(*pto); + uint32_t nFetchFlags = GetFetchFlags(*peer); vGetData.push_back(CInv(MSG_BLOCK | nFetchFlags, pindex->GetBlockHash())); BlockRequested(pto->GetId(), *pindex); LogPrint(BCLog::NET, "Requesting block %s (%d) peer=%d\n", pindex->GetBlockHash().ToString(), @@ -5295,7 +5296,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) if (!AlreadyHaveTx(gtxid)) { LogPrint(BCLog::NET, "Requesting %s %s peer=%d\n", gtxid.IsWtxid() ? "wtx" : "tx", gtxid.GetHash().ToString(), pto->GetId()); - vGetData.emplace_back(gtxid.IsWtxid() ? MSG_WTX : (MSG_TX | GetFetchFlags(*pto)), gtxid.GetHash()); + vGetData.emplace_back(gtxid.IsWtxid() ? MSG_WTX : (MSG_TX | GetFetchFlags(*peer)), gtxid.GetHash()); if (vGetData.size() >= MAX_GETDATA_SZ) { m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::GETDATA, vGetData)); vGetData.clear(); From d9079fe18dc5d81ce290876353555b51125127d1 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Mon, 20 Jul 2020 18:46:13 +0100 Subject: [PATCH 5/7] [net processing] Remove CNode::nServices Use Peer::m_their_services instead --- src/net.cpp | 3 +-- src/net.h | 5 +++-- src/net_processing.cpp | 7 +++---- src/net_processing.h | 1 + src/qt/rpcconsole.cpp | 2 +- src/rpc/net.cpp | 5 +++-- src/test/util/net.cpp | 2 +- 7 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 5082d6eb685..3244772ef60 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -603,7 +603,6 @@ Network CNode::ConnectedThroughNetwork() const void CNode::CopyStats(CNodeStats& stats) { stats.nodeid = this->GetId(); - X(nServices); X(addr); X(addrBind); stats.m_network = ConnectedThroughNetwork(); @@ -880,7 +879,7 @@ bool CConnman::AttemptToEvictConnection() .m_min_ping_time = node->m_min_ping_time, .m_last_block_time = node->m_last_block_time, .m_last_tx_time = node->m_last_tx_time, - .fRelevantServices = HasAllDesirableServiceFlags(node->nServices), + .fRelevantServices = node->m_has_all_wanted_services, .m_relay_txs = node->m_relays_txs.load(), .fBloomFilter = node->m_bloom_filter_loaded.load(), .nKeyedNetGroup = node->nKeyedNetGroup, diff --git a/src/net.h b/src/net.h index a986226d861..b9f69902751 100644 --- a/src/net.h +++ b/src/net.h @@ -187,7 +187,6 @@ class CNodeStats { public: NodeId nodeid; - ServiceFlags nServices; std::chrono::seconds m_last_send; std::chrono::seconds m_last_recv; std::chrono::seconds m_last_tx_time; @@ -346,7 +345,6 @@ public: std::unique_ptr m_serializer; NetPermissionFlags m_permissionFlags{NetPermissionFlags::None}; - std::atomic nServices{NODE_NONE}; /** * Socket used for communication with the node. @@ -482,6 +480,9 @@ public: // Peer selected us as (compact blocks) high-bandwidth peer (BIP152) std::atomic m_bip152_highbandwidth_from{false}; + /** Whether this peer provides all services that we want. Used for eviction decisions */ + std::atomic_bool m_has_all_wanted_services{false}; + /** Whether we should relay transactions to this peer (their version * message did not include fRelay=false and this is not a block-relay-only * connection). This only changes from false to true. It will never change diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 8889266a020..64ce34a7b85 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -223,9 +223,7 @@ struct Peer { * * TODO: remove redundant CNode::nLocalServices*/ const ServiceFlags m_our_services; - /** Services this peer offered to us. - * - * TODO: remove redundant CNode::nServices */ + /** Services this peer offered to us. */ std::atomic m_their_services{NODE_NONE}; /** Protects misbehavior data members */ @@ -1472,6 +1470,7 @@ bool PeerManagerImpl::GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) c PeerRef peer = GetPeerRef(nodeid); if (peer == nullptr) return false; + stats.their_services = peer->m_their_services; stats.m_starting_height = peer->m_starting_height; // It is common for nodes with good ping times to suddenly become lagged, // due to a new block arriving or other large transfer. @@ -2885,7 +2884,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::VERACK)); - pfrom.nServices = nServices; + pfrom.m_has_all_wanted_services = HasAllDesirableServiceFlags(nServices); peer->m_their_services = nServices; pfrom.SetAddrLocal(addrMe); { diff --git a/src/net_processing.h b/src/net_processing.h index 5fbae98c272..bcda9614d49 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -34,6 +34,7 @@ struct CNodeStateStats { uint64_t m_addr_processed = 0; uint64_t m_addr_rate_limited = 0; bool m_addr_relay_enabled{false}; + ServiceFlags their_services; }; class PeerManager : public CValidationInterface, public NetEventsInterface diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp index b791fd30c4e..70fccdef1cb 100644 --- a/src/qt/rpcconsole.cpp +++ b/src/qt/rpcconsole.cpp @@ -1162,7 +1162,6 @@ void RPCConsole::updateDetailWidget() if (!stats->nodeStats.addrLocal.empty()) peerAddrDetails += "
" + tr("via %1").arg(QString::fromStdString(stats->nodeStats.addrLocal)); ui->peerHeading->setText(peerAddrDetails); - ui->peerServices->setText(GUIUtil::formatServicesStr(stats->nodeStats.nServices)); QString bip152_hb_settings; if (stats->nodeStats.m_bip152_highbandwidth_to) bip152_hb_settings = ts.to; if (stats->nodeStats.m_bip152_highbandwidth_from) bip152_hb_settings += (bip152_hb_settings.isEmpty() ? ts.from : QLatin1Char('/') + ts.from); @@ -1197,6 +1196,7 @@ void RPCConsole::updateDetailWidget() // This check fails for example if the lock was busy and // nodeStateStats couldn't be fetched. if (stats->fNodeStateStatsAvailable) { + ui->peerServices->setText(GUIUtil::formatServicesStr(stats->nodeStateStats.their_services)); // Sync height is init to -1 if (stats->nodeStateStats.nSyncHeight > -1) { ui->peerSyncHeight->setText(QString("%1").arg(stats->nodeStateStats.nSyncHeight)); diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index fad92629c59..27eea824bcd 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -195,8 +195,9 @@ static RPCHelpMan getpeerinfo() if (stats.m_mapped_as != 0) { obj.pushKV("mapped_as", uint64_t(stats.m_mapped_as)); } - obj.pushKV("services", strprintf("%016x", stats.nServices)); - obj.pushKV("servicesnames", GetServicesNames(stats.nServices)); + ServiceFlags services{fStateStats ? statestats.their_services : ServiceFlags::NODE_NONE}; + obj.pushKV("services", strprintf("%016x", services)); + obj.pushKV("servicesnames", GetServicesNames(services)); obj.pushKV("lastsend", count_seconds(stats.m_last_send)); obj.pushKV("lastrecv", count_seconds(stats.m_last_recv)); obj.pushKV("last_transaction", count_seconds(stats.m_last_tx_time)); diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index 90a49d52d7c..ab4dcb93f74 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -51,10 +51,10 @@ void ConnmanTestMsg::Handshake(CNode& node, if (node.fDisconnect) return; assert(node.nVersion == version); assert(node.GetCommonVersion() == std::min(version, PROTOCOL_VERSION)); - assert(node.nServices == remote_services); CNodeStateStats statestats; assert(peerman.GetNodeStateStats(node.GetId(), statestats)); assert(statestats.m_relay_txs == (relay_txs && !node.IsBlockOnlyConn())); + assert(statestats.their_services == remote_services); node.m_permissionFlags = permission_flags; if (successfully_connected) { CSerializedNetMsg msg_verack{mm.Make(NetMsgType::VERACK)}; From 5961f8eea1ad5be1a4bf8da63651e197a20359b2 Mon Sep 17 00:00:00 2001 From: dergoegge Date: Mon, 4 Jul 2022 18:02:28 +0200 Subject: [PATCH 6/7] [net] Return CService from GetLocalAddrForPeer and GetLocalAddress --- src/net.cpp | 26 ++++++++++++-------------- src/net.h | 6 +++--- src/net_processing.cpp | 7 ++++--- src/test/net_tests.cpp | 13 +++++-------- 4 files changed, 24 insertions(+), 28 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 3244772ef60..41e85423d17 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -206,15 +206,13 @@ static std::vector ConvertSeeds(const std::vector &vSeedsIn) // Otherwise, return the unroutable 0.0.0.0 but filled in with // the normal parameters, since the IP may be changed to a useful // one by discovery. -CAddress GetLocalAddress(const CNetAddr *paddrPeer, ServiceFlags nLocalServices) +CService GetLocalAddress(const CNetAddr& addrPeer) { - CAddress ret(CService(CNetAddr(),GetListenPort()), nLocalServices); + CService ret{CNetAddr(), GetListenPort()}; CService addr; - if (GetLocal(addr, paddrPeer)) - { - ret = CAddress(addr, nLocalServices); + if (GetLocal(addr, &addrPeer)) { + ret = CService{addr}; } - ret.nTime = GetAdjustedTime(); return ret; } @@ -233,35 +231,35 @@ bool IsPeerAddrLocalGood(CNode *pnode) IsReachable(addrLocal.GetNetwork()); } -std::optional GetLocalAddrForPeer(CNode *pnode) +std::optional GetLocalAddrForPeer(CNode& node) { - CAddress addrLocal = GetLocalAddress(&pnode->addr, pnode->GetLocalServices()); + CService addrLocal{GetLocalAddress(node.addr)}; if (gArgs.GetBoolArg("-addrmantest", false)) { // use IPv4 loopback during addrmantest - addrLocal = CAddress(CService(LookupNumeric("127.0.0.1", GetListenPort())), pnode->GetLocalServices()); + addrLocal = CService(LookupNumeric("127.0.0.1", GetListenPort())); } // If discovery is enabled, sometimes give our peer the address it // tells us that it sees us as in case it has a better idea of our // address than we do. FastRandomContext rng; - if (IsPeerAddrLocalGood(pnode) && (!addrLocal.IsRoutable() || + if (IsPeerAddrLocalGood(&node) && (!addrLocal.IsRoutable() || rng.randbits((GetnScore(addrLocal) > LOCAL_MANUAL) ? 3 : 1) == 0)) { - if (pnode->IsInboundConn()) { + if (node.IsInboundConn()) { // For inbound connections, assume both the address and the port // as seen from the peer. - addrLocal = CAddress{pnode->GetAddrLocal(), addrLocal.nServices, addrLocal.nTime}; + addrLocal = CService{node.GetAddrLocal()}; } else { // For outbound connections, assume just the address as seen from // the peer and leave the port in `addrLocal` as returned by // `GetLocalAddress()` above. The peer has no way to observe our // listening port when we have initiated the connection. - addrLocal.SetIP(pnode->GetAddrLocal()); + addrLocal.SetIP(node.GetAddrLocal()); } } if (addrLocal.IsRoutable() || gArgs.GetBoolArg("-addrmantest", false)) { - LogPrint(BCLog::NET, "Advertising address %s to peer=%d\n", addrLocal.ToString(), pnode->GetId()); + LogPrint(BCLog::NET, "Advertising address %s to peer=%d\n", addrLocal.ToString(), node.GetId()); return addrLocal; } // Address is unroutable. Don't advertise. diff --git a/src/net.h b/src/net.h index b9f69902751..185fcd4c9af 100644 --- a/src/net.h +++ b/src/net.h @@ -144,8 +144,8 @@ enum }; bool IsPeerAddrLocalGood(CNode *pnode); -/** Returns a local address that we should advertise to this peer */ -std::optional GetLocalAddrForPeer(CNode *pnode); +/** Returns a local address that we should advertise to this peer. */ +std::optional GetLocalAddrForPeer(CNode& node); /** * Mark a network as reachable or unreachable (no automatic connects to it) @@ -163,7 +163,7 @@ void RemoveLocal(const CService& addr); bool SeenLocal(const CService& addr); bool IsLocal(const CService& addr); bool GetLocal(CService &addr, const CNetAddr *paddrPeer = nullptr); -CAddress GetLocalAddress(const CNetAddr *paddrPeer, ServiceFlags nLocalServices); +CService GetLocalAddress(const CNetAddr& addrPeer); extern bool fDiscover; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 64ce34a7b85..9d19b959865 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2930,7 +2930,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // indicate to the peer that we will participate in addr relay. if (fListen && !m_chainman.ActiveChainstate().IsInitialBlockDownload()) { - CAddress addr = GetLocalAddress(&pfrom.addr, pfrom.GetLocalServices()); + CAddress addr{GetLocalAddress(pfrom.addr), peer->m_our_services, (uint32_t)GetAdjustedTime()}; FastRandomContext insecure_rand; if (addr.IsRoutable()) { @@ -4685,9 +4685,10 @@ void PeerManagerImpl::MaybeSendAddr(CNode& node, Peer& peer, std::chrono::micros if (peer.m_next_local_addr_send != 0us) { peer.m_addr_known->reset(); } - if (std::optional local_addr = GetLocalAddrForPeer(&node)) { + if (std::optional local_service = GetLocalAddrForPeer(node)) { + CAddress local_addr{*local_service, peer.m_our_services, (uint32_t)GetAdjustedTime()}; FastRandomContext insecure_rand; - PushAddress(peer, *local_addr, insecure_rand); + PushAddress(peer, local_addr, insecure_rand); } peer.m_next_local_addr_send = GetExponentialRand(current_time, AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL); } diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index 0ac508bea95..a77b5212317 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -648,7 +648,7 @@ BOOST_AUTO_TEST_CASE(ipv4_peer_with_ipv6_addrMe_test) pnode->SetAddrLocal(addrLocal); // before patch, this causes undefined behavior detectable with clang's -fsanitize=memory - GetLocalAddrForPeer(&*pnode); + GetLocalAddrForPeer(*pnode); // suppress no-checks-run warning; if this test fails, it's by triggering a sanitizer BOOST_CHECK(1); @@ -675,13 +675,10 @@ BOOST_AUTO_TEST_CASE(get_local_addr_for_peer_port) const uint16_t bind_port = 20001; m_node.args->ForceSetArg("-bind", strprintf("3.4.5.6:%u", bind_port)); - const uint32_t current_time = static_cast(GetAdjustedTime()); - SetMockTime(current_time); - // Our address:port as seen from the peer, completely different from the above. in_addr peer_us_addr; peer_us_addr.s_addr = htonl(0x02030405); - const CAddress peer_us{CService{peer_us_addr, 20002}, NODE_NETWORK, current_time}; + const CService peer_us{peer_us_addr, 20002}; // Create a peer with a routable IPv4 address (outbound). in_addr peer_out_in_addr; @@ -700,9 +697,9 @@ BOOST_AUTO_TEST_CASE(get_local_addr_for_peer_port) peer_out.SetAddrLocal(peer_us); // Without the fix peer_us:8333 is chosen instead of the proper peer_us:bind_port. - auto chosen_local_addr = GetLocalAddrForPeer(&peer_out); + auto chosen_local_addr = GetLocalAddrForPeer(peer_out); BOOST_REQUIRE(chosen_local_addr); - const CAddress expected{CService{peer_us_addr, bind_port}, NODE_NETWORK, current_time}; + const CService expected{peer_us_addr, bind_port}; BOOST_CHECK(*chosen_local_addr == expected); // Create a peer with a routable IPv4 address (inbound). @@ -722,7 +719,7 @@ BOOST_AUTO_TEST_CASE(get_local_addr_for_peer_port) peer_in.SetAddrLocal(peer_us); // Without the fix peer_us:8333 is chosen instead of the proper peer_us:peer_us.GetPort(). - chosen_local_addr = GetLocalAddrForPeer(&peer_in); + chosen_local_addr = GetLocalAddrForPeer(peer_in); BOOST_REQUIRE(chosen_local_addr); BOOST_CHECK(*chosen_local_addr == peer_us); From 8d8eeb422e64ccd08bce92eed2ee9cbbf55ffd67 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Mon, 20 Jul 2020 20:28:37 +0100 Subject: [PATCH 7/7] [net processing] Remove CNode::nLocalServices --- src/net.cpp | 10 ++-- src/net.h | 23 +++------ src/net_processing.cpp | 83 +++++++++++++++--------------- src/test/denialofservice_tests.cpp | 17 +++--- src/test/fuzz/net.cpp | 1 - src/test/fuzz/util.cpp | 1 + src/test/fuzz/util.h | 3 -- src/test/net_tests.cpp | 10 +--- src/test/util/net.cpp | 3 +- src/test/util/net.h | 1 + 10 files changed, 65 insertions(+), 87 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 41e85423d17..2c4a1e4bae5 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -541,7 +541,6 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo addr_bind = GetBindAddress(*sock); } CNode* pnode = new CNode(id, - nLocalServices, std::move(sock), addrConnect, CalculateKeyedNetGroup(addrConnect), @@ -1011,7 +1010,6 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr&& sock, const bool inbound_onion = std::find(m_onion_binds.begin(), m_onion_binds.end(), addr_bind) != m_onion_binds.end(); CNode* pnode = new CNode(id, - nodeServices, std::move(sock), addr, CalculateKeyedNetGroup(addr), @@ -2705,7 +2703,10 @@ ServiceFlags CConnman::GetLocalServices() const unsigned int CConnman::GetReceiveFloodSize() const { return nReceiveFloodSize; } -CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, std::shared_ptr sock, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress& addrBindIn, const std::string& addrNameIn, ConnectionType conn_type_in, bool inbound_onion) +CNode::CNode(NodeId idIn, std::shared_ptr sock, const CAddress& addrIn, + uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, + const CAddress& addrBindIn, const std::string& addrNameIn, + ConnectionType conn_type_in, bool inbound_onion) : m_sock{sock}, m_connected{GetTime()}, addr(addrIn), @@ -2715,8 +2716,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, std::shared_ptr s nKeyedNetGroup(nKeyedNetGroupIn), id(idIn), nLocalHostNonce(nLocalHostNonceIn), - m_conn_type(conn_type_in), - nLocalServices(nLocalServicesIn) + m_conn_type(conn_type_in) { if (inbound_onion) assert(conn_type_in == ConnectionType::INBOUND); diff --git a/src/net.h b/src/net.h index 185fcd4c9af..f3be7e8dff1 100644 --- a/src/net.h +++ b/src/net.h @@ -513,7 +513,10 @@ public: * criterium in CConnman::AttemptToEvictConnection. */ std::atomic m_min_ping_time{std::chrono::microseconds::max()}; - CNode(NodeId id, ServiceFlags nLocalServicesIn, std::shared_ptr sock, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress& addrBindIn, const std::string& addrNameIn, ConnectionType conn_type_in, bool inbound_onion); + CNode(NodeId id, std::shared_ptr sock, const CAddress& addrIn, + uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, + const CAddress& addrBindIn, const std::string& addrNameIn, + ConnectionType conn_type_in, bool inbound_onion); CNode(const CNode&) = delete; CNode& operator=(const CNode&) = delete; @@ -571,11 +574,6 @@ public: void CopyStats(CNodeStats& stats) EXCLUSIVE_LOCKS_REQUIRED(!m_subver_mutex, !m_addr_local_mutex, !cs_vSend, !cs_vRecv); - ServiceFlags GetLocalServices() const - { - return nLocalServices; - } - std::string ConnectionTypeAsString() const { return ::ConnectionTypeAsString(m_conn_type); } /** A ping-pong round trip has completed successfully. Update latest and minimum ping times. */ @@ -590,9 +588,6 @@ private: const ConnectionType m_conn_type; std::atomic m_greatest_common_version{INIT_PROTO_VERSION}; - //! Services offered to this peer. - const ServiceFlags nLocalServices; - std::list vRecvMsg; // Used only by SocketHandler thread // Our address, as reported by the peer @@ -1020,16 +1015,14 @@ private: std::map m_addr_response_caches; /** - * Services this instance offers. + * Services this node offers. * - * This data is replicated in each CNode instance we create during peer - * connection (in ConnectNode()) under a member also called - * nLocalServices. + * This data is replicated in each Peer instance we create. * * This data is not marked const, but after being set it should not - * change. See the note in CNode::nLocalServices documentation. + * change. * - * \sa CNode::nLocalServices + * \sa Peer::our_services */ ServiceFlags nLocalServices; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 9d19b959865..1ea082eb260 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -219,9 +219,7 @@ struct Peer { * to serve, but still advertises NODE_NETWORK because it will eventually * fulfill this role after IBD completes. P2P code is written in such a * way that it can gracefully handle peers who don't make good on their - * service advertisements. - * - * TODO: remove redundant CNode::nLocalServices*/ + * service advertisements. */ const ServiceFlags m_our_services; /** Services this peer offered to us. */ std::atomic m_their_services{NODE_NONE}; @@ -867,6 +865,7 @@ private: * * May disconnect from the peer in the case of a bad request. * + * @param[in] node The node that we received the request from * @param[in] peer The peer that we received the request from * @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 @@ -876,7 +875,7 @@ private: * @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, + bool PrepareBlockFilterRequest(CNode& node, Peer& peer, BlockFilterType filter_type, uint32_t start_height, const uint256& stop_hash, uint32_t max_height_diff, const CBlockIndex*& stop_index, @@ -887,30 +886,33 @@ private: * * May disconnect from the peer in the case of a bad request. * + * @param[in] node The node that we received the request from * @param[in] peer The peer that we received the request from * @param[in] vRecv The raw message received */ - void ProcessGetCFilters(CNode& peer, CDataStream& vRecv); + void ProcessGetCFilters(CNode& node, Peer& peer, CDataStream& vRecv); /** * Handle a cfheaders request. * * May disconnect from the peer in the case of a bad request. * + * @param[in] node The node that we received the request from * @param[in] peer The peer that we received the request from * @param[in] vRecv The raw message received */ - void ProcessGetCFHeaders(CNode& peer, CDataStream& vRecv); + void ProcessGetCFHeaders(CNode& node, Peer& peer, CDataStream& vRecv); /** * Handle a getcfcheckpt request. * * May disconnect from the peer in the case of a bad request. * + * @param[in] node The node that we received the request from * @param[in] peer The peer that we received the request from * @param[in] vRecv The raw message received */ - void ProcessGetCFCheckPt(CNode& peer, CDataStream& vRecv); + void ProcessGetCFCheckPt(CNode& node, Peer& peer, CDataStream& vRecv); /** Checks if address relay is permitted with peer. If needed, initializes * the m_addr_known bloom filter and sets m_addr_relay_enabled to true. @@ -1278,10 +1280,7 @@ void PeerManagerImpl::FindNextBlocksToDownload(const Peer& peer, unsigned int co void PeerManagerImpl::PushNodeVersion(CNode& pnode, const Peer& peer) { - // Note that pnode->GetLocalServices() is a reflection of the local - // services we were offering when the CNode object was created for this - // peer. - uint64_t my_services{pnode.GetLocalServices()}; + uint64_t my_services{peer.m_our_services}; const int64_t nTime{count_seconds(GetTime())}; uint64_t nonce = pnode.GetLocalNonce(); const int nNodeStartingHeight{m_best_height}; @@ -2016,7 +2015,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& } // Avoid leaking prune-height by never sending blocks below the NODE_NETWORK_LIMITED threshold if (!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 */) ) + (((peer.m_our_services & NODE_NETWORK_LIMITED) == NODE_NETWORK_LIMITED) && ((peer.m_our_services & 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, disconnect peer=%d\n", pfrom.GetId()); //disconnect node and prevent it from stalling (would otherwise wait for the missing block) @@ -2597,7 +2596,7 @@ void PeerManagerImpl::ProcessOrphanTx(std::set& orphan_work_set) } } -bool PeerManagerImpl::PrepareBlockFilterRequest(CNode& peer, +bool PeerManagerImpl::PrepareBlockFilterRequest(CNode& node, Peer& peer, BlockFilterType filter_type, uint32_t start_height, const uint256& stop_hash, uint32_t max_height_diff, const CBlockIndex*& stop_index, @@ -2605,11 +2604,11 @@ bool PeerManagerImpl::PrepareBlockFilterRequest(CNode& peer, { const bool supported_filter_type = (filter_type == BlockFilterType::BASIC && - (peer.GetLocalServices() & NODE_COMPACT_FILTERS)); + (peer.m_our_services & NODE_COMPACT_FILTERS)); if (!supported_filter_type) { LogPrint(BCLog::NET, "peer %d requested unsupported block filter type: %d\n", - peer.GetId(), static_cast(filter_type)); - peer.fDisconnect = true; + node.GetId(), static_cast(filter_type)); + node.fDisconnect = true; return false; } @@ -2620,8 +2619,8 @@ bool PeerManagerImpl::PrepareBlockFilterRequest(CNode& peer, // Check that the stop block exists and the peer would be allowed to fetch it. 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; + node.GetId(), stop_hash.ToString()); + node.fDisconnect = true; return false; } } @@ -2630,14 +2629,14 @@ bool PeerManagerImpl::PrepareBlockFilterRequest(CNode& peer, if (start_height > stop_height) { LogPrint(BCLog::NET, "peer %d sent invalid getcfilters/getcfheaders with " /* Continued */ "start height %d and stop height %d\n", - peer.GetId(), start_height, stop_height); - peer.fDisconnect = true; + node.GetId(), start_height, stop_height); + node.fDisconnect = true; return false; } if (stop_height - start_height >= max_height_diff) { LogPrint(BCLog::NET, "peer %d requested too many cfilters/cfheaders: %d / %d\n", - peer.GetId(), stop_height - start_height + 1, max_height_diff); - peer.fDisconnect = true; + node.GetId(), stop_height - start_height + 1, max_height_diff); + node.fDisconnect = true; return false; } @@ -2650,7 +2649,7 @@ bool PeerManagerImpl::PrepareBlockFilterRequest(CNode& peer, return true; } -void PeerManagerImpl::ProcessGetCFilters(CNode& peer, CDataStream& vRecv) +void PeerManagerImpl::ProcessGetCFilters(CNode& node,Peer& peer, CDataStream& vRecv) { uint8_t filter_type_ser; uint32_t start_height; @@ -2662,7 +2661,7 @@ void PeerManagerImpl::ProcessGetCFilters(CNode& peer, CDataStream& vRecv) const CBlockIndex* stop_index; BlockFilterIndex* filter_index; - if (!PrepareBlockFilterRequest(peer, filter_type, start_height, stop_hash, + if (!PrepareBlockFilterRequest(node, peer, filter_type, start_height, stop_hash, MAX_GETCFILTERS_SIZE, stop_index, filter_index)) { return; } @@ -2675,13 +2674,13 @@ void PeerManagerImpl::ProcessGetCFilters(CNode& peer, CDataStream& vRecv) } for (const auto& filter : filters) { - CSerializedNetMsg msg = CNetMsgMaker(peer.GetCommonVersion()) + CSerializedNetMsg msg = CNetMsgMaker(node.GetCommonVersion()) .Make(NetMsgType::CFILTER, filter); - m_connman.PushMessage(&peer, std::move(msg)); + m_connman.PushMessage(&node, std::move(msg)); } } -void PeerManagerImpl::ProcessGetCFHeaders(CNode& peer, CDataStream& vRecv) +void PeerManagerImpl::ProcessGetCFHeaders(CNode& node, Peer& peer, CDataStream& vRecv) { uint8_t filter_type_ser; uint32_t start_height; @@ -2693,7 +2692,7 @@ void PeerManagerImpl::ProcessGetCFHeaders(CNode& peer, CDataStream& vRecv) const CBlockIndex* stop_index; BlockFilterIndex* filter_index; - if (!PrepareBlockFilterRequest(peer, filter_type, start_height, stop_hash, + if (!PrepareBlockFilterRequest(node, peer, filter_type, start_height, stop_hash, MAX_GETCFHEADERS_SIZE, stop_index, filter_index)) { return; } @@ -2716,16 +2715,16 @@ void PeerManagerImpl::ProcessGetCFHeaders(CNode& peer, CDataStream& vRecv) return; } - CSerializedNetMsg msg = CNetMsgMaker(peer.GetCommonVersion()) + CSerializedNetMsg msg = CNetMsgMaker(node.GetCommonVersion()) .Make(NetMsgType::CFHEADERS, filter_type_ser, stop_index->GetBlockHash(), prev_header, filter_hashes); - m_connman.PushMessage(&peer, std::move(msg)); + m_connman.PushMessage(&node, std::move(msg)); } -void PeerManagerImpl::ProcessGetCFCheckPt(CNode& peer, CDataStream& vRecv) +void PeerManagerImpl::ProcessGetCFCheckPt(CNode& node, Peer& peer, CDataStream& vRecv) { uint8_t filter_type_ser; uint256 stop_hash; @@ -2736,7 +2735,7 @@ void PeerManagerImpl::ProcessGetCFCheckPt(CNode& peer, CDataStream& vRecv) const CBlockIndex* stop_index; BlockFilterIndex* filter_index; - if (!PrepareBlockFilterRequest(peer, filter_type, /*start_height=*/0, stop_hash, + if (!PrepareBlockFilterRequest(node, peer, filter_type, /*start_height=*/0, stop_hash, /*max_height_diff=*/std::numeric_limits::max(), stop_index, filter_index)) { return; @@ -2757,12 +2756,12 @@ void PeerManagerImpl::ProcessGetCFCheckPt(CNode& peer, CDataStream& vRecv) } } - CSerializedNetMsg msg = CNetMsgMaker(peer.GetCommonVersion()) + CSerializedNetMsg msg = CNetMsgMaker(node.GetCommonVersion()) .Make(NetMsgType::CFCHECKPT, filter_type_ser, stop_index->GetBlockHash(), headers); - m_connman.PushMessage(&peer, std::move(msg)); + m_connman.PushMessage(&node, std::move(msg)); } void PeerManagerImpl::ProcessBlock(CNode& node, const std::shared_ptr& block, bool force_processing) @@ -2898,7 +2897,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // - fRelay=true or we're offering NODE_BLOOM to this peer // (NODE_BLOOM means that the peer may turn on tx relay later) if (!pfrom.IsBlockOnlyConn() && - (fRelay || (pfrom.GetLocalServices() & NODE_BLOOM))) { + (fRelay || (peer->m_our_services & NODE_BLOOM))) { auto* const tx_relay = peer->SetTxRelay(); { LOCK(tx_relay->m_bloom_filter_mutex); @@ -4092,7 +4091,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, } if (msg_type == NetMsgType::MEMPOOL) { - if (!(pfrom.GetLocalServices() & NODE_BLOOM) && !pfrom.HasPermission(NetPermissionFlags::Mempool)) + if (!(peer->m_our_services & NODE_BLOOM) && !pfrom.HasPermission(NetPermissionFlags::Mempool)) { if (!pfrom.HasPermission(NetPermissionFlags::NoBan)) { @@ -4195,7 +4194,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, } if (msg_type == NetMsgType::FILTERLOAD) { - if (!(pfrom.GetLocalServices() & NODE_BLOOM)) { + if (!(peer->m_our_services & NODE_BLOOM)) { LogPrint(BCLog::NET, "filterload received despite not offering bloom services from peer=%d; disconnecting\n", pfrom.GetId()); pfrom.fDisconnect = true; return; @@ -4220,7 +4219,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, } if (msg_type == NetMsgType::FILTERADD) { - if (!(pfrom.GetLocalServices() & NODE_BLOOM)) { + if (!(peer->m_our_services & NODE_BLOOM)) { LogPrint(BCLog::NET, "filteradd received despite not offering bloom services from peer=%d; disconnecting\n", pfrom.GetId()); pfrom.fDisconnect = true; return; @@ -4248,7 +4247,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, } if (msg_type == NetMsgType::FILTERCLEAR) { - if (!(pfrom.GetLocalServices() & NODE_BLOOM)) { + if (!(peer->m_our_services & NODE_BLOOM)) { LogPrint(BCLog::NET, "filterclear received despite not offering bloom services from peer=%d; disconnecting\n", pfrom.GetId()); pfrom.fDisconnect = true; return; @@ -4279,17 +4278,17 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, } if (msg_type == NetMsgType::GETCFILTERS) { - ProcessGetCFilters(pfrom, vRecv); + ProcessGetCFilters(pfrom, *peer, vRecv); return; } if (msg_type == NetMsgType::GETCFHEADERS) { - ProcessGetCFHeaders(pfrom, vRecv); + ProcessGetCFHeaders(pfrom, *peer, vRecv); return; } if (msg_type == NetMsgType::GETCFCHECKPT) { - ProcessGetCFCheckPt(pfrom, vRecv); + ProcessGetCFCheckPt(pfrom, *peer, vRecv); return; } diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index f34ddf50abe..66c4605afaf 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -54,7 +54,6 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) CAddress addr1(ip(0xa0b0c001), NODE_NONE); NodeId id{0}; CNode dummyNode1{id++, - ServiceFlags(NODE_NETWORK | NODE_WITNESS), /*sock=*/nullptr, addr1, /*nKeyedNetGroupIn=*/0, @@ -68,6 +67,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) /*node=*/dummyNode1, /*successfully_connected=*/true, /*remote_services=*/ServiceFlags(NODE_NETWORK | NODE_WITNESS), + /*local_services=*/ServiceFlags(NODE_NETWORK | NODE_WITNESS), /*permission_flags=*/NetPermissionFlags::None, /*version=*/PROTOCOL_VERSION, /*relay_txs=*/true); @@ -117,7 +117,6 @@ static void AddRandomOutboundPeer(NodeId& id, std::vector& vNodes, PeerM { CAddress addr(ip(g_insecure_rand_ctx.randbits(32)), NODE_NONE); vNodes.emplace_back(new CNode{id++, - ServiceFlags(NODE_NETWORK | NODE_WITNESS), /*sock=*/nullptr, addr, /*nKeyedNetGroupIn=*/0, @@ -129,7 +128,7 @@ static void AddRandomOutboundPeer(NodeId& id, std::vector& vNodes, PeerM CNode &node = *vNodes.back(); node.SetCommonVersion(PROTOCOL_VERSION); - peerLogic.InitializeNode(node, node.GetLocalServices()); + peerLogic.InitializeNode(node, ServiceFlags(NODE_NETWORK | NODE_WITNESS)); node.fSuccessfullyConnected = true; connman.AddTestNode(node); @@ -297,7 +296,6 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) banman->ClearBanned(); NodeId id{0}; nodes[0] = new CNode{id++, - NODE_NETWORK, /*sock=*/nullptr, addr[0], /*nKeyedNetGroupIn=*/0, @@ -307,7 +305,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) ConnectionType::INBOUND, /*inbound_onion=*/false}; nodes[0]->SetCommonVersion(PROTOCOL_VERSION); - peerLogic->InitializeNode(*nodes[0], nodes[0]->GetLocalServices()); + peerLogic->InitializeNode(*nodes[0], NODE_NETWORK); nodes[0]->fSuccessfullyConnected = true; connman->AddTestNode(*nodes[0]); peerLogic->UnitTestMisbehaving(nodes[0]->GetId(), DISCOURAGEMENT_THRESHOLD); // Should be discouraged @@ -320,7 +318,6 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) BOOST_CHECK(!banman->IsDiscouraged(other_addr)); // Different address, not discouraged nodes[1] = new CNode{id++, - NODE_NETWORK, /*sock=*/nullptr, addr[1], /*nKeyedNetGroupIn=*/1, @@ -330,7 +327,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) ConnectionType::INBOUND, /*inbound_onion=*/false}; nodes[1]->SetCommonVersion(PROTOCOL_VERSION); - peerLogic->InitializeNode(*nodes[1], nodes[1]->GetLocalServices()); + peerLogic->InitializeNode(*nodes[1], NODE_NETWORK); nodes[1]->fSuccessfullyConnected = true; connman->AddTestNode(*nodes[1]); peerLogic->UnitTestMisbehaving(nodes[1]->GetId(), DISCOURAGEMENT_THRESHOLD - 1); @@ -358,7 +355,6 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) // Make sure non-IP peers are discouraged and disconnected properly. nodes[2] = new CNode{id++, - NODE_NETWORK, /*sock=*/nullptr, addr[2], /*nKeyedNetGroupIn=*/1, @@ -368,7 +364,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) ConnectionType::OUTBOUND_FULL_RELAY, /*inbound_onion=*/false}; nodes[2]->SetCommonVersion(PROTOCOL_VERSION); - peerLogic->InitializeNode(*nodes[2], nodes[2]->GetLocalServices()); + peerLogic->InitializeNode(*nodes[2], NODE_NETWORK); nodes[2]->fSuccessfullyConnected = true; connman->AddTestNode(*nodes[2]); peerLogic->UnitTestMisbehaving(nodes[2]->GetId(), DISCOURAGEMENT_THRESHOLD); @@ -403,7 +399,6 @@ BOOST_AUTO_TEST_CASE(DoS_bantime) CAddress addr(ip(0xa0b0c001), NODE_NONE); NodeId id{0}; CNode dummyNode{id++, - NODE_NETWORK, /*sock=*/nullptr, addr, /*nKeyedNetGroupIn=*/4, @@ -413,7 +408,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime) ConnectionType::INBOUND, /*inbound_onion=*/false}; dummyNode.SetCommonVersion(PROTOCOL_VERSION); - peerLogic->InitializeNode(dummyNode, dummyNode.GetLocalServices()); + peerLogic->InitializeNode(dummyNode, NODE_NETWORK); dummyNode.fSuccessfullyConnected = true; peerLogic->UnitTestMisbehaving(dummyNode.GetId(), DISCOURAGEMENT_THRESHOLD); diff --git a/src/test/fuzz/net.cpp b/src/test/fuzz/net.cpp index 49812871525..741810f6a20 100644 --- a/src/test/fuzz/net.cpp +++ b/src/test/fuzz/net.cpp @@ -68,7 +68,6 @@ FUZZ_TARGET_INIT(net, initialize_net) (void)node.GetAddrLocal(); (void)node.GetId(); (void)node.GetLocalNonce(); - (void)node.GetLocalServices(); const int ref_count = node.GetRefCount(); assert(ref_count >= 0); (void)node.GetCommonVersion(); diff --git a/src/test/fuzz/util.cpp b/src/test/fuzz/util.cpp index f0cff74f940..fabcea22c34 100644 --- a/src/test/fuzz/util.cpp +++ b/src/test/fuzz/util.cpp @@ -294,6 +294,7 @@ void FillNode(FuzzedDataProvider& fuzzed_data_provider, ConnmanTestMsg& connman, connman.Handshake(node, /*successfully_connected=*/fuzzed_data_provider.ConsumeBool(), /*remote_services=*/ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS), + /*local_services=*/ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS), /*permission_flags=*/ConsumeWeakEnum(fuzzed_data_provider, ALL_NET_PERMISSION_FLAGS), /*version=*/fuzzed_data_provider.ConsumeIntegralInRange(MIN_PEER_PROTO_VERSION, std::numeric_limits::max()), /*relay_txs=*/fuzzed_data_provider.ConsumeBool()); diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h index d189a50a51b..406e11c573e 100644 --- a/src/test/fuzz/util.h +++ b/src/test/fuzz/util.h @@ -296,7 +296,6 @@ template auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional& node_id_in = std::nullopt) noexcept { const NodeId node_id = node_id_in.value_or(fuzzed_data_provider.ConsumeIntegralInRange(0, std::numeric_limits::max())); - const ServiceFlags local_services = ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS); const auto sock = std::make_shared(fuzzed_data_provider); const CAddress address = ConsumeAddress(fuzzed_data_provider); const uint64_t keyed_net_group = fuzzed_data_provider.ConsumeIntegral(); @@ -307,7 +306,6 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional(node_id, - local_services, sock, address, keyed_net_group, @@ -318,7 +316,6 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional pnode1 = std::make_unique(id++, - NODE_NETWORK, /*sock=*/nullptr, addr, /*nKeyedNetGroupIn=*/0, @@ -77,7 +76,6 @@ BOOST_AUTO_TEST_CASE(cnode_simple_test) BOOST_CHECK_EQUAL(pnode1->ConnectedThroughNetwork(), Network::NET_IPV4); std::unique_ptr pnode2 = std::make_unique(id++, - NODE_NETWORK, /*sock=*/nullptr, addr, /*nKeyedNetGroupIn=*/1, @@ -96,7 +94,6 @@ BOOST_AUTO_TEST_CASE(cnode_simple_test) BOOST_CHECK_EQUAL(pnode2->ConnectedThroughNetwork(), Network::NET_IPV4); std::unique_ptr pnode3 = std::make_unique(id++, - NODE_NETWORK, /*sock=*/nullptr, addr, /*nKeyedNetGroupIn=*/0, @@ -115,7 +112,6 @@ BOOST_AUTO_TEST_CASE(cnode_simple_test) BOOST_CHECK_EQUAL(pnode3->ConnectedThroughNetwork(), Network::NET_IPV4); std::unique_ptr pnode4 = std::make_unique(id++, - NODE_NETWORK, /*sock=*/nullptr, addr, /*nKeyedNetGroupIn=*/1, @@ -629,7 +625,6 @@ BOOST_AUTO_TEST_CASE(ipv4_peer_with_ipv6_addrMe_test) ipv4AddrPeer.s_addr = 0xa0b0c001; CAddress addr = CAddress(CService(ipv4AddrPeer, 7777), NODE_NETWORK); std::unique_ptr pnode = std::make_unique(/*id=*/0, - NODE_NETWORK, /*sock=*/nullptr, addr, /*nKeyedNetGroupIn=*/0, @@ -684,7 +679,6 @@ BOOST_AUTO_TEST_CASE(get_local_addr_for_peer_port) in_addr peer_out_in_addr; peer_out_in_addr.s_addr = htonl(0x01020304); CNode peer_out{/*id=*/0, - /*nLocalServicesIn=*/NODE_NETWORK, /*sock=*/nullptr, /*addrIn=*/CAddress{CService{peer_out_in_addr, 8333}, NODE_NETWORK}, /*nKeyedNetGroupIn=*/0, @@ -706,7 +700,6 @@ BOOST_AUTO_TEST_CASE(get_local_addr_for_peer_port) in_addr peer_in_in_addr; peer_in_in_addr.s_addr = htonl(0x05060708); CNode peer_in{/*id=*/0, - /*nLocalServicesIn=*/NODE_NETWORK, /*sock=*/nullptr, /*addrIn=*/CAddress{CService{peer_in_in_addr, 8333}, NODE_NETWORK}, /*nKeyedNetGroupIn=*/0, @@ -834,7 +827,6 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message) in_addr peer_in_addr; peer_in_addr.s_addr = htonl(0x01020304); CNode peer{/*id=*/0, - /*nLocalServicesIn=*/NODE_NETWORK, /*sock=*/nullptr, /*addrIn=*/CAddress{CService{peer_in_addr, 8333}, NODE_NETWORK}, /*nKeyedNetGroupIn=*/0, @@ -854,7 +846,7 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message) *static_cast(&m_node.chainman->ActiveChainstate()); chainstate.JumpOutOfIbd(); - m_node.peerman->InitializeNode(peer, peer.GetLocalServices()); + m_node.peerman->InitializeNode(peer, NODE_NETWORK); std::atomic interrupt_dummy{false}; std::chrono::microseconds time_received_dummy{0}; diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index ab4dcb93f74..223db16c6c1 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -16,6 +16,7 @@ void ConnmanTestMsg::Handshake(CNode& node, bool successfully_connected, ServiceFlags remote_services, + ServiceFlags local_services, NetPermissionFlags permission_flags, int32_t version, bool relay_txs) @@ -24,7 +25,7 @@ void ConnmanTestMsg::Handshake(CNode& node, auto& connman{*this}; const CNetMsgMaker mm{0}; - peerman.InitializeNode(node, node.GetLocalServices()); + peerman.InitializeNode(node, local_services); CSerializedNetMsg msg_version{ mm.Make(NetMsgType::VERSION, diff --git a/src/test/util/net.h b/src/test/util/net.h index 0cf55f8a22d..7f61a03d279 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -42,6 +42,7 @@ struct ConnmanTestMsg : public CConnman { void Handshake(CNode& node, bool successfully_connected, ServiceFlags remote_services, + ServiceFlags local_services, NetPermissionFlags permission_flags, int32_t version, bool relay_txs);