Merge #1616: feat(wallet)!: enable RBF by default on TxBuilder

f15551b feat(wallet)!: enable RBF by default on TxBuilder (Luis Schwab)

Pull request description:

  ### Description

  Closes #791

  This PR enables RBF by default on `TxBuilder`

  ### Notes to the reviewers

  <!-- In this section you can include notes directed to the reviewers, like explaining why some parts
  of the PR were done in a specific way -->

  ### Changelog notice
  - On TxParams, `rbf` becomes `sequence`
  - Added `set_exact_sequence()`, so the user can define an arbitrary sequence value
  - `n_sequence` now defaults to `0xFFFFFFFD`
  - Adjusted tests accordingly

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

  #### New Features:

  * [x] I've added tests for the new feature
  * [ ] I've added docs for the new feature

  #### Bugfixes:

  * [x] This pull request breaks the existing API
  * [ ] I've added tests to reproduce the issue which are now passing
  * [x] I'm linking the issue being fixed by this PR

ACKs for top commit:
  notmandatory:
    ACK f15551b

Tree-SHA512: 784538ffd102f315a7a96bd7f9afcdca7d35252ee2b4489d9853797c7f6d8cf95537f0dea3855ea9fffc9bf148c81a8c090cfdfd1e94144b9343651129ab9504

crates/wallet/README.md

@@ -174,7 +174,6 @@ println!("Your new receive address is: {}", receive_address.address);
 <!--         let mut builder = wallet.build_tx(); -->
 <!--         builder -->
 <!--             .add_recipient(send_to.script_pubkey(), 50_000) -->
-<!--             .enable_rbf() -->
 <!--             .do_not_spend_change() -->
 <!--             .fee_rate(FeeRate::from_sat_per_vb(5.0)); -->
 <!--         builder.finish()? -->

crates/wallet/src/wallet/error.rs

@@ -65,12 +65,10 @@ pub enum CreateTxError {
         /// Required `LockTime`
         required: absolute::LockTime,
     },
-    /// Cannot enable RBF with a `Sequence` >= 0xFFFFFFFE
-    RbfSequence,
     /// Cannot enable RBF with `Sequence` given a required OP_CSV
     RbfSequenceCsv {
         /// Given RBF `Sequence`
-        rbf: Sequence,
+        sequence: Sequence,
         /// Required OP_CSV `Sequence`
         csv: Sequence,
     },
@@ -131,14 +129,11 @@ impl fmt::Display for CreateTxError {
             } => {
                 write!(f, "TxBuilder requested timelock of `{:?}`, but at least `{:?}` is required to spend from this script", required, requested)
             }
