Adds consistency checks for the HostConfiguration (#2087)

* Adds consistency checks for the `HostConfiguration`

Besides that it fixes the chain specs to make the consistency checks happy.

* Update runtime/parachains/src/configuration.rs

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>

* Review feedback and test fixes etc

* Update

* More

* I'm an idiot

* Fix tests...

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>

polkadot/runtime/parachains/src/configuration.rs

@@ -28,9 +28,10 @@ use frame_support::{
 };
 use parity_scale_codec::{Encode, Decode};
 use frame_system::ensure_root;
+use sp_runtime::traits::Zero;
 
 /// All configuration of the runtime with respect to parachains and parathreads.
-#[derive(Clone, Encode, Decode, PartialEq, Default, sp_core::RuntimeDebug)]
+#[derive(Clone, Encode, Decode, PartialEq, sp_core::RuntimeDebug)]
 #[cfg_attr(feature = "std", derive(serde::Serialize, serde::Deserialize))]
 pub struct HostConfiguration<BlockNumber> {
 	/// The minimum frequency at which parachains can update their validation code.
@@ -50,23 +51,32 @@ pub struct HostConfiguration<BlockNumber> {
 	pub parathread_cores: u32,
 	/// The number of retries that a parathread author has to submit their block.
 	pub parathread_retries: u32,
-	/// How often parachain groups should be rotated across parachains. Must be non-zero.
+	/// How often parachain groups should be rotated across parachains.
+	///
+	/// Must be non-zero.
 	pub group_rotation_frequency: BlockNumber,
 	/// The availability period, in blocks, for parachains. This is the amount of blocks
 	/// after inclusion that validators have to make the block available and signal its availability to
-	/// the chain. Must be at least 1.
+	/// the chain.
+	///
+	/// Must be at least 1.
 	pub chain_availability_period: BlockNumber,
 	/// The availability period, in blocks, for parathreads. Same as the `chain_availability_period`,
-	/// but a differing timeout due to differing requirements. Must be at least 1.
+	/// but a differing timeout due to differing requirements.
+	///
+	/// Must be at least 1.
 	pub thread_availability_period: BlockNumber,
 	/// The amount of blocks ahead to schedule parachains and parathreads.
 	pub scheduling_lookahead: u32,
-	/// The maximum number of validators to have per core. `None` means no maximum.
+	/// The maximum number of validators to have per core.
+	///
+	/// `None` means no maximum.
 	pub max_validators_per_core: Option<u32>,
 	/// The amount of sessions to keep for disputes.
 	pub dispute_period: SessionIndex,
 	/// The amount of consensus slots that must pass between submitting an assignment and
 	/// submitting an approval vote before a validator is considered a no-show.
+	///
 	/// Must be at least 1.
 	pub no_show_slots: u32,
 	/// The number of delay tranches in total.
@@ -132,6 +142,74 @@ pub struct HostConfiguration<BlockNumber> {
 	pub hrmp_max_message_num_per_candidate: u32,
 }
 
+impl<BlockNumber: Default + From<u32>> Default for HostConfiguration<BlockNumber> {
+	fn default() -> Self {
+		Self {
+			group_rotation_frequency: 1u32.into(),
+			chain_availability_period: 1u32.into(),
+			thread_availability_period: 1u32.into(),
+			no_show_slots: 1u32.into(),
+			validation_upgrade_frequency: Default::default(),
+			validation_upgrade_delay: Default::default(),
+			acceptance_period: Default::default(),
+			max_code_size: Default::default(),
+			max_pov_size: Default::default(),
+			max_head_data_size: Default::default(),
+			parathread_cores: Default::default(),
+			parathread_retries: Default::default(),
+			scheduling_lookahead: Default::default(),
+			max_validators_per_core: Default::default(),
+			dispute_period: Default::default(),
+			n_delay_tranches: Default::default(),
+			zeroth_delay_tranche_width: Default::default(),
+			needed_approvals: Default::default(),
+			relay_vrf_modulo_samples: Default::default(),
+			max_upward_queue_count: Default::default(),
+			max_upward_queue_size: Default::default(),
+			max_downward_message_size: Default::default(),
+			preferred_dispatchable_upward_messages_step_weight: Default::default(),
+			max_upward_message_size: Default::default(),
+			max_upward_message_num_per_candidate: Default::default(),
+			hrmp_open_request_ttl: Default::default(),
+			hrmp_sender_deposit: Default::default(),
+			hrmp_recipient_deposit: Default::default(),
+			hrmp_channel_max_capacity: Default::default(),
+			hrmp_channel_max_total_size: Default::default(),
+			hrmp_max_parachain_inbound_channels: Default::default(),
+			hrmp_max_parathread_inbound_channels: Default::default(),
+			hrmp_channel_max_message_size: Default::default(),
+			hrmp_max_parachain_outbound_channels: Default::default(),
+			hrmp_max_parathread_outbound_channels: Default::default(),
+			hrmp_max_message_num_per_candidate: Default::default(),
+		}
+	}
+}
+
+impl<BlockNumber: Zero> HostConfiguration<BlockNumber> {
+	/// Checks that this instance is consistent with the requirements on each individual member.
+	///
+	/// # Panic
+	///
+	/// This function panics if any member is not set properly.
+	fn check_consistency(&self) {
+		if self.group_rotation_frequency.is_zero() {
+			panic!("`group_rotation_frequency` must be non-zero!")
+		}
+
+		if self.chain_availability_period.is_zero() {
+			panic!("`chain_availability_period` must be at least 1!")
+		}
+
+		if self.thread_availability_period.is_zero() {
+			panic!("`thread_availability_period` must be at least 1!")
+		}
+
+		if self.no_show_slots.is_zero() {
+			panic!("`no_show_slots` must be at least 1!")
+		}
+	}
+}
+
 pub trait Config: frame_system::Config { }
 
 decl_storage! {
@@ -141,10 +219,18 @@ decl_storage! {
 		/// Pending configuration (if any) for the next session.
 		PendingConfig: Option<HostConfiguration<T::BlockNumber>>;
 	}
+	add_extra_genesis {
+		build(|config: &Self| {
+			config.config.check_consistency();
+		})
+	}
 }
 
 decl_error! {
-	pub enum Error for Module<T: Config> { }
+	pub enum Error for Module<T: Config> {
+		/// The new value for a configuration parameter is invalid.
+		InvalidNewValue,
+	}
 }
 
 decl_module! {
@@ -237,6 +323,9 @@ decl_module! {
 		#[weight = (1_000, DispatchClass::Operational)]
 		pub fn set_group_rotation_frequency(origin, new: T::BlockNumber) -> DispatchResult {
 			ensure_root(origin)?;
+
+			ensure!(!new.is_zero(), Error::<T>::InvalidNewValue);
+
 			Self::update_config_member(|config| {
 				sp_std::mem::replace(&mut config.group_rotation_frequency, new) != new
 			});
@@ -247,6 +336,9 @@ decl_module! {
 		#[weight = (1_000, DispatchClass::Operational)]
 		pub fn set_chain_availability_period(origin, new: T::BlockNumber) -> DispatchResult {
 			ensure_root(origin)?;
+
+			ensure!(!new.is_zero(), Error::<T>::InvalidNewValue);
+
 			Self::update_config_member(|config| {
 				sp_std::mem::replace(&mut config.chain_availability_period, new) != new
 			});
@@ -257,6 +349,9 @@ decl_module! {
 		#[weight = (1_000, DispatchClass::Operational)]
 		pub fn set_thread_availability_period(origin, new: T::BlockNumber) -> DispatchResult {
 			ensure_root(origin)?;
+
+			ensure!(!new.is_zero(), Error::<T>::InvalidNewValue);
+
 			Self::update_config_member(|config| {
 				sp_std::mem::replace(&mut config.thread_availability_period, new) != new
 			});
@@ -298,7 +393,9 @@ decl_module! {
 		#[weight = (1_000, DispatchClass::Operational)]
 		pub fn set_no_show_slots(origin, new: u32) -> DispatchResult {
 			ensure_root(origin)?;
-			ensure!(new >= 1, "no_show_slots must be at least 1");
+
+			ensure!(!new.is_zero(), Error::<T>::InvalidNewValue);
+
 			Self::update_config_member(|config| {
 				sp_std::mem::replace(&mut config.no_show_slots, new) != new
 			});

polkadot/runtime/parachains/src/initializer.rs

@@ -33,7 +33,7 @@ use crate::{
 };
 
 /// Information about a session change that has just occurred.
-#[derive(Default, Clone)]
+#[derive(Clone)]
 pub struct SessionChangeNotification<BlockNumber> {
 	/// The new validators in the session.
 	pub validators: Vec<ValidatorId>,
@@ -49,6 +49,19 @@ pub struct SessionChangeNotification<BlockNumber> {
 	pub session_index: sp_staking::SessionIndex,
 }
 
+impl<BlockNumber: Default + From<u32>> Default for SessionChangeNotification<BlockNumber> {
+	fn default() -> Self {
+		Self {
+			validators: Vec::new(),
+			queued: Vec::new(),
+			prev_config: HostConfiguration::default(),
+			new_config: HostConfiguration::default(),
+			random_seed: Default::default(),
+			session_index: Default::default(),
+		}
+	}
+}
+
 #[derive(Encode, Decode)]
 struct BufferedSessionChange<N> {
 	apply_at: N,

polkadot/runtime/parachains/src/runtime_api_impl/v1.rs

@@ -26,7 +26,6 @@ use primitives::v1::{
 	GroupIndex, CandidateEvent, PersistedValidationData, SessionInfo,
 	InboundDownwardMessage, InboundHrmpMessage,
 };
-use sp_runtime::traits::Zero;
 use frame_support::debug;
 use crate::{initializer, inclusion, scheduler, configuration, paras, session_info, dmp, hrmp};
 
@@ -57,10 +56,6 @@ pub fn availability_cores<T: initializer::Config>() -> Vec<CoreState<T::BlockNum
 	let time_out_at = |backed_in_number, availability_period| {
 		let time_out_at = backed_in_number + availability_period;
 
-		if rotation_info.group_rotation_frequency == Zero::zero() {
-			return time_out_at;
-		}
-
 		let current_window = rotation_info.last_rotation_at() + availability_period;
 		let next_rotation = rotation_info.next_rotation_at();
 

polkadot/runtime/parachains/src/scheduler.rs

@@ -46,7 +46,7 @@ use frame_support::{
 	weights::Weight,
 };
 use parity_scale_codec::{Encode, Decode};
-use sp_runtime::traits::{Saturating, Zero};
+use sp_runtime::traits::Saturating;
 
 use rand::{SeedableRng, seq::SliceRandom};
 use rand_chacha::ChaCha20Rng;
@@ -568,11 +568,6 @@ impl<T: Config> Module<T> {
 
 		if at < session_start_block { return None }
 
-		if config.group_rotation_frequency.is_zero() {
-			// interpret this as "no rotations"
-			return Some(GroupIndex(core.0));
-		}
-
 		let validator_groups = ValidatorGroups::get();
 
 		if core.0 as usize >= validator_groups.len() { return None }
@@ -599,9 +594,6 @@ impl<T: Config> Module<T> {
 	/// timeouts, i.e. only within `max(config.chain_availability_period, config.thread_availability_period)`
 	/// of the last rotation would this return `Some`, unless there are no rotations.
 	///
-	/// If there are no rotations (config.group_rotation_frequency == 0),
-	/// availability timeouts can occur at any block.
-	///
 	/// This really should not be a box, but is working around a compiler limitation filed here:
 	/// https://github.com/rust-lang/rust/issues/73226
 	/// which prevents us from testing the code if using `impl Trait`.
@@ -611,12 +603,7 @@ impl<T: Config> Module<T> {
 
 		let session_start = <SessionStartBlock<T>>::get();
 		let blocks_since_session_start = now.saturating_sub(session_start);
-		let no_rotation = config.group_rotation_frequency.is_zero();
-		let blocks_since_last_rotation = if no_rotation {
-			<T::BlockNumber>::zero()
-		} else {
-			blocks_since_session_start % config.group_rotation_frequency
-		};
+		let blocks_since_last_rotation = blocks_since_session_start % config.group_rotation_frequency;
 
 		let absolute_cutoff = sp_std::cmp::max(
 			config.chain_availability_period,
@@ -1658,8 +1645,10 @@ mod tests {
 		} = default_config();
 		let collator = CollatorId::from(Sr25519Keyring::Alice.public());
 
-		assert!(chain_availability_period < thread_availability_period &&
-			thread_availability_period < group_rotation_frequency);
+		assert!(
+			chain_availability_period < thread_availability_period
+				&& thread_availability_period < group_rotation_frequency
+		);
 
 		let chain_a = ParaId::from(1);
 		let thread_a = ParaId::from(2);
@@ -1749,92 +1738,6 @@ mod tests {
 		});
 	}
 
-	#[test]
-	fn availability_predicate_no_rotation() {
-		let genesis_config = MockGenesisConfig {
-			configuration: crate::configuration::GenesisConfig {
-				config: HostConfiguration {
-					group_rotation_frequency: 0, // no rotation
-					..default_config()
-				},
-				..Default::default()
-			},
-			..Default::default()
-		};
-		let HostConfiguration {
-			chain_availability_period,
-			thread_availability_period,
-			..
-		} = default_config();
-		let collator = CollatorId::from(Sr25519Keyring::Alice.public());
-
-		let chain_a = ParaId::from(1);
-		let thread_a = ParaId::from(2);
-
-		let schedule_blank_para = |id, is_chain| Paras::schedule_para_initialize(id, ParaGenesisArgs {
-			genesis_head: Vec::new().into(),
-			validation_code: Vec::new().into(),
-			parachain: is_chain,
-		});
-
-		new_test_ext(genesis_config).execute_with(|| {
-			schedule_blank_para(chain_a, true);
-			schedule_blank_para(thread_a, false);
-
-			// start a new session with our chain & thread registered.
-			run_to_block(1, |number| match number {
-				1 => Some(SessionChangeNotification {
-					new_config: HostConfiguration{
-						// Note: the `group_rotation_frequency` config change
-						// is not accounted for on session change
-						// group_rotation_frequency: 0,
-						..default_config()
-					},
-					validators: vec![
-						ValidatorId::from(Sr25519Keyring::Alice.public()),
-						ValidatorId::from(Sr25519Keyring::Bob.public()),
-						ValidatorId::from(Sr25519Keyring::Charlie.public()),
-						ValidatorId::from(Sr25519Keyring::Dave.public()),
-						ValidatorId::from(Sr25519Keyring::Eve.public()),
-					],
-					..Default::default()
-				}),
-				_ => None,
-			});
-
-			// assign some availability cores.
-			{
-				AvailabilityCores::mutate(|cores| {
-					cores[0] = Some(CoreOccupied::Parachain);
-					cores[1] = Some(CoreOccupied::Parathread(ParathreadEntry {
-						claim: ParathreadClaim(thread_a, collator),
-						retries: 0,
-					}))
-				});
-			}
-			run_to_block(1 + 1, |_| None);
-			run_to_block(1 + 1 + 100500, |_| None);
-			{
-				let pred = Scheduler::availability_timeout_predicate()
-					.expect("predicate exists with no rotation");
-
-				let now = System::block_number();
-
-				assert!(!pred(CoreIndex(0), now)); // assigned: chain
-				assert!(!pred(CoreIndex(1), now)); // assigned: thread
-				assert!(pred(CoreIndex(2), now));
-
-				// check the tighter bound on chains vs threads.
-				assert!(pred(CoreIndex(0), now - chain_availability_period));
-				assert!(pred(CoreIndex(1), now - thread_availability_period));
-
-				// check the threshold is exact.
-				assert!(!pred(CoreIndex(0), now - chain_availability_period + 1));
-				assert!(!pred(CoreIndex(1), now - thread_availability_period + 1));
-			}
-		});
-	}
-
 	#[test]
 	fn next_up_on_available_uses_next_scheduled_or_none_for_thread() {
 		let mut config = default_config();