From a0e3eb7549d2ba4dd3af12b9ce65e29158f59078 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Fri, 3 Nov 2023 11:01:52 +0100 Subject: [PATCH 1/7] tx fees, policy: bugfix: move `removeTx` into reason != `BLOCK` condition If the removal reason of a transaction is BLOCK, then the `removeTx` boolean argument should be true. Before this PR, `CBlockPolicyEstimator` have to complete updating the fee stats before the mempool clears that's why having removeTx call outside reason!= `BLOCK` in `addUnchecked` was not a bug. But in a case where the `CBlockPolicyEstimator` update is asynchronous, the mempool might clear before we update the `CBlockPolicyEstimator` fee stats. Transactions that are removed for `BLOCK` reasons will also be incorrectly removed from `CBlockPolicyEstimator` stats as failures. --- src/txmempool.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index e057d7ece1..7fd9c1cc25 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -497,12 +497,16 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason) // even if not directly reported below. uint64_t mempool_sequence = GetAndIncrementSequence(); + const auto& hash = it->GetTx().GetHash(); if (reason != MemPoolRemovalReason::BLOCK) { // Notify clients that a transaction has been removed from the mempool // for any reason except being included in a block. Clients interested // in transactions included in blocks can subscribe to the BlockConnected // notification. GetMainSignals().TransactionRemovedFromMempool(it->GetSharedTx(), reason, mempool_sequence); + if (minerPolicyEstimator) { + minerPolicyEstimator->removeTx(hash, false); + } } TRACE5(mempool, removed, it->GetTx().GetHash().data(), @@ -512,7 +516,6 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason) std::chrono::duration_cast>(it->GetTime()).count() ); - const uint256 hash = it->GetTx().GetHash(); for (const CTxIn& txin : it->GetTx().vin) mapNextTx.erase(txin.prevout); @@ -535,7 +538,6 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason) cachedInnerUsage -= memusage::DynamicUsage(it->GetMemPoolParentsConst()) + memusage::DynamicUsage(it->GetMemPoolChildrenConst()); mapTx.erase(it); nTransactionsUpdated++; - if (minerPolicyEstimator) {minerPolicyEstimator->removeTx(hash, false);} } // Calculates descendants of entry that are not already in setDescendants, and adds to From 0889e07987294d4ef2814abfca16d8e2a0c5f541 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Fri, 1 Sep 2023 11:03:21 +0100 Subject: [PATCH 2/7] tx fees, policy: cast with static_cast instead of C-Style cast --- src/policy/fees.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index 654c4cf0ce..df5885b059 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -610,11 +610,11 @@ void CBlockPolicyEstimator::processTransaction(const CTxMemPoolEntry& entry, boo CFeeRate feeRate(entry.GetFee(), entry.GetTxSize()); mapMemPoolTxs[hash].blockHeight = txHeight; - unsigned int bucketIndex = feeStats->NewTx(txHeight, (double)feeRate.GetFeePerK()); + unsigned int bucketIndex = feeStats->NewTx(txHeight, static_cast(feeRate.GetFeePerK())); mapMemPoolTxs[hash].bucketIndex = bucketIndex; - unsigned int bucketIndex2 = shortStats->NewTx(txHeight, (double)feeRate.GetFeePerK()); + unsigned int bucketIndex2 = shortStats->NewTx(txHeight, static_cast(feeRate.GetFeePerK())); assert(bucketIndex == bucketIndex2); - unsigned int bucketIndex3 = longStats->NewTx(txHeight, (double)feeRate.GetFeePerK()); + unsigned int bucketIndex3 = longStats->NewTx(txHeight, static_cast(feeRate.GetFeePerK())); assert(bucketIndex == bucketIndex3); } @@ -640,9 +640,9 @@ bool CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const CTxM // Feerates are stored and reported as BTC-per-kb: CFeeRate feeRate(entry->GetFee(), entry->GetTxSize()); - feeStats->Record(blocksToConfirm, (double)feeRate.GetFeePerK()); - shortStats->Record(blocksToConfirm, (double)feeRate.GetFeePerK()); - longStats->Record(blocksToConfirm, (double)feeRate.GetFeePerK()); + feeStats->Record(blocksToConfirm, static_cast(feeRate.GetFeePerK())); + shortStats->Record(blocksToConfirm, static_cast(feeRate.GetFeePerK())); + longStats->Record(blocksToConfirm, static_cast(feeRate.GetFeePerK())); return true; } From bfcd401368fc0dc43827a8969a37b7e038d5ca79 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Fri, 3 Nov 2023 12:34:29 +0100 Subject: [PATCH 3/7] CValidationInterface, mempool: add new callback to `CValidationInterface` This commit adds a new callback `MempoolTransactionsRemovedForBlock` which notify its listeners of the transactions that are removed from the mempool because a new block is connected, along with the block height the transactions were removed. The transactions are in `RemovedMempoolTransactionInfo` format. `CTransactionRef`, base fee, virtual size, and height which the transaction was added to the mempool are all members of the struct called `RemovedMempoolTransactionInfo`. A struct `NewMempoolTransactionInfo`, which has fields similar to `RemovedMempoolTransactionInfo`, will be added in a later commit, create a struct `TransactionInfo` with all similar fields. They can both have a member with type `TransactionInfo`. --- src/kernel/mempool_entry.h | 30 ++++++++++++++++++++++++++++++ src/txmempool.cpp | 4 ++++ src/validationinterface.cpp | 11 +++++++++++ src/validationinterface.h | 17 ++++++++++++++--- 4 files changed, 59 insertions(+), 3 deletions(-) diff --git a/src/kernel/mempool_entry.h b/src/kernel/mempool_entry.h index 7c905ca4f4..ce32fe20dc 100644 --- a/src/kernel/mempool_entry.h +++ b/src/kernel/mempool_entry.h @@ -178,4 +178,34 @@ public: using CTxMemPoolEntryRef = CTxMemPoolEntry::CTxMemPoolEntryRef; +struct TransactionInfo { + const CTransactionRef m_tx; + /* The fee the transaction paid */ + const CAmount m_fee; + /** + * The virtual transaction size. + * + * This is a policy field which considers the sigop cost of the + * transaction as well as its weight, and reinterprets it as bytes. + * + * It is the primary metric by which the mining algorithm selects + * transactions. + */ + const int64_t m_virtual_transaction_size; + /* The block height the transaction entered the mempool */ + const unsigned int txHeight; + + TransactionInfo(const CTransactionRef& tx, const CAmount& fee, const int64_t vsize, const unsigned int height) + : m_tx{tx}, + m_fee{fee}, + m_virtual_transaction_size{vsize}, + txHeight{height} {} +}; + +struct RemovedMempoolTransactionInfo { + TransactionInfo info; + explicit RemovedMempoolTransactionInfo(const CTxMemPoolEntry& entry) + : info{entry.GetSharedTx(), entry.GetFee(), entry.GetTxSize(), entry.GetHeight()} {} +}; + #endif // BITCOIN_KERNEL_MEMPOOL_ENTRY_H diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 7fd9c1cc25..f75dd7efea 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -654,17 +654,21 @@ void CTxMemPool::removeForBlock(const std::vector& vtx, unsigne } // Before the txs in the new block have been removed from the mempool, update policy estimates if (minerPolicyEstimator) {minerPolicyEstimator->processBlock(nBlockHeight, entries);} + std::vector txs_removed_for_block; + txs_removed_for_block.reserve(vtx.size()); for (const auto& tx : vtx) { txiter it = mapTx.find(tx->GetHash()); if (it != mapTx.end()) { setEntries stage; stage.insert(it); + txs_removed_for_block.emplace_back(*it); RemoveStaged(stage, true, MemPoolRemovalReason::BLOCK); } removeConflicts(*tx); ClearPrioritisation(tx->GetHash()); } + GetMainSignals().MempoolTransactionsRemovedForBlock(txs_removed_for_block, nBlockHeight); lastRollingFeeUpdate = GetTime(); blockSinceLastRollingFeeBump = true; } diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 9241395ad5..893ef69582 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -233,6 +234,16 @@ void CMainSignals::BlockConnected(ChainstateRole role, const std::shared_ptrnHeight); } +void CMainSignals::MempoolTransactionsRemovedForBlock(const std::vector& txs_removed_for_block, unsigned int nBlockHeight) +{ + auto event = [txs_removed_for_block, nBlockHeight, this] { + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.MempoolTransactionsRemovedForBlock(txs_removed_for_block, nBlockHeight); }); + }; + ENQUEUE_AND_LOG_EVENT(event, "%s: block height=%s txs removed=%s", __func__, + nBlockHeight, + txs_removed_for_block.size()); +} + void CMainSignals::BlockDisconnected(const std::shared_ptr& pblock, const CBlockIndex* pindex) { auto event = [pblock, pindex, this] { diff --git a/src/validationinterface.h b/src/validationinterface.h index eb15aa4d5f..ea49a45aa8 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -21,6 +21,7 @@ struct CBlockLocator; class CValidationInterface; class CScheduler; enum class MemPoolRemovalReason; +struct RemovedMempoolTransactionInfo; /** Register subscriber */ void RegisterValidationInterface(CValidationInterface* callbacks); @@ -60,10 +61,10 @@ void CallFunctionInValidationInterfaceQueue(std::function func); void SyncWithValidationInterfaceQueue() LOCKS_EXCLUDED(cs_main); /** - * Implement this to subscribe to events generated in validation + * Implement this to subscribe to events generated in validation and mempool * * Each CValidationInterface() subscriber will receive event callbacks - * in the order in which the events were generated by validation. + * in the order in which the events were generated by validation and mempool. * Furthermore, each ValidationInterface() subscriber may assume that * callbacks effectively run in a single thread with single-threaded * memory consistency. That is, for a given ValidationInterface() @@ -113,7 +114,7 @@ protected: * This does not fire for transactions that are removed from the mempool * because they have been included in a block. Any client that is interested * in transactions removed from the mempool for inclusion in a block can learn - * about those transactions from the BlockConnected notification. + * about those transactions from the MempoolTransactionsRemovedForBlock notification. * * Transactions that are removed from the mempool because they conflict * with a transaction in the new block will have @@ -131,6 +132,14 @@ protected: * Called on a background thread. */ virtual void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence) {} + /* + * Notifies listeners of transactions removed from the mempool as + * as a result of new block being connected. + * MempoolTransactionsRemovedForBlock will be fired before BlockConnected. + * + * Called on a background thread. + */ + virtual void MempoolTransactionsRemovedForBlock(const std::vector& txs_removed_for_block, unsigned int nBlockHeight) {} /** * Notifies listeners of a block being connected. * Provides a vector of transactions evicted from the mempool as a result. @@ -140,6 +149,7 @@ protected: virtual void BlockConnected(ChainstateRole role, const std::shared_ptr &block, const CBlockIndex *pindex) {} /** * Notifies listeners of a block being disconnected + * Provides the block that was connected. * * Called on a background thread. Only called for the active chainstate, since * background chainstates should never disconnect blocks. @@ -202,6 +212,7 @@ public: void UpdatedBlockTip(const CBlockIndex *, const CBlockIndex *, bool fInitialDownload); void TransactionAddedToMempool(const CTransactionRef&, uint64_t mempool_sequence); void TransactionRemovedFromMempool(const CTransactionRef&, MemPoolRemovalReason, uint64_t mempool_sequence); + void MempoolTransactionsRemovedForBlock(const std::vector&, unsigned int nBlockHeight); void BlockConnected(ChainstateRole, const std::shared_ptr &, const CBlockIndex *pindex); void BlockDisconnected(const std::shared_ptr &, const CBlockIndex* pindex); void ChainStateFlushed(ChainstateRole, const CBlockLocator &); From 91532bd38223d7d04166e05de11d0d0b55e60f13 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Fri, 3 Nov 2023 13:23:30 +0100 Subject: [PATCH 4/7] tx fees, policy: update `CBlockPolicyEstimator::processBlock` parameter Update `processBlock` parameter to reference to a vector of `RemovedMempoolTransactionInfo`. --- src/policy/fees.cpp | 18 +++++++++--------- src/policy/fees.h | 7 ++++--- src/test/fuzz/policy_estimator.cpp | 8 ++++---- src/txmempool.cpp | 15 ++++----------- 4 files changed, 21 insertions(+), 27 deletions(-) diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index df5885b059..6d550b1d5a 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -618,10 +618,10 @@ void CBlockPolicyEstimator::processTransaction(const CTxMemPoolEntry& entry, boo assert(bucketIndex == bucketIndex3); } -bool CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const CTxMemPoolEntry* entry) +bool CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const RemovedMempoolTransactionInfo& tx) { AssertLockHeld(m_cs_fee_estimator); - if (!_removeTx(entry->GetTx().GetHash(), true)) { + if (!_removeTx(tx.info.m_tx->GetHash(), true)) { // This transaction wasn't being tracked for fee estimation return false; } @@ -629,7 +629,7 @@ bool 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 - tx.info.txHeight; 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 @@ -638,7 +638,7 @@ bool CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const CTxM } // Feerates are stored and reported as BTC-per-kb: - CFeeRate feeRate(entry->GetFee(), entry->GetTxSize()); + CFeeRate feeRate(tx.info.m_fee, tx.info.m_virtual_transaction_size); feeStats->Record(blocksToConfirm, static_cast(feeRate.GetFeePerK())); shortStats->Record(blocksToConfirm, static_cast(feeRate.GetFeePerK())); @@ -646,8 +646,8 @@ bool CBlockPolicyEstimator::processBlockTx(unsigned int nBlockHeight, const CTxM return true; } -void CBlockPolicyEstimator::processBlock(unsigned int nBlockHeight, - std::vector& entries) +void CBlockPolicyEstimator::processBlock(const std::vector& txs_removed_for_block, + unsigned int nBlockHeight) { LOCK(m_cs_fee_estimator); if (nBlockHeight <= nBestSeenHeight) { @@ -676,8 +676,8 @@ void CBlockPolicyEstimator::processBlock(unsigned int nBlockHeight, unsigned int countedTxs = 0; // Update averages with data points from current block - for (const auto& entry : entries) { - if (processBlockTx(nBlockHeight, entry)) + for (const auto& tx : txs_removed_for_block) { + if (processBlockTx(nBlockHeight, tx)) countedTxs++; } @@ -688,7 +688,7 @@ void CBlockPolicyEstimator::processBlock(unsigned int nBlockHeight, LogPrint(BCLog::ESTIMATEFEE, "Blockpolicy estimates updated by %u of %u block txs, since last block %u of %u tracked, mempool map size %u, max target %u from %s\n", - countedTxs, entries.size(), trackedTxs, trackedTxs + untrackedTxs, mapMemPoolTxs.size(), + countedTxs, txs_removed_for_block.size(), trackedTxs, trackedTxs + untrackedTxs, mapMemPoolTxs.size(), MaxUsableEstimate(), HistoricalBlockSpan() > BlockSpan() ? "historical" : "current"); trackedTxs = 0; diff --git a/src/policy/fees.h b/src/policy/fees.h index 69bda195be..cff0c4dbe2 100644 --- a/src/policy/fees.h +++ b/src/policy/fees.h @@ -37,6 +37,7 @@ static constexpr bool DEFAULT_ACCEPT_STALE_FEE_ESTIMATES{false}; class AutoFile; class CTxMemPoolEntry; class TxConfirmStats; +struct RemovedMempoolTransactionInfo; /* Identifier for each of the 3 different TxConfirmStats which will track * history over different time horizons. */ @@ -201,8 +202,8 @@ public: ~CBlockPolicyEstimator(); /** Process all the transactions that have been included in a block */ - void processBlock(unsigned int nBlockHeight, - std::vector& entries) + void processBlock(const std::vector& txs_removed_for_block, + unsigned int nBlockHeight) EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); /** Process a transaction accepted to the mempool*/ @@ -290,7 +291,7 @@ private: std::map bucketMap GUARDED_BY(m_cs_fee_estimator); // Map of bucket upper-bound to index into all vectors by bucket /** Process a transaction confirmed in a block*/ - bool processBlockTx(unsigned int nBlockHeight, const CTxMemPoolEntry* entry) EXCLUSIVE_LOCKS_REQUIRED(m_cs_fee_estimator); + bool processBlockTx(unsigned int nBlockHeight, const RemovedMempoolTransactionInfo& tx) EXCLUSIVE_LOCKS_REQUIRED(m_cs_fee_estimator); /** Helper for estimateSmartFee */ double estimateCombinedFee(unsigned int confTarget, double successThreshold, bool checkShorterHorizon, EstimationResult *result) const EXCLUSIVE_LOCKS_REQUIRED(m_cs_fee_estimator); diff --git a/src/test/fuzz/policy_estimator.cpp b/src/test/fuzz/policy_estimator.cpp index 220799be41..e5e0b0d8da 100644 --- a/src/test/fuzz/policy_estimator.cpp +++ b/src/test/fuzz/policy_estimator.cpp @@ -61,12 +61,12 @@ FUZZ_TARGET(policy_estimator, .init = initialize_policy_estimator) const CTransaction tx{*mtx}; mempool_entries.push_back(ConsumeTxMemPoolEntry(fuzzed_data_provider, tx)); } - std::vector ptrs; - ptrs.reserve(mempool_entries.size()); + std::vector txs; + txs.reserve(mempool_entries.size()); for (const CTxMemPoolEntry& mempool_entry : mempool_entries) { - ptrs.push_back(&mempool_entry); + txs.emplace_back(mempool_entry); } - block_policy_estimator.processBlock(fuzzed_data_provider.ConsumeIntegral(), ptrs); + block_policy_estimator.processBlock(txs, fuzzed_data_provider.ConsumeIntegral()); }, [&] { (void)block_policy_estimator.removeTx(ConsumeUInt256(fuzzed_data_provider), /*inBlock=*/fuzzed_data_provider.ConsumeBool()); diff --git a/src/txmempool.cpp b/src/txmempool.cpp index f75dd7efea..f5036a9301 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -643,17 +643,6 @@ void CTxMemPool::removeConflicts(const CTransaction &tx) void CTxMemPool::removeForBlock(const std::vector& vtx, unsigned int nBlockHeight) { AssertLockHeld(cs); - 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); - } - // Before the txs in the new block have been removed from the mempool, update policy estimates - if (minerPolicyEstimator) {minerPolicyEstimator->processBlock(nBlockHeight, entries);} std::vector txs_removed_for_block; txs_removed_for_block.reserve(vtx.size()); for (const auto& tx : vtx) @@ -668,6 +657,10 @@ void CTxMemPool::removeForBlock(const std::vector& vtx, unsigne removeConflicts(*tx); ClearPrioritisation(tx->GetHash()); } + // Update policy estimates + if (minerPolicyEstimator) { + minerPolicyEstimator->processBlock(txs_removed_for_block, nBlockHeight); + } GetMainSignals().MempoolTransactionsRemovedForBlock(txs_removed_for_block, nBlockHeight); lastRollingFeeUpdate = GetTime(); blockSinceLastRollingFeeBump = true; From dff5ad3b9944cbb56126ba37a8da180d1327ba39 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Fri, 3 Nov 2023 17:04:30 +0100 Subject: [PATCH 5/7] CValidationInterface: modify the parameter of `TransactionAddedToMempool` Create a new struct `NewMempoolTransactionInfo` that will be used as the new parameter of `TransactionAddedToMempool` callback. --- src/kernel/mempool_entry.h | 29 ++++++++++++++++++++++++++++ src/node/interfaces.cpp | 4 ++-- src/test/fuzz/package_eval.cpp | 10 +++++----- src/test/fuzz/tx_pool.cpp | 4 ++-- src/validation.cpp | 16 +++++++++++++-- src/validationinterface.cpp | 7 ++++--- src/validationinterface.h | 5 +++-- src/zmq/zmqnotificationinterface.cpp | 5 +++-- src/zmq/zmqnotificationinterface.h | 3 ++- 9 files changed, 64 insertions(+), 19 deletions(-) diff --git a/src/kernel/mempool_entry.h b/src/kernel/mempool_entry.h index ce32fe20dc..5824a4576b 100644 --- a/src/kernel/mempool_entry.h +++ b/src/kernel/mempool_entry.h @@ -208,4 +208,33 @@ struct RemovedMempoolTransactionInfo { : info{entry.GetSharedTx(), entry.GetFee(), entry.GetTxSize(), entry.GetHeight()} {} }; +struct NewMempoolTransactionInfo { + TransactionInfo info; + /* + * This boolean indicates whether the transaction was added + * without enforcing mempool fee limits. + */ + const bool m_from_disconnected_block; + /* This boolean indicates whether the transaction is part of a package. */ + const bool m_submitted_in_package; + /* + * This boolean indicates whether the blockchain is up to date when the + * transaction is added to the mempool. + */ + const bool m_chainstate_is_current; + /* Indicates whether the transaction has unconfirmed parents. */ + const bool m_has_no_mempool_parents; + + explicit NewMempoolTransactionInfo(const CTransactionRef& tx, const CAmount& fee, + const int64_t vsize, const unsigned int height, + const bool from_disconnected_block, const bool submitted_in_package, + const bool chainstate_is_current, + const bool has_no_mempool_parents) + : info{tx, fee, vsize, height}, + m_from_disconnected_block{from_disconnected_block}, + m_submitted_in_package{submitted_in_package}, + m_chainstate_is_current{chainstate_is_current}, + m_has_no_mempool_parents{has_no_mempool_parents} {} +}; + #endif // BITCOIN_KERNEL_MEMPOOL_ENTRY_H diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index f4ecfeb9d5..b5691f0e57 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -428,9 +428,9 @@ public: explicit NotificationsProxy(std::shared_ptr notifications) : m_notifications(std::move(notifications)) {} virtual ~NotificationsProxy() = default; - void TransactionAddedToMempool(const CTransactionRef& tx, uint64_t mempool_sequence) override + void TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t mempool_sequence) override { - m_notifications->transactionAddedToMempool(tx); + m_notifications->transactionAddedToMempool(tx.info.m_tx); } void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence) override { diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp index 8658c0b45a..021acc02f2 100644 --- a/src/test/fuzz/package_eval.cpp +++ b/src/test/fuzz/package_eval.cpp @@ -55,13 +55,13 @@ struct OutpointsUpdater final : public CValidationInterface { explicit OutpointsUpdater(std::set& r) : m_mempool_outpoints{r} {} - void TransactionAddedToMempool(const CTransactionRef& tx, uint64_t /* mempool_sequence */) override + void TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t /* mempool_sequence */) override { // for coins spent we always want to be able to rbf so they're not removed // outputs from this tx can now be spent - for (uint32_t index{0}; index < tx->vout.size(); ++index) { - m_mempool_outpoints.insert(COutPoint{tx->GetHash(), index}); + for (uint32_t index{0}; index < tx.info.m_tx->vout.size(); ++index) { + m_mempool_outpoints.insert(COutPoint{tx.info.m_tx->GetHash(), index}); } } @@ -85,10 +85,10 @@ struct TransactionsDelta final : public CValidationInterface { explicit TransactionsDelta(std::set& a) : m_added{a} {} - void TransactionAddedToMempool(const CTransactionRef& tx, uint64_t /* mempool_sequence */) override + void TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t /* mempool_sequence */) override { // Transactions may be entered and booted any number of times - m_added.insert(tx); + m_added.insert(tx.info.m_tx); } void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t /* mempool_sequence */) override diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index 96095539ec..001b452722 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -59,9 +59,9 @@ struct TransactionsDelta final : public CValidationInterface { explicit TransactionsDelta(std::set& r, std::set& a) : m_removed{r}, m_added{a} {} - void TransactionAddedToMempool(const CTransactionRef& tx, uint64_t /* mempool_sequence */) override + void TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t /* mempool_sequence */) override { - Assert(m_added.insert(tx).second); + Assert(m_added.insert(tx.info.m_tx).second); } void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t /* mempool_sequence */) override diff --git a/src/validation.cpp b/src/validation.cpp index ed72a7c97a..1c95ba08c5 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1222,7 +1222,13 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector& results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, ws.m_base_fees, effective_feerate, effective_feerate_wtxids)); - GetMainSignals().TransactionAddedToMempool(ws.m_ptx, m_pool.GetAndIncrementSequence()); + const CTransaction& tx = *ws.m_ptx; + const auto tx_info = NewMempoolTransactionInfo(ws.m_ptx, ws.m_base_fees, + ws.m_vsize, ws.m_entry->GetHeight(), + args.m_bypass_limits, args.m_package_submission, + IsCurrentForFeeEstimation(m_active_chainstate), + m_pool.HasNoInputsOf(tx)); + GetMainSignals().TransactionAddedToMempool(tx_info, m_pool.GetAndIncrementSequence()); } return all_submitted; } @@ -1265,7 +1271,13 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef return MempoolAcceptResult::FeeFailure(ws.m_state, CFeeRate(ws.m_modified_fees, ws.m_vsize), {ws.m_ptx->GetWitnessHash()}); } - GetMainSignals().TransactionAddedToMempool(ptx, m_pool.GetAndIncrementSequence()); + const CTransaction& tx = *ws.m_ptx; + const auto tx_info = NewMempoolTransactionInfo(ws.m_ptx, ws.m_base_fees, + ws.m_vsize, ws.m_entry->GetHeight(), + args.m_bypass_limits, args.m_package_submission, + IsCurrentForFeeEstimation(m_active_chainstate), + m_pool.HasNoInputsOf(tx)); + GetMainSignals().TransactionAddedToMempool(tx_info, m_pool.GetAndIncrementSequence()); return MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, ws.m_base_fees, effective_feerate, single_wtxid); diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 893ef69582..5e944a7c47 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -206,13 +206,14 @@ void CMainSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockInd fInitialDownload); } -void CMainSignals::TransactionAddedToMempool(const CTransactionRef& tx, uint64_t mempool_sequence) { +void CMainSignals::TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t mempool_sequence) +{ auto event = [tx, mempool_sequence, this] { m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionAddedToMempool(tx, mempool_sequence); }); }; ENQUEUE_AND_LOG_EVENT(event, "%s: txid=%s wtxid=%s", __func__, - tx->GetHash().ToString(), - tx->GetWitnessHash().ToString()); + tx.info.m_tx->GetHash().ToString(), + tx.info.m_tx->GetWitnessHash().ToString()); } void CMainSignals::TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence) { diff --git a/src/validationinterface.h b/src/validationinterface.h index ea49a45aa8..e1d6869fab 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -22,6 +22,7 @@ class CValidationInterface; class CScheduler; enum class MemPoolRemovalReason; struct RemovedMempoolTransactionInfo; +struct NewMempoolTransactionInfo; /** Register subscriber */ void RegisterValidationInterface(CValidationInterface* callbacks); @@ -97,7 +98,7 @@ protected: * * Called on a background thread. */ - virtual void TransactionAddedToMempool(const CTransactionRef& tx, uint64_t mempool_sequence) {} + virtual void TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t mempool_sequence) {} /** * Notifies listeners of a transaction leaving mempool. @@ -210,7 +211,7 @@ public: void UpdatedBlockTip(const CBlockIndex *, const CBlockIndex *, bool fInitialDownload); - void TransactionAddedToMempool(const CTransactionRef&, uint64_t mempool_sequence); + void TransactionAddedToMempool(const NewMempoolTransactionInfo&, uint64_t mempool_sequence); void TransactionRemovedFromMempool(const CTransactionRef&, MemPoolRemovalReason, uint64_t mempool_sequence); void MempoolTransactionsRemovedForBlock(const std::vector&, unsigned int nBlockHeight); void BlockConnected(ChainstateRole, const std::shared_ptr &, const CBlockIndex *pindex); diff --git a/src/zmq/zmqnotificationinterface.cpp b/src/zmq/zmqnotificationinterface.cpp index 03aae86577..63c2737706 100644 --- a/src/zmq/zmqnotificationinterface.cpp +++ b/src/zmq/zmqnotificationinterface.cpp @@ -6,6 +6,7 @@ #include #include +#include #include #include #include @@ -152,9 +153,9 @@ void CZMQNotificationInterface::UpdatedBlockTip(const CBlockIndex *pindexNew, co }); } -void CZMQNotificationInterface::TransactionAddedToMempool(const CTransactionRef& ptx, uint64_t mempool_sequence) +void CZMQNotificationInterface::TransactionAddedToMempool(const NewMempoolTransactionInfo& ptx, uint64_t mempool_sequence) { - const CTransaction& tx = *ptx; + const CTransaction& tx = *(ptx.info.m_tx); TryForEachAndRemoveFailed(notifiers, [&tx, mempool_sequence](CZMQAbstractNotifier* notifier) { return notifier->NotifyTransaction(tx) && notifier->NotifyTransactionAcceptance(tx, mempool_sequence); diff --git a/src/zmq/zmqnotificationinterface.h b/src/zmq/zmqnotificationinterface.h index 4246c53bd3..45d0982bd3 100644 --- a/src/zmq/zmqnotificationinterface.h +++ b/src/zmq/zmqnotificationinterface.h @@ -16,6 +16,7 @@ class CBlock; class CBlockIndex; class CZMQAbstractNotifier; +struct NewMempoolTransactionInfo; class CZMQNotificationInterface final : public CValidationInterface { @@ -31,7 +32,7 @@ protected: void Shutdown(); // CValidationInterface - void TransactionAddedToMempool(const CTransactionRef& tx, uint64_t mempool_sequence) override; + void TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t mempool_sequence) override; void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence) override; void BlockConnected(ChainstateRole role, const std::shared_ptr& pblock, const CBlockIndex* pindexConnected) override; void BlockDisconnected(const std::shared_ptr& pblock, const CBlockIndex* pindexDisconnected) override; From 714523918ba2b853fc69bee6b04a33ba0c828bf5 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Fri, 3 Nov 2023 18:34:58 +0100 Subject: [PATCH 6/7] tx fees, policy: CBlockPolicyEstimator update from `CValidationInterface` notifications `CBlockPolicyEstimator` will implement `CValidationInterface` and subscribe to its notification to process transactions added and removed from the mempool. Re-delegate calculation of `validForFeeEstimation` from validation to fee estimator. Also clean up the validForFeeEstimation arg thats no longer needed in `CTxMempool`. Co-authored-by: Matt Corallo --- src/Makefile.am | 1 - src/init.cpp | 10 ++- src/kernel/mempool_options.h | 4 -- src/policy/fees.cpp | 40 ++++++++---- src/policy/fees.h | 22 +++++-- src/test/fuzz/package_eval.cpp | 1 - src/test/fuzz/policy_estimator.cpp | 13 +++- src/test/fuzz/tx_pool.cpp | 1 - src/test/policyestimator_tests.cpp | 97 +++++++++++++++++++++++++++--- src/test/util/txmempool.cpp | 1 - src/txmempool.cpp | 24 ++------ src/txmempool.h | 7 +-- src/validation.cpp | 11 +--- 13 files changed, 158 insertions(+), 74 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 99b2184cf2..b746a931c1 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -954,7 +954,6 @@ libbitcoinkernel_la_SOURCES = \ node/chainstate.cpp \ node/utxo_snapshot.cpp \ policy/feerate.cpp \ - policy/fees.cpp \ policy/packages.cpp \ policy/policy.cpp \ policy/rbf.cpp \ diff --git a/src/init.cpp b/src/init.cpp index d6dc62f707..92aa187626 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -285,8 +285,12 @@ void Shutdown(NodeContext& node) DumpMempool(*node.mempool, MempoolPath(*node.args)); } - // Drop transactions we were still watching, and record fee estimations. - if (node.fee_estimator) node.fee_estimator->Flush(); + // Drop transactions we were still watching, record fee estimations and Unregister + // fee estimator from validation interface. + if (node.fee_estimator) { + node.fee_estimator->Flush(); + UnregisterValidationInterface(node.fee_estimator.get()); + } // FlushStateToDisk generates a ChainStateFlushed callback, which we should avoid missing if (node.chainman) { @@ -1258,6 +1262,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) // Flush estimates to disk periodically CBlockPolicyEstimator* fee_estimator = node.fee_estimator.get(); node.scheduler->scheduleEvery([fee_estimator] { fee_estimator->FlushFeeEstimates(); }, FEE_FLUSH_INTERVAL); + RegisterValidationInterface(fee_estimator); } // Check port numbers @@ -1471,7 +1476,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) assert(!node.chainman); CTxMemPool::Options mempool_opts{ - .estimator = node.fee_estimator.get(), .check_ratio = chainparams.DefaultConsistencyChecks() ? 1 : 0, }; auto result{ApplyArgsManOptions(args, chainparams, mempool_opts)}; diff --git a/src/kernel/mempool_options.h b/src/kernel/mempool_options.h index d09fd2ba35..753aebd455 100644 --- a/src/kernel/mempool_options.h +++ b/src/kernel/mempool_options.h @@ -13,8 +13,6 @@ #include #include -class CBlockPolicyEstimator; - /** Default for -maxmempool, maximum megabytes of mempool memory usage */ static constexpr unsigned int DEFAULT_MAX_MEMPOOL_SIZE_MB{300}; /** Default for -maxmempool when blocksonly is set */ @@ -37,8 +35,6 @@ namespace kernel { * Most of the time, this struct should be referenced as CTxMemPool::Options. */ struct MemPoolOptions { - /* Used to estimate appropriate transaction fees. */ - CBlockPolicyEstimator* estimator{nullptr}; /* The ratio used to determine how often sanity checks will run. */ int check_ratio{0}; int64_t max_size_bytes{DEFAULT_MAX_MEMPOOL_SIZE_MB * 1'000'000}; diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index 6d550b1d5a..74c688060d 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -515,15 +515,10 @@ 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, bool inBlock) +bool CBlockPolicyEstimator::removeTx(uint256 hash) { LOCK(m_cs_fee_estimator); - return _removeTx(hash, inBlock); + return _removeTx(hash, /*inBlock=*/false); } bool CBlockPolicyEstimator::_removeTx(const uint256& hash, bool inBlock) @@ -579,11 +574,26 @@ CBlockPolicyEstimator::CBlockPolicyEstimator(const fs::path& estimation_filepath CBlockPolicyEstimator::~CBlockPolicyEstimator() = default; -void CBlockPolicyEstimator::processTransaction(const CTxMemPoolEntry& entry, bool validFeeEstimate) +void CBlockPolicyEstimator::TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t /*unused*/) +{ + processTransaction(tx); +} + +void CBlockPolicyEstimator::TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason /*unused*/, uint64_t /*unused*/) +{ + removeTx(tx->GetHash()); +} + +void CBlockPolicyEstimator::MempoolTransactionsRemovedForBlock(const std::vector& txs_removed_for_block, unsigned int nBlockHeight) +{ + processBlock(txs_removed_for_block, nBlockHeight); +} + +void CBlockPolicyEstimator::processTransaction(const NewMempoolTransactionInfo& tx) { LOCK(m_cs_fee_estimator); - unsigned int txHeight = entry.GetHeight(); - uint256 hash = entry.GetTx().GetHash(); + const unsigned int txHeight = tx.info.txHeight; + const auto& hash = tx.info.m_tx->GetHash(); if (mapMemPoolTxs.count(hash)) { LogPrint(BCLog::ESTIMATEFEE, "Blockpolicy error mempool tx %s already being tracked\n", hash.ToString()); @@ -597,17 +607,23 @@ void CBlockPolicyEstimator::processTransaction(const CTxMemPoolEntry& entry, boo // It will be synced next time a block is processed. return; } + // This transaction should only count for fee estimation if: + // - it's not being re-added during a reorg which bypasses typical mempool fee limits + // - the node is not behind + // - the transaction is not dependent on any other transactions in the mempool + // - it's not part of a package. + const bool validForFeeEstimation = !tx.m_from_disconnected_block && !tx.m_submitted_in_package && tx.m_chainstate_is_current && tx.m_has_no_mempool_parents; // 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 (!validForFeeEstimation) { untrackedTxs++; return; } trackedTxs++; // Feerates are stored and reported as BTC-per-kb: - CFeeRate feeRate(entry.GetFee(), entry.GetTxSize()); + const CFeeRate feeRate(tx.info.m_fee, tx.info.m_virtual_transaction_size); mapMemPoolTxs[hash].blockHeight = txHeight; unsigned int bucketIndex = feeStats->NewTx(txHeight, static_cast(feeRate.GetFeePerK())); diff --git a/src/policy/fees.h b/src/policy/fees.h index cff0c4dbe2..f34f66d3f0 100644 --- a/src/policy/fees.h +++ b/src/policy/fees.h @@ -12,6 +12,7 @@ #include #include #include +#include #include #include @@ -35,9 +36,9 @@ static constexpr std::chrono::hours MAX_FILE_AGE{60}; static constexpr bool DEFAULT_ACCEPT_STALE_FEE_ESTIMATES{false}; class AutoFile; -class CTxMemPoolEntry; class TxConfirmStats; struct RemovedMempoolTransactionInfo; +struct NewMempoolTransactionInfo; /* Identifier for each of the 3 different TxConfirmStats which will track * history over different time horizons. */ @@ -144,7 +145,7 @@ struct FeeCalculation * a certain number of blocks. Every time a block is added to the best chain, this class records * stats on the transactions included in that block */ -class CBlockPolicyEstimator +class CBlockPolicyEstimator : public CValidationInterface { private: /** Track confirm delays up to 12 blocks for short horizon */ @@ -199,7 +200,7 @@ private: public: /** Create new BlockPolicyEstimator and initialize stats tracking classes with default values */ CBlockPolicyEstimator(const fs::path& estimation_filepath, const bool read_stale_estimates); - ~CBlockPolicyEstimator(); + virtual ~CBlockPolicyEstimator(); /** Process all the transactions that have been included in a block */ void processBlock(const std::vector& txs_removed_for_block, @@ -207,11 +208,11 @@ public: EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); /** Process a transaction accepted to the mempool*/ - void processTransaction(const CTxMemPoolEntry& entry, bool validFeeEstimate) + void processTransaction(const NewMempoolTransactionInfo& tx) EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); - /** Remove a transaction from the mempool tracking stats*/ - bool removeTx(uint256 hash, bool inBlock) + /** Remove a transaction from the mempool tracking stats for non BLOCK removal reasons*/ + bool removeTx(uint256 hash) EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); /** DEPRECATED. Return a feerate estimate */ @@ -261,6 +262,15 @@ public: /** Calculates the age of the file, since last modified */ std::chrono::hours GetFeeEstimatorFileAge(); +protected: + /** Overridden from CValidationInterface. */ + void TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t /*unused*/) override + EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); + void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason /*unused*/, uint64_t /*unused*/) override + EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); + void MempoolTransactionsRemovedForBlock(const std::vector& txs_removed_for_block, unsigned int nBlockHeight) override + EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); + private: mutable Mutex m_cs_fee_estimator; diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp index 021acc02f2..08b8be26e7 100644 --- a/src/test/fuzz/package_eval.cpp +++ b/src/test/fuzz/package_eval.cpp @@ -121,7 +121,6 @@ CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeConte mempool_opts.expiry = std::chrono::hours{fuzzed_data_provider.ConsumeIntegralInRange(0, 999)}; nBytesPerSigOp = fuzzed_data_provider.ConsumeIntegralInRange(1, 999); - mempool_opts.estimator = nullptr; mempool_opts.check_ratio = 1; mempool_opts.require_standard = fuzzed_data_provider.ConsumeBool(); diff --git a/src/test/fuzz/policy_estimator.cpp b/src/test/fuzz/policy_estimator.cpp index e5e0b0d8da..4a41707edf 100644 --- a/src/test/fuzz/policy_estimator.cpp +++ b/src/test/fuzz/policy_estimator.cpp @@ -44,9 +44,16 @@ FUZZ_TARGET(policy_estimator, .init = initialize_policy_estimator) return; } const CTransaction tx{*mtx}; - block_policy_estimator.processTransaction(ConsumeTxMemPoolEntry(fuzzed_data_provider, tx), fuzzed_data_provider.ConsumeBool()); + const CTxMemPoolEntry& entry = ConsumeTxMemPoolEntry(fuzzed_data_provider, tx); + const auto tx_info = NewMempoolTransactionInfo(entry.GetSharedTx(), entry.GetFee(), + entry.GetTxSize(), entry.GetHeight(), + /* m_from_disconnected_block */ false, + /* m_submitted_in_package */ false, + /* m_chainstate_is_current */ true, + /* m_has_no_mempool_parents */ fuzzed_data_provider.ConsumeBool()); + block_policy_estimator.processTransaction(tx_info); if (fuzzed_data_provider.ConsumeBool()) { - (void)block_policy_estimator.removeTx(tx.GetHash(), /*inBlock=*/fuzzed_data_provider.ConsumeBool()); + (void)block_policy_estimator.removeTx(tx.GetHash()); } }, [&] { @@ -69,7 +76,7 @@ FUZZ_TARGET(policy_estimator, .init = initialize_policy_estimator) block_policy_estimator.processBlock(txs, fuzzed_data_provider.ConsumeIntegral()); }, [&] { - (void)block_policy_estimator.removeTx(ConsumeUInt256(fuzzed_data_provider), /*inBlock=*/fuzzed_data_provider.ConsumeBool()); + (void)block_policy_estimator.removeTx(ConsumeUInt256(fuzzed_data_provider)); }, [&] { block_policy_estimator.FlushUnconfirmed(); diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index 001b452722..cc816f213b 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -123,7 +123,6 @@ CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeConte CTxMemPool::Options mempool_opts{MemPoolOptionsForTest(node)}; // ...override specific options for this specific fuzz suite - mempool_opts.estimator = nullptr; mempool_opts.check_ratio = 1; mempool_opts.require_standard = fuzzed_data_provider.ConsumeBool(); diff --git a/src/test/policyestimator_tests.cpp b/src/test/policyestimator_tests.cpp index bc9c672560..13ec89663a 100644 --- a/src/test/policyestimator_tests.cpp +++ b/src/test/policyestimator_tests.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include @@ -19,7 +20,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) { CBlockPolicyEstimator& feeEst = *Assert(m_node.fee_estimator); CTxMemPool& mpool = *Assert(m_node.mempool); - LOCK2(cs_main, mpool.cs); + RegisterValidationInterface(&feeEst); TestMemPoolEntryHelper entry; CAmount basefee(2000); CAmount deltaFee(100); @@ -59,8 +60,23 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) for (int j = 0; j < 10; j++) { // For each fee for (int k = 0; k < 4; k++) { // add 4 fee txs tx.vin[0].prevout.n = 10000*blocknum+100*j+k; // make transaction unique + { + LOCK2(cs_main, mpool.cs); + mpool.addUnchecked(entry.Fee(feeV[j]).Time(Now()).Height(blocknum).FromTx(tx)); + // Since TransactionAddedToMempool callbacks are generated in ATMP, + // not addUnchecked, we cheat and create one manually here + const int64_t virtual_size = GetVirtualTransactionSize(*MakeTransactionRef(tx)); + const NewMempoolTransactionInfo tx_info{NewMempoolTransactionInfo(MakeTransactionRef(tx), + feeV[j], + virtual_size, + entry.nHeight, + /* m_from_disconnected_block */ false, + /* m_submitted_in_package */ false, + /* m_chainstate_is_current */ true, + /* m_has_no_mempool_parents */ true)}; + GetMainSignals().TransactionAddedToMempool(tx_info, mpool.GetAndIncrementSequence()); + } uint256 hash = tx.GetHash(); - mpool.addUnchecked(entry.Fee(feeV[j]).Time(Now()).Height(blocknum).FromTx(tx)); txHashes[j].push_back(hash); } } @@ -76,10 +92,17 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) txHashes[9-h].pop_back(); } } - mpool.removeForBlock(block, ++blocknum); + + { + LOCK(mpool.cs); + mpool.removeForBlock(block, ++blocknum); + } + block.clear(); // Check after just a few txs that combining buckets works as expected if (blocknum == 3) { + // Wait for fee estimator to catch up + SyncWithValidationInterfaceQueue(); // At this point we should need to combine 3 buckets to get enough data points // So estimateFee(1) should fail and estimateFee(2) should return somewhere around // 9*baserate. estimateFee(2) %'s are 100,100,90 = average 97% @@ -114,8 +137,13 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) // Mine 50 more blocks with no transactions happening, estimates shouldn't change // We haven't decayed the moving average enough so we still have enough data points in every bucket - while (blocknum < 250) + while (blocknum < 250) { + LOCK(mpool.cs); mpool.removeForBlock(block, ++blocknum); + } + + // Wait for fee estimator to catch up + SyncWithValidationInterfaceQueue(); BOOST_CHECK(feeEst.estimateFee(1) == CFeeRate(0)); for (int i = 2; i < 10;i++) { @@ -130,14 +158,35 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) for (int j = 0; j < 10; j++) { // For each fee multiple for (int k = 0; k < 4; k++) { // add 4 fee txs tx.vin[0].prevout.n = 10000*blocknum+100*j+k; + { + LOCK2(cs_main, mpool.cs); + mpool.addUnchecked(entry.Fee(feeV[j]).Time(Now()).Height(blocknum).FromTx(tx)); + // Since TransactionAddedToMempool callbacks are generated in ATMP, + // not addUnchecked, we cheat and create one manually here + const int64_t virtual_size = GetVirtualTransactionSize(*MakeTransactionRef(tx)); + const NewMempoolTransactionInfo tx_info{NewMempoolTransactionInfo(MakeTransactionRef(tx), + feeV[j], + virtual_size, + entry.nHeight, + /* m_from_disconnected_block */ false, + /* m_submitted_in_package */ false, + /* m_chainstate_is_current */ true, + /* m_has_no_mempool_parents */ true)}; + GetMainSignals().TransactionAddedToMempool(tx_info, mpool.GetAndIncrementSequence()); + } uint256 hash = tx.GetHash(); - mpool.addUnchecked(entry.Fee(feeV[j]).Time(Now()).Height(blocknum).FromTx(tx)); txHashes[j].push_back(hash); } } - mpool.removeForBlock(block, ++blocknum); + { + LOCK(mpool.cs); + mpool.removeForBlock(block, ++blocknum); + } } + // Wait for fee estimator to catch up + SyncWithValidationInterfaceQueue(); + for (int i = 1; i < 10;i++) { BOOST_CHECK(feeEst.estimateFee(i) == CFeeRate(0) || feeEst.estimateFee(i).GetFeePerK() > origFeeEst[i-1] - deltaFee); } @@ -152,8 +201,16 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) txHashes[j].pop_back(); } } - mpool.removeForBlock(block, 266); + + { + LOCK(mpool.cs); + mpool.removeForBlock(block, 266); + } block.clear(); + + // Wait for fee estimator to catch up + SyncWithValidationInterfaceQueue(); + BOOST_CHECK(feeEst.estimateFee(1) == CFeeRate(0)); for (int i = 2; i < 10;i++) { BOOST_CHECK(feeEst.estimateFee(i) == CFeeRate(0) || feeEst.estimateFee(i).GetFeePerK() > origFeeEst[i-1] - deltaFee); @@ -165,17 +222,39 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) for (int j = 0; j < 10; j++) { // For each fee multiple for (int k = 0; k < 4; k++) { // add 4 fee txs tx.vin[0].prevout.n = 10000*blocknum+100*j+k; + { + LOCK2(cs_main, mpool.cs); + mpool.addUnchecked(entry.Fee(feeV[j]).Time(Now()).Height(blocknum).FromTx(tx)); + // Since TransactionAddedToMempool callbacks are generated in ATMP, + // not addUnchecked, we cheat and create one manually here + const int64_t virtual_size = GetVirtualTransactionSize(*MakeTransactionRef(tx)); + const NewMempoolTransactionInfo tx_info{NewMempoolTransactionInfo(MakeTransactionRef(tx), + feeV[j], + virtual_size, + entry.nHeight, + /* m_from_disconnected_block */ false, + /* m_submitted_in_package */ false, + /* m_chainstate_is_current */ true, + /* m_has_no_mempool_parents */ true)}; + GetMainSignals().TransactionAddedToMempool(tx_info, mpool.GetAndIncrementSequence()); + } uint256 hash = tx.GetHash(); - mpool.addUnchecked(entry.Fee(feeV[j]).Time(Now()).Height(blocknum).FromTx(tx)); CTransactionRef ptx = mpool.get(hash); if (ptx) block.push_back(ptx); } } - mpool.removeForBlock(block, ++blocknum); + + { + LOCK(mpool.cs); + mpool.removeForBlock(block, ++blocknum); + } + block.clear(); } + // Wait for fee estimator to catch up + SyncWithValidationInterfaceQueue(); BOOST_CHECK(feeEst.estimateFee(1) == CFeeRate(0)); for (int i = 2; i < 9; i++) { // At 9, the original estimate was already at the bottom (b/c scale = 2) BOOST_CHECK(feeEst.estimateFee(i).GetFeePerK() < origFeeEst[i-1] - deltaFee); diff --git a/src/test/util/txmempool.cpp b/src/test/util/txmempool.cpp index 6d3bb01be8..379c3c9329 100644 --- a/src/test/util/txmempool.cpp +++ b/src/test/util/txmempool.cpp @@ -18,7 +18,6 @@ using node::NodeContext; CTxMemPool::Options MemPoolOptionsForTest(const NodeContext& node) { CTxMemPool::Options mempool_opts{ - .estimator = node.fee_estimator.get(), // Default to always checking mempool regardless of // chainparams.DefaultConsistencyChecks for tests .check_ratio = 1, diff --git a/src/txmempool.cpp b/src/txmempool.cpp index f5036a9301..8a2af807df 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -12,9 +12,9 @@ #include #include #include -#include #include #include +#include #include #include #include @@ -402,7 +402,6 @@ void CTxMemPoolEntry::UpdateAncestorState(int32_t modifySize, CAmount modifyFee, CTxMemPool::CTxMemPool(const Options& opts) : m_check_ratio{opts.check_ratio}, - minerPolicyEstimator{opts.estimator}, m_max_size_bytes{opts.max_size_bytes}, m_expiry{opts.expiry}, m_incremental_relay_feerate{opts.incremental_relay_feerate}, @@ -433,7 +432,7 @@ void CTxMemPool::AddTransactionsUpdated(unsigned int n) nTransactionsUpdated += n; } -void CTxMemPool::addUnchecked(const CTxMemPoolEntry &entry, setEntries &setAncestors, bool validFeeEstimate) +void CTxMemPool::addUnchecked(const CTxMemPoolEntry &entry, setEntries &setAncestors) { // Add to memory pool without checking anything. // Used by AcceptToMemoryPool(), which DOES do @@ -477,9 +476,6 @@ void CTxMemPool::addUnchecked(const CTxMemPoolEntry &entry, setEntries &setAnces nTransactionsUpdated++; totalTxSize += entry.GetTxSize(); m_total_fee += entry.GetFee(); - if (minerPolicyEstimator) { - minerPolicyEstimator->processTransaction(entry, validFeeEstimate); - } txns_randomized.emplace_back(newit->GetSharedTx()); newit->idx_randomized = txns_randomized.size() - 1; @@ -497,16 +493,12 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason) // even if not directly reported below. uint64_t mempool_sequence = GetAndIncrementSequence(); - const auto& hash = it->GetTx().GetHash(); if (reason != MemPoolRemovalReason::BLOCK) { // Notify clients that a transaction has been removed from the mempool // for any reason except being included in a block. Clients interested // in transactions included in blocks can subscribe to the BlockConnected // notification. GetMainSignals().TransactionRemovedFromMempool(it->GetSharedTx(), reason, mempool_sequence); - if (minerPolicyEstimator) { - minerPolicyEstimator->removeTx(hash, false); - } } TRACE5(mempool, removed, it->GetTx().GetHash().data(), @@ -519,7 +511,7 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason) for (const CTxIn& txin : it->GetTx().vin) mapNextTx.erase(txin.prevout); - RemoveUnbroadcastTx(hash, true /* add logging because unchecked */ ); + RemoveUnbroadcastTx(it->GetTx().GetHash(), true /* add logging because unchecked */); if (txns_randomized.size() > 1) { // Update idx_randomized of the to-be-moved entry. @@ -638,7 +630,7 @@ void CTxMemPool::removeConflicts(const CTransaction &tx) } /** - * Called when a block is connected. Removes from mempool and updates the miner fee estimator. + * Called when a block is connected. Removes from mempool. */ void CTxMemPool::removeForBlock(const std::vector& vtx, unsigned int nBlockHeight) { @@ -657,10 +649,6 @@ void CTxMemPool::removeForBlock(const std::vector& vtx, unsigne removeConflicts(*tx); ClearPrioritisation(tx->GetHash()); } - // Update policy estimates - if (minerPolicyEstimator) { - minerPolicyEstimator->processBlock(txs_removed_for_block, nBlockHeight); - } GetMainSignals().MempoolTransactionsRemovedForBlock(txs_removed_for_block, nBlockHeight); lastRollingFeeUpdate = GetTime(); blockSinceLastRollingFeeBump = true; @@ -1091,10 +1079,10 @@ int CTxMemPool::Expire(std::chrono::seconds time) return stage.size(); } -void CTxMemPool::addUnchecked(const CTxMemPoolEntry &entry, bool validFeeEstimate) +void CTxMemPool::addUnchecked(const CTxMemPoolEntry &entry) { auto ancestors{AssumeCalculateMemPoolAncestors(__func__, entry, Limits::NoLimits())}; - return addUnchecked(entry, ancestors, validFeeEstimate); + return addUnchecked(entry, ancestors); } void CTxMemPool::UpdateChild(txiter entry, txiter child, bool add) diff --git a/src/txmempool.h b/src/txmempool.h index aed6acd9da..bac7a445d6 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -202,8 +202,6 @@ struct entry_time {}; struct ancestor_score {}; struct index_by_wtxid {}; -class CBlockPolicyEstimator; - /** * Information about a mempool transaction. */ @@ -303,7 +301,6 @@ class CTxMemPool protected: const int m_check_ratio; //!< Value n means that 1 times in n we check. std::atomic nTransactionsUpdated{0}; //!< Used by getblocktemplate to trigger CreateNewBlock() invocation - CBlockPolicyEstimator* const minerPolicyEstimator; uint64_t totalTxSize GUARDED_BY(cs){0}; //!< sum of all mempool tx's virtual sizes. Differs from serialized tx size since witness data is discounted. Defined in BIP 141. CAmount m_total_fee GUARDED_BY(cs){0}; //!< sum of all mempool tx's fees (NOT modified fee) @@ -472,8 +469,8 @@ public: // Note that addUnchecked is ONLY called from ATMP outside of tests // and any other callers may break wallet's in-mempool tracking (due to // lack of CValidationInterface::TransactionAddedToMempool callbacks). - void addUnchecked(const CTxMemPoolEntry& entry, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); - void addUnchecked(const CTxMemPoolEntry& entry, setEntries& setAncestors, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); + void addUnchecked(const CTxMemPoolEntry& entry) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); + void addUnchecked(const CTxMemPoolEntry& entry, setEntries& setAncestors) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); void removeRecursive(const CTransaction& tx, MemPoolRemovalReason reason) EXCLUSIVE_LOCKS_REQUIRED(cs); /** After reorg, filter the entries that would no longer be valid in the next block, and update diff --git a/src/validation.cpp b/src/validation.cpp index 1c95ba08c5..f9e5d1db82 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1126,17 +1126,8 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws) ws.m_replaced_transactions.push_back(it->GetSharedTx()); } m_pool.RemoveStaged(ws.m_all_conflicting, false, MemPoolRemovalReason::REPLACED); - - // This transaction should only count for fee estimation if: - // - it's not being re-added during a reorg which bypasses typical mempool fee limits - // - the node is not behind - // - the transaction is not dependent on any other transactions in the mempool - // - it's not part of a package. Since package relay is not currently supported, this - // transaction has not necessarily been accepted to miners' mempools. - bool validForFeeEstimation = !bypass_limits && !args.m_package_submission && IsCurrentForFeeEstimation(m_active_chainstate) && m_pool.HasNoInputsOf(tx); - // Store transaction in memory - m_pool.addUnchecked(*entry, ws.m_ancestors, validForFeeEstimation); + m_pool.addUnchecked(*entry, ws.m_ancestors); // trim mempool and check if tx was trimmed // If we are validating a package, don't trim here because we could evict a previous transaction From 91504cbe0de2b74ef1aa2709761aaf0597ec66a2 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Wed, 8 Nov 2023 11:21:49 +0100 Subject: [PATCH 7/7] rpc: `SyncWithValidationInterfaceQueue` on fee estimation RPC's This ensures that the most recent fee estimation data is used for the fee estimation with `estimateSmartfee` and `estimaterawfee` RPC's. --- src/rpc/fees.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/rpc/fees.cpp b/src/rpc/fees.cpp index 62396d4c58..57ba486ed9 100644 --- a/src/rpc/fees.cpp +++ b/src/rpc/fees.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include #include @@ -67,6 +68,7 @@ static RPCHelpMan estimatesmartfee() const NodeContext& node = EnsureAnyNodeContext(request.context); const CTxMemPool& mempool = EnsureMemPool(node); + SyncWithValidationInterfaceQueue(); unsigned int max_target = fee_estimator.HighestTargetTracked(FeeEstimateHorizon::LONG_HALFLIFE); unsigned int conf_target = ParseConfirmTarget(request.params[0], max_target); bool conservative = true; @@ -155,6 +157,7 @@ static RPCHelpMan estimaterawfee() { CBlockPolicyEstimator& fee_estimator = EnsureAnyFeeEstimator(request.context); + SyncWithValidationInterfaceQueue(); unsigned int max_target = fee_estimator.HighestTargetTracked(FeeEstimateHorizon::LONG_HALFLIFE); unsigned int conf_target = ParseConfirmTarget(request.params[0], max_target); double threshold = 0.95;