From b3db8c9d5ccfe5c31341169fa7ac044427122921 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 19 Sep 2023 08:09:23 -0300 Subject: [PATCH] rpc: bumpfee, improve doc for 'reduce_output' arg 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. Co-authored-by: Murch --- src/rpc/client.cpp | 4 ++-- src/wallet/feebumper.cpp | 14 +++++++------- src/wallet/feebumper.h | 4 ++-- src/wallet/rpc/spend.cpp | 18 +++++++++++------- test/functional/wallet_bumpfee.py | 24 ++++++++++++------------ 5 files changed, 34 insertions(+), 30 deletions(-) diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 0ee3f27761e..1e5e231cefa 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -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" }, diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index 512a011dfc0..f4cb4bbd66c 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -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& errors, - CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx, bool require_mine, const std::vector& outputs, std::optional reduce_output) + CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx, bool require_mine, const std::vector& outputs, std::optional 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; } @@ -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; } @@ -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}; diff --git a/src/wallet/feebumper.h b/src/wallet/feebumper.h index f00bf15730f..d3d43861efc 100644 --- a/src/wallet/feebumper.h +++ b/src/wallet/feebumper.h @@ -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, @@ -55,7 +55,7 @@ Result CreateRateBumpTransaction(CWallet& wallet, CMutableTransaction& mtx, bool require_mine, const std::vector& outputs, - std::optional reduce_output = std::nullopt); + std::optional original_change_index = std::nullopt); //! Sign the new transaction, //! @return false if the tx couldn't be found or if it was diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index c2f4321a608..6b96fc4e49f 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -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"}}, }, @@ -1058,7 +1062,7 @@ static RPCHelpMan bumpfee_helper(std::string method_name) coin_control.m_signal_bip125_rbf = true; std::vector outputs; - std::optional reduce_output; + std::optional original_change_index; if (!request.params[1].isNull()) { UniValue options = request.params[1]; @@ -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); @@ -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(); + if (options.exists("original_change_index")) { + original_change_index = options["original_change_index"].getInt(); } } @@ -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: diff --git a/test/functional/wallet_bumpfee.py b/test/functional/wallet_bumpfee.py index c66f279e88d..fea933a93b2 100755 --- a/test/functional/wallet_bumpfee.py +++ b/test/functional/wallet_bumpfee.py @@ -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() @@ -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() @@ -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) @@ -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)