beefy: simplify equivocations and fix tests

client/beefy/src/keystore.rs

@@ -19,7 +19,6 @@
 use sp_application_crypto::RuntimeAppPublic;
 use sp_core::keccak_256;
 use sp_keystore::{SyncCryptoStore, SyncCryptoStorePtr};
-use sp_runtime::traits::Keccak256;
 
 use log::warn;
 
@@ -30,6 +29,9 @@ use beefy_primitives::{
 
 use crate::error;
 
+/// Hasher used for BEEFY signatures.
+pub(crate) type BeefySignatureHasher = sp_runtime::traits::Keccak256;
+
 /// A BEEFY specific keystore implemented as a `Newtype`. This is basically a
 /// wrapper around [`sp_keystore::SyncCryptoStore`] and allows to customize
 /// common cryptographic functionality.
@@ -99,7 +101,7 @@ impl BeefyKeystore {
 	///
 	/// Return `true` if the signature is authentic, `false` otherwise.
 	pub fn verify(public: &Public, sig: &Signature, message: &[u8]) -> bool {
-		BeefyAuthorityId::<Keccak256>::verify(public, sig, message)
+		BeefyAuthorityId::<BeefySignatureHasher>::verify(public, sig, message)
 	}
 }
 

client/beefy/src/round.rs

@@ -18,8 +18,7 @@
 
 use beefy_primitives::{
 	crypto::{AuthorityId, Public, Signature},
-	Commitment, Equivocation, EquivocationProof, SignedCommitment, ValidatorSet, ValidatorSetId,
-	VoteMessage,
+	Commitment, EquivocationProof, SignedCommitment, ValidatorSet, ValidatorSetId, VoteMessage,
 };
 use codec::{Decode, Encode};
 use log::debug;
@@ -164,14 +163,10 @@ where
 					target: "beefy", "🥩 detected equivocated vote: 1st: {:?}, 2nd: {:?}",
 					existing_vote, vote
 				);
