superstruct the AttesterSlashing

[ethDreamer]

Issue Addressed

• Implement Electra Fork #5605

Proposed Changes

There's gonna be a lot of sub issues tied together in this branch as it's the only way we'll get this to compile..

[eserilev]

just a question, why are we superstruct-ing AttestationSlashing? Is it to handle some fork boundary edge case? If so, are we going to have separate fields for the base and electra variants of IndexedAttestation?

[realbigsean]

the change in IndexedAttestation makes it necessary since slashings are included in blocks. So a deneb block needs to have the deneb IndexedAttestation and same for electra.

are we going to have separate fields for the base and electra variants of IndexedAttestation

I believe that's the plan, right @ethDreamer ?

[ethDreamer]

yeah I thought the plan was to use superstruct(flatten) for the IndexedAttestation so that the types would be embedded

[dapplion]

why are we superstruct-ing AttestationSlashing?

The change is only relevant when computing the block root. In all other codepaths the difference does not matter

[michaelsproul]

This is a breaking change to the on-disk format, because we add the tag. I would ideally like to release v5.2.0 without a schema upgrade, so that if anything goes wrong with tree-states, users are able to downgrade easily.

However this is not a hard requirement, so we have a few options:

1. Do the database upgrade and include in v5.2.0
2. Wait to merge this PR until after v5.2.0 (maybe not a good option with interop fast approaching, but feasible if we fast-track v5.2.0)
3. Merge this PR prior to v5.2.0 with a temporary implementation of Decode which tries both variants in order. This is obviously janky and not ideal, so the plan would be to clean it up in a subsequent PR that does introduce a schema upgrade.

My preference is for (2), then (3), then (1)

[ethDreamer]

I'm in favor of fast tracking 5.2.0 because there's gonna be a ton of changes in Electra and it's probably best to separate all the other changes that have been merged in from the Electra code.

[dapplion]

Note that you can infer the tag with the current database schema. A phase0 AttesterSlashing is:

class AttesterSlashing(Container):
    attestation_1: IndexedAttestation
    attestation_2: IndexedAttestation

Where IndexedAttestation is a variable size type. So the first 8 bytes of the serialized stream are offset. The first offset will always be 00000008 and the second is variable on the size of the first attestation. So a non-tag AttesterSlashing always starts by 00000008 or just 00 and a tagged AttesterSlashing can start by 0100000008 or just 01. Now for 00 the deserializer should consider the tag size to process the rest of the stream, or append a single zero byte

[michaelsproul]

@dapplion I'm fine with that kind of implementation so long as it's well-tested. We don't have any machinery to generate code like that, so it will need to be carefully written by hand and kept up to date.