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

Validator identities endpoint. #452

Merged
merged 8 commits into from
Sep 24, 2024
Merged

Conversation

mcdee
Copy link
Contributor

@mcdee mcdee commented Jun 10, 2024

In support of #449.

@mcdee mcdee mentioned this pull request Jun 10, 2024
@rolfyone
Copy link
Collaborator

This looks good. Will cross-post and seek more opinions.

pubkey:
$ref: './primitive.yaml#/Pubkey'
description: "Public key of validator."
activation_epoch:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why add the activation_epoch? It's a curious in-between of no epoch and all epochs in the validator record

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's in case validator index reuse comes in, it will allow us to use (index,public key, activation epoch) as a unique identifier for a validator across all time regardless of reuse.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Index re-use won't come until years down the line if ever. Is it better to do a v2 in 2030 when that happens?

Copy link
Collaborator

Choose a reason for hiding this comment

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

i was thinking there's no harm having it?

@rkapka
Copy link
Collaborator

rkapka commented Jun 13, 2024

I support this. Our validator client currently needs to grab the full validator to get the index

@nflaig
Copy link
Collaborator

nflaig commented Jun 13, 2024

I support this. Our validator client currently needs to grab the full validator to get the index

Curious, do you track the validator status on the VC side? This has a lot of benefits, e.g. not querying duties for exited validators, not registering to builder etc. but it would still require to query the full validator object if we don't include all information in this API to recalculate the status, or include the status itself here, preferably in a format that can be ssz-serialized.

@rkapka
Copy link
Collaborator

rkapka commented Jun 17, 2024

Curious, do you track the validator status on the VC side?

No, we don't. I agree it would allow for certain optimizations, but I am not sure if it's worth the extra complexity.

@rolfyone
Copy link
Collaborator

I think i prefer the idea that this is a simple index lookup that would allow us to continue to use it in a post index being unique world, and for the rest of the data you have the indices you could look up further info...

@rolfyone
Copy link
Collaborator

we can definitely add this to changes table though...

match any known validator, no identity will be returned but this will not cause an error. There are no guarantees for the
returned data in terms of ordering.

Depending on `Accept` header data can be returned either as JSON or as bytes serialized by SSZ.
Copy link
Collaborator

Choose a reason for hiding this comment

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

was wondering if we wanna state this here, seems a bit redundant considering we have a more general note about this now since #457 and it's already implied by the response content type + the Accept header is mentioned there as well.

The reason to add the note is that it seems consistent with other endpoint that support SSZ, however could consider cleaning those up a bit

Copy link
Collaborator

Choose a reason for hiding this comment

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

i'd be happy to just merge and cleanup as a separate task...

@rolfyone rolfyone merged commit 30f0568 into ethereum:master Sep 24, 2024
3 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