fix: Inconsistent checking of RBF rules.

wallet/src/wallet/coin_selection.rs

@@ -47,17 +47,19 @@
 //!     ) -> Result<CoinSelectionResult, coin_selection::InsufficientFunds> {
 //!         let mut selected_amount = Amount::ZERO;
 //!         let mut additional_weight = Weight::ZERO;
+//!         let mut satisfaction_weight = Weight::ZERO;
 //!         let all_utxos_selected = required_utxos
 //!             .into_iter()
 //!             .chain(optional_utxos)
 //!             .scan(
-//!                 (&mut selected_amount, &mut additional_weight),
-//!                 |(selected_amount, additional_weight), weighted_utxo| {
+//!                 (&mut selected_amount, &mut additional_weight, &mut satisfaction_weight),
+//!                 |(selected_amount, additional_weight, satisfaction_weight), weighted_utxo| {
 //!                     **selected_amount += weighted_utxo.utxo.txout().value;
 //!                     **additional_weight += TxIn::default()
 //!                         .segwit_weight()
 //!                         .checked_add(weighted_utxo.satisfaction_weight)
 //!                         .expect("`Weight` addition should not cause an integer overflow");
+//!                     **satisfaction_weight += weighted_utxo.satisfaction_weight;
 //!                     Some(weighted_utxo.utxo)
 //!                 },
 //!             )
@@ -79,6 +81,7 @@
 //!             selected: all_utxos_selected,
 //!             fee_amount: additional_fees,
 //!             excess,
+//!             satisfaction_weight,
 //!         })
 //!     }
 //! }
