EIP-7549: Move committee index outside Attestation #3559

dapplion · 2023-12-05T12:18:53Z

Implements https://eips.ethereum.org/EIPS/eip-7549

Motivation

This proposal aims to make Casper FFG clients more efficient by reducing the average number of pairings needed to verify consensus rules. While all types of clients can benefit from this EIP, ZK circuits proving Casper FFG consensus are likely to have to most impact.

On a beacon chain network with at least 262144 active indexes it’s necessary to verify a minimum of ceil(32*64 * 2/3) = 1366 attestations to reach a 2/3 threshold. Participants cast two votes at once: LMD GHOST vote and Casper-FFG vote. However, the Attestation message contains three elements:

LMD GHOST vote (beacon_block_root, slot). Note: includes slot in the event (block, slot) voting is adopted.
FFG vote (source, target)
Committee index (index)

Signing over the 3rd item causes tuples of equal votes to produce different signing roots. If the committee index is moved outside of the Attestation message the minimum number of attestations to verify to reach a 2/3 threshold is reduced to ceil(32 * 2/3) = 22 (a factor of 62).

Deprecation strategy

The index field in AttestationData can be deprecated by:

Removing the field
Preserving the field and setting it to be zero
Changing the field type to Optional (from EIP-7495 StableContainer)

This PR implements the first for simplicity, but open discussion on the rest

hwwhww · 2023-12-06T07:39:49Z

Had a quick scan. So far I agree it was a flawed design in phase0. 😨

For EIP-7549, a note that it seems troublesome to have both Deneb.Attestation and EIP7549.Attestation in EIP7549.BeaconBlock right after the fork boundary. Although we can use SSZ Optional/Union, we have to represent the different versions of the same container name in the spec.

dapplion · 2023-12-06T08:07:48Z

Had a quick scan. So far I agree it was a flawed design in phase0. 😨

For EIP-7549, a note that it seems troublesome to have both Deneb.Attestation and EIP7549.Attestation in EIP7549.BeaconBlock right after the fork boundary. Although we can use SSZ Optional/Union, we have to represent the different versions of the same container name in the spec.

Let's assume the current spec where EIP7549.BeaconBlock can only include EIP7549.Attestation objects:

The first block after the fork will have to contain 0 attestations
Attestations from the last slot before the fork will never be included on chain
Late attestations from the last epoch before the fork will never be included on chain

The attesters at the second into the fork should attest to the first block into the fork, which should become head

W = weight, N= weight of a slot committee

In good network conditions, only the attesters assigned to relative slot -1 will suffer inactivity penalties for 1 epoch. Not sure if I missing some possible instability caused by this anomaly. Otherwise, is this 1 epoch penalty for 1/32 of the validator set okay?

Clients will have to add an extra condition in their block packing algorithm to ignore pre EIP7549 attestations after the EIP7549 fork.

mkalinin · 2023-12-06T08:23:33Z

Wouldn’t it be easier to retain the index field and allow for attestations from epochs before the fork to have it set to non-zero value? I don’t see a big inefficiency in having 8 legacy bytes per attestation (1kb at max per block) neither from chain nor from the network perspective.

dapplion · 2023-12-06T08:53:34Z

packing algorithm to ignore pre EIP7549 attestations after the

The Attestation struct gets a new index in EIP7549 so it's a breaking change from pre-EIP7549. AttestationData structs can be made backwards compatible.

Actually we may want to make AttestationData backwards compatible so AttesterSlashing can be included on chain well after the fork

arnetheduck · 2023-12-06T08:55:58Z

Haven't looked into solutions, but one annoying redundancy is that the target epoch always must match the attestation slot, which pointlessly must be verified everywhere - the target epoch could be removed without any information loss.

dapplion · 2023-12-06T09:13:07Z

Haven't looked into solutions, but one annoying redundancy is that the target epoch always must match the attestation slot, which pointlessly must be verified everywhere - the target epoch could be removed without any information loss.

Definitely I agree here.

But I would like to contain the scope creep for EIP-7549. The impact of reducing a the number of signatures required to verify casper FFG is much more significant than compared to tens of byte / message network bandwidth optimisation

rolfyone · 2023-12-14T04:24:33Z

Actually we may want to make AttestationData backwards compatible so AttesterSlashing can be included on chain well after the fork

i think it's definitely desirable to be able to include attester slashings from before the fork, otherwise we end up in a situation where we have a period of time where you're likely to be able to get away with things.. although i guess the slashing would just have to be converted to the new format?

ppopth · 2023-12-17T07:12:31Z

I think it's good to move the committee index out of the signing root, but is the benefit of this improvement worthy compared to the cost of implementing this?

