Skip to content

fix: address review feedback for CoinJoin promotion/demotion (PR #7052) #44

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 6 commits into from
Mar 17, 2026
Merged

fix: address review feedback for CoinJoin promotion/demotion (PR #7052) #44

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9c02367
fix: add IsLockedCoin/IsSpent check in SelectFullyMixedForPromotion
thepastaclaw Mar 17, 2026
1ead9a5
fix: add bounds checking for outpoint.n in coinjoin client
thepastaclaw Mar 17, 2026
2578855
fix: acquire cs_main for V24 activation checks in server
thepastaclaw Mar 17, 2026
3af9ac2
fix: enforce standard mixer minimum on full-session finalization
thepastaclaw Mar 17, 2026
d9428e1
refactor: use ValidatePromotionEntry/ValidateDemotionEntry in IsValid…
thepastaclaw Mar 17, 2026
cc50997
refactor: rename m_vecPromotionInputs to m_vecRebalanceInputs
thepastaclaw Mar 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
44 changes: 26 additions & 18 deletions src/coinjoin/client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,10 +296,10 @@ void CCoinJoinClientSession::SetNull()
// Post-V24: Unlock promotion/demotion inputs before clearing state
// These coins were locked in JoinExistingQueue/StartNewQueue but may not
// have been added to vecOutPointLocked yet if the session failed early
if (!m_vecPromotionInputs.empty()) {
if (!m_vecRebalanceInputs.empty()) {
// Add to vecOutPointLocked so UnlockCoins() will handle them properly
// with its retry mechanism if the wallet is locked
for (const auto& outpoint : m_vecPromotionInputs) {
for (const auto& outpoint : m_vecRebalanceInputs) {
// Only add if not already in the list (avoid duplicates)
if (std::find(vecOutPointLocked.begin(), vecOutPointLocked.end(), outpoint) == vecOutPointLocked.end()) {
vecOutPointLocked.push_back(outpoint);
Expand All @@ -308,7 +308,7 @@ void CCoinJoinClientSession::SetNull()
}
m_fPromotion = false;
m_fDemotion = false;
m_vecPromotionInputs.clear();
m_vecRebalanceInputs.clear();

CCoinJoinBaseSession::SetNull();
}
Expand Down Expand Up @@ -1209,6 +1209,10 @@ bool CCoinJoinClientSession::JoinExistingQueue(CAmount nBalanceNeedsAnonymized,
const auto it = m_wallet->mapWallet.find(outpoint.hash);
if (it != m_wallet->mapWallet.end()) {
const wallet::CWalletTx& wtx = it->second;
if (outpoint.n >= wtx.tx->vout.size()) {
WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::JoinExistingQueue -- invalid outpoint index %u for tx %s\n", outpoint.n, outpoint.hash.ToString());
continue;
}
CTxDSIn txdsin(CTxIn(outpoint), wtx.tx->vout[outpoint.n].scriptPubKey,
m_wallet->GetRealOutpointCoinJoinRounds(outpoint));
vecTxDSInTmp.push_back(txdsin);
Expand Down Expand Up @@ -1278,23 +1282,23 @@ bool CCoinJoinClientSession::JoinExistingQueue(CAmount nBalanceNeedsAnonymized,

// Store promotion inputs for use in PreparePromotionEntry
if (fPromotion) {
m_vecPromotionInputs.clear();
m_vecRebalanceInputs.clear();
for (const auto& txdsin : vecTxDSInTmp) {
m_vecPromotionInputs.push_back(txdsin.prevout);
m_vecRebalanceInputs.push_back(txdsin.prevout);
}
WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::JoinExistingQueue -- pending PROMOTION connection, masternode=%s, nSessionDenom=%d (%s), %d inputs\n",
dmn->proTxHash.ToString(), nSessionDenom, CoinJoin::DenominationToString(nSessionDenom),
m_vecPromotionInputs.size());
m_vecRebalanceInputs.size());
} else if (fDemotion) {
// For demotion, store the single input
m_vecPromotionInputs.clear();
m_vecRebalanceInputs.clear();
if (!vecTxDSInTmp.empty()) {
m_vecPromotionInputs.push_back(vecTxDSInTmp[0].prevout);
m_vecRebalanceInputs.push_back(vecTxDSInTmp[0].prevout);
}
WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::JoinExistingQueue -- pending DEMOTION connection, masternode=%s, nSessionDenom=%d (%s)\n",
dmn->proTxHash.ToString(), nSessionDenom, CoinJoin::DenominationToString(nSessionDenom));
} else {
m_vecPromotionInputs.clear();
m_vecRebalanceInputs.clear();
WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::JoinExistingQueue -- pending connection, masternode=%s, nSessionDenom=%d (%s)\n",
dmn->proTxHash.ToString(), nSessionDenom, CoinJoin::DenominationToString(nSessionDenom));
}
Expand Down Expand Up @@ -1411,6 +1415,10 @@ bool CCoinJoinClientSession::StartNewQueue(CAmount nBalanceNeedsAnonymized, CCon
const auto it = m_wallet->mapWallet.find(outpoint.hash);
if (it != m_wallet->mapWallet.end()) {
const wallet::CWalletTx& wtx = it->second;
if (outpoint.n >= wtx.tx->vout.size()) {
WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::StartNewQueue -- invalid outpoint index %u for tx %s\n", outpoint.n, outpoint.hash.ToString());
continue;
}
CTxDSIn txdsin(CTxIn(outpoint), wtx.tx->vout[outpoint.n].scriptPubKey,
m_wallet->GetRealOutpointCoinJoinRounds(outpoint));
vecTxDSInTmp.push_back(txdsin);
Expand Down Expand Up @@ -1500,15 +1508,15 @@ bool CCoinJoinClientSession::StartNewQueue(CAmount nBalanceNeedsAnonymized, CCon
// Store promotion/demotion state and inputs
m_fPromotion = fPromotion;
m_fDemotion = fDemotion;
m_vecPromotionInputs.clear();
m_vecRebalanceInputs.clear();
for (const auto& txdsin : vecTxDSInTmp) {
m_vecPromotionInputs.push_back(txdsin.prevout);
m_vecRebalanceInputs.push_back(txdsin.prevout);
}

WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::StartNewQueue -- pending %s connection, masternode=%s, nSessionDenom=%d (%s), %zu inputs\n",
fPromotion ? "PROMOTION" : "DEMOTION",
dmn->proTxHash.ToString(), nSessionDenom, CoinJoin::DenominationToString(nSessionDenom),
m_vecPromotionInputs.size());
m_vecRebalanceInputs.size());
strAutoDenomResult = _("Trying to connect…");
return true;
}
Expand Down Expand Up @@ -1751,8 +1759,8 @@ bool CCoinJoinClientSession::PreparePromotionEntry(std::string& strErrorRet, std

vecPSInOutPairsRet.clear();

if (m_vecPromotionInputs.size() != static_cast<size_t>(CoinJoin::PROMOTION_RATIO)) {
strErrorRet = strprintf("Invalid promotion input count: %d (expected %d)", m_vecPromotionInputs.size(), CoinJoin::PROMOTION_RATIO);
if (m_vecRebalanceInputs.size() != static_cast<size_t>(CoinJoin::PROMOTION_RATIO)) {
strErrorRet = strprintf("Invalid promotion input count: %d (expected %d)", m_vecRebalanceInputs.size(), CoinJoin::PROMOTION_RATIO);
return false;
}

Expand All @@ -1765,7 +1773,7 @@ bool CCoinJoinClientSession::PreparePromotionEntry(std::string& strErrorRet, std
const CAmount nLargerAmount = CoinJoin::DenominationToAmount(nLargerDenom);

// Create 10 inputs from stored promotion inputs
for (const auto& outpoint : m_vecPromotionInputs) {
for (const auto& outpoint : m_vecRebalanceInputs) {
const auto it = m_wallet->mapWallet.find(outpoint.hash);
if (it == m_wallet->mapWallet.end()) {
strErrorRet = "Promotion input not found in wallet";
Expand Down Expand Up @@ -1822,8 +1830,8 @@ bool CCoinJoinClientSession::PrepareDemotionEntry(std::string& strErrorRet, std:

vecPSInOutPairsRet.clear();

if (m_vecPromotionInputs.size() != 1) {
strErrorRet = strprintf("Invalid demotion input count: %d (expected 1)", m_vecPromotionInputs.size());
if (m_vecRebalanceInputs.size() != 1) {
strErrorRet = strprintf("Invalid demotion input count: %d (expected 1)", m_vecRebalanceInputs.size());
return false;
}

Expand All @@ -1836,7 +1844,7 @@ bool CCoinJoinClientSession::PrepareDemotionEntry(std::string& strErrorRet, std:
}

// Get the single input (larger denom)
const COutPoint& outpoint = m_vecPromotionInputs[0];
const COutPoint& outpoint = m_vecRebalanceInputs[0];
const auto it = m_wallet->mapWallet.find(outpoint.hash);
if (it == m_wallet->mapWallet.end()) {
strErrorRet = "Demotion input not found in wallet";
Expand Down
2 changes: 1 addition & 1 deletion src/coinjoin/client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ class CCoinJoinClientSession : public CCoinJoinBaseSession
// Post-V24: Promotion/demotion session state
bool m_fPromotion{false}; // True if this session is promoting smaller -> larger denom
bool m_fDemotion{false}; // True if this session is demoting larger -> smaller denom
std::vector<COutPoint> m_vecPromotionInputs; // Selected inputs for promotion (10 coins)
std::vector<COutPoint> m_vecRebalanceInputs; // Selected inputs for promotion/demotion rebalancing

/// Create denominations
bool CreateDenominated(CAmount nBalanceToDenominate);
Expand Down
21 changes: 6 additions & 15 deletions src/coinjoin/coinjoin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,32 +256,23 @@ bool CCoinJoinBaseSession::IsValidInOuts(CChainState& active_chainstate, const l
return false;
}

const int nLargerAdjacentDenom = CoinJoin::GetLargerAdjacentDenom(nSessionDenom);

// Determine expected denominations based on entry type
// Validate promotion/demotion entries using dedicated validators
// and determine expected denominations for UTXO input validation
int nExpectedInputDenom = nSessionDenom;
int nExpectedOutputDenom = nSessionDenom;

if (entryType == EntryType::PROMOTION) {
// Promotion: inputs = session denom (smaller), output = larger adjacent
nExpectedInputDenom = nSessionDenom;
nExpectedOutputDenom = nLargerAdjacentDenom;
if (nLargerAdjacentDenom == 0) {
LogPrint(BCLog::COINJOIN, "CCoinJoinBaseSession::%s -- ERROR: no larger adjacent denom for promotion\n", __func__);
nMessageIDRet = ERR_DENOM;
if (!CoinJoin::ValidatePromotionEntry(vin, vout, nSessionDenom, nMessageIDRet)) {
if (fConsumeCollateralRet) *fConsumeCollateralRet = true;
return false;
}
nExpectedOutputDenom = CoinJoin::GetLargerAdjacentDenom(nSessionDenom);
} else if (entryType == EntryType::DEMOTION) {
// Demotion: input = larger adjacent, outputs = session denom (smaller)
nExpectedInputDenom = nLargerAdjacentDenom;
nExpectedOutputDenom = nSessionDenom;
if (nLargerAdjacentDenom == 0) {
LogPrint(BCLog::COINJOIN, "CCoinJoinBaseSession::%s -- ERROR: no larger adjacent denom for demotion\n", __func__);
nMessageIDRet = ERR_DENOM;
if (!CoinJoin::ValidateDemotionEntry(vin, vout, nSessionDenom, nMessageIDRet)) {
if (fConsumeCollateralRet) *fConsumeCollateralRet = true;
return false;
}
nExpectedInputDenom = CoinJoin::GetLargerAdjacentDenom(nSessionDenom);
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Mar 17, 2026

Choose a reason for hiding this comment

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

P2 Badge Reject demotion at the largest session denomination

This path now sets nExpectedInputDenom from GetLargerAdjacentDenom(nSessionDenom) without checking for 0, so a demotion-shaped entry (1 input, 10 outputs) in a session already at the largest denom can pass structure/output validation and then accept a non-denominated input (AmountToDenomination == 0) if values balance. Before this commit, the explicit nLargerAdjacentDenom == 0 guard rejected this case, so this is a regression that allows an invalid demotion mode and weakens the mixing rules for largest-denom sessions.

Useful? React with 👍 / 👎.

}

auto checkTxOut = [&](const CTxOut& txout, int nExpectedDenom) {
Expand Down
25 changes: 18 additions & 7 deletions src/coinjoin/server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,8 +228,12 @@ void CCoinJoinServer::ProcessDSVIN(CNode& peer, CDataStream& vRecv)
// Post-V24: Check if unbalanced entries (promotion/demotion) are allowed
if (entry.vecTxDSIn.size() != entry.vecTxOut.size()) {
// This is a promotion or demotion entry - requires V24 activation
const CBlockIndex* pindex = m_chainman.ActiveChain().Tip();
const bool fV24Active = pindex && DeploymentActiveAt(*pindex, Params().GetConsensus(), Consensus::DEPLOYMENT_V24);
bool fV24Active{false};
{
LOCK(::cs_main);
const CBlockIndex* pindex = m_chainman.ActiveChain().Tip();
fV24Active = pindex && DeploymentActiveAt(*pindex, Params().GetConsensus(), Consensus::DEPLOYMENT_V24);
}
if (!fV24Active) {
LogPrint(BCLog::COINJOIN, "DSVIN -- promotion/demotion entry rejected: V24 not active\n");
PushStatus(peer, STATUS_REJECTED, ERR_MODE);
Expand Down Expand Up @@ -294,9 +298,12 @@ void CCoinJoinServer::CheckPool()

// If we have an entry for each collateral, then create final tx
if (nState == POOL_STATE_ACCEPTING_ENTRIES && size_t(GetEntriesCount()) == vecSessionCollaterals.size()) {
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckPool -- FINALIZE TRANSACTIONS\n");
CreateFinalTransaction();
return;
if (GetStandardEntriesCount() >= CoinJoin::GetMinPoolParticipants()) {
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckPool -- FINALIZE TRANSACTIONS\n");
CreateFinalTransaction();
return;
}
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckPool -- all entries received but insufficient standard mixers (%d), waiting for timeout\n", GetStandardEntriesCount());
}

// Check for Time Out
Expand Down Expand Up @@ -617,8 +624,12 @@ bool CCoinJoinServer::AddEntry(const CCoinJoinEntry& entry, PoolMessage& nMessag

// Post-V24: allow up to PROMOTION_RATIO (10) inputs for promotion entries
// Pre-V24: max COINJOIN_ENTRY_MAX_SIZE (9) inputs
const CBlockIndex* pindex = m_chainman.ActiveChain().Tip();
const bool fV24Active = pindex && DeploymentActiveAt(*pindex, Params().GetConsensus(), Consensus::DEPLOYMENT_V24);
bool fV24Active{false};
{
LOCK(::cs_main);
const CBlockIndex* pindex = m_chainman.ActiveChain().Tip();
fV24Active = pindex && DeploymentActiveAt(*pindex, Params().GetConsensus(), Consensus::DEPLOYMENT_V24);
}
const size_t nMaxEntryInputs = fV24Active ? CoinJoin::PROMOTION_RATIO : COINJOIN_ENTRY_MAX_SIZE;

if (entry.vecTxDSIn.size() > nMaxEntryInputs) {
Expand Down
4 changes: 4 additions & 0 deletions src/wallet/coinjoin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,8 @@ int CWallet::CountCoinsByDenomination(int nDenom, bool fFullyMixedOnly) const
const auto it{mapWallet.find(outpoint.hash)};
if (it == mapWallet.end()) continue;

if (IsSpent(outpoint) || IsLockedCoin(outpoint)) continue;

const CAmount nValue = it->second.tx->vout[outpoint.n].nValue;
if (nValue != nDenomAmount) continue;

Expand Down Expand Up @@ -455,6 +457,8 @@ std::vector<COutPoint> CWallet::SelectFullyMixedForPromotion(int nDenom, int nCo
const auto it{mapWallet.find(outpoint.hash)};
if (it == mapWallet.end()) continue;

if (IsSpent(outpoint) || IsLockedCoin(outpoint)) continue;

const CAmount nValue = it->second.tx->vout[outpoint.n].nValue;
if (nValue != nDenomAmount) continue;

Expand Down
Loading