diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 714cd1b3..8072802e 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -8,10 +8,11 @@ use crate::{ bitcoin::BitcoinInterface, database::{Coin, DatabaseConnection, DatabaseInterface}, descriptors, + miniscript::bitcoin::absolute::LockTime, poller::PollerMessage, spend::{ - create_spend, AddrInfo, AncestorInfo, CandidateCoin, CreateSpendRes, SpendCreationError, - SpendOutputAddress, SpendTxFees, TxGetter, + self, create_spend, AddrInfo, AncestorInfo, CandidateCoin, CreateSpendRes, + SpendCreationError, SpendOutputAddress, SpendTxFees, TxGetter, }, DaemonControl, VERSION, }; @@ -25,8 +26,10 @@ use utils::{ use std::{ collections::{hash_map, HashMap, HashSet}, + convert::TryInto, fmt, sync::{self, mpsc}, + time::SystemTime, }; use miniscript::{ @@ -277,6 +280,21 @@ impl DaemonControl { } } } + + // Pass relevant values to the spend module function of same name. + fn anti_fee_sniping_locktime(&self) -> LockTime { + let now = SystemTime::now() + .duration_since(SystemTime::UNIX_EPOCH) + .expect("time measured now cannot be before unix epoch"); + let tip_time = self.bitcoin.tip_time(); + let tip_height: u32 = self + .bitcoin + .chain_tip() + .height + .try_into() + .expect("block height must fit in u32"); + spend::anti_fee_sniping_locktime(now, tip_height, tip_time) + } } impl DaemonControl { @@ -529,6 +547,7 @@ impl DaemonControl { // derivation index in case any address in the transaction outputs was ours and from the // future. let change_info = change_address.info; + let locktime = self.anti_fee_sniping_locktime(); let CreateSpendRes { psbt, has_change, @@ -541,6 +560,7 @@ impl DaemonControl { &candidate_coins, SpendTxFees::Regular(feerate_vb), change_address, + locktime, ) { Ok(res) => res, Err(SpendCreationError::CoinSelection(e)) => { @@ -901,6 +921,7 @@ impl DaemonControl { // will ensure that the replacement transaction additionally pays for its own weight as per // RBF rule 4. let replaced_fee = descendant_fees.to_sat(); + let locktime = self.anti_fee_sniping_locktime(); // This loop can have up to 2 iterations in the case of cancel and otherwise only 1. loop { match create_spend( @@ -911,6 +932,7 @@ impl DaemonControl { &candidate_coins, SpendTxFees::Rbf(feerate_vb, replaced_fee), change_address.clone(), + locktime, ) { Ok(CreateSpendRes { psbt, @@ -1078,6 +1100,7 @@ impl DaemonControl { } let sweep_addr_info = sweep_addr.info; + let locktime = self.anti_fee_sniping_locktime(); let CreateSpendRes { psbt, has_change, .. } = create_spend( @@ -1088,6 +1111,7 @@ impl DaemonControl { &sweepable_coins, SpendTxFees::Regular(feerate_vb), sweep_addr, + locktime, )?; if has_change { self.maybe_increase_next_deriv_index(&mut db_conn, &sweep_addr_info); diff --git a/src/spend.rs b/src/spend.rs index ef858801..9465b775 100644 --- a/src/spend.rs +++ b/src/spend.rs @@ -4,6 +4,7 @@ use std::{ collections::{BTreeMap, HashMap}, convert::TryInto, fmt, + time::Duration, }; pub use bdk_coin_select::InsufficientFunds; @@ -34,6 +35,10 @@ pub const MAX_FEE: bitcoin::Amount = bitcoin::Amount::ONE_BTC; /// Assume that paying more than 1000sat/vb in feerate is a bug. pub const MAX_FEERATE: u64 = 1_000; +/// Do not set locktime if tip age in seconds is older than this. +// See also https://github.com/bitcoin/bitcoin/blob/ecd23656db174adef61d3bd753d02698c3528192/src/wallet/spend.cpp#L906. +pub const MAX_ANTI_FEE_SNIPING_TIP_AGE_SECS: u64 = 8 * 60 * 60; // 8 hours + #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum InsaneFeeInfo { NegativeFee, @@ -498,6 +503,40 @@ fn derived_desc( desc.derive(coin.deriv_index, secp) } +/// Get value to use for transaction nLockTime in order to +/// discourage fee sniping. +/// +/// The approach follows that taken by Bitcoin Core: +/// - most of the time, the value returned will be the current +/// block height, but will randomly be up to 100 blocks earlier. +/// - if the current tip is more than [`MAX_ANTI_FEE_SNIPING_TIP_AGE_SECS`] +/// seconds old, a locktime value of 0 will be returned. +pub fn anti_fee_sniping_locktime( + now: Duration, + tip_height: u32, + tip_time_secs: Option, +) -> LockTime { + tip_time_secs + .map(|tip_time| now.as_secs().saturating_sub(tip_time.into())) + .filter(|tip_age| *tip_age <= MAX_ANTI_FEE_SNIPING_TIP_AGE_SECS) + .map(|_| { + // Randomly (approx 10% of cases) set locktime further back + // using current time as source of randomness. + let nanos = now.subsec_nanos(); + // Note this condition will fail if nano precision is not available + // and so nothing will be subtracted. + let delta = if nanos % 10 == 1 { + (nanos % 1000) / 10 + 1 // a number in [1, 100] + } else { + 0 + }; + let height = tip_height.saturating_sub(delta); + LockTime::from_height(height) + .expect("height is valid block height as it cannot be bigger than tip height") + }) + .unwrap_or(LockTime::Blocks(Height::ZERO)) +} + #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct AddrInfo { pub index: bip32::ChildNumber, @@ -590,6 +629,8 @@ pub struct CreateSpendRes { /// * `change_addr`: the address to use for a change output if we need to create one. Can be set to /// an external address (if combined with an empty list of `destinations` it's useful to sweep some /// or all coins of a wallet to an external address). +/// * `locktime`: the locktime to use for the transaction. +#[allow(clippy::too_many_arguments)] pub fn create_spend( main_descriptor: &descriptors::LianaDescriptor, secp: &secp256k1::Secp256k1, @@ -598,6 +639,7 @@ pub fn create_spend( candidate_coins: &[CandidateCoin], fees: SpendTxFees, change_addr: SpendOutputAddress, + locktime: LockTime, ) -> Result { // This method does quite a few things. In addition, we support different modes (coin control // vs automated coin selection, self-spend, sweep, etc..) which make the logic a bit more @@ -626,7 +668,7 @@ pub fn create_spend( // Create transaction with no inputs and no outputs. let mut tx = bitcoin::Transaction { version: bitcoin::transaction::Version::TWO, - lock_time: LockTime::Blocks(Height::ZERO), // TODO: randomized anti fee sniping + lock_time: locktime, input: Vec::with_capacity(candidate_coins.iter().filter(|c| c.must_select).count()), output: Vec::with_capacity(destinations.len()), }; @@ -788,3 +830,122 @@ pub fn create_spend( warnings, }) } + +#[cfg(test)] +mod tests { + use super::*; + + use std::time::Duration; + + use miniscript::bitcoin::absolute::{Height, LockTime}; + + #[test] + fn test_anti_fee_sniping_locktime() { + // If we have no tip time, locktime is 0. + assert_eq!( + anti_fee_sniping_locktime(Duration::from_secs(100), 123_456, None), + LockTime::Blocks(Height::ZERO) + ); + + // If tip time is too old, locktime is 0. + assert_eq!( + anti_fee_sniping_locktime( + Duration::from_secs(100_000), + 123_456, + Some(100_000 - (8 * 60 * 60) - 1) + ), + LockTime::Blocks(Height::ZERO) + ); + + // If tip age is exactly the max threshold, we set locktime. + assert_eq!( + anti_fee_sniping_locktime( + Duration::from_secs(100_000), + 123_456, + Some(100_000 - (8 * 60 * 60)) + ), + LockTime::from_height(123_456).unwrap() + ); + + // If tip time is later than now, we set locktime depending on nanos. + // If nanos are 0, set to current height. + assert_eq!( + anti_fee_sniping_locktime(Duration::from_secs(50_000), 123_456, Some(100_000)), + LockTime::from_height(123_456).unwrap() + ); + + // We might set locktime earlier than current height. + assert_eq!( + anti_fee_sniping_locktime( + Duration::from_secs(50_000) + Duration::from_nanos(1), + 123_456, + Some(100_000) + ), + LockTime::from_height(123_455).unwrap() // subtract 1 + ); + + // If tip time is older than now, we also vary the locktime depending on current nanos. + // If nanos are truncated or 0, set locktime to current height. + assert_eq!( + anti_fee_sniping_locktime(Duration::from_secs(100), 123_456, Some(100)), + LockTime::from_height(123_456).unwrap() // subtract 1 + ); + + assert_eq!( + anti_fee_sniping_locktime( + Duration::from_secs(100) + Duration::from_nanos(1), + 123_456, + Some(100) + ), + LockTime::from_height(123_455).unwrap() // subtract 1 + ); + + assert_eq!( + anti_fee_sniping_locktime( + Duration::from_secs(100) + Duration::from_nanos(10_000_041), + 123_456, + Some(100) + ), + LockTime::from_height(123_451).unwrap() // subtract 5 + ); + + // If nanos % 10 != 1, don't subtract anything. + assert_eq!( + anti_fee_sniping_locktime( + Duration::from_secs(100) + Duration::from_nanos(10_000_040), + 123_456, + Some(100) + ), + LockTime::from_height(123_456).unwrap() // subtract 0 + ); + + assert_eq!( + anti_fee_sniping_locktime( + Duration::from_secs(100) + Duration::from_nanos(100_000_891), + 123_456, + Some(100) + ), + LockTime::from_height(123_366).unwrap() // subtract 90 + ); + + // We would subtract 90, but current height is 56, so return locktime of 0. + assert_eq!( + anti_fee_sniping_locktime( + Duration::from_secs(100) + Duration::from_nanos(100_000_891), + 56, + Some(100) + ), + LockTime::Blocks(Height::ZERO) + ); + + // If block height is 91, we can now subtract 90. + assert_eq!( + anti_fee_sniping_locktime( + Duration::from_secs(100) + Duration::from_nanos(100_000_891), + 91, + Some(100) + ), + LockTime::from_height(1).unwrap() // subtract 90 + ); + } +} diff --git a/src/testutils.rs b/src/testutils.rs index 712d3906..0d4b028e 100644 --- a/src/testutils.rs +++ b/src/testutils.rs @@ -114,7 +114,7 @@ impl BitcoinInterface for DummyBitcoind { } fn tip_time(&self) -> Option { - todo!() + None } fn wallet_transaction( diff --git a/tests/test_chain.py b/tests/test_chain.py index 0dcf2b49..26487c99 100644 --- a/tests/test_chain.py +++ b/tests/test_chain.py @@ -150,6 +150,8 @@ def test_reorg_status_recovery(lianad, bitcoind): """ list_coins = lambda: lianad.rpc.listcoins()["coins"] + # Generate blocks in order to test locktime set correctly. + bitcoind.generate_block(200) # Create two confirmed coins. Note how we take the initial_height after having # mined them, as we'll reorg back to this height and due to anti fee-sniping # these deposit transactions might not be valid anymore! @@ -157,28 +159,37 @@ def test_reorg_status_recovery(lianad, bitcoind): txids = [bitcoind.rpc.sendtoaddress(addr, 0.5670) for addr in addresses] bitcoind.generate_block(1, wait_for_mempool=txids) initial_height = bitcoind.rpc.getblockcount() + assert initial_height > 100 wait_for(lambda: lianad.rpc.getinfo()["block_height"] == initial_height) # Both coins are confirmed. Spend the second one then get their infos. wait_for(lambda: len(list_coins()) == 2) wait_for(lambda: all(c["block_height"] is not None for c in list_coins())) coin_b = get_coin(lianad, txids[1]) - spend_coins(lianad, bitcoind, [coin_b]) + tx = spend_coins(lianad, bitcoind, [coin_b]) + locktime = bitcoind.rpc.decoderawtransaction(tx)["locktime"] + assert initial_height - 100 <= locktime <= initial_height bitcoind.generate_block(1, wait_for_mempool=1) wait_for(lambda: spend_confirmed_noticed(lianad, coin_b["outpoint"])) coin_a = get_coin(lianad, txids[0]) coin_b = get_coin(lianad, txids[1]) # Reorg the chain down to the initial height without shifting nor malleating - # any transaction. The coin info should be identical (except the transaction - # spending the second coin will be mined at the height the reorg happened). + # any transaction. The coin info should be identical (except the spend info + # of the transaction spending the second coin). bitcoind.simple_reorg(initial_height, shift=0) new_height = bitcoind.rpc.getblockcount() wait_for(lambda: lianad.rpc.getinfo()["block_height"] == new_height) new_coin_a = get_coin(lianad, coin_a["outpoint"]) assert coin_a == new_coin_a new_coin_b = get_coin(lianad, coin_b["outpoint"]) - coin_b["spend_info"]["height"] = initial_height + + if locktime == initial_height: + # Cannot be mined until next block (initial_height + 1). + coin_b["spend_info"] = None + else: + # Otherwise, the tx will be mined at the height the reorg happened. + coin_b["spend_info"]["height"] = initial_height assert new_coin_b == coin_b diff --git a/tests/test_rpc.py b/tests/test_rpc.py index 38894620..968b7072 100644 --- a/tests/test_rpc.py +++ b/tests/test_rpc.py @@ -872,6 +872,8 @@ def test_listtransactions(lianad, bitcoind): def test_create_recovery(lianad, bitcoind): """Test the sweep of coins that are available through the timelocked path.""" + # Generate blocks in order to test locktime set correctly. + bitcoind.generate_block(200) # Start by getting a few coins destinations = { lianad.rpc.getnewaddress()["address"]: 0.1, @@ -900,6 +902,13 @@ def test_create_recovery(lianad, bitcoind): # Now we can create a recovery tx that sweeps the first 3 coins. res = lianad.rpc.createrecovery(bitcoind.rpc.getnewaddress(), 18) reco_psbt = PSBT.from_base64(res["psbt"]) + + # Check locktime being set correctly. + tip_height = bitcoind.rpc.getblockcount() + assert tip_height > 100 + locktime = reco_psbt.tx.nLockTime + assert tip_height - 100 <= locktime <= tip_height + assert len(reco_psbt.tx.vin) == 3, "The last coin's timelock hasn't matured yet" assert len(reco_psbt.tx.vout) == 1 assert int(0.5999 * COIN) < int(reco_psbt.tx.vout[0].nValue) < int(0.6 * COIN) @@ -1048,6 +1057,8 @@ def test_labels(lianad, bitcoind): def test_rbfpsbt_bump_fee(lianad, bitcoind): """Test the use of RBF to bump the fee of a transaction.""" + # Generate blocks in order to test locktime set correctly. + bitcoind.generate_block(200) # Get three coins. destinations = { lianad.rpc.getnewaddress()["address"]: 0.003, @@ -1100,6 +1111,12 @@ def test_rbfpsbt_bump_fee(lianad, bitcoind): # Let's use an even higher feerate. rbf_1_res = lianad.rpc.rbfpsbt(first_txid, False, 10) rbf_1_psbt = PSBT.from_base64(rbf_1_res["psbt"]) + + # Check the locktime is being set. + tip_height = bitcoind.rpc.getblockcount() + locktime = rbf_1_psbt.tx.nLockTime + assert tip_height - 100 <= locktime <= tip_height + # The inputs are the same in both (no new inputs needed in the replacement). assert sorted(i.prevout.serialize() for i in first_psbt.tx.vin) == sorted( i.prevout.serialize() for i in rbf_1_psbt.tx.vin