From e663486ea1708d19da8f3e48b7d5b07ed0383194 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Mon, 5 Feb 2024 18:31:02 +0300 Subject: [PATCH 1/5] depreacte submit_finality_proof and introduce submit_finality_proof_ex instead --- modules/grandpa/src/lib.rs | 216 ++++++++++++++++++++++++------------- 1 file changed, 140 insertions(+), 76 deletions(-) diff --git a/modules/grandpa/src/lib.rs b/modules/grandpa/src/lib.rs index f58db2481ad..21600117af3 100644 --- a/modules/grandpa/src/lib.rs +++ b/modules/grandpa/src/lib.rs @@ -151,7 +151,85 @@ pub mod pallet { #[pallet::call] impl, I: 'static> Pallet { - /// Verify a target header is finalized according to the given finality proof. + /// This call is deprecated and will be removed around May 2024. Use the + /// `submit_finality_proof_ex` instead. Semantically, this call is an equivalent of the + /// `submit_finality_proof_ex` call without current authority set id check. + #[pallet::call_index(0)] + #[pallet::weight(::submit_finality_proof( + justification.commit.precommits.len().saturated_into(), + justification.votes_ancestries.len().saturated_into(), + ))] + #[deprecated( + note = "`submit_finality_proof` will be removed in May 2024. Use `submit_finality_proof_ex` instead." + )] + pub fn submit_finality_proof( + origin: OriginFor, + finality_target: Box>, + justification: GrandpaJustification>, + ) -> DispatchResultWithPostInfo { + Self::submit_finality_proof_ex( + origin, + finality_target, + justification, + // the `submit_finality_proof_ex` also reads this value, but it is done from the + // cache, so we don't treat it as an additional db access + >::get().set_id, + ) + } + + /// Bootstrap the bridge pallet with an initial header and authority set from which to sync. + /// + /// The initial configuration provided does not need to be the genesis header of the bridged + /// chain, it can be any arbitrary header. You can also provide the next scheduled set + /// change if it is already know. + /// + /// This function is only allowed to be called from a trusted origin and writes to storage + /// with practically no checks in terms of the validity of the data. It is important that + /// you ensure that valid data is being passed in. + #[pallet::call_index(1)] + #[pallet::weight((T::DbWeight::get().reads_writes(2, 5), DispatchClass::Operational))] + pub fn initialize( + origin: OriginFor, + init_data: super::InitializationData>, + ) -> DispatchResultWithPostInfo { + Self::ensure_owner_or_root(origin)?; + + let init_allowed = !>::exists(); + ensure!(init_allowed, >::AlreadyInitialized); + initialize_bridge::(init_data.clone())?; + + log::info!( + target: LOG_TARGET, + "Pallet has been initialized with the following parameters: {:?}", + init_data + ); + + Ok(().into()) + } + + /// Change `PalletOwner`. + /// + /// May only be called either by root, or by `PalletOwner`. + #[pallet::call_index(2)] + #[pallet::weight((T::DbWeight::get().reads_writes(1, 1), DispatchClass::Operational))] + pub fn set_owner(origin: OriginFor, new_owner: Option) -> DispatchResult { + >::set_owner(origin, new_owner) + } + + /// Halt or resume all pallet operations. + /// + /// May only be called either by root, or by `PalletOwner`. + #[pallet::call_index(3)] + #[pallet::weight((T::DbWeight::get().reads_writes(1, 1), DispatchClass::Operational))] + pub fn set_operating_mode( + origin: OriginFor, + operating_mode: BasicOperatingMode, + ) -> DispatchResult { + >::set_operating_mode(origin, operating_mode) + } + + /// Verify a target header is finalized according to the given finality proof. The proof + /// is assumed to be signed by GRANDPA authorities set with `current_set_id` id. /// /// It will use the underlying storage pallet to fetch information about the current /// authorities and best finalized header in order to verify that the header is finalized. @@ -165,18 +243,22 @@ pub mod pallet { /// /// - the pallet knows better header than the `finality_target`; /// + /// - the id of best GRANDPA authority set, known to the pallet is not equal to the + /// `current_set_id`; + /// /// - verification is not optimized or invalid; /// /// - header contains forced authorities set change or change with non-zero delay. - #[pallet::call_index(0)] + #[pallet::call_index(4)] #[pallet::weight(::submit_finality_proof( justification.commit.precommits.len().saturated_into(), justification.votes_ancestries.len().saturated_into(), ))] - pub fn submit_finality_proof( + pub fn submit_finality_proof_ex( origin: OriginFor, finality_target: Box>, justification: GrandpaJustification>, + current_set_id: sp_consensus_grandpa::SetId, ) -> DispatchResultWithPostInfo { Self::ensure_not_halted().map_err(Error::::BridgeModule)?; ensure_signed(origin)?; @@ -193,6 +275,8 @@ pub mod pallet { let authority_set = >::get(); let unused_proof_size = authority_set.unused_proof_size(); let set_id = authority_set.set_id; + ensure!(current_set_id == set_id, >::InvalidAuthoritySetId); + let authority_set: AuthoritySet = authority_set.into(); verify_justification::(&justification, hash, number, authority_set)?; @@ -248,57 +332,6 @@ pub mod pallet { Ok(PostDispatchInfo { actual_weight: Some(actual_weight), pays_fee }) } - - /// Bootstrap the bridge pallet with an initial header and authority set from which to sync. - /// - /// The initial configuration provided does not need to be the genesis header of the bridged - /// chain, it can be any arbitrary header. You can also provide the next scheduled set - /// change if it is already know. - /// - /// This function is only allowed to be called from a trusted origin and writes to storage - /// with practically no checks in terms of the validity of the data. It is important that - /// you ensure that valid data is being passed in. - #[pallet::call_index(1)] - #[pallet::weight((T::DbWeight::get().reads_writes(2, 5), DispatchClass::Operational))] - pub fn initialize( - origin: OriginFor, - init_data: super::InitializationData>, - ) -> DispatchResultWithPostInfo { - Self::ensure_owner_or_root(origin)?; - - let init_allowed = !>::exists(); - ensure!(init_allowed, >::AlreadyInitialized); - initialize_bridge::(init_data.clone())?; - - log::info!( - target: LOG_TARGET, - "Pallet has been initialized with the following parameters: {:?}", - init_data - ); - - Ok(().into()) - } - - /// Change `PalletOwner`. - /// - /// May only be called either by root, or by `PalletOwner`. - #[pallet::call_index(2)] - #[pallet::weight((T::DbWeight::get().reads_writes(1, 1), DispatchClass::Operational))] - pub fn set_owner(origin: OriginFor, new_owner: Option) -> DispatchResult { - >::set_owner(origin, new_owner) - } - - /// Halt or resume all pallet operations. - /// - /// May only be called either by root, or by `PalletOwner`. - #[pallet::call_index(3)] - #[pallet::weight((T::DbWeight::get().reads_writes(1, 1), DispatchClass::Operational))] - pub fn set_operating_mode( - origin: OriginFor, - operating_mode: BasicOperatingMode, - ) -> DispatchResult { - >::set_operating_mode(origin, operating_mode) - } } /// Number mandatory headers that we may accept in the current block for free (returning @@ -436,6 +469,9 @@ pub mod pallet { TooManyAuthoritiesInSet, /// Error generated by the `OwnedBridgeModule` trait. BridgeModule(bp_runtime::OwnedBridgeModuleError), + /// The `current_set_id` argument of the `submit_finality_proof_ex` doesn't match + /// the id of the current set, known to the pallet. + InvalidAuthoritySetId, } /// Check the given header for a GRANDPA scheduled authority set change. If a change @@ -671,10 +707,12 @@ mod tests { storage::generator::StorageValue, }; use frame_system::{EventRecord, Phase}; - use sp_consensus_grandpa::{ConsensusLog, GRANDPA_ENGINE_ID}; + use sp_consensus_grandpa::{ConsensusLog, SetId, GRANDPA_ENGINE_ID}; use sp_core::Get; use sp_runtime::{Digest, DigestItem, DispatchError}; + const DEFAULT_SET_ID: SetId = 1; + fn initialize_substrate_bridge() { System::set_block_number(1); System::reset_events(); @@ -693,7 +731,7 @@ mod tests { let init_data = InitializationData { header: Box::new(genesis), authority_list: authority_list(), - set_id: 1, + set_id: DEFAULT_SET_ID, operating_mode: BasicOperatingMode::Normal, }; @@ -704,10 +742,11 @@ mod tests { let header = test_header(header.into()); let justification = make_default_justification(&header); - Pallet::::submit_finality_proof( + Pallet::::submit_finality_proof_ex( RuntimeOrigin::signed(1), Box::new(header), justification, + DEFAULT_SET_ID, ) } @@ -722,10 +761,11 @@ mod tests { ..Default::default() }); - Pallet::::submit_finality_proof( + Pallet::::submit_finality_proof_ex( RuntimeOrigin::signed(1), Box::new(header), justification, + set_id, ) } @@ -749,10 +789,11 @@ mod tests { ..Default::default() }); - Pallet::::submit_finality_proof( + Pallet::::submit_finality_proof_ex( RuntimeOrigin::signed(1), Box::new(header), justification, + set_id, ) } @@ -955,17 +996,30 @@ mod tests { let header = test_header(1); - let params = - JustificationGeneratorParams:: { set_id: 2, ..Default::default() }; + let next_set_id = 2; + let params = JustificationGeneratorParams:: { + set_id: next_set_id, + ..Default::default() + }; let justification = make_justification_for_header(params); assert_err!( - Pallet::::submit_finality_proof( + Pallet::::submit_finality_proof_ex( + RuntimeOrigin::signed(1), + Box::new(header.clone()), + justification.clone(), + DEFAULT_SET_ID, + ), + >::InvalidJustification + ); + assert_err!( + Pallet::::submit_finality_proof_ex( RuntimeOrigin::signed(1), Box::new(header), justification, + next_set_id, ), - >::InvalidJustification + >::InvalidAuthoritySetId ); }) } @@ -980,10 +1034,11 @@ mod tests { justification.round = 42; assert_err!( - Pallet::::submit_finality_proof( + Pallet::::submit_finality_proof_ex( RuntimeOrigin::signed(1), Box::new(header), justification, + DEFAULT_SET_ID, ), >::InvalidJustification ); @@ -1009,10 +1064,11 @@ mod tests { let justification = make_default_justification(&header); assert_err!( - Pallet::::submit_finality_proof( + Pallet::::submit_finality_proof_ex( RuntimeOrigin::signed(1), Box::new(header), justification, + DEFAULT_SET_ID, ), >::InvalidAuthoritySet ); @@ -1047,10 +1103,11 @@ mod tests { let justification = make_default_justification(&header); // Let's import our test header - let result = Pallet::::submit_finality_proof( + let result = Pallet::::submit_finality_proof_ex( RuntimeOrigin::signed(1), Box::new(header.clone()), justification.clone(), + DEFAULT_SET_ID, ); assert_ok!(result); assert_eq!(result.unwrap().pays_fee, frame_support::dispatch::Pays::No); @@ -1109,10 +1166,11 @@ mod tests { // without large digest item ^^^ the relayer would have paid zero transaction fee // (`Pays::No`) - let result = Pallet::::submit_finality_proof( + let result = Pallet::::submit_finality_proof_ex( RuntimeOrigin::signed(1), Box::new(header.clone()), justification, + DEFAULT_SET_ID, ); assert_ok!(result); assert_eq!(result.unwrap().pays_fee, frame_support::dispatch::Pays::Yes); @@ -1140,10 +1198,11 @@ mod tests { // without many headers in votes ancestries ^^^ the relayer would have paid zero // transaction fee (`Pays::No`) - let result = Pallet::::submit_finality_proof( + let result = Pallet::::submit_finality_proof_ex( RuntimeOrigin::signed(1), Box::new(header.clone()), justification, + DEFAULT_SET_ID, ); assert_ok!(result); assert_eq!(result.unwrap().pays_fee, frame_support::dispatch::Pays::Yes); @@ -1169,10 +1228,11 @@ mod tests { // Should not be allowed to import this header assert_err!( - Pallet::::submit_finality_proof( + Pallet::::submit_finality_proof_ex( RuntimeOrigin::signed(1), Box::new(header), - justification + justification, + DEFAULT_SET_ID, ), >::UnsupportedScheduledChange ); @@ -1194,10 +1254,11 @@ mod tests { // Should not be allowed to import this header assert_err!( - Pallet::::submit_finality_proof( + Pallet::::submit_finality_proof_ex( RuntimeOrigin::signed(1), Box::new(header), - justification + justification, + DEFAULT_SET_ID, ), >::UnsupportedScheduledChange ); @@ -1219,10 +1280,11 @@ mod tests { // Should not be allowed to import this header assert_err!( - Pallet::::submit_finality_proof( + Pallet::::submit_finality_proof_ex( RuntimeOrigin::signed(1), Box::new(header), - justification + justification, + DEFAULT_SET_ID, ), >::TooManyAuthoritiesInSet ); @@ -1283,10 +1345,11 @@ mod tests { let mut invalid_justification = make_default_justification(&header); invalid_justification.round = 42; - Pallet::::submit_finality_proof( + Pallet::::submit_finality_proof_ex( RuntimeOrigin::signed(1), Box::new(header), invalid_justification, + DEFAULT_SET_ID, ) }; @@ -1451,10 +1514,11 @@ mod tests { let justification = make_default_justification(&header); assert_noop!( - Pallet::::submit_finality_proof( + Pallet::::submit_finality_proof_ex( RuntimeOrigin::root(), Box::new(header), justification, + DEFAULT_SET_ID, ), DispatchError::BadOrigin, ); From 9e60475a3f0fc449c1f55d3566cb257ebd2d0cb2 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Tue, 6 Feb 2024 08:56:15 +0300 Subject: [PATCH 2/5] propagate changes to the rest of runtime crates --- .../src/refund_relayer_extension.rs | 11 +++- modules/grandpa/README.md | 10 +-- modules/grandpa/src/benchmarking.rs | 11 ++-- modules/grandpa/src/call_ext.rs | 64 ++++++++++++++++--- modules/grandpa/src/lib.rs | 35 +++++----- modules/parachains/src/lib.rs | 4 +- primitives/header-chain/src/lib.rs | 10 +++ 7 files changed, 104 insertions(+), 41 deletions(-) diff --git a/bin/runtime-common/src/refund_relayer_extension.rs b/bin/runtime-common/src/refund_relayer_extension.rs index 27b7ff1a551..ede831899ea 100644 --- a/bin/runtime-common/src/refund_relayer_extension.rs +++ b/bin/runtime-common/src/refund_relayer_extension.rs @@ -844,7 +844,7 @@ mod tests { use bp_parachains::{BestParaHeadHash, ParaInfo}; use bp_polkadot_core::parachains::{ParaHeadsProof, ParaId}; use bp_runtime::{BasicOperatingMode, HeaderId}; - use bp_test_utils::{make_default_justification, test_keyring}; + use bp_test_utils::{make_default_justification, test_keyring, TEST_GRANDPA_SET_ID}; use frame_support::{ assert_storage_noop, parameter_types, traits::{fungible::Mutate, ReservableCurrency}, @@ -929,7 +929,7 @@ mod tests { let authorities = test_keyring().into_iter().map(|(a, w)| (a.into(), w)).collect(); let best_relay_header = HeaderId(best_relay_header_number, RelayBlockHash::default()); pallet_bridge_grandpa::CurrentAuthoritySet::::put( - StoredAuthoritySet::try_new(authorities, 0).unwrap(), + StoredAuthoritySet::try_new(authorities, TEST_GRANDPA_SET_ID).unwrap(), ); pallet_bridge_grandpa::BestFinalized::::put(best_relay_header); @@ -971,9 +971,10 @@ mod tests { ); let relay_justification = make_default_justification(&relay_header); - RuntimeCall::BridgeGrandpa(GrandpaCall::submit_finality_proof { + RuntimeCall::BridgeGrandpa(GrandpaCall::submit_finality_proof_ex { finality_target: Box::new(relay_header), justification: relay_justification, + current_set_id: TEST_GRANDPA_SET_ID, }) } @@ -1105,6 +1106,7 @@ mod tests { call_info: CallInfo::AllFinalityAndMsgs( SubmitFinalityProofInfo { block_number: 200, + current_set_id: Some(TEST_GRANDPA_SET_ID), extra_weight: Weight::zero(), extra_size: 0, }, @@ -1134,6 +1136,7 @@ mod tests { call_info: CallInfo::AllFinalityAndMsgs( SubmitFinalityProofInfo { block_number: 200, + current_set_id: Some(TEST_GRANDPA_SET_ID), extra_weight: Weight::zero(), extra_size: 0, }, @@ -1159,6 +1162,7 @@ mod tests { call_info: CallInfo::RelayFinalityAndMsgs( SubmitFinalityProofInfo { block_number: 200, + current_set_id: Some(TEST_GRANDPA_SET_ID), extra_weight: Weight::zero(), extra_size: 0, }, @@ -1183,6 +1187,7 @@ mod tests { call_info: CallInfo::RelayFinalityAndMsgs( SubmitFinalityProofInfo { block_number: 200, + current_set_id: Some(TEST_GRANDPA_SET_ID), extra_weight: Weight::zero(), extra_size: 0, }, diff --git a/modules/grandpa/README.md b/modules/grandpa/README.md index 27b4d2389c4..43ee5c316d1 100644 --- a/modules/grandpa/README.md +++ b/modules/grandpa/README.md @@ -38,11 +38,11 @@ There are two main things in GRANDPA that help building light clients: ## Pallet Operations -The main entrypoint of the pallet is the `submit_finality_proof` call. It has two arguments - the finalized -headers and associated GRANDPA justification. The call simply verifies the justification using current -validators set and checks if header is better than the previous best header. If both checks are passed, the -header (only its useful fields) is inserted into the runtime storage and may be used by other pallets to verify -storage proofs. +The main entrypoint of the pallet is the `submit_finality_proof_ex` call. It has three arguments - the finalized +headers, associated GRANDPA justification and ID of the authority set that has generated this justification. The +call simply verifies the justification using current validators set and checks if header is better than the +previous best header. If both checks are passed, the header (only its useful fields) is inserted into the runtime +storage and may be used by other pallets to verify storage proofs. The submitter pays regular fee for submitting all headers, except for the mandatory header. Since it is required for the pallet operations, submitting such header is free. So if you're ok with session-length diff --git a/modules/grandpa/src/benchmarking.rs b/modules/grandpa/src/benchmarking.rs index 182b2f56eb1..2b3e8b992c5 100644 --- a/modules/grandpa/src/benchmarking.rs +++ b/modules/grandpa/src/benchmarking.rs @@ -16,8 +16,8 @@ //! Benchmarks for the GRANDPA Pallet. //! -//! The main dispatchable for the GRANDPA pallet is `submit_finality_proof`, so these benchmarks are -//! based around that. There are to main factors which affect finality proof verification: +//! The main dispatchable for the GRANDPA pallet is `submit_finality_proof_ex`, so these benchmarks +//! are based around that. There are to main factors which affect finality proof verification: //! //! 1. The number of `votes-ancestries` in the justification //! 2. The number of `pre-commits` in the justification @@ -77,7 +77,7 @@ fn precommits_range_end, I: 'static>() -> u32 { required_justification_precommits(max_bridged_authorities) } -/// Prepare header and its justification to submit using `submit_finality_proof`. +/// Prepare header and its justification to submit using `submit_finality_proof_ex`. fn prepare_benchmark_data, I: 'static>( precommits: u32, ancestors: u32, @@ -118,12 +118,13 @@ fn prepare_benchmark_data, I: 'static>( benchmarks_instance_pallet! { // This is the "gold standard" benchmark for this extrinsic, and it's what should be used to // annotate the weight in the pallet. - submit_finality_proof { + submit_finality_proof_ex { let p in 1 .. precommits_range_end::(); let v in MAX_VOTE_ANCESTRIES_RANGE_BEGIN..MAX_VOTE_ANCESTRIES_RANGE_END; let caller: T::AccountId = whitelisted_caller(); let (header, justification) = prepare_benchmark_data::(p, v); - }: submit_finality_proof(RawOrigin::Signed(caller), Box::new(header), justification) + let set_id = TEST_GRANDPA_SET_ID; + }: submit_finality_proof_ex(RawOrigin::Signed(caller), Box::new(header), justification, set_id) verify { let genesis_header: BridgedHeader = bp_test_utils::test_header(Zero::zero()); let header: BridgedHeader = bp_test_utils::test_header(One::one()); diff --git a/modules/grandpa/src/call_ext.rs b/modules/grandpa/src/call_ext.rs index c1585020be1..73e1f4a5402 100644 --- a/modules/grandpa/src/call_ext.rs +++ b/modules/grandpa/src/call_ext.rs @@ -14,7 +14,10 @@ // You should have received a copy of the GNU General Public License // along with Parity Bridges Common. If not, see . -use crate::{weights::WeightInfo, BridgedBlockNumber, BridgedHeader, Config, Error, Pallet}; +use crate::{ + weights::WeightInfo, BridgedBlockNumber, BridgedHeader, Config, CurrentAuthoritySet, Error, + Pallet, +}; use bp_header_chain::{ justification::GrandpaJustification, max_expected_submit_finality_proof_arguments_size, ChainWithGrandpa, GrandpaConsensusLogReader, @@ -22,6 +25,7 @@ use bp_header_chain::{ use bp_runtime::{BlockNumberOf, OwnedBridgeModule}; use codec::Encode; use frame_support::{dispatch::CallableCallFor, traits::IsSubType, weights::Weight}; +use sp_consensus_grandpa::SetId; use sp_runtime::{ traits::{Header, Zero}, transaction_validity::{InvalidTransaction, TransactionValidity, ValidTransaction}, @@ -33,6 +37,9 @@ use sp_runtime::{ pub struct SubmitFinalityProofInfo { /// Number of the finality target. pub block_number: N, + /// An identifier of the validators set that has signed the submitted justification. + /// It might be `None` if deprecated version of the `submit_finality_proof` is used. + pub current_set_id: Option, /// Extra weight that we assume is included in the call. /// /// We have some assumptions about headers and justifications of the bridged chain. @@ -61,9 +68,11 @@ pub struct SubmitFinalityProofHelper, I: 'static> { impl, I: 'static> SubmitFinalityProofHelper { /// Check that the GRANDPA head provided by the `SubmitFinalityProof` is better than the best - /// one we know. + /// one we know. Additionally, checks if `current_set_id` matches the current authority set + /// id, if specified. pub fn check_obsolete( finality_target: BlockNumberOf, + current_set_id: Option, ) -> Result<(), Error> { let best_finalized = crate::BestFinalized::::get().ok_or_else(|| { log::trace!( @@ -85,6 +94,20 @@ impl, I: 'static> SubmitFinalityProofHelper { return Err(Error::::OldHeader) } + if let Some(current_set_id) = current_set_id { + let actual_set_id = >::get().set_id; + if current_set_id != actual_set_id { + log::trace!( + target: crate::LOG_TARGET, + "Cannot finalize header signed by unknown authority set: bundled {:?}, best {:?}", + current_set_id, + actual_set_id, + ); + + return Err(Error::::InvalidAuthoritySetId) + } + } + Ok(()) } @@ -111,6 +134,18 @@ pub trait CallSubType, I: 'static>: return Some(submit_finality_proof_info_from_args::( finality_target, justification, + None, + )) + } else if let Some(crate::Call::::submit_finality_proof_ex { + finality_target, + justification, + current_set_id, + }) = self.is_sub_type() + { + return Some(submit_finality_proof_info_from_args::( + finality_target, + justification, + Some(*current_set_id), )) } @@ -133,7 +168,10 @@ pub trait CallSubType, I: 'static>: return InvalidTransaction::Call.into() } - match SubmitFinalityProofHelper::::check_obsolete(finality_target.block_number) { + match SubmitFinalityProofHelper::::check_obsolete( + finality_target.block_number, + finality_target.current_set_id, + ) { Ok(_) => Ok(ValidTransaction::default()), Err(Error::::OldHeader) => InvalidTransaction::Stale.into(), Err(_) => InvalidTransaction::Call.into(), @@ -150,6 +188,7 @@ impl, I: 'static> CallSubType for T::RuntimeCall where pub(crate) fn submit_finality_proof_info_from_args, I: 'static>( finality_target: &BridgedHeader, justification: &GrandpaJustification>, + current_set_id: Option, ) -> SubmitFinalityProofInfo> { let block_number = *finality_target.number(); @@ -191,7 +230,7 @@ pub(crate) fn submit_finality_proof_info_from_args, I: 'static>( ); let extra_size = actual_call_size.saturating_sub(max_expected_call_size); - SubmitFinalityProofInfo { block_number, extra_weight, extra_size } + SubmitFinalityProofInfo { block_number, current_set_id, extra_weight, extra_size } } #[cfg(test)] @@ -205,14 +244,17 @@ mod tests { use bp_runtime::{BasicOperatingMode, HeaderId}; use bp_test_utils::{ make_default_justification, make_justification_for_header, JustificationGeneratorParams, + TEST_GRANDPA_SET_ID, }; use frame_support::weights::Weight; use sp_runtime::{testing::DigestItem, traits::Header as _, SaturatedConversion}; fn validate_block_submit(num: TestNumber) -> bool { - let bridge_grandpa_call = crate::Call::::submit_finality_proof { + let bridge_grandpa_call = crate::Call::::submit_finality_proof_ex { finality_target: Box::new(test_header(num)), justification: make_default_justification(&test_header(num)), + // not initialized => zero + current_set_id: 0, }; RuntimeCall::check_obsolete_submit_finality_proof(&RuntimeCall::Grandpa( bridge_grandpa_call, @@ -275,9 +317,10 @@ mod tests { ..Default::default() }; let small_justification = make_justification_for_header(justification_params); - let small_call = RuntimeCall::Grandpa(crate::Call::submit_finality_proof { + let small_call = RuntimeCall::Grandpa(crate::Call::submit_finality_proof_ex { finality_target: Box::new(small_finality_target), justification: small_justification, + current_set_id: TEST_GRANDPA_SET_ID, }); assert_eq!(small_call.submit_finality_proof_info().unwrap().extra_size, 0); @@ -291,9 +334,10 @@ mod tests { ..Default::default() }; let large_justification = make_justification_for_header(justification_params); - let large_call = RuntimeCall::Grandpa(crate::Call::submit_finality_proof { + let large_call = RuntimeCall::Grandpa(crate::Call::submit_finality_proof_ex { finality_target: Box::new(large_finality_target), justification: large_justification, + current_set_id: TEST_GRANDPA_SET_ID, }); assert_ne!(large_call.submit_finality_proof_info().unwrap().extra_size, 0); } @@ -309,9 +353,10 @@ mod tests { // when there are `REASONABLE_HEADERS_IN_JUSTIFICATON_ANCESTRY` headers => no refund let justification = make_justification_for_header(justification_params.clone()); - let call = RuntimeCall::Grandpa(crate::Call::submit_finality_proof { + let call = RuntimeCall::Grandpa(crate::Call::submit_finality_proof_ex { finality_target: Box::new(finality_target.clone()), justification, + current_set_id: TEST_GRANDPA_SET_ID, }); assert_eq!(call.submit_finality_proof_info().unwrap().extra_weight, Weight::zero()); @@ -322,9 +367,10 @@ mod tests { justification.commit.precommits.len().saturated_into(), justification.votes_ancestries.len().saturated_into(), ); - let call = RuntimeCall::Grandpa(crate::Call::submit_finality_proof { + let call = RuntimeCall::Grandpa(crate::Call::submit_finality_proof_ex { finality_target: Box::new(finality_target), justification, + current_set_id: TEST_GRANDPA_SET_ID, }); assert_eq!(call.submit_finality_proof_info().unwrap().extra_weight, call_weight); } diff --git a/modules/grandpa/src/lib.rs b/modules/grandpa/src/lib.rs index 21600117af3..5d858b95aee 100644 --- a/modules/grandpa/src/lib.rs +++ b/modules/grandpa/src/lib.rs @@ -270,7 +270,7 @@ pub mod pallet { finality_target ); - SubmitFinalityProofHelper::::check_obsolete(number)?; + SubmitFinalityProofHelper::::check_obsolete(number, Some(current_set_id))?; let authority_set = >::get(); let unused_proof_size = authority_set.unused_proof_size(); @@ -286,7 +286,7 @@ pub mod pallet { // if we have seen too many mandatory headers in this block, we don't want to refund Self::free_mandatory_headers_remaining() > 0 && // if arguments out of expected bounds, we don't want to refund - submit_finality_proof_info_from_args::(&finality_target, &justification) + submit_finality_proof_info_from_args::(&finality_target, &justification, Some(current_set_id)) .fits_limits(); if may_refund_call_fee { FreeMandatoryHeadersRemaining::::mutate(|count| { @@ -699,6 +699,7 @@ mod tests { use bp_test_utils::{ authority_list, generate_owned_bridge_module_tests, make_default_justification, make_justification_for_header, JustificationGeneratorParams, ALICE, BOB, + TEST_GRANDPA_SET_ID, }; use codec::Encode; use frame_support::{ @@ -707,12 +708,10 @@ mod tests { storage::generator::StorageValue, }; use frame_system::{EventRecord, Phase}; - use sp_consensus_grandpa::{ConsensusLog, SetId, GRANDPA_ENGINE_ID}; + use sp_consensus_grandpa::{ConsensusLog, GRANDPA_ENGINE_ID}; use sp_core::Get; use sp_runtime::{Digest, DigestItem, DispatchError}; - const DEFAULT_SET_ID: SetId = 1; - fn initialize_substrate_bridge() { System::set_block_number(1); System::reset_events(); @@ -731,7 +730,7 @@ mod tests { let init_data = InitializationData { header: Box::new(genesis), authority_list: authority_list(), - set_id: DEFAULT_SET_ID, + set_id: TEST_GRANDPA_SET_ID, operating_mode: BasicOperatingMode::Normal, }; @@ -746,7 +745,7 @@ mod tests { RuntimeOrigin::signed(1), Box::new(header), justification, - DEFAULT_SET_ID, + TEST_GRANDPA_SET_ID, ) } @@ -1008,7 +1007,7 @@ mod tests { RuntimeOrigin::signed(1), Box::new(header.clone()), justification.clone(), - DEFAULT_SET_ID, + TEST_GRANDPA_SET_ID, ), >::InvalidJustification ); @@ -1038,7 +1037,7 @@ mod tests { RuntimeOrigin::signed(1), Box::new(header), justification, - DEFAULT_SET_ID, + TEST_GRANDPA_SET_ID, ), >::InvalidJustification ); @@ -1068,7 +1067,7 @@ mod tests { RuntimeOrigin::signed(1), Box::new(header), justification, - DEFAULT_SET_ID, + TEST_GRANDPA_SET_ID, ), >::InvalidAuthoritySet ); @@ -1107,7 +1106,7 @@ mod tests { RuntimeOrigin::signed(1), Box::new(header.clone()), justification.clone(), - DEFAULT_SET_ID, + TEST_GRANDPA_SET_ID, ); assert_ok!(result); assert_eq!(result.unwrap().pays_fee, frame_support::dispatch::Pays::No); @@ -1170,7 +1169,7 @@ mod tests { RuntimeOrigin::signed(1), Box::new(header.clone()), justification, - DEFAULT_SET_ID, + TEST_GRANDPA_SET_ID, ); assert_ok!(result); assert_eq!(result.unwrap().pays_fee, frame_support::dispatch::Pays::Yes); @@ -1202,7 +1201,7 @@ mod tests { RuntimeOrigin::signed(1), Box::new(header.clone()), justification, - DEFAULT_SET_ID, + TEST_GRANDPA_SET_ID, ); assert_ok!(result); assert_eq!(result.unwrap().pays_fee, frame_support::dispatch::Pays::Yes); @@ -1232,7 +1231,7 @@ mod tests { RuntimeOrigin::signed(1), Box::new(header), justification, - DEFAULT_SET_ID, + TEST_GRANDPA_SET_ID, ), >::UnsupportedScheduledChange ); @@ -1258,7 +1257,7 @@ mod tests { RuntimeOrigin::signed(1), Box::new(header), justification, - DEFAULT_SET_ID, + TEST_GRANDPA_SET_ID, ), >::UnsupportedScheduledChange ); @@ -1284,7 +1283,7 @@ mod tests { RuntimeOrigin::signed(1), Box::new(header), justification, - DEFAULT_SET_ID, + TEST_GRANDPA_SET_ID, ), >::TooManyAuthoritiesInSet ); @@ -1349,7 +1348,7 @@ mod tests { RuntimeOrigin::signed(1), Box::new(header), invalid_justification, - DEFAULT_SET_ID, + TEST_GRANDPA_SET_ID, ) }; @@ -1518,7 +1517,7 @@ mod tests { RuntimeOrigin::root(), Box::new(header), justification, - DEFAULT_SET_ID, + TEST_GRANDPA_SET_ID, ), DispatchError::BadOrigin, ); diff --git a/modules/parachains/src/lib.rs b/modules/parachains/src/lib.rs index 87c57e84622..1363a637604 100644 --- a/modules/parachains/src/lib.rs +++ b/modules/parachains/src/lib.rs @@ -731,6 +731,7 @@ pub(crate) mod tests { }; use bp_test_utils::{ authority_list, generate_owned_bridge_module_tests, make_default_justification, + TEST_GRANDPA_SET_ID, }; use frame_support::{ assert_noop, assert_ok, @@ -777,10 +778,11 @@ pub(crate) mod tests { let hash = header.hash(); let justification = make_default_justification(&header); assert_ok!( - pallet_bridge_grandpa::Pallet::::submit_finality_proof( + pallet_bridge_grandpa::Pallet::::submit_finality_proof_ex( RuntimeOrigin::signed(1), Box::new(header), justification.clone(), + TEST_GRANDPA_SET_ID, ) ); diff --git a/primitives/header-chain/src/lib.rs b/primitives/header-chain/src/lib.rs index f5485aca1ee..84a6a881a83 100644 --- a/primitives/header-chain/src/lib.rs +++ b/primitives/header-chain/src/lib.rs @@ -244,6 +244,16 @@ pub enum BridgeGrandpaCall { /// All data, required to initialize the pallet. init_data: InitializationData
, }, + /// `pallet-bridge-grandpa::Call::submit_finality_proof_ex` + #[codec(index = 4)] + submit_finality_proof_ex { + /// The header that we are going to finalize. + finality_target: Box
, + /// Finality justification for the `finality_target`. + justification: justification::GrandpaJustification
, + /// An identifier of the validators set, that have signed the justification. + current_set_id: SetId, + }, } /// The `BridgeGrandpaCall` used by a chain. From 2338cfe4eb82de68dc9db7c08be4d5919c56746e Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Tue, 6 Feb 2024 09:53:28 +0300 Subject: [PATCH 3/5] tests for new call --- .../src/refund_relayer_extension.rs | 267 +++++++++++++++++- modules/grandpa/src/call_ext.rs | 51 +++- modules/grandpa/src/lib.rs | 4 +- 3 files changed, 315 insertions(+), 7 deletions(-) diff --git a/bin/runtime-common/src/refund_relayer_extension.rs b/bin/runtime-common/src/refund_relayer_extension.rs index ede831899ea..bfcb82ad166 100644 --- a/bin/runtime-common/src/refund_relayer_extension.rs +++ b/bin/runtime-common/src/refund_relayer_extension.rs @@ -195,6 +195,19 @@ impl CallInfo { } } + /// Returns mutable reference to pre-dispatch `finality_target` sent to the + /// `SubmitFinalityProof` call. + #[cfg(test)] + fn submit_finality_proof_info_mut( + &mut self, + ) -> Option<&mut SubmitFinalityProofInfo> { + match *self { + Self::AllFinalityAndMsgs(ref mut info, _, _) => Some(info), + Self::RelayFinalityAndMsgs(ref mut info, _) => Some(info), + _ => None, + } + } + /// Returns the pre-dispatch `SubmitParachainHeadsInfo`. fn submit_parachain_heads_info(&self) -> Option<&SubmitParachainHeadsInfo> { match self { @@ -971,6 +984,22 @@ mod tests { ); let relay_justification = make_default_justification(&relay_header); + RuntimeCall::BridgeGrandpa(GrandpaCall::submit_finality_proof { + finality_target: Box::new(relay_header), + justification: relay_justification, + }) + } + + fn submit_relay_header_call_ex(relay_header_number: RelayBlockNumber) -> RuntimeCall { + let relay_header = BridgedChainHeader::new( + relay_header_number, + Default::default(), + Default::default(), + Default::default(), + Default::default(), + ); + let relay_justification = make_default_justification(&relay_header); + RuntimeCall::BridgeGrandpa(GrandpaCall::submit_finality_proof_ex { finality_target: Box::new(relay_header), justification: relay_justification, @@ -1060,6 +1089,18 @@ mod tests { }) } + fn relay_finality_and_delivery_batch_call_ex( + relay_header_number: RelayBlockNumber, + best_message: MessageNonce, + ) -> RuntimeCall { + RuntimeCall::Utility(UtilityCall::batch_all { + calls: vec![ + submit_relay_header_call_ex(relay_header_number), + message_delivery_call(best_message), + ], + }) + } + fn relay_finality_and_confirmation_batch_call( relay_header_number: RelayBlockNumber, best_message: MessageNonce, @@ -1072,6 +1113,18 @@ mod tests { }) } + fn relay_finality_and_confirmation_batch_call_ex( + relay_header_number: RelayBlockNumber, + best_message: MessageNonce, + ) -> RuntimeCall { + RuntimeCall::Utility(UtilityCall::batch_all { + calls: vec![ + submit_relay_header_call_ex(relay_header_number), + message_confirmation_call(best_message), + ], + }) + } + fn all_finality_and_delivery_batch_call( relay_header_number: RelayBlockNumber, parachain_head_at_relay_header_number: RelayBlockNumber, @@ -1086,6 +1139,20 @@ mod tests { }) } + fn all_finality_and_delivery_batch_call_ex( + relay_header_number: RelayBlockNumber, + parachain_head_at_relay_header_number: RelayBlockNumber, + best_message: MessageNonce, + ) -> RuntimeCall { + RuntimeCall::Utility(UtilityCall::batch_all { + calls: vec![ + submit_relay_header_call_ex(relay_header_number), + submit_parachain_head_call(parachain_head_at_relay_header_number), + message_delivery_call(best_message), + ], + }) + } + fn all_finality_and_confirmation_batch_call( relay_header_number: RelayBlockNumber, parachain_head_at_relay_header_number: RelayBlockNumber, @@ -1100,13 +1167,27 @@ mod tests { }) } + fn all_finality_and_confirmation_batch_call_ex( + relay_header_number: RelayBlockNumber, + parachain_head_at_relay_header_number: RelayBlockNumber, + best_message: MessageNonce, + ) -> RuntimeCall { + RuntimeCall::Utility(UtilityCall::batch_all { + calls: vec![ + submit_relay_header_call_ex(relay_header_number), + submit_parachain_head_call(parachain_head_at_relay_header_number), + message_confirmation_call(best_message), + ], + }) + } + fn all_finality_pre_dispatch_data() -> PreDispatchData { PreDispatchData { relayer: relayer_account_at_this_chain(), call_info: CallInfo::AllFinalityAndMsgs( SubmitFinalityProofInfo { block_number: 200, - current_set_id: Some(TEST_GRANDPA_SET_ID), + current_set_id: None, extra_weight: Weight::zero(), extra_size: 0, }, @@ -1130,13 +1211,20 @@ mod tests { } } + fn all_finality_pre_dispatch_data_ex() -> PreDispatchData { + let mut data = all_finality_pre_dispatch_data(); + data.call_info.submit_finality_proof_info_mut().unwrap().current_set_id = + Some(TEST_GRANDPA_SET_ID); + data + } + fn all_finality_confirmation_pre_dispatch_data() -> PreDispatchData { PreDispatchData { relayer: relayer_account_at_this_chain(), call_info: CallInfo::AllFinalityAndMsgs( SubmitFinalityProofInfo { block_number: 200, - current_set_id: Some(TEST_GRANDPA_SET_ID), + current_set_id: None, extra_weight: Weight::zero(), extra_size: 0, }, @@ -1156,13 +1244,20 @@ mod tests { } } + fn all_finality_confirmation_pre_dispatch_data_ex() -> PreDispatchData { + let mut data = all_finality_confirmation_pre_dispatch_data(); + data.call_info.submit_finality_proof_info_mut().unwrap().current_set_id = + Some(TEST_GRANDPA_SET_ID); + data + } + fn relay_finality_pre_dispatch_data() -> PreDispatchData { PreDispatchData { relayer: relayer_account_at_this_chain(), call_info: CallInfo::RelayFinalityAndMsgs( SubmitFinalityProofInfo { block_number: 200, - current_set_id: Some(TEST_GRANDPA_SET_ID), + current_set_id: None, extra_weight: Weight::zero(), extra_size: 0, }, @@ -1181,13 +1276,20 @@ mod tests { } } + fn relay_finality_pre_dispatch_data_ex() -> PreDispatchData { + let mut data = relay_finality_pre_dispatch_data(); + data.call_info.submit_finality_proof_info_mut().unwrap().current_set_id = + Some(TEST_GRANDPA_SET_ID); + data + } + fn relay_finality_confirmation_pre_dispatch_data() -> PreDispatchData { PreDispatchData { relayer: relayer_account_at_this_chain(), call_info: CallInfo::RelayFinalityAndMsgs( SubmitFinalityProofInfo { block_number: 200, - current_set_id: Some(TEST_GRANDPA_SET_ID), + current_set_id: None, extra_weight: Weight::zero(), extra_size: 0, }, @@ -1202,6 +1304,13 @@ mod tests { } } + fn relay_finality_confirmation_pre_dispatch_data_ex() -> PreDispatchData { + let mut data = relay_finality_confirmation_pre_dispatch_data(); + data.call_info.submit_finality_proof_info_mut().unwrap().current_set_id = + Some(TEST_GRANDPA_SET_ID); + data + } + fn parachain_finality_pre_dispatch_data() -> PreDispatchData { PreDispatchData { relayer: relayer_account_at_this_chain(), @@ -1398,6 +1507,10 @@ mod tests { run_validate(all_finality_and_delivery_batch_call(200, 200, 200)), Ok(Default::default()), ); + assert_eq!( + run_validate(all_finality_and_delivery_batch_call_ex(200, 200, 200)), + Ok(Default::default()), + ); // message confirmation validation is passing assert_eq!( run_validate_ignore_priority(message_confirmation_call(200)), @@ -1415,6 +1528,12 @@ mod tests { )), Ok(Default::default()), ); + assert_eq!( + run_validate_ignore_priority(all_finality_and_confirmation_batch_call_ex( + 200, 200, 200 + )), + Ok(Default::default()), + ); }); } @@ -1505,12 +1624,24 @@ mod tests { run_validate_ignore_priority(all_finality_and_delivery_batch_call(200, 200, 200)), Ok(ValidTransaction::default()), ); + assert_eq!( + run_validate_ignore_priority(all_finality_and_delivery_batch_call_ex( + 200, 200, 200 + )), + Ok(ValidTransaction::default()), + ); assert_eq!( run_validate_ignore_priority(all_finality_and_confirmation_batch_call( 200, 200, 200 )), Ok(ValidTransaction::default()), ); + assert_eq!( + run_validate_ignore_priority(all_finality_and_confirmation_batch_call_ex( + 200, 200, 200 + )), + Ok(ValidTransaction::default()), + ); }); } @@ -1523,11 +1654,19 @@ mod tests { run_pre_dispatch(all_finality_and_delivery_batch_call(100, 200, 200)), Err(TransactionValidityError::Invalid(InvalidTransaction::Stale)), ); + assert_eq!( + run_pre_dispatch(all_finality_and_delivery_batch_call_ex(100, 200, 200)), + Err(TransactionValidityError::Invalid(InvalidTransaction::Stale)), + ); assert_eq!( run_validate(all_finality_and_delivery_batch_call(100, 200, 200)), Err(TransactionValidityError::Invalid(InvalidTransaction::Stale)), ); + assert_eq!( + run_validate(all_finality_and_delivery_batch_call_ex(100, 200, 200)), + Err(TransactionValidityError::Invalid(InvalidTransaction::Stale)), + ); }); } @@ -1540,10 +1679,18 @@ mod tests { run_pre_dispatch(all_finality_and_delivery_batch_call(101, 100, 200)), Err(TransactionValidityError::Invalid(InvalidTransaction::Stale)), ); + assert_eq!( + run_pre_dispatch(all_finality_and_delivery_batch_call_ex(101, 100, 200)), + Err(TransactionValidityError::Invalid(InvalidTransaction::Stale)), + ); assert_eq!( run_validate(all_finality_and_delivery_batch_call(101, 100, 200)), Err(TransactionValidityError::Invalid(InvalidTransaction::Stale)), ); + assert_eq!( + run_validate(all_finality_and_delivery_batch_call_ex(101, 100, 200)), + Err(TransactionValidityError::Invalid(InvalidTransaction::Stale)), + ); assert_eq!( run_pre_dispatch(parachain_finality_and_delivery_batch_call(100, 200)), @@ -1565,19 +1712,35 @@ mod tests { run_pre_dispatch(all_finality_and_delivery_batch_call(200, 200, 100)), Err(TransactionValidityError::Invalid(InvalidTransaction::Stale)), ); + assert_eq!( + run_pre_dispatch(all_finality_and_delivery_batch_call_ex(200, 200, 100)), + Err(TransactionValidityError::Invalid(InvalidTransaction::Stale)), + ); assert_eq!( run_pre_dispatch(all_finality_and_confirmation_batch_call(200, 200, 100)), Err(TransactionValidityError::Invalid(InvalidTransaction::Stale)), ); + assert_eq!( + run_pre_dispatch(all_finality_and_confirmation_batch_call_ex(200, 200, 100)), + Err(TransactionValidityError::Invalid(InvalidTransaction::Stale)), + ); assert_eq!( run_validate(all_finality_and_delivery_batch_call(200, 200, 100)), Err(TransactionValidityError::Invalid(InvalidTransaction::Stale)), ); + assert_eq!( + run_validate(all_finality_and_delivery_batch_call_ex(200, 200, 100)), + Err(TransactionValidityError::Invalid(InvalidTransaction::Stale)), + ); assert_eq!( run_validate(all_finality_and_confirmation_batch_call(200, 200, 100)), Err(TransactionValidityError::Invalid(InvalidTransaction::Stale)), ); + assert_eq!( + run_validate(all_finality_and_confirmation_batch_call_ex(200, 200, 100)), + Err(TransactionValidityError::Invalid(InvalidTransaction::Stale)), + ); assert_eq!( run_pre_dispatch(parachain_finality_and_delivery_batch_call(200, 100)), @@ -1614,10 +1777,18 @@ mod tests { run_pre_dispatch(all_finality_and_delivery_batch_call(200, 200, 200)), Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), ); + assert_eq!( + run_pre_dispatch(all_finality_and_delivery_batch_call_ex(200, 200, 200)), + Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), + ); assert_eq!( run_pre_dispatch(all_finality_and_confirmation_batch_call(200, 200, 200)), Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), ); + assert_eq!( + run_pre_dispatch(all_finality_and_confirmation_batch_call_ex(200, 200, 200)), + Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), + ); }); } @@ -1636,10 +1807,18 @@ mod tests { run_pre_dispatch(all_finality_and_delivery_batch_call(200, 200, 200)), Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), ); + assert_eq!( + run_pre_dispatch(all_finality_and_delivery_batch_call_ex(200, 200, 200)), + Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), + ); assert_eq!( run_pre_dispatch(all_finality_and_confirmation_batch_call(200, 200, 200)), Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), ); + assert_eq!( + run_pre_dispatch(all_finality_and_confirmation_batch_call_ex(200, 200, 200)), + Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), + ); assert_eq!( run_pre_dispatch(parachain_finality_and_delivery_batch_call(200, 200)), @@ -1667,10 +1846,18 @@ mod tests { run_pre_dispatch(all_finality_and_delivery_batch_call(200, 200, 200)), Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), ); + assert_eq!( + run_pre_dispatch(all_finality_and_delivery_batch_call_ex(200, 200, 200)), + Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), + ); assert_eq!( run_pre_dispatch(all_finality_and_confirmation_batch_call(200, 200, 200)), Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), ); + assert_eq!( + run_pre_dispatch(all_finality_and_confirmation_batch_call_ex(200, 200, 200)), + Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), + ); assert_eq!( run_pre_dispatch(parachain_finality_and_delivery_batch_call(200, 200)), @@ -1701,10 +1888,18 @@ mod tests { run_pre_dispatch(all_finality_and_delivery_batch_call(200, 200, 200)), Ok(Some(all_finality_pre_dispatch_data())), ); + assert_eq!( + run_pre_dispatch(all_finality_and_delivery_batch_call_ex(200, 200, 200)), + Ok(Some(all_finality_pre_dispatch_data_ex())), + ); assert_eq!( run_pre_dispatch(all_finality_and_confirmation_batch_call(200, 200, 200)), Ok(Some(all_finality_confirmation_pre_dispatch_data())), ); + assert_eq!( + run_pre_dispatch(all_finality_and_confirmation_batch_call_ex(200, 200, 200)), + Ok(Some(all_finality_confirmation_pre_dispatch_data_ex())), + ); }); } @@ -2131,6 +2326,12 @@ mod tests { ), Ok(None), ); + assert_eq!( + TestGrandpaExtensionProvider::parse_and_check_for_obsolete_call( + &all_finality_and_delivery_batch_call_ex(200, 200, 200) + ), + Ok(None), + ); // relay + parachain + message confirmation calls batch is ignored assert_eq!( @@ -2139,6 +2340,12 @@ mod tests { ), Ok(None), ); + assert_eq!( + TestGrandpaExtensionProvider::parse_and_check_for_obsolete_call( + &all_finality_and_confirmation_batch_call_ex(200, 200, 200) + ), + Ok(None), + ); // parachain + message delivery call batch is ignored assert_eq!( @@ -2163,6 +2370,12 @@ mod tests { ), Ok(Some(relay_finality_pre_dispatch_data().call_info)), ); + assert_eq!( + TestGrandpaExtensionProvider::parse_and_check_for_obsolete_call( + &relay_finality_and_delivery_batch_call_ex(200, 200) + ), + Ok(Some(relay_finality_pre_dispatch_data_ex().call_info)), + ); // relay + message confirmation call batch is accepted assert_eq!( @@ -2171,6 +2384,12 @@ mod tests { ), Ok(Some(relay_finality_confirmation_pre_dispatch_data().call_info)), ); + assert_eq!( + TestGrandpaExtensionProvider::parse_and_check_for_obsolete_call( + &relay_finality_and_confirmation_batch_call_ex(200, 200) + ), + Ok(Some(relay_finality_confirmation_pre_dispatch_data_ex().call_info)), + ); // message delivery call batch is accepted assert_eq!( @@ -2199,11 +2418,19 @@ mod tests { run_grandpa_pre_dispatch(relay_finality_and_delivery_batch_call(100, 200)), Err(TransactionValidityError::Invalid(InvalidTransaction::Stale)), ); + assert_eq!( + run_grandpa_pre_dispatch(relay_finality_and_delivery_batch_call_ex(100, 200)), + Err(TransactionValidityError::Invalid(InvalidTransaction::Stale)), + ); assert_eq!( run_grandpa_validate(relay_finality_and_delivery_batch_call(100, 200)), Err(TransactionValidityError::Invalid(InvalidTransaction::Stale)), ); + assert_eq!( + run_grandpa_validate(relay_finality_and_delivery_batch_call_ex(100, 200)), + Err(TransactionValidityError::Invalid(InvalidTransaction::Stale)), + ); }); } @@ -2216,19 +2443,35 @@ mod tests { run_grandpa_pre_dispatch(relay_finality_and_delivery_batch_call(200, 100)), Err(TransactionValidityError::Invalid(InvalidTransaction::Stale)), ); + assert_eq!( + run_grandpa_pre_dispatch(relay_finality_and_delivery_batch_call_ex(200, 100)), + Err(TransactionValidityError::Invalid(InvalidTransaction::Stale)), + ); assert_eq!( run_grandpa_pre_dispatch(relay_finality_and_confirmation_batch_call(200, 100)), Err(TransactionValidityError::Invalid(InvalidTransaction::Stale)), ); + assert_eq!( + run_grandpa_pre_dispatch(relay_finality_and_confirmation_batch_call_ex(200, 100)), + Err(TransactionValidityError::Invalid(InvalidTransaction::Stale)), + ); assert_eq!( run_grandpa_validate(relay_finality_and_delivery_batch_call(200, 100)), Err(TransactionValidityError::Invalid(InvalidTransaction::Stale)), ); + assert_eq!( + run_grandpa_validate(relay_finality_and_delivery_batch_call_ex(200, 100)), + Err(TransactionValidityError::Invalid(InvalidTransaction::Stale)), + ); assert_eq!( run_grandpa_validate(relay_finality_and_confirmation_batch_call(200, 100)), Err(TransactionValidityError::Invalid(InvalidTransaction::Stale)), ); + assert_eq!( + run_grandpa_validate(relay_finality_and_confirmation_batch_call_ex(200, 100)), + Err(TransactionValidityError::Invalid(InvalidTransaction::Stale)), + ); assert_eq!( run_grandpa_pre_dispatch(message_delivery_call(100)), @@ -2259,19 +2502,35 @@ mod tests { run_grandpa_pre_dispatch(relay_finality_and_delivery_batch_call(200, 200)), Ok(Some(relay_finality_pre_dispatch_data()),) ); + assert_eq!( + run_grandpa_pre_dispatch(relay_finality_and_delivery_batch_call_ex(200, 200)), + Ok(Some(relay_finality_pre_dispatch_data_ex()),) + ); assert_eq!( run_grandpa_pre_dispatch(relay_finality_and_confirmation_batch_call(200, 200)), Ok(Some(relay_finality_confirmation_pre_dispatch_data())), ); + assert_eq!( + run_grandpa_pre_dispatch(relay_finality_and_confirmation_batch_call_ex(200, 200)), + Ok(Some(relay_finality_confirmation_pre_dispatch_data_ex())), + ); assert_eq!( run_grandpa_validate(relay_finality_and_delivery_batch_call(200, 200)), Ok(Default::default()), ); + assert_eq!( + run_grandpa_validate(relay_finality_and_delivery_batch_call_ex(200, 200)), + Ok(Default::default()), + ); assert_eq!( run_grandpa_validate(relay_finality_and_confirmation_batch_call(200, 200)), Ok(Default::default()), ); + assert_eq!( + run_grandpa_validate(relay_finality_and_confirmation_batch_call_ex(200, 200)), + Ok(Default::default()), + ); assert_eq!( run_grandpa_pre_dispatch(message_delivery_call(200)), diff --git a/modules/grandpa/src/call_ext.rs b/modules/grandpa/src/call_ext.rs index 73e1f4a5402..e3c778b480b 100644 --- a/modules/grandpa/src/call_ext.rs +++ b/modules/grandpa/src/call_ext.rs @@ -238,7 +238,8 @@ mod tests { use crate::{ call_ext::CallSubType, mock::{run_test, test_header, RuntimeCall, TestBridgedChain, TestNumber, TestRuntime}, - BestFinalized, Config, PalletOperatingMode, WeightInfo, + BestFinalized, Config, CurrentAuthoritySet, PalletOperatingMode, StoredAuthoritySet, + SubmitFinalityProofInfo, WeightInfo, }; use bp_header_chain::ChainWithGrandpa; use bp_runtime::{BasicOperatingMode, HeaderId}; @@ -298,6 +299,18 @@ mod tests { }); } + #[test] + fn extension_rejects_new_header_if_set_id_is_invalid() { + run_test(|| { + // when set id is different from the passed one => tx is rejected + sync_to_header_10(); + let next_set = StoredAuthoritySet::::try_new(vec![], 0x42).unwrap(); + CurrentAuthoritySet::::put(next_set); + + assert!(!validate_block_submit(15)); + }); + } + #[test] fn extension_accepts_new_header() { run_test(|| { @@ -308,6 +321,42 @@ mod tests { }); } + #[test] + fn submit_finality_proof_info_is_parsed() { + // when `submit_finality_proof` is used, `current_set_id` is set to `None` + let deprecated_call = + RuntimeCall::Grandpa(crate::Call::::submit_finality_proof { + finality_target: Box::new(test_header(42)), + justification: make_default_justification(&test_header(42)), + }); + assert_eq!( + deprecated_call.submit_finality_proof_info(), + Some(SubmitFinalityProofInfo { + block_number: 42, + current_set_id: None, + extra_weight: Weight::zero(), + extra_size: 0, + }) + ); + + // when `submit_finality_proof_ex` is used, `current_set_id` is set to `Some` + let deprecated_call = + RuntimeCall::Grandpa(crate::Call::::submit_finality_proof_ex { + finality_target: Box::new(test_header(42)), + justification: make_default_justification(&test_header(42)), + current_set_id: 777, + }); + assert_eq!( + deprecated_call.submit_finality_proof_info(), + Some(SubmitFinalityProofInfo { + block_number: 42, + current_set_id: Some(777), + extra_weight: Weight::zero(), + extra_size: 0, + }) + ); + } + #[test] fn extension_returns_correct_extra_size_if_call_arguments_are_too_large() { // when call arguments are below our limit => no refund diff --git a/modules/grandpa/src/lib.rs b/modules/grandpa/src/lib.rs index 5d858b95aee..072ea0a56f1 100644 --- a/modules/grandpa/src/lib.rs +++ b/modules/grandpa/src/lib.rs @@ -270,13 +270,13 @@ pub mod pallet { finality_target ); + // it checks whether the `number` is better than the current best block number + // and whether the `current_set_id` matches the best known set id SubmitFinalityProofHelper::::check_obsolete(number, Some(current_set_id))?; let authority_set = >::get(); let unused_proof_size = authority_set.unused_proof_size(); let set_id = authority_set.set_id; - ensure!(current_set_id == set_id, >::InvalidAuthoritySetId); - let authority_set: AuthoritySet = authority_set.into(); verify_justification::(&justification, hash, number, authority_set)?; From cef9865dacf42a334b8e5bc30b75f2cc47391829 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Tue, 6 Feb 2024 09:53:46 +0300 Subject: [PATCH 4/5] suppress deprecation warning --- modules/grandpa/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/modules/grandpa/src/lib.rs b/modules/grandpa/src/lib.rs index 072ea0a56f1..ce2c47da954 100644 --- a/modules/grandpa/src/lib.rs +++ b/modules/grandpa/src/lib.rs @@ -159,6 +159,7 @@ pub mod pallet { justification.commit.precommits.len().saturated_into(), justification.votes_ancestries.len().saturated_into(), ))] + #[allow(deprecated)] #[deprecated( note = "`submit_finality_proof` will be removed in May 2024. Use `submit_finality_proof_ex` instead." )] From b1bdcb8522464e46a32ab9f687fe558d1333e658 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Tue, 6 Feb 2024 09:56:58 +0300 Subject: [PATCH 5/5] revert changes to benchmarks to avoid unnecessary compilation issues when integrating to other repos --- modules/grandpa/src/benchmarking.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/modules/grandpa/src/benchmarking.rs b/modules/grandpa/src/benchmarking.rs index 2b3e8b992c5..11033373ce4 100644 --- a/modules/grandpa/src/benchmarking.rs +++ b/modules/grandpa/src/benchmarking.rs @@ -16,8 +16,9 @@ //! Benchmarks for the GRANDPA Pallet. //! -//! The main dispatchable for the GRANDPA pallet is `submit_finality_proof_ex`, so these benchmarks -//! are based around that. There are to main factors which affect finality proof verification: +//! The main dispatchable for the GRANDPA pallet is `submit_finality_proof_ex`. Our benchmarks +//! are based around `submit_finality_proof`, though - from weight PoV they are the same calls. +//! There are to main factors which affect finality proof verification: //! //! 1. The number of `votes-ancestries` in the justification //! 2. The number of `pre-commits` in the justification @@ -77,7 +78,7 @@ fn precommits_range_end, I: 'static>() -> u32 { required_justification_precommits(max_bridged_authorities) } -/// Prepare header and its justification to submit using `submit_finality_proof_ex`. +/// Prepare header and its justification to submit using `submit_finality_proof`. fn prepare_benchmark_data, I: 'static>( precommits: u32, ancestors: u32, @@ -118,13 +119,12 @@ fn prepare_benchmark_data, I: 'static>( benchmarks_instance_pallet! { // This is the "gold standard" benchmark for this extrinsic, and it's what should be used to // annotate the weight in the pallet. - submit_finality_proof_ex { + submit_finality_proof { let p in 1 .. precommits_range_end::(); let v in MAX_VOTE_ANCESTRIES_RANGE_BEGIN..MAX_VOTE_ANCESTRIES_RANGE_END; let caller: T::AccountId = whitelisted_caller(); let (header, justification) = prepare_benchmark_data::(p, v); - let set_id = TEST_GRANDPA_SET_ID; - }: submit_finality_proof_ex(RawOrigin::Signed(caller), Box::new(header), justification, set_id) + }: submit_finality_proof(RawOrigin::Signed(caller), Box::new(header), justification) verify { let genesis_header: BridgedHeader = bp_test_utils::test_header(Zero::zero()); let header: BridgedHeader = bp_test_utils::test_header(One::one());