From 375f276f317d1b478f46cfbcefd7db5eec8e906d Mon Sep 17 00:00:00 2001 From: Etan Kissling Date: Mon, 20 Nov 2023 06:42:29 -0800 Subject: [PATCH] reorder gossip validation checks (#5610) * 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 --- .../consensus_object_pools/spec_cache.nim | 9 ++++++- .../gossip_processing/gossip_validation.nim | 24 +++++++++++-------- 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/beacon_chain/consensus_object_pools/spec_cache.nim b/beacon_chain/consensus_object_pools/spec_cache.nim index 44420dff5c..6e8292cdb1 100644 --- a/beacon_chain/consensus_object_pools/spec_cache.nim +++ b/beacon_chain/consensus_object_pools/spec_cache.nim @@ -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( diff --git a/beacon_chain/gossip_processing/gossip_validation.nim b/beacon_chain/gossip_processing/gossip_validation.nim index c2ed5e9488..0d55c25aac 100644 --- a/beacon_chain/gossip_processing/gossip_validation.nim +++ b/beacon_chain/gossip_processing/gossip_validation.nim @@ -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") @@ -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: @@ -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,