#25722 refactor: Use util::Result class for wallet loading

ReviewabilityReviewsAgreementSize
#25722 ryanofsky refactor: Use util::Result class for wallet loadingReview #25665 first0Strong1,156 + 280 tests
  • Standardize error reporting and propagation across wallet loading, creation, and migration
  • Eliminate cumbersome mutable out-parameters for error statuses and warning lists across the stack
  • No review verdicts recorded
  • Substantive support praising the expanded result utility as powerful yet simple to use (martinus)
  • Resolved objections regarding compile errors and ambiguous boolean type conversions (martinus, sedited)
  • 1,156 lines added or modified outside tests
  • 280 lines added or modified in tests
  • 783 lines removed in total
  • 44 files, 24 commits
Utilities (logging, arguments, libraries)#38 of 59
P3 · cleanup
  • P3 because it enhances general result utilities to support structured error types and warnings
  • Benefits maintainers with cleaner error propagation across subsystems without urgent timing
  • Ranking pass: Moved down: a behaviour-preserving refactor of wallet error plumbing, reviewable only after #25665 and #26022.
Ranking notes
Wallet#70 of 92
P3 · cleanup
  • P3 because it standardizes internal error handling across wallet loading and migration routines
  • Provides maintainers a cleaner wallet architecture without changing behavior or user-facing impact
  • Ranking pass: Moved down: behavior-preserving error-plumbing refactor, ranked with the other internal cleanups.
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

Refactors wallet loading, creation, restoration, verification, and migration across the wallet subsystem to uniformly return util::Result and util::ResultPtr. It eliminates mutable out-parameters for database status enums, bilingual error strings, and warning vectors. As part of this standardization, DatabaseStatus is renamed to DatabaseError with its unused success state removed.

Problem: Wallet initialization and loading functions across the stack handle errors inconsistently, passing multiple mutable out-parameters for status codes, error strings, and lists of warnings. This creates cumbersome function signatures and boilerplate error-propagation code across wallet internals, RPC handlers, and GUI controllers.

Evidence of importance:

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

Scope: This PR is stacked on #25665 and #26022, including commits that enhance util::Result to support error payload types, warning messages, and pointer dereferencing sugar. It also includes a scripted-diff commit replacing DatabaseStatus with DatabaseError.

Risk of a mistake: medium (wallet, wallet_storage, rpc_api). A mistake in error translation or result checking could cause wallet database loading to fail unexpectedly or return incorrect error codes via RPC.

Changed since the previous assessment: Rebased onto master to incorporate recent wallet architectural changes (such as the splitting of CWallet::Create), and updated base commits with a CI configuration workaround for GCC maybe-uninitialized compiler warnings.

Reviewability

Ready · Review #25665 first

Stacked on #25665; the base PR's changes are included in this diff.

Waiting on (from the thread read):

Agreement

Strong

substantive support, no open objection (martinus)

Thread notes: Added waiting_on tracking; author logged periodic rebase and conflict resolutions while PR remains in draft.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
martinus (contributor)suggestion, maintenanceuse a bool template argument instead of passing a lambda to simplify recursive Construct callsfixed · fix pushed · author adopted the suggestion renaming the template argument to Failure and pushed the changeansweredyes2023-04-16: 'It took me a while to figure out what's going on here with the recursive `Construct` calls and the lambda that's passed along. How about instead of passing a lambda along use a `bool` template argumen'
Status from: 2023-05-02: ryanofsky: 'Thanks, that's much better! Took suggestion jsut renaming argument to "Failure"'
martinus (contributor)suggestion, interfaceexplicitly mark move-assignment operator as deleted for clearer compiler error messagesfixed · fix pushed · author marked the operator deleted and added a commentansweredyes2023-04-16: 'Mayby explicitly mark the move-assignment as delete'
Status from: 2023-05-02: ryanofsky: 'Thanks, that changes error message from `error: object of type 'util::Result<int>' cannot be assigned because its copy a'
martinus (contributor)suggestion, maintenanceremove unnecessary check if (!src.m_info->errors.empty())answered · author explained the check avoids allocating ErrorInfo in the common error-free caseansweredyes2023-04-16: 'nit: I don't think the `if (!src.m_info->errors.empty())` check is necessary.'
Status from: 2023-05-02: ryanofsky: 'It's not strictly necessary, but it avoids allocating an ErrorInfo object in the common case where there are no errors o'
martinus (contributor)objection, correctnesscompile error in spend.cpp ChooseSelectionResult after rebasefixed · fix pushed · fixed in rebasechangedno2023-04-22: 'After rebasing I get a compile error in `spend.cpp` `ChooseSelectionResult`, adding a bunch of `std::move` helps'
Status from: 2023-07-21: ryanofsky: 'making most of the suggested changes'
martinus (contributor)objection, interfaceallowing Result<bool> is error-prone because Result converts to bool for success or failureanswered · author agreed that types like bool and optional<T> are ambiguous and should be disallowed, while clarifying pointers and ints should remain allowedansweredyes2023-04-22: 'maybe consider forbidding type `bool` for `T`. I recently had similar code in another project, and its utterly confusing; because Result can convert to bool but that only tells if it *has* a bool or n'
Status from: 2023-07-20: ryanofsky: 'I agree with all of this and want to do more work to address it.'
martinus (contributor)suggestion, correctnessadd static_assert checks to ensure sizeof(util::Result) matches expected sizefixed · fix pushed · author added static_asserts in result_tests.cppansweredyes2023-04-22: 'I'd add a check somewhere to ensure that the size of the Result is as claimed in the commit message'
Status from: 2023-05-02: ryanofsky: 'Thanks, added to result_tests.cpp'
martinus (contributor)suggestion, safetymark move constructors noexcept along with methods they useanswered · author explained conditions required for noexcept and deferred optimization for lateransweredyes2023-04-22: 'move constructors should usually be `noexcept`, but then all methods it uses also have to be `noexcept`.'
Status from: 2023-05-02: ryanofsky: 'Right now I'm just trying to do the simplest thing that's correct, and I think probably in most cases where the result c'
sedited (member)question, interfacehow are Result<std::optional<T>> and other types implementing operator bool() handledanswered · author answered agreeing that Result<optional<T>> is ambiguous and should be disallowedansweredyes2023-06-19: 'I guess the same goes for something like `Result<std::optional<int>, int>`, or really any type implementing `operator bool()`? How did you handle these cases in your code?'
Status from: 2023-07-20: ryanofsky: 'I think by default types convertible to bool should be disallowed with `Result`. I think `Result<bool>` or `Result<optio'

Support:

  • martinus (contributor) (2023-04-22) [read]: This makes util::Result quite a bit more powerful, and it's still simple to use.

Participants:

  • martinus (contributor, objection, 13 statements 2023-04-16 to 2023-04-22): reviewed code, reported a rebase compile error, warned against Result<bool>, and suggested template and test improvements
  • sedited (member, question, 1 statements 2023-06-19 to 2023-06-19): asked how types convertible to bool and Result<optional<T>> are handled
  • maflcko (member, neutral, 1 statements 2023-08-01 to 2023-08-01): suggested keeping PR in draft while CI is red and based on other PRs

Dependencies

Depends on: #25665, #26022

Based on (shares commits with): #25665, #26022

Files

361 lines under test/bench/ci.

  • src/wallet/wallet.cpp +278/-259
  • src/util/result.h +415/-62
  • src/test/result_tests.cpp +239/-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 +52/-0
  • src/wallet/load.cpp +22/-25
  • src/init.cpp +24/-21
  • src/qt/walletcontroller.cpp +29/-16
  • src/wallet/rpc/wallet.cpp +20/-19
  • src/wallet/walletdb.cpp +12/-27
  • src/util/messages.cpp +32/-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/test/walletload_tests.cpp +4/-9
  • 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/dump.h +3/-2
  • src/wallet/rpc/util.h +3/-2
  • src/wallet/sqlite.h +3/-2
  • ci/test/00_setup_env_win64.sh +2/-1
  • src/wallet/migrate.h +2/-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

The short text the ranking pass compares across PRs.

This pull request refactors wallet loading, creation, restoration, and migration functions across the wallet subsystem to uniformly return util::Result and util::ResultPtr. It removes disparate out-parameters for database status codes, bilingual error strings, and warning vectors, standardizing error reporting without changing behavior. The PR is an internal refactoring with no reported bug fixes or user-visible functional changes. It depends on base PRs #25665 and #26022 for extensions to the util::Result template.

Data

dossier JSON · extract JSON · model ledger, generated 2026-11-01T01:16, confidence high, input hash 8e69c065fc7cecf6
thread read through 2024-11-04 (31 statements), head d3ee1e6368
labels: Refactoring · draft

Categories

Utilities (logging, arguments, libraries)

P3 · cleanup (ranking pass: Moved down: a behaviour-preserving refactor of wallet error plumbing, reviewable only after #25665 and #26022.)

The PR expands util::Result capabilities to handle structured error types and warnings. While valuable for consistent error reporting across the codebase, this is an internal utility enhancement without urgent timing.

Membership: Extensively enhances src/util/result.h (+415/-62) and adds src/util/messages.* to support error payload types, warning messages, and pointer sugar.

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

Wallet

P3 · cleanup (ranking pass: Moved down: behavior-preserving error-plumbing refactor, ranked with the other internal cleanups.)

Standardizes wallet loading error handling and removes error out-parameters across the wallet stack 'without changing behavior'. It provides a cleaner internal architecture across wallet loading and migration routines, but has no direct user-facing or fund-safety impact.

Membership: Refactors wallet initialization, loading, creation, restoration, and migration throughout src/wallet/ to uniformly return util::Result.

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