Merge bitcoin/bitcoin#26467: bumpfee: Allow the user to choose which output is change

e8c31f1 tests: Test for bumping single output transaction (Andrew Chow)
4f4d440 test: Test bumpfee reduce_output (Andrew Chow)
7d83502 bumpfee: Allow original change position to be specified (Andrew Chow)

Pull request description:

  When bumping the transaction fee, we will try to find the change output of the transaction in order to have an output whose value we can modify so that we can meet the target feerate. However we do not always find the change output which can cause us to unnecessarily add an additional output to the transaction. We can avoid this issue by outsourcing the determination of change to the user if they so desire.

  This PR adds a `orig_change_pos` option to bumpfee which the user can use to specify the index of the change output.

  Fixes #11233
  Fixes #20795

ACKs for top commit:
  ismaelsadeeq:
    re ACK e8c31f1
  pinheadmz:
    re-ACK e8c31f1
  furszy:
    Code review ACK e8c31f1

Tree-SHA512: 3a230655934af17f7c1a5953fafb5ef0d687c21355cf284d5e98fece411f589cd69ea505f06d6bdcf82836b08d268c366ad2dd30ae3d71541c9cdf94d1f698ee

Author: fanquake

src/rpc/client.cpp

@@ -259,11 +259,13 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "bumpfee", 1, "fee_rate"},
     { "bumpfee", 1, "replaceable"},
     { "bumpfee", 1, "outputs"},
+    { "bumpfee", 1, "reduce_output"},
     { "psbtbumpfee", 1, "options" },
     { "psbtbumpfee", 1, "conf_target"},
     { "psbtbumpfee", 1, "fee_rate"},
     { "psbtbumpfee", 1, "replaceable"},
     { "psbtbumpfee", 1, "outputs"},
+    { "psbtbumpfee", 1, "reduce_output"},
     { "logging", 0, "include" },
     { "logging", 1, "exclude" },
     { "disconnectnode", 1, "nodeid" },

src/wallet/feebumper.cpp

@@ -152,8 +152,14 @@ 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)
+                                 CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx, bool require_mine, const std::vector<CTxOut>& outputs, std::optional<uint32_t> reduce_output)
 {
+    // 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"));
+        return Result::INVALID_PARAMETER;
+    }
+
     // We are going to modify coin control later, copy to re-use
     CCoinControl new_coin_control(coin_control);
 
@@ -166,6 +172,12 @@ 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()) {
+        errors.push_back(Untranslated("Change position is out of range"));
+        return Result::INVALID_PARAMETER;
+    }
+
     // Retrieve all of the UTXOs and add them to coin control
     // While we're here, calculate the input amount
     std::map<COutPoint, Coin> coins;
@@ -233,14 +245,15 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
     std::vector<CRecipient> recipients;
     CAmount new_outputs_value = 0;
     const auto& txouts = outputs.empty() ? wtx.tx->vout : outputs;
-    for (const auto& output : txouts) {
-        if (!OutputIsChange(wallet, output)) {
-            CRecipient recipient = {output.scriptPubKey, output.nValue, false};
-            recipients.push_back(recipient);
-        } else {
+    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)) {
             CTxDestination change_dest;
             ExtractDestination(output.scriptPubKey, change_dest);
             new_coin_control.destChange = change_dest;
+        } else {
+            CRecipient recipient = {output.scriptPubKey, output.nValue, false};
+            recipients.push_back(recipient);
         }
         new_outputs_value += output.nValue;
     }

src/wallet/feebumper.h

@@ -43,6 +43,8 @@ bool TransactionCanBeBumped(const CWallet& wallet, const uint256& txid);
  * @param[out] new_fee the fee that the bump transaction pays
  * @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
  */
 Result CreateRateBumpTransaction(CWallet& wallet,
     const uint256& txid,
@@ -52,7 +54,8 @@ Result CreateRateBumpTransaction(CWallet& wallet,
     CAmount& new_fee,
     CMutableTransaction& mtx,
     bool require_mine,
-    const std::vector<CTxOut>& outputs);
+    const std::vector<CTxOut>& outputs,
+    std::optional<uint32_t> reduce_output = std::nullopt);
 
 //! Sign the new transaction,
 //! @return false if the tx couldn't be found or if it was

src/wallet/rpc/spend.cpp

