diff --git a/src/miner.cpp b/src/miner.cpp --- a/src/miner.cpp +++ b/src/miner.cpp @@ -455,8 +455,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,13 @@ 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 +515,13 @@ 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 @@ -701,6 +701,7 @@ CTxMemPoolEntry entry(ptx, nFees, nAcceptTime, chainActive.Height(), fSpendsCoinbase, nSigOpsCount, lp); unsigned int nSize = entry.GetTxSize(); + unsigned int nVirtualSize = entry.GetTxVirtualSize(); // No transactions are allowed below minRelayTxFee except from // disconnected blocks. @@ -715,7 +716,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-sigops-mempool-mining.py b/test/functional/abc-sigops-mempool-mining.py new file mode 100644 --- /dev/null +++ b/test/functional/abc-sigops-mempool-mining.py @@ -0,0 +1,320 @@ +#!/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 and mining priorities using high-sigops transactions, probing +how the virtualsize mechanism behaves. + +This test assumes: + -bytespersigop default is 50 + -blockmintxfee default is 1000 + -minrelaytxfee default is 1000 + -MEMPOOL_FULL_FEE_INCREMENT is 1000 + +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 + +SATOSHI = Decimal('0.00000001') + + +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 + + ctx.calc_sha256() + 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"]] + 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(600): + 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( + 'PART 1: testing of priority under congestion (mempool eviction priority and mining priority).') + + self.log.info( + "Put a regular transaction in mempool at 1 sat/byte.") + txid1 = node.sendtoaddress(node.getnewaddress(), 0.001) + + self.log.info( + "Mempool accepts 1 sat/byte txes even with very many sigops") + script_4000_sigops = bytes([OP_CHECKMULTISIG] * 200) + # vsize = 200000, so 0.0015 sat/vbyte. + ctx = create_var_transaction( + self.spendable_outputs.popleft(), script_4000_sigops, 300, 300) + txid_parent = node.sendrawtransaction(ToHex(ctx)) + + self.log.info( + "Create a 1 sat/byte compensating child of a high-sigop parent") + # package vsize = 200000, so 0.5015 sat/vbyte. + ctx_child = create_var_transaction( + ctx, b'', 100000, 100000) + txid_child = node.sendrawtransaction(ToHex(ctx_child)) + + # make one with 270 sat/byte, but only 0.4 sat/vbyte. + ctx = create_var_transaction( + self.spendable_outputs.popleft(), script_4000_sigops, 300, 80000) + txid_highsigops = node.sendrawtransaction(ToHex(ctx)) + + self.log.info( + "Flood the mempool with 10-kB transactions @ 9.9 sat/byte and 0.495 sat/vbyte, until it is full") + # About 500 should fill up our 5 MB mempool. + # Note that even starting from an empty pool, somewhat fewer than 500 + # would be accepted depending on mempool data structure sizes. + for i in range(500): + spendfrom = self.spendable_outputs.popleft() + 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") + + self.log.info( + 'The flooding only caused eviction of lower low sat/vbyte packages') + + mempool_txids = set(node.getrawmempool()) + + # The 9.9 sat/byte txes caused the 270 sat/byte tx to get evicted, because + # the latter was more dense in sigops. + assert txid_highsigops not in mempool_txids + + # Other txes did get kept, including txid_parent which had ultralow + # virtual feerate (but its child supplied enough fee). + settxids = set([txid1, txid_parent, txid_child]) + assert_equal(settxids.difference(mempool_txids), set()) + + self.log.info('Mempool fee floor has jumped to 1.495 sat/vbyte') + # 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')) + + self.log.info( + 'Broadcasting a regular tx still works, because wallet knows what fee to use.') + txid2 = node.sendtoaddress(node.getnewaddress(), 0.001) + settxids.add(txid2) + + self.log.info( + "But, a regular 1 sat/byte transaction can't get in now.") + spendfrom = self.spendable_outputs.popleft() + assert_raises_rpc_error(-26, "mempool min fee not met", node.sendrawtransaction, + ToHex(create_var_transaction(spendfrom, b'', 500, 500))) + + self.log.info( + "Broadcasting regular transactions will push out the high-sigops txns.") + # We can broadcast a bunch of regular txes. They need to pay a bit more fee (1.5 sat/vbyte) than the floor. + for i in range(15): + spendfrom = self.spendable_outputs.popleft() + ctx = create_var_transaction(spendfrom, b'', 100000, 150000) + settxids.add(node.sendrawtransaction(ToHex(ctx))) + + self.log.info("Mining picks all the 'good' txes first") + # These slightly higher-fee transactions also have more priority. + [lastblockhash, ] = node.generate(1) + blocktxes = set(node.getblock(lastblockhash, 1)['tx']) + assert_equal(settxids.difference(blocktxes), set()) + # there are still hundreds of the flooded txes left in mempool. + assert len(node.getrawmempool()) > 200 + + self.log.info( + 'PART 2: The following tests focus on mineability according to blockmintxfee.') + + self.log.info('Clear the mempool by mining out everything.') + node.generate(100) + assert_equal(node.getrawmempool(), []) + + self.log.info( + 'Reset the mempool fee floor (currently, restarting the node achieves this).') + # could also be done by setting mocktime in future (the floor decays over a period of hours) + self.restart_node(0, self.extra_args[0]) + (node,) = self.nodes + node.add_p2p_connection(P2PDataStore()) + assert_equal(node.getmempoolinfo()[ + 'mempoolminfee'], Decimal('0.00001000')) + + self.log.info( + 'Get a 0.998 sat/byte transaction into mempool by having it mined then reorged out.') + self.log.info(node.getbestblockhash()) + tip = self.getbestblock(node) + self.log.info(tip.hash) + ctx = create_var_transaction( + self.spendable_outputs.popleft(), b'', 1000, 998) + stucktxid = ctx.hash + # check that indeed it can't get in normally: + assert_raises_rpc_error(-26, "min relay fee not met", + node.sendrawtransaction, ToHex(ctx)) + block = self.build_block(tip, [ctx]) + node.p2p.send_blocks_and_test([block], node, success=True) + node.invalidateblock(block.hash) + assert stucktxid in node.getrawmempool(), "should be returned to pool" + + self.log.info( + "Since blockmintxfee is 1 sat/byte, that 0.998 sat/byte tx won't get mined by default, and is stuck permanently.") + node.generate(1) + assert stucktxid in node.getrawmempool(), "should be stuck" + + self.log.info( + 'Get wallet to make an normal tx sending money to 12 people, which is just under 1 sat/vbyte.') + txid = node.sendmany( + "", {node.getnewaddress(): 0.01 for _ in range(12)}) + + # analyze the constructed tx just to make sure it's as expected + ctx = FromHex(CTransaction(), node.getrawtransaction(txid)) + assert len(ctx.vin) == 1, "only a single input should be needed" + assert len(ctx.vout) == 13, "12 recipients and 1 change" + # tx size should be ~ 599 bytes and fees should be 599 sats. + # 13 sigops and 50 sigops / byte = 650 vbytes. + txsize = len(ctx.serialize()) + txvsize = max(txsize, 50 * len(ctx.vout)) + assert txvsize > txsize, "transactions is dense-sigops" + fee_sats = node.getmempoolentry(txid)['fee'] / SATOSHI + assert_equal(fee_sats, 599) + assert fee_sats / txsize >= 1.0, "just over 1 sat/byte" + assert fee_sats / txvsize < 0.95, "just under 1 sat/vbyte" + + self.log.info( + "Even though another transaction is stuck permanently, it should not prevent a normal tx from being mined.") + [bhash, ] = node.generate(1) + blocktxes = set(node.getblock(bhash, 1)['tx']) + assert txid in blocktxes, "normal tx is not stuck" + assert stucktxid not in blocktxes, "lowfee tx is still stuck" + + +if __name__ == '__main__': + MempoolLimitSigopsTest().main()