New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from Jan 10, 2019

Conversation

Projects
None yet
7 participants
@MarcoFalke
Copy link
Member

MarcoFalke commented Dec 26, 2018

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 MarcoFalke added the Wallet label Dec 26, 2018

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1812-walletLocktimeFingerprint branch Dec 26, 2018

Show resolved Hide resolved src/wallet/wallet.cpp Outdated

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1812-walletLocktimeFingerprint branch 2 times, most recently Dec 26, 2018

Show resolved Hide resolved src/wallet/wallet.cpp Outdated

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1812-walletLocktimeFingerprint branch Dec 27, 2018

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1812-walletLocktimeFingerprint branch to fa48baf Dec 27, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Dec 28, 2018

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

This comment has been minimized.

Copy link
Member

laanwj commented Jan 2, 2019

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 fanquake requested a review from MeshCollider Jan 3, 2019

@MeshCollider
Copy link
Member

MeshCollider left a comment

utACK fa48baf

if (IsInitialBlockDownload()) {
return false;
}
constexpr int64_t MAX_ANTI_FEE_SNIPING_TIP_AGE = 8 * 60 * 60; // in seconds

This comment has been minimized.

@MeshCollider

MeshCollider Jan 3, 2019

Member

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

// that transactions that are delayed after signing for whatever reason,
// e.g. high-latency mix networks and some CoinJoin implementations, have
// better privacy.
if (GetRandInt(10) == 0)

This comment has been minimized.

@laanwj

laanwj Jan 4, 2019

Member

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

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Jan 8, 2019

utACK fa48baf

pull bot pushed a commit to jaschadub/bitcoin that referenced this pull request Jan 10, 2019

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

fa48baf wallet: Avoid leaking locktime fingerprint when anti-fee-sniping (MarcoFalke)
453803a [test] wallet_txn_clone: Correctly clone txin sequence (MarcoFalke)

Pull request description:

  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](https://user-images.githubusercontent.com/6399679/50446163-b8256d80-0913-11e9-9832-40b76052b2b9.png)

Tree-SHA512: 2af259dd8f9f863312e2732d80ca8ba6a20c8d6d1c486b10a48479e1c85ccf13b0c38723740ebadde0f28d321cd9c133ad3e5d1e925472eb27681143bda2d0e7

@laanwj laanwj merged commit fa48baf into bitcoin:master Jan 10, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MarcoFalke MarcoFalke deleted the MarcoFalke:Mf1812-walletLocktimeFingerprint branch Jan 10, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

MarcoFalke commented Jan 10, 2019

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

distribution of height-based tx locktimes

@keystrike

This comment has been minimized.

Copy link
Contributor

keystrike commented Jan 15, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment