From 873b14e9558ac8d9c11d031504083853b5d30678 Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Fri, 26 Jun 2026 04:24:23 -0500 Subject: [PATCH] test(coinjoin): cover AddScriptSig, queue duplicate/readiness, dstx mempool re-entry Adds focused unit tests for behaviors the server/client mixing paths depend on but which were not previously exercised: - CCoinJoinEntry::AddScriptSig: matching prevout + sequence copies scriptSig and sets fHasSig; duplicate signature for the same input is rejected without overwriting; wrong prevout or wrong sequence is rejected; with multiple inputs only the matching one mutates. - CoinJoinQueueManager: HasQueueFromMasternode honors fReady; TryHasQueueFromMasternode finds by outpoint only; TryCheckDuplicate flags exact duplicates and same-masternode/same-readiness re-broadcasts but allows different readiness or a different masternode. - CDSTXManager: TransactionAddedToMempool clears the confirmed height set by BlockConnected (reorg/re-entry path); a mempool event for an unrelated tx does not mutate the stored DSTX. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/test/coinjoin_dstxmanager_tests.cpp | 67 +++++++++++++++++++++++ src/test/coinjoin_inouts_tests.cpp | 71 +++++++++++++++++++++++++ src/test/coinjoin_queue_tests.cpp | 55 +++++++++++++++++++ 3 files changed, 193 insertions(+) diff --git a/src/test/coinjoin_dstxmanager_tests.cpp b/src/test/coinjoin_dstxmanager_tests.cpp index 68bf245dec4b..1aeba26a923d 100644 --- a/src/test/coinjoin_dstxmanager_tests.cpp +++ b/src/test/coinjoin_dstxmanager_tests.cpp @@ -122,4 +122,71 @@ BOOST_AUTO_TEST_CASE(update_heights_block_connect_disconnect) } } +BOOST_AUTO_TEST_CASE(mempool_reentry_clears_confirmed_height) +{ + // After a block connect sets the confirmed height, the same tx being + // (re)added to the mempool, e.g. via a reorg, should clear it. This + // drives DSTX expiry logic that watches GetConfirmedHeight(). + CCoinJoinBroadcastTx dstx = MakeDSTX(); + auto& man = *Assert(m_node.dstxman); + man.AddDSTX(dstx); + + auto block = std::make_shared(); + block->vtx.push_back(dstx.tx); + CBlockIndex index; + index.nHeight = 200; + uint256 bh = uint256S("0c"); + index.phashBlock = &bh; + + man.BlockConnected(block, &index); + { + auto got = man.GetDSTX(dstx.tx->GetHash()); + BOOST_REQUIRE(static_cast(got)); + BOOST_REQUIRE(got.GetConfirmedHeight().has_value()); + BOOST_CHECK_EQUAL(*got.GetConfirmedHeight(), 200); + } + + // Re-entry to the mempool clears the confirmed height (without removing the entry). + man.TransactionAddedToMempool(dstx.tx); + { + auto got = man.GetDSTX(dstx.tx->GetHash()); + BOOST_REQUIRE(static_cast(got)); + BOOST_CHECK(!got.GetConfirmedHeight().has_value()); + } +} + +BOOST_AUTO_TEST_CASE(mempool_event_unrelated_tx_does_not_mutate_dstx) +{ + // TransactionAddedToMempool for a transaction that is not a tracked DSTX + // must be a no-op for any stored DSTX state. + CCoinJoinBroadcastTx dstx = MakeDSTX(); + auto& man = *Assert(m_node.dstxman); + man.AddDSTX(dstx); + + // Establish a confirmed height to detect any unwanted mutation. + auto block = std::make_shared(); + block->vtx.push_back(dstx.tx); + CBlockIndex index; + index.nHeight = 321; + uint256 bh = uint256S("0d"); + index.phashBlock = &bh; + man.BlockConnected(block, &index); + + // Build an unrelated transaction by mutating one input of the tracked tx so it hashes differently. + CMutableTransaction other_mtx(*dstx.tx); + other_mtx.vin[0].prevout = COutPoint(uint256S("ee"), 7); + auto other_tx = MakeTransactionRef(other_mtx); + BOOST_REQUIRE(other_tx->GetHash() != dstx.tx->GetHash()); + + man.TransactionAddedToMempool(other_tx); + + // Stored DSTX still has its confirmed height. + auto got = man.GetDSTX(dstx.tx->GetHash()); + BOOST_REQUIRE(static_cast(got)); + BOOST_REQUIRE(got.GetConfirmedHeight().has_value()); + BOOST_CHECK_EQUAL(*got.GetConfirmedHeight(), 321); + // Unrelated tx was not inserted as a DSTX. + BOOST_CHECK(!static_cast(man.GetDSTX(other_tx->GetHash()))); +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/coinjoin_inouts_tests.cpp b/src/test/coinjoin_inouts_tests.cpp index 8a41bd710c58..516aa4ae737a 100644 --- a/src/test/coinjoin_inouts_tests.cpp +++ b/src/test/coinjoin_inouts_tests.cpp @@ -84,6 +84,77 @@ BOOST_AUTO_TEST_CASE(broadcasttx_isvalidstructure_good_and_bad) BOOST_CHECK(!bad_amount.IsValidStructure()); } +BOOST_AUTO_TEST_CASE(entry_addscriptsig_matches_and_rejects) +{ + // Build an entry with two distinct inputs so we can check both the match + // and the isolation (only the matching input mutates). + const COutPoint op0(uint256::ONE, 0); + const COutPoint op1(uint256::ONE, 1); + const uint32_t seq0 = 0xfffffffeU; + const uint32_t seq1 = 0xfffffffdU; + + auto make_dsin = [](const COutPoint& op, uint32_t seq) { + CTxIn in(op); + in.nSequence = seq; + return CTxDSIn(in, P2PKHScript(0x10), /*nRounds=*/0); + }; + + std::vector dsins{make_dsin(op0, seq0), make_dsin(op1, seq1)}; + CCoinJoinEntry entry(dsins, /*vecTxOut=*/{}, CTransaction{CMutableTransaction{}}); + + // The scriptSig we expect to be copied across on a successful match. + const CScript scriptSig0 = CScript() << std::vector{0xde, 0xad} << std::vector{0xbe, 0xef}; + + // Matching prevout + matching sequence -> copies scriptSig, sets fHasSig. + { + CTxIn signed_in(op0, scriptSig0, seq0); + BOOST_CHECK(entry.AddScriptSig(signed_in)); + BOOST_CHECK(entry.vecTxDSIn[0].fHasSig); + BOOST_CHECK(entry.vecTxDSIn[0].scriptSig == scriptSig0); + // Other input is untouched. + BOOST_CHECK(!entry.vecTxDSIn[1].fHasSig); + BOOST_CHECK(entry.vecTxDSIn[1].scriptSig.empty()); + } + + // Duplicate signature for the already-signed input -> rejected, no overwrite. + { + const CScript scriptSig_other = CScript() << std::vector{0x01}; + CTxIn dup_in(op0, scriptSig_other, seq0); + BOOST_CHECK(!entry.AddScriptSig(dup_in)); + // Still holds the original signature. + BOOST_CHECK(entry.vecTxDSIn[0].scriptSig == scriptSig0); + BOOST_CHECK(entry.vecTxDSIn[0].fHasSig); + } + + // Wrong prevout (sequence matches an existing input) -> rejected. + { + const COutPoint op_wrong(uint256S("ff"), 9); + CTxIn wrong_in(op_wrong, scriptSig0, seq1); + BOOST_CHECK(!entry.AddScriptSig(wrong_in)); + BOOST_CHECK(!entry.vecTxDSIn[1].fHasSig); + BOOST_CHECK(entry.vecTxDSIn[1].scriptSig.empty()); + } + + // Right prevout but wrong sequence -> rejected (guards against malleated nSequence). + { + CTxIn badseq_in(op1, scriptSig0, /*nSequence=*/seq1 ^ 0xffU); + BOOST_CHECK(!entry.AddScriptSig(badseq_in)); + BOOST_CHECK(!entry.vecTxDSIn[1].fHasSig); + BOOST_CHECK(entry.vecTxDSIn[1].scriptSig.empty()); + } + + // Correct prevout + correct sequence on the second input -> succeeds, doesn't disturb the first. + { + const CScript scriptSig1 = CScript() << std::vector{0xca, 0xfe}; + CTxIn signed_in(op1, scriptSig1, seq1); + BOOST_CHECK(entry.AddScriptSig(signed_in)); + BOOST_CHECK(entry.vecTxDSIn[1].fHasSig); + BOOST_CHECK(entry.vecTxDSIn[1].scriptSig == scriptSig1); + // First input unchanged. + BOOST_CHECK(entry.vecTxDSIn[0].scriptSig == scriptSig0); + } +} + BOOST_AUTO_TEST_CASE(queue_timeout_bounds) { CCoinJoinQueue dsq; diff --git a/src/test/coinjoin_queue_tests.cpp b/src/test/coinjoin_queue_tests.cpp index a3e44281b230..8e1c3de6f564 100644 --- a/src/test/coinjoin_queue_tests.cpp +++ b/src/test/coinjoin_queue_tests.cpp @@ -188,4 +188,59 @@ BOOST_AUTO_TEST_CASE(queuemanager_getqueueitem_marks_tried_once) BOOST_CHECK(!man.GetQueueItemAndTry(picked2)); } +BOOST_AUTO_TEST_CASE(queuemanager_has_queue_from_masternode_readiness) +{ + CoinJoinQueueManager man; + const int denom = CoinJoin::AmountToDenomination(CoinJoin::GetSmallestDenomination()); + const int64_t now = GetAdjustedTime(); + const COutPoint mn(uint256S("31"), 0); + // Single queue from `mn` with fReady=false. + man.AddQueue(MakeQueue(denom, now, /*fReady=*/false, mn)); + + // Readiness-aware lookup honors fReady. + BOOST_CHECK(man.HasQueueFromMasternode(mn, /*fReady=*/false)); + BOOST_CHECK(!man.HasQueueFromMasternode(mn, /*fReady=*/true)); + + // Unrelated masternode is never matched. + const COutPoint other(uint256S("32"), 0); + BOOST_CHECK(!man.HasQueueFromMasternode(other, /*fReady=*/false)); + BOOST_CHECK(!man.HasQueueFromMasternode(other, /*fReady=*/true)); + + // TRY variant ignores readiness, finds by outpoint only. + auto try_present = man.TryHasQueueFromMasternode(mn); + BOOST_REQUIRE(try_present.has_value()); + BOOST_CHECK(*try_present); + + auto try_absent = man.TryHasQueueFromMasternode(other); + BOOST_REQUIRE(try_absent.has_value()); + BOOST_CHECK(!*try_absent); +} + +BOOST_AUTO_TEST_CASE(queuemanager_try_check_duplicate) +{ + CoinJoinQueueManager man; + const int denom = CoinJoin::AmountToDenomination(CoinJoin::GetSmallestDenomination()); + const int64_t now = GetAdjustedTime(); + const COutPoint mn(uint256S("41"), 0); + const COutPoint other_mn(uint256S("42"), 0); + + const auto seed = MakeQueue(denom, now, /*fReady=*/false, mn); + man.AddQueue(seed); + + auto is_dup = [&](const CCoinJoinQueue& q) { + auto r = man.TryCheckDuplicate(q); + BOOST_REQUIRE(r.has_value()); + return *r; + }; + + // Exact duplicate. + BOOST_CHECK(is_dup(seed)); + // Same masternode + same readiness, different nTime: the "too many dsqs" guard still flags it. + BOOST_CHECK(is_dup(MakeQueue(denom, now + 1, /*fReady=*/false, mn))); + // Same masternode but different readiness: allowed. + BOOST_CHECK(!is_dup(MakeQueue(denom, now, /*fReady=*/true, mn))); + // Different masternode: allowed. + BOOST_CHECK(!is_dup(MakeQueue(denom, now, /*fReady=*/false, other_mn))); +} + BOOST_AUTO_TEST_SUITE_END()