Skip to content

Commit

Permalink
refactor: pass CRecipient to FundTransaction
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@josibake
josibake committed Dec 11, 2023
1 parent ef1e445 commit ae53820
Show file tree
Hide file tree
Showing 4 changed files with 56 additions and 45 deletions.
76 changes: 45 additions & 31 deletions src/wallet/rpc/spend.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,16 +29,27 @@ std::vector<CRecipient> ParseRecipients(const UniValue& address_amounts, const s
std::set<CTxDestination> destinations;
std::vector<CRecipient> 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<unsigned char> 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++;
Expand Down Expand Up @@ -536,17 +547,14 @@ 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)
{
// 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 @@ -643,9 +651,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 @@ -754,18 +759,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<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 @@ -891,11 +885,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<int> 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<std::string> destinations(tx.vout.size(), "dummy");
setSubtractFeeFromOutputs = ParseSubtractFeeFromOutputs(subtractFeeFromOutputs, destinations);
}
std::vector<CRecipient> 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));
Expand Down Expand Up @@ -1323,13 +1335,14 @@ RPCHelpMan send()


bool rbf{options.exists("replaceable") ? options["replaceable"].get_bool() : pwallet->m_signal_rbf};
std::vector<CRecipient> 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));
}
Expand Down Expand Up @@ -1759,12 +1772,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<CRecipient> 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));
Expand Down
13 changes: 1 addition & 12 deletions src/wallet/spend.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1360,19 +1360,8 @@ 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);
}

// Set the user desired locktime
coinControl.m_locktime = tx.nLockTime;

Expand Down
2 changes: 1 addition & 1 deletion src/wallet/spend.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ util::Result<CreatedTransactionResult> CreateTransaction(CWallet& wallet, const
* Insert additional inputs into the transaction by
* calling CreateTransaction();
*/
util::Result<CreatedTransactionResult> FundTransaction(CWallet& wallet, const CMutableTransaction& tx, std::optional<unsigned int> change_pos, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl);
util::Result<CreatedTransactionResult> FundTransaction(CWallet& wallet, const CMutableTransaction& tx, const std::vector<CRecipient>& recipients, std::optional<unsigned int> change_pos, bool lockUnspents, CCoinControl);
} // namespace wallet

#endif // BITCOIN_WALLET_SPEND_H
10 changes: 9 additions & 1 deletion src/wallet/test/fuzz/notifications.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,14 @@ struct FuzzedWallet {
}
}
}
std::vector<CRecipient> 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(
Expand All @@ -158,7 +166,7 @@ struct FuzzedWallet {

int change_position{fuzzed_data_provider.ConsumeIntegralInRange<int>(-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);
}
};

Expand Down

0 comments on commit ae53820

Please sign in to comment.