From 97bc9dc7170c336e97cd756b1f07ac3c23a39626 Mon Sep 17 00:00:00 2001 From: Daniela Brozzoni Date: Wed, 30 Mar 2022 16:29:31 +0200 Subject: [PATCH] Discourage fee sniping with nLockTime By default bdk sets the transaction's nLockTime to current_height to discourage 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 #533 --- CHANGELOG.md | 1 + src/wallet/mod.rs | 76 ++++++++++++++++++++++++++++++++++++++-- src/wallet/tx_builder.rs | 12 +++++++ 3 files changed, 87 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7eaf9bff..a720dfb5 100644 --- a/CHANGELOG.md +++ b/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] diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs index 6d2ff385..5caf1624 100644 --- a/src/wallet/mod.rs +++ b/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); } diff --git a/src/wallet/tx_builder.rs b/src/wallet/tx_builder.rs index 59dedcf8..5f16b853 100644 --- a/src/wallet/tx_builder.rs +++ b/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, + pub(crate) current_height: Option, } #[derive(Clone, Copy, Debug)] @@ -543,6 +544,17 @@ impl<'a, D: BatchDatabase, Cs: CoinSelectionAlgorithm, 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> TxBuilder<'a, D, Cs, CreateTx> {