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

[nflaig]

applying this PR together with #3787

I think the idea is to be more explicit about aggregated attestations, or would those just use the electra modifiedAttestation container?

[tbenr]

I've spent some time on this and the related #3787
My current impression is that together they add complexity in code and some impact on API (including event stream).
At the current stage I'm not in favour of considering for inclusion.

I see the impact of having SingleAttestation and we could consider including only this change, but still API (including event stream) make me doubting about cost\benefits as of right now.

[arnetheduck]

API (including event stream).

I would probably not change the event stream for the same reason as we left index in AttestationData - this affects a greater amount of downstream tooling and I agree this would probably not be worth it. In part, this is why the PR focuses on the network format of the gossip channel which is critical to security in a way that APIs aren't (because it's exposed to the wider p2p network).

The event stream encoding can trivially be generated from SingleAttestation.

[rkapka]

We started implementing this in Prysm and found that implementing this is not so trivial - quite a few changes are required throughout the codebase. It would be better to postpone this until the next fork and implement it along with #3787.

[nflaig]

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.

What kind of changes are required here? Slashing protection only operates on the data (type AttestationData) which is unaffected by this change.

[eserilev]

What kind of changes are required here? Slashing protection only operates on the data (type AttestationData) which is unaffected by this change.

Signature verification is one example of a check that doesn't operate on AttestationData

My comments were more directed at how Lighthouse has implemented these codepaths. Though there are some checks that only operate on AttestationData we are, in many cases, passing the full Attestation object to these methods. Introducing this new type to those codepaths will require a significant refactor.

In the interim we've decided to simply translate SingleAttestation to Attestation when received over gossip. This keeps the rest of the downstream code unchanged while being fully spec compliant for any upcoming devnets. In parallel we'll be working on a larger refactor.

[dapplion]

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

In normal network conditions, clients will either already have the shuffling cached and have to cache it for later processing of aggregate attestations.

If under a deliberate DOS attack, a malicious peer can still trigger wasteful shuffling computations by forging aggregated attestations

[arnetheduck]

If under a deliberate DOS attack, a malicious peer can still trigger wasteful shuffling computations by forging aggregated attestations

The aggregate topic uses an envelope that is already signed by a validator - it does not offer the opportunity of DoS by a non-validator as the attestation topic does (see previous discussion).

[dapplion]

The #3900 (comment) uses an envelope that is already signed by a validator - it does not offer the opportunity of DoS by a non-validator as the attestation topic does (see previous discussion).

I see this as a strong point in favor of this PR. With this PR only validators can attack nodes to compute wasteful epoch transitions. So we'll have attributability in case of an attack.

[ppopth]

I think this should not be named "SingleAttestation". This should be "Attestation" while the other is "AggregatedAttestion". When people see "Attestation", they more likely think of an attestaion (a single one) rather an aggregated one.

[rkapka]

Would things be even more clear if we had a SingleAttestation and an AggregatedAttestation (and potentially an OnChainAttestation as per #3787), and just let go of the generic Attestation?

[arnetheduck]

I'm happy to pick any name that gets consensus ;)

I agree that things would be better if #3787 was also merged and if the aggregated attestation was renamed, but not to the point that I want this PR to be delayed by it as the severity of the issue they fix is different.

In Nimbus, it's trivial to implement #3787 simply by changing a constant - we're planning on having a separate OnchainAttestation type anyway because they appear in different contexts and are conceptually different even if the spec shoehorns them into one type. If you want to experiment with the complexity of implementing it, this is one way to go and then we can decide on 3787 separately from this PR.

Notably though, any renames can also be done after the substance of this PR has been merged.

[tbenr]

+1 for detaching this from eventual renamings. Let's merge as is.

[ensi321]

In Nimbus, it's trivial to implement #3787 simply by changing a constant - we're planning on having a separate OnchainAttestation type anyway because they appear in different contexts and are conceptually different even if the spec shoehorns them into one type. If you want to experiment with the complexity of implementing it, this is one way to go and then we can decide on 3787 separately from this PR.

This is Lodestar's experience as well. If we decide to merge #3900, might as well merge #3787 too since it's not that hard to implement.