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 a1b785e3e..5407aefb4 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -281,23 +281,25 @@ void TxConfirmStats::removeTx(unsigned int entryHeight, unsigned int nBestSeenHe } } -void CBlockPolicyEstimator::removeTx(uint256 hash) +// 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); - 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) - : 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; @@ -309,7 +311,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(); @@ -319,23 +321,21 @@ 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; } // 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; - - 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 + if (!validFeeEstimate) { + untrackedTxs++; return; } + trackedTxs++; // Feerates are stored and reported as BTC-per-kb: CFeeRate feeRate(entry.GetFee(), entry.GetTxSize()); @@ -344,34 +344,33 @@ 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 (!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 - return; + if (!removeTx(entry->GetTx().GetHash())) { + // This transaction wasn't being tracked for fee estimation + return false; } // 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 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()); + CFeeRate feeRate(entry->GetFee(), entry->GetTxSize()); feeStats.Record(blocksToConfirm, (double)feeRate.GetFeePerK()); + return true; } 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 @@ -381,25 +380,30 @@ void CBlockPolicyEstimator::processBlock(unsigned int nBlockHeight, // transaction fees." 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. 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 + // 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 1f10cfb23..064466afe 100644 --- a/src/policy/fees.h +++ b/src/policy/fees.h @@ -203,16 +203,16 @@ 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); + bool 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*/ - void removeTx(uint256 hash); + bool removeTx(uint256 hash); /** Return a feerate estimate */ CFeeRate estimateFee(int confTarget); @@ -258,6 +258,9 @@ private: /** Classes to track historical data on transaction confirmations */ TxConfirmStats feeStats; + + unsigned int trackedTxs; + unsigned int untrackedTxs; }; class FeeFilterRounder 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..4f4540a1f 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); @@ -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; @@ -591,19 +591,20 @@ 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; + 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); for (const auto& tx : vtx) { txiter it = mapTx.find(tx->GetHash()); @@ -615,8 +616,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; } @@ -1015,14 +1014,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 3f8ab5950..6a00b540a 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; } @@ -525,14 +523,13 @@ 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); 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 f5f685112..648fed8bc 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) @@ -692,7 +704,8 @@ 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(), + inChainInputValue, fSpendsCoinbase, nSigOpsCost, lp); unsigned int nSize = entry.GetTxSize(); // Check that the transaction doesn't have an excessive number of @@ -940,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.addUnchecked(hash, entry, setAncestors, validForFeeEstimation); // trim mempool and check if tx was trimmed if (!fOverrideMempoolLimit) { @@ -2206,7 +2224,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); diff --git a/src/validation.h b/src/validation.h index 97fc9df1f..631602a70 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 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 */ static const bool DEFAULT_PERMIT_BAREMULTISIG = true; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 53b031094..ff7a03bc5 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2578,7 +2578,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;