validation: use unused SetTargetBlockHash #36137

fanquake commented at 9:04 AM on September 1, 2026: member

This was pointed out as unused in #36103 by jeanpablojp, but that seems like a mistake from #30214, where it was introduced. See: #36137 (review).

DrahtBot added the label Validation on Sep 1, 2026

DrahtBot commented at 9:04 AM on September 1, 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/36137.

Reviews

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

Type	Reviewers
ACK	ryanofsky, stickies-v
Stale ACK	hebasto, Eunovo, sedited

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

hebasto approved

hebasto commented at 10:24 AM on September 1, 2026: member

ACK b86c98b3afe0d3652c4b372486f387ef3aa1fc84.

Eunovo commented at 10:44 AM on September 1, 2026: contributor

ACK https://github.com/bitcoin/bitcoin/pull/36137/commits/b86c98b3afe0d3652c4b372486f387ef3aa1fc84

sedited approved

sedited commented at 11:09 AM on September 1, 2026: contributor

ACK b86c98b3afe0d3652c4b372486f387ef3aa1fc84

stickies-v commented at 11:33 AM on September 1, 2026: contributor

.

in src/validation.h:665 in b86c98b3af

 659 | @@ -660,9 +660,6 @@ class Chainstate
 660 |      //! the most-work valid block. If non-null chainstate will be a historic
 661 |      //! chainstate and target the specified block.
 662 |      void SetTargetBlock(CBlockIndex* block) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 663 | -    //! Set target block for this chainstate using just a block hash. Useful
 664 | -    //! when the block database has not been loaded yet.
 665 | -    void SetTargetBlockHash(uint256 block_hash) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);

stickies-v commented at 11:33 AM on September 1, 2026:

I don't think removing it is the correct approach. When the target blockhash changes, the cache needs to be invalidated too. Why would we not use proper encapsulation to achieve that? We already have a lot the machinery in place too.

diff --git a/src/validation.cpp b/src/validation.cpp
index 6c960f6b5c..92aea9cd3c 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -1903,6 +1903,12 @@ void Chainstate::SetTargetBlock(CBlockIndex* block)
     m_cached_target_block = block;
 }
 
