feat: merge-train/spartan-v5 - #24851
Merged
Merged
Conversation
…bmission-window expiry Decouple "a checkpoint prover failed" (a fact) from "the epoch failed" (a decision). A proving or L1-submission fault now settles the EpochSession in the non-declaring terminal 'stopped' instead of 'failed'; the reconciler rebuilds the epoch over current canonical content each tick (retry-to-converge), cheap because the broker reuses already-completed sub-proofs. An epoch is declared terminally failed — with its post-mortem upload — only when its L1 proof-submission window closes with the proven tip settled and the epoch still unproven, from ProverNode.expireEpoch. This removes the racy, lagging-replica "was this a prune?" classification entirely. Deletes lastTickEpoch (the epoch-keyed anti-retry gate), the checkpointsMatch upload-suppression in SessionManager.runSession, and the onSessionFailed callback. The post-mortem upload moves to tryUploadEpochFailure(epoch, checkpoints), built from the store's last-known canonical provers.
…fault path directly Address review feedback on the retry-to-converge change: - checkEpochExpiry was called from both handleBlockStreamEvent and the periodic ticker, so two sweeps could interleave and both upload a post-mortem for the same epoch before either advanced lastExpiredEpoch. Drop the inline block-stream call: the ticker (a RunningPromise, which never overlaps its own runs) is now the sole driver, so the high-water mark advances — and each epoch uploads — exactly once. Expiry is a background sweep keyed off the archiver's synced slot; it never needed to be on the event path. The A-1041 tips-unadvanced guard now covers only the registration/prune handling that genuinely needs it. - Add a checkpoint-prover test for the actual data-plane race: dbProvider.fork rejecting mid-proof rejects whenBlockProofsReady(), which the EpochSession maps to 'stopped'. Point the expiry unit tests at checkEpochExpiry directly rather than through a block-stream event.
The expiry sweep no longer runs from handleBlockStreamEvent. Rename "Per-event expiry sweep" to "Periodic expiry sweep", redraw the diagram around the expiryTicker (RunningPromise) as the sole driver, and fix the prose: the high-water mark advances per sweep (not per event) and is seeded from resolveLastFullyProvenEpoch. Drop the stale getCheckpointsData and computeStartupState references (expireEpoch uses getBlocks; there is no computeStartupState).
… re-proved every tick Retry-to-converge was naively per-tick: for an epoch that keeps failing, every tick cleared the stopped session and re-created a fresh one, re-running proving work until the deadline for no benefit. Key the retry off content instead, per the original design: record the content key of a full session that ends in 'stopped', and have the tick skip an epoch whose current canonical content matches an already-failed attempt. Recovery is unaffected — it flows through the ungated checkpoint/prune triggers, which fire on a genuine change (a re-add or reorg, including an identical-content re-add whose world-state has resettled) and reopen the epoch regardless. The gate resets when the epoch is proven, expires, or the proven frontier passes it. Only full sessions are affected: partials are opened solely by an explicit startProof and are never reopened by the tick or by events, so they never entered the re-spin loop.
…er mark Replace the content-keyed retry gate (a per-epoch content-key map plus two helpers and record/clear bookkeeping) with the monotonic lastTickEpoch high-water mark: the tick opens an epoch once and does not re-create a session for it every tick. Recovery from a genuine change still flows through the ungated checkpoint/prune triggers, so a pruned-then-re-added epoch recovers exactly as before. The tradeoff — a transient failure on an already-complete epoch waits for the deadline rather than being auto-retried by the tick — is unchanged from the content-keyed version, at a fraction of the machinery.
…nt prover Replace the lastTickEpoch high-water mark with a check at the point of construction: a CheckpointProver whose block proofs rejected for a non-cancel reason (a sub-tree fault or a prune-induced fork fault) now records isFailed(), and the SessionManager refuses to open (or rebuild) an EpochSession over any set that contains a failed prover. A stuck epoch is therefore skipped cheaply each tick — no session, no re-proving — rather than being gated by per-epoch bookkeeping. This keeps the resiliency and drops the tick gate: a pruned/re-added epoch recovers because the re-add installs a fresh (non-failed) prover, and a session that stops with healthy provers (a transient top-tree/submit error) is still retried by the next tick. The failure lives on the prover, where it happened, with the store as the single source of truth.
…upload eagerly, not at expiry
The expiry-time post-mortem upload could never fire for a persistently-failing epoch:
by the time its window closes, its checkpoint provers have been pruned, so there was
nothing to upload (the upload_failed_proof e2e test hung as a result).
Give EpochSession a genuine-failure state, told apart by the checkpoint provers'
isFailed() flag:
- a fault while a prover under it failed → 'stopped' (maybe a prune): not uploaded,
not retried over the failed prover, recovered on re-add.
- the session's own top-tree/submit work failed while every prover was healthy →
'failed' (hasFailed()): definitively not a prune, so it is race-free. The reconciler
retains such a full session (so the tick doesn't re-prove a deterministic failure)
and uploads a post-mortem once, eagerly, from the session's checkpoints.
Removes the fail-at-expiry upload (expireEpoch is back to chonk-release + reap only);
reinstates the onSessionFailed → tryUploadEpochFailure wiring on the genuine-failure
path. Reverts the e2e test to warp-to-epoch-1 + the eager upload trigger.
…st per failed session A checkpoint prover that fails to produce its block proofs (a sub-tree fault or a prune-induced fork fault) now fires an onFailed callback, and ProverNode uploads a snapshot for that single checkpoint via tryUploadCheckpointFailure. This captures a genuine checkpoint proving failure that ends its session in 'stopped' — which the session-level upload (only on a session's own 'failed') deliberately does not cover. The checkpoint upload fires for prune-induced faults too, on purpose: a prune-caused checkpoint snapshot is harmless, and not trying to tell prune from genuine failure is what keeps it race-free. A cancelled prover (control-plane prune / shutdown) is not a failure and does not upload.
…kpoint upload Add rerunCheckpointProvingJob: reuses the epoch rerun's offline setup (world state + archiver snapshot, local broker/prover, replaying tx provider) but rebuilds just the one checkpoint's sub-tree prover and awaits its block proofs — no epoch top-tree or L1 submit. Extract the shared setup into createRerunContext / buildCheckpointProver. Add a test-only checkpointProveOverride hook (CheckpointProverDeps → CheckpointStore setTestHooks → ProverNode.setCheckpointHooks) so a test can force a sub-tree failure, mirroring the existing session topTreeProveOverride hook. Extend upload_failed_proof.e2e with a second test: force a checkpoint sub-tree failure, capture the eager per-checkpoint upload URL via tryUploadCheckpointFailure, download, and re-prove that single checkpoint with rerunCheckpointProvingJob.
…cle flags; drop public isCompleted completed/failed/cancelled are three orthogonal facts, not a single status — a prover can be completed+cancelled (routine teardown) or completed+failed (enqueued then the sub-tree faulted); only failed+cancelled is excluded. Add a comment explaining why they aren't one enum, with per-field docs. isCompleted() had no callers outside tests (internally the `completed` field is used directly), so remove the public getter and the two secondary test assertions that used it.
…ke-epoch-proving-robust-to-prune-induced-fork
…l/a-1418-prover-node-make-epoch-proving-robust-to-prune-induced-fork
… jobId uploadEpochProofFailure already prefixes the upload path with the epoch number, so the epoch in the jobId was redundant — and the epoch-only string dropped the per-upload uniqueness the original session.getId() UUID gave. Use each entity's own id instead: the session's id for a session (epoch) failure, and the prover's content-addressed id for a checkpoint failure. Drop the now-unused epoch param from tryUploadEpochFailure.
Two prune-vs-failure gaps remained after decoupling checkpoint failure from epoch failure: - Session classification only checked isFailed(), missing a control-plane cancel that reaches start()'s catch before the reconcile marks the session 'cancelled'. Such a cancelled-but-not-failed prover was misclassified as the session's own 'failed', triggering a spurious full-snapshot upload for a prune. Treat a cancelled prover as prune-ambiguous too. - A data-plane fork fault reaches the checkpoint-level onFailed upload indistinguishable from a genuine sub-tree failure (no cancel has landed yet). Gate the upload on the archiver: a pruned checkpoint's last block is no longer canonical there, so skip the expensive world-state + archiver snapshot when it has been pruned out.
…24840) ## Problem The safe JSON-RPC server built success responses as `{ jsonrpc, id, result }`. When a handler returns `undefined`, `JSON.stringify` drops the `result` key, producing `{"jsonrpc":"2.0","id":N}` — a response with **neither `result` nor `error`**, which violates JSON-RPC 2.0 and leaves raw/external callers unable to tell "not found" from a malformed reply. Surfaced via `node_getContract` on an undeployed (but valid) address: the response had no `result` field at all. ## Fix One line in the shared server (`safe_json_rpc_server.ts`): coerce an `undefined` return to `null` (`result: result ?? null`, so `0`/`false`/`''` are preserved). The `result` key is now always present. ## Blast radius Framework-level, so it fixes every method that can return `undefined` — ~20 on the node interface (`getContract`, `getContractClass`, `getTxEffect`, `getTxByHash`, `getBlock`, the witness getters, synced slot/epoch, validator stats, …) plus every other server on this framework (PXE, prover-node, archiver). Void methods now also return `result: null`. ## Why it's safe for existing clients The TS client short-circuits `null`/`undefined`/`'null'`/`'undefined'` **before** schema validation, so it never runs `null` through an `.optional()` schema — omitted vs `result: null` both resolve to `undefined` client-side. Only raw/external callers change, and they now get a spec-compliant response. ## Testing - Added a test: a handler returning `undefined` now yields `{ jsonrpc, result: null }` (red before, green after). - Updated the existing void-method (`clear`) assertions, single and batch, to the new shape. - Added integration tests (`test/integration.test.ts`) pinning that optional result schemas tolerate `null` responses: - end-to-end: the wire response for an undefined return carries an explicit `result: null`, and the safe client resolves it to `undefined`; - client-isolated: a stubbed fetch returning `result: null` verbatim resolves to `undefined` against an `.optional()` schema instead of throwing a `ZodError`. Both go red if the client's null short-circuit is removed (verified: `Invalid input: expected object, received null`). - Full `json-rpc` suite: 102 passed; `foundation` lint clean.
AztecBot
enabled auto-merge
July 21, 2026 20:25
Collaborator
Author
|
🤖 Auto-merge enabled after 4 hours of inactivity. This PR will be merged automatically once all checks pass. |
…ults (#24678) Follow-up to #24436 (A-1290). That PR fixed the immediate prune-induced failure but left a class of race conditions around L1-reorg prunes: every "was this failure a prune or a genuine failure?" predicate samples control-plane state that lags the data-plane world-state unwind, so it is racy by construction. The fix pushes the fact down to where it is unambiguous — **decouple "a checkpoint prover failed" (a fact about the prover) from "the epoch failed" (a decision)**, and never treat a *cancel* (prune / reap / shutdown) as a failure. A genuine failure is then told apart from a prune with no control-plane sampling. ## What changes - **`CheckpointProver`** — gains `isFailed()`, set when its block proofs reject for a non-cancel reason (a sub-tree fault or a prune-induced fork fault). On failure it fires an `onFailed` callback so the owner can upload that single checkpoint's post-mortem. A cancel leaves `isFailed()` false. - **`EpochSession`** — a fault now settles in one of two terminal states, decided by the provers: - `stopped` — a `CheckpointProver` under it failed. May be a prune, so it is **not** uploaded and the session is dropped; the epoch recovers when a prune/re-add installs a fresh prover. - `failed` (`hasFailed()`) — the session's **own** top-tree/submit work failed while *every* prover was healthy. Healthy provers rule out a prune, so this is a genuine, race-free failure. - **`SessionManager`** — removed `lastTickEpoch`. Two mechanisms stop a doomed epoch being re-proved every tick: `openFullSessionIfReady` refuses to build over a set containing a failed prover, and `recreateInvalidSessions` **retains** a genuinely-`failed` full session as a do-not-re-prove marker, replacing it only when its canonical content changes (a re-add). A `failed` session fires `onSessionFailed` (session-level upload). Recovery from prune/reorg flows through the ungated `checkpoint`/`prune` triggers, reusing already-completed sub-proofs from the content-addressed broker. - **`ProverNode`** — uploads post-mortems at **two levels**: per checkpoint (`tryUploadCheckpointFailure`, wired to `CheckpointProver.onFailed` — fires for prune-induced faults too, on purpose) and per session (`tryUploadEpochFailure`, wired to `onSessionFailed`). `expireEpoch` only releases the chonk cache and reaps — it does **not** upload, since a missed-window epoch's provers may already be pruned by then. - **Re-proving** — add `rerunCheckpointProvingJob` to re-prove a single downloaded checkpoint in isolation (sub-tree only; no epoch top-tree or L1 submit), sharing offline setup with `rerunEpochProvingJob`. `downloadEpochProvingJob` is unchanged (same serialized format). ## Tests - `epoch-session.test.ts` — a fault ends `stopped` (a prover failed) vs `failed` (top-tree/submit with healthy provers). - `checkpoint-prover.test.ts` — a mid-proof fork fault sets `isFailed()` and fires `onFailed` once; a cancel does neither. - `session-manager.test.ts` — skips building over a failed prover; retains + uploads a `failed` session and replaces it on content change; does not upload on `stopped`; data-plane fault + re-add (identical and different content) recovers and completes. - `prover-node.test.ts` — a registered checkpoint prover failure routes to `tryUploadCheckpointFailure`; `expireEpoch` reaps without uploading. - `upload_failed_proof.test.ts` (e2e) — the existing epoch upload/rerun test, plus a new checkpoint test: force a sub-tree failure via the `checkpointProveOverride` hook, capture the per-checkpoint upload, and re-prove it via `rerunCheckpointProvingJob`. - prover-node README rewritten to the failed-prover / two-level-upload model, with the "how a genuine failure is told apart from a prune" rationale. prover-node unit suite green; full `yarn build`; format + lint clean. Resolves A-1418.
Collaborator
Author
|
🤖 Auto-merge enabled after 4 hours of inactivity. This PR will be merged automatically once all checks pass. |
fcarreiro
added a commit
that referenced
this pull request
Jul 23, 2026
…4939) ## Summary Retargeted to `merge-train/spartan-v5` per request. The matching `merge-train/spartan` port is [aztec-packages#24940](#24940). The merge-train merged Slack alert was reporting the PR head branch as the destination. For [aztec-packages#24851](#24851), the PR head was `merge-train/spartan-v5` but the actual base was `v5-next`, so the Slack message was misleading. ## Changes - Pass `github.event.pull_request.base.ref` to the merged notification step as `TARGET_BRANCH`. - Make `ci3/merge_train_failure_slack_notify --merged` prefer `TARGET_BRANCH`, with a `REF_NAME` fallback for direct/local use. ## Tests - `bash -n ci3/merge_train_failure_slack_notify` - `git diff --check` --- *Created by [claudebox](https://claudebox.work/v2/sessions/85cdf7ecc9727322/jobs/1) · group: `slackbot` · requested by Facundo Carreiro · [Slack thread](https://aztecprotocol.slack.com/archives/C0AU8BULZHC/p1784809391722469?thread_ts=1784809391.722469&cid=C0AU8BULZHC)*
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
4 participants
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.
BEGIN_COMMIT_OVERRIDE
fix(foundation): always include a result key in json-rpc responses (#24840)
feat(prover-node): make epoch proving robust to prune-induced fork faults (#24678)
END_COMMIT_OVERRIDE