From 435fe5cd96599c518e26efe444c9d94d1277996b Mon Sep 17 00:00:00 2001 From: josibake Date: Fri, 22 Dec 2023 17:47:03 +0100 Subject: [PATCH 1/6] test: add tests for fundrawtx and sendmany rpcs If the serialized transaction passed to `fundrawtransaction` contains duplicates, they will be deserialized and added to the transaction. Add a test to ensure this behavior is not changed during the refactor. A user can pass any number of duplicated and unrelated addresses as an SFFO argument to `sendmany` and the RPC will not throw an error (note, all the rest of the RPCs which take SFFO as an argument will error if the user passes duplicates or specifies outputs not present in the transaction). Add a test to ensure this behavior is not changed during the refactor. --- test/functional/test_runner.py | 2 + test/functional/wallet_fundrawtransaction.py | 31 ++++++++++++++ test/functional/wallet_sendmany.py | 43 ++++++++++++++++++++ 3 files changed, 76 insertions(+) create mode 100755 test/functional/wallet_sendmany.py diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 9c8f0eca262..fcec2ecdbef 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -333,6 +333,8 @@ BASE_SCRIPTS = [ 'wallet_send.py --descriptors', 'wallet_sendall.py --legacy-wallet', 'wallet_sendall.py --descriptors', + 'wallet_sendmany.py --descriptors', + 'wallet_sendmany.py --legacy-wallet', 'wallet_create_tx.py --descriptors', 'wallet_inactive_hdchains.py --legacy-wallet', 'wallet_spend_unconfirmed.py', diff --git a/test/functional/wallet_fundrawtransaction.py b/test/functional/wallet_fundrawtransaction.py index a331ba997e9..d886a59ac15 100755 --- a/test/functional/wallet_fundrawtransaction.py +++ b/test/functional/wallet_fundrawtransaction.py @@ -8,10 +8,13 @@ from decimal import Decimal from itertools import product from math import ceil +from test_framework.address import address_to_scriptpubkey from test_framework.descriptors import descsum_create from test_framework.messages import ( COIN, + CTransaction, + CTxOut, ) from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( @@ -147,6 +150,34 @@ class RawTransactionsTest(BitcoinTestFramework): self.test_22670() self.test_feerate_rounding() self.test_input_confs_control() + self.test_duplicate_outputs() + + def test_duplicate_outputs(self): + self.log.info("Test deserializing and funding a transaction with duplicate outputs") + self.nodes[1].createwallet("fundtx_duplicate_outputs") + w = self.nodes[1].get_wallet_rpc("fundtx_duplicate_outputs") + + addr = w.getnewaddress(address_type="bech32") + self.nodes[0].sendtoaddress(addr, 5) + self.generate(self.nodes[0], 1) + + address = self.nodes[0].getnewaddress("bech32") + tx = CTransaction() + tx.vin = [] + tx.vout = [CTxOut(1 * COIN, bytearray(address_to_scriptpubkey(address)))] * 2 + tx.nLockTime = 0 + tx_hex = tx.serialize().hex() + res = w.fundrawtransaction(tx_hex, add_inputs=True) + signed_res = w.signrawtransactionwithwallet(res["hex"]) + txid = w.sendrawtransaction(signed_res["hex"]) + assert self.nodes[1].getrawtransaction(txid) + + self.log.info("Test SFFO with duplicate outputs") + + res_sffo = w.fundrawtransaction(tx_hex, add_inputs=True, subtractFeeFromOutputs=[0,1]) + signed_res_sffo = w.signrawtransactionwithwallet(res_sffo["hex"]) + txid_sffo = w.sendrawtransaction(signed_res_sffo["hex"]) + assert self.nodes[1].getrawtransaction(txid_sffo) def test_change_position(self): """Ensure setting changePosition in fundraw with an exact match is handled properly.""" diff --git a/test/functional/wallet_sendmany.py b/test/functional/wallet_sendmany.py new file mode 100755 index 00000000000..57519931433 --- /dev/null +++ b/test/functional/wallet_sendmany.py @@ -0,0 +1,43 @@ +#!/usr/bin/env python3 +# 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. +"""Test the sendmany RPC command.""" + +from test_framework.test_framework import BitcoinTestFramework + +class SendmanyTest(BitcoinTestFramework): + # Setup and helpers + def add_options(self, parser): + self.add_wallet_options(parser) + + + def skip_test_if_missing_module(self): + self.skip_if_no_wallet() + + + def set_test_params(self): + self.num_nodes = 1 + self.setup_clean_chain = True + + def test_sffo_repeated_address(self): + addr_1 = self.wallet.getnewaddress() + addr_2 = self.wallet.getnewaddress() + addr_3 = self.wallet.getnewaddress() + + self.log.info("Test using duplicate address in SFFO argument") + self.def_wallet.sendmany(dummy='', amounts={addr_1: 1, addr_2: 1}, subtractfeefrom=[addr_1, addr_1, addr_1]) + self.log.info("Test using address not present in tx.vout in SFFO argument") + self.def_wallet.sendmany(dummy='', amounts={addr_1: 1, addr_2: 1}, subtractfeefrom=[addr_3]) + + def run_test(self): + self.nodes[0].createwallet("activewallet") + self.wallet = self.nodes[0].get_wallet_rpc("activewallet") + self.def_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name) + self.generate(self.nodes[0], 101) + + self.test_sffo_repeated_address() + + +if __name__ == '__main__': + SendmanyTest().main() From 6f569ac903e5ddaac275996a5d0c31b2220b7b81 Mon Sep 17 00:00:00 2001 From: josibake Date: Fri, 22 Dec 2023 11:10:39 +0100 Subject: [PATCH 2/6] refactor: move normalization to new function Move the univalue formatting logic out of AddOutputs and into its own function, `NormalizeOutputs`. This allows us to re-use this logic in later commits. --- src/rpc/rawtransaction_util.cpp | 9 ++++++++- src/rpc/rawtransaction_util.h | 5 ++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/rpc/rawtransaction_util.cpp b/src/rpc/rawtransaction_util.cpp index c471986a448..eb8067cc235 100644 --- a/src/rpc/rawtransaction_util.cpp +++ b/src/rpc/rawtransaction_util.cpp @@ -70,7 +70,7 @@ void AddInputs(CMutableTransaction& rawTx, const UniValue& inputs_in, std::optio } } -void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in) +UniValue NormalizeOutputs(const UniValue& outputs_in) { if (outputs_in.isNull()) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, output argument must be non-null"); @@ -94,6 +94,13 @@ void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in) } outputs = std::move(outputs_dict); } + return outputs; +} + +void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in) +{ + UniValue outputs(UniValue::VOBJ); + outputs = NormalizeOutputs(outputs_in); // Duplicate checking std::set destinations; diff --git a/src/rpc/rawtransaction_util.h b/src/rpc/rawtransaction_util.h index a863432b7a8..f3d5c3616ef 100644 --- a/src/rpc/rawtransaction_util.h +++ b/src/rpc/rawtransaction_util.h @@ -42,7 +42,10 @@ void ParsePrevouts(const UniValue& prevTxsUnival, FillableSigningProvider* keyst /** Normalize univalue-represented inputs and add them to the transaction */ void AddInputs(CMutableTransaction& rawTx, const UniValue& inputs_in, bool rbf); -/** Normalize univalue-represented outputs and add them to the transaction */ +/** Normalize univalue-represented outputs */ +UniValue NormalizeOutputs(const UniValue& outputs_in); + +/** Normalize, parse, and add outputs to the transaction */ void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in); /** Create a transaction from univalue parameters */ From f7384b921c3460c7a3cc7827a68b2c613bd98f8e Mon Sep 17 00:00:00 2001 From: josibake Date: Fri, 22 Dec 2023 11:27:53 +0100 Subject: [PATCH 3/6] refactor: move parsing to new function Move the parsing and validation out of `AddOutputs` into its own function, `ParseOutputs`. This allows us to re-use this logic in `ParseRecipients` in a later commit, where the code is currently duplicated. The new `ParseOutputs` function returns a CTxDestination,CAmount tuples. This allows the caller to then translate the validated outputs into either CRecipients or CTxOuts. --- src/rpc/rawtransaction_util.cpp | 38 ++++++++++++++++++++------------- src/rpc/rawtransaction_util.h | 6 +++++- 2 files changed, 28 insertions(+), 16 deletions(-) diff --git a/src/rpc/rawtransaction_util.cpp b/src/rpc/rawtransaction_util.cpp index eb8067cc235..a9e11622a7a 100644 --- a/src/rpc/rawtransaction_util.cpp +++ b/src/rpc/rawtransaction_util.cpp @@ -97,15 +97,12 @@ UniValue NormalizeOutputs(const UniValue& outputs_in) return outputs; } -void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in) +std::vector> ParseOutputs(const UniValue& outputs) { - UniValue outputs(UniValue::VOBJ); - outputs = NormalizeOutputs(outputs_in); - // Duplicate checking std::set destinations; + std::vector> parsed_outputs; bool has_data{false}; - for (const std::string& name_ : outputs.getKeys()) { if (name_ == "data") { if (has_data) { @@ -113,11 +110,12 @@ void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in) } has_data = true; std::vector data = ParseHexV(outputs[name_].getValStr(), "Data"); - - CTxOut out(0, CScript() << OP_RETURN << data); - rawTx.vout.push_back(out); + CTxDestination destination{CNoDestination{CScript() << OP_RETURN << data}}; + CAmount amount{0}; + parsed_outputs.emplace_back(destination, amount); } else { - CTxDestination destination = DecodeDestination(name_); + CTxDestination destination{DecodeDestination(name_)}; + CAmount amount{AmountFromValue(outputs[name_])}; if (!IsValidDestination(destination)) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, std::string("Invalid Bitcoin address: ") + name_); } @@ -125,14 +123,24 @@ void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in) if (!destinations.insert(destination).second) { throw JSONRPCError(RPC_INVALID_PARAMETER, std::string("Invalid parameter, duplicated address: ") + name_); } - - CScript scriptPubKey = GetScriptForDestination(destination); - CAmount nAmount = AmountFromValue(outputs[name_]); - - CTxOut out(nAmount, scriptPubKey); - rawTx.vout.push_back(out); + parsed_outputs.emplace_back(destination, amount); } } + return parsed_outputs; +} + +void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in) +{ + UniValue outputs(UniValue::VOBJ); + outputs = NormalizeOutputs(outputs_in); + + std::vector> parsed_outputs = ParseOutputs(outputs); + for (const auto& [destination, nAmount] : parsed_outputs) { + CScript scriptPubKey = GetScriptForDestination(destination); + + CTxOut out(nAmount, scriptPubKey); + rawTx.vout.push_back(out); + } } CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniValue& outputs_in, const UniValue& locktime, std::optional rbf) diff --git a/src/rpc/rawtransaction_util.h b/src/rpc/rawtransaction_util.h index f3d5c3616ef..964d0b095b1 100644 --- a/src/rpc/rawtransaction_util.h +++ b/src/rpc/rawtransaction_util.h @@ -5,6 +5,8 @@ #ifndef BITCOIN_RPC_RAWTRANSACTION_UTIL_H #define BITCOIN_RPC_RAWTRANSACTION_UTIL_H +#include +#include #include #include #include @@ -38,13 +40,15 @@ void SignTransactionResultToJSON(CMutableTransaction& mtx, bool complete, const */ void ParsePrevouts(const UniValue& prevTxsUnival, FillableSigningProvider* keystore, std::map& coins); - /** Normalize univalue-represented inputs and add them to the transaction */ void AddInputs(CMutableTransaction& rawTx, const UniValue& inputs_in, bool rbf); /** Normalize univalue-represented outputs */ UniValue NormalizeOutputs(const UniValue& outputs_in); +/** Parse normalized outputs into destination, amount tuples */ +std::vector> ParseOutputs(const UniValue& outputs); + /** Normalize, parse, and add outputs to the transaction */ void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in); From 47353a608dc6e20e5fd2ca53850d6f9aa3240d4a Mon Sep 17 00:00:00 2001 From: josibake Date: Fri, 22 Dec 2023 13:58:56 +0100 Subject: [PATCH 4/6] refactor: remove out param from `ParseRecipients` Have `ParseRecipients` return a vector of `CRecipients` and rename to `CreateRecipients`. --- src/wallet/rpc/spend.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index 5a13b5ac8ec..eb3e6073201 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -24,8 +24,9 @@ namespace wallet { -static void ParseRecipients(const UniValue& address_amounts, const UniValue& subtract_fee_outputs, std::vector& recipients) +std::vector CreateRecipients(const UniValue& address_amounts, const UniValue& subtract_fee_outputs) { + std::vector recipients; std::set destinations; int i = 0; for (const std::string& address: address_amounts.getKeys()) { @@ -52,6 +53,7 @@ static void ParseRecipients(const UniValue& address_amounts, const UniValue& sub CRecipient recipient = {dest, amount, subtract_fee}; recipients.push_back(recipient); } + return recipients; } static void InterpretFeeEstimationInstructions(const UniValue& conf_target, const UniValue& estimate_mode, const UniValue& fee_rate, UniValue& options) @@ -301,8 +303,7 @@ RPCHelpMan sendtoaddress() subtractFeeFromAmount.push_back(address); } - std::vector recipients; - ParseRecipients(address_amounts, subtractFeeFromAmount, recipients); + std::vector recipients = CreateRecipients(address_amounts, subtractFeeFromAmount); const bool verbose{request.params[10].isNull() ? false : request.params[10].get_bool()}; return SendMoney(*pwallet, coin_control, recipients, mapValue, verbose); @@ -397,8 +398,7 @@ RPCHelpMan sendmany() SetFeeEstimateMode(*pwallet, coin_control, /*conf_target=*/request.params[6], /*estimate_mode=*/request.params[7], /*fee_rate=*/request.params[8], /*override_min_fee=*/false); - std::vector recipients; - ParseRecipients(sendTo, subtractFeeFromAmount, recipients); + std::vector recipients = CreateRecipients(sendTo, subtractFeeFromAmount); const bool verbose{request.params[9].isNull() ? false : request.params[9].get_bool()}; return SendMoney(*pwallet, coin_control, recipients, std::move(mapValue), verbose); From 5ad19668dbcc47486d1c18f711cea3d8a9d2e7e2 Mon Sep 17 00:00:00 2001 From: josibake Date: Mon, 2 Oct 2023 14:36:53 +0200 Subject: [PATCH 5/6] refactor: simplify `CreateRecipients` Move validation logic out of `CreateRecipients` and instead take the already validated outputs from `ParseOutputs` as an input. Move SFFO parsing out of `CreateRecipients` into a new function, `InterpretSubtractFeeFromOutputsInstructions`. This takes the SFFO instructions from `sendmany` and `sendtoaddress` and turns them into a set of integers. In a later commit, we will also move the SFFO parsing logic from `FundTransaction` into this function. Worth noting: a user can pass duplicate addresses and addresses that dont exist in the transaction outputs as SFFO args to `sendmany` and `sendtoaddress` without triggering a warning. This behavior is preserved in to keep this commit strictly a refactor. --- src/wallet/rpc/spend.cpp | 74 ++++++++++++++++++---------------------- 1 file changed, 33 insertions(+), 41 deletions(-) diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index eb3e6073201..6e39e4d5a6f 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -24,33 +24,12 @@ namespace wallet { -std::vector CreateRecipients(const UniValue& address_amounts, const UniValue& subtract_fee_outputs) +std::vector CreateRecipients(const std::vector>& outputs, const std::set& subtract_fee_outputs) { std::vector recipients; - std::set destinations; - int i = 0; - 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); - } - destinations.insert(dest); - - CAmount amount = AmountFromValue(address_amounts[i++]); - - bool subtract_fee = false; - for (unsigned int idx = 0; idx < subtract_fee_outputs.size(); idx++) { - const UniValue& addr = subtract_fee_outputs[idx]; - if (addr.get_str() == address) { - subtract_fee = true; - } - } - - CRecipient recipient = {dest, amount, subtract_fee}; + for (size_t i = 0; i < outputs.size(); ++i) { + const auto& [destination, amount] = outputs.at(i); + CRecipient recipient{destination, amount, subtract_fee_outputs.contains(i)}; recipients.push_back(recipient); } return recipients; @@ -78,6 +57,27 @@ static void InterpretFeeEstimationInstructions(const UniValue& conf_target, cons } } +std::set InterpretSubtractFeeFromOutputInstructions(const UniValue& sffo_instructions, const std::vector& destinations) +{ + std::set sffo_set; + if (sffo_instructions.isNull()) return sffo_set; + if (sffo_instructions.isBool()) { + if (sffo_instructions.get_bool()) sffo_set.insert(0); + return sffo_set; + } + for (const auto& sffo : sffo_instructions.getValues()) { + if (sffo.isStr()) { + for (size_t i = 0; i < destinations.size(); ++i) { + if (sffo.get_str() == destinations.at(i)) { + sffo_set.insert(i); + break; + } + } + } + } + return sffo_set; +} + static UniValue FinishTransaction(const std::shared_ptr pwallet, const UniValue& options, const CMutableTransaction& rawTx) { // Make a blank psbt @@ -277,11 +277,6 @@ RPCHelpMan sendtoaddress() if (!request.params[3].isNull() && !request.params[3].get_str().empty()) mapValue["to"] = request.params[3].get_str(); - bool fSubtractFeeFromAmount = false; - if (!request.params[4].isNull()) { - fSubtractFeeFromAmount = request.params[4].get_bool(); - } - CCoinControl coin_control; if (!request.params[5].isNull()) { coin_control.m_signal_bip125_rbf = request.params[5].get_bool(); @@ -298,12 +293,10 @@ RPCHelpMan sendtoaddress() UniValue address_amounts(UniValue::VOBJ); const std::string address = request.params[0].get_str(); address_amounts.pushKV(address, request.params[1]); - UniValue subtractFeeFromAmount(UniValue::VARR); - if (fSubtractFeeFromAmount) { - subtractFeeFromAmount.push_back(address); - } - - std::vector recipients = CreateRecipients(address_amounts, subtractFeeFromAmount); + std::vector recipients = CreateRecipients( + ParseOutputs(address_amounts), + InterpretSubtractFeeFromOutputInstructions(request.params[4], address_amounts.getKeys()) + ); const bool verbose{request.params[10].isNull() ? false : request.params[10].get_bool()}; return SendMoney(*pwallet, coin_control, recipients, mapValue, verbose); @@ -387,10 +380,6 @@ RPCHelpMan sendmany() if (!request.params[3].isNull() && !request.params[3].get_str().empty()) mapValue["comment"] = request.params[3].get_str(); - UniValue subtractFeeFromAmount(UniValue::VARR); - if (!request.params[4].isNull()) - subtractFeeFromAmount = request.params[4].get_array(); - CCoinControl coin_control; if (!request.params[5].isNull()) { coin_control.m_signal_bip125_rbf = request.params[5].get_bool(); @@ -398,7 +387,10 @@ RPCHelpMan sendmany() SetFeeEstimateMode(*pwallet, coin_control, /*conf_target=*/request.params[6], /*estimate_mode=*/request.params[7], /*fee_rate=*/request.params[8], /*override_min_fee=*/false); - std::vector recipients = CreateRecipients(sendTo, subtractFeeFromAmount); + std::vector recipients = CreateRecipients( + ParseOutputs(sendTo), + InterpretSubtractFeeFromOutputInstructions(request.params[4], sendTo.getKeys()) + ); const bool verbose{request.params[9].isNull() ? false : request.params[9].get_bool()}; return SendMoney(*pwallet, coin_control, recipients, std::move(mapValue), verbose); From 18ad1b9142e91cef2f5c6a693eeb2d0fbb8c517d Mon Sep 17 00:00:00 2001 From: josibake Date: Mon, 2 Oct 2023 16:57:44 +0200 Subject: [PATCH 6/6] refactor: pass CRecipient to FundTransaction Instead turning tx.vout into a vector of `CRecipient`, 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. This also allows us to remove SFFO from both `FundTransaction` function signatures. 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 | 78 +++++++++++++++++--------- src/wallet/spend.cpp | 15 ++--- src/wallet/spend.h | 2 +- src/wallet/test/fuzz/notifications.cpp | 13 ++++- 4 files changed, 70 insertions(+), 38 deletions(-) diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index 6e39e4d5a6f..6060f017ce9 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -74,6 +74,16 @@ std::set InterpretSubtractFeeFromOutputInstructions(const UniValue& sffo_in } } } + if (sffo.isNum()) { + int pos = sffo.getInt(); + if (sffo_set.contains(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(destinations.size())) + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid parameter, position too large: %d", pos)); + sffo_set.insert(pos); + } } return sffo_set; } @@ -480,17 +490,17 @@ 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) { + // We want to make sure tx.vout is not used now that we are passing outputs as a vector of recipients. + // This sets us up to remove tx completely in a future PR in favor of passing the inputs directly. + CHECK_NONFATAL(tx.vout.empty()); // 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(); 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 @@ -545,7 +555,7 @@ CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransact if (options.exists("changePosition") || options.exists("change_position")) { int pos = (options.exists("change_position") ? options["change_position"] : options["changePosition"]).getInt(); - if (pos < 0 || (unsigned int)pos > tx.vout.size()) { + if (pos < 0 || (unsigned int)pos > recipients.size()) { throw JSONRPCError(RPC_INVALID_PARAMETER, "changePosition out of bounds"); } change_position = (unsigned int)pos; @@ -587,9 +597,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(); } @@ -695,21 +702,10 @@ CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransact } } - if (tx.vout.size() == 0) + if (recipients.empty()) 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); } @@ -835,11 +831,25 @@ 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::vector> destinations; + for (const auto& tx_out : tx.vout) { + CTxDestination dest; + ExtractDestination(tx_out.scriptPubKey, dest); + destinations.emplace_back(dest, tx_out.nValue); + } + std::vector dummy(destinations.size(), "dummy"); + std::vector recipients = CreateRecipients( + destinations, + InterpretSubtractFeeFromOutputInstructions(options["subtractFeeFromOutputs"], dummy) + ); 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); + // Clear tx.vout since it is not meant to be used now that we are passing outputs directly. + // This sets us up for a future PR to completely remove tx from the function signature in favor of passing inputs directly + tx.vout.clear(); + auto txr = FundTransaction(*pwallet, tx, recipients, options, coin_control, /*override_min_fee=*/true); UniValue result(UniValue::VOBJ); result.pushKV("hex", EncodeHexTx(*txr.tx)); @@ -1267,13 +1277,22 @@ RPCHelpMan send() bool rbf{options.exists("replaceable") ? options["replaceable"].get_bool() : pwallet->m_signal_rbf}; + UniValue outputs(UniValue::VOBJ); + outputs = NormalizeOutputs(request.params[0]); + std::vector recipients = CreateRecipients( + ParseOutputs(outputs), + InterpretSubtractFeeFromOutputInstructions(options["subtract_fee_from_outputs"], outputs.getKeys()) + ); 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); + // Clear tx.vout since it is not meant to be used now that we are passing outputs directly. + // This sets us up for a future PR to completely remove tx from the function signature in favor of passing inputs directly + rawTx.vout.clear(); + auto txr = FundTransaction(*pwallet, rawTx, recipients, options, coin_control, /*override_min_fee=*/false); return FinishTransaction(pwallet, options, CMutableTransaction(*txr.tx)); } @@ -1703,12 +1722,21 @@ 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); + UniValue outputs(UniValue::VOBJ); + outputs = NormalizeOutputs(request.params[1]); + std::vector recipients = CreateRecipients( + ParseOutputs(outputs), + InterpretSubtractFeeFromOutputInstructions(options["subtractFeeFromOutputs"], outputs.getKeys()) + ); 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); + // Clear tx.vout since it is not meant to be used now that we are passing outputs directly. + // This sets us up for a future PR to completely remove tx from the function signature in favor of passing inputs directly + rawTx.vout.clear(); + 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 b51cd6332fb..a2bf4976dc8 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -1365,18 +1365,11 @@ 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); - } + // We want to make sure tx.vout is not used now that we are passing outputs as a vector of recipients. + // This sets us up to remove tx completely in a future PR in favor of passing the inputs directly. + assert(tx.vout.empty()); // Set the user desired locktime coinControl.m_locktime = tx.nLockTime; diff --git a/src/wallet/spend.h b/src/wallet/spend.h index 3bd37cfd0ec..62a7b4e4c89 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 203ab5f606a..376060421c1 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,10 @@ 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); + // Clear tx.vout since it is not meant to be used now that we are passing outputs directly. + // This sets us up for a future PR to completely remove tx from the function signature in favor of passing inputs directly + tx.vout.clear(); + (void)FundTransaction(*wallet, tx, recipients, change_position, /*lockUnspents=*/false, coin_control); } };