Skip to content

fix: bind CoinJoin signatures to the approved transaction #7815

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

Open
PastaPastaPasta wants to merge 2 commits into
base: develop
Choose a base branch
from
+199 −78
Open

fix: bind CoinJoin signatures to the approved transaction #7815

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d55d0e6
fix(coinjoin): bind signatures to the approved transaction
PastaPastaPasta Oct 7, 2026
45f92e5
test: cover CoinJoin signature admission rejection
PastaPastaPasta Oct 7, 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
123 changes: 49 additions & 74 deletions src/coinjoin/server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@
#include <masternode/meta.h>
#include <masternode/sync.h>

#include <coins.h>
#include <core_io.h>
#include <net.h>
#include <net_processing.h>
#include <netmessagemaker.h>
#include <policy/policy.h>
#include <scheduler.h>
#include <script/interpreter.h>
#include <serialize.h>
Expand All @@ -26,6 +28,8 @@

#include <univalue.h>

#include <algorithm>
#include <iterator>
#include <ranges>

CCoinJoinServer::CCoinJoinServer(PeerManagerInternal* peer_manager, ChainstateManager& chainman, CConnman& _connman,
Expand Down Expand Up @@ -270,7 +274,7 @@ void CCoinJoinServer::ProcessDSSIGNFINALTX(CNode& peer, CDataStream& vRecv)

for (const auto& txin : vecTxIn) {
nTxInIndex++;
if (!AddScriptSig(txin)) {
if (!AddScriptSig(txin, peer.addr)) {
LogPrint(BCLog::COINJOIN, "DSSIGNFINALTX -- AddScriptSig() failed at %d/%d, session: %d\n", nTxInIndex, nTxInsCount, nSessionID);
LOCK(cs_coinjoin);
RelayStatus(STATUS_REJECTED);
Expand Down Expand Up @@ -660,51 +664,6 @@ void CCoinJoinServer::CheckForCompleteQueue()
m_queueman.AddQueue(std::move(dsq));
}

// Check to make sure a given input matches an input in the pool and its scriptSig is valid
bool CCoinJoinServer::IsInputScriptSigValid(const CTxIn& txin) const
{
AssertLockHeld(cs_coinjoin);
CMutableTransaction txNew;
txNew.vin.clear();
txNew.vout.clear();

int nTxInIndex = -1;
CScript sigPubKey = CScript();

{
int i = 0;
for (const auto &entry: vecEntries) {
for (const auto &txout: entry.vecTxOut) {
txNew.vout.push_back(txout);
}
for (const auto &txdsin: entry.vecTxDSIn) {
txNew.vin.push_back(txdsin);

if (txdsin.prevout == txin.prevout) {
nTxInIndex = i;
sigPubKey = txdsin.prevPubKey;
}
i++;
}
}
}
if (nTxInIndex >= 0) { //might have to do this one input at a time?
txNew.vin[nTxInIndex].scriptSig = txin.scriptSig;
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::IsInputScriptSigValid -- verifying scriptSig %s\n", ScriptToAsmStr(txin.scriptSig).substr(0, 24));
// TODO we're using amount=0 here but we should use the correct amount. This works because Dash ignores the amount while signing/verifying (only used in Bitcoin/Segwit)
if (!VerifyScript(txNew.vin[nTxInIndex].scriptSig, sigPubKey, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_STRICTENC, MutableTransactionSignatureChecker(&txNew, nTxInIndex, 0, MissingDataBehavior::ASSERT_FAIL))) {
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::IsInputScriptSigValid -- VerifyScript() failed on input %d\n", nTxInIndex);
return false;
}
} else {
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::IsInputScriptSigValid -- Failed to find matching input in pool, %s\n", txin.ToString());
return false;
}

LogPrint(BCLog::COINJOIN, "CCoinJoinServer::IsInputScriptSigValid -- Successfully validated input and scriptSig\n");
return true;
}

//
// Add a client's transaction inputs/outputs to the pool
//
Expand Down Expand Up @@ -848,6 +807,21 @@ bool CCoinJoinServer::AddEntry(const CCoinJoinEntry& entry, PoolMessage& nMessag
return false;
}

CCoinJoinEntry admitted_entry{entry};
{
LOCK(cs_main);
CCoinsViewMemPool view{&m_chainman.ActiveChainstate().CoinsTip(), mempool};
for (auto& input : admitted_entry.vecTxDSIn) {
Coin coin;
if (!view.GetCoin(input.prevout, coin) || coin.IsSpent()) {
nMessageIDRet = ERR_MISSING_TX;
return false;
}
// prevPubKey is memory-only; keep the admitted script even if chainstate changes before signing.
input.prevPubKey = coin.out.scriptPubKey;
}
}

{
LOCK(cs_coinjoin);
// cs_coinjoin was released around the UTXO checks above, so the scheduler thread may
Expand All @@ -861,7 +835,7 @@ bool CCoinJoinServer::AddEntry(const CCoinJoinEntry& entry, PoolMessage& nMessag
nMessageIDRet = ERR_SESSION;
return false;
}
vecEntries.push_back(entry);
vecEntries.push_back(std::move(admitted_entry));
}

LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- adding entry %d of %d required\n", __func__, GetEntriesCount(), CoinJoin::GetMaxPoolParticipants());
Expand All @@ -870,42 +844,43 @@ bool CCoinJoinServer::AddEntry(const CCoinJoinEntry& entry, PoolMessage& nMessag
return true;
}

bool CCoinJoinServer::AddScriptSig(const CTxIn& txinNew)
bool CCoinJoinServer::AddScriptSig(const CTxIn& txinNew, const CService& sender)
{
AssertLockNotHeld(cs_coinjoin);
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::AddScriptSig -- scriptSig=%s\n", ScriptToAsmStr(txinNew.scriptSig).substr(0, 24));

LOCK(cs_coinjoin);
for (const auto& entry : vecEntries) {
if (std::ranges::any_of(entry.vecTxDSIn,
[&txinNew](const auto& txdsin) { return txdsin.scriptSig == txinNew.scriptSig; })) {
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::AddScriptSig -- already exists\n");
return false;
}
}
if (nState != POOL_STATE_SIGNING) return false;

if (!IsInputScriptSigValid(txinNew)) {
const auto matches_input = [&txinNew](const CTxIn& input) {
return input.prevout == txinNew.prevout && input.nSequence == txinNew.nSequence;
};
const auto entry = std::ranges::find_if(vecEntries, [&](const auto& candidate) {
return candidate.addr == sender && std::ranges::any_of(candidate.vecTxDSIn, matches_input);
});
if (entry == vecEntries.end()) return false;

const auto admitted_input = std::ranges::find_if(entry->vecTxDSIn, matches_input);
const auto final_input = std::ranges::find_if(finalMutableTransaction.vin, matches_input);
if (final_input == finalMutableTransaction.vin.end() || admitted_input->prevPubKey.empty()) return false;
if (admitted_input->fHasSig && admitted_input->scriptSig == txinNew.scriptSig) return true;

const auto index = static_cast<unsigned int>(std::distance(finalMutableTransaction.vin.begin(), final_input));
// Dash's legacy signature hash does not use the input amount.
if (!VerifyScript(txinNew.scriptSig, admitted_input->prevPubKey,
STANDARD_SCRIPT_VERIFY_FLAGS | SCRIPT_VERIFY_SIGPUSHONLY,
MutableTransactionSignatureChecker(&finalMutableTransaction, index, 0, MissingDataBehavior::ASSERT_FAIL))) {
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::AddScriptSig -- Invalid scriptSig\n");
return false;
}

LogPrint(BCLog::COINJOIN, "CCoinJoinServer::AddScriptSig -- scriptSig=%s new\n", ScriptToAsmStr(txinNew.scriptSig).substr(0, 24));

for (auto& txin : finalMutableTransaction.vin) {
if (txin.prevout == txinNew.prevout && txin.nSequence == txinNew.nSequence) {
txin.scriptSig = txinNew.scriptSig;
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::AddScriptSig -- adding to finalMutableTransaction, scriptSig=%s\n", ScriptToAsmStr(txinNew.scriptSig).substr(0, 24));
}
}
for (auto& entry : vecEntries) {
if (entry.AddScriptSig(txinNew)) {
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::AddScriptSig -- adding to entries, scriptSig=%s\n", ScriptToAsmStr(txinNew.scriptSig).substr(0, 24));
return true;
if (admitted_input->fHasSig) return true;
for (const auto& other_entry : vecEntries) {
if (std::ranges::any_of(other_entry.vecTxDSIn,
[&txinNew](const auto& input) { return input.scriptSig == txinNew.scriptSig; })) {
return false;
}
}

LogPrint(BCLog::COINJOIN, "CCoinJoinServer::AddScriptSig -- Couldn't set sig!\n");
return false;
if (!entry->AddScriptSig(txinNew)) return false;
final_input->scriptSig = txinNew.scriptSig;
return true;
}

// Check to make sure everything is signed
Expand Down
4 changes: 1 addition & 3 deletions src/coinjoin/server.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ class CCoinJoinServer : public CCoinJoinBaseSession, public NetHandler
bool fUnitTest;

/// Add signature to a txin
bool AddScriptSig(const CTxIn& txin) EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin);
bool AddScriptSig(const CTxIn& txin, const CService& sender) EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin);

/// Charge fees to bad actors (Charge clients a fee if they're abusive)
void ChargeFees() const EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin);
Expand Down Expand Up @@ -127,8 +127,6 @@ class CCoinJoinServer : public CCoinJoinBaseSession, public NetHandler

/// Check that all inputs are signed. (Are all inputs signed?)
bool IsSignaturesComplete() const EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin);
/// Check to make sure a given input matches an input in the pool and its scriptSig is valid
bool IsInputScriptSigValid(const CTxIn& txin) const EXCLUSIVE_LOCKS_REQUIRED(cs_coinjoin);

// Set the 'state' value, with some logging and capturing when the state changed
void SetState(PoolState nStateNew);
Expand Down
150 changes: 149 additions & 1 deletion src/test/coinjoin_inouts_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,24 @@
#include <coinjoin/common.h>
#include <coinjoin/options.h>
#include <coinjoin/server.h>
#include <coins.h>
#include <key.h>
#include <llmq/context.h>
#include <masternode/sync.h>
#include <net.h>
#include <node/connection_types.h>
#include <policy/policy.h>
#include <protocol.h>
#include <script/interpreter.h>
#include <script/script.h>
#include <script/sign.h>
#include <script/signingprovider.h>
#include <script/standard.h>
#include <streams.h>
#include <test/util/net.h>
#include <test/util/setup_common.h>
#include <tinyformat.h>
#include <txmempool.h>
#include <uint256.h>
#include <util/check.h>
#include <util/time.h>
Expand All @@ -29,7 +38,9 @@
#include <algorithm>
#include <array>
#include <cstdint>
#include <iterator>
#include <memory>
#include <ranges>
#include <vector>

BOOST_FIXTURE_TEST_SUITE(coinjoin_inouts_tests, TestingSetup)
Expand Down Expand Up @@ -188,7 +199,13 @@ class TestableCoinJoinServer : public CCoinJoinServer
// A live session always carries a non-zero id, and AddEntry rejects entries that don't
// belong to one, so seed an id along with the state.
void EnterSigningState() { nSessionID = 1; nState = POOL_STATE_SIGNING; }
void EnterAcceptingEntriesState() { nSessionID = 1; nState = POOL_STATE_ACCEPTING_ENTRIES; }
void EnterAcceptingEntriesState(int denom = 0) { nSessionID = 1; nSessionDenom = denom; nState = POOL_STATE_ACCEPTING_ENTRIES; }

CMutableTransaction FinalTransaction() const EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin)
{
LOCK(cs_coinjoin);
return finalMutableTransaction;
}

void SeedParticipant(const CService& addr) EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin)
{
Expand Down Expand Up @@ -231,6 +248,137 @@ static std::unique_ptr<CNode> MakePeer(NodeId id, uint32_t ipv4)
return peer;
}

