Merge branch 'master' of https://github.com/bitcoindevkit/bdk into rpc-pruned

Author: kjain1810

.github/workflows/cont_integration.yml

@@ -12,7 +12,7 @@ jobs:
         rust:
           - version: 1.60.0 # STABLE
             clippy: true
-          - version: 1.56.0 # MSRV
+          - version: 1.56.1 # MSRV
         features:
           - default
           - minimal
@@ -146,7 +146,7 @@ jobs:
       - run: sudo apt-get update || exit 1
       - run: sudo apt-get install -y libclang-common-10-dev clang-10 libc6-dev-i386 || exit 1
       - name: Set default toolchain
-        run: rustup default 1.56.0 # STABLE
+        run: rustup default 1.56.1 # STABLE
       - name: Set profile
         run: rustup set profile minimal
       - name: Add target wasm32

CHANGELOG.md

@@ -5,6 +5,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## [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]
 

README.md

@@ -13,7 +13,7 @@
     <a href="https://github.com/bitcoindevkit/bdk/actions?query=workflow%3ACI"><img alt="CI Status" src="https://github.com/bitcoindevkit/bdk/workflows/CI/badge.svg"></a>
     <a href="https://codecov.io/gh/bitcoindevkit/bdk"><img src="https://codecov.io/gh/bitcoindevkit/bdk/branch/master/graph/badge.svg"/></a>
     <a href="https://docs.rs/bdk"><img alt="API Docs" src="https://img.shields.io/badge/docs.rs-bdk-green"/></a>
-    <a href="https://blog.rust-lang.org/2020/08/27/Rust-1.56.0.html"><img alt="Rustc Version 1.56+" src="https://img.shields.io/badge/rustc-1.56%2B-lightgrey.svg"/></a>
+    <a href="https://blog.rust-lang.org/2021/11/01/Rust-1.56.1.html"><img alt="Rustc Version 1.56.1+" src="https://img.shields.io/badge/rustc-1.56.1%2B-lightgrey.svg"/></a>
     <a href="https://discord.gg/d7NkDKm"><img alt="Chat on Discord" src="https://img.shields.io/discord/753336465005608961?logo=discord"></a>
   </p>
 

src/descriptor/mod.rs

@@ -134,13 +134,13 @@ impl IntoWalletDescriptor for (ExtendedDescriptor, KeyMap) {
 
         let check_key = |pk: &DescriptorPublicKey| {
             let (pk, _, networks) = if self.0.is_witness() {
-                let desciptor_key: DescriptorKey<miniscript::Segwitv0> =
+                let descriptor_key: DescriptorKey<miniscript::Segwitv0> =
                     pk.clone().into_descriptor_key()?;
-                desciptor_key.extract(secp)?
+                descriptor_key.extract(secp)?
             } else {
-                let desciptor_key: DescriptorKey<miniscript::Legacy> =
+                let descriptor_key: DescriptorKey<miniscript::Legacy> =
                     pk.clone().into_descriptor_key()?;
-                desciptor_key.extract(secp)?
+                descriptor_key.extract(secp)?
             };
 
             if networks.contains(&network) {

src/types.rs

@@ -202,8 +202,10 @@ pub struct TransactionDetails {
     pub txid: Txid,
 
     /// Received value (sats)
+    /// Sum of owned outputs of this transaction.
     pub received: u64,
     /// Sent value (sats)
+    /// Sum of owned inputs of this transaction.
     pub sent: u64,
     /// Fee value (sats) if available.
     /// The availability of the fee depends on the backend. It's never `None` with an Electrum

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
@@ -791,7 +809,14 @@ where
         let drain_val = (coin_selection.selected_amount() - outgoing).saturating_sub(fee_amount);
 
         if tx.output.is_empty() {
-            if params.drain_to.is_some() {
+            // Uh oh, our transaction has no outputs.
+            // We allow this when:
+            // - We have a drain_to address and the utxos we must spend (this happens,
+            // for example, when we RBF)
+            // - We have a drain_to address and drain_wallet set
+            // Otherwise, we don't know who we should send the funds to, and how much
+            // we should send!
+            if params.drain_to.is_some() && (params.drain_wallet || !params.utxos.is_empty()) {
                 if drain_val.is_dust(&drain_output.script_pubkey) {
                     return Err(Error::InsufficientFunds {
                         needed: drain_output.script_pubkey.dust_value().as_sat(),
@@ -1980,9 +2005,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 +2076,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);
     }
 
@@ -2176,6 +2255,40 @@ pub(crate) mod test {
         assert_eq!(drain_output.value, 30_000 - details.fee.unwrap_or(0));
     }
 
+    #[test]
+    fn test_create_tx_drain_to_and_utxos() {
+        let (wallet, _, _) = get_funded_wallet(get_test_wpkh());
+        let addr = wallet.get_address(New).unwrap();
+        let utxos: Vec<_> = wallet
+            .get_available_utxos()
+            .unwrap()
+            .into_iter()
+            .map(|(u, _)| u.outpoint)
+            .collect();
+        let mut builder = wallet.build_tx();
+        builder
+            .drain_to(addr.script_pubkey())
+            .add_utxos(&utxos)
+            .unwrap();
+        let (psbt, details) = builder.finish().unwrap();
+
+        assert_eq!(psbt.unsigned_tx.output.len(), 1);
+        assert_eq!(
+            psbt.unsigned_tx.output[0].value,
+            50_000 - details.fee.unwrap_or(0)
+        );
+    }
+
+    #[test]
+    #[should_panic(expected = "NoRecipients")]
+    fn test_create_tx_drain_to_no_drain_wallet_no_utxos() {
+        let (wallet, _, _) = get_funded_wallet(get_test_wpkh());
+        let drain_addr = wallet.get_address(New).unwrap();
+        let mut builder = wallet.build_tx();
+        builder.drain_to(drain_addr.script_pubkey());
+        builder.finish().unwrap();
+    }
+
     #[test]
     fn test_create_tx_default_fee_rate() {
         let (wallet, _, _) = get_funded_wallet(get_test_wpkh());

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> {
@@ -574,6 +586,9 @@ impl<'a, D: BatchDatabase, Cs: CoinSelectionAlgorithm<D>> TxBuilder<'a, D, Cs, C
     /// difference is that it is valid to use `drain_to` without setting any ordinary recipients
     /// with [`add_recipient`] (but it is perfectly fine to add recipients as well).
     ///
+    /// If you choose not to set any recipients, you should either provide the utxos that the
+    /// transaction should spend via [`add_utxos`], or set [`drain_wallet`] to spend all of them.
+    ///
     /// When bumping the fees of a transaction made with this option, you probably want to
     /// use [`allow_shrinking`] to allow this output to be reduced to pay for the extra fees.
     ///
@@ -604,6 +619,7 @@ impl<'a, D: BatchDatabase, Cs: CoinSelectionAlgorithm<D>> TxBuilder<'a, D, Cs, C
     ///
     /// [`allow_shrinking`]: Self::allow_shrinking
     /// [`add_recipient`]: Self::add_recipient
+    /// [`add_utxos`]: Self::add_utxos
     /// [`drain_wallet`]: Self::drain_wallet
     pub fn drain_to(&mut self, script_pubkey: Script) -> &mut Self {
         self.params.drain_to = Some(script_pubkey);