Add support for dealing with overlapping attestations (#262)

beacon_chain/attestation_pool.nim

@@ -13,20 +13,21 @@ proc init*(T: type AttestationPool, blockPool: BlockPool): T =
     latestAttestations: initTable[ValidatorPubKey, BlockRef]()
   )
 
-
 proc combine*(tgt: var Attestation, src: Attestation, flags: UpdateFlags) =
-  # Combine the signature and participation bitfield, with the assumption that
+  ## Combine the signature and participation bitfield, with the assumption that
-  # the same data is being signed!
+  ## the same data is being signed - if the signatures overlap, they are not
+  ## combined.
 
   doAssert tgt.data == src.data
 
-  # TODO:
+  # In a BLS aggregate signature, one needs to count how many times a
-  # when BLS signatures are combined, we must ensure that
+  # particular public key has been added - since we use a single bit per key, we
-  # the same participant key is not included on both sides
+  # can only it once, thus we can never combine signatures that overlap already!
-  tgt.aggregation_bitfield.combine(src.aggregation_bitfield)
+  if not tgt.aggregation_bitfield.overlaps(src.aggregation_bitfield):
+    tgt.aggregation_bitfield.combine(src.aggregation_bitfield)
 
-  if skipValidation notin flags:
+    if skipValidation notin flags:
-    tgt.aggregate_signature.combine(src.aggregate_signature)
+      tgt.aggregate_signature.combine(src.aggregate_signature)
 
 proc validate(
     state: BeaconState, attestation: Attestation, flags: UpdateFlags): bool =
