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) 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/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/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..09ad45822f13 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -56,14 +56,17 @@ 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 EraseKey(CDataStream&&) 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; } + bool ErasePrefix(Span) override { return m_pass && m_pass_erase; } public: - explicit FailBatch(bool pass) : m_pass(pass) {} + 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 {} @@ -83,7 +86,9 @@ 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 + bool m_pass_write{true}; // false when only writes should fail void Open() override {} void AddRef() override {} @@ -97,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); } + std::unique_ptr MakeBatch(bool = true) override { return std::make_unique(m_pass, m_pass_erase, m_pass_write); } }; } // namespace @@ -112,9 +117,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)); + + // 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)); +} + +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")