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

pull achow101 wants to merge 1 commits into bitcoin:master from achow101:improve-lockcoin-interface changing 8 files +49 −47
  1. 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

  2. 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
    ACK rkrux, Sjors

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

    Conflicts

    No conflicts as of last run.

  3. achow101 force-pushed on May 22, 2025
  4. DrahtBot added the label CI failed on May 22, 2025
  5. 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.

  6. DrahtBot removed the label CI failed on May 22, 2025
  7. in src/wallet/wallet.h:503 in 11d0513379 outdated
    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:

    1. outside the current wallet and not in our mempool
    2. 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.
  8. in src/wallet/wallet.h:553 in 11d0513379 outdated
    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.

    rkrux commented at 2:27 pm on June 2, 2025:

    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

  9. in src/wallet/wallet.h:336 in 11d0513379 outdated
    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.

  10. in src/wallet/rpc/spend.cpp:1582 in 11d0513379 outdated
    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.
  11. Sjors commented at 11:00 am on May 23, 2025: member
    Concept ACK
  12. in src/wallet/walletdb.cpp:1075 in 11d0513379 outdated
    1071@@ -1072,7 +1072,7 @@ static DBErrors LoadTxRecords(CWallet* pwallet, DatabaseBatch& batch, std::vecto
    1072         uint32_t n;
    1073         key >> hash;
    1074         key >> n;
    1075-        pwallet->LockCoin(COutPoint(hash, n));
    1076+        pwallet->LoadLockedCoin(COutPoint(hash, n), /*persistent=*/true);
    


    rkrux commented at 1:29 pm on June 2, 2025:
    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.
  13. in src/wallet/wallet.cpp:2640 in 11d0513379 outdated
    2642+bool CWallet::UnlockCoin(const COutPoint& output)
    2643 {
    2644     AssertLockHeld(cs_wallet);
    2645-    bool was_locked = setLockedCoins.erase(output);
    2646-    if (batch && was_locked) {
    2647-        return batch->EraseLockedUTXO(output);
    


    rkrux commented at 1:44 pm on June 2, 2025:
    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.
  14. in src/wallet/wallet.cpp:2658 in 11d0513379 outdated
    2653@@ -2647,26 +2654,24 @@ bool CWallet::UnlockAllCoins()
    2654     AssertLockHeld(cs_wallet);
    2655     bool success = true;
    2656     WalletBatch batch(GetDatabase());
    2657-    for (auto it = setLockedCoins.begin(); it != setLockedCoins.end(); ++it) {
    2658-        success &= batch.EraseLockedUTXO(*it);
    2659+    for (const auto& [coin, _] : m_locked_coins) {
    2660+        success &= batch.EraseLockedUTXO(coin);
    


    rkrux commented at 2:05 pm on June 2, 2025:

    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


    achow101 commented at 7:28 pm on June 2, 2025:
    Added a check for persistent.
  15. in src/wallet/wallet.cpp:2651 in 11d0513379 outdated
    2653@@ -2647,26 +2654,24 @@ bool CWallet::UnlockAllCoins()
    2654     AssertLockHeld(cs_wallet);
    2655     bool success = true;
    2656     WalletBatch batch(GetDatabase());
    2657-    for (auto it = setLockedCoins.begin(); it != setLockedCoins.end(); ++it) {
    2658-        success &= batch.EraseLockedUTXO(*it);
    


    rkrux commented at 2:07 pm on June 2, 2025:
    Though it was present in previous code as well, I find the usage of bitwise and in boolean operations undesirable.

    achow101 commented at 7:28 pm on June 2, 2025:
    Changed.
  16. in src/wallet/rpc/coins.cpp:364 in 11d0513379 outdated
    365         if (fUnlock) {
    366-            if (!pwallet->UnlockCoin(outpt, batch.get())) throw JSONRPCError(RPC_WALLET_ERROR, "Unlocking coin failed");
    367+            if (!pwallet->UnlockCoin(outpt)) throw JSONRPCError(RPC_WALLET_ERROR, "Unlocking coin failed");
    368         } else {
    369-            if (!pwallet->LockCoin(outpt, batch.get())) throw JSONRPCError(RPC_WALLET_ERROR, "Locking coin failed");
    370+            if (!pwallet->LockCoin(outpt, persistent)) throw JSONRPCError(RPC_WALLET_ERROR, "Locking coin failed");
    


    rkrux commented at 2:10 pm on June 2, 2025:
    This is much cleaner! This PR’s intent seems fine to me as it does indeed improve code readability.
  17. in src/wallet/wallet.cpp:2630 in 11d0513379 outdated
    2629+
    2630+bool CWallet::LockCoin(const COutPoint& output, bool persist)
    2631+{
    2632+    AssertLockHeld(cs_wallet);
    2633+    LoadLockedCoin(output, persist);
    2634+    if (persist) {
    


    rkrux commented at 2:14 pm on June 2, 2025:
    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.

    achow101 commented at 7:28 pm on June 2, 2025:
    It’s generally a pattern in the wallet that we update in memory before writing to disk.

    Eunovo commented at 4:53 pm on June 14, 2025:

    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.

  18. in src/wallet/spend.cpp:1470 in 11d0513379 outdated
    1466@@ -1467,7 +1467,7 @@ util::Result<CreatedTransactionResult> FundTransaction(CWallet& wallet, const CM
    1467 
    1468     if (lockUnspents) {
    1469         for (const CTxIn& txin : res->tx->vin) {
    1470-            wallet.LockCoin(txin.prevout);
    1471+            wallet.LockCoin(txin.prevout, /*persist=*/false);
    


    rkrux commented at 2:29 pm on June 2, 2025:
    This is fine because earlier LockCoin didn’t pass a DB batch here that meant not to persist.
  19. in src/wallet/rpc/coins.cpp:356 in 11d0513379 outdated
    355@@ -356,16 +356,12 @@ RPCHelpMan lockunspent()
    356         outputs.push_back(outpt);
    


    rkrux commented at 2:38 pm on June 2, 2025:
    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.

    achow101 commented at 7:29 pm on June 2, 2025:
    It is a bit weird, but changing the interface is probably not going to happen as it would be a breaking change.
  20. rkrux commented at 2:40 pm on June 2, 2025: contributor

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

    Is the only benefit that while exporting watch only version of the wallet, the code need not read the locked coin from the disk and instead can read from the m_locked_coins map?

    https://github.com/bitcoin/bitcoin/pull/32489/files#diff-1f2db0e4d5c12d109c7f0962333c245b49b696cb39ff432da048e9d6c08944d8R4604-R4608

    Earlier I had few concerns regarding the persistence of the locked coins incase of backup and restoration of the wallet but I don’t think they are valid concerns for this PR because this diff doesn’t change this behaviour, it only refactors and maintains a persistence boolean param in memory. One such concern being whether the persisted locked coins are handled properly in case of reorgs.

    ACK 11d0513379d9cbb375f5f6e8877743872225a0ff

    I’m convinced that this diff leaves the wallet codebase in a slightly better state.

  21. DrahtBot requested review from Sjors on Jun 2, 2025
  22. achow101 force-pushed on Jun 2, 2025
  23. DrahtBot added the label CI failed on Jun 2, 2025
  24. DrahtBot commented at 7:54 pm on June 2, 2025: contributor

    🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/runs/43333360130 LLM reason (✨ experimental): Compilation failed due to a self-comparison in wallet.cpp at line 2644, caused by an incorrect use of the ‘==’ operator instead of an assignment operator.

    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.

  25. 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.
    6135e0553e
  26. achow101 force-pushed on Jun 2, 2025
  27. DrahtBot removed the label CI failed on Jun 2, 2025
  28. rkrux commented at 2:26 pm on June 3, 2025: contributor

    ACK 6135e05

    0git range-diff 11d0513...6135e05
    

    The PR is a refactor that improves code readability and robustness upto some extent.

  29. in src/wallet/wallet.cpp:2629 in 6135e0553e
    2628+}
    2629+
    2630+bool CWallet::LockCoin(const COutPoint& output, bool persist)
    2631+{
    2632+    AssertLockHeld(cs_wallet);
    2633+    LoadLockedCoin(output, persist);
    


    Sjors commented at 2:41 pm on June 5, 2025:

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

     0diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
     1index 6a40cfe97e..a633d7966e 100644
     2--- a/src/wallet/wallet.cpp
     3+++ b/src/wallet/wallet.cpp
     4@@ -2617,19 +2617,13 @@ util::Result<void> CWallet::DisplayAddress(const CTxDestination& dest)
     5     return util::Error{_("There is no ScriptPubKeyManager for this address")};
     6 }
     7
     8-void CWallet::LoadLockedCoin(const COutPoint& coin, bool persistent)
     9+bool CWallet::LockCoin(const COutPoint& coin, bool persistent, bool persist_now)
    10 {
    11     AssertLockHeld(cs_wallet);
    12     m_locked_coins.emplace(coin, persistent);
    13-}
    14-
    15-bool CWallet::LockCoin(const COutPoint& output, bool persist)
    16-{
    17-    AssertLockHeld(cs_wallet);
    18-    LoadLockedCoin(output, persist);
    19-    if (persist) {
    20+    if (persistent && persist_now) {
    21         WalletBatch batch(GetDatabase());
    22-        return batch.WriteLockedUTXO(output);
    23+        return batch.WriteLockedUTXO(coin);
    24     }
    25     return true;
    26 }
    27diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
    28index 7fe557f71d..7629de2f9a 100644
    29--- a/src/wallet/wallet.h
    30+++ b/src/wallet/wallet.h
    31@@ -548,8 +548,7 @@ public:
    32     util::Result<void> DisplayAddress(const CTxDestination& dest) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
    33
    34     bool IsLockedCoin(const COutPoint& output) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
    35-    void LoadLockedCoin(const COutPoint& coin, bool persistent) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
    36-    bool LockCoin(const COutPoint& output, bool persist) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
    37+    bool LockCoin(const COutPoint& output, bool persistent, bool persist_now = true) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
    38     bool UnlockCoin(const COutPoint& output) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
    39     bool UnlockAllCoins() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
    40     void ListLockedCoins(std::vector<COutPoint>& vOutpts) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
    41diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
    42index 0f0e228fc8..58f7409b58 100644
    43--- a/src/wallet/walletdb.cpp
    44+++ b/src/wallet/walletdb.cpp
    45@@ -1065,14 +1065,14 @@ static DBErrors LoadTxRecords(CWallet* pwallet, DatabaseBatch& batch, std::vecto
    46     });
    47     result = std::max(result, tx_res.m_result);
    48
    49-    // Load locked utxo record
    50+    // Load utxo record and lock
    51     LoadResult locked_utxo_res = LoadRecords(pwallet, batch, DBKeys::LOCKED_UTXO,
    52         [] (CWallet* pwallet, DataStream& key, DataStream& value, std::string& err) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet) {
    53         Txid hash;
    54         uint32_t n;
    55         key >> hash;
    56         key >> n;
    57-        pwallet->LoadLockedCoin(COutPoint(hash, n), /*persistent=*/true);
    58+        pwallet->LockCoin(COutPoint(hash, n), /*persistent=*/true, /*persist_now=*/false);
    59         return DBErrors::LOAD_OK;
    60     });
    61     result = std::max(result, locked_utxo_res.m_result);
    

    achow101 commented at 8:46 pm on June 5, 2025:
    The naming follows our common pattern where Load is used to set things in memory because it is called by the wallet loading code.

    Sjors commented at 7:57 am on June 6, 2025:
    But LockCoin isn’t loading the from the database, which is what makes this call site confusing.

    rkrux commented at 8:20 am on June 6, 2025:
    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.

    achow101 commented at 6:10 pm on June 6, 2025:

    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.

  30. Sjors commented at 2:58 pm on June 5, 2025: member
    ACK 6135e0553e6e58fcf506700991fa178f2c50a266

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-06-18 12:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me