2 files changed, 32 insertions, 1 deletions

diff --git a/src/net.cpp b/src/net.cpp
index 3af0189..7091981 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -1030,7 +1030,12 @@ void CNode::Disconnect()
 {
     LogPrint("net", "disconnecting node %s\n", addr.ToString().c_str());
 
+    // Invalidate before anything else can look at it. This used to close the
+    // socket and leave the handle number sitting in the object, so ~CNode
+    // closed it a second time and the select loop kept arming it in the
+    // meantime. See the note on ~CNode in net.h.
     BtfCloseSocket(hSocket);
+    hSocket = INVALID_SOCKET;
 
     // All of a nodes broadcasts and subscriptions are automatically torn down
     // when it goes down, so a node has to stay up to keep its broadcast going.
@@ -1210,6 +1215,11 @@ void ThreadSocketHandler2(void* parg)
             {
                 if (nWatched >= FD_SETSIZE)
                     break;
+                // A disconnected node stays in vNodes until its references go.
+                // Arming its closed handle asks select() to watch a number the
+                // operating system may already have given to somebody else.
+                if (pnode->hSocket == INVALID_SOCKET)
+                    continue;
                 FD_SET(pnode->hSocket, &fdsetRecv);
                 nWatched++;
                 hSocketMax = max(hSocketMax, pnode->hSocket);
@@ -1272,6 +1282,11 @@ void ThreadSocketHandler2(void* parg)
             {
                 foreach(CNode* pnode, vNodes)
                 {
+                    // Already disconnected: its handle is gone on purpose, and
+                    // reporting it here would name the wrong node for a select()
+                    // failure it did not cause.
+                    if (pnode->hSocket == INVALID_SOCKET)
+                        continue;
                     int nType = 0;
 #ifdef _WIN32
                     int nTypeLen = sizeof(nType);
diff --git a/src/net.h b/src/net.h
index 314c709..1c0c24e 100644
--- a/src/net.h
+++ b/src/net.h
@@ -638,10 +638,26 @@ public:
         PushMessage("version", VERSION, nLocalServices, nTime, addr, nBestHeight);
     }
 
+    // Disconnect() closes the socket and sets this to INVALID_SOCKET, so by the
+    // time a node is deleted there is normally nothing left to do here. It used
+    // not to: Disconnect() closed the handle and left the number in place, and
+    // this destructor closed that same number a second time -- with a raw
+    // closesocket() that the socket accounting never saw, so neither the double
+    // close nor the stale claim showed up in any report.
+    //
+    // A double close is not a harmless no-op. Between the two, the operating
+    // system is free to hand that number to a new socket -- this node opens
+    // hundreds a minute -- and the second close then takes down a connection
+    // that belongs to somebody else, which the log can only report as a peer
+    // vanishing for no reason. `dropping node: its socket is no longer valid`
+    // is what that looks like from the outside.
     ~CNode()
     {
         if (hSocket != INVALID_SOCKET)
-            closesocket(hSocket);
+        {
+            BtfCloseSocket(hSocket);
+            hSocket = INVALID_SOCKET;
+        }
     }
 
 private: