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

Add graffiti API #63

Merged
merged 7 commits into from
Nov 24, 2023
Merged

Add graffiti API #63

merged 7 commits into from
Nov 24, 2023

Conversation

nflaig
Copy link
Collaborator

@nflaig nflaig commented Nov 6, 2023

Add keymanager API to manage graffiti per validator.

The rationale for this is similar as for gas limit (#39), the graffiti is already part of proposer config file and it should be quite simple to implement (ChainSafe/lodestar#6083).

@dappnodedev
Copy link

It would be awesome to have this! In Dappnode we could add the feature of editing the graffiti individually from the Web3Signer package UI!

apis/graffiti.yaml Outdated Show resolved Hide resolved
apis/graffiti.yaml Outdated Show resolved Hide resolved
apis/graffiti.yaml Outdated Show resolved Hide resolved
types/graffiti.yaml Outdated Show resolved Hide resolved
description: |
Get the graffiti for an individual validator. This graffiti is the one used by the
validator when proposing blocks. If no graffiti is set explicitly, returns
the process-wide default.
Copy link
Contributor

Choose a reason for hiding this comment

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

Lighthouse has a custom graffiti endpoint (used by our Siren UI) that returns all graffitis of each validator. It would be nice to consider returning all graffitis and allow user to query specific pubkeys if needed, so user don't have to make multiple requests if they are interested in all validators running on a client. This would be useful for nodes running a larger number of validators.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The idea was to have the same behavior as current fee recipient and gas limit API but if there is a use case for it we could add an additional API.

How should that one look like, GET /eth/v1/validators/graffiti?pubkeys=0x01,0x02? Passing the pubkeys in query string might be problematic as it suffers from the same issue as getStateValidators API.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good point. I think the main use case to return multiple graffitis is when users have a large number of validators - so perhaps it make sense to have a separate endpoint to return all (GET /eth/v1/validators/graffiti), and one to return a specified validator (what you have proposed)?

Siren currently uses a custom endpoint that returns all graffitis, so it would be useful if we have something similar in the spec.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not opposed to adding this but might be good to get feedback from other client teams on this. Maybe it makes sense to add this in a separate PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

this whole api is based on individual validators, we can do a fetch all, but it needs to be a different pattern to this endpoint.

Previously the argument for doing individual is its simple, and MVP, and it's never needed to be expanded upon, so I guess the real question is 'why provide get all for this specific interface?'

I get the desire to get all data back may be convenient, especially for large players, but equally so, the ability to set them all in one call (or set the new global default) may be desirable, and it can snowball quickly...

If it's needed and clients want to support it, thats fine. In the first instance, MVP is probably just the already defined interface.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me!

@rolfyone
Copy link
Collaborator

rolfyone commented Nov 10, 2023

It would be awesome to have this! In Dappnode we could add the feature of editing the graffiti individually from the Web3Signer package UI!

Given web3signer is only signing (not creating blocks) it won't be able to control graffiti, you'd still need to have the CL with keymanager and control the graffiti through that...

I realised that may have been random... the time graffiti is supplied is when the validator node makes a call to https://ethereum.github.io/beacon-APIs/#/ValidatorRequiredApi/produceBlockV3 (all versions, blinded, unblinded etc allow supplying graffiti to the BN)

Once the block is produced, that's what is then passed to the web3signer for signing, complete with the graffiti that was included during that create unsigned call...

@rolfyone
Copy link
Collaborator

Any further comments? Looks like at least Prysm/teku happy... @michaelsproul or @jimmygchen , @dapplion, @nflaig ?

@rolfyone rolfyone merged commit a8c45a1 into ethereum:master Nov 24, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants