Skip to content

backport: bind CoinJoin signatures to the approved transaction (#7815) for v24.0.x #8022

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

Closed
PastaPastaPasta wants to merge 3 commits into from
Closed

backport: bind CoinJoin signatures to the approved transaction (#7815) for v24.0.x #8022

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7371cdd
fix(coinjoin): bind signatures to the approved transaction
PastaPastaPasta Oct 7, 2026
9390a67
test: cover CoinJoin signature admission rejection
PastaPastaPasta Oct 7, 2026
62609bf
test: reject a CoinJoin signature that only passes consensus rules
PastaPastaPasta Nov 3, 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 @@ -281,7 +285,7 @@ void CCoinJoinServer::ProcessDSSIGNFINALTX(CNode& peer, CDataStream& vRecv)
int nTxInsCount = static_cast<int>(vecTxIn.size());
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, session_id);
RelayStatus(STATUS_REJECTED);
Expand Down Expand Up @@ -778,51 +782,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 @@ -966,6 +925,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 @@ -979,7 +953,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 @@ -988,41 +962,42 @@ 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)
{
AssertLockHeld(cs_coinjoin);
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::AddScriptSig -- scriptSig=%s\n", ScriptToAsmStr(txinNew.scriptSig).substr(0, 24));

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 @@ -147,7 +147,7 @@ class CCoinJoinServer : public CCoinJoinBaseSession, public NetHandler
Mutex cs_check_pool;

/// 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);

/// Collaterals chosen to be charged whose consumption has not finished yet. They are chosen
/// under cs_coinjoin, but ConsumeCollateral() takes cs_main and runs after cs_coinjoin is
Expand Down Expand Up @@ -210,8 +210,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.
// Requires cs_coinjoin so that a transition and the session data it describes are always
Expand Down
157 changes: 156 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,16 +10,25 @@
#include <coinjoin/common.h>
#include <coinjoin/options.h>
#include <coinjoin/server.h>
#include <coins.h>
#include <evo/chainhelper.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 @@ -30,7 +39,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,144 @@ 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));
Comment thread
PastaPastaPasta marked this conversation as resolved.
Resolved
for (const auto& entry : entries) {
PoolMessage message{MSG_NOERR};
BOOST_REQUIRE(server.AddEntry(entry, message));
}
server.CreateFinalTransaction(/*session_id=*/1, /*charge_fees=*/false);
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]);
// A signature that only passes consensus rules would keep the final transaction out of the mempool.
CTxIn padded{signed_inputs[0]};
padded.scriptSig.insert(padded.scriptSig.begin(), OP_1);
BOOST_REQUIRE(VerifyScript(padded.scriptSig, previous_scripts[0], SCRIPT_VERIFY_P2SH,
MutableTransactionSignatureChecker(&final, first_index, amount,
MissingDataBehavior::ASSERT_FAIL)));
check_rejected(0, padded);

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);
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