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 eventstreamV2 #459

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Add eventstreamV2 #459

wants to merge 7 commits into from

Conversation

ensi321
Copy link
Contributor

@ensi321 ensi321 commented Jul 17, 2024

We need to communicate fork version during attestation and attester_slashing since it is modified by EIP-7549.

For example, "aggregation_bits":"0x01" will mean different validators in pre and post-EIP-7549.

This PR adds eventstreamV2 which has version in attestation and attester_slashing

Related to #445

@ensi321 ensi321 changed the title Add version field to attestation and attestation_slashing in eventstream Add eventstreamV2 Jul 17, 2024
@nflaig
Copy link
Collaborator

nflaig commented Jul 17, 2024

I am in favor of bumping the whole api to v2 instead of just single topics (previous discussion) but I think we should use this opportunity to make other topics more forward compatible to not have to bump the api again at some (unknown) point in the future.

Further topics we could wrap inside version container are

  • voluntary_exit: SignedVoluntaryExit (phase0 container of spec)
  • proposer_slashing: ProposerSlashing (phase0 container of spec)
  • bls_to_execution_change: SignedBLSToExecutionChange (capella container of spec)
  • contribution_and_proof: SignedContributionAndProof (altair container of spec)

In the worst case if those never happen to change in the future, it would at least make the format of events more consistent

@rolfyone
Copy link
Collaborator

rolfyone commented Jul 18, 2024

i think if we're wanting to go this way we want to put version on every type...

One option would be to just add versioned_attestation to event stream v1... It lets us version out the attestations with no change to the eventstream, and just change the single message we're wanting to update. in future we can do the same at any point with any other event message... This is probably a MVP type solution if we updated V1.

This lets us not duplicate our eventstream framework, im not sure if that only affects our specific endpoints... currently teku has a whole framework around the event stream so this might be a bit of fun (not saying to avoid it, but just as context)

@nflaig
Copy link
Collaborator

nflaig commented Jul 18, 2024

One option would be to just add versioned_attestation to event stream v1

If we do that, does the attestation topic just handle phase0 attestations then?

This lets us not duplicate our eventstream framework

yeah, we need more feedback from others on the implementation complexity here, might not be as trivial as bumping other apis, it's also a bit more effort on Lodestar side to do this.

@rolfyone
Copy link
Collaborator

rolfyone commented Sep 23, 2024

If we do that, does the attestation topic just handle phase0 attestations then?

yes i would expect that it would just handle phase0, and you'd need a new topic for the versioned attestation format...

The event stream is basically a bunch of different objects anyway, so we could do
attestation - only supports phase0 attestation types
versioned_attestation - supports all attestations, and has version information embedded

then in future if we need something else versioned, we can just build that out... I really don't mind the name, we could potentially just do v_attestation if length is a problem, but i'd probably prefer the readability unless theres a strong preference.

@rolfyone
Copy link
Collaborator

quick update - currently its looking like we may be able to avoid this change, I've raised a question in discord.

@nflaig
Copy link
Collaborator

nflaig commented Sep 25, 2024

Not in favor of a v2 api due to high code duplication and maintenance overhead of the server-side implementation

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.

3 participants