babe, grandpa: waive fees on valid equivocation report (#6981)

* babe: waive fees on report_equivocation

* grandpa: waive fees on report_equivocation

* babe: add test for fee waiving on valid equivocation report

* grandpa: add test for fee waiving on valid equivocation report

* grandpa: remove stray comment

Author: andresilva

frame/babe/src/lib.rs

@@ -24,8 +24,9 @@
 use codec::{Decode, Encode};
 use frame_support::{
 	decl_error, decl_module, decl_storage,
+	dispatch::DispatchResultWithPostInfo,
 	traits::{FindAuthor, Get, KeyOwnerProofSystem, Randomness as RandomnessT},
-	weights::Weight,
+	weights::{Pays, Weight},
 	Parameter,
 };
 use frame_system::{ensure_none, ensure_signed};
@@ -260,14 +261,14 @@ decl_module! {
 			origin,
 			equivocation_proof: EquivocationProof<T::Header>,
 			key_owner_proof: T::KeyOwnerProof,
-		) {
+		) -> DispatchResultWithPostInfo {
 			let reporter = ensure_signed(origin)?;
 
 			Self::do_report_equivocation(
 				Some(reporter),
 				equivocation_proof,
 				key_owner_proof,
-			)?;
+			)
 		}
 
 		/// Report authority equivocation/misbehavior. This method will verify
@@ -283,14 +284,14 @@ decl_module! {
 			origin,
 			equivocation_proof: EquivocationProof<T::Header>,
 			key_owner_proof: T::KeyOwnerProof,
-		) {
+		) -> DispatchResultWithPostInfo {
 			ensure_none(origin)?;
 
 			Self::do_report_equivocation(
 				T::HandleEquivocation::block_author(),
 				equivocation_proof,
 				key_owner_proof,
-			)?;
+			)
 		}
 	}
 }
