#35000 test: Add block validation unit tests
https://github.com/bitcoin/bitcoin/pull/35000 · · +1384/-0 in 8 files, 3 commits · labels: Tests
Goal
- Add fast unit test coverage for block validation consensus rules
- Catch consensus regressions earlier during validation modularization refactors
This pull request adds four new unit test suites covering block header, stateless, contextual, and stateful consensus validation failure modes. It also introduces reusable block validity test utilities in src/test/util/block_validity.h to exercise mutated blocks across TestBlockValidity, ProcessNewBlock, and ConnectBlock.
Problem: Block validation logic is exercised across multiple entry points and subtle consensus regressions can easily go unnoticed during refactoring, as occurred during the review of kernel block validation modularization (#32317). Existing functional tests exercise happy paths well but lack structured unit-level verification of specific consensus reject reasons.
Category: Test infrastructure (#1 of 45)
P2 · test coverage
- P3 because unit tests on high-risk consensus paths provide faster test feedback than functional tests
- Provides reusable test utilities for constructing mutated blocks and checking outcomes
P2 because it adds comprehensive unit-level test coverage for high-risk consensus and validation code paths across multiple entry points. As noted in the description citing #32317, subtle validation regressions have historically gone unnoticed due to lack of granular failure mode tests, and these tests significantly improve confidence in consensus-critical code.
Membership: Adds unit tests and unit testing utilities under src/test/ for block validation
Factors: security/stability 2, bug 0, performance 0, user value 0, leverage 2
Reviewability: Stale: Author silent 103 days
- Author has been inactive for over three months
- CI is passing and earlier review threads have been addressed
Author activity has been absent for 103 days, exceeding the 60-day inactivity threshold. However, all previous reviewer comments were addressed in the final push and CI is green.
Author status: silent since 2026-06-05 after addressing all review feedback
Resolved concerns:
- stratospher and marcofleon noted overlap between an earlier fuzz target in this PR and #34651; author pruned the fuzz commit in favor of #34651.
- sedited suggested expanding tests beyond TestBlockValidity to include ProcessNewBlock and ConnectBlock as well as missing/invalid prev block handling; author implemented these suites.
- w0xlt noted test assertion nits around AddCoin optional handling and integer fee calculation; author resolved them in the latest push.
Agreement: Strong
- Strong support for adding structured block validity unit tests
- Approach approval because the comprehensive test cases are valuable (sedited)
- Full approval after earlier nits and helper assertions were addressed (w0xlt)
- Suggested removing the redundant fuzz target to keep the focus on unit tests (marcofleon)
Strong consensus across reviewers (sedited, w0xlt, marcofleon) with all suggestions and nits addressed.
The PR has received an Approach ACK from sedited and a full ACK from w0xlt. All suggestions regarding scope, additional validation entry points, and test assertion helpers have been resolved.
- 2026-04-22 sedited: 'Approach ACK. This looks like a good place to start - having all these cases is valuable.'
- 2026-06-04 w0xlt: 'ACK c9d6eb5d9d1db6d6c0ffc9096932d40b121b2875'
- 2026-04-10 marcofleon: 'the unit tests, which look well-written to me.'
Objections:
| Reviewer | Kind | Harm | Status | Blocking | Author replied | Quote |
|---|---|---|---|---|---|---|
| Bortlesboat | correctness | test bailout check used logical AND instead of OR, risking bad optional access if one coin addition failed | resolved | no | 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().' Settled: 2026-04-07: 'Good catch, I will update.' followed by force push updating the checks |
| marcofleon | scope | maintaining a redundant fuzz harness alongside PR #34651 wastes review and CI resources | resolved | no | yes | 2026-04-10: 'I agree that the fuzz test looks a bit redundant with #34651... it could make sense to drop the fuzz target here and focus on the unit tests' Settled: 2026-04-10: 'Pruned the fuzz harness added here in favour of the one in #34651' |
| w0xlt | style | tests could silently return early instead of failing loudly if test setup failed | resolved | no | 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.' Settled: 2026-06-05: 'took the second approach, it's more DRY' followed by force push making AddCoin assert directly |
Support:
- w0xlt: ACK c9d6eb5d9d1db6d6c0ffc9096932d40b121b2875 after earlier Concept ACK
- sedited: Approach ACK, noting that having all these test cases is valuable
- marcofleon: Praised the unit tests as well-written
Participants: stratospher (question), Bortlesboat (objection), marcofleon (objection), w0xlt (support), sedited (support)
State derived from the lists: substantive support, no open objection (w0xlt, sedited, marcofleon)
Review verdicts (DrahtBot): 0 (+1)
Dependencies
Enables:
- De-risks validation modularization for libbitcoinkernel (#32317)
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
Uncertainties
- The PR tests consensus and block validation rules, but because it touches only src/test/ and carries only the Tests label, it is assessed under tests rather than validation per the scoping rules.
Card
This PR adds four unit test suites exercising block header, stateless, contextual, and stateful consensus rules across TestBlockValidity, ProcessNewBlock, and ConnectBlock. It provides structured test coverage for specific consensus rejection reasons, directly motivated by subtle validation issues encountered during kernel modularization work (#32317). The PR enjoys strong reviewer support with an Approach ACK from sedited and an ACK from w0xlt. Reviewability is technically ready with all comments resolved, though the author has been silent for 103 days since the last push.