Skip to content

Commit

Permalink
Merge bitcoin#28560: wallet, rpc: FundTransaction refactor
Browse files Browse the repository at this point in the history 
18ad1b9 refactor: pass CRecipient to FundTransaction (josibake)
5ad1966 refactor: simplify `CreateRecipients` (josibake)
47353a6 refactor: remove out param from `ParseRecipients` (josibake)
f7384b9 refactor: move parsing to new function (josibake)
6f569ac refactor: move normalization to new function (josibake)
435fe5c test: add tests for fundrawtx and sendmany rpcs (josibake)

Pull request description:

  ## Motivation

  The primary motivation for this PR is to enable `FundTransaction` to take a vector of `CRecipient` objects to allow passing BIP352 silent payment addresses to RPCs that use `FundTransaction` (e.g. `send`, `walletcreatefundedpsbt`). To do that, SFFO logic needs to be moved out of `FundTransaction` so the `CRecipient` objects with the correct SFFO information can be created and then passed to `FundTransaction`.

  As a secondary motivation, this PR moves the SFFO stuff closer to the caller, making the code cleaner and easier to understand. This is done by having a single function which parses RPC inputs for SFFO and consistently using the `set<int>` method for communicating SFFO.

  I'm also not convinced we need to pass a full `CMutableTx` object to `FundTransaction`, but I'm leaving that for a follow-up PR/discussion, as its not a blocker for silent payments.

ACKs for top commit:
  S3RK:
    reACK 18ad1b9
  josibake:
    > According to my `range-diff` nothing changed. reACK [18ad1b9](bitcoin@18ad1b9)
  achow101:
    ACK 18ad1b9

Tree-SHA512: d61f017cf7d98489ef216475b68693fd77e7b53a26a6477dcd73e7e5ceff5036b2d21476e377839e710bb73644759d42c4f9f4b14ed96b3e56ed87b07aa6d1a7
  • Loading branch information
@achow101
achow101 committed Jan 23, 2024
2 parents 2f218c6 + 18ad1b9 commit e69796c
Show file tree
Hide file tree
Showing 9 changed files with 216 additions and 94 deletions.
37 changes: 26 additions & 11 deletions src/rpc/rawtransaction_util.cpp
View file
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);
} 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
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
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)
{
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)
{
// 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
Original file line number Diff line number Diff line change
Expand Up @@ -1377,18 +1377,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
Loading

0 comments on commit e69796c

Please sign in to comment.