diff --git a/wallet/src/wallet/coin_selection.rs b/wallet/src/wallet/coin_selection.rs index e5654fb4..81dbb7ca 100644 --- a/wallet/src/wallet/coin_selection.rs +++ b/wallet/src/wallet/coin_selection.rs @@ -47,17 +47,19 @@ //! ) -> Result { //! 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,10 +330,16 @@ fn select_sorted_utxos( ) -> Result { 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() @@ -336,6 +347,7 @@ fn select_sorted_utxos( .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, } } diff --git a/wallet/src/wallet/mod.rs b/wallet/src/wallet/mod.rs index e4dc6d05..1be67130 100644 --- a/wallet/src/wallet/mod.rs +++ b/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 diff --git a/wallet/tests/wallet.rs b/wallet/tests/wallet.rs index 35cbd85d..3f3a0d97 100644 --- a/wallet/tests/wallet.rs +++ b/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 = + 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