From 5c44010e74c89c4f5c7f2564dfc805d172546dca Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Fri, 21 Aug 2026 22:47:35 +0300 Subject: [PATCH 1/3] feat(wallet): let a deliberate unlock opt out of the automatic coin locks AutoLockMasternodeCollaterals() and LockExistingDustOutputs() run on every wallet load and lock every masternode collateral and dust-protection target they find. They recompute lock policy, so they cannot tell an outpoint that was never unlocked from one the user unlocked on purpose with `lockunspent`, which is the documented way to spend a protected output. Restarting the node therefore silently took the decision back and the output became unspendable again with no indication why. Give the wallet a way to record that decision instead of inferring it. A deliberate unlock adds the outpoint to m_autolock_optout, persisted as DBKeys::AUTOLOCK_OPTOUT because the automatic locks outlive a restart; locking it again clears the record and hands the outpoint back to them. The two chokepoints that apply those locks, LockProTxCoins() and IsDustProtectionTarget(), skip outpoints carrying the record, which covers every path that reapplies them. Only genuinely user-driven paths record intent: `lockunspent` and the GUI coin-control entry points. interfaces::unlockCoin() was also used to drop the transient hold CollateralLockGuard takes, so acquireCoinLock() gains a matching releaseCoinLock() and the guard uses that, keeping an internal hold from being mistaken for a decision. The record is written only alongside the lock change it belongs to: a lock the caller keeps in memory only leaves the decision standing, an unlock always persists both, and a failed write is rolled back in memory so what the process believes matches what a reload would find. Records whose output the wallet no longer knows about are dropped after a clean load. Co-Authored-By: Claude Opus 5 --- src/evo/providertx_service.cpp | 2 +- src/interfaces/wallet.h | 7 +- src/wallet/interfaces.cpp | 14 ++- src/wallet/rpc/coins.cpp | 4 +- src/wallet/test/availablecoins_tests.cpp | 140 +++++++++++++++++++++- src/wallet/test/wallet_tests.cpp | 103 ++++++++++++++-- src/wallet/test/walletload_tests.cpp | 31 +++++ src/wallet/wallet.cpp | 90 +++++++++++++- src/wallet/wallet.h | 17 +++ src/wallet/walletdb.cpp | 17 +++ src/wallet/walletdb.h | 4 + test/functional/wallet_dust_protection.py | 72 +++++++++++ 12 files changed, 482 insertions(+), 19 deletions(-) diff --git a/src/evo/providertx_service.cpp b/src/evo/providertx_service.cpp index 47961aba49d5..f9790ae63295 100644 --- a/src/evo/providertx_service.cpp +++ b/src/evo/providertx_service.cpp @@ -400,7 +400,7 @@ class CollateralLockGuard ~CollateralLockGuard() { - if (m_result == CoinLockResult::ACQUIRED && !m_keep) m_wallet.unlockCoin(m_outpoint); + if (m_result == CoinLockResult::ACQUIRED && !m_keep) m_wallet.releaseCoinLock(m_outpoint, /*write_to_db=*/false); } bool Succeeded() const { return m_result != CoinLockResult::FAILED; } diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index 6bcde7e75af5..ff07cf07f9db 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -202,13 +202,16 @@ class Wallet //! Display address on external signer virtual bool displayAddress(const CTxDestination& dest) = 0; - //! Lock coin. + //! Lock coin on the user's request, handing it back to the automatic locks. virtual bool lockCoin(const COutPoint& output, const bool write_to_db) = 0; //! Atomically lock a coin only when it is not already locked. virtual CoinLockResult acquireCoinLock(const COutPoint& output, bool write_to_db) = 0; - //! Unlock coin. + //! Release a hold taken with acquireCoinLock(), leaving user intent untouched. + virtual bool releaseCoinLock(const COutPoint& output, bool write_to_db) = 0; + + //! Unlock coin on the user's request, opting it out of the automatic locks. virtual bool unlockCoin(const COutPoint& output) = 0; //! Return whether coin is locked. diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 39f2080d388c..0f63729e7d40 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -362,7 +362,7 @@ class WalletImpl : public Wallet { LOCK(m_wallet->cs_wallet); std::unique_ptr batch = write_to_db ? std::make_unique(m_wallet->GetDatabase()) : nullptr; - return m_wallet->LockCoin(output, batch.get()); + return m_wallet->LockCoinByUser(output, batch.get()); } CoinLockResult acquireCoinLock(const COutPoint& output, bool write_to_db) override { @@ -378,11 +378,17 @@ class WalletImpl : public Wallet } return CoinLockResult::ACQUIRED; } + bool releaseCoinLock(const COutPoint& output, bool write_to_db) override + { + LOCK(m_wallet->cs_wallet); + std::unique_ptr batch = write_to_db ? std::make_unique(m_wallet->GetDatabase()) : nullptr; + return m_wallet->UnlockCoin(output, batch.get()); + } bool unlockCoin(const COutPoint& output) override { LOCK(m_wallet->cs_wallet); std::unique_ptr batch = std::make_unique(m_wallet->GetDatabase()); - return m_wallet->UnlockCoin(output, batch.get()); + return m_wallet->UnlockCoinByUser(output, batch.get()); } bool isLockedCoin(const COutPoint& output) override { @@ -399,7 +405,7 @@ class WalletImpl : public Wallet LOCK(m_wallet->cs_wallet); WalletBatch batch(m_wallet->GetDatabase()); for (const auto& output : outputs) { - if (!m_wallet->LockCoin(output, &batch)) return false; + if (!m_wallet->LockCoinByUser(output, &batch)) return false; } return true; } @@ -408,7 +414,7 @@ class WalletImpl : public Wallet LOCK(m_wallet->cs_wallet); WalletBatch batch(m_wallet->GetDatabase()); for (const auto& output : outputs) { - if (!m_wallet->UnlockCoin(output, &batch)) return false; + if (!m_wallet->UnlockCoinByUser(output, &batch)) return false; } return true; } diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp index 5a060111fb3a..850296b2be56 100644 --- a/src/wallet/rpc/coins.cpp +++ b/src/wallet/rpc/coins.cpp @@ -412,9 +412,9 @@ RPCHelpMan lockunspent() // Atomically set (un)locked status for the outputs. for (const COutPoint& outpt : outputs) { if (fUnlock) { - if (!pwallet->UnlockCoin(outpt, batch.get())) throw JSONRPCError(RPC_WALLET_ERROR, "Unlocking coin failed"); + if (!pwallet->UnlockCoinByUser(outpt, batch.get())) throw JSONRPCError(RPC_WALLET_ERROR, "Unlocking coin failed"); } else { - if (!pwallet->LockCoin(outpt, batch.get())) throw JSONRPCError(RPC_WALLET_ERROR, "Locking coin failed"); + if (!pwallet->LockCoinByUser(outpt, batch.get())) throw JSONRPCError(RPC_WALLET_ERROR, "Locking coin failed"); } } diff --git a/src/wallet/test/availablecoins_tests.cpp b/src/wallet/test/availablecoins_tests.cpp index 0afb81d062e0..ffe2ceb1406e 100644 --- a/src/wallet/test/availablecoins_tests.cpp +++ b/src/wallet/test/availablecoins_tests.cpp @@ -2,6 +2,11 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or https://www.opensource.org/licenses/mit-license.php. +#include +#include +#include +#include +#include #include #include #include @@ -15,7 +20,8 @@ BOOST_FIXTURE_TEST_SUITE(availablecoins_tests, WalletTestingSetup) class AvailableCoinsTestingSetup : public TestChain100Setup { public: - AvailableCoinsTestingSetup() + explicit AvailableCoinsTestingSetup(const std::vector& extra_args = {}) : + TestChain100Setup{CBaseChainParams::REGTEST, extra_args} { CreateAndProcessBlock({}, {}); wallet = CreateSyncedWallet(*m_node.chain, *m_node.coinjoin_loader, *m_node.chainman, m_args, coinbaseKey); @@ -89,5 +95,137 @@ BOOST_FIXTURE_TEST_CASE(BasicOutputTypesTest, AvailableCoinsTestingSetup) BOOST_CHECK_EQUAL(available_coins.legacy.size(), 2U); } +BOOST_FIXTURE_TEST_CASE(DeliberateUnlockSurvivesAutomaticLocking, AvailableCoinsTestingSetup) +{ + LOCK(wallet->cs_wallet); + wallet->m_dust_protection_threshold = 1 * COIN; + + const auto dest{wallet->GetNewDestination("")}; + BOOST_ASSERT(dest); + + // An external transaction pays us a dust-protection target; AddToWallet() locks it. + CMutableTransaction dust_mtx; + dust_mtx.vin.emplace_back(COutPoint{uint256::ONE, 0}); + dust_mtx.vout.emplace_back(COIN / 100, GetScriptForDestination(*dest)); + const CTransactionRef dust_tx{MakeTransactionRef(dust_mtx)}; + const COutPoint dust_outpoint{dust_tx->GetHash(), 0}; + BOOST_CHECK(wallet->AddToWallet(dust_tx, TxStateInMempool{})); + BOOST_CHECK(wallet->IsLockedCoin(dust_outpoint)); + + // The user releases the automatic lock because the spend is really intended. + WalletBatch batch{wallet->GetDatabase()}; + BOOST_CHECK(wallet->UnlockCoinByUser(dust_outpoint, &batch)); + BOOST_CHECK(wallet->IsAutoLockOptOut(dust_outpoint)); + + // A wallet load reapplies the automatic locks, so the decision has to outlive it. + wallet->LockExistingDustOutputs(); + BOOST_CHECK(!wallet->IsLockedCoin(dust_outpoint)); + + // A lock the caller keeps in memory only leaves the decision standing. + BOOST_CHECK(wallet->LockCoinByUser(dust_outpoint, /*batch=*/nullptr)); + BOOST_CHECK(wallet->IsAutoLockOptOut(dust_outpoint)); + BOOST_CHECK(wallet->UnlockCoin(dust_outpoint, &batch)); + + // Locking again persistently hands the outpoint back to the automatic protection. + BOOST_CHECK(wallet->LockCoinByUser(dust_outpoint, &batch)); + BOOST_CHECK(!wallet->IsAutoLockOptOut(dust_outpoint)); + BOOST_CHECK(wallet->UnlockCoin(dust_outpoint, &batch)); + wallet->LockExistingDustOutputs(); + BOOST_CHECK(wallet->IsLockedCoin(dust_outpoint)); +} + +// The shared fixture leaves the chain at height 101, so activate DIP3 just above it +// instead of mining to the regtest default of 432. +struct MasternodeCollateralTestingSetup : public AvailableCoinsTestingSetup { + MasternodeCollateralTestingSetup() : + AvailableCoinsTestingSetup{{"-dip3params=105:500", "-testactivationheight=v20@105", + "-testactivationheight=mn_rr@105"}} + { + } +}; + +BOOST_FIXTURE_TEST_CASE(DeliberateUnlockSurvivesCollateralAutoLocking, MasternodeCollateralTestingSetup) +{ + const CScript wallet_script{GetScriptForDestination(PKHash(coinbaseKey.GetPubKey()))}; + while (WITH_LOCK(m_node.chainman->GetMutex(), return m_node.chainman->ActiveChain().Height()) < + Params().GetConsensus().DIP0003Height) { + CreateAndProcessBlock({}, wallet_script); + } + + CKey owner_key; + CBLSSecretKey operator_key; + auto utxos{BuildSimpleUtxoMap(m_coinbase_txns)}; + CMutableTransaction pro_reg_mtx{CreateProRegTx(*m_node.chainman, utxos, /*port=*/1, wallet_script, + coinbaseKey, owner_key, operator_key)}; + const CTransactionRef pro_reg_tx{MakeTransactionRef(pro_reg_mtx)}; + const CBlock block{CreateAndProcessBlock({pro_reg_mtx}, wallet_script)}; + const CBlockIndex* tip{WITH_LOCK(m_node.chainman->GetMutex(), return m_node.chainman->ActiveChain().Tip())}; + { + LOCK(::cs_main); + m_node.dmnman->UpdatedBlockTip(tip); + BOOST_REQUIRE(m_node.dmnman->GetListAtChainTip().HasMN(pro_reg_tx->GetHash())); + } + + const uint256 block_hash{block.GetHash()}; + interfaces::BlockInfo block_info{block_hash}; + block_info.prev_hash = &block.hashPrevBlock; + block_info.height = tip->nHeight; + block_info.data = █ + wallet->blockConnected(block_info); + + const COutPoint collateral{pro_reg_tx->GetHash(), 0}; + BOOST_CHECK(WITH_LOCK(wallet->cs_wallet, return wallet->IsLockedCoin(collateral))); + + // Unlocking the collateral deliberately opts it out of the automatic protection, so + // that a reload cannot silently take back the decision to spend it. + { + LOCK(wallet->cs_wallet); + WalletBatch batch{wallet->GetDatabase()}; + BOOST_CHECK(wallet->UnlockCoinByUser(collateral, &batch)); + BOOST_CHECK(wallet->IsAutoLockOptOut(collateral)); + } + wallet->AutoLockMasternodeCollaterals(); + BOOST_CHECK(WITH_LOCK(wallet->cs_wallet, return !wallet->IsLockedCoin(collateral))); + + // Locking it again hands it back to the automatic protection. + { + LOCK(wallet->cs_wallet); + WalletBatch batch{wallet->GetDatabase()}; + BOOST_CHECK(wallet->LockCoinByUser(collateral, &batch)); + BOOST_CHECK(!wallet->IsAutoLockOptOut(collateral)); + BOOST_CHECK(wallet->UnlockCoin(collateral, &batch)); + } + wallet->AutoLockMasternodeCollaterals(); + BOOST_CHECK(WITH_LOCK(wallet->cs_wallet, return wallet->IsLockedCoin(collateral))); +} + +BOOST_FIXTURE_TEST_CASE(DeliberateUnlockPrecedesDustProtection, AvailableCoinsTestingSetup) +{ + LOCK(wallet->cs_wallet); + + const auto dest{wallet->GetNewDestination("")}; + BOOST_ASSERT(dest); + + CMutableTransaction mtx; + mtx.vin.emplace_back(COutPoint{uint256::ONE, 0}); + mtx.vout.emplace_back(COIN / 100, GetScriptForDestination(*dest)); + const CTransactionRef tx{MakeTransactionRef(mtx)}; + const COutPoint outpoint{tx->GetHash(), 0}; + + // Dust protection is off, so nothing locks the output automatically. + BOOST_CHECK(wallet->AddToWallet(tx, TxStateInMempool{})); + BOOST_CHECK(!wallet->IsLockedCoin(outpoint)); + + WalletBatch batch{wallet->GetDatabase()}; + BOOST_CHECK(wallet->LockCoinByUser(outpoint, &batch)); + BOOST_CHECK(wallet->UnlockCoinByUser(outpoint, &batch)); + + // Turning dust protection on afterwards must not take the unlock back: an output can + // become a target long after the user decided to spend it. + wallet->m_dust_protection_threshold = 1 * COIN; + wallet->LockExistingDustOutputs(); + BOOST_CHECK(!wallet->IsLockedCoin(outpoint)); +} + BOOST_AUTO_TEST_SUITE_END() } // namespace wallet diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index db1bccb74f4f..7af2b1b469a4 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -56,14 +56,15 @@ class FailBatch : public DatabaseBatch { private: bool m_pass{true}; + bool m_pass_erase{true}; bool ReadKey(CDataStream&&, CDataStream&) override { return m_pass; } bool WriteKey(CDataStream&&, CDataStream&&, bool) override { return m_pass; } - bool EraseKey(CDataStream&&) override { return m_pass; } + bool EraseKey(CDataStream&&) override { return m_pass && m_pass_erase; } bool HasKey(CDataStream&&) override { return m_pass; } - bool ErasePrefix(Span) override { return m_pass; } + bool ErasePrefix(Span) override { return m_pass && m_pass_erase; } public: - explicit FailBatch(bool pass) : m_pass(pass) {} + FailBatch(bool pass, bool pass_erase) : m_pass(pass), m_pass_erase(pass_erase) {} void Flush() override {} void Close() override {} @@ -83,7 +84,8 @@ class FailBatch : public DatabaseBatch class FailDatabase : public WalletDatabase { public: - bool m_pass{true}; // false when this db should fail + bool m_pass{true}; // false when this db should fail + bool m_pass_erase{true}; // false when only erases should fail void Open() override {} void AddRef() override {} @@ -97,7 +99,7 @@ class FailDatabase : public WalletDatabase void ReloadDbEnv() override {} std::string Filename() override { return "faildb"; } std::string Format() override { return "faildb"; } - std::unique_ptr MakeBatch(bool = true) override { return std::make_unique(m_pass); } + std::unique_ptr MakeBatch(bool = true) override { return std::make_unique(m_pass, m_pass_erase); } }; } // namespace @@ -112,9 +114,96 @@ BOOST_AUTO_TEST_CASE(interface_coin_lock_ownership) BOOST_CHECK(wallet_interface->acquireCoinLock(outpoint, /*write_to_db=*/false) == interfaces::CoinLockResult::ACQUIRED); BOOST_CHECK(wallet_interface->acquireCoinLock(outpoint, /*write_to_db=*/false) == interfaces::CoinLockResult::ALREADY_LOCKED); - BOOST_CHECK(wallet_interface->unlockCoin(outpoint)); + BOOST_CHECK(wallet_interface->releaseCoinLock(outpoint, /*write_to_db=*/false)); BOOST_CHECK(wallet_interface->acquireCoinLock(outpoint, /*write_to_db=*/false) == interfaces::CoinLockResult::ACQUIRED); - BOOST_CHECK(wallet_interface->unlockCoin(outpoint)); + BOOST_CHECK(wallet_interface->releaseCoinLock(outpoint, /*write_to_db=*/false)); +} + +BOOST_AUTO_TEST_CASE(unlock_coin_by_user_failed_persist) +{ + auto database{std::make_unique()}; + auto* database_ptr{database.get()}; + auto wallet{std::make_shared(m_node.chain.get(), m_coinjoin_loader.get(), "", m_args, + std::move(database))}; + BOOST_REQUIRE(wallet->LoadWallet() == DBErrors::LOAD_OK); + const COutPoint outpoint{uint256::ONE, 0}; + + LOCK(wallet->cs_wallet); + { + WalletBatch batch{wallet->GetDatabase()}; + BOOST_REQUIRE(wallet->LockCoin(outpoint, &batch)); + } + + // An unlock that failed to persist must not leave a durable opt-out behind: the coin + // is still locked on disk, so a reload would find the two records contradicting. + // Only erases fail, so writing the opt-out would succeed if it were attempted first. + database_ptr->m_pass_erase = false; + WalletBatch batch{wallet->GetDatabase()}; + BOOST_CHECK(!wallet->UnlockCoinByUser(outpoint, &batch)); + BOOST_CHECK(!wallet->IsAutoLockOptOut(outpoint)); +} + +BOOST_AUTO_TEST_CASE(unlock_all_coins_failed_persist) +{ + auto database{std::make_unique()}; + auto* database_ptr{database.get()}; + auto wallet{std::make_shared(m_node.chain.get(), m_coinjoin_loader.get(), "", m_args, + std::move(database))}; + BOOST_REQUIRE(wallet->LoadWallet() == DBErrors::LOAD_OK); + const COutPoint outpoint{uint256::ONE, 0}; + + LOCK(wallet->cs_wallet); + BOOST_REQUIRE(wallet->LockCoin(outpoint)); + + // A failed opt-out write must not leave an opt-out in memory that is not on disk, + // or the next load would relock a coin this process treats as unlocked. + database_ptr->m_pass = false; + BOOST_CHECK(!wallet->UnlockAllCoins()); + BOOST_CHECK(!wallet->IsAutoLockOptOut(outpoint)); +} + +BOOST_AUTO_TEST_CASE(unlock_coin_by_user_without_batch_erases_lock) +{ + auto database{std::make_unique()}; + auto* database_ptr{database.get()}; + auto wallet{std::make_shared(m_node.chain.get(), m_coinjoin_loader.get(), "", m_args, + std::move(database))}; + BOOST_REQUIRE(wallet->LoadWallet() == DBErrors::LOAD_OK); + const COutPoint outpoint{uint256::ONE, 0}; + + LOCK(wallet->cs_wallet); + { + WalletBatch batch{wallet->GetDatabase()}; + BOOST_REQUIRE(wallet->LockCoinByUser(outpoint, &batch)); + } + + // Unlocking without a batch must still take the persisted lock with it, so a failing + // erase has to fail the call rather than leave a durable lock beside a durable opt-out. + database_ptr->m_pass_erase = false; + BOOST_CHECK(!wallet->UnlockCoinByUser(outpoint, /*batch=*/nullptr)); + BOOST_CHECK(!wallet->IsAutoLockOptOut(outpoint)); +} + +BOOST_AUTO_TEST_CASE(unlock_all_coins_failed_erase) +{ + auto database{std::make_unique()}; + auto* database_ptr{database.get()}; + auto wallet{std::make_shared(m_node.chain.get(), m_coinjoin_loader.get(), "", m_args, + std::move(database))}; + BOOST_REQUIRE(wallet->LoadWallet() == DBErrors::LOAD_OK); + const COutPoint outpoint{uint256::ONE, 0}; + + LOCK(wallet->cs_wallet); + { + WalletBatch batch{wallet->GetDatabase()}; + BOOST_REQUIRE(wallet->LockCoin(outpoint, &batch)); + } + + // The lock record survives the failed erase, so no opt-out may be recorded alongside + // it: writes still succeed here, so the opt-out would land if it were attempted. + database_ptr->m_pass_erase = false; + BOOST_CHECK(!wallet->UnlockAllCoins()); + BOOST_CHECK(!wallet->IsAutoLockOptOut(outpoint)); } BOOST_AUTO_TEST_CASE(interface_coin_lock_failed_persist) diff --git a/src/wallet/test/walletload_tests.cpp b/src/wallet/test/walletload_tests.cpp index c77768a20564..fa09ed659e3b 100644 --- a/src/wallet/test/walletload_tests.cpp +++ b/src/wallet/test/walletload_tests.cpp @@ -50,5 +50,36 @@ BOOST_FIXTURE_TEST_CASE(wallet_load_unknown_descriptor, TestingSetup) } } +BOOST_FIXTURE_TEST_CASE(wallet_load_autolock_optout, TestingSetup) +{ + CMutableTransaction mtx; + mtx.vin.emplace_back(COutPoint{uint256::ONE, 0}); + mtx.vout.emplace_back(1000, CScript()); + const CTransactionRef tx{MakeTransactionRef(mtx)}; + const COutPoint known{tx->GetHash(), 0}; + const COutPoint unknown{uint256::ONE, 3}; + + std::unique_ptr database = CreateMockWalletDatabase(); + { + WalletBatch batch(*database, false); + BOOST_CHECK(batch.WriteTx(CWalletTx{tx, TxStateInactive{}})); + BOOST_CHECK(batch.WriteAutoLockOptOut(known)); + BOOST_CHECK(batch.WriteAutoLockOptOut(unknown)); + } + + const std::shared_ptr wallet(new CWallet(m_node.chain.get(), /*coinjoin_loader=*/nullptr, "", m_args, std::move(database))); + BOOST_CHECK_EQUAL(wallet->LoadWallet(), DBErrors::LOAD_OK); + + LOCK(wallet->cs_wallet); + // A deliberate unlock outlives the reload that reapplies the automatic locks. + BOOST_CHECK(wallet->IsAutoLockOptOut(known)); + // A record whose output the wallet no longer knows about is dropped instead. + BOOST_CHECK(!wallet->IsAutoLockOptOut(unknown)); + + WalletBatch batch(wallet->GetDatabase()); + BOOST_CHECK(wallet->LockCoinByUser(known, &batch)); + BOOST_CHECK(!wallet->IsAutoLockOptOut(known)); +} + BOOST_AUTO_TEST_SUITE_END() } // namespace wallet diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index da4a2e08d885..3b49adce4729 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2442,6 +2442,22 @@ DBErrors CWallet::LoadWallet() assert(m_internal_spk_managers == nullptr); } + // Drop opt-outs for outputs the wallet no longer knows about, so that a record cannot + // outlive the output it refers to (ZapSelectTx() removes transactions). Only after a + // clean load: a partial one may simply not have read the transaction yet. + if (nLoadWalletRet == DBErrors::LOAD_OK && !m_autolock_optout.empty()) { + WalletBatch batch(GetDatabase()); + for (auto it = m_autolock_optout.begin(); it != m_autolock_optout.end();) { + const auto wtx_it{mapWallet.find(it->hash)}; + const bool known{wtx_it != mapWallet.end() && it->n < wtx_it->second.tx->vout.size()}; + if (!known && batch.EraseAutoLockOptOut(*it)) { + it = m_autolock_optout.erase(it); + } else { + ++it; + } + } + } + if (HaveChain()) { const std::optional tip_height = chain().getHeight(); if (tip_height) { @@ -2769,13 +2785,81 @@ bool CWallet::UnlockCoin(const COutPoint& output, WalletBatch* batch) return true; } +void CWallet::LoadAutoLockOptOut(const COutPoint& output) +{ + AssertLockHeld(cs_wallet); + m_autolock_optout.insert(output); +} + +bool CWallet::IsAutoLockOptOut(const COutPoint& output) const +{ + AssertLockHeld(cs_wallet); + return m_autolock_optout.count(output) > 0; +} + +bool CWallet::PersistAutoLockOptOut(const COutPoint& output, bool optout, WalletBatch& batch) +{ + AssertLockHeld(cs_wallet); + return optout ? batch.WriteAutoLockOptOut(output) : batch.EraseAutoLockOptOut(output); +} + +// The opt-out is updated only once the lock change itself has stuck, so that a call which +// fails leaves nothing durable behind, and rolled back in memory when its own write fails, +// so that what this process believes always matches what a reload would find. +bool CWallet::LockCoinByUser(const COutPoint& output, WalletBatch* batch) +{ + AssertLockHeld(cs_wallet); + if (!LockCoin(output, batch)) return false; + // A lock the caller keeps in memory only must not clear the opt-out durably: the lock + // is gone after a reload while the decision it was taken against would not be, and the + // automatic protection would take the output back. + if (batch == nullptr) return true; + if (m_autolock_optout.erase(output) > 0 && !PersistAutoLockOptOut(output, /*optout=*/false, *batch)) { + m_autolock_optout.insert(output); + return false; + } + return true; +} + +bool CWallet::UnlockCoinByUser(const COutPoint& output, WalletBatch* batch) +{ + AssertLockHeld(cs_wallet); + if (batch == nullptr) { + // Unlocking is always persistent, and both records have to move together, so a + // caller that brought no batch gets one covering the pair rather than just the + // opt-out. + WalletBatch temp_batch(GetDatabase()); + return UnlockCoinByUser(output, &temp_batch); + } + if (!UnlockCoin(output, batch)) return false; + // Recorded whether or not an automatic protection currently targets `output`: one may + // start to (a ProRegTx registers it as collateral, the dust threshold is raised) long + // after the user made the decision. + if (m_autolock_optout.insert(output).second && !PersistAutoLockOptOut(output, /*optout=*/true, *batch)) { + m_autolock_optout.erase(output); + return false; + } + return true; +} + bool CWallet::UnlockAllCoins() { AssertLockHeld(cs_wallet); bool success = true; WalletBatch batch(GetDatabase()); - for (auto it = setLockedCoins.begin(); it != setLockedCoins.end(); ++it) { - success &= batch.EraseLockedUTXO(*it); + for (const auto& output : setLockedCoins) { + if (!batch.EraseLockedUTXO(output)) { + // The lock record is still on disk, so recording an opt-out for it would leave + // a reload finding the coin locked and the automatic protection told to skip it. + success = false; + continue; + } + // Unlocking everything is a deliberate unlock of each output in turn, so the + // automatic protections must not take them back on the next load either. + if (m_autolock_optout.insert(output).second && !batch.WriteAutoLockOptOut(output)) { + m_autolock_optout.erase(output); + success = false; + } } setLockedCoins.clear(); return success; @@ -2815,6 +2899,7 @@ void CWallet::LockProTxCoins(const std::set& utxos, WalletBatch* batc { AssertLockHeld(cs_wallet); for (const auto& utxo : ListProTxCoins(utxos)) { + if (IsAutoLockOptOut(utxo)) continue; LockCoin(utxo, batch); } } @@ -2824,6 +2909,7 @@ bool CWallet::IsDustProtectionTarget(const CWalletTx& wtx, unsigned int output_i AssertLockHeld(cs_wallet); if (m_dust_protection_threshold <= 0) return false; + if (IsAutoLockOptOut(COutPoint(wtx.GetHash(), output_index))) return false; const CTransactionRef& tx = wtx.tx; if (tx->IsCoinBase() || tx->nType != TRANSACTION_NORMAL) return false; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 641065321e1e..415edad8b8b1 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -336,6 +336,11 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati TxSpends mapTxSpends GUARDED_BY(cs_wallet); void AddToSpends(const COutPoint& outpoint, const uint256& wtxid, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void AddToSpends(const CWalletTx& wtx, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + /** Outpoints the user unlocked deliberately, and which the automatic + * masternode-collateral and dust locks therefore leave alone until the user locks + * them again. Persisted, because the automatic locks are reapplied on every load. */ + std::set m_autolock_optout GUARDED_BY(cs_wallet); + bool PersistAutoLockOptOut(const COutPoint& output, bool optout, WalletBatch& batch) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); std::set setWalletUTXO; /** Add new UTXOs to the wallet UTXO set @@ -651,6 +656,18 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati bool IsLockedCoin(const COutPoint& output) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool LockCoin(const COutPoint& output, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool UnlockCoin(const COutPoint& output, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + /** Lock/unlock `output` because the user asked for it, rather than as part of an + * automatic protection or an internal transient hold. + * + * Unlocking this way also opts `output` out of the automatic masternode-collateral + * and dust locks, which are otherwise reapplied on every wallet load and would put + * the lock back on. Locking it again clears the opt-out and hands the outpoint back + * to the automatic protections. */ + bool LockCoinByUser(const COutPoint& output, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + bool UnlockCoinByUser(const COutPoint& output, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + void LoadAutoLockOptOut(const COutPoint& output) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + /** Has the user unlocked `output` deliberately, opting it out of the automatic locks? */ + bool IsAutoLockOptOut(const COutPoint& output) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool UnlockAllCoins() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); const std::set& ListLockedCoins() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); std::vector ListProTxCoins() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 0dc1f7f68203..8702a8c248bb 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -49,6 +49,7 @@ const std::string HDCHAIN{"hdchain"}; const std::string HDPUBKEY{"hdpubkey"}; const std::string KEYMETA{"keymeta"}; const std::string KEY{"key"}; +const std::string AUTOLOCK_OPTOUT{"autolockoptout"}; const std::string LOCKED_UTXO{"lockedutxo"}; const std::string MASTER_KEY{"mkey"}; const std::string MINVERSION{"minversion"}; @@ -348,6 +349,16 @@ bool WalletBatch::EraseLockedUTXO(const COutPoint& output) return EraseIC(std::make_pair(DBKeys::LOCKED_UTXO, std::make_pair(output.hash, output.n))); } +bool WalletBatch::WriteAutoLockOptOut(const COutPoint& output) +{ + return WriteIC(std::make_pair(DBKeys::AUTOLOCK_OPTOUT, std::make_pair(output.hash, output.n)), uint8_t{'1'}); +} + +bool WalletBatch::EraseAutoLockOptOut(const COutPoint& output) +{ + return EraseIC(std::make_pair(DBKeys::AUTOLOCK_OPTOUT, std::make_pair(output.hash, output.n))); +} + class CWalletScanState { public: unsigned int nKeys{0}; @@ -810,6 +821,12 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, wss.crypted_mnemonics.insert(std::make_pair(std::make_pair(desc_id, pubkey.GetID()), std::make_pair(mnemonic, mnemonic_passphrase))); } + } else if (strType == DBKeys::AUTOLOCK_OPTOUT) { + uint256 hash; + uint32_t n; + ssKey >> hash; + ssKey >> n; + pwallet->LoadAutoLockOptOut(COutPoint(hash, n)); } else if (strType == DBKeys::LOCKED_UTXO) { uint256 hash; uint32_t n; diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index f06177efeb2f..d7c67eb68fdf 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -81,6 +81,7 @@ extern const std::string HDCHAIN; extern const std::string HDPUBKEY; extern const std::string KEY; extern const std::string KEYMETA; +extern const std::string AUTOLOCK_OPTOUT; extern const std::string LOCKED_UTXO; extern const std::string MASTER_KEY; extern const std::string MINVERSION; @@ -247,6 +248,9 @@ class WalletBatch bool WriteLockedUTXO(const COutPoint& output); bool EraseLockedUTXO(const COutPoint& output); + bool WriteAutoLockOptOut(const COutPoint& output); + bool EraseAutoLockOptOut(const COutPoint& output); + bool WriteAddressPreviouslySpent(const CTxDestination& dest, bool previously_spent); bool WriteAddressReceiveRequest(const CTxDestination& dest, const std::string& id, const std::string& receive_request); bool EraseAddressReceiveRequest(const CTxDestination& dest, const std::string& id); diff --git a/test/functional/wallet_dust_protection.py b/test/functional/wallet_dust_protection.py index b4a8fadfa497..81c249b91872 100755 --- a/test/functional/wallet_dust_protection.py +++ b/test/functional/wallet_dust_protection.py @@ -13,6 +13,7 @@ from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, + find_vout_for_address, ) # 1 DASH = 100_000_000 duffs @@ -48,6 +49,8 @@ def run_test(self): self.test_above_threshold_not_locked() self.test_disabled_threshold() self.test_existing_utxos_locked_on_restart() + self.test_deliberate_unlock_survives_restart() + self.test_deliberate_unlock_precedes_protection() self.test_multi_wallet() self.test_invalid_args() @@ -176,6 +179,75 @@ def test_existing_utxos_locked_on_restart(self): # Cleanup node3.lockunspent(True, locked) + def test_deliberate_unlock_survives_restart(self): + """A UTXO the user unlocked deliberately must stay unlocked across restarts.""" + self.log.info("Test: deliberate unlock survives restart") + node1 = self.nodes[1] # dust protection at 10000 duffs + + addr = node1.getnewaddress() + txid = self.nodes[0].sendtoaddress(addr, 9000 * DUFFS) + self.generate(self.nodes[0], 1) + + outpoint = {"txid": txid, "vout": find_vout_for_address(node1, txid, addr)} + assert outpoint in node1.listlockunspent() + + # Unlocking is the documented way to spend a protected output. + node1.lockunspent(True, [outpoint]) + assert outpoint not in node1.listlockunspent() + + # The automatic protection is reapplied on every load, so a deliberate unlock + # has to outlive that or the output could never be spent. + self.restart_node(1, self.extra_args[1]) + # node1 sits in the middle of the node0 <- node1 <- node2 <- node3 chain. + self.connect_nodes(1, 0) + self.connect_nodes(2, 1) + assert outpoint not in node1.listlockunspent() + + # A memory-only lock (the `lockunspent` default) does not take the decision back: + # the lock itself is gone after the restart, so the output must still be unlocked. + node1.lockunspent(False, [outpoint]) + self.restart_node(1, self.extra_args[1]) + # node1 sits in the middle of the node0 <- node1 <- node2 <- node3 chain. + self.connect_nodes(1, 0) + self.connect_nodes(2, 1) + assert outpoint not in node1.listlockunspent() + + # Locking it persistently does hand the output back to the automatic protection. + node1.lockunspent(False, [outpoint], True) + self.restart_node(1, self.extra_args[1]) + self.connect_nodes(1, 0) + self.connect_nodes(2, 1) + assert outpoint in node1.listlockunspent() + + # `lockunspent true` with no outputs unlocks everything, and is just as deliberate. + node1.lockunspent(True) + assert_equal(node1.listlockunspent(), []) + self.restart_node(1, self.extra_args[1]) + self.connect_nodes(1, 0) + self.connect_nodes(2, 1) + assert_equal(node1.listlockunspent(), []) + + def test_deliberate_unlock_precedes_protection(self): + """Unlocking before dust protection is enabled must still opt the output out.""" + self.log.info("Test: deliberate unlock precedes protection") + node3 = self.nodes[3] # no dust protection + + addr = node3.getnewaddress() + txid = self.nodes[0].sendtoaddress(addr, 7000 * DUFFS) + self.generate(self.nodes[0], 1) + outpoint = {"txid": txid, "vout": find_vout_for_address(node3, txid, addr)} + + # Nothing locks it yet, so lock and unlock it by hand. + assert outpoint not in node3.listlockunspent() + node3.lockunspent(False, [outpoint], True) + node3.lockunspent(True, [outpoint]) + assert outpoint not in node3.listlockunspent() + + # Enabling dust protection afterwards must not take the unlock back. + self.restart_node(3, ["-dustrelayfee=0", "-dustprotectionthreshold=10000"]) + self.connect_nodes(3, 2) + assert outpoint not in node3.listlockunspent() + def test_multi_wallet(self): """Dust protection should work across multiple wallets on the same node.""" self.log.info("Test: multi-wallet dust protection") From 723796e7aed4eb030629ba57b2d753375fae7070 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Sat, 22 Aug 2026 10:59:22 +0300 Subject: [PATCH 2/3] doc: add release notes for #7635 Co-Authored-By: Claude Opus 5 --- doc/release-notes-7635.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 doc/release-notes-7635.md diff --git a/doc/release-notes-7635.md b/doc/release-notes-7635.md new file mode 100644 index 000000000000..7654449011be --- /dev/null +++ b/doc/release-notes-7635.md @@ -0,0 +1,13 @@ +Wallet changes +-------------- + +- Unlocking an output with `lockunspent` (or through the Dash-Qt coin control + dialog) now also opts that output out of the automatic masternode-collateral + and dust-protection locks until you lock it again. Previously those automatic + locks were reapplied on every wallet load, which silently undid the unlock and + left the output unspendable with no indication why. Locking the output again + hands it back to the automatic protection, as long as that lock is persistent: + `lockunspent` writes a lock to the wallet file only when asked to, and a + memory-only lock leaves the decision standing. The opt-out is stored in the + wallet file; an older Dash Core release reading the same wallet ignores the + record and reapplies the automatic locks as it did before. (#7635) From 894dfe855312439eee312e34df7677a57b94d9bf Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Sat, 22 Aug 2026 11:22:26 +0300 Subject: [PATCH 3/3] fix(qt): release wizard collateral holds without recording user intent The masternode registration wizard takes a transient collateral hold while a registration is prepared, and releases it when the wizard is destroyed, when the prepare returns no transaction, or when the user discards the prepared registration. Those three paths called unlockCoin(), which now records a deliberate unlock, so cancelling a registration would opt a live collateral out of the automatic lock. The in-memory lock hides that until a restart, after which AutoLockMasternodeCollaterals() skips the collateral and leaves it eligible for ordinary coin selection. Use releaseCoinLock(), the counterpart to the acquireCoinLock() that took the hold, as CollateralLockGuard already does. Also give FailBatch a write-only failure flag so unlock_all_coins_failed_persist reaches the branch it documents: with every operation failing, the lock erase failed first and the opt-out write it means to exercise was never attempted. Co-Authored-By: Claude Opus 5 --- src/qt/masternodewizard.cpp | 6 +++--- src/wallet/test/wallet_tests.cpp | 15 +++++++++------ 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/qt/masternodewizard.cpp b/src/qt/masternodewizard.cpp index 290282e1ab8e..985f32762ffd 100644 --- a/src/qt/masternodewizard.cpp +++ b/src/qt/masternodewizard.cpp @@ -220,7 +220,7 @@ RegisterMasternodeWizard::~RegisterMasternodeWizard() if (m_runner) m_runner->shutdown(); m_runner.reset(); if (m_prepared_collateral_lock_acquired && m_walletModel != nullptr) { - m_walletModel->wallet().unlockCoin(m_prepared_collateral_outpoint); + m_walletModel->wallet().releaseCoinLock(m_prepared_collateral_outpoint, /*write_to_db=*/false); } for (QLineEdit* const edit : {m_secret_edit, m_conf_line_edit, m_confirm_edit}) { edit->setText(QString(edit->text().size(), QLatin1Char('0'))); @@ -1706,7 +1706,7 @@ void RegisterMasternodeWizard::finishPrepare( auto prepared{std::get(std::move(result))}; if (!prepared.tx) { if (prepared.collateral_lock_acquired && m_walletModel != nullptr) { - m_walletModel->wallet().unlockCoin(m_prepared_collateral_outpoint); + m_walletModel->wallet().releaseCoinLock(m_prepared_collateral_outpoint, /*write_to_db=*/false); } if (!m_destroying) showError(tr("The prepare operation completed without returning a transaction.")); return; @@ -1779,7 +1779,7 @@ void RegisterMasternodeWizard::reject() return; } if (m_prepared_collateral_lock_acquired && m_walletModel != nullptr) { - m_walletModel->wallet().unlockCoin(m_prepared_collateral_outpoint); + m_walletModel->wallet().releaseCoinLock(m_prepared_collateral_outpoint, /*write_to_db=*/false); m_prepared_collateral_lock_acquired = false; } m_prepared_tx.reset(); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 7af2b1b469a4..09ad45822f13 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -57,14 +57,16 @@ class FailBatch : public DatabaseBatch private: bool m_pass{true}; bool m_pass_erase{true}; + bool m_pass_write{true}; bool ReadKey(CDataStream&&, CDataStream&) override { return m_pass; } - bool WriteKey(CDataStream&&, CDataStream&&, bool) override { return m_pass; } + bool WriteKey(CDataStream&&, CDataStream&&, bool) override { return m_pass && m_pass_write; } bool EraseKey(CDataStream&&) override { return m_pass && m_pass_erase; } bool HasKey(CDataStream&&) override { return m_pass; } bool ErasePrefix(Span) override { return m_pass && m_pass_erase; } public: - FailBatch(bool pass, bool pass_erase) : m_pass(pass), m_pass_erase(pass_erase) {} + FailBatch(bool pass, bool pass_erase, bool pass_write) : + m_pass(pass), m_pass_erase(pass_erase), m_pass_write(pass_write) {} void Flush() override {} void Close() override {} @@ -86,6 +88,7 @@ class FailDatabase : public WalletDatabase public: bool m_pass{true}; // false when this db should fail bool m_pass_erase{true}; // false when only erases should fail + bool m_pass_write{true}; // false when only writes should fail void Open() override {} void AddRef() override {} @@ -99,7 +102,7 @@ class FailDatabase : public WalletDatabase void ReloadDbEnv() override {} std::string Filename() override { return "faildb"; } std::string Format() override { return "faildb"; } - std::unique_ptr MakeBatch(bool = true) override { return std::make_unique(m_pass, m_pass_erase); } + std::unique_ptr MakeBatch(bool = true) override { return std::make_unique(m_pass, m_pass_erase, m_pass_write); } }; } // namespace @@ -155,9 +158,9 @@ BOOST_AUTO_TEST_CASE(unlock_all_coins_failed_persist) LOCK(wallet->cs_wallet); BOOST_REQUIRE(wallet->LockCoin(outpoint)); - // A failed opt-out write must not leave an opt-out in memory that is not on disk, - // or the next load would relock a coin this process treats as unlocked. - database_ptr->m_pass = false; + // The lock record is erased but the opt-out write fails, so the rollback has to run: + // an opt-out left in memory alone would be gone after a reload. + database_ptr->m_pass_write = false; BOOST_CHECK(!wallet->UnlockAllCoins()); BOOST_CHECK(!wallet->IsAutoLockOptOut(outpoint)); }