I think it would be reasonable to do it if we hadn't launched the beacon chain yet, but, since we had, we would have to worry about the fork boundary and that is the cost that we have to bear.

dapplion · 2023-12-18T01:17:58Z

I think it's good to move the committee index out of the signing root, but is the benefit of this improvement worthy compared to the cost of implementing this?

The benefit is massive for constrained verifiers of the Casper FFG. Reduces the count of pairing necessary to verify by 64x. This alone can accelerate how early we see fully trust-less bridges on Ethereum secured by full consensus significantly.

I think it would be reasonable to do it if we hadn't launched the beacon chain yet, but, since we had, we would have to worry about the fork boundary and that is the cost that we have to bear.

When launching the beacon chain this property was necessary in the fully sharded roadmap. So we must bite the bullet to earn the benefits on the roadmap change towards danksharding.

djrtwo · 2023-12-19T14:33:55Z

Haven't looked into solutions, but one annoying redundancy is that the target epoch always must match the attestation slot, which pointlessly must be verified everywhere - the target epoch could be removed without any information loss.

This is not the case. For an attestation to be slashable on any chain (which you want if there are two competing chains), you must explicitly note the target epoch because you don't necessarily have the head block info to do the lookup on a competing chain.

Additionally, for the attestation to be incentivized on a chain in which that head does not exist (e.g. short re-org), the target must be noted explicitly

potuz · 2023-12-19T16:23:45Z

This is not the case. For an attestation to be slashable on any chain (which you want if there are two competing chains), you must explicitly note the target epoch because you don't necessarily have the head block info to do the lookup on a competing chain.

Why is this? if you have the attestation slot you do have already the target checkpoint's epoch. This is independent of any chain or any headroot.

arnetheduck · 2023-12-19T17:39:41Z

you must explicitly note the target epoch

https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/beacon-chain.md#attestations

assert data.target.epoch == compute_epoch_at_slot(data.slot)

djrtwo · 2024-01-23T20:07:48Z

Why is this? if you have the attestation slot you do have already the target checkpoint's epoch. This is independent of any chain or any headroot.

Apologies -- my rebuttal is removing the entire target checkpoint -- (epoch, root) -- rather than just the epoch. I do believe you can remove the epoch (as it is a function of slot) but that you cannot remove the root and adequately incentivize or slash on chains that don't necessarily contained the signed head

djrtwo · 2024-01-23T20:14:08Z

This is not what I expected to happen in this EIP.

I suspected that instead, you would extend the bitfield and allow for full aggregation of the sub-committees from a slot into a single message (fully virtualizing the committee index). Rather than hoisting the committee-index into the Attestation rather than data.

Instead here, these still show up on chain as different messages but when doing verification, you can aggregate further? Is that the idea?

dapplion · 2024-01-24T14:40:24Z

I suspected that instead, you would extend the bitfield and allow for full aggregation of the sub-committees from a slot into a single message (fully virtualizing the committee index). Rather than hoisting the committee-index into the Attestation rather than data.

That would be more data efficient but require a more complex change.

Instead here, these still show up on chain as different messages but when doing verification, you can aggregate further? Is that the idea?

Yes, you can do the bitfield extension on the verifier side without having to change the protocol itself.

djrtwo · 2024-01-25T13:03:54Z

Makes sense. Isolating the change like this seems reasonable

mkalinin · 2024-01-26T07:14:39Z

Potential approach to a more efficient on chain aggregation:

class Attestation(Container):
     aggregation_bits: List[Bitlist[MAX_VALIDATORS_PER_COMMITTEE], MAX_COMMITTEES_PER_SLOT]  # [Modified in EIP7549]
     data: AttestationData
     committee_bits: Bitvector[MAX_COMMITTEES_PER_SLOT]  # [New in EIP7549]
     signature: BLSSignature


def get_committee_indexes(attestation: Attestation) -> List[CommitteeIndex]:
     return [CommitteeIndex(index) for bit, index in enumerate(commitee_bits) if bit]

...

# process_attestation checks
assert len(attestation.aggregation_bits) == len([b for b in attestation.committee_bits if b])
assert len(attestation.aggregation_bits) > 0
for index in get_committee_indexes(attestation):
    assert data.index < get_committee_count_per_slot(state, data.target.epoch)
    committee = get_beacon_committee(state, data.slot, index)
    assert len(attestation.aggregation_bits[index]) == len(committee)

...

# attestation_aggregation topic checks
assert len(attestation.aggregation_bits) == len([b for b in attestation.committee_bits if b])
assert len(attestation.aggregation_bits) == 1

…

# Reduce a limit on a number of on chain attestation
MAX_ATTESTATIONS = 8

This approach makes proposer responsible for a final aggregation which is simply packing aggreation bits into a list, setting corresponding bits in the committee bitvector and aggregating signatures. This approach allows to re-use the same struct for on chain and network attestations.

djrtwo · 2024-01-26T19:19:30Z

I like this technique @mkalinin. It still seems very contained/simple and is certainly a more complete solution

dankrad · 2024-01-26T20:23:25Z

I do like @mkalinin's approach as well. Is committee_bits truly required or can we simply include all committees? It would mostly help with compressing sparse attestations (where many committees are empty), but I don't see that to be a common case.

dapplion · 2024-01-28T07:24:43Z

@mkalinin approach is pretty neat I agree with including it.

How does this change affect the data complexity of AttersterSlashings? When slashing a small fraction of validators it can be more expensive if you only learn of large aggregates. For a large fraction, you would expect to have many offenders in the same signature, but I haven't thought about it in depth.

Also without a very low MAX_ATTESTATIONS config value, it increases the worst-case block size. However with = 8 the max size of the bitfields is 900000/32 / 8 * 8 = 28KB, which looks okay.

mkalinin · 2024-01-29T09:01:42Z

Is committee_bits truly required or can we simply include all committees?

I like the idea of having a single bitfield per slot. The problem about it is that without compression it would become inefficient especially for aggregating subnets where only 1/64th of validators are presented in a bitfield. If we can agree that for any existing use case this inefficiency won’t hurt because say data compression (e.g. snappy) is always used then a single bitfield would make more sense than a list of them.

mkalinin · 2024-01-29T09:25:07Z

How does this change affect the data complexity of AttersterSlashings? When slashing a small fraction of validators it can be more expensive if you only learn of large aggregates. For a large fraction, you would expect to have many offenders in the same signature, but I haven't thought about it in depth.

Good point! The worst case slashing scenario will have 64 times data complexity increase comparing to what we have today:

437Kb of uncompressed data vs 7Kb as of today when both attestations are taken from the on chain aggregates
222Kb vs 3.5Kb respectively, when one attestations is an on chain aggregate while another one is a network aggregate representing a single committee

So to alleviate the data complexity a slasher will have to keep attestations in a network format — supporting this does seem to not require any changes, and seems to be feasible for accidental equivocations due to node’s misconfiguration.

In the case of an attack with mass equivocations the data complexity doesn’t seem to increase in general, but with the proposed change it can result in a block size spikes vs gradual slashings inclusion as it would happen with an existing on chain format — this is where we can expect ~1Mb beacon blocks (with two slashings of a 437Kb size) — so we might consider reducing MAX_ATTESTER_SLASHINGS to 1.

Note that all the above numbers are calculated with 900k validator size and will increase within the growth of the validator set.

wemeetagain · 2024-02-08T15:46:38Z

Is committee_bits truly required or can we simply include all committees?

Do you mean

class Attestation(Container):
     aggregation_bits: Vector[Bitlist[MAX_VALIDATORS_PER_COMMITTEE], MAX_COMMITTEES_PER_SLOT]  # [Modified in EIP7549]
     data: AttestationData
     signature: BLSSignature

or

class Attestation(Container):
     aggregation_bits: Bitlist[MAX_VALIDATORS_PER_COMMITTEE * MAX_COMMITTEES_PER_SLOT]  # [Modified in EIP7549]
     data: AttestationData
     signature: BLSSignature

dapplion · 2024-02-19T07:04:50Z

@wemeetagain doesn't matter too much. Either way I don't think we should include bits for all committees in each attestation

dapplion · 2024-03-05T13:00:25Z

I have updated the spec to incorporate @mkalinin ideas allowing for efficient block packing of attestations. Rationale for adjusting the preset values can be found here https://hackmd.io/@n0ble/eip7549_onchain_aggregates

I have also updated the deprecation strategy of AttestationData.index to be set to 0.

Now the PR should be in good shape to be merged and iterate further in other PRs

mkalinin

Looks good to me 👍

specs/_features/eip7549/beacon-chain.md

specs/_features/eip7549/validator.md

specs/_features/eip7549/beacon-chain.md

hwwhww · 2024-03-06T16:48:12Z

Good job @dapplion @mkalinin!

I made it linter checkable. Note that it is not testable yet.

Given that this PR contains a phase0 refactoring, I kinda want to hold it before the official Deneb release (v.1.4.0) next week for cleanliness. What do you think?

/cc @djrtwo

specs/_features/eip7549/beacon-chain.md

dapplion · 2024-03-11T03:11:27Z

