Set default transaction nLocktime to discourage fee sniping

[notmandatory]

As suggested by @TheBlueMatt and in discussion with 0x10w#1026 on discord, by default BDK should follow the anti-fee-sniping logic used by Bitcoin Core and Electrum to set the nLocktime when creating new transactions. This default nLocktime value should still be able to be overridden if the the existing TxParams.locktime parameter is set. See:

Bitcoin Core anti-fee-sniping code:
https://github.com/bitcoin/bitcoin/blob/6d859cbd794a36ed1c0c72324a4ec54d029b3d3b/src/wallet/spend.cpp#L591-L631

Electrum anti-fee-sniping code:
https://github.com/spesmilo/electrum/blob/7e6d65ec11c0dccfc24478471c5951d3ae586937/electrum/wallet.py#L211-L224

Currently BDK sets the default transaction nLocktime to 0. See:

bdk/src/wallet/mod.rs

Lines 511 to 512 in b1346d4

	// No nLockTime, default to 0
	None => requirements.timelock.unwrap_or(0),

[afilini]

I think this could be tricky to implement with our planned architectural changes, because the code that builds the transaction will not have access to the blockchain to get the current height.

I don't think it's worth changing the architecture just to do this: after all it's very easy for the users of our library to explicitly set the nlocktime, so I don't think it's a huge problem if it's not done by default.

[kafaichoi]

Sorry where can I find information about the planned architectural changes you mentioned? I was thinking to give this issue a try but want to make sure I wasn't doing sth opposite of planned change🙏🏽

[afilini]

In this case I'm mainly referring to the idea of splitting up the Blockchain trait into multiple traits. After that we can make some methods of the Wallet struct available only if the given blockhain instance supports the right traits, which mean that we will probably make the transaction creation completely offline

[TheBlueMatt]

I'm sure its a nontrivial layer violation, but I'd implore you to think harder about at least setting this by default for most users. Sure, some designs may understandably not be able to support it, but making users explicitly think about passing a None instead of a Some(blockchain_ref) to the relevant functions is much better than nothing here. Its quite important for Bitcoin's long-term sustainability that wallets set nLocktime by default, not to mention privacy improvements for users.

[w0xlt]

I created a simple example that sets the nlocktime of a transaction using Bitcoin Core as backend.
https://github.com/w0xlt/bdk_nlocktime_test/blob/master/src/main.rs

I will add this code to #526.

As mentioned by @afilini, this is difficult to do by default as the transaction builder is completely independent of the backend and works offline.

Perhaps exposing TxParams publicly, as suggested in the TODO below, might help.
https://github.com/bitcoindevkit/bdk/blob/master/src/wallet/tx_builder.rs#L131

[notmandatory]

I removed the "good first issue" label since sounds like this needs more discussion.

[notmandatory]

We discussed this one in our team chat yesterday and as @afilini mentioned above once we separate the blockchain traits from Wallet we won't be able to automatically get the current block height and set the nLockTime as recommended. But we can still do the following:

1. Store the latest block height after each walled.sync in the wallet cache DB and use this value for calculating the default anti-fee-sniping nLockTime. The only down side is if the user hasn't sync'd recently then we will use an older height and leave more room for sniping.
2. Provide some sort of function on the TxBuilder that takes the current block height (or a blockchain trait?) and calculates the nLockTime based on this value. Something like TxBuilder.setAntiFeeSnipingLockTime.
3. Document clearly in the Rust docs that the Wallet should be sync'd prior to using TxBuilder or if syncing isn't possible they should use the TxBuilder.setAntiFeeSnipingLockTime with the current block height (or a blockchain trait?).

[notmandatory]

PS. the current block height (and time) from the latest Wallet.sync are already stored in the wallet's Database.

https://docs.rs/bdk/latest/bdk/database/trait.Database.html#tymethod.get_sync_time

bdk/src/database/mod.rs

Line 155 in b1346d4

fn get_sync_time(&self) -> Result<Option<SyncTime>, Error>;

[danielabrozzoni]

Store the latest block height after each walled.sync in the wallet cache DB and use this value for calculating the default anti-fee-sniping nLockTime. The only down side is if the user hasn't sync'd recently then we will use an older height and leave more room for sniping.

This means that if we don't sync for a while, all our transactions are going to have the same nLockTime, which means that a potential observer could link all our transactions together. Are we ok with that?
(See bitcoin/bitcoin#15039)

[afilini]

It shouldn't be a big issue imho. In general you should always sync before creating txs (otherwise you may accidentally double-spend an input)