ChainstateManager::CheckBlockIndex() is not a const method. To try and assert that this method was not causing any side-effects, I modified the method to make it const. It did not surface any errors, but I think it would be good to merge this change regardless, even if CheckBlockIndex is only used in regtest.
It does not modify any logical state. This change is required
for future const-correctness commits.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32364.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | shahsb |
| Concept ACK | mzumsande, TheCharlatan |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Concept ACK
Don’t really love duplicating GetAll in the second commit, but if there is no better solution, marking CBI as const seems more important.
Concept ACK
I wrote this simpler approach here https://github.com/TheCharlatan/bitcoin/commit/fe13fa082e3d6befa6d5332428584570ead608ab after seeing you mentioning it last week. I think making sure that all block indexes are declared const in the method might be beneficial though. Maybe instead of adding a separate GetAll method just make it const and then declare the chainstate pointers as const in the for loop?
A read-only accessor into the Chainstates is required for a future
commit to make CheckBlockIndex const.
As a check/test method, this function should not mutate logical
state. Mark it as const to better help ensure this.
Force-pushed to deduplicate the GetAll() overloads by adding a GetAllImpl() templated helper, addressing @mzumsande’s comment.
I wrote this simpler approach here TheCharlatan@fe13fa0 after seeing you mentioning it last week. I think making sure that all block indexes are declared const in the method might be beneficial though. Maybe instead of adding a separate
GetAllmethod just make it const and then declare the chainstate pointers as const in the for loop?
Allowing a const ChainstateManager to modify its Chainstate logical state feels like a const-correctness violation to me. If you agree that’s true, then I don’t think the shorter diff is a worthwhile trade-off. Furthermore, there are other places (such as GetPruneRange()) that would benefit from a GetAll() const, so I think this is worth adding?
963@@ -964,6 +964,13 @@ class ChainstateManager
964 SteadyClock::duration GUARDED_BY(::cs_main) time_chainstate{};
965 SteadyClock::duration GUARDED_BY(::cs_main) time_post_connect{};
966
967+ //! Internal helper for GetAll() overloads.
968+ //! @see GetAll() const
969+ //! @see GetAll()
970+ template <typename T>
971+ auto GetAllImpl(T* self) const
Super nitty, I was just curious about what the possible ways of doing this are, and maybe this is preferable:
Making GetAllImpl a static member, so it doesn’t need to be passed both a self and a this pointer:
0diff --git a/src/validation.cpp b/src/validation.cpp
1index 8fb8a876b3f..58bcd9e84e6 100644
2--- a/src/validation.cpp
3+++ b/src/validation.cpp
4@@ -5646,14 +5646,14 @@ std::optional<uint256> ChainstateManager::SnapshotBlockhash() const
5 }
6
7 template <typename T>
8-auto ChainstateManager::GetAllImpl(T* self) const
9+auto ChainstateManager::GetAllImpl(T* self)
10 -> std::vector<std::conditional_t<std::is_const_v<T>, const Chainstate*, Chainstate*>>
11 {
12 LOCK(::cs_main);
13 using ChainstatePtr = std::conditional_t<std::is_const_v<T>, const Chainstate*, Chainstate*>;
14 std::vector<ChainstatePtr> out;
15
16- for (ChainstatePtr cs : {m_ibd_chainstate.get(), m_snapshot_chainstate.get()}) {
17+ for (ChainstatePtr cs : {self->m_ibd_chainstate.get(), self->m_snapshot_chainstate.get()}) {
18 if (self->IsUsable(cs)) out.push_back(cs);
19 }
20
21diff --git a/src/validation.h b/src/validation.h
22index 40420e11223..3ccb25dc29f 100644
23--- a/src/validation.h
24+++ b/src/validation.h
25@@ -968,7 +968,7 @@ private:
26 //! [@see](/bitcoin-bitcoin/contributor/see/) GetAll() const
27 //! [@see](/bitcoin-bitcoin/contributor/see/) GetAll()
28 template <typename T>
29- auto GetAllImpl(T* self) const
30+ static auto GetAllImpl(T* self)
31 -> std::vector<std::conditional_t<std::is_const_v<T>, const Chainstate*, Chainstate*>>;
32
33 public:
Given that #30214 wants to removes GetAll() anyway in 64fc6603525f515c5303f557b038753904c6541(FYI @ryanofsky), the simplest solution might be to not use GetAll and just iterate over m_ibd_chainstate.get() and m_snapshot_chainstate.get() by hand:
0for (const Chainstate* c : {m_ibd_chainstate.get(), m_snapshot_chainstate.get()}) {
1 if (!c) continue;
2 (...)
this would have the added benefit that we could replace the IsUsable(cs) by just a not-nullptr check for cs, because I don’t see a conceptual reason why a disabled chainstate (whether it’s invalid, or fully validated) shouldn’t be subject to CheckBlockIndex() anymore.