db/snapshotsync: don't return the wrong block when a frozen-height hash lookup misses

[Sahil-4555]

Issue

As per #23539 - BlockReader lets you ask for a block by its exact hash together with a height. That is supposed to mean "give me this block", not "give me whatever sits at this height".

Once a height is retired into snapshot files, the reader stopped honouring that. Segments are indexed by height and hold canonical blocks only, so a positional read there can only ever answer for one block - but the reader handed that block back without checking it against the hash the caller asked for. In practice: ask for a reorged-out block by its hash at a frozen height and you silently got the canonical sibling instead. Different transactions, different state root, no error.

This is the same code path devp2p uses to answer peers asking for bodies, receipts and headers by hash, and the one the Engine API uses for engine_getPayloadBodiesByHash. Both could serve a block nobody asked for, with nothing in the response saying so.

The db-backed path never had this problem - kv.Headers and kv.BlockBody are keyed by (number, hash), so a mismatch there is simply a miss.

Closes #23539

Fix

All four hash-taking methods - Header, BlockWithSenders, Body, BodyWithTransactions - now check the requested hash against the header the block files actually hold at that height, through a small shared frozenHashAt helper. Header and blockWithSenders already decode that header, so for them the comparison is free.

On a mismatch we do not just give up. We fall back to the (hash, number)-keyed db read first, and only return "not found" if that misses too.

That fallback matters because freezing and pruning do not happen at the same distance from the tip:

• retire runs up to curBlockNum - MaxReorgDepth (96), rounded to segment boundaries
• CanDeleteTo only prunes to (curBlockNum/1000)*1000 - 1024

which leaves a steady 24–1024 block window where a height is already covered by the files but the reorged-out sibling is still fully present in MDBX. Refusing it there would turn "wrong block" into "no block" for a block that is sitting right there. Once the height is genuinely pruned the db read misses and nil is the correct answer, so the fallback limits itself.

Two things came out of review and are fixed in the same pass:

• BodyRlp was flattening nil into an empty body. rlp.EncodeToBytes on a nil *types.Body returns 0xc0, not a zero-length slice, so AnswerGetBlockBodiesQuery's len(bodyRLP) == 0 guard never fired and the peer decoded a positive "this block is empty" claim. Both BlockReader.BodyRlp and RemoteBlockReader.BodyRlp now return nil for a nil body.
• HasSenders was the same bug class, returning a blanket true for any frozen height without looking at the hash. It now runs the same check.

A zero hash still means "read by height, no hash constraint". This is not a leftover — BlockByNumber deliberately leaves the hash empty for frozen heights (block_reader.go:1393-1406; the emptyHash bail-out sits inside the non-frozen branch), so every frozen-block read through it depends on this. The test asserts BlockByNumber directly so the dependency is visible.

Tests

TestBlockReaderFrozenHashLookup builds two branches forking at the same height, makes one canonical, then freezes and prunes past the segment end so the files are the only thing left that can answer. It covers a reorged-out sibling, a genuinely canonical hash asked for at its neighbour's height, the canonical hash resolving normally, and the zero-hash path.

TestBlockReaderFrozenHashFallsBackToDB freezes but deliberately skips the prune, pinning the window described above.

[Copilot]

Pull request overview

Prevents snapshot-backed block lookups from returning a canonical block when a different hash was requested.

Changes:

• Validates hashes for frozen header, body, and block lookups.
• Adds regression, wrong-height, zero-hash, and snapshot-boundary tests.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
db/snapshotsync/freezeblocks/block_reader.go	Adds frozen snapshot identity checks.
db/snapshotsync/freezeblocks/block_reader_frozen_hash_test.go	Tests frozen hash lookup behavior.

Suppressed comments (1)

db/snapshotsync/freezeblocks/block_reader.go:709

• BodyRlp delegates to this method and unconditionally RLP-encodes its result. When this branch returns a nil *types.Body, the encoder produces the non-empty byte sequence 0xc0; consequently AnswerGetBlockBodiesQuery appends it and the HasBlock implementations report true for the rejected hash. Add a nil check in BodyRlp before encoding (including the remote implementation for consistent semantics), and cover this path in the regression test.

	if !matches {
		if dbgLogs {
			log.Info(dbgPrefix + "requested hash is not canonical at this height")
		}
		return nil, nil

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Sahil-4555]

Correct. The frozen path did tolerate tx == nil (viewSingleFile falls back to r.sn), and routing through CanonicalHash broke that.

Fixed by removing CanonicalHash from this path entirely - the check is now frozenHashAt, which goes through viewSingleFile and handles a nil tx like the rest of the frozen path. The new db fallbacks are all guarded with if tx == nil.

[AskAlexSharov]

if hash == emptyHash { - it feels like all BlockReader methods need to follow same semantic? If BlockWithSenders method has this logic then IsCanonical also need this logic? then can pushdown if hash == emptyHash { inside IsCanonical?

[Sahil-4555]

I would rather not push it into IsCanonical. That method is the loop-termination condition in both fork validators (fork_validator.go:192/204/241 - "have we reached canonical yet"). If IsCanonical(emptyHash, N) starts returning true, those walks stop early and treat a fork as canonical.

Also, after this round the body methods don't call IsCanonical any more, so the zero-hash meaning now sits only in the reader's own read methods, which is where "no hash constraint" belongs.

[AskAlexSharov]

seems can move this check earlier?

[Sahil-4555]

Moved it, but the other way - it now sits after the viewSingleFile(Bodies) !ok early-out, so heights with no bodies file don't pay for it. Your later comment on this same line suggested the same thing.

If you actually meant earlier (above the db branch), let me know - I think that one would break the db path.

[AskAlexSharov]

BlockReader.Body() - I don't understand why this method can't return non-canonical-block info?

[Sahil-4555]

Fair question, and you are right that it should be able to. The old check proved canonical-ness, which is stronger than what the caller asked for.

Now Body/BodyWithTransactions compare the requested hash against the header the files hold at that height, and on mismatch fall back to rawdb.ReadBody/ReadBodyWithTransactions, which are (hash, number)-keyed. So a side-fork body is served as long as it is still in the db.

[AskAlexSharov]

seems this PR mixing 2 things:

• to check that hash is canonical
• to check that returned hash is equal to requested hash (like there)
  Better separate this 2 concerns. Then will be easier to review.

[Sahil-4555]

Agreed, and it is down to one concern now. Everything checks identity - "is the requested hash the block the files hold at this height". Canonical-ness is gone from these paths, no more IsCanonical/CanonicalHash.

[AskAlexSharov]

Identity - is new word which doesn't exists in Erigon. plz avoid.

[Sahil-4555]

Renamed. Tests are now TestBlockReaderFrozenHashLookup and TestBlockReaderFrozenHashFallsBackToDB, and the word is out of the comments too.

[AskAlexSharov]

Returning nil here drops a block that is still fully readable from the DB.

blockWithSenders only reaches rawdb.ReadBlockWithSenders(tx, hash, blockHeight) when blockHeight == 0 || maxBlockNumInFiles == 0 || blockHeight > maxBlockNumInFiles. So for any height at or below the snapshot tip the DB is never consulted, and on mismatch we now return nil instead of falling back to the hash-keyed read that would answer correctly.

Freeze and prune do not run at the same distance from the tip:

• retire goes up to curBlockNum - MaxReorgDepth (96 by default) - block_snapshots.go:180
• prune only goes to (curBlockNum/1_000)*1_000 - 1024 - CanDeleteTo, block_snapshots.go:188-200

That leaves roughly a 1000-block window that is frozen but not pruned. Side-fork blocks at those heights still have their kv.Headers, body, kv.EthTx and kv.HeaderNumber rows.

User-visible effect: eth_getBlockByHash(sideForkHash) resolves the number fine (BlockByHash -> HeaderNumber -> rawdb.ReadHeaderNumber, which is hash-keyed and finds it), then BlockWithSenders returns nil, so the RPC answers "block not found" for a block that is entirely present in the DB. Before this PR the answer was the wrong block; now it is no block, when the right block was available.

A fallback instead of a bare return would keep the data reachable:

if hash != emptyHash && h.Hash() != hash {
    release()
    if tx != nil {
        return rawdb.ReadBlockWithSenders(tx, hash, blockHeight)
    }
    return
}

Once the height is genuinely pruned the DB read misses and nil is the correct answer, so the fallback is self-limiting.

[Sahil-4555]

You are right, and thanks for working out the numbers - I checked them and they hold. Retire goes to tip-96 segment-rounded, CanDeleteTo holds (tip/1000)*1000 - 1024, so the window is roughly 24-1024 blocks depending on where the tip sits.

Added the fallback basically as you wrote it - on mismatch blockWithSenders now does rawdb.ReadBlockWithSenders(tx, hash, blockHeight), guarded on tx != nil. Self-limiting as you say: once the height is really pruned the read misses and nil is correct.

Also added TestBlockReaderFrozenHashFallsBackToDB, which freezes but deliberately does not prune, so it pins this window. It fails on the merge-base.

[AskAlexSharov]

Same DB-fallback gap as in blockWithSenders, and here it turns into a hard error on the engine API path.

For blockHeight <= maxBlockNumInFiles this method never reads the DB, so on a non-canonical hash it now returns nil, nil even when the side-fork body is still in kv.BlockBody / kv.EthTx (frozen-but-unpruned window, ~1000 blocks - see the CanDeleteTo vs retire-distance note on blockWithSenders).

fork_validator.go:225 pairs this call with Header():

header, criticalError = fv.blockReader.Header(fv.ctx, tx, currentHash, unwindPoint)
...
body, criticalError = fv.blockReader.BodyWithTransactions(fv.ctx, tx, currentHash, unwindPoint)
...
if body == nil {
    criticalError = fmt.Errorf("found chain gap in block body at hash %s, number %d", currentHash, unwindPoint)
    return
}

Header() reads the DB first (rawdb.ReadHeader(tx, hash, blockHeight)), so it still returns the side-fork header. BodyWithTransactions does not, so it returns nil. The pair header != nil, body == nil is exactly the branch that raises found chain gap in block body - a critical error surfaced out of ValidatePayload to engine_newPayload, where before the PR the walk simply continued.

A rawdb.ReadBodyWithTransactions(tx, hash, blockHeight) fallback on the mismatch path fixes both this and the RPC case, and misses harmlessly once the height is really pruned.

[Sahil-4555]

Same fix here - BodyWithTransactions falls back to rawdb.ReadBodyWithTransactions on mismatch.

That also removes the header != nil, body == nil pair you pointed at, so the found chain gap in block body path in fork_validator.go:225 is not reachable through this any more.

[AskAlexSharov]

This is the concrete reason Body() can no longer return non-canonical block info, and I think it is the wrong invariant.

Header() and blockWithSenders() enforce identity: does the decoded header hash equal the requested hash. Body() and BodyWithTransactions() enforce canonical-ness via IsCanonical -> CanonicalHash. Canonical-ness is strictly stronger, and it is what makes a legitimately-requested side-fork body unreachable.

The two also answer from different sources. CanonicalHash reads kv.HeaderCanonical first and only then the snapshot, so Body()'s answer depends on a table that says nothing about the block the caller asked for - it only says which block won at that height.

For a body read there is no decoded header to compare against, so identity cannot be checked directly here. But the same effect is available for free by keying the DB read: rawdb.ReadBody(tx, hash, blockHeight) is (hash, number)-keyed, so reading the DB on the frozen path answers the identity question and returns the correct body in one step, instead of proving canonical-ness and then discarding the request.

That also removes the divergence with Header(), which today can return a side-fork header at a height where Body() refuses its body - see exec_module.go:528 (header == nil || body == nil -> ExecutionStatusMissingSegment) and stage_senders.go:229-243, which both fetch the pair.

[Sahil-4555]

Agreed, identity was the right invariant and canonical-ness was too strong.

Both body methods now compare against the segment header - the same thing Header/blockWithSenders compare against - and fall back to the (hash, number)-keyed db read on mismatch. So Header and Body no longer disagree at the same height, which was the exec_module.go:528 / stage_senders.go problem.

[AskAlexSharov]

This whole subtest passes on main, without the fix - none of its three assertions reaches the snapshot identity check.

rawdb.PruneBlocks(rwTx, uint64(segSize) /* 1000 */, 2*segSize) computes stopAtBlock := min(blockTo, blockFrom+limit) = 1000 and breaks at if n >= stopAtBlock (accessors_chain.go:879-891), so it prunes [1, 1000) and block 1000 stays in the DB.

• Header(lastFrozenBlock.Hash(), lastFrozen /* 1000 */) - Header() starts with rawdb.ReadHeader(tx, hash, blockHeight) and returns early on a hit. Block 1000 survived pruning, so this is a plain DB read.
• Header(firstNonFrozenBlock.Hash(), firstNonFrozen /* 1001 */) - never frozen, never pruned, also a plain DB read.
• Header(lastFrozenBlock.Hash(), firstNonFrozen /* 1001 */) expecting nil - rawdb.ReadHeader misses on key (1001, hash-of-1000), then viewSingleFile(tx, Headers, 1001) returns ok == false because the only segment covers [0, 1000). Header returns nil before the new check runs, so the nil is not evidence of anything this PR added.

To make it bite, prune past the segment end (PruneBlocks(rwTx, segSize+1, ...)) so the last frozen height is really DB-less, and assert on a height strictly inside the pruned range.

The first subtest in this test (blockAtHeight1.Hash() at height 2) is fine - height 2 is inside [1, 1000), so it is genuinely pruned and does exercise the fix.

[Sahil-4555]

Good catch, you are right. I even had a pre-fix run where that subtest passed and did not read what it meant.

PruneBlocks(rwTx, 1000, 2000) stops at 1000 so block 1000 survives, and the cross-boundary case returns at viewSingleFile !ok before the check ever runs. Removed that subtest.

Pruning now goes to segSize+1 so every frozen height is really db-less, and the wrong-height case uses a canonical hash at its neighbour's height instead. I re-ran everything against the merge-base and each remaining case now fails there for the right reason.

[AskAlexSharov]

This second fixture duplicates the first one for no extra coverage, and it is the most expensive thing in the file.

TestBlockReaderFrozenHashIdentity already builds a canonical branch of Erigon2MinSegmentSize+10 = 1010 blocks, inserts it, dumps [0,1000) and prunes. This test builds another 1010-block chain, another execmoduletester, another full InsertChain, another DumpBlocks - and every assertion it makes (a canonical hash from height 1 requested at height 2, plus the boundary cases) is satisfiable against the first test's canonicalBranch, which spans the same heights in the same freeze/prune state.

Both tests are testing.Short()-skipped, so the full "All tests" job pays the doubled cost.

Also, the generator calls b.AddTx for every i, so all 1010 blocks get a signed tx executed and frozen, while only blocks 1, 2 and the boundary are asserted on. makeBranch in the sibling test already shows the cheap pattern (if i != 0 { return }).

Folding the wrong-height assertions into the first test as extra t.Run subtests over canonicalBranch.Blocks[0]/[1] would drop the second fixture entirely.

[Sahil-4555]

Done - folded into the first test and the second fixture is gone. The generator also only adds txs for i <= 1 now instead of all 1010 blocks.

There is one new fixture left (TestBlockReaderFrozenHashFallsBackToDB), but that one needs a different state - frozen and deliberately not pruned - so it can't share the first test's setup.

[AskAlexSharov]

Worth weighing the cost this adds to every snapshot-backed body read, not just the mismatching ones.

On a canonicalHashCache miss (10k entries against a ~23M-block frozen range, so essentially always on a history-serving node) CanonicalHash does: an MDBX GetOne on kv.HeaderCanonical that is guaranteed to miss once pruned, an LRU Get (golang-lru v2 Get takes Lock(), not RLock(), because it bumps recency), then viewSingleFile(Headers) + OrdinalLookup + decompress + rlp.DecodeBytes into a Header, then Hash(). A body-only read now touches the header segments it would never otherwise page in - on a cold datadir that is the expensive part, not the CPU.

There is also a cache side effect: headerFromSnapshot Adds every header it decodes into headerByNumCache, which is sized 1000. One p2p GetBlockBodies response (MaxBodiesServe = 1024) can now decode and insert >1000 headers whose only purpose is a comparison that is then discarded, flushing the cache that HeaderByNumber / Header / CanonicalHash depend on.

And for several hot callers the check is provably redundant:

• stage_txlookup.go:110 and :308 iterate kv.HeaderCanonical and pass common.CastToHash(v) - literally the value just read from the table CanonicalHash re-reads.
• p2p/protocols/eth/handlers.go:158-162 resolves the number via HeaderNumber(hash) (a hash-keyed lookup), so the hash/height binding is already proven, then calls BodyRlp -> BodyWithTransactions, up to 1024 times per message.
• trace_filtering.go:594 passes lastHeader.Hash() where lastHeader came from HeaderByNumber(blockNum).

If the identity check moves to a (hash, number)-keyed DB read on the mismatch path (see my other comments), these callers pay nothing in the common case.

[Sahil-4555]

Mostly agree. One small correction: CanonicalHash reads kv.HeaderCanonical first and PruneBlocks does not delete those rows, so in the common case it was one point read rather than a header decode - the decode only starts once pruneCanonicalMarkers has swept.

But the point stands and it is gone anyway. The check no longer calls CanonicalHash, and it now runs after the viewSingleFile(Bodies) !ok early-out. It does read the header segment, but through headerByNumCache, which the header-plus-body callers already warm.

[AskAlexSharov]

snapcfg.KnownCfg returns a process-shared pointer, so this mutation leaks into every other test in the binary.

preverifiedRegistry.Get caches and returns the same *Cfg (db/snapcfg/util.go:76-106: if cfg, ok := r.cached[networkName]; ok { return cfg, true } ... r.cached[networkName] = cfg). Setting ExpectBlocks = math.MaxUint64 therefore mutates the mainnet config that any later test - in this package or another - will read back.

dump_test.go:222 already does this, so the pattern is pre-existing, but this PR adds two more mutation sites (here and line 235). A local copy would keep it contained:

known, _ := snapcfg.KnownCfg(networkname.Mainnet)
snCfg := *known
snCfg.ExpectBlocks = math.MaxUint64

[Sahil-4555]

Fixed, taking a local copy now at both sites:

knownCfg, _ := snapcfg.KnownCfg(networkname.Mainnet)
snCfg := *knownCfg
snCfg.ExpectBlocks = math.MaxUint64

Left dump_test.go alone since it is pre-existing and not really part of this change.

[AskAlexSharov]

Header() and blockWithSenders() now disagree about the same (hash, blockHeight) pair.

Header() reads the DB first and returns early on a hit:

if tx != nil {
    h = rawdb.ReadHeader(tx, hash, blockHeight)
    if h != nil {
        return h, nil
    }
}

rawdb.ReadHeader is (number, hash)-keyed, so a side-fork header at a frozen-but-unpruned height still resolves. blockWithSenders() and Body*() never consult the DB at those heights, so for the same pair they return nil.

Callers that fetch header and body together see the inconsistent pair header != nil, body == nil:

• exec_module.go:501-528 -> ExecutionStatusMissingSegment
• fork_validator.go:216-231 -> found chain gap in block body, a critical error
• stage_senders.go:229-243 -> a warn and a skipped block

Whichever way this resolves (DB fallback everywhere, or DB nowhere), the four methods should agree - right now the answer depends on which one you call.

[Sahil-4555]

Agreed, this was the real problem with my first version. Resolved in the "db everywhere" direction: blockWithSenders, Body and BodyWithTransactions now fall back to the hash-keyed db read on mismatch, the same way Header reads it first.

So all four agree for the same (hash, blockHeight) pair now, and the header != nil, body == nil pair those three callers see is gone.

[AskAlexSharov]

Data point for the "should this move into IsCanonical" question: no production caller passes a zero hash to any of these methods. I grepped for common.Hash{} / emptyHash as the hash argument of Header / Body / BodyWithTransactions / BlockWithSenders and found none. The internal caller that could is BlockByNumber (line ~1402), and it explicitly bails out first:

if hash == emptyHash {
    return nil, nil
}

So the zero-hash escape hatch here, and the hash != emptyHash guards in Header and blockWithSenders, are only reachable from tests today.

It is also not a consistent contract across the snapshot boundary. BlockWithSenders(common.Hash{}, N) returns the canonical block when N is frozen, but for N above the snapshot tip it goes to rawdb.ReadBlockWithSenders(tx, common.Hash{}, N), which misses and returns nil. The new zero hash keeps existing height-only behavior subtest pins the frozen half of that, so a caller who relies on it would silently break as heights cross the boundary in the other direction.

Dropping the special case and always enforcing identity would remove both the dead path and the boundary-dependent contract. If it stays, pinning the non-frozen half in the test too would at least make the asymmetry visible.

[Sahil-4555]

This one I don't think holds, unfortunately.

The if hash == emptyHash { return nil, nil } in BlockByNumber is inside the non-frozen branch (block_reader.go:1393-1405):

hash := emptyHash
if number == 0 || maxBlockNumInFiles == 0 || number > maxBlockNumInFiles {
    hash, err = rawdb.ReadCanonicalHash(db, number)
    if hash == emptyHash { return nil, nil }   // only reached here
}
block, _, err := r.BlockWithSenders(ctx, db, hash, number)

For number <= maxBlockNumInFiles that branch is skipped entirely, hash stays emptyHash, and BlockWithSenders(emptyHash, number) is called at :1406. So the zero-hash path is live for every frozen-block read through BlockByNumber - dropping the escape would make all of them return nil.

I have added a BlockByNumber assertion to the zero-hash subtest so this dependency is visible rather than implied.

[AskAlexSharov]

This new nil escapes to peers as an affirmatively empty body rather than as "not available".

BodyRlp (same file, ~line 766) does not check for nil:

body, err := r.BodyWithTransactions(ctx, tx, hash, blockHeight)
if err != nil {
    return nil, err
}
bodyRlp, err = rlp.EncodeToBytes(body)

rlp.EncodeToBytes on a nil *types.Body does not error - it writes nilEncoding (execution/rlp/encode.go:358-372), i.e. 0xc0 for a struct pointer. So bodyRlp is []byte{0xc0}, length 1.

p2p/protocols/eth/handlers.go:163 only skips on len(bodyRLP) == 0:

bodyRLP, _ := blockReader.BodyRlp(context.Background(), db, hash, *number)
if len(bodyRLP) == 0 {
    continue
}
bodies = append(bodies, bodyRLP)

so the 0xc0 is appended and the peer decodes a BlockBody with zero transactions, zero uncles - a positive claim that the block is empty, for a block that is not.

Reachable because AnswerGetBlockBodiesQuery resolves the number via HeaderNumber(hash), which reads kv.HeaderNumber and therefore finds side-fork hashes too. Before this PR that request returned the canonical sibling's body; now it returns an empty one.

BodyRlp missing the nil check is pre-existing, but this PR widens the input set that reaches it, so it is worth a if body == nil { return nil, nil } guard in BodyRlp either way.

[Sahil-4555]

Confirmed and fixed. I checked it directly - rlp.EncodeToBytes((*types.Body)(nil)) gives err=nil len=1 bytes=c0, so handlers.go:163's len(bodyRLP) == 0 never fires and the peer decodes a positively-empty body.

Added if body == nil { return nil, nil } to BodyRlp. RemoteBlockReader.BodyRlp had the same gap so I fixed both. The test now asserts the orphan case returns empty rlp.

[AskAlexSharov]

The new nil ("this hash is not the block at this height") is destroyed one frame up: BodyRlp (line 766) calls rlp.EncodeToBytes(body) with body == nil, and a nil *types.Body encodes as the single byte 0xc0 (verified: err=nil len=1 bytes=c0). So AnswerGetBlockBodiesQuery (p2p/protocols/eth/handlers.go:162) never hits its len(bodyRLP) == 0 guard and ships an empty body to the peer for an orphan hash at a frozen height — the orphan's kv.HeaderNumber row survives PruneBlocks, so the height still resolves. The three HasBlock impls built on BodyRlp != nil (db/consensuschain/consensus_chain_reader.go:101, execution/stagedsync/chain_reader.go:97, execution/exec/chain_reader.go:80) also stay true while GetBlock returns nil. Suggest: make BodyRlp return nil when body == nil.

[Sahil-4555]

Same as the other BodyRlp thread - fixed in both BlockReader.BodyRlp and RemoteBlockReader.BodyRlp, with a test assertion on the orphan case. Your 0xc0 matches what I got when I checked it.

[AskAlexSharov]

The refusal has no DB fallback, while Header (line 648) reads the hash-keyed DB first. For 0 < height <= BlocksAvailable() this function never consults the DB, but there is a steady-state window (~1024 blocks: retire keeps MaxReorgDepth=96 behind tip, CanDeleteTo keeps 1024 in DB) where a reorged-out sibling is at a frozen height and still fully present in MDBX. Result: eth_getBlockByHash(sideForkHash) returns null while the header, body and senders rows are all still readable, and Header for the same (hash, height) still answers — an inconsistent view of the same block. In the fork validator (execution/execmodule/fork_validator.go:225) the nil body even becomes criticalError "found chain gap in block body" when the side-fork header was found in the DB. Suggest: on identity mismatch, fall back to rawdb.ReadBlockWithSenders (and rawdb.ReadBodyWithTransactions in the body methods) before returning nil, mirroring Header.

[Sahil-4555]

Yes, done. blockWithSenders falls back to rawdb.ReadBlockWithSenders on mismatch, and the body methods to rawdb.ReadBody/ReadBodyWithTransactions. HasSenders too.

Covered by the new TestBlockReaderFrozenHashFallsBackToDB, which stops after freezing and skips the prune so the window is real.

[AskAlexSharov]

Two different sources of truth now enforce one invariant: Header/blockWithSenders compare against the header decoded from the immutable segment, while this helper goes through IsCanonical -> CanonicalHash, which consults the mutable kv.HeaderCanonical table first — and PruneBlocks never deletes those rows, so in practice the DB row, not the segment, answers. If the marker and the segment ever disagree at a frozen height (canonical-table corruption, rebuilt/rebased segments), Body(hash, h) can serve the positional body for a hash the segment does not hold, or refuse the hash that Header serves. Comparing against the segment header (already cached via headerByNumCache/canonicalHashCache) keeps all four methods on one source of truth and drops the extra MDBX read.

[Sahil-4555]

Agreed. verifyFrozenIdentity is gone.

The body methods now compare against the header the segment holds (frozenHashAt), which is exactly what Header/blockWithSenders compare against, so kv.HeaderCanonical is out of this path entirely and there is only one source of truth left.

[AskAlexSharov]

Nil-tx regression: the frozen path used to tolerate tx == nil (viewSingleFile type-asserts the nil kv.Getter safely and falls back to r.sn), and both body methods still carry explicit if tx == nil guards in their DB branch. This call now reaches rawdb.ReadCanonicalHash(tx, ...) -> tx.GetOne on a nil interface -> panic for any non-zero hash at a frozen height. No in-tree caller passes nil today, so it is latent — but Header and blockWithSenders still honour the old contract, so the two mechanisms also disagree on nil-tx handling.

[Sahil-4555]

Right, and fixed by dropping CanonicalHash from this path. The check is now frozenHashAt, which goes through viewSingleFile and handles a nil tx the same way the rest of the frozen path does, so Header, blockWithSenders and the body methods agree on nil-tx handling again. The db fallbacks are all guarded with if tx == nil.

[AskAlexSharov]

HasSenders (line 809) is the same bug class left unpatched: for any blockHeight <= BlocksAvailable() it returns true without looking at hash. After this PR the reader contradicts itself: HasSenders(orphanHash, frozenHeight) == true while Body/BlockWithSenders for the same pair return nil. Today's only caller iterates canonical hashes, so nothing breaks yet, but the interface now lies for exactly the inputs this PR is about.

[Sahil-4555]

Correct, fixed. HasSenders now runs the same check and falls back to rawdb.HasSenders on mismatch instead of returning a blanket true. The test asserts it returns false for the orphan hash at a frozen height.

[AskAlexSharov]

Altitude: the invariant is now spelled four times in two dialects (inline h.Hash() != hash in Header/blockWithSenders; verifyFrozenIdentity here), plus the pre-existing third spelling in the forceCanonical branch (line 853) that was not converted, plus two methods that missed it — HasSenders, and TxnByIdxInBlock, which cannot even express it because the hash is dropped at the interface. Nothing on the read path forces the next hash-addressed accessor to ask the question. Consider one shared guard (e.g. the snapshot half of CanonicalHash extracted as frozenHashAt(tx, height)) used by all body/sender paths, with Header/blockWithSenders keeping their free in-place compare.

[Sahil-4555]

Agreed, and I used your suggested shape - frozenHashAt(tx, height) is now the single guard for the body and sender paths, with Header/blockWithSenders keeping their free in-place compare as you suggested. HasSenders is fixed in the same pass.

TxnByIdxInBlock I have left alone - it cannot express the check without an interface change, so that is better as its own PR.

[AskAlexSharov]

Every frozen body read — including the common canonical-hash case (stage loops, RPC range scans, p2p serving) — now pays an extra MDBX point-lookup on kv.HeaderCanonical plus LRU traffic before the body is even opened; on a canonicalHashCache miss it additionally decodes the header segment. The check also runs before viewSingleFile(Bodies, ...), so heights with no bodies file pay it for nothing. Comparing against the cached segment header hash, or at least moving the check after the cheap !ok early-outs, removes most of the cost.

[Sahil-4555]

Done - the check now sits after the viewSingleFile(Bodies) !ok early-out, so heights with no bodies file don't pay for it, and it no longer touches kv.HeaderCanonical. It compares against the segment header through headerByNumCache, which callers fetching header and body together already warm.

[AskAlexSharov]

This subtest is a coverage no-op — all three assertions pass on the merge-base without the fix: lastFrozen is 999 (pruned, snapshot-served, hash matches, so the new guard takes its false branch), firstNonFrozen is 1000 whose header row survives PruneBlocks (prunes [1,1000)) so it is a pure rawdb.ReadHeader hit, and the cross-boundary case returns at viewSingleFile !ok (1000 is outside the [0,1000) segment) before the new check runs. The comment "must still be rejected on both sides" is misleading — nothing here is rejected by the fix; the request is simply out of segment range.

[Sahil-4555]

Agreed, that subtest was proving nothing - removed.

The replacement prunes to segSize+1 so the frozen heights are actually db-less, and the wrong-height case now uses a canonical hash at its neighbour's height. I confirmed each remaining case fails on the merge-base before pushing.

[AskAlexSharov]

This second test rebuilds the whole fixture (~1010 generated and executed blocks, DumpBlocks, PruneBlocks) to pin the same new branch the first test already drives — the wrong-height case is rejected by the identical h.Hash() != hash / IsCanonical comparisons, so no fix that passes the first test can fail this one. Folding the wrong-height assertions into the first test as another subtest (its canonical chain is already 1010 frozen blocks) halves this file's non-short CI cost and removes ~45 duplicated lines. The freeze+prune scaffolding also already exists in dump_test.go and exec_module_devp2p_test.go — worth a shared helper.

[Sahil-4555]

Folded in and the second fixture is dropped.

On the shared helper - agreed it would help, but I would rather do that as its own cleanup across dump_test.go and exec_module_devp2p_test.go than grow this PR further.

[AskAlexSharov]

.claude/rules/comments.md: "The same rationale repeated at multiple sites — state the why once at the canonical place; use terse pointers elsewhere." This 3-line comment appears verbatim here and in blockWithSenders, with a third restatement in the verifyFrozenIdentity docstring and a fourth in the test's doc comment. Keep the why once (the helper's docstring is the natural place) and let the call sites be bare code.

[Sahil-4555]

Fixed - the why lives once in the frozenHashAt docstring now, and the call sites just point at it.

[AskAlexSharov]

Reviewer-facing narration ("adversarially checks a case the primary regression test does not cover", "A fix that only compared ... would wrongly accept this"; same pattern in the "Baseline:"/"Control:" comments at lines 95 and 126) is PR-description material per .claude/rules/comments.md — test docstrings get latitude for non-obvious scenarios, but not for scope narration about other tests and hypothetical fixes.

[Sahil-4555]

Removed. The "Baseline"/"Control" narration and the cross-test commentary are gone; the docstrings now just say what state the test puts the reader in.

[AskAlexSharov]

After the guard above, hash == h.Hash() in every case except hash == emptyHash, so this assignment now only fills in the zero-hash case — yet it still reads like caller-hash normalization, which is exactly the substitution this PR removes. Dropping it and passing h.Hash() at the single use site (types.NewBlockFromStorage) makes the guard impossible to "preserve" incorrectly in a future refactor.

[Sahil-4555]

Agreed, dropped it. types.NewBlockFromStorage takes h.Hash() directly now, so there is no caller-hash variable left for a future refactor to "preserve" wrongly.

[AskAlexSharov]

New files added since 2026 use // Copyright 2026 The Erigon Authors (recent examples across db/, execution/, rpc/); 2024 looks copied from an older file.

Suggested change

	// Copyright 2024 The Erigon Authors
	// Copyright 2026 The Erigon Authors

[Sahil-4555]

Done, 2026.