Repository navigation
fix(prover): apply the anchor window to forced-inclusion sources - #55
Open
davidtaikocha wants to merge 3 commits into
Open
davidtaikocha wants to merge 3 commits into
davidtaikocha wants to merge 3 commits into
Conversation
validateManifestAnchorNumbers short-circuited for forced-inclusion sources: it checked only that each anchor equalled the parent anchor and returned before validateManifestAnchorProgression, the only place the [origin-MAX_ANCHOR_OFFSET, origin] window is enforced. Canonical derivation (taiko-client-rs validate_anchor_numbers) applies the monotonicity and window rules to every block of every source, and exempts forced inclusions only from the strict-advance rule. Because inherited metadata pins a forced source's anchor to the parent's, a parent anchor that had stalled more than MAX_ANCHOR_OFFSET behind the proposal origin left the window: canonical derivation replaces such a source with the transaction-free default manifest, while gaiko2 kept the source and derived a block still carrying its transactions. Same proposal, two different block hashes and state roots. Run the progression check for every source and skip only the advance rule for forced inclusions. The per-source equality check is dropped as redundant: applyInheritedManifestMetadata already pins forced anchors to the parent value before validation, and validateSourceAwareManifestAnchors re-checks the same equality across sources. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous test documented prepareSourceManifest's fallback but only called validateSourceManifest and asserted false, so it never executed the replacement at manifest_validate.go:331-334 and never checked the outcome that actually matters: that the source's transactions are gone. Drive prepareSourceManifest with a blob-backed forced-inclusion source carrying a transaction and a stalled parent anchor, and assert the default manifest it returns has exactly one block, zero transactions, and the inherited timestamp, coinbase, anchor and gas limit. Against the pre-fix source this now fails with "got 1 transactions" rather than passing vacuously. The predicate-level test is kept alongside it, driven from the same fixture, so the composition it isolates (timestamps and gas limit in range, anchor window the only rejecting rule) stays covered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
decodeBlobBackedSourceManifest degrades to the default manifest — one block,
zero transactions — on any codec failure, and that default satisfies every
assertion both stalled-anchor tests make. A blob-codec or fixture regression
would therefore leave them green while covering nothing; corrupting the fixture
blob confirmed both still passed.
The fixture now decodes its own blob and asserts exactly one block carrying one
transaction, so callers assert against what the blob really contains. The
predicate test additionally pins which rule rejects: timestamps and gas limit
must pass and the anchor window must be what fails, so a fixture drift that
pushed either out of range cannot make the test pass for the wrong reason.
Verified three ways: green with the fix, red against the pre-fix production code
("got 1 transactions"), and red under a forced decode fallback ("fixture must
decode to exactly one transaction, got 0").
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
|
Let’s hold off on merging this for now, since this is a permissionless feature that will only be enabled in about 3–6 months. |
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
validateManifestAnchorNumbersshort-circuits for forced-inclusion sources:It never reaches
validateManifestAnchorProgression, the only place the[origin-MAX_ANCHOR_OFFSET, origin]window is enforced (512 blocks on mainnet, 128 elsewhere).Canonical derivation applies the monotonicity and window rules to every block of every source
and exempts forced inclusions only from the strict-advance rule
(
taiko-client-rscrates/driver/src/derivation/pipeline/shasta/validation.rs,validate_anchor_numbers: the range checks sit in the per-block loop with nois_forced_inclusionguard; only the trailing
!is_forced_inclusion && highest_anchor <= parent_anchorrule is gated).applyInheritedManifestMetadatapins a forced source's anchor to the parent's and leavesTransactionsuntouched. So whenever the parent anchor has stalled more thanMAX_ANCHOR_OFFSETbehind the proposal origin, the inherited anchor is outside the window and the two implementations
disagree:
Same proposal, two different block hashes and state roots — so gaiko2 would attest to a checkpoint
over a block canonical node software does not derive.
git log -L 509,545:internal/prover/manifest_validate.goshows the window check used to apply here and was replaced by this early return in
3d4b50c.Fix
Run
validateManifestAnchorProgressionfor every source, and skip only the advance rule for forcedinclusions — a direct mirror of the canonical function.
The per-source
anchor == parentAnchorcheck is dropped as redundant, not relaxed:applyInheritedManifestMetadataalready pins forced anchors to the parent value before validation,and
validateSourceAwareManifestAnchorsre-checks the same equality across sources. Keeping itwould have made gaiko2 stricter than canonical, which is its own divergence risk.
Note this is consistent with
Derivation.md, which exempts forced inclusions from being defaulted"even when the anchor number does not advance" — the advance rule is exactly what this change still
skips for them.
Tests
Five new tests, each watched failing before the fix:
validateSourceManifestcomposition: an otherwise well-formed forced source with transactions anda stalled anchor is invalidated, so
prepareSourceManifestreplaces it with the transaction-freedefault manifest
go test ./...,go vet ./...andgofmt -lare clean.Sequencing
Please land this before any fix to the parent
CheckpointStorestorage-slot derivation inshastaCheckpointStorageSlots. That slot derivation currently fails closed for every proposalcontaining a forced-inclusion source, which is the only reason the divergence above is not reachable
today; correcting the slot first would make it reachable. With this merged, that ordering constraint
goes away.
🤖 Generated with Claude Code