fix: data races and check-then-act races in the CoinJoin server

src/coinjoin/client.cpp

@@ -475,7 +475,7 @@ bool CCoinJoinClientSession::SignFinalTransaction(CNode& peer, Chainstate& activ
     // Make sure all inputs/outputs are valid
     PoolMessage nMessageID{MSG_NOERR};
     if (!IsValidInOuts(active_chainstate, m_isman, mempool, finalMutableTransaction.vin, finalMutableTransaction.vout,
-                       nMessageID, nullptr)) {
+                       nSessionDenom.load(), nMessageID, nullptr)) {
         WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::%s -- ERROR! IsValidInOuts() failed: %s\n", __func__, CoinJoin::GetMessageByID(nMessageID).translated);
         UnlockCoins();
         keyHolderStorage.ReturnAll();
@@ -1229,8 +1229,9 @@ bool CCoinJoinClientSession::JoinExistingQueue(CAmount nBalanceNeedsAnonymized,
         SetState(POOL_STATE_QUEUE);
         nTimeLastSuccessfulStep = GetTime();
         WalletCJLogPrint(m_wallet, /* Continued */
-                         "CCoinJoinClientSession::JoinExistingQueue -- pending connection, masternode=%s, nSessionDenom=%d (%s)\n",
-                         dmn->proTxHash.ToString(), nSessionDenom, CoinJoin::DenominationToString(nSessionDenom));
+                         "CCoinJoinClientSession::JoinExistingQueue -- pending connection, masternode=%s, "
+                         "nSessionDenom=%d (%s)\n",
+                         dmn->proTxHash.ToString(), nSessionDenom.load(), CoinJoin::DenominationToString(nSessionDenom));
         strAutoDenomResult = _("Trying to connect…");
         return true;
     }
@@ -1310,9 +1311,11 @@ bool CCoinJoinClientSession::StartNewQueue(CAmount nBalanceNeedsAnonymized, CCon
         pendingDsaRequest = CPendingDsaRequest(dmn->proTxHash, CCoinJoinAccept(nSessionDenom, txMyCollateral));
         SetState(POOL_STATE_QUEUE);
         nTimeLastSuccessfulStep = GetTime();
-        WalletCJLogPrint( /* Continued */
-            m_wallet, "CCoinJoinClientSession::StartNewQueue -- pending connection, masternode=%s, nSessionDenom=%d (%s)\n",
-            dmn->proTxHash.ToString(), nSessionDenom, CoinJoin::DenominationToString(nSessionDenom));
+        WalletCJLogPrint(/* Continued */
+                         m_wallet,
+                         "CCoinJoinClientSession::StartNewQueue -- pending connection, masternode=%s, nSessionDenom=%d "
+                         "(%s)\n",
+                         dmn->proTxHash.ToString(), nSessionDenom.load(), CoinJoin::DenominationToString(nSessionDenom));
         strAutoDenomResult = _("Trying to connect…");
         return true;
     }
@@ -1424,7 +1427,7 @@ bool CCoinJoinClientSession::SubmitDenominate(CConnman& connman)
         return a.second > b.second || (a.second == b.second && a.first < b.first);
     });
 
-    WalletCJLogPrint(m_wallet, "vecInputsByRounds for denom %d\n", nSessionDenom);
+    WalletCJLogPrint(m_wallet, "vecInputsByRounds for denom %d\n", nSessionDenom.load());
     for (const auto& pair : vecInputsByRounds) {
         WalletCJLogPrint(m_wallet, "vecInputsByRounds: rounds: %d, inputs: %d\n", pair.first, pair.second);
     }
@@ -2001,4 +2004,3 @@ UniValue CCoinJoinClientManager::getJsonInfo() const
     obj.pushKV("sessions", arrSessions);
     return obj;
 }
-

src/coinjoin/coinjoin.cpp

@@ -206,8 +206,8 @@ std::string CCoinJoinBaseSession::GetStateString() const
 
 bool CCoinJoinBaseSession::IsValidInOuts(Chainstate& active_chainstate, const llmq::CInstantSendManager& isman,
                                          const CTxMemPool& mempool, const std::vector<CTxIn>& vin,
-                                         const std::vector<CTxOut>& vout, PoolMessage& nMessageIDRet,
-                                         bool* fConsumeCollateralRet) const
+                                         const std::vector<CTxOut>& vout, int session_denom,
+                                         PoolMessage& nMessageIDRet, bool* fConsumeCollateralRet) const
 {
     std::set<CScript> setScripPubKeys;
     nMessageIDRet = MSG_NOERR;
@@ -221,9 +221,10 @@ bool CCoinJoinBaseSession::IsValidInOuts(Chainstate& active_chainstate, const ll
     }
 
     auto checkTxOut = [&](const CTxOut& txout) {
-        if (int nDenom = CoinJoin::AmountToDenomination(txout.nValue); nDenom != nSessionDenom) {
+        if (int nDenom = CoinJoin::AmountToDenomination(txout.nValue); nDenom != session_denom) {
             LogPrint(BCLog::COINJOIN, "CCoinJoinBaseSession::IsValidInOuts -- ERROR: incompatible denom %d (%s) != nSessionDenom %d (%s)\n",
-                    nDenom, CoinJoin::DenominationToString(nDenom), nSessionDenom, CoinJoin::DenominationToString(nSessionDenom));
+                    nDenom, CoinJoin::DenominationToString(nDenom), session_denom,
+                    CoinJoin::DenominationToString(session_denom));
             nMessageIDRet = ERR_DENOM;
             if (fConsumeCollateralRet) *fConsumeCollateralRet = true;
             return false;

src/coinjoin/coinjoin.h

@@ -339,10 +339,15 @@ class CCoinJoinBaseSession
 
     bool IsValidInOuts(Chainstate& active_chainstate, const llmq::CInstantSendManager& isman,
                        const CTxMemPool& mempool, const std::vector<CTxIn>& vin, const std::vector<CTxOut>& vout,
-                       PoolMessage& nMessageIDRet, bool* fConsumeCollateralRet) const;
+                       int session_denom, PoolMessage& nMessageIDRet, bool* fConsumeCollateralRet) const;
 
 public:
-    int nSessionDenom{0}; // Users must submit a denom matching this
+    // Users must submit a denom matching this. Atomic like its sibling session fields: it is
+    // written by the message-handling thread when a session opens and by either thread in
+    // SetNull(), and read unlocked by both plus RPC threads via GetJsonInfo().
+    // Note when logging: LogPrint() takes its arguments by const reference so a bare read works,
+    // but WalletCJLogPrint() takes them by value and needs an explicit .load().
+    std::atomic<int> nSessionDenom{0};
 
     CCoinJoinBaseSession() = default;
     virtual ~CCoinJoinBaseSession() = default;