From 1a07627a06d1763b1c6d3af778ed0603138e35ec Mon Sep 17 00:00:00 2001 From: Nazar Hussain Date: Fri, 15 Mar 2024 06:58:31 +0100 Subject: [PATCH 1/4] Use the right slot number for future epoch for duties --- packages/beacon-node/src/api/impl/validator/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/beacon-node/src/api/impl/validator/index.ts b/packages/beacon-node/src/api/impl/validator/index.ts index 553146a6cc5..cd4c199c0f3 100644 --- a/packages/beacon-node/src/api/impl/validator/index.ts +++ b/packages/beacon-node/src/api/impl/validator/index.ts @@ -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]}); From 594cb885ec987f97f928dc54c98b8d8ccf32a9a1 Mon Sep 17 00:00:00 2001 From: Nazar Hussain Date: Fri, 15 Mar 2024 08:06:17 +0100 Subject: [PATCH 2/4] Add unit tests for block propoer duties --- .../src/api/impl/validator/index.ts | 2 +- .../beacon-node/test/mocks/beaconSyncMock.ts | 4 +- .../test/mocks/mockedBeaconChain.ts | 25 ++-- .../impl/validator/duties/proposer.test.ts | 138 +++++++++--------- packages/beacon-node/test/utils/api.ts | 6 +- 5 files changed, 95 insertions(+), 80 deletions(-) diff --git a/packages/beacon-node/src/api/impl/validator/index.ts b/packages/beacon-node/src/api/impl/validator/index.ts index cd4c199c0f3..2aef5616911 100644 --- a/packages/beacon-node/src/api/impl/validator/index.ts +++ b/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 diff --git a/packages/beacon-node/test/mocks/beaconSyncMock.ts b/packages/beacon-node/test/mocks/beaconSyncMock.ts index 00164afc5d6..00ef193fdf6 100644 --- a/packages/beacon-node/test/mocks/beaconSyncMock.ts +++ b/packages/beacon-node/test/mocks/beaconSyncMock.ts @@ -8,7 +8,9 @@ vi.mock("../../src/sync/index.js", async (importActual) => { const mod = await importActual(); const BeaconSync = vi.fn().mockImplementation(() => { - const sync = {}; + const sync = { + isSynced: vi.fn(), + }; Object.defineProperty(sync, "state", {value: undefined, configurable: true}); return sync; diff --git a/packages/beacon-node/test/mocks/mockedBeaconChain.ts b/packages/beacon-node/test/mocks/mockedBeaconChain.ts index 21881875ba6..154de5a8acb 100644 --- a/packages/beacon-node/test/mocks/mockedBeaconChain.ts +++ b/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; diff --git a/packages/beacon-node/test/unit/api/impl/validator/duties/proposer.test.ts b/packages/beacon-node/test/unit/api/impl/validator/duties/proposer.test.ts index 27daaadae1f..af91f1262be 100644 --- a/packages/beacon-node/test/unit/api/impl/validator/duties/proposer.test.ts +++ b/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; let modules: ApiTestModules; + let state: BeaconStateAllForks; + let cachedState: ReturnType; 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)); }); }); diff --git a/packages/beacon-node/test/utils/api.ts b/packages/beacon-node/test/utils/api.ts index 7429ac79189..593bae0c264 100644 --- a/packages/beacon-node/test/utils/api.ts +++ b/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): ApiTestModules { + const chainStub = getMockedBeaconChain(opts); const syncStub = getMockedBeaconSync(); const dbStub = getMockedBeaconDb(); const networkStub = getMockedNetwork(); From c0887de07db45ebaf07020cc39b626d00fc253d1 Mon Sep 17 00:00:00 2001 From: Nazar Hussain Date: Fri, 15 Mar 2024 15:43:04 +0100 Subject: [PATCH 3/4] Update the clock util --- packages/beacon-node/test/mocks/clock.ts | 14 ++++++++++++++ .../test/mocks/mockedBeaconChain.ts | 18 +++--------------- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/packages/beacon-node/test/mocks/clock.ts b/packages/beacon-node/test/mocks/clock.ts index 15533490956..c38794bf16d 100644 --- a/packages/beacon-node/test/mocks/clock.ts +++ b/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 { + return { + get currentSlot() { + return 0; + }, + get currentEpoch() { + return 0; + }, + currentSlotWithGossipDisparity: undefined, + isCurrentSlotGivenGossipDisparity: vi.fn(), + } as unknown as Mocked; +} diff --git a/packages/beacon-node/test/mocks/mockedBeaconChain.ts b/packages/beacon-node/test/mocks/mockedBeaconChain.ts index 154de5a8acb..aa8228dcece 100644 --- a/packages/beacon-node/test/mocks/mockedBeaconChain.ts +++ b/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 & { logger: Mocked; @@ -105,25 +106,12 @@ 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: clk, + clock: + clock === "real" ? new Clock({config, genesisTime, signal: new AbortController().signal}) : getMockedClock(), forkChoice: getMockedForkChoice(), executionEngine: { notifyForkchoiceUpdate: vi.fn(), From 3e39f618a2aaeb18c1cc04233e54135172052206 Mon Sep 17 00:00:00 2001 From: Nazar Hussain Date: Fri, 15 Mar 2024 16:09:06 +0100 Subject: [PATCH 4/4] Fix the lint error --- .../test/unit/api/impl/validator/duties/proposer.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/beacon-node/test/unit/api/impl/validator/duties/proposer.test.ts b/packages/beacon-node/test/unit/api/impl/validator/duties/proposer.test.ts index af91f1262be..bf9544ad683 100644 --- a/packages/beacon-node/test/unit/api/impl/validator/duties/proposer.test.ts +++ b/packages/beacon-node/test/unit/api/impl/validator/duties/proposer.test.ts @@ -1,6 +1,6 @@ import {describe, it, expect, beforeEach, afterEach, vi} from "vitest"; import {config} from "@lodestar/config/default"; -import {MAX_EFFECTIVE_BALANCE, SLOTS_PER_EPOCH, activePreset} from "@lodestar/params"; +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";