Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

wallet, rpc: FundTransaction refactor #28560

Merged
merged 6 commits into from
Jan 23, 2024
Merged

wallet, rpc: FundTransaction refactor #28560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
435fe5c
test: add tests for fundrawtx and sendmany rpcs
josibake Dec 22, 2023
6f569ac
refactor: move normalization to new function
josibake Dec 22, 2023
f7384b9
refactor: move parsing to new function
josibake Dec 22, 2023
47353a6
refactor: remove out param from `ParseRecipients`
josibake Dec 22, 2023
5ad1966
refactor: simplify `CreateRecipients`
josibake Oct 2, 2023
18ad1b9
refactor: pass CRecipient to FundTransaction
josibake Oct 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
37 changes: 26 additions & 11 deletions src/rpc/rawtransaction_util.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand All @@ -94,37 +94,52 @@ void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in)
}
outputs = std::move(outputs_dict);
}
return outputs;
}

std::vector<std::pair<CTxDestination, CAmount>> ParseOutputs(const UniValue& outputs)
{
// Duplicate checking
std::set<CTxDestination> destinations;
std::vector<std::pair<CTxDestination, CAmount>> parsed_outputs;
bool has_data{false};

for (const std::string& name_ : outputs.getKeys()) {
if (name_ == "data") {
if (has_data) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, duplicate key: data");
}
has_data = true;
std::vector<unsigned char> 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);
Comment on lines +113 to +115
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In f7384b9 "refactor: move parsing to new function"

nit: Making temp variables is not necessary.

} 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_);
}

if (!destinations.insert(destination).second) {
throw JSONRPCError(RPC_INVALID_PARAMETER, std::string("Invalid parameter, duplicated address: ") + name_);
}
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);

CScript scriptPubKey = GetScriptForDestination(destination);
CAmount nAmount = AmountFromValue(outputs[name_]);
std::vector<std::pair<CTxDestination, CAmount>> parsed_outputs = ParseOutputs(outputs);
for (const auto& [destination, nAmount] : parsed_outputs) {
CScript scriptPubKey = GetScriptForDestination(destination);

CTxOut out(nAmount, scriptPubKey);
rawTx.vout.push_back(out);
}
CTxOut out(nAmount, scriptPubKey);
rawTx.vout.push_back(out);
}
}

Expand Down
11 changes: 9 additions & 2 deletions src/rpc/rawtransaction_util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
#ifndef BITCOIN_RPC_RAWTRANSACTION_UTIL_H
#define BITCOIN_RPC_RAWTRANSACTION_UTIL_H

#include <addresstype.h>
#include <consensus/amount.h>
#include <map>
#include <string>
#include <optional>
Expand Down Expand Up @@ -38,11 +40,16 @@ void SignTransactionResultToJSON(CMutableTransaction& mtx, bool complete, const
*/
void ParsePrevouts(const UniValue& prevTxsUnival, FillableSigningProvider* keystore, std::map<COutPoint, Coin>& 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 and add them to the transaction */
/** Normalize univalue-represented outputs */
UniValue NormalizeOutputs(const UniValue& outputs_in);

/** Parse normalized outputs into destination, amount tuples */
std::vector<std::pair<CTxDestination, CAmount>> ParseOutputs(const UniValue& outputs);

/** Normalize, parse, and add outputs to the transaction */
void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in);

/** Create a transaction from univalue parameters */
Expand Down
156 changes: 88 additions & 68 deletions src/wallet/rpc/spend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,34 +24,15 @@


namespace wallet {
static void ParseRecipients(const UniValue& address_amounts, const UniValue& subtract_fee_outputs, std::vector<CRecipient>& recipients)
std::vector<CRecipient> CreateRecipients(const std::vector<std::pair<CTxDestination, CAmount>>& outputs, const std::set<int>& subtract_fee_outputs)
{
std::set<CTxDestination> 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};
std::vector<CRecipient> recipients;
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;
}

static void InterpretFeeEstimationInstructions(const UniValue& conf_target, const UniValue& estimate_mode, const UniValue& fee_rate, UniValue& options)
Expand All @@ -76,6 +57,37 @@ static void InterpretFeeEstimationInstructions(const UniValue& conf_target, cons
}
}

std::set<int> InterpretSubtractFeeFromOutputInstructions(const UniValue& sffo_instructions, const std::vector<std::string>& destinations)
Copy link
Contributor

@S3RK S3RK Jan 10, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in 0b0276b

I don't see a reason to extract this function at this point (could be done later in the PR).
There are two disparate cases covered (bool and vector<string>). And each case is only used in one and only one place, further commits also don't introduce further usage of this code path as they pass vector<int>.

