Skip to content

Commit

Permalink
Forkchoice fix bug and cleanup (#5126)
Browse files Browse the repository at this point in the history 
* Fix forkchoice bugs and clean up

* Fix justified balances

* Refactor variable name in nodeIsViableForHead

* Remove countUnrealizedFull flag

* Add getAncestorOrNull()

* Remove SAFE_SLOTS_TO_UPDATE_JUSTIFIED

* Fix unrealized checkpoint reused from parent

* Remove best_justified_checkpoint step from forkchoice spec test

* Reenable forkchoice tests

* Revert forkchoice unit test modification

* Update specConfigCommit

* Fix e2e in params

* computeUnrealized: use default chain option
  • Loading branch information
@twoeths
twoeths authored Mar 19, 2023
1 parent 9f7b830 commit 8bc1f69
Show file tree
Hide file tree
Showing 19 changed files with 116 additions and 243 deletions.
1 change: 0 additions & 1 deletion packages/beacon-node/src/chain/options.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ export const defaultChainOptions: IChainOptions = {
disableBlsBatchVerify: false,
proposerBoostEnabled: true,
computeUnrealized: true,
countUnrealizedFull: false,
safeSlotsToImportOptimistically: SAFE_SLOTS_TO_IMPORT_OPTIMISTICALLY,
suggestedFeeRecipient: defaultValidatorOptions.suggestedFeeRecipient,
assertCorrectProgressiveBalances: false,
Expand Down
4 changes: 0 additions & 4 deletions packages/beacon-node/test/perf/chain/opPools/aggregatedAttestationPool.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,10 +107,6 @@ describe("getAttestationsForBlock", () => {
checkpoint: {...justifiedCheckpoint, rootHex: toHexString(justifiedCheckpoint.root)},
balances: originalState.epochCtx.effectiveBalanceIncrements,
},
bestJustified: {
checkpoint: {...justifiedCheckpoint, rootHex: toHexString(justifiedCheckpoint.root)},
balances: originalState.epochCtx.effectiveBalanceIncrements,
},
unrealizedJustified: {
checkpoint: {...justifiedCheckpoint, rootHex: toHexString(justifiedCheckpoint.root)},
balances: originalState.epochCtx.effectiveBalanceIncrements,
Expand Down
10 changes: 0 additions & 10 deletions packages/beacon-node/test/spec/presets/fork_choice.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,6 @@ export const forkChoiceTest = (opts: {onlyPredefinedResponses: boolean}): TestRu
// we don't use these in fork choice spec tests
disablePrepareNextSlot: true,
assertCorrectProgressiveBalances,
computeUnrealized: false,
},
{
config: createBeaconConfig(config, state.genesisValidatorsRoot),
Expand Down Expand Up @@ -246,14 +245,6 @@ export const forkChoiceTest = (opts: {onlyPredefinedResponses: boolean}): TestRu
`Invalid finalized checkpoint at step ${i}`
);
}
if (step.checks.best_justified_checkpoint) {
expect(
toSpecTestCheckpoint((chain.forkChoice as ForkChoice).getBestJustifiedCheckpoint())
).to.be.deep.equal(
step.checks.best_justified_checkpoint,
`Invalid best justified checkpoint at step ${i}`
);
}
}

// None of the above
Expand Down Expand Up @@ -398,7 +389,6 @@ type Checks = {
time?: bigint;
justified_checkpoint?: SpecTestCheckpoint;
finalized_checkpoint?: SpecTestCheckpoint;
best_justified_checkpoint?: SpecTestCheckpoint;
proposer_boost_root?: RootHex;
};
};
Expand Down
3 changes: 0 additions & 3 deletions packages/beacon-node/test/spec/presets/index.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,6 @@ import {transition} from "./transition.js";
const skipOpts: SkipOpts = {
// To be enabled in decouple blobs PR: https://github.com/ChainSafe/lodestar/pull/5181
skippedForks: ["deneb"],
// To be enabled with the fork choice safe slots to justified removal PR
// https://github.com/ChainSafe/lodestar/pull/5126
skippedRunners: ["fork_choice"],
// TODO: capella
// BeaconBlockBody proof in lightclient is the new addition in v1.3.0-rc.2-hotfix
// Skip them for now to enable subsequently
Expand Down
10 changes: 0 additions & 10 deletions packages/cli/src/options/beaconNodeOptions/chain.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ export type ChainArgs = {
"chain.proposerBoostEnabled": boolean;
"chain.disableImportExecutionFcU": boolean;
"chain.computeUnrealized": boolean;
"chain.countUnrealizedFull": boolean;
"chain.assertCorrectProgressiveBalances": boolean;
"chain.maxSkipSlots": number;
"safe-slots-to-import-optimistically": number;
Expand All @@ -32,7 +31,6 @@ export function parseArgs(args: ChainArgs): IBeaconNodeOptions["chain"] {
proposerBoostEnabled: args["chain.proposerBoostEnabled"],
disableImportExecutionFcU: args["chain.disableImportExecutionFcU"],
computeUnrealized: args["chain.computeUnrealized"],
countUnrealizedFull: args["chain.countUnrealizedFull"],
assertCorrectProgressiveBalances: args["chain.assertCorrectProgressiveBalances"],
maxSkipSlots: args["chain.maxSkipSlots"],
safeSlotsToImportOptimistically: args["safe-slots-to-import-optimistically"],
Expand Down Expand Up @@ -105,14 +103,6 @@ Will double processing times. Use only for debugging purposes.",
group: "chain",
},

"chain.countUnrealizedFull": {
hidden: true,
type: "boolean",
description: "Compute unrealized checkpoints and fully use it",
defaultDescription: String(defaultOptions.chain.computeUnrealized),
group: "chain",
},

"chain.maxSkipSlots": {
hidden: true,
type: "number",
Expand Down
2 changes: 0 additions & 2 deletions packages/cli/test/unit/options/beaconNodeOptions.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ describe("options / beaconNodeOptions", () => {
"chain.proposerBoostEnabled": false,
"chain.disableImportExecutionFcU": false,
"chain.computeUnrealized": true,
"chain.countUnrealizedFull": true,
suggestedFeeRecipient: "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
"chain.assertCorrectProgressiveBalances": true,
"chain.maxSkipSlots": 100,
Expand Down Expand Up @@ -111,7 +110,6 @@ describe("options / beaconNodeOptions", () => {
proposerBoostEnabled: false,
disableImportExecutionFcU: false,
computeUnrealized: true,
countUnrealizedFull: true,
safeSlotsToImportOptimistically: 256,
suggestedFeeRecipient: "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
assertCorrectProgressiveBalances: true,
Expand Down
148 changes: 20 additions & 128 deletions packages/fork-choice/src/forkChoice/forkChoice.ts
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,6 @@
import {toHexString} from "@chainsafe/ssz";
import {fromHex} from "@lodestar/utils";
import {
SAFE_SLOTS_TO_UPDATE_JUSTIFIED,
SLOTS_PER_HISTORICAL_ROOT,
SLOTS_PER_EPOCH,
INTERVALS_PER_SLOT,
} from "@lodestar/params";
import {SLOTS_PER_HISTORICAL_ROOT, SLOTS_PER_EPOCH, INTERVALS_PER_SLOT} from "@lodestar/params";
import {bellatrix, Slot, ValidatorIndex, phase0, allForks, ssz, RootHex, Epoch, Root} from "@lodestar/types";
import {
computeSlotsSinceEpochStart,
Expand Down Expand Up @@ -50,7 +45,6 @@ import {IForkChoiceStore, CheckpointWithHex, toCheckpointWithHex, JustifiedBalan
export type ForkChoiceOpts = {
proposerBoostEnabled?: boolean;
computeUnrealized?: boolean;
countUnrealizedFull?: boolean;
};

/**
Expand Down Expand Up @@ -131,31 +125,7 @@ export class ForkChoice implements IForkChoice {
* https://github.com/ethereum/consensus-specs/blob/v1.1.10/specs/phase0/fork-choice.md#get_ancestor
*/
getAncestor(blockRoot: RootHex, ancestorSlot: Slot): RootHex {
const block = this.protoArray.getBlock(blockRoot);
if (!block) {
throw new ForkChoiceError({
code: ForkChoiceErrorCode.MISSING_PROTO_ARRAY_BLOCK,
root: blockRoot,
});
}

if (block.slot > ancestorSlot) {
// Search for a slot that is lte the target slot.
// We check for lower slots to account for skip slots.
for (const node of this.protoArray.iterateAncestorNodes(blockRoot)) {
if (node.slot <= ancestorSlot) {
return node.blockRoot;
}
}
throw new ForkChoiceError({
code: ForkChoiceErrorCode.UNKNOWN_ANCESTOR,
descendantRoot: blockRoot,
ancestorSlot,
});
} else {
// Root is older or equal than queried slot, thus a skip slot. Return most recent root prior to slot.
return blockRoot;
}
return this.protoArray.getAncestor(blockRoot, ancestorSlot);
}

/**
Expand Down Expand Up @@ -284,10 +254,6 @@ export class ForkChoice implements IForkChoice {
return this.fcStore.justified.checkpoint;
}

getBestJustifiedCheckpoint(): CheckpointWithHex {
return this.fcStore.bestJustified.checkpoint;
}

/**
* Add `block` to the fork choice DAG.
*
Expand Down Expand Up @@ -394,20 +360,19 @@ export class ForkChoice implements IForkChoice {
// 1. Its prudent to fail fast and not try importing a block in forkChoice.
// 2. Also the data to run such a validation is readily available there.

const currentJustifiedCheckpoint = toCheckpointWithHex(state.currentJustifiedCheckpoint);
const stateJustifiedEpoch = currentJustifiedCheckpoint.epoch;

const justifiedCheckpoint = toCheckpointWithHex(state.currentJustifiedCheckpoint);
const finalizedCheckpoint = toCheckpointWithHex(state.finalizedCheckpoint);
const stateJustifiedEpoch = justifiedCheckpoint.epoch;

// Justified balances for `justifiedCheckpoint` are new to the fork-choice. Compute them on demand only if
// the justified checkpoint changes
this.updateCheckpoints(state.slot, justifiedCheckpoint, finalizedCheckpoint, () =>
this.updateCheckpoints(justifiedCheckpoint, finalizedCheckpoint, () =>
this.fcStore.justifiedBalancesGetter(justifiedCheckpoint, state)
);

const blockEpoch = computeEpochAtSlot(slot);

// same logic to compute_pulled_up_tip because a lot of varible could be reused here
// If the parent checkpoints are already at the same epoch as the block being imported,
// it's impossible for the unrealized checkpoints to differ from the parent's. This
// holds true because:
Expand All @@ -422,7 +387,10 @@ export class ForkChoice implements IForkChoice {
let unrealizedJustifiedCheckpoint: CheckpointWithHex;
let unrealizedFinalizedCheckpoint: CheckpointWithHex;
if (this.opts?.computeUnrealized) {
if (parentBlock.unrealizedJustifiedEpoch === blockEpoch && parentBlock.unrealizedFinalizedEpoch >= blockEpoch) {
if (
parentBlock.unrealizedJustifiedEpoch === blockEpoch &&
parentBlock.unrealizedFinalizedEpoch + 1 >= blockEpoch
) {
// reuse from parent, happens at 1/3 last blocks of epoch as monitored in mainnet
unrealizedJustifiedCheckpoint = {
epoch: parentBlock.unrealizedJustifiedEpoch,
Expand Down Expand Up @@ -459,13 +427,7 @@ export class ForkChoice implements IForkChoice {

// If block is from past epochs, try to update store's justified & finalized checkpoints right away
if (blockEpoch < computeEpochAtSlot(currentSlot)) {
// Compute justified balances for unrealizedJustifiedCheckpoint on demand
if (unrealizedJustifiedCheckpoint === undefined) {
throw Error();
}
this.updateCheckpoints(state.slot, unrealizedJustifiedCheckpoint, unrealizedFinalizedCheckpoint, () =>
this.fcStore.justifiedBalancesGetter(unrealizedJustifiedCheckpoint as CheckpointWithHex, state)
);
this.pullUpStoreCheckpoints();
}

const targetSlot = computeStartSlotAtEpoch(blockEpoch);
Expand Down Expand Up @@ -884,54 +846,13 @@ export class ForkChoice implements IForkChoice {
return executionStatus;
}

/**
* Returns `true` if the given `store` should be updated to set
* `state.current_justified_checkpoint` its `justified_checkpoint`.
*
* ## Specification
*
* Is equivalent to:
*
* https://github.com/ethereum/consensus-specs/blob/v1.1.10/specs/phase0/fork-choice.md#should_update_justified_checkpoint
*/
private shouldUpdateJustifiedCheckpoint(newJustifiedCheckpoint: CheckpointWithHex, stateSlot: Slot): boolean {
// To address the bouncing attack, only update conflicting justified checkpoints in the first 1/3 of the epoch.
// Otherwise, delay consideration until the next epoch boundary with bestJustifiedCheckpoint
// See https://ethresear.ch/t/prevention-of-bouncing-attack-on-ffg/6114 for more detailed analysis and discussion.
if (computeSlotsSinceEpochStart(this.fcStore.currentSlot) < SAFE_SLOTS_TO_UPDATE_JUSTIFIED) {
return true;
}

const justifiedSlot = computeStartSlotAtEpoch(this.fcStore.justified.checkpoint.epoch);

// This sanity check is not in the spec, but the invariant is implied
if (justifiedSlot >= stateSlot) {
throw new ForkChoiceError({
code: ForkChoiceErrorCode.ATTEMPT_TO_REVERT_JUSTIFICATION,
store: justifiedSlot,
state: stateSlot,
});
}

// at regular sync time we don't want to wait for clock time next epoch to update bestJustifiedCheckpoint
if (computeEpochAtSlot(stateSlot) < computeEpochAtSlot(this.fcStore.currentSlot)) {
return true;
}

// We know that the slot for `new_justified_checkpoint.root` is not greater than
// `state.slot`, since a state cannot justify its own slot.
//
// We know that `new_justified_checkpoint.root` is an ancestor of `state`, since a `state`
// only ever justifies ancestors.
//
// A prior `if` statement protects against a justified_slot that is greater than
// `state.slot`
const justifiedAncestor = this.getAncestor(toHexString(newJustifiedCheckpoint.root), justifiedSlot);
if (justifiedAncestor !== this.fcStore.justified.checkpoint.rootHex) {
return false;
}

return true;
private pullUpStoreCheckpoints(): void {
this.updateCheckpoints(
this.fcStore.unrealizedJustified.checkpoint,
this.fcStore.unrealizedFinalizedCheckpoint,
// Provide pre-computed balances for unrealizedJustified, will never trigger .justifiedBalancesGetter()
() => this.fcStore.unrealizedJustified.balances
);
}

/**
Expand All @@ -951,32 +872,23 @@ export class ForkChoice implements IForkChoice {
*
* **`on_tick`**
* May need the justified balances of:
* - bestJustified: Already available in `CheckpointHexWithBalance`
* - unrealizedJustified: Already available in `CheckpointHexWithBalance`
* Since this balances are already available the getter is just `() => balances`, without cache interaction
*/
private updateCheckpoints(
stateSlot: Slot,
justifiedCheckpoint: CheckpointWithHex,
finalizedCheckpoint: CheckpointWithHex,
getJustifiedBalances: () => JustifiedBalances
): void {
// Update justified checkpoint.
if (justifiedCheckpoint.epoch > this.fcStore.justified.checkpoint.epoch) {
if (justifiedCheckpoint.epoch > this.fcStore.bestJustified.checkpoint.epoch) {
this.fcStore.bestJustified = {checkpoint: justifiedCheckpoint, balances: getJustifiedBalances()};
}

if (this.shouldUpdateJustifiedCheckpoint(justifiedCheckpoint, stateSlot)) {
this.fcStore.justified = {checkpoint: justifiedCheckpoint, balances: getJustifiedBalances()};
this.justifiedProposerBoostScore = null;
}
this.fcStore.justified = {checkpoint: justifiedCheckpoint, balances: getJustifiedBalances()};
this.justifiedProposerBoostScore = null;
}

// Update finalized checkpoint.
if (finalizedCheckpoint.epoch > this.fcStore.finalizedCheckpoint.epoch) {
this.fcStore.finalizedCheckpoint = finalizedCheckpoint;
this.fcStore.justified = {checkpoint: justifiedCheckpoint, balances: getJustifiedBalances()};
this.justifiedProposerBoostScore = null;
}
}
Expand Down Expand Up @@ -1209,28 +1121,8 @@ export class ForkChoice implements IForkChoice {
return;
}

// Reason: A better justifiedCheckpoint from a block is only updated immediately if in the first 1/3 of the epoch
// This addresses a bouncing attack, see https://ethresear.ch/t/prevention-of-bouncing-attack-on-ffg/6114
if (this.fcStore.bestJustified.checkpoint.epoch > this.fcStore.justified.checkpoint.epoch) {
// TODO: Is this check necessary? It checks that bestJustifiedCheckpoint is still descendant of finalized
// From https://github.com/ChainSafe/lodestar/commit/6a0745e9db27dfce67b6e6c25bba452283dbbea9#
const finalizedSlot = computeStartSlotAtEpoch(this.fcStore.finalizedCheckpoint.epoch);
const ancestorAtFinalizedSlot = this.getAncestor(this.fcStore.bestJustified.checkpoint.rootHex, finalizedSlot);
if (ancestorAtFinalizedSlot === this.fcStore.finalizedCheckpoint.rootHex) {
// Provide pre-computed balances for bestJustified, will never trigger .justifiedBalancesGetter()
this.fcStore.justified = this.fcStore.bestJustified;
this.justifiedProposerBoostScore = null;
}
}

// Update store.justified_checkpoint if a better unrealized justified checkpoint is known
this.updateCheckpoints(
time,
this.fcStore.unrealizedJustified.checkpoint,
this.fcStore.unrealizedFinalizedCheckpoint,
// Provide pre-computed balances for unrealizedJustified, will never trigger .justifiedBalancesGetter()
() => this.fcStore.unrealizedJustified.balances
);
this.pullUpStoreCheckpoints();
}
}

Expand Down
3 changes: 0 additions & 3 deletions packages/fork-choice/src/forkChoice/store.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ export type JustifiedBalancesGetter = (
export interface IForkChoiceStore {
currentSlot: Slot;
justified: CheckpointHexWithBalance;
bestJustified: CheckpointHexWithBalance;
unrealizedJustified: CheckpointHexWithBalance;
finalizedCheckpoint: CheckpointWithHex;
unrealizedFinalizedCheckpoint: CheckpointWithHex;
Expand All @@ -51,7 +50,6 @@ export interface IForkChoiceStore {
*/
export class ForkChoiceStore implements IForkChoiceStore {
private _justified: CheckpointHexWithBalance;
bestJustified: CheckpointHexWithBalance;
unrealizedJustified: CheckpointHexWithBalance;
private _finalizedCheckpoint: CheckpointWithHex;
unrealizedFinalizedCheckpoint: CheckpointWithHex;
Expand All @@ -73,7 +71,6 @@ export class ForkChoiceStore implements IForkChoiceStore {
balances: justifiedBalances,
};
this._justified = justified;
this.bestJustified = justified;
this.unrealizedJustified = justified;
this._finalizedCheckpoint = toCheckpointWithHex(finalizedCheckpoint);
this.unrealizedFinalizedCheckpoint = this._finalizedCheckpoint;
Expand Down
Loading

0 comments on commit 8bc1f69

Please sign in to comment.