From 892281eb3e925fdae14ddf309b1df9fa1cfeb6fa Mon Sep 17 00:00:00 2001 From: pasta Date: Thu, 13 Aug 2026 00:06:39 -0500 Subject: [PATCH 1/3] fix(coinjoin): bind entry admission to session snapshot --- src/coinjoin/client.cpp | 3 +- src/coinjoin/coinjoin.cpp | 11 ++-- src/coinjoin/coinjoin.h | 6 +- src/coinjoin/server.cpp | 99 ++++++++++++++++++++++-------- src/coinjoin/server.h | 1 + src/test/coinjoin_inouts_tests.cpp | 28 +++++++++ 6 files changed, 112 insertions(+), 36 deletions(-) diff --git a/src/coinjoin/client.cpp b/src/coinjoin/client.cpp index 13be545cd2d8..000a7eec4d28 100644 --- a/src/coinjoin/client.cpp +++ b/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; } - diff --git a/src/coinjoin/coinjoin.cpp b/src/coinjoin/coinjoin.cpp index 3711dbc89f01..fbcd959a4b70 100644 --- a/src/coinjoin/coinjoin.cpp +++ b/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& vin, - const std::vector& vout, PoolMessage& nMessageIDRet, - bool* fConsumeCollateralRet) const + const std::vector& vout, int session_denom, PoolMessage& nMessageIDRet, + bool* fConsumeCollateralRet) { std::set 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; diff --git a/src/coinjoin/coinjoin.h b/src/coinjoin/coinjoin.h index 863774b84dfb..6993820ba210 100644 --- a/src/coinjoin/coinjoin.h +++ b/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& vin, const std::vector& vout, - PoolMessage& nMessageIDRet, bool* fConsumeCollateralRet) const; + static bool IsValidInOuts(Chainstate& active_chainstate, const llmq::CInstantSendManager& isman, + const CTxMemPool& mempool, const std::vector& vin, const std::vector& vout, + int session_denom, PoolMessage& nMessageIDRet, bool* fConsumeCollateralRet); public: // Atomic because the message-handling and scheduler threads write it while those threads and diff --git a/src/coinjoin/server.cpp b/src/coinjoin/server.cpp index e380df099c4d..d2cca4b9033c 100644 --- a/src/coinjoin/server.cpp +++ b/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; @@ -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 (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 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 (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; diff --git a/src/coinjoin/server.h b/src/coinjoin/server.h index 49cb7a3002d7..289e6ca876be 100644 --- a/src/coinjoin/server.h +++ b/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); diff --git a/src/test/coinjoin_inouts_tests.cpp b/src/test/coinjoin_inouts_tests.cpp index f0d49250f53d..03592cd855c7 100644 --- a/src/test/coinjoin_inouts_tests.cpp +++ b/src/test/coinjoin_inouts_tests.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include @@ -188,6 +189,13 @@ class TestableCoinJoinServer : public CCoinJoinServer LOCK(cs_coinjoin); vecEntries.push_back(std::move(entry)); } + + bool ValidateInOuts(Chainstate& active_chainstate, const llmq::CInstantSendManager& isman, + const CTxMemPool& mempool, const std::vector& vin, const std::vector& vout, + int session_denom, PoolMessage& message, bool& consume_collateral) + { + return IsValidInOuts(active_chainstate, isman, mempool, vin, vout, session_denom, message, &consume_collateral); + } }; static std::unique_ptr MakePeer(NodeId id, uint32_t ipv4) @@ -287,6 +295,26 @@ BOOST_AUTO_TEST_CASE(server_signfinaltx_participant_oversized_count_is_rejected_ BOOST_CHECK_EQUAL(server.GetEntriesCount(), 1); } +BOOST_AUTO_TEST_CASE(server_validation_uses_session_denom_snapshot) +{ + CActiveMasternodeManager mn_activeman(*Assert(m_node.connman), *Assert(m_node.dmnman), MakeSecretKey()); + TestableCoinJoinServer server(m_node.peerman.get(), *Assert(m_node.chainman), *Assert(m_node.connman), + *Assert(m_node.dmnman), *Assert(m_node.dstxman), *Assert(m_node.mn_metaman), + *Assert(m_node.mempool), mn_activeman, *Assert(m_node.mn_sync), + *Assert(m_node.llmq_ctx->isman)); + + const int session_denom{CoinJoin::AmountToDenomination(CoinJoin::GetSmallestDenomination())}; + const std::vector vin{CTxIn{COutPoint{uint256::ONE, 0}}}; + const std::vector vout{CTxOut{CoinJoin::GetSmallestDenomination(), P2PKHScript()}}; + PoolMessage message{MSG_NOERR}; + bool consume_collateral{false}; + + BOOST_CHECK(!server.ValidateInOuts(Assert(m_node.chainman)->ActiveChainstate(), *Assert(m_node.llmq_ctx->isman), + *Assert(m_node.mempool), vin, vout, session_denom, message, consume_collateral)); + BOOST_CHECK_EQUAL(message, ERR_MISSING_TX); + BOOST_CHECK(!consume_collateral); +} + BOOST_AUTO_TEST_CASE(entry_deserializes_vectors_through_wire_cap) { const size_t wire_cap{CoinJoin::GetMaxPoolInputOutputCount()}; From ca442a60d089fe7b5f2ffe338688bb06d00c1ba7 Mon Sep 17 00:00:00 2001 From: pasta Date: Sun, 16 Aug 2026 09:56:55 -0500 Subject: [PATCH 2/3] test: call IsValidInOuts directly in denom-snapshot regression test Drop the TestableCoinJoinServer harness and ValidateInOuts wrapper: the helper is static, so the test re-exports it and calls it directly. Also cover the mismatched-denomination path (ERR_DENOM + collateral consumption flag), which exercises the snapshot parameter itself. Co-Authored-By: Claude Fable 5 --- src/test/coinjoin_inouts_tests.cpp | 39 ++++++++++++++++++------------ 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/src/test/coinjoin_inouts_tests.cpp b/src/test/coinjoin_inouts_tests.cpp index 03592cd855c7..0753a9f56d88 100644 --- a/src/test/coinjoin_inouts_tests.cpp +++ b/src/test/coinjoin_inouts_tests.cpp @@ -189,13 +189,6 @@ class TestableCoinJoinServer : public CCoinJoinServer LOCK(cs_coinjoin); vecEntries.push_back(std::move(entry)); } - - bool ValidateInOuts(Chainstate& active_chainstate, const llmq::CInstantSendManager& isman, - const CTxMemPool& mempool, const std::vector& vin, const std::vector& vout, - int session_denom, PoolMessage& message, bool& consume_collateral) - { - return IsValidInOuts(active_chainstate, isman, mempool, vin, vout, session_denom, message, &consume_collateral); - } }; static std::unique_ptr MakePeer(NodeId id, uint32_t ipv4) @@ -295,13 +288,18 @@ BOOST_AUTO_TEST_CASE(server_signfinaltx_participant_oversized_count_is_rejected_ BOOST_CHECK_EQUAL(server.GetEntriesCount(), 1); } -BOOST_AUTO_TEST_CASE(server_validation_uses_session_denom_snapshot) +//! Re-export the protected static validation helper so it can be called +//! directly, without standing up a CCoinJoinServer. +struct InOutsChecker : CCoinJoinBaseSession { - CActiveMasternodeManager mn_activeman(*Assert(m_node.connman), *Assert(m_node.dmnman), MakeSecretKey()); - TestableCoinJoinServer server(m_node.peerman.get(), *Assert(m_node.chainman), *Assert(m_node.connman), - *Assert(m_node.dmnman), *Assert(m_node.dstxman), *Assert(m_node.mn_metaman), - *Assert(m_node.mempool), mn_activeman, *Assert(m_node.mn_sync), - *Assert(m_node.llmq_ctx->isman)); + 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 vin{CTxIn{COutPoint{uint256::ONE, 0}}}; @@ -309,10 +307,21 @@ BOOST_AUTO_TEST_CASE(server_validation_uses_session_denom_snapshot) PoolMessage message{MSG_NOERR}; bool consume_collateral{false}; - BOOST_CHECK(!server.ValidateInOuts(Assert(m_node.chainman)->ActiveChainstate(), *Assert(m_node.llmq_ctx->isman), - *Assert(m_node.mempool), vin, vout, session_denom, message, consume_collateral)); + // 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) From 93c47eb10fcbd8b904bcaa080113d73e172fe6f9 Mon Sep 17 00:00:00 2001 From: pasta Date: Mon, 17 Aug 2026 12:22:22 -0500 Subject: [PATCH 3/3] refactor(coinjoin): use static_cast for entry count checks --- src/coinjoin/server.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coinjoin/server.cpp b/src/coinjoin/server.cpp index d2cca4b9033c..a5e4c3b91e04 100644 --- a/src/coinjoin/server.cpp +++ b/src/coinjoin/server.cpp @@ -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(GetEntriesCount()) == vecSessionCollaterals.size()) { LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckPool -- FINALIZE TRANSACTIONS\n"); CreateFinalTransaction(); return; @@ -628,7 +628,7 @@ bool CCoinJoinServer::AddEntry(const CCoinJoinEntry& entry, PoolMessage& nMessag nMessageIDRet = ERR_SESSION; return false; } - if (size_t(GetEntriesCountLocked()) >= vecSessionCollaterals.size()) { + if (static_cast(GetEntriesCountLocked()) >= vecSessionCollaterals.size()) { LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- ERROR: entries is full!\n", __func__); nMessageIDRet = ERR_ENTRIES_FULL; return false; @@ -704,7 +704,7 @@ bool CCoinJoinServer::AddEntry(const CCoinJoinEntry& entry, PoolMessage& nMessag nMessageIDRet = ERR_SESSION; return false; } - if (size_t(GetEntriesCountLocked()) >= vecSessionCollaterals.size()) { + if (static_cast(GetEntriesCountLocked()) >= vecSessionCollaterals.size()) { nMessageIDRet = ERR_ENTRIES_FULL; return false; }