Separate type for onchain attestation aggregates#3787
Separate type for onchain attestation aggregates#3787mkalinin wants to merge 3 commits intoethereum:masterfrom
Conversation
|
I had the impression that @arnetheduck main goal was to have a separate type (e.g. |
|
+1 for |
As a complement to ethereum#3787, this PR introduces a `SingleAttestation` type used for network propagation only. In Electra, the on-chain attestation format introduced in [EIP-7549](ethereum#3559) presents several difficulties - not only are the new fields to be interpreted differently during network processing and onchain which adds complexity in clients, they also introduce inefficiency both in hash computation and bandwidth. The new type puts the validator and committee indices directly in the attestation type, this simplifying processing and increasing security. * placing the validator index directly in the attestation allows verifying the signature without computing a shuffling - this closes a loophole where clients either must drop attestations or risk being overwhelmed by shuffling computations during attestation verification * the simpler "structure" of the attestation saves several hash calls during processing (a single-item List has significant hashing overhead compared to a field) * we save a few bytes here and there - we can also put stricter bounds on message size on the attestation topic because `SingleAttestation` is now fixed-size * the ambiguity of interpreting the `attestation_bits` list indices which became contextual under EIP-7549 is removed Because this change only affects the network encoding (and not block contents), the implementation impact on clients should be minimal.
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. |
Co-authored-by: realbigsean <[email protected]>
Co-authored-by: Mehdi AOUADI <[email protected]>
| aggregation_bits: Bitlist[MAX_VALIDATORS_PER_COMMITTEE * MAX_COMMITTEES_PER_SLOT] | ||
| data: AttestationData | ||
| committee_bits: Bitvector[MAX_COMMITTEES_PER_SLOT] | ||
| signature: BLSSignature |
There was a problem hiding this comment.
I think we want to be consistent with the current Attestation by having committee_bits to be after the signature field
| @@ -91,7 +89,7 @@ def compute_on_chain_aggregate(network_aggregates: Sequence[Attestation]) -> Att | |||
| committee_flags = [(index in committee_indices) for index in range(0, MAX_COMMITTEES_PER_SLOT)] | |||
| committee_bits = Bitvector[MAX_COMMITTEES_PER_SLOT](committee_flags) | |||
There was a problem hiding this comment.
committee_bits is computed from Attestation.committee_bits. If that's true should we drop the checks in p2p-interface.md?
| ##### `beacon_aggregate_and_proof` | ||
|
|
||
| The following convenience variables are re-defined | ||
| - `index = get_committee_indices(aggregate.committee_bits)[0]` |
There was a problem hiding this comment.
committee_bits is still used in compute_on_chain_aggregate below
specs/electra/p2p-interface.md
Outdated
|
|
||
| The following validations are added: | ||
| * [REJECT] `len(committee_indices) == 1`, where `committee_indices = get_committee_indices(attestation)`. | ||
| * [REJECT] `attestation.data.index == 0` |
There was a problem hiding this comment.
the key thing of EIP-7549 is to have data.index = 0 so we should not remove this? same to above
|
As far as I'm concerned, we can close this - the ship has sailed and given SingleAttestation was implemented, this PR no longer has that much value. |
9 participants
Experimental PR outlining an idea of having a separate container for on chain attestation aggregates proposed by @arnetheduck during ACDC#134.
Change set:
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 mempoolcompute_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)Attestationtype are dropped as the type remain unmodified, except for the signature verification partOther changes that aren’t scoped in this PR:
AttestationandOnchainAttestationtypes, and use different validation functions for them which depending on the pool design might have different impact on the client implementationon_attestationhandler will have to be appended withon_onchain_attestationto handle the on chain aggregate typeP.S. The above scope of changes might not be exhaustive.