From 51ecf820929492e2a189db22bc9d8252b1df8d9d Mon Sep 17 00:00:00 2001 From: pasta Date: Sat, 25 Jul 2026 12:11:43 -0500 Subject: [PATCH 1/9] fix(coinjoin): keep mixed inputs locked until finalized tx is observed A client releases its session inputs as soon as it processes DSCOMPLETE(MSG_SUCCESS), but the finalized DSTX is relayed via delayed inventory trickling and can arrive much later. During that window the inputs are neither wallet-locked nor marked spent, so another session (especially with -coinjoinmultisession=1) can select and sign them again, producing two valid CoinJoin transactions spending the same outpoint. Conflicting InstantSend votes then leave both transactions without an ISLock, which can stall ChainLocks. Instead of unlocking on success, move the session's mixing inputs into a per-wallet pending-observation set: they stay wallet-locked and are released only once the wallet observes a mempool or confirmed transaction spending them. The set is mirrored to the wallet database under a CoinJoin-specific record, so both the locks and their grace period survive a restart and are never confused with locks the user set themselves via lockunspent. If no spend is ever observed the input is released after a conservative timeout, but only on a synced chain and only if it is unspent per the chain UTXO set and the mempool. Both checks are needed: findCoins() reports outputs which exist in the UTXO set or are created by a mempool transaction, but CCoinsViewMemPool::GetCoin never consults mapNextTx, so a finalized mixing transaction sitting unprocessed in our own mempool would otherwise be misread as "never propagated". The release check runs before the mixing gates in DoMaintenance so pending inputs can still unlock while mixing or CoinJoin itself is disabled. In block-only mode no CoinJoin maintenance is scheduled at all, so the check is scheduled on its own there - locks restored from a previous run would never be released otherwise. A manual user unlock purges the pending entry, and collateral inputs and all failure paths keep the old immediate-unlock behavior. Co-Authored-By: Claude Fable 5 --- src/coinjoin/client.cpp | 169 ++++++++++++++++++++++++++++++++++++- src/coinjoin/client.h | 36 +++++++- src/coinjoin/walletman.cpp | 21 ++++- src/rpc/coinjoin.cpp | 1 + src/wallet/walletdb.cpp | 13 ++- src/wallet/walletdb.h | 8 ++ 6 files changed, 243 insertions(+), 5 deletions(-) diff --git a/src/coinjoin/client.cpp b/src/coinjoin/client.cpp index fece2711d5c0..81572ec4d3c2 100644 --- a/src/coinjoin/client.cpp +++ b/src/coinjoin/client.cpp @@ -14,8 +14,10 @@ #include #include +#include #include #include +#include #include #include #include @@ -28,6 +30,7 @@ #include #include #include +#include #include #include @@ -577,6 +580,33 @@ void CCoinJoinClientSession::CompletedTransaction(PoolMessage nMessageID) if (nMessageID == MSG_SUCCESS) { m_clientman.UpdatedSuccessBlock(); keyHolderStorage.KeepAll(); + // Our inputs are now committed to the finalized mixing transaction but the + // DSCOMPLETE message may arrive well before the (trickle-relayed) DSTX does. + // Do not release those inputs yet: keep them locked until the wallet actually + // observes a transaction spending them, otherwise another session could pick + // them up in the meantime (esp. with -coinjoinmultisession) and double-spend + // them. Anything else we locked (e.g. collateral inputs which may never be + // spent) is unlocked immediately as before. + std::set setMixingInputs; + { + LOCK(cs_coinjoin); + for (const auto& entry : vecEntries) { + for (const auto& txdsin : entry.vecTxDSIn) { + setMixingInputs.emplace(txdsin.prevout); + } + } + } + std::vector vecPending; + std::vector vecUnlockNow; + for (const auto& outpoint : vecOutPointLocked) { + if (setMixingInputs.count(outpoint)) { + vecPending.push_back(outpoint); + } else { + vecUnlockNow.push_back(outpoint); + } + } + vecOutPointLocked = std::move(vecUnlockNow); + m_clientman.AddPendingObservation(vecPending); WalletCJLogPrint(m_wallet, "CompletedTransaction -- success\n"); } else { WITH_LOCK(m_wallet->cs_wallet, keyHolderStorage.ReturnAll()); @@ -592,6 +622,135 @@ void CCoinJoinClientManager::UpdatedSuccessBlock() nCachedLastSuccessBlock = nCachedBlockHeight; } +void CCoinJoinClientManager::AddPendingObservation(const std::vector& outpoints) +{ + AssertLockNotHeld(cs_pending_obs); + if (outpoints.empty()) return; + + LOCK(m_wallet->cs_wallet); + LOCK(cs_pending_obs); + wallet::WalletBatch batch(m_wallet->GetDatabase()); + LoadPendingObservations(batch); + + const int64_t nNow{GetTime()}; + bool fPersisted{true}; + for (const auto& outpoint : outpoints) { + // Persist the lock so that a restart before the finalized transaction is + // observed cannot make the input available for selection again + fPersisted &= m_wallet->LockCoin(outpoint, &batch); + m_pending_obs.emplace(outpoint, nNow); + WalletCJLogPrint(m_wallet, "CCoinJoinClientManager::%s -- %s is locked until the finalized mixing transaction is observed\n", + __func__, outpoint.ToStringShort()); + } + fPersisted &= batch.WriteCoinJoinPendingObs(m_pending_obs); + if (!fPersisted) { + // The in-memory lock still protects these inputs for as long as this process + // runs, but a restart would make them selectable again while a valid mixing + // transaction spending them may already be in flight. Nothing we can do about + // it here beyond making the failure loud - the wallet database is broken. + LogPrintf("CCoinJoinClientManager::%s -- ERROR: failed to persist locks for %d successfully mixed input(s), " + "they will not survive a restart\n", + __func__, outpoints.size()); + } +} + +void CCoinJoinClientManager::LoadPendingObservations(wallet::WalletBatch& batch) +{ + AssertLockHeld(cs_pending_obs); + + if (m_pending_obs_loaded) return; + m_pending_obs_loaded = true; + // Missing record simply means nothing was pending when we last shut down + if (!batch.ReadCoinJoinPendingObs(m_pending_obs)) return; + WalletCJLogPrint(m_wallet, "CCoinJoinClientManager::%s -- loaded %d pending observation(s)\n", __func__, + m_pending_obs.size()); +} + +void CCoinJoinClientManager::CheckPendingObservations(const CTxMemPool& mempool) +{ + AssertLockNotHeld(cs_pending_obs); + + // nothing to do in the common case, don't touch the wallet at all + if (WITH_LOCK(cs_pending_obs, return m_pending_obs_loaded && m_pending_obs.empty())) return; + + LOCK(m_wallet->cs_wallet); + LOCK(cs_pending_obs); + + wallet::WalletBatch batch(m_wallet->GetDatabase()); + LoadPendingObservations(batch); + + const int64_t nNow{GetTime()}; + const bool fSynced{m_mn_sync.IsBlockchainSynced()}; + bool fChanged{false}; + for (auto it = m_pending_obs.begin(); it != m_pending_obs.end();) { + const COutPoint& outpoint = it->first; + if (!m_wallet->IsLockedCoin(outpoint)) { + // The user released the lock manually (e.g. via lockunspent), respect that + it = m_pending_obs.erase(it); + fChanged = true; + continue; + } + if (m_wallet->IsSpent(outpoint)) { + // The wallet observed a transaction spending this input, it is no longer + // selectable anyway, drop the protective lock + WalletCJLogPrint(m_wallet, "CCoinJoinClientManager::%s -- observed spend of %s, releasing lock\n", __func__, + outpoint.ToStringShort()); + m_wallet->UnlockCoin(outpoint, &batch); + it = m_pending_obs.erase(it); + fChanged = true; + continue; + } + // Only ever consult chain/mempool spentness on a synced chain: while catching up + // the spending transaction may sit in a block we have not downloaded yet, and + // releasing the input on the strength of that would defeat the whole point + if (fSynced && nNow - it->second >= PENDING_OBSERVATION_TIMEOUT_SECONDS) { + // NOTE: findCoins() reports outputs which exist in the chain UTXO set or are + // created by a mempool transaction; it does NOT know whether a mempool + // transaction spends them (CCoinsViewMemPool::GetCoin never looks at + // mapNextTx). The finalized mixing transaction sitting unprocessed in our own + // mempool is exactly the case we must not misread as "never propagated", so + // ask the mempool about spentness explicitly as well. + std::map coins{{outpoint, Coin{}}}; + m_wallet->chain().findCoins(coins); + if (!coins.at(outpoint).IsSpent() && !mempool.isSpent(outpoint)) { + // Still unspent in chain and mempool long after the session completed - + // the finalized transaction most likely never propagated, release the + // input so the wallet does not lose it forever + LogPrintf("CCoinJoinClientManager::%s -- WARNING: never observed finalized mixing transaction for %s, " + "releasing lock after %d seconds\n", + __func__, outpoint.ToStringShort(), PENDING_OBSERVATION_TIMEOUT_SECONDS); + m_wallet->UnlockCoin(outpoint, &batch); + it = m_pending_obs.erase(it); + fChanged = true; + continue; + } + // Spent according to chain/mempool but the wallet has not recorded the + // spending transaction yet, keep waiting for it + it->second = nNow; + fChanged = true; + } + ++it; + } + if (fChanged && !batch.WriteCoinJoinPendingObs(m_pending_obs)) { + LogPrintf("CCoinJoinClientManager::%s -- ERROR: failed to persist %d pending observation(s)\n", __func__, + m_pending_obs.size()); + } +} + +bool CCoinJoinClientManager::IsPendingObservation(const COutPoint& outpoint) const +{ + AssertLockNotHeld(cs_pending_obs); + LOCK(cs_pending_obs); + return m_pending_obs.count(outpoint) > 0; +} + +size_t CCoinJoinClientManager::GetPendingObservationCount() const +{ + AssertLockNotHeld(cs_pending_obs); + LOCK(cs_pending_obs); + return m_pending_obs.size(); +} + bool CCoinJoinClientManager::WaitForAnotherBlock() const { if (!m_mn_sync.IsBlockchainSynced()) return true; @@ -1714,9 +1873,16 @@ void CCoinJoinClientManager::UpdatedBlockTip(const CBlockIndex* pindex) void CCoinJoinClientManager::DoMaintenance(ChainstateManager& chainman, CConnman& connman, const CTxMemPool& mempool) { + if (ShutdownRequested()) return; + + // Run this before the mixing gates below: inputs of already completed sessions + // must be able to unlock even when mixing or CoinJoin itself is disabled, + // otherwise their persisted locks could linger forever + CheckPendingObservations(mempool); + if (!CCoinJoinClientOptions::IsEnabled()) return; - if (!m_mn_sync.IsBlockchainSynced() || ShutdownRequested()) return; + if (!m_mn_sync.IsBlockchainSynced()) return; static int nTick = 0; static int nDoAutoNextRun = nTick + COINJOIN_AUTO_TIMEOUT_MIN; @@ -1751,6 +1917,7 @@ UniValue CCoinJoinClientManager::getJsonInfo() const { UniValue obj(UniValue::VOBJ); obj.pushKV("running", isMixing()); + obj.pushKV("pending_inputs", static_cast(GetPendingObservationCount())); UniValue arrSessions(UniValue::VARR); AssertLockNotHeld(cs_deqsessions); diff --git a/src/coinjoin/client.h b/src/coinjoin/client.h index 90fd6928783c..7d88f3acf12c 100644 --- a/src/coinjoin/client.h +++ b/src/coinjoin/client.h @@ -12,10 +12,15 @@ #include #include +#include #include #include #include +namespace wallet { +class WalletBatch; +} // namespace wallet + class CCoinJoinClientManager; class CConnman; class CDeterministicMNManager; @@ -174,6 +179,21 @@ class CCoinJoinClientManager : public interfaces::CoinJoin::Client // how many blocks to wait for after one successful mixing tx in non-multisession mode static constexpr int nMinBlocksToWait{1}; + mutable Mutex cs_pending_obs; + //! Inputs of successfully completed mixing sessions which must stay locked until + //! the wallet observes the finalized mixing transaction spending them. Without this + //! a client which processes DSCOMPLETE(MSG_SUCCESS) before the (trickle-relayed) + //! finalized DSTX arrives could select the very same inputs for another session + //! (esp. with -coinjoinmultisession) and double-spend them. + //! Maps outpoint to the time it was added. Mirrored to the wallet database so that + //! both the locks and their grace period survive a restart, and so that these are + //! never confused with locks the user set themselves via `lockunspent`. + std::map m_pending_obs GUARDED_BY(cs_pending_obs); + bool m_pending_obs_loaded GUARDED_BY(cs_pending_obs){false}; + + /// Populate m_pending_obs from the wallet database, once per run + void LoadPendingObservations(wallet::WalletBatch& batch) EXCLUSIVE_LOCKS_REQUIRED(cs_pending_obs); + // Keep track of current block height int nCachedBlockHeight{0}; @@ -211,15 +231,27 @@ class CCoinJoinClientManager : public interfaces::CoinJoin::Client void UpdatedSuccessBlock(); + //! How long to keep waiting for the finalized mixing transaction before + //! double-checking chain/mempool spentness and potentially releasing the inputs + static constexpr int64_t PENDING_OBSERVATION_TIMEOUT_SECONDS{60 * 60}; + + /// Keep the given successfully mixed inputs locked (persistently) until the wallet + /// observes a transaction spending them + void AddPendingObservation(const std::vector& outpoints) EXCLUSIVE_LOCKS_REQUIRED(!cs_pending_obs); + /// Release pending inputs whose spend has been observed by the wallet + void CheckPendingObservations(const CTxMemPool& mempool) EXCLUSIVE_LOCKS_REQUIRED(!cs_pending_obs); + bool IsPendingObservation(const COutPoint& outpoint) const EXCLUSIVE_LOCKS_REQUIRED(!cs_pending_obs); + size_t GetPendingObservationCount() const EXCLUSIVE_LOCKS_REQUIRED(!cs_pending_obs); + void UpdatedBlockTip(const CBlockIndex* pindex); void DoMaintenance(ChainstateManager& chainman, CConnman& connman, const CTxMemPool& mempool) - EXCLUSIVE_LOCKS_REQUIRED(!cs_deqsessions); + EXCLUSIVE_LOCKS_REQUIRED(!cs_deqsessions, !cs_pending_obs); // interfaces::CoinJoin::Client overrides void disableAutobackups() override { fCreateAutoBackups = false; } void resetPool() override EXCLUSIVE_LOCKS_REQUIRED(!cs_deqsessions); - UniValue getJsonInfo() const override EXCLUSIVE_LOCKS_REQUIRED(!cs_deqsessions); + UniValue getJsonInfo() const override EXCLUSIVE_LOCKS_REQUIRED(!cs_deqsessions, !cs_pending_obs); std::vector getSessionStatuses() const override EXCLUSIVE_LOCKS_REQUIRED(!cs_deqsessions); std::string getSessionDenoms() const override EXCLUSIVE_LOCKS_REQUIRED(!cs_deqsessions); bool isMixing() const override; diff --git a/src/coinjoin/walletman.cpp b/src/coinjoin/walletman.cpp index 2ae939688a5f..dab047af7a7b 100644 --- a/src/coinjoin/walletman.cpp +++ b/src/coinjoin/walletman.cpp @@ -73,6 +73,7 @@ class CJWalletManagerImpl final : public CJWalletManager std::map> m_wallet_manager_map GUARDED_BY(cs_wallet_manager_map); void DoMaintenance(CConnman& connman) EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet_manager_map); + void CheckPendingObservations() EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet_manager_map); [[nodiscard]] MessageProcessingResult ProcessDSQueue(NodeId from, CConnman& connman, std::string_view msg_type, CDataStream& vRecv) EXCLUSIVE_LOCKS_REQUIRED(!cs_ProcessDSQueue, !cs_wallet_manager_map); @@ -119,7 +120,16 @@ CJWalletManagerImpl::~CJWalletManagerImpl() void CJWalletManagerImpl::Schedule(CConnman& connman, CScheduler& scheduler) { - if (!m_relay_txes) return; + if (!m_relay_txes) { + // Mixing needs transaction relay, so no CoinJoin activity is scheduled here. + // Inputs of a session which completed before an earlier shutdown are a different + // matter: their locks were persisted and are restored with the wallet, so the + // check which releases them once their spend is observed (or once they time out) + // has to keep running even in block-only mode - nothing else would ever unlock them. + scheduler.scheduleEvery(std::bind(&CJWalletManagerImpl::CheckPendingObservations, this), + std::chrono::minutes{1}); + return; + } scheduler.scheduleEvery(std::bind(&CJWalletManagerImpl::DoMaintenance, this, std::ref(connman)), std::chrono::seconds{1}); } @@ -205,6 +215,15 @@ void CJWalletManagerImpl::DoMaintenance(CConnman& connman) } } +void CJWalletManagerImpl::CheckPendingObservations() +{ + if (ShutdownRequested()) return; + LOCK(cs_wallet_manager_map); + for (auto& [_, clientman] : m_wallet_manager_map) { + clientman->CheckPendingObservations(m_mempool); + } +} + MessageProcessingResult CJWalletManagerImpl::processMessage(CNode& pfrom, Chainstate& chainstate, CConnman& connman, CTxMemPool& mempool, std::string_view msg_type, CDataStream& vRecv) diff --git a/src/rpc/coinjoin.cpp b/src/rpc/coinjoin.cpp index b5afdb84df12..bdb91b1da51d 100644 --- a/src/rpc/coinjoin.cpp +++ b/src/rpc/coinjoin.cpp @@ -429,6 +429,7 @@ static RPCHelpMan getcoinjoininfo() {RPCResult::Type::NUM, "denoms_hardcap", "Maximum limit of how many inputs of each denominated amount to create"}, {RPCResult::Type::NUM, "queue_size", "How many queues there are currently on the network"}, {RPCResult::Type::BOOL, "running", "Whether mixing is currently running"}, + {RPCResult::Type::NUM, "pending_inputs", "The number of successfully mixed inputs kept locked until the transaction spending them is observed"}, {RPCResult::Type::ARR, "sessions", "", { {RPCResult::Type::OBJ, "", "", diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index ea0d80cef37b..f7cb440e1b4b 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -38,6 +38,7 @@ const std::string BESTBLOCK_NOMERKLE{"bestblock_nomerkle"}; const std::string BESTBLOCK{"bestblock"}; const std::string CRYPTED_KEY{"ckey"}; const std::string CRYPTED_HDCHAIN{"chdchain"}; +const std::string COINJOIN_PENDING_OBS{"cj_pending_obs"}; const std::string COINJOIN_SALT{"cj_salt"}; const std::string CSCRIPT{"cscript"}; const std::string DEFAULTKEY{"defaultkey"}; @@ -226,6 +227,16 @@ bool WalletBatch::WriteCoinJoinSalt(const uint256& salt) return WriteIC(DBKeys::COINJOIN_SALT, salt); } +bool WalletBatch::ReadCoinJoinPendingObs(std::map& pending_obs) +{ + return m_batch->Read(std::string(DBKeys::COINJOIN_PENDING_OBS), pending_obs); +} + +bool WalletBatch::WriteCoinJoinPendingObs(const std::map& pending_obs) +{ + return WriteIC(DBKeys::COINJOIN_PENDING_OBS, pending_obs); +} + bool WalletBatch::WriteGovernanceObject(const Governance::Object& obj) { return WriteIC(std::make_pair(DBKeys::G_OBJECT, obj.GetHash()), obj, false); @@ -778,7 +789,7 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, strType != DBKeys::MINVERSION && strType != DBKeys::ACENTRY && strType != DBKeys::VERSION && strType != DBKeys::SETTINGS && strType != DBKeys::PRIVATESEND_SALT && strType != DBKeys::COINJOIN_SALT && - strType != DBKeys::FLAGS) { + strType != DBKeys::COINJOIN_PENDING_OBS && strType != DBKeys::FLAGS) { wss.m_unknown_records++; } } catch (const std::exception& e) { diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index eb5ce4b37d9a..a91f3240467b 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -13,6 +13,7 @@ #include #include +#include #include #include @@ -68,6 +69,7 @@ extern const std::string BESTBLOCK; extern const std::string BESTBLOCK_NOMERKLE; extern const std::string CRYPTED_HDCHAIN; extern const std::string CRYPTED_KEY; +extern const std::string COINJOIN_PENDING_OBS; extern const std::string COINJOIN_SALT; extern const std::string CSCRIPT; extern const std::string DEFAULTKEY; @@ -216,6 +218,12 @@ class WalletBatch bool ReadCoinJoinSalt(uint256& salt, bool fLegacy = false); bool WriteCoinJoinSalt(const uint256& salt); + /** Outpoints of successfully mixed inputs which are locked until the transaction + * spending them is observed, mapped to the time each was recorded. Stored as one + * record because the set is small and always rewritten as a whole. */ + bool ReadCoinJoinPendingObs(std::map& pending_obs); + bool WriteCoinJoinPendingObs(const std::map& pending_obs); + /** Write a CGovernanceObject to the database */ bool WriteGovernanceObject(const Governance::Object& obj); From bced3b75a7e4d2b7abebe4522a5eb839d727cd80 Mon Sep 17 00:00:00 2001 From: pasta Date: Sat, 25 Jul 2026 12:11:52 -0500 Subject: [PATCH 2/9] test(coinjoin): cover pending-observation input locking Cover the pending-observation lifecycle: a user's own lock on a denominated coin is never adopted as pending, pending inputs stay locked and excluded from coin selection, the set is mirrored to the wallet database, an observed spend releases the lock, the terminal timeout only fires on a synced chain, an input spent by a mempool transaction the wallet has not recorded yet is kept locked while a genuinely unspent one is released, and a manual user unlock purges the pending entry. Co-Authored-By: Claude Fable 5 --- src/wallet/test/coinjoin_tests.cpp | 119 +++++++++++++++++++++++++++++ 1 file changed, 119 insertions(+) diff --git a/src/wallet/test/coinjoin_tests.cpp b/src/wallet/test/coinjoin_tests.cpp index f9b43818c505..e59cabee2934 100644 --- a/src/wallet/test/coinjoin_tests.cpp +++ b/src/wallet/test/coinjoin_tests.cpp @@ -3,6 +3,7 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include +#include #include #include @@ -11,14 +12,18 @@ #include #include #include +#include #include #include #include #include +#include #include +#include #include #include #include +#include #include @@ -224,6 +229,120 @@ class CTransactionBuilderTestSetup : public TestChain100Setup } }; +BOOST_FIXTURE_TEST_CASE(coinjoin_pending_observation_tests, CTransactionBuilderTestSetup) +{ + // 0.100001 DASH, a valid CoinJoin denomination + constexpr CAmount nDenomAmount{10000100}; + BOOST_REQUIRE(CoinJoin::IsDenominatedAmount(nDenomAmount)); + CompactTallyItem tallyItem = GetTallyItem({nDenomAmount, nDenomAmount, nDenomAmount, nDenomAmount}); + const COutPoint outpointUserLocked = tallyItem.outpoints[0]; + const COutPoint outpointPending = tallyItem.outpoints[1]; + const COutPoint outpointTimeout = tallyItem.outpoints[2]; + const COutPoint outpointInMempool = tallyItem.outpoints[3]; + + // A denominated coin the user locked themselves, e.g. via `lockunspent` + WITH_LOCK(wallet->cs_wallet, wallet->LockCoin(outpointUserLocked)); + + BOOST_CHECK(m_node.cj_walletman->doForClient("", [&](CCoinJoinClientManager& cj_man) { + // A user-created lock is never adopted as a pending observation: it has no + // CoinJoin record backing it, so it is left strictly alone + cj_man.CheckPendingObservations(*m_node.mempool); + BOOST_CHECK(!cj_man.IsPendingObservation(outpointUserLocked)); + BOOST_CHECK_EQUAL(cj_man.GetPendingObservationCount(), 0); + + const int64_t nStart{GetTime()}; + SetMockTime(nStart); + cj_man.AddPendingObservation({outpointPending}); + BOOST_CHECK(cj_man.IsPendingObservation(outpointPending)); + BOOST_CHECK_EQUAL(cj_man.GetPendingObservationCount(), 1); + BOOST_CHECK(WITH_LOCK(wallet->cs_wallet, return wallet->IsLockedCoin(outpointPending))); + + // Pending inputs are excluded from coin selection while unrelated inputs are not + { + LOCK(wallet->cs_wallet); + bool fFoundPending{false}; + bool fFoundFree{false}; + for (const auto& out : AvailableCoinsListUnspent(*wallet).all()) { + fFoundPending |= out.outpoint == outpointPending; + fFoundFree |= out.outpoint == outpointTimeout; + } + BOOST_CHECK(!fFoundPending); + BOOST_CHECK(fFoundFree); + } + + // The pending set is mirrored to the wallet database so it survives a restart + { + std::map persisted; + WalletBatch batch(wallet->GetDatabase()); + BOOST_REQUIRE(batch.ReadCoinJoinPendingObs(persisted)); + BOOST_CHECK_EQUAL(persisted.size(), 1); + BOOST_CHECK(persisted.count(outpointPending) > 0); + BOOST_CHECK_EQUAL(persisted.at(outpointPending), nStart); + } + + // Nothing is released while the inputs remain unspent and the timeout has not passed + cj_man.CheckPendingObservations(*m_node.mempool); + BOOST_CHECK_EQUAL(cj_man.GetPendingObservationCount(), 1); + + // Once the wallet observes a transaction spending a pending input its lock is dropped + CMutableTransaction mtxSpend; + mtxSpend.vin.emplace_back(outpointPending); + mtxSpend.vout.emplace_back(nDenomAmount - 1000, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); + BOOST_REQUIRE(wallet->AddToWallet(MakeTransactionRef(mtxSpend), TxStateInMempool{})); + cj_man.CheckPendingObservations(*m_node.mempool); + BOOST_CHECK(!cj_man.IsPendingObservation(outpointPending)); + BOOST_CHECK(!WITH_LOCK(wallet->cs_wallet, return wallet->IsLockedCoin(outpointPending))); + BOOST_CHECK_EQUAL(cj_man.GetPendingObservationCount(), 0); + + // An input whose spending transaction sits in the mempool but has not reached the + // wallet yet must NOT be released: findCoins() reports it as unspent (it only + // knows outputs mempool transactions create, not the ones they spend), so the + // mempool has to be consulted separately + CMutableTransaction mtxMempool; + mtxMempool.vin.emplace_back(outpointInMempool); + mtxMempool.vout.emplace_back(nDenomAmount - 1000, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); + { + LOCK2(::cs_main, m_node.mempool->cs); + m_node.mempool->addUnchecked(TestMemPoolEntryHelper().FromTx(MakeTransactionRef(mtxMempool))); + } + BOOST_REQUIRE(m_node.mempool->isSpent(outpointInMempool)); + BOOST_REQUIRE(WITH_LOCK(wallet->cs_wallet, return wallet->GetWalletTx(mtxMempool.GetHash())) == nullptr); + + cj_man.AddPendingObservation({outpointTimeout, outpointInMempool}); + BOOST_CHECK_EQUAL(cj_man.GetPendingObservationCount(), 2); + SetMockTime(nStart + CCoinJoinClientManager::PENDING_OBSERVATION_TIMEOUT_SECONDS + 1); + + // The timeout never fires while the chain is still catching up: the spending + // transaction could be sitting in a block we have not downloaded yet + BOOST_REQUIRE(!m_node.mn_sync->IsBlockchainSynced()); + cj_man.CheckPendingObservations(*m_node.mempool); + BOOST_CHECK_EQUAL(cj_man.GetPendingObservationCount(), 2); + BOOST_CHECK(WITH_LOCK(wallet->cs_wallet, return wallet->IsLockedCoin(outpointTimeout))); + + m_node.mn_sync->SwitchToNextAsset(); + BOOST_REQUIRE(m_node.mn_sync->IsBlockchainSynced()); + cj_man.CheckPendingObservations(*m_node.mempool); + + // Unspent everywhere - released (with a warning) after the terminal timeout + BOOST_CHECK(!cj_man.IsPendingObservation(outpointTimeout)); + BOOST_CHECK(!WITH_LOCK(wallet->cs_wallet, return wallet->IsLockedCoin(outpointTimeout))); + // Spent by an unconfirmed transaction the wallet has not recorded - kept locked + BOOST_CHECK(cj_man.IsPendingObservation(outpointInMempool)); + BOOST_CHECK(WITH_LOCK(wallet->cs_wallet, return wallet->IsLockedCoin(outpointInMempool))); + BOOST_CHECK_EQUAL(cj_man.GetPendingObservationCount(), 1); + + // A manual unlock (e.g. via lockunspent) purges the pending entry + WITH_LOCK(wallet->cs_wallet, wallet->UnlockCoin(outpointInMempool)); + cj_man.CheckPendingObservations(*m_node.mempool); + BOOST_CHECK(!cj_man.IsPendingObservation(outpointInMempool)); + BOOST_CHECK_EQUAL(cj_man.GetPendingObservationCount(), 0); + + // The user's own lock was never touched throughout + BOOST_CHECK(WITH_LOCK(wallet->cs_wallet, return wallet->IsLockedCoin(outpointUserLocked))); + SetMockTime(0); + })); +} + BOOST_FIXTURE_TEST_CASE(coinjoin_manager_start_stop_tests, CTransactionBuilderTestSetup) { BOOST_CHECK(m_node.cj_walletman->doForClient("", [](auto& cj_man) { From f9c4bebea16e4a91a45bbda63917c0da78e9ec77 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Sat, 25 Jul 2026 22:45:03 +0300 Subject: [PATCH 3/9] fix(coinjoin): make pending-observation locks crash-safe and cheap to poll Review follow-ups to the pending-observation mechanism: - AddPendingObservation() persisted the wallet locks before the cj_pending_obs record which owns them. Those are two separate writes, so a crash in between left persistently locked coins behind with nothing tracking them and no timeout to release them. Write the record first and only persist the locks if that succeeded; the resulting order is self-healing, CheckPendingObservations() drops a pending entry as soon as it sees its coin is not locked. - CheckPendingObservations() constructed a WalletBatch on every pass, and every WalletBatch checkpoints the wallet database when it goes out of scope. Open it lazily, on the first actual write, and read the record with fFlushOnClose=false. - Do not persist the timer refresh applied to an input which is spent in chain/mempool but not according to the wallet. Such an entry can be terminal, and rewriting the record for it buys nothing: the worst a restart can do is re-run the check once. Log it so it is diagnosable. - Schedule the check on its own rather than running it from DoMaintenance ahead of the mixing gates. It keeps the same property - pending inputs unlock even while mixing or CoinJoin itself is disabled - with one cadence for both relay and block-only mode instead of once per second in one and once per minute in the other. Releasing these locks is pure bookkeeping: the outpoints are the ones the finalized mixing transaction spends, so by the time the wallet observes the spend they could not be selected again anyway. - Move the timeout next to the other CoinJoin timeouts in coinjoin.h and restore the alphabetical include order. - Mark the wrapped log format strings with /* Continued */ so lint-logs.py is satisfied, matching init.cpp and net_processing.cpp. It only inspects the first line of a log call and requires it to end with "\n". Co-Authored-By: Claude Opus 5 --- src/coinjoin/client.cpp | 74 +++++++++++++++++++++++++------------- src/coinjoin/client.h | 6 +--- src/coinjoin/coinjoin.h | 4 +++ src/coinjoin/walletman.cpp | 23 ++++++------ 4 files changed, 67 insertions(+), 40 deletions(-) diff --git a/src/coinjoin/client.cpp b/src/coinjoin/client.cpp index 81572ec4d3c2..f4c8a637e05d 100644 --- a/src/coinjoin/client.cpp +++ b/src/coinjoin/client.cpp @@ -17,9 +17,9 @@ #include #include #include -#include #include #include +#include #include #include #include @@ -33,6 +33,7 @@ #include #include +#include #include #include @@ -633,22 +634,32 @@ void CCoinJoinClientManager::AddPendingObservation(const std::vector& LoadPendingObservations(batch); const int64_t nNow{GetTime()}; - bool fPersisted{true}; for (const auto& outpoint : outpoints) { - // Persist the lock so that a restart before the finalized transaction is - // observed cannot make the input available for selection again - fPersisted &= m_wallet->LockCoin(outpoint, &batch); m_pending_obs.emplace(outpoint, nNow); + } + + // NOTE: the record which owns these locks has to be written BEFORE the locks + // themselves. These are two separate writes and a crash in between must not leave + // persistently locked coins behind with nothing tracking them: nothing would ever + // release them again. The opposite order is self-healing, CheckPendingObservations() + // drops a pending entry as soon as it sees its coin is not locked. + bool fPersisted{batch.WriteCoinJoinPendingObs(m_pending_obs)}; + for (const auto& outpoint : outpoints) { + // The coins are locked in memory already (see PrepareDenominate), this only + // persists the lock so that a restart before the finalized transaction is + // observed cannot make the input available for selection again. Skip persisting + // it if the record above could not be written though, a persistent lock with + // nothing left to release it strands the input. + if (!m_wallet->LockCoin(outpoint, fPersisted ? &batch : nullptr)) fPersisted = false; WalletCJLogPrint(m_wallet, "CCoinJoinClientManager::%s -- %s is locked until the finalized mixing transaction is observed\n", __func__, outpoint.ToStringShort()); } - fPersisted &= batch.WriteCoinJoinPendingObs(m_pending_obs); if (!fPersisted) { // The in-memory lock still protects these inputs for as long as this process // runs, but a restart would make them selectable again while a valid mixing // transaction spending them may already be in flight. Nothing we can do about // it here beyond making the failure loud - the wallet database is broken. - LogPrintf("CCoinJoinClientManager::%s -- ERROR: failed to persist locks for %d successfully mixed input(s), " + LogPrintf("CCoinJoinClientManager::%s -- ERROR: failed to persist locks for %d successfully mixed input(s), " /* Continued */ "they will not survive a restart\n", __func__, outpoints.size()); } @@ -676,8 +687,20 @@ void CCoinJoinClientManager::CheckPendingObservations(const CTxMemPool& mempool) LOCK(m_wallet->cs_wallet); LOCK(cs_pending_obs); - wallet::WalletBatch batch(m_wallet->GetDatabase()); - LoadPendingObservations(batch); + if (!m_pending_obs_loaded) { + // Read-only, no need to checkpoint the database on the way out + wallet::WalletBatch batch_load(m_wallet->GetDatabase(), /*_fFlushOnClose=*/false); + LoadPendingObservations(batch_load); + } + + // Constructed on the first write only: this keeps running for as long as anything is + // pending and every WalletBatch checkpoints the wallet database when it goes out of + // scope, which is far too expensive to pay for a pass which changes nothing. + std::optional batch; + const auto get_batch = [&]() -> wallet::WalletBatch& { + if (!batch.has_value()) batch.emplace(m_wallet->GetDatabase()); + return batch.value(); + }; const int64_t nNow{GetTime()}; const bool fSynced{m_mn_sync.IsBlockchainSynced()}; @@ -695,7 +718,7 @@ void CCoinJoinClientManager::CheckPendingObservations(const CTxMemPool& mempool) // selectable anyway, drop the protective lock WalletCJLogPrint(m_wallet, "CCoinJoinClientManager::%s -- observed spend of %s, releasing lock\n", __func__, outpoint.ToStringShort()); - m_wallet->UnlockCoin(outpoint, &batch); + m_wallet->UnlockCoin(outpoint, &get_batch()); it = m_pending_obs.erase(it); fChanged = true; continue; @@ -703,7 +726,7 @@ void CCoinJoinClientManager::CheckPendingObservations(const CTxMemPool& mempool) // Only ever consult chain/mempool spentness on a synced chain: while catching up // the spending transaction may sit in a block we have not downloaded yet, and // releasing the input on the strength of that would defeat the whole point - if (fSynced && nNow - it->second >= PENDING_OBSERVATION_TIMEOUT_SECONDS) { + if (fSynced && nNow - it->second >= COINJOIN_PENDING_OBSERVATION_TIMEOUT) { // NOTE: findCoins() reports outputs which exist in the chain UTXO set or are // created by a mempool transaction; it does NOT know whether a mempool // transaction spends them (CCoinsViewMemPool::GetCoin never looks at @@ -716,22 +739,30 @@ void CCoinJoinClientManager::CheckPendingObservations(const CTxMemPool& mempool) // Still unspent in chain and mempool long after the session completed - // the finalized transaction most likely never propagated, release the // input so the wallet does not lose it forever - LogPrintf("CCoinJoinClientManager::%s -- WARNING: never observed finalized mixing transaction for %s, " + LogPrintf("CCoinJoinClientManager::%s -- WARNING: never observed finalized mixing transaction for %s, " /* Continued */ "releasing lock after %d seconds\n", - __func__, outpoint.ToStringShort(), PENDING_OBSERVATION_TIMEOUT_SECONDS); - m_wallet->UnlockCoin(outpoint, &batch); + __func__, outpoint.ToStringShort(), COINJOIN_PENDING_OBSERVATION_TIMEOUT); + m_wallet->UnlockCoin(outpoint, &get_batch()); it = m_pending_obs.erase(it); fChanged = true; continue; } // Spent according to chain/mempool but the wallet has not recorded the - // spending transaction yet, keep waiting for it + // spending transaction yet, keep waiting for it. Note that this can be + // terminal: if the wallet never learns about the spend (restored from an old + // backup and never rescanned, say) the entry stays for good. Releasing it + // would be wrong - such a coin still looks unspent to the wallet and could be + // selected for another session - so only refresh the timer to keep the check + // above from running on every pass. The refresh is deliberately not persisted, + // the worst a restart can do is re-run the check once. + WalletCJLogPrint(m_wallet, "CCoinJoinClientManager::%s -- %s is spent in chain/mempool but not according to " /* Continued */ + "the wallet, keeping it locked\n", + __func__, outpoint.ToStringShort()); it->second = nNow; - fChanged = true; } ++it; } - if (fChanged && !batch.WriteCoinJoinPendingObs(m_pending_obs)) { + if (fChanged && !get_batch().WriteCoinJoinPendingObs(m_pending_obs)) { LogPrintf("CCoinJoinClientManager::%s -- ERROR: failed to persist %d pending observation(s)\n", __func__, m_pending_obs.size()); } @@ -1873,16 +1904,9 @@ void CCoinJoinClientManager::UpdatedBlockTip(const CBlockIndex* pindex) void CCoinJoinClientManager::DoMaintenance(ChainstateManager& chainman, CConnman& connman, const CTxMemPool& mempool) { - if (ShutdownRequested()) return; - - // Run this before the mixing gates below: inputs of already completed sessions - // must be able to unlock even when mixing or CoinJoin itself is disabled, - // otherwise their persisted locks could linger forever - CheckPendingObservations(mempool); - if (!CCoinJoinClientOptions::IsEnabled()) return; - if (!m_mn_sync.IsBlockchainSynced()) return; + if (!m_mn_sync.IsBlockchainSynced() || ShutdownRequested()) return; static int nTick = 0; static int nDoAutoNextRun = nTick + COINJOIN_AUTO_TIMEOUT_MIN; diff --git a/src/coinjoin/client.h b/src/coinjoin/client.h index 7d88f3acf12c..485f8f67b698 100644 --- a/src/coinjoin/client.h +++ b/src/coinjoin/client.h @@ -231,10 +231,6 @@ class CCoinJoinClientManager : public interfaces::CoinJoin::Client void UpdatedSuccessBlock(); - //! How long to keep waiting for the finalized mixing transaction before - //! double-checking chain/mempool spentness and potentially releasing the inputs - static constexpr int64_t PENDING_OBSERVATION_TIMEOUT_SECONDS{60 * 60}; - /// Keep the given successfully mixed inputs locked (persistently) until the wallet /// observes a transaction spending them void AddPendingObservation(const std::vector& outpoints) EXCLUSIVE_LOCKS_REQUIRED(!cs_pending_obs); @@ -246,7 +242,7 @@ class CCoinJoinClientManager : public interfaces::CoinJoin::Client void UpdatedBlockTip(const CBlockIndex* pindex); void DoMaintenance(ChainstateManager& chainman, CConnman& connman, const CTxMemPool& mempool) - EXCLUSIVE_LOCKS_REQUIRED(!cs_deqsessions, !cs_pending_obs); + EXCLUSIVE_LOCKS_REQUIRED(!cs_deqsessions); // interfaces::CoinJoin::Client overrides void disableAutobackups() override { fCreateAutoBackups = false; } diff --git a/src/coinjoin/coinjoin.h b/src/coinjoin/coinjoin.h index d01b9539e098..20662233091e 100644 --- a/src/coinjoin/coinjoin.h +++ b/src/coinjoin/coinjoin.h @@ -47,6 +47,10 @@ static constexpr int COINJOIN_AUTO_TIMEOUT_MIN = 5; static constexpr int COINJOIN_AUTO_TIMEOUT_MAX = 15; static constexpr int COINJOIN_QUEUE_TIMEOUT = 30; static constexpr int COINJOIN_SIGNING_TIMEOUT = 15; +//! How long a successfully mixed input is kept locked while waiting for the finalized +//! mixing transaction before chain/mempool spentness is double-checked and the input is +//! potentially released, in seconds +static constexpr int64_t COINJOIN_PENDING_OBSERVATION_TIMEOUT = 60 * 60; static constexpr size_t COINJOIN_ENTRY_MAX_SIZE = 9; diff --git a/src/coinjoin/walletman.cpp b/src/coinjoin/walletman.cpp index dab047af7a7b..a4cf2881cc6c 100644 --- a/src/coinjoin/walletman.cpp +++ b/src/coinjoin/walletman.cpp @@ -120,16 +120,19 @@ CJWalletManagerImpl::~CJWalletManagerImpl() void CJWalletManagerImpl::Schedule(CConnman& connman, CScheduler& scheduler) { - if (!m_relay_txes) { - // Mixing needs transaction relay, so no CoinJoin activity is scheduled here. - // Inputs of a session which completed before an earlier shutdown are a different - // matter: their locks were persisted and are restored with the wallet, so the - // check which releases them once their spend is observed (or once they time out) - // has to keep running even in block-only mode - nothing else would ever unlock them. - scheduler.scheduleEvery(std::bind(&CJWalletManagerImpl::CheckPendingObservations, this), - std::chrono::minutes{1}); - return; - } + // Inputs of a successfully completed session stay locked until the finalized mixing + // transaction is observed and those locks are persisted, so they are restored with + // the wallet. Releasing them has to keep running independently of mixing itself: + // even with CoinJoin disabled, mixing stopped or transaction relay unavailable + // (block-only mode, where nothing below is scheduled at all) nothing else would ever + // unlock them. NOTE: no CJWalletManager exists on a masternode (see init.cpp), a + // wallet with pending observations opened there keeps its inputs locked until it is + // opened on a regular node again or the user unlocks them via `lockunspent`. + scheduler.scheduleEvery(std::bind(&CJWalletManagerImpl::CheckPendingObservations, this), std::chrono::minutes{1}); + + // Mixing needs transaction relay + if (!m_relay_txes) return; + scheduler.scheduleEvery(std::bind(&CJWalletManagerImpl::DoMaintenance, this, std::ref(connman)), std::chrono::seconds{1}); } From 490f54cb96f0ca5fd32c04cc55ba26fa569eddae Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Sat, 25 Jul 2026 22:45:36 +0300 Subject: [PATCH 4/9] test(coinjoin): cover pending-observation reload, fix test-order dependence coinjoin_pending_observation_reload_tests drops the client manager and creates a fresh one for the same wallet, the way a restart would, and checks that the pending entry is read back from the wallet database along with its grace period: the terminal timeout has to be measured from when the session completed, not from when the manager was recreated. Without the record surviving, the persisted lock would be left with nothing to release it. Also pin minRelayTxFee in the fixture, which fixes the win64 CI failure on coinjoin_pending_observation_tests: coinjoin_tests.cpp(208): fatal error: critical check res has failed minRelayTxFee is a global. transaction_tests raises it and then restores DUST_RELAY_TX_FEE rather than DEFAULT_MIN_RELAY_TX_FEE, so it stays at 3 duff/B for the rest of the process. GetTallyItem() asks for CFeeRate(1000), which is below that, and every CreateTransaction() call in the fixture then fails with Fee rate (1.000 duff/B) is lower than the minimum fee rate setting (3.000 duff/B) Boost shuffles test order in CI, which is why this only shows up in some runs and never in declaration order locally. It reproduces reliably with test_dash --run_test=transaction_tests,coinjoin_tests CTransactionBuilderTest and wallet_tests already defend against this by resetting the global themselves; do it in the fixture constructor instead so every test using it is covered rather than each one having to remember. GetTallyItem() also asserted CreateTransaction() succeeded with a bare BOOST_REQUIRE, reporting only "critical check res has failed" with no indication of why - which is what made the above take a while to pin down. Report the error string and the amount instead. rpc_coinjoin.py asserts the new getcoinjoininfo pending_inputs field. Co-Authored-By: Claude Opus 5 --- src/wallet/test/coinjoin_tests.cpp | 57 ++++++++++++++++++++++++++++-- test/functional/rpc_coinjoin.py | 2 ++ 2 files changed, 57 insertions(+), 2 deletions(-) diff --git a/src/wallet/test/coinjoin_tests.cpp b/src/wallet/test/coinjoin_tests.cpp index e59cabee2934..911ec1669891 100644 --- a/src/wallet/test/coinjoin_tests.cpp +++ b/src/wallet/test/coinjoin_tests.cpp @@ -144,6 +144,12 @@ class CTransactionBuilderTestSetup : public TestChain100Setup CTransactionBuilderTestSetup() : wallet{std::make_unique(m_node.chain.get(), m_node.coinjoin_loader.get(), "", m_args, CreateMockWalletDatabase())} { + // NOTE: minRelayTxFee is a global other suites mutate without restoring the + // default (transaction_tests leaves it at DUST_RELAY_TX_FEE), which makes the + // feerate GetTallyItem() asks for lower than the minimum and fails every + // CreateTransaction() call below. Boost shuffles test order in CI, so pin it + // here for every test using this fixture instead of per test. + minRelayTxFee = CFeeRate(DEFAULT_MIN_RELAY_TX_FEE); context.args = &m_args; context.chain = m_node.chain.get(); context.coinjoin_loader = m_node.coinjoin_loader.get(); @@ -205,7 +211,9 @@ class CTransactionBuilderTestSetup : public TestChain100Setup CTransactionRef tx; { auto res = CreateTransaction(*wallet, {{GetScriptForDestination(tallyItem.txdest), nAmount, false}}, nChangePosRet, coinControl); - BOOST_REQUIRE(res); + // Report why it failed, a bare "critical check res has failed" says nothing + BOOST_REQUIRE_MESSAGE(res, strprintf("CreateTransaction(%d) failed: %s", nAmount, + util::ErrorString(res).original)); tx = res->tx; nChangePosRet = res->change_pos; } @@ -310,7 +318,7 @@ BOOST_FIXTURE_TEST_CASE(coinjoin_pending_observation_tests, CTransactionBuilderT cj_man.AddPendingObservation({outpointTimeout, outpointInMempool}); BOOST_CHECK_EQUAL(cj_man.GetPendingObservationCount(), 2); - SetMockTime(nStart + CCoinJoinClientManager::PENDING_OBSERVATION_TIMEOUT_SECONDS + 1); + SetMockTime(nStart + COINJOIN_PENDING_OBSERVATION_TIMEOUT + 1); // The timeout never fires while the chain is still catching up: the spending // transaction could be sitting in a block we have not downloaded yet @@ -343,6 +351,51 @@ BOOST_FIXTURE_TEST_CASE(coinjoin_pending_observation_tests, CTransactionBuilderT })); } +BOOST_FIXTURE_TEST_CASE(coinjoin_pending_observation_reload_tests, CTransactionBuilderTestSetup) +{ + // 0.100001 DASH, a valid CoinJoin denomination + constexpr CAmount nDenomAmount{10000100}; + CompactTallyItem tallyItem = GetTallyItem({nDenomAmount}); + const COutPoint outpointPending = tallyItem.outpoints[0]; + + const int64_t nStart{GetTime()}; + SetMockTime(nStart); + BOOST_CHECK(m_node.cj_walletman->doForClient("", [&](CCoinJoinClientManager& cj_man) { + cj_man.AddPendingObservation({outpointPending}); + BOOST_CHECK(cj_man.IsPendingObservation(outpointPending)); + })); + BOOST_CHECK(WITH_LOCK(wallet->cs_wallet, return wallet->IsLockedCoin(outpointPending))); + + // Drop the client manager and create a fresh one for the same wallet: it has to pick + // the pending observation back up from the wallet database, the way it would after a + // restart, otherwise the persisted lock would be left with nothing to release it + m_node.cj_walletman->removeWallet(wallet->GetName()); + m_node.cj_walletman->addWallet(wallet); + + BOOST_CHECK(m_node.cj_walletman->doForClient("", [&](CCoinJoinClientManager& cj_man) { + // The record is read lazily, on the first check + BOOST_CHECK_EQUAL(cj_man.GetPendingObservationCount(), 0); + cj_man.CheckPendingObservations(*m_node.mempool); + BOOST_CHECK(cj_man.IsPendingObservation(outpointPending)); + BOOST_CHECK_EQUAL(cj_man.GetPendingObservationCount(), 1); + BOOST_CHECK(WITH_LOCK(wallet->cs_wallet, return wallet->IsLockedCoin(outpointPending))); + + m_node.mn_sync->SwitchToNextAsset(); + BOOST_REQUIRE(m_node.mn_sync->IsBlockchainSynced()); + + // The grace period was restored along with the entry rather than restarted: the + // terminal timeout is measured from when the session completed, not from reload + SetMockTime(nStart + COINJOIN_PENDING_OBSERVATION_TIMEOUT - 1); + cj_man.CheckPendingObservations(*m_node.mempool); + BOOST_CHECK(cj_man.IsPendingObservation(outpointPending)); + SetMockTime(nStart + COINJOIN_PENDING_OBSERVATION_TIMEOUT + 1); + cj_man.CheckPendingObservations(*m_node.mempool); + BOOST_CHECK(!cj_man.IsPendingObservation(outpointPending)); + BOOST_CHECK(!WITH_LOCK(wallet->cs_wallet, return wallet->IsLockedCoin(outpointPending))); + })); + SetMockTime(0); +} + BOOST_FIXTURE_TEST_CASE(coinjoin_manager_start_stop_tests, CTransactionBuilderTestSetup) { BOOST_CHECK(m_node.cj_walletman->doForClient("", [](auto& cj_man) { diff --git a/test/functional/rpc_coinjoin.py b/test/functional/rpc_coinjoin.py index 409af623ec8c..a2ee1a5fab83 100755 --- a/test/functional/rpc_coinjoin.py +++ b/test/functional/rpc_coinjoin.py @@ -84,6 +84,8 @@ def test_coinjoin_start_stop(self, node): cj_info = node.getcoinjoininfo() assert_equal(cj_info['enabled'], True) assert_equal(cj_info['running'], True) + # No session has completed, so nothing is waiting to be observed + assert_equal(cj_info['pending_inputs'], 0) # Repeated start should yield error assert_raises_rpc_error(-32603, 'Mixing has been started already.', node.coinjoin, 'start') # Requesting status shouldn't complain From 22608a9ff4e51df1d9b862a545cbdb1432004a2d Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Sat, 25 Jul 2026 22:45:36 +0300 Subject: [PATCH 5/9] doc: add release notes for the CoinJoin pending-observation locks Covers the new getcoinjoininfo pending_inputs field, the behavioral change to when mixed inputs are released, and the new cj_pending_obs wallet database record. Co-Authored-By: Claude Opus 5 --- doc/release-notes-7480.md | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 doc/release-notes-7480.md diff --git a/doc/release-notes-7480.md b/doc/release-notes-7480.md new file mode 100644 index 000000000000..ec2892275529 --- /dev/null +++ b/doc/release-notes-7480.md @@ -0,0 +1,28 @@ +Updated RPCs +------------ + +* `getcoinjoininfo` gained a `pending_inputs` field, reporting how many + successfully mixed inputs are currently kept locked while waiting for the + finalized mixing transaction spending them to be observed. + +CoinJoin +-------- + +* After a mixing session completes successfully, the inputs a client + contributed to it are no longer released as soon as the `DSCOMPLETE` message + is processed. They stay locked until the wallet observes a transaction + spending them, or for at most an hour if that transaction never propagates. + Previously the completion message routinely overtook the trickle-relayed + mixing transaction, leaving a window in which another session (with + `-coinjoinmultisession=1`, where the one-block cooldown does not apply) could + select and sign the very same inputs and produce a second, conflicting + CoinJoin transaction. + + These locks are persisted, so they survive a restart. They are tracked in a + new wallet database record, `cj_pending_obs`, which is written only when a + mixing session completes successfully. Older clients loading such a wallet + ignore the record, counting it as an unknown record; the accompanying + `lockedutxo` entries are understood by them regardless, so the inputs stay + locked either way. Locks the user set themselves via `lockunspent` are never + adopted or released by this mechanism, and unlocking a pending input manually + purges it from the pending set. From 71c8dde8a15bcc918ffaffb60c813291ca190d0f Mon Sep 17 00:00:00 2001 From: pasta Date: Tue, 28 Jul 2026 19:32:09 -0500 Subject: [PATCH 6/9] fix(coinjoin): commit pending-observation record and locks atomically WalletBatch writes are each their own implicit transaction, so a crash between writing cj_pending_obs and the lockedutxo records could persist the pending record without its locks. On restart the missing lock reads as a manual unlock and CheckPendingObservations drops the entry, making the input selectable again while the finalized mixing transaction may still be in flight - exactly the window these locks exist to close. Wrap the record and lock writes in one explicit database transaction, aborting it as a whole if any write fails. --- src/coinjoin/client.cpp | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/src/coinjoin/client.cpp b/src/coinjoin/client.cpp index f4c8a637e05d..30c5c387855f 100644 --- a/src/coinjoin/client.cpp +++ b/src/coinjoin/client.cpp @@ -638,22 +638,31 @@ void CCoinJoinClientManager::AddPendingObservation(const std::vector& m_pending_obs.emplace(outpoint, nNow); } - // NOTE: the record which owns these locks has to be written BEFORE the locks - // themselves. These are two separate writes and a crash in between must not leave - // persistently locked coins behind with nothing tracking them: nothing would ever - // release them again. The opposite order is self-healing, CheckPendingObservations() - // drops a pending entry as soon as it sees its coin is not locked. - bool fPersisted{batch.WriteCoinJoinPendingObs(m_pending_obs)}; + // NOTE: the record which owns the locks and the persistent locks themselves are + // separate writes and each write is its own implicit transaction, so commit them in + // one explicit database transaction: a crash in between must neither leave + // persistently locked coins behind with nothing tracking them (nothing would ever + // release them again) nor persist the record without its locks (on restart the + // missing lock reads as a manual unlock, the entry is dropped and the input becomes + // selectable again while the finalized mixing transaction may still be in flight). + // If no transaction can be started, don't persist anything rather than risk exactly + // those partial states. + const bool fTxn{batch.TxnBegin()}; + bool fPersisted{fTxn && batch.WriteCoinJoinPendingObs(m_pending_obs)}; for (const auto& outpoint : outpoints) { // The coins are locked in memory already (see PrepareDenominate), this only // persists the lock so that a restart before the finalized transaction is - // observed cannot make the input available for selection again. Skip persisting - // it if the record above could not be written though, a persistent lock with - // nothing left to release it strands the input. + // observed cannot make the input available for selection again. Stop writing + // once anything failed, the transaction is aborted as a whole below. if (!m_wallet->LockCoin(outpoint, fPersisted ? &batch : nullptr)) fPersisted = false; WalletCJLogPrint(m_wallet, "CCoinJoinClientManager::%s -- %s is locked until the finalized mixing transaction is observed\n", __func__, outpoint.ToStringShort()); } + if (fPersisted) { + fPersisted = batch.TxnCommit(); + } else if (fTxn) { + batch.TxnAbort(); + } if (!fPersisted) { // The in-memory lock still protects these inputs for as long as this process // runs, but a restart would make them selectable again while a valid mixing From 66957267bb1e2b6938822124225fbc4866e2ef86 Mon Sep 17 00:00:00 2001 From: pasta Date: Tue, 28 Jul 2026 22:23:05 -0500 Subject: [PATCH 7/9] fix(coinjoin): don't treat an unreadable pending-observation record as empty LoadPendingObservations marked the state loaded before checking the read result, so a transient read or deserialization failure of an existing cj_pending_obs record was indistinguishable from an absent one: the persisted locks it tracks would be left with nothing to ever release them, and a later AddPendingObservation or CheckPendingObservations write would overwrite the record, orphaning those locks for good. Tell the two cases apart via DatabaseBatch::Exists. An absent record still short-circuits as before; a failed read of an existing record is now retried on the next pass, reads into a scratch map so partially deserialized entries cannot leak, and blocks every record write until the contents have been recovered. --- src/coinjoin/client.cpp | 33 ++++++++++++++++++++++++++++----- src/wallet/walletdb.cpp | 5 +++++ src/wallet/walletdb.h | 4 +++- 3 files changed, 36 insertions(+), 6 deletions(-) diff --git a/src/coinjoin/client.cpp b/src/coinjoin/client.cpp index 30c5c387855f..1101a78a0d1d 100644 --- a/src/coinjoin/client.cpp +++ b/src/coinjoin/client.cpp @@ -646,8 +646,10 @@ void CCoinJoinClientManager::AddPendingObservation(const std::vector& // missing lock reads as a manual unlock, the entry is dropped and the input becomes // selectable again while the finalized mixing transaction may still be in flight). // If no transaction can be started, don't persist anything rather than risk exactly - // those partial states. - const bool fTxn{batch.TxnBegin()}; + // those partial states. Likewise if the existing record could not be read + // (LoadPendingObservations() left m_pending_obs_loaded unset): overwriting it would + // permanently orphan the locks it still tracks. + const bool fTxn{m_pending_obs_loaded && batch.TxnBegin()}; bool fPersisted{fTxn && batch.WriteCoinJoinPendingObs(m_pending_obs)}; for (const auto& outpoint : outpoints) { // The coins are locked in memory already (see PrepareDenominate), this only @@ -679,9 +681,26 @@ void CCoinJoinClientManager::LoadPendingObservations(wallet::WalletBatch& batch) AssertLockHeld(cs_pending_obs); if (m_pending_obs_loaded) return; - m_pending_obs_loaded = true; // Missing record simply means nothing was pending when we last shut down - if (!batch.ReadCoinJoinPendingObs(m_pending_obs)) return; + if (!batch.HasCoinJoinPendingObs()) { + m_pending_obs_loaded = true; + return; + } + // Read into a scratch map: a failed read of an existing record must not be mistaken + // for an empty one, or the persisted locks it tracks would be left with nothing to + // ever release them, and must not leave partially deserialized entries behind + // either. Not marking the state loaded retries the read on the next pass and, more + // importantly, keeps everything else from overwriting the record before its + // contents have been recovered. + std::map pending; + if (!batch.ReadCoinJoinPendingObs(pending)) { + LogPrintf("CCoinJoinClientManager::%s -- ERROR: failed to read the pending observation record, will retry\n", + __func__); + return; + } + // Keep any entries added in memory while the record was unreadable + m_pending_obs.insert(pending.begin(), pending.end()); + m_pending_obs_loaded = true; WalletCJLogPrint(m_wallet, "CCoinJoinClientManager::%s -- loaded %d pending observation(s)\n", __func__, m_pending_obs.size()); } @@ -771,7 +790,11 @@ void CCoinJoinClientManager::CheckPendingObservations(const CTxMemPool& mempool) } ++it; } - if (fChanged && !get_batch().WriteCoinJoinPendingObs(m_pending_obs)) { + // Never overwrite a record which could not be read (m_pending_obs_loaded unset), + // it may still track locks nothing else would ever release. An entry released + // above but left in such a record self-heals: once the record is readable again + // the entry reloads, its coin is no longer locked and it is dropped right here. + if (fChanged && m_pending_obs_loaded && !get_batch().WriteCoinJoinPendingObs(m_pending_obs)) { LogPrintf("CCoinJoinClientManager::%s -- ERROR: failed to persist %d pending observation(s)\n", __func__, m_pending_obs.size()); } diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index f7cb440e1b4b..030f28a9a20f 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -227,6 +227,11 @@ bool WalletBatch::WriteCoinJoinSalt(const uint256& salt) return WriteIC(DBKeys::COINJOIN_SALT, salt); } +bool WalletBatch::HasCoinJoinPendingObs() +{ + return m_batch->Exists(std::string(DBKeys::COINJOIN_PENDING_OBS)); +} + bool WalletBatch::ReadCoinJoinPendingObs(std::map& pending_obs) { return m_batch->Read(std::string(DBKeys::COINJOIN_PENDING_OBS), pending_obs); diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index a91f3240467b..fac9f7453e02 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -220,7 +220,9 @@ class WalletBatch /** Outpoints of successfully mixed inputs which are locked until the transaction * spending them is observed, mapped to the time each was recorded. Stored as one - * record because the set is small and always rewritten as a whole. */ + * record because the set is small and always rewritten as a whole. Has() tells a + * missing record apart from one Read() could not read or deserialize. */ + bool HasCoinJoinPendingObs(); bool ReadCoinJoinPendingObs(std::map& pending_obs); bool WriteCoinJoinPendingObs(const std::map& pending_obs); From f13e0e022c193b0223ca028c8fa8061fd6086c72 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Sat, 1 Aug 2026 22:44:28 +0300 Subject: [PATCH 8/9] fix(coinjoin): report an unreadable pending-observation record only once A corrupt (as opposed to transiently unreadable) cj_pending_obs record never becomes readable, and LoadPendingObservations() deliberately keeps retrying it for as long as the node runs. Logging the failure on every pass turned that into an ERROR line every minute forever. Report it once instead and point at `lockunspent` as the manual way out, the retry itself stays silent. Also correct the load log, which reported the size of the merged map rather than the number of entries actually read, and spell out what HasCoinJoinPendingObs() can and cannot tell apart: it separates a missing record from one which fails to deserialize, but both HasKey() implementations report a database error as a miss, so an error while looking for the record still reads as "nothing was pending". --- src/coinjoin/client.cpp | 21 +++++++++++++++++---- src/coinjoin/client.h | 3 +++ 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/src/coinjoin/client.cpp b/src/coinjoin/client.cpp index 1101a78a0d1d..1a16f48acadd 100644 --- a/src/coinjoin/client.cpp +++ b/src/coinjoin/client.cpp @@ -681,7 +681,13 @@ void CCoinJoinClientManager::LoadPendingObservations(wallet::WalletBatch& batch) AssertLockHeld(cs_pending_obs); if (m_pending_obs_loaded) return; - // Missing record simply means nothing was pending when we last shut down + // Missing record simply means nothing was pending when we last shut down. + // NOTE: this only tells a missing record apart from one which exists but cannot be + // deserialized, which is the realistic failure. It cannot tell a missing record apart + // from a database error while looking for it: both BerkeleyBatch::HasKey() and + // SQLiteBatch::HasKey() report anything which is not a hit as a miss, so an error + // there still reads as "nothing was pending". Telling those apart would need a + // tri-state DatabaseBatch API for both backends, which is not worth it for this. if (!batch.HasCoinJoinPendingObs()) { m_pending_obs_loaded = true; return; @@ -694,15 +700,22 @@ void CCoinJoinClientManager::LoadPendingObservations(wallet::WalletBatch& batch) // contents have been recovered. std::map pending; if (!batch.ReadCoinJoinPendingObs(pending)) { - LogPrintf("CCoinJoinClientManager::%s -- ERROR: failed to read the pending observation record, will retry\n", - __func__); + // Only complain once: a corrupt (as opposed to transiently unreadable) record + // never becomes readable, and the retry runs for as long as this node does. + if (!m_pending_obs_load_failed) { + m_pending_obs_load_failed = true; + LogPrintf("CCoinJoinClientManager::%s -- ERROR: failed to read the pending observation record, will keep " /* Continued */ + "retrying silently. The inputs it tracks stay locked until it can be read, `lockunspent` " + "releases them manually\n", + __func__); + } return; } // Keep any entries added in memory while the record was unreadable m_pending_obs.insert(pending.begin(), pending.end()); m_pending_obs_loaded = true; WalletCJLogPrint(m_wallet, "CCoinJoinClientManager::%s -- loaded %d pending observation(s)\n", __func__, - m_pending_obs.size()); + pending.size()); } void CCoinJoinClientManager::CheckPendingObservations(const CTxMemPool& mempool) diff --git a/src/coinjoin/client.h b/src/coinjoin/client.h index 485f8f67b698..dd1d5f62593b 100644 --- a/src/coinjoin/client.h +++ b/src/coinjoin/client.h @@ -190,6 +190,9 @@ class CCoinJoinClientManager : public interfaces::CoinJoin::Client //! never confused with locks the user set themselves via `lockunspent`. std::map m_pending_obs GUARDED_BY(cs_pending_obs); bool m_pending_obs_loaded GUARDED_BY(cs_pending_obs){false}; + //! Whether the failure to read the record has been reported already, the read is + //! retried for as long as this node runs and a corrupt record never becomes readable + bool m_pending_obs_load_failed GUARDED_BY(cs_pending_obs){false}; /// Populate m_pending_obs from the wallet database, once per run void LoadPendingObservations(wallet::WalletBatch& batch) EXCLUSIVE_LOCKS_REQUIRED(cs_pending_obs); From 82794fe402d3e9d757095be91520e5390b0a2140 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Sat, 1 Aug 2026 22:44:29 +0300 Subject: [PATCH 9/9] test(coinjoin): cover an unreadable pending-observation record Nothing pinned the invariant that an existing but undeserializable cj_pending_obs record is never mistaken for an absent one. Corrupt the record behind the manager's back and check that the locks it tracks stay in place, that neither AddPendingObservation() nor a releasing CheckPendingObservations() pass overwrites it while its contents are unknown, and that the entry is picked back up (and can finally be released) once the record is readable again. --- src/wallet/test/coinjoin_tests.cpp | 77 ++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/src/wallet/test/coinjoin_tests.cpp b/src/wallet/test/coinjoin_tests.cpp index 911ec1669891..3f9c6ca5a1ad 100644 --- a/src/wallet/test/coinjoin_tests.cpp +++ b/src/wallet/test/coinjoin_tests.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -396,6 +397,82 @@ BOOST_FIXTURE_TEST_CASE(coinjoin_pending_observation_reload_tests, CTransactionB SetMockTime(0); } +BOOST_FIXTURE_TEST_CASE(coinjoin_pending_observation_unreadable_tests, CTransactionBuilderTestSetup) +{ + // 0.100001 DASH, a valid CoinJoin denomination + constexpr CAmount nDenomAmount{10000100}; + CompactTallyItem tallyItem = GetTallyItem({nDenomAmount, nDenomAmount}); + const COutPoint outpointPersisted = tallyItem.outpoints[0]; + const COutPoint outpointInMemory = tallyItem.outpoints[1]; + + const int64_t nStart{GetTime()}; + SetMockTime(nStart); + BOOST_CHECK(m_node.cj_walletman->doForClient("", [&](CCoinJoinClientManager& cj_man) { + cj_man.AddPendingObservation({outpointPersisted}); + })); + BOOST_CHECK(WITH_LOCK(wallet->cs_wallet, return wallet->IsLockedCoin(outpointPersisted))); + + // Corrupt the record behind the manager's back: it exists but cannot be deserialized + // into the pending map anymore. This must not read as "nothing was ever pending", the + // locks it tracks would be left with nothing to ever release them. + BOOST_REQUIRE(wallet->GetDatabase().MakeBatch()->Write(std::string(DBKeys::COINJOIN_PENDING_OBS), + std::string("not a pending observation map"))); + + // A fresh manager for the same wallet, the way a restart would build one + m_node.cj_walletman->removeWallet(wallet->GetName()); + m_node.cj_walletman->addWallet(wallet); + + BOOST_CHECK(m_node.cj_walletman->doForClient("", [&](CCoinJoinClientManager& cj_man) { + // The read fails, so nothing is loaded and the persisted lock is left in place + cj_man.CheckPendingObservations(*m_node.mempool); + BOOST_CHECK_EQUAL(cj_man.GetPendingObservationCount(), 0); + BOOST_CHECK(WITH_LOCK(wallet->cs_wallet, return wallet->IsLockedCoin(outpointPersisted))); + + // Adding a new observation while the record is unreadable must not overwrite it, + // that would orphan the locks it still tracks for good. The new entry is kept in + // memory (and its coin locked) but deliberately not persisted. + cj_man.AddPendingObservation({outpointInMemory}); + BOOST_CHECK(cj_man.IsPendingObservation(outpointInMemory)); + BOOST_CHECK(WITH_LOCK(wallet->cs_wallet, return wallet->IsLockedCoin(outpointInMemory))); + { + std::map persisted; + WalletBatch batch(wallet->GetDatabase()); + BOOST_CHECK(!batch.ReadCoinJoinPendingObs(persisted)); + BOOST_CHECK(persisted.empty()); + } + + // Same for the release path: a pass which changes the pending set must not write + // the record out either while its contents are still unknown + WITH_LOCK(wallet->cs_wallet, wallet->UnlockCoin(outpointInMemory)); + cj_man.CheckPendingObservations(*m_node.mempool); + BOOST_CHECK(!cj_man.IsPendingObservation(outpointInMemory)); + BOOST_CHECK(WITH_LOCK(wallet->cs_wallet, return wallet->IsLockedCoin(outpointPersisted))); + { + std::map persisted; + WalletBatch batch(wallet->GetDatabase()); + BOOST_CHECK(!batch.ReadCoinJoinPendingObs(persisted)); + } + + // Once the record is readable again the entry it tracks is picked back up and the + // lock behind it can finally be released + { + WalletBatch batch(wallet->GetDatabase()); + BOOST_REQUIRE(batch.WriteCoinJoinPendingObs({{outpointPersisted, nStart}})); + } + cj_man.CheckPendingObservations(*m_node.mempool); + BOOST_CHECK(cj_man.IsPendingObservation(outpointPersisted)); + BOOST_CHECK_EQUAL(cj_man.GetPendingObservationCount(), 1); + + m_node.mn_sync->SwitchToNextAsset(); + BOOST_REQUIRE(m_node.mn_sync->IsBlockchainSynced()); + SetMockTime(nStart + COINJOIN_PENDING_OBSERVATION_TIMEOUT + 1); + cj_man.CheckPendingObservations(*m_node.mempool); + BOOST_CHECK(!cj_man.IsPendingObservation(outpointPersisted)); + BOOST_CHECK(!WITH_LOCK(wallet->cs_wallet, return wallet->IsLockedCoin(outpointPersisted))); + })); + SetMockTime(0); +} + BOOST_FIXTURE_TEST_CASE(coinjoin_manager_start_stop_tests, CTransactionBuilderTestSetup) { BOOST_CHECK(m_node.cj_walletman->doForClient("", [](auto& cj_man) {