Merge bitcoin/bitcoin#32487: blocks: avoid recomputing block header hash in ReadBlock

09ee8b7 node: avoid recomputing block hash in `ReadBlock` (Lőrinc)
2bf1732 test: exercise `ReadBlock` hash‑mismatch path (Lőrinc)

Pull request description:

  Eliminate one block header hash calculation per block-read by reusing the hash for:
  * proof‑of‑work verification;
  * (optional) integrity check against the supplied hash.

  This part of the code wasn't covered by tests either, so the first commit exercises this part first, before pushing the validation to the delegate method.

ACKs for top commit:
  maflcko:
    lgtm ACK 09ee8b7
  achow101:
    ACK 09ee8b7
  jonatack:
    ACK 09ee8b7
  pinheadmz:
    ACK 09ee8b7

Tree-SHA512: 43fe51b478ea574b6d4c952684b13ca54fb8cbd67c3b6c136f460122d9ee953cc70b88778537117eecea71ccb8d88311faeac21b866e11d117f1145973204ed4

Author: achow101

src/node/blockstorage.cpp

@@ -38,6 +38,7 @@
 
 #include <cstddef>
 #include <map>
+#include <optional>
 #include <unordered_map>
 
 namespace kernel {
@@ -989,7 +990,7 @@ bool BlockManager::WriteBlockUndo(const CBlockUndo& blockundo, BlockValidationSt
     return true;
 }
 
-bool BlockManager::ReadBlock(CBlock& block, const FlatFilePos& pos) const
+bool BlockManager::ReadBlock(CBlock& block, const FlatFilePos& pos, const std::optional<uint256>& expected_hash) const
 {
     block.SetNull();
 
@@ -1007,8 +1008,10 @@ bool BlockManager::ReadBlock(CBlock& block, const FlatFilePos& pos) const
         return false;
     }
 
+    const auto block_hash{block.GetHash()};
+
     // Check the header
-    if (!CheckProofOfWork(block.GetHash(), block.nBits, GetConsensus())) {
+    if (!CheckProofOfWork(block_hash, block.nBits, GetConsensus())) {
         LogError("Errors in block header at %s while reading block", pos.ToString());
         return false;
     }
@@ -1019,21 +1022,19 @@ bool BlockManager::ReadBlock(CBlock& block, const FlatFilePos& pos) const
         return false;
     }
 
+    if (expected_hash && block_hash != *expected_hash) {
+        LogError("GetHash() doesn't match index at %s while reading block (%s != %s)",
+                 pos.ToString(), block_hash.ToString(), expected_hash->ToString());
+        return false;
+    }
+
     return true;
 }
 
 bool BlockManager::ReadBlock(CBlock& block, const CBlockIndex& index) const
 {
     const FlatFilePos block_pos{WITH_LOCK(cs_main, return index.GetBlockPos())};
-
-    if (!ReadBlock(block, block_pos)) {
-        return false;
-    }
-    if (block.GetHash() != index.GetBlockHash()) {
-        LogError("GetHash() doesn't match index for %s at %s while reading block", index.ToString(), block_pos.ToString());
-        return false;
-    }
-    return true;
+    return ReadBlock(block, block_pos, index.GetBlockHash());
 }
 
 bool BlockManager::ReadRawBlock(std::vector<uint8_t>& block, const FlatFilePos& pos) const

src/node/blockstorage.h

@@ -411,7 +411,7 @@ class BlockManager
     void UnlinkPrunedFiles(const std::set<int>& setFilesToPrune) const;
 
     /** Functions for disk access for blocks */
-    bool ReadBlock(CBlock& block, const FlatFilePos& pos) const;
+    bool ReadBlock(CBlock& block, const FlatFilePos& pos, const std::optional<uint256>& expected_hash = {}) const;
     bool ReadBlock(CBlock& block, const CBlockIndex& index) const;
     bool ReadRawBlock(std::vector<uint8_t>& block, const FlatFilePos& pos) const;
 

src/test/blockmanager_tests.cpp

@@ -137,6 +137,16 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_block_data_availability, TestChain100Setup)
     BOOST_CHECK(!blockman.CheckBlockDataAvailability(tip, *last_pruned_block));
 }
 
+BOOST_FIXTURE_TEST_CASE(blockmanager_readblock_hash_mismatch, TestingSetup)
+{
+    CBlockIndex* fake_index{WITH_LOCK(m_node.chainman->GetMutex(), return m_node.chainman->ActiveChain().Tip())};
+    fake_index->phashBlock = &uint256::ONE; // invalid block hash
+
+    ASSERT_DEBUG_LOG("GetHash() doesn't match index");
+    CBlock dummy;
+    BOOST_CHECK(!m_node.chainman->m_blockman.ReadBlock(dummy, *fake_index));
+}
+
 BOOST_AUTO_TEST_CASE(blockmanager_flush_block_file)
 {
     KernelNotifications notifications{Assert(m_node.shutdown_request), m_node.exit_status, *Assert(m_node.warnings)};