From ab72366c47babac0dd5b31381e7e6a9fa139cbde Mon Sep 17 00:00:00 2001 From: Santiago Palladino Date: Fri, 11 Sep 2026 21:05:09 -0300 Subject: [PATCH 1/2] test(archiver): drain the block-triggered sync inside the local-block helper addBlock resolves once the block is stored but triggers a sync it does not await, so every fixture that adds local blocks and then moves the L1 head backwards races the pass left in flight: recovery against the stale head can commit after the pass for the new head and leave the old height as the synced one. Two fixtures hit this, and both carried their own inline drain. Drain once in the shared helper instead, so the hazard is gone for the whole block and the two call-site drains become the no-op second calls they now are. --- yarn-project/archiver/src/archiver-sync.test.ts | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/yarn-project/archiver/src/archiver-sync.test.ts b/yarn-project/archiver/src/archiver-sync.test.ts index f19e2b993b..f5272a2d74 100644 --- a/yarn-project/archiver/src/archiver-sync.test.ts +++ b/yarn-project/archiver/src/archiver-sync.test.ts @@ -1607,7 +1607,14 @@ describe('Archiver Sync', () => { // Local blocks are placed far ahead on L1 so their slot never expires while the tests move the L1 head. const LOCAL_BLOCKS_L1_BLOCK = 5000n; - /** Locally proposed blocks chained on genesis, each consuming through the given message counts. */ + /** + * Locally proposed blocks chained on genesis, each consuming through the given message counts. + * + * `addBlock` resolves once the block is stored but triggers a sync it does not await, so the pass it starts + * outlives this helper with the head captured as it is now. Tests that then move the head backwards would race + * it: recovery against the stale head can commit after the pass for the new head and leave the old height as + * the synced one. Draining it here settles that pass before the caller changes anything. + */ const addLocalBlocksConsuming = async (leafCounts: number[]) => { const { checkpoint } = await mockCheckpointAndMessages(CheckpointNumber(1), { startBlockNumber: BlockNumber(1), @@ -1620,6 +1627,7 @@ describe('Archiver Sync', () => { for (const block of checkpoint.blocks) { await addLocalBlock(block); } + await archiver.syncImmediate(); return checkpoint.blocks; }; const localBlockNumbers = async () => @@ -1972,10 +1980,6 @@ describe('Archiver Sync', () => { fake.setL1BlockNumber(115n); await archiver.syncImmediate(); await addLocalBlocksConsuming([4]); - // addBlock triggers a sync it does not await, and that pass captures the head as it is now. Drain it before - // moving the head backwards: left in flight, it recovers against the pre-reorg head and can commit after the - // pass below, leaving 115 as the synced height. - await archiver.syncImmediate(); // A replacement chain shorter than every stored height, carrying none of the stored messages. Each candidate's // window is clipped to the new head rather than slid down to keep its width, so it cannot reach an event above @@ -2614,9 +2618,6 @@ describe('Archiver Sync', () => { fake.setL1BlockNumber(110n); await archiver.syncImmediate(); await addLocalBlocksConsuming([1, 2]); - // Drain the sync addBlock triggers but does not await, so it cannot commit against the pre-reorg head after - // the pass below and leave 110 as the synced height. - await archiver.syncImmediate(); // L1 really does drop B and shorten: the syncpoint's block is replaced, so nothing vouches for the tail. fake.removeMessagesAfter(1); From 665a73086e9953bcc58676c21e183b06773bcf78 Mon Sep 17 00:00:00 2001 From: Santiago Palladino Date: Fri, 11 Sep 2026 21:05:25 -0300 Subject: [PATCH 2/2] test(archiver): make the re-mine fixture reach recovery The fixture moved the messages to a later L1 block without replacing any block identity, so nothing on L1 ever disagreed with the log: it passed as an ordinary forward append and never reached recovery, while its name claimed it exercised a re-mine beyond the lookup window. Deleting the move left it green and unchanged, which is the proof. It is now split in two. The plain forward append it was really testing keeps its assertions under a name that says so, and a new fixture re-mines the descendants as well: L1 replaces every block from the messages' height on, the messages come back with their content, index and rolling hash intact sixty blocks later, and two more follow them. Every bounded lookup misses, the anchor falls back to the deployment block and the block that consumed the three messages is pruned even though they return unchanged. Discarding work that moved outside the bounded lookup window is the accepted cost of rolling back before refetching, and the test says so rather than only pinning the end state: it counts the three lookups and the prune that recovery performs. --- .../archiver/src/archiver-sync.test.ts | 45 ++++++++++++++++--- 1 file changed, 40 insertions(+), 5 deletions(-) diff --git a/yarn-project/archiver/src/archiver-sync.test.ts b/yarn-project/archiver/src/archiver-sync.test.ts index f5272a2d74..5ac7210ff1 100644 --- a/yarn-project/archiver/src/archiver-sync.test.ts +++ b/yarn-project/archiver/src/archiver-sync.test.ts @@ -1636,23 +1636,21 @@ describe('Archiver Sync', () => { ); const randomLeaves = (count: number) => times(count, () => Fr.random()); - it('re-mines the same messages beyond the lookup window and appends new ones without touching proposed blocks', async () => { + it('appends new messages without disturbing the stored ones or the blocks that consumed them', async () => { const msgs = randomLeaves(3); fake.addMessages(CheckpointNumber(1), 100n, msgs); fake.setL1BlockNumber(110n); await archiver.syncImmediate(); await addLocalBlocksConsuming([3]); - // The messages move 60 L1 blocks later, past the window a lookup around their old height covers, and two new - // ones follow them. - fake.moveMessagesToL1Block(100n, 160n); + // The L1 blocks holding the stored messages are untouched and two new messages follow them: a plain forward + // append, with nothing to look up or roll back. const appended = randomLeaves(2); fake.addMessages(CheckpointNumber(2), 161n, appended); fake.setL1BlockNumber(165n); await archiver.syncImmediate(); expect(await getStoredLeaves()).toEqual(asHex([...msgs, ...appended])); - // Unchanged content is a plain forward append; nothing needed to be looked up or pruned. expect(eventByHashSpy).not.toHaveBeenCalled(); expect(pruneSpy).not.toHaveBeenCalled(); expect(await localBlockNumbers()).toEqual([1]); @@ -1660,6 +1658,43 @@ describe('Archiver Sync', () => { expect(synchronizer.isRecoveringMessages()).toBe(false); }); + it('discards a block consuming unchanged messages that were re-mined beyond the lookup window', async () => { + const msgs = randomLeaves(3); + fake.addMessages(CheckpointNumber(1), 100n, msgs); + fake.setL1BlockNumber(110n); + await archiver.syncImmediate(); + await addLocalBlocksConsuming([3]); + + // L1 replaces every block from 100 on. The three messages survive the replacement with their content, index + // and rolling hash intact, but are re-mined 60 blocks later, past the top of the window a lookup around their + // old height covers, and two new messages follow them. + fake.moveMessagesToL1Block(100n, 160n); + const appended = randomLeaves(2); + fake.addMessages(CheckpointNumber(2), 161n, appended); + fake.reorgL1BlocksFrom(100n); + fake.setL1BlockNumber(165n); + await archiver.syncImmediate(); + + // Every bounded lookup misses, so the anchor falls back to the deployment block and the block that consumed + // the three messages is pruned even though they come straight back unchanged. That is the accepted cost of a + // conservative recovery, not a defect: the rollback precedes the refetch. + expect(eventByHashSpy).toHaveBeenCalledTimes(3); + expect(pruneSpy).toHaveBeenCalledTimes(1); + expect(pruneSpy).toHaveBeenCalledWith( + expect.objectContaining({ blocks: [expect.objectContaining({ number: 1 })] }), + ); + expect(await localBlockNumbers()).toEqual([]); + expect(await getStoredLeaves()).toEqual(asHex([...msgs, ...appended])); + expect(archiver.getL1BlockNumber()).toEqual(165n); + expect(synchronizer.isRecoveringMessages()).toBe(false); + + // Syncing again at the same head refetches from L1 rather than resurrecting the pruned block. + await archiver.syncImmediate(); + expect(await getStoredLeaves()).toEqual(asHex([...msgs, ...appended])); + expect(await localBlockNumbers()).toEqual([]); + expect(pruneSpy).toHaveBeenCalledTimes(1); + }); + it('rolls back to the newest message still found on L1 and re-fetches the rest, dropping unchanged work', async () => { const [a, b, c, d] = randomLeaves(4); fake.addMessages(CheckpointNumber(1), 100n, [a, b]);