net: stop closing a descriptor number that now belongs to somebody else

THE WEDGE.

CNode::Disconnect() closed hSocket and left the member holding the stale
number. ~CNode's only guard is 'hSocket != INVALID_SOCKET' and nothing in
the tree ever assigned that sentinel, so the destructor closed the same
number a second time.

The two closes are five minutes apart by design: ThreadSocketHandler2
disconnects a node, sets nReleaseTime to GetTime() + 5*60, and parks it in
vNodesDisconnected; GetRefCount() reports it busy until that expires, so
'delete pnode' -- and the second close -- cannot run before then.

Linux hands out the lowest free descriptor. Measured on the pool: 202 open
descriptors with a high-water mark of 216, so a freed number is reissued
within seconds -- to a Nostr relay TLS socket, a rendezvous socket, a
tunnel end, or a miner connection. Five minutes later the destructor
closes it. The victim gets no error and no log line. It simply stops.

That is the whole signature, including the part that never made sense:
.btf block delivery and Nostr descriptor lookup share no lock, no queue
and no thread, so they should not fail together. They share the file
descriptor table, and a stray close is drawn from all of it.

It also explains why #78 did not help and may have hurt. #78 drops stuck
nodes -- it fired 41 times in one window -- and every drop is another
Disconnect(), which is another delayed close armed against an innocent
socket.

Second bug, same symptom, different subsystem: the pooled relay sockets.

ResolveDescriptor returned the instant a descriptor verified, leaving the
relay's EOSE unread on a socket that goes straight back into g_relayPool,
so the next caller read our leftovers as its own answer -- permanently one
message out of step, and no code path anywhere inspected the subscription
id that would have caught it. Worse, BtfResolve set lease.fOk = true on
every failure path including a dead RecvText, labelling it 'exchange
completed, just no match'. fOk = false is the only thing that ever closes
a pooled socket or clears fConnected, so a relay connection that died was
pinned in the pool, reported healthy, and reused for the life of the
process. That is 'no Nostr descriptor found' for a descriptor that nak can
see, cleared only by a restart.

Now: read to EOSE before returning, match the subscription id, and keep a
pooled socket only when the relay actually finished the conversation.

(cherry picked from commit ed4640c306cc46dbdbd92cb6dc55e7528651269c)

1 files changed, 45 insertions, 11 deletions

diff --git a/src/nostr.cpp b/src/nostr.cpp
index b3e4a7c..3f43302 100644
--- a/src/nostr.cpp
+++ b/src/nostr.cpp
@@ -1057,9 +1057,14 @@ static void HandlePoolAnnouncement(const json& ev)
 
 // Resolve a `.btf` address via a relay: fetch the descriptor published by that
 // address's key and verify it self-certifies under the decoded pubkey.
+// fExchangeOk reports whether the relay actually finished the conversation
+// (an EOSE arrived) as opposed to the socket dying mid-read. The caller pools
+// this socket for reuse, and only a completed exchange leaves it in a state
+// the next caller can trust.
 static bool ResolveDescriptor(CWebSocket& ws, void* ctx, const string& btfAddr,
-                              btf::Descriptor& out)
+                              btf::Descriptor& out, bool& fExchangeOk)
 {
+    fExchangeOk = false;
     unsigned char pk[32];
     if (!btf::ParseAddress(btfAddr, pk))
         return false;
@@ -1070,26 +1075,42 @@ static bool ResolveDescriptor(CWebSocket& ws, void* ctx, const string& btfAddr,
     json req = json::array({ "REQ", "btf-resolve", filter });
     if (!ws.SendText(req.dump()))
         return false;
+    bool fFound = false;
     for (;;)
     {
         string msg;
         if (!ws.RecvText(msg))
-            break;
+            return fFound;  // socket died mid-exchange; fExchangeOk stays false
         json j;
         try { j = json::parse(msg); } catch (...) { continue; }
         if (!j.is_array() || j.empty() || !j[0].is_string()) continue;
         string t = j[0].get<string>();
+        // Every relay message names the subscription it belongs to. This socket
+        // is pooled and reused, so a message left over from an earlier exchange
+        // is the ordinary case here, not an exotic one -- and an unmatched EOSE
+        // ends this query before it has begun, which is indistinguishable from
+        // "that descriptor does not exist". Check the id.
+        if (j.size() >= 2 && j[1].is_string() && j[1].get<string>() != "btf-resolve")
+            continue;
         if (t == "EVENT" && j.size() >= 3)
         {
             const json& ev = j[2];
-            if (ev.contains("content") &&
+            if (!fFound && ev.contains("content") &&
                 btf::VerifyDescriptor(ctx, ev["content"].get<string>(), pk, out))
-                return true;
+                fFound = true;
+            // Deliberately keep reading to EOSE rather than returning here.
+            // Returning early leaves the rest of the relay's answer queued on a
+            // socket that goes straight back into the pool, and the next caller
+            // reads our leftovers as its own -- one message out of step, for
+            // the life of the process.
         }
         else if (t == "EOSE")
+        {
+            fExchangeOk = true;  // relay said "that is all": socket is clean
             break;
+        }
     }
-    return false;
+    return fFound;
 }
 
 // Resolve a `.btf` address into its rendezvous coordinates by querying the
@@ -1108,12 +1129,22 @@ bool BtfResolve(const std::string& btfAddr, std::string& meetingHostPort,
             if (!lease.Connected())
                 continue;
             btf::Descriptor d;
-            if (!ResolveDescriptor(lease.Ws(), g_nostrKey.ctx, btfAddr, d))
+            bool fExchangeOk = false;
+            if (!ResolveDescriptor(lease.Ws(), g_nostrKey.ctx, btfAddr, d, fExchangeOk))
             {
-                lease.fOk = true; // exchange completed, just no match
+                // Only keep the pooled socket if the relay actually finished
+                // the conversation. This used to say "exchange completed, just
+                // no match" on every failure -- including a dead socket -- so
+                // ~CRelayLease never closed it and fConnected was never
+                // cleared. A relay connection that died was then pinned in the
+                // pool, reported healthy, and reused for every later lookup,
+                // which is why "no Nostr descriptor found" persisted for a
+                // descriptor that was demonstrably published, and why only a
+                // restart cleared it.
+                lease.fOk = fExchangeOk;
                 continue;
             }
-            lease.fOk = true;
+            lease.fOk = fExchangeOk;
             if (d.meeting_node.empty() || d.meeting_node == "rendezvous-pending")
                 continue; // descriptor predates the owner's relay config
             if (!HexDecode(d.enc, enc_pub, 32))
@@ -1761,14 +1792,17 @@ void ThreadNostrSeed(void* parg)
             PublishDescriptor(ws, key);
             Sleep(700); // let the relay store the replaceable event
             btf::Descriptor d;
-            if (ResolveDescriptor(ws, key.ctx, key.BtfAddress(), d))
+            bool fExchangeOk = false;
+            if (ResolveDescriptor(ws, key.ctx, key.BtfAddress(), d, fExchangeOk))
             {
                 LogPrint("nostr", "Nostr: .btf self-resolve OK via %s (meeting_node=%s, enc=%s...)\n",
                        pszNostrRelays[i], d.meeting_node.c_str(), d.enc.substr(0, 16).c_str());
-                lease.fOk = true;
+                lease.fOk = fExchangeOk;
                 break;
             }
-            lease.fOk = true;
+            // Same rule as BtfResolve: a socket only goes back in the pool if
+            // the relay finished the conversation on it.
+            lease.fOk = fExchangeOk;
         }
         CATCH_PRINT_EXCEPTION("btf self-test")
     }