validation: refactor: encapsulate Chainstate::m_target_blockhash #36166

pull stickies-v wants to merge 1 commits into bitcoin:master from stickies-v:2026-09/set-target-hash-encapsulation changing 2 files +21 −12
  1. stickies-v commented at 8:58 AM on September 4, 2026: contributor

    m_target_blockhash is paired with a mutable m_cached_target_block that must be kept in sync whenever the hash changes.

    Refactor, no behaviour change.

    Addresses #36137 (review)

  2. validation: refactor: encapsulate Chainstate::m_target_blockhash
    m_target_blockhash is paired with a mutable m_cached_target_block that
must be kept in sync whenever the hash changes.
    852f201e09
  3. DrahtBot added the label Validation on Sep 4, 2026
  4. DrahtBot commented at 8:58 AM on September 4, 2026: 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/36166.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline and AI policy for information on the review process.

    Type Reviewers
    ACK kevkevinpal, purpleKarrot, l0rinc, alexanderwiederin, sedited

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  5. kevkevinpal commented at 2:03 PM on September 4, 2026: contributor

    ACK 852f201

    This looks good to me it moves m_target_blockhash from public to protected and adds setters to interface with it while keeping the cache in sync

  6. purpleKarrot commented at 2:44 PM on September 4, 2026: contributor

    ACK 852f201e09cb98e98ad7418ae11933203cd57896

    It is a good approach to no longer let clients break that invariant. But why still allow it to derived classes? Data members should better be private, not protected.

    PS: I read elsewhere that encapsulation "simply buys code churn and inefficiency", so I am triggered whenever I come across encapsulation in this project. If you believe encapsulation to be a good thing, then I would also appreciate your voice in that PR.

  7. l0rinc commented at 5:29 PM on September 4, 2026: contributor

    code review ACK 852f201e09cb98e98ad7418ae11933203cd57896

    m_target_blockhash and m_cached_target_block must stay in sync, closing off direct writes makes sense - either protected or private works, both beat the previous public member.

  8. in src/validation.cpp:5997 in 852f201e09 
    5992 | @@ -5993,8 +5993,8 @@ SnapshotCompletionResult ChainstateManager::MaybeValidateSnapshot(Chainstate& va
5993 |              validated_cs.m_assumeutxo != Assumeutxo::VALIDATED ||
5994 |              !validated_cs.m_chain.Tip() ||
5995 |              // Or the validated chainstate is not targeting the snapshot block...
5996 | -            !validated_cs.m_target_blockhash ||
5997 | -            *validated_cs.m_target_blockhash != *unvalidated_cs.m_from_snapshot_blockhash ||
5998 | +            !validated_cs.TargetBlockHash() ||
5999 | +            *validated_cs.TargetBlockHash() != *unvalidated_cs.m_from_snapshot_blockhash ||
    alexanderwiederin commented at 6:51 PM on September 4, 2026:

    nit: a local const auto target{validated_cs.TargetBlockHash()} would be tidier

    Amrrty commented at 8:23 PM on September 4, 2026:

    🙏

  9. alexanderwiederin commented at 7:11 PM on September 4, 2026: contributor

    ACK 852f201e09cb98e98ad7418ae11933203cd57896

  10. sedited approved
  11. sedited commented at 11:52 AM on September 5, 2026: contributor

    ACK 852f201e09cb98e98ad7418ae11933203cd57896

  12. sedited merged this on Sep 5, 2026
  13. sedited closed this on Sep 5, 2026
Contributors
stickies-vDrahtBotkevkevinpalpurpleKarrotl0rincalexanderwiederinAmrrtysedited
Labels
Validation
Linked (view graph)
#36137 validation: use unused SetTargetBlockHash
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-09-09 07:56 UTC