validation: Return BlockValidationState from ProcessNewBlockHeaders

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.

Author: yuvicc

src/net_processing.cpp

@@ -3046,27 +3046,24 @@ 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.
-    BlockValidationState state;
-    const bool processed{m_chainman.ProcessNewBlockHeaders(headers,
+    // Now process all the headers and return the block validation state.
+    BlockValidationState state{m_chainman.ProcessNewBlockHeaders(headers,
                                                            /*min_pow_checked=*/true,
-                                                           state, &pindexLast)};
-    if (!processed) {
-        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");
-            return;
-        }
+                                                           &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");
+        return;
     }
     assert(pindexLast);
 
-    if (processed && received_new_header) {
+    if (state.IsValid() && received_new_header) {
         LogBlockHeader(*pindexLast, pfrom, /*via_compact_block=*/false);
     }
 
@@ -4558,12 +4555,10 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
         }
 
         const CBlockIndex *pindex = nullptr;
-        BlockValidationState state;
-        if (!m_chainman.ProcessNewBlockHeaders({{cmpctblock.header}}, /*min_pow_checked=*/true, state, &pindex)) {
-            if (state.IsInvalid()) {
-                MaybePunishNodeForBlock(pfrom.GetId(), state, /*via_compact_block=*/true, "invalid header via cmpctblock");
-                return;
-            }
+        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");
+            return;
         }
 
         // If AcceptBlockHeader returned true, it set pindex

src/rpc/mining.cpp

@@ -1123,8 +1123,7 @@ static RPCHelpMan submitheader()
         }
     }
 
-    BlockValidationState state;
-    chainman.ProcessNewBlockHeaders({{h}}, /*min_pow_checked=*/true, state);
+    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());

src/test/blockfilter_index_tests.cpp

@@ -103,8 +103,7 @@ bool BuildChainTestingSetup::BuildChain(const CBlockIndex* pindex,
     for (auto& block : chain) {
         block = std::make_shared<CBlock>(CreateBlock(pindex, no_txns, coinbase_script_pub_key));
 
-        BlockValidationState state;
-        if (!Assert(m_node.chainman)->ProcessNewBlockHeaders({{*block}}, true, state, &pindex)) {
+        if (!Assert(m_node.chainman)->ProcessNewBlockHeaders({{*block}}, true, &pindex).IsValid()) {
             return false;
         }
     }

src/test/fuzz/utxo_snapshot.cpp

@@ -88,9 +88,8 @@ void initialize_chain()
     if constexpr (INVALID) {
         auto& chainman{*setup->m_node.chainman};
         for (const auto& block : chain) {
-            BlockValidationState dummy;
-            bool processed{chainman.ProcessNewBlockHeaders({{*block}}, true, dummy)};
-            Assert(processed);
+            auto result{chainman.ProcessNewBlockHeaders({{*block}}, true)};
+            Assert(result.IsValid());
             const auto* index{WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockIndex(block->GetHash()))};
             Assert(index);
         }
@@ -170,9 +169,8 @@ 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) {
-                BlockValidationState dummy;
-                bool processed{chainman.ProcessNewBlockHeaders({{*block}}, true, dummy)};
-                Assert(processed);
+                auto result{chainman.ProcessNewBlockHeaders({{*block}}, true)};
+                Assert(result.IsValid());
                 const auto* index{WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockIndex(block->GetHash()))};
                 Assert(index);
             }

src/test/validation_block_tests.cpp

@@ -104,8 +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
-    BlockValidationState ignored;
-    BOOST_CHECK(Assert(m_node.chainman)->ProcessNewBlockHeaders({{*pblock}}, true, ignored));
+    BOOST_CHECK(Assert(m_node.chainman)->ProcessNewBlockHeaders({{*pblock}}, true).IsValid());
 
     return pblock;
 }

src/validation.cpp

@@ -4300,9 +4300,11 @@ bool ChainstateManager::AcceptBlockHeader(const CBlockHeader& block, BlockValida
 }
 
 // Exposed wrapper for AcceptBlockHeader
-bool ChainstateManager::ProcessNewBlockHeaders(std::span<const CBlockHeader> headers, bool min_pow_checked, BlockValidationState& state, const CBlockIndex** ppindex)
+BlockValidationState ChainstateManager::ProcessNewBlockHeaders(std::span<const CBlockHeader> headers, bool min_pow_checked, const CBlockIndex** ppindex)
 {
     AssertLockNotHeld(cs_main);
+    Assume(!headers.empty());
+    BlockValidationState state;
     {
         LOCK(cs_main);
         for (const CBlockHeader& header : headers) {
@@ -4311,8 +4313,10 @@ bool ChainstateManager::ProcessNewBlockHeaders(std::span<const CBlockHeader> hea
             CheckBlockIndex();
 
             if (!accepted) {
-                return false;
+                if (state.IsValid()) NONFATAL_UNREACHABLE();
+                return state;
             }
+
             if (ppindex) {
                 *ppindex = pindex;
             }
@@ -4327,7 +4331,9 @@ bool ChainstateManager::ProcessNewBlockHeaders(std::span<const CBlockHeader> hea
             LogInfo("Synchronizing blockheaders, height: %d (~%.2f%%)\n", last_accepted.nHeight, progress);
         }
     }
-    return true;
+
+    if (!state.IsValid()) NONFATAL_UNREACHABLE();
+    return state;
 }
 
 void ChainstateManager::ReportHeadersPresync(int64_t height, int64_t timestamp)

src/validation.h

@@ -1239,11 +1239,12 @@ class ChainstateManager
      *
      * @param[in]  headers The block headers themselves
      * @param[in]  min_pow_checked  True if proof-of-work anti-DoS checks have been done by caller for headers chain
-     * @param[out] state This may be set to an Error state if any error occurred processing them
      * @param[out] ppindex If set, the pointer will be set to point to the last new block index object for the given headers
-     * @returns false if AcceptBlockHeader fails on any of the headers, true otherwise (including if headers were already known)
+     * @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.
      */
-    bool ProcessNewBlockHeaders(std::span<const CBlockHeader> headers, bool min_pow_checked, BlockValidationState& state, const CBlockIndex** ppindex = nullptr) LOCKS_EXCLUDED(cs_main);
+    BlockValidationState ProcessNewBlockHeaders(std::span<const CBlockHeader> headers, bool min_pow_checked, const CBlockIndex** ppindex = nullptr) LOCKS_EXCLUDED(cs_main);
 
     /**
      * Sufficiently validate a block for disk storage (and store on disk).