Skip to content
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

Merged
merged 7 commits into from
Mar 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ tests/core/pyspec/eth2spec/capella/
tests/core/pyspec/eth2spec/deneb/
tests/core/pyspec/eth2spec/eip6110/
tests/core/pyspec/eth2spec/eip7002/
tests/core/pyspec/eth2spec/eip7549/
tests/core/pyspec/eth2spec/whisk/
tests/core/pyspec/eth2spec/eip7594/

Expand Down
1 change: 1 addition & 0 deletions pysetup/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
DENEB = 'deneb'
EIP6110 = 'eip6110'
EIP7002 = 'eip7002'
EIP7549 = 'eip7549'
WHISK = 'whisk'
EIP7594 = 'eip7594'

Expand Down
4 changes: 3 additions & 1 deletion pysetup/md_doc_paths.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,9 @@
CAPELLA,
DENEB,
EIP6110,
WHISK,
EIP7002,
EIP7549,
WHISK,
EIP7594,
)

Expand All @@ -20,6 +21,7 @@
CAPELLA: BELLATRIX,
DENEB: CAPELLA,
EIP6110: DENEB,
EIP7549: DENEB,
WHISK: CAPELLA,
EIP7002: CAPELLA,
EIP7594: DENEB,
Expand Down
3 changes: 2 additions & 1 deletion pysetup/spec_builders/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from .deneb import DenebSpecBuilder
from .eip6110 import EIP6110SpecBuilder
from .eip7002 import EIP7002SpecBuilder
from .eip7549 import EIP7549SpecBuilder
from .whisk import WhiskSpecBuilder
from .eip7594 import EIP7594SpecBuilder

Expand All @@ -13,6 +14,6 @@
builder.fork: builder
for builder in (
Phase0SpecBuilder, AltairSpecBuilder, BellatrixSpecBuilder, CapellaSpecBuilder, DenebSpecBuilder,
EIP6110SpecBuilder, EIP7002SpecBuilder, WhiskSpecBuilder, EIP7594SpecBuilder,
EIP6110SpecBuilder, EIP7002SpecBuilder, EIP7549SpecBuilder, WhiskSpecBuilder, EIP7594SpecBuilder,
)
}
11 changes: 11 additions & 0 deletions pysetup/spec_builders/eip7549.py
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'''
'''
4 changes: 2 additions & 2 deletions pysetup/spec_builders/phase0.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,8 @@ def wrapper(*args, **kw): # type: ignore

_get_attesting_indices = get_attesting_indices
get_attesting_indices = cache_this(
lambda state, data, bits: (
lambda state, attestation: (
state.randao_mixes.hash_tree_root(),
state.validators.hash_tree_root(), data.hash_tree_root(), bits.hash_tree_root()
state.validators.hash_tree_root(), attestation.hash_tree_root()
),
_get_attesting_indices, lru_size=SLOTS_PER_EPOCH * MAX_COMMITTEES_PER_SLOT * 3)'''
4 changes: 2 additions & 2 deletions specs/_features/custody_game/beacon-chain.md
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,7 @@ def process_chunk_challenge(state: BeaconState, challenge: CustodyChunkChallenge
# Verify responder is slashable
assert is_slashable_validator(responder, get_current_epoch(state))
# Verify the responder participated in the attestation
attesters = get_attesting_indices(state, challenge.attestation.data, challenge.attestation.aggregation_bits)
attesters = get_attesting_indices(state, challenge)
assert challenge.responder_index in attesters
# Verify shard transition is correctly given
assert hash_tree_root(challenge.shard_transition) == challenge.attestation.data.shard_transition_root
Expand Down Expand Up @@ -594,7 +594,7 @@ def process_custody_slashing(state: BeaconState, signed_custody_slashing: Signed
assert len(custody_slashing.data) == shard_transition.shard_block_lengths[custody_slashing.data_index]
assert hash_tree_root(custody_slashing.data) == shard_transition.shard_data_roots[custody_slashing.data_index]
# Verify existence and participation of claimed malefactor
attesters = get_attesting_indices(state, attestation.data, attestation.aggregation_bits)
attesters = get_attesting_indices(state, attestation)
assert custody_slashing.malefactor_index in attesters

# Verify the malefactor custody key
Expand Down
138 changes: 138 additions & 0 deletions specs/_features/eip7549/beacon-chain.md
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]
Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Collaborator Author

@dapplion dapplion Mar 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@arnetheduck arnetheduck Mar 25, 2024

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)

Copy link
Collaborator Author

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

Copy link
Collaborator Author

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

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Collaborator

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 to SingleAttestation would take a step back from this path

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is initially removed in #3360

Copy link
Contributor

Choose a reason for hiding this comment

The 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)
```
54 changes: 54 additions & 0 deletions specs/_features/eip7549/p2p-interface.md
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`

51 changes: 51 additions & 0 deletions specs/_features/eip7549/validator.md
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

2 changes: 1 addition & 1 deletion specs/altair/beacon-chain.md
Original file line number Diff line number Diff line change
Expand Up @@ -496,7 +496,7 @@ def process_attestation(state: BeaconState, attestation: Attestation) -> None:
epoch_participation = state.previous_epoch_participation

proposer_reward_numerator = 0
for index in get_attesting_indices(state, data, attestation.aggregation_bits):
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)
Expand Down
2 changes: 1 addition & 1 deletion specs/altair/fork.md
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ def translate_participation(state: BeaconState, pending_attestations: Sequence[p

# Apply flags to all attesting validators
epoch_participation = state.previous_epoch_participation
for index in get_attesting_indices(state, data, attestation.aggregation_bits):
for index in get_attesting_indices(state, attestation):
for flag_index in participation_flag_indices:
epoch_participation[index] = add_flag(epoch_participation[index], flag_index)

Expand Down
2 changes: 1 addition & 1 deletion specs/deneb/beacon-chain.md
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ def process_attestation(state: BeaconState, attestation: Attestation) -> None:
epoch_participation = state.previous_epoch_participation

proposer_reward_numerator = 0
for index in get_attesting_indices(state, data, attestation.aggregation_bits):
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)
Expand Down
Loading
Loading