From 531f11784411a292dac39fb242d685fd5a0066ce Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 19 Aug 2026 14:29:38 -0500 Subject: [PATCH] fix(coinjoin): recheck live side coverage before finalizing timeout session The timeout branch in CheckPool decides to finalize based on a snapshot taken before ChargeFees, but entries can commit while that call runs since the session stays in POOL_STATE_ACCEPTING_ENTRIES. This could change a covered 0/3 side into an uncovered 1/3 side, which CreateFinalTransaction previously accepted after only checking the session id/state and then built from the live vecEntries. Validate the live MixSideCounts and minimum entry count under the same cs_coinjoin lock used to build the final transaction so the decision cannot be invalidated between check and construction. Add a unit test exercising the 3-demotion + 1-promotion (1/3) regression that must stay in accepting state until the second promoter arrives. --- src/coinjoin/server.cpp | 14 +++++++++- src/coinjoin/server.h | 4 +-- src/test/coinjoin_inouts_tests.cpp | 41 +++++++++++++++++++++++++++++- 3 files changed, 55 insertions(+), 4 deletions(-) diff --git a/src/coinjoin/server.cpp b/src/coinjoin/server.cpp index 15d0e9961a5c..8146bb269542 100644 --- a/src/coinjoin/server.cpp +++ b/src/coinjoin/server.cpp @@ -373,11 +373,23 @@ void CCoinJoinServer::CreateFinalTransaction(int session_id) LOCK(cs_coinjoin); // The decision to finalize came from a snapshot taken before this lock, so make sure it - // still describes the live session - it may have timed out and been replaced in between. + // still describes an eligible live session. An entry can finish validation and commit while + // the timeout path charges fees, changing a covered side from empty to a lone participant. + // Check the live entries under the same lock used to build the transaction so that entry + // admission cannot invalidate the decision before the state moves to signing. if (!IsCurrentSession(session_id)) { LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CreateFinalTransaction -- session changed, not finalizing\n"); return; } + const auto sides = GetMixSideCountsLocked(); + if (vecEntries.size() < static_cast(CoinJoin::GetMinPoolParticipants()) || !sides.IsCovered()) { + LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CreateFinalTransaction -- session no longer eligible, entries=%d, sides=%d/%d\n", + vecEntries.size(), sides.inputs, sides.outputs); + // Deliberately no timer refresh: on the timeout path the session stays timed out and + // the scheduler's next CheckTimeout() resets it. The missing side already had the full + // entry window, so waiting another one would only keep everyone else's coins locked. + return; + } CMutableTransaction txNew; diff --git a/src/coinjoin/server.h b/src/coinjoin/server.h index 080aae76716f..475808c3052b 100644 --- a/src/coinjoin/server.h +++ b/src/coinjoin/server.h @@ -76,6 +76,8 @@ class CCoinJoinServer : public CCoinJoinBaseSession, public NetHandler bool AddEntry(const CCoinJoinEntry& entry, PoolMessage& nMessageIDRet) EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin); /// Record an accepted collateral and index its input prevouts void CommitSessionCollateral(const CMutableTransaction& txCollateral) EXCLUSIVE_LOCKS_REQUIRED(cs_coinjoin); + /// Build and relay the final transaction if the live session is still eligible + void CreateFinalTransaction(int session_id) EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin); private: bool fUnitTest; @@ -114,8 +116,6 @@ class CCoinJoinServer : public CCoinJoinBaseSession, public NetHandler /// Check for process void CheckPool(); - /// Build and relay the final transaction, unless session_id is no longer the live session - void CreateFinalTransaction(int session_id) EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin); void CommitFinalTransaction() EXCLUSIVE_LOCKS_REQUIRED(!cs_coinjoin); /// Is this nDenom and txCollateral acceptable? diff --git a/src/test/coinjoin_inouts_tests.cpp b/src/test/coinjoin_inouts_tests.cpp index e4d19fcb32d1..8050184f0026 100644 --- a/src/test/coinjoin_inouts_tests.cpp +++ b/src/test/coinjoin_inouts_tests.cpp @@ -181,8 +181,9 @@ BOOST_AUTO_TEST_CASE(entry_addscriptsig_matches_and_rejects) class TestableCoinJoinServer : public CCoinJoinServer { public: - using CCoinJoinServer::CCoinJoinServer; using CCoinJoinServer::AddEntry; + using CCoinJoinServer::CCoinJoinServer; + using CCoinJoinServer::CreateFinalTransaction; // 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. @@ -427,6 +428,44 @@ BOOST_AUTO_TEST_CASE(server_addentry_rejects_entries_once_the_session_finalized) BOOST_CHECK_EQUAL(server.GetEntriesCount(), 0); } +BOOST_AUTO_TEST_CASE(server_finalization_rechecks_live_side_coverage) +{ + 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.isman)); + + const auto make_entry = [](CoinJoin::MixShape shape, uint32_t tag) { + const size_t input_count{shape == CoinJoin::MixShape::PROMOTION ? size_t{CoinJoin::PROMOTION_RATIO} : 1}; + const size_t output_count{shape == CoinJoin::MixShape::DEMOTION ? size_t{CoinJoin::PROMOTION_RATIO} : 1}; + std::vector inputs; + std::vector outputs; + for (size_t i{0}; i < input_count; ++i) { + inputs.emplace_back(CTxIn{COutPoint{uint256::ONE, tag + static_cast(i)}}, P2PKHScript(), 0); + } + for (size_t i{0}; i < output_count; ++i) { + outputs.emplace_back(CoinJoin::GetSmallestDenomination(), P2PKHScript(static_cast(tag + i))); + } + return CCoinJoinEntry{inputs, outputs, CTransaction{CMutableTransaction{}}}; + }; + + server.SeedEntry(make_entry(CoinJoin::MixShape::DEMOTION, 0)); + server.SeedEntry(make_entry(CoinJoin::MixShape::DEMOTION, 10)); + server.SeedEntry(make_entry(CoinJoin::MixShape::DEMOTION, 20)); + server.SeedEntry(make_entry(CoinJoin::MixShape::PROMOTION, 30)); + server.EnterAcceptingEntriesState(); + + // A timeout snapshot could have observed only the three demotions as covered (0/3), then + // this first promotion could commit while ChargeFees() ran. Finalization must use the live + // 1/3 side counts and refuse to build the uncovered transaction, staying out of + // POOL_STATE_SIGNING; the still-timed-out session is then reset by the scheduler's + // regular CheckTimeout() pass instead of leaking a lone promoter on-chain. + server.CreateFinalTransaction(/*session_id=*/1); + BOOST_CHECK_EQUAL(server.GetState(), int{POOL_STATE_ACCEPTING_ENTRIES}); + BOOST_CHECK_EQUAL(server.GetEntriesCount(), 4); +} + BOOST_AUTO_TEST_CASE(entry_deserializes_vectors_through_wire_cap) { const size_t wire_cap{CoinJoin::GetMaxPoolInputOutputCount()};