Forkchoice fix bug and cleanup

packages/beacon-node/src/chain/options.ts

@@ -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,

packages/beacon-node/test/perf/chain/opPools/aggregatedAttestationPool.test.ts

@@ -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,

packages/beacon-node/test/spec/presets/fork_choice.ts

@@ -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),
@@ -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
@@ -398,7 +389,6 @@ type Checks = {
     time?: bigint;
     justified_checkpoint?: SpecTestCheckpoint;
     finalized_checkpoint?: SpecTestCheckpoint;
-    best_justified_checkpoint?: SpecTestCheckpoint;
     proposer_boost_root?: RootHex;
   };
 };

packages/beacon-node/test/spec/presets/index.test.ts

@@ -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

packages/cli/src/options/beaconNodeOptions/chain.ts

@@ -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;
@@ -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"],
@@ -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",

packages/cli/test/unit/options/beaconNodeOptions.test.ts

@@ -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,
@@ -111,7 +110,6 @@ describe("options / beaconNodeOptions", () => {
         proposerBoostEnabled: false,
         disableImportExecutionFcU: false,
         computeUnrealized: true,
-        countUnrealizedFull: true,
         safeSlotsToImportOptimistically: 256,
         suggestedFeeRecipient: "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
         assertCorrectProgressiveBalances: true,

packages/fork-choice/src/forkChoice/forkChoice.ts

@@ -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,
@@ -50,7 +45,6 @@ import {IForkChoiceStore, CheckpointWithHex, toCheckpointWithHex, JustifiedBalan
 export type ForkChoiceOpts = {
   proposerBoostEnabled?: boolean;
   computeUnrealized?: boolean;
-  countUnrealizedFull?: boolean;
 };
 
 /**
@@ -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);
   }
 
   /**
@@ -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.
    *
@@ -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:
@@ -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,
@@ -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);
@@ -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
+    );
   }
 
   /**
@@ -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;
     }
   }
@@ -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();
   }
 }
 

packages/fork-choice/src/forkChoice/store.ts

@@ -38,7 +38,6 @@ export type JustifiedBalancesGetter = (
 export interface IForkChoiceStore {
   currentSlot: Slot;
   justified: CheckpointHexWithBalance;
-  bestJustified: CheckpointHexWithBalance;
   unrealizedJustified: CheckpointHexWithBalance;
   finalizedCheckpoint: CheckpointWithHex;
   unrealizedFinalizedCheckpoint: CheckpointWithHex;
@@ -51,7 +50,6 @@ export interface IForkChoiceStore {
  */
 export class ForkChoiceStore implements IForkChoiceStore {
   private _justified: CheckpointHexWithBalance;
-  bestJustified: CheckpointHexWithBalance;
   unrealizedJustified: CheckpointHexWithBalance;
   private _finalizedCheckpoint: CheckpointWithHex;
   unrealizedFinalizedCheckpoint: CheckpointWithHex;
@@ -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;