diff --git a/src/Makefile.am b/src/Makefile.am index 8508d13b349..99b2184cf25 100644 --- a/src/Makefile.am +++ b/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 \ diff --git a/src/Makefile.test.include b/src/Makefile.test.include index b610dabd075..4a42557372b 100644 --- a/src/Makefile.test.include +++ b/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 \ diff --git a/src/bench/disconnected_transactions.cpp b/src/bench/disconnected_transactions.cpp index 264c0aa1e86..91ce904dd94 100644 --- a/src/bench/disconnected_transactions.cpp +++ b/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()); diff --git a/src/kernel/disconnected_transactions.cpp b/src/kernel/disconnected_transactions.cpp new file mode 100644 index 00000000000..f865fed688c --- /dev/null +++ b/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 + +#include +#include +#include +#include +#include + +#include +#include + +// 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 DisconnectedBlockTransactions::LimitMemoryUsage() +{ + std::vector 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 DisconnectedBlockTransactions::AddTransactionsFromBlock(const std::vector& 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& 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 DisconnectedBlockTransactions::take() +{ + std::list ret = std::move(queuedTx); + clear(); + return ret; +} diff --git a/src/kernel/disconnected_transactions.h b/src/kernel/disconnected_transactions.h index 7db39ba5cae..401ec435e6b 100644 --- a/src/kernel/disconnected_transactions.h +++ b/src/kernel/disconnected_transactions.h @@ -5,8 +5,6 @@ #ifndef BITCOIN_KERNEL_DISCONNECTED_TRANSACTIONS_H #define BITCOIN_KERNEL_DISCONNECTED_TRANSACTIONS_H -#include -#include #include #include @@ -14,8 +12,8 @@ #include #include -/** 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,8 +36,7 @@ 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 queuedTx; @@ -47,39 +44,15 @@ private: std::unordered_map iters_by_txid; /** Trim the earliest-added entries until we are within memory bounds. */ - std::vector LimitMemoryUsage() - { - std::vector 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 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 @@ public: * corresponding entry in iters_by_txid. * @returns vector of transactions that were evicted for size-limiting. */ - [[nodiscard]] std::vector AddTransactionsFromBlock(const std::vector& 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 AddTransactionsFromBlock(const std::vector& vtx); /** Remove any entries that are in this block. */ - void removeForBlock(const std::vector& 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& 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 take() - { - std::list ret = std::move(queuedTx); - clear(); - return ret; - } + std::list take(); }; #endif // BITCOIN_KERNEL_DISCONNECTED_TRANSACTIONS_H diff --git a/src/test/disconnected_transactions.cpp b/src/test/disconnected_transactions.cpp new file mode 100644 index 00000000000..d4dc124b7b1 --- /dev/null +++ b/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 +#include +#include +#include + +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 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 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 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() diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index 227d7d4633e..e2541a74fd6 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/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()); diff --git a/src/validation.cpp b/src/validation.cpp index 471bedfe62b..ed4ea70ecb8 100644 --- a/src/validation.cpp +++ b/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