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.

Sign up for GitHub

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

Jump to bottom

EIP-7549: replace committee_bits with committee_indices #3688

Closed
wants to merge 2 commits into from
Closed

EIP-7549: replace committee_bits with committee_indices #3688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
13b82c3
EIP-7549: replace committee_bits with committee_indices
mkalinin Apr 17, 2024
e8d4a13
Allow get_attesting_indices to return duplicates
mkalinin Apr 18, 2024
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
26 changes: 8 additions & 18 deletions specs/electra/beacon-chain.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@
- [Updated `is_fully_withdrawable_validator`](#updated-is_fully_withdrawable_validator)
- [Updated `is_partially_withdrawable_validator`](#updated-is_partially_withdrawable_validator)
- [Misc](#misc-1)
- [`get_committee_indices`](#get_committee_indices)
- [`get_validator_max_effective_balance`](#get_validator_max_effective_balance)
- [Beacon state accessors](#beacon-state-accessors)
- [New `get_balance_churn_limit`](#new-get_balance_churn_limit)
Expand Down Expand Up @@ -280,7 +279,7 @@ class AttesterSlashing(Container):
class Attestation(Container):
aggregation_bits: Bitlist[MAX_VALIDATORS_PER_COMMITTEE * MAX_COMMITTEES_PER_SLOT] # [Modified in Electra:EIP7549]
data: AttestationData
committee_bits: Bitvector[MAX_COMMITTEES_PER_SLOT] # [New in Electra:EIP7549]
committee_indices: List[CommitteeIndex, MAX_COMMITTEES_PER_SLOT] # [New in Electra:EIP7549]
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should enforce these committee indices to be sorted

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tbh, I don’t know, do we have any reason for enforcing them to be sorted?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing other than it being cheap to do and nice to display, but whoever displays it can also sort it out of band. I'm indifferent, but thought it's worth considering so I don't miss anything.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, it worth considering! but beyond potential UX improvement I don’t see any reason to enforce that. So by default i’d leave it as is unless there are other reasons for doing that

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like enforcing sorting makes things more clear and preserve equality: with non-sorted we may have Attestation1.hash_root != Attestation2.hash_root but they could have exact same coverage. (correct me if i'm wrong)
Having a unique way of expressing the same thing maybe isn't super important but seems a nice property to have.

Copy link
Contributor

Choose a reason for hiding this comment

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

as per offline discussion, worth noting that, as we introduce potential duplications in index list, then the the assumption same attestation -> same hash_root will be broken anyway

Copy link
Contributor

Choose a reason for hiding this comment

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

as we introduce potential duplications in index list

duplications would imply that signature is different already (since each entry represents a term being added to the aggregate sig), which was the motivation for this PR..

I'd be interested to see how much of an improvement this possibility actually provides, ie how often is it that when we're packing attestations, we would benefit from overlapping aggregates - this would imply that aggregators observed different attestations which should be rare.

Copy link
Contributor

@arnetheduck arnetheduck May 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

basically, if I casually look at the aggregates coming in from aggregators, apart from poor quality due to timing most likely, it's rare that there are multiple overlapping aggregates of equal quality.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we take a look at on chain data (i picked a few random blocks — not that representative tho), they have duplicates. Probably, those duplicates are added by the proposer from what it has observed in the attestation subnet itself as the duplicates are usually scarce.

The issue that we have discussed with @tbenr is that the same attestation can have different representations. Consider we have n aggregates from the same committee in one on chain aggregate, if we enforce the order of committee indices in the array then by changing the order of the aggregation bits we can get up to n! different on chain aggregates with distinct hash_tree_root() each that are actually carrying the same information.

Copy link
Contributor

Choose a reason for hiding this comment

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

changing the order of the aggregation bits

Sure, as long as they're permutations - I misunderstood it as entries with different number of duplicates being discussed.

I'm bringing it up because one alternative encoding is to list the validators that were not included - usually, there's only a few of those and this way we would probably have a significantly smaller list, but that again brings us back to the only-one-item territory - it's been discussed as an optimisation strategy as well, ie keep an aggregated committee key and subtract - probably makes more sense for sync committee messages but still.

signature: BLSSignature
```

Expand Down Expand Up @@ -504,13 +503,6 @@ def is_partially_withdrawable_validator(validator: Validator, balance: Gwei) ->

### Misc

#### `get_committee_indices`

```python
def get_committee_indices(commitee_bits: Bitvector) -> Sequence[CommitteeIndex]:
return [CommitteeIndex(index) for index, bit in enumerate(commitee_bits) if bit]
```

#### `get_validator_max_effective_balance`

```python
Expand Down Expand Up @@ -577,18 +569,17 @@ def get_pending_balance_to_withdraw(state: BeaconState, validator_index: Validat
#### Modified `get_attesting_indices`

```python
def get_attesting_indices(state: BeaconState, attestation: Attestation) -> Set[ValidatorIndex]:
def get_attesting_indices(state: BeaconState, attestation: Attestation) -> List[ValidatorIndex]:
"""
Return the set of attesting indices corresponding to ``aggregation_bits`` and ``committee_bits``.
Return the set of attesting indices corresponding to ``aggregation_bits`` and ``committee_indices``.
"""
output: Set[ValidatorIndex] = set()
committee_indices = get_committee_indices(attestation.committee_bits)
output: List[ValidatorIndex] = list()
committee_offset = 0
for index in committee_indices:
for index in attestation.committee_indices:
committee = get_beacon_committee(state, attestation.data.slot, index)
committee_attesters = set(
committee_attesters = list(
index for i, index in enumerate(committee) if attestation.aggregation_bits[committee_offset + i])
output = output.union(committee_attesters)
output += committee_attesters

committee_offset += len(committee)

Expand Down Expand Up @@ -1049,9 +1040,8 @@ def process_attestation(state: BeaconState, attestation: Attestation) -> None:

# [Modified in Electra:EIP7549]
assert data.index == 0
committee_indices = get_committee_indices(attestation.committee_bits)
participants_count = 0
for index in committee_indices:
for index in attestation.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)
Expand Down
8 changes: 4 additions & 4 deletions specs/electra/p2p-interface.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,17 +43,17 @@ The derivation of the `message-id` remains stable.
##### `beacon_aggregate_and_proof`

The following convenience variables are re-defined
- `index = get_committee_indices(aggregate.committee_bits)[0]`
- `index = aggregate.committee_indices[0]`

The following validations are added:
* [REJECT] `len(committee_indices) == 1`, where `committee_indices = get_committee_indices(aggregate)`.
* [REJECT] `len(attestation.committee_indices) == 1`
* [REJECT] `aggregate.data.index == 0`

##### `beacon_attestation_{subnet_id}`

The following convenience variables are re-defined
- `index = get_committee_indices(attestation.committee_bits)[0]`
- `index = attestation.committee_indices[0]`

The following validations are added:
* [REJECT] `len(committee_indices) == 1`, where `committee_indices = get_committee_indices(attestation)`.
* [REJECT] `len(attestation.committee_indices) == 1`
* [REJECT] `attestation.data.index == 0`
29 changes: 16 additions & 13 deletions specs/electra/validator.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,26 +47,29 @@ Changed the max attester slashings size to `MAX_ATTESTER_SLASHINGS_ELECTRA`.
Changed the max attestations size to `MAX_ATTESTATIONS_ELECTRA`.

The network attestation aggregates contain only the assigned committee attestations.
Attestation aggregates received by the block proposer from the committee aggregators with disjoint `committee_bits` sets and equal `AttestationData` SHOULD be consolidated into a single `Attestation` object.
Attestation aggregates received by the block proposer from the committee aggregators with equal `AttestationData` SHOULD be consolidated into a single `Attestation` object.
The proposer should run the following function to construct an on chain final aggregate form a list of network aggregates with equal `AttestationData`:

```python
def compute_on_chain_aggregate(network_aggregates: Sequence[Attestation]) -> Attestation:
aggregates = sorted(network_aggregates, key=lambda a: get_committee_indices(a.committee_bits)[0])
data = network_aggregates[0].data
for att in network_aggregates:
assert data == att.data

data = aggregates[0].data
aggregation_bits = Bitlist[MAX_VALIDATORS_PER_COMMITTEE * MAX_COMMITTEES_PER_SLOT]()
for a in aggregates:
for b in a.aggregation_bits:
for att in network_aggregates:
for b in att.aggregation_bits:
aggregation_bits.append(b)

signature = bls.Aggregate([a.signature for a in aggregates])
signature = bls.Aggregate([att.signature for att in network_aggregates])
committee_indices = [att.committee_indices[0] for att in network_aggregates]

committee_indices = [get_committee_indices(a.committee_bits)[0] for a in aggregates]
committee_flags = [(index in committee_indices) for index in range(0, MAX_COMMITTEES_PER_SLOT)]
committee_bits = Bitvector[MAX_COMMITTEES_PER_SLOT](committee_flags)

return Attestation(aggregation_bits, data, committee_bits, signature)
return Attestation(
aggregation_bits=aggregation_bits,
data=data,
committee_indices=committee_indices,
signature=signature,
)
```

#### Deposits
Expand Down Expand Up @@ -124,7 +127,7 @@ def prepare_execution_payload(state: BeaconState,

- Set `attestation_data.index = 0`.
- Let `attestation.aggregation_bits` be a `Bitlist[MAX_VALIDATORS_PER_COMMITTEE * MAX_COMMITTEES_PER_SLOT]` of length `len(committee)`, where the bit of the index of the validator in the `committee` is set to `0b1`.
- Let `attestation.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`.
- Let `attestation.committee_indices` be a `List[CommitteeIndex, MAX_COMMITTEES_PER_SLOT]` of length `1` with the index associated with the validator's committee.

*Note*: Calling `get_attesting_indices(state, attestation)` should return a list of length equal to 1, containing `validator_index`.

Expand All @@ -134,4 +137,4 @@ def prepare_execution_payload(state: BeaconState,

- Set `attestation_data.index = 0`.
- Let `aggregation_bits` be a `Bitlist[MAX_VALIDATORS_PER_COMMITTEE * MAX_COMMITTEES_PER_SLOT]` of length `len(committee)`, where each bit set from each individual attestation is set to `0b1`.
- Set `attestation.committee_bits = committee_bits`, where `committee_bits` has the same value as in each individual attestation.
- Set `attestation.committee_indices = committee_indices`, where `committee_indices` has the same value as in each individual attestation.