diff --git a/beacon_chain/attestation_pool.nim b/beacon_chain/attestation_pool.nim index 5764e7dbc..bf55724ec 100644 --- a/beacon_chain/attestation_pool.nim +++ b/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 - # the same data is being signed! + ## Combine the signature and participation bitfield, with the assumption that + ## the same data is being signed - if the signatures overlap, they are not + ## combined. doAssert tgt.data == src.data - # TODO: - # when BLS signatures are combined, we must ensure that - # the same participant key is not included on both sides - tgt.aggregation_bitfield.combine(src.aggregation_bitfield) + # In a BLS aggregate signature, one needs to count how many times a + # particular public key has been added - since we use a single bit per key, we + # can only it once, thus we can never combine signatures that overlap already! + if not tgt.aggregation_bitfield.overlaps(src.aggregation_bitfield): + tgt.aggregation_bitfield.combine(src.aggregation_bitfield) - if skipValidation notin flags: - tgt.aggregate_signature.combine(src.aggregate_signature) + if skipValidation notin flags: + 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): - # TODO this check is here so that later, when we combine signatures, - # there is no overlap (each validator must be represented once - # only). this is wrong - we could technically receive - # attestations that have already been combined (for example when - # feeding in attestations from blocks, which we're not doing yet) - # but then we'll also have to update the combine logic to deal - # with this complication. - debug "Ignoring overlapping attestation", + if validation.aggregation_bitfield.isSubsetOf(v.aggregation_bitfield): + # The validations in the new attestation are a subset of one of the + # attestations that we already have on file - no need to add this + # attestation to the database + # TODO what if the new attestation is useful for creating bigger + # sets by virtue of not overlapping with some other attestation + # and therefore being useful after all? + debug "Ignoring subset 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( diff --git a/beacon_chain/beacon_node.nim b/beacon_chain/beacon_node.nim index 50f3fa903..dc6fbd0c8 100644 --- a/beacon_chain/beacon_node.nim +++ b/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 - # attestations! - discard # node.onAttestation(attestation) + node.onAttestation(attestation) proc handleAttestations(node: BeaconNode, head: BlockRef, slot: Slot) = ## Perform all attestations that the validators attached to this node should diff --git a/beacon_chain/spec/bitfield.nim b/beacon_chain/spec/bitfield.nim index a52c75d6c..21d7e768e 100644 --- a/beacon_chain/spec/bitfield.nim +++ b/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.. 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 diff --git a/tests/test_attestation_pool.nim b/tests/test_attestation_pool.nim index 58164b825..b79da89b8 100644 --- a/tests/test_attestation_pool.nim +++ b/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 - blockPool = BlockPool.init(makeTestDB(genState, genBlock)) - pool = AttestationPool.init(blockPool) - state = blockPool.loadTailState() - # Slot 0 is a finalized slot - won't be making attestations for it.. - advanceState(state.data) + withPool: + let + # Create an attestation for slot 1! + 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]) - let - # 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) - pool.add(state.data.data, attestation) + for i in 0..