perf: pass pre-computed block hash through reindex path to avoid redundant X11 computations#7272
Conversation
…ndant X11 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. Thread the already-computed hash through AcceptBlock, AcceptBlockHeader, CheckBlock, and ReadBlockFromDisk to eliminate redundant X11 computations (from 3-5 per block down to 1). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
Review GateCommit:
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReadBlockFromDisk now returns std::optional (the block hash) instead of bool; failures return nullopt. CheckBlock gains an optional known_hash parameter and uses it for PoW/header checks when provided. CChainState::AcceptBlock similarly accepts an optional known_hash and forwards the hash into ChainstateManager::AcceptBlockHeader, whose signature now requires a const uint256& hash. LoadExternalBlockFile and out-of-order import logic propagate the disk-read hash into logging, queuing, and AcceptBlock calls. A unit test verifies acceptance using a precomputed known hash. Sequence Diagram(s)sequenceDiagram
participant Disk
participant Reader as ReadBlockFromDisk
participant Loader as LoadExternalBlockFile
participant Chain as CChainState::AcceptBlock
participant Check as CheckBlock
participant Header as ChainstateManager::AcceptBlockHeader
participant Index as CBlockIndex
Disk->>Reader: read raw block bytes
Reader->>Reader: parse block, compute hash (uint256)
Reader-->>Loader: return block + optional hash
Loader->>Chain: AcceptBlock(block, dbp, known_hash=&hash)
Chain->>Check: CheckBlock(block, ..., known_hash=&hash)
Check-->>Chain: validation result
Chain->>Header: AcceptBlockHeader(header, state, ppindex, hash)
Header->>Index: find/create index entry using hash
Index-->>Header: index entry
Header-->>Chain: header acceptance result
Chain-->>Loader: finalize acceptance / enqueue children with known hashes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/validation.cpp (1)
3974-3985: Add a debug-only invariant for caller-supplied hashes.These entry points now trust the provided hash for PoW, duplicate detection, and
block.fChecked. A release-freeassertthat the supplied value matches the header would catch future misuse before it silently validates or indexes the wrong block.🛡️ Suggested guard
bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW, bool fCheckMerkleRoot, const uint256* known_hash) { + assert(!known_hash || *known_hash == block.GetHash()); // These are checks that are independent of context.bool ChainstateManager::AcceptBlockHeader(const CBlockHeader& block, BlockValidationState& state, CBlockIndex** ppindex, const uint256& hash) { AssertLockHeld(cs_main); + assert(hash == block.GetHash()); // TODO : ENABLE BLOCK CACHE IN SPECIFIC CASESAlso applies to: 4185-4209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/validation.cpp` around lines 3974 - 3985, Add a debug-only invariant in CheckBlock to ensure a caller-supplied known_hash actually matches the header-derived hash before trusting it: assert that known_hash is null or *known_hash == block.GetHash() near the start of CheckBlock (before using known_hash for PoW/dup detection or checking block.fChecked), so misuse is caught in debug builds; apply the same assert in the other similar entry point around lines 4185-4209 that also accepts a known_hash parameter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/validation.cpp`:
- Around line 3974-3985: Add a debug-only invariant in CheckBlock to ensure a
caller-supplied known_hash actually matches the header-derived hash before
trusting it: assert that known_hash is null or *known_hash == block.GetHash()
near the start of CheckBlock (before using known_hash for PoW/dup detection or
checking block.fChecked), so misuse is caught in debug builds; apply the same
assert in the other similar entry point around lines 4185-4209 that also accepts
a known_hash parameter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8d4ed55e-f159-4e9e-9bdf-fa5b36eb6b65
📒 Files selected for processing (4)
src/node/blockstorage.cppsrc/node/blockstorage.hsrc/validation.cppsrc/validation.h
Add assert() checks in CheckBlock, AcceptBlockHeader, and AcceptBlock to ensure caller-supplied pre-computed hashes match block.GetHash(). Catches misuse in debug builds; compiles out in release builds. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/validation_block_tests.cpp (1)
205-237: Well-structured test for the known_hash optimization path.The test correctly exercises the new
known_hashparameter for bothCheckBlockandAcceptBlock, following the existing test patterns in this file. Thecs_mainlock is properly acquired forAcceptBlock, and the use ofBOOST_REQUIREfor setup vsBOOST_CHECKfor assertions is appropriate.Consider strengthening the assertions to verify state validity and the returned block index:
💡 Optional: additional assertions for robustness
// CheckBlock with correct known_hash should succeed { BlockValidationState state; BOOST_CHECK(CheckBlock(*good, state, chainparams.GetConsensus(), /*fCheckPOW=*/true, /*fCheckMerkleRoot=*/true, &hash)); + BOOST_CHECK(state.IsValid()); } // AcceptBlock with correct known_hash should succeed { LOCK(::cs_main); BlockValidationState state; CBlockIndex* pindex = nullptr; bool newblock = false; BOOST_CHECK(m_node.chainman->ActiveChainstate().AcceptBlock( good, state, &pindex, /*fRequested=*/true, /*dbp=*/nullptr, &newblock, &hash)); + BOOST_CHECK(state.IsValid()); BOOST_CHECK(newblock); + BOOST_CHECK(pindex != nullptr); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/validation_block_tests.cpp` around lines 205 - 237, Add assertions after AcceptBlock and CheckBlock to verify the validation state and returned block index: after CheckBlock inspect BlockValidationState (state) for validity (e.g. state.IsValid()), and after AcceptBlock assert that pindex is non-null, newblock is true (already checked), and that pindex->GetBlockHash() (or pindex->GetBlockHash if accessor differs) equals the expected hash; also assert state.IsValid() to ensure no validation errors were recorded. Use the existing local variables state, pindex, newblock, and hash to implement these checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/test/validation_block_tests.cpp`:
- Around line 205-237: Add assertions after AcceptBlock and CheckBlock to verify
the validation state and returned block index: after CheckBlock inspect
BlockValidationState (state) for validity (e.g. state.IsValid()), and after
AcceptBlock assert that pindex is non-null, newblock is true (already checked),
and that pindex->GetBlockHash() (or pindex->GetBlockHash if accessor differs)
equals the expected hash; also assert state.IsValid() to ensure no validation
errors were recorded. Use the existing local variables state, pindex, newblock,
and hash to implement these checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7ba88749-624f-4c14-a141-7e4814d419c1
📒 Files selected for processing (2)
src/test/validation_block_tests.cppsrc/validation.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/validation.cpp
UdjinM6
force-pushed
the
perf/reindex-pass-hash-through
branch
from
358ba26 to
4618f52
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/test/validation_block_tests.cpp`:
- Around line 233-239: The test currently uses non-fatal BOOST_CHECK for
critical results of ActiveChainstate().AcceptBlock, which can leave pindex null
and lead to a dereference; change the critical assertions to fatal checks (use
BOOST_REQUIRE) for AcceptBlock, state.IsValid(), and newblock (e.g., replace
BOOST_CHECK calls that validate AcceptBlock, state, and newblock with
BOOST_REQUIRE) or add an explicit null check guard before dereferencing pindex;
update references in this block to use BOOST_REQUIRE for AcceptBlock,
state.IsValid(), and newblock (or add a check for pindex != nullptr before using
pindex->GetBlockHash()) so the test stops on failure and avoids undefined
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 56ee1d2d-2162-4dc5-b393-6b82607b4640
📒 Files selected for processing (1)
src/test/validation_block_tests.cpp
Add checkblock_accept_known_hash test to cover the reindex optimization path where a caller supplies a pre-computed block hash. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
UdjinM6
force-pushed
the
perf/reindex-pass-hash-through
branch
from
4618f52 to
147d89a
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The hash-reuse optimization is wired through the intended reindex path, but the new API relies on debug-only assertions for a consensus-critical invariant and the added unit test does not cover the actual import/reindex flow the patch changes.
Reviewed commit: 4618f52
🔴 1 blocking | 🟡 1 suggestion(s)
🤖 Prompt for all review comments 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`:
- [BLOCKING] lines 3980-4403: `known_hash` is only debug-checked before being trusted as the block identity
The new `known_hash` parameter is guarded only by `assert(!known_hash || *known_hash == block.GetHash())` in `CheckBlock()` and `AcceptBlock()`, plus `assert(hash == block.GetHash())` in `AcceptBlockHeader()`. In release builds those checks disappear, but the supplied hash is still fed into `CheckBlockHeader()` for PoW/devnet-genesis validation, into duplicate and chainlock checks in `AcceptBlockHeader()`, and into `AddToBlockIndex()` as the map key. A stale caller-supplied hash would therefore be silently accepted and used to index the block under the wrong hash instead of producing a clean validation failure. Because this is consensus-critical validation code, the mismatch needs a runtime rejection path rather than a debug-only contract.
In `src/test/validation_block_tests.cpp`:
- [SUGGESTION] lines 205-240: The new test does not exercise the actual reindex/import hash-reuse path
The new test proves that `CheckBlock()` and `AcceptBlock()` accept a manually precomputed `good->GetHash()`, but it never covers the production flow added by this patch: `LoadExternalBlockFile()` computing `header.GetHash()`, `ReadBlockFromDisk(..., &blockhash)` returning the cached hash, and the recursive out-of-order child import path forwarding that hash into `AcceptBlock()`. If one of those reindex call sites stopped forwarding the cached hash, this test would still pass, so it does not protect the optimization the PR is introducing.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I reviewed the PR and validated the existing CodeRabbit thread. I did not have separate additional findings to post under my own review.
Reviewed commit: 147d89a
| // Check for duplicate | ||
| uint256 hash = block.GetHash(); | ||
|
|
||
| assert(hash == block.GetHash()); |
There was a problem hiding this comment.
AcceptBlockHeader still calculate GetHash for this assert; there's no performance benefits then, but removing it makes consensus validation implementation less robust IMO
|
|
||
| auto start = Now<SteadyMicroseconds>(); | ||
|
|
||
| assert(!known_hash || *known_hash == block.GetHash()); |
There was a problem hiding this comment.
CheckBlock still calculate GetHash for this assert; there's no performance benefits then but removing it makes consensus validation less robust IMO
| } | ||
|
|
||
| bool ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos, const Consensus::Params& consensusParams) | ||
| bool ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos, const Consensus::Params& consensusParams, uint256* hash_out) |
There was a problem hiding this comment.
consider ReadBlockFromDisk to return std::optional
So, if (!ReadBlockFromDisk) will still be valid without changing semantic all over codebase; but in the places where it's important such as loop over multiple blocks in LoadExternalBlockFile where hash is actually used, the code maybe rewritten.
std::optional is very explicit about life term of this variable and less likely the pointer will be dangling / invalid ever
Switch the known_hash debug invariants from assert() to ASSERT_IF_DEBUG() so the extra X11 hashing only occurs in debug builds. Dash always compiles with assertions enabled, so plain assert() would add unnecessary overhead in release builds. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…lePos) Replace the uint256* hash_out output parameter with a std::optional return value. The optional is empty on failure, contains the block hash on success. This makes the hash lifetime explicit and eliminates the risk of dangling pointers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
UdjinM6
force-pushed
the
perf/reindex-pass-hash-through
branch
from
147d89a to
01af278
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Both agents converge on the same concern: the known_hash optimization uses ASSERT_IF_DEBUG as its only explicit validation. However, both miss that CheckBlockHeader's PoW check (line 3960) provides an implicit runtime guard — a wrong hash will fail proof-of-work validation before reaching AddToBlockIndex. This downgrades the concern from blocking to suggestion-level. The test coverage finding is valid but low-priority.
Reviewed commit: 01af278
🟡 2 suggestion(s)
🤖 Prompt for all review comments 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.
In `src/test/validation_block_tests.cpp`:
- [SUGGESTION] lines 219-241: Test exercises known_hash through AcceptBlockHeader but not the reindex-specific CheckBlock path
The `checkblock_accept_known_hash` test calls `CheckBlock` at line 222 (which sets the mutable `fChecked = true`), then calls `AcceptBlock` with the same block at line 233. Inside `AcceptBlock`, when `CheckBlock` is invoked again (validation.cpp:4403), the `fChecked` short-circuit at line 3982 skips the `known_hash` code path entirely. This means the test validates `AcceptBlockHeader` with the precomputed hash (including the PoW guard), but does not exercise the scenario this optimization targets: the reindex path where `LoadExternalBlockFile` calls `AcceptBlock` on a fresh block with `fChecked = false`.
To better match the production reindex path, the test could skip the standalone `CheckBlock` call before `AcceptBlock`, so that `fChecked` is false when `AcceptBlock`'s internal `CheckBlock` runs with `known_hash`.
| 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)}; |
There was a problem hiding this comment.
🟡 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.
| // CheckBlock with correct known_hash should succeed | ||
| { | ||
| BlockValidationState state; | ||
| BOOST_CHECK(CheckBlock(*good, state, chainparams.GetConsensus(), | ||
| /*fCheckPOW=*/true, /*fCheckMerkleRoot=*/true, &hash)); | ||
| BOOST_CHECK(state.IsValid()); | ||
| } | ||
|
|
||
| // AcceptBlock with correct known_hash should succeed | ||
| { | ||
| 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); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Test exercises known_hash through AcceptBlockHeader but not the reindex-specific CheckBlock path
The checkblock_accept_known_hash test calls CheckBlock at line 222 (which sets the mutable fChecked = true), then calls AcceptBlock with the same block at line 233. Inside AcceptBlock, when CheckBlock is invoked again (validation.cpp:4403), the fChecked short-circuit at line 3982 skips the known_hash code path entirely. This means the test validates AcceptBlockHeader with the precomputed hash (including the PoW guard), but does not exercise the scenario this optimization targets: the reindex path where LoadExternalBlockFile calls AcceptBlock on a fresh block with fChecked = false.
To better match the production reindex path, the test could skip the standalone CheckBlock call before AcceptBlock, so that fChecked is false when AcceptBlock's internal CheckBlock runs with known_hash.
source: ['claude']
🤖 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/test/validation_block_tests.cpp`:
- [SUGGESTION] lines 219-241: Test exercises known_hash through AcceptBlockHeader but not the reindex-specific CheckBlock path
The `checkblock_accept_known_hash` test calls `CheckBlock` at line 222 (which sets the mutable `fChecked = true`), then calls `AcceptBlock` with the same block at line 233. Inside `AcceptBlock`, when `CheckBlock` is invoked again (validation.cpp:4403), the `fChecked` short-circuit at line 3982 skips the `known_hash` code path entirely. This means the test validates `AcceptBlockHeader` with the precomputed hash (including the PoW guard), but does not exercise the scenario this optimization targets: the reindex path where `LoadExternalBlockFile` calls `AcceptBlock` on a fresh block with `fChecked = false`.
To better match the production reindex path, the test could skip the standalone `CheckBlock` call before `AcceptBlock`, so that `fChecked` is false when `AcceptBlock`'s internal `CheckBlock` runs with `known_hash`.
Add comment explaining that CheckProofOfWork serves as the runtime consistency check for known_hash in release builds. Restructure test to skip standalone CheckBlock before AcceptBlock so that fChecked remains false and AcceptBlock's internal CheckBlock actually exercises the known_hash code path, mirroring the reindex flow. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
4 participants
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 inAcceptBlockHeader, again inCheckBlock, and more for out-of-order blocks.See #6610 for more info.
What was done?
Thread the already-computed hash through
AcceptBlock,AcceptBlockHeader,CheckBlock, andReadBlockFromDiskto eliminate redundant X11 computations (from 3-5 per block down to 1).How Has This Been Tested?
Reindexed on testnet with
--stopatheight=1to confirm performance is on par with #6610. Then competed reindex on testnet with no issues. Tests are green too.Breaking Changes
n/a
Checklist: