[draft] Refactor some validation.cpp methods to return BlockValidationState

src/bench/checkblock.cpp

@@ -40,9 +40,8 @@ static void CheckBlockTest(benchmark::Bench& bench)
             assert(block.vtx.size() == 1557);
         })
         .run([&] {
-            BlockValidationState validationState;
-            const bool checked{CheckBlock(block, validationState, chain_params->GetConsensus())};
-            assert(checked);
+            BlockValidationState validationState{CheckBlock(block, chain_params->GetConsensus())};
+            assert(validationState.IsValid());
         });
 }
 

src/bench/duplicate_inputs.cpp

@@ -70,8 +70,8 @@ static void DuplicateInputs(benchmark::Bench& bench)
     block.hashMerkleRoot = BlockMerkleRoot(block);
 
     bench.run([&] {
-        BlockValidationState cvstate{};
-        assert(!CheckBlock(block, cvstate, chainparams.GetConsensus(), false, false));
+        BlockValidationState cvstate{CheckBlock(block, chainparams.GetConsensus(), false, false)};
+        assert(!cvstate.IsValid());
         assert(cvstate.GetRejectReason() == "bad-txns-inputs-duplicate");
     });
 }

src/kernel/bitcoinkernel.cpp

@@ -1146,9 +1146,9 @@ int btck_block_check(const btck_Block* block, const btck_ConsensusParams* consen
     const bool check_pow    = (flags & btck_BlockCheckFlags_POW) != 0;
     const bool check_merkle = (flags & btck_BlockCheckFlags_MERKLE) != 0;
 
-    const bool result = CheckBlock(*btck_Block::get(block), state, btck_ConsensusParams::get(consensus_params), /*fCheckPOW=*/check_pow, /*fCheckMerkleRoot=*/check_merkle);
+    state = CheckBlock(*btck_Block::get(block), btck_ConsensusParams::get(consensus_params), /*fCheckPOW=*/check_pow, /*fCheckMerkleRoot=*/check_merkle);
 
-    return result ? 1 : 0;
+    return state.IsValid() ? 1 : 0;
 }
 
 size_t btck_block_count_transactions(const btck_Block* block)
@@ -1346,11 +1346,9 @@ btck_BlockValidationState* btck_chainstate_manager_process_block_header(
 {
     try {
         auto& chainman = btck_ChainstateManager::get(chainstate_manager).m_chainman;
+        auto state = chainman->ProcessNewBlockHeaders({&btck_BlockHeader::get(header), 1}, /*min_pow_checked=*/true);
 
-        auto state = btck_BlockValidationState::create();
-        bool 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;
+        return btck_BlockValidationState::create(state);
     } catch (const std::exception& e) {
         LogError("Failed to process block header: %s", e.what());
         return nullptr;

src/net_processing.cpp

@@ -3079,26 +3079,26 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
     bool received_new_header{last_received_header == nullptr};
 
     // Now process all the headers.
-    BlockValidationState state;
-    const bool processed{m_chainman.ProcessNewBlockHeaders(headers,
+    BlockValidationState state{m_chainman.ProcessNewBlockHeaders(headers,
                                                            /*min_pow_checked=*/true,
-                                                           state, &pindexLast)};
-    if (!processed) {
-        if (state.IsInvalid()) {
+                                                           &pindexLast)};
+    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());
+                            "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;
         }
+        return;
     }
     assert(pindexLast);
 
-    if (processed && received_new_header) {
+    if (received_new_header) {
         LogBlockHeader(*pindexLast, pfrom, /*via_compact_block=*/false);
     }
 
@@ -4501,12 +4501,13 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string
         }
 
         const CBlockIndex *pindex = nullptr;
-        BlockValidationState state;
-        if (!m_chainman.ProcessNewBlockHeaders({{cmpctblock.header}}, /*min_pow_checked=*/true, state, &pindex)) {
-            if (state.IsInvalid()) {
+        BlockValidationState state{m_chainman.ProcessNewBlockHeaders({{cmpctblock.header}}, /*min_pow_checked=*/true, &pindex)};
+        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;
             }
+            return;
         }
 
         // If AcceptBlockHeader returned true, it set pindex

src/rpc/mining.cpp

@@ -1134,8 +1134,7 @@ static RPCMethod 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

@@ -105,8 +105,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/coinstatsindex_tests.cpp

@@ -88,8 +88,8 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_unclean_shutdown, TestChain100Setup)
             new_block = std::make_shared<CBlock>(block);
 
             LOCK(cs_main);
-            BlockValidationState state;
-            BOOST_CHECK(CheckBlock(block, state, params.GetConsensus()));
+            BlockValidationState state{CheckBlock(block, params.GetConsensus())};
+            BOOST_CHECK(state.IsValid());
             BOOST_CHECK(m_node.chainman->AcceptBlock(new_block, state, &new_block_index, true, nullptr, nullptr, true));
             CCoinsViewCache view(&chainstate.CoinsTip());
             BOOST_CHECK(chainstate.ConnectBlock(block, state, new_block_index, view));

src/test/fuzz/block.cpp

@@ -31,23 +31,19 @@ FUZZ_TARGET(block, .init = initialize_block)
         return;
     }
     const Consensus::Params& consensus_params = Params().GetConsensus();
