From 3dfcb817d8f939df1f46d56e242b18bd604dfb93 Mon Sep 17 00:00:00 2001 From: josibake Date: Mon, 2 Oct 2023 16:57:44 +0200 Subject: [PATCH] refactor: pass CRecipient to FundTransaction Instead of parsing tx.vout, make `FundTransaction` take a `CRecipient` vector directly. This allows us to remove SFFO logic from the wrapper RPC `FundTransaction` since the `CRecipient` objects have already been created with the correct SFFO values. Copy the data field check from `AddOutputs` (used in `ConstructTransaction`) over to `ParseRecipients`. This lets us use `ParseOutputs` anytime we are parsing user provided outputs before passing them to `FundTransaction`. This sets us up in a future PR to be able to use these RPCs with BIP352 static payment codes. --- src/wallet/rpc/spend.cpp | 76 +++++++++++++++----------- src/wallet/spend.cpp | 13 +---- src/wallet/spend.h | 2 +- src/wallet/test/fuzz/notifications.cpp | 10 +++- 4 files changed, 56 insertions(+), 45 deletions(-) diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index 675145494ec643..43d096d8dbe9e3 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -29,16 +29,27 @@ std::vector ParseRecipients(const UniValue& address_amounts, const s std::set destinations; std::vector recipients; int idx{0}; + bool has_data{false}; for (const std::string& address: address_amounts.getKeys()) { - CTxDestination dest = DecodeDestination(address); - if (!IsValidDestination(dest)) { - throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, std::string("Invalid Bitcoin address: ") + address); - } - if (destinations.count(dest)) { - throw JSONRPCError(RPC_INVALID_PARAMETER, std::string("Invalid parameter, duplicated address: ") + address); + CTxDestination dest; + CAmount amount{0}; + if (address == "data") { + if (has_data) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, duplicate key: data"); + } + std::vector data = ParseHexV(address_amounts[address].getValStr(), "Data"); + dest = CNoDestination{CScript() << OP_RETURN << data}; + } else { + dest = DecodeDestination(address); + if (!IsValidDestination(dest)) { + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, std::string("Invalid Bitcoin address: ") + address); + } + if (destinations.count(dest)) { + throw JSONRPCError(RPC_INVALID_PARAMETER, std::string("Invalid parameter, duplicated address: ") + address); + } + destinations.insert(dest); + amount = AmountFromValue(address_amounts[address]); } - destinations.insert(dest); - CAmount amount = AmountFromValue(address_amounts[address]); CRecipient recipient = {dest, amount, subtract_fee_outputs.count(idx) == 1}; recipients.push_back(recipient); idx++; @@ -532,7 +543,7 @@ static std::vector FundTxDoc(bool solving_data = true) return args; } -CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransaction& tx, const UniValue& options, CCoinControl& coinControl, bool override_min_fee) +CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransaction& tx, const std::vector& recipients, 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 @@ -540,9 +551,6 @@ CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransact std::optional change_position; bool lockUnspents = false; - UniValue subtractFeeFromOutputs; - std::set setSubtractFeeFromOutputs; - if (!options.isNull()) { if (options.type() == UniValue::VBOOL) { // backward compatibility bool only fallback @@ -639,9 +647,6 @@ CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransact coinControl.fOverrideFeeRate = true; } - if (options.exists("subtractFeeFromOutputs") || options.exists("subtract_fee_from_outputs") ) - subtractFeeFromOutputs = (options.exists("subtract_fee_from_outputs") ? options["subtract_fee_from_outputs"] : options["subtractFeeFromOutputs"]).get_array(); - if (options.exists("replaceable")) { coinControl.m_signal_bip125_rbf = options["replaceable"].get_bool(); } @@ -750,18 +755,7 @@ CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransact if (tx.vout.size() == 0) throw JSONRPCError(RPC_INVALID_PARAMETER, "TX must have at least one output"); - for (unsigned int idx = 0; idx < subtractFeeFromOutputs.size(); idx++) { - int pos = subtractFeeFromOutputs[idx].getInt(); - if (setSubtractFeeFromOutputs.count(pos)) - throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid parameter, duplicated position: %d", pos)); - if (pos < 0) - throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid parameter, negative position: %d", pos)); - if (pos >= int(tx.vout.size())) - throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid parameter, position too large: %d", pos)); - setSubtractFeeFromOutputs.insert(pos); - } - - auto txr = FundTransaction(wallet, tx, change_position, lockUnspents, setSubtractFeeFromOutputs, coinControl); + auto txr = FundTransaction(wallet, tx, recipients, change_position, lockUnspents, coinControl); if (!txr) { throw JSONRPCError(RPC_WALLET_ERROR, ErrorString(txr).original); } @@ -887,11 +881,29 @@ RPCHelpMan fundrawtransaction() if (!DecodeHexTx(tx, request.params[0].get_str(), try_no_witness, try_witness)) { throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed"); } - + UniValue options = request.params[1]; + std::set setSubtractFeeFromOutputs; + if (options.exists("subtractFeeFromOutputs") || options.exists("subtract_fee_from_outputs") ) { + UniValue subtractFeeFromOutputs = (options.exists("subtract_fee_from_outputs") ? options["subtract_fee_from_outputs"] : options["subtractFeeFromOutputs"]).get_array(); + // We need to know the number of destinations for the transaction in order to create the CRecipient vector with + // the correct SFFO information. To do this, pass a dummy vector with the correct number of destinations + // + // This works because fundrawtransaction can only take integers for specifying SFFO + std::vector destinations(tx.vout.size(), "dummy"); + setSubtractFeeFromOutputs = InterpretSubtractFeeFromOutputInstructions(subtractFeeFromOutputs, destinations); + } + std::vector recipients; + for (size_t idx = 0; idx < tx.vout.size(); idx++) { + const CTxOut& txOut = tx.vout[idx]; + CTxDestination dest; + ExtractDestination(txOut.scriptPubKey, dest); + CRecipient recipient = {dest, txOut.nValue, setSubtractFeeFromOutputs.count(idx) == 1}; + recipients.push_back(recipient); + } CCoinControl coin_control; // Automatically select (additional) coins. Can be overridden by options.add_inputs. coin_control.m_allow_other_inputs = true; - auto txr = FundTransaction(*pwallet, tx, request.params[1], coin_control, /*override_min_fee=*/true); + auto txr = FundTransaction(*pwallet, tx, recipients, options, coin_control, /*override_min_fee=*/true); UniValue result(UniValue::VOBJ); result.pushKV("hex", EncodeHexTx(*txr.tx)); @@ -1319,13 +1331,14 @@ RPCHelpMan send() bool rbf{options.exists("replaceable") ? options["replaceable"].get_bool() : pwallet->m_signal_rbf}; + std::vector recipients = ParseOutputs(request.params[0], options); CMutableTransaction rawTx = ConstructTransaction(options["inputs"], request.params[0], options["locktime"], rbf); CCoinControl coin_control; // Automatically select coins, unless at least one is manually selected. Can // be overridden by options.add_inputs. coin_control.m_allow_other_inputs = rawTx.vin.size() == 0; SetOptionsInputWeights(options["inputs"], options); - auto txr = FundTransaction(*pwallet, rawTx, options, coin_control, /*override_min_fee=*/false); + auto txr = FundTransaction(*pwallet, rawTx, recipients, options, coin_control, /*override_min_fee=*/false); return FinishTransaction(pwallet, options, CMutableTransaction(*txr.tx)); } @@ -1755,12 +1768,13 @@ RPCHelpMan walletcreatefundedpsbt() 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); + std::vector recipients = ParseOutputs(request.params[1], options); CCoinControl coin_control; // Automatically select coins, unless at least one is manually selected. Can // be overridden by options.add_inputs. coin_control.m_allow_other_inputs = rawTx.vin.size() == 0; SetOptionsInputWeights(request.params[0], options); - auto txr = FundTransaction(wallet, rawTx, options, coin_control, /*override_min_fee=*/true); + auto txr = FundTransaction(wallet, rawTx, recipients, options, coin_control, /*override_min_fee=*/true); // Make a blank psbt PartiallySignedTransaction psbtx(CMutableTransaction(*txr.tx)); diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 5b28d38c37d4e1..50fca31a03784e 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -1360,19 +1360,8 @@ util::Result CreateTransaction( return res; } -util::Result FundTransaction(CWallet& wallet, const CMutableTransaction& tx, std::optional change_pos, bool lockUnspents, const std::set& setSubtractFeeFromOutputs, CCoinControl coinControl) +util::Result FundTransaction(CWallet& wallet, const CMutableTransaction& tx, const std::vector& vecSend, std::optional change_pos, bool lockUnspents, CCoinControl coinControl) { - std::vector vecSend; - - // Turn the txout set into a CRecipient vector. - for (size_t idx = 0; idx < tx.vout.size(); idx++) { - const CTxOut& txOut = tx.vout[idx]; - CTxDestination dest; - ExtractDestination(txOut.scriptPubKey, dest); - CRecipient recipient = {dest, txOut.nValue, setSubtractFeeFromOutputs.count(idx) == 1}; - vecSend.push_back(recipient); - } - // Set the user desired locktime coinControl.m_locktime = tx.nLockTime; diff --git a/src/wallet/spend.h b/src/wallet/spend.h index 504c078b803f8f..5d8f0fe2a5296d 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(); */ -util::Result FundTransaction(CWallet& wallet, const CMutableTransaction& tx, std::optional change_pos, bool lockUnspents, const std::set& setSubtractFeeFromOutputs, CCoinControl); +util::Result FundTransaction(CWallet& wallet, const CMutableTransaction& tx, const std::vector& recipients, std::optional change_pos, bool lockUnspents, 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 203ab5f606a796..9b1e34f611194d 100644 --- a/src/wallet/test/fuzz/notifications.cpp +++ b/src/wallet/test/fuzz/notifications.cpp @@ -132,6 +132,14 @@ struct FuzzedWallet { } } } + std::vector recipients; + for (size_t idx = 0; idx < tx.vout.size(); idx++) { + const CTxOut& tx_out = tx.vout[idx]; + CTxDestination dest; + ExtractDestination(tx_out.scriptPubKey, dest); + CRecipient recipient = {dest, tx_out.nValue, subtract_fee_from_outputs.count(idx) == 1}; + recipients.push_back(recipient); + } CCoinControl coin_control; coin_control.m_allow_other_inputs = fuzzed_data_provider.ConsumeBool(); CallOneOf( @@ -158,7 +166,7 @@ struct FuzzedWallet { int change_position{fuzzed_data_provider.ConsumeIntegralInRange(-1, tx.vout.size() - 1)}; bilingual_str error; - (void)FundTransaction(*wallet, tx, change_position, /*lockUnspents=*/false, subtract_fee_from_outputs, coin_control); + (void)FundTransaction(*wallet, tx, recipients, change_position, /*lockUnspents=*/false, coin_control); } };