fix: use right slot number for future epoch of proposers duties (#6545)

* Use the right slot number for future epoch for duties

* Add unit tests for block propoer duties

* Update the clock util

* Fix the lint error

packages/beacon-node/src/api/impl/validator/index.ts

@@ -72,7 +72,7 @@ import {computeSubnetForCommitteesAtSlot, getPubkeysForIndices, selectBlockProdu
  * they are 8 epochs apart) and causes an OOM. Research a proper solution once regen and the state
  * caches are better.
  */
-const SYNC_TOLERANCE_EPOCHS = 1;
+export const SYNC_TOLERANCE_EPOCHS = 1;
 
 /**
  * Cutoff time to wait for execution and builder block production apis to resolve
@@ -912,7 +912,7 @@ export function getValidatorApi({
       // TODO: Add a flag to just send 0x00 as pubkeys since the Lodestar validator does not need them.
       const pubkeys = getPubkeysForIndices(state.validators, indexes);
 
-      const startSlot = computeStartSlotAtEpoch(stateEpoch);
+      const startSlot = computeStartSlotAtEpoch(epoch);
       const duties: routes.validator.ProposerDuty[] = [];
       for (let i = 0; i < SLOTS_PER_EPOCH; i++) {
         duties.push({slot: startSlot + i, validatorIndex: indexes[i], pubkey: pubkeys[i]});

packages/beacon-node/test/mocks/beaconSyncMock.ts

@@ -8,7 +8,9 @@ vi.mock("../../src/sync/index.js", async (importActual) => {
   const mod = await importActual<typeof import("../../src/sync/index.js")>();
 
   const BeaconSync = vi.fn().mockImplementation(() => {
-    const sync = {};
+    const sync = {
+      isSynced: vi.fn(),
+    };
     Object.defineProperty(sync, "state", {value: undefined, configurable: true});
 
     return sync;

packages/beacon-node/test/mocks/clock.ts

@@ -1,4 +1,5 @@
 import EventEmitter from "node:events";
+import {Mocked, vi} from "vitest";
 import {computeEpochAtSlot} from "@lodestar/state-transition";
 import {Epoch, Slot} from "@lodestar/types";
 import {IClock} from "../../src/util/clock.js";
@@ -62,3 +63,16 @@ export class ClockStopped extends EventEmitter implements IClock {
     this.slot = slot;
   }
 }
+
+export function getMockedClock(): Mocked<IClock> {
+  return {
+    get currentSlot() {
+      return 0;
+    },
+    get currentEpoch() {
+      return 0;
+    },
+    currentSlotWithGossipDisparity: undefined,
+    isCurrentSlotGivenGossipDisparity: vi.fn(),
+  } as unknown as Mocked<IClock>;
+}

packages/beacon-node/test/mocks/mockedBeaconChain.ts

@@ -16,6 +16,7 @@ import {Clock} from "../../src/util/clock.js";
 import {QueuedStateRegenerator} from "../../src/chain/regen/index.js";
 import {ShufflingCache} from "../../src/chain/shufflingCache.js";
 import {getMockedLogger} from "./loggerMock.js";
+import {getMockedClock} from "./clock.js";
 
 export type MockedBeaconChain = Mocked<BeaconChain> & {
   logger: Mocked<Logger>;
@@ -110,13 +111,7 @@ vi.mock("../../src/chain/chain.js", async (importActual) => {
       opts: {},
       genesisTime,
       clock:
-        clock === "real"
-          ? new Clock({config, genesisTime: 0, signal: new AbortController().signal})
-          : {
-              currentSlot: undefined,
-              currentSlotWithGossipDisparity: undefined,
-              isCurrentSlotGivenGossipDisparity: vi.fn(),
-            },
+        clock === "real" ? new Clock({config, genesisTime, signal: new AbortController().signal}) : getMockedClock(),
       forkChoice: getMockedForkChoice(),
       executionEngine: {
         notifyForkchoiceUpdate: vi.fn(),
@@ -162,7 +157,7 @@ vi.mock("../../src/chain/chain.js", async (importActual) => {
   };
 });
 
-type MockedBeaconChainOptions = {
+export type MockedBeaconChainOptions = {
   clock: "real" | "fake";
   genesisTime: number;
   config: ChainForkConfig;

packages/beacon-node/test/unit/api/impl/validator/duties/proposer.test.ts

@@ -1,30 +1,27 @@
-import {describe, it, expect, beforeEach, vi} from "vitest";
+import {describe, it, expect, beforeEach, afterEach, vi} from "vitest";
 import {config} from "@lodestar/config/default";
 import {MAX_EFFECTIVE_BALANCE, SLOTS_PER_EPOCH} from "@lodestar/params";
+import {BeaconStateAllForks} from "@lodestar/state-transition";
 import {ApiTestModules, getApiTestModules} from "../../../../../utils/api.js";
 import {FAR_FUTURE_EPOCH} from "../../../../../../src/constants/index.js";
-import {getValidatorApi} from "../../../../../../src/api/impl/validator/index.js";
+import {SYNC_TOLERANCE_EPOCHS, getValidatorApi} from "../../../../../../src/api/impl/validator/index.js";
 import {generateState, zeroProtoBlock} from "../../../../../utils/state.js";
 import {generateValidators} from "../../../../../utils/validator.js";
 import {createCachedBeaconStateTest} from "../../../../../utils/cachedBeaconState.js";
+import {SyncState} from "../../../../../../src/sync/interface.js";
 
-describe.skip("get proposers api impl", function () {
+describe("get proposers api impl", function () {
   let api: ReturnType<typeof getValidatorApi>;
   let modules: ApiTestModules;
+  let state: BeaconStateAllForks;
+  let cachedState: ReturnType<typeof createCachedBeaconStateTest>;
 
   beforeEach(function () {
-    modules = getApiTestModules();
+    vi.useFakeTimers({now: 0});
+    modules = getApiTestModules({clock: "real"});
     api = getValidatorApi(modules);
 
-    modules.forkChoice.getHead.mockReturnValue(zeroProtoBlock);
-  });
-
-  it("should get proposers for next epoch", async function () {
-    modules.sync.isSynced.mockReturnValue(true);
-    vi.spyOn(modules.chain.clock, "currentEpoch", "get").mockReturnValue(0);
-    vi.spyOn(modules.chain.clock, "currentSlot", "get").mockReturnValue(0);
-    modules.db.block.get.mockResolvedValue({message: {stateRoot: Buffer.alloc(32)}} as any);
-    const state = generateState(
+    state = generateState(
       {
         slot: 0,
         validators: generateValidators(25, {
@@ -36,67 +33,76 @@ describe.skip("get proposers api impl", function () {
       },
       config
     );
+    cachedState = createCachedBeaconStateTest(state, config);
 
-    const cachedState = createCachedBeaconStateTest(state, config);
     modules.chain.getHeadStateAtCurrentEpoch.mockResolvedValue(cachedState);
-    const stubGetNextBeaconProposer = vi.spyOn(cachedState.epochCtx, "getBeaconProposersNextEpoch");
-    const stubGetBeaconProposer = vi.spyOn(cachedState.epochCtx, "getBeaconProposer");
-    stubGetNextBeaconProposer.mockReturnValue([1]);
+    modules.forkChoice.getHead.mockReturnValue(zeroProtoBlock);
+    modules.db.block.get.mockResolvedValue({message: {stateRoot: Buffer.alloc(32)}} as any);
+
+    vi.spyOn(modules.sync, "state", "get").mockReturnValue(SyncState.Synced);
+    vi.spyOn(cachedState.epochCtx, "getBeaconProposersNextEpoch");
+    vi.spyOn(cachedState.epochCtx, "getBeaconProposers");
+  });
+
+  afterEach(() => {
+    vi.useRealTimers();
+  });
+
+  it("should raise error if node head is behind", async () => {
+    vi.advanceTimersByTime((SYNC_TOLERANCE_EPOCHS * SLOTS_PER_EPOCH + 1) * config.SECONDS_PER_SLOT * 1000);
+    vi.spyOn(modules.sync, "state", "get").mockReturnValue(SyncState.SyncingHead);
+
+    await expect(api.getProposerDuties(1)).rejects.toThrow("Node is syncing - headSlot 0 currentSlot 9");
+  });
+
+  it("should raise error if node stalled", async () => {
+    vi.advanceTimersByTime((SYNC_TOLERANCE_EPOCHS * SLOTS_PER_EPOCH + 1) * config.SECONDS_PER_SLOT * 1000);
+    vi.spyOn(modules.sync, "state", "get").mockReturnValue(SyncState.Stalled);
+
+    await expect(api.getProposerDuties(1)).rejects.toThrow("Node is syncing - waiting for peers");
+  });
+
+  it("should get proposers for current epoch", async () => {
+    const {data: result} = await api.getProposerDuties(0);
+
+    expect(result.length).toBe(SLOTS_PER_EPOCH);
+    expect(cachedState.epochCtx.getBeaconProposers).toHaveBeenCalledOnce();
+    expect(cachedState.epochCtx.getBeaconProposersNextEpoch).not.toHaveBeenCalled();
+    expect(result.map((p) => p.slot)).toEqual(Array.from({length: SLOTS_PER_EPOCH}, (_, i) => i));
+  });
+
+  it("should get proposers for next epoch", async () => {
     const {data: result} = await api.getProposerDuties(1);
+
     expect(result.length).toBe(SLOTS_PER_EPOCH);
-    // "stubGetBeaconProposer function should not have been called"
-    expect(stubGetNextBeaconProposer).toHaveBeenCalledWith();
-    // "stubGetBeaconProposer function should have been called"
-    expect(stubGetBeaconProposer).not.toHaveBeenCalledWith();
+    expect(cachedState.epochCtx.getBeaconProposers).not.toHaveBeenCalled();
+    expect(cachedState.epochCtx.getBeaconProposersNextEpoch).toHaveBeenCalledOnce();
+    expect(result.map((p) => p.slot)).toEqual(Array.from({length: SLOTS_PER_EPOCH}, (_, i) => SLOTS_PER_EPOCH + i));
   });
 
-  it("should have different proposer for current and next epoch", async function () {
-    modules.sync.isSynced.mockReturnValue(true);
-    vi.spyOn(modules.chain.clock, "currentEpoch", "get").mockReturnValue(0);
-    vi.spyOn(modules.chain.clock, "currentSlot", "get").mockReturnValue(0);
-    modules.db.block.get.mockResolvedValue({message: {stateRoot: Buffer.alloc(32)}} as any);
-    const state = generateState(
-      {
-        slot: 0,
-        validators: generateValidators(25, {
-          effectiveBalance: MAX_EFFECTIVE_BALANCE,
-          activationEpoch: 0,
-          exitEpoch: FAR_FUTURE_EPOCH,
-        }),
-        balances: Array.from({length: 25}, () => MAX_EFFECTIVE_BALANCE),
-      },
-      config
-    );
-    const cachedState = createCachedBeaconStateTest(state, config);
-    modules.chain.getHeadStateAtCurrentEpoch.mockResolvedValue(cachedState);
-    const stubGetBeaconProposer = vi.spyOn(cachedState.epochCtx, "getBeaconProposer");
-    stubGetBeaconProposer.mockReturnValue(1);
+  it("should raise error for more than one epoch in the future", async () => {
+    await expect(api.getProposerDuties(2)).rejects.toThrow("Requested epoch 2 must equal current 0 or next epoch 1");
+  });
+
+  it("should have different proposer validator public keys for current and next epoch", async () => {
     const {data: currentProposers} = await api.getProposerDuties(0);
     const {data: nextProposers} = await api.getProposerDuties(1);
-    expect(currentProposers).not.toEqual(nextProposers);
+
+    // Public keys should be different, but for tests we are generating a static list of validators with same public key
+    expect(currentProposers.map((p) => p.pubkey)).toEqual(nextProposers.map((p) => p.pubkey));
   });
 
-  it("should not get proposers for more than one epoch in the future", async function () {
-    modules.sync.isSynced.mockReturnValue(true);
-    vi.spyOn(modules.chain.clock, "currentEpoch", "get").mockReturnValue(0);
-    vi.spyOn(modules.chain.clock, "currentSlot", "get").mockReturnValue(0);
-    modules.db.block.get.mockResolvedValue({message: {stateRoot: Buffer.alloc(32)}} as any);
-    const state = generateState(
-      {
-        slot: 0,
-        validators: generateValidators(25, {
-          effectiveBalance: MAX_EFFECTIVE_BALANCE,
-          activationEpoch: 0,
-          exitEpoch: FAR_FUTURE_EPOCH,
-        }),
-        balances: Array.from({length: 25}, () => MAX_EFFECTIVE_BALANCE),
-      },
-      config
-    );
-    const cachedState = createCachedBeaconStateTest(state, config);
-    modules.chain.getHeadStateAtCurrentEpoch.mockResolvedValue(cachedState);
-    const stubGetBeaconProposer = vi.spyOn(cachedState.epochCtx, "getBeaconProposer");
-    await expect(stubGetBeaconProposer).rejects.toThrow();
-    await expect(api.getProposerDuties(2)).rejects.toThrow();
+  it("should have different proposer validator indexes for current and next epoch", async () => {
+    const {data: currentProposers} = await api.getProposerDuties(0);
+    const {data: nextProposers} = await api.getProposerDuties(1);
+
+    expect(currentProposers.map((p) => p.validatorIndex)).not.toEqual(nextProposers.map((p) => p.validatorIndex));
+  });
+
+  it("should have different proposer slots for current and next epoch", async () => {
+    const {data: currentProposers} = await api.getProposerDuties(0);
+    const {data: nextProposers} = await api.getProposerDuties(1);
+
+    expect(currentProposers.map((p) => p.slot)).not.toEqual(nextProposers.map((p) => p.slot));
   });
 });

packages/beacon-node/test/utils/api.ts

@@ -1,7 +1,7 @@
 import {Mocked} from "vitest";
 import {config} from "@lodestar/config/default";
 import {ForkChoice} from "@lodestar/fork-choice";
-import {MockedBeaconChain, getMockedBeaconChain} from "../mocks/mockedBeaconChain.js";
+import {MockedBeaconChain, MockedBeaconChainOptions, getMockedBeaconChain} from "../mocks/mockedBeaconChain.js";
 import {getMockedBeaconSync} from "../mocks/beaconSyncMock.js";
 import {MockedBeaconDb, getMockedBeaconDb} from "../mocks/mockedBeaconDb.js";
 import {getMockedNetwork} from "../mocks/mockedNetwork.js";
@@ -16,8 +16,8 @@ export type ApiTestModules = {[K in keyof ApiModulesWithoutConfig]: Mocked<ApiMo
   config: ApiModules["config"];
 };
 
-export function getApiTestModules(): ApiTestModules {
-  const chainStub = getMockedBeaconChain();
+export function getApiTestModules(opts?: Partial<MockedBeaconChainOptions>): ApiTestModules {
+  const chainStub = getMockedBeaconChain(opts);
   const syncStub = getMockedBeaconSync();
   const dbStub = getMockedBeaconDb();
   const networkStub = getMockedNetwork();