From d05481df644cc958cb309f20758bec996b8cfcfa Mon Sep 17 00:00:00 2001 From: stickies-v Date: Wed, 23 Apr 2025 15:35:02 +0100 Subject: [PATCH 1/3] refactor: validation: mark SnapshotBase as const It does not modify any logical state. This change is required for future const-correctness commits. --- src/validation.cpp | 2 +- src/validation.h | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 1213d8be9f9..cd53727d250 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1970,7 +1970,7 @@ Chainstate::Chainstate( m_chainman(chainman), m_from_snapshot_blockhash(from_snapshot_blockhash) {} -const CBlockIndex* Chainstate::SnapshotBase() +const CBlockIndex* Chainstate::SnapshotBase() const { if (!m_from_snapshot_blockhash) return nullptr; if (!m_cached_snapshot_base) m_cached_snapshot_base = Assert(m_chainman.m_blockman.LookupBlockIndex(*m_from_snapshot_blockhash)); diff --git a/src/validation.h b/src/validation.h index e361c7af101..1951120d15c 100644 --- a/src/validation.h +++ b/src/validation.h @@ -532,7 +532,7 @@ protected: bool m_disabled GUARDED_BY(::cs_main) {false}; //! Cached result of LookupBlockIndex(*m_from_snapshot_blockhash) - const CBlockIndex* m_cached_snapshot_base GUARDED_BY(::cs_main) {nullptr}; + mutable const CBlockIndex* m_cached_snapshot_base GUARDED_BY(::cs_main){nullptr}; public: //! Reference to a BlockManager instance which itself is shared across all @@ -596,7 +596,7 @@ public: * * nullptr if this chainstate was not created from a snapshot. */ - const CBlockIndex* SnapshotBase() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + const CBlockIndex* SnapshotBase() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main); /** * The set of all CBlockIndex entries that have as much work as our current From 61a51eccbba1e7ccbdaac2bb7c74503bcf6fc9a5 Mon Sep 17 00:00:00 2001 From: stickies-v Date: Tue, 6 May 2025 14:24:56 +0100 Subject: [PATCH 2/3] validation: don't use GetAll() in CheckBlockIndex() GetAll() is non-const, preventing CheckBlockIndex() from being const. Rather than add a const GetAll() method, just iterate over the chainstates directly. Slight behaviour change by also subjecting non-`IsUsable()` chainstates to consistency checks. --- src/validation.cpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index cd53727d250..a9bde737401 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5354,8 +5354,8 @@ void ChainstateManager::CheckBlockIndex() if (pindex->pprev == nullptr) { // Genesis block checks. assert(pindex->GetBlockHash() == GetConsensus().hashGenesisBlock); // Genesis block's hash must match. - for (auto c : GetAll()) { - if (c->m_chain.Genesis() != nullptr) { + for (const Chainstate* c : {m_ibd_chainstate.get(), m_snapshot_chainstate.get()}) { + if (c && c->m_chain.Genesis() != nullptr) { assert(pindex == c->m_chain.Genesis()); // The chain's genesis block must be this block. } } @@ -5408,8 +5408,8 @@ void ChainstateManager::CheckBlockIndex() } // Chainstate-specific checks on setBlockIndexCandidates - for (auto c : GetAll()) { - if (c->m_chain.Tip() == nullptr) continue; + for (const Chainstate* c : {m_ibd_chainstate.get(), m_snapshot_chainstate.get()}) { + if (!c || c->m_chain.Tip() == nullptr) continue; // Two main factors determine whether pindex is a candidate in // setBlockIndexCandidates: // @@ -5492,7 +5492,8 @@ void ChainstateManager::CheckBlockIndex() // tip. // So if this block is itself better than any m_chain.Tip() and it wasn't in // setBlockIndexCandidates, then it must be in m_blocks_unlinked. - for (auto c : GetAll()) { + for (const Chainstate* c : {m_ibd_chainstate.get(), m_snapshot_chainstate.get()}) { + if (!c) continue; const bool is_active = c == &ActiveChainstate(); if (!CBlockIndexWorkComparator()(pindex, c->m_chain.Tip()) && c->setBlockIndexCandidates.count(pindex) == 0) { if (pindexFirstInvalid == nullptr) { From 3e6ac5bf772751c66cdcd015dcb7e6ce4ea2cc77 Mon Sep 17 00:00:00 2001 From: stickies-v Date: Wed, 23 Apr 2025 15:52:56 +0100 Subject: [PATCH 3/3] refactor: validation: mark CheckBlockIndex as const As a check/test method, this function should not mutate logical state. Mark it as const to better help ensure this. --- src/validation.cpp | 34 +++++++++++++++++----------------- src/validation.h | 2 +- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index a9bde737401..4c5f7a8a503 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5257,7 +5257,7 @@ bool ChainstateManager::ShouldCheckBlockIndex() const return true; } -void ChainstateManager::CheckBlockIndex() +void ChainstateManager::CheckBlockIndex() const { if (!ShouldCheckBlockIndex()) { return; @@ -5282,7 +5282,7 @@ void ChainstateManager::CheckBlockIndex() assert(m_best_header); best_hdr_chain.SetTip(*m_best_header); - std::multimap forward; + std::multimap forward; for (auto& [_, block_index] : m_blockman.m_block_index) { // Only save indexes in forward that are not part of the best header chain. if (!best_hdr_chain.Contains(&block_index)) { @@ -5293,27 +5293,27 @@ void ChainstateManager::CheckBlockIndex() } assert(forward.size() + best_hdr_chain.Height() + 1 == m_blockman.m_block_index.size()); - CBlockIndex* pindex = best_hdr_chain[0]; + const 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. size_t nNodes = 0; int nHeight = 0; - CBlockIndex* pindexFirstInvalid = nullptr; // Oldest ancestor of pindex which is invalid. - CBlockIndex* pindexFirstMissing = nullptr; // Oldest ancestor of pindex which does not have BLOCK_HAVE_DATA, since assumeutxo snapshot if used. - CBlockIndex* pindexFirstNeverProcessed = nullptr; // Oldest ancestor of pindex for which nTx == 0, since assumeutxo snapshot if used. - CBlockIndex* pindexFirstNotTreeValid = nullptr; // Oldest ancestor of pindex which does not have BLOCK_VALID_TREE (regardless of being valid or not). - CBlockIndex* pindexFirstNotTransactionsValid = nullptr; // Oldest ancestor of pindex which does not have BLOCK_VALID_TRANSACTIONS (regardless of being valid or not), since assumeutxo snapshot if used. - CBlockIndex* pindexFirstNotChainValid = nullptr; // Oldest ancestor of pindex which does not have BLOCK_VALID_CHAIN (regardless of being valid or not), since assumeutxo snapshot if used. - CBlockIndex* pindexFirstNotScriptsValid = nullptr; // Oldest ancestor of pindex which does not have BLOCK_VALID_SCRIPTS (regardless of being valid or not), since assumeutxo snapshot if used. + const CBlockIndex* pindexFirstInvalid = nullptr; // Oldest ancestor of pindex which is invalid. + const CBlockIndex* pindexFirstMissing = nullptr; // Oldest ancestor of pindex which does not have BLOCK_HAVE_DATA, since assumeutxo snapshot if used. + const CBlockIndex* pindexFirstNeverProcessed = nullptr; // Oldest ancestor of pindex for which nTx == 0, since assumeutxo snapshot if used. + const CBlockIndex* pindexFirstNotTreeValid = nullptr; // Oldest ancestor of pindex which does not have BLOCK_VALID_TREE (regardless of being valid or not). + const CBlockIndex* pindexFirstNotTransactionsValid = nullptr; // Oldest ancestor of pindex which does not have BLOCK_VALID_TRANSACTIONS (regardless of being valid or not), since assumeutxo snapshot if used. + const CBlockIndex* pindexFirstNotChainValid = nullptr; // Oldest ancestor of pindex which does not have BLOCK_VALID_CHAIN (regardless of being valid or not), since assumeutxo snapshot if used. + const CBlockIndex* pindexFirstNotScriptsValid = nullptr; // Oldest ancestor of pindex which does not have BLOCK_VALID_SCRIPTS (regardless of being valid or not), since assumeutxo snapshot if used. // After checking an assumeutxo snapshot block, reset pindexFirst pointers // to earlier blocks that have not been downloaded or validated yet, so // checks for later blocks can assume the earlier blocks were validated and // be stricter, testing for more requirements. const CBlockIndex* snap_base{GetSnapshotBaseBlock()}; - CBlockIndex *snap_first_missing{}, *snap_first_notx{}, *snap_first_notv{}, *snap_first_nocv{}, *snap_first_nosv{}; + const CBlockIndex *snap_first_missing{}, *snap_first_notx{}, *snap_first_notv{}, *snap_first_nocv{}, *snap_first_nosv{}; auto snap_update_firsts = [&] { if (pindex == snap_base) { std::swap(snap_first_missing, pindexFirstMissing); @@ -5453,7 +5453,7 @@ void ChainstateManager::CheckBlockIndex() // pindex only needs to be added if it is an ancestor of // the snapshot that is being validated. if (c == &ActiveChainstate() || snap_base->GetAncestor(pindex->nHeight) == pindex) { - assert(c->setBlockIndexCandidates.count(pindex)); + assert(c->setBlockIndexCandidates.contains(const_cast(pindex))); } } // If some parent is missing, then it could be that this block was in @@ -5461,11 +5461,11 @@ void ChainstateManager::CheckBlockIndex() // In this case it must be in m_blocks_unlinked -- see test below. } } else { // If this block sorts worse than the current tip or some ancestor's block has never been seen, it cannot be in setBlockIndexCandidates. - assert(c->setBlockIndexCandidates.count(pindex) == 0); + assert(!c->setBlockIndexCandidates.contains(const_cast(pindex))); } } // Check whether this block is in m_blocks_unlinked. - std::pair::iterator,std::multimap::iterator> rangeUnlinked = m_blockman.m_blocks_unlinked.equal_range(pindex->pprev); + auto rangeUnlinked{m_blockman.m_blocks_unlinked.equal_range(pindex->pprev)}; bool foundInUnlinked = false; while (rangeUnlinked.first != rangeUnlinked.second) { assert(rangeUnlinked.first->first == pindex->pprev); @@ -5495,7 +5495,7 @@ void ChainstateManager::CheckBlockIndex() for (const Chainstate* c : {m_ibd_chainstate.get(), m_snapshot_chainstate.get()}) { if (!c) continue; const bool is_active = c == &ActiveChainstate(); - if (!CBlockIndexWorkComparator()(pindex, c->m_chain.Tip()) && c->setBlockIndexCandidates.count(pindex) == 0) { + if (!CBlockIndexWorkComparator()(pindex, c->m_chain.Tip()) && !c->setBlockIndexCandidates.contains(const_cast(pindex))) { if (pindexFirstInvalid == nullptr) { if (is_active || snap_base->GetAncestor(pindex->nHeight) == pindex) { assert(foundInUnlinked); @@ -5510,7 +5510,7 @@ void ChainstateManager::CheckBlockIndex() // 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); + auto range{forward.equal_range(pindex)}; if (range.first != range.second) { // A subnode not part of the best header chain was found. pindex = range.first->second; @@ -5539,7 +5539,7 @@ void ChainstateManager::CheckBlockIndex() // Find our parent. CBlockIndex* pindexPar = pindex->pprev; // Find which child we just visited. - std::pair::iterator,std::multimap::iterator> rangePar = forward.equal_range(pindexPar); + auto rangePar{forward.equal_range(pindexPar)}; while (rangePar.first->second != pindex) { assert(rangePar.first != rangePar.second); // Our parent must have at least the node we're coming from as child. rangePar.first++; diff --git a/src/validation.h b/src/validation.h index 1951120d15c..d1c7a811d44 100644 --- a/src/validation.h +++ b/src/validation.h @@ -985,7 +985,7 @@ public: * * By default this only executes fully when using the Regtest chain; see: m_options.check_block_index. */ - void CheckBlockIndex(); + void CheckBlockIndex() const; /** * Alias for ::cs_main.