Implement a missing ingnore rule for sync committee contributions

status-im · Aug 7, 2022 · cd23346 · cd23346
1 parent 250f7b4
commit cd23346
Show file tree

Hide file tree

Showing 9 changed files with 65 additions and 25 deletions.
diff --git a/AllTests-mainnet.md b/AllTests-mainnet.md
@@ -513,9 +513,9 @@ OK: 3/3 Fail: 0/3 Skip: 0/3
 + Add keystore files [REMOTE]                                                                OK
 + Add keystore files twice [LOCAL]                                                           OK
 + Add keystore files twice [REMOTE]                                                          OK
-+ `createValidatorFiles` with `keystoreDir` without permissions                              OK
-+ `createValidatorFiles` with `secretsDir` without permissions                               OK
-+ `createValidatorFiles` with `validatorsDir` without permissions                            OK
++ `createLocalValidatorFiles` with `keystoreDir` without permissions                         OK
++ `createLocalValidatorFiles` with `secretsDir` without permissions                          OK
++ `createLocalValidatorFiles` with `validatorsDir` without permissions                       OK
 + `createValidatorFiles` with already existing dirs and any error                            OK
 ```
 OK: 8/8 Fail: 0/8 Skip: 0/8

diff --git a/beacon_chain/consensus_object_pools/sync_committee_msg_pool.nim b/beacon_chain/consensus_object_pools/sync_committee_msg_pool.nim
@@ -176,8 +176,14 @@ func produceContribution*(
   else:
     false
 
+type
+  AddContributionResult* = enum
+    newBest
+    notBestButNotSubsetOfBest
+    strictSubsetOfTheBest
+
 func addAggregateAux(bestVotes: var BestSyncSubcommitteeContributions,
-                     contribution: SyncCommitteeContribution) =
+                     contribution: SyncCommitteeContribution): AddContributionResult =
   let
     currentBestTotalParticipants =
       bestVotes.subnets[contribution.subcommittee_index].totalParticipants
@@ -189,6 +195,12 @@ func addAggregateAux(bestVotes: var BestSyncSubcommitteeContributions,
         totalParticipants: newBestTotalParticipants,
         participationBits: contribution.aggregation_bits,
         signature: contribution.signature.load.get)
+    newBest
+  elif contribution.aggregation_bits.isSubsetOf(
+         bestVotes.subnets[contribution.subcommittee_index].participationBits):
+    strictSubsetOfTheBest
+  else:
+    notBestButNotSubsetOfBest
 
 func isSeen*(
     pool: SyncCommitteeMsgPool,
@@ -200,9 +212,9 @@ func isSeen*(
   seenKey in pool.seenContributionByAuthor
 
 proc addContribution(pool: var SyncCommitteeMsgPool,
-                         aggregator_index: uint64,
-                         contribution: SyncCommitteeContribution,
-                         signature: CookedSig) =
+                     aggregator_index: uint64,
+                     contribution: SyncCommitteeContribution,
+                     signature: CookedSig): AddContributionResult =
   let seenKey = SyncCommitteeMsgKey(
     originator: aggregator_index,
     slot: contribution.slot,
@@ -223,16 +235,17 @@ proc addContribution(pool: var SyncCommitteeMsgPool,
         signature: signature)
 
     pool.bestContributions[blockRoot] = initialBestContributions
+    newBest
   else:
     try:
       addAggregateAux(pool.bestContributions[blockRoot], contribution)
     except KeyError:
       raiseAssert "We have checked for the key upfront"
 
 proc addContribution*(pool: var SyncCommitteeMsgPool,
-                          scproof: SignedContributionAndProof,
-                          signature: CookedSig) =
-  pool.addContribution(
+                      scproof: SignedContributionAndProof,
+                      signature: CookedSig): AddContributionResult =
+  result = pool.addContribution(
     scproof.message.aggregator_index, scproof.message.contribution, signature)
 
   if not(isNil(pool.onContributionReceived)):

diff --git a/beacon_chain/gossip_processing/eth2_processor.nim b/beacon_chain/gossip_processing/eth2_processor.nim
@@ -536,15 +536,26 @@ proc processSignedContributionAndProof*(
 
   return if v.isOk():
     trace "Contribution validated"
-    self.syncCommitteeMsgPool[].addContribution(
+    let addStatus = self.syncCommitteeMsgPool[].addContribution(
       contributionAndProof, v.get()[0])
 
     self.validatorMonitor[].registerSyncContribution(
       src, wallTime, contributionAndProof.message, v.get()[1])
 
     beacon_sync_committee_contributions_received.inc()
 
-    ok()
+    case addStatus
+    of newBest, notBestButNotSubsetOfBest: ok()
+    of strictSubsetOfTheBest:
+      # This implements the spec directive:
+      #
+      # _[IGNORE]_ A valid sync committee contribution with equal `slot`, `beacon_block_root`
+      # and `subcommittee_index` whose `aggregation_bits` is non-strict superset has _not_
+      # already been seen.
+      #
+      # We are implementing this here, because this may be an unique contribution, so we would
+      # like for it to be counted and registered by the validator monitor above.
+      errIgnore("strict superset already seen")
   else:
     debug "Dropping contribution", error = v.error
     beacon_sync_committee_contributions_dropped.inc(1, [$v.error[0]])

diff --git a/beacon_chain/gossip_processing/gossip_validation.nim b/beacon_chain/gossip_processing/gossip_validation.nim
@@ -982,6 +982,14 @@ proc validateContribution*(
   let participants = dag.syncCommitteeParticipants(
     msg.message.contribution.slot, subcommitteeIdx)
 
+  # The following spec rule:
+  #
+  # _[IGNORE]_ A valid sync committee contribution with equal `slot`, `beacon_block_root`
+  # and `subcommittee_index` whose `aggregation_bits` is non-strict superset has _not_
+  # already been seen.
+  #
+  # is implemented in eth2_processor.nim
+
   let sig = if checkSignature:
     let deferredCrypto = batchCrypto.scheduleContributionChecks(
       fork, genesis_validators_root, msg, subcommitteeIdx, dag)

diff --git a/research/block_sim.nim b/research/block_sim.nim
@@ -233,7 +233,7 @@ cli do(slots = SLOTS_PER_EPOCH * 6,
 
         doAssert res.isOk
 
-        syncCommitteePool[].addContribution(
+        discard syncCommitteePool[].addContribution(
           signedContributionAndProof, res.get()[0])
 
   proc getNewBlock[T](

diff --git a/tests/test_gossip_validation.nim b/tests/test_gossip_validation.nim
@@ -245,11 +245,13 @@ suite "Gossip validation - Extra": # Not based on preset config
     let
       contribution = block:
         let contribution = (ref SignedContributionAndProof)()
-        check: syncCommitteeMsgPool[].produceContribution(
-          slot, state[].root, subcommitteeIdx,
-          contribution.message.contribution)
-        syncCommitteeMsgPool[].addContribution(
+        check:
+          syncCommitteeMsgPool[].produceContribution(
+            slot, state[].root, subcommitteeIdx,
+            contribution.message.contribution)
+        let addContributionRes = syncCommitteeMsgPool[].addContribution(
           contribution[], contribution.message.contribution.signature.load.get)
+        check addContributionRes == newBest
         let signRes = waitFor validator.getContributionAndProofSignature(
           state[].data.fork, state[].data.genesis_validators_root,
           contribution[].message)

diff --git a/tests/test_sync_committee_pool.nim b/tests/test_sync_committee_pool.nim
@@ -118,8 +118,10 @@ suite "Sync committee pool":
         contribution.aggregation_bits[10] == true
         contribution.signature == expectedSig.toValidatorSig
 
-      pool.addContribution(outContribution, expectedSig)
-      check: pool.isSeen(outContribution.message)
+      let addContributionRes = pool.addContribution(outContribution, expectedSig)
+      check:
+        addContributionRes == newBest
+        pool.isSeen(outContribution.message)
 
     block:
       # Checking a committee with a signle participant:
@@ -140,8 +142,10 @@ suite "Sync committee pool":
         contribution.aggregation_bits[7] == true
         contribution.signature == sig3.toValidatorSig
 
-      pool.addContribution(outContribution, sig3)
-      check: pool.isSeen(outContribution.message)
+      let addContributionRes = pool.addContribution(outContribution, sig3)
+      check:
+        addContributionRes == newBest
+        pool.isSeen(outContribution.message)
 
     block:
       # Checking another committee with a signle participant
@@ -163,8 +167,10 @@ suite "Sync committee pool":
         contribution.aggregation_bits[3] == true
         contribution.signature == sig4.toValidatorSig
 
-      pool.addContribution(outContribution, sig4)
-      check: pool.isSeen(outContribution.message)
+      let addContributionRes = pool.addContribution(outContribution, sig4)
+      check:
+        addContributionRes == newBest
+        pool.isSeen(outContribution.message)
 
     block:
       # Checking a block root nobody voted for

diff --git a/tests/testblockutil.nim b/tests/testblockutil.nim
@@ -411,7 +411,7 @@ proc makeSyncAggregate(
         signedContributionAndProof = SignedContributionAndProof(
           message: contributionAndProof,
           signature: contributionSig.toValidatorSig)
-      syncCommitteePool[].addContribution(
+      discard syncCommitteePool[].addContribution(
         signedContributionAndProof, contribution.signature.load.get)
 
   syncCommitteePool[].produceSyncAggregate(latest_block_root)

diff --git a/vendor/nim-ssz-serialization b/vendor/nim-ssz-serialization