Skip to content
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
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -2516,6 +2516,65 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC
return true;
}

static bool IsCurrentForAntiFeeSniping(interfaces::Chain::Lock& locked_chain)
{
if (IsInitialBlockDownload()) {
return false;
}
constexpr int64_t MAX_ANTI_FEE_SNIPING_TIP_AGE = 8 * 60 * 60; // in seconds

This comment has been minimized.

Copy link
@meshcollider

meshcollider Jan 3, 2019

Member

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

if (chainActive.Tip()->GetBlockTime() < (GetTime() - MAX_ANTI_FEE_SNIPING_TIP_AGE)) {
return false;
}
return true;
}

/**
* Return a height-based locktime for new transactions (uses the height of the
* current chain tip unless we are not synced with the current chain
*/
static uint32_t GetLocktimeForNewTransaction(interfaces::Chain::Lock& locked_chain)
{
uint32_t locktime;
// Discourage fee sniping.
//
// For a large miner the value of the transactions in the best block and
// the mempool can exceed the cost of deliberately attempting to mine two
// blocks to orphan the current best block. By setting nLockTime such that
// only the next block can include the transaction, we discourage this
// practice as the height restricted and limited blocksize gives miners
// considering fee sniping fewer options for pulling off this attack.
//
// A simple way to think about this is from the wallet's point of view we
// always want the blockchain to move forward. By setting nLockTime this
// way we're basically making the statement that we only want this
// transaction to appear in the next block; we don't want to potentially
// encourage reorgs by allowing transactions to appear at lower heights
// than the next block in forks of the best chain.
//
// Of course, the subsidy is high enough, and transaction volume low
// enough, that fee sniping isn't a problem yet, but by implementing a fix
// now we ensure code won't be written that makes assumptions about
// nLockTime that preclude a fix later.
if (IsCurrentForAntiFeeSniping(locked_chain)) {
locktime = chainActive.Height();

// Secondly occasionally randomly pick a nLockTime even further back, so
// 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.

Copy link
@laanwj

laanwj Jan 4, 2019

Member

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

locktime = std::max(0, (int)locktime - GetRandInt(100));
} else {
// If our chain is lagging behind, we can't discourage fee sniping nor help
// the privacy of high-latency transactions. To avoid leaking a potentially
// unique "nLockTime fingerprint", set nLockTime to a constant.
locktime = 0;
}
assert(locktime <= (unsigned int)chainActive.Height());
assert(locktime < LOCKTIME_THRESHOLD);
return locktime;
}

OutputType CWallet::TransactionChangeType(OutputType change_type, const std::vector<CRecipient>& vecSend)
{
// If -changetype is specified, always use that change type.
@@ -2570,37 +2629,8 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std

CMutableTransaction txNew;

// Discourage fee sniping.
//
// For a large miner the value of the transactions in the best block and
// the mempool can exceed the cost of deliberately attempting to mine two
// blocks to orphan the current best block. By setting nLockTime such that
// only the next block can include the transaction, we discourage this
// practice as the height restricted and limited blocksize gives miners
// considering fee sniping fewer options for pulling off this attack.
//
// A simple way to think about this is from the wallet's point of view we
// always want the blockchain to move forward. By setting nLockTime this
// way we're basically making the statement that we only want this
// transaction to appear in the next block; we don't want to potentially
// encourage reorgs by allowing transactions to appear at lower heights
// than the next block in forks of the best chain.
//
// Of course, the subsidy is high enough, and transaction volume low
// enough, that fee sniping isn't a problem yet, but by implementing a fix
// now we ensure code won't be written that makes assumptions about
// nLockTime that preclude a fix later.
txNew.nLockTime = chainActive.Height();

// Secondly occasionally randomly pick a nLockTime even further back, so
// 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)
txNew.nLockTime = std::max(0, (int)txNew.nLockTime - GetRandInt(100));
txNew.nLockTime = GetLocktimeForNewTransaction(locked_chain);

assert(txNew.nLockTime <= (unsigned int)chainActive.Height());
assert(txNew.nLockTime < LOCKTIME_THRESHOLD);
FeeCalculation feeCalc;
CAmount nFeeNeeded;
int nBytes;
@@ -174,6 +174,7 @@
'wallet_fallbackfee.py',
'feature_minchainwork.py',
'rpc_getblockstats.py',
'wallet_create_tx.py',
'p2p_fingerprint.py',
'feature_uacomment.py',
'wallet_coinbase_category.py',
@@ -0,0 +1,35 @@
#!/usr/bin/env python3
# Copyright (c) 2018 The Bitcoin Core developers
# Distributed under the MIT software license, see the accompanying
# file COPYING or http://www.opensource.org/licenses/mit-license.php.

from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import (
assert_equal,
)


class CreateTxWalletTest(BitcoinTestFramework):
def set_test_params(self):
self.setup_clean_chain = False
self.num_nodes = 1

def skip_test_if_missing_module(self):
self.skip_if_no_wallet()

def run_test(self):
self.log.info('Check that we have some (old) blocks and that anti-fee-sniping is disabled')
assert_equal(self.nodes[0].getblockchaininfo()['blocks'], 200)
txid = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 1)
tx = self.nodes[0].decoderawtransaction(self.nodes[0].gettransaction(txid)['hex'])
assert_equal(tx['locktime'], 0)

self.log.info('Check that anti-fee-sniping is enabled when we mine a recent block')
self.nodes[0].generate(1)
txid = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 1)
tx = self.nodes[0].decoderawtransaction(self.nodes[0].gettransaction(txid)['hex'])
assert 0 < tx['locktime'] <= 201


if __name__ == '__main__':
CreateTxWalletTest().main()
@@ -65,7 +65,7 @@ def run_test(self):

# Construct a clone of tx1, to be malleated
rawtx1 = self.nodes[0].getrawtransaction(txid1, 1)
clone_inputs = [{"txid": rawtx1["vin"][0]["txid"], "vout": rawtx1["vin"][0]["vout"]}]
clone_inputs = [{"txid": rawtx1["vin"][0]["txid"], "vout": rawtx1["vin"][0]["vout"], "sequence": rawtx1["vin"][0]["sequence"]}]
clone_outputs = {rawtx1["vout"][0]["scriptPubKey"]["addresses"][0]: rawtx1["vout"][0]["value"],
rawtx1["vout"][1]["scriptPubKey"]["addresses"][0]: rawtx1["vout"][1]["value"]}
clone_locktime = rawtx1["locktime"]
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.