Skip to content

Commit 7133758

Browse files
committed
fix(prover-node): don't treat prune-induced faults as genuine failures
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.
1 parent 9d756c7 commit 7133758

4 files changed

Lines changed: 111 additions & 13 deletions

File tree

yarn-project/prover-node/src/job/epoch-session.test.ts

Lines changed: 31 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -321,6 +321,29 @@ describe('EpochSession', () => {
321321
await expect(startResult).resolves.toBe('stopped');
322322
});
323323

324+
it('ends the session in "stopped" (not "failed") when a prover was cancelled by a prune (isCancelled, not isFailed)', async () => {
325+
// A control-plane prune cancels the prover: its blockProofs reject with "cancelled" and it reports
326+
// isCancelled()===true / isFailed()===false. If that rejection reaches start()'s catch before the
327+
// reconcile marks the session 'cancelled', it must NOT be classified 'failed' (which would trigger a
328+
// spurious full-snapshot upload) — a cancelled prover is prune-ambiguous, so classify 'stopped'.
329+
const cancelledProver = makeStubProver(cp, {
330+
blockProofsError: new Error('Checkpoint cancelled'),
331+
isFailed: false,
332+
isCancelled: true,
333+
});
334+
const session = new EpochSession(
335+
makeSpec(),
336+
[cancelledProver],
337+
makeDeps({
338+
hooks: { topTreeProveOverride: () => cancelledProver.whenBlockProofsReady().then(() => synthProof) },
339+
}),
340+
);
341+
const state = await session.start();
342+
expect(state).toBe('stopped');
343+
expect(session.hasFailed()).toBe(false);
344+
expect(publishingService.submit).not.toHaveBeenCalled();
345+
});
346+
324347
it('a top-tree prove that rejects with healthy provers ends the session in "failed" without submitting', async () => {
325348
// The provers are healthy but the top-tree (root) prove itself rejects — the session's own work
326349
// failed, and since no prover failed this is definitively not a prune, so it ends in 'failed'.
@@ -447,7 +470,10 @@ class TestEpochSession extends EpochSession {
447470
* path where CheckpointProver.executeCheckpoint catches an internal failure and rejects
448471
* its blockProofs promise.
449472
*/
450-
function makeStubProver(checkpoint: Checkpoint, opts: { blockProofsError?: Error } = {}): CheckpointProver {
473+
function makeStubProver(
474+
checkpoint: Checkpoint,
475+
opts: { blockProofsError?: Error; isFailed?: boolean; isCancelled?: boolean } = {},
476+
): CheckpointProver {
451477
const id = CheckpointProver.idFor(checkpoint);
452478
// By default whenBlockProofsReady never resolves in these tests; the prove override
453479
// bypasses any path that would actually await it.
@@ -468,9 +494,10 @@ function makeStubProver(checkpoint: Checkpoint, opts: { blockProofsError?: Error
468494
previousArchiveSiblingPath: makeTuple(ARCHIVE_HEIGHT, () => Fr.ZERO),
469495
txs: new Map(),
470496
whenBlockProofsReady: () => blockProofs,
471-
isCancelled: () => false,
472-
// A prover configured with a blockProofsError is one whose block proofs rejected — i.e. failed.
473-
isFailed: () => opts.blockProofsError !== undefined,
497+
isCancelled: () => opts.isCancelled ?? false,
498+
// A prover configured with a blockProofsError is one whose block proofs rejected — i.e. failed,
499+
// unless the caller decouples the two (e.g. to model a cancelled-but-not-failed prune).
500+
isFailed: () => opts.isFailed ?? opts.blockProofsError !== undefined,
474501
cancel: () => {},
475502
whenDone: () => Promise.resolve(),
476503
getAbortSignal: () => new AbortController().signal,

yarn-project/prover-node/src/job/epoch-session.ts

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -229,15 +229,16 @@ export class EpochSession implements Traceable {
229229
...this.spec,
230230
});
231231
// Distinguish the two ways an attempt can fault:
232-
// - a checkpoint prover in the set has failed (a sub-tree fault or prune-induced fork fault):
233-
// end in the non-declaring terminal 'stopped'. This is not the session's own failure and may
234-
// be a prune, so the reconciler does not upload it; a re-add installs a fresh prover.
235-
// - no prover failed, yet top-tree proving or L1 submission failed: this is the session's own,
236-
// genuine failure — and, because every prover succeeded, it is definitively NOT a prune. End
237-
// in terminal 'failed' so the reconciler retains it (no pointless re-prove) and uploads a
238-
// race-free post-mortem.
232+
// - a checkpoint prover in the set has failed OR was cancelled (a sub-tree fault, a prune-induced
233+
// fork fault, or a control-plane cancel that reached this catch before the reconcile marked the
234+
// session 'cancelled'): end in the non-declaring terminal 'stopped'. This is not the session's own
235+
// failure and may be a prune, so the reconciler does not upload it; a re-add installs a fresh prover.
236+
// - no prover failed or was cancelled, yet top-tree proving or L1 submission failed: this is the
237+
// session's own, genuine failure — and, because every prover is healthy and un-cancelled, it is
238+
// definitively NOT a prune. End in terminal 'failed' so the reconciler retains it (no pointless
239+
// re-prove) and uploads a race-free post-mortem.
239240
if (!this.isTerminal()) {
240-
this.state = this.checkpoints.some(c => c.isFailed()) ? 'stopped' : 'failed';
241+
this.state = this.checkpoints.some(c => c.isFailed() || c.isCancelled()) ? 'stopped' : 'failed';
241242
}
242243
} finally {
243244
clearTimeout(this.deadlineTimeoutHandler);

yarn-project/prover-node/src/prover-node.test.ts

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -374,6 +374,46 @@ describe('ProverNode', () => {
374374
expect(uploadSpy).toHaveBeenCalledWith(prover);
375375
});
376376

377+
describe('isCheckpointCanonical', () => {
378+
it('is true when the archiver holds a block at the checkpoint tip with a matching archive root', async () => {
379+
const archiveRoot = Fr.random();
380+
const checkpoint = makeCheckpoint(3, 3, 3, archiveRoot);
381+
l2BlockSource.getBlock.mockResolvedValue({ archive: { root: archiveRoot } } as unknown as L2Block);
382+
383+
await expect(proverNode.callIsCheckpointCanonical(checkpoint)).resolves.toBe(true);
384+
});
385+
386+
it('is false when the checkpoint tip block was pruned out (archiver returns nothing)', async () => {
387+
const checkpoint = makeCheckpoint(3, 3, 3);
388+
l2BlockSource.getBlock.mockResolvedValue(undefined);
389+
390+
await expect(proverNode.callIsCheckpointCanonical(checkpoint)).resolves.toBe(false);
391+
});
392+
393+
it('is false when the tip block was replaced by a reorg (archive root differs)', async () => {
394+
const checkpoint = makeCheckpoint(3, 3, 3, Fr.random());
395+
l2BlockSource.getBlock.mockResolvedValue({ archive: { root: Fr.random() } } as unknown as L2Block);
396+
397+
await expect(proverNode.callIsCheckpointCanonical(checkpoint)).resolves.toBe(false);
398+
});
399+
});
400+
401+
it('tryUploadCheckpointFailure skips the upload for a checkpoint pruned out of the canonical chain', async () => {
402+
// A prune-induced fork fault reaches onFailed just like a genuine sub-tree failure, but the pruned
403+
// checkpoint no longer exists on-chain — the expensive full snapshot must not be produced for it.
404+
(proverNode as any).config.proverNodeFailedEpochStore = 'file:///tmp/does-not-matter';
405+
const checkpoint = makeCheckpoint(3, 3, 3);
406+
l2BlockSource.getBlock.mockResolvedValue(undefined);
407+
const failedProver = { id: 'prover-3', checkpoint } as unknown as Parameters<
408+
typeof proverNode.tryUploadCheckpointFailure
409+
>[0];
410+
411+
await expect(proverNode.tryUploadCheckpointFailure(failedProver)).resolves.toBeUndefined();
412+
expect(l2BlockSource.getBlock).toHaveBeenCalledWith({ number: checkpoint.blocks.at(-1)!.number });
413+
// World-state snapshotting is the first thing the real upload path touches; it must never be reached.
414+
expect(worldState.getSnapshot).not.toHaveBeenCalled();
415+
});
416+
377417
// ---------------- forwarders ----------------
378418

379419
it('startProof forwards to the session manager and returns the job id', async () => {
@@ -772,6 +812,10 @@ class TestProverNode extends ProverNode {
772812
return this.isProvenBlockLastOfItsEpoch(provenBlock, provenEpoch, l1Constants as any);
773813
}
774814

815+
public callIsCheckpointCanonical(checkpoint: Checkpoint) {
816+
return this.isCheckpointCanonical(checkpoint);
817+
}
818+
775819
public getLastExpiredEpoch(): EpochNumber | undefined {
776820
return this.lastExpiredEpoch;
777821
}

yarn-project/prover-node/src/prover-node.ts

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -638,13 +638,25 @@ export class ProverNode implements L2BlockStreamEventHandler, ProverNodeApi, Tra
638638
* Uploads a post-mortem for a single failed checkpoint prover, built from just that checkpoint's
639639
* proving data. Fired (fire-and-forget) from the store's `onFailed` callback for any non-cancel
640640
* block-proof failure — a genuine sub-tree fault or a prune-induced fork fault alike. No-ops if no
641-
* failed-epoch store is configured. Swallows its own errors so a fire-and-forget caller can't leak.
641+
* failed-epoch store is configured, or if the checkpoint is no longer canonical (a prune left nothing
642+
* to diagnose). Swallows its own errors so a fire-and-forget caller can't leak.
642643
*/
643644
public async tryUploadCheckpointFailure(prover: CheckpointProver): Promise<string | undefined> {
644645
if (!this.config.proverNodeFailedEpochStore) {
645646
return undefined;
646647
}
647648
try {
649+
// A prune-induced fork fault and a genuine sub-tree failure are indistinguishable at the moment the
650+
// prover rejects (no control-plane cancel has landed yet). But the archiver is the authoritative
651+
// committed chain: if this checkpoint was pruned out, its last block is no longer canonical there.
652+
// Only upload for a checkpoint that still exists on-chain — a prune leaves nothing to diagnose, and
653+
// the snapshot (full world-state + archiver) is expensive to produce and store.
654+
if (!(await this.isCheckpointCanonical(prover.checkpoint))) {
655+
this.log.debug(`Skipping checkpoint-failure upload for ${prover.id}: no longer canonical (pruned)`, {
656+
checkpointNumber: prover.checkpoint.number,
657+
});
658+
return undefined;
659+
}
648660
const data = SessionManager.buildProvingData([prover]);
649661
return await uploadEpochProofFailure(
650662
this.config.proverNodeFailedEpochStore,
@@ -664,6 +676,20 @@ export class ProverNode implements L2BlockStreamEventHandler, ProverNodeApi, Tra
664676

665677
// ---------------- helpers ----------------
666678

679+
/**
680+
* True if the checkpoint still exists on the canonical chain: the archiver holds a block at its last
681+
* block's height whose archive root matches. A prune (fork fault) leaves the block missing or replaced,
682+
* so this returns false. Protected for direct unit-test access.
683+
*/
684+
protected async isCheckpointCanonical(checkpoint: Checkpoint): Promise<boolean> {
685+
const lastBlock = checkpoint.blocks.at(-1);
686+
if (!lastBlock) {
687+
return false;
688+
}
689+
const onChain = await this.l2BlockSource.getBlock({ number: lastBlock.number });
690+
return !!onChain && onChain.archive.root.equals(checkpoint.archive.root);
691+
}
692+
667693
@memoize
668694
private getL1Constants(): Promise<L1RollupConstants> {
669695
return this.l2BlockSource.getL1Constants();

0 commit comments

Comments
 (0)