From eff96a963332153060907133cf8de1914c8b0092 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Thu, 3 Sep 2020 13:22:00 +0200 Subject: [PATCH] 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/net.cpp | 55 ++++++++++---------- src/net.h | 80 ++++++++++++++++++++++++------ src/net_processing.cpp | 36 ++++++++++---- src/test/denialofservice_tests.cpp | 4 +- src/test/fuzz/net.cpp | 2 +- src/test/net_tests.cpp | 16 ++++-- 6 files changed, 134 insertions(+), 59 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 58e2d70992ef1d..9ad9d1d68970be 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2294,16 +2294,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 } } @@ -2330,16 +2330,32 @@ void CConnman::ThreadOpenConnections(const std::vector connect) // 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(); @@ -2429,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); } } diff --git a/src/net.h b/src/net.h index 0a92fe46236202..979d6576a73236 100644 --- a/src/net.h +++ b/src/net.h @@ -135,12 +135,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{ @@ -259,7 +301,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 = ConnectionType::OUTBOUND_FULL_RELAY, 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); @@ -578,7 +620,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); @@ -1134,8 +1176,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: @@ -1143,13 +1185,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 { @@ -1172,17 +1214,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); } @@ -1197,7 +1245,7 @@ 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}; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 0255581e07a418..f67bb2f5744550 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1156,8 +1156,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) const @@ -1895,7 +1896,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) { @@ -2472,7 +2473,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, const std::vectorpindexBestKnownBlock != nullptr && pfrom.IsAddrRelayPeer()) { + if (!pfrom.fDisconnect && pfrom.IsFullOutboundConn() && 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; @@ -2938,9 +2939,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()); @@ -2960,6 +2975,9 @@ void PeerManagerImpl::ProcessMessage( m_connman.PushMessage(&pfrom, CNetMsgMaker(nSendVersion).Make(NetMsgType::GETADDR)); pfrom.fGetAddr = true; m_addrman.Good(pfrom.addr); + + // Moves address from New to Tried table in Addrman, resolves + // tried-table collisions, etc. } std::string remoteAddr; @@ -3091,7 +3109,7 @@ void PeerManagerImpl::ProcessMessage( s >> vAddr; - if (!pfrom.IsAddrRelayPeer()) { + if (!pfrom.RelayAddrsWithConn()) { return; } if (vAddr.size() > MAX_ADDR_TO_SEND) @@ -4658,7 +4676,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) int64_t nNow = GetTimeMicros(); auto current_time = GetTime(); - if (pto->IsAddrRelayPeer() && !m_chainman.ActiveChainstate().IsInitialBlockDownload() && pto->m_next_local_addr_send < current_time) { + if (pto->RelayAddrsWithConn() && !m_chainman.ActiveChainstate().IsInitialBlockDownload() && pto->m_next_local_addr_send < current_time) { AdvertiseLocal(pto); pto->m_next_local_addr_send = PoissonNextSend(current_time, AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL); } @@ -4666,7 +4684,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()); diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index 94d07095445172..101fe3c1b7a2b8 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -84,7 +84,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) // Mock an outbound peer CAddress addr1(ip(0xa0b0c001), NODE_NONE); - CNode dummyNode1(id++, ServiceFlags(NODE_NETWORK), 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); @@ -136,7 +136,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 285f68a913f2e8..1454d99b5e2162 100644 --- a/src/test/fuzz/net.cpp +++ b/src/test/fuzz/net.cpp @@ -122,7 +122,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 b0cb1bebde36ca..74e7e0f94e823d 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -187,10 +187,20 @@ BOOST_AUTO_TEST_CASE(cnode_simple_test) CAddress addr = CAddress(CService(ipv4Addr, 7777), NODE_NETWORK); std::string pszDest; - std::unique_ptr pnode1 = MakeUnique(id++, NODE_NETWORK, height, 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 = MakeUnique(id++, NODE_NETWORK, height, hSocket, addr, 1, 1, CAddress(), pszDest, ConnectionType::INBOUND); + 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); } @@ -651,7 +661,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