@@ -113,6 +114,7 @@ proc slotIndex(
   # epoch to now, because these are the attestations that may affect the voting
   # outcome. Some of these attestations will already have been added to blocks,
   # while others are fresh off the network.
+  # TODO only the latest vote of each validator counts. Can we use that somehow?
 
   doAssert attestationSlot >= pool.startingSlot,
     """
@@ -186,15 +188,14 @@ proc add*(pool: var AttestationPool,
   for a in slotData.attestations.mitems():
     if a.data == attestation.data:
       for v in a.validations:
-        if v.aggregation_bitfield.overlaps(validation.aggregation_bitfield):
+        if validation.aggregation_bitfield.isSubsetOf(v.aggregation_bitfield):
-          # TODO this check is here so that later, when we combine signatures,
+          # The validations in the new attestation are a subset of one of the
-          #      there is no overlap (each validator must be represented once
+          # attestations that we already have on file - no need to add this
-          #      only). this is wrong - we could technically receive
+          # attestation to the database
-          #      attestations that have already been combined (for example when
+          # TODO what if the new attestation is useful for creating bigger
-          #      feeding in attestations from blocks, which we're not doing yet)
+          #      sets by virtue of not overlapping with some other attestation
-          #      but then we'll also have to update the combine logic to deal
+          #      and therefore being useful after all?
-          #      with this complication.
+          debug "Ignoring subset attestation",
-          debug "Ignoring overlapping attestation",
             existingParticipants = get_attestation_participants(
               state, a.data, v.aggregation_bitfield),
             newParticipants = participants
@@ -202,12 +203,25 @@ proc add*(pool: var AttestationPool,
           break
 
       if not found:
+        # Attestations in the pool that are a subset of the new attestation
+        # can now be removed per same logic as above
+        a.validations.keepItIf(
+          if it.aggregation_bitfield.isSubsetOf(
+              validation.aggregation_bitfield):
+            debug "Removing subset attestation",
+              existingParticipants = get_attestation_participants(
+                state, a.data, it.aggregation_bitfield),
+              newParticipants = participants
+            false
+          else:
+            true)
+
         a.validations.add(validation)
         pool.updateLatestVotes(state, attestationSlot, participants, a.blck)
 
         info "Attestation resolved",
           attestationData = shortLog(attestation.data),
-          validations = a.validations.len() # TODO popcount of union
+          validations = a.validations.len()
 
         found = true
 
@@ -286,6 +300,13 @@ proc getAttestationsForBlock*(
       continue
 
     for v in a.validations[1..^1]:
+      # TODO We need to select a set of attestations that maximise profit by
+      #      adding the largest combined attestation set that we can find - this
+      #      unfortunately looks an awful lot like
+      #      https://en.wikipedia.org/wiki/Set_packing - here we just iterate
+      #      and naively add as much as possible in one go, by we could also
+      #      add the same attestation data twice, as long as there's at least
+      #      one new attestation in there
       if not attestation.aggregation_bitfield.overlaps(
           v.aggregation_bitfield):
         attestation.aggregation_bitfield.combine(

beacon_chain/beacon_node.nim

@@ -429,10 +429,9 @@ proc onBeaconBlock(node: BeaconNode, blck: BeaconBlock) =
   # The block we received contains attestations, and we might not yet know about
   # all of them. Let's add them to the attestation pool - in case they block
   # is not yet resolved, neither will the attestations be!
+  # TODO shouldn't add attestations if the block turns out to be invalid..
   for attestation in blck.body.attestations:
-    # TODO attestation pool needs to be taught to deal with overlapping
+    node.onAttestation(attestation)
-    #      attestations!
-    discard # node.onAttestation(attestation)
 
 proc handleAttestations(node: BeaconNode, head: BlockRef, slot: Slot) =
   ## Perform all attestations that the validators attached to this node should

beacon_chain/spec/bitfield.nim

@@ -1,4 +1,4 @@
-import byteutils, json_serialization
+import byteutils, json_serialization, std_shims/support/bitops2
 
 type
   BitField* = object
@@ -45,7 +45,19 @@ func combine*(tgt: var BitField, src: BitField) =
   for i in 0 ..< tgt.bits.len:
     tgt.bits[i] = tgt.bits[i] or src.bits[i]
 
-proc overlaps*(a, b: BitField): bool =
+func overlaps*(a, b: BitField): bool =
   for i in 0..<a.bits.len:
     if (a.bits[i] and b.bits[i]) > 0'u8:
       return true
+
+func countOnes*(a: BitField): int {.inline.} =
+  for v in a.bits: result += countOnes(v)
+
+func len*(a: BitField): int {.inline.} =
+  countOnes(a)
+
+func isSubsetOf*(a, b: Bitfield): bool =
+  for i in 0 ..< (len(a.bits) * 8):
+    if get_bitfield_bit(a, i) and not get_bitfield_bit(b, i):
+      return false
+  true

tests/test_attestation_pool.nim

@@ -11,71 +11,145 @@ import
   ../beacon_chain/spec/[beaconstate, crypto, datatypes, digest, helpers, validator],
   ../beacon_chain/[beacon_node_types, attestation_pool, block_pool, extras, state_transition, ssz]
 
+template withPool(body: untyped) =
+  mixin genState, genBlock
+
+  var
+    blockPool {.inject.} = BlockPool.init(makeTestDB(genState, genBlock))
+    pool {.inject.} = AttestationPool.init(blockPool)
+    state {.inject.} = loadTailState(blockPool)
+  # Slot 0 is a finalized slot - won't be making attestations for it..
+  advanceState(state.data)
+
+  body
+
 suite "Attestation pool processing" & preset():
   ## For now just test that we can compile and execute block processing with
   ## mock data.
 
-  # Genesis state with minimal number of deposits
+  # Genesis state that results in 2 members per committee
   let
     genState = get_genesis_beacon_state(
-      makeInitialDeposits(flags = {skipValidation}), 0, Eth1Data(),
+      makeInitialDeposits(SLOTS_PER_EPOCH * 2, {skipValidation}), 0, Eth1Data(),
         {skipValidation})
     genBlock = get_initial_beacon_block(genState)
 
   test "Can add and retrieve simple attestation" & preset():
-    var
+    withPool:
-      blockPool = BlockPool.init(makeTestDB(genState, genBlock))
+      let
-      pool = AttestationPool.init(blockPool)
+        # Create an attestation for slot 1!
-      state = blockPool.loadTailState()
+        crosslink_committees =
-    # Slot 0 is a finalized slot - won't be making attestations for it..
+          get_crosslink_committees_at_slot(state.data.data, state.data.data.slot)
-    advanceState(state.data)
+        attestation = makeAttestation(
+          state.data.data, state.blck.root, crosslink_committees[0].committee[0])
 
-    let
+      pool.add(state.data.data, attestation)
-      # Create an attestation for slot 1 signed by the only attester we have!
-      crosslink_committees =
-        get_crosslink_committees_at_slot(state.data.data, state.data.data.slot)
-      attestation = makeAttestation(
-        state.data.data, state.blck.root, crosslink_committees[0].committee[0])
 
-    pool.add(state.data.data, attestation)
+      for i in 0..<MIN_ATTESTATION_INCLUSION_DELAY.int - 1:
+        advanceState(state.data)
 
-    let attestations = pool.getAttestationsForBlock(
+      let attestations = pool.getAttestationsForBlock(
-      state.data.data, state.data.data.slot + MIN_ATTESTATION_INCLUSION_DELAY)
+        state.data.data, state.data.data.slot + 1)
 
-    # TODO test needs fixing for new attestation validation
+      check:
-    # check:
+        attestations.len == 1
-    #   attestations.len == 1
 
   test "Attestations may arrive in any order" & preset():
-    var
+    withPool:
-      blockPool = BlockPool.init(makeTestDB(genState, genBlock))
+      let
-      pool = AttestationPool.init(blockPool)
+        # Create an attestation for slot 1!
-      state = blockPool.loadTailState()
+        cc0 =
-    # Slot 0 is a finalized slot - won't be making attestations for it..
+          get_crosslink_committees_at_slot(state.data.data, state.data.data.slot)
-    advanceState(state.data)
+        attestation0 = makeAttestation(
+          state.data.data, state.blck.root, cc0[0].committee[0])
 
-    let
+      advanceState(state.data)
-      # Create an attestation for slot 1 signed by the only attester we have!
-      crosslink_committees1 =
-        get_crosslink_committees_at_slot(state.data.data, state.data.data.slot)
-      attestation1 = makeAttestation(
-        state.data.data, state.blck.root, crosslink_committees1[0].committee[0])
 
-    advanceState(state.data)
+      let
+        cc1 =
+          get_crosslink_committees_at_slot(state.data.data, state.data.data.slot)
+        attestation1 = makeAttestation(
+          state.data.data, state.blck.root, cc1[0].committee[0])
 
-    let
+      # test reverse order
-      crosslink_committees2 =
+      pool.add(state.data.data, attestation1)
-        get_crosslink_committees_at_slot(state.data.data, state.data.data.slot)
+      pool.add(state.data.data, attestation0)
-      attestation2 = makeAttestation(
-        state.data.data, state.blck.root, crosslink_committees2[0].committee[0])
 
-    # test reverse order
+      for i in 0..<MIN_ATTESTATION_INCLUSION_DELAY.int - 1: advanceState(state.data)
-    pool.add(state.data.data, attestation2)
-    pool.add(state.data.data, attestation1)
 
-    let attestations = pool.getAttestationsForBlock(
+      let attestations = pool.getAttestationsForBlock(
-      state.data.data, state.data.data.slot + MIN_ATTESTATION_INCLUSION_DELAY)
+        state.data.data, state.data.data.slot + 1)
 
-    # TODO test needs fixing for new attestation validation
+      check:
-    # check:
+        attestations.len == 1
-    #   attestations.len == 1
+
+  test "Attestations should be combined" & preset():
+    withPool:
+      let
+        # Create an attestation for slot 1!
+        cc0 =
+          get_crosslink_committees_at_slot(state.data.data, state.data.data.slot)
+        attestation0 = makeAttestation(
+          state.data.data, state.blck.root, cc0[0].committee[0])
+        attestation1 = makeAttestation(
+          state.data.data, state.blck.root, cc0[0].committee[1])
+
+      pool.add(state.data.data, attestation0)
+      pool.add(state.data.data, attestation1)
+
+      for i in 0..<MIN_ATTESTATION_INCLUSION_DELAY.int - 1: advanceState(state.data)
+
+      let attestations = pool.getAttestationsForBlock(
+        state.data.data, state.data.data.slot + 1)
+
+      check:
+        attestations.len == 1
+
+  test "Attestations may overlap, bigger first" & preset():
+    withPool:
+
+      var
+        # Create an attestation for slot 1!
+        cc0 =
+          get_crosslink_committees_at_slot(state.data.data, state.data.data.slot)
+        attestation0 = makeAttestation(
+          state.data.data, state.blck.root, cc0[0].committee[0])
+        attestation1 = makeAttestation(
+          state.data.data, state.blck.root, cc0[0].committee[1])
+
+      attestation0.combine(attestation1, {skipValidation})
+
+      pool.add(state.data.data, attestation0)
+      pool.add(state.data.data, attestation1)
+
+      for i in 0..<MIN_ATTESTATION_INCLUSION_DELAY.int - 1: advanceState(state.data)
+
+      let attestations = pool.getAttestationsForBlock(
+        state.data.data, state.data.data.slot + 1)
+
+      check:
+        attestations.len == 1
+
+  test "Attestations may overlap, smaller first" & preset():
+    withPool:
+      var
+        # Create an attestation for slot 1!
+        cc0 =
+          get_crosslink_committees_at_slot(state.data.data, state.data.data.slot)
+        attestation0 = makeAttestation(
+          state.data.data, state.blck.root, cc0[0].committee[0])
+        attestation1 = makeAttestation(
+          state.data.data, state.blck.root, cc0[0].committee[1])
+
+      attestation0.combine(attestation1, {skipValidation})
+
+      pool.add(state.data.data, attestation1)
+      pool.add(state.data.data, attestation0)
+
+      for i in 0..<MIN_ATTESTATION_INCLUSION_DELAY.int - 1: advanceState(state.data)
+
+      let attestations = pool.getAttestationsForBlock(
+        state.data.data, state.data.data.slot + 1)
+
+      check:
+        attestations.len == 1