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

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)

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

DrahtBot added the label Validation on Sep 4, 2026
DrahtBot commented at 8:58 AM on September 4, 2026: contributor



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



Code Coverage & Benchmarks

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



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></code> into the comment that the bot should ignore.
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
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.
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.

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

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:

🙏

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

ACK 852f201e09cb98e98ad7418ae11933203cd57896
sedited approved
sedited commented at 11:52 AM on September 5, 2026: contributor

ACK 852f201e09cb98e98ad7418ae11933203cd57896
sedited merged this on Sep 5, 2026
sedited closed this on Sep 5, 2026
Kino1994 referenced this in commit ca901ccfe5 on Sep 6, 2026
stickies-v deleted the branch on Sep 9, 2026

Contributors

Labels

Linked (view graph)

#36137 validation: use unused SetTargetBlockHash

validation: refactor: encapsulate Chainstate::m_target_blockhash #36166

Code Coverage & Benchmarks

Reviews