diff options
author | Ryan Ofsky <ryan@ofsky.org> | 2022-07-21 08:30:39 -0400 |
---|---|---|
committer | Ryan Ofsky <ryan@ofsky.org> | 2022-08-03 07:33:01 -0400 |
commit | a23cca56c0a7f4a267915b4beba3af3454c51603 (patch) | |
tree | 5d85193889f5a4552a568cfd2b472670c2c6d513 | |
parent | 4a4289e2c98cfbc51b05716f21065838afed80f6 (diff) | |
download | bitcoin-a23cca56c0a7f4a267915b4beba3af3454c51603.tar.xz |
refactor: Replace BResult with util::Result
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 https://github.com/bitcoin/bitcoin/pull/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.
-rw-r--r-- | src/Makefile.test.include | 1 | ||||
-rw-r--r-- | src/bench/wallet_loading.cpp | 5 | ||||
-rw-r--r-- | src/interfaces/wallet.h | 6 | ||||
-rw-r--r-- | src/qt/addresstablemodel.cpp | 2 | ||||
-rw-r--r-- | src/qt/walletcontroller.cpp | 4 | ||||
-rw-r--r-- | src/qt/walletmodel.cpp | 4 | ||||
-rw-r--r-- | src/test/result_tests.cpp | 96 | ||||
-rw-r--r-- | src/test/util/wallet.cpp | 6 | ||||
-rw-r--r-- | src/util/result.h | 87 | ||||
-rw-r--r-- | src/wallet/feebumper.cpp | 4 | ||||
-rw-r--r-- | src/wallet/interfaces.cpp | 16 | ||||
-rw-r--r-- | src/wallet/rpc/addresses.cpp | 8 | ||||
-rw-r--r-- | src/wallet/rpc/spend.cpp | 6 | ||||
-rw-r--r-- | src/wallet/scriptpubkeyman.cpp | 20 | ||||
-rw-r--r-- | src/wallet/scriptpubkeyman.h | 6 | ||||
-rw-r--r-- | src/wallet/spend.cpp | 50 | ||||
-rw-r--r-- | src/wallet/spend.h | 2 | ||||
-rw-r--r-- | src/wallet/test/availablecoins_tests.cpp | 18 | ||||
-rw-r--r-- | src/wallet/test/coinselector_tests.cpp | 4 | ||||
-rw-r--r-- | src/wallet/test/fuzz/notifications.cpp | 5 | ||||
-rw-r--r-- | src/wallet/test/spend_tests.cpp | 2 | ||||
-rw-r--r-- | src/wallet/test/wallet_tests.cpp | 6 | ||||
-rw-r--r-- | src/wallet/wallet.cpp | 10 | ||||
-rw-r--r-- | src/wallet/wallet.h | 4 |
24 files changed, 247 insertions, 125 deletions
diff --git a/src/Makefile.test.include b/src/Makefile.test.include index f52964b033..9ed5731af1 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -119,6 +119,7 @@ BITCOIN_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 \ diff --git a/src/bench/wallet_loading.cpp b/src/bench/wallet_loading.cpp index a10f7ff7d1..27e4dd015d 100644 --- a/src/bench/wallet_loading.cpp +++ b/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{}); diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index e29fefae56..9bd033d991 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -88,7 +88,7 @@ public: 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; @@ -139,7 +139,7 @@ public: virtual void listLockedCoins(std::vector<COutPoint>& outputs) = 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, @@ -329,7 +329,7 @@ public: 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; diff --git a/src/qt/addresstablemodel.cpp b/src/qt/addresstablemodel.cpp index bb50b5384a..8b5da7f9f0 100644 --- a/src/qt/addresstablemodel.cpp +++ b/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 { diff --git a/src/qt/walletcontroller.cpp b/src/qt/walletcontroller.cpp index 01d84624e8..6e07dc09a0 100644 --- a/src/qt/walletcontroller.cpp +++ b/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); }); diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index fde136b727..ac6c8bea46 100644 --- a/src/qt/walletmodel.cpp +++ b/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; } diff --git a/src/test/result_tests.cpp b/src/test/result_tests.cpp new file mode 100644 index 0000000000..6a23a7b895 --- /dev/null +++ b/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() diff --git a/src/test/util/wallet.cpp b/src/test/util/wallet.cpp index 7a00ac9e1f..b54774cbb9 100644 --- a/src/test/util/wallet.cpp +++ b/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 diff --git a/src/util/result.h b/src/util/result.h index 2f586a4c9b..972b1aada0 100644 --- a/src/util/result.h +++ b/src/util/result.h @@ -5,45 +5,80 @@ #ifndef BITCOIN_UTIL_RESULT_H #define BITCOIN_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 // BITCOIN_UTIL_RESULT_H diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index 43fdcee48d..bda4c76bea 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -221,11 +221,11 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo constexpr int RANDOM_CHANGE_POSITION = -1; 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()); + errors.push_back(Untranslated("Unable to create transaction.") + Untranslated(" ") + util::ErrorString(res)); return Result::WALLET_ERROR; } - const auto& txr = res.GetObj(); + const auto& txr = *res; // Write back new fee if successful new_fee = txr.fee; diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 4f30060e29..91b188a59c 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -148,7 +148,7 @@ public: 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(); } - BResult<CTxDestination> getNewDestination(const OutputType type, const std::string label) override + util::Result<CTxDestination> getNewDestination(const OutputType type, const std::string label) override { LOCK(m_wallet->cs_wallet); return m_wallet->GetNewDestination(type, label); @@ -251,17 +251,17 @@ public: LOCK(m_wallet->cs_wallet); return m_wallet->ListLockedCoins(outputs); } - BResult<CTransactionRef> createTransaction(const std::vector<CRecipient>& recipients, + util::Result<CTransactionRef> createTransaction(const std::vector<CRecipient>& recipients, const CCoinControl& coin_control, bool sign, int& change_pos, CAmount& fee) override { LOCK(m_wallet->cs_wallet); - const auto& res = CreateTransaction(*m_wallet, recipients, change_pos, + auto res = CreateTransaction(*m_wallet, recipients, change_pos, coin_control, sign); - if (!res) return res.GetError(); - const auto& txr = res.GetObj(); + if (!res) return util::Error{util::ErrorString(res)}; + const auto& txr = *res; fee = txr.fee; change_pos = txr.change_pos; @@ -571,12 +571,12 @@ public: options.require_existing = true; return MakeWallet(m_context, LoadWallet(m_context, name, true /* load_on_start */, options, status, error, warnings)); } - BResult<std::unique_ptr<Wallet>> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, std::vector<bilingual_str>& warnings) override + util::Result<std::unique_ptr<Wallet>> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, std::vector<bilingual_str>& warnings) override { DatabaseStatus status; bilingual_str error; - BResult<std::unique_ptr<Wallet>> wallet{MakeWallet(m_context, RestoreWallet(m_context, backup_file, wallet_name, /*load_on_start=*/true, status, error, warnings))}; - if (!wallet) return error; + util::Result<std::unique_ptr<Wallet>> wallet{MakeWallet(m_context, RestoreWallet(m_context, backup_file, wallet_name, /*load_on_start=*/true, status, error, warnings))}; + if (!wallet) return util::Error{error}; return wallet; } std::string getWalletDir() override diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp index 2a6eb96871..148343a8b0 100644 --- a/src/wallet/rpc/addresses.cpp +++ b/src/wallet/rpc/addresses.cpp @@ -60,10 +60,10 @@ RPCHelpMan getnewaddress() auto op_dest = pwallet->GetNewDestination(output_type, label); if (!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); } - return EncodeDestination(op_dest.GetObj()); + return EncodeDestination(*op_dest); }, }; } @@ -107,9 +107,9 @@ RPCHelpMan getrawchangeaddress() auto op_dest = pwallet->GetNewChangeDestination(output_type); if (!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); } - return EncodeDestination(op_dest.GetObj()); + return EncodeDestination(*op_dest); }, }; } diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index e27cb1139e..628bf3cc99 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -158,14 +158,14 @@ UniValue SendMoney(CWallet& wallet, const CCoinControl &coin_control, std::vecto constexpr int RANDOM_CHANGE_POSITION = -1; auto res = CreateTransaction(wallet, recipients, RANDOM_CHANGE_POSITION, coin_control, true); if (!res) { - throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, res.GetError().original); + throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, util::ErrorString(res).original); } - const CTransactionRef& tx = res.GetObj().tx; + const CTransactionRef& tx = res->tx; wallet.CommitTransaction(tx, std::move(map_value), {} /* orderForm */); if (verbose) { UniValue entry(UniValue::VOBJ); entry.pushKV("txid", tx->GetHash().GetHex()); - entry.pushKV("fee_reason", StringForFeeReason(res.GetObj().fee_calc.reason)); + entry.pushKV("fee_reason", StringForFeeReason(res->fee_calc.reason)); return entry; } return tx->GetHash().GetHex(); diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index bba31dfe0b..2f242901ab 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -21,10 +21,10 @@ namespace wallet { //! Value for the first BIP 32 hardened derivation. Can be used as a bit mask and as a value. See BIP 32 for more details. const uint32_t BIP32_HARDENED_KEY_LIMIT = 0x80000000; -BResult<CTxDestination> LegacyScriptPubKeyMan::GetNewDestination(const OutputType type) +util::Result<CTxDestination> LegacyScriptPubKeyMan::GetNewDestination(const OutputType type) { if (LEGACY_OUTPUT_TYPES.count(type) == 0) { - return _("Error: Legacy wallets only support the \"legacy\", \"p2sh-segwit\", and \"bech32\" address types");; + return util::Error{_("Error: Legacy wallets only support the \"legacy\", \"p2sh-segwit\", and \"bech32\" address types")}; } assert(type != OutputType::BECH32M); @@ -33,7 +33,7 @@ BResult<CTxDestination> LegacyScriptPubKeyMan::GetNewDestination(const OutputTyp // Generate a new key that is added to wallet CPubKey new_key; if (!GetKeyFromPool(new_key, type)) { - return _("Error: Keypool ran out, please call keypoolrefill first"); + return util::Error{_("Error: Keypool ran out, please call keypoolrefill first")}; } LearnRelatedScripts(new_key, type); return GetDestinationForKey(new_key, type); @@ -1654,11 +1654,11 @@ std::set<CKeyID> LegacyScriptPubKeyMan::GetKeys() const return set_address; } -BResult<CTxDestination> DescriptorScriptPubKeyMan::GetNewDestination(const OutputType type) +util::Result<CTxDestination> DescriptorScriptPubKeyMan::GetNewDestination(const OutputType type) { // Returns true if this descriptor supports getting new addresses. Conditions where we may be unable to fetch them (e.g. locked) are caught later if (!CanGetAddresses()) { - return _("No addresses available"); + return util::Error{_("No addresses available")}; } { LOCK(cs_desc_man); @@ -1676,11 +1676,11 @@ BResult<CTxDestination> DescriptorScriptPubKeyMan::GetNewDestination(const Outpu std::vector<CScript> scripts_temp; if (m_wallet_descriptor.range_end <= m_max_cached_index && !TopUp(1)) { // We can't generate anymore keys - return _("Error: Keypool ran out, please call keypoolrefill first"); + return util::Error{_("Error: Keypool ran out, please call keypoolrefill first")}; } if (!m_wallet_descriptor.descriptor->ExpandFromCache(m_wallet_descriptor.next_index, m_wallet_descriptor.cache, scripts_temp, out_keys)) { // We can't generate anymore keys - return _("Error: Keypool ran out, please call keypoolrefill first"); + return util::Error{_("Error: Keypool ran out, please call keypoolrefill first")}; } CTxDestination dest; @@ -1766,11 +1766,11 @@ bool DescriptorScriptPubKeyMan::GetReservedDestination(const OutputType type, bo auto op_dest = GetNewDestination(type); index = m_wallet_descriptor.next_index - 1; if (op_dest) { - address = op_dest.GetObj(); + address = *op_dest; } else { - error = op_dest.GetError(); + error = util::ErrorString(op_dest); } - return op_dest.HasRes(); + return bool(op_dest); } void DescriptorScriptPubKeyMan::ReturnDestination(int64_t index, bool internal, const CTxDestination& addr) diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index eebc05330f..b772dde533 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -172,7 +172,7 @@ protected: public: explicit ScriptPubKeyMan(WalletStorage& storage) : m_storage(storage) {} virtual ~ScriptPubKeyMan() {}; - virtual BResult<CTxDestination> GetNewDestination(const OutputType type) { return Untranslated("Not supported"); } + virtual util::Result<CTxDestination> GetNewDestination(const OutputType type) { return util::Error{Untranslated("Not supported")}; } virtual isminetype IsMine(const CScript& script) const { return ISMINE_NO; } //! Check that the given decryption key is valid for this ScriptPubKeyMan, i.e. it decrypts all of the keys handled by it. @@ -360,7 +360,7 @@ private: public: using ScriptPubKeyMan::ScriptPubKeyMan; - BResult<CTxDestination> GetNewDestination(const OutputType type) override; + util::Result<CTxDestination> GetNewDestination(const OutputType type) override; isminetype IsMine(const CScript& script) const override; bool CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys = false) override; @@ -568,7 +568,7 @@ public: mutable RecursiveMutex cs_desc_man; - BResult<CTxDestination> GetNewDestination(const OutputType type) override; + util::Result<CTxDestination> GetNewDestination(const OutputType type) override; isminetype IsMine(const CScript& script) const override; bool CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys = false) override; diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index c00a2cd023..5d320feff0 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -758,7 +758,7 @@ static void DiscourageFeeSniping(CMutableTransaction& tx, FastRandomContext& rng } } -static BResult<CreatedTransactionResult> CreateTransactionInternal( +static util::Result<CreatedTransactionResult> CreateTransactionInternal( CWallet& wallet, const std::vector<CRecipient>& vecSend, int change_pos, @@ -844,11 +844,11 @@ static BResult<CreatedTransactionResult> CreateTransactionInternal( // Do not, ever, assume that it's fine to change the fee rate if the user has explicitly // provided one if (coin_control.m_feerate && coin_selection_params.m_effective_feerate > *coin_control.m_feerate) { - return strprintf(_("Fee rate (%s) is lower than the minimum fee rate setting (%s)"), coin_control.m_feerate->ToString(FeeEstimateMode::SAT_VB), coin_selection_params.m_effective_feerate.ToString(FeeEstimateMode::SAT_VB)); + return util::Error{strprintf(_("Fee rate (%s) is lower than the minimum fee rate setting (%s)"), coin_control.m_feerate->ToString(FeeEstimateMode::SAT_VB), coin_selection_params.m_effective_feerate.ToString(FeeEstimateMode::SAT_VB))}; } if (feeCalc.reason == FeeReason::FALLBACK && !wallet.m_allow_fallback_fee) { // eventually allow a fallback fee - return _("Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable -fallbackfee."); + return util::Error{_("Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable -fallbackfee.")}; } // Calculate the cost of change @@ -874,7 +874,7 @@ static BResult<CreatedTransactionResult> CreateTransactionInternal( } if (IsDust(txout, wallet.chain().relayDustFee())) { - return _("Transaction amount too small"); + return util::Error{_("Transaction amount too small")}; } txNew.vout.push_back(txout); } @@ -895,7 +895,7 @@ static BResult<CreatedTransactionResult> CreateTransactionInternal( // Choose coins to use std::optional<SelectionResult> result = SelectCoins(wallet, available_coins, /*nTargetValue=*/selection_target, coin_control, coin_selection_params); if (!result) { - return _("Insufficient funds"); + return util::Error{_("Insufficient funds")}; } TRACE5(coin_selection, selected_coins, wallet.GetName().c_str(), GetAlgorithmName(result->m_algo).c_str(), result->m_target, result->GetWaste(), result->GetSelectedValue()); @@ -910,7 +910,7 @@ static BResult<CreatedTransactionResult> CreateTransactionInternal( nChangePosInOut = rng_fast.randrange(txNew.vout.size() + 1); } else if ((unsigned int)nChangePosInOut > txNew.vout.size()) { - return _("Transaction change output index out of range"); + return util::Error{_("Transaction change output index out of range")}; } assert(nChangePosInOut != -1); @@ -937,7 +937,7 @@ static BResult<CreatedTransactionResult> CreateTransactionInternal( TxSize tx_sizes = CalculateMaximumSignedTxSize(CTransaction(txNew), &wallet, &coin_control); int nBytes = tx_sizes.vsize; if (nBytes == -1) { - return _("Missing solving data for estimating transaction size"); + return util::Error{_("Missing solving data for estimating transaction size")}; } nFeeRet = coin_selection_params.m_effective_feerate.GetFee(nBytes); @@ -997,9 +997,9 @@ static BResult<CreatedTransactionResult> CreateTransactionInternal( // Error if this output is reduced to be below dust if (IsDust(txout, wallet.chain().relayDustFee())) { if (txout.nValue < 0) { - return _("The transaction amount is too small to pay the fee"); + return util::Error{_("The transaction amount is too small to pay the fee")}; } else { - return _("The transaction amount is too small to send after the fee has been deducted"); + return util::Error{_("The transaction amount is too small to send after the fee has been deducted")}; } } } @@ -1010,11 +1010,11 @@ static BResult<CreatedTransactionResult> CreateTransactionInternal( // Give up if change keypool ran out and change is required if (scriptChange.empty() && nChangePosInOut != -1) { - return error; + return util::Error{error}; } if (sign && !wallet.SignTransaction(txNew)) { - return _("Signing transaction failed"); + return util::Error{_("Signing transaction failed")}; } // Return the constructed transaction data. @@ -1024,17 +1024,17 @@ static BResult<CreatedTransactionResult> CreateTransactionInternal( if ((sign && GetTransactionWeight(*tx) > MAX_STANDARD_TX_WEIGHT) || (!sign && tx_sizes.weight > MAX_STANDARD_TX_WEIGHT)) { - return _("Transaction too large"); + return util::Error{_("Transaction too large")}; } if (nFeeRet > wallet.m_default_max_tx_fee) { - return TransactionErrorString(TransactionError::MAX_FEE_EXCEEDED); + return util::Error{TransactionErrorString(TransactionError::MAX_FEE_EXCEEDED)}; } if (gArgs.GetBoolArg("-walletrejectlongchains", DEFAULT_WALLET_REJECT_LONG_CHAINS)) { // Lastly, ensure this tx will pass the mempool's chain limits if (!wallet.chain().checkChainLimits(tx)) { - return _("Transaction has too long of a mempool chain"); + return util::Error{_("Transaction has too long of a mempool chain")}; } } @@ -1053,7 +1053,7 @@ static BResult<CreatedTransactionResult> CreateTransactionInternal( return CreatedTransactionResult(tx, nFeeRet, nChangePosInOut, feeCalc); } -BResult<CreatedTransactionResult> CreateTransaction( +util::Result<CreatedTransactionResult> CreateTransaction( CWallet& wallet, const std::vector<CRecipient>& vecSend, int change_pos, @@ -1061,28 +1061,26 @@ BResult<CreatedTransactionResult> CreateTransaction( bool sign) { if (vecSend.empty()) { - return _("Transaction must have at least one recipient"); + return util::Error{_("Transaction must have at least one recipient")}; } if (std::any_of(vecSend.cbegin(), vecSend.cend(), [](const auto& recipient){ return recipient.nAmount < 0; })) { - return _("Transaction amounts must not be negative"); + return util::Error{_("Transaction amounts must not be negative")}; } LOCK(wallet.cs_wallet); auto res = CreateTransactionInternal(wallet, vecSend, change_pos, coin_control, sign); - TRACE4(coin_selection, normal_create_tx_internal, wallet.GetName().c_str(), res.HasRes(), - res ? res.GetObj().fee : 0, res ? res.GetObj().change_pos : 0); + TRACE4(coin_selection, normal_create_tx_internal, wallet.GetName().c_str(), bool(res), + res ? res->fee : 0, res ? res->change_pos : 0); if (!res) return res; - const auto& txr_ungrouped = res.GetObj(); + const auto& txr_ungrouped = *res; // try with avoidpartialspends unless it's enabled already if (txr_ungrouped.fee > 0 /* 0 means non-functional fee rate estimation */ && wallet.m_max_aps_fee > -1 && !coin_control.m_avoid_partial_spends) { TRACE1(coin_selection, attempting_aps_create_tx, wallet.GetName().c_str()); CCoinControl tmp_cc = coin_control; tmp_cc.m_avoid_partial_spends = true; - auto res_tx_grouped = CreateTransactionInternal(wallet, vecSend, change_pos, tmp_cc, sign); - // Helper optional class for now - std::optional<CreatedTransactionResult> txr_grouped{res_tx_grouped.HasRes() ? std::make_optional(res_tx_grouped.GetObj()) : std::nullopt}; + auto txr_grouped = CreateTransactionInternal(wallet, vecSend, change_pos, tmp_cc, sign); // if fee of this alternative one is within the range of the max fee, we use this one const bool use_aps{txr_grouped.has_value() ? (txr_grouped->fee <= txr_ungrouped.fee + wallet.m_max_aps_fee) : false}; TRACE5(coin_selection, aps_create_tx_internal, wallet.GetName().c_str(), use_aps, txr_grouped.has_value(), @@ -1090,7 +1088,7 @@ BResult<CreatedTransactionResult> CreateTransaction( if (txr_grouped) { wallet.WalletLogPrintf("Fee non-grouped = %lld, grouped = %lld, using %s\n", txr_ungrouped.fee, txr_grouped->fee, use_aps ? "grouped" : "non-grouped"); - if (use_aps) return res_tx_grouped; + if (use_aps) return txr_grouped; } } return res; @@ -1131,10 +1129,10 @@ bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet, auto res = CreateTransaction(wallet, vecSend, nChangePosInOut, coinControl, false); if (!res) { - error = res.GetError(); + error = util::ErrorString(res); return false; } - const auto& txr = res.GetObj(); + const auto& txr = *res; CTransactionRef tx_new = txr.tx; nFeeRet = txr.fee; nChangePosInOut = txr.change_pos; diff --git a/src/wallet/spend.h b/src/wallet/spend.h index 96ecd690be..fdb5113ba4 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -155,7 +155,7 @@ struct CreatedTransactionResult * selected by SelectCoins(); Also create the change output, when needed * @note passing change_pos as -1 will result in setting a random position */ -BResult<CreatedTransactionResult> CreateTransaction(CWallet& wallet, const std::vector<CRecipient>& vecSend, int change_pos, const CCoinControl& coin_control, bool sign = true); +util::Result<CreatedTransactionResult> CreateTransaction(CWallet& wallet, const std::vector<CRecipient>& vecSend, int change_pos, const CCoinControl& coin_control, bool sign = true); /** * Insert additional inputs into the transaction by diff --git a/src/wallet/test/availablecoins_tests.cpp b/src/wallet/test/availablecoins_tests.cpp index 01d24da981..3356128112 100644 --- a/src/wallet/test/availablecoins_tests.cpp +++ b/src/wallet/test/availablecoins_tests.cpp @@ -33,7 +33,7 @@ public: constexpr int RANDOM_CHANGE_POSITION = -1; auto res = CreateTransaction(*wallet, {recipient}, RANDOM_CHANGE_POSITION, dummy); BOOST_CHECK(res); - tx = res.GetObj().tx; + tx = res->tx; } wallet->CommitTransaction(tx, {}, {}); CMutableTransaction blocktx; @@ -57,7 +57,7 @@ public: BOOST_FIXTURE_TEST_CASE(BasicOutputTypesTest, AvailableCoinsTestingSetup) { CoinsResult available_coins; - BResult<CTxDestination> dest; + util::Result<CTxDestination> dest{util::Error{}}; LOCK(wallet->cs_wallet); // Verify our wallet has one usable coinbase UTXO before starting @@ -75,28 +75,28 @@ BOOST_FIXTURE_TEST_CASE(BasicOutputTypesTest, AvailableCoinsTestingSetup) // Bech32m dest = wallet->GetNewDestination(OutputType::BECH32M, ""); - BOOST_ASSERT(dest.HasRes()); - AddTx(CRecipient{{GetScriptForDestination(dest.GetObj())}, 1 * COIN, /*fSubtractFeeFromAmount=*/true}); + BOOST_ASSERT(dest); + AddTx(CRecipient{{GetScriptForDestination(*dest)}, 1 * COIN, /*fSubtractFeeFromAmount=*/true}); available_coins = AvailableCoins(*wallet); BOOST_CHECK_EQUAL(available_coins.bech32m.size(), 2U); // Bech32 dest = wallet->GetNewDestination(OutputType::BECH32, ""); - BOOST_ASSERT(dest.HasRes()); - AddTx(CRecipient{{GetScriptForDestination(dest.GetObj())}, 2 * COIN, /*fSubtractFeeFromAmount=*/true}); + BOOST_ASSERT(dest); + AddTx(CRecipient{{GetScriptForDestination(*dest)}, 2 * COIN, /*fSubtractFeeFromAmount=*/true}); available_coins = AvailableCoins(*wallet); BOOST_CHECK_EQUAL(available_coins.bech32.size(), 2U); // P2SH-SEGWIT dest = wallet->GetNewDestination(OutputType::P2SH_SEGWIT, ""); - AddTx(CRecipient{{GetScriptForDestination(dest.GetObj())}, 3 * COIN, /*fSubtractFeeFromAmount=*/true}); + AddTx(CRecipient{{GetScriptForDestination(*dest)}, 3 * COIN, /*fSubtractFeeFromAmount=*/true}); available_coins = AvailableCoins(*wallet); BOOST_CHECK_EQUAL(available_coins.P2SH_segwit.size(), 2U); // Legacy (P2PKH) dest = wallet->GetNewDestination(OutputType::LEGACY, ""); - BOOST_ASSERT(dest.HasRes()); - AddTx(CRecipient{{GetScriptForDestination(dest.GetObj())}, 4 * COIN, /*fSubtractFeeFromAmount=*/true}); + BOOST_ASSERT(dest); + AddTx(CRecipient{{GetScriptForDestination(*dest)}, 4 * COIN, /*fSubtractFeeFromAmount=*/true}); available_coins = AvailableCoins(*wallet); BOOST_CHECK_EQUAL(available_coins.legacy.size(), 2U); } diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index cd7fd3f4dd..a4a7216436 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -74,9 +74,7 @@ static void add_coin(CoinsResult& available_coins, CWallet& wallet, const CAmoun tx.vout.resize(nInput + 1); tx.vout[nInput].nValue = nValue; if (spendable) { - 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, ""))); } uint256 txid = tx.GetHash(); diff --git a/src/wallet/test/fuzz/notifications.cpp b/src/wallet/test/fuzz/notifications.cpp index 5c173773e4..5e9cd4001b 100644 --- a/src/wallet/test/fuzz/notifications.cpp +++ b/src/wallet/test/fuzz/notifications.cpp @@ -69,14 +69,13 @@ struct FuzzedWallet { CScript GetScriptPubKey(FuzzedDataProvider& fuzzed_data_provider) { auto type{fuzzed_data_provider.PickValueInArray(OUTPUT_TYPES)}; - BResult<CTxDestination> op_dest; + util::Result<CTxDestination> op_dest{util::Error{}}; 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)); } }; diff --git a/src/wallet/test/spend_tests.cpp b/src/wallet/test/spend_tests.cpp index 53c3b5d2ae..dc935a1a04 100644 --- a/src/wallet/test/spend_tests.cpp +++ b/src/wallet/test/spend_tests.cpp @@ -35,7 +35,7 @@ BOOST_FIXTURE_TEST_CASE(SubtractFee, TestChain100Setup) coin_control.m_change_type = OutputType::LEGACY; auto res = CreateTransaction(*wallet, {recipient}, RANDOM_CHANGE_POSITION, coin_control); BOOST_CHECK(res); - const auto& txr = res.GetObj(); + const auto& txr = *res; BOOST_CHECK_EQUAL(txr.tx->vout.size(), 1); BOOST_CHECK_EQUAL(txr.tx->vout[0].nValue, recipient.nAmount + leftover_input_amount - txr.fee); BOOST_CHECK_GT(txr.fee, 0); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index d3a760742d..1d2235be3d 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -536,7 +536,7 @@ public: constexpr int RANDOM_CHANGE_POSITION = -1; auto res = CreateTransaction(*wallet, {recipient}, RANDOM_CHANGE_POSITION, dummy); BOOST_CHECK(res); - tx = res.GetObj().tx; + tx = res->tx; } wallet->CommitTransaction(tx, {}, {}); CMutableTransaction blocktx; @@ -918,8 +918,8 @@ BOOST_FIXTURE_TEST_CASE(wallet_sync_tx_invalid_state_test, TestingSetup) // Add tx to wallet const auto& op_dest = wallet.GetNewDestination(OutputType::BECH32M, ""); - BOOST_ASSERT(op_dest.HasRes()); - const CTxDestination& dest = op_dest.GetObj(); + BOOST_ASSERT(op_dest); + const CTxDestination& dest = *op_dest; CMutableTransaction mtx; mtx.vout.push_back({COIN, GetScriptForDestination(dest)}); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 9a48a1ca21..e750cd5a2c 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2330,24 +2330,24 @@ bool CWallet::TopUpKeyPool(unsigned int kpSize) return res; } -BResult<CTxDestination> CWallet::GetNewDestination(const OutputType type, const std::string label) +util::Result<CTxDestination> CWallet::GetNewDestination(const OutputType type, const std::string label) { LOCK(cs_wallet); auto spk_man = GetScriptPubKeyMan(type, false /* internal */); if (!spk_man) { - return strprintf(_("Error: No %s addresses available."), FormatOutputType(type)); + return util::Error{strprintf(_("Error: No %s addresses available."), FormatOutputType(type))}; } spk_man->TopUp(); auto op_dest = spk_man->GetNewDestination(type); if (op_dest) { - SetAddressBook(op_dest.GetObj(), label, "receive"); + SetAddressBook(*op_dest, label, "receive"); } return op_dest; } -BResult<CTxDestination> CWallet::GetNewChangeDestination(const OutputType type) +util::Result<CTxDestination> CWallet::GetNewChangeDestination(const OutputType type) { LOCK(cs_wallet); @@ -2355,7 +2355,7 @@ BResult<CTxDestination> CWallet::GetNewChangeDestination(const OutputType type) bilingual_str error; ReserveDestination reservedest(this, type); if (!reservedest.GetReservedDestination(dest, true, error)) { - return error; + return util::Error{error}; } reservedest.KeepDestination(); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 20f0c3579c..f2b9723840 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -670,8 +670,8 @@ public: */ void MarkDestinationsDirty(const std::set<CTxDestination>& destinations) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - BResult<CTxDestination> GetNewDestination(const OutputType type, const std::string label); - BResult<CTxDestination> GetNewChangeDestination(const OutputType type); + util::Result<CTxDestination> GetNewDestination(const OutputType type, const std::string label); + util::Result<CTxDestination> GetNewChangeDestination(const OutputType type); isminetype IsMine(const CTxDestination& dest) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); isminetype IsMine(const CScript& script) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); |