diff --git a/l1-contracts/l1-artifacts/scripts/generate-artifacts.sh b/l1-contracts/l1-artifacts/scripts/generate-artifacts.sh index 8a224afdcdb0..5c0245208477 100755 --- a/l1-contracts/l1-artifacts/scripts/generate-artifacts.sh +++ b/l1-contracts/l1-artifacts/scripts/generate-artifacts.sh @@ -15,6 +15,7 @@ contracts=( "EscapeHatch" "SlashingProposer" "EmpireBase" + "EpochProofExtLib" "RollupOperationsExtLib" "ValidatorOperationsExtLib" "RewardExtLib" diff --git a/noir-projects/fnd/noir-protocol-circuits/crates/rollup-lib/src/tests/rollup_fixture_builder.nr b/noir-projects/fnd/noir-protocol-circuits/crates/rollup-lib/src/tests/rollup_fixture_builder.nr index 6ca236b27c8f..78cedab31afc 100644 --- a/noir-projects/fnd/noir-protocol-circuits/crates/rollup-lib/src/tests/rollup_fixture_builder.nr +++ b/noir-projects/fnd/noir-protocol-circuits/crates/rollup-lib/src/tests/rollup_fixture_builder.nr @@ -28,7 +28,7 @@ use types::{ }, hash::{poseidon2_hash, poseidon2_hash_with_separator}, merkle_tree::{ - append_only_tree::append_leaves_to_snapshot, compute_empty_tree_root, MerkleTree, + append_only_tree::append_leaves_to_snapshot, MerkleTree, test_utils::SingleSubtreeMerkleTree, }, proof::proof_data::{AvmV2ProofData, ProofData}, @@ -139,52 +139,6 @@ impl RollupFixtureBuilder { (previous_snapshot, sibling_path_for_insertion) } - pub fn build_l1_to_l2_message_subtree_for_insertion( - self, - slot_number: Field, - ) -> (AppendOnlyTreeSnapshot, [Field; L1_TO_L2_MSG_TREE_HEIGHT - L1_TO_L2_MSG_SUBTREE_HEIGHT], AppendOnlyTreeSnapshot) { - // Create a subtree of size 4 filled with subtree roots before the current slot. - let new_slot_index = self.get_l1_to_l2_message_next_available_leaf_index(slot_number - 1) - as u32 - >> L1_TO_L2_MSG_SUBTREE_HEIGHT; - let start_slot_index = new_slot_index - new_slot_index % 4; - let mut subtree_roots = [0; 4]; - for i in start_slot_index..new_slot_index { - subtree_roots[i % 4] = - self.get_l1_to_l2_message_subtree_root(slot_number - 4 + i as Field); - } - let empty_subtree_root = compute_empty_tree_root::(); - subtree_roots[new_slot_index % 4] = empty_subtree_root; - - let mut tree = SingleSubtreeMerkleTree::<4, 2, L1_TO_L2_MSG_TREE_HEIGHT - L1_TO_L2_MSG_SUBTREE_HEIGHT>::new_tree_roots_at_index::( - subtree_roots, - start_slot_index as Field, - ); - let previous_snapshot = AppendOnlyTreeSnapshot { - root: tree.get_root(), - next_available_leaf_index: self.get_l1_to_l2_message_next_available_leaf_index( - slot_number - 1, - ), - }; - - // Get the sibling path for inserting the current l1-to-l2 messages. - let sibling_path_for_insertion = tree.get_sibling_path(new_slot_index as Field); - - // Insert subtree of the current l1-to-l2 messages. - tree.update_leaf( - new_slot_index as Field, - self.get_l1_to_l2_message_subtree_root(slot_number), - ); - let new_snapshot = AppendOnlyTreeSnapshot { - root: tree.get_root(), - next_available_leaf_index: self.get_l1_to_l2_message_next_available_leaf_index( - slot_number, - ), - }; - - (previous_snapshot, sibling_path_for_insertion, new_snapshot) - } - /// Builds a per-block L1-to-L2 message bundle appended to an empty tree, returning /// `(previous_snapshot, leaves, frontier_hint, new_snapshot)`. The resulting `new_snapshot` is exactly what the /// block root circuit recomputes from the same inputs. @@ -588,10 +542,6 @@ impl RollupFixtureBuilder { } } - fn get_l1_to_l2_message_subtree_root(_self: Self, slot_number: Field) -> Field { - slot_number * 7639901 - } - fn get_l1_to_l2_message_next_available_leaf_index(_self: Self, slot_number: Field) -> Field { (slot_number + 1) * (1 << L1_TO_L2_MSG_SUBTREE_HEIGHT) as Field } diff --git a/yarn-project/archiver/src/archiver-sync.test.ts b/yarn-project/archiver/src/archiver-sync.test.ts index 0d8fa7ff0400..67b5d09aea1f 100644 --- a/yarn-project/archiver/src/archiver-sync.test.ts +++ b/yarn-project/archiver/src/archiver-sync.test.ts @@ -1583,7 +1583,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), @@ -1596,6 +1603,7 @@ describe('Archiver Sync', () => { for (const block of checkpoint.blocks) { await addLocalBlock(block); } + await archiver.syncImmediate(); return checkpoint.blocks; }; const localBlockNumbers = async () => @@ -1604,23 +1612,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 20 L1 blocks later, well past the +-5 window a lookup around their old height covers, - // and two new ones follow them. - fake.moveMessagesToL1Block(100n, 120n); + // 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), 121n, appended); fake.setL1BlockNumber(125n); 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]); @@ -1628,6 +1634,46 @@ describe('Archiver Sync', () => { expect(synchronizer.isRecoveringMessages()).toBe(false); }); + 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); + 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 20 blocks later, past the +-5 window a lookup around their old + // height covers, and two new messages follow them. + fake.moveMessagesToL1Block(100n, 120n); + const appended = randomLeaves(2); + fake.addMessages(CheckpointNumber(2), 121n, appended); + fake.reorgL1BlocksFrom(100n); + fake.setL1BlockNumber(125n); + 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(125n); + 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]); @@ -1772,6 +1818,92 @@ describe('Archiver Sync', () => { expect(archiver.getL1BlockNumber()).toEqual(112n); }); + // A head reporting fewer messages than the local log looks the same whether the chain shortened or this + // provider is behind. The certified syncpoint above it is the only thing that tells them apart, so a + // syncpoint that cannot be read leaves the question open rather than settling it destructively. + describe('shorter head under a syncpoint that cannot be read', () => { + /** Certifies five messages at L1 block 110 and adds two local blocks consuming them. */ + const certifyThroughBlock110 = async () => { + const msgs = randomLeaves(5); + fake.addMessages(CheckpointNumber(1), 100n, msgs.slice(0, 3)); + fake.addMessages(CheckpointNumber(1), 102n, msgs.slice(3)); + fake.setL1BlockNumber(110n); + await archiver.syncImmediate(); + const blocks = await addLocalBlocksConsuming([3, 5]); + return { msgs, blockNumbers: blocks.map(b => b.number) }; + }; + + /** Makes reads of one L1 block fail the way a provider that has not reached it does; returns the undo. */ + const breakReadsOf = (l1BlockNumber: bigint, how: 'throws' | 'no block') => { + const readBlock = publicClient.getBlock.getMockImplementation()!; + publicClient.getBlock.mockImplementation((async (args: { blockNumber?: bigint } = {}) => { + if (args.blockNumber === l1BlockNumber) { + if (how === 'throws') { + throw new Error('connection reset by peer'); + } + return undefined; + } + return await readBlock(args); + }) as any); + return () => publicClient.getBlock.mockImplementation(readBlock); + }; + + /** A provider that has only reached block 105, where the Inbox holds the first three messages. */ + const showLaggedHead = () => { + fake.removeMessagesAfter(3); + fake.setL1BlockNumber(105n); + }; + + it.each(['throws', 'no block'] as const)( + 'keeps the certified messages and their blocks when the syncpoint read %s', + async how => { + const { msgs, blockNumbers } = await certifyThroughBlock110(); + breakReadsOf(110n, how); + showLaggedHead(); + + await archiver.syncImmediate(); + + expect(await getStoredLeaves()).toEqual(asHex(msgs)); + expect(await localBlockNumbers()).toEqual(blockNumbers); + expect(pruneSpy).not.toHaveBeenCalled(); + // The lower head is not advertised as a newly certified view of the log. + expect(archiver.getL1BlockNumber()).toEqual(110n); + }, + ); + + it('truncates once the syncpoint reads back as a different block', async () => { + const { msgs } = await certifyThroughBlock110(); + // Only block 110 is re-mined, so the head at 105 keeps its identity and is positive evidence of a shorter + // chain rather than of a provider that has not caught up. + fake.reorgL1BlocksFrom(110n); + showLaggedHead(); + + await archiver.syncImmediate(); + + expect(await getStoredLeaves()).toEqual(asHex(msgs.slice(0, 3))); + expect(pruneSpy).toHaveBeenCalledTimes(1); + expect(archiver.getL1BlockNumber()).toEqual(105n); + }); + + it('resumes without deleting or refetching anything once the provider catches up', async () => { + const { msgs, blockNumbers } = await certifyThroughBlock110(); + const restoreReads = breakReadsOf(110n, 'throws'); + showLaggedHead(); + await archiver.syncImmediate(); + + restoreReads(); + fake.addMessages(CheckpointNumber(1), 102n, msgs.slice(3)); + fake.setL1BlockNumber(110n); + await archiver.syncImmediate(); + + expect(await getStoredLeaves()).toEqual(asHex(msgs)); + expect(await localBlockNumbers()).toEqual(blockNumbers); + expect(pruneSpy).not.toHaveBeenCalled(); + expect(eventByHashSpy).not.toHaveBeenCalled(); + expect(archiver.getL1BlockNumber()).toEqual(110n); + }); + }); + it('recovers from a same-height head replacement', async () => { const msgs = randomLeaves(3); fake.addMessages(CheckpointNumber(1), 100n, msgs); @@ -1846,7 +1978,7 @@ describe('Archiver Sync', () => { // resumes there. No comparison with the Inbox covered them, so they leave no syncpoint and nothing was // announced. expect(await getStoredLeaves()).toEqual(asHex(msgs.slice(0, 4))); - expect((await archiverStore.messages.getScannedL1Block())?.l1BlockNumber).toEqual(106n); + expect((await archiverStore.messages.getScannedL1Block())?.l1BlockNumber).toEqual(107n); expect(await archiverStore.messages.getSynchedL1Block()).toBeUndefined(); expect(archiver.getL1BlockNumber()).toBeUndefined(); @@ -2105,12 +2237,12 @@ describe('Archiver Sync', () => { const [a] = randomLeaves(1); fake.addMessages(CheckpointNumber(1), 2n, [a]); fake.setL1BlockNumber(4n); - // While the first batch (blocks 1-2) is being fetched, L1 replaces block 2 with a block carrying no message and + // While the batch covering blocks 2-3 is being fetched, L1 replaces block 2 with a block carrying no message and // shortens to it: the batch's logs belong to the old chain, its syncpoint block to the new one. const readLogs = inboxContract.getMessageSentEvents.getMockImplementation()!; inboxContract.getMessageSentEvents.mockImplementation(async (from, to) => { const logs = await readLogs(from, to); - if (to === 2n) { + if (to === 3n) { inboxContract.getMessageSentEvents.mockImplementation(readLogs); fake.removeMessagesAfter(0); fake.reorgL1BlocksFrom(2n); @@ -2289,28 +2421,28 @@ describe('Archiver Sync', () => { fake.addMessages(CheckpointNumber(1), 10n, [c]); fake.setL1BlockNumber(10n); - // The provider answers the blocks 1-2 range without B and then stops serving logs, so the empty blocks 3-4 + // The provider answers the blocks 2-3 range without B and then stops serving logs, so the empty blocks 4-5 // batch is the last one to be scanned. Every block involved stays canonical: only the response was incomplete. const readLogs = inboxContract.getMessageSentEvents.getMockImplementation()!; inboxContract.getMessageSentEvents.mockImplementation(async (from, to) => { const logs = await readLogs(from, to); - return to === 2n ? logs.slice(0, 1) : logs; + return to === 3n ? logs.slice(0, 1) : logs; }); - fake.setMessageSentEventsFailure(from => from >= 5n); + fake.setMessageSentEventsFailure(from => from >= 6n); await expect(archiver.syncImmediate()).rejects.toThrow(/Cannot serve MessageSent logs/); expect(await getStoredLeaves()).toEqual(asHex([a])); expect(archiver.getL1BlockNumber()).toBeUndefined(); - // A later view of L1 ends exactly at block 4, the last block scanned. Nothing has compared the log with the + // A later view of L1 ends exactly at block 5, the last block scanned. Nothing has compared the log with the // Inbox there, so the head must not be answered from the scanned cursor: B is still missing. inboxContract.getMessageSentEvents.mockImplementation(readLogs); fake.setMessageSentEventsFailure(undefined); - fake.setL1BlockNumber(4n); + fake.setL1BlockNumber(5n); await archiver.syncImmediate(); expect(await getStoredLeaves()).toEqual(asHex([a, b])); - expect(archiver.getL1BlockNumber()).toEqual(4n); + expect(archiver.getL1BlockNumber()).toEqual(5n); fake.setL1BlockNumber(10n); await archiver.syncImmediate(); @@ -2354,7 +2486,7 @@ describe('Archiver Sync', () => { const [a] = randomLeaves(1); fake.addMessages(CheckpointNumber(1), 2n, [a]); fake.setL1BlockNumber(4n); - fake.setMessageSentEventsFailure((_from, to) => to >= 3n); + fake.setMessageSentEventsFailure((_from, to) => to >= 4n); await expect(archiver.syncImmediate()).rejects.toThrow(/Cannot serve MessageSent logs/); expect(await getStoredLeaves()).toEqual(asHex([a])); expect(await archiverStore.messages.getSynchedL1Block()).toBeUndefined(); @@ -2369,6 +2501,188 @@ describe('Archiver Sync', () => { expect(archiver.getL1BlockNumber()).toEqual(0n); }); + describe('provider uncertainty versus chain replacement', () => { + /** L1 block numbers the provider currently refuses to answer for, as a momentarily unavailable one would. */ + const unreadable = new Set(); + const withUnreadableL1Blocks = () => { + const readBlock = publicClient.getBlock.getMockImplementation()!; + publicClient.getBlock.mockImplementation(((args: { blockNumber?: bigint } = {}) => + args?.blockNumber !== undefined && unreadable.has(args.blockNumber) + ? Promise.reject(new Error('provider unavailable')) + : readBlock(args)) as any); + }; + + beforeEach(() => { + unreadable.clear(); + withUnreadableL1Blocks(); + }); + + it('does not delete messages when a provider reports a head behind the certified syncpoint', async () => { + const [a, b] = randomLeaves(2); + fake.addMessages(CheckpointNumber(1), 100n, [a]); + fake.addMessages(CheckpointNumber(1), 106n, [b]); + fake.setL1BlockNumber(110n); + await archiver.syncImmediate(); + const [block1, block2] = await addLocalBlocksConsuming([1, 2]); + expect(await getStoredLeaves()).toEqual(asHex([a, b])); + + // A lagged provider answers at block 105, before B was mined. Block 110 is still canonical, so the log and + // the block that consumed B must survive. + fake.setL1BlockNumber(105n); + await archiver.syncImmediate(); + + expect(await getStoredLeaves()).toEqual(asHex([a, b])); + expect(await localBlockNumbers()).toEqual([block1.number, block2.number]); + expect(pruneSpy).not.toHaveBeenCalled(); + expect(archiver.getL1BlockNumber()).toEqual(110n); + + // The provider catches up and nothing had to be refetched. + fake.setL1BlockNumber(110n); + await archiver.syncImmediate(); + expect(await getStoredLeaves()).toEqual(asHex([a, b])); + expect(await localBlockNumbers()).toEqual([block1.number, block2.number]); + }); + + it('truncates to a genuinely shorter chain once the syncpoint block is positively replaced', async () => { + const [a, b] = randomLeaves(2); + fake.addMessages(CheckpointNumber(1), 100n, [a]); + fake.addMessages(CheckpointNumber(1), 106n, [b]); + fake.setL1BlockNumber(110n); + await archiver.syncImmediate(); + await addLocalBlocksConsuming([1, 2]); + + // L1 really does drop B and shorten: the syncpoint's block is replaced, so nothing vouches for the tail. + fake.removeMessagesAfter(1); + fake.reorgL1BlocksFrom(106n); + fake.setL1BlockNumber(105n); + await archiver.syncImmediate(); + + expect(await getStoredLeaves()).toEqual(asHex([a])); + expect(archiver.getL1BlockNumber()).toEqual(105n); + }); + + it('does not start recovery when the scanned cursor block cannot be read', async () => { + await useArchiver({ batchSize: 1 }); + const [a, b] = randomLeaves(2); + fake.addMessages(CheckpointNumber(1), 2n, [a]); + fake.setL1BlockNumber(4n); + await archiver.syncImmediate(); + const [block1] = await addLocalBlocksConsuming([1]); + + fake.addMessages(CheckpointNumber(1), 6n, [b]); + fake.setL1BlockNumber(8n); + unreadable.add(4n); + await archiver.syncImmediate(); + + // The cursor's block was unreadable, so the pass waited rather than rolling the log back. + expect(await getStoredLeaves()).toEqual(asHex([a])); + expect(await localBlockNumbers()).toEqual([block1.number]); + expect(pruneSpy).not.toHaveBeenCalled(); + expect(synchronizer.isRecoveringMessages()).toBe(false); + + // Once the provider answers for it again, the same forward ingestion continues; nothing had to be refetched. + unreadable.clear(); + await archiver.syncImmediate(); + expect(await getStoredLeaves()).toEqual(asHex([a, b])); + expect(pruneSpy).not.toHaveBeenCalled(); + expect(archiver.getL1BlockNumber()).toEqual(8n); + }); + }); + + describe('deployment block ingestion', () => { + // The Inbox's first message can be sent by a later transaction inside the block the contracts were deployed + // in. An exclusive scanned cursor defaulting to that block would resume one block later and never read it. + it('fetches a message emitted in the deployment block when the head is still there', async () => { + const [a] = randomLeaves(1); + fake.addMessages(CheckpointNumber(1), 0n, [a]); + fake.setL1BlockNumber(0n); + await archiver.syncImmediate(); + + expect(await getStoredLeaves()).toEqual(asHex([a])); + expect(synchronizer.isRecoveringMessages()).toBe(false); + }); + + it('fetches a deployment-block message once L1 has advanced past it', async () => { + const [a] = randomLeaves(1); + fake.addMessages(CheckpointNumber(1), 0n, [a]); + fake.setL1BlockNumber(6n); + await archiver.syncImmediate(); + + expect(await getStoredLeaves()).toEqual(asHex([a])); + expect(archiver.getL1BlockNumber()).toEqual(6n); + }); + + it('does not lose index 0 when a later message arrives after it', async () => { + const [a, b] = randomLeaves(2); + fake.addMessages(CheckpointNumber(1), 0n, [a]); + fake.setL1BlockNumber(2n); + await archiver.syncImmediate(); + fake.addMessages(CheckpointNumber(1), 4n, [b]); + fake.setL1BlockNumber(6n); + await archiver.syncImmediate(); + + expect(await getStoredLeaves()).toEqual(asHex([a, b])); + expect(synchronizer.isRecoveringMessages()).toBe(false); + }); + + it('re-reads the deployment block after a restart, one L1 block at a time', async () => { + await useArchiver({ batchSize: 1 }); + const [a] = randomLeaves(1); + fake.addMessages(CheckpointNumber(1), 0n, [a]); + fake.setL1BlockNumber(4n); + await archiver.syncImmediate(); + expect(await getStoredLeaves()).toEqual(asHex([a])); + + // A fresh archiver over the same store resumes from the persisted cursor and must not re-read or duplicate. + const restarted = await buildArchiver('archiver_message_recovery', { batchSize: 1, store: archiverStore }); + try { + fake.setL1BlockNumber(6n); + await restarted.archiver.syncImmediate(); + expect(await getStoredLeaves()).toEqual(asHex([a])); + expect(restarted.synchronizer.isRecoveringMessages()).toBe(false); + } finally { + await restarted.archiver.stop(); + } + }); + + it('unsticks a store whose cursor was already rewound onto the deployment block', async () => { + const [a] = randomLeaves(1); + fake.addMessages(CheckpointNumber(1), 0n, [a]); + // Reproduce what a zero-anchor recovery persists: cursor pinned at the deployment block, empty log. + await archiverStore.messages.setMessageSyncState({ + l1Block: { l1BlockNumber: 0n, l1BlockHash: fake.getL1BlockHash(0n) }, + authenticated: false, + }); + fake.setL1BlockNumber(6n); + await archiver.syncImmediate(); + + expect(await getStoredLeaves()).toEqual(asHex([a])); + expect(synchronizer.isRecoveringMessages()).toBe(false); + }); + + it('stays synced when the deployment block holds no message', async () => { + fake.setL1BlockNumber(4n); + await archiver.syncImmediate(); + + expect(await getStoredLeaves()).toEqual([]); + expect(archiver.getL1BlockNumber()).toEqual(4n); + expect(synchronizer.isRecoveringMessages()).toBe(false); + }); + + it('keeps exclusive semantics once the cursor is past the deployment block', async () => { + const [a, b] = randomLeaves(2); + fake.addMessages(CheckpointNumber(1), 2n, [a]); + fake.setL1BlockNumber(4n); + await archiver.syncImmediate(); + fake.addMessages(CheckpointNumber(1), 6n, [b]); + fake.setL1BlockNumber(8n); + await archiver.syncImmediate(); + + expect(await getStoredLeaves()).toEqual(asHex([a, b])); + expect(synchronizer.isRecoveringMessages()).toBe(false); + }); + }); + it('leaves the batch that refills the log after a rollback uncommitted when it disagrees with the Inbox', async () => { const [a, b, c, d] = randomLeaves(4); fake.addMessages(CheckpointNumber(1), 100n, [a]); diff --git a/yarn-project/archiver/src/errors.ts b/yarn-project/archiver/src/errors.ts index 1b64961a7565..db1d613ed32f 100644 --- a/yarn-project/archiver/src/errors.ts +++ b/yarn-project/archiver/src/errors.ts @@ -109,11 +109,12 @@ export class BlockAlreadyCheckpointedError extends Error { export class InboxMessagePrefixChangedError extends Error { constructor( public readonly totalMessageCount: bigint, - public readonly expected: Fr, + /** The rolling hash the prefix was expected to have, when a caller knows it for this exact count. */ + public readonly expected: Fr | undefined, public readonly actual: Fr | undefined, ) { super( - `Inbox message prefix at count ${totalMessageCount} changed from ${expected.toString()} to ` + + `Inbox message prefix at count ${totalMessageCount} changed from ${expected?.toString() ?? 'unavailable'} to ` + `${actual?.toString() ?? 'unavailable'} while a replacement was being prepared`, ); this.name = 'InboxMessagePrefixChangedError'; diff --git a/yarn-project/archiver/src/modules/data_store_updater.ts b/yarn-project/archiver/src/modules/data_store_updater.ts index f69f4d239162..898208c1246f 100644 --- a/yarn-project/archiver/src/modules/data_store_updater.ts +++ b/yarn-project/archiver/src/modules/data_store_updater.ts @@ -407,7 +407,7 @@ export class ArchiverDataStoreUpdater { if (block === undefined || block.header.getBlockNumber() !== blockNumber) { throw new Error(`Block ${blockNumber} is missing from the store`); } - return BigInt(block.header.state.l1ToL2MessageTree.nextAvailableLeafIndex); + return blockLeafCount(block); } /** diff --git a/yarn-project/archiver/src/modules/inbox_message_synchronizer.ts b/yarn-project/archiver/src/modules/inbox_message_synchronizer.ts index 706bb4addfa4..6969d80bfa6e 100644 --- a/yarn-project/archiver/src/modules/inbox_message_synchronizer.ts +++ b/yarn-project/archiver/src/modules/inbox_message_synchronizer.ts @@ -37,7 +37,6 @@ type RecoveryState = { head: L1BlockId; /** The Inbox's position at `head`. */ remote: InboxContractState; - finalizedL1Block: L1BlockId | undefined; /** The next stored message to look up on L1, or undefined once the search has run out of candidates. */ nextCandidateIndex: bigint | undefined; /** Number of per-message event lookups made so far, for progress reporting. */ @@ -86,7 +85,9 @@ export type InboxMessageRecoveryProgress = { * needs no event lookups), or a stored message whose event L1 still emits at the same index and hash within five L1 * blocks of the height it was observed at, found by walking the log backwards with a bounded number of event lookups * per pass. A lookup that misses moves the search to an older candidate, and running out of candidates falls back to - * the deployment block. Once an anchor is chosen the log is rolled back to it in one store transaction: the suffix + * the deployment block, which is itself re-read: the Inbox's first message can be emitted by a later transaction in + * the block the contracts were deployed in, so the deployment block is the one block an exclusive cursor may not + * skip. Once an anchor is chosen the log is rolled back to it in one store transaction: the suffix * rows are deleted, the proposed blocks that consumed more messages than the retained count are pruned with their * descendants, the scanned cursor rewinds to the block before the anchor's and the syncpoint is cleared. Nothing is * fetched in that pass; ordinary forward ingestion refills the log from the rewound cursor, rewriting the retained @@ -100,11 +101,20 @@ export type InboxMessageRecoveryProgress = { * deleted rows are re-fetched, and published checkpoints are never deleted by this path. An RPC exception is not a * miss and commits nothing. * - * Recovery is pinned to the head it started against: a merely advancing `latest` does not reset it, only a replaced - * or unavailable head does. Event lookups are bounded above by that head, so an anchor can never sit at or past it - * and leave the rewound cursor unreachable. The search position is process-local; after a restart, anchor discovery + * Recovery is pinned to the head it started against: a merely advancing `latest` does not reset it, only a positively + * replaced head does. Event lookups are bounded above by that head, so an anchor can never sit at or past it and + * leave the rewound cursor unreachable. The search position is process-local; after a restart, anchor discovery * starts over from the stored log, which is correct. * + * Every L1 block this class depends on is checked with three outcomes, not two: canonical, positively replaced, or + * unreadable. An RPC exception, a provider behind the height and a pruned range all read as unreadable, and none of + * them is evidence of a reorg. Unreadable therefore commits nothing, deletes nothing, logs no replacement and keeps + * an in-flight recovery's search position; only a block that reads back with a different hash restarts recovery. For + * the same reason a head reporting fewer messages than the local log is not truncated against while a syncpoint + * above it is still canonical: that syncpoint certified the whole log at a higher block, so the shortfall is the + * 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 @@ -165,11 +175,17 @@ export class InboxMessageSynchronizer { private async syncPass(head: L1BlockId, finalizedL1Block: L1BlockId | undefined): Promise { if (this.recovery !== undefined) { - const pinnedHead = this.recovery.head; - if (await this.isHeadStillCanonical(pinnedHead)) { - const result = await this.continueRecovery(); - // Recovery is complete relative to the head it was pinned to; blocks after it still need normal ingestion. - return result.status === 'synced' && !sameL1Block(pinnedHead, head) ? { ...result, status: 'pending' } : result; + const pinnedStatus = await this.checkL1Block(this.recovery.head); + if (pinnedStatus === 'canonical') { + return await this.continueRecovery(); + } + if (pinnedStatus === 'unknown') { + // The pinned head could not be read. That is a provider problem, not evidence its chain is gone: keep the + // search position and its lookup progress and try again next pass. + this.log.verbose(`Could not confirm the L1 head recovery is pinned to; keeping recovery progress`, { + ...this.getRecoveryProgress(), + }); + return pending(); } this.log.warn(`L1 head ${this.recovery.head.l1BlockNumber} recovery was pinned to has been replaced`, { ...this.getRecoveryProgress(), @@ -198,8 +214,8 @@ export class InboxMessageSynchronizer { const local = await this.stores.messages.getSyncedMessagePosition(); if (positionMatches(local, remote)) { // The state was read by block number: only a head that is still canonical proves it was this head's. - if (!(await this.isHeadStillCanonical(head))) { - this.log.verbose(`L1 head ${head.l1BlockNumber} was replaced while reading the Inbox state`); + if ((await this.checkL1Block(head)) !== 'canonical') { + this.log.verbose(`Could not confirm L1 head ${head.l1BlockNumber} after reading the Inbox state`); return pending(); } await this.stores.messages.setMessageSyncState({ l1Block: head, authenticated: true, finalizedL1Block }); @@ -207,41 +223,73 @@ export class InboxMessageSynchronizer { } if (remote.totalMessagesInserted < local.totalMessageCount) { + // A head shorter than the local log is ambiguous: the chain really did shorten, or this provider is behind the + // one the log was certified against. A retained syncpoint above this head that is still canonical settles it as + // lag, and lag must not delete messages or claim a lower head as synced. + const laterSyncPoint = await this.checkLaterSyncPoint(head, persistedSyncPoint); + if (laterSyncPoint === 'lagged') { + return pending(); + } // A shorter canonical sequence whose tip hash is our prefix hash at that count is a pure truncation; the tip // itself proves where it ends, so no old placement lookup is needed. const localAtRemote = await this.stores.messages.getMessagePosition(remote.totalMessagesInserted); if (localAtRemote !== undefined && localAtRemote.rollingHash.equals(remote.rollingHash)) { - if (!(await this.isHeadStillCanonical(head))) { - this.log.verbose(`L1 head ${head.l1BlockNumber} was replaced while reading the Inbox state; not truncating`); + // A head that agrees with the local log through its own count is what a provider that has not caught up + // looks like, so shortening to it needs the later certified syncpoint to have been positively replaced. An + // unreadable syncpoint is no such evidence: an older canonical ancestor cannot prove the messages certified + // above it are gone, and deleting them would prune the speculative blocks that consumed them for nothing. + if (laterSyncPoint === 'unknown') { + return pending(); + } + if ((await this.checkL1Block(head)) !== 'canonical') { + this.log.verbose( + `Could not confirm L1 head ${head.l1BlockNumber} after reading the Inbox state; ` + `not truncating`, + ); return pending(); } return this.truncate(localAtRemote, head, finalizedL1Block); } - return this.startRecovery(head, remote, finalizedL1Block); + // The head disagrees with the local log at the head's own count, so this is not a view of the same chain that + // has yet to catch up: recovery searches L1 for a common anchor and only rolls back to one it found there. + return this.startRecovery(head, remote); } - if (head.l1BlockNumber <= cursor.l1BlockNumber) { - // A head at or below what has already been scanned, and the log does not agree with it: there is no forward + const ingestFrom = this.ingestionStartFor(cursor); + if (head.l1BlockNumber < ingestFrom) { + // A head below the first block still to be scanned, and the log does not agree with it: there is no forward // range to fetch, so find where the local log and the canonical one part ways. - return this.startRecovery(head, remote, finalizedL1Block); + return this.startRecovery(head, remote); } // Forward ingestion inherits the scanned log as canonical, and the head batch's comparison then certifies that // inherited prefix. That is only sound while the cursor's block is still on the chain: after a reorg below it, the // messages read up to it may belong to a chain L1 no longer has, so the log has to be compared with the canonical // one instead. - if (persistedCursor !== undefined && !(await this.isHeadStillCanonical(persistedCursor))) { - this.log.warn(`L1 block ${cursor.l1BlockNumber} the message log was scanned through has been replaced`, { - cursor, - syncPoint: persistedSyncPoint, - headL1BlockNumber: head.l1BlockNumber, - }); - return this.startRecovery(head, remote, finalizedL1Block); + if (persistedCursor !== undefined) { + const cursorStatus = await this.checkL1Block(persistedCursor); + if (cursorStatus === 'unknown') { + // Nothing was learned about the cursor's block, so the inherited prefix is neither proven nor disproven. + // Fetching forward would inherit an unverified prefix and recovering would delete on no evidence: wait. + this.log.verbose(`Could not confirm L1 block ${cursor.l1BlockNumber} the message log was scanned through`, { + cursor, + syncPoint: persistedSyncPoint, + headL1BlockNumber: head.l1BlockNumber, + }); + return pending(); + } + if (cursorStatus === 'replaced') { + this.log.warn(`L1 block ${cursor.l1BlockNumber} the message log was scanned through has been replaced`, { + cursor, + syncPoint: persistedSyncPoint, + headL1BlockNumber: head.l1BlockNumber, + }); + return this.startRecovery(head, remote); + } } let headBatch: InboxMessage[]; try { - headBatch = await this.ingestForward(cursor.l1BlockNumber + 1n, head); + headBatch = await this.ingestForward(ingestFrom, head); } catch (err) { if (err instanceof CapturedHeadReplacedError) { this.log.verbose(`L1 head ${head.l1BlockNumber} was replaced while fetching L1 to L2 messages`); @@ -251,15 +299,15 @@ export class InboxMessageSynchronizer { this.log.warn(`Fetched L1 to L2 messages do not continue the local log: ${err.message}`, { inboxMessage: err.inboxMessage, }); - return this.startRecovery(head, remote, finalizedL1Block); + return this.startRecovery(head, remote); } throw err; } - if (!(await this.isHeadStillCanonical(head))) { - // The chain moved under the fetch: the logs may belong to another chain than the position they are compared - // with, so neither the head batch nor a recovery is committed; the next pass reads the replacement head. - this.log.verbose(`L1 head ${head.l1BlockNumber} was replaced while fetching L1 to L2 messages`); + if ((await this.checkL1Block(head)) !== 'canonical') { + // The chain may have moved under the fetch: the logs would then belong to another chain than the position they + // are compared with, so neither the head batch nor a recovery is committed; the next pass reads a fresh head. + this.log.verbose(`Could not confirm L1 head ${head.l1BlockNumber} after fetching L1 to L2 messages`); return pending(); } const positionAfterHeadBatch = @@ -274,13 +322,72 @@ export class InboxMessageSynchronizer { this.log.warn(`Head batch of L1 to L2 messages does not continue the local log: ${err.message}`, { inboxMessage: err.inboxMessage, }); - return this.startRecovery(head, remote, finalizedL1Block); + return this.startRecovery(head, remote); } throw err; } return synced(); } - return this.startRecovery(head, remote, finalizedL1Block); + return this.startRecovery(head, remote); + } + + /** + * What a retained syncpoint above a head shorter than the local log says about that head. + * + * The syncpoint is the highest L1 block at which the whole stored log was found equal to the Inbox's own position. + * If that block is above this head and still canonical, the chain did not shorten past it: the messages the head + * appears to be missing are on the chain, and this provider simply has not reached them. Deleting them here would + * throw away certified messages and prune the proposed blocks that consumed them, only for the next pass to fetch + * them straight back. + * + * A syncpoint that cannot be read is the same three-outcome question as any other block, and the answer has to + * survive to the caller: an unreadable one is no evidence that the certified messages above the head are gone, and + * truncating on it would delete them on the strength of a provider that could not answer. Only a syncpoint that + * reads back with a different hash is positive evidence the certified view was replaced, which the ordinary + * shorter-head path is then free to act on. + */ + private async checkLaterSyncPoint(head: L1BlockId, syncPoint: L1BlockId | undefined): Promise { + if (syncPoint === undefined || syncPoint.l1BlockNumber <= head.l1BlockNumber) { + return 'none'; + } + const status = await this.checkL1Block(syncPoint); + if (status === 'unknown') { + this.log.verbose( + `Could not confirm the certified syncpoint at ${syncPoint.l1BlockNumber} above L1 head ` + + `${head.l1BlockNumber}; keeping the message log until the shorter head can be explained`, + { headL1BlockNumber: head.l1BlockNumber, syncPointL1BlockNumber: syncPoint.l1BlockNumber }, + ); + return 'unknown'; + } + if (status === 'replaced') { + this.log.warn( + `Certified syncpoint at ${syncPoint.l1BlockNumber} has been replaced; the shorter L1 head ` + + `${head.l1BlockNumber} is handled as a chain replacement`, + { headL1BlockNumber: head.l1BlockNumber, syncPointL1BlockNumber: syncPoint.l1BlockNumber }, + ); + return 'replaced'; + } + this.log.verbose( + `L1 head ${head.l1BlockNumber} is behind the certified syncpoint at ${syncPoint.l1BlockNumber}, which is ` + + `still canonical; keeping the message log and waiting for the provider to catch up`, + { headL1BlockNumber: head.l1BlockNumber, syncPointL1BlockNumber: syncPoint.l1BlockNumber }, + ); + return 'lagged'; + } + + /** + * First L1 block ordinary ingestion must read, given the scanned cursor. + * + * The cursor is exclusive, so fetching normally resumes at the block after it. The deployment block is the + * exception: message index 0 can be emitted by a later transaction in that very block, and a cursor sitting at it + * means nothing has read it yet — that is where the archiver starts with no persisted cursor, and where the + * zero-anchor rollback rewinds to. Resuming one block later would skip index 0 permanently, since no later message + * can fill the gap and every pass would rediscover the same disagreement. Only the deployment block is re-read; + * genuine completed cursors keep exclusive semantics, and re-reading it is harmless because the store rewrites an + * unchanged message in place. + */ + private ingestionStartFor(cursor: L1BlockId): bigint { + return cursor.l1BlockNumber <= this.l1Start.l1BlockNumber ? this.l1Start.l1BlockNumber : cursor.l1BlockNumber + 1n; } /** @@ -307,7 +414,7 @@ export class InboxMessageSynchronizer { // Logs and the batch-end block are read by number: only a head still canonical after both reads proves they // came from the captured chain, so a batch is never committed under a replacement chain's cursor. const l1Block = await this.l1BlockIdFor(end, head); - if (!(await this.isHeadStillCanonical(head))) { + if ((await this.checkL1Block(head)) !== 'canonical') { throw new CapturedHeadReplacedError(head); } await this.storeMessages(messages, { l1Block, authenticated: false }); @@ -336,11 +443,7 @@ export class InboxMessageSynchronizer { } } - private async startRecovery( - head: L1BlockId, - remote: InboxContractState, - finalizedL1Block: L1BlockId | undefined, - ): Promise { + private async startRecovery(head: L1BlockId, remote: InboxContractState): Promise { const local = await this.stores.messages.getSyncedMessagePosition(); // Messages past the canonical count cannot be on the canonical chain at their index, so the search for a common // message starts at the canonical tip or the local one, whichever is lower. @@ -348,7 +451,6 @@ export class InboxMessageSynchronizer { this.recovery = { head, remote, - finalizedL1Block, nextCandidateIndex: lastCandidate < 0n ? undefined : lastCandidate, lookups: 0, startedAt: new Timer(), @@ -396,11 +498,13 @@ export class InboxMessageSynchronizer { headL1BlockNumber: recovery.head.l1BlockNumber, lookups: recovery.lookups, }); - return { keep: zeroMessagePosition(), anchorL1Block: this.l1Start.l1BlockNumber + 1n }; + return { keep: zeroMessagePosition(), anchorL1Block: this.l1Start.l1BlockNumber }; } const candidate = await this.stores.messages.getL1ToL2Message(candidateIndex); if (candidate === undefined) { - throw new InboxMessagePrefixChangedError(candidateIndex + 1n, recovery.remote.rollingHash, undefined); + // The row the search expected is gone; nothing here knows what its rolling hash was, so report the position + // 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}`, { @@ -459,9 +563,17 @@ export class InboxMessageSynchronizer { const cursor = await this.l1BlockIdFor(maxBigint(anchor.anchorL1Block - 1n, this.l1Start.l1BlockNumber), head); // The anchor event and the cursor block were both read by number: only a head still canonical after both reads // proves they came from the captured chain, so a rollback never commits against a replacement chain. - if (!(await this.isHeadStillCanonical(head))) { - this.log.warn(`L1 head ${head.l1BlockNumber} was replaced during L1 to L2 message recovery; restarting`); - this.recovery = undefined; + const headStatus = await this.checkL1Block(head); + if (headStatus !== 'canonical') { + // A replaced head invalidates the anchor evidence, so the search starts over against the new view. An + // unreadable one proves nothing: keep the recovery and its lookup progress and retry next pass. + this.log.warn(`Could not confirm L1 head ${head.l1BlockNumber} during L1 to L2 message recovery`, { + headStatus, + ...this.getRecoveryProgress(), + }); + if (headStatus === 'replaced') { + this.recovery = undefined; + } return pending(); } this.log.warn( @@ -511,17 +623,43 @@ export class InboxMessageSynchronizer { return { l1BlockNumber, l1BlockHash: Buffer32.fromString(block.hash) }; } - private async isHeadStillCanonical(head: L1BlockId): Promise { + /** + * Whether an L1 block this pass depends on is still the one that was captured. + * + * An exception or a missing answer is `unknown`, not `replaced`: a provider that lags behind the height, is + * temporarily unreachable, or answers a pruned range cannot distinguish a reorg from its own view. Treating that + * as a replacement would delete messages and restart recovery on nothing more than an RPC failure, so callers keep + * their pending work and retry instead. Only a block that reads back with a different hash is `replaced`. + */ + private async checkL1Block(block: L1BlockId): Promise { + let remote; try { - const block = await this.publicClient.getBlock({ blockNumber: head.l1BlockNumber, includeTransactions: false }); - return Buffer32.fromString(block.hash).equals(head.l1BlockHash); + remote = await this.publicClient.getBlock({ blockNumber: block.l1BlockNumber, includeTransactions: false }); } catch (err) { - this.log.debug(`Could not read L1 block ${head.l1BlockNumber} to confirm the captured head: ${err}`); - return false; + this.log.debug(`Could not read L1 block ${block.l1BlockNumber} to confirm it is still canonical: ${err}`); + return 'unknown'; + } + if (remote?.hash === undefined || remote.hash === null) { + this.log.debug(`L1 block ${block.l1BlockNumber} was returned without a hash; canonicality is unknown`); + return 'unknown'; } + return Buffer32.fromString(remote.hash).equals(block.l1BlockHash) ? 'canonical' : 'replaced'; } } +/** + * Whether a captured L1 block is still the canonical one at its height, was positively replaced, or could not be + * read. `unknown` is deliberately not merged into `replaced`: only the latter is evidence of a chain replacement. + */ +type L1BlockStatus = 'canonical' | 'replaced' | 'unknown'; + +/** + * What a retained certified syncpoint says about a head that reports fewer messages than the local log: there is no + * later syncpoint to ask, the syncpoint is still canonical so the head is a lagged view, the syncpoint was positively + * replaced, or it could not be read and the shortfall stays unexplained. + */ +type LaterSyncPointStatus = 'none' | 'lagged' | 'replaced' | 'unknown'; + /** The L1 head a sync pass was captured against is no longer canonical; the pass's uncommitted work is discarded. */ class CapturedHeadReplacedError extends Error { constructor(head: L1BlockId) { diff --git a/yarn-project/archiver/src/modules/l1_synchronizer.ts b/yarn-project/archiver/src/modules/l1_synchronizer.ts index 93d15e109d37..59cb2852d991 100644 --- a/yarn-project/archiver/src/modules/l1_synchronizer.ts +++ b/yarn-project/archiver/src/modules/l1_synchronizer.ts @@ -42,7 +42,7 @@ import { import type { RejectedCheckpoint } from '../store/block_store.js'; import { type ArchiverDataStores, getArchiverSynchPoint } from '../store/data_stores.js'; import type { L2TipsCache } from '../store/l2_tips_cache.js'; -import { ArchiverDataStoreUpdater } from './data_store_updater.js'; +import { ArchiverDataStoreUpdater, blockLeafCount } from './data_store_updater.js'; import { InboxMessageSynchronizer } from './inbox_message_synchronizer.js'; import type { ArchiverInstrumentation } from './instrumentation.js'; import { validateCheckpointAttestationsFromCalldata } from './validation.js'; @@ -549,7 +549,7 @@ export class ArchiverL1Synchronizer implements Traceable { if (lastBlock === undefined || lastBlock.header.getBlockNumber() !== lastBlockNumber) { return false; } - const consumedCount = BigInt(lastBlock.header.state.l1ToL2MessageTree.nextAvailableLeafIndex); + const consumedCount = blockLeafCount(lastBlock); const position = await this.stores.messages.getMessagePosition(consumedCount); return position !== undefined && position.rollingHash.equals(checkpoint.header.inboxRollingHash); } diff --git a/yarn-project/aztec-node/src/sentinel/sentinel.test.ts b/yarn-project/aztec-node/src/sentinel/sentinel.test.ts index ee95f5af5187..e61c2fdbc699 100644 --- a/yarn-project/aztec-node/src/sentinel/sentinel.test.ts +++ b/yarn-project/aztec-node/src/sentinel/sentinel.test.ts @@ -29,6 +29,7 @@ import { import type { ValidatorStats, ValidatorStatusHistory, + ValidatorStatusInSlot, ValidatorsEpochPerformance, ValidatorsStats, } from '@aztec/stdlib/validators'; @@ -194,6 +195,26 @@ describe('sentinel', () => { expect(activity[proposer.toString()]).toEqual('checkpoint-unvalidated'); }); + // An observer that ran out of time, or could not check a proposal against L1, records that it saw one, so the + // slot is not read as one the proposer skipped — the fallback below would otherwise make it checkpoint-missed + // or blocks-missed, both of which are counted against the proposer. + it('flags checkpoint as unverifiable when tracker outcome is unverifiable', async () => { + reexecutionTracker.recordOutcome(slot, block.archive.root, 'unverifiable', CheckpointNumber(1)); + p2p.getCheckpointAttestationsForSlot.mockResolvedValue([]); + p2p.hasBlockProposalsForSlot.mockResolvedValue(true); + const activity = await sentinel.getSlotActivity(slot, epoch, proposer, committee); + expect(activity[proposer.toString()]).toEqual('checkpoint-unverifiable'); + }); + + it('does not tag attestors as missed when the checkpoint is unverifiable', async () => { + reexecutionTracker.recordOutcome(slot, block.archive.root, 'unverifiable', CheckpointNumber(1)); + p2p.getCheckpointAttestationsForSlot.mockResolvedValue(attestations.slice(0, -1)); + + const activity = await sentinel.getSlotActivity(slot, epoch, proposer, committee); + expect(activity[proposer.toString()]).toEqual('checkpoint-unverifiable'); + expect(activity[committee[3].toString()]).not.toBe('attestation-missed'); + }); + it('flags as blocks-missed when there is no tracker outcome and no block proposals (case 1)', async () => { p2p.getCheckpointAttestationsForSlot.mockResolvedValue([]); p2p.hasBlockProposalsForSlot.mockResolvedValue(false); @@ -407,6 +428,40 @@ describe('sentinel', () => { expect(stats.missedProposals.total).toEqual(5); }); + // The taxonomy's whole point: a slot this node could not check must not reach the proposer's inactivity + // accounting, while the slots it could check keep the accounting they always had. + it('does not count checkpoint-unverifiable as a missed proposal', () => { + const stats = sentinel.computeStatsForValidator(validator, [ + { slot: SlotNumber(1), status: 'checkpoint-mined' }, + { slot: SlotNumber(2), status: 'checkpoint-unverifiable' }, + { slot: SlotNumber(3), status: 'checkpoint-unverifiable' }, + { slot: SlotNumber(4), status: 'checkpoint-invalid' }, + { slot: SlotNumber(5), status: 'checkpoint-missed' }, + ]); + + expect(stats.missedProposals.count).toEqual(2); + // Out of the denominator too, so unknowns cannot dilute the misses that are real: two of the three slots + // this node could actually assess were missed, not two of five. + expect(stats.missedProposals.total).toEqual(3); + expect(stats.missedProposals.rate).toBeCloseTo(2 / 3); + // Nor do they break the streak the two real misses form. + expect(stats.missedProposals.currentStreak).toEqual(2); + }); + + // A proposer whose checkpoints this node can never check must not come out looking better than one whose + // checkpoints it can: an unassessable slot is not a slot survived. + it('does not let unverifiable slots dilute the missed-proposal rate', () => { + const stats = sentinel.computeStatsForValidator(validator, [ + { slot: SlotNumber(1), status: 'checkpoint-missed' }, + ...times(9, (i): { slot: SlotNumber; status: ValidatorStatusInSlot } => ({ + slot: SlotNumber(i + 2), + status: 'checkpoint-unverifiable', + })), + ]); + + expect(stats.missedProposals).toEqual(expect.objectContaining({ count: 1, total: 1, rate: 1, currentStreak: 1 })); + }); + it('resets streaks correctly', () => { const stats = sentinel.computeStatsForValidator(validator, [ { slot: SlotNumber(1), status: 'checkpoint-mined' }, diff --git a/yarn-project/aztec-node/src/sentinel/sentinel.ts b/yarn-project/aztec-node/src/sentinel/sentinel.ts index ef2fc3c69340..9ff5d4785293 100644 --- a/yarn-project/aztec-node/src/sentinel/sentinel.ts +++ b/yarn-project/aztec-node/src/sentinel/sentinel.ts @@ -41,6 +41,14 @@ export type SentinelRuntimeConfig = Pick< Pick & Pick; +/** + * Statuses that record only that this node could not assess the slot. They are evidence of nothing, so they are + * left out of both halves of a missed-duty rate: counting them as misses would charge a validator for an + * observer's failure, and counting them as successes would let unknowns dilute the misses that are real. A slot + * nobody could assess simply does not count towards the rate. + */ +const UNASSESSABLE_STATUSES: ValidatorStatusInSlot[] = ['checkpoint-unverifiable']; + /** Maps a validator status to its category: proposer or attestation. */ function statusToCategory(status: ValidatorStatusInSlot): ValidatorStatusType { switch (status) { @@ -78,10 +86,9 @@ function statusToCategory(status: ValidatorStatusInSlot): ValidatorStatusType { * Triggering per-epoch evaluation off local L2 state — rather than waiting for L1 proof * publication — decouples slashing from prover availability. * - * ## Six-case taxonomy in `getSlotActivity` + * ## Proposer taxonomy in `getSlotActivity` * - * For each slot, the sentinel assigns the proposer one of six statuses, ranked highest-confidence - * first: + * For each slot, the sentinel assigns the proposer one status, ranked highest-confidence first: * * - `checkpoint-mined` — a checkpoint covering this slot has landed on L1 * (fetched on demand via `archiver.getCheckpoint({ slot })`). @@ -93,18 +100,22 @@ function statusToCategory(status: ValidatorStatusInSlot): ValidatorStatusType { * - `checkpoint-unvalidated` — the local node observed a checkpoint proposal but could not * validate it (missing blocks/txs, timeouts). Treated as * proposer-fault for slashing. + * - `checkpoint-unverifiable` — the local node observed a checkpoint proposal and found its + * content valid, but could not check it against an authority + * outside itself. Recorded so the slot is not mistaken for one + * without a proposal, and charged to nobody. * - `checkpoint-missed` — block proposals seen on P2P but no checkpoint proposal at all. * - `blocks-missed` — no block proposals seen for this slot. * * Missing-attestor faults are recorded only in `checkpoint-mined` and `checkpoint-valid`, where - * the local node has positive evidence the checkpoint was canonical or valid. In the other four - * cases the proposer is at fault and no attestor penalty applies. + * the local node has positive evidence the checkpoint was canonical or valid. In the other cases + * no attestor penalty applies. * * ## Re-execution tracker * * `CheckpointReexecutionTracker` is populated by the validator client's checkpoint proposal * handler. Every early return in `validateCheckpointProposal` records an outcome - * (`valid` / `invalid` / `unvalidated`) keyed by slot. + * (`valid` / `invalid` / `unvalidated` / `unverifiable`) keyed by slot. * * ## Inactivity slashing * @@ -114,7 +125,7 @@ function statusToCategory(status: ValidatorStatusInSlot): ValidatorStatusType { * (read from `SentinelStore.epochMap`). Only validators meeting both conditions are emitted as * `WANT_TO_SLASH_EVENT` with `OffenseType.INACTIVITY`. The slot-level counters that feed this — * `missedProposals` and `missedAttestations` — include the four proposer-fault statuses plus - * `attestation-missed`. + * `attestation-missed`; `checkpoint-unverifiable` is a local-inability record and is in neither. * * ## Escape hatch * @@ -513,6 +524,7 @@ export class Sentinel extends (EventEmitter as new () => WatcherEmitter) impleme | 'checkpoint-valid' | 'checkpoint-invalid' | 'checkpoint-unvalidated' + | 'checkpoint-unverifiable' | 'checkpoint-missed' | 'blocks-missed'; if (checkpoint) { @@ -523,6 +535,10 @@ export class Sentinel extends (EventEmitter as new () => WatcherEmitter) impleme status = 'checkpoint-invalid'; } else if (reexecutionOutcome === 'unvalidated') { status = 'checkpoint-unvalidated'; + } else if (reexecutionOutcome === 'unverifiable') { + // A proposal this node saw but could not check against anything outside itself. Recording it stops the + // fallback below reading the slot as one the proposer never proposed in. + status = 'checkpoint-unverifiable'; } else { // No L1 checkpoint, no local re-execution outcome for this slot. Distinguish "proposer // sent block proposals but never made a checkpoint" from "proposer sent nothing". @@ -652,6 +668,9 @@ export class Sentinel extends (EventEmitter as new () => WatcherEmitter) impleme lastProposal: this.computeFromSlot(lastProposal?.slot), lastAttestation: this.computeFromSlot(lastAttestation?.slot), totalSlots: history.length, + // `checkpoint-unverifiable` is deliberately absent, and {@link UNASSESSABLE_STATUSES} keeps it out of the + // denominator too: it records this node's own inability to check a proposal it did see, so counting it + // either way would misreport the proposer. missedProposals: this.computeMissed(history, 'proposer', [ 'checkpoint-missed', 'blocks-missed', @@ -669,7 +688,9 @@ export class Sentinel extends (EventEmitter as new () => WatcherEmitter) impleme filter: ValidatorStatusInSlot[], ) { const relevantHistory = history.filter( - h => !computeOverCategory || statusToCategory(h.status) === computeOverCategory, + h => + !UNASSESSABLE_STATUSES.includes(h.status) && + (!computeOverCategory || statusToCategory(h.status) === computeOverCategory), ); const filteredHistory = relevantHistory.filter(h => filter.includes(h.status)); return { diff --git a/yarn-project/aztec-node/src/sentinel/store.test.ts b/yarn-project/aztec-node/src/sentinel/store.test.ts index cd8adf3d352e..8218237ac643 100644 --- a/yarn-project/aztec-node/src/sentinel/store.test.ts +++ b/yarn-project/aztec-node/src/sentinel/store.test.ts @@ -27,12 +27,13 @@ describe('sentinel-store', () => { it('inserts new validators with all statuses', async () => { const slot = SlotNumber(1); - const validators: `0x${string}`[] = times(8, () => EthAddress.random().toString()); + const validators: `0x${string}`[] = times(9, () => EthAddress.random().toString()); const statuses: ValidatorStatusInSlot[] = [ 'checkpoint-mined', 'checkpoint-valid', 'checkpoint-invalid', 'checkpoint-unvalidated', + 'checkpoint-unverifiable', 'checkpoint-missed', 'blocks-missed', 'attestation-sent', diff --git a/yarn-project/aztec-node/src/sentinel/store.ts b/yarn-project/aztec-node/src/sentinel/store.ts index 6f74c8c50a76..1e45adf19274 100644 --- a/yarn-project/aztec-node/src/sentinel/store.ts +++ b/yarn-project/aztec-node/src/sentinel/store.ts @@ -161,6 +161,8 @@ export class SentinelStore { return 7; case 'checkpoint-unvalidated': return 8; + case 'checkpoint-unverifiable': + return 9; default: { const _exhaustive: never = status; throw new Error(`Unknown status: ${status}`); @@ -186,6 +188,8 @@ export class SentinelStore { return 'checkpoint-invalid'; case 8: return 'checkpoint-unvalidated'; + case 9: + return 'checkpoint-unverifiable'; default: throw new Error(`Unknown status: ${status}`); } diff --git a/yarn-project/end-to-end/src/multi-node/block-production/proof_boundary.parallel.test.ts b/yarn-project/end-to-end/src/multi-node/block-production/proof_boundary.parallel.test.ts index 6588864fcbd0..c3f1b89385fe 100644 --- a/yarn-project/end-to-end/src/multi-node/block-production/proof_boundary.parallel.test.ts +++ b/yarn-project/end-to-end/src/multi-node/block-production/proof_boundary.parallel.test.ts @@ -50,6 +50,10 @@ describe('multi-node/block-production/proof_boundary', () => { test = await MultiNodeTestContext.setup({ ...MOCK_GOSSIP_MULTI_VALIDATOR_OPTS, ...MULTI_VALIDATOR_BLOCK_PRODUCTION_TIMING, + // The profile leaves this per-test. At the default of 2 the 24s slot derives only 3 block opportunities, + // under the Inbox catch-up floor, and the sequencer refuses the config at startup; 1 derives the 4 the + // floor wants. Gossip is mocked here, so the shorter propagation budget costs this suite nothing. + attestationPropagationTime: 1, initialValidators: validators, ...overrides, }); diff --git a/yarn-project/end-to-end/src/single-node/cross-chain/streaming_inbox.test.ts b/yarn-project/end-to-end/src/single-node/cross-chain/streaming_inbox.test.ts index e9f2162c62d3..a4d4b6486546 100644 --- a/yarn-project/end-to-end/src/single-node/cross-chain/streaming_inbox.test.ts +++ b/yarn-project/end-to-end/src/single-node/cross-chain/streaming_inbox.test.ts @@ -1,9 +1,11 @@ import type { Archiver } from '@aztec/archiver'; import type { AztecAddress } from '@aztec/aztec.js/addresses'; +import { NO_WAIT } from '@aztec/aztec.js/contracts'; import { generateClaimSecret } from '@aztec/aztec.js/ethereum'; import { Fr } from '@aztec/aztec.js/fields'; import type { Logger } from '@aztec/aztec.js/log'; import type { AztecNode } from '@aztec/aztec.js/node'; +import { waitForTx } from '@aztec/aztec.js/node'; import { TxExecutionResult } from '@aztec/aztec.js/tx'; import type { Wallet } from '@aztec/aztec.js/wallet'; import { BlockNumber, SlotNumber } from '@aztec/foundation/branded-types'; @@ -95,30 +97,86 @@ describe('single-node/cross-chain/streaming_inbox', () => { 0.1, ); + /** Leaves in a block's committed L1-to-L2 message tree; genesis holds none and an unknown block reports none. */ + const committedMessageCount = async (blockNumber: number): Promise => { + if (blockNumber <= 0) { + return 0n; + } + const data = await aztecNode.getBlockData(BlockNumber(blockNumber)); + return data && BigInt(data.header.state.l1ToL2MessageTree.nextAvailableLeafIndex); + }; + /** - * Finds the L2 block that inserted `msgHash` into the L1-to-L2 message tree by scanning forward from - * `fromBlock` for the first block whose committed tree resolves a membership witness. Under the streaming - * Inbox a message enters the tree at the first block the proposer builds after its archiver observed the - * message, which need not be the first block of a checkpoint. Returns the block-data (checkpoint number + index - * within checkpoint) of that block. + * Finds the L2 block that inserted `msgHash` into the L1-to-L2 message tree. Under the streaming Inbox a message + * enters the tree at the first block the proposer builds after its archiver observed it, which need not be the + * first block of a checkpoint. Returns the block-data (checkpoint number + index within checkpoint) of that block. + * + * The tree is append-only, so every block built after the insertion resolves a membership witness for the message + * just as the inserting one does: retaining membership is not inserting it, and scanning forward from a block + * number sampled by the caller reports where the search started whenever the message was already inserted by then. + * The block is instead located by the message's compact leaf index against the committed leaf count, which grows + * monotonically along the chain: the inserting block is the first whose count is past the index, found by bisecting + * the whole chain rather than trusting any sampled bound. + * + * The result is then confirmed at both states: the block's parent has no membership witness for the message and + * the block itself resolves one at the message's own compact index. A chain that moves under the search (the tip + * advancing, a prune) fails that confirmation, which is a genuine timing miss and is retried. */ - const findInsertingBlock = (msgHash: Fr, fromBlock: BlockNumber) => { - return retryUntil( - async () => { - const tip = await aztecNode.getBlockNumber(); - for (let n = fromBlock; n <= tip; n = BlockNumber(n + 1)) { - const witness = await aztecNode.getL1ToL2MessageMembershipWitness(n, msgHash); - if (witness !== undefined) { - const data = await aztecNode.getBlockData(n); - return { blockNumber: n, checkpointNumber: data!.checkpointNumber, index: data!.indexWithinCheckpoint }; - } + const findInsertingBlock = async (msgHash: Fr) => { + const attempts = 3; + for (let attempt = 0; attempt < attempts; attempt++) { + const { leafIndex } = await retryUntil( + async () => { + const index = await aztecNode.getL1ToL2MessageIndex(msgHash); + return index === undefined ? undefined : { leafIndex: index }; + }, + `node assigns a compact index to message ${msgHash.toString()}`, + 240, + 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 aztecNode.getBlockNumber(); + const count = await committedMessageCount(tip); + return count !== undefined && count > leafIndex ? { tip } : undefined; + }, + `a block committing message ${msgHash.toString()}`, + 240, + 0.5, + ); + + // Bisect for the first block past the index: genesis holds no messages, the tip holds this one. + let below = 0; + let holding = Number(tip); + while (holding - below > 1) { + const middle = below + Math.floor((holding - below) / 2); + const count = await committedMessageCount(middle); + if (count !== undefined && count > leafIndex) { + holding = middle; + } else { + below = middle; } - return undefined; - }, - `find block inserting message ${msgHash.toString()}`, - 240, - 0.5, - ); + } + + const blockNumber = BlockNumber(holding); + const data = await aztecNode.getBlockData(blockNumber); + const witness = await aztecNode.getL1ToL2MessageMembershipWitness(blockNumber, msgHash); + const parentWitness = + below === 0 ? undefined : await aztecNode.getL1ToL2MessageMembershipWitness(BlockNumber(below), msgHash); + if (data !== undefined && witness !== undefined && witness[0] === leafIndex && parentWitness === undefined) { + return { blockNumber, checkpointNumber: data.checkpointNumber, index: data.indexWithinCheckpoint }; + } + log.warn(`Block ${blockNumber} did not confirm as the one inserting ${msgHash.toString()}; searching again`, { + attempt, + leafIndex, + hasBlockData: data !== undefined, + resolvedIndex: witness?.[0], + parentHoldsMessage: parentWitness !== undefined, + }); + } + throw new Error(`Could not confirm which block inserted message ${msgHash.toString()} in ${attempts} attempts`); }; /** @@ -188,7 +246,7 @@ describe('single-node/cross-chain/streaming_inbox', () => { // The background feeder drives block production; findInsertingBlock polls the committed tree without // sending its own wallet txs (which would race the feeder on the nonce). - const found = await findInsertingBlock(msgHash, BlockNumber(blockAtSend + 1)); + const found = await findInsertingBlock(msgHash); log.warn(`Message ${msgHash.toString()} inserted at block ${found.blockNumber}`, { checkpointNumber: found.checkpointNumber, index: found.index, @@ -227,7 +285,6 @@ describe('single-node/cross-chain/streaming_inbox', () => { const maxDelaySeconds = BigInt(t.constants.ethereumSlotDuration) + 2n * BigInt(slotDuration); await withBackgroundFeeder(async () => { - const blockAtSend = await aztecNode.getBlockNumber(); const wallClockAtSend = Date.now(); const [, secretHash] = await generateClaimSecret(); const message = { recipient: testContract.address, content: Fr.random(), secretHash }; @@ -237,7 +294,7 @@ describe('single-node/cross-chain/streaming_inbox', () => { // The background feeder drives block production; findInsertingBlock polls the committed tree without // sending its own wallet txs (which would race the feeder on the nonce). - const inserting = await findInsertingBlock(msgHash, BlockNumber(blockAtSend + 1)); + const inserting = await findInsertingBlock(msgHash); const wallClockLatencyMs = Date.now() - wallClockAtSend; const insertingBlock = (await aztecNode.getBlock(inserting.blockNumber))!; const includingBlockTs = insertingBlock.header.globalVariables.timestamp; @@ -270,7 +327,6 @@ describe('single-node/cross-chain/streaming_inbox', () => { 0.5, ); - const blockAtSend = await aztecNode.getBlockNumber(); const [, secretHash] = await generateClaimSecret(); const message = { recipient: testContract.address, content: Fr.random(), secretHash }; const { msgHash } = await sendMessageToL2(message); @@ -279,7 +335,7 @@ describe('single-node/cross-chain/streaming_inbox', () => { // Do not feed txs; the sequencer builds empty checkpoints until the message ages past the lag, at which // point a zero-tx block consumes it. findInsertingBlock polls the committed tree without sending txs, so // the pool stays empty and the block that consumes the message carries only the bundle. - const inserting = await findInsertingBlock(msgHash, BlockNumber(blockAtSend + 1)); + const inserting = await findInsertingBlock(msgHash); const insertingBlock = (await aztecNode.getBlock(inserting.blockNumber, { includeTransactions: true }))!; log.warn(`Message ${msgHash.toString()} inserted at block ${inserting.blockNumber}`, { @@ -313,14 +369,13 @@ describe('single-node/cross-chain/streaming_inbox', () => { // double-spend revert. Mirrors cross_chain_public_message.test.ts. it('consumes a streaming-inserted message by compact index and rejects double-spend', async () => { const l1Account = t.ethAccount; - const blockAtSend = await aztecNode.getBlockNumber(); const [secret, secretHash] = await generateClaimSecret(); const message = { recipient: testContract.address, content: Fr.random(), secretHash }; const { msgHash, globalLeafIndex } = await sendMessageToL2(message); log.warn(`Sent message ${msgHash.toString()} with compact index ${globalLeafIndex}`); await waitForMessageReady(msgHash, 'public'); - const inserting = await findInsertingBlock(msgHash, BlockNumber(blockAtSend + 1)); + const inserting = await findInsertingBlock(msgHash); const { receipt: txReceipt } = await testContract.methods .consume_message_from_arbitrary_sender_public(message.content, secret, l1Account, globalLeafIndex.toBigInt()) @@ -345,53 +400,55 @@ describe('single-node/cross-chain/streaming_inbox', () => { // Test 5 (forced same-block consumption): a public tx consuming a message that the *same* block inserts must // succeed. The block builder appends the block's messages to its fork before executing txs, matching the prover // and the block-root circuit (which pins each tx's L1-to-L2 tree snapshot to the post-append root); if it did - // not, this tx would revert at proposal time and succeed at proving time, making the epoch unprovable. The node - // simulates public calls against the messages predicted for the next block, so a consume tx sent as soon as the - // node's archiver has observed the message passes simulation and lands in the pool before the inserting block - // is built. Observation is the only trigger: a message in a mined L1 block is consumable at once, without waiting - // for a descendant L1 block. The send is timed to the archiver's observation and retried with fresh messages when a - // block slips in between. + // not, this tx would revert at proposal time and succeed at proving time, making the epoch unprovable. + // + // A block freezes its L1-to-L2 message set when it is prepared, which happens before the message's L1 tx is even + // mined, so a consume tx sent while a block is already building can only join that block -- one too early, against + // a frozen set that does not hold the message, and it reverts. Waiting for the archiver alone cannot avoid this: + // it leaves which block the tx joins up to where the send lands in the block cadence. Block production is + // therefore held while the message is sent and observed and the consume tx is put in the pool, so that the first + // block prepared after the resume is the one that both inserts the message and executes the tx. it('consumes a message in the same block that inserts it', async () => { const l1Account = t.ethAccount; - const maxAttempts = 5; - let sameBlockReceipt: { blockNumber: BlockNumber; msgHash: Fr; globalLeafIndex: bigint } | undefined; + const sequencer = t.context.aztecNodeService.getSequencer()!; + const [secret, secretHash] = await generateClaimSecret(); + const message = { recipient: testContract.address, content: Fr.random(), secretHash }; - for (let attempt = 0; attempt < maxAttempts && sameBlockReceipt === undefined; attempt++) { - const [secret, secretHash] = await generateClaimSecret(); - const message = { recipient: testContract.address, content: Fr.random(), secretHash }; - const { msgHash, globalLeafIndex, txReceipt: l1Receipt } = await sendMessageToL2(message); - log.warn(`Attempt ${attempt}: sent message ${msgHash.toString()} in L1 block ${l1Receipt.blockNumber}`); - - // Do not drive L2 blocks while waiting: an extra block here only shifts the timing of the inserting block. - // The archiver polls L1 within an Ethereum slot; no later L1 block is needed for the message to be usable. - await waitForMessageObserved(msgHash); - const blockAtSend = await aztecNode.getBlockNumber(); - - const { receipt } = await testContract.methods - .consume_message_from_arbitrary_sender_public(message.content, secret, l1Account, globalLeafIndex.toBigInt()) - .send({ from: user1Address, wait: { dontThrowOnRevert: true } }); - const inserting = await findInsertingBlock(msgHash, BlockNumber(blockAtSend + 1)); - const consumeBlock = BlockNumber(Number(receipt.blockNumber)); - log.warn(`Consume tx for ${msgHash.toString()} landed in block ${consumeBlock}`, { - insertingBlock: inserting.blockNumber, - consumeBlock, - executionResult: receipt.executionResult, - }); + // Proven before the pause so the proof window cannot expire while nothing is being built. + await markAsProven(); + await sequencer.pause(); - if (consumeBlock !== inserting.blockNumber) { - // A block was built between eligibility and the tx's arrival; that is timing, not a bug. - log.warn(`Consume did not land in the inserting block; retrying with a fresh message`); - continue; - } + const { msgHash, globalLeafIndex } = await sendMessageToL2(message); + // The archiver polls L1 independently of block production, so it still observes the message while paused. + await waitForMessageObserved(msgHash); - // Same block reached: the consume must have succeeded against the block's own messages. - expect(receipt.executionResult).toBe(TxExecutionResult.SUCCESS); - sameBlockReceipt = { blockNumber: consumeBlock, msgHash, globalLeafIndex: globalLeafIndex.toBigInt() }; - } + // NO_WAIT: the tx has to reach the pool while production is held, so it cannot be awaited here -- nothing will + // mine it until the resume below. Simulation runs against the messages predicted for the next block, which the + // archiver has already observed, so the consume simulates against a tree that holds the message. + const { txHash } = await testContract.methods + .consume_message_from_arbitrary_sender_public(message.content, secret, l1Account, globalLeafIndex.toBigInt()) + .send({ from: user1Address, wait: NO_WAIT }); + log.warn(`Queued consume for ${msgHash.toString()} while production is held`, { txHash: txHash.toString() }); - if (sameBlockReceipt === undefined) { - throw new Error(`Could not produce a same-block consume in ${maxAttempts} attempts`); - } + // Resuming on a slot boundary gives the proposer its whole build frame for the block that carries both. + await t.monitor.waitUntilNextL2Slot(); + await sequencer.start(); + + const receipt = await waitForTx(aztecNode, txHash); + const consumeBlock = BlockNumber(Number(receipt.blockNumber)); + const inserting = await findInsertingBlock(msgHash); + log.warn(`Consume tx for ${msgHash.toString()} landed in block ${consumeBlock}`, { + insertingBlock: inserting.blockNumber, + consumeBlock, + executionResult: receipt.executionResult, + }); + + // The premise of the case: holding production must put both in the same block. Anything else is a real failure + // -- a consume one block late would succeed against an already-inserted message and prove nothing. + expect(consumeBlock).toBe(inserting.blockNumber); + expect(receipt.executionResult).toBe(TxExecutionResult.SUCCESS); + + const sameBlockReceipt = { blockNumber: consumeBlock, msgHash, globalLeafIndex: globalLeafIndex.toBigInt() }; const [resolvedIndex] = (await aztecNode.getL1ToL2MessageMembershipWitness( sameBlockReceipt.blockNumber, sameBlockReceipt.msgHash, diff --git a/yarn-project/ethereum/src/contracts/chain_state_override.ts b/yarn-project/ethereum/src/contracts/chain_state_override.ts index b17d841b0683..dfe03b814a79 100644 --- a/yarn-project/ethereum/src/contracts/chain_state_override.ts +++ b/yarn-project/ethereum/src/contracts/chain_state_override.ts @@ -1,29 +1,20 @@ import { toHex as toPaddedHex } from '@aztec/foundation/bigint-buffer'; -import type { CheckpointNumber, SlotNumber } from '@aztec/foundation/branded-types'; -import type { Buffer32 } from '@aztec/foundation/buffer'; +import type { CheckpointNumber } from '@aztec/foundation/branded-types'; import { merge } from '@aztec/foundation/collection'; import type { Fr } from '@aztec/foundation/curves/bn254'; import type { StateOverride } from 'viem'; -import { type FeeHeader, RollupContract } from './rollup.js'; +import { type FeeHeader, RollupContract, type TempCheckpointLogOverrideFields } from './rollup.js'; /** - * Override values for the pending checkpoint that the simulation should treat as already applied. - * Every field is optional at plan-building time so callers can populate them incrementally; whatever - * is present at translation time is forwarded to the partial `tempCheckpointLogs` helper so the - * load-bearing `slotNumber` can land even if other fields could not be derived locally. + * Override values for the pending checkpoint that the simulation should treat as already applied: the + * `tempCheckpointLogs` cell fields plus the archive root, which lives in its own mapping. Every field is optional at + * plan-building time so callers can populate them incrementally; whatever is present at translation time is forwarded + * to the partial `tempCheckpointLogs` helper so the required `slotNumber` can land even if other fields could not be + * derived locally. */ -export type PendingCheckpointOverrideState = { - archive?: Fr; - feeHeader?: FeeHeader; - headerHash?: Fr; - outHash?: Fr; - payloadDigest?: Buffer32; - slotNumber?: SlotNumber; - inboxMsgTotal?: bigint; - inboxConsumedBucket?: bigint; -}; +export type PendingCheckpointOverrideState = TempCheckpointLogOverrideFields & { archive?: Fr }; export type ChainTipsOverride = { pending?: CheckpointNumber; @@ -105,14 +96,7 @@ export class SimulationOverridesBuilder { * back to 0 and the contract treats the pending tip as belonging to epoch 0, triggering a phantom * prune that silently undoes the `pending` override. */ - public withPendingTempCheckpointLogFields(fields: { - headerHash?: Fr; - outHash?: Fr; - payloadDigest?: Buffer32; - slotNumber?: SlotNumber; - inboxMsgTotal?: bigint; - inboxConsumedBucket?: bigint; - }): this { + public withPendingTempCheckpointLogFields(fields: Omit): this { this.assertPendingCheckpointNumber(); this.pendingCheckpointState = { ...(this.pendingCheckpointState ?? {}), ...fields }; return this; @@ -172,17 +156,10 @@ export async function buildSimulationOverridesStateOverride( } if (plan.pendingCheckpointState) { + // `archive` rides along in the same state but lives in its own mapping, and the temp-log helper ignores it. rollupStateDiff.push( ...extractRollupStateDiff( - await rollup.makeTempCheckpointLogOverride(plan.chainTipsOverride!.pending!, { - headerHash: plan.pendingCheckpointState.headerHash, - outHash: plan.pendingCheckpointState.outHash, - payloadDigest: plan.pendingCheckpointState.payloadDigest, - slotNumber: plan.pendingCheckpointState.slotNumber, - inboxMsgTotal: plan.pendingCheckpointState.inboxMsgTotal, - inboxConsumedBucket: plan.pendingCheckpointState.inboxConsumedBucket, - feeHeader: plan.pendingCheckpointState.feeHeader, - }), + await rollup.makeTempCheckpointLogOverride(plan.chainTipsOverride!.pending!, plan.pendingCheckpointState), ), ); } diff --git a/yarn-project/ethereum/src/contracts/rollup.ts b/yarn-project/ethereum/src/contracts/rollup.ts index 97be4a3d1961..a1fb0b79b725 100644 --- a/yarn-project/ethereum/src/contracts/rollup.ts +++ b/yarn-project/ethereum/src/contracts/rollup.ts @@ -7,7 +7,6 @@ import type { ViemSignature } from '@aztec/foundation/eth-signature'; import { createLogger } from '@aztec/foundation/log'; import { makeBackoff, retry } from '@aztec/foundation/retry'; import { getErrorCause } from '@aztec/foundation/types'; -import { ErrorsAbi } from '@aztec/l1-artifacts/ErrorsAbi'; import { EscapeHatchAbi } from '@aztec/l1-artifacts/EscapeHatchAbi'; import { RollupAbi } from '@aztec/l1-artifacts/RollupAbi'; import { RollupStorage } from '@aztec/l1-artifacts/RollupStorage'; @@ -37,7 +36,7 @@ import type { L1ContractAddresses } from '../l1_contract_addresses.js'; import type { L1ReaderConfig } from '../l1_reader.js'; import type { L1TxRequest, L1TxUtils, ReadOnlyL1TxUtils } from '../l1_tx_utils/index.js'; import type { ViemClient } from '../types.js'; -import { formatViemError, mergeAbis } from '../utils.js'; +import { formatViemError } from '../utils.js'; import { GSEContract } from './gse.js'; import type { L1EventLog } from './log.js'; import { SlasherContract } from './slasher_contract.js'; @@ -925,7 +924,7 @@ export class RollupContract { }, { time: opts.time }, opts.stateOverrides ?? [], - mergeAbis([RollupAbi, ErrorsAbi]), + RollupAbi, ); return decodeFunctionResult({ abi: RollupAbi, functionName: 'validateCheckpointHeaderAndInbox', data: result }); } diff --git a/yarn-project/ethereum/src/l1_artifacts.test.ts b/yarn-project/ethereum/src/l1_artifacts.test.ts new file mode 100644 index 000000000000..26de03ce230b --- /dev/null +++ b/yarn-project/ethereum/src/l1_artifacts.test.ts @@ -0,0 +1,25 @@ +import { RollupArtifact } from './l1_artifacts.js'; + +describe('l1 artifacts', () => { + describe('RollupArtifact libraries', () => { + const { linkReferences, libraryCode } = RollupArtifact.libraries; + const linkedNames = Object.values(linkReferences).flatMap(refs => Object.keys(refs)); + + // The generic deployer deploys exactly the libraries in libraryCode and links exactly the names in + // linkReferences, so a name in one but not the other is either a deployment failure or a wasted deployment. + it('supplies code for every link reference the compiler emitted', () => { + expect(linkedNames.filter(name => !(name in libraryCode))).toEqual([]); + }); + + it('carries no library the rollup bytecode does not link against', () => { + expect(Object.keys(libraryCode).filter(name => !linkedNames.includes(name))).toEqual([]); + }); + + it('exports nonempty bytecode for every library', () => { + for (const [name, lib] of Object.entries(libraryCode)) { + expect(`${name}:${lib.contractBytecode.slice(0, 2)}`).toEqual(`${name}:0x`); + expect(lib.contractBytecode.length).toBeGreaterThan(2); + } + }); + }); +}); diff --git a/yarn-project/ethereum/src/l1_artifacts.ts b/yarn-project/ethereum/src/l1_artifacts.ts index a8273e56b8fd..e9aa7162e4d7 100644 --- a/yarn-project/ethereum/src/l1_artifacts.ts +++ b/yarn-project/ethereum/src/l1_artifacts.ts @@ -3,6 +3,8 @@ import { CoinIssuerBytecode, DateGatedRelayerAbi, DateGatedRelayerBytecode, + EpochProofExtLibAbi, + EpochProofExtLibBytecode, FeeAssetHandlerAbi, FeeAssetHandlerBytecode, FeeJuicePortalAbi, @@ -50,8 +52,6 @@ import { TestERC20Bytecode, ValidatorOperationsExtLibAbi, ValidatorOperationsExtLibBytecode, - ValidatorSelectionLibAbi, - ValidatorSelectionLibBytecode, } from '@aztec/l1-artifacts'; import type { Hex } from 'viem'; @@ -81,10 +81,10 @@ export const RollupArtifact = { libraries: { linkReferences: RollupLinkReferences, libraryCode: { - ValidatorSelectionLib: { - name: 'ValidatorSelectionLib', - contractAbi: ValidatorSelectionLibAbi, - contractBytecode: ValidatorSelectionLibBytecode as Hex, + EpochProofExtLib: { + name: 'EpochProofExtLib', + contractAbi: EpochProofExtLibAbi, + contractBytecode: EpochProofExtLibBytecode as Hex, }, RollupOperationsExtLib: { name: 'RollupOperationsExtLib', diff --git a/yarn-project/prover-client/src/orchestrator/block-proving-state.ts b/yarn-project/prover-client/src/orchestrator/block-proving-state.ts index a1b48de90321..65383de004b0 100644 --- a/yarn-project/prover-client/src/orchestrator/block-proving-state.ts +++ b/yarn-project/prover-client/src/orchestrator/block-proving-state.ts @@ -57,6 +57,7 @@ export class BlockProvingState { private builtArchive: AppendOnlyTreeSnapshot | undefined; private endState: StateReference | undefined; private endSpongeBlob: SpongeBlob | undefined; + private verified = false; private txs: TxProvingState[] = []; private error: string | undefined; @@ -216,6 +217,20 @@ export class BlockProvingState { return this.builtArchive; } + /** + * Records that this block's proof outputs were checked against the locally built header and archive and agreed. + * Field presence is not the same thing: the archive snapshot is captured before the fork is closed and before the + * comparison runs, so a caller that only looks for the pieces can see a block that has not been verified at all. + */ + public markVerified() { + this.verified = true; + } + + /** Whether {@link markVerified} has recorded a successful check for this block. */ + public isVerified() { + return this.verified; + } + public getStartSpongeBlob() { return this.startSpongeBlob; } diff --git a/yarn-project/prover-client/src/orchestrator/checkpoint-proving-state.ts b/yarn-project/prover-client/src/orchestrator/checkpoint-proving-state.ts index 7ebede67f5a3..6dbd40ef668e 100644 --- a/yarn-project/prover-client/src/orchestrator/checkpoint-proving-state.ts +++ b/yarn-project/prover-client/src/orchestrator/checkpoint-proving-state.ts @@ -236,6 +236,15 @@ export class CheckpointProvingState { : this.blockProofs.getChildren(rootLocation).map(c => c?.provingOutput); } + /** + * Whether every block of this checkpoint has been started and has passed its local verification. A checkpoint + * whose blocks are still being started has not, so a proof arriving before the caller reached the later blocks + * cannot be mistaken for a finished sub-tree. + */ + public allBlocksVerified() { + return this.blocks.length === this.totalNumBlocks && this.blocks.every(block => block?.isVerified()); + } + /** Sibling path of the archive tree captured before any block in this checkpoint landed. */ public getLastArchiveSiblingPath() { return this.lastArchiveSiblingPath; diff --git a/yarn-project/prover-client/src/orchestrator/checkpoint-sub-tree-orchestrator.test.ts b/yarn-project/prover-client/src/orchestrator/checkpoint-sub-tree-orchestrator.test.ts index e74aded283ae..7ccb2cb8200f 100644 --- a/yarn-project/prover-client/src/orchestrator/checkpoint-sub-tree-orchestrator.test.ts +++ b/yarn-project/prover-client/src/orchestrator/checkpoint-sub-tree-orchestrator.test.ts @@ -4,10 +4,17 @@ import { padArrayEnd, sum } from '@aztec/foundation/collection'; import { Fr } from '@aztec/foundation/curves/bn254'; import { EthAddress } from '@aztec/foundation/eth-address'; import { createLogger } from '@aztec/foundation/log'; +import { type PromiseWithResolvers, promiseWithResolvers } from '@aztec/foundation/promise'; +import { retryUntil } from '@aztec/foundation/retry'; +import { sleep } from '@aztec/foundation/sleep'; import { L1ToL2MessageSponge, ScopedL2ToL1Message, computeBlockOutHash } from '@aztec/stdlib/messaging'; +import type { CheckpointConstantData } from '@aztec/stdlib/rollup'; import { makeScopedL2ToL1Message } from '@aztec/stdlib/testing'; +import type { BlockHeader } from '@aztec/stdlib/tx'; import { TestContext, makeTestDeferredJobQueue } from '../mocks/test_context.js'; +import type { BlockProvingState } from './block-proving-state.js'; +import type { CheckpointProvingState } from './checkpoint-proving-state.js'; import { CheckpointSubTreeOrchestrator } from './checkpoint-sub-tree-orchestrator.js'; import { ChonkCache } from './chonk-cache.js'; @@ -311,4 +318,308 @@ describe('prover/orchestrator/checkpoint-sub-tree', () => { await subTree.stop(); } }); + + describe('local completion join', () => { + it('withholds the result while a block whose proofs are already in is still using its fork', async () => { + // An empty block's root rollup is enqueued from startNewBlock, so its proof — and the checkpoint's parity + // proof — can land while the caller still holds the block's processing fork and has not called + // setBlockCompleted. Resolving here would hand the sub-tree to a consumer that may tear it down underneath + // that caller. + const { constants, blocks, l1ToL2Messages, previousBlockHeader } = await context.makeCheckpoint(1); + const subTree = await startInspectableSubTree(1, constants, l1ToL2Messages, previousBlockHeader); + try { + const result = trackSettlement(subTree.getSubTreeResult()); + const { blockNumber, timestamp } = blocks[0].header.globalVariables; + + await subTree.startNewBlock(blockNumber, timestamp, 0, l1ToL2Messages); + await waitForAllProofs(subTree); + + await sleep(50); + expect(result.settled).toBe(false); + + await subTree.setBlockCompleted(blockNumber, blocks[0].header); + await expect(subTree.getSubTreeResult()).resolves.toBeDefined(); + } finally { + await subTree.stop(); + } + }); + + it('withholds the result while verification runs, even with header, archive and proofs all present', async () => { + // The narrower window inside setBlockCompleted: the archive snapshot is captured and the fork closed before + // verification runs, so every piece the resolution used to look for is present while the block is still + // unverified. Parity landing in that window must not resolve the sub-tree. + const { constants, blocks, l1ToL2Messages, previousBlockHeader } = await context.makeCheckpoint(1); + const subTree = await startInspectableSubTree(1, constants, l1ToL2Messages, previousBlockHeader); + try { + const result = trackSettlement(subTree.getSubTreeResult()); + const { blockNumber, timestamp } = blocks[0].header.globalVariables; + + subTree.holdVerification(); + await subTree.startNewBlock(blockNumber, timestamp, 0, l1ToL2Messages); + await waitForAllProofs(subTree); + + const completed = subTree.setBlockCompleted(blockNumber, blocks[0].header); + await sleep(50); + expect(result.settled).toBe(false); + + subTree.releaseVerification(); + await completed; + await expect(subTree.getSubTreeResult()).resolves.toBeDefined(); + } finally { + subTree.releaseVerification(); + await subTree.stop(); + } + }); + + it('withholds the result while the block fork is still closing', async () => { + // The narrowest window: the archive snapshot is captured from the fork, so it exists before `close()` + // returns. A block root proof landing during the close runs verification itself and would find header, + // proof and archive all present, so publishing the archive any earlier resolves the sub-tree with the + // block's own fork still open. + const closeGate = promiseWithResolvers(); + const proofGate = promiseWithResolvers(); + const { constants, blocks, l1ToL2Messages, previousBlockHeader } = await context.makeCheckpoint(1); + const subTree = await startInspectableSubTree(1, constants, l1ToL2Messages, previousBlockHeader, { + worldState: withHeldForkClose(context.worldState, closeGate.promise), + prover: withHeldBlockRootProof(context.prover, proofGate.promise), + }); + try { + const result = trackSettlement(subTree.getSubTreeResult()); + const { blockNumber, timestamp } = blocks[0].header.globalVariables; + + await subTree.startNewBlock(blockNumber, timestamp, 0, l1ToL2Messages); + + // Park completion inside the fork close, then let the block root proof land in that window. + const completed = subTree.setBlockCompleted(blockNumber, blocks[0].header); + await sleep(50); + proofGate.resolve(); + await waitForAllProofs(subTree); + + expect(result.settled).toBe(false); + + closeGate.resolve(); + await completed; + await expect(subTree.getSubTreeResult()).resolves.toBeDefined(); + } finally { + proofGate.resolve(); + closeGate.resolve(); + await subTree.stop(); + } + }); + + it('withholds the result until the last block of a multi-block checkpoint is verified', async () => { + // Same race with the block merge in play: the second block is message-only, so its root proof is enqueued + // from startNewBlock and the merge can complete while that block is still being driven. + const l1ToL2MessagesPerBlock = [[], [new Fr(2001)]]; + const { constants, blocks, l1ToL2Messages, previousBlockHeader } = + await context.makeCheckpointWithMessagesPerBlock(l1ToL2MessagesPerBlock, { numTxsPerBlock: [1, 0] }); + const subTree = await startInspectableSubTree(2, constants, l1ToL2Messages, previousBlockHeader); + try { + const result = trackSettlement(subTree.getSubTreeResult()); + + const first = blocks[0].header.globalVariables; + await subTree.startNewBlock(first.blockNumber, first.timestamp, blocks[0].txs.length, []); + await subTree.addTxs(blocks[0].txs); + await subTree.setBlockCompleted(first.blockNumber, blocks[0].header); + + const second = blocks[1].header.globalVariables; + await subTree.startNewBlock(second.blockNumber, second.timestamp, 0, l1ToL2MessagesPerBlock[1]); + await waitForAllProofs(subTree); + + await sleep(50); + expect(result.settled).toBe(false); + + await subTree.setBlockCompleted(second.blockNumber, blocks[1].header); + await expect(subTree.getSubTreeResult()).resolves.toBeDefined(); + } finally { + await subTree.stop(); + } + }); + + it('rejects and never resolves when the built archive disagrees with the synced one', async () => { + // A verification mismatch reports through provingState.reject rather than by throwing, so the failure has to + // win over the proofs that are already in hand. + const { constants, blocks, l1ToL2Messages, previousBlockHeader } = await context.makeCheckpoint(1); + const subTree = await startInspectableSubTree(1, constants, l1ToL2Messages, previousBlockHeader, { + worldState: withStaleArchiveSnapshots(context.worldState), + }); + try { + const result = trackSettlement(subTree.getSubTreeResult()); + const { blockNumber, timestamp } = blocks[0].header.globalVariables; + + await subTree.startNewBlock(blockNumber, timestamp, 0, l1ToL2Messages); + await waitForAllProofs(subTree); + await subTree.setBlockCompleted(blockNumber, blocks[0].header); + + await expect(subTree.getSubTreeResult()).rejects.toThrow(/Archive tree mismatch/); + expect(result.resolved).toBe(false); + } finally { + await subTree.stop(); + } + }); + + it('does not publish a result when the caller supplies a header the block does not match', async () => { + const { constants, blocks, l1ToL2Messages, previousBlockHeader } = await context.makeCheckpoint(1); + const subTree = await startInspectableSubTree(1, constants, l1ToL2Messages, previousBlockHeader); + try { + const result = trackSettlement(subTree.getSubTreeResult()); + const { blockNumber, timestamp } = blocks[0].header.globalVariables; + + await subTree.startNewBlock(blockNumber, timestamp, 0, l1ToL2Messages); + await waitForAllProofs(subTree); + + // Any header that is not this block's: completion must refuse it rather than verify against it. + await expect(subTree.setBlockCompleted(blockNumber, previousBlockHeader)).rejects.toThrow( + /Block header mismatch/, + ); + + await sleep(50); + expect(result.settled).toBe(false); + } finally { + await subTree.stop(); + } + }); + }); + + /** Starts a sub-tree that exposes its checkpoint state and can park local verification on demand. */ + function startInspectableSubTree( + numBlocks: number, + constants: CheckpointConstantData, + l1ToL2Messages: Fr[], + previousBlockHeader: BlockHeader, + { + worldState = context.worldState, + prover = context.prover, + }: { worldState?: typeof context.worldState; prover?: typeof context.prover } = {}, + ): Promise { + return InspectableSubTree.startInspectable( + worldState, + prover, + EthAddress.ZERO, + chonkCache, + EpochNumber(1), + false, + makeTestDeferredJobQueue(), + constants, + l1ToL2Messages, + Fr.ZERO, + numBlocks, + previousBlockHeader, + ); + } }); + +/** + * A sub-tree that lets a test observe the checkpoint's proofs and hold local verification open, so proofs can be + * made to land while a block is deliberately left unfinished. + */ +class InspectableSubTree extends CheckpointSubTreeOrchestrator { + private verificationGate: PromiseWithResolvers | undefined; + + public static async startInspectable( + ...args: Parameters + ): Promise { + const subTree = await InspectableSubTree.start(...args); + if (!(subTree instanceof InspectableSubTree)) { + throw new Error('The start factory did not construct the subclass it was called on.'); + } + return subTree; + } + + public getCheckpointProvingState(): CheckpointProvingState { + if (!this.provingState) { + throw new Error('Sub-tree has no checkpoint state.'); + } + return this.provingState; + } + + /** Parks every subsequent local verification until {@link releaseVerification}. */ + public holdVerification(): void { + this.verificationGate ??= promiseWithResolvers(); + } + + public releaseVerification(): void { + this.verificationGate?.resolve(); + } + + protected override async verifyBuiltBlockAgainstSyncedState(provingState: BlockProvingState) { + await this.verificationGate?.promise; + return await super.verifyBuiltBlockAgainstSyncedState(provingState); + } +} + +/** Waits until every block proof of the checkpoint and its parity proof are in hand. */ +async function waitForAllProofs(subTree: InspectableSubTree): Promise { + await retryUntil( + () => { + const state = subTree.getCheckpointProvingState(); + return state.getSubTreeOutputProofs().every(proof => !!proof) && !!state.getInboxParityProof(); + }, + 'checkpoint proofs', + 30, + 0.05, + ); +} + +/** Records how a promise settled without awaiting it, so a test can assert it stayed pending. */ +function trackSettlement(promise: Promise) { + const status = { settled: false, resolved: false }; + void promise.then( + () => { + status.settled = true; + status.resolved = true; + }, + () => { + status.settled = true; + }, + ); + return status; +} + +/** + * A world state whose archive snapshots are taken one block early, so the block's built archive cannot agree with + * the synced one and verification must reject the checkpoint. + */ +function withStaleArchiveSnapshots(worldState: T): T { + return replacingMethod(worldState, 'getSnapshot', getSnapshot => blockNumber => getSnapshot(Number(blockNumber) - 1)); +} + +/** A world state whose block forks park in `close()` until `held` settles. */ +function withHeldForkClose(worldState: T, held: Promise): T { + return replacingMethod(worldState, 'fork', fork => async (...args) => { + const db = await fork(...args); + if (typeof db !== 'object' || db === null) { + throw new Error('Expected a fork.'); + } + return replacingMethod(db, 'close', close => async () => { + await held; + return await close(); + }); + }); +} + +/** A prover that withholds an empty block's root proof until `held` settles. */ +function withHeldBlockRootProof(prover: T, held: Promise): T { + return replacingMethod(prover, 'getBlockRootNoTxsRollupProof', getProof => async (...args) => { + await held; + return await getProof(...args); + }); +} + +/** Forwards every member to `target` except the named method, which `replace` rebuilds from the original. */ +function replacingMethod( + target: T, + name: string, + replace: (original: (...args: unknown[]) => unknown) => (...args: unknown[]) => unknown, +): T { + return new Proxy(target, { + get(_target, prop) { + const value = Reflect.get(target, prop); + if (typeof value !== 'function') { + return value; + } + const method = value.bind(target); + return prop === name ? replace(method) : method; + }, + }); +} diff --git a/yarn-project/prover-client/src/orchestrator/checkpoint-sub-tree-orchestrator.ts b/yarn-project/prover-client/src/orchestrator/checkpoint-sub-tree-orchestrator.ts index 6c12bcfae800..73f84607e777 100644 --- a/yarn-project/prover-client/src/orchestrator/checkpoint-sub-tree-orchestrator.ts +++ b/yarn-project/prover-client/src/orchestrator/checkpoint-sub-tree-orchestrator.ts @@ -204,7 +204,7 @@ export class CheckpointSubTreeOrchestrator extends ProvingScheduler { telemetryClient: TelemetryClient = getTelemetryClient(), bindings?: LoggerBindings, ): Promise { - const subTree = new CheckpointSubTreeOrchestrator( + const subTree = new this( dbProvider, prover, proverId, @@ -471,16 +471,22 @@ export class CheckpointSubTreeOrchestrator extends ProvingScheduler { this.dbs.delete(provingState.blockNumber); // Update the archive tree, capture the snapshot, and close the fork deterministically. + let builtArchive: AppendOnlyTreeSnapshot; try { this.logger.verbose( `Updating archive tree with block ${provingState.blockNumber} header ${(await header.hash()).toString()}`, ); await db.updateArchive(header); - provingState.setBuiltArchive(await getTreeSnapshot(MerkleTreeId.ARCHIVE, db)); + builtArchive = await getTreeSnapshot(MerkleTreeId.ARCHIVE, db); } finally { await db.close(); } + // Publish the archive only once the fork is closed. Verification also runs from the block root proof callback, + // so an archive published before the close lets a proof arriving during it find every piece present, verify, + // and resolve the sub-tree while this block's fork is still open. + provingState.setBuiltArchive(builtArchive); + await this.verifyBuiltBlockAgainstSyncedState(provingState); return header; @@ -763,11 +769,10 @@ export class CheckpointSubTreeOrchestrator extends ProvingScheduler { // Verification is called from both here and setBlockCompleted. Whichever runs last // will be the first to see all three pieces (header, proof output, archive) and run the checks. + // It enqueues the sub-tree resolution itself, so a single-block checkpoint needs nothing further. await this.verifyBuiltBlockAgainstSyncedState(provingState); - if (checkpointProvingState.totalNumBlocks === 1) { - this.checkAndEnqueueSubTreeResolution(checkpointProvingState); - } else { + if (checkpointProvingState.totalNumBlocks > 1) { this.checkAndEnqueueNextBlockMergeRollup(checkpointProvingState, leafLocation); } }, @@ -889,6 +894,13 @@ export class CheckpointSubTreeOrchestrator extends ProvingScheduler { // Parity not proven yet — retried when the inbox parity proof lands. return; } + // Proofs being present is not the same as the checkpoint being finished locally. The archive snapshot is + // captured before the block's fork is closed and before its outputs are compared with it, and parity can land + // during those awaits and reach here directly, so a proof-only gate would hand the sub-tree off — and let its + // caller tear it down — while a block is still being finalized. Retried as each block finishes verifying. + if (!provingState.allBlocksVerified()) { + return; + } this.subTreeResult.resolve({ blockProofOutputs: nonEmpty, inboxParityProof, @@ -982,6 +994,12 @@ export class CheckpointSubTreeOrchestrator extends ProvingScheduler { provingState.reject(`New archive mismatch.`); return; } + + // Only now is this block's proof output known to agree with what was built locally. Recording it here (rather + // than inferring it from the pieces being present) is what lets the sub-tree resolution wait for it, and it is + // the last piece for a block whose proofs arrived before its fork was closed. + provingState.markVerified(); + this.checkAndEnqueueSubTreeResolution(provingState.parentCheckpoint); } private getDbForBlock(blockNumber: BlockNumber): MerkleTreeWriteOperations { diff --git a/yarn-project/prover-client/src/test/epoch_proving_sim.test.ts b/yarn-project/prover-client/src/test/epoch_proving_sim.test.ts index 6dc5dfadcf54..6dd2f2b2c9c5 100644 --- a/yarn-project/prover-client/src/test/epoch_proving_sim.test.ts +++ b/yarn-project/prover-client/src/test/epoch_proving_sim.test.ts @@ -46,6 +46,9 @@ type Worker = { end: number; }; +// A finished job together with the window the simulated worker held it for. +type Completion = Job & { start: number; end: number }; + // New type definitions for flexible test configuration // Each public tx group: [count, avmDuration in ms] type PublicTxGroup = [count: number, avmDurationMs: number]; @@ -62,7 +65,8 @@ type TestConfig = { // State tracking for dependency resolution type SimState = { - // Parity tracking: one InboxParity proof per checkpoint, gating the first block root. + // Parity tracking: one InboxParity proof per checkpoint. It is proven from the start of the checkpoint and feeds + // the checkpoint root rollup, so it runs alongside block production rather than gating any block root. inboxParityComplete: Map; // Public tx dependency tracking (aggregate per block) @@ -78,11 +82,17 @@ type SimState = { txTreeLeafCount: Map; // total leaves (txs) per block txTreeNextLeafIndex: Map; // next leaf index to assign per block + // Per-block guard so a block root is queued exactly once, whichever dependency completes last + blockRootEnqueued: Map; + // Block merge tree tracking per checkpoint // Key format: "checkpoint-level" blockTreeNodes: Map>; blockRootComplete: Map; + // Per-checkpoint guard so the checkpoint root is queued exactly once, whichever of its two dependencies lands last + checkpointRootEnqueued: Map; + // Checkpoint merge tree tracking checkpointTreeNodes: Map>; // level -> index -> completed checkpointRootComplete: Map; @@ -93,7 +103,6 @@ type SimState = { // Block configuration for determining proof types blockConfigs: Map; blocksPerCheckpoint: Map; - isFirstBlock: Map; totalCheckpoints: number; }; @@ -241,14 +250,15 @@ function initializeState(checkpoints: Checkpoint[]): SimState { txTreeComplete: new Map(), txTreeLeafCount: new Map(), txTreeNextLeafIndex: new Map(), + blockRootEnqueued: new Map(), blockTreeNodes: new Map(), blockRootComplete: new Map(), + checkpointRootEnqueued: new Map(), checkpointTreeNodes: new Map(), checkpointRootComplete: new Map(), rootRollupComplete: false, blockConfigs: new Map(), blocksPerCheckpoint: new Map(), - isFirstBlock: new Map(), totalCheckpoints: checkpoints.length, }; @@ -258,6 +268,7 @@ function initializeState(checkpoints: Checkpoint[]): SimState { state.blocksPerCheckpoint.set(cp, blocks.length); state.inboxParityComplete.set(cp, false); state.blockRootComplete.set(cp, false); + state.checkpointRootEnqueued.set(cp, false); state.checkpointRootComplete.set(cp, false); for (let b = 1; b <= blocks.length; b++) { @@ -265,13 +276,8 @@ function initializeState(checkpoints: Checkpoint[]): SimState { const block = blocks[b - 1]; const { totalTxs } = getBlockTxCount(block); - // Empty blocks are only allowed as the first block in a checkpoint (matches orchestrator constraint) - if (totalTxs === 0 && b !== 1) { - throw new Error(`Cannot create a block with 0 txs, unless it's the first block. Checkpoint ${cp}, Block ${b}`); - } - state.blockConfigs.set(key, block); - state.isFirstBlock.set(key, b === 1); + state.blockRootEnqueued.set(key, false); state.vmComplete.set(key, 0); state.chonkComplete.set(key, 0); state.publicBaseEnqueued.set(key, 0); @@ -284,7 +290,7 @@ function initializeState(checkpoints: Checkpoint[]): SimState { return state; } -function fillQueue(queues: Queues, checkpoints: Checkpoint[]): void { +function fillQueue(queues: Queues, checkpoints: Checkpoint[], state: SimState): void { for (let cp = 1; cp <= checkpoints.length; cp++) { const blocks = checkpoints[cp - 1]; @@ -296,6 +302,12 @@ function fillQueue(queues: Queues, checkpoints: Checkpoint[]): void { const privateTxs = block[0]; const publicGroups = block.slice(1) as PublicTxGroup[]; + // A block with no txs has no base or merge proof whose completion would queue its block root, so it is queued + // as soon as the block starts. This holds wherever the block sits in the checkpoint. + if (getBlockTxCount(block).totalTxs === 0) { + tryEnqueueBlockRoot(cp, b, state, queues); + } + // Add private tx base rollups if (privateTxs > 0) { queues[ProvingRequestType.PRIVATE_TX_BASE_ROLLUP].push( @@ -323,7 +335,7 @@ function enqueueDependentJobs(job: Job, state: SimState, queues: Queues, checkpo switch (type) { case ProvingRequestType.INBOX_PARITY: { state.inboxParityComplete.set(checkpoint, true); - tryEnqueueBlockRoot(checkpoint, 1, state, queues); + tryEnqueueCheckpointRoot(checkpoint, state, queues); break; } @@ -488,20 +500,19 @@ function handleTxMergeComplete( function tryEnqueueBlockRoot(checkpoint: number, blk: number, state: SimState, queue: Queues): void { const key = blockKey(checkpoint, blk); - const blockConfig = state.blockConfigs.get(key)!; - const { totalTxs } = getBlockTxCount(blockConfig); - const isFirst = state.isFirstBlock.get(key)!; - // Need tx tree complete - if (!state.txTreeComplete.get(key)) { + if (state.blockRootEnqueued.get(key)) { return; } - // First block also needs the checkpoint's InboxParity proof - if (isFirst && !state.inboxParityComplete.get(checkpoint)) { + // A block root depends only on its own tx tree. The checkpoint's parity proof is not a dependency. + if (!state.txTreeComplete.get(key)) { return; } + const { totalTxs } = getBlockTxCount(state.blockConfigs.get(key)!); + state.blockRootEnqueued.set(key, true); + const blockRootType = getBlockRootType(totalTxs); queue[blockRootType].push(createJob(checkpoint, blk, blockRootType)); } @@ -569,11 +580,19 @@ function handleBlockMergeComplete( tryEnqueueBlockMerge(checkpoint, level, index, state, queues); } +// The checkpoint root joins the two independent branches of a checkpoint: its reduced block tree and its single +// InboxParity proof. Called whenever either lands, so whichever is last drives the enqueue. function tryEnqueueCheckpointRoot(checkpoint: number, state: SimState, queues: Queues): void { - if (!state.blockRootComplete.get(checkpoint)) { + if (state.checkpointRootEnqueued.get(checkpoint)) { + return; + } + + if (!state.blockRootComplete.get(checkpoint) || !state.inboxParityComplete.get(checkpoint)) { return; } + state.checkpointRootEnqueued.set(checkpoint, true); + const numBlocks = state.blocksPerCheckpoint.get(checkpoint)!; const checkpointRootType = getCheckpointRootType(numBlocks); queues[checkpointRootType].push(createJob(checkpoint, 0, checkpointRootType)); @@ -685,6 +704,7 @@ type SimulationResult = { workerUtilization: number; jobBreakdown: Record; workerActivity: WorkerActivityPoint[]; + completions: Completion[]; }; }; @@ -693,9 +713,9 @@ function runSimulationWithConfig(config: TestConfig): SimulationResult { const state = initializeState(checkpoints); const queues: Queues = times(Object.values(ProvingRequestType).length, () => []) as any; const workerPool: Worker[] = []; - const completed: Job[] = []; + const completed: Completion[] = []; - fillQueue(queues, checkpoints); + fillQueue(queues, checkpoints, state); let time = 0; let totalWorkerTime = 0; @@ -731,7 +751,7 @@ function runSimulationWithConfig(config: TestConfig): SimulationResult { // Process completions and enqueue dependent jobs for (const worker of justCompleted) { - completed.push(worker.job); + completed.push({ ...worker.job, start: worker.start, end: worker.end }); enqueueDependentJobs(worker.job, state, queues, checkpoints.length); } @@ -780,6 +800,7 @@ function runSimulationWithConfig(config: TestConfig): SimulationResult { workerUtilization, jobBreakdown, workerActivity, + completions: completed, }, }; } @@ -880,3 +901,114 @@ describe('epoch proving simulation', () => { expect(result.results.totalTimeMs).toBeLessThan(30 * 72 * 1000); // targeting root rollup in less than 30 slots }); }); + +const BLOCK_ROOT_TYPES = [ + ProvingRequestType.BLOCK_ROOT_ROLLUP, + ProvingRequestType.BLOCK_ROOT_SINGLE_TX_ROLLUP, + ProvingRequestType.BLOCK_ROOT_NO_TXS_ROLLUP, +]; + +const CHECKPOINT_ROOT_TYPES = [ + ProvingRequestType.CHECKPOINT_ROOT_ROLLUP, + ProvingRequestType.CHECKPOINT_ROOT_SINGLE_BLOCK_ROLLUP, +]; + +function simulate(name: string, checkpoints: Checkpoint[], workers: number): SimulationResult { + return runSimulationWithConfig({ name, workers, checkpoints }); +} + +function completionsOf(result: SimulationResult, types: ProvingRequestType[]): Completion[] { + return result.results.completions.filter(c => types.includes(c.type)); +} + +function soleCompletion(result: SimulationResult, types: ProvingRequestType[]): Completion { + const found = completionsOf(result, types); + expect(found).toHaveLength(1); + return found[0]; +} + +// The moment a checkpoint's block tree is fully reduced: the last block root or block merge to finish for it. +function blockTreeReadyAt(result: SimulationResult, checkpoint: number): number { + const ends = completionsOf(result, [...BLOCK_ROOT_TYPES, ProvingRequestType.BLOCK_MERGE_ROLLUP]) + .filter(c => c.checkpoint === checkpoint) + .map(c => c.end); + return Math.max(...ends); +} + +describe('checkpoint dependency model', () => { + it('proves an empty block root without waiting for inbox parity', () => { + const result = simulate('no transactions', [[[0]]], 16); + + const blockRoot = soleCompletion(result, BLOCK_ROOT_TYPES); + const parity = soleCompletion(result, [ProvingRequestType.INBOX_PARITY]); + + expect(blockRoot.type).toBe(ProvingRequestType.BLOCK_ROOT_NO_TXS_ROLLUP); + expect(blockRoot.start).toBe(0); + expect(parity.start).toBe(0); + expect(blockRoot.end).toBeLessThan(parity.end); + }); + + it('holds the checkpoint root until parity lands when parity finishes last', () => { + const result = simulate('parity last', [[[0], [0]]], 1); + + const parity = soleCompletion(result, [ProvingRequestType.INBOX_PARITY]); + const checkpointRoot = soleCompletion(result, CHECKPOINT_ROOT_TYPES); + + expect(checkpointRoot.type).toBe(ProvingRequestType.CHECKPOINT_ROOT_ROLLUP); + expect(blockTreeReadyAt(result, 1)).toBeLessThan(parity.end); + expect(checkpointRoot.start).toBe(parity.end); + }); + + it('holds the checkpoint root until the block tree lands when parity finishes first', () => { + const result = simulate('parity first', [[[2]]], 16); + + const parity = soleCompletion(result, [ProvingRequestType.INBOX_PARITY]); + const checkpointRoot = soleCompletion(result, CHECKPOINT_ROOT_TYPES); + const blockTreeReady = blockTreeReadyAt(result, 1); + + expect(parity.end).toBeLessThan(blockTreeReady); + expect(checkpointRoot.start).toBe(blockTreeReady); + }); + + it('routes a single-transaction block through the single-tx block root', () => { + const result = simulate('one transaction', [[[1]]], 16); + + const blockRoot = soleCompletion(result, BLOCK_ROOT_TYPES); + const base = soleCompletion(result, [ProvingRequestType.PRIVATE_TX_BASE_ROLLUP]); + + expect(blockRoot.type).toBe(ProvingRequestType.BLOCK_ROOT_SINGLE_TX_ROLLUP); + expect(completionsOf(result, [ProvingRequestType.TX_MERGE_ROLLUP])).toEqual([]); + expect(blockRoot.start).toBeGreaterThanOrEqual(base.end); + }); + + it('reduces a multi-transaction block before its block root', () => { + const result = simulate('multiple transactions', [[[4]]], 16); + + const blockRoot = soleCompletion(result, BLOCK_ROOT_TYPES); + const bases = completionsOf(result, [ProvingRequestType.PRIVATE_TX_BASE_ROLLUP]); + + expect(blockRoot.type).toBe(ProvingRequestType.BLOCK_ROOT_ROLLUP); + expect(bases).toHaveLength(4); + expect(blockRoot.start).toBeGreaterThanOrEqual(Math.max(...bases.map(b => b.end))); + }); + + it('proves zero-transaction blocks at any position in a checkpoint', () => { + const result = simulate('mixed empty blocks', [[[0], [2], [0], [1]]], 16); + + const blockRoots = completionsOf(result, BLOCK_ROOT_TYPES).sort((a, b) => a.block - b.block); + const parity = soleCompletion(result, [ProvingRequestType.INBOX_PARITY]); + const checkpointRoot = soleCompletion(result, CHECKPOINT_ROOT_TYPES); + + expect(blockRoots.map(r => [r.block, r.type])).toEqual([ + [1, ProvingRequestType.BLOCK_ROOT_NO_TXS_ROLLUP], + [2, ProvingRequestType.BLOCK_ROOT_ROLLUP], + [3, ProvingRequestType.BLOCK_ROOT_NO_TXS_ROLLUP], + [4, ProvingRequestType.BLOCK_ROOT_SINGLE_TX_ROLLUP], + ]); + // Neither empty block waits on parity or on the blocks around it. + expect(blockRoots.filter(r => r.type === ProvingRequestType.BLOCK_ROOT_NO_TXS_ROLLUP).map(r => r.start)).toEqual([ + 0, 0, + ]); + expect(checkpointRoot.start).toBe(Math.max(parity.end, blockTreeReadyAt(result, 1))); + }); +}); diff --git a/yarn-project/prover-client/src/test/regenerate_rollup_sample_inputs.test.ts b/yarn-project/prover-client/src/test/regenerate_rollup_sample_inputs.test.ts index 79059cc680af..d864d6adbbf9 100644 --- a/yarn-project/prover-client/src/test/regenerate_rollup_sample_inputs.test.ts +++ b/yarn-project/prover-client/src/test/regenerate_rollup_sample_inputs.test.ts @@ -37,13 +37,23 @@ import { type CheckpointTopTreeData, TopTreeOrchestrator } from '../orchestrator // before the (two-input) block root, whereas one- or two-tx blocks feed the block root directly, so a // dedicated three-tx scenario is what regenerates the tx-merge sample. The samples for the variants // that thread a start sponge from a previous block are taken from mid-checkpoint blocks, so those -// scenarios need a per-block message distribution. Every scenario also produces the root rollup. +// scenarios need a per-block message distribution, and they declare which block they mean: blocks are +// proven concurrently, so the order the inputs were captured in does not identify them. Every scenario +// also produces the root rollup. const describeOrSkip = isGenerateTestDataEnabled() ? describe : describe.skip; describeOrSkip('prover/regenerate-rollup-sample-inputs', () => { let context: TestContext; let log: Logger; + /** + * How many L1-to-L2 messages a block inherits from earlier blocks of its checkpoint (its start sponge) and how + * many its own bundle inserts. A scenario that runs a block-root circuit once per block identifies the run it + * means by this shape, which is a property of the block itself, rather than by the order the inputs happened to + * be captured in. + */ + type MessageShape = { inherited: number; bundle: number }; + interface Scenario { numCheckpoints: number; numBlocksPerCheckpoint: number; @@ -56,6 +66,8 @@ describeOrSkip('prover/regenerate-rollup-sample-inputs', () => { l1ToL2MessagesPerBlock?: Fr[][]; /** Circuits whose sample inputs this scenario is responsible for regenerating. */ dump: CircuitName[]; + /** The block whose inputs each dumped block-root sample must be taken from. */ + sampleFrom?: Partial>; } // `makeCheckpoint` puts the scenario's whole message list into the first block, so the most a @@ -76,6 +88,9 @@ describeOrSkip('prover/regenerate-rollup-sample-inputs', () => { numTxsPerBlock: 1, numL1ToL2Messages: withMessages, dump: ['rollup-block-root-single-tx', 'rollup-block-merge', 'rollup-checkpoint-root'], + // All the messages go to the first block, so the single-tx sample is the block that carries the bundle; the + // two blocks after it insert nothing and only inherit the sponge. + sampleFrom: { 'rollup-block-root-single-tx': { inherited: 0, bundle: withMessages } }, }, // Messages split across both blocks so the block-root sample is taken from a mid-checkpoint block with a // non-empty bundle, exercising the per-block sponge continuity asserts in the circuit. @@ -86,6 +101,9 @@ describeOrSkip('prover/regenerate-rollup-sample-inputs', () => { numL1ToL2Messages: 0, // Overridden by l1ToL2MessagesPerBlock. l1ToL2MessagesPerBlock: [times(2, i => new Fr(0xb00 + i)), times(3, i => new Fr(0xc00 + i))], dump: ['rollup-block-root'], + // The second block: it inherits the two messages the first block inserted and inserts three of its own, so + // its start sponge is a continuation rather than the initial empty one. + sampleFrom: { 'rollup-block-root': { inherited: 2, bundle: 3 } }, }, // Three txs in a block force a tx-merge to pair the base proofs down to the two the block root // takes; one- or two-tx blocks feed the block root directly and never exercise tx-merge. @@ -105,6 +123,7 @@ describeOrSkip('prover/regenerate-rollup-sample-inputs', () => { numL1ToL2Messages: 0, // Overridden by l1ToL2MessagesPerBlock. l1ToL2MessagesPerBlock: [times(2, i => new Fr(0x900 + i)), times(3, i => new Fr(0xa00 + i))], dump: ['rollup-block-root-no-txs'], + sampleFrom: { 'rollup-block-root-no-txs': { inherited: 2, bundle: 3 } }, }, // The checkpoint-merge only appears with three checkpoints. Independently-built checkpoints do // not carry the inbox message state forward, so this scenario runs with no L1-to-L2 messages and @@ -118,6 +137,51 @@ describeOrSkip('prover/regenerate-rollup-sample-inputs', () => { }, ]; + /** The message shape of a captured input, or undefined for circuits that take no message bundle. */ + const messageShapeOf = (captured: unknown): MessageShape | undefined => { + const { inputs } = (captured ?? {}) as { + inputs?: { message_bundle?: { num_msgs?: string }; start_msg_sponge?: { num_absorbed?: string } }; + }; + const bundle = inputs?.message_bundle?.num_msgs; + const inherited = inputs?.start_msg_sponge?.num_absorbed; + return bundle === undefined || inherited === undefined + ? undefined + : { inherited: Number(BigInt(inherited)), bundle: Number(BigInt(bundle)) }; + }; + + /** + * Picks the one captured input a scenario means to commit for `circuitName`. A circuit the scenario runs once has + * a single candidate; one it runs per block is identified by the message shape the scenario declared. Nothing + * captured, several runs with no declared shape, or a declared shape matching no run or more than one all throw: + * committing whichever input happened to be captured first would silently pin the wrong block. + */ + const selectSample = (circuitName: CircuitName, wanted: MessageShape | undefined): unknown => { + const captured = getTestData(circuitName) ?? []; + const shapes = () => captured.map(entry => JSON.stringify(messageShapeOf(entry) ?? 'no bundle')).join(', '); + if (captured.length === 0) { + throw new Error(`No test data captured for ${circuitName}; scenario does not exercise it.`); + } + if (wanted === undefined) { + if (captured.length > 1) { + throw new Error( + `${circuitName} ran ${captured.length} times (${shapes()}); declare in sampleFrom which run to commit.`, + ); + } + return captured[0]; + } + const matching = captured.filter(entry => { + const shape = messageShapeOf(entry); + return shape?.inherited === wanted.inherited && shape.bundle === wanted.bundle; + }); + if (matching.length !== 1) { + throw new Error( + `Expected exactly one ${circuitName} run inheriting ${wanted.inherited} messages and inserting ` + + `${wanted.bundle}, found ${matching.length} of ${captured.length} runs (${shapes()}).`, + ); + } + return matching[0]; + }; + beforeEach(async () => { log = createLogger('prover-client:test:regenerate-rollup-sample-inputs'); context = await TestContext.new(log, { proverCount: 1 }); @@ -136,6 +200,7 @@ describeOrSkip('prover/regenerate-rollup-sample-inputs', () => { numL1ToL2Messages, l1ToL2MessagesPerBlock, dump, + sampleFrom, }) => { const makeProcessedTxOpts = (_: unknown, txIndex: number) => ({ privateOnly: txIndex % 2 === 0 }); const checkpoints = await timesAsync(numCheckpoints, () => @@ -204,12 +269,9 @@ describeOrSkip('prover/regenerate-rollup-sample-inputs', () => { } for (const circuitName of dump) { - const data = getTestData(circuitName); - if (!data || data.length === 0) { - throw new Error(`No test data captured for ${circuitName}; scenario does not exercise it.`); - } - updateProtocolCircuitSampleInputs(circuitName, TOML.stringify(data[0] as any)); - log.info(`Regenerated sample inputs for ${circuitName}`); + const sample = selectSample(circuitName, sampleFrom?.[circuitName]); + updateProtocolCircuitSampleInputs(circuitName, TOML.stringify(sample as any)); + log.info(`Regenerated sample inputs for ${circuitName}`, messageShapeOf(sample)); } } finally { await Promise.all(subTrees.map(s => s.stop())); diff --git a/yarn-project/prover-node/src/job/checkpoint-prover.test.ts b/yarn-project/prover-node/src/job/checkpoint-prover.test.ts index 36d6f75a9997..4301dbcc99e0 100644 --- a/yarn-project/prover-node/src/job/checkpoint-prover.test.ts +++ b/yarn-project/prover-node/src/job/checkpoint-prover.test.ts @@ -417,6 +417,101 @@ describe('CheckpointProver', () => { }); }); + // ---------------- proofs ready before local processing finishes ---------------- + + describe('handoff join', () => { + /** A sub-tree whose proofs are ready from the outset, with its first block parked on a gate. */ + function makeEagerSubTree(gate: Promise, stop: jest.Mock<() => Promise>) { + let firstBlock = true; + return { + getSubTreeResult: () => + Promise.resolve({ + blockProofOutputs: [{ tag: 'block-proof-output' }], + inboxParityProof: { tag: 'inbox-parity-proof' }, + previousArchiveSiblingPath: makeTuple(ARCHIVE_HEIGHT, () => Fr.ZERO), + } as unknown as SubTreeResult), + startNewBlock: () => { + if (firstBlock) { + firstBlock = false; + return gate; + } + return Promise.resolve(); + }, + startChonkVerifierCircuits: () => Promise.resolve(), + addTxs: () => Promise.resolve(), + setBlockCompleted: () => Promise.resolve(), + cancel: () => {}, + stop, + }; + } + + it('holds the proofs and the sub-tree until the block loop is done with them', async () => { + // A cached identical-input proof, or a checkpoint of empty blocks, can make the sub-tree's proofs available + // before the caller has finished creating and using its processing forks. Handing them over then — and + // tearing the sub-tree down on the way — pulls it out from under the loop still running against it. + checkpoint = await Checkpoint.random(CheckpointNumber(1), { numBlocks: 2, txsPerBlock: 0 }); + + txProvider.getTxsForBlock.mockReset(); + txProvider.getTxsForBlock.mockResolvedValue({ txs: [], missingTxs: [] }); + + const firstBlockGate = promiseWithResolvers(); + const stop = jest.fn(() => Promise.resolve()); + proverFactory.createCheckpointSubTreeOrchestrator.mockResolvedValue( + makeEagerSubTree(firstBlockGate.promise, stop) as any, + ); + dbProvider.fork.mockResolvedValue({ + appendLeaves: () => Promise.resolve(), + close: () => Promise.resolve(), + } as any); + publicProcessorFactory.create.mockReturnValue({ process: () => Promise.resolve([[], []]) } as any); + + const prover = makeProver(); + let handedOff = false; + const proofs = prover.whenSubTreeProofsReady().then(result => { + handedOff = true; + return result; + }); + + // The proofs are ready but the first block is still parked, so nothing may be handed over or released. + await sleep(50); + expect(handedOff).toBe(false); + expect(stop).not.toHaveBeenCalled(); + + firstBlockGate.resolve(); + await expect(proofs).resolves.toEqual({ + blockProofOutputs: [{ tag: 'block-proof-output' }], + inboxParityProof: { tag: 'inbox-parity-proof' }, + }); + await prover.whenDone(); + expect(stop).toHaveBeenCalledTimes(1); + expect(prover.isFailed()).toBe(false); + }); + + it('fails rather than handing off proofs that arrived before a block loop that then broke', async () => { + // Same early arrival, but the loop gives up. The failure has to win: no success handoff off the back of + // proofs the local work never validated, and exactly one teardown. + checkpoint = await Checkpoint.random(CheckpointNumber(1), { numBlocks: 2, txsPerBlock: 0 }); + + txProvider.getTxsForBlock.mockReset(); + txProvider.getTxsForBlock.mockResolvedValue({ txs: [], missingTxs: [] }); + + const stop = jest.fn(() => Promise.resolve()); + proverFactory.createCheckpointSubTreeOrchestrator.mockResolvedValue( + makeEagerSubTree(Promise.resolve(), stop) as any, + ); + // The base block was unwound underneath the prover, so the loop cannot fork it. + dbProvider.fork.mockRejectedValue(new Error('Unable to get meta data for block 0')); + + const prover = makeProver(); + + await expect(prover.whenSubTreeProofsReady()).rejects.toThrow(/did not complete block processing/); + await prover.whenDone(); + expect(stop).toHaveBeenCalledTimes(1); + expect(prover.isFailed()).toBe(true); + expect(onFailed).toHaveBeenCalledTimes(1); + }); + }); + // ---------------- data-plane reorg fork fault ---------------- describe('data-plane reorg fault', () => { diff --git a/yarn-project/prover-node/src/job/checkpoint-prover.ts b/yarn-project/prover-node/src/job/checkpoint-prover.ts index 5518bab686a7..8788b72c85cc 100644 --- a/yarn-project/prover-node/src/job/checkpoint-prover.ts +++ b/yarn-project/prover-node/src/job/checkpoint-prover.ts @@ -109,6 +109,15 @@ export class CheckpointProver { * InboxParity proof (which feeds the checkpoint root in the top tree). */ private readonly subTreeProofs: PromiseWithResolvers = promiseWithResolvers(); + /** + * Settles when this prover has finished using the sub-tree: resolved once every block has been processed and + * handed over, rejected if processing gave up. The sub-tree's proofs can be ready before that — a single empty + * block needs none of the local work, and a cached identical-input proof returns immediately — so handing the + * proofs off and releasing the orchestrator on proof arrival alone would pull the sub-tree out from under a + * caller still building forks against it. + */ + private readonly localProcessingDone: PromiseWithResolvers = promiseWithResolvers(); + // Three independent lifecycle facts — deliberately not collapsed into one status enum, because several // combinations are legal and relied on: a prover can be `completed` and then `cancelled` (routine // teardown of an already-proven checkpoint), or `completed` and then `failed` (block proving was @@ -146,6 +155,7 @@ export class CheckpointProver { // Mark subTreeProofs as observed so a cancel that lands before any consumer awaits // does not surface as an unhandled rejection. this.subTreeProofs.promise.catch(() => {}); + this.localProcessingDone.promise.catch(() => {}); deps.log.info(`Created CheckpointProver ${this.id}`, { checkpointNumber: this.checkpoint.number, epochNumber: this.epochNumber, @@ -334,9 +344,16 @@ export class CheckpointProver { this.previousBlockHeader, ); subTreeStarted = true; - // Bridge the sub-tree's result onto subTreeProofs. - void this.subTree.getSubTreeResult().then( - result => { + // Bridge the sub-tree's result onto subTreeProofs, once this prover is also done with the sub-tree. + void this.subTree + .getSubTreeResult() + .then(async result => { + // Join with local processing before handing anything off. Proofs can be ready while the loop below is + // still creating forks and completing later blocks; resolving and tearing down here would close the + // orchestrator's dbs under it. A processing failure rejects the join instead, and the shared catch below + // routes it to `failSubTreeProofs` — which the failing path has already called — so a failure always wins + // over a later success. + await this.localProcessingDone.promise; this.deps.log.info(`Sub-tree block proofs ready for checkpoint ${this.checkpoint.number}`, { checkpointNumber: this.checkpoint.number, blockProofCount: result.blockProofOutputs.length, @@ -355,9 +372,8 @@ export class CheckpointProver { // top-tree job, a rebuilt EpochSession, failure upload) read only `whenSubTreeProofsReady()` and // this prover's own fields (`checkpoint`, `txs`, headers, sibling paths), never the sub-tree. this.teardownPromise = this.teardownSubTree(); - }, - err => this.failSubTreeProofs(err instanceof Error ? err : new Error(String(err))), - ); + }) + .catch(err => this.failSubTreeProofs(err instanceof Error ? err : new Error(String(err)))); if (signal.aborted) { return; } @@ -442,7 +458,12 @@ export class CheckpointProver { }, ); } finally { - if (!this.completed) { + if (this.completed) { + // Release the result callback: it may already be holding a finished set of proofs. + this.localProcessingDone.resolve(); + } else { + // Fail the join first, so the result callback cannot resolve or start a teardown while this one runs. + this.localProcessingDone.reject(new Error(`Checkpoint ${this.id} did not complete block processing`)); if (subTreeStarted) { await this.teardownSubTree(); } diff --git a/yarn-project/sequencer-client/src/sequencer/checkpoint_proposal_job.test.ts b/yarn-project/sequencer-client/src/sequencer/checkpoint_proposal_job.test.ts index 1eb680abdab0..07b0ff6470f7 100644 --- a/yarn-project/sequencer-client/src/sequencer/checkpoint_proposal_job.test.ts +++ b/yarn-project/sequencer-client/src/sequencer/checkpoint_proposal_job.test.ts @@ -176,9 +176,11 @@ describe('CheckpointProposalJob', () => { // ManualDateProvider freezes time (it does not track real wall-clock progression), so timing-sensitive // assertions on dateProvider.now() are deterministic regardless of how long the test takes to execute. dateProvider = new ManualDateProvider(); - // Set time to be at the start of the slot (slot 1 starts at l1GenesisTime + slotDuration - ethereumSlotDuration) - const slotStartTime = Number(l1GenesisTime) + newSlotNumber * slotDuration - ethereumSlotDuration; - dateProvider.setTime(slotStartTime * 1000); // Convert to milliseconds + // Start at the target slot's build frame opening (target_slot_start - S - E), which is when a proposer actually + // begins its turn. Anchoring at the target slot start instead would put every job past the deadlines the + // timetable derives from the build frame, including the proposal send deadline. + const buildFrameStart = Number(l1GenesisTime) + (newSlotNumber - 1) * slotDuration - ethereumSlotDuration; + dateProvider.setTime(buildFrameStart * 1000); // Convert to milliseconds epochCache = mockDeep(); epochCache.getCommittee.mockResolvedValue({ @@ -396,6 +398,11 @@ describe('CheckpointProposalJob', () => { deadline: undefined, isLastBlock: false, }); + // Freezes the clock past every tx-waiting deadline but still inside the proposal send budget. ManualDateProvider + // does not advance, so a job that genuinely waits for txs would hang; tests whose subject is the give-up path + // rather than the waiting itself start here instead. + const setTimePastTxWaits = () => + dateProvider.setTime((job.getTimetable().getCheckpointProposalSendDeadline(SlotNumber(newSlotNumber)) - 1) * 1000); const makeSingleBlockTimetable = () => makeProposerTimetable({ l1Constants, @@ -483,6 +490,7 @@ describe('CheckpointProposalJob', () => { job.updateConfig({ minTxsPerBlock: 2 }); + setTimePastTxWaits(); const checkpoint = await job.executeAndAwait(); expect(checkpoint).toBeUndefined(); @@ -1295,6 +1303,7 @@ describe('CheckpointProposalJob', () => { const waitSpy = jest.spyOn(job, 'waitUntilNextSubslot'); job.updateConfig({ minTxsPerBlock: 0 }); + setTimePastTxWaits(); const checkpoint = await job.executeAndAwait(); expect(checkpoint).toBeDefined(); @@ -1353,6 +1362,7 @@ describe('CheckpointProposalJob', () => { const waitSpy = jest.spyOn(job, 'waitUntilNextSubslot'); job.updateConfig({ minTxsPerBlock: 5, buildCheckpointIfEmpty: false }); + setTimePastTxWaits(); const checkpoint = await job.executeAndAwait(); expect(checkpoint).toBeUndefined(); @@ -1391,6 +1401,72 @@ describe('CheckpointProposalJob', () => { expect(publisher.enqueueProposeCheckpoint).not.toHaveBeenCalled(); }); + // The tx-polling interval, which must match TXS_POLLING_MS in checkpoint_proposal_job.ts. + const TXS_POLLING_MS = 500; + + // Sets the clock inside the tx-waiting deadline, so only the send budget can stop the wait, and pins the send + // deadline `remainingMs` away from it. Returns a spy that counts waits and advances the clock like a real one. + const armTxPollWithSendBudget = (remainingMs: number) => { + jest + .spyOn(job.getTimetable(), 'selectNextSubslot') + .mockReturnValueOnce(subslot(10, 0, true)) + .mockReturnValue(noSubslot()); + p2p.getPendingTxCount.mockResolvedValue(10); + p2p.hasEligiblePendingTxs.mockResolvedValue(false); + + // The wait-for-txs deadline is the subslot deadline (+10s) less minBlockDuration (2s), so +1s is well inside it. + const nowMs = (buildFrameStartSeconds() + 1) * 1000; + dateProvider.setTime(nowMs); + jest.spyOn(job.getTimetable(), 'getCheckpointProposalSendDeadline').mockReturnValue((nowMs + remainingMs) / 1000); + + job.updateConfig({ minTxsPerBlock: 5, buildCheckpointIfEmpty: false }); + return jest.spyOn(job, 'waitForTxsPollingInterval').mockImplementation(() => { + dateProvider.setTime(dateProvider.now() + TXS_POLLING_MS); + return Promise.resolve(); + }); + }; + + it('does not start a tx poll the proposal send budget cannot cover', async () => { + const pollSpy = armTxPollWithSendBudget(TXS_POLLING_MS - 1); + + await job.executeAndAwait(); + + // A poll here would end past the send deadline, so it buys nothing and costs the rest of the budget. + expect(pollSpy).not.toHaveBeenCalled(); + expect(checkpointBuilder.buildBlockCalls).toHaveLength(0); + }); + + it('still waits for txs when a full poll fits inside the proposal send budget', async () => { + const pollSpy = armTxPollWithSendBudget(TXS_POLLING_MS + 1); + + await job.executeAndAwait(); + + expect(pollSpy).toHaveBeenCalled(); + }); + + it('does not wait out another sub-slot once the proposal send budget is spent', async () => { + // Two buildable sub-slots, so a failed first block would normally wait for the second and retry. + jest + .spyOn(job.getTimetable(), 'selectNextSubslot') + .mockReturnValueOnce(subslot(10, 0, false)) + .mockReturnValueOnce(subslot(20, 1, true)) + .mockReturnValue(noSubslot()); + p2p.getPendingTxCount.mockResolvedValue(10); + p2p.hasEligiblePendingTxs.mockResolvedValue(false); + + const nowMs = (buildFrameStartSeconds() + 1) * 1000; + dateProvider.setTime(nowMs); + jest + .spyOn(job.getTimetable(), 'getCheckpointProposalSendDeadline') + .mockReturnValue((nowMs + TXS_POLLING_MS - 1) / 1000); + const subslotSpy = jest.spyOn(job, 'waitUntilNextSubslot'); + + job.updateConfig({ minTxsPerBlock: 5, buildCheckpointIfEmpty: false }); + await job.executeAndAwait(); + + expect(subslotSpy).not.toHaveBeenCalled(); + }); + it('stops building when selectNextSubslot returns false', async () => { // Mock timetable to stop after 1 block (simulating time running out) jest @@ -1592,6 +1668,7 @@ describe('CheckpointProposalJob', () => { validatorClient.collectAttestations.mockResolvedValue(getAttestations(lastBlock)); job.updateConfig({ minTxsPerBlock: 1, buildCheckpointIfEmpty: false }); + setTimePastTxWaits(); const checkpoint = await job.executeAndAwait(); expect(checkpoint).toBeDefined(); @@ -2042,18 +2119,23 @@ describe('CheckpointProposalJob', () => { // point at which its signature or its send could still land. describe('preflight deadlines', () => { const attestationDeadlineMs = () => job.getTimetable().getAttestationDeadline(SlotNumber(newSlotNumber)) * 1000; + const sendDeadlineMs = () => + job.getTimetable().getCheckpointProposalSendDeadline(SlotNumber(newSlotNumber)) * 1000; const l1PublishDeadlineMs = () => (Number(l1Constants.l1GenesisTime) + newSlotNumber * slotDuration + slotDuration - ethereumSlotDuration) * 1000; - it('does not sign the checkpoint when the pre-gossip preflight resolves after the attestation deadline', async () => { + it('does not sign the checkpoint when the pre-gossip preflight resolves after the send deadline', async () => { mockSubslots(1); streamingInbox.set(leaves(2)); + // A verdict that lands after peers stop accepting proposals but well before the attestation cutoff: the + // two are a whole ethereum slot plus a block duration apart, and only the earlier one bounds the send. publisher.validateCheckpointHeaderAndInbox.mockImplementation(() => { - dateProvider.setTime(attestationDeadlineMs() + 1_000); + dateProvider.setTime(sendDeadlineMs() + 1_000); return Promise.resolve(0n); }); await setupMultipleBlocks(1, [1]); + expect(sendDeadlineMs()).toBeLessThan(attestationDeadlineMs()); const checkpoint = await job.executeAndAwait(); expect(checkpoint).toBeUndefined(); @@ -2062,15 +2144,15 @@ describe('CheckpointProposalJob', () => { expect(metrics.recordCheckpointProposalFailed).toHaveBeenCalledWith('header_validation_timeout'); }); - it('abandons a pre-gossip preflight that does not answer within the remaining attestation window', async () => { + it('abandons a pre-gossip preflight that does not answer within the remaining send window', async () => { mockSubslots(1); streamingInbox.set(leaves(2)); publisher.validateCheckpointHeaderAndInbox.mockImplementation(() => new Promise(() => {})); await setupMultipleBlocks(1, [1]); - // Once the block is built there are 200ms left to gossip a proposal validators could still attest to. + // Once the block is built there are 200ms left to gossip a proposal peers would still accept. const completeCheckpoint = checkpointBuilder.completeCheckpoint.bind(checkpointBuilder); jest.spyOn(checkpointBuilder, 'completeCheckpoint').mockImplementation(() => { - dateProvider.setTime(attestationDeadlineMs() - 200); + dateProvider.setTime(sendDeadlineMs() - 200); return completeCheckpoint(); }); @@ -2081,6 +2163,55 @@ describe('CheckpointProposalJob', () => { expect(metrics.recordCheckpointProposalFailed).toHaveBeenCalledWith('header_validation_timeout'); }); + it('does not gossip when a slow signer returns after the send deadline', async () => { + mockSubslots(1); + streamingInbox.set(leaves(2)); + const createProposal = validatorClient.createCheckpointProposal.getMockImplementation()!; + validatorClient.createCheckpointProposal.mockImplementation(((...args: unknown[]) => { + dateProvider.setTime(sendDeadlineMs() + 1_000); + return (createProposal as (...a: unknown[]) => unknown)(...args); + }) as any); + await setupMultipleBlocks(1, [1]); + + const checkpoint = await job.executeAndAwait(); + + expect(checkpoint).toBeUndefined(); + // The signature was produced and its duty record stands; only the send is abandoned. + expect(validatorClient.createCheckpointProposal).toHaveBeenCalledTimes(1); + expect(p2p.broadcastCheckpointProposal).not.toHaveBeenCalled(); + expect(metrics.recordCheckpointProposalFailed).toHaveBeenCalledWith('proposal_send_timeout'); + }); + + it('does not gossip when the queued archiver insertion returns after the send deadline', async () => { + mockSubslots(1); + streamingInbox.set(leaves(2)); + blockSink.addProposedCheckpoint.mockImplementation(() => { + dateProvider.setTime(sendDeadlineMs() + 1_000); + return Promise.resolve(); + }); + await setupMultipleBlocks(1, [1]); + + const checkpoint = await job.executeAndAwait(); + + expect(checkpoint).toBeUndefined(); + expect(validatorClient.createCheckpointProposal).toHaveBeenCalledTimes(1); + expect(p2p.broadcastCheckpointProposal).not.toHaveBeenCalled(); + expect(metrics.recordCheckpointProposalFailed).toHaveBeenCalledWith('proposal_send_timeout'); + }); + + it('broadcasts once when preflight, signing and insertion all fit inside the send budget', async () => { + mockSubslots(1); + streamingInbox.set(leaves(2)); + const { lastBlock } = await setupMultipleBlocks(1, [1]); + validatorClient.collectAttestations.mockResolvedValue(getAttestations(lastBlock)); + + const checkpoint = await job.executeAndAwait(); + + expect(checkpoint).toBeDefined(); + expect(p2p.broadcastCheckpointProposal).toHaveBeenCalledTimes(1); + expect(metrics.recordCheckpointProposalFailed).not.toHaveBeenCalledWith('proposal_send_timeout'); + }); + it('does not sign the checkpoint when the job is interrupted while the pre-gossip preflight runs', async () => { mockSubslots(1); streamingInbox.set(leaves(2)); @@ -2522,6 +2653,11 @@ class TestCheckpointProposalJob extends CheckpointProposalJob { await this.pendingRequests.awaitRequests(); } + /** Widened so tests whose subject is whether the job waits at all can observe or stub the wait. */ + public override waitForTxsPollingInterval(): Promise { + return super.waitForTxsPollingInterval(); + } + /** Wraps execute + awaitPendingSubmission so tests see the full pipeline complete. */ public async executeAndAwait(): Promise { const result = await this.execute(); diff --git a/yarn-project/sequencer-client/src/sequencer/checkpoint_proposal_job.ts b/yarn-project/sequencer-client/src/sequencer/checkpoint_proposal_job.ts index 929339434464..e894e8515c00 100644 --- a/yarn-project/sequencer-client/src/sequencer/checkpoint_proposal_job.ts +++ b/yarn-project/sequencer-client/src/sequencer/checkpoint_proposal_job.ts @@ -120,11 +120,6 @@ type CheckpointProposalBroadcast = { checkpoint: Checkpoint; proposal: CheckpointProposal; blockProposedAt: number; - /** - * Sequence number of the live Inbox bucket the checkpoint's final message position resolved to at the pre-gossip - * preflight: the unsigned L1 `propose` lookup aid. Re-resolved by the pre-publication preflight before the send. - */ - bucketHint: bigint; /** The checkpoint's final streaming state, for the pre-publication preflight. */ streamingState: StreamingCheckpointState; }; @@ -542,6 +537,44 @@ export class CheckpointProposalJob implements Traceable { return new Date(this.timetable.getAttestationDeadline(this.targetSlot) * 1000); } + /** + * The latest moment at which this proposal may still be sent: the consensus receive deadline less one propagation + * budget. Everything on the path to `broadcastCheckpointProposal` — the pre-gossip preflight, signing, the local + * archiver insertion — is budgeted against it, because a proposal that leaves this node later than this is refused + * on ingress by every peer. This is deliberately tighter than {@link getAttestationDeadline}, which bounds + * attestation collection, and than {@link getL1PublishDeadline}, which bounds the send. + */ + private getProposalSendDeadline(): Date { + return new Date(this.timetable.getCheckpointProposalSendDeadline(this.targetSlot) * 1000); + } + + /** + * Whether there is still time to gossip this slot's proposal. Signing may be remote (HA) and the archiver insertion + * resolves through a queue, so neither is guaranteed to be quick; a timer wrapped around the preflight alone does + * not cover them. Callers re-check before each further side effect, and abandon the send rather than starting one + * that peers will refuse. An already-produced signature and its duty record are left untouched. + */ + private reportSendBudgetExpired(stage: string, deadline: Date): void { + const context = { + slot: this.targetSlot, + checkpointNumber: this.checkpointNumber, + stage, + deadline: deadline.toISOString(), + reason: 'proposal_send_timeout', + }; + this.log.warn( + `Checkpoint proposal for slot ${this.targetSlot} missed the proposal send deadline during ${stage}; ` + + `not gossiping it`, + context, + ); + this.metrics.recordCheckpointProposalFailed('proposal_send_timeout'); + this.eventEmitter.emit('header-validation-failed', { + slot: this.targetSlot, + checkpointNumber: this.checkpointNumber, + reason: `proposal send deadline ${deadline.toISOString()} passed during ${stage}`, + }); + } + /** * Latest L1 block the propose can still land in for the target slot: the last Ethereum block inside the target slot * (`target_slot_start + S - E`). This is one ethereum slot later than `attestation_deadline`, which bounds when @@ -1064,14 +1097,8 @@ export class CheckpointProposalJob implements Traceable { ); this.metrics.recordCheckpointSuccess(); // Return a broadcast result with a dummy proposal — fisherman mode skips attestation collection and never - // publishes, so the bucket hint is never read. - return { - checkpoint, - proposal: undefined!, - blockProposedAt: this.dateProvider.now(), - bucketHint: 0n, - streamingState, - }; + // publishes. + return { checkpoint, proposal: undefined!, blockProposedAt: this.dateProvider.now(), streamingState }; } // Validate the header and the Inbox consumption against L1 state before broadcasting: the parent the header @@ -1079,15 +1106,15 @@ export class CheckpointProposalJob implements Traceable { // rules. If this fails the slot is aborted before any gossip work. The pipelined parent is supplied through // the simulation overrides, so this verdict is conditional on that parent landing; the pre-publication // preflight repeats it once the parent has landed, keeping only the assumptions still outstanding then. - // The simulation is bounded by the attestation deadline: a verdict that arrives once no validator can attest - // any more must not lead to signing. - let bucketHint: bigint; + // The simulation is bounded by the proposal send deadline, not the attestation deadline: a verdict arriving + // once peers have stopped accepting proposals for this slot must not lead to signing and gossiping one. + const sendDeadline = this.getProposalSendDeadline(); try { - bucketHint = await this.preflightWithinDeadline( + await this.preflightWithinDeadline( checkpoint.header, streamingState, this.checkpointSimulationOverridesPlan, - this.getAttestationDeadline(), + sendDeadline, ); } catch (err) { if (err instanceof SequencerInterruptedError) { @@ -1121,6 +1148,13 @@ export class CheckpointProposalJob implements Traceable { checkpointProposalOptions, ); + // Signing may be remote (HA), so the preflight's own budget check does not cover it. Stop here rather than + // start the archiver insertion for a proposal no peer will accept; the signature and its duty record stand. + if (this.dateProvider.now() >= sendDeadline.getTime()) { + this.reportSendBudgetExpired('signing', sendDeadline); + return undefined; + } + // Advance our own optimistic proposed-checkpoint tip locally before gossiping. Gossipsub // doesn't echo our own messages back, so this is how the proposer makes its own proposed // checkpoint visible for pipelining the next slot. Built from local checkpoint data — never @@ -1128,6 +1162,13 @@ export class CheckpointProposalJob implements Traceable { // Fail closed: if this throws, the outer catch aborts the slot before gossiping. await this.syncProposedCheckpointToArchiver(checkpoint, blocksInCheckpoint.length, feeAssetPriceModifier); + // The insertion resolves through the archiver's queue, so re-check immediately before the broadcast itself. + // The local tip stays advanced: it is this node's own optimistic state, not something peers acted on. + if (this.dateProvider.now() >= sendDeadline.getTime()) { + this.reportSendBudgetExpired('archiver insertion', sendDeadline); + return undefined; + } + const blockProposedAt = this.dateProvider.now(); if (this.config.skipBroadcastCheckpointProposal) { // Test-only: suppress the CheckpointProposal so peers never see a proposed checkpoint for @@ -1142,10 +1183,8 @@ export class CheckpointProposalJob implements Traceable { this.checkpointMetrics.noteCheckpointBroadcast(this.dateProvider.now()); } - // Return immediately after broadcast — attestation collection happens in the background. The bucket hint is - // the live bucket the preflight resolved the header's final position to, whether or not a last block was held - // for broadcast. - return { checkpoint, proposal, blockProposedAt, bucketHint, streamingState }; + // Return immediately after broadcast — attestation collection happens in the background. + return { checkpoint, proposal, blockProposedAt, streamingState }; } catch (err) { if (err && (err instanceof DutyAlreadySignedError || err instanceof SlashingProtectionError)) { // swallow this error. It's already been logged by a function deeper in the stack @@ -1249,6 +1288,16 @@ export class CheckpointProposalJob implements Traceable { if (timingInfo.isLastBlock) { break; } + // Waiting out a whole sub-slot is subject to the same budget as waiting out a tx poll, and is longer: once + // the proposal can no longer be sent, another attempt cannot produce a block any peer would accept, and the + // wait would take the checkpoint we already have past the deadline with it. + if (this.getProposalSendDeadline().getTime() - this.dateProvider.now() < TXS_POLLING_MS) { + this.log.verbose( + `Not waiting for another sub-slot in slot ${this.targetSlot}: the proposal send deadline is too close`, + { slot: this.targetSlot, checkpointNumber: this.checkpointNumber, blocksBuilt }, + ); + break; + } // Otherwise, if there is still time for more blocks, we wait until the next subslot and try again await this.waitUntilNextSubslot(timingInfo.deadline); continue; @@ -1466,8 +1515,7 @@ export class CheckpointProposalJob implements Traceable { if (this.dateProvider.now() / 1000 < lastBlockBuildTime) { return { deadline: new Date(lastBlockBuildTime * 1000), pastLastBlockBuildTime: false }; } - const hardStop = - this.timetable.getCheckpointProposalReceiveDeadline(this.targetSlot) - this.timetable.p2pPropagationTime; + const hardStop = this.timetable.getCheckpointProposalSendDeadline(this.targetSlot); return { deadline: new Date(hardStop * 1000), pastLastBlockBuildTime: true }; } @@ -1522,9 +1570,11 @@ export class CheckpointProposalJob implements Traceable { * L1 bucket end, or when its prospective end would pass the threshold one bucket below the cap and could leave the * last legal endpoint behind. The lookup is bounded by the checkpoint cap on a non-final block, so a mandatory * bucket beyond this block's own reach is not stranded by a nearer endpoint, and additionally by one block's - * capacity on the final block. The block then ends at the further of what the lookup allows and the safe local - * step, so consulting L1 never consumes less than staying below the threshold would have; a block that consulted - * L1 may legitimately end inside a bucket. Nothing is retained: the next attempt decides again. + * capacity on the final block. A non-final block then ends at the further of what the lookup allows and the safe + * local step, so consulting L1 never consumes less than staying below the threshold would have, and it may + * legitimately end inside a bucket. The final block instead ends exactly on the resolved boundary, so whenever the + * last live boundary within reach sits behind the safe local step it consumes fewer messages than the local log + * alone would allow. Nothing is retained: the next attempt decides again. * * An endpoint that cannot be resolved on a non-final block (local lag, no live endpoint yet) leaves the block with * that safe local step and is retried on the next block; on the final block it abandons the checkpoint. A local @@ -1596,8 +1646,10 @@ export class CheckpointProposalJob implements Traceable { bucketSeq: resolved.bucketSeq, end, }); - // Re-read the selected prefix so the signed hash is the one at `end`, never the farther endpoint's. - return this.readStreamingRange(state, end); + // An end short of or past the endpoint needs its own read, so the signed hash is the one at `end` and never the + // endpoint's; landing exactly on the endpoint reuses the snapshot the resolver already read and checked against + // the cursor's hash. + return end === endpointTotal ? { kind: 'consume', range: resolved.range } : this.readStreamingRange(state, end); } /** @@ -1959,6 +2011,26 @@ export class CheckpointProposalJob implements Traceable { return { canStartBuilding: false, minTxs }; } + // Never start a poll the send budget cannot cover. Peers refuse this slot's proposal once the send deadline + // passes, so a full interval that ends past it cannot produce a block anyone would accept, and spending it + // leaves nothing for the signing and archiver insertion still to come. Stop waiting and let the checkpoint + // go with the blocks already built. + const sendDeadline = this.getProposalSendDeadline(); + if (sendDeadline.getTime() - now.getTime() < TXS_POLLING_MS) { + this.log.verbose( + `Not waiting for txs to build block ${blockNumber} at index ${indexWithinCheckpoint} in slot ` + + `${this.targetSlot}: a poll would outlast the proposal send deadline`, + { + blockNumber, + slot: this.targetSlot, + indexWithinCheckpoint, + minTxs, + sendDeadline: sendDeadline.toISOString(), + }, + ); + return { canStartBuilding: false, minTxs }; + } + // Wait a bit before checking again this.setState(SequencerState.WAITING_FOR_TXS); this.log.verbose( diff --git a/yarn-project/sequencer-client/src/sequencer/sequencer.test.ts b/yarn-project/sequencer-client/src/sequencer/sequencer.test.ts index 74883fa47efe..01ca0d14910a 100644 --- a/yarn-project/sequencer-client/src/sequencer/sequencer.test.ts +++ b/yarn-project/sequencer-client/src/sequencer/sequencer.test.ts @@ -43,8 +43,9 @@ import { type WorldStateSynchronizer, type WorldStateSynchronizerStatus, } from '@aztec/stdlib/interfaces/server'; -import type { L1ToL2MessageSource } from '@aztec/stdlib/messaging'; +import { type L1ToL2MessageSource, MIN_BLOCKS_FOR_INBOX_CATCHUP } from '@aztec/stdlib/messaging'; import { CheckpointHeader } from '@aztec/stdlib/rollup'; +import { FAST_PROFILE_ETHEREUM_SLOT_DURATION } from '@aztec/stdlib/timetable'; import { AppendOnlyTreeSnapshot } from '@aztec/stdlib/trees'; import { BlockHeader, GlobalVariables, type Tx } from '@aztec/stdlib/tx'; import type { FullNodeCheckpointsBuilder, ValidatorClient } from '@aztec/validator-client'; @@ -93,6 +94,7 @@ describe('sequencer', () => { >; let sequencer: TestSequencer; + let config: SequencerConfig & Pick; const slotDuration = 8; const ethereumSlotDuration = 4; @@ -390,7 +392,7 @@ describe('sequencer', () => { dateProvider = new TestDateProvider(); signatureContext = { chainId: chainId.toNumber(), rollupAddress: EthAddress.random() }; - const config: SequencerConfig & Pick = { + config = { maxTxsPerBlock: 4, l1ChainId: signatureContext.chainId, // With aztecSlotDuration=8 and ethereumSlotDuration=4 (fast profile), a 2s block duration derives @@ -419,6 +421,79 @@ describe('sequencer', () => { sequencer.updateConfig(config); }); + describe('Inbox catch-up capacity guard', () => { + // Production profile: 12s ethereum slots keep the conservative budgets, so maxBlocks is driven purely by the + // slot and block durations. floor((S - 1 - D - 2*2 - 1) / D) with S=36 gives 8 blocks at D=3 and 2 at D=10. + const productionConstants = () => ({ ...l1Constants, slotDuration: 36, ethereumSlotDuration: 12 }); + + const buildSequencer = (overrides: Partial, constants = productionConstants()) => { + const sequencerConfig = { ...config, blockDurationMs: 3000, ...overrides }; + return new TestSequencer( + publisherFactory, + validatorClient, + globalVariableBuilder, + p2p, + worldState, + slasherClient, + l2BlockSource, + l1ToL2MessageSource, + checkpointsBuilder as unknown as FullNodeCheckpointsBuilder, + constants, + dateProvider, + epochCache, + rollupContract, + inboxContract, + sequencerConfig, + ); + }; + + it.each([1, 2, 3])('rejects a configured cap of %i block(s) per checkpoint', maxBlocksPerCheckpoint => { + expect(() => buildSequencer({ maxBlocksPerCheckpoint })).toThrow(/streaming-Inbox backlog/); + }); + + it('accepts a configured cap at the floor', () => { + expect(() => buildSequencer({ maxBlocksPerCheckpoint: MIN_BLOCKS_FOR_INBOX_CATCHUP })).not.toThrow(); + }); + + it('rejects timings that derive fewer blocks than the floor even when the configured cap is above it', () => { + // D=10 leaves floor((36 - 1 - 10 - 4 - 1) / 10) = 2 sub-slots, under a generous configured cap. + expect(() => buildSequencer({ maxBlocksPerCheckpoint: 8, blockDurationMs: 10_000 })).toThrow( + /streaming-Inbox backlog/, + ); + }); + + it('warns rather than rejects on a fast local profile', () => { + expect(() => buildSequencer({ maxBlocksPerCheckpoint: 1, blockDurationMs: 2000 }, l1Constants)).not.toThrow(); + }); + + // The single-node e2e default (DEFAULT_L1_BLOCK_TIME) sits at exactly this boundary, deliberately, so that + // those runs keep the production timing budgets. Rejecting there would fail every default-cadence e2e run. + it('warns rather than rejects at the fast-profile boundary', () => { + const atBoundary = { ...productionConstants(), ethereumSlotDuration: FAST_PROFILE_ETHEREUM_SLOT_DURATION }; + expect(() => buildSequencer({ maxBlocksPerCheckpoint: 1 }, atBoundary)).not.toThrow(); + }); + + // The exemption is a threshold on the Ethereum slot duration, not a declaration that this is a development + // network, so pin where it ends: the same undersized configuration one second slower is rejected outright. + it('rejects the same undersized configuration just above the fast-profile boundary', () => { + const aboveBoundary = { + ...productionConstants(), + ethereumSlotDuration: FAST_PROFILE_ETHEREUM_SLOT_DURATION + 1, + }; + expect(() => buildSequencer({ maxBlocksPerCheckpoint: 1 }, aboveBoundary)).toThrow(/streaming-Inbox backlog/); + }); + + it('leaves the committed config and timetable intact when an update is rejected', () => { + const sequencer = buildSequencer({ maxBlocksPerCheckpoint: 8 }); + const before = sequencer.getTimeTable(); + + expect(() => sequencer.updateConfig({ blockDurationMs: 10_000 })).toThrow(/streaming-Inbox backlog/); + + expect(sequencer.getTimeTable()).toBe(before); + expect(sequencer.getTimeTable().blockDuration).toEqual(3); + }); + }); + describe('perBlockAllocationMultiplier guard', () => { it('rejects a multiplier below the network minimum', () => { expect(() => sequencer.updateConfig({ perBlockAllocationMultiplier: 1.0 })).toThrow( diff --git a/yarn-project/sequencer-client/src/sequencer/sequencer.ts b/yarn-project/sequencer-client/src/sequencer/sequencer.ts index 07f3f405917c..211bea98781e 100644 --- a/yarn-project/sequencer-client/src/sequencer/sequencer.ts +++ b/yarn-project/sequencer-client/src/sequencer/sequencer.ts @@ -36,10 +36,14 @@ import { SequencerConfigSchema, type WorldStateSynchronizer, } from '@aztec/stdlib/interfaces/server'; -import type { L1ToL2MessageSource } from '@aztec/stdlib/messaging'; +import { type L1ToL2MessageSource, MIN_BLOCKS_FOR_INBOX_CATCHUP } from '@aztec/stdlib/messaging'; import type { CoordinationSignatureContext } from '@aztec/stdlib/p2p'; import { pickFromSchema } from '@aztec/stdlib/schemas'; -import { ProposerTimetable, buildProposerTimetable } from '@aztec/stdlib/timetable'; +import { + FAST_PROFILE_ETHEREUM_SLOT_DURATION, + ProposerTimetable, + buildProposerTimetable, +} from '@aztec/stdlib/timetable'; import { Attributes, type TelemetryClient, type Tracer, getTelemetryClient, trackSpan } from '@aztec/telemetry-client'; import { FullNodeCheckpointsBuilder, NodeKeystoreAdapter, type ValidatorClient } from '@aztec/validator-client'; @@ -215,11 +219,63 @@ export class Sequencer extends (EventEmitter as new () => TypedEventEmitter= MIN_BLOCKS_FOR_INBOX_CATCHUP) { + return; + } + + const detail = + `this sequencer can build at most ${effectiveMaxBlocks} block(s) per checkpoint ` + + `(MAX_BLOCKS_PER_CHECKPOINT ${config.maxBlocksPerCheckpoint}, ${timetableMaxBlocks} derived from slot ` + + `timings), below the ${MIN_BLOCKS_FOR_INBOX_CATCHUP} needed to clear a mandatory streaming-Inbox backlog`; + + if (this.l1Constants.ethereumSlotDuration <= FAST_PROFILE_ETHEREUM_SLOT_DURATION) { + this.log.warn(`Inbox catch-up capacity below the floor: ${detail}.`, { + effectiveMaxBlocks, + maxBlocksPerCheckpoint: config.maxBlocksPerCheckpoint, + timetableMaxBlocks, + minBlocksForInboxCatchup: MIN_BLOCKS_FOR_INBOX_CATCHUP, + }); + return; + } + + throw new Error( + `Rejecting sequencer configuration: ${detail}. Raise MAX_BLOCKS_PER_CHECKPOINT, lower the block duration, ` + + `or raise the slot duration.`, + ); + } + /** * Checks this node's configured per-block allocation against the network admission limit. A node * advertises and admits txs up to the limit derived from the network-minimum multipliers (see diff --git a/yarn-project/stdlib/src/checkpoint/checkpoint_reexecution_tracker.ts b/yarn-project/stdlib/src/checkpoint/checkpoint_reexecution_tracker.ts index c4b85c73e2ea..9c9333b0c3f2 100644 --- a/yarn-project/stdlib/src/checkpoint/checkpoint_reexecution_tracker.ts +++ b/yarn-project/stdlib/src/checkpoint/checkpoint_reexecution_tracker.ts @@ -10,8 +10,11 @@ import type { Fr } from '@aztec/foundation/curves/bn254'; * - `unvalidated` — the local node could not complete validation for non-deterministic reasons * (missing blocks/txs, timeouts, infra errors). Treated as proposer-fault for * slashing but surfaced separately for telemetry. + * - `unverifiable` — this observer could not check the proposal against an authority outside itself, + * so it learned nothing about the proposer. Never proposer-fault: it is recorded + * only to keep the absence of a verdict from being read as an absent proposal. */ -export type ReexecutionOutcome = 'valid' | 'invalid' | 'unvalidated'; +export type ReexecutionOutcome = 'valid' | 'invalid' | 'unvalidated' | 'unverifiable'; /** * Tracks two pieces of per-slot state collected during proposal handling: diff --git a/yarn-project/stdlib/src/p2p/checkpoint_proposal.test.ts b/yarn-project/stdlib/src/p2p/checkpoint_proposal.test.ts index 98c4d6a26bc4..24f9e223d9f1 100644 --- a/yarn-project/stdlib/src/p2p/checkpoint_proposal.test.ts +++ b/yarn-project/stdlib/src/p2p/checkpoint_proposal.test.ts @@ -7,7 +7,7 @@ import { bufferToHex, hexToBuffer } from '@aztec/foundation/string'; import { InboxMessagePrefixRef } from '../messaging/inbox_message_prefix_ref.js'; import { CheckpointHeader } from '../rollup/checkpoint_header.js'; -import { makeCheckpointProposal } from '../tests/mocks.js'; +import { makeCheckpointProposal, mockTx } from '../tests/mocks.js'; import { BlockHeader } from '../tx/block_header.js'; import { TxHash } from '../tx/tx_hash.js'; import { CheckpointProposal } from './checkpoint_proposal.js'; @@ -34,6 +34,32 @@ const makeLegacyFixtureCheckpointProposal = () => ); describe('CheckpointProposal serialization / deserialization', () => { + it.each([ + ['no lastBlock', () => makeCheckpointProposal({})], + ['a nonzero fee asset price modifier', () => makeCheckpointProposal({ feeAssetPriceModifier: -1234n })], + ['a lastBlock', () => makeCheckpointProposal({ lastBlock: {} })], + [ + 'a lastBlock carrying its txs', + async () => { + const tx = await mockTx(1); + return makeCheckpointProposal({ lastBlock: { txHashes: [tx.getTxHash()], txs: [tx] } }); + }, + ], + [ + 'a lastBlock with an inbox prefix reference', + () => { + const checkpointHeader = CheckpointHeader.random(); + return makeCheckpointProposal({ + checkpointHeader, + lastBlock: { inboxPrefixRef: new InboxMessagePrefixRef(checkpointHeader.inboxRollingHash) }, + }); + }, + ], + ])('reports the actual serialized size with %s', async (_name, build) => { + const proposal = await build(); + expect(proposal.getSize()).toEqual(proposal.toBuffer().length); + }); + it('round-trips with a lastBlock', async () => { const proposal = await makeCheckpointProposal({ lastBlock: {} }); const deserialized = CheckpointProposal.fromBuffer(proposal.toBuffer()); diff --git a/yarn-project/stdlib/src/p2p/checkpoint_proposal.ts b/yarn-project/stdlib/src/p2p/checkpoint_proposal.ts index 0459de3e84b2..8ca8d697c0a6 100644 --- a/yarn-project/stdlib/src/p2p/checkpoint_proposal.ts +++ b/yarn-project/stdlib/src/p2p/checkpoint_proposal.ts @@ -365,29 +365,12 @@ export class CheckpointProposal extends Gossipable implements Signable { return new CheckpointProposal(checkpointHeader, archive, feeAssetPriceModifier, signature, signatureContext); } + /** + * Size in bytes of the serialized proposal. Measured from the serialization itself so it cannot drift from + * `toBuffer` as optional fields are added or their widths change. + */ getSize(): number { - let size = - this.checkpointHeader.toBuffer().length + - this.archive.size + - this.signature.getSize() + - 8 /* feeAssetPriceModifier */ + - 4 /* chainId */ + - 20 /* rollupAddress */ + - 4; /* hasLastBlock flag */ - - if (this.lastBlock) { - size += - this.lastBlock.blockHeader.getSize() + - 4 /* indexWithinCheckpoint */ + - this.lastBlock.signature.getSize() + - 4 /* txHashes.length */ + - this.lastBlock.txHashes.length * TxHash.SIZE + - 4 /* hasSignedTxs flag */ + - (this.lastBlock.signedTxs ? this.lastBlock.signedTxs.getSize() : 0) + - (this.lastBlock.inboxPrefixRef ? 4 /* hasInboxPrefixRef flag */ + this.lastBlock.inboxPrefixRef.getSize() : 0); - } - - return size; + return this.toBuffer().length; } static empty(): CheckpointProposal { diff --git a/yarn-project/stdlib/src/timetable/budgets.ts b/yarn-project/stdlib/src/timetable/budgets.ts index c455bd800b65..bcab8cb6cff6 100644 --- a/yarn-project/stdlib/src/timetable/budgets.ts +++ b/yarn-project/stdlib/src/timetable/budgets.ts @@ -43,6 +43,14 @@ export const FAST_PROFILE_CHECKPOINT_PROPOSAL_PREPARE_TIME = 0.5; /** Fast-profile minimum block-building budget (seconds). See {@link FAST_PROFILE_P2P_PROPAGATION_TIME}. */ export const FAST_PROFILE_MIN_BLOCK_DURATION = 1; +/** + * Whether a network's Ethereum slot duration marks it as a fast local/e2e profile with mocked p2p rather than a + * production deployment. See {@link FAST_PROFILE_ETHEREUM_SLOT_DURATION}. + */ +export function isFastLocalProfile(ethereumSlotDuration: number): boolean { + return ethereumSlotDuration < FAST_PROFILE_ETHEREUM_SLOT_DURATION; +} + /** Resolved operational timing budgets used to size the proposer build window. */ export type ResolvedTimingBudgets = { minBlockDuration: number; @@ -70,8 +78,7 @@ export function getDefaultCheckpointProposalSyncGrace(blockDuration: number): nu export function resolveTimingBudgets(ethereumSlotDuration: number, opts: ResolvedTimingBudgets): ResolvedTimingBudgets { const { minBlockDuration, p2pPropagationTime, checkpointProposalPrepareTime, checkpointProposalInitTime } = opts; - const isFastProfile = ethereumSlotDuration < FAST_PROFILE_ETHEREUM_SLOT_DURATION; - if (!isFastProfile) { + if (!isFastLocalProfile(ethereumSlotDuration)) { return { minBlockDuration, p2pPropagationTime, checkpointProposalPrepareTime, checkpointProposalInitTime }; } diff --git a/yarn-project/stdlib/src/timetable/proposer_timetable.ts b/yarn-project/stdlib/src/timetable/proposer_timetable.ts index 42f01fdd3672..3555e99837be 100644 --- a/yarn-project/stdlib/src/timetable/proposer_timetable.ts +++ b/yarn-project/stdlib/src/timetable/proposer_timetable.ts @@ -166,6 +166,19 @@ export class ProposerTimetable extends ConsensusTimetable { return this.maxBlocksPerCheckpoint; } + /** + * Latest moment at which the proposer may still start work that ends in gossiping a checkpoint proposal: + * {@link getCheckpointProposalReceiveDeadline} less one propagation budget, so a proposal sent at it still reaches + * peers before they stop accepting proposals for the slot. + * + * This is not {@link getAttestationDeadline}, which is a full ethereum slot plus a block duration later and bounds + * when attestations must exist, not when the proposal must have been sent. Work budgeted against the attestation + * deadline can finish after peers have already refused the proposal. + */ + public getCheckpointProposalSendDeadline(slot: SlotNumber): number { + return this.getCheckpointProposalReceiveDeadline(slot) - this.p2pPropagationTime; + } + /** * Selects the next block sub-slot to build for the target slot given the current wall-clock time. * diff --git a/yarn-project/stdlib/src/validators/schemas.ts b/yarn-project/stdlib/src/validators/schemas.ts index 15bc07beb498..6e3dc41171c6 100644 --- a/yarn-project/stdlib/src/validators/schemas.ts +++ b/yarn-project/stdlib/src/validators/schemas.ts @@ -17,6 +17,7 @@ export const ValidatorStatusInSlotSchema = zodFor()( 'checkpoint-valid', 'checkpoint-invalid', 'checkpoint-unvalidated', + 'checkpoint-unverifiable', 'checkpoint-missed', 'blocks-missed', 'attestation-sent', diff --git a/yarn-project/stdlib/src/validators/types.ts b/yarn-project/stdlib/src/validators/types.ts index 4adedf0dab43..27093dada71d 100644 --- a/yarn-project/stdlib/src/validators/types.ts +++ b/yarn-project/stdlib/src/validators/types.ts @@ -11,6 +11,11 @@ export type ValidatorStatusType = 'proposer' | 'attestation'; * - `checkpoint-missed` — block proposals seen but no checkpoint proposal (case 2). * - `checkpoint-unvalidated` — checkpoint proposal seen but local re-execution couldn't * validate (missing txs, timeouts, etc.) (case 3). + * - `checkpoint-unverifiable` — checkpoint proposal seen and content-valid, but this observer could + * not check it against an authority outside itself (an L1 read that + * failed or answered from a view it could not pin). Recorded so the + * slot is not mistaken for one without a proposal, and never counted + * against the proposer: the failure is this node's. * - `checkpoint-invalid` — checkpoint proposal re-executed and rejected as invalid (case 4). * - `checkpoint-valid` — checkpoint proposal re-executed locally as valid (case 5). * - `checkpoint-mined` — checkpoint published on L1 (case 6). @@ -25,6 +30,7 @@ export type ValidatorStatusInSlot = | 'checkpoint-valid' | 'checkpoint-invalid' | 'checkpoint-unvalidated' + | 'checkpoint-unverifiable' | 'checkpoint-missed' | 'blocks-missed' | 'attestation-sent' diff --git a/yarn-project/validator-client/src/duty_budget.test.ts b/yarn-project/validator-client/src/duty_budget.test.ts new file mode 100644 index 000000000000..391d27305673 --- /dev/null +++ b/yarn-project/validator-client/src/duty_budget.test.ts @@ -0,0 +1,61 @@ +import { TestDateProvider } from '@aztec/foundation/timer'; + +import { describe, expect, it } from '@jest/globals'; + +import { DutyBudget, DutyBudgetExpiredError } from './duty_budget.js'; + +describe('DutyBudget', () => { + let dateProvider: TestDateProvider; + + beforeEach(() => { + dateProvider = new TestDateProvider(); + }); + + /** A budget whose deadline is already `ms` in the past. */ + const expiredBudget = (ms = 5_000) => new DutyBudget(new Date(dateProvider.now() - ms), dateProvider); + + const hang = () => new Promise(() => {}); + + it('lets a duty past its deadline finish a read that answers at once', async () => { + await expect(expiredBudget().run('read', () => Promise.resolve('answer'))).resolves.toEqual('answer'); + }); + + // The grace exists so a duty entered past its deadline can still make the one attempt its callers rely on. It + // is one allowance for the whole duty: renewing it per stage would let an expired duty walk through as many + // seconds as it has stages. + it('gives a duty past its deadline one grace allowance, not one per stage', async () => { + const budget = expiredBudget(); + + const firstStage = Date.now(); + await expect(budget.run('first stage', hang)).rejects.toThrow(DutyBudgetExpiredError); + expect(Date.now() - firstStage).toBeGreaterThanOrEqual(500); + + const secondStage = Date.now(); + await expect(budget.run('second stage', hang)).rejects.toThrow(DutyBudgetExpiredError); + expect(Date.now() - secondStage).toBeLessThan(200); + }); + + it('reports the grace as somewhere left to go, and the deadline as gone, past the deadline', () => { + const budget = expiredBudget(); + + expect(budget.expired()).toBe(true); + expect(budget.canContinue()).toBe(true); + }); + + it('stops offering the grace once the duty is stopped', async () => { + const budget = expiredBudget(); + budget.stop('sibling failed'); + + expect(budget.canContinue()).toBe(false); + await expect(budget.run('read', () => Promise.resolve('answer'))).rejects.toThrow(DutyBudgetExpiredError); + }); + + it('bounds a read by what is left of the budget rather than by a fresh full timeout', async () => { + const budget = new DutyBudget(new Date(dateProvider.now() + 300), dateProvider); + + const started = Date.now(); + await expect(budget.run('read', hang)).rejects.toThrow(DutyBudgetExpiredError); + + expect(Date.now() - started).toBeLessThan(1_000); + }); +}); diff --git a/yarn-project/validator-client/src/duty_budget.ts b/yarn-project/validator-client/src/duty_budget.ts new file mode 100644 index 000000000000..4a1d150c2902 --- /dev/null +++ b/yarn-project/validator-client/src/duty_budget.ts @@ -0,0 +1,119 @@ +import type { DateProvider } from '@aztec/foundation/timer'; +import { execWithSignal } from '@aztec/foundation/timer'; + +/** + * How long a duty gets in total once its budget has already run out. The duty still makes the single attempt its + * callers rely on, but bounded, so an unresponsive read cannot hold an expired duty open. Long enough for a local + * store read, short enough that nothing waits on it. + */ +const EXPIRED_BUDGET_GRACE_MS = 1_000; + +/** Thrown when a duty's absolute budget runs out while work it was awaiting is still outstanding. */ +export class DutyBudgetExpiredError extends Error { + constructor( + public readonly what: string, + public readonly deadline: Date, + ) { + super(`Duty budget for ${what} ran out at ${deadline.toISOString()}`); + this.name = 'DutyBudgetExpiredError'; + } +} + +/** + * The one absolute budget a slot's duty runs under, shared by every stage instead of each read starting a fresh + * full timeout of its own. + * + * Two separate problems need solving and a timeout alone solves neither on its own. A read that never settles has + * to stop blocking the duty, which needs a race; and racing does not stop the losing continuation, because + * JavaScript cannot be killed, so anything that continuation would start afterwards has to consult the budget + * first. {@link run} covers the race and hands the attempt the signal; {@link expired} covers everything a caller + * is about to begin. + * + * The budget can also be stopped early, for work whose sibling has already failed: retries that no caller will + * read any more are cancelled rather than left forcing syncs for the rest of the slot. + */ +export class DutyBudget { + private readonly controller = new AbortController(); + + /** Absolute end of the one grace allowance, fixed by the first run that finds the budget already gone. */ + private graceDeadlineMs: number | undefined; + + constructor( + public readonly deadline: Date, + private readonly dateProvider: DateProvider, + ) {} + + /** Milliseconds left before the budget runs out; zero once it has, or once the duty was stopped. */ + public remainingMs(): number { + if (this.controller.signal.aborted) { + return 0; + } + return Math.max(0, this.deadline.getTime() - this.dateProvider.now()); + } + + /** + * Whether the budget is gone, because the deadline passed or because the duty was stopped. This is the hard + * question, the one output nobody would accept any more depends on: a signature produced after it is unusable. + * Work that only needs to finish the attempt already under way asks {@link canContinue} instead. + */ + public expired(): boolean { + return this.remainingMs() === 0; + } + + /** + * Whether the duty may start or commit further work: either it is still within budget, or it is within the + * single grace allowance described on {@link run}. Asking opens that allowance if nothing has opened it yet, + * so a duty entered past its deadline can finish the one attempt its callers rely on and no more. + */ + public canContinue(): boolean { + return this.remainingMs() > 0 || this.remainingGraceMs() > 0; + } + + /** The signal callees should pass on and check; aborted once the budget is gone. */ + public get signal(): AbortSignal { + return this.controller.signal; + } + + /** + * Stops the duty now. Work already in flight is not killed — it settles into a caller that has moved on — but + * every stage that checks the budget stops starting more. + */ + public stop(reason: string): void { + if (!this.controller.signal.aborted) { + this.controller.abort(new DutyBudgetExpiredError(reason, this.deadline)); + } + } + + /** + * Runs `fn` bounded by the budget, throwing {@link DutyBudgetExpiredError} rather than waiting on a read that + * never settles. The attempt itself keeps running, so `fn` should honour the signal it is handed and callers + * must still check {@link expired} before starting anything further. + * + * A budget that has already run out still gets {@link EXPIRED_BUDGET_GRACE_MS}: a duty past its deadline is + * expected to make the one attempt its callers rely on — a proposal whose blocks are already local validates + * without waiting for anything — and the point here is that even that attempt cannot hang. The grace is one + * absolute allowance for the whole duty, anchored by the first run that finds the budget gone and shared by + * every run after it, so a duty past its deadline cannot walk stage by stage through a fresh second each time. + * A duty that was explicitly stopped gets no grace: its result has no reader left. + */ + public async run(what: string, fn: (signal: AbortSignal) => Promise): Promise { + if (this.controller.signal.aborted) { + throw new DutyBudgetExpiredError(what, this.deadline); + } + const remainingMs = this.remainingMs() || this.remainingGraceMs(); + if (remainingMs === 0) { + throw new DutyBudgetExpiredError(what, this.deadline); + } + const signal = AbortSignal.any([this.controller.signal, AbortSignal.timeout(remainingMs)]); + return await execWithSignal(fn, signal, () => new DutyBudgetExpiredError(what, this.deadline)); + } + + /** Milliseconds left of the single grace allowance, opening it on the first call. Zero once the duty is stopped. */ + private remainingGraceMs(): number { + if (this.controller.signal.aborted) { + return 0; + } + this.graceDeadlineMs ??= this.dateProvider.now() + EXPIRED_BUDGET_GRACE_MS; + return Math.max(0, this.graceDeadlineMs - this.dateProvider.now()); + } +} diff --git a/yarn-project/validator-client/src/proposal_handler.test.ts b/yarn-project/validator-client/src/proposal_handler.test.ts index 424b5a337798..731809128c83 100644 --- a/yarn-project/validator-client/src/proposal_handler.test.ts +++ b/yarn-project/validator-client/src/proposal_handler.test.ts @@ -6,6 +6,8 @@ import { MAX_FEE_ASSET_PRICE_MODIFIER_BPS } from '@aztec/ethereum/contracts'; import { BlockNumber, CheckpointNumber, EpochNumber, SlotNumber } from '@aztec/foundation/branded-types'; import { Secp256k1Signer } from '@aztec/foundation/crypto/secp256k1-signer'; import { Fr } from '@aztec/foundation/curves/bn254'; +import { promiseWithResolvers } from '@aztec/foundation/promise'; +import { sleep } from '@aztec/foundation/sleep'; import { TestDateProvider } from '@aztec/foundation/timer'; import { type FieldsOf, unfreeze } from '@aztec/foundation/types'; import type { P2P } from '@aztec/p2p'; @@ -398,6 +400,50 @@ describe('ProposalHandler checkpoint validation', () => { }); }); + // retryUntil only consults its deadline once an attempt returns, so an attempt that never settles never + // reaches it. The duty budget races the whole loop, so the caller settles instead of being held open. Running + // out of time is this node giving up rather than a verdict, so it is reported as its own reason. + it('settles within the duty budget when a readiness read never returns', async () => { + blockSource.getBlocksForSlot.mockImplementation(() => new Promise(() => {})); + // attestation_deadline(slot=1) is 40s; leave a short but nonzero budget. + dateProvider.setTime(39_800); + + const result = await handler.handleCheckpointProposal(await makeProposal(), proposalInfo); + + expect(result).toEqual({ isValid: false, reason: 'validation_deadline_expired' }); + }); + + it('settles within the grace when a readiness read never returns past the deadline', async () => { + blockSource.getBlocksForSlot.mockImplementation(() => new Promise(() => {})); + dateProvider.setTime(41_000); + + const result = await handler.handleCheckpointProposal(await makeProposal(), proposalInfo); + + expect(result).toEqual({ isValid: false, reason: 'validation_deadline_expired' }); + }); + + // The cached-valid path re-reads the checkpoint's blocks before reusing a verdict; that read is a store + // call like any other and must not be able to hold the duty open either. + it('settles when the cached-verdict re-check never returns', async () => { + const archiveRoot = Fr.random(); + blockSource.getBlocksForSlot.mockResolvedValue(makeSlotBlocks([archiveRoot])); + blockSource.getCheckpointData.mockResolvedValue({ checkpointNumber: CheckpointNumber(1) } as CheckpointData); + const proposal = await makeProposal({ archiveRoot }); + + // First call caches a verdict; it is not valid, so make one that is by re-running against a valid result. + const cached = { isValid: true as const, checkpointNumber: CheckpointNumber(1) }; + (handler as unknown as { lastCheckpointValidationResult: unknown }).lastCheckpointValidationResult = { + payloadHash: proposal.getPayloadHash(), + result: cached, + }; + blockSource.getBlockData.mockImplementation(() => new Promise(() => {})); + dateProvider.setTime(39_800); + + const result = await handler.handleCheckpointProposal(proposal, proposalInfo); + + expect(result).toEqual({ isValid: false, reason: 'validation_deadline_expired' }); + }); + // With <1s remaining the old Math.floor(...) timeout collapsed to 0 ("never time out"). The fix uses // a strictly-positive fractional timeout, so the wait still terminates instead of hanging. it('terminates with a fractional sub-second timeout when <1s remains before the deadline', async () => { @@ -407,7 +453,81 @@ describe('ProposalHandler checkpoint validation', () => { dateProvider.setTime(39_700); const result = await handler.handleCheckpointProposal(await makeProposal(), proposalInfo); - expect(result).toEqual({ isValid: false, reason: 'last_block_not_found' }); + expect(result).toEqual({ isValid: false, reason: 'validation_deadline_expired' }); + }); + + // The snapshot wait is only the first stage. Every read after it is another chance for the duty to hang, so + // the budget has to cover the whole method rather than the wait it started with. + it('settles when a read after the snapshot never returns', async () => { + const archiveRoot = Fr.random(); + blockSource.getBlocksForSlot.mockResolvedValue(makeSlotBlocks([archiveRoot])); + blockSource.getCheckpointData.mockImplementation(() => new Promise(() => {})); + dateProvider.setTime(39_800); + + const result = await handler.handleCheckpointProposal(await makeProposal({ archiveRoot }), proposalInfo); + + expect(result).toEqual({ isValid: false, reason: 'validation_deadline_expired' }); + }); + + // Running out of time is this node giving up, not something it learned about the proposal. It caches no + // verdict, and records the one outcome that is charged to nobody — recording nothing would leave the sentinel + // to fall back to `checkpoint-missed`, which is counted against the proposer. + it('records the slot as unverifiable, and caches nothing, when the duty budget runs out', async () => { + blockSource.getBlocksForSlot.mockImplementation(() => new Promise(() => {})); + dateProvider.setTime(39_800); + const proposal = await makeProposal(); + + expect(await handler.handleCheckpointProposal(proposal, proposalInfo)).toEqual({ + isValid: false, + reason: 'validation_deadline_expired', + }); + expect(reexecutionTracker.getOutcomeForSlot(SlotNumber(1))).toEqual('unverifiable'); + + // The next caller revalidates from scratch instead of inheriting the abandoned duty's non-verdict. + const archiveRoot = proposal.archive; + blockSource.getBlocksForSlot.mockResolvedValue(makeSlotBlocks([archiveRoot])); + blockSource.getCheckpointData.mockResolvedValue({ checkpointNumber: CheckpointNumber(1) } as CheckpointData); + dateProvider.setTime(20_000); + + expect(await handler.handleCheckpointProposal(proposal, proposalInfo)).toEqual({ + isValid: false, + reason: 'checkpoint_already_published', + checkpointNumber: CheckpointNumber(1), + }); + }); + + // The recording inside `handleCheckpointProposal` never runs for an expiry in a stage before it — and on a + // node that is not a validator the all-nodes callback is the only handler there is, so the outermost + // boundary has to record too, or the slot reads as one the proposer skipped. + it('records the slot as unverifiable when the duty runs out before validation is reached', async () => { + const proposal = await makeProposal(); + const p2p = mock(); + let checkpointHandler: ((proposal: any, sender: any) => Promise) | undefined; + p2p.registerAllNodesCheckpointProposalHandler.mockImplementation(handler => { + checkpointHandler = handler; + }); + epochCache.isEscapeHatchOpenAtSlot.mockImplementation(() => new Promise(() => {})); + dateProvider.setTime(39_800); + + handler.register(p2p, true); + await checkpointHandler!(proposal, {} as any); + + expect(reexecutionTracker.getOutcomeForSlot(SlotNumber(1))).toEqual('unverifiable'); + }); + + // A duty that gave up learned nothing that could revise what the slot already says, in either direction. + it('leaves an outcome the slot already has alone when a later duty runs out', async () => { + const proposal = await makeProposal(); + reexecutionTracker.recordOutcome(SlotNumber(1), proposal.archive, 'valid', CheckpointNumber(1)); + + blockSource.getBlocksForSlot.mockImplementation(() => new Promise(() => {})); + dateProvider.setTime(39_800); + + expect(await handler.handleCheckpointProposal(proposal, proposalInfo)).toEqual({ + isValid: false, + reason: 'validation_deadline_expired', + }); + expect(reexecutionTracker.getOutcomeForSlot(SlotNumber(1))).toEqual('valid'); }); }); @@ -441,6 +561,41 @@ describe('ProposalHandler checkpoint validation', () => { expect(metrics.recordCheckpointProposalToPipelinedStateDuration).toHaveBeenCalledWith(expect.any(Number)); }); + // Recording a proposed checkpoint is a mutation the next slot builds on top of. Its own block read can settle + // long after the duty was answered, so the insertion has to be barred by the budget, not just preceded by it. + it('does not record a proposed checkpoint from a block read that lands after the budget is gone', async () => { + const proposal = await makeProposal(); + const p2p = mock(); + let checkpointHandler: ((proposal: any, sender: any) => Promise) | undefined; + p2p.registerAllNodesCheckpointProposalHandler.mockImplementation(handler => { + checkpointHandler = handler; + }); + + const archiver = mock>(); + archiver.addProposedCheckpoint.mockResolvedValue(undefined); + + const blockRead = promiseWithResolvers(); + blockSource.getBlockData.mockReturnValue(blockRead.promise); + + jest + .spyOn(handler, 'handleCheckpointProposal') + .mockResolvedValue({ isValid: true, checkpointNumber: CheckpointNumber(3) }); + + // 200ms of budget left, and a block read that answers only after it is spent. + dateProvider.setTime(39_800); + handler.register(p2p, true, archiver); + await checkpointHandler!(proposal, {} as any); + + blockRead.resolve({ + checkpointNumber: CheckpointNumber(3), + header: { getBlockNumber: () => 9 }, + indexWithinCheckpoint: 2, + } as BlockData); + await sleep(50); + + expect(archiver.addProposedCheckpoint).not.toHaveBeenCalled(); + }); + // The checkpoint-validation block-sync deadline is the L1 publish deadline (12s/one Ethereum // slot before the last L1 block of the target slot), which is later than the target-slot start // used for block re-execution. With slotDuration=24, ethereumSlotDuration=4 and proposal slot=1: @@ -955,6 +1110,30 @@ describe('ProposalHandler checkpoint validation', () => { expect(mockDispose).toHaveBeenCalled(); }); + // A stage that settles after the deadline must not carry the duty into the next one: the fork arriving late + // cannot be turned into a checkpoint rebuild nobody is waiting for. + it('starts no reconstruction stage when the world-state fork settles past the deadline', async () => { + setupDeepValidationMocks({ header: makeHeader() }); + const fork = promiseWithResolvers(); + checkpointsBuilder.getFork.mockReturnValue(fork.promise as any); + + // 200ms of budget left, and a fork that answers only after it is spent. + dateProvider.setTime(39_800); + const proposal = await makeProposal({ archiveRoot, checkpointHeader: makeHeader() }); + const validation = handler.handleCheckpointProposal(proposal, proposalInfo); + await sleep(400); + fork.resolve({ + [Symbol.asyncDispose]: mockDispose, + getTreeInfo: () => Promise.resolve({ root: Fr.ZERO.toBuffer() }), + }); + + expect(await validation).toEqual({ isValid: false, reason: 'validation_deadline_expired' }); + expect(checkpointsBuilder.openCheckpoint).not.toHaveBeenCalled(); + // The caller never reached the `await using` that would have closed this fork, so the abandoned read has + // to close it: otherwise every duty that gives up here leaks a world-state fork. + expect(mockDispose).toHaveBeenCalled(); + }); + it('disposes fork even when validation fails', async () => { setupDeepValidationMocks({ header: CheckpointHeader.empty() }); @@ -1399,6 +1578,31 @@ describe('ProposalHandler checkpoint validation', () => { expect(txProvider.getTxsForBlockProposal).not.toHaveBeenCalled(); }); + // The bundle read and tx collection are started together. Once collection has failed there is no proposal + // left to validate, so the bundle's retries must stop rather than keep forcing archiver syncs for the rest + // of the slot; a catch handler on the abandoned promise only silences its rejection. + it('stops the bundle retries when tx collection rejects', async () => { + const { proposal, blockHandler, txProvider } = await setupStreamingProposal(signedRef, { + nowMs: BEFORE_DEADLINE_MS, + }); + // The metadata check passes so both siblings start; only the bundle read keeps missing. + l1ToL2MessageSource.getMessagePosition.mockImplementation(count => + Promise.resolve(count === 0n ? position(0n, Fr.ZERO) : count === 2n ? position(2n, prefixHash) : undefined), + ); + l1ToL2MessageSource.getL1ToL2MessageRange.mockRejectedValue(new Error('range is not fully synced')); + txProvider.getTxsForBlockProposal.mockRejectedValue(new Error('tx provider down')); + + const startMs = Date.now(); + await expect(blockHandler.handleBlockProposal(proposal, {} as any, true)).rejects.toThrow('tx provider down'); + const elapsedMs = Date.now() - startMs; + + // It gave up as soon as its sibling failed rather than running out the remaining budget. + expect(elapsedMs).toBeLessThan(WAIT_BUDGET_MS); + const syncsAtGiveUp = blockSource.syncImmediate.mock.calls.length; + await sleep(3 * WAIT_INTERVAL_MS); + expect(blockSource.syncImmediate).toHaveBeenCalledTimes(syncsAtGiveUp); + }); + it('rejects immediately without syncing when the attestation deadline has already passed', async () => { const { proposal, blockHandler } = await setupStreamingProposal(signedRef, { nowMs: PAST_DEADLINE_MS }); mockLocalView(undefined); diff --git a/yarn-project/validator-client/src/proposal_handler.ts b/yarn-project/validator-client/src/proposal_handler.ts index 718e48c9e668..eadd3414ebc0 100644 --- a/yarn-project/validator-client/src/proposal_handler.ts +++ b/yarn-project/validator-client/src/proposal_handler.ts @@ -62,6 +62,7 @@ import { import { type TelemetryClient, type Tracer, getTelemetryClient } from '@aztec/telemetry-client'; import type { FullNodeCheckpointsBuilder } from './checkpoint_builder.js'; +import { DutyBudget, DutyBudgetExpiredError } from './duty_budget.js'; import type { ValidatorMetrics } from './metrics.js'; import { type StreamingBlockCheckReason, @@ -131,6 +132,9 @@ export type CheckpointProposalValidationFailureReason = // consumed. Local-view outcomes, never proposer misconduct. | 'inbox_prefix_unavailable' | 'inbox_prefix_mismatch' + // The slot's duty budget ran out before validation reached a verdict. Says nothing about the proposal, so it is + // neither cached for the next caller nor recorded against the proposer. + | 'validation_deadline_expired' | 'checkpoint_validation_failed'; /** The streaming-Inbox reasons a checkpoint proposal can fail on; both are retried through a bounded local sync. */ @@ -190,6 +194,9 @@ const CHECKPOINT_VALIDATION_REASON_TO_OUTCOME: Record< // Not proposer misbehavior: this node's Inbox view could not confirm the consumed prefix, or disagrees with it. inbox_prefix_unavailable: 'unvalidated', inbox_prefix_mismatch: 'unvalidated', + // This node ran out of time to look; it observed nothing about the proposer. Recorded by the duty-expiry path + // itself, which also refuses to overwrite an outcome the slot already has. + validation_deadline_expired: 'unverifiable', checkpoint_validation_failed: 'invalid', }; @@ -267,6 +274,7 @@ export const SLASHABLE_CHECKPOINT_PROPOSAL_VALIDATION_RESULT: Record< // A reorg / divergent local chain, not a proposer offense (mirrors the block path's initial_state_mismatch). ['initial_archive_mismatch']: false, ['checkpoint_already_published']: false, + ['validation_deadline_expired']: false, }; /** @@ -442,64 +450,32 @@ export class ProposalHandler { proposal: ValidatedCheckpointProposalCore, _sender: PeerId, ): Promise => { + // This callback runs before the validator's own, and does the reads and waits the attestation later rests on, + // so the duty's absolute budget starts here rather than around the attestation alone. A wrapper further in + // would leave this work — and every non-validator node, which only ever runs this callback — unbounded. + const budget = new DutyBudget(this.getReexecutionDeadline(proposal.slotNumber), this.dateProvider); try { - const pipeliningTimer = new Timer(); - const proposalInfo: LogData = { - slot: proposal.slotNumber, - archive: proposal.archive.toString(), - proposer: proposal.getSender()?.toString(), - }; - - if (this.config.skipCheckpointProposalValidation) { - this.log.warn(`Skipping checkpoint proposal validation for slot ${proposal.slotNumber}`, proposalInfo); - return undefined; - } - - if (await this.epochCache.isEscapeHatchOpenAtSlot(proposal.slotNumber)) { - this.log.warn( - `Escape hatch open for slot ${proposal.slotNumber}, skipping checkpoint proposal validation`, - proposalInfo, - ); - return undefined; - } - - // A proposal is "own" when it was signed by a validator key this node also owns. The true local - // proposer already built, validated, and stored this checkpoint before broadcasting, so a matching - // proposed checkpoint is already in its archiver — skip the redundant re-validation. An HA peer that - // shares the proposer's keys sees the same "own" proposal over gossip but never built it, so it has - // nothing stored; it falls through to the normal validate-and-persist path below to hydrate the - // proposed-checkpoint metadata it needs to build the next slot on top of this checkpoint. - const proposer = proposal.getSender(); - const ownAddresses = this.getOwnValidatorAddresses?.(); - const isOwnProposal = proposer && ownAddresses?.some(addr => addr === proposer.toString()); - - if (isOwnProposal) { - const existing = await this.archiver?.getProposedCheckpointData({ slot: proposal.slotNumber }); - if (existing?.archive.root.equals(proposal.archive)) { - this.log.debug(`Skipping sync for existing own checkpoint proposal at slot ${proposal.slotNumber}`); - return undefined; - } - } - - const result = await this.handleCheckpointProposal(proposal, proposalInfo); - if (!result.isValid) { - // Track invalid checkpoint proposals so offense observers (the attested-invalid-proposal watcher) - // work on non-validator nodes too. This handler runs for all nodes; validators also mark via the - // failure callback below (idempotent). - if (SLASHABLE_CHECKPOINT_PROPOSAL_VALIDATION_RESULT[result.reason]) { - this.markInvalidProposalSlot(proposal.slotNumber); - } - await this.checkpointProposalValidationFailureCallback?.(proposal, result, proposalInfo); - } else if (this.archiver) { - const set = await this.setProposedCheckpoint(proposal); - if (set) { - this.metrics?.recordCheckpointProposalToPipelinedStateDuration(pipeliningTimer.ms()); - } - } + return await budget.run(`checkpoint proposal validation for slot ${proposal.slotNumber}`, () => + this.handleAllNodesCheckpointProposal(proposal, budget), + ); } catch (err) { - this.log.warn(`Error handling checkpoint proposal for slot ${proposal.slotNumber}`, { err }); + if (err instanceof DutyBudgetExpiredError) { + this.log.warn(`Checkpoint proposal handling for slot ${proposal.slotNumber} ran out of duty budget`, { + slot: proposal.slotNumber, + deadline: budget.deadline.toISOString(), + }); + // An expiry in a stage before validation is reached — the escape-hatch or own-proposal read — never + // passes through the recording inside. This is the outermost boundary, and on a node that is not a + // validator it is the only one, so the neutral record is made here too. + this.recordUncheckedProposal(proposal); + } else { + this.log.warn(`Error handling checkpoint proposal for slot ${proposal.slotNumber}`, { err }); + } + return undefined; + } finally { + // Anything still retrying belongs to a duty nobody will read from any more. + budget.stop(`checkpoint proposal validation for slot ${proposal.slotNumber}`); } - return undefined; }; p2pClient.registerAllNodesCheckpointProposalHandler(checkpointHandler); @@ -507,6 +483,76 @@ export class ProposalHandler { return this; } + /** + * The body of the all-nodes checkpoint proposal callback: validates, caches and (for pipelining) records the + * proposal as this node's proposed checkpoint. Runs inside the slot's duty budget, which every stage consults + * before starting further work. + */ + private async handleAllNodesCheckpointProposal( + proposal: ValidatedCheckpointProposalCore, + budget: DutyBudget, + ): Promise { + const pipeliningTimer = new Timer(); + const proposalInfo: LogData = { + slot: proposal.slotNumber, + archive: proposal.archive.toString(), + proposer: proposal.getSender()?.toString(), + }; + + if (this.config.skipCheckpointProposalValidation) { + this.log.warn(`Skipping checkpoint proposal validation for slot ${proposal.slotNumber}`, proposalInfo); + return undefined; + } + + const escapeHatchOpen = await budget.run(`escape hatch check for slot ${proposal.slotNumber}`, () => + this.epochCache.isEscapeHatchOpenAtSlot(proposal.slotNumber), + ); + if (escapeHatchOpen) { + this.log.warn( + `Escape hatch open for slot ${proposal.slotNumber}, skipping checkpoint proposal validation`, + proposalInfo, + ); + return undefined; + } + + // A proposal is "own" when it was signed by a validator key this node also owns. The true local + // proposer already built, validated, and stored this checkpoint before broadcasting, so a matching + // proposed checkpoint is already in its archiver — skip the redundant re-validation. An HA peer that + // shares the proposer's keys sees the same "own" proposal over gossip but never built it, so it has + // nothing stored; it falls through to the normal validate-and-persist path below to hydrate the + // proposed-checkpoint metadata it needs to build the next slot on top of this checkpoint. + const proposer = proposal.getSender(); + const ownAddresses = this.getOwnValidatorAddresses?.(); + const isOwnProposal = proposer && ownAddresses?.some(addr => addr === proposer.toString()); + + if (isOwnProposal) { + const existing = await budget.run(`own proposed checkpoint read for slot ${proposal.slotNumber}`, () => + Promise.resolve(this.archiver?.getProposedCheckpointData({ slot: proposal.slotNumber })), + ); + if (existing?.archive.root.equals(proposal.archive)) { + this.log.debug(`Skipping sync for existing own checkpoint proposal at slot ${proposal.slotNumber}`); + return undefined; + } + } + + const result = await this.handleCheckpointProposal(proposal, proposalInfo, budget); + if (!result.isValid) { + // Track invalid checkpoint proposals so offense observers (the attested-invalid-proposal watcher) + // work on non-validator nodes too. This handler runs for all nodes; validators also mark via the + // failure callback below (idempotent). + if (SLASHABLE_CHECKPOINT_PROPOSAL_VALIDATION_RESULT[result.reason]) { + this.markInvalidProposalSlot(proposal.slotNumber); + } + await this.checkpointProposalValidationFailureCallback?.(proposal, result, proposalInfo); + } else if (this.archiver) { + const set = await this.setProposedCheckpoint(proposal, budget); + if (set) { + this.metrics?.recordCheckpointProposalToPipelinedStateDuration(pipeliningTimer.ms()); + } + } + return undefined; + } + /** * Processes a block proposal: collects its txs and, if requested, re-executes them to check the resulting * block against the proposal. Expects the proposal to have already passed p2p ingress validation (signature @@ -625,11 +671,20 @@ export class ProposalHandler { // Collect txs from the proposal. We start doing this as early as possible, // and we do it even if we don't plan to re-execute the txs, so that we have them if another node needs them. // The block's message bundle is an independent read, so derive it concurrently with the collection. + const bundleStop = new AbortController(); const txsPromise = this.collectProposalTxs(proposal, blockNumber, proposalSender, proposalInfo); - const bundlePromise = this.awaitStreamingBlockBundle(proposal, blockNumber, parentBlock, proposalInfo); + const bundlePromise = this.awaitStreamingBlockBundle( + proposal, + blockNumber, + parentBlock, + proposalInfo, + bundleStop.signal, + ); // Promise.all settles on the first rejection, so without a handler of its own the loser's later rejection - // would surface as an unhandled rejection. Awaiting below still observes whichever rejected first. - txsPromise.catch(() => {}); + // would surface as an unhandled rejection. Awaiting below still observes whichever rejected first. A handler + // silences the rejection but does not stop the work: once tx collection has failed there is no proposal left + // to validate, so the bundle's retries are cancelled instead of forcing archiver syncs for the rest of the slot. + txsPromise.catch(() => bundleStop.abort()); bundlePromise.catch(() => {}); const [collected, bundle] = await Promise.all([txsPromise, bundlePromise]); if (collected === 'invalid_embedded_txs') { @@ -795,19 +850,31 @@ export class ProposalHandler { * passed on entry (nothing is forced in that case) or when it passes while waiting; anything other than the * timeout propagates. Callers own their own logging and whatever they fall back to on `undefined`, and check * the deadline themselves when they need to tell "no budget on entry" apart from "timed out while waiting". + * + * `stop` cancels the loop for callers whose result nobody will read any more — a sibling read that already + * failed, or a duty whose budget is gone. Attaching a rejection handler to an abandoned promise keeps the + * process quiet but does not stop it forcing an archiver sync every half second for the rest of the slot. The + * shared archiver run itself is never cancelled, only this caller's retries. */ private async awaitLocalSync( slotNumber: SlotNumber, what: string, resolve: () => Promise, + { budget, stop = budget?.signal }: { budget?: DutyBudget; stop?: AbortSignal } = {}, ): Promise { const deadline = this.getReexecutionDeadline(slotNumber); - if (deadline.getTime() - this.dateProvider.now() <= 0) { + if (deadline.getTime() - this.dateProvider.now() <= 0 || stop?.aborted) { return undefined; } - try { - return await retryUntil( + // `retryUntil` consults its deadline only after an attempt returns, so a sync or a read that never settles + // never reaches it. A caller that owns a duty budget races the whole loop against it as well; one that does + // not (the block-proposal paths) keeps the deadline-between-attempts behaviour it always had. + const loop = () => + retryUntil( async () => { + if (stop?.aborted) { + throw new TimeoutError(`Stopped waiting for ${what}`); + } await this.blockSource.syncImmediate(); return await resolve(); }, @@ -815,6 +882,10 @@ export class ProposalHandler { { deadline, dateProvider: this.dateProvider }, 0.5, ); + try { + // A budget expiry propagates: it is the duty giving up, which the caller answers with its own reason + // rather than as a local-view verdict against the proposer. + return budget ? await budget.run(what, loop) : await loop(); } catch (err) { if (err instanceof TimeoutError) { return undefined; @@ -1027,7 +1098,7 @@ export class ProposalHandler { * checkpoint can land on L1 in the target slot; all nodes agree on it. Loosened from the previous * next-wall-clock-slot-boundary bound (see the timetable spec / refactor notes). */ - private getReexecutionDeadline(slotNumber: SlotNumber): Date { + public getReexecutionDeadline(slotNumber: SlotNumber): Date { return new Date(this.timetable.getAttestationDeadline(slotNumber) * 1000); } @@ -1101,6 +1172,7 @@ export class ProposalHandler { blockNumber: BlockNumber, parentBlock: 'genesis' | BlockData, proposalInfo: LogData, + stop?: AbortSignal, ): Promise { const readBundle = async (): Promise => { const metadata = await this.checkStreamingBlockMetadata(proposal, blockNumber, parentBlock); @@ -1118,14 +1190,21 @@ export class ProposalHandler { ...proposalInfo, }); const timer = new Timer(); - const resolved = await this.awaitLocalSync(slotNumber, `inbox bundle for block ${blockNumber}`, async () => { - const result = await readBundle(); - return !result.accepted && isRetryableStreamingBlockCheckReason(result.reason) ? undefined : result; - }); + const resolved = await this.awaitLocalSync( + slotNumber, + `inbox bundle for block ${blockNumber}`, + async () => { + const result = await readBundle(); + return !result.accepted && isRetryableStreamingBlockCheckReason(result.reason) ? undefined : result; + }, + { stop }, + ); if (resolved === undefined) { this.log.warn(`Timed out reading a consistent Inbox bundle, rejecting proposal`, { reason: 'inbox_prefix_sync_timeout', firstReason: first.reason, + // Set only when the message source failed for a reason the checks did not anticipate, rather than sync lag. + error: first.error, slot: slotNumber, waitedMs: timer.ms(), ...proposalInfo, @@ -1340,10 +1419,12 @@ export class ProposalHandler { lastBlockTotal: bigint, checkpointInboxRollingHash: Fr, proposalInfo: LogData, + budget?: DutyBudget, ): Promise<{ accepted: true; messages: Fr[] } | { accepted: false; reason: CheckpointInboxPrefixReason }> { const read = () => this.readCheckpointConsumedMessages(checkpointStartTotal, lastBlockTotal, checkpointInboxRollingHash); - const first = await read(); + const what = `inbox prefix for checkpoint at slot ${slot}`; + const first = budget ? await budget.run(what, read) : await read(); if (first.accepted) { return first; } @@ -1353,10 +1434,15 @@ export class ProposalHandler { lastBlockTotal, ...proposalInfo, }); - const resolved = await this.awaitLocalSync(slot, `inbox prefix for checkpoint at slot ${slot}`, async () => { - const result = await read(); - return result.accepted ? result : undefined; - }); + const resolved = await this.awaitLocalSync( + slot, + what, + async () => { + const result = await read(); + return result.accepted ? result : undefined; + }, + { budget }, + ); if (resolved === undefined) { this.log.warn(`Timed out waiting for the checkpoint's consumed Inbox prefix to sync, refusing to attest`, { reason: 'inbox_prefix_sync_timeout', @@ -1508,6 +1594,45 @@ export class ProposalHandler { async handleCheckpointProposal( proposal: ValidatedCheckpointProposalCore, proposalInfo: LogData, + budget: DutyBudget = new DutyBudget(this.getReexecutionDeadline(proposal.slotNumber), this.dateProvider), + ): Promise { + try { + return await this.validateAndRecordCheckpointProposal(proposal, proposalInfo, budget); + } catch (err) { + if (!(err instanceof DutyBudgetExpiredError)) { + throw err; + } + // Every stage runs inside the budget, so an expiry anywhere lands here with nothing cached: the duty + // stopped looking, which is not a verdict on the proposal and must not be reused as one. + this.log.warn(`Ran out of duty budget validating the checkpoint proposal for slot ${proposal.slotNumber}`, { + ...proposalInfo, + deadline: budget.deadline.toISOString(), + }); + this.recordUncheckedProposal(proposal); + return { isValid: false, reason: 'validation_deadline_expired' }; + } + } + + /** + * Records that a proposal was seen and could not be checked, for a duty that ran out of time. + * + * Something does have to be recorded. With no record and no checkpoint on L1 the sentinel reads the slot as one + * the proposer never proposed in, which is counted against it — so silence here would blame the proposer for + * this node's clock running out. `unverifiable` says a proposal was seen and could not be checked, and is + * counted against nobody. It never revises an outcome the slot already has: a duty that gave up learned nothing + * that could. + */ + private recordUncheckedProposal(proposal: CheckpointProposalCore): void { + if (this.reexecutionTracker.getOutcomeForSlot(proposal.slotNumber) === undefined) { + this.reexecutionTracker.recordOutcome(proposal.slotNumber, proposal.archive, 'unverifiable'); + } + } + + /** {@link handleCheckpointProposal}'s body, outside its duty-budget handling. */ + private async validateAndRecordCheckpointProposal( + proposal: ValidatedCheckpointProposalCore, + proposalInfo: LogData, + budget: DutyBudget, ): Promise { const slot = proposal.slotNumber; const payloadHash = proposal.getPayloadHash(); @@ -1516,9 +1641,15 @@ export class ProposalHandler { // on blocks this node holds locally, and p2p makes two calls for one proposal (the all-nodes validation, then // the attestation), so an archiver rollback in between can prune those blocks. Re-check that the checkpoint's // last block is still local before reusing a valid verdict, or the attestation outlives what it was based on. + // That re-check is a store read like any other, so it runs inside the duty budget rather than unbounded. if (this.lastCheckpointValidationResult && this.lastCheckpointValidationResult.payloadHash === payloadHash) { const cached = this.lastCheckpointValidationResult.result; - if (!cached.isValid || (await this.blockSource.getBlockData({ archive: proposal.archive })) !== undefined) { + const blocksStillLocal = + cached.isValid && + (await budget.run(`cached checkpoint verdict re-check for slot ${slot}`, () => + this.blockSource.getBlockData({ archive: proposal.archive }), + )) !== undefined; + if (!cached.isValid || blocksStillLocal) { this.log.debug(`Returning cached validation result for checkpoint proposal at slot ${slot}`, proposalInfo); return cached; } @@ -1536,7 +1667,7 @@ export class ProposalHandler { ); result = { isValid: false, reason: 'invalid_fee_asset_price_modifier' }; } else { - result = await this.validateCheckpointProposal(proposal, proposalInfo); + result = await this.validateCheckpointProposal(proposal, proposalInfo, budget); } this.lastCheckpointValidationResult = { payloadHash, result }; @@ -1547,15 +1678,21 @@ export class ProposalHandler { this.reexecutionTracker.recordOutcome(slot, proposal.archive, outcome, result.checkpointNumber); } - // Drop tracker entries for checkpoints that have reached L1 finality. + // Drop tracker entries for checkpoints that have reached L1 finality. This is bookkeeping the verdict does not + // depend on, so it runs inside the budget and is skipped once the budget is gone rather than holding a caller + // that has already been answered on a store read. try { - const tips = await this.blockSource.getL2Tips(); + const tips = await budget.run(`reexecution tracker prune for slot ${slot}`, () => this.blockSource.getL2Tips()); const finalizedCheckpointNumber = tips.finalized.checkpoint.number; if (finalizedCheckpointNumber > 0) { this.reexecutionTracker.removeBefore(CheckpointNumber(finalizedCheckpointNumber + 1)); } } catch (err) { - this.log.error(`Error pruning reexecution tracker`, err, proposalInfo); + if (err instanceof DutyBudgetExpiredError) { + this.log.debug(`Skipped pruning the reexecution tracker for slot ${slot}: the duty budget is gone`); + } else { + this.log.error(`Error pruning reexecution tracker`, err, proposalInfo); + } } // Upload blobs to filestore if validation passed (fire and forget) @@ -1573,6 +1710,7 @@ export class ProposalHandler { async validateCheckpointProposal( proposal: CheckpointProposalCore, proposalInfo: LogData, + budget: DutyBudget = new DutyBudget(this.getReexecutionDeadline(proposal.slotNumber), this.dateProvider), ): Promise { const slot = proposal.slotNumber; @@ -1591,16 +1729,25 @@ export class ProposalHandler { // out after a single attempt instead of looping (the immediate-timeout semantics of the deadline overload). let snapshot: CheckpointBlocksSnapshot | undefined; try { - snapshot = await retryUntil( - async () => { - await this.blockSource.syncImmediate(); - return await this.readCheckpointBlocksSnapshot(slot, proposal.archive); - }, - `waiting for block with archive ${proposal.archive.toString()} for slot ${slot}`, - { deadline, dateProvider: this.dateProvider }, - 0.5, + // `retryUntil` only checks its own deadline after an attempt returns, so an attempt that never settles never + // reaches it. Race the whole loop against the budget as well, so a hanging read cannot hold the duty open. + snapshot = await budget.run(`checkpoint blocks snapshot for slot ${slot}`, () => + retryUntil( + async () => { + await this.blockSource.syncImmediate(); + return await this.readCheckpointBlocksSnapshot(slot, proposal.archive); + }, + `waiting for block with archive ${proposal.archive.toString()} for slot ${slot}`, + { deadline, dateProvider: this.dateProvider }, + 0.5, + ), ); } catch (err) { + // A budget expiry is the duty giving up, not a verdict on the proposal: it propagates to the caller, which + // answers with `validation_deadline_expired`, which is charged to nobody. + if (err instanceof DutyBudgetExpiredError) { + throw err; + } if (err instanceof TimeoutError) { this.log.warn(`Timed out waiting for block with archive matching checkpoint proposal`, proposalInfo); return { isValid: false, reason: 'last_block_not_found' }; @@ -1617,7 +1764,9 @@ export class ProposalHandler { const lastBlock = blocks[lastBlockIndex]; // Refuse to attest if the block's enclosing checkpoint has already been published to L1. - const existingCheckpoint = await this.blockSource.getCheckpointData({ number: lastBlock.checkpointNumber }); + const existingCheckpoint = await budget.run(`published checkpoint check for slot ${slot}`, () => + this.blockSource.getCheckpointData({ number: lastBlock.checkpointNumber }), + ); if (existingCheckpoint) { this.log.warn(`Refusing to attest to checkpoint proposal whose checkpoint is already on L1`, { ...proposalInfo, @@ -1679,7 +1828,9 @@ export class ProposalHandler { // The checkpoint's Inbox consumption starts at the leaf count of the block before its first block. Without that // block the consumed bundle cannot be derived; an empty bundle would make a valid proposal fail its rolling-hash // recomputation and be classified as a proposer offense, so a missing parent is a local fetch failure instead. - const checkpointStartTotal = await this.getPreBlockConsumedTotal(firstBlock.number); + const checkpointStartTotal = await budget.run(`checkpoint start position read for slot ${slot}`, () => + this.getPreBlockConsumedTotal(firstBlock.number), + ); if (checkpointStartTotal === undefined) { this.log.warn(`Block before checkpoint proposal's first block ${firstBlock.number} is unavailable locally`, { ...proposalInfo, @@ -1699,6 +1850,7 @@ export class ProposalHandler { this.blockLeafCount(blocks[blocks.length - 1]), proposal.checkpointHeader.inboxRollingHash, proposalInfo, + budget, ); if (!consumed.accepted) { this.log.warn(`Streaming Inbox checkpoint content check failed, refusing to attest`, { @@ -1713,20 +1865,24 @@ export class ProposalHandler { // Collect the out hashes of all the checkpoints before this one in the same epoch. // See note on the analogous block-proposal site: the helper handles pipelining lag. const epoch = getEpochAtSlot(slot, this.epochCache.getL1Constants()); - const previousCheckpointOutHashes = await getPreviousCheckpointOutHashes({ - blockSource: this.blockSource, - epoch, - checkpointNumber, - l1Constants: this.epochCache.getL1Constants(), - pipeliningEnabled: true, - log: this.log, - }); + const previousCheckpointOutHashes = await budget.run(`previous checkpoint out hashes for slot ${slot}`, () => + getPreviousCheckpointOutHashes({ + blockSource: this.blockSource, + epoch, + checkpointNumber, + l1Constants: this.epochCache.getL1Constants(), + pipeliningEnabled: true, + log: this.log, + }), + ); - const previousInboxRollingHash = await getPreviousCheckpointInboxRollingHash({ - blockSource: this.blockSource, - checkpointNumber, - log: this.log, - }); + const previousInboxRollingHash = await budget.run(`previous checkpoint Inbox position for slot ${slot}`, () => + getPreviousCheckpointInboxRollingHash({ + blockSource: this.blockSource, + checkpointNumber, + log: this.log, + }), + ); // Fork world state at the block before the first block. getFork syncs world state to the parent block // first (see its doc): the block source (archiver) can already hold the block while world state still @@ -1737,9 +1893,22 @@ export class ProposalHandler { const parentBlockNumber = BlockNumber(firstBlock.number - 1); let forkResult: MerkleTreeWriteOperations; try { - const parentBlockHash = (await this.blockSource.getBlockData({ number: parentBlockNumber }))?.blockHash; - forkResult = await this.checkpointsBuilder.getFork(parentBlockNumber, parentBlockHash); + const what = `world state fork at block ${parentBlockNumber} for slot ${slot}`; + forkResult = await budget.run(what, async signal => { + const parentBlockHash = (await this.blockSource.getBlockData({ number: parentBlockNumber }))?.blockHash; + const opened = await this.checkpointsBuilder.getFork(parentBlockNumber, parentBlockHash); + if (signal.aborted) { + // The caller lost the race and never reached the `await using` that would have closed this fork, so + // closing it is this continuation's job: otherwise every abandoned duty leaks a world-state fork. + await opened[Symbol.asyncDispose](); + throw new DutyBudgetExpiredError(what, budget.deadline); + } + return opened; + }); } catch (err) { + if (err instanceof DutyBudgetExpiredError) { + throw err; + } this.log.warn(`Failed to fork world state at block ${parentBlockNumber} for checkpoint proposal`, { ...proposalInfo, parentBlockNumber, @@ -1754,7 +1923,9 @@ export class ProposalHandler { // built on (e.g. a reorg), so recomputing the checkpoint against it would be meaningless. This mirrors // the block-proposal re-execution check and fails fast with a clean, non-slashable result instead of a // confusing downstream mismatch. - const forkArchiveRoot = new Fr((await fork.getTreeInfo(MerkleTreeId.ARCHIVE)).root); + const forkArchiveRoot = new Fr( + (await budget.run(`fork archive root for slot ${slot}`, () => fork.getTreeInfo(MerkleTreeId.ARCHIVE))).root, + ); if (!forkArchiveRoot.equals(proposal.checkpointHeader.lastArchiveRoot)) { this.log.warn(`Fork archive root does not match checkpoint proposal's last archive`, { ...proposalInfo, @@ -1764,21 +1935,27 @@ export class ProposalHandler { return { isValid: false, reason: 'initial_archive_mismatch', checkpointNumber }; } - // Create checkpoint builder with all existing blocks - const checkpointBuilder = await this.checkpointsBuilder.openCheckpoint( - checkpointNumber, - constants, - proposal.feeAssetPriceModifier, - l1ToL2Messages, - previousCheckpointOutHashes, - previousInboxRollingHash, - fork, - blocks, - this.log.getBindings(), - ); - - // Complete the checkpoint to get computed values - const computedCheckpoint = await checkpointBuilder.completeCheckpoint(); + // Create checkpoint builder with all existing blocks, and complete it to get the computed values. + const reconstruction = `checkpoint reconstruction for slot ${slot}`; + const computedCheckpoint = await budget.run(reconstruction, async signal => { + const checkpointBuilder = await this.checkpointsBuilder.openCheckpoint( + checkpointNumber, + constants, + proposal.feeAssetPriceModifier, + l1ToL2Messages, + previousCheckpointOutHashes, + previousInboxRollingHash, + fork, + blocks, + this.log.getBindings(), + ); + // The caller's `await using` starts closing the fork the moment it loses the race, and the rebuild reads + // from that fork, so a late `openCheckpoint` must not run one against a fork that is going away. + if (signal.aborted) { + throw new DutyBudgetExpiredError(reconstruction, budget.deadline); + } + return await checkpointBuilder.completeCheckpoint(); + }); // Compare checkpoint header with proposal if (!computedCheckpoint.header.equals(proposal.checkpointHeader)) { @@ -1889,11 +2066,13 @@ export class ProposalHandler { * pipeline building on top of the checkpoint. Does not retry, since validation already waited for the * last block to sync. */ - private async setProposedCheckpoint(proposal: CheckpointProposalCore): Promise { + private async setProposedCheckpoint(proposal: CheckpointProposalCore, budget: DutyBudget): Promise { if (!this.archiver) { return false; } - const blockData = await this.blockSource.getBlockData({ archive: proposal.archive }); + const blockData = await budget.run(`proposed checkpoint block read for slot ${proposal.slotNumber}`, () => + this.blockSource.getBlockData({ archive: proposal.archive }), + ); if (!blockData) { this.log.debug(`Block data not found for checkpoint proposal archive, cannot set proposed checkpoint`, { archive: proposal.archive.toString(), @@ -1901,6 +2080,16 @@ export class ProposalHandler { return false; } + // The read above can settle after the duty is over. Accepting a pipelining parent is a mutation of state the + // next slot builds on, so it needs the budget checked here rather than only before the read. + if (!budget.canContinue()) { + this.log.warn(`Not recording the proposed checkpoint for slot ${proposal.slotNumber}: the duty budget is gone`, { + archive: proposal.archive.toString(), + deadline: budget.deadline.toISOString(), + }); + return false; + } + await this.archiver.addProposedCheckpoint({ header: proposal.checkpointHeader, checkpointNumber: blockData.checkpointNumber, diff --git a/yarn-project/validator-client/src/streaming_inbox_checks.test.ts b/yarn-project/validator-client/src/streaming_inbox_checks.test.ts index 788d02426eb8..6da435e675cf 100644 --- a/yarn-project/validator-client/src/streaming_inbox_checks.test.ts +++ b/yarn-project/validator-client/src/streaming_inbox_checks.test.ts @@ -256,5 +256,17 @@ describe('checkStreamingBlockProposal', () => { const result = await readStreamingBlockBundle(view, metadata as typeof metadata & { accepted: true }); expect(result).toEqual({ accepted: false, reason: 'inbox_prefix_unavailable' }); }); + + it('carries the error text of a range read that failed for an unanticipated reason', async () => { + const failing = { + getL1ToL2MessageRange: () => Promise.reject(new Error('database is closed')), + }; + const result = await readStreamingBlockBundle(failing, { + parentTotalMsgCount: 0n, + endTotalMsgCount: 1n, + inboxPrefixRef: InboxMessagePrefixRef.random(), + }); + expect(result).toEqual({ accepted: false, reason: 'inbox_prefix_unavailable', error: 'database is closed' }); + }); }); }); diff --git a/yarn-project/validator-client/src/streaming_inbox_checks.ts b/yarn-project/validator-client/src/streaming_inbox_checks.ts index d509ac376ccd..f7d98f9f69e9 100644 --- a/yarn-project/validator-client/src/streaming_inbox_checks.ts +++ b/yarn-project/validator-client/src/streaming_inbox_checks.ts @@ -89,6 +89,11 @@ export type StreamingBlockCheckResult = /** A check failed; `reason` mirrors the acceptance condition that rejected the proposal. */ accepted: false; reason: StreamingBlockCheckReason; + /** + * Text of an unexpected message-source failure behind an `inbox_prefix_unavailable` verdict, so a caller whose + * retries run out can say why. Absent for ordinary sync lag and for every other reason. + */ + error?: string; }; /** @@ -179,6 +184,10 @@ export async function checkStreamingBlockProposalMetadata( * same local-view condition as a missing prefix hash and the caller retries both the same way. A replacement that * lands *after* this read is not this function's race: the bundle and the proposal still agree, so re-execution is * consistent, and the archiver's insert guard is what refuses to store the block. + * + * Every failure of the range read is non-punitive, but not every failure is ordinary sync lag: a store fault or a + * broken provider surfaces here as the same verdict. The unexpected ones carry their error text on the result so a + * caller that gives up waiting can report it, without changing what the validator does with the proposal. */ export async function readStreamingBlockBundle( messageSource: Pick, @@ -193,8 +202,8 @@ export async function readStreamingBlockBundle( messages, end: { rollingHash: endRollingHash }, } = await messageSource.getL1ToL2MessageRange(parentTotalMsgCount, endTotalMsgCount)); - } catch { - return { accepted: false, reason: 'inbox_prefix_unavailable' }; + } catch (err) { + return { accepted: false, reason: 'inbox_prefix_unavailable', error: unexpectedRangeReadError(err) }; } if (!endRollingHash.equals(inboxPrefixRef.inboxRollingHash)) { @@ -204,6 +213,25 @@ export async function readStreamingBlockBundle( return { accepted: true, bundle: messages }; } +/** + * Message fragments that identify an ordinary local-view shortfall from the archiver's range read: the range reaches + * past what this node has synced, or its bounds are not a valid range at all. Matched on the message text rather than + * on the error's class, since a JSON-RPC hop between the validator and its message source leaves the class behind + * while preserving the message. + */ +const EXPECTED_RANGE_READ_FAILURES = ['is not fully synced', 'Invalid Inbox leaf count range']; + +/** Upper bound on the error text carried onto a result, so a verbose provider error cannot blow up a log record. */ +const MAX_REPORTED_ERROR_LENGTH = 200; + +/** Bounded text of a range-read failure the checks did not anticipate, or undefined for ordinary sync lag. */ +function unexpectedRangeReadError(err: unknown): string | undefined { + const message = err instanceof Error ? err.message : String(err); + return EXPECTED_RANGE_READ_FAILURES.some(fragment => message.includes(fragment)) + ? undefined + : message.slice(0, MAX_REPORTED_ERROR_LENGTH); +} + /** * Runs the per-block acceptance checks a validator applies to a streaming block proposal, and derives the * message-leaf bundle the block consumes. Composes {@link checkStreamingBlockProposalMetadata} with diff --git a/yarn-project/validator-client/src/validator.test.ts b/yarn-project/validator-client/src/validator.test.ts index f18c832ee443..406a4d31372d 100644 --- a/yarn-project/validator-client/src/validator.test.ts +++ b/yarn-project/validator-client/src/validator.test.ts @@ -63,6 +63,7 @@ import type { FullNodeCheckpointsBuilder, } from './checkpoint_builder.js'; import { type ValidatorClientConfig, validatorClientConfigMappings } from './config.js'; +import type { ValidationService } from './duties/validation_service.js'; import { HAKeyStore } from './key_store/ha_key_store.js'; import { type CheckpointProposalValidationFailureReason, ProposalHandler } from './proposal_handler.js'; import { ValidatorClient } from './validator.js'; @@ -902,6 +903,92 @@ describe('ValidatorClient', () => { validateCheckpointSpy.mockRestore(); }); + // Signing may be remote (HA), so a request that started in time can return past the attestation deadline. + // Peers reject a stale attestation, so it must not reach the pool or be handed back for gossip; the + // signing-protection record it produced still stands. + it('discards a checkpoint attestation whose signer returns past the attestation deadline', async () => { + const addCheckpointAttestationsSpy = jest.spyOn(p2pClient, 'addOwnCheckpointAttestations'); + epochCache.filterInCommittee.mockResolvedValue([EthAddress.fromString(validatorAccounts[0].address)]); + + const checkpointProposal = await makeCheckpointProposal({ + archiveRoot: proposal.archive, + checkpointHeader: makeCheckpointHeader(0, { slotNumber: proposal.slotNumber }), + lastBlock: { + blockHeader: makeBlockHeader(1, { blockNumber: BlockNumber(123), slotNumber: proposal.slotNumber }), + indexWithinCheckpoint: IndexWithinCheckpoint(0), + txHashes: proposal.txHashes, + }, + }); + + const validateCheckpointSpy = jest + .spyOn(validatorClient.getProposalHandler(), 'validateCheckpointProposal') + .mockResolvedValue({ isValid: true, checkpointNumber: CheckpointNumber(1) }); + + // The signer takes long enough that the slot's attestation deadline passes while it runs. + const deadline = validatorClient.getProposalHandler().getReexecutionDeadline(proposal.slotNumber); + const validationService = (validatorClient as unknown as { validationService: ValidationService }) + .validationService; + const attest = validationService.attestToCheckpointProposal.bind(validationService); + jest + .spyOn(validationService, 'attestToCheckpointProposal') + .mockImplementation(async (...args: Parameters) => { + const attestations = await attest(...args); + dateProvider.setTime(deadline.getTime() + 1_000); + return attestations; + }); + + const attestations = await validatorClient.attestToCheckpointProposal( + ValidatedCheckpointProposalCore(checkpointProposal), + sender, + ); + + expect(attestations).toBeUndefined(); + expect(addCheckpointAttestationsSpy).not.toHaveBeenCalled(); + validateCheckpointSpy.mockRestore(); + }); + + // A remote (HA) signer that never answers must not hold the duty open either. The request stands as issued, + // so the equivocation record it produced is kept, but nothing is pooled or handed back for gossip. + it('settles when the remote signer never returns, pooling nothing and keeping the signing record', async () => { + const addCheckpointAttestationsSpy = jest.spyOn(p2pClient, 'addOwnCheckpointAttestations'); + epochCache.filterInCommittee.mockResolvedValue([EthAddress.fromString(validatorAccounts[0].address)]); + + const checkpointProposal = await makeCheckpointProposal({ + archiveRoot: proposal.archive, + checkpointHeader: makeCheckpointHeader(0, { slotNumber: proposal.slotNumber }), + lastBlock: { + blockHeader: makeBlockHeader(1, { blockNumber: BlockNumber(123), slotNumber: proposal.slotNumber }), + indexWithinCheckpoint: IndexWithinCheckpoint(0), + txHashes: proposal.txHashes, + }, + }); + + const validateCheckpointSpy = jest + .spyOn(validatorClient.getProposalHandler(), 'validateCheckpointProposal') + .mockResolvedValue({ isValid: true, checkpointNumber: CheckpointNumber(1) }); + + // A short but nonzero budget, and a signer that never answers within it. + const deadline = validatorClient.getProposalHandler().getReexecutionDeadline(proposal.slotNumber); + dateProvider.setTime(deadline.getTime() - 200); + const validationService = (validatorClient as unknown as { validationService: ValidationService }) + .validationService; + jest.spyOn(validationService, 'attestToCheckpointProposal').mockImplementation(() => new Promise(() => {})); + + const attestations = await validatorClient.attestToCheckpointProposal( + ValidatedCheckpointProposalCore(checkpointProposal), + sender, + ); + + expect(attestations).toBeUndefined(); + expect(addCheckpointAttestationsSpy).not.toHaveBeenCalled(); + // The slot stays protected: a signer that never answered may still have signed. + expect( + (validatorClient as unknown as { lastAttestedProposal?: { slotNumber: SlotNumber } }).lastAttestedProposal + ?.slotNumber, + ).toEqual(proposal.slotNumber); + validateCheckpointSpy.mockRestore(); + }); + it('should not attest to a checkpoint proposal that references a middle block instead of the last', async () => { const addCheckpointAttestationsSpy = jest.spyOn(p2pClient, 'addOwnCheckpointAttestations'); diff --git a/yarn-project/validator-client/src/validator.ts b/yarn-project/validator-client/src/validator.ts index 3940619e97ac..675313dca950 100644 --- a/yarn-project/validator-client/src/validator.ts +++ b/yarn-project/validator-client/src/validator.ts @@ -61,6 +61,7 @@ import type { TypedDataDefinition } from 'viem'; import type { FullNodeCheckpointsBuilder } from './checkpoint_builder.js'; import { ValidationService } from './duties/validation_service.js'; +import { DutyBudget, DutyBudgetExpiredError } from './duty_budget.js'; import { HAKeyStore } from './key_store/ha_key_store.js'; import type { ExtendedValidatorKeyStore } from './key_store/interface.js'; import { NodeKeystoreAdapter } from './key_store/node_keystore_adapter.js'; @@ -519,12 +520,45 @@ export class ValidatorClient extends (EventEmitter as new () => WatcherEmitter) async attestToCheckpointProposal( proposal: ValidatedCheckpointProposalCore, _proposalSender: PeerId, + ): Promise { + // The all-nodes callback ran under a budget of its own; this one repeats the committee reads, may revalidate + // and then signs, so it takes an absolute budget for the same slot covering all of it. Without one the + // preliminary reads and the signer await are unbounded, and p2p waits on this callback. + const budget = new DutyBudget(this.proposalHandler.getReexecutionDeadline(proposal.slotNumber), this.dateProvider); + try { + // Every stage inside consults the budget, and the whole body is raced against it as well: an await this + // audit missed, or one a later change adds, still cannot hold p2p's callback open past the slot. + return await budget.run(`checkpoint attestation for slot ${proposal.slotNumber}`, () => + this.attestToCheckpointProposalWithinBudget(proposal, budget), + ); + } catch (err) { + if (!(err instanceof DutyBudgetExpiredError)) { + throw err; + } + this.log.warn(`Ran out of duty budget attesting to the checkpoint proposal for slot ${proposal.slotNumber}`, { + slot: proposal.slotNumber, + deadline: budget.deadline.toISOString(), + }); + return undefined; + } finally { + // Anything still retrying belongs to a duty nobody will read from any more. + budget.stop(`checkpoint attestation for slot ${proposal.slotNumber}`); + } + } + + /** {@link attestToCheckpointProposal}'s body, with every stage bound to the slot's duty budget. */ + private async attestToCheckpointProposalWithinBudget( + proposal: ValidatedCheckpointProposalCore, + budget: DutyBudget, ): Promise { const proposalSlotNumber = proposal.slotNumber; const proposer = proposal.getSender(); // If escape hatch is open for this slot's epoch, do not attest. - if (await this.epochCache.isEscapeHatchOpenAtSlot(proposalSlotNumber)) { + const escapeHatchOpen = await budget.run(`escape hatch check for slot ${proposalSlotNumber}`, () => + this.epochCache.isEscapeHatchOpenAtSlot(proposalSlotNumber), + ); + if (escapeHatchOpen) { this.log.warn(`Escape hatch open for slot ${proposalSlotNumber}, skipping checkpoint attestation handling`); return undefined; } @@ -544,7 +578,9 @@ export class ValidatorClient extends (EventEmitter as new () => WatcherEmitter) } // Check that I have any address in the committee where this checkpoint will land before attesting - const inCommittee = await this.epochCache.filterInCommittee(proposalSlotNumber, this.getValidatorAddresses()); + const inCommittee = await budget.run(`committee membership check for slot ${proposalSlotNumber}`, () => + this.epochCache.filterInCommittee(proposalSlotNumber, this.getValidatorAddresses()), + ); const partOfCommittee = inCommittee.length > 0; const proposalInfo = { @@ -564,7 +600,7 @@ export class ValidatorClient extends (EventEmitter as new () => WatcherEmitter) this.log.warn(`Skipping checkpoint proposal validation for slot ${proposalSlotNumber}`, proposalInfo); checkpointNumber = CheckpointNumber(0); } else { - const validationResult = await this.proposalHandler.handleCheckpointProposal(proposal, proposalInfo); + const validationResult = await this.proposalHandler.handleCheckpointProposal(proposal, proposalInfo, budget); if (!validationResult.isValid) { this.log.warn(`Checkpoint proposal validation failed: ${validationResult.reason}`, proposalInfo); return undefined; @@ -627,7 +663,10 @@ export class ValidatorClient extends (EventEmitter as new () => WatcherEmitter) return undefined; } - return await this.createCheckpointAttestationsFromProposal(proposal, attestors, checkpointNumber); + // Everything above may have waited on local state to catch up, so the signing gate takes the slot's duty budget, + // whose deadline is the consensus attestation deadline: nothing signed after it reaches a peer that would still + // accept it. + return await this.createCheckpointAttestationsFromProposal(proposal, attestors, checkpointNumber, budget); } /** @@ -651,22 +690,87 @@ export class ValidatorClient extends (EventEmitter as new () => WatcherEmitter) return true; } + /** + * The one place a checkpoint attestation is produced, so `budget` gates every path that reaches it, the + * cached-verdict one included. Callers responding to a peer's proposal pass the slot's duty budget, whose + * deadline is the consensus attestation deadline; the proposer's own attestations are produced inside its + * publish budget instead and pass none. + */ private async createCheckpointAttestationsFromProposal( proposal: CheckpointProposalCore, attestors: EthAddress[] = [], checkpointNumber: CheckpointNumber, + budget?: DutyBudget, ): Promise { // Equivocation check: must happen right before signing to minimize the race window if (!this.shouldAttestToSlot(proposal.slotNumber)) { return undefined; } - const attestations = await this.validationService.attestToCheckpointProposal(proposal, attestors, checkpointNumber); + if (budget?.expired()) { + this.log.warn(`Not requesting an attestation for slot ${proposal.slotNumber}: past the attestation deadline`, { + slot: proposal.slotNumber, + deadline: budget.deadline.toISOString(), + }); + return undefined; + } - // Track the proposal we attested to (to prevent equivocation) + // Track the proposal we attested to (to prevent equivocation) before the request goes out, not after it comes + // back. A remote signer may have signed a request this node stopped waiting for, so the protection record has + // to stand for every request that was issued, however it ends. this.lastAttestedProposal = proposal; - await this.p2pClient.addOwnCheckpointAttestations(attestations); + const sign = () => this.validationService.attestToCheckpointProposal(proposal, attestors, checkpointNumber); + let attestations: CheckpointAttestation[]; + try { + attestations = budget + ? await budget.run(`checkpoint attestation signing for slot ${proposal.slotNumber}`, sign) + : await sign(); + } catch (err) { + if (!(err instanceof DutyBudgetExpiredError)) { + throw err; + } + // Signing may be remote (HA) and may never answer. The request stands as issued; this caller stops waiting. + this.log.warn(`Abandoning attestations for slot ${proposal.slotNumber}: the signer did not answer in time`, { + slot: proposal.slotNumber, + deadline: budget!.deadline.toISOString(), + }); + return undefined; + } + + // Signing may be remote (HA), so a request that started in time can still return past the deadline. Peers + // reject a stale attestation, so discard it rather than putting it in the pool or handing it back for gossip. + if (budget?.expired()) { + this.log.warn(`Discarding attestations for slot ${proposal.slotNumber}: the signer returned too late`, { + slot: proposal.slotNumber, + deadline: budget.deadline.toISOString(), + }); + return undefined; + } + + // The pool write is another await that can outlast the slot, and what comes back from here is gossiped, so + // it is bounded and the deadline rechecked after it rather than only after signing. + const pool = () => this.p2pClient.addOwnCheckpointAttestations(attestations); + try { + budget ? await budget.run(`attestation pool write for slot ${proposal.slotNumber}`, pool) : await pool(); + } catch (err) { + if (!(err instanceof DutyBudgetExpiredError)) { + throw err; + } + this.log.warn(`Abandoning attestations for slot ${proposal.slotNumber}: the pool did not accept them in time`, { + slot: proposal.slotNumber, + deadline: budget!.deadline.toISOString(), + }); + return undefined; + } + + if (budget?.expired()) { + this.log.warn(`Not gossiping attestations for slot ${proposal.slotNumber}: the deadline passed`, { + slot: proposal.slotNumber, + deadline: budget.deadline.toISOString(), + }); + return undefined; + } return attestations; }