diff options
-rw-r--r-- | src/Makefile.am | 1 | ||||
-rw-r--r-- | src/bench/wallet_loading.cpp | 7 | ||||
-rw-r--r-- | src/interfaces/wallet.h | 8 | ||||
-rw-r--r-- | src/qt/addresstablemodel.cpp | 14 | ||||
-rw-r--r-- | src/qt/walletmodel.cpp | 6 | ||||
-rw-r--r-- | src/test/util/wallet.cpp | 7 | ||||
-rw-r--r-- | src/util/result.h | 43 | ||||
-rw-r--r-- | src/wallet/feebumper.cpp | 13 | ||||
-rw-r--r-- | src/wallet/interfaces.cpp | 24 | ||||
-rw-r--r-- | src/wallet/rpc/addresses.cpp | 18 | ||||
-rw-r--r-- | src/wallet/rpc/spend.cpp | 12 | ||||
-rw-r--r-- | src/wallet/scriptpubkeyman.cpp | 35 | ||||
-rw-r--r-- | src/wallet/scriptpubkeyman.h | 7 | ||||
-rw-r--r-- | src/wallet/spend.cpp | 103 | ||||
-rw-r--r-- | src/wallet/spend.h | 9 | ||||
-rw-r--r-- | src/wallet/test/coinselector_tests.cpp | 8 | ||||
-rw-r--r-- | src/wallet/test/fuzz/notifications.cpp | 11 | ||||
-rw-r--r-- | src/wallet/test/spend_tests.cpp | 15 | ||||
-rw-r--r-- | src/wallet/test/wallet_tests.cpp | 18 | ||||
-rw-r--r-- | src/wallet/wallet.cpp | 29 | ||||
-rw-r--r-- | src/wallet/wallet.h | 6 |
21 files changed, 201 insertions, 193 deletions
diff --git a/src/Makefile.am b/src/Makefile.am index 3fbbe180fc..a9e9db0a7d 100644 --- a/src/Makefile.am +++ b/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 \ diff --git a/src/bench/wallet_loading.cpp b/src/bench/wallet_loading.cpp index f611383788..d258d7a29e 100644 --- a/src/bench/wallet_loading.cpp +++ b/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{}); diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index b3cb0ae387..fe198c999b 100644 --- a/src/interfaces/wallet.h +++ b/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 @@ public: 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 @@ public: 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, diff --git a/src/qt/addresstablemodel.cpp b/src/qt/addresstablemodel.cpp index 27ee9509e6..bb50b5384a 100644 --- a/src/qt/addresstablemodel.cpp +++ b/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 { diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index ab4d1cae3f..bb6079afee 100644 --- a/src/qt/walletmodel.cpp +++ b/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; } diff --git a/src/test/util/wallet.cpp b/src/test/util/wallet.cpp index 52aaeabccf..7a00ac9e1f 100644 --- a/src/test/util/wallet.cpp +++ b/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 diff --git a/src/util/result.h b/src/util/result.h new file mode 100644 index 0000000000..dcf5edaa5b --- /dev/null +++ b/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 diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index 5e70ed4a30..43fdcee48d 100644 --- a/src/wallet/feebumper.cpp +++ b/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; } diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 327276f2ed..547d972c8d 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -146,11 +146,10 @@ 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(); } - 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 @@ public: 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, diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp index da4cc44ee6..9428d049de 100644 --- a/src/wallet/rpc/addresses.cpp +++ b/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()); }, }; } diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index 0e8cee5db8..22c1f2bdc2 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -156,18 +156,16 @@ UniValue SendMoney(CWallet& wallet, const CCoinControl &coin_control, std::vecto // Send constexpr int RANDOM_CHANGE_POSITION = -1; - bilingual_str error; - FeeCalculation fee_calc_out; - std::optional<CreatedTransactionResult> txr = CreateTransaction(wallet, recipients, RANDOM_CHANGE_POSITION, error, coin_control, fee_calc_out, true); - if (!txr) { - throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, error.original); + auto res = CreateTransaction(wallet, recipients, RANDOM_CHANGE_POSITION, coin_control, true); + if (!res) { + throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, res.GetError().original); } - CTransactionRef tx = txr->tx; + const CTransactionRef& tx = res.GetObj().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(fee_calc_out.reason)); + entry.pushKV("fee_reason", StringForFeeReason(res.GetObj().fee_calc.reason)); return entry; } return tx->GetHash().GetHex(); diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 1fec82a485..bba31dfe0b 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -21,26 +21,22 @@ 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; -bool LegacyScriptPubKeyMan::GetNewDestination(const OutputType type, CTxDestination& dest, bilingual_str& error) +BResult<CTxDestination> LegacyScriptPubKeyMan::GetNewDestination(const OutputType type) { if (LEGACY_OUTPUT_TYPES.count(type) == 0) { - error = _("Error: Legacy wallets only support the \"legacy\", \"p2sh-segwit\", and \"bech32\" address types"); - return false; + return _("Error: Legacy wallets only support the \"legacy\", \"p2sh-segwit\", and \"bech32\" address types");; } assert(type != OutputType::BECH32M); LOCK(cs_KeyStore); - error.clear(); // Generate a new key that is added to wallet CPubKey new_key; if (!GetKeyFromPool(new_key, type)) { - error = _("Error: Keypool ran out, please call keypoolrefill first"); - return false; + return _("Error: Keypool ran out, please call keypoolrefill first"); } LearnRelatedScripts(new_key, type); - dest = GetDestinationForKey(new_key, type); - return true; + return GetDestinationForKey(new_key, type); } typedef std::vector<unsigned char> valtype; @@ -1658,12 +1654,11 @@ std::set<CKeyID> LegacyScriptPubKeyMan::GetKeys() const return set_address; } -bool DescriptorScriptPubKeyMan::GetNewDestination(const OutputType type, CTxDestination& dest, bilingual_str& error) +BResult<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()) { - error = _("No addresses available"); - return false; + return _("No addresses available"); } { LOCK(cs_desc_man); @@ -1681,15 +1676,14 @@ bool DescriptorScriptPubKeyMan::GetNewDestination(const OutputType type, CTxDest std::vector<CScript> scripts_temp; if (m_wallet_descriptor.range_end <= m_max_cached_index && !TopUp(1)) { // We can't generate anymore keys - error = _("Error: Keypool ran out, please call keypoolrefill first"); - return false; + return _("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 - error = _("Error: Keypool ran out, please call keypoolrefill first"); - return false; + return _("Error: Keypool ran out, please call keypoolrefill first"); } + CTxDestination dest; std::optional<OutputType> out_script_type = m_wallet_descriptor.descriptor->GetOutputType(); if (out_script_type && out_script_type == type) { ExtractDestination(scripts_temp[0], dest); @@ -1698,7 +1692,7 @@ bool DescriptorScriptPubKeyMan::GetNewDestination(const OutputType type, CTxDest } m_wallet_descriptor.next_index++; WalletBatch(m_storage.GetDatabase()).WriteDescriptor(GetID(), m_wallet_descriptor); - return true; + return dest; } } @@ -1769,9 +1763,14 @@ bool DescriptorScriptPubKeyMan::Encrypt(const CKeyingMaterial& master_key, Walle bool DescriptorScriptPubKeyMan::GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool, bilingual_str& error) { LOCK(cs_desc_man); - bool result = GetNewDestination(type, address, error); + auto op_dest = GetNewDestination(type); index = m_wallet_descriptor.next_index - 1; - return result; + if (op_dest) { + address = op_dest.GetObj(); + } else { + error = op_dest.GetError(); + } + return op_dest.HasRes(); } void DescriptorScriptPubKeyMan::ReturnDestination(int64_t index, bool internal, const CTxDestination& addr) diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index ad924791af..eebc05330f 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -11,6 +11,7 @@ #include <script/standard.h> #include <util/error.h> #include <util/message.h> +#include <util/result.h> #include <util/time.h> #include <wallet/crypter.h> #include <wallet/ismine.h> @@ -171,7 +172,7 @@ protected: public: explicit ScriptPubKeyMan(WalletStorage& storage) : m_storage(storage) {} virtual ~ScriptPubKeyMan() {}; - virtual bool GetNewDestination(const OutputType type, CTxDestination& dest, bilingual_str& error) { return false; } + virtual BResult<CTxDestination> GetNewDestination(const OutputType type) { return 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. @@ -359,7 +360,7 @@ private: public: using ScriptPubKeyMan::ScriptPubKeyMan; - bool GetNewDestination(const OutputType type, CTxDestination& dest, bilingual_str& error) override; + BResult<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; @@ -567,7 +568,7 @@ public: mutable RecursiveMutex cs_desc_man; - bool GetNewDestination(const OutputType type, CTxDestination& dest, bilingual_str& error) override; + BResult<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 21007f5a92..9be29c4709 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -652,19 +652,16 @@ static void DiscourageFeeSniping(CMutableTransaction& tx, FastRandomContext& rng } } -static std::optional<CreatedTransactionResult> CreateTransactionInternal( +static BResult<CreatedTransactionResult> CreateTransactionInternal( CWallet& wallet, const std::vector<CRecipient>& vecSend, int change_pos, - bilingual_str& error, const CCoinControl& coin_control, - FeeCalculation& fee_calc_out, bool sign) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) { AssertLockHeld(wallet.cs_wallet); // out variables, to be packed into returned result structure - CTransactionRef tx; CAmount nFeeRet; int nChangePosInOut = change_pos; @@ -693,6 +690,7 @@ static std::optional<CreatedTransactionResult> CreateTransactionInternal( // Create change script that will be used if we need change CScript scriptChange; + bilingual_str error; // possible error str // coin control: send change to custom address if (!std::get_if<CNoDestination>(&coin_control.destChange)) { @@ -740,13 +738,11 @@ static std::optional<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) { - 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)); - return std::nullopt; + 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)); } if (feeCalc.reason == FeeReason::FALLBACK && !wallet.m_allow_fallback_fee) { // eventually allow a fallback fee - error = _("Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable -fallbackfee."); - return std::nullopt; + return _("Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable -fallbackfee."); } // Calculate the cost of change @@ -771,10 +767,8 @@ static std::optional<CreatedTransactionResult> CreateTransactionInternal( coin_selection_params.tx_noinputs_size += ::GetSerializeSize(txout, PROTOCOL_VERSION); } - if (IsDust(txout, wallet.chain().relayDustFee())) - { - error = _("Transaction amount too small"); - return std::nullopt; + if (IsDust(txout, wallet.chain().relayDustFee())) { + return _("Transaction amount too small"); } txNew.vout.push_back(txout); } @@ -795,8 +789,7 @@ static std::optional<CreatedTransactionResult> CreateTransactionInternal( // Choose coins to use std::optional<SelectionResult> result = SelectCoins(wallet, res_available_coins.coins, /*nTargetValue=*/selection_target, coin_control, coin_selection_params); if (!result) { - error = _("Insufficient funds"); - return std::nullopt; + return _("Insufficient funds"); } TRACE5(coin_selection, selected_coins, wallet.GetName().c_str(), GetAlgorithmName(result->m_algo).c_str(), result->m_target, result->GetWaste(), result->GetSelectedValue()); @@ -810,10 +803,8 @@ static std::optional<CreatedTransactionResult> CreateTransactionInternal( // Insert change txn at random position: nChangePosInOut = rng_fast.randrange(txNew.vout.size() + 1); } - else if ((unsigned int)nChangePosInOut > txNew.vout.size()) - { - error = _("Transaction change output index out of range"); - return std::nullopt; + else if ((unsigned int)nChangePosInOut > txNew.vout.size()) { + return _("Transaction change output index out of range"); } assert(nChangePosInOut != -1); @@ -840,8 +831,7 @@ static std::optional<CreatedTransactionResult> CreateTransactionInternal( TxSize tx_sizes = CalculateMaximumSignedTxSize(CTransaction(txNew), &wallet, &coin_control); int nBytes = tx_sizes.vsize; if (nBytes == -1) { - error = _("Missing solving data for estimating transaction size"); - return std::nullopt; + return _("Missing solving data for estimating transaction size"); } nFeeRet = coin_selection_params.m_effective_feerate.GetFee(nBytes); @@ -901,11 +891,10 @@ static std::optional<CreatedTransactionResult> CreateTransactionInternal( // Error if this output is reduced to be below dust if (IsDust(txout, wallet.chain().relayDustFee())) { if (txout.nValue < 0) { - error = _("The transaction amount is too small to pay the fee"); + return _("The transaction amount is too small to pay the fee"); } else { - error = _("The transaction amount is too small to send after the fee has been deducted"); + return _("The transaction amount is too small to send after the fee has been deducted"); } - return std::nullopt; } } ++i; @@ -915,42 +904,37 @@ static std::optional<CreatedTransactionResult> CreateTransactionInternal( // Give up if change keypool ran out and change is required if (scriptChange.empty() && nChangePosInOut != -1) { - return std::nullopt; + return error; } if (sign && !wallet.SignTransaction(txNew)) { - error = _("Signing transaction failed"); - return std::nullopt; + return _("Signing transaction failed"); } // Return the constructed transaction data. - tx = MakeTransactionRef(std::move(txNew)); + CTransactionRef tx = MakeTransactionRef(std::move(txNew)); // Limit size if ((sign && GetTransactionWeight(*tx) > MAX_STANDARD_TX_WEIGHT) || (!sign && tx_sizes.weight > MAX_STANDARD_TX_WEIGHT)) { - error = _("Transaction too large"); - return std::nullopt; + return _("Transaction too large"); } if (nFeeRet > wallet.m_default_max_tx_fee) { - error = TransactionErrorString(TransactionError::MAX_FEE_EXCEEDED); - return std::nullopt; + return 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)) { - error = _("Transaction has too long of a mempool chain"); - return std::nullopt; + return _("Transaction has too long of a mempool chain"); } } // Before we return success, we assume any change key will be used to prevent // accidental re-use. reservedest.KeepDestination(); - fee_calc_out = feeCalc; wallet.WalletLogPrintf("Fee Calculation: Fee:%d Bytes:%u Tgt:%d (requested %d) Reason:\"%s\" Decay %.5f: Estimation: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out) Fail: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out)\n", nFeeRet, nBytes, feeCalc.returnedTarget, feeCalc.desiredTarget, StringForFeeReason(feeCalc.reason), feeCalc.est.decay, @@ -960,52 +944,50 @@ static std::optional<CreatedTransactionResult> CreateTransactionInternal( feeCalc.est.fail.start, feeCalc.est.fail.end, (feeCalc.est.fail.totalConfirmed + feeCalc.est.fail.inMempool + feeCalc.est.fail.leftMempool) > 0.0 ? 100 * feeCalc.est.fail.withinTarget / (feeCalc.est.fail.totalConfirmed + feeCalc.est.fail.inMempool + feeCalc.est.fail.leftMempool) : 0.0, feeCalc.est.fail.withinTarget, feeCalc.est.fail.totalConfirmed, feeCalc.est.fail.inMempool, feeCalc.est.fail.leftMempool); - return CreatedTransactionResult(tx, nFeeRet, nChangePosInOut); + return CreatedTransactionResult(tx, nFeeRet, nChangePosInOut, feeCalc); } -std::optional<CreatedTransactionResult> CreateTransaction( +BResult<CreatedTransactionResult> CreateTransaction( CWallet& wallet, const std::vector<CRecipient>& vecSend, int change_pos, - bilingual_str& error, const CCoinControl& coin_control, - FeeCalculation& fee_calc_out, bool sign) { if (vecSend.empty()) { - error = _("Transaction must have at least one recipient"); - return std::nullopt; + return _("Transaction must have at least one recipient"); } if (std::any_of(vecSend.cbegin(), vecSend.cend(), [](const auto& recipient){ return recipient.nAmount < 0; })) { - error = _("Transaction amounts must not be negative"); - return std::nullopt; + return _("Transaction amounts must not be negative"); } LOCK(wallet.cs_wallet); - std::optional<CreatedTransactionResult> txr_ungrouped = CreateTransactionInternal(wallet, vecSend, change_pos, error, coin_control, fee_calc_out, sign); - TRACE4(coin_selection, normal_create_tx_internal, wallet.GetName().c_str(), txr_ungrouped.has_value(), - txr_ungrouped.has_value() ? txr_ungrouped->fee : 0, txr_ungrouped.has_value() ? txr_ungrouped->change_pos : 0); - if (!txr_ungrouped) return std::nullopt; + 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); + if (!res) return res; + const auto& txr_ungrouped = res.GetObj(); // 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) { + 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; - bilingual_str error2; // fired and forgotten; if an error occurs, we discard the results - std::optional<CreatedTransactionResult> txr_grouped = CreateTransactionInternal(wallet, vecSend, change_pos, error2, tmp_cc, fee_calc_out, sign); + 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}; // 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}; + 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(), txr_grouped.has_value() ? txr_grouped->fee : 0, txr_grouped.has_value() ? txr_grouped->change_pos : 0); 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 txr_grouped; + txr_ungrouped.fee, txr_grouped->fee, use_aps ? "grouped" : "non-grouped"); + if (use_aps) return res_tx_grouped; } } - return txr_ungrouped; + return res; } bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl coinControl) @@ -1041,12 +1023,15 @@ bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet, } } - FeeCalculation fee_calc_out; - std::optional<CreatedTransactionResult> txr = CreateTransaction(wallet, vecSend, nChangePosInOut, error, coinControl, fee_calc_out, false); - if (!txr) return false; - CTransactionRef tx_new = txr->tx; - nFeeRet = txr->fee; - nChangePosInOut = txr->change_pos; + auto res = CreateTransaction(wallet, vecSend, nChangePosInOut, coinControl, false); + if (!res) { + error = res.GetError(); + return false; + } + const auto& txr = res.GetObj(); + CTransactionRef tx_new = txr.tx; + nFeeRet = txr.fee; + nChangePosInOut = txr.change_pos; if (nChangePosInOut != -1) { tx.vout.insert(tx.vout.begin() + nChangePosInOut, tx_new->vout[nChangePosInOut]); diff --git a/src/wallet/spend.h b/src/wallet/spend.h index 21f0095e77..ab0ff1ee58 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -6,6 +6,8 @@ #define BITCOIN_WALLET_SPEND_H #include <consensus/amount.h> +#include <policy/fees.h> // for FeeCalculation +#include <util/result.h> #include <wallet/coinselection.h> #include <wallet/transaction.h> #include <wallet/wallet.h> @@ -100,10 +102,11 @@ struct CreatedTransactionResult { CTransactionRef tx; CAmount fee; + FeeCalculation fee_calc; int change_pos; - CreatedTransactionResult(CTransactionRef tx, CAmount fee, int change_pos) - : tx(tx), fee(fee), change_pos(change_pos) {} + CreatedTransactionResult(CTransactionRef _tx, CAmount _fee, int _change_pos, const FeeCalculation& _fee_calc) + : tx(_tx), fee(_fee), fee_calc(_fee_calc), change_pos(_change_pos) {} }; /** @@ -111,7 +114,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 */ -std::optional<CreatedTransactionResult> CreateTransaction(CWallet& wallet, const std::vector<CRecipient>& vecSend, int change_pos, bilingual_str& error, const CCoinControl& coin_control, FeeCalculation& fee_calc_out, bool sign = true); +BResult<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/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 9dd17c8e48..a418105ee1 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -74,11 +74,9 @@ static void add_coin(std::vector<COutput>& coins, CWallet& wallet, const CAmount tx.vout.resize(nInput + 1); tx.vout[nInput].nValue = nValue; if (spendable) { - CTxDestination dest; - bilingual_str error; - const bool destination_ok = wallet.GetNewDestination(OutputType::BECH32, "", dest, error); - assert(destination_ok); - tx.vout[nInput].scriptPubKey = GetScriptForDestination(dest); + auto op_dest = wallet.GetNewDestination(OutputType::BECH32, ""); + assert(op_dest.HasRes()); + tx.vout[nInput].scriptPubKey = GetScriptForDestination(op_dest.GetObj()); } uint256 txid = tx.GetHash(); diff --git a/src/wallet/test/fuzz/notifications.cpp b/src/wallet/test/fuzz/notifications.cpp index 9089c8ff46..816bef6148 100644 --- a/src/wallet/test/fuzz/notifications.cpp +++ b/src/wallet/test/fuzz/notifications.cpp @@ -69,15 +69,14 @@ struct FuzzedWallet { CScript GetScriptPubKey(FuzzedDataProvider& fuzzed_data_provider) { auto type{fuzzed_data_provider.PickValueInArray(OUTPUT_TYPES)}; - CTxDestination dest; - bilingual_str error; + BResult<CTxDestination> op_dest; if (fuzzed_data_provider.ConsumeBool()) { - assert(wallet->GetNewDestination(type, "", dest, error)); + op_dest = wallet->GetNewDestination(type, ""); } else { - assert(wallet->GetNewChangeDestination(type, dest, error)); + op_dest = wallet->GetNewChangeDestination(type); } - assert(error.empty()); - return GetScriptForDestination(dest); + assert(op_dest.HasRes()); + return GetScriptForDestination(op_dest.GetObj()); } }; diff --git a/src/wallet/test/spend_tests.cpp b/src/wallet/test/spend_tests.cpp index bdc148afb4..53c3b5d2ae 100644 --- a/src/wallet/test/spend_tests.cpp +++ b/src/wallet/test/spend_tests.cpp @@ -28,19 +28,18 @@ BOOST_FIXTURE_TEST_CASE(SubtractFee, TestChain100Setup) auto check_tx = [&wallet](CAmount leftover_input_amount) { CRecipient recipient{GetScriptForRawPubKey({}), 50 * COIN - leftover_input_amount, true /* subtract fee */}; constexpr int RANDOM_CHANGE_POSITION = -1; - bilingual_str error; CCoinControl coin_control; coin_control.m_feerate.emplace(10000); coin_control.fOverrideFeeRate = true; // We need to use a change type with high cost of change so that the leftover amount will be dropped to fee instead of added as a change output coin_control.m_change_type = OutputType::LEGACY; - FeeCalculation fee_calc; - std::optional<CreatedTransactionResult> txr = CreateTransaction(*wallet, {recipient}, RANDOM_CHANGE_POSITION, error, coin_control, fee_calc); - BOOST_CHECK(txr.has_value()); - 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); - return txr->fee; + auto res = CreateTransaction(*wallet, {recipient}, RANDOM_CHANGE_POSITION, coin_control); + BOOST_CHECK(res); + const auto& txr = res.GetObj(); + 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); + return txr.fee; }; // Send full input amount to recipient, check that only nonzero fee is diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 4093392ff9..81c01e5023 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -4,7 +4,6 @@ #include <wallet/wallet.h> -#include <any> #include <future> #include <memory> #include <stdint.h> @@ -13,7 +12,6 @@ #include <interfaces/chain.h> #include <key_io.h> #include <node/blockstorage.h> -#include <node/context.h> #include <policy/policy.h> #include <rpc/server.h> #include <test/util/logging.h> @@ -536,14 +534,12 @@ public: CWalletTx& AddTx(CRecipient recipient) { CTransactionRef tx; - bilingual_str error; CCoinControl dummy; - FeeCalculation fee_calc_out; { constexpr int RANDOM_CHANGE_POSITION = -1; - std::optional<CreatedTransactionResult> txr = CreateTransaction(*wallet, {recipient}, RANDOM_CHANGE_POSITION, error, dummy, fee_calc_out); - BOOST_CHECK(txr.has_value()); - tx = txr->tx; + auto res = CreateTransaction(*wallet, {recipient}, RANDOM_CHANGE_POSITION, dummy); + BOOST_CHECK(res); + tx = res.GetObj().tx; } wallet->CommitTransaction(tx, {}, {}); CMutableTransaction blocktx; @@ -629,9 +625,7 @@ BOOST_FIXTURE_TEST_CASE(wallet_disableprivkeys, TestChain100Setup) wallet->SetMinVersion(FEATURE_LATEST); wallet->SetWalletFlag(WALLET_FLAG_DISABLE_PRIVATE_KEYS); BOOST_CHECK(!wallet->TopUpKeyPool(1000)); - CTxDestination dest; - bilingual_str error; - BOOST_CHECK(!wallet->GetNewDestination(OutputType::BECH32, "", dest, error)); + BOOST_CHECK(!wallet->GetNewDestination(OutputType::BECH32, "")); } { const std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>(m_node.chain.get(), "", m_args, CreateDummyWalletDatabase()); @@ -639,9 +633,7 @@ BOOST_FIXTURE_TEST_CASE(wallet_disableprivkeys, TestChain100Setup) wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); wallet->SetMinVersion(FEATURE_LATEST); wallet->SetWalletFlag(WALLET_FLAG_DISABLE_PRIVATE_KEYS); - CTxDestination dest; - bilingual_str error; - BOOST_CHECK(!wallet->GetNewDestination(OutputType::BECH32, "", dest, error)); + BOOST_CHECK(!wallet->GetNewDestination(OutputType::BECH32, "")); } } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 79ec29f19a..de4a8a6eb7 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2318,37 +2318,36 @@ bool CWallet::TopUpKeyPool(unsigned int kpSize) return res; } -bool CWallet::GetNewDestination(const OutputType type, const std::string label, CTxDestination& dest, bilingual_str& error) +BResult<CTxDestination> CWallet::GetNewDestination(const OutputType type, const std::string label) { LOCK(cs_wallet); - error.clear(); - bool result = false; auto spk_man = GetScriptPubKeyMan(type, false /* internal */); - if (spk_man) { - spk_man->TopUp(); - result = spk_man->GetNewDestination(type, dest, error); - } else { - error = strprintf(_("Error: No %s addresses available."), FormatOutputType(type)); + if (!spk_man) { + return strprintf(_("Error: No %s addresses available."), FormatOutputType(type)); } - if (result) { - SetAddressBook(dest, label, "receive"); + + spk_man->TopUp(); + auto op_dest = spk_man->GetNewDestination(type); + if (op_dest) { + SetAddressBook(op_dest.GetObj(), label, "receive"); } - return result; + return op_dest; } -bool CWallet::GetNewChangeDestination(const OutputType type, CTxDestination& dest, bilingual_str& error) +BResult<CTxDestination> CWallet::GetNewChangeDestination(const OutputType type) { LOCK(cs_wallet); - error.clear(); + CTxDestination dest; + bilingual_str error; ReserveDestination reservedest(this, type); if (!reservedest.GetReservedDestination(dest, true, error)) { - return false; + return error; } reservedest.KeepDestination(); - return true; + return dest; } std::optional<int64_t> CWallet::GetOldestKeyPoolTime() const diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 865ce92311..e2c3d76438 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -15,6 +15,7 @@ #include <psbt.h> #include <tinyformat.h> #include <util/message.h> +#include <util/result.h> #include <util/strencodings.h> #include <util/string.h> #include <util/system.h> @@ -45,7 +46,6 @@ using LoadWalletFn = std::function<void(std::unique_ptr<interfaces::Wallet> wall class CScript; enum class FeeEstimateMode; -struct FeeCalculation; struct bilingual_str; namespace wallet { @@ -666,8 +666,8 @@ public: */ void MarkDestinationsDirty(const std::set<CTxDestination>& destinations) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - bool GetNewDestination(const OutputType type, const std::string label, CTxDestination& dest, bilingual_str& error); - bool GetNewChangeDestination(const OutputType type, CTxDestination& dest, bilingual_str& error); + BResult<CTxDestination> GetNewDestination(const OutputType type, const std::string label); + BResult<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); |