#35000 test: Add block validation unit tests
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #35000 test: Add block validation unit tests | Ready | 0 (+1) | Strong | 0 + 1,384 tests | |||
|
|
|
| ||||
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
This pull request adds a suite of block validation unit tests along with a reusable test fixture and assertions. The new tests verify consensus failure modes across block header, stateless, contextual, and stateful validation rules. Mutations are exercised across multiple validation entry points, specifically TestBlockValidity, ProcessNewBlock, and ConnectBlock.
Problem: Block validation logic is exercised across multiple entry points with differing checks and requirements, but test coverage targeting specific consensus failure modes and exact reject states at the unit level is limited. As demonstrated during review of efforts to modularize validation (such as PR #32317), subtle validation regressions can slip past existing functional and unit tests.
Evidence of importance:
- The PR description notes: "As noticeable during review of #32317, a subtle issue went unnoticed for some time, which suggests that the current test coverage around block validation paths is not sufficiently comprehensive."
Scope: This PR consists purely of new unit test infrastructure and test suites totaling over 1,300 lines of test code across eight files.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- author: author is evaluating sedited's review feedback on declarative unit test structure while PR is in draft 2026-09-24
- author: evaluating sedited's review feedback and declarative test style suggestions 2026-09-24
Agreement
Strong
substantive support, no open objection (sedited)
Thread notes: Re-read conversion adjusted marcofleon's and w0xlt's claims from objections to suggestions. Added stratospher's initial question and sedited's review suggestions and questions; author put the PR into draft to evaluate adopting a declarative test style. / Converted marcofleon's comment from an objection to a suggestion per current definitions. Added suggestions and questions raised across the review thread, including sedited's recent review feedback which the author deferred to evaluate while mov
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| stratospher (member) | question, approach | whether the fuzz test harness duplicates PR #34651 | answered · author explained differences and acknowledged shared utilities would be useful | answered | yes | 2026-04-04: 'have you seen https://github.com/bitcoin/bitcoin/pull/34651 btw? from a quick skim, this fuzz test looks similar. it uses a wrapper around `ConnectBlock` instead of calling it directly. or am I missing something?' Status from: 2026-04-07: ismaelsadeeq: 'Thanks for pointing that out. I wasn’t aware of it. #34651 and the second commit in this PR, e9115735a45289a783b561fe482ab1540b03d076 have a lot in common' |
| Bortlesboat (contributor) | objection, correctness | test bailout check using logical AND instead of OR risks bad_optional_access if only one AddCoin call fails | fixed · fix pushed · fixed in push | changed | yes | 2026-04-07: 'This uses `&&` so it only bails if both `AddCoin` calls fail. If just one fails you get a `bad_optional_access` from `.value()`. The rest of the file uses `if (!outpoint) return;` for each one individually. Should this be `||`?' Status from: 2026-04-07: ismaelsadeeq: 'Fixed https://github.com/bitcoin/bitcoin/pull/35000#discussion_r3046803139' |
| marcofleon (member) | suggestion, scope | drop the redundant fuzz target and focus on the unit tests | fixed · fix pushed · author pruned redundant fuzz harness in push | answered | yes | 2026-04-10: 'I agree that the fuzz test looks a bit redundant with https://github.com/bitcoin/bitcoin/pull/34651. And I'm not sure it's worth having this PR's harness only as a way to cover the contextual checks... I think it could make sense to drop the fuzz target here and focus on the unit tests, which look well-written to me.' Status from: 2026-04-10: ismaelsadeeq: 'Pruned the fuzz harness added here in favour of the one in #34651' |
| sedited (member) | suggestion, approach | add the same checks through ProcessNewBlock by subscribing to the BlockChecked validation interface event | fixed · fix pushed · author implemented ProcessNewBlock checks in push | answered | yes | 2026-04-22: 'Can we add the same checks through `ProcessNewBlock` by subscribing to the `BlockChecked` validation interface event?' Status from: 2026-05-28: ismaelsadeeq: 'These tests are added now' |
| sedited (member) | suggestion, approach | call ConnectBlock directly with invalid blocks | fixed · fix pushed · author added ConnectBlock tests in push | answered | yes | 2026-04-22: 'As an addition I would also be interested in calling `ConnectBlock` directly with these invalid blocks.' Status from: 2026-05-28: ismaelsadeeq: 'Done in the recent push.' |
| sedited (member) | suggestion, approach | tag tests with an enum and pass through validation entry points instead of splitting into separate tests | answered · author declined to keep the test structure simple | answered | yes | 2026-04-22: 'Instead of splitting them up into separate tests I am wondering if we could tag them with an enum and then pass them to various entry points of validation (i.e. TestBlockValidity, ProcessNewBlock, ConnectBlock).' Status from: 2026-05-28: ismaelsadeeq: 'Hmm, won't this be more involved? I would like to keep it simple as it is currently, if you don't feel strongly about this.' |
| sedited (member) | suggestion, maintenance | move test utilities into the new block_validity util module | fixed · fix pushed · author resolved test util module placement in push | answered | yes | 2026-04-22: 'Why not put these into the new block_validity util module?' Status from: 2026-05-28: ismaelsadeeq: 'https://github.com/bitcoin/bitcoin/pull/35000#discussion_r3156571901 fixed now' |
| w0xlt (contributor) | objection, correctness | assert unused outpoints in AddCoin or test setup so tests fail loudly instead of returning early | fixed · fix pushed · author centralized assertion inside AddCoin in push | changed | yes | 2026-06-04: 'BOOST_REQUIRE(outpoint) seems to be more appropriate than if (!outpoint) return because the outpoint is required for the rest of the test. If helper setup fails, the test will fail immediately instead of silently passing by returning early.' Status from: 2026-06-05: ismaelsadeeq: 'https://github.com/bitcoin/bitcoin/pull/35000#discussion_r3352677834 took the second approach, it's more DRY' |
| w0xlt (contributor) | suggestion, style | use integer arithmetic COIN * 9 / 10 instead of floating-point 0.9 * COIN for CAmount | fixed · fix pushed · applied by author in commit ccfc34482d1120aa05d65c83d3e41f8dbee851b3 | answered | yes | 2026-06-04: '0.9 * COIN uses floating-point arithmetic. COIN * 9 / 10 is evaluated as (COIN * 9) / 10, using integer arithmetic throughout, which fits CAmount better.' Status from: 2026-06-05: ismaelsadeeq: 'https://github.com/bitcoin/bitcoin/pull/35000#discussion_r3352838025 taken' |
| sedited (member) | suggestion, maintenance | move ResetBlock, MakeBlock, AddCoin, and SolveBlockPow into util/mining.h | open | answered | no | 2026-09-18: 'Could ResetBlock, MakeBlock, and AddCoin better fit as standalone functions in `util/mining.h`? Maybe `SolveBlockPow` could be moved over there as well and be called in `MineBlock` and some of the other blaces where we are mining in the tests.' |
| sedited (member) | suggestion, maintenance | remove redundant comment since inheriting from TestChain100Setup is self-explanatory | open | answered | no | 2026-09-18: 'This comment seems redundant, inheriting from TestChain100Setup seems enough of an explainer to me.' |
| sedited (member) | question, maintenance | whether the comment describing returning a template instead of a block is outdated | open | answered | no | 2026-09-18: 'Is this comment outdated? Looks like it is returning a block now, not a template.' |
| sedited (member) | suggestion, correctness | remove retry loop in AddCoin and set possible_overwrite to false | open | answered | no | 2026-09-18: 'A collision seems unlikely to me given we are streaming 256 bits from the chacha keystream. Even if so, would trying again five times really help? The `possible_overwrite` argument could also be set to `false` (and should get a `/*=*/` arg comment).' |
| sedited (member) | suggestion, approach | consider moving toward a declarative, vectorized test style | open | answered | no | 2026-09-19: 'If we want to add way more cases for exercising edge cases and boundary conditions, then I could see the benefits for a more declarative, vectorized style. What are your thought on this?' |
| sedited (member) | question, approach | clarify what belongs in the contextual check tests versus ConnectBlock tests | open | answered | no | 2026-09-19: 'I'm a bit confused about what exactly should be in this file. The comment says this should cover `ContextualCheckBlock`, but some of these are only called in `ConnectBlock`, like this one and `bad-cb-amount`, and others like the witness malleation checks are in another file.' |
| sedited (member) | question, correctness | whether the test comment should say exceeds the block height | open | answered | no | 2026-09-19: 'Should this say exceeds the block height?' |
| sedited (member) | suggestion, scope | drop TestBlockValidity-specific check covered by functional tests | open | answered | no | 2026-09-19: 'Should we really be checking this here? It's probably one of the few things that are purely `TestBlockValidity` specific and is already covered by a functional test.' |
| sedited (member) | question, correctness | explain if the loop in header tests is needed due to non-deterministic setup | open | answered | no | 2026-09-19: 'Is this loop really needed? Is there something non-deterministic about the setup here?' |
| sedited (member) | suggestion, approach | add a unit test for timewarp | open | answered | no | 2026-09-19: 'Do you want to add a test for timewarp?' |
| sedited (member) | suggestion, scope | remove redundant block-script-verify-flag-failed cases covered by interpreter tests | open | answered | no | 2026-09-19: 'Isn't one case exercising the `block-script-verify-flag-failed` failure enough? The other script failure cases should already be covered exhaustively in the interpreter tests.' |
| sedited (member) | suggestion, approach | add a test checking coinbase amounts for a block with fees and multiple coinbase outputs | open | answered | no | 2026-09-19: 'Might it be interesting to add another test that checks coinbase amounts for a block with some fees and multiple coinbase outputs?' |
| sedited (member) | suggestion, approach | check transaction chains within a block | open | answered | no | 2026-09-19: 'Could this be extended with a check for chains of transactions within a block?' |
| sedited (member) | suggestion, approach | add a case exercising SequenceLocks failure | open | answered | no | 2026-09-19: 'Can you add a case here that exercises the SequenceLocks failure ("contains a non-BIP68-final transasction")?' |
Support:
- marcofleon (member) (2026-04-10) [read: unit tests]: the unit tests, which look well-written to me. [not substantive]
- w0xlt (contributor) (Concept ACK, 2026-04-12): Concept ACK [not substantive]
- sedited (member) (Approach ACK, 2026-04-22) [read: Accept*, BLOCK_INVALID_PREV, BLOCK_MISSING_PREV, ConnectBlock, ProcessNewBlock, TestBlockValidity]: This looks like a good place to start - having all these cases is valuable.
- w0xlt (contributor) (ACK, 2026-06-04) [read: src/test/validation_block_contextual_tests.cpp, src/test/validation_block_stateful_tests.cpp]: ACK c9d6eb5d9d1db6d6c0ffc9096932d40b121b2875 [not substantive]
Participants:
- stratospher (member, question, 1 statements 2026-04-04 to 2026-04-04): asked whether the block validity fuzz harness duplicated PR #34651
- Bortlesboat (contributor, objection, 3 statements 2026-04-07 to 2026-04-07): pointed out bad_optional_access bug in test bailout check
- marcofleon (member, support, 1 statements 2026-04-10 to 2026-04-10): suggested dropping redundant fuzz test in favor of unit tests, which looked well-written
- w0xlt (contributor, support, 4 statements 2026-04-12 to 2026-06-04): Concept ACK and ACK; suggested test setup assertion improvement
- sedited (member, support, 19 statements 2026-04-22 to 2026-09-19): Approach ACK; suggested entry points, coverage additions, and declarative test style
Files
1384 lines under test/bench/ci.
- src/test/validation_block_stateful_tests.cpp +485/-0
- src/test/validation_block_stateless_tests.cpp +339/-0
- src/test/validation_block_contextual_tests.cpp +196/-0
- src/test/validation_block_header_tests.cpp +142/-0
- src/test/util/block_validity.cpp +85/-0
- src/test/util/block_validity.h +72/-0
- src/test/block_validity_checks.h +60/-0
- src/test/CMakeLists.txt +5/-0
Card
The short text the ranking pass compares across PRs.
Adds a comprehensive unit test suite and testing fixture covering block validation rules across header, stateless, contextual, and stateful validation layers. Tests verify exact reject reasons and validation results across TestBlockValidity, ProcessNewBlock, and ConnectBlock entry points. This addresses gaps in unit-level consensus testing that allowed regressions to go unnoticed during refactoring efforts like PR #32317. Has no prerequisites.
Data
dossier JSON · extract JSON · model ledger, generated 2026-09-24T23:12, confidence high, input hash edf8b56e81157107
thread read through 2026-09-24 (39 statements), head ccfc34482d
labels: Tests · draft
thread read through 2026-09-24 (39 statements), head ccfc34482d
labels: Tests · draft
Categories
Test infrastructure
P2 · test coverage
Adds critical unit-level coverage for consensus failure modes across multiple block validation entry points. As noted in the description, subtle validation regressions previously went unnoticed during validation modularization (#32317) because existing test coverage was insufficient, making coverage of these high-risk consensus paths strategically important.
Membership: Carries the Tests label and adds reusable unit test utilities (src/test/util/block_validity.cpp/.h) alongside comprehensive block validation suites.
Factors: security/stability 1, bug 0, performance 0, user value 0, leverage 1