Introduce generic 'Result' class and connect it to CreateTransaction and GetNewDestination (refactoring)

Host: stickies-v  -  PR author: furszy

The PR branch HEAD was 111ea3a at the time of this review club meeting.


  • 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:
    bool doSomething(arg1, arg2, arg3, arg4, &result_obj, &error_string) {
      // do something...
      if (error) {
          error_string = "something bad happened";
          return false;
      result = goodResult;
      return true;
  • 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";
      return goodResult;
  • 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.


  1. Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK?

  2. 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?

  3. 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?

  4. Do you know of any other commonly used return type(s) in the codebase that are similar to BResult?

  5. 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()?

  6. Should someone now follow up with a big PR that refactors all functions that would benefit from using BResult? Why (not)?

  7. (Bonus) Do you know of any other functions that you think would benefit nicely from being refactored to use BResult?

Meeting Log

  117:00 <stickies-v> #startmeeting
  217:00 <larryruane> hi
  317:00 <gamliel> hi
  417:00 <furszy> hi
  517:00 <dariusp> hi
  617:01 <josie[m]> hi
  717:01 <wieland7> hi
  817:01 <satsie> hi
  917:01 <Paul_C> Hey everyone
 1017:01 <Lov3r_Of_Bitcoin> hello
 1117:01 <adam2k> 👋
 1217:01 <stickies-v> welcome everyone! Today we're looking at #25218, authored by furszy. The notes and questions are available on
 1317:02 <michaelfolkson2> hi
 1417:02 <stickies-v> very glad to see that we've got the author himself present here as well!
 1517:02 <stickies-v> anyone joining us for the first time today? even if you're just lurking, feel free to say hi!
 1617:02 <gamliel> hi  o/
 1717:02 <adam2k> First time for me
 1817:03 <gamliel> just lurking, eager to collab some day :)
 1917:03 <stickies-v> glad you found your way here adam2k, welcome and don't hesitate to ask or participate if you feel like it
 2017:03 <stickies-v> yes - lurkers very welcome!
 2117:03 <stickies-v> who got the chance to review the PR or read the notes? (y/n)
 2217:03 <gamliel> <3
 2317:04 <adam2k> y
 2417:04 <khorner> lurker - n
 2517:04 <satsie> y
 2617:04 <wieland7> partially
 2717:04 <gamliel> y
 2817:04 <stickies-v> nice, lots of eyes on the code!
 2917:04 <dariusp> notes - y, PR - briefly
 3017:04 <stickies-v> for those of you who were able to review, would you give it a Concept ACK, Approach ACK, Tested ACK, or NACK?
 3117:05 <josie[m]> y (read notes and ended up needing to rebase on top of this PR)
 3217:05 <stickies-v> (and in general, even though the PR is already merged, post-merge (N)ACKs are always very welcome too)
 3317:06 <josie[m]> Aproach ACK
 3417:06 <stickies-v> josie[m]: good, more usage :-D
 3517:06 <Lov3r_Of_Bitcoin> Concept ACK
 3617:06 <satsie> approach ack - but I was really surprised to see a bunch of things I hadn't considered in the PR discussion
 3717:06 <gamliel> sorry, my first time in this meeting too :P
 3817:06 <schmidty_> hi
 3917:07 <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
 4017:08 <stickies-v> very welcome, gamliel!
 4117:08 <satsie> indeed!
 4217:08 <adam2k> Approach ACK
 4317:08 <stickies-v> alright let's get going with the first question
 4417:08 <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?
 4517:10 <adam2k> bilingual_str was removed from the function signature in a bunch of places.
 4617:10 <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
 4717:11 <larryruane> satsie: yes, and typically the return value type is `bol`
 4817:11 <larryruane> *bool
 4917:11 <larryruane> (indicating success or failure)
 5017:11 <satsie> ah, yes! good point
 5117:11 <stickies-v> satsie & larryruane - yes exactly, those I'd say are the main 4 categories here
 5217:12 <stickies-v> so this PR should not really affect the input parameters, but it does affect the function signature for all 3 other categories
 5317:13 <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?
 5417:13 <satsie> stickies-v can you clarify what the 3 other categories are?
 5517:13 <stickies-v> (note: the discussion is async, so even if i move on to the next question, feel free to continue the discussion on previous points)
 5617:14 <larryruane> yeah, I couldn't figure this out! I commented out the bilingual_str constructor, and it still compiled
 5717:14 <stickies-v> satsie: the ones you mentioned, actually. 1) inputs 2) outputs 3) error handling
 5817:15 <stickies-v> error handling is also a kind of output of course, but I think it's still different enough
 5917:15 <satsie> 👍
 6017:16 <larryruane> is the separate constructor just for code clarity? so it's understood that there are these two different types of `BResult`s? I'm unsure
 6117:16 <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&
 6217:16 <adam2k> For the second question does the separate constructor exist to be an overloaded constructor for this particular type?
 6317:16 <satsie> +1 to what you're saying about code clarity Larry
 6417:17 <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?
 6517:18 <stickies-v> (without the dedicated `bilingual_str&` constructor)
 6617:18 <larryruane> maybe I messed up :)
 6717:19 <furszy> yeah, that shouldn't be compiling.
 6817:19 <satsie> is it to show that you can create a BResult with just one input, and the other part of the result (the error or the T) is inferred?
 6917:19 <furszy> should get something like "no known conversion from 'bilingual_str' to 'const BResul<something>"
 7017:20 <larryruane> i checked again and dont see anything i did wrong ... i'm using clang BTW (tho shouldn't matter)
 7117:20 <josie[m]> adam2k: what do you mean by "overloaded constructor"?
 7217:21 <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
 7317:22 <furszy> larryruane: just checked it locally and build failed.
 7417:22 <larryruane> why isn't the error type templated? Is it just because `bilingual_str` is so common, we don't think anything else will be needed?
 7517:23 <larryruane> furszy: ok I'll investigate later, thanks
 7617:23 <adam2k> josibake___ just that the parameters are different for the constructor in lines 21-23 here
 7717:23 <stickies-v> larryruane: yes I believe that was the consensus in the discussion but I think furszy may be able to elaborate
 7817:24 <josie[m]> adam2k: ah! got it
 7917:24 <stickies-v> removing the `bilingual_str&` constructor also fails to compile for me here
 8017:25 <satsie> larryruane: Here's a link to a comment on an argument to not make `bilingual_str` customizable:
 8117:25 <josie[m]> larryruane, furszy: i also had the same question regarding error type templates
 8217:25 <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.
 8317:26 <wieland7> also get a compiler error when commenting out the constructor
 8417:26 <larryruane> satsie: thanks!
 8517:26 <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)`?
 8617:27 <adam2k> both look like pass by reference objects to me, but one has the specific `bilingual_str` type, right?
 8717:27 <stickies-v> adam2k: look at commit , for example
 8817:28 <stickies-v> you'll see that the return type of `getNewDestination` becomes `virtual BResult<CTxDestination> `
 8917:29 <stickies-v> this means that the templated constructor now expects `T` to be of type `CTxDestination`
 9017:30 <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
 9117:31 <adam2k> ah!  Thanks, got it.
 9217:31 <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.
 9317:32 <stickies-v> there was a second part to the question though that I think hasn't been answered yet
 9417:32 <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!
 9517:32 <stickies-v> "Does this introduce any edge cases or limitations, and if so - are they documented?"
 9617:32 <josie[m]> furszy: thanks! catching up on the review comments and it makes more sense
 9717:32 <stickies-v> larryruane: yeah thanks that's a helpful way to think about it!
 9817:33 <stickies-v> hint: what if we have a function that produces a `bilingual_str` as an output?
 9917:34 <larryruane> stickies-v: you mean an existing function (with that return type), and we want to change it to a `BResult` type?
10017:35 <satsie> A function that produces a `bilingual_str` as an output without intending for it to be an error?
10117:35 <adam2k> Does that mean that the destination could be an error?
10217:35 <adam2k> +1 to @sat
10317:35 <adam2k> +1 to satsie
10417:36 <larryruane> oh, you probably can't do `BResult<bilingual_str>`
10517:36 <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
10617:38 <larryruane> yes i see the problem there ... maybe there should have been a new error string type?
10717:38 <adam2k> Or different error handling for this case?
10817:39 <larryruane> (could be just a wrapper around a bilingual_str)
10917:39 <stickies-v> there are 2 follow up PRs that tackle this: and
11017:39 <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
11117:40 <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
11217:41 <stickies-v> Do you know of any other commonly used return type(s) in the codebase that are similar to `BResult`?
11317:42 <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)
11417:42 <larryruane> oh this is interesting, i also get the compile error with gcc, but not with clang! maybe someone else can try clang
11517:43 <larryruane> (this is if i've commented out `BResult(const bilingual_str& error) : m_variant(error) {}` in result.h)
11617:43 <stickies-v> satsie: yeah, and a couple more extensions too (which I'll be getting to in this very question actually!)
11717:45 <adam2k> RPCResult looks like it might be another return type that is similar to BResult?
11817:46 <josie[m]> skimming these follow-up PRs and both are really informative. any interest in adding them to a follow-up PR review club?
11917:48 <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
12017:48 <larryruane> stickies-v: a similar return type, or at least has the same general goal, i would say is `std::optional<T>`
12117:49 <stickies-v> larryruane: yes exactly, I think `std::optional` and `BResult` serve very similar purposes, where `BResult` adds support for accessing the error message
12217:49 <larryruane> i see many places where an object is return inside the `optional` if successful, otherwise return `nullopt`
12317:50 <stickies-v> and 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)
12417:50 <larryruane> probably another similar practice is to return a pointer (a `std::unique_ptr`) where it's nullptr if error
12517:50 <josie[m]> larryruane: ive been really confused by this in the past.. the use of nullopt and std::optional doesn't seem to be consistent
12617:51 <josie[m]> stickies-v, larryruane: could we eventually replace all the uses of std::optional with BResult (or something like it)?
12717:52 <stickies-v> josie[m]: what kind of inconsistency do you mean? that some functions use `std::unique_ptr` and others use `std::optional`?
12817:52 <larryruane> stickies-v: I *think* so, are you getting now to question 6?
12917:53 <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?
13017:53 <stickies-v> larryruane: hmm I mean it all kinda overlaps but I don't think so? :-D
13117:54 <larryruane> stickies-v: +1 good point
13217:54 <furszy> agree, BResult is useful when the function retrieves something else aside from the succeed value.
13317:54 <josie[m]> stickies-v: yes, that, and also im thinking of another specific example but looking at it again, it might be unrelated
13417:55 <larryruane> furszy: when you say succeed value, do you mean the boolean?
13517:55 <stickies-v> larryruane: OHH sorry I was looking at question 5 instead of 6. yes, you're right!
13617:56 <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)
13717:56 <larryruane> furszy: +1 thanks
13817:57 <stickies-v> let's wrap it up with a final discussion question
13917:57 <stickies-v> Should someone now follow up with a big PR that refactors all functions that would benefit from using `BResult`? Why (not)?
14017:57 <larryruane> no, it would be too disruptive, many unmerged PRs and downstream projects would need rebase, it's not currently broken, use for new code
14117:57 <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!
14217:58 <stickies-v> yeah I think you raise 2 very important points
14317:58 <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
14417:58 <josie[m]> larryruane: ++1
14517:58 <larryruane> (i just learned about the further improvements here!)
14617:58 <adam2k> yeah, I'd agree with larryruane.  It's probably better to make this a pattern that is implemented in future PRs.
14717:58 <stickies-v> we're already kind of short on review capacity, so dumping a huge PR like that would not be very responsible
14817:59 <stickies-v> and also that as we're using it in more and more places, we can iteratively improve/extend the interface
14917:59 <josie[m]> also a really good point that even tho the first PR has been merged, folks are actively still iterating on the design
15017:59 <larryruane> yeah and you can't really do a scripted diff for this, unfortunately
15118:00 <josie[m]> satsie: +1
15218:00 <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
15318:00 <stickies-v> but, a very high price to pay
15418:00 <adam2k> +1
15518:01 <stickies-v> alright now that we're all in agreement, looks like a nice place to wrap it up!
15618:01 <stickies-v> #endmeeting