From 4df44794c9f71d47648e858385d37eae6d0a9db3 Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Thu, 10 Nov 2016 14:16:42 -0500 Subject: [PATCH 01/10] Remove extraneous LogPrint from fee estimation Once priority estimation was removed, not all transactions in the mempool are tracked in the fee estimation mempool tracking. So there is no error if a transaction is not found for removal. --- src/policy/fees.cpp | 17 +++++++---------- src/policy/fees.h | 2 +- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index a1b785e3e..52d0e5c5e 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -281,19 +281,16 @@ void TxConfirmStats::removeTx(unsigned int entryHeight, unsigned int nBestSeenHe } } -void CBlockPolicyEstimator::removeTx(uint256 hash) +bool CBlockPolicyEstimator::removeTx(uint256 hash) { std::map::iterator pos = mapMemPoolTxs.find(hash); - if (pos == mapMemPoolTxs.end()) { - LogPrint("estimatefee", "Blockpolicy error mempool tx %s not found for removeTx\n", - hash.ToString().c_str()); - return; + if (pos != mapMemPoolTxs.end()) { + feeStats.removeTx(pos->second.blockHeight, nBestSeenHeight, pos->second.bucketIndex); + mapMemPoolTxs.erase(hash); + return true; + } else { + return false; } - unsigned int entryHeight = pos->second.blockHeight; - unsigned int bucketIndex = pos->second.bucketIndex; - - feeStats.removeTx(entryHeight, nBestSeenHeight, bucketIndex); - mapMemPoolTxs.erase(hash); } CBlockPolicyEstimator::CBlockPolicyEstimator(const CFeeRate& _minRelayFee) diff --git a/src/policy/fees.h b/src/policy/fees.h index 1f10cfb23..c88ba5b3c 100644 --- a/src/policy/fees.h +++ b/src/policy/fees.h @@ -212,7 +212,7 @@ public: void processTransaction(const CTxMemPoolEntry& entry, bool fCurrentEstimate); /** Remove a transaction from the mempool tracking stats*/ - void removeTx(uint256 hash); + bool removeTx(uint256 hash); /** Return a feerate estimate */ CFeeRate estimateFee(int confTarget); From 60ac00de854981333656d17c9b2ea2efd1efc436 Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Fri, 11 Nov 2016 11:17:59 -0500 Subject: [PATCH 02/10] Don't track transactions at all during IBD. This was an oversight, where blocks and mempool tracking were ignored during IBD, but transactions that arrived during IBD but were included in blocks after IBD were not ignored. --- src/validation.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/validation.cpp b/src/validation.cpp index bc14c1d8c..2783a313f 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -692,7 +692,9 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C } } - CTxMemPoolEntry entry(ptx, nFees, nAcceptTime, dPriority, chainActive.Height(), pool.HasNoInputsOf(tx), inChainInputValue, fSpendsCoinbase, nSigOpsCost, lp); + CTxMemPoolEntry entry(ptx, nFees, nAcceptTime, dPriority, chainActive.Height(), + !IsInitialBlockDownload() && pool.HasNoInputsOf(tx), + inChainInputValue, fSpendsCoinbase, nSigOpsCost, lp); unsigned int nSize = entry.GetTxSize(); // Check that the transaction doesn't have an excessive number of From 84f7ab08d2e8e83a584d72fdf44f68b34baf8165 Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Fri, 11 Nov 2016 11:57:51 -0500 Subject: [PATCH 03/10] Remove member variable hadNoDependencies from CTxMemPoolEntry Fee estimation can just check its own mapMemPoolTxs to determine the same information. Note that now fee estimation for block processing must happen before those transactions are removed, but this shoudl be a speedup. --- src/bench/mempool_eviction.cpp | 2 +- src/policy/fees.cpp | 21 ++++++++------------- src/test/mempool_tests.cpp | 2 -- src/test/test_bitcoin.cpp | 5 ++--- src/test/test_bitcoin.h | 4 +--- src/txmempool.cpp | 8 ++++---- src/txmempool.h | 4 +--- src/validation.cpp | 3 +-- src/wallet/wallet.cpp | 2 +- 9 files changed, 19 insertions(+), 32 deletions(-) diff --git a/src/bench/mempool_eviction.cpp b/src/bench/mempool_eviction.cpp index d04c03684..5790d51a8 100644 --- a/src/bench/mempool_eviction.cpp +++ b/src/bench/mempool_eviction.cpp @@ -18,7 +18,7 @@ static void AddTx(const CTransaction& tx, const CAmount& nFee, CTxMemPool& pool) unsigned int sigOpCost = 4; LockPoints lp; pool.addUnchecked(tx.GetHash(), CTxMemPoolEntry( - MakeTransactionRef(tx), nFee, nTime, dPriority, nHeight, pool.HasNoInputsOf(tx), + MakeTransactionRef(tx), nFee, nTime, dPriority, nHeight, tx.GetValueOut(), spendsCoinbase, sigOpCost, lp)); } diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index 52d0e5c5e..ac2d7edae 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -327,13 +327,6 @@ void CBlockPolicyEstimator::processTransaction(const CTxMemPoolEntry& entry, boo if (!fCurrentEstimate) return; - if (!entry.WasClearAtEntry()) { - // This transaction depends on other transactions in the mempool to - // be included in a block before it will be able to be included, so - // we shouldn't include it in our calculations - return; - } - // Feerates are stored and reported as BTC-per-kb: CFeeRate feeRate(entry.GetFee(), entry.GetTxSize()); @@ -343,10 +336,8 @@ void CBlockPolicyEstimator::processTransaction(const CTxMemPoolEntry& entry, boo void CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const CTxMemPoolEntry& entry) { - if (!entry.WasClearAtEntry()) { - // This transaction depended on other transactions in the mempool to - // be included in a block before it was able to be included, so - // we shouldn't include it in our calculations + if (!removeTx(entry.GetTx().GetHash())) { + // This transaction wasn't being tracked for fee estimation return; } @@ -378,14 +369,18 @@ void CBlockPolicyEstimator::processBlock(unsigned int nBlockHeight, // transaction fees." return; } - nBestSeenHeight = nBlockHeight; // Only want to be updating estimates when our blockchain is synced, // otherwise we'll miscalculate how many blocks its taking to get included. if (!fCurrentEstimate) return; - // Clear the current block state + // Must update nBestSeenHeight in sync with ClearCurrent so that + // calls to removeTx (via processBlockTx) correctly calculate age + // of unconfirmed txs to remove from tracking. + nBestSeenHeight = nBlockHeight; + + // Clear the current block state and update unconfirmed circular buffer feeStats.ClearCurrent(nBlockHeight); // Repopulate the current block states diff --git a/src/test/mempool_tests.cpp b/src/test/mempool_tests.cpp index 37f0de354..91f549fe4 100644 --- a/src/test/mempool_tests.cpp +++ b/src/test/mempool_tests.cpp @@ -120,7 +120,6 @@ BOOST_AUTO_TEST_CASE(MempoolIndexingTest) { CTxMemPool pool(CFeeRate(0)); TestMemPoolEntryHelper entry; - entry.hadNoDependencies = true; /* 3rd highest fee */ CMutableTransaction tx1 = CMutableTransaction(); @@ -323,7 +322,6 @@ BOOST_AUTO_TEST_CASE(MempoolAncestorIndexingTest) { CTxMemPool pool(CFeeRate(0)); TestMemPoolEntryHelper entry; - entry.hadNoDependencies = true; /* 3rd highest fee */ CMutableTransaction tx1 = CMutableTransaction(); diff --git a/src/test/test_bitcoin.cpp b/src/test/test_bitcoin.cpp index 672cd1142..f0eaab221 100644 --- a/src/test/test_bitcoin.cpp +++ b/src/test/test_bitcoin.cpp @@ -147,12 +147,11 @@ CTxMemPoolEntry TestMemPoolEntryHelper::FromTx(const CMutableTransaction &tx, CT } CTxMemPoolEntry TestMemPoolEntryHelper::FromTx(const CTransaction &txn, CTxMemPool *pool) { - bool hasNoDependencies = pool ? pool->HasNoInputsOf(txn) : hadNoDependencies; // Hack to assume either its completely dependent on other mempool txs or not at all - CAmount inChainValue = hasNoDependencies ? txn.GetValueOut() : 0; + CAmount inChainValue = pool && pool->HasNoInputsOf(txn) ? txn.GetValueOut() : 0; return CTxMemPoolEntry(MakeTransactionRef(txn), nFee, nTime, dPriority, nHeight, - hasNoDependencies, inChainValue, spendsCoinbase, sigOpCost, lp); + inChainValue, spendsCoinbase, sigOpCost, lp); } void Shutdown(void* parg) diff --git a/src/test/test_bitcoin.h b/src/test/test_bitcoin.h index 0fe77437e..5ef6fa764 100644 --- a/src/test/test_bitcoin.h +++ b/src/test/test_bitcoin.h @@ -70,14 +70,13 @@ struct TestMemPoolEntryHelper int64_t nTime; double dPriority; unsigned int nHeight; - bool hadNoDependencies; bool spendsCoinbase; unsigned int sigOpCost; LockPoints lp; TestMemPoolEntryHelper() : nFee(0), nTime(0), dPriority(0.0), nHeight(1), - hadNoDependencies(false), spendsCoinbase(false), sigOpCost(4) { } + spendsCoinbase(false), sigOpCost(4) { } CTxMemPoolEntry FromTx(const CMutableTransaction &tx, CTxMemPool *pool = NULL); CTxMemPoolEntry FromTx(const CTransaction &tx, CTxMemPool *pool = NULL); @@ -87,7 +86,6 @@ struct TestMemPoolEntryHelper TestMemPoolEntryHelper &Time(int64_t _time) { nTime = _time; return *this; } TestMemPoolEntryHelper &Priority(double _priority) { dPriority = _priority; return *this; } 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 &SigOpsCost(unsigned int _sigopsCost) { sigOpCost = _sigopsCost; return *this; } }; diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 4ccbcadef..57300bb51 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -22,10 +22,10 @@ using namespace std; CTxMemPoolEntry::CTxMemPoolEntry(const CTransactionRef& _tx, const CAmount& _nFee, int64_t _nTime, double _entryPriority, unsigned int _entryHeight, - bool poolHasNoInputsOf, CAmount _inChainInputValue, + CAmount _inChainInputValue, bool _spendsCoinbase, int64_t _sigOpsCost, LockPoints lp): tx(_tx), nFee(_nFee), nTime(_nTime), entryPriority(_entryPriority), entryHeight(_entryHeight), - hadNoDependencies(poolHasNoInputsOf), inChainInputValue(_inChainInputValue), + inChainInputValue(_inChainInputValue), spendsCoinbase(_spendsCoinbase), sigOpCost(_sigOpsCost), lockPoints(lp) { nTxWeight = GetTransactionWeight(*tx); @@ -604,6 +604,8 @@ void CTxMemPool::removeForBlock(const std::vector& vtx, unsigne if (i != mapTx.end()) entries.push_back(*i); } + // Before the txs in the new block have been removed from the mempool, update policy estimates + minerPolicyEstimator->processBlock(nBlockHeight, entries, fCurrentEstimate); for (const auto& tx : vtx) { txiter it = mapTx.find(tx->GetHash()); @@ -615,8 +617,6 @@ void CTxMemPool::removeForBlock(const std::vector& vtx, unsigne removeConflicts(*tx); ClearPrioritisation(tx->GetHash()); } - // After the txs in the new block have been removed from the mempool, update policy estimates - minerPolicyEstimator->processBlock(nBlockHeight, entries, fCurrentEstimate); lastRollingFeeUpdate = GetTime(); blockSinceLastRollingFeeBump = true; } diff --git a/src/txmempool.h b/src/txmempool.h index 7c68053f2..5c8cf7af1 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -88,7 +88,6 @@ private: int64_t nTime; //!< Local time when entering the mempool double entryPriority; //!< Priority when entering the mempool unsigned int entryHeight; //!< Chain height when entering the mempool - 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 int64_t sigOpCost; //!< Total sigop cost @@ -113,7 +112,7 @@ private: public: CTxMemPoolEntry(const CTransactionRef& _tx, const CAmount& _nFee, int64_t _nTime, double _entryPriority, unsigned int _entryHeight, - bool poolHasNoInputsOf, CAmount _inChainInputValue, bool spendsCoinbase, + CAmount _inChainInputValue, bool spendsCoinbase, int64_t nSigOpsCost, LockPoints lp); CTxMemPoolEntry(const CTxMemPoolEntry& other); @@ -130,7 +129,6 @@ public: size_t GetTxWeight() const { return nTxWeight; } int64_t GetTime() const { return nTime; } unsigned int GetHeight() const { return entryHeight; } - bool WasClearAtEntry() const { return hadNoDependencies; } int64_t GetSigOpCost() const { return sigOpCost; } int64_t GetModifiedFee() const { return nFee + feeDelta; } size_t DynamicMemoryUsage() const { return nUsageSize; } diff --git a/src/validation.cpp b/src/validation.cpp index 2783a313f..a68763e48 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -693,7 +693,6 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C } CTxMemPoolEntry entry(ptx, nFees, nAcceptTime, dPriority, chainActive.Height(), - !IsInitialBlockDownload() && pool.HasNoInputsOf(tx), inChainInputValue, fSpendsCoinbase, nSigOpsCost, lp); unsigned int nSize = entry.GetTxSize(); @@ -943,7 +942,7 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C pool.RemoveStaged(allConflicting, false); // Store transaction in memory - pool.addUnchecked(hash, entry, setAncestors, !IsInitialBlockDownload()); + pool.addUnchecked(hash, entry, setAncestors, !IsInitialBlockDownload() && pool.HasNoInputsOf(tx)); // trim mempool and check if tx was trimmed if (!fOverrideMempoolLimit) { diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 967683256..0b13c2ba7 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2563,7 +2563,7 @@ bool CWallet::CreateTransaction(const vector& vecSend, CWalletTx& wt if (GetBoolArg("-walletrejectlongchains", DEFAULT_WALLET_REJECT_LONG_CHAINS)) { // Lastly, ensure this tx will pass the mempool's chain limits LockPoints lp; - CTxMemPoolEntry entry(wtxNew.tx, 0, 0, 0, 0, false, 0, false, 0, lp); + CTxMemPoolEntry entry(wtxNew.tx, 0, 0, 0, 0, 0, false, 0, lp); CTxMemPool::setEntries setAncestors; size_t nLimitAncestors = GetArg("-limitancestorcount", DEFAULT_ANCESTOR_LIMIT); size_t nLimitAncestorSize = GetArg("-limitancestorsize", DEFAULT_ANCESTOR_SIZE_LIMIT)*1000; From 6f06b268c1f383affb2cf397f325d48d25bc8880 Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Fri, 11 Nov 2016 12:48:01 -0500 Subject: [PATCH 04/10] rename bool to validFeeEstimate --- src/policy/fees.cpp | 4 ++-- src/policy/fees.h | 2 +- src/txmempool.cpp | 8 ++++---- src/txmempool.h | 4 ++-- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index ac2d7edae..6b4567d19 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -306,7 +306,7 @@ CBlockPolicyEstimator::CBlockPolicyEstimator(const CFeeRate& _minRelayFee) feeStats.Initialize(vfeelist, MAX_BLOCK_CONFIRMS, DEFAULT_DECAY); } -void CBlockPolicyEstimator::processTransaction(const CTxMemPoolEntry& entry, bool fCurrentEstimate) +void CBlockPolicyEstimator::processTransaction(const CTxMemPoolEntry& entry, bool validFeeEstimate) { unsigned int txHeight = entry.GetHeight(); uint256 hash = entry.GetTx().GetHash(); @@ -324,7 +324,7 @@ void CBlockPolicyEstimator::processTransaction(const CTxMemPoolEntry& entry, boo // Only want to be updating estimates when our blockchain is synced, // otherwise we'll miscalculate how many blocks its taking to get included. - if (!fCurrentEstimate) + if (!validFeeEstimate) return; // Feerates are stored and reported as BTC-per-kb: diff --git a/src/policy/fees.h b/src/policy/fees.h index c88ba5b3c..e062cd87b 100644 --- a/src/policy/fees.h +++ b/src/policy/fees.h @@ -209,7 +209,7 @@ public: void processBlockTx(unsigned int nBlockHeight, const CTxMemPoolEntry& entry); /** Process a transaction accepted to the mempool*/ - void processTransaction(const CTxMemPoolEntry& entry, bool fCurrentEstimate); + void processTransaction(const CTxMemPoolEntry& entry, bool validFeeEstimate); /** Remove a transaction from the mempool tracking stats*/ bool removeTx(uint256 hash); diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 57300bb51..53f3e0e19 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -392,7 +392,7 @@ void CTxMemPool::AddTransactionsUpdated(unsigned int n) nTransactionsUpdated += n; } -bool CTxMemPool::addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, setEntries &setAncestors, bool fCurrentEstimate) +bool CTxMemPool::addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, setEntries &setAncestors, bool validFeeEstimate) { // Add to memory pool without checking anything. // Used by main.cpp AcceptToMemoryPool(), which DOES do @@ -442,7 +442,7 @@ bool CTxMemPool::addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, nTransactionsUpdated++; totalTxSize += entry.GetTxSize(); - minerPolicyEstimator->processTransaction(entry, fCurrentEstimate); + minerPolicyEstimator->processTransaction(entry, validFeeEstimate); vTxHashes.emplace_back(tx.GetWitnessHash(), newit); newit->vTxHashesIdx = vTxHashes.size() - 1; @@ -1015,14 +1015,14 @@ int CTxMemPool::Expire(int64_t time) { return stage.size(); } -bool CTxMemPool::addUnchecked(const uint256&hash, const CTxMemPoolEntry &entry, bool fCurrentEstimate) +bool CTxMemPool::addUnchecked(const uint256&hash, const CTxMemPoolEntry &entry, bool validFeeEstimate) { LOCK(cs); setEntries setAncestors; uint64_t nNoLimit = std::numeric_limits::max(); std::string dummy; CalculateMemPoolAncestors(entry, setAncestors, nNoLimit, nNoLimit, nNoLimit, nNoLimit, dummy); - return addUnchecked(hash, entry, setAncestors, fCurrentEstimate); + return addUnchecked(hash, entry, setAncestors, validFeeEstimate); } void CTxMemPool::UpdateChild(txiter entry, txiter child, bool add) diff --git a/src/txmempool.h b/src/txmempool.h index 5c8cf7af1..16125bd73 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -523,8 +523,8 @@ public: // to track size/count of descendant transactions. First version of // addUnchecked can be used to have it call CalculateMemPoolAncestors(), and // then invoke the second version. - bool addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, bool fCurrentEstimate = true); - bool addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, setEntries &setAncestors, bool fCurrentEstimate = true); + bool addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, bool validFeeEstimate = true); + bool addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, setEntries &setAncestors, bool validFeeEstimate = true); void removeRecursive(const CTransaction &tx); void removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight, int flags); From d825838e6472f73c491f93506cb003472f071602 Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Fri, 11 Nov 2016 13:14:45 -0500 Subject: [PATCH 05/10] Always update fee estimates on new blocks. All decisions about whether the transactions are valid data points are made at the time the transaction arrives. Updating on blocks all the time will now cause stale fee estimates to decay quickly when we restart a node. --- src/policy/fees.cpp | 7 +------ src/policy/fees.h | 2 +- src/txmempool.cpp | 5 ++--- src/txmempool.h | 3 +-- src/validation.cpp | 2 +- 5 files changed, 6 insertions(+), 13 deletions(-) diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index 6b4567d19..eb9fdc77d 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -359,7 +359,7 @@ void CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const CTxM } void CBlockPolicyEstimator::processBlock(unsigned int nBlockHeight, - std::vector& entries, bool fCurrentEstimate) + std::vector& entries) { if (nBlockHeight <= nBestSeenHeight) { // Ignore side chains and re-orgs; assuming they are random @@ -370,11 +370,6 @@ void CBlockPolicyEstimator::processBlock(unsigned int nBlockHeight, return; } - // Only want to be updating estimates when our blockchain is synced, - // otherwise we'll miscalculate how many blocks its taking to get included. - if (!fCurrentEstimate) - return; - // Must update nBestSeenHeight in sync with ClearCurrent so that // calls to removeTx (via processBlockTx) correctly calculate age // of unconfirmed txs to remove from tracking. diff --git a/src/policy/fees.h b/src/policy/fees.h index e062cd87b..a61ae1813 100644 --- a/src/policy/fees.h +++ b/src/policy/fees.h @@ -203,7 +203,7 @@ public: /** Process all the transactions that have been included in a block */ void processBlock(unsigned int nBlockHeight, - std::vector& entries, bool fCurrentEstimate); + std::vector& entries); /** Process a transaction confirmed in a block*/ void processBlockTx(unsigned int nBlockHeight, const CTxMemPoolEntry& entry); diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 53f3e0e19..e97099eb2 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -591,8 +591,7 @@ void CTxMemPool::removeConflicts(const CTransaction &tx) /** * Called when a block is connected. Removes from mempool and updates the miner fee estimator. */ -void CTxMemPool::removeForBlock(const std::vector& vtx, unsigned int nBlockHeight, - bool fCurrentEstimate) +void CTxMemPool::removeForBlock(const std::vector& vtx, unsigned int nBlockHeight) { LOCK(cs); std::vector entries; @@ -605,7 +604,7 @@ void CTxMemPool::removeForBlock(const std::vector& vtx, unsigne entries.push_back(*i); } // Before the txs in the new block have been removed from the mempool, update policy estimates - minerPolicyEstimator->processBlock(nBlockHeight, entries, fCurrentEstimate); + minerPolicyEstimator->processBlock(nBlockHeight, entries); for (const auto& tx : vtx) { txiter it = mapTx.find(tx->GetHash()); diff --git a/src/txmempool.h b/src/txmempool.h index 16125bd73..b4f52e647 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -529,8 +529,7 @@ public: void removeRecursive(const CTransaction &tx); void removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight, int flags); void removeConflicts(const CTransaction &tx); - void removeForBlock(const std::vector& vtx, unsigned int nBlockHeight, - bool fCurrentEstimate = true); + void removeForBlock(const std::vector& vtx, unsigned int nBlockHeight); void clear(); void _clear(); //lock free bool CompareDepthAndScore(const uint256& hasha, const uint256& hashb); diff --git a/src/validation.cpp b/src/validation.cpp index a68763e48..fb6a902bc 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2204,7 +2204,7 @@ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, int64_t nTime5 = GetTimeMicros(); nTimeChainState += nTime5 - nTime4; LogPrint("bench", " - Writing chainstate: %.2fms [%.2fs]\n", (nTime5 - nTime4) * 0.001, nTimeChainState * 0.000001); // Remove conflicting transactions from the mempool.; - mempool.removeForBlock(blockConnecting.vtx, pindexNew->nHeight, !IsInitialBlockDownload()); + mempool.removeForBlock(blockConnecting.vtx, pindexNew->nHeight); // Update chainActive & related variables. UpdateTip(pindexNew, chainparams); From ebafdcabb10a89b491cdb8430bc43b0220d436df Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Fri, 11 Nov 2016 14:16:42 -0500 Subject: [PATCH 06/10] Pass pointers to existing CTxMemPoolEntries to fee estimation --- src/policy/fees.cpp | 10 +++++----- src/policy/fees.h | 4 ++-- src/txmempool.cpp | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index eb9fdc77d..cb83bcf71 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -334,9 +334,9 @@ void CBlockPolicyEstimator::processTransaction(const CTxMemPoolEntry& entry, boo mapMemPoolTxs[hash].bucketIndex = feeStats.NewTx(txHeight, (double)feeRate.GetFeePerK()); } -void CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const CTxMemPoolEntry& entry) +void CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const CTxMemPoolEntry* entry) { - if (!removeTx(entry.GetTx().GetHash())) { + if (!removeTx(entry->GetTx().GetHash())) { // This transaction wasn't being tracked for fee estimation return; } @@ -344,7 +344,7 @@ void CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const CTxM // How many blocks did it take for miners to include this transaction? // blocksToConfirm is 1-based, so a transaction included in the earliest // possible block has confirmation count of 1 - int blocksToConfirm = nBlockHeight - entry.GetHeight(); + int blocksToConfirm = nBlockHeight - entry->GetHeight(); if (blocksToConfirm <= 0) { // This can't happen because we don't process transactions from a block with a height // lower than our greatest seen height @@ -353,13 +353,13 @@ void CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const CTxM } // Feerates are stored and reported as BTC-per-kb: - CFeeRate feeRate(entry.GetFee(), entry.GetTxSize()); + CFeeRate feeRate(entry->GetFee(), entry->GetTxSize()); feeStats.Record(blocksToConfirm, (double)feeRate.GetFeePerK()); } void CBlockPolicyEstimator::processBlock(unsigned int nBlockHeight, - std::vector& entries) + std::vector& entries) { if (nBlockHeight <= nBestSeenHeight) { // Ignore side chains and re-orgs; assuming they are random diff --git a/src/policy/fees.h b/src/policy/fees.h index a61ae1813..e1684ebcb 100644 --- a/src/policy/fees.h +++ b/src/policy/fees.h @@ -203,10 +203,10 @@ public: /** Process all the transactions that have been included in a block */ void processBlock(unsigned int nBlockHeight, - std::vector& entries); + std::vector& entries); /** Process a transaction confirmed in a block*/ - void processBlockTx(unsigned int nBlockHeight, const CTxMemPoolEntry& entry); + void processBlockTx(unsigned int nBlockHeight, const CTxMemPoolEntry* entry); /** Process a transaction accepted to the mempool*/ void processTransaction(const CTxMemPoolEntry& entry, bool validFeeEstimate); diff --git a/src/txmempool.cpp b/src/txmempool.cpp index e97099eb2..4f4540a1f 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -594,14 +594,14 @@ void CTxMemPool::removeConflicts(const CTransaction &tx) void CTxMemPool::removeForBlock(const std::vector& vtx, unsigned int nBlockHeight) { LOCK(cs); - std::vector entries; + std::vector entries; for (const auto& tx : vtx) { uint256 hash = tx->GetHash(); indexed_transaction_set::iterator i = mapTx.find(hash); if (i != mapTx.end()) - entries.push_back(*i); + entries.push_back(&*i); } // Before the txs in the new block have been removed from the mempool, update policy estimates minerPolicyEstimator->processBlock(nBlockHeight, entries); From dc008c462f6df84dd444c9646f7ca64ee1c8c841 Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Fri, 11 Nov 2016 13:29:13 -0500 Subject: [PATCH 07/10] Add IsCurrentForFeeEstimatation Make a more conservative notion of whether the node is caught up to the rest of the network and only count transactions as fee estimation data points if the node is caught up. --- src/validation.cpp | 19 ++++++++++++++++++- src/validation.h | 2 ++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/validation.cpp b/src/validation.cpp index fb6a902bc..264fdd83d 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -525,6 +525,18 @@ std::string FormatStateMessage(const CValidationState &state) state.GetRejectCode()); } +static bool IsCurrentForFeeEstimation() +{ + AssertLockHeld(cs_main); + if (IsInitialBlockDownload()) + return false; + if (chainActive.Tip()->GetBlockTime() < (GetTime() - MAX_FEE_ESTIMATION_TIP_AGE)) + return false; + if (chainActive.Height() < pindexBestHeader->nHeight - 1) + return false; + return true; +} + bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const CTransactionRef& ptx, bool fLimitFree, bool* pfMissingInputs, int64_t nAcceptTime, bool fOverrideMempoolLimit, const CAmount& nAbsurdFee, std::vector& vHashTxnToUncache) @@ -941,8 +953,13 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C } pool.RemoveStaged(allConflicting, false); + // This transaction should only count for fee estimation if + // the node is not behind and it is not dependent on any other + // transactions in the mempool + bool validForFeeEstimation = IsCurrentForFeeEstimation() && pool.HasNoInputsOf(tx); + // Store transaction in memory - pool.addUnchecked(hash, entry, setAncestors, !IsInitialBlockDownload() && pool.HasNoInputsOf(tx)); + pool.addUnchecked(hash, entry, setAncestors, validForFeeEstimation); // trim mempool and check if tx was trimmed if (!fOverrideMempoolLimit) { diff --git a/src/validation.h b/src/validation.h index 981f65963..0f421d59e 100644 --- a/src/validation.h +++ b/src/validation.h @@ -130,6 +130,8 @@ static const int64_t BLOCK_DOWNLOAD_TIMEOUT_PER_PEER = 500000; static const unsigned int DEFAULT_LIMITFREERELAY = 0; static const bool DEFAULT_RELAYPRIORITY = true; static const int64_t DEFAULT_MAX_TIP_AGE = 24 * 60 * 60; +/** Maximum age of our tip for us to be considered current for fee estimation */ +static const int64_t MAX_FEE_ESTIMATION_TIP_AGE = 3 * 60 * 60; /** Default for -permitbaremultisig */ static const bool DEFAULT_PERMIT_BAREMULTISIG = true; From 5fe0f47aa7d4cd4ed58ad37e6e4035f5c0625a23 Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Fri, 11 Nov 2016 13:40:27 -0500 Subject: [PATCH 08/10] Add extra logging to processBlock in fee estimation. --- src/policy/fees.cpp | 28 +++++++++++++++++++--------- src/policy/fees.h | 5 ++++- 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index cb83bcf71..ec1dd492c 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -294,7 +294,7 @@ bool CBlockPolicyEstimator::removeTx(uint256 hash) } CBlockPolicyEstimator::CBlockPolicyEstimator(const CFeeRate& _minRelayFee) - : nBestSeenHeight(0) + : nBestSeenHeight(0), trackedTxs(0), untrackedTxs(0) { static_assert(MIN_FEERATE > 0, "Min feerate must be nonzero"); minTrackedFee = _minRelayFee < CFeeRate(MIN_FEERATE) ? CFeeRate(MIN_FEERATE) : _minRelayFee; @@ -324,8 +324,11 @@ void CBlockPolicyEstimator::processTransaction(const CTxMemPoolEntry& entry, boo // Only want to be updating estimates when our blockchain is synced, // otherwise we'll miscalculate how many blocks its taking to get included. - if (!validFeeEstimate) + if (!validFeeEstimate) { + untrackedTxs++; return; + } + trackedTxs++; // Feerates are stored and reported as BTC-per-kb: CFeeRate feeRate(entry.GetFee(), entry.GetTxSize()); @@ -334,11 +337,11 @@ void CBlockPolicyEstimator::processTransaction(const CTxMemPoolEntry& entry, boo mapMemPoolTxs[hash].bucketIndex = feeStats.NewTx(txHeight, (double)feeRate.GetFeePerK()); } -void CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const CTxMemPoolEntry* entry) +bool CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const CTxMemPoolEntry* entry) { if (!removeTx(entry->GetTx().GetHash())) { // This transaction wasn't being tracked for fee estimation - return; + return false; } // How many blocks did it take for miners to include this transaction? @@ -349,13 +352,14 @@ void CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const CTxM // This can't happen because we don't process transactions from a block with a height // lower than our greatest seen height LogPrint("estimatefee", "Blockpolicy error Transaction had negative blocksToConfirm\n"); - return; + return false; } // Feerates are stored and reported as BTC-per-kb: CFeeRate feeRate(entry->GetFee(), entry->GetTxSize()); feeStats.Record(blocksToConfirm, (double)feeRate.GetFeePerK()); + return true; } void CBlockPolicyEstimator::processBlock(unsigned int nBlockHeight, @@ -378,15 +382,21 @@ void CBlockPolicyEstimator::processBlock(unsigned int nBlockHeight, // Clear the current block state and update unconfirmed circular buffer feeStats.ClearCurrent(nBlockHeight); + unsigned int countedTxs = 0; // Repopulate the current block states - for (unsigned int i = 0; i < entries.size(); i++) - processBlockTx(nBlockHeight, entries[i]); + for (unsigned int i = 0; i < entries.size(); i++) { + if (processBlockTx(nBlockHeight, entries[i])) + countedTxs++; + } // Update all exponential averages with the current block state feeStats.UpdateMovingAverages(); - LogPrint("estimatefee", "Blockpolicy after updating estimates for %u confirmed entries, new mempool map size %u\n", - entries.size(), mapMemPoolTxs.size()); + LogPrint("estimatefee", "Blockpolicy after updating estimates for %u of %u txs in block, since last block %u of %u tracked, new mempool map size %u\n", + countedTxs, entries.size(), trackedTxs, trackedTxs + untrackedTxs, mapMemPoolTxs.size()); + + trackedTxs = 0; + untrackedTxs = 0; } CFeeRate CBlockPolicyEstimator::estimateFee(int confTarget) diff --git a/src/policy/fees.h b/src/policy/fees.h index e1684ebcb..064466afe 100644 --- a/src/policy/fees.h +++ b/src/policy/fees.h @@ -206,7 +206,7 @@ public: std::vector& entries); /** Process a transaction confirmed in a block*/ - void processBlockTx(unsigned int nBlockHeight, const CTxMemPoolEntry* entry); + bool processBlockTx(unsigned int nBlockHeight, const CTxMemPoolEntry* entry); /** Process a transaction accepted to the mempool*/ void processTransaction(const CTxMemPoolEntry& entry, bool validFeeEstimate); @@ -258,6 +258,9 @@ private: /** Classes to track historical data on transaction confirmations */ TxConfirmStats feeStats; + + unsigned int trackedTxs; + unsigned int untrackedTxs; }; class FeeFilterRounder From 78ae62d2646c5e6db410f324046d0c3b15e0ad0a Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Tue, 29 Nov 2016 13:55:26 -0500 Subject: [PATCH 09/10] Add clarifying comments to fee estimation --- src/policy/fees.cpp | 5 +++++ src/validation.h | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index ec1dd492c..f1c93a497 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -281,6 +281,11 @@ void TxConfirmStats::removeTx(unsigned int entryHeight, unsigned int nBestSeenHe } } +// This function is called from CTxMemPool::removeUnchecked to ensure +// txs removed from the mempool for any reason are no longer +// tracked. Txs that were part of a block have already been removed in +// processBlockTx to ensure they are never double tracked, but it is +// of no harm to try to remove them again. bool CBlockPolicyEstimator::removeTx(uint256 hash) { std::map::iterator pos = mapMemPoolTxs.find(hash); diff --git a/src/validation.h b/src/validation.h index 0f421d59e..b41fa8aad 100644 --- a/src/validation.h +++ b/src/validation.h @@ -130,7 +130,7 @@ static const int64_t BLOCK_DOWNLOAD_TIMEOUT_PER_PEER = 500000; static const unsigned int DEFAULT_LIMITFREERELAY = 0; static const bool DEFAULT_RELAYPRIORITY = true; static const int64_t DEFAULT_MAX_TIP_AGE = 24 * 60 * 60; -/** Maximum age of our tip for us to be considered current for fee estimation */ +/** Maximum age of our tip in seconds for us to be considered current for fee estimation */ static const int64_t MAX_FEE_ESTIMATION_TIP_AGE = 3 * 60 * 60; /** Default for -permitbaremultisig */ From 44b64b933dec10f5ffcd7f34e7bf5e4ed97f671e Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Tue, 29 Nov 2016 15:40:03 -0500 Subject: [PATCH 10/10] Fix edge case with stale fee estimates --- src/policy/fees.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index f1c93a497..5407aefb4 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -321,9 +321,11 @@ void CBlockPolicyEstimator::processTransaction(const CTxMemPoolEntry& entry, boo return; } - if (txHeight < nBestSeenHeight) { + if (txHeight != nBestSeenHeight) { // Ignore side chains and re-orgs; assuming they are random they don't // affect the estimate. We'll potentially double count transactions in 1-block reorgs. + // Ignore txs if BlockPolicyEstimator is not in sync with chainActive.Tip(). + // It will be synced next time a block is processed. return; }