grandpa: missing equivocation reporting nits (#5953)

* primitives: move reporting key type to common key types

* session: remove useless methods on MembershipProof

* grandpa: remove std special-casing when checking signatures

* grandpa: add some more docs

* grandpa: use proper error types rather than strings

substrate/bin/node/primitives/src/lib.rs

@@ -69,11 +69,11 @@ pub type BlockId = generic::BlockId<Block>;
 pub mod report {
 	use super::{Signature, Verify};
 	use frame_system::offchain::AppCrypto;
-	use sp_core::crypto::KeyTypeId;
+	use sp_core::crypto::{key_types, KeyTypeId};
 
 	/// Key type for the reporting module. Used for reporting BABE and GRANDPA
 	/// equivocations.
-	pub const KEY_TYPE: KeyTypeId = KeyTypeId(*b"fish");
+	pub const KEY_TYPE: KeyTypeId = key_types::REPORTING;
 
 	mod app {
 		use sp_application_crypto::{app_crypto, sr25519};

substrate/frame/grandpa/src/equivocation.rs

@@ -29,6 +29,12 @@
 //! And in a runtime context, so that the GRANDPA module can validate the
 //! equivocation proofs in the extrinsic and report the offences.
 //!
+//! IMPORTANT:
+//! When using this module for enabling equivocation reporting it is required
+//! that the `ValidateEquivocationReport` signed extension is used in the runtime
+//! definition. Failure to do so will allow invalid equivocation reports to be
+//! accepted by the runtime.
+//!
 
 use sp_std::prelude::*;
 
@@ -395,12 +401,12 @@ impl GetValidatorCount for frame_support::Void {
 
 impl GetSessionNumber for sp_session::MembershipProof {
 	fn session(&self) -> SessionIndex {
-		self.session()
+		self.session
 	}
 }
 
 impl GetValidatorCount for sp_session::MembershipProof {
 	fn validator_count(&self) -> sp_session::ValidatorCount {
-		self.validator_count()
+		self.validator_count
 	}
 }

substrate/frame/grandpa/src/lib.rs

@@ -184,6 +184,10 @@ decl_error! {
 		ChangePending,
 		/// Cannot signal forced change so soon after last.
 		TooSoon,
+		/// A key ownership proof provided as part of an equivocation report is invalid.
+		InvalidKeyOwnershipProof,
+		/// A given equivocation report is valid but already previously reported.
+		DuplicateOffenceReport,
 	}
 }
 
@@ -228,8 +232,9 @@ decl_module! {
 		/// against the extracted offender. If both are valid, the offence
 		/// will be reported.
 		///
-		/// Since the weight is 0 in order to avoid DoS pre-validation is implemented in a
-		/// `SignedExtension`.
+		/// Since the weight of the extrinsic is 0, in order to avoid DoS by
+		/// submission of invalid equivocation reports, a mandatory pre-validation of
+		/// the extrinsic is implemented in a `SignedExtension`.
 		#[weight = 0]
 		fn report_equivocation(
 			origin,
@@ -249,7 +254,7 @@ decl_module! {
 				T::KeyOwnerProofSystem::check_proof(
 					(fg_primitives::KEY_TYPE, equivocation_proof.offender().clone()),
 					key_owner_proof,
-				).ok_or("Invalid key ownership proof.")?;
+				).ok_or(Error::<T>::InvalidKeyOwnershipProof)?;
 
 			// the set id and round when the offence happened
 			let set_id = equivocation_proof.set_id();
@@ -265,7 +270,7 @@ decl_module! {
 					set_id,
 					round,
 				),
-			).map_err(|_| "Duplicate offence report.")?;
+			).map_err(|_| Error::<T>::DuplicateOffenceReport)?;
 		}
 
 		fn on_finalize(block_number: T::BlockNumber) {
@@ -440,9 +445,9 @@ impl<T: Trait> Module<T> {
 			Self::set_grandpa_authorities(authorities);
 		}
 
-		// NOTE: initialize first session of first set. this is necessary
-		// because we only update this `on_new_session` which isn't called
-		// for the genesis session.
+		// NOTE: initialize first session of first set. this is necessary for
+		// the genesis set and session since we only update the set -> session
+		// mapping whenever a new session starts, i.e. through `on_new_session`.
 		SetIdSession::insert(0, 0);
 	}
 
@@ -454,10 +459,7 @@ impl<T: Trait> Module<T> {
 		equivocation_proof: EquivocationProof<T::Hash, T::BlockNumber>,
 		key_owner_proof: T::KeyOwnerProof,
 	) -> Option<()> {
-		T::HandleEquivocation::submit_equivocation_report(equivocation_proof, key_owner_proof)
-			.ok()?;
-
-		Some(())
+		T::HandleEquivocation::submit_equivocation_report(equivocation_proof, key_owner_proof).ok()
 	}
 }
 

substrate/primitives/core/src/crypto.rs

@@ -1006,6 +1006,8 @@ pub mod key_types {
 	pub const AUTHORITY_DISCOVERY: KeyTypeId = KeyTypeId(*b"audi");
 	/// Key type for staking, built-in. Identified as `stak`.
 	pub const STAKING: KeyTypeId = KeyTypeId(*b"stak");
+	/// Key type for equivocation reporting, built-in. Identified as `fish`.
+	pub const REPORTING: KeyTypeId = KeyTypeId(*b"fish");
 	/// A key type ID useful for tests.
 	pub const DUMMY: KeyTypeId = KeyTypeId(*b"dumy");
 }

substrate/primitives/finality-grandpa/src/lib.rs

@@ -352,21 +352,11 @@ where
 	H: Encode,
 	N: Encode,
 {
+	use sp_application_crypto::RuntimeAppPublic;
+
 	localized_payload_with_buffer(round, set_id, message, buf);
 
-	#[cfg(not(feature = "std"))]
-	let verify = || {
-		use sp_application_crypto::RuntimeAppPublic;
-		id.verify(&buf, signature)
-	};
-
-	#[cfg(feature = "std")]
-	let verify = || {
-		use sp_application_crypto::Pair;
-		AuthorityPair::verify(signature, &buf, &id)
-	};
-
-	if verify() {
+	if id.verify(&buf, signature) {
 		Ok(())
 	} else {
 		#[cfg(feature = "std")]
@@ -501,7 +491,10 @@ sp_api::decl_runtime_apis! {
 		/// provide the equivocation proof and a key ownership proof (should be
 		/// obtained using `generate_key_ownership_proof`). This method will
 		/// sign the extrinsic with any reporting keys available in the keystore
-		/// and will push the transaction to the pool.
+		/// and will push the transaction to the pool. This method returns `None`
+		/// when creation of the extrinsic fails, either due to unavailability
+		/// of keys to sign, or because equivocation reporting is disabled for
+		/// the given runtime (i.e. this method is hardcoded to return `None`).
 		/// Only useful in an offchain context.
 		fn submit_report_equivocation_extrinsic(
 			equivocation_proof: EquivocationProof<Block::Hash, NumberFor<Block>>,

substrate/primitives/session/src/lib.rs

@@ -63,18 +63,6 @@ pub struct MembershipProof {
 	pub validator_count: ValidatorCount,
 }
 
-impl MembershipProof {
-	/// Returns a session this proof was generated for.
-	pub fn session(&self) -> SessionIndex {
-		self.session
-	}
-
-	/// Returns the validator count of the session this proof was generated for.
-	pub fn validator_count(&self) -> ValidatorCount {
-		self.validator_count
-	}
-}
-
 /// Generate the initial session keys with the given seeds, at the given block and store them in
 /// the client's keystore.
 #[cfg(feature = "std")]