Skip to content

fix checking for existing covering Electra aggregates in attestation pool #7043

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.

Sign up for GitHub

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

Merged
tersec merged 2 commits into from
Apr 1, 2025
Merged

fix checking for existing covering Electra aggregates in attestation pool #7043

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3ad76ff
fix checking for existing covering Electra aggregates in attestation …
tersec Apr 1, 2025
53a7ad1
fix gossip validation usage of Electra covers
tersec Apr 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 13 additions & 13 deletions beacon_chain/consensus_object_pools/attestation_pool.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand All @@ -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

Expand Down
32 changes: 23 additions & 9 deletions beacon_chain/gossip_processing/gossip_validation.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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 +
Expand Down Expand Up @@ -1281,11 +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
Expand All @@ -1298,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,
Expand Down
10 changes: 9 additions & 1 deletion tests/test_attestation_pool.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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()
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down
Loading