Skip to content

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

Open
rkrux wants to merge 5 commits into
bitcoin:masterfrom
rkrux:wallet-migration
Open

wallet, refactor: modularise wallet by extracting out legacy wallet migration#34909
rkrux wants to merge 5 commits into
bitcoin:masterfrom
rkrux:wallet-migration

Conversation

@rkrux

@rkrux rkrux commented Mar 24, 2026

Copy link
Copy Markdown
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 800 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.

@DrahtBot

DrahtBot commented Mar 24, 2026

Copy link
Copy Markdown
Contributor

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

Code Coverage & Benchmarks

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

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK pablomartin4btc

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

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)
  • #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)
  • #30343 (wallet, logging: Replace WalletLogPrintf() with LogInfo() by ryanofsky)
  • #29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)
  • #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.

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(..., 0, 0, 0) in src/wallet/migration.cpp
  • AddWalletDescriptor(w_desc, keys, "", false) in src/wallet/migration.cpp
  • AddWalletDescriptor(w_desc, keys, "", false) in src/wallet/migration.cpp

2026-06-25 09:31:53

@rkrux

rkrux commented Mar 24, 2026

Copy link
Copy Markdown
Contributor Author

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.

@rkrux rkrux force-pushed the wallet-migration branch from d41f17f to 1b3fc52 Compare March 25, 2026 11:01
@rkrux rkrux changed the title wallet, refactor: extract migration portion from the wallet file wallet, refactor: modularise wallet by extracting out legacy wallet migration Mar 25, 2026
@DrahtBot DrahtBot changed the title wallet, refactor: modularise wallet by extracting out legacy wallet migration wallet, refactor: modularise wallet by extracting out legacy wallet migration Mar 25, 2026
@rkrux

rkrux commented Mar 25, 2026

Copy link
Copy Markdown
Contributor Author

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

@rkrux rkrux force-pushed the wallet-migration branch from 9860208 to 5447bdf Compare May 29, 2026 11:15
@rkrux

rkrux commented May 29, 2026

Copy link
Copy Markdown
Contributor Author

Rebased over master to incorporate changes from #28333.

@pablomartin4btc pablomartin4btc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rkrux

rkrux commented Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

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.

@rkrux

rkrux commented Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

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.

@rkrux

rkrux commented Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

Rebased over master to incorporate changes from #35414.

@DrahtBot

Copy link
Copy Markdown
Contributor

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

Hints

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.

@rkrux

rkrux commented Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

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

@rkrux

rkrux commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Rebased over master to incorporate changes from PR #35266.

rkrux added 5 commits June 25, 2026 15:00
From wallet/wallet.cpp to wallet/migration.cpp.

This patch should be reviewed with --color-moved=dimmed-zebra option.
To wallet/walletutil.h

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

This patch should be reviewed with --color-moved=dimmed-zebra option.
To wallet/migration. Also, update the includes in the call sites.

This patch should be reviewed with --color-moved=dimmed-zebra.
To wallet/migration.

This patch should be reviewed with --color-moved=dimmed-zebra.
The wallet/migrate.h|cpp file contain all the code related to the
Berkeley DB that was used in the legacy wallets. Although it's true
that this code is used only within the context of wallet migration
but the name of the file is not exact.

This patch renames the file to legacybdb so that the intent is clearer in the
call sites. I felt this rename might be necessary so that the name
doesn't conflict with the new migration.cpp introduced in the preceding commits.
@rkrux

rkrux commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Force-pushed to emphasise in the commit messages that most of the commits should be reviewed with the --color-moved=dimmed-zebra git option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants