wallet: Avoid leaking nLockTime fingerprint when anti-fee-sniping

MarcoFalke commented at 3:07 pm on December 26, 2018: member

The wallet sets the locktime to the current height of our active chain. This is fine, as long as our node is connected to other nodes. However, when we fall back and get stuck at a particular height (e.g. taking the wallet offline), the same (potentially unique) locktime is used for all transactions. This makes it easier for passive observers to cluster transactions by wallet.

For reference, I visualized “locktime-reuse” with the data:

blocks 545k-555k (both inclusive)
locktimes<=60k
excluding coinbase txs

distribution of height-based tx locktimes used at least twice

MarcoFalke added the label Wallet on Dec 26, 2018

MarcoFalke force-pushed on Dec 26, 2018

in src/wallet/wallet.cpp:2619 in fa6cb28ff0 outdated

2612@@ -2599,8 +2613,15 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
2613     if (GetRandInt(10) == 0)
2614         txNew.nLockTime = std::max(0, (int)txNew.nLockTime - GetRandInt(100));
2615 
2616+    // If our chain is lagging behind, we can't discourage fee sniping nor help
2617+    // the privacy of high-latency transactions. To avoid leaking a potentially
2618+    // unique "nLockTime fingerprint", set nLockTime to a constant.
2619+    if (!IsCurrentForAntiFeeSniping()) txNew.nLockTime = 0;

luke-jr commented at 10:37 pm on December 26, 2018:

Why not just skip the above setting it?

[test] wallet_txn_clone: Correctly clone txin sequence 453803adc9

MarcoFalke force-pushed on Dec 26, 2018

in src/wallet/wallet.cpp:2553 in fa7c5c7eef outdated

2548+{
2549+    if (IsInitialBlockDownload()) {
2550+        return false;
2551+    }
2552+    constexpr int64_t MAX_ANTI_FEE_SNIPING_TIP_AGE = 8 * 60 * 60; // in seconds
2553+    if (chainActive.Tip()->GetBlockTime() < (GetTime() - MAX_ANTI_FEE_SNIPING_TIP_AGE)) {

Empact commented at 0:27 am on December 27, 2018:

nit: could return this directly (inverted)

MarcoFalke force-pushed on Dec 27, 2018

wallet: Avoid leaking locktime fingerprint when anti-fee-sniping fa48baf23e

MarcoFalke force-pushed on Dec 27, 2018

DrahtBot commented at 11:57 am on December 28, 2018: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14711 (Remove uses of chainActive and mapBlockIndex in wallet code by ryanofsky)
#10973 (Refactor: separate wallet from node by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

laanwj commented at 4:16 pm on January 2, 2019: member

Concept ACK

This is fine, as long as our node is connected to other nodes.

I remember this was one of the remarks back then too. Anti-fee sniping only makes sense if it’s catched up with the chain.

fanquake requested review from meshcollider on Jan 3, 2019

in src/wallet/wallet.cpp:2524 in fa48baf23e

2515@@ -2516,6 +2516,65 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC
2516     return true;
2517 }
2518 
2519+static bool IsCurrentForAntiFeeSniping(interfaces::Chain::Lock& locked_chain)
2520+{
2521+    if (IsInitialBlockDownload()) {
2522+        return false;
2523+    }
2524+    constexpr int64_t MAX_ANTI_FEE_SNIPING_TIP_AGE = 8 * 60 * 60; // in seconds

meshcollider commented at 10:45 am on January 3, 2019:

Any rationale on why 8 hours was chosen? Seems sane though

meshcollider commented at 10:45 am on January 3, 2019: contributor

utACK https://github.com/bitcoin/bitcoin/pull/15039/commits/fa48baf23eb2ec5e9b71e3e07c24432fc1fed39c

in src/wallet/wallet.cpp:2565 in fa48baf23e

2560+
2561+        // Secondly occasionally randomly pick a nLockTime even further back, so
2562+        // that transactions that are delayed after signing for whatever reason,
2563+        // e.g. high-latency mix networks and some CoinJoin implementations, have
2564+        // better privacy.
2565+        if (GetRandInt(10) == 0)

laanwj commented at 8:30 am on January 4, 2019:

This if needs {} but I understand if you prefer to keep this move-only.

laanwj commented at 3:05 pm on January 8, 2019: member

utACK https://github.com/bitcoin/bitcoin/pull/15039/commits/fa48baf23eb2ec5e9b71e3e07c24432fc1fed39c

pull[bot] referenced this in commit cebe910718 on Jan 10, 2019

laanwj merged this on Jan 10, 2019

laanwj closed this on Jan 10, 2019

MarcoFalke deleted the branch on Jan 10, 2019

MarcoFalke commented at 9:12 pm on January 10, 2019: member

Unrelated also the same dataset (blocks 545k-555k (both inclusive), excl. coinbase) in a different representation:

distribution of height-based tx locktimes

keystrike commented at 3:35 pm on January 15, 2019: contributor

Thanks for fixing this. My analysis from 2017 is here, although the full output text file is no longer online. I had looked at all blocks to mid-2017. At that time there were 33 old locktimes in the blockchain in 94 transactions.

deadalnix referenced this in commit 35224e8f86 on Mar 26, 2020

ftrader referenced this in commit 95a9c0ab4b on Aug 17, 2020

kittywhiskers referenced this in commit 7b905fc2a9 on Oct 16, 2021

kittywhiskers referenced this in commit 26f3988977 on Oct 19, 2021

PastaPastaPasta referenced this in commit c70ada1e33 on Oct 19, 2021

kittywhiskers referenced this in commit d75e1e00f1 on Oct 21, 2021

kittywhiskers referenced this in commit e354a0e847 on Oct 31, 2021

kittywhiskers referenced this in commit e5c21c917e on Oct 31, 2021

kittywhiskers referenced this in commit aa2c3ed3e7 on Nov 1, 2021

kittywhiskers referenced this in commit 84087b4318 on Nov 4, 2021

kittywhiskers referenced this in commit 365e5c4205 on Nov 14, 2021

kittywhiskers referenced this in commit a97eebd068 on Nov 14, 2021

UdjinM6 referenced this in commit a3a8bfee08 on Nov 15, 2021

pravblockc referenced this in commit 878727b529 on Nov 18, 2021

pravblockc referenced this in commit 491608ffa3 on Nov 18, 2021

DrahtBot locked this on Dec 16, 2021

wallet: Avoid leaking nLockTime fingerprint when anti-fee-sniping #15039

Conflicts