refactor: Replace BResult with util::Result #25721

ryanofsky · 2022-07-27T19:26:41Z

Rename BResult class to util::Result and update the class interface to be more compatible with std::optional and with a full-featured result class implemented in #25665. Motivation for this change is to update existing BResult usages now so they don't have to change later when more features are added in #25665.

This change makes the following improvements originally implemented in #25665:

More explicit API. Drops potentially misleading BResult constructor that treats any bilingual string argument as an error. Adds util::Error constructor so it is never ambiguous when a result is being assigned an error or non-error value.
Better type compatibility. Supports util::Result<bilingual_str> return values to hold translated messages which are not errors.
More standard and consistent API. util::Result supports most of the same operators and methods as std::optional. BResult had a less familiar interface with HasRes/GetObj/ReleaseObj methods. The Result/Res/Obj naming was also not internally consistent.
Better code organization. Puts src/util/ code in the util:: namespace so naming reflects code organization and it is obvious where the class is coming from. Drops "B" from name because it is undocumented what it stands for (bilingual?)
Has unit tests.

DrahtBot · 2022-07-27T20:55:49Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25756 (rest: Remove support for a number of -deprecatedrest options by stickies-v)
#25755 (rest: Use from_blockhash and txdetails query parameters by stickies-v)
#25754 (rest: Extend HTTPRequest interface to support querying path (parameters) by stickies-v)
#25753 (rest: Move format string from path-like parameter to query parameter by stickies-v)
#25734 (wallet, refactor: wallet: avoid mixing different OutputTypes during coin selection #24584 follow-ups by josibake)
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
#25685 (wallet: Faster transaction creation by removing pre-set-inputs fetching responsibility from Coin Selection by furszy)
#25666 (refactor: wallet, do not translate init options names by furszy)
#25665 (refactor: Add util::Result class and use it in LoadChainstate by ryanofsky)
#25656 (refactor: wallet: return BResult from GetReservedDestination methods by theStack)
#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)
#25269 (wallet: re-activate the not triggered "AmountWithFeeExceedsBalance" error by furszy)
#25183 (rpc: Filter inputs by type during CoinSelection by aureleoules)
#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.

ryanofsky · 2022-07-27T21:30:34Z

Updated 13aeab1 -> 2aa408b (pr/bresult-del.1 -> pr/bresult-del.2, compare) dropping dependency on #25665 as suggested #25665 (comment)
Updated 2aa408b -> e71b858 (pr/bresult-del.2 -> pr/bresult-del.3, compare) adding missing test file
Updated e71b858 -> 42f4f7d (pr/bresult-del.3 -> pr/bresult-del.4, compare) fixing comment

w0xlt

Concept ACK.
But maybe the changes can be split in two or more commits.

maflcko

looks good, but doesn't compile in many places

maflcko · 2022-08-01T14:14:12Z

src/util/result.h

+    const T* operator->() const { return &value(); }
+    const T& operator*() const { return value(); }
+    T* operator->() { return &value(); }
+    T& operator*() { return value(); }


For reference, this addresses my feedback from #25218 (comment)

src/util/result.h

src/wallet/spend.cpp

src/bench/wallet_loading.cpp

src/qt/walletcontroller.cpp

src/util/result.h

maflcko

(Feel free to ignore the feedback)

src/wallet/test/coinselector_tests.cpp

src/wallet/test/fuzz/notifications.cpp

maflcko

42f4f7d 🖌

Show signature

Signature:

ryanofsky · 2022-08-01T19:30:07Z

Thanks for reviews!

Updated 42f4f7d -> e0289b1 (pr/bresult-del.4 -> pr/bresult-del.5, compare) with suggestions

re: #25721 (review)

maybe the changes can be split in two or more commits.

I'm not so sure there is a way to do this that's a clear improvement. It would be possible to split by adding the new result interface and changing the old result interface to wrap the new interface in one commit, then deleting the old interface and changing usages to call the new interface in a second commit. This would split up the PR but also make it a little more complicated. Wouldn't oppose this, just not sure it would be worth it

