-
-
Notifications
You must be signed in to change notification settings - Fork 451
fix: pass prepareSlot to fork choice head for Gloas FULL vs EMPTY tie-breaker #9164
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: unstable
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 |
|---|---|---|
|
|
@@ -20,9 +20,22 @@ import {IBeaconChain} from "./interface.js"; | |
| import {getPayloadAttributesForSSE, prepareExecutionPayload} from "./produceBlock/produceBlockBody.js"; | ||
| import {RegenCaller} from "./regen/index.js"; | ||
|
|
||
| // TODO GLOAS: re-evaluate this timing | ||
| /* With 12s slot times, this scheduler will run 4s before the start of each slot (`12 - 0.6667 * 12 = 4`). */ | ||
| export const PREPARE_NEXT_SLOT_BPS = 6667; | ||
| /* Pre-Gloas: prepare at 6667 BPS (~8s into a 12s slot), leaving 4s for preparation. */ | ||
| const PREPARE_NEXT_SLOT_BPS = 6667; | ||
|
|
||
| /** | ||
| * Gloas+: prepare at 8333 BPS (~10s into a 12s slot), leaving 2s for preparation. | ||
| * This is after PAYLOAD_ATTESTATION_DUE_BPS (7500 / 9s) so that fork choice | ||
| * incorporates PTC votes (payload PRESENT/ABSENT) before computing the head | ||
| * and preparing the execution payload for the next slot. | ||
| * TODO GLOAS: re-check before Gloas mainnet | ||
| */ | ||
| const PREPARE_NEXT_SLOT_BPS_GLOAS = 8333; | ||
|
Member
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. after my spec changes we can keep previous something like this could work await Promise.race([
sleep(SOME_BPS_VALUE - this.clock.msFromSlot(slot), signal),
this.emitter.waitForPtcThreshold(slot),
]); |
||
|
|
||
| export function getPrepareNextSlotMs(config: ChainForkConfig, slot: Slot): number { | ||
| const bps = ForkSeq[config.getForkName(slot)] >= ForkSeq.gloas ? PREPARE_NEXT_SLOT_BPS_GLOAS : PREPARE_NEXT_SLOT_BPS; | ||
| return config.getSlotComponentDurationMs(bps); | ||
| } | ||
|
|
||
| /* We don't want to do more epoch transition than this */ | ||
| const PREPARE_EPOCH_LIMIT = 1; | ||
|
|
@@ -73,12 +86,11 @@ export class PrepareNextSlotScheduler { | |
| } | ||
|
|
||
| try { | ||
| // At PREPARE_NEXT_SLOT_BPS (~67%) of the current slot we prepare payload for the next slot | ||
| // or precompute epoch transition | ||
| await sleep(this.config.getSlotComponentDurationMs(PREPARE_NEXT_SLOT_BPS), this.signal); | ||
| // Wait until the appropriate point in the slot to prepare payload or precompute epoch transition | ||
| await sleep(getPrepareNextSlotMs(this.config, prepareSlot), this.signal); | ||
|
|
||
| // calling updateHead() here before we produce a block to reduce reorg possibility | ||
| const headBlock = this.chain.recomputeForkChoiceHead(ForkchoiceCaller.prepareNextSlot); | ||
| const headBlock = this.chain.recomputeForkChoiceHead(ForkchoiceCaller.prepareNextSlot, prepareSlot); | ||
|
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.
Passing Useful? React with 👍 / 👎.
Contributor
Author
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. after 67% of the slot we can make an ephemeral head in this case, but it will make the current design more complex
Member
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. doesn't this cause the head to be recomputed so any caller to
Contributor
Author
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.
yes. From getting both EMPTY + FULL till the next slot, we need to fairly choose one of them, recompute head and cache. My interpretation would be: when preparing for the next slot, if we have both EMPTY and FULL variant, we need to get through tie-breaker logic to get the winner |
||
| const {slot: headSlot, blockRoot: headRoot} = headBlock; | ||
|
|
||
| // PS: previously this was comparing slots, but that gave no leway on the skipped | ||
|
|
||
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.
Discussed this offline with @nflaig
would prefer an interface like so:
Pass optional opts to
forkChoice.updateHeadand do theconst currentSlot = opts?.prepareNextSlot ? store.currentSlot + 1 : store.curentSlot;thereThere 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.
just realized we already have
ForkchoiceCaller.prepareNextSlot, so the boolean here seems redundant