fix(archiver): skip descendants of invalid-attestations checkpoints#23502
Draft
spalladino wants to merge 2 commits into
Draft
fix(archiver): skip descendants of invalid-attestations checkpoints#23502spalladino wants to merge 2 commits into
spalladino wants to merge 2 commits into
Conversation
Adds an e2e test that deterministically reproduces the archiver retry loop documented at l1_synchronizer.ts:905-908: when a checkpoint with insufficient attestations lands and the next proposer publishes a valid descendant without first invalidating it, block_store.addCheckpoints throws InitialCheckpointNumberNotSequentialError on every poll. Gated by a new test-only sequencer flag skipWaitForValidParentCheckpointOnL1 that bypasses the parent-validity check inside the checkpoint proposal job.
When a proposer published a valid-attestations checkpoint that extended an earlier checkpoint with insufficient attestations, the archiver would skip the bad parent then throw InitialCheckpointNumberNotSequentialError when storing the descendant, the catch handler would roll back the L1 sync point, and the next poll would loop on the same range indefinitely. This change persists rejected ancestors in the block store, detects descendants by matching header.lastArchiveRoot against the stored set, emits a new CheckpointBuiltOnInvalidAncestorDetected event, and prunes the set by L1 finality so reorg-vulnerable rows never linger. The slasher's AttestationsBlockWatcher is updated to slash the proposer (rather than the attestors) under offense PROPOSED_DESCENDANT_OF_CHECKPOINT_WITH_INVALID_ATTESTATIONS for both the existing invalid-descendant path and the new valid-descendant path. The e2e test introduced in this branch is flipped from asserting the stuck-state bug to asserting the post-fix behavior: the chain advances past the bad descendant, the new event fires, and the proposer is slashed.
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.
Motivation
archiver/src/modules/l1_synchronizer.tsskipped checkpoints with insufficient/invalid attestations under the assumption that the next proposer would invalidate them before publishing. When that assumption was violated — i.e., proposer P2 published a valid-attestations checkpoint that extended P1's invalid one — the archiver hitInitialCheckpointNumberNotSequentialErrorinblock_store.addCheckpoints, the catch handler rolled back the L1 sync point, and the next poll re-fetched the same range and re-threw. The archiver looped indefinitely. The protocol already definesOffenseType.PROPOSED_DESCENDANT_OF_CHECKPOINT_WITH_INVALID_ATTESTATIONSfor exactly this case but the slasher couldn't see valid-attestations descendants because the archiver threw before emitting any event.Human Note
This is particularly relevant under pipelining. Attestors now attest to a checkpoint before the previous one is pushed to L1, so they can be inadvertently attesting to a checkpoint built on top of one that became invalid as it was published to the rollup the contract with wrong attestations. So an honest attestor could get slashed if the proposer was malicious.
Approach
In the synchronizer, persist rejected ancestors in the block store keyed by archive root. On each new checkpoint, before attestation validation, compare its
header.lastArchiveRootagainst the persisted set — if it matches, skip the checkpoint as a descendant of an invalid ancestor and emit a newL2BlockSourceEvents.CheckpointBuiltOnInvalidAncestorDetectedevent with enough metadata to resolve the proposer. Prune the set by L1 finality so reorg-vulnerable rows are dropped only once they can no longer be displaced. The slasher'sAttestationsBlockWatcheris fixed to slash the proposer (not the attestors) under the existing offense type and gains a new handler for the new event.Changes
L2BlockSourceEvents.CheckpointBuiltOnInvalidAncestorDetectedenum value andCheckpointBuiltOnInvalidAncestorEventpayload (committee/seed/epoch/checkpoint/ancestor info).RejectedCheckpointtype and LMDB-backed store API inblock_store.ts(getRejectedCheckpoints,addRejectedCheckpoint,removeRejectedCheckpointsBelowL1Block).l1_synchronizer.tslearns Branch A (descendant skip — emit event, persist, continue) ahead of attestation validation, and prunes rejected rows below the finalized L1 block at the end of each sync iteration. AddedgetEpochInfoForCheckpointhelper invalidation.tsso the descendant path can populate the event payload without paying for signature verification twice.slashAttestorsOnAncestorInvalidrenamed toslashProposerOnAncestorInvalidand switched from attestor list to a single proposer (resolved the same wayslashProposerdoes), fixing the inconsistency between the offense's proposer-oriented naming and the previous attestor-slashing behavior. NewhandleDescendantOfInvalidlistens to the new event and slashes the proposer of the valid descendant. Class JSDoc updated to describe both paths.block_store.test.tscases for the rejected-checkpoints store.archiver-sync.test.tsgains a regression for the valid-descendant skip and updates the existing invalid-attestations test now that descendants also persist.PROPOSED_INSUFFICIENT_ATTESTATIONSfor P1's proposer andPROPOSED_DESCENDANT_OF_CHECKPOINT_WITH_INVALID_ATTESTATIONSfor P2's proposer) are recorded.skipWaitForValidParentCheckpointOnL1flag (from the original test commit) bypasses the parent-validity check insideCheckpointProposalJob.waitForValidParentCheckpointOnL1so the e2e can engineer the fault.