wallet, refactor: modularise wallet by extracting out legacy wallet migration #34909

I've been meaning to scratch this itch for quite some time. It has been noted by many developers that the wallet/wallet.cpp file is quite hard to reason about.

It is more than 4500 lines long currently and this patch is an attempt to shortern it by around 780 lines by extracting out the wallet migration code into a dedicated wallet/migration.cpp file.

The other benefits of this PR I see are that it moves the legacy wallet stuff from the main wallet file that paves the way for it being descriptor specific (as much as possible), and makes legacy wallet migration code a first-class citizen in its own file.

I hope it makes it easier for everyone to go through the main wallet file.

This patchset should be reviewed with --color-moved=dimmed-zebra option.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34909.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #35501 (wallet: store all witness variants of a transaction by achow101)
• #35436 (wallet: Add addHDkey interface by pseudoramdom)
• #35266 (rpc, wallet: add an option to not load the wallet after migrating by polespinasa)
• #34603 (wallet: Fix detection of symlinks on Windows by achow101)
• #33034 (wallet: Store transactions in a separate sqlite table by achow101)
• #32895 (wallet: Prepare for future upgrades by recording versions of last client to open and decrypt by achow101)
• #32489 (wallet: Add exportwatchonlywallet RPC to export a watchonly version of a wallet by achow101)
• #27865 (wallet: Track no-longer-spendable TXOs separately by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

• WalletDescriptor w_desc(std::move(descs.at(0)), creation_time, 0, 0, 0) in src/wallet/migration.cpp
• AddWalletDescriptor(w_desc, keys, "", false) in src/wallet/migration.cpp
• WalletDescriptor w_desc(std::move(descs.at(0)), creation_time, 0, 0, 0) in src/wallet/migration.cpp
• AddWalletDescriptor(w_desc, keys, "", false) in src/wallet/migration.cpp

^{2026-06-15 14:44:55}

Fixed two things:

• the linter issue where the trailing new line at the end of the file was missing.
• forgot to update the migration portion in bench for which I didn't get an error in local, checking why.

Pushed an update where three more migration specific CWallet methods are extracted out.

Rebased over master from 8723a4f to ecaef3d.

<!--85328a0da195eb286784d51f73fa0af9-->

🚧 At least one of the CI tasks failed. _{Task Windows native, fuzz, VS: https://github.com/bitcoin/bitcoin/actions/runs/24783607828/job/72521322210} _{LLM reason (✨ experimental): CI failed because the spkm_migration fuzz target hit a wallet database format assertion (m_database->Format() != "bdb_ro" && != "mock") in wallet::CWallet::SetupLegacyScriptPubKeyMan and exited with code 1.}

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

• Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

• A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

• An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

Force pushed to fix the CI failures because of incorrectly resolving conflicts during rebase due to the changes in this file of this PR.

Split the diff into multiple commits to aid review from 7b87334 to e9f50db.

<!--85328a0da195eb286784d51f73fa0af9-->

🚧 At least one of the CI tasks failed. _{Task lint: https://github.com/bitcoin/bitcoin/actions/runs/24828329553/job/72670043902} _{LLM reason (✨ experimental): CI failed because the trailing_newline lint check reported a missing trailing newline in src/wallet/migration.h.}

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

• Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

• A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

• An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

Fixed lint failure due to missing new line in wallet/migration.h.

Rebased over master to incorporate changes from #34544.

Rebased over master to incorporate changes from #34806.

 Build FAILED.

"D:\a\bitcoin\bitcoin\build\ALL_BUILD.vcxproj" (default target) (1) ->
"D:\a\bitcoin\bitcoin\build\src\bench\bench_bitcoin.vcxproj" (default target) (4) ->
"D:\a\bitcoin\bitcoin\build\src\wallet\bitcoin_wallet.vcxproj" (default target) (12) ->
(ClCompile target) -> 
  D:\a\bitcoin\bitcoin\src\wallet\walletutil.cpp(78,14): error C2027: use of undefined type 'UniValue' [D:\a\bitcoin\bitcoin\build\src\wallet\bitcoin_wallet.vcxproj]
  D:\a\bitcoin\bitcoin\src\wallet\walletutil.cpp(78,39): error C2027: use of undefined type 'UniValue' [D:\a\bitcoin\bitcoin\build\src\wallet\bitcoin_wallet.vcxproj]
  D:\a\bitcoin\bitcoin\src\wallet\walletutil.cpp(79,34): error C2027: use of undefined type 'UniValue' [D:\a\bitcoin\bitcoin\build\src\wallet\bitcoin_wallet.vcxproj]
  D:\a\bitcoin\bitcoin\src\wallet\walletutil.cpp(79,26): error C2530: 'value': references must be initialized [D:\a\bitcoin\bitcoin\build\src\wallet\bitcoin_wallet.vcxproj]
  D:\a\bitcoin\bitcoin\src\wallet\walletutil.cpp(79,26): error C3531: 'value': a symbol whose type contains 'auto' must have an initializer [D:\a\bitcoin\bitcoin\build\src\wallet\bitcoin_wallet.vcxproj]
  D:\a\bitcoin\bitcoin\src\wallet\walletutil.cpp(79,32): error C2143: syntax error: missing ';' before ':' [D:\a\bitcoin\bitcoin\build\src\wallet\bitcoin_wallet.vcxproj]
  D:\a\bitcoin\bitcoin\src\wallet\walletutil.cpp(79,59): error C2143: syntax error: missing ';' before ')' [D:\a\bitcoin\bitcoin\build\src\wallet\bitcoin_wallet.vcxproj]
  D:\a\bitcoin\bitcoin\src\wallet\walletutil.cpp(82,9): error C2027: use of undefined type 'UniValue' [D:\a\bitcoin\bitcoin\build\src\wallet\bitcoin_wallet.vcxproj]
  D:\a\bitcoin\bitcoin\src\wallet\walletutil.cpp(91,14): error C2027: use of undefined type 'UniValue' [D:\a\bitcoin\bitcoin\build\src\wallet\bitcoin_wallet.vcxproj]
  D:\a\bitcoin\bitcoin\src\wallet\walletutil.cpp(92,64): error C2027: use of undefined type 'UniValue' [D:\a\bitcoin\bitcoin\build\src\wallet\bitcoin_wallet.vcxproj]
  D:\a\bitcoin\bitcoin\src\wallet\walletutil.cpp(92,31): error C2079: 'new_value' uses undefined class 'UniValue' [D:\a\bitcoin\bitcoin\build\src\wallet\bitcoin_wallet.vcxproj]
  D:\a\bitcoin\bitcoin\src\wallet\walletutil.cpp(92,64): error C2065: 'VARR': undeclared identifier [D:\a\bitcoin\bitcoin\build\src\wallet\bitcoin_wallet.vcxproj]
  D:\a\bitcoin\bitcoin\src\wallet\walletutil.cpp(93,34): error C2027: use of undefined type 'UniValue' [D:\a\bitcoin\bitcoin\build\src\wallet\bitcoin_wallet.vcxproj]
  D:\a\bitcoin\bitcoin\src\wallet\walletutil.cpp(93,26): error C2530: 'value': references must be initialized [D:\a\bitcoin\bitcoin\build\src\wallet\bitcoin_wallet.vcxproj]
  D:\a\bitcoin\bitcoin\src\wallet\walletutil.cpp(93,26): error C3531: 'value': a symbol whose type contains 'auto' must have an initializer [D:\a\bitcoin\bitcoin\build\src\wallet\bitcoin_wallet.vcxproj]
  D:\a\bitcoin\bitcoin\src\wallet\walletutil.cpp(93,32): error C2143: syntax error: missing ';' before ':' [D:\a\bitcoin\bitcoin\build\src\wallet\bitcoin_wallet.vcxproj]
  D:\a\bitcoin\bitcoin\src\wallet\walletutil.cpp(93,59): error C2143: syntax error: missing ';' before ')' [D:\a\bitcoin\bitcoin\build\src\wallet\bitcoin_wallet.vcxproj]
  D:\a\bitcoin\bitcoin\src\wallet\walletutil.cpp(96,33): error C2027: use of undefined type 'UniValue' [D:\a\bitcoin\bitcoin\build\src\wallet\bitcoin_wallet.vcxproj]
  D:\a\bitcoin\bitcoin\src\wallet\walletutil.cpp(97,23): error C2582: 'operator =' function is unavailable in 'UniValue' [D:\a\bitcoin\bitcoin\build\src\wallet\bitcoin_wallet.vcxproj]

Rebased over master to incorporate changes from #28333.

Concept ACK

I see you opened also #34910, wouldn't it make sense to add that commit here?

As a future consideration, have you thought about whether this structure could accommodate future migration types beyond legacy-to-descriptor, perhaps with a dedicated migration class? Curious if that's something worth exploring as a follow-up.

I see you opened also #34910, wouldn't it make sense to add that commit here?

I had raised it separately because that change in itself seemed fine as well and I didn't want the changes of this PR to hold merging that PR. But given that #34910 has not been merged yet, I have cherry-picked that commit here and closed that PR now in favour of this one.

As a future consideration, have you thought about whether this structure could accommodate future migration types beyond legacy-to-descriptor, perhaps with a dedicated migration class? Curious if that's something worth exploring as a follow-up.

I believe the migration is more of a one-off event and the likelihood of adding other migration flows beyond legacy-to-descriptor is very low, so I don't think it's worth introducing a dedicated migration class at this stage.

Rebased over master to incorporate changes from #35414.

<!--85328a0da195eb286784d51f73fa0af9-->

🚧 At least one of the CI tasks failed. _{Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/27554400650/job/81454235475} _{LLM reason (✨ experimental): CI failed because IWYU reported a failure (“Failure generated from IWYU”) due to incorrect/missing #include ordering/content in src/bench/pool.cpp.}

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

• Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

• A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

• An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

The CI / iwyu (pull_request) failure is unrelated and should have been fixed post #35535.