From 3ad76ff6dbbb85c93c4ffb3af7fb153baae3926f Mon Sep 17 00:00:00 2001 From: tersec Date: Tue, 1 Apr 2025 04:20:27 +0000 Subject: [PATCH 1/2] fix checking for existing covering Electra aggregates in attestation pool --- .../attestation_pool.nim | 26 +++++++++---------- .../gossip_processing/gossip_validation.nim | 8 ++++++ tests/test_attestation_pool.nim | 10 ++++++- 3 files changed, 30 insertions(+), 14 deletions(-) diff --git a/beacon_chain/consensus_object_pools/attestation_pool.nim b/beacon_chain/consensus_object_pools/attestation_pool.nim index 298147ac6e..14248b6025 100644 --- a/beacon_chain/consensus_object_pools/attestation_pool.nim +++ b/beacon_chain/consensus_object_pools/attestation_pool.nim @@ -507,7 +507,8 @@ proc addAttestation*( template addAttToPool(_: electra.Attestation) {.used.} = let - committee_index = get_committee_index_one(attestation.committee_bits).expect("TODO") + committee_index = get_committee_index_one( + attestation.committee_bits).expect("Gossip validation requires this") data = AttestationData( slot: attestation.data.slot, index: uint64 committee_index, @@ -557,11 +558,10 @@ func covers*( ## the existing aggregates, making it redundant ## the `var` attestation pool is needed to use `withValue`, else Table becomes ## unusably inefficient - let candidateIdx = pool.candidateIdx(data.slot, CandidateIdxType.phase0Idx) - if candidateIdx.isNone: + let candidateIdx = pool.candidateIdx(data.slot, CandidateIdxType.phase0Idx).valueOr: return false - pool.phase0Candidates[candidateIdx.get()].withValue( + pool.phase0Candidates[candidateIdx].withValue( getAttestationCandidateKey(data, Opt.none CommitteeIndex), entry): if entry[].covers(bits): return true @@ -570,21 +570,21 @@ func covers*( func covers*( pool: var AttestationPool, data: AttestationData, - bits: ElectraCommitteeValidatorsBits): bool = + aggregation_bits: ElectraCommitteeValidatorsBits, + committee_bits: AttestationCommitteeBits): bool = ## Return true iff the given attestation already is fully covered by one of ## the existing aggregates, making it redundant ## the `var` attestation pool is needed to use `withValue`, else Table becomes ## unusably inefficient - let candidateIdx = pool.candidateIdx(data.slot, CandidateIdxType.electraIdx) - if candidateIdx.isNone: + let candidateIdx = pool.candidateIdx(data.slot, CandidateIdxType.electraIdx).valueOr: return false - debugComment "foo" - # needs to know more than attestationdata now - #let attestation_data_root = hash_tree_root(data) - #pool.electraCandidates[candidateIdx.get()].withValue(attestation_data_root, entry): - # if entry[].covers(bits): - # return true + pool.electraCandidates[candidateIdx].withValue( + getAttestationCandidateKey( + data, Opt.some get_committee_index_one( + committee_bits).expect("Gossip validation requires this")), entry): + if entry[].covers(aggregation_bits): + return true false diff --git a/beacon_chain/gossip_processing/gossip_validation.nim b/beacon_chain/gossip_processing/gossip_validation.nim index 056309c459..6d7b9ae6fa 100644 --- a/beacon_chain/gossip_processing/gossip_validation.nim +++ b/beacon_chain/gossip_processing/gossip_validation.nim @@ -1193,6 +1193,7 @@ proc validateAttestation*( # https://github.com/ethereum/consensus-specs/blob/v1.4.0-beta.1/specs/phase0/p2p-interface.md#beacon_aggregate_and_proof # https://github.com/ethereum/consensus-specs/blob/v1.4.0-beta.5/specs/deneb/p2p-interface.md#beacon_aggregate_and_proof +# https://github.com/ethereum/consensus-specs/blob/v1.5.0-beta.4/specs/electra/p2p-interface.md#beacon_aggregate_and_proof proc validateAggregate*( pool: ref AttestationPool, batchCrypto: ref BatchCrypto, signedAggregateAndProof: @@ -1217,6 +1218,11 @@ proc validateAggregate*( return pool.checkedReject(v.error) v.get() + # [REJECT] aggregate.data.index == 0 + when signedAggregateAndProof is electra.SignedAggregateAndProof: + if not(aggregate.data.index == 0): + return pool.checkedReject("Aggregate: Electra aggregate.data.index != 0") + # [IGNORE] aggregate.data.slot is within the last # ATTESTATION_PROPAGATION_SLOT_RANGE slots (with a # MAXIMUM_GOSSIP_CLOCK_DISPARITY allowance) -- i.e. aggregate.data.slot + @@ -1282,6 +1288,8 @@ proc validateAggregate*( # data.index < get_committee_count_per_slot(state, data.target.epoch). let committee_index = block: when signedAggregateAndProof is electra.SignedAggregateAndProof: + # [REJECT] len(committee_indices) == 1, where committee_indices = + # get_committee_indices(aggregate) let agg_idx = get_committee_index_one(aggregate.committee_bits).valueOr: return pool.checkedReject("Aggregate: got multiple committee bits") let idx = shufflingRef.get_committee_index(agg_idx.uint64) diff --git a/tests/test_attestation_pool.nim b/tests/test_attestation_pool.nim index d6291e2d33..9422e84ab1 100644 --- a/tests/test_attestation_pool.nim +++ b/tests/test_attestation_pool.nim @@ -323,6 +323,7 @@ suite "Attestation pool processing" & preset(): check: not pool[].covers(att0.data, att0.aggregation_bits) + not pool[].covers(att1.data, att1.aggregation_bits) pool[].addAttestation( att0, @[bc0[0], bc0[2]], att0.aggregation_bits.len, att0.loadSig, @@ -339,6 +340,7 @@ suite "Attestation pool processing" & preset(): check: pool[].covers(att0.data, att0.aggregation_bits) + pool[].covers(att1.data, att1.aggregation_bits) pool[].getAttestationsForBlock(state[], cache).len() == 2 # Can get either aggregate here, random! pool[].getPhase0AggregatedAttestation(1.Slot, 0.CommitteeIndex).isSome() @@ -1035,7 +1037,9 @@ suite "Attestation pool electra processing" & preset(): check: verifyAttestationSignature(att0) verifyAttestationSignature(att1) - not pool[].covers(att0.data, att0.aggregation_bits) + not pool[].covers(att0.data, att0.aggregation_bits, att0.committee_bits) + not pool[].covers(att1.data, att1.aggregation_bits, att1.committee_bits) + not pool[].covers(att2.data, att2.aggregation_bits, att2.committee_bits) pool[].addAttestation( att0, @[bc0[0], bc0[2]], att0.aggregation_bits.len, att0.loadSig, @@ -1048,6 +1052,10 @@ suite "Attestation pool electra processing" & preset(): check: verifyAttestationSignature(att) check: + pool[].covers(att0.data, att0.aggregation_bits, att0.committee_bits) + pool[].covers(att1.data, att1.aggregation_bits, att1.committee_bits) + pool[].covers(att2.data, att2.aggregation_bits, att2.committee_bits) + process_slots( defaultRuntimeConfig, state[], getStateField(state[], slot) + MIN_ATTESTATION_INCLUSION_DELAY, cache, From 53a7ad1051d80ce77d35ce339c8d8ac3df4ad57d Mon Sep 17 00:00:00 2001 From: tersec Date: Tue, 1 Apr 2025 07:53:35 +0000 Subject: [PATCH 2/2] fix gossip validation usage of Electra covers --- .../gossip_processing/gossip_validation.nim | 24 ++++++++++++------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/beacon_chain/gossip_processing/gossip_validation.nim b/beacon_chain/gossip_processing/gossip_validation.nim index 6d7b9ae6fa..8b4a33d622 100644 --- a/beacon_chain/gossip_processing/gossip_validation.nim +++ b/beacon_chain/gossip_processing/gossip_validation.nim @@ -1287,13 +1287,13 @@ proc validateAggregate*( # [REJECT] The committee index is within the expected range -- i.e. # data.index < get_committee_count_per_slot(state, data.target.epoch). let committee_index = block: - when signedAggregateAndProof is electra.SignedAggregateAndProof: + when kind(typeof(signedAggregateAndProof)) == ConsensusFork.Electra: # [REJECT] len(committee_indices) == 1, where committee_indices = # get_committee_indices(aggregate) let agg_idx = get_committee_index_one(aggregate.committee_bits).valueOr: return pool.checkedReject("Aggregate: got multiple committee bits") let idx = shufflingRef.get_committee_index(agg_idx.uint64) - elif signedAggregateAndProof is phase0.SignedAggregateAndProof: + elif kind(typeof(signedAggregateAndProof)) == ConsensusFork.Phase0: let idx = shufflingRef.get_committee_index(aggregate.data.index) else: static: doAssert false @@ -1306,13 +1306,19 @@ proc validateAggregate*( 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") + # [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 + when kind(typeof(signedAggregateAndProof)) == ConsensusFork.Electra: + if checkCover and + pool[].covers(aggregate.data, aggregate.aggregation_bits, + aggregate.committee_bits): + return errIgnore("Aggregate: already covered") + else: + if checkCover and + pool[].covers(aggregate.data, aggregate.aggregation_bits): + 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,