From 507c871ed5c59f1ef11e946f7436c94561803d86 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Fri, 19 Jul 2019 19:56:12 +0200 Subject: [PATCH 1/2] Merge #15681: [mempool] Allow one extra single-ancestor transaction per package 50cede3f5a4d4fbfbb7c420b94e661a6a159bced [mempool] Allow one extra single-ancestor transaction per package (Matt Corallo) Pull request description: This implements the proposed policy change from [1], which allows certain classes of contract protocols involving revocation punishments to use CPFP. Note that some such use-cases may still want some form of one-deep package relay, though even this alone may greatly simplify some lightning fee negotiation. [1] https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html ACKs for top commit: ajtowns: ACK 50cede3f5a4d4fbfbb7c420b94e661a6a159bced -- looked over code again, compared with previous commit, compiles, etc. sdaftuar: ACK 50cede3f5a4d4fbfbb7c420b94e661a6a159bced ryanofsky: utACK 50cede3f5a4d4fbfbb7c420b94e661a6a159bced. Changes since last review: adding EXTRA_DESCENDANT_TX_SIZE_LIMIT constant, changing max ancestor size from 1,000,000 to nLimitAncestorSize constant (101,000), fixing test comment and getting rid of unused test node. Tree-SHA512: b052c2a0f384855572b4579310131897b612201214b5abbb225167224e4f550049e300b471dbf320928652571e92ca2d650050b7cf39ac92b3bc1d2bcd386c1c --- src/validation.cpp | 16 +++- src/validation.h | 6 ++ test/functional/mempool_package_onemore.py | 86 ++++++++++++++++++++++ test/functional/test_runner.py | 1 + 4 files changed, 108 insertions(+), 1 deletion(-) create mode 100755 test/functional/mempool_package_onemore.py diff --git a/src/validation.cpp b/src/validation.cpp index ea31597809..b8fcc68d21 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -708,7 +708,21 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool size_t nLimitDescendantSize = gArgs.GetArg("-limitdescendantsize", DEFAULT_DESCENDANT_SIZE_LIMIT)*1000; std::string errString; if (!pool.CalculateMemPoolAncestors(entry, setAncestors, nLimitAncestors, nLimitAncestorSize, nLimitDescendants, nLimitDescendantSize, errString)) { - return state.DoS(0, false, REJECT_NONSTANDARD, "too-long-mempool-chain", false, errString); + setAncestors.clear(); + // If the new transaction is relatively small (up to 40k weight) + // and has at most one ancestor (ie ancestor limit of 2, including + // the new transaction), allow it if its parent has exactly the + // descendant limit descendants. + // + // This allows protocols which rely on distrusting counterparties + // being able to broadcast descendants of an unconfirmed transaction + // to be secure by simply only having two immediately-spendable + // outputs - one for each counterparty. For more info on the uses for + // this, see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html + if (nSize > EXTRA_DESCENDANT_TX_SIZE_LIMIT || + !pool.CalculateMemPoolAncestors(entry, setAncestors, 2, nLimitAncestorSize, nLimitDescendants + 1, nLimitDescendantSize + EXTRA_DESCENDANT_TX_SIZE_LIMIT, errString)) { + return state.DoS(0, false, REJECT_NONSTANDARD, "too-long-mempool-chain", false, errString); + } } // check special TXs after all the other checks. If we'd do this before the other checks, we might end up diff --git a/src/validation.h b/src/validation.h index 20a0e0b7eb..86f92934b5 100644 --- a/src/validation.h +++ b/src/validation.h @@ -67,6 +67,12 @@ static const unsigned int DEFAULT_ANCESTOR_SIZE_LIMIT = 101; static const unsigned int DEFAULT_DESCENDANT_LIMIT = 25; /** Default for -limitdescendantsize, maximum kilobytes of in-mempool descendants */ static const unsigned int DEFAULT_DESCENDANT_SIZE_LIMIT = 101; +/** + * An extra transaction can be added to a package, as long as it only has one + * ancestor and is no larger than this. Not really any reason to make this + * configurable as it doesn't materially change DoS parameters. + */ +static const unsigned int EXTRA_DESCENDANT_TX_SIZE_LIMIT = 10000; /** Default for -mempoolexpiry, expiration time for mempool transactions in hours */ static const unsigned int DEFAULT_MEMPOOL_EXPIRY = 336; /** The maximum size of a blk?????.dat file (since 0.8) */ diff --git a/test/functional/mempool_package_onemore.py b/test/functional/mempool_package_onemore.py new file mode 100755 index 0000000000..962f902047 --- /dev/null +++ b/test/functional/mempool_package_onemore.py @@ -0,0 +1,86 @@ +#!/usr/bin/env python3 +# Copyright (c) 2014-2019 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. +"""Test descendant package tracking carve-out allowing one final transaction in + an otherwise-full package as long as it has only one parent and is <= 10k in + size. +""" + +from decimal import Decimal + +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import assert_equal, assert_raises_rpc_error, satoshi_round + +MAX_ANCESTORS = 25 +MAX_DESCENDANTS = 25 + +class MempoolPackagesTest(BitcoinTestFramework): + def set_test_params(self): + self.num_nodes = 1 + self.extra_args = [["-maxorphantxsize=100000"]] + + def skip_test_if_missing_module(self): + self.skip_if_no_wallet() + + # Build a transaction that spends parent_txid:vout + # Return amount sent + def chain_transaction(self, node, parent_txids, vouts, value, fee, num_outputs): + send_value = satoshi_round((value - fee)/num_outputs) + inputs = [] + for (txid, vout) in zip(parent_txids, vouts): + inputs.append({'txid' : txid, 'vout' : vout}) + outputs = {} + for i in range(num_outputs): + outputs[node.getnewaddress()] = send_value + rawtx = node.createrawtransaction(inputs, outputs) + signedtx = node.signrawtransactionwithwallet(rawtx) + txid = node.sendrawtransaction(signedtx['hex']) + fulltx = node.getrawtransaction(txid, 1) + assert len(fulltx['vout']) == num_outputs # make sure we didn't generate a change output + return (txid, send_value) + + def run_test(self): + # Mine some blocks and have them mature. + self.nodes[0].generate(101) + utxo = self.nodes[0].listunspent(10) + txid = utxo[0]['txid'] + vout = utxo[0]['vout'] + value = utxo[0]['amount'] + + fee = Decimal("0.0002") + # MAX_ANCESTORS transactions off a confirmed tx should be fine + chain = [] + for _ in range(4): + (txid, sent_value) = self.chain_transaction(self.nodes[0], [txid], [vout], value, fee, 2) + vout = 0 + value = sent_value + chain.append([txid, value]) + for _ in range(MAX_ANCESTORS - 4): + (txid, sent_value) = self.chain_transaction(self.nodes[0], [txid], [0], value, fee, 1) + value = sent_value + chain.append([txid, value]) + (second_chain, second_chain_value) = self.chain_transaction(self.nodes[0], [utxo[1]['txid']], [utxo[1]['vout']], utxo[1]['amount'], fee, 1) + + # Check mempool has MAX_ANCESTORS + 1 transactions in it + assert_equal(len(self.nodes[0].getrawmempool(True)), MAX_ANCESTORS + 1) + + # Adding one more transaction on to the chain should fail. + assert_raises_rpc_error(-26, "too-long-mempool-chain", self.chain_transaction, self.nodes[0], [txid], [0], value, fee, 1) + # ...even if it chains on from some point in the middle of the chain. + assert_raises_rpc_error(-26, "too-long-mempool-chain", self.chain_transaction, self.nodes[0], [chain[2][0]], [1], chain[2][1], fee, 1) + assert_raises_rpc_error(-26, "too-long-mempool-chain", self.chain_transaction, self.nodes[0], [chain[1][0]], [1], chain[1][1], fee, 1) + # ...even if it chains on to two parent transactions with one in the chain. + assert_raises_rpc_error(-26, "too-long-mempool-chain", self.chain_transaction, self.nodes[0], [chain[0][0], second_chain], [1, 0], chain[0][1] + second_chain_value, fee, 1) + # ...especially if its > 40k weight + assert_raises_rpc_error(-26, "too-long-mempool-chain", self.chain_transaction, self.nodes[0], [chain[0][0]], [1], chain[0][1], fee, 350) + # But not if it chains directly off the first transaction + self.chain_transaction(self.nodes[0], [chain[0][0]], [1], chain[0][1], fee, 1) + # and the second chain should work just fine + self.chain_transaction(self.nodes[0], [second_chain], [0], second_chain_value, fee, 1) + + # Finally, check that we added two transactions + assert_equal(len(self.nodes[0].getrawmempool(True)), MAX_ANCESTORS + 3) + +if __name__ == '__main__': + MempoolPackagesTest().main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 417a5aef84..729ce509ea 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -166,6 +166,7 @@ BASE_SCRIPTS = [ 'rpc_invalidateblock.py', 'feature_txindex.py', 'mempool_packages.py', + 'mempool_package_onemore.py', 'feature_versionbits_warning.py', 'rpc_preciousblock.py', 'wallet_importprunedfunds.py', From c31713b401646c3ade6037cab4db7f3fc81850a5 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Mon, 29 Jul 2019 18:55:12 +0200 Subject: [PATCH 2/2] Merge #16471: [mempool] log correct messages when CPFP fails 42a5e912ee4e91a5191d659588f0605e1ada2f33 [mempool] log correct messages when CPFP fails (John Newbery) Pull request description: Fixes a logging issue introduced in #15681 ACKs for top commit: laanwj: ACK 42a5e912ee4e91a5191d659588f0605e1ada2f33 (+utACK from bluematt that isn't registered because it has no commit id) Tree-SHA512: ff5f423cc4d22838eea00c5b1d39ceda89cd61474c72f256a97c698eb0ec3f2156a97139f537669376132902c1e3943bf84c356a4b98a9a306b4ec57302c2761 --- src/validation.cpp | 4 +++- test/functional/mempool_package_onemore.py | 10 +++++----- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index b8fcc68d21..170e0b36e1 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -709,6 +709,8 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool std::string errString; if (!pool.CalculateMemPoolAncestors(entry, setAncestors, nLimitAncestors, nLimitAncestorSize, nLimitDescendants, nLimitDescendantSize, errString)) { setAncestors.clear(); + // If CalculateMemPoolAncestors fails second time, we want the original error string. + std::string dummy_err_string; // If the new transaction is relatively small (up to 40k weight) // and has at most one ancestor (ie ancestor limit of 2, including // the new transaction), allow it if its parent has exactly the @@ -720,7 +722,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool // outputs - one for each counterparty. For more info on the uses for // this, see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html if (nSize > EXTRA_DESCENDANT_TX_SIZE_LIMIT || - !pool.CalculateMemPoolAncestors(entry, setAncestors, 2, nLimitAncestorSize, nLimitDescendants + 1, nLimitDescendantSize + EXTRA_DESCENDANT_TX_SIZE_LIMIT, errString)) { + !pool.CalculateMemPoolAncestors(entry, setAncestors, 2, nLimitAncestorSize, nLimitDescendants + 1, nLimitDescendantSize + EXTRA_DESCENDANT_TX_SIZE_LIMIT, dummy_err_string)) { return state.DoS(0, false, REJECT_NONSTANDARD, "too-long-mempool-chain", false, errString); } } diff --git a/test/functional/mempool_package_onemore.py b/test/functional/mempool_package_onemore.py index 962f902047..464e163596 100755 --- a/test/functional/mempool_package_onemore.py +++ b/test/functional/mempool_package_onemore.py @@ -66,14 +66,14 @@ class MempoolPackagesTest(BitcoinTestFramework): assert_equal(len(self.nodes[0].getrawmempool(True)), MAX_ANCESTORS + 1) # Adding one more transaction on to the chain should fail. - assert_raises_rpc_error(-26, "too-long-mempool-chain", self.chain_transaction, self.nodes[0], [txid], [0], value, fee, 1) + assert_raises_rpc_error(-26, "too-long-mempool-chain, too many unconfirmed ancestors [limit: 25]", self.chain_transaction, self.nodes[0], [txid], [0], value, fee, 1) # ...even if it chains on from some point in the middle of the chain. - assert_raises_rpc_error(-26, "too-long-mempool-chain", self.chain_transaction, self.nodes[0], [chain[2][0]], [1], chain[2][1], fee, 1) - assert_raises_rpc_error(-26, "too-long-mempool-chain", self.chain_transaction, self.nodes[0], [chain[1][0]], [1], chain[1][1], fee, 1) + assert_raises_rpc_error(-26, "too-long-mempool-chain, too many descendants", self.chain_transaction, self.nodes[0], [chain[2][0]], [1], chain[2][1], fee, 1) + assert_raises_rpc_error(-26, "too-long-mempool-chain, too many descendants", self.chain_transaction, self.nodes[0], [chain[1][0]], [1], chain[1][1], fee, 1) # ...even if it chains on to two parent transactions with one in the chain. - assert_raises_rpc_error(-26, "too-long-mempool-chain", self.chain_transaction, self.nodes[0], [chain[0][0], second_chain], [1, 0], chain[0][1] + second_chain_value, fee, 1) + assert_raises_rpc_error(-26, "too-long-mempool-chain, too many descendants", self.chain_transaction, self.nodes[0], [chain[0][0], second_chain], [1, 0], chain[0][1] + second_chain_value, fee, 1) # ...especially if its > 40k weight - assert_raises_rpc_error(-26, "too-long-mempool-chain", self.chain_transaction, self.nodes[0], [chain[0][0]], [1], chain[0][1], fee, 350) + assert_raises_rpc_error(-26, "too-long-mempool-chain, too many descendants", self.chain_transaction, self.nodes[0], [chain[0][0]], [1], chain[0][1], fee, 350) # But not if it chains directly off the first transaction self.chain_transaction(self.nodes[0], [chain[0][0]], [1], chain[0][1], fee, 1) # and the second chain should work just fine