wallet: fix persistent coin lock upgrade #36313

ArvinFarrelP commented at 2:27 AM on September 22, 2026: contributor

Fix a stale persistent coin lock when a temporary coin lock is upgraded to persistent.

Previously, upgrading an existing non-persistent lock to a persistent lock used emplace() without updating the existing value. As a result, the in-memory lock remained non-persistent.

When the coin was later spent, UnlockCoin() therefore did not erase the persistent LOCKED_UTXO database record. After restarting the wallet, the stale lock was loaded again.

This change updates the existing lock state when the coin is already present.

Adds a functional test covering:

temporary coin lock
upgrade to persistent lock
spending the locked coin
wallet reload
verification that the spent lock is not restored

Fixes #36304

DrahtBot added the label Wallet on Sep 22, 2026

DrahtBot commented at 2:28 AM on September 22, 2026: contributor

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

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36313
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36313.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36313#author-ralph

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	jcastros
Concept ACK	vicjuma

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

lockunspent(False, [unspent_0], True) in test/functional/wallet_basic.py

in src/wallet/wallet.cpp:2537 in 8efd73f405

2532 | @@ -2533,7 +2533,10 @@ util::Result<void> CWallet::DisplayAddress(const CTxDestination& dest)
2533 |  void CWallet::LoadLockedCoin(const COutPoint& coin, bool persistent)
2534 |  {
2535 |      AssertLockHeld(cs_wallet);
2536 | -    m_locked_coins.emplace(coin, persistent);
2537 | +    auto [it, inserted] = m_locked_coins.emplace(coin, persistent);
2538 | +    if (!inserted) {

vicjuma commented at 1:11 PM on September 23, 2026:

Testing

Your solution works as expected

Before:

ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ ./bitcoin-cli -regtest lockunspent false '[{"txid":"'"56e8518c816e3e0f4f7c3320bb7fc8cdebc4997ff8bb46777a2e2955283dd9a5"'","vout":0}]' true
true
ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$  RAW=$(./bitcoin-cli -regtest createrawtransaction \
  '[{"txid":"'"56e8518c816e3e0f4f7c3320bb7fc8cdebc4997ff8bb46777a2e2955283dd9a5"'","vout":0}]' \
  '{"'"$ADDR"'":49.999}')
ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ SIGNED=$(./bitcoin-cli -regtest signrawtransactionwithwallet $RAW | jq -r .hex)
ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ ./bitcoin-cli -regtest sendrawtransaction $SIGNED
c5ceb2a49687731fc359f9bfa4bc4deb0353bac100b2267325dc149d6d85720c
ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ ./bitcoin-cli -regtest generatetoaddress 1 $ADDR
[
  "54a452dcaa240214de9de207fa470cbaad9e037b6da66280c019abd2ed9ea868"
]
ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ ./bitcoin-cli -regtest listlockunspent
[
]
ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ ./bitcoin-cli stop
Bitcoin Core stopping
ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ ./bitcoind
Bitcoin Core starting
ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ ./bitcoin-cli loadwallet locktest
{
  "name": "locktest"
}
ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ ./bitcoin-cli -regtest listlockunspent
[
  {
    "txid": "56e8518c816e3e0f4f7c3320bb7fc8cdebc4997ff8bb46777a2e2955283dd9a5",
    "vout": 0
  }
]

After:

ratedg@0xratedg:~/projects/contributions/bitcoin/build2/bin$ ./bitcoin-cli -regtest lockunspent false '[{"txid":"'"642c8da8bd3c98c34a9fcbbc6361ebb33847efb60fc37b4f0dbee5779fde8caf"'","vout":0}]' true
true
ratedg@0xratedg:~/projects/contributions/bitcoin/build2/bin$ ./bitcoin-cli getbalance
50.00000000
ratedg@0xratedg:~/projects/contributions/bitcoin/build2/bin$ RAW=$(./bitcoin-cli -regtest createrawtransaction \
  '[{"txid":"'"$TXID"'","vout":0}]' \
  '{"'"$ADDR"'":49.999}')
error code: -8
error message:
txid must be of length 64 (not 0, for '')
ratedg@0xratedg:~/projects/contributions/bitcoin/build2/bin$ RAW=$(./bitcoin-cli -regtest createrawtransaction \
  '[{"txid":"'"642c8da8bd3c98c34a9fcbbc6361ebb33847efb60fc37b4f0dbee5779fde8caf"'","vout":0}]' \
  '{"'"$ADDR"'":49.999}')
ratedg@0xratedg:~/projects/contributions/bitcoin/build2/bin$ SIGNED=$(./bitcoin-cli -regtest signrawtransactionwithwallet $RAW | jq -r .hex)
ratedg@0xratedg:~/projects/contributions/bitcoin/build2/bin$ ./bitcoin-cli -regtest sendrawtransaction $SIGNED
bf2dbc5d32806197c9beddd750a5b90dd50f855fe2736ed8a7101db1618664aa
ratedg@0xratedg:~/projects/contributions/bitcoin/build2/bin$ ./bitcoin-cli -regtest generatetoaddress 1 $ADDR
[
  "1821ed455cf0c0b5c5b5dbd537922cef873b802e732ae1b2acb53835a5588e06"
]
ratedg@0xratedg:~/projects/contributions/bitcoin/build2/bin$ ./bitcoin-cli -regtest listlockunspent
[
]
ratedg@0xratedg:~/projects/contributions/bitcoin/build2/bin$ ./bitcoin-cli stop
Bitcoin Core stopping
ratedg@0xratedg:~/projects/contributions/bitcoin/build2/bin$ ./bitcoind
Bitcoin Core starting
ratedg@0xratedg:~/projects/contributions/bitcoin/build2/bin$ ./bitcoin-cli loadwallet locktest
{
  "name": "locktest"
}
ratedg@0xratedg:~/projects/contributions/bitcoin/build2/bin$ ./bitcoin-cli -regtest listlockunspent
[
]

ArvinFarrelP commented at 1:02 AM on September 24, 2026:

Thanks for the Concept ACK and testing! I didn't go with unlock-then-lock because it requires two RPC calls and isn't atomic — another process could lock the coin in between. The fix updates the existing entry when emplace reports the coin was already present, handling the upgrade in a single call and matching the existing behavior in coins.cpp:328.

vicjuma commented at 1:22 PM on September 23, 2026: contributor

Concept ACK.

But why won't the user just unlock then lock it again with persistence:-)? So that after

ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ ./bitcoin-cli lockunspent false '[{"txid":"'"ad83e72a347518506d6043643b42a7be682d2178aa2905d63c918e7eeb800feb"'","vout":0}]'
true
ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ ./bitcoin-cli listlockunspent
[
  {
    "txid": "ad83e72a347518506d6043643b42a7be682d2178aa2905d63c918e7eeb800feb",
    "vout": 0
  }
]

they can

ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ ./bitcoin-cli lockunspent true '[{"txid":"'"ad83e72a347518506d6043643b42a7be682d2178aa2905d63c918e7eeb800feb"'","vout":0}]'
true
ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ ./bitcoin-cli listlockunspent
[
]
ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ ./bitcoin-cli lockunspent false '[{"txid":"'"ad83e72a347518506d6043643b42a7be682d2178aa2905d63c918e7eeb800feb"'","vout":0}]' true
true
ratedg@0xratedg:~/projects/contributions/bitcoin/build/bin$ ./bitcoin-cli listlockunspent
[
  {
    "txid": "ad83e72a347518506d6043643b42a7be682d2178aa2905d63c918e7eeb800feb",
    "vout": 0
  }
]

Maybe your version offers a great UX. This code src/wallet/rpc/coins.cpp:328: throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, output already locked");

    if (!fUnlock && is_locked && !persistent) {
            throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, output already locked");
        }

also supports your solution cause it dictates that we could go from non-persistence to persistence in a single call but not the other way around

in src/wallet/wallet.cpp:2536 in 8efd73f405

2532 | @@ -2533,7 +2533,10 @@ util::Result<void> CWallet::DisplayAddress(const CTxDestination& dest)
2533 |  void CWallet::LoadLockedCoin(const COutPoint& coin, bool persistent)
2534 |  {
2535 |      AssertLockHeld(cs_wallet);
2536 | -    m_locked_coins.emplace(coin, persistent);
2537 | +    auto [it, inserted] = m_locked_coins.emplace(coin, persistent);

achow101 commented at 10:01 PM on September 29, 2026:

In 8efd73f405bb1c62ef2d2736552f0cfd78cd62eb "wallet: fix persistent coin lock upgrade"

This can use insert_or_assign rather than setting the fields afterwards on failed emplacement.

ArvinFarrelP commented at 5:58 AM on September 30, 2026:

Thanks for the suggestion! I've updated the code to use insert_or_assign as suggested.

ArvinFarrelP force-pushed on Sep 30, 2026

jcastros commented at 3:57 PM on October 3, 2026: none

Concept ACK on fixing #36304.

Tested bb5dc52082 against its base, bb5296576e.

On the base, the #36304 steps still show the spent coin in listlockunspent after unloadwallet / loadwallet, and wallet_basic.py fails with not(1 == 0). On this commit the list stays empty and the test passes.

The other direction looks broken. Lock the coin with lockunspent persistent, then spend that same coin. On the base, listlockunspent is empty before and after a reload. On this commit it is empty before the reload and the spent output is back afterwards (gettxout is null). I got that from both of these:

createrawtransaction of that input, then fundrawtransaction with lockUnspents=true
sendall with that coin in inputs and lock_unspents=true ("complete": true)

fundrawtransaction and sendall call LockCoin(prevout, false) (spend.cpp:1543, rpc/spend.cpp:1581). insert_or_assign sets the in-memory flag to false and leaves the database record. The spend goes through AddToSpends to UnlockCoin, which skips the erase because the flag is no longer persistent.

A later non-persistent lock should not clear a persistent one. A test for this path would have caught it.

wallet: fix persistent coin lock upgrade 23dbc1fe5c

ArvinFarrelP force-pushed on Oct 3, 2026

ArvinFarrelP commented at 1:05 AM on October 4, 2026: contributor

Good catch. insert_or_assign could downgrade an existing persistent lock when LockCoin() was called non-persistently. I've changed the implementation to preserve the persistent state when the coin is already locked, and added a regression test for this case. wallet_basic.py passes with the new test.

jcastros commented at 1:42 AM on October 4, 2026: none

Tested ACK 23dbc1fe5c.

The downgrade is gone. I locked a coin persistently, spent it with fundrawtransaction lockUnspents=true, and listlockunspent stayed empty across unloadwallet / loadwallet (gettxout null). Same with sendall, that coin in inputs, and lock_unspents=true ("complete": true).

wallet_basic.py passes. Putting insert_or_assign back in LoadLockedCoin fails the new check at wallet_basic.py:239 with not(1 == 0).

DrahtBot requested review from vicjuma on Oct 4, 2026