Merge bitcoin/bitcoin#28530: tests, bug fix: DisconnectedBlockTransactions rewrite followups

9b3da70 [test] DisconnectedBlockTransactions::DynamicMemoryUsage (glozow)
b2d0447 bugfix: correct DisconnectedBlockTransactions memory usage (stickies-v)
f4254e2 assume duplicate transactions are not added to `iters_by_txid` (ismaelsadeeq)
29eb219 move only: move implementation code to disconnected_transactions.cpp (ismaelsadeeq)
81dfedd refactor: update `MAX_DISCONNECTED_TX_POOL` from kb to bytes (ismaelsadeeq)

Pull request description:

  This PR is a follow-up to fix review comments and a bugfix from #28385

  The PR

  - Updated `DisconnectedBlockTransactions`'s `MAX_DISCONNECTED_TX_POOL` from kb to bytes.
  - Moved `DisconnectedBlockTransactions` implementation code to `kernel/disconnected_transactions.cpp`.
  - `AddTransactionsFromBlock` now assume duplicate transactions are not passed by asserting after inserting each transaction to `iters_by_txid`.
  - Included a Bug fix: In the current master we are underestimating the memory usage of `DisconnectedBlockTransactions`.

      * When adding and subtracting `cachedInnerUsage` we call `RecursiveDynamicUsage` with `CTransaction` which invokes this [`RecursiveDynamicUsage(const CTransaction& tx)`](https://github.com/bitcoin/bitcoin/blob/6e721c923c87abdb8d99674093d08d8c17bc52c2/src/core_memusage.h#L32) version of `RecursiveDynamicUsage`, the output of that call only account for the memory usage of the inputs and outputs of the `CTransaction`, this omits the memory usage of the `CTransaction` object and the control block.
      * This PR fixes this bug by calling `RecursiveDynamicUsage` with `CTransactionRef` when adding and subtracting `cachedInnerUsage` which invokes [`RecursiveDynamicUsage(const std::shared_ptr<X>& p)`](https://github.com/bitcoin/bitcoin/blob/6e721c923c87abdb8d99674093d08d8c17bc52c2/src/core_memusage.h#L67) version of `RecursiveDynamicUsage` the output of the calculation accounts for the` CTransaction` object, the control blocks, inputs and outputs memory usage.
      * see  [comment ](bitcoin/bitcoin#28385 (comment))
  - Added test for DisconnectedBlockTransactions memory limit.

ACKs for top commit:
  stickies-v:
    ACK 9b3da70 - nice work!
  BrandonOdiwuor:
    re ACK 9b3da70
  glozow:
    ACK 9b3da70

Tree-SHA512: 69b9595d09f4d0209038f97081d790cea92ccf63efb94e9e372749979fcbe527f7f17a8e454720cedd12021be0c8e11cf99874625d3dafd9ec602b12dbeb4098

Author: glozow

src/Makefile.am

@@ -404,6 +404,7 @@ libbitcoin_node_a_SOURCES = \
   kernel/coinstats.cpp \
   kernel/context.cpp \
   kernel/cs_main.cpp \
+  kernel/disconnected_transactions.cpp \
   kernel/mempool_persist.cpp \
   kernel/mempool_removal_reason.cpp \
   mapport.cpp \
@@ -944,6 +945,7 @@ libbitcoinkernel_la_SOURCES = \
   kernel/coinstats.cpp \
   kernel/context.cpp \
   kernel/cs_main.cpp \
+  kernel/disconnected_transactions.cpp \
   kernel/mempool_persist.cpp \
   kernel/mempool_removal_reason.cpp \
   key.cpp \

src/Makefile.test.include

@@ -92,6 +92,7 @@ BITCOIN_TESTS =\
   test/dbwrapper_tests.cpp \
   test/denialofservice_tests.cpp \
   test/descriptor_tests.cpp \
+  test/disconnected_transactions.cpp \
   test/flatfile_tests.cpp \
   test/fs_tests.cpp \
   test/getarg_tests.cpp \

src/bench/disconnected_transactions.cpp

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

src/kernel/disconnected_transactions.cpp

@@ -0,0 +1,90 @@
+// Copyright (c) 2023 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <kernel/disconnected_transactions.h>
+
+#include <assert.h>
+#include <core_memusage.h>
+#include <memusage.h>
+#include <primitives/transaction.h>
+#include <util/hasher.h>
+
+#include <memory>
+#include <utility>
+
+// 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.
+// For now, assert() that we've emptied out this object on destruction.
+// This assert() can always be removed if the reorg-processing code were
+// to be refactored such that this assumption is no longer true (for
+// instance if there was some other way we cleaned up the mempool after a
+// reorg, besides draining this object).
+DisconnectedBlockTransactions::~DisconnectedBlockTransactions()
+{
+    assert(queuedTx.empty());
+    assert(iters_by_txid.empty());
+    assert(cachedInnerUsage == 0);
+}
+
+std::vector<CTransactionRef> DisconnectedBlockTransactions::LimitMemoryUsage()
+{
+    std::vector<CTransactionRef> evicted;
+
+    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;
+}
+
+size_t DisconnectedBlockTransactions::DynamicMemoryUsage() const
+{
+    return cachedInnerUsage + memusage::DynamicUsage(iters_by_txid) + memusage::DynamicUsage(queuedTx);
+}
+
+[[nodiscard]] std::vector<CTransactionRef> DisconnectedBlockTransactions::AddTransactionsFromBlock(const std::vector<CTransactionRef>& vtx)
+{
+    iters_by_txid.reserve(iters_by_txid.size() + vtx.size());
+    for (auto block_it = vtx.rbegin(); block_it != vtx.rend(); ++block_it) {
+        auto it = queuedTx.insert(queuedTx.end(), *block_it);
+        auto [_, inserted] = iters_by_txid.emplace((*block_it)->GetHash(), it);
+        assert(inserted); // callers may never pass multiple transactions with the same txid
+        cachedInnerUsage += RecursiveDynamicUsage(*block_it);
+    }
+    return LimitMemoryUsage();
+}
+
+void DisconnectedBlockTransactions::removeForBlock(const std::vector<CTransactionRef>& vtx)
+{
+    // Short-circuit in the common case of a block being added to the tip
+    if (queuedTx.empty()) {
+        return;
+    }
+    for (const auto& tx : vtx) {
+        auto iter = iters_by_txid.find(tx->GetHash());
+        if (iter != iters_by_txid.end()) {
+            auto list_iter = iter->second;
+            iters_by_txid.erase(iter);
+            cachedInnerUsage -= RecursiveDynamicUsage(*list_iter);
+            queuedTx.erase(list_iter);
+        }
+    }
+}
+
+void DisconnectedBlockTransactions::clear()
+{
+    cachedInnerUsage = 0;
+    iters_by_txid.clear();
+    queuedTx.clear();
+}
+
+std::list<CTransactionRef> DisconnectedBlockTransactions::take()
+{
+    std::list<CTransactionRef> ret = std::move(queuedTx);
+    clear();
+    return ret;
+}

src/kernel/disconnected_transactions.h

@@ -5,17 +5,15 @@
 #ifndef BITCOIN_KERNEL_DISCONNECTED_TRANSACTIONS_H
 #define BITCOIN_KERNEL_DISCONNECTED_TRANSACTIONS_H
 
-#include <core_memusage.h>
-#include <memusage.h>
 #include <primitives/transaction.h>
 #include <util/hasher.h>
 
 #include <list>
 #include <unordered_map>
 #include <vector>
 
-/** Maximum kilobytes for transactions to store for processing during reorg */
-static const unsigned int MAX_DISCONNECTED_TX_POOL_SIZE = 20'000;
+/** Maximum bytes for transactions to store for processing during reorg */
+static const unsigned int MAX_DISCONNECTED_TX_POOL_BYTES{20'000'000};
 /**
  * DisconnectedBlockTransactions
 
@@ -38,48 +36,23 @@ static const unsigned int MAX_DISCONNECTED_TX_POOL_SIZE = 20'000;
  */
 class DisconnectedBlockTransactions {
 private:
-    /** Cached dynamic memory usage for the CTransactions (memory for the shared pointers is
-     * included in the container calculations). */
+    /** Cached dynamic memory usage for the `CTransactionRef`s */
     uint64_t cachedInnerUsage = 0;
     const size_t m_max_mem_usage;
     std::list<CTransactionRef> queuedTx;
     using TxList = decltype(queuedTx);
     std::unordered_map<uint256, TxList::iterator, SaltedTxidHasher> iters_by_txid;
 
     /** Trim the earliest-added entries until we are within memory bounds. */
-    std::vector<CTransactionRef> LimitMemoryUsage()
-    {
-        std::vector<CTransactionRef> evicted;
-
-        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;
-    }
+    std::vector<CTransactionRef> LimitMemoryUsage();
 
 public:
-    DisconnectedBlockTransactions(size_t max_mem_usage) : m_max_mem_usage{max_mem_usage} {}
+    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.
-    // For now, assert() that we've emptied out this object on destruction.
-    // This assert() can always be removed if the reorg-processing code were
-    // to be refactored such that this assumption is no longer true (for
-    // instance if there was some other way we cleaned up the mempool after a
-    // reorg, besides draining this object).
-    ~DisconnectedBlockTransactions() {
-        assert(queuedTx.empty());
-        assert(iters_by_txid.empty());
-        assert(cachedInnerUsage == 0);
-    }
+    ~DisconnectedBlockTransactions();
 
-    size_t DynamicMemoryUsage() const {
-        return cachedInnerUsage + memusage::DynamicUsage(iters_by_txid) + memusage::DynamicUsage(queuedTx);
-    }
+    size_t DynamicMemoryUsage() const;
 
     /** Add transactions from the block, iterating through vtx in reverse order. Callers should call
      * this function for blocks in descending order by block height.
@@ -88,50 +61,16 @@ class DisconnectedBlockTransactions {
      * corresponding entry in iters_by_txid.
      * @returns vector of transactions that were evicted for size-limiting.
      */
-    [[nodiscard]] std::vector<CTransactionRef> AddTransactionsFromBlock(const std::vector<CTransactionRef>& vtx)
-    {
-        iters_by_txid.reserve(iters_by_txid.size() + vtx.size());
-        for (auto block_it = vtx.rbegin(); block_it != vtx.rend(); ++block_it) {
-            auto it = queuedTx.insert(queuedTx.end(), *block_it);
-            iters_by_txid.emplace((*block_it)->GetHash(), it);
-            cachedInnerUsage += RecursiveDynamicUsage(**block_it);
-        }
-        return LimitMemoryUsage();
-    }
+    [[nodiscard]] std::vector<CTransactionRef> AddTransactionsFromBlock(const std::vector<CTransactionRef>& vtx);
 
     /** Remove any entries that are in this block. */
-    void removeForBlock(const std::vector<CTransactionRef>& vtx)
-    {
-        // Short-circuit in the common case of a block being added to the tip
-        if (queuedTx.empty()) {
-            return;
-        }
-        for (const auto& tx : vtx) {
-            auto iter = iters_by_txid.find(tx->GetHash());
-            if (iter != iters_by_txid.end()) {
-                auto list_iter = iter->second;
-                iters_by_txid.erase(iter);
-                cachedInnerUsage -= RecursiveDynamicUsage(**list_iter);
-                queuedTx.erase(list_iter);
-            }
-        }
-    }
+    void removeForBlock(const std::vector<CTransactionRef>& vtx);
 
     size_t size() const { return queuedTx.size(); }
 
-    void clear()
-    {
-        cachedInnerUsage = 0;
-        iters_by_txid.clear();
-        queuedTx.clear();
-    }
+    void clear();
 
     /** Clear all data structures and return the list of transactions. */
-    std::list<CTransactionRef> take()
-    {
-        std::list<CTransactionRef> ret = std::move(queuedTx);
-        clear();
-        return ret;
-    }
+    std::list<CTransactionRef> take();
 };
 #endif // BITCOIN_KERNEL_DISCONNECTED_TRANSACTIONS_H

src/test/disconnected_transactions.cpp

@@ -0,0 +1,95 @@
+// Copyright (c) 2023 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+//
+#include <boost/test/unit_test.hpp>
+#include <core_memusage.h>
+#include <kernel/disconnected_transactions.h>
+#include <test/util/setup_common.h>
+
+BOOST_FIXTURE_TEST_SUITE(disconnected_transactions, TestChain100Setup)
+
+//! Tests that DisconnectedBlockTransactions limits its own memory properly
+BOOST_AUTO_TEST_CASE(disconnectpool_memory_limits)
+{
+    // Use the coinbase transactions from TestChain100Setup. It doesn't matter whether these
+    // transactions would realistically be in a block together, they just need distinct txids and
+    // uniform size for this test to work.
+    std::vector<CTransactionRef> block_vtx(m_coinbase_txns);
+    BOOST_CHECK_EQUAL(block_vtx.size(), 100);
+
+    // Roughly estimate sizes to sanity check that DisconnectedBlockTransactions::DynamicMemoryUsage
+    // is within an expected range.
+
+    // Overhead for the hashmap depends on number of buckets
+    std::unordered_map<uint256, CTransaction*, SaltedTxidHasher> temp_map;
+    temp_map.reserve(1);
+    const size_t MAP_1{memusage::DynamicUsage(temp_map)};
+    temp_map.reserve(100);
+    const size_t MAP_100{memusage::DynamicUsage(temp_map)};
+
+    const size_t TX_USAGE{RecursiveDynamicUsage(block_vtx.front())};
+    for (const auto& tx : block_vtx)
+        BOOST_CHECK_EQUAL(RecursiveDynamicUsage(tx), TX_USAGE);
+
+    // Our overall formula is unordered map overhead + usage per entry.
+    // Implementations may vary, but we're trying to guess the usage of data structures.
+    const size_t ENTRY_USAGE_ESTIMATE{
+        TX_USAGE
+        // list entry: 2 pointers (next pointer and prev pointer) + element itself
+        + memusage::MallocUsage((2 * sizeof(void*)) + sizeof(decltype(block_vtx)::value_type))
+        // unordered map: 1 pointer for the hashtable + key and value
+        + memusage::MallocUsage(sizeof(void*) + sizeof(decltype(temp_map)::key_type)
+                                + sizeof(decltype(temp_map)::value_type))};
+
+    // DisconnectedBlockTransactions that's just big enough for 1 transaction.
+    {
+        DisconnectedBlockTransactions disconnectpool{MAP_1 + ENTRY_USAGE_ESTIMATE};
+        // Add just 2 (and not all 100) transactions to keep the unordered map's hashtable overhead
+        // to a minimum and avoid all (instead of all but 1) transactions getting evicted.
+        std::vector<CTransactionRef> two_txns({block_vtx.at(0), block_vtx.at(1)});
+        auto evicted_txns{disconnectpool.AddTransactionsFromBlock(two_txns)};
+        BOOST_CHECK(disconnectpool.DynamicMemoryUsage() <= MAP_1 + ENTRY_USAGE_ESTIMATE);
+
+        // Only 1 transaction can be kept
+        BOOST_CHECK_EQUAL(1, evicted_txns.size());
+        // Transactions are added from back to front and eviction is FIFO.
+        BOOST_CHECK_EQUAL(block_vtx.at(1), evicted_txns.front());
+
+        disconnectpool.clear();
+    }
+
+    // DisconnectedBlockTransactions with a comfortable maximum memory usage so that nothing is evicted.
+    // Record usage so we can check size limiting in the next test.
+    size_t usage_full{0};
+    {
+        const size_t USAGE_100_OVERESTIMATE{MAP_100 + ENTRY_USAGE_ESTIMATE * 100};
+        DisconnectedBlockTransactions disconnectpool{USAGE_100_OVERESTIMATE};
+        auto evicted_txns{disconnectpool.AddTransactionsFromBlock(block_vtx)};
+        BOOST_CHECK_EQUAL(evicted_txns.size(), 0);
+        BOOST_CHECK(disconnectpool.DynamicMemoryUsage() <= USAGE_100_OVERESTIMATE);
+
+        usage_full = disconnectpool.DynamicMemoryUsage();
+
+        disconnectpool.clear();
+    }
+
+    // DisconnectedBlockTransactions that's just a little too small for all of the transactions.
+    {
+        const size_t MAX_MEMUSAGE_99{usage_full - sizeof(void*)};
+        DisconnectedBlockTransactions disconnectpool{MAX_MEMUSAGE_99};
+        auto evicted_txns{disconnectpool.AddTransactionsFromBlock(block_vtx)};
+        BOOST_CHECK(disconnectpool.DynamicMemoryUsage() <= MAX_MEMUSAGE_99);
+
+        // Only 1 transaction needed to be evicted
+        BOOST_CHECK_EQUAL(1, evicted_txns.size());
+
+        // Transactions are added from back to front and eviction is FIFO.
+        // The last transaction of block_vtx should be the first to be evicted.
+        BOOST_CHECK_EQUAL(block_vtx.back(), evicted_txns.front());
+
+        disconnectpool.clear();
+    }
+}
+
+BOOST_AUTO_TEST_SUITE_END()

src/test/validation_chainstatemanager_tests.cpp

@@ -579,7 +579,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{MAX_DISCONNECTED_TX_POOL_SIZE * 1000};
+    DisconnectedBlockTransactions unused_pool{MAX_DISCONNECTED_TX_POOL_BYTES};
     BlockValidationState unused_state;
     {
         LOCK2(::cs_main, bg_chainstate.MempoolMutex());

src/validation.cpp

@@ -3062,7 +3062,7 @@ bool Chainstate::ActivateBestChainStep(BlockValidationState& state, CBlockIndex*
 
     // Disconnect active blocks which are no longer in the best chain.
     bool fBlocksDisconnected = false;
-    DisconnectedBlockTransactions disconnectpool{MAX_DISCONNECTED_TX_POOL_SIZE * 1000};
+    DisconnectedBlockTransactions disconnectpool{MAX_DISCONNECTED_TX_POOL_BYTES};
     while (m_chain.Tip() && m_chain.Tip() != pindexFork) {
         if (!DisconnectTip(state, &disconnectpool)) {
             // This is likely a fatal error, but keep the mempool consistent,
@@ -3420,7 +3420,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{MAX_DISCONNECTED_TX_POOL_SIZE * 1000};
+        DisconnectedBlockTransactions disconnectpool{MAX_DISCONNECTED_TX_POOL_BYTES};
         bool ret = DisconnectTip(state, &disconnectpool);
         // DisconnectTip will add transactions to disconnectpool.
         // Adjust the mempool to be consistent with the new tip, adding