@@ -174,6 +177,8 @@ pub struct CoinSelectionResult {
     pub fee_amount: Amount,
     /// Remaining amount after deducing fees and outgoing outputs
     pub excess: Excess,
+    /// The total satisfaction weight of the selected inputs
+    pub satisfaction_weight: Weight,
 }
 
 impl CoinSelectionResult {
@@ -325,17 +330,24 @@ fn select_sorted_utxos(
 ) -> Result<CoinSelectionResult, InsufficientFunds> {
     let mut selected_amount = Amount::ZERO;
     let mut fee_amount = Amount::ZERO;
+    let mut satisfaction_weight = Weight::ZERO;
+
     let selected = utxos
         .scan(
-            (&mut selected_amount, &mut fee_amount),
-            |(selected_amount, fee_amount), (must_use, weighted_utxo)| {
+            (
+                &mut selected_amount,
+                &mut fee_amount,
+                &mut satisfaction_weight,
+            ),
+            |(selected_amount, fee_amount, satisfaction_weight), (must_use, weighted_utxo)| {
                 if must_use || **selected_amount < target_amount + **fee_amount {
                     **fee_amount += fee_rate
                         * TxIn::default()
                             .segwit_weight()
                             .checked_add(weighted_utxo.satisfaction_weight)
                             .expect("`Weight` addition should not cause an integer overflow");
                     **selected_amount += weighted_utxo.utxo.txout().value;
+                    **satisfaction_weight += weighted_utxo.satisfaction_weight;
                     Some(weighted_utxo.utxo)
                 } else {
                     None
@@ -360,6 +372,7 @@ fn select_sorted_utxos(
         selected,
         fee_amount,
         excess,
+        satisfaction_weight,
     })
 }
 
@@ -706,6 +719,9 @@ fn calculate_cs_result(
 ) -> CoinSelectionResult {
     selected_utxos.append(&mut required_utxos);
     let fee_amount = selected_utxos.iter().map(|u| u.fee).sum();
+    let satisfaction_weight = selected_utxos.iter().fold(Weight::ZERO, |acc, u| {
+        acc + u.weighted_utxo.satisfaction_weight
+    });
     let selected = selected_utxos
         .into_iter()
         .map(|u| u.weighted_utxo.utxo)
@@ -715,6 +731,7 @@ fn calculate_cs_result(
         selected,
         fee_amount,
         excess,
+        satisfaction_weight,
     }
 }
 

wallet/src/wallet/mod.rs

@@ -1358,10 +1358,11 @@ impl Wallet {
             (Some(sequence), _) => sequence,
         };
 
+        let bumping_fee = params.bumping_fee;
         let (fee_rate, mut fee_amount) = match params.fee_policy.unwrap_or_default() {
-            //FIXME: see https://github.com/bitcoindevkit/bdk/issues/256
+            // Preliminary check to see if the fee or feerate is too low before doing CoinSelection
             FeePolicy::FeeAmount(fee) => {
-                if let Some(previous_fee) = params.bumping_fee {
+                if let Some(previous_fee) = bumping_fee {
                     if fee < previous_fee.absolute {
                         return Err(CreateTxError::FeeTooLow {
                             required: previous_fee.absolute,
@@ -1371,14 +1372,12 @@ impl Wallet {
                 (FeeRate::ZERO, fee)
             }
             FeePolicy::FeeRate(rate) => {
-                if let Some(previous_fee) = params.bumping_fee {
-                    let required_feerate = FeeRate::from_sat_per_kwu(
-                        previous_fee.rate.to_sat_per_kwu()
-                            + FeeRate::BROADCAST_MIN.to_sat_per_kwu(), // +1 sat/vb
-                    );
-                    if rate < required_feerate {
+                if let Some(previous_fee) = bumping_fee {
+                    if rate <= previous_fee.rate {
                         return Err(CreateTxError::FeeRateTooLow {
-                            required: required_feerate,
+                            required: FeeRate::from_sat_per_kwu(
+                                previous_fee.rate.to_sat_per_kwu() + 1,
+                            ),
                         });
                     }
                 }
@@ -1527,6 +1526,38 @@ impl Wallet {
 
         let psbt = self.complete_transaction(tx, coin_selection.selected, params)?;
 
+        // RBF checks.
+        // BIP125 imposes two restrictions on RBF transactions:
+        // 1. The replacement transaction must have a higher absolute fee from the original(s)
+        // 2. The additional fees (current_fee - original_fee) pays for the replacement transaction's
+        //    bandwidth at or above the rate set by the node's incremental relay feerate
+        // Additionally, Bitcoin Core enforces a additional rule:
+        // https://github.com/bitcoin/bitcoin/blob/master/doc/policy/mempool-replacements.md#current-replace-by-fee-policy
+        // 3. The replacement transaction's feerate is greater than the (cumulative) feerates of all
+        //    directly conflicting transactions.
+        if let Some(previous_fee) = bumping_fee {
+            // Only check if we can determine the fee, else assume user knows what they're doing.
+            // TODO: ask someone if it'd be more appropriate to error with UnknownUTXO
+            if let Some(fee) = psbt.fee_amount() {
+                let tx_weight = psbt.unsigned_tx.weight() + coin_selection.satisfaction_weight;
+                let fee_rate = fee / tx_weight;
+
+                // Ensure the tx fee is atleast the previous and also pays for it's broadcast fee
+                if (fee - previous_fee.absolute) < tx_weight * FeeRate::BROADCAST_MIN {
+                    return Err(CreateTxError::FeeTooLow {
+                        required: tx_weight * FeeRate::BROADCAST_MIN + previous_fee.absolute,
+                    });
+                }
+
+                // Ensure the fee rate is strictly higher
+                if fee_rate <= previous_fee.rate {
+                    return Err(CreateTxError::FeeRateTooLow {
+                        required: FeeRate::from_sat_per_kwu(previous_fee.rate.to_sat_per_kwu() + 1),
+                    });
+                }
+            }
+        }
+
         // recording changes to the change keychain
         if let (Excess::Change { .. }, Some((keychain, index))) = (excess, drain_index) {
             let (_, index_changeset) = self

wallet/tests/wallet.rs

@@ -1880,8 +1880,7 @@ fn test_bump_fee_low_fee_rate() {
         "expected FeeRateTooLow error"
     );
 
-    let required = feerate.to_sat_per_kwu() + 250; // +1 sat/vb
-    let sat_vb = required as f64 / 250.0;
+    let sat_vb = (feerate.to_sat_per_kwu() as f64 + 1.0) / 250.0;
     let expect = format!("Fee rate too low: required {} sat/vb", sat_vb);
     assert_eq!(res.unwrap_err().to_string(), expect);
 }
@@ -1976,7 +1975,7 @@ fn test_bump_fee_reduce_change() {
     assert_fee_rate!(psbt, fee.unwrap_or(Amount::ZERO), feerate, @add_signature);
 
     let mut builder = wallet.build_fee_bump(txid).unwrap();
-    builder.fee_absolute(Amount::from_sat(200));
+    builder.fee_absolute(Amount::from_sat(283));
     let psbt = builder.finish().unwrap();
     let sent_received =
         wallet.sent_and_received(&psbt.clone().extract_tx().expect("failed to extract tx"));
@@ -2013,7 +2012,7 @@ fn test_bump_fee_reduce_change() {
         sent_received.1
     );
 
-    assert_eq!(fee.unwrap_or(Amount::ZERO), Amount::from_sat(200));
+    assert_eq!(fee.unwrap_or(Amount::ZERO), Amount::from_sat(283));
 }
 
 #[test]
@@ -2566,7 +2565,7 @@ fn test_bump_fee_absolute_force_add_input() {
     builder
         .add_utxo(incoming_op)
         .unwrap()
-        .fee_absolute(Amount::from_sat(250));
+        .fee_absolute(Amount::from_sat(351));
     let psbt = builder.finish().unwrap();
     let sent_received =
         wallet.sent_and_received(&psbt.clone().extract_tx().expect("failed to extract tx"));
@@ -2601,7 +2600,7 @@ fn test_bump_fee_absolute_force_add_input() {
         sent_received.1
     );
 
-    assert_eq!(fee.unwrap_or(Amount::ZERO), Amount::from_sat(250));
+    assert_eq!(fee.unwrap_or(Amount::ZERO), Amount::from_sat(351));
 }
 
 #[test]
@@ -2670,6 +2669,90 @@ fn test_bump_fee_unconfirmed_input() {
     builder.finish().unwrap();
 }
 
+#[test]
+fn test_bump_fee_bip125_minimal_feerate() {
+    // BIP125 requires that the replacement transaction must overpay by it's relay fee, and a cursory
+    // analysis might indicate that just means that the replacement transaction must have
+    // feerate >= previous_feerate + BROADCAST_MIN (1 sat/vB).
+    // However this is not the case, as the replacement transaction can be larger than the original, and
+    // therefore can pay the extra broadcast fee with the same feerate, as long as the original feerate
+    // was high enough. In addition Bitcoin Core requires that the feerate be strictly higher.
+    // This test checks the edgecase where the feerate is just slightly higher, but it's valid RBF.
+
+    let (mut wallet, _) = get_funded_wallet_wpkh();
+
+    // Extra UTXO
+    let init_tx = Transaction {
+        version: transaction::Version::ONE,
+        lock_time: absolute::LockTime::ZERO,
+        input: vec![],
+        output: vec![TxOut {
+            script_pubkey: wallet
+                .next_unused_address(KeychainKind::External)
+                .script_pubkey(),
+            value: Amount::from_sat(25_000),
+        }],
+    };
+    let txid: Txid = init_tx.compute_txid();
+    let pos: ChainPosition<ConfirmationBlockTime> =
+        wallet.transactions().last().unwrap().chain_position;
+    insert_tx(&mut wallet, init_tx);
+    match pos {
+        ChainPosition::Confirmed { anchor, .. } => insert_anchor(&mut wallet, txid, anchor),
+        other => panic!("all wallet txs must be confirmed: {:?}", other),
+    }
+
+    let addr = Address::from_str("2N1Ffz3WaNzbeLFBb51xyFMHYSEUXcbiSoX")
+        .unwrap()
+        .assume_checked();
+
+    // Create original transaction, RBF is on by default, fee_rate is set to 40 sat/vB
+    let tx1 = {
+        let mut builder = wallet.build_tx().coin_selection(LargestFirstCoinSelection);
+        builder
+            .add_recipient(addr.script_pubkey(), Amount::from_sat(30_000))
+            .fee_rate(FeeRate::from_sat_per_kwu(592));
+        let mut psbt = builder.finish().unwrap();
+        wallet
+            .sign(&mut psbt, Default::default())
+            .expect("failed to sign");
+        psbt.extract_tx().expect("failed to extract tx")
+    };
+    let txid = tx1.compute_txid();
+    let fee_rate = wallet.calculate_fee_rate(&tx1).unwrap();
+    insert_tx(&mut wallet, tx1.clone());
+
+    // Create replacement transaction with a slightly higher feerate, and bigger size (extra input and output)
+    let mut builder = wallet.build_fee_bump(txid).unwrap();
+    builder
+        .add_recipient(
+            Address::from_str("2N4eQYCbKUHCCTUjBJeHcJp9ok6J2GZsTDt")
+                .unwrap()
+                .assume_checked(),
+            Amount::from_sat(25_000),
+        )
+        .fee_rate(FeeRate::from_sat_per_kwu(fee_rate.to_sat_per_kwu() + 1));
+    let mut psbt = builder.finish().expect(
+        "This should create a valid transaction, even though the feerate is only slightly higher",
+    );
+    wallet.sign(&mut psbt, Default::default()).unwrap();
+    let tx2 = psbt.extract_tx().expect("failed to extract tx");
+
+    assert!(
+        wallet.calculate_fee(&tx2).unwrap() >= wallet.calculate_fee(&tx1).unwrap(),
+        "Replacement transaction should have a higher absolute fee"
+    );
+    assert!(
+        wallet.calculate_fee(&tx2).unwrap() - wallet.calculate_fee(&tx1).unwrap()
+            > tx2.weight() * FeeRate::BROADCAST_MIN,
+        "Replacement transaction should pay for the additional bandwidth"
+    );
+    assert!(
+        wallet.calculate_fee_rate(&tx2).unwrap() > wallet.calculate_fee_rate(&tx1).unwrap(),
+        "Replacement transaction should have a higher feerate"
+    );
+}
+
 #[test]
 fn test_fee_amount_negative_drain_val() {
     // While building the transaction, bdk would calculate the drain_value