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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | murchandamus, S3RK, achow101 |
| Concept ACK | ismaelsadeeq |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
1016@@ -1017,10 +1017,10 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
1017 {"outputs", RPCArg::Type::ARR, RPCArg::Default{UniValue::VARR}, "The outputs specified as key-value pairs.\n"
1018 "Each key may only appear once, i.e. there can only be one 'data' output, and no address may be duplicated.\n"
1019 "At least one output of either type must be specified.\n"
1020- "Cannot be provided if 'reduce_output' is specified.",
1021+ "Cannot be provided if 'change_output_index' is specified.",
1022 OutputsDoc(),
1023 RPCArgOptions{.skip_type_check = true}},
1024- {"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."},
1025+ {"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."},
282@@ -283,10 +283,10 @@ def test_single_output(self):
283 tx = wallet.sendall(recipients=[wallet.getnewaddress()], fee_rate=2, options={"inputs": [utxos[0]]})
284
285 # Reduce the only output with a crazy high feerate, should fail as the output would be dust
282@@ -283,10 +283,10 @@ def test_single_output(self):
283 tx = wallet.sendall(recipients=[wallet.getnewaddress()], fee_rate=2, options={"inputs": [utxos[0]]})
284
285 # Reduce the only output with a crazy high feerate, should fail as the output would be dust
286- 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})
287+ 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})
288
289 # Reduce the only output successfully
161@@ -162,10 +162,10 @@ bool TransactionCanBeBumped(const CWallet& wallet, const uint256& txid)
162 }
163
164 Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCoinControl& coin_control, std::vector<bilingual_str>& errors,
165- CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx, bool require_mine, const std::vector<CTxOut>& outputs, std::optional<uint32_t> reduce_output)
166+ CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx, bool require_mine, const std::vector<CTxOut>& outputs, std::optional<uint32_t> change_output_index)
167 {
168 // Cannot both specify new outputs and an output to reduce
Approach ACK
Explicitly specifying change output for bumpfee and reducing an arbitrary output is two different features. #26467 originally was developed to specify the change output, hence updating documentation is the correct way to fix #28180 for 26.0
We can discuss whether we want to support deducting fees from an arbitrary output in a separate issue.
262@@ -263,13 +263,13 @@ static const CRPCConvertParam vRPCConvertParams[] =
263 { "bumpfee", 1, "fee_rate"},
264 { "bumpfee", 1, "replaceable"},
265 { "bumpfee", 1, "outputs"},
266- { "bumpfee", 1, "reduce_output"},
267+ { "bumpfee", 1, "change_output_index"},
original_change_index might be more self-explanatory.
1015@@ -1016,10 +1016,10 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
1016 {"outputs", RPCArg::Type::ARR, RPCArg::Default{UniValue::VARR}, "The outputs specified as key-value pairs.\n"
1017 "Each key may only appear once, i.e. there can only be one 'data' output, and no address may be duplicated.\n"
1018 "At least one output of either type must be specified.\n"
1019- "Cannot be provided if 'reduce_output' is specified.",
1020+ "Cannot be provided if 'change_output_index' is specified.",
1021 OutputsDoc(),
1022 RPCArgOptions{.skip_type_check = true}},
1023- {"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."},
1024+ {"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."},
How about:
0 {"change_output_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."},
168- // Cannot both specify new outputs and an output to reduce
169- if (!outputs.empty() && reduce_output.has_value()) {
170- errors.push_back(Untranslated("Cannot specify both new outputs to use and an output index to reduce"));
171+ // For now, cannot specify both new outputs to use and an output index to send change
172+ if (!outputs.empty() && change_output_index.has_value()) {
173+ errors.push_back(Untranslated("Cannot specify both new outputs to use and an output index to send change"));
0 errors.push_back(Untranslated("The options 'outputs' and 'change_output_index' are incompatible. You can only either specify a new set of outputs, or designate a change output to be recycled."));
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 <murch@murch.one>
ACK b3db8c9d5ccfe5c31341169fa7ac044427122921
Maybe add a test case where bumping leads to new input and increased change output?
The bug here is the behaviour, not the documentation! It shouldn’t do that!
Changing the documentation to reflect the current behavior does not preclude reintroducing reduce_output in the future that has the correct behavior. The discussion at CoreDev concluded that both an option to indicate which output is change and an option to indicate which output the fee should be taken from are reasonable things that a user may wish to do. As the former is what was implemented, the documentation should reflect that.
Labels
RPC/REST/ZMQ
CI failed
Milestone
26.0