feat(qt): replace "toggle lock state" button with a new "(un)lock all" one#7155
Open
UdjinM6 wants to merge 3 commits intodashpay:developfrom
Open
feat(qt): replace "toggle lock state" button with a new "(un)lock all" one#7155UdjinM6 wants to merge 3 commits intodashpay:developfrom
UdjinM6 wants to merge 3 commits intodashpay:developfrom
Conversation
5 tasks
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughAdds two batch methods to the Wallet interface: Sequence Diagram(s)sequenceDiagram
participant User
participant Dialog as "CoinControlDialog"
participant Wallet as "Wallet Interface"
participant Impl as "WalletImpl"
participant Batch as "WalletBatch"
User->>Dialog: Click "(un)lock all"
Dialog->>Dialog: Collect visible UTXOs
alt any UTXO locked
Dialog->>Wallet: unlockCoins(outputs)
else none locked
Dialog->>Wallet: lockCoins(outputs)
end
Wallet->>Impl: lockCoins/unlockCoins(outputs)
Impl->>Batch: create WalletBatch()
loop for each output
Impl->>Impl: LockCoin/UnlockCoin(output, &Batch)
end
Impl-->>Wallet: return success/failure
Wallet-->>Dialog: return success/failure
Dialog->>Dialog: updateView(), updateLabelLocked(), updateLabels()
Dialog-->>User: show success or error
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 (2)
src/qt/coincontroldialog.cpp (1)
189-218: Asymmetric scope: unlock affects all wallet coins; lock affects only visible UTXOs.
unlockAllCoins()(line 194) unlocks every locked coin in the wallet, including coins not currently displayed (e.g., filtered out by the CoinJoin/hide toggle). The lock path (lines 197-213) only locks visible UTXOs from the tree widget. This is consistent with the PR description but worth noting for reviewers: users in a filtered view who click the button will unlock coins they cannot see.Consider adding a brief confirmation dialog (or at minimum a status-bar message) when unlocking coins that aren't currently visible, to avoid user surprise.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/coincontroldialog.cpp` around lines 189 - 218, buttonUnlockAllClicked currently calls model->wallet().unlockAllCoins() which unlocks every locked coin in the wallet while the lock path only locks visible UTXOs; change this by detecting which locked coins are not visible and prompting the user before unlocking them: compute the set of locked COutPoint from model->wallet().listLockedCoins(), compare to the visible outputs collected from ui->treeWidget (the same logic used to build outputs for lockAllCoins), and if there are locked coins that are not in the visible set show a confirmation dialog (e.g. QMessageBox) indicating how many non-visible coins will be unlocked and require explicit user consent before calling unlockAllCoins(); otherwise proceed as before.src/wallet/interfaces.cpp (1)
329-342: Unchecked return values fromlockAllCoinsandunlockAllCoinsat the call site.Both methods return
boolto indicate success, but the caller insrc/qt/coincontroldialog.cpp(lines 194 and 213) discards the return values. IfLockCoinfails mid-iteration inlockAllCoins, the early-return leaves coins partially locked with no user feedback.Consider either: (1) logging/warning on failure at the call site, or (2) not short-circuiting on failure (continue locking remaining coins and return a count or aggregate status). The current early-return behavior is arguably worse than best-effort since it silently stops partway.
#!/bin/bash # Check if LockCoin can actually fail, and what UnlockAllCoins returns echo "=== LockCoin signature and implementation ===" ast-grep --pattern $'bool $_::LockCoin(const COutPoint& $_$$$) { $$$ }' echo "" echo "=== UnlockAllCoins signature and implementation ===" ast-grep --pattern $'bool $_::UnlockAllCoins($$$) { $$$ }'
🤖 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/qt/coincontroldialog.cpp`:
- Around line 189-218: buttonUnlockAllClicked currently calls
model->wallet().unlockAllCoins() which unlocks every locked coin in the wallet
while the lock path only locks visible UTXOs; change this by detecting which
locked coins are not visible and prompting the user before unlocking them:
compute the set of locked COutPoint from model->wallet().listLockedCoins(),
compare to the visible outputs collected from ui->treeWidget (the same logic
used to build outputs for lockAllCoins), and if there are locked coins that are
not in the visible set show a confirmation dialog (e.g. QMessageBox) indicating
how many non-visible coins will be unlocked and require explicit user consent
before calling unlockAllCoins(); otherwise proceed as before.
UdjinM6
force-pushed
the
feat/coin-control-unlock-all
branch
from
9a362fd to
7c83a7c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/qt/coincontroldialog.cpp`:
- Around line 189-221: buttonUnlockAllClicked currently decides fUnlock using
model->wallet().listLockedCoins() (all wallet locks) which can include
hidden/autolocked coins; instead determine unlock intent from the visible UTXOs
you just collected: iterate the existing outputs vector and call
model->wallet().isLockedCoin(COutPoint) for each to build a
visible_locked_outputs vector, set fUnlock = !visible_locked_outputs.empty(),
and when unlocking call model->wallet().unlockCoins(visible_locked_outputs)
(otherwise call lockCoins(outputs)); this ensures the toggle is based only on
visible coins and only-locked visible outpoints are passed to unlockCoins.
In `@src/wallet/interfaces.cpp`:
- Around line 329-346: The loop in lockCoins/unlockCoins mutates the wallet's
in-memory setLockedCoins before the corresponding WalletBatch write succeeds,
causing divergences on early-return; change the logic in lockCoins and
unlockCoins (interfaces.cpp) to perform the persistent write via WalletBatch
first for each COutPoint and only update m_wallet->setLockedCoins (insert or
erase) after the batch WriteLockedUTXO/EraseLockedUTXO returns true; if a batch
write fails, return false immediately without touching in-memory state for that
output so in-memory and on-disk remain consistent.
7c83a7c to
3b92fff
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/qt/coincontroldialog.cpp`:
- Around line 195-214: The code currently sets fUnlock based on the UI disabled
state (item->isDisabled()), which conflates visual disabling with actual
wallet-locked state and causes CoinJoin-denominated-but-unlocked UTXOs to force
the "unlock" branch; instead, determine real lock state via the wallet and only
mark fUnlock when the output is actually locked. In both the List and Tree
branches where you push to outputs (using uint256S(...) and VOutRole), construct
the corresponding COutPoint (from the txid string and vout), call
model->wallet().isLockedCoin(outpoint) (or the appropriate wallet API that
checks a single coin's lock state) and set fUnlock = true only when that returns
true; leave the outputs vector population and the final conditional calling
model->wallet().unlockCoins(outputs) / lockCoins(outputs) unchanged.
3b92fff to
dad8bd6
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/qt/coincontroldialog.cpp (1)
215-216: This is purely an efficiency suggestion — the current code is functionally correct.When
fUnlock = true,unlockCoins(outputs)receives all visible outputs including already-unlocked coins. However,CWallet::UnlockCoinsafely handles this by returningtrueas a no-op for coins not insetLockedCoins(it only attempts database erasure if the coin was actually locked). The loop will not short-circuit spuriously.Passing only locked outputs is cleaner and avoids unnecessary batch operations, but not required for correctness:
♻️ Optional efficiency improvement
- std::vector<COutPoint> outputs; - bool fUnlock = false; + std::vector<COutPoint> outputs; + std::vector<COutPoint> locked_outputs; for (int i = 0; i < ui->treeWidget->topLevelItemCount(); i++) { QTreeWidgetItem* item = ui->treeWidget->topLevelItem(i); if (item->data(COLUMN_ADDRESS, TxHashRole).toString().length() == 64) { const COutPoint outpt(uint256S(item->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()), item->data(COLUMN_ADDRESS, VOutRole).toUInt()); outputs.emplace_back(outpt); - if (model->wallet().isLockedCoin(outpt)) fUnlock = true; + if (model->wallet().isLockedCoin(outpt)) locked_outputs.emplace_back(outpt); } else { for (int j = 0; j < item->childCount(); j++) { QTreeWidgetItem* child = item->child(j); const COutPoint outpt(uint256S(child->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()), child->data(COLUMN_ADDRESS, VOutRole).toUInt()); outputs.emplace_back(outpt); - if (model->wallet().isLockedCoin(outpt)) fUnlock = true; + if (model->wallet().isLockedCoin(outpt)) locked_outputs.emplace_back(outpt); } } } - bool success = fUnlock ? model->wallet().unlockCoins(outputs) - : model->wallet().lockCoins(outputs); + bool fUnlock = !locked_outputs.empty(); + bool success = fUnlock ? model->wallet().unlockCoins(locked_outputs) + : model->wallet().lockCoins(outputs);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/coincontroldialog.cpp` around lines 215 - 216, When fUnlock is true, avoid calling model->wallet().unlockCoins(outputs) with all visible outputs; instead filter outputs to only those present in the wallet's locked set before calling unlockCoins to avoid unnecessary operations. Update the branch that currently calls model->wallet().unlockCoins(outputs) (the conditional using fUnlock) to construct a filtered container of outputs that exist in the wallet's setLockedCoins (or via CWallet::IsLockedCoin/locked coin lookup) and pass that filtered list to unlockCoins; keep the lockCoins(outputs) call unchanged for the fUnlock == false path.
🤖 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/qt/coincontroldialog.cpp`:
- Around line 215-216: When fUnlock is true, avoid calling
model->wallet().unlockCoins(outputs) with all visible outputs; instead filter
outputs to only those present in the wallet's locked set before calling
unlockCoins to avoid unnecessary operations. Update the branch that currently
calls model->wallet().unlockCoins(outputs) (the conditional using fUnlock) to
construct a filtered container of outputs that exist in the wallet's
setLockedCoins (or via CWallet::IsLockedCoin/locked coin lookup) and pass that
filtered list to unlockCoins; keep the lockCoins(outputs) call unchanged for the
fUnlock == false path.
Add a new "(un)lock all" button that batch-locks or batch-unlocks all visible UTXOs. If any visible coin is locked, clicking unlocks only the locked ones; if none are locked, clicking locks them all. New `lockCoins()` / `unlockCoins()` interface methods use a single `WalletBatch` for the entire operation, avoiding per-coin DB overhead. Works in both tree and list modes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The new "(un)lock all" button supersedes "toggle lock state" — it handles both locking and unlocking in a single batch operation and works in both tree and list modes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
UdjinM6
force-pushed
the
feat/coin-control-unlock-all
branch
from
dad8bd6 to
747f88a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/qt/coincontroldialog.cpp (1)
189-226: Batch lock/unlock implementation looks correct; both previous review concerns properly resolved.
lockedOutputsis populated via per-itemisLockedCoin()(notitem->isDisabled()orlistLockedCoins()), soshould_lockis derived from visible coins only.- The unlock path passes only
lockedOutputstounlockCoins, avoiding redundantUnlockCoincalls on already-unlocked outputs.- Post-operation refresh (
updateView→updateLabelLocked→updateLabels) keeps the UI consistent even after a partial failure.Optional: The inner loop calls
isLockedCoin(outpt)once per visible UTXO (N separatecs_walletacquisitions). For wallets with many UTXOs this could be replaced by a singlelistLockedCoins()call + set membership test:♻️ Suggested optimization (optional)
+ const auto allLocked = model->wallet().listLockedCoins(); + const std::unordered_set<COutPoint, SaltedOutpointHasher> lockedSet(allLocked.begin(), allLocked.end()); std::vector<COutPoint> outputs; std::vector<COutPoint> lockedOutputs; for (int i = 0; i < ui->treeWidget->topLevelItemCount(); i++) { QTreeWidgetItem* item = ui->treeWidget->topLevelItem(i); if (item->data(COLUMN_ADDRESS, TxHashRole).toString().length() == 64) { const COutPoint outpt(uint256S(item->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()), item->data(COLUMN_ADDRESS, VOutRole).toUInt()); outputs.emplace_back(outpt); - if (model->wallet().isLockedCoin(outpt)) lockedOutputs.emplace_back(outpt); + if (lockedSet.count(outpt)) lockedOutputs.emplace_back(outpt); } else { for (int j = 0; j < item->childCount(); j++) { QTreeWidgetItem* child = item->child(j); const COutPoint outpt(uint256S(child->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()), child->data(COLUMN_ADDRESS, VOutRole).toUInt()); outputs.emplace_back(outpt); - if (model->wallet().isLockedCoin(outpt)) lockedOutputs.emplace_back(outpt); + if (lockedSet.count(outpt)) lockedOutputs.emplace_back(outpt); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/coincontroldialog.cpp` around lines 189 - 226, The current buttonLockAllClicked implementation correctly collects visible UTXOs but calls model->wallet().isLockedCoin(outpt) for each UTXO (causing N cs_wallet locks); optimize by calling model->wallet().listLockedCoins() once to obtain the set of locked COutPoint and then use set membership checks to populate lockedOutputs instead of per-item isLockedCoin calls, leaving the rest of buttonLockAllClicked, outputs, lockedOutputs, should_lock, lockCoins/unlockCoins, updateView/updateLabelLocked and CoinControlDialog::updateLabels unchanged.
🤖 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/qt/coincontroldialog.cpp`:
- Around line 189-226: The current buttonLockAllClicked implementation correctly
collects visible UTXOs but calls model->wallet().isLockedCoin(outpt) for each
UTXO (causing N cs_wallet locks); optimize by calling
model->wallet().listLockedCoins() once to obtain the set of locked COutPoint and
then use set membership checks to populate lockedOutputs instead of per-item
isLockedCoin calls, leaving the rest of buttonLockAllClicked, outputs,
lockedOutputs, should_lock, lockCoins/unlockCoins, updateView/updateLabelLocked
and CoinControlDialog::updateLabels unchanged.
Avoid a pointless set→vector copy: return `const std::set<COutPoint>&` directly from the wallet layer, and `std::set<COutPoint>` through the interface (copied under lock). In buttonLockAllClicked, fetch the locked set once instead of calling isLockedCoin() per visible UTXO (1 lock acquisition instead of N). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
UdjinM6
force-pushed
the
feat/coin-control-unlock-all
branch
from
85e7a78 to
77bf57c
Compare
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
With dust protection off user could have 10s (or even 100s) of outpoints locked in Coin Control and the only way to unlock them all is going one by one. There is a "toggle lock state" button (#589) which is not very useful in this case imo because it's like half of outpoints are locked and half are not.
What was done?
Replace "toggle lock state" button with a new "(un)lock all" one which behaves similar to "(un)select all" - locks all outpoint when no outpoint is locked, unlocks all if any of them is locked. Works in both modes (tree/list) unlike the old button. Locking 100s of outpoints is also much faster because
lockAllCoins()actually usesWalletBatchto batch operations instead of locking outpoints one by one.How Has This Been Tested?
Breaking Changes
n/a
Checklist: