diff --git a/yarn-project/archiver/src/archiver-sync.test.ts b/yarn-project/archiver/src/archiver-sync.test.ts index 67b5d09aea1f..c44d3944278a 100644 --- a/yarn-project/archiver/src/archiver-sync.test.ts +++ b/yarn-project/archiver/src/archiver-sync.test.ts @@ -1114,7 +1114,7 @@ describe('Archiver Sync', () => { expect(await getStoredLeaves()).toEqual(asHex([msgs1[0], msgs1[1], msgs3[0], msgs3[1], msg40, msg50, msg51])); }); - it('short-circuits rollback at the finalized L1 block', async () => { + it('looks up a message recorded at the finalized L1 block before anchoring on it', async () => { // Sync two checkpoints worth of messages so we have history to roll back over. const msgs1 = [Fr.random(), Fr.random()]; fake.addMessages(CheckpointNumber(1), 100n, msgs1); @@ -1122,15 +1122,16 @@ describe('Archiver Sync', () => { const msgs3 = [Fr.random(), Fr.random(), Fr.random(), Fr.random()]; fake.addMessages(CheckpointNumber(3), 101n, msgs3); - // Mark block 100 as finalized so messages there cannot be reorged. + // Finality sits at block 100, the height the first two messages are recorded at. fake.setFinalizedL1BlockNumber(100n); fake.setL1BlockNumber(110n); await archiver.syncImmediate(); expect(await getStoredLeaves()).toEqual(asHex([...msgs1, ...msgs3])); - // Simulate L1 reorg: remove the last 2 messages from checkpoint 3 and add new ones. - fake.removeMessagesAfter(4); + // Simulate L1 reorg: remove every checkpoint-3 message and add a new one, so the search walks back past the + // messages recorded at the finalized block. + fake.removeMessagesAfter(2); const msg40 = Fr.random(); fake.addMessages(CheckpointNumber(4), 102n, [msg40]); @@ -1141,17 +1142,17 @@ describe('Archiver Sync', () => { await archiver.syncImmediate(); - // The two checkpoint-1 messages sit at L1 block 100 (≤ finalized). The rollback loop - // should stop there without issuing a per-message log query for them. + // A recorded height at or below the finalized block is a search hint, not proof that the message is still on + // the chain there, so the anchor at L1 block 100 is the one a lookup found. const callsAtFinalizedOrBelow = eventByHashSpy.mock.calls.filter( ([, aroundL1BlockNumber]) => aroundL1BlockNumber <= 100n, ); - expect(callsAtFinalizedOrBelow).toHaveLength(0); + expect(callsAtFinalizedOrBelow).toHaveLength(1); - expect(await getStoredLeaves()).toEqual(asHex([msgs1[0], msgs1[1], msgs3[0], msgs3[1], msg40])); + expect(await getStoredLeaves()).toEqual(asHex([msgs1[0], msgs1[1], msg40])); }); - it('falls back to per-message log queries when finalized block is undefined', async () => { + it('finds the common point with per-message log queries when no block is finalized yet', async () => { const msgs1 = [Fr.random(), Fr.random()]; fake.addMessages(CheckpointNumber(1), 100n, msgs1); @@ -1173,9 +1174,8 @@ describe('Archiver Sync', () => { await archiver.syncImmediate(); - // Without a finalized pointer the synchronizer must use per-message log queries to find the common point. The - // search starts below the canonical count (5), so msgs3[3] is never looked up: msgs3[2] mismatches on remote and - // msgs3[1] matches, anchoring the replay. + // The search starts below the canonical count (5), so msgs3[3] is never looked up: msgs3[2] mismatches on + // remote and msgs3[1] matches, anchoring the replay. expect(eventByHashSpy).toHaveBeenCalledTimes(2); expect(await getStoredLeaves()).toEqual(asHex([msgs1[0], msgs1[1], msgs3[0], msgs3[1], msg40])); @@ -1635,9 +1635,6 @@ describe('Archiver Sync', () => { }); it('discards a block consuming unchanged messages that were re-mined beyond the lookup window', async () => { - // The finalized marker stays below the messages' L1 block, so the inherited-finality shortcut cannot anchor - // them without a lookup. - fake.setFinalizedL1BlockNumber(95n); const msgs = randomLeaves(3); fake.addMessages(CheckpointNumber(1), 100n, msgs); fake.setL1BlockNumber(110n); @@ -1945,7 +1942,6 @@ describe('Archiver Sync', () => { fake.setL1BlockNumber(115n); await archiver.syncImmediate(); await addLocalBlocksConsuming([4]); - // A replacement chain shorter than every stored height by more than the lookup window, carrying none of the // stored messages. Each candidate's window (95..105 and 105..115) starts above the new head, so bounding it by // the head inverts the range: no anchor is found, and the log rolls back to the deployment block. @@ -2347,7 +2343,7 @@ describe('Archiver Sync', () => { expect(archiver.getL1BlockNumber()).toEqual(111n); }); - it('anchors on the finality marker of the last agreed sync, not on a fresher finalized height', async () => { + it('advances the persisted finality marker only once the log agrees with L1 again', async () => { const msgs = randomLeaves(4); fake.addMessages(CheckpointNumber(1), 100n, msgs.slice(0, 2)); fake.addMessages(CheckpointNumber(1), 101n, msgs.slice(2)); @@ -2356,8 +2352,8 @@ describe('Archiver Sync', () => { await archiver.syncImmediate(); expect((await archiverStore.messages.getMessagesFinalizedL1Block())?.l1BlockNumber).toEqual(90n); - // Every message is replaced and L1 finality has since moved past them. Trusting the fresh height would anchor - // on a stale message without a lookup and try to graft the replacement onto a prefix L1 no longer has. + // Every message is replaced and L1 finality has since moved past the heights they were recorded at. Recovery + // keeps nothing, and the marker the store carries through it does not stop the replacement from landing. const replacement = randomLeaves(4); fake.removeMessagesAfter(0); fake.addMessages(CheckpointNumber(1), 100n, replacement.slice(0, 2)); @@ -2373,6 +2369,39 @@ describe('Archiver Sync', () => { expect((await archiverStore.messages.getMessagesFinalizedL1Block())?.l1BlockNumber).toEqual(150n); }); + it('escapes a recovery whose stored messages sit below the finality marker but were re-mined above it', async () => { + const msgs = randomLeaves(2); + fake.addMessages(CheckpointNumber(1), 100n, msgs); + fake.setFinalizedL1BlockNumber(95n); + fake.setL1BlockNumber(110n); + await archiver.syncImmediate(); + expect(await getStoredLeaves()).toEqual(asHex(msgs)); + + // L1 re-mines both messages thirty blocks later with their content, index and rolling hash intact. The log + // still agrees with the Inbox, so nothing refetches them and their stored rows keep the height they were first + // observed at, while finality passes that height without reaching where they now sit. + fake.moveMessagesToL1Block(100n, 130n); + fake.reorgL1BlocksFrom(100n); + fake.setFinalizedL1BlockNumber(120n); + fake.setL1BlockNumber(135n); + await archiver.syncImmediate(); + expect(await getStoredLeaves()).toEqual(asHex(msgs)); + expect((await archiverStore.messages.getMessagesFinalizedL1Block())?.l1BlockNumber).toEqual(120n); + + // L1 now replaces block 130 on, which is above the marker, so both messages really are gone. Anchoring on + // their recorded height would keep a prefix L1 no longer has and the refetch would never chain onto it. + const replacement = randomLeaves(2); + fake.removeMessagesAfter(0); + fake.addMessages(CheckpointNumber(2), 130n, replacement); + fake.reorgL1BlocksFrom(130n); + fake.setL1BlockNumber(140n); + await archiver.syncImmediate(); + + expect(await getStoredLeaves()).toEqual(asHex(replacement)); + expect(archiver.getL1BlockNumber()).toEqual(140n); + expect(synchronizer.isRecoveringMessages()).toBe(false); + }); + it('re-verifies the persisted syncpoint before ingesting forward, so an empty batch cannot certify a stale tail', async () => { // One slot of L1 blocks per batch: two blocks. A arrives in block 2 and B in block 4; the log is synced to head 4. await useArchiver({ batchSize: 1 }); diff --git a/yarn-project/archiver/src/modules/inbox_message_synchronizer.ts b/yarn-project/archiver/src/modules/inbox_message_synchronizer.ts index 6969d80bfa6e..b3b433b142de 100644 --- a/yarn-project/archiver/src/modules/inbox_message_synchronizer.ts +++ b/yarn-project/archiver/src/modules/inbox_message_synchronizer.ts @@ -115,10 +115,11 @@ export type InboxMessageRecoveryProgress = { * provider's view, not the chain's. When neither reading can settle the ambiguity the pass reports pending rather * than inventing evidence either way. * - * The inherited finalized-height shortcut is kept: a stored message observed at or below the finality marker - * persisted by the last sync that reached agreement with L1 is accepted as an anchor without a lookup, and the marker - * only advances on such agreement. A message re-mined above the finalized height whose old height was below it can - * therefore be trusted wrongly; that exception is deliberately retained and not repaired here. + * A stored message's recorded L1 height is a search hint and never evidence in itself: it says where the message was + * observed, not where the canonical chain carries it now, so every anchor is a message an event lookup found. That + * holds for heights at or below the persisted finality marker too. A message re-mined above the marker keeps the + * height it was first seen at, since a log that still agrees with the Inbox is never refetched, so its recorded + * height would claim a finality the message does not have and anchor the log on a prefix L1 has replaced. */ export class InboxMessageSynchronizer { private recovery: RecoveryState | undefined; @@ -482,14 +483,10 @@ export class InboxMessageSynchronizer { /** * Walks the local log backwards from the current candidate looking for a message L1 still emits at the same index * and hash, spending at most the per-pass lookup budget. Returns the prefix to roll back to, or undefined when the - * budget ran out first. A message at or below the finalized L1 block is accepted without a lookup; a search that - * runs out of candidates keeps nothing and starts again from the deployment block. + * budget ran out first. A search that runs out of candidates keeps nothing and starts again from the deployment + * block. */ private async searchAnchor(recovery: RecoveryState): Promise { - // Only the finality marker persisted by the last sync that reached agreement with L1 is trusted here: a fresher - // finalized height covers messages this node never verified against it, and trusting them would widen the - // inherited shortcut to whatever the local log happens to hold. - const finalizedL1Block = await this.stores.messages.getMessagesFinalizedL1Block(); let lookups = 0; while (true) { const candidateIndex = recovery.nextCandidateIndex; @@ -506,13 +503,6 @@ export class InboxMessageSynchronizer { // with no expected value rather than the Inbox's tip hash, which belongs to a different count entirely. throw new InboxMessagePrefixChangedError(candidateIndex + 1n, undefined, undefined); } - if (finalizedL1Block !== undefined && candidate.l1BlockNumber <= finalizedL1Block.l1BlockNumber) { - this.log.info(`Anchoring L1 to L2 message recovery at finalized L1 block ${candidate.l1BlockNumber}`, { - candidateIndex, - l1BlockNumber: candidate.l1BlockNumber, - }); - return { keep: positionAfter([candidate]), anchorL1Block: candidate.l1BlockNumber }; - } if (lookups >= this.opts.maxAnchorLookupsPerPass) { this.log.verbose(`L1 to L2 message anchor search paused after ${lookups} lookups`, this.getRecoveryProgress()); return undefined;