ethereum / beacon-APIs

Collection of RESTful APIs provided by Ethereum Beacon nodes
https://ethereum.github.io/beacon-APIs/
Creative Commons Zero v1.0 Universal
328 stars 167 forks source link

Add versioned attestation and attester slashing events #461

Closed nflaig closed 3 weeks ago

nflaig commented 1 month ago

Alternative solution to https://github.com/ethereum/beacon-APIs/pull/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 https://github.com/ethereum/beacon-APIs/issues/445

dapplion commented 1 month ago

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 commented 1 month ago

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 https://github.com/ethereum/beacon-APIs/pull/459#issuecomment-2232984112, 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 commented 1 month ago

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

mcdee commented 1 month ago

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

rolfyone commented 3 weeks ago

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 commented 3 weeks ago

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 https://github.com/ethereum/beacon-APIs/pull/459