eip7549: Ensure non-zero bits for each committee bitfield comprising an aggregate#4002
Conversation
jtraglia
left a comment
jtraglia
left a comment
There was a problem hiding this comment.
Good idea, I agree we should check for this. LGTM, with one little nit.
|
Do we need to add another condition to here? consensus-specs/specs/electra/p2p-interface.md Lines 51 to 53 in 1b408e9 |
what if a whole committee is not voting? ie there are no attestations from that commiteee? |
The validator who does aggregation should include its own vote at the very least. So there should be at least one bit in aggregation bits. edit: I notice https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/validator.md#construct-aggregate says aggregator should collect all the votes (including its own) that come solely from gossip. But I believe aggregator should include its own vote even if it doesn't receive its own vote through gossip. |
Then the bit corresponding to this committee should be |
Note that a network aggregate only allows a single committee, so the following Phase0 check is enough: consensus-specs/specs/phase0/p2p-interface.md Lines 371 to 372 in 49b6840 |
specs/electra/beacon-chain.md
Outdated
| #### Modified `get_indexed_attestation` | ||
|
|
||
| *Note*: The function is modified to use the new `get_attesting_indices`. | ||
|
|
||
| ```python | ||
| def get_indexed_attestation(state: BeaconState, attestation: Attestation) -> IndexedAttestation: | ||
| """ | ||
| Return the indexed attestation corresponding to ``attestation``. | ||
| """ | ||
| # [Modified in Electra:EIP7549] | ||
| attesting_indices = get_attesting_indices(state, attestation) | ||
|
|
||
| return IndexedAttestation( | ||
| attesting_indices=sorted(attesting_indices), | ||
| data=attestation.data, | ||
| signature=attestation.signature, | ||
| ) | ||
| ``` | ||
|
|
There was a problem hiding this comment.
For pyspec, it is not needed since get_attesting_indices would be swapped automatically.
There was a problem hiding this comment.
This function is redefined to capture the Attestation type modification and get_attesting_indexes change. If we’re not used to re-introduce functions in such cases then I am happy to remove it
specs/electra/beacon-chain.md
Outdated
| participation_flag_indices = get_attestation_participation_flag_indices(state, data, state.slot - data.slot) | ||
|
|
||
| # Verify signature | ||
| # Verify signature [Modified in Electra:EIP7549] |
There was a problem hiding this comment.
Related to get_indexed_attestation modification
specs/electra/beacon-chain.md
Outdated
| epoch_participation = state.previous_epoch_participation | ||
|
|
||
| proposer_reward_numerator = 0 | ||
| # [Modified in Electra:EIP7549] |
There was a problem hiding this comment.
imo it's not required because it didn't change "literally" in the specs. Devs should address it when they see the get_attesting_indices change.
If we apply this comment, it's weird that there were many other lines in the previous forks that we didn't highlight like this.
| for committee_index in committee_indices: | ||
| assert committee_index < get_committee_count_per_slot(state, data.target.epoch) | ||
| committee = get_beacon_committee(state, data.slot, committee_index) | ||
| committee_attesters = set( |
There was a problem hiding this comment.
nit: not that important but this part is quite similar to the logic in get_attesting_indices(). I think we can extract common part to a separate util function.
There was a problem hiding this comment.
I was looking at it as well but it doesn’t seem there is a good way to decompose
5 participants
Currently, the spec allows an on chain aggregate to have only a single non-zero bit regardless of the number of committees. This allows to create an on chain aggregate with 63 committees worth of zeroes which successfully passes all validations (~4kb of uncompressed and wasteful data per aggregate).
This PR adds a validation that requires at least a single non-zero bit for each attesting committee bitfield comprising the aggregate.