fix: bind CoinJoin entry admission to session snapshot

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, nMessageID, nullptr)) {
         WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::%s -- ERROR! IsValidInOuts() failed: %s\n", __func__, CoinJoin::GetMessageByID(nMessageID).translated);
         UnlockCoins();
         keyHolderStorage.ReturnAll();
@@ -2004,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)
 {
     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) {
-            LogPrint(BCLog::COINJOIN, "CCoinJoinBaseSession::IsValidInOuts -- ERROR: incompatible denom %d (%s) != nSessionDenom %d (%s)\n",
-                    nDenom, CoinJoin::DenominationToString(nDenom), nSessionDenom, CoinJoin::DenominationToString(nSessionDenom));
+        if (int nDenom = CoinJoin::AmountToDenomination(txout.nValue); nDenom != session_denom) {
+            LogPrint(BCLog::COINJOIN, "CCoinJoinBaseSession::IsValidInOuts -- incompatible denom %d (%s) != %d (%s)\n",
+                     nDenom, CoinJoin::DenominationToString(nDenom), session_denom,
+                     CoinJoin::DenominationToString(session_denom));
             nMessageIDRet = ERR_DENOM;
             if (fConsumeCollateralRet) *fConsumeCollateralRet = true;
             return false;

src/coinjoin/coinjoin.h

@@ -337,9 +337,9 @@ class CCoinJoinBaseSession
 
     virtual void SetNull() EXCLUSIVE_LOCKS_REQUIRED(cs_coinjoin);
 
-    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;
+    static bool IsValidInOuts(Chainstate& active_chainstate, const llmq::CInstantSendManager& isman,
+                              const CTxMemPool& mempool, const std::vector<CTxIn>& vin, const std::vector<CTxOut>& vout,
+                              int session_denom, PoolMessage& nMessageIDRet, bool* fConsumeCollateralRet);
 
 public:
     // Atomic because the message-handling and scheduler threads write it while those threads and

src/coinjoin/server.cpp

@@ -200,7 +200,7 @@ void CCoinJoinServer::ProcessDSQUEUE(NodeId from, CDataStream& vRecv)
 void CCoinJoinServer::ProcessDSVIN(CNode& peer, CDataStream& vRecv)
 {
     //do we have enough users in the current session?
-    if (!IsSessionReady()) {
+    if (!WITH_LOCK(cs_coinjoin, return IsSessionReady())) {
         LogPrint(BCLog::COINJOIN, "DSVIN -- session not complete!\n");
         PushStatus(peer, STATUS_REJECTED, ERR_SESSION);
         return;
@@ -298,7 +298,7 @@ void CCoinJoinServer::CheckPool()
         LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckPool -- entries count %lu\n", entries);
 
     // If we have an entry for each collateral, then create final tx
-    if (nState == POOL_STATE_ACCEPTING_ENTRIES && size_t(GetEntriesCount()) == vecSessionCollaterals.size()) {
+    if (nState == POOL_STATE_ACCEPTING_ENTRIES && static_cast<size_t>(GetEntriesCount()) == vecSessionCollaterals.size()) {
         LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckPool -- FINALIZE TRANSACTIONS\n");
         CreateFinalTransaction();
         return;
@@ -620,10 +620,21 @@ bool CCoinJoinServer::AddEntry(const CCoinJoinEntry& entry, PoolMessage& nMessag
 {
     AssertLockNotHeld(cs_coinjoin);
 
-    if (size_t(GetEntriesCount()) >= vecSessionCollaterals.size()) {
-        LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- ERROR: entries is full!\n", __func__);
-        nMessageIDRet = ERR_ENTRIES_FULL;
-        return false;
+    int session_id;
+    int session_denom;
+    {
+        LOCK(cs_coinjoin);
+        if (nSessionID == 0 || nState != POOL_STATE_ACCEPTING_ENTRIES) {
+            nMessageIDRet = ERR_SESSION;
+            return false;
+        }
+        if (static_cast<size_t>(GetEntriesCountLocked()) >= vecSessionCollaterals.size()) {
+            LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- ERROR: entries is full!\n", __func__);
+            nMessageIDRet = ERR_ENTRIES_FULL;
+            return false;
+        }
+        session_id = nSessionID;
+        session_denom = nSessionDenom;
     }
 
     if (entry.vecTxDSIn.size() > COINJOIN_ENTRY_MAX_SIZE || entry.vecTxOut.size() > COINJOIN_ENTRY_MAX_SIZE) {
@@ -635,11 +646,13 @@ bool CCoinJoinServer::AddEntry(const CCoinJoinEntry& entry, PoolMessage& nMessag
         CTransactionRef txCollateralToConsume;
         {
             LOCK(cs_coinjoin);
-            const auto it = std::ranges::find_if(vecSessionCollaterals, [&entry](const auto& txCollateral) {
-                return *entry.txCollateral == *txCollateral;
-            });
-            if (it != vecSessionCollaterals.end()) {
-                txCollateralToConsume = *it;
+            if (IsCurrentSession(session_id, session_denom, POOL_STATE_ACCEPTING_ENTRIES)) {
+                const auto it = std::ranges::find_if(vecSessionCollaterals, [&entry](const auto& txCollateral) {
+                    return *entry.txCollateral == *txCollateral;
+                });
+                if (it != vecSessionCollaterals.end()) {
+                    txCollateralToConsume = *it;
+                }
             }
         }
         if (txCollateralToConsume) {
@@ -655,34 +668,62 @@ bool CCoinJoinServer::AddEntry(const CCoinJoinEntry& entry, PoolMessage& nMessag
     }
 
     std::vector<CTxIn> vin;
+    vin.reserve(entry.vecTxDSIn.size());
     for (const auto& txin : entry.vecTxDSIn) {
         LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- txin=%s\n", __func__, txin.ToString());
-        LOCK(cs_coinjoin);
-        for (const auto& inner_entry : vecEntries) {
-            if (std::ranges::any_of(inner_entry.vecTxDSIn,
-                                    [&txin](const auto& txdsin) { return txdsin.prevout == txin.prevout; })) {
-                LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- ERROR: already have this txin in entries\n", __func__);
-                nMessageIDRet = ERR_ALREADY_HAVE;
-                // Two peers sent the same input? Can't really say who is the malicious one here,
-                // could be that someone is picking someone else's inputs randomly trying to force
-                // collateral consumption. Do not punish.
-                return false;
-            }
-        }
         vin.emplace_back(txin);
     }
 
     bool fConsumeCollateral{false};
-    if (!IsValidInOuts(m_chainman.ActiveChainstate(), m_isman, mempool, vin, entry.vecTxOut, nMessageIDRet,
-                       &fConsumeCollateral)) {
+    if (!IsValidInOuts(m_chainman.ActiveChainstate(), m_isman, mempool, vin, entry.vecTxOut, session_denom,
+                       nMessageIDRet, &fConsumeCollateral)) {
         LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- ERROR! IsValidInOuts() failed: %s\n", __func__, CoinJoin::GetMessageByID(nMessageIDRet).translated);
         if (fConsumeCollateral) {
-            ConsumeCollateral(entry.txCollateral);
+            CTransactionRef txCollateralToConsume;
+            {
+                LOCK(cs_coinjoin);
+                if (IsCurrentSession(session_id, session_denom, POOL_STATE_ACCEPTING_ENTRIES)) {
+                    const auto it = std::ranges::find_if(vecSessionCollaterals, [&entry](const auto& txCollateral) {
+                        return *entry.txCollateral == *txCollateral;
+                    });
+                    if (it != vecSessionCollaterals.end()) {
+                        txCollateralToConsume = *it;
+                    }
+                }
+            }
+            if (txCollateralToConsume) {
+                ConsumeCollateral(txCollateralToConsume);
+            }
         }
         return false;
     }
 
-    WITH_LOCK(cs_coinjoin, vecEntries.push_back(entry));
+    {
+        LOCK(cs_coinjoin);
+        if (!IsCurrentSession(session_id, session_denom, POOL_STATE_ACCEPTING_ENTRIES)) {
+            nMessageIDRet = ERR_SESSION;
+            return false;
+        }
+        if (static_cast<size_t>(GetEntriesCountLocked()) >= vecSessionCollaterals.size()) {
+            nMessageIDRet = ERR_ENTRIES_FULL;
+            return false;
+        }
+        for (const auto& txin : vin) {
+            for (const auto& inner_entry : vecEntries) {
+                if (std::ranges::any_of(inner_entry.vecTxDSIn,
+                                        [&txin](const auto& txdsin) { return txdsin.prevout == txin.prevout; })) {
+                    LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- ERROR: already have this txin in entries\n",
+                             __func__);
+                    nMessageIDRet = ERR_ALREADY_HAVE;
+                    // Two peers sent the same input? Can't really say who is the malicious one here,
+                    // could be that someone is picking someone else's inputs randomly trying to force
+                    // collateral consumption. Do not punish.
+                    return false;
+                }
+            }
+        }
+        vecEntries.push_back(entry);
+    }
 
     LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- adding entry %d of %d required\n", __func__, GetEntriesCount(), CoinJoin::GetMaxPoolParticipants());
     nMessageIDRet = MSG_ENTRIES_ADDED;
@@ -767,6 +808,12 @@ void CCoinJoinServer::CommitSessionCollateral(const CMutableTransaction& txColla
     }
 }
 
+bool CCoinJoinServer::IsCurrentSession(int session_id, int session_denom, PoolState state) const
+{
+    AssertLockHeld(cs_coinjoin);
+    return nSessionID == session_id && nSessionDenom == session_denom && nState == state;
+}
+
 bool CCoinJoinServer::CreateNewSession(const CCoinJoinAccept& dsa, PoolMessage& nMessageIDRet)
 {
     if (nSessionID != 0) return false;

src/coinjoin/server.h

@@ -72,6 +72,7 @@ class CCoinJoinServer : public CCoinJoinBaseSession, public NetHandler
 
     /// Is this nDenom and txCollateral acceptable?
     bool IsAcceptableDSA(const CCoinJoinAccept& dsa, PoolMessage& nMessageIDRet) const;
+    bool IsCurrentSession(int session_id, int session_denom, PoolState state) const EXCLUSIVE_LOCKS_REQUIRED(cs_coinjoin);
     /// Record an accepted collateral and index its input prevouts
     void CommitSessionCollateral(const CMutableTransaction& txCollateral) EXCLUSIVE_LOCKS_REQUIRED(cs_coinjoin);
     bool CreateNewSession(const CCoinJoinAccept& dsa, PoolMessage& nMessageIDRet) EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin);

src/test/coinjoin_inouts_tests.cpp

@@ -20,6 +20,7 @@
 #include <uint256.h>
 #include <util/check.h>
 #include <util/time.h>
+#include <validation.h>
 
 #include <boost/test/unit_test.hpp>
 
@@ -287,6 +288,42 @@ BOOST_AUTO_TEST_CASE(server_signfinaltx_participant_oversized_count_is_rejected_
     BOOST_CHECK_EQUAL(server.GetEntriesCount(), 1);
 }
 
+//! Re-export the protected static validation helper so it can be called
+//! directly, without standing up a CCoinJoinServer.
+struct InOutsChecker : CCoinJoinBaseSession
+{
+    using CCoinJoinBaseSession::IsValidInOuts;
+};
+
+BOOST_AUTO_TEST_CASE(validation_uses_session_denom_snapshot)
+{
+    Chainstate& chainstate{Assert(m_node.chainman)->ActiveChainstate()};
+    const auto& isman{*Assert(m_node.llmq_ctx->isman)};
+    const auto& mempool{*Assert(m_node.mempool)};
+
+    const int session_denom{CoinJoin::AmountToDenomination(CoinJoin::GetSmallestDenomination())};
+    const std::vector<CTxIn> vin{CTxIn{COutPoint{uint256::ONE, 0}}};
+    const std::vector<CTxOut> vout{CTxOut{CoinJoin::GetSmallestDenomination(), P2PKHScript()}};
+    PoolMessage message{MSG_NOERR};
+    bool consume_collateral{false};
+
+    // Outputs matching the captured denomination pass the denom check and fail
+    // only later on the unknown input.
+    BOOST_CHECK(!InOutsChecker::IsValidInOuts(chainstate, isman, mempool, vin, vout, session_denom, message,
+                                              &consume_collateral));
+    BOOST_CHECK_EQUAL(message, ERR_MISSING_TX);
+    BOOST_CHECK(!consume_collateral);
+
+    // A mismatched captured denomination is rejected up front and flags the
+    // entry's collateral for consumption.
+    const int other_denom{CoinJoin::AmountToDenomination(CoinJoin::GetStandardDenominations().front())};
+    BOOST_REQUIRE(other_denom != session_denom);
+    BOOST_CHECK(!InOutsChecker::IsValidInOuts(chainstate, isman, mempool, vin, vout, other_denom, message,
+                                              &consume_collateral));
+    BOOST_CHECK_EQUAL(message, ERR_DENOM);
+    BOOST_CHECK(consume_collateral);
+}
+
 BOOST_AUTO_TEST_CASE(entry_deserializes_vectors_through_wire_cap)
 {
     const size_t wire_cap{CoinJoin::GetMaxPoolInputOutputCount()};