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

Include EL client info in graffiti #6463

Open
ensi321 opened this issue Feb 21, 2024 · 8 comments · May be fixed by #6753
Open

Include EL client info in graffiti #6463

ensi321 opened this issue Feb 21, 2024 · 8 comments · May be fixed by #6753
Labels
meta-feature-request Issues to track feature requests.
Milestone

Comments

@ensi321
Copy link
Contributor

ensi321 commented Feb 21, 2024

Problem description

Upon agreement reached in ethereum/execution-apis#517 to expose EL client info (ie. standardized client code and version) via Engine API engine_getClientVersionV1, it is sensible to include such info in the graffiti such that EL client diversity could be accurately measured.

Solution description

Currently the default graffiti is ${lodestarPackageName}-${version} with the option to override it with user-defined value passed by --graffiti flag.

I suggest the user-defined value to be appended after the necessary client info (EL info from engine_getClientVersionV1 and CL info from itself) instead of erasing it altogether.

The discussion in the Engine API PR suggested to use the format laid out here but it is not officially decided yet.

Additional context

No response

@ensi321 ensi321 added the meta-feature-request Issues to track feature requests. label Feb 21, 2024
@nflaig
Copy link
Member

nflaig commented Feb 21, 2024

I suggest the user-defined value to be appended after the necessary client info (EL info from engine_getClientVersionV1 and CL info from itself) instead of erasing it altogether.

We must give users the option to customize their graffiti or even set an empty graffiti. I'd rather see this as an extension to the default graffiti.

And if users want client version info to be included in addition to their custom graffiti, we could make it opt-in by adding another CLI flag.

@philknows
Copy link
Member

I think we might be the last client team right now that needs to implement this 😅

@philknows
Copy link
Member

philknows commented May 1, 2024

We also have a --private flag which hides the agent version of our client which is an opt-out option for privacy. Perhaps this is something to also include as the spec change leading to this is meant to be opt-out.

@philknows philknows added this to the v1.19.0 milestone May 1, 2024
@ensi321
Copy link
Contributor Author

ensi321 commented May 8, 2024

We must give users the option to customize their graffiti or even set an empty graffiti. I'd rather see this as an extension to the default graffiti.

And if users want client version info to be included in addition to their custom graffiti, we could make it opt-in by adding another CLI flag.

@nflaig I like Teku's approach:
Include user's graffiti, if there is enough space left in the graffiti, we will try to squeeze in the client info at the end:

  • If there is 13 bytes left: <space>ELcode|2bytecommit|CLcode|2bytecommit
  • If there is 5 bytes left: <space>ELcode|CLcode
  • If < 5 bytes left: don't include client info

This behaviour is enabled by default, and can be disabled via a flag.

Teku has a CLIENT_CODES option in their flag for users who prefer to expose client name but hide the commit versions for privacy reason.

I wonder if we need to give users flexibility on this, or just a on/off flag be enough?

@nflaig
Copy link
Member

nflaig commented May 8, 2024

I think we might be the last client team right now that needs to implement this 😅

Good news, at least Prysm does seem to have it yet as well prysmaticlabs/prysm#13558, and not sure about Nimbus

we will try to squeeze in the client info at the end

How would this look technically? As far as I understand this information is only available to the beacon node by calling the execution client? Imo the graffiti needs to be dictated by the validator client as this is where it is configured

commit versions for privacy reason

This would rather be a security concern but we already leak too much information on p2p so not sure how much it matters at this point, but even without including the commit hash, if we append client details to a user defined graffiti, it is trivial to correlate validators by simply analyzing block proposals.

This behaviour is enabled by default, and can be disabled via a flag.

I am against enabling this by default as it is not something I would like as a node operator myself.

@philknows
Copy link
Member

This would rather be a security concern but we already leak too much information on p2p so not sure how much it matters at this point, but even without including the commit hash, if we append client details to a user defined graffiti, it is trivial to correlate validators by simply analyzing block proposals.

We already provide AgentVersion by default on p2p don't we? And this also includes the version commit IIRC. And of course, we already append version + commit to the graffiti already. All of these are opt-out. So by keeping with standards, this should function the same way. I get the privacy aspect, but based on the history we have of already exposing version and commit numbers, we haven't seen any instances of targeted attacks using this data, but the insight has been and will be useful as described in ethereum/execution-apis#517. There will always be a way to opt-out.

@ensi321 ensi321 linked a pull request May 9, 2024 that will close this issue
@nflaig
Copy link
Member

nflaig commented May 9, 2024

We already provide AgentVersion by default on p2p don't we?

On p2p it's just the client name, no version / commit details

const simpleVersionStr = version.split("/")[0];

And of course, we already append version + commit to the graffiti already

This is a good default we can extend with EL client info, however, a user explicitly specifying --graffiti already opted out of this. If we append client details by default to the user specified graffiti, the user has to opt out once more, but this time it is a flag which has to be set on the beacon node side.

@philknows
Copy link
Member

I see the argument here. I think the point of this execution API change was to capture the data from those who don't bother with opting out of the default because they presumably don't care enough to do so.

So if that is the preference, we should just modify our default graffiti to this new format and won't have to figure out how to deal with mixing it into custom graffiti set by the flag? Open to discussion.

@wemeetagain wemeetagain modified the milestones: v1.19.0, v1.20.0 Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta-feature-request Issues to track feature requests.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants