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.

Sign up for GitHub

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

fix: handle skip_randao_verification query param as per spec #6576

Merged
merged 10 commits into from
Mar 21, 2024
Merged

fix: handle skip_randao_verification query param as per spec #6576

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
df9e7de
Fix validator produceBlockV3 req param
nazarhussain Mar 21, 2024
3e1ee02
Update the lighthouse version for sim tests
nazarhussain Mar 21, 2024
f1a5e44
Include deneb in mixed-client sim tests
nazarhussain Mar 21, 2024
e765907
Fix query param schema type
nazarhussain Mar 21, 2024
31a699e
Fix the unit tests
nazarhussain Mar 21, 2024
df95b69
Fix the code as per feedback
nazarhussain Mar 21, 2024
3266f45
Update the parse method
nazarhussain Mar 21, 2024
6fde9d9
Add cancun support to geth client for sim tests
nazarhussain Mar 21, 2024
1418586
Fix the types
nazarhussain Mar 21, 2024
f8f7898
Add deneb support to multi-fork sim tests
nazarhussain Mar 21, 2024
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
2 changes: 1 addition & 1 deletion .env.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
GETH_DOCKER_IMAGE=ethereum/client-go:v1.13.14
# Use either image or local binary for the testing
GETH_BINARY_DIR=
LIGHTHOUSE_DOCKER_IMAGE=sigp/lighthouse:v4.6.0-amd64-modern-dev
LIGHTHOUSE_DOCKER_IMAGE=sigp/lighthouse:v5.1.1-amd64-modern-dev

# We can't upgrade nethermind further due to genesis hash mismatch with the geth
# https://github.com/NethermindEth/nethermind/issues/6683
Expand Down
12 changes: 8 additions & 4 deletions packages/api/src/beacon/routes/validator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,7 @@ export type ReqTypes = {
query: {
randao_reveal: string;
graffiti: string;
skip_randao_verification?: boolean;
skip_randao_verification?: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nimbus also treated this attribute as string.

https://github.com/status-im/nimbus-eth2/blob/fc9c72f0ebc2013858f6b28c1940838c97a2f814/beacon_chain/rpc/rest_validator_api.nim#L566

nazarhussain marked this conversation as resolved.
Show resolved Hide resolved
fee_recipient?: string;
builder_selection?: string;
builder_boost_factor?: string;
Expand Down Expand Up @@ -556,7 +556,7 @@ export function getReqSerializers(): ReqSerializers<Api, ReqTypes> {
randao_reveal: toHexString(randaoReveal),
graffiti: toGraffitiHex(graffiti),
fee_recipient: opts?.feeRecipient,
skip_randao_verification: skipRandaoVerification,
nazarhussain marked this conversation as resolved.
Show resolved Hide resolved
...(skipRandaoVerification && {skip_randao_verification: ""}),
builder_selection: opts?.builderSelection,
builder_boost_factor: opts?.builderBoostFactor?.toString(),
strict_fee_recipient_check: opts?.strictFeeRecipientCheck,
Expand All @@ -567,7 +567,7 @@ export function getReqSerializers(): ReqSerializers<Api, ReqTypes> {
params.slot,
fromHexString(query.randao_reveal),
fromGraffitiHex(query.graffiti),
query.skip_randao_verification,
parseSkipRandaoVerification(query.skip_randao_verification),
{
feeRecipient: query.fee_recipient,
builderSelection: query.builder_selection as BuilderSelection,
Expand All @@ -582,7 +582,7 @@ export function getReqSerializers(): ReqSerializers<Api, ReqTypes> {
randao_reveal: Schema.StringRequired,
graffiti: Schema.String,
fee_recipient: Schema.String,
skip_randao_verification: Schema.Boolean,
skip_randao_verification: Schema.String,
builder_selection: Schema.String,
builder_boost_factor: Schema.String,
strict_fee_recipient_check: Schema.Boolean,
Expand Down Expand Up @@ -795,3 +795,7 @@ export function getReturnTypes(): ReturnTypes<Api> {
function parseBuilderBoostFactor(builderBoostFactorInput?: string | number | bigint): bigint | undefined {
return builderBoostFactorInput !== undefined ? BigInt(builderBoostFactorInput) : undefined;
}

function parseSkipRandaoVerification(skipRandaoVerification?: string): boolean {
return skipRandaoVerification !== undefined && skipRandaoVerification === "";
Copy link
Member

@nflaig nflaig Mar 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This query parameter is a flag and does not take a value.

I think checking the value doesn't even make sense as the spec states it does not take a value, might be best to just checking if the flag exists

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding of does not take a value implies an empty string in terms a query string parameter for a url. So I believe that's correct to check it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From this Lighthouse implementation and Nimbus it's also clear they check for the empty string value.

https://github.com/sigp/lighthouse/blob/2a3c709f8ceec4d47c1934446511660c429fe90a/common/eth2/src/types.rs#L751

https://github.com/status-im/nimbus-eth2/blob/fc9c72f0ebc2013858f6b28c1940838c97a2f814/beacon_chain/rpc/rest_validator_api.nim#L590

Copy link
Member

Choose a reason for hiding this comment

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

implies an empty string in terms a query string parameter for a url

Hmm yeah might be, or potentially the difference between setting ?skip_randao_verification and ?skip_randao_verification=. If other clients are strictly checking it that way we should be fine as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

?skip_randao_verification is not a right query string parameter format, it query parameters always suffix with =

Copy link
Member

Choose a reason for hiding this comment

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

Was looking through the RFC but didn't find anything on it, but even the openapi docs have this as an example

image

I would expect that ?skip_randao_verification is allowed by fastify but in the end it might be an implementation detail.

}
6 changes: 3 additions & 3 deletions packages/api/test/unit/beacon/testData/validator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ export const testData: GenericServerTestCases<Api> = {
32000,
randaoReveal,
graffiti,
undefined,
false,
{
feeRecipient,
builderSelection: undefined,
Expand All @@ -65,7 +65,7 @@ export const testData: GenericServerTestCases<Api> = {
32000,
randaoReveal,
graffiti,
undefined,
false,
{
feeRecipient,
builderSelection: undefined,
Expand Down Expand Up @@ -109,7 +109,7 @@ export const testData: GenericServerTestCases<Api> = {
32000,
randaoReveal,
graffiti,
undefined,
false,
{
feeRecipient,
builderSelection: undefined,
Expand Down
13 changes: 4 additions & 9 deletions packages/cli/test/sim/mixed_client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,15 @@ import {connectAllNodes, waitForSlot} from "../utils/simulation/utils/network.js
const altairForkEpoch = 2;
const bellatrixForkEpoch = 4;
const capellaForkEpoch = 6;
const runTillEpoch = 8;
const denebForkEpoch = 8;
const runTillEpoch = 10;
const syncWaitEpoch = 2;

const {estimatedTimeoutMs, forkConfig} = defineSimTestConfig({
ALTAIR_FORK_EPOCH: altairForkEpoch,
BELLATRIX_FORK_EPOCH: bellatrixForkEpoch,
CAPELLA_FORK_EPOCH: capellaForkEpoch,
DENEB_FORK_EPOCH: denebForkEpoch,
runTillEpoch: runTillEpoch + syncWaitEpoch,
initialNodes: 2,
});
Expand All @@ -41,18 +43,11 @@ const env = await SimulationEnvironment.initWithDefaults(
keysCount: 32,
remote: true,
beacon: BeaconClient.Lighthouse,
// for cross client make sure lodestar doesn't use v3 for now untill lighthouse supports
validator: {
type: ValidatorClient.Lodestar,
options: {
clientOptions: {
useProduceBlockV3: false,
// this should cause usage of produceBlockV2
//
// but if blinded production is enabled in lighthouse beacon then this should cause
// usage of produce blinded block which should return execution block in blinded format
// but only enable that after testing lighthouse beacon
"builder.selection": "executiononly",
useProduceBlockV3: true,
Copy link
Member

Choose a reason for hiding this comment

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

do we even have to set this explicitly, or do we want to test as it works in production, i.e. pre deneb use v2 and post deneb v3?

Copy link
Member

Choose a reason for hiding this comment

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

I don't believe we need to explicitly state this anymore now that we're post Deneb and use it by default. Everything on our side should've flipped to v3 automatically at Deneb unless explicitly stated false.

Copy link
Member

Choose a reason for hiding this comment

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

yes but sim tests go through all forks from phase0, just wondering if it makes more sense to test this more closely to production behavior were this only switches to v3 post deneb. On the other hand, there is no public network I know of that is pre deneb anymore, so it just matters for private / testnets or maybe emphemery which does not start genesis at deneb right now iirc

},
},
},
Expand Down
4 changes: 3 additions & 1 deletion packages/cli/test/sim/multi_fork.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,15 @@ import {createWithdrawalAssertions} from "../utils/simulation/assertions/withdra
const altairForkEpoch = 2;
const bellatrixForkEpoch = 4;
const capellaForkEpoch = 6;
const runTillEpoch = 8;
const denebForkEpoch = 8;
const runTillEpoch = 10;
const syncWaitEpoch = 2;

const {estimatedTimeoutMs, forkConfig} = defineSimTestConfig({
ALTAIR_FORK_EPOCH: altairForkEpoch,
BELLATRIX_FORK_EPOCH: bellatrixForkEpoch,
CAPELLA_FORK_EPOCH: capellaForkEpoch,
DENEB_FORK_EPOCH: denebForkEpoch,
runTillEpoch: runTillEpoch + syncWaitEpoch,
initialNodes: 5,
});
Expand Down
14 changes: 11 additions & 3 deletions packages/cli/test/utils/simulation/execution_clients/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
ExecutionNode,
ExecutionStartMode,
} from "../interfaces.js";
import {getEstimatedShanghaiTime} from "../utils/index.js";
import {getEstimatedForkTime} from "../utils/index.js";
import {getGethGenesisBlock} from "../utils/execution_genesis.js";
import {ensureDirectories} from "../utils/paths.js";
import {generateGethNode} from "./geth.js";
Expand All @@ -28,8 +28,16 @@ export async function createExecutionNode<E extends ExecutionClient>(
cliqueSealingPeriod: options.cliqueSealingPeriod ?? CLIQUE_SEALING_PERIOD,
shanghaiTime:
options.shanghaiTime ??
getEstimatedShanghaiTime({
capellaForkEpoch: forkConfig.CAPELLA_FORK_EPOCH,
getEstimatedForkTime({
forkEpoch: forkConfig.CAPELLA_FORK_EPOCH,
genesisTime: options.genesisTime,
secondsPerSlot: forkConfig.SECONDS_PER_SLOT,
additionalSlots: 0,
}),
cancunTime:
options.cancunTime ??
getEstimatedForkTime({
forkEpoch: forkConfig.DENEB_FORK_EPOCH,
genesisTime: options.genesisTime,
secondsPerSlot: forkConfig.SECONDS_PER_SLOT,
additionalSlots: 0,
Expand Down
24 changes: 21 additions & 3 deletions packages/cli/test/utils/simulation/execution_clients/nethermind.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,19 @@ export const generateNethermindNode: ExecutionNodeGenerator<ExecutionClient.Neth
throw Error(`EL ENV must be provided, NETHERMIND_DOCKER_IMAGE: ${process.env.NETHERMIND_DOCKER_IMAGE}`);
}

const {id, mode, ttd, address, mining, clientOptions, nodeIndex, cliqueSealingPeriod, shanghaiTime, genesisTime} =
opts;
const {
id,
mode,
ttd,
address,
mining,
clientOptions,
nodeIndex,
cliqueSealingPeriod,
shanghaiTime,
cancunTime,
genesisTime,
} = opts;
const ports = getNodePorts(nodeIndex);

const {rootDir, rootDirMounted, logFilePath, jwtsecretFilePathMounted} = getNodeMountedPaths(
Expand Down Expand Up @@ -48,7 +59,14 @@ export const generateNethermindNode: ExecutionNodeGenerator<ExecutionClient.Neth
await writeFile(
chainSpecPath,
JSON.stringify(
getNethermindChainSpec(mode, {ttd, cliqueSealingPeriod, shanghaiTime, genesisTime, clientOptions: []})
getNethermindChainSpec(mode, {
ttd,
cliqueSealingPeriod,
shanghaiTime,
cancunTime,
genesisTime,
clientOptions: [],
})
)
);
},
Expand Down
1 change: 1 addition & 0 deletions packages/cli/test/utils/simulation/interfaces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ export interface ExecutionGenesisOptions<E extends ExecutionClient = ExecutionCl
ttd: bigint;
cliqueSealingPeriod: number;
shanghaiTime: number;
cancunTime: number;
genesisTime: number;
clientOptions: ExecutionClientsOptions[E];
}
Expand Down
3 changes: 2 additions & 1 deletion packages/cli/test/utils/simulation/utils/execution_genesis.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ export const getGethGenesisBlock = (
mode: ExecutionStartMode,
options: ExecutionGenesisOptions
): Record<string, unknown> => {
const {ttd, cliqueSealingPeriod, shanghaiTime, genesisTime} = options;
const {ttd, cliqueSealingPeriod, shanghaiTime, genesisTime, cancunTime} = options;

const genesis = {
config: {
Expand All @@ -24,6 +24,7 @@ export const getGethGenesisBlock = (
berlinBlock: 0,
londonBlock: 0,
shanghaiTime,
cancunTime,
terminalTotalDifficulty: Number(ttd as bigint),
clique: {period: cliqueSealingPeriod, epoch: 30000},
},
Expand Down
10 changes: 5 additions & 5 deletions packages/cli/test/utils/simulation/utils/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,20 +125,20 @@ export const getEstimatedTTD = ({
return BigInt(Math.ceil(secondsTillBellatrix / cliqueSealingPeriod) * ETH_TTD_INCREMENT);
};

export const getEstimatedShanghaiTime = ({
export const getEstimatedForkTime = ({
genesisTime,
secondsPerSlot,
capellaForkEpoch,
forkEpoch,
additionalSlots,
}: {
genesisTime: number;
secondsPerSlot: number;
capellaForkEpoch: number;
forkEpoch: number;
additionalSlots: number;
}): number => {
const secondsTillCapella = capellaForkEpoch * activePreset.SLOTS_PER_EPOCH * secondsPerSlot;
const secondsTillFork = forkEpoch * activePreset.SLOTS_PER_EPOCH * secondsPerSlot;

return genesisTime + secondsTillCapella + additionalSlots * secondsPerSlot;
return genesisTime + secondsTillFork + additionalSlots * secondsPerSlot;
};

export const squeezeString = (val: string, length: number, sep = "..."): string => {
Expand Down
Loading