From 2865a2d142f895029329ce2366c8bdb9f9819a04 Mon Sep 17 00:00:00 2001 From: fanquake Date: Wed, 12 Aug 2020 09:41:24 +0800 Subject: [PATCH 1/4] Merge #19316: [net] Cleanup logic around connection types 01e283068b9e6214f2d77a2f772a4244ebfe2274 [net] Remove unnecessary default args on CNode constructor (Amiti Uttarwar) bc5d65b3ca41eebb1738fdda4451d1466e77772e [refactor] Remove IsOutboundDisconnectionCandidate (Amiti Uttarwar) 2f2e13b6c2c8741ca9d825eaaef736ede484bc85 [net/refactor] Simplify multiple-connection checks (Amiti Uttarwar) 7f7b83deb2427599c129f4ff581d4d045461e459 [net/refactor] Rework ThreadOpenConnections logic (Amiti Uttarwar) 35839e963bf61d2da0d12f5b8cea74ac0e0fbd7b [net] Fix bug where AddrFetch connections would be counted as outbound full relay (Amiti Uttarwar) 4972c21b671ff73f13a1b5053338b6abbdb471b5 [net/refactor] Clarify logic for selecting connections in ThreadOpenConnections (Amiti Uttarwar) 60156f5fc40d56bb532278f16ce632c5a8b8035e [net/refactor] Remove fInbound flag from CNode (Amiti Uttarwar) 7b322df6296609570e368e5f326979279041c11f [net/refactor] Remove m_addr_fetch member var from CNode (Amiti Uttarwar) 14923422b08ac4b21b35c426bf0e1b9e7c97983b [net/refactor] Remove fFeeler flag from CNode (Amiti Uttarwar) 49efac5cae7333c6700d9b737d09fae0f3f4d7fa [net/refactor] Remove m_manual_connection flag from CNode (Amiti Uttarwar) d3698b5ee309cf0f0cdfb286d6b30a256d7deae5 [net/refactor] Add connection type as a member var to CNode (Amiti Uttarwar) 46578c03e92a55925308363ccdad04dcfc820d96 [doc] Describe different connection types (Amiti Uttarwar) 442abae2bac7bff85886143df01e14215532b974 [net/refactor] Add AddrFetch connections to ConnectionType enum (Amiti Uttarwar) af59feb05235ecb85ec9d75b09c66e71268c9889 [net/refactor] Extract m_addr_known logic from initializer list (Amiti Uttarwar) e1bc29812ddf1d946bc5acca406a7ed2dca064a6 [net/refactor] Add block relay only connections to ConnectionType enum (Amiti Uttarwar) 0e52a659a2de915fc3dce37fc8fac39be1c8b6fa [net/refactor] Add feeler connections to ConnectionType enum (Amiti Uttarwar) 1521c47438537e192230486dffcec0228a53878d [net/refactor] Add manual connections to ConnectionType enum (Amiti Uttarwar) 26304b4100201754fb32440bec3e3b78cd3f0e6d [net/refactor] Introduce an enum to distinguish type of connection (Amiti Uttarwar) 3f1b7140e95d0f8f958cb35f31c3d964c57e484d scripted-diff: Rename OneShot to AddrFetch (Amiti Uttarwar) Pull request description: **This is part 1 of #19315, which enables the ability to test `outbound` and `block-relay-only` connections from the functional tests.** Please see that PR for more information of overall functionality. **This PR simplifies how we manage different connection types.** It introduces an enum with the various types of connections so we can explicitly define the connection type. The existing system relies on a series of independent flags, then has asserts scattered around to ensure that conflicting flags are not enabled at the same time. I find this approach to be both brittle and confusing. While making these changes, I found a small bug due to the silent assumptions. This PR also proposes a rename from `OneShot` to `AddrFetch`. I find the name `OneShot` to be very confusing, especially when we also have `onetry` manual connections. Everyone I've talked to offline has agreed that the name is confusing, so I propose a potential alternative. I think this is a good opportunity for a rename since I'm creating an enum to explicitly define the connection types. (some context for the unfamiliar: `oneshot` or `addrfetch` connections are short-lived connections created on startup. They connect to the seed peers, send a `getaddr` to solicit addresses, then close the connection.) Overview of this PR: * rename `oneshot` to `addrfetch` * introduce `ConnectionType` enum * one by one, add different connection types to the enum * expose the `conn_type` on CNode, and use this to reduce reliance on flags (& asserts) * fix the bug in counting different type of connections * some additional cleanup to simplify logic and make expectations explicit/inclusive rather than implicit/exclusive. ACKs for top commit: jnewbery: utACK 01e283068b9e6214f2d77a2f772a4244ebfe2274 laanwj: Code review ACK 01e283068b9e6214f2d77a2f772a4244ebfe2274, the commits are pretty straightforward to follow, and I think this is a move in the right direction overall vasild: ACK 01e283068 sdaftuar: ACK 01e283068b9e6214f2d77a2f772a4244ebfe2274. fanquake: ACK 01e283068b9e6214f2d77a2f772a4244ebfe2274 - I don't have as much experience with the networking code but these changes look fairly straight forward, the new code seems more robust/understandable and the additional documentation is great. I'm glad that a followup branch is already underway. There might be some more review comments here later today, so keep an eye on the discussion, however I'm going to merge this now. jb55: wow this code was messy before... ACK 01e283068b9e6214f2d77a2f772a4244ebfe2274 Tree-SHA512: 7bb644a6ed5849913d777ebc2ff89133ca0fbef680355a9a344e07496a979e6f9ff21a958e8eea93dcd7d5c343682b0c7174b1a3de380a4247eaae73da436e15 --- src/chainparams.cpp | 2 +- src/evo/mnauth.cpp | 18 ++-- src/governance/governance.cpp | 2 +- src/masternode/sync.cpp | 6 +- src/masternode/utils.cpp | 8 +- src/net.cpp | 167 +++++++++++++++-------------- src/net.h | 85 ++++++++++++--- src/net_processing.cpp | 56 +++++----- src/rpc/net.cpp | 2 +- src/test/denialofservice_tests.cpp | 12 +-- src/test/fuzz/util.h | 1 + src/test/net_tests.cpp | 15 +-- 12 files changed, 215 insertions(+), 159 deletions(-) diff --git a/src/chainparams.cpp b/src/chainparams.cpp index ef2adfa23acb..72331964d178 100644 --- a/src/chainparams.cpp +++ b/src/chainparams.cpp @@ -252,7 +252,7 @@ class CMainParams : public CChainParams { // 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("dnsseed.dash.org"); diff --git a/src/evo/mnauth.cpp b/src/evo/mnauth.cpp index 48f482189603..de5874b9a938 100644 --- a/src/evo/mnauth.cpp +++ b/src/evo/mnauth.cpp @@ -44,9 +44,9 @@ void CMNAuth::PushMNAUTH(CNode& peer, CConnman& connman, const CBlockIndex* tip) const bool is_basic_scheme_active{DeploymentActiveAfter(tip, Params().GetConsensus(), Consensus::DEPLOYMENT_V19)}; const CBLSPublicKeyVersionWrapper pubKey(*activeMasternodeInfo.blsPubKeyOperator, !is_basic_scheme_active); if (peer.nVersion < MNAUTH_NODE_VER_VERSION || nOurNodeVersion < MNAUTH_NODE_VER_VERSION) { - signHash = ::SerializeHash(std::make_tuple(pubKey, receivedMNAuthChallenge, peer.fInbound)); + signHash = ::SerializeHash(std::make_tuple(pubKey, receivedMNAuthChallenge, peer.IsInboundConn())); } else { - signHash = ::SerializeHash(std::make_tuple(pubKey, receivedMNAuthChallenge, peer.fInbound, nOurNodeVersion)); + signHash = ::SerializeHash(std::make_tuple(pubKey, receivedMNAuthChallenge, peer.IsInboundConn(), nOurNodeVersion)); } CMNAuth mnauth; @@ -111,9 +111,9 @@ void CMNAuth::ProcessMessage(CNode& peer, PeerManager& peerman, CConnman& connma ConstCBLSPublicKeyVersionWrapper pubKey(dmn->pdmnState->pubKeyOperator.Get(), !is_basic_scheme_active); // See comment in PushMNAUTH (fInbound is negated here as we're on the other side of the connection) if (peer.nVersion < MNAUTH_NODE_VER_VERSION || nOurNodeVersion < MNAUTH_NODE_VER_VERSION) { - signHash = ::SerializeHash(std::make_tuple(pubKey, peer.GetSentMNAuthChallenge(), !peer.fInbound)); + signHash = ::SerializeHash(std::make_tuple(pubKey, peer.GetSentMNAuthChallenge(), !peer.IsInboundConn())); } else { - signHash = ::SerializeHash(std::make_tuple(pubKey, peer.GetSentMNAuthChallenge(), !peer.fInbound, peer.nVersion.load())); + signHash = ::SerializeHash(std::make_tuple(pubKey, peer.GetSentMNAuthChallenge(), !peer.IsInboundConn(), peer.nVersion.load())); } LogPrint(BCLog::NET_NETCONN, "CMNAuth::%s -- constructed signHash for nVersion %d, peer=%d\n", __func__, peer.nVersion, peer.GetId()); @@ -124,7 +124,7 @@ void CMNAuth::ProcessMessage(CNode& peer, PeerManager& peerman, CConnman& connma return; } - if (!peer.fInbound) { + if (!peer.IsInboundConn()) { mmetaman->GetMetaInfo(mnauth.proRegTxHash)->SetLastOutboundSuccess(GetTime().count()); if (peer.m_masternode_probe_connection) { LogPrint(BCLog::NET_NETCONN, "CMNAuth::ProcessMessage -- Masternode probe successful for %s, disconnecting. peer=%d\n", @@ -150,18 +150,18 @@ void CMNAuth::ProcessMessage(CNode& peer, PeerManager& peerman, CConnman& connma if (deterministicOutbound == myProTxHash) { // NOTE: do not drop inbound nodes here, mark them as probes so that // they would be disconnected later in CMasternodeUtils::DoMaintenance - if (pnode2->fInbound) { + if (pnode2->IsInboundConn()) { LogPrint(BCLog::NET_NETCONN, "CMNAuth::ProcessMessage -- marking old inbound for dropping it later, peer=%d\n", pnode2->GetId()); pnode2->m_masternode_probe_connection = true; - } else if (peer.fInbound) { + } else if (peer.IsInboundConn()) { LogPrint(BCLog::NET_NETCONN, "CMNAuth::ProcessMessage -- marking new inbound for dropping it later, peer=%d\n", peer.GetId()); peer.m_masternode_probe_connection = true; } } else { - if (!pnode2->fInbound) { + if (!pnode2->IsInboundConn()) { LogPrint(BCLog::NET_NETCONN, "CMNAuth::ProcessMessage -- dropping old outbound, peer=%d\n", pnode2->GetId()); pnode2->fDisconnect = true; - } else if (!peer.fInbound) { + } else if (!peer.IsInboundConn()) { LogPrint(BCLog::NET_NETCONN, "CMNAuth::ProcessMessage -- dropping new outbound, peer=%d\n", peer.GetId()); peer.fDisconnect = true; } diff --git a/src/governance/governance.cpp b/src/governance/governance.cpp index 7ddc736d649c..966b4142a9a4 100644 --- a/src/governance/governance.cpp +++ b/src/governance/governance.cpp @@ -1274,7 +1274,7 @@ int CGovernanceManager::RequestGovernanceObjectVotes(Span vNodesCopy, CC // Don't try to sync any data from outbound non-relay "masternode" connections. // Inbound connection this early is most likely a "masternode" connection // initiated from another node, so skip it too. - if (!pnode->CanRelay() || (fMasternodeMode && pnode->fInbound)) continue; + if (!pnode->CanRelay() || (fMasternodeMode && pnode->IsInboundConn())) continue; // stop early to prevent setAskFor overflow { LOCK(cs_main); diff --git a/src/masternode/sync.cpp b/src/masternode/sync.cpp index c9299119b8b7..aa732beb6e7a 100644 --- a/src/masternode/sync.cpp +++ b/src/masternode/sync.cpp @@ -157,10 +157,10 @@ void CMasternodeSync::ProcessTick() // Don't try to sync any data from outbound non-relay "masternode" connections. // Inbound connection this early is most likely a "masternode" connection // initiated from another node, so skip it too. - if (!pnode->CanRelay() || (fMasternodeMode && pnode->fInbound)) continue; + if (!pnode->CanRelay() || (fMasternodeMode && pnode->IsInboundConn())) continue; { - if ((pnode->HasPermission(PF_NOBAN) || pnode->m_manual_connection) && !netfulfilledman->HasFulfilledRequest(pnode->addr, strAllow)) { + if ((pnode->HasPermission(PF_NOBAN) || pnode->IsManualConn()) && !netfulfilledman->HasFulfilledRequest(pnode->addr, strAllow)) { netfulfilledman->RemoveAllFulfilledRequests(pnode->addr); netfulfilledman->AddFulfilledRequest(pnode->addr, strAllow); LogPrintf("CMasternodeSync::ProcessTick -- skipping mnsync restrictions for peer=%d\n", pnode->GetId()); @@ -203,7 +203,7 @@ void CMasternodeSync::ProcessTick() // Now that the blockchain is synced request the mempool from the connected outbound nodes if possible for (auto pNodeTmp : vNodesCopy) { bool fRequestedEarlier = netfulfilledman->HasFulfilledRequest(pNodeTmp->addr, "mempool-sync"); - if (pNodeTmp->nVersion >= 70216 && !pNodeTmp->fInbound && !fRequestedEarlier && !pNodeTmp->IsBlockRelayOnly()) { + if (pNodeTmp->nVersion >= 70216 && !pNodeTmp->IsInboundConn() && !fRequestedEarlier && !pNodeTmp->IsBlockRelayOnly()) { netfulfilledman->AddFulfilledRequest(pNodeTmp->addr, "mempool-sync"); connman.PushMessage(pNodeTmp, msgMaker.Make(NetMsgType::MEMPOOL)); LogPrint(BCLog::MNSYNC, "CMasternodeSync::ProcessTick -- nTick %d nCurrentAsset %d -- syncing mempool from peer=%d\n", nTick, nCurrentAsset, pNodeTmp->GetId()); diff --git a/src/masternode/utils.cpp b/src/masternode/utils.cpp index 454ed9024340..88c053c1870b 100644 --- a/src/masternode/utils.cpp +++ b/src/masternode/utils.cpp @@ -30,9 +30,9 @@ void CMasternodeUtils::DoMaintenance(CConnman& connman, const CMasternodeSync& m // Don't disconnect masternode connections when we have less then the desired amount of outbound nodes int nonMasternodeCount = 0; connman.ForEachNode(CConnman::AllNodes, [&](CNode* pnode) { - if ((!pnode->fInbound && - !pnode->fFeeler && - !pnode->m_manual_connection && + if ((!pnode->IsInboundConn() && + !pnode->IsFeelerConn() && + !pnode->IsManualConn() && !pnode->m_masternode_connection && !pnode->m_masternode_probe_connection) || @@ -62,7 +62,7 @@ void CMasternodeUtils::DoMaintenance(CConnman& connman, const CMasternodeSync& m return; } // keep _verified_ inbound connections - if (pnode->fInbound) { + if (pnode->IsInboundConn()) { return; } } else if (GetSystemTimeInSeconds() - pnode->nTimeConnected < 5) { diff --git a/src/net.cpp b/src/net.cpp index 507c19c93807..05cca1fe89f7 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -139,10 +139,10 @@ std::map mapLocalHost GUARDED_BY(g_maplocalhost_mute static bool vfLimited[NET_MAX] GUARDED_BY(g_maplocalhost_mutex) = {}; 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() @@ -389,7 +389,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; @@ -411,8 +411,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) { + assert(conn_type != ConnectionType::INBOUND); + if (pszDest == nullptr) { bool fAllowLocal = Params().AllowMultiplePorts() && addrConnect.GetPort() != GetListenPort(); if (!fAllowLocal && IsLocal(addrConnect)) { @@ -494,7 +496,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo if (!sock) { return nullptr; } - connected = ConnectSocketDirectly(addrConnect, *sock, nConnectTimeout, manual_connection); + connected = ConnectSocketDirectly(addrConnect, *sock, 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 @@ -523,7 +525,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo if (!addr_bind.IsValid()) { addr_bind = GetBindAddress(sock->Get()); } - CNode* pnode = new CNode(id, nLocalServices, sock->Release(), addrConnect, CalculateKeyedNetGroup(addrConnect), nonce, addr_bind, pszDest ? pszDest : "", false, block_relay_only); + CNode* pnode = new CNode(id, nLocalServices, sock->Release(), addrConnect, CalculateKeyedNetGroup(addrConnect), nonce, addr_bind, pszDest ? pszDest : "", conn_type); pnode->AddRef(); statsClient.inc("peers.connect", 1.0f); @@ -611,7 +613,7 @@ std::string CNode::GetLogString() const Network CNode::ConnectedThroughNetwork() const { - return fInbound && m_inbound_onion ? NET_ONION : addr.GetNetClass(); + return IsInboundConn() && m_inbound_onion ? NET_ONION : addr.GetNetClass(); } #undef X @@ -640,8 +642,8 @@ void CNode::copyStats(CNodeStats &stats, const std::vector &m_asmap) LOCK(cs_SubVer); X(cleanSubVer); } - X(fInbound); - X(m_manual_connection); + stats.fInbound = IsInboundConn(); + stats.m_manual_connection = IsManualConn(); X(nStartingHeight); { LOCK(cs_vSend); @@ -998,7 +1000,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; @@ -1147,7 +1149,7 @@ void CConnman::CreateNodeFromAcceptedSocket(SOCKET hSocket, { LOCK(cs_vNodes); for (const CNode* pnode : vNodes) { - if (pnode->fInbound) { + if (pnode->IsInboundConn()) { nInbound++; if (!pnode->GetVerifiedProRegTxHash().IsNull()) { nVerifiedInboundMasternodes++; @@ -1231,7 +1233,7 @@ void CConnman::CreateNodeFromAcceptedSocket(SOCKET hSocket, } const bool inbound_onion = std::find(m_onion_binds.begin(), m_onion_binds.end(), addr_bind) != m_onion_binds.end(); - CNode* pnode = new CNode(id, nodeServices, hSocket, addr, CalculateKeyedNetGroup(addr), nonce, addr_bind, "", true, inbound_onion); + CNode* pnode = new CNode(id, nodeServices, hSocket, addr, CalculateKeyedNetGroup(addr), nonce, addr_bind, "", ConnectionType::INBOUND, inbound_onion); 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) @@ -1310,10 +1312,10 @@ void CConnman::DisconnectNodes() if (fLogIPs) { LogPrintf("ThreadSocketHandler -- removing node: peer=%d addr=%s nRefCount=%d fInbound=%d m_masternode_connection=%d m_masternode_iqr_connection=%d\n", - pnode->GetId(), pnode->addr.ToString(), pnode->GetRefCount(), pnode->fInbound, pnode->m_masternode_connection, pnode->m_masternode_iqr_connection); + pnode->GetId(), pnode->addr.ToString(), pnode->GetRefCount(), pnode->IsInboundConn(), pnode->m_masternode_connection, pnode->m_masternode_iqr_connection); } else { LogPrintf("ThreadSocketHandler -- removing node: peer=%d nRefCount=%d fInbound=%d m_masternode_connection=%d m_masternode_iqr_connection=%d\n", - pnode->GetId(), pnode->GetRefCount(), pnode->fInbound, pnode->m_masternode_connection, pnode->m_masternode_iqr_connection); + pnode->GetId(), pnode->GetRefCount(), pnode->IsInboundConn(), pnode->m_masternode_connection, pnode->m_masternode_iqr_connection); } // remove from vNodes @@ -1424,7 +1426,7 @@ void CConnman::CalculateNumConnectionsChangedStats() spvNodes++; else fullNodes++; - if(pnode->fInbound) + if(pnode->IsInboundConn()) inboundNodes++; else outboundNodes++; @@ -2086,7 +2088,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 && !pnode->m_masternode_probe_connection; + if (pnode->fSuccessfullyConnected && !pnode->IsOutboundOrBlockRelayConn() && !pnode->m_masternode_probe_connection) ++nRelevant; } } if (nRelevant >= 2) { @@ -2114,7 +2116,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; @@ -2136,8 +2138,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; @@ -2145,17 +2147,6 @@ void CConnman::ThreadDNSAddressSeed() LogPrintf("%d addresses found from DNS seeds\n", found); } - - - - - - - - - - - void CConnman::DumpAddresses() { int64_t nStart = GetTimeMillis(); @@ -2167,20 +2158,20 @@ 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); if (grant) { - OpenNetworkConnection(addr, false, &grant, strDest.c_str(), true); + OpenNetworkConnection(addr, false, &grant, strDest.c_str(), ConnectionType::ADDR_FETCH); } } @@ -2211,7 +2202,7 @@ int CConnman::GetExtraOutboundCount() if (pnode->m_masternode_connection) { continue; } - if (!pnode->fInbound && !pnode->m_manual_connection && !pnode->fFeeler && !pnode->fDisconnect && !pnode->fOneShot && pnode->fSuccessfullyConnected && !pnode->m_masternode_probe_connection) { + if (pnode->fSuccessfullyConnected && !pnode->fDisconnect && pnode->IsOutboundOrBlockRelayConn() && !pnode->m_masternode_probe_connection) { ++nOutbound; } } @@ -2227,11 +2218,11 @@ 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); - OpenNetworkConnection(addr, false, nullptr, strAddr.c_str(), false, false, true); + 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))) @@ -2250,7 +2241,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; @@ -2287,18 +2278,23 @@ void CConnman::ThreadOpenConnections(const std::vector connect) if (!Params().AllowMultipleAddressesFromGroup()) { LOCK(cs_vNodes); for (const CNode* pnode : vNodes) { - if (!pnode->fInbound && !pnode->m_masternode_connection && !pnode->m_manual_connection) { - // 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->IsAddrRelayPeer()) { - nOutboundBlockRelay++; - } else if (!pnode->fFeeler) { - nOutboundFullRelay++; - } + if (pnode->IsFullOutboundConn() && !pnode->m_masternode_connection) 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)); } } } @@ -2424,14 +2420,24 @@ 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; + ConnectionType conn_type; + // 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 (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; + } - 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, conn_type); } } } @@ -2468,11 +2474,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)); } } } @@ -2519,7 +2525,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(), ConnectionType::MANUAL); if (!interruptNet.sleep_for(std::chrono::milliseconds(500))) return; } @@ -2561,7 +2567,7 @@ void CConnman::ThreadOpenMasternodeConnections() auto verifiedProRegTxHash = pnode->GetVerifiedProRegTxHash(); connectedNodes.emplace(pnode->addr); if (!verifiedProRegTxHash.IsNull()) { - connectedProRegTxHashes.emplace(verifiedProRegTxHash, pnode->fInbound); + connectedProRegTxHashes.emplace(verifiedProRegTxHash, pnode->IsInboundConn()); } }); @@ -2707,8 +2713,10 @@ void CConnman::ThreadOpenMasternodeConnections() } // 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, MasternodeConn masternode_connection, MasternodeProbeConn masternode_probe_connection) +void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound, const char *pszDest, ConnectionType conn_type, MasternodeConn masternode_connection, MasternodeProbeConn masternode_probe_connection) { + assert(conn_type != ConnectionType::INBOUND); + // // Initiate outbound network connection // @@ -2751,7 +2759,7 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai return; LogPrint(BCLog::NET_NETCONN, "CConnman::%s -- connecting to %s\n", __func__, getIpStr()); - CNode* pnode = ConnectNode(addrConnect, pszDest, fCountFailure, manual_connection, block_relay_only); + CNode* pnode = ConnectNode(addrConnect, pszDest, fCountFailure, conn_type); if (!pnode) { LogPrint(BCLog::NET_NETCONN, "CConnman::%s -- ConnectNode failed for %s\n", __func__, getIpStr()); @@ -2765,12 +2773,7 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai if (grantOutbound) grantOutbound->MoveTo(pnode->grantOutbound); - if (fOneShot) - pnode->fOneShot = true; - if (fFeeler) - pnode->fFeeler = true; - if (manual_connection) - pnode->m_manual_connection = true; + if (masternode_connection == MasternodeConn::IsConnection) pnode->m_masternode_connection = true; if (masternode_probe_connection == MasternodeProbeConn::IsConnection) @@ -2791,7 +2794,8 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai } void CConnman::OpenMasternodeConnection(const CAddress &addrConnect, MasternodeProbeConn probe) { - OpenNetworkConnection(addrConnect, false, nullptr, nullptr, false, false, false, false, MasternodeConn::IsConnection, probe); + + OpenNetworkConnection(addrConnect, false, nullptr, nullptr, ConnectionType::OUTBOUND, MasternodeConn::IsConnection, probe); } void CConnman::ThreadMessageHandler() @@ -3135,7 +3139,7 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions) } for (const auto& strDest : connOptions.vSeedNodes) { - AddOneShot(strDest); + AddAddrFetch(strDest); } if (clientInterface) { @@ -3238,7 +3242,7 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions) else threadDNSAddressSeed = std::thread(&util::TraceThread, "dnsseed", [this] { ThreadDNSAddressSeed(); }); - // Initiate outbound connections from -addnode + // Initiate manual connections threadOpenAddedConnections = std::thread(&util::TraceThread, "addcon", [this] { ThreadOpenAddedConnections(); }); if (connOptions.m_use_addrman_outgoing && !connOptions.m_specified_outgoing.empty()) { @@ -3616,7 +3620,7 @@ bool CConnman::IsMasternodeQuorumNode(const CNode* pnode) // Let's see if this is an outgoing connection to an address that is known to be a masternode // We however only need to know this if the node did not authenticate itself as a MN yet uint256 assumedProTxHash; - if (pnode->GetVerifiedProRegTxHash().IsNull() && !pnode->fInbound) { + if (pnode->GetVerifiedProRegTxHash().IsNull() && !pnode->IsInboundConn()) { auto mnList = deterministicMNManager->GetListAtChainTip(); auto dmn = mnList.GetMNByService(pnode->addr); if (dmn == nullptr) { @@ -3673,7 +3677,7 @@ size_t CConnman::GetNodeCount(NumConnections flags) if ((flags & CONNECTIONS_VERIFIED) && pnode->GetVerifiedProRegTxHash().IsNull()) { continue; } - if (flags & (pnode->fInbound ? CONNECTIONS_IN : CONNECTIONS_OUT)) { + if (flags & (pnode->IsInboundConn() ? CONNECTIONS_IN : CONNECTIONS_OUT)) { nNum++; } else if (flags == CONNECTIONS_VERIFIED) { nNum++; @@ -3901,15 +3905,14 @@ ServiceFlags CConnman::GetLocalServices() const unsigned int CConnman::GetReceiveFloodSize() const { return nReceiveFloodSize; } -CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, SOCKET hSocketIn, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress& addrBindIn, const std::string& addrNameIn, bool fInboundIn, bool block_relay_only, bool inbound_onion) +CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, SOCKET hSocketIn, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress& addrBindIn, const std::string& addrNameIn, ConnectionType conn_type_in, bool inbound_onion) : nTimeConnected(GetSystemTimeInSeconds()), addr(addrIn), addrBind(addrBindIn), - fInbound(fInboundIn), nKeyedNetGroup(nKeyedNetGroupIn), - m_addr_known{block_relay_only ? nullptr : std::make_unique(5000, 0.001)}, id(idIn), nLocalHostNonce(nLocalHostNonceIn), + m_conn_type(conn_type_in), nLocalServices(nLocalServicesIn), m_inbound_onion(inbound_onion) { @@ -3917,6 +3920,10 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, SOCKET hSocketIn, const addrName = addrNameIn == "" ? addr.ToStringIPPort() : addrNameIn; hashContinue = uint256(); + if (conn_type_in != ConnectionType::BLOCK_RELAY) { + m_addr_known = std::make_unique(5000, 0.001); + } + for (const std::string &msg : getAllNetMessageTypes()) mapRecvBytesPerMsgCmd[msg] = 0; mapRecvBytesPerMsgCmd[NET_MESSAGE_COMMAND_OTHER] = 0; diff --git a/src/net.h b/src/net.h index b2a73db5c154..103e3e452f0f 100644 --- a/src/net.h +++ b/src/net.h @@ -132,6 +132,17 @@ struct CSerializedNetMsg std::string command; }; +/** 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, /**< 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; class CConnman @@ -240,7 +251,7 @@ friend class CNode; IsConnection, }; - 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, MasternodeConn masternode_connection = MasternodeConn::IsNotConnection, MasternodeProbeConn masternode_probe_connection = MasternodeProbeConn::IsNotConnection); + void OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound = nullptr, const char *strDest = nullptr, ConnectionType conn_type = ConnectionType::OUTBOUND, MasternodeConn masternode_connection = MasternodeConn::IsNotConnection, MasternodeProbeConn masternode_probe_connection = MasternodeProbeConn::IsNotConnection); void OpenMasternodeConnection(const CAddress& addrConnect, MasternodeProbeConn probe = MasternodeProbeConn::IsConnection); bool CheckIncomingNonce(uint64_t nonce); @@ -510,8 +521,8 @@ friend class CNode; const std::vector& onion_binds); 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 ThreadI2PAcceptIncoming(); @@ -559,7 +570,7 @@ friend class CNode; CNode* FindNode(const CService& addr, bool fExcludeDisconnecting = true); bool AttemptToEvictConnection(); - CNode* ConnectNode(CAddress addrConnect, const char *pszDest = nullptr, bool fCountFailure = false, bool manual_connection = false, bool block_relay_only = false); + CNode* ConnectNode(CAddress addrConnect, const char *pszDest = nullptr, bool fCountFailure = false, ConnectionType conn_type = ConnectionType::OUTBOUND); void AddWhitelistPermissionFlags(NetPermissionFlags& flags, const CNetAddr &addr) const; void DeleteNode(CNode* pnode); @@ -610,8 +621,8 @@ friend class CNode; 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 vPendingMasternodes; @@ -1083,12 +1094,8 @@ class CNode } // 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_manual_connection{false}; bool fClient{false}; // set by version message bool m_limited_node{false}; //after BIP159, set by version message - const bool fInbound; /** * Whether the peer has signaled support for receiving ADDRv2 (BIP155) @@ -1134,6 +1141,59 @@ class CNode * @return network the peer connected through. */ Network ConnectedThroughNetwork() const; + 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; + } + + 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; + } + + bool IsAddrFetchConn() const { + return m_conn_type == ConnectionType::ADDR_FETCH; + } + + bool IsInboundConn() const { + 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 GUARDED_BY(cs_vSend); @@ -1145,7 +1205,7 @@ class CNode // 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}; @@ -1226,7 +1286,7 @@ class CNode // If true, we will send him all quorum related messages, even if he is not a member of our quorums std::atomic qwatch{false}; - CNode(NodeId id, ServiceFlags nLocalServicesIn, 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, bool inbound_onion = false); + CNode(NodeId id, ServiceFlags nLocalServicesIn, SOCKET hSocketIn, const CAddress &addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress &addrBindIn, const std::string &addrNameIn, ConnectionType conn_type_in, bool inbound_onion = false); ~CNode(); CNode(const CNode&) = delete; CNode& operator=(const CNode&) = delete; @@ -1234,6 +1294,7 @@ class CNode private: const NodeId id; const uint64_t nLocalHostNonce; + const ConnectionType m_conn_type; //! Services offered to this peer. //! diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 292ff224e7c4..a03c58f2cee1 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -736,7 +736,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.IsInboundConn() || node.HasPermission(PF_NOBAN)) && !node.IsAddrFetchConn() && !node.fClient; nPreferredDownload += state->fPreferredDownload; } @@ -1165,26 +1165,19 @@ 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.fOneShot); -} - void PeerManagerImpl::InitializeNode(CNode *pnode) { CAddress addr = pnode->addr; 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, pnode->fInbound)); + mapNodeState.emplace_hint(mapNodeState.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(addr, pnode->IsInboundConn())); } { PeerRef peer = std::make_shared(nodeid); LOCK(m_peer_mutex); m_peer_map.emplace_hint(m_peer_map.end(), nodeid, std::move(peer)); } - if (!pnode->fInbound) + if (!pnode->IsInboundConn()) PushNodeVersion(*pnode, GetTime()); } @@ -1244,7 +1237,7 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) { } } // cs_main - if (node.fSuccessfullyConnected && misbehavior == 0 && node.IsAddrRelayPeer() && !node.fInbound) { + if (node.fSuccessfullyConnected && misbehavior == 0 && node.IsAddrRelayPeer() && !node.IsInboundConn()) { // Only change visible addrman state for full outbound peers. We don't // call Connected() for feeler connections since they don't have // fSuccessfullyConnected set. @@ -2480,7 +2473,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, const std::vectorpindexBestKnownBlock != nullptr && pfrom.IsAddrRelayPeer()) { + if (!pfrom.fDisconnect && pfrom.IsOutboundOrBlockRelayConn() && nodestate->pindexBestKnownBlock != nullptr && pfrom.IsAddrRelayPeer()) { if (m_outbound_peers_with_protect_from_disconnect < MAX_OUTBOUND_PEERS_TO_PROTECT_FROM_DISCONNECT && nodestate->pindexBestKnownBlock->nChainWork >= m_chainman.ActiveChain().Tip()->nChainWork && !nodestate->m_chain_sync.m_protect) { LogPrint(BCLog::NET, "Protecting outbound peer=%d from eviction\n", pfrom.GetId()); nodestate->m_chain_sync.m_protect = true; @@ -2854,11 +2847,11 @@ void PeerManagerImpl::ProcessMessage( nTime = 0; } nServices = ServiceFlags(nServiceInt); - if (!pfrom.fInbound) + if (!pfrom.IsInboundConn()) { m_addrman.SetServices(pfrom.addr, nServices); } - if (!pfrom.fInbound && !pfrom.fFeeler && !pfrom.m_manual_connection && !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; @@ -2892,7 +2885,7 @@ void PeerManagerImpl::ProcessMessage( if (!vRecv.empty()) { bool fOtherMasternode = false; vRecv >> fOtherMasternode; - if (pfrom.fInbound) { + if (pfrom.IsInboundConn()) { pfrom.m_masternode_connection = fOtherMasternode; if (fOtherMasternode) { LogPrint(BCLog::NET_NETCONN, "peer=%d is an inbound masternode connection, not relaying anything to it\n", pfrom.GetId()); @@ -2905,26 +2898,26 @@ void PeerManagerImpl::ProcessMessage( } } // Disconnect if we connected to ourself - if (pfrom.fInbound && !m_connman.CheckIncomingNonce(nNonce)) + if (pfrom.IsInboundConn() && !m_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, GetAdjustedTime()); if (Params().NetworkIDString() == CBaseChainParams::DEVNET) { if (cleanSubVer.find(strprintf("devnet.%s", gArgs.GetDevNetName())) == std::string::npos) { LogPrintf("connected to wrong devnet. Reported version is %s, expected devnet name is %s\n", cleanSubVer, gArgs.GetDevNetName()); - if (!pfrom.fInbound) + if (!pfrom.IsInboundConn()) Misbehaving(pfrom.GetId(), 100); // don't try to connect again else Misbehaving(pfrom.GetId(), 1); // whover connected, might just have made a mistake, don't ban him immediately @@ -2973,7 +2966,7 @@ void PeerManagerImpl::ProcessMessage( UpdatePreferredDownload(pfrom, State(pfrom.GetId())); } - if (!pfrom.fInbound && pfrom.IsAddrRelayPeer()) + if (!pfrom.IsInboundConn() && pfrom.IsAddrRelayPeer()) { // Advertise our address if (fListen && !m_chainman.ActiveChainstate().IsInitialBlockDownload()) @@ -3011,8 +3004,7 @@ void PeerManagerImpl::ProcessMessage( AddTimeData(pfrom.addr, nTimeOffset); // Feeler connections exist only to verify if address is online. - if (pfrom.fFeeler) { - assert(pfrom.fInbound == false); + if (pfrom.IsFeelerConn()) { pfrom.fDisconnect = true; } return; @@ -3033,7 +3025,7 @@ void PeerManagerImpl::ProcessMessage( { pfrom.SetRecvVersion(std::min(pfrom.nVersion.load(), PROTOCOL_VERSION)); - if (!pfrom.fInbound) { + if (!pfrom.IsInboundConn()) { LogPrintf("New outbound peer connected: version: %d, blocks=%d, peer=%d%s (%s)\n", pfrom.nVersion.load(), pfrom.nStartingHeight, pfrom.GetId(), (fLogIPs ? strprintf(", peeraddr=%s", pfrom.addr.ToString()) : ""), @@ -3168,7 +3160,7 @@ void PeerManagerImpl::ProcessMessage( m_addrman.Add(vAddrOk, pfrom.addr, 2 * 60 * 60); if (vAddr.size() < 1000) pfrom.fGetAddr = false; - if (pfrom.fOneShot) + if (pfrom.IsAddrFetchConn()) pfrom.fDisconnect = true; return; } @@ -3256,7 +3248,7 @@ void PeerManagerImpl::ProcessMessage( } // Download if this is a nice peer, or we have no nice peers and this one might do. - bool fFetch = state->fPreferredDownload || (nPreferredDownload == 0 && !pfrom.fOneShot); + bool fFetch = state->fPreferredDownload || (nPreferredDownload == 0 && !pfrom.IsAddrFetchConn()); // Only actively request headers from a single peer, unless we're close to end of initial download. if ((nSyncStarted == 0 && fFetch) || pindexBestHeader->GetBlockTime() > GetAdjustedTime() - nMaxTipAge) { // Make sure to mark this peer as the one we are currently syncing with etc. @@ -4040,7 +4032,7 @@ void PeerManagerImpl::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; } @@ -4373,7 +4365,7 @@ bool PeerManagerImpl::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; @@ -4478,7 +4470,7 @@ void PeerManagerImpl::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 @@ -4547,7 +4539,7 @@ void PeerManagerImpl::EvictExtraOutboundPeers(int64_t time_in_seconds) // Don't disconnect masternodes just because they were slow in block announcement if (pnode->m_masternode_connection) return; // 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 @@ -4733,7 +4725,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) // Start block sync if (pindexBestHeader == nullptr) pindexBestHeader = m_chainman.ActiveChain().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->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 && pto->CanRelay()) { // Only actively request headers from a single peer, unless we're close to end of initial download. if ((nSyncStarted == 0 && fFetch) || pindexBestHeader->GetBlockTime() > GetAdjustedTime() - nMaxTipAge) { @@ -4957,7 +4949,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) bool fSendTrickle = pto->HasPermission(PF_NOBAN) || fMasternodeMode; if (pto->m_tx_relay->nNextInvSend < current_time) { fSendTrickle = true; - if (pto->fInbound) { + if (pto->IsInboundConn()) { pto->m_tx_relay->nNextInvSend = std::chrono::microseconds{m_connman.PoissonNextSendInbound(current_time.count(), INVENTORY_BROADCAST_INTERVAL)}; } else { // Use half the delay for regular outbound peers, as there is less privacy concern for them. diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 168d3abdeb56..fa7d7981db80 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -273,7 +273,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(), ConnectionType::MANUAL); return NullUniValue; } diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index f737f5ecc48c..d9b17f27d9f0 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -86,7 +86,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), INVALID_SOCKET, addr1, 0, 0, CAddress(), "", /*fInboundIn=*/ false); + CNode dummyNode1(id++, ServiceFlags(NODE_NETWORK), INVALID_SOCKET, addr1, 0, 0, CAddress(), "", ConnectionType::OUTBOUND); dummyNode1.SetSendVersion(PROTOCOL_VERSION); peerLogic->InitializeNode(&dummyNode1); @@ -138,7 +138,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) static void AddRandomOutboundPeer(std::vector &vNodes, PeerManager &peerLogic, CConnmanTest* connman) { CAddress addr(ip(g_insecure_rand_ctx.randbits(32)), NODE_NONE); - vNodes.emplace_back(new CNode(id++, ServiceFlags(NODE_NETWORK), INVALID_SOCKET, addr, 0, 0, CAddress(), "", /*fInboundIn=*/ false)); + vNodes.emplace_back(new CNode(id++, ServiceFlags(NODE_NETWORK), INVALID_SOCKET, addr, 0, 0, CAddress(), "", ConnectionType::OUTBOUND)); CNode &node = *vNodes.back(); node.SetSendVersion(PROTOCOL_VERSION); @@ -234,7 +234,7 @@ BOOST_AUTO_TEST_CASE(DoS_banning) banman->ClearBanned(); CAddress addr1(ip(0xa0b0c001), NODE_NONE); - CNode dummyNode1(id++, NODE_NETWORK, INVALID_SOCKET, addr1, 0, 0, CAddress(), "", true); + CNode dummyNode1(id++, NODE_NETWORK, INVALID_SOCKET, addr1, 0, 0, CAddress(), "", ConnectionType::INBOUND); dummyNode1.SetSendVersion(PROTOCOL_VERSION); peerLogic->InitializeNode(&dummyNode1); dummyNode1.nVersion = 1; @@ -248,7 +248,7 @@ BOOST_AUTO_TEST_CASE(DoS_banning) BOOST_CHECK(!banman->IsDiscouraged(ip(0xa0b0c001|0x0000ff00))); // Different IP, not banned CAddress addr2(ip(0xa0b0c002), NODE_NONE); - CNode dummyNode2(id++, NODE_NETWORK, INVALID_SOCKET, addr2, 1, 1, CAddress(), "", true); + CNode dummyNode2(id++, NODE_NETWORK, INVALID_SOCKET, addr2, 1, 1, CAddress(), "", ConnectionType::INBOUND); dummyNode2.SetSendVersion(PROTOCOL_VERSION); peerLogic->InitializeNode(&dummyNode2); dummyNode2.nVersion = 1; @@ -283,7 +283,7 @@ BOOST_AUTO_TEST_CASE(DoS_banscore) banman->ClearBanned(); gArgs.ForceSetArg("-banscore", "111"); // because 11 is my favorite number CAddress addr1(ip(0xa0b0c001), NODE_NONE); - CNode dummyNode1(id++, NODE_NETWORK, INVALID_SOCKET, addr1, 3, 1, CAddress(), "", true); + CNode dummyNode1(id++, NODE_NETWORK, INVALID_SOCKET, addr1, 3, 1, CAddress(), "", ConnectionType::INBOUND); dummyNode1.SetSendVersion(PROTOCOL_VERSION); peerLogic->InitializeNode(&dummyNode1); dummyNode1.nVersion = 1; @@ -331,7 +331,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, INVALID_SOCKET, addr, 4, 4, CAddress(), "", true); + CNode dummyNode(id++, NODE_NETWORK, INVALID_SOCKET, addr, 4, 4, CAddress(), "", ConnectionType::INBOUND); dummyNode.SetSendVersion(PROTOCOL_VERSION); peerLogic->InitializeNode(&dummyNode); dummyNode.nVersion = 1; diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h index 30a3580e3913..d24d3aa40ca2 100644 --- a/src/test/fuzz/util.h +++ b/src/test/fuzz/util.h @@ -312,6 +312,7 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional(node_id, local_services, socket, address, keyed_net_group, local_host_nonce, addr_bind, addr_name, inbound, block_relay_only); } else { diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index 8187fb58d563..e4616bd74281 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -187,17 +187,12 @@ 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 = std::make_unique(id++, NODE_NETWORK, hSocket, addr, 0, 0, CAddress(), pszDest, fInboundIn); - BOOST_CHECK(pnode1->fInbound == false); - BOOST_CHECK(pnode1->fFeeler == false); + std::unique_ptr pnode1 = std::make_unique(id++, NODE_NETWORK, hSocket, addr, 0, 0, CAddress(), pszDest, ConnectionType::OUTBOUND); + BOOST_CHECK(pnode1->IsInboundConn() == false); - fInboundIn = true; - std::unique_ptr pnode2 = std::make_unique(id++, NODE_NETWORK, hSocket, addr, 1, 1, CAddress(), pszDest, fInboundIn); - BOOST_CHECK(pnode2->fInbound == true); - BOOST_CHECK(pnode2->fFeeler == false); + std::unique_ptr pnode2 = std::make_unique(id++, NODE_NETWORK, hSocket, addr, 1, 1, CAddress(), pszDest, ConnectionType::INBOUND); + BOOST_CHECK(pnode2->IsInboundConn() == true); } BOOST_AUTO_TEST_CASE(PoissonNextSend) @@ -704,7 +699,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 = std::make_unique(0, NODE_NETWORK, INVALID_SOCKET, addr, 0, 0, CAddress{}, std::string{}, false); + std::unique_ptr pnode = std::make_unique(0, NODE_NETWORK, INVALID_SOCKET, addr, 0, 0, CAddress{}, std::string{}, ConnectionType::OUTBOUND); pnode->fSuccessfullyConnected.store(true); // the peer claims to be reaching us via IPv6 From 4143e359f70ddb7a1ddd676ff2d30211ccb40ca5 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sat, 26 Sep 2020 17:24:33 +0200 Subject: [PATCH 2/4] (Partial) Merge #19725: [RPC] Add connection type to getpeerinfo, improve logs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit a512925e19a70d7f6b80ac530a169f45ffaafa1c [doc] Release notes (Amiti Uttarwar) 50f94b34a33c954f6e207f509c93d33267a5c3e2 [rpc] Deprecate getpeerinfo addnode field (Amiti Uttarwar) df091b9b509f0b10e4315c0bfa2da0cc0c31c22f [refactor] Rename test file to allow any getpeerinfo deprecations. (Amiti Uttarwar) 395acfa83a5436790c1a722a5609ac9d48df235f [rpc] Add connection type to getpeerinfo RPC, update tests (Amiti Uttarwar) 49c10a9ca40967d28ae16dfea9cccc6f3a6624a1 [log] Add connection type to log statement (Amiti Uttarwar) Pull request description: After #19316, we can more directly expose information about the connection type on the `getpeerinfo` RPC. Doing so also makes the existing addnode field redundant, so this PR begins the process of deprecating this field. This PR also includes one commit that improves a log message, as both use a shared function to return the connection type as a string. Suggested by sdaftuar- https://github.com/bitcoin/bitcoin/pull/19316#discussion_r468001604 & https://github.com/bitcoin/bitcoin/pull/19316#discussion_r468018093 ACKs for top commit: jnewbery: Code review ACK a512925e19a70d7f6b80ac530a169f45ffaafa1c. sipa: utACK a512925e19a70d7f6b80ac530a169f45ffaafa1c guggero: Tested and code review ACK a512925e. MarcoFalke: cr ACK a512925e19a70d7f6b80ac530a169f45ffaafa1c 🌇 promag: Code review ACK a512925e19a70d7f6b80ac530a169f45ffaafa1c. Tree-SHA512: 601a7a38aee235ee59aca690784f886dc2ae4e418b2e6422c4b58cd597376c00f74910f66920b08a08a0bec28bf8022e71a1435785ff6ba8a188954261aba78e --- doc/release-notes-19725.md | 4 ++++ src/net.cpp | 21 +++++++++++++++++++++ src/net.h | 12 ++++++++++++ src/net_processing.cpp | 6 +----- src/rpc/net.cpp | 2 ++ test/functional/rpc_net.py | 10 ++++++++++ 6 files changed, 50 insertions(+), 5 deletions(-) create mode 100644 doc/release-notes-19725.md diff --git a/doc/release-notes-19725.md b/doc/release-notes-19725.md new file mode 100644 index 000000000000..664a55a56f14 --- /dev/null +++ b/doc/release-notes-19725.md @@ -0,0 +1,4 @@ +Updated RPC +-------- + +The `getpeerinfo` RPC now returns a `connection_type` field. This indicates the type of connection established with the peer. It will return one of six options. For more information, see the `getpeerinfo` help documentation. diff --git a/src/net.cpp b/src/net.cpp index 05cca1fe89f7..c5d026970f59 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -580,6 +580,26 @@ bool CNode::IsBlockRelayOnly() const { return (ignores_incoming_txs && !HasPermission(PF_RELAY)) || !IsAddrRelayPeer(); } +std::string CNode::ConnectionTypeAsString() const +{ + switch (m_conn_type) { + case ConnectionType::INBOUND: + return "inbound"; + case ConnectionType::MANUAL: + return "manual"; + case ConnectionType::FEELER: + return "feeler"; + case ConnectionType::OUTBOUND_FULL_RELAY: + return "outbound-full-relay"; + case ConnectionType::BLOCK_RELAY: + return "block-relay-only"; + case ConnectionType::ADDR_FETCH: + return "addr-fetch"; + } // no default case, so the compiler can warn about missing cases + + assert(false); +} + std::string CNode::GetAddrName() const { LOCK(cs_addrName); return addrName; @@ -684,6 +704,7 @@ void CNode::copyStats(CNodeStats &stats, const std::vector &m_asmap) X(verifiedPubKeyHash); } X(m_masternode_connection); + stats.m_conn_type_string = ConnectionTypeAsString(); } #undef X diff --git a/src/net.h b/src/net.h index 103e3e452f0f..037a6537ad40 100644 --- a/src/net.h +++ b/src/net.h @@ -144,6 +144,15 @@ enum class ConnectionType { ADDR_FETCH, /**< short lived connections used to solicit addrs when starting the node without a populated AddrMan */ }; +const std::vector CONNECTION_TYPE_DOC{ + "outbound-full-relay (default automatic connections)", + "block-relay-only (does not relay transactions or addresses)", + "inbound (initiated by the peer)", + "manual (added via addnode RPC or -addnode/-connect configuration options)", + "addr-fetch (short-lived automatic connection for soliciting addresses)", + "feeler (short-lived automatic connection for testing addresses)"}; + + class NetEventsInterface; class CConnman { @@ -918,6 +927,7 @@ class CNodeStats // In case this is a verified MN, this value is the hashed operator pubkey of the MN uint256 verifiedPubKeyHash; bool m_masternode_connection; + std::string m_conn_type_string; }; @@ -1471,6 +1481,8 @@ class CNode //! Sets the addrName only if it was not previously set void MaybeSetAddrName(const std::string& addrNameIn); + + std::string ConnectionTypeAsString() const; std::string GetLogString() const; bool CanRelay() const { return !m_masternode_connection || m_masternode_iqr_connection; } diff --git a/src/net_processing.cpp b/src/net_processing.cpp index a03c58f2cee1..ae700a6fc12a 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -4033,11 +4033,7 @@ void PeerManagerImpl::ProcessMessage( // Making nodes which are behind NAT and can only make outgoing connections ignore // the getaddr message mitigates the attack. if (!pfrom.IsInboundConn()) { - LogPrint(BCLog::NET, "Ignoring \"getaddr\" from outbound connection. peer=%d\n", pfrom.GetId()); - return; - } - if (!pfrom.IsAddrRelayPeer()) { - LogPrint(BCLog::NET, "Ignoring \"getaddr\" from block-relay-only connection. peer=%d\n", pfrom.GetId()); + LogPrint(BCLog::NET, "Ignoring \"getaddr\" from %s connection. peer=%d\n", pfrom.ConnectionTypeAsString(), pfrom.GetId()); return; } diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index fa7d7981db80..7a7ba6be8aa6 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -117,6 +117,7 @@ static UniValue getpeerinfo(const JSONRPCRequest& request) {RPCResult::Type::STR, "subver", "The string version"}, {RPCResult::Type::BOOL, "inbound", "Inbound (true) or Outbound (false)"}, {RPCResult::Type::BOOL, "addnode", "Whether connection was due to addnode/-connect or if it was an automatic/inbound connection"}, + {RPCResult::Type::STR, "connection_type", "Type of connection: \n" + Join(CONNECTION_TYPE_DOC, ",\n") + "."}, {RPCResult::Type::BOOL, "masternode", "Whether connection was due to masternode connection attempt"}, {RPCResult::Type::NUM, "startingheight", "The starting height (block) of the peer"}, {RPCResult::Type::NUM, "banscore", "The ban score"}, @@ -236,6 +237,7 @@ static UniValue getpeerinfo(const JSONRPCRequest& request) recvPerMsgCmd.pushKV(i.first, i.second); } obj.pushKV("bytesrecv_per_msg", recvPerMsgCmd); + obj.pushKV("connection_type", stats.m_conn_type_string); ret.push_back(obj); } diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py index d086db4e596a..724202c67da9 100755 --- a/test/functional/rpc_net.py +++ b/test/functional/rpc_net.py @@ -163,6 +163,16 @@ def _test_getpeerinfo(self): # Check dynamically generated networks list in getpeerinfo help output. assert "(ipv4, ipv6, onion, i2p, not_publicly_routable)" in self.nodes[0].help("getpeerinfo") + # This part is slightly different comparing to the Bitcoin implementation. This is expected because we create connections on network setup a bit differently too. + # We also create more connection during the test itself to test mn specific stats + assert_equal(peer_info[0][0]['connection_type'], 'manual') + assert_equal(peer_info[0][1]['connection_type'], 'inbound') + + assert_equal(peer_info[1][0]['connection_type'], 'inbound') + assert_equal(peer_info[1][1]['connection_type'], 'manual') + assert_equal(peer_info[1][2]['connection_type'], 'manual') + + assert_equal(peer_info[2][0]['connection_type'], 'inbound') def test_service_flags(self): self.nodes[0].add_p2p_connection(P2PInterface(), services=(1 << 4) | (1 << 63)) From 796353adaaa21623c0556bb7e24b1590e28bbf0a Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Thu, 3 Sep 2020 13:22:00 +0200 Subject: [PATCH 3/4] Merge #19724: [net] Cleanup connection types- followups eb1c5d090f4ef844dabc1e8bf3864c1afe1c791c [doc] Follow developer notes, add comment about missing default. (Amiti Uttarwar) d5a57cef62ee9e9d30f7e3b80e178149ceeef67c [doc] Describe connection types in more depth. (Amiti Uttarwar) 4829b6fcc6489b445f80689af6c2a1a919f176b1 [refactor] Simplify connection type logic in ThreadOpenConnections (Amiti Uttarwar) 1e563aed785565af6b7aed7f7399c52205d8f19c [refactor] Simplify check for block-relay-only connection. (Amiti Uttarwar) da3a0be61b025224231206cb4656e420453bfdeb [test] Add explicit tests that connection types get set correctly (Amiti Uttarwar) 1d74fc7df621b31d1b8adc9d7f53e7478d6e40b5 [trivial] Small style updates (Amiti Uttarwar) ff6b9081add3a40d53b1cc1352b57eeb46e41d45 [doc] Explain address handling logic in process messages (Amiti Uttarwar) dff16b184b1428a068d144e5e4dde7595b4729c0 [refactor] Restructure logic to check for addr relay. (Amiti Uttarwar) a6ab1e81f964df131cfa0e11e07bedb3283b823f [net] Remove unnecessary default args on OpenNetworkConnection (Amiti Uttarwar) 8d6ff46f55f373e344278ab3f1ac27b1dba36623 scripted-diff: Rename `OUTBOUND` ConnectionType to `OUTBOUND_FULL_RELAY` (Amiti Uttarwar) Pull request description: This PR addresses outstanding review comments from #19316. It further simplifies `net.cpp` complexity and adds documentation about design goals about different connection types. ACKs for top commit: naumenkogs: ACK eb1c5d090f4ef844dabc1e8bf3864c1afe1c791c laanwj: Code review ACK eb1c5d090f4ef844dabc1e8bf3864c1afe1c791c Tree-SHA512: 2fe14e428404c95661e5518c8c90db07ab5b9ebb1bac921b3bdf82b181f444fae379f8fc0a2b619e6b4693f01c55bd246fbd8c8eedadd96849a30de3161afee5 --- src/llmq/instantsend.cpp | 2 +- src/net.cpp | 79 ++++++++++++--------------- src/net.h | 87 ++++++++++++++++++++++-------- src/net_processing.cpp | 65 ++++++++++++++-------- src/test/denialofservice_tests.cpp | 4 +- src/test/fuzz/net.cpp | 2 +- src/test/net_tests.cpp | 14 ++++- 7 files changed, 156 insertions(+), 97 deletions(-) diff --git a/src/llmq/instantsend.cpp b/src/llmq/instantsend.cpp index 1e23f9e37854..e44d4e7a6249 100644 --- a/src/llmq/instantsend.cpp +++ b/src/llmq/instantsend.cpp @@ -1465,7 +1465,7 @@ void CInstantSendManager::AskNodesForLockedTx(const uint256& txid, const CConnma if (nodesToAskFor.size() >= 4) { return; } - if (pnode->IsAddrRelayPeer()) { + if (pnode->RelayAddrsWithConn()) { LOCK(pnode->m_tx_relay->cs_tx_inventory); if (pnode->m_tx_relay->filterInventoryKnown.contains(txid)) { pnode->AddRef(); diff --git a/src/net.cpp b/src/net.cpp index c5d026970f59..50b8f799e1e3 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -577,7 +577,7 @@ bool CNode::IsBlockRelayOnly() const { // Stop processing non-block data early if // 1) We are in blocks only mode and peer has no relay permission // 2) This peer is a block-relay-only peer - return (ignores_incoming_txs && !HasPermission(PF_RELAY)) || !IsAddrRelayPeer(); + return (ignores_incoming_txs && !HasPermission(PF_RELAY)) || !RelayAddrsWithConn(); } std::string CNode::ConnectionTypeAsString() const @@ -646,7 +646,7 @@ void CNode::copyStats(CNodeStats &stats, const std::vector &m_asmap) X(addrBind); stats.m_network = ConnectedThroughNetwork(); stats.m_mapped_as = addr.GetMappedAS(m_asmap); - if (IsAddrRelayPeer()) { + if (RelayAddrsWithConn()) { LOCK(m_tx_relay->cs_filter); stats.fRelayTxes = m_tx_relay->fRelayTxes; } else { @@ -1047,7 +1047,7 @@ bool CConnman::AttemptToEvictConnection() bool peer_relay_txes = false; bool peer_filter_not_null = false; - if (node->IsAddrRelayPeer()) { + if (node->RelayAddrsWithConn()) { LOCK(node->m_tx_relay->cs_filter); peer_relay_txes = node->m_tx_relay->fRelayTxes; peer_filter_not_null = node->m_tx_relay->pfilter != nullptr; @@ -2307,16 +2307,16 @@ void CConnman::ThreadOpenConnections(const std::vector connect) // 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){ + switch (pnode->m_conn_type) { case ConnectionType::INBOUND: case ConnectionType::MANUAL: break; - case ConnectionType::OUTBOUND: + case ConnectionType::OUTBOUND_FULL_RELAY: case ConnectionType::BLOCK_RELAY: case ConnectionType::ADDR_FETCH: case ConnectionType::FEELER: setConnected.insert(pnode->addr.GetGroup(addrman.m_asmap)); - } + } // no default case, so the compiler can warn about missing cases } } @@ -2331,28 +2331,32 @@ void CConnman::ThreadOpenConnections(const std::vector connect) } } - // Feeler Connections - // - // Design goals: - // * Increase the number of connectable addresses in the tried table. - // - // Method: - // * Choose a random address from new and attempt to connect to it if we can connect - // successfully it is added to tried. - // * Start attempting feeler connections only after node finishes making outbound - // connections. - // * Only make a feeler connection once every few minutes. - // + ConnectionType conn_type = ConnectionType::OUTBOUND_FULL_RELAY; + int64_t nTime = GetTimeMicros(); bool fFeeler = false; - if (nOutboundFullRelay >= m_max_outbound_full_relay && nOutboundBlockRelay >= m_max_outbound_block_relay && !GetTryNewOutboundPeer()) { - int64_t nTime = GetTimeMicros(); // The current time right now (in microseconds). - if (nTime > nNextFeeler) { - nNextFeeler = PoissonNextSend(nTime, FEELER_INTERVAL); - fFeeler = true; - } else { - continue; - } + // Determine what type of connection to open. Opening + // OUTBOUND_FULL_RELAY connections gets the highest priority until we + // meet our full-relay capacity. Then we open BLOCK_RELAY connection + // until we hit our block-relay-only peer limit. + // GetTryNewOutboundPeer() gets set when a stale tip is detected, so we + // try opening an additional OUTBOUND_FULL_RELAY connection. If none of + // these conditions are met, check the nNextFeeler timer to decide if + // we should open a FEELER. + + if (nOutboundFullRelay < m_max_outbound_full_relay) { + // OUTBOUND_FULL_RELAY + } else if (nOutboundBlockRelay < m_max_outbound_block_relay) { + conn_type = ConnectionType::BLOCK_RELAY; + } else if (GetTryNewOutboundPeer()) { + // OUTBOUND_FULL_RELAY + } else if (nTime > nNextFeeler) { + nNextFeeler = PoissonNextSend(nTime, FEELER_INTERVAL); + conn_type = ConnectionType::FEELER; + fFeeler = true; + } else { + // skip to next iteration of while loop + continue; } addrman.ResolveCollisions(); @@ -2441,23 +2445,6 @@ void CConnman::ThreadOpenConnections(const std::vector connect) } } - ConnectionType conn_type; - // 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 (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; - } - OpenNetworkConnection(addrConnect, (int)setConnected.size() >= std::min(nMaxConnections - 1, 2), &grant, nullptr, conn_type); } } @@ -2816,7 +2803,7 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai void CConnman::OpenMasternodeConnection(const CAddress &addrConnect, MasternodeProbeConn probe) { - OpenNetworkConnection(addrConnect, false, nullptr, nullptr, ConnectionType::OUTBOUND, MasternodeConn::IsConnection, probe); + OpenNetworkConnection(addrConnect, false, nullptr, nullptr, ConnectionType::OUTBOUND_FULL_RELAY, MasternodeConn::IsConnection, probe); } void CConnman::ThreadMessageHandler() @@ -3791,7 +3778,7 @@ void CConnman::RelayInvFiltered(CInv &inv, const CTransaction& relatedTx, const { LOCK(cs_vNodes); for (const auto& pnode : vNodes) { - if (pnode->nVersion < minProtoVersion || !pnode->CanRelay() || !pnode->IsAddrRelayPeer()) { + if (pnode->nVersion < minProtoVersion || !pnode->CanRelay() || !pnode->RelayAddrsWithConn()) { continue; } { @@ -3811,7 +3798,7 @@ void CConnman::RelayInvFiltered(CInv &inv, const uint256& relatedTxHash, const i { LOCK(cs_vNodes); for (const auto& pnode : vNodes) { - if (pnode->nVersion < minProtoVersion || !pnode->CanRelay() || !pnode->IsAddrRelayPeer()) { + if (pnode->nVersion < minProtoVersion || !pnode->CanRelay() || !pnode->RelayAddrsWithConn()) { continue; } { diff --git a/src/net.h b/src/net.h index 037a6537ad40..557bb2f5c9cf 100644 --- a/src/net.h +++ b/src/net.h @@ -136,12 +136,54 @@ struct CSerializedNetMsg * 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, /**< 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 */ + /** + * Inbound connections are those initiated by a peer. This is the only + * property we know at the time of connection, until P2P messages are + * exchanged. + */ + INBOUND, + + /** + * These are the default connections that we use to connect with the + * network. There is no restriction on what is relayed- by default we relay + * blocks, addresses & transactions. We automatically attempt to open + * MAX_OUTBOUND_FULL_RELAY_CONNECTIONS using addresses from our AddrMan. + */ + OUTBOUND_FULL_RELAY, + + + /** + * We open manual connections to addresses that users explicitly inputted + * via the addnode RPC, or the -connect command line argument. Even if a + * manual connection is misbehaving, we do not automatically disconnect or + * add it to our discouragement filter. + */ + MANUAL, + + /** + * Feeler connections are short lived connections used to increase the + * number of connectable addresses in our AddrMan. Approximately every + * FEELER_INTERVAL, we attempt to connect to a random address from the new + * table. If successful, we add it to the tried table. + */ + FEELER, + + /** + * We use block-relay-only connections to help prevent against partition + * attacks. By not relaying transactions or addresses, these connections + * are harder to detect by a third party, thus helping obfuscate the + * network topology. We automatically attempt to open + * MAX_BLOCK_RELAY_ONLY_CONNECTIONS using addresses from our AddrMan. + */ + BLOCK_RELAY, + + /** + * AddrFetch connections are short lived connections used to solicit + * addresses from peers. These are initiated to addresses submitted via the + * -seednode command line argument, or under certain conditions when the + * AddrMan is empty. + */ + ADDR_FETCH, }; const std::vector CONNECTION_TYPE_DOC{ @@ -260,7 +302,7 @@ friend class CNode; IsConnection, }; - void OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound = nullptr, const char *strDest = nullptr, ConnectionType conn_type = ConnectionType::OUTBOUND, MasternodeConn masternode_connection = MasternodeConn::IsNotConnection, MasternodeProbeConn masternode_probe_connection = MasternodeProbeConn::IsNotConnection); + void OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant* grantOutbound, const char* strDest, ConnectionType conn_type, MasternodeConn masternode_connection = MasternodeConn::IsNotConnection, MasternodeProbeConn masternode_probe_connection = MasternodeProbeConn::IsNotConnection); void OpenMasternodeConnection(const CAddress& addrConnect, MasternodeProbeConn probe = MasternodeProbeConn::IsConnection); bool CheckIncomingNonce(uint64_t nonce); @@ -579,7 +621,7 @@ friend class CNode; CNode* FindNode(const CService& addr, bool fExcludeDisconnecting = true); bool AttemptToEvictConnection(); - CNode* ConnectNode(CAddress addrConnect, const char *pszDest = nullptr, bool fCountFailure = false, ConnectionType conn_type = ConnectionType::OUTBOUND); + CNode* ConnectNode(CAddress addrConnect, const char *pszDest = nullptr, bool fCountFailure = false, ConnectionType conn_type = ConnectionType::OUTBOUND_FULL_RELAY); void AddWhitelistPermissionFlags(NetPermissionFlags& flags, const CNetAddr &addr) const; void DeleteNode(CNode* pnode); @@ -1152,8 +1194,8 @@ class CNode */ Network ConnectedThroughNetwork() const; bool IsOutboundOrBlockRelayConn() const { - switch(m_conn_type) { - case ConnectionType::OUTBOUND: + switch (m_conn_type) { + case ConnectionType::OUTBOUND_FULL_RELAY: case ConnectionType::BLOCK_RELAY: return true; case ConnectionType::INBOUND: @@ -1161,13 +1203,13 @@ class CNode case ConnectionType::ADDR_FETCH: case ConnectionType::FEELER: return false; - } + } // no default case, so the compiler can warn about missing cases assert(false); } bool IsFullOutboundConn() const { - return m_conn_type == ConnectionType::OUTBOUND; + return m_conn_type == ConnectionType::OUTBOUND_FULL_RELAY; } bool IsManualConn() const { @@ -1190,17 +1232,23 @@ class CNode return m_conn_type == ConnectionType::INBOUND; } + /* Whether we send addr messages over this connection */ + bool RelayAddrsWithConn() const + { + return m_conn_type != ConnectionType::BLOCK_RELAY; + } + bool ExpectServicesFromConn() const { - switch(m_conn_type) { + switch (m_conn_type) { case ConnectionType::INBOUND: case ConnectionType::MANUAL: case ConnectionType::FEELER: return false; - case ConnectionType::OUTBOUND: + case ConnectionType::OUTBOUND_FULL_RELAY: case ConnectionType::BLOCK_RELAY: case ConnectionType::ADDR_FETCH: return true; - } + } // no default case, so the compiler can warn about missing cases assert(false); } @@ -1215,16 +1263,11 @@ class CNode // flood relay std::vector vAddrToSend; - std::unique_ptr m_addr_known = nullptr; + 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}; - // 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). - bool IsAddrRelayPeer() const { return m_addr_known != nullptr; } - bool IsBlockRelayOnly() const; // List of block ids we still have announce. @@ -1270,7 +1313,7 @@ class CNode }; // in bitcoin: m_tx_relay == nullptr if we're not relaying transactions with this peer - // in dash: m_tx_relay should never be nullptr, use `IsAddrRelayPeer() == false` instead + // in dash: m_tx_relay should never be nullptr, use `RelayAddrsWithConn() == false` instead std::unique_ptr m_tx_relay{std::make_unique()}; // Used for headers announcements - unfiltered blocks to relay diff --git a/src/net_processing.cpp b/src/net_processing.cpp index ae700a6fc12a..80de06a66993 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1006,7 +1006,7 @@ void PeerManagerImpl::PushNodeVersion(CNode& pnode, int64_t nTime) } m_connman.PushMessage(&pnode, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERSION, nProtocolVersion, (uint64_t)nLocalNodeServices, nTime, addrYou, addrMe, - nonce, strSubVersion, nNodeStartingHeight, !m_ignore_incoming_txs && pnode.IsAddrRelayPeer(), mnauthChallenge, pnode.m_masternode_connection.load())); + nonce, strSubVersion, nNodeStartingHeight, !m_ignore_incoming_txs && pnode.RelayAddrsWithConn(), mnauthChallenge, pnode.m_masternode_connection.load())); if (fLogIPs) { LogPrint(BCLog::NET, "send version message: version %d, blocks=%d, us=%s, them=%s, peer=%d\n", nProtocolVersion, nNodeStartingHeight, addrMe.ToString(), addrYou.ToString(), nodeid); @@ -1177,8 +1177,9 @@ void PeerManagerImpl::InitializeNode(CNode *pnode) { LOCK(m_peer_mutex); m_peer_map.emplace_hint(m_peer_map.end(), nodeid, std::move(peer)); } - if (!pnode->IsInboundConn()) + if (!pnode->IsInboundConn()) { PushNodeVersion(*pnode, GetTime()); + } } void PeerManagerImpl::ReattemptInitialBroadcast(CScheduler& scheduler) @@ -1237,7 +1238,7 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) { } } // cs_main - if (node.fSuccessfullyConnected && misbehavior == 0 && node.IsAddrRelayPeer() && !node.IsInboundConn()) { + if (node.fSuccessfullyConnected && misbehavior == 0 && node.RelayAddrsWithConn() && !node.IsInboundConn()) { // Only change visible addrman state for full outbound peers. We don't // call Connected() for feeler connections since they don't have // fSuccessfullyConnected set. @@ -1904,7 +1905,7 @@ static void RelayAddress(const CAddress& addr, bool fReachable, const CConnman& assert(nRelayNodes <= best.size()); auto sortfunc = [&best, &hasher, nRelayNodes, addr](CNode* pnode) { - if (pnode->IsAddrRelayPeer() && pnode->IsAddrCompatible(addr)) { + if (pnode->RelayAddrsWithConn() && pnode->IsAddrCompatible(addr)) { uint64_t hashKey = CSipHasher(hasher).Write(pnode->GetId()).Finalize(); for (unsigned int i = 0; i < nRelayNodes; i++) { if (hashKey > best[i].first) { @@ -2011,7 +2012,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, const CChainParams& chai else if (inv.type == MSG_FILTERED_BLOCK) { bool sendMerkleBlock = false; CMerkleBlock merkleBlock; - if (pfrom.IsAddrRelayPeer()) { + if (pfrom.RelayAddrsWithConn()) { LOCK(pfrom.m_tx_relay->cs_filter); if (pfrom.m_tx_relay->pfilter) { sendMerkleBlock = true; @@ -2114,7 +2115,7 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic // mempool entries added before this time have likely expired from mapRelay const std::chrono::seconds longlived_mempool_time = GetTime() - RELAY_TX_CACHE_TIME; // Get last mempool request time - const std::chrono::seconds mempool_req = !pfrom.IsAddrRelayPeer() ? pfrom.m_tx_relay->m_last_mempool_req.load() + const std::chrono::seconds mempool_req = !pfrom.RelayAddrsWithConn() ? pfrom.m_tx_relay->m_last_mempool_req.load() : std::chrono::seconds::min(); // Process as many TX items from the front of the getdata queue as @@ -2135,7 +2136,7 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic } ++it; - if (!pfrom.IsAddrRelayPeer() && NetMessageViolatesBlocksOnly(inv.GetCommand())) { + if (!pfrom.RelayAddrsWithConn() && NetMessageViolatesBlocksOnly(inv.GetCommand())) { // Note that if we receive a getdata for non-block messages // from a block-relay-only outbound peer that violate the policy, // we skip such getdata messages from this peer @@ -2484,7 +2485,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, const std::vectorpindexBestKnownBlock != nullptr && pfrom.IsAddrRelayPeer()) { + if (!pfrom.fDisconnect && pfrom.IsFullOutboundConn() && nodestate->pindexBestKnownBlock != nullptr) { if (m_outbound_peers_with_protect_from_disconnect < MAX_OUTBOUND_PEERS_TO_PROTECT_FROM_DISCONNECT && nodestate->pindexBestKnownBlock->nChainWork >= m_chainman.ActiveChain().Tip()->nChainWork && !nodestate->m_chain_sync.m_protect) { LogPrint(BCLog::NET, "Protecting outbound peer=%d from eviction\n", pfrom.GetId()); nodestate->m_chain_sync.m_protect = true; @@ -2951,7 +2952,7 @@ void PeerManagerImpl::ProcessMessage( // set nodes not capable of serving the complete blockchain history as "limited nodes" pfrom.m_limited_node = (!(nServices & NODE_NETWORK) && (nServices & NODE_NETWORK_LIMITED)); - if (pfrom.IsAddrRelayPeer()) { + if (pfrom.RelayAddrsWithConn()) { LOCK(pfrom.m_tx_relay->cs_filter); pfrom.m_tx_relay->fRelayTxes = fRelay; // set to true after we get the first filter* message } @@ -2966,9 +2967,23 @@ void PeerManagerImpl::ProcessMessage( UpdatePreferredDownload(pfrom, State(pfrom.GetId())); } - if (!pfrom.IsInboundConn() && pfrom.IsAddrRelayPeer()) - { - // Advertise our address + if (!pfrom.IsInboundConn() && !pfrom.IsBlockOnlyConn()) { + // For outbound peers, we try to relay our address (so that other + // nodes can try to find us more quickly, as we have no guarantee + // that an outbound peer is even aware of how to reach us) and do a + // one-time address fetch (to help populate/update our addrman). If + // we're starting up for the first time, our addrman may be pretty + // empty and no one will know who we are, so these mechanisms are + // important to help us connect to the network. + // + // We also update the addrman to record connection success for + // these peers (which include OUTBOUND_FULL_RELAY and FEELER + // connections) so that addrman will have an up-to-date notion of + // which peers are online and available. + // + // We skip these operations for BLOCK_RELAY peers to avoid + // potentially leaking information about our BLOCK_RELAY + // connections via the addrman or address relay. if (fListen && !m_chainman.ActiveChainstate().IsInitialBlockDownload()) { CAddress addr = GetLocalAddress(&pfrom.addr, pfrom.GetLocalServices()); @@ -2987,7 +3002,11 @@ void PeerManagerImpl::ProcessMessage( // Get recent addresses m_connman.PushMessage(&pfrom, CNetMsgMaker(nSendVersion).Make(NetMsgType::GETADDR)); pfrom.fGetAddr = true; + + // Moves address from New to Tried table in Addrman, resolves + // tried-table collisions, etc. m_addrman.Good(pfrom.addr); + } std::string remoteAddr; @@ -3029,7 +3048,7 @@ void PeerManagerImpl::ProcessMessage( LogPrintf("New outbound peer connected: version: %d, blocks=%d, peer=%d%s (%s)\n", pfrom.nVersion.load(), pfrom.nStartingHeight, pfrom.GetId(), (fLogIPs ? strprintf(", peeraddr=%s", pfrom.addr.ToString()) : ""), - pfrom.IsAddrRelayPeer()? "full-relay" : "block-relay"); + pfrom.RelayAddrsWithConn()? "full-relay" : "block-relay"); } if (!pfrom.m_masternode_probe_connection) { @@ -3119,7 +3138,7 @@ void PeerManagerImpl::ProcessMessage( s >> vAddr; - if (!pfrom.IsAddrRelayPeer()) { + if (!pfrom.RelayAddrsWithConn()) { return; } if (vAddr.size() > MAX_ADDR_TO_SEND) @@ -4080,7 +4099,7 @@ void PeerManagerImpl::ProcessMessage( return; } - if (pfrom.IsAddrRelayPeer()) { + if (pfrom.RelayAddrsWithConn()) { LOCK(pfrom.m_tx_relay->cs_tx_inventory); pfrom.m_tx_relay->fSendMempool = true; } @@ -4170,7 +4189,7 @@ void PeerManagerImpl::ProcessMessage( // There is no excuse for sending a too-large filter Misbehaving(pfrom.GetId(), 100, "too-large bloom filter"); } - else if (pfrom.IsAddrRelayPeer()) + else if (pfrom.RelayAddrsWithConn()) { LOCK(pfrom.m_tx_relay->cs_filter); pfrom.m_tx_relay->pfilter.reset(new CBloomFilter(filter)); @@ -4188,7 +4207,7 @@ void PeerManagerImpl::ProcessMessage( bool bad = false; if (vData.size() > MAX_SCRIPT_ELEMENT_SIZE) { bad = true; - } else if (pfrom.IsAddrRelayPeer()) { + } else if (pfrom.RelayAddrsWithConn()) { LOCK(pfrom.m_tx_relay->cs_filter); if (pfrom.m_tx_relay->pfilter) { pfrom.m_tx_relay->pfilter->insert(vData); @@ -4203,7 +4222,7 @@ void PeerManagerImpl::ProcessMessage( } if (msg_type == NetMsgType::FILTERCLEAR) { - if (!pfrom.IsAddrRelayPeer()) { + if (!pfrom.RelayAddrsWithConn()) { return; } LOCK(pfrom.m_tx_relay->cs_filter); @@ -4541,7 +4560,7 @@ void PeerManagerImpl::EvictExtraOutboundPeers(int64_t time_in_seconds) // Don't evict our protected peers if (state->m_chain_sync.m_protect) return; // Don't evict our block-relay-only peers. - if (!pnode->IsAddrRelayPeer()) return; + if (!pnode->RelayAddrsWithConn()) return; if (state->m_last_block_announcement < oldest_block_announcement || (state->m_last_block_announcement == oldest_block_announcement && pnode->GetId() > worst_peer)) { worst_peer = pnode->GetId(); oldest_block_announcement = state->m_last_block_announcement; @@ -4667,7 +4686,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) int64_t nNow = GetTimeMicros(); auto current_time = GetTime(); - if (fListen && pto->IsAddrRelayPeer() && + if (fListen && pto->RelayAddrsWithConn() && !m_chainman.ActiveChainstate().IsInitialBlockDownload() && pto->m_next_local_addr_send < current_time) { if (std::optional local_addr = GetLocalAddrForPeer(pto)) { @@ -4680,7 +4699,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) // // Message: addr // - if (pto->IsAddrRelayPeer() && pto->m_next_addr_send < current_time) { + if (pto->RelayAddrsWithConn() && pto->m_next_addr_send < current_time) { pto->m_next_addr_send = PoissonNextSend(current_time, AVG_ADDRESS_BROADCAST_INTERVAL); std::vector vAddr; vAddr.reserve(pto->vAddrToSend.size()); @@ -4907,7 +4926,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) LOCK2(m_mempool.cs, pto->cs_inventory); size_t reserve = INVENTORY_BROADCAST_MAX_PER_1MB_BLOCK * MaxBlockSize() / 1000000; - if (pto->IsAddrRelayPeer()) { + if (pto->RelayAddrsWithConn()) { LOCK(pto->m_tx_relay->cs_tx_inventory); reserve = std::min(pto->m_tx_relay->setInventoryTxToSend.size(), reserve); } @@ -4937,7 +4956,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) } }; - if (pto->IsAddrRelayPeer()) { + if (pto->RelayAddrsWithConn()) { LOCK(pto->m_tx_relay->cs_tx_inventory); // Check whether periodic sends should happen // Note: If this node is running in a Masternode mode, it makes no sense to delay outgoing txes diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index d9b17f27d9f0..53e93023baf0 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -86,7 +86,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), INVALID_SOCKET, addr1, 0, 0, CAddress(), "", ConnectionType::OUTBOUND); + CNode dummyNode1(id++, ServiceFlags(NODE_NETWORK), INVALID_SOCKET, addr1, 0, 0, CAddress(), "", ConnectionType::OUTBOUND_FULL_RELAY); dummyNode1.SetSendVersion(PROTOCOL_VERSION); peerLogic->InitializeNode(&dummyNode1); @@ -138,7 +138,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) static void AddRandomOutboundPeer(std::vector &vNodes, PeerManager &peerLogic, CConnmanTest* connman) { CAddress addr(ip(g_insecure_rand_ctx.randbits(32)), NODE_NONE); - vNodes.emplace_back(new CNode(id++, ServiceFlags(NODE_NETWORK), INVALID_SOCKET, addr, 0, 0, CAddress(), "", ConnectionType::OUTBOUND)); + vNodes.emplace_back(new CNode(id++, ServiceFlags(NODE_NETWORK), INVALID_SOCKET, addr, 0, 0, CAddress(), "", ConnectionType::OUTBOUND_FULL_RELAY)); CNode &node = *vNodes.back(); node.SetSendVersion(PROTOCOL_VERSION); diff --git a/src/test/fuzz/net.cpp b/src/test/fuzz/net.cpp index 61e3453ed6eb..7d265c2eda1d 100644 --- a/src/test/fuzz/net.cpp +++ b/src/test/fuzz/net.cpp @@ -123,7 +123,7 @@ FUZZ_TARGET_INIT(net, initialize_net) const int ref_count = node.GetRefCount(); assert(ref_count >= 0); (void)node.GetSendVersion(); - (void)node.IsAddrRelayPeer(); + (void)node.RelayAddrsWithConn(); const NetPermissionFlags net_permission_flags = ConsumeWeakEnum(fuzzed_data_provider, ALL_NET_PERMISSION_FLAGS); (void)node.HasPermission(net_permission_flags); diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index e4616bd74281..459d01ccf365 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -188,10 +188,20 @@ BOOST_AUTO_TEST_CASE(cnode_simple_test) CAddress addr = CAddress(CService(ipv4Addr, 7777), NODE_NETWORK); std::string pszDest; - std::unique_ptr pnode1 = std::make_unique(id++, NODE_NETWORK, hSocket, addr, 0, 0, CAddress(), pszDest, ConnectionType::OUTBOUND); + std::unique_ptr pnode1 = std::make_unique(id++, NODE_NETWORK, hSocket, addr, 0, 0, CAddress(), pszDest, ConnectionType::OUTBOUND_FULL_RELAY); + BOOST_CHECK(pnode1->IsFullOutboundConn() == true); + BOOST_CHECK(pnode1->IsManualConn() == false); + BOOST_CHECK(pnode1->IsBlockOnlyConn() == false); + BOOST_CHECK(pnode1->IsFeelerConn() == false); + BOOST_CHECK(pnode1->IsAddrFetchConn() == false); BOOST_CHECK(pnode1->IsInboundConn() == false); std::unique_ptr pnode2 = std::make_unique(id++, NODE_NETWORK, hSocket, addr, 1, 1, CAddress(), pszDest, ConnectionType::INBOUND); + BOOST_CHECK(pnode2->IsFullOutboundConn() == false); + BOOST_CHECK(pnode2->IsManualConn() == false); + BOOST_CHECK(pnode2->IsBlockOnlyConn() == false); + BOOST_CHECK(pnode2->IsFeelerConn() == false); + BOOST_CHECK(pnode2->IsAddrFetchConn() == false); BOOST_CHECK(pnode2->IsInboundConn() == true); } @@ -699,7 +709,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 = std::make_unique(0, NODE_NETWORK, INVALID_SOCKET, addr, 0, 0, CAddress{}, std::string{}, ConnectionType::OUTBOUND); + std::unique_ptr pnode = std::make_unique(0, NODE_NETWORK, INVALID_SOCKET, addr, 0, 0, CAddress{}, std::string{}, ConnectionType::OUTBOUND_FULL_RELAY); pnode->fSuccessfullyConnected.store(true); // the peer claims to be reaching us via IPv6 From aa74d0b91034cc0f8e1b439b6549cb142bb459b6 Mon Sep 17 00:00:00 2001 From: Vijay Das Manikpuri <2348066+vijaydasmp@users.noreply.github.com> Date: Thu, 4 Jan 2024 19:56:47 +0530 Subject: [PATCH 4/4] fix: follow-up missing changes from Merge #20188: tests: Add fuzzing harness for CConnman --- src/test/fuzz/util.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h index d24d3aa40ca2..6cab0751a11b 100644 --- a/src/test/fuzz/util.h +++ b/src/test/fuzz/util.h @@ -310,13 +310,13 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional(); const CAddress addr_bind = ConsumeAddress(fuzzed_data_provider); const std::string addr_name = fuzzed_data_provider.ConsumeRandomLengthString(64); - const bool inbound = fuzzed_data_provider.ConsumeBool(); - const bool block_relay_only = fuzzed_data_provider.ConsumeBool(); + const ConnectionType conn_type = fuzzed_data_provider.PickValueInArray({ConnectionType::INBOUND, ConnectionType::OUTBOUND_FULL_RELAY, ConnectionType::MANUAL, ConnectionType::FEELER, ConnectionType::BLOCK_RELAY, ConnectionType::ADDR_FETCH}); + const bool inbound_onion = fuzzed_data_provider.ConsumeBool(); if constexpr (ReturnUniquePtr) { - return std::make_unique(node_id, local_services, socket, address, keyed_net_group, local_host_nonce, addr_bind, addr_name, inbound, block_relay_only); + return std::make_unique(node_id, local_services, socket, address, keyed_net_group, local_host_nonce, addr_bind, addr_name, conn_type, inbound_onion); } else { - return CNode{node_id, local_services, socket, address, keyed_net_group, local_host_nonce, addr_bind, addr_name, inbound, block_relay_only}; + return CNode{node_id, local_services, socket, address, keyed_net_group, local_host_nonce, addr_bind, addr_name, conn_type, inbound_onion}; } } inline std::unique_ptr ConsumeNodeAsUniquePtr(FuzzedDataProvider& fdp, const std::optional& node_id_in = std::nullopt) { return ConsumeNode(fdp, node_id_in); }