From 057d60781d16e320470958ac887a8c0624d7d2ff Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Thu, 13 Aug 2015 02:00:10 -0700 Subject: [PATCH 01/16] Refactor: AcceptConnection (modified to include 95a50390) --- src/net.cpp | 130 ++++++++++++++++++++++++++++------------------------ 1 file changed, 71 insertions(+), 59 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 412f4aa3d..05bb93870 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -671,6 +671,76 @@ 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 + { + // According to the internet TCP_NODELAY is not carried into accepted sockets + // on all platforms. Set it again here just to be sure. + int set = 1; +#ifdef WIN32 + setsockopt(hSocket, IPPROTO_TCP, TCP_NODELAY, (const char*)&set, sizeof(int)); +#else + setsockopt(hSocket, IPPROTO_TCP, TCP_NODELAY, (void*)&set, sizeof(int)); +#endif + + 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; @@ -828,65 +898,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; - - 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 >= nMaxConnections - MAX_OUTBOUND_CONNECTIONS) - { - LogPrint("net", "connection from %s dropped (full)\n", addr.ToString()); - CloseSocket(hSocket); - } - else if (CNode::IsBanned(addr) && !whitelisted) - { - LogPrintf("connection from %s dropped (banned)\n", addr.ToString()); - CloseSocket(hSocket); - } - else - { - // According to the internet TCP_NODELAY is not carried into accepted sockets - // on all platforms. Set it again here just to be sure. - int set = 1; -#ifdef WIN32 - setsockopt(hSocket, IPPROTO_TCP, TCP_NODELAY, (const char*)&set, sizeof(int)); -#else - setsockopt(hSocket, IPPROTO_TCP, TCP_NODELAY, (void*)&set, sizeof(int)); -#endif - - CNode* pnode = new CNode(hSocket, addr, "", true); - pnode->AddRef(); - pnode->fWhitelisted = whitelisted; - - { - LOCK(cs_vNodes); - vNodes.push_back(pnode); - } - } + AcceptConnection(hListenSocket); } } From 12005016cda5e0023579d3e53a537352448a19ad Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Thu, 13 Aug 2015 02:16:46 -0700 Subject: [PATCH 02/16] Refactor: Bail early in AcceptConnection --- src/net.cpp | 45 ++++++++++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 05bb93870..140fd44a3 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -696,48 +696,55 @@ 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 - { - // According to the internet TCP_NODELAY is not carried into accepted sockets - // on all platforms. Set it again here just to be sure. - int set = 1; + + // According to the internet TCP_NODELAY is not carried into accepted sockets + // on all platforms. Set it again here just to be sure. + int set = 1; #ifdef WIN32 - setsockopt(hSocket, IPPROTO_TCP, TCP_NODELAY, (const char*)&set, sizeof(int)); + setsockopt(hSocket, IPPROTO_TCP, TCP_NODELAY, (const char*)&set, sizeof(int)); #else - setsockopt(hSocket, IPPROTO_TCP, TCP_NODELAY, (void*)&set, sizeof(int)); + setsockopt(hSocket, IPPROTO_TCP, TCP_NODELAY, (void*)&set, sizeof(int)); #endif - CNode* pnode = new CNode(hSocket, addr, "", true); - pnode->AddRef(); - pnode->fWhitelisted = whitelisted; + CNode* pnode = new CNode(hSocket, addr, "", true); + pnode->AddRef(); + pnode->fWhitelisted = whitelisted; - LogPrint("net", "connection from %s accepted\n", addr.ToString()); + LogPrint("net", "connection from %s accepted\n", addr.ToString()); - { - LOCK(cs_vNodes); - vNodes.push_back(pnode); - } + { + LOCK(cs_vNodes); + vNodes.push_back(pnode); } } From bd80ec0d2379b44050d6f833b030684ffae7016a Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Thu, 13 Aug 2015 02:19:17 -0700 Subject: [PATCH 03/16] 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 140fd44a3..6662bf624 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -706,6 +706,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()); @@ -720,13 +727,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; - } - // According to the internet TCP_NODELAY is not carried into accepted sockets // on all platforms. Set it again here just to be sure. int set = 1; From e279e5f90aeaba08285a5c0c7a1b00cb3c2106b5 Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Thu, 13 Aug 2015 02:31:46 -0700 Subject: [PATCH 04/16] 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 b5d4f3d94..e4504c0f6 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -4937,6 +4937,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 17502b97e..28f44646c 100644 --- a/src/net.h +++ b/src/net.h @@ -318,6 +318,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 2fa41ff9b3c5a4a1791cec6d6c1e0c99a4d3ddf0 Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Thu, 13 Aug 2015 02:58:58 -0700 Subject: [PATCH 05/16] AttemptToEvictConnection --- src/net.cpp | 116 +++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 106 insertions(+), 10 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 6662bf624..53d649d1d 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -671,6 +671,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); @@ -715,16 +815,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; + } } // According to the internet TCP_NODELAY is not carried into accepted sockets From dd99be0f8c0d285f6345461e74332514afdd93d0 Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Thu, 13 Aug 2015 17:22:35 -0700 Subject: [PATCH 06/16] 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 53d649d1d..0a044c9de 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -713,7 +713,7 @@ public: } }; -static bool AttemptToEvictConnection() { +static bool AttemptToEvictConnection(bool fPreferNewConnection) { std::vector vEvictionCandidates; { LOCK(cs_vNodes); @@ -761,8 +761,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); @@ -815,7 +818,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 0560d671abc50eca967007323239af10b6350fd2 Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Thu, 13 Aug 2015 17:32:57 -0700 Subject: [PATCH 07/16] Remove redundant whiteconnections option --- src/init.cpp | 1 - src/net.h | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/src/init.cpp b/src/init.cpp index 6b3d9a1df..45dad5b58 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -337,7 +337,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")); - #ifdef ENABLE_WALLET strUsage += HelpMessageGroup(_("Wallet options:")); diff --git a/src/net.h b/src/net.h index 28f44646c..66f733e01 100644 --- a/src/net.h +++ b/src/net.h @@ -140,6 +140,7 @@ extern bool fListen; extern uint64_t nLocalServices; extern uint64_t nLocalHostNonce; extern CAddrMan addrman; +/** Maximum number of connections to simultaneously allow (aka connection slots) */ extern int nMaxConnections; extern std::vector vNodes; From 396bd999adfeba5f72f1ff608affb5368526c1bd Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Thu, 20 Aug 2015 16:47:49 -0700 Subject: [PATCH 08/16] Add comments to AttemptToEvictConnection --- src/net.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/net.cpp b/src/net.cpp index 0a044c9de..f7e2ae601 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -732,13 +732,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 6e966f67fd4d882ab36f4b7d1b020f320e245a49 Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Thu, 20 Aug 2015 17:29:04 -0700 Subject: [PATCH 09/16] 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 f7e2ae601..c726f3714 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -671,12 +671,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; } @@ -691,7 +702,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; @@ -714,7 +725,7 @@ public: }; static bool AttemptToEvictConnection(bool fPreferNewConnection) { - std::vector vEvictionCandidates; + std::vector vEvictionCandidates; { LOCK(cs_vNodes); @@ -727,7 +738,7 @@ static bool AttemptToEvictConnection(bool fPreferNewConnection) { continue; if (node->addr.IsLocal()) continue; - vEvictionCandidates.push_back(node); + vEvictionCandidates.push_back(CNodeRef(node)); } } @@ -755,8 +766,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 4dad09350f3d87d0140e422ee62241fe728ae1d4 Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Fri, 21 Aug 2015 18:42:05 -0700 Subject: [PATCH 10/16] 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 c726f3714..cbc15fffa 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -758,7 +758,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 ce94413e038ae150e26fe97538744755cae9b42a Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Sat, 22 Aug 2015 15:15:39 -0700 Subject: [PATCH 11/16] 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 cbc15fffa..ad52ab4d7 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -742,6 +742,8 @@ static bool AttemptToEvictConnection(bool fPreferNewConnection) { } } + if (vEvictionCandidates.empty()) return false; + // Protect connections with certain characteristics // Deterministically select 4 peers to protect by netgroup. @@ -750,18 +752,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 75c0598caccfed9f505ebd6c6e13f5dad9470bcc Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Tue, 25 Aug 2015 15:33:29 -0700 Subject: [PATCH 12/16] 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 ad52ab4d7..edfe4ada7 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -678,6 +678,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 f1e7e371306d683fbb0aba9603305b32e184d203 Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Tue, 25 Aug 2015 16:30:02 -0700 Subject: [PATCH 13/16] 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 edfe4ada7..e1b6176f3 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -673,8 +673,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;}; @@ -682,6 +689,8 @@ public: CNodeRef& operator =(const CNodeRef& other) { if (this != &other) { + LOCK(cs_vNodes); + _pnode->Release(); _pnode = other._pnode; _pnode->AddRef(); @@ -692,6 +701,7 @@ public: CNodeRef(const CNodeRef& other): _pnode(other._pnode) { + LOCK(cs_vNodes); _pnode->AddRef(); } private: From b63e75ae12414c831e947a59c7f1a477f339ede5 Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Tue, 25 Aug 2015 16:31:13 -0700 Subject: [PATCH 14/16] Fix comment --- src/net.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/net.cpp b/src/net.cpp index e1b6176f3..392bbec87 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -787,7 +787,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 85e84f5e5866b5f995a9db2119966f6a6aff8337 Mon Sep 17 00:00:00 2001 From: Patrick Strateman Date: Tue, 25 Aug 2015 17:06:15 -0700 Subject: [PATCH 15/16] 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 392bbec87..c2a202b2f 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -794,29 +794,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; From 9dcea38a6b44129b83e0503beb1ba1178856b19b Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Fri, 4 Sep 2015 15:43:21 +0200 Subject: [PATCH 16/16] net: correctly initialize nMinPingUsecTime `nMinPingUsecTime` was left uninitialized in CNode. The correct initialization for a minimum-until-now is int64_t's max value, so initialize it to that. Thanks @MarcoFalke for noticing. --- src/net.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/net.cpp b/src/net.cpp index c2a202b2f..0c3096405 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2147,6 +2147,7 @@ CNode::CNode(SOCKET hSocketIn, CAddress addrIn, std::string addrNameIn, bool fIn nPingUsecStart = 0; nPingUsecTime = 0; fPingQueued = false; + nMinPingUsecTime = std::numeric_limits::max(); { LOCK(cs_nLastNodeId);