#36313 wallet: fix persistent coin lock upgrade

ReviewabilityReviewsAgreementSize
#36313 ArvinFarrelP wallet: fix persistent coin lock upgradeReady1Strong4 + 39 tests
  • Prevent spent coins from reappearing as locked when a wallet is restarted or reloaded
  • Ensure upgraded persistent coin locks are properly cleared when coins are spent
  • Review is worthwhile now with no blocking changes or pending author updates
  • General agreement that the fix is correct and improves wallet consistency (vicjuma, jcastros)
  • Verified through manual testing and automated regression tests (vicjuma, jcastros)
  • Previous objection about temporary locks downgrading persistent locks was fixed and verified (jcastros)
  • 4 lines added or modified outside tests
  • 39 lines added or modified in tests
  • 1 lines removed in total
  • 2 files, 1 commits
Wallet#37 of 92
P3 · bug fix
  • P3 because it fixes an edge-case locking inconsistency that does not risk funds or wallet corruption
  • Prevents confusing ghost locks for wallet users upgrading temporary coin locks before spending
  • Ranking pass: Moved up: a reproducible persistent-lock bug with a linked issue outranks the P3 cleanups it was scored beside.
Ranking notes
Reviewability: ReadyStalePaused
Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets
Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs.
Size: lines added or modified outside tests, then in tests; greener = smaller.

Goal

Updates `CWallet::LoadLockedCoin` to set `it->second |= persistent` when an entry already exists in `m_locked_coins`. This ensures that upgrading a non-persistent lock to a persistent lock updates the in-memory flag while preventing subsequent temporary locks from downgrading a persistent lock.

Problem: When a coin with an existing non-persistent lock (such as from `fundrawtransaction`) is upgraded to a persistent lock, `m_locked_coins.emplace()` leaves the in-memory persistence flag set to `false`. When the coin is subsequently spent, `UnlockCoin()` skips deleting the persistent `LOCKED_UTXO` database record, causing the spent coin to reappear as locked whenever the wallet is restarted or reloaded.

Evidence of importance:

  • Fixes issue #36304: 'wallet: upgrading a non-persistent coin lock to persistent leaves a stale lock on the spent output after restart'
  • PR author description confirms `m_locked_coins.emplace()` did not update the value, preventing `UnlockCoin()` from erasing the database record upon spending

Risk of a mistake: low (wallet, wallet_storage, tests). Flaws in wallet coin lock state tracking could lead to spent coins remaining locked or unspent coins failing to unlock properly across restarts.

Changed since the previous assessment: Switched from replacing entries with `insert_or_assign` to `it->second |= persistent` on existing entries, preventing temporary locks like `fundrawtransaction` from downgrading an existing persistent lock, and added a test case for downgrade prevention.

Reviewability

Ready

Nothing known that would invalidate a review now.

Waiting on (from the thread read):

  • nothing: nothing pending; objection resolved and PR has tested ACK 2026-10-04

Agreement

Strong

substantive support, no open objection (vicjuma, vicjuma, jcastros, jcastros)

Thread notes: Author pushed a fix preventing non-persistent locks from downgrading persistent ones along with a regression test; jcastros verified the fix and gave a Tested ACK.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
achow101 (member)suggestion, approachUse insert_or_assign rather than setting the fields afterwards on failed emplacementfixed · fix pushed · author updated the code to use insert_or_assignansweredyes2026-09-29: 'This can use `insert_or_assign` rather than setting the fields afterwards on failed emplacement.'
Status from: 2026-09-30: ArvinFarrelP: 'I've updated the code to use `insert_or_assign` as suggested.'
jcastros (none)objection, correctnessA non-persistent lock overwrites an existing persistent lock in memory without removing the db record, causing spent outputs to be restored as locked on reloadaccepted · fix pushed · author fixed downgrade and added test; reviewer confirmed and Tested ACKedchangedyes2026-10-03: '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 '
Status from: 2026-10-04: jcastros: 'Tested ACK 23dbc1fe5c. The downgrade is gone.'

Support:

  • vicjuma (contributor) (Concept ACK, 2026-09-23) [read]: offers good UX and existing RPC validation already supports upgrading from non-persistent to persistent in a single call
  • vicjuma (contributor) (2026-09-23) [tested]: tested solution and confirmed before/after behavior works as expected
  • jcastros (none) (Concept ACK, 2026-10-03): Concept ACK on fixing #36304
  • jcastros (none) (Tested ACK, 2026-10-04) [both: test/functional/wallet_basic.py]: verified persistent lock is not downgraded when spent with lockUnspents=true across unload/load wallet, and regression test catches insert_or_assign behavior

Participants:

  • vicjuma (contributor, support, 2 statements 2026-09-23 to 2026-09-23): Concept ACK and manual testing confirmation
  • achow101 (member, neutral, 1 statements 2026-09-29 to 2026-09-29): suggested using insert_or_assign
  • jcastros (none, objection, 2 statements 2026-10-03 to 2026-10-04): objected to persistent lock downgrade regression, then Tested ACK after fix

Files

39 lines under test/bench/ci.

  • test/functional/wallet_basic.py +39/-0
  • src/wallet/wallet.cpp +4/-1

Card

The short text the ranking pass compares across PRs.

Updates LoadLockedCoin to set the persistence flag if an entry is already present in m_locked_coins instead of ignoring updates via emplace. Upgrading a temporary coin lock to persistent previously left the in-memory entry marked non-persistent; spending the coin then skipped deleting the persistent database record and left a phantom lock on reload. It also ensures non-persistent locking attempts cannot downgrade an existing persistent lock. Fixes issue #36304 with accompanying functional tests.

Data

dossier JSON · extract JSON · model ledger, generated 2026-10-05T01:16, confidence high, input hash b79f918a9a4189be
thread read through 2026-10-04 (8 statements), head 23dbc1fe5c
labels: Wallet

Categories

Wallet

P3 · bug fix (ranking pass: Moved up: a reproducible persistent-lock bug with a linked issue outranks the P3 cleanups it was scored beside.)

Fixes a bug where upgrading an existing non-persistent coin lock to persistent leaves stale `LOCKED_UTXO` records in the database if the coin is spent, causing spent coins to reappear as locked upon wallet reload (#36304). While an annoying state inconsistency, it is an edge-case locking bug that does not risk funds or wallet corruption, making it reasonably deferrable.

Membership: Matches the Wallet label and modifies coin locking logic in `src/wallet/wallet.cpp`.

Factors: security/stability 1, bug 1, performance 0, user value 1, leverage 0