Merge bitcoin/bitcoin#27308: bumpfee: avoid making bumped transactions with too low fee when replacing outputs

d52fa1b0a5 tests: Make sure that bumpfee feerate checks work when replacing outputs (Andrew Chow)
be177c15a4 bumpfee: Check the correct feerate when replacing outputs (Andrew Chow)

Pull request description:

  When replacing the outputs of a transaction during `bumpfee`, it is possible to accidentally create a transaction that will not be accepted into the mempool as it does not meet the incremental relay fee requirements. This occurs because the size estimation used for checking the provided feerate does not account for the replaced outputs; it instead uses the original outputs. When the replaced outputs is significantly different from the original, there can be a large difference in estimated transaction sizes that can make a transaction miss the absolute fee requirements for the incremental relay fee. Unfortunately we do not currently inform the user when the bumped transaction fails to relay, so they could use `bumpfee` and think the transaction has been bumped when it actually has not.

  This issue is resolved by replacing the outputs before doing the size estimation, and also updating the feerate checker to use the actual fee values when calculating the required minimum fee.

  Also added a test for this scenario.

ACKs for top commit:
  ishaanam:
    reACK d52fa1b0a5
  Xekyo:
    reACK d52fa1b0a5

Tree-SHA512: d18301b587465322dd3fb1bb86496c3675265a56072047576e2baa5cf907dd3b54778f30721f662f0c235709a5568427c18542eb7efbfb6fdd9f481fe676c66b

src/wallet/feebumper.cpp

@@ -63,7 +63,7 @@ static feebumper::Result PreconditionChecks(const CWallet& wallet, const CWallet
 }
 
 //! Check if the user provided a valid feeRate
-static feebumper::Result CheckFeeRate(const CWallet& wallet, const CWalletTx& wtx, const CFeeRate& newFeerate, const int64_t maxTxSize, CAmount old_fee, std::vector<bilingual_str>& errors)
+static feebumper::Result CheckFeeRate(const CWallet& wallet, const CFeeRate& newFeerate, const int64_t maxTxSize, CAmount old_fee, std::vector<bilingual_str>& errors)
 {
     // check that fee rate is higher than mempool's minimum fee
     // (no point in bumping fee if we know that the new tx won't be accepted to the mempool)
@@ -84,15 +84,12 @@ static feebumper::Result CheckFeeRate(const CWallet& wallet, const CWalletTx& wt
 
     CFeeRate incrementalRelayFee = std::max(wallet.chain().relayIncrementalFee(), CFeeRate(WALLET_INCREMENTAL_RELAY_FEE));
 
-    // Given old total fee and transaction size, calculate the old feeRate
-    const int64_t txSize = GetVirtualTransactionSize(*(wtx.tx));
-    CFeeRate nOldFeeRate(old_fee, txSize);
     // Min total fee is old fee + relay fee
-    CAmount minTotalFee = nOldFeeRate.GetFee(maxTxSize) + incrementalRelayFee.GetFee(maxTxSize);
+    CAmount minTotalFee = old_fee + incrementalRelayFee.GetFee(maxTxSize);
 
     if (new_total_fee < minTotalFee) {
         errors.push_back(strprintf(Untranslated("Insufficient total fee %s, must be at least %s (oldFee %s + incrementalFee %s)"),
-            FormatMoney(new_total_fee), FormatMoney(minTotalFee), FormatMoney(nOldFeeRate.GetFee(maxTxSize)), FormatMoney(incrementalRelayFee.GetFee(maxTxSize))));
+            FormatMoney(new_total_fee), FormatMoney(minTotalFee), FormatMoney(old_fee), FormatMoney(incrementalRelayFee.GetFee(maxTxSize))));
         return feebumper::Result::INVALID_PARAMETER;
     }
 
@@ -234,7 +231,8 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
     // is one). If outputs vector is non-empty, replace original
     // outputs with its contents, otherwise use original outputs.
     std::vector<CRecipient> recipients;
