The PR branch HEAD was 111ea3a at the time of this review club meeting.
Notes
To simplify error handling in the call site, functions in Bitcoin Core quite often return a boolean to indicate if the operation was successful. One or multiple out-parameters can then be used to access the function result and information about the operation (e.g. error messages).
To put it in (pseudo)code, you’ll find many functions that look somewhat like this:
booldoSomething(arg1,arg2,arg3,arg4,&result_obj,&error_string){// do something...if(error){error_string="something bad happened";returnfalse;}result=goodResult;returntrue;}
Using out-parameters is not always encouraged because they can be confusing or harder to read. Intuitively, parameters represent the input to a function and the return value represents the output.
In #25218, a new BResult utility class is introduced that can be used to simplify function signatures by removing dependencies on out-parameters, while keeping it easy for call sites to verify that the called function returned successfully.
BResult internally uses the private std::variant member std::variant<bilingual_str, T> m_variant which allows different types to reside in a single memory space. In practice, this m_variant stores either the result object of type T, or the error string of type bilingual_str.
Using BResult, the previous pseudo code can now be simplified to:
BResult<Obj>doSomething(arg1,arg2,arg3,arg4){// do something...if(error)return"something bad happened";returngoodResult;}
Taking advantage of this new BResult class for existing code requires both the function as well all of its call sites to be refactored.
The result of the kind of code simplification enabled by #25218 is quite visible in e.g. CWallet::GetNewDestination, where both the actual result as well as the error string are no longer out-parameters.
What are the different types of parameters and return values that the PR assumes we commonly use in a function signature? Which of these parameters are affected by the PR?
Why does BResult have a separate constructor BResult(const bilingual_str& error) that seems to do the exact same as the templated constructor BResult(const T& _obj)? Does this introduce any edge cases or limitations, and if so - are they documented?
Do you know of any other commonly used return type(s) in the codebase that are similar to BResult?
In commit wallet: refactor, include 'FeeCalculation' inside 'CreatedTransactionResult', what do you think is the rationale for making FeeCalculation a member of CreatedTransactionResult instead of having it as an out-parameter in CreateTransaction()?
Should someone now follow up with a big PR that refactors all functions that would benefit from using BResult? Why (not)?
(Bonus) Do you know of any other functions that you think would benefit nicely from being refactored to use BResult?
<stickies-v> welcome everyone! Today we're looking at #25218, authored by furszy. The notes and questions are available on https://bitcoincore.reviews/25218
<stickies-v> satsie: and there's even more discussion in other places too, but we'll get to that in a second! but the discussion is always a nice place to see how different people catch different things and why having many eyes on the code is so important
<stickies-v> What are the different types of parameters and return values that the PR assumes we commonly use in a function signature? Which of these parameters are affected by the PR?
<satsie> I think the PR assumes most function signatures have your typical set of in parameters, and two out parameters (error and return value). The PR affects the two out parameters
<stickies-v> Why does `BResult` have a separate constructor `BResult(const bilingual_str& error)` that seems to do the exact same as the templated constructor `BResult(const T& _obj)`? Does this introduce any edge cases or limitations, and if so - are they documented?
<satsie> for the second question, I was also a little stumped. I'm sure there's some C++ stuff going on here that I don't know about but the two constructors seem to enforce that m_variant can only be bilingual_str& OR T&
<stickies-v> larryruane: I'm very surprised it would still compile. If e.g. a function is meant to return `int`, we'd define the return type as `BResult<int>`. If we then have an error and just return "some error string", then that shouldn't compile, I think?
<stickies-v> satsie adam2k: so `BResult` is templated to whatever we expect a function to return, e.g. an `int` or any other type. But we also want it to be straightforward (and obvious) to raise an error within the function. So by overloading the constructor with another `bilingual_str&` constructor, to raise an error anywhere in the function we can just return a (bilingual) string
<furszy> stickies-v: yeah ok, the goal of this initial implementation was to introduce the BResult class without the generic error. Just the simplest, and more beneficial, use case for it.
<adam2k> stickies-v I'm still confused on the previous comment about `bilingual_str&`. Maybe I'm just rusty on C++, but how does the `BResult(const T& _obj)` constructor differ from `BResult(const bilingual_str& error)`?
<stickies-v> just because something is templated, doesn't mean that just accepts any type. Even though templates can infer types automatically, in this case we always explicitly specify the type in our function signature
<furszy> josie[m]: initially, I implemented it a bit different. There was a pure generic base class Result<T, E> and a specialization of it with Result<T, bilingual_str>. But.. not many were happy introducing a class that wasn't connected to any function yet. So, ended up unifying them.
<larryruane> yeah what helps me understand templating is to remember that it generates separate code for each type, like actually separate, different places in memory!
<stickies-v> yeah bilingual_str isn't exclusively used for errors, I'm not expert in the GUI but I believe that's where it's mostly used to represent translations
<satsie> got it. So to take it a step further, `BResult.HasRes()` assumes that the presence of a bilingual_str means there is an error, and in cases when bilingual_str doesn't actually mean an error, the caller is going to run into trouble
<stickies-v> satsie: exactly! it was a bit of a trick question, because it's not necessarily the constructor that's problematic, it's the HasRes() function
<satsie> woo hoo! so that follow up PR you just posted is an extension of the discussion in the original PR about the choice to make `bilingual_str` customizable/generic, right? (which furszy commented on a bit earlier in this chat)
<stickies-v> adam2k: interesting, I hadn't thought of that. RPCResult does standardize return types for the RPC, but maybe not really with that much of a focus on error handling
<stickies-v> larryruane: yes exactly, I think `std::optional` and `BResult` serve very similar purposes, where `BResult` adds support for accessing the error message
<stickies-v> and https://github.com/bitcoin/bitcoin/pull/25608 also aims to streamline this, by keeping the interface identical with `std::optional` (e.g. using `value()`, `value_or()`, `has_value()` functions as well as overloading the pointer `*` and `->` operators)
<stickies-v> hmm I'm not sure that would make sense. Not all functions require access to further error handling data, I think there are quite a few cases where getting a `std::nullopt` is perfectly clear and then using `std::optional` seems like a good choice?
<furszy> larryruane: right now, BResult is implemented as an OR (internally uses an std::variant which is analogous to an union). It contains the succeed object OR the failure object (which is currently hardcoded to a single type)
<larryruane> but it is I think nice to initially use it in a few places, to make sure the interface is good and the code actually works! and also there can be further improvements!
<satsie> also it looks like there are already changes underway to upgrade BResult. It wouldn't make sense to do a sweeping refactor PR until some of that dust settles
<stickies-v> i'd on the plus side say it would be very nice to have more uniform coding patterns across the codebase, makes it much easier for newcomes to onboard the codebase