foster/bitcoin-bitcoin-core
0
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-02-03 09:56:38 -05:00
Code Issues Activity

send: refactor CreateTransaction flow to return a BResult<CTransactionRef>

Browse source
This commit is contained in:
furszy 2022-04-08 16:43:10 -03:00
parent 198fcca162
commit 22351725bc
No known key found for this signature in database
GPG key ID: 5DD23CCC686AA623
9 changed files with 81 additions and 93 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

6
src/interfaces/wallet.h
View file

@ -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>
@ -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,

6
src/qt/walletmodel.cpp
View file

@ -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;
}

12
src/wallet/feebumper.cpp
View file

@ -219,18 +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;
std::optional<CreatedTransactionResult> txr = CreateTransaction(wallet, recipients, RANDOM_CHANGE_POSITION, fail_reason, new_coin_control, 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;
}

18
src/wallet/interfaces.cpp
View file

@ -250,21 +250,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);
std::optional<CreatedTransactionResult> txr = CreateTransaction(*m_wallet, recipients, change_pos,
fail_reason, coin_control, 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,

11
src/wallet/rpc/spend.cpp
View file

@ -156,17 +156,16 @@ UniValue SendMoney(CWallet& wallet, const CCoinControl &coin_control, std::vecto
// Send
constexpr int RANDOM_CHANGE_POSITION = -1;
bilingual_str error;
std::optional<CreatedTransactionResult> txr = CreateTransaction(wallet, recipients, RANDOM_CHANGE_POSITION, error, coin_control, 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(txr->fee_calc.reason));
entry.pushKV("fee_reason", StringForFeeReason(res.GetObj().fee_calc.reason));
return entry;
}
return tx->GetHash().GetHex();

97
src/wallet/spend.cpp
View file

@ -656,18 +656,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,
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;
@ -696,6 +694,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)) {
@ -743,13 +742,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
@ -774,10 +771,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);
}
@ -798,8 +793,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());
@ -813,10 +807,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);
@ -843,8 +835,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);
@ -904,11 +895,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;
@ -918,35 +908,31 @@ 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");
}
}
@ -965,48 +951,47 @@ static std::optional<CreatedTransactionResult> CreateTransactionInternal(
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,
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, 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, 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)
@ -1042,11 +1027,15 @@ bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet,
}
}
std::optional<CreatedTransactionResult> txr = CreateTransaction(wallet, vecSend, nChangePosInOut, error, coinControl, 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]);

3
src/wallet/spend.h
View file

@ -7,6 +7,7 @@
#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>
@ -120,7 +121,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, 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

14
src/wallet/test/spend_tests.cpp
View file

@ -28,18 +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;
std::optional<CreatedTransactionResult> txr = CreateTransaction(*wallet, {recipient}, RANDOM_CHANGE_POSITION, error, coin_control);
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

7
src/wallet/test/wallet_tests.cpp
View file

@ -519,13 +519,12 @@ public:
CWalletTx& AddTx(CRecipient recipient)
{
CTransactionRef tx;
bilingual_str error;
CCoinControl dummy;
{
constexpr int RANDOM_CHANGE_POSITION = -1;
std::optional<CreatedTransactionResult> txr = CreateTransaction(*wallet, {recipient}, RANDOM_CHANGE_POSITION, error, dummy);
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;