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

Separate type for aggregated committee attestation #4005

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

ensi321
Copy link
Contributor

@ensi321 ensi321 commented Nov 8, 2024

Description

Introducing separate container for aggregated attestation in a committee. The new container replaces the unnecessary committee bits with committee index. It also has 63x shorter max length than Attestation on aggregation_bits which yields max size going from 16.23kb to 0.48kb. This ensures aggregates with 63 committees worth of zeros in aggregation bits with not be deserialized.

The current Attestation represents aggregated attestation both on-chain and in a committee. Having a dedicated container for committee attestation improves readability and removes the need to maintain a one-bit-set committee bit list.

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.
The proposer should run the following function to construct an on chain final aggregate form a list of network aggregates with equal `AttestationData`:
Committee attestations received by the block proposer from the committee aggregators with different `committee_index` sets and 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 committee attestations with equal `AttestationData`:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The proposer should run the following function to construct an on chain final aggregate form a list of committee attestations with equal `AttestationData`:
The proposer should run the following function to construct an on-chain final aggregate from a list of committee attestations with equal `AttestationData`:

@@ -220,4 +221,30 @@ with updated field assignments:

- 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 bit set corresponding to `committee_index` in each individual attestation.
- Set `attestation.committee_index = committee_index`, where `committee_index` is the `committee_index` in each individual attestation.
Copy link
Member

Choose a reason for hiding this comment

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

Making it the same as in "Construct attestation" would be easier. Reordering this to be the second one as in "Construct attestation" would even be better.

Suggested change
- Set `attestation.committee_index = committee_index`, where `committee_index` is the `committee_index` in each individual attestation.
- Set `attestation.committee_index` to the index associated with the validator's committee.

Comment on lines +225 to +250

#### Aggregate signature

Set `aggregate_attestation.signature = aggregate_signature` where `aggregate_signature` is obtained from:

```python
def get_aggregate_signature(attestations: Sequence[CommitteeAttestation]) -> BLSSignature:
signatures = [attestation.signature for attestation in attestations]
return bls.Aggregate(signatures)
```

### Broadcast aggregate

`get_aggregate_and_proof` is modified to accept `CommitteeAttestation` for `aggregate`.

```python
def get_aggregate_and_proof(state: BeaconState,
aggregator_index: ValidatorIndex,
committee_attestation: CommitteeAttestation,
privkey: int) -> AggregateAndProof:
return AggregateAndProof(
aggregator_index=aggregator_index,
aggregate=committee_attestation,
selection_proof=get_slot_signature(state, aggregate.data.slot, privkey),
)
```
Copy link
Member

Choose a reason for hiding this comment

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

I think these two sections are trivial and wordy and unrelated to the change, right? I would say they are not needed.

Suggested change
#### Aggregate signature
Set `aggregate_attestation.signature = aggregate_signature` where `aggregate_signature` is obtained from:
```python
def get_aggregate_signature(attestations: Sequence[CommitteeAttestation]) -> BLSSignature:
signatures = [attestation.signature for attestation in attestations]
return bls.Aggregate(signatures)
```
### Broadcast aggregate
`get_aggregate_and_proof` is modified to accept `CommitteeAttestation` for `aggregate`.
```python
def get_aggregate_and_proof(state: BeaconState,
aggregator_index: ValidatorIndex,
committee_attestation: CommitteeAttestation,
privkey: int) -> AggregateAndProof:
return AggregateAndProof(
aggregator_index=aggregator_index,
aggregate=committee_attestation,
selection_proof=get_slot_signature(state, aggregate.data.slot, privkey),
)
```

@ppopth
Copy link
Member

ppopth commented Nov 27, 2024

Referencing #3900 (comment) and #3787 which have kind of similar ideas.

Copy link
Member

@ppopth ppopth left a comment

Choose a reason for hiding this comment

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

Looks good to me after all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants