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

achow101 commented at 8:35 pm on May 22, 2025: member

If the locked coin needs to be persisted to the wallet database, insteead of having the RPC figure out when to create a WalletBatch and having LockCoin’s behavior depend on it, have LockCoin take whether to persist as a parameter so it makes the batch.

Since unlocking a persisted locked coin requires a database write as well, we need to track whether the locked coin was persisted to the wallet database so that it can erase the locked coin when necessary.

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

Split from #32489

DrahtBot commented at 8:35 pm on May 22, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32593.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	Sjors

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#bitcoin-core/gui/872 (Menu action to export a watchonly wallet by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

achow101 force-pushed on May 22, 2025

DrahtBot added the label CI failed on May 22, 2025

DrahtBot commented at 9:19 pm on May 22, 2025: contributor

🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/runs/42741157527 LLM reason (✨ experimental): The CI failure is due to a clang-tidy error.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

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

If the locked coin needs to be persisted to the wallet database,
insteead of having the RPC figure out when to create a WalletBatch and
having LockCoin's behavior depend on it, have LockCoin take whether to
persist as a parameter so it makes the batch.

Since unlocking a persisted locked coin requires a database write as
well, we need to track whether the locked coin was persisted to the
wallet database so that it can erase the locked coin when necessary.

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

11d0513379

DrahtBot removed the label CI failed on May 22, 2025

in src/wallet/wallet.h:503 in 11d0513379

496@@ -497,8 +497,10 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
497     /** Set of Coins owned by this wallet that we won't try to spend from. A
498      * Coin may be locked if it has already been used to fund a transaction
499      * that hasn't confirmed yet. We wouldn't consider the Coin spent already,
500-     * but also shouldn't try to use it again. */
501-    std::set<COutPoint> setLockedCoins GUARDED_BY(cs_wallet);
502+     * but also shouldn't try to use it again.
503+     * bool to track whether this locked coin is persisted to disk.
504+     */
505+    std::map<COutPoint, bool> m_locked_coins GUARDED_BY(cs_wallet);

Sjors commented at 10:41 am on May 23, 2025:

/*peristed=*/bool ?

Sjors commented at 10:54 am on May 23, 2025:

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:

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

achow101 commented at 1:20 am on May 24, 2025:

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.

in src/wallet/wallet.h:553 in 11d0513379

547@@ -546,8 +548,9 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
548     util::Result<void> DisplayAddress(const CTxDestination& dest) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
549 
550     bool IsLockedCoin(const COutPoint& output) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
551-    bool LockCoin(const COutPoint& output, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
552-    bool UnlockCoin(const COutPoint& output, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
553+    void LoadLockedCoin(const COutPoint& coin, bool persistent) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
554+    bool LockCoin(const COutPoint& output, bool persist) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
555+    bool UnlockCoin(const COutPoint& output) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

Sjors commented at 10:44 am on May 23, 2025:

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

achow101 commented at 1:15 am on May 24, 2025:

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

in src/wallet/wallet.h:336 in 11d0513379

332@@ -333,8 +333,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
333      */
334     typedef std::unordered_multimap<COutPoint, Txid, SaltedOutpointHasher> TxSpends;
335     TxSpends mapTxSpends GUARDED_BY(cs_wallet);
336-    void AddToSpends(const COutPoint& outpoint, const Txid& txid, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
337-    void AddToSpends(const CWalletTx& wtx, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
338+    void AddToSpends(const COutPoint& outpoint, const Txid& txid) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

Sjors commented at 10:48 am on May 23, 2025:

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.

achow101 commented at 1:17 am on May 24, 2025:

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.

in src/wallet/rpc/spend.cpp:1582 in 11d0513379

1578@@ -1579,7 +1579,7 @@ RPCHelpMan sendall()
1579             const bool lock_unspents{options.exists("lock_unspents") ? options["lock_unspents"].get_bool() : false};
1580             if (lock_unspents) {
1581                 for (const CTxIn& txin : rawTx.vin) {
1582-                    pwallet->LockCoin(txin.prevout);
1583+                    pwallet->LockCoin(txin.prevout, /*persist=*/false);

Sjors commented at 10:59 am on May 23, 2025:

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)

achow101 commented at 1:18 am on May 24, 2025:

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.

Sjors commented at 11:00 am on May 23, 2025: member

Concept ACK

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

Code Coverage & Benchmarks

Reviews

Conflicts