Merge bitcoin/bitcoin#28385: [refactor] rewrite DisconnectedBlockTransactions to not use boost

4313c77 make DisconnectedBlockTransactions responsible for its own memory management (glozow)
cf5f1fa MOVEONLY: DisconnectedBlockTransactions to its own file (glozow)
2765d6f rewrite DisconnectedBlockTransactions as a list + map (glozow)
79ce9f0 add std::list to memusage (glozow)
59a35a7 [bench] DisconnectedBlockTransactions (glozow)
925bb72 [refactor] batch-add transactions to DisconnectedBlockTransactions (glozow)

Pull request description:

  Motivation
  - I think it's preferable to use stdlib data structures instead of depending on boost if we can achieve the same thing.
  - Also see #28335 for further context/motivation. This PR simplifies that one.

  Things done in this PR:
  - Add a bench for `DisconnectedBlockTransactions` where we reorg and the new chain has {100%, 90%, 10%} of the same transactions. AFAIU in practice, it's usually close to 100%.
  - Rewrite `DisconnectedBlockTransactions` as a `std::list` + `unordered_map` instead of a boost multi index container.
    - On my machine, the bench suggests the performance is very similar.
  - Move `DisconnectedBlockTransactions` from txmempool.h to its own kernel/disconnected_transactions.h. This struct isn't used by txmempool and doesn't have much to do with txmempool. My guess is that it's been living there for convenience since the boost includes are there.

ACKs for top commit:
  ismaelsadeeq:
    Tested ACK 4313c77
  stickies-v:
    ACK 4313c77
  TheCharlatan:
    ACK 4313c77

Tree-SHA512: 273c80866bf3acd39b2a039dc082b7719d2d82e0940e1eb6c402f1c0992e997256722b85c7e310c9811238a770cfbdeb122ea4babbc23835d17128f214a1ef9e

Author: fanquake

src/Makefile.am

@@ -186,6 +186,7 @@ BITCOIN_CORE_H = \
   kernel/coinstats.h \
   kernel/context.h \
   kernel/cs_main.h \
+  kernel/disconnected_transactions.h \
   kernel/mempool_entry.h \
   kernel/mempool_limits.h \
   kernel/mempool_options.h \

src/Makefile.bench.include

@@ -28,6 +28,7 @@ bench_bench_bitcoin_SOURCES = \
   bench/data.cpp \
   bench/data.h \
   bench/descriptors.cpp \
+  bench/disconnected_transactions.cpp \
   bench/duplicate_inputs.cpp \
   bench/ellswift.cpp \
   bench/examples.cpp \

src/bench/disconnected_transactions.cpp

@@ -0,0 +1,130 @@
+// 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 <bench/bench.h>
+#include <kernel/disconnected_transactions.h>
+#include <primitives/block.h>
+#include <test/util/random.h>
+#include <test/util/setup_common.h>
+
+constexpr size_t BLOCK_VTX_COUNT{4000};
+constexpr size_t BLOCK_VTX_COUNT_10PERCENT{400};
+
+using BlockTxns = decltype(CBlock::vtx);
+
+/** Reorg where 1 block is disconnected and 2 blocks are connected. */
+struct ReorgTxns {
+    /** Disconnected block. */
+    BlockTxns disconnected_txns;
+    /** First connected block. */
+    BlockTxns connected_txns_1;
+    /** Second connected block, new chain tip. Has no overlap with disconnected_txns. */
+    BlockTxns connected_txns_2;
+    /** Transactions shared between disconnected_txns and connected_txns_1. */
+    size_t num_shared;
+};
+
+static BlockTxns CreateRandomTransactions(size_t num_txns)
+{
+    // Ensure every transaction has a different txid by having each one spend the previous one.
+    static uint256 prevout_hash{uint256::ZERO};
+
+    BlockTxns txns;
+    txns.reserve(num_txns);
+    // Simplest spk for every tx
+    CScript spk = CScript() << OP_TRUE;
+    for (uint32_t i = 0; i < num_txns; ++i) {
+        CMutableTransaction tx;
+        tx.vin.emplace_back(CTxIn{COutPoint{prevout_hash, 0}});
+        tx.vout.emplace_back(CTxOut{CENT, spk});
+        auto ptx{MakeTransactionRef(tx)};
+        txns.emplace_back(ptx);
+        prevout_hash = ptx->GetHash();
+    }
+    return txns;
+}
+
+/** Creates blocks for a Reorg, each with BLOCK_VTX_COUNT transactions. Between the disconnected
+ * block and the first connected block, there will be num_not_shared transactions that are
+ * different, and all other transactions the exact same. The second connected block has all unique
+ * transactions. This is to simulate a reorg in which all but num_not_shared transactions are
+ * confirmed in the new chain. */
+static ReorgTxns CreateBlocks(size_t num_not_shared)
+{
+    auto num_shared{BLOCK_VTX_COUNT - num_not_shared};
+    const auto shared_txns{CreateRandomTransactions(/*num_txns=*/num_shared)};
+
+    // Create different sets of transactions...
+    auto disconnected_block_txns{CreateRandomTransactions(/*num_txns=*/num_not_shared)};
+    std::copy(shared_txns.begin(), shared_txns.end(), std::back_inserter(disconnected_block_txns));
+
+    auto connected_block_txns{CreateRandomTransactions(/*num_txns=*/num_not_shared)};
+    std::copy(shared_txns.begin(), shared_txns.end(), std::back_inserter(connected_block_txns));
+
+    assert(disconnected_block_txns.size() == BLOCK_VTX_COUNT);
+    assert(connected_block_txns.size() == BLOCK_VTX_COUNT);
+
+    return ReorgTxns{/*disconnected_txns=*/disconnected_block_txns,
+                     /*connected_txns_1=*/connected_block_txns,
+                     /*connected_txns_2=*/CreateRandomTransactions(BLOCK_VTX_COUNT),
+                     /*num_shared=*/num_shared};
+}
+
+static void Reorg(const ReorgTxns& reorg)
+{
+    DisconnectedBlockTransactions disconnectpool{MAX_DISCONNECTED_TX_POOL_SIZE * 1000};
+    // Disconnect block
+    const auto evicted = disconnectpool.AddTransactionsFromBlock(reorg.disconnected_txns);
+    assert(evicted.empty());
+
+    // Connect first block
+    disconnectpool.removeForBlock(reorg.connected_txns_1);
+    // Connect new tip
+    disconnectpool.removeForBlock(reorg.connected_txns_2);
+
+    // Sanity Check
+    assert(disconnectpool.size() == BLOCK_VTX_COUNT - reorg.num_shared);
+
+    disconnectpool.clear();
+}
+
+/** Add transactions from DisconnectedBlockTransactions, remove all but one (the disconnected
+ * block's coinbase transaction) of them, and then pop from the front until empty. This is a reorg
+ * in which all of the non-coinbase transactions in the disconnected chain also exist in the new
+ * chain. */
+static void AddAndRemoveDisconnectedBlockTransactionsAll(benchmark::Bench& bench)
+{
+    const auto chains{CreateBlocks(/*num_not_shared=*/1)};
+    assert(chains.num_shared == BLOCK_VTX_COUNT - 1);
+
+    bench.minEpochIterations(10).run([&]() NO_THREAD_SAFETY_ANALYSIS {
+        Reorg(chains);
+    });
+}
+
+/** Add transactions from DisconnectedBlockTransactions, remove 90% of them, and then pop from the front until empty. */
+static void AddAndRemoveDisconnectedBlockTransactions90(benchmark::Bench& bench)
+{
+    const auto chains{CreateBlocks(/*num_not_shared=*/BLOCK_VTX_COUNT_10PERCENT)};
+    assert(chains.num_shared == BLOCK_VTX_COUNT - BLOCK_VTX_COUNT_10PERCENT);
+
+    bench.minEpochIterations(10).run([&]() NO_THREAD_SAFETY_ANALYSIS {
+        Reorg(chains);
+    });
+}
+
+/** Add transactions from DisconnectedBlockTransactions, remove 10% of them, and then pop from the front until empty. */
+static void AddAndRemoveDisconnectedBlockTransactions10(benchmark::Bench& bench)
+{
+    const auto chains{CreateBlocks(/*num_not_shared=*/BLOCK_VTX_COUNT - BLOCK_VTX_COUNT_10PERCENT)};
+    assert(chains.num_shared == BLOCK_VTX_COUNT_10PERCENT);
+
+    bench.minEpochIterations(10).run([&]() NO_THREAD_SAFETY_ANALYSIS {
+        Reorg(chains);
+    });
+}
+
+BENCHMARK(AddAndRemoveDisconnectedBlockTransactionsAll, benchmark::PriorityLevel::HIGH);
+BENCHMARK(AddAndRemoveDisconnectedBlockTransactions90, benchmark::PriorityLevel::HIGH);
+BENCHMARK(AddAndRemoveDisconnectedBlockTransactions10, benchmark::PriorityLevel::HIGH);

src/kernel/disconnected_transactions.h

@@ -0,0 +1,137 @@
+// 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.
+
+#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;
+/**
+ * DisconnectedBlockTransactions
+
+ * During the reorg, it's desirable to re-add previously confirmed transactions
+ * to the mempool, so that anything not re-confirmed in the new chain is
+ * available to be mined. However, it's more efficient to wait until the reorg
+ * is complete and process all still-unconfirmed transactions at that time,
+ * since we expect most confirmed transactions to (typically) still be
+ * confirmed in the new chain, and re-accepting to the memory pool is expensive
+ * (and therefore better to not do in the middle of reorg-processing).
+ * Instead, store the disconnected transactions (in order!) as we go, remove any
+ * that are included in blocks in the new chain, and then process the remaining
+ * still-unconfirmed transactions at the end.
+ *
+ * Order of queuedTx:
+ * The front of the list should be the most recently-confirmed transactions (transactions at the
+ * end of vtx of blocks closer to the tip). If memory usage grows too large, we trim from the front
+ * of the list. After trimming, transactions can be re-added to the mempool from the back of the
+ * list to the front without running into missing inputs.
+ */
+class DisconnectedBlockTransactions {
+private:
+    /** Cached dynamic memory usage for the CTransactions (memory for the shared pointers is
+     * included in the container calculations). */
+    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;
+    }
+
+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.
+    // 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);
+    }
+
+    size_t DynamicMemoryUsage() const {
+        return cachedInnerUsage + memusage::DynamicUsage(iters_by_txid) + memusage::DynamicUsage(queuedTx);
+    }
+
+    /** Add transactions from the block, iterating through vtx in reverse order. Callers should call
+     * this function for blocks in descending order by block height.
+     * We assume that callers never pass 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.
+     * @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();
+    }
+
+    /** 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);
+            }
+        }
+    }
+
+    size_t size() const { return queuedTx.size(); }
+
+    void clear()
+    {
+        cachedInnerUsage = 0;
+        iters_by_txid.clear();
+        queuedTx.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;
+    }
+};
+#endif // BITCOIN_KERNEL_DISCONNECTED_TRANSACTIONS_H

src/memusage.h

@@ -11,6 +11,7 @@
 
 #include <cassert>
 #include <cstdlib>
+#include <list>
 #include <map>
 #include <memory>
 #include <set>
@@ -148,6 +149,21 @@ static inline size_t DynamicUsage(const std::shared_ptr<X>& p)
     return p ? MallocUsage(sizeof(X)) + MallocUsage(sizeof(stl_shared_counter)) : 0;
 }
 
+template<typename X>
+struct list_node
+{
+private:
+    void* ptr_next;
+    void* ptr_prev;
+    X x;
+};
+
+template<typename X>
+static inline size_t DynamicUsage(const std::list<X>& l)
+{
+    return MallocUsage(sizeof(list_node<X>)) * l.size();
+}
+
 template<typename X>
 struct unordered_node : private X
 {

src/test/validation_chainstatemanager_tests.cpp

@@ -4,6 +4,7 @@
 //
 #include <chainparams.h>
 #include <consensus/validation.h>
+#include <kernel/disconnected_transactions.h>
 #include <node/kernel_notifications.h>
 #include <node/utxo_snapshot.h>
 #include <random.h>
@@ -536,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());