I kinda want to hold it before the official Deneb release (v.1.4.0) next week for cleanliness. What do you think?

Okay to me 👍

hwwhww

a rough approval here. will need to make it testable and add tests in other PRs.

Well done @dapplion @mkalinin and co!

arnetheduck · 2024-03-22T09:29:17Z

specs/_features/eip7549/beacon-chain.md

+
+```python
+class Attestation(Container):
+    aggregation_bits: List[Bitlist[MAX_VALIDATORS_PER_COMMITTEE], MAX_COMMITTEES_PER_SLOT]  # [Modified in EIP7549]


why the nested lists here? this is a deeply inefficient encoding which should be computable.

ie we should be able to flatten this to a single list which would save on merkle tree size / hashing time, data size, redundancy in encoding etc etc - important for lots of reasons given the hundreds of thousands of attestations we have to process

Having

aggregation_bits: Bitlist[MAX_VALIDATORS_PER_COMMITTEE * MAX_COMMITTEES_PER_SLOT]

will make the uncompressed size of Attestation outside of block (unaggregated + aggregated) very big. This List has exactly 1 element except when included in a blog after which it's likely to be fully populated. With your suggestion the Bitlist will be 99% zeros while on p2p topics. Snappy will eat those zeroes, but still.

SSZ serialization involves an extra offset for the List wrapper. SSZ merkleization will have ~8 extra depth which is indeed inefficient but not sure if it's that bad.

EDIT: Ok I'm onboard with the change 👍 slightly more complex parsing logic but overall a simplification

This begs the question: should we use a simpler wrapper for spreading attestations (vs the ones that end up in the block) on the single-attestation topics?

Something like class SingleAttestation(container): (attester_index: ValidatorIndex, data: AttestationData, sig: Signature)

As a developer I like such cleaner distinction but it has marginal benefit and does not justify adding a new container IMO

The optimized container only shaves a few bytes, won't have a meaningful impact on bandwidth nor CPU, you still have to verify a BLS sig

Well apart from bandwidth and compression time, it also shaves hashing for the message id - true, not the heaviest thing, but still appears when profiling when I look at it (specially on subscribe-all-subnets nodes).

We can "internally" reduce the memory footprint that an extra List indirection adds, for the purpose of building an attestation pool, but there are these observable aspects to it which prevent it from being done across the board.

I would not suggest this if we weren't changing Attestation already.

Also, notably, this would enforce the rule that the gossip topic should have only one signatory, simplifying the gossip validation rules.

fwiw, Attestation data structure in attestation subnets keeps open optimisation path where every attesting node publishes a single aggregate instead of multiple single attestations. Although, it does require either core protocol or network layer changes but switching to SingleAttestation would take a step back from this path

keeps open optimisation path where every attesting

this is a bridge to cross when it's time to cross it, I suspect - ie one reason we don't allow it already is because it's hard to guarantee disjoint attestation sets like that - the single attestation is fundamentally more simple to reason about and it already exists as a "concept" in the protocol, even if it doesn't have its own container.

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.

etan-status · 2024-08-28T12:40:44Z

specs/_features/eip7549/beacon-chain.md

+    data = attestation.data
+    assert data.target.epoch in (get_previous_epoch(state), get_current_epoch(state))
+    assert data.target.epoch == compute_epoch_at_slot(data.slot)
+    assert data.slot + MIN_ATTESTATION_INCLUSION_DELAY <= state.slot


Why is the max age removed here? (state.slot <= data.slot + SLOTS_PER_EPOCH)

https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/beacon-chain.md#attestations

assert data.slot + MIN_ATTESTATION_INCLUSION_DELAY <= state.slot <= data.slot + SLOTS_PER_EPOCH

It is initially removed in #3360

Oh, thanks for the pointer! Have missed the Deneb specs while reviewing.

move attestation index outside signed message

e8e00f3

dapplion changed the title ~~move attestation index outside signed message~~ EIP-7549: Move committee index outside Attestation Dec 5, 2023

hwwhww added the general:enhancement New feature or request label Dec 6, 2023

hwwhww mentioned this pull request Jan 25, 2024

Electra EIPs for discussion #3449

Closed

macladson mentioned this pull request Jan 26, 2024

EIP-7549: Move committee index outside Attestation sigp/lighthouse#5130

Closed

mkalinin mentioned this pull request Feb 5, 2024

Consensus-layer Call 127 ethereum/pm#951

Closed

This was referenced Feb 24, 2024

[EIP-7549] Move committee index outside attestation Consensys/teku#7965

Open

Update the attestations containers Consensys/teku#8026

Closed

dapplion added 2 commits March 5, 2024 17:19

Merge remote-tracking branch 'upstream/dev' into attestation-index

accee2b

Update spec with @mkalinin suggestions

38f269c

dapplion force-pushed the attestation-index branch from 60f10ab to 38f269c Compare March 5, 2024 12:58

mkalinin approved these changes Mar 5, 2024

View reviewed changes

hwwhww added the Electra label Mar 6, 2024

mkalinin mentioned this pull request Mar 6, 2024

Consensus-layer Call 129 ethereum/pm#973

Closed

mkalinin reviewed Mar 6, 2024

View reviewed changes

specs/_features/eip7549/beacon-chain.md Outdated Show resolved Hide resolved

specs/_features/eip7549/beacon-chain.md Outdated Show resolved Hide resolved

specs/_features/eip7549/validator.md Show resolved Hide resolved

specs/_features/eip7549/validator.md Show resolved Hide resolved

mkalinin reviewed Mar 6, 2024

View reviewed changes

specs/_features/eip7549/beacon-chain.md Outdated Show resolved Hide resolved

fix broken tests

dc37dcd

mehdi-aouadi reviewed Mar 6, 2024

View reviewed changes

specs/_features/eip7549/beacon-chain.md Outdated Show resolved Hide resolved

mehdi-aouadi reviewed Mar 6, 2024

View reviewed changes

specs/_features/eip7549/beacon-chain.md Outdated Show resolved Hide resolved

hwwhww added 2 commits March 6, 2024 23:24

make it executable for basic linter checks & fix errors

03c23c6

Fix ToC and apply @mehdi-aouadi's suggestion

43dbf8c

mehdi-aouadi reviewed Mar 7, 2024

View reviewed changes

specs/_features/eip7549/beacon-chain.md Show resolved Hide resolved

apply suggestions

5f78d2b

hwwhww approved these changes Mar 14, 2024

View reviewed changes

hwwhww merged commit 5d80b19 into ethereum:dev Mar 14, 2024
30 checks passed

mehdi-aouadi mentioned this pull request Mar 20, 2024

EIP-7549 Clarify network vs on chain aggregation #3628

Merged

arnetheduck reviewed Mar 22, 2024

View reviewed changes

mkalinin mentioned this pull request Mar 25, 2024

eip7549: flatten aggregate_bits #3631

Merged

arnetheduck mentioned this pull request Aug 26, 2024

Separate type for unaggregated network attestations #3900

Merged

etan-status reviewed Aug 28, 2024

View reviewed changes

dapplion deleted the attestation-index branch September 2, 2024 12:00

Scutua approved these changes Sep 16, 2024

View reviewed changes

EIP-7549: Move committee index outside Attestation #3559

EIP-7549: Move committee index outside Attestation #3559

Conversation

dapplion commented Dec 5, 2023 • edited Loading

Motivation

Deprecation strategy

hwwhww commented Dec 6, 2023 • edited Loading

dapplion commented Dec 6, 2023 • edited Loading

mkalinin commented Dec 6, 2023

dapplion commented Dec 6, 2023

arnetheduck commented Dec 6, 2023

dapplion commented Dec 6, 2023

rolfyone commented Dec 14, 2023

ppopth commented Dec 17, 2023

dapplion commented Dec 18, 2023

djrtwo commented Dec 19, 2023 • edited Loading

potuz commented Dec 19, 2023

arnetheduck commented Dec 19, 2023

djrtwo commented Jan 23, 2024

djrtwo commented Jan 23, 2024

dapplion commented Jan 24, 2024

djrtwo commented Jan 25, 2024 • edited Loading

mkalinin commented Jan 26, 2024

djrtwo commented Jan 26, 2024

dankrad commented Jan 26, 2024

dapplion commented Jan 28, 2024

mkalinin commented Jan 29, 2024

mkalinin commented Jan 29, 2024

wemeetagain commented Feb 8, 2024

dapplion commented Feb 19, 2024

dapplion commented Mar 5, 2024

mkalinin left a comment

Choose a reason for hiding this comment

hwwhww commented Mar 6, 2024

dapplion commented Mar 11, 2024

hwwhww left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dapplion Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

arnetheduck Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dapplion commented Dec 5, 2023 •

edited

Loading

hwwhww commented Dec 6, 2023 •

edited

Loading

dapplion commented Dec 6, 2023 •

edited

Loading

djrtwo commented Dec 19, 2023 •

edited

Loading

djrtwo commented Jan 25, 2024 •

edited

Loading

dapplion Mar 22, 2024 •

edited

Loading

arnetheduck Mar 25, 2024 •

edited

Loading