From a22edf0a24ed5469d216a067f739e2cc45c78bb4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= <123550+andresilva@users.noreply.github.com> Date: Tue, 8 Sep 2020 11:05:36 +0100 Subject: [PATCH] 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 --- frame/babe/src/lib.rs | 24 +++++++------- frame/babe/src/tests.rs | 59 +++++++++++++++++++++++++++++++++++ frame/grandpa/src/lib.rs | 19 ++++++----- frame/grandpa/src/tests.rs | 64 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 147 insertions(+), 19 deletions(-) diff --git a/frame/babe/src/lib.rs b/frame/babe/src/lib.rs index 891411e8ede57..2c1b2b16efc07 100644 --- a/frame/babe/src/lib.rs +++ b/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, 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, 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 Module { reporter: Option, equivocation_proof: EquivocationProof, key_owner_proof: T::KeyOwnerProof, - ) -> Result<(), Error> { + ) -> 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::::InvalidEquivocationProof.into()); } let validator_set_count = key_owner_proof.validator_count(); @@ -655,13 +656,13 @@ impl Module { // 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::::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::::InvalidKeyOwnershipProof)?; let offence = BabeEquivocationOffence { slot: slot_number, @@ -676,9 +677,10 @@ impl Module { }; T::HandleEquivocation::report_offence(reporters, offence) - .map_err(|_| Error::DuplicateOffenceReport)?; + .map_err(|_| Error::::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 diff --git a/frame/babe/src/tests.rs b/frame/babe/src/tests.rs index 2b24e1208de1d..66229e5a6c8a1 100644 --- a/frame/babe/src/tests.rs +++ b/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::::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); + }) +} diff --git a/frame/grandpa/src/lib.rs b/frame/grandpa/src/lib.rs index 09d32662d349a..e0f2d7beda2a6 100644 --- a/frame/grandpa/src/lib.rs +++ b/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, 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, 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 Module { reporter: Option, equivocation_proof: EquivocationProof, key_owner_proof: T::KeyOwnerProof, - ) -> Result<(), Error> { + ) -> 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 Module { set_id, round, ), - ).map_err(|_| Error::::DuplicateOffenceReport) + ).map_err(|_| Error::::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 diff --git a/frame/grandpa/src/tests.rs b/frame/grandpa/src/tests.rs index aa1b48681d402..951b28df57ec9 100644 --- a/frame/grandpa/src/tests.rs +++ b/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::::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); + }) +}