-            CreateTxError::RbfSequence => {
-                write!(f, "Cannot enable RBF with a nSequence >= 0xFFFFFFFE")
-            }
-            CreateTxError::RbfSequenceCsv { rbf, csv } => {
+            CreateTxError::RbfSequenceCsv { sequence, csv } => {
                 write!(
                     f,
                     "Cannot enable RBF with nSequence `{:?}` given a required OP_CSV of `{:?}`",
-                    rbf, csv
+                    sequence, csv
                 )
             }
             CreateTxError::FeeTooLow { required } => {

crates/wallet/src/wallet/mod.rs

@@ -1366,36 +1366,20 @@ impl Wallet {
             }
         };
 
-        // The nSequence to be by default for inputs unless an explicit sequence is specified.
-        let n_sequence = match (params.rbf, requirements.csv) {
-            // No RBF or CSV but there's an nLockTime, so the nSequence cannot be final
-            (None, None) if lock_time != absolute::LockTime::ZERO => {
-                Sequence::ENABLE_LOCKTIME_NO_RBF
-            }
-            // No RBF, CSV or nLockTime, make the transaction final
-            (None, None) => Sequence::MAX,
-
-            // No RBF requested, use the value from CSV. Note that this value is by definition
-            // non-final, so even if a timelock is enabled this nSequence is fine, hence why we
-            // don't bother checking for it here. The same is true for all the other branches below
+        // nSequence value for inputs
+        // When not explicitly specified, defaults to 0xFFFFFFFD,
+        // meaning RBF signaling is enabled
+        let n_sequence = match (params.sequence, requirements.csv) {
+            // Enable RBF by default
+            (None, None) => Sequence::ENABLE_RBF_NO_LOCKTIME,
+            // None requested, use required
             (None, Some(csv)) => csv,
-
-            // RBF with a specific value but that value is too high
-            (Some(tx_builder::RbfValue::Value(rbf)), _) if !rbf.is_rbf() => {
-                return Err(CreateTxError::RbfSequence)
+            // Requested sequence is incompatible with requirements
+            (Some(sequence), Some(csv)) if !check_nsequence_rbf(sequence, csv) => {
+                return Err(CreateTxError::RbfSequenceCsv { sequence, csv })
             }
-            // RBF with a specific value requested, but the value is incompatible with CSV
-            (Some(tx_builder::RbfValue::Value(rbf)), Some(csv))
-                if !check_nsequence_rbf(rbf, csv) =>
-            {
-                return Err(CreateTxError::RbfSequenceCsv { rbf, csv })
-            }
-
-            // RBF enabled with the default value with CSV also enabled. CSV takes precedence
-            (Some(tx_builder::RbfValue::Default), Some(csv)) => csv,
-            // Valid RBF, either default or with a specific value. We ignore the `CSV` value
-            // because we've already checked it before
-            (Some(rbf), _) => rbf.get_value(),
+            // Use requested nSequence value
+            (Some(sequence), _) => sequence,
         };
 
         let (fee_rate, mut fee_amount) = match params.fee_policy.unwrap_or_default() {
@@ -1609,8 +1593,7 @@ impl Wallet {
     /// let mut psbt = {
     ///     let mut builder = wallet.build_tx();
     ///     builder
-    ///         .add_recipient(to_address.script_pubkey(), Amount::from_sat(50_000))
-    ///         .enable_rbf();
+    ///         .add_recipient(to_address.script_pubkey(), Amount::from_sat(50_000));
     ///     builder.finish()?
     /// };
     /// let _ = wallet.sign(&mut psbt, SignOptions::default())?;

crates/wallet/src/wallet/tx_builder.rs

@@ -31,9 +31,7 @@
 //!     // With a custom fee rate of 5.0 satoshi/vbyte
 //!     .fee_rate(FeeRate::from_sat_per_vb(5).expect("valid feerate"))
 //!     // Only spend non-change outputs
-//!     .do_not_spend_change()
-//!     // Turn on RBF signaling
-//!     .enable_rbf();
+//!     .do_not_spend_change();
 //! let psbt = tx_builder.finish()?;
 //! # Ok::<(), anyhow::Error>(())
 //! ```
@@ -134,7 +132,7 @@ pub(crate) struct TxParams {
     pub(crate) sighash: Option<psbt::PsbtSighashType>,
     pub(crate) ordering: TxOrdering,
     pub(crate) locktime: Option<absolute::LockTime>,
-    pub(crate) rbf: Option<RbfValue>,
+    pub(crate) sequence: Option<Sequence>,
     pub(crate) version: Option<Version>,
     pub(crate) change_policy: ChangeSpendPolicy,
     pub(crate) only_witness_utxo: bool,
@@ -554,23 +552,12 @@ impl<'a, Cs> TxBuilder<'a, Cs> {
         }
     }
 
-    /// Enable signaling RBF
+    /// Set an exact nSequence value
     ///
-    /// This will use the default nSequence value of `0xFFFFFFFD`.
-    pub fn enable_rbf(&mut self) -> &mut Self {
-        self.params.rbf = Some(RbfValue::Default);
-        self
-    }
-
-    /// Enable signaling RBF with a specific nSequence value
-    ///
-    /// This can cause conflicts if the wallet's descriptors contain an "older" (OP_CSV) operator
-    /// and the given `nsequence` is lower than the CSV value.
-    ///
-    /// If the `nsequence` is higher than `0xFFFFFFFD` an error will be thrown, since it would not
-    /// be a valid nSequence to signal RBF.
-    pub fn enable_rbf_with_sequence(&mut self, nsequence: Sequence) -> &mut Self {
-        self.params.rbf = Some(RbfValue::Value(nsequence));
+    /// This can cause conflicts if the wallet's descriptors contain an
+    /// "older" (OP_CSV) operator and the given `nsequence` is lower than the CSV value.
+    pub fn set_exact_sequence(&mut self, n_sequence: Sequence) -> &mut Self {
+        self.params.sequence = Some(n_sequence);
         self
     }
 
@@ -654,8 +641,7 @@ impl<'a, Cs> TxBuilder<'a, Cs> {
     ///     .drain_wallet()
     ///     // Send the excess (which is all the coins minus the fee) to this address.
     ///     .drain_to(to_address.script_pubkey())
-    ///     .fee_rate(FeeRate::from_sat_per_vb(5).expect("valid feerate"))
-    ///     .enable_rbf();
+    ///     .fee_rate(FeeRate::from_sat_per_vb(5).expect("valid feerate"));
     /// let psbt = tx_builder.finish()?;
     /// # Ok::<(), anyhow::Error>(())
     /// ```
@@ -835,24 +821,6 @@ impl Default for Version {
     }
 }
 
-/// RBF nSequence value
-///
-/// Has a default value of `0xFFFFFFFD`
-#[derive(Debug, Ord, PartialOrd, Eq, PartialEq, Hash, Clone, Copy)]
-pub(crate) enum RbfValue {
-    Default,
-    Value(Sequence),
-}
-
-impl RbfValue {
-    pub(crate) fn get_value(&self) -> Sequence {
-        match self {
-            RbfValue::Default => Sequence::ENABLE_RBF_NO_LOCKTIME,
-            RbfValue::Value(v) => *v,
-        }
-    }
-}
-
 /// Policy regarding the use of change outputs when creating a transaction
 #[derive(Default, Debug, Ord, PartialOrd, Eq, PartialEq, Hash, Clone, Copy)]
 pub enum ChangeSpendPolicy {

crates/wallet/src/wallet/utils.rs

@@ -52,20 +52,20 @@ impl After {
     }
 }
 
-pub(crate) fn check_nsequence_rbf(rbf: Sequence, csv: Sequence) -> bool {
-    // The RBF value must enable relative timelocks
-    if !rbf.is_relative_lock_time() {
+pub(crate) fn check_nsequence_rbf(sequence: Sequence, csv: Sequence) -> bool {
+    // The nSequence value must enable relative timelocks
+    if !sequence.is_relative_lock_time() {
         return false;
     }
 
     // Both values should be represented in the same unit (either time-based or
     // block-height based)
-    if rbf.is_time_locked() != csv.is_time_locked() {
+    if sequence.is_time_locked() != csv.is_time_locked() {
         return false;
     }
 
     // The value should be at least `csv`
-    if rbf < csv {
+    if sequence < csv {
         return false;
     }