Remove pallet::without_storage_info from bridge GRANDPA pallet (#1478)

* remove pallet::without_storage_info from bridge GRANDPA pallet * StoredBridgedHeader * spelling * fix benchmarks * MAX_BRIDGED_AUTHORITIES: 256 -> 2048 * Update modules/grandpa/src/storage_types.rs Co-authored-by: Adrian Catangiu <adrian@parity.io> * Update modules/grandpa/src/storage_types.rs Co-authored-by: Adrian Catangiu <adrian@parity.io> * moved max authorities + header size to chain primitives * removed unused code * new -> try_new * fix benchmarks compilation Co-authored-by: Adrian Catangiu <adrian@parity.io>
2026-05-31 08:41:02 +00:00 · 2022-10-10 11:32:53 +03:00
parent 6f9bda5db0
commit f38852f661
12 changed files with 349 additions and 66 deletions
@@ -36,6 +36,8 @@
 // Runtime-generated enums
 #![allow(clippy::large_enum_variant)]

+use storage_types::{StoredAuthoritySet, StoredBridgedHeader};
+
 use bp_header_chain::{justification::GrandpaJustification, InitializationData};
 use bp_runtime::{BlockNumberOf, Chain, HashOf, HasherOf, HeaderOf, OwnedBridgeModule};
 use finality_grandpa::voter_set::VoterSet;
@@ -48,6 +50,7 @@ use sp_std::{boxed::Box, convert::TryInto};
 mod extension;
 #[cfg(test)]
 mod mock;
+mod storage_types;

 /// Module, containing weights for this pallet.
 pub mod weights;
@@ -102,12 +105,24 @@ pub mod pallet {
 		#[pallet::constant]
 		type HeadersToKeep: Get<u32>;

+		/// Max number of authorities at the bridged chain.
+		#[pallet::constant]
+		type MaxBridgedAuthorities: Get<u32>;
+		/// Maximal size (in bytes) of the SCALE-encoded bridged chain header.
+		///
+		/// This constant must be selected with care. The pallet requires mandatory headers to be
+		/// submitted to be able to proceed. Mandatory headers contain public keys of all GRANDPA
+		/// authorities. E.g. for 1024 authorities, the size of encoded keys will be at least 32 KB.
+		/// The same header may also contain other digest items as well, so some reserve here
+		/// is required.
+		#[pallet::constant]
+		type MaxBridgedHeaderSize: Get<u32>;
+
 		/// Weights gathered through benchmarking.
 		type WeightInfo: WeightInfo;
 	}

 	#[pallet::pallet]
-	#[pallet::without_storage_info]
 	pub struct Pallet<T, I = ()>(PhantomData<(T, I)>);

 	#[pallet::hooks]
@@ -180,12 +195,14 @@ pub mod pallet {

 			let authority_set = <CurrentAuthoritySet<T, I>>::get();
 			let set_id = authority_set.set_id;
-			verify_justification::<T, I>(&justification, hash, *number, authority_set)?;
+			verify_justification::<T, I>(&justification, hash, *number, authority_set.into())?;

 			let is_authorities_change_enacted =
 				try_enact_authority_change::<T, I>(&finality_target, set_id)?;
+			let finality_target =
+				StoredBridgedHeader::<T, I>::try_from_bridged_header(*finality_target)?;
 			<RequestCount<T, I>>::mutate(|count| *count += 1);
-			insert_header::<T, I>(*finality_target, hash);
+			insert_header::<T, I>(finality_target, hash);
 			log::info!(
 				target: LOG_TARGET,
 				"Successfully imported finalized header with hash {:?}!",
@@ -221,7 +238,7 @@ pub mod pallet {

 			let init_allowed = !<BestFinalized<T, I>>::exists();
 			ensure!(init_allowed, <Error<T, I>>::AlreadyInitialized);
-			initialize_bridge::<T, I>(init_data.clone());
+			initialize_bridge::<T, I>(init_data.clone())?;

 			log::info!(
 				target: LOG_TARGET,
@@ -286,12 +303,12 @@ pub mod pallet {
 	/// Headers which have been imported into the pallet.
 	#[pallet::storage]
 	pub type ImportedHeaders<T: Config<I>, I: 'static = ()> =
-		StorageMap<_, Identity, BridgedBlockHash<T, I>, BridgedHeader<T, I>>;
+		StorageMap<_, Identity, BridgedBlockHash<T, I>, StoredBridgedHeader<T, I>>;

 	/// The current GRANDPA Authority set.
 	#[pallet::storage]
 	pub(super) type CurrentAuthoritySet<T: Config<I>, I: 'static = ()> =
-		StorageValue<_, bp_header_chain::AuthoritySet, ValueQuery>;
+		StorageValue<_, StoredAuthoritySet<T, I>, ValueQuery>;

 	/// Optional pallet owner.
 	///
@@ -333,7 +350,7 @@ pub mod pallet {
 			}

 			if let Some(init_data) = self.init_data.clone() {
-				initialize_bridge::<T, I>(init_data);
+				initialize_bridge::<T, I>(init_data).expect("genesis config is correct; qed");
 			} else {
 				// Since the bridge hasn't been initialized we shouldn't allow anyone to perform
 				// transactions.
@@ -364,6 +381,10 @@ pub mod pallet {
 		AlreadyInitialized,
 		/// The storage proof doesn't contains storage root. So it is invalid for given header.
 		StorageRootMismatch,
+		/// Too many authorities in the set.
+		TooManyAuthoritiesInSet,
+		/// Too large header.
+		TooLargeHeader,
 		/// Error generated by the `OwnedBridgeModule` trait.
 		BridgeModule(bp_runtime::OwnedBridgeModuleError),
 	}
@@ -392,8 +413,11 @@ pub mod pallet {
 			ensure!(change.delay == Zero::zero(), <Error<T, I>>::UnsupportedScheduledChange);

 			// TODO [#788]: Stop manually increasing the `set_id` here.
-			let next_authorities = bp_header_chain::AuthoritySet {
-				authorities: change.next_authorities,
+			let next_authorities = StoredAuthoritySet::<T, I> {
+				authorities: change
+					.next_authorities
+					.try_into()
+					.map_err(|_| Error::<T, I>::TooManyAuthoritiesInSet)?,
 				set_id: current_set_id + 1,
 			};

@@ -454,7 +478,7 @@ pub mod pallet {
 	/// Note this function solely takes care of updating the storage and pruning old entries,
 	/// but does not verify the validity of such import.
 	pub(crate) fn insert_header<T: Config<I>, I: 'static>(
-		header: BridgedHeader<T, I>,
+		header: StoredBridgedHeader<T, I>,
 		hash: BridgedBlockHash<T, I>,
 	) {
 		let index = <ImportedHashesPointer<T, I>>::get();
@@ -475,19 +499,23 @@ pub mod pallet {
 	/// were called by a trusted origin.
 	pub(crate) fn initialize_bridge<T: Config<I>, I: 'static>(
 		init_params: super::InitializationData<BridgedHeader<T, I>>,
-	) {
+	) -> Result<(), Error<T, I>> {
 		let super::InitializationData { header, authority_list, set_id, operating_mode } =
 			init_params;
+		let authority_set = StoredAuthoritySet::<T, I>::try_new(authority_list, set_id)
+			.map_err(|_| Error::TooManyAuthoritiesInSet)?;
+		let header = StoredBridgedHeader::<T, I>::try_from_bridged_header(*header)?;

 		let initial_hash = header.hash();
 		<InitialHash<T, I>>::put(initial_hash);
 		<ImportedHashesPointer<T, I>>::put(0);
-		insert_header::<T, I>(*header, initial_hash);
+		insert_header::<T, I>(header, initial_hash);

-		let authority_set = bp_header_chain::AuthoritySet::new(authority_list, set_id);
 		<CurrentAuthoritySet<T, I>>::put(authority_set);

 		<PalletOperatingMode<T, I>>::put(operating_mode);
+
+		Ok(())
 	}

 	#[cfg(feature = "runtime-benchmarks")]
@@ -496,7 +524,7 @@ pub mod pallet {
 	) {
 		let start_number = *init_params.header.number();
 		let end_number = start_number + T::HeadersToKeep::get().into();
-		initialize_bridge::<T, I>(init_params);
+		initialize_bridge::<T, I>(init_params).expect("benchmarks are correct");

 		let mut number = start_number;
 		while number < end_number {
@@ -509,7 +537,11 @@ pub mod pallet {
 				Default::default(),
 			);
 			let hash = header.hash();
-			insert_header::<T, I>(header, hash);
+			insert_header::<T, I>(
+				StoredBridgedHeader::try_from_bridged_header(header)
+					.expect("only used from benchmarks; benchmarks are correct; qed"),
+				hash,
+			);
 		}
 	}
 }
@@ -521,7 +553,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 	/// if the pallet has not been initialized yet.
 	pub fn best_finalized() -> Option<BridgedHeader<T, I>> {
 		let (_, hash) = <BestFinalized<T, I>>::get()?;
-		<ImportedHeaders<T, I>>::get(hash)
+		<ImportedHeaders<T, I>>::get(hash).map(|h| h.0)
 	}

 	/// Check if a particular header is known to the bridge pallet.
@@ -591,13 +623,17 @@ pub fn initialize_for_benchmarks<T: Config<I>, I: 'static>(header: BridgedHeader
 		                                          * benchmarks */
 		set_id: 0,
 		operating_mode: bp_runtime::BasicOperatingMode::Normal,
-	});
+	})
+	.expect("only used from benchmarks; benchmarks are correct; qed");
 }

 #[cfg(test)]
 mod tests {
 	use super::*;
-	use crate::mock::{run_test, test_header, Origin, TestHeader, TestNumber, TestRuntime};
+	use crate::mock::{
+		run_test, test_header, Origin, TestHeader, TestNumber, TestRuntime,
+		MAX_BRIDGED_AUTHORITIES, MAX_HEADER_SIZE,
+	};
 	use bp_runtime::BasicOperatingMode;
 	use bp_test_utils::{
 		authority_list, generate_owned_bridge_module_tests, make_default_justification,
@@ -673,6 +709,22 @@ mod tests {
 		Digest { logs: vec![DigestItem::Consensus(GRANDPA_ENGINE_ID, consensus_log.encode())] }
 	}

+	fn many_authorities_log() -> Digest {
+		let consensus_log =
+			ConsensusLog::<TestNumber>::ScheduledChange(sp_finality_grandpa::ScheduledChange {
+				next_authorities: std::iter::repeat((ALICE.into(), 1))
+					.take(MAX_BRIDGED_AUTHORITIES as usize + 1)
+					.collect(),
+				delay: 0,
+			});
+
+		Digest { logs: vec![DigestItem::Consensus(GRANDPA_ENGINE_ID, consensus_log.encode())] }
+	}
+
+	fn large_digest() -> Digest {
+		Digest { logs: vec![DigestItem::Other(vec![42; MAX_HEADER_SIZE as _])] }
+	}
+
 	#[test]
 	fn init_root_or_owner_origin_can_initialize_pallet() {
 		run_test(|| {
@@ -715,6 +767,45 @@ mod tests {
 		})
 	}

+	#[test]
+	fn init_fails_if_there_are_too_many_authorities_in_the_set() {
+		run_test(|| {
+			let genesis = test_header(0);
+			let init_data = InitializationData {
+				header: Box::new(genesis),
+				authority_list: std::iter::repeat(authority_list().remove(0))
+					.take(MAX_BRIDGED_AUTHORITIES as usize + 1)
+					.collect(),
+				set_id: 1,
+				operating_mode: BasicOperatingMode::Normal,
+			};
+
+			assert_noop!(
+				Pallet::<TestRuntime>::initialize(Origin::root(), init_data),
+				Error::<TestRuntime>::TooManyAuthoritiesInSet,
+			);
+		});
+	}
+
+	#[test]
+	fn init_fails_if_header_is_too_large() {
+		run_test(|| {
+			let mut genesis = test_header(0);
+			genesis.digest = large_digest();
+			let init_data = InitializationData {
+				header: Box::new(genesis),
+				authority_list: authority_list(),
+				set_id: 1,
+				operating_mode: BasicOperatingMode::Normal,
+			};
+
+			assert_noop!(
+				Pallet::<TestRuntime>::initialize(Origin::root(), init_data),
+				Error::<TestRuntime>::TooLargeHeader,
+			);
+		});
+	}
+
 	#[test]
 	fn pallet_rejects_transactions_if_halted() {
 		run_test(|| {
@@ -880,7 +971,8 @@ mod tests {
 			// Make sure that the authority set actually changed upon importing our header
 			assert_eq!(
 				<CurrentAuthoritySet<TestRuntime>>::get(),
-				bp_header_chain::AuthoritySet::new(next_authorities, next_set_id),
+				StoredAuthoritySet::<TestRuntime, ()>::try_new(next_authorities, next_set_id)
+					.unwrap(),
 			);
 		})
 	}
@@ -935,6 +1027,56 @@ mod tests {
 		})
 	}

+	#[test]
+	fn importing_header_rejects_header_with_too_many_authorities() {
+		run_test(|| {
+			initialize_substrate_bridge();
+
+			// Need to update the header digest to indicate that our header signals an authority set
+			// change. However, the change doesn't happen until the next block.
+			let mut header = test_header(2);
+			header.digest = many_authorities_log();
+
+			// Create a valid justification for the header
+			let justification = make_default_justification(&header);
+
+			// Should not be allowed to import this header
+			assert_err!(
+				Pallet::<TestRuntime>::submit_finality_proof(
+					Origin::signed(1),
+					Box::new(header),
+					justification
+				),
+				<Error<TestRuntime>>::TooManyAuthoritiesInSet
+			);
+		});
+	}
+
+	#[test]
+	fn importing_header_rejects_header_if_it_is_too_large() {
+		run_test(|| {
+			initialize_substrate_bridge();
+
+			// Need to update the header digest to indicate that our header signals an authority set
+			// change. However, the change doesn't happen until the next block.
+			let mut header = test_header(2);
+			header.digest = large_digest();
+
+			// Create a valid justification for the header
+			let justification = make_default_justification(&header);
+
+			// Should not be allowed to import this header
+			assert_err!(
+				Pallet::<TestRuntime>::submit_finality_proof(
+					Origin::signed(1),
+					Box::new(header),
+					justification
+				),
+				<Error<TestRuntime>>::TooLargeHeader
+			);
+		});
+	}
+
 	#[test]
 	fn parse_finalized_storage_proof_rejects_proof_on_unknown_header() {
 		run_test(|| {
@@ -959,7 +1101,10 @@ mod tests {

 			let hash = header.hash();
 			<BestFinalized<TestRuntime>>::put((2, hash));
-			<ImportedHeaders<TestRuntime>>::insert(hash, header);
+			<ImportedHeaders<TestRuntime>>::insert(
+				hash,
+				StoredBridgedHeader::try_from_bridged_header(header).unwrap(),
+			);

 			assert_ok!(
 				Pallet::<TestRuntime>::parse_finalized_storage_proof(hash, storage_proof, |_| (),),