From d5bcd347cd8d156e3a6d4526fd2470741194a64a Mon Sep 17 00:00:00 2001 From: Bitflash Date: Fri, 31 Jul 2026 17:40:58 -0300 Subject: net: a node could only ever watch 63 peers (#78) --- src/makefile.mingw | 7 +++- src/net.cpp | 96 +++++++++++++++++++++++++++++++++++++++++++++++++----- src/net.h | 26 +++++++++++++++ 3 files changed, 120 insertions(+), 9 deletions(-) (limited to 'src') diff --git a/src/makefile.mingw b/src/makefile.mingw index 01f7702..ae533ef 100644 --- a/src/makefile.mingw +++ b/src/makefile.mingw @@ -14,8 +14,13 @@ WARNFLAGS = -Wformat -Werror=format \ -Wno-deprecated-declarations -Wno-write-strings \ -Wno-narrowing -Wno-unused-result +# FD_SETSIZE has to be defined before winsock2.h and it has to be the same +# number in every translation unit -- fd_set carries an array of that length, +# so disagreeing about it means disagreeing about sizeof(fd_set). That is why +# it lives here as a -D and not as a #define in a header. Windows defaults to +# 64, which capped this node at 63 watchable peers no matter how many it had. CXXFLAGS = -std=gnu++14 -O2 $(WARNFLAGS) -fpermissive -pthread \ - -DWIN32 -DNOPCH \ + -DWIN32 -DNOPCH -DFD_SETSIZE=1024 \ -DSECP256K1_STATIC \ -I$(DEPS)/include -I$(RANDOMX)/src \ -I$(DEPS)/include/nlohmann \ diff --git a/src/net.cpp b/src/net.cpp index 37c081f..5ef8316 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -954,8 +954,27 @@ void ThreadSocketHandler2(void* parg) vector vNodesCopy = vNodes; foreach(CNode* pnode, vNodesCopy) { - if (pnode->ReadyToDisconnect() && pnode->vRecv.empty() && pnode->vSend.empty()) + // Wait for the buffers to drain before letting a node go, so a + // last message still gets out -- but not forever. vSend never + // drains through a socket whose far end is gone, and a node + // stuck that way stayed in vNodes for the life of the process, + // holding its socket and counting as a peer. + if (!pnode->ReadyToDisconnect()) + pnode->nDisconnectSince = 0; + else if (pnode->nDisconnectSince == 0) + pnode->nDisconnectSince = GetTime(); + + bool fDrained = pnode->vRecv.empty() && pnode->vSend.empty(); + bool fStuck = pnode->nDisconnectSince != 0 && + GetTime() - pnode->nDisconnectSince > DISCONNECT_DRAIN_SECS; + + if (pnode->ReadyToDisconnect() && (fDrained || fStuck)) { + if (!fDrained) + printf("dropping stuck node %s after %d s with %d bytes unsent\n", + pnode->addr.ToString().c_str(), + (int)(GetTime() - pnode->nDisconnectSince), + (int)pnode->vSend.size()); // remove from vNodes vNodes.erase(remove(vNodes.begin(), vNodes.end(), pnode), vNodes.end()); pnode->Disconnect(); @@ -1010,16 +1029,35 @@ void ThreadSocketHandler2(void* parg) SOCKET hSocketMax = 0; FD_SET(hListenSocket, &fdsetRecv); hSocketMax = max(hSocketMax, hListenSocket); + // FD_SET drops silently once the set is full, so count what goes in + // and say so. A peer that never makes it into the set is never read, + // and that is indistinguishable from a peer with nothing to say -- + // the failure this whole path used to hide. + unsigned int nWatched = 1; // the listen socket CRITICAL_BLOCK(cs_vNodes) { foreach(CNode* pnode, vNodes) { + if (nWatched >= FD_SETSIZE) + break; FD_SET(pnode->hSocket, &fdsetRecv); + nWatched++; hSocketMax = max(hSocketMax, pnode->hSocket); TRY_CRITICAL_BLOCK(pnode->cs_vSend) if (!pnode->vSend.empty()) FD_SET(pnode->hSocket, &fdsetSend); } + if (vNodes.size() + 1 > (size_t)FD_SETSIZE) + { + static int64 nLastWarned = 0; + if (GetTime() - nLastWarned > 60) + { + nLastWarned = GetTime(); + printf("WARNING: %d peers but select() can only watch %d -- %d are not being read\n", + (int)vNodes.size(), (int)FD_SETSIZE - 1, + (int)vNodes.size() - (int)FD_SETSIZE + 1); + } + } } vfThreadRunning[0] = false; @@ -1028,12 +1066,37 @@ void ThreadSocketHandler2(void* parg) CheckForShutdown(0); if (nSelect == SOCKET_ERROR) { + // This used to answer a failed select() by marking every + // descriptor from 0 to hSocketMax ready, which is code written for + // POSIX, where a descriptor is a small integer. A Windows SOCKET is + // a kernel handle in the thousands, so the loop iterated over + // numbers that are not sockets and told the rest of the function + // that every peer had data. One bad handle in the set makes + // select() fail on every pass, so a single closed socket put the + // whole thread into that state permanently. + // + // Find the bad handle instead and let it be dropped. int nErr = WSAGetLastError(); LogPrint("net", "select failed: %d\n", nErr); - for (int i = 0; i <= hSocketMax; i++) + FD_ZERO(&fdsetRecv); + FD_ZERO(&fdsetSend); + CRITICAL_BLOCK(cs_vNodes) { - FD_SET(i, &fdsetRecv); - FD_SET(i, &fdsetSend); + foreach(CNode* pnode, vNodes) + { + int nType = 0; +#ifdef _WIN32 + int nTypeLen = sizeof(nType); +#else + socklen_t nTypeLen = sizeof(nType); +#endif + if (getsockopt(pnode->hSocket, SOL_SOCKET, SO_TYPE, (char*)&nType, &nTypeLen) != 0) + { + printf("dropping node %s: its socket is no longer valid\n", + pnode->addr.ToString().c_str()); + pnode->fDisconnect = true; + } + } } Sleep(timeout.tv_usec/1000); } @@ -1068,11 +1131,28 @@ void ThreadSocketHandler2(void* parg) } else { - LogPrint("net", "accepted connection from %s\n", addr.ToString().c_str()); - CNode* pnode = new CNode(hSocket, addr, true); - pnode->AddRef(); + // Refuse rather than accept a connection this node cannot + // watch. Without this the listen socket kept taking peers -- + // it is always in the select set -- while everything past the + // limit sat open and unread, which fed on itself: the deafer + // the node got, the more connections it collected. + unsigned int nNodes = 0; CRITICAL_BLOCK(cs_vNodes) - vNodes.push_back(pnode); + nNodes = (unsigned int)vNodes.size(); + if (nNodes >= MAX_CONNECTIONS) + { + LogPrint("net", "refusing connection from %s, already at %u\n", + addr.ToString().c_str(), nNodes); + closesocket(hSocket); + } + else + { + LogPrint("net", "accepted connection from %s\n", addr.ToString().c_str()); + CNode* pnode = new CNode(hSocket, addr, true); + pnode->AddRef(); + CRITICAL_BLOCK(cs_vNodes) + vNodes.push_back(pnode); + } } } diff --git a/src/net.h b/src/net.h index 5e828c3..01f314d 100644 --- a/src/net.h +++ b/src/net.h @@ -67,6 +67,30 @@ static const int BTF_RECV_TIMEOUT_SECS = 30 * 60; static const int BTF_SEND_STALL_SECS = 10 * 60; static const int BTF_HANDSHAKE_GRACE_SECS = 60; +// Ceiling on simultaneous connections. +// +// ThreadSocketHandler watches every peer through one select(), and select() +// cannot watch more descriptors than FD_SETSIZE. On Windows FD_SET simply +// stops adding once the set is full -- no error, no log -- so every socket +// past the limit stays open and is never read again. Nothing capped vNodes, +// so a node accepted connections it had no way to service, and because new +// entries go on the end of vNodes it was always the newest connection that +// starved, including the ones dialed to fix connectivity. +// +// Measured on two nodes before this ceiling existed: 277 and 90 CNode objects +// against 93 and 90 live sockets, and a status bar reporting hundreds of +// peers for a node that was really talking to thirty. +// +// 125 is Bitcoin's number and sits far below FD_SETSIZE on both platforms. +static const unsigned int MAX_CONNECTIONS = 125; + +// A node marked for disconnect is normally held until its buffers drain, so +// a last message still goes out. But vSend cannot drain through a socket +// whose far end is gone, and that made such a node immortal: it stayed in +// vNodes, kept its socket open, and kept being counted. Force it out after +// this long regardless of what is still buffered. +static const int64 DISCONNECT_DRAIN_SECS = 60; + // Send a ping after this long with nothing to say, so a peer running the // inactivity check above does not mistake a quiet node for a dead one. Must // stay well under BTF_RECV_TIMEOUT_SECS. @@ -520,6 +544,7 @@ protected: int nRefCount; public: int64 nReleaseTime; + int64 nDisconnectSince; map mapRequests; CCriticalSection cs_mapRequests; @@ -572,6 +597,7 @@ public: fDisconnect = false; nRefCount = 0; nReleaseTime = 0; + nDisconnectSince = 0; nLastPexRecv = 0; pindexLastGetBlocksBegin = NULL; hashLastGetBlocksEnd = 0; -- cgit v1.2.3