fix(wallet): commit DB write before updating in-memory lock state#7158
fix(wallet): commit DB write before updating in-memory lock state#7158UdjinM6 wants to merge 2 commits intodashpay:developfrom
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughLocking/unlocking UTXOs in CWallet was reworked to persist database changes before updating in-memory state and to only remove in-memory locks after successful batch operations. LockCoin now attempts a WalletBatch write for the UTXO before inserting into the in-memory locked set and returns false if the batch write fails. UnlockCoin now returns true immediately if the coin was not locked; if it was locked, it performs a WalletBatch erase and only removes the in-memory lock and recalculates mixed credit after a successful batch erase, returning false on batch failure. UnlockAllCoins now attempts batch erases per locked coin, removes in-memory entries only for successful erases, and then recomputes mixed credit for affected transactions. Sequence Diagram(s)sequenceDiagram
participant Caller
participant CWallet
participant WalletBatch
participant InMemorySet
participant Recalc
Caller->>CWallet: LockCoin(coin, batch?)
CWallet->>WalletBatch: WriteLockedUTXO(coin)
alt WalletBatch write succeeds
WalletBatch-->>CWallet: OK
CWallet->>InMemorySet: InsertLocked(coin)
CWallet-->>Caller: true
else WalletBatch write fails
WalletBatch-->>CWallet: Error
CWallet-->>Caller: false
end
sequenceDiagram
participant Caller
participant CWallet
participant InMemorySet
participant WalletBatch
participant Recalc
Caller->>CWallet: UnlockCoin(coin, batch?)
CWallet->>InMemorySet: contains(coin)?
alt coin was locked
CWallet->>WalletBatch: EraseLockedUTXO(coin)
alt WalletBatch erase succeeds
WalletBatch-->>CWallet: OK
CWallet->>InMemorySet: EraseLocked(coin)
CWallet->>Recalc: RecalculateMixedCredit(txid)
CWallet-->>Caller: true
else WalletBatch erase fails
WalletBatch-->>CWallet: Error
CWallet-->>Caller: false
end
else coin not locked
CWallet-->>Caller: true
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/wallet/wallet.cpp (1)
2728-2738:UnlockAllCoinshas the same class of atomicity gap this PR fixes elsewhere.
setLockedCoins.clear()is called unconditionally regardless of whether the DB batch writes succeed. If anyEraseLockedUTXOcall returns false (or the batch fails to commit on destruction), in-memory state is cleared while the DB retains the locked entries — the reverse divergence from what this PR corrects. This is a pre-existing issue, but worth a follow-up to apply the same "write first, update in-memory only on success" pattern:💡 Suggested follow-up fix
bool CWallet::UnlockAllCoins() { AssertLockHeld(cs_wallet); bool success = true; WalletBatch batch(GetDatabase()); for (auto it = setLockedCoins.begin(); it != setLockedCoins.end(); ++it) { success &= batch.EraseLockedUTXO(*it); } - setLockedCoins.clear(); + if (success) { + setLockedCoins.clear(); + } return success; }Note: even with this change, batch commit failure on
WalletBatchdestruction isn't surfaced. A deeper fix would require a transactional commit check, but this at least prevents clearing in-memory state when individual erases are known to have failed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/wallet.cpp` around lines 2728 - 2738, UnlockAllCoins currently clears in-memory setLockedCoins unconditionally even if WalletBatch::EraseLockedUTXO fails or the batch fails to persist; change CWallet::UnlockAllCoins so it first attempts all EraseLockedUTXO calls (using WalletBatch) and only clear setLockedCoins if every EraseLockedUTXO returned true (i.e., track a local success flag while iterating over setLockedCoins and call setLockedCoins.clear() only when success is true), leaving the in-memory state intact on failure; reference CWallet::UnlockAllCoins, WalletBatch, EraseLockedUTXO, and setLockedCoins when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/wallet/wallet.cpp`:
- Around line 2728-2738: UnlockAllCoins currently clears in-memory
setLockedCoins unconditionally even if WalletBatch::EraseLockedUTXO fails or the
batch fails to persist; change CWallet::UnlockAllCoins so it first attempts all
EraseLockedUTXO calls (using WalletBatch) and only clear setLockedCoins if every
EraseLockedUTXO returned true (i.e., track a local success flag while iterating
over setLockedCoins and call setLockedCoins.clear() only when success is true),
leaving the in-memory state intact on failure; reference
CWallet::UnlockAllCoins, WalletBatch, EraseLockedUTXO, and setLockedCoins when
making the change.
LockCoin and UnlockCoin previously mutated setLockedCoins before writing to the DB. A failed batch write left in-memory and on-disk state diverged until wallet restart. Fix by writing to the DB first and only updating setLockedCoins (and calling RecalculateMixedCredit) on success. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Only remove a coin from setLockedCoins after its DB entry is successfully erased. On failure, leave that coin locked and continue with the rest. Batch RecalculateMixedCredit by affected txid at the end instead of per-item. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
UdjinM6
force-pushed
the
fix/wallet-lock-atomicity
branch
from
e0a6815 to
f956291
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/wallet/wallet.cpp (1)
2716-2726:RecalculateMixedCredit(and the no-operase) called unconditionally even whenwas_locked == false.The
was_lockedvariable is introduced but only used to gate the DB erase. Lines 2723-2724 execute regardless:bool was_locked = setLockedCoins.contains(output); if (batch && was_locked && !batch->EraseLockedUTXO(output)) { return false; } setLockedCoins.erase(output); // no-op when !was_locked RecalculateMixedCredit(output.hash); // unnecessary when !was_locked return true;
setLockedCoins.eraseon a non-member is harmless, butRecalculateMixedCreditdoes non-trivial work (cache invalidation / mixing credit recompute) for a tx whose lock state hasn't changed. Sincewas_lockedis already computed, a guard is straightforward:♻️ Proposed optional refactor
bool CWallet::UnlockCoin(const COutPoint& output, WalletBatch* batch) { AssertLockHeld(cs_wallet); bool was_locked = setLockedCoins.contains(output); if (batch && was_locked && !batch->EraseLockedUTXO(output)) { return false; } - setLockedCoins.erase(output); - RecalculateMixedCredit(output.hash); + if (was_locked) { + setLockedCoins.erase(output); + RecalculateMixedCredit(output.hash); + } return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/wallet.cpp` around lines 2716 - 2726, CWallet::UnlockCoin currently computes was_locked but still calls setLockedCoins.erase(output) and RecalculateMixedCredit(output.hash) unconditionally; change the logic to only attempt batch->EraseLockedUTXO, call setLockedCoins.erase(output) and invoke RecalculateMixedCredit(output.hash) when was_locked is true (i.e., the lock state actually changed), preserving the existing early-return if batch && was_locked && !batch->EraseLockedUTXO(output) fails; references: CWallet::UnlockCoin, setLockedCoins, WalletBatch::EraseLockedUTXO, RecalculateMixedCredit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/wallet/wallet.cpp`:
- Around line 2716-2726: CWallet::UnlockCoin currently computes was_locked but
still calls setLockedCoins.erase(output) and RecalculateMixedCredit(output.hash)
unconditionally; change the logic to only attempt batch->EraseLockedUTXO, call
setLockedCoins.erase(output) and invoke RecalculateMixedCredit(output.hash) when
was_locked is true (i.e., the lock state actually changed), preserving the
existing early-return if batch && was_locked && !batch->EraseLockedUTXO(output)
fails; references: CWallet::UnlockCoin, setLockedCoins,
WalletBatch::EraseLockedUTXO, RecalculateMixedCredit.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/wallet/wallet.cpp (1)
2705-2714: Consider adding an early-return guard for already-locked coins (symmetry withUnlockCoin)
LockCoinnow correctly writes to the DB before updatingsetLockedCoins. However, unlikeUnlockCoin, there is no guard for the case whereoutputis already insetLockedCoins. This results in a redundantWriteLockedUTXODB write and an unnecessaryRecalculateMixedCreditcall — most visible duringAutoLockMasternodeCollaterals()on startup, where masternodes collaterals are re-locked on an already-populated set.♻️ Suggested symmetrical guard
bool CWallet::LockCoin(const COutPoint& output, WalletBatch* batch) { AssertLockHeld(cs_wallet); + if (setLockedCoins.contains(output)) return true; if (batch && !batch->WriteLockedUTXO(output)) { return false; } setLockedCoins.insert(output); RecalculateMixedCredit(output.hash); return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/wallet.cpp` around lines 2705 - 2714, LockCoin should short-circuit when the output is already locked to avoid redundant DB writes and RecalculateMixedCredit calls; modify CWallet::LockCoin to check setLockedCoins (e.g., setLockedCoins.count(output) or contains) at the top and return true immediately if already present, before calling batch->WriteLockedUTXO or RecalculateMixedCredit, preserving the existing behavior when the coin is not yet locked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/wallet/wallet.cpp`:
- Around line 2705-2714: LockCoin should short-circuit when the output is
already locked to avoid redundant DB writes and RecalculateMixedCredit calls;
modify CWallet::LockCoin to check setLockedCoins (e.g.,
setLockedCoins.count(output) or contains) at the top and return true immediately
if already present, before calling batch->WriteLockedUTXO or
RecalculateMixedCredit, preserving the existing behavior when the coin is not
yet locked.
98a30a0 to
f956291
Compare
Can't actually do early returns, dropped 98a30a0.
|
1 participant
Issue being fixed or feature implemented
LockCoinandUnlockCoinpreviously mutatedsetLockedCoinsbefore writing to the DB.UnlockAllCoinsclearedsetLockedCoinsregardless of success status. A failed batch write left in-memory and on-disk state diverged until wallet restart.Discovered in #7155 (comment) by @coderabbitai
What was done?
Fix by writing to the DB first and only updating
setLockedCoins(and callingRecalculateMixedCredit) on success. Also batchRecalculateMixedCreditby affected txid at the end ofUnlockAllCoins.How Has This Been Tested?
Breaking Changes
n/a
Checklist: