make DisconnectedBlockTransactions responsible for its own memory man…

…agement

Co-authored-by: Cory Fields <cory-nospam-@coryfields.com>

src/bench/disconnected_transactions.cpp

@@ -73,9 +73,10 @@ static ReorgTxns CreateBlocks(size_t num_not_shared)
 
 static void Reorg(const ReorgTxns& reorg)
 {
-    DisconnectedBlockTransactions disconnectpool;
+    DisconnectedBlockTransactions disconnectpool{MAX_DISCONNECTED_TX_POOL_SIZE * 1000};
     // Disconnect block
-    disconnectpool.AddTransactionsFromBlock(reorg.disconnected_txns);
+    const auto evicted = disconnectpool.AddTransactionsFromBlock(reorg.disconnected_txns);
+    assert(evicted.empty());
 
     // Connect first block
     disconnectpool.removeForBlock(reorg.connected_txns_1);

src/kernel/disconnected_transactions.h

@@ -13,6 +13,8 @@
 #include <list>
 #include <unordered_map>
 
+/** Maximum kilobytes for transactions to store for processing during reorg */
+static const unsigned int MAX_DISCONNECTED_TX_POOL_SIZE = 20000;
 /**
  * DisconnectedBlockTransactions
 
@@ -30,11 +32,14 @@
 class DisconnectedBlockTransactions {
 private:
     uint64_t cachedInnerUsage = 0;
+    const size_t m_max_mem_usage;
     std::list<CTransactionRef> queuedTx;
     using List = decltype(queuedTx);
     std::unordered_map<uint256, List::iterator, SaltedTxidHasher> iters_by_txid;
 
 public:
+    DisconnectedBlockTransactions(size_t max_mem_usage) : m_max_mem_usage{max_mem_usage} {}
+
     // It's almost certainly a logic bug if we don't clear out queuedTx before
     // destruction, as we add to it while disconnecting blocks, and then we
     // need to re-process remaining transactions to ensure mempool consistency.
@@ -56,8 +61,10 @@ class DisconnectedBlockTransactions {
     // Add transactions from the block, in reverse order. We assume that callers will never give us
     // multiple transactions with the same txid, otherwise things can go very wrong in
     // removeForBlock due to queuedTx containing an item without a corresponding entry in iters_by_txid.
-    void AddTransactionsFromBlock(const std::vector<CTransactionRef>& vtx)
+    // Returns vector of transactions that were evicted for size-limiting.
+    [[nodiscard]] std::vector<CTransactionRef> AddTransactionsFromBlock(const std::vector<CTransactionRef>& vtx)
     {
+        std::vector<CTransactionRef> evicted;
         // Blocks are disconnected in descending order by block height. Within each block, add
         // transactions in reverse order so that transactions with dependencies on other
         // transactions (if any) are at the beginning. If this data structure grows too large, we
@@ -69,6 +76,15 @@ class DisconnectedBlockTransactions {
             iters_by_txid.emplace((*block_it)->GetHash(), it);
             cachedInnerUsage += RecursiveDynamicUsage(*block_it);
         }
+
+        // Trim the earliest-added entries until we are within memory bounds.
+        while (!queuedTx.empty() && DynamicMemoryUsage() > m_max_mem_usage) {
+            evicted.emplace_back(queuedTx.front());
+            cachedInnerUsage -= RecursiveDynamicUsage(queuedTx.front());
+            iters_by_txid.erase(queuedTx.front()->GetHash());
+            queuedTx.pop_front();
+        }
+        return evicted;
     }
 
     // Remove entries that are in this block.
@@ -89,19 +105,6 @@ class DisconnectedBlockTransactions {
         }
     }
 
-    // Remove the first entry and update memory usage.
-    CTransactionRef take_first()
-    {
-        CTransactionRef first_tx;
-        if (!queuedTx.empty()) {
-            first_tx = queuedTx.front();
-            cachedInnerUsage -= RecursiveDynamicUsage(queuedTx.front());
-            iters_by_txid.erase(queuedTx.front()->GetHash());
-            queuedTx.pop_front();
-        }
-        return first_tx;
-    }
-
     size_t size() const { return queuedTx.size(); }
 
     void clear()

src/test/validation_chainstatemanager_tests.cpp

@@ -537,7 +537,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_init, SnapshotTestSetup)
     // it will initialize instead of attempting to complete validation.
     //
     // Note that this is not a realistic use of DisconnectTip().
-    DisconnectedBlockTransactions unused_pool;
+    DisconnectedBlockTransactions unused_pool{MAX_DISCONNECTED_TX_POOL_SIZE * 1000};
     BlockValidationState unused_state;
     {
         LOCK2(::cs_main, bg_chainstate.MempoolMutex());

src/validation.cpp

@@ -80,8 +80,6 @@ using node::CBlockIndexWorkComparator;
 using node::fReindex;
 using node::SnapshotMetadata;
 
-/** Maximum kilobytes for transactions to store for processing during reorg */
-static const unsigned int MAX_DISCONNECTED_TX_POOL_SIZE = 20000;
 /** Time to wait between writing blocks/block index to disk. */
 static constexpr std::chrono::hours DATABASE_WRITE_INTERVAL{1};
 /** Time to wait between flushing chainstate to disk. */
@@ -2724,11 +2722,9 @@ bool Chainstate::DisconnectTip(BlockValidationState& state, DisconnectedBlockTra
     }
 
     if (disconnectpool && m_mempool) {
-        // Save transactions to re-add to mempool at end of reorg
-        disconnectpool->AddTransactionsFromBlock(block.vtx);
-        while (disconnectpool->DynamicMemoryUsage() > MAX_DISCONNECTED_TX_POOL_SIZE * 1000) {
-            // Drop the earliest entry, and remove its children from the mempool.
-            auto ptx = disconnectpool->take_first();
+        // Save transactions to re-add to mempool at end of reorg. If any entries are dropped,
+        // remove their children from the mempool.
+        for (auto&& ptx : disconnectpool->AddTransactionsFromBlock(block.vtx)) {
             m_mempool->removeRecursive(*ptx, MemPoolRemovalReason::REORG);
         }
     }
@@ -2979,7 +2975,7 @@ bool Chainstate::ActivateBestChainStep(BlockValidationState& state, CBlockIndex*
 
     // Disconnect active blocks which are no longer in the best chain.
     bool fBlocksDisconnected = false;
-    DisconnectedBlockTransactions disconnectpool;
+    DisconnectedBlockTransactions disconnectpool{MAX_DISCONNECTED_TX_POOL_SIZE * 1000};
     while (m_chain.Tip() && m_chain.Tip() != pindexFork) {
         if (!DisconnectTip(state, &disconnectpool)) {
             // This is likely a fatal error, but keep the mempool consistent,
@@ -3313,7 +3309,7 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
 
         // ActivateBestChain considers blocks already in m_chain
         // unconditionally valid already, so force disconnect away from it.
-        DisconnectedBlockTransactions disconnectpool;
+        DisconnectedBlockTransactions disconnectpool{MAX_DISCONNECTED_TX_POOL_SIZE * 1000};
         bool ret = DisconnectTip(state, &disconnectpool);
         // DisconnectTip will add transactions to disconnectpool.
         // Adjust the mempool to be consistent with the new tip, adding