@@ -637,13 +638,13 @@ impl<T: Trait> Module<T> {
 		reporter: Option<T::AccountId>,
 		equivocation_proof: EquivocationProof<T::Header>,
 		key_owner_proof: T::KeyOwnerProof,
-	) -> Result<(), Error<T>> {
+	) -> DispatchResultWithPostInfo {
 		let offender = equivocation_proof.offender.clone();
 		let slot_number = equivocation_proof.slot_number;
 
 		// validate the equivocation proof
 		if !sp_consensus_babe::check_equivocation_proof(equivocation_proof) {
-			return Err(Error::InvalidEquivocationProof.into());
+			return Err(Error::<T>::InvalidEquivocationProof.into());
 		}
 
 		let validator_set_count = key_owner_proof.validator_count();
@@ -655,13 +656,13 @@ impl<T: Trait> Module<T> {
 		// check that the slot number is consistent with the session index
 		// in the key ownership proof (i.e. slot is for that epoch)
 		if epoch_index != session_index {
-			return Err(Error::InvalidKeyOwnershipProof.into());
+			return Err(Error::<T>::InvalidKeyOwnershipProof.into());
 		}
 
 		// check the membership proof and extract the offender's id
 		let key = (sp_consensus_babe::KEY_TYPE, offender);
 		let offender = T::KeyOwnerProofSystem::check_proof(key, key_owner_proof)
-			.ok_or(Error::InvalidKeyOwnershipProof)?;
+			.ok_or(Error::<T>::InvalidKeyOwnershipProof)?;
 
 		let offence = BabeEquivocationOffence {
 			slot: slot_number,
@@ -676,9 +677,10 @@ impl<T: Trait> Module<T> {
 		};
 
 		T::HandleEquivocation::report_offence(reporters, offence)
-			.map_err(|_| Error::DuplicateOffenceReport)?;
+			.map_err(|_| Error::<T>::DuplicateOffenceReport)?;
 
-		Ok(())
+		// waive the fee since the report is valid and beneficial
+		Ok(Pays::No.into())
 	}
 
 	/// Submits an extrinsic to report an equivocation. This method will create

frame/babe/src/tests.rs

@@ -21,6 +21,7 @@ use super::{Call, *};
 use frame_support::{
 	assert_err, assert_ok,
 	traits::{Currency, OnFinalize},
+	weights::{GetDispatchInfo, Pays},
 };
 use mock::*;
 use pallet_session::ShouldEndSession;
@@ -608,3 +609,61 @@ fn report_equivocation_has_valid_weight() {
 			.all(|w| w[0] < w[1])
 	);
 }
+
+#[test]
+fn valid_equivocation_reports_dont_pay_fees() {
+	let (pairs, mut ext) = new_test_ext_with_pairs(3);
+
+	ext.execute_with(|| {
+		start_era(1);
+
+		let offending_authority_pair = &pairs[0];
+
+		// generate an equivocation proof.
+		let equivocation_proof =
+			generate_equivocation_proof(0, &offending_authority_pair, CurrentSlot::get());
+
+		// create the key ownership proof.
+		let key_owner_proof = Historical::prove((
+			sp_consensus_babe::KEY_TYPE,
+			&offending_authority_pair.public(),
+		))
+		.unwrap();
+
+		// check the dispatch info for the call.
+		let info = Call::<Test>::report_equivocation_unsigned(
+			equivocation_proof.clone(),
+			key_owner_proof.clone(),
+		)
+		.get_dispatch_info();
+
+		// it should have non-zero weight and the fee has to be paid.
+		assert!(info.weight > 0);
+		assert_eq!(info.pays_fee, Pays::Yes);
+
+		// report the equivocation.
+		let post_info = Babe::report_equivocation_unsigned(
+			Origin::none(),
+			equivocation_proof.clone(),
+			key_owner_proof.clone(),
+		)
+		.unwrap();
+
+		// the original weight should be kept, but given that the report
+		// is valid the fee is waived.
+		assert!(post_info.actual_weight.is_none());
+		assert_eq!(post_info.pays_fee, Pays::No);
+
+		// report the equivocation again which is invalid now since it is
+		// duplicate.
+		let post_info =
+			Babe::report_equivocation_unsigned(Origin::none(), equivocation_proof, key_owner_proof)
+				.err()
+				.unwrap()
+				.post_info;
+
+		// the fee is not waived and the original weight is kept.
+		assert!(post_info.actual_weight.is_none());
+		assert_eq!(post_info.pays_fee, Pays::Yes);
+	})
+}

frame/grandpa/src/lib.rs

@@ -40,8 +40,8 @@ use fg_primitives::{
 	GRANDPA_ENGINE_ID,
 };
 use frame_support::{
-	decl_error, decl_event, decl_module, decl_storage, storage, traits::KeyOwnerProofSystem,
-	Parameter,
+	decl_error, decl_event, decl_module, decl_storage, dispatch::DispatchResultWithPostInfo,
+	storage, traits::KeyOwnerProofSystem, weights::Pays, Parameter,
 };
 use frame_system::{ensure_none, ensure_root, ensure_signed};
 use pallet_finality_tracker::OnFinalizationStalled;
@@ -247,14 +247,14 @@ decl_module! {
 			origin,
 			equivocation_proof: EquivocationProof<T::Hash, T::BlockNumber>,
 			key_owner_proof: T::KeyOwnerProof,
-		) {
+		) -> DispatchResultWithPostInfo {
 			let reporter = ensure_signed(origin)?;
 
 			Self::do_report_equivocation(
 				Some(reporter),
 				equivocation_proof,
 				key_owner_proof,
-			)?;
+			)
 		}
 
 		/// Report voter equivocation/misbehavior. This method will verify the
