Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
PastaPastaPasta merged 6 commits into from
Apr 11, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f7c43a2
perf: pass pre-computed block hash through reindex path to avoid redu…
UdjinM6 Apr 6, 2026
ec87196
assert: add debug-only invariant to verify known_hash matches block hash
UdjinM6 Apr 7, 2026
01e72cb
test: exercise CheckBlock and AcceptBlock with pre-computed known_hash
UdjinM6 Apr 7, 2026
89d8c45
assert: use ASSERT_IF_DEBUG for known_hash invariants
UdjinM6 Apr 10, 2026
01af278
refactor: return std::optional<uint256> from ReadBlockFromDisk(FlatFi…
UdjinM6 Apr 10, 2026
6e138f0
review: document known_hash safety and fix test to exercise reindex path
UdjinM6 Apr 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 13 additions & 8 deletions src/node/blockstorage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -745,39 +745,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());
}
Expand Down
3 changes: 2 additions & 1 deletion src/node/blockstorage.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <txdb.h>

#include <cstdint>
#include <optional>
#include <unordered_map>
#include <vector>

Expand Down Expand Up @@ -221,7 +222,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);
Expand Down
33 changes: 33 additions & 0 deletions src/test/validation_block_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Comment thread
UdjinM6 marked this conversation as resolved.
}

/**
* Test that mempool updates happen atomically with reorgs.
*
Expand Down
38 changes: 24 additions & 14 deletions src/validation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3971,18 +3971,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.
Expand Down Expand Up @@ -4182,11 +4185,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)};
Expand Down Expand Up @@ -4320,7 +4323,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) {
Expand All @@ -4343,7 +4346,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>();

Expand All @@ -4355,7 +4358,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)};
Comment on lines +4364 to +4367
Copy link
Copy Markdown

@thepastaclaw thepastaclaw Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: known_hash correctness relies on implicit PoW guard rather than explicit validation

AcceptBlock derives hash from known_hash (line 4363) and threads it through AcceptBlockHeader and CheckBlock. Both review agents flagged that the only explicit consistency check is ASSERT_IF_DEBUG (lines 4361, 4192, 3980), which compiles to nothing in release builds.

However, this is not unguarded in practice: AcceptBlockHeader calls CheckBlockHeader(block, hash, ...) at line 4213, which performs CheckProofOfWork(hash, block.nBits, ...) at runtime. A mismatched known_hash will almost certainly fail this check because only the true block hash satisfies the difficulty target — the PoW check IS a runtime rejection path.

The residual concern is that this protection is incidental rather than documented. If a future change introduced a fCheckPOW=false path for reindex callers (plausible as a further optimization), the implicit guard would be silently lost. Consider adding a brief comment at the known_hash parameter documenting that PoW validation serves as the runtime consistency check, or upgrading one of the ASSERT_IF_DEBUG calls to an Assume() that logs in release builds without aborting.

source: ['claude', 'codex']

🤖 Fix this with AI agents 
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/validation.cpp`:
- [SUGGESTION] lines 4361-4364: known_hash correctness relies on implicit PoW guard rather than explicit validation
  `AcceptBlock` derives `hash` from `known_hash` (line 4363) and threads it through `AcceptBlockHeader` and `CheckBlock`. Both review agents flagged that the only explicit consistency check is `ASSERT_IF_DEBUG` (lines 4361, 4192, 3980), which compiles to nothing in release builds.

However, this is not unguarded in practice: `AcceptBlockHeader` calls `CheckBlockHeader(block, hash, ...)` at line 4213, which performs `CheckProofOfWork(hash, block.nBits, ...)` at runtime. A mismatched `known_hash` will almost certainly fail this check because only the true block hash satisfies the difficulty target — the PoW check IS a runtime rejection path.

The residual concern is that this protection is incidental rather than documented. If a future change introduced a `fCheckPOW=false` path for reindex callers (plausible as a further optimization), the implicit guard would be silently lost. Consider adding a brief comment at the `known_hash` parameter documenting that PoW validation serves as the runtime consistency check, or upgrading one of the `ASSERT_IF_DEBUG` calls to an `Assume()` that logs in release builds without aborting.

CheckBlockIndex();

if (!accepted_header)
Expand Down Expand Up @@ -4394,7 +4403,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;
Expand Down Expand Up @@ -5146,7 +5155,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()) {
Expand Down Expand Up @@ -5179,14 +5188,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++;
Expand Down
7 changes: 4 additions & 3 deletions src/validation.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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:
Expand Down
Loading