diff --git a/src/miner.cpp b/src/miner.cpp --- a/src/miner.cpp +++ b/src/miner.cpp @@ -456,8 +456,17 @@ } if (packageFees < blockMinFeeRate.GetFee(packageSize)) { - // Everything else we might consider has a lower fee rate - return; + // Don't include this package, but don't stop yet because something + // else we might consider may have a sufficient fee rate (since txes + // are ordered by virtualsize feerate, not actual feerate). + if (fUsingModified) { + // Since we always look at the best entry in mapModifiedTx, we + // must erase failed entries so that we can consider the next + // best entry on the next loop iteration + mapModifiedTx.get().erase(modit); + failedTx.insert(iter); + } + continue; } // The following must not use virtual size since TestPackage relies on diff --git a/src/test/mempool_tests.cpp b/src/test/mempool_tests.cpp --- a/src/test/mempool_tests.cpp +++ b/src/test/mempool_tests.cpp @@ -324,6 +324,11 @@ LOCK2(cs_main, pool.cs); TestMemPoolEntryHelper entry; + /* Remove the default nonzero sigops, since the below tests are focussing on + * fee-based ordering and involve some artificially very tiny 21-byte + * transactions without any inputs. */ + entry.SigOpCount(0); + /* 3rd highest fee */ CMutableTransaction tx1 = CMutableTransaction(); tx1.vout.resize(1); @@ -508,6 +513,11 @@ LOCK2(cs_main, pool.cs); TestMemPoolEntryHelper entry; + /* Remove the default nonzero sigops, since the below tests are focussing on + * fee-based ordering and involve some artificially very tiny 21-byte + * transactions without any inputs. */ + entry.SigOpCount(0); + /* 3rd highest fee */ CMutableTransaction tx1 = CMutableTransaction(); tx1.vout.resize(1); diff --git a/src/txmempool.h b/src/txmempool.h --- a/src/txmempool.h +++ b/src/txmempool.h @@ -252,15 +252,17 @@ double &size) const { // Compare feerate with descendants to feerate of the transaction, and // return the fee/size for the max. - double f1 = a.GetSizeWithDescendants() * (a.GetModifiedFee() / SATOSHI); - double f2 = a.GetTxSize() * (a.GetModFeesWithDescendants() / SATOSHI); + double f1 = + a.GetVirtualSizeWithDescendants() * (a.GetModifiedFee() / SATOSHI); + double f2 = + a.GetTxVirtualSize() * (a.GetModFeesWithDescendants() / SATOSHI); if (f2 > f1) { mod_fee = a.GetModFeesWithDescendants() / SATOSHI; - size = a.GetSizeWithDescendants(); + size = a.GetVirtualSizeWithDescendants(); } else { mod_fee = a.GetModifiedFee() / SATOSHI; - size = a.GetTxSize(); + size = a.GetTxVirtualSize(); } } }; @@ -319,15 +321,17 @@ void GetModFeeAndSize(const T &a, double &mod_fee, double &size) const { // Compare feerate with ancestors to feerate of the transaction, and // return the fee/size for the min. - double f1 = a.GetSizeWithAncestors() * (a.GetModifiedFee() / SATOSHI); - double f2 = a.GetTxSize() * (a.GetModFeesWithAncestors() / SATOSHI); + double f1 = + a.GetVirtualSizeWithAncestors() * (a.GetModifiedFee() / SATOSHI); + double f2 = + a.GetTxVirtualSize() * (a.GetModFeesWithAncestors() / SATOSHI); if (f1 > f2) { mod_fee = a.GetModFeesWithAncestors() / SATOSHI; - size = a.GetSizeWithAncestors(); + size = a.GetVirtualSizeWithAncestors(); } else { mod_fee = a.GetModifiedFee() / SATOSHI; - size = a.GetTxSize(); + size = a.GetTxVirtualSize(); } } }; diff --git a/src/txmempool.cpp b/src/txmempool.cpp --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -1190,7 +1190,7 @@ // mempool with feerate equal to txn which were removed with no block in // between. CFeeRate removed(it->GetModFeesWithDescendants(), - it->GetSizeWithDescendants()); + it->GetVirtualSizeWithDescendants()); removed += MEMPOOL_FULL_FEE_INCREMENT; trackPackageRemoved(removed); diff --git a/src/validation.cpp b/src/validation.cpp --- a/src/validation.cpp +++ b/src/validation.cpp @@ -683,6 +683,7 @@ CTxMemPoolEntry entry(ptx, nFees, nAcceptTime, chainActive.Height(), fSpendsCoinbase, nSigOpsCount, lp); unsigned int nSize = entry.GetTxSize(); + unsigned int nVirtualSize = entry.GetTxVirtualSize(); // Check that the transaction doesn't have an excessive number of // sigops, making it impossible to mine. Since the coinbase transaction @@ -708,7 +709,7 @@ pool.GetMinFee( gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000) - .GetFee(nSize); + .GetFee(nVirtualSize); if (!bypass_limits && mempoolRejectFee > Amount::zero() && nModifiedFees < mempoolRejectFee) { return state.DoS( diff --git a/test/functional/abc-mempool-limit-sigops.py b/test/functional/abc-mempool-limit-sigops.py new file mode 100644 --- /dev/null +++ b/test/functional/abc-mempool-limit-sigops.py @@ -0,0 +1,208 @@ +#!/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 mempool limiting together/eviction with the wallet, using high-sigops transactions. + +based on mempool_limit.py +""" + +from test_framework.blocktools import ( + create_block, + create_coinbase, + make_conform_to_ctor, +) +from test_framework.messages import ( + CBlock, + COutPoint, + CTransaction, + CTxIn, + CTxOut, + FromHex, + ToHex, +) +from test_framework.mininode import ( + P2PDataStore, +) +from test_framework.script import ( + OP_CHECKMULTISIG, + OP_RETURN, + OP_TRUE, +) +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import ( + assert_equal, + assert_raises_rpc_error, + JSONRPCException +) + +from decimal import Decimal +from collections import deque + + +def create_var_transaction(spendfrom, custom_script, size_bytes, fee_sats): + # Fund and sign a transaction to a given output, padding it to exactly + # size = size_bytes and providing the given fee. + # spendfrom should be a CTransaction with first output to OP_TRUE. + + customout = CTxOut(0, custom_script) + # set output amount to required dust + customout.nValue = (len(customout.serialize()) + 148) * 3 + + ctx = CTransaction() + ctx.vin.append(CTxIn(COutPoint(spendfrom.sha256, 0), b'')) + ctx.vout.append( + CTxOut(spendfrom.vout[0].nValue - customout.nValue - fee_sats, bytes([OP_TRUE]))) + ctx.vout.append(customout) + # two padding outputs + ctx.vout.append(CTxOut(0, b'')) + ctx.vout.append(CTxOut(0, b'')) + + size_without_padding = len(ctx.serialize()) + padding_needed = size_bytes - size_without_padding + assert padding_needed >= 0, (size_bytes, size_without_padding) + + # Now insert padding. We need to be careful due to the flexible + # var_int size. + if padding_needed > 0xFFFF + 4: + # We can fit all padding in one script. + # 4 extra bytes will be used for var_int + ctx.vout[2].scriptPubKey = bytes( + [OP_RETURN]) * (padding_needed - 4) + padding_needed = 0 + elif padding_needed >= 0xFD + 2: + # We can pad most (probably all) using a script that is between + # 253 and 65535 in length. Probably fit everything in one script. + # 2 extra bytes will be used for var_int + scriptsize = min(padding_needed - 2, 0xFFFF) + ctx.vout[2].scriptPubKey = bytes([OP_RETURN]) * scriptsize + padding_needed -= scriptsize + 2 + elif padding_needed >= 0xFD: + # 0xFD or 0xFD+1. We can't pad with just one script, so just + # take off some. + ctx.vout[2].scriptPubKey = bytes([OP_RETURN]) * 50 + padding_needed -= 50 + + # extra padding on another output is needed if original padding_needed + # was <= 0xfd+1, or was 0xffff+3 or 0xffff+4 . + assert padding_needed < 0xfd + ctx.vout[3].scriptPubKey = bytes([OP_RETURN]) * padding_needed + + assert_equal(len(ctx.serialize()), size_bytes) + return ctx + + +class MempoolLimitSigopsTest(BitcoinTestFramework): + + def set_test_params(self): + self.setup_clean_chain = True + self.num_nodes = 1 + self.extra_args = [["-maxmempool=5", "-spendzeroconfchange=0"]] + self.block_heights = {} + + def getbestblock(self, node): + """Get the best block. Register its height so we can use build_block.""" + block_height = node.getblockcount() + blockhash = node.getblockhash(block_height) + block = FromHex(CBlock(), node.getblock(blockhash, 0)) + block.calc_sha256() + self.block_heights[block.sha256] = block_height + return block + + def build_block(self, parent, transactions=(), nTime=None): + """Make a new block with an OP_1 coinbase output. + + Requires parent to have its height registered.""" + parent.calc_sha256() + block_height = self.block_heights[parent.sha256] + 1 + block_time = (parent.nTime + 1) if nTime is None else nTime + + block = create_block( + parent.sha256, create_coinbase(block_height), block_time) + block.nVersion = 4 + block.vtx.extend(transactions) + make_conform_to_ctor(block) + block.hashMerkleRoot = block.calc_merkle_root() + block.solve() + self.block_heights[block.sha256] = block_height + return block + + def run_test(self): + (node,) = self.nodes + node.add_p2p_connection(P2PDataStore()) + + tip = self.getbestblock(node) + + self.log.info("Create some blocks with OP_1 coinbase for spending.") + blocks = [] + for _ in range(700): + tip = self.build_block(tip) + blocks.append(tip) + node.p2p.send_blocks_and_test(blocks, node, success=True) + self.spendable_outputs = deque(block.vtx[0] for block in blocks) + + self.log.info( + "Mature the blocks and get out of IBD; also make some coinbases for the node wallet.") + node.generate(120) + + tip = self.getbestblock(node) + + self.log.info( + "Put a regular transaction in mempool at 1 sat/byte.") + txid1 = node.sendtoaddress(node.getnewaddress(), 0.001) + + self.log.info( + "Flood the mempool with 10-kB transactions @ 9.9 sat/byte each with 4000 sigops, until it is full") + script_4000_sigops = bytes([OP_CHECKMULTISIG] * 200) + # About 500 should fill up our 5 MB mempool. + # Even with an empty pool, somewhat fewer than 500 will be accepted + # depending on mempool data structure sizes. + for i in range(500): + spendfrom = self.spendable_outputs.pop() + ctx = create_var_transaction( + spendfrom, script_4000_sigops, 10000, 99000) + try: + node.sendrawtransaction(ToHex(ctx)) + except JSONRPCException as e: + if 'mempool full' in e.error['message'] or 'mempool min fee not met' in e.error['message']: + self.log.info( + "Mempool filled after {} transactions".format(i,)) + break + raise + else: + raise RuntimeError("didn't fill mempool") + + # Did not get txid1 ejected + mempool_txids = set(node.getrawmempool()) + assert txid1 in mempool_txids + + # The removed txes have feerate 495 sat/kvbyte and the fee floor jumps by + # 1000 sat/kbyte (MEMPOOL_FULL_FEE_INCREMENT) on top of that: + assert_equal(node.getmempoolinfo()[ + 'mempoolminfee'], Decimal('0.00001495')) + + # We can broadcast a regular tx. + txid2 = node.sendtoaddress(node.getnewaddress(), 0.001) + + # A regular 1 sat/byte transaction can't get in now. + spendfrom = self.spendable_outputs.pop() + assert_raises_rpc_error(-26, "mempool min fee not met", node.sendrawtransaction, + ToHex(create_var_transaction(spendfrom, b'', 500, 500))) + + settxids = set([txid1, txid2]) + + # We can broadcast a bunch of regular txes, they just need to pay a bit more fee (1.5 sat/byte). + # They will push out many of the high-sigops transactions. + for i in range(15): + spendfrom = self.spendable_outputs.pop() + ctx = create_var_transaction(spendfrom, b'', 100000, 150000) + settxids.add(node.sendrawtransaction(ToHex(ctx))) + + [lastblockhash, ] = node.generate(1) + blocktxes = set(node.getblock(lastblockhash, 1)['tx']) + assert settxids.issubset(blocktxes) + + +if __name__ == '__main__': + MempoolLimitSigopsTest().main()