Skip to content

Commit

Permalink
rpc: bumpfee, improve doc for 'reduce_output' arg
Browse files Browse the repository at this point in the history 
The current argument name and description is dangerous as it doesn't
describe the case where the user selects the recipient output as the
change address. This one could end up been increased by the remainder
between the inputs minus outputs. Which, when `bumpfee` adds new inputs
to the transaction, leads the process to send more coins to the
recipient. Which is not what the user would expect by the previous
'reduce_output' param naming.
  • Loading branch information
@furszy
furszy committed Sep 19, 2023
1 parent f01416e commit 4dbc595
Show file tree
Hide file tree
Showing 5 changed files with 25 additions and 25 deletions.
4 changes: 2 additions & 2 deletions src/rpc/client.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -263,13 +263,13 @@ static const CRPCConvertParam vRPCConvertParams[] =
{ "bumpfee", 1, "fee_rate"},
{ "bumpfee", 1, "replaceable"},
{ "bumpfee", 1, "outputs"},
{ "bumpfee", 1, "reduce_output"},
{ "bumpfee", 1, "change_output_index"},
{ "psbtbumpfee", 1, "options" },
{ "psbtbumpfee", 1, "conf_target"},
{ "psbtbumpfee", 1, "fee_rate"},
{ "psbtbumpfee", 1, "replaceable"},
{ "psbtbumpfee", 1, "outputs"},
{ "psbtbumpfee", 1, "reduce_output"},
{ "psbtbumpfee", 1, "change_output_index"},
{ "logging", 0, "include" },
{ "logging", 1, "exclude" },
{ "disconnectnode", 1, "nodeid" },
Expand Down
10 changes: 5 additions & 5 deletions src/wallet/feebumper.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,10 +162,10 @@ bool TransactionCanBeBumped(const CWallet& wallet, const uint256& txid)
}

Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCoinControl& coin_control, std::vector<bilingual_str>& errors,
CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx, bool require_mine, const std::vector<CTxOut>& outputs, std::optional<uint32_t> reduce_output)
CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx, bool require_mine, const std::vector<CTxOut>& outputs, std::optional<uint32_t> change_output_index)
{
// Cannot both specify new outputs and an output to reduce
if (!outputs.empty() && reduce_output.has_value()) {
if (!outputs.empty() && change_output_index.has_value()) {
errors.push_back(Untranslated("Cannot specify both new outputs to use and an output index to reduce"));
return Result::INVALID_PARAMETER;
}
Expand All @@ -182,8 +182,8 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
}
const CWalletTx& wtx = it->second;

// Make sure that reduce_output is valid
if (reduce_output.has_value() && reduce_output.value() >= wtx.tx->vout.size()) {
// Make sure that change_output_index is valid
if (change_output_index.has_value() && change_output_index.value() >= wtx.tx->vout.size()) {
errors.push_back(Untranslated("Change position is out of range"));
return Result::INVALID_PARAMETER;
}
Expand Down Expand Up @@ -257,7 +257,7 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
const auto& txouts = outputs.empty() ? wtx.tx->vout : outputs;
for (size_t i = 0; i < txouts.size(); ++i) {
const CTxOut& output = txouts.at(i);
if (reduce_output.has_value() ? reduce_output.value() == i : OutputIsChange(wallet, output)) {
if (change_output_index.has_value() ? change_output_index.value() == i : OutputIsChange(wallet, output)) {
CTxDestination change_dest;
ExtractDestination(output.scriptPubKey, change_dest);
new_coin_control.destChange = change_dest;
Expand Down
4 changes: 2 additions & 2 deletions src/wallet/feebumper.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ bool TransactionCanBeBumped(const CWallet& wallet, const uint256& txid);
* @param[out] mtx The bump transaction itself
* @param[in] require_mine Whether the original transaction must consist of inputs that can be spent by the wallet
* @param[in] outputs Vector of new outputs to replace the bumped transaction's outputs
* @param[in] reduce_output The position of the change output to deduct the fee from in the transaction being bumped
* @param[in] change_output_index The position of the change output to deduct the fee from in the transaction being bumped
*/
Result CreateRateBumpTransaction(CWallet& wallet,
const uint256& txid,
Expand All @@ -55,7 +55,7 @@ Result CreateRateBumpTransaction(CWallet& wallet,
CMutableTransaction& mtx,
bool require_mine,
const std::vector<CTxOut>& outputs,
std::optional<uint32_t> reduce_output = std::nullopt);
std::optional<uint32_t> change_output_index = std::nullopt);

//! Sign the new transaction,
//! @return false if the tx couldn't be found or if it was
Expand Down
14 changes: 7 additions & 7 deletions src/wallet/rpc/spend.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1017,10 +1017,10 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
{"outputs", RPCArg::Type::ARR, RPCArg::Default{UniValue::VARR}, "The outputs specified as key-value pairs.\n"
"Each key may only appear once, i.e. there can only be one 'data' output, and no address may be duplicated.\n"
"At least one output of either type must be specified.\n"
"Cannot be provided if 'reduce_output' is specified.",
"Cannot be provided if 'change_output_index' is specified.",
OutputsDoc(),
RPCArgOptions{.skip_type_check = true}},
{"reduce_output", RPCArg::Type::NUM, RPCArg::DefaultHint{"not set, detect change automatically"}, "The 0-based index of the output from which the additional fees will be deducted. In general, this should be the position of change output. Cannot be provided if 'outputs' is specified."},
{"change_output_index", RPCArg::Type::NUM, RPCArg::DefaultHint{"not set, detect change automatically"}, "The change of the bumped transaction will be sent to the output script specified by this 0-based index parameter. The to-be-replaced transaction output pointed by this parameter could either be increased or decreased depending on the inputs minus outputs remainder. Cannot be provided if 'outputs' is specified."},
},
RPCArgOptions{.oneline_description="options"}},
},
Expand Down Expand Up @@ -1059,7 +1059,7 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
coin_control.m_signal_bip125_rbf = true;
std::vector<CTxOut> outputs;

std::optional<uint32_t> reduce_output;
std::optional<uint32_t> change_output_index;

if (!request.params[1].isNull()) {
UniValue options = request.params[1];
Expand All @@ -1071,7 +1071,7 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
{"replaceable", UniValueType(UniValue::VBOOL)},
{"estimate_mode", UniValueType(UniValue::VSTR)},
{"outputs", UniValueType()}, // will be checked by AddOutputs()
{"reduce_output", UniValueType(UniValue::VNUM)},
{"change_output_index", UniValueType(UniValue::VNUM)},
},
true, true);

Expand All @@ -1096,8 +1096,8 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
outputs = tempTx.vout;
}

if (options.exists("reduce_output")) {
reduce_output = options["reduce_output"].getInt<uint32_t>();
if (options.exists("change_output_index")) {
change_output_index = options["change_output_index"].getInt<uint32_t>();
}
}

Expand All @@ -1116,7 +1116,7 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
CMutableTransaction mtx;
feebumper::Result res;
// Targeting feerate bump.
res = feebumper::CreateRateBumpTransaction(*pwallet, hash, coin_control, errors, old_fee, new_fee, mtx, /*require_mine=*/ !want_psbt, outputs, reduce_output);
res = feebumper::CreateRateBumpTransaction(*pwallet, hash, coin_control, errors, old_fee, new_fee, mtx, /*require_mine=*/ !want_psbt, outputs, change_output_index);
if (res != feebumper::Result::OK) {
switch(res) {
case feebumper::Result::INVALID_ADDRESS_OR_KEY:
Expand Down
18 changes: 9 additions & 9 deletions test/functional/wallet_bumpfee.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,12 +178,12 @@ def test_invalid_parameters(self, rbf_node, peer_node, dest_address):
assert_raises_rpc_error(-8, "Invalid parameter, duplicate key: data",
rbf_node.bumpfee, rbfid, {"outputs": [{"data": "deadbeef"}, {"data": "deadbeef"}]})

self.log.info("Test reduce_output option")
assert_raises_rpc_error(-1, "JSON integer out of range", rbf_node.bumpfee, rbfid, {"reduce_output": -1})
assert_raises_rpc_error(-8, "Change position is out of range", rbf_node.bumpfee, rbfid, {"reduce_output": 2})
self.log.info("Test change_output_index option")
assert_raises_rpc_error(-1, "JSON integer out of range", rbf_node.bumpfee, rbfid, {"change_output_index": -1})
assert_raises_rpc_error(-8, "Change position is out of range", rbf_node.bumpfee, rbfid, {"change_output_index": 2})

self.log.info("Test outputs and reduce_output cannot both be provided")
assert_raises_rpc_error(-8, "Cannot specify both new outputs to use and an output index to reduce", rbf_node.bumpfee, rbfid, {"reduce_output": 2, "outputs": [{dest_address: 0.1}]})
self.log.info("Test outputs and change_output_index cannot both be provided")
assert_raises_rpc_error(-8, "Cannot specify both new outputs to use and an output index to reduce", rbf_node.bumpfee, rbfid, {"change_output_index": 2, "outputs": [{dest_address: 0.1}]})

self.clear_mempool()

Expand Down Expand Up @@ -237,7 +237,7 @@ def test_bump_back_to_yourself(self):
node.unloadwallet("back_to_yourself")

def test_provided_change_pos(self, rbf_node):
self.log.info("Test the reduce_output option")
self.log.info("Test the change_output_index option")

change_addr = rbf_node.getnewaddress()
dest_addr = rbf_node.getnewaddress()
Expand All @@ -254,7 +254,7 @@ def test_provided_change_pos(self, rbf_node):
change_pos = find_vout_for_address(rbf_node, txid, change_addr)
change_value = tx["decoded"]["vout"][change_pos]["value"]

bumped = rbf_node.bumpfee(txid, {"reduce_output": change_pos})
bumped = rbf_node.bumpfee(txid, {"change_output_index": change_pos})
new_txid = bumped["txid"]

new_tx = rbf_node.gettransaction(txid=new_txid, verbose=True)
Expand Down Expand Up @@ -283,10 +283,10 @@ def test_single_output(self):
tx = wallet.sendall(recipients=[wallet.getnewaddress()], fee_rate=2, options={"inputs": [utxos[0]]})

# Reduce the only output with a crazy high feerate, should fail as the output would be dust
assert_raises_rpc_error(-4, "The transaction amount is too small to pay the fee", wallet.bumpfee, txid=tx["txid"], options={"fee_rate": 1100, "reduce_output": 0})
assert_raises_rpc_error(-4, "The transaction amount is too small to pay the fee", wallet.bumpfee, txid=tx["txid"], options={"fee_rate": 1100, "change_output_index": 0})

# Reduce the only output successfully
bumped = wallet.bumpfee(txid=tx["txid"], options={"fee_rate": 10, "reduce_output": 0})
bumped = wallet.bumpfee(txid=tx["txid"], options={"fee_rate": 10, "change_output_index": 0})
bumped_tx = wallet.gettransaction(txid=bumped["txid"], verbose=True)
assert_equal(len(bumped_tx["decoded"]["vout"]), 1)
assert_equal(len(bumped_tx["decoded"]["vin"]), 1)
Expand Down

0 comments on commit 4dbc595

Please sign in to comment.