First steps towards a stateless, side-effect free validation library #35906

purpleKarrot commented at 6:50 PM on August 5, 2026: contributor

This implements the following actions from #35904:

Expose the validation interface

Make the existing CheckBlockHeader, ContextualCheckBlockHeader, and ContextualCheckBlock functions part of the public validation interface by declaring them in validation.h.
Interface Segregation: Remove dependency on ChainstateManager

ContextualCheck* functions need a ChainstateManager only to access the consensus parameters. Pass Consensus::Params instead and access them at the call site.
Make environmental dependencies explicit

Pass the validation time explicitly to ContextualCheckBlockHeader instead of reading the system clock internally.

DrahtBot commented at 6:50 PM on August 5, 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/35906.

Reviews

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

Type	Reviewers
Concept ACK	josibake, yuvicc
Stale ACK	janb84

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:

#35820 (refactor: keep duration calculations typed by l0rinc)
#35570 (refactor: Change some validation.cpp methods to return BlockValidationState by optout21)
#35557 (kernel, validation: Add btck_chainstate_manager_set_clock_time by ryanofsky)
#30342 (kernel, logging: Pass Logger instances to kernel objects by ryanofsky)

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.

in src/validation.cpp:4123 in 7b358c2637 outdated

4119 | @@ -4121,14 +4120,14 @@ static bool ContextualCheckBlockHeader(const CBlockHeader& block, BlockValidatio
4120 |      }
4121 |  
4122 |      // Check timestamp
4123 | -    if (block.Time() > NodeClock::now() + std::chrono::seconds{MAX_FUTURE_BLOCK_TIME}) {
4124 | +    if (block.Time() > now + std::chrono::seconds{MAX_FUTURE_BLOCK_TIME}) {

maflcko commented at 6:54 AM on August 6, 2026:

This conflicts with #35557, which is an alternative to #35496.

No opinion on the merge order, but I think it would be good if the authors of all three alternatives provided reviews on each pull request, so that ideally the same line of code is only changed once.

in src/validation.cpp:4128 in 7b358c2637 outdated

4127 |  
4128 |      // Reject blocks with outdated version
4129 | -    if ((block.nVersion < 2 && DeploymentActiveAfter(pindexPrev, chainman, Consensus::DEPLOYMENT_HEIGHTINCB)) ||
4130 | -        (block.nVersion < 3 && DeploymentActiveAfter(pindexPrev, chainman, Consensus::DEPLOYMENT_DERSIG)) ||
4131 | -        (block.nVersion < 4 && DeploymentActiveAfter(pindexPrev, chainman, Consensus::DEPLOYMENT_CLTV))) {
4132 | +    if ((block.nVersion < 2 && DeploymentActiveAfter(pindexPrev, consensusParams, Consensus::DEPLOYMENT_HEIGHTINCB)) ||

maflcko commented at 6:57 AM on August 6, 2026:

this partially reverts 78adef17536edef833a0bfca06b61ce28120e486. Maybe that is fine, but if the motivation is better tests, maybe the refactor change would come with better tests included?

E.g. this pull conflicts with #34895, which seems to be adding tests.

purpleKarrot commented at 11:13 AM on August 7, 2026:

maybe the refactor change would come with better tests included

Good point. Done.

purpleKarrot force-pushed on Aug 7, 2026

purpleKarrot force-pushed on Aug 11, 2026

janb84 commented at 11:50 AM on August 12, 2026: contributor

t ACK 6b9891ee27d892283d1e9bbd91c08ae9a1087a55

This PR undertakes three (small) refactors that increase what is practical to test. All three functions were static before, so CheckBlockHeader, ContextualCheckBlockHeader and ContextualCheckBlock could only be tested indirectly, by pushing a block through validation (from a unit test or by submitting it over RPC or p2p). With these changes they can be called directly. (Big improvement IMHO)

PR is part of a bigger refactor to make a side-effect validation library possible.

josibake commented at 12:24 PM on August 14, 2026: member

Concept ACK

Loving the new tests. Will be digging into this more next week, but at first glance the changes seem relatively straightforward. Also very happy to see ContextualCheckBlockHeader no longer reading system time.

in src/validation.h:397 in 1eab32ec19 outdated

 391 | @@ -392,8 +392,12 @@ class ValidationCache
 392 |  /** Functions for validating blocks and updating the block tree */
 393 |  
 394 |  /** Context-independent validity checks */
 395 | +bool CheckBlockHeader(const CBlockHeader& block, BlockValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW = true);
 396 |  bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW = true, bool fCheckMerkleRoot = true);
 397 |

alexanderwiederin commented at 2:14 PM on August 20, 2026:

I would suggest adding something like: /** Context-dependent validity checks */ for consistency with line 394.

purpleKarrot commented at 7:43 PM on August 20, 2026:

obsolete with the comment below.

in src/validation.cpp:4093 in 1eab32ec19

4090 | @@ -4091,7 +4091,7 @@ arith_uint256 CalculateClaimedHeadersWork(std::span<const CBlockHeader> headers)
4091 |   *  v0.12 and v0.15 (when no additional protection was in place) whereby an attacker could unboundedly
4092 |   *  grow our in-memory block index. See https://bitcoincore.org/en/2024/07/03/disclose-header-spam.
4093 |   */

alexanderwiederin commented at 2:15 PM on August 20, 2026:

Should these doc comments move to the header now?

purpleKarrot commented at 7:43 PM on August 20, 2026:

You are right. the docs should be part of the declaration.

in src/validation.cpp:4364 in 5cf507887c

4360 | @@ -4362,7 +4361,7 @@ bool ChainstateManager::AcceptBlock(const std::shared_ptr<const CBlock>& pblock,
4361 |      const CChainParams& params{GetParams()};
4362 |  
4363 |      if (!CheckBlock(block, state, params.GetConsensus()) ||
4364 | -        !ContextualCheckBlock(block, state, *this, pindex->pprev)) {
4365 | +        !ContextualCheckBlock(block, state, GetConsensus(), pindex->pprev)) {

alexanderwiederin commented at 2:22 PM on August 20, 2026:

params.GetConsensus() for CheckBlock and GetConsensus() for ContectualBlockCheck. Should they be aligned?

purpleKarrot commented at 7:44 PM on August 20, 2026:

ok.

in src/test/validation_check_block_tests.cpp:27 in 6b9891ee27 outdated

  22 | +#include <span>
  23 | +#include <string>
  24 | +#include <utility>
  25 | +#include <vector>
  26 | +
  27 | +BOOST_AUTO_TEST_SUITE(validation_check_block_tests)

alexanderwiederin commented at 2:34 PM on August 20, 2026:

Any reason we don't use BOOST_FIXTURE_TEST_SUITE like the other validation_*_tests.cpp do?

purpleKarrot commented at 7:50 PM on August 20, 2026:

Since the functions under test are stateless, there is not much test setup that would justify a fixture.

alexanderwiederin commented at 11:27 PM on August 20, 2026:

Make sense - thank you!

in src/test/validation_check_block_tests.cpp:94 in 6b9891ee27 outdated

  89 | +    block.nNonce = 0;
  90 | +    BOOST_CHECK(CheckBlock(block, state, consensusParams));
  91 | +    BOOST_CHECK(state.IsValid());
  92 | +}
  93 | +
  94 | +BOOST_AUTO_TEST_CASE(block_not_cached_on_partial_check)

alexanderwiederin commented at 2:35 PM on August 20, 2026:

This test reuses BlockValidationState across CheckBlock calls, the other tests create new ones. Should we make it consistent?

purpleKarrot commented at 7:52 PM on August 20, 2026:

No strong opinion for this test. I think the state parameter is a smell anyway and it should (and will) become a return type in the long run, which will solve this.

in src/test/validation_check_header_tests.cpp:50 in 6b9891ee27 outdated

  45 | +
  46 | +    BlockValidationState state;
  47 | +
  48 | +    BOOST_CHECK(!CheckBlockHeader(header, state, consensusParams));
  49 | +    BOOST_CHECK(state.IsInvalid());
  50 | +    BOOST_CHECK(state.GetRejectReason() == "high-hash");

alexanderwiederin commented at 2:38 PM on August 20, 2026:

What about BOOST_CHECK_EQUAL?

purpleKarrot commented at 7:48 PM on August 20, 2026:

Is there an advantage?

alexanderwiederin commented at 11:28 PM on August 20, 2026:

I think the error messages are better, but not worth touching again for this.

Make validation functions accessible

Make the existing `CheckBlockHeader`, `ContextualCheckBlockHeader`, and
`ContextualCheckBlock` functions part of the public validation interface
by declaring them in `validation.h`.

3d000d1d58

Make `ContextualCheck*` not depend on `ChainstateManager`

`ContextualCheck*` functions need a `ChainstateManager` only to access
the consensus parameters. Pass `Consensus::Params` instead and access
them at the call site.

2f704427d9

Make `ContextualCheckHeader` not depend on system time

Pass the validation time explicitly to `ContextualCheckBlockHeader`
instead of reading the system clock internally.

da437c50d4

validation: Add unit tests for `CheckBlockHeader` e1143e3914

validation: Add unit tests for `ContextualCheckBlockHeader` e167e61663

validation: Add unit tests for `CheckBlock` c135dfde11

validation: Add unit tests for `ContextualCheckBlock` 4d4f0331cf

purpleKarrot force-pushed on Aug 20, 2026

yuvicc commented at 4:00 AM on August 31, 2026: contributor

Concept ACK