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

doc/release-notes-7635.md

@@ -0,0 +1,16 @@
+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. 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. Registering the output as a masternode
+  collateral also ends the opt-out and locks the output again, because the
+  decision was made about an ordinary coin and does not carry over to live
+  collateral. 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/interfaces/wallet.h

@@ -224,6 +224,14 @@ class Wallet
     //! Unlock the provided coins in a single batch.
     virtual bool unlockCoins(const std::vector<COutPoint>& outputs) = 0;
 
+    //! Lock a coin because the user asked for it, handing it back to the automatic
+    //! masternode-collateral and dust locks. Use lockCoin() for anything else.
+    virtual bool lockCoinByUser(const COutPoint& output, bool write_to_db) = 0;
+
+    //! Unlock a coin because the user asked for it, opting it out of those automatic
+    //! locks. Use unlockCoin() to release an internal or transient hold.
+    virtual bool unlockCoinByUser(const COutPoint& output) = 0;
+
     //! Set dust protection threshold (does not lock anything by itself).
     virtual void setDustProtectionThreshold(CAmount threshold) = 0;
 

src/qt/coincontroldialog.cpp

@@ -312,7 +312,7 @@ void CoinControlDialog::lockCoin()
         contextMenuItem->setCheckState(COLUMN_CHECKBOX, Qt::Unchecked);
 
     COutPoint outpt(uint256S(contextMenuItem->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()), contextMenuItem->data(COLUMN_ADDRESS, VOutRole).toUInt());
-    model->wallet().lockCoin(outpt, /*write_to_db=*/true);
+    model->wallet().lockCoinByUser(outpt, /*write_to_db=*/true);
     contextMenuItem->setDisabled(true);
     contextMenuItem->setIcon(COLUMN_CHECKBOX, GUIUtil::getIcon("lock_closed", GUIUtil::ThemedColor::RED));
     updateLabelLocked();
@@ -322,7 +322,7 @@ void CoinControlDialog::lockCoin()
 void CoinControlDialog::unlockCoin()
 {
     COutPoint outpt(uint256S(contextMenuItem->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()), contextMenuItem->data(COLUMN_ADDRESS, VOutRole).toUInt());
-    model->wallet().unlockCoin(outpt);
+    model->wallet().unlockCoinByUser(outpt);
     contextMenuItem->setDisabled(false);
     contextMenuItem->setIcon(COLUMN_CHECKBOX, QIcon());
     updateLabelLocked();

src/qt/transactionview.cpp

@@ -491,7 +491,7 @@ void TransactionView::unlockDust()
 
     // Create the outpoint and unlock
     COutPoint outpoint(hash, outputIdx);
-    model->wallet().unlockCoin(outpoint);
+    model->wallet().unlockCoinByUser(outpoint);
 
     // Refresh the transaction view to update the display
     model->getTransactionTableModel()->refreshWallet(true);

src/test/evo_deterministicmns_tests.cpp

@@ -40,51 +40,6 @@
 #include <optional>
 #include <vector>
 
-static CMutableTransaction CreateSpendTx(const ChainstateManager& chainman, SimpleUTXOMap& utxos, const CScript& scriptPayout, CAmount amount, const CKey& coinbaseKey)
-{
-    CMutableTransaction tx;
-    const auto spent = FundTransaction(chainman, tx, utxos, scriptPayout, amount);
-    SignTransaction(tx, spent, coinbaseKey);
-    return tx;
-}
-
-static COutPoint GetCollateralOutpoint(const CMutableTransaction& tx)
-{
-    for (size_t i = 0; i < tx.vout.size(); ++i) {
-        if (tx.vout[i].nValue == dmn_types::Regular.collat_amount) {
-            return COutPoint(tx.GetHash(), i);
-        }
-    }
-    return COutPoint();
-}
-
-// ProRegTx that references a pre-existing collateral output instead of funding the collateral inline.
-static CMutableTransaction CreateProRegTxExternalCollateral(const ChainstateManager& chainman, SimpleUTXOMap& utxos, int port, const COutPoint& collateralOutpoint, const CScript& scriptPayout, const CKey& ownerKey, const CBLSSecretKey& operatorKey, const CKey& collateralKey, const CKey& coinbaseKey)
-{
-    CProRegTx proTx;
-    proTx.nVersion = ProTxVersion::GetMax(!bls::bls_legacy_scheme, /*is_extended_addr=*/false);
-    proTx.netInfo = NetInfoInterface::MakeNetInfo(proTx.nVersion);
-    BOOST_CHECK_EQUAL(proTx.netInfo->AddEntry(NetInfoPurpose::CORE_P2P, strprintf("1.1.1.1:%d", port)),
-                      NetInfoStatus::Success);
-    proTx.collateralOutpoint = collateralOutpoint;
-    proTx.keyIDOwner = ownerKey.GetPubKey().GetID();
-    proTx.pubKeyOperator.Set(operatorKey.GetPublicKey(), bls::bls_legacy_scheme.load());
-    proTx.keyIDVoting = ownerKey.GetPubKey().GetID();
-    proTx.scriptPayout = scriptPayout;
-
-    CMutableTransaction tx;
-    tx.nVersion = 3;
-    tx.nType = TRANSACTION_PROVIDER_REGISTER;
-    // The collateral is external (referenced via collateralOutpoint), so this tx only needs to fund a fee.
-    const auto spent = FundTransaction(chainman, tx, utxos, scriptPayout, /*amount=*/1 * COIN);
-    proTx.inputsHash = CalcTxInputsHash(CTransaction(tx));
-    CMessageSigner::SignMessage(proTx.MakeSignString(), proTx.vchSig, collateralKey);
-    SetTxPayload(tx, proTx);
-    SignTransaction(tx, spent, coinbaseKey);
-
-    return tx;
-}
-
 static CMutableTransaction CreateProUpServTx(const ChainstateManager& chainman, SimpleUTXOMap& utxos, const uint256& proTxHash, const CBLSSecretKey& operatorKey, int port, const CScript& scriptOperatorPayout, const CKey& coinbaseKey,
                                              uint16_t version = ProTxVersion::GetMax(!bls::bls_legacy_scheme, /*is_extended_addr=*/false))
 {

src/test/util/masternode.cpp

@@ -9,6 +9,7 @@
 #include <evo/providertx.h>
 #include <evo/specialtx.h>
 #include <key.h>
+#include <messagesigner.h>
 #include <script/interpreter.h>
 #include <script/sign.h>
 #include <script/signingprovider.h>
@@ -94,6 +95,54 @@ void SignTransaction(CMutableTransaction& tx, const SimpleUTXOMap& coins, const
     Assert(::SignTransaction(tx, &keystore, coins, SIGHASH_ALL, input_errors));
 }
 
+CMutableTransaction CreateSpendTx(const ChainstateManager& chainman, SimpleUTXOMap& utxos,
+                                  const CScript& script_payout, CAmount amount, const CKey& coinbase_key)
+{
+    CMutableTransaction tx;
+    const auto spent = FundTransaction(chainman, tx, utxos, script_payout, amount);
+    SignTransaction(tx, spent, coinbase_key);
+    return tx;
+}
+
+COutPoint GetCollateralOutpoint(const CMutableTransaction& tx)
+{
+    for (size_t i = 0; i < tx.vout.size(); ++i) {
+        if (tx.vout[i].nValue == dmn_types::Regular.collat_amount) {
+            return COutPoint(tx.GetHash(), i);
+        }
+    }
+    return COutPoint();
+}
+
+CMutableTransaction CreateProRegTxExternalCollateral(const ChainstateManager& chainman, SimpleUTXOMap& utxos, int port,
+                                                     const COutPoint& collateral_outpoint, const CScript& script_payout,
+                                                     const CKey& owner_key, const CBLSSecretKey& operator_key,
+                                                     const CKey& collateral_key, const CKey& coinbase_key)
+{
+    CProRegTx pro_tx;
+    pro_tx.nVersion = ProTxVersion::GetMax(!bls::bls_legacy_scheme, /*is_extended_addr=*/false);
+    pro_tx.netInfo = NetInfoInterface::MakeNetInfo(pro_tx.nVersion);
+    Assert(pro_tx.netInfo->AddEntry(NetInfoPurpose::CORE_P2P, strprintf("1.1.1.1:%d", port)) == NetInfoStatus::Success);
+    pro_tx.collateralOutpoint = collateral_outpoint;
+    pro_tx.keyIDOwner = owner_key.GetPubKey().GetID();
+    pro_tx.pubKeyOperator.Set(operator_key.GetPublicKey(), bls::bls_legacy_scheme.load());
+    pro_tx.keyIDVoting = owner_key.GetPubKey().GetID();
+    pro_tx.scriptPayout = script_payout;
+
+    CMutableTransaction tx;
+    tx.nVersion = 3;
+    tx.nType = TRANSACTION_PROVIDER_REGISTER;
+    // The collateral is external, referenced through collateralOutpoint, so this transaction
+    // only has to fund a fee.
+    const auto spent = FundTransaction(chainman, tx, utxos, script_payout, /*amount=*/1 * COIN);
+    pro_tx.inputsHash = CalcTxInputsHash(CTransaction(tx));
+    CMessageSigner::SignMessage(pro_tx.MakeSignString(), pro_tx.vchSig, collateral_key);
+    SetTxPayload(tx, pro_tx);
+    SignTransaction(tx, spent, coinbase_key);
+
+    return tx;
+}
+
 CMutableTransaction CreateProRegTx(const ChainstateManager& chainman, SimpleUTXOMap& utxos, int port,
                                    const CScript& script_payout, const CKey& coinbase_key, CKey& owner_key_ret,
                                    CBLSSecretKey& operator_key_ret)

src/test/util/masternode.h

@@ -27,6 +27,16 @@ void SignTransaction(CMutableTransaction& tx, const SimpleUTXOMap& coins, const
 CMutableTransaction CreateProRegTx(const ChainstateManager& chainman, SimpleUTXOMap& utxos, int port,
                                    const CScript& script_payout, const CKey& coinbase_key, CKey& owner_key_ret,
                                    CBLSSecretKey& operator_key_ret);
+//! Spend `amount` to `script_payout`, e.g. to create a collateral output.
+CMutableTransaction CreateSpendTx(const ChainstateManager& chainman, SimpleUTXOMap& utxos,
+                                  const CScript& script_payout, CAmount amount, const CKey& coinbase_key);
+//! The first output of `tx` holding a regular masternode collateral, or a null outpoint.
+COutPoint GetCollateralOutpoint(const CMutableTransaction& tx);
+//! ProRegTx that references a pre-existing collateral output instead of funding one inline.
+CMutableTransaction CreateProRegTxExternalCollateral(const ChainstateManager& chainman, SimpleUTXOMap& utxos, int port,
+                                                     const COutPoint& collateral_outpoint, const CScript& script_payout,
+                                                     const CKey& owner_key, const CBLSSecretKey& operator_key,
+                                                     const CKey& collateral_key, const CKey& coinbase_key);
 CScript GenerateRandomAddress();
 
 #endif // BITCOIN_TEST_UTIL_MASTERNODE_H

src/wallet/interfaces.cpp

@@ -385,6 +385,18 @@ class WalletImpl : public Wallet
         std::unique_ptr<WalletBatch> batch = std::make_unique<WalletBatch>(m_wallet->GetDatabase());
         return m_wallet->UnlockCoin(output, batch.get());
     }
+    bool lockCoinByUser(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->LockCoinByUser(output, batch.get());
+    }
+    bool unlockCoinByUser(const COutPoint& output) override
+    {
+        LOCK(m_wallet->cs_wallet);
+        WalletBatch batch{m_wallet->GetDatabase()};
+        return m_wallet->UnlockCoinByUser(output, &batch);
+    }
     bool isLockedCoin(const COutPoint& output) override
     {
         LOCK(m_wallet->cs_wallet);
@@ -400,7 +412,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;
     }
@@ -409,7 +421,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");
         }
     }