Repository: bitcoin/bitcoin (language: cpp)

Issue:
Unsafe reduce_output when new coins are added
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

See inline discussion at https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1269177446

@whitslack:

> Consider the case where I am paying someone on chain under the stipulation that _they_ will eat the mining fee. (Thus, I specify _their_ address in `subtractfeefrom` when I'm constructing my transaction.) Later, they tell me that they can't wait for confirmation any longer and need an urgent fee bump. If I naïvely specify their output as the `reduce_output`, I may end up paying them _**much more**_ than I intended to if I only have large UTxOs in my wallet.


### Expected behaviour

Not sure.

Improving the docs is one option, but this behaviour seem quite counter intuitive.

My own suggestion:

> Should we disable the adding of new inputs when `reduce_output` is set? If someone really intends to do that, they should probably use `outputs`.

### Steps to reproduce

Haven't tried.

### Relevant log output

_No response_

### How did you obtain Bitcoin Core

Compiled from source

### What version of Bitcoin Core are you using?

Master after #26467

### Operating system and version

Any

### Machine specifications

_No response_


Discussion / hints:
> Should we disable the adding of new inputs when reduce_output is set? If someone really intends to do that, they should probably use outputs.

I don't think so. This is more an implementation issue rather than a missing doc or a missing restriction.
This could also happen without someone intending to do it. For instance, could be a higher bump fee that make the tx require another input.

The issue arises because the code (as is now) mixes two different concepts; it treats the change output and the reduce output (output from where the fees will be taken) as they would be the same, when they are not. It can easily be seen [here](https://github.com/bitcoin/bitcoin/blob/f4f1d6d230dd390f0524b8852b8cfc912d006958/src/wallet/feebumper.cpp#L250C12-L250C12).
Instead of instructing the wallet to deduce the fee from certain output (by using SFFO), we discard the output from the recipients list and make the output script the custom change destination, so the wallet **automatically** adds the change output, at the end of the transaction creation process, going to the discarded output script, with an amount equal to the remainder: `inputs - outputs - new_bump_fee`.

To untangle the two concepts, we need to fix #27601 first. It is the reason why we drop the change output from the recipients list instead of simply mark it as SFFO (which would be the easiest implementation for a "reduce output" mechanism).
What is the status here? This is the only issue tagged 26.0 that doesn't have a pull request associated 
> What is the status here? This is the only issue tagged 26.0 that doesn't have a pull request associated

Status is "waiting for others" in #27601 since July 27th. Happy to receive feedback on my last message there (https://github.com/bitcoin/bitcoin/pull/27601#pullrequestreview-1550813509).

The reason why this happens is explained above https://github.com/bitcoin/bitcoin/issues/28180#issuecomment-1659172967.  if we manage to merge #27601 (in any form), we can fix this issue cleanly.

Basically, the only remaining point to discuss there is whether to have the transaction creation process automatically detecting change outputs when they match the custom change address or not.
I could also code a different version of the PR moving the automatic change output detection outside of the tx creation process too. Placing it on the upper layer (similar to what we do with the duplicate recipients check). But, in any case, concluding the discussion there prior to any changes would be preferable.

Fix patch:
diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp
index 0ee3f27761ec4..1e5e231cefa7b 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 512a011dfc038..f4cb4bbd66c56 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<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;
     }
 
@@ -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 f00bf15730fc6..d3d43861efcb6 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<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
diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp
index c2f4321a60829..6b96fc4e49f13 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<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];
@@ -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<uint32_t>();
+        if (options.exists("original_change_index")) {
+            original_change_index = options["original_change_index"].getInt<uint32_t>();
         }
     }
 
@@ -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:


Test patch:
diff --git a/test/functional/wallet_bumpfee.py b/test/functional/wallet_bumpfee.py
index c66f279e88df8..fea933a93b2b8 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)