From 29e57008385250966d1872a49dfd1221e4c76a2f Mon Sep 17 00:00:00 2001 From: Zahary Karadjov Date: Thu, 4 Nov 2021 20:22:24 +0200 Subject: [PATCH] Bugfix: Avoid the aggregation of duplicate signatures when creating sync committee contributions --- .../sync_committee_msg_pool.nim | 16 +++++++++------- .../gossip_processing/gossip_validation.nim | 12 +++++------- beacon_chain/ssz/types.nim | 1 + beacon_chain/validators/validator_duties.nim | 8 ++++++-- tests/test_gossip_validation.nim | 9 +++++---- tests/test_sync_committee_pool.nim | 3 +++ 6 files changed, 29 insertions(+), 20 deletions(-) diff --git a/beacon_chain/consensus_object_pools/sync_committee_msg_pool.nim b/beacon_chain/consensus_object_pools/sync_committee_msg_pool.nim index 30d009d29..a1c083c9e 100644 --- a/beacon_chain/consensus_object_pools/sync_committee_msg_pool.nim +++ b/beacon_chain/consensus_object_pools/sync_committee_msg_pool.nim @@ -131,13 +131,14 @@ func computeAggregateSig(votes: seq[TrustedSyncCommitteeMsg], if vote.subcommitteeIndex != subcommitteeIndex: continue - if not initialized: - initialized = true - aggregateSig.init(vote.signature) - else: - aggregateSig.aggregate(vote.signature) + if not contribution.aggregation_bits[vote.positionInCommittee]: + if not initialized: + initialized = true + aggregateSig.init(vote.signature) + else: + aggregateSig.aggregate(vote.signature) - contribution.aggregation_bits.setBit vote.positionInCommittee + contribution.aggregation_bits.setBit vote.positionInCommittee if initialized: contribution.signature = aggregateSig.finish.toValidatorSig @@ -156,7 +157,8 @@ func produceContribution*( outContribution.subcommittee_index = subcommitteeIndex.asUInt64 try: computeAggregateSig(pool.syncMessages[headRoot], - subcommitteeIndex, outContribution) + subcommitteeIndex, + outContribution) except KeyError: raiseAssert "We have checked for the key upfront" else: diff --git a/beacon_chain/gossip_processing/gossip_validation.nim b/beacon_chain/gossip_processing/gossip_validation.nim index be9132e31..2a2035d98 100644 --- a/beacon_chain/gossip_processing/gossip_validation.nim +++ b/beacon_chain/gossip_processing/gossip_validation.nim @@ -853,19 +853,17 @@ proc validateSignedContributionAndProof*( var committeeAggKey {.noInit.}: AggregatePublicKey initialized = false - mixedKeys = 0 + syncCommitteeSlot = msg.message.contribution.slot + 1 for validatorPubKey in dag.syncCommitteeParticipants( - msg.message.contribution.slot + 1, + syncCommitteeSlot, committeeIdx, msg.message.contribution.aggregation_bits): let validatorPubKey = validatorPubKey.loadWithCache.get if not initialized: initialized = true committeeAggKey.init(validatorPubKey) - inc mixedKeys else: - inc mixedKeys committeeAggKey.aggregate(validatorPubKey) if not initialized: @@ -883,10 +881,10 @@ proc validateSignedContributionAndProof*( epoch, msg.message.contribution.beacon_block_root, fork, genesisValidatorsRoot, committeeAggKey.finish, cookedSignature.get): debug "failing_sync_contribution", - slot = msg.message.contribution.slot + 1, + blk = msg.message.contribution.beacon_block_root, + slot = syncCommitteeSlot, subnet = committeeIdx, - participants = $(msg.message.contribution.aggregation_bits), - mixedKeys + participants = $(msg.message.contribution.aggregation_bits) return errReject( "SignedContributionAndProof: aggregate signature fails to verify") diff --git a/beacon_chain/ssz/types.nim b/beacon_chain/ssz/types.nim index aa6c0dd34..12e59ffcb 100644 --- a/beacon_chain/ssz/types.nim +++ b/beacon_chain/ssz/types.nim @@ -188,6 +188,7 @@ template bytes*(x: BitList): auto = seq[byte](x) template `[]`*(x: BitList, idx: auto): auto = BitSeq(x)[idx] template `[]=`*(x: var BitList, idx: auto, val: bool) = BitSeq(x)[idx] = val template `==`*(a, b: BitList): bool = BitSeq(a) == BitSeq(b) +template getBit*(x: var BitList, idx: Natural): bool = getBit(BitSeq(x), idx) template setBit*(x: var BitList, idx: Natural) = setBit(BitSeq(x), idx) template clearBit*(x: var BitList, idx: Natural) = clearBit(BitSeq(x), idx) template overlaps*(a, b: BitList): bool = overlaps(BitSeq(a), BitSeq(b)) diff --git a/beacon_chain/validators/validator_duties.nim b/beacon_chain/validators/validator_duties.nim index cbbf75528..f3fe81fb8 100644 --- a/beacon_chain/validators/validator_duties.nim +++ b/beacon_chain/validators/validator_duties.nim @@ -708,6 +708,7 @@ proc signAndSendContribution(node: BeaconNode, # Failures logged in sendSyncCommitteeContribution discard await node.sendSyncCommitteeContribution(msg[], false) + notice "Contribution sent", contribution = shortLog(msg[]) except CatchableError as exc: # An error could happen here when the signature task fails - we must # not leak the exception because this is an asyncSpawn task @@ -751,6 +752,7 @@ proc handleSyncCommitteeContributions(node: BeaconNode, count = selectionProofs.len, time var contributionsSent = 0 + time = timeIt: for i in 0 ..< selectionProofs.len: if not selectionProofs[i].completed: @@ -762,7 +764,10 @@ proc handleSyncCommitteeContributions(node: BeaconNode, var contribution: SyncCommitteeContribution let contributionWasProduced = node.syncCommitteeMsgPool[].produceContribution( - slot, head.root, candidateAggregators[i].subcommitteeIdx, contribution) + slot, + head.root, + candidateAggregators[i].subcommitteeIdx, + contribution) if contributionWasProduced: asyncSpawn signAndSendContribution( @@ -770,7 +775,6 @@ proc handleSyncCommitteeContributions(node: BeaconNode, candidateAggregators[i].validator, contribution, selectionProof) - notice "Contribution sent", contribution = shortLog(contribution) inc contributionsSent else: debug "Failure to produce contribution", diff --git a/tests/test_gossip_validation.nim b/tests/test_gossip_validation.nim index b5db215ab..fba5fbdd0 100644 --- a/tests/test_gossip_validation.nim +++ b/tests/test_gossip_validation.nim @@ -207,9 +207,10 @@ suite "Gossip validation - Extra": # Not based on preset config dag.updateHead(added[], quarantine) dag state = assignClone(dag.headState.data.altairData) + slot = state[].data.slot subcommitteeIdx = 0.SyncSubcommitteeIndex - syncCommittee = @(dag.syncCommitteeParticipants(state[].data.slot)) + syncCommittee = @(dag.syncCommitteeParticipants(slot)) subcommittee = toSeq(syncCommittee.syncSubcommittee(subcommitteeIdx)) pubkey = subcommittee[0] @@ -220,13 +221,13 @@ suite "Gossip validation - Extra": # Not based on preset config validator = AttachedValidator(pubKey: pubkey, kind: ValidatorKind.Local, data: privateItem, index: some(index)) msg = waitFor signSyncCommitteeMessage( - validator, state[].data.slot, + validator, slot, state[].data.fork, state[].data.genesis_validators_root, state[].root) syncCommitteeMsgPool = newClone(SyncCommitteeMsgPool.init()) res = validateSyncCommitteeMessage( dag, syncCommitteeMsgPool[], msg, subcommitteeIdx, - state[].data.slot.toBeaconTime(), true) + slot.toBeaconTime(), true) (positions, cookedSig) = res.get() syncCommitteeMsgPool[].addSyncCommitteeMsg( @@ -241,7 +242,7 @@ suite "Gossip validation - Extra": # Not based on preset config contribution = block: var contribution = (ref SignedContributionAndProof)() check: syncCommitteeMsgPool[].produceContribution( - state[].data.slot, state[].root, subcommitteeIdx, + slot, state[].root, subcommitteeIdx, contribution.message.contribution) syncCommitteeMsgPool[].addSyncContribution( contribution[], contribution.message.contribution.signature.load.get) diff --git a/tests/test_sync_committee_pool.nim b/tests/test_sync_committee_pool.nim index 70c55a1b0..93016f163 100644 --- a/tests/test_sync_committee_pool.nim +++ b/tests/test_sync_committee_pool.nim @@ -81,6 +81,9 @@ suite "Sync committee pool": pool.addSyncCommitteeMsg(root2Slot, root1, 3, sig3, subcommittee2, [7'u64]) pool.addSyncCommitteeMsg(root2Slot, root2, 4, sig4, subcommittee2, [3'u64]) + # Insert a duplicate message (this should be handled gracefully) + pool.addSyncCommitteeMsg(root1Slot, root1, 1, sig1, subcommittee1, [1'u64]) + # Producing contributions # block: