fix usage of committee index, vs index in committee; uniformly set tr…

…ailing/following distance; document how the only-broadcast-if mechanism works better and what aggregation already happens, not otherwise sufficiently clear; use correct BlockSlot across epoch boundaries

beacon_chain/attestation_aggregation.nim

@@ -17,7 +17,6 @@ import
   ./spec/[beaconstate, datatypes, crypto, digest, helpers, validator],
   ./attestation_pool, ./beacon_node_types, ./ssz
 
-# TODO gossipsub validation lives somewhere, maybe here
 # TODO add tests, especially for validation
 # https://github.com/status-im/nim-beacon-chain/issues/122#issuecomment-562479965
 
@@ -45,33 +44,41 @@ func is_aggregator(state: BeaconState, slot: Slot, index: uint64,
 
 proc aggregate_attestations*(
     pool: AttestationPool, state: BeaconState, index: uint64,
-    privkey: ValidatorPrivKey): Option[AggregateAndProof] =
+    privkey: ValidatorPrivKey, trailing_distance: uint64): Option[AggregateAndProof] =
   # TODO alias CommitteeIndex to actual type then convert various uint64's here
 
-  if state.slot < 3:
-    return none(AggregateAndProof)
+  doAssert state.slot >= trailing_distance
 
-  # TODO handle epoch boundary condition better; this is ugly kludge
-  # makeAttestationData() checks slot's epoch is state.slot's epoch currently
-  if state.slot mod SLOTS_PER_EPOCH == 0:
-    return none(AggregateAndProof)
+  # https://github.com/ethereum/eth2.0-specs/blob/v0.10.1/specs/phase0/p2p-interface.md#configuration
+  doAssert trailing_distance <= ATTESTATION_PROPAGATION_SLOT_RANGE
 
   let
-    slot = state.slot - 1
+    slot = state.slot - trailing_distance
     slot_signature = get_slot_signature(state, slot, privkey)
 
   doAssert slot + ATTESTATION_PROPAGATION_SLOT_RANGE >= state.slot
   doAssert state.slot >= slot
 
+  # TODO performance issue for future, via get_active_validator_indices(...)
+  doAssert index < get_committee_count_at_slot(state, slot)
+
+  # TODO for testing purposes, refactor this into the condition check
+  # and just calculation
   # https://github.com/ethereum/eth2.0-specs/blob/v0.10.1/specs/phase0/validator.md#aggregation-selection
   if not is_aggregator(state, slot, index, slot_signature):
     return none(AggregateAndProof)
 
-  let attestation_data =
-    makeAttestationData(state, slot, index, get_block_root_at_slot(state, slot))
+  # https://github.com/ethereum/eth2.0-specs/blob/v0.10.1/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,
+    beacon_block_root: get_block_root_at_slot(state, slot))
 
   # https://github.com/ethereum/eth2.0-specs/blob/v0.10.1/specs/phase0/validator.md#construct-aggregate
   for attestation in getAttestationsForBlock(pool, state, slot):
+    # getAttestationsForBlock(...) already aggregates
     if attestation.data == attestation_data:
       # https://github.com/ethereum/eth2.0-specs/blob/v0.10.1/specs/phase0/validator.md#aggregateandproof
       return some(AggregateAndProof(

beacon_chain/beacon_node.nim

@@ -624,17 +624,19 @@ proc verifyFinalization(node: BeaconNode, slot: Slot) =
       node.blockPool.finalizedHead.blck.slot.compute_epoch_at_slot()
     doAssert finalizedEpoch + 2 == epoch
 
-proc broadcastAggregatedAttestations(node: BeaconNode, state: auto, head: var auto, slot: Slot) =
-  # The exact head isn't that important, since here, it's transitively used
-  # by getAttestionsForBlock(...) which uses it to validate. Any state from
-  # the current/future of relevant attestations is fine. The index is via a
+proc broadcastAggregatedAttestations(
+    node: BeaconNode, state: auto, head: var auto, slot: Slot,
+    trailing_distance: uint64) =
+  # 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
   # works), so the machinery that has to handle looping across, basically a
   # set of locally attached validators is in principle not necessary, but a
   # way to organize this. Then the private key for that validator should be
   # the corresponding one -- whatver they are, they match.
-  let bs = BlockSlot(blck: head, slot: head.slot)
+
+  let bs = BlockSlot(blck: head, slot: slot)
+
   let committees_per_slot = get_committee_count_at_slot(state, head.slot)
   var cache = get_empty_per_epoch_cache()
   for committee_index in 0'u64..<committees_per_slot:
@@ -650,11 +652,13 @@ proc broadcastAggregatedAttestations(node: BeaconNode, state: auto, head: var au
         # one isSome() with test.
         let option_aggregateandproof =
           aggregate_attestations(node.attestationPool, state,
-            index_in_committee.uint64,
+            committee_index,
             # TODO https://github.com/status-im/nim-beacon-chain/issues/545
             # this assumes in-process private keys
-            validator.privKey)
+            validator.privKey,
+            trailing_distance)
 
+        # Don't broadcast when, e.g., this node isn't an aggregator
         if option_aggregateandproof.isSome:
           let aggregate_and_proof = option_aggregateandproof.get
           node.network.broadcast(
@@ -756,7 +760,7 @@ proc onSlotStart(node: BeaconNode, lastSlot, scheduledSlot: Slot) {.gcsafe, asyn
 
   var head = node.updateHead()
 
-  # TODO is the slot of the clock or the head block more interestion? provide
+  # TODO is the slot of the clock or the head block more interesting? provide
   #      rationale in comment
   beacon_head_slot.set slot.int64
 
@@ -851,15 +855,18 @@ proc onSlotStart(node: BeaconNode, lastSlot, scheduledSlot: Slot) {.gcsafe, asyn
   # If the validator is selected to aggregate (is_aggregator), then they
   # broadcast their best aggregate to the global aggregate channel
   # (beacon_aggregate_and_proof) two-thirds of the way through the slot
-  # TODO available resultion seems to be integral seconds
-  let bs = BlockSlot(blck: head, slot: head.slot)
-  node.blockPool.withState(node.blockPool.tmpState, bs):
-    let
-      committees_per_slot = get_committee_count_at_slot(state, head.slot)
-      twoThirdsSlot =
+  if slot > 2:
+    const TRAILING_DISTANCE = 1
+    let aggregationSlot = slot - TRAILING_DISTANCE
+    var aggregationHead = get_ancestor(head, aggregationSlot)
+
+    let bs = BlockSlot(blck: aggregationHead, slot: aggregationSlot)
+    node.blockPool.withState(node.blockPool.tmpState, bs):
+      let twoThirdsSlot =
         toBeaconTime(slot, seconds(2*int64(SECONDS_PER_SLOT)) div 3)
-    addTimer(saturate(node.beaconClock.fromNow(twoThirdsSlot))) do (p: pointer):
-      broadcastAggregatedAttestations(node, state, head, slot)
+      addTimer(saturate(node.beaconClock.fromNow(twoThirdsSlot))) do (p: pointer):
+        broadcastAggregatedAttestations(
+          node, state, aggregationHead, aggregationSlot, TRAILING_DISTANCE)
 
   # TODO ... and beacon clock might jump here also. sigh.
   let