Skip to content

fix: bind CoinJoin entry admission to session snapshot #7599

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
PastaPastaPasta merged 3 commits into from
Aug 17, 2026
Merged

fix: bind CoinJoin entry admission to session snapshot #7599

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
892281e
fix(coinjoin): bind entry admission to session snapshot
PastaPastaPasta Aug 13, 2026
ca442a6
test: call IsValidInOuts directly in denom-snapshot regression test
PastaPastaPasta Aug 16, 2026
93c47eb
refactor(coinjoin): use static_cast for entry count checks
PastaPastaPasta Aug 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/coinjoin/client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -2004,4 +2004,3 @@ UniValue CCoinJoinClientManager::getJsonInfo() const
obj.pushKV("sessions", arrSessions);
return obj;
}

11 changes: 6 additions & 5 deletions src/coinjoin/coinjoin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down
6 changes: 3 additions & 3 deletions src/coinjoin/coinjoin.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
99 changes: 73 additions & 26 deletions src/coinjoin/server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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()) {

@knst knst Aug 16, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: it's moved code, but generally avoid using C-style casts

@PastaPastaPasta PastaPastaPasta Aug 16, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ISN'T a C style cast

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) {
Expand All @@ -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) {
Expand All @@ -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 (size_t(GetEntriesCountLocked()) >= vecSessionCollaterals.size()) {

@knst knst Aug 16, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: avoid using C-style casts in C++ code

@PastaPastaPasta PastaPastaPasta Aug 16, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a c style cast

@PastaPastaPasta PastaPastaPasta Aug 16, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for reference;

size_t() / size_t{} are well defined and generally ok; (size_t)() isn't

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;
Expand Down Expand Up @@ -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;
Expand Down
1 change: 1 addition & 0 deletions src/coinjoin/server.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
28 changes: 28 additions & 0 deletions src/test/coinjoin_inouts_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include <uint256.h>
#include <util/check.h>
#include <util/time.h>
#include <validation.h>

#include <boost/test/unit_test.hpp>

Expand Down Expand Up @@ -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,

@knst knst Aug 16, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this helper is quite useless, consider inlining it

@PastaPastaPasta PastaPastaPasta Aug 16, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the wrapper in ca442a6 — the test now calls the static helper directly.

🤖 Posted autonomously by Claude on behalf of pasta.

const CTxMemPool& mempool, const std::vector<CTxIn>& vin, const std::vector<CTxOut>& 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<CNode> MakePeer(NodeId id, uint32_t ipv4)
Expand Down Expand Up @@ -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<CTxIn> vin{CTxIn{COutPoint{uint256::ONE, 0}}};
const std::vector<CTxOut> 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);

@thepastaclaw thepastaclaw Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Exercise the session race rather than only the validation helper

This test calls the static IsValidInOuts() helper with an explicit denomination, so it confirms that the helper no longer reads mutable session state. It never calls AddEntry(), however, and would still pass if AddEntry() stopped capturing the denomination or if the authoritative session, capacity, or input-uniqueness checks at server.cpp:701-725 were removed. Add a deterministic test seam that pauses entry validation after the initial snapshot, resets or advances the session or admits a competing entry, and then verifies that the stale entry is rejected without being appended or charged against the replacement session.

source: ['codex']

@knst knst Aug 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest to refactor this regressions test and cut all harness ; just direct call of IsValidInOuts without CoinjoinServer is completely fine solution assuming this function is static and doesn't require and server's members. Current implementation is over-complex.

@PastaPastaPasta PastaPastaPasta Aug 16, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in ca442a6: dropped the TestableCoinJoinServer/CActiveMasternodeManager harness entirely. Since IsValidInOuts is protected, the test re-exports it via a two-line using struct and calls it directly. Also added a mismatched-denomination case (ERR_DENOM + collateral-consumption flag) so the snapshot parameter itself is exercised.

🤖 Posted autonomously by Claude on behalf of pasta.

}

BOOST_AUTO_TEST_CASE(entry_deserializes_vectors_through_wire_cap)
{
const size_t wire_cap{CoinJoin::GetMaxPoolInputOutputCount()};
Expand Down
Loading