kernel: Refactor process_block_header to return btck

yuvicc commented at 5:23 PM on November 11, 2025: contributor

This PR refactors btck_chainstate_manager_process_block_header to return btck_BlockValidationState by value instead of using out-parameters or boolean returns.

DrahtBot commented at 5:24 PM on November 11, 2025: 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/33856.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	optout21, stickies-v, w0xlt, hodlinator
Concept ACK	exp3rimenter
Stale ACK	danielabrozzoni, stringintech

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35189 (kernel: reset header validation state before processing by w0xlt)

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.

in src/validation.cpp:4549 in 4c6301c01c outdated

4547 | +        return state;
4548 |      }
4549 |  
4550 |      Chainstate* bg_chain{WITH_LOCK(cs_main, return BackgroundSyncInProgress() ? m_ibd_chainstate.get() : nullptr)};
4551 |      BlockValidationState bg_state;
4552 |      if (bg_chain && !bg_chain->ActivateBestChain(bg_state, block)) {

hodlinator commented at 8:13 AM on November 12, 2025:

Why not reuse state here?

yuvicc commented at 6:44 AM on November 16, 2025:

The problem is that ActivateBestChain would overwrite the state result, when the background chain's ActivateBestChain succeeds, it would overwrite state with its own state. Then at line 4554, we'd be returning the background chain's state instead of the active chainstate's state. So by separating out, we preserve the active chainstate result.

hodlinator commented at 1:21 PM on November 17, 2025:

But if state contained anything interesting, wouldn't we already have returned it before reaching this block? Callers of the function only see either bg_state or state as this PR stands anyway.

Thanks for taking my other suggestions!

yuvicc commented at 10:53 AM on November 19, 2025:

Correct. Good observation. So at this stage the state already represents success. If the background chain fails, overwriting it with the failure state is fine - that's what you want to return anyway. If the background chain succeeds, state gets overwritten with success - also fine. The only argument for keeping bg_state separate would be if you wanted clearer variable naming to show intent, but functionally reusing state works fine here.

w0xlt commented at 6:47 PM on November 20, 2025:

nit: Maybe the variables could be renamed in this PR for better clarity. The “description” rows below can be converted into comments to document the variables.

Current name	Suggestion	Description
`state`	`accept_state`	determines if the block data is valid enough to be written to the disk and entered into the block index.
`bg_state`	`activation_state`	attempts to connect the block to the tip of the active chain (executing scripts and updating the UTXO set).

yuvicc commented at 4:53 AM on November 23, 2025:

Makes sense. Thanks.

in src/validation.cpp:4536 in 4c6301c01c outdated

4532 | @@ -4533,26 +4533,25 @@ bool ChainstateManager::ProcessNewBlock(const std::shared_ptr<const CBlock>& blo
4533 |                  m_options.signals->BlockChecked(block, state);
4534 |              }
4535 |              LogError("%s: AcceptBlock FAILED (%s)\n", __func__, state.ToString());
4536 | -            return false;
4537 | +            return state;

hodlinator commented at 8:14 AM on November 12, 2025:

Should remove the shadowing state on line 4515 IMO.

in src/validation.cpp:4355 in b9d4ea94a0 outdated

4348 | @@ -4349,9 +4349,10 @@ bool ChainstateManager::AcceptBlockHeader(const CBlockHeader& block, BlockValida
4349 |  }
4350 |  
4351 |  // Exposed wrapper for AcceptBlockHeader
4352 | -bool ChainstateManager::ProcessNewBlockHeaders(std::span<const CBlockHeader> headers, bool min_pow_checked, BlockValidationState& state, const CBlockIndex** ppindex)
4353 | +BlockValidationState ChainstateManager::ProcessNewBlockHeaders(std::span<const CBlockHeader> headers, bool min_pow_checked, const CBlockIndex** ppindex)
4354 |  {
4355 |      AssertLockNotHeld(cs_main);
4356 | +    BlockValidationState state;

hodlinator commented at 8:22 AM on November 12, 2025:

Worth adding an Assume(!headers.empty()) before this, as state will be unset if we don't process any, whereas before we would return true regardless?

yuvicc commented at 6:59 AM on November 16, 2025:

Correct, we will definitely need that.

danielabrozzoni commented at 4:55 PM on December 1, 2025:

I'm ok with adding the Assume, but I don't think it would have been a problem, as BlockValidationState is initialized to valid: https://github.com/bitcoin/bitcoin/blob/6356041e58d1ba86695e2e7c219c68ee5abe583f/src/consensus/validation.h#L82

I tested locally with:

diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp
index 9973b33b57..f9080ef40f 100644
--- a/src/test/validation_block_tests.cpp
+++ b/src/test/validation_block_tests.cpp
@@ -363,4 +363,9 @@ BOOST_AUTO_TEST_CASE(witness_commitment_index)
 
     BOOST_CHECK_EQUAL(GetWitnessCommitmentIndex(pblock), 2);
 }
+
+BOOST_AUTO_TEST_CASE(test_empty_process_new_block_headers) {
+    auto res = m_node.chainman->ProcessNewBlockHeaders({}, true);
+    BOOST_CHECK(res.IsValid());
+}
 BOOST_AUTO_TEST_SUITE_END()

hodlinator commented at 1:23 PM on March 25, 2026:

Looking at it a bit closer I prefer removing Assume() and possibly adding @danielabrozzoni's test. Sorry for flip-flopping, feel more comfortable with default-initialized BlockValidationState now.

hodlinator commented at 8:32 AM on November 12, 2025: contributor

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.

w0xlt commented at 11:18 PM on November 13, 2025: contributor

Concept ACK

yuvicc force-pushed on Nov 16, 2025

DrahtBot added the label CI failed on Nov 16, 2025

DrahtBot commented at 7:24 AM on November 16, 2025: contributor

🚧 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).

<details><summary>Hints</summary>

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.

</details>

yuvicc commented at 7:32 AM on November 16, 2025: contributor

Thanks for the review @hodlinator

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

DrahtBot removed the label CI failed on Nov 17, 2025

