backport: partial bitcoin#28868 (wallet: reload wallet if migration exited early, keep mixed watchonly txs)

[PastaPastaPasta]

Issue being fixed or feature implemented

migratewallet (and the GUI "Migrate wallet" action, which calls the same MigrateLegacyToDescriptor()) has two bugs that upstream fixed in bitcoin#28868. That PR is listed as outstanding in the "next batch" section of #7277.

1. A failed migration leaves the wallet unloaded. If the wallet is loaded, MigrateLegacyToDescriptor() unloads it first and then runs its checks. Several of those checks return early without loading it again: the wallet is already a descriptor wallet, the backup could not be written, or the passphrase is missing or wrong. After any of these errors the wallet is gone from listwallets, and RPC calls to it fail with Requested wallet does not exist or is not loaded until the user runs loadwallet manually. Mistyping the passphrase once on an encrypted legacy wallet is enough to trigger this.
2. A transaction with outputs in both resulting wallets only stays in the migrated wallet. During migration, ApplyMigrationData() only offers a transaction to the new <name>_watchonly wallet when the migrated wallet does not claim it. If one output pays a spendable address and another pays an imported watch-only address, the transaction stays in the migrated wallet and is never copied to <name>_watchonly. That wallet is created at the chain tip, so it does not rescan, and its history and balance silently omit that transaction. This is the watch-only bug mentioned in feat: new RPC migratewallet to migrate legacy wallets to backport wallets #7275.

Why this is a real problem

Code path at c14104b:

• The loaded wallet is unloaded before any validation: wallet.cpp#L5006-L5012
• These early returns do not reload it: "already a descriptor wallet" #L5033-L5035, backup failure #L5055-L5057, missing or wrong passphrase #L5062-L5075
• Watch-only copying only runs when !IsMine(tx) && !IsFromMe(tx): wallet.cpp#L4759-L4781

I reproduced all three cases against an unfixed dashd built from c14104b, using a functional-test-style script on regtest:

Reproduction on c14104b (unfixed)

Steps:

1. createwallet desc (descriptor), then migratewallet desc, then listwallets
2. createwallet enc descriptors=false, then encryptwallet pass, then migratewallet enc badpass, then listwallets and getwalletinfo on enc
3. createwallet imports descriptors=false, then importaddress <addr from default wallet>. From the default wallet, send one output to an imports address and one to the imported address, then mine a block. Then migratewallet and gettransaction <txid> on imports_watchonly

TestFramework (INFO): Case 1: migratewallet on a loaded descriptor wallet
TestFramework (INFO): listwallets before: ['default_wallet', 'desc']
TestFramework (INFO): listwallets after:  ['default_wallet']
TestFramework (INFO): Case 2: migratewallet on a loaded encrypted legacy wallet with a wrong passphrase
TestFramework (INFO): listwallets before: ['default_wallet', 'enc']
TestFramework (INFO): listwallets after:  ['default_wallet']
TestFramework (INFO): getwalletinfo on 'enc' -> RPC error -18 (wallet not loaded)
TestFramework (INFO): Case 3: tx with a spendable output and a watch-only output
TestFramework (INFO): imports_watchonly.gettransaction(cfbc39814ffc474a35bae511c660de7a759653f3a362639f08e38fe2bd6382ef) -> Invalid or non-wallet transaction id (-5)
TestFramework (INFO): RESULT descriptor wallet still loaded:          False
TestFramework (INFO): RESULT encrypted wallet still loaded:           False
TestFramework (INFO): RESULT mixed tx present in watchonly wallet:    False

With this branch, the same script reports listwallets after: ['default_wallet', 'desc'] and ['default_wallet', 'desc', 'enc'], and all three results are True.

The upstream tests backported here also fail on the unfixed code (see "How Has This Been Tested?").

Why it matters

