validation: Separate check-only version of ConnectBlock #36066

optout21 commented at 1:26 PM on August 24, 2026: contributor

This is a small code-hygiene refactor to ConnectBlock method.

Problem: In the "check-only" mode of ConnectBlock the mutability of the CBlockIndex parameter is inconsistent with the semantics. In the default operation, ConnectBlock changes the provided CBlockIndex. However, it has a special "check-only" mode, where the provided chain is not changed. This mode is used from only one call site (TestBlockValidity), and is signaled by the fJustCheck = true flag.

Solution: Provide two versions of ConnectBlock: one with mutable chain and one "check-only" with const chain. There is no need for the fJustCheck flag. Both reuse the same internal implementation.

Benefits:

The mutability of the pindex parameter is in sync with the semantics
One less feature flag (fJustCheck)
Calling "check-only" mode is smoother, creating a dummy mutable CBlockIndex is now unnecessary

Additional details:

This has been triggered by #35187, there is also a new usage of "check-only" ConnectBlock, with dummy CBlockIndex.
The "check-only" mode was introduced as early as 3cd01fdf0e540c4e06cd27b6c0d6b6abc00767d1 .
No behavior change (pure refactor).
Some internal variables (blockundo, nInputs, nSigOpsCost) are now outputs of the internal method, and used in the default outer method (ConnectBlock).
SetBestBlock has been moved to the outer ConnectBlock. It was also present in the special genesis block early return, this is preserved.
The tracepoints are in the outer methods
Timing is present both in the inner method (logs) and the outer methods (logs, tracepoint)

Separate out check-only version of ConnectBlock

New `TestConnectBlock` method for the check-only variant of `ConnectBlock`,
with const `CBlockIndex` parameter. The no-longer-needed `fJustCheck` flag
is dropped.

0e832dbd04

DrahtBot added the label Validation on Aug 24, 2026

DrahtBot commented at 1:26 PM on August 24, 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/36066.

Reviews

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

Type	Reviewers
Concept ACK	l0rinc

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36000 (validation: prefetch blocks while connecting by l0rinc)
#35751 (validation: use parallel input prevout fetching in TestBlockValidity by andrewtoth)
#35646 (RFC: Separate out runtime errors from BlockValidationState using util::Expected by yuvicc)
#35570 (refactor: Change some validation.cpp methods to return BlockValidationState by optout21)
#35511 (RFC: consensus: Make CAmount a class by hodlinator)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

optout21 marked this as ready for review on Aug 24, 2026

in src/validation.cpp:2692 in 0e832dbd04

