-
Notifications
You must be signed in to change notification settings - Fork 999
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
EIP-7549: Move committee index outside Attestation #3559
Changes from all commits
e8e00f3
accee2b
38f269c
dc37dcd
03c23c6
43dbf8c
5f78d2b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
from .base import BaseSpecBuilder | ||
from ..constants import EIP7549 | ||
|
||
|
||
class EIP7549SpecBuilder(BaseSpecBuilder): | ||
fork: str = EIP7549 | ||
|
||
@classmethod | ||
def imports(cls, preset_name: str): | ||
return super().imports(preset_name) + f''' | ||
''' |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,138 @@ | ||
# EIP-7549 -- The Beacon Chain | ||
|
||
## Table of contents | ||
|
||
<!-- TOC --> | ||
<!-- START doctoc generated TOC please keep comment here to allow auto update --> | ||
<!-- DON'T EDIT THIS SECTION, INSTEAD RE-RUN doctoc TO UPDATE --> | ||
|
||
- [Introduction](#introduction) | ||
- [Preset](#preset) | ||
- [Containers](#containers) | ||
- [Modified containers](#modified-containers) | ||
- [`Attestation`](#attestation) | ||
- [`IndexedAttestation`](#indexedattestation) | ||
- [Helper functions](#helper-functions) | ||
- [Misc](#misc) | ||
- [`get_committee_indices`](#get_committee_indices) | ||
- [Beacon state accessors](#beacon-state-accessors) | ||
- [Modified `get_attesting_indices`](#modified-get_attesting_indices) | ||
- [Block processing](#block-processing) | ||
- [Modified `process_attestation`](#modified-process_attestation) | ||
|
||
<!-- END doctoc generated TOC please keep comment here to allow auto update --> | ||
<!-- /TOC --> | ||
|
||
## Introduction | ||
|
||
This is the beacon chain specification to move the attestation committee index outside of the signed message. For motivation, refer to [EIP-7549](https://eips.ethereum.org/EIPS/eip-7549). | ||
|
||
*Note:* This specification is built upon [Deneb](../../deneb/beacon_chain.md) and is under active development. | ||
|
||
## Preset | ||
|
||
| Name | Value | Description | | ||
| - | - | - | | ||
| `MAX_ATTESTER_SLASHINGS` | `2**0` (= 1) | | ||
| `MAX_ATTESTATIONS` | `2**3` (= 8) | | ||
|
||
## Containers | ||
|
||
### Modified containers | ||
|
||
#### `Attestation` | ||
|
||
```python | ||
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 | ||
``` | ||
|
||
#### `IndexedAttestation` | ||
|
||
```python | ||
class IndexedAttestation(Container): | ||
# [Modified in EIP7549] | ||
attesting_indices: List[ValidatorIndex, MAX_VALIDATORS_PER_COMMITTEE * MAX_COMMITTEES_PER_SLOT] | ||
data: AttestationData | ||
signature: BLSSignature | ||
``` | ||
|
||
## Helper functions | ||
|
||
### Misc | ||
|
||
#### `get_committee_indices` | ||
|
||
```python | ||
def get_committee_indices(commitee_bits: Bitvector) -> List[CommitteeIndex]: | ||
return [CommitteeIndex(index) for bit, index in enumerate(commitee_bits) if bit] | ||
dapplion marked this conversation as resolved.
Show resolved
Hide resolved
|
||
``` | ||
|
||
### Beacon state accessors | ||
|
||
#### Modified `get_attesting_indices` | ||
|
||
```python | ||
def get_attesting_indices(state: BeaconState, attestation: Attestation) -> Set[ValidatorIndex]: | ||
""" | ||
Return the set of attesting indices corresponding to ``aggregation_bits`` and ``committee_bits``. | ||
""" | ||
|
||
output = set() | ||
committee_indices = get_committee_indices(attestation.committee_bits) | ||
for index in committee_indices: | ||
attesting_bits = attestation.aggregation_bits[index] | ||
committee = get_beacon_committee(state, attestation.data.slot, index) | ||
committee_attesters = set(index for i, index in enumerate(committee) if attesting_bits[i]) | ||
output = output.union(committee_attesters) | ||
|
||
return output | ||
``` | ||
|
||
### Block processing | ||
|
||
#### Modified `process_attestation` | ||
|
||
```python | ||
def process_attestation(state: BeaconState, attestation: Attestation) -> None: | ||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is the max age removed here? ( assert data.slot + MIN_ATTESTATION_INCLUSION_DELAY <= state.slot <= data.slot + SLOTS_PER_EPOCH There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is initially removed in #3360 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, thanks for the pointer! Have missed the Deneb specs while reviewing. |
||
|
||
# [Modified in EIP7549] | ||
assert data.index == 0 | ||
committee_indices = get_committee_indices(attestation.committee_bits) | ||
assert len(committee_indices) == len(attestation.aggregation_bits) | ||
for index in committee_indices: | ||
assert 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) | ||
|
||
# Participation flag indices | ||
participation_flag_indices = get_attestation_participation_flag_indices(state, data, state.slot - data.slot) | ||
|
||
# Verify signature | ||
assert is_valid_indexed_attestation(state, get_indexed_attestation(state, attestation)) | ||
|
||
# Update epoch participation flags | ||
if data.target.epoch == get_current_epoch(state): | ||
epoch_participation = state.current_epoch_participation | ||
else: | ||
epoch_participation = state.previous_epoch_participation | ||
|
||
proposer_reward_numerator = 0 | ||
for index in get_attesting_indices(state, attestation): | ||
for flag_index, weight in enumerate(PARTICIPATION_FLAG_WEIGHTS): | ||
if flag_index in participation_flag_indices and not has_flag(epoch_participation[index], flag_index): | ||
epoch_participation[index] = add_flag(epoch_participation[index], flag_index) | ||
proposer_reward_numerator += get_base_reward(state, index) * weight | ||
|
||
# Reward proposer | ||
proposer_reward_denominator = (WEIGHT_DENOMINATOR - PROPOSER_WEIGHT) * WEIGHT_DENOMINATOR // PROPOSER_WEIGHT | ||
proposer_reward = Gwei(proposer_reward_numerator // proposer_reward_denominator) | ||
increase_balance(state, get_beacon_proposer_index(state), proposer_reward) | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
# EIP-7549 -- Networking | ||
|
||
This document contains the consensus-layer networking specification for EIP-7549. | ||
|
||
The specification of these changes continues in the same format as the network specifications of previous upgrades, and assumes them as pre-requisite. | ||
|
||
## Table of contents | ||
|
||
<!-- TOC --> | ||
<!-- START doctoc generated TOC please keep comment here to allow auto update --> | ||
<!-- DON'T EDIT THIS SECTION, INSTEAD RE-RUN doctoc TO UPDATE --> | ||
|
||
- [Modifications in EIP-7549](#modifications-in-eip-7549) | ||
- [The gossip domain: gossipsub](#the-gossip-domain-gossipsub) | ||
- [Topics and messages](#topics-and-messages) | ||
- [Global topics](#global-topics) | ||
- [`beacon_aggregate_and_proof`](#beacon_aggregate_and_proof) | ||
- [`beacon_attestation_{subnet_id}`](#beacon_attestation_subnet_id) | ||
|
||
<!-- END doctoc generated TOC please keep comment here to allow auto update --> | ||
<!-- /TOC --> | ||
|
||
## Modifications in EIP-7549 | ||
|
||
### The gossip domain: gossipsub | ||
|
||
#### Topics and messages | ||
|
||
The `beacon_aggregate_and_proof` and `beacon_attestation_{subnet_id}` topics are modified to support the gossip of a new attestation type. | ||
|
||
##### Global topics | ||
|
||
###### `beacon_aggregate_and_proof` | ||
|
||
*[Modified in EIP7549]* | ||
|
||
The following convenience variables are re-defined | ||
- `index = get_committee_indices(aggregate.committee_bits)[0]` | ||
- `aggregation_bits = aggregate.aggregation_bits[0]` | ||
|
||
The following validations are added: | ||
* [REJECT] `len(committee_indices) == len(aggregate.attestation_bits) == 1`, where `committee_indices = get_committee_indices(aggregate)`. | ||
* [REJECT] `aggregate.data.index == 0` | ||
|
||
###### `beacon_attestation_{subnet_id}` | ||
|
||
The following convenience variables are re-defined | ||
- `index = get_committee_indices(attestation.committee_bits)[0]` | ||
- `aggregation_bits = attestation.aggregation_bits[0]` | ||
|
||
The following validations are added: | ||
* [REJECT] `len(committee_indices) == len(attestation.attestation_bits) == 1`, where `committee_indices = get_committee_indices(attestation)`. | ||
* [REJECT] `attestation.data.index == 0` | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
# Deneb -- Honest Validator | ||
|
||
## Table of contents | ||
|
||
<!-- TOC --> | ||
<!-- START doctoc generated TOC please keep comment here to allow auto update --> | ||
<!-- DON'T EDIT THIS SECTION, INSTEAD RE-RUN doctoc TO UPDATE --> | ||
|
||
- [Modifications in EIP-7549](#modifications-in-eip-7549) | ||
- [Block proposal](#block-proposal) | ||
- [Constructing the `BeaconBlockBody`](#constructing-the-beaconblockbody) | ||
- [Attestations](#attestations) | ||
- [Attesting](#attesting) | ||
- [Construct attestation](#construct-attestation) | ||
- [Attestation aggregation](#attestation-aggregation) | ||
- [Construct aggregate](#construct-aggregate) | ||
|
||
<!-- END doctoc generated TOC please keep comment here to allow auto update --> | ||
<!-- /TOC --> | ||
|
||
## Modifications in EIP-7549 | ||
|
||
### Block proposal | ||
|
||
#### Constructing the `BeaconBlockBody` | ||
|
||
##### Attestations | ||
|
||
Attestations received from aggregators with disjoint `committee_bits` sets and equal `AttestationData` SHOULD be consolidated into a single `Attestation` object. | ||
|
||
### Attesting | ||
|
||
#### Construct attestation | ||
|
||
- Set `attestation_data.index = 0`. | ||
- Let `aggregation_bits` be a `Bitlist[MAX_VALIDATORS_PER_COMMITTEE]` of length `len(committee)`, where the bit of the index of the validator in the `committee` is set to `0b1`. | ||
- Set `attestation.aggregation_bits = [aggregation_bits]`, a list of length 1 | ||
dapplion marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- Let `committee_bits` be a `Bitvector[MAX_COMMITTEES_PER_SLOT]`, where the bit at the index associated with the validator's committee is set to `0b1` | ||
- Set `attestation.committee_bits = committee_bits` | ||
|
||
*Note*: Calling `get_attesting_indices(state, attestation)` should return a list of length equal to 1, containing `validator_index`. | ||
|
||
### Attestation aggregation | ||
|
||
#### Construct aggregate | ||
|
||
- Set `attestation_data.index = 0`. | ||
- Let `aggregation_bits` be a `Bitlist[MAX_VALIDATORS_PER_COMMITTEE]` of length `len(committee)`, where each bit set from each individual attestation is set to `0b1`. | ||
- Set `attestation.aggregation_bits = [aggregation_bits]`, a list of length 1 | ||
dapplion marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- Set `attestation.committee_bits = committee_bits`, where `committee_bits` has the same value as in each individual attestation | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the nested lists here? this is a deeply inefficient encoding which should be computable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a developer I like such cleaner distinction but it has marginal benefit and does not justify adding a new container IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, notably, this would enforce the rule that the gossip topic should have only one signatory, simplifying the gossip validation rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 toSingleAttestation
would take a step back from this pathThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.