Neither bug loses funds or keys. The first one is a usability trap in a v24 feature: a typo in the passphrase makes the wallet disappear from the node, the GUI and RPC clients, with no hint that a loadwallet is needed. The second one leaves the post-migration watch-only wallet with incomplete history and balance, and nothing reports it. Users migrating real wallets have already hit it (see #7275).

What was done?

Backport of bitcoin#28868, minus one commit:

• bitcoin/bitcoin commit 78ba0e6748d2 "wallet: Reload the wallet if migration exited early": remember whether the wallet was loaded (was_loaded), define the reload_wallet helper before the early exits, and reload the wallet on the descriptor-wallet, backup-failure and passphrase-failure exits. As upstream does, the passphrase unlock moves out of the LOCK(cs_wallet) scope so the reload does not happen while the wallet lock is held.
• bitcoin/bitcoin commit 71cb28ea8cb5 "test: Make sure that migration test does not rescan on reloading": adds the migrate_wallet() helper, which reloads the wallet before migrating and checks that migration does not log Rescanning. Because the helper calls getwalletinfo on the wallet first, it fails if an earlier failed migratewallet left the wallet unloaded. The Dash-only test_wallet_name_with_slashes case also goes through the helper.
• bitcoin/bitcoin commit c62a8d03a862 "wallet: Keep txs that belong to both watchonly and migrated wallets": every transaction is offered to the watch-only wallet. It is removed from the migrated wallet only if the migrated wallet does not also own it.
• bitcoin/bitcoin commit 4da76ca24725 "test: Test migration of tx with both spendable and watchonly"

Dash adaptations and omitted hunks:

• Omitted bitcoin/bitcoin commit 9332c7edda79 "wallet: Write bestblock to watchonly and solvable wallets". Upstream needs it because, after wallet: Reload watchonly and solvables wallets after migration bitcoin/bitcoin#28609, the watch-only and solvable wallets are created in an empty context and reloaded at the end of migration, and without a best block record they rescan on that reload. Dash has not backported wallet: Reload watchonly and solvables wallets after migration bitcoin/bitcoin#28609. Here those wallets are created with CreateWallet(context, ...), which already writes the chain tip as their best block on first run, and they are not reloaded during migration. The commit fixes nothing on Dash today and belongs with a wallet: Reload watchonly and solvables wallets after migration bitcoin/bitcoin#28609 backport.
• 78ba0e6: Dash's "already a descriptor wallet" check (!GetLegacyScriptPubKeyMan()) and Dash's backup filename logic are kept as they are. The upstream hunk that moves reload_wallet out of the success branch does not apply, because that helper came from wallet: Reload watchonly and solvables wallets after migration bitcoin/bitcoin#28609. The success path keeps its existing direct reload.
• c62a8d0: the watch-only copy still uses AddToWallet(), because wallet: bugfix, disallow migration of invalid scripts bitcoin/bitcoin#28125 (LoadToWallet()/CopyFrom() and the shared watch-only WalletBatch) is not backported. Only the ownership logic changes, exactly as upstream.
• 71cb28e: the upstream hunk that routes test_addressbook through self.migrate_wallet() is not carried over. That scenario was added by wallet: address book migration bug fixes bitcoin/bitcoin#28038, which Dash has not backported, so there is no test_addressbook to change. Every other scenario that upstream routes through the helper does so here as well.
• 4da76ca: applied to Dash's existing test_other_watchonly. The upstream context lines that check copied tx metadata come from wallet: bugfix, disallow migration of invalid scripts bitcoin/bitcoin#28125 and are not part of this change.

Why this is the correct minimal fix

The wallet goes missing because the unload happens before the checks, while only the success path reloads it. Reloading on each early exit is the upstream fix, and every exit taken after the unload but before any database change is covered. Moving the checks ahead of the unload would mean running the backup and the passphrase check against the loaded instance, which is a larger restructuring than this fix needs. As upstream does, no reload is attempted when MakeWalletDatabase()/CWallet::Create() fail (the same open would fail again) or when MigrateToSQLite() fails (by then the original BDB file may already have been removed, and the failed-migration restore path does not run). The .legacy.bak file written before a wrong-passphrase exit is also kept, as upstream does. The watch-only change is the upstream one-condition fix, and the rest of bitcoin#28609/bitcoin#28125 is not needed for either bug.

The migratewallet RPC first shipped in v24.0.0-rc.1 (#7275/#7277 are on v24.0.x), so this is a candidate for v24.0.x.

How Has This Been Tested?

macOS arm64, --enable-debug --enable-werror, built dashd/dash-cli/dash-wallet at each step.

• Reproduction script above: on c14104b it fails with all three bugs present. With this branch it passes.
• test/functional/wallet_migration.py with only the 71cb28e test change, against the unfixed c14104b binary: fails in test_encrypted. After the expected wrong-passphrase errors, migrate_wallet() → getwalletinfo raises Requested wallet does not exist or is not loaded (-18). All earlier migrations pass the no-Rescanning check.
• Same test after 78ba0e6: passes.
• wallet_migration.py with the 4da76ca test change, against a binary that has 78ba0e6 but not c62a8d0: fails at watchonly.gettransaction(watchonly_spendable_txid) with Invalid or non-wallet transaction id (-5). The new listtransactions counts before migration (6) and in the migrated wallet (2) already pass at that point.
• Full branch: test/functional/test_runner.py wallet_migration.py passes.
• test/lint/lint-python.py and test/lint/lint-whitespace.py: clean. clang-format-diff only suggests re-wrapping the passphrase error strings. They keep upstream's layout to stay 1:1, and wallet.cpp is not in non-backported.txt.

Breaking Changes

None. After a failed migratewallet, a wallet that was loaded beforehand is now loaded again, as it was before the call.

Checklist:

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 25 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 15c2b8df-7d51-4542-aa5e-1bd5b55a5a8f

📥 Commits

Reviewing files that changed from the base of the PR and between 8819507 and 17b0589.

📒 Files selected for processing (2)

• src/wallet/wallet.cpp
• test/functional/wallet_migration.py

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[thepastaclaw]

✅ Final review complete — no blockers (commit 17b0589)

[thepastaclaw]

Final validation — Phase 1 + Phase 2

At the exact reviewed head, the wallet fixes preserve the intended upstream behavior without an identified production correctness defect. One non-blocking upstream test transformation is missing and is not documented among the declared omissions. Verification was static: the supplied CI snapshot shows successful formatting, merge, and amd64 container checks, queued arm64 container checks, and a description-validation failure confirmed by its job log to be caused by upstream commit-reference syntax.

🟡 1 suggestion(s)

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: normal by gpt-6.1-sol (effort low) — The diff changes wallet migration error recovery and transaction assignment between migrated wallets, but its contained logic does not meet the large-or-intricate threshold for critical.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 15% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — backport-reviewer (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort high); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `test/functional/wallet_migration.py`:
- [SUGGESTION] test/functional/wallet_migration.py:54-64: Missing test prerequisite: bitcoin#28038 addressbook migration scenario
  Upstream commit 71cb28ea8cb5 also routes the migration in `test_addressbook` through `self.migrate_wallet(wallet)`, adding the no-Rescanning assertion to a scenario that creates both auxiliary wallets and checks migrated addressbook data. That scenario was introduced by 7ecc29a0b7a23d8f5d3c1e6a0dad29b3ad839eb9 in bitcoin#28038 and is absent from both the Dash base and this head, so this upstream test hunk has not been carried over. The existing multisig scenario exercises both auxiliary wallets, but it does not reproduce the addressbook assertions. This does not establish a prerequisite for either production fix, and the PR explicitly advertises a partial backport; however, its omission list does not identify this additional dropped test hunk. Add the adapted scenario and use this helper, or explicitly document the omitted transformation and its deferred test prerequisite.

[thepastaclaw]

⏭️ Deferred (maintainer decision; not blocking) at 17b05897; see the replies below.

🟡 Suggestion: Missing test prerequisite: bitcoin#28038 addressbook migration scenario

Upstream commit 71cb28e also routes the migration in test_addressbook through self.migrate_wallet(wallet), adding the no-Rescanning assertion to a scenario that creates both auxiliary wallets and checks migrated addressbook data. That scenario was introduced by 7ecc29a in bitcoin#28038 and is absent from both the Dash base and this head, so this upstream test hunk has not been carried over. The existing multisig scenario exercises both auxiliary wallets, but it does not reproduce the addressbook assertions. This does not establish a prerequisite for either production fix, and the PR explicitly advertises a partial backport; however, its omission list does not identify this additional dropped test hunk. Add the adapted scenario and use this helper, or explicitly document the omitted transformation and its deferred test prerequisite.

_{source: gpt-6.1-sol (phase2-reviewer: general, backport-reviewer, dash-core-commit-history)}

[PastaPastaPasta]

Agreed that it was undocumented. The hunk cannot be carried over because test_addressbook comes from bitcoin#28038, which is not backported, so there is nothing for it to change here. The omission is now listed under "Dash adaptations and omitted hunks" in the PR description.

---

🤖 Posted autonomously by Claude on behalf of pasta.

[thepastaclaw]

I confirmed that test_addressbook is absent from both the Dash base and this head, and the updated PR description now explicitly identifies the omitted helper transformation and its bitcoin#28038 prerequisite; that addresses my documentation concern, and I’m satisfied to leave the scenario outside this backport.

Noted as intentionally deferred; I will not press it further here.

[thepastaclaw]

Re-review after discussion — commit 17b0589

Standing review was COMMENTED; this re-review is APPROVE: no blocking findings remain.

Approved: the discussion withdrew or deferred every finding the full review of 17b05897 (run 3281) raised, and the approval rests on that review.

Same commit as the standing review; the inline threads above carry the per-finding outcome. This follow-up exists only to correct the verdict.

Review provenance

Source: conversation lane gpt-6.1-sol (agent: conversation); no reviewer or verifier lanes ran for this follow-up

• Verdict moved because the inline discussion withdrew or deferred findings of the standing review of this commit; the code was not re-reviewed

[knst]

utACK 17b0589