From 98c0b86d7cb8455e3252c264ec7133d1ebc2305a Mon Sep 17 00:00:00 2001
From: Cory Fields <cory-nospam-@coryfields.com>
Date: Tue, 5 Sep 2023 19:00:25 +0000
Subject: [PATCH] Fully encapsulate DisconnectedBlockTransactions

---
 src/kernel/disconnected_transactions.h | 20 ++++++++++++++++----
 src/validation.cpp                     | 14 +++++++-------
 2 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/src/kernel/disconnected_transactions.h b/src/kernel/disconnected_transactions.h
index 48ac5aa02f794..462c613eb4ad6 100644
--- a/src/kernel/disconnected_transactions.h
+++ b/src/kernel/disconnected_transactions.h
@@ -28,11 +28,13 @@
  * still-unconfirmed transactions at the end.
  */
 struct DisconnectedBlockTransactions {
+private:
     uint64_t cachedInnerUsage = 0;
     std::list<CTransactionRef> queuedTx;
     using Queue = decltype(queuedTx);
     std::unordered_map<uint256, Queue::iterator, SaltedTxidHasher> iters_by_txid;
 
+public:
     // 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.
@@ -75,11 +77,16 @@ struct DisconnectedBlockTransactions {
     }
 
     // Remove the first entry and update memory usage.
-    void remove_first()
+    CTransactionRef take_first()
     {
-        cachedInnerUsage -= RecursiveDynamicUsage(queuedTx.front());
-        iters_by_txid.erase(queuedTx.front()->GetHash());
-        queuedTx.pop_front();
+        CTransactionRef ret;
+        if (!queuedTx.empty()) {
+            ret = queuedTx.front();
+            cachedInnerUsage -= RecursiveDynamicUsage(queuedTx.front());
+            iters_by_txid.erase(queuedTx.front()->GetHash());
+            queuedTx.pop_front();
+        }
+        return ret;
     }
 
     void clear()
@@ -88,6 +95,11 @@ struct DisconnectedBlockTransactions {
         iters_by_txid.clear();
         queuedTx.clear();
     }
+    std::list<CTransactionRef> take() {
+        std::list<CTransactionRef> ret = queuedTx;
+        clear();
+        return ret;
+    }
 };
 
 #endif // BITCOIN_KERNEL_DISCONNECTED_TRANSACTIONS_H
diff --git a/src/validation.cpp b/src/validation.cpp
index cbb38ccc90612..4d0bf84953aef 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -302,8 +302,9 @@ void Chainstate::MaybeUpdateMempoolForReorg(
     // Iterate disconnectpool in reverse, so that we add transactions
     // back to the mempool starting with the earliest transaction that had
     // been previously seen in a block.
-    auto it = disconnectpool.queuedTx.rbegin();
-    while (it != disconnectpool.queuedTx.rend()) {
+    std::list<CTransactionRef> queuedTx = disconnectpool.take();
+    auto it = queuedTx.rbegin();
+    while (it != queuedTx.rend()) {
         // ignore errors related to fees in resurrected transactions.
         if (!fAddToMempool || (*it)->IsCoinBase() ||
             AcceptToMemoryPool(*this, *it, GetTime(),
@@ -317,7 +318,6 @@ void Chainstate::MaybeUpdateMempoolForReorg(
         }
         ++it;
     }
-    disconnectpool.queuedTx.clear();
     // AcceptToMemoryPool/addUnchecked all assume that new mempool entries have
     // no in-mempool children, which is generally not true when adding
     // previously-confirmed transactions back to the mempool.
@@ -2725,11 +2725,11 @@ bool Chainstate::DisconnectTip(BlockValidationState& state, DisconnectedBlockTra
         for (auto it = block.vtx.rbegin(); it != block.vtx.rend(); ++it) {
             disconnectpool->addTransaction(*it);
         }
-        while (!disconnectpool->queuedTx.empty() && disconnectpool->DynamicMemoryUsage() > MAX_DISCONNECTED_TX_POOL_SIZE * 1000) {
+        while (disconnectpool->DynamicMemoryUsage() > MAX_DISCONNECTED_TX_POOL_SIZE * 1000) {
             // Drop the earliest entry, and remove its children from the mempool.
-            auto ptx = disconnectpool->queuedTx.front();
-            m_mempool->removeRecursive(*ptx, MemPoolRemovalReason::REORG);
-            disconnectpool->remove_first();
+            if (auto ptx = disconnectpool->take_first()) {
+                m_mempool->removeRecursive(*ptx, MemPoolRemovalReason::REORG);
+            }
         }
     }