feat(pipeline): allow syncing blocks ontop of the proposed chain#21025
feat(pipeline): allow syncing blocks ontop of the proposed chain#21025Maddiaa0 wants to merge 2 commits intomerge-train/spartanfrom
Conversation
8de6157 to
31f941d
Compare
3a75fdb to
1ca98d8
Compare
| } | ||
|
|
||
| const requiredAttestationCount = Math.floor((committee.length * 2) / 3) + 1; | ||
| const requiredAttestationCount = computeQuorum(committee.length); |
| // Atomically set the pending checkpoint number alongside the block if provided | ||
| pendingCheckpointNumber !== undefined && | ||
| this.store.blockStore.setPendingCheckpointNumber(pendingCheckpointNumber), |
There was a problem hiding this comment.
This doesn't match the comment that this Sets the pending checkpoint number (quorum-attested but not yet L1-confirmed), right?
That said, let's discuss the model. Weirdly, I like the concept of having an uncheckpointed checkpoint. But it seems like we have two different things:
- A checkpoint-being-built, which is the checkpoint being built via proposed blocks. This is just a checkpoint number, since a
Checkpointobject needs all its blocks to be ready. Today we already have this, but don't need to explicitly store it. - A checkpoint-being-proposed, which is the
Checkpointobject for which the current proposer has sent a checkpoint proposal, and would ultimately make it onto L1.
We need to define which ones we expose to clients of the archiver, and also to users via APIs like getL2Tips.
| }); | ||
| const additionalSlotOffset = slot.now !== slot.pipeline ? getEthSlotsPerAztecSlot(this.l1Constants) : undefined; | ||
|
|
||
| // When pipelining, if the previous checkpoint has quorum attestations from P2P, we can skip the |
There was a problem hiding this comment.
Do we want to wait for the attestations, or just for our own reexecution?
There was a problem hiding this comment.
Updated to just be own re-execution
| /** Tracks the fire-and-forget L1 submission promise so it can be awaited during shutdown. */ | ||
| private pendingL1Submission: Promise<void> | undefined; |
There was a problem hiding this comment.
Do we want to fire-and-forget the L1 submission from the checkpoint proposal job, or the entire checkpoint proposal job from the sequencer?
There was a problem hiding this comment.
As discussed, its really the same thing since the checkpoint proposal job owns this.
We keep track of this item just so that we can cancel a pending submission job in the event of a rollback (cancelled in next pr)
6a16e98 to
28a0520
Compare
27f8a4b to
f5c6308
Compare
b1b1b14 to
ce0e422
Compare
f5c6308 to
ae6b651
Compare
ce0e422 to
d8b8d31
Compare
3a47fbf to
4bb6845
Compare
d8b8d31 to
6973d24
Compare
4bb6845 to
0e9b52e
Compare
0e9b52e to
dee426a
Compare
6973d24 to
21cdcd5
Compare
21cdcd5 to
9543a61
Compare
7c2c34a to
2548aed
Compare
c60ca91 to
9bba2e5
Compare
9bba2e5 to
4902133
Compare
2548aed to
4c620ca
Compare
| } | ||
|
|
||
| /** Sets the pipelining tree-in-progress boundary for building ahead of L1 confirmation. */ | ||
| public setPipeliningTreeInProgress(value: bigint): Promise<void> { |
There was a problem hiding this comment.
this was added to deal with #21494 - i did not want to directly set the other tree in progress, I think my current solution is nasty and would appreciate a discussion around it
| /** Stores the checkpoint number whose message tree is currently being filled on L1. */ | ||
| #inboxTreeInProgress: AztecAsyncSingleton<bigint>; | ||
| /** Stores the pipelining tree-in-progress boundary, set by the sequencer when building ahead. | ||
| * Separate from the synced value so normal callers still use the strict L1-synced guard. */ |
There was a problem hiding this comment.
nasty solution in question
There was a problem hiding this comment.
How is this nasty? Isn't this just tracking the inbox tree that is at the tip of the proposed chain?
There was a problem hiding this comment.
I mean more that I'm not confident it is correct
| debugLogStore, | ||
| ); | ||
|
|
||
| // Register a callback for all nodes to set the pending checkpoint number when a checkpoint proposal is received. |
There was a problem hiding this comment.
Adds a lovely callback, the existing one was only relevant to nodes performing validation duties - allNodesCheckpointProposalHandler is triggered by everyone, not just validators
| } | ||
|
|
||
| /** Merges multiple StateOverride arrays, combining stateDiff entries for the same address. */ | ||
| public static mergeStateOverrides(...overrides: StateOverride[]): StateOverride { |
There was a problem hiding this comment.
This duplicates packing the fee slot
Whenever checkpoint blocks become very full, the fee will drift from block to block, this prevents simulating into the future failing
| // Knowledege of pending checkpoints is in the PR above | ||
| const { targetSlot } = this.epochCache.getTargetAndNextSlot(); | ||
| if (targetSlot <= this.lastSlotProcessed) { | ||
| let slot; |
4c620ca to
c2cc23c
Compare
c2cc23c to
366f809
Compare
| await store.addCheckpoints([checkpoint1]); | ||
|
|
||
| // Set pending checkpoint to 3 (far ahead) | ||
| await store.blockStore.setPendingCheckpoint({ |
There was a problem hiding this comment.
You have a good point, will restrict
| await store.addCheckpoints([checkpoint1, checkpoint2, checkpoint3]); | ||
|
|
||
| // Set pending to 2 (already confirmed, but that's fine for the test) | ||
| await store.blockStore.setPendingCheckpoint({ |
There was a problem hiding this comment.
Should this be allowed either?
| lastArchive: block2.archive, | ||
| }); | ||
|
|
||
| // Block for checkpoint 2 should work (previous confirmed = 1) |
There was a problem hiding this comment.
Maybe I misunderstand the flow. But shoudl we allow adding proposed blocks for multiple different checkpoints?
Unless we are already potentially supporting build ahead of multiple checkpoints?
There was a problem hiding this comment.
It has been restricted to +1
| block: { number: provenBlockNumber, hash: provenBlockData.blockHash.toString() }, | ||
| checkpoint: provenCheckpointId, | ||
| }, | ||
| pendingCheckpoint: pendingCheckpointBlockData |
There was a problem hiding this comment.
Shall we just call this pending?
| // The checkpoint proposal often arrives before the last block finishes re-execution. | ||
| // Trigger a sync to flush any queued blocks, then retry. | ||
| if (!blockData) { | ||
| await archiver.syncImmediate(); |
There was a problem hiding this comment.
Will probably need a retry loop here won't we?
| this.allNodesCheckpointReceivedCallback = ( | ||
| _checkpoint: CheckpointProposalCore, | ||
| ): Promise<CheckpointAttestation[] | undefined> => { | ||
| return Promise.resolve(undefined); |
There was a problem hiding this comment.
Should probably log an error here shouldn't we? Everyone should register a handler here.
| this.validatorCheckpointReceivedCallback = ( | ||
| checkpoint: CheckpointProposalCore, | ||
| ): Promise<CheckpointAttestation[] | undefined> => { | ||
| this.logger.debug( |
There was a problem hiding this comment.
And this log line could maybe go if this is validator only. Otherwise full nodes will print this routinely.
dcd68a6 to
2d31671
Compare
| const canProposeCheck = await publisher.canProposeAt(syncedTo.archive, proposer ?? EthAddress.ZERO, { | ||
| ...invalidateCheckpoint, | ||
| }); | ||
| // Determine the correct archive and L1 state overrides for the canProposeAt check. |
There was a problem hiding this comment.
This block has been edited since last review @PhilWindle
| } | ||
|
|
||
| // What's the slot of the first uncheckpointed block? | ||
| // Don't prune blocks that are covered by a pending checkpoint (awaiting L1 submission from pipelining) |
There was a problem hiding this comment.
Slightly confused by this. What happens if a checkpoint fails to land on L1? Surely the blocks covered by that (pending) checkpoint are removed? As well as all blocks built afterwards?
There was a problem hiding this comment.
ah, md/pipeline-recovery-2 im dealing with this currently in this branch
2d31671 to
2591475
Compare

Overview
Key contributions:
Adds a second p2p callback that separates what runs for all nodes / validator nodes
Testing
epochs_mbps.pipeline now expects 3 blocks per checkpoint, just like the original epochs_mbps test, now it is fully pipelined.
Upcoming