From 925bb723ca71aa76380b769d8926c7c2ad9bbb7b Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 6 Sep 2023 11:24:42 +0100 Subject: [PATCH 1/6] [refactor] batch-add transactions to DisconnectedBlockTransactions No behavior change. In a future commit, we can optimize by reserving vtx.size(). --- src/txmempool.h | 14 +++++++++++--- src/validation.cpp | 4 +--- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/txmempool.h b/src/txmempool.h index 869612a4a2d..8b4ac2dce1d 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -892,10 +892,18 @@ struct DisconnectedBlockTransactions { return memusage::MallocUsage(sizeof(CTransactionRef) + 6 * sizeof(void*)) * queuedTx.size() + cachedInnerUsage; } - void addTransaction(const CTransactionRef& tx) + /** 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. + */ + void AddTransactionsFromBlock(const std::vector& vtx) { - queuedTx.insert(tx); - cachedInnerUsage += RecursiveDynamicUsage(tx); + for (auto block_it = vtx.rbegin(); block_it != vtx.rend(); ++block_it) { + queuedTx.insert(*block_it); + cachedInnerUsage += RecursiveDynamicUsage(*block_it); + } } // Remove entries based on txid_index, and update memory usage. diff --git a/src/validation.cpp b/src/validation.cpp index 0b6327ec558..9326dad10dd 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2721,9 +2721,7 @@ bool Chainstate::DisconnectTip(BlockValidationState& state, DisconnectedBlockTra if (disconnectpool && m_mempool) { // Save transactions to re-add to mempool at end of reorg - for (auto it = block.vtx.rbegin(); it != block.vtx.rend(); ++it) { - disconnectpool->addTransaction(*it); - } + 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 it = disconnectpool->queuedTx.get().begin(); From 59a35a7398f5bcb3e3805d1e4f363e4c2fb336b3 Mon Sep 17 00:00:00 2001 From: glozow Date: Fri, 1 Sep 2023 11:58:23 +0100 Subject: [PATCH 2/6] [bench] DisconnectedBlockTransactions --- src/Makefile.bench.include | 1 + src/bench/disconnected_transactions.cpp | 128 ++++++++++++++++++++++++ 2 files changed, 129 insertions(+) create mode 100644 src/bench/disconnected_transactions.cpp diff --git a/src/Makefile.bench.include b/src/Makefile.bench.include index 934e9a1fae1..28b779a5a88 100644 --- a/src/Makefile.bench.include +++ b/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 \ diff --git a/src/bench/disconnected_transactions.cpp b/src/bench/disconnected_transactions.cpp new file mode 100644 index 00000000000..77c8690ce4a --- /dev/null +++ b/src/bench/disconnected_transactions.cpp @@ -0,0 +1,128 @@ +// 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 + +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; + // Disconnect block + disconnectpool.AddTransactionsFromBlock(reorg.disconnected_txns); + + // Connect first block + disconnectpool.removeForBlock(reorg.connected_txns_1); + // Connect new tip + disconnectpool.removeForBlock(reorg.connected_txns_2); + + // Sanity Check + assert(disconnectpool.queuedTx.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); From 79ce9f0aa46de8ff742be83fd6f68eab40e073ec Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 6 Sep 2023 10:49:31 +0100 Subject: [PATCH 3/6] add std::list to memusage --- src/memusage.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/memusage.h b/src/memusage.h index bb39066a7df..08be66172e3 100644 --- a/src/memusage.h +++ b/src/memusage.h @@ -11,6 +11,7 @@ #include #include +#include #include #include #include @@ -148,6 +149,21 @@ static inline size_t DynamicUsage(const std::shared_ptr& p) return p ? MallocUsage(sizeof(X)) + MallocUsage(sizeof(stl_shared_counter)) : 0; } +template +struct list_node +{ +private: + void* ptr_next; + void* ptr_prev; + X x; +}; + +template +static inline size_t DynamicUsage(const std::list& l) +{ + return MallocUsage(sizeof(list_node)) * l.size(); +} + template struct unordered_node : private X { From 2765d6f3434c101fe2d46e9313e540aa680fbd77 Mon Sep 17 00:00:00 2001 From: glozow Date: Fri, 25 Aug 2023 16:26:22 +0100 Subject: [PATCH 4/6] rewrite DisconnectedBlockTransactions as a list + map And encapsulate underlying data structures to avoid misuse. It's better to use stdlib instead of boost when we can achieve the same thing. Behavior change: the number returned by DynamicMemoryUsage for the same transactions is higher (due to change in usage or more accurate accounting), which effectively decreases the maximum amount of transactions kept for resubmission in a reorg. Co-authored-by: Cory Fields --- src/bench/disconnected_transactions.cpp | 2 +- src/txmempool.h | 100 ++++++++++++++---------- src/validation.cpp | 47 +++++------ src/validation.h | 2 +- 4 files changed, 83 insertions(+), 68 deletions(-) diff --git a/src/bench/disconnected_transactions.cpp b/src/bench/disconnected_transactions.cpp index 77c8690ce4a..096165bd1af 100644 --- a/src/bench/disconnected_transactions.cpp +++ b/src/bench/disconnected_transactions.cpp @@ -82,7 +82,7 @@ static void Reorg(const ReorgTxns& reorg) disconnectpool.removeForBlock(reorg.connected_txns_2); // Sanity Check - assert(disconnectpool.queuedTx.size() == BLOCK_VTX_COUNT - reorg.num_shared); + assert(disconnectpool.size() == BLOCK_VTX_COUNT - reorg.num_shared); disconnectpool.clear(); } diff --git a/src/txmempool.h b/src/txmempool.h index 8b4ac2dce1d..44f79a9826f 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -850,29 +850,23 @@ public: * 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; + std::list queuedTx; + using TxList = decltype(queuedTx); + std::unordered_map iters_by_txid; -// multi_index tag names -struct txid_index {}; -struct insertion_order {}; - -struct DisconnectedBlockTransactions { - typedef boost::multi_index_container< - CTransactionRef, - boost::multi_index::indexed_by< - // sorted by txid - boost::multi_index::hashed_unique< - boost::multi_index::tag, - mempoolentry_txid, - SaltedTxidHasher - >, - // sorted by order in the blockchain - boost::multi_index::sequenced< - boost::multi_index::tag - > - > - > indexed_disconnected_transactions; - +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. @@ -881,18 +875,17 @@ struct DisconnectedBlockTransactions { // 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()); } - - indexed_disconnected_transactions queuedTx; - uint64_t cachedInnerUsage = 0; - - // Estimate the overhead of queuedTx to be 6 pointers + an allocation, as - // no exact formula for boost::multi_index_contained is implemented. - size_t DynamicMemoryUsage() const { - return memusage::MallocUsage(sizeof(CTransactionRef) + 6 * sizeof(void*)) * queuedTx.size() + cachedInnerUsage; + ~DisconnectedBlockTransactions() { + assert(queuedTx.empty()); + assert(iters_by_txid.empty()); + assert(cachedInnerUsage == 0); } - /** Add transactions from the block, iterating through vtx in reverse order. Callers should call + 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 @@ -900,40 +893,61 @@ struct DisconnectedBlockTransactions { */ void 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) { - queuedTx.insert(*block_it); - cachedInnerUsage += RecursiveDynamicUsage(*block_it); + auto it = queuedTx.insert(queuedTx.end(), *block_it); + iters_by_txid.emplace((*block_it)->GetHash(), it); + cachedInnerUsage += RecursiveDynamicUsage(**block_it); } } - // Remove entries based on txid_index, and update memory usage. + /** 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 (auto const &tx : vtx) { - auto it = queuedTx.find(tx->GetHash()); - if (it != queuedTx.end()) { - cachedInnerUsage -= RecursiveDynamicUsage(*it); - queuedTx.erase(it); + 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); } } } - // Remove an entry by insertion_order index, and update memory usage. - void removeEntry(indexed_disconnected_transactions::index::type::iterator entry) + /** Remove the first entry and update memory usage. */ + CTransactionRef take_first() { - cachedInnerUsage -= RecursiveDynamicUsage(*entry); - queuedTx.get().erase(entry); + 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() { cachedInnerUsage = 0; + iters_by_txid.clear(); queuedTx.clear(); } + + /** Clear all data structures and return the list of transactions. */ + std::list take() + { + std::list ret = std::move(queuedTx); + clear(); + return ret; + } }; #endif // BITCOIN_TXMEMPOOL_H diff --git a/src/validation.cpp b/src/validation.cpp index 9326dad10dd..b1561f4906b 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -295,28 +295,30 @@ void Chainstate::MaybeUpdateMempoolForReorg( AssertLockHeld(cs_main); AssertLockHeld(m_mempool->cs); std::vector vHashUpdate; - // disconnectpool's insertion_order index sorts the entries from - // oldest to newest, but the oldest entry will be the last tx from the - // latest mined block that was disconnected. - // 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.get().rbegin(); - while (it != disconnectpool.queuedTx.get().rend()) { - // ignore validation errors in resurrected transactions - if (!fAddToMempool || (*it)->IsCoinBase() || - AcceptToMemoryPool(*this, *it, GetTime(), - /*bypass_limits=*/true, /*test_accept=*/false).m_result_type != - MempoolAcceptResult::ResultType::VALID) { - // If the transaction doesn't make it in to the mempool, remove any - // transactions that depend on it (which would now be orphans). - m_mempool->removeRecursive(**it, MemPoolRemovalReason::REORG); - } else if (m_mempool->exists(GenTxid::Txid((*it)->GetHash()))) { - vHashUpdate.push_back((*it)->GetHash()); + { + // disconnectpool is ordered so that the front is the most recently-confirmed + // transaction (the last tx of the block at the tip) in the disconnected chain. + // 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. + const auto queuedTx = disconnectpool.take(); + auto it = queuedTx.rbegin(); + while (it != queuedTx.rend()) { + // ignore validation errors in resurrected transactions + if (!fAddToMempool || (*it)->IsCoinBase() || + AcceptToMemoryPool(*this, *it, GetTime(), + /*bypass_limits=*/true, /*test_accept=*/false).m_result_type != + MempoolAcceptResult::ResultType::VALID) { + // If the transaction doesn't make it in to the mempool, remove any + // transactions that depend on it (which would now be orphans). + m_mempool->removeRecursive(**it, MemPoolRemovalReason::REORG); + } else if (m_mempool->exists(GenTxid::Txid((*it)->GetHash()))) { + vHashUpdate.push_back((*it)->GetHash()); + } + ++it; } - ++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. @@ -2724,9 +2726,8 @@ bool Chainstate::DisconnectTip(BlockValidationState& state, DisconnectedBlockTra 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 it = disconnectpool->queuedTx.get().begin(); - m_mempool->removeRecursive(**it, MemPoolRemovalReason::REORG); - disconnectpool->removeEntry(it); + auto ptx = disconnectpool->take_first(); + m_mempool->removeRecursive(*ptx, MemPoolRemovalReason::REORG); } } diff --git a/src/validation.h b/src/validation.h index ba427afc64f..03b0d8c516d 100644 --- a/src/validation.h +++ b/src/validation.h @@ -51,7 +51,7 @@ class CBlockTreeDB; class CTxMemPool; class ChainstateManager; struct ChainTxData; -struct DisconnectedBlockTransactions; +class DisconnectedBlockTransactions; struct PrecomputedTransactionData; struct LockPoints; struct AssumeutxoData; From cf5f1faa037e9a40a5029cc7dd4ee61454b62466 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 5 Sep 2023 13:45:09 +0100 Subject: [PATCH 5/6] MOVEONLY: DisconnectedBlockTransactions to its own file This struct is only used in validation + tests and has very little to do with txmempool. --- src/Makefile.am | 1 + src/bench/disconnected_transactions.cpp | 3 +- src/kernel/disconnected_transactions.h | 128 ++++++++++++++++++ .../validation_chainstatemanager_tests.cpp | 1 + src/txmempool.h | 114 ---------------- src/validation.cpp | 1 + 6 files changed, 133 insertions(+), 115 deletions(-) create mode 100644 src/kernel/disconnected_transactions.h diff --git a/src/Makefile.am b/src/Makefile.am index feed4a0061c..fa063dac851 100644 --- a/src/Makefile.am +++ b/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 \ diff --git a/src/bench/disconnected_transactions.cpp b/src/bench/disconnected_transactions.cpp index 096165bd1af..72ae9d6c5c3 100644 --- a/src/bench/disconnected_transactions.cpp +++ b/src/bench/disconnected_transactions.cpp @@ -3,9 +3,10 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include +#include +#include #include #include -#include constexpr size_t BLOCK_VTX_COUNT{4000}; constexpr size_t BLOCK_VTX_COUNT_10PERCENT{400}; diff --git a/src/kernel/disconnected_transactions.h b/src/kernel/disconnected_transactions.h new file mode 100644 index 00000000000..a5d02c33ee8 --- /dev/null +++ b/src/kernel/disconnected_transactions.h @@ -0,0 +1,128 @@ +// 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 +#include +#include +#include + +#include +#include + +/** + * 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; + std::list queuedTx; + using TxList = decltype(queuedTx); + std::unordered_map 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. + // 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. + */ + void 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); + } + } + + /** 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); + } + } + } + + /** 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() + { + cachedInnerUsage = 0; + iters_by_txid.clear(); + queuedTx.clear(); + } + + /** Clear all data structures and return the list of transactions. */ + std::list take() + { + std::list ret = std::move(queuedTx); + clear(); + return ret; + } +}; +#endif // BITCOIN_KERNEL_DISCONNECTED_TRANSACTIONS_H diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index 9e359eeee41..a793f56cba6 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -4,6 +4,7 @@ // #include #include +#include #include #include #include diff --git a/src/txmempool.h b/src/txmempool.h index 44f79a9826f..ad4d9e71011 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -836,118 +836,4 @@ public: * m_temp_added and cannot be flushed to the back end. Only used for package validation. */ void PackageAddTransaction(const CTransactionRef& tx); }; - -/** - * 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; - std::list queuedTx; - using TxList = decltype(queuedTx); - std::unordered_map 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. - // 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. - */ - void 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); - } - } - - /** 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); - } - } - } - - /** 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() - { - cachedInnerUsage = 0; - iters_by_txid.clear(); - queuedTx.clear(); - } - - /** Clear all data structures and return the list of transactions. */ - std::list take() - { - std::list ret = std::move(queuedTx); - clear(); - return ret; - } -}; - #endif // BITCOIN_TXMEMPOOL_H diff --git a/src/validation.cpp b/src/validation.cpp index b1561f4906b..84846add807 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include From 4313c77400eb8eaa8586db39a7e29a861772ea80 Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 6 Sep 2023 16:25:15 +0100 Subject: [PATCH 6/6] make DisconnectedBlockTransactions responsible for its own memory management Co-authored-by: Cory Fields --- src/bench/disconnected_transactions.cpp | 5 ++- src/kernel/disconnected_transactions.h | 37 ++++++++++++------- .../validation_chainstatemanager_tests.cpp | 2 +- src/validation.cpp | 16 +++----- 4 files changed, 33 insertions(+), 27 deletions(-) diff --git a/src/bench/disconnected_transactions.cpp b/src/bench/disconnected_transactions.cpp index 72ae9d6c5c3..d6f15909505 100644 --- a/src/bench/disconnected_transactions.cpp +++ b/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); diff --git a/src/kernel/disconnected_transactions.h b/src/kernel/disconnected_transactions.h index a5d02c33ee8..7db39ba5cae 100644 --- a/src/kernel/disconnected_transactions.h +++ b/src/kernel/disconnected_transactions.h @@ -12,7 +12,10 @@ #include #include +#include +/** Maximum kilobytes for transactions to store for processing during reorg */ +static const unsigned int MAX_DISCONNECTED_TX_POOL_SIZE = 20'000; /** * DisconnectedBlockTransactions @@ -38,11 +41,28 @@ 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 queuedTx; using TxList = decltype(queuedTx); 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; + } + 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. @@ -66,8 +86,9 @@ public: * 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. */ - void AddTransactionsFromBlock(const std::vector& vtx) + [[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) { @@ -75,6 +96,7 @@ public: iters_by_txid.emplace((*block_it)->GetHash(), it); cachedInnerUsage += RecursiveDynamicUsage(**block_it); } + return LimitMemoryUsage(); } /** Remove any entries that are in this block. */ @@ -95,19 +117,6 @@ public: } } - /** 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() diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index a793f56cba6..7b7be4be9ee 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/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()); diff --git a/src/validation.cpp b/src/validation.cpp index 84846add807..a9a0912d2a4 100644 --- a/src/validation.cpp +++ b/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. */ @@ -2723,12 +2721,10 @@ 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(); - m_mempool->removeRecursive(*ptx, MemPoolRemovalReason::REORG); + // Save transactions to re-add to mempool at end of reorg. If any entries are evicted for + // exceeding memory limits, remove them and their descendants from the mempool. + for (auto&& evicted_tx : disconnectpool->AddTransactionsFromBlock(block.vtx)) { + m_mempool->removeRecursive(*evicted_tx, MemPoolRemovalReason::REORG); } } @@ -2978,7 +2974,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, @@ -3312,7 +3308,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