ryanofsky · 2022-08-02T04:02:38Z

Rebased e0289b1 -> 3262acf (pr/bresult-del.5 -> pr/bresult-del.6, compare) due to silent conflict with #24584

maflcko

ACK 3262acf 🍕

Show signature

Signature:

src/wallet/spend.cpp

src/wallet/test/coinselector_tests.cpp

src/test/result_tests.cpp

src/util/result.h

ryanofsky

Very helpful review!

Updated 3262acf -> 6777df6 (pr/bresult-del.6 -> pr/bresult-del.7, compare) with suggested changes

src/test/result_tests.cpp

src/wallet/spend.cpp

src/wallet/test/coinselector_tests.cpp

src/util/result.h

src/bench/wallet_loading.cpp

src/util/result.h

maflcko · 2022-08-02T15:55:11Z

ACK 6777df6 🏏

Show signature

Signature:

src/test/result_tests.cpp

ryanofsky

Updated 6777df6 -> c654ec5 (pr/bresult-del.7 -> pr/bresult-del.8, compare) with more complete commit description and suggestions from last review
Updated c654ec5 -> 7b249b3 (pr/bresult-del.8 -> pr/bresult-del.9, compare) with a few more cleanups

src/test/result_tests.cpp

maflcko

Some nits, but please don't take them or I might leave more

src/util/result.h

maflcko · 2022-08-02T17:32:19Z

src/util/result.h

+    //! std::optional methods, so functions returning optional<T> can change to
+    //! return Result<T> with minimal changes to existing code, and vice versa.
+    bool has_value() const { return m_variant.index() == 1; }
+    const T& value() const LIFETIMEBOUND


re: #25721 (comment)

same?

Can throw bad_variant_access according to https://en.cppreference.com/w/cpp/utility/variant/get. The equivalent std::optional method can also throw https://en.cppreference.com/w/cpp/utility/optional/value

I don't think it can throw after our assert, can it?

src/util/result.h

maflcko · 2022-08-02T17:33:12Z

src/util/result.h

+        return has_value() ? std::move(value()) : std::forward<U>(default_value);
+    }
+    explicit operator bool() const { return has_value(); }
+    const T* operator->() const LIFETIMEBOUND { return &value(); }


Same, per https://en.cppreference.com/w/cpp/utility/optional/operator*

re: #25721 (comment)

Same, per https://en.cppreference.com/w/cpp/utility/optional/operator*

I guess this is a difference between std::optional and std::variant. It doesn't seem like there is a way to get the address of an object inside a variant that is noexcept. it would be possible to make this noexcept in followup #25665 which removes the std::variant.

It should be possible, given our assert, no?

ryanofsky · 2022-08-02T17:46:04Z

Some nits, but please don't take them or I might leave more

Ok, will skip noexcept stuff for now unless there is another push. If I rebase #25665 on top of this PR, that would be another chance to make noexcept or constexpr or similar improvements.

furszy

Concept ACK, pretty cool stuff.
Will review it deeper in the coming days.

Have to say that would have loved to talk about some of this stuff on the initial PR and/or via DM. Including the thrashing to the current result class name. But well, no hard feelings.

src/wallet/spend.cpp

furszy · 2022-08-03T04:07:33Z

src/util/result.h

-    /* Whether the function succeeded or not */
-    bool HasRes() const { return std::holds_alternative<T>(m_variant); }
+public:
+    Result(T obj) : m_variant{std::in_place_index_t<1>{}, std::move(obj)} {}


styling nit: what about creating an enum for the indexes?

enum { ERR=0, VAL=1 }; // then std::in_place_index_t<VAL>{}

re: #25721 (comment)

enum { ERR=0, VAL=1 }; // then std::in_place_index_t<VAL>{}

#25665 should drop the std::variant entirely making this moot, but using enum for this seems more indirect and fragile since nothing keeps enum and variant declarations lined up. I'm not against adding this, but would prefer not to unless there is more demand

