wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC #32593

/*peristed=*/bool ?

The original documentation is also a bit unclear.

A Coin may be locked if it has already been used to fund a transaction that hasn't confirmed yet.

Is this referring to transactions that were generated either:

1. outside the current wallet and not in our mempool
2. by our wallet but without broadcast, e.g. PSBT workflow with lock_unspents

We never automatically lock coins, all locked coins must be initiated by the user explicitly, whether that is lockcoin or they specify to lock inputs during funding. Locking automatically would result in users being unable to spend some of their funds which we generally want to avoid if there's no good reason to do that behavior.

Could document this to point out that this will update the wallet database if the coin is persisted.

I don't think that's necessary. Many functions make db changes internally and don't have such comments.

The (un)lockCoin functions maintain their own db batches now. I thought it would have led to a decoupling of database read/write operations from the callers but I don't think that's the case because of "each read and write being its own transaction" already unless TxBegin/Commit() is used, which is not the case here.

https://github.com/bitcoin/bitcoin/blob/1c6602399be6de0148938a3fd7b51e6c48b985d2/src/wallet/walletdb.h#L182-L185

This may be a good time to document this function. In particular why it unlocks coins. I'm guessing because we don't want to pollute the wallet db with locks for things that can't be spend anyway? And if there's a reorg we want them to be available.

I think documenting this is rather orthogonal, and especially documenting the coin unlock specifically since it is incidental and not the reason this function exists at all.

The reason we unlock is to not pollute the wallet with things that are meaningless; things that are spent don't need to be locked.

11d0513379d9cbb375f5f6e8877743872225a0ff: why don't we persist this?

Could be a separate commit if we want to change the behaviour. Other than with MuSig2, where the wallet has to stay loaded throughout the session, there's generally no reason to expect that the user will keep the wallet open while co-signers do their thing with e.g. a PSBT.

(ditto for the other RPC methods)

lockunspent did not originally persist to disk. This is a (relatively) recent addition. The point of this is to refactor the code so that it can simplify future PRs, not change the existing behavior.

This looks better to me from readability POV. Earlier it seemed to "lock coin" again right after loading the locked coin and didn't write it again in the disk based on the DB batch not being passed in the second argument that was not intuitive while reading.

I see the previous code also needed to figure out whether the locked coin was persisted on disk while unlocking, but it did so in a slightly complicated way as it depended on whether the DB batch was passed in the unlock flow. Theoretically it could be possible that the code forgets to pass the said batch in the unlock flow leading to unintended scenarios. The new code seems more robust to me as it explicitly tracks whether the coin was persisted on disk.

Should it not check the persistent boolean parameter of the locked coin just like it's checked in UnlockCoin? Otherwise, it tries to erase a locked coin that was not persisted to disk.

Scenario: I lock a coin by calling lockunspent RPC with unlock: false and persistent: false. I then try to unlock all coins by calling lockunspent RPC with unlock: true and letting the persistent param being null. Though the RPC still returns true probably because of the SQLITE's design: https://stackoverflow.com/q/55409865

Added a check for persistent.

Though it was present in previous code as well, I find the usage of bitwise and in boolean operations undesirable.

Changed.

This is much cleaner! This PR's intent seems fine to me as it does indeed improve code readability.

This is the only place where I find the usage of LoadLockedCoin unusual because it comes prior to actually locking the coin in disk. But seems unavoidable because persisting in disk is conditional.

It's generally a pattern in the wallet that we update in memory before writing to disk.

https://github.com/bitcoin/bitcoin/pull/32593/commits/6135e0553e6e58fcf506700991fa178f2c50a266: What happens if persistence fails?

Keeping track of whether a locked coin was persisted is also useful Information for future PRs.

Wouldn't a failed disk write cause this information to be potentially incorrect?

Also, it looks like LockCoin could return false, but the in-memory cache is set.

Also, it looks like LockCoin could return false, but the in-memory cache is set.

Agreed, although it makes sense to set in-memory values before persisting in general, it would not make sense here since the user would see an error indicating that the coin had not been locked, even though it is locked in memory.

