Bugfix: Avoid the aggregation of duplicate signatures when creating sync committee contributions

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:

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")

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))

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",

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)

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: