Merge #1092: spend: randomized anti-fee sniping

f2791744d8ad3068d12a39bb07f3745012785a5f spend: set locktime for anti-fee sniping (jp1ac4)

Pull request description:

  This is to resolve #44.

  In the first commit, I combine the destinations and change parameters in `spend::create_spend` in order to avoid "too many arguments" error when adding a new parameter. I think these two parameters combine naturally so that `destinations` includes both recipients and change address.

  The second commit sets locktime following the same approach as Bitcoin Core:
  - locktime is set to current tip height, but randomly (about 10% of cases) value is set up to 100 blocks earlier
  - if tip is more than 8 hours old, locktime is set to 0

  For randomness, I'm currently using the current time's milliseconds in order not to add another dependency, which I think is good enough for this use case.

  For consistency, I decided to use "locktime" everywhere instead of "lock time" or "lock-time".

ACKs for top commit:
  darosior:
    ACK f2791744d8ad3068d12a39bb07f3745012785a5f

Tree-SHA512: 69a57cf664e24b32a835c35eaf9016961b2d0f396891a826582044e6302b2ca04dcf5bf2617b5e18dcbfa25cc254a6e8025262718984095677c829ce051a66cc

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,
+        self, create_spend, AddrInfo, AncestorInfo, CandidateCoin, CreateSpendRes,
-        SpendOutputAddress, SpendTxFees, TxGetter,
+        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);

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<u32>,
+) -> 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<secp256k1::VerifyOnly>,
@@ -598,6 +639,7 @@ pub fn create_spend(
     candidate_coins: &[CandidateCoin],
     fees: SpendTxFees,
     change_addr: SpendOutputAddress,
+    locktime: LockTime,
 ) -> Result<CreateSpendRes, SpendCreationError> {
     // 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
+        );
+    }
+}

src/testutils.rs

@@ -114,7 +114,7 @@ impl BitcoinInterface for DummyBitcoind {
     }
 
     fn tip_time(&self) -> Option<u32> {
-        todo!()
+        None
     }
 
     fn wallet_transaction(

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
+    # any transaction. The coin info should be identical (except the spend info
-    # spending the second coin will be mined at the height the reorg happened).
+    # 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
 
 

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