feat(qt): introduce Wallet::startRescan() to replace append-and-restart method in Qt wallet, enable wallet rescan for multiple loaded wallets
#7072
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughIntroduces wallet rescan support across interfaces, implementation, and Qt UI. Adds enum Sequence DiagramsequenceDiagram
participant User
participant RPCConsole
participant WalletController
participant RescanActivity
participant WalletModel
participant WalletImpl
User->>RPCConsole: Click "Rescan" (from_genesis?)
RPCConsole->>WalletController: request rescan(wallet_model, from_genesis)
WalletController->>RescanActivity: create activity and call rescan(...)
RescanActivity->>WalletModel: validate / pass request
RescanActivity->>WalletImpl: call startRescan(from_genesis) [worker thread]
WalletImpl-->>RescanActivity: return wallet::RescanStatus
RescanActivity->>WalletController: emit rescanComplete or rescanFailed
WalletController->>User: show dialogs/signals for result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (3)src/**/*.{cpp,h,hpp,cc}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/qt/**/*.{cpp,h}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/wallet/**/*.{cpp,h}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (20)📓 Common learnings📚 Learning: 2025-12-17T13:58:19.813ZApplied to files:
📚 Learning: 2025-10-05T20:38:28.457ZApplied to files:
📚 Learning: 2025-06-09T16:43:20.996ZApplied to files:
📚 Learning: 2025-02-06T14:34:30.466ZApplied to files:
📚 Learning: 2025-10-25T07:08:51.918ZApplied to files:
📚 Learning: 2025-07-29T14:33:01.040ZApplied to files:
📚 Learning: 2025-10-13T12:37:12.357ZApplied to files:
📚 Learning: 2025-08-11T17:16:36.654ZApplied to files:
📚 Learning: 2025-07-29T14:32:48.369ZApplied to files:
📚 Learning: 2025-11-04T18:24:27.241ZApplied to files:
📚 Learning: 2025-08-08T07:01:47.332ZApplied to files:
📚 Learning: 2025-02-14T15:19:17.218ZApplied to files:
📚 Learning: 2025-11-24T16:41:22.457ZApplied to files:
📚 Learning: 2025-11-24T16:41:22.457ZApplied to files:
📚 Learning: 2025-12-17T13:58:26.891ZApplied to files:
📚 Learning: 2025-11-24T16:41:22.457ZApplied to files:
📚 Learning: 2025-11-24T16:41:22.457ZApplied to files:
📚 Learning: 2025-11-24T16:41:22.457ZApplied to files:
📚 Learning: 2025-11-24T16:41:22.457ZApplied to files:
🧬 Code graph analysis (4)src/qt/walletcontroller.cpp (2)
src/qt/rpcconsole.cpp (3)
src/qt/walletcontroller.h (2)
src/wallet/interfaces.cpp (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
🔇 Additional comments (21)
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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/qt/forms/debugwindow.ui (1)
82-90: Repair tab help text is now misleading relative to in‑place wallet rescansThe updated labels for:
btn_rescan1→ “Rescan blockchain (partial)” / “starting from wallet creation time”, andbtn_rescan2→ “Rescan blockchain” / “starting from genesis block”fit the new
startRescan(from_genesis)behavior well.However,
label_repair_helptextstill states that “The buttons below will restart the node with command-line options…”, which is no longer true for the rescan buttons now that they run via the wallet interface without a restart. Only the reindex path still restarts.Suggest rewording that label to distinguish between:
- options that trigger an in‑place wallet rescan, and
- options like “Rebuild index” that actually restart the node with extra flags.
This avoids confusing users who see a rescan complete without any restart.
Also applies to: 1729-1797
🧹 Nitpick comments (6)
src/interfaces/wallet.h (1)
40-47: RescanStatus forward‑decl and startRescan API are coherent with the wallet layerThe new
wallet::RescanStatusforward declaration andstartRescan(bool from_genesis)interface cleanly expose rescan status to callers and match the concrete implementation inwallet::CWallet.You might optionally expand the comment to spell out:
- that
from_genesis == falsetriggers a “from earliest key time” partial rescan, and- that
RescanStatus::BUSYspecifically means another rescan or scan is already running.Functionally this looks good.
Also applies to: 97-101
src/wallet/wallet.h (2)
237-258: RescanStatus enum cleanly mirrors ScanResult outcomesDefining
enum class RescanStatus : uint8_t { BUSY, FAILURE, SUCCESS, USER_ABORT }here is consistent withCWallet::ScanResult::statusand the interface return type.If you want to make this easier to maintain, consider adding brief comments per value (e.g., “BUSY – a rescan or scan is already in progress”) so future contributors don’t have to trace through
ScanForWalletTransactionsto understand them. Otherwise this looks fine.
274-279: WalletRescanReserver RAII behavior and concurrency look sound; consider unifying the time source
WalletRescanReserver::reserve()correctly:
- prevents concurrent scans via
m_wallet.fScanningWallet.exchange(true),- initializes
m_scanning_start,m_scanning_progress, andfAbortRescan, and- relies on the destructor to reset
fScanningWalleteven on early exit.Two small polish points to consider:
You already have an injectable
Clock/NowFn(now()/setNow()), butreserve()still usesSystemClock::now()directly. If tests intend to control time, it may be clearer to route the start‑time assignment throughnow()(or otherwise drop the injection if it isn’t actually used anywhere).If you ever extend rescan metrics, it may be worth explicitly zeroing
m_scanning_progress(and related fields) also on destructor, not just onreserve(), to guarantee a clean state after any scan path.Current behavior is correct; these are just maintainability tweaks.
Also applies to: 1090-1131
src/wallet/interfaces.cpp (1)
176-204: startRescan() wiring is correct; consider handling a couple of edge cases more explicitlyThe implementation aligns well with the new interface:
from_genesis == falsefinds the earliest key time across all script‑pubkey managers and starts from the first block at (time −TIMESTAMP_WINDOW),from_genesis == truestarts from height 0, andWalletRescanReserverenforces a single active scan, returningRescanStatus::BUSYwhen another rescan is in progress.Two small robustness tweaks you might consider:
findFirstBlockWithTimeAndHeight return value
Right now, iffindFirstBlockWithTimeAndHeight(...)fails (e.g. unexpected pruning/DB issue),rescan_heightsilently remains 0 and the “partial” option degrades into a full rescan. That’s safe but opaque. Checking the bool return and either logging or returningRescanStatus::FAILUREin that case would make behavior more intentional.Default switch branch on ScanResult
Theswitchfalls through toreturn RescanStatus::SUCCESS;for any futureCWallet::ScanResultvalues. IfScanResultis ever extended, that would misreport unknown outcomes as success. Treating the default as a failure (or adding anassert(false)/UNREACHABLE) would be safer.None of this blocks the current change; the core behavior looks solid.
src/qt/walletcontroller.cpp (1)
531-551: Consider more specific message box titles.Lines 538 and 546 both use the title "Rescan wallet failed", but they represent different scenarios:
- Line 538: Actual failure (potentially corrupted data)
- Line 546: Wallet is busy (temporary state)
For Line 546, consider using a more specific title like "Rescan unavailable" to distinguish it from actual failures.
Apply this diff:
case wallet::RescanStatus::BUSY: - QMessageBox::warning(m_parent_widget, tr("Rescan wallet failed"), tr("Wallet is currently rescanning. Abort existing rescan or wait.")); + QMessageBox::warning(m_parent_widget, tr("Rescan unavailable"), tr("Wallet is currently rescanning. Abort existing rescan or wait.")); Q_EMIT rescanFailed(); break;src/qt/rpcconsole.cpp (1)
926-942: Document the wallet selector index assumption.Line 934 hardcodes
itemData(1)to retrieve the wallet model. Based on theaddWallet()andremoveWallet()logic (lines 829-870), index 0 appears reserved and index 1 is the first/only wallet. The rescan buttons are disabled when multiple wallets are loaded (lines 846-847).Add a clarifying comment:
+ // Index 1 always contains the wallet when exactly one wallet is loaded + // (rescan buttons are disabled for 0 or multiple wallets) WalletModel* wallet_model{ui->WalletSelector->itemData(1).value<WalletModel*>()};Alternatively, consider using
ui->WalletSelector->currentData()if the intent is to operate on the currently selected wallet, though the current approach aligns with the single-wallet enforcement.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between ea73b83 and 28c3dff0036459ec42c18282913837dd0c6bfb25.
📒 Files selected for processing (9)
src/interfaces/wallet.h(2 hunks)src/qt/bitcoingui.cpp(1 hunks)src/qt/forms/debugwindow.ui(5 hunks)src/qt/rpcconsole.cpp(4 hunks)src/qt/rpcconsole.h(5 hunks)src/qt/walletcontroller.cpp(1 hunks)src/qt/walletcontroller.h(2 hunks)src/wallet/interfaces.cpp(1 hunks)src/wallet/wallet.h(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,h,hpp,cc}
📄 CodeRabbit inference engine (CLAUDE.md)
Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1
Files:
src/interfaces/wallet.hsrc/wallet/interfaces.cppsrc/qt/walletcontroller.cppsrc/wallet/wallet.hsrc/qt/rpcconsole.cppsrc/qt/walletcontroller.hsrc/qt/rpcconsole.hsrc/qt/bitcoingui.cpp
src/wallet/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Wallet implementation must use Berkeley DB and SQLite
Files:
src/wallet/interfaces.cppsrc/wallet/wallet.h
src/qt/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
GUI implementation in src/qt/ must use Qt 5
Files:
src/qt/walletcontroller.cppsrc/qt/rpcconsole.cppsrc/qt/walletcontroller.hsrc/qt/rpcconsole.hsrc/qt/bitcoingui.cpp
🧠 Learnings (6)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/interfaces/wallet.hsrc/wallet/interfaces.cppsrc/wallet/wallet.hsrc/qt/rpcconsole.cppsrc/qt/rpcconsole.hsrc/qt/forms/debugwindow.ui
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,txmempool}/**/*.{cpp,h} : Block validation and mempool handling must use extensions to Bitcoin Core mechanisms for special transaction validation and enhanced transaction relay
Applied to files:
src/wallet/interfaces.cpp
📚 Learning: 2025-02-14T15:19:17.218Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Applied to files:
src/qt/rpcconsole.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/wallet/**/*.{cpp,h} : Wallet implementation must use Berkeley DB and SQLite
Applied to files:
src/qt/rpcconsole.cppsrc/qt/walletcontroller.hsrc/qt/rpcconsole.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/qt/**/*.{cpp,h} : GUI implementation in src/qt/ must use Qt 5
Applied to files:
src/qt/rpcconsole.cpp
🧬 Code graph analysis (6)
src/interfaces/wallet.h (1)
src/wallet/interfaces.cpp (2)
from_genesis(176-204)from_genesis(176-176)
src/wallet/interfaces.cpp (2)
src/interfaces/chain.h (1)
FoundBlock(54-79)src/wallet/test/wallet_tests.cpp (4)
reserver(123-123)reserver(143-143)reserver(187-187)reserver(214-214)
src/qt/walletcontroller.cpp (2)
src/wallet/interfaces.cpp (2)
from_genesis(176-204)from_genesis(176-176)src/qt/walletcontroller.h (2)
m_rescan_wallet_model(191-193)worker(98-98)
src/qt/rpcconsole.cpp (1)
src/qt/bitcoingui.cpp (2)
setWalletController(920-943)setWalletController(920-920)
src/qt/walletcontroller.h (2)
src/qt/walletcontroller.cpp (16)
RescanWalletActivity(515-518)WalletControllerActivity(189-195)rescan(520-529)rescan(520-520)rescanComplete(535-535)rescanFailed(539-539)rescanFailed(543-543)rescanFailed(547-547)finish(280-367)finish(280-280)finish(408-419)finish(408-408)finish(497-513)finish(497-497)finish(531-552)finish(531-531)src/wallet/interfaces.cpp (2)
from_genesis(176-204)from_genesis(176-176)
src/qt/rpcconsole.h (4)
src/qt/walletcontroller.cpp (2)
WalletController(42-59)WalletController(63-68)src/qt/walletcontroller.h (1)
WalletController(47-83)src/qt/rpcconsole.cpp (10)
setWalletController(824-827)setWalletController(824-824)walletReindex(958-961)walletReindex(958-958)walletRescan1(945-948)walletRescan1(945-945)walletRescan2(951-954)walletRescan2(951-951)walletRescan(927-942)walletRescan(927-927)src/qt/bitcoingui.cpp (2)
setWalletController(920-943)setWalletController(920-920)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: win64-build / Build source
🔇 Additional comments (9)
src/qt/bitcoingui.cpp (1)
920-943: Hooking RPCConsole up to WalletController looks correct; just confirm lifetime handlingWiring
wallet_controllerintorpcConsolehere makes sense so the console can drive wallet‑level actions (like rescan). Please just confirm thatRPCConsole::setWalletController()mirrorsBitcoinGUI’sWalletController::destroyedhandling (e.g., clears its internal pointer or disconnects signals) so it never dereferences a destroyed controller.src/qt/walletcontroller.h (1)
175-194: LGTM!The new
RescanWalletActivityclass follows the established pattern of other wallet controller activities (CreateWalletActivity, OpenWalletActivity, etc.). The structure is clean with appropriate signals for completion and failure states.src/qt/walletcontroller.cpp (1)
515-518: LGTM!The constructor follows the established pattern for wallet controller activities.
src/qt/rpcconsole.cpp (3)
576-584: LGTM!The comment change from "Wallet Repair Buttons" to "Repair Buttons" is appropriate since the reindex button (Line 584) is not wallet-specific. The wallet-specific wiring is correctly guarded by
#ifdef ENABLE_WALLET.
824-827: LGTM!Simple setter method that integrates the wallet controller with the RPC console.
944-954: LGTM!The two rescan methods clearly delegate to the common
walletRescan()helper with appropriatefrom_genesisparameters. The comments effectively document the difference between the two rescan modes.src/qt/rpcconsole.h (3)
27-27: LGTM!The forward declaration and
setWalletController()method are properly declared and guarded by#ifdef ENABLE_WALLET.Also applies to: 62-62
122-127: LGTM!The reorganization correctly places
walletReindex()outside the#ifdef ENABLE_WALLETguard since it only triggers a restart with the-reindexflag, which is not wallet-specific. The wallet-specific rescan methods remain properly guarded.
181-184: LGTM!The private
walletRescan()helper method andm_wallet_controllermember are properly declared and guarded by#ifdef ENABLE_WALLET. The member initialization follows modern C++ conventions.Also applies to: 199-199
kwvg
force-pushed
the
qt_rescan
branch
3 times, most recently
from
9295d46 to
f4daa88
Compare
Co-authored-by: UdjinM6 <[email protected]>
kwvg
changed the title
Wallet::startRescan() to replace append-and-restart method in Qt walletWallet::startRescan() to replace append-and-restart method in Qt wallet, enable wallet rescan for multiple loaded wallets
|
80e4a33: changes in |
3 participants
Motivation
bitcoin#23123 removes the
-rescanstartup parameter outright and users are expected to utilise therescanblockchainRPC instead. This requires us to rework how the rescan option is implemented in Dash Qt, which uses the startup parameter.This pull request replaces it by implementing the core rescan logic in the wallet interface (as opposed to trying to invoke an RPC call from the Qt wallet, which would violate separation of concerns) and then implementing a wallet activity to avoid blocking the main thread.
An additional benefit of this approach is that the rescan happens immediately without the overhead of a client restart.
Additional Information
m_rescan_wallet_modelis now aQPointer(which will auto-reset when the underlying entity is destroyed, source) and we do one final check in the one-shot lambda before resolving the pointer.Breaking Changes
None expected.
Checklist