diff --git a/beacon_chain/attestation_aggregation.nim b/beacon_chain/attestation_aggregation.nim index 3a654be77..a980ed0d9 100644 --- a/beacon_chain/attestation_aggregation.nim +++ b/beacon_chain/attestation_aggregation.nim @@ -28,21 +28,15 @@ func is_aggregator(state: BeaconState, slot: Slot, index: CommitteeIndex, proc aggregate_attestations*( pool: AttestationPool, state: BeaconState, index: CommitteeIndex, - privkey: ValidatorPrivKey, trailing_distance: uint64, - cache: var StateCache): Option[AggregateAndProof] = - doAssert state.slot >= trailing_distance - - # https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/p2p-interface.md#configuration - doAssert trailing_distance <= ATTESTATION_PROPAGATION_SLOT_RANGE - + validatorIndex: ValidatorIndex, privkey: ValidatorPrivKey, + cache: var StateCache): + Option[AggregateAndProof] = let - slot = state.slot - trailing_distance + slot = state.slot slot_signature = get_slot_signature( state.fork, state.genesis_validators_root, slot, privkey) - doAssert slot + ATTESTATION_PROPAGATION_SLOT_RANGE >= state.slot - doAssert state.slot >= slot - + doAssert validatorIndex in get_beacon_committee(state, slot, index, cache) doAssert index.uint64 < get_committee_count_per_slot(state, slot.epoch, cache) # TODO for testing purposes, refactor this into the condition check @@ -51,27 +45,16 @@ proc aggregate_attestations*( if not is_aggregator(state, slot, index, slot_signature, cache): return none(AggregateAndProof) - # https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/validator.md#attestation-data - # describes how to construct an attestation, which applies for makeAttestationData(...) - # TODO this won't actually match anything - let attestation_data = AttestationData( - slot: slot, - index: index.uint64, - beacon_block_root: get_block_root_at_slot(state, slot)) + let maybe_slot_attestation = getAggregatedAttestation(pool, slot, index) + if maybe_slot_attestation.isNone: + return none(AggregateAndProof) # https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/validator.md#construct-aggregate - # TODO once EV goes in w/ refactoring of getAttestationsForBlock, pull out the getSlot version and use - # it. This is incorrect. - for attestation in getAttestationsForBlock(pool, state): - # getAttestationsForBlock(...) already aggregates - if attestation.data == attestation_data: - # https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/validator.md#aggregateandproof - return some(AggregateAndProof( - aggregator_index: index.uint64, - aggregate: attestation, - selection_proof: slot_signature)) - - none(AggregateAndProof) + # https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/validator.md#aggregateandproof + some(AggregateAndProof( + aggregator_index: validatorIndex.uint64, + aggregate: maybe_slot_attestation.get, + selection_proof: slot_signature)) proc isValidAttestationSlot( pool: AttestationPool, attestationSlot: Slot, attestationBlck: BlockRef): bool = @@ -251,7 +234,7 @@ proc isValidAggregatedAttestation*( # MAXIMUM_GOSSIP_CLOCK_DISPARITY allowance) -- i.e. aggregate.data.slot + # ATTESTATION_PROPAGATION_SLOT_RANGE >= current_slot >= aggregate.data.slot if not checkPropagationSlotRange(aggregate.data, current_slot): - debug "Aggregate slot not in propoagation range" + debug "Aggregate slot not in propagation range" return false # [IGNORE] The valid aggregate attestation defined by diff --git a/beacon_chain/attestation_pool.nim b/beacon_chain/attestation_pool.nim index 01afd29f2..1cf7029be 100644 --- a/beacon_chain/attestation_pool.nim +++ b/beacon_chain/attestation_pool.nim @@ -238,7 +238,7 @@ proc getAttestationsForSlot*(pool: AttestationPool, newBlockSlot: Slot): candidateIdx = pool.candidateIdx(attestationSlot) if candidateIdx.isNone: - info "No attestations matching the slot range", + trace "No attestations matching the slot range", attestationSlot = shortLog(attestationSlot), startingSlot = shortLog(pool.startingSlot) return none(AttestationsSeen) @@ -256,13 +256,7 @@ proc getAttestationsForBlock*(pool: AttestationPool, let newBlockSlot = state.slot var attestations: seq[AttestationEntry] - # This isn't maximally efficient -- iterators or other approaches would - # avoid lots of memory allocations -- but this provides a more flexible - # base upon which to experiment with, and isn't yet profiling hot-path, - # while avoiding penalizing slow attesting too much (as, in the spec it - # is supposed to be available two epochs back; it's not meant as). This - # isn't a good solution, either -- see the set-packing comment below as - # one issue. It also creates problems with lots of repeat attestations, + # This potentially creates problems with lots of repeated attestations, # as a bunch of synchronized beacon_nodes do almost the opposite of the # intended thing -- sure, _blocks_ have to be popular (via attestation) # but _attestations_ shouldn't have to be so frequently repeated, as an @@ -326,6 +320,41 @@ proc getAttestationsForBlock*(pool: AttestationPool, attestationSlot = newBlockSlot - 1 return +proc getAggregatedAttestation*(pool: AttestationPool, + slot: Slot, + index: CommitteeIndex): Option[Attestation] = + let attestations = pool.getAttestationsForSlot( + slot + MIN_ATTESTATION_INCLUSION_DELAY) + if attestations.isNone: + return none(Attestation) + + for a in attestations.get.attestations: + doAssert a.data.slot == slot + if index.uint64 != a.data.index: + continue + + var + # https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/validator.md#construct-attestation + attestation = Attestation( + aggregation_bits: a.validations[0].aggregation_bits, + data: a.data, + signature: a.validations[0].aggregate_signature + ) + + agg {.noInit.}: AggregateSignature + + agg.init(a.validations[0].aggregate_signature) + for v in a.validations[1..^1]: + if not attestation.aggregation_bits.overlaps(v.aggregation_bits): + attestation.aggregation_bits.combine(v.aggregation_bits) + agg.aggregate(v.aggregate_signature) + + attestation.signature = agg.finish() + + return some(attestation) + + none(Attestation) + proc resolve*(pool: var AttestationPool, wallSlot: Slot) = ## Check attestations in our unresolved deque ## if they can be integrated to the fork choice diff --git a/beacon_chain/validator_duties.nim b/beacon_chain/validator_duties.nim index 60275b0f1..88fdef203 100644 --- a/beacon_chain/validator_duties.nim +++ b/beacon_chain/validator_duties.nim @@ -388,8 +388,7 @@ proc handleProposal(node: BeaconNode, head: BlockRef, slot: Slot): return head proc broadcastAggregatedAttestations( - node: BeaconNode, aggregationHead: BlockRef, aggregationSlot: Slot, - trailing_distance: uint64) = + node: BeaconNode, aggregationHead: BlockRef, aggregationSlot: Slot) = # The index is via a # locally attested validator. Unlike in handleAttestations(...) there's a # single one at most per slot (because that's how aggregation attestation @@ -419,10 +418,11 @@ proc broadcastAggregatedAttestations( committee_index.CommitteeIndex, # TODO https://github.com/status-im/nim-beacon-chain/issues/545 # this assumes in-process private keys + validatorIdx, validator.privKey, - trailing_distance, cache) + cache) - # Don't broadcast when, e.g., this node isn't an aggregator + # Don't broadcast when, e.g., this node isn't aggregator if aggregateAndProof.isSome: var signedAP = SignedAggregateAndProof( message: aggregateAndProof.get, @@ -515,9 +515,12 @@ proc handleValidatorDuties*( "Waiting to aggregate attestations") const TRAILING_DISTANCE = 1 + # https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/p2p-interface.md#configuration + static: + doAssert TRAILING_DISTANCE <= ATTESTATION_PROPAGATION_SLOT_RANGE + let aggregationSlot = slot - TRAILING_DISTANCE aggregationHead = get_ancestor(head, aggregationSlot) - broadcastAggregatedAttestations( - node, aggregationHead, aggregationSlot, TRAILING_DISTANCE) + broadcastAggregatedAttestations(node, aggregationHead, aggregationSlot)