fix(coinjoin): apply a DSSIGNFINALTX message atomically

A DSSIGNFINALTX message can carry several signed inputs, but each was applied through a separate AddScriptSig() call that took and released cs_coinjoin on its own, and the signing-state and participant checks were released before the body was even decoded. At the signing-timeout boundary the scheduler's CheckTimeout() could take the lock between two inputs of the same already-received message, still find the sender's later inputs unsigned, select the sender as a non-signer to charge, and reset the session; the remaining inputs then failed against the cleared pool.

Capture the signing session the sender was checked against, then revalidate it after decoding and apply the whole batch under one cs_coinjoin critical section, relaying a rejection from inside the same section if an input fails. AddScriptSig() now requires cs_coinjoin. The lock is released before CheckPool(), which then commits the fully signed session; CheckTimeout() sees IsSignaturesComplete() and defers to it. No cs_main is taken in this section: signature verification already ran under cs_coinjoin inside AddScriptSig().

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Author: PastaPastaPasta

src/coinjoin/server.cpp

@@ -232,14 +232,14 @@ void CCoinJoinServer::ProcessDSSIGNFINALTX(CNode& peer, CDataStream& vRecv)
     // Only accept signatures while we are actually collecting them, and only
     // from peers that are active participants in this session. Otherwise a
     // stray or unauthenticated peer could abort the session for everyone.
-    if (nState != POOL_STATE_SIGNING) {
-        LogPrint(BCLog::COINJOIN, "DSSIGNFINALTX -- wrong state, nState=%d, peer=%d\n",
-                 nState.load(), peer.GetId());
-        PushStatus(peer, STATUS_REJECTED, ERR_SESSION);
-        return;
-    }
+    int session_id{0};
     {
         LOCK(cs_coinjoin);
+        if (nState != POOL_STATE_SIGNING) {
+            LogPrint(BCLog::COINJOIN, "DSSIGNFINALTX -- wrong state, nState=%d, peer=%d\n", nState.load(), peer.GetId());
+            PushStatus(peer, STATUS_REJECTED, ERR_SESSION);
+            return;
+        }
         const bool is_participant = std::ranges::any_of(
             vecEntries, [&peer](const auto& entry) { return entry.addr == peer.addr; });
         if (!is_participant) {
@@ -248,6 +248,7 @@ void CCoinJoinServer::ProcessDSSIGNFINALTX(CNode& peer, CDataStream& vRecv)
             PushStatus(peer, STATUS_REJECTED, ERR_INVALID_INPUT);
             return;
         }
+        session_id = nSessionID;
     }
 
     const size_t max_txins{CoinJoin::GetMaxPoolInputOutputCount()};
@@ -264,18 +265,30 @@ void CCoinJoinServer::ProcessDSSIGNFINALTX(CNode& peer, CDataStream& vRecv)
 
     LogPrint(BCLog::COINJOIN, "DSSIGNFINALTX -- vecTxIn.size() %s\n", vecTxIn.size());
 
-    int nTxInIndex = 0;
-    int nTxInsCount = static_cast<int>(vecTxIn.size());
-
-    for (const auto& txin : vecTxIn) {
-        nTxInIndex++;
-        if (!AddScriptSig(txin)) {
-            LogPrint(BCLog::COINJOIN, "DSSIGNFINALTX -- AddScriptSig() failed at %d/%d, session: %d\n", nTxInIndex, nTxInsCount, nSessionID);
-            LOCK(cs_coinjoin);
-            RelayStatus(STATUS_REJECTED);
+    {
+        LOCK(cs_coinjoin);
+        // Apply the whole message in one critical section, against the signing session it was
+        // checked for above. The lock was released while the body was decoded, and releasing it
+        // between inputs would let a timeout reset the session halfway through one sender's
+        // already-received signatures, after charging that sender as a non-signer.
+        if (nSessionID != session_id || nState != POOL_STATE_SIGNING) {
+            LogPrint(BCLog::COINJOIN, "DSSIGNFINALTX -- session %d is no longer signing, peer=%d\n", session_id,
+                     peer.GetId());
+            PushStatus(peer, STATUS_REJECTED, ERR_SESSION);
             return;
         }
-        LogPrint(BCLog::COINJOIN, "DSSIGNFINALTX -- AddScriptSig() %d/%d success\n", nTxInIndex, nTxInsCount);
+        int nTxInIndex = 0;
+        int nTxInsCount = static_cast<int>(vecTxIn.size());
+        for (const auto& txin : vecTxIn) {
+            nTxInIndex++;
+            if (!AddScriptSig(txin)) {
+                LogPrint(BCLog::COINJOIN, "DSSIGNFINALTX -- AddScriptSig() failed at %d/%d, session: %d\n", nTxInIndex,
+                         nTxInsCount, session_id);
+                RelayStatus(STATUS_REJECTED);
+                return;
+            }
+            LogPrint(BCLog::COINJOIN, "DSSIGNFINALTX -- AddScriptSig() %d/%d success\n", nTxInIndex, nTxInsCount);
+        }
     }
     // all is good
     CheckPool();
@@ -940,10 +953,9 @@ bool CCoinJoinServer::AddEntry(const CCoinJoinEntry& entry, PoolMessage& nMessag
 
 bool CCoinJoinServer::AddScriptSig(const CTxIn& txinNew)
 {
-    AssertLockNotHeld(cs_coinjoin);
+    AssertLockHeld(cs_coinjoin);
     LogPrint(BCLog::COINJOIN, "CCoinJoinServer::AddScriptSig -- scriptSig=%s\n", ScriptToAsmStr(txinNew.scriptSig).substr(0, 24));
 
-    LOCK(cs_coinjoin);
     for (const auto& entry : vecEntries) {
         if (std::ranges::any_of(entry.vecTxDSIn,
                                 [&txinNew](const auto& txdsin) { return txdsin.scriptSig == txinNew.scriptSig; })) {

src/coinjoin/server.h

@@ -128,7 +128,7 @@ class CCoinJoinServer : public CCoinJoinBaseSession, public NetHandler
     Mutex cs_check_pool;
 
     /// Add signature to a txin
-    bool AddScriptSig(const CTxIn& txin) EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin);
+    bool AddScriptSig(const CTxIn& txin) EXCLUSIVE_LOCKS_REQUIRED(cs_coinjoin);
 
     /// Choose one bad actor whose collateral should be consumed, if any.
     CTransactionRef SelectCollateralToCharge() const EXCLUSIVE_LOCKS_REQUIRED(cs_coinjoin);