feat(wallet): let a deliberate unlock opt out of the automatic coin locks

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)

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; }

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.

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<interfaces::PreparedProviderRegistration>(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();

src/wallet/interfaces.cpp

@@ -362,7 +362,7 @@ class WalletImpl : public Wallet
     {
         LOCK(m_wallet->cs_wallet);
         std::unique_ptr<WalletBatch> batch = write_to_db ? std::make_unique<WalletBatch>(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<WalletBatch> batch = write_to_db ? std::make_unique<WalletBatch>(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<WalletBatch> batch = std::make_unique<WalletBatch>(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;
     }

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");
         }
     }
 

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 <bls/bls.h>
+#include <evo/deterministicmns.h>
+#include <evo/dmn_types.h>
+#include <interfaces/chain.h>
+#include <test/util/masternode.h>
 #include <validation.h>
 #include <wallet/coincontrol.h>
 #include <wallet/spend.h>
@@ -15,7 +20,8 @@ BOOST_FIXTURE_TEST_SUITE(availablecoins_tests, WalletTestingSetup)
 class AvailableCoinsTestingSetup : public TestChain100Setup
 {
 public:
-    AvailableCoinsTestingSetup()
+    explicit AvailableCoinsTestingSetup(const std::vector<const char*>& 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 = &block;
+    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