sigp / lighthouse

Ethereum consensus client in Rust
https://lighthouse.sigmaprime.io/
Apache License 2.0
2.88k stars 724 forks source link

Migrate from AbstractPayload -> `superstruct(meta_variants)` #5176

Open realbigsean opened 7 months ago

realbigsean commented 7 months ago

Description

I'm working on adding some features to superstruct that should make it easier to work with blinded vs. full payloads as enums https://github.com/sigp/superstruct/pull/35

It will probably be a very difficult migration away from AbstractExecPayload, but I think it'd end up making our types a lot more clear and consistent.

Blocks V3 got me thinking about this because it can return either a blinded or full payload. Because of this we've opted to use concrete types a Blinded vs Full enum in the APIs. At some point we have to convert these types to our generic types though. This is a bug where this conversion ended up biting us: https://github.com/sigp/lighthouse/pull/5005/files BlockProposalContents is both generic over AbstractExecPayload and an enum over blinded vs. full (essentially). If we opt for superstruct maximalism we should be able to get rid of this type of conversion (and types where the generic and enum variants are reduntant).

I also think this would end up being a solid complexity reduction because we should be able to just delete the the payload.rs file. If we do need to add a new variant to payloads in addition to blinded vs full, I think this should make it easier.

This is a minimal example of how it'd look in lighthouse:

    #[superstruct(
        variants(Merge, Capella),
        variant_attributes(derive(Debug, PartialEq, Clone)),
    )]
    #[derive(Clone, PartialEq, Debug)]
    pub struct Payload {
        pub transactions: u64,
    }

    #[superstruct(
        variants(Merge, Capella),
        variant_attributes(derive(Debug, PartialEq, Clone)),
    )]
    #[derive(Clone, PartialEq, Debug)]
    pub struct PayloadHeader {
        pub transactions_root: u64,
    }

    #[superstruct(
        meta_variants(Blinded, Full),
        variants(Base, Merge, Capella),
        variant_attributes(derive(Debug, PartialEq, Clone)),
    )]
    #[derive(Clone, PartialEq, Debug)]
    pub struct Block {
        #[superstruct(flatten(Merge, Capella), meta_only(Full))]
        pub payload: Payload,
        #[superstruct(flatten(Merge, Capella), meta_only(Blinded))]
        pub payload_header: PayloadHeader,
    }

    let block = Block::Full(BlockFull::Merge(BlockFullMerge {
        payload: PayloadMerge { transactions: 1 },
    }));
    let blinded_block = Block::Blinded(BlockBlinded::Merge(BlockBlindedMerge {
        payload_header: PayloadHeaderMerge {
            transactions_root: 1,
        },
    }));

And you should be able to keep using this flatten pattern all the way up to our api types (PublishBlockRequest).

Interested what people think.

dapplion commented 7 months ago

+1 on the change