From 2a9bb8f7fa59df81d3f4fc701f2c07d9aaa82274 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Mon, 19 Oct 2020 09:31:51 -0400 Subject: [PATCH] merge bitcoin#20187: test-before-evict bugfix and improvements for block-relay-only peers completion of 698a717e from dash#5163 by including: - 4fe338ab3ed73b3ffb20eedf95500c56ec2920e1 - e8b215a086d91a8774210bb6ce8d1560aaaf0789 - 16d9bfc4172b4f6ce24a3cd1a1cfa3933cd26751 --- src/net.cpp | 34 +++++++++++++++++++++++++++++----- src/net.h | 6 ++++++ src/net_processing.cpp | 29 ++++++++++++++++++----------- 3 files changed, 53 insertions(+), 16 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index d1b27e2733..78630bd731 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -385,6 +385,11 @@ CNode* CConnman::FindNode(const CService& addr, bool fExcludeDisconnecting) return nullptr; } +bool CConnman::AlreadyConnectedToAddress(const CAddress& addr) +{ + return FindNode(addr.ToStringIPPort()); +} + bool CConnman::CheckIncomingNonce(uint64_t nonce) { LOCK(cs_vNodes); @@ -2397,11 +2402,30 @@ void CConnman::ThreadOpenConnections(const std::vector connect) if (nTries > 100) break; - CAddrInfo addr = addrman.SelectTriedCollision(); + CAddrInfo addr; - // SelectTriedCollision returns an invalid address if it is empty. - if (!fFeeler || !addr.IsValid()) { - addr = addrman.Select(fFeeler); + if (fFeeler) { + // First, try to get a tried table collision address. This returns + // an empty (invalid) address if there are no collisions to try. + addr = addrman.SelectTriedCollision(); + + if (!addr.IsValid()) { + // No tried table collisions. Select a new table address + // for our feeler. + addr = addrman.Select(true); + } else if (AlreadyConnectedToAddress(addr)) { + // If test-before-evict logic would have us connect to a + // peer that we're already connected to, just mark that + // address as Good(). We won't be able to initiate the + // connection anyway, so this avoids inadvertently evicting + // a currently-connected peer. + addrman.Good(addr); + // Select a new table address for our feeler instead. + addr = addrman.Select(true); + } + } else { + // Not a feeler + addr = addrman.Select(); } auto dmn = mnList.GetMNByService(addr); @@ -2768,7 +2792,7 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai if (!pszDest) { // banned, discouraged or exact match? - if ((m_banman && (m_banman->IsDiscouraged(addrConnect) || m_banman->IsBanned(addrConnect))) || FindNode(addrConnect.ToStringIPPort())) + if ((m_banman && (m_banman->IsDiscouraged(addrConnect) || m_banman->IsBanned(addrConnect))) || AlreadyConnectedToAddress(addrConnect)) return; // local and not a connection to itself? bool fAllowLocal = Params().AllowMultiplePorts() && addrConnect.GetPort() != GetListenPort(); diff --git a/src/net.h b/src/net.h index e1a2fcea80..b013ba183f 100644 --- a/src/net.h +++ b/src/net.h @@ -625,6 +625,12 @@ private: CNode* FindNode(const std::string& addrName, bool fExcludeDisconnecting = true); CNode* FindNode(const CService& addr, bool fExcludeDisconnecting = true); + /** + * Determine whether we're already connected to a given address, in order to + * avoid initiating duplicate connections. + */ + bool AlreadyConnectedToAddress(const CAddress& addr); + bool AttemptToEvictConnection(); 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; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 98cb54d55a..f1af6ca544 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3017,14 +3017,8 @@ void PeerManagerImpl::ProcessMessage( // 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. + // We skip this for BLOCK_RELAY peers to avoid potentially leaking + // information about our BLOCK_RELAY connections via address relay. if (fListen && !m_chainman.ActiveChainstate().IsInitialBlockDownload()) { CAddress addr = GetLocalAddress(&pfrom.addr, pfrom.GetLocalServices()); @@ -3043,11 +3037,24 @@ void PeerManagerImpl::ProcessMessage( // Get recent addresses m_connman.PushMessage(&pfrom, CNetMsgMaker(greatest_common_version).Make(NetMsgType::GETADDR)); pfrom.fGetAddr = true; + } - // Moves address from New to Tried table in Addrman, resolves - // tried-table collisions, etc. + if (!pfrom.IsInboundConn()) { + // For non-inbound connections, we update the addrman to record + // connection success so that addrman will have an up-to-date + // notion of which peers are online and available. + // + // While we strive to not leak information about block-relay-only + // connections via the addrman, not moving an address to the tried + // table is also potentially detrimental because new-table entries + // are subject to eviction in the event of addrman collisions. We + // mitigate the information-leak by never calling + // CAddrMan::Connected() on block-relay-only peers; see + // FinalizeNode(). + // + // This moves an address from New to Tried table in Addrman, + // resolves tried-table collisions, etc. m_addrman.Good(pfrom.addr); - } std::string remoteAddr;