@@ -1015,9 +1015,11 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
                              "\"" + FeeModes("\"\n\"") + "\""},
                     {"outputs", RPCArg::Type::ARR, RPCArg::Default{UniValue::VARR}, "New outputs (key-value pairs) which will replace\n"
                              "the original ones, if provided. Each address can only appear once and there can\n"
-                             "only be one \"data\" object.\n",
+                             "only be one \"data\" object.\n"
+                             "Cannot be provided if 'reduce_output' 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."},
                 },
                 RPCArgOptions{.oneline_description="options"}},
         },
@@ -1056,6 +1058,8 @@ 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;
+
     if (!request.params[1].isNull()) {
         UniValue options = request.params[1];
         RPCTypeCheckObj(options,
@@ -1066,6 +1070,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)},
             },
             true, true);
 
@@ -1089,6 +1094,10 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
             AddOutputs(tempTx, options["outputs"]);
             outputs = tempTx.vout;
         }
+
+        if (options.exists("reduce_output")) {
+            reduce_output = options["reduce_output"].getInt<uint32_t>();
+        }
     }
 
     // Make sure the results are valid at least up to the most recent block
@@ -1106,7 +1115,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);
+    res = feebumper::CreateRateBumpTransaction(*pwallet, hash, coin_control, errors, old_fee, new_fee, mtx, /*require_mine=*/ !want_psbt, outputs, reduce_output);
     if (res != feebumper::Result::OK) {
         switch(res) {
             case feebumper::Result::INVALID_ADDRESS_OR_KEY:

test/functional/wallet_bumpfee.py

@@ -24,9 +24,11 @@
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
+    assert_fee_amount,
     assert_greater_than,
     assert_raises_rpc_error,
     get_fee,
+    find_vout_for_address,
 )
 from test_framework.wallet import MiniWallet
 
@@ -109,6 +111,8 @@ def run_test(self):
         test_small_output_with_feerate_succeeds(self, rbf_node, dest_address)
         test_no_more_inputs_fails(self, rbf_node, dest_address)
         self.test_bump_back_to_yourself()
+        self.test_provided_change_pos(rbf_node)
+        self.test_single_output()
 
         # Context independent tests
         test_feerate_checks_replaced_outputs(self, rbf_node, peer_node)
@@ -174,6 +178,13 @@ 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 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.clear_mempool()
 
     def test_bump_back_to_yourself(self):
@@ -225,6 +236,72 @@ 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")
+
+        change_addr = rbf_node.getnewaddress()
+        dest_addr = rbf_node.getnewaddress()
+        assert_equal(rbf_node.getaddressinfo(change_addr)["ischange"], False)
+        assert_equal(rbf_node.getaddressinfo(dest_addr)["ischange"], False)
+
+        send_res = rbf_node.send(outputs=[{dest_addr: 1}], options={"change_address": change_addr})
+        assert send_res["complete"]
+        txid = send_res["txid"]
+
+        tx = rbf_node.gettransaction(txid=txid, verbose=True)
+        assert_equal(len(tx["decoded"]["vout"]), 2)
+
+        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})
+        new_txid = bumped["txid"]
+
+        new_tx = rbf_node.gettransaction(txid=new_txid, verbose=True)
+        assert_equal(len(new_tx["decoded"]["vout"]), 2)
+        new_change_pos = find_vout_for_address(rbf_node, new_txid, change_addr)
+        new_change_value = new_tx["decoded"]["vout"][new_change_pos]["value"]
+
+        assert_greater_than(change_value, new_change_value)
+
+
+    def test_single_output(self):
+        self.log.info("Test that single output txs can be bumped")
+        node = self.nodes[1]
+
+        node.createwallet("single_out_rbf")
+        wallet = node.get_wallet_rpc("single_out_rbf")
+
+        addr = wallet.getnewaddress()
+        amount = Decimal("0.001")
+        # Make 2 UTXOs
+        self.nodes[0].sendtoaddress(addr, amount)
+        self.nodes[0].sendtoaddress(addr, amount)
+        self.generate(self.nodes[0], 1)
+        utxos = wallet.listunspent()
+
+        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})
+
+        # Reduce the only output successfully
+        bumped = wallet.bumpfee(txid=tx["txid"], options={"fee_rate": 10, "reduce_output": 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
+        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)
+        assert_equal(len(bumped_tx["decoded"]["vin"]), 2)
+        assert_fee_amount(bumped["fee"], bumped_tx["decoded"]["vsize"], Decimal(20) / Decimal(1e8) * 1000)
+
+        wallet.unloadwallet()
+
 def test_simple_bumpfee_succeeds(self, mode, rbf_node, peer_node, dest_address):
     self.log.info('Test simple bumpfee: {}'.format(mode))
     rbfid = spend_one_input(rbf_node, dest_address)