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

ReviewabilityReviewsAgreementSize
#35906 purpleKarrot First steps towards a stateless, side-effect free validation libraryReady1 (+1)Strong46 + 809 tests
  • Allow consensus block and header validation checks to run and be tested in isolation
  • Remove the requirement for chainstate setup and system clock reads during validity tests
  • Ready to review
  • Strong support for enabling direct unit tests and passing validation parameters explicitly
  • Tested ACK noting it makes testing validation routines practical (janb84)
  • Approves reducing complexity and making error cases simpler to test (optout21, josibake)
  • Clarification requested on a suggestion to compress tests into a declarative style (sedited)
  • 46 lines added or modified outside tests
  • 809 lines added or modified in tests
  • 37 lines removed in total
  • 6 files, 7 commits
Validation#36 of 53
P3 · test coverage
  • P3 because it makes consensus checking safer to test in isolation and unblocks validation modularization
  • It adds extensive unit test coverage for isolated validity checks without urgent operational need
  • Ranking pass: Moved down: first step of a modularization project with no user-visible effect yet; ranks below measured fixes and speedups
Ranking notes
Reviewability: ReadyStalePaused
Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets
Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs.
Size: lines added or modified outside tests, then in tests; greener = smaller.

Goal

Expose `CheckBlockHeader`, `ContextualCheckBlockHeader`, and `ContextualCheckBlock` in `validation.h`, decoupling them from `ChainstateManager` by passing `Consensus::Params` directly. In `ContextualCheckBlockHeader`, replace the internal system clock call with an explicit timestamp parameter. Extensive unit test suites are added for isolated header and block validity checks.

Problem: Contextual block and header validation functions were file-local static functions in `validation.cpp` tightly coupled to `ChainstateManager` and internal clock reads. This prevented using or unit-testing these consensus validation routines in isolation without heavy chainstate setup.

Evidence of importance:

  • Part of tracking issue #35904 ('Factoring out a stateless, side-effect free validation library').
  • Author description: 'Make the existing CheckBlockHeader, ContextualCheckBlockHeader, and ContextualCheckBlock functions part of the public validation interface... ContextualCheck* functions need a ChainstateManager only to access the consensus parameters. Pass Consensus::Params instead... Pass the validation time explicitly to ContextualCheckBlockHeader instead of reading the system clock internally.'

Scope: Over 90% of the diff (+807 of +855 lines) consists of new Boost unit tests in `src/test/validation_check_block_tests.cpp` and `src/test/validation_check_header_tests.cpp`.

Reviewability

Ready

Nothing known that would invalidate a review now.

Waiting on (from the thread read):

  • reviewer: sedited to clarify their suggestion on declarative test style 2026-10-22
  • reviewer: sedited to clarify their suggestion regarding declarative tests 2026-10-22

Agreement

Strong

substantive support, no open objection (janb84, josibake, optout21)

Thread notes: Converted maflcko's schema 1 objection to a suggestion fixed by the author. Added questions and suggestions from alexanderwiederin, optout21, and sedited. Waiting on sedited to clarify testing style suggestion. / Added inline questions from alexanderwiederin and a test refactoring suggestion from sedited under the re-read. Currently waiting on sedited to clarify their declarative testing suggestion after the author's response.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
maflcko (member)objection, scopeinclude unit tests with the refactor to justify partially reverting earlier commitfixed · fix pushed · author agreed and added testsansweredyes2026-08-06: '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?'
Status from: 2026-08-07: purpleKarrot: 'Good point. Done.'
alexanderwiederin (member)suggestion, interfaceadd a doc comment for context-dependent validity checks for consistencyanswered · author noted comment was superseded by moving docs to headeransweredyes2026-08-20: 'I would suggest adding something like: `/** Context-dependent validity checks */` for consistency with line 394.'
Status from: 2026-08-20: purpleKarrot: 'obsolete with the comment below.'
alexanderwiederin (member)question, interfacewhether doc comments should move to the headeranswered · author agreed docs belong in declarationansweredyes2026-08-20: 'Should these doc comments move to the header now?'
Status from: 2026-08-20: purpleKarrot: 'You are right. the docs should be part of the declaration.'
alexanderwiederin (member)suggestion, interfacealign GetConsensus() calls between CheckBlock and ContextualCheckBlockanswered · author agreedansweredyes2026-08-20: '`params.GetConsensus()` for CheckBlock and `GetConsensus()` for ContectualBlockCheck. Should they be aligned?'
Status from: 2026-08-20: purpleKarrot: 'ok.'
alexanderwiederin (member)question, approachwhy BOOST_FIXTURE_TEST_SUITE is not used like other validation test suitesaccepted · reviewer accepted explanation that stateless functions do not justify a fixtureansweredyes2026-08-20: 'Any reason we don't use `BOOST_FIXTURE_TEST_SUITE` like the other `validation_*_tests.cpp` do?'
Status from: 2026-08-20: alexanderwiederin: 'Make sense - thank you!'
alexanderwiederin (member)question, approachwhether BlockValidationState should be recreated rather than reused across CheckBlock callsanswered · author explained lack of strong opinion and long-term plan to make state a return typeansweredyes2026-08-20: 'This test reuses BlockValidationState across CheckBlock calls, the other tests create new ones. Should we make it consistent?'
Status from: 2026-08-20: purpleKarrot: '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.'
alexanderwiederin (member)suggestion, approachuse BOOST_CHECK_EQUALaccepted · reviewer dropped suggestion as not worth touching againansweredyes2026-08-20: 'What about `BOOST_CHECK_EQUAL`?'
Status from: 2026-08-20: alexanderwiederin: 'I think the error messages are better, but not worth touching again for this.'
optout21 (contributor)suggestion, approachstrengthen test claim by showing invalid result with nNonce = 0 and no prior checkopenansweredno2026-09-07: 'The test claim could be made stronger, by showing that with `nNonce = 0` and no prior check the result is invalid'
sedited (member)suggestion, approachcompress test files using a more declarative testing style instead of repeated boilerplateanswered · author explained current constraints and asked reviewer to clarifyansweredyes2026-10-22: 'Could these two test files be compressed a bit with that in mind?'
Status from: 2026-10-22: purpleKarrot: 'I am not sure we use the same definition of the word "declarative"... But I assume this is not what you meant here. Please clarify.'

