#25722 refactor: Use util::Result class for wallet loading
https://github.com/bitcoin/bitcoin/pull/25722 · · +1397/-773 in 44 files, 25 commits · labels: Refactoring, Needs rebase · draft
Goal
- Standardize error and warning handling across wallet loading and initialization
- Reduce boilerplate and avoid mishandling errors by removing inconsistent out-parameters
This pull request refactors wallet loading, creation, and migration functions throughout the wallet subsystem to uniformly return util::Result and util::ResultPtr. It removes disparate out-parameters for database status enums, bilingual error strings, and warning vectors in favor of standardized structured result types.
Problem: Wallet loading functions across the stack handle errors and warnings inconsistently, often passing multiple mutable out-parameters for status, errors, and warnings. This causes boilerplate and increases the chance of mishandling error states during wallet initialization.
Category: Wallet (#54 of 84)
P3 · cleanup
- P3 because it is an internal cleanup of wallet error propagation without changing behavior
- Reduces maintenance overhead across wallet initialization and migration without direct user impact
P3 because it standardizes wallet loading, creation, and migration error and warning handling onto util::Result, removing cumbersome out-parameters across the wallet subsystem. The author notes that 'Wallet loading functions up and down the stack have lots of error and warning parameters, and return error information in different ways. This PR makes them uniformly return util::Result, without changing behavior.' While a clear code quality improvement, it is an internal refactoring without direct user impact.
Membership: Refactors wallet initialization, creation, migration, and loading functions across src/wallet/.
Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 1
Reviewability: Stale: Needs rebase
The PR has merge conflicts with master and is stacked on open base PRs #25665 and #26022.
Author status: active, periodically rebasing against base PRs and master conflicts
Resolved concerns:
- martinus noted compiler errors on rebase and suggested multiple API improvements to util::Result recursive construction and size assertions, which the author adopted.
Agreement: Positive
- Positive feedback on the result helper improvements (martinus)
- Requested API improvements and compiler fixes were addressed by the author
- No reviews yet on the wallet loading changes themselves
Positive feedback on util::Result changes from martinus with requests addressed; no reviews yet on wallet loading refactor.
martinus gave code review on the underlying util::Result machinery and approved of its direction once nits were resolved. The wallet-specific commits have not yet received formal review comments.
- martinus noted 'This makes util::Result quite a bit more powerful, and it's still simple to use' while requesting minor cleanups.
- ryanofsky addressed all review suggestions from martinus and updated the PR accordingly.
Review verdicts (DrahtBot): 0
Dependencies
Depends on: #25665, #26022
Files
342 lines under test/bench/ci.
- src/wallet/wallet.cpp +270/-254
- src/util/result.h +399/-62
- src/test/result_tests.cpp +230/-11
- src/wallet/dump.cpp +49/-54
- src/node/chainstate.cpp +56/-39
- src/wallet/sqlite.cpp +37/-33
- src/wallet/interfaces.cpp +32/-32
- src/wallet/wallettool.cpp +27/-28
- src/util/messages.h +53/-0
- src/wallet/load.cpp +22/-25
- src/qt/walletcontroller.cpp +29/-16
- src/init.cpp +22/-21
- src/wallet/rpc/wallet.cpp +20/-19
- src/wallet/walletdb.cpp +12/-27
- src/util/messages.cpp +30/-0
- src/node/chainstate.h +6/-22
- src/wallet/wallet.h +12/-13
- src/wallet/rpc/util.cpp +11/-11
- src/wallet/test/util.cpp +7/-15
- src/bench/wallet_create.cpp +5/-10
- src/bench/wallet_loading.cpp +7/-7
- src/kernel/bitcoinkernel.cpp +7/-7
- src/wallet/rpc/backup.cpp +5/-7
- src/wallet/test/db_tests.cpp +3/-9
- src/wallet/export.cpp +6/-5
- src/wallet/migrate.cpp +3/-7
- src/wallet/test/fuzz/wallet_bdb_parser.cpp +3/-6
- src/wallet/test/wallet_tests.cpp +3/-6
- src/test/util/setup_common.cpp +4/-4
- src/interfaces/wallet.h +3/-3
- src/wallet/db.h +3/-3
- src/wallet/test/walletload_tests.cpp +2/-4
- src/wallet/dump.h +3/-2
- src/wallet/rpc/util.h +3/-2
- src/wallet/sqlite.h +3/-2
- src/wallet/migrate.h +2/-1
- .github/workflows/ci.yml +1/-1
- ci/test/00_setup_env_native_previous_releases.sh +1/-1
- src/addrdb.cpp +1/-1
- src/addrdb.h +1/-1
- src/qt/test/addressbooktests.cpp +1/-1
- src/qt/test/wallettests.cpp +1/-1
- src/kernel/CMakeLists.txt +1/-0
- src/util/CMakeLists.txt +1/-0
Card
This pull request refactors wallet loading, creation, and migration functions across the wallet stack to return util::Result and util::ResultPtr rather than using separate output parameters for status, errors, and warnings. The change improves internal code consistency and error handling across wallet initialization without altering observable behavior. While it provides substantial cleanup across wallet entry points, it is currently in draft and needs a rebase over substantial conflicts. The PR also depends on base PRs #25665 and #26022 which enhance util::Result capabilities.