util: Make `BResult` error a generic type instead of only `bilingual

w0xlt commented at 1:20 am on July 13, 2022: contributor

This PR makes BResult error a generic type instead of only bilingual_str and changes the class name to StructuredResult as BResut is not related to the purpose of the class.

The motivation is that some methods, like src/wallet/wallet.h:{RestoreWallet(...),CreateWallet(...),LoadWallet(...)} have more output parameters than just bilingual_str& error such as DatabaseStatus& status and std::vector<bilingual_str>& warnings.

With a generic template for error, it will be possible to create a struct that has bilingual_str& error and DatabaseStatus& status, for example, and use it in StructuredResult.

Built on top of #25594

Prepare BResult for non-copyable types fa8de09edc

refactor: Return BResult from restoreWallet fa475e9c79

DrahtBot added the label Utils/log/libs on Jul 13, 2022

ryanofsky commented at 3:11 am on July 13, 2022: contributor

The current BResult class is definitely very crippled, and needs to be improved to return real error handling information, not just an error reporting string.

Ironically the original implementation #25218 that had T m_result; and std::optional<bilingual_str> m_error members was not crippled, and was perfectly capable of returning rich error handling information. But after that (against all rationality!) the implementation was changed to use a std::variant so no information about the error could be returned other than the reporting string.

This PR (as of 09b796a7c7f62e1d24f7b173825b7dd4409e91b4) adds back the ability to return structured error handling information, but it loses the ability to return standard error reporting information. So it is one step forward, one step backwards, IMO.

I think a better solution is possible that:

adds back ability to return arbitrary data for error handling (like this PR)
doesn’t drop the standard error reporting format, which I think we can expand and build more utilities around for more complete reporting and less boilerplate (unlike this PR)
doesn’t require changing existing BResult usages (unlike this PR)

I had started writing up a version of this today built on #25308. Will try to post in the next few days and we can compare notes.

ryanofsky commented at 3:20 am on July 13, 2022: contributor

Would add I don’t think iterations on the BResult class should hold up either PR #25594 or #25308. I think both PR’s are in a good state and would be good to merge when they have enough review.

DrahtBot commented at 6:02 am on July 13, 2022: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
#25721 (refactor: Replace BResult with util::Result by ryanofsky)
#25665 (refactor: Add util::Result class and use it in LoadChainstate by ryanofsky)
#25616 (refactor: Return BResult from WalletLoader methods by w0xlt)
#25297 (wallet: speedup transactions sync, rescan and load by grouping all independent db writes on a single batched db transaction by furszy)
#25273 (wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction by achow101)
#24897 ([Draft / POC] Silent Payments by w0xlt)
#19602 (wallet: Migrate legacy wallets to descriptor wallets 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.

util: Add `StructuredResult`

'BResult' is a generic class useful for wrapping
a return object o an error, but the error type is
only `bilingual_str`. This commit changes the error
for a generic type, allowing the use of this class
where the error type is not `bilingual_str`.

28c81d7317

scripted-diff: rename BResult<CTxDestination> -> SingeErrorResult<CTxDestination>

-BEGIN VERIFY SCRIPT-
sed -i 's/BResult<CTxDestination>/SingeErrorResult<CTxDestination>/g' -- $(git grep --files-with-matches 'BResult<CTxDestination>')
-END VERIFY SCRIPT-

927b605957

scripted-diff: rename BResult<CTransactionRef> -> SingeErrorResult<CTransactionRef>

-BEGIN VERIFY SCRIPT-
sed -i 's/BResult<CTransactionRef>/SingeErrorResult<CTransactionRef>/g' -- $(git grep --files-with-matches 'BResult<CTransactionRef>')
-END VERIFY SCRIPT-

ebd0a77036

scripted-diff: rename BResult<CreatedTransactionResult> -> SingeErrorResult<CreatedTransactionResult>

-BEGIN VERIFY SCRIPT-
sed -i 's/BResult<CreatedTransactionResult>/SingeErrorResult<CreatedTransactionResult>/g' -- $(git grep --files-with-matches 'BResult<CreatedTransactionResult>')
-END VERIFY SCRIPT-

5f2e041b9f

scripted-diff: rename BResult<std::unique_ptr<Wallet>> -> SingeErrorResult<std::unique_ptr<Wallet>>

-BEGIN VERIFY SCRIPT-
sed -i 's/BResult<std::unique_ptr<Wallet>>/SingeErrorResult<std::unique_ptr<Wallet>>/g' -- $(git grep --files-with-matches 'BResult<std::unique_ptr<Wallet>>')
-END VERIFY SCRIPT-

463e63170f

util: Remove `class BResult`

Remove `class BResult` as all references to it
were removed in the last commits.

696f953655

w0xlt force-pushed on Jul 13, 2022

w0xlt commented at 5:30 pm on July 13, 2022: contributor

@ryanofsky in https://github.com/bitcoin/bitcoin/pull/25601/commits/28c81d7317b248c7a0b6ee1b4cef4b822a5e4286, I added the class SingeErrorResult which meets all the all the requirements you mentioned:

adds back ability to return arbitrary data for error handling (it is a class derived from StructuredResult).
doesn’t drop the standard error reporting format (SingeErrorResult() : StructuredResult<T, bilingual_str>(Untranslated("")) {}).
doesn’t require changing existing BResult usages (except for the name change, this keeps the same BResult usage).

ryanofsky commented at 7:14 pm on July 13, 2022: contributor

Thanks! Glad to see this (as of 696f9536556f78698cc82c64515fa8b1e0b01ef5) adding back a standard type for error reporting.

I do think there are still some issues here. The main one is that I don’t see a point to the StructuredResult<S, E> class. The class doesn’t have any functionality, and is just a nonstandard wrapper around a std::variant<std::monostate, E, S> field. I can’t imagine code that could use StructuredResult<S, E> that wouldn’t be better off using std::optional<E> or std::variant<S, E1, E2, ...> avoiding the need to nest multiple error types in single error type, or call nonstandard HasRes GetObj methods, or deal with impossible null states.

Also think it would be better to standardize on an error reporting class that can return multiple errors and warnings (and optionally format the result as a single error) than a class that can only return one error. Draft PR #25608 is a little rough but starts to go in this direction.

w0xlt commented at 4:15 am on August 3, 2022: contributor

Closing this in favor of #25721.

w0xlt closed this on Aug 3, 2022

bitcoin locked this on Aug 3, 2023

util: Make BResult error a generic type instead of only bilingual_str #25601

Conflicts

util: Make `BResult` error a generic type instead of only `bilingual_str` #25601