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. diff --git a/src/coinjoin/client.cpp b/src/coinjoin/client.cpp index fece2711d5c0..1a16f48acadd 100644 --- a/src/coinjoin/client.cpp +++ b/src/coinjoin/client.cpp @@ -14,10 +14,12 @@ #include #include +#include #include #include #include #include +#include #include #include #include @@ -28,8 +30,10 @@ #include #include #include +#include #include +#include #include #include @@ -577,6 +581,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 +623,210 @@ 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()}; + for (const auto& outpoint : outpoints) { + m_pending_obs.emplace(outpoint, nNow); + } + + // 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. 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 + // persists the lock so that a restart before the finalized transaction is + // 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 + // 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), " /* Continued */ + "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; + // 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; + } + // 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)) { + // 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__, + pending.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); + + 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()}; + 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, &get_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 >= 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 + // 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, " /* Continued */ + "releasing lock after %d seconds\n", + __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. 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; + } + ++it; + } + // 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()); + } +} + +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; @@ -1751,6 +1986,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..dd1d5f62593b 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,24 @@ 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}; + //! 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); + // Keep track of current block height int nCachedBlockHeight{0}; @@ -211,6 +234,14 @@ class CCoinJoinClientManager : public interfaces::CoinJoin::Client void UpdatedSuccessBlock(); + /// 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) @@ -219,7 +250,7 @@ class CCoinJoinClientManager : public interfaces::CoinJoin::Client // 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/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 2ae939688a5f..a4cf2881cc6c 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,19 @@ CJWalletManagerImpl::~CJWalletManagerImpl() void CJWalletManagerImpl::Schedule(CConnman& connman, CScheduler& scheduler) { + // 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}); } @@ -205,6 +218,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/test/coinjoin_tests.cpp b/src/wallet/test/coinjoin_tests.cpp index f9b43818c505..3f9c6ca5a1ad 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,19 @@ #include #include #include +#include #include #include #include #include +#include #include +#include #include +#include #include #include +#include #include @@ -139,6 +145,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(); @@ -200,7 +212,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; } @@ -224,6 +238,241 @@ 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 + 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 + 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_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_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) { diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index ea0d80cef37b..030f28a9a20f 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,21 @@ 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); +} + +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 +794,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..fac9f7453e02 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,14 @@ 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. 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); + /** Write a CGovernanceObject to the database */ bool WriteGovernanceObject(const Governance::Object& obj); 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