Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement support for validator next-epoch proposer duties #3782

Merged
merged 29 commits into from
May 16, 2022
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
9a0a792
Implementation to be able to get block proposer an epoch ahead - stil…
dadepo Feb 21, 2022
5a1426a
revert changes made to waitForSlot
dadepo Feb 21, 2022
bcdfbb6
caching the results of computing future proposers. Also extended test
dadepo Feb 21, 2022
4846918
using effectiveBalanceIncrements from state instead of recomputing it
dadepo Feb 24, 2022
18cde25
fix lint errors
dadepo Feb 25, 2022
9bee893
revert check not needed in getBeaconProposer
dadepo Feb 25, 2022
5a632b1
Update tests to include assertion messages
dadepo Feb 25, 2022
0a1aba3
Move caching of next proposer duties to BeaconChain class
dadepo Feb 28, 2022
ac83ab8
Delete the block proposer previously cached when next proposer was re…
dadepo Mar 1, 2022
fafca52
moved next epoch proposers from the chain to the state
dadepo Mar 7, 2022
7d08ada
Compute next proposer on demand and cache
dadepo Mar 14, 2022
f0a9c3f
Fix lint errors
dadepo Mar 15, 2022
520c749
Merged in master and fix conflicts.
dadepo Apr 26, 2022
d54e124
update implementation to work with changes from master
dadepo Apr 26, 2022
02a722a
caching epoch seed in context so that getNextEpochBeaconProposer can …
dadepo Apr 29, 2022
b7e526a
Revert "caching epoch seed in context so that getNextEpochBeaconPropo…
dadepo Apr 29, 2022
72fd6ea
caching epoch seed in context so that getNextEpochBeaconProposer can …
dadepo Apr 29, 2022
12aa60d
removing the need to delete from nextEpochProposers in call to getBea…
dadepo Apr 29, 2022
b6b1b8c
no need to recompute currrentProposerSeed again
dadepo May 2, 2022
c75d926
Revert "no need to recompute currrentProposerSeed again"
dadepo May 2, 2022
3c252a7
removed empty file left after fixing merge conflicts
dadepo May 5, 2022
162a66b
remove some unnecessary variable from the epoch context.
dadepo May 10, 2022
8b58292
add some comments
dadepo May 12, 2022
99e973f
Fix lint
dadepo May 12, 2022
be65566
import from the right location
dadepo May 13, 2022
ab35439
Review PR
dapplion May 16, 2022
1ec1ccb
Merge imports
dapplion May 16, 2022
736cef7
Delete get proposers api impl test
dapplion May 16, 2022
9a51aa4
Remove duplicated comment
dapplion May 16, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 47 additions & 3 deletions packages/beacon-state-transition/src/cache/epochContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {BLSSignature, CommitteeIndex, Epoch, Slot, ValidatorIndex, phase0, SyncP
import {createIBeaconConfig, IBeaconConfig, IChainConfig} from "@chainsafe/lodestar-config";
import {
ATTESTATION_SUBNET_COUNT,
DOMAIN_BEACON_PROPOSER,
EFFECTIVE_BALANCE_INCREMENT,
FAR_FUTURE_EPOCH,
GENESIS_EPOCH,
Expand All @@ -18,8 +19,9 @@ import {
getChurnLimit,
isActiveValidator,
isAggregatorFromCommitteeLength,
computeProposers,
computeSyncPeriodAtEpoch,
getSeed,
computeProposers,
} from "../util";
import {computeEpochShuffling, IEpochShuffling} from "../util/epochShuffling";
import {EffectiveBalanceIncrements, getEffectiveBalanceIncrementsWithLen} from "./effectiveBalanceIncrements";
Expand Down Expand Up @@ -95,6 +97,17 @@ export class EpochContext {
* 32 x Number
*/
proposers: ValidatorIndex[];

/**
* Map of Indexes of the block proposers keyed by the next epoch.
*
* We allow requesting proposal duties only one epoch in the future
* Note: There is a small probability that returned validators differs
* than what is returned when the epoch is reached.
*
* 32 x Number
*/
nextEpochProposers: Map<Epoch, ValidatorIndex[]> = new Map<Epoch, ValidatorIndex[]>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cache exactly 1 or 0 next proposers for the next epoch of this epochCtx. We should research the safety of +2, +3 epoch duties before generalizing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cache exactly 1 or 0 next proposers for the next epoch of this epochCtx

Yeah. That is what is being done. Are you hinting at something else?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's exactly 1 possible value for that why do a map?

nextEpochProposers: ValidatorIndex[] | null;

is more intuitive

/**
* Shuffling of validator indexes. Immutable through the epoch, then it's replaced entirely.
* Note: Per spec definition, shuffling will always be defined. They are never called before loadState()
Expand Down Expand Up @@ -150,6 +163,9 @@ export class EpochContext {
epoch: Epoch;
syncPeriod: SyncPeriod;

currentProposerSeed: Uint8Array;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does needs to be persisted? What's the advantage of keeping this around?

nextProposerSeed: Uint8Array;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, what's the advantage of keeping this around? Why not compute on demand and discard after caching nextEpochProposers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue in that, is related to what you observed in the comment here

getNextEpochBeaconProposer should be in the epoch context and use only data available there immutable during an epoch

Computing the seed via getSeed method, requires the state which makes it impossible to compute using data available in the context.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then please add comments 🙏 all non-obvious decisions must be rationalized in committed comments


constructor(data: {
config: IBeaconConfig;
pubkey2index: PubkeyIndexMap;
Expand All @@ -170,6 +186,8 @@ export class EpochContext {
nextSyncCommitteeIndexed: SyncCommitteeCache;
epoch: Epoch;
syncPeriod: SyncPeriod;
currentProposerSeed: Uint8Array;
nextProposerSeed: Uint8Array;
}) {
this.config = data.config;
this.pubkey2index = data.pubkey2index;
Expand All @@ -190,6 +208,8 @@ export class EpochContext {
this.nextSyncCommitteeIndexed = data.nextSyncCommitteeIndexed;
this.epoch = data.epoch;
this.syncPeriod = data.syncPeriod;
this.currentProposerSeed = data.currentProposerSeed;
this.nextProposerSeed = data.nextProposerSeed;
}

/**
Expand Down Expand Up @@ -269,9 +289,14 @@ export class EpochContext {
: computeEpochShuffling(state, previousActiveIndices, previousEpoch);
const nextShuffling = computeEpochShuffling(state, nextActiveIndices, nextEpoch);

const currentProposerSeed = getSeed(state, currentShuffling.epoch, DOMAIN_BEACON_PROPOSER);
const nextProposerSeed = getSeed(state, nextShuffling.epoch, DOMAIN_BEACON_PROPOSER);

// Allow to create CachedBeaconState for empty states
const proposers =
state.validators.length > 0 ? computeProposers(state, currentShuffling, effectiveBalanceIncrements) : [];
state.validators.length > 0
? computeProposers(currentProposerSeed, currentShuffling, effectiveBalanceIncrements)
: [];

// Only after altair, compute the indices of the current sync committee
const afterAltairFork = currentEpoch >= config.ALTAIR_FORK_EPOCH;
Expand Down Expand Up @@ -334,6 +359,8 @@ export class EpochContext {
nextSyncCommitteeIndexed,
epoch: currentEpoch,
syncPeriod: computeSyncPeriodAtEpoch(currentEpoch),
currentProposerSeed,
nextProposerSeed,
});
}

Expand Down Expand Up @@ -369,6 +396,8 @@ export class EpochContext {
nextSyncCommitteeIndexed: this.nextSyncCommitteeIndexed,
epoch: this.epoch,
syncPeriod: this.syncPeriod,
currentProposerSeed: this.currentProposerSeed,
nextProposerSeed: this.nextProposerSeed,
});
}

Expand All @@ -389,7 +418,10 @@ export class EpochContext {
const nextEpoch = currEpoch + 1;

this.nextShuffling = computeEpochShuffling(state, epochProcess.nextEpochShufflingActiveValidatorIndices, nextEpoch);
this.proposers = computeProposers(state, this.currentShuffling, this.effectiveBalanceIncrements);
this.currentProposerSeed = getSeed(state, this.currentShuffling.epoch, DOMAIN_BEACON_PROPOSER);
this.nextProposerSeed = getSeed(state, this.nextShuffling.epoch, DOMAIN_BEACON_PROPOSER);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we only need to call getSeed() once for this.nextProposerSeed, this.currrentProposerSeed = this.nextPrroposerSeed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JSDoc for nextProposerSeed must be placed in the variable declaration (here), here it has no effect. You can duplicate the comment here too if appropiate but use regular // comment syntax. Also try to fit the comment to 120 wide characters for consistent formatting.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here you can comment (with //) that the cost of getSeed is negligible provided that this path runs once per epoch.


this.proposers = computeProposers(this.currentProposerSeed, this.currentShuffling, this.effectiveBalanceIncrements);

// TODO: DEDUPLICATE from createEpochContext
//
Expand Down Expand Up @@ -467,6 +499,18 @@ export class EpochContext {
return (committeesSinceEpochStart + committeeIndex) % ATTESTATION_SUBNET_COUNT;
}

getNextEpochBeaconProposer(): ValidatorIndex[] {
const nextShuffling = this.nextShuffling;
let nextProposers = this.nextEpochProposers.get(nextShuffling.epoch);
if (!nextProposers) {
nextProposers = computeProposers(this.nextProposerSeed, nextShuffling, this.effectiveBalanceIncrements);
// Only keep proposers for one epoch in the future, so clear before setting new value
this.nextEpochProposers.clear();
this.nextEpochProposers.set(nextShuffling.epoch, nextProposers);
dapplion marked this conversation as resolved.
Show resolved Hide resolved
}
return nextProposers;
}

getBeaconProposer(slot: Slot): ValidatorIndex {
const epoch = computeEpochAtSlot(slot);
if (epoch !== this.currentShuffling.epoch) {
Expand Down
7 changes: 6 additions & 1 deletion packages/beacon-state-transition/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,9 @@ export {EpochProcess, beforeProcessEpoch} from "./cache/epochProcess";

// Aux data-structures
export {PubkeyIndexMap, Index2PubkeyCache} from "./cache/pubkeyCache";
export {EffectiveBalanceIncrements, getEffectiveBalanceIncrementsZeroed} from "./cache/effectiveBalanceIncrements";

export {
EffectiveBalanceIncrements,
getEffectiveBalanceIncrementsZeroed,
getEffectiveBalanceIncrementsWithLen,
} from "./cache/effectiveBalanceIncrements";
4 changes: 1 addition & 3 deletions packages/beacon-state-transition/src/util/seed.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import {digest} from "@chainsafe/as-sha256";
import {Epoch, Bytes32, DomainType, ValidatorIndex} from "@chainsafe/lodestar-types";
import {assert, bytesToBigInt, intToBytes} from "@chainsafe/lodestar-utils";
import {
DOMAIN_BEACON_PROPOSER,
DOMAIN_SYNC_COMMITTEE,
EFFECTIVE_BALANCE_INCREMENT,
EPOCHS_PER_HISTORICAL_VECTOR,
Expand All @@ -25,11 +24,10 @@ import {computeEpochAtSlot} from "./epoch";
* Compute proposer indices for an epoch
*/
export function computeProposers(
state: BeaconStateAllForks,
epochSeed: Uint8Array,
shuffling: {epoch: Epoch; activeIndices: ValidatorIndex[]},
effectiveBalanceIncrements: EffectiveBalanceIncrements
): number[] {
const epochSeed = getSeed(state, shuffling.epoch, DOMAIN_BEACON_PROPOSER);
const startSlot = computeStartSlotAtEpoch(shuffling.epoch);
const proposers = [];
for (let slot = startSlot; slot < startSlot + SLOTS_PER_EPOCH; slot++) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
import {itBench} from "@dapplion/benchmark";
import {Epoch} from "@chainsafe/lodestar-types";
import {DOMAIN_BEACON_PROPOSER} from "@chainsafe/lodestar-params";
import {
computeEpochAtSlot,
CachedBeaconStateAllForks,
computeEpochShuffling,
getNextSyncCommittee,
} from "../../../../src";
import {computeProposers} from "../../../../src";
import {getSeed} from "../../../../lib";
dapplion marked this conversation as resolved.
Show resolved Hide resolved
import {generatePerfTestCachedStatePhase0, numValidators} from "../../util";
import {computeProposers} from "../../../../src/util/seed";

describe("epoch shufflings", () => {
let state: CachedBeaconStateAllForks;
Expand All @@ -25,7 +27,8 @@ describe("epoch shufflings", () => {
itBench({
id: `computeProposers - vc ${numValidators}`,
fn: () => {
computeProposers(state, state.epochCtx.nextShuffling, state.epochCtx.effectiveBalanceIncrements);
const epochSeed = getSeed(state, state.epochCtx.nextShuffling.epoch, DOMAIN_BEACON_PROPOSER);
computeProposers(epochSeed, state.epochCtx.nextShuffling, state.epochCtx.effectiveBalanceIncrements);
},
});

Expand Down
19 changes: 13 additions & 6 deletions packages/lodestar/src/api/impl/validator/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -276,16 +276,23 @@ export function getValidatorApi({chain, config, logger, metrics, network, sync}:
const startSlot = computeStartSlotAtEpoch(epoch);
await waitForSlot(startSlot); // Must never request for a future slot > currentSlot
twoeths marked this conversation as resolved.
Show resolved Hide resolved

const nextEpoch = chain.clock.currentEpoch + 1;
const state = await chain.getHeadStateAtCurrentEpoch();

const duties: routes.validator.ProposerDuty[] = [];
const indexes: ValidatorIndex[] = [];

// Gather indexes to get pubkeys in batch (performance optimization)
for (let i = 0; i < SLOTS_PER_EPOCH; i++) {
// getBeaconProposer ensures the requested epoch is correct
const validatorIndex = state.epochCtx.getBeaconProposer(startSlot + i);
indexes.push(validatorIndex);
// We allow requesting proposal duties only one epoch in the future
// Note: There is a small probability that returned validators differs
// than what is returned when the epoch is reached.
if (epoch === nextEpoch) {
indexes.push(...state.epochCtx.getNextEpochBeaconProposer());
} else {
// Gather indexes to get pubkeys in batch (performance optimization)
for (let i = 0; i < SLOTS_PER_EPOCH; i++) {
// getBeaconProposer ensures the requested epoch is correct
const validatorIndex = state.epochCtx.getBeaconProposer(startSlot + i);
indexes.push(validatorIndex);
}
}

// NOTE: this is the fastest way of getting compressed pubkeys.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,91 @@ describe("get proposers api impl", function () {
);
const cachedState = createCachedBeaconStateTest(state, config);
chainStub.getHeadStateAtCurrentEpoch.resolves(cachedState);
sinon.stub(cachedState.epochCtx, "getBeaconProposer").returns(1);
const stubGetBeaconProposer = sinon.stub(cachedState.epochCtx, "getBeaconProposer");
stubGetBeaconProposer.returns(1);
const {data: result} = await api.getProposerDuties(0);
expect(result.length).to.be.equal(SLOTS_PER_EPOCH);
expect(result.length).to.be.equal(SLOTS_PER_EPOCH, "result should be equals to slots per epoch");
expect(stubGetBeaconProposer.called, "stubGetBeaconProposer function should have been called").to.be.true;
});

it("should get proposers for next epoch", async function () {
syncStub.isSynced.returns(true);
server.sandbox.stub(chainStub.clock, "currentEpoch").get(() => 0);
server.sandbox.stub(chainStub.clock, "currentSlot").get(() => 0);
dbStub.block.get.resolves({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);
chainStub.getHeadStateAtCurrentEpoch.resolves(cachedState);
const stubGetNextBeaconProposer = sinon.stub(cachedState.epochCtx, "getNextEpochBeaconProposer");
const stubGetBeaconProposer = sinon.stub(cachedState.epochCtx, "getBeaconProposer");
stubGetNextBeaconProposer.returns([1]);
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment,@typescript-eslint/no-unsafe-call,@typescript-eslint/no-unsafe-member-access
const {data: result} = await api.getProposerDuties(1);
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
expect(result.length).to.be.equal(SLOTS_PER_EPOCH, "result should be equals to slots per epoch");
expect(stubGetNextBeaconProposer.called, "stubGetBeaconProposer function should not have been called").to.be.true;
expect(stubGetBeaconProposer.called, "stubGetBeaconProposer function should have been called").to.be.false;
});

it("should have different proposer for current and next epoch", async function () {
syncStub.isSynced.returns(true);
server.sandbox.stub(chainStub.clock, "currentEpoch").get(() => 0);
server.sandbox.stub(chainStub.clock, "currentSlot").get(() => 0);
dbStub.block.get.resolves({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);
chainStub.getHeadStateAtCurrentEpoch.resolves(cachedState);
const stubGetBeaconProposer = sinon.stub(cachedState.epochCtx, "getBeaconProposer");
stubGetBeaconProposer.returns(1);
const {data: currentProposers} = await api.getProposerDuties(0);
const {data: nextProposers} = await api.getProposerDuties(1);
expect(currentProposers).to.not.deep.equal(nextProposers, "current proposer and next proposer should be different");
});

it("should not get proposers for more than one epoch in the future", async function () {
syncStub.isSynced.returns(true);
server.sandbox.stub(chainStub.clock, "currentEpoch").get(() => 0);
server.sandbox.stub(chainStub.clock, "currentSlot").get(() => 0);
dbStub.block.get.resolves({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);
chainStub.getHeadStateAtCurrentEpoch.resolves(cachedState);
const stubGetBeaconProposer = sinon.stub(cachedState.epochCtx, "getBeaconProposer");
stubGetBeaconProposer.throws();
expect(api.getProposerDuties(2), "calling getProposerDuties should throw").to.eventually.throws;
});
});