consensus-specs
consensus-specs copied to clipboard
Separate type for onchain attestation aggregates
Experimental PR outlining an idea of having a separate container for on chain attestation aggregates proposed by @arnetheduck during ACDC#134.
Change set:
- New
OnchainAttestationtype to be used for aggregates included into a block with the correspondingprocess_onchain_attestationfunction used during the block processing; the oldprocess_attestationis preserved for the purpose of gossip and attestation mempool - New
compute_signing_attestation_datawhich is used to compute signing attestation data. The reason for that is to keep the logic aroundAttestationcontainer that is still used in the network as is as far as it is possible. EIP7549 requires signing data to have committee index set to 0, and to retain the logic around non-zeroindexfield inside of theAttestationthis PR employs a trick when the index is zeroed for the purpose of signing. There is the corresponding change to the honest validator doc and aggregate signature verification.OnchainAttestationcontains signingAttestationData(i.e. with index always set to0). The oldprocess_attestationfunction should switch to the modifiedis_valid_indexed_attestation(not explicit in this PR) - p2p changes related to the modification of the
Attestationtype are dropped as the type remain unmodified, except for the signature verification part
Other changes that aren’t scoped in this PR:
- Attestation mempool will have to support
AttestationandOnchainAttestationtypes, and use different validation functions for them which depending on the pool design might have different impact on the client implementation - Fork choice
on_attestationhandler will have to be appended withon_onchain_attestationto handle the on chain aggregate type
P.S. The above scope of changes might not be exhaustive.
I had the impression that @arnetheduck main goal was to have a separate type (e.g. UnaggregatedAttestation) only used in the beacon_attestation topic. And leave Attestation as is for the rest of usages.
+1 for UnaggregatedAttestation (used on the single-voter attestation topic) - it holds a ValidatorIndex so we don't have to do CommitteeIndex -> ValidatorIndex conversion here - and also +1 for OnChainAttestation - meaning that we leave Attestation as-is (in previous versions) on the aggregate attestation gossip channel.
Attestation mempool will have to support Attestation and OnchainAttestation types, and use different validation functions for them which depending on the pool design might have different impact on the client implementation
Nimbus at least uses custom types in the attestation pool - from a software design perspective, the implementation would be more clean with separate types since these are separate concerns and encodings - the types would prevent accidental leakage of network format into the chain parts by construction.
IINM we could drop https://github.com/ethereum/beacon-APIs/blob/master/apis/validator/aggregate_attestation.v2.yaml with this change.