bumpfee: Allow original change position to be specified

If the user used a custom change address, it may not be detected as a
change output, resulting in an additional change output being added to
the bumped transaction. We can avoid this issue by allowing the user to
specify the position of the change output.

Author: achow101

src/wallet/feebumper.cpp

@@ -155,7 +155,7 @@ 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)
+                                 CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx, bool require_mine, std::optional<uint32_t> reduce_output)
 {
     // We are going to modify coin control later, copy to re-use
     CCoinControl new_coin_control(coin_control);
@@ -169,6 +169,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;
@@ -226,14 +232,15 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
     // While we're here, calculate the output amount
     std::vector<CRecipient> recipients;
     CAmount output_value = 0;
-    for (const auto& output : wtx.tx->vout) {
-        if (!OutputIsChange(wallet, output)) {
-            CRecipient recipient = {output.scriptPubKey, output.nValue, false};
-            recipients.push_back(recipient);
-        } else {
+    for (size_t i = 0; i < wtx.tx->vout.size(); ++i) {
+        const CTxOut& output = wtx.tx->vout.at(i);
+        if ((reduce_output.has_value() && reduce_output.value() == i) || (!reduce_output.has_value() && 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);
         }
         output_value += output.nValue;
     }

src/wallet/feebumper.h

@@ -43,6 +43,7 @@ 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] 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,
@@ -51,7 +52,8 @@ Result CreateRateBumpTransaction(CWallet& wallet,
     CAmount& old_fee,
     CAmount& new_fee,
     CMutableTransaction& mtx,
-    bool require_mine);
+    bool require_mine,
+    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

@@ -967,6 +967,7 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
                              "are replaceable).\n"},
                     {"estimate_mode", RPCArg::Type::STR, RPCArg::Default{"unset"}, "The fee estimate mode, must be one of (case insensitive):\n"
                      "\"" + FeeModes("\"\n\"") + "\""},
+                    {"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."},
                 },
                 RPCArgOptions{.oneline_description="options"}},
         },
@@ -1005,6 +1006,8 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
     // optional parameters
     coin_control.m_signal_bip125_rbf = true;
 
+    std::optional<uint32_t> reduce_output;
+
     if (!request.params[1].isNull()) {
         UniValue options = request.params[1];
         RPCTypeCheckObj(options,
@@ -1014,6 +1017,7 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
                 {"fee_rate", UniValueType()}, // will be checked by AmountFromValue() in SetFeeEstimateMode()
                 {"replaceable", UniValueType(UniValue::VBOOL)},
                 {"estimate_mode", UniValueType(UniValue::VSTR)},
+                {"reduce_output", UniValueType(UniValue::VNUM)},
             },
             true, true);
 
@@ -1027,6 +1031,10 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
             coin_control.m_signal_bip125_rbf = options["replaceable"].get_bool();
         }
         SetFeeEstimateMode(*pwallet, coin_control, conf_target, options["estimate_mode"], options["fee_rate"], /*override_min_fee=*/false);
+
+        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
@@ -1044,7 +1052,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);
+    res = feebumper::CreateRateBumpTransaction(*pwallet, hash, coin_control, errors, old_fee, new_fee, mtx, /*require_mine=*/ !want_psbt, reduce_output);
     if (res != feebumper::Result::OK) {
         switch(res) {
             case feebumper::Result::INVALID_ADDRESS_OR_KEY: