Merge bitcoindevkit/bdk#611: Discourage fee sniping with nLockTime

97bc9dc7170c336e97cd756b1f07ac3c23a39626 Discourage fee sniping with nLockTime (Daniela Brozzoni)

Pull request description:

  ### Description
  By default bdk sets the transaction's nLockTime to current_height
  to prevent fee sniping.
  current_height can be provided by the user through TxParams; if the user
  didn't provide it, we use the last sync height, or 0 if we never synced.

  Fixes https://github.com/bitcoindevkit/bdk/issues/533

  ### Notes to the reviewers:

  If you want to know more about fee sniping: https://bitcoinops.org/en/topics/fee-sniping/

  ### 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
  * [x] I've added docs for the new feature
  * [x] I've updated `CHANGELOG.md`

ACKs for top commit:
  afilini:
    ACK 97bc9dc7170c336e97cd756b1f07ac3c23a39626

Tree-SHA512: e92d1ae907687d9fee44d120d790f1ebdf14b698194979e1be8433310fd5636afa63808effed12fce6091f968ec6b76b727cfee6fed54068af0a7450239fdd26

CHANGELOG.md

@@ -6,6 +6,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 - New MSRV set to `1.56.1`
+- Fee sniping discouraging through nLockTime - if the user specifies a `current_height`, we use that as a nlocktime, otherwise we use the last sync height (or 0 if we never synced)
 
 ## [v0.19.0] - [v0.18.0]
 

src/wallet/mod.rs

@@ -619,9 +619,27 @@ where
             _ => 1,
         };
 
+        // We use a match here instead of a map_or_else as it's way more readable :)
+        let current_height = match params.current_height {
+            // If they didn't tell us the current height, we assume it's the latest sync height.
+            None => self
+                .database()
+                .get_sync_time()?
+                .map(|sync_time| sync_time.block_time.height),
+            h => h,
+        };
+
         let lock_time = match params.locktime {
-            // No nLockTime, default to 0
-            None => requirements.timelock.unwrap_or(0),
+            // When no nLockTime is specified, we try to prevent fee sniping, if possible
+            None => {
+                // Fee sniping can be partially prevented by setting the timelock
+                // to current_height. If we don't know the current_height,
+                // we default to 0.
+                let fee_sniping_height = current_height.unwrap_or(0);
+                // We choose the biggest between the required nlocktime and the fee sniping
+                // height
+                std::cmp::max(requirements.timelock.unwrap_or(0), fee_sniping_height)
+            }
             // Specific nLockTime required and we have no constraints, so just set to that value
             Some(x) if requirements.timelock.is_none() => x,
             // Specific nLockTime required and it's compatible with the constraints
@@ -1980,9 +1998,59 @@ pub(crate) mod test {
         builder.add_recipient(addr.script_pubkey(), 25_000);
         let (psbt, _) = builder.finish().unwrap();
 
+        // Since we never synced the wallet we don't have a last_sync_height
+        // we could use to try to prevent fee sniping. We default to 0.
         assert_eq!(psbt.unsigned_tx.lock_time, 0);
     }
 
+    #[test]
+    fn test_create_tx_fee_sniping_locktime_provided_height() {
+        let (wallet, _, _) = get_funded_wallet(get_test_wpkh());
+        let addr = wallet.get_address(New).unwrap();
+        let mut builder = wallet.build_tx();
+        builder.add_recipient(addr.script_pubkey(), 25_000);
+        let sync_time = SyncTime {
+            block_time: BlockTime {
+                height: 24,
+                timestamp: 0,
+            },
+        };
+        wallet
+            .database
+            .borrow_mut()
+            .set_sync_time(sync_time)
+            .unwrap();
+        let current_height = 25;
+        builder.set_current_height(current_height);
+        let (psbt, _) = builder.finish().unwrap();
+
+        // current_height will override the last sync height
+        assert_eq!(psbt.unsigned_tx.lock_time, current_height);
+    }
+
+    #[test]
+    fn test_create_tx_fee_sniping_locktime_last_sync() {
+        let (wallet, _, _) = get_funded_wallet(get_test_wpkh());
+        let addr = wallet.get_address(New).unwrap();
+        let mut builder = wallet.build_tx();
+        builder.add_recipient(addr.script_pubkey(), 25_000);
+        let sync_time = SyncTime {
+            block_time: BlockTime {
+                height: 25,
+                timestamp: 0,
+            },
+        };
+        wallet
+            .database
+            .borrow_mut()
+            .set_sync_time(sync_time.clone())
+            .unwrap();
+        let (psbt, _) = builder.finish().unwrap();
+
+        // If there's no current_height we're left with using the last sync height
+        assert_eq!(psbt.unsigned_tx.lock_time, sync_time.block_time.height);
+    }
+
     #[test]
     fn test_create_tx_default_locktime_cltv() {
         let (wallet, _, _) = get_funded_wallet(get_test_single_sig_cltv());
@@ -2001,9 +2069,13 @@ pub(crate) mod test {
         let mut builder = wallet.build_tx();
         builder
             .add_recipient(addr.script_pubkey(), 25_000)
+            .set_current_height(630_001)
             .nlocktime(630_000);
         let (psbt, _) = builder.finish().unwrap();
 
+        // When we explicitly specify a nlocktime
+        // we don't try any fee sniping prevention trick
+        // (we ignore the current_height)
         assert_eq!(psbt.unsigned_tx.lock_time, 630_000);
     }
 

src/wallet/tx_builder.rs

@@ -147,6 +147,7 @@ pub(crate) struct TxParams {
     pub(crate) add_global_xpubs: bool,
     pub(crate) include_output_redeem_witness_script: bool,
     pub(crate) bumping_fee: Option<PreviousFee>,
+    pub(crate) current_height: Option<u32>,
 }
 
 #[derive(Clone, Copy, Debug)]
@@ -543,6 +544,17 @@ impl<'a, D: BatchDatabase, Cs: CoinSelectionAlgorithm<D>, Ctx: TxBuilderContext>
         self.params.rbf = Some(RbfValue::Value(nsequence));
         self
     }
+
+    /// Set the current blockchain height.
+    ///
+    /// This will be used to set the nLockTime for preventing fee sniping. If the current height is
+    /// not provided, the last sync height will be used instead.
+    ///
+    /// **Note**: This will be ignored if you manually specify a nlocktime using [`TxBuilder::nlocktime`].
+    pub fn set_current_height(&mut self, height: u32) -> &mut Self {
+        self.params.current_height = Some(height);
+        self
+    }
 }
 
 impl<'a, D: BatchDatabase, Cs: CoinSelectionAlgorithm<D>> TxBuilder<'a, D, Cs, CreateTx> {