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

ReviewabilityReviewsAgreementSize
#34909 rkrux wallet, refactor: modularise wallet by extracting out legacy wallet migrationReady0Positive865 + 4 tests
  • Make core wallet code easier to maintain by separating legacy wallet migration logic
  • Isolate legacy migration flows to keep the primary wallet implementation descriptor-focused
  • Ready to review
  • Concept approval without stated reasons (pablomartin4btc, w0xlt)
  • Clarified scope and design questions around future migration types (pablomartin4btc)
  • 865 lines added or modified outside tests
  • 4 lines added or modified in tests
  • 837 lines removed in total
  • 16 files, 5 commits
Wallet#65 of 92
P3 · cleanup
  • P3 because it is an internal cleanup with no user-facing changes or correctness fixes
  • Reduces wallet maintenance complexity by separating one-off legacy upgrades from core operations
  • Ranking pass: Heads the internal refactor block; see note about conflict ordering with #34193.
Ranking notes
Reviewability: ReadyStalePaused
Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets
Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs.
Size: lines added or modified outside tests, then in tests; greener = smaller.

Goal

Extracts legacy wallet migration methods and the public MigrateLegacyToDescriptor routines out of wallet/wallet.cpp into dedicated wallet/migration.{h,cpp} files. Relocates wallet path and persistent startup setting helper functions to wallet/walletutil.{h,cpp}. Renames wallet/migrate.{h,cpp} to wallet/legacybdb.{h,cpp} to clarify that it handles legacy Berkeley DB storage rather than general migration logic.

Problem: wallet/wallet.cpp exceeds 4,500 lines and commingles legacy BDB migration logic with core descriptor wallet operations, making the primary wallet implementation difficult to read, maintain, and reason about.

Evidence of importance:

  • PR description: '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.'
  • PR description: '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.'

Scope: Largely a move-only refactoring across wallet files, including moving utility functions and renaming wallet/migrate to wallet/legacybdb.

Risk of a mistake: medium (wallet, wallet_storage). Moving wallet migration and setting persistence functions could introduce regressions in legacy wallet upgrading or startup wallet loading if symbol visibility or logic were inadvertently changed.

Changed since the previous assessment: Rebased on master to incorporate recently added migration options and settings, including moving UpdateWalletSetting to walletutil and updating MigrateLegacyToDescriptor signatures.

Reviewability

Ready

Nothing known that would invalidate a review now.

Waiting on (from the thread read):

Agreement

Positive

support without stated reasons, no open objection (pablomartin4btc, w0xlt)

Thread notes: The author rebased the branch over master to incorporate changes from #36397.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
pablomartin4btc (member)suggestion, scopeincorporate the commit from #34910 into this PRfixed · fix pushed · author cherry-picked the commit from #34910 into this PR and closed #34910answeredyes2026-06-13: 'I see you opened also #34910, wouldn't it make sense to add that commit here?'
Status from: 2026-06-15: rkrux: 'I have cherry-picked that commit here and closed that PR now in favour of this one.'
pablomartin4btc (member)question, approachwhether the structure could accommodate future migration types beyond legacy-to-descriptor, perhaps with a dedicated migration classanswered · author answered that additional migration flows are very unlikely so a dedicated class is not warrantedansweredyes2026-06-13: 'have you thought about whether this structure could accommodate future migration types beyond legacy-to-descriptor, perhaps with a dedicated migration class?'
Status from: 2026-06-15: rkrux: 'I believe the migration is more of a one-off event and the likelihood of adding other migration flows beyond legacy-to-d'

Support:

  • pablomartin4btc (member) (Concept ACK, 2026-06-13): (no reason given) [not substantive]
  • w0xlt (contributor) (Concept ACK, 2026-10-08): (no reason given) [not substantive]

Participants:

  • fanquake (member, neutral, 1 statements 2026-05-22 to 2026-05-22): posted MSVC build failure log from CI
  • pablomartin4btc (member, support, 1 statements 2026-06-13 to 2026-06-13): Concept ACK; suggested incorporating #34910 and asked about accommodating future migrations
  • w0xlt (contributor, support, 1 statements 2026-10-08 to 2026-10-08): Concept ACK

Files

6 lines under test/bench/ci.

  • src/wallet/wallet.cpp +0/-804
  • src/wallet/migration.cpp +727/-0
  • src/wallet/walletutil.cpp +89/-0
  • src/wallet/migration.h +25/-0
  • src/wallet/wallet.h +0/-25
  • src/wallet/walletutil.h +15/-0
  • src/wallet/{migrate.h => legacybdb.h} +3/-3
  • src/wallet/CMakeLists.txt +2/-1
  • src/wallet/{migrate.cpp => legacybdb.cpp} +1/-1
  • src/wallet/test/db_tests.cpp +1/-1
  • src/wallet/test/fuzz/wallet_bdb_parser.cpp +1/-1
  • src/wallet/walletdb.cpp +1/-1
  • src/bench/wallet_migration.cpp +1/-0
  • src/wallet/interfaces.cpp +1/-0
  • src/wallet/rpc/wallet.cpp +1/-0
  • src/wallet/test/wallet_tests.cpp +1/-0

Card

The short text the ranking pass compares across PRs.

Extracts approximately 800 lines of legacy Berkeley DB wallet migration code out of wallet.cpp into a dedicated wallet/migration module. Relocates wallet path and startup setting helpers into walletutil, and renames wallet/migrate to wallet/legacybdb to clarify its purpose. The change is a move-only refactor aimed at reducing the size and complexity of wallet.cpp without modifying runtime behavior.

Data

dossier JSON · extract JSON · model ledger, generated 2026-10-25T01:14, confidence high, input hash a80203be1438d803
thread read through 2026-10-23 (26 statements), head 60136a68ba
labels: none

Categories

Wallet

P3 · cleanup (ranking pass: Heads the internal refactor block; see note about conflict ordering with #34193.)

Extracts around 800 lines of legacy BDB migration logic from wallet.cpp into separate files to improve modularity. The change provides clear maintainability value by separating legacy migration from core descriptor wallet operations, but it is an internal refactoring without user-facing or correctness impact.

Membership: Extensively refactors wallet internals across src/wallet/, moving legacy migration logic out of wallet.cpp into dedicated migration files.

Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 1