BOOST_AUTO_TEST_CASE(server_signfinaltx_completes_with_legacy_and_full_signatures)
{
m_node.mn_sync->SwitchToNextAsset();
BOOST_REQUIRE(m_node.mn_sync->IsBlockchainSynced());
auto& connman{static_cast<ConnmanTestMsg&>(*Assert(m_node.connman))};
CActiveMasternodeManager mn_activeman(connman, *Assert(m_node.dmnman), MakeSecretKey());
TestableCoinJoinServer server(m_node.peerman.get(), *Assert(m_node.chainman), 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.isman));
struct PeerCleanup {
ConnmanTestMsg& connman;
~PeerCleanup() { connman.ClearTestNodes(); }
} cleanup{connman};
FillableSigningProvider provider;
const CAmount amount{CoinJoin::GetSmallestDenomination()};
const int participants{std::max(3, CoinJoin::GetMinPoolParticipants())};
std::vector<CNode*> peers;
std::vector<CScript> previous_scripts;
std::vector<CCoinJoinEntry> entries;

for (int i{0}; i < participants; ++i) {
auto peer = MakePeer(i + 100, 0x0a000010 + i);
peer->fSuccessfullyConnected = true;
peers.push_back(peer.get());
connman.AddTestNode(*peer.release());
CKey key;
key.MakeNewKey(true);
BOOST_REQUIRE(provider.AddKey(key));
const CScript previous_script{GetScriptForDestination(PKHash(key.GetPubKey()))};
previous_scripts.push_back(previous_script);
const COutPoint input{uint256::ONE, static_cast<uint32_t>(100 + participants - i)};
const COutPoint collateral_input{uint256::TWO, static_cast<uint32_t>(100 + i)};
{
LOCK(cs_main);
auto& coins{m_node.chainman->ActiveChainstate().CoinsTip()};
coins.AddCoin(input, Coin{CTxOut{amount, previous_script}, 0, false}, false);
coins.AddCoin(collateral_input, Coin{CTxOut{COIN / 10, previous_script}, 0, false}, false);
}
CMutableTransaction collateral;
collateral.vin.emplace_back(collateral_input);
collateral.vout.emplace_back(COIN / 10 - CoinJoin::GetCollateralAmount(), P2PKHScript(100 + i));
BOOST_REQUIRE(SignSignature(provider, previous_script, collateral, 0, COIN / 10, SIGHASH_ALL));
server.SeedSessionCollateral(collateral, CoinJoin::MixShape::STANDARD);
// Descending output order requires verification against the canonical BIP69 transaction.
CCoinJoinEntry original{{CTxDSIn{CTxIn{input}, previous_script, 0}},
{CTxOut{amount, P2PKHScript(participants - i)}}, CTransaction{collateral}};
CDataStream wire{SER_NETWORK, PROTOCOL_VERSION};
wire << original;
CCoinJoinEntry entry;
wire >> entry;
BOOST_REQUIRE(entry.vecTxDSIn[0].prevPubKey.empty());
entry.addr = peers[i == participants - 1 ? 0 : i]->addr;
entries.push_back(std::move(entry));
}
server.EnterAcceptingEntriesState(CoinJoin::AmountToDenomination(amount));
for (const auto& entry : entries) {
PoolMessage message{MSG_NOERR};
BOOST_REQUIRE(server.AddEntry(entry, message));
}
server.CreateFinalTransaction(1);
BOOST_REQUIRE_EQUAL(server.GetState(), int{POOL_STATE_SIGNING});
CMutableTransaction final{server.FinalTransaction()};
BOOST_REQUIRE(final.vout.front().scriptPubKey == P2PKHScript(1));

const auto input_index = [&final](const COutPoint& prevout) {
return std::distance(final.vin.begin(), std::ranges::find(final.vin, prevout, &CTxIn::prevout));
};
std::vector<CTxIn> signed_inputs;
for (int i{0}; i < participants; ++i) {
const auto index{input_index(entries[i].vecTxDSIn[0].prevout)};
BOOST_REQUIRE(SignSignature(provider, previous_scripts[i], final, index, amount,
SIGHASH_ALL | (i == 0 ? SIGHASH_ANYONECANPAY : 0)));
signed_inputs.push_back(final.vin[index]);
}
const auto rejected = [](CNode& peer) {
LOCK(peer.cs_vSend);
for (const auto& msg : peer.vSendMsg) {
if (msg.m_type != NetMsgType::DSSTATUSUPDATE) continue;
CDataStream wire{msg.data, SER_NETWORK, PROTOCOL_VERSION};
CCoinJoinStatusUpdate status;
wire >> status;
if (status.nStatusUpdate == STATUS_REJECTED) return true;
}
return false;
};
const auto submit = [&server, &peers, participants](int participant, const CTxIn& input) {
for (auto* peer : peers) {
LOCK(peer->cs_vSend);
peer->vSendMsg.clear();
}
CDataStream wire{SER_NETWORK, PROTOCOL_VERSION};
wire << std::vector<CTxIn>{input};
server.ProcessMessage(*peers[participant == participants - 1 ? 0 : participant], NetMsgType::DSSIGNFINALTX, wire);
};
const auto check_rejected = [&](int participant, const CTxIn& input) {
const auto before{server.FinalTransaction()};
submit(participant, input);
BOOST_CHECK(rejected(*peers[participant == participants - 1 ? 0 : participant]));
BOOST_CHECK(server.FinalTransaction().GetHash() == before.GetHash());
BOOST_CHECK_EQUAL(server.GetState(), int{POOL_STATE_SIGNING});
};

