Skip to content

Commit

Permalink
Merge bitcoin/bitcoin#28505: rpc: bumpfee, improve doc for 'reduce_ou…
Browse files Browse the repository at this point in the history 
…tput' arg

b3db8c9 rpc: bumpfee, improve doc for 'reduce_output' arg (furszy)

Pull request description:

  Fixes #28180. Resulted from discussions with S3RK, achow101, and Murch.

  The current argument name and description are dangerous as it don't
  describe the case where the user selects the recipient output as the
  change address. This one could end up been increased by the inputs
  minus outputs remainder. 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 from a
  'reduce_output' param naming.

ACKs for top commit:
  S3RK:
    ACK b3db8c9
  achow101:
    ACK b3db8c9
  murchandamus:
    ACK b3db8c9

Tree-SHA512: 91f607e2f5849041d7c099afdddae11af8bed5b1ac90c9d22921267f272e21b44e107d6968e037f05f958a61fe29e94e5fb44b224fb3606f197f83ec4ba3b1e7
  • Loading branch information
@achow101
achow101 committed Sep 27, 2023
2 parents c9f2882 + b3db8c9 commit 19a7e60
Show file tree
Hide file tree
Showing 5 changed files with 34 additions and 30 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, "original_change_index"},
{ "psbtbumpfee", 1, "options" },
{ "psbtbumpfee", 1, "conf_target"},
{ "psbtbumpfee", 1, "fee_rate"},
{ "psbtbumpfee", 1, "replaceable"},
{ "psbtbumpfee", 1, "outputs"},
{ "psbtbumpfee", 1, "reduce_output"},
{ "psbtbumpfee", 1, "original_change_index"},
{ "logging", 0, "include" },
{ "logging", 1, "exclude" },
{ "disconnectnode", 1, "nodeid" },
Expand Down
14 changes: 7 additions & 7 deletions src/wallet/feebumper.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,11 +162,11 @@ 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> original_change_index)
{
// Cannot both specify new outputs and an output to reduce
if (!outputs.empty() && reduce_output.has_value()) {
errors.push_back(Untranslated("Cannot specify both new outputs to use and an output index to reduce"));
// For now, cannot specify both new outputs to use and an output index to send change
if (!outputs.empty() && original_change_index.has_value()) {
errors.push_back(Untranslated("The options 'outputs' and 'original_change_index' are incompatible. You can only either specify a new set of outputs, or designate a change output to be recycled."));
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 original_change_index is valid
if (original_change_index.has_value() && original_change_index.value() >= wtx.tx->vout.size()) {
errors.push_back(Untranslated("Change position is out of range"));
return Result::INVALID_PARAMETER;
}
Expand Down Expand Up @@ -259,7 +259,7 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
const CTxOut& output = txouts.at(i);
CTxDestination dest;
ExtractDestination(output.scriptPubKey, dest);
if (reduce_output.has_value() ? reduce_output.value() == i : OutputIsChange(wallet, output)) {
if (original_change_index.has_value() ? original_change_index.value() == i : OutputIsChange(wallet, output)) {
new_coin_control.destChange = dest;
} else {
CRecipient recipient = {dest, output.nValue, false};
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] original_change_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> original_change_index = std::nullopt);

//! Sign the new transaction,
//! @return false if the tx couldn't be found or if it was
Expand Down
18 changes: 11 additions & 7 deletions src/wallet/rpc/spend.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1016,10 +1016,14 @@ 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 'original_change_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."},
{"original_change_index", RPCArg::Type::NUM, RPCArg::DefaultHint{"not set, detect change automatically"}, "The 0-based index of the change output on the original transaction. "
"The indicated output will be recycled into the new change output on the bumped transaction. "
"The remainder after paying the recipients and fees will be sent to the output script of the "
"original change output. The change output’s amount can increase if bumping the transaction "
"adds new inputs, otherwise it will decrease. Cannot be used in combination with the 'outputs' option."},
},
RPCArgOptions{.oneline_description="options"}},
},
Expand Down Expand Up @@ -1058,7 +1062,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> original_change_index;

if (!request.params[1].isNull()) {
UniValue options = request.params[1];
Expand All @@ -1070,7 +1074,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)},
{"original_change_index", UniValueType(UniValue::VNUM)},
},
true, true);

Expand All @@ -1095,8 +1099,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("original_change_index")) {
original_change_index = options["original_change_index"].getInt<uint32_t>();
}
}

Expand All @@ -1115,7 +1119,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, original_change_index);
if (res != feebumper::Result::OK) {
switch(res) {
case feebumper::Result::INVALID_ADDRESS_OR_KEY:
Expand Down
24 changes: 12 additions & 12 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 original_change_index option")
assert_raises_rpc_error(-1, "JSON integer out of range", rbf_node.bumpfee, rbfid, {"original_change_index": -1})
assert_raises_rpc_error(-8, "Change position is out of range", rbf_node.bumpfee, rbfid, {"original_change_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 original_change_index cannot both be provided")
assert_raises_rpc_error(-8, "The options 'outputs' and 'original_change_index' are incompatible. You can only either specify a new set of outputs, or designate a change output to be recycled.", rbf_node.bumpfee, rbfid, {"original_change_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 original_change_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, {"original_change_index": change_pos})
new_txid = bumped["txid"]

new_tx = rbf_node.gettransaction(txid=new_txid, verbose=True)
Expand Down Expand Up @@ -282,18 +282,18 @@ 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})
# Set the only output with a crazy high feerate as change, 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, "original_change_index": 0})

# Reduce the only output successfully
bumped = wallet.bumpfee(txid=tx["txid"], options={"fee_rate": 10, "reduce_output": 0})
# Specify single output as change successfully
bumped = wallet.bumpfee(txid=tx["txid"], options={"fee_rate": 10, "original_change_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)
assert_equal(bumped_tx["decoded"]["vout"][0]["value"] + bumped["fee"], amount)
assert_fee_amount(bumped["fee"], bumped_tx["decoded"]["vsize"], Decimal(10) / Decimal(1e8) * 1000)

# Bumping without reducing adds a new input and output
# Bumping without specifying change adds a new input and output
bumped = wallet.bumpfee(txid=bumped["txid"], options={"fee_rate": 20})
bumped_tx = wallet.gettransaction(txid=bumped["txid"], verbose=True)
assert_equal(len(bumped_tx["decoded"]["vout"]), 2)
Expand Down

0 comments on commit 19a7e60

Please sign in to comment.