diff --git a/src/Makefile.am b/src/Makefile.am index e542a067a47..8905c0ad1cd 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/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..d6f15909505 --- /dev/null +++ b/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 +#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{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); diff --git a/src/kernel/disconnected_transactions.h b/src/kernel/disconnected_transactions.h new file mode 100644 index 00000000000..7db39ba5cae --- /dev/null +++ b/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 +#include +#include +#include + +#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 + + * 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 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. + // 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 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(); + } + + /** 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); + } + } + } + + 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/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 { diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index 9e359eeee41..7b7be4be9ee 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 @@ -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()); diff --git a/src/txmempool.h b/src/txmempool.h index 8ea3373cb47..cbeabb31fa6 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -848,96 +848,4 @@ class CCoinsViewMemPool : public CCoinsViewBacked /** Clear m_temp_added and m_non_base_coins. */ void Reset(); }; - -/** - * 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. - */ - -// 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; - - // 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()); } - - 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; - } - - void addTransaction(const CTransactionRef& tx) - { - queuedTx.insert(tx); - cachedInnerUsage += RecursiveDynamicUsage(tx); - } - - // Remove entries based on txid_index, and update memory usage. - 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); - } - } - } - - // Remove an entry by insertion_order index, and update memory usage. - void removeEntry(indexed_disconnected_transactions::index::type::iterator entry) - { - cachedInnerUsage -= RecursiveDynamicUsage(*entry); - queuedTx.get().erase(entry); - } - - void clear() - { - cachedInnerUsage = 0; - queuedTx.clear(); - } -}; - #endif // BITCOIN_TXMEMPOOL_H diff --git a/src/validation.cpp b/src/validation.cpp index db3310c2c36..0ce29021959 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -81,8 +82,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. */ @@ -297,28 +296,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()); - } - ++it; - } - disconnectpool.queuedTx.clear(); + { + // 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; + } + } + // 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. @@ -2795,15 +2796,10 @@ 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); - } - 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); + // 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); } } @@ -3053,7 +3049,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, @@ -3387,7 +3383,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 diff --git a/src/validation.h b/src/validation.h index f1ff6bb6719..6640bb5b50a 100644 --- a/src/validation.h +++ b/src/validation.h @@ -50,7 +50,7 @@ class Chainstate; class CTxMemPool; class ChainstateManager; struct ChainTxData; -struct DisconnectedBlockTransactions; +class DisconnectedBlockTransactions; struct PrecomputedTransactionData; struct LockPoints; struct AssumeutxoData;