// An admitted connection cannot sign an input belonging to another connection.
check_rejected(1, signed_inputs[0]);
CMutableTransaction modified_proposal{final};
modified_proposal.vout.front().scriptPubKey = P2PKHScript(0xff);
const auto first_index{input_index(signed_inputs[0].prevout)};
constexpr int legacy_mode{SIGHASH_ALL | SIGHASH_ANYONECANPAY};
BOOST_REQUIRE(SignSignature(provider, previous_scripts[0], modified_proposal, first_index, amount, legacy_mode));
BOOST_REQUIRE(VerifyScript(modified_proposal.vin[first_index].scriptSig, previous_scripts[0],
STANDARD_SCRIPT_VERIFY_FLAGS,
MutableTransactionSignatureChecker(&modified_proposal, first_index, amount,
MissingDataBehavior::ASSERT_FAIL)));
check_rejected(0, modified_proposal.vin[first_index]);

submit(0, signed_inputs[0]);
BOOST_CHECK(!rejected(*peers[0]));
// Multiple entries on this connection can receive the same final proposal again.
submit(0, signed_inputs[0]);
BOOST_CHECK(!rejected(*peers[0]));
CTxIn invalid_retry{signed_inputs[0]};
invalid_retry.scriptSig.clear();
check_rejected(0, invalid_retry);
Comment on lines +366 to +373

