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..a5e4c3b91e04 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; @@ -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; @@ -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(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 (static_cast(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..0753a9f56d88 100644 --- a/src/test/coinjoin_inouts_tests.cpp +++ b/src/test/coinjoin_inouts_tests.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include @@ -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 vin{CTxIn{COutPoint{uint256::ONE, 0}}}; + const std::vector 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()};