grandpa: report equivocations with unsigned extrinsics (#6656)

* grandpa: use unsigned extrinsics for equivocation reporting

* grandpa: allow signed reports as well

* grandpa: change runtime api for submitting unsigned extrinsics

* grandpa: fix tests

* grandpa: add test for unsigned validation

* grandpa: add benchmark for equivocation proof checking

* offences: fix grandpa benchmark

* grandpa: add proper weight for equivocation reporting extrinsic

* grandpa: fix weight unit

substrate/frame/grandpa/Cargo.toml

@@ -21,12 +21,15 @@ sp-session = { version = "2.0.0-rc4", default-features = false, path = "../../pr
 sp-std = { version = "2.0.0-rc4", default-features = false, path = "../../primitives/std" }
 sp-runtime = { version = "2.0.0-rc4", default-features = false, path = "../../primitives/runtime" }
 sp-staking = { version = "2.0.0-rc4", default-features = false, path = "../../primitives/staking" }
+frame-benchmarking = { version = "2.0.0-rc4", default-features = false, path = "../benchmarking", optional = true }
 frame-support = { version = "2.0.0-rc4", default-features = false, path = "../support" }
 frame-system = { version = "2.0.0-rc4", default-features = false, path = "../system" }
+pallet-authorship = { version = "2.0.0-rc4", default-features = false, path = "../authorship" }
 pallet-session = { version = "2.0.0-rc4", default-features = false, path = "../session" }
 pallet-finality-tracker = { version = "2.0.0-rc4", default-features = false, path = "../finality-tracker" }
 
 [dev-dependencies]
+frame-benchmarking = { version = "2.0.0-rc4", path = "../benchmarking" }
 grandpa = { package = "finality-grandpa", version = "0.12.3", features = ["derive-codec"] }
 sp-io = { version = "2.0.0-rc4", path = "../../primitives/io" }
 sp-keyring = { version = "2.0.0-rc4", path = "../../primitives/keyring" }
@@ -41,6 +44,7 @@ default = ["std"]
 std = [
 	"serde",
 	"codec/std",
+	"frame-benchmarking/std",
 	"sp-application-crypto/std",
 	"sp-core/std",
 	"sp-finality-grandpa/std",
@@ -50,6 +54,8 @@ std = [
 	"sp-runtime/std",
 	"sp-staking/std",
 	"frame-system/std",
+	"pallet-authorship/std",
 	"pallet-session/std",
 	"pallet-finality-tracker/std",
 ]
+runtime-benchmarks = ["frame-benchmarking"]

substrate/frame/grandpa/src/benchmarking.rs

@@ -0,0 +1,106 @@
+// This file is part of Substrate.
+
+// Copyright (C) 2020 Parity Technologies (UK) Ltd.
+// SPDX-License-Identifier: Apache-2.0
+
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// 	http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//! Benchmarks for the GRANDPA pallet.
+
+#![cfg_attr(not(feature = "std"), no_std)]
+
+use super::*;
+use frame_benchmarking::benchmarks;
+use sp_core::H256;
+
+benchmarks! {
+	_ {	}
+
+	check_equivocation_proof {
+		let x in 0 .. 1;
+
+		// NOTE: generated with the test below `test_generate_equivocation_report_blob`.
+		// the output should be deterministic since the keys we use are static.
+		// with the current benchmark setup it is not possible to generate this
+		// programatically from the benchmark setup.
+		const EQUIVOCATION_PROOF_BLOB: [u8; 257] = [
+			1, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 136, 220, 52, 23,
+			213, 5, 142, 196, 180, 80, 62, 12, 18, 234, 26, 10, 137, 190, 32,
+			15, 233, 137, 34, 66, 61, 67, 52, 1, 79, 166, 176, 238, 207, 48,
+			195, 55, 171, 225, 252, 130, 161, 56, 151, 29, 193, 32, 25, 157,
+			249, 39, 80, 193, 214, 96, 167, 147, 25, 130, 45, 42, 64, 208, 182,
+			164, 10, 0, 0, 0, 0, 0, 0, 0, 234, 236, 231, 45, 70, 171, 135, 246,
+			136, 153, 38, 167, 91, 134, 150, 242, 215, 83, 56, 238, 16, 119, 55,
+			170, 32, 69, 255, 248, 164, 20, 57, 50, 122, 115, 135, 96, 80, 203,
+			131, 232, 73, 23, 149, 86, 174, 59, 193, 92, 121, 76, 154, 211, 44,
+			96, 10, 84, 159, 133, 211, 56, 103, 0, 59, 2, 96, 20, 69, 2, 32,
+			179, 16, 184, 108, 76, 215, 64, 195, 78, 143, 73, 177, 139, 20, 144,
+			98, 231, 41, 117, 255, 220, 115, 41, 59, 27, 75, 56, 10, 0, 0, 0, 0,
+			0, 0, 0, 128, 179, 250, 48, 211, 76, 10, 70, 74, 230, 219, 139, 96,
+			78, 88, 112, 33, 170, 44, 184, 59, 200, 155, 143, 128, 40, 222, 179,
+			210, 190, 84, 16, 182, 21, 34, 94, 28, 193, 163, 226, 51, 251, 134,
+			233, 187, 121, 63, 157, 240, 165, 203, 92, 16, 146, 120, 190, 229,
+			251, 129, 29, 45, 32, 29, 6
+		];
+
+		let equivocation_proof1: sp_finality_grandpa::EquivocationProof<H256, u64> =
+			Decode::decode(&mut &EQUIVOCATION_PROOF_BLOB[..]).unwrap();
+
+		let equivocation_proof2 = equivocation_proof1.clone();
+	}: {
+		sp_finality_grandpa::check_equivocation_proof(equivocation_proof1);
+	} verify {
+		assert!(sp_finality_grandpa::check_equivocation_proof(equivocation_proof2));
+	}
+}
+
+#[cfg(test)]
+mod tests {
+	use super::*;
+	use crate::mock::*;
+	use frame_support::assert_ok;
+
+	#[test]
+	fn test_benchmarks() {
+		new_test_ext(vec![(1, 1), (2, 1), (3, 1)]).execute_with(|| {
+			assert_ok!(test_benchmark_check_equivocation_proof::<Test>());
+		})
+	}
+
+	#[test]
+	fn test_generate_equivocation_report_blob() {
+		let authorities = crate::tests::test_authorities();
+
+		let equivocation_authority_index = 0;
+		let equivocation_key = &authorities[equivocation_authority_index].0;
+		let equivocation_keyring = extract_keyring(equivocation_key);
+
+		new_test_ext_raw_authorities(authorities).execute_with(|| {
+			start_era(1);
+
+			// generate an equivocation proof, with two votes in the same round for
+			// different block hashes signed by the same key
+			let equivocation_proof = generate_equivocation_proof(
+				1,
+				(1, H256::random(), 10, &equivocation_keyring),
+				(1, H256::random(), 10, &equivocation_keyring),
+			);
+
+			println!("equivocation_proof: {:?}", equivocation_proof);
+			println!(
+				"equivocation_proof.encode(): {:?}",
+				equivocation_proof.encode()
+			);
+		});
+	}
+}

substrate/frame/grandpa/src/equivocation.rs

@@ -24,6 +24,7 @@
 //! part of a session);
 //! - a system for reporting offences;
 //! - a system for signing and submitting transactions;
+//! - a way to get the current block author;
 //!
 //! These can be used in an offchain context in order to submit equivocation
 //! reporting extrinsics (from the client that's running the GRANDPA protocol).
@@ -32,165 +33,34 @@
 //!
 //! 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.
+//! that the `ValidateUnsigned` for the GRANDPA pallet is used in the runtime
+//! definition.
 //!
 
 use sp_std::prelude::*;
 
 use codec::{self as codec, Decode, Encode};
-use frame_support::{debug, dispatch::IsSubType, traits::KeyOwnerProofSystem};
-use frame_system::offchain::{AppCrypto, CreateSignedTransaction, Signer};
+use frame_support::{debug, traits::KeyOwnerProofSystem};
 use sp_finality_grandpa::{EquivocationProof, RoundNumber, SetId};
 use sp_runtime::{
-	traits::{DispatchInfoOf, SignedExtension},
 	transaction_validity::{
-		InvalidTransaction, TransactionValidity, TransactionValidityError, ValidTransaction,
+		InvalidTransaction, TransactionPriority, TransactionSource, TransactionValidity,
+		TransactionValidityError, ValidTransaction,
 	},
 	DispatchResult, Perbill,
 };
-use sp_session::GetSessionNumber;
 use sp_staking::{
 	offence::{Kind, Offence, OffenceError, ReportOffence},
 	SessionIndex,
 };
 
-/// Ensure that equivocation reports are only processed if valid.
-#[derive(Encode, Decode, Clone, Eq, PartialEq)]
-pub struct ValidateEquivocationReport<T>(sp_std::marker::PhantomData<T>);
-
-impl<T> Default for ValidateEquivocationReport<T> {
-	fn default() -> ValidateEquivocationReport<T> {
-		ValidateEquivocationReport::new()
-	}
-}
-
-impl<T> ValidateEquivocationReport<T> {
-	pub fn new() -> ValidateEquivocationReport<T> {
-		ValidateEquivocationReport(Default::default())
-	}
-}
-
-impl<T> sp_std::fmt::Debug for ValidateEquivocationReport<T> {
-	fn fmt(&self, f: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result {
-		write!(f, "ValidateEquivocationReport<T>")
-	}
-}
-
-/// Custom validity error used when validating equivocation reports.
-#[derive(Debug)]
-#[repr(u8)]
-pub enum ReportEquivocationValidityError {
-	/// The proof provided in the report is not valid.
-	InvalidEquivocationProof = 1,
-	/// The proof provided in the report is not valid.
-	InvalidKeyOwnershipProof = 2,
-	/// The set id provided in the report is not valid.
-	InvalidSetId = 3,
-	/// The session index provided in the report is not valid.
-	InvalidSession = 4,
-}
-
-impl From<ReportEquivocationValidityError> for TransactionValidityError {
-	fn from(e: ReportEquivocationValidityError) -> TransactionValidityError {
-		TransactionValidityError::from(InvalidTransaction::Custom(e as u8))
-	}
-}
-
-impl<T: super::Trait + Send + Sync> SignedExtension for ValidateEquivocationReport<T>
-where
-	<T as frame_system::Trait>::Call: IsSubType<super::Call<T>>,
-{
-	const IDENTIFIER: &'static str = "ValidateEquivocationReport";
-	type AccountId = T::AccountId;
-	type Call = <T as frame_system::Trait>::Call;
-	type AdditionalSigned = ();
-	type Pre = ();
-
-	fn additional_signed(
-		&self,
-	) -> sp_std::result::Result<Self::AdditionalSigned, TransactionValidityError> {
-		Ok(())
-	}
-
-	fn validate(
-		&self,
-		_who: &Self::AccountId,
-		call: &Self::Call,
-		_info: &DispatchInfoOf<Self::Call>,
-		_len: usize,
-	) -> TransactionValidity {
-		let (equivocation_proof, key_owner_proof) = match call.is_sub_type() {
-			Some(super::Call::report_equivocation(equivocation_proof, key_owner_proof)) => {
-				(equivocation_proof, key_owner_proof)
-			}
-			_ => return Ok(ValidTransaction::default()),
-		};
-
-		// validate the key ownership proof extracting the id of the offender.
-		if let None = T::KeyOwnerProofSystem::check_proof(
-			(
-				sp_finality_grandpa::KEY_TYPE,
-				equivocation_proof.offender().clone(),
-			),
-			key_owner_proof.clone(),
-		) {
-			return Err(ReportEquivocationValidityError::InvalidKeyOwnershipProof.into());
-		}
-
-		// we check the equivocation within the context of its set id (and
-		// associated session).
-		let set_id = equivocation_proof.set_id();
-		let session_index = key_owner_proof.session();
-
-		// validate equivocation proof (check votes are different and
-		// signatures are valid).
-		if !sp_finality_grandpa::check_equivocation_proof(equivocation_proof.clone()) {
-			return Err(ReportEquivocationValidityError::InvalidEquivocationProof.into());
-		}
-
-		// fetch the current and previous sets last session index. on the
-		// genesis set there's no previous set.
-		let previous_set_id_session_index = if set_id == 0 {
-			None
-		} else {
-			let session_index =
-				if let Some(session_id) = <super::Module<T>>::session_for_set(set_id - 1) {
-					session_id
-				} else {
-					return Err(ReportEquivocationValidityError::InvalidSetId.into());
-				};
-
-			Some(session_index)
-		};
-
-		let set_id_session_index =
-			if let Some(session_id) = <super::Module<T>>::session_for_set(set_id) {
-				session_id
-			} else {
-				return Err(ReportEquivocationValidityError::InvalidSetId.into());
-			};
-
-		// check that the session id for the membership proof is within the
-		// bounds of the set id reported in the equivocation.
-		if session_index > set_id_session_index ||
-			previous_set_id_session_index
-				.map(|previous_index| session_index <= previous_index)
-				.unwrap_or(false)
-		{
-			return Err(ReportEquivocationValidityError::InvalidSession.into());
-		}
-
-		Ok(ValidTransaction::default())
-	}
-}
+use super::{Call, Module, Trait};
 
 /// A trait with utility methods for handling equivocation reports in GRANDPA.
 /// The offence type is generic, and the trait provides , reporting an offence
 /// triggered by a valid equivocation report, and also for creating and
 /// submitting equivocation report extrinsics (useful only in offchain context).
-pub trait HandleEquivocation<T: super::Trait> {
+pub trait HandleEquivocation<T: Trait> {
 	/// The offence type used for reporting offences on valid equivocation reports.
 	type Offence: GrandpaOffence<T::KeyOwnerIdentification>;
 
@@ -200,14 +70,23 @@ pub trait HandleEquivocation<T: super::Trait> {
 		offence: Self::Offence,
 	) -> Result<(), OffenceError>;
 
+	/// Returns true if all of the offenders at the given time slot have already been reported.
+	fn is_known_offence(
+		offenders: &[T::KeyOwnerIdentification],
+		time_slot: &<Self::Offence as Offence<T::KeyOwnerIdentification>>::TimeSlot,
+	) -> bool;
+
 	/// Create and dispatch an equivocation report extrinsic.
-	fn submit_equivocation_report(
+	fn submit_unsigned_equivocation_report(
 		equivocation_proof: EquivocationProof<T::Hash, T::BlockNumber>,
 		key_owner_proof: T::KeyOwnerProof,
 	) -> DispatchResult;
+
+	/// Fetch the current block author id, if defined.
+	fn block_author() -> Option<T::AccountId>;
 }
 
-impl<T: super::Trait> HandleEquivocation<T> for () {
+impl<T: Trait> HandleEquivocation<T> for () {
 	type Offence = GrandpaEquivocationOffence<T::KeyOwnerIdentification>;
 
 	fn report_offence(
@@ -217,23 +96,34 @@ impl<T: super::Trait> HandleEquivocation<T> for () {
 		Ok(())
 	}
 
-	fn submit_equivocation_report(
+	fn is_known_offence(
+		_offenders: &[T::KeyOwnerIdentification],
+		_time_slot: &GrandpaTimeSlot,
+	) -> bool {
+		true
+	}
+
+	fn submit_unsigned_equivocation_report(
 		_equivocation_proof: EquivocationProof<T::Hash, T::BlockNumber>,
 		_key_owner_proof: T::KeyOwnerProof,
 	) -> DispatchResult {
 		Ok(())
 	}
+
+	fn block_author() -> Option<T::AccountId> {
+		None
+	}
 }
 
 /// Generic equivocation handler. This type implements `HandleEquivocation`
 /// using existing subsystems that are part of frame (type bounds described
 /// below) and will dispatch to them directly, it's only purpose is to wire all
 /// subsystems together.
-pub struct EquivocationHandler<I, C, S, R, O = GrandpaEquivocationOffence<I>> {
-	_phantom: sp_std::marker::PhantomData<(I, C, S, R, O)>,
+pub struct EquivocationHandler<I, R, O = GrandpaEquivocationOffence<I>> {
+	_phantom: sp_std::marker::PhantomData<(I, R, O)>,
 }
 
-impl<I, C, S, R, O> Default for EquivocationHandler<I, C, S, R, O> {
+impl<I, R, O> Default for EquivocationHandler<I, R, O> {
 	fn default() -> Self {
 		Self {
 			_phantom: Default::default(),
@@ -241,18 +131,17 @@ impl<I, C, S, R, O> Default for EquivocationHandler<I, C, S, R, O> {
 	}
 }
 
-impl<T, C, S, R, O> HandleEquivocation<T>
-	for EquivocationHandler<T::KeyOwnerIdentification, C, S, R, O>
+impl<T, R, O> HandleEquivocation<T> for EquivocationHandler<T::KeyOwnerIdentification, R, O>
 where
-	// A signed transaction creator. Used for signing and submitting equivocation reports.
-	T: super::Trait + CreateSignedTransaction<super::Call<T>>,
-	// Application-specific crypto bindings.
-	C: AppCrypto<T::Public, T::Signature>,
-	// The offence type that should be used when reporting.
-	O: GrandpaOffence<T::KeyOwnerIdentification>,
+	// We use the authorship pallet to fetch the current block author and use
+	// `offchain::SendTransactionTypes` for unsigned extrinsic creation and
+	// submission.
+	T: Trait + pallet_authorship::Trait + frame_system::offchain::SendTransactionTypes<Call<T>>,
 	// A system for reporting offences after valid equivocation reports are
 	// processed.
 	R: ReportOffence<T::AccountId, T::KeyOwnerIdentification, O>,
+	// The offence type that should be used when reporting.
+	O: GrandpaOffence<T::KeyOwnerIdentification>,
 {
 	type Offence = O;
 
@@ -260,36 +149,29 @@ where
 		R::report_offence(reporters, offence)
 	}
 
-	fn submit_equivocation_report(
+	fn is_known_offence(offenders: &[T::KeyOwnerIdentification], time_slot: &O::TimeSlot) -> bool {
+		R::is_known_offence(offenders, time_slot)
+	}
+
+	fn submit_unsigned_equivocation_report(
 		equivocation_proof: EquivocationProof<T::Hash, T::BlockNumber>,
 		key_owner_proof: T::KeyOwnerProof,
 	) -> DispatchResult {
-		use frame_system::offchain::SendSignedTransaction;
+		use frame_system::offchain::SubmitTransaction;
 
-		let signer = Signer::<T, C>::all_accounts();
-		if !signer.can_sign() {
-			return Err(
-				"No local accounts available. Consider adding one via `author_insertKey` RPC.",
-			)?;
-		}
+		let call = Call::report_equivocation_unsigned(equivocation_proof, key_owner_proof);
 
-		let results = signer.send_signed_transaction(|_account| {
-			super::Call::report_equivocation(equivocation_proof.clone(), key_owner_proof.clone())
-		});
-
-		for (acc, res) in &results {
-			match res {
-				Ok(()) => debug::info!("[{:?}] Submitted GRANDPA equivocation report.", acc.id),
-				Err(e) => debug::error!(
-					"[{:?}] Error submitting equivocation report: {:?}",
-					acc.id,
-					e
-				),
-			}
+		match SubmitTransaction::<T, Call<T>>::submit_unsigned_transaction(call.into()) {
+			Ok(()) => debug::info!("Submitted GRANDPA equivocation report."),
+			Err(e) => debug::error!("Error submitting equivocation report: {:?}", e),
 		}
 
 		Ok(())
 	}
+
+	fn block_author() -> Option<T::AccountId> {
+		Some(<pallet_authorship::Module<T>>::author())
+	}
 }
 
 /// A round number and set id which point on the time of an offence.
@@ -302,6 +184,75 @@ pub struct GrandpaTimeSlot {
 	pub round: RoundNumber,
 }
 
+/// A `ValidateUnsigned` implementation that restricts calls to `report_equivocation_unsigned`
+/// to local calls (i.e. extrinsics generated on this node) or that already in a block. This
+/// guarantees that only block authors can include unsigned equivocation reports.
+impl<T: Trait> frame_support::unsigned::ValidateUnsigned for Module<T> {
+	type Call = Call<T>;
+	fn validate_unsigned(source: TransactionSource, call: &Self::Call) -> TransactionValidity {
+		if let Call::report_equivocation_unsigned(equivocation_proof, _) = call {
+			// discard equivocation report not coming from the local node
+			match source {
+				TransactionSource::Local | TransactionSource::InBlock => { /* allowed */ }
+				_ => {
+					debug::warn!(
+						target: "afg",
+						"rejecting unsigned report equivocation transaction because it is not local/in-block."
+					);
+
+					return InvalidTransaction::Call.into();
+				}
+			}
+
+			ValidTransaction::with_tag_prefix("GrandpaEquivocation")
+				// We assign the maximum priority for any equivocation report.
+				.priority(TransactionPriority::max_value())
+				// Only one equivocation report for the same offender at the same slot.
+				.and_provides((
+					equivocation_proof.offender().clone(),
+					equivocation_proof.set_id(),
+					equivocation_proof.round(),
+				))
+				// We don't propagate this. This can never be included on a remote node.
+				.propagate(false)
+				.build()
+		} else {
+			InvalidTransaction::Call.into()
+		}
+	}
+
+	fn pre_dispatch(call: &Self::Call) -> Result<(), TransactionValidityError> {
+		if let Call::report_equivocation_unsigned(equivocation_proof, key_owner_proof) = call {
+			// check the membership proof to extract the offender's id
+			let key = (
+				sp_finality_grandpa::KEY_TYPE,
+				equivocation_proof.offender().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.round(),
+				);
+
+			let is_known_offence = T::HandleEquivocation::is_known_offence(&[offender], &time_slot);
+
+			if is_known_offence {
+				Err(InvalidTransaction::Stale.into())
+			} else {
+				Ok(())
+			}
+		} else {
+			Err(InvalidTransaction::Call.into())
+		}
+	}
+}
+
 /// A grandpa equivocation offence report.
 #[allow(dead_code)]
 pub struct GrandpaEquivocationOffence<FullIdentification> {
@@ -327,6 +278,9 @@ pub trait GrandpaOffence<FullIdentification>: Offence<FullIdentification> {
 		set_id: SetId,
 		round: RoundNumber,
 	) -> Self;
+
+	/// Create a new GRANDPA offence time slot.
+	fn new_time_slot(set_id: SetId, round: RoundNumber) -> Self::TimeSlot;
 }
 
 impl<FullIdentification: Clone> GrandpaOffence<FullIdentification>
@@ -346,6 +300,10 @@ impl<FullIdentification: Clone> GrandpaOffence<FullIdentification>
 			time_slot: GrandpaTimeSlot { set_id, round },
 		}
 	}
+
+	fn new_time_slot(set_id: SetId, round: RoundNumber) -> Self::TimeSlot {
+		GrandpaTimeSlot { set_id, round }
+	}
 }
 
 impl<FullIdentification: Clone> Offence<FullIdentification>

substrate/frame/grandpa/src/lib.rs

@@ -43,7 +43,7 @@ use frame_support::{
 	decl_error, decl_event, decl_module, decl_storage, storage, traits::KeyOwnerProofSystem,
 	Parameter,
 };
-use frame_system::{ensure_signed, DigestOf};
+use frame_system::{ensure_none, ensure_signed, DigestOf};
 use sp_runtime::{
 	generic::{DigestItem, OpaqueDigestItemId},
 	traits::Zero,
@@ -53,6 +53,9 @@ use sp_session::{GetSessionNumber, GetValidatorCount};
 use sp_staking::SessionIndex;
 
 mod equivocation;
+
+#[cfg(any(feature = "runtime-benchmarks", test))]
+mod benchmarking;
 #[cfg(all(feature = "std", test))]
 mod mock;
 #[cfg(all(feature = "std", test))]
@@ -60,7 +63,7 @@ mod tests;
 
 pub use equivocation::{
 	EquivocationHandler, GrandpaEquivocationOffence, GrandpaOffence, GrandpaTimeSlot,
-	HandleEquivocation, ValidateEquivocationReport,
+	HandleEquivocation,
 };
 
 pub trait Trait: frame_system::Trait {
@@ -90,9 +93,8 @@ pub trait Trait: frame_system::Trait {
 	/// offence (after the equivocation has been validated) and for submitting a
 	/// transaction to report an equivocation (from an offchain context).
 	/// NOTE: when enabling equivocation handling (i.e. this type isn't set to
-	/// `()`) you must add the `equivocation::ValidateEquivocationReport` signed
-	/// extension to the runtime's `SignedExtra` definition, otherwise
-	/// equivocation reports won't be properly validated.
+	/// `()`) you must use this pallet's `ValidateUnsigned` in the runtime
+	/// definition.
 	type HandleEquivocation: HandleEquivocation<Self>;
 }
 
@@ -190,6 +192,8 @@ decl_error! {
 		TooSoon,
 		/// A key ownership proof provided as part of an equivocation report is invalid.
 		InvalidKeyOwnershipProof,
+		/// An equivocation proof provided as part of an equivocation report is invalid.
+		InvalidEquivocationProof,
 		/// A given equivocation report is valid but already previously reported.
 		DuplicateOffenceReport,
 	}
@@ -237,46 +241,43 @@ decl_module! {
 		/// equivocation proof and validate the given key ownership proof
 		/// against the extracted offender. If both are valid, the offence
 		/// will be reported.
-		///
-		/// 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]
+		#[weight = weight_for::report_equivocation::<T>(key_owner_proof.validator_count())]
 		fn report_equivocation(
 			origin,
 			equivocation_proof: EquivocationProof<T::Hash, T::BlockNumber>,
 			key_owner_proof: T::KeyOwnerProof,
 		) {
-			let reporter_id = ensure_signed(origin)?;
+			let reporter = ensure_signed(origin)?;
 
-			let (session_index, validator_set_count) = (
-				key_owner_proof.session(),
-				key_owner_proof.validator_count(),
-			);
+			Self::do_report_equivocation(
+				Some(reporter),
+				equivocation_proof,
+				key_owner_proof,
+			)?;
+		}
 
-			// we have already checked this proof in `SignedExtension`, we to
-			// check it again to get the full identification of the offender.
-			let offender =
-				T::KeyOwnerProofSystem::check_proof(
-					(fg_primitives::KEY_TYPE, equivocation_proof.offender().clone()),
-					key_owner_proof,
-				).ok_or(Error::<T>::InvalidKeyOwnershipProof)?;
+		/// Report voter equivocation/misbehavior. This method will verify the
+		/// equivocation proof and validate the given key ownership proof
+		/// against the extracted offender. If both are valid, the offence
+		/// will be reported.
+		///
+		/// This extrinsic must be called unsigned and it is expected that only
+		/// block authors will call it (validated in `ValidateUnsigned`), as such
+		/// if the block author is defined it will be defined as the equivocation
+		/// reporter.
+		#[weight = weight_for::report_equivocation::<T>(key_owner_proof.validator_count())]
+		fn report_equivocation_unsigned(
+			origin,
+			equivocation_proof: EquivocationProof<T::Hash, T::BlockNumber>,
+			key_owner_proof: T::KeyOwnerProof,
+		) {
+			ensure_none(origin)?;
 
-			// the set id and round when the offence happened
-			let set_id = equivocation_proof.set_id();
-			let round = equivocation_proof.round();
-
-			// report to the offences module rewarding the sender.
-			T::HandleEquivocation::report_offence(
-				vec![reporter_id],
-				<T::HandleEquivocation as HandleEquivocation<T>>::Offence::new(
-					session_index,
-					validator_set_count,
-					offender,
-					set_id,
-					round,
-				),
-			).map_err(|_| Error::<T>::DuplicateOffenceReport)?;
+			Self::do_report_equivocation(
+				T::HandleEquivocation::block_author(),
+				equivocation_proof,
+				key_owner_proof,
+			)?;
 		}
 
 		fn on_finalize(block_number: T::BlockNumber) {
@@ -344,6 +345,40 @@ decl_module! {
 	}
 }
 
+mod weight_for {
+	use frame_support::{
+		traits::Get,
+		weights::{
+			constants::{WEIGHT_PER_MICROS, WEIGHT_PER_NANOS},
+			Weight,
+		},
+	};
+
+	pub fn report_equivocation<T: super::Trait>(validator_count: u32) -> Weight {
+		// we take the validator set count from the membership proof to
+		// calculate the weight but we set a floor of 100 validators.
+		let validator_count = validator_count.min(100) as u64;
+
+		// worst case we are considering is that the given offender
+		// is backed by 200 nominators
+		const MAX_NOMINATORS: u64 = 200;
+
+		// checking membership proof
+		(35 * WEIGHT_PER_MICROS)
+			.saturating_add((175 * WEIGHT_PER_NANOS).saturating_mul(validator_count))
+			.saturating_add(T::DbWeight::get().reads(5))
+			// check equivocation proof
+			.saturating_add(95 * WEIGHT_PER_MICROS)
+			// report offence
+			.saturating_add(110 * WEIGHT_PER_MICROS)
+			.saturating_add(25 * WEIGHT_PER_MICROS * MAX_NOMINATORS)
+			.saturating_add(T::DbWeight::get().reads(14 + 3 * MAX_NOMINATORS))
+			.saturating_add(T::DbWeight::get().writes(10 + 3 * MAX_NOMINATORS))
+			// fetching set id -> session index mappings
+			.saturating_add(T::DbWeight::get().reads(2))
+	}
+}
+
 impl<T: Trait> Module<T> {
 	/// Get the current set of authorities, along with their respective weights.
 	pub fn grandpa_authorities() -> AuthorityList {
@@ -457,15 +492,91 @@ impl<T: Trait> Module<T> {
 		SetIdSession::insert(0, 0);
 	}
 
-	/// Submits an extrinsic to report an equivocation. This method will sign an
-	/// extrinsic with a call to `report_equivocation` with any reporting keys
-	/// available in the keystore and will push the transaction to the pool.
-	/// Only useful in an offchain context.
-	pub fn submit_report_equivocation_extrinsic(
+	fn do_report_equivocation(
+		reporter: Option<T::AccountId>,
+		equivocation_proof: EquivocationProof<T::Hash, T::BlockNumber>,
+		key_owner_proof: T::KeyOwnerProof,
+	) -> Result<(), Error<T>> {
+		// we check the equivocation within the context of its set id (and
+		// associated session) and round. we also need to know the validator
+		// set count when the offence since it is required to calculate the
+		// slash amount.
+		let set_id = equivocation_proof.set_id();
+		let round = equivocation_proof.round();
+		let session_index = key_owner_proof.session();
+		let validator_count = key_owner_proof.validator_count();
+
+		// validate the key ownership proof extracting the id of the offender.
+		let offender =
+			T::KeyOwnerProofSystem::check_proof(
+				(fg_primitives::KEY_TYPE, equivocation_proof.offender().clone()),
+				key_owner_proof,
+			).ok_or(Error::<T>::InvalidKeyOwnershipProof)?;
+
+		// validate equivocation proof (check votes are different and
+		// signatures are valid).
+		if !sp_finality_grandpa::check_equivocation_proof(equivocation_proof) {
+			return Err(Error::<T>::InvalidEquivocationProof.into());
+		}
+
+		// fetch the current and previous sets last session index. on the
+		// genesis set there's no previous set.
+		let previous_set_id_session_index = if set_id == 0 {
+			None
+		} else {
+			let session_index =
+				if let Some(session_id) = Self::session_for_set(set_id - 1) {
+					session_id
+				} else {
+					return Err(Error::<T>::InvalidEquivocationProof.into());
+				};
+
+			Some(session_index)
+		};
+
+		let set_id_session_index =
+			if let Some(session_id) = Self::session_for_set(set_id) {
+				session_id
+			} else {
+				return Err(Error::<T>::InvalidEquivocationProof.into());
+			};
+
+		// check that the session id for the membership proof is within the
+		// bounds of the set id reported in the equivocation.
+		if session_index > set_id_session_index ||
+			previous_set_id_session_index
+			.map(|previous_index| session_index <= previous_index)
+			.unwrap_or(false)
+		{
+			return Err(Error::<T>::InvalidEquivocationProof.into());
+		}
+
+		// report to the offences module rewarding the sender.
+		T::HandleEquivocation::report_offence(
+			reporter.into_iter().collect(),
+			<T::HandleEquivocation as HandleEquivocation<T>>::Offence::new(
+				session_index,
+				validator_count,
+				offender,
+				set_id,
+				round,
+			),
+		).map_err(|_| Error::<T>::DuplicateOffenceReport)
+	}
+
+	/// Submits an extrinsic to report an equivocation. This method will create
+	/// an unsigned extrinsic with a call to `report_equivocation_unsigned` and
+	/// will push the transaction to the pool. Only useful in an offchain
+	/// context.
+	pub fn submit_unsigned_equivocation_report(
 		equivocation_proof: EquivocationProof<T::Hash, T::BlockNumber>,
 		key_owner_proof: T::KeyOwnerProof,
 	) -> Option<()> {
-		T::HandleEquivocation::submit_equivocation_report(equivocation_proof, key_owner_proof).ok()
+		T::HandleEquivocation::submit_unsigned_equivocation_report(
+			equivocation_proof,
+			key_owner_proof,
+		)
+		.ok()
 	}
 }
 

substrate/frame/grandpa/src/mock.rs

@@ -19,16 +19,13 @@
 
 #![cfg(test)]
 
-use crate::{
-	equivocation::ValidateEquivocationReport, AuthorityId, AuthorityList, Call as GrandpaCall,
-	ConsensusLog, Module, Trait,
-};
+use crate::{AuthorityId, AuthorityList, ConsensusLog, Module, Trait};
 use ::grandpa as finality_grandpa;
 use codec::Encode;
 use frame_support::{
 	impl_outer_dispatch, impl_outer_event, impl_outer_origin, parameter_types,
 	traits::{KeyOwnerProofSystem, OnFinalize, OnInitialize},
-	weights::{DispatchInfo, Weight},
+	weights::Weight,
 };
 use pallet_staking::EraIndex;
 use sp_core::{crypto::KeyTypeId, H256};
@@ -39,11 +36,7 @@ use sp_runtime::{
 	curve::PiecewiseLinear,
 	impl_opaque_keys,
 	testing::{Header, TestXt, UintAuthorityId},
-	traits::{
-		Convert, Extrinsic as ExtrinsicT, IdentityLookup, OpaqueKeys, SaturatedConversion,
-		SignedExtension,
-	},
-	transaction_validity::TransactionValidityError,
+	traits::{Convert, IdentityLookup, OpaqueKeys, SaturatedConversion},
 	DigestItem, Perbill,
 };
 use sp_staking::SessionIndex;
@@ -154,6 +147,17 @@ impl session::historical::Trait for Test {
 	type FullIdentificationOf = staking::ExposureOf<Self>;
 }
 
+parameter_types! {
+	pub const UncleGenerations: u64 = 0;
+}
+
+impl pallet_authorship::Trait for Test {
+	type FindAuthor = ();
+	type UncleGenerations = UncleGenerations;
+	type FilterUncle = ();
+	type EventHandler = ();
+}
+
 parameter_types! {
 	pub const ExistentialDeposit: u128 = 1;
 }
@@ -264,66 +268,7 @@ impl Trait for Test {
 		AuthorityId,
 	)>>::IdentificationTuple;
 
-	type HandleEquivocation = super::EquivocationHandler<
-		Self::KeyOwnerIdentification,
-		reporting_keys::ReporterAppCrypto,
-		Test,
-		Offences,
-	>;
-}
-
-pub mod reporting_keys {
-	use sp_core::crypto::KeyTypeId;
-
-	pub const KEY_TYPE: KeyTypeId = KeyTypeId(*b"test");
-
-	mod app {
-		use sp_application_crypto::{app_crypto, ed25519};
-		app_crypto!(ed25519, super::KEY_TYPE);
-
-		impl sp_runtime::traits::IdentifyAccount for Public {
-			type AccountId = u64;
-			fn into_account(self) -> Self::AccountId {
-				super::super::Grandpa::grandpa_authorities()
-					.iter()
-					.map(|(k, _)| k)
-					.position(|b| *b == self.0.clone().into())
-					.unwrap() as u64
-			}
-		}
-	}
-
-	pub type ReporterId = app::Public;
-
-	pub struct ReporterAppCrypto;
-	impl frame_system::offchain::AppCrypto<ReporterId, sp_core::ed25519::Signature>
-		for ReporterAppCrypto
-	{
-		type RuntimeAppPublic = ReporterId;
-		type GenericSignature = sp_core::ed25519::Signature;
-		type GenericPublic = sp_core::ed25519::Public;
-	}
-}
-
-type Extrinsic = TestXt<Call, ()>;
-
-impl<LocalCall> system::offchain::CreateSignedTransaction<LocalCall> for Test
-where
-	Call: From<LocalCall>,
-{
-	fn create_transaction<C: system::offchain::AppCrypto<Self::Public, Self::Signature>>(
-		call: Call,
-		_public: reporting_keys::ReporterId,
-		_account: <Test as system::Trait>::AccountId,
-		nonce: <Test as system::Trait>::Index,
-	) -> Option<(Call, <Extrinsic as ExtrinsicT>::SignaturePayload)> {
-		Some((call, (nonce, ())))
-	}
-}
-
-impl frame_system::offchain::SigningTypes for Test {
-	type Public = reporting_keys::ReporterId;
-	type Signature = sp_core::ed25519::Signature;
+	type HandleEquivocation = super::EquivocationHandler<Self::KeyOwnerIdentification, Offences>;
 }
 
 mod grandpa {
@@ -468,18 +413,6 @@ pub fn initialize_block(number: u64, parent_hash: H256) {
 	);
 }
 
-pub fn report_equivocation(
-	equivocation_proof: sp_finality_grandpa::EquivocationProof<H256, u64>,
-	key_owner_proof: sp_session::MembershipProof,
-) -> Result<GrandpaCall<Test>, TransactionValidityError> {
-	let inner = GrandpaCall::report_equivocation(equivocation_proof, key_owner_proof);
-	let call = Call::Grandpa(inner.clone());
-
-	ValidateEquivocationReport::<Test>::new().validate(&0, &call, &DispatchInfo::default(), 0)?;
-
-	Ok(inner)
-}
-
 pub fn generate_equivocation_proof(
 	set_id: SetId,
 	vote1: (RoundNumber, H256, u64, &Ed25519Keyring),

substrate/frame/grandpa/src/tests.rs

@@ -19,13 +19,13 @@
 
 #![cfg(test)]
 
-use super::*;
+use super::{Call, *};
 use crate::mock::*;
 use codec::{Decode, Encode};
 use fg_primitives::ScheduledChange;
 use frame_support::{
 	assert_err, assert_ok,
-	traits::{Currency, OnFinalize, UnfilteredDispatchable},
+	traits::{Currency, OnFinalize},
 };
 use frame_system::{EventRecord, Phase};
 use sp_core::H256;
@@ -316,7 +316,9 @@ fn time_slot_have_sane_ord() {
 	assert!(FIXTURE.windows(2).all(|f| f[0] < f[1]));
 }
 
-fn test_authorities() -> AuthorityList {
+/// Returns a list with 3 authorities with known keys:
+/// Alice, Bob and Charlie.
+pub fn test_authorities() -> AuthorityList {
 	let authorities = vec![
 		Ed25519Keyring::Alice,
 		Ed25519Keyring::Bob,
@@ -375,8 +377,13 @@ fn report_equivocation_current_set_works() {
 			Historical::prove((sp_finality_grandpa::KEY_TYPE, &equivocation_key)).unwrap();
 
 		// report the equivocation and the tx should be dispatched successfully
-		let inner = report_equivocation(equivocation_proof, key_owner_proof).unwrap();
-		assert_ok!(inner.dispatch_bypass_filter(Origin::signed(1)));
+		assert_ok!(
+			Grandpa::report_equivocation_unsigned(
+				Origin::none(),
+				equivocation_proof,
+				key_owner_proof,
+			),
+		);
 
 		start_era(2);
 
@@ -456,8 +463,13 @@ fn report_equivocation_old_set_works() {
 
 		// report the equivocation using the key ownership proof generated on
 		// the old set, the tx should be dispatched successfully
-		let inner = report_equivocation(equivocation_proof, key_owner_proof).unwrap();
-		assert_ok!(inner.dispatch_bypass_filter(Origin::signed(1)));
+		assert_ok!(
+			Grandpa::report_equivocation_unsigned(
+				Origin::none(),
+				equivocation_proof,
+				key_owner_proof,
+			),
+		);
 
 		start_era(3);
 
@@ -516,10 +528,14 @@ fn report_equivocation_invalid_set_id() {
 			(1, H256::random(), 10, &equivocation_keyring),
 		);
 
-		// it should be filtered by the signed extension validation
+		// the call for reporting the equivocation should error
 		assert_err!(
-			report_equivocation(equivocation_proof, key_owner_proof),
-			equivocation::ReportEquivocationValidityError::InvalidSetId,
+			Grandpa::report_equivocation_unsigned(
+				Origin::none(),
+				equivocation_proof,
+				key_owner_proof,
+			),
+			Error::<Test>::InvalidEquivocationProof,
 		);
 	});
 }
@@ -555,8 +571,12 @@ fn report_equivocation_invalid_session() {
 		// report an equivocation for the current set using an key ownership
 		// proof from the previous set, the session should be invalid.
 		assert_err!(
-			report_equivocation(equivocation_proof, key_owner_proof),
-			equivocation::ReportEquivocationValidityError::InvalidSession,
+			Grandpa::report_equivocation_unsigned(
+				Origin::none(),
+				equivocation_proof,
+				key_owner_proof,
+			),
+			Error::<Test>::InvalidEquivocationProof,
 		);
 	});
 }
@@ -596,8 +616,12 @@ fn report_equivocation_invalid_key_owner_proof() {
 		// report an equivocation for the current set using a key ownership
 		// proof for a different key than the one in the equivocation proof.
 		assert_err!(
-			report_equivocation(equivocation_proof, invalid_key_owner_proof),
-			equivocation::ReportEquivocationValidityError::InvalidKeyOwnershipProof,
+			Grandpa::report_equivocation_unsigned(
+				Origin::none(),
+				equivocation_proof,
+				invalid_key_owner_proof,
+			),
+			Error::<Test>::InvalidKeyOwnershipProof,
 		);
 	});
 }
@@ -623,8 +647,12 @@ fn report_equivocation_invalid_equivocation_proof() {
 
 		let assert_invalid_equivocation_proof = |equivocation_proof| {
 			assert_err!(
-				report_equivocation(equivocation_proof, key_owner_proof.clone()),
-				equivocation::ReportEquivocationValidityError::InvalidEquivocationProof,
+				Grandpa::report_equivocation_unsigned(
+					Origin::none(),
+					equivocation_proof,
+					key_owner_proof.clone(),
+				),
+				Error::<Test>::InvalidEquivocationProof,
 			);
 		};
 
@@ -660,3 +688,82 @@ fn report_equivocation_invalid_equivocation_proof() {
 		));
 	});
 }
+
+#[test]
+fn report_equivocation_validate_unsigned_prevents_duplicates() {
+	use sp_runtime::transaction_validity::{
+		InvalidTransaction, TransactionLongevity, TransactionPriority, TransactionSource,
+		TransactionValidity, ValidTransaction,
+	};
+
+	let authorities = test_authorities();
+
+	new_test_ext_raw_authorities(authorities).execute_with(|| {
+		start_era(1);
+
+		let authorities = Grandpa::grandpa_authorities();
+
+		// generate and report an equivocation for the validator at index 0
+		let equivocation_authority_index = 0;
+		let equivocation_key = &authorities[equivocation_authority_index].0;
+		let equivocation_keyring = extract_keyring(equivocation_key);
+		let set_id = Grandpa::current_set_id();
+
+		let equivocation_proof = generate_equivocation_proof(
+			set_id,
+			(1, H256::random(), 10, &equivocation_keyring),
+			(1, H256::random(), 10, &equivocation_keyring),
+		);
+
+		let key_owner_proof =
+			Historical::prove((sp_finality_grandpa::KEY_TYPE, &equivocation_key)).unwrap();
+
+		let call = Call::report_equivocation_unsigned(
+			equivocation_proof.clone(),
+			key_owner_proof.clone(),
+		);
+
+		// only local/inblock reports are allowed
+		assert_eq!(
+			<Grandpa as sp_runtime::traits::ValidateUnsigned>::validate_unsigned(
+				TransactionSource::External,
+				&call,
+			),
+			InvalidTransaction::Call.into(),
+		);
+
+		// the transaction is valid when passed as local
+		let tx_tag = (
+			equivocation_key,
+			set_id,
+			1u64,
+		);
+
+		assert_eq!(
+			<Grandpa as sp_runtime::traits::ValidateUnsigned>::validate_unsigned(
+				TransactionSource::Local,
+				&call,
+			),
+			TransactionValidity::Ok(ValidTransaction {
+				priority: TransactionPriority::max_value(),
+				requires: vec![],
+				provides: vec![("GrandpaEquivocation", tx_tag).encode()],
+				longevity: TransactionLongevity::max_value(),
+				propagate: false,
+			})
+		);
+
+		// the pre dispatch checks should also pass
+		assert_ok!(<Grandpa as sp_runtime::traits::ValidateUnsigned>::pre_dispatch(&call));
+
+		// we submit the report
+		Grandpa::report_equivocation_unsigned(Origin::none(), equivocation_proof, key_owner_proof)
+			.unwrap();
+
+		// the report should now be considered stale and the transaction is invalid
+		assert_err!(
+			<Grandpa as sp_runtime::traits::ValidateUnsigned>::pre_dispatch(&call),
+			InvalidTransaction::Stale,
+		);
+	});
+}