-				let equivocation = Equivocation {
-					round_number: equivocation_key.1,
-					id: equivocation_key.0,
+				return VoteImportResult::Equivocation(EquivocationProof {
 					first: existing_vote.clone(),
 					second: vote,
-				};
-				let set_id = self.validator_set_id();
-				return VoteImportResult::Equivocation(EquivocationProof { set_id, equivocation })
+				})
 			}
 		} else {
 			// this is the first vote sent by `id` for `num`, all good
@@ -218,7 +213,7 @@ mod tests {
 	use sc_network_test::Block;
 
 	use beefy_primitives::{
-		crypto::Public, keyring::Keyring, known_payloads::MMR_ROOT_ID, Commitment, Equivocation,
+		crypto::Public, keyring::Keyring, known_payloads::MMR_ROOT_ID, Commitment,
 		EquivocationProof, Payload, SignedCommitment, ValidatorSet, VoteMessage,
 	};
 
@@ -501,13 +496,8 @@ mod tests {
 		alice_vote2.commitment = commitment2;
 
 		let expected_result = VoteImportResult::Equivocation(EquivocationProof {
-			set_id: validator_set_id,
-			equivocation: Equivocation {
-				id: Keyring::Alice.public(),
-				round_number: 1,
-				first: alice_vote1.clone(),
-				second: alice_vote2.clone(),
-			},
+			first: alice_vote1.clone(),
+			second: alice_vote2.clone(),
 		});
 
 		// vote on one payload - ok

client/beefy/src/worker.rs

@@ -23,13 +23,14 @@ use crate::{
 	},
 	error::Error,
 	justification::BeefyVersionedFinalityProof,
-	keystore::BeefyKeystore,
+	keystore::{BeefyKeystore, BeefySignatureHasher},
 	metric_inc, metric_set,
 	metrics::Metrics,
 	round::{Rounds, VoteImportResult},
 	BeefyVoterLinks,
 };
 use beefy_primitives::{
+	check_equivocation_proof,
 	crypto::{AuthorityId, Signature},
 	BeefyApi, Commitment, ConsensusLog, EquivocationProof, PayloadProvider, ValidatorSet,
 	VersionedFinalityProof, VoteMessage, BEEFY_ENGINE_ID,
@@ -914,18 +915,22 @@ where
 		let rounds =
 			self.persisted_state.voting_oracle.active_rounds().ok_or(Error::UninitSession)?;
 		let (validators, validator_set_id) = (rounds.validators(), rounds.validator_set_id());
+		let offender_id = proof.offender_id().clone();
 
-		if proof.set_id != validator_set_id {
-			debug!(target: "beefy", "🥩 Skip equivocation report for old set id: {:?}", proof.set_id);
+		if proof.set_id() != validator_set_id {
+			debug!(target: "beefy", "🥩 Skip equivocation report for old set id: {:?}", proof.set_id());
+			return Ok(())
+		} else if !check_equivocation_proof::<_, _, BeefySignatureHasher>(&proof) {
+			debug!(target: "beefy", "🥩 Skip report for bad equivocation {:?}", proof);
 			return Ok(())
 		} else if let Some(local_id) = self.key_store.authority_id(validators) {
-			if proof.equivocation.id == local_id {
+			if offender_id == local_id {
 				debug!(target: "beefy", "🥩 Skip equivocation report for own equivocation");
 				return Ok(())
 			}
 		}
 
-		let number = proof.equivocation.round_number;
+		let number = *proof.round_number();
 		let hash = self
 			.backend
 			.blockchain()
@@ -942,11 +947,7 @@ where
 		let runtime_api = self.runtime.runtime_api();
 		// generate key ownership proof at that block
 		let key_owner_proof = match runtime_api
-			.generate_key_ownership_proof(
-				&BlockId::Hash(hash),
-				validator_set_id,
-				proof.equivocation.id.clone(),
-			)
+			.generate_key_ownership_proof(&BlockId::Hash(hash), validator_set_id, offender_id)
 			.map_err(Error::RuntimeApi)?
 		{
 			Some(proof) => proof,

frame/beefy/src/equivocation.rs

@@ -239,9 +239,9 @@ impl<T: Config> Pallet<T> {
 				.priority(TransactionPriority::MAX)
 				// Only one equivocation report for the same offender at the same slot.
 				.and_provides((
-					equivocation_proof.equivocation.id.clone(),
-					equivocation_proof.set_id,
-					equivocation_proof.equivocation.round_number,
+					equivocation_proof.offender_id().clone(),
+					equivocation_proof.set_id(),
+					*equivocation_proof.round_number(),
 				))
 				.longevity(longevity)
 				// We don't propagate this. This can never be included on a remote node.
@@ -269,17 +269,18 @@ fn is_known_offence<T: Config>(
 	>,
 	key_owner_proof: &T::KeyOwnerProof,
 ) -> Result<(), TransactionValidityError> {
-	// check the membership proof to extract the offender's id
-	let key = (beefy_primitives::KEY_TYPE, equivocation_proof.equivocation.id.clone());
+	// check the membership proof to extract the offender's id,
+	// equivocation validity will be fully checked during the call.
+	let key = (beefy_primitives::KEY_TYPE, equivocation_proof.offender_id().clone());
 
 	let offender = T::KeyOwnerProofSystem::check_proof(key, key_owner_proof.clone())
 		.ok_or(InvalidTransaction::BadProof)?;
 
 	// check if the offence has already been reported,
 	// and if so then we can discard the report.
 	let time_slot = <T::HandleEquivocation as HandleEquivocation<T>>::Offence::new_time_slot(
-		equivocation_proof.set_id,
-		equivocation_proof.equivocation.round_number,
+		equivocation_proof.set_id(),
+		*equivocation_proof.round_number(),
 	);
 
 	let is_known_offence = T::HandleEquivocation::is_known_offence(&[offender], &time_slot);

frame/beefy/src/lib.rs

@@ -347,9 +347,9 @@ impl<T: Config> Pallet<T> {
 		// associated session) and round. We also need to know the validator
 		// set count at the time of the offence since it is required to calculate
 		// the slash amount.
-		let set_id = equivocation_proof.set_id;
-		let round = equivocation_proof.equivocation.round_number;
-		let offender_id = equivocation_proof.equivocation.id.clone();
+		let set_id = equivocation_proof.set_id();
+		let round = *equivocation_proof.round_number();
+		let offender_id = equivocation_proof.offender_id().clone();
 		let session_index = key_owner_proof.session();
 		let validator_count = key_owner_proof.validator_count();
 
@@ -361,7 +361,7 @@ impl<T: Config> Pallet<T> {
 		.ok_or(Error::<T>::InvalidKeyOwnershipProof)?;
 
 		// validate equivocation proof (check votes are different and signatures are valid).
-		if !beefy_primitives::check_equivocation_proof(equivocation_proof) {
+		if !beefy_primitives::check_equivocation_proof(&equivocation_proof) {
 			return Err(Error::<T>::InvalidEquivocationProof.into())
 		}
 

frame/beefy/src/mock.rs

@@ -18,8 +18,7 @@
 use std::vec;
 
 use beefy_primitives::{
-	keyring::Keyring as BeefyKeyring, Commitment, Equivocation, Payload, ValidatorSetId,
-	VoteMessage,
+	keyring::Keyring as BeefyKeyring, Commitment, Payload, ValidatorSetId, VoteMessage,
 };
 use codec::Encode;
 use frame_election_provider_support::{onchain, SequentialPhragmen};
@@ -346,28 +345,18 @@ pub fn start_era(era_index: EraIndex) {
 }
 
 pub fn generate_equivocation_proof(
-	validator_set_id: ValidatorSetId,
-	reporter_public: BeefyId,
-	vote1: (u64, Payload, &BeefyKeyring),
-	vote2: (u64, Payload, &BeefyKeyring),
+	vote1: (u64, Payload, ValidatorSetId, &BeefyKeyring),
+	vote2: (u64, Payload, ValidatorSetId, &BeefyKeyring),
 ) -> EquivocationProof<u64, BeefyId, BeefySignature> {
-	let block_number = vote1.0;
-
-	let signed_vote = |block_number: u64, payload: Payload, keyring: &BeefyKeyring| {
+	let signed_vote = |block_number: u64,
+	                   payload: Payload,
+	                   validator_set_id: ValidatorSetId,
+	                   keyring: &BeefyKeyring| {
 		let commitment = Commitment { validator_set_id, block_number, payload };
 		let signature = keyring.sign(&commitment.encode());
 		VoteMessage { commitment, id: keyring.public(), signature }
 	};
-
-	let first = signed_vote(vote1.0, vote1.1, vote1.2);
-	let second = signed_vote(vote2.0, vote2.1, vote2.2);
-	EquivocationProof {
-		set_id: validator_set_id,
-		equivocation: Equivocation {
-			round_number: block_number,
-			id: reporter_public,
-			first,
-			second,
-		},
-	}
+	let first = signed_vote(vote1.0, vote1.1, vote1.2, vote1.3);
+	let second = signed_vote(vote2.0, vote2.1, vote2.2, vote2.3);
+	EquivocationProof { first, second }
 }