EIP-7549: Append new committee_bits field to end of Attestation

[etan-status]

Introducing new fields in the middle of an existing Container pointlessly breaks merkleization of all subsequent fields. In the case of committee_bits, it is also misleading, as signature only covers data inside Attestation.

[mkalinin]

This sounds quite reasonable to me! I’d wait for more approvals from CL client devs and then merge it

[etan-status]

The other open question is, that, technically, changing List limits also affects the shape of the Merkle proof...

If we want to stop breaking smart contract verifiers randomly, we have to consider using theoretical limits as the List capacity, and then use a state transition logic to enforce the fork-specific limit, similar to how we do it for the MAX_BLOBS_PER_BLOCK.

With Electra, we get a clean slate and could revisit data types and update limits accordingly. Especially with EIP-7495 StableContainer in mind.

[ensi321]

In Lodestar we cache seen attestations by extracting (attestation data + committee bits). Currently it is a convenient one-time slice from the ssz bytes since attestation data and committee bits are together.

This proposal would mean we do slicing twice. Minor annoyance but doable.

[etan-status]

We should also consider renaming aggregation_bits to avoid subtle breakage of clients who expect aggregation_bits to have its old meaning. Maybe something like committee_validator_bits, in line with sync_committee_bits for SyncAggregate, as this spans the entire validator set assigned to the slot. Or, participant_bits, to match the process_attestation naming:

    # [Modified in Electra:EIP7549]
    assert data.index == 0
    committee_indices = get_committee_indices(attestation.committee_bits)
    participants_count = 0
    for index in committee_indices:
        assert index < get_committee_count_per_slot(state, data.target.epoch)
        committee = get_beacon_committee(state, data.slot, index)
        participants_count += len(committee)

    assert len(attestation.aggregation_bits) == participants_count  

Note that with EIP-7688 based forward compatibility (StableContainer), field names get bound to types more strictly and can no longer randomly be changed across forks. The way forward is to stop using the old field and append a new field with the new type. This is in line with how historical_summaries replaced historical_roots in Capella, or how previous_epoch_participation/current_epoch_participation replaced previous_epoch_attestations/current_epoch_attestations in Altair. We should keep this going, and avoid introducing fork specific meanings for field names!

[etan-status]

As for naming, one suggestion how it could look:

  Attestation(Profile[StableAttestation]):
    data: AttestationData
    signature: BLSSignature
    participant_bits: Bitvector[
        MAX_VALIDATORS_PER_COMMITTEE * MAX_COMMITTEES_PER_SLOT]  # [New in Electra:EIP7549]
    committee_bits: Bitvector[MAX_COMMITTEES_PER_SLOT]  # [New in Electra:EIP7549]

  IndexedAttestation(Profile[StableIndexedAttestation]):
    data: AttestationData
    signature: ValidatorSig
    participant_indices*: List[uint64,
        MAX_VALIDATORS_PER_COMMITTEE * MAX_COMMITTEES_PER_SLOT]  # [New in Electra:EIP7549]

Rationale:

1. The phase0 aggregation_bits and attesting_indices have length MAX_VALIDATORS_PER_COMMITTEE, so are insufficient to cover the new list lengths.
   • Removing them prevents any subtle bugs where code assumes the old length.
   • Removing them also ensures that existing clients using the fields (smart contracts etc) _will_notice that a software update / redeployment is required. Better than have them deal with subtle bugs.
   • Therefore, remove the fields.
2. As this burns aggregation_bits and attesting_indices field names, we need new names. Use participant_bits / participant_indices for the new lists to align naming with Altair's sync_committee_bits (identify who signed the message with the field name, instead of identifying what the bits represent - all of them are aggregates).
   • Also append to the end, as they are new fields.
3. Consider whether participant_indices even makes sense or whether it should also become participant_bits. We only have 8 attestations now so switching to bitfield may be more efficient.

[mkalinin]

3. Consider whether participant_indices even makes sense or whether it should also become participant_bits. We only have 8 attestations now so switching to bitfield may be more efficient.

Switching to a bitfield in this case would require to obtain the state in order to process attester slashing which i don’t think is desirable.

Do I understand correctly that if we use StableContainer for Attestation then we will have to append participant_bits and leave aggregation_bits? Is the name of the field or its position within the container is important in this case? Can we rename the old one to phase0_attestation_bits? Alternatively, can stable container implementation read the type of the aggregation_bits and conclude that this field has changed?

[etan-status]

My comment on the naming was independent of StableContainer, and backed by aforementioned reasons, namely reducing the potential for subtle bugs, and communicating clearly that client logic consuming the field needs updating.

Existing field names cannot be easily renamed, because JSON is used on the builder API, so, it is not possible to suddenly transition existing aggregation_bits to phase0_aggregation_bits.

You are correct that if we already had StableContainer back in Deneb, that it would force the clean transition of deprecating the old field and introducing a new one, when the field type changes. However, StableContainer is not in Deneb, so there are no technical requirements to follow such standards at this time.

I cannot recall a past fork, though, where a field retained its name across type changes (except when inner containers got extended, which is also still allowed as today in a StableContainer world).

If people don't like renaming to participant_bits/participant_indices and would prefer to go with the PR as is, that's also fine by me. I only have a weak opinion on the naming aspect.

require to obtain the state in order to process attester slashing

How would an attester slashing be processed without access to the state?

You are right, though, IndexedAttestation is only used in AttesterSlashing, indeed, so possible gains would not scale. I confused this type with the one for the actual Attestation, where the bitfield is already used.

[mkalinin]

How would an attester slashing be processed without access to the state?

IndexedAttestation makes slashing processing rely on index -> pubkey mapping only which is available in a recent state copy. If it were bits instead then shuffling in the exact epoch would be required which is much harder to compute. I should have mentioned the access to the states of the epochs where each attestation belongs to.

[etan-status]

OK, let's keep the indices as is then.

Only remaining question then is whether the lists that changed capacities should be renamed to participant_indices / participant_bits to ensure that any software using these lists gets a proper update for the new logic.

[mkalinin]

Only remaining question then is whether the lists that changed capacities should be renamed to participant_indices / participant_bits to ensure that any software using these lists gets a proper update for the new logic.

Aren’t implementations introduce a new type whenever the struct is changing? Wouldn’t this mean that they also check where the old version is used and change the logic of those places accordingly? If the new type would name the bitfield differently it doesn’t allow to avoid checking where the old one is used in the code.

[etan-status]

Aren’t implementations introduce a new type whenever the struct is changing?

Yes, they have to be aware of the change though and make sure to actually update all of the logic. That extends beyond CL full node implementations to any clients of beacon-API or smart contract.

Wouldn’t this mean that they also check where the old version is used and change the logic of those places accordingly? If the new type would name the bitfield differently it doesn’t allow to avoid checking where the old one is used in the code.

It forces more oversights to be compilation errors instead of runtime errors, as the entire category of bugs where someone accidentally uses parts of the phase0 logic post Electra is ruled out.

It also discourages shortcut-taking. For example, an implementation may switch logic depending on the capacity of aggregation_bits and use phase0 if it's MAX_VALIDATORS_PER_COMMITTEE and electra if it's MAX_VALIDATORS_PER_COMMITTEE * MAX_COMMITTEES_PER_SLOT because of it being slightly more effort to properly route fork information through the code base. Which may lead to random bugs in edge cases such as network with MAX_COMMITTEES_PER_SLOT = 1 when the types suddenly start aliasing.

Wouldn’t this mean that they also check where the old version is used and change the logic of those places accordingly?

Ultimately, implementations have to adjust logic everywhere to be correct / compliant. Forcing new field names when the type / semantics change reveals oversights sooner, but the overall work required is obviously still the same.

One aspect to keep in mind is that this also affects non-CL implementations. For example, validator reward smoothing pools may track attestation performance of each validator for computing their share of the pooled funds. If they continue consuming the existing aggregation_bits field on beacon-API with existing logic, it breaks; they have to know that suddenly they have to start also looking at the slot number of the surrounding block to determine if existing or new logic is necessary. If they don't change anything due to lack of awareness, the existing logic is used, possibly breaking the code in weird ways. With a new field name, e.g., participant_bits, it is guaranteed that the existing code breaks in a clean way and that existing logic cannot be used by accident.

How highly to prioritize these aspects I'm not sure. Generally, forward / backward compatibility issues have a tendency of only getting attention once something blew up.

Personally I'd accept both 'keeping names as is' and 'rename the fields that got new semantics', with slight preference for the latter but also no strong opposition for the former.

[rkapka]

Introducing new fields in the middle of an existing Container pointlessly breaks merkleization of all subsequent fields

Does this have any practical consequences?

In the case of committee_bits, it is also misleading, as signature only covers data inside Attestation.

aggregation_bits are also not signed over and they come before the signature field, so having committee_bits there doesn't set a precedent.

Actually, if we are to change the field's position, then the type definition would be the most readable to me if both bits fields are next to each other. And I like readability 😄

[etan-status]

Does this have any practical consequences?

Not for Ethereum CL and EL core implemenations themselves, but it affects smart contracts that verify beacon state proofs using EIP-4788.

aggregation_bits are also not signed over and they come before the signature field

Yes, the order is misleading, it's this way for historical reasons, and incorrectly suggests that the signature signs over them.

Actually, if we are to change the field's position, then the type definition would be the most readable to me if both bits fields are next to each other.

True, but field order determines the "shape" of the Merkle proofs. While appending new fields is fine to some extent (it doens't move existing fields), inserting things in the middle is breaking.

As for readability, that could be solved by implementations. e.g., you could implement it by semantically grouping the field accessors (while still keeping the memory layout append-only). The order in which fields are pushed to the network / into Merkle proofs does not necessarily have to be linked to the semantic groups.

[tbenr]

I don't feel strongly about aggretation_bits to participant_bits, simply because in teku we abstracted away the two attestation types and we will deal with an interface that is going to hide the difference anyway. We will probably adopt the new name on the interface, 'cose I agree it is better, but it will introduce a name inconsistency for phase0 type (but we can leave with it)

[etan-status]

To clarify, namings have to be aligned on, as they appear in JSON builder API (mevboost) and in beacon API (for developers only, real tools should use SSZ)

[ralexstokes]

if we are going to change this, I do have a preference to put committee_bits next to aggregation_bits, with a slight preference to have it first

the status quo already breaks the generalized indices so this would be no-change on that front; however, it would prevent the "have data after the signature in some Container" issue