Wouldn't it be better if we keep this logic at the call site?
Benefits of my proposal:

  • we don't need second parameter for this function
  • we don't need to pass dummy value in fundrawtransaction RPC code
  • we can combine NormalizeOutputs and ParseOutputs. they are used always together and the only reason for NormalizeOutputs to exist now is too pass second param to InterpretSubtractFeeFromOutputInstructions in send and walletfundpsbt RPC. But there second parameter is useless as subtract_fee_from_outputs option of those RPCs can't be of the vector<string> type

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer having a single function (as opposed to keeping the logic at the call site) for the following reasons:

  • We avoid duplicating code at the RPC call sites:
    • The logic for validating SFFO inputs would need to be duplicated at send, fundrawtx and walletcreatefundedpsbt. This is the code that checks that the ints passed are not duplicated and in bounds and previously used to be inside rpc/FundTransaction
  • We have one place to validate all SFFO inputs:
    • Currently, sendmany SFFO inputs are not validated (documented in the added functional tests)
    • Having one function for parsing and validating makes it cleaner to add validation for all inputs in a follow-up PR. The only reason I'm not doing it in this PR is that its a behavior change (it could make calls to sendmany invalid that were previously accepted) and I'd like to keep this strictly a refactor

we don't need second parameter for this function

In the case of sendmany, we do need the array of destinations to lookup their position. In the case of send, walletcreatefundedpsbt, and fundrawtransaction, we need the array of destinations to make sure our ints are in bounds. There are definitely other ways to do it, but it seems fine to use the same vector for lookups and size. More just making the point that the argument is always used.

we can combine NormalizeOutputs and ParseOutputs. they are used always together

Actually, the main goal here was to separate the logic in AddOutputs to get rid of duplicate validation logic in AddOutputs and also the original ParseRecipients function. sendtoaddress and sendtoaddress need the validation logic, but don't need NormalizeOutputs since those RPCs already only allow the user to pass a key-value pair for address and amounts.

The other RPCs (send, fundrawtransaction, walletcreatefundedpsbt) need the logic for parsing how the univalue is passed (NormalizeOutputs) and validating the addresses (ParseOutputs).

{
std::set<int> 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;
}
}
}
if (sffo.isNum()) {
int pos = sffo.getInt<int>();
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;
}

static UniValue FinishTransaction(const std::shared_ptr<CWallet> pwallet, const UniValue& options, const CMutableTransaction& rawTx)
{
// Make a blank psbt
Expand Down Expand Up @@ -275,11 +287,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();
Expand All @@ -296,13 +303,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<CRecipient> recipients;
ParseRecipients(address_amounts, subtractFeeFromAmount, recipients);
std::vector<CRecipient> 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);
Expand Down Expand Up @@ -386,19 +390,17 @@ 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();
}

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<CRecipient> recipients;
ParseRecipients(sendTo, subtractFeeFromAmount, recipients);
std::vector<CRecipient> 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);
Expand Down Expand Up @@ -488,17 +490,17 @@ static std::vector<RPCArg> 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<CRecipient>& recipients, const UniValue& options, CCoinControl& coinControl, bool override_min_fee)
josibake marked this conversation as resolved.
Show resolved Hide resolved
{
// 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<unsigned int> change_position;
bool lockUnspents = false;
UniValue subtractFeeFromOutputs;
std::set<int> setSubtractFeeFromOutputs;

if (!options.isNull()) {
if (options.type() == UniValue::VBOOL) {
// backward compatibility bool only fallback
Expand Down Expand Up @@ -553,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<int>();
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;
Expand Down Expand Up @@ -595,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();
}
Expand Down Expand Up @@ -703,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<int>();
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);
}
Expand Down Expand Up @@ -843,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<std::pair<CTxDestination, CAmount>> destinations;
for (const auto& tx_out : tx.vout) {
CTxDestination dest;
ExtractDestination(tx_out.scriptPubKey, dest);
destinations.emplace_back(dest, tx_out.nValue);
}
std::vector<std::string> dummy(destinations.size(), "dummy");
std::vector<CRecipient> 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));
Expand Down Expand Up @@ -1275,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<CRecipient> 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));
}
Expand Down Expand Up @@ -1711,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<CRecipient> 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));
Expand Down
15 changes: 4 additions & 11 deletions src/wallet/spend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1365,18 +1365,11 @@ util::Result<CreatedTransactionResult> CreateTransaction(
return res;
}

util::Result<CreatedTransactionResult> FundTransaction(CWallet& wallet, const CMutableTransaction& tx, std::optional<unsigned int> change_pos, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl coinControl)
util::Result<CreatedTransactionResult> FundTransaction(CWallet& wallet, const CMutableTransaction& tx, const std::vector<CRecipient>& vecSend, std::optional<unsigned int> change_pos, bool lockUnspents, CCoinControl coinControl)
{
std::vector<CRecipient> 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;
Expand Down