Skip to content

Commit

Permalink
refactor: pass CRecipient to FundTransaction
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@josibake
josibake committed Dec 22, 2023
1 parent 711f6ab commit ebf118f
Show file tree
Hide file tree
Showing 4 changed files with 56 additions and 39 deletions.
70 changes: 45 additions & 25 deletions src/wallet/rpc/spend.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,20 @@ std::set<int> InterpretSubtractFeeFromOutputInstructions(const UniValue& options
}
return sffo_set;
}
if (options.exists("subtractFeeFromOutputs") || options.exists("subtract_fee_from_outputs") ) {
subtract_fee_outputs = (options.exists("subtract_fee_from_outputs") ? options["subtract_fee_from_outputs"] : options["subtractFeeFromOutputs"]).get_array();
for (const auto& sffo : subtract_fee_outputs.getValues()) {
pos = sffo.getInt<int>();
if (sffo_set.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(destinations.size()))
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid parameter, position too large: %d", pos));
sffo_set.insert(pos);
}
return sffo_set;
}
// If no SFFO instructions, return an empty set
return sffo_set;
}
Expand Down Expand Up @@ -495,17 +509,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 @@ -560,7 +571,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 @@ -602,9 +613,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 @@ -710,21 +718,10 @@ CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransact
}
}

if (tx.vout.size() == 0)
if (recipients.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 @@ -850,11 +847,22 @@ 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, 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);
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 @@ -1282,13 +1290,19 @@ 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, 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);
auto txr = FundTransaction(*pwallet, rawTx, recipients, options, coin_control, /*override_min_fee=*/false);

return FinishTransaction(pwallet, options, CMutableTransaction(*txr.tx));
}
Expand Down Expand Up @@ -1718,12 +1732,18 @@ 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, 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);
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 ebf118f

Please sign in to comment.