Merge #7272: perf: pass pre-computed block hash through reindex path to avoid redundant X11 computations

6e138f0 review: document known_hash safety and fix test to exercise reindex path (UdjinM6)
01af278 refactor: return std::optional<uint256> from ReadBlockFromDisk(FlatFilePos) (UdjinM6)
89d8c45 assert: use ASSERT_IF_DEBUG for known_hash invariants (UdjinM6)
01e72cb test: exercise CheckBlock and AcceptBlock with pre-computed known_hash (UdjinM6)
ec87196 assert: add debug-only invariant to verify known_hash matches block hash (UdjinM6)
f7c43a2 perf: pass pre-computed block hash through reindex path to avoid redundant X11 (UdjinM6)

Pull request description:

  ## Issue being fixed or feature implemented
  During reindex, the X11 block header hash was computed multiple times for the same block: once in `LoadExternalBlockFile`, again in `AcceptBlockHeader`, again in `CheckBlock`, and more for out-of-order blocks.

  See #6610 for more info.

  ## What was done?
  Thread the already-computed hash through `AcceptBlock`, `AcceptBlockHeader`, `CheckBlock`, and `ReadBlockFromDisk` to eliminate redundant X11 computations (from 3-5 per block down to 1).

  ## How Has This Been Tested?
  Reindexed on testnet with `--stopatheight=1` to confirm performance is on par with #6610. Then competed reindex on testnet with no issues. Tests are green too.

  ## Breaking Changes
  n/a

  ## Checklist:
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

Top commit has no ACKs.

Tree-SHA512: 473fb7bf177c721f8d0f50e04394015554e779b26b1f004e52efb7803d267a13692a0f6a31426f084f1fbc25d2db44a56b0621fe7ed1b65c9c49ed322d98f1e9

Author: PastaPastaPasta

src/node/blockstorage.cpp

@@ -756,39 +756,44 @@ bool BlockManager::WriteUndoDataForBlock(const CBlockUndo& blockundo, BlockValid
     return true;
 }
 