-    for (const auto& output : outputs.empty() ? wtx.tx->vout : outputs) {
+    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);
@@ -249,13 +247,14 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
         // The user provided a feeRate argument.
         // We calculate this here to avoid compiler warning on the cs_wallet lock
         // We need to make a temporary transaction with no input witnesses as the dummy signer expects them to be empty for external inputs
-        CMutableTransaction mtx{*wtx.tx};
+        CMutableTransaction temp_mtx{*wtx.tx};
-        for (auto& txin : mtx.vin) {
+        for (auto& txin : temp_mtx.vin) {
             txin.scriptSig.clear();
             txin.scriptWitness.SetNull();
         }
-        const int64_t maxTxSize{CalculateMaximumSignedTxSize(CTransaction(mtx), &wallet, &new_coin_control).vsize};
+        temp_mtx.vout = txouts;
-        Result res = CheckFeeRate(wallet, wtx, *new_coin_control.m_feerate, maxTxSize, old_fee, errors);
+        const int64_t maxTxSize{CalculateMaximumSignedTxSize(CTransaction(temp_mtx), &wallet, &new_coin_control).vsize};
+        Result res = CheckFeeRate(wallet, *new_coin_control.m_feerate, maxTxSize, old_fee, errors);
         if (res != Result::OK) {
             return res;
         }

test/functional/wallet_bumpfee.py

@@ -26,6 +26,7 @@ from test_framework.util import (
     assert_equal,
     assert_greater_than,
     assert_raises_rpc_error,
+    get_fee,
 )
 from test_framework.wallet import MiniWallet
 
@@ -100,6 +101,7 @@ class BumpFeeTest(BitcoinTestFramework):
         test_change_script_match(self, rbf_node, dest_address)
         test_settxfee(self, rbf_node, dest_address)
         test_maxtxfee_fails(self, rbf_node, dest_address)
+        test_feerate_checks_replaced_outputs(self, rbf_node)
         # These tests wipe out a number of utxos that are expected in other tests
         test_small_output_with_feerate_succeeds(self, rbf_node, dest_address)
         test_no_more_inputs_fails(self, rbf_node, dest_address)
@@ -668,5 +670,30 @@ def test_no_more_inputs_fails(self, rbf_node, dest_address):
     self.clear_mempool()
 
 
+def test_feerate_checks_replaced_outputs(self, rbf_node):
+    self.log.info("Test that feerate checks use replaced outputs")
+    outputs = []
+    for i in range(50):
+        outputs.append({rbf_node.getnewaddress(address_type="bech32"): 1})
+    tx_res = rbf_node.send(outputs=outputs, fee_rate=5)
+    tx_details = rbf_node.gettransaction(txid=tx_res["txid"], verbose=True)
+
+    # Calculate the minimum feerate required for the bump to work.
+    # Since the bumped tx will replace all of the outputs with a single output, we can estimate that its size will 31 * (len(outputs) - 1) bytes smaller
+    tx_size = tx_details["decoded"]["vsize"]
+    est_bumped_size = tx_size - (len(tx_details["decoded"]["vout"]) - 1) * 31
+    inc_fee_rate = max(rbf_node.getmempoolinfo()["incrementalrelayfee"], Decimal(0.00005000)) # Wallet has a fixed incremental relay fee of 5 sat/vb
+    # RPC gives us fee as negative
+    min_fee = (-tx_details["fee"] + get_fee(est_bumped_size, inc_fee_rate)) * Decimal(1e8)
+    min_fee_rate = (min_fee / est_bumped_size).quantize(Decimal("1.000"))
+
+    # Attempt to bumpfee and replace all outputs with a single one using a feerate slightly less than the minimum
+    new_outputs = [{rbf_node.getnewaddress(address_type="bech32"): 49}]
+    assert_raises_rpc_error(-8, "Insufficient total fee", rbf_node.bumpfee, tx_res["txid"], {"fee_rate": min_fee_rate - 1, "outputs": new_outputs})
+
+    # Bumpfee and replace all outputs with a single one using the minimum feerate
+    rbf_node.bumpfee(tx_res["txid"], {"fee_rate": min_fee_rate, "outputs": new_outputs})
+
+
 if __name__ == "__main__":
     BumpFeeTest().main()