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

pull rkrux wants to merge 4 commits into bitcoin:master from rkrux:wallet-migration changing 11 files +817 −782
  1. rkrux commented at 1:00 PM on March 24, 2026: contributor

    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 can be reviewed with --color-moved=dimmed-zebra option.

    <!-- *** Please remove the following help text before submitting: *** Pull requests without a rationale and clear improvement may be closed immediately. GUI-related pull requests should be opened against https://github.com/bitcoin-core/gui first. See CONTRIBUTING.md -->

    <!-- Please provide clear motivation for your patch and explain how it improves Bitcoin Core user experience or Bitcoin Core developer experience significantly: * Any test improvements or new tests that improve coverage are always welcome. * All other changes should have accompanying unit tests (see `src/test/`) or functional tests (see `test/`). Contributors should note which tests cover modified code. If no tests exist for a region of modified code, new tests should accompany the change. * Bug fixes are most welcome when they come with steps to reproduce or an explanation of the potential issue as well as reasoning for the way the bug was fixed. * Features are welcome, but might be rejected due to design or scope issues. If a feature is based on a lot of dependencies, contributors should first consider building the system outside of Bitcoin Core, if possible. * Refactoring changes are only accepted if they are required for a feature or bug fix or otherwise improve developer experience significantly. For example, most "code style" refactoring changes require a thorough explanation why they are useful, what downsides they have and why they *significantly* improve developer experience or avoid serious programming bugs. Note that code style is often a subjective matter. Unless they are explicitly mentioned to be preferred in the [developer notes](/doc/developer-notes.md), stylistic code changes are usually rejected. -->

    <!-- Bitcoin Core has a thorough review process and even the most trivial change needs to pass a lot of eyes and requires non-zero or even substantial time effort to review. There is a huge lack of active reviewers on the project, so patches often sit for a long time. -->

  2. DrahtBot commented at 1:01 PM on March 24, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process. A summary of reviews will appear here.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #34910 (wallet, refactor: rename file from migrate to legacybdb by rkrux)
    • #34838 (interfaces: typed receive-request APIs by MkDev11)
    • #34806 (refactor: logging: Various API improvements by ajtowns)
    • #34603 (wallet: Fix detection of symlinks on Windows by achow101)
    • #34544 (wallet: Disallow wallet names that are paths including .. and . elements by achow101)
    • #34490 (wallet: Follow-ups to create/load split (#32636) by davidgumberg)
    • #34198 (wallet: fix ancient wallets migration by furszy)
    • #34193 (wallet: make migration more robust against failures by furszy)
    • #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)
    • #28333 (wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively by achow101)
    • #27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
    • #26022 (Add util::ResultPtr class by ryanofsky)
    • #25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
    • #25665 (refactor: Add util::Result failure types and ability to merge result values by ryanofsky)

    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 typos and grammar issues:

    • a multipath descriptors -> a multipath descriptor [grammar error; the singular/plural mismatch breaks the sentence]
    • a multipath descriptors -> a multipath descriptor [same typo appears again in the repeated comment]

    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(std::move(descs.at(0)), creation_time, 0, 0, 0) in src/wallet/migration.cpp (watchonly wallet creation)
    • AddWalletDescriptor(w_desc, keys, "", false) in src/wallet/migration.cpp (watchonly wallet creation)
    • WalletDescriptor(std::move(descs.at(0)), creation_time, 0, 0, 0) in src/wallet/migration.cpp (solvable wallet creation)
    • AddWalletDescriptor(w_desc, keys, "", false) in src/wallet/migration.cpp (solvable wallet creation)

    <sup>2026-04-23 09:51:37</sup>

  3. rkrux force-pushed on Mar 24, 2026
  4. DrahtBot added the label CI failed on Mar 24, 2026
  5. rkrux force-pushed on Mar 24, 2026
  6. rkrux commented at 1:12 PM on March 24, 2026: contributor

    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.
  7. DrahtBot removed the label CI failed on Mar 24, 2026
  8. rkrux force-pushed on Mar 25, 2026
  9. rkrux renamed this:
    wallet, refactor: extract migration portion from the wallet file
    wallet, refactor: modularise wallet by extracting out legacy wallet migration
    on Mar 25, 2026
  10. DrahtBot renamed this:
    wallet, refactor: modularise wallet by extracting out legacy wallet migration
    wallet, refactor: modularise wallet by extracting out legacy wallet migration
    on Mar 25, 2026
  11. rkrux commented at 11:24 AM on March 25, 2026: contributor

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

  12. rkrux force-pushed on Mar 25, 2026
  13. DrahtBot added the label CI failed on Mar 25, 2026
  14. DrahtBot removed the label CI failed on Mar 25, 2026
  15. DrahtBot added the label Needs rebase on Apr 19, 2026
  16. rkrux force-pushed on Apr 22, 2026
  17. rkrux commented at 2:21 PM on April 22, 2026: contributor

    Rebased over master from 8723a4f to ecaef3d.

  18. rkrux force-pushed on Apr 22, 2026
  19. DrahtBot added the label CI failed on Apr 22, 2026
  20. DrahtBot commented at 2:47 PM on April 22, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task Windows native, fuzz, VS: https://github.com/bitcoin/bitcoin/actions/runs/24783607828/job/72521322210</sub> <sub>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.</sub>

    <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>

  21. rkrux commented at 2:48 PM on April 22, 2026: contributor

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

  22. DrahtBot removed the label Needs rebase on Apr 23, 2026
  23. DrahtBot removed the label CI failed on Apr 23, 2026
  24. wallet: (move-only) extract out migration specific CWallet methods
    From wallet/wallet.cpp to wallet/migration.cpp.

This patch can be reviewed with --color-moved=dimmed-zebra option.
    6350145546
  25. wallet: extract out utility functions from wallet/wallet
    To wallet/walletutil.h

Couple of functions had to be made non-static for them to be used by
wallet/wallet.

This patch can be reviewed with --color-moved=dimmed-zebra option.
    9ace3f7d69
  26. rkrux force-pushed on Apr 23, 2026
  27. rkrux commented at 9:43 AM on April 23, 2026: contributor

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

  28. wallet: extract out public facing MigrateLegacyToDescriptor functions
    To wallet/migration. Also, update the includes in the call sites.

This patch can be reviewed with --color-moved=dimmed-zebra.
    64693790b1
  29. wallet: extract out migration specific LegacySPKM CWallet methods
    To wallet/migration.

This patch can be reviewed with --color-moved=dimmed-zebra.
    fa7fb5a09f
  30. rkrux force-pushed on Apr 23, 2026
  31. DrahtBot added the label CI failed on Apr 23, 2026
  32. DrahtBot commented at 9:52 AM on April 23, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

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

    <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>

  33. rkrux commented at 9:52 AM on April 23, 2026: contributor

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

  34. DrahtBot removed the label CI failed on Apr 23, 2026
Contributors
rkruxDrahtBot
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-24 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me