-bool ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos, const Consensus::Params& consensusParams)
+std::optional<uint256> ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos, const Consensus::Params& consensusParams)
 {
     block.SetNull();
 
     // Open history file to read
     CAutoFile filein(OpenBlockFile(pos, true), SER_DISK, CLIENT_VERSION);
     if (filein.IsNull()) {
-        return error("ReadBlockFromDisk: OpenBlockFile failed for %s", pos.ToString());
+        error("ReadBlockFromDisk: OpenBlockFile failed for %s", pos.ToString());
+        return std::nullopt;
     }
 
     // Read block
     try {
         filein >> block;
     } catch (const std::exception& e) {
-        return error("%s: Deserialize or I/O error - %s at %s", __func__, e.what(), pos.ToString());
+        error("%s: Deserialize or I/O error - %s at %s", __func__, e.what(), pos.ToString());
+        return std::nullopt;
     }
 
     // Check the header
-    if (!CheckProofOfWork(block.GetHash(), block.nBits, consensusParams)) {
-        return error("ReadBlockFromDisk: Errors in block header at %s", pos.ToString());
+    const uint256 hash{block.GetHash()};
+    if (!CheckProofOfWork(hash, block.nBits, consensusParams)) {
+        error("ReadBlockFromDisk: Errors in block header at %s", pos.ToString());
+        return std::nullopt;
     }
 
-    return true;
+    return hash;
 }
 
 bool ReadBlockFromDisk(CBlock& block, const CBlockIndex* pindex, const Consensus::Params& consensusParams)
 {
     const FlatFilePos block_pos{WITH_LOCK(cs_main, return pindex->GetBlockPos())};
 
-    if (!ReadBlockFromDisk(block, block_pos, consensusParams)) {
+    const auto hash{ReadBlockFromDisk(block, block_pos, consensusParams)};
+    if (!hash) {
         return false;
     }
-    if (block.GetHash() != pindex->GetBlockHash()) {
+    if (*hash != pindex->GetBlockHash()) {
         return error("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match index for %s at %s",
                      pindex->ToString(), block_pos.ToString());
     }

src/node/blockstorage.h

@@ -15,6 +15,7 @@
 #include <txdb.h>
 
 #include <cstdint>
+#include <optional>
 #include <unordered_map>
 #include <vector>
 
@@ -231,7 +232,7 @@ fs::path GetBlockPosFilename(const FlatFilePos& pos);
 void UnlinkPrunedFiles(const std::set<int>& setFilesToPrune);
 
 /** Functions for disk access for blocks */
-bool ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos, const Consensus::Params& consensusParams);
+std::optional<uint256> ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos, const Consensus::Params& consensusParams);
 bool ReadBlockFromDisk(CBlock& block, const CBlockIndex* pindex, const Consensus::Params& consensusParams);
 
 bool UndoReadFromDisk(CBlockUndo& blockundo, const CBlockIndex* pindex);

src/test/validation_block_tests.cpp

@@ -202,6 +202,39 @@ BOOST_AUTO_TEST_CASE(processnewblock_signals_ordering)
     BOOST_CHECK_EQUAL(sub->m_expected_tip, m_node.chainman->ActiveChain().Tip()->GetBlockHash());
 }
 
+/**
+ * Test that AcceptBlock works correctly when a pre-computed known_hash is
+ * supplied, exercising the reindex optimization path through AcceptBlockHeader
+ * and CheckBlock.
+ */
+BOOST_AUTO_TEST_CASE(checkblock_accept_known_hash)
+{
+    bool ignored;
+    BOOST_REQUIRE(Assert(m_node.chainman)->ProcessNewBlock(
+        std::make_shared<CBlock>(Params().GenesisBlock()), true, &ignored));
+
+    auto good = GoodBlock(Params().GenesisBlock().GetHash());
+    const uint256 hash{good->GetHash()};
+
+    // AcceptBlock with correct known_hash should succeed.
+    // Do not call CheckBlock beforehand: that would set fChecked=true,
+    // causing AcceptBlock's internal CheckBlock to short-circuit and skip
+    // the known_hash path. Keeping fChecked=false mirrors the reindex flow.
+    {
+        LOCK(::cs_main);
+        BlockValidationState state;
+        CBlockIndex* pindex = nullptr;
+        bool newblock = false;
+        BOOST_REQUIRE(m_node.chainman->ActiveChainstate().AcceptBlock(
+            good, state, &pindex, /*fRequested=*/true,
+            /*dbp=*/nullptr, &newblock, &hash));
+        BOOST_REQUIRE(state.IsValid());
+        BOOST_REQUIRE(newblock);
+        BOOST_REQUIRE(pindex != nullptr);
+        BOOST_CHECK_EQUAL(pindex->GetBlockHash(), hash);
+    }
+}
+
 /**
  * Test that mempool updates happen atomically with reorgs.
  *

src/validation.cpp

@@ -3981,18 +3981,21 @@ static bool CheckBlockHeader(const CBlockHeader& block, const uint256& hash, Blo
     return true;
 }
 
-bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW, bool fCheckMerkleRoot)
+bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW, bool fCheckMerkleRoot, const uint256* known_hash)
 {
     // These are checks that are independent of context.
 
     auto start = Now<SteadyMicroseconds>();
 
+    ASSERT_IF_DEBUG(!known_hash || *known_hash == block.GetHash());
+
     if (block.fChecked)
         return true;
 
     // Check that the header is valid (particularly PoW).  This is mostly
     // redundant with the call in AcceptBlockHeader.
-    if (!CheckBlockHeader(block, block.GetHash(), state, consensusParams, fCheckPOW))
+    const uint256 hash{known_hash ? *known_hash : block.GetHash()};
+    if (!CheckBlockHeader(block, hash, state, consensusParams, fCheckPOW))
         return false;
 
     // Check the merkle root.
@@ -4192,11 +4195,11 @@ static bool ContextualCheckBlock(const CBlock& block, BlockValidationState& stat
     return true;
 }
 
-bool ChainstateManager::AcceptBlockHeader(const CBlockHeader& block, BlockValidationState& state, CBlockIndex** ppindex)
+bool ChainstateManager::AcceptBlockHeader(const CBlockHeader& block, BlockValidationState& state, CBlockIndex** ppindex, const uint256& hash)
 {
     AssertLockHeld(cs_main);
-    // Check for duplicate
-    uint256 hash = block.GetHash();
+
+    ASSERT_IF_DEBUG(hash == block.GetHash());
 
     // TODO : ENABLE BLOCK CACHE IN SPECIFIC CASES
     BlockMap::iterator miSelf{m_blockman.m_block_index.find(hash)};
@@ -4330,7 +4333,7 @@ bool ChainstateManager::ProcessNewBlockHeaders(const std::vector<CBlockHeader>&
         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)};
+            bool accepted{AcceptBlockHeader(header, state, &pindex, header.GetHash())};
             ActiveChainstate().CheckBlockIndex();
 
             if (!accepted) {
@@ -4353,7 +4356,7 @@ bool ChainstateManager::ProcessNewBlockHeaders(const std::vector<CBlockHeader>&
 }
 
 /** Store block on disk. If dbp is non-nullptr, the file is known to already reside on disk */
-bool CChainState::AcceptBlock(const std::shared_ptr<const CBlock>& pblock, BlockValidationState& state, CBlockIndex** ppindex, bool fRequested, const FlatFilePos* dbp, bool* fNewBlock)
+bool CChainState::AcceptBlock(const std::shared_ptr<const CBlock>& pblock, BlockValidationState& state, CBlockIndex** ppindex, bool fRequested, const FlatFilePos* dbp, bool* fNewBlock, const uint256* known_hash)
 {
     auto start = Now<SteadyMicroseconds>();
 
@@ -4365,7 +4368,13 @@ bool CChainState::AcceptBlock(const std::shared_ptr<const CBlock>& pblock, Block
     CBlockIndex *pindexDummy = nullptr;
     CBlockIndex *&pindex = ppindex ? *ppindex : pindexDummy;
 
-    bool accepted_header{m_chainman.AcceptBlockHeader(block, state, &pindex)};
+    // If the caller supplies a pre-computed hash, verify it in debug builds.
+    // In release builds a wrong hash is still caught: AcceptBlockHeader calls
+    // CheckBlockHeader which runs CheckProofOfWork against the header's nBits.
+    ASSERT_IF_DEBUG(!known_hash || *known_hash == block.GetHash());
+
+    const uint256 hash{known_hash ? *known_hash : block.GetHash()};
+    bool accepted_header{m_chainman.AcceptBlockHeader(block, state, &pindex, hash)};
     CheckBlockIndex();
 
     if (!accepted_header)
@@ -4404,7 +4413,7 @@ bool CChainState::AcceptBlock(const std::shared_ptr<const CBlock>& pblock, Block
         if (pindex->nChainWork < nMinimumChainWork) return true;
     }
 
-    if (!CheckBlock(block, state, m_params.GetConsensus()) ||
+    if (!CheckBlock(block, state, m_params.GetConsensus(), true, true, &hash) ||
         !ContextualCheckBlock(block, state, m_chainman, pindex->pprev)) {
         if (state.IsInvalid() && state.GetResult() != BlockValidationResult::BLOCK_MUTATED) {
             pindex->nStatus |= BLOCK_FAILED_VALID;
@@ -5159,7 +5168,7 @@ void CChainState::LoadExternalBlockFile(
                         nRewind = blkdat.GetPos();
 
                         BlockValidationState state;
-                        if (AcceptBlock(pblock, state, nullptr, true, dbp, nullptr)) {
+                        if (AcceptBlock(pblock, state, nullptr, true, dbp, nullptr, &hash)) {
                             nLoaded++;
                         }
                         if (state.IsError()) {
@@ -5207,14 +5216,15 @@ void CChainState::LoadExternalBlockFile(
                     while (range.first != range.second) {
                         std::multimap<uint256, FlatFilePos>::iterator it = range.first;
                         std::shared_ptr<CBlock> pblockrecursive = std::make_shared<CBlock>();
-                        if (ReadBlockFromDisk(*pblockrecursive, it->second, m_params.GetConsensus())) {
-                            LogPrint(BCLog::REINDEX, "%s: Processing out of order child %s of %s\n", __func__, pblockrecursive->GetHash().ToString(),
+                        if (auto opt_hash{ReadBlockFromDisk(*pblockrecursive, it->second, m_params.GetConsensus())}) {
+                            const uint256& blockhash = *opt_hash;
+                            LogPrint(BCLog::REINDEX, "%s: Processing out of order child %s of %s\n", __func__, blockhash.ToString(),
                                     head.ToString());
                             LOCK(cs_main);
                             BlockValidationState dummy;
-                            if (AcceptBlock(pblockrecursive, dummy, nullptr, true, &it->second, nullptr)) {
+                            if (AcceptBlock(pblockrecursive, dummy, nullptr, true, &it->second, nullptr, &blockhash)) {
                                 nLoaded++;
-                                queue.push_back(pblockrecursive->GetHash());
+                                queue.push_back(blockhash);
                             }
                         }
                         range.first++;

src/validation.h

@@ -371,7 +371,7 @@ void InitScriptExecutionCache();
 /** Functions for validating blocks and updating the block tree */
 
 /** Context-independent validity checks */
-bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW = true, bool fCheckMerkleRoot = true);
+bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW = true, bool fCheckMerkleRoot = true, const uint256* known_hash = nullptr);
 
 /** Check a block is completely valid from start to finish (only works on top of our current best block) */
 bool TestBlockValidity(BlockValidationState& state,
@@ -705,7 +705,7 @@ class CChainState
         EXCLUSIVE_LOCKS_REQUIRED(!m_chainstate_mutex)
         LOCKS_EXCLUDED(::cs_main);
 
-    bool AcceptBlock(const std::shared_ptr<const CBlock>& pblock, BlockValidationState& state, CBlockIndex** ppindex, bool fRequested, const FlatFilePos* dbp, bool* fNewBlock) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+    bool AcceptBlock(const std::shared_ptr<const CBlock>& pblock, BlockValidationState& state, CBlockIndex** ppindex, bool fRequested, const FlatFilePos* dbp, bool* fNewBlock, const uint256* known_hash = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
     // Block (dis)connection on a given view:
     DisconnectResult DisconnectBlock(const CBlock& block, const CBlockIndex* pindex, CCoinsViewCache& view) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
@@ -915,7 +915,8 @@ class ChainstateManager
     bool AcceptBlockHeader(
         const CBlockHeader& block,
         BlockValidationState& state,
-        CBlockIndex** ppindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+        CBlockIndex** ppindex,
+        const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
     friend CChainState;
 
 public: