Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 27, 2022

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #bitcoin-core/gui/807 (refactor: interfaces, make 'createTransaction' less error-prone by furszy)
  • #29997 (rpc: Remove index-based Arg accessor by maflcko)
  • #29817 (kernel: De-globalize fReindex by TheCharlatan)
  • #29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)
  • #29278 (Wallet: Add maxfeerate wallet startup option by ismaelsadeeq)
  • #29129 (wallet, rpc: add BIP44 account in createwallet by brunoerg)
  • #28574 (wallet: optimize migration process, batch db transactions by furszy)
  • #28333 (wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively by achow101)
  • #27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
  • #26606 (wallet: Implement independent BDB parser by achow101)
  • #26596 (wallet: Migrate legacy wallets to descriptor wallets without requiring BDB by achow101)

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.

@ryanofsky ryanofsky marked this pull request as ready for review August 15, 2022 18:18
@ryanofsky
Copy link
Contributor Author

ryanofsky commented Aug 15, 2022

Rebased a09c21c -> e7457d0 (pr/bresult-load.2 -> pr/bresult-load.3, compare) due to conflict with #25616
Rebased e7457d0 -> 874003f (pr/bresult-load.3 -> pr/bresult-load.4, compare) due to conflict with #25504
Rebased 874003f -> d020d0c (pr/bresult-load.4 -> pr/bresult-load.5, compare) on top of newer base PR #25665 tag r/bresult2.18 for derived-to-base conversion bugfix
Rebased d020d0c -> f942d92 (pr/bresult-load.5 -> pr/bresult-load.6, compare) due to silent conflict with #19602
Rebased f942d92 -> 4e53392 (pr/bresult-load.6 -> pr/bresult-load.7, compare) on top of newer version of #25665 (pr/bresult2.21)
Rebased 4e53392 -> f3d1658 (pr/bresult-load.7 -> pr/bresult-load.8, compare) due to conflict with #26021
Rebased f3d1658 -> f718aec (pr/bresult-load.8 -> pr/bresult-load.9, compare) due to conflict with #26005
Rebased f718aec -> 7bd578a (pr/bresult-load.9 -> pr/bresult-load.10, compare) due to conflicts with #25667, #26198, and #26282
Rebased 7bd578a -> 2c675f0 (pr/bresult-load.10 -> pr/bresult-load.11, compare) due to conflicts with #26238, #26462, #26594, #26618, #26758
Rebased 2c675f0 -> 2e12b25 (pr/bresult-load.11 -> pr/bresult-load.12, compare) on top of pr/bresult2.28
Rebased 2e12b25 -> 5c1be53 (pr/bresult-load.12 -> pr/bresult-load.13, compare) due to conflicts with #26595 and #26032
Rebased 5c1be53 -> 9c28172 (pr/bresult-load.13 -> pr/bresult-load.14, compare) due to conflicts with #25666 and #24845
Rebased 9c28172 -> 2a9db07 (pr/bresult-load.14 -> pr/bresult-load.15, compare) due to conflict with #27279

The util::ResultPtr class is a wrapper for util::Result providing syntax sugar
to make it less awkward to use with returned pointers. Instead of needing to be
deferenced twice with **result or (*result)->member, it only needs to be
dereferenced once with *result or result->member. Also when it is used in a
boolean context, instead of evaluating to true when there is no error and the
pointer is null, it evaluates to false so it is straightforward to determine
whether the result points to something.

This PR is only partial a solution to bitcoin#26004 because while it makes it easier
to check for null pointer values, it does not make it impossible to return a
null pointer value inside a successful result. It would be possible to enforce
that successful results always contain non-null pointers by using a not_null
type (bitcoin#24423) in combination with
ResultPtr, though.
-BEGIN VERIFY SCRIPT-
git grep -l DatabaseStatus src | xargs sed -i s/DatabaseStatus/DatabaseError/g
sed -i '/^    SUCCESS,$/d' src/wallet/db.h
-END VERIFY SCRIPT-
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request May 1, 2024
Add util::Result support for returning warning messages and multiple errors,
not just a single error string. This provides a way for functions to report
errors and warnings in a standard way, and simplifies interfaces.

The functionality is unit tested here, and put to use in followup PR
bitcoin#25722
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request May 1, 2024
Suggested by Martin Leitner-Ankerl <martin.ankerl@gmail.com>
bitcoin#25722 (comment)

Co-authored-by: Martin Leitner-Ankerl <martin.ankerl@gmail.com>
@DrahtBot
Copy link
Contributor

DrahtBot commented May 1, 2024

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is 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.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/24477515855

@ryanofsky ryanofsky force-pushed the pr/bresult-load branch 3 times, most recently from ae0b9b8 to 5beaefa Compare May 3, 2024 15:15
@DrahtBot DrahtBot removed the CI failed label May 3, 2024
@ryanofsky
Copy link
Contributor Author

Rebased 2385cb3 -> 987b28b (pr/bresult-load.21 -> pr/bresult-load.22, compare) to use newer result API in #25722.
Updated 987b28b -> 548bf50 (pr/bresult-load.22 -> pr/bresult-load.23, compare) to fix a bug in wallet tool and an accidental change to error message ordering
Updated 548bf50 -> ae0b9b8 (pr/bresult-load.23 -> pr/bresult-load.24, compare) to fix more unintended changes to error message order.
Updated ae0b9b8 -> 5beaefa (pr/bresult-load.24 -> pr/bresult-load.25, compare) to fix clang-tidy errors in https://cirrus-ci.com/task/6679456805289984 and ResultExtract bug in intermediate commit https://github.com/bitcoin/bitcoin/actions/runs/8931046064/job/24532379963?pr=25722

@DrahtBot
Copy link
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

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

Successfully merging this pull request may close these issues.

None yet

5 participants