kernel, validation: Refactor ProcessNewBlockHeaders to return BlockValidationState #33856

yuvicc · 2025-11-11T17:23:59Z

Motivation

This PR refactors ProcessNewBlockHeaders() to return BlockValidationState by value instead of using out-parameters or boolean returns. This follows the pattern established in #31981 (commit 74690f4) which refactored TestBlockValidity() in a similar manner.

Current Issues

ProcessNewBlockHeaders(): Uses an out-parameter BlockValidationState& state, making the API less intuitive.

As noted by @TheCharlatan in #33822 comment and can be a fix for that too:

One thing that could be considered here is returning the BlockValidationState directly instead of having an in/out param. To safely do that I think we'd need to refactor ProcessNewBlockHeaders though, similarly to what was done in 74690f4.

The changes are split into two commits, see individual commit message for more info.

DrahtBot · 2025-11-11T17:24:10Z

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/33856.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	w0xlt
Concept ACK	hodlinator, exp3rimenter
Stale ACK	danielabrozzoni

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33822 (kernel: Add block header support and validation by yuvicc)
#32740 (refactor: Header sync optimisations & simplifications by danielabrozzoni)

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.

hodlinator

Concept ACK moving out-reference-parameters to return value

Not familiar with the kernel API but had a brief look at net_processing and validation.

src/validation.cpp

w0xlt · 2025-11-13T23:18:14Z

Concept ACK

DrahtBot · 2025-11-16T07:24:42Z

🚧 At least one of the CI tasks failed.
_{Task Linux->Windows cross, no tests: https://github.com/bitcoin/bitcoin/actions/runs/19402124289/job/55511133875}
_{LLM reason (✨ experimental): Compilation failed due to a syntax error in validation.cpp (missing semicolon before BlockValidationState in ProcessNewBlockHeaders).}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

yuvicc · 2025-11-16T07:32:41Z

Thanks for the review @hodlinator

Added Assume(!headers.empty()) before instantiating validation state as suggested in comment
Removed shadow state comment

src/test/blockfilter_index_tests.cpp

exp3rimenter · 2025-11-19T14:50:53Z

Concept ACK on refactoring bool to return state.

w0xlt

This PR eliminates the ambiguity of the boolean return value and improves the clarity of both the internal and kernel APIs.

ACK 1f589e1 (with the above-mentioned nit)

yuvicc · 2025-11-23T06:03:24Z

Thanks for the review @w0xlt

Addressed the suggestion and added as co-author

src/validation.cpp

yuvicc · 2025-11-25T14:28:16Z

Thanks for the review @TheCharlatan.

Addressed comment on using post-condition checks.
Dropped the change to use separate state for reporting errors and not invalidity as suggested by @TheCharlatan comment

src/validation.cpp

yuvicc · 2025-11-27T11:27:12Z

I agree with @sedited comment, the belt-and-suspenders check in ActivateBestChain in validation.cpp is broken by this change. When m_disabled is true, ActivateBestChain returns false without setting the BlockValidationState to invalid. This causes the new NONFATAL_UNREACHABLE() assert in ProcessNewBlock to trigger incorrectly. The belt-and-suspenders check exists precisely because we can't guarantee ActivateBestChain won't be called on a disabled chainstate.

So dropping the change in ProcessNewBlock and only keeping ProcessNewBlockHeaders now.

danielabrozzoni

light ACK f31f7c21ff7093a4c90199cd0bb2128d19bf2d33

Code looks good to me, and the interface is cleaner now. I'm only light-acking sicne I'm not familiar with the kernel :)

As I pointed out in a comment, I don't think we needed the Assume(!headers.empty()) at the start of ProcessNewBlockHeaders, but I'm ok with keeping it.

src/validation.cpp

Add C API functions for managing BlockValidationState lifecycle: - btck_block_validation_state_create() - btck_block_validation_state_copy() - btck_block_validation_state_destroy() Introduce BlockValidationStateApi<> template to share common getter methods between BlockValidationState (Handle) and BlockValidationStateView (View) classes in the C++ wrapper. Update ValidationInterface::BlockChecked to use BlockValidationStateView since it doesn't need ownership. This changes prepares the kernel API to return BlockValidationState by value in subsequent commits.

Return BlockValidationState by value instead of using an out-parameter, similar to the TestBlockValidity refactoring in 74690f4. This provides a cleaner API and enables callers to inspect detailed validation state without relying on side effects through reference parameters.