@@ -271,14 +271,14 @@ decl_module! {
 			origin,
 			equivocation_proof: EquivocationProof<T::Hash, T::BlockNumber>,
 			key_owner_proof: T::KeyOwnerProof,
-		) {
+		) -> DispatchResultWithPostInfo {
 			ensure_none(origin)?;
 
 			Self::do_report_equivocation(
 				T::HandleEquivocation::block_author(),
 				equivocation_proof,
 				key_owner_proof,
-			)?;
+			)
 		}
 
 		/// Note that the current authority set of the GRANDPA finality gadget has
@@ -520,7 +520,7 @@ impl<T: Trait> Module<T> {
 		reporter: Option<T::AccountId>,
 		equivocation_proof: EquivocationProof<T::Hash, T::BlockNumber>,
 		key_owner_proof: T::KeyOwnerProof,
-	) -> Result<(), Error<T>> {
+	) -> DispatchResultWithPostInfo {
 		// 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
@@ -585,7 +585,10 @@ impl<T: Trait> Module<T> {
 				set_id,
 				round,
 			),
-		).map_err(|_| Error::<T>::DuplicateOffenceReport)
+		).map_err(|_| Error::<T>::DuplicateOffenceReport)?;
+
+		// waive the fee since the report is valid and beneficial
+		Ok(Pays::No.into())
 	}
 
 	/// Submits an extrinsic to report an equivocation. This method will create

frame/grandpa/src/tests.rs

@@ -26,6 +26,7 @@ use fg_primitives::ScheduledChange;
 use frame_support::{
 	assert_err, assert_ok,
 	traits::{Currency, OnFinalize},
+	weights::{GetDispatchInfo, Pays},
 };
 use frame_system::{EventRecord, Phase};
 use pallet_session::OneSessionHandler;
@@ -865,3 +866,66 @@ fn report_equivocation_has_valid_weight() {
 			.all(|w| w[0] < w[1])
 	);
 }
+
+#[test]
+fn valid_equivocation_reports_dont_pay_fees() {
+	let authorities = test_authorities();
+
+	new_test_ext_raw_authorities(authorities).execute_with(|| {
+		start_era(1);
+
+		let equivocation_key = &Grandpa::grandpa_authorities()[0].0;
+		let equivocation_keyring = extract_keyring(equivocation_key);
+		let set_id = Grandpa::current_set_id();
+
+		// generate an equivocation proof.
+		let equivocation_proof = generate_equivocation_proof(
+			set_id,
+			(1, H256::random(), 10, &equivocation_keyring),
+			(1, H256::random(), 10, &equivocation_keyring),
+		);
+
+		// create the key ownership proof.
+		let key_owner_proof =
+			Historical::prove((sp_finality_grandpa::KEY_TYPE, &equivocation_key)).unwrap();
+
+		// check the dispatch info for the call.
+		let info = Call::<Test>::report_equivocation_unsigned(
+			equivocation_proof.clone(),
+			key_owner_proof.clone(),
+		)
+		.get_dispatch_info();
+
+		// it should have non-zero weight and the fee has to be paid.
+		assert!(info.weight > 0);
+		assert_eq!(info.pays_fee, Pays::Yes);
+
+		// report the equivocation.
+		let post_info = Grandpa::report_equivocation_unsigned(
+			Origin::none(),
+			equivocation_proof.clone(),
+			key_owner_proof.clone(),
+		)
+		.unwrap();
+
+		// the original weight should be kept, but given that the report
+		// is valid the fee is waived.
+		assert!(post_info.actual_weight.is_none());
+		assert_eq!(post_info.pays_fee, Pays::No);
+
+		// report the equivocation again which is invalid now since it is
+		// duplicate.
+		let post_info = Grandpa::report_equivocation_unsigned(
+			Origin::none(),
+			equivocation_proof,
+			key_owner_proof,
+		)
+		.err()
+		.unwrap()
+		.post_info;
+
+		// the fee is not waived and the original weight is kept.
+		assert!(post_info.actual_weight.is_none());
+		assert_eq!(post_info.pays_fee, Pays::Yes);
+	})
+}