validation: Ensure Invalid ValidationState has result #36318

pull optout21 wants to merge 4 commits into bitcoin:master from optout21:2609-invalid-must-have-reason changing 10 files +27 −11

optout21 commented at 11:51 AM on September 23, 2026: contributor

Block and transaction validation results are represented in ValidationState, and invalid results in "Invalid" states. Invalid validation results have a result, an enum value, and a reject_reason, a structured string.

The change is to ensure that an invalid result always has a set result and reject reason. This increases code robustness/security.

One instance was identified where the result was not set, and is being fixed by a new BLOCK_INCONCLUSIVE result. PR #31981 moved a previous-block check from outside of TestBlockValidity() to inside it, and introduced a new usage of BlockValidationState with unset result. The result field is not checked outside. This instance is changed now to use the newly-introduced BLOCK_INCONCLUSIVE value.

The new enum value is added to the kernel C API and its C++ wrapper. In P2P, a BLOCK_INCONCLUSIVE result does not lead to punishing the peer.

A few test-only instances with empty reject reason are changed to use a dummy value ("dummy-reason").

History: this was found on the side of #35570, but left untouched there; see #35570 (comment) and #35570 (review).

validation: Add BlockValidationResult::BLOCK_INCONCLUSIVE (unused)

The goal is to ensure that an Invalid `ValidationState` always has a set result.
However, there is a violating usage in `TestBlockValidity`.
As a preparation to address that, a new value is added to the `BlockValidationResult`
enum, `BLOCK_INCONCLUSIVE`, representing a result where validity could not be
determined, e.g. due to some missing data (previous block).
A corresponding value is added to the kernel C API
(btck_BlockValidationResult_INCONCLUSIVE) and its C++ wrapper.
In P2P, a `BLOCK_INCONCLUSIVE` result does not lead to punishing the peer.
The new value is used in a later commit.

34c6f9ff71

validation: In TestBlockValidity, use INCONCLUSIVE

The goal is to ensure that an Invalid ValidationState always has a set result.
A violating usage in `TestBlockValidity` is being changed: instead of the
default `BLOCK_RESULT_UNSET`, it now uses `BLOCK_INCONCLUSIVE`.
The result field from `TestBlockValidity` is currently not inspected by callers,
but the constraint "an Invalid state must have a set result"
is satisfied this way.

ee04f82075

validation: Ensure that Invalid ValidationState always has a result

Ensure that an Invalid `ValidationState`, constructed through `Invalid()`,
always has a set result field, by using an `Assume`.
This increases code robustness/security.

db793b6ef2

validation: Ensure Invalid ValidationState has non-empty reject_reason

In addition to ensuring a set result, also ensure that `reject_reason`
is not empty for an Invalid `ValidationState`.
Also remove its empty default value, as it should never be empty.
Some test-only usages, where empty reject reason was used, are changed
to use a dummy value.

22cf9d54b3

DrahtBot added the label Validation on Sep 23, 2026
DrahtBot commented at 11:51 AM on September 23, 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/36318.



Reviews

See the guideline and AI policy for information on the review process. A summary of reviews will appear here.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #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)
- #29700 (kernel, refactor: return error status on all fatal errors 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.
optout21 commented at 11:54 AM on September 23, 2026: contributor

CC: @Sjors (author of #31981), @hodlinator (reviewer of #35570).
hodlinator commented at 12:20 PM on September 23, 2026: contributor

From what I deduce the defaulted result enum was suggested in the diff posted by @ryanofsky in #31981 (review), so he might also be interested in this.
optout21 commented at 4:00 PM on September 23, 2026: contributor

Undrafting upon clean CI
optout21 marked this as ready for review on Sep 23, 2026

Contributors

Labels

Linked (view graph)

#31981 Add checkBlock() to Mining interface #35570 refactor: Change some validation.cpp methods to return BlockValidationState

validation: Ensure Invalid ValidationState has result #36318

Code Coverage & Benchmarks

Reviews

Conflicts