Rename `BResult` class to `util::Result` and update the class interface to be more compatible with `std::optional` and with a full-featured result class implemented in bitcoin#25665. Motivation for this change is to update existing `BResult` usages now so they don't have to change later when more features are added in bitcoin#25665. This change makes the following improvements originally implemented in bitcoin#25665: - More explicit API. Drops potentially misleading `BResult` constructor that treats any bilingual string argument as an error. Adds `util::Error` constructor so it is never ambiguous when a result is being assigned an error or non-error value. - Better type compatibility. Supports `util::Result<bilingual_str>` return values to hold translated messages which are not errors. - More standard and consistent API. `util::Result` supports most of the same operators and methods as `std::optional`. `BResult` had a less familiar interface with `HasRes`/`GetObj`/`ReleaseObj` methods. The Result/Res/Obj naming was also not internally consistent. - Better code organization. Puts `src/util/` code in the `util::` namespace so naming reflects code organization and it is obvious where the class is coming from. Drops "B" from name because it is undocumented what it stands for (bilingual?) - Has unit tests.

ryanofsky

Rebased 7b249b3 -> a23cca5 (pr/bresult-del.9 -> pr/bresult-del.10, compare) due to silent conflict with #25272

re: #25721 (review)

Have to say that would have loved to talk about some of this stuff on the initial PR and/or via DM. Including the thrashing to the current result class name. But well, no hard feelings.

Thanks. I did bring up all of the issues I saw with the result class in the original PR, including the name. But I didn't want to get bogged down, and in general I think it's more productive to iterate on designs across PRs than to have back and forth debates in PR comments. Iterating across PRs puts PR authors in a position to propose unified designs, and avoid problems of design-by-committtee, or intractable disagreements, or compromises that split things down the middle and don't leave anybody happy. Better to give priority to PR authors and just take turns making improvements. Also, iterating on designs with actual PRs instead of PR comments centers code changes over verbal descriptions, which I think leads to better decisions in the end.

src/wallet/spend.cpp

ryanofsky · 2022-08-03T13:42:16Z

src/util/result.h

-    /* Whether the function succeeded or not */
-    bool HasRes() const { return std::holds_alternative<T>(m_variant); }
+public:
+    Result(T obj) : m_variant{std::in_place_index_t<1>{}, std::move(obj)} {}


re: #25721 (comment)

enum { ERR=0, VAL=1 }; // then std::in_place_index_t<VAL>{}

#25665 should drop the std::variant entirely making this moot, but using enum for this seems more indirect and fragile since nothing keeps enum and variant declarations lined up. I'm not against adding this, but would prefer not to unless there is more demand

src/util/result.h

ryanofsky · 2022-08-03T14:57:35Z

src/util/result.h

+    //! std::optional methods, so functions returning optional<T> can change to
+    //! return Result<T> with minimal changes to existing code, and vice versa.
+    bool has_value() const { return m_variant.index() == 1; }
+    const T& value() const LIFETIMEBOUND


re: #25721 (comment)

same?

Can throw bad_variant_access according to https://en.cppreference.com/w/cpp/utility/variant/get. The equivalent std::optional method can also throw https://en.cppreference.com/w/cpp/utility/optional/value

src/util/result.h

ryanofsky · 2022-08-03T15:04:00Z

src/util/result.h

+        return has_value() ? std::move(value()) : std::forward<U>(default_value);
+    }
+    explicit operator bool() const { return has_value(); }
+    const T* operator->() const LIFETIMEBOUND { return &value(); }


re: #25721 (comment)

Same, per https://en.cppreference.com/w/cpp/utility/optional/operator*

I guess this is a difference between std::optional and std::variant. It doesn't seem like there is a way to get the address of an object inside a variant that is noexcept. it would be possible to make this noexcept in followup #25665 which removes the std::variant.

jonatack

ACK a23cca5

If you retouch, suggest running the clang-format script on these changes for indentation fixups: git diff -U0 HEAD~1.. | ./contrib/devtools/clang-format-diff.py -p1 -i -v

jonatack · 2022-08-05T11:53:43Z

src/test/result_tests.cpp

+}
+
+template <typename T, typename... Args>
+void ExpectFail(const util::Result<T>& result, const bilingual_str& str)