+void Chainstate::SetTargetBlockHash(std::optional<uint256> block_hash)
+{
+    m_target_blockhash = block_hash;
+    m_cached_target_block = nullptr;
+}
+
 void Chainstate::InitCoinsDB(
     size_t cache_size_bytes,
     bool in_memory,
@@ -5987,8 +5993,8 @@ SnapshotCompletionResult ChainstateManager::MaybeValidateSnapshot(Chainstate& va
             validated_cs.m_assumeutxo != Assumeutxo::VALIDATED ||
             !validated_cs.m_chain.Tip() ||
             // Or the validated chainstate is not targeting the snapshot block...
-            !validated_cs.m_target_blockhash ||
-            *validated_cs.m_target_blockhash != *unvalidated_cs.m_from_snapshot_blockhash ||
+            !validated_cs.TargetBlockHash() ||
+            *validated_cs.TargetBlockHash() != *unvalidated_cs.m_from_snapshot_blockhash ||
             // Or the validated chainstate has not reached the snapshot block yet...
             !validated_cs.ReachedTarget()) {
        // Then the snapshot cannot be validated and there is nothing to do.
@@ -6184,8 +6190,8 @@ Chainstate& ChainstateManager::AddChainstate(std::unique_ptr<Chainstate> chainst
     Chainstate& prev_chainstate{CurrentChainstate()};
     assert(prev_chainstate.m_assumeutxo == Assumeutxo::VALIDATED);
     // Set target block for historical chainstate to snapshot block.
-    assert(!prev_chainstate.m_target_blockhash);
-    prev_chainstate.m_target_blockhash = chainstate->m_from_snapshot_blockhash;
+    assert(!prev_chainstate.TargetBlockHash());
+    prev_chainstate.SetTargetBlockHash(chainstate->m_from_snapshot_blockhash);
     m_chainstates.push_back(std::move(chainstate));
     Chainstate& curr_chainstate{CurrentChainstate()};
     assert(&curr_chainstate == m_chainstates.back().get());
@@ -6262,7 +6268,7 @@ bool ChainstateManager::DeleteChainstate(Chainstate& chainstate)
 
 ChainstateRole Chainstate::GetRole() const
 {
-    return ChainstateRole{.validated = m_assumeutxo == Assumeutxo::VALIDATED, .historical = bool{m_target_blockhash}};
+    return ChainstateRole{.validated = m_assumeutxo == Assumeutxo::VALIDATED, .historical = TargetBlockHash().has_value()};
 }
 
 void ChainstateManager::RecalculateBestHeader()
diff --git a/src/validation.h b/src/validation.h
index 44998f1fa9..ac08e26cfa 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -566,6 +566,12 @@ protected:
     //! Cached result of LookupBlockIndex(*m_from_snapshot_blockhash)
     mutable const CBlockIndex* m_cached_snapshot_base GUARDED_BY(::cs_main){nullptr};
 
+    //! Target block for this chainstate. If this is not set, chainstate will
+    //! target the most-work, valid block. If this is set, ChainstateManager
+    //! considers this a "historical" chainstate since it will only contain old
+    //! blocks up to the target block, not newer blocks.
+    std::optional<uint256> m_target_blockhash GUARDED_BY(::cs_main);
+
     //! Cached result of LookupBlockIndex(*m_target_blockhash)
     mutable const CBlockIndex* m_cached_target_block GUARDED_BY(::cs_main){nullptr};
 
@@ -635,12 +641,6 @@ public:
      */
     const std::optional<uint256> m_from_snapshot_blockhash;
 
-    //! Target block for this chainstate. If this is not set, chainstate will
-    //! target the most-work, valid block. If this is set, ChainstateManager
-    //! considers this a "historical" chainstate since it will only contain old
-    //! blocks up to the target block, not newer blocks.
-    std::optional<uint256> m_target_blockhash GUARDED_BY(::cs_main);
-
     //! Hash of the UTXO set at the target block, computed when the chainstate
     //! reaches the target block, and null before then.
     std::optional<AssumeutxoHash> m_target_utxohash GUARDED_BY(::cs_main);
@@ -654,13 +654,29 @@ public:
 
     //! Return target block which chainstate tip is expected to reach, if this
     //! is a historic chainstate being used to validate a snapshot, or null if
-    //! chainstate targets the most-work block.
+    //! chainstate targets the most-work block. Requires the block index to be
+    //! loaded, so prefer TargetBlockHash() when the block itself is not needed.
     const CBlockIndex* TargetBlock() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+
+    //! Return hash of the target block, or nullopt if this chainstate targets
+    //! the most-work block. Unlike TargetBlock(), does not require the block
+    //! index to be loaded.
+    std::optional<uint256> TargetBlockHash() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
+    {
+        AssertLockHeld(::cs_main);
+        return m_target_blockhash;
+    }
+
     //! Set target block for this chainstate. If null, chainstate will target
     //! the most-work valid block. If non-null chainstate will be a historic
     //! chainstate and target the specified block.
     void SetTargetBlock(CBlockIndex* block) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 
+    //! Set target block for this chainstate using just a block hash, for use
+    //! when the block index has not been loaded yet and no CBlockIndex is
+    //! available. The block is looked up lazily by TargetBlock().
+    void SetTargetBlockHash(std::optional<uint256> block_hash) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+
     //! Return true if chainstate reached target block.
     bool ReachedTarget() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
     {
@@ -1120,7 +1136,7 @@ public:
     Chainstate& CurrentChainstate() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex())
     {
         for (auto& cs : m_chainstates) {
-            if (cs && cs->m_assumeutxo != Assumeutxo::INVALID && !cs->m_target_blockhash) return *cs;
+            if (cs && cs->m_assumeutxo != Assumeutxo::INVALID && !cs->TargetBlockHash()) return *cs;
         }
         abort();
     }
@@ -1129,7 +1145,7 @@ public:
     Chainstate* HistoricalChainstate() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex())
     {
         for (auto& cs : m_chainstates) {
-            if (cs && cs->m_assumeutxo != Assumeutxo::INVALID && cs->m_target_blockhash && !cs->m_target_utxohash) return cs.get();
+            if (cs && cs->m_assumeutxo != Assumeutxo::INVALID && cs->TargetBlockHash() && !cs->m_target_utxohash) return cs.get();
         }
         return nullptr;
     }

</details>

stickies-v commented at 11:42 AM on September 1, 2026:

Note: the usage of SetTargetBlockHash was removed (491d827d5284ed984ee2b11daaee50321217eac5) in the same PR (#30214)that introduced (6082c84713f42f5fa66f9a76baef17e8ed231633) the function. I'm not sure if 491d827d5284ed984ee2b11daaee50321217eac5 did that on purpose or if it's an accident that snuck in during rebase. Given that the rest of the code does properly use encapsulation with SetTargetBlock and TargetBlock functions, I think this was an accident. @ryanofsky perhaps you can elaborate?

ryanofsky commented at 4:49 PM on September 1, 2026:

Yes I think you're right. It looks like an accident to drop the SetTargetBlockHash call in 491d827d5284ed984ee2b11daaee50321217eac5, and would be safer to keep it. Maybe this PR can be updated to restore it in a one-line change:

-prev_chainstate.m_target_blockhash = chainstate->m_from_snapshot_blockhash;
+prev_chainstate.SetTargetBlockHash(*Assert(chainstate->m_from_snapshot_blockhash));

I think your encapsulation diff #36137 (review) also looks good, and it could be another alternative, or a followup.

fanquake commented at 10:32 AM on September 3, 2026:

I've swapped this to your suggested one-liner. Maybe @stickies-v can improve the encapsulation in a followup?

stickies-v commented at 8:58 AM on September 4, 2026:

Follow-up in https://github.com/bitcoin/bitcoin/pull/36166

stickies-v commented at 11:34 AM on September 1, 2026: contributor

Approach NACK. Improving encapsulation seems like the better direction to me.

fanquake marked this as a draft on Sep 1, 2026

validation: use unused SetTargetBlockHash

This was pointed out as unused in #36103, but that seems like a mistake
from #30214, where it was introduced.

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

4550801058

fanquake renamed this:
~~validation: remove unused SetTargetBlockHash~~
validation: use unused SetTargetBlockHash
on Sep 3, 2026

fanquake force-pushed on Sep 3, 2026

fanquake marked this as ready for review on Sep 3, 2026

ryanofsky commented at 11:42 AM on September 3, 2026: contributor

Code review ACK 455080105835b07b3eaba7a309ec51abe566b379

New assert on m_from_snapshot_blockhash is good because it wouldn't make sense for ChainstateManager::AddChainstate to set the target block for the old chainstate and transfer the mempool if m_from_snapshot_blockhash wasn't set, and both current callers (ActivateSnapshot and LoadAssumeutxoChainstate) are adding snapshot chainstates and set it.

Calling SetTargetBlockHash is also better than setting m_cached_target_block, for encapsulation reasons stickies mentioned. It is not a change in behavior even though SetTargetBlockHash sets m_cached_target_block to null because it should have been null before. The assert(!prev_chainstate.m_target_blockhash) on line 6193 confirms no target block could have been set previously.

DrahtBot requested review from Eunovo on Sep 3, 2026

DrahtBot requested review from stickies-v on Sep 3, 2026

DrahtBot requested review from hebasto on Sep 3, 2026

DrahtBot requested review from sedited on Sep 3, 2026

stickies-v commented at 12:29 PM on September 3, 2026: contributor

ACK 455080105835b07b3eaba7a309ec51abe566b379

sedited merged this on Sep 3, 2026

sedited closed this on Sep 3, 2026

fanquake deleted the branch on Sep 3, 2026