Separate type for unaggregated network attestations

[arnetheduck]

As a complement to
#3787, this PR introduces a SingleAttestation type used for network propagation only.

In Electra, the on-chain attestation format introduced in EIP-7549 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.

[ralexstokes]

will wait for other client teams to chime in around implementation benefits but PR description makes sense and changes look reasonable

[mkalinin]

Attestation signature verification in the existing protocol implicitly runs committee association check. Without this check and with EIP-7549 a committee_index can be tampered and a subnet can be flooded with attestations from attesters that doesn’t belong to the subnet’s committee. Before EIP-7549 this wouldn’t be a problem as signing incorrect committee_index would be quite expensive attack.

The committee association check requires shuffling computations. Given the EIP-7549 can we avoid this check anyhow? @arnetheduck

[arnetheduck]

The committee association check requires shuffling computations.

The committee_index still needs to be checked but crucially, with this change we can do so after signature verification meaning that only validators can create new shufflings. This is why the PR doesn't remove the committe index check from the list of required checks.

Since the shuffling order is needed to tell whether the validator should vote in the particular slot, I don't see that it can be avoided without more extensive surgery to the protocol.

Edit: made this explicit in 5761fb4 - I thought there was a check like this but there actually isn't :)

[mkalinin]

The committee_index still needs to be checked but crucially, with this change we can do so after signature verification meaning that only validators can create new shufflings.

Considering two scenarios:

1. Adversary creates fake attestations with invalid signatures making nodes wasting computational resources on obtaining the shuffling required to verify the signature
2. Adversary relays valid attestations from other subnets tampering committee_index

The proposed change prevents (1) but doesn’t help with (2). Is this correct?

[arnetheduck]

The proposed change prevents (1) but doesn’t help with (2). Is this correct?

the shuffling is computed from the signed fields (target / block_root depending on design), so this is harmless to the extent that this shuffling will be signed by a validator (and therefore likely cached)

[ralexstokes]

I think the combination of these two conditions from the phase0 p2p spec are sufficient to handle DoS concerns around committee_index:

[REJECT] The committee index is within the expected range -- i.e. index < get_committee_count_per_slot(state, attestation.data.target.epoch).
[IGNORE] There has been no other valid attestation seen on an attestation subnet that has an identical attestation.data.target.epoch and participating validator index.

If we are going to have committee_index handy anyway, its not a big lift to add the explicit check as a gossip validation so I'd support tightening further

[arnetheduck]

add the explicit check

broadly, the phase0 conditions still apply since they reference index which we replace in this PR.

[eserilev]

If this change is introduced I believe we should also make a change to the submitPoolAttestationsV2 beacon-api endpoint

PR: ethereum/beacon-APIs#472

[eserilev]

I wanted to surface a small concern I had regarding treating this PR as a minimal change. Introducing the SingleAttestation type requires us to make the following changes (not an exhaustive list)

1. Verification + Slashability checks need to be implemented for this new type. These are sensitive codepaths and will now need to be either significantly refactored or duplicated to handle this new type.

2. The VC/BN now needs to handle two separate types pre/post electra instead of two variants of the same type

3. We will need to make some changes to the beacon api

I'm not against this change, but I just wanted to push back on the idea of it being "minimal". I think we all sort of learned our lesson from EIP-7549 that changes to attestations might look simple at the surface, but end up being more complex in practice.

[dapplion]

Echoing @eserilev concerns, we may force on ourselves another decent churn of type changes across the codebase. While I see the benefits of this change I believe the upside of EIP-7549 is much more significant and impactful towards Ethereum participants than this change. The unified type of Attestation is annoying yes, but our codebases are already implemented to handle it.

[arnetheduck]

of it being "minimal"

minimal here means that it does not affect the consensus spec significantly since the parts outside of the state transition are much less strictly specified leaving clients more leeway to implement this as they wish - in fact, one possible implementation is to simply translate to Attestation in the gossip handler which indeed would be a "minimal" implementation that requires no further changes.

Also notable is that the comparison is done not to the work-in-progress spec but rather the deneb spec: seen through this lens, this PR (together with 3787) makes the total change of going from deneb to electra much smaller (since the changes much less invasive), even if in-between there existed a high-churn version in the development version of electra.

If anything, the churn should really be attributed EIP-7549 which changed more than it needed (had it introduced just the on-chain change, it would have had much smaller impact) and with #3787 we're just reverting some of that and simplifying further).

Strictly, the REST API changes are not necessary and should be judged on their own merit (since the full Attestation object is a superset).

[terencechain]

Wouldn't you want to update the validator spec here as well, in particular how validator constructs the attestation should use SingleAttestation?
Example: https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/validator.md#construct-attestation

[mkalinin]

If anything, the churn should really be attributed EIP-7549 which changed more than it needed (had it introduced just the on-chain change, it would have had much smaller impact)

The core change introduce by EIP-7549 is removing the committee_index from the signing data which unlocked the opportunity to pack attestations more tightly. The form of the on chain aggregate isn’t a source of the vast majority of the engineering complexity around this EIP. Having a separate type for on chain aggregate is cleaner, but isn’t crucial IMO, tho client devs might disagree

[arnetheduck]

Wouldn't you want to update the validator spec here as well, in particular how validator constructs the attestation should use SingleAttestation?

Agreed: 1c529a8

[arnetheduck]

Given #3900 (comment)
there is an impact on Web3Signer API to introduce SINGLE_ATTESTATION signing request type

this is not 100% necessary - we can also translate back and forth - Attestation, SingleAttestation etc are just envelopes - one reason we kept AttestationData intact with the zeroed index was indeed to avoid changing web3signer and other third-party tooling that just blindly signs what it is given.

Notably, web3signer is trusted so the rationale about shufflings doesn't apply and since this is the producer, the performance argument is less important as well (since you don't have to do it thousands of times per slot).

All in all, I have no strong opinion either way.

[tbenr]

@arnetheduck
I nearly immediately deleted the comment because double checking the Web3Signer API i saw only AttestationData is sent. So I'm ok with leaving everything untouched there.