backport: batch 2 of migratewallet RPC#7277
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
🔍 Review in progress — actively reviewing now (commit 16071be) |
WalkthroughThis pull request introduces a comprehensive wallet migration feature that allows users to convert legacy BDB-based wallets to modern SQLite-based descriptor wallets. The changes span the interfaces layer (defining the migration result types and WalletLoader API), the Qt GUI layer (adding menu actions and an activity-based workflow), and supporting infrastructure (passphrase collection modifications). The implementation enables users to initiate migration through the File menu, confirm the migration with appropriate warnings, supply the wallet passphrase if encrypted, and receive feedback on successful completion or errors. Sequence DiagramsequenceDiagram
participant User as User
participant BitcoinGUI as BitcoinGUI
participant Activity as MigrateWalletActivity
participant WalletLoader as WalletLoader
participant WalletModel as WalletModel
participant Backend as Migration Backend
User->>BitcoinGUI: Click File > Migrate Wallet
BitcoinGUI->>Activity: launch (wallet_model)
Activity->>User: Show confirmation dialog
User-->>Activity: Confirm + enter passphrase
Activity->>WalletLoader: migrateWallet(name, passphrase)
WalletLoader->>Backend: convert BDB to SQLite
Backend->>Backend: extract legacy descriptors
Backend->>Backend: apply migration data
Backend-->>WalletLoader: WalletMigrationResult
WalletLoader-->>Activity: migration complete
Activity->>WalletModel: getOrCreateWallet
Activity->>BitcoinGUI: setCurrentWallet
Activity->>User: Show success message
User-->>BitcoinGUI: Migrated wallet active
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)src/qt/askpassphrasedialog.cppsrc/qt/askpassphrasedialog.cpp:10:10: fatal error: 'qt/askpassphrasedialog.h' file not found ... [truncated 1118 characters] ... tall/lib/clang/18/include" src/qt/bitcoingui.cppsrc/qt/bitcoingui.cpp:6:10: fatal error: 'qt/bitcoingui.h' file not found ... [truncated 1063 characters] ... clang/install/lib/clang/18/include" src/qt/walletcontroller.cppsrc/qt/walletcontroller.cpp:5:10: fatal error: 'qt/walletcontroller.h' file not found ... [truncated 1099 characters] ... install/lib/clang/18/include" 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: 7
🧹 Nitpick comments (2)
src/wallet/wallet.h (1)
1072-1073: Pass the mnemonic passphrase byconst SecureString&.Both overloads currently copy
SecureString, which needlessly duplicates secret material in memory. Changeconst SecureString mnemonic_passphrasetoconst SecureString& mnemonic_passphrasefor both function declarations.♻️ Proposed fix
- void SetupDescriptorScriptPubKeyMans(const CExtKey& master_key, const SecureString& mnemonic, const SecureString mnemonic_passphrase) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - void SetupDescriptorScriptPubKeyMans(const SecureString& mnemonic, const SecureString mnemonic_passphrase) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + void SetupDescriptorScriptPubKeyMans(const CExtKey& master_key, const SecureString& mnemonic, const SecureString& mnemonic_passphrase) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + void SetupDescriptorScriptPubKeyMans(const SecureString& mnemonic, const SecureString& mnemonic_passphrase) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/wallet.h` around lines 1072 - 1073, Change the two declarations of SetupDescriptorScriptPubKeyMans to take the mnemonic passphrase by reference instead of by value: replace the parameter type `const SecureString mnemonic_passphrase` with `const SecureString& mnemonic_passphrase` in both overloads (the functions declared as `void SetupDescriptorScriptPubKeyMans(const CExtKey& master_key, const SecureString& mnemonic, const SecureString mnemonic_passphrase) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);` and `void SetupDescriptorScriptPubKeyMans(const SecureString& mnemonic, const SecureString mnemonic_passphrase) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);`) so they become `const SecureString&` for the passphrase.src/wallet/scriptpubkeyman.cpp (1)
1930-1940: Consider adding a nullptr check afterParsefor defensive programming.While the descriptor string constructed from a valid key should always parse successfully, the HD chain path (line 2018-2021) includes explicit nullptr checking. Adding consistency here would guard against unexpected failures.
🔧 Optional: Add nullptr check for consistency
// Construct the combo descriptor std::string desc_str = "combo(" + origin_str + HexStr(key.GetPubKey()) + ")"; FlatSigningProvider keys; std::string error; std::unique_ptr<Descriptor> desc = Parse(desc_str, keys, error, false); + if (!desc) { + throw std::runtime_error(std::string(__func__) + ": failed to parse combo descriptor: " + error); + } WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/scriptpubkeyman.cpp` around lines 1930 - 1940, Add a nullptr check after calling Parse on desc_str to mirror the defensive checks used elsewhere: verify that the returned std::unique_ptr<Descriptor> desc is non-null before constructing WalletDescriptor and creating DescriptorScriptPubKeyMan; if desc is null, surface the parse error (from the existing error string) and handle it consistently (e.g., log and return or throw) instead of proceeding to call WalletDescriptor, DescriptorScriptPubKeyMan, AddDescriptorKey, and TopUp.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/managing-wallets.md`:
- Around line 125-144: Confirm whether documentation updates were intended for
this backport: if the backport explicitly requested doc changes, add a note to
the PR description referencing that approval and keep the edits in the doc
(references: "Migrating Legacy Wallets to Descriptor Wallets", migratewallet,
restorewallet); otherwise revert the changes to doc/managing-wallets.md (lines
introducing migratewallet documentation) and move them to a separate
documentation-only follow-up PR so this backport contains only code changes.
In `@src/wallet/wallet.cpp`:
- Around line 4629-4630: UpdateWalletSetting is being called before helper
wallets (e.g. *_watchonly and *_solvables) are fully committed, causing
load_on_startup entries to remain if a later step fails and the wallet
directories are rolled back; move the UpdateWalletSetting calls (the ones that
set load_on_startup=true) so they execute only after the wallet commit/migration
has completed successfully (i.e., after all filesystem operations and rollback
points are passed) and do the same change for the other instance noted (the
second call around the other occurrence), ensuring settings are updated only on
successful commit of the new wallet directories.
- Around line 4794-4796: The backup copy operation in the wallet
migration/rollback code can attempt to copy a file onto itself for direct-file
wallets (where backup_path and temp_backup_location resolve to the same path),
causing copy_file to fail; update the logic around the uses of
temp_backup_location and backup_path (the copy_file calls around the code that
uses GetWalletDir(), backup_filename, and the rollback path) to guard against
this by checking if fs::equivalent(backup_path, temp_backup_location) or
backup_path == temp_backup_location and skipping the copy (or generating a
unique temp name) when they refer to the same file; apply the same guard to the
later copy_file occurrence(s) referenced near the other block (around the code
at the second copy_file use).
- Around line 4040-4041: The exception currently echoes the full seed phrase
when CMnemonic::Check(mnemonic) fails; remove the sensitive mnemonic from the
error text and throw a generic message instead (e.g., change the throw in the
CMnemonic::Check failure branch to use only __func__ and a non-sensitive string
like ": invalid mnemonic" without including the mnemonic variable or any
backticks). Ensure the change is applied where the throw is raised (the block
that calls CMnemonic::Check and constructs std::runtime_error).
In `@src/wallet/walletdb.cpp`:
- Around line 1182-1184: In EraseRecords(), the return value of
m_batch->Erase(key_data) is ignored causing EraseRecords() to always succeed;
update the loop that checks types.count(type) > 0 to capture the boolean result
of m_batch->Erase(key_data) and if it returns false, immediately return false
(or set an overall failure flag and return false at the end) so EraseRecords()
propagates delete failures back to the caller; ensure you reference the
EraseRecords() method and the m_batch->Erase(...) call when making this change.
In `@test/functional/wallet_migration.py`:
- Around line 141-143: The test currently mutates the dict returned by
basic0.getaddressinfo(new_addr) instead of checking it; replace the assignment
on basic0.getaddressinfo(new_addr)["hdkeypath"] = "m/44'/1'/0'/0/1" with a real
assertion: call info = basic0.getaddressinfo(new_addr) and assert
info["hdkeypath"] == "m/44'/1'/0'/0/1" (while keeping the existing assert not
new_addr == addr), so the test actually verifies the derived path returned for
new_addr.
---
Nitpick comments:
In `@src/wallet/scriptpubkeyman.cpp`:
- Around line 1930-1940: Add a nullptr check after calling Parse on desc_str to
mirror the defensive checks used elsewhere: verify that the returned
std::unique_ptr<Descriptor> desc is non-null before constructing
WalletDescriptor and creating DescriptorScriptPubKeyMan; if desc is null,
surface the parse error (from the existing error string) and handle it
consistently (e.g., log and return or throw) instead of proceeding to call
WalletDescriptor, DescriptorScriptPubKeyMan, AddDescriptorKey, and TopUp.
In `@src/wallet/wallet.h`:
- Around line 1072-1073: Change the two declarations of
SetupDescriptorScriptPubKeyMans to take the mnemonic passphrase by reference
instead of by value: replace the parameter type `const SecureString
mnemonic_passphrase` with `const SecureString& mnemonic_passphrase` in both
overloads (the functions declared as `void SetupDescriptorScriptPubKeyMans(const
CExtKey& master_key, const SecureString& mnemonic, const SecureString
mnemonic_passphrase) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);` and `void
SetupDescriptorScriptPubKeyMans(const SecureString& mnemonic, const SecureString
mnemonic_passphrase) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);`) so they become
`const SecureString&` for the passphrase.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: be43b772-e7a7-47b1-af64-43f1656f21c8
📒 Files selected for processing (18)
doc/managing-wallets.mddoc/release-notes-19602.mdsrc/script/descriptor.cppsrc/wallet/rpc/wallet.cppsrc/wallet/scriptpubkeyman.cppsrc/wallet/scriptpubkeyman.hsrc/wallet/test/ismine_tests.cppsrc/wallet/transaction.cppsrc/wallet/transaction.hsrc/wallet/wallet.cppsrc/wallet/wallet.hsrc/wallet/walletdb.cppsrc/wallet/walletdb.hsrc/wallet/walletutil.htest/functional/test_framework/test_node.pytest/functional/test_runner.pytest/functional/tool_wallet.pytest/functional/wallet_migration.py
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
bp-descriptor-migrate-2
branch
from
8b96a2b to
db077ef
Compare
knst
force-pushed
the
bp-descriptor-migrate-2
branch
from
db077ef to
3c8b4c6
Compare
7ecc29a test: wallet, add coverage for addressbook migration (furszy) a277f83 wallet: migration bugfix, persist empty labels (furszy) 1b64f64 wallet: migration bugfix, clone 'send' record label to all wallets (furszy) Pull request description: Addressing two specific bugs encountered during the wallet migration process, related to the address book, and improves the test coverage for it. Bug 1: Non-Cloning of External 'Send' Records The external 'send' records were not being correctly cloned to all wallets. Bug 2: Persistence of Empty Labels As address book entries without associated db label records can be treated as change (the `label` field inside the `CAddressBookData` class is optional, `nullopt` labels make `CAddressBookData ::IsChange()` return true), we must persist empty labels during the migration process. The user might have called `setlabel` with an "" string for an external address and that must be retained during migration. ACKs for top commit: achow101: ACK 7ecc29a Tree-SHA512: b8a8483a4178a37c49af11eb7ba8a82ca95e54a6cd799e155e33f9fbe7f37b259e28372c77d6944d46b6765f9eaca6b8ca8d1cdd9d223120a3653e4e41d0b6b7 Co-authored-by: fanquake <fanquake@gmail.com>
knst
force-pushed
the
bp-descriptor-migrate-2
branch
from
3c8b4c6 to
b7cbf55
Compare
48aae2c gui: Add File > Migrate Wallet (Andrew Chow) 577be88 gui: Optionally return passphrase after unlocking (Andrew Chow) 5b3a85b interfaces, wallet: Expose migrate wallet (Andrew Chow) Pull request description: GUI users need to be able to migrate wallets without going to the RPC console. ACKs for top commit: jarolrod: ACK 48aae2c pablomartin4btc: tACK 48aae2c hebasto: ACK 48aae2c Tree-SHA512: 2d02b1e85e7d6cfbf503f417f150cdaa0c63822942e9a6fe28c0ad3e7f40a957bb01a375c909a60432dc600e84574881aa446c7ec983b56f0bb23f07ef15de54 Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
17554ef test: prefer sqlite for wallet tests (S3RK) 8e0faba test: make wallet_migration.py pass with both wallet flags (S3RK) Pull request description: Fixes bitcoin#26511 ACKs for top commit: MarcoFalke: review ACK 17554ef achow101: ACK 17554ef Tree-SHA512: 97cae275998f07032feffe1b533d4747b8ff03c3c1fb830af69ee38cadb75fd58532956f66f79c0d275b00620ce53b0b5240f885e4f29b8bd4d0b6e6cbc683fa Co-authored-by: Andrew Chow <github@achow101.com>
… dir a1e6538 test: Add test for migrating default wallet and plain file wallet (Andrew Chow) bdbe3fd wallet: Generated migrated wallet's path from walletdir and name (Andrew Chow) Pull request description: This PR fixes an assertion error that is hit during the setup of the new database during migration of a wallet that was not contained in a wallet dir. Also added a test for this case as well as one for migrating the default wallet. ACKs for top commit: ryanofsky: Code review ACK a1e6538 furszy: ACK a1e6538 Tree-SHA512: 96b218c0de8567d8650ec96e1bf58b0f8ca4c4726f5efc6362453979b56b9d569baea0bb09befb3a5aed8d16d29bf75ed5cd8ffc432bbd4cbcad3ac5574bc479 Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
knst
force-pushed
the
bp-descriptor-migrate-2
branch
from
b7cbf55 to
16071be
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16071beef7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (enc_status == WalletModel::EncryptionStatus::Locked || enc_status == WalletModel::EncryptionStatus::Unlocked) { | ||
| AskPassphraseDialog dlg(AskPassphraseDialog::Unlock, m_parent_widget, &passphrase); | ||
| dlg.setModel(wallet_model); | ||
| dlg.exec(); |
There was a problem hiding this comment.
For encrypted wallets, dlg.exec() can return Rejected when the user cancels or closes the passphrase dialog, but this result is ignored and the code immediately removes the wallet model and starts migration with an empty passphrase. In that scenario migration fails decryption after unloading/removing the wallet from the GUI, leaving the user’s wallet closed even though they canceled; return early unless the dialog is accepted.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
backported code; if there's no relevant backport from bitcoin-core, this comment would be ignored. @thepastaclaw any relevant backport for this fix?
There was a problem hiding this comment.
Yes — the relevant upstream follow-up is bitcoin-core/gui#824, specifically commit d56a450 (gui: Use wallet name for wallet migration rather than WalletModel).
That commit changes the migration passphrase flow to use AskPassphraseDialog::UnlockMigration and returns early on cancellation:
AskPassphraseDialog dlg(AskPassphraseDialog::UnlockMigration, m_parent_widget, &passphrase);
if (dlg.exec() == QDialog::Rejected) return;So this Codex finding is real upstream-covered behavior, but it is part of the broader “migrate legacy wallets that are not loaded” GUI follow-up rather than a small standalone fix on top of gui#738.
There was a problem hiding this comment.
it can't be done right now, because bitcoin-core/gui#824 depends on bitcoin#27790 which seems impossible to be done right now; I am going to prioritize it for batch 3 but it seems as non-trivial yet.
For Bitcoin Core the gap between bitcoin-core/gui#824 and bitcoin-core/gui#738 is 2 major versions btw.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16071beef7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| auto res = wallet::MigrateLegacyToDescriptor(name, passphrase, m_context); | ||
| if (!res) return util::Error{util::ErrorString(res)}; | ||
| WalletMigrationResult out{ | ||
| .wallet = MakeWallet(m_context, res->wallet), |
There was a problem hiding this comment.
If the post-migration LoadWallet in wallet.cpp fails after the on-disk migration succeeds, res->wallet is left null; MakeWallet then returns a null WalletMigrationResult::wallet while this interface still reports success. The GUI success path immediately dereferences res->wallet in MigrateWalletActivity, so a reload error turns into a GUI crash instead of an error message. Please validate the reloaded wallet and propagate the load failure before returning success.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/qt/bitcoingui.cpp`:
- Around line 622-626: After migrating a wallet via m_migrate_wallet_action,
also update the RPC console context so it matches the GUI current wallet; modify
the lambda that creates MigrateWalletActivity to, after calling
BitcoinGUI::setCurrentWallet with walletFrame->currentWalletModel(), invoke the
same RPC-console update used by the open/create/restore flows (for example call
the existing RPC console update method such as
rpcConsole->setCurrentWalletModel(walletFrame->currentWalletModel()) or the
BitcoinGUI helper that those flows use) so the RPC console's wallet context is
switched to the migrated wallet as well.
In `@src/qt/walletcontroller.cpp`:
- Around line 581-590: The AskPassphraseDialog result is ignored, allowing
cancellation to continue migration with an empty passphrase; change the flow so
the wallet removal and subsequent migration only proceed when the dialog is
accepted. After creating AskPassphraseDialog (AskPassphraseDialog::Unlock) and
setting the model (dlg.setModel(wallet_model)), check the dialog result (e.g. if
(dlg.exec() == QDialog::Accepted) or dlg.result() == QDialog::Accepted) and only
then continue to call m_wallet_controller->removeAndDeleteWallet(wallet_model)
and perform migration; if the dialog is rejected/cancelled, abort/return from
the enclosing function to avoid removing the wallet or using an empty
passphrase.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9fb1eb2e-5432-4d6c-90ba-c586dae44966
📒 Files selected for processing (6)
src/interfaces/wallet.hsrc/qt/askpassphrasedialog.cppsrc/qt/bitcoingui.cppsrc/qt/bitcoingui.hsrc/qt/walletcontroller.cppsrc/qt/walletcontroller.h
| connect(m_migrate_wallet_action, &QAction::triggered, [this] { | ||
| auto activity = new MigrateWalletActivity(m_wallet_controller, this); | ||
| connect(activity, &MigrateWalletActivity::migrated, this, &BitcoinGUI::setCurrentWallet); | ||
| activity->migrate(walletFrame->currentWalletModel()); | ||
| }); |
There was a problem hiding this comment.
Propagate migrated wallet selection to RPC console as well.
Migration updates the GUI current wallet, but not the RPC console context (unlike open/create/restore flows).
Suggested fix
connect(m_migrate_wallet_action, &QAction::triggered, [this] {
auto activity = new MigrateWalletActivity(m_wallet_controller, this);
connect(activity, &MigrateWalletActivity::migrated, this, &BitcoinGUI::setCurrentWallet);
+ connect(activity, &MigrateWalletActivity::migrated, rpcConsole, &RPCConsole::setCurrentWallet);
activity->migrate(walletFrame->currentWalletModel());
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| connect(m_migrate_wallet_action, &QAction::triggered, [this] { | |
| auto activity = new MigrateWalletActivity(m_wallet_controller, this); | |
| connect(activity, &MigrateWalletActivity::migrated, this, &BitcoinGUI::setCurrentWallet); | |
| activity->migrate(walletFrame->currentWalletModel()); | |
| }); | |
| connect(m_migrate_wallet_action, &QAction::triggered, [this] { | |
| auto activity = new MigrateWalletActivity(m_wallet_controller, this); | |
| connect(activity, &MigrateWalletActivity::migrated, this, &BitcoinGUI::setCurrentWallet); | |
| connect(activity, &MigrateWalletActivity::migrated, rpcConsole, &RPCConsole::setCurrentWallet); | |
| activity->migrate(walletFrame->currentWalletModel()); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/qt/bitcoingui.cpp` around lines 622 - 626, After migrating a wallet via
m_migrate_wallet_action, also update the RPC console context so it matches the
GUI current wallet; modify the lambda that creates MigrateWalletActivity to,
after calling BitcoinGUI::setCurrentWallet with
walletFrame->currentWalletModel(), invoke the same RPC-console update used by
the open/create/restore flows (for example call the existing RPC console update
method such as
rpcConsole->setCurrentWalletModel(walletFrame->currentWalletModel()) or the
BitcoinGUI helper that those flows use) so the RPC console's wallet context is
switched to the migrated wallet as well.
| if (enc_status == WalletModel::EncryptionStatus::Locked || enc_status == WalletModel::EncryptionStatus::Unlocked) { | ||
| AskPassphraseDialog dlg(AskPassphraseDialog::Unlock, m_parent_widget, &passphrase); | ||
| dlg.setModel(wallet_model); | ||
| dlg.exec(); | ||
| } | ||
|
|
||
| // GUI needs to remove the wallet so that it can actually be unloaded by migration | ||
| const std::string name = wallet_model->wallet().getWalletName(); | ||
| m_wallet_controller->removeAndDeleteWallet(wallet_model); | ||
|
|
There was a problem hiding this comment.
Abort migration when passphrase entry is cancelled.
The dialog result is ignored, so canceling still removes the wallet and proceeds with migration using an empty passphrase.
Suggested fix
if (enc_status == WalletModel::EncryptionStatus::Locked || enc_status == WalletModel::EncryptionStatus::Unlocked) {
AskPassphraseDialog dlg(AskPassphraseDialog::Unlock, m_parent_widget, &passphrase);
dlg.setModel(wallet_model);
- dlg.exec();
+ if (dlg.exec() != QDialog::Accepted) return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (enc_status == WalletModel::EncryptionStatus::Locked || enc_status == WalletModel::EncryptionStatus::Unlocked) { | |
| AskPassphraseDialog dlg(AskPassphraseDialog::Unlock, m_parent_widget, &passphrase); | |
| dlg.setModel(wallet_model); | |
| dlg.exec(); | |
| } | |
| // GUI needs to remove the wallet so that it can actually be unloaded by migration | |
| const std::string name = wallet_model->wallet().getWalletName(); | |
| m_wallet_controller->removeAndDeleteWallet(wallet_model); | |
| if (enc_status == WalletModel::EncryptionStatus::Locked || enc_status == WalletModel::EncryptionStatus::Unlocked) { | |
| AskPassphraseDialog dlg(AskPassphraseDialog::Unlock, m_parent_widget, &passphrase); | |
| dlg.setModel(wallet_model); | |
| if (dlg.exec() != QDialog::Accepted) return; | |
| } | |
| // GUI needs to remove the wallet so that it can actually be unloaded by migration | |
| const std::string name = wallet_model->wallet().getWalletName(); | |
| m_wallet_controller->removeAndDeleteWallet(wallet_model); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/qt/walletcontroller.cpp` around lines 581 - 590, The AskPassphraseDialog
result is ignored, allowing cancellation to continue migration with an empty
passphrase; change the flow so the wallet removal and subsequent migration only
proceed when the dialog is accepted. After creating AskPassphraseDialog
(AskPassphraseDialog::Unlock) and setting the model
(dlg.setModel(wallet_model)), check the dialog result (e.g. if (dlg.exec() ==
QDialog::Accepted) or dlg.result() == QDialog::Accepted) and only then continue
to call m_wallet_controller->removeAndDeleteWallet(wallet_model) and perform
migration; if the dialog is rejected/cancelled, abort/return from the enclosing
function to avoid removing the wallet or using an empty passphrase.
|
Checked the new reload finding too — it looks valid in this backport: I pushed a fix branch here: https://github.com/thepastaclaw/dash/tree/tracker-1433 It keeps the prior two Qt review fixes and also makes the reload failure fall through the existing migration cleanup/restore path instead of returning a null wallet as success. Validation: |
2 participants
Issue being fixed or feature implemented
Further improvements of
migratewalletRPC to migrate legacy wallets to descriptor wallets.What was done?
Backports:
Further changes to be done in the next batch ; out-of-scope of current PR:
CMasternodeMan::CheckAndRemoveand smartfee (+test) #738), depends on walletdb: Add PrefixCursor bitcoin/bitcoin#27790How Has This Been Tested?
Run unit test & functional tests.
Click in Qt-UI "Migrate wallet" - it works as expected.
Breaking Changes
None
Checklist: