refactor: validation: mark CheckBlockIndex as const #32364

pull stickies-v wants to merge 3 commits into bitcoin:master from stickies-v:2025-04/const-checkblockindex changing 2 files +27 −26
  1. stickies-v commented at 2:51 PM on April 28, 2025: contributor

    While reviewing another PR, I noticed that 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.

    This PR removes CheckBlockIndex()'s calls to non-const ChainstateManager methods by marking SnapshotBase const and ~inlining the GetAll() calls (thereby also performing consistency checks on invalid or fully validated m_disabled==true chainstates, as slight behaviour change), and finally marks CheckBlockIndex() as const.

  2. refactor: validation: mark SnapshotBase as const
    It does not modify any logical state. This change is required
    for future const-correctness commits.
    d05481df64
  3. DrahtBot commented at 2:51 PM on April 28, 2025: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32364.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK mzumsande, TheCharlatan, achow101
    Stale ACK shahsb

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  4. DrahtBot renamed this:
    refactor: validation: mark CheckBlockIndex as const
    refactor: validation: mark CheckBlockIndex as const
    on Apr 28, 2025
  5. DrahtBot added the label Refactoring on Apr 28, 2025
  6. mzumsande commented at 5:58 PM on April 29, 2025: contributor

    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.

  7. sedited commented at 8:54 PM on April 29, 2025: contributor

    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?

  8. stickies-v force-pushed on Apr 30, 2025
  9. stickies-v commented at 11:25 AM on April 30, 2025: contributor

    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 GetAll method 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?

  10. shahsb commented at 7:48 AM on May 1, 2025: none

    Concept ACK

  11. DrahtBot requested review from mzumsande on May 1, 2025
  12. DrahtBot requested review from sedited on May 1, 2025
  13. in src/validation.h:971 in 68b75849e2 outdated
     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
    


    sipa commented at 2:22 PM on May 1, 2025:

    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:

    diff --git a/src/validation.cpp b/src/validation.cpp
    index 8fb8a876b3f..58bcd9e84e6 100644
    --- a/src/validation.cpp
    +++ b/src/validation.cpp
    @@ -5646,14 +5646,14 @@ std::optional<uint256> ChainstateManager::SnapshotBlockhash() const
     }
     
     template <typename T>
    -auto ChainstateManager::GetAllImpl(T* self) const
    +auto ChainstateManager::GetAllImpl(T* self)
         -> std::vector<std::conditional_t<std::is_const_v<T>, const Chainstate*, Chainstate*>>
     {
         LOCK(::cs_main);
         using ChainstatePtr = std::conditional_t<std::is_const_v<T>, const Chainstate*, Chainstate*>;
         std::vector<ChainstatePtr> out;
     
    -    for (ChainstatePtr cs : {m_ibd_chainstate.get(), m_snapshot_chainstate.get()}) {
    +    for (ChainstatePtr cs : {self->m_ibd_chainstate.get(), self->m_snapshot_chainstate.get()}) {
             if (self->IsUsable(cs)) out.push_back(cs);
         }
     
    diff --git a/src/validation.h b/src/validation.h
    index 40420e11223..3ccb25dc29f 100644
    --- a/src/validation.h
    +++ b/src/validation.h
    @@ -968,7 +968,7 @@ private:
         //! [@see](/github-metadata-backup-bitcoin-bitcoin/contributor/see/) GetAll() const
         //! [@see](/github-metadata-backup-bitcoin-bitcoin/contributor/see/) GetAll()
         template <typename T>
    -    auto GetAllImpl(T* self) const
    +    static auto GetAllImpl(T* self)
             -> std::vector<std::conditional_t<std::is_const_v<T>, const Chainstate*, Chainstate*>>;
     
     public:
    

    stickies-v commented at 1:50 PM on May 6, 2025:

    Oh yes, keeping only self is a better approach, thanks! Marking as resolved because the commit has been dropped.

  14. mzumsande commented at 3:39 PM on May 1, 2025: contributor

    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:

    for (const Chainstate* c : {m_ibd_chainstate.get(), m_snapshot_chainstate.get()}) {
        if (!c) continue;
        (...)
    

    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.

  15. DrahtBot requested review from mzumsande on May 1, 2025
  16. 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.
    61a51eccbb
  17. 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.
    3e6ac5bf77
  18. stickies-v force-pushed on May 6, 2025
  19. stickies-v commented at 1:49 PM on May 6, 2025: contributor

    Force-pushed to remove the GetAll() const method (and commit), replacing its callsites with directly accessing {m_ibd_chainstate.get(), m_snapshot_chainstate.get()} as suggested by @mzumsande.

    Given that #30214 wants to removes GetAll() anyway in 64fc660

    Thanks, I hadn't yet reviewed that PR, and the direction seems sensible. I've adopted your suggestion to minimize work in this and conflicting PRs.

  20. mzumsande commented at 7:19 PM on May 8, 2025: contributor

    Code Review ACK 3e6ac5bf772751c66cdcd015dcb7e6ce4ea2cc77

  21. DrahtBot requested review from shahsb on May 8, 2025
  22. sedited approved
  23. sedited commented at 10:31 AM on May 17, 2025: contributor

    ACK 3e6ac5bf772751c66cdcd015dcb7e6ce4ea2cc77

  24. achow101 commented at 10:23 PM on May 27, 2025: member

    ACK 3e6ac5bf772751c66cdcd015dcb7e6ce4ea2cc77

  25. achow101 merged this on May 27, 2025
  26. achow101 closed this on May 27, 2025

  27. stickies-v deleted the branch on May 28, 2025
  28. sedited referenced this in commit cb3cd9b8b9 on May 29, 2025
  29. yuvicc referenced this in commit 069643f094 on Jul 6, 2025
  30. yuvicc referenced this in commit 22f55cf11d on Aug 26, 2025
  31. bug-castercv502 referenced this in commit 2858098810 on Sep 28, 2025
  32. Kino1994 referenced this in commit bd48f4055b on Jun 28, 2026
  33. BigcoinBGC referenced this in commit d5cbcb7cdd on Jun 30, 2026

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-07-09 06:47 UTC