From eb2e32e14b67d0eaad17cfa7086d8d7ebaf7f00f Mon Sep 17 00:00:00 2001 From: bitflash-maintainer Date: Tue, 11 Aug 2026 21:52:45 +0000 Subject: 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) --- src/nostr.cpp | 56 +++++++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 45 insertions(+), 11 deletions(-) (limited to 'src') 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(); + // 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() != "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(), 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") } -- cgit v1.2.3