Add static for the non-template functions in this test file?

template should imply inline, which again implies that this can't be used in another translation unit, but correct me if I am wrong.

jonatack · 2022-08-05T11:56:33Z

src/bench/wallet_loading.cpp

    CMutableTransaction mtx;
-    mtx.vout.push_back({COIN, GetScriptForDestination(dest.GetObj())});
+    mtx.vout.push_back({COIN, GetScriptForDestination(*Assert(wallet.GetNewDestination(OutputType::BECH32, "")))});


Add util/check.h include header for Assert

jonatack · 2022-08-05T12:06:47Z

src/interfaces/wallet.h

@@ -88,7 +88,7 @@ class Wallet
    virtual std::string getWalletName() = 0;

    // Get a new address.
-    virtual BResult<CTxDestination> getNewDestination(const OutputType type, const std::string label) = 0;
+    virtual util::Result<CTxDestination> getNewDestination(const OutputType type, const std::string label) = 0;


While touching this line here and in the override function in src/wallet/interfaces.cpp

Suggested change

virtual util::Result<CTxDestination> getNewDestination(const OutputType type, const std::string label) = 0;

virtual util::Result<CTxDestination> getNewDestination(const OutputType type, const std::string& label) = 0;

Picked up in #25616.

jonatack · 2022-08-05T12:22:48Z

src/wallet/test/coinselector_tests.cpp

-        auto op_dest = wallet.GetNewDestination(OutputType::BECH32, "");
-        assert(op_dest.HasRes());
-        tx.vout[nInput].scriptPubKey = GetScriptForDestination(op_dest.GetObj());
+        tx.vout[nInput].scriptPubKey = GetScriptForDestination(*Assert(wallet.GetNewDestination(OutputType::BECH32, "")));


Add util/check.h include header for Assert

jonatack · 2022-08-05T12:23:37Z

src/wallet/test/fuzz/notifications.cpp

        if (fuzzed_data_provider.ConsumeBool()) {
            op_dest = wallet->GetNewDestination(type, "");
        } else {
            op_dest = wallet->GetNewChangeDestination(type);
        }
-        assert(op_dest.HasRes());
-        return GetScriptForDestination(op_dest.GetObj());
+        return GetScriptForDestination(*Assert(op_dest));


Add util/check.h include header for Assert

Can drop the temporary here, if desired

// Add tx to wallet const auto& op_dest = wallet.GetNewDestination(OutputType::BECH32M, ""); BOOST_ASSERT(op_dest); - const CTxDestination& dest = *op_dest; CMutableTransaction mtx; - mtx.vout.push_back({COIN, GetScriptForDestination(dest)}); + mtx.vout.push_back({COIN, GetScriptForDestination(*op_dest)});

Picked up in #25616.

jonatack · 2022-08-05T12:28:55Z

src/wallet/scriptpubkeyman.cpp

    }
-    return op_dest.HasRes();
+    return bool(op_dest);


pico-nit

Suggested change

return bool(op_dest);

return bool{op_dest};

or just has_value (from HasRes)?

Picked up in #25616.

maflcko · 2022-08-05T13:33:12Z

ACK a23cca5 🏵

Show signature

Signature:

ryanofsky mentioned this pull request Jul 27, 2022

refactor: Add util::Result failure values, multiple error and warning messages #25665

Open

DrahtBot added the Refactoring label Jul 27, 2022

DrahtBot mentioned this pull request Jul 27, 2022

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

Draft

ryanofsky force-pushed the pr/bresult-del branch from 13aeab1 to 2aa408b Compare July 27, 2022 21:27

ryanofsky force-pushed the pr/bresult-del branch from 2aa408b to e71b858 Compare July 27, 2022 21:34

ryanofsky marked this pull request as ready for review July 27, 2022 21:35

ryanofsky force-pushed the pr/bresult-del branch from e71b858 to 42f4f7d Compare July 27, 2022 21:48

w0xlt approved these changes Jul 30, 2022

View reviewed changes

maflcko approved these changes Aug 1, 2022

