fix: use right slot number for future epoch of proposers duties

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/mockedBeaconChain.ts

@@ -105,18 +105,25 @@ vi.mock("../../src/chain/chain.js", async (importActual) => {
   const BeaconChain = vi.fn().mockImplementation(({clock, genesisTime, config}: MockedBeaconChainOptions) => {
     const logger = getMockedLogger();
 
+    const clk =
+      clock === "real"
+        ? new Clock({config, genesisTime: 0, signal: new AbortController().signal})
+        : {
+            get currentSlot() {
+              return 0;
+            },
+            get currentEpoch() {
+              return 0;
+            },
+            currentSlotWithGossipDisparity: undefined,
+            isCurrentSlotGivenGossipDisparity: vi.fn(),
+          };
+
     return {
       config,
       opts: {},
       genesisTime,
-      clock:
-        clock === "real"
-          ? new Clock({config, genesisTime: 0, signal: new AbortController().signal})
-          : {
-              currentSlot: undefined,
-              currentSlotWithGossipDisparity: undefined,
-              isCurrentSlotGivenGossipDisparity: vi.fn(),
-            },
+      clock: clk,
       forkChoice: getMockedForkChoice(),
       executionEngine: {
         notifyForkchoiceUpdate: vi.fn(),
@@ -162,7 +169,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 {MAX_EFFECTIVE_BALANCE, SLOTS_PER_EPOCH, activePreset} 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();