feat: include EL client info in graffiti #6753
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Performance Report✔️ no performance regression detected 🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
nflaig
reviewed
Jun 5, 2024
There was a problem hiding this comment.
General approach of having the default moved from validator client to beacon node looks good to me, just need to make sure to always respect graffiti if explicitly set by user.
I haven't looked at the spec PR, will do more detailed review once the high-level concerns are addressed.
Co-authored-by: Nico Flaig <nflaig@protonmail.com>
wemeetagain
reviewed
Jun 24, 2024
Co-authored-by: Cayman <caymannava@gmail.com>
nflaig
reviewed
Jun 24, 2024
nflaig
reviewed
Jun 24, 2024
nflaig
reviewed
Jun 26, 2024
| return this.executionClientVersion; | ||
| } | ||
|
|
||
| getConsensusClientVersion(): ClientVersion { |
There was a problem hiding this comment.
it's a bit strange that this is part of execution engine
| return {code, name: cv.name, version: cv.version, commit: cv.commit}; | ||
| }); | ||
|
|
||
| this.logger.debug(`executionClientVersion is set to ${JSON.stringify(this.executionClientVersion)}`); |
There was a problem hiding this comment.
just dumping the data like this doesn't seem too useful, maybe we could include the version info in a nicely formatted way in the state logs (likely just the online one)
| * Not to be confused with `getClientVersion`. This method returns cached client version | ||
| * from `getClientVersion` which is a rpc call to EL client | ||
| */ | ||
| getExecutionClientVersion(): ClientVersion[]; |
There was a problem hiding this comment.
instead of using a function for this, could have a public property and use a getter in the implementation
There was a problem hiding this comment.
instead of using a function for this, could have a public property and use a getter in the implementation
It was to conform with the getState() function. But I agree using a getter. We don't need property to be public though.
There was a problem hiding this comment.
We don't need property to be public though.
I mean on the interface, like
readonly clientVersion?: ClientVersion;and the implementation would use a getter
I don't like that getState() is a method if it just returns a property
| @@ -429,6 +474,9 @@ export class ExecutionEngineHttp implements IExecutionEngine { | |||
| switch (newState) { | |||
| case ExecutionEngineState.ONLINE: | |||
| this.logger.info("Execution client became online", {oldState, newState}); | |||
| this.getClientVersion().catch((e) => { | |||
There was a problem hiding this comment.
The online state is good place to trigger this as the online time this will be the newState if el was offline but there is a small issue on startup with this as initial state is online, see
I would assume it's currently not fetched on startup, other than handling inital fetch this looks good to me
nflaig
reviewed
Jun 27, 2024
| @@ -335,6 +332,26 @@ export function getValidatorApi({ | |||
| ); | |||
| } | |||
|
|
|||
| function getDefaultGraffiti(): string { | |||
There was a problem hiding this comment.
Looking at code in this PR, it seems a bit scattered around, we should consider moving this into utils file and having graffiti related code there. Would also allow to add unit tests for this which would document the expected format as well.
The execution engine is responsible for providing the version of the connected EL client and shouldn't have to know more than that. I guess the execution engine needs to lodestar version, maybe we can just import that statically, similar to the previous graffiti util
| * Client code as defined in https://github.com/ethereum/execution-apis/blob/v1.0.0-beta.4/src/engine/identification.md#clientcode | ||
| * ClientCode.XX is dedicated to other clients which do not have their own code | ||
| */ | ||
| export enum ClientCode { |
There was a problem hiding this comment.
this is not really related to execution engine as it's specific to format in graffiti ah nvm this is relevant to the engine api format
nflaig
reviewed
Jun 27, 2024
| const response = await this.rpc.fetchWithRetries< | ||
| EngineApiRpcReturnTypes[typeof method], | ||
| EngineApiRpcParamTypes[typeof method] | ||
| >({method, params: [clientVersion ?? this.getConsensusClientVersion()]}); |
There was a problem hiding this comment.
Need to confirm that we sent 8 bytes as the commit hash, I think we currently use 7 to match github hashes
Depending how strict the ELs validate this request it might cause issues, see related
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR shifts the responsibility of setting the default graffiti from
validatortobeacon-nodeand also definesengine_getClientVersionV1.Due to
validatorpackage no longer determines default graffiti, graffitiValidatorStoreis now marked as optional (typestring | undefined). This implies produce block endpoints in validator api accepts optional graffiti andgetGraffitiin key manager api now may throw error if no graffiti is available for the queried pubkey.When the api module first starts.
engine_getClientVersionV1will be called to determine the default graffiti and store it in memory. Afterwards this endpoint will be called inprepareNextSlotif proposing next slot.Default graffiti will be used during produceBlock api call when the block graffiti is undefined and
privateflag is unset.Closes #6463