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 9a79ee285f21fc6c12d5a37487916b2badb3a75c Mon Sep 17 00:00:00 2001 From: stickies-v Date: Wed, 23 Apr 2025 15:44:09 +0100 Subject: [PATCH 2/3] refactor: validation: add const GetAll method A read-only accessor into the Chainstates is required for a future commit to make CheckBlockIndex const. --- src/validation.cpp | 12 ++++++++++++ src/validation.h | 1 + 2 files changed, 13 insertions(+) diff --git a/src/validation.cpp b/src/validation.cpp index cd53727d250..fedfafdd065 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5657,6 +5657,18 @@ std::vector ChainstateManager::GetAll() return out; } +std::vector ChainstateManager::GetAll() const +{ + LOCK(::cs_main); + std::vector out; + + for (const Chainstate* cs : {m_ibd_chainstate.get(), m_snapshot_chainstate.get()}) { + if (this->IsUsable(cs)) out.push_back(cs); + } + + return out; +} + Chainstate& ChainstateManager::InitializeChainstate(CTxMemPool* mempool) { AssertLockHeld(::cs_main); diff --git a/src/validation.h b/src/validation.h index 1951120d15c..b3eb20b4870 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1078,6 +1078,7 @@ public: //! Get all chainstates currently being used. std::vector GetAll(); + std::vector GetAll() const; //! Construct and activate a Chainstate on the basis of UTXO snapshot data. //! From 12645d1deec9b2418e5369eafccb234fd80b3272 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 fedfafdd065..45582c6f563 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); @@ -5494,7 +5494,7 @@ void ChainstateManager::CheckBlockIndex() // setBlockIndexCandidates, then it must be in m_blocks_unlinked. for (auto c : GetAll()) { 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); @@ -5509,7 +5509,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; @@ -5538,7 +5538,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 b3eb20b4870..22de8f40b0d 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.