kernel, validation: Refactor ProcessNewBlockHeaders to return BlockValidationState

src/bitcoin-chainstate.cpp

@@ -44,7 +44,7 @@ class TestValidationInterface : public ValidationInterface
 
     std::optional<std::string> m_expected_valid_block = std::nullopt;
 
-    void BlockChecked(const Block block, const BlockValidationState state) override
+    void BlockChecked(const Block block, const BlockValidationStateView state) override
     {
         auto mode{state.GetValidationMode()};
         switch (mode) {

src/kernel/bitcoinkernel.cpp

@@ -860,6 +860,11 @@ const btck_BlockTreeEntry* btck_block_tree_entry_get_previous(const btck_BlockTr
     return btck_BlockTreeEntry::ref(btck_BlockTreeEntry::get(entry).pprev);
 }
 
+btck_BlockValidationState* btck_block_validation_state_create()
+{
+    return btck_BlockValidationState::create(BlockValidationState{});
+}
+
 btck_ValidationMode btck_block_validation_state_get_validation_mode(const btck_BlockValidationState* block_validation_state_)
 {
     auto& block_validation_state = btck_BlockValidationState::get(block_validation_state_);
@@ -894,6 +899,16 @@ btck_BlockValidationResult btck_block_validation_state_get_block_validation_resu
     assert(false);
 }
 
+btck_BlockValidationState* btck_block_validation_state_copy(const btck_BlockValidationState* state)
+{
+    return btck_BlockValidationState::copy(state);
+}
+
+void btck_block_validation_state_destroy(btck_BlockValidationState* state)
+{
+    delete state;
+}
+
 btck_ChainstateManagerOptions* btck_chainstate_manager_options_create(const btck_Context* context, const char* data_dir, size_t data_dir_len, const char* blocks_dir, size_t blocks_dir_len)
 {
     if (data_dir == nullptr || data_dir_len == 0 || blocks_dir == nullptr || blocks_dir_len == 0) {

src/kernel/bitcoinkernel.h

@@ -1207,6 +1207,11 @@ BITCOINKERNEL_API void btck_block_destroy(btck_Block* block);
  */
 ///@{
 
+/**
+ * Create a new block validation state.
+ */
+BITCOINKERNEL_API btck_BlockValidationState* BITCOINKERNEL_WARN_UNUSED_RESULT btck_block_validation_state_create();
+
 /**
  * Returns the validation mode from an opaque block validation state pointer.
  */
@@ -1219,6 +1224,21 @@ BITCOINKERNEL_API btck_ValidationMode btck_block_validation_state_get_validation
 BITCOINKERNEL_API btck_BlockValidationResult btck_block_validation_state_get_block_validation_result(
     const btck_BlockValidationState* block_validation_state) BITCOINKERNEL_ARG_NONNULL(1);
 
+/**
+ * @brief Copies the block validation state.
+ *
+ * @param[in] block_validation_state Non-null.
+ * @return                           The copied block validation state.
+ */
+BITCOINKERNEL_API btck_BlockValidationState* BITCOINKERNEL_WARN_UNUSED_RESULT btck_block_validation_state_copy(
+    const btck_BlockValidationState* block_validation_state) BITCOINKERNEL_ARG_NONNULL(1);
+
+/**
+ * Destroy the block validation state.
+ */
+BITCOINKERNEL_API void btck_block_validation_state_destroy(
+    btck_BlockValidationState* block_validation_state) BITCOINKERNEL_ARG_NONNULL(1);
+
 ///@}
 
 /** @name Chain

src/kernel/bitcoinkernel_wrapper.h

@@ -829,36 +829,52 @@ class KernelNotifications
     virtual void FatalErrorHandler(std::string_view error) {}
 };
 
-class BlockValidationState
+template<typename Derived>
+class BlockValidationStateApi
 {
 private:
-    const btck_BlockValidationState* m_state;
-
-public:
-    BlockValidationState(const btck_BlockValidationState* state) : m_state{state} {}
+    auto impl() const
+    {
+        return static_cast<const Derived*>(this)->get();
+    }
 
-    BlockValidationState(const BlockValidationState&) = delete;
-    BlockValidationState& operator=(const BlockValidationState&) = delete;
-    BlockValidationState(BlockValidationState&&) = delete;
-    BlockValidationState& operator=(BlockValidationState&&) = delete;
+    friend Derived;
+    BlockValidationStateApi() = default;
 
+public:
     ValidationMode GetValidationMode() const
     {
-        return static_cast<ValidationMode>(btck_block_validation_state_get_validation_mode(m_state));
+        return static_cast<ValidationMode>(btck_block_validation_state_get_validation_mode(impl()));
     }
 
     BlockValidationResult GetBlockValidationResult() const
     {
-        return static_cast<BlockValidationResult>(btck_block_validation_state_get_block_validation_result(m_state));
+        return static_cast<BlockValidationResult>(btck_block_validation_state_get_block_validation_result(impl()));
     }
 };
 
+class BlockValidationStateView : public View<btck_BlockValidationState>, public BlockValidationStateApi<BlockValidationStateView>
+{
+public:
+    explicit BlockValidationStateView(const btck_BlockValidationState* ptr) : View{ptr} {}
+};
+
+class BlockValidationState : public Handle<btck_BlockValidationState, btck_block_validation_state_copy, btck_block_validation_state_destroy>, public BlockValidationStateApi<BlockValidationState>
+{
+public:
+    explicit BlockValidationState() : Handle{btck_block_validation_state_create()} {}
+
+    explicit BlockValidationState(btck_BlockValidationState* ptr) : Handle{ptr} {}
+
+    BlockValidationState(const BlockValidationStateView& view) : Handle{view} {}
+};
+
 class ValidationInterface
 {
 public:
     virtual ~ValidationInterface() = default;
 
-    virtual void BlockChecked(Block block, const BlockValidationState state) {}
+    virtual void BlockChecked(Block block, const BlockValidationStateView state) {}
 
     virtual void PowValidBlock(BlockTreeEntry entry, Block block) {}
 
@@ -916,7 +932,7 @@ class ContextOptions : public UniqueHandle<btck_ContextOptions, btck_context_opt
             btck_ValidationInterfaceCallbacks{
                 .user_data = heap_vi.release(),
                 .user_data_destroy = +[](void* user_data) { delete static_cast<user_type>(user_data); },
-                .block_checked = +[](void* user_data, btck_Block* block, const btck_BlockValidationState* state) { (*static_cast<user_type>(user_data))->BlockChecked(Block{block}, BlockValidationState{state}); },
+                .block_checked = +[](void* user_data, btck_Block* block, const btck_BlockValidationState* state) { (*static_cast<user_type>(user_data))->BlockChecked(Block{block}, BlockValidationStateView{state}); },
                 .pow_valid_block = +[](void* user_data, btck_Block* block, const btck_BlockTreeEntry* entry) { (*static_cast<user_type>(user_data))->PowValidBlock(BlockTreeEntry{entry}, Block{block}); },
                 .block_connected = +[](void* user_data, btck_Block* block, const btck_BlockTreeEntry* entry) { (*static_cast<user_type>(user_data))->BlockConnected(Block{block}, BlockTreeEntry{entry}); },
                 .block_disconnected = +[](void* user_data, btck_Block* block, const btck_BlockTreeEntry* entry) { (*static_cast<user_type>(user_data))->BlockDisconnected(Block{block}, BlockTreeEntry{entry}); },

src/net_processing.cpp

@@ -2948,20 +2948,17 @@ 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()) {
-            MaybePunishNodeForBlock(pfrom.GetId(), state, via_compact_block, "invalid header received");
-            return;
-        }
+                                                           &pindexLast)};
+    if (state.IsInvalid()) {
+        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);
     }
 
@@ -4361,12 +4358,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

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

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) {
-            BlockValidationState dummy;
-            bool processed{chainman.ProcessNewBlockHeaders({{block->GetBlockHeader()}}, true, dummy)};
+            bool processed{chainman.ProcessNewBlockHeaders({{block->GetBlockHeader()}}, true).IsValid()};
             Assert(processed);
             const auto* index{WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockIndex(block->GetHash()))};
             Assert(index);
@@ -170,8 +169,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) {
-                BlockValidationState dummy;
-                bool processed{chainman.ProcessNewBlockHeaders({{block->GetBlockHeader()}}, true, dummy)};
+                bool processed{chainman.ProcessNewBlockHeaders({{block->GetBlockHeader()}}, true).IsValid()};
                 Assert(processed);
                 const auto* index{WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockIndex(block->GetHash()))};
                 Assert(index);

src/test/kernel/test_kernel.cpp

@@ -145,7 +145,7 @@ class TestValidationInterface : public ValidationInterface
 public:
     std::optional<std::vector<std::byte>> m_expected_valid_block = std::nullopt;
 
-    void BlockChecked(Block block, const BlockValidationState state) override
+    void BlockChecked(Block block, const BlockValidationStateView state) override
     {
         if (m_expected_valid_block.has_value()) {
             auto ser_block{block.ToBytes()};

src/test/validation_block_tests.cpp

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

src/validation.cpp

@@ -4284,9 +4284,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) {
@@ -4295,8 +4297,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;
             }
@@ -4311,7 +4315,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(const arith_uint256& work, int64_t height, int64_t timestamp)

src/validation.h

@@ -1231,11 +1231,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).