From 32c80413fdb063199f3bee719c4651bd63f05fce Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Wed, 20 Dec 2023 15:41:05 -0500 Subject: [PATCH 1/3] bench: add benchmark for checkblockindex --- src/Makefile.bench.include | 1 + src/bench/checkblockindex.cpp | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+) create mode 100644 src/bench/checkblockindex.cpp diff --git a/src/Makefile.bench.include b/src/Makefile.bench.include index 7ba0111fa6..2ba72c9e76 100644 --- a/src/Makefile.bench.include +++ b/src/Makefile.bench.include @@ -23,6 +23,7 @@ bench_bench_bitcoin_SOURCES = \ bench/ccoins_caching.cpp \ bench/chacha20.cpp \ bench/checkblock.cpp \ + bench/checkblockindex.cpp \ bench/checkqueue.cpp \ bench/crypto_hash.cpp \ bench/data.cpp \ diff --git a/src/bench/checkblockindex.cpp b/src/bench/checkblockindex.cpp new file mode 100644 index 0000000000..e8a848dbd4 --- /dev/null +++ b/src/bench/checkblockindex.cpp @@ -0,0 +1,20 @@ +// Copyright (c) 2023-present 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 + +static void CheckBlockIndex(benchmark::Bench& bench) +{ + auto testing_setup{MakeNoLogFileContext()}; + // Mine some more blocks + testing_setup->mineBlocks(1000); + bench.run([&] { + testing_setup->m_node.chainman->CheckBlockIndex(); + }); +} + + +BENCHMARK(CheckBlockIndex, benchmark::PriorityLevel::HIGH); From d5a631b9597e5029a5048d9b8ad84ea4536bbac0 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Wed, 23 Aug 2023 14:05:42 -0400 Subject: [PATCH 2/3] validation: improve performance of CheckBlockIndex by not saving all indexes in a std::multimap, but only those that are not part of the best header chain. The indexes of the best header chain are stored in a vector, which, in the typical case of a mostly linear chain with a few forks, results in a much smaller multimap, and increases performance noticeably for long chains. This does not change the actual consistency checks that are being performed for each index, just the way the block index tree is stored and traversed. Co-authored-by: Ryan Ofsky --- src/validation.cpp | 50 +++++++++++++++++++++++++++++++++------------- 1 file changed, 36 insertions(+), 14 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index b6d0c38f39..9dad39e657 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5050,19 +5050,28 @@ void ChainstateManager::CheckBlockIndex() return; } - // Build forward-pointing map of the entire block tree. + // Build forward-pointing data structure for the entire block tree. + // For performance reasons, indexes of the best header chain are stored in a vector (within CChain). + // All remaining blocks are stored in a multimap. + // The best header chain can differ from the active chain: E.g. its entries may belong to blocks that + // are not yet validated. + CChain best_hdr_chain; + assert(m_best_header); + best_hdr_chain.SetTip(*m_best_header); + std::multimap forward; for (auto& [_, block_index] : m_blockman.m_block_index) { - forward.emplace(block_index.pprev, &block_index); + // Only save indexes in forward that are not part of the best header chain. + if (!best_hdr_chain.Contains(&block_index)) { + // Only genesis, which must be part of the best header chain, can have a nullptr parent. + assert(block_index.pprev); + forward.emplace(block_index.pprev, &block_index); + } } + assert(forward.size() + best_hdr_chain.Height() + 1 == m_blockman.m_block_index.size()); - assert(forward.size() == m_blockman.m_block_index.size()); - - std::pair::iterator,std::multimap::iterator> rangeGenesis = forward.equal_range(nullptr); - CBlockIndex *pindex = rangeGenesis.first->second; - rangeGenesis.first++; - assert(rangeGenesis.first == rangeGenesis.second); // There is only one index entry with parent nullptr. - + CBlockIndex* pindex = best_hdr_chain[0]; + assert(pindex); // Iterate over the entire block tree, using depth-first search. // Along the way, remember whether there are blocks on the path from genesis // block being explored which are the first to have certain properties. @@ -5274,14 +5283,21 @@ void ChainstateManager::CheckBlockIndex() // assert(pindex->GetBlockHash() == pindex->GetBlockHeader().GetHash()); // Perhaps too slow // End: actual consistency checks. - // Try descending into the first subnode. + + // Try descending into the first subnode. Always process forks first and the best header chain after. snap_update_firsts(); std::pair::iterator,std::multimap::iterator> range = forward.equal_range(pindex); if (range.first != range.second) { - // A subnode was found. + // A subnode not part of the best header chain was found. pindex = range.first->second; nHeight++; continue; + } else if (best_hdr_chain.Contains(pindex)) { + // Descend further into best header chain. + nHeight++; + pindex = best_hdr_chain[nHeight]; + if (!pindex) break; // we are finished, since the best header chain is always processed last + continue; } // This is a leaf node. // Move upwards until we reach a node of which we have not yet visited the last child. @@ -5307,9 +5323,15 @@ void ChainstateManager::CheckBlockIndex() // Proceed to the next one. rangePar.first++; if (rangePar.first != rangePar.second) { - // Move to the sibling. + // Move to a sibling not part of the best header chain. pindex = rangePar.first->second; break; + } else if (pindexPar == best_hdr_chain[nHeight - 1]) { + // Move to pindex's sibling on the best-chain, if it has one. + pindex = best_hdr_chain[nHeight]; + // There will not be a next block if (and only if) parent block is the best header. + assert((pindex == nullptr) == (pindexPar == best_hdr_chain.Tip())); + break; } else { // Move up further. pindex = pindexPar; @@ -5319,8 +5341,8 @@ void ChainstateManager::CheckBlockIndex() } } - // Check that we actually traversed the entire map. - assert(nNodes == forward.size()); + // Check that we actually traversed the entire block index. + assert(nNodes == forward.size() + best_hdr_chain.Height() + 1); } std::string Chainstate::ToString() From 5bc2077e8f592442b089affdf0b5795fbc053bb8 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Thu, 21 Dec 2023 17:24:07 -0500 Subject: [PATCH 3/3] validation: allow to specify frequency for -checkblockindex This makes it similar to -checkaddrman and -checkmempool, which also allow to run the check occasionally instead of always / never. Co-authored-by: Ryan Ofsky --- src/init.cpp | 2 +- src/kernel/chainstatemanager_opts.h | 2 +- src/node/chainstatemanager_args.cpp | 5 ++++- src/test/util/setup_common.cpp | 2 +- src/validation.cpp | 8 ++++++++ src/validation.h | 2 +- 6 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 0aa04755cb..927786e3d5 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -604,7 +604,7 @@ void SetupServerArgs(ArgsManager& argsman) argsman.AddArg("-checkblocks=", strprintf("How many blocks to check at startup (default: %u, 0 = all)", DEFAULT_CHECKBLOCKS), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-checklevel=", strprintf("How thorough the block verification of -checkblocks is: %s (0-4, default: %u)", Join(CHECKLEVEL_DOC, ", "), DEFAULT_CHECKLEVEL), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); - argsman.AddArg("-checkblockindex", strprintf("Do a consistency check for the block tree, chainstate, and other validation data structures occasionally. (default: %u, regtest: %u)", defaultChainParams->DefaultConsistencyChecks(), regtestChainParams->DefaultConsistencyChecks()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); + argsman.AddArg("-checkblockindex", strprintf("Do a consistency check for the block tree, chainstate, and other validation data structures every operations. Use 0 to disable. (default: %u, regtest: %u)", defaultChainParams->DefaultConsistencyChecks(), regtestChainParams->DefaultConsistencyChecks()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-checkaddrman=", strprintf("Run addrman consistency checks every operations. Use 0 to disable. (default: %u)", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-checkmempool=", strprintf("Run mempool consistency checks every transactions. Use 0 to disable. (default: %u, regtest: %u)", defaultChainParams->DefaultConsistencyChecks(), regtestChainParams->DefaultConsistencyChecks()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-checkpoints", strprintf("Enable rejection of any forks from the known historical chain until block %s (default: %u)", defaultChainParams->Checkpoints().GetHeight(), DEFAULT_CHECKPOINTS_ENABLED), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST); diff --git a/src/kernel/chainstatemanager_opts.h b/src/kernel/chainstatemanager_opts.h index de5f78494a..076841c3c9 100644 --- a/src/kernel/chainstatemanager_opts.h +++ b/src/kernel/chainstatemanager_opts.h @@ -33,7 +33,7 @@ namespace kernel { struct ChainstateManagerOpts { const CChainParams& chainparams; fs::path datadir; - std::optional check_block_index{}; + std::optional check_block_index{}; bool checkpoints_enabled{DEFAULT_CHECKPOINTS_ENABLED}; //! If set, it will override the minimum work we will assume exists on some valid chain. std::optional minimum_chain_work{}; diff --git a/src/node/chainstatemanager_args.cpp b/src/node/chainstatemanager_args.cpp index 1cc126cb05..bc4a815a3e 100644 --- a/src/node/chainstatemanager_args.cpp +++ b/src/node/chainstatemanager_args.cpp @@ -24,7 +24,10 @@ namespace node { util::Result ApplyArgsManOptions(const ArgsManager& args, ChainstateManager::Options& opts) { - if (auto value{args.GetBoolArg("-checkblockindex")}) opts.check_block_index = *value; + if (auto value{args.GetIntArg("-checkblockindex")}) { + // Interpret bare -checkblockindex argument as 1 instead of 0. + opts.check_block_index = args.GetArg("-checkblockindex")->empty() ? 1 : *value; + } if (auto value{args.GetBoolArg("-checkpoints")}) opts.checkpoints_enabled = *value; diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 2c18184261..666a7a0303 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -238,7 +238,7 @@ ChainTestingSetup::ChainTestingSetup(const ChainType chainType, const std::vecto const ChainstateManager::Options chainman_opts{ .chainparams = chainparams, .datadir = m_args.GetDataDirNet(), - .check_block_index = true, + .check_block_index = 1, .notifications = *m_node.notifications, .signals = m_node.validation_signals.get(), .worker_threads_num = 2, diff --git a/src/validation.cpp b/src/validation.cpp index 9dad39e657..a547614fc1 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5034,6 +5034,14 @@ void ChainstateManager::LoadExternalBlockFile( LogPrintf("Loaded %i blocks from external file in %dms\n", nLoaded, Ticks(SteadyClock::now() - start)); } +bool ChainstateManager::ShouldCheckBlockIndex() const +{ + // Assert to verify Flatten() has been called. + if (!*Assert(m_options.check_block_index)) return false; + if (GetRand(*m_options.check_block_index) >= 1) return false; + return true; +} + void ChainstateManager::CheckBlockIndex() { if (!ShouldCheckBlockIndex()) { diff --git a/src/validation.h b/src/validation.h index bcf153719a..15a1cbdc8d 100644 --- a/src/validation.h +++ b/src/validation.h @@ -933,7 +933,7 @@ public: const CChainParams& GetParams() const { return m_options.chainparams; } const Consensus::Params& GetConsensus() const { return m_options.chainparams.GetConsensus(); } - bool ShouldCheckBlockIndex() const { return *Assert(m_options.check_block_index); } + bool ShouldCheckBlockIndex() const; const arith_uint256& MinimumChainWork() const { return *Assert(m_options.minimum_chain_work); } const uint256& AssumedValidBlock() const { return *Assert(m_options.assumed_valid_block); } kernel::Notifications& GetNotifications() const { return m_options.notifications; };