From 259cb4baefee9c6394d438d4477cde2ca02639c4 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 10 Oct 2023 11:10:17 +0300 Subject: [PATCH 01/34] Filter backing votes in `paras_inherent` - initial work --- .../parachains/src/paras_inherent/mod.rs | 96 ++++++++++++++++++- 1 file changed, 95 insertions(+), 1 deletion(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index 8e918d35d5ff0..440a60a65fea2 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -121,7 +121,11 @@ pub mod pallet { #[pallet::config] #[pallet::disable_frame_system_supertrait_check] pub trait Config: - inclusion::Config + scheduler::Config + initializer::Config + pallet_babe::Config + inclusion::Config + + scheduler::Config + + initializer::Config + + pallet_babe::Config + + pallet_session::Config { /// Weight information for extrinsics in this pallet. type WeightInfo: WeightInfo; @@ -430,6 +434,9 @@ impl Pallet { T::DisputesHandler::filter_dispute_data(set, post_conclusion_acceptance_period) }; + // Filter out backing statements from disabled validators + let statements_were_dropped = filter_backed_statements::(&mut backed_candidates)?; + // Limit the disputes first, since the following statements depend on the votes include // here. let (checked_disputes_sets, checked_disputes_sets_consumed_weight) = @@ -480,6 +487,7 @@ impl Pallet { } ensure!(all_weight_before.all_lte(max_block_weight), Error::::InherentOverweight); + ensure!(!statements_were_dropped, Error::::InherentOverweight); all_weight_before }; @@ -1029,3 +1037,89 @@ fn limit_and_sanitize_disputes< (checked, checked_disputes_weight) } } + +// Filters statements from disabled validators in `BackedCandidate` and `MultiDisputeStatementSet`. +// Returns `true` if at least one statement is removed and `false` otherwise. +fn filter_backed_statements( + backed_candidates: &mut Vec::Hash>>, +) -> Result { + // `active_validator_indices` contain 'raw indexes' aka indexes in the broad validator set. + // We want mapping from `ValidatorIndex` (which is an index within `active_validator_indices`) + // to 'raw indexes' to process disabled validators. + let raw_idx_to_val_index = shared::Pallet::::active_validator_indices() + .iter() + .enumerate() + .map(|(i, v)| (v.0, ValidatorIndex(i as u32))) + .collect::>(); + // `disabled_validators` in pallet session contains 'raw indexes' (see the previous comment). We + // want them converted to `ValidatorIndex` so that we can look them up easily when processing + // backed candidates. We skip disabled validators which are not found in the active set. + let disabled_validators = >::disabled_validators() + .iter() + .filter_map(|i| raw_idx_to_val_index.get(i)) + .collect::>(); + + // `BackedCandidate` contains `validator_indices` which are indecies within the validator group. + // To obtain `ValidatorIndex` from them we do the following steps: + // 1. Get `para_id` from `CandidateDescriptor` + // 2. Get the `core_idx`` for the corresponding `para_id` + // 3. Get the validator group assigned to the corresponding core idx + + // Map `para_id` to `core_idx` for step 2 from the above list + let para_id_to_core_idx = >::scheduled_paras() + .map(|(core_idx, para_id)| (para_id, core_idx)) + .collect::>(); + + // Flag which will be returned. Set to `true` if at least one vote is filtered. + let mut filtered = false; + + // Process all backed candidates. + for bc in backed_candidates { + // Get `core_idx` assigned to the `para_id` of the candidate (step 2) + let core_idx = *para_id_to_core_idx.get(&bc.descriptor().para_id).unwrap(); // TODO + + // Get relay parent block number of the candidate. We need this to get the group index + // assigned to this core at this block number + let relay_parent_block_number = >::allowed_relay_parents() + .acquire_info(bc.descriptor().relay_parent, None) + .unwrap() // todo + .1; + + // Get the group index for the core + let group_idx = >::group_assigned_to_core( + core_idx, + relay_parent_block_number + One::one(), + ) + .unwrap(); + + // And finally get the validator group for this group index + let validator_group = >::group_validators(group_idx).unwrap(); // TODO + + // `validator_indices` in `BackedCandidate` are indecies within the validator group. Convert + // them to `ValidatorId`. + let voted_validator_ids = bc + .validator_indices + .iter() + .enumerate() + .filter_map(|(idx, bitval)| match *bitval { + true => Some(validator_group.get(idx).unwrap()), //todo + false => None, + }) + .collect::>(); + + { + // `validity_votes` should match `validator_indices` + let mut idx = 0; + bc.validity_votes.retain(|_| { + let voted_validator_index = voted_validator_ids.get(idx).unwrap(); // todo + idx += 1; + + filtered = disabled_validators.contains(voted_validator_index); + + !filtered + }); + } + } + + Ok(filtered) +} From d451886916fd63ef9b7cdbf2a56e7d0c66996ebb Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 12 Oct 2023 15:01:03 +0300 Subject: [PATCH 02/34] Error handling --- .../parachains/src/paras_inherent/mod.rs | 53 +++++++++++++++---- 1 file changed, 43 insertions(+), 10 deletions(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index 440a60a65fea2..03fc8b76f23bf 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -1074,16 +1074,27 @@ fn filter_backed_statements( let mut filtered = false; // Process all backed candidates. - for bc in backed_candidates { + backed_candidates.retain_mut(|bc| { // Get `core_idx` assigned to the `para_id` of the candidate (step 2) - let core_idx = *para_id_to_core_idx.get(&bc.descriptor().para_id).unwrap(); // TODO + let core_idx = match para_id_to_core_idx.get(&bc.descriptor().para_id) { + Some(core_idx) => *core_idx, + None => { + log::debug!(target: LOG_TARGET, "Can't get core idx got a backed candidate for para id {:?}. Dropping the candidate.", bc.descriptor().para_id); + return false + } + }; // Get relay parent block number of the candidate. We need this to get the group index // assigned to this core at this block number - let relay_parent_block_number = >::allowed_relay_parents() - .acquire_info(bc.descriptor().relay_parent, None) - .unwrap() // todo - .1; + let relay_parent_block_number = match >::allowed_relay_parents() + .acquire_info(bc.descriptor().relay_parent, None) { + Some((_, block_num)) => block_num, + None => { + log::debug!(target: LOG_TARGET, "Relay parent {:?} for candidate is not in the allowed relay parents. Dropping the candidate.", bc.descriptor().relay_parent); + return false + } + }; + // Get the group index for the core let group_idx = >::group_assigned_to_core( @@ -1093,7 +1104,14 @@ fn filter_backed_statements( .unwrap(); // And finally get the validator group for this group index - let validator_group = >::group_validators(group_idx).unwrap(); // TODO + let validator_group = match >::group_validators(group_idx) { + Some(validator_group) => validator_group, + None => { + // TODO: this should be an assert? + log::debug!(target: LOG_TARGET, "Can't get the validators from group {:?}. Dropping the candidate.", group_idx); + return false + } + }; // `validator_indices` in `BackedCandidate` are indecies within the validator group. Convert // them to `ValidatorId`. @@ -1102,7 +1120,7 @@ fn filter_backed_statements( .iter() .enumerate() .filter_map(|(idx, bitval)| match *bitval { - true => Some(validator_group.get(idx).unwrap()), //todo + true => validator_group.get(idx), // drop indecies not found in the validator group. This will lead to filtering the vote false => None, }) .collect::>(); @@ -1111,15 +1129,30 @@ fn filter_backed_statements( // `validity_votes` should match `validator_indices` let mut idx = 0; bc.validity_votes.retain(|_| { - let voted_validator_index = voted_validator_ids.get(idx).unwrap(); // todo + let voted_validator_index = match voted_validator_ids.get(idx) { + Some(validator_index) => validator_index, + None => { + log::debug!(target: LOG_TARGET, "Can't find the voted validator index {:?} in the validator group. Dropping the vote.", group_idx); + idx += 1; + return false + } + }; idx += 1; filtered = disabled_validators.contains(voted_validator_index); + // If we are removing a validity vote - modify `validator_indices` too + if filtered { + bc.validator_indices.set(idx, false); + } + !filtered }); } - } + + // Remove the candidate if all entries are filtered out + !bc.validity_votes.is_empty() + }); Ok(filtered) } From cb0d8fc868fb5f21150d7ab73897de06e616b35a Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Mon, 16 Oct 2023 16:39:27 +0300 Subject: [PATCH 03/34] Add `fn disabled_validators` to `trait DisabledValidators`. Use the trait to fetch the disabled validators from paras_inherent. --- polkadot/runtime/parachains/src/mock.rs | 1 + polkadot/runtime/parachains/src/paras_inherent/mod.rs | 11 ++++------- polkadot/runtime/rococo/src/lib.rs | 1 + polkadot/runtime/test-runtime/src/lib.rs | 1 + polkadot/runtime/westend/src/lib.rs | 1 + substrate/frame/aura/src/mock.rs | 4 ++++ substrate/frame/session/src/lib.rs | 4 ++++ substrate/frame/support/src/traits/validation.rs | 7 +++++++ 8 files changed, 23 insertions(+), 7 deletions(-) diff --git a/polkadot/runtime/parachains/src/mock.rs b/polkadot/runtime/parachains/src/mock.rs index ded7de08e4fa9..8c34439f1a7ec 100644 --- a/polkadot/runtime/parachains/src/mock.rs +++ b/polkadot/runtime/parachains/src/mock.rs @@ -368,6 +368,7 @@ impl crate::inclusion::Config for Test { impl crate::paras_inherent::Config for Test { type WeightInfo = crate::paras_inherent::TestWeightInfo; + type DisabledValidators = (); } pub struct MockValidatorSet; diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index 03fc8b76f23bf..e1bf9674c9a9b 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -37,7 +37,7 @@ use frame_support::{ dispatch::{DispatchErrorWithPostInfo, PostDispatchInfo}, inherent::{InherentData, InherentIdentifier, MakeFatalError, ProvideInherent}, pallet_prelude::*, - traits::Randomness, + traits::{DisabledValidators, Randomness}, }; use frame_system::pallet_prelude::*; use pallet_babe::{self, ParentBlockRandomness}; @@ -121,14 +121,11 @@ pub mod pallet { #[pallet::config] #[pallet::disable_frame_system_supertrait_check] pub trait Config: - inclusion::Config - + scheduler::Config - + initializer::Config - + pallet_babe::Config - + pallet_session::Config + inclusion::Config + scheduler::Config + initializer::Config + pallet_babe::Config { /// Weight information for extrinsics in this pallet. type WeightInfo: WeightInfo; + type DisabledValidators: DisabledValidators; } #[pallet::error] @@ -1054,7 +1051,7 @@ fn filter_backed_statements( // `disabled_validators` in pallet session contains 'raw indexes' (see the previous comment). We // want them converted to `ValidatorIndex` so that we can look them up easily when processing // backed candidates. We skip disabled validators which are not found in the active set. - let disabled_validators = >::disabled_validators() + let disabled_validators = ::DisabledValidators::disabled_validators() .iter() .filter_map(|i| raw_idx_to_val_index.get(i)) .collect::>(); diff --git a/polkadot/runtime/rococo/src/lib.rs b/polkadot/runtime/rococo/src/lib.rs index 3c08b9b2f94ef..01ef016589475 100644 --- a/polkadot/runtime/rococo/src/lib.rs +++ b/polkadot/runtime/rococo/src/lib.rs @@ -954,6 +954,7 @@ impl parachains_hrmp::Config for Runtime { impl parachains_paras_inherent::Config for Runtime { type WeightInfo = weights::runtime_parachains_paras_inherent::WeightInfo; + type DisabledValidators = Session; } impl parachains_scheduler::Config for Runtime { diff --git a/polkadot/runtime/test-runtime/src/lib.rs b/polkadot/runtime/test-runtime/src/lib.rs index 99fd2198400bf..9e7f8ef80a600 100644 --- a/polkadot/runtime/test-runtime/src/lib.rs +++ b/polkadot/runtime/test-runtime/src/lib.rs @@ -520,6 +520,7 @@ impl parachains_slashing::Config for Runtime { impl parachains_paras_inherent::Config for Runtime { type WeightInfo = parachains_paras_inherent::TestWeightInfo; + type DisabledValidators = Session; } impl parachains_initializer::Config for Runtime { diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index 0e93b3449fec5..31f34891e19db 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -1210,6 +1210,7 @@ impl parachains_hrmp::Config for Runtime { impl parachains_paras_inherent::Config for Runtime { type WeightInfo = weights::runtime_parachains_paras_inherent::WeightInfo; + type DisabledValidators = Session; } impl parachains_scheduler::Config for Runtime { diff --git a/substrate/frame/aura/src/mock.rs b/substrate/frame/aura/src/mock.rs index 39b798c2f6841..ed180b139fc7d 100644 --- a/substrate/frame/aura/src/mock.rs +++ b/substrate/frame/aura/src/mock.rs @@ -95,6 +95,10 @@ impl DisabledValidators for MockDisabledValidators { fn is_disabled(index: AuthorityIndex) -> bool { DisabledValidatorTestValue::get().binary_search(&index).is_ok() } + + fn disabled_validators() -> Vec { + DisabledValidatorTestValue::get() + } } impl pallet_aura::Config for Test { diff --git a/substrate/frame/session/src/lib.rs b/substrate/frame/session/src/lib.rs index bf4671a247f0d..178d43f596b27 100644 --- a/substrate/frame/session/src/lib.rs +++ b/substrate/frame/session/src/lib.rs @@ -918,6 +918,10 @@ impl frame_support::traits::DisabledValidators for Pallet { fn is_disabled(index: u32) -> bool { >::disabled_validators().binary_search(&index).is_ok() } + + fn disabled_validators() -> Vec { + >::disabled_validators() + } } /// Wraps the author-scraping logic for consensus engines that can recover diff --git a/substrate/frame/support/src/traits/validation.rs b/substrate/frame/support/src/traits/validation.rs index 617cdb2d3f461..4b099b2c766fe 100644 --- a/substrate/frame/support/src/traits/validation.rs +++ b/substrate/frame/support/src/traits/validation.rs @@ -251,10 +251,17 @@ pub trait ValidatorRegistration { pub trait DisabledValidators { /// Returns true if the given validator is disabled. fn is_disabled(index: u32) -> bool; + + /// Returns all disabled validators + fn disabled_validators() -> Vec; } impl DisabledValidators for () { fn is_disabled(_index: u32) -> bool { false } + + fn disabled_validators() -> Vec { + vec![] + } } From 8a8893ecc020c5006b54a3df1a165b8d62be6956 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 17 Oct 2023 14:53:20 +0300 Subject: [PATCH 04/34] Extract the implementation of `disabled_validators` from rutnime api and use it it in paras_inherent too --- .../parachains/src/paras_inherent/mod.rs | 16 +--------- .../src/runtime_api_impl/vstaging.rs | 19 ++---------- polkadot/runtime/parachains/src/shared.rs | 30 +++++++++++++++++-- polkadot/runtime/rococo/src/lib.rs | 4 ++- polkadot/runtime/westend/src/lib.rs | 4 ++- 5 files changed, 37 insertions(+), 36 deletions(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index e1bf9674c9a9b..603f2e63b5205 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -1040,21 +1040,7 @@ fn limit_and_sanitize_disputes< fn filter_backed_statements( backed_candidates: &mut Vec::Hash>>, ) -> Result { - // `active_validator_indices` contain 'raw indexes' aka indexes in the broad validator set. - // We want mapping from `ValidatorIndex` (which is an index within `active_validator_indices`) - // to 'raw indexes' to process disabled validators. - let raw_idx_to_val_index = shared::Pallet::::active_validator_indices() - .iter() - .enumerate() - .map(|(i, v)| (v.0, ValidatorIndex(i as u32))) - .collect::>(); - // `disabled_validators` in pallet session contains 'raw indexes' (see the previous comment). We - // want them converted to `ValidatorIndex` so that we can look them up easily when processing - // backed candidates. We skip disabled validators which are not found in the active set. - let disabled_validators = ::DisabledValidators::disabled_validators() - .iter() - .filter_map(|i| raw_idx_to_val_index.get(i)) - .collect::>(); + let disabled_validators = shared::Pallet::::disabled_validators(); // `BackedCandidate` contains `validator_indices` which are indecies within the validator group. // To obtain `ValidatorIndex` from them we do the following steps: diff --git a/polkadot/runtime/parachains/src/runtime_api_impl/vstaging.rs b/polkadot/runtime/parachains/src/runtime_api_impl/vstaging.rs index 24a076f3a4431..5e95d165367ae 100644 --- a/polkadot/runtime/parachains/src/runtime_api_impl/vstaging.rs +++ b/polkadot/runtime/parachains/src/runtime_api_impl/vstaging.rs @@ -18,27 +18,14 @@ use crate::shared; use primitives::ValidatorIndex; -use sp_std::{collections::btree_map::BTreeMap, prelude::Vec}; +use sp_std::prelude::Vec; /// Implementation for `DisabledValidators` // CAVEAT: this should only be called on the node side // as it might produce incorrect results on session boundaries pub fn disabled_validators() -> Vec where - T: pallet_session::Config + shared::Config, + T: shared::Config, { - let shuffled_indices = >::active_validator_indices(); - // mapping from raw validator index to `ValidatorIndex` - // this computation is the same within a session, but should be cheap - let reverse_index = shuffled_indices - .iter() - .enumerate() - .map(|(i, v)| (v.0, ValidatorIndex(i as u32))) - .collect::>(); - - // we might have disabled validators who are not parachain validators - >::disabled_validators() - .iter() - .filter_map(|v| reverse_index.get(v).cloned()) - .collect() + >::disabled_validators() } diff --git a/polkadot/runtime/parachains/src/shared.rs b/polkadot/runtime/parachains/src/shared.rs index ad13c9e48448f..96077a7f49ae9 100644 --- a/polkadot/runtime/parachains/src/shared.rs +++ b/polkadot/runtime/parachains/src/shared.rs @@ -19,11 +19,14 @@ //! To avoid cyclic dependencies, it is important that this pallet is not //! dependent on any of the other pallets. -use frame_support::pallet_prelude::*; +use frame_support::{pallet_prelude::*, traits::DisabledValidators}; use frame_system::pallet_prelude::BlockNumberFor; use primitives::{SessionIndex, ValidatorId, ValidatorIndex}; use sp_runtime::traits::AtLeast32BitUnsigned; -use sp_std::{collections::vec_deque::VecDeque, vec::Vec}; +use sp_std::{ + collections::{btree_map::BTreeMap, vec_deque::VecDeque}, + vec::Vec, +}; use rand::{seq::SliceRandom, SeedableRng}; use rand_chacha::ChaCha20Rng; @@ -129,7 +132,9 @@ pub mod pallet { pub struct Pallet(_); #[pallet::config] - pub trait Config: frame_system::Config {} + pub trait Config: frame_system::Config { + type DisabledValidators: frame_support::traits::DisabledValidators; + } /// The current session index. #[pallet::storage] @@ -216,6 +221,25 @@ impl Pallet { Self::session_index().saturating_add(SESSION_DELAY) } + /// Fetches disabled validators list from session pallet. + /// CAVEAT: this might produce incorrect results on session boundaries + pub fn disabled_validators() -> Vec { + let shuffled_indices = Pallet::::active_validator_indices(); + // mapping from raw validator index to `ValidatorIndex` + // this computation is the same within a session, but should be cheap + let reverse_index = shuffled_indices + .iter() + .enumerate() + .map(|(i, v)| (v.0, ValidatorIndex(i as u32))) + .collect::>(); + + // we might have disabled validators who are not parachain validators + T::DisabledValidators::disabled_validators() + .iter() + .filter_map(|v| reverse_index.get(v).cloned()) + .collect() + } + /// Test function for setting the current session index. #[cfg(any(feature = "std", feature = "runtime-benchmarks", test))] pub fn set_session_index(index: SessionIndex) { diff --git a/polkadot/runtime/rococo/src/lib.rs b/polkadot/runtime/rococo/src/lib.rs index 01ef016589475..430fe24511910 100644 --- a/polkadot/runtime/rococo/src/lib.rs +++ b/polkadot/runtime/rococo/src/lib.rs @@ -859,7 +859,9 @@ impl parachains_configuration::Config for Runtime { type WeightInfo = weights::runtime_parachains_configuration::WeightInfo; } -impl parachains_shared::Config for Runtime {} +impl parachains_shared::Config for Runtime { + type DisabledValidators = Session; +} impl parachains_session_info::Config for Runtime { type ValidatorSet = Historical; diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index 31f34891e19db..322a7b31c0f78 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -1122,7 +1122,9 @@ impl parachains_configuration::Config for Runtime { type WeightInfo = weights::runtime_parachains_configuration::WeightInfo; } -impl parachains_shared::Config for Runtime {} +impl parachains_shared::Config for Runtime { + type DisabledValidators = Session; +} impl parachains_session_info::Config for Runtime { type ValidatorSet = Historical; From 7b868b07f78a5d2cd56c1cdca49f43695a91ac76 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 17 Oct 2023 15:10:03 +0300 Subject: [PATCH 05/34] Error handling --- polkadot/runtime/parachains/src/paras_inherent/mod.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index 603f2e63b5205..6507e0abe5262 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -1080,11 +1080,16 @@ fn filter_backed_statements( // Get the group index for the core - let group_idx = >::group_assigned_to_core( + let group_idx = match >::group_assigned_to_core( core_idx, relay_parent_block_number + One::one(), - ) - .unwrap(); + ) { + Some(group_idx) => group_idx, + None => { + log::debug!(target: LOG_TARGET, "Can't fetch group index for core idx {:?}. Dropping the candidate.", core_idx); + return false + }, + }; // And finally get the validator group for this group index let validator_group = match >::group_validators(group_idx) { From a41d21cff0046ac9165a8ae86696e5644e9ffc17 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 17 Oct 2023 17:13:45 +0300 Subject: [PATCH 06/34] Remove `DisabledValidators` from paras_inherent --- polkadot/runtime/common/src/assigned_slots/mod.rs | 4 +++- polkadot/runtime/common/src/integration_tests.rs | 4 +++- polkadot/runtime/common/src/paras_registrar/mod.rs | 4 +++- polkadot/runtime/parachains/src/mock.rs | 5 +++-- polkadot/runtime/parachains/src/paras_inherent/mod.rs | 1 - polkadot/runtime/rococo/src/lib.rs | 1 - polkadot/runtime/test-runtime/src/lib.rs | 5 +++-- polkadot/runtime/westend/src/lib.rs | 1 - polkadot/xcm/xcm-builder/tests/mock/mod.rs | 4 +++- polkadot/xcm/xcm-simulator/example/src/relay_chain.rs | 4 +++- polkadot/xcm/xcm-simulator/fuzzer/src/relay_chain.rs | 4 +++- 11 files changed, 24 insertions(+), 13 deletions(-) diff --git a/polkadot/runtime/common/src/assigned_slots/mod.rs b/polkadot/runtime/common/src/assigned_slots/mod.rs index cc8ec339c1184..04f3367946231 100644 --- a/polkadot/runtime/common/src/assigned_slots/mod.rs +++ b/polkadot/runtime/common/src/assigned_slots/mod.rs @@ -742,7 +742,9 @@ mod tests { type OnNewHead = (); } - impl parachains_shared::Config for Test {} + impl parachains_shared::Config for Test { + type DisabledValidators = (); + } parameter_types! { pub const LeasePeriod: BlockNumber = 3; diff --git a/polkadot/runtime/common/src/integration_tests.rs b/polkadot/runtime/common/src/integration_tests.rs index f14db68267d5b..444cdd17bb32f 100644 --- a/polkadot/runtime/common/src/integration_tests.rs +++ b/polkadot/runtime/common/src/integration_tests.rs @@ -190,7 +190,9 @@ impl configuration::Config for Test { type WeightInfo = configuration::TestWeightInfo; } -impl shared::Config for Test {} +impl shared::Config for Test { + type DisabledValidators = (); +} impl origin::Config for Test {} diff --git a/polkadot/runtime/common/src/paras_registrar/mod.rs b/polkadot/runtime/common/src/paras_registrar/mod.rs index 8b8c6d89d0185..8611ac0ff8e46 100644 --- a/polkadot/runtime/common/src/paras_registrar/mod.rs +++ b/polkadot/runtime/common/src/paras_registrar/mod.rs @@ -797,7 +797,9 @@ mod tests { type MaxFreezes = ConstU32<1>; } - impl shared::Config for Test {} + impl shared::Config for Test { + type DisabledValidators = (); + } impl origin::Config for Test {} diff --git a/polkadot/runtime/parachains/src/mock.rs b/polkadot/runtime/parachains/src/mock.rs index 8c34439f1a7ec..df4240f295d6a 100644 --- a/polkadot/runtime/parachains/src/mock.rs +++ b/polkadot/runtime/parachains/src/mock.rs @@ -182,7 +182,9 @@ impl crate::configuration::Config for Test { type WeightInfo = crate::configuration::TestWeightInfo; } -impl crate::shared::Config for Test {} +impl crate::shared::Config for Test { + type DisabledValidators = (); +} impl origin::Config for Test {} @@ -368,7 +370,6 @@ impl crate::inclusion::Config for Test { impl crate::paras_inherent::Config for Test { type WeightInfo = crate::paras_inherent::TestWeightInfo; - type DisabledValidators = (); } pub struct MockValidatorSet; diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index 6507e0abe5262..9ac27686c3195 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -125,7 +125,6 @@ pub mod pallet { { /// Weight information for extrinsics in this pallet. type WeightInfo: WeightInfo; - type DisabledValidators: DisabledValidators; } #[pallet::error] diff --git a/polkadot/runtime/rococo/src/lib.rs b/polkadot/runtime/rococo/src/lib.rs index 430fe24511910..96165aff42b32 100644 --- a/polkadot/runtime/rococo/src/lib.rs +++ b/polkadot/runtime/rococo/src/lib.rs @@ -956,7 +956,6 @@ impl parachains_hrmp::Config for Runtime { impl parachains_paras_inherent::Config for Runtime { type WeightInfo = weights::runtime_parachains_paras_inherent::WeightInfo; - type DisabledValidators = Session; } impl parachains_scheduler::Config for Runtime { diff --git a/polkadot/runtime/test-runtime/src/lib.rs b/polkadot/runtime/test-runtime/src/lib.rs index 9e7f8ef80a600..43ee98d9dc976 100644 --- a/polkadot/runtime/test-runtime/src/lib.rs +++ b/polkadot/runtime/test-runtime/src/lib.rs @@ -484,7 +484,9 @@ impl parachains_configuration::Config for Runtime { type WeightInfo = parachains_configuration::TestWeightInfo; } -impl parachains_shared::Config for Runtime {} +impl parachains_shared::Config for Runtime { + type DisabledValidators = Session; +} impl parachains_inclusion::Config for Runtime { type RuntimeEvent = RuntimeEvent; @@ -520,7 +522,6 @@ impl parachains_slashing::Config for Runtime { impl parachains_paras_inherent::Config for Runtime { type WeightInfo = parachains_paras_inherent::TestWeightInfo; - type DisabledValidators = Session; } impl parachains_initializer::Config for Runtime { diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index 322a7b31c0f78..ae67cab495b35 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -1212,7 +1212,6 @@ impl parachains_hrmp::Config for Runtime { impl parachains_paras_inherent::Config for Runtime { type WeightInfo = weights::runtime_parachains_paras_inherent::WeightInfo; - type DisabledValidators = Session; } impl parachains_scheduler::Config for Runtime { diff --git a/polkadot/xcm/xcm-builder/tests/mock/mod.rs b/polkadot/xcm/xcm-builder/tests/mock/mod.rs index 363748940ca62..b5062649354e6 100644 --- a/polkadot/xcm/xcm-builder/tests/mock/mod.rs +++ b/polkadot/xcm/xcm-builder/tests/mock/mod.rs @@ -124,7 +124,9 @@ impl pallet_balances::Config for Runtime { type MaxFreezes = ConstU32<0>; } -impl shared::Config for Runtime {} +impl shared::Config for Runtime { + type DisabledValidators = (); +} impl configuration::Config for Runtime { type WeightInfo = configuration::TestWeightInfo; diff --git a/polkadot/xcm/xcm-simulator/example/src/relay_chain.rs b/polkadot/xcm/xcm-simulator/example/src/relay_chain.rs index 4e9195a8454f5..171901004ee36 100644 --- a/polkadot/xcm/xcm-simulator/example/src/relay_chain.rs +++ b/polkadot/xcm/xcm-simulator/example/src/relay_chain.rs @@ -118,7 +118,9 @@ impl pallet_uniques::Config for Runtime { type Helper = (); } -impl shared::Config for Runtime {} +impl shared::Config for Runtime { + type DisabledValidators = (); +} impl configuration::Config for Runtime { type WeightInfo = configuration::TestWeightInfo; diff --git a/polkadot/xcm/xcm-simulator/fuzzer/src/relay_chain.rs b/polkadot/xcm/xcm-simulator/fuzzer/src/relay_chain.rs index a29ead9e6c3be..5e30c3c085e04 100644 --- a/polkadot/xcm/xcm-simulator/fuzzer/src/relay_chain.rs +++ b/polkadot/xcm/xcm-simulator/fuzzer/src/relay_chain.rs @@ -96,7 +96,9 @@ impl pallet_balances::Config for Runtime { type MaxFreezes = ConstU32<0>; } -impl shared::Config for Runtime {} +impl shared::Config for Runtime { + type DisabledValidators = (); +} impl configuration::Config for Runtime { type WeightInfo = configuration::TestWeightInfo; From 34a3bf080093b55f3c22476d81522d688200e096 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 17 Oct 2023 22:32:37 +0300 Subject: [PATCH 07/34] Fix a warning --- polkadot/runtime/parachains/src/paras_inherent/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index 9ac27686c3195..b8d80bd31861d 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -37,7 +37,7 @@ use frame_support::{ dispatch::{DispatchErrorWithPostInfo, PostDispatchInfo}, inherent::{InherentData, InherentIdentifier, MakeFatalError, ProvideInherent}, pallet_prelude::*, - traits::{DisabledValidators, Randomness}, + traits::Randomness, }; use frame_system::pallet_prelude::*; use pallet_babe::{self, ParentBlockRandomness}; From e24131241698deb0e059deeeab9cd5fad5a999a0 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 26 Oct 2023 08:57:11 +0300 Subject: [PATCH 08/34] Add setters used for unit tests * `set_claimqueue` in `scheduler` * `add_allowed_relay_parent` in `shared` --- polkadot/runtime/parachains/src/scheduler.rs | 7 +++++++ polkadot/runtime/parachains/src/shared.rs | 12 ++++++++++++ 2 files changed, 19 insertions(+) diff --git a/polkadot/runtime/parachains/src/scheduler.rs b/polkadot/runtime/parachains/src/scheduler.rs index f3333d5cf8577..0edcc6cedbf21 100644 --- a/polkadot/runtime/parachains/src/scheduler.rs +++ b/polkadot/runtime/parachains/src/scheduler.rs @@ -744,4 +744,11 @@ impl Pallet { pub(crate) fn set_validator_groups(validator_groups: Vec>) { ValidatorGroups::::set(validator_groups); } + + #[cfg(test)] + pub(crate) fn set_claimqueue( + claimqueue: BTreeMap>>>>, + ) { + ClaimQueue::::set(claimqueue); + } } diff --git a/polkadot/runtime/parachains/src/shared.rs b/polkadot/runtime/parachains/src/shared.rs index 96077a7f49ae9..bdaffcd505f8e 100644 --- a/polkadot/runtime/parachains/src/shared.rs +++ b/polkadot/runtime/parachains/src/shared.rs @@ -263,4 +263,16 @@ impl Pallet { ActiveValidatorIndices::::set(indices); ActiveValidatorKeys::::set(keys); } + + #[cfg(test)] + pub(crate) fn add_allowed_relay_parent( + relay_parent: T::Hash, + state_root: T::Hash, + number: BlockNumberFor, + max_ancestry_len: u32, + ) { + AllowedRelayParents::::mutate(|tracker| { + tracker.update(relay_parent, state_root, number, max_ancestry_len) + }) + } } From 2aa1323204411aa9b5f8665b2c508d08e5c0ba84 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 26 Oct 2023 09:15:55 +0300 Subject: [PATCH 09/34] Mock for `DisabledValidators` in Test runtime --- polkadot/runtime/parachains/src/mock.rs | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/polkadot/runtime/parachains/src/mock.rs b/polkadot/runtime/parachains/src/mock.rs index b123e9955a5eb..523c9fd420860 100644 --- a/polkadot/runtime/parachains/src/mock.rs +++ b/polkadot/runtime/parachains/src/mock.rs @@ -183,8 +183,21 @@ impl crate::configuration::Config for Test { type WeightInfo = crate::configuration::TestWeightInfo; } +pub struct MockDisabledValidators {} +impl frame_support::traits::DisabledValidators for MockDisabledValidators { + /// Returns true if the given validator is disabled. + fn is_disabled(index: u32) -> bool { + disabled_validators().iter().any(|v| *v == index) + } + + /// Returns a hardcoded list (`DISABLED_VALIDATORS`) of disabled validators + fn disabled_validators() -> Vec { + disabled_validators() + } +} + impl crate::shared::Config for Test { - type DisabledValidators = (); + type DisabledValidators = MockDisabledValidators; } impl origin::Config for Test {} @@ -433,6 +446,8 @@ thread_local! { pub static AVAILABILITY_REWARDS: RefCell> = RefCell::new(HashMap::new()); + + pub static DISABLED_VALIDATORS: RefCell> = RefCell::new(vec![4]); } pub fn backing_rewards() -> HashMap { @@ -443,6 +458,10 @@ pub fn availability_rewards() -> HashMap { AVAILABILITY_REWARDS.with(|r| r.borrow().clone()) } +pub fn disabled_validators() -> Vec { + DISABLED_VALIDATORS.with(|r| r.borrow().clone()) +} + parameter_types! { pub static Processed: Vec<(ParaId, UpwardMessage)> = vec![]; } From a8efd1558c0d4a4470202ee8de777bdaffcba40d Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 26 Oct 2023 10:40:15 +0300 Subject: [PATCH 10/34] Fix test state and add an optimisation for `filter_backed_statements` --- .../parachains/src/paras_inherent/mod.rs | 12 +- .../parachains/src/paras_inherent/tests.rs | 105 +++++++++++++++++- 2 files changed, 111 insertions(+), 6 deletions(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index b8d80bd31861d..f8e369fad02c4 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -1041,10 +1041,15 @@ fn filter_backed_statements( ) -> Result { let disabled_validators = shared::Pallet::::disabled_validators(); + if disabled_validators.len() == 0 { + // No disabled validators - nothing to do + return Ok(false) + } + // `BackedCandidate` contains `validator_indices` which are indecies within the validator group. // To obtain `ValidatorIndex` from them we do the following steps: // 1. Get `para_id` from `CandidateDescriptor` - // 2. Get the `core_idx`` for the corresponding `para_id` + // 2. Get the `core_idx` for the corresponding `para_id` // 3. Get the validator group assigned to the corresponding core idx // Map `para_id` to `core_idx` for step 2 from the above list @@ -1056,6 +1061,7 @@ fn filter_backed_statements( let mut filtered = false; // Process all backed candidates. + let backed_len_before = backed_candidates.len(); backed_candidates.retain_mut(|bc| { // Get `core_idx` assigned to the `para_id` of the candidate (step 2) let core_idx = match para_id_to_core_idx.get(&bc.descriptor().para_id) { @@ -1141,5 +1147,9 @@ fn filter_backed_statements( !bc.validity_votes.is_empty() }); + if !filtered { + filtered = backed_len_before > backed_candidates.len(); + } + Ok(filtered) } diff --git a/polkadot/runtime/parachains/src/paras_inherent/tests.rs b/polkadot/runtime/parachains/src/paras_inherent/tests.rs index 4fc60792e3468..fb08b7fd68e44 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/tests.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/tests.rs @@ -1206,15 +1206,20 @@ mod sanitizers { } mod candidates { + use crate::scheduler::{common::Assignment, ParasEntry}; + use sp_std::collections::vec_deque::VecDeque; + use super::*; // Backed candidates and scheduled parachains used for `sanitize_backed_candidates` testing struct TestData { backed_candidates: Vec, scheduled_paras: BTreeMap, + validator_ids: Vec, } - // Generate test data for the candidates test + // Generate test data for the candidates and asserts that the evnironment is set as expected + // (check the comments for details) fn get_test_data() -> TestData { const RELAY_PARENT_NUM: u32 = 3; @@ -1226,11 +1231,13 @@ mod sanitizers { let keystore = Arc::new(keystore) as KeystorePtr; let signing_context = SigningContext { parent_hash: relay_parent, session_index }; + // 5 active validators where `Eve` is disabled let validators = vec![ keyring::Sr25519Keyring::Alice, keyring::Sr25519Keyring::Bob, keyring::Sr25519Keyring::Charlie, keyring::Sr25519Keyring::Dave, + keyring::Sr25519Keyring::Eve, ]; for validator in validators.iter() { Keystore::sr25519_generate_new( @@ -1240,12 +1247,16 @@ mod sanitizers { ) .unwrap(); } + let validator_ids = + validators.iter().map(|v| v.public().into()).collect::>(); + // Two scheduled parachains - ParaId(1) on CoreIndex(0) and ParaId(2) on CoreIndex(1) let scheduled = (0_usize..2) .into_iter() .map(|idx| (ParaId::from(1_u32 + idx as u32), CoreIndex::from(idx as u32))) .collect::>(); + // Two validator groups for each core let group_validators = |group_index: GroupIndex| { match group_index { group_index if group_index == GroupIndex::from(0) => Some(vec![0, 1]), @@ -1255,6 +1266,7 @@ mod sanitizers { .map(|m| m.into_iter().map(ValidatorIndex).collect::>()) }; + // Two backed candidates from each parachain let backed_candidates = (0_usize..2) .into_iter() .map(|idx0| { @@ -1283,13 +1295,14 @@ mod sanitizers { }) .collect::>(); - TestData { backed_candidates, scheduled_paras: scheduled } + TestData { backed_candidates, scheduled_paras: scheduled, validator_ids } } #[test] fn happy_path() { new_test_ext(MockGenesisConfig::default()).execute_with(|| { - let TestData { backed_candidates, scheduled_paras: scheduled } = get_test_data(); + let TestData { backed_candidates, scheduled_paras: scheduled, validator_ids: _ } = + get_test_data(); let has_concluded_invalid = |_idx: usize, _backed_candidate: &BackedCandidate| -> bool { false }; @@ -1311,7 +1324,8 @@ mod sanitizers { #[test] fn nothing_scheduled() { new_test_ext(MockGenesisConfig::default()).execute_with(|| { - let TestData { backed_candidates, scheduled_paras: _ } = get_test_data(); + let TestData { backed_candidates, scheduled_paras: _, validator_ids: _ } = + get_test_data(); let scheduled = &BTreeMap::new(); let has_concluded_invalid = |_idx: usize, _backed_candidate: &BackedCandidate| -> bool { false }; @@ -1329,7 +1343,8 @@ mod sanitizers { #[test] fn invalid_are_filtered_out() { new_test_ext(MockGenesisConfig::default()).execute_with(|| { - let TestData { backed_candidates, scheduled_paras: scheduled } = get_test_data(); + let TestData { backed_candidates, scheduled_paras: scheduled, validator_ids: _ } = + get_test_data(); // mark every second one as concluded invalid let set = { @@ -1354,5 +1369,85 @@ mod sanitizers { ); }); } + + #[test] + fn disabled() { + new_test_ext(MockGenesisConfig::default()).execute_with(|| { + let TestData { mut backed_candidates, scheduled_paras: _, validator_ids } = + get_test_data(); + + shared::Pallet::::set_active_validators_ascending(validator_ids); + shared::Pallet::::add_allowed_relay_parent( + default_header().hash(), + Default::default(), + 3, + 1, + ); + + assert_ne!(shared::Pallet::::disabled_validators().len(), 0); + + scheduler::Pallet::::set_validator_groups(vec![ + vec![ValidatorIndex(0), ValidatorIndex(1)], + vec![ValidatorIndex(2), ValidatorIndex(3)], + ]); + + let mut claimqueue = BTreeMap::new(); + claimqueue.insert( + CoreIndex::from(0), + VecDeque::from([Some(ParasEntry::new(Assignment::new(1.into()), 1))]), + ); + claimqueue.insert( + CoreIndex::from(1), + VecDeque::from([Some(ParasEntry::new(Assignment::new(2.into()), 1))]), + ); + scheduler::Pallet::::set_claimqueue(claimqueue); + + assert_eq!( + >::scheduled_paras().collect::>(), + vec![(CoreIndex(0), ParaId::from(1)), (CoreIndex(1), ParaId::from(2))] + ); + + // assert_ne!(shared::Pallet::::active_validator_indices().len(), 0); + + // let group_validators = |group_index: GroupIndex| { + // match group_index { + // group_index if group_index == GroupIndex::from(0) => Some(vec![0, 1]), + // group_index if group_index == GroupIndex::from(1) => Some(vec![2, 3]), + // _ => panic!("Group index out of bounds for 2 parachains and 1 parathread core"), + // } + // .map(|m| m.into_iter().map(ValidatorIndex).collect::>()) + // }; + + // let mut backed_candidates = (0_usize..2) + // .into_iter() + // .map(|idx0| { + // let idx1 = idx0 + 1; + // let mut candidate = TestCandidateBuilder { + // para_id: ParaId::from(idx1), + // relay_parent, + // pov_hash: Hash::repeat_byte(idx1 as u8), + // persisted_validation_data_hash: [42u8; 32].into(), + // hrmp_watermark: RELAY_PARENT_NUM, + // ..Default::default() + // } + // .build(); + + // collator_sign_candidate(Sr25519Keyring::One, &mut candidate); + + // let backed = back_candidate( + // candidate, + // &validators, + // group_validators(GroupIndex::from(idx0 as u32)).unwrap().as_ref(), + // &keystore, + // &signing_context, + // BackingKind::Threshold, + // ); + // backed + // }) + // .collect::>(); + + assert!(!filter_backed_statements::(&mut backed_candidates).unwrap()); + }); + } } } From 3567a930cc23e9375146027d021595f638abec7c Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 26 Oct 2023 11:19:55 +0300 Subject: [PATCH 11/34] Make disabled validators mock mutable --- polkadot/runtime/parachains/src/mock.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/polkadot/runtime/parachains/src/mock.rs b/polkadot/runtime/parachains/src/mock.rs index 523c9fd420860..393a5bf8ef5b2 100644 --- a/polkadot/runtime/parachains/src/mock.rs +++ b/polkadot/runtime/parachains/src/mock.rs @@ -447,7 +447,7 @@ thread_local! { pub static AVAILABILITY_REWARDS: RefCell> = RefCell::new(HashMap::new()); - pub static DISABLED_VALIDATORS: RefCell> = RefCell::new(vec![4]); + pub static DISABLED_VALIDATORS: RefCell> = RefCell::new(vec![]); } pub fn backing_rewards() -> HashMap { @@ -601,3 +601,7 @@ pub(crate) fn deregister_parachain(id: ParaId) { pub(crate) fn try_deregister_parachain(id: ParaId) -> crate::DispatchResult { frame_support::storage::transactional::with_storage_layer(|| Paras::schedule_para_cleanup(id)) } + +pub(crate) fn set_disabled_validators(disabled: Vec) { + DISABLED_VALIDATORS.with(|d| *d.borrow_mut() = disabled) +} From 04dc32a51eb1e059264d349292c4dd5bd62d50ab Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 26 Oct 2023 11:42:52 +0300 Subject: [PATCH 12/34] Extract common code in `get_test_data` --- .../parachains/src/paras_inherent/tests.rs | 143 ++++++++---------- 1 file changed, 61 insertions(+), 82 deletions(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/tests.rs b/polkadot/runtime/parachains/src/paras_inherent/tests.rs index fb08b7fd68e44..1cc388f13f2a5 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/tests.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/tests.rs @@ -1206,7 +1206,10 @@ mod sanitizers { } mod candidates { - use crate::scheduler::{common::Assignment, ParasEntry}; + use crate::{ + mock::set_disabled_validators, + scheduler::{common::Assignment, ParasEntry}, + }; use sp_std::collections::vec_deque::VecDeque; use super::*; @@ -1215,7 +1218,6 @@ mod sanitizers { struct TestData { backed_candidates: Vec, scheduled_paras: BTreeMap, - validator_ids: Vec, } // Generate test data for the candidates and asserts that the evnironment is set as expected @@ -1223,6 +1225,15 @@ mod sanitizers { fn get_test_data() -> TestData { const RELAY_PARENT_NUM: u32 = 3; + // Add the relay parent to `shared` pallet. Otherwise some code (e.g. filtering backing + // votes) won't behave correctly + shared::Pallet::::add_allowed_relay_parent( + default_header().hash(), + Default::default(), + 3, + 1, + ); + let header = default_header(); let relay_parent = header.hash(); let session_index = SessionIndex::from(0_u32); @@ -1247,8 +1258,11 @@ mod sanitizers { ) .unwrap(); } + + // Set active validators in `shared` pallet let validator_ids = validators.iter().map(|v| v.public().into()).collect::>(); + shared::Pallet::::set_active_validators_ascending(validator_ids); // Two scheduled parachains - ParaId(1) on CoreIndex(0) and ParaId(2) on CoreIndex(1) let scheduled = (0_usize..2) @@ -1256,7 +1270,25 @@ mod sanitizers { .map(|idx| (ParaId::from(1_u32 + idx as u32), CoreIndex::from(idx as u32))) .collect::>(); - // Two validator groups for each core + // Set the validator groups in `scheduler` + scheduler::Pallet::::set_validator_groups(vec![ + vec![ValidatorIndex(0), ValidatorIndex(1)], + vec![ValidatorIndex(2), ValidatorIndex(3)], + ]); + + // Update scheduler's claimqueue with the parachains + scheduler::Pallet::::set_claimqueue(BTreeMap::from([ + ( + CoreIndex::from(0), + VecDeque::from([Some(ParasEntry::new(Assignment::new(1.into()), 1))]), + ), + ( + CoreIndex::from(1), + VecDeque::from([Some(ParasEntry::new(Assignment::new(2.into()), 1))]), + ), + ])); + + // Callback used for backing candidates let group_validators = |group_index: GroupIndex| { match group_index { group_index if group_index == GroupIndex::from(0) => Some(vec![0, 1]), @@ -1295,14 +1327,29 @@ mod sanitizers { }) .collect::>(); - TestData { backed_candidates, scheduled_paras: scheduled, validator_ids } + // Some sanity checks + assert_eq!( + >::scheduled_paras().collect::>(), + vec![(CoreIndex(0), ParaId::from(1)), (CoreIndex(1), ParaId::from(2))] + ); + assert_eq!( + shared::Pallet::::active_validator_indices(), + vec![ + ValidatorIndex(0), + ValidatorIndex(1), + ValidatorIndex(2), + ValidatorIndex(3), + ValidatorIndex(4) + ] + ); + + TestData { backed_candidates, scheduled_paras: scheduled } } #[test] fn happy_path() { new_test_ext(MockGenesisConfig::default()).execute_with(|| { - let TestData { backed_candidates, scheduled_paras: scheduled, validator_ids: _ } = - get_test_data(); + let TestData { backed_candidates, scheduled_paras: scheduled } = get_test_data(); let has_concluded_invalid = |_idx: usize, _backed_candidate: &BackedCandidate| -> bool { false }; @@ -1324,8 +1371,7 @@ mod sanitizers { #[test] fn nothing_scheduled() { new_test_ext(MockGenesisConfig::default()).execute_with(|| { - let TestData { backed_candidates, scheduled_paras: _, validator_ids: _ } = - get_test_data(); + let TestData { backed_candidates, scheduled_paras: _ } = get_test_data(); let scheduled = &BTreeMap::new(); let has_concluded_invalid = |_idx: usize, _backed_candidate: &BackedCandidate| -> bool { false }; @@ -1343,8 +1389,7 @@ mod sanitizers { #[test] fn invalid_are_filtered_out() { new_test_ext(MockGenesisConfig::default()).execute_with(|| { - let TestData { backed_candidates, scheduled_paras: scheduled, validator_ids: _ } = - get_test_data(); + let TestData { backed_candidates, scheduled_paras: scheduled } = get_test_data(); // mark every second one as concluded invalid let set = { @@ -1371,81 +1416,15 @@ mod sanitizers { } #[test] - fn disabled() { + fn disabled_non_signing_validator_doesnt_get_filtered() { new_test_ext(MockGenesisConfig::default()).execute_with(|| { - let TestData { mut backed_candidates, scheduled_paras: _, validator_ids } = - get_test_data(); - - shared::Pallet::::set_active_validators_ascending(validator_ids); - shared::Pallet::::add_allowed_relay_parent( - default_header().hash(), - Default::default(), - 3, - 1, - ); - - assert_ne!(shared::Pallet::::disabled_validators().len(), 0); - - scheduler::Pallet::::set_validator_groups(vec![ - vec![ValidatorIndex(0), ValidatorIndex(1)], - vec![ValidatorIndex(2), ValidatorIndex(3)], - ]); - - let mut claimqueue = BTreeMap::new(); - claimqueue.insert( - CoreIndex::from(0), - VecDeque::from([Some(ParasEntry::new(Assignment::new(1.into()), 1))]), - ); - claimqueue.insert( - CoreIndex::from(1), - VecDeque::from([Some(ParasEntry::new(Assignment::new(2.into()), 1))]), - ); - scheduler::Pallet::::set_claimqueue(claimqueue); - - assert_eq!( - >::scheduled_paras().collect::>(), - vec![(CoreIndex(0), ParaId::from(1)), (CoreIndex(1), ParaId::from(2))] - ); + let TestData { mut backed_candidates, scheduled_paras: _ } = get_test_data(); - // assert_ne!(shared::Pallet::::active_validator_indices().len(), 0); - - // let group_validators = |group_index: GroupIndex| { - // match group_index { - // group_index if group_index == GroupIndex::from(0) => Some(vec![0, 1]), - // group_index if group_index == GroupIndex::from(1) => Some(vec![2, 3]), - // _ => panic!("Group index out of bounds for 2 parachains and 1 parathread core"), - // } - // .map(|m| m.into_iter().map(ValidatorIndex).collect::>()) - // }; - - // let mut backed_candidates = (0_usize..2) - // .into_iter() - // .map(|idx0| { - // let idx1 = idx0 + 1; - // let mut candidate = TestCandidateBuilder { - // para_id: ParaId::from(idx1), - // relay_parent, - // pov_hash: Hash::repeat_byte(idx1 as u8), - // persisted_validation_data_hash: [42u8; 32].into(), - // hrmp_watermark: RELAY_PARENT_NUM, - // ..Default::default() - // } - // .build(); - - // collator_sign_candidate(Sr25519Keyring::One, &mut candidate); - - // let backed = back_candidate( - // candidate, - // &validators, - // group_validators(GroupIndex::from(idx0 as u32)).unwrap().as_ref(), - // &keystore, - // &signing_context, - // BackingKind::Threshold, - // ); - // backed - // }) - // .collect::>(); + // Disable Eve + set_disabled_validators(vec![4]); + // Eve is disabled but no backing statement is signed by it so nothing should be + // filtered assert!(!filter_backed_statements::(&mut backed_candidates).unwrap()); }); } From cce71ee9f7aca6e2747d3f7b4e83ae1465fdb3f5 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 26 Oct 2023 15:07:58 +0300 Subject: [PATCH 13/34] Additional cases --- .../parachains/src/paras_inherent/tests.rs | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/polkadot/runtime/parachains/src/paras_inherent/tests.rs b/polkadot/runtime/parachains/src/paras_inherent/tests.rs index 1cc388f13f2a5..12549973c6284 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/tests.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/tests.rs @@ -1423,9 +1423,79 @@ mod sanitizers { // Disable Eve set_disabled_validators(vec![4]); + let before = backed_candidates.clone(); + // Eve is disabled but no backing statement is signed by it so nothing should be // filtered assert!(!filter_backed_statements::(&mut backed_candidates).unwrap()); + assert_eq!(backed_candidates, before); + }); + } + + #[test] + fn drop_single_disabled_vote() { + new_test_ext(MockGenesisConfig::default()).execute_with(|| { + let TestData { mut backed_candidates, scheduled_paras: _ } = get_test_data(); + + // Disable Alice + set_disabled_validators(vec![0]); + + // Verify the initial state is as expected + assert_eq!(backed_candidates.get(0).unwrap().validity_votes.len(), 2); + assert_eq!( + backed_candidates.get(0).unwrap().validator_indices.get(0).unwrap(), + true + ); + assert_eq!( + backed_candidates.get(0).unwrap().validator_indices.get(0).unwrap(), + true + ); + assert_eq!( + backed_candidates.get(0).unwrap().validator_indices.get(1).unwrap(), + true + ); + let untouched = backed_candidates.get(1).unwrap().clone(); + + // Eve is disabled but no backing statement is signed by it so nothing should be + // filtered + assert!(filter_backed_statements::(&mut backed_candidates).unwrap()); + + // there should still be two backed candidates + assert_eq!(backed_candidates.len(), 2); + // but the first one should have only one validity vote + assert_eq!(backed_candidates.get(0).unwrap().validity_votes.len(), 1); + // Validator 0 vote should be dropped, validator 1 - retained + assert_eq!( + backed_candidates.get(0).unwrap().validator_indices.get(0).unwrap(), + false + ); + assert_eq!( + backed_candidates.get(0).unwrap().validator_indices.get(1).unwrap(), + true + ); + // the second candidate shouldn't be modified + assert_eq!(*backed_candidates.get(1).unwrap(), untouched); + }); + } + + #[test] + fn drop_candidate_if_all_statements_are_from_disabled() { + new_test_ext(MockGenesisConfig::default()).execute_with(|| { + let TestData { mut backed_candidates, scheduled_paras: _ } = get_test_data(); + + // Disable Alice + set_disabled_validators(vec![0, 1]); + + // Verify the initial state is as expected + assert_eq!(backed_candidates.get(0).unwrap().validity_votes.len(), 2); + let untouched = backed_candidates.get(1).unwrap().clone(); + + // Eve is disabled but no backing statement is signed by it so nothing should be + // filtered + assert!(filter_backed_statements::(&mut backed_candidates).unwrap()); + + assert_eq!(backed_candidates.len(), 1); + assert_eq!(*backed_candidates.get(0).unwrap(), untouched); }); } } From 7a19f55b4cdc82f8880333572a9fb31cab817fb9 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 26 Oct 2023 15:08:48 +0300 Subject: [PATCH 14/34] Fixes in `filter_backed_statements` --- .../runtime/parachains/src/paras_inherent/mod.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index f8e369fad02c4..2f35d289e722e 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -1130,16 +1130,17 @@ fn filter_backed_statements( return false } }; - idx += 1; - - filtered = disabled_validators.contains(voted_validator_index); // If we are removing a validity vote - modify `validator_indices` too - if filtered { + let res = if disabled_validators.contains(voted_validator_index) { bc.validator_indices.set(idx, false); - } - - !filtered + filtered = true; + false // drop the validity vote + } else { + true // keep the validity vote + }; + idx += 1; + res }); } From 326018740588c9e1276173ed9742e1d2c51f8f8c Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 26 Oct 2023 16:47:46 +0300 Subject: [PATCH 15/34] Refactor `filter_backed_statements` --- .../parachains/src/paras_inherent/mod.rs | 59 ++++++++++--------- 1 file changed, 30 insertions(+), 29 deletions(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index 2f35d289e722e..00b8634e58d74 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -1091,8 +1091,8 @@ fn filter_backed_statements( ) { Some(group_idx) => group_idx, None => { - log::debug!(target: LOG_TARGET, "Can't fetch group index for core idx {:?}. Dropping the candidate.", core_idx); - return false + log::debug!(target: LOG_TARGET, "Can't fetch group index for core idx {:?}. Dropping the candidate.", core_idx); + return false }, }; @@ -1100,7 +1100,6 @@ fn filter_backed_statements( let validator_group = match >::group_validators(group_idx) { Some(validator_group) => validator_group, None => { - // TODO: this should be an assert? log::debug!(target: LOG_TARGET, "Can't get the validators from group {:?}. Dropping the candidate.", group_idx); return false } @@ -1118,34 +1117,36 @@ fn filter_backed_statements( }) .collect::>(); - { - // `validity_votes` should match `validator_indices` - let mut idx = 0; - bc.validity_votes.retain(|_| { - let voted_validator_index = match voted_validator_ids.get(idx) { - Some(validator_index) => validator_index, - None => { - log::debug!(target: LOG_TARGET, "Can't find the voted validator index {:?} in the validator group. Dropping the vote.", group_idx); - idx += 1; - return false - } - }; - - // If we are removing a validity vote - modify `validator_indices` too - let res = if disabled_validators.contains(voted_validator_index) { - bc.validator_indices.set(idx, false); - filtered = true; - false // drop the validity vote - } else { - true // keep the validity vote - }; - idx += 1; - res - }); + let validity_votes = std::mem::take(&mut bc.validity_votes); + let (validity_votes, dropped) : (Vec<(usize, ValidityAttestation)>, Vec<(usize, ValidityAttestation)>) = validity_votes.into_iter().enumerate().partition(|(idx, _)| { + let voted_validator_index = match voted_validator_ids.get(*idx) { + Some(validator_index) => validator_index, + None => { + log::debug!(target: LOG_TARGET, "Can't find the voted validator index {:?} in the validator group. Dropping the vote.", group_idx); + return false + } + }; + + !disabled_validators.contains(voted_validator_index) + }); + + if !dropped.is_empty() { + filtered = true; + } + + if validity_votes.is_empty() { + //everything is filtered out - remove the whole candidate + return false + } + + bc.validity_votes = validity_votes.into_iter().map(|(_, v)| v).collect::>(); + + // let removed_indecies = dropped.into_iter().map(|(idx, _)| idx).collect::>(); + for idx in dropped.into_iter().map(|(idx, _)| idx) { + bc.validator_indices.set(idx, false); } - // Remove the candidate if all entries are filtered out - !bc.validity_votes.is_empty() + true }); if !filtered { From 876655daee63f2b95136180f4c0eaaa1879b2229 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Sat, 28 Oct 2023 13:47:28 +0300 Subject: [PATCH 16/34] Apply suggestions from code review Co-authored-by: ordian --- polkadot/runtime/parachains/src/paras_inherent/mod.rs | 6 +++--- polkadot/runtime/parachains/src/paras_inherent/tests.rs | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index 00b8634e58d74..2a7418cd2d5f7 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -1041,12 +1041,12 @@ fn filter_backed_statements( ) -> Result { let disabled_validators = shared::Pallet::::disabled_validators(); - if disabled_validators.len() == 0 { + if disabled_validators.is_empty() { // No disabled validators - nothing to do return Ok(false) } - // `BackedCandidate` contains `validator_indices` which are indecies within the validator group. + // `BackedCandidate` contains `validator_indices` which are indices within the validator group. // To obtain `ValidatorIndex` from them we do the following steps: // 1. Get `para_id` from `CandidateDescriptor` // 2. Get the `core_idx` for the corresponding `para_id` @@ -1105,7 +1105,7 @@ fn filter_backed_statements( } }; - // `validator_indices` in `BackedCandidate` are indecies within the validator group. Convert + // `validator_indices` in `BackedCandidate` are indices within the validator group. Convert // them to `ValidatorId`. let voted_validator_ids = bc .validator_indices diff --git a/polkadot/runtime/parachains/src/paras_inherent/tests.rs b/polkadot/runtime/parachains/src/paras_inherent/tests.rs index 12549973c6284..2cd1f12dffd5e 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/tests.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/tests.rs @@ -1483,7 +1483,7 @@ mod sanitizers { new_test_ext(MockGenesisConfig::default()).execute_with(|| { let TestData { mut backed_candidates, scheduled_paras: _ } = get_test_data(); - // Disable Alice + // Disable Alice and Bob set_disabled_validators(vec![0, 1]); // Verify the initial state is as expected From ed2d430cbab713e4bc3138231c57dd35eff9b325 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 31 Oct 2023 10:53:33 +0200 Subject: [PATCH 17/34] wip: Filtering backed statements from disabled validators is performed in `sanitize_backed_candidates` --- .../parachains/src/paras_inherent/mod.rs | 44 ++++++++++--------- .../parachains/src/paras_inherent/tests.rs | 36 +++++++++++++-- 2 files changed, 56 insertions(+), 24 deletions(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index 2a7418cd2d5f7..1db28d54eb442 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -30,7 +30,8 @@ use crate::{ metrics::METRICS, paras, scheduler::{self, FreedReason}, - shared, ParaId, + shared::{self, AllowedRelayParentsTracker}, + ParaId, }; use bitvec::prelude::BitVec; use frame_support::{ @@ -430,9 +431,6 @@ impl Pallet { T::DisputesHandler::filter_dispute_data(set, post_conclusion_acceptance_period) }; - // Filter out backing statements from disabled validators - let statements_were_dropped = filter_backed_statements::(&mut backed_candidates)?; - // Limit the disputes first, since the following statements depend on the votes include // here. let (checked_disputes_sets, checked_disputes_sets_consumed_weight) = @@ -483,7 +481,11 @@ impl Pallet { } ensure!(all_weight_before.all_lte(max_block_weight), Error::::InherentOverweight); - ensure!(!statements_were_dropped, Error::::InherentOverweight); + // TODO + // ensure!( + // !statements_were_dropped, + // Error::::InherentOverweight + // ); all_weight_before }; @@ -593,6 +595,7 @@ impl Pallet { let backed_candidates = sanitize_backed_candidates::( backed_candidates, + &allowed_relay_parents, |candidate_idx: usize, backed_candidate: &BackedCandidate<::Hash>| -> bool { @@ -920,6 +923,7 @@ fn sanitize_backed_candidates< F: FnMut(usize, &BackedCandidate) -> bool, >( mut backed_candidates: Vec>, + allowed_relay_parents: &AllowedRelayParentsTracker>, mut candidate_has_concluded_invalid_dispute_or_is_invalid: F, scheduled: &BTreeMap, ) -> Vec> { @@ -940,6 +944,13 @@ fn sanitize_backed_candidates< scheduled.get(&desc.para_id).is_some() }); + // Filter out backing statements from disabled validators + filter_backed_statements_from_disabled::( + &mut backed_candidates, + &allowed_relay_parents, + scheduled, + ); + // Sort the `Vec` last, once there is a guarantee that these // `BackedCandidates` references the expected relay chain parent, // but more importantly are scheduled for a free core. @@ -1036,27 +1047,18 @@ fn limit_and_sanitize_disputes< // Filters statements from disabled validators in `BackedCandidate` and `MultiDisputeStatementSet`. // Returns `true` if at least one statement is removed and `false` otherwise. -fn filter_backed_statements( +fn filter_backed_statements_from_disabled( backed_candidates: &mut Vec::Hash>>, -) -> Result { + allowed_relay_parents: &AllowedRelayParentsTracker>, + para_id_to_core_idx: &BTreeMap, +) -> bool { let disabled_validators = shared::Pallet::::disabled_validators(); if disabled_validators.is_empty() { // No disabled validators - nothing to do - return Ok(false) + return false } - // `BackedCandidate` contains `validator_indices` which are indices within the validator group. - // To obtain `ValidatorIndex` from them we do the following steps: - // 1. Get `para_id` from `CandidateDescriptor` - // 2. Get the `core_idx` for the corresponding `para_id` - // 3. Get the validator group assigned to the corresponding core idx - - // Map `para_id` to `core_idx` for step 2 from the above list - let para_id_to_core_idx = >::scheduled_paras() - .map(|(core_idx, para_id)| (para_id, core_idx)) - .collect::>(); - // Flag which will be returned. Set to `true` if at least one vote is filtered. let mut filtered = false; @@ -1074,7 +1076,7 @@ fn filter_backed_statements( // Get relay parent block number of the candidate. We need this to get the group index // assigned to this core at this block number - let relay_parent_block_number = match >::allowed_relay_parents() + let relay_parent_block_number = match allowed_relay_parents .acquire_info(bc.descriptor().relay_parent, None) { Some((_, block_num)) => block_num, None => { @@ -1153,5 +1155,5 @@ fn filter_backed_statements( filtered = backed_len_before > backed_candidates.len(); } - Ok(filtered) + filtered } diff --git a/polkadot/runtime/parachains/src/paras_inherent/tests.rs b/polkadot/runtime/parachains/src/paras_inherent/tests.rs index 2cd1f12dffd5e..5c2e68d9ffff2 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/tests.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/tests.rs @@ -1357,6 +1357,7 @@ mod sanitizers { assert_eq!( sanitize_backed_candidates::( backed_candidates.clone(), + &>::allowed_relay_parents(), has_concluded_invalid, &scheduled ), @@ -1378,6 +1379,7 @@ mod sanitizers { assert!(sanitize_backed_candidates::( backed_candidates.clone(), + &>::allowed_relay_parents(), has_concluded_invalid, &scheduled ) @@ -1406,6 +1408,7 @@ mod sanitizers { assert_eq!( sanitize_backed_candidates::( backed_candidates.clone(), + &>::allowed_relay_parents(), has_concluded_invalid, &scheduled ) @@ -1425,9 +1428,18 @@ mod sanitizers { let before = backed_candidates.clone(); + // TODO: fix this. No need to keep this in runtime anymore. + let scheduled = >::scheduled_paras() + .map(|(core_idx, para_id)| (para_id, core_idx)) + .collect(); + // Eve is disabled but no backing statement is signed by it so nothing should be // filtered - assert!(!filter_backed_statements::(&mut backed_candidates).unwrap()); + assert!(!filter_backed_statements_from_disabled::( + &mut backed_candidates, + &>::allowed_relay_parents(), + &scheduled + )); assert_eq!(backed_candidates, before); }); } @@ -1456,9 +1468,18 @@ mod sanitizers { ); let untouched = backed_candidates.get(1).unwrap().clone(); + // TODO: fix this. No need to keep this in runtime anymore. + let scheduled = >::scheduled_paras() + .map(|(core_idx, para_id)| (para_id, core_idx)) + .collect(); + // Eve is disabled but no backing statement is signed by it so nothing should be // filtered - assert!(filter_backed_statements::(&mut backed_candidates).unwrap()); + assert!(filter_backed_statements_from_disabled::( + &mut backed_candidates, + &>::allowed_relay_parents(), + &scheduled + )); // there should still be two backed candidates assert_eq!(backed_candidates.len(), 2); @@ -1490,9 +1511,18 @@ mod sanitizers { assert_eq!(backed_candidates.get(0).unwrap().validity_votes.len(), 2); let untouched = backed_candidates.get(1).unwrap().clone(); + // TODO: fix this. No need to keep this in runtime anymore. + let scheduled = >::scheduled_paras() + .map(|(core_idx, para_id)| (para_id, core_idx)) + .collect(); + // Eve is disabled but no backing statement is signed by it so nothing should be // filtered - assert!(filter_backed_statements::(&mut backed_candidates).unwrap()); + assert!(filter_backed_statements_from_disabled::( + &mut backed_candidates, + &>::allowed_relay_parents(), + &scheduled + )); assert_eq!(backed_candidates.len(), 1); assert_eq!(*backed_candidates.get(0).unwrap(), untouched); From 9a8985a6cc978f6592e85cb652eb38df487ba13d Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 31 Oct 2023 11:20:44 +0200 Subject: [PATCH 18/34] para_id_to_core_idx -> scheduled --- polkadot/runtime/parachains/src/paras_inherent/mod.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index 1db28d54eb442..1c5dd7972f908 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -1050,7 +1050,7 @@ fn limit_and_sanitize_disputes< fn filter_backed_statements_from_disabled( backed_candidates: &mut Vec::Hash>>, allowed_relay_parents: &AllowedRelayParentsTracker>, - para_id_to_core_idx: &BTreeMap, + scheduled: &BTreeMap, ) -> bool { let disabled_validators = shared::Pallet::::disabled_validators(); @@ -1066,7 +1066,7 @@ fn filter_backed_statements_from_disabled let backed_len_before = backed_candidates.len(); backed_candidates.retain_mut(|bc| { // Get `core_idx` assigned to the `para_id` of the candidate (step 2) - let core_idx = match para_id_to_core_idx.get(&bc.descriptor().para_id) { + let core_idx = match scheduled.get(&bc.descriptor().para_id) { Some(core_idx) => *core_idx, None => { log::debug!(target: LOG_TARGET, "Can't get core idx got a backed candidate for para id {:?}. Dropping the candidate.", bc.descriptor().para_id); @@ -1143,7 +1143,6 @@ fn filter_backed_statements_from_disabled bc.validity_votes = validity_votes.into_iter().map(|(_, v)| v).collect::>(); - // let removed_indecies = dropped.into_iter().map(|(idx, _)| idx).collect::>(); for idx in dropped.into_iter().map(|(idx, _)| idx) { bc.validator_indices.set(idx, false); } From f7544d747865834615c6de454c2d85406990c442 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 31 Oct 2023 13:18:51 +0200 Subject: [PATCH 19/34] Fix code duplication in tests --- .../parachains/src/paras_inherent/tests.rs | 32 ++++--------------- 1 file changed, 6 insertions(+), 26 deletions(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/tests.rs b/polkadot/runtime/parachains/src/paras_inherent/tests.rs index 5c2e68d9ffff2..db64d3df119aa 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/tests.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/tests.rs @@ -1242,7 +1242,6 @@ mod sanitizers { let keystore = Arc::new(keystore) as KeystorePtr; let signing_context = SigningContext { parent_hash: relay_parent, session_index }; - // 5 active validators where `Eve` is disabled let validators = vec![ keyring::Sr25519Keyring::Alice, keyring::Sr25519Keyring::Bob, @@ -1421,24 +1420,19 @@ mod sanitizers { #[test] fn disabled_non_signing_validator_doesnt_get_filtered() { new_test_ext(MockGenesisConfig::default()).execute_with(|| { - let TestData { mut backed_candidates, scheduled_paras: _ } = get_test_data(); + let TestData { mut backed_candidates, scheduled_paras } = get_test_data(); // Disable Eve set_disabled_validators(vec![4]); let before = backed_candidates.clone(); - // TODO: fix this. No need to keep this in runtime anymore. - let scheduled = >::scheduled_paras() - .map(|(core_idx, para_id)| (para_id, core_idx)) - .collect(); - // Eve is disabled but no backing statement is signed by it so nothing should be // filtered assert!(!filter_backed_statements_from_disabled::( &mut backed_candidates, &>::allowed_relay_parents(), - &scheduled + &scheduled_paras )); assert_eq!(backed_candidates, before); }); @@ -1447,7 +1441,7 @@ mod sanitizers { #[test] fn drop_single_disabled_vote() { new_test_ext(MockGenesisConfig::default()).execute_with(|| { - let TestData { mut backed_candidates, scheduled_paras: _ } = get_test_data(); + let TestData { mut backed_candidates, scheduled_paras } = get_test_data(); // Disable Alice set_disabled_validators(vec![0]); @@ -1468,17 +1462,10 @@ mod sanitizers { ); let untouched = backed_candidates.get(1).unwrap().clone(); - // TODO: fix this. No need to keep this in runtime anymore. - let scheduled = >::scheduled_paras() - .map(|(core_idx, para_id)| (para_id, core_idx)) - .collect(); - - // Eve is disabled but no backing statement is signed by it so nothing should be - // filtered assert!(filter_backed_statements_from_disabled::( &mut backed_candidates, &>::allowed_relay_parents(), - &scheduled + &scheduled_paras )); // there should still be two backed candidates @@ -1502,7 +1489,7 @@ mod sanitizers { #[test] fn drop_candidate_if_all_statements_are_from_disabled() { new_test_ext(MockGenesisConfig::default()).execute_with(|| { - let TestData { mut backed_candidates, scheduled_paras: _ } = get_test_data(); + let TestData { mut backed_candidates, scheduled_paras } = get_test_data(); // Disable Alice and Bob set_disabled_validators(vec![0, 1]); @@ -1511,17 +1498,10 @@ mod sanitizers { assert_eq!(backed_candidates.get(0).unwrap().validity_votes.len(), 2); let untouched = backed_candidates.get(1).unwrap().clone(); - // TODO: fix this. No need to keep this in runtime anymore. - let scheduled = >::scheduled_paras() - .map(|(core_idx, para_id)| (para_id, core_idx)) - .collect(); - - // Eve is disabled but no backing statement is signed by it so nothing should be - // filtered assert!(filter_backed_statements_from_disabled::( &mut backed_candidates, &>::allowed_relay_parents(), - &scheduled + &scheduled_paras )); assert_eq!(backed_candidates.len(), 1); From 59fc1764d0d39ffd4d197bef5168ffbd1b8495f1 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 31 Oct 2023 14:43:21 +0200 Subject: [PATCH 20/34] Fix a compilation error --- polkadot/runtime/parachains/src/paras_inherent/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index 1c5dd7972f908..6f4a3a742749a 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -1119,7 +1119,7 @@ fn filter_backed_statements_from_disabled }) .collect::>(); - let validity_votes = std::mem::take(&mut bc.validity_votes); + let validity_votes = sp_std::mem::take(&mut bc.validity_votes); let (validity_votes, dropped) : (Vec<(usize, ValidityAttestation)>, Vec<(usize, ValidityAttestation)>) = validity_votes.into_iter().enumerate().partition(|(idx, _)| { let voted_validator_index = match voted_validator_ids.get(*idx) { Some(validator_index) => validator_index, From cb2fe60e0d0e8c4d96f1f5ddf15f45a52e333695 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 2 Nov 2023 11:34:11 +0200 Subject: [PATCH 21/34] Fix backing votes filtering - if more than `effective_minimum_backing_votes` is removed, drop the candidate --- .../runtime/parachains/src/paras_inherent/mod.rs | 14 ++++++++++---- .../runtime/parachains/src/paras_inherent/tests.rs | 5 +++++ 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index 6f4a3a742749a..bf3f86c549270 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -43,8 +43,8 @@ use frame_support::{ use frame_system::pallet_prelude::*; use pallet_babe::{self, ParentBlockRandomness}; use primitives::{ - BackedCandidate, CandidateHash, CandidateReceipt, CheckedDisputeStatementSet, - CheckedMultiDisputeStatementSet, CoreIndex, DisputeStatementSet, + effective_minimum_backing_votes, BackedCandidate, CandidateHash, CandidateReceipt, + CheckedDisputeStatementSet, CheckedMultiDisputeStatementSet, CoreIndex, DisputeStatementSet, InherentData as ParachainsInherentData, MultiDisputeStatementSet, ScrapedOnChainVotes, SessionIndex, SignedAvailabilityBitfields, SigningContext, UncheckedSignedAvailabilityBitfield, UncheckedSignedAvailabilityBitfields, ValidatorId, ValidatorIndex, ValidityAttestation, @@ -1136,8 +1136,14 @@ fn filter_backed_statements_from_disabled filtered = true; } - if validity_votes.is_empty() { - //everything is filtered out - remove the whole candidate + // By filtering votes we might render the candidate invalid and cause a failure in + // [`process_candidates`]. To avoid this we have to perform a sanity check here. If there + // are not enough backing votes after filtering we will remove the whole candidate. + if validity_votes.len() < effective_minimum_backing_votes( + validator_group.len(), + configuration::Pallet::::config().minimum_backing_votes + + ) { return false } diff --git a/polkadot/runtime/parachains/src/paras_inherent/tests.rs b/polkadot/runtime/parachains/src/paras_inherent/tests.rs index db64d3df119aa..d08ee086000b8 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/tests.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/tests.rs @@ -1446,6 +1446,11 @@ mod sanitizers { // Disable Alice set_disabled_validators(vec![0]); + // Update `minimum_backing_votes` in HostConfig + let mut hc = configuration::Pallet::::config(); + hc.minimum_backing_votes = 1; + configuration::Pallet::::force_set_active_config(hc); + // Verify the initial state is as expected assert_eq!(backed_candidates.get(0).unwrap().validity_votes.len(), 2); assert_eq!( From be85b5d91f61271c6973c798245ec70c39692ff8 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 2 Nov 2023 13:43:09 +0200 Subject: [PATCH 22/34] Ensure inherent data contains no backing votes from disabled validators in execution context --- .../parachains/src/paras_inherent/mod.rs | 68 ++++++++++++------- .../parachains/src/paras_inherent/tests.rs | 38 +++++++---- 2 files changed, 67 insertions(+), 39 deletions(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index bf3f86c549270..8b98eb50fb25b 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -481,11 +481,6 @@ impl Pallet { } ensure!(all_weight_before.all_lte(max_block_weight), Error::::InherentOverweight); - // TODO - // ensure!( - // !statements_were_dropped, - // Error::::InherentOverweight - // ); all_weight_before }; @@ -593,18 +588,19 @@ impl Pallet { METRICS.on_candidates_processed_total(backed_candidates.len() as u64); - let backed_candidates = sanitize_backed_candidates::( - backed_candidates, - &allowed_relay_parents, - |candidate_idx: usize, - backed_candidate: &BackedCandidate<::Hash>| - -> bool { - let para_id = backed_candidate.descriptor().para_id; - let prev_context = >::para_most_recent_context(para_id); - let check_ctx = CandidateCheckContext::::new(prev_context); - - // never include a concluded-invalid candidate - current_concluded_invalid_disputes.contains(&backed_candidate.hash()) || + let SanitizedBackedCandidates { backed_candidates, votes_from_disabled_were_dropped } = + sanitize_backed_candidates::( + backed_candidates, + &allowed_relay_parents, + |candidate_idx: usize, + backed_candidate: &BackedCandidate<::Hash>| + -> bool { + let para_id = backed_candidate.descriptor().para_id; + let prev_context = >::para_most_recent_context(para_id); + let check_ctx = CandidateCheckContext::::new(prev_context); + + // never include a concluded-invalid candidate + current_concluded_invalid_disputes.contains(&backed_candidate.hash()) || // Instead of checking the candidates with code upgrades twice // move the checking up here and skip it in the training wheels fallback. // That way we avoid possible duplicate checks while assuring all @@ -614,12 +610,18 @@ impl Pallet { check_ctx .verify_backed_candidate(&allowed_relay_parents, candidate_idx, backed_candidate) .is_err() - }, - &scheduled, - ); + }, + &scheduled, + ); METRICS.on_candidates_sanitized(backed_candidates.len() as u64); + // In execution context there should be no backing votes from disabled validators because + // they should have been filtered out during inherent data preparation. Abort in such cases. + if context == ProcessInherentDataContext::Enter { + ensure!(!votes_from_disabled_were_dropped, Error::::InherentOverweight); + } + // Process backed candidates according to scheduled cores. let inclusion::ProcessedCandidates::< as HeaderT>::Hash> { core_indices: occupied, @@ -907,7 +909,19 @@ pub(crate) fn sanitize_bitfields( bitfields } -/// Filter out any candidates that have a concluded invalid dispute. +// Represents a result from `sanitize_backed_candidates` +#[derive(Debug, PartialEq)] +struct SanitizedBackedCandidates { + // Sanitized backed candidates. The `Vec` is sorted according to the occupied core index. + backed_candidates: Vec>, + // Set to true if any votes from disabled validators were dropped. + votes_from_disabled_were_dropped: bool, +} + +/// Filter out: +/// 1. any candidates that have a concluded invalid dispute +/// 2. all backing votes from disabled validators +/// 3. any candidates that end up with less than `effective_minimum_backing_votes` backing votes /// /// `scheduled` follows the same naming scheme as provided in the /// guide: Currently `free` but might become `occupied`. @@ -917,7 +931,8 @@ pub(crate) fn sanitize_bitfields( /// `candidate_has_concluded_invalid_dispute` must return `true` if the candidate /// is disputed, false otherwise. The passed `usize` is the candidate index. /// -/// The returned `Vec` is sorted according to the occupied core index. +/// Returns struct `SanitizedBackedCandidates` where `backed_candidates` are sorted according to the +/// occupied core index. fn sanitize_backed_candidates< T: crate::inclusion::Config, F: FnMut(usize, &BackedCandidate) -> bool, @@ -926,7 +941,7 @@ fn sanitize_backed_candidates< allowed_relay_parents: &AllowedRelayParentsTracker>, mut candidate_has_concluded_invalid_dispute_or_is_invalid: F, scheduled: &BTreeMap, -) -> Vec> { +) -> SanitizedBackedCandidates { // Remove any candidates that were concluded invalid. // This does not assume sorting. backed_candidates.indexed_retain(move |candidate_idx, backed_candidate| { @@ -945,7 +960,7 @@ fn sanitize_backed_candidates< }); // Filter out backing statements from disabled validators - filter_backed_statements_from_disabled::( + let dropped_disabled = filter_backed_statements_from_disabled::( &mut backed_candidates, &allowed_relay_parents, scheduled, @@ -961,7 +976,10 @@ fn sanitize_backed_candidates< scheduled[&x.descriptor().para_id].cmp(&scheduled[&y.descriptor().para_id]) }); - backed_candidates + SanitizedBackedCandidates { + backed_candidates, + votes_from_disabled_were_dropped: dropped_disabled, + } } /// Derive entropy from babe provided per block randomness. diff --git a/polkadot/runtime/parachains/src/paras_inherent/tests.rs b/polkadot/runtime/parachains/src/paras_inherent/tests.rs index d08ee086000b8..6d6f05cc539db 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/tests.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/tests.rs @@ -1360,7 +1360,10 @@ mod sanitizers { has_concluded_invalid, &scheduled ), - backed_candidates + SanitizedBackedCandidates { + backed_candidates, + votes_from_disabled_were_dropped: false + } ); {} @@ -1376,13 +1379,18 @@ mod sanitizers { let has_concluded_invalid = |_idx: usize, _backed_candidate: &BackedCandidate| -> bool { false }; - assert!(sanitize_backed_candidates::( + let SanitizedBackedCandidates { + backed_candidates: sanitized_backed_candidates, + votes_from_disabled_were_dropped, + } = sanitize_backed_candidates::( backed_candidates.clone(), &>::allowed_relay_parents(), has_concluded_invalid, - &scheduled - ) - .is_empty()); + &scheduled, + ); + + assert!(sanitized_backed_candidates.is_empty()); + assert!(!votes_from_disabled_were_dropped); }); } @@ -1404,16 +1412,18 @@ mod sanitizers { }; let has_concluded_invalid = |_idx: usize, candidate: &BackedCandidate| set.contains(&candidate.hash()); - assert_eq!( - sanitize_backed_candidates::( - backed_candidates.clone(), - &>::allowed_relay_parents(), - has_concluded_invalid, - &scheduled - ) - .len(), - backed_candidates.len() / 2 + let SanitizedBackedCandidates { + backed_candidates: sanitized_backed_candidates, + votes_from_disabled_were_dropped, + } = sanitize_backed_candidates::( + backed_candidates.clone(), + &>::allowed_relay_parents(), + has_concluded_invalid, + &scheduled, ); + + assert_eq!(sanitized_backed_candidates.len(), 1); + assert!(!votes_from_disabled_were_dropped); }); } From f472bb165b1559674e14268c392a39cfcd8b27bd Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Thu, 2 Nov 2023 14:09:18 +0200 Subject: [PATCH 23/34] Comments and small fixes --- .../runtime/parachains/src/paras_inherent/mod.rs | 10 ++++++---- .../runtime/parachains/src/paras_inherent/tests.rs | 14 ++++++++------ 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index 8b98eb50fb25b..9d84f87510a0b 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -379,6 +379,7 @@ impl Pallet { let bitfields_weight = signed_bitfields_weight::(&bitfields); let disputes_weight = multi_dispute_statement_sets_weight::(&disputes); + // Weight before filtering/sanitization let all_weight_before = candidates_weight + bitfields_weight + disputes_weight; METRICS.on_before_filter(all_weight_before.ref_time()); @@ -616,8 +617,9 @@ impl Pallet { METRICS.on_candidates_sanitized(backed_candidates.len() as u64); - // In execution context there should be no backing votes from disabled validators because - // they should have been filtered out during inherent data preparation. Abort in such cases. + // In `Enter` context (invoked during execution) there should be no backing votes from + // disabled validators because they should have been filtered out during inherent data + // preparation. Abort in such cases. if context == ProcessInherentDataContext::Enter { ensure!(!votes_from_disabled_were_dropped, Error::::InherentOverweight); } @@ -909,12 +911,12 @@ pub(crate) fn sanitize_bitfields( bitfields } -// Represents a result from `sanitize_backed_candidates` +// Result from `sanitize_backed_candidates` call #[derive(Debug, PartialEq)] struct SanitizedBackedCandidates { // Sanitized backed candidates. The `Vec` is sorted according to the occupied core index. backed_candidates: Vec>, - // Set to true if any votes from disabled validators were dropped. + // Set to true if any votes from disabled validators were dropped from the input. votes_from_disabled_were_dropped: bool, } diff --git a/polkadot/runtime/parachains/src/paras_inherent/tests.rs b/polkadot/runtime/parachains/src/paras_inherent/tests.rs index 6d6f05cc539db..2afa4375e76c7 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/tests.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/tests.rs @@ -1220,7 +1220,7 @@ mod sanitizers { scheduled_paras: BTreeMap, } - // Generate test data for the candidates and asserts that the evnironment is set as expected + // Generate test data for the candidates and assert that the evnironment is set as expected // (check the comments for details) fn get_test_data() -> TestData { const RELAY_PARENT_NUM: u32 = 3; @@ -1230,7 +1230,7 @@ mod sanitizers { shared::Pallet::::add_allowed_relay_parent( default_header().hash(), Default::default(), - 3, + RELAY_PARENT_NUM, 1, ); @@ -1326,7 +1326,7 @@ mod sanitizers { }) .collect::>(); - // Some sanity checks + // State sanity checks assert_eq!( >::scheduled_paras().collect::>(), vec![(CoreIndex(0), ParaId::from(1)), (CoreIndex(1), ParaId::from(2))] @@ -1422,7 +1422,7 @@ mod sanitizers { &scheduled, ); - assert_eq!(sanitized_backed_candidates.len(), 1); + assert_eq!(sanitized_backed_candidates.len(), backed_candidates.len() / 2); assert!(!votes_from_disabled_were_dropped); }); } @@ -1449,14 +1449,16 @@ mod sanitizers { } #[test] - fn drop_single_disabled_vote() { + fn drop_statements_from_disabled_without_dropping_candidate() { new_test_ext(MockGenesisConfig::default()).execute_with(|| { let TestData { mut backed_candidates, scheduled_paras } = get_test_data(); // Disable Alice set_disabled_validators(vec![0]); - // Update `minimum_backing_votes` in HostConfig + // Update `minimum_backing_votes` in HostConfig. We want `minimum_backing_votes` set + // to one so that the candidate will have enough backing votes even after dropping + // Alice's one. let mut hc = configuration::Pallet::::config(); hc.minimum_backing_votes = 1; configuration::Pallet::::force_set_active_config(hc); From 53aaffa438cb800f32ee1789978dea7dc1868fb6 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 3 Nov 2023 09:53:58 +0200 Subject: [PATCH 24/34] Fix disabled statements filtering --- .../parachains/src/paras_inherent/mod.rs | 61 ++++++++----------- 1 file changed, 24 insertions(+), 37 deletions(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index 9d84f87510a0b..a895713dfea6e 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -1072,7 +1072,8 @@ fn filter_backed_statements_from_disabled allowed_relay_parents: &AllowedRelayParentsTracker>, scheduled: &BTreeMap, ) -> bool { - let disabled_validators = shared::Pallet::::disabled_validators(); + let disabled_validators = + BTreeSet::<_>::from_iter(shared::Pallet::::disabled_validators().into_iter()); if disabled_validators.is_empty() { // No disabled validators - nothing to do @@ -1085,7 +1086,13 @@ fn filter_backed_statements_from_disabled // Process all backed candidates. let backed_len_before = backed_candidates.len(); backed_candidates.retain_mut(|bc| { - // Get `core_idx` assigned to the `para_id` of the candidate (step 2) + // `validator_indices` in `BackedCandidates` are indices within the validator group assigned to the parachain. + // To obtain this group we need: + // 1. Core index assigned to the parachain which has produced the candidate + // 2. The relay chain block number of the candidate + // We perform these steps below. + + // Get `core_idx` assigned to the `para_id` of the candidate let core_idx = match scheduled.get(&bc.descriptor().para_id) { Some(core_idx) => *core_idx, None => { @@ -1094,8 +1101,7 @@ fn filter_backed_statements_from_disabled } }; - // Get relay parent block number of the candidate. We need this to get the group index - // assigned to this core at this block number + // Get relay parent block number of the candidate. We need this to get the group index assigned to this core at this block number let relay_parent_block_number = match allowed_relay_parents .acquire_info(bc.descriptor().relay_parent, None) { Some((_, block_num)) => block_num, @@ -1105,7 +1111,6 @@ fn filter_backed_statements_from_disabled } }; - // Get the group index for the core let group_idx = match >::group_assigned_to_core( core_idx, @@ -1127,39 +1132,26 @@ fn filter_backed_statements_from_disabled } }; - // `validator_indices` in `BackedCandidate` are indices within the validator group. Convert - // them to `ValidatorId`. - let voted_validator_ids = bc - .validator_indices - .iter() - .enumerate() - .filter_map(|(idx, bitval)| match *bitval { - true => validator_group.get(idx), // drop indecies not found in the validator group. This will lead to filtering the vote - false => None, - }) - .collect::>(); - - let validity_votes = sp_std::mem::take(&mut bc.validity_votes); - let (validity_votes, dropped) : (Vec<(usize, ValidityAttestation)>, Vec<(usize, ValidityAttestation)>) = validity_votes.into_iter().enumerate().partition(|(idx, _)| { - let voted_validator_index = match voted_validator_ids.get(*idx) { - Some(validator_index) => validator_index, - None => { - log::debug!(target: LOG_TARGET, "Can't find the voted validator index {:?} in the validator group. Dropping the vote.", group_idx); - return false - } - }; - - !disabled_validators.contains(voted_validator_index) - }); + // Bitmask with the disabled indecies within the validator group + let disabled_indices = BitVec::::from_iter(validator_group.iter().map(|idx| disabled_validators.contains(idx))); + // The indices of statements from disabled validators in `BackedCandidate`. We have to drop these. + let indices_to_drop = disabled_indices.clone() & &bc.validator_indices; + // Apply the bitmask to drop the disabled validator from `validator_indices` + bc.validator_indices &= !disabled_indices; + // Remove the corresponding votes from `validity_votes` + for idx in indices_to_drop.iter_ones().rev() { + bc.validity_votes.remove(idx); + } - if !dropped.is_empty() { + // If at least one statement was dropped we need to return `true` + if indices_to_drop.count_ones() > 0 { filtered = true; } // By filtering votes we might render the candidate invalid and cause a failure in // [`process_candidates`]. To avoid this we have to perform a sanity check here. If there // are not enough backing votes after filtering we will remove the whole candidate. - if validity_votes.len() < effective_minimum_backing_votes( + if bc.validity_votes.len() < effective_minimum_backing_votes( validator_group.len(), configuration::Pallet::::config().minimum_backing_votes @@ -1167,15 +1159,10 @@ fn filter_backed_statements_from_disabled return false } - bc.validity_votes = validity_votes.into_iter().map(|(_, v)| v).collect::>(); - - for idx in dropped.into_iter().map(|(idx, _)| idx) { - bc.validator_indices.set(idx, false); - } - true }); + // Also return `true` if a whole candidate was dropped from the set if !filtered { filtered = backed_len_before > backed_candidates.len(); } From 16823f471fbddb1b69bdf56c941d00b0154d2d83 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Fri, 3 Nov 2023 10:16:55 +0200 Subject: [PATCH 25/34] Updated comments --- .../parachains/src/paras_inherent/mod.rs | 26 +++++++++---------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index a895713dfea6e..7e1a6ea2a3fcd 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -619,7 +619,7 @@ impl Pallet { // In `Enter` context (invoked during execution) there should be no backing votes from // disabled validators because they should have been filtered out during inherent data - // preparation. Abort in such cases. + // preparation (`ProvideInherent` context). Abort in such cases. if context == ProcessInherentDataContext::Enter { ensure!(!votes_from_disabled_were_dropped, Error::::InherentOverweight); } @@ -911,7 +911,7 @@ pub(crate) fn sanitize_bitfields( bitfields } -// Result from `sanitize_backed_candidates` call +// Result from `sanitize_backed_candidates` #[derive(Debug, PartialEq)] struct SanitizedBackedCandidates { // Sanitized backed candidates. The `Vec` is sorted according to the occupied core index. @@ -1065,8 +1065,8 @@ fn limit_and_sanitize_disputes< } } -// Filters statements from disabled validators in `BackedCandidate` and `MultiDisputeStatementSet`. -// Returns `true` if at least one statement is removed and `false` otherwise. +// Filters statements from disabled validators in `BackedCandidate`. Returns `true` if at least one +// statement is removed and `false` otherwise. fn filter_backed_statements_from_disabled( backed_candidates: &mut Vec::Hash>>, allowed_relay_parents: &AllowedRelayParentsTracker>, @@ -1080,23 +1080,21 @@ fn filter_backed_statements_from_disabled return false } + let backed_len_before = backed_candidates.len(); + // Flag which will be returned. Set to `true` if at least one vote is filtered. let mut filtered = false; - // Process all backed candidates. - let backed_len_before = backed_candidates.len(); + // Process all backed candidates. `validator_indices` in `BackedCandidates` are indices within + // the validator group assigned to the parachain. To obtain this group we need: + // 1. Core index assigned to the parachain which has produced the candidate + // 2. The relay chain block number of the candidate backed_candidates.retain_mut(|bc| { - // `validator_indices` in `BackedCandidates` are indices within the validator group assigned to the parachain. - // To obtain this group we need: - // 1. Core index assigned to the parachain which has produced the candidate - // 2. The relay chain block number of the candidate - // We perform these steps below. - // Get `core_idx` assigned to the `para_id` of the candidate let core_idx = match scheduled.get(&bc.descriptor().para_id) { Some(core_idx) => *core_idx, None => { - log::debug!(target: LOG_TARGET, "Can't get core idx got a backed candidate for para id {:?}. Dropping the candidate.", bc.descriptor().para_id); + log::debug!(target: LOG_TARGET, "Can't get core idx of a backed candidate for para id {:?}. Dropping the candidate.", bc.descriptor().para_id); return false } }; @@ -1118,7 +1116,7 @@ fn filter_backed_statements_from_disabled ) { Some(group_idx) => group_idx, None => { - log::debug!(target: LOG_TARGET, "Can't fetch group index for core idx {:?}. Dropping the candidate.", core_idx); + log::debug!(target: LOG_TARGET, "Can't get the group index for core idx {:?}. Dropping the candidate.", core_idx); return false }, }; From cb60cf0919065a361f2c59fb116489a71cf41ad0 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 15 Nov 2023 13:53:22 +0200 Subject: [PATCH 26/34] Apply suggestions from code review Co-authored-by: ordian --- polkadot/runtime/parachains/src/paras_inherent/mod.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index 7e1a6ea2a3fcd..0ca1862b0ebf5 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -1130,7 +1130,7 @@ fn filter_backed_statements_from_disabled } }; - // Bitmask with the disabled indecies within the validator group + // Bitmask with the disabled indices within the validator group let disabled_indices = BitVec::::from_iter(validator_group.iter().map(|idx| disabled_validators.contains(idx))); // The indices of statements from disabled validators in `BackedCandidate`. We have to drop these. let indices_to_drop = disabled_indices.clone() & &bc.validator_indices; @@ -1161,9 +1161,5 @@ fn filter_backed_statements_from_disabled }); // Also return `true` if a whole candidate was dropped from the set - if !filtered { - filtered = backed_len_before > backed_candidates.len(); - } - - filtered + filtered || backed_len_before != backed_candidates.len() } From 750f2db623d31eeeca3f18c49e0ffdbf0e55728a Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 15 Nov 2023 14:01:42 +0200 Subject: [PATCH 27/34] Code review feedback --- .../runtime/parachains/src/paras_inherent/mod.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index 0ca1862b0ebf5..3b4ef20cf6139 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -143,6 +143,8 @@ pub mod pallet { DisputeStatementsUnsortedOrDuplicates, /// A dispute statement was invalid. DisputeInvalid, + /// A candidate was backed by a disabled validator + BackedByDisabled, } /// Whether the paras inherent was included within this block. @@ -621,7 +623,7 @@ impl Pallet { // disabled validators because they should have been filtered out during inherent data // preparation (`ProvideInherent` context). Abort in such cases. if context == ProcessInherentDataContext::Enter { - ensure!(!votes_from_disabled_were_dropped, Error::::InherentOverweight); + ensure!(!votes_from_disabled_were_dropped, Error::::BackedByDisabled); } // Process backed candidates according to scheduled cores. @@ -1065,8 +1067,9 @@ fn limit_and_sanitize_disputes< } } -// Filters statements from disabled validators in `BackedCandidate`. Returns `true` if at least one -// statement is removed and `false` otherwise. +// Filters statements from disabled validators in `BackedCandidate`, non-scheduled candidates and +// few more sanity checks. Returns `true` if at least one statement is removed and `false` +// otherwise. fn filter_backed_statements_from_disabled( backed_candidates: &mut Vec::Hash>>, allowed_relay_parents: &AllowedRelayParentsTracker>, @@ -1085,6 +1088,8 @@ fn filter_backed_statements_from_disabled // Flag which will be returned. Set to `true` if at least one vote is filtered. let mut filtered = false; + let minimum_backing_votes = configuration::Pallet::::config().minimum_backing_votes; + // Process all backed candidates. `validator_indices` in `BackedCandidates` are indices within // the validator group assigned to the parachain. To obtain this group we need: // 1. Core index assigned to the parachain which has produced the candidate @@ -1151,7 +1156,7 @@ fn filter_backed_statements_from_disabled // are not enough backing votes after filtering we will remove the whole candidate. if bc.validity_votes.len() < effective_minimum_backing_votes( validator_group.len(), - configuration::Pallet::::config().minimum_backing_votes + minimum_backing_votes ) { return false From 8b5e5a8622f63fe930818b0f4592b5d621134238 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 28 Nov 2023 13:41:47 +0200 Subject: [PATCH 28/34] Apply suggestions from code review: remove duplicated assert Co-authored-by: Maciej --- polkadot/runtime/parachains/src/paras_inherent/tests.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/tests.rs b/polkadot/runtime/parachains/src/paras_inherent/tests.rs index 2afa4375e76c7..3319144ee8bea 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/tests.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/tests.rs @@ -1469,10 +1469,6 @@ mod sanitizers { backed_candidates.get(0).unwrap().validator_indices.get(0).unwrap(), true ); - assert_eq!( - backed_candidates.get(0).unwrap().validator_indices.get(0).unwrap(), - true - ); assert_eq!( backed_candidates.get(0).unwrap().validator_indices.get(1).unwrap(), true From a50800fdcb2a6521aa4399f4c49165e7645bdc7c Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 28 Nov 2023 13:46:40 +0200 Subject: [PATCH 29/34] Code review feedback - filter_backed_statements_from_disabled -> filter_backed_statements_from_disabled_validators --- polkadot/runtime/parachains/src/paras_inherent/mod.rs | 4 ++-- polkadot/runtime/parachains/src/paras_inherent/tests.rs | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/polkadot/runtime/parachains/src/paras_inherent/mod.rs b/polkadot/runtime/parachains/src/paras_inherent/mod.rs index 3b4ef20cf6139..7a4cb8ae31060 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/mod.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/mod.rs @@ -964,7 +964,7 @@ fn sanitize_backed_candidates< }); // Filter out backing statements from disabled validators - let dropped_disabled = filter_backed_statements_from_disabled::( + let dropped_disabled = filter_backed_statements_from_disabled_validators::( &mut backed_candidates, &allowed_relay_parents, scheduled, @@ -1070,7 +1070,7 @@ fn limit_and_sanitize_disputes< // Filters statements from disabled validators in `BackedCandidate`, non-scheduled candidates and // few more sanity checks. Returns `true` if at least one statement is removed and `false` // otherwise. -fn filter_backed_statements_from_disabled( +fn filter_backed_statements_from_disabled_validators( backed_candidates: &mut Vec::Hash>>, allowed_relay_parents: &AllowedRelayParentsTracker>, scheduled: &BTreeMap, diff --git a/polkadot/runtime/parachains/src/paras_inherent/tests.rs b/polkadot/runtime/parachains/src/paras_inherent/tests.rs index 3319144ee8bea..364c6192bebed 100644 --- a/polkadot/runtime/parachains/src/paras_inherent/tests.rs +++ b/polkadot/runtime/parachains/src/paras_inherent/tests.rs @@ -1439,7 +1439,7 @@ mod sanitizers { // Eve is disabled but no backing statement is signed by it so nothing should be // filtered - assert!(!filter_backed_statements_from_disabled::( + assert!(!filter_backed_statements_from_disabled_validators::( &mut backed_candidates, &>::allowed_relay_parents(), &scheduled_paras @@ -1475,7 +1475,7 @@ mod sanitizers { ); let untouched = backed_candidates.get(1).unwrap().clone(); - assert!(filter_backed_statements_from_disabled::( + assert!(filter_backed_statements_from_disabled_validators::( &mut backed_candidates, &>::allowed_relay_parents(), &scheduled_paras @@ -1511,7 +1511,7 @@ mod sanitizers { assert_eq!(backed_candidates.get(0).unwrap().validity_votes.len(), 2); let untouched = backed_candidates.get(1).unwrap().clone(); - assert!(filter_backed_statements_from_disabled::( + assert!(filter_backed_statements_from_disabled_validators::( &mut backed_candidates, &>::allowed_relay_parents(), &scheduled_paras From 7a308cd2b97ad5ff4a5d3c01e05489897550af1a Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 28 Nov 2023 15:37:35 +0200 Subject: [PATCH 30/34] Update parainherent.md with some details about data sanitization --- .../src/runtime/parainherent.md | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/polkadot/roadmap/implementers-guide/src/runtime/parainherent.md b/polkadot/roadmap/implementers-guide/src/runtime/parainherent.md index 4a771f1df6441..4c181425e4bef 100644 --- a/polkadot/roadmap/implementers-guide/src/runtime/parainherent.md +++ b/polkadot/roadmap/implementers-guide/src/runtime/parainherent.md @@ -60,3 +60,32 @@ processing it, so the processed inherent data is simply dropped. This also means that the `enter` function keeps data around for no good reason. This seems acceptable though as the size of a block is rather limited. Nevertheless if we ever wanted to optimize this we can easily implement an inherent collector that has two implementations, where one clones and stores the data and the other just passes it on. + +## Data sanitization +`ParasInherent` performs sanitization on the provided input data. When the module is invoked via `create_inherent` the +input data is sanitized. `enter` entry point requires sanitized input. If unsanitized data is provided the module +generates an error. + +Disputes are included in the block with a priority for a security reasons. It's important to pump as many dispute votes +onchain as possible so that disputes conclude faster and the offenders are punished. However if there are too many +disputes to include in a block the dispute set is trimmed so that it respects max block weight. + +Dispute data is first deduplicated and sorted by block number (older first) and dispute location (local then remote). +Concluded and ancient (disputes initiated before the post conclusion acceptance period ) disputes are filtered out. +Votes with invalid signatures and from unknown validators (not found in the active set for the current session) are also +filtered out. + +All dispute statements are included included in the order described in the previous paragraph until the available block +weight is exhausted. After the dispute data is included all remaining weight is filled in with candidates and +availability bitfields. Bitfields are included with priority, then candidates containing code updates and finaly any +backed candidates. If there is not enough weight for all backed candidates they are trimmed by random selection. +Disputes are processed in three separate functions - `deduplicate_and_sort_dispute_data`, `filter_dispute_data` and +`limit_and_sanitize_disputes`. + +Availability bitfields are also sanitized by dropping malformed ones, containing disputed cores or bad signatures. Refer +to `sanitize_bitfields` function for implementation details. + +Backed candidates sanitization removes malformed ones, candidates which has got concluded invalid disputes against them +or candidates produced by unassigned cores. Furthermore any backing votes from disabled validators for a candidate are +dropped. This is part of the validator disabling strategy. These checks are implemented in `sanitize_backed_candidates` +and `filter_backed_statements_from_disabled_validators`. \ No newline at end of file From 7ccb9427981537c557dd382b3bdcdbbbc3a2acc2 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 28 Nov 2023 17:45:45 +0200 Subject: [PATCH 31/34] Add newline to parainherent.md --- polkadot/roadmap/implementers-guide/src/runtime/parainherent.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/roadmap/implementers-guide/src/runtime/parainherent.md b/polkadot/roadmap/implementers-guide/src/runtime/parainherent.md index 4c181425e4bef..84a78a6026563 100644 --- a/polkadot/roadmap/implementers-guide/src/runtime/parainherent.md +++ b/polkadot/roadmap/implementers-guide/src/runtime/parainherent.md @@ -88,4 +88,4 @@ to `sanitize_bitfields` function for implementation details. Backed candidates sanitization removes malformed ones, candidates which has got concluded invalid disputes against them or candidates produced by unassigned cores. Furthermore any backing votes from disabled validators for a candidate are dropped. This is part of the validator disabling strategy. These checks are implemented in `sanitize_backed_candidates` -and `filter_backed_statements_from_disabled_validators`. \ No newline at end of file +and `filter_backed_statements_from_disabled_validators`. From 1834e20a2e2c923e64238b8fb1eea5484fe75b85 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Tue, 28 Nov 2023 19:08:58 +0200 Subject: [PATCH 32/34] Apply suggestions from code review Co-authored-by: Maciej --- .../implementers-guide/src/runtime/parainherent.md | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/polkadot/roadmap/implementers-guide/src/runtime/parainherent.md b/polkadot/roadmap/implementers-guide/src/runtime/parainherent.md index 84a78a6026563..520e16e9494ec 100644 --- a/polkadot/roadmap/implementers-guide/src/runtime/parainherent.md +++ b/polkadot/roadmap/implementers-guide/src/runtime/parainherent.md @@ -62,11 +62,9 @@ of a block is rather limited. Nevertheless if we ever wanted to optimize this we collector that has two implementations, where one clones and stores the data and the other just passes it on. ## Data sanitization -`ParasInherent` performs sanitization on the provided input data. When the module is invoked via `create_inherent` the -input data is sanitized. `enter` entry point requires sanitized input. If unsanitized data is provided the module -generates an error. +`ParasInherent` with the entry point of `create_inherent` sanitizes the input data, while the `enter` entry point enforces already sanitized input data. If unsanitized data is provided the module generates an error. -Disputes are included in the block with a priority for a security reasons. It's important to pump as many dispute votes +Disputes are included in the block with a priority for a security reasons. It's important to include as many dispute votes onchain as possible so that disputes conclude faster and the offenders are punished. However if there are too many disputes to include in a block the dispute set is trimmed so that it respects max block weight. @@ -77,7 +75,7 @@ filtered out. All dispute statements are included included in the order described in the previous paragraph until the available block weight is exhausted. After the dispute data is included all remaining weight is filled in with candidates and -availability bitfields. Bitfields are included with priority, then candidates containing code updates and finaly any +availability bitfields. Bitfields are included with priority, then candidates containing code updates and finally any backed candidates. If there is not enough weight for all backed candidates they are trimmed by random selection. Disputes are processed in three separate functions - `deduplicate_and_sort_dispute_data`, `filter_dispute_data` and `limit_and_sanitize_disputes`. @@ -85,7 +83,7 @@ Disputes are processed in three separate functions - `deduplicate_and_sort_dispu Availability bitfields are also sanitized by dropping malformed ones, containing disputed cores or bad signatures. Refer to `sanitize_bitfields` function for implementation details. -Backed candidates sanitization removes malformed ones, candidates which has got concluded invalid disputes against them +Backed candidates sanitization removes malformed ones, candidates which have got concluded invalid disputes against them or candidates produced by unassigned cores. Furthermore any backing votes from disabled validators for a candidate are dropped. This is part of the validator disabling strategy. These checks are implemented in `sanitize_backed_candidates` and `filter_backed_statements_from_disabled_validators`. From 610b45db9a613ae088c93cbbf171ce205ed4bc85 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 29 Nov 2023 09:21:05 +0200 Subject: [PATCH 33/34] Apply suggestions from code review Co-authored-by: Maciej --- polkadot/roadmap/implementers-guide/src/runtime/parainherent.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/roadmap/implementers-guide/src/runtime/parainherent.md b/polkadot/roadmap/implementers-guide/src/runtime/parainherent.md index 520e16e9494ec..7c4e357450a4e 100644 --- a/polkadot/roadmap/implementers-guide/src/runtime/parainherent.md +++ b/polkadot/roadmap/implementers-guide/src/runtime/parainherent.md @@ -73,7 +73,7 @@ Concluded and ancient (disputes initiated before the post conclusion acceptance Votes with invalid signatures and from unknown validators (not found in the active set for the current session) are also filtered out. -All dispute statements are included included in the order described in the previous paragraph until the available block +All dispute statements are included in the order described in the previous paragraph until the available block weight is exhausted. After the dispute data is included all remaining weight is filled in with candidates and availability bitfields. Bitfields are included with priority, then candidates containing code updates and finally any backed candidates. If there is not enough weight for all backed candidates they are trimmed by random selection. From 83adc9332a298c42e63fd64d910a6c6e70385e40 Mon Sep 17 00:00:00 2001 From: Tsvetomir Dimitrov Date: Wed, 29 Nov 2023 09:35:04 +0200 Subject: [PATCH 34/34] Fixes in parainherent.md --- .../src/runtime/parainherent.md | 28 +++++++++++-------- 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/polkadot/roadmap/implementers-guide/src/runtime/parainherent.md b/polkadot/roadmap/implementers-guide/src/runtime/parainherent.md index 7c4e357450a4e..9489a286a0cbf 100644 --- a/polkadot/roadmap/implementers-guide/src/runtime/parainherent.md +++ b/polkadot/roadmap/implementers-guide/src/runtime/parainherent.md @@ -62,22 +62,23 @@ of a block is rather limited. Nevertheless if we ever wanted to optimize this we collector that has two implementations, where one clones and stores the data and the other just passes it on. ## Data sanitization -`ParasInherent` with the entry point of `create_inherent` sanitizes the input data, while the `enter` entry point enforces already sanitized input data. If unsanitized data is provided the module generates an error. +`ParasInherent` with the entry point of `create_inherent` sanitizes the input data, while the `enter` entry point +enforces already sanitized input data. If unsanitized data is provided the module generates an error. -Disputes are included in the block with a priority for a security reasons. It's important to include as many dispute votes -onchain as possible so that disputes conclude faster and the offenders are punished. However if there are too many +Disputes are included in the block with a priority for a security reasons. It's important to include as many dispute +votes onchain as possible so that disputes conclude faster and the offenders are punished. However if there are too many disputes to include in a block the dispute set is trimmed so that it respects max block weight. Dispute data is first deduplicated and sorted by block number (older first) and dispute location (local then remote). -Concluded and ancient (disputes initiated before the post conclusion acceptance period ) disputes are filtered out. -Votes with invalid signatures and from unknown validators (not found in the active set for the current session) are also +Concluded and ancient (disputes initiated before the post conclusion acceptance period) disputes are filtered out. +Votes with invalid signatures or from unknown validators (not found in the active set for the current session) are also filtered out. -All dispute statements are included in the order described in the previous paragraph until the available block -weight is exhausted. After the dispute data is included all remaining weight is filled in with candidates and -availability bitfields. Bitfields are included with priority, then candidates containing code updates and finally any -backed candidates. If there is not enough weight for all backed candidates they are trimmed by random selection. -Disputes are processed in three separate functions - `deduplicate_and_sort_dispute_data`, `filter_dispute_data` and +All dispute statements are included in the order described in the previous paragraph until the available block weight is +exhausted. After the dispute data is included all remaining weight is filled in with candidates and availability +bitfields. Bitfields are included with priority, then candidates containing code updates and finally any backed +candidates. If there is not enough weight for all backed candidates they are trimmed by random selection. Disputes are +processed in three separate functions - `deduplicate_and_sort_dispute_data`, `filter_dispute_data` and `limit_and_sanitize_disputes`. Availability bitfields are also sanitized by dropping malformed ones, containing disputed cores or bad signatures. Refer @@ -85,5 +86,8 @@ to `sanitize_bitfields` function for implementation details. Backed candidates sanitization removes malformed ones, candidates which have got concluded invalid disputes against them or candidates produced by unassigned cores. Furthermore any backing votes from disabled validators for a candidate are -dropped. This is part of the validator disabling strategy. These checks are implemented in `sanitize_backed_candidates` -and `filter_backed_statements_from_disabled_validators`. +dropped. This is part of the validator disabling strategy. After filtering the statements from disabled validators a +backed candidate may end up with votes count less than `minimum_backing_votes` (a parameter from `HostConfiguiration`). +In this case the whole candidate is dropped otherwise it will be rejected by `process_candidates` from pallet inclusion. +All checks related to backed candidates are implemented in `sanitize_backed_candidates` and +`filter_backed_statements_from_disabled_validators`.