From c769c4af11fc58dd4813d328c7f71042bc577676 Mon Sep 17 00:00:00 2001 From: Gregory Maxwell Date: Sun, 19 Apr 2015 12:34:43 -0700 Subject: [PATCH 1/2] Avoid counting failed connect attempts when probably offline. If a node is offline failed outbound connection attempts will crank up the addrman counter and effectively blow away our state. This change reduces the problem by only counting attempts made while the node believes it has outbound connections to at least two netgroups. Connect and addnode connections are also not counted, as there is no reason to unequally penalize them for their more frequent connections -- though there should be no real effect from this unless their addnode configureation is later removed. Wasteful repeated connection attempts while only a few connections are up are avoided via nLastTry. This is still somewhat incomplete protection because our outbound peers could be down but not timed out or might all be on 'local' networks (although the requirement for multiple netgroups helps). --- src/addrman.cpp | 4 ++-- src/addrman.h | 6 +++--- src/net.cpp | 20 ++++++++++---------- src/net.h | 2 +- src/rpc/net.cpp | 2 +- 5 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index 6c54cfa4c..0a1745c10 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -311,7 +311,7 @@ bool CAddrMan::Add_(const CAddress& addr, const CNetAddr& source, int64_t nTimeP return fNew; } -void CAddrMan::Attempt_(const CService& addr, int64_t nTime) +void CAddrMan::Attempt_(const CService& addr, bool fCountFailure, int64_t nTime) { CAddrInfo* pinfo = Find(addr); @@ -327,7 +327,7 @@ void CAddrMan::Attempt_(const CService& addr, int64_t nTime) // update info info.nLastTry = nTime; - info.nAttempts++; + if (fCountFailure) info.nAttempts++; } CAddrInfo CAddrMan::Select_(bool newOnly) diff --git a/src/addrman.h b/src/addrman.h index 308545045..65ca79fa0 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -230,7 +230,7 @@ protected: bool Add_(const CAddress &addr, const CNetAddr& source, int64_t nTimePenalty); //! Mark an entry as attempted to connect. - void Attempt_(const CService &addr, int64_t nTime); + void Attempt_(const CService &addr, bool fCountFailure, int64_t nTime); //! Select an address to connect to, if newOnly is set to true, only the new table is selected from. CAddrInfo Select_(bool newOnly); @@ -532,12 +532,12 @@ public: } //! Mark an entry as connection attempted to. - void Attempt(const CService &addr, int64_t nTime = GetAdjustedTime()) + void Attempt(const CService &addr, bool fCountFailure, int64_t nTime = GetAdjustedTime()) { { LOCK(cs); Check(); - Attempt_(addr, nTime); + Attempt_(addr, fCountFailure, nTime); Check(); } } diff --git a/src/net.cpp b/src/net.cpp index bbd23d292..f30321869 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -367,7 +367,7 @@ CNode* FindNode(const CService& addr) return NULL; } -CNode* ConnectNode(CAddress addrConnect, const char *pszDest) +CNode* ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure) { if (pszDest == NULL) { if (IsLocal(addrConnect)) @@ -399,7 +399,7 @@ CNode* ConnectNode(CAddress addrConnect, const char *pszDest) return NULL; } - addrman.Attempt(addrConnect); + addrman.Attempt(addrConnect, fCountFailure); // Add node CNode* pnode = new CNode(hSocket, addrConnect, pszDest ? pszDest : "", false); @@ -416,7 +416,7 @@ CNode* ConnectNode(CAddress addrConnect, const char *pszDest) } else if (!proxyConnectionFailed) { // If connecting to the node failed, and failure is not caused by a problem connecting to // the proxy, mark this as an attempt. - addrman.Attempt(addrConnect); + addrman.Attempt(addrConnect, fCountFailure); } return NULL; @@ -1533,7 +1533,7 @@ void static ProcessOneShot() CAddress addr; CSemaphoreGrant grant(*semOutbound, true); if (grant) { - if (!OpenNetworkConnection(addr, &grant, strDest.c_str(), true)) + if (!OpenNetworkConnection(addr, false, &grant, strDest.c_str(), true)) AddOneShot(strDest); } } @@ -1549,7 +1549,7 @@ void ThreadOpenConnections() BOOST_FOREACH(const std::string& strAddr, mapMultiArgs["-connect"]) { CAddress addr; - OpenNetworkConnection(addr, NULL, strAddr.c_str()); + OpenNetworkConnection(addr, false, NULL, strAddr.c_str()); for (int i = 0; i < 10 && i < nLoop; i++) { MilliSleep(500); @@ -1633,7 +1633,7 @@ void ThreadOpenConnections() } if (addrConnect.IsValid()) - OpenNetworkConnection(addrConnect, &grant); + OpenNetworkConnection(addrConnect, (int)setConnected.size() >= min(nMaxConnections - 1, 2), &grant); } } @@ -1655,7 +1655,7 @@ void ThreadOpenAddedConnections() BOOST_FOREACH(const std::string& strAddNode, lAddresses) { CAddress addr; CSemaphoreGrant grant(*semOutbound); - OpenNetworkConnection(addr, &grant, strAddNode.c_str()); + OpenNetworkConnection(addr, false, &grant, strAddNode.c_str()); MilliSleep(500); } MilliSleep(120000); // Retry every 2 minutes @@ -1694,7 +1694,7 @@ void ThreadOpenAddedConnections() BOOST_FOREACH(std::vector& vserv, lservAddressesToAdd) { CSemaphoreGrant grant(*semOutbound); - OpenNetworkConnection(CAddress(vserv[i % vserv.size()]), &grant); + OpenNetworkConnection(CAddress(vserv[i % vserv.size()]), false, &grant); MilliSleep(500); } MilliSleep(120000); // Retry every 2 minutes @@ -1702,7 +1702,7 @@ void ThreadOpenAddedConnections() } // if successful, this moves the passed grant to the constructed node -bool OpenNetworkConnection(const CAddress& addrConnect, CSemaphoreGrant *grantOutbound, const char *pszDest, bool fOneShot) +bool OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound, const char *pszDest, bool fOneShot) { // // Initiate outbound network connection @@ -1716,7 +1716,7 @@ bool OpenNetworkConnection(const CAddress& addrConnect, CSemaphoreGrant *grantOu } else if (FindNode(std::string(pszDest))) return false; - CNode* pnode = ConnectNode(addrConnect, pszDest); + CNode* pnode = ConnectNode(addrConnect, pszDest, fCountFailure); boost::this_thread::interruption_point(); if (!pnode) diff --git a/src/net.h b/src/net.h index 998ee4926..3332496c0 100644 --- a/src/net.h +++ b/src/net.h @@ -83,7 +83,7 @@ CNode* FindNode(const CNetAddr& ip); CNode* FindNode(const CSubNet& subNet); CNode* FindNode(const std::string& addrName); CNode* FindNode(const CService& ip); -bool OpenNetworkConnection(const CAddress& addrConnect, CSemaphoreGrant *grantOutbound = NULL, const char *strDest = NULL, bool fOneShot = false); +bool OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound = NULL, const char *strDest = NULL, bool fOneShot = false); void MapPort(bool fUseUPnP); unsigned short GetListenPort(); bool BindListenPort(const CService &bindAddr, std::string& strError, bool fWhitelisted = false); diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index e09af8965..8f62d636d 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -219,7 +219,7 @@ UniValue addnode(const UniValue& params, bool fHelp) if (strCommand == "onetry") { CAddress addr; - OpenNetworkConnection(addr, NULL, strNode.c_str()); + OpenNetworkConnection(addr, false, NULL, strNode.c_str()); return NullUniValue; } From 6182d10503ae3af222a7e4575724dce7ef563fec Mon Sep 17 00:00:00 2001 From: Gregory Maxwell Date: Sun, 19 Apr 2015 13:39:38 -0700 Subject: [PATCH 2/2] Do not increment nAttempts by more than one for every Good connection. This slows the increase of the nAttempts in addrman while partitioned, even if the node hasn't yet noticed the partitioning. --- src/addrman.cpp | 8 +++++++- src/addrman.h | 8 ++++++++ src/net.cpp | 2 +- 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index 0a1745c10..00f6fe99e 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -197,6 +197,9 @@ void CAddrMan::MakeTried(CAddrInfo& info, int nId) void CAddrMan::Good_(const CService& addr, int64_t nTime) { int nId; + + nLastGood = nTime; + CAddrInfo* pinfo = Find(addr, &nId); // if not found, bail out @@ -327,7 +330,10 @@ void CAddrMan::Attempt_(const CService& addr, bool fCountFailure, int64_t nTime) // update info info.nLastTry = nTime; - if (fCountFailure) info.nAttempts++; + if (fCountFailure && info.nLastCountAttempt < nLastGood) { + info.nLastCountAttempt = nTime; + info.nAttempts++; + } } CAddrInfo CAddrMan::Select_(bool newOnly) diff --git a/src/addrman.h b/src/addrman.h index 65ca79fa0..c5923e941 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -29,6 +29,9 @@ public: //! last try whatsoever by us (memory only) int64_t nLastTry; + //! last counted attempt (memory only) + int64_t nLastCountAttempt; + private: //! where knowledge about this address first came from CNetAddr source; @@ -66,6 +69,7 @@ public: { nLastSuccess = 0; nLastTry = 0; + nLastCountAttempt = 0; nAttempts = 0; nRefCount = 0; fInTried = false; @@ -200,6 +204,9 @@ private: //! list of "new" buckets int vvNew[ADDRMAN_NEW_BUCKET_COUNT][ADDRMAN_BUCKET_SIZE]; + //! last time Good was called (memory only) + int64_t nLastGood; + protected: //! secret key to randomize bucket select with uint256 nKey; @@ -458,6 +465,7 @@ public: nIdCount = 0; nTried = 0; nNew = 0; + nLastGood = 1; //Initially at 1 so that "never" is strictly worse. } CAddrMan() diff --git a/src/net.cpp b/src/net.cpp index f30321869..c15a4692e 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1633,7 +1633,7 @@ void ThreadOpenConnections() } if (addrConnect.IsValid()) - OpenNetworkConnection(addrConnect, (int)setConnected.size() >= min(nMaxConnections - 1, 2), &grant); + OpenNetworkConnection(addrConnect, (int)setConnected.size() >= std::min(nMaxConnections - 1, 2), &grant); } }