-
Notifications
You must be signed in to change notification settings - Fork 592
feat(pipeline): minimize deadzone w cross slot attesting #21435
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: md/pipeline-spartan-config
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,10 +30,10 @@ import { EpochsTestContext } from './epochs_test.js'; | |
| jest.setTimeout(1000 * 60 * 20); | ||
|
|
||
| const NODE_COUNT = 4; | ||
| const EXPECTED_BLOCKS_PER_CHECKPOINT = 3; | ||
| const EXPECTED_BLOCKS_PER_CHECKPOINT = 8; | ||
|
|
||
| // Send enough transactions to trigger multiple blocks within a checkpoint assuming 2 txs per block. | ||
| const TX_COUNT = 10; | ||
| const TX_COUNT = 24; | ||
|
|
||
| /** | ||
| * E2E tests for proposer pipelining with Multiple Blocks Per Slot (MBPS). | ||
|
|
@@ -72,16 +72,18 @@ describe('e2e_epochs/epochs_mbps_pipeline', () => { | |
| initialValidators: validators, | ||
| enableProposerPipelining: true, // <- yehaw | ||
| mockGossipSubNetwork: true, | ||
| mockGossipSubNetworkLatency: 500, // 200 ms delay in message prop - adverse network conditions | ||
| disableAnvilTestWatcher: true, | ||
| startProverNode: true, | ||
| perBlockAllocationMultiplier: 1, | ||
| perBlockAllocationMultiplier: 8, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will lead to very big blocks at the beginning of the checkpoint, we sure about it? |
||
| aztecEpochDuration: 4, | ||
| enforceTimeTable: true, | ||
| ethereumSlotDuration: 4, | ||
| aztecSlotDuration: 36, | ||
| ethereumSlotDuration: 12, | ||
| aztecSlotDuration: 72, | ||
| blockDurationMs: 8000, | ||
| l1PublishingTime: 2, | ||
| attestationPropagationTime: 0.5, | ||
| // maxDABlockGas: 786432, // Set max DA block gas to be the same as the checkpoint | ||
| // l1PublishingTime: 2, | ||
| // attestationPropagationTime: 1, | ||
|
Comment on lines
+81
to
+86
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why these changes? |
||
| aztecTargetCommitteeSize: 3, | ||
| ...setupOpts, | ||
| pxeOpts: { syncChainTip }, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,7 @@ import { | |
| type ValidationResult, | ||
| } from '@aztec/stdlib/p2p'; | ||
|
|
||
| import { isWithinClockTolerance } from '../clock_tolerance.js'; | ||
| import { isWithinClockTolerance, isWithinPipeliningGracePeriod } from '../clock_tolerance.js'; | ||
|
|
||
| export class CheckpointAttestationValidator implements P2PValidator<CheckpointAttestation> { | ||
| protected epochCache: EpochCacheInterface; | ||
|
|
@@ -23,19 +23,23 @@ export class CheckpointAttestationValidator implements P2PValidator<CheckpointAt | |
| const slotNumber = message.payload.header.slotNumber; | ||
|
|
||
| try { | ||
| // Use target slots since proposals target pipeline slots (slot + 1 when pipelining) | ||
| // Use target slots since proposals target pipeline slots (slot + 1 when pipelining). | ||
| const { targetSlot, nextSlot } = this.epochCache.getTargetAndNextSlot(); | ||
|
|
||
| if (slotNumber !== targetSlot && slotNumber !== nextSlot) { | ||
| // Check if message is for previous slot and within clock tolerance | ||
| if (!isWithinClockTolerance(slotNumber, targetSlot, this.epochCache)) { | ||
| // When pipelining, accept attestations for the current slot (built in the previous slot) | ||
| // if we're within the first ethereumSlotDuration/2 seconds of the slot. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment is wrong? The implementation accepts up until the p2p propagation time. |
||
| if (isWithinPipeliningGracePeriod(slotNumber, this.epochCache)) { | ||
| // Fall through to remaining validation (signature, committee, etc.) | ||
| } else if (!isWithinClockTolerance(slotNumber, targetSlot, this.epochCache)) { | ||
| this.logger.warn( | ||
| `Checkpoint attestation slot ${slotNumber} is not current (${targetSlot}) or next (${nextSlot}) slot`, | ||
| ); | ||
| return { result: 'reject', severity: PeerErrorSeverity.HighToleranceError }; | ||
| } else { | ||
| this.logger.debug(`Ignoring checkpoint attestation for previous slot ${slotNumber} within clock tolerance`); | ||
| return { result: 'ignore' }; | ||
| } | ||
| this.logger.debug(`Ignoring checkpoint attestation for previous slot ${slotNumber} within clock tolerance`); | ||
| return { result: 'ignore' }; | ||
| } | ||
|
|
||
| // Verify the signature is valid | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| import type { EpochCacheInterface } from '@aztec/epoch-cache'; | ||
| import { SlotNumber } from '@aztec/foundation/branded-types'; | ||
| import { DEFAULT_P2P_PROPAGATION_TIME } from '@aztec/stdlib/timetable'; | ||
|
|
||
| /** | ||
| * Maximum clock disparity tolerance for P2P message validation (in milliseconds). | ||
|
|
@@ -50,3 +51,33 @@ export function isWithinClockTolerance( | |
|
|
||
| return elapsedMs < MAXIMUM_GOSSIP_CLOCK_DISPARITY_MS; | ||
| } | ||
|
|
||
| /** | ||
| * Checks if a message should be accepted under the pipelining grace period. | ||
| * | ||
| * When pipelining is enabled, `targetSlot = slotNow + 1`. A proposal built in slot N-1 | ||
| * for slot N arrives when validators are in slot N, so their `targetSlot = N+1`. | ||
| * This function accepts proposals for the current wallclock slot if we're within the | ||
| * first `DEFAULT_P2P_PROPAGATION_TIME` seconds of the slot (the pipelining grace period). | ||
| * | ||
| * @param messageSlot - The slot number from the received message | ||
| * @param epochCache - EpochCache to get timing and pipelining state | ||
| * @returns true if pipelining is enabled, the message is for the current slot, and we're within the grace period | ||
| */ | ||
| export function isWithinPipeliningGracePeriod(messageSlot: SlotNumber, epochCache: EpochCacheInterface): boolean { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this function also consider the clock disparity time?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, can't we just merge this function with the one above? They are all used in all the same places after all. |
||
| if (!epochCache.isProposerPipeliningEnabled()) { | ||
| return false; | ||
| } | ||
|
|
||
| const currentSlot = epochCache.getSlotNow(); | ||
| if (messageSlot !== currentSlot) { | ||
| return false; | ||
| } | ||
|
|
||
| const { ts: slotStartTs, nowMs } = epochCache.getEpochAndSlotNow(); | ||
| const slotStartMs = slotStartTs * 1000n; | ||
| const elapsedMs = Number(nowMs - slotStartMs); | ||
| const gracePeriodMs = DEFAULT_P2P_PROPAGATION_TIME * 1000; | ||
|
|
||
| return elapsedMs < gracePeriodMs; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's amazing how fast comments become outdated