From c82a4e9a63a28fc8c482c7c8e5b7bfcc51a6805a Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Sat, 20 Feb 2016 21:02:44 -0500 Subject: [PATCH 1/2] Use ancestor-feerate based transaction selection for mining Includes changes by Pieter Wuille --- src/miner.cpp | 206 +++++++++++++++++++++++++++++++++++++++++++++++++- src/miner.h | 118 +++++++++++++++++++++++++++++ 2 files changed, 323 insertions(+), 1 deletion(-) diff --git a/src/miner.cpp b/src/miner.cpp index 99eb0a2ebd..989ad11a26 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -25,6 +25,7 @@ #include "utilmoneystr.h" #include "validationinterface.h" +#include #include #include #include @@ -134,7 +135,7 @@ CBlockTemplate* BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn) : pblock->GetBlockTime(); addPriorityTxs(); - addScoreTxs(); + addPackageTxs(); nLastBlockTx = nBlockTx; nLastBlockSize = nBlockSize; @@ -177,7 +178,38 @@ bool BlockAssembler::isStillDependent(CTxMemPool::txiter iter) return false; } +void BlockAssembler::onlyUnconfirmed(CTxMemPool::setEntries& testSet) +{ + for (CTxMemPool::setEntries::iterator iit = testSet.begin(); iit != testSet.end(); ) { + // Only test txs not already in the block + if (inBlock.count(*iit)) { + testSet.erase(iit++); + } + else { + iit++; + } + } +} + +bool BlockAssembler::TestPackage(uint64_t packageSize, unsigned int packageSigOps) +{ + if (nBlockSize + packageSize >= nBlockMaxSize) + return false; + if (nBlockSigOps + packageSigOps >= MAX_BLOCK_SIGOPS) + return false; + return true; +} +// Block size and sigops have already been tested. Check that all transactions +// are final. +bool BlockAssembler::TestPackageFinality(const CTxMemPool::setEntries& package) +{ + BOOST_FOREACH (const CTxMemPool::txiter it, package) { + if (!IsFinalTx(it->GetTx(), nHeight, nLockTimeCutoff)) + return false; + } + return true; +} bool BlockAssembler::TestForBlock(CTxMemPool::txiter iter) { @@ -297,6 +329,178 @@ void BlockAssembler::addScoreTxs() } } +void BlockAssembler::UpdatePackagesForAdded(const CTxMemPool::setEntries& alreadyAdded, + indexed_modified_transaction_set &mapModifiedTx) +{ + BOOST_FOREACH(const CTxMemPool::txiter it, alreadyAdded) { + CTxMemPool::setEntries descendants; + mempool.CalculateDescendants(it, descendants); + // Insert all descendants (not yet in block) into the modified set + BOOST_FOREACH(CTxMemPool::txiter desc, descendants) { + if (alreadyAdded.count(desc)) + continue; + modtxiter mit = mapModifiedTx.find(desc); + if (mit == mapModifiedTx.end()) { + CTxMemPoolModifiedEntry modEntry(desc); + modEntry.nSizeWithAncestors -= it->GetTxSize(); + modEntry.nModFeesWithAncestors -= it->GetModifiedFee(); + modEntry.nSigOpCountWithAncestors -= it->GetSigOpCount(); + mapModifiedTx.insert(modEntry); + } else { + mapModifiedTx.modify(mit, update_for_parent_inclusion(it)); + } + } + } +} + +// Skip entries in mapTx that are already in a block or are present +// in mapModifiedTx (which implies that the mapTx ancestor state is +// stale due to ancestor inclusion in the block) +// Also skip transactions that we've already failed to add. This can happen if +// we consider a transaction in mapModifiedTx and it fails: we can then +// potentially consider it again while walking mapTx. It's currently +// guaranteed to fail again, but as a belt-and-suspenders check we put it in +// failedTx and avoid re-evaluation, since the re-evaluation would be using +// cached size/sigops/fee values that are not actually correct. +bool BlockAssembler::SkipMapTxEntry(CTxMemPool::txiter it, indexed_modified_transaction_set &mapModifiedTx, CTxMemPool::setEntries &failedTx) +{ + assert (it != mempool.mapTx.end()); + if (mapModifiedTx.count(it) || inBlock.count(it) || failedTx.count(it)) + return true; + return false; +} + +void BlockAssembler::SortForBlock(const CTxMemPool::setEntries& package, CTxMemPool::txiter entry, std::vector& sortedEntries) +{ + // Sort package by ancestor count + // If a transaction A depends on transaction B, then A's ancestor count + // must be greater than B's. So this is sufficient to validly order the + // transactions for block inclusion. + sortedEntries.clear(); + sortedEntries.insert(sortedEntries.begin(), package.begin(), package.end()); + std::sort(sortedEntries.begin(), sortedEntries.end(), CompareTxIterByAncestorCount()); +} + +// This transaction selection algorithm orders the mempool based +// on feerate of a transaction including all unconfirmed ancestors. +// Since we don't remove transactions from the mempool as we select them +// for block inclusion, we need an alternate method of updating the feerate +// of a transaction with its not-yet-selected ancestors as we go. +// This is accomplished by walking the in-mempool descendants of selected +// transactions and storing a temporary modified state in mapModifiedTxs. +// Each time through the loop, we compare the best transaction in +// mapModifiedTxs with the next transaction in the mempool to decide what +// transaction package to work on next. +void BlockAssembler::addPackageTxs() +{ + // mapModifiedTx will store sorted packages after they are modified + // because some of their txs are already in the block + indexed_modified_transaction_set mapModifiedTx; + // Keep track of entries that failed inclusion, to avoid duplicate work + CTxMemPool::setEntries failedTx; + + // Start by adding all descendants of previously added txs to mapModifiedTx + // and modifying them for their already included ancestors + UpdatePackagesForAdded(inBlock, mapModifiedTx); + + CTxMemPool::indexed_transaction_set::index::type::iterator mi = mempool.mapTx.get().begin(); + CTxMemPool::txiter iter; + while (mi != mempool.mapTx.get().end() || !mapModifiedTx.empty()) + { + // First try to find a new transaction in mapTx to evaluate. + if (mi != mempool.mapTx.get().end() && + SkipMapTxEntry(mempool.mapTx.project<0>(mi), mapModifiedTx, failedTx)) { + ++mi; + continue; + } + + // Now that mi is not stale, determine which transaction to evaluate: + // the next entry from mapTx, or the best from mapModifiedTx? + bool fUsingModified = false; + + modtxscoreiter modit = mapModifiedTx.get().begin(); + if (mi == mempool.mapTx.get().end()) { + // We're out of entries in mapTx; use the entry from mapModifiedTx + iter = modit->iter; + fUsingModified = true; + } else { + // Try to compare the mapTx entry to the mapModifiedTx entry + iter = mempool.mapTx.project<0>(mi); + if (modit != mapModifiedTx.get().end() && + CompareModifiedEntry()(*modit, CTxMemPoolModifiedEntry(iter))) { + // The best entry in mapModifiedTx has higher score + // than the one from mapTx. + // Switch which transaction (package) to consider + iter = modit->iter; + fUsingModified = true; + } else { + // Either no entry in mapModifiedTx, or it's worse than mapTx. + // Increment mi for the next loop iteration. + ++mi; + } + } + + // We skip mapTx entries that are inBlock, and mapModifiedTx shouldn't + // contain anything that is inBlock. + assert(!inBlock.count(iter)); + + uint64_t packageSize = iter->GetSizeWithAncestors(); + CAmount packageFees = iter->GetModFeesWithAncestors(); + unsigned int packageSigOps = iter->GetSigOpCountWithAncestors(); + if (fUsingModified) { + packageSize = modit->nSizeWithAncestors; + packageFees = modit->nModFeesWithAncestors; + packageSigOps = modit->nSigOpCountWithAncestors; + } + + if (packageFees < ::minRelayTxFee.GetFee(packageSize) && nBlockSize >= nBlockMinSize) { + // Everything else we might consider has a lower fee rate + return; + } + + if (!TestPackage(packageSize, packageSigOps)) { + 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; + } + + CTxMemPool::setEntries ancestors; + uint64_t nNoLimit = std::numeric_limits::max(); + std::string dummy; + mempool.CalculateMemPoolAncestors(*iter, ancestors, nNoLimit, nNoLimit, nNoLimit, nNoLimit, dummy, false); + + onlyUnconfirmed(ancestors); + ancestors.insert(iter); + + // Test if all tx's are Final + if (!TestPackageFinality(ancestors)) { + if (fUsingModified) { + mapModifiedTx.get().erase(modit); + failedTx.insert(iter); + } + continue; + } + + // Package can be added. Sort the entries in a valid order. + vector sortedEntries; + SortForBlock(ancestors, iter, sortedEntries); + + for (size_t i=0; i #include +#include "boost/multi_index_container.hpp" +#include "boost/multi_index/ordered_index.hpp" class CBlockIndex; class CChainParams; @@ -29,6 +31,104 @@ struct CBlockTemplate std::vector vTxSigOps; }; +// Container for tracking updates to ancestor feerate as we include (parent) +// transactions in a block +struct CTxMemPoolModifiedEntry { + CTxMemPoolModifiedEntry(CTxMemPool::txiter entry) + { + iter = entry; + nSizeWithAncestors = entry->GetSizeWithAncestors(); + nModFeesWithAncestors = entry->GetModFeesWithAncestors(); + nSigOpCountWithAncestors = entry->GetSigOpCountWithAncestors(); + } + + CTxMemPool::txiter iter; + uint64_t nSizeWithAncestors; + CAmount nModFeesWithAncestors; + unsigned int nSigOpCountWithAncestors; +}; + +/** Comparator for CTxMemPool::txiter objects. + * It simply compares the internal memory address of the CTxMemPoolEntry object + * pointed to. This means it has no meaning, and is only useful for using them + * as key in other indexes. + */ +struct CompareCTxMemPoolIter { + bool operator()(const CTxMemPool::txiter& a, const CTxMemPool::txiter& b) const + { + return &(*a) < &(*b); + } +}; + +struct modifiedentry_iter { + typedef CTxMemPool::txiter result_type; + result_type operator() (const CTxMemPoolModifiedEntry &entry) const + { + return entry.iter; + } +}; + +// This matches the calculation in CompareTxMemPoolEntryByAncestorFee, +// except operating on CTxMemPoolModifiedEntry. +// TODO: refactor to avoid duplication of this logic. +struct CompareModifiedEntry { + bool operator()(const CTxMemPoolModifiedEntry &a, const CTxMemPoolModifiedEntry &b) + { + double f1 = (double)a.nModFeesWithAncestors * b.nSizeWithAncestors; + double f2 = (double)b.nModFeesWithAncestors * a.nSizeWithAncestors; + if (f1 == f2) { + return CTxMemPool::CompareIteratorByHash()(a.iter, b.iter); + } + return f1 > f2; + } +}; + +// A comparator that sorts transactions based on number of ancestors. +// This is sufficient to sort an ancestor package in an order that is valid +// to appear in a block. +struct CompareTxIterByAncestorCount { + bool operator()(const CTxMemPool::txiter &a, const CTxMemPool::txiter &b) + { + if (a->GetCountWithAncestors() != b->GetCountWithAncestors()) + return a->GetCountWithAncestors() < b->GetCountWithAncestors(); + return CTxMemPool::CompareIteratorByHash()(a, b); + } +}; + +typedef boost::multi_index_container< + CTxMemPoolModifiedEntry, + boost::multi_index::indexed_by< + boost::multi_index::ordered_unique< + modifiedentry_iter, + CompareCTxMemPoolIter + >, + // sorted by modified ancestor fee rate + boost::multi_index::ordered_non_unique< + // Reuse same tag from CTxMemPool's similar index + boost::multi_index::tag, + boost::multi_index::identity, + CompareModifiedEntry + > + > +> indexed_modified_transaction_set; + +typedef indexed_modified_transaction_set::nth_index<0>::type::iterator modtxiter; +typedef indexed_modified_transaction_set::index::type::iterator modtxscoreiter; + +struct update_for_parent_inclusion +{ + update_for_parent_inclusion(CTxMemPool::txiter it) : iter(it) {} + + void operator() (CTxMemPoolModifiedEntry &e) + { + e.nModFeesWithAncestors -= iter->GetFee(); + e.nSizeWithAncestors -= iter->GetTxSize(); + e.nSigOpCountWithAncestors -= iter->GetSigOpCount(); + } + + CTxMemPool::txiter iter; +}; + /** Generate a new block, without valid proof-of-work */ class BlockAssembler { @@ -74,12 +174,30 @@ private: void addScoreTxs(); /** Add transactions based on tx "priority" */ void addPriorityTxs(); + /** Add transactions based on feerate including unconfirmed ancestors */ + void addPackageTxs(); // helper function for addScoreTxs and addPriorityTxs /** Test if tx will still "fit" in the block */ bool TestForBlock(CTxMemPool::txiter iter); /** Test if tx still has unconfirmed parents not yet in block */ bool isStillDependent(CTxMemPool::txiter iter); + + // helper functions for addPackageTxs() + /** Remove confirmed (inBlock) entries from given set */ + void onlyUnconfirmed(CTxMemPool::setEntries& testSet); + /** Test if a new package would "fit" in the block */ + bool TestPackage(uint64_t packageSize, unsigned int packageSigOps); + /** Test if a set of transactions are all final */ + bool TestPackageFinality(const CTxMemPool::setEntries& package); + /** Return true if given transaction from mapTx has already been evaluated, + * or if the transaction's cached data in mapTx is incorrect. */ + bool SkipMapTxEntry(CTxMemPool::txiter it, indexed_modified_transaction_set &mapModifiedTx, CTxMemPool::setEntries &failedTx); + /** Sort the package in an order that is valid to appear in a block */ + void SortForBlock(const CTxMemPool::setEntries& package, CTxMemPool::txiter entry, std::vector& sortedEntries); + /** Add descendants of given transactions to mapModifiedTx with ancestor + * state updated assuming given transactions are inBlock. */ + void UpdatePackagesForAdded(const CTxMemPool::setEntries& alreadyAdded, indexed_modified_transaction_set &mapModifiedTx); }; /** Modify the extranonce in a block */ From 29fac19c93fabfed4163ee9ffa85f9188c9ee6ac Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Sat, 20 Feb 2016 20:58:23 -0500 Subject: [PATCH 2/2] Add unit tests for ancestor feerate mining --- src/test/miner_tests.cpp | 109 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 109 insertions(+) diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index 3fb7967881..ca8d6d2e05 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -71,6 +71,113 @@ bool TestSequenceLocks(const CTransaction &tx, int flags) return CheckSequenceLocks(tx, flags); } +// Test suite for ancestor feerate transaction selection. +// Implemented as an additional function, rather than a separate test case, +// to allow reusing the blockchain created in CreateNewBlock_validity. +// Note that this test assumes blockprioritysize is 0. +void TestPackageSelection(const CChainParams& chainparams, CScript scriptPubKey, std::vector& txFirst) +{ + // Test the ancestor feerate transaction selection. + TestMemPoolEntryHelper entry; + + // Test that a medium fee transaction will be selected after a higher fee + // rate package with a low fee rate parent. + CMutableTransaction tx; + tx.vin.resize(1); + tx.vin[0].scriptSig = CScript() << OP_1; + tx.vin[0].prevout.hash = txFirst[0]->GetHash(); + tx.vin[0].prevout.n = 0; + tx.vout.resize(1); + tx.vout[0].nValue = 5000000000LL - 1000; + // This tx has a low fee: 1000 satoshis + uint256 hashParentTx = tx.GetHash(); // save this txid for later use + mempool.addUnchecked(hashParentTx, entry.Fee(1000).Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); + + // This tx has a medium fee: 10000 satoshis + tx.vin[0].prevout.hash = txFirst[1]->GetHash(); + tx.vout[0].nValue = 5000000000LL - 10000; + uint256 hashMediumFeeTx = tx.GetHash(); + mempool.addUnchecked(hashMediumFeeTx, entry.Fee(10000).Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); + + // This tx has a high fee, but depends on the first transaction + tx.vin[0].prevout.hash = hashParentTx; + tx.vout[0].nValue = 5000000000LL - 1000 - 50000; // 50k satoshi fee + uint256 hashHighFeeTx = tx.GetHash(); + mempool.addUnchecked(hashHighFeeTx, entry.Fee(50000).Time(GetTime()).SpendsCoinbase(false).FromTx(tx)); + + CBlockTemplate *pblocktemplate = BlockAssembler(chainparams).CreateNewBlock(scriptPubKey); + BOOST_CHECK(pblocktemplate->block.vtx[1].GetHash() == hashParentTx); + BOOST_CHECK(pblocktemplate->block.vtx[2].GetHash() == hashHighFeeTx); + BOOST_CHECK(pblocktemplate->block.vtx[3].GetHash() == hashMediumFeeTx); + + // Test that a package below the min relay fee doesn't get included + tx.vin[0].prevout.hash = hashHighFeeTx; + tx.vout[0].nValue = 5000000000LL - 1000 - 50000; // 0 fee + uint256 hashFreeTx = tx.GetHash(); + mempool.addUnchecked(hashFreeTx, entry.Fee(0).FromTx(tx)); + size_t freeTxSize = ::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION); + + // Calculate a fee on child transaction that will put the package just + // below the min relay fee (assuming 1 child tx of the same size). + CAmount feeToUse = minRelayTxFee.GetFee(2*freeTxSize) - 1; + + tx.vin[0].prevout.hash = hashFreeTx; + tx.vout[0].nValue = 5000000000LL - 1000 - 50000 - feeToUse; + uint256 hashLowFeeTx = tx.GetHash(); + mempool.addUnchecked(hashLowFeeTx, entry.Fee(feeToUse).FromTx(tx)); + pblocktemplate = BlockAssembler(chainparams).CreateNewBlock(scriptPubKey); + // Verify that the free tx and the low fee tx didn't get selected + for (size_t i=0; iblock.vtx.size(); ++i) { + BOOST_CHECK(pblocktemplate->block.vtx[i].GetHash() != hashFreeTx); + BOOST_CHECK(pblocktemplate->block.vtx[i].GetHash() != hashLowFeeTx); + } + + // Test that packages above the min relay fee do get included, even if one + // of the transactions is below the min relay fee + // Remove the low fee transaction and replace with a higher fee transaction + std::list dummy; + mempool.removeRecursive(tx, dummy); + tx.vout[0].nValue -= 2; // Now we should be just over the min relay fee + hashLowFeeTx = tx.GetHash(); + mempool.addUnchecked(hashLowFeeTx, entry.Fee(feeToUse+2).FromTx(tx)); + pblocktemplate = BlockAssembler(chainparams).CreateNewBlock(scriptPubKey); + BOOST_CHECK(pblocktemplate->block.vtx[4].GetHash() == hashFreeTx); + BOOST_CHECK(pblocktemplate->block.vtx[5].GetHash() == hashLowFeeTx); + + // Test that transaction selection properly updates ancestor fee + // calculations as ancestor transactions get included in a block. + // Add a 0-fee transaction that has 2 outputs. + tx.vin[0].prevout.hash = txFirst[2]->GetHash(); + tx.vout.resize(2); + tx.vout[0].nValue = 5000000000LL - 100000000; + tx.vout[1].nValue = 100000000; // 1BTC output + uint256 hashFreeTx2 = tx.GetHash(); + mempool.addUnchecked(hashFreeTx2, entry.Fee(0).SpendsCoinbase(true).FromTx(tx)); + + // This tx can't be mined by itself + tx.vin[0].prevout.hash = hashFreeTx2; + tx.vout.resize(1); + feeToUse = minRelayTxFee.GetFee(freeTxSize); + tx.vout[0].nValue = 5000000000LL - 100000000 - feeToUse; + uint256 hashLowFeeTx2 = tx.GetHash(); + mempool.addUnchecked(hashLowFeeTx2, entry.Fee(feeToUse).SpendsCoinbase(false).FromTx(tx)); + pblocktemplate = BlockAssembler(chainparams).CreateNewBlock(scriptPubKey); + + // Verify that this tx isn't selected. + for (size_t i=0; iblock.vtx.size(); ++i) { + BOOST_CHECK(pblocktemplate->block.vtx[i].GetHash() != hashFreeTx2); + BOOST_CHECK(pblocktemplate->block.vtx[i].GetHash() != hashLowFeeTx2); + } + + // This tx will be mineable, and should cause hashLowFeeTx2 to be selected + // as well. + tx.vin[0].prevout.n = 1; + tx.vout[0].nValue = 100000000 - 10000; // 10k satoshi fee + mempool.addUnchecked(tx.GetHash(), entry.Fee(10000).FromTx(tx)); + pblocktemplate = BlockAssembler(chainparams).CreateNewBlock(scriptPubKey); + BOOST_CHECK(pblocktemplate->block.vtx[8].GetHash() == hashLowFeeTx2); +} + // NOTE: These tests rely on CreateNewBlock doing its own self-validation! BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) { @@ -385,6 +492,8 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) SetMockTime(0); mempool.clear(); + TestPackageSelection(chainparams, scriptPubKey, txFirst); + BOOST_FOREACH(CTransaction *_tx, txFirst) delete _tx;