Skip to content

Commit

Permalink
reorder gossip validation checks (#5610)
Browse files Browse the repository at this point in the history
* reorder gossip validation checks

Doing the coverage check only after the corresponding committee index is
known allows optimization by early rejecting invalid data.

* use same helper for individual attestations as well
  • Loading branch information
etan-status authored Nov 20, 2023
1 parent e1e809e commit 375f276
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 11 deletions.
9 changes: 8 additions & 1 deletion beacon_chain/consensus_object_pools/spec_cache.nim
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,19 @@ func get_beacon_committee_len*(
)

# https://github.com/ethereum/consensus-specs/blob/v1.4.0-beta.4/specs/phase0/beacon-chain.md#get_attesting_indices
func compatible_with_shuffling*(
bits: CommitteeValidatorsBits,
shufflingRef: ShufflingRef,
slot: Slot,
committee_index: CommitteeIndex): bool =
bits.lenu64 == get_beacon_committee_len(shufflingRef, slot, committee_index)

iterator get_attesting_indices*(shufflingRef: ShufflingRef,
slot: Slot,
committee_index: CommitteeIndex,
bits: CommitteeValidatorsBits):
ValidatorIndex =
if bits.lenu64 != get_beacon_committee_len(shufflingRef, slot, committee_index):
if not bits.compatible_with_shuffling(shufflingRef, slot, committee_index):
trace "get_attesting_indices: inconsistent aggregation and committee length"
else:
for index_in_committee, validator_index in get_beacon_committee(
Expand Down
24 changes: 14 additions & 10 deletions beacon_chain/gossip_processing/gossip_validation.nim
Original file line number Diff line number Diff line change
Expand Up @@ -724,8 +724,8 @@ proc validateAttestation*(
# This uses the same epochRef as data.target.epoch, because the attestation's
# epoch matches its target and attestation.data.target.root is an ancestor of
# attestation.data.beacon_block_root.
if not (attestation.aggregation_bits.lenu64 == get_beacon_committee_len(
shufflingRef, attestation.data.slot, committee_index)):
if not attestation.aggregation_bits.compatible_with_shuffling(
shufflingRef, slot, committee_index):
return pool.checkedReject(
"Attestation: number of aggregation bits and committee size mismatch")

Expand Down Expand Up @@ -872,14 +872,6 @@ proc validateAggregate*(
return pool.checkedResult(v.error)
v.get()

if checkCover and
pool[].covers(aggregate.data, aggregate.aggregation_bits):
# [IGNORE] A valid aggregate attestation defined by
# `hash_tree_root(aggregate.data)` whose `aggregation_bits` is a non-strict
# superset has _not_ already been seen.
# https://github.com/ethereum/consensus-specs/pull/2847
return errIgnore("Aggregate already covered")

let
shufflingRef =
pool.dag.getShufflingRef(target.blck, target.slot.epoch, false).valueOr:
Expand All @@ -896,6 +888,18 @@ proc validateAggregate*(
return pool.checkedReject(
"Attestation: committee index not within expected range")
idx.get()
if not aggregate.aggregation_bits.compatible_with_shuffling(
shufflingRef, slot, committee_index):
return pool.checkedReject(
"Aggregate: number of aggregation bits and committee size mismatch")

if checkCover and
pool[].covers(aggregate.data, aggregate.aggregation_bits):
# [IGNORE] A valid aggregate attestation defined by
# `hash_tree_root(aggregate.data)` whose `aggregation_bits` is a non-strict
# superset has _not_ already been seen.
# https://github.com/ethereum/consensus-specs/pull/2847
return errIgnore("Aggregate already covered")

# [REJECT] aggregate_and_proof.selection_proof selects the validator as an
# aggregator for the slot -- i.e. is_aggregator(state, aggregate.data.slot,
Expand Down

0 comments on commit 375f276

Please sign in to comment.