-    BlockValidationState validation_state_pow_and_merkle;
-    const bool valid_incl_pow_and_merkle = CheckBlock(block, validation_state_pow_and_merkle, consensus_params, /* fCheckPOW= */ true, /* fCheckMerkleRoot= */ true);
+    BlockValidationState validation_state_pow_and_merkle{CheckBlock(block, consensus_params, /* fCheckPOW= */ true, /* fCheckMerkleRoot= */ true)};
     assert(validation_state_pow_and_merkle.IsValid() || validation_state_pow_and_merkle.IsInvalid() || validation_state_pow_and_merkle.IsError());
     (void)validation_state_pow_and_merkle.Error("");
-    BlockValidationState validation_state_pow;
-    const bool valid_incl_pow = CheckBlock(block, validation_state_pow, consensus_params, /* fCheckPOW= */ true, /* fCheckMerkleRoot= */ false);
+    BlockValidationState validation_state_pow{CheckBlock(block, consensus_params, /* fCheckPOW= */ true, /* fCheckMerkleRoot= */ false)};
     assert(validation_state_pow.IsValid() || validation_state_pow.IsInvalid() || validation_state_pow.IsError());
-    BlockValidationState validation_state_merkle;
-    const bool valid_incl_merkle = CheckBlock(block, validation_state_merkle, consensus_params, /* fCheckPOW= */ false, /* fCheckMerkleRoot= */ true);
+    BlockValidationState validation_state_merkle{CheckBlock(block, consensus_params, /* fCheckPOW= */ false, /* fCheckMerkleRoot= */ true)};
     assert(validation_state_merkle.IsValid() || validation_state_merkle.IsInvalid() || validation_state_merkle.IsError());
-    BlockValidationState validation_state_none;
-    const bool valid_incl_none = CheckBlock(block, validation_state_none, consensus_params, /* fCheckPOW= */ false, /* fCheckMerkleRoot= */ false);
+    BlockValidationState validation_state_none{CheckBlock(block, consensus_params, /* fCheckPOW= */ false, /* fCheckMerkleRoot= */ false)};
     assert(validation_state_none.IsValid() || validation_state_none.IsInvalid() || validation_state_none.IsError());
-    if (valid_incl_pow_and_merkle) {
-        assert(valid_incl_pow && valid_incl_merkle && valid_incl_none);
-    } else if (valid_incl_merkle || valid_incl_pow) {
-        assert(valid_incl_none);
+    if (validation_state_pow_and_merkle.IsValid()) {
+        assert(validation_state_pow.IsValid() && validation_state_merkle.IsValid() && validation_state_none.IsValid());
+    } else if (validation_state_merkle.IsValid() || validation_state_pow.IsValid()) {
+        assert(validation_state_none.IsValid());
     }
     (void)block.GetHash();
     (void)block.ToString();

src/test/fuzz/utxo_snapshot.cpp

@@ -89,9 +89,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);
         }
@@ -171,9 +170,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

@@ -105,8 +105,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;
 }
@@ -366,4 +365,10 @@ 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()

src/test/validation_chainstate_tests.cpp

@@ -147,8 +147,8 @@ BOOST_FIXTURE_TEST_CASE(chainstate_update_tip, TestChain100Setup)
     // once it is changed to support multiple chainstates.
     {
         LOCK(::cs_main);
-        bool checked = CheckBlock(*pblockone, state, chainparams.GetConsensus());
-        BOOST_CHECK(checked);
+        state = CheckBlock(*pblockone, chainparams.GetConsensus());
+        BOOST_CHECK(state.IsValid());
         bool accepted = chainman.AcceptBlock(
             pblockone, state, &pindex, true, nullptr, &newblock, true);
         BOOST_CHECK(accepted);