yuvicc · 2025-12-10T04:58:27Z

Rebased f31f7c2 -> be379fd.

w0xlt

reACK be379fd

yuvicc mentioned this pull request Nov 11, 2025

kernel: Add block header support and validation #33822

Open

hodlinator reviewed Nov 12, 2025

View reviewed changes

src/validation.cpp Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Show resolved Hide resolved

yuvicc force-pushed the 2025-11-refractor_bool_to_blockvalidationstate branch 2 times, most recently from 6cdba25 to 1f589e1 Compare November 16, 2025 07:23

DrahtBot added the CI failed label Nov 16, 2025

yuvicc mentioned this pull request Nov 16, 2025

interface_ipc functional test failing in CI #33884

Closed

DrahtBot removed the CI failed label Nov 17, 2025

yancyribbens reviewed Nov 18, 2025

View reviewed changes

src/test/blockfilter_index_tests.cpp Show resolved Hide resolved

w0xlt reviewed Nov 20, 2025

View reviewed changes

DrahtBot requested a review from hodlinator November 20, 2025 19:23

DrahtBot mentioned this pull request Nov 21, 2025

kernel: add context‑free block validation API (btck_check_block_context_free) with POW/Merkle flags #33908

Open

yuvicc force-pushed the 2025-11-refractor_bool_to_blockvalidationstate branch from 1f589e1 to 8d97193 Compare November 23, 2025 06:01

sedited reviewed Nov 23, 2025

View reviewed changes

src/validation.cpp Show resolved Hide resolved

sedited reviewed Nov 23, 2025

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Nov 24, 2025

refactor: Improve assumeutxo state representation #30214

Merged

yuvicc force-pushed the 2025-11-refractor_bool_to_blockvalidationstate branch from 8d97193 to 857ebdd Compare November 25, 2025 14:23

janb84 mentioned this pull request Nov 25, 2025

kernel: don't use assert to handle invalid user input #33943

Draft

sedited reviewed Nov 26, 2025

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

yuvicc force-pushed the 2025-11-refractor_bool_to_blockvalidationstate branch from 857ebdd to f31f7c2 Compare November 27, 2025 11:27

yuvicc changed the title ~~kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState~~ kernel, validation: Refactor ProcessNewBlockHeaders to return BlockValidationState Nov 27, 2025

DrahtBot mentioned this pull request Dec 7, 2025

validation: Remove min_pow_checked arg in ProcessNewBlockHeaders #34022

Closed

danielabrozzoni reviewed Dec 8, 2025

View reviewed changes

src/validation.cpp Show resolved Hide resolved

DrahtBot requested a review from w0xlt December 8, 2025 10:31

DrahtBot added the Needs rebase label Dec 9, 2025

yuvicc added 2 commits December 10, 2025 08:35

yuvicc force-pushed the 2025-11-refractor_bool_to_blockvalidationstate branch from f31f7c2 to be379fd Compare December 10, 2025 04:55

DrahtBot removed the Needs rebase label Dec 10, 2025

w0xlt reviewed Dec 11, 2025

View reviewed changes

DrahtBot requested a review from danielabrozzoni December 11, 2025 00:33

kernel, validation: Refactor ProcessNewBlockHeaders to return BlockValidationState #33856

Are you sure you want to change the base?

kernel, validation: Refactor ProcessNewBlockHeaders to return BlockValidationState #33856

Uh oh!

Conversation

yuvicc commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Current Issues

Uh oh!

DrahtBot commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

w0xlt commented Nov 13, 2025

Uh oh!

DrahtBot commented Nov 16, 2025

Uh oh!

yuvicc commented Nov 16, 2025

Uh oh!

Uh oh!

exp3rimenter commented Nov 19, 2025

Uh oh!

w0xlt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuvicc commented Nov 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yuvicc commented Nov 25, 2025

Uh oh!

Uh oh!

yuvicc commented Nov 27, 2025

Uh oh!

danielabrozzoni left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yuvicc commented Dec 10, 2025

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

yuvicc commented Nov 11, 2025 •

edited

Loading

DrahtBot commented Nov 11, 2025 •

edited

Loading

w0xlt left a comment •

edited

Loading

yuvicc commented Nov 23, 2025 •

edited

Loading