@thepastaclaw thepastaclaw Oct 7, 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.

⏭️ Deferred (maintainer decision; not blocking) at 14edd931; see the replies below.

🟡 Suggestion: Cover alternate valid retries and post-admission coin changes

The scenario exercises an identical retransmission and an invalid non-identical retry, but not a different valid signature after acceptance or a coin disappearing after entry admission. These are explicit PR guarantees implemented by retained prevPubKey and server.cpp:874, and neither is protected by the current assertions. Rejecting all different valid retries, replacing the first accepted signature, or re-reading chainstate during signature verification would therefore go undetected.

Extend this public coordinator scenario with both controls. After accepting the first legacy signature, sign the same canonical proposal with SIGHASH_ALL, require different scriptSig bytes and successful VerifyScript verification, then submit it through ProcessMessage and check that no rejection is emitted and the original signature remains unchanged. Separately, SpendCoin after AddEntry but before the first valid submission, check successful signature admission and signing state, and restore the coin before the remaining submissions trigger mempool acceptance. Switching sighash modes avoids the nonce-overlap complication in the removed test.

source: muse-spark-1.3-contributor (phase1-reviewer: general); gpt-6.1-sol (phase2-reviewer: general, dash-core-commit-history)

@PastaPastaPasta PastaPastaPasta Oct 7, 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.

Declining: the alternate-valid-signature and post-admission coin-change scenarios were removed from this test on purpose during the maintainer cleanup. They mostly re-exercised AddScriptSig replacement and coin-view setup rather than the fix, and the test was already several scenarios in one case. The kept assertions (wrong-peer and modified-proposal rejection, identical retransmission, mixed ACP/ALL completion) are the ones that fail without the fix.

🤖 Posted autonomously by Claude on behalf of pasta.

@thepastaclaw thepastaclaw Oct 7, 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.

I accept the deliberate choice to leave those two scenarios out and keep this regression focused; after checking that verification uses the retained script and that a different valid retry returns without replacing the accepted signature, I don't see a correctness defect that makes the extra coverage necessary for merge.

Noted as intentionally deferred; I will not press it further here.

for (int i{1}; i < participants; ++i) {
submit(i, signed_inputs[i]);
BOOST_CHECK(!rejected(*peers[i == participants - 1 ? 0 : i]));
}
BOOST_CHECK(m_node.mempool->exists(final.GetHash()));
BOOST_CHECK_EQUAL(server.GetState(), int{POOL_STATE_IDLE});
}

BOOST_AUTO_TEST_CASE(server_signfinaltx_nonparticipant_cannot_abort_session)
{
BOOST_REQUIRE(m_node.mn_sync);
Expand Down
Loading