From 3f1b7140e95d0f8f958cb35f31c3d964c57e484d Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Fri, 17 Jul 2020 14:56:34 -0700 Subject: [PATCH 01/19] scripted-diff: Rename OneShot to AddrFetch -BEGIN VERIFY SCRIPT- sed -i 's/a oneshot/an addrfetch/g' src/chainparams.cpp #comment sed -i 's/oneshot/addrfetch/g' src/net.cpp #comment sed -i 's/AddOneShot/AddAddrFetch/g' src/net.h src/net.cpp sed -i 's/cs_vOneShots/m_addr_fetches_mutex/g' src/net.h src/net.cpp sed -i 's/vOneShots/m_addr_fetches/g' src/net.h src/net.cpp sed -i 's/fOneShot/m_addr_fetch/g' src/net.h src/net.cpp src/net_processing.cpp sed -i 's/ProcessOneShot/ProcessAddrFetch/g' src/net.h src/net.cpp -END VERIFY SCRIPT- --- src/chainparams.cpp | 2 +- src/net.cpp | 38 +++++++++++++++++++------------------- src/net.h | 12 ++++++------ src/net_processing.cpp | 8 ++++---- 4 files changed, 30 insertions(+), 30 deletions(-) diff --git a/src/chainparams.cpp b/src/chainparams.cpp index 092c45e4c..5447c0ece 100644 --- a/src/chainparams.cpp +++ b/src/chainparams.cpp @@ -110,7 +110,7 @@ public: // Note that of those which support the service bits prefix, most only support a subset of // possible options. - // This is fine at runtime as we'll fall back to using them as a oneshot if they don't support the + // This is fine at runtime as we'll fall back to using them as an addrfetch if they don't support the // service bits we want, but we should get them updated to support all service bits wanted by any // release ASAP to avoid it where possible. vSeeds.emplace_back("seed.bitcoin.sipa.be"); // Pieter Wuille, only supports x1, x5, x9, and xd diff --git a/src/net.cpp b/src/net.cpp index 0c56cddbd..683a89ad1 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -105,10 +105,10 @@ std::map mapLocalHost GUARDED_BY(cs_mapLocalHost); static bool vfLimited[NET_MAX] GUARDED_BY(cs_mapLocalHost) = {}; std::string strSubVersion; -void CConnman::AddOneShot(const std::string& strDest) +void CConnman::AddAddrFetch(const std::string& strDest) { - LOCK(cs_vOneShots); - vOneShots.push_back(strDest); + LOCK(m_addr_fetches_mutex); + m_addr_fetches.push_back(strDest); } uint16_t GetListenPort() @@ -1646,7 +1646,7 @@ void CConnman::ThreadDNSAddressSeed() { LOCK(cs_vNodes); for (const CNode* pnode : vNodes) { - nRelevant += pnode->fSuccessfullyConnected && !pnode->fFeeler && !pnode->fOneShot && !pnode->m_manual_connection && !pnode->fInbound; + nRelevant += pnode->fSuccessfullyConnected && !pnode->fFeeler && !pnode->m_addr_fetch && !pnode->m_manual_connection && !pnode->fInbound; } } if (nRelevant >= 2) { @@ -1674,7 +1674,7 @@ void CConnman::ThreadDNSAddressSeed() LogPrintf("Loading addresses from DNS seed %s\n", seed); if (HaveNameProxy()) { - AddOneShot(seed); + AddAddrFetch(seed); } else { std::vector vIPs; std::vector vAdd; @@ -1696,8 +1696,8 @@ void CConnman::ThreadDNSAddressSeed() addrman.Add(vAdd, resolveSource); } else { // We now avoid directly using results from DNS Seeds which do not support service bit filtering, - // instead using them as a oneshot to get nodes with our desired service bits. - AddOneShot(seed); + // instead using them as a addrfetch to get nodes with our desired service bits. + AddAddrFetch(seed); } } --seeds_right_now; @@ -1727,15 +1727,15 @@ void CConnman::DumpAddresses() addrman.size(), GetTimeMillis() - nStart); } -void CConnman::ProcessOneShot() +void CConnman::ProcessAddrFetch() { std::string strDest; { - LOCK(cs_vOneShots); - if (vOneShots.empty()) + LOCK(m_addr_fetches_mutex); + if (m_addr_fetches.empty()) return; - strDest = vOneShots.front(); - vOneShots.pop_front(); + strDest = m_addr_fetches.front(); + m_addr_fetches.pop_front(); } CAddress addr; CSemaphoreGrant grant(*semOutbound, true); @@ -1767,7 +1767,7 @@ int CConnman::GetExtraOutboundCount() { LOCK(cs_vNodes); for (const CNode* pnode : vNodes) { - if (!pnode->fInbound && !pnode->m_manual_connection && !pnode->fFeeler && !pnode->fDisconnect && !pnode->fOneShot && pnode->fSuccessfullyConnected) { + if (!pnode->fInbound && !pnode->m_manual_connection && !pnode->fFeeler && !pnode->fDisconnect && !pnode->m_addr_fetch && pnode->fSuccessfullyConnected) { ++nOutbound; } } @@ -1782,7 +1782,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect) { for (int64_t nLoop = 0;; nLoop++) { - ProcessOneShot(); + ProcessAddrFetch(); for (const std::string& strAddr : connect) { CAddress addr(CService(), NODE_NONE); @@ -1805,7 +1805,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect) int64_t nNextFeeler = PoissonNextSend(nStart*1000*1000, FEELER_INTERVAL); while (!interruptNet) { - ProcessOneShot(); + ProcessAddrFetch(); if (!interruptNet.sleep_for(std::chrono::milliseconds(500))) return; @@ -2039,7 +2039,7 @@ void CConnman::ThreadOpenAddedConnections() } // if successful, this moves the passed grant to the constructed node -void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound, const char *pszDest, bool fOneShot, bool fFeeler, bool manual_connection, bool block_relay_only) +void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound, const char *pszDest, bool m_addr_fetch, bool fFeeler, bool manual_connection, bool block_relay_only) { // // Initiate outbound network connection @@ -2064,8 +2064,8 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai return; if (grantOutbound) grantOutbound->MoveTo(pnode->grantOutbound); - if (fOneShot) - pnode->fOneShot = true; + if (m_addr_fetch) + pnode->m_addr_fetch = true; if (fFeeler) pnode->fFeeler = true; if (manual_connection) @@ -2337,7 +2337,7 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions) } for (const auto& strDest : connOptions.vSeedNodes) { - AddOneShot(strDest); + AddAddrFetch(strDest); } if (clientInterface) { diff --git a/src/net.h b/src/net.h index 17d8fda37..99d1c21f9 100644 --- a/src/net.h +++ b/src/net.h @@ -197,7 +197,7 @@ public: bool GetNetworkActive() const { return fNetworkActive; }; bool GetUseAddrmanOutgoing() const { return m_use_addrman_outgoing; }; void SetNetworkActive(bool active); - void OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound = nullptr, const char *strDest = nullptr, bool fOneShot = false, bool fFeeler = false, bool manual_connection = false, bool block_relay_only = false); + void OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound = nullptr, const char *strDest = nullptr, bool m_addr_fetch = false, bool fFeeler = false, bool manual_connection = false, bool block_relay_only = false); bool CheckIncomingNonce(uint64_t nonce); bool ForNode(NodeId id, std::function func); @@ -340,8 +340,8 @@ private: bool Bind(const CService& addr, unsigned int flags, NetPermissionFlags permissions); bool InitBinds(const std::vector& binds, const std::vector& whiteBinds); void ThreadOpenAddedConnections(); - void AddOneShot(const std::string& strDest); - void ProcessOneShot(); + void AddAddrFetch(const std::string& strDest); + void ProcessAddrFetch(); void ThreadOpenConnections(std::vector connect); void ThreadMessageHandler(); void AcceptConnection(const ListenSocket& hListenSocket); @@ -405,8 +405,8 @@ private: std::atomic fNetworkActive{true}; bool fAddressesInitialized{false}; CAddrMan addrman; - std::deque vOneShots GUARDED_BY(cs_vOneShots); - RecursiveMutex cs_vOneShots; + std::deque m_addr_fetches GUARDED_BY(m_addr_fetches_mutex); + RecursiveMutex m_addr_fetches_mutex; std::vector vAddedNodes GUARDED_BY(cs_vAddedNodes); RecursiveMutex cs_vAddedNodes; std::vector vNodes GUARDED_BY(cs_vNodes); @@ -765,7 +765,7 @@ public: // This boolean is unusued in actual processing, only present for backward compatibility at RPC/QT level bool m_legacyWhitelisted{false}; bool fFeeler{false}; // If true this node is being used as a short lived feeler. - bool fOneShot{false}; + bool m_addr_fetch{false}; bool m_manual_connection{false}; bool fClient{false}; // set by version message bool m_limited_node{false}; //after BIP159, set by version message diff --git a/src/net_processing.cpp b/src/net_processing.cpp index ca701a7e5..ba30f7a7b 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -473,7 +473,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.fInbound || node.HasPermission(PF_NOBAN)) && !node.fOneShot && !node.fClient; + state->fPreferredDownload = (!node.fInbound || node.HasPermission(PF_NOBAN)) && !node.m_addr_fetch && !node.fClient; nPreferredDownload += state->fPreferredDownload; } @@ -831,7 +831,7 @@ void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds) // one-shots. static bool IsOutboundDisconnectionCandidate(const CNode& node) { - return !(node.fInbound || node.m_manual_connection || node.fFeeler || node.fOneShot); + return !(node.fInbound || node.m_manual_connection || node.fFeeler || node.m_addr_fetch); } void PeerLogicValidation::InitializeNode(CNode *pnode) { @@ -2584,7 +2584,7 @@ void ProcessMessage( connman.AddNewAddresses(vAddrOk, pfrom.addr, 2 * 60 * 60); if (vAddr.size() < 1000) pfrom.fGetAddr = false; - if (pfrom.fOneShot) + if (pfrom.m_addr_fetch) pfrom.fDisconnect = true; return; } @@ -4097,7 +4097,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto) // Start block sync if (pindexBestHeader == nullptr) pindexBestHeader = ::ChainActive().Tip(); - bool fFetch = state.fPreferredDownload || (nPreferredDownload == 0 && !pto->fClient && !pto->fOneShot); // Download if this is a nice peer, or we have no nice peers and this one might do. + bool fFetch = state.fPreferredDownload || (nPreferredDownload == 0 && !pto->fClient && !pto->m_addr_fetch); // Download if this is a nice peer, or we have no nice peers and this one might do. if (!state.fSyncStarted && !pto->fClient && !fImporting && !fReindex) { // Only actively request headers from a single peer, unless we're close to today. if ((nSyncStarted == 0 && fFetch) || pindexBestHeader->GetBlockTime() > GetAdjustedTime() - 24 * 60 * 60) { From 26304b4100201754fb32440bec3e3b78cd3f0e6d Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Wed, 6 May 2020 18:09:24 -0700 Subject: [PATCH 02/19] [net/refactor] Introduce an enum to distinguish type of connection - extract inbound & outbound types --- src/net.cpp | 8 ++++---- src/net.h | 6 +++++- src/test/denialofservice_tests.cpp | 10 +++++----- src/test/fuzz/process_message.cpp | 2 +- src/test/fuzz/process_messages.cpp | 4 ++-- src/test/net_tests.cpp | 8 +++----- 6 files changed, 20 insertions(+), 18 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 683a89ad1..061605f95 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -459,7 +459,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo NodeId id = GetNewNodeId(); uint64_t nonce = GetDeterministicRandomizer(RANDOMIZER_ID_LOCALHOSTNONCE).Write(id).Finalize(); CAddress addr_bind = GetBindAddress(hSocket); - CNode* pnode = new CNode(id, nLocalServices, GetBestHeight(), hSocket, addrConnect, CalculateKeyedNetGroup(addrConnect), nonce, addr_bind, pszDest ? pszDest : "", false, block_relay_only); + CNode* pnode = new CNode(id, nLocalServices, GetBestHeight(), hSocket, addrConnect, CalculateKeyedNetGroup(addrConnect), nonce, addr_bind, pszDest ? pszDest : "", ConnectionType::OUTBOUND, block_relay_only); pnode->AddRef(); // We're making a new connection, harvest entropy from the time (and our peer count) @@ -1048,7 +1048,7 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) { if (NetPermissions::HasFlag(permissionFlags, PF_BLOOMFILTER)) { nodeServices = static_cast(nodeServices | NODE_BLOOM); } - CNode* pnode = new CNode(id, nodeServices, GetBestHeight(), hSocket, addr, CalculateKeyedNetGroup(addr), nonce, addr_bind, "", true); + CNode* pnode = new CNode(id, nodeServices, GetBestHeight(), hSocket, addr, CalculateKeyedNetGroup(addr), nonce, addr_bind, "", ConnectionType::INBOUND); pnode->AddRef(); pnode->m_permissionFlags = permissionFlags; // If this flag is present, the user probably expect that RPC and QT report it as whitelisted (backward compatibility) @@ -2748,11 +2748,11 @@ int CConnman::GetBestHeight() const unsigned int CConnman::GetReceiveFloodSize() const { return nReceiveFloodSize; } -CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn, SOCKET hSocketIn, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress& addrBindIn, const std::string& addrNameIn, bool fInboundIn, bool block_relay_only) +CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn, SOCKET hSocketIn, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress& addrBindIn, const std::string& addrNameIn, ConnectionType conn_type_in, bool block_relay_only) : nTimeConnected(GetSystemTimeInSeconds()), addr(addrIn), addrBind(addrBindIn), - fInbound(fInboundIn), + fInbound(conn_type_in == ConnectionType::INBOUND), nKeyedNetGroup(nKeyedNetGroupIn), // 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 diff --git a/src/net.h b/src/net.h index 99d1c21f9..da99c5f67 100644 --- a/src/net.h +++ b/src/net.h @@ -113,6 +113,10 @@ struct CSerializedNetMsg std::string m_type; }; +enum class ConnectionType { + INBOUND, + OUTBOUND, +}; class NetEventsInterface; class CConnman @@ -856,7 +860,7 @@ public: std::set orphan_work_set; - CNode(NodeId id, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn, SOCKET hSocketIn, const CAddress &addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress &addrBindIn, const std::string &addrNameIn = "", bool fInboundIn = false, bool block_relay_only = false); + CNode(NodeId id, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn, SOCKET hSocketIn, const CAddress &addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress &addrBindIn, const std::string &addrNameIn = "", ConnectionType conn_type_in = ConnectionType::OUTBOUND, bool block_relay_only = false); ~CNode(); CNode(const CNode&) = delete; CNode& operator=(const CNode&) = delete; diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index b1a635d9d..0115803e5 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -84,7 +84,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) // Mock an outbound peer CAddress addr1(ip(0xa0b0c001), NODE_NONE); - CNode dummyNode1(id++, ServiceFlags(NODE_NETWORK|NODE_WITNESS), 0, INVALID_SOCKET, addr1, 0, 0, CAddress(), "", /*fInboundIn=*/ false); + CNode dummyNode1(id++, ServiceFlags(NODE_NETWORK|NODE_WITNESS), 0, INVALID_SOCKET, addr1, 0, 0, CAddress(), "", ConnectionType::OUTBOUND); dummyNode1.SetSendVersion(PROTOCOL_VERSION); peerLogic->InitializeNode(&dummyNode1); @@ -136,7 +136,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) static void AddRandomOutboundPeer(std::vector &vNodes, PeerLogicValidation &peerLogic, CConnmanTest* connman) { CAddress addr(ip(g_insecure_rand_ctx.randbits(32)), NODE_NONE); - vNodes.emplace_back(new CNode(id++, ServiceFlags(NODE_NETWORK|NODE_WITNESS), 0, INVALID_SOCKET, addr, 0, 0, CAddress(), "", /*fInboundIn=*/ false)); + vNodes.emplace_back(new CNode(id++, ServiceFlags(NODE_NETWORK|NODE_WITNESS), 0, INVALID_SOCKET, addr, 0, 0, CAddress(), "", ConnectionType::OUTBOUND)); CNode &node = *vNodes.back(); node.SetSendVersion(PROTOCOL_VERSION); @@ -227,7 +227,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) banman->ClearBanned(); CAddress addr1(ip(0xa0b0c001), NODE_NONE); - CNode dummyNode1(id++, NODE_NETWORK, 0, INVALID_SOCKET, addr1, 0, 0, CAddress(), "", true); + CNode dummyNode1(id++, NODE_NETWORK, 0, INVALID_SOCKET, addr1, 0, 0, CAddress(), "", ConnectionType::INBOUND); dummyNode1.SetSendVersion(PROTOCOL_VERSION); peerLogic->InitializeNode(&dummyNode1); dummyNode1.nVersion = 1; @@ -244,7 +244,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) BOOST_CHECK(!banman->IsDiscouraged(ip(0xa0b0c001|0x0000ff00))); // Different IP, not discouraged CAddress addr2(ip(0xa0b0c002), NODE_NONE); - CNode dummyNode2(id++, NODE_NETWORK, 0, INVALID_SOCKET, addr2, 1, 1, CAddress(), "", true); + CNode dummyNode2(id++, NODE_NETWORK, 0, INVALID_SOCKET, addr2, 1, 1, CAddress(), "", ConnectionType::INBOUND); dummyNode2.SetSendVersion(PROTOCOL_VERSION); peerLogic->InitializeNode(&dummyNode2); dummyNode2.nVersion = 1; @@ -286,7 +286,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime) SetMockTime(nStartTime); // Overrides future calls to GetTime() CAddress addr(ip(0xa0b0c001), NODE_NONE); - CNode dummyNode(id++, NODE_NETWORK, 0, INVALID_SOCKET, addr, 4, 4, CAddress(), "", true); + CNode dummyNode(id++, NODE_NETWORK, 0, INVALID_SOCKET, addr, 4, 4, CAddress(), "", ConnectionType::INBOUND); dummyNode.SetSendVersion(PROTOCOL_VERSION); peerLogic->InitializeNode(&dummyNode); dummyNode.nVersion = 1; diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp index 9e40d5cd5..ad3a91e2b 100644 --- a/src/test/fuzz/process_message.cpp +++ b/src/test/fuzz/process_message.cpp @@ -80,7 +80,7 @@ void test_one_input(const std::vector& buffer) return; } CDataStream random_bytes_data_stream{fuzzed_data_provider.ConsumeRemainingBytes(), SER_NETWORK, PROTOCOL_VERSION}; - CNode& p2p_node = *MakeUnique(0, ServiceFlags(NODE_NETWORK | NODE_WITNESS | NODE_BLOOM), 0, INVALID_SOCKET, CAddress{CService{in_addr{0x0100007f}, 7777}, NODE_NETWORK}, 0, 0, CAddress{}, std::string{}, false).release(); + CNode& p2p_node = *MakeUnique(0, ServiceFlags(NODE_NETWORK | NODE_WITNESS | NODE_BLOOM), 0, INVALID_SOCKET, CAddress{CService{in_addr{0x0100007f}, 7777}, NODE_NETWORK}, 0, 0, CAddress{}, std::string{}, ConnectionType::OUTBOUND, false).release(); p2p_node.fSuccessfullyConnected = true; p2p_node.nVersion = PROTOCOL_VERSION; p2p_node.SetSendVersion(PROTOCOL_VERSION); diff --git a/src/test/fuzz/process_messages.cpp b/src/test/fuzz/process_messages.cpp index 91ebf9fb1..1013113e3 100644 --- a/src/test/fuzz/process_messages.cpp +++ b/src/test/fuzz/process_messages.cpp @@ -44,9 +44,9 @@ void test_one_input(const std::vector& buffer) const auto num_peers_to_add = fuzzed_data_provider.ConsumeIntegralInRange(1, 3); for (int i = 0; i < num_peers_to_add; ++i) { const ServiceFlags service_flags = ServiceFlags(fuzzed_data_provider.ConsumeIntegral()); - const bool inbound{fuzzed_data_provider.ConsumeBool()}; + const ConnectionType conn_type = fuzzed_data_provider.PickValueInArray({ConnectionType::INBOUND, ConnectionType::OUTBOUND}); const bool block_relay_only{fuzzed_data_provider.ConsumeBool()}; - peers.push_back(MakeUnique(i, service_flags, 0, INVALID_SOCKET, CAddress{CService{in_addr{0x0100007f}, 7777}, NODE_NETWORK}, 0, 0, CAddress{}, std::string{}, inbound, block_relay_only).release()); + peers.push_back(MakeUnique(i, service_flags, 0, INVALID_SOCKET, CAddress{CService{in_addr{0x0100007f}, 7777}, NODE_NETWORK}, 0, 0, CAddress{}, std::string{}, conn_type, block_relay_only).release()); CNode& p2p_node = *peers.back(); p2p_node.fSuccessfullyConnected = true; diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index ab42be21b..5840f5a72 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -180,15 +180,13 @@ BOOST_AUTO_TEST_CASE(cnode_simple_test) CAddress addr = CAddress(CService(ipv4Addr, 7777), NODE_NETWORK); std::string pszDest; - bool fInboundIn = false; // Test that fFeeler is false by default. - std::unique_ptr pnode1 = MakeUnique(id++, NODE_NETWORK, height, hSocket, addr, 0, 0, CAddress(), pszDest, fInboundIn); + std::unique_ptr pnode1 = MakeUnique(id++, NODE_NETWORK, height, hSocket, addr, 0, 0, CAddress(), pszDest, ConnectionType::OUTBOUND); BOOST_CHECK(pnode1->fInbound == false); BOOST_CHECK(pnode1->fFeeler == false); - fInboundIn = true; - std::unique_ptr pnode2 = MakeUnique(id++, NODE_NETWORK, height, hSocket, addr, 1, 1, CAddress(), pszDest, fInboundIn); + std::unique_ptr pnode2 = MakeUnique(id++, NODE_NETWORK, height, hSocket, addr, 1, 1, CAddress(), pszDest, ConnectionType::INBOUND); BOOST_CHECK(pnode2->fInbound == true); BOOST_CHECK(pnode2->fFeeler == false); } @@ -214,7 +212,7 @@ BOOST_AUTO_TEST_CASE(ipv4_peer_with_ipv6_addrMe_test) in_addr ipv4AddrPeer; ipv4AddrPeer.s_addr = 0xa0b0c001; CAddress addr = CAddress(CService(ipv4AddrPeer, 7777), NODE_NETWORK); - std::unique_ptr pnode = MakeUnique(0, NODE_NETWORK, 0, INVALID_SOCKET, addr, 0, 0, CAddress{}, std::string{}, false); + std::unique_ptr pnode = MakeUnique(0, NODE_NETWORK, 0, INVALID_SOCKET, addr, 0, 0, CAddress{}, std::string{}, ConnectionType::OUTBOUND); pnode->fSuccessfullyConnected.store(true); // the peer claims to be reaching us via IPv6 From 1521c47438537e192230486dffcec0228a53878d Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Wed, 29 Apr 2020 14:55:59 -0700 Subject: [PATCH 03/19] [net/refactor] Add manual connections to ConnectionType enum --- src/net.cpp | 41 ++++++++++-------------------- src/net.h | 5 ++-- src/rpc/net.cpp | 2 +- src/test/fuzz/process_messages.cpp | 2 +- 4 files changed, 19 insertions(+), 31 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 061605f95..272ea35c2 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -368,8 +368,10 @@ static CAddress GetBindAddress(SOCKET sock) return addr_bind; } -CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, bool manual_connection, bool block_relay_only) +CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, ConnectionType conn_type, bool block_relay_only) { + assert(conn_type != ConnectionType::INBOUND); + if (pszDest == nullptr) { if (IsLocal(addrConnect)) return nullptr; @@ -432,7 +434,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo if (hSocket == INVALID_SOCKET) { return nullptr; } - connected = ConnectSocketDirectly(addrConnect, hSocket, nConnectTimeout, manual_connection); + connected = ConnectSocketDirectly(addrConnect, hSocket, nConnectTimeout, conn_type == ConnectionType::MANUAL); } if (!proxyConnectionFailed) { // If a connection to the node was attempted, and failure (if any) is not caused by a problem connecting to @@ -459,7 +461,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo NodeId id = GetNewNodeId(); uint64_t nonce = GetDeterministicRandomizer(RANDOMIZER_ID_LOCALHOSTNONCE).Write(id).Finalize(); CAddress addr_bind = GetBindAddress(hSocket); - CNode* pnode = new CNode(id, nLocalServices, GetBestHeight(), hSocket, addrConnect, CalculateKeyedNetGroup(addrConnect), nonce, addr_bind, pszDest ? pszDest : "", ConnectionType::OUTBOUND, block_relay_only); + CNode* pnode = new CNode(id, nLocalServices, GetBestHeight(), hSocket, addrConnect, CalculateKeyedNetGroup(addrConnect), nonce, addr_bind, pszDest ? pszDest : "", conn_type, block_relay_only); pnode->AddRef(); // We're making a new connection, harvest entropy from the time (and our peer count) @@ -1705,17 +1707,6 @@ void CConnman::ThreadDNSAddressSeed() LogPrintf("%d addresses found from DNS seeds\n", found); } - - - - - - - - - - - void CConnman::DumpAddresses() { int64_t nStart = GetTimeMillis(); @@ -1786,7 +1777,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect) for (const std::string& strAddr : connect) { CAddress addr(CService(), NODE_NONE); - OpenNetworkConnection(addr, false, nullptr, strAddr.c_str(), false, false, true); + OpenNetworkConnection(addr, false, nullptr, strAddr.c_str(), false, false, ConnectionType::MANUAL); for (int i = 0; i < 10 && i < nLoop; i++) { if (!interruptNet.sleep_for(std::chrono::milliseconds(500))) @@ -1952,7 +1943,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect) // well for sanity.) bool block_relay_only = nOutboundBlockRelay < m_max_outbound_block_relay && !fFeeler && nOutboundFullRelay >= m_max_outbound_full_relay; - OpenNetworkConnection(addrConnect, (int)setConnected.size() >= std::min(nMaxConnections - 1, 2), &grant, nullptr, false, fFeeler, false, block_relay_only); + OpenNetworkConnection(addrConnect, (int)setConnected.size() >= std::min(nMaxConnections - 1, 2), &grant, nullptr, false, fFeeler, ConnectionType::OUTBOUND, block_relay_only); } } } @@ -2027,7 +2018,7 @@ void CConnman::ThreadOpenAddedConnections() } tried = true; CAddress addr(CService(), NODE_NONE); - OpenNetworkConnection(addr, false, &grant, info.strAddedNode.c_str(), false, false, true); + OpenNetworkConnection(addr, false, &grant, info.strAddedNode.c_str(), false, false, ConnectionType::MANUAL); if (!interruptNet.sleep_for(std::chrono::milliseconds(500))) return; } @@ -2039,8 +2030,10 @@ void CConnman::ThreadOpenAddedConnections() } // if successful, this moves the passed grant to the constructed node -void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound, const char *pszDest, bool m_addr_fetch, bool fFeeler, bool manual_connection, bool block_relay_only) +void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound, const char *pszDest, bool m_addr_fetch, bool fFeeler, ConnectionType conn_type, bool block_relay_only) { + assert(conn_type != ConnectionType::INBOUND); + // // Initiate outbound network connection // @@ -2058,7 +2051,7 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai } else if (FindNode(std::string(pszDest))) return; - CNode* pnode = ConnectNode(addrConnect, pszDest, fCountFailure, manual_connection, block_relay_only); + CNode* pnode = ConnectNode(addrConnect, pszDest, fCountFailure, conn_type, block_relay_only); if (!pnode) return; @@ -2068,8 +2061,6 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai pnode->m_addr_fetch = true; if (fFeeler) pnode->fFeeler = true; - if (manual_connection) - pnode->m_manual_connection = true; m_msgproc->InitializeNode(pnode); { @@ -2127,11 +2118,6 @@ void CConnman::ThreadMessageHandler() } } - - - - - bool CConnman::BindListenPort(const CService& addrBind, bilingual_str& strError, NetPermissionFlags permissions) { int nOne = 1; @@ -2390,7 +2376,7 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions) else threadDNSAddressSeed = std::thread(&TraceThread >, "dnsseed", std::function(std::bind(&CConnman::ThreadDNSAddressSeed, this))); - // Initiate outbound connections from -addnode + // Initiate manual connections threadOpenAddedConnections = std::thread(&TraceThread >, "addcon", std::function(std::bind(&CConnman::ThreadOpenAddedConnections, this))); if (connOptions.m_use_addrman_outgoing && !connOptions.m_specified_outgoing.empty()) { @@ -2752,6 +2738,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn : nTimeConnected(GetSystemTimeInSeconds()), addr(addrIn), addrBind(addrBindIn), + m_manual_connection(conn_type_in == ConnectionType::MANUAL), fInbound(conn_type_in == ConnectionType::INBOUND), nKeyedNetGroup(nKeyedNetGroupIn), // Don't relay addr messages to peers that we connect to as block-relay-only diff --git a/src/net.h b/src/net.h index da99c5f67..f7741e837 100644 --- a/src/net.h +++ b/src/net.h @@ -116,6 +116,7 @@ struct CSerializedNetMsg enum class ConnectionType { INBOUND, OUTBOUND, + MANUAL, }; class NetEventsInterface; @@ -201,7 +202,7 @@ public: bool GetNetworkActive() const { return fNetworkActive; }; bool GetUseAddrmanOutgoing() const { return m_use_addrman_outgoing; }; void SetNetworkActive(bool active); - void OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound = nullptr, const char *strDest = nullptr, bool m_addr_fetch = false, bool fFeeler = false, bool manual_connection = false, bool block_relay_only = false); + void OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound = nullptr, const char *strDest = nullptr, bool m_addr_fetch = false, bool fFeeler = false, ConnectionType conn_type = ConnectionType::OUTBOUND, bool block_relay_only = false); bool CheckIncomingNonce(uint64_t nonce); bool ForNode(NodeId id, std::function func); @@ -366,7 +367,7 @@ private: CNode* FindNode(const CService& addr); bool AttemptToEvictConnection(); - CNode* ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, bool manual_connection, bool block_relay_only); + CNode* ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, ConnectionType conn_type, bool block_relay_only); void AddWhitelistPermissionFlags(NetPermissionFlags& flags, const CNetAddr &addr) const; void DeleteNode(CNode* pnode); diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 9981ea35d..69955b5b2 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -264,7 +264,7 @@ static UniValue addnode(const JSONRPCRequest& request) if (strCommand == "onetry") { CAddress addr; - node.connman->OpenNetworkConnection(addr, false, nullptr, strNode.c_str(), false, false, true); + node.connman->OpenNetworkConnection(addr, false, nullptr, strNode.c_str(), false, false, ConnectionType::MANUAL); return NullUniValue; } diff --git a/src/test/fuzz/process_messages.cpp b/src/test/fuzz/process_messages.cpp index 1013113e3..723be1362 100644 --- a/src/test/fuzz/process_messages.cpp +++ b/src/test/fuzz/process_messages.cpp @@ -44,7 +44,7 @@ void test_one_input(const std::vector& buffer) const auto num_peers_to_add = fuzzed_data_provider.ConsumeIntegralInRange(1, 3); for (int i = 0; i < num_peers_to_add; ++i) { const ServiceFlags service_flags = ServiceFlags(fuzzed_data_provider.ConsumeIntegral()); - const ConnectionType conn_type = fuzzed_data_provider.PickValueInArray({ConnectionType::INBOUND, ConnectionType::OUTBOUND}); + const ConnectionType conn_type = fuzzed_data_provider.PickValueInArray({ConnectionType::INBOUND, ConnectionType::OUTBOUND, ConnectionType::MANUAL}); const bool block_relay_only{fuzzed_data_provider.ConsumeBool()}; peers.push_back(MakeUnique(i, service_flags, 0, INVALID_SOCKET, CAddress{CService{in_addr{0x0100007f}, 7777}, NODE_NETWORK}, 0, 0, CAddress{}, std::string{}, conn_type, block_relay_only).release()); CNode& p2p_node = *peers.back(); From 0e52a659a2de915fc3dce37fc8fac39be1c8b6fa Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Wed, 29 Apr 2020 16:28:56 -0700 Subject: [PATCH 04/19] [net/refactor] Add feeler connections to ConnectionType enum --- src/net.cpp | 12 ++++++------ src/net.h | 3 ++- src/rpc/net.cpp | 2 +- src/test/fuzz/process_messages.cpp | 2 +- 4 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 272ea35c2..580d0a184 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1777,7 +1777,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect) for (const std::string& strAddr : connect) { CAddress addr(CService(), NODE_NONE); - OpenNetworkConnection(addr, false, nullptr, strAddr.c_str(), false, false, ConnectionType::MANUAL); + OpenNetworkConnection(addr, false, nullptr, strAddr.c_str(), false, ConnectionType::MANUAL); for (int i = 0; i < 10 && i < nLoop; i++) { if (!interruptNet.sleep_for(std::chrono::milliseconds(500))) @@ -1943,7 +1943,8 @@ void CConnman::ThreadOpenConnections(const std::vector connect) // well for sanity.) bool block_relay_only = nOutboundBlockRelay < m_max_outbound_block_relay && !fFeeler && nOutboundFullRelay >= m_max_outbound_full_relay; - OpenNetworkConnection(addrConnect, (int)setConnected.size() >= std::min(nMaxConnections - 1, 2), &grant, nullptr, false, fFeeler, ConnectionType::OUTBOUND, block_relay_only); + ConnectionType conn_type = (fFeeler ? ConnectionType::FEELER : ConnectionType::OUTBOUND); + OpenNetworkConnection(addrConnect, (int)setConnected.size() >= std::min(nMaxConnections - 1, 2), &grant, nullptr, false, conn_type, block_relay_only); } } } @@ -2018,7 +2019,7 @@ void CConnman::ThreadOpenAddedConnections() } tried = true; CAddress addr(CService(), NODE_NONE); - OpenNetworkConnection(addr, false, &grant, info.strAddedNode.c_str(), false, false, ConnectionType::MANUAL); + OpenNetworkConnection(addr, false, &grant, info.strAddedNode.c_str(), false, ConnectionType::MANUAL); if (!interruptNet.sleep_for(std::chrono::milliseconds(500))) return; } @@ -2030,7 +2031,7 @@ void CConnman::ThreadOpenAddedConnections() } // if successful, this moves the passed grant to the constructed node -void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound, const char *pszDest, bool m_addr_fetch, bool fFeeler, ConnectionType conn_type, bool block_relay_only) +void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound, const char *pszDest, bool m_addr_fetch, ConnectionType conn_type, bool block_relay_only) { assert(conn_type != ConnectionType::INBOUND); @@ -2059,8 +2060,6 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai grantOutbound->MoveTo(pnode->grantOutbound); if (m_addr_fetch) pnode->m_addr_fetch = true; - if (fFeeler) - pnode->fFeeler = true; m_msgproc->InitializeNode(pnode); { @@ -2738,6 +2737,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn : nTimeConnected(GetSystemTimeInSeconds()), addr(addrIn), addrBind(addrBindIn), + fFeeler(conn_type_in == ConnectionType::FEELER), m_manual_connection(conn_type_in == ConnectionType::MANUAL), fInbound(conn_type_in == ConnectionType::INBOUND), nKeyedNetGroup(nKeyedNetGroupIn), diff --git a/src/net.h b/src/net.h index f7741e837..7eb99e3ed 100644 --- a/src/net.h +++ b/src/net.h @@ -117,6 +117,7 @@ enum class ConnectionType { INBOUND, OUTBOUND, MANUAL, + FEELER, }; class NetEventsInterface; @@ -202,7 +203,7 @@ public: bool GetNetworkActive() const { return fNetworkActive; }; bool GetUseAddrmanOutgoing() const { return m_use_addrman_outgoing; }; void SetNetworkActive(bool active); - void OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound = nullptr, const char *strDest = nullptr, bool m_addr_fetch = false, bool fFeeler = false, ConnectionType conn_type = ConnectionType::OUTBOUND, bool block_relay_only = false); + void OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound = nullptr, const char *strDest = nullptr, bool m_addr_fetch = false, ConnectionType conn_type = ConnectionType::OUTBOUND, bool block_relay_only = false); bool CheckIncomingNonce(uint64_t nonce); bool ForNode(NodeId id, std::function func); diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 69955b5b2..716e43347 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -264,7 +264,7 @@ static UniValue addnode(const JSONRPCRequest& request) if (strCommand == "onetry") { CAddress addr; - node.connman->OpenNetworkConnection(addr, false, nullptr, strNode.c_str(), false, false, ConnectionType::MANUAL); + node.connman->OpenNetworkConnection(addr, false, nullptr, strNode.c_str(), false, ConnectionType::MANUAL); return NullUniValue; } diff --git a/src/test/fuzz/process_messages.cpp b/src/test/fuzz/process_messages.cpp index 723be1362..50f0457e4 100644 --- a/src/test/fuzz/process_messages.cpp +++ b/src/test/fuzz/process_messages.cpp @@ -44,7 +44,7 @@ void test_one_input(const std::vector& buffer) const auto num_peers_to_add = fuzzed_data_provider.ConsumeIntegralInRange(1, 3); for (int i = 0; i < num_peers_to_add; ++i) { const ServiceFlags service_flags = ServiceFlags(fuzzed_data_provider.ConsumeIntegral()); - const ConnectionType conn_type = fuzzed_data_provider.PickValueInArray({ConnectionType::INBOUND, ConnectionType::OUTBOUND, ConnectionType::MANUAL}); + const ConnectionType conn_type = fuzzed_data_provider.PickValueInArray({ConnectionType::INBOUND, ConnectionType::OUTBOUND, ConnectionType::MANUAL, ConnectionType::FEELER}); const bool block_relay_only{fuzzed_data_provider.ConsumeBool()}; peers.push_back(MakeUnique(i, service_flags, 0, INVALID_SOCKET, CAddress{CService{in_addr{0x0100007f}, 7777}, NODE_NETWORK}, 0, 0, CAddress{}, std::string{}, conn_type, block_relay_only).release()); CNode& p2p_node = *peers.back(); From e1bc29812ddf1d946bc5acca406a7ed2dca064a6 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Wed, 29 Apr 2020 17:33:06 -0700 Subject: [PATCH 05/19] [net/refactor] Add block relay only connections to ConnectionType enum --- src/net.cpp | 30 +++++++++++++++++------------- src/net.h | 7 ++++--- src/test/fuzz/process_message.cpp | 2 +- src/test/fuzz/process_messages.cpp | 5 ++--- 4 files changed, 24 insertions(+), 20 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 580d0a184..e18128dc1 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -368,7 +368,7 @@ static CAddress GetBindAddress(SOCKET sock) return addr_bind; } -CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, ConnectionType conn_type, bool block_relay_only) +CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, ConnectionType conn_type) { assert(conn_type != ConnectionType::INBOUND); @@ -461,7 +461,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo NodeId id = GetNewNodeId(); uint64_t nonce = GetDeterministicRandomizer(RANDOMIZER_ID_LOCALHOSTNONCE).Write(id).Finalize(); CAddress addr_bind = GetBindAddress(hSocket); - CNode* pnode = new CNode(id, nLocalServices, GetBestHeight(), hSocket, addrConnect, CalculateKeyedNetGroup(addrConnect), nonce, addr_bind, pszDest ? pszDest : "", conn_type, block_relay_only); + CNode* pnode = new CNode(id, nLocalServices, GetBestHeight(), hSocket, addrConnect, CalculateKeyedNetGroup(addrConnect), nonce, addr_bind, pszDest ? pszDest : "", conn_type); pnode->AddRef(); // We're making a new connection, harvest entropy from the time (and our peer count) @@ -1938,13 +1938,17 @@ void CConnman::ThreadOpenConnections(const std::vector connect) // Open this connection as block-relay-only if we're already at our // full-relay capacity, but not yet at our block-relay peer limit. - // (It should not be possible for fFeeler to be set if we're not - // also at our block-relay peer limit, but check against that as - // well for sanity.) - bool block_relay_only = nOutboundBlockRelay < m_max_outbound_block_relay && !fFeeler && nOutboundFullRelay >= m_max_outbound_full_relay; + bool block_relay_only = nOutboundBlockRelay < m_max_outbound_block_relay && nOutboundFullRelay >= m_max_outbound_full_relay; + ConnectionType conn_type; + if(fFeeler) { + conn_type = ConnectionType::FEELER; + } else if (block_relay_only) { + conn_type = ConnectionType::BLOCK_RELAY; + } else { + conn_type = ConnectionType::OUTBOUND; + } - ConnectionType conn_type = (fFeeler ? ConnectionType::FEELER : ConnectionType::OUTBOUND); - OpenNetworkConnection(addrConnect, (int)setConnected.size() >= std::min(nMaxConnections - 1, 2), &grant, nullptr, false, conn_type, block_relay_only); + OpenNetworkConnection(addrConnect, (int)setConnected.size() >= std::min(nMaxConnections - 1, 2), &grant, nullptr, false, conn_type); } } } @@ -2031,7 +2035,7 @@ void CConnman::ThreadOpenAddedConnections() } // if successful, this moves the passed grant to the constructed node -void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound, const char *pszDest, bool m_addr_fetch, ConnectionType conn_type, bool block_relay_only) +void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound, const char *pszDest, bool m_addr_fetch, ConnectionType conn_type) { assert(conn_type != ConnectionType::INBOUND); @@ -2052,7 +2056,7 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai } else if (FindNode(std::string(pszDest))) return; - CNode* pnode = ConnectNode(addrConnect, pszDest, fCountFailure, conn_type, block_relay_only); + CNode* pnode = ConnectNode(addrConnect, pszDest, fCountFailure, conn_type); if (!pnode) return; @@ -2733,7 +2737,7 @@ int CConnman::GetBestHeight() const unsigned int CConnman::GetReceiveFloodSize() const { return nReceiveFloodSize; } -CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn, SOCKET hSocketIn, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress& addrBindIn, const std::string& addrNameIn, ConnectionType conn_type_in, bool block_relay_only) +CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn, SOCKET hSocketIn, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress& addrBindIn, const std::string& addrNameIn, ConnectionType conn_type_in) : nTimeConnected(GetSystemTimeInSeconds()), addr(addrIn), addrBind(addrBindIn), @@ -2744,7 +2748,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn // 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). - m_addr_known{block_relay_only ? nullptr : MakeUnique(5000, 0.001)}, + m_addr_known{conn_type_in == ConnectionType::BLOCK_RELAY ? nullptr : MakeUnique(5000, 0.001)}, id(idIn), nLocalHostNonce(nLocalHostNonceIn), nLocalServices(nLocalServicesIn), @@ -2753,7 +2757,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn hSocket = hSocketIn; addrName = addrNameIn == "" ? addr.ToStringIPPort() : addrNameIn; hashContinue = uint256(); - if (!block_relay_only) { + if (conn_type_in != ConnectionType::BLOCK_RELAY) { m_tx_relay = MakeUnique(); } diff --git a/src/net.h b/src/net.h index 7eb99e3ed..21ee6e72a 100644 --- a/src/net.h +++ b/src/net.h @@ -118,6 +118,7 @@ enum class ConnectionType { OUTBOUND, MANUAL, FEELER, + BLOCK_RELAY, }; class NetEventsInterface; @@ -203,7 +204,7 @@ public: bool GetNetworkActive() const { return fNetworkActive; }; bool GetUseAddrmanOutgoing() const { return m_use_addrman_outgoing; }; void SetNetworkActive(bool active); - void OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound = nullptr, const char *strDest = nullptr, bool m_addr_fetch = false, ConnectionType conn_type = ConnectionType::OUTBOUND, bool block_relay_only = false); + void OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound = nullptr, const char *strDest = nullptr, bool m_addr_fetch = false, ConnectionType conn_type = ConnectionType::OUTBOUND); bool CheckIncomingNonce(uint64_t nonce); bool ForNode(NodeId id, std::function func); @@ -368,7 +369,7 @@ private: CNode* FindNode(const CService& addr); bool AttemptToEvictConnection(); - CNode* ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, ConnectionType conn_type, bool block_relay_only); + CNode* ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, ConnectionType conn_type); void AddWhitelistPermissionFlags(NetPermissionFlags& flags, const CNetAddr &addr) const; void DeleteNode(CNode* pnode); @@ -862,7 +863,7 @@ public: std::set orphan_work_set; - CNode(NodeId id, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn, SOCKET hSocketIn, const CAddress &addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress &addrBindIn, const std::string &addrNameIn = "", ConnectionType conn_type_in = ConnectionType::OUTBOUND, bool block_relay_only = false); + CNode(NodeId id, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn, SOCKET hSocketIn, const CAddress &addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress &addrBindIn, const std::string &addrNameIn = "", ConnectionType conn_type_in = ConnectionType::OUTBOUND); ~CNode(); CNode(const CNode&) = delete; CNode& operator=(const CNode&) = delete; diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp index ad3a91e2b..677b87a47 100644 --- a/src/test/fuzz/process_message.cpp +++ b/src/test/fuzz/process_message.cpp @@ -80,7 +80,7 @@ void test_one_input(const std::vector& buffer) return; } CDataStream random_bytes_data_stream{fuzzed_data_provider.ConsumeRemainingBytes(), SER_NETWORK, PROTOCOL_VERSION}; - CNode& p2p_node = *MakeUnique(0, ServiceFlags(NODE_NETWORK | NODE_WITNESS | NODE_BLOOM), 0, INVALID_SOCKET, CAddress{CService{in_addr{0x0100007f}, 7777}, NODE_NETWORK}, 0, 0, CAddress{}, std::string{}, ConnectionType::OUTBOUND, false).release(); + CNode& p2p_node = *MakeUnique(0, ServiceFlags(NODE_NETWORK | NODE_WITNESS | NODE_BLOOM), 0, INVALID_SOCKET, CAddress{CService{in_addr{0x0100007f}, 7777}, NODE_NETWORK}, 0, 0, CAddress{}, std::string{}, ConnectionType::OUTBOUND).release(); p2p_node.fSuccessfullyConnected = true; p2p_node.nVersion = PROTOCOL_VERSION; p2p_node.SetSendVersion(PROTOCOL_VERSION); diff --git a/src/test/fuzz/process_messages.cpp b/src/test/fuzz/process_messages.cpp index 50f0457e4..f9e3f769a 100644 --- a/src/test/fuzz/process_messages.cpp +++ b/src/test/fuzz/process_messages.cpp @@ -44,9 +44,8 @@ void test_one_input(const std::vector& buffer) const auto num_peers_to_add = fuzzed_data_provider.ConsumeIntegralInRange(1, 3); for (int i = 0; i < num_peers_to_add; ++i) { const ServiceFlags service_flags = ServiceFlags(fuzzed_data_provider.ConsumeIntegral()); - const ConnectionType conn_type = fuzzed_data_provider.PickValueInArray({ConnectionType::INBOUND, ConnectionType::OUTBOUND, ConnectionType::MANUAL, ConnectionType::FEELER}); - const bool block_relay_only{fuzzed_data_provider.ConsumeBool()}; - peers.push_back(MakeUnique(i, service_flags, 0, INVALID_SOCKET, CAddress{CService{in_addr{0x0100007f}, 7777}, NODE_NETWORK}, 0, 0, CAddress{}, std::string{}, conn_type, block_relay_only).release()); + const ConnectionType conn_type = fuzzed_data_provider.PickValueInArray({ConnectionType::INBOUND, ConnectionType::OUTBOUND, ConnectionType::MANUAL, ConnectionType::FEELER, ConnectionType::BLOCK_RELAY}); + peers.push_back(MakeUnique(i, service_flags, 0, INVALID_SOCKET, CAddress{CService{in_addr{0x0100007f}, 7777}, NODE_NETWORK}, 0, 0, CAddress{}, std::string{}, conn_type).release()); CNode& p2p_node = *peers.back(); p2p_node.fSuccessfullyConnected = true; From af59feb05235ecb85ec9d75b09c66e71268c9889 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Tue, 12 May 2020 14:48:24 -0700 Subject: [PATCH 06/19] [net/refactor] Extract m_addr_known logic from initializer list --- src/net.cpp | 2 +- src/net.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index e18128dc1..fd1cb3e64 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2748,7 +2748,6 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn // 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). - m_addr_known{conn_type_in == ConnectionType::BLOCK_RELAY ? nullptr : MakeUnique(5000, 0.001)}, id(idIn), nLocalHostNonce(nLocalHostNonceIn), nLocalServices(nLocalServicesIn), @@ -2759,6 +2758,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn hashContinue = uint256(); if (conn_type_in != ConnectionType::BLOCK_RELAY) { m_tx_relay = MakeUnique(); + m_addr_known = MakeUnique(5000, 0.001); } for (const std::string &msg : getAllNetMessageTypes()) diff --git a/src/net.h b/src/net.h index 21ee6e72a..d3a423fd1 100644 --- a/src/net.h +++ b/src/net.h @@ -799,7 +799,7 @@ public: // flood relay std::vector vAddrToSend; - const std::unique_ptr m_addr_known; + 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}; From 442abae2bac7bff85886143df01e14215532b974 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Thu, 30 Apr 2020 10:57:03 -0700 Subject: [PATCH 07/19] [net/refactor] Add AddrFetch connections to ConnectionType enum - AddrFetch connections are short lived connections used to getaddr from a peer - previously called "one shot" connections --- src/net.cpp | 13 ++++++------- src/net.h | 3 ++- src/rpc/net.cpp | 2 +- src/test/fuzz/process_messages.cpp | 2 +- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index fd1cb3e64..a43d02bbd 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1731,7 +1731,7 @@ void CConnman::ProcessAddrFetch() CAddress addr; CSemaphoreGrant grant(*semOutbound, true); if (grant) { - OpenNetworkConnection(addr, false, &grant, strDest.c_str(), true); + OpenNetworkConnection(addr, false, &grant, strDest.c_str(), ConnectionType::ADDR_FETCH); } } @@ -1777,7 +1777,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect) for (const std::string& strAddr : connect) { CAddress addr(CService(), NODE_NONE); - OpenNetworkConnection(addr, false, nullptr, strAddr.c_str(), false, ConnectionType::MANUAL); + OpenNetworkConnection(addr, false, nullptr, strAddr.c_str(), ConnectionType::MANUAL); for (int i = 0; i < 10 && i < nLoop; i++) { if (!interruptNet.sleep_for(std::chrono::milliseconds(500))) @@ -1948,7 +1948,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect) conn_type = ConnectionType::OUTBOUND; } - OpenNetworkConnection(addrConnect, (int)setConnected.size() >= std::min(nMaxConnections - 1, 2), &grant, nullptr, false, conn_type); + OpenNetworkConnection(addrConnect, (int)setConnected.size() >= std::min(nMaxConnections - 1, 2), &grant, nullptr, conn_type); } } } @@ -2023,7 +2023,7 @@ void CConnman::ThreadOpenAddedConnections() } tried = true; CAddress addr(CService(), NODE_NONE); - OpenNetworkConnection(addr, false, &grant, info.strAddedNode.c_str(), false, ConnectionType::MANUAL); + OpenNetworkConnection(addr, false, &grant, info.strAddedNode.c_str(), ConnectionType::MANUAL); if (!interruptNet.sleep_for(std::chrono::milliseconds(500))) return; } @@ -2035,7 +2035,7 @@ void CConnman::ThreadOpenAddedConnections() } // if successful, this moves the passed grant to the constructed node -void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound, const char *pszDest, bool m_addr_fetch, ConnectionType conn_type) +void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound, const char *pszDest, ConnectionType conn_type) { assert(conn_type != ConnectionType::INBOUND); @@ -2062,8 +2062,6 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai return; if (grantOutbound) grantOutbound->MoveTo(pnode->grantOutbound); - if (m_addr_fetch) - pnode->m_addr_fetch = true; m_msgproc->InitializeNode(pnode); { @@ -2742,6 +2740,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn addr(addrIn), addrBind(addrBindIn), fFeeler(conn_type_in == ConnectionType::FEELER), + m_addr_fetch(conn_type_in == ConnectionType::ADDR_FETCH), m_manual_connection(conn_type_in == ConnectionType::MANUAL), fInbound(conn_type_in == ConnectionType::INBOUND), nKeyedNetGroup(nKeyedNetGroupIn), diff --git a/src/net.h b/src/net.h index d3a423fd1..269877b21 100644 --- a/src/net.h +++ b/src/net.h @@ -119,6 +119,7 @@ enum class ConnectionType { MANUAL, FEELER, BLOCK_RELAY, + ADDR_FETCH, }; class NetEventsInterface; @@ -204,7 +205,7 @@ public: bool GetNetworkActive() const { return fNetworkActive; }; bool GetUseAddrmanOutgoing() const { return m_use_addrman_outgoing; }; void SetNetworkActive(bool active); - void OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound = nullptr, const char *strDest = nullptr, bool m_addr_fetch = false, ConnectionType conn_type = ConnectionType::OUTBOUND); + void OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound = nullptr, const char *strDest = nullptr, ConnectionType conn_type = ConnectionType::OUTBOUND); bool CheckIncomingNonce(uint64_t nonce); bool ForNode(NodeId id, std::function func); diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 716e43347..09265bc48 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -264,7 +264,7 @@ static UniValue addnode(const JSONRPCRequest& request) if (strCommand == "onetry") { CAddress addr; - node.connman->OpenNetworkConnection(addr, false, nullptr, strNode.c_str(), false, ConnectionType::MANUAL); + node.connman->OpenNetworkConnection(addr, false, nullptr, strNode.c_str(), ConnectionType::MANUAL); return NullUniValue; } diff --git a/src/test/fuzz/process_messages.cpp b/src/test/fuzz/process_messages.cpp index f9e3f769a..ef427442e 100644 --- a/src/test/fuzz/process_messages.cpp +++ b/src/test/fuzz/process_messages.cpp @@ -44,7 +44,7 @@ void test_one_input(const std::vector& buffer) const auto num_peers_to_add = fuzzed_data_provider.ConsumeIntegralInRange(1, 3); for (int i = 0; i < num_peers_to_add; ++i) { const ServiceFlags service_flags = ServiceFlags(fuzzed_data_provider.ConsumeIntegral()); - const ConnectionType conn_type = fuzzed_data_provider.PickValueInArray({ConnectionType::INBOUND, ConnectionType::OUTBOUND, ConnectionType::MANUAL, ConnectionType::FEELER, ConnectionType::BLOCK_RELAY}); + const ConnectionType conn_type = fuzzed_data_provider.PickValueInArray({ConnectionType::INBOUND, ConnectionType::OUTBOUND, ConnectionType::MANUAL, ConnectionType::FEELER, ConnectionType::BLOCK_RELAY, ConnectionType::ADDR_FETCH}); peers.push_back(MakeUnique(i, service_flags, 0, INVALID_SOCKET, CAddress{CService{in_addr{0x0100007f}, 7777}, NODE_NETWORK}, 0, 0, CAddress{}, std::string{}, conn_type).release()); CNode& p2p_node = *peers.back(); From 46578c03e92a55925308363ccdad04dcfc820d96 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Tue, 2 Jun 2020 11:43:57 -0700 Subject: [PATCH 08/19] [doc] Describe different connection types --- src/net.h | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/net.h b/src/net.h index 269877b21..6a4ef6ab4 100644 --- a/src/net.h +++ b/src/net.h @@ -113,13 +113,16 @@ struct CSerializedNetMsg std::string m_type; }; +/** Different types of connections to a peer. This enum encapsulates the + * information we have available at the time of opening or accepting the + * connection. Aside from INBOUND, all types are initiated by us. */ enum class ConnectionType { - INBOUND, - OUTBOUND, - MANUAL, - FEELER, - BLOCK_RELAY, - ADDR_FETCH, + INBOUND, /**< peer initiated connections */ + OUTBOUND, /**< full relay connections (blocks, addrs, txns) made automatically. Addresses selected from AddrMan. */ + MANUAL, /**< connections to addresses added via addnode or the connect command line argument */ + FEELER, /**< short lived connections used to test address validity */ + BLOCK_RELAY, /**< only relay blocks to these automatic outbound connections. Addresses selected from AddrMan. */ + ADDR_FETCH, /**< short lived connections used to solicit addrs when starting the node without a populated AddrMan */ }; class NetEventsInterface; From d3698b5ee309cf0f0cdfb286d6b30a256d7deae5 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Thu, 30 Apr 2020 11:21:33 -0700 Subject: [PATCH 09/19] [net/refactor] Add connection type as a member var to CNode - Directly maintaining the connection type prevents having to deduce it from several flags. --- src/net.cpp | 1 + src/net.h | 1 + 2 files changed, 2 insertions(+) diff --git a/src/net.cpp b/src/net.cpp index a43d02bbd..42748dfc8 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2749,6 +2749,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn // traffic). id(idIn), nLocalHostNonce(nLocalHostNonceIn), + m_conn_type(conn_type_in), nLocalServices(nLocalServicesIn), nMyStartingHeight(nMyStartingHeightIn) { diff --git a/src/net.h b/src/net.h index 6a4ef6ab4..115effdef 100644 --- a/src/net.h +++ b/src/net.h @@ -875,6 +875,7 @@ public: private: const NodeId id; const uint64_t nLocalHostNonce; + const ConnectionType m_conn_type; //! Services offered to this peer. //! From 49efac5cae7333c6700d9b737d09fae0f3f4d7fa Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Tue, 2 Jun 2020 08:39:47 -0700 Subject: [PATCH 10/19] [net/refactor] Remove m_manual_connection flag from CNode --- src/net.cpp | 9 ++++----- src/net.h | 5 ++++- src/net_processing.cpp | 10 ++++------ 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 42748dfc8..4270b16d5 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -539,7 +539,7 @@ void CNode::copyStats(CNodeStats &stats, const std::vector &m_asmap) X(cleanSubVer); } X(fInbound); - X(m_manual_connection); + stats.m_manual_connection = IsManualConn(); X(nStartingHeight); { LOCK(cs_vSend); @@ -1648,7 +1648,7 @@ void CConnman::ThreadDNSAddressSeed() { LOCK(cs_vNodes); for (const CNode* pnode : vNodes) { - nRelevant += pnode->fSuccessfullyConnected && !pnode->fFeeler && !pnode->m_addr_fetch && !pnode->m_manual_connection && !pnode->fInbound; + nRelevant += pnode->fSuccessfullyConnected && !pnode->fFeeler && !pnode->m_addr_fetch && !pnode->IsManualConn() && !pnode->fInbound; } } if (nRelevant >= 2) { @@ -1758,7 +1758,7 @@ int CConnman::GetExtraOutboundCount() { LOCK(cs_vNodes); for (const CNode* pnode : vNodes) { - if (!pnode->fInbound && !pnode->m_manual_connection && !pnode->fFeeler && !pnode->fDisconnect && !pnode->m_addr_fetch && pnode->fSuccessfullyConnected) { + if (!pnode->fInbound && !pnode->IsManualConn() && !pnode->fFeeler && !pnode->fDisconnect && !pnode->m_addr_fetch && pnode->fSuccessfullyConnected) { ++nOutbound; } } @@ -1832,7 +1832,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect) { LOCK(cs_vNodes); for (const CNode* pnode : vNodes) { - if (!pnode->fInbound && !pnode->m_manual_connection) { + if (!pnode->fInbound && (pnode->m_conn_type != ConnectionType::MANUAL)) { // Netgroups for inbound and addnode peers are not excluded because our goal here // is to not use multiple of our limited outbound slots on a single netgroup // but inbound and addnode peers do not use our outbound slots. Inbound peers @@ -2741,7 +2741,6 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn addrBind(addrBindIn), fFeeler(conn_type_in == ConnectionType::FEELER), m_addr_fetch(conn_type_in == ConnectionType::ADDR_FETCH), - m_manual_connection(conn_type_in == ConnectionType::MANUAL), fInbound(conn_type_in == ConnectionType::INBOUND), nKeyedNetGroup(nKeyedNetGroupIn), // Don't relay addr messages to peers that we connect to as block-relay-only diff --git a/src/net.h b/src/net.h index 115effdef..0c6e93977 100644 --- a/src/net.h +++ b/src/net.h @@ -777,7 +777,6 @@ public: bool m_legacyWhitelisted{false}; bool fFeeler{false}; // If true this node is being used as a short lived feeler. bool m_addr_fetch{false}; - bool m_manual_connection{false}; bool fClient{false}; // set by version message bool m_limited_node{false}; //after BIP159, set by version message const bool fInbound; @@ -793,6 +792,10 @@ public: std::atomic_bool fPauseRecv{false}; std::atomic_bool fPauseSend{false}; + bool IsManualConn() const { + return m_conn_type == ConnectionType::MANUAL; + } + protected: mapMsgCmdSize mapSendBytesPerMsgCmd; mapMsgCmdSize mapRecvBytesPerMsgCmd GUARDED_BY(cs_vRecv); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index ba30f7a7b..62886064e 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -827,11 +827,9 @@ void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds) if (state) state->m_last_block_announcement = time_in_seconds; } -// Returns true for outbound peers, excluding manual connections, feelers, and -// one-shots. static bool IsOutboundDisconnectionCandidate(const CNode& node) { - return !(node.fInbound || node.m_manual_connection || node.fFeeler || node.m_addr_fetch); + return !(node.fInbound || node.IsManualConn() || node.fFeeler || node.m_addr_fetch); } void PeerLogicValidation::InitializeNode(CNode *pnode) { @@ -840,7 +838,7 @@ void PeerLogicValidation::InitializeNode(CNode *pnode) { NodeId nodeid = pnode->GetId(); { LOCK(cs_main); - mapNodeState.emplace_hint(mapNodeState.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(addr, std::move(addrName), pnode->fInbound, pnode->m_manual_connection)); + mapNodeState.emplace_hint(mapNodeState.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(addr, std::move(addrName), pnode->fInbound, pnode->IsManualConn())); } if(!pnode->fInbound) PushNodeVersion(*pnode, *connman, GetTime()); @@ -2326,7 +2324,7 @@ void ProcessMessage( { connman.SetServices(pfrom.addr, nServices); } - if (!pfrom.fInbound && !pfrom.fFeeler && !pfrom.m_manual_connection && !HasAllDesirableServiceFlags(nServices)) + if (!pfrom.fInbound && !pfrom.fFeeler && !pfrom.IsManualConn() && !HasAllDesirableServiceFlags(nServices)) { LogPrint(BCLog::NET, "peer=%d does not offer the expected services (%08x offered, %08x expected); disconnecting\n", pfrom.GetId(), nServices, GetDesirableServiceFlags(nServices)); pfrom.fDisconnect = true; @@ -3736,7 +3734,7 @@ bool PeerLogicValidation::MaybeDiscourageAndDisconnect(CNode& pnode) return false; } - if (pnode.m_manual_connection) { + if (pnode.IsManualConn()) { // We never disconnect or discourage manual peers for bad behavior LogPrintf("Warning: not punishing manually connected peer %d!\n", peer_id); return false; From 14923422b08ac4b21b35c426bf0e1b9e7c97983b Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Tue, 12 May 2020 12:58:41 -0700 Subject: [PATCH 11/19] [net/refactor] Remove fFeeler flag from CNode --- src/net.cpp | 7 +++---- src/net.h | 5 ++++- src/net_processing.cpp | 7 +++---- src/test/net_tests.cpp | 3 --- 4 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 4270b16d5..047153972 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1648,7 +1648,7 @@ void CConnman::ThreadDNSAddressSeed() { LOCK(cs_vNodes); for (const CNode* pnode : vNodes) { - nRelevant += pnode->fSuccessfullyConnected && !pnode->fFeeler && !pnode->m_addr_fetch && !pnode->IsManualConn() && !pnode->fInbound; + nRelevant += pnode->fSuccessfullyConnected && !pnode->IsFeelerConn() && !pnode->m_addr_fetch && !pnode->IsManualConn() && !pnode->fInbound; } } if (nRelevant >= 2) { @@ -1758,7 +1758,7 @@ int CConnman::GetExtraOutboundCount() { LOCK(cs_vNodes); for (const CNode* pnode : vNodes) { - if (!pnode->fInbound && !pnode->IsManualConn() && !pnode->fFeeler && !pnode->fDisconnect && !pnode->m_addr_fetch && pnode->fSuccessfullyConnected) { + if (!pnode->fInbound && !pnode->IsManualConn() && !pnode->IsFeelerConn() && !pnode->fDisconnect && !pnode->m_addr_fetch && pnode->fSuccessfullyConnected) { ++nOutbound; } } @@ -1841,7 +1841,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect) setConnected.insert(pnode->addr.GetGroup(addrman.m_asmap)); if (pnode->m_tx_relay == nullptr) { nOutboundBlockRelay++; - } else if (!pnode->fFeeler) { + } else if (!pnode->IsFeelerConn()) { nOutboundFullRelay++; } } @@ -2739,7 +2739,6 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn : nTimeConnected(GetSystemTimeInSeconds()), addr(addrIn), addrBind(addrBindIn), - fFeeler(conn_type_in == ConnectionType::FEELER), m_addr_fetch(conn_type_in == ConnectionType::ADDR_FETCH), fInbound(conn_type_in == ConnectionType::INBOUND), nKeyedNetGroup(nKeyedNetGroupIn), diff --git a/src/net.h b/src/net.h index 0c6e93977..f4fd33ec9 100644 --- a/src/net.h +++ b/src/net.h @@ -775,7 +775,6 @@ public: } // This boolean is unusued in actual processing, only present for backward compatibility at RPC/QT level bool m_legacyWhitelisted{false}; - bool fFeeler{false}; // If true this node is being used as a short lived feeler. bool m_addr_fetch{false}; bool fClient{false}; // set by version message bool m_limited_node{false}; //after BIP159, set by version message @@ -796,6 +795,10 @@ public: return m_conn_type == ConnectionType::MANUAL; } + bool IsFeelerConn() const { + return m_conn_type == ConnectionType::FEELER; + } + protected: mapMsgCmdSize mapSendBytesPerMsgCmd; mapMsgCmdSize mapRecvBytesPerMsgCmd GUARDED_BY(cs_vRecv); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 62886064e..53d47b63b 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -829,7 +829,7 @@ void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds) static bool IsOutboundDisconnectionCandidate(const CNode& node) { - return !(node.fInbound || node.IsManualConn() || node.fFeeler || node.m_addr_fetch); + return !(node.fInbound || node.IsManualConn() || node.IsFeelerConn() || node.m_addr_fetch); } void PeerLogicValidation::InitializeNode(CNode *pnode) { @@ -2324,7 +2324,7 @@ void ProcessMessage( { connman.SetServices(pfrom.addr, nServices); } - if (!pfrom.fInbound && !pfrom.fFeeler && !pfrom.IsManualConn() && !HasAllDesirableServiceFlags(nServices)) + if (!pfrom.fInbound && !pfrom.IsFeelerConn() && !pfrom.IsManualConn() && !HasAllDesirableServiceFlags(nServices)) { LogPrint(BCLog::NET, "peer=%d does not offer the expected services (%08x offered, %08x expected); disconnecting\n", pfrom.GetId(), nServices, GetDesirableServiceFlags(nServices)); pfrom.fDisconnect = true; @@ -2452,8 +2452,7 @@ void ProcessMessage( } // Feeler connections exist only to verify if address is online. - if (pfrom.fFeeler) { - assert(pfrom.fInbound == false); + if (pfrom.IsFeelerConn()) { pfrom.fDisconnect = true; } return; diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index 5840f5a72..34be51d5a 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -181,14 +181,11 @@ BOOST_AUTO_TEST_CASE(cnode_simple_test) CAddress addr = CAddress(CService(ipv4Addr, 7777), NODE_NETWORK); std::string pszDest; - // Test that fFeeler is false by default. std::unique_ptr pnode1 = MakeUnique(id++, NODE_NETWORK, height, hSocket, addr, 0, 0, CAddress(), pszDest, ConnectionType::OUTBOUND); BOOST_CHECK(pnode1->fInbound == false); - BOOST_CHECK(pnode1->fFeeler == false); std::unique_ptr pnode2 = MakeUnique(id++, NODE_NETWORK, height, hSocket, addr, 1, 1, CAddress(), pszDest, ConnectionType::INBOUND); BOOST_CHECK(pnode2->fInbound == true); - BOOST_CHECK(pnode2->fFeeler == false); } // prior to PR #14728, this test triggers an undefined behavior From 7b322df6296609570e368e5f326979279041c11f Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Tue, 28 Jul 2020 13:17:16 -0700 Subject: [PATCH 12/19] [net/refactor] Remove m_addr_fetch member var from CNode --- src/net.cpp | 5 ++--- src/net.h | 5 ++++- src/net_processing.cpp | 8 ++++---- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 047153972..750914064 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1648,7 +1648,7 @@ void CConnman::ThreadDNSAddressSeed() { LOCK(cs_vNodes); for (const CNode* pnode : vNodes) { - nRelevant += pnode->fSuccessfullyConnected && !pnode->IsFeelerConn() && !pnode->m_addr_fetch && !pnode->IsManualConn() && !pnode->fInbound; + nRelevant += pnode->fSuccessfullyConnected && !pnode->IsFeelerConn() && !pnode->IsAddrFetchConn() && !pnode->IsManualConn() && !pnode->fInbound; } } if (nRelevant >= 2) { @@ -1758,7 +1758,7 @@ int CConnman::GetExtraOutboundCount() { LOCK(cs_vNodes); for (const CNode* pnode : vNodes) { - if (!pnode->fInbound && !pnode->IsManualConn() && !pnode->IsFeelerConn() && !pnode->fDisconnect && !pnode->m_addr_fetch && pnode->fSuccessfullyConnected) { + if (!pnode->fInbound && !pnode->IsManualConn() && !pnode->IsFeelerConn() && !pnode->fDisconnect && !pnode->IsAddrFetchConn() && pnode->fSuccessfullyConnected) { ++nOutbound; } } @@ -2739,7 +2739,6 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn : nTimeConnected(GetSystemTimeInSeconds()), addr(addrIn), addrBind(addrBindIn), - m_addr_fetch(conn_type_in == ConnectionType::ADDR_FETCH), fInbound(conn_type_in == ConnectionType::INBOUND), nKeyedNetGroup(nKeyedNetGroupIn), // Don't relay addr messages to peers that we connect to as block-relay-only diff --git a/src/net.h b/src/net.h index f4fd33ec9..74f96d3a3 100644 --- a/src/net.h +++ b/src/net.h @@ -775,7 +775,6 @@ public: } // This boolean is unusued in actual processing, only present for backward compatibility at RPC/QT level bool m_legacyWhitelisted{false}; - bool m_addr_fetch{false}; bool fClient{false}; // set by version message bool m_limited_node{false}; //after BIP159, set by version message const bool fInbound; @@ -799,6 +798,10 @@ public: return m_conn_type == ConnectionType::FEELER; } + bool IsAddrFetchConn() const { + return m_conn_type == ConnectionType::ADDR_FETCH; + } + protected: mapMsgCmdSize mapSendBytesPerMsgCmd; mapMsgCmdSize mapRecvBytesPerMsgCmd GUARDED_BY(cs_vRecv); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 53d47b63b..6a78924fe 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -473,7 +473,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.fInbound || node.HasPermission(PF_NOBAN)) && !node.m_addr_fetch && !node.fClient; + state->fPreferredDownload = (!node.fInbound || node.HasPermission(PF_NOBAN)) && !node.IsAddrFetchConn() && !node.fClient; nPreferredDownload += state->fPreferredDownload; } @@ -829,7 +829,7 @@ void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds) static bool IsOutboundDisconnectionCandidate(const CNode& node) { - return !(node.fInbound || node.IsManualConn() || node.IsFeelerConn() || node.m_addr_fetch); + return !(node.fInbound || node.IsManualConn() || node.IsFeelerConn() || node.IsAddrFetchConn()); } void PeerLogicValidation::InitializeNode(CNode *pnode) { @@ -2581,7 +2581,7 @@ void ProcessMessage( connman.AddNewAddresses(vAddrOk, pfrom.addr, 2 * 60 * 60); if (vAddr.size() < 1000) pfrom.fGetAddr = false; - if (pfrom.m_addr_fetch) + if (pfrom.IsAddrFetchConn()) pfrom.fDisconnect = true; return; } @@ -4094,7 +4094,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto) // Start block sync if (pindexBestHeader == nullptr) pindexBestHeader = ::ChainActive().Tip(); - bool fFetch = state.fPreferredDownload || (nPreferredDownload == 0 && !pto->fClient && !pto->m_addr_fetch); // Download if this is a nice peer, or we have no nice peers and this one might do. + bool fFetch = state.fPreferredDownload || (nPreferredDownload == 0 && !pto->fClient && !pto->IsAddrFetchConn()); // Download if this is a nice peer, or we have no nice peers and this one might do. if (!state.fSyncStarted && !pto->fClient && !fImporting && !fReindex) { // Only actively request headers from a single peer, unless we're close to today. if ((nSyncStarted == 0 && fFetch) || pindexBestHeader->GetBlockTime() > GetAdjustedTime() - 24 * 60 * 60) { From 60156f5fc40d56bb532278f16ce632c5a8b8035e Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Tue, 28 Jul 2020 13:39:38 -0700 Subject: [PATCH 13/19] [net/refactor] Remove fInbound flag from CNode --- src/net.cpp | 21 ++++++++++----------- src/net.h | 5 ++++- src/net_processing.cpp | 26 +++++++++++++------------- src/test/net_tests.cpp | 4 ++-- 4 files changed, 29 insertions(+), 27 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 750914064..9d33bed2f 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -346,7 +346,7 @@ bool CConnman::CheckIncomingNonce(uint64_t nonce) { LOCK(cs_vNodes); for (const CNode* pnode : vNodes) { - if (!pnode->fSuccessfullyConnected && !pnode->fInbound && pnode->GetLocalNonce() == nonce) + if (!pnode->fSuccessfullyConnected && !pnode->IsInboundConn() && pnode->GetLocalNonce() == nonce) return false; } return true; @@ -538,7 +538,7 @@ void CNode::copyStats(CNodeStats &stats, const std::vector &m_asmap) LOCK(cs_SubVer); X(cleanSubVer); } - X(fInbound); + stats.fInbound = IsInboundConn(); stats.m_manual_connection = IsManualConn(); X(nStartingHeight); { @@ -874,7 +874,7 @@ bool CConnman::AttemptToEvictConnection() for (const CNode* node : vNodes) { if (node->HasPermission(PF_NOBAN)) continue; - if (!node->fInbound) + if (!node->IsInboundConn()) continue; if (node->fDisconnect) continue; @@ -985,7 +985,7 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) { { LOCK(cs_vNodes); for (const CNode* pnode : vNodes) { - if (pnode->fInbound) nInbound++; + if (pnode->IsInboundConn()) nInbound++; } } @@ -1648,7 +1648,7 @@ void CConnman::ThreadDNSAddressSeed() { LOCK(cs_vNodes); for (const CNode* pnode : vNodes) { - nRelevant += pnode->fSuccessfullyConnected && !pnode->IsFeelerConn() && !pnode->IsAddrFetchConn() && !pnode->IsManualConn() && !pnode->fInbound; + nRelevant += pnode->fSuccessfullyConnected && !pnode->IsFeelerConn() && !pnode->IsAddrFetchConn() && !pnode->IsManualConn() && !pnode->IsInboundConn(); } } if (nRelevant >= 2) { @@ -1758,7 +1758,7 @@ int CConnman::GetExtraOutboundCount() { LOCK(cs_vNodes); for (const CNode* pnode : vNodes) { - if (!pnode->fInbound && !pnode->IsManualConn() && !pnode->IsFeelerConn() && !pnode->fDisconnect && !pnode->IsAddrFetchConn() && pnode->fSuccessfullyConnected) { + if (!pnode->IsInboundConn() && !pnode->IsManualConn() && !pnode->IsFeelerConn() && !pnode->fDisconnect && !pnode->IsAddrFetchConn() && pnode->fSuccessfullyConnected) { ++nOutbound; } } @@ -1832,7 +1832,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect) { LOCK(cs_vNodes); for (const CNode* pnode : vNodes) { - if (!pnode->fInbound && (pnode->m_conn_type != ConnectionType::MANUAL)) { + if (!pnode->IsInboundConn() && (pnode->m_conn_type != ConnectionType::MANUAL)) { // Netgroups for inbound and addnode peers are not excluded because our goal here // is to not use multiple of our limited outbound slots on a single netgroup // but inbound and addnode peers do not use our outbound slots. Inbound peers @@ -1972,11 +1972,11 @@ std::vector CConnman::GetAddedNodeInfo() LOCK(cs_vNodes); for (const CNode* pnode : vNodes) { if (pnode->addr.IsValid()) { - mapConnected[pnode->addr] = pnode->fInbound; + mapConnected[pnode->addr] = pnode->IsInboundConn(); } std::string addrName = pnode->GetAddrName(); if (!addrName.empty()) { - mapConnectedByName[std::move(addrName)] = std::make_pair(pnode->fInbound, static_cast(pnode->addr)); + mapConnectedByName[std::move(addrName)] = std::make_pair(pnode->IsInboundConn(), static_cast(pnode->addr)); } } } @@ -2551,7 +2551,7 @@ size_t CConnman::GetNodeCount(NumConnections flags) int nNum = 0; for (const auto& pnode : vNodes) { - if (flags & (pnode->fInbound ? CONNECTIONS_IN : CONNECTIONS_OUT)) { + if (flags & (pnode->IsInboundConn() ? CONNECTIONS_IN : CONNECTIONS_OUT)) { nNum++; } } @@ -2739,7 +2739,6 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn : nTimeConnected(GetSystemTimeInSeconds()), addr(addrIn), addrBind(addrBindIn), - fInbound(conn_type_in == ConnectionType::INBOUND), nKeyedNetGroup(nKeyedNetGroupIn), // 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 diff --git a/src/net.h b/src/net.h index 74f96d3a3..f52fea72b 100644 --- a/src/net.h +++ b/src/net.h @@ -777,7 +777,6 @@ public: bool m_legacyWhitelisted{false}; bool fClient{false}; // set by version message bool m_limited_node{false}; //after BIP159, set by version message - const bool fInbound; std::atomic_bool fSuccessfullyConnected{false}; // Setting fDisconnect to true will cause the node to be disconnected the // next time DisconnectNodes() runs @@ -802,6 +801,10 @@ public: return m_conn_type == ConnectionType::ADDR_FETCH; } + bool IsInboundConn() const { + return m_conn_type == ConnectionType::INBOUND; + } + protected: mapMsgCmdSize mapSendBytesPerMsgCmd; mapMsgCmdSize mapRecvBytesPerMsgCmd GUARDED_BY(cs_vRecv); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 6a78924fe..da2fa8026 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -473,7 +473,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.fInbound || node.HasPermission(PF_NOBAN)) && !node.IsAddrFetchConn() && !node.fClient; + state->fPreferredDownload = (!node.IsInboundConn() || node.HasPermission(PF_NOBAN)) && !node.IsAddrFetchConn() && !node.fClient; nPreferredDownload += state->fPreferredDownload; } @@ -829,7 +829,7 @@ void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds) static bool IsOutboundDisconnectionCandidate(const CNode& node) { - return !(node.fInbound || node.IsManualConn() || node.IsFeelerConn() || node.IsAddrFetchConn()); + return !(node.IsInboundConn() || node.IsManualConn() || node.IsFeelerConn() || node.IsAddrFetchConn()); } void PeerLogicValidation::InitializeNode(CNode *pnode) { @@ -838,9 +838,9 @@ void PeerLogicValidation::InitializeNode(CNode *pnode) { NodeId nodeid = pnode->GetId(); { LOCK(cs_main); - mapNodeState.emplace_hint(mapNodeState.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(addr, std::move(addrName), pnode->fInbound, pnode->IsManualConn())); + mapNodeState.emplace_hint(mapNodeState.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(addr, std::move(addrName), pnode->IsInboundConn(), pnode->IsManualConn())); } - if(!pnode->fInbound) + if(!pnode->IsInboundConn()) PushNodeVersion(*pnode, *connman, GetTime()); } @@ -2320,11 +2320,11 @@ void ProcessMessage( vRecv >> nVersion >> nServiceInt >> nTime >> addrMe; nSendVersion = std::min(nVersion, PROTOCOL_VERSION); nServices = ServiceFlags(nServiceInt); - if (!pfrom.fInbound) + if (!pfrom.IsInboundConn()) { connman.SetServices(pfrom.addr, nServices); } - if (!pfrom.fInbound && !pfrom.IsFeelerConn() && !pfrom.IsManualConn() && !HasAllDesirableServiceFlags(nServices)) + if (!pfrom.IsInboundConn() && !pfrom.IsFeelerConn() && !pfrom.IsManualConn() && !HasAllDesirableServiceFlags(nServices)) { LogPrint(BCLog::NET, "peer=%d does not offer the expected services (%08x offered, %08x expected); disconnecting\n", pfrom.GetId(), nServices, GetDesirableServiceFlags(nServices)); pfrom.fDisconnect = true; @@ -2351,20 +2351,20 @@ void ProcessMessage( if (!vRecv.empty()) vRecv >> fRelay; // Disconnect if we connected to ourself - if (pfrom.fInbound && !connman.CheckIncomingNonce(nNonce)) + if (pfrom.IsInboundConn() && !connman.CheckIncomingNonce(nNonce)) { LogPrintf("connected to self at %s, disconnecting\n", pfrom.addr.ToString()); pfrom.fDisconnect = true; return; } - if (pfrom.fInbound && addrMe.IsRoutable()) + if (pfrom.IsInboundConn() && addrMe.IsRoutable()) { SeenLocal(addrMe); } // Be shy and don't send version until we hear - if (pfrom.fInbound) + if (pfrom.IsInboundConn()) PushNodeVersion(pfrom, connman, GetAdjustedTime()); if (nVersion >= WTXID_RELAY_VERSION) { @@ -2408,7 +2408,7 @@ void ProcessMessage( UpdatePreferredDownload(pfrom, State(pfrom.GetId())); } - if (!pfrom.fInbound && pfrom.IsAddrRelayPeer()) + if (!pfrom.IsInboundConn() && pfrom.IsAddrRelayPeer()) { // Advertise our address if (fListen && !::ChainstateActive().IsInitialBlockDownload()) @@ -2472,7 +2472,7 @@ void ProcessMessage( { pfrom.SetRecvVersion(std::min(pfrom.nVersion.load(), PROTOCOL_VERSION)); - if (!pfrom.fInbound) { + if (!pfrom.IsInboundConn()) { // Mark this node as currently connected, so we update its timestamp later. LOCK(cs_main); State(pfrom.GetId())->fCurrentlyConnected = true; @@ -3452,7 +3452,7 @@ void ProcessMessage( // to users' AddrMan and later request them by sending getaddr messages. // Making nodes which are behind NAT and can only make outgoing connections ignore // the getaddr message mitigates the attack. - if (!pfrom.fInbound) { + if (!pfrom.IsInboundConn()) { LogPrint(BCLog::NET, "Ignoring \"getaddr\" from outbound connection. peer=%d\n", pfrom.GetId()); return; } @@ -4279,7 +4279,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto) bool fSendTrickle = pto->HasPermission(PF_NOBAN); if (pto->m_tx_relay->nNextInvSend < current_time) { fSendTrickle = true; - if (pto->fInbound) { + if (pto->IsInboundConn()) { pto->m_tx_relay->nNextInvSend = std::chrono::microseconds{connman->PoissonNextSendInbound(nNow, INVENTORY_BROADCAST_INTERVAL)}; } else { // Use half the delay for outbound peers, as there is less privacy concern for them. diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index 34be51d5a..317000c77 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -182,10 +182,10 @@ BOOST_AUTO_TEST_CASE(cnode_simple_test) std::string pszDest; std::unique_ptr pnode1 = MakeUnique(id++, NODE_NETWORK, height, hSocket, addr, 0, 0, CAddress(), pszDest, ConnectionType::OUTBOUND); - BOOST_CHECK(pnode1->fInbound == false); + BOOST_CHECK(pnode1->IsInboundConn() == false); std::unique_ptr pnode2 = MakeUnique(id++, NODE_NETWORK, height, hSocket, addr, 1, 1, CAddress(), pszDest, ConnectionType::INBOUND); - BOOST_CHECK(pnode2->fInbound == true); + BOOST_CHECK(pnode2->IsInboundConn() == true); } // prior to PR #14728, this test triggers an undefined behavior From 4972c21b671ff73f13a1b5053338b6abbdb471b5 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Wed, 20 May 2020 14:16:24 -0700 Subject: [PATCH 14/19] [net/refactor] Clarify logic for selecting connections in ThreadOpenConnections --- src/net.cpp | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 9d33bed2f..67db02d0a 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1936,15 +1936,20 @@ void CConnman::ThreadOpenConnections(const std::vector connect) LogPrint(BCLog::NET, "Making feeler connection to %s\n", addrConnect.ToString()); } - // Open this connection as block-relay-only if we're already at our - // full-relay capacity, but not yet at our block-relay peer limit. - bool block_relay_only = nOutboundBlockRelay < m_max_outbound_block_relay && nOutboundFullRelay >= m_max_outbound_full_relay; ConnectionType conn_type; - if(fFeeler) { + // Determine what type of connection to open. If fFeeler is not + // set, open OUTBOUND connections until we meet our full-relay + // capacity. Then open BLOCK_RELAY connections until we hit our + // block-relay peer limit. Otherwise, default to opening an + // OUTBOUND connection. + if (fFeeler) { conn_type = ConnectionType::FEELER; - } else if (block_relay_only) { + } else if (nOutboundFullRelay < m_max_outbound_full_relay) { + conn_type = ConnectionType::OUTBOUND; + } else if (nOutboundBlockRelay < m_max_outbound_block_relay) { conn_type = ConnectionType::BLOCK_RELAY; } else { + // GetTryNewOutboundPeer() is true conn_type = ConnectionType::OUTBOUND; } From 35839e963bf61d2da0d12f5b8cea74ac0e0fbd7b Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Tue, 2 Jun 2020 09:01:36 -0700 Subject: [PATCH 15/19] [net] Fix bug where AddrFetch connections would be counted as outbound full relay The desired logic is for us to only open feeler connections after we have hit the max count for outbound full relay connections. A short lived AddrFetch connection (previously called oneshot) could cause ThreadOpenConnections to miscount and mistakenly open a feeler instead of full relay. --- src/net.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/net.cpp b/src/net.cpp index 67db02d0a..f2dcec784 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1841,7 +1841,7 @@ void CConnman::ThreadOpenConnections(const std::vector connect) setConnected.insert(pnode->addr.GetGroup(addrman.m_asmap)); if (pnode->m_tx_relay == nullptr) { nOutboundBlockRelay++; - } else if (!pnode->IsFeelerConn()) { + } else if (pnode->m_conn_type == ConnectionType::OUTBOUND) { nOutboundFullRelay++; } } From 7f7b83deb2427599c129f4ff581d4d045461e459 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Tue, 2 Jun 2020 21:23:44 -0700 Subject: [PATCH 16/19] [net/refactor] Rework ThreadOpenConnections logic Make the connection counts explicit and extract into interface functions around m_conn_type. Using explicit counting and switch statements where possible should help prevent counting bugs in the future. --- src/net.cpp | 30 ++++++++++++++++++------------ src/net.h | 23 +++++++++++++++++++++++ 2 files changed, 41 insertions(+), 12 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index f2dcec784..35daddbbe 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1829,21 +1829,27 @@ void CConnman::ThreadOpenConnections(const std::vector connect) int nOutboundFullRelay = 0; int nOutboundBlockRelay = 0; std::set > setConnected; + { LOCK(cs_vNodes); for (const CNode* pnode : vNodes) { - if (!pnode->IsInboundConn() && (pnode->m_conn_type != ConnectionType::MANUAL)) { - // Netgroups for inbound and addnode peers are not excluded because our goal here - // is to not use multiple of our limited outbound slots on a single netgroup - // but inbound and addnode peers do not use our outbound slots. Inbound peers - // also have the added issue that they're attacker controlled and could be used - // to prevent us from connecting to particular hosts if we used them here. - setConnected.insert(pnode->addr.GetGroup(addrman.m_asmap)); - if (pnode->m_tx_relay == nullptr) { - nOutboundBlockRelay++; - } else if (pnode->m_conn_type == ConnectionType::OUTBOUND) { - nOutboundFullRelay++; - } + if (pnode->IsFullOutboundConn()) nOutboundFullRelay++; + if (pnode->IsBlockOnlyConn()) nOutboundBlockRelay++; + + // Netgroups for inbound and manual peers are not excluded because our goal here + // is to not use multiple of our limited outbound slots on a single netgroup + // but inbound and manual peers do not use our outbound slots. Inbound peers + // also have the added issue that they could be attacker controlled and used + // to prevent us from connecting to particular hosts if we used them here. + switch(pnode->m_conn_type){ + case ConnectionType::INBOUND: + case ConnectionType::MANUAL: + break; + case ConnectionType::OUTBOUND: + case ConnectionType::BLOCK_RELAY: + case ConnectionType::ADDR_FETCH: + case ConnectionType::FEELER: + setConnected.insert(pnode->addr.GetGroup(addrman.m_asmap)); } } } diff --git a/src/net.h b/src/net.h index f52fea72b..483405012 100644 --- a/src/net.h +++ b/src/net.h @@ -789,10 +789,18 @@ public: std::atomic_bool fPauseRecv{false}; std::atomic_bool fPauseSend{false}; + bool IsFullOutboundConn() const { + return m_conn_type == ConnectionType::OUTBOUND; + } + bool IsManualConn() const { return m_conn_type == ConnectionType::MANUAL; } + bool IsBlockOnlyConn() const { + return m_conn_type == ConnectionType::BLOCK_RELAY; + } + bool IsFeelerConn() const { return m_conn_type == ConnectionType::FEELER; } @@ -805,6 +813,21 @@ public: return m_conn_type == ConnectionType::INBOUND; } + bool ExpectServicesFromConn() const { + switch(m_conn_type) { + case ConnectionType::INBOUND: + case ConnectionType::MANUAL: + case ConnectionType::FEELER: + return false; + case ConnectionType::OUTBOUND: + case ConnectionType::BLOCK_RELAY: + case ConnectionType::ADDR_FETCH: + return true; + } + + assert(false); + } + protected: mapMsgCmdSize mapSendBytesPerMsgCmd; mapMsgCmdSize mapRecvBytesPerMsgCmd GUARDED_BY(cs_vRecv); From 2f2e13b6c2c8741ca9d825eaaef736ede484bc85 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Mon, 20 Jul 2020 14:24:48 -0700 Subject: [PATCH 17/19] [net/refactor] Simplify multiple-connection checks Extract logic that check multiple connection types into interface functions & structure as switch statements. This makes it very clear what touch points are for accessing `m_conn_type` & using the switch statements enables the compiler to warn if a new connection type is introduced but not handled for these cases. --- src/net.cpp | 4 ++-- src/net.h | 15 +++++++++++++++ src/net_processing.cpp | 4 ++-- 3 files changed, 19 insertions(+), 4 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 35daddbbe..539ba9f40 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1648,7 +1648,7 @@ void CConnman::ThreadDNSAddressSeed() { LOCK(cs_vNodes); for (const CNode* pnode : vNodes) { - nRelevant += pnode->fSuccessfullyConnected && !pnode->IsFeelerConn() && !pnode->IsAddrFetchConn() && !pnode->IsManualConn() && !pnode->IsInboundConn(); + if (pnode->fSuccessfullyConnected && pnode->IsOutboundOrBlockRelayConn()) ++nRelevant; } } if (nRelevant >= 2) { @@ -1758,7 +1758,7 @@ int CConnman::GetExtraOutboundCount() { LOCK(cs_vNodes); for (const CNode* pnode : vNodes) { - if (!pnode->IsInboundConn() && !pnode->IsManualConn() && !pnode->IsFeelerConn() && !pnode->fDisconnect && !pnode->IsAddrFetchConn() && pnode->fSuccessfullyConnected) { + if (pnode->fSuccessfullyConnected && !pnode->fDisconnect && pnode->IsOutboundOrBlockRelayConn()) { ++nOutbound; } } diff --git a/src/net.h b/src/net.h index 483405012..444a1ceed 100644 --- a/src/net.h +++ b/src/net.h @@ -789,6 +789,21 @@ public: std::atomic_bool fPauseRecv{false}; std::atomic_bool fPauseSend{false}; + bool IsOutboundOrBlockRelayConn() const { + switch(m_conn_type) { + case ConnectionType::OUTBOUND: + case ConnectionType::BLOCK_RELAY: + return true; + case ConnectionType::INBOUND: + case ConnectionType::MANUAL: + case ConnectionType::ADDR_FETCH: + case ConnectionType::FEELER: + return false; + } + + assert(false); + } + bool IsFullOutboundConn() const { return m_conn_type == ConnectionType::OUTBOUND; } diff --git a/src/net_processing.cpp b/src/net_processing.cpp index da2fa8026..f90ae658e 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -829,7 +829,7 @@ void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds) static bool IsOutboundDisconnectionCandidate(const CNode& node) { - return !(node.IsInboundConn() || node.IsManualConn() || node.IsFeelerConn() || node.IsAddrFetchConn()); + return node.IsOutboundOrBlockRelayConn(); } void PeerLogicValidation::InitializeNode(CNode *pnode) { @@ -2324,7 +2324,7 @@ void ProcessMessage( { connman.SetServices(pfrom.addr, nServices); } - if (!pfrom.IsInboundConn() && !pfrom.IsFeelerConn() && !pfrom.IsManualConn() && !HasAllDesirableServiceFlags(nServices)) + if (pfrom.ExpectServicesFromConn() && !HasAllDesirableServiceFlags(nServices)) { LogPrint(BCLog::NET, "peer=%d does not offer the expected services (%08x offered, %08x expected); disconnecting\n", pfrom.GetId(), nServices, GetDesirableServiceFlags(nServices)); pfrom.fDisconnect = true; From bc5d65b3ca41eebb1738fdda4451d1466e77772e Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Tue, 21 Jul 2020 16:28:47 -0700 Subject: [PATCH 18/19] [refactor] Remove IsOutboundDisconnectionCandidate --- src/net_processing.cpp | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index f90ae658e..9e60716fa 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -827,11 +827,6 @@ void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds) if (state) state->m_last_block_announcement = time_in_seconds; } -static bool IsOutboundDisconnectionCandidate(const CNode& node) -{ - return node.IsOutboundOrBlockRelayConn(); -} - void PeerLogicValidation::InitializeNode(CNode *pnode) { CAddress addr = pnode->addr; std::string addrName = pnode->GetAddrName(); @@ -1963,14 +1958,14 @@ static void ProcessHeadersMessage(CNode& pfrom, CConnman& connman, ChainstateMan // until we have a headers chain that has at least // nMinimumChainWork, even if a peer has a chain past our tip, // as an anti-DoS measure. - if (IsOutboundDisconnectionCandidate(pfrom)) { + if (pfrom.IsOutboundOrBlockRelayConn()) { LogPrintf("Disconnecting outbound peer %d -- headers chain has insufficient work\n", pfrom.GetId()); pfrom.fDisconnect = true; } } } - if (!pfrom.fDisconnect && IsOutboundDisconnectionCandidate(pfrom) && nodestate->pindexBestKnownBlock != nullptr && pfrom.m_tx_relay != nullptr) { + if (!pfrom.fDisconnect && pfrom.IsOutboundOrBlockRelayConn() && nodestate->pindexBestKnownBlock != nullptr && pfrom.m_tx_relay != nullptr) { // If this is an outbound full-relay peer, check to see if we should protect // it from the bad/lagging chain logic. // Note that block-relay-only peers are already implicitly protected, so we @@ -3854,7 +3849,7 @@ void PeerLogicValidation::ConsiderEviction(CNode& pto, int64_t time_in_seconds) CNodeState &state = *State(pto.GetId()); const CNetMsgMaker msgMaker(pto.GetSendVersion()); - if (!state.m_chain_sync.m_protect && IsOutboundDisconnectionCandidate(pto) && state.fSyncStarted) { + if (!state.m_chain_sync.m_protect && pto.IsOutboundOrBlockRelayConn() && state.fSyncStarted) { // This is an outbound peer subject to disconnection if they don't // announce a block with as much work as the current tip within // CHAIN_SYNC_TIMEOUT + HEADERS_RESPONSE_TIME seconds (note: if @@ -3916,7 +3911,7 @@ void PeerLogicValidation::EvictExtraOutboundPeers(int64_t time_in_seconds) AssertLockHeld(cs_main); // Ignore non-outbound peers, or nodes marked for disconnect already - if (!IsOutboundDisconnectionCandidate(*pnode) || pnode->fDisconnect) return; + if (!pnode->IsOutboundOrBlockRelayConn() || pnode->fDisconnect) return; CNodeState *state = State(pnode->GetId()); if (state == nullptr) return; // shouldn't be possible, but just in case // Don't evict our protected peers From 01e283068b9e6214f2d77a2f772a4244ebfe2274 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Wed, 29 Jul 2020 18:43:28 -0700 Subject: [PATCH 19/19] [net] Remove unnecessary default args on CNode constructor --- src/net.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/net.h b/src/net.h index 444a1ceed..af14ba7f5 100644 --- a/src/net.h +++ b/src/net.h @@ -917,7 +917,7 @@ public: std::set orphan_work_set; - CNode(NodeId id, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn, SOCKET hSocketIn, const CAddress &addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress &addrBindIn, const std::string &addrNameIn = "", ConnectionType conn_type_in = ConnectionType::OUTBOUND); + CNode(NodeId id, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn, SOCKET hSocketIn, const CAddress &addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress &addrBindIn, const std::string &addrNameIn, ConnectionType conn_type_in); ~CNode(); CNode(const CNode&) = delete; CNode& operator=(const CNode&) = delete;