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

111ea3a wallet: refactor GetNewDestination, use BResult (furszy)
2235172 send: refactor CreateTransaction flow to return a BResult<CTransactionRef> (furszy)
198fcca wallet: refactor, include 'FeeCalculation' inside 'CreatedTransactionResult' (furszy)
7a45c33 Introduce generic 'Result' class (furszy)

Pull request description:

  Based on a common function signature pattern that we have all around the sources:
  ```cpp
  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;
  }
  ```

  Introduced a generic class `BResult` that encapsulate the function boolean result, the result object (in case of having it) and, in case of failure, the string error reason.

  Obtaining in this way cleaner function signatures and removing boilerplate code:

  ```cpp
  BResult<Obj> doSomething(arg1, arg2, arg3, arg4) {
      // do something...
      if (error) return "something bad happened";

      return goodResult;
  }
  ```

  Same cleanup applies equally to the function callers' side as well. There is no longer need to add the error string and the result object declarations before calling the function:

  Before:
  ```cpp
  Obj result_obj;
  std::string error_string;
  if (!doSomething(arg1, arg2, arg3, arg4, result_obj, error_string)) {
      LogPrintf("Error: %s", error_string);
  }
  return result_obj;
  ```

  Now:
  ```cpp
  BResult<Obj> op_res = doSomething(arg1, arg2, arg3, arg4);
  if (!op_res) {
      LogPrintf("Error: %s", op_res.GetError());
  }
  return op_res.GetObjResult();
  ```

  ### Initial Implementation:

  Have connected this new concept to two different flows for now:

  1) The `CreateTransaction` flow. --> 7ba2b87c
  2) The `GetNewDestination` flow. --> bcee0912

  Happy note: even when introduced a new class into the sources, the amount of lines removed is almost equal to added ones :).

  Extra note: this work is an extended version (and a decoupling) of the work that is inside #24845 (which does not contain the `GetNewDestination` changes nor the inclusion of the `FeeCalculation` field inside `CreatedTransactionResult`).

ACKs for top commit:
  achow101:
    ACK 111ea3a
  w0xlt:
    reACK bitcoin/bitcoin@111ea3a
  theStack:
    re-ACK 111ea3a
  MarcoFalke:
    review ACK 111ea3a 🎏

Tree-SHA512: 6d84d901a4cb923727067f25ff64542a40edd1ea84fdeac092312ac684c34e3688a52ac5eb012717d2b73f4cb742b9d78e458eb0e9cb9d6d72a916395be91f69

Author: MacroFake

src/Makefile.am

@@ -277,6 +277,7 @@ BITCOIN_CORE_H = \
   util/overloaded.h \
   util/rbf.h \
   util/readwritefile.h \
+  util/result.h \
   util/serfloat.h \
   util/settings.h \
   util/sock.h \

src/bench/wallet_loading.cpp

@@ -47,12 +47,11 @@ static void BenchUnloadWallet(std::shared_ptr<CWallet>&& wallet)
 
 static void AddTx(CWallet& wallet)
 {
-    bilingual_str error;
-    CTxDestination dest;
-    wallet.GetNewDestination(OutputType::BECH32, "", dest, error);
+    const auto& dest = wallet.GetNewDestination(OutputType::BECH32, "");
+    assert(dest.HasRes());
 
     CMutableTransaction mtx;
-    mtx.vout.push_back({COIN, GetScriptForDestination(dest)});
+    mtx.vout.push_back({COIN, GetScriptForDestination(dest.GetObj())});
     mtx.vin.push_back(CTxIn());
 
     wallet.AddToWallet(MakeTransactionRef(mtx), TxStateInactive{});

src/interfaces/wallet.h

@@ -12,6 +12,7 @@
 #include <script/standard.h>           // For CTxDestination
 #include <support/allocators/secure.h> // For SecureString
 #include <util/message.h>
+#include <util/result.h>
 #include <util/ui_change_type.h>
 
 #include <cstdint>
@@ -87,7 +88,7 @@ class Wallet
     virtual std::string getWalletName() = 0;
 
     // Get a new address.
-    virtual bool getNewDestination(const OutputType type, const std::string label, CTxDestination& dest) = 0;
+    virtual BResult<CTxDestination> getNewDestination(const OutputType type, const std::string label) = 0;
 
     //! Get public key.
     virtual bool getPubKey(const CScript& script, const CKeyID& address, CPubKey& pub_key) = 0;
@@ -138,12 +139,11 @@ class Wallet
     virtual void listLockedCoins(std::vector<COutPoint>& outputs) = 0;
 
     //! Create transaction.
-    virtual CTransactionRef createTransaction(const std::vector<wallet::CRecipient>& recipients,
+    virtual BResult<CTransactionRef> createTransaction(const std::vector<wallet::CRecipient>& recipients,
         const wallet::CCoinControl& coin_control,
         bool sign,
         int& change_pos,
-        CAmount& fee,
-        bilingual_str& fail_reason) = 0;
+        CAmount& fee) = 0;
 
     //! Commit transaction.
     virtual void commitTransaction(CTransactionRef tx,

src/qt/addresstablemodel.cpp

@@ -370,23 +370,21 @@ QString AddressTableModel::addRow(const QString &type, const QString &label, con
     else if(type == Receive)
     {
         // Generate a new address to associate with given label
-        CTxDestination dest;
-        if(!walletModel->wallet().getNewDestination(address_type, strLabel, dest))
-        {
+        auto op_dest = walletModel->wallet().getNewDestination(address_type, strLabel);
+        if (!op_dest) {
             WalletModel::UnlockContext ctx(walletModel->requestUnlock());
-            if(!ctx.isValid())
-            {
+            if (!ctx.isValid()) {
                 // Unlock wallet failed or was cancelled
                 editStatus = WALLET_UNLOCK_FAILURE;
                 return QString();
             }
-            if(!walletModel->wallet().getNewDestination(address_type, strLabel, dest))
-            {
+            op_dest = walletModel->wallet().getNewDestination(address_type, strLabel);
+            if (!op_dest) {
                 editStatus = KEY_GENERATION_FAILURE;
                 return QString();
             }
         }
-        strAddress = EncodeDestination(dest);
+        strAddress = EncodeDestination(op_dest.GetObj());
     }
     else
     {

src/qt/walletmodel.cpp

@@ -204,10 +204,10 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact
     {
         CAmount nFeeRequired = 0;
         int nChangePosRet = -1;
-        bilingual_str error;
 
         auto& newTx = transaction.getWtx();
-        newTx = m_wallet->createTransaction(vecSend, coinControl, !wallet().privateKeysDisabled() /* sign */, nChangePosRet, nFeeRequired, error);
+        const auto& res = m_wallet->createTransaction(vecSend, coinControl, !wallet().privateKeysDisabled() /* sign */, nChangePosRet, nFeeRequired);
+        newTx = res ? res.GetObj() : nullptr;
         transaction.setTransactionFee(nFeeRequired);
         if (fSubtractFeeFromAmount && newTx)
             transaction.reassignAmounts(nChangePosRet);
@@ -218,7 +218,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact
             {
                 return SendCoinsReturn(AmountWithFeeExceedsBalance);
             }
-            Q_EMIT message(tr("Send Coins"), QString::fromStdString(error.translated),
+            Q_EMIT message(tr("Send Coins"), QString::fromStdString(res.GetError().translated),
                 CClientUIInterface::MSG_ERROR);
             return TransactionCreationFailed;
         }

src/test/util/wallet.cpp

@@ -20,11 +20,10 @@ const std::string ADDRESS_BCRT1_UNSPENDABLE = "bcrt1qqqqqqqqqqqqqqqqqqqqqqqqqqqq
 std::string getnewaddress(CWallet& w)
 {
     constexpr auto output_type = OutputType::BECH32;
-    CTxDestination dest;
-    bilingual_str error;
-    if (!w.GetNewDestination(output_type, "", dest, error)) assert(false);
+    auto op_dest = w.GetNewDestination(output_type, "");
+    assert(op_dest.HasRes());
 
-    return EncodeDestination(dest);
+    return EncodeDestination(op_dest.GetObj());
 }
 
 #endif // ENABLE_WALLET

src/util/result.h

@@ -0,0 +1,43 @@
+// Copyright (c) 2022 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or https://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_UTIL_RESULT_H
+#define BITCOIN_UTIL_RESULT_H
+
+#include <util/translation.h>
+#include <variant>
+
+/*
+ * 'BResult' is a generic class useful for wrapping a return object
+ * (in case of success) or propagating the error cause.
+*/
+template<class T>
+class BResult {
+private:
+    std::variant<bilingual_str, T> m_variant;
+
+public:
+    BResult() : m_variant(Untranslated("")) {}
+    BResult(const T& _obj) : m_variant(_obj) {}
+    BResult(const bilingual_str& error) : m_variant(error) {}
+
+    /* Whether the function succeeded or not */
+    bool HasRes() const { return std::holds_alternative<T>(m_variant); }
+
+    /* In case of success, the result object */
+    const T& GetObj() const {
+        assert(HasRes());
+        return std::get<T>(m_variant);
+    }
+
+    /* In case of failure, the error cause */
+    const bilingual_str& GetError() const {
+        assert(!HasRes());
+        return std::get<bilingual_str>(m_variant);
+    }
+
+    explicit operator bool() const { return HasRes(); }
+};
+
+#endif // BITCOIN_UTIL_RESULT_H

src/wallet/feebumper.cpp

@@ -219,19 +219,18 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
     new_coin_control.m_min_depth = 1;
 
     constexpr int RANDOM_CHANGE_POSITION = -1;
-    bilingual_str fail_reason;
-    FeeCalculation fee_calc_out;
-    std::optional<CreatedTransactionResult> txr = CreateTransaction(wallet, recipients, RANDOM_CHANGE_POSITION, fail_reason, new_coin_control, fee_calc_out, false);
-    if (!txr) {
-        errors.push_back(Untranslated("Unable to create transaction.") + Untranslated(" ") + fail_reason);
+    auto res = CreateTransaction(wallet, recipients, RANDOM_CHANGE_POSITION, new_coin_control, false);
+    if (!res) {
+        errors.push_back(Untranslated("Unable to create transaction.") + Untranslated(" ") + res.GetError());
         return Result::WALLET_ERROR;
     }
 
+    const auto& txr = res.GetObj();
     // Write back new fee if successful
-    new_fee = txr->fee;
+    new_fee = txr.fee;
 
     // Write back transaction
-    mtx = CMutableTransaction(*txr->tx);
+    mtx = CMutableTransaction(*txr.tx);
 
     return Result::OK;
 }

src/wallet/interfaces.cpp

@@ -146,11 +146,10 @@ class WalletImpl : public Wallet
     void abortRescan() override { m_wallet->AbortRescan(); }
     bool backupWallet(const std::string& filename) override { return m_wallet->BackupWallet(filename); }
     std::string getWalletName() override { return m_wallet->GetName(); }
-    bool getNewDestination(const OutputType type, const std::string label, CTxDestination& dest) override
+    BResult<CTxDestination> getNewDestination(const OutputType type, const std::string label) override
     {
         LOCK(m_wallet->cs_wallet);
-        bilingual_str error;
-        return m_wallet->GetNewDestination(type, label, dest, error);
+        return m_wallet->GetNewDestination(type, label);
     }
     bool getPubKey(const CScript& script, const CKeyID& address, CPubKey& pub_key) override
     {
@@ -250,22 +249,21 @@ class WalletImpl : public Wallet
         LOCK(m_wallet->cs_wallet);
         return m_wallet->ListLockedCoins(outputs);
     }
-    CTransactionRef createTransaction(const std::vector<CRecipient>& recipients,
+    BResult<CTransactionRef> createTransaction(const std::vector<CRecipient>& recipients,
         const CCoinControl& coin_control,
         bool sign,
         int& change_pos,
-        CAmount& fee,
-        bilingual_str& fail_reason) override
+        CAmount& fee) override
     {
         LOCK(m_wallet->cs_wallet);
-        FeeCalculation fee_calc_out;
-        std::optional<CreatedTransactionResult> txr = CreateTransaction(*m_wallet, recipients, change_pos,
-                fail_reason, coin_control, fee_calc_out, sign);
-        if (!txr) return {};
-        fee = txr->fee;
-        change_pos = txr->change_pos;
+        const auto& res = CreateTransaction(*m_wallet, recipients, change_pos,
+                                     coin_control, sign);
+        if (!res) return res.GetError();
+        const auto& txr = res.GetObj();
+        fee = txr.fee;
+        change_pos = txr.change_pos;
 
-        return txr->tx;
+        return txr.tx;
     }
     void commitTransaction(CTransactionRef tx,
         WalletValueMap value_map,

src/wallet/rpc/addresses.cpp

@@ -58,13 +58,12 @@ RPCHelpMan getnewaddress()
         output_type = parsed.value();
     }
 
-    CTxDestination dest;
-    bilingual_str error;
-    if (!pwallet->GetNewDestination(output_type, label, dest, error)) {
-        throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, error.original);
+    auto op_dest = pwallet->GetNewDestination(output_type, label);
+    if (!op_dest) {
+        throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, op_dest.GetError().original);
     }
 
-    return EncodeDestination(dest);
+    return EncodeDestination(op_dest.GetObj());
 },
     };
 }
@@ -106,12 +105,11 @@ RPCHelpMan getrawchangeaddress()
         output_type = parsed.value();
     }
 
-    CTxDestination dest;
-    bilingual_str error;
-    if (!pwallet->GetNewChangeDestination(output_type, dest, error)) {
-        throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, error.original);
+    auto op_dest = pwallet->GetNewChangeDestination(output_type);
+    if (!op_dest) {
+        throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, op_dest.GetError().original);
     }
-    return EncodeDestination(dest);
+    return EncodeDestination(op_dest.GetObj());
 },
     };
 }