diff --git a/roadmap/implementers-guide/src/runtime/inclusion.md b/roadmap/implementers-guide/src/runtime/inclusion.md index 84b513cb985c..d332333e41e0 100644 --- a/roadmap/implementers-guide/src/runtime/inclusion.md +++ b/roadmap/implementers-guide/src/runtime/inclusion.md @@ -51,13 +51,14 @@ All failed checks should lead to an unrecoverable error making the block invalid 1. For each applied bit of each availability-bitfield, set the bit for the validator in the `CandidatePendingAvailability`'s `availability_votes` bitfield. Track all candidates that now have >2/3 of bits set in their `availability_votes`. These candidates are now available and can be enacted. 1. For all now-available candidates, invoke the `enact_candidate` routine with the candidate and relay-parent number. 1. Return a list of `(CoreIndex, CandidateHash)` from freed cores consisting of the cores where candidates have become available. -* `sanitize_bitfields( +* `sanitize_bitfields( unchecked_bitfields: UncheckedSignedAvailabilityBitfields, disputed_bitfield: DisputedBitfield, expected_bits: usize, parent_hash: T::Hash, session_index: SessionIndex, validators: &[ValidatorId], + full_check: FullCheck, )`: 1. check that `disputed_bitfield` has the same number of bits as the `expected_bits`, iff not return early with an empty vec. 1. each of the below checks is for each bitfield. If a check does not pass the bitfield will be skipped. @@ -65,7 +66,7 @@ All failed checks should lead to an unrecoverable error making the block invalid 1. check that the number of bits is equal to `expected_bits`. 1. check that the validator index is strictly increasing (and thus also unique). 1. check that the validator bit index is not out of bounds. - 1. check the validators signature, iff `CHECK_SIGS=true`. + 1. check the validators signature, iff `full_check=FullCheck::Yes`. * `sanitize_backed_candidates bool>( relay_parent: T::Hash, diff --git a/runtime/parachains/src/inclusion.rs b/runtime/parachains/src/inclusion.rs index e1a5e0efc258..23ae25ab1449 100644 --- a/runtime/parachains/src/inclusion.rs +++ b/runtime/parachains/src/inclusion.rs @@ -22,7 +22,7 @@ use crate::{ configuration, disputes, dmp, hrmp, paras, - paras_inherent::{sanitize_bitfields, DisputedBitfield, VERIFY_SIGS}, + paras_inherent::{sanitize_bitfields, DisputedBitfield}, scheduler::CoreAssignment, shared, ump, }; @@ -56,6 +56,19 @@ pub struct AvailabilityBitfieldRecord { submitted_at: N, // for accounting, as meaning of bits may change over time. } +/// Determines if all checks should be applied or if a subset was already completed +/// in a code path that will be executed afterwards or was already executed before. +#[derive(Encode, Decode, PartialEq, Eq, RuntimeDebug, TypeInfo)] +pub(crate) enum FullCheck { + /// Yes, do a full check, skip nothing. + Yes, + /// Skip a subset of checks that are already completed before. + /// + /// Attention: Should only be used when absolutely sure that the required + /// checks are completed before. + Skip, +} + /// A backed candidate pending availability. #[derive(Encode, Decode, PartialEq, TypeInfo)] #[cfg_attr(test, derive(Debug, Default))] @@ -403,13 +416,14 @@ impl Pallet { let session_index = shared::Pallet::::session_index(); let parent_hash = frame_system::Pallet::::parent_hash(); - let checked_bitfields = sanitize_bitfields::( + let checked_bitfields = sanitize_bitfields::( signed_bitfields, disputed_bitfield, expected_bits, parent_hash, session_index, &validators[..], + FullCheck::Yes, ); let freed_cores = Self::update_pending_availability_and_get_freed_cores::<_, true>( @@ -427,12 +441,16 @@ impl Pallet { /// /// Both should be sorted ascending by core index, and the candidates should be a subset of /// scheduled cores. If these conditions are not met, the execution of the function fails. - pub(crate) fn process_candidates( + pub(crate) fn process_candidates( parent_storage_root: T::Hash, candidates: Vec>, scheduled: Vec, - group_validators: impl Fn(GroupIndex) -> Option>, - ) -> Result, DispatchError> { + group_validators: GV, + full_check: FullCheck, + ) -> Result, DispatchError> + where + GV: Fn(GroupIndex) -> Option>, + { ensure!(candidates.len() <= scheduled.len(), Error::::UnscheduledCandidate); if scheduled.is_empty() { @@ -446,7 +464,7 @@ impl Pallet { // before of the block where we include a candidate (i.e. this code path). let now = >::block_number(); let relay_parent_number = now - One::one(); - let check_cx = CandidateCheckContext::::new(now, relay_parent_number); + let check_ctx = CandidateCheckContext::::new(now, relay_parent_number); // Collect candidate receipts with backers. let mut candidate_receipt_with_backing_validator_indices = @@ -481,54 +499,20 @@ impl Pallet { // // In the meantime, we do certain sanity checks on the candidates and on the scheduled // list. - 'a: for (candidate_idx, backed_candidate) in candidates.iter().enumerate() { + 'next_backed_candidate: for (candidate_idx, backed_candidate) in + candidates.iter().enumerate() + { + if let FullCheck::Yes = full_check { + check_ctx.verify_backed_candidate( + parent_hash, + candidate_idx, + backed_candidate, + )?; + } + let para_id = backed_candidate.descriptor().para_id; let mut backers = bitvec::bitvec![BitOrderLsb0, u8; 0; validators.len()]; - // we require that the candidate is in the context of the parent block. - ensure!( - backed_candidate.descriptor().relay_parent == parent_hash, - Error::::CandidateNotInParentContext, - ); - ensure!( - backed_candidate.descriptor().check_collator_signature().is_ok(), - Error::::NotCollatorSigned, - ); - - let validation_code_hash = - >::validation_code_hash_at(para_id, now, None) - // A candidate for a parachain without current validation code is not scheduled. - .ok_or_else(|| Error::::UnscheduledCandidate)?; - ensure!( - backed_candidate.descriptor().validation_code_hash == validation_code_hash, - Error::::InvalidValidationCodeHash, - ); - - ensure!( - backed_candidate.descriptor().para_head == - backed_candidate.candidate.commitments.head_data.hash(), - Error::::ParaHeadMismatch, - ); - - if let Err(err) = check_cx.check_validation_outputs( - para_id, - &backed_candidate.candidate.commitments.head_data, - &backed_candidate.candidate.commitments.new_validation_code, - backed_candidate.candidate.commitments.processed_downward_messages, - &backed_candidate.candidate.commitments.upward_messages, - T::BlockNumber::from(backed_candidate.candidate.commitments.hrmp_watermark), - &backed_candidate.candidate.commitments.horizontal_messages, - ) { - log::debug!( - target: LOG_TARGET, - "Validation outputs checking during inclusion of a candidate {} for parachain `{}` failed: {:?}", - candidate_idx, - u32::from(para_id), - err, - ); - Err(err.strip_into_dispatch_err::())?; - }; - for (i, assignment) in scheduled[skip..].iter().enumerate() { check_assignment_in_order(assignment)?; @@ -631,7 +615,7 @@ impl Pallet { backers, assignment.group_idx, )); - continue 'a + continue 'next_backed_candidate } } @@ -682,7 +666,7 @@ impl Pallet { availability_votes, relay_parent_number, backers: backers.to_bitvec(), - backed_in_number: check_cx.now, + backed_in_number: check_ctx.now, backing_group: group, }, ); @@ -704,9 +688,9 @@ impl Pallet { // `relay_parent_number` is equal to `now`. let now = >::block_number(); let relay_parent_number = now; - let check_cx = CandidateCheckContext::::new(now, relay_parent_number); + let check_ctx = CandidateCheckContext::::new(now, relay_parent_number); - if let Err(err) = check_cx.check_validation_outputs( + if let Err(err) = check_ctx.check_validation_outputs( para_id, &validation_outputs.head_data, &validation_outputs.new_validation_code, @@ -941,17 +925,78 @@ impl AcceptanceCheckErr { } /// A collection of data required for checking a candidate. -struct CandidateCheckContext { +pub(crate) struct CandidateCheckContext { config: configuration::HostConfiguration, now: T::BlockNumber, relay_parent_number: T::BlockNumber, } impl CandidateCheckContext { - fn new(now: T::BlockNumber, relay_parent_number: T::BlockNumber) -> Self { + pub(crate) fn new(now: T::BlockNumber, relay_parent_number: T::BlockNumber) -> Self { Self { config: >::config(), now, relay_parent_number } } + /// Execute verification of the candidate. + /// + /// Assures: + /// * correct expected relay parent reference + /// * collator signature check passes + /// * code hash of commitments matches current code hash + /// * para head in the descriptor and commitments match + pub(crate) fn verify_backed_candidate( + &self, + parent_hash: ::Hash, + candidate_idx: usize, + backed_candidate: &BackedCandidate<::Hash>, + ) -> Result<(), Error> { + let para_id = backed_candidate.descriptor().para_id; + let now = self.now; + + // we require that the candidate is in the context of the parent block. + ensure!( + backed_candidate.descriptor().relay_parent == parent_hash, + Error::::CandidateNotInParentContext, + ); + ensure!( + backed_candidate.descriptor().check_collator_signature().is_ok(), + Error::::NotCollatorSigned, + ); + + let validation_code_hash = >::validation_code_hash_at(para_id, now, None) + // A candidate for a parachain without current validation code is not scheduled. + .ok_or_else(|| Error::::UnscheduledCandidate)?; + ensure!( + backed_candidate.descriptor().validation_code_hash == validation_code_hash, + Error::::InvalidValidationCodeHash, + ); + + ensure!( + backed_candidate.descriptor().para_head == + backed_candidate.candidate.commitments.head_data.hash(), + Error::::ParaHeadMismatch, + ); + + if let Err(err) = self.check_validation_outputs( + para_id, + &backed_candidate.candidate.commitments.head_data, + &backed_candidate.candidate.commitments.new_validation_code, + backed_candidate.candidate.commitments.processed_downward_messages, + &backed_candidate.candidate.commitments.upward_messages, + T::BlockNumber::from(backed_candidate.candidate.commitments.hrmp_watermark), + &backed_candidate.candidate.commitments.horizontal_messages, + ) { + log::debug!( + target: LOG_TARGET, + "Validation outputs checking during inclusion of a candidate {} for parachain `{}` failed: {:?}", + candidate_idx, + u32::from(para_id), + err, + ); + Err(err.strip_into_dispatch_err::())?; + }; + Ok(()) + } + /// Check the given outputs after candidate validation on whether it passes the acceptance /// criteria. fn check_validation_outputs( @@ -1935,6 +1980,7 @@ pub(crate) mod tests { vec![backed], vec![chain_b_assignment.clone()], &group_validators, + FullCheck::Yes, ), Error::::UnscheduledCandidate ); @@ -1990,6 +2036,7 @@ pub(crate) mod tests { vec![backed_b, backed_a], vec![chain_a_assignment.clone(), chain_b_assignment.clone()], &group_validators, + FullCheck::Yes, ), Error::::UnscheduledCandidate ); @@ -2023,6 +2070,7 @@ pub(crate) mod tests { vec![backed], vec![chain_a_assignment.clone()], &group_validators, + FullCheck::Yes, ), Error::::InsufficientBacking ); @@ -2058,6 +2106,7 @@ pub(crate) mod tests { vec![backed], vec![chain_a_assignment.clone()], &group_validators, + FullCheck::Yes, ), Error::::CandidateNotInParentContext ); @@ -2097,6 +2146,7 @@ pub(crate) mod tests { thread_a_assignment.clone(), ], &group_validators, + FullCheck::Yes, ), Error::::WrongCollator, ); @@ -2135,6 +2185,7 @@ pub(crate) mod tests { vec![backed], vec![thread_a_assignment.clone()], &group_validators, + FullCheck::Yes, ), Error::::NotCollatorSigned ); @@ -2185,6 +2236,7 @@ pub(crate) mod tests { vec![backed], vec![chain_a_assignment.clone()], &group_validators, + FullCheck::Yes, ), Error::::CandidateScheduledBeforeParaFree ); @@ -2228,6 +2280,7 @@ pub(crate) mod tests { vec![backed], vec![chain_a_assignment.clone()], &group_validators, + FullCheck::Yes, ), Error::::CandidateScheduledBeforeParaFree ); @@ -2279,6 +2332,7 @@ pub(crate) mod tests { vec![backed], vec![chain_a_assignment.clone()], &group_validators, + FullCheck::Yes, ), Error::::PrematureCodeUpgrade ); @@ -2313,6 +2367,7 @@ pub(crate) mod tests { vec![backed], vec![chain_a_assignment.clone()], &group_validators, + FullCheck::Yes, ), Err(Error::::ValidationDataHashMismatch.into()), ); @@ -2348,6 +2403,7 @@ pub(crate) mod tests { vec![backed], vec![chain_a_assignment.clone()], &group_validators, + FullCheck::Yes, ), Error::::InvalidValidationCodeHash ); @@ -2383,6 +2439,7 @@ pub(crate) mod tests { vec![backed], vec![chain_a_assignment.clone()], &group_validators, + FullCheck::Yes, ), Error::::ParaHeadMismatch ); @@ -2552,6 +2609,7 @@ pub(crate) mod tests { thread_a_assignment.clone(), ], &group_validators, + FullCheck::Yes, ) .expect("candidates scheduled, in order, and backed"); @@ -2742,6 +2800,7 @@ pub(crate) mod tests { vec![backed_a], vec![chain_a_assignment.clone()], &group_validators, + FullCheck::Yes, ) .expect("candidates scheduled, in order, and backed"); diff --git a/runtime/parachains/src/paras_inherent.rs b/runtime/parachains/src/paras_inherent.rs index a501bb4676db..94f0e5bca643 100644 --- a/runtime/parachains/src/paras_inherent.rs +++ b/runtime/parachains/src/paras_inherent.rs @@ -23,7 +23,9 @@ use crate::{ disputes::DisputesHandler, - inclusion, initializer, + inclusion, + inclusion::{CandidateCheckContext, FullCheck}, + initializer, scheduler::{self, CoreAssignment, FreedReason}, shared, ump, }; @@ -42,21 +44,21 @@ use primitives::v1::{ UncheckedSignedAvailabilityBitfields, ValidatorId, ValidatorIndex, PARACHAINS_INHERENT_IDENTIFIER, }; -use rand::{Rng, SeedableRng}; +use rand::{seq::SliceRandom, SeedableRng}; + use scale_info::TypeInfo; -use sp_runtime::traits::Header as HeaderT; +use sp_runtime::traits::{Header as HeaderT, One}; use sp_std::{ cmp::Ordering, collections::{btree_map::BTreeMap, btree_set::BTreeSet}, prelude::*, vec::Vec, }; + #[cfg(feature = "runtime-benchmarks")] mod benchmarking; const LOG_TARGET: &str = "runtime::inclusion-inherent"; -const SKIP_SIG_VERIFY: bool = false; -pub(crate) const VERIFY_SIGS: bool = true; pub trait WeightInfo { /// Variant over `v`, the count of dispute statements in a dispute statement set. This gives the @@ -158,6 +160,29 @@ fn backed_candidates_weight( .fold(0, |acc, x| acc.saturating_add(x)) } +/// A helper trait to allow calling retain while getting access +/// to the index of the item in the `vec`. +trait IndexedRetain { + /// Retains only the elements specified by the predicate. + /// + /// In other words, remove all elements `e` residing at + /// index `i` such that `f(i, &e)` returns `false`. This method + /// operates in place, visiting each element exactly once in the + /// original order, and preserves the order of the retained elements. + fn indexed_retain(&mut self, f: impl FnMut(usize, &T) -> bool); +} + +impl IndexedRetain for Vec { + fn indexed_retain(&mut self, mut f: impl FnMut(usize, &T) -> bool) { + let mut idx = 0_usize; + self.retain(move |item| { + let ret = f(idx, item); + idx += 1_usize; + ret + }) + } +} + /// A bitfield concerning concluded disputes for candidates /// associated to the core index equivalent to the bit position. #[derive(Default, PartialEq, Eq, Clone, Encode, Decode, RuntimeDebug, TypeInfo)] @@ -252,25 +277,24 @@ pub mod pallet { // (`enter`) and the off-chain checks by the block author (this function). Once we are confident // in all the logic in this module this check should be removed to optimize performance. - let inherent_data = - match Self::enter(frame_system::RawOrigin::None.into(), inherent_data.clone()) { - Ok(_) => inherent_data, - Err(err) => { - log::error!( - target: LOG_TARGET, - "dropping paras inherent data because they produced \ + let inherent_data = match Self::enter_inner(inherent_data.clone(), FullCheck::Skip) { + Ok(_) => inherent_data, + Err(err) => { + log::error!( + target: LOG_TARGET, + "dropping paras inherent data because they produced \ an invalid paras inherent: {:?}", - err.error, - ); + err.error, + ); - ParachainsInherentData { - bitfields: Vec::new(), - backed_candidates: Vec::new(), - disputes: Vec::new(), - parent_header: inherent_data.parent_header, - } - }, - }; + ParachainsInherentData { + bitfields: Vec::new(), + backed_candidates: Vec::new(), + disputes: Vec::new(), + parent_header: inherent_data.parent_header, + } + }, + }; Some(Call::enter { data: inherent_data }) } @@ -329,172 +353,192 @@ pub mod pallet { ensure!(!Included::::exists(), Error::::TooManyInclusionInherents); Included::::set(Some(())); - let ParachainsInherentData { - bitfields: mut signed_bitfields, - mut backed_candidates, - parent_header, - mut disputes, - } = data; - - // Check that the submitted parent header indeed corresponds to the previous block hash. - let parent_hash = >::parent_hash(); - ensure!( - parent_header.hash().as_ref() == parent_hash.as_ref(), - Error::::InvalidParentHeader, - ); + Self::enter_inner(data, FullCheck::Yes) + } + } +} - let mut candidate_weight = backed_candidates_weight::(&backed_candidates); - let mut bitfields_weight = signed_bitfields_weight::(signed_bitfields.len()); - let disputes_weight = dispute_statements_weight::(&disputes); +impl Pallet { + pub(crate) fn enter_inner( + data: ParachainsInherentData, + full_check: FullCheck, + ) -> DispatchResultWithPostInfo { + let ParachainsInherentData { + bitfields: mut signed_bitfields, + mut backed_candidates, + parent_header, + mut disputes, + } = data; + + log::debug!( + target: LOG_TARGET, + "[enter] bitfields.len(): {}, backed_candidates.len(): {}, disputes.len() {}", + signed_bitfields.len(), + backed_candidates.len(), + disputes.len() + ); - let max_block_weight = ::BlockWeights::get().max_block; + // Check that the submitted parent header indeed corresponds to the previous block hash. + let parent_hash = >::parent_hash(); + ensure!( + parent_header.hash().as_ref() == parent_hash.as_ref(), + Error::::InvalidParentHeader, + ); - // Potentially trim inherent data to ensure processing will be within weight limits - let total_weight = { - if candidate_weight - .saturating_add(bitfields_weight) - .saturating_add(disputes_weight) > - max_block_weight - { - // if the total weight is over the max block weight, first try clearing backed - // candidates and bitfields. - backed_candidates.clear(); - candidate_weight = 0; - signed_bitfields.clear(); - bitfields_weight = 0; - } + let now = >::block_number(); - if disputes_weight > max_block_weight { - // if disputes are by themselves overweight already, trim the disputes. - debug_assert!(candidate_weight == 0 && bitfields_weight == 0); + let mut candidate_weight = backed_candidates_weight::(&backed_candidates); + let mut bitfields_weight = signed_bitfields_weight::(signed_bitfields.len()); + let disputes_weight = dispute_statements_weight::(&disputes); - let entropy = compute_entropy::(parent_hash); - let mut rng = rand_chacha::ChaChaRng::from_seed(entropy.into()); + let max_block_weight = ::BlockWeights::get().max_block; - let remaining_weight = - limit_disputes::(&mut disputes, max_block_weight, &mut rng); - max_block_weight.saturating_sub(remaining_weight) - } else { - candidate_weight - .saturating_add(bitfields_weight) - .saturating_add(disputes_weight) - } - }; + // Potentially trim inherent data to ensure processing will be within weight limits + let total_weight = { + if candidate_weight + .saturating_add(bitfields_weight) + .saturating_add(disputes_weight) > + max_block_weight + { + // if the total weight is over the max block weight, first try clearing backed + // candidates and bitfields. + backed_candidates.clear(); + candidate_weight = 0; + signed_bitfields.clear(); + bitfields_weight = 0; + } - let expected_bits = >::availability_cores().len(); + if disputes_weight > max_block_weight { + // if disputes are by themselves overweight already, trim the disputes. + debug_assert!(candidate_weight == 0 && bitfields_weight == 0); - // Handle disputes logic. - let current_session = >::session_index(); - let disputed_bitfield = { - let new_current_dispute_sets: Vec<_> = disputes - .iter() - .filter(|s| s.session == current_session) - .map(|s| (s.session, s.candidate_hash)) - .collect(); - - // Note that `provide_multi_dispute_data` will iterate, verify, and import each - // dispute; so the input here must be reasonably bounded. - let _ = T::DisputesHandler::provide_multi_dispute_data(disputes.clone())?; - if T::DisputesHandler::is_frozen() { - // The relay chain we are currently on is invalid. Proceed no further on parachains. - return Ok(Some(dispute_statements_weight::(&disputes)).into()) - } + let entropy = compute_entropy::(parent_hash); + let mut rng = rand_chacha::ChaChaRng::from_seed(entropy.into()); - let mut freed_disputed = if !new_current_dispute_sets.is_empty() { - let concluded_invalid_disputes = new_current_dispute_sets - .iter() - .filter(|(session, candidate)| { - T::DisputesHandler::concluded_invalid(*session, *candidate) - }) - .map(|(_, candidate)| *candidate) - .collect::>(); - - let freed_disputed = - >::collect_disputed(&concluded_invalid_disputes) - .into_iter() - .map(|core| (core, FreedReason::Concluded)) - .collect(); - freed_disputed - } else { - Vec::new() - }; + let remaining_weight = + limit_disputes::(&mut disputes, max_block_weight, &mut rng); + max_block_weight.saturating_sub(remaining_weight) + } else { + candidate_weight + .saturating_add(bitfields_weight) + .saturating_add(disputes_weight) + } + }; - // Create a bit index from the set of core indices where each index corresponds to - // a core index that was freed due to a dispute. - let disputed_bitfield = create_disputed_bitfield( - expected_bits, - freed_disputed.iter().map(|(core_index, _)| core_index), - ); + let expected_bits = >::availability_cores().len(); - if !freed_disputed.is_empty() { - // unstable sort is fine, because core indices are unique - // i.e. the same candidate can't occupy 2 cores at once. - freed_disputed.sort_unstable_by_key(|pair| pair.0); // sort by core index - >::free_cores(freed_disputed); - } + // Handle disputes logic. + let current_session = >::session_index(); + let disputed_bitfield = { + let new_current_dispute_sets: Vec<_> = disputes + .iter() + .filter(|s| s.session == current_session) + .map(|s| (s.session, s.candidate_hash)) + .collect(); + + // Note that `provide_multi_dispute_data` will iterate, verify, and import each + // dispute; so the input here must be reasonably bounded. + let _ = T::DisputesHandler::provide_multi_dispute_data(disputes.clone())?; + if T::DisputesHandler::is_frozen() { + // The relay chain we are currently on is invalid. Proceed no further on parachains. + return Ok(Some(dispute_statements_weight::(&disputes)).into()) + } - disputed_bitfield + let mut freed_disputed = if !new_current_dispute_sets.is_empty() { + let concluded_invalid_disputes = new_current_dispute_sets + .iter() + .filter(|(session, candidate)| { + T::DisputesHandler::concluded_invalid(*session, *candidate) + }) + .map(|(_, candidate)| *candidate) + .collect::>(); + + let freed_disputed = + >::collect_disputed(&concluded_invalid_disputes) + .into_iter() + .map(|core| (core, FreedReason::Concluded)) + .collect(); + freed_disputed + } else { + Vec::new() }; - // Process new availability bitfields, yielding any availability cores whose - // work has now concluded. - let freed_concluded = >::process_bitfields( + // Create a bit index from the set of core indices where each index corresponds to + // a core index that was freed due to a dispute. + let disputed_bitfield = create_disputed_bitfield( expected_bits, - signed_bitfields, - disputed_bitfield, - >::core_para, + freed_disputed.iter().map(|(core_index, _)| core_index), ); - // Inform the disputes module of all included candidates. - let now = >::block_number(); - for (_, candidate_hash) in &freed_concluded { - T::DisputesHandler::note_included(current_session, *candidate_hash, now); + if !freed_disputed.is_empty() { + // unstable sort is fine, because core indices are unique + // i.e. the same candidate can't occupy 2 cores at once. + freed_disputed.sort_unstable_by_key(|pair| pair.0); // sort by core index + >::free_cores(freed_disputed); } - let freed = collect_all_freed_cores::(freed_concluded.iter().cloned()); + disputed_bitfield + }; - >::clear(); - >::schedule(freed, now); + // Process new availability bitfields, yielding any availability cores whose + // work has now concluded. + let freed_concluded = >::process_bitfields( + expected_bits, + signed_bitfields, + disputed_bitfield, + >::core_para, + ); - let scheduled = >::scheduled(); - let backed_candidates = sanitize_backed_candidates::( - parent_hash, - backed_candidates, - move |candidate_hash: CandidateHash| -> bool { - ::DisputesHandler::concluded_invalid(current_session, candidate_hash) - }, - &scheduled[..], - ); + // Inform the disputes module of all included candidates. + for (_, candidate_hash) in &freed_concluded { + T::DisputesHandler::note_included(current_session, *candidate_hash, now); + } - // Process backed candidates according to scheduled cores. - let parent_storage_root = parent_header.state_root().clone(); - let inclusion::ProcessedCandidates::<::Hash> { - core_indices: occupied, - candidate_receipt_with_backing_validator_indices, - } = >::process_candidates( - parent_storage_root, - backed_candidates, - scheduled, - >::group_validators, - )?; - - // The number of disputes included in a block is - // limited by the weight as well as the number of candidate blocks. - OnChainVotes::::put(ScrapedOnChainVotes::<::Hash> { - session: current_session, - backing_validators_per_candidate: candidate_receipt_with_backing_validator_indices, - disputes, - }); + let freed = collect_all_freed_cores::(freed_concluded.iter().cloned()); - // Note which of the scheduled cores were actually occupied by a backed candidate. - >::occupied(&occupied); + >::clear(); + >::schedule(freed, now); - // Give some time slice to dispatch pending upward messages. - // this is max config.ump_service_total_weight - let _ump_weight = >::process_pending_upward_messages(); + let scheduled = >::scheduled(); + let backed_candidates = sanitize_backed_candidates::( + parent_hash, + backed_candidates, + move |_candidate_index: usize, backed_candidate: &BackedCandidate| -> bool { + ::DisputesHandler::concluded_invalid(current_session, backed_candidate.hash()) + // `fn process_candidates` does the verification checks + }, + &scheduled[..], + ); - Ok(Some(total_weight).into()) - } + // Process backed candidates according to scheduled cores. + let parent_storage_root = parent_header.state_root().clone(); + let inclusion::ProcessedCandidates::<::Hash> { + core_indices: occupied, + candidate_receipt_with_backing_validator_indices, + } = >::process_candidates( + parent_storage_root, + backed_candidates, + scheduled, + >::group_validators, + full_check, + )?; + + // The number of disputes included in a block is + // limited by the weight as well as the number of candidate blocks. + OnChainVotes::::put(ScrapedOnChainVotes::<::Hash> { + session: current_session, + backing_validators_per_candidate: candidate_receipt_with_backing_validator_indices, + disputes, + }); + + // Note which of the scheduled cores were actually occupied by a backed candidate. + >::occupied(&occupied); + + // Give some time slice to dispatch pending upward messages. + // this is max config.ump_service_total_weight + let _ump_weight = >::process_pending_upward_messages(); + + Ok(Some(total_weight).into()) } } @@ -532,7 +576,7 @@ impl Pallet { T::DisputesHandler::filter_multi_dispute_data(&mut disputes); - let (concluded_invalid_disputes, mut bitfields, scheduled) = + let (mut backed_candidates, mut bitfields) = frame_support::storage::with_transaction(|| { // we don't care about fresh or not disputes // this writes them to storage, so let's query it via those means @@ -547,9 +591,13 @@ impl Pallet { e }); - // current concluded invalid disputes, only including the current block's votes - // TODO why does this say "only including the current block's votes"? This can include - // remote disputes, right? + // Contains the disputes that are concluded in the current session only, + // since these are the only ones that are relevant for the occupied cores + // and lightens the load on `collect_disputed` significantly. + // Cores can't be occupied with candidates of the previous sessions, and only + // things with new votes can have just concluded. We only need to collect + // cores with disputes that conclude just now, because disputes that + // concluded longer ago have already had any corresponding cores cleaned up. let current_concluded_invalid_disputes = disputes .iter() .filter(|dss| dss.session == current_session) @@ -560,8 +608,8 @@ impl Pallet { .map(|(_session, candidate)| candidate) .collect::>(); - // all concluded invalid disputes, that are relevant for the set of candidates - // the inherent provided + // All concluded invalid disputes, that are relevant for the set of candidates + // the inherent provided. let concluded_invalid_disputes = backed_candidates .iter() .map(|backed_candidate| backed_candidate.hash()) @@ -588,13 +636,14 @@ impl Pallet { // The following 3 calls are equiv to a call to `process_bitfields` // but we can retain access to `bitfields`. - let bitfields = sanitize_bitfields::( + let bitfields = sanitize_bitfields::( bitfields, disputed_bitfield, expected_bits, parent_hash, current_session, &validator_public[..], + FullCheck::Skip, ); let freed_concluded = @@ -611,31 +660,45 @@ impl Pallet { let freed = collect_all_freed_cores::(freed_concluded.iter().cloned()); >::clear(); - >::schedule(freed, >::block_number()); + let now = >::block_number(); + >::schedule(freed, now); let scheduled = >::scheduled(); + let relay_parent_number = now - One::one(); + + let check_ctx = CandidateCheckContext::::new(now, relay_parent_number); + let backed_candidates = sanitize_backed_candidates::( + parent_hash, + backed_candidates, + move |candidate_idx: usize, + backed_candidate: &BackedCandidate<::Hash>| + -> bool { + // never include a concluded-invalid candidate + 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 + // backed candidates fine to pass on. + check_ctx + .verify_backed_candidate(parent_hash, candidate_idx, backed_candidate) + .is_err() + }, + &scheduled[..], + ); + frame_support::storage::TransactionOutcome::Rollback(( - // concluded disputes for backed candidates in this block - concluded_invalid_disputes, - // filtered bitfields, + // filtered backed candidates + backed_candidates, + // filtered bitfields bitfields, - // updated schedule - scheduled, )) }); - let mut backed_candidates = sanitize_backed_candidates::( - parent_hash, - backed_candidates, - move |candidate_hash: CandidateHash| -> bool { - concluded_invalid_disputes.contains(&candidate_hash) - }, - &scheduled[..], - ); - let entropy = compute_entropy::(parent_hash); let mut rng = rand_chacha::ChaChaRng::from_seed(entropy.into()); + + // Assure the maximum block weight is adhered. let max_block_weight = ::BlockWeights::get().max_block; let _consumed_weight = apply_weight_limit::( &mut backed_candidates, @@ -698,15 +761,11 @@ fn random_sel Weight>( let mut weight_acc = 0 as Weight; - while !preferred_indices.is_empty() { - // randomly pick an index from the preferred set - let pick = rng.gen_range(0..preferred_indices.len()); - // remove the index from the available set of preferred indices - let preferred_idx = preferred_indices.swap_remove(pick); - + preferred_indices.shuffle(rng); + for preferred_idx in preferred_indices { // preferred indices originate from outside if let Some(item) = selectables.get(preferred_idx) { - let updated = weight_acc + weight_fn(item); + let updated = weight_acc.saturating_add(weight_fn(item)); if updated > weight_limit { continue } @@ -715,14 +774,10 @@ fn random_sel Weight>( } } - while !indices.is_empty() { - // randomly pick an index - let pick = rng.gen_range(0..indices.len()); - // remove the index from the available set of indices - let idx = indices.swap_remove(pick); - + indices.shuffle(rng); + for idx in indices { let item = &selectables[idx]; - let updated = weight_acc + weight_fn(item); + let updated = weight_acc.saturating_add(weight_fn(item)); if updated > weight_limit { continue @@ -738,14 +793,18 @@ fn random_sel Weight>( (weight_acc, picked_indices) } -/// Considers an upper threshold that the candidates must not exceed. +/// Considers an upper threshold that the inherent data must not exceed. /// -/// If there is sufficient space, all bitfields and candidates will be included. +/// If there is sufficient space, all disputes, all bitfields and all candidates +/// will be included. /// -/// Otherwise tries to include all bitfields, and fills in the remaining weight with candidates. +/// Otherwise tries to include all disputes, and then tries to fill the remaining space with bitfields and then candidates. /// -/// If even the bitfields are too large to fit into the `max_weight` limit, bitfields are randomly -/// picked and _no_ candidates will be included. +/// The selection process is random. For candidates, there is an exception for code upgrades as they are preferred. +/// And for disputes, local and older disputes are preferred (see `limit_disputes`). +/// for backed candidates, since with a increasing number of parachains their chances of +/// inclusion become slim. All backed candidates are checked beforehands in `fn create_inherent_inner` +/// which guarantees sanity. fn apply_weight_limit( candidates: &mut Vec::Hash>>, bitfields: &mut UncheckedSignedAvailabilityBitfields, @@ -788,12 +847,7 @@ fn apply_weight_limit( |c| backed_candidate_weight::(c), remaining_weight, ); - let mut idx = 0_usize; - candidates.retain(|_backed_candidate| { - let exists = indices.binary_search(&idx).is_ok(); - idx += 1; - exists - }); + candidates.indexed_retain(|idx, _backed_candidate| indices.binary_search(&idx).is_ok()); // pick all bitfields, and // fill the remaining space with candidates let total = acc_candidate_weight.saturating_add(total_bitfields_weight); @@ -812,12 +866,7 @@ fn apply_weight_limit( remaining_weight, ); - let mut idx = 0_usize; - bitfields.retain(|_bitfield| { - let exists = indices.binary_search(&idx).is_ok(); - idx += 1; - exists - }); + bitfields.indexed_retain(|idx, _bitfield| indices.binary_search(&idx).is_ok()); total } @@ -838,15 +887,16 @@ fn apply_weight_limit( /// they were actually checked and filtered to allow using it in both /// cases, as `filtering` and `checking` stage. /// -/// `CHECK_SIGS` determines if validator signatures are checked. If true, bitfields that have an -/// invalid signature will be filtered out. -pub(crate) fn sanitize_bitfields( +/// `full_check` determines if validator signatures are checked. If `::Yes`, +/// bitfields that have an invalid signature will be filtered out. +pub(crate) fn sanitize_bitfields( unchecked_bitfields: UncheckedSignedAvailabilityBitfields, disputed_bitfield: DisputedBitfield, expected_bits: usize, parent_hash: T::Hash, session_index: SessionIndex, validators: &[ValidatorId], + full_check: FullCheck, ) -> UncheckedSignedAvailabilityBitfields { let mut bitfields = Vec::with_capacity(unchecked_bitfields.len()); @@ -866,8 +916,8 @@ pub(crate) fn sanitize_bitfields= validators.len() { log::trace!( target: LOG_TARGET, - "[CHECK_SIGS: {}] bitfield validator index is out of bounds: {} >= {}", - CHECK_SIGS, + "[{:?}] bitfield validator index is out of bounds: {} >= {}", + full_check, validator_index.0, validators.len(), ); @@ -912,7 +962,7 @@ pub(crate) fn sanitize_bitfields bool>( +fn sanitize_backed_candidates< + T: crate::inclusion::Config, + F: FnMut(usize, &BackedCandidate) -> bool, +>( relay_parent: T::Hash, mut backed_candidates: Vec>, - candidate_has_concluded_invalid_dispute: F, + mut candidate_has_concluded_invalid_dispute_or_is_invalid: F, scheduled: &[CoreAssignment], ) -> Vec> { // Remove any candidates that were concluded invalid. - backed_candidates.retain(|backed_candidate| { - !candidate_has_concluded_invalid_dispute(backed_candidate.candidate.hash()) + backed_candidates.indexed_retain(move |idx, backed_candidate| { + !candidate_has_concluded_invalid_dispute_or_is_invalid(idx, backed_candidate) }); // Assure the backed candidate's `ParaId`'s core is free. @@ -1641,7 +1694,7 @@ mod tests { #[test] // Ensure that when a block is over weight due to disputes and bitfields, we abort - fn limit_candidates_over_weight() { + fn limit_candidates_over_weight_1() { new_test_ext(MockGenesisConfig::default()).execute_with(|| { // Create the inherent data for this block let mut dispute_statements = BTreeMap::new(); @@ -1712,7 +1765,7 @@ mod tests { #[test] // Ensure that when a block is over weight due to disputes and bitfields, we abort - fn limit_candidates_over_weight_overweight() { + fn limit_candidates_over_weight_0() { new_test_ext(MockGenesisConfig::default()).execute_with(|| { // Create the inherent data for this block let mut dispute_statements = BTreeMap::new(); @@ -1855,24 +1908,26 @@ mod tests { { assert_eq!( - sanitize_bitfields::( + sanitize_bitfields::( unchecked_bitfields.clone(), disputed_bitfield.clone(), expected_bits, parent_hash, session_index, - &validator_public[..] + &validator_public[..], + FullCheck::Skip, ), unchecked_bitfields.clone() ); assert_eq!( - sanitize_bitfields::( + sanitize_bitfields::( unchecked_bitfields.clone(), disputed_bitfield.clone(), expected_bits, parent_hash, session_index, - &validator_public[..] + &validator_public[..], + FullCheck::Yes ), unchecked_bitfields.clone() ); @@ -1886,25 +1941,27 @@ mod tests { disputed_bitfield.0.set(0, true); assert_eq!( - sanitize_bitfields::( + sanitize_bitfields::( unchecked_bitfields.clone(), disputed_bitfield.clone(), expected_bits, parent_hash, session_index, - &validator_public[..] + &validator_public[..], + FullCheck::Yes ) .len(), 1 ); assert_eq!( - sanitize_bitfields::( + sanitize_bitfields::( unchecked_bitfields.clone(), disputed_bitfield.clone(), expected_bits, parent_hash, session_index, - &validator_public[..] + &validator_public[..], + FullCheck::Skip ) .len(), 1 @@ -1913,22 +1970,24 @@ mod tests { // bitfield size mismatch { - assert!(sanitize_bitfields::( + assert!(sanitize_bitfields::( unchecked_bitfields.clone(), disputed_bitfield.clone(), expected_bits + 1, parent_hash, session_index, - &validator_public[..] + &validator_public[..], + FullCheck::Yes ) .is_empty()); - assert!(sanitize_bitfields::( + assert!(sanitize_bitfields::( unchecked_bitfields.clone(), disputed_bitfield.clone(), expected_bits + 1, parent_hash, session_index, - &validator_public[..] + &validator_public[..], + FullCheck::Skip ) .is_empty()); } @@ -1937,24 +1996,26 @@ mod tests { { let shortened = validator_public.len() - 2; assert_eq!( - &sanitize_bitfields::( + &sanitize_bitfields::( unchecked_bitfields.clone(), disputed_bitfield.clone(), expected_bits, parent_hash, session_index, - &validator_public[..shortened] + &validator_public[..shortened], + FullCheck::Yes, )[..], &unchecked_bitfields[..shortened] ); assert_eq!( - &sanitize_bitfields::( + &sanitize_bitfields::( unchecked_bitfields.clone(), disputed_bitfield.clone(), expected_bits, parent_hash, session_index, - &validator_public[..shortened] + &validator_public[..shortened], + FullCheck::Skip, )[..], &unchecked_bitfields[..shortened] ); @@ -1966,24 +2027,26 @@ mod tests { let x = unchecked_bitfields.swap_remove(0); unchecked_bitfields.push(x); assert_eq!( - &sanitize_bitfields::( + &sanitize_bitfields::( unchecked_bitfields.clone(), disputed_bitfield.clone(), expected_bits, parent_hash, session_index, - &validator_public[..] + &validator_public[..], + FullCheck::Yes )[..], &unchecked_bitfields[..(unchecked_bitfields.len() - 2)] ); assert_eq!( - &sanitize_bitfields::( + &sanitize_bitfields::( unchecked_bitfields.clone(), disputed_bitfield.clone(), expected_bits, parent_hash, session_index, - &validator_public[..] + &validator_public[..], + FullCheck::Skip )[..], &unchecked_bitfields[..(unchecked_bitfields.len() - 2)] ); @@ -2001,24 +2064,26 @@ mod tests { .and_then(|u| Some(u.set_signature(ValidatorSignature::default()))) .expect("we are accessing a valid index"); assert_eq!( - &sanitize_bitfields::( + &sanitize_bitfields::( unchecked_bitfields.clone(), disputed_bitfield.clone(), expected_bits, parent_hash, session_index, - &validator_public[..] + &validator_public[..], + FullCheck::Yes )[..], &unchecked_bitfields[..last_bit_idx] ); assert_eq!( - &sanitize_bitfields::( + &sanitize_bitfields::( unchecked_bitfields.clone(), disputed_bitfield.clone(), expected_bits, parent_hash, session_index, - &validator_public[..] + &validator_public[..], + FullCheck::Skip )[..], &unchecked_bitfields[..] ); @@ -2052,7 +2117,8 @@ mod tests { .unwrap(); } - let has_concluded_invalid = |_candidate: CandidateHash| -> bool { false }; + let has_concluded_invalid = + |_idx: usize, _backed_candidate: &BackedCandidate| -> bool { false }; let scheduled = (0_usize..2) .into_iter() @@ -2152,7 +2218,8 @@ mod tests { } set }; - let has_concluded_invalid = |candidate: CandidateHash| set.contains(&candidate); + let has_concluded_invalid = + |_idx: usize, candidate: &BackedCandidate| set.contains(&candidate.hash()); assert_eq!( sanitize_backed_candidates::( relay_parent,