Support:

  • janb84 (member) (Tested ACK, 2026-08-12) [both: CheckBlockHeader, ContextualCheckBlock, ContextualCheckBlockHeader]: increases what is practical to test by allowing CheckBlockHeader, ContextualCheckBlockHeader, and ContextualCheckBlock to be called directly
  • josibake (member) (Concept ACK, 2026-08-14) [read: ContextualCheckBlockHeader, tests]: welcomes the new tests and making ContextualCheckBlockHeader no longer read system time
  • yuvicc (contributor) (Concept ACK, 2026-08-31): (no reason given) [not substantive]
  • optout21 (contributor) (ACK, 2026-09-07) [read: CheckBlock, CheckBlockHeader, ContextualCheckBlock, ContextualCheckBlockHeader]: reduces complexity around ContextualCheckBlock/Header by taking consensus params and explicit time instead of chainstate manager, enabling tests of error cases with simple inputs

Participants:

  • maflcko (member, objection, 2 statements 2026-08-06 to 2026-08-06): noted conflict and suggested refactor should include tests to justify reverting earlier commit; author resolved
  • janb84 (member, support, 1 statements 2026-08-12 to 2026-08-12): Tested ACK; highlighted direct testability of validation functions as a big improvement
  • josibake (member, support, 1 statements 2026-08-14 to 2026-08-14): Concept ACK; welcomed new tests and passing validation time explicitly
  • alexanderwiederin (member, question, 8 statements 2026-08-20 to 2026-08-20): asked inline questions on docs, consistency, and test fixtures; satisfied with author's explanations
  • yuvicc (contributor, support, 1 statements 2026-08-31 to 2026-08-31): Concept ACK without elaboration
  • optout21 (contributor, support, 2 statements 2026-09-07 to 2026-09-07): ACK; noted reduced complexity and error testability, with minor test and prefix nits
  • sedited (member, question, 1 statements 2026-10-22 to 2026-10-22): suggested compressing test files with declarative style; author asked for clarification

Dependencies

Enables:

Files

809 lines under test/bench/ci.

  • src/test/validation_check_block_tests.cpp +462/-0
  • src/test/validation_check_header_tests.cpp +345/-0
  • src/validation.cpp +15/-37
  • src/validation.h +25/-0
  • src/deploymentstatus.h +6/-0
  • src/test/CMakeLists.txt +2/-0

Card

The short text the ranking pass compares across PRs.

Refactors `CheckBlockHeader`, `ContextualCheckBlockHeader`, and `ContextualCheckBlock` into public functions in `validation.h`, replacing their `ChainstateManager` dependency with `Consensus::Params` and passing the current time explicitly into `ContextualCheckBlockHeader`. This addresses the tight coupling of block validation logic to chainstate management and system clocks, enabling isolated validation calls and direct unit testing. Over 800 lines of the PR consist of new unit tests covering header and block validation edge cases. The work implements initial steps toward the stateless validation library outlined in #35904.

Data

dossier JSON · extract JSON · model ledger, generated 2026-10-23T01:18, confidence high, input hash c9e928489cf77d85
thread read through 2026-10-22 (24 statements), head 4d4f0331cf
labels: none

Categories

Validation

P3 · test coverage (ranking pass: Moved down: first step of a modularization project with no user-visible effect yet; ranks below measured fixes and speedups)

Decoupling block and header validation functions from chainstate and system clock state makes consensus verification safer to test in isolation and unblocks #35904. The PR adds extensive unit test coverage for isolated validity checks, providing clear and worthwhile architectural improvement that can be reviewed without urgency.

Membership: Changes src/validation.cpp, src/validation.h, and adds isolated consensus validation unit tests pinning block and header checking rules.

Factors: security/stability 1, bug 0, performance 0, user value 0, leverage 2