feat(pipeline): introduce pipeline views for building#21026
feat(pipeline): introduce pipeline views for building#21026Maddiaa0 merged 1 commit intomerge-train/spartanfrom
Conversation
9e57ad3 to
82d017c
Compare
3a75fdb to
1ca98d8
Compare
There was a problem hiding this comment.
Changes look good, but I don't love the new API for the epoch cache. I'd suggest either:
- Rolling back the changes in epoch cache, have it just report the
nowinfo as it did, and have clients apply the pipelining offset as needed (most likely via a shared helper somewhere). - Keeping the same API as we did in the epoch cache, but add new methods like
getEpochAndSlotPipelinewhich return the pipelined view for their counterparts (getEpochAndSlotNowhere). I prefer this option.
Aside from that, I'd review naming: I found the "pipeline slot" confusing. The suggestion I like the most from Claude was current/target instead of now/pipeline.
| submissionSlot: this.pipelineSlot, | ||
| submissionSlotTimestamp, | ||
| }); | ||
| await sleepUntil(new Date(Number(submissionSlotTimestamp) * 1000), this.dateProvider.nowAsDate()); |
There was a problem hiding this comment.
I think we can sleep until 12s before the submission slot timestamp, so L1 txs can get included on the first L1 block of the L2 slot
|
|
||
| // Check that I have any address in current committee before attesting | ||
| const inCommittee = await this.epochCache.filterInCommittee(slotNumber, this.getValidatorAddresses()); | ||
| // TODO(md): we check based on the slot number that is in the proposal - but not that the slot number makes sense????? |
There was a problem hiding this comment.
IIRC that's handled on the p2p proposal validator, but yeah, still worth looking into
| proposer ?? EthAddress.ZERO, | ||
| invalidateCheckpoint, | ||
| ); | ||
| const canProposeCheck = await publisher.canProposeAtNextEthBlock(syncedTo.archive, proposer ?? EthAddress.ZERO, { |
There was a problem hiding this comment.
I think we should update the publisher methods so, instead of setting an offset to now, we just pass the publishing timestamp range for the slot and let the publisher handle it.
| @@ -749,10 +749,11 @@ export class RollupContract { | |||
| archive: Buffer, | |||
| account: `0x${string}` | Account, | |||
| slotDuration: number, | |||
| opts: { forcePendingCheckpointNumber?: CheckpointNumber } = {}, | |||
| opts: { forcePendingCheckpointNumber?: CheckpointNumber; additionalSlotOffset?: number } = {}, | |||
| ): Promise<{ slot: SlotNumber; checkpointNumber: CheckpointNumber; timeOfNextL1Slot: bigint }> { | |||
There was a problem hiding this comment.
I'd change this to a canProposeAt for robustness
b1b1b14 to
ce0e422
Compare
ce0e422 to
d8b8d31
Compare
82d017c to
21053ab
Compare
d8b8d31 to
6973d24
Compare
21053ab to
f3565ef
Compare
6973d24 to
21cdcd5
Compare
f3565ef to
e14a469
Compare
21cdcd5 to
9543a61
Compare
e14a469 to
09904a9
Compare
9543a61 to
6644382
Compare
6644382 to
ed993b1
Compare
09904a9 to
27a398a
Compare
| return this.l1constants; | ||
| } | ||
|
|
||
| public isProposerPipeliningEnabled(): boolean { |
There was a problem hiding this comment.
Majority of work here is creating these views -> then other components use the version that matters to them.
if pipelining is disabled - this return what it previously did - so backwards compatible
d527e2e to
c60ca91
Compare
spalladino
left a comment
There was a problem hiding this comment.
Epoch cache looks great now. Left a few comments, but looks good to merge.
| // When pipelining, anchor the simulation timestamp to the checkpoint's own slot start time | ||
| // rather than the current L1 block timestamp, which may overshoot into the next slot if the build ran late. |
There was a problem hiding this comment.
I think we should always anchor to the checkpoint, rather than relying on current L1 block timestamp? We'll have to fix it in #14766 anyway.
| // Sync the pending broadcast block to the archiver before broadcasting the proposal. | ||
| // Same as non-last blocks in the build loop, this catches consistency errors early. | ||
| if (blockPendingBroadcast) { | ||
| await this.syncProposedBlockToArchiver(blockPendingBroadcast.block); | ||
| } |
| this.log.info(`Preparing checkpoint proposal ${checkpointNumber} at slot ${slot}`, { ...logCtx, proposer }); | ||
| this.lastSlotForCheckpointProposalJob = targetSlot; | ||
|
|
||
| await this.p2pClient.prepareForSlot(targetSlot); |
There was a problem hiding this comment.
Let's make a note to review this. IIRC prepareForSlot(N) unprotects txs from slot N-1. Since we're now calling with the target slot, I'm worried we may be unprotecting txs we still need.
There was a problem hiding this comment.
P2PClent also calls prepareForSlot so would need to be aligned.
There was a problem hiding this comment.
I think that calling prepareForSlot with the targetSlot is correct. Ill update the p2p client to use it whenever we have a pendingCheckpointProposal (introduced in the next pr) therefore are pipelining
| } | ||
|
|
||
| // Check all components are synced to latest as seen by the archiver (queries all subsystems) | ||
| const syncedTo = await this.checkSync({ ts, slot }); |
There was a problem hiding this comment.
Maybe it comes on a later PR, but it seems like checkSync is missing something. Let's say we're at slot N, building N+1. This checks that we have synced N-1, not N. Maybe we should wait a bit if we have seen block proposals for N, but are still waiting on or processing the checkpoint proposal? Otherwise we risk start building our slot too early while there's still lingering execution from slot N.
There was a problem hiding this comment.
I don't think so. These are only checking that components are 1) synced between each other, and 2) synced at least to slot N-1. But this should not affect the happy path.
There was a problem hiding this comment.
In the pr above it also checks the existence of a pendingCheckpointProposal, we can reconsider this there. I do agree that it may be abit thin
9bba2e5 to
485647b
Compare
| } | ||
|
|
||
| // Then send everything to L1 | ||
| const l1Response = await this.publisher.sendRequests(); |
There was a problem hiding this comment.
Presumably we will only want to submit the proposal if our parent checkpoint didn't get pruned.
4d09a7d to
14c625e
Compare
| // ensures the deploy tx is in the pool before the call tx regardless of gossip timing. | ||
| const timeout = test.L2_SLOT_DURATION_IN_S * 5; | ||
| logger.warn(`Sending both txs and waiting for checkpointed receipts`); | ||
| logger.warn(`Sending deploy tx first, then call tx`); |
14c625e to
255f12c
Compare
BEGIN_COMMIT_OVERRIDE fix(p2p): fall back to maxTxsPerCheckpoint for per-block tx validation (#21605) chore: fixing M3 devcontainer builds (#21611) fix: clamp finalized block to oldest available in world-state (#21643) chore: fix proving logs script (#21335) fix: (A-649) tx collector bench test (#21619) fix(validator): process block proposals from own validator keys in HA setups (#21603) fix: add bounds when allocating arrays in deserialization (#21622) fix: skip handleChainFinalized when block is behind oldest available (#21656) chore: demote finalized block skip log to trace (#21661) fix: skip -march auto-detection for cross-compilation presets (#21356) chore: revert "add bounds when allocating arrays in deserialization" (#21622) (#21666) fix: capture txs not available error reason in proposal handler (#21670) fix: estimate gas in bot and make BatchCall.simulate() return SimulationResult (#21676) fix: prevent HA peer proposals from blocking equivocation in duplicate proposal test (#21673) fix(p2p): penalize peers for errors during response reading (#21680) feat(sequencer): add build-ahead config and metrics (#20779) chore: fixing build on mac (#21685) fix: HA deadlock for last block edge case (#21690) fix: process all contract classes in storeBroadcastedIndividualFunctions (A-683) (#21686) chore: add slack success post on nightly scenario (#21701) fix(builder): persist contractsDB across blocks within a checkpoint (#21520) fix: only delete logs from rolled-back blocks, not entire tag (A-686) (#21687) chore(p2p): lower attestation pool per-slot caps to 2 (#21709) chore(p2p): remove unused method (#21678) fix(p2p): penalize peer on tx rejected by pool (#21677) fix(test): workaround slow mock creation (#21708) fix(sequencer): fix checkpoint budget redistribution for multi-block slots (#21692) fix: batch checkpoint unwinding in handleEpochPrune (A-690) (#21668) fix(sequencer): add missing opts arg to checkpoint_builder tests (#21733) fix: race condition in fast tx collection (#21496) fix: increase default postgres disk size from 1Gi to 10Gi (#21741) fix: update batch_tx_requester tests to use RequestTracker (#21734) chore: replace dead BOOTSTRAP_TO env var with bootstrap.sh build arg (#21744) fix(sequencer): extract gas and blob configs from valid requests only (A-677) (#21747) fix: deflake attempt for l1_tx_utils (#21743) fix(test): fix flaky keystore reload test (#21749) fix(test): fix flaky duplicate_attestation_slash test (#21753) feat(pipeline): introduce pipeline views for building (#21026) END_COMMIT_OVERRIDE


Overview
Epoch cache operations now return two views, the current slot and the pipelined slot / epoch
Testing
The main test which showcases this functionality is epoch_mbps.pipeline, this runs the sequencers with the pipeline mode enabled. At the moment it only expects 1 block per slot as it still waits until the proposal slot to send the checkpoint to l1. For this PR it uses a blocking sleep here, that stops all sequencers for this test. This is addressed in a pr stacked ontop.