mempool: Remove BatchUpdater classes, consolidate & clean-up removeFo…

…rBlock

Summary
---

As discussed in review and raised in issue Bitcoin-ABC#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 Bitcoin-ABC#285.

Test Plan
---

- `ninja all check-all`

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

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 \

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);

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();
     }
 

src/txmempool.cpp

@@ -15,7 +15,6 @@
 #include <consensus/validation.h>
 #include <dsproof/dsproof.h>
 #include <dsproof/storage.h>
-#include <mempool/defaultbatchupdater.h>
 #include <policy/fees.h>
 #include <policy/mempool.h>
 #include <policy/policy.h>
@@ -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<mempool::DefaultBatchUpdater>(*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<CTransactionRef> &vtx,
-                                unsigned int nBlockHeight) {
+void CTxMemPool::removeForBlock(const std::vector<CTransactionRef> &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<insertion_order>())) {
+        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*/) {

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<mempool::BatchUpdater> 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<CTransactionRef> &vtx,
-                        unsigned int nBlockHeight);
+    void removeForBlock(const std::vector<CTransactionRef> &vtx);
 
     void clear(bool clearDspOrphans = true);
     // lock free

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

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