From b5ee4d39adaf5bf9f6264a36f09e9c3e4bfc499f Mon Sep 17 00:00:00 2001 From: Sergei Shulepov Date: Tue, 28 Dec 2021 14:53:52 +0100 Subject: [PATCH] configuration: Consistency checks for PVF pre-checking (#4580) As was suggested by Alexander Popiak [here][comment], this commit checks the consistency of the configuration. [comment]: https://github.com/paritytech/polkadot/pull/4420#discussion_r764796519 --- node/service/src/chain_spec.rs | 1 + node/test/service/src/chain_spec.rs | 1 + runtime/parachains/src/configuration.rs | 68 ++++++++++++++++++++++--- runtime/parachains/src/paras.rs | 6 ++- runtime/parachains/src/scheduler.rs | 4 ++ 5 files changed, 71 insertions(+), 9 deletions(-) diff --git a/node/service/src/chain_spec.rs b/node/service/src/chain_spec.rs index 31b15107d1c4..b2cf24c90ec2 100644 --- a/node/service/src/chain_spec.rs +++ b/node/service/src/chain_spec.rs @@ -210,6 +210,7 @@ fn default_parachains_host_configuration( needed_approvals: 2, relay_vrf_modulo_samples: 2, zeroth_delay_tranche_width: 0, + minimum_validation_upgrade_delay: 5, ..Default::default() } } diff --git a/node/test/service/src/chain_spec.rs b/node/test/service/src/chain_spec.rs index 6203593f65c3..3aed0210e363 100644 --- a/node/test/service/src/chain_spec.rs +++ b/node/test/service/src/chain_spec.rs @@ -171,6 +171,7 @@ fn polkadot_testnet_genesis( chain_availability_period: 4, thread_availability_period: 4, no_show_slots: 10, + minimum_validation_upgrade_delay: 5, ..Default::default() }, }, diff --git a/runtime/parachains/src/configuration.rs b/runtime/parachains/src/configuration.rs index 8e27ee86fc59..11c257bea433 100644 --- a/runtime/parachains/src/configuration.rs +++ b/runtime/parachains/src/configuration.rs @@ -205,6 +205,9 @@ pub struct HostConfiguration { /// /// To prevent that, we introduce the minimum number of blocks after which the upgrade can be /// scheduled. This number is controlled by this field. + /// + /// This value should be greater than [`chain_availability_period`] and + /// [`thread_availability_period`]. pub minimum_validation_upgrade_delay: BlockNumber, } @@ -253,14 +256,14 @@ impl> Default for HostConfiguration { /// `group_rotation_frequency` is set to zero. ZeroGroupRotationFrequency, /// `chain_availability_period` is set to zero. @@ -275,6 +278,16 @@ pub enum InconsistentError { MaxHeadDataSizeExceedHardLimit { max_head_data_size: u32 }, /// `max_pov_size` exceeds the hard limit of `MAX_POV_SIZE`. MaxPovSizeExceedHardLimit { max_pov_size: u32 }, + /// `minimum_validation_upgrade_delay` is less than `chain_availability_period`. + MinimumValidationUpgradeDelayLessThanChainAvailabilityPeriod { + minimum_validation_upgrade_delay: BlockNumber, + chain_availability_period: BlockNumber, + }, + /// `minimum_validation_upgrade_delay` is less than `thread_availability_period`. + MinimumValidationUpgradeDelayLessThanThreadAvailabilityPeriod { + minimum_validation_upgrade_delay: BlockNumber, + thread_availability_period: BlockNumber, + }, } impl HostConfiguration { @@ -283,7 +296,7 @@ impl HostConfigurat /// # Errors /// /// This function returns an error if the configuration is inconsistent. - pub fn check_consistency(&self) -> Result<(), InconsistentError> { + pub fn check_consistency(&self) -> Result<(), InconsistentError> { use InconsistentError::*; if self.group_rotation_frequency.is_zero() { @@ -316,6 +329,18 @@ impl HostConfigurat return Err(MaxPovSizeExceedHardLimit { max_pov_size: self.max_pov_size }) } + if self.minimum_validation_upgrade_delay <= self.chain_availability_period { + return Err(MinimumValidationUpgradeDelayLessThanChainAvailabilityPeriod { + minimum_validation_upgrade_delay: self.minimum_validation_upgrade_delay.clone(), + chain_availability_period: self.chain_availability_period.clone(), + }) + } else if self.minimum_validation_upgrade_delay <= self.thread_availability_period { + return Err(MinimumValidationUpgradeDelayLessThanThreadAvailabilityPeriod { + minimum_validation_upgrade_delay: self.minimum_validation_upgrade_delay.clone(), + thread_availability_period: self.thread_availability_period.clone(), + }) + } + Ok(()) } @@ -999,6 +1024,8 @@ pub mod pallet { /// Sets the minimum delay between announcing the upgrade block for a parachain until the /// upgrade taking place. + /// + /// See the field documentation for information and constraints for the new value. #[pallet::weight(( T::WeightInfo::set_config_with_block_number(), DispatchClass::Operational, @@ -1449,6 +1476,29 @@ mod tests { Configuration::set_thread_availability_period(Origin::root(), 0), Error::::InvalidNewValue ); + assert_err!( + Configuration::set_no_show_slots(Origin::root(), 0), + Error::::InvalidNewValue + ); + + ::ActiveConfig::put(HostConfiguration { + chain_availability_period: 10, + thread_availability_period: 8, + minimum_validation_upgrade_delay: 11, + ..Default::default() + }); + assert_err!( + Configuration::set_chain_availability_period(Origin::root(), 12), + Error::::InvalidNewValue + ); + assert_err!( + Configuration::set_thread_availability_period(Origin::root(), 12), + Error::::InvalidNewValue + ); + assert_err!( + Configuration::set_minimum_validation_upgrade_delay(Origin::root(), 9), + Error::::InvalidNewValue + ); }); } @@ -1554,6 +1604,13 @@ mod tests { new_config.group_rotation_frequency, ) .unwrap(); + // This comes out of order to satisfy the validity criteria for the chain and thread + // availability periods. + Configuration::set_minimum_validation_upgrade_delay( + Origin::root(), + new_config.minimum_validation_upgrade_delay, + ) + .unwrap(); Configuration::set_chain_availability_period( Origin::root(), new_config.chain_availability_period, @@ -1694,11 +1751,6 @@ mod tests { ) .unwrap(); Configuration::set_pvf_voting_ttl(Origin::root(), new_config.pvf_voting_ttl).unwrap(); - Configuration::set_minimum_validation_upgrade_delay( - Origin::root(), - new_config.minimum_validation_upgrade_delay, - ) - .unwrap(); assert_eq!( ::PendingConfigs::get(), diff --git a/runtime/parachains/src/paras.rs b/runtime/parachains/src/paras.rs index 601e280a5fc5..8fdef24ec227 100644 --- a/runtime/parachains/src/paras.rs +++ b/runtime/parachains/src/paras.rs @@ -2453,7 +2453,11 @@ mod tests { code_retention_period, validation_upgrade_delay, pvf_checking_enabled: false, - minimum_validation_upgrade_delay: 0, + minimum_validation_upgrade_delay: 2, + // Those are not relevant to this test. However, HostConfiguration is still a + // subject for the consistency check. + chain_availability_period: 1, + thread_availability_period: 1, ..Default::default() }, ..Default::default() diff --git a/runtime/parachains/src/scheduler.rs b/runtime/parachains/src/scheduler.rs index fead0d39ec23..984db51d8902 100644 --- a/runtime/parachains/src/scheduler.rs +++ b/runtime/parachains/src/scheduler.rs @@ -850,6 +850,10 @@ mod tests { scheduling_lookahead: 2, parathread_retries: 1, pvf_checking_enabled: false, + // This field does not affect anything that scheduler does. However, `HostConfiguration` + // is still a subject to consistency test. It requires that `minimum_validation_upgrade_delay` + // is greater than `chain_availability_period` and `thread_availability_period`. + minimum_validation_upgrade_delay: 6, ..Default::default() } }