diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2914,6 +2914,69 @@ return true; } +static bool IsCurrentForAntiFeeSniping(interfaces::Chain::Lock &locked_chain) { + if (IsInitialBlockDownload()) { + return false; + } + + // in seconds + constexpr int64_t MAX_ANTI_FEE_SNIPING_TIP_AGE = 8 * 60 * 60; + 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) { + 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 <= uint32_t(ChainActive().Height())); + assert(locktime < LOCKTIME_THRESHOLD); + return locktime; +} + OutputType CWallet::TransactionChangeType(OutputType change_type, const std::vector &vecSend) { @@ -2960,38 +3023,7 @@ 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)); - } - - assert(txNew.nLockTime <= (unsigned int)::ChainActive().Height()); - assert(txNew.nLockTime < LOCKTIME_THRESHOLD); + txNew.nLockTime = GetLocktimeForNewTransaction(locked_chainIn); { std::set setCoins; diff --git a/test/functional/wallet_create_tx.py b/test/functional/wallet_create_tx.py new file mode 100755 --- /dev/null +++ b/test/functional/wallet_create_tx.py @@ -0,0 +1,39 @@ +#!/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() diff --git a/test/functional/wallet_txn_clone.py b/test/functional/wallet_txn_clone.py --- a/test/functional/wallet_txn_clone.py +++ b/test/functional/wallet_txn_clone.py @@ -63,8 +63,9 @@ # 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"]