Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions packages/beacon-node/test/spec/utils/specTestIterator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,11 +102,6 @@ export const defaultSkipOpts: SkipOpts = {
// TODO-GLOAS: re-enable after gloas light client is implemented
/\/gloas_fork$/,
/\/heze_fork$/,
// TODO GLOAS: Proposer-boost dependent-root gate uses stale cached head across epoch-boundary ticks;
// boost wrongly denied. Fails identically on every pre-gloas fork.
// Enable this after https://github.com/ChainSafe/lodestar/issues/9666 is resolved
// The case name embeds the generation seed, so it changes whenever comptests are regenerated.
/fork_choice_compliance\/block_tree_test\/pyspec_tests\/block_tree_test_17_381675768_1$/,
// TODO GLOAS: gloas/heze take ~23-24s on the mainnet preset (~7.5x pre-gloas) because every
// post-gloas slot writes into the SLOTS_PER_HISTORICAL_ROOT-wide executionPayloadAvailability
// bitvector, and this suite steps 8192 slots. That is 76-81% of the 30s sanity/slots timeout,
Expand Down
42 changes: 32 additions & 10 deletions packages/fork-choice/src/forkChoice/forkChoice.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1081,11 +1081,20 @@ export class ForkChoice implements IForkChoice {
while (this.fcStore.currentSlot < currentSlot) {
const previousSlot = this.fcStore.currentSlot;
// Note: we are relying upon `onTick` to update `fcStore.time` to ensure we don't get stuck in a loop.
this.onTick(previousSlot + 1);
const didUpdateCheckpoints = this.onTick(previousSlot + 1);
this.queuedAttestationsPreviousSlot = 0;
// Process any attestations that might now be eligible before running FCR for this slot.
this.processAttestationQueue();
this.runFastConfirmation();
const didRecomputeHead = this.runFastConfirmation();

// An epoch-boundary checkpoint pull-up can move the head's dependent root and stale the cached
// head before block 0 of the new epoch is imported, making isProposerBoostSameDependentRoot()
// wrong for that block. Recompute the head so it reflects the new checkpoint and the queued
// votes — unless fast confirmation already did, to avoid a redundant head calculation.
if (didUpdateCheckpoints && !didRecomputeHead) {
this.updateHead();
}
Comment on lines +1094 to +1096

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this still does it unconditionally at every epoch boundary? I wonder if we should incorporate #9853 or similar in addition, unless there is another case besides dependent root and proposer boost in which we wanna re-compute head

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think #9853 still has its value to use justified checkpoint when it's safe to use it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this still does it unconditionally at every epoch boundary?

yes, I don't think there's an issue with it

I think #9853 still has its value to use justified checkpoint when it's safe to use it.

I'd only do it if we find a scenario for it


this.validatedAttestationDatas = new Set();
}
}
Expand Down Expand Up @@ -1732,23 +1741,31 @@ export class ForkChoice implements IForkChoice {
* May need the justified balances of:
* - unrealizedJustified: Already available in `CheckpointWithBalance`
* Since this balances are already available the getter is just `() => balances`, without cache interaction
*
* @returns Whether either checkpoint was updated.
*/
private updateCheckpoints(
justifiedCheckpoint: CheckpointWithHex,
finalizedCheckpoint: CheckpointWithHex,
getJustifiedBalances: () => JustifiedBalances
): void {
): boolean {
Comment thread
wemeetagain marked this conversation as resolved.
let updated = false;

// Update justified checkpoint.
if (justifiedCheckpoint.epoch > this.fcStore.justified.checkpoint.epoch) {
this.fcStore.justified = {checkpoint: justifiedCheckpoint, balances: getJustifiedBalances()};
this.justifiedProposerBoostScore = null;
updated = true;
}

// Update finalized checkpoint.
if (finalizedCheckpoint.epoch > this.fcStore.finalizedCheckpoint.epoch) {
this.fcStore.finalizedCheckpoint = finalizedCheckpoint;
this.justifiedProposerBoostScore = null;
updated = true;
}

return updated;
}

/**
Expand Down Expand Up @@ -2034,8 +2051,10 @@ export class ForkChoice implements IForkChoice {
* Equivalent to:
*
* https://github.com/ethereum/consensus-specs/blob/v1.1.10/specs/phase0/fork-choice.md#on_tick
*
* @returns Whether an epoch-boundary checkpoint was updated.
*/
private onTick(time: Slot): void {
private onTick(time: Slot): boolean {
const previousSlot = this.fcStore.currentSlot;

if (time > previousSlot + 1) {
Expand All @@ -2056,11 +2075,11 @@ export class ForkChoice implements IForkChoice {

// Not a new epoch, return.
if (computeSlotsSinceEpochStart(time) !== 0) {
return;
return false;
}

// If a new epoch, pull-up justification and finalization from previous epoch
this.updateCheckpoints(
// If a new epoch, pull-up justification and finalization from previous epoch.
return this.updateCheckpoints(
this.fcStore.unrealizedJustified.checkpoint,
this.fcStore.unrealizedFinalizedCheckpoint,
() => this.fcStore.unrealizedJustified.balances
Expand Down Expand Up @@ -2119,10 +2138,11 @@ export class ForkChoice implements IForkChoice {
return {prelimProposerHead};
}

private runFastConfirmation(): void {
/** Returns whether it recomputed the head, so the caller can avoid a redundant `updateHead()`. */
private runFastConfirmation(): boolean {
const fastConfirmationRule = this.fastConfirmationRule;
const fastConfirmationContext = this.fastConfirmationContext;
if (!fastConfirmationRule || !fastConfirmationContext) return;
if (!fastConfirmationRule || !fastConfirmationContext) return false;

if (this.fastConfirmationPaused) {
// Keep consumers on a safe, available root while the rule is paused
Expand All @@ -2133,7 +2153,7 @@ export class ForkChoice implements IForkChoice {
// Runs outside the timed try/catch below; a throw would escape to the clock listener
this.logger?.debug("Fast confirmation notify failed", {slot: this.fcStore.currentSlot}, err as Error);
}
return;
return false;
}

withObservedDuration(this.metrics?.fastConfirmation.totalDuration.startTimer(), () => {
Expand All @@ -2156,6 +2176,8 @@ export class ForkChoice implements IForkChoice {
);
}
});

return true;
}

private createFastConfirmationContext(): FastConfirmationContext {
Expand Down
Loading