From dc50abbc9096021b1dbe7254468a35d8a26754fd Mon Sep 17 00:00:00 2001 From: zah Date: Tue, 9 Aug 2022 12:52:11 +0300 Subject: [PATCH] Implement a missing ingnore rule for sync committee contributions (#3941) --- AllTests-mainnet.md | 6 ++--- .../sync_committee_msg_pool.nim | 27 ++++++++++++++----- .../gossip_processing/eth2_processor.nim | 15 +++++++++-- .../gossip_processing/gossip_validation.nim | 8 ++++++ research/block_sim.nim | 2 +- tests/test_gossip_validation.nim | 10 ++++--- tests/test_sync_committee_pool.nim | 18 ++++++++----- tests/testblockutil.nim | 2 +- vendor/nim-ssz-serialization | 2 +- 9 files changed, 65 insertions(+), 25 deletions(-) diff --git a/AllTests-mainnet.md b/AllTests-mainnet.md index 989d1fc95..289be94c7 100644 --- 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 index f7d4b032e..4f5d485dc 100644 --- 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,6 +235,7 @@ proc addContribution(pool: var SyncCommitteeMsgPool, signature: signature) pool.bestContributions[blockRoot] = initialBestContributions + newBest else: try: addAggregateAux(pool.bestContributions[blockRoot], contribution) @@ -230,9 +243,9 @@ proc addContribution(pool: var SyncCommitteeMsgPool, 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 index 38dba1fee..1940ce8eb 100644 --- a/beacon_chain/gossip_processing/eth2_processor.nim +++ b/beacon_chain/gossip_processing/eth2_processor.nim @@ -536,7 +536,7 @@ proc processSignedContributionAndProof*( return if v.isOk(): trace "Contribution validated" - self.syncCommitteeMsgPool[].addContribution( + let addStatus = self.syncCommitteeMsgPool[].addContribution( contributionAndProof, v.get()[0]) self.validatorMonitor[].registerSyncContribution( @@ -544,7 +544,18 @@ proc processSignedContributionAndProof*( 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 index fb8838414..771f51603 100644 --- 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 index 1faf96e72..feb6ea540 100644 --- 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 index 72c825ce6..5dde6d18d 100644 --- 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 index 530ef31d8..2b56e5ed9 100644 --- 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 index 55ba3ad3c..cfb3382fc 100644 --- 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 index f1b148757..c0ea14a1a 160000 --- a/vendor/nim-ssz-serialization +++ b/vendor/nim-ssz-serialization @@ -1 +1 @@ -Subproject commit f1b14875792df7b1e76c98c9ee669026d7cfe6bb +Subproject commit c0ea14a1acbc92ff9fe646665c52d1a16d81cbfd