From c49d5bc9e6c97c47c0bd78604b2c393a7e4af097 Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Mon, 26 Oct 2015 11:08:46 -0400 Subject: [PATCH 1/6] Store the total sig op count of a tx. Store sum of legacy and P2SH sig op counts. This is calculated in AcceptToMemory pool and storing it saves redoing the expensive calculation in block template creation. --- src/main.cpp | 2 +- src/test/test_bitcoin.cpp | 2 +- src/test/test_bitcoin.h | 8 +++++--- src/txmempool.cpp | 4 ++-- src/txmempool.h | 5 ++++- 5 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index e3c77e850..fb6d32b2f 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -964,7 +964,7 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa } } - CTxMemPoolEntry entry(tx, nFees, GetTime(), dPriority, chainActive.Height(), pool.HasNoInputsOf(tx), inChainInputValue, fSpendsCoinbase); + CTxMemPoolEntry entry(tx, nFees, GetTime(), dPriority, chainActive.Height(), pool.HasNoInputsOf(tx), inChainInputValue, fSpendsCoinbase, nSigOps); unsigned int nSize = entry.GetTxSize(); // Don't accept it if it can't get into a block diff --git a/src/test/test_bitcoin.cpp b/src/test/test_bitcoin.cpp index 9645c7c94..2147dbb06 100644 --- a/src/test/test_bitcoin.cpp +++ b/src/test/test_bitcoin.cpp @@ -150,7 +150,7 @@ CTxMemPoolEntry TestMemPoolEntryHelper::FromTx(CMutableTransaction &tx, CTxMemPo CAmount inChainValue = hasNoDependencies ? txn.GetValueOut() : 0; return CTxMemPoolEntry(txn, nFee, nTime, dPriority, nHeight, - hasNoDependencies, inChainValue, spendsCoinbase); + hasNoDependencies, inChainValue, spendsCoinbase, sigOpCount); } void Shutdown(void* parg) diff --git a/src/test/test_bitcoin.h b/src/test/test_bitcoin.h index 343c27673..273bfdd7f 100644 --- a/src/test/test_bitcoin.h +++ b/src/test/test_bitcoin.h @@ -66,11 +66,12 @@ struct TestMemPoolEntryHelper unsigned int nHeight; bool hadNoDependencies; bool spendsCoinbase; - + unsigned int sigOpCount; + TestMemPoolEntryHelper() : nFee(0), nTime(0), dPriority(0.0), nHeight(1), - hadNoDependencies(false), spendsCoinbase(false) { } - + hadNoDependencies(false), spendsCoinbase(false), sigOpCount(1) { } + CTxMemPoolEntry FromTx(CMutableTransaction &tx, CTxMemPool *pool = NULL); // Change the default value @@ -80,5 +81,6 @@ struct TestMemPoolEntryHelper TestMemPoolEntryHelper &Height(unsigned int _height) { nHeight = _height; return *this; } TestMemPoolEntryHelper &HadNoDependencies(bool _hnd) { hadNoDependencies = _hnd; return *this; } TestMemPoolEntryHelper &SpendsCoinbase(bool _flag) { spendsCoinbase = _flag; return *this; } + TestMemPoolEntryHelper &SigOps(unsigned int _sigops) { sigOpCount = _sigops; return *this; } }; #endif diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 9d2513948..fa62cbf16 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -22,10 +22,10 @@ using namespace std; CTxMemPoolEntry::CTxMemPoolEntry(const CTransaction& _tx, const CAmount& _nFee, int64_t _nTime, double _entryPriority, unsigned int _entryHeight, bool poolHasNoInputsOf, CAmount _inChainInputValue, - bool _spendsCoinbase): + bool _spendsCoinbase, unsigned int _sigOps): tx(_tx), nFee(_nFee), nTime(_nTime), entryPriority(_entryPriority), entryHeight(_entryHeight), hadNoDependencies(poolHasNoInputsOf), inChainInputValue(_inChainInputValue), - spendsCoinbase(_spendsCoinbase) + spendsCoinbase(_spendsCoinbase), sigOpCount(_sigOps) { nTxSize = ::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION); nModSize = tx.CalculateModifiedSize(nTxSize); diff --git a/src/txmempool.h b/src/txmempool.h index c4ea51557..334b54782 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -68,6 +68,7 @@ private: bool hadNoDependencies; //! Not dependent on any other txs when it entered the mempool CAmount inChainInputValue; //! Sum of all txin values that are already in blockchain bool spendsCoinbase; //! keep track of transactions that spend a coinbase + unsigned int sigOpCount; //! Legacy sig ops plus P2SH sig op count // Information about descendants of this transaction that are in the // mempool; if we remove this transaction we must remove all of these @@ -81,7 +82,8 @@ private: public: CTxMemPoolEntry(const CTransaction& _tx, const CAmount& _nFee, int64_t _nTime, double _entryPriority, unsigned int _entryHeight, - bool poolHasNoInputsOf, CAmount _inChainInputValue, bool spendsCoinbase); + bool poolHasNoInputsOf, CAmount _inChainInputValue, bool spendsCoinbase, + unsigned int nSigOps); CTxMemPoolEntry(const CTxMemPoolEntry& other); const CTransaction& GetTx() const { return this->tx; } @@ -95,6 +97,7 @@ public: int64_t GetTime() const { return nTime; } unsigned int GetHeight() const { return entryHeight; } bool WasClearAtEntry() const { return hadNoDependencies; } + unsigned int GetSigOpCount() const { return sigOpCount; } size_t DynamicMemoryUsage() const { return nUsageSize; } // Adjusts the descendant state, if this entry is not dirty. From f3fe83673e84ef4d20b3026faa397cad17212ff8 Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Mon, 26 Oct 2015 14:06:06 -0400 Subject: [PATCH 2/6] Add a score index to the mempool. The score index is meant to represent the order of priority for being included in a block for miners. Initially this is set to the transactions modified (by any feeDelta) fee rate. Index improvements and unit tests by sdaftuar. --- src/rpcblockchain.cpp | 2 ++ src/test/mempool_tests.cpp | 52 +++++++++++++++++++++++++++++++------- src/txmempool.cpp | 24 ++++++++++++++++-- src/txmempool.h | 40 ++++++++++++++++++++++++++++- 4 files changed, 106 insertions(+), 12 deletions(-) diff --git a/src/rpcblockchain.cpp b/src/rpcblockchain.cpp index c87282275..aede79753 100644 --- a/src/rpcblockchain.cpp +++ b/src/rpcblockchain.cpp @@ -190,6 +190,7 @@ UniValue mempoolToJSON(bool fVerbose = false) UniValue info(UniValue::VOBJ); info.push_back(Pair("size", (int)e.GetTxSize())); info.push_back(Pair("fee", ValueFromAmount(e.GetFee()))); + info.push_back(Pair("modifiedfee", ValueFromAmount(e.GetModifiedFee()))); info.push_back(Pair("time", e.GetTime())); info.push_back(Pair("height", (int)e.GetHeight())); info.push_back(Pair("startingpriority", e.GetPriority(e.GetHeight()))); @@ -247,6 +248,7 @@ UniValue getrawmempool(const UniValue& params, bool fHelp) " \"transactionid\" : { (json object)\n" " \"size\" : n, (numeric) transaction size in bytes\n" " \"fee\" : n, (numeric) transaction fee in " + CURRENCY_UNIT + "\n" + " \"modifiedfee\" : n, (numeric) transaction fee with fee deltas used for mining priority\n" " \"time\" : n, (numeric) local time transaction entered pool in seconds since 1 Jan 1970 GMT\n" " \"height\" : n, (numeric) block height when transaction entered pool\n" " \"startingpriority\" : n, (numeric) priority when transaction entered pool\n" diff --git a/src/test/mempool_tests.cpp b/src/test/mempool_tests.cpp index 896e1237e..e9f7378f7 100644 --- a/src/test/mempool_tests.cpp +++ b/src/test/mempool_tests.cpp @@ -102,12 +102,13 @@ BOOST_AUTO_TEST_CASE(MempoolRemoveTest) removed.clear(); } +template void CheckSort(CTxMemPool &pool, std::vector &sortedOrder) { BOOST_CHECK_EQUAL(pool.size(), sortedOrder.size()); - CTxMemPool::indexed_transaction_set::nth_index<1>::type::iterator it = pool.mapTx.get<1>().begin(); + typename CTxMemPool::indexed_transaction_set::nth_index::type::iterator it = pool.mapTx.get().begin(); int count=0; - for (; it != pool.mapTx.get<1>().end(); ++it, ++count) { + for (; it != pool.mapTx.get().end(); ++it, ++count) { BOOST_CHECK_EQUAL(it->GetTx().GetHash().ToString(), sortedOrder[count]); } } @@ -163,7 +164,7 @@ BOOST_AUTO_TEST_CASE(MempoolIndexingTest) sortedOrder[2] = tx1.GetHash().ToString(); // 10000 sortedOrder[3] = tx4.GetHash().ToString(); // 15000 sortedOrder[4] = tx2.GetHash().ToString(); // 20000 - CheckSort(pool, sortedOrder); + CheckSort<1>(pool, sortedOrder); /* low fee but with high fee child */ /* tx6 -> tx7 -> tx8, tx9 -> tx10 */ @@ -175,7 +176,7 @@ BOOST_AUTO_TEST_CASE(MempoolIndexingTest) BOOST_CHECK_EQUAL(pool.size(), 6); // Check that at this point, tx6 is sorted low sortedOrder.insert(sortedOrder.begin(), tx6.GetHash().ToString()); - CheckSort(pool, sortedOrder); + CheckSort<1>(pool, sortedOrder); CTxMemPool::setEntries setAncestors; setAncestors.insert(pool.mapTx.find(tx6.GetHash())); @@ -201,7 +202,7 @@ BOOST_AUTO_TEST_CASE(MempoolIndexingTest) sortedOrder.erase(sortedOrder.begin()); sortedOrder.push_back(tx6.GetHash().ToString()); sortedOrder.push_back(tx7.GetHash().ToString()); - CheckSort(pool, sortedOrder); + CheckSort<1>(pool, sortedOrder); /* low fee child of tx7 */ CMutableTransaction tx8 = CMutableTransaction(); @@ -216,7 +217,7 @@ BOOST_AUTO_TEST_CASE(MempoolIndexingTest) // Now tx8 should be sorted low, but tx6/tx both high sortedOrder.insert(sortedOrder.begin(), tx8.GetHash().ToString()); - CheckSort(pool, sortedOrder); + CheckSort<1>(pool, sortedOrder); /* low fee child of tx7 */ CMutableTransaction tx9 = CMutableTransaction(); @@ -231,7 +232,7 @@ BOOST_AUTO_TEST_CASE(MempoolIndexingTest) // tx9 should be sorted low BOOST_CHECK_EQUAL(pool.size(), 9); sortedOrder.insert(sortedOrder.begin(), tx9.GetHash().ToString()); - CheckSort(pool, sortedOrder); + CheckSort<1>(pool, sortedOrder); std::vector snapshotOrder = sortedOrder; @@ -273,7 +274,7 @@ BOOST_AUTO_TEST_CASE(MempoolIndexingTest) sortedOrder.insert(sortedOrder.begin()+5, tx9.GetHash().ToString()); sortedOrder.insert(sortedOrder.begin()+6, tx8.GetHash().ToString()); sortedOrder.insert(sortedOrder.begin()+7, tx10.GetHash().ToString()); // tx10 is just before tx6 - CheckSort(pool, sortedOrder); + CheckSort<1>(pool, sortedOrder); // there should be 10 transactions in the mempool BOOST_CHECK_EQUAL(pool.size(), 10); @@ -281,9 +282,42 @@ BOOST_AUTO_TEST_CASE(MempoolIndexingTest) // Now try removing tx10 and verify the sort order returns to normal std::list removed; pool.remove(pool.mapTx.find(tx10.GetHash())->GetTx(), removed, true); - CheckSort(pool, snapshotOrder); + CheckSort<1>(pool, snapshotOrder); + + pool.remove(pool.mapTx.find(tx9.GetHash())->GetTx(), removed, true); + pool.remove(pool.mapTx.find(tx8.GetHash())->GetTx(), removed, true); + /* Now check the sort on the mining score index. + * Final order should be: + * + * tx7 (2M) + * tx2 (20k) + * tx4 (15000) + * tx1/tx5 (10000) + * tx3/6 (0) + * (Ties resolved by hash) + */ + sortedOrder.clear(); + sortedOrder.push_back(tx7.GetHash().ToString()); + sortedOrder.push_back(tx2.GetHash().ToString()); + sortedOrder.push_back(tx4.GetHash().ToString()); + if (tx1.GetHash() < tx5.GetHash()) { + sortedOrder.push_back(tx5.GetHash().ToString()); + sortedOrder.push_back(tx1.GetHash().ToString()); + } else { + sortedOrder.push_back(tx1.GetHash().ToString()); + sortedOrder.push_back(tx5.GetHash().ToString()); + } + if (tx3.GetHash() < tx6.GetHash()) { + sortedOrder.push_back(tx6.GetHash().ToString()); + sortedOrder.push_back(tx3.GetHash().ToString()); + } else { + sortedOrder.push_back(tx3.GetHash().ToString()); + sortedOrder.push_back(tx6.GetHash().ToString()); + } + CheckSort<3>(pool, sortedOrder); } + BOOST_AUTO_TEST_CASE(MempoolSizeLimitTest) { CTxMemPool pool(CFeeRate(1000)); diff --git a/src/txmempool.cpp b/src/txmempool.cpp index fa62cbf16..35be21628 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -36,6 +36,8 @@ CTxMemPoolEntry::CTxMemPoolEntry(const CTransaction& _tx, const CAmount& _nFee, nFeesWithDescendants = nFee; CAmount nValueIn = tx.GetValueOut()+nFee; assert(inChainInputValue <= nValueIn); + + feeDelta = 0; } CTxMemPoolEntry::CTxMemPoolEntry(const CTxMemPoolEntry& other) @@ -53,6 +55,11 @@ CTxMemPoolEntry::GetPriority(unsigned int currentHeight) const return dResult; } +void CTxMemPoolEntry::UpdateFeeDelta(int64_t newFeeDelta) +{ + feeDelta = newFeeDelta; +} + // Update the given tx for any in-mempool descendants. // Assumes that setMemPoolChildren is correct for the given tx and all // descendants. @@ -392,6 +399,15 @@ bool CTxMemPool::addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, } UpdateAncestorsOf(true, newit, setAncestors); + // Update transaction's score for any feeDelta created by PrioritiseTransaction + std::map >::const_iterator pos = mapDeltas.find(hash); + if (pos != mapDeltas.end()) { + const std::pair &deltas = pos->second; + if (deltas.second) { + mapTx.modify(newit, update_fee_delta(deltas.second)); + } + } + nTransactionsUpdated++; totalTxSize += entry.GetTxSize(); minerPolicyEstimator->processTransaction(entry, fCurrentEstimate); @@ -769,6 +785,10 @@ void CTxMemPool::PrioritiseTransaction(const uint256 hash, const string strHash, std::pair &deltas = mapDeltas[hash]; deltas.first += dPriorityDelta; deltas.second += nFeeDelta; + txiter it = mapTx.find(hash); + if (it != mapTx.end()) { + mapTx.modify(it, update_fee_delta(deltas.second)); + } } LogPrintf("PrioritiseTransaction: %s priority += %f, fee += %d\n", strHash, dPriorityDelta, FormatMoney(nFeeDelta)); } @@ -818,8 +838,8 @@ bool CCoinsViewMemPool::HaveCoins(const uint256 &txid) const { size_t CTxMemPool::DynamicMemoryUsage() const { LOCK(cs); - // Estimate the overhead of mapTx to be 9 pointers + an allocation, as no exact formula for boost::multi_index_contained is implemented. - return memusage::MallocUsage(sizeof(CTxMemPoolEntry) + 9 * sizeof(void*)) * mapTx.size() + memusage::DynamicUsage(mapNextTx) + memusage::DynamicUsage(mapDeltas) + memusage::DynamicUsage(mapLinks) + cachedInnerUsage; + // Estimate the overhead of mapTx to be 12 pointers + an allocation, as no exact formula for boost::multi_index_contained is implemented. + return memusage::MallocUsage(sizeof(CTxMemPoolEntry) + 12 * sizeof(void*)) * mapTx.size() + memusage::DynamicUsage(mapNextTx) + memusage::DynamicUsage(mapDeltas) + memusage::DynamicUsage(mapLinks) + cachedInnerUsage; } void CTxMemPool::RemoveStaged(setEntries &stage) { diff --git a/src/txmempool.h b/src/txmempool.h index 334b54782..b011c3882 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -69,6 +69,7 @@ private: CAmount inChainInputValue; //! Sum of all txin values that are already in blockchain bool spendsCoinbase; //! keep track of transactions that spend a coinbase unsigned int sigOpCount; //! Legacy sig ops plus P2SH sig op count + int64_t feeDelta; //! Used for determining the priority of the transaction for mining in a block // Information about descendants of this transaction that are in the // mempool; if we remove this transaction we must remove all of these @@ -98,10 +99,13 @@ public: unsigned int GetHeight() const { return entryHeight; } bool WasClearAtEntry() const { return hadNoDependencies; } unsigned int GetSigOpCount() const { return sigOpCount; } + int64_t GetModifiedFee() const { return nFee + feeDelta; } size_t DynamicMemoryUsage() const { return nUsageSize; } // Adjusts the descendant state, if this entry is not dirty. void UpdateState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount); + // Updates the fee delta used for mining priority score + void UpdateFeeDelta(int64_t feeDelta); /** We can set the entry to be dirty if doing the full calculation of in- * mempool descendants will be too expensive, which can potentially happen @@ -139,6 +143,16 @@ struct set_dirty { e.SetDirty(); } }; +struct update_fee_delta +{ + update_fee_delta(int64_t _feeDelta) : feeDelta(_feeDelta) { } + + void operator() (CTxMemPoolEntry &e) { e.UpdateFeeDelta(feeDelta); } + +private: + int64_t feeDelta; +}; + // extracts a TxMemPoolEntry's transaction hash struct mempoolentry_txid { @@ -186,6 +200,24 @@ public: } }; +/** \class CompareTxMemPoolEntryByScore + * + * Sort by score of entry ((fee+delta)/size) in descending order + */ +class CompareTxMemPoolEntryByScore +{ +public: + bool operator()(const CTxMemPoolEntry& a, const CTxMemPoolEntry& b) + { + double f1 = (double)a.GetModifiedFee() * b.GetTxSize(); + double f2 = (double)b.GetModifiedFee() * a.GetTxSize(); + if (f1 == f2) { + return b.GetTx().GetHash() < a.GetTx().GetHash(); + } + return f1 > f2; + } +}; + class CompareTxMemPoolEntryByEntryTime { public: @@ -223,10 +255,11 @@ public: * * CTxMemPool::mapTx, and CTxMemPoolEntry bookkeeping: * - * mapTx is a boost::multi_index that sorts the mempool on 3 criteria: + * mapTx is a boost::multi_index that sorts the mempool on 4 criteria: * - transaction hash * - feerate [we use max(feerate of tx, feerate of tx with all descendants)] * - time in mempool + * - mining score (feerate modified by any fee deltas from PrioritiseTransaction) * * Note: the term "descendant" refers to in-mempool transactions that depend on * this one, while "ancestor" refers to in-mempool transactions that a given @@ -323,6 +356,11 @@ public: boost::multi_index::ordered_non_unique< boost::multi_index::identity, CompareTxMemPoolEntryByEntryTime + >, + // sorted by score (for mining prioritization) + boost::multi_index::ordered_unique< + boost::multi_index::identity, + CompareTxMemPoolEntryByScore > > > indexed_transaction_set; From 7230187b1d8bf39da5f1502e4c9eeb8e72dccdff Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Tue, 3 Nov 2015 10:02:20 -0500 Subject: [PATCH 3/6] Add TxPriority class and comparator --- src/txmempool.h | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/txmempool.h b/src/txmempool.h index b011c3882..deeb2f7b6 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -587,4 +587,17 @@ public: bool HaveCoins(const uint256 &txid) const; }; +// We want to sort transactions by coin age priority +typedef std::pair TxCoinAgePriority; + +struct TxCoinAgePriorityCompare +{ + bool operator()(const TxCoinAgePriority& a, const TxCoinAgePriority& b) + { + if (a.first == b.first) + return CompareTxMemPoolEntryByScore()(*(b.second), *(a.second)); //Reverse order to make sort less than + return a.first < b.first; + } +}; + #endif // BITCOIN_TXMEMPOOL_H From 1f09287c667d3a7d10ab9c5c96037fe48899490d Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Mon, 26 Oct 2015 14:06:19 -0400 Subject: [PATCH 4/6] Make accessing mempool parents and children public --- src/txmempool.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/txmempool.h b/src/txmempool.h index deeb2f7b6..5652969f4 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -375,6 +375,8 @@ public: }; typedef std::set setEntries; + const setEntries & GetMemPoolParents(txiter entry) const; + const setEntries & GetMemPoolChildren(txiter entry) const; private: typedef std::map cacheMap; @@ -386,8 +388,6 @@ private: typedef std::map txlinksMap; txlinksMap mapLinks; - const setEntries & GetMemPoolParents(txiter entry) const; - const setEntries & GetMemPoolChildren(txiter entry) const; void UpdateParent(txiter entry, txiter parent, bool add); void UpdateChild(txiter entry, txiter child, bool add); From 5f122633020ce5d9f78c73394cda576a8657a3ac Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Wed, 28 Oct 2015 14:56:28 -0400 Subject: [PATCH 5/6] Expose FormatStateMessage --- src/main.cpp | 2 +- src/main.h | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/main.cpp b/src/main.cpp index fb6d32b2f..e9e982043 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -816,7 +816,7 @@ CAmount GetMinRelayFee(const CTransaction& tx, const CTxMemPool& pool, unsigned } /** Convert CValidationState to a human-readable message for logging */ -static std::string FormatStateMessage(const CValidationState &state) +std::string FormatStateMessage(const CValidationState &state) { return strprintf("%s%s (code %i)", state.GetRejectReason(), diff --git a/src/main.h b/src/main.h index 16dff2836..19623f4d9 100644 --- a/src/main.h +++ b/src/main.h @@ -257,6 +257,8 @@ void PruneAndFlush(); bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransaction &tx, bool fLimitFree, bool* pfMissingInputs, bool fOverrideMempoolLimit=false, bool fRejectAbsurdFee=false); +/** Convert CValidationState to a human-readable message for logging */ +std::string FormatStateMessage(const CValidationState &state); struct CNodeStateStats { int nMisbehavior; From 553cad94e29c33872b60d97b8574ed2773355f0b Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Tue, 3 Nov 2015 10:35:39 -0500 Subject: [PATCH 6/6] Rewrite CreateNewBlock Use the score index on the mempool to only add sorted txs in order. Remove much of the validation while building the block, relying on mempool to be consistent and only contain txs that can be mined. The mempool is assumed to be consistent as far as not containing txs which spend non-existent outputs or double spends, and scripts are valid. Finality of txs is still checked (except not coinbase maturity, assumed in mempool). Still TestBlockValidity in case mempool consistency breaks and return error state if an invalid block was created. Unit tests are modified to realize that invalid blocks can now be constructed if the mempool breaks its consistency assumptions and also updated to have the right fees, since the cached value is now used for block construction. Conflicts: src/miner.cpp --- src/miner.cpp | 301 ++++++++++++++++----------------------- src/test/miner_tests.cpp | 60 ++++---- 2 files changed, 156 insertions(+), 205 deletions(-) diff --git a/src/miner.cpp b/src/miner.cpp index 27a1fbcf8..c6db00d30 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -27,6 +27,7 @@ #include #include +#include using namespace std; @@ -40,48 +41,18 @@ using namespace std; // transactions in the memory pool. When we select transactions from the // pool, we select by highest priority or fee rate, so we might consider // transactions that depend on transactions that aren't yet in the block. -// The COrphan class keeps track of these 'temporary orphans' while -// CreateBlock is figuring out which transactions to include. -// -class COrphan -{ -public: - const CTransaction* ptx; - set setDependsOn; - CFeeRate feeRate; - double dPriority; - - COrphan(const CTransaction* ptxIn) : ptx(ptxIn), feeRate(0), dPriority(0) - { - } -}; uint64_t nLastBlockTx = 0; uint64_t nLastBlockSize = 0; -// We want to sort transactions by priority and fee rate, so: -typedef boost::tuple TxPriority; -class TxPriorityCompare +class ScoreCompare { - bool byFee; - public: - TxPriorityCompare(bool _byFee) : byFee(_byFee) { } + ScoreCompare() {} - bool operator()(const TxPriority& a, const TxPriority& b) + bool operator()(const CTxMemPool::txiter a, const CTxMemPool::txiter b) { - if (byFee) - { - if (a.get<1>() == b.get<1>()) - return a.get<0>() < b.get<0>(); - return a.get<1>() < b.get<1>(); - } - else - { - if (a.get<0>() == b.get<0>()) - return a.get<1>() < b.get<1>(); - return a.get<0>() < b.get<0>(); - } + return CompareTxMemPoolEntryByScore()(*b,*a); // Convert to less than } }; @@ -141,6 +112,22 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const CScript& s nBlockMinSize = std::min(nBlockMaxSize, nBlockMinSize); // Collect memory pool transactions into the block + CTxMemPool::setEntries inBlock; + CTxMemPool::setEntries waitSet; + + // This vector will be sorted into a priority queue: + vector vecPriority; + TxCoinAgePriorityCompare pricomparer; + std::map waitPriMap; + typedef std::map::iterator waitPriIter; + double actualPriority = -1; + + std::priority_queue, ScoreCompare> clearedTxs; + bool fPrintPriority = GetBoolArg("-printpriority", DEFAULT_PRINTPRIORITY); + uint64_t nBlockSize = 1000; + uint64_t nBlockTx = 0; + unsigned int nBlockSigOps = 100; + int lastFewTxs = 0; CAmount nFees = 0; { @@ -149,157 +136,102 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const CScript& s const int nHeight = pindexPrev->nHeight + 1; pblock->nTime = GetAdjustedTime(); const int64_t nMedianTimePast = pindexPrev->GetMedianTimePast(); - CCoinsViewCache view(pcoinsTip); - - // Priority order to process transactions - list vOrphan; // list memory doesn't move - map > mapDependers; - bool fPrintPriority = GetBoolArg("-printpriority", DEFAULT_PRINTPRIORITY); - - // This vector will be sorted into a priority queue: - vector vecPriority; - vecPriority.reserve(mempool.mapTx.size()); - for (CTxMemPool::indexed_transaction_set::iterator mi = mempool.mapTx.begin(); - mi != mempool.mapTx.end(); ++mi) - { - const CTransaction& tx = mi->GetTx(); - - int64_t nLockTimeCutoff = (STANDARD_LOCKTIME_VERIFY_FLAGS & LOCKTIME_MEDIAN_TIME_PAST) - ? nMedianTimePast - : pblock->GetBlockTime(); - - if (tx.IsCoinBase() || !IsFinalTx(tx, nHeight, nLockTimeCutoff)) - continue; - - COrphan* porphan = NULL; - double dPriority = 0; - CAmount nTotalIn = 0; - bool fMissingInputs = false; - BOOST_FOREACH(const CTxIn& txin, tx.vin) - { - // Read prev transaction - if (!view.HaveCoins(txin.prevout.hash)) - { - // This should never happen; all transactions in the memory - // pool should connect to either transactions in the chain - // or other transactions in the memory pool. - if (!mempool.mapTx.count(txin.prevout.hash)) - { - LogPrintf("ERROR: mempool transaction missing input\n"); - if (fDebug) assert("mempool transaction missing input" == 0); - fMissingInputs = true; - if (porphan) - vOrphan.pop_back(); - break; - } - - // Has to wait for dependencies - if (!porphan) - { - // Use list for automatic deletion - vOrphan.push_back(COrphan(&tx)); - porphan = &vOrphan.back(); - } - mapDependers[txin.prevout.hash].push_back(porphan); - porphan->setDependsOn.insert(txin.prevout.hash); - nTotalIn += mempool.mapTx.find(txin.prevout.hash)->GetTx().vout[txin.prevout.n].nValue; - continue; - } - const CCoins* coins = view.AccessCoins(txin.prevout.hash); - assert(coins); - - CAmount nValueIn = coins->vout[txin.prevout.n].nValue; - nTotalIn += nValueIn; - - int nConf = nHeight - coins->nHeight; - - dPriority += (double)nValueIn * nConf; - } - if (fMissingInputs) continue; - - // Priority is sum(valuein * age) / modified_txsize - unsigned int nTxSize = ::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION); - dPriority = tx.ComputePriority(dPriority, nTxSize); - - uint256 hash = tx.GetHash(); - mempool.ApplyDeltas(hash, dPriority, nTotalIn); - CFeeRate feeRate(nTotalIn-tx.GetValueOut(), nTxSize); + int64_t nLockTimeCutoff = (STANDARD_LOCKTIME_VERIFY_FLAGS & LOCKTIME_MEDIAN_TIME_PAST) + ? nMedianTimePast + : pblock->GetBlockTime(); - if (porphan) + bool fPriorityBlock = nBlockPrioritySize > 0; + if (fPriorityBlock) { + vecPriority.reserve(mempool.mapTx.size()); + for (CTxMemPool::indexed_transaction_set::iterator mi = mempool.mapTx.begin(); + mi != mempool.mapTx.end(); ++mi) { - porphan->dPriority = dPriority; - porphan->feeRate = feeRate; + double dPriority = mi->GetPriority(nHeight); + CAmount dummy; + mempool.ApplyDeltas(mi->GetTx().GetHash(), dPriority, dummy); + vecPriority.push_back(TxCoinAgePriority(dPriority, mi)); } - else - vecPriority.push_back(TxPriority(dPriority, feeRate, &(mi->GetTx()))); + std::make_heap(vecPriority.begin(), vecPriority.end(), pricomparer); } - // Collect transactions into block - uint64_t nBlockSize = 1000; - uint64_t nBlockTx = 0; - int nBlockSigOps = 100; - bool fSortedByFee = (nBlockPrioritySize <= 0); + CTxMemPool::indexed_transaction_set::nth_index<3>::type::iterator mi = mempool.mapTx.get<3>().begin(); + CTxMemPool::txiter iter; - TxPriorityCompare comparer(fSortedByFee); - std::make_heap(vecPriority.begin(), vecPriority.end(), comparer); - - while (!vecPriority.empty()) + while (mi != mempool.mapTx.get<3>().end() || !clearedTxs.empty()) { - // Take highest priority transaction off the priority queue: - double dPriority = vecPriority.front().get<0>(); - CFeeRate feeRate = vecPriority.front().get<1>(); - const CTransaction& tx = *(vecPriority.front().get<2>()); - - std::pop_heap(vecPriority.begin(), vecPriority.end(), comparer); - vecPriority.pop_back(); - - // Size limits - unsigned int nTxSize = ::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION); - if (nBlockSize + nTxSize >= nBlockMaxSize) - continue; + bool priorityTx = false; + if (fPriorityBlock && !vecPriority.empty()) { // add a tx from priority queue to fill the blockprioritysize + priorityTx = true; + iter = vecPriority.front().second; + actualPriority = vecPriority.front().first; + std::pop_heap(vecPriority.begin(), vecPriority.end(), pricomparer); + vecPriority.pop_back(); + } + else if (clearedTxs.empty()) { // add tx with next highest score + iter = mempool.mapTx.project<0>(mi); + mi++; + } + else { // try to add a previously postponed child tx + iter = clearedTxs.top(); + clearedTxs.pop(); + } - // Legacy limits on sigOps: - unsigned int nTxSigOps = GetLegacySigOpCount(tx); - if (nBlockSigOps + nTxSigOps >= MAX_BLOCK_SIGOPS) - continue; + if (inBlock.count(iter)) + continue; // could have been added to the priorityBlock - // Skip free transactions if we're past the minimum block size: - const uint256& hash = tx.GetHash(); - double dPriorityDelta = 0; - CAmount nFeeDelta = 0; - mempool.ApplyDeltas(hash, dPriorityDelta, nFeeDelta); - if (fSortedByFee && (dPriorityDelta <= 0) && (nFeeDelta <= 0) && (feeRate < ::minRelayTxFee) && (nBlockSize + nTxSize >= nBlockMinSize)) - continue; + const CTransaction& tx = iter->GetTx(); - // Prioritise by fee once past the priority size or we run out of high-priority - // transactions: - if (!fSortedByFee && - ((nBlockSize + nTxSize >= nBlockPrioritySize) || !AllowFree(dPriority))) + bool fOrphan = false; + BOOST_FOREACH(CTxMemPool::txiter parent, mempool.GetMemPoolParents(iter)) { - fSortedByFee = true; - comparer = TxPriorityCompare(fSortedByFee); - std::make_heap(vecPriority.begin(), vecPriority.end(), comparer); + if (!inBlock.count(parent)) { + fOrphan = true; + break; + } } - - if (!view.HaveInputs(tx)) + if (fOrphan) { + if (priorityTx) + waitPriMap.insert(std::make_pair(iter,actualPriority)); + else + waitSet.insert(iter); continue; + } - CAmount nTxFees = view.GetValueIn(tx)-tx.GetValueOut(); - - nTxSigOps += GetP2SHSigOpCount(tx, view); - if (nBlockSigOps + nTxSigOps >= MAX_BLOCK_SIGOPS) + unsigned int nTxSize = iter->GetTxSize(); + if (fPriorityBlock && + (nBlockSize + nTxSize >= nBlockPrioritySize || !AllowFree(actualPriority))) { + fPriorityBlock = false; + waitPriMap.clear(); + } + if (!priorityTx && + (iter->GetModifiedFee() < ::minRelayTxFee.GetFee(nTxSize) && nBlockSize >= nBlockMinSize)) { + break; + } + if (nBlockSize + nTxSize >= nBlockMaxSize) { + if (nBlockSize > nBlockMaxSize - 100 || lastFewTxs > 50) { + break; + } + // Once we're within 1000 bytes of a full block, only look at 50 more txs + // to try to fill the remaining space. + if (nBlockSize > nBlockMaxSize - 1000) { + lastFewTxs++; + } continue; + } - // Note that flags: we don't want to set mempool/IsStandard() - // policy here, but we still have to ensure that the block we - // create only contains transactions that are valid in new blocks. - CValidationState state; - if (!CheckInputs(tx, state, view, true, MANDATORY_SCRIPT_VERIFY_FLAGS, true)) + if (!IsFinalTx(tx, nHeight, nLockTimeCutoff)) continue; - UpdateCoins(tx, state, view, nHeight); + unsigned int nTxSigOps = iter->GetSigOpCount(); + if (nBlockSigOps + nTxSigOps >= MAX_BLOCK_SIGOPS) { + if (nBlockSigOps > MAX_BLOCK_SIGOPS - 2) { + break; + } + continue; + } + CAmount nTxFees = iter->GetFee(); // Added pblock->vtx.push_back(tx); pblocktemplate->vTxFees.push_back(nTxFees); @@ -311,31 +243,37 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const CScript& s if (fPrintPriority) { + double dPriority = iter->GetPriority(nHeight); + CAmount dummy; + mempool.ApplyDeltas(tx.GetHash(), dPriority, dummy); LogPrintf("priority %.1f fee %s txid %s\n", - dPriority, feeRate.ToString(), tx.GetHash().ToString()); + dPriority , CFeeRate(iter->GetModifiedFee(), nTxSize).ToString(), tx.GetHash().ToString()); } + inBlock.insert(iter); + // Add transactions that depend on this one to the priority queue - if (mapDependers.count(hash)) + BOOST_FOREACH(CTxMemPool::txiter child, mempool.GetMemPoolChildren(iter)) { - BOOST_FOREACH(COrphan* porphan, mapDependers[hash]) - { - if (!porphan->setDependsOn.empty()) - { - porphan->setDependsOn.erase(hash); - if (porphan->setDependsOn.empty()) - { - vecPriority.push_back(TxPriority(porphan->dPriority, porphan->feeRate, porphan->ptx)); - std::push_heap(vecPriority.begin(), vecPriority.end(), comparer); - } + if (fPriorityBlock) { + waitPriIter wpiter = waitPriMap.find(child); + if (wpiter != waitPriMap.end()) { + vecPriority.push_back(TxCoinAgePriority(wpiter->second,child)); + std::push_heap(vecPriority.begin(), vecPriority.end(), pricomparer); + waitPriMap.erase(wpiter); + } + } + else { + if (waitSet.count(child)) { + clearedTxs.push(child); + waitSet.erase(child); } } } } - nLastBlockTx = nBlockTx; nLastBlockSize = nBlockSize; - LogPrintf("CreateNewBlock(): total size %u\n", nBlockSize); + LogPrintf("CreateNewBlock(): total size %u txs: %u fees: %ld sigops %d\n", nBlockSize, nBlockTx, nFees, nBlockSigOps); // Compute final coinbase transaction. txNew.vout[0].nValue = nFees + GetBlockSubsidy(nHeight, chainparams.GetConsensus()); @@ -351,8 +289,9 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const CScript& s pblocktemplate->vTxSigOps[0] = GetLegacySigOpCount(pblock->vtx[0]); CValidationState state; - if (!TestBlockValidity(state, chainparams, *pblock, pindexPrev, false, false)) - throw std::runtime_error("CreateNewBlock(): TestBlockValidity failed"); + if (!TestBlockValidity(state, chainparams, *pblock, pindexPrev, false, false)) { + throw std::runtime_error(strprintf("%s: TestBlockValidity failed: %s", __func__, FormatStateMessage(state))); + } } return pblocktemplate.release(); diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index 531cd59d5..19ddb5b79 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -120,7 +120,22 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) tx.vout[0].nValue -= 1000000; hash = tx.GetHash(); bool spendsCoinbase = (i == 0) ? true : false; // only first tx spends coinbase - mempool.addUnchecked(hash, entry.Time(GetTime()).SpendsCoinbase(spendsCoinbase).FromTx(tx)); + // If we don't set the # of sig ops in the CTxMemPoolEntry, template creation fails + mempool.addUnchecked(hash, entry.Fee(1000000).Time(GetTime()).SpendsCoinbase(spendsCoinbase).FromTx(tx)); + tx.vin[0].prevout.hash = hash; + } + BOOST_CHECK_THROW(CreateNewBlock(chainparams, scriptPubKey), std::runtime_error); + mempool.clear(); + + tx.vin[0].prevout.hash = txFirst[0]->GetHash(); + tx.vout[0].nValue = 5000000000LL; + for (unsigned int i = 0; i < 1001; ++i) + { + tx.vout[0].nValue -= 1000000; + hash = tx.GetHash(); + bool spendsCoinbase = (i == 0) ? true : false; // only first tx spends coinbase + // If we do set the # of sig ops in the CTxMemPoolEntry, template creation passes + mempool.addUnchecked(hash, entry.Fee(1000000).Time(GetTime()).SpendsCoinbase(spendsCoinbase).SigOps(20).FromTx(tx)); tx.vin[0].prevout.hash = hash; } BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); @@ -141,18 +156,17 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) tx.vout[0].nValue -= 10000000; hash = tx.GetHash(); bool spendsCoinbase = (i == 0) ? true : false; // only first tx spends coinbase - mempool.addUnchecked(hash, entry.Time(GetTime()).SpendsCoinbase(spendsCoinbase).FromTx(tx)); + mempool.addUnchecked(hash, entry.Fee(1000000).Time(GetTime()).SpendsCoinbase(spendsCoinbase).FromTx(tx)); tx.vin[0].prevout.hash = hash; } BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); delete pblocktemplate; mempool.clear(); - // orphan in mempool + // orphan in mempool, template creation fails hash = tx.GetHash(); - mempool.addUnchecked(hash, entry.Time(GetTime()).FromTx(tx)); - BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); - delete pblocktemplate; + mempool.addUnchecked(hash, entry.Fee(1000000).Time(GetTime()).FromTx(tx)); + BOOST_CHECK_THROW(CreateNewBlock(chainparams, scriptPubKey), std::runtime_error); mempool.clear(); // child with higher priority than parent @@ -160,7 +174,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) tx.vin[0].prevout.hash = txFirst[1]->GetHash(); tx.vout[0].nValue = 4900000000LL; hash = tx.GetHash(); - mempool.addUnchecked(hash, entry.Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); + mempool.addUnchecked(hash, entry.Fee(100000000LL).Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); tx.vin[0].prevout.hash = hash; tx.vin.resize(2); tx.vin[1].scriptSig = CScript() << OP_1; @@ -168,23 +182,23 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) tx.vin[1].prevout.n = 0; tx.vout[0].nValue = 5900000000LL; hash = tx.GetHash(); - mempool.addUnchecked(hash, entry.Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); + mempool.addUnchecked(hash, entry.Fee(400000000LL).Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); delete pblocktemplate; mempool.clear(); - // coinbase in mempool + // coinbase in mempool, template creation fails tx.vin.resize(1); tx.vin[0].prevout.SetNull(); tx.vin[0].scriptSig = CScript() << OP_0 << OP_1; tx.vout[0].nValue = 0; hash = tx.GetHash(); - mempool.addUnchecked(hash, entry.Time(GetTime()).SpendsCoinbase(false).FromTx(tx)); - BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); - delete pblocktemplate; + // give it a fee so it'll get mined + mempool.addUnchecked(hash, entry.Fee(100000).Time(GetTime()).SpendsCoinbase(false).FromTx(tx)); + BOOST_CHECK_THROW(CreateNewBlock(chainparams, scriptPubKey), std::runtime_error); mempool.clear(); - // invalid (pre-p2sh) txn in mempool + // invalid (pre-p2sh) txn in mempool, template creation fails tx.vin[0].prevout.hash = txFirst[0]->GetHash(); tx.vin[0].prevout.n = 0; tx.vin[0].scriptSig = CScript() << OP_1; @@ -192,28 +206,26 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) script = CScript() << OP_0; tx.vout[0].scriptPubKey = GetScriptForDestination(CScriptID(script)); hash = tx.GetHash(); - mempool.addUnchecked(hash, entry.Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); + mempool.addUnchecked(hash, entry.Fee(10000000L).Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); tx.vin[0].prevout.hash = hash; tx.vin[0].scriptSig = CScript() << std::vector(script.begin(), script.end()); tx.vout[0].nValue -= 1000000; hash = tx.GetHash(); - mempool.addUnchecked(hash, entry.Time(GetTime()).SpendsCoinbase(false).FromTx(tx)); - BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); - delete pblocktemplate; + mempool.addUnchecked(hash, entry.Fee(1000000).Time(GetTime()).SpendsCoinbase(false).FromTx(tx)); + BOOST_CHECK_THROW(CreateNewBlock(chainparams, scriptPubKey), std::runtime_error); mempool.clear(); - // double spend txn pair in mempool + // double spend txn pair in mempool, template creation fails tx.vin[0].prevout.hash = txFirst[0]->GetHash(); tx.vin[0].scriptSig = CScript() << OP_1; tx.vout[0].nValue = 4900000000LL; tx.vout[0].scriptPubKey = CScript() << OP_1; hash = tx.GetHash(); - mempool.addUnchecked(hash, entry.Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); + mempool.addUnchecked(hash, entry.Fee(100000000L).Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); tx.vout[0].scriptPubKey = CScript() << OP_2; hash = tx.GetHash(); - mempool.addUnchecked(hash, entry.Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); - BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); - delete pblocktemplate; + mempool.addUnchecked(hash, entry.Fee(100000000L).Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); + BOOST_CHECK_THROW(CreateNewBlock(chainparams, scriptPubKey), std::runtime_error); mempool.clear(); // subsidy changing @@ -237,7 +249,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) tx.vout[0].scriptPubKey = CScript() << OP_1; tx.nLockTime = chainActive.Tip()->nHeight+1; hash = tx.GetHash(); - mempool.addUnchecked(hash, entry.Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); + mempool.addUnchecked(hash, entry.Fee(100000000L).Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); BOOST_CHECK(!CheckFinalTx(tx, LOCKTIME_MEDIAN_TIME_PAST)); // time locked @@ -251,7 +263,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) tx2.vout[0].scriptPubKey = CScript() << OP_1; tx2.nLockTime = chainActive.Tip()->GetMedianTimePast()+1; hash = tx2.GetHash(); - mempool.addUnchecked(hash, entry.Time(GetTime()).SpendsCoinbase(true).FromTx(tx2)); + mempool.addUnchecked(hash, entry.Fee(100000000L).Time(GetTime()).SpendsCoinbase(true).FromTx(tx2)); BOOST_CHECK(!CheckFinalTx(tx2, LOCKTIME_MEDIAN_TIME_PAST)); BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey));