fix(archiver): fully clean up removed blocks whose txs were re-included elsewhere - #24732
Closed
benesjan wants to merge 1 commit into
Closed
fix(archiver): fully clean up removed blocks whose txs were re-included elsewhere#24732benesjan wants to merge 1 commit into
benesjan wants to merge 1 commit into
Conversation
…ed elsewhere BlockStore.removeBlocksAfter had two defects that corrupt the tx-effect index (txHash -> block position) when the same tx exists in two stored blocks, e.g. re-included after its original proposal expired: - deleteBlock removed #txEffects entries blindly by txHash, destroying the entry of a tx whose index already points at another stored block, which makes that block unreadable. - removeBlocksAfter skipped cleanup entirely for blocks it could not reconstruct, leaking their row, tx effects, and indices; a later insert at the same number then overwrites the row in place, leaving stale tx-effect entries pointing into the new chain. A stale entry makes getL2ToL1MembershipWitness resolve the tx at wrong coordinates and throw 'The L2ToL1Message you are trying to prove inclusion of does not exist' for a message in a proven block, and lets getTxReceipt report a proven position the chain no longer has. Cleanup now works from the raw storage row (so unreadable blocks are still fully released) and only deletes tx-effect entries still owned by the block being removed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
Closed in favor of #24765 |
PhilWindle
added a commit
that referenced
this pull request
Jul 27, 2026
…eck tx-effect deletes (#24765) ## Problem Two defects in `BlockStore` block removal could corrupt the tx-effect index (txHash → owning block hash/position) if the store ever held two blocks sharing a tx: 1. **Blind delete** — `deleteBlock` removed `#txEffects` entries by txHash without checking the entry still pointed at the block being deleted. If another stored block contained the same tx, that block's entry was destroyed collaterally, making the block unreadable (`Could not find tx effect for tx…`). 2. **Skip-on-unreadable leak** — `removeBlocksAfter` did `getBlock(bn) === undefined → warn + continue`, so an unreadable block's row, tx effects, and indices were never released. A later `addCheckpoints` overwrites the leaked row in place ("L1 data is authoritative"), leaving stale tx-effect entries pointing at coordinates now occupied by different txs — `getTxReceipt` then reports positions the chain no longer has, and `getL2ToL1MembershipWitness` resolves the wrong tx and throws `The L2ToL1Message you are trying to prove inclusion of does not exist`. This is hardening, not an incident fix. No honest code path produces two stored blocks sharing a tx: duplicate nullifiers are rejected at block building (double-spend validation against the build fork), at validator re-execution, and at world-state sync (nullifier leaves are non-updateable). The incident that prompted the investigation turned out to be a downstream client bug following a v5 behavior change in `L2BlockSynchronizer`, unrelated to the store. The store should still not amplify a duplicated-tx state into silent corruption if it ever appears — e.g. via a future upstream bug, or byzantine tx effects that repeat a txHash with distinct nullifiers (invisible to every nullifier-based check). ## Fix - `removeBlocksAfter` cleans up from the raw storage row, so blocks whose bodies can no longer be fully loaded still release their row, tx effects (enumerated from `#blockTxs`), and hash/archive indices. - `deleteBlock` only deletes a tx-effect entry still owned by the block being removed (compares the entry's leading block hash). - Both anomalies now log a warning (an entry owned by another block, or a missing entry). These states should be unreachable, so a warning in production logs is direct evidence of an upstream bug worth investigating — previously the ownership skip was silent, hiding exactly that signal. ## Cost The added reads are confined to prune/reorg paths: one `#blockTxs` read plus one tx-effect point read per tx of each *removed* block (roughly doubling point reads on block removal). Block ingestion and query paths are untouched. ## Verification - Regression test from #24732: two proposed blocks sharing a tx effect; asserts removal releases both blocks fully with no residue (fails on the pre-fix implementation). - Full `block_store.test.ts` suite passes (160 tests). Recreates #24732 with the original commit cherry-picked and authorship preserved (thanks @benesjan). On top of it, this PR updates the description and code comments to match what the follow-up investigation established about how a duplicated-tx state can and cannot arise, and adds the anomaly warnings. Supersedes #24732. Related to A-1426
AztecBot
pushed a commit
that referenced
this pull request
Jul 27, 2026
…eck tx-effect deletes (#24765) ## Problem Two defects in `BlockStore` block removal could corrupt the tx-effect index (txHash → owning block hash/position) if the store ever held two blocks sharing a tx: 1. **Blind delete** — `deleteBlock` removed `#txEffects` entries by txHash without checking the entry still pointed at the block being deleted. If another stored block contained the same tx, that block's entry was destroyed collaterally, making the block unreadable (`Could not find tx effect for tx…`). 2. **Skip-on-unreadable leak** — `removeBlocksAfter` did `getBlock(bn) === undefined → warn + continue`, so an unreadable block's row, tx effects, and indices were never released. A later `addCheckpoints` overwrites the leaked row in place ("L1 data is authoritative"), leaving stale tx-effect entries pointing at coordinates now occupied by different txs — `getTxReceipt` then reports positions the chain no longer has, and `getL2ToL1MembershipWitness` resolves the wrong tx and throws `The L2ToL1Message you are trying to prove inclusion of does not exist`. This is hardening, not an incident fix. No honest code path produces two stored blocks sharing a tx: duplicate nullifiers are rejected at block building (double-spend validation against the build fork), at validator re-execution, and at world-state sync (nullifier leaves are non-updateable). The incident that prompted the investigation turned out to be a downstream client bug following a v5 behavior change in `L2BlockSynchronizer`, unrelated to the store. The store should still not amplify a duplicated-tx state into silent corruption if it ever appears — e.g. via a future upstream bug, or byzantine tx effects that repeat a txHash with distinct nullifiers (invisible to every nullifier-based check). ## Fix - `removeBlocksAfter` cleans up from the raw storage row, so blocks whose bodies can no longer be fully loaded still release their row, tx effects (enumerated from `#blockTxs`), and hash/archive indices. - `deleteBlock` only deletes a tx-effect entry still owned by the block being removed (compares the entry's leading block hash). - Both anomalies now log a warning (an entry owned by another block, or a missing entry). These states should be unreachable, so a warning in production logs is direct evidence of an upstream bug worth investigating — previously the ownership skip was silent, hiding exactly that signal. ## Cost The added reads are confined to prune/reorg paths: one `#blockTxs` read plus one tx-effect point read per tx of each *removed* block (roughly doubling point reads on block removal). Block ingestion and query paths are untouched. ## Verification - Regression test from #24732: two proposed blocks sharing a tx effect; asserts removal releases both blocks fully with no residue (fails on the pre-fix implementation). - Full `block_store.test.ts` suite passes (160 tests). Recreates #24732 with the original commit cherry-picked and authorship preserved (thanks @benesjan). On top of it, this PR updates the description and code comments to match what the follow-up investigation established about how a duplicated-tx state can and cannot arise, and adds the anomaly warnings. Supersedes #24732. Related to A-1426
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
BlockStore's tx-effect index (txHash → owning block hash/position) can be corrupted when the same tx exists in two stored blocks at once — routine when a checkpoint proposal expires and the sequencer re-includes its txs:deleteBlockremoved#txEffectsentries by txHash without checking the entry still points at the block being deleted. Inserting the re-included block repoints the shared tx's entry at the new block; deleting the old block then destroys the new block's entry, making that block unreadable (Could not find tx effect for tx…).removeBlocksAfterdidgetBlock(bn) === undefined → warn + continue, so an unreadable block's row, tx effects, and indices were never released.addCheckpointslater overwrites the leaked row in place ("L1 data is authoritative"), leaving stale tx-effect entries pointing at coordinates now occupied by different txs.Downstream symptoms of a corrupted index:
getTxReceiptreporting positions the chain no longer has, unreadable blocks being silently dropped fromgetBlocks/getCheckpointsresponses (consumers see a checkpoint with missing blocks and no error), andgetL2ToL1MembershipWitnessresolving the wrong tx and throwingThe L2ToL1Message you are trying to prove inclusion of does not existfor a message that is on-chain and proven.Fix
removeBlocksAfternow cleans up from the raw storage row, so blocks whose bodies can no longer be fully loaded still release their row, tx effects (enumerated from#blockTxs), and hash/archive indices.deleteBlockonly deletes a tx-effect entry that is still owned by the block being removed (compares the entry's leading block hash), so a re-included tx's index survives the removal of its old block.The regression test adds two proposed blocks sharing a tx effect and asserts
removeBlocksAfterremoves both and leaves no residue. Before this change it fails: block 1 is removed, block 2 becomes unreadable and leaks.Verification
block_store.test.ts(fails on the previous implementation, passes now).Related observation while debugging downstream (can file separately if useful): during reorg ingestion there is a read window where
getCheckpoints(n, …, { includeBlocks: true })returns a checkpoint whose blocks are missing or empty with no warning — unreadable or not-yet-visible blocks are silently filtered out. Consumers that treat the returned payload as complete lose data; a loud error or a retryable signal would be safer.🤖 Generated with Claude Code