This is fine because earlier LockCoin didn't pass a DB batch here that meant not to persist.

Unrelated: I find the lockunspent RPC request params unusual though. The second arg is called transactions but expects transaction outputs. The RPC is called lockunspent but the first arg is called unlock that leads me to straightaway do mental negation calculation while calling the RPC that seems undesirable to me.

It is a bit weird, but changing the interface is probably not going to happen as it would be a breaking change.

This isn't loading anything so the name is a bit odd. Perhaps instead of splitting:

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 6a40cfe97e..a633d7966e 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -2617,19 +2617,13 @@ util::Result<void> CWallet::DisplayAddress(const CTxDestination& dest)
     return util::Error{_("There is no ScriptPubKeyManager for this address")};
 }

-void CWallet::LoadLockedCoin(const COutPoint& coin, bool persistent)
+bool CWallet::LockCoin(const COutPoint& coin, bool persistent, bool persist_now)
 {
     AssertLockHeld(cs_wallet);
     m_locked_coins.emplace(coin, persistent);
-}
-
-bool CWallet::LockCoin(const COutPoint& output, bool persist)
-{
-    AssertLockHeld(cs_wallet);
-    LoadLockedCoin(output, persist);
-    if (persist) {
+    if (persistent && persist_now) {
         WalletBatch batch(GetDatabase());
-        return batch.WriteLockedUTXO(output);
+        return batch.WriteLockedUTXO(coin);
     }
     return true;
 }
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 7fe557f71d..7629de2f9a 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -548,8 +548,7 @@ public:
     util::Result<void> DisplayAddress(const CTxDestination& dest) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

     bool IsLockedCoin(const COutPoint& output) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
-    void LoadLockedCoin(const COutPoint& coin, bool persistent) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
-    bool LockCoin(const COutPoint& output, bool persist) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+    bool LockCoin(const COutPoint& output, bool persistent, bool persist_now = true) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
     bool UnlockCoin(const COutPoint& output) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
     bool UnlockAllCoins() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
     void ListLockedCoins(std::vector<COutPoint>& vOutpts) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
index 0f0e228fc8..58f7409b58 100644
--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -1065,14 +1065,14 @@ static DBErrors LoadTxRecords(CWallet* pwallet, DatabaseBatch& batch, std::vecto
     });
     result = std::max(result, tx_res.m_result);

-    // Load locked utxo record
+    // Load utxo record and lock
     LoadResult locked_utxo_res = LoadRecords(pwallet, batch, DBKeys::LOCKED_UTXO,
         [] (CWallet* pwallet, DataStream& key, DataStream& value, std::string& err) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet) {
         Txid hash;
         uint32_t n;
         key >> hash;
         key >> n;
-        pwallet->LoadLockedCoin(COutPoint(hash, n), /*persistent=*/true);
+        pwallet->LockCoin(COutPoint(hash, n), /*persistent=*/true, /*persist_now=*/false);
         return DBErrors::LOAD_OK;
     });
     result = std::max(result, locked_utxo_res.m_result);

The naming follows our common pattern where Load is used to set things in memory because it is called by the wallet loading code.

But LockCoin isn't loading the from the database, which is what makes this call site confusing.

The way I understood it is that LoadLockedCoin is loading the data into memory and the value (at least parts of it) passed to it in the first argument comes from the database.

But LockCoin isn't loading the from the database, which is what makes this call site confusing.

Neither does SetAddressPreviouslySpent but it still calls LoadAddressPreviouslySpent to set it in memory.

Anyways, I don't think adding more parameters is going to make this any easier to read, nor do I want to bikeshed this naming.

Unrelated nit but mentioning because I noticed it now, the value is not used in this load func, only key is used.

- [] (CWallet* pwallet, DataStream& key, DataStream& value, std::string& err) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet) {
+ [] (CWallet* pwallet, DataStream& key, DataStream&, std::string& err) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet) {

    for (const auto& [coin, persistent] : m_locked_coins) {
        if (!success) break;
        if (persistent) success = batch.EraseLockedUTXO(coin);
    }
    m_locked_coins.clear();

If I need to retouch.