2688 | @@ -2670,7 +2689,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
2689 |               Ticks<MillisecondsDouble>(m_chainman.time_index) / m_chainman.num_blocks_total);
2690 |  
2691 |      TRACEPOINT(validation, block_connected,
2692 | -        block_hash.data(),
2693 | +        block.GetHash().data(),

l0rinc commented at 3:29 AM on August 25, 2026:

ConnectBlockChecks() already hashes the block and verifies that it matches pindex. Could we use the stored index hash here instead?

        pindex->GetBlockHash().data(),

in src/validation.cpp:4581 in 0e832dbd04

4576 | @@ -4547,8 +4577,8 @@ BlockValidationState TestBlockValidity(
4577 |      index_dummy.phashBlock = &block_hash;
4578 |      CCoinsViewCache view_dummy(&chainstate.CoinsTip());
4579 |  
4580 | -    // Set fJustCheck to true in order to update, and not clear, validation caches.
4581 | -    if(!chainstate.ConnectBlock(block, state, &index_dummy, view_dummy, /*fJustCheck=*/true)) {
4582 | +    // Call TestConnectBlock(), it updates, and does not clear, validation caches.
4583 | +    if (!chainstate.TestConnectBlock(block, state, &index_dummy, view_dummy)) {

l0rinc commented at 4:23 AM on August 25, 2026:

TestConnectBlock is never called with a possibly-nullptr block index, consider a const reference instead:

<details><summary>pass block index by reference </summary>

diff --git a/src/validation.cpp b/src/validation.cpp
index a4b86f18a9..83bd02944c 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2700,7 +2700,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
     return true;
 }
 
-bool Chainstate::TestConnectBlock(const CBlock& block, BlockValidationState& state, const CBlockIndex* pindex,
+bool Chainstate::TestConnectBlock(const CBlock& block, BlockValidationState& state, const CBlockIndex& index,
                                    CCoinsViewCache& view)
 {
     AssertLockHeld(cs_main);
@@ -2708,7 +2708,7 @@ bool Chainstate::TestConnectBlock(const CBlock& block, BlockValidationState& sta
     CBlockUndo blockundo;
     int nInputs = 0;
     int64_t nSigOpsCost = 0;
-    return ConnectBlockChecks(block, state, pindex, view, /*fJustCheck=*/true, blockundo, nInputs, nSigOpsCost);
+    return ConnectBlockChecks(block, state, &index, view, /*fJustCheck=*/true, blockundo, nInputs, nSigOpsCost);
 }
 
 CoinsCacheSizeState Chainstate::GetCoinsCacheSizeState()
@@ -4578,7 +4578,7 @@ BlockValidationState TestBlockValidity(
     CCoinsViewCache view_dummy(&chainstate.CoinsTip());
 
     // Call TestConnectBlock(), it updates, and does not clear, validation caches.
-    if (!chainstate.TestConnectBlock(block, state, &index_dummy, view_dummy)) {
+    if (!chainstate.TestConnectBlock(block, state, index_dummy, view_dummy)) {
         if (state.IsValid()) NONFATAL_UNREACHABLE();
         return state;
     }
diff --git a/src/validation.h b/src/validation.h
index dfc87331e8..1d2c773296 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -784,8 +784,8 @@ public:
     //! Connect a block to the chain, updating pindex and the block undo/index files on disk.
     bool ConnectBlock(const CBlock& block, BlockValidationState& state, CBlockIndex* pindex,
         CCoinsViewCache& view) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
-    //! Run the same validity checks as ConnectBlock() without mutating pindex or writing anything to disk.
-    bool TestConnectBlock(const CBlock& block, BlockValidationState& state, const CBlockIndex* pindex,
+    //! Run the same validity checks as ConnectBlock() without mutating the block index or writing anything to disk.
+    bool TestConnectBlock(const CBlock& block, BlockValidationState& state, const CBlockIndex& index,
         CCoinsViewCache& view) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
     // Apply the effects of a block disconnection on the UTXO set.

</details>

in src/validation.h:787 in 0e832dbd04

 780 | @@ -781,8 +781,12 @@ class Chainstate
 781 |      // Block (dis)connection on a given view:
 782 |      DisconnectResult DisconnectBlock(const CBlock& block, const CBlockIndex* pindex, CCoinsViewCache& view)
 783 |          EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 784 | +    //! Connect a block to the chain, updating pindex and the block undo/index files on disk.
 785 |      bool ConnectBlock(const CBlock& block, BlockValidationState& state, CBlockIndex* pindex,
 786 | -                      CCoinsViewCache& view, bool fJustCheck = false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 787 | +        CCoinsViewCache& view) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 788 | +    //! Run the same validity checks as ConnectBlock() without mutating pindex or writing anything to disk.

l0rinc commented at 4:25 AM on August 25, 2026:

Some of the related comments could be updated now.

<details><summary>update check-only block comments</summary>

diff --git a/src/validation.cpp b/src/validation.cpp
index 611fc16805..3c0dd35729 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -4568,7 +4568,7 @@ BlockValidationState TestBlockValidity(
         return state;
     }
 
-    // We don't want ConnectBlock to update the actual chainstate, so create
+    // We don't want TestConnectBlock to update the actual chainstate, so create
     // a cache on top of it, along with a dummy block index.
     CBlockIndex index_dummy{block};
     uint256 block_hash(block.GetHash());
diff --git a/test/functional/interface_usdt_utxocache.py b/test/functional/interface_usdt_utxocache.py
index da90790d70..37cc205541 100755
--- a/test/functional/interface_usdt_utxocache.py
+++ b/test/functional/interface_usdt_utxocache.py
@@ -241,7 +241,7 @@ class UTXOCacheTracepointTest(BitcoinTestFramework):
         # of the UTXO set (see CoinsTip() of CCoinsViewCache). However, in some cases
         # temporary clones of the active cache are made. For example, during mining with
         # the generate RPC call, the block is first tested in TestBlockValidity(). There,
-        # a clone of the active cache is modified during a test ConnectBlock() call.
+        # a clone of the active cache is modified during a TestConnectBlock() call.
         # These are implementation details we don't want to test here. Thus, after
         # mining, we invalidate the block, start the tracing, and then trace the cache
         # changes to the active utxo cache.

</details>

purpleKarrot commented at 9:49 AM on August 25, 2026: contributor

What about the mutability and mutations of the Chainstate (the implicit this argument)? With the proposed change, calling TestConnectBlock still has the side effect of mutating num_blocks_total, for example.

Please have a look at #35904, which proposes extracting the block validation parts from ConnectBlock into a function that has no side effects.

optout21 commented at 10:07 AM on August 25, 2026: contributor

Thanks for the feedback, @purpleKarrot ! I will check out #35904 (I've seen 35906, but not 35904). I'm also continuing exploring, currently looking at these small issues:

in check-only mode, block undo data is computed unnecessarily (a small optimization potential).
in check-only mode, the coins view that is being updated can be created inside (by TestConnectBlock), so the caller does not need to create a dummy. Also, the CBlockIndex supplied can be simplified (not to require a CBlockIndex that already contains the new block).

l0rinc changes_requested

l0rinc commented at 7:33 PM on August 26, 2026: contributor

Concept ACK

Separating the check-only entry point makes the CBlockIndex mutability clearer and removes fJustCheck from the public ConnectBlock() interface. I've split this locally (consider splitting the change similarly, see https://github.com/bitcoin/bitcoin/compare/master...l0rinc:bitcoin:l0rinc/validation-test-connect-block):

add and route through TestConnectBlock()
expose the connection outputs
handle failed checks and genesis in ConnectBlock()
move the ordered undo/index/tracepoint updates
then make the check-only index const.

My pending comments cover reusing the stored index hash, passing the index by const reference, and updating the related comments. The broader Chainstate side effects are already being discussed above.

proposes extracting the block validation parts from ConnectBlock into a function that has no side effects

+1 for these functional-style refactors (though, as mentioned, exceptions are the opposite of that), but given how consensus-critical this code is, I’d prefer moving toward side-effect-free validation through tiny, trivial-to-review changes, with this PR limited to making the CBlockIndex mutability explicit (and split into smaller changes that are all obviously correct).