View reviewed changes

maflcko reviewed Aug 1, 2022

View reviewed changes

src/wallet/test/coinselector_tests.cpp Outdated Show resolved Hide resolved

src/wallet/test/fuzz/notifications.cpp Outdated Show resolved Hide resolved

maflcko reviewed Aug 1, 2022

View reviewed changes

ryanofsky force-pushed the pr/bresult-del branch from 42f4f7d to e0289b1 Compare August 1, 2022 19:19

ryanofsky force-pushed the pr/bresult-del branch from e0289b1 to 3262acf Compare August 2, 2022 04:00

maflcko approved these changes Aug 2, 2022

View reviewed changes

maflcko mentioned this pull request Aug 2, 2022

refactor: introduce generic 'Result' class and connect it to CreateTransaction and GetNewDestination #25218

Merged

ryanofsky force-pushed the pr/bresult-del branch from 3262acf to 6777df6 Compare August 2, 2022 15:47

ryanofsky commented Aug 2, 2022

View reviewed changes

maflcko reviewed Aug 2, 2022

View reviewed changes

src/test/result_tests.cpp Outdated Show resolved Hide resolved

ryanofsky force-pushed the pr/bresult-del branch from 6777df6 to c654ec5 Compare August 2, 2022 17:06

ryanofsky commented Aug 2, 2022

View reviewed changes

src/test/result_tests.cpp Outdated Show resolved Hide resolved

ryanofsky force-pushed the pr/bresult-del branch from c654ec5 to 7b249b3 Compare August 2, 2022 17:24

maflcko reviewed Aug 2, 2022

View reviewed changes

furszy reviewed Aug 3, 2022

View reviewed changes

ryanofsky force-pushed the pr/bresult-del branch from 7b249b3 to a23cca5 Compare August 3, 2022 15:35

ryanofsky commented Aug 3, 2022

View reviewed changes

This was referenced Aug 4, 2022

wallet, refactor: #24584 follow-ups #25734

Merged

wallet: Faster transaction creation by removing pre-set-inputs fetching responsibility from Coin Selection #25685

Merged

rpc: Filter inputs by type during CoinSelection #25183

Closed

jonatack reviewed Aug 5, 2022

View reviewed changes

maflcko added this to the 24.0 milestone Aug 5, 2022

maflcko merged commit 006740b into bitcoin:master Aug 5, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 5, 2022

Merge bitcoin#25721: refactor: Replace BResult with util::Result

1be3362

furszy mentioned this pull request Aug 9, 2022

wallet: Check max transaction weight in CoinSelection #25729

Merged

bitcoin locked and limited conversation to collaborators Aug 9, 2023

	virtual util::Result<CTxDestination> getNewDestination(const OutputType type, const std::string label) = 0;
	virtual util::Result<CTxDestination> getNewDestination(const OutputType type, const std::string& label) = 0;

refactor: Replace BResult with util::Result #25721

refactor: Replace BResult with util::Result #25721

Conversation

ryanofsky commented Jul 27, 2022 • edited

DrahtBot commented Jul 27, 2022 • edited

Conflicts

ryanofsky commented Jul 27, 2022 • edited

w0xlt left a comment

Choose a reason for hiding this comment

maflcko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maflcko left a comment

Choose a reason for hiding this comment

maflcko left a comment

Choose a reason for hiding this comment

ryanofsky commented Aug 1, 2022

ryanofsky commented Aug 2, 2022

maflcko left a comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

maflcko commented Aug 2, 2022

ryanofsky left a comment • edited

Choose a reason for hiding this comment

maflcko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanofsky commented Aug 2, 2022

furszy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonatack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonatack Aug 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonatack Aug 9, 2022 • edited

Choose a reason for hiding this comment

maflcko commented Aug 5, 2022

ryanofsky commented Jul 27, 2022 •

edited

DrahtBot commented Jul 27, 2022 •

edited

ryanofsky commented Jul 27, 2022 •

edited

ryanofsky left a comment •

edited

jonatack Aug 9, 2022 •

edited

jonatack Aug 9, 2022 •

edited