From 541a1dd9e664ac0b3929abeac42190ac8e88fc21 Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Thu, 13 Aug 2015 02:00:10 -0700 Subject: [PATCH 01/15] Refactor: AcceptConnection --- src/net.cpp | 120 +++++++++++++++++++++++++++------------------------- 1 file changed, 62 insertions(+), 58 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index fb5726a2b..8f1db2695 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -776,6 +776,67 @@ void SocketSendData(CNode *pnode) static list vNodesDisconnected; +static void AcceptConnection(const ListenSocket& hListenSocket) { + struct sockaddr_storage sockaddr; + socklen_t len = sizeof(sockaddr); + SOCKET hSocket = accept(hListenSocket.socket, (struct sockaddr*)&sockaddr, &len); + CAddress addr; + int nInbound = 0; + int nMaxInbound = nMaxConnections - MAX_OUTBOUND_CONNECTIONS; + + if (hSocket != INVALID_SOCKET) + if (!addr.SetSockAddr((const struct sockaddr*)&sockaddr)) + LogPrintf("Warning: Unknown socket family\n"); + + bool whitelisted = hListenSocket.whitelisted || CNode::IsWhitelistedRange(addr); + { + LOCK(cs_vNodes); + BOOST_FOREACH(CNode* pnode, vNodes) + if (pnode->fInbound) + nInbound++; + } + + if (hSocket == INVALID_SOCKET) + { + int nErr = WSAGetLastError(); + if (nErr != WSAEWOULDBLOCK) + LogPrintf("socket error accept failed: %s\n", NetworkErrorString(nErr)); + } + else if (!IsSelectableSocket(hSocket)) + { + LogPrintf("connection from %s dropped: non-selectable socket\n", addr.ToString()); + CloseSocket(hSocket); + } + else if (nInbound >= nMaxInbound) + { + LogPrint("net", "connection from %s dropped (full)\n", addr.ToString()); + CloseSocket(hSocket); + } + else if (!whitelisted && (nInbound >= (nMaxInbound - nWhiteConnections))) + { + LogPrint("net", "connection from %s dropped (non-whitelisted)\n", addr.ToString()); + CloseSocket(hSocket); + } + else if (CNode::IsBanned(addr) && !whitelisted) + { + LogPrintf("connection from %s dropped (banned)\n", addr.ToString()); + CloseSocket(hSocket); + } + else + { + CNode* pnode = new CNode(hSocket, addr, "", true); + pnode->AddRef(); + pnode->fWhitelisted = whitelisted; + + LogPrint("net", "connection from %s accepted\n", addr.ToString()); + + { + LOCK(cs_vNodes); + vNodes.push_back(pnode); + } + } +} + void ThreadSocketHandler() { unsigned int nPrevNodeCount = 0; @@ -933,64 +994,7 @@ void ThreadSocketHandler() { if (hListenSocket.socket != INVALID_SOCKET && FD_ISSET(hListenSocket.socket, &fdsetRecv)) { - struct sockaddr_storage sockaddr; - socklen_t len = sizeof(sockaddr); - SOCKET hSocket = accept(hListenSocket.socket, (struct sockaddr*)&sockaddr, &len); - CAddress addr; - int nInbound = 0; - int nMaxInbound = nMaxConnections - MAX_OUTBOUND_CONNECTIONS; - - if (hSocket != INVALID_SOCKET) - if (!addr.SetSockAddr((const struct sockaddr*)&sockaddr)) - LogPrintf("Warning: Unknown socket family\n"); - - bool whitelisted = hListenSocket.whitelisted || CNode::IsWhitelistedRange(addr); - { - LOCK(cs_vNodes); - BOOST_FOREACH(CNode* pnode, vNodes) - if (pnode->fInbound) - nInbound++; - } - - if (hSocket == INVALID_SOCKET) - { - int nErr = WSAGetLastError(); - if (nErr != WSAEWOULDBLOCK) - LogPrintf("socket error accept failed: %s\n", NetworkErrorString(nErr)); - } - else if (!IsSelectableSocket(hSocket)) - { - LogPrintf("connection from %s dropped: non-selectable socket\n", addr.ToString()); - CloseSocket(hSocket); - } - else if (nInbound >= nMaxInbound) - { - LogPrint("net", "connection from %s dropped (full)\n", addr.ToString()); - CloseSocket(hSocket); - } - else if (!whitelisted && (nInbound >= (nMaxInbound - nWhiteConnections))) - { - LogPrint("net", "connection from %s dropped (non-whitelisted)\n", addr.ToString()); - CloseSocket(hSocket); - } - else if (CNode::IsBanned(addr) && !whitelisted) - { - LogPrintf("connection from %s dropped (banned)\n", addr.ToString()); - CloseSocket(hSocket); - } - else - { - CNode* pnode = new CNode(hSocket, addr, "", true); - pnode->AddRef(); - pnode->fWhitelisted = whitelisted; - - LogPrint("net", "connection from %s accepted\n", addr.ToString()); - - { - LOCK(cs_vNodes); - vNodes.push_back(pnode); - } - } + AcceptConnection(hListenSocket); } } From 1ef481761477e26651dc56b4378b146cd5c416db Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Thu, 13 Aug 2015 02:16:46 -0700 Subject: [PATCH 02/15] Refactor: Bail early in AcceptConnection --- src/net.cpp | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 8f1db2695..6214c754c 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -801,39 +801,46 @@ static void AcceptConnection(const ListenSocket& hListenSocket) { int nErr = WSAGetLastError(); if (nErr != WSAEWOULDBLOCK) LogPrintf("socket error accept failed: %s\n", NetworkErrorString(nErr)); + return; } - else if (!IsSelectableSocket(hSocket)) + + if (!IsSelectableSocket(hSocket)) { LogPrintf("connection from %s dropped: non-selectable socket\n", addr.ToString()); CloseSocket(hSocket); + return; } - else if (nInbound >= nMaxInbound) + + if (nInbound >= nMaxInbound) { LogPrint("net", "connection from %s dropped (full)\n", addr.ToString()); CloseSocket(hSocket); + return; } - else if (!whitelisted && (nInbound >= (nMaxInbound - nWhiteConnections))) + + if (!whitelisted && (nInbound >= (nMaxInbound - nWhiteConnections))) { LogPrint("net", "connection from %s dropped (non-whitelisted)\n", addr.ToString()); CloseSocket(hSocket); + return; } - else if (CNode::IsBanned(addr) && !whitelisted) + + if (CNode::IsBanned(addr) && !whitelisted) { LogPrintf("connection from %s dropped (banned)\n", addr.ToString()); CloseSocket(hSocket); + return; } - else + + CNode* pnode = new CNode(hSocket, addr, "", true); + pnode->AddRef(); + pnode->fWhitelisted = whitelisted; + + LogPrint("net", "connection from %s accepted\n", addr.ToString()); + { - CNode* pnode = new CNode(hSocket, addr, "", true); - pnode->AddRef(); - pnode->fWhitelisted = whitelisted; - - LogPrint("net", "connection from %s accepted\n", addr.ToString()); - - { - LOCK(cs_vNodes); - vNodes.push_back(pnode); - } + LOCK(cs_vNodes); + vNodes.push_back(pnode); } } From ae037b707ce164087790f149c048871c66e14cfd Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Thu, 13 Aug 2015 02:19:17 -0700 Subject: [PATCH 03/15] Refactor: Move failure conditions to the top of AcceptConnection --- src/net.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 6214c754c..248aedfa1 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -811,6 +811,13 @@ static void AcceptConnection(const ListenSocket& hListenSocket) { return; } + if (CNode::IsBanned(addr) && !whitelisted) + { + LogPrintf("connection from %s dropped (banned)\n", addr.ToString()); + CloseSocket(hSocket); + return; + } + if (nInbound >= nMaxInbound) { LogPrint("net", "connection from %s dropped (full)\n", addr.ToString()); @@ -825,13 +832,6 @@ static void AcceptConnection(const ListenSocket& hListenSocket) { return; } - if (CNode::IsBanned(addr) && !whitelisted) - { - LogPrintf("connection from %s dropped (banned)\n", addr.ToString()); - CloseSocket(hSocket); - return; - } - CNode* pnode = new CNode(hSocket, addr, "", true); pnode->AddRef(); pnode->fWhitelisted = whitelisted; From 4bac60161029de6d71ef1e51e7af803ce6fb8405 Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Thu, 13 Aug 2015 02:31:46 -0700 Subject: [PATCH 04/15] Record nMinPingUsecTime --- src/main.cpp | 1 + src/net.h | 2 ++ 2 files changed, 3 insertions(+) diff --git a/src/main.cpp b/src/main.cpp index 33b57a528..35fbec666 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -4522,6 +4522,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, if (pingUsecTime > 0) { // Successful ping time measurement, replace previous pfrom->nPingUsecTime = pingUsecTime; + pfrom->nMinPingUsecTime = std::min(pfrom->nMinPingUsecTime, pingUsecTime); } else { // This should never happen sProblem = "Timing mishap"; diff --git a/src/net.h b/src/net.h index 954cdd49d..de2955732 100644 --- a/src/net.h +++ b/src/net.h @@ -395,6 +395,8 @@ public: int64_t nPingUsecStart; // Last measured round-trip time. int64_t nPingUsecTime; + // Best measured round-trip time. + int64_t nMinPingUsecTime; // Whether a ping is requested. bool fPingQueued; From 2c701537c8fc7f4cfb0163ec1f49662120e61eb7 Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Thu, 13 Aug 2015 02:58:58 -0700 Subject: [PATCH 05/15] AttemptToEvictConnection --- src/net.cpp | 116 +++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 106 insertions(+), 10 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 248aedfa1..0fab0f82a 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -776,6 +776,106 @@ void SocketSendData(CNode *pnode) static list vNodesDisconnected; +static bool ReverseCompareNodeMinPingTime(CNode *a, CNode *b) +{ + return a->nMinPingUsecTime > b->nMinPingUsecTime; +} + +static bool ReverseCompareNodeTimeConnected(CNode *a, CNode *b) +{ + return a->nTimeConnected > b->nTimeConnected; +} + +class CompareNetGroupKeyed +{ + std::vector vchSecretKey; +public: + CompareNetGroupKeyed() + { + vchSecretKey.resize(32, 0); + GetRandBytes(vchSecretKey.data(), vchSecretKey.size()); + } + + bool operator()(CNode *a, CNode *b) + { + std::vector vchGroupA, vchGroupB; + CSHA256 hashA, hashB; + std::vector vchA(32), vchB(32); + + vchGroupA = a->addr.GetGroup(); + vchGroupB = b->addr.GetGroup(); + + hashA.Write(begin_ptr(vchGroupA), vchGroupA.size()); + hashB.Write(begin_ptr(vchGroupB), vchGroupB.size()); + + hashA.Write(begin_ptr(vchSecretKey), vchSecretKey.size()); + hashB.Write(begin_ptr(vchSecretKey), vchSecretKey.size()); + + hashA.Finalize(begin_ptr(vchA)); + hashB.Finalize(begin_ptr(vchB)); + + return vchA < vchB; + } +}; + +static bool AttemptToEvictConnection() { + std::vector vEvictionCandidates; + { + LOCK(cs_vNodes); + + BOOST_FOREACH(CNode *node, vNodes) { + if (node->fWhitelisted) + continue; + if (!node->fInbound) + continue; + if (node->fDisconnect) + continue; + if (node->addr.IsLocal()) + continue; + vEvictionCandidates.push_back(node); + } + } + + // Protect connections with certain characteristics + static CompareNetGroupKeyed comparerNetGroupKeyed; + std::sort(vEvictionCandidates.begin(), vEvictionCandidates.end(), comparerNetGroupKeyed); + vEvictionCandidates.erase(vEvictionCandidates.end() - std::min(4, static_cast(vEvictionCandidates.size())), vEvictionCandidates.end()); + + std::sort(vEvictionCandidates.begin(), vEvictionCandidates.end(), ReverseCompareNodeMinPingTime); + vEvictionCandidates.erase(vEvictionCandidates.end() - std::min(8, static_cast(vEvictionCandidates.size())), vEvictionCandidates.end()); + + std::sort(vEvictionCandidates.begin(), vEvictionCandidates.end(), ReverseCompareNodeTimeConnected); + vEvictionCandidates.erase(vEvictionCandidates.end() - std::min(64, static_cast(vEvictionCandidates.size())), vEvictionCandidates.end()); + + if (vEvictionCandidates.empty()) + return false; + + // Identify CNetAddr with the most connections + CNetAddr naMostConnections; + unsigned int nMostConnections = 0; + std::map > mapAddrCounts; + BOOST_FOREACH(CNode *node, vEvictionCandidates) { + mapAddrCounts[node->addr].push_back(node); + + if (mapAddrCounts[node->addr].size() > nMostConnections) { + nMostConnections = mapAddrCounts[node->addr].size(); + naMostConnections = node->addr; + } + } + + // Reduce to the CNetAddr with the most connections + vEvictionCandidates = mapAddrCounts[naMostConnections]; + + if (vEvictionCandidates.size() <= 1) + return false; + + // Disconnect the most recent connection from the CNetAddr with the most connections + std::sort(vEvictionCandidates.begin(), vEvictionCandidates.end(), ReverseCompareNodeTimeConnected); + vEvictionCandidates[0]->fDisconnect = true; + + return true; +} + static void AcceptConnection(const ListenSocket& hListenSocket) { struct sockaddr_storage sockaddr; socklen_t len = sizeof(sockaddr); @@ -820,16 +920,12 @@ static void AcceptConnection(const ListenSocket& hListenSocket) { if (nInbound >= nMaxInbound) { - LogPrint("net", "connection from %s dropped (full)\n", addr.ToString()); - CloseSocket(hSocket); - return; - } - - if (!whitelisted && (nInbound >= (nMaxInbound - nWhiteConnections))) - { - LogPrint("net", "connection from %s dropped (non-whitelisted)\n", addr.ToString()); - CloseSocket(hSocket); - return; + if (!AttemptToEvictConnection()) { + // No connection to evict, disconnect the new connection + LogPrint("net", "failed to find an eviction candidate - connection dropped (full)\n"); + CloseSocket(hSocket); + return; + } } CNode* pnode = new CNode(hSocket, addr, "", true); From b105ba398b20789eb482e45f26ae1761800b81be Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Thu, 13 Aug 2015 17:22:35 -0700 Subject: [PATCH 06/15] Prefer to disconnect peers in favor of whitelisted peers --- src/net.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 0fab0f82a..77dde9944 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -818,7 +818,7 @@ public: } }; -static bool AttemptToEvictConnection() { +static bool AttemptToEvictConnection(bool fPreferNewConnection) { std::vector vEvictionCandidates; { LOCK(cs_vNodes); @@ -866,8 +866,11 @@ static bool AttemptToEvictConnection() { // Reduce to the CNetAddr with the most connections vEvictionCandidates = mapAddrCounts[naMostConnections]; + // Do not disconnect peers who have only 1 evictable connection if (vEvictionCandidates.size() <= 1) - return false; + // unless we prefer the new connection (for whitelisted peers) + if (!fPreferNewConnection) + return false; // Disconnect the most recent connection from the CNetAddr with the most connections std::sort(vEvictionCandidates.begin(), vEvictionCandidates.end(), ReverseCompareNodeTimeConnected); @@ -920,7 +923,7 @@ static void AcceptConnection(const ListenSocket& hListenSocket) { if (nInbound >= nMaxInbound) { - if (!AttemptToEvictConnection()) { + if (!AttemptToEvictConnection(whitelisted)) { // No connection to evict, disconnect the new connection LogPrint("net", "failed to find an eviction candidate - connection dropped (full)\n"); CloseSocket(hSocket); From a8f6e45249e815414cc99e7b594a8a7ab7ab9247 Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Thu, 13 Aug 2015 17:32:57 -0700 Subject: [PATCH 07/15] Remove redundant whiteconnections option --- src/init.cpp | 29 ----------------------------- src/net.cpp | 1 - src/net.h | 11 ----------- 3 files changed, 41 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 085e04fdf..3aebe4f7b 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -335,7 +335,6 @@ std::string HelpMessage(HelpMessageMode mode) strUsage += HelpMessageOpt("-whitebind=", _("Bind to given address and whitelist peers connecting to it. Use [host]:port notation for IPv6")); strUsage += HelpMessageOpt("-whitelist=", _("Whitelist peers connecting from the given netmask or IP address. Can be specified multiple times.") + " " + _("Whitelisted peers cannot be DoS banned and their transactions are always relayed, even if they are already in the mempool, useful e.g. for a gateway")); - strUsage += HelpMessageOpt("-whiteconnections=", strprintf(_("Reserve this many inbound connections for whitelisted peers (default: %d)"), 0)); #ifdef ENABLE_WALLET strUsage += HelpMessageGroup(_("Wallet options:")); @@ -754,25 +753,6 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler) int nBind = std::max((int)mapArgs.count("-bind") + (int)mapArgs.count("-whitebind"), 1); int nUserMaxConnections = GetArg("-maxconnections", DEFAULT_MAX_PEER_CONNECTIONS); nMaxConnections = std::max(nUserMaxConnections, 0); - int nUserWhiteConnections = GetArg("-whiteconnections", 0); - nWhiteConnections = std::max(nUserWhiteConnections, 0); - - if ((mapArgs.count("-whitelist")) || (mapArgs.count("-whitebind"))) { - if (!(mapArgs.count("-maxconnections"))) { - // User is using whitelist feature, - // but did not specify -maxconnections parameter. - // Silently increase the default to compensate, - // so that the whitelist connection reservation feature - // does not inadvertently reduce the default - // inbound connection capacity of the network. - nMaxConnections += nWhiteConnections; - } - } else { - // User not using whitelist feature. - // Silently disable connection reservation, - // for the same reason as above. - nWhiteConnections = 0; - } // Trim requested connection counts, to fit into system limitations nMaxConnections = std::max(std::min(nMaxConnections, (int)(FD_SETSIZE - nBind - MIN_CORE_FILEDESCRIPTORS)), 0); @@ -784,13 +764,6 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler) if (nMaxConnections < nUserMaxConnections) InitWarning(strprintf(_("Reducing -maxconnections from %d to %d, because of system limitations."), nUserMaxConnections, nMaxConnections)); - // Connection capacity is prioritized in this order: - // outbound connections (hardcoded to 8), - // then whitelisted connections, - // then non-whitelisted connections get whatever's left (if any). - if ((nWhiteConnections > 0) && (nWhiteConnections >= (nMaxConnections - 8))) - InitWarning(strprintf(_("All non-whitelisted incoming connections will be dropped, because -whiteconnections is %d and -maxconnections is only %d."), nWhiteConnections, nMaxConnections)); - // ********************************************************* Step 3: parameter-to-internal-flags fDebug = !mapMultiArgs["-debug"].empty(); @@ -968,8 +941,6 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler) LogPrintf("Using data directory %s\n", strDataDir); LogPrintf("Using config file %s\n", GetConfigFile().string()); LogPrintf("Using at most %i connections (%i file descriptors available)\n", nMaxConnections, nFD); - if (nWhiteConnections > 0) - LogPrintf("Reserving %i of these connections for whitelisted inbound peers\n", nWhiteConnections); std::ostringstream strErrors; LogPrintf("Using %u threads for script verification\n", nScriptCheckThreads); diff --git a/src/net.cpp b/src/net.cpp index 77dde9944..9cfb9d71d 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -81,7 +81,6 @@ uint64_t nLocalHostNonce = 0; static std::vector vhListenSocket; CAddrMan addrman; int nMaxConnections = DEFAULT_MAX_PEER_CONNECTIONS; -int nWhiteConnections = 0; bool fAddressesInitialized = false; std::string strSubVersion; diff --git a/src/net.h b/src/net.h index de2955732..f370bf1ff 100644 --- a/src/net.h +++ b/src/net.h @@ -143,19 +143,8 @@ extern uint64_t nLocalServices; extern uint64_t nLocalHostNonce; extern CAddrMan addrman; -// The allocation of connections against the maximum allowed (nMaxConnections) -// is prioritized as follows: -// 1st: Outbound connections (MAX_OUTBOUND_CONNECTIONS) -// 2nd: Inbound connections from whitelisted peers (nWhiteConnections) -// 3rd: Inbound connections from non-whitelisted peers -// Thus, the number of connection slots for the general public to use is: -// nMaxConnections - (MAX_OUTBOUND_CONNECTIONS + nWhiteConnections) -// Any additional inbound connections beyond limits will be immediately closed - /** Maximum number of connections to simultaneously allow (aka connection slots) */ extern int nMaxConnections; -/** Number of connection slots to reserve for inbound from whitelisted peers */ -extern int nWhiteConnections; extern std::vector vNodes; extern CCriticalSection cs_vNodes; From df239374224e6585d5b6ba37a39282d0fc647173 Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Thu, 20 Aug 2015 16:47:49 -0700 Subject: [PATCH 08/15] Add comments to AttemptToEvictConnection --- src/net.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/net.cpp b/src/net.cpp index 9cfb9d71d..d8d2783c4 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -836,13 +836,20 @@ static bool AttemptToEvictConnection(bool fPreferNewConnection) { } // Protect connections with certain characteristics + + // Deterministically select 4 peers to protect by netgroup. + // An attacker cannot predict which netgroups will be protected. static CompareNetGroupKeyed comparerNetGroupKeyed; std::sort(vEvictionCandidates.begin(), vEvictionCandidates.end(), comparerNetGroupKeyed); vEvictionCandidates.erase(vEvictionCandidates.end() - std::min(4, static_cast(vEvictionCandidates.size())), vEvictionCandidates.end()); + // Protect the 8 nodes with the best ping times. + // An attacker cannot manipulate this metric without physically moving nodes closer to the target. std::sort(vEvictionCandidates.begin(), vEvictionCandidates.end(), ReverseCompareNodeMinPingTime); vEvictionCandidates.erase(vEvictionCandidates.end() - std::min(8, static_cast(vEvictionCandidates.size())), vEvictionCandidates.end()); + // Protect the 64 nodes which have been connected the longest. + // This replicates the existing implicit behavior. std::sort(vEvictionCandidates.begin(), vEvictionCandidates.end(), ReverseCompareNodeTimeConnected); vEvictionCandidates.erase(vEvictionCandidates.end() - std::min(64, static_cast(vEvictionCandidates.size())), vEvictionCandidates.end()); From 1317cd1928afbae14fedb39c8d23589a32fe2951 Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Thu, 20 Aug 2015 17:29:04 -0700 Subject: [PATCH 09/15] RAII wrapper for CNode* --- src/net.cpp | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index d8d2783c4..709c65243 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -775,12 +775,23 @@ void SocketSendData(CNode *pnode) static list vNodesDisconnected; -static bool ReverseCompareNodeMinPingTime(CNode *a, CNode *b) +class CNodeRef { +public: + CNodeRef(CNode *pnode) : _pnode(pnode) {_pnode->AddRef();} + ~CNodeRef() {_pnode->Release();} + + CNode& operator *() const {return *_pnode;}; + CNode* operator ->() const {return _pnode;}; +private: + CNode *_pnode; +}; + +static bool ReverseCompareNodeMinPingTime(const CNodeRef &a, const CNodeRef &b) { return a->nMinPingUsecTime > b->nMinPingUsecTime; } -static bool ReverseCompareNodeTimeConnected(CNode *a, CNode *b) +static bool ReverseCompareNodeTimeConnected(const CNodeRef &a, const CNodeRef &b) { return a->nTimeConnected > b->nTimeConnected; } @@ -795,7 +806,7 @@ public: GetRandBytes(vchSecretKey.data(), vchSecretKey.size()); } - bool operator()(CNode *a, CNode *b) + bool operator()(const CNodeRef &a, const CNodeRef &b) { std::vector vchGroupA, vchGroupB; CSHA256 hashA, hashB; @@ -818,7 +829,7 @@ public: }; static bool AttemptToEvictConnection(bool fPreferNewConnection) { - std::vector vEvictionCandidates; + std::vector vEvictionCandidates; { LOCK(cs_vNodes); @@ -831,7 +842,7 @@ static bool AttemptToEvictConnection(bool fPreferNewConnection) { continue; if (node->addr.IsLocal()) continue; - vEvictionCandidates.push_back(node); + vEvictionCandidates.push_back(CNodeRef(node)); } } @@ -859,8 +870,8 @@ static bool AttemptToEvictConnection(bool fPreferNewConnection) { // Identify CNetAddr with the most connections CNetAddr naMostConnections; unsigned int nMostConnections = 0; - std::map > mapAddrCounts; - BOOST_FOREACH(CNode *node, vEvictionCandidates) { + std::map > mapAddrCounts; + BOOST_FOREACH(const CNodeRef &node, vEvictionCandidates) { mapAddrCounts[node->addr].push_back(node); if (mapAddrCounts[node->addr].size() > nMostConnections) { From 17f3533c8484f02732fff5cf004d251c0df50eb8 Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Fri, 21 Aug 2015 18:42:05 -0700 Subject: [PATCH 10/15] Better support for nodes with non-standard nMaxConnections --- src/net.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/net.cpp b/src/net.cpp index 709c65243..4d08f63e3 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -862,7 +862,7 @@ static bool AttemptToEvictConnection(bool fPreferNewConnection) { // Protect the 64 nodes which have been connected the longest. // This replicates the existing implicit behavior. std::sort(vEvictionCandidates.begin(), vEvictionCandidates.end(), ReverseCompareNodeTimeConnected); - vEvictionCandidates.erase(vEvictionCandidates.end() - std::min(64, static_cast(vEvictionCandidates.size())), vEvictionCandidates.end()); + vEvictionCandidates.erase(vEvictionCandidates.end() - std::min(static_cast(vEvictionCandidates.size() / 2), static_cast(vEvictionCandidates.size())), vEvictionCandidates.end()); if (vEvictionCandidates.empty()) return false; From dc81dd02a1d5f47ca45f74577e0696dfba6fa15c Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Sat, 22 Aug 2015 15:15:39 -0700 Subject: [PATCH 11/15] Return false early if vEvictionCandidates is empty --- src/net.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 4d08f63e3..4f4c7b81c 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -846,6 +846,8 @@ static bool AttemptToEvictConnection(bool fPreferNewConnection) { } } + if (vEvictionCandidates.empty()) return false; + // Protect connections with certain characteristics // Deterministically select 4 peers to protect by netgroup. @@ -854,18 +856,21 @@ static bool AttemptToEvictConnection(bool fPreferNewConnection) { std::sort(vEvictionCandidates.begin(), vEvictionCandidates.end(), comparerNetGroupKeyed); vEvictionCandidates.erase(vEvictionCandidates.end() - std::min(4, static_cast(vEvictionCandidates.size())), vEvictionCandidates.end()); + if (vEvictionCandidates.empty()) return false; + // Protect the 8 nodes with the best ping times. // An attacker cannot manipulate this metric without physically moving nodes closer to the target. std::sort(vEvictionCandidates.begin(), vEvictionCandidates.end(), ReverseCompareNodeMinPingTime); vEvictionCandidates.erase(vEvictionCandidates.end() - std::min(8, static_cast(vEvictionCandidates.size())), vEvictionCandidates.end()); + if (vEvictionCandidates.empty()) return false; + // Protect the 64 nodes which have been connected the longest. // This replicates the existing implicit behavior. std::sort(vEvictionCandidates.begin(), vEvictionCandidates.end(), ReverseCompareNodeTimeConnected); - vEvictionCandidates.erase(vEvictionCandidates.end() - std::min(static_cast(vEvictionCandidates.size() / 2), static_cast(vEvictionCandidates.size())), vEvictionCandidates.end()); + vEvictionCandidates.erase(vEvictionCandidates.end() - static_cast(vEvictionCandidates.size() / 2), vEvictionCandidates.end()); - if (vEvictionCandidates.empty()) - return false; + if (vEvictionCandidates.empty()) return false; // Identify CNetAddr with the most connections CNetAddr naMostConnections; From 69ee1aab00b9189865dfca6fb5c33c61a3c3ea67 Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Tue, 25 Aug 2015 15:33:29 -0700 Subject: [PATCH 12/15] CNodeRef copy constructor and assignment operator --- src/net.cpp | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/net.cpp b/src/net.cpp index 4f4c7b81c..cb5a24f0a 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -782,6 +782,22 @@ public: CNode& operator *() const {return *_pnode;}; CNode* operator ->() const {return _pnode;}; + + CNodeRef& operator =(const CNodeRef& other) + { + if (this != &other) { + _pnode->Release(); + _pnode = other._pnode; + _pnode->AddRef(); + } + return *this; + } + + CNodeRef(const CNodeRef& other): + _pnode(other._pnode) + { + _pnode->AddRef(); + } private: CNode *_pnode; }; From fed30940ef22f242b9dada2dc4f7c5348faf8922 Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Tue, 25 Aug 2015 16:30:02 -0700 Subject: [PATCH 13/15] Acquire cs_vNodes before changing refrence counts --- src/net.cpp | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index cb5a24f0a..e1b0e83e9 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -777,8 +777,15 @@ static list vNodesDisconnected; class CNodeRef { public: - CNodeRef(CNode *pnode) : _pnode(pnode) {_pnode->AddRef();} - ~CNodeRef() {_pnode->Release();} + CNodeRef(CNode *pnode) : _pnode(pnode) { + LOCK(cs_vNodes); + _pnode->AddRef(); + } + + ~CNodeRef() { + LOCK(cs_vNodes); + _pnode->Release(); + } CNode& operator *() const {return *_pnode;}; CNode* operator ->() const {return _pnode;}; @@ -786,6 +793,8 @@ public: CNodeRef& operator =(const CNodeRef& other) { if (this != &other) { + LOCK(cs_vNodes); + _pnode->Release(); _pnode = other._pnode; _pnode->AddRef(); @@ -796,6 +805,7 @@ public: CNodeRef(const CNodeRef& other): _pnode(other._pnode) { + LOCK(cs_vNodes); _pnode->AddRef(); } private: From 000c18aaceeebdcaf65508fcdc3d00397971dcae Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Tue, 25 Aug 2015 16:31:13 -0700 Subject: [PATCH 14/15] Fix comment --- src/net.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/net.cpp b/src/net.cpp index e1b0e83e9..d266b2f21 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -891,7 +891,7 @@ static bool AttemptToEvictConnection(bool fPreferNewConnection) { if (vEvictionCandidates.empty()) return false; - // Protect the 64 nodes which have been connected the longest. + // Protect the half of the remaining nodes which have been connected the longest. // This replicates the existing implicit behavior. std::sort(vEvictionCandidates.begin(), vEvictionCandidates.end(), ReverseCompareNodeTimeConnected); vEvictionCandidates.erase(vEvictionCandidates.end() - static_cast(vEvictionCandidates.size() / 2), vEvictionCandidates.end()); From 027de94e1fba5484aed2393afd89edbaaffdb0eb Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Tue, 25 Aug 2015 17:06:15 -0700 Subject: [PATCH 15/15] Use network group instead of CNetAddr in final pass to select node to disconnect --- src/net.cpp | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index d266b2f21..9264d6865 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -898,29 +898,29 @@ static bool AttemptToEvictConnection(bool fPreferNewConnection) { if (vEvictionCandidates.empty()) return false; - // Identify CNetAddr with the most connections - CNetAddr naMostConnections; + // Identify the network group with the most connections + std::vector naMostConnections; unsigned int nMostConnections = 0; - std::map > mapAddrCounts; + std::map, std::vector > mapAddrCounts; BOOST_FOREACH(const CNodeRef &node, vEvictionCandidates) { - mapAddrCounts[node->addr].push_back(node); + mapAddrCounts[node->addr.GetGroup()].push_back(node); - if (mapAddrCounts[node->addr].size() > nMostConnections) { - nMostConnections = mapAddrCounts[node->addr].size(); - naMostConnections = node->addr; + if (mapAddrCounts[node->addr.GetGroup()].size() > nMostConnections) { + nMostConnections = mapAddrCounts[node->addr.GetGroup()].size(); + naMostConnections = node->addr.GetGroup(); } } - // Reduce to the CNetAddr with the most connections + // Reduce to the network group with the most connections vEvictionCandidates = mapAddrCounts[naMostConnections]; - // Do not disconnect peers who have only 1 evictable connection + // Do not disconnect peers if there is only 1 connection from their network group if (vEvictionCandidates.size() <= 1) // unless we prefer the new connection (for whitelisted peers) if (!fPreferNewConnection) return false; - // Disconnect the most recent connection from the CNetAddr with the most connections + // Disconnect the most recent connection from the network group with the most connections std::sort(vEvictionCandidates.begin(), vEvictionCandidates.end(), ReverseCompareNodeTimeConnected); vEvictionCandidates[0]->fDisconnect = true;