in src/test/blockfilter_index_tests.cpp:107 in 1f589e1af1 outdated

 103 | @@ -104,8 +104,7 @@ bool BuildChainTestingSetup::BuildChain(const CBlockIndex* pindex,
 104 |          block = std::make_shared<CBlock>(CreateBlock(pindex, no_txns, coinbase_script_pub_key));
 105 |          CBlockHeader header = block->GetBlockHeader();
 106 |  
 107 | -        BlockValidationState state;
 108 | -        if (!Assert(m_node.chainman)->ProcessNewBlockHeaders({{header}}, true, state, &pindex)) {
 109 | +        if (BlockValidationState state{Assert(m_node.chainman)->ProcessNewBlockHeaders({{header}}, true, &pindex)}; !state.IsValid()) {

yancyribbens commented at 12:03 AM on November 18, 2025:

in 1f589e1af162c2c2705f0404496c549785f2f545 could !state.IsValud() be state.IsInvalid()?

yuvicc commented at 11:00 AM on November 19, 2025:

No we cannot as:

    enum class ModeState {
        M_VALID,   //!< everything ok
        M_INVALID, //!< network rule violation (DoS value may be set)
        M_ERROR,   //!< run-time error
    }

!state.IsValid() returns true when the state is either M_INVALID or M_ERROR state.IsInvalid() returns true only when the state is M_INVALID, misses the M_ERROR or run-time error case.

exp3rimenter commented at 2:50 PM on November 19, 2025: none

Concept ACK on refactoring bool to return state.

w0xlt commented at 7:23 PM on November 20, 2025: contributor

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

ACK https://github.com/bitcoin/bitcoin/pull/33856/commits/1f589e1af162c2c2705f0404496c549785f2f545 (with the above-mentioned nit)

DrahtBot requested review from hodlinator on Nov 20, 2025

yuvicc force-pushed on Nov 23, 2025

yuvicc commented at 6:03 AM on November 23, 2025: contributor

Thanks for the review @w0xlt

Addressed the suggestion and added as co-author

in src/validation.cpp:4363 in 8d971934f9 outdated

4361 | @@ -4360,7 +4362,7 @@ bool ChainstateManager::ProcessNewBlockHeaders(std::span<const CBlockHeader> hea
4362 |              CheckBlockIndex();
4363 |  
4364 |              if (!accepted) {
4365 | -                return false;

sedited commented at 1:35 PM on November 23, 2025:

We should include post-condition checks here. For the TestBlockValidity refactor, we added

if (state.IsValid()) NONFATAL_UNREACHABLE();

I think such a check should be added wherever a boolean was returned previously.

in src/validation.cpp:4510 in 8d971934f9 outdated

4506 | @@ -4505,14 +4507,15 @@ bool ChainstateManager::AcceptBlock(const std::shared_ptr<const CBlock>& pblock,
4507 |      return true;
4508 |  }
4509 |  
4510 | -bool ChainstateManager::ProcessNewBlock(const std::shared_ptr<const CBlock>& block, bool force_processing, bool min_pow_checked, bool* new_block)
4511 | +BlockValidationState ChainstateManager::ProcessNewBlock(const std::shared_ptr<const CBlock>& block, bool force_processing, bool min_pow_checked, bool* new_block)

sedited commented at 1:47 PM on November 23, 2025:

I don't think this change is correct. Previously false was returned when the block failed to be accepted or on a fatal error condition, not when it failed to validate. If you look into ActivateBestChainStep, you'll see that we break on a validation failure, reset the state again and don't return false. Can you drop this change again?

yuvicc force-pushed on Nov 25, 2025

yuvicc commented at 2:28 PM on November 25, 2025: contributor

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

in src/validation.cpp:4560 in 857ebdd20c

4562 | -        return false;
4563 | -     }
4564 |  
4565 | -    return true;
4566 | +    // Attempts to connect the block to the tip of the active chain.
4567 | +    BlockValidationState activation_state;

sedited commented at 9:13 PM on November 26, 2025:

I'm confused by this change. Why are you introducing this variable? The comment is also wrong, since this processes the block on the background chain. I would still prefer if the behaviour of ProcessNewBlock would not be changed as part of this patch set.

yuvicc commented at 3:09 AM on November 27, 2025:

I don't think the behavior of ProcessNewBlock is changed here. It's just a change in variable name (bg_state -> activation_state) see comment, we can keep the change as is if you prefer that way?

sedited commented at 10:23 AM on November 27, 2025:

The comment you linked seems misleading. bg_state is the correct name in my view. We are not operating on the active chain here.

I think the belt-and-suspenders check in ActivateBestChain is broken by this change. When we return false in the case of the chain being disabled, we now run into NONFATAL_UNREACHABLE. It might be true that there are no similar cases in the call graph of ActivateBestChain, but can we guarantee that? For this reason, I think the commit should be dropped.

yuvicc force-pushed on Nov 27, 2025

yuvicc commented at 11:27 AM on November 27, 2025: contributor

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.

yuvicc renamed this:
~~kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState~~
kernel, validation: Refactor ProcessNewBlockHeaders to return BlockValidationState
on Nov 27, 2025

danielabrozzoni commented at 10:31 AM on December 8, 2025: member

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.

DrahtBot requested review from w0xlt on Dec 8, 2025

DrahtBot added the label Needs rebase on Dec 9, 2025

yuvicc force-pushed on Dec 10, 2025

yuvicc commented at 4:58 AM on December 10, 2025: contributor

Rebased f31f7c2 -> be379fd.

DrahtBot removed the label Needs rebase on Dec 10, 2025

w0xlt commented at 12:33 AM on December 11, 2025: contributor

reACK https://github.com/bitcoin/bitcoin/pull/33856/commits/be379fd52b82805ab99400457ea44f93430f16e2

DrahtBot requested review from danielabrozzoni on Dec 11, 2025

DrahtBot added the label Needs rebase on Jan 14, 2026

yuvicc force-pushed on Jan 15, 2026

yuvicc commented at 6:04 PM on January 15, 2026: contributor

Rebased to master

DrahtBot added the label CI failed on Jan 15, 2026

DrahtBot commented at 7:14 PM on January 15, 2026: contributor

🚧 At least one of the CI tasks failed. Task macOS-cross to x86_64: https://github.com/bitcoin/bitcoin/actions/runs/21041288656/job/60503831243 LLM reason (✨ experimental): Test build failed due to a mismatched API call: ProcessNewBlockHeaders is invoked with four args, but its signature accepts three (causing compilation errors in blockfilter_index_tests.cpp).

<details><summary>Hints</summary>

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.

</details>

DrahtBot removed the label Needs rebase on Jan 15, 2026

yuvicc force-pushed on Jan 18, 2026

DrahtBot removed the label CI failed on Jan 18, 2026

DrahtBot added the label Needs rebase on Jan 23, 2026

stringintech commented at 3:13 PM on February 4, 2026: contributor

@yuvicc I'd be happy to review once rebased.

in src/validation.h:1247 in cb5b22214e outdated

1245 | +     * @returns BlockValidationState indicating the result. IsValid() returns true if all headers
1246 | +     *          were accepted. On failure, IsInvalid() is true and the state contains the specific
1247 | +     *          validation failure reason.
1248 |       */
1249 | -    bool ProcessNewBlockHeaders(std::span<const CBlockHeader> headers, bool min_pow_checked, BlockValidationState& state, const CBlockIndex** ppindex = nullptr) LOCKS_EXCLUDED(cs_main);
1250 | +    BlockValidationState ProcessNewBlockHeaders(std::span<const CBlockHeader> headers, bool min_pow_checked, const CBlockIndex** ppindex = nullptr) LOCKS_EXCLUDED(cs_main);

stickies-v commented at 5:45 PM on February 4, 2026:

IIUC, we only care about state when something goes wrong, and we should only care about ppindex when all headers are processed. So I think a util::Expected<CBlockIndex*, BlockValidationState> return type fits really well here. It cleans up both out-parameters, prevents accessing ppindex when not all headers have been processed, and hides state when we don't care about it.

Example diff (didn't review carefully):

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index f2768efe19..3f0f328840 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2960,8 +2960,6 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
         return;
     }
 
-    const CBlockIndex *pindexLast = nullptr;
-
     // We'll set already_validated_work to true if these headers are
     // successfully processed as part of a low-work headers sync in progress
     // (either in PRESYNC or REDOWNLOAD phase).
@@ -3046,11 +3044,9 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
     // something new (if these headers are valid).
     bool received_new_header{last_received_header == nullptr};
 
-    // Now process all the headers and return the block validation state.
-    BlockValidationState state{m_chainman.ProcessNewBlockHeaders(headers,
-                                                           /*min_pow_checked=*/true,
-                                                           &pindexLast)};
-    if (state.IsInvalid()) {
+    auto headers_processed{m_chainman.ProcessNewBlockHeaders(headers, /*min_pow_checked=*/true)};
+    if (!headers_processed) {
+        const auto& state{headers_processed.error()};
         if (!pfrom.IsInboundConn() && state.GetResult() == BlockValidationResult::BLOCK_CACHED_INVALID) {
             // Warn user if outgoing peers send us headers of blocks that we previously marked as invalid.
             LogWarning("%s (received from peer=%i). "
@@ -3061,9 +3057,10 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
         MaybePunishNodeForBlock(pfrom.GetId(), state, via_compact_block, "invalid header received");
         return;
     }
+    const CBlockIndex* pindexLast{headers_processed.value()};
     assert(pindexLast);
 
-    if (state.IsValid() && received_new_header) {
+    if (received_new_header) {
         LogBlockHeader(*pindexLast, pfrom, /*via_compact_block=*/false);
     }
 
@@ -4554,15 +4551,14 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
         }
         }
 
-        const CBlockIndex *pindex = nullptr;
-        BlockValidationState state{m_chainman.ProcessNewBlockHeaders({{cmpctblock.header}}, /*min_pow_checked=*/true, &pindex)};
-        if (state.IsInvalid()) {
-            MaybePunishNodeForBlock(pfrom.GetId(), state, /*via_compact_block=*/true, "invalid header via cmpctblock");
+        auto headers_processed{m_chainman.ProcessNewBlockHeaders({{cmpctblock.header}}, /*min_pow_checked=*/true)};
+        if (!headers_processed) {
+            MaybePunishNodeForBlock(pfrom.GetId(), headers_processed.error(), /*via_compact_block=*/true, "invalid header via cmpctblock");
             return;
         }
 
         // If AcceptBlockHeader returned true, it set pindex
-        Assert(pindex);
+        const CBlockIndex* pindex{Assert(headers_processed.value())};
         if (received_new_header) {
             LogBlockHeader(*pindex, pfrom, /*via_compact_block=*/true);
         }
diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
index e979ccc9a4..fe3c827d15 100644
--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -1123,12 +1123,15 @@ static RPCHelpMan submitheader()
         }
     }
 
-    BlockValidationState state{chainman.ProcessNewBlockHeaders({{h}}, /*min_pow_checked=*/true)};
-    if (state.IsValid()) return UniValue::VNULL;
-    if (state.IsError()) {
-        throw JSONRPCError(RPC_VERIFY_ERROR, state.ToString());
+    if (auto headers_processed{chainman.ProcessNewBlockHeaders({{h}}, /*min_pow_checked=*/true)}) {
+        return UniValue::VNULL;
+    } else {
+        const auto& state{headers_processed.error()};
+        if (state.IsError()) {
+            throw JSONRPCError(RPC_VERIFY_ERROR, state.ToString());
+        }
+        throw JSONRPCError(RPC_VERIFY_ERROR, state.GetRejectReason());
     }
-    throw JSONRPCError(RPC_VERIFY_ERROR, state.GetRejectReason());
 },
     };
 }
diff --git a/src/test/blockfilter_index_tests.cpp b/src/test/blockfilter_index_tests.cpp
index 0fef42eb88..3ec5f35258 100644
--- a/src/test/blockfilter_index_tests.cpp
+++ b/src/test/blockfilter_index_tests.cpp
@@ -103,7 +103,9 @@ bool BuildChainTestingSetup::BuildChain(const CBlockIndex* pindex,
     for (auto& block : chain) {
         block = std::make_shared<CBlock>(CreateBlock(pindex, no_txns, coinbase_script_pub_key));
 
-        if (!Assert(m_node.chainman)->ProcessNewBlockHeaders({{*block}}, true, &pindex).IsValid()) {
+        if (auto headers_processed{Assert(m_node.chainman)->ProcessNewBlockHeaders({{*block}}, true)}) {
+            pindex = *headers_processed;
+        } else {
             return false;
         }
     }
diff --git a/src/test/fuzz/utxo_snapshot.cpp b/src/test/fuzz/utxo_snapshot.cpp
index 197ae9c871..db4b0165f2 100644
--- a/src/test/fuzz/utxo_snapshot.cpp
+++ b/src/test/fuzz/utxo_snapshot.cpp
@@ -88,8 +88,7 @@ void initialize_chain()
     if constexpr (INVALID) {
         auto& chainman{*setup->m_node.chainman};
         for (const auto& block : chain) {
-            auto result{chainman.ProcessNewBlockHeaders({{*block}}, true)};
-            Assert(result.IsValid());
+            Assert(chainman.ProcessNewBlockHeaders({{*block}}, true));
             const auto* index{WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockIndex(block->GetHash()))};
             Assert(index);
         }
@@ -169,8 +168,7 @@ void utxo_snapshot_fuzz(FuzzBufferType buffer)
         // Consume the bool, but skip the code for the INVALID fuzz target
         if constexpr (!INVALID) {
             for (const auto& block : *g_chain) {
-                auto result{chainman.ProcessNewBlockHeaders({{*block}}, true)};
-                Assert(result.IsValid());
+                Assert(chainman.ProcessNewBlockHeaders({{*block}}, true));
                 const auto* index{WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockIndex(block->GetHash()))};
                 Assert(index);
             }
diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp
index 85ed77a9ab..ca2624859f 100644
--- a/src/test/validation_block_tests.cpp
+++ b/src/test/validation_block_tests.cpp
@@ -104,7 +104,7 @@ std::shared_ptr<CBlock> MinerTestingSetup::FinalizeBlock(std::shared_ptr<CBlock>
 
     // submit block header, so that miner can get the block height from the
     // global state and the node has the topology of the chain
-    BOOST_CHECK(Assert(m_node.chainman)->ProcessNewBlockHeaders({{*pblock}}, true).IsValid());
+    BOOST_CHECK(Assert(m_node.chainman)->ProcessNewBlockHeaders({{*pblock}}, true));
 
     return pblock;
 }
diff --git a/src/validation.cpp b/src/validation.cpp
index b323b5c06e..4c123c1cc7 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -4300,31 +4300,27 @@ bool ChainstateManager::AcceptBlockHeader(const CBlockHeader& block, BlockValida
 }
 
 // Exposed wrapper for AcceptBlockHeader
-BlockValidationState ChainstateManager::ProcessNewBlockHeaders(std::span<const CBlockHeader> headers, bool min_pow_checked, const CBlockIndex** ppindex)
+util::Expected<CBlockIndex*, BlockValidationState> ChainstateManager::ProcessNewBlockHeaders(std::span<const CBlockHeader> headers, bool min_pow_checked)
 {
     AssertLockNotHeld(cs_main);
     Assume(!headers.empty());
     BlockValidationState state;
+    CBlockIndex* pindex{nullptr};
     {
         LOCK(cs_main);
         for (const CBlockHeader& header : headers) {
-            CBlockIndex *pindex = nullptr; // Use a temp pindex instead of ppindex to avoid a const_cast
             bool accepted{AcceptBlockHeader(header, state, &pindex, min_pow_checked)};
             CheckBlockIndex();
 
             if (!accepted) {
                 if (state.IsValid()) NONFATAL_UNREACHABLE();
-                return state;
-            }
-
-            if (ppindex) {
-                *ppindex = pindex;
+                return util::Unexpected{state};
             }
         }
     }
     if (NotifyHeaderTip()) {
-        if (IsInitialBlockDownload() && ppindex && *ppindex) {
-            const CBlockIndex& last_accepted{**ppindex};
+        if (IsInitialBlockDownload() && pindex) {
+            const CBlockIndex& last_accepted{*pindex};
             int64_t blocks_left{(NodeClock::now() - last_accepted.Time()) / GetConsensus().PowTargetSpacing()};
             blocks_left = std::max<int64_t>(0, blocks_left);
             const double progress{100.0 * last_accepted.nHeight / (last_accepted.nHeight + blocks_left)};
@@ -4333,7 +4329,7 @@ BlockValidationState ChainstateManager::ProcessNewBlockHeaders(std::span<const C
     }
 
     if (!state.IsValid()) NONFATAL_UNREACHABLE();
-    return state;
+    return pindex;
 }
 
 void ChainstateManager::ReportHeadersPresync(int64_t height, int64_t timestamp)
diff --git a/src/validation.h b/src/validation.h
index 85f252f6ab..0d755a40d6 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -1239,12 +1239,10 @@ public:
      *
      * [@param](/bitcoin-bitcoin/contributor/param/)[in]  headers The block headers themselves
      * [@param](/bitcoin-bitcoin/contributor/param/)[in]  min_pow_checked  True if proof-of-work anti-DoS checks have been done by caller for headers chain
-     * [@param](/bitcoin-bitcoin/contributor/param/)[out] ppindex If set, the pointer will be set to point to the last new block index object for the given headers
-     * [@returns](/bitcoin-bitcoin/contributor/returns/) BlockValidationState indicating the result. IsValid() returns true if all headers
-     *          were accepted. On failure, IsInvalid() is true and the state contains the specific
-     *          validation failure reason.
+     * [@returns](/bitcoin-bitcoin/contributor/returns/) The last new block index object for the given headers if all headers were accepted.
+     *          Otherwise, BlockValidationState containing the validation failure reason.
      */
-    BlockValidationState ProcessNewBlockHeaders(std::span<const CBlockHeader> headers, bool min_pow_checked, const CBlockIndex** ppindex = nullptr) LOCKS_EXCLUDED(cs_main);
+    util::Expected<CBlockIndex*, BlockValidationState> ProcessNewBlockHeaders(std::span<const CBlockHeader> headers, bool min_pow_checked) LOCKS_EXCLUDED(cs_main);
 
     /**
      * Sufficiently validate a block for disk storage (and store on disk).

</details>

stringintech commented at 2:03 PM on February 6, 2026:

I'm not sure, but this might make some use cases awkward. BlockValidationState has three modes (M_VALID, M_INVALID, M_ERROR) representing the full validation outcome, unlike simpler error types in other util::Expected uses (std::string, ReadRawError). With Expected, we might end up with code like result.error().IsError() - inspecting an "error" to see if it's actually an error vs invalid.
Also if we want the C API (btck_chainstate_manager_process_block_header) to return BlockValidationState* (null on error), we'd need to manually create a "success state" when using Expected.

stickies-v commented at 11:56 AM on February 9, 2026:

With Expected, we might end up with code like result.error().IsError() - inspecting an "error" to see if it's actually an error vs invalid.

Yeah, that's awkward. I'm not sure how enthusiastic I am about BlockValidationState representing both validation states and runtime errors, but I'll need to look into it more.

Also if we want the C API (btck_chainstate_manager_process_block_header) to return BlockValidationState* (null on error), we'd need to manually create a "success state" when using Expected.

I think btck_chainstate_manager_process_block_header returning a btck_BlockTreeEntry* (null if header could not be added to the block tree) makes for a much more natural and chainable API.

yuvicc commented at 12:49 PM on February 9, 2026:

Maybe we can also return state as a param for callers who need any failure reason and returning btck_BlockTreeEntry as the natural return for chaining. I am not sure here.

I think btck_chainstate_manager_process_block_header returning a btck_BlockTreeEntry* (null if header could not be added to the block tree) makes for a much more natural and chainable API.

hodlinator commented at 8:50 AM on March 26, 2026:

Experimented with making @stickies-v diff compile and pass the kernel test, but I'm still getting used to the kernel interface so I'll spare you the diff. :grimacing:

Would be nice to switch to util::Expected long-term and probably remove ValidationState::M_VALID altogether, probably not tactically correct to expand this PR into that.

stringintech commented at 8:44 PM on April 11, 2026:

I'm not sure how enthusiastic I am about BlockValidationState representing both validation states and runtime errors, but I'll need to look into it more.

Yeah... I'm not sure why runtime errors ended up mixed into ValidationState in the first place. I experimented with removing M_ERROR mode entirely here: https://github.com/stringintech/bitcoin/commit/beaa25ff76626fddf54f23f63adbc8a87ac2417c. Functions like FlushStateToDisk, ActivateBestChain aren't exactly about validating a certain block, yet they take a BlockValidationState& out-param just to absorb runtime errors, which feels like the wrong abstraction. Since we seem to prefer error handling explicit at call sites, util::Expected seems like a natural fit (over throwing exceptions). Those functions could return util::Expected<void, std::string> and drop the out-param entirely, while functions that can both perform validation and hit a runtime error (like ConnectBlock, AcceptBlock) could return util::Expected<BlockValidationState, std::string>, making the two failure modes distinct. The changes feel fairly mechanical, though I'm not sure it's worth the review churn as a standalone effort.

stickies-v commented at 5:46 PM on February 4, 2026: contributor

Concept ACK. As commented, I think it makes sense to use util::Expected here.

yuvicc force-pushed on Feb 4, 2026

DrahtBot removed the label Needs rebase on Feb 4, 2026

in src/validation.cpp:4294 in 3a528de7ea

4287 | @@ -4288,9 +4288,11 @@ bool ChainstateManager::AcceptBlockHeader(const CBlockHeader& block, BlockValida
4288 |  }
4289 |  
4290 |  // Exposed wrapper for AcceptBlockHeader
4291 | -bool ChainstateManager::ProcessNewBlockHeaders(std::span<const CBlockHeader> headers, bool min_pow_checked, BlockValidationState& state, const CBlockIndex** ppindex)
4292 | +BlockValidationState ChainstateManager::ProcessNewBlockHeaders(std::span<const CBlockHeader> headers, bool min_pow_checked, const CBlockIndex** ppindex)
4293 |  {
4294 |      AssertLockNotHeld(cs_main);
4295 | +    Assume(!headers.empty());

sedited commented at 11:39 AM on March 25, 2026:

I'm not sure about this assume. Is there a change in behaviour here that makes this a requirement? If not, I would just preserve existing behaviour.

hodlinator commented at 1:30 PM on March 25, 2026:

Responded below #33856 (review)

yuvicc commented at 12:36 PM on March 26, 2026:

Done. Thanks.

yuvicc force-pushed on Mar 26, 2026

in src/test/validation_block_tests.cpp:6 in 697b8a7802

   2 | @@ -3,14 +3,14 @@
   3 |  // file COPYING or http://www.opensource.org/licenses/mit-license.php.
   4 |  
   5 |  #include <boost/test/unit_test.hpp>
   6 | +#include <boost/test/unit_test_suite.hpp>

hodlinator commented at 1:19 PM on March 26, 2026:

Conflict between IWYU and anti-boost linter rule? Would try dropping it.

Context: https://github.com/bitcoin/bitcoin/actions/runs/23594588120/job/68707982818?pr=33856

A new Boost dependency in the form of "boost/test/unit_test_suite.hpp" appears to have been introduced:
src/test/validation_block_tests.cpp:#include <boost/test/unit_test_suite.hpp>

^---- ⚠️ Failure generated from lint-includes.py

fanquake commented at 1:40 AM on March 27, 2026:

IWYU isn't enforced on src/test yet, but it's saying to remove this line in any case: https://github.com/bitcoin/bitcoin/actions/runs/23594588120/job/68707982908?pr=33856#step:11:28669.

stringintech commented at 3:15 PM on April 13, 2026:

Seems this line caused the lint job to fail (as already mentioned in #33856 (review)).

yuvicc commented at 1:58 PM on April 14, 2026:

Done. Thanks.

in src/test/validation_block_tests.cpp:371 in 697b8a7802

 366 | @@ -368,4 +367,9 @@ BOOST_AUTO_TEST_CASE(witness_commitment_index)
 367 |  
 368 |      BOOST_CHECK_EQUAL(GetWitnessCommitmentIndex(pblock), 2);
 369 |  }
 370 | +
 371 | +BOOST_AUTO_TEST_CASE(test_empty_process_new_block_headers) {

hodlinator commented at 1:21 PM on March 26, 2026:

nit:

₿ git grep "BOOST_AUTO_TEST_CASE.*{" |wc -l
30
₿ git grep "BOOST_AUTO_TEST_CASE" |wc -l
647

BOOST_AUTO_TEST_CASE(test_empty_process_new_block_headers)
{

yuvicc commented at 1:58 PM on April 14, 2026:

Done. Thanks.

DrahtBot added the label CI failed on Mar 26, 2026

hodlinator approved

hodlinator commented at 1:30 PM on March 26, 2026: contributor

ACK 697b8a78028322a91738da47bc06cec0ddd544a9

This PR basically removes the second column for btck_chainstate_manager_process_block_header:

state	int return value
M_VALID	0
M_INVALID	-1
M_ERROR	-1
nullptr (exception)	-1

The int return value was kind of nice to have for the happy path, the PR requires consumers of the API to check for nullptr state before checking for M_VALID. Good for rigor, slight friction for prototyping. One less out-parameter is nice.

DrahtBot requested review from w0xlt on Mar 26, 2026

DrahtBot requested review from stickies-v on Mar 26, 2026

in src/kernel/bitcoinkernel.cpp:1307 in 697b8a7802

1306 | +    const btck_BlockHeader* header)
1307 |  {
1308 |      try {
1309 |          auto& chainman = btck_ChainstateManager::get(chainstate_manager).m_chainman;
1310 | -        auto result = chainman->ProcessNewBlockHeaders({&btck_BlockHeader::get(header), 1}, /*min_pow_checked=*/true, btck_BlockValidationState::get(state), /*ppindex=*/nullptr);
1311 | +        auto state = chainman->ProcessNewBlockHeaders({&btck_BlockHeader::get(header), 1}, /*min_pow_checked=*/true, /*ppindex=*/nullptr);

stringintech commented at 3:10 PM on April 13, 2026:

nit

        auto state = chainman->ProcessNewBlockHeaders({&btck_BlockHeader::get(header), 1}, /*min_pow_checked=*/true);

in src/net_processing.cpp:3053 in 697b8a7802

3049 | @@ -3076,35 +3050,33 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
3050 |      // something new (if these headers are valid).
3051 |      bool received_new_header{last_received_header == nullptr};
3052 |  
3053 | -    // Now process all the headers.
3054 | -    BlockValidationState state;
3055 | -    const bool processed{m_chainman.ProcessNewBlockHeaders(headers,
3056 | +    // Now process all the headers and return the block validation state.

stringintech commented at 3:18 PM on April 13, 2026:

nit: added comment seems redundant.

    // Now process all the headers.

DrahtBot commented at 3:44 PM on April 13, 2026: contributor

Could turn into draft while CI is red?

in src/net_processing.cpp:3054 in 697b8a7802 outdated

3049 | @@ -3076,35 +3050,33 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
3050 |      // something new (if these headers are valid).
3051 |      bool received_new_header{last_received_header == nullptr};
3052 |  
3053 | -    // Now process all the headers.
3054 | -    BlockValidationState state;
3055 | -    const bool processed{m_chainman.ProcessNewBlockHeaders(headers,
3056 | +    // Now process all the headers and return the block validation state.
3057 | +    BlockValidationState state{m_chainman.ProcessNewBlockHeaders(headers,

stringintech commented at 3:53 PM on April 13, 2026:

Although ProcessNewBlockHeaders doesn't seem to set state to M_ERROR mode, I think it is better to not rely on that assumption - similar to how submitheader explicitly handles the error case when calling the same function. Perhaps we can have sth like:

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index a0273f7d82..da9ff706f3 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -3054,15 +3054,18 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
     BlockValidationState state{m_chainman.ProcessNewBlockHeaders(headers,
                                                            /*min_pow_checked=*/true,
                                                            &pindexLast)};
-    if (state.IsInvalid()) {
-        if (!pfrom.IsInboundConn() && state.GetResult() == BlockValidationResult::BLOCK_CACHED_INVALID) {
-            // Warn user if outgoing peers send us headers of blocks that we previously marked as invalid.
-            LogWarning("%s (received from peer=%i). "
-                        "If this happens with all peers, consider database corruption (that -reindex may fix) "
-                        "or a potential consensus incompatibility.",
-                        state.GetDebugMessage(), pfrom.GetId());
-        }
-        MaybePunishNodeForBlock(pfrom.GetId(), state, via_compact_block, "invalid header received");
+    if (!state.IsValid()) {
+        // ProcessNewBlockHeaders only sets Invalid, never Error state.
+        if (Assume(state.IsInvalid())) {
+            if (!pfrom.IsInboundConn() && state.GetResult() == BlockValidationResult::BLOCK_CACHED_INVALID) {
+                // Warn user if outgoing peers send us headers of blocks that we previously marked as invalid.
+                LogWarning("%s (received from peer=%i). "
+                            "If this happens with all peers, consider database corruption (that -reindex may fix) "
+                            "or a potential consensus incompatibility.",
+                            state.GetDebugMessage(), pfrom.GetId());
+            }
+            MaybePunishNodeForBlock(pfrom.GetId(), state, via_compact_block, "invalid header received");
+        }
         return;
     }
     assert(pindexLast);
@@ -4560,8 +4563,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
 
         const CBlockIndex *pindex = nullptr;
         BlockValidationState state{m_chainman.ProcessNewBlockHeaders({{cmpctblock.header}}, /*min_pow_checked=*/true, &pindex)};
-        if (state.IsInvalid()) {
-            MaybePunishNodeForBlock(pfrom.GetId(), state, /*via_compact_block=*/true, "invalid header via cmpctblock");
+        if (!state.IsValid()) {
+            // ProcessNewBlockHeaders only sets Invalid, never Error state.
+            if (Assume(state.IsInvalid())) {
+                MaybePunishNodeForBlock(pfrom.GetId(), state, /*via_compact_block=*/true, "invalid header via cmpctblock");
+            }
             return;
         }

</details>

yuvicc commented at 1:01 PM on April 14, 2026:

Agree!

in src/net_processing.cpp:3070 in 697b8a7802

3082 | +        return;
3083 |      }
3084 |      assert(pindexLast);
3085 |  
3086 | -    if (processed && received_new_header) {
3087 | +    if (state.IsValid() && received_new_header) {

stringintech commented at 3:53 PM on April 13, 2026:

In combination with above comment, the first check seems redundant:

    if (received_new_header) {

A related discussion is here #27826 (review).

yuvicc force-pushed on Apr 14, 2026

yuvicc commented at 1:57 PM on April 14, 2026: contributor

Thanks for review @stringintech

Fixed nits: #33856 (review), #33856 (review), #33856 (review) & #33856 (review)
Explicitly handle the error case for state. comment

DrahtBot removed the label CI failed on Apr 14, 2026

w0xlt commented at 2:38 PM on April 15, 2026: contributor

reACK 93ea1b76686552b16486e55dc4e081f4b2661723

DrahtBot requested review from hodlinator on Apr 15, 2026

in src/net_processing.cpp:4548 in 93ea1b7668 outdated

4546 | @@ -4485,7 +4547,7 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string
4547 |          if (!prev_block) {
4548 |              // Doesn't connect (or is genesis), instead of DoSing in AcceptBlockHeader, request deeper headers

optout21 commented at 7:34 AM on April 16, 2026:

93ea1b7 validation: Return BlockValidationState from ProcessNewBlockHeaders:

A nit outside the scope of this PR: "AcceptBlockHeader" in the comment (L4548) seems to be outdated and seems to refer to the ProcessNewBlockHeaders call below (L4565). Similarly, the comment on L4574.

Consider if it makes sense to check these in this PR, and if so, consider if they are indeed stale.

optout21 commented at 7:37 AM on April 16, 2026: contributor

ACK 93ea1b76686552b16486e55dc4e081f4b2661723

A refactor of ProcessNewBlockHeaders() parameters, to return validation state in return value instead of an output parameter. The redundant return value is removed. The kernel wrapper is also updated. The change set is compact, self-sufficient and well-rounded. I have not found any issue with it.

What I did: Review the code; check compile and unit tests locally. Check the call sites, and their usage of the result. Check BlockValidationState and its usage in general.

Left a non-blocking nit regarding potentially stale related comments.

A note with scope beyond this PR: there are several other methods that return state in a similar fashion (in an output parameter), such as AcceptBlockHeader().

stringintech commented at 2:23 PM on April 27, 2026: contributor

ACK 93ea1b76

I’m a bit uneasy that the C API can return nullptr for runtime errors while BlockValidationState could also carry error state. Feels like two failure channels in one abstraction. Would be nice to split validation result from runtime failure more cleanly in the future, maybe along the lines of #33856 (review).

w0xlt commented at 6:03 PM on April 28, 2026: contributor

reACK 93ea1b76686552b16486e55dc4e081f4b2661723

in src/net_processing.cpp:3065 in 93ea1b7668

3068 | -                           "If this happens with all peers, consider database corruption (that -reindex may fix) "
3069 | -                           "or a potential consensus incompatibility.",
3070 | -                           state.GetDebugMessage(), pfrom.GetId());
3071 | +                            "If this happens with all peers, consider database corruption (that -reindex may fix) "
3072 | +                            "or a potential consensus incompatibility.",
3073 | +                            state.GetDebugMessage(), pfrom.GetId());

hodlinator commented at 6:59 AM on April 29, 2026:

Adding this new 1-space indentation for these 3 lines makes the code worse and messes with git blame history. No other reviewer caught this, and not the author? Oh, I see I also didn't catch it in my A-C-K of the earlier 697b8a78028322a91738da47bc06cec0ddd544a9 which also has this issue.

I think it's fair to invalidate A-C-Ks to undo this.

in src/net_processing.cpp:3057 in 93ea1b7668

hodlinator commented at 7:07 AM on April 29, 2026:

nit: Could align indentation:

--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -3052,8 +3052,8 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
 
     // Now process all the headers.
     BlockValidationState state{m_chainman.ProcessNewBlockHeaders(headers,
-                                                           /*min_pow_checked=*/true,
-                                                           &pindexLast)};
+                                                                 /*min_pow_checked=*/true,
+                                                                 &pindexLast)};
     if (!state.IsValid()) {
         // ProcessNewBlockHeaders only sets Invalid, never Error state.
         if (Assume(state.IsInvalid())) {

hodlinator commented at 10:47 PM on May 12, 2026:

nit: Commit message has typo "ProcessNewBlockHeadersalready\nalready".

hodlinator commented at 7:41 AM on April 29, 2026: contributor

Approach NACK 93ea1b76686552b16486e55dc4e081f4b2661723

I regret A-C-King this change now, it feels incomplete. Sorry for changing my mind.

Adding all the checks that we're either Valid or Invalid but not in Error may be technically correct in the present state but feels like laying land mines. Even if it would grow the PR a lot I would rather see a refactoring which makes the Error state impossible, along with AcceptBlock() et all not both returning a bool and a state which could theoretically mismatch. As it is we are making the validation code harder to maintain in order to please the kernel API.

hodlinator commented at 9:43 PM on April 29, 2026: contributor

Started going down the rabbit hole of implementing something more fully baked and making unexpected states impossible instead of adding a bunch of checks. Didn't find a really elegant way in my quick exploration but there may be one.

I would prefer this change was cut back to only touching the kernel API, or do a deeper change as I suggested above.

<details><summary>Suggested minimal diff that I would feel more comfortable A-C-King</summary>

diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
index ea646cd551..50ed46cd0c 100644
--- a/src/kernel/bitcoinkernel.cpp
+++ b/src/kernel/bitcoinkernel.cpp
@@ -1298,19 +1298,19 @@ int btck_chainstate_manager_process_block(
     return result ? 0 : -1;
 }
 
-int btck_chainstate_manager_process_block_header(
+btck_BlockValidationState* btck_chainstate_manager_process_block_header(
     btck_ChainstateManager* chainstate_manager,
-    const btck_BlockHeader* header,
-    btck_BlockValidationState* state)
+    const btck_BlockHeader* header)
 {
     try {
         auto& chainman = btck_ChainstateManager::get(chainstate_manager).m_chainman;
-        auto result = chainman->ProcessNewBlockHeaders({&btck_BlockHeader::get(header), 1}, /*min_pow_checked=*/true, btck_BlockValidationState::get(state), /*ppindex=*/nullptr);
-
-        return result ? 0 : -1;
+        auto state = btck_BlockValidationState::create();
+        auto result = chainman->ProcessNewBlockHeaders({&btck_BlockHeader::get(header), 1}, /*min_pow_checked=*/true, btck_BlockValidationState::get(state));
+        assert(result == btck_BlockValidationState::get(state).IsValid());
+        return state;
     } catch (const std::exception& e) {
         LogError("Failed to process block header: %s", e.what());
-        return -1;
+        return nullptr;
     }
 }
 
diff --git a/src/kernel/bitcoinkernel.h b/src/kernel/bitcoinkernel.h
index 5427e77617..f6eb4c3b70 100644
--- a/src/kernel/bitcoinkernel.h
+++ b/src/kernel/bitcoinkernel.h
@@ -1117,13 +1117,11 @@ BITCOINKERNEL_API const btck_BlockTreeEntry* BITCOINKERNEL_WARN_UNUSED_RESULT bt
  *
  * [@param](/bitcoin-bitcoin/contributor/param/)[in] chainstate_manager        Non-null.
  * [@param](/bitcoin-bitcoin/contributor/param/)[in] header                    Non-null btck_BlockHeader to be validated.
- * [@param](/bitcoin-bitcoin/contributor/param/)[out] block_validation_state   The result of the btck_BlockHeader validation.
- * [@return](/bitcoin-bitcoin/contributor/return/)                              0 if btck_BlockHeader processing completed successfully, non-zero on error.
+ * [@return](/bitcoin-bitcoin/contributor/return/)                              The btck_BlockValidationState containing validation results, or null on error.
  */
-BITCOINKERNEL_API int BITCOINKERNEL_WARN_UNUSED_RESULT btck_chainstate_manager_process_block_header(
+BITCOINKERNEL_API btck_BlockValidationState* BITCOINKERNEL_WARN_UNUSED_RESULT btck_chainstate_manager_process_block_header(
     btck_ChainstateManager* chainstate_manager,
-    const btck_BlockHeader* header,
-    btck_BlockValidationState* block_validation_state) BITCOINKERNEL_ARG_NONNULL(1, 2, 3);
+    const btck_BlockHeader* header) BITCOINKERNEL_ARG_NONNULL(1, 2);
 
 /**
  * [@brief](/bitcoin-bitcoin/contributor/brief/) Triggers the start of a reindex if the wipe options were previously
diff --git a/src/kernel/bitcoinkernel_wrapper.h b/src/kernel/bitcoinkernel_wrapper.h
index 064d0dd14b..632a89d0d2 100644
--- a/src/kernel/bitcoinkernel_wrapper.h
+++ b/src/kernel/bitcoinkernel_wrapper.h
@@ -940,6 +940,8 @@ public:
     explicit BlockValidationState() : Handle{btck_block_validation_state_create()} {}
 
     BlockValidationState(const BlockValidationStateView& view) : Handle{view} {}
+
+    BlockValidationState(btck_BlockValidationState* state) : Handle{state} {}
 };
 
 class ValidationInterface
@@ -1217,9 +1219,13 @@ public:
         return res == 0;
     }
 
-    bool ProcessBlockHeader(const BlockHeader& header, BlockValidationState& state)
+    BlockValidationState ProcessBlockHeader(const BlockHeader& header)
     {
-        return btck_chainstate_manager_process_block_header(get(), header.get(), state.get()) == 0;
+        auto state = btck_chainstate_manager_process_block_header(get(), header.get());
+        if (!state) {
+            throw std::runtime_error("Failed to process block header");
+        }
+        return BlockValidationState{state};
     }
 
     ChainView GetChain() const
diff --git a/src/test/kernel/test_kernel.cpp b/src/test/kernel/test_kernel.cpp
index 5a38006539..af29942058 100644
--- a/src/test/kernel/test_kernel.cpp
+++ b/src/test/kernel/test_kernel.cpp
@@ -1013,10 +1013,9 @@ BOOST_AUTO_TEST_CASE(btck_chainman_regtest_tests)
         for (const auto& data : REGTEST_BLOCK_DATA) {
             Block block{hex_string_to_byte_vec(data)};
             BlockHeader header = block.GetHeader();
-            BlockValidationState state{};
-            BOOST_CHECK(state.GetBlockValidationResult() == BlockValidationResult::UNSET);
-            BOOST_CHECK(chainman->ProcessBlockHeader(header, state));
+            BlockValidationState state = chainman->ProcessBlockHeader(header);
             BOOST_CHECK(state.GetValidationMode() == ValidationMode::VALID);
+            BOOST_CHECK(state.GetBlockValidationResult() == BlockValidationResult::UNSET);
             BlockTreeEntry entry{*chainman->GetBlockTreeEntry(header.Hash())};
             BOOST_CHECK(!chainman->GetChain().Contains(entry));
             BlockTreeEntry best_entry{chainman->GetBestEntry()};

</details>

Apologies again for raising my bar after you already passed over it.

yuvicc commented at 6:08 PM on May 12, 2026: contributor

Adding all the checks that we're either Valid or Invalid but not in Error may be technically correct in the present state but feels like laying land mines. Even if it would grow the PR a lot I would rather see a refactoring which makes the Error state impossible, along with AcceptBlock() et all not both returning a bool and a state which could theoretically mismatch.

I guess I agree with you @hodlinator and @stringintech, for this PR specifically I would like to keep it to only kernel API rather than refactoring the whole validation code which could be done in another PR which would also reduce the headache while reviewing the PR and separation of concerns.

yuvicc force-pushed on May 12, 2026

yuvicc renamed this:
~~kernel, validation: Refactor ProcessNewBlockHeaders to return BlockValidationState~~
kernel: Refactor process_block_header to return btck_BlockValidationState
on May 12, 2026

DrahtBot added the label Validation on May 12, 2026

yuvicc commented at 6:15 PM on May 12, 2026: contributor

Thanks for the review @hodlinator I have updated the PR title and description as well as the suggestions you mentioned above and added you as co-author.

yuvicc force-pushed on May 12, 2026

DrahtBot added the label CI failed on May 12, 2026

DrahtBot commented at 6:23 PM on May 12, 2026: contributor

🚧 At least one of the CI tasks failed. Task macOS-cross to x86_64: https://github.com/bitcoin/bitcoin/actions/runs/25753207447/job/75634813034 LLM reason (✨ experimental): CI failed during compilation because Clang (with -Werror) reports a documentation error: block_validation_state parameter referenced in bitcoinkernel.h is not found in the corresponding function declaration.

<details><summary>Hints</summary>

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.

</details>

DrahtBot removed the label CI failed on May 12, 2026

w0xlt commented at 9:49 PM on May 12, 2026: contributor

reACK 2471404f7d17898e1f5792b7db1bc2b10f837b47

DrahtBot requested review from stringintech on May 12, 2026

DrahtBot requested review from hodlinator on May 12, 2026

DrahtBot requested review from optout21 on May 12, 2026

in src/kernel/bitcoinkernel.cpp:1309 in 2471404f7d

1309 |          auto& chainman = btck_ChainstateManager::get(chainstate_manager).m_chainman;
1310 | -        auto result = chainman->ProcessNewBlockHeaders({&btck_BlockHeader::get(header), 1}, /*min_pow_checked=*/true, btck_BlockValidationState::get(state), /*ppindex=*/nullptr);
1311 |  
1312 | -        return result ? 0 : -1;
1313 | +        auto state = btck_BlockValidationState::create();
1314 | +        auto result = chainman->ProcessNewBlockHeaders({&btck_BlockHeader::get(header), 1}, /*min_pow_checked=*/true, btck_BlockValidationState::get(state));

hodlinator commented at 10:53 PM on May 12, 2026:

nanonit: Could make bool explicit to reader and use curly-braces to prevent narrowing:

        bool result{chainman->ProcessNewBlockHeaders({&btck_BlockHeader::get(header), 1}, /*min_pow_checked=*/true, btck_BlockValidationState::get(state))};

stickies-v commented at 12:54 PM on May 18, 2026:

+1

hodlinator approved

hodlinator commented at 11:06 PM on May 12, 2026: contributor

ACK 2471404f7d17898e1f5792b7db1bc2b10f837b47

Thanks for taking my feedback @yuvicc.

I stared some more today at your previous version of the PR, 93ea1b76686552b16486e55dc4e081f4b2661723 - specifically the changes to net_processing.cpp and validation.cpp. There is some nuance to them that probably make them less scary than I was previously thinking. I was somehow hallucinating:

if (!state.IsValid()) {
    CHECK_NONFATAL(state.IsInvalid());

or

if (!accepted) {
    if (!state.IsInvalid()) NONFATAL_UNREACHABLE();

But your change at least allowed for BlockValidationState::M_ERROR in the upper part of ChainstateManager::ProcessNewBlockHeaders().

On the other hand you added Assume()s in net_processing.cpp which some seem to have issues with in other contexts: #34440 (review)

Was coming to terms with 93ea1b76686552b16486e55dc4e081f4b2661723 minus the whitespace issues and the Assume()s. I then had a health issue and was unable to finish my re-review earlier today.

Here is my experimental branch which builds upon your earlier push and consistently returns BlockValidationState or NoErrorBlockValidationState from functions: https://github.com/bitcoin/bitcoin/compare/master...hodlinator:bitcoin:pr/33856_suggestions It's a bit of a beast to polish & review, and should probably be broken up into many commits.

That experiment makes your inclusion of the refactor of ChainstateManager::ProcessNewBlockHeaders() look like a valid strategic move. Sorry for the onset of paranoia.

yuvicc commented at 4:56 AM on May 13, 2026: contributor

Thanks @hodlinator I will take a look at this and push when ready.

Here is my experimental branch which builds upon your earlier push and consistently returns BlockValidationState or NoErrorBlockValidationState from functions: https://github.com/bitcoin/bitcoin/compare/master...hodlinator:bitcoin:pr/33856_suggestions It's a bit of a beast to polish & review, and should probably be broken up into many commits.

yuvicc marked this as a draft on May 13, 2026

optout21 commented at 8:40 AM on May 18, 2026: contributor

ACK 88d9bc5aa4f9e054487e8cfa60a1f12c5d508237 Re-acked, only minor changes since my last review.

Prev: ACK 2471404f7d17898e1f5792b7db1bc2b10f837b47

I've processed the new discussion since my last review, re-reviewed the changes. The change-set has been narrowed down, restricted to kernel part. LGTM in the current form. However, from the discussions I gather that it may still change in scope (that's fine).

optout21 commented at 12:38 PM on May 18, 2026: contributor

Here is my experimental branch which builds upon your earlier push and consistently returns BlockValidationState or NoErrorBlockValidationState from functions @hodlinator (cc @yuvicc) : I've checked your branch, I've started something very similar 1-2 weeks ago, visible here: https://github.com/optout21/bitcoin/pull/7 I've started changing the validation state output parameter to a return value in a few methods. I meant it as a continuation of this PR, but in fact it can go independently. I may improve/finalize it and open into a new PR, unless there are objections.

in src/kernel/bitcoinkernel_wrapper.h:944 in 2471404f7d

 962 |  
 963 | -    TxValidationResult GetTxValidationResult() const
 964 | -    {
 965 | -        return static_cast<TxValidationResult>(btck_tx_validation_state_get_tx_validation_result(get()));
 966 | -    }
 967 | +    BlockValidationState(btck_BlockValidationState* state) : Handle{state} {}

stickies-v commented at 12:54 PM on May 18, 2026:

nit: I think this ctor (and above) should be explicit

in src/kernel/bitcoinkernel_wrapper.h:1226 in 2471404f7d

1223 | +    BlockValidationState ProcessBlockHeader(const BlockHeader& header)
1224 |      {
1225 | -        return btck_chainstate_manager_process_block_header(get(), header.get(), state.get()) == 0;
1226 | +        auto state = btck_chainstate_manager_process_block_header(get(), header.get());
1227 | +        if (!state) {
1228 | +            throw std::runtime_error("Failed to process block header");

stickies-v commented at 12:56 PM on May 18, 2026:

I don't think we need this, Handle already calls check in its ctor.

hodlinator commented at 5:36 PM on May 19, 2026:

Since I'm unfamiliar with this code I confirmed this by always setting the pointer to nullptr and running test_kernel and got the expected exception. This appears to be the call: https://github.com/bitcoin/bitcoin/blob/88d9bc5aa4f9e054487e8cfa60a1f12c5d508237/src/kernel/bitcoinkernel_wrapper.h#L315

stickies-v commented at 12:59 PM on May 18, 2026: contributor

code LGTM 2471404f7d17898e1f5792b7db1bc2b10f837b47 modulo outstanding comments. I think this is a clear, contained interface win and think we can make other changes in follow-ups. Would prefer to undraft this and merge ~as-is (after addressing outstanding comments)

DrahtBot requested review from stickies-v on May 18, 2026

hodlinator commented at 1:04 PM on May 18, 2026: contributor

Agree with @stickies-v that the best way forward for this PR would be to as-is (+ potentially addressing some nits), maybe also bringing back some of the changes from 93ea1b76686552b16486e55dc4e081f4b2661723 which I was paranoid about but was coming to terms with.

I would caution against doing the full refactor to return BlockValidationState from AcceptBlockHeader() etc as me and @optout21 experimented with as part of this PR. Happy to help those changes along in another PR, whoever authors it.

yuvicc force-pushed on May 19, 2026

yuvicc marked this as ready for review on May 19, 2026

kernel: Return btck_BlockValidationState from process_block_header API

Remove redundant int return from btck_chainstate_manager_process_block_header.
Previously returned both an int result and an output validation state parameter, creating ambiguity
where non-zero could mean either invalid header or processing failure. Since ProcessNewBlockHeaders
already provides complete validation info, the int return was redundant.

Co-authored-by: stringintech <stringintech@gmail.com>
Co-authored-by: stickies-v <stickies-v@protonmail.com>
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>

88d9bc5aa4

yuvicc force-pushed on May 19, 2026

DrahtBot added the label CI failed on May 19, 2026

yuvicc commented at 10:45 AM on May 19, 2026: contributor

I have addressed all the nits and marked this as ready for review. Further changes can be done in a follow-up PR. Thanks @stickies-v and @hodlinator.

DrahtBot removed the label CI failed on May 19, 2026

DrahtBot requested review from hodlinator on May 19, 2026

DrahtBot requested review from w0xlt on May 19, 2026

stickies-v approved

stickies-v commented at 1:55 PM on May 19, 2026: contributor

ACK 88d9bc5aa4f9e054487e8cfa60a1f12c5d508237

PR description needs updating: return btck_BlockValidationState by value - we return by ptr.

w0xlt commented at 4:40 PM on May 19, 2026: contributor

reACK 88d9bc5aa4f9e054487e8cfa60a1f12c5d508237

hodlinator approved

hodlinator commented at 5:41 PM on May 19, 2026: contributor

re-ACK 88d9bc5aa4f9e054487e8cfa60a1f12c5d508237

Only addressed nits since previous review.

fanquake merged this on May 19, 2026

fanquake closed this on May 19, 2026

kernel: Refactor process_block_header to return btck_BlockValidationState #33856

Code Coverage & Benchmarks

Reviews

Conflicts