fix(archiver): clean up removed blocks from raw rows and ownership-check tx-effect deletes#24765
Open
spalladino wants to merge 3 commits into
Open
fix(archiver): clean up removed blocks from raw rows and ownership-check tx-effect deletes#24765spalladino wants to merge 3 commits into
spalladino wants to merge 3 commits 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>
…oval The ownership-checked delete silently skipped entries owned by another block. That state (two stored blocks sharing a tx) should be unreachable for honest chains, so skipping it silently hides direct evidence of an upstream bug. Warn on both anomalies (foreign-owned entry and missing entry) and correct the comment that attributed the duplication to routine proposal expiry, which cannot produce it.
spalladino
force-pushed
the
spl/archiver-tx-effect-cleanup
branch
from
July 17, 2026 17:34
5a61d75 to
1f57f94
Compare
spalladino
requested review from
a team,
IlyasRidhuan,
LeilaWang,
MirandaWood,
Thunkar,
charlielye,
just-mitch,
nventuro and
sirasistant
as code owners
July 17, 2026 17:34
spalladino
changed the base branch from
merge-train/spartan
to
merge-train/spartan-v5
July 17, 2026 17:35
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
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
Two defects in
BlockStoreblock removal could corrupt the tx-effect index (txHash → owning block hash/position) if the store ever held two blocks sharing a tx:deleteBlockremoved#txEffectsentries 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…).removeBlocksAfterdidgetBlock(bn) === undefined → warn + continue, so an unreadable block's row, tx effects, and indices were never released. A lateraddCheckpointsoverwrites the leaked row in place ("L1 data is authoritative"), leaving stale tx-effect entries pointing at coordinates now occupied by different txs —getTxReceiptthen reports positions the chain no longer has, andgetL2ToL1MembershipWitnessresolves the wrong tx and throwsThe 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
removeBlocksAftercleans 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 still owned by the block being removed (compares the entry's leading block hash).Cost
The added reads are confined to prune/reorg paths: one
#blockTxsread 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
block_store.test.tssuite 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