From 0295b44c257fe23f1ad344aff11372d67864f0db Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Wed, 1 Jun 2022 17:04:03 -0400 Subject: [PATCH] wallet: return CreatedTransactionResult from FundTransaction Instead of using the output parameters, return CreatedTransactionResult from FundTransaction in the same way that CreateTransaction does. Additionally, instead of modifying the original CMutableTransaction, the result from CreateTransactionInternal is used. --- src/wallet/rpc/spend.cpp | 47 ++++++++++++-------------- src/wallet/spend.cpp | 36 ++++---------------- src/wallet/spend.h | 2 +- src/wallet/test/fuzz/notifications.cpp | 3 +- 4 files changed, 30 insertions(+), 58 deletions(-) diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index 7cde644d9b..b121c8e1a7 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -488,13 +488,13 @@ static std::vector FundTxDoc(bool solving_data = true) return args; } -void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out, int& change_position, const UniValue& options, CCoinControl& coinControl, bool override_min_fee) +CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransaction& tx, const UniValue& options, CCoinControl& coinControl, bool override_min_fee) { // Make sure the results are valid at least up to the most recent block // the user could have gotten from another RPC command prior to now wallet.BlockUntilSyncedToCurrentChain(); - change_position = -1; + std::optional change_position; bool lockUnspents = false; UniValue subtractFeeFromOutputs; std::set setSubtractFeeFromOutputs; @@ -552,7 +552,11 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out, } if (options.exists("changePosition") || options.exists("change_position")) { - change_position = (options.exists("change_position") ? options["change_position"] : options["changePosition"]).getInt(); + int pos = (options.exists("change_position") ? options["change_position"] : options["changePosition"]).getInt(); + if (pos < 0 || (unsigned int)pos > tx.vout.size()) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "changePosition out of bounds"); + } + change_position = (unsigned int)pos; } if (options.exists("change_type")) { @@ -702,9 +706,6 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out, if (tx.vout.size() == 0) throw JSONRPCError(RPC_INVALID_PARAMETER, "TX must have at least one output"); - if (change_position != -1 && (change_position < 0 || (unsigned int)change_position > tx.vout.size())) - throw JSONRPCError(RPC_INVALID_PARAMETER, "changePosition out of bounds"); - for (unsigned int idx = 0; idx < subtractFeeFromOutputs.size(); idx++) { int pos = subtractFeeFromOutputs[idx].getInt(); if (setSubtractFeeFromOutputs.count(pos)) @@ -716,11 +717,11 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out, setSubtractFeeFromOutputs.insert(pos); } - bilingual_str error; - - if (!FundTransaction(wallet, tx, fee_out, change_position, error, lockUnspents, setSubtractFeeFromOutputs, coinControl)) { - throw JSONRPCError(RPC_WALLET_ERROR, error.original); + auto txr = FundTransaction(wallet, tx, change_position, lockUnspents, setSubtractFeeFromOutputs, coinControl); + if (!txr) { + throw JSONRPCError(RPC_WALLET_ERROR, ErrorString(txr).original); } + return *txr; } static void SetOptionsInputWeights(const UniValue& inputs, UniValue& options) @@ -843,17 +844,15 @@ RPCHelpMan fundrawtransaction() throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed"); } - CAmount fee; - int change_position; CCoinControl coin_control; // Automatically select (additional) coins. Can be overridden by options.add_inputs. coin_control.m_allow_other_inputs = true; - FundTransaction(*pwallet, tx, fee, change_position, request.params[1], coin_control, /*override_min_fee=*/true); + auto txr = FundTransaction(*pwallet, tx, request.params[1], coin_control, /*override_min_fee=*/true); UniValue result(UniValue::VOBJ); - result.pushKV("hex", EncodeHexTx(CTransaction(tx))); - result.pushKV("fee", ValueFromAmount(fee)); - result.pushKV("changepos", change_position); + result.pushKV("hex", EncodeHexTx(*txr.tx)); + result.pushKV("fee", ValueFromAmount(txr.fee)); + result.pushKV("changepos", txr.change_pos ? (int)*txr.change_pos : -1); return result; }, @@ -1275,8 +1274,6 @@ RPCHelpMan send() PreventOutdatedOptions(options); - CAmount fee; - int change_position; bool rbf{options.exists("replaceable") ? options["replaceable"].get_bool() : pwallet->m_signal_rbf}; CMutableTransaction rawTx = ConstructTransaction(options["inputs"], request.params[0], options["locktime"], rbf); CCoinControl coin_control; @@ -1284,9 +1281,9 @@ RPCHelpMan send() // be overridden by options.add_inputs. coin_control.m_allow_other_inputs = rawTx.vin.size() == 0; SetOptionsInputWeights(options["inputs"], options); - FundTransaction(*pwallet, rawTx, fee, change_position, options, coin_control, /*override_min_fee=*/false); + auto txr = FundTransaction(*pwallet, rawTx, options, coin_control, /*override_min_fee=*/false); - return FinishTransaction(pwallet, options, rawTx); + return FinishTransaction(pwallet, options, CMutableTransaction(*txr.tx)); } }; } @@ -1711,8 +1708,6 @@ RPCHelpMan walletcreatefundedpsbt() UniValue options{request.params[3].isNull() ? UniValue::VOBJ : request.params[3]}; - CAmount fee; - int change_position; const UniValue &replaceable_arg = options["replaceable"]; const bool rbf{replaceable_arg.isNull() ? wallet.m_signal_rbf : replaceable_arg.get_bool()}; CMutableTransaction rawTx = ConstructTransaction(request.params[0], request.params[1], request.params[2], rbf); @@ -1721,10 +1716,10 @@ RPCHelpMan walletcreatefundedpsbt() // be overridden by options.add_inputs. coin_control.m_allow_other_inputs = rawTx.vin.size() == 0; SetOptionsInputWeights(request.params[0], options); - FundTransaction(wallet, rawTx, fee, change_position, options, coin_control, /*override_min_fee=*/true); + auto txr = FundTransaction(wallet, rawTx, options, coin_control, /*override_min_fee=*/true); // Make a blank psbt - PartiallySignedTransaction psbtx(rawTx); + PartiallySignedTransaction psbtx(CMutableTransaction(*txr.tx)); // Fill transaction with out data but don't sign bool bip32derivs = request.params[4].isNull() ? true : request.params[4].get_bool(); @@ -1740,8 +1735,8 @@ RPCHelpMan walletcreatefundedpsbt() UniValue result(UniValue::VOBJ); result.pushKV("psbt", EncodeBase64(ssTx.str())); - result.pushKV("fee", ValueFromAmount(fee)); - result.pushKV("changepos", change_position); + result.pushKV("fee", ValueFromAmount(txr.fee)); + result.pushKV("changepos", txr.change_pos ? (int)*txr.change_pos : -1); return result; }, }; diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index f911e52ee1..5b28d38c37 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -1360,7 +1360,7 @@ util::Result CreateTransaction( return res; } -bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, bool lockUnspents, const std::set& setSubtractFeeFromOutputs, CCoinControl coinControl) +util::Result FundTransaction(CWallet& wallet, const CMutableTransaction& tx, std::optional change_pos, bool lockUnspents, const std::set& setSubtractFeeFromOutputs, CCoinControl coinControl) { std::vector vecSend; @@ -1396,8 +1396,7 @@ bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet, PreselectedInput& preset_txin = coinControl.Select(outPoint); if (!wallet.IsMine(outPoint)) { if (coins[outPoint].out.IsNull()) { - error = _("Unable to find UTXO for external input"); - return false; + return util::Error{_("Unable to find UTXO for external input")}; } // The input was not in the wallet, but is in the UTXO set, so select as external @@ -1408,38 +1407,17 @@ bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet, preset_txin.SetScriptWitness(txin.scriptWitness); } - auto res = CreateTransaction(wallet, vecSend, nChangePosInOut == -1 ? std::nullopt : std::optional((unsigned int)nChangePosInOut), coinControl, false); + auto res = CreateTransaction(wallet, vecSend, change_pos, coinControl, false); if (!res) { - error = util::ErrorString(res); - return false; - } - const auto& txr = *res; - CTransactionRef tx_new = txr.tx; - nFeeRet = txr.fee; - nChangePosInOut = txr.change_pos ? *txr.change_pos : -1; - - if (nChangePosInOut != -1) { - tx.vout.insert(tx.vout.begin() + nChangePosInOut, tx_new->vout[nChangePosInOut]); + return res; } - // Copy output sizes from new transaction; they may have had the fee - // subtracted from them. - for (unsigned int idx = 0; idx < tx.vout.size(); idx++) { - tx.vout[idx].nValue = tx_new->vout[idx].nValue; - } - - // Add new txins while keeping original txin scriptSig/order. - for (const CTxIn& txin : tx_new->vin) { - if (!coinControl.IsSelected(txin.prevout)) { - tx.vin.push_back(txin); - - } - if (lockUnspents) { + if (lockUnspents) { + for (const CTxIn& txin : res->tx->vin) { wallet.LockCoin(txin.prevout); } - } - return true; + return res; } } // namespace wallet diff --git a/src/wallet/spend.h b/src/wallet/spend.h index ccadbed1f5..504c078b80 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -224,7 +224,7 @@ util::Result CreateTransaction(CWallet& wallet, const * Insert additional inputs into the transaction by * calling CreateTransaction(); */ -bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, bool lockUnspents, const std::set& setSubtractFeeFromOutputs, CCoinControl); +util::Result FundTransaction(CWallet& wallet, const CMutableTransaction& tx, std::optional change_pos, bool lockUnspents, const std::set& setSubtractFeeFromOutputs, CCoinControl); } // namespace wallet #endif // BITCOIN_WALLET_SPEND_H diff --git a/src/wallet/test/fuzz/notifications.cpp b/src/wallet/test/fuzz/notifications.cpp index 82cdd32302..203ab5f606 100644 --- a/src/wallet/test/fuzz/notifications.cpp +++ b/src/wallet/test/fuzz/notifications.cpp @@ -156,10 +156,9 @@ struct FuzzedWallet { coin_control.fOverrideFeeRate = fuzzed_data_provider.ConsumeBool(); // Add solving data (m_external_provider and SelectExternal)? - CAmount fee_out; int change_position{fuzzed_data_provider.ConsumeIntegralInRange(-1, tx.vout.size() - 1)}; bilingual_str error; - (void)FundTransaction(*wallet, tx, fee_out, change_position, error, /*lockUnspents=*/false, subtract_fee_from_outputs, coin_control); + (void)FundTransaction(*wallet, tx, change_position, /*lockUnspents=*/false, subtract_fee_from_outputs, coin_control); } };