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 versioned attestation and attester slashing events #461

Closed
wants to merge 4 commits into from

Conversation

nflaig
Copy link
Collaborator

@nflaig nflaig commented Aug 5, 2024

Alternative solution to #459 which adds versioned attestation and attester slashing events.

As highlighted in the other PR, bumping the whole events api to v2 introduces a lot of implementation complexity while adding two new events should be trivial.

The un-versioned events will only support phase0 types and hence consumers need to use to versioned events post-electra. From a client implementation perspective, the best solution is likely to stop emitting un-versioned events altogether on fork transition instead of converting electra to phase0 objects.

Part of #445

@dapplion
Copy link
Collaborator

dapplion commented Aug 5, 2024

What if consumers first decode a type

{
  data: {
    slot: u64
  }
}

and use the slot number to decode the attestation. Since it's a stream event 2 epochs after the fork all attestations will be electra. A bit hacky but the *_versioned solution doesn't feel too clean either. Will we keep adding *_versioned topics as data structures change?

@nflaig
Copy link
Collaborator Author

nflaig commented Aug 5, 2024

What if consumers first decode a type

That would work as well and simplifies things a little bit when emitting events, I am open to that solution but from previous discussion there was preference to use a versioned container.

Will we keep adding *_versioned topics as data structures change?

Depends on if we change the types I listed here #459 (comment), that's why another solution would be to version all those and bumping the apis to v2.

The un-versioned events are deprecated now and could be removed in the future so at least some cleanup in that regard, although the event name would not be as clean.

@rkapka
Copy link
Collaborator

rkapka commented Aug 13, 2024

I think I am slightly leaning towards leaving this work to the consumer.

@mcdee
Copy link
Contributor

mcdee commented Aug 13, 2024

Having a v2 of the events stream with every event versioned looks like a cleaner long-term solution.

@rolfyone
Copy link
Collaborator

Having a v2 of the events stream with every event versioned looks like a cleaner long-term solution.

If we do go down the v2 route, can we make sure we have version on every object? otherwise this is just going to repeat itself next time...

@nflaig
Copy link
Collaborator Author

nflaig commented Aug 23, 2024

Closing this as preference seems to be to either go with a v2 or not do anything at all and extract slot to determine fork type. We have implemented the later on Lodestar for now as it does not require any spec change.

Let's continue discussion regarding v2 in #459

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