consensus-specs icon indicating copy to clipboard operation
consensus-specs copied to clipboard

Separate type for onchain attestation aggregates

Open mkalinin opened this issue 1 year ago • 4 comments

Experimental PR outlining an idea of having a separate container for on chain attestation aggregates proposed by @arnetheduck during ACDC#134.

Change set:

  • New OnchainAttestation type to be used for aggregates included into a block with the corresponding process_onchain_attestation function used during the block processing; the old process_attestation is preserved for the purpose of gossip and attestation mempool
  • New compute_signing_attestation_data which is used to compute signing attestation data. The reason for that is to keep the logic around Attestation container 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-zero index field inside of the Attestation this 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. OnchainAttestation contains signing AttestationData (i.e. with index always set to 0). The old process_attestation function should switch to the modified is_valid_indexed_attestation (not explicit in this PR)
  • p2p changes related to the modification of the Attestation type 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 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
  • Fork choice on_attestation handler will have to be appended with on_onchain_attestation to handle the on chain aggregate type

P.S. The above scope of changes might not be exhaustive.

mkalinin avatar Jun 04 '24 10:06 mkalinin

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.

dapplion avatar Jun 05 '24 12:06 dapplion

+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.

arnetheduck avatar Jun 05 '24 20:06 arnetheduck

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.

arnetheduck avatar Aug 28 '24 05:08 arnetheduck

IINM we could drop https://github.com/ethereum/beacon-APIs/blob/master/apis/validator/aggregate_attestation.v2.yaml with this change.

tbenr avatar Sep 20 '24 16:09 tbenr