From 575d68a65d65ad1bdc460a05768dc68aa3abe753 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bastian=20K=C3=B6cher?= <bkchr@users.noreply.github.com>
Date: Tue, 8 Dec 2020 16:05:00 +0100
Subject: [PATCH] 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/node/service/src/chain_spec.rs | 10 +-
polkadot/node/test/service/src/chain_spec.rs | 5 +-
polkadot/primitives/src/v1.rs | 18 +--
.../src/runtime-api/validator-groups.md | 4 -
.../runtime/parachains/src/configuration.rs | 111 ++++++++++++++++--
.../runtime/parachains/src/initializer.rs | 15 ++-
.../parachains/src/runtime_api_impl/v1.rs | 5 -
polkadot/runtime/parachains/src/scheduler.rs | 109 +----------------
8 files changed, 137 insertions(+), 140 deletions(-)
diff --git a/polkadot/node/service/src/chain_spec.rs b/polkadot/node/service/src/chain_spec.rs
index 03cb99c012d..0acc287feea 100644
--- a/polkadot/node/service/src/chain_spec.rs
+++ b/polkadot/node/service/src/chain_spec.rs
@@ -779,7 +779,10 @@ fn rococo_staging_testnet_config_genesis(wasm_binary: &[u8]) -> rococo_runtime::
max_code_size: 5 * 1024 * 1024,
max_pov_size: 50 * 1024 * 1024,
max_head_data_size: 32 * 1024,
- group_rotation_frequency: 10,
+ group_rotation_frequency: 20,
+ chain_availability_period: 4,
+ thread_availability_period: 4,
+ no_show_slots: 10,
..Default::default()
},
}),
@@ -1232,7 +1235,10 @@ pub fn rococo_testnet_genesis(
max_code_size: 5 * 1024 * 1024,
max_pov_size: 50 * 1024 * 1024,
max_head_data_size: 32 * 1024,
- group_rotation_frequency: 10,
+ group_rotation_frequency: 20,
+ chain_availability_period: 4,
+ thread_availability_period: 4,
+ no_show_slots: 10,
max_upward_queue_count: 8,
max_upward_queue_size: 8 * 1024,
max_downward_message_size: 1024,
diff --git a/polkadot/node/test/service/src/chain_spec.rs b/polkadot/node/test/service/src/chain_spec.rs
index da0b4d844eb..546467ad8cc 100644
--- a/polkadot/node/test/service/src/chain_spec.rs
+++ b/polkadot/node/test/service/src/chain_spec.rs
@@ -169,7 +169,10 @@ fn polkadot_testnet_genesis(
max_code_size: 5 * 1024 * 1024,
max_pov_size: 50 * 1024 * 1024,
max_head_data_size: 32 * 1024,
- group_rotation_frequency: 10,
+ group_rotation_frequency: 20,
+ chain_availability_period: 4,
+ thread_availability_period: 4,
+ no_show_slots: 10,
..Default::default()
},
}),
diff --git a/polkadot/primitives/src/v1.rs b/polkadot/primitives/src/v1.rs
index 3985254028c..18a84a9b61d 100644
--- a/polkadot/primitives/src/v1.rs
+++ b/polkadot/primitives/src/v1.rs
@@ -24,7 +24,7 @@ use bitvec::vec::BitVec;
use primitives::RuntimeDebug;
use runtime_primitives::traits::AppVerify;
use inherents::InherentIdentifier;
-use sp_arithmetic::traits::{BaseArithmetic, Saturating, Zero};
+use sp_arithmetic::traits::{BaseArithmetic, Saturating};
use application_crypto::KeyTypeId;
pub use runtime_primitives::traits::{BlakeTwo256, Hash as HashT};
@@ -573,11 +573,7 @@ impl GroupRotationInfo {
impl<N: Saturating + BaseArithmetic + Copy> GroupRotationInfo<N> {
/// Returns the block number of the next rotation after the current block. If the current block
/// is 10 and the rotation frequency is 5, this should return 15.
- ///
- /// If the group rotation frequency is 0, returns 0.
pub fn next_rotation_at(&self) -> N {
- if self.group_rotation_frequency.is_zero() { return Zero::zero() }
-
let cycle_once = self.now + self.group_rotation_frequency;
cycle_once - (
cycle_once.saturating_sub(self.session_start_block) % self.group_rotation_frequency
@@ -586,10 +582,7 @@ impl<N: Saturating + BaseArithmetic + Copy> GroupRotationInfo<N> {
/// Returns the block number of the last rotation before or including the current block. If the
/// current block is 10 and the rotation frequency is 5, this should return 10.
- ///
- /// If the group rotation frequency is 0, returns 0.
pub fn last_rotation_at(&self) -> N {
- if self.group_rotation_frequency.is_zero() { return Zero::zero() }
self.now - (
self.now.saturating_sub(self.session_start_block) % self.group_rotation_frequency
)
@@ -837,15 +830,6 @@ mod tests {
assert_eq!(info.next_rotation_at(), 20);
assert_eq!(info.last_rotation_at(), 15);
-
- let info = GroupRotationInfo {
- session_start_block: 10u32,
- now: 11,
- group_rotation_frequency: 0,
- };
-
- assert_eq!(info.next_rotation_at(), 0);
- assert_eq!(info.last_rotation_at(), 0);
}
#[test]
diff --git a/polkadot/roadmap/implementers-guide/src/runtime-api/validator-groups.md b/polkadot/roadmap/implementers-guide/src/runtime-api/validator-groups.md
index 42b39f976d1..75a94e23497 100644
--- a/polkadot/roadmap/implementers-guide/src/runtime-api/validator-groups.md
+++ b/polkadot/roadmap/implementers-guide/src/runtime-api/validator-groups.md
@@ -17,14 +17,10 @@ impl GroupRotationInfo {
/// Returns the block number of the next rotation after the current block. If the current block
/// is 10 and the rotation frequency is 5, this should return 15.
- ///
- /// If the group rotation frequency is 0, returns 0.
fn next_rotation_at(&self) -> BlockNumber;
/// Returns the block number of the last rotation before or including the current block. If the
/// current block is 10 and the rotation frequency is 5, this should return 10.
- ///
- /// If the group rotation frequency is 0, returns 0.
fn last_rotation_at(&self) -> BlockNumber;
}
diff --git a/polkadot/runtime/parachains/src/configuration.rs b/polkadot/runtime/parachains/src/configuration.rs
index a37234238cc..af693ed6762 100644
--- a/polkadot/runtime/parachains/src/configuration.rs
+++ b/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
});
diff --git a/polkadot/runtime/parachains/src/initializer.rs b/polkadot/runtime/parachains/src/initializer.rs
index 15484ed5fb3..409b52a2607 100644
--- a/polkadot/runtime/parachains/src/initializer.rs
+++ b/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,
diff --git a/polkadot/runtime/parachains/src/runtime_api_impl/v1.rs b/polkadot/runtime/parachains/src/runtime_api_impl/v1.rs
index a5733612716..f099c0db150 100644
--- a/polkadot/runtime/parachains/src/runtime_api_impl/v1.rs
+++ b/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();
diff --git a/polkadot/runtime/parachains/src/scheduler.rs b/polkadot/runtime/parachains/src/scheduler.rs
index 139e2dfa6a7..7eaf8e6c136 100644
--- a/polkadot/runtime/parachains/src/scheduler.rs
+++ b/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();
--
GitLab