From 14dcc05357d487432af049b0b7598e8475f64b5a Mon Sep 17 00:00:00 2001 From: Calin Culianu Date: Thu, 15 Apr 2021 08:48:30 -0600 Subject: [PATCH] mempool: Remove BatchUpdater classes, consolidate & clean-up removeForBlock Summary --- As discussed in review and raised in issue #285, the `BatchUpdater` class hierarchy is no longer needed. It was added originally to abstract-out the logic for confirmed txs being removed from the mempool, in order to try out different algorithms. However, we never ended up making use of this abstract interface. During review of the recent mempool changes, it was decided that this interface probably needs to be reverted back to a simple direct method inside `CTxMemPool` (for clarity and maintainability). As such, this MR does just that. Additionally, the unused second parameter `nBlockHeight` to `CTxMemPool::removeForBlock()` was removed, in the interests of code quality. Should we need this parameter again, we can always bring it back. All call-sites were updated to not pass this argument. This closes #285. Test Plan --- - `ninja all check-all` --- src/CMakeLists.txt | 1 - src/Makefile.am | 3 -- src/mempool/batchupdater.h | 39 ------------------------- src/mempool/defaultbatchupdater.cpp | 35 ---------------------- src/mempool/defaultbatchupdater.h | 31 -------------------- src/test/mempool_tests.cpp | 2 +- src/test/policyestimator_tests.cpp | 3 +- src/txmempool.cpp | 23 +++++++++++---- src/txmempool.h | 7 +---- src/validation.cpp | 2 +- test/lint/lint-circular-dependencies.sh | 1 - 11 files changed, 22 insertions(+), 125 deletions(-) delete mode 100644 src/mempool/batchupdater.h delete mode 100644 src/mempool/defaultbatchupdater.cpp delete mode 100644 src/mempool/defaultbatchupdater.h diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 089c2673e0..30af8a2b4e 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -504,7 +504,6 @@ add_library(server net_processing.cpp node/transaction.cpp noui.cpp - mempool/defaultbatchupdater.cpp outputtype.cpp policy/fees.cpp policy/policy.cpp diff --git a/src/Makefile.am b/src/Makefile.am index c0bd345ff7..6b070bdfc7 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -166,8 +166,6 @@ BITCOIN_CORE_H = \ dbwrapper.h \ limitedmap.h \ logging.h \ - mempool/batchupdater.h \ - mempool/defaultbatchupdater.h \ memusage.h \ merkleblock.h \ miner.h \ @@ -293,7 +291,6 @@ libbitcoin_server_a_SOURCES = \ interfaces/handler.cpp \ interfaces/node.cpp \ dbwrapper.cpp \ - mempool/defaultbatchupdater.cpp \ merkleblock.cpp \ miner.cpp \ net.cpp \ diff --git a/src/mempool/batchupdater.h b/src/mempool/batchupdater.h deleted file mode 100644 index 3643a741fa..0000000000 --- a/src/mempool/batchupdater.h +++ /dev/null @@ -1,39 +0,0 @@ -// Copyright (c) 2020 The Bitcoin developers -// Distributed under the MIT software license, see the accompanying -// file COPYING or http://www.opensource.org/licenses/mit-license.php. - -#ifndef BITCOIN_MEMPOOL_BATCHUPDATER_H -#define BITCOIN_MEMPOOL_BATCHUPDATER_H - -#include - -#include -#include - -namespace mempool { - -/** - * Interface for doing large operations on the mempool, such as updating the - * mempool after a new block has connected. - * - * This interface exists as to allow for exploring alternative algorithms, - * while being able to default to the stable (and slower) one. - */ -class BatchUpdater { -public: - virtual ~BatchUpdater() = 0; - - /** - * Called when a block is connected. Removes transactions from mempool. - * - * Requires lock held on mempool.cs - */ - virtual void removeForBlock(const std::vector &vtx, - uint64_t nBlockHeight) = 0; -}; - -// Linker error without this default implementation -inline BatchUpdater::~BatchUpdater() { } -} - -#endif diff --git a/src/mempool/defaultbatchupdater.cpp b/src/mempool/defaultbatchupdater.cpp deleted file mode 100644 index 32eaeb0c4d..0000000000 --- a/src/mempool/defaultbatchupdater.cpp +++ /dev/null @@ -1,35 +0,0 @@ -// Copyright (c) 2020 The Bitcoin developers -// Distributed under the MIT software license, see the accompanying -// file COPYING or http://www.opensource.org/licenses/mit-license.php. - -#include -#include -#include - -namespace mempool { - -void DefaultBatchUpdater::removeForBlock(const std::vector &vtx, - uint64_t nBlockHeight [[maybe_unused]]) -{ - AssertLockHeld(mempool.cs); - DisconnectedBlockTransactions disconnectpool; - disconnectpool.addForBlock(vtx); - - for (const CTransactionRef &tx : - reverse_iterate(disconnectpool.GetQueuedTx().get())) { - CTxMemPool::txiter it = mempool.mapTx.find(tx->GetId()); - if (it != mempool.mapTx.end()) { - CTxMemPool::setEntries stage; - stage.insert(it); - mempool.RemoveStaged(stage, MemPoolRemovalReason::BLOCK); - } - mempool.removeConflicts(*tx); - mempool.ClearPrioritisation(tx->GetId()); - } - - disconnectpool.clear(); -} - -DefaultBatchUpdater::~DefaultBatchUpdater() { } - -} // ns mempool diff --git a/src/mempool/defaultbatchupdater.h b/src/mempool/defaultbatchupdater.h deleted file mode 100644 index 1dfe617c18..0000000000 --- a/src/mempool/defaultbatchupdater.h +++ /dev/null @@ -1,31 +0,0 @@ -// Copyright (c) 2020 The Bitcoin developers -// Distributed under the MIT software license, see the accompanying -// file COPYING or http://www.opensource.org/licenses/mit-license.php. - -#ifndef BITCOIN_MEMPOOL_DEFAULTBATCHUPDATER_H -#define BITCOIN_MEMPOOL_DEFAULTBATCHUPDATER_H - -#include - -#include - -class CTxMemPool; - -namespace mempool { - -class DefaultBatchUpdater : public mempool::BatchUpdater { -public: - DefaultBatchUpdater(CTxMemPool& m) : mempool(m) { } - - void removeForBlock(const std::vector &vtx, - uint64_t nBlockHeight) override; - - ~DefaultBatchUpdater() override; - -private: - CTxMemPool& mempool; -}; - -} // ns mempool - -#endif diff --git a/src/test/mempool_tests.cpp b/src/test/mempool_tests.cpp index 2f24a14311..1b2bbfef0a 100644 --- a/src/test/mempool_tests.cpp +++ b/src/test/mempool_tests.cpp @@ -468,7 +468,7 @@ BOOST_AUTO_TEST_CASE(MempoolSizeLimitTest) { BOOST_CHECK_EQUAL(pool.GetMinFee(1).GetFeePerK(), maxFeeRateRemoved.GetFeePerK() + feeIncrement); // ... we should keep the same min fee until we get a block - pool.removeForBlock(vtx, 1); + pool.removeForBlock(vtx); SetMockTime(42 + 2 * CTxMemPool::ROLLING_FEE_HALFLIFE); BOOST_CHECK_EQUAL(pool.GetMinFee(1).GetFeePerK(), (maxFeeRateRemoved.GetFeePerK() + feeIncrement) / 2); diff --git a/src/test/policyestimator_tests.cpp b/src/test/policyestimator_tests.cpp index 093adab86a..7f9d95ac07 100644 --- a/src/test/policyestimator_tests.cpp +++ b/src/test/policyestimator_tests.cpp @@ -51,7 +51,8 @@ BOOST_AUTO_TEST_CASE(MempoolMinimumFeeEstimate) { CTransactionRef ptx = mpool.get(txid); block.push_back(ptx); } - mpool.removeForBlock(block, ++blocknum); + ++blocknum; + mpool.removeForBlock(block); block.clear(); } diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 1ac6748988..c67a6f16d1 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -15,7 +15,6 @@ #include #include #include -#include #include #include #include @@ -239,8 +238,6 @@ CTxMemPool::CTxMemPool() // transactions becomes O(N^2) where N is the number of transactions in the // pool nCheckFrequency = 0; - - batchUpdater = std::make_unique(*this); } CTxMemPool::~CTxMemPool() {} @@ -421,14 +418,28 @@ 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) { +void CTxMemPool::removeForBlock(const std::vector &vtx) { + DisconnectedBlockTransactions disconnectpool; + disconnectpool.addForBlock(vtx); + LOCK(cs); - batchUpdater->removeForBlock(vtx, nBlockHeight); + // iterate in topological order (parents before children) + for (const CTransactionRef &tx : reverse_iterate(disconnectpool.GetQueuedTx().get())) { + const txiter it = mapTx.find(tx->GetId()); + if (it != mapTx.end()) { + setEntries stage; + stage.insert(it); + RemoveStaged(stage, MemPoolRemovalReason::BLOCK); + } + removeConflicts(*tx); + ClearPrioritisation(tx->GetId()); + } lastRollingFeeUpdate = GetTime(); blockSinceLastRollingFeeBump = true; + + disconnectpool.clear(); } void CTxMemPool::_clear(bool clearDspOrphans /*= true*/) { diff --git a/src/txmempool.h b/src/txmempool.h index 557acd0170..b1f39392c7 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -38,8 +38,6 @@ class Config; class DoubleSpendProof; class DoubleSpendProofStorage; -namespace mempool { class BatchUpdater; } - extern RecursiveMutex cs_main; @@ -372,8 +370,6 @@ class CTxMemPool { bool m_is_loaded GUARDED_BY(cs){false}; - std::unique_ptr batchUpdater; - //! Used by addUnchecked to generate ever-increasing CTxMemPoolEntry::entryId's uint64_t nextEntryId GUARDED_BY(cs) = 1; @@ -584,8 +580,7 @@ class CTxMemPool { const CTransaction &tx, MemPoolRemovalReason reason = MemPoolRemovalReason::UNKNOWN); void removeConflicts(const CTransaction &tx) EXCLUSIVE_LOCKS_REQUIRED(cs); - void removeForBlock(const std::vector &vtx, - unsigned int nBlockHeight); + void removeForBlock(const std::vector &vtx); void clear(bool clearDspOrphans = true); // lock free diff --git a/src/validation.cpp b/src/validation.cpp index 8977cc15c9..afd4a18287 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2633,7 +2633,7 @@ bool CChainState::ConnectTip(const Config &config, CValidationState &state, nTimeChainState * MILLI / nBlocksTotal); // Remove conflicting transactions from the mempool.; - g_mempool.removeForBlock(blockConnecting.vtx, pindexNew->nHeight); + g_mempool.removeForBlock(blockConnecting.vtx); disconnectpool.removeForBlock(blockConnecting.vtx); // If this block is activating a fork, we move all mempool transactions diff --git a/test/lint/lint-circular-dependencies.sh b/test/lint/lint-circular-dependencies.sh index c4174281d3..fa4529ca0f 100755 --- a/test/lint/lint-circular-dependencies.sh +++ b/test/lint/lint-circular-dependencies.sh @@ -39,7 +39,6 @@ EXPECTED_CIRCULAR_DEPENDENCIES=( "chainparams -> protocol -> config -> chainparams" "config -> policy/policy -> validation -> config" "config -> policy/policy -> validation -> protocol -> config" - "mempool/defaultbatchupdater -> txmempool -> mempool/defaultbatchupdater" ) EXIT_CODE=0