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

src/Makefile.test.include

@@ -132,6 +132,7 @@ SYSCOIN_TESTS =\
   test/random_tests.cpp \
   test/rbf_tests.cpp \
   test/rest_tests.cpp \
+  test/result_tests.cpp \
   test/reverselock_tests.cpp \
   test/rpc_tests.cpp \
   test/sanity_tests.cpp \

src/bench/wallet_loading.cpp

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

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;
 
     //! Get public key.
     virtual bool getPubKey(const CScript& script, const CKeyID& address, CPubKey& pub_key) = 0;
@@ -142,7 +142,7 @@ class Wallet
     // SYSCOIN
     virtual void listProTxCoins(std::vector<COutPoint>& vOutpts) = 0;
     //! Create transaction.
-    virtual BResult<CTransactionRef> createTransaction(const std::vector<wallet::CRecipient>& recipients,
+    virtual util::Result<CTransactionRef> createTransaction(const std::vector<wallet::CRecipient>& recipients,
         const wallet::CCoinControl& coin_control,
         bool sign,
         int& change_pos,
@@ -332,7 +332,7 @@ class WalletLoader : public ChainClient
    virtual std::string getWalletDir() = 0;
 
    //! Restore backup wallet
-   virtual BResult<std::unique_ptr<Wallet>> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, std::vector<bilingual_str>& warnings) = 0;
+   virtual util::Result<std::unique_ptr<Wallet>> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, std::vector<bilingual_str>& warnings) = 0;
 
    //! Return available wallets in wallet directory.
    virtual std::vector<std::string> listWalletDir() = 0;

src/net.cpp

@@ -1973,7 +1973,7 @@ void CConnman::ThreadOpenMasternodeConnections()
         if (interruptNet)
             return;
 
-        int64_t nANow = GetAdjustedTime();
+        int64_t nANow = Now<NodeSeconds>().time_since_epoch().count();
 
         // NOTE: Process only one pending masternode at a time
 

src/qt/addresstablemodel.cpp

@@ -384,7 +384,7 @@ QString AddressTableModel::addRow(const QString &type, const QString &label, con
                 return QString();
             }
         }
-        strAddress = EncodeDestination(op_dest.GetObj());
+        strAddress = EncodeDestination(*op_dest);
     }
     else
     {

src/qt/walletcontroller.cpp

@@ -393,8 +393,8 @@ void RestoreWalletActivity::restore(const fs::path& backup_file, const std::stri
     QTimer::singleShot(0, worker(), [this, backup_file, wallet_name] {
         auto wallet{node().walletLoader().restoreWallet(backup_file, wallet_name, m_warning_message)};
 
-        m_error_message = wallet ? bilingual_str{} : wallet.GetError();
-        if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(wallet.ReleaseObj());
+        m_error_message = util::ErrorString(wallet);
+        if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(*wallet));
 
         QTimer::singleShot(0, this, &RestoreWalletActivity::finish);
     });

src/qt/walletmodel.cpp

@@ -207,7 +207,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact
 
         auto& newTx = transaction.getWtx();
         const auto& res = m_wallet->createTransaction(vecSend, coinControl, !wallet().privateKeysDisabled() /* sign */, nChangePosRet, nFeeRequired);
-        newTx = res ? res.GetObj() : nullptr;
+        newTx = res ? *res : 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(res.GetError().translated),
+            Q_EMIT message(tr("Send Coins"), QString::fromStdString(util::ErrorString(res).translated),
                 CClientUIInterface::MSG_ERROR);
             return TransactionCreationFailed;
         }

src/services/rpc/wallet/assetwalletrpc.cpp

@@ -33,6 +33,7 @@
 #include <index/txindex.h>
 #include <node/context.h>
 #include <services/assetconsensus.h>
+#include <util/result.h>
 using namespace wallet;
 extern std::string EncodeDestination(const CTxDestination& dest);
 extern CTxDestination DecodeDestination(const std::string& str);
@@ -370,10 +371,10 @@ static RPCHelpMan syscoinburntoassetallocation()
 
     auto op_dest = pwallet->GetNewChangeDestination(pwallet->m_default_address_type);
     if (fChangeAddress.empty() && !op_dest) {
-        throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, op_dest.GetError().original);
+        throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, util::ErrorString(op_dest).original);
     }
 
-    const CScript& scriptPubKey = GetScriptForDestination(op_dest.GetObj());
+    const CScript& scriptPubKey = GetScriptForDestination(*op_dest);
     CTxOut change_prototype_txout(0, scriptPubKey);
     CRecipient recp = {scriptPubKey, GetDustThreshold(change_prototype_txout, GetDiscardRate(*pwallet)), false };
 
@@ -415,8 +416,8 @@ static RPCHelpMan syscoinburntoassetallocation()
     CTransactionRef tx;
     FeeCalculation fee_calc_out;
     auto resTx = CreateTransaction(*pwallet, vecSend, nChangePosInOut, error, coin_control, fee_calc_out, false /* sign*/, SYSCOIN_TX_VERSION_SYSCOIN_BURN_TO_ALLOCATION);
-    if (!resTx) throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, error.original);
-    auto &txr = resTx.GetObj();
+    if (!resTx) throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, util::ErrorString(resTx).original);
+    auto &txr = *resTx;
     tx = txr.tx;
     CMutableTransaction mtx(*tx);
     // Script verification errors
@@ -639,9 +640,9 @@ RPCHelpMan assetnew()
 
     auto op_dest = pwallet->GetNewChangeDestination(pwallet->m_default_address_type);
     if (fChangeAddress.empty() && !op_dest) {
-        throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, op_dest.GetError().original);
+        throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, util::ErrorString(op_dest).original);
     }
-    auto dest = op_dest.GetObj();
+    auto dest = *op_dest;
     if(!fChangeAddress.empty())
         dest = DecodeDestination(fChangeAddress);
     if (!IsValidDestination(dest)) {
@@ -864,9 +865,9 @@ UniValue CreateAssetUpdateTx(const std::any& context, const int32_t& nVersionIn,
     if(!recpIn || nGas > (MIN_CHANGE + pwallet.m_default_max_tx_fee)) {
         auto op_dest = pwallet.GetNewChangeDestination(pwallet.m_default_address_type);
         if (fChangeAddress.empty() && !op_dest) {
-            throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, op_dest.GetError().original);
+            throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, util::ErrorString(op_dest).original);
         }     
-        recp = { GetScriptForDestination(op_dest.GetObj()), nGas, false};  
+        recp = { GetScriptForDestination(*op_dest), nGas, false};  
     }
     // if enough for change + max fee, we try to take fee from this output
     if(nGas > (MIN_CHANGE + pwallet.m_default_max_tx_fee)) {
@@ -895,8 +896,8 @@ UniValue CreateAssetUpdateTx(const std::any& context, const int32_t& nVersionIn,
     CTransactionRef tx;
     FeeCalculation fee_calc_out;
     auto resTx = CreateTransaction(pwallet, vecSend, nChangePosInOut, error, coin_control, fee_calc_out, false /* sign*/, nVersionIn);
-    if (!resTx) throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, error.original);
-    auto &txr = resTx.GetObj();
+    if (!resTx) throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, util::ErrorString(resTx).original);
+    auto &txr = *resTx;
     tx = txr.tx;
 
     CMutableTransaction mtx(*tx);
@@ -1840,10 +1841,10 @@ static RPCHelpMan assetallocationburn()
 
     auto op_dest = pwallet->GetNewChangeDestination(pwallet->m_default_address_type);
     if (fChangeAddress.empty() && !op_dest) {
-        throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, op_dest.GetError().original);
+        throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, util::ErrorString(op_dest).original);
     }
 
-    const CScript& scriptPubKey = GetScriptForDestination(op_dest.GetObj());
+    const CScript& scriptPubKey = GetScriptForDestination(*op_dest);
     CRecipient recp = {scriptPubKey, nAmount, false };
 
     std::vector<unsigned char> data;

src/test/result_tests.cpp

@@ -0,0 +1,96 @@
+// Copyright (c) 2022 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <util/result.h>
+
+#include <boost/test/unit_test.hpp>
+
+inline bool operator==(const bilingual_str& a, const bilingual_str& b)
+{
+    return a.original == b.original && a.translated == b.translated;
+}
+
+inline std::ostream& operator<<(std::ostream& os, const bilingual_str& s)
+{
+    return os << "bilingual_str('" << s.original << "' , '" << s.translated << "')";
+}
+
+BOOST_AUTO_TEST_SUITE(result_tests)
+
+struct NoCopy {
+    NoCopy(int n) : m_n{std::make_unique<int>(n)} {}
+    std::unique_ptr<int> m_n;
+};
+
+bool operator==(const NoCopy& a, const NoCopy& b)
+{
+    return *a.m_n == *b.m_n;
+}
+
+std::ostream& operator<<(std::ostream& os, const NoCopy& o)
+{
+    return os << "NoCopy(" << *o.m_n << ")";
+}
+
+util::Result<int> IntFn(int i, bool success)
+{
+    if (success) return i;
+    return util::Error{Untranslated(strprintf("int %i error.", i))};
+}
+
+util::Result<bilingual_str> StrFn(bilingual_str s, bool success)
+{
+    if (success) return s;
+    return util::Error{strprintf(Untranslated("str %s error."), s.original)};
+}
+
+util::Result<NoCopy> NoCopyFn(int i, bool success)
+{
+    if (success) return {i};
+    return util::Error{Untranslated(strprintf("nocopy %i error.", i))};
+}
+
+template <typename T>
+void ExpectResult(const util::Result<T>& result, bool success, const bilingual_str& str)
+{
+    BOOST_CHECK_EQUAL(bool(result), success);
+    BOOST_CHECK_EQUAL(util::ErrorString(result), str);
+}
+
+template <typename T, typename... Args>
+void ExpectSuccess(const util::Result<T>& result, const bilingual_str& str, Args&&... args)
+{
+    ExpectResult(result, true, str);
+    BOOST_CHECK_EQUAL(result.has_value(), true);
+    BOOST_CHECK_EQUAL(result.value(), T{std::forward<Args>(args)...});
+    BOOST_CHECK_EQUAL(&result.value(), &*result);
+}
+
+template <typename T, typename... Args>
+void ExpectFail(const util::Result<T>& result, const bilingual_str& str)
+{
+    ExpectResult(result, false, str);
+}
+
+BOOST_AUTO_TEST_CASE(check_returned)
+{
+    ExpectSuccess(IntFn(5, true), {}, 5);
+    ExpectFail(IntFn(5, false), Untranslated("int 5 error."));
+    ExpectSuccess(NoCopyFn(5, true), {}, 5);
+    ExpectFail(NoCopyFn(5, false), Untranslated("nocopy 5 error."));
+    ExpectSuccess(StrFn(Untranslated("S"), true), {}, Untranslated("S"));
+    ExpectFail(StrFn(Untranslated("S"), false), Untranslated("str S error."));
+}
+
+BOOST_AUTO_TEST_CASE(check_value_or)
+{
+    BOOST_CHECK_EQUAL(IntFn(10, true).value_or(20), 10);
+    BOOST_CHECK_EQUAL(IntFn(10, false).value_or(20), 20);
+    BOOST_CHECK_EQUAL(NoCopyFn(10, true).value_or(20), 10);
+    BOOST_CHECK_EQUAL(NoCopyFn(10, false).value_or(20), 20);
+    BOOST_CHECK_EQUAL(StrFn(Untranslated("A"), true).value_or(Untranslated("B")), Untranslated("A"));
+    BOOST_CHECK_EQUAL(StrFn(Untranslated("A"), false).value_or(Untranslated("B")), Untranslated("B"));
+}
+
+BOOST_AUTO_TEST_SUITE_END()

src/test/util/wallet.cpp

@@ -8,6 +8,7 @@
 #include <outputtype.h>
 #include <script/standard.h>
 #ifdef ENABLE_WALLET
+#include <util/check.h>
 #include <util/translation.h>
 #include <wallet/wallet.h>
 #endif
@@ -20,10 +21,7 @@ const std::string ADDRESS_BCRT1_UNSPENDABLE = "bcrt1qqqqqqqqqqqqqqqqqqqqqqqqqqqq
 std::string getnewaddress(CWallet& w)
 {
     constexpr auto output_type = OutputType::BECH32;
-    auto op_dest = w.GetNewDestination(output_type, "");
-    assert(op_dest.HasRes());
-
-    return EncodeDestination(op_dest.GetObj());
+    return EncodeDestination(*Assert(w.GetNewDestination(output_type, "")));
 }
 
 #endif // ENABLE_WALLET

src/util/result.h

@@ -5,45 +5,80 @@
 #ifndef SYSCOIN_UTIL_RESULT_H
 #define SYSCOIN_UTIL_RESULT_H
 
+#include <attributes.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 {
+namespace util {
+
+struct Error {
+    bilingual_str message;
+};
+
+//! The util::Result class provides a standard way for functions to return
+//! either error messages or result values.
+//!
+//! It is intended for high-level functions that need to report error strings to
+//! end users. Lower-level functions that don't need this error-reporting and
+//! only need error-handling should avoid util::Result and instead use standard
+//! classes like std::optional, std::variant, and std::tuple, or custom structs
+//! and enum types to return function results.
+//!
+//! Usage examples can be found in \example ../test/result_tests.cpp, but in
+//! general code returning `util::Result<T>` values is very similar to code
+//! returning `std::optional<T>` values. Existing functions returning
+//! `std::optional<T>` can be updated to return `util::Result<T>` and return
+//! error strings usually just replacing `return std::nullopt;` with `return
+//! util::Error{error_string};`.
+template <class T>
+class Result
+{
 private:
     std::variant<bilingual_str, T> m_variant;
 
-public:
-    BResult() : m_variant{Untranslated("")} {}
-    BResult(T obj) : m_variant{std::move(obj)} {}
-    BResult(bilingual_str error) : m_variant{std::move(error)} {}
+    template <typename FT>
+    friend bilingual_str ErrorString(const Result<FT>& result);
 
-    /* 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)} {}
+    Result(Error error) : m_variant{std::in_place_index_t<0>{}, std::move(error.message)} {}
 
-    /* In case of success, the result object */
-    const T& GetObj() const {
-        assert(HasRes());
-        return std::get<T>(m_variant);
+    //! 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 noexcept { return m_variant.index() == 1; }
+    const T& value() const LIFETIMEBOUND
+    {
+        assert(has_value());
+        return std::get<1>(m_variant);
     }
-    T ReleaseObj()
+    T& value() LIFETIMEBOUND
     {
-        assert(HasRes());
-        return std::move(std::get<T>(m_variant));
+        assert(has_value());
+        return std::get<1>(m_variant);
     }
-
-    /* In case of failure, the error cause */
-    const bilingual_str& GetError() const {
-        assert(!HasRes());
-        return std::get<bilingual_str>(m_variant);
+    template <class U>
+    T value_or(U&& default_value) const&
+    {
+        return has_value() ? value() : std::forward<U>(default_value);
     }
-
-    explicit operator bool() const { return HasRes(); }
+    template <class U>
+    T value_or(U&& default_value) &&
+    {
+        return has_value() ? std::move(value()) : std::forward<U>(default_value);
+    }
+    explicit operator bool() const noexcept { return has_value(); }
+    const T* operator->() const LIFETIMEBOUND { return &value(); }
+    const T& operator*() const LIFETIMEBOUND { return value(); }
+    T* operator->() LIFETIMEBOUND { return &value(); }
+    T& operator*() LIFETIMEBOUND { return value(); }
 };
 
+template <typename T>
+bilingual_str ErrorString(const Result<T>& result)
+{
+    return result ? bilingual_str{} : std::get<0>(result.m_variant);
+}
+} // namespace util
+
 #endif // SYSCOIN_UTIL_RESULT_H