fix: resolve the confirmed Greptile findings across the Fast Inbox stack - #271
spalladino wants to merge 11 commits into
Conversation
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const attestationDeadline = this.proposalHandler.getAttestationDeadline(proposal.slotNumber); | ||
| if (+attestationDeadline <= this.dateProvider.now()) { | ||
| this.log.warn(`Attestation deadline for slot ${proposal.slotNumber} passed during validation, not signing`, { | ||
| slot: proposal.slotNumber, | ||
| archive: proposal.archive.toString(), | ||
| attestationDeadline: attestationDeadline.toISOString(), | ||
| }); | ||
| return undefined; | ||
| } | ||
|
|
||
| const attestations = await this.validationService.attestToCheckpointProposal(proposal, attestors, checkpointNumber); |
There was a problem hiding this comment.
The deadline is checked before an asynchronous signing operation that can spend up to 30 seconds calling Web3Signer and can also perform HA coordination. If validation finishes shortly before the cutoff, the deadline can pass while signatures are being produced. There is no second check before addOwnCheckpointAttestations, so those late attestations are still stored and broadcast. Enforce the deadline through signing or check it again before publishing the result.
Knowledge Base Used:
| /** Records that a background attempt or probe reached its expected outcome, clearing the background streak. */ | ||
| private registerBackgroundSuccess(): void { | ||
| this.consecutiveBackgroundFailures = 0; | ||
| this.updateConsumptionHealth(); |
There was a problem hiding this comment.
registerBackgroundSuccess resets one global failure streak even though up to eight consumption attempts and replay probes can settle concurrently. One successful job can therefore erase several RPC failures from other jobs. With one consistently successful lane among failing lanes, the bot never reaches maxConsecutiveErrors and remains healthy despite persistent consumption failures. Track failures per sequential poll or batch, or aggregate concurrent outcomes before resetting the streak.
| { attempts = 3, timeoutSeconds = 240 }: { attempts?: number; timeoutSeconds?: number } = {}, | ||
| ): Promise<InsertingBlock> { | ||
| let lastError: unknown; | ||
| for (let attempt = 0; attempt < attempts; attempt++) { | ||
| try { | ||
| const { leafIndex } = await retryUntil( | ||
| async () => { | ||
| const index = await node.getL1ToL2MessageIndex(msgHash); | ||
| return index === undefined ? undefined : { leafIndex: index }; | ||
| }, | ||
| `node assigns a compact index to message ${msgHash.toString()}`, | ||
| timeoutSeconds, | ||
| 0.5, | ||
| ); | ||
|
|
||
| // The chain holds the message once its tip's tree has grown past the message's index. | ||
| const { tip } = await retryUntil( | ||
| async () => { | ||
| const tip = await node.getBlockNumber(); | ||
| const count = await committedMessageCount(node, tip); | ||
| return count !== undefined && count > leafIndex ? { tip } : undefined; | ||
| }, | ||
| `a block committing message ${msgHash.toString()}`, | ||
| timeoutSeconds, |
There was a problem hiding this comment.
Each outer attempt starts two independent retryUntil calls with the full timeoutSeconds allowance. With the defaults, one search can wait up to 3 × 2 × 240s before accounting for the historical reads, which exceeds the calling suite's 900-second timeout. This can make the test time out before the helper reports its own failure. Use one absolute deadline for the complete search or divide the budget across attempts.
| return Promise.resolve(blockNumber >= insertingBlockNumber ? [leafIndex] : undefined); | ||
| }, | ||
| }; | ||
| return view as unknown as InsertingBlockNodeView & { historicalReads: number }; |
There was a problem hiding this comment.
This new fixture uses as unknown as here and repeated as Error assertions in the rejection test at lines 77–79. The repository requires avoiding as Type casts in favor of type guards. Define the fixture with a structurally compatible typed object and narrow caught values with instanceof Error. This explicit repository requirement must be satisfied before merging.
Context Used: yarn-project/CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| const job = this.runConsumptionAttempt(message) | ||
| .catch(err => this.registerConsumptionFailure(err)) | ||
| .catch(err => this.registerBackgroundFailure(err)) | ||
| .finally(() => this.attemptsInFlight.delete(message.messageId)); |
There was a problem hiding this comment.
The changed consumption handler uses a .catch() callback here, and the replay-probe path repeats the pattern at lines 1791–1795. The repository requires preferring async/await over .then() and .catch() callbacks. Wrap these jobs in an async helper with try/catch while retaining cleanup. This explicit repository requirement must be satisfied before merging.
Context Used: yarn-project/CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…ge-limited log queries An L1 block hash the provider returns in a form Buffer32 cannot parse escaped the archiver's guarded read as an exception, from a check whose whole contract is canonical/replaced/unknown. Parsing now happens inside the guard, so unparseable provider data leaves synchronization pending instead of throwing or rolling back. fetchLogsBisectingRange also halved the range on every failure. A persistent authentication, rate-limit, outage or missing-state failure over a 100-block window therefore turned one failed request into 199 identical ones before reporting the same error. Only failures a narrower range can fix — the provider's own range and response-size refusals, classified by isLogRangeLimitError — now split, with a request budget as a safety net.
The exemption from MIN_BLOCKS_FOR_INBOX_CATCHUP was a threshold on the Ethereum slot duration, which is not a statement about the deployment: a real network running short L1 slots was exempted too, and short L1 slots do not raise the per-block message cap, so such a proposer can still be unable to reach a mandatory endpoint and lose all of its slots against an aged backlog. The exemption is now an explicit SequencerConfig flag, allowUnsafeInboxCatchupCapacity, defaulting to false and set only by the sandbox and e2e fixtures that deliberately derive one or two blocks per slot against an Inbox nobody is filling.
…ne honest
Four separate defects in checkpoint and block proposal handling:
- Timeout diagnostics reported only the first attempt's failure, so a store or provider fault that appeared later
was discarded exactly when it was the reason this node gave up. The latest unexpected failure is kept instead.
- The expected-failure filter matched message substrings, so an unrelated fault quoting one of them ("database
unavailable: Inbox message range ... is not fully synced") was reported as ordinary sync lag. Matching is now
anchored to the complete canonical forms.
- Pruning the block a cached valid verdict rested on makes revalidation fail before the blocks are loaded, so its
failure carries no checkpoint number and could overwrite that verdict with 'unvalidated', contaminating
inactivity and slashing accounting. The tracker now answers by slot and archive as well.
- The Inbox endpoint gate granted at least a second even after the attestation deadline had passed, and nothing
re-read the deadline before signing. A late validation is still finished for telemetry; it is no longer signed.
The decoders read optional-field presence as "any non-zero word" and stopped wherever the fields ran out, so one proposal had unboundedly many encodings: trailing bytes were ignored, and a payload from an older format whose bytes happened to line up parsed as a different proposal shape. Signature verification bounds what that buys an attacker but does not make the byte string an identity. Presence flags now accept only 0 and 1, and a top-level Buffer decode must consume its whole input. Nested decodes still read their part of a larger buffer, which is why only the Buffer entry point is strict.
…g proof work Verifier circuits were started before the checkpoint's overall message span was checked. Those jobs go into a shared cache that outlives this checkpoint's sub-tree, so one started before the check survives the cancellation that follows and keeps proving for a checkpoint nothing will accept. The whole span — the total supplied count, every per-block leaf count and the monotonic boundaries between them — is now validated before the sub-tree is created, and the per-block slices it computes are what the block loop consumes.
…ound failures - l1ToL2SeedCount is optional in BotConfig so an unset value is distinguishable from an explicit one. Inbox mode's 512 is applied from the absence of a value, not from it matching the general default, so BOT_L1_TO_L2_SEED_COUNT=1 stays 1 and is rejected by validation instead of being silently rewritten. - The pending-transaction check read the pool once and then launched every candidate. A job does not reach the pool until its transaction is submitted, several awaits later, so a poll could overshoot a cap the operator set on purpose. Availability is computed once, counts every in-flight attempt, and is spent as jobs are dispatched. - A replay probe that timed out still marked its batch probed, letting a run whose messages all succeeded close as a success with replay protection never demonstrated. It now records a failed check first. - The consumption poll reset the consecutive-failure counter before the jobs it dispatched had settled, so one failing background job per poll could repeat forever without reaching the unhealthy threshold. Poll failures and background failures are counted apart, and only a background operation that succeeds clears the background streak.
…s confirmation findInsertingBlock retried a false confirmation but let a historical world-state or witness read throw straight out of the helper, which is the same kind of timing miss: a prune drops the block a bisection step is asking about. The whole attempt now sits inside the retry boundary and the last failure is carried as the cause. Extracted to fixtures/find_inserting_block.ts so the retry boundary has a unit test rather than an e2e injection.
Publisher keys belong to prover nodes: an agent proves and hands its result back, while the node signs and submits to L1, and only the prover stack's node sub-chart reads PUBLISHERS_PER_PROVER. Sizing the range off autoscaled KEDA agent capacity created keys nothing ever uses and moved the start of every index range allocated after it — the fast-inbox network allocated 8000..8007 where it needs 8000,8001. Both the deployment script and the funding calculation now source one helper so they cannot diverge, checked by prover_publisher_count.test.sh.
The gate's retry window is bounded by the attestation deadline and is now zero once it has passed, so this case has to run with budget left: retrying L1 past the deadline only buys a verdict that cannot be signed.
- The attestation deadline check moved from the peer path to `createCheckpointAttestationsFromProposal`, the one place both the peer path and the proposer's own `collectOwnAttestations` sign through, next to the equivocation check that is there for the same reason. The peer-path check was bypassable. - The Inbox endpoint gate keeps its bounded floor past the deadline rather than returning a zero-length window: a late node still wants the content verdict for telemetry (which is what the finding asks for), and `AbortSignal.timeout(0)` aborts on the next tick rather than synchronously, so a zero window would not have been a reliable refusal anyway. The signature is what the deadline stops, and that is now enforced in one place. - `allowUnsafeInboxCatchupCapacity` is no longer settable from the environment, matching the other test-only sequencer flags: a deployment must not be able to opt out of the catch-up floor by accident. - The log range-limit classifier no longer matches every message containing "block range", so a pruned or unavailable range is not split; the provider's own last refusal is kept as the cause when the budget runs out. - Terraform sizes the prover keystore from publisher-bearing prover nodes too, so key generation and prefunding cannot disagree. - Two bot tests now prove their descriptions: a genuinely empty poll, and the batch state a timed-out replay probe leaves behind.
The Inbox-backlog floor no longer exempts short L1 slot durations, so the 8s ethereumSlotDuration this bench adopted to stay inside that exemption no longer buys anything. The e2e fixture sets allowUnsafeInboxCatchupCapacity, which covers the bench, so restore the 12s production slot duration.
cd54e4d to
810ae99
Compare
1 participant
Implements the 20 confirmed Greptile findings from
tmp/greptile-stack-bug-fix-plan-20260918.md.The plan was written against the Fast Inbox PR stack and asks for a fix in each owning PR. That stack is fully
merged (
mainis at #243), so there are no owning branches left to edit and this lands as one PR onmain.Four findings needed no change —
mainalready has the fixMockL1ToL2MessageSource.setL1ToL2Messagesreplaces the whole indexed log, andsetInboxBucketno longer exists.readCheckpointConsumedMessagesalreadywraps the range read and returns the non-punitive
inbox_prefix_unavailable.ARCHIVER_DB_VERSIONis 11, bumped by fix(archiver): key contract instance updates by the block that carried them #230. Every persisted layoutthat ever shipped already has a distinct version, so the finding's failure mode — reopening an older layout under
the same number — is not reachable. Bumping to 12 was deliberately not done: it would invalidate every existing
v11 database and force a resync for no safety gain. Flagging it here because the plan's checklist asks for 12;
if you want the bump anyway, say so and I'll add it.
CheckpointProposalJobTestGate.withHoldalready races the failure channel against the match and the whole body, and both e2e files use
withHold.The sixteen fixes
Each has a regression test that fails without the fix.
Archiver / Ethereum
whose contract is canonical/replaced/unknown. Parsing moved inside the guard.
fetchLogsBisectingRangehalved the range on every failure, turning one persistent auth/rate-limit/outagefailure over a 100-block window into 199 identical requests. A new
isLogRangeLimitErrorclassifier restrictssplitting to the provider refusals a narrower range can fix, with a request budget as a safety net.
Sequencer
MIN_BLOCKS_FOR_INBOX_CATCHUPexemption was a threshold onethereumSlotDuration, which is not a statementabout the deployment: a production network running short L1 slots was exempted too, and short L1 slots do not
raise the per-block message cap. Replaced by an explicit
allowUnsafeInboxCatchupCapacityflagflag, default false, set by the e2e and local-network fixtures (not settable from the environment).
Validator
that actually explains why the node gave up.
database unavailable: Inbox message range ... is not fully syncedwas reported as ordinary sync lag. Now anchored to the complete canonical message forms.validverdict rested on makes revalidation fail before the blocks load, so itsfailure carries no checkpoint number and could overwrite that verdict with
unvalidated. The re-executiontracker now answers by slot and archive (
hasValidOutcomeForSlot).createCheckpointAttestationsFromProposal, the one place both the peer path and the proposer's owncollectOwnAttestationssign through, beside the equivocation check that is there for the same reason. Theendpoint gate keeps its bounded floor past the deadline on purpose: a late node still wants the content verdict
for telemetry, and it is the signature the deadline stops.
Wire format
valid encodings and an old-format payload could parse as a different shape. Flags now accept only 0 and 1, and a
top-level
Bufferdecode must consume its whole input; nested reader decodes are unchanged.Prover node
that outlives the sub-tree, so one started early survives cancellation and keeps proving for a checkpoint nothing
will accept. The whole span and every per-block boundary is validated before any proof work begins.
Inbox bot
l1ToL2SeedCountis optional so an unset value is distinguishable from an explicit one; inbox mode's 512 isapplied from absence, so an explicit
1stays1and is rejected by validation instead of silently becoming 512.operator set on purpose. Availability is computed once, counts in-flight attempts, and is spent per dispatch.
never demonstrated. It now records a failed check (new
replay_unprovenreason) first.failing background job per poll could repeat forever without reaching the unhealthy threshold. Poll and
background failures are now counted apart.
End-to-end
findInsertingBlockretried a false confirmation but let historical reads throw straight out. The whole attemptis inside the retry boundary now, with the last failure as the cause. Extracted to a fixture module so the retry
boundary has a unit test instead of an e2e injection.
Spartan
node signs and submits. The fast-inbox network allocated
8000..8007where it needs8000,8001. Both thedeployment script and the funding calculation now source one helper, checked by
spartan/scripts/prover_publisher_count.test.sh.Behaviour changes worth knowing about
SequencerConfig.allowUnsafeInboxCatchupCapacityis new, defaults to false, and is deliberately notsettable from the environment (like the other test-only flags in that config) so a deployment cannot opt out of
the floor by accident. Any fixture that derives fewer than
MIN_BLOCKS_FOR_INBOX_CATCHUPblock opportunitiesmust set it programmatically or the sequencer refuses to start. The e2e and local-network paths set it;
short-slot production profiles are now correctly rejected.
BotConfig.l1ToL2SeedCountbecame optional (number | undefined).applyInboxModeDefaultsresolves it andreturns the new
ResolvedBotConfig.replay_unproven.PROVER_AGENT_KEDA_ENABLED=true; the keys thatdrop out were never used, but the prefunding list changes. Terraform's
PROVER_COUNT(which drives web3signerkey generation) moves to the same number, so generation and funding cannot disagree. Already-deployed
networks are unaffected: funds already sent stay sent, and the prover node keeps using the first range.
Review
Reviewed by codex (sol/high) before opening. It found three real problems, all fixed here: the deadline check was
bypassable from the proposer's own attestation path; a zero-length endpoint window was both unreliable
(
AbortSignal.timeout(0)aborts on the next tick) and against the finding's intent to let late validation finishfor telemetry; and terraform still generated prover keys from agent capacity. It confirmed the four
"already fixed on main" conclusions and agreed no schema-12 bump is needed.
Verification
yarn build,yarn format --checkandyarn lintclean inyarn-project.(
foundation/src/crypto/poseidonandarchiver/src/modules/data_store_updater.test.ts:656report pre-existingtype errors on
main; they are unchanged here.)prover-node, bot (13 suites / 177 tests).
spartan/scripts/prover_publisher_count.test.shpasses; it fails on the previous sizing with8000,8001,8002,8003,8004,8005,8006,8007.l1_publisher.integration.test.ts, theethereumcontract suites) were notrun locally and are left to CI; they fail the same way on an unmodified
mainin this environment.streaming_inbox.test.tsandmessages.parallel.test.tsare the ones thistouches.