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 12 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
19 changes: 18 additions & 1 deletion packages/beacon-state-transition/src/cache/cachedBeaconState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
readonlyValues,
TreeBacked,
} from "@chainsafe/ssz";
import {allForks, altair, Number64, ParticipationFlags} from "@chainsafe/lodestar-types";
import {allForks, altair, Number64, ParticipationFlags, ValidatorIndex} from "@chainsafe/lodestar-types";
import {createIBeaconConfig, IBeaconConfig, IChainForkConfig} from "@chainsafe/lodestar-config";
import {Tree} from "@chainsafe/persistent-merkle-tree";
import {MutableVector} from "@chainsafe/persistent-ts";
Expand All @@ -20,6 +20,7 @@ import {CachedEpochParticipation, CachedEpochParticipationProxyHandler} from "./
import {ForkName} from "@chainsafe/lodestar-params";
import {CachedInactivityScoreList, CachedInactivityScoreListProxyHandler} from "./cachedInactivityScoreList";
import {newFilledArray} from "../util/array";
import {computeProposers} from "../util";

/**
* `BeaconState` with various caches
Expand Down Expand Up @@ -273,6 +274,22 @@ export class BeaconStateContext<T extends allForks.BeaconState> {
) as CachedBeaconState<T>;
}

getState(): allForks.BeaconState {
return this.type.createTreeBacked(this.tree);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this necessary? this should already be treeBacked

}

getNextEpochBeaconProposer(): ValidatorIndex[] {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is dangerous because it uses a state, which mutates every slot to mutate data in the epoch context, which is shared between multiple states. getNextEpochBeaconProposer should be in the epoch context and use only data available there immutable during an epoch

const nextShuffling = this.epochCtx.nextShuffling;
let nextProposers = this.epochCtx.nextEpochProposers.get(nextShuffling.epoch);
if (!nextProposers) {
nextProposers = computeProposers(this.getState(), nextShuffling, this.epochCtx.effectiveBalanceIncrements);
// Only keep proposers for one epoch in the future, so clear before setting new value
this.epochCtx.nextEpochProposers.clear();
this.epochCtx.nextEpochProposers.set(nextShuffling.epoch, nextProposers);
}
return nextProposers;
}

/**
* Toggle all `MutableVector` caches to use `TransientVector`
*/
Expand Down
18 changes: 18 additions & 0 deletions packages/beacon-state-transition/src/cache/epochContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,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 @@ -474,6 +485,13 @@ export class EpochContext {
`Requesting beacon proposer for different epoch current shuffling: ${epoch} != ${this.currentShuffling.epoch}`
);
}

for (const cachedEpoch of this.nextEpochProposers.keys()) {
// Do not keep past cached future proposal duties.
if (cachedEpoch <= epoch) {
this.nextEpochProposers.delete(cachedEpoch);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this necessary? Once you have computed nextEpochProposers entry it can be left there

}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Now computation of proposers is deferred AND cached when computed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dapplion this won't cache though, and on every request to get next proposers, the this.proposersNextEpoch.computed check here will always return false, and the if branch check always run.

You can confirm this by running the application, and trigger the end point multiple times, the if branch will always run.

I noticed this while working on the implementation and I think the reason why this is the case is how the chain.getHeadStateAtCurrentEpoch here works, but I did not go ahead with caching when I realised that the request to the endpoint itself was actually pretty fast and no need to even cache.

Copy link
Contributor

@dapplion dapplion May 18, 2022

Choose a reason for hiding this comment

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

I noticed this while working on the implementation and I think the reason why this is the case is how the chain.getHeadStateAtCurrentEpoch here works

If this is true, then it's a big issue! Can you open a dedicated issue for it? It should be investigated latter

I did not go ahead with caching when I realised that the request to the endpoint itself was actually pretty fast and no need to even cache.

This code runs on the main thread and according to benchmarks it takes ~100ms. That's still a significant amount of time to block the main thread and repeated computations must be avoided if possible

Copy link
Member

Choose a reason for hiding this comment

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

And please link back to this context after opening a new dedicated issue. :)

Copy link
Member

Choose a reason for hiding this comment

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

return this.proposers[slot % SLOTS_PER_EPOCH];
}

Expand Down
6 changes: 5 additions & 1 deletion packages/beacon-state-transition/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,8 @@ export {EpochContext} from "./cache/epochContext";
export {EpochProcess, beforeProcessEpoch} from "./cache/epochProcess";
export {PubkeyIndexMap, Index2PubkeyCache} from "./cache/pubkeyCache";

export {EffectiveBalanceIncrements, getEffectiveBalanceIncrementsZeroed} from "./cache/effectiveBalanceIncrements";
export {
EffectiveBalanceIncrements,
getEffectiveBalanceIncrementsZeroed,
getEffectiveBalanceIncrementsWithLen,
} from "./cache/effectiveBalanceIncrements";
20 changes: 14 additions & 6 deletions packages/lodestar/src/api/impl/validator/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -277,16 +277,24 @@ 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.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.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
// Epoch should be equal to current epoch, if not the getBeaconProposer call throws an error
const validatorIndex = state.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 @@ -72,8 +72,86 @@ describe("get proposers api impl", function () {
);
const cachedState = createCachedBeaconState(config, state);
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: generateInitialMaxBalances(config, 25),
},
config
);
const cachedState = createCachedBeaconState(config, state);
chainStub.getHeadStateAtCurrentEpoch.resolves(cachedState);
const stubGetBeaconProposer = sinon.stub(cachedState.epochCtx, "getBeaconProposer");
stubGetBeaconProposer.returns(1);
const {data: result} = await api.getProposerDuties(1);
expect(result.length).to.be.equal(SLOTS_PER_EPOCH, "result should be equals to slots per epoch");
expect(stubGetBeaconProposer.called, "stubGetBeaconProposer function should not 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: generateInitialMaxBalances(config, 25),
},
config
);
const cachedState = createCachedBeaconState(config, state);
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: generateInitialMaxBalances(config, 25),
},
config
);
const cachedState = createCachedBeaconState(config, state);
chainStub.getHeadStateAtCurrentEpoch.resolves(cachedState);
const stubGetBeaconProposer = sinon.stub(cachedState.epochCtx, "getBeaconProposer");
stubGetBeaconProposer.throws();
expect(api.getProposerDuties(2), "calling getProposerDuties should throw").to.eventually.throws;
});
});