From ff4828f6a6c0eebd94551c61f75438c3e9bd1b48 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Mon, 22 Nov 2021 17:16:28 +0100 Subject: [PATCH 01/17] minor: move checks into separate fn --- runtime/parachains/src/inclusion.rs | 115 +++++++++++++++++----------- 1 file changed, 69 insertions(+), 46 deletions(-) diff --git a/runtime/parachains/src/inclusion.rs b/runtime/parachains/src/inclusion.rs index e1a5e0efc258..35ba3dea1d90 100644 --- a/runtime/parachains/src/inclusion.rs +++ b/runtime/parachains/src/inclusion.rs @@ -422,6 +422,67 @@ impl Pallet { freed_cores } + /// Execute verification of the candidate. + /// + /// Assures: + /// * correct expecte 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( + parent_hash: Hash, + now: ::BlockNumber, + check_ctx: &CandidateCheckContext, + backed_candidate: &BackedCandidate, + ) -> Result<(), Error> { + let para_id = backed_candidate.descriptor().para_id; + + // 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_ctx.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(()) + } + /// Process candidates that have been backed. Provide the relay storage root, a set of candidates /// and scheduled cores. /// @@ -446,7 +507,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 = @@ -485,52 +546,14 @@ impl Pallet { 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::())?; - }; + if let Err(_err) = verify_backed_candidate(parent_hash, check_ctx, &backed_candidate) { + continue; + } for (i, assignment) in scheduled[skip..].iter().enumerate() { - check_assignment_in_order(assignment)?; + if let Err(_err) = check_assignment_in_order(assignment) { + continue; + } if para_id == assignment.para_id { if let Some(required_collator) = assignment.required_collator() { @@ -682,7 +705,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, }, ); From d54fbcf9814cc64af7dd6e3e6380a6fedabb9b57 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Mon, 22 Nov 2021 18:49:28 +0100 Subject: [PATCH 02/17] add additional validity checks --- runtime/parachains/src/inclusion.rs | 144 +++++++++++------------ runtime/parachains/src/paras_inherent.rs | 53 +++++++-- 2 files changed, 111 insertions(+), 86 deletions(-) diff --git a/runtime/parachains/src/inclusion.rs b/runtime/parachains/src/inclusion.rs index 35ba3dea1d90..c981e8d43c3b 100644 --- a/runtime/parachains/src/inclusion.rs +++ b/runtime/parachains/src/inclusion.rs @@ -422,67 +422,6 @@ impl Pallet { freed_cores } - /// Execute verification of the candidate. - /// - /// Assures: - /// * correct expecte 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( - parent_hash: Hash, - now: ::BlockNumber, - check_ctx: &CandidateCheckContext, - backed_candidate: &BackedCandidate, - ) -> Result<(), Error> { - let para_id = backed_candidate.descriptor().para_id; - - // 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_ctx.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(()) - } - /// Process candidates that have been backed. Provide the relay storage root, a set of candidates /// and scheduled cores. /// @@ -542,18 +481,16 @@ 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() + { + 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()]; - if let Err(_err) = verify_backed_candidate(parent_hash, check_ctx, &backed_candidate) { - continue; - } - for (i, assignment) in scheduled[skip..].iter().enumerate() { - if let Err(_err) = check_assignment_in_order(assignment) { - continue; - } + check_assignment_in_order(assignment)?; if para_id == assignment.para_id { if let Some(required_collator) = assignment.required_collator() { @@ -727,9 +664,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 checker_ctx = CandidateCheckContext::::new(now, relay_parent_number); - if let Err(err) = check_cx.check_validation_outputs( + if let Err(err) = checker_ctx.check_validation_outputs( para_id, &validation_outputs.head_data, &validation_outputs.new_validation_code, @@ -964,17 +901,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 expecte 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( diff --git a/runtime/parachains/src/paras_inherent.rs b/runtime/parachains/src/paras_inherent.rs index a9f63df5f7c5..c72fef1811d4 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, + initializer, scheduler::{self, CoreAssignment, FreedReason}, shared, ump, }; @@ -44,13 +46,14 @@ use primitives::v1::{ }; use rand::{Rng, 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; @@ -351,6 +354,8 @@ pub mod pallet { Error::::InvalidParentHeader, ); + let now = >::block_number(); + 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); @@ -454,7 +459,6 @@ pub mod pallet { ); // 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); } @@ -468,8 +472,14 @@ pub mod pallet { let backed_candidates = sanitize_backed_candidates::( parent_hash, backed_candidates, - move |candidate_hash: CandidateHash| -> bool { - ::DisputesHandler::concluded_invalid(current_session, candidate_hash) + move |_candidate_index: usize, + backed_candidate: &BackedCandidate| + -> bool { + ::DisputesHandler::concluded_invalid( + current_session, + backed_candidate.hash(), + ) + // `fn process_candidates` does the verification checks }, &scheduled[..], ); @@ -548,7 +558,7 @@ impl Pallet { T::DisputesHandler::filter_multi_dispute_data(&mut disputes); - let (concluded_invalid_disputes, mut bitfields, scheduled) = + let (concluded_invalid_disputes, mut bitfields, scheduled, now) = 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 @@ -627,7 +637,8 @@ 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(); @@ -638,14 +649,24 @@ impl Pallet { bitfields, // updated schedule scheduled, + // current block + now, )) }); + let relay_parent_number = now - One::one(); + // we never check duplicate code + let checker_ctx = CandidateCheckContext::::new(now, relay_parent_number); let mut backed_candidates = sanitize_backed_candidates::( parent_hash, backed_candidates, - move |candidate_hash: CandidateHash| -> bool { - concluded_invalid_disputes.contains(&candidate_hash) + move |candidate_idx: usize, + backed_candidate: &BackedCandidate<::Hash>| + -> bool { + concluded_invalid_disputes.contains(&backed_candidate.hash()) || + checker_ctx + .verify_backed_candidate(parent_hash, candidate_idx, backed_candidate) + .is_err() }, &scheduled[..], ); @@ -954,15 +975,21 @@ 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()) + let mut index = 0; + backed_candidates.retain(move |backed_candidate| { + let ret = !candidate_has_concluded_invalid_dispute_or_is_invalid(index, backed_candidate); + index += 1; + ret }); // Assure the backed candidate's `ParaId`'s core is free. From 585f16d0d9179286e7248f37b97c59614a070e44 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 23 Nov 2021 13:25:35 +0100 Subject: [PATCH 03/17] simplify shuffling --- runtime/parachains/src/paras_inherent.rs | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/runtime/parachains/src/paras_inherent.rs b/runtime/parachains/src/paras_inherent.rs index c72fef1811d4..af9e123e7bf6 100644 --- a/runtime/parachains/src/paras_inherent.rs +++ b/runtime/parachains/src/paras_inherent.rs @@ -44,7 +44,8 @@ use primitives::v1::{ UncheckedSignedAvailabilityBitfields, ValidatorId, ValidatorIndex, PARACHAINS_INHERENT_IDENTIFIER, }; -use rand::{Rng, SeedableRng}; +use rand::{SeedableRng, seq::SliceRandom}; + use scale_info::TypeInfo; use sp_runtime::traits::{Header as HeaderT, One}; use sp_std::{ @@ -735,11 +736,8 @@ 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) { @@ -752,12 +750,8 @@ 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); From e421e7b1c0fc6a1e7694f30fcd99f6d6bd274655 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 23 Nov 2021 13:25:44 +0100 Subject: [PATCH 04/17] Closes potential OOB weight --- runtime/parachains/src/paras_inherent.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/runtime/parachains/src/paras_inherent.rs b/runtime/parachains/src/paras_inherent.rs index af9e123e7bf6..499dd943b82c 100644 --- a/runtime/parachains/src/paras_inherent.rs +++ b/runtime/parachains/src/paras_inherent.rs @@ -741,7 +741,7 @@ fn random_sel Weight>( // 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 } @@ -753,7 +753,7 @@ fn random_sel Weight>( 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 From f55b459e151f1fd19e4c4ef7fdf72a9b57d361b0 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 23 Nov 2021 13:42:19 +0100 Subject: [PATCH 05/17] improve docs --- runtime/parachains/src/paras_inherent.rs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/runtime/parachains/src/paras_inherent.rs b/runtime/parachains/src/paras_inherent.rs index 499dd943b82c..96079d7ced4a 100644 --- a/runtime/parachains/src/paras_inherent.rs +++ b/runtime/parachains/src/paras_inherent.rs @@ -769,14 +769,19 @@ 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 bitfields, and fills the remaining space with bitfields. +/// In the common case, the block still has plenty(most) of remaining weight, which is used +/// to place as many backed candidates as possible. /// -/// 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. There is an exception for code upgrades that are prefered +/// for backed candidates, since with a increasing number of parachains their chances of +/// inclusion become slim. The DoS vector opened to a potentially malicious collator +/// is mitigated by checking all backed candidates beforehand in `fn create_inherent`. fn apply_weight_limit( candidates: &mut Vec::Hash>>, bitfields: &mut UncheckedSignedAvailabilityBitfields, @@ -869,8 +874,8 @@ 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. +/// `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( unchecked_bitfields: UncheckedSignedAvailabilityBitfields, disputed_bitfield: DisputedBitfield, From 92b12546229a768c7e1332a4f1f5bd70360b0241 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 23 Nov 2021 14:42:00 +0100 Subject: [PATCH 06/17] fooo --- runtime/parachains/src/inclusion.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime/parachains/src/inclusion.rs b/runtime/parachains/src/inclusion.rs index c981e8d43c3b..62515ef363f3 100644 --- a/runtime/parachains/src/inclusion.rs +++ b/runtime/parachains/src/inclusion.rs @@ -591,7 +591,7 @@ impl Pallet { backers, assignment.group_idx, )); - continue 'a + continue 'next_backed_candidate } } From 06d9c3f572735c3ada483823b08d4d781aca7e9d Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 23 Nov 2021 15:11:44 +0100 Subject: [PATCH 07/17] remove obsolete comment --- runtime/parachains/src/paras_inherent.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/runtime/parachains/src/paras_inherent.rs b/runtime/parachains/src/paras_inherent.rs index 96079d7ced4a..854a86c57fb2 100644 --- a/runtime/parachains/src/paras_inherent.rs +++ b/runtime/parachains/src/paras_inherent.rs @@ -574,9 +574,6 @@ 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? let current_concluded_invalid_disputes = disputes .iter() .filter(|dss| dss.session == current_session) From 720ff45999d0e30a52e94d47e2f6925fa7050da4 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 23 Nov 2021 15:17:17 +0100 Subject: [PATCH 08/17] move filtering into the rollback-transaction Technically this is not necessary but avoids future footguns. --- runtime/parachains/src/paras_inherent.rs | 58 +++++++++++++----------- 1 file changed, 32 insertions(+), 26 deletions(-) diff --git a/runtime/parachains/src/paras_inherent.rs b/runtime/parachains/src/paras_inherent.rs index 854a86c57fb2..852c6d5e7386 100644 --- a/runtime/parachains/src/paras_inherent.rs +++ b/runtime/parachains/src/paras_inherent.rs @@ -559,7 +559,7 @@ impl Pallet { T::DisputesHandler::filter_multi_dispute_data(&mut disputes); - let (concluded_invalid_disputes, mut bitfields, scheduled, now) = + 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 @@ -640,37 +640,43 @@ impl Pallet { let scheduled = >::scheduled(); + let relay_parent_number = now - One::one(); + + let checker_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()) || + // assure this only includes valid candidates + // and only code updates that are not overly frequent or too large by themselves + // and hence do not pose a DoS risk. Since we already have logic for that, + // we do this upfront to avoid intentionally submitted code upgrades above the allowed + // frequency to avoid anything other code-upgrade being included. + // This has to happen before the call to `fn apply_weight_limits`. + (backed_candidate.candidate.commitments.new_validation_code.is_some() && checker_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, - // current block - now, )) }); - let relay_parent_number = now - One::one(); - // we never check duplicate code - let checker_ctx = CandidateCheckContext::::new(now, relay_parent_number); - let mut backed_candidates = sanitize_backed_candidates::( - parent_hash, - backed_candidates, - move |candidate_idx: usize, - backed_candidate: &BackedCandidate<::Hash>| - -> bool { - concluded_invalid_disputes.contains(&backed_candidate.hash()) || - checker_ctx - .verify_backed_candidate(parent_hash, candidate_idx, backed_candidate) - .is_err() - }, - &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, @@ -2091,7 +2097,7 @@ 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() @@ -2191,7 +2197,7 @@ 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, From c59b710e143453591f7cc0f0170f608bdae6bc67 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 23 Nov 2021 19:21:35 +0100 Subject: [PATCH 09/17] move check up and avoid duplicate checks --- runtime/parachains/src/inclusion.rs | 41 +++++++++++++----------- runtime/parachains/src/paras_inherent.rs | 32 ++++++++++-------- 2 files changed, 42 insertions(+), 31 deletions(-) diff --git a/runtime/parachains/src/inclusion.rs b/runtime/parachains/src/inclusion.rs index 62515ef363f3..112b118d280e 100644 --- a/runtime/parachains/src/inclusion.rs +++ b/runtime/parachains/src/inclusion.rs @@ -427,12 +427,15 @@ 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, + ) -> Result, DispatchError> + where + GV: Fn(GroupIndex) -> Option> + { ensure!(candidates.len() <= scheduled.len(), Error::::UnscheduledCandidate); if scheduled.is_empty() { @@ -484,7 +487,9 @@ impl Pallet { 'next_backed_candidate: for (candidate_idx, backed_candidate) in candidates.iter().enumerate() { - check_ctx.verify_backed_candidate(parent_hash, candidate_idx, backed_candidate)?; + if !SKIP_CHECKER_CHECKS { + 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()]; @@ -1951,7 +1956,7 @@ pub(crate) mod tests { )); assert_noop!( - ParaInclusion::process_candidates( + ParaInclusion::process_candidates::<_, false>( Default::default(), vec![backed], vec![chain_b_assignment.clone()], @@ -2006,7 +2011,7 @@ pub(crate) mod tests { // out-of-order manifests as unscheduled. assert_noop!( - ParaInclusion::process_candidates( + ParaInclusion::process_candidates::<_, false>( Default::default(), vec![backed_b, backed_a], vec![chain_a_assignment.clone(), chain_b_assignment.clone()], @@ -2039,7 +2044,7 @@ pub(crate) mod tests { )); assert_noop!( - ParaInclusion::process_candidates( + ParaInclusion::process_candidates::<_, false>( Default::default(), vec![backed], vec![chain_a_assignment.clone()], @@ -2074,7 +2079,7 @@ pub(crate) mod tests { )); assert_noop!( - ParaInclusion::process_candidates( + ParaInclusion::process_candidates::<_, false>( Default::default(), vec![backed], vec![chain_a_assignment.clone()], @@ -2109,7 +2114,7 @@ pub(crate) mod tests { )); assert_noop!( - ParaInclusion::process_candidates( + ParaInclusion::process_candidates::<_, false>( Default::default(), vec![backed], vec![ @@ -2151,7 +2156,7 @@ pub(crate) mod tests { )); assert_noop!( - ParaInclusion::process_candidates( + ParaInclusion::process_candidates::<_, false>( Default::default(), vec![backed], vec![thread_a_assignment.clone()], @@ -2201,7 +2206,7 @@ pub(crate) mod tests { >::insert(&chain_a, candidate.commitments); assert_noop!( - ParaInclusion::process_candidates( + ParaInclusion::process_candidates::<_, false>( Default::default(), vec![backed], vec![chain_a_assignment.clone()], @@ -2244,7 +2249,7 @@ pub(crate) mod tests { )); assert_noop!( - ParaInclusion::process_candidates( + ParaInclusion::process_candidates::<_, false>( Default::default(), vec![backed], vec![chain_a_assignment.clone()], @@ -2295,7 +2300,7 @@ pub(crate) mod tests { } assert_noop!( - ParaInclusion::process_candidates( + ParaInclusion::process_candidates::<_, false>( Default::default(), vec![backed], vec![chain_a_assignment.clone()], @@ -2329,7 +2334,7 @@ pub(crate) mod tests { )); assert_eq!( - ParaInclusion::process_candidates( + ParaInclusion::process_candidates::<_, false>( Default::default(), vec![backed], vec![chain_a_assignment.clone()], @@ -2364,7 +2369,7 @@ pub(crate) mod tests { )); assert_noop!( - ParaInclusion::process_candidates( + ParaInclusion::process_candidates::<_, false>( Default::default(), vec![backed], vec![chain_a_assignment.clone()], @@ -2399,7 +2404,7 @@ pub(crate) mod tests { )); assert_noop!( - ParaInclusion::process_candidates( + ParaInclusion::process_candidates::<_, false>( Default::default(), vec![backed], vec![chain_a_assignment.clone()], @@ -2564,7 +2569,7 @@ pub(crate) mod tests { let ProcessedCandidates { core_indices: occupied_cores, candidate_receipt_with_backing_validator_indices, - } = ParaInclusion::process_candidates( + } = ParaInclusion::process_candidates::<_, false>( Default::default(), backed_candidates.clone(), vec![ @@ -2758,7 +2763,7 @@ pub(crate) mod tests { )); let ProcessedCandidates { core_indices: occupied_cores, .. } = - ParaInclusion::process_candidates( + ParaInclusion::process_candidates::<_, false>( Default::default(), vec![backed_a], vec![chain_a_assignment.clone()], diff --git a/runtime/parachains/src/paras_inherent.rs b/runtime/parachains/src/paras_inherent.rs index 852c6d5e7386..9c902c792a29 100644 --- a/runtime/parachains/src/paras_inherent.rs +++ b/runtime/parachains/src/paras_inherent.rs @@ -257,7 +257,7 @@ pub mod pallet { // 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()) { + match Self::enter_inner::(inherent_data.clone()) { Ok(_) => inherent_data, Err(err) => { log::error!( @@ -333,6 +333,14 @@ pub mod pallet { ensure!(!Included::::exists(), Error::::TooManyInclusionInherents); Included::::set(Some(())); + Self::enter_inner::(data) + } + } +} + +impl Pallet { + + pub(crate) fn enter_inner(data: ParachainsInherentData) -> DispatchResultWithPostInfo { let ParachainsInherentData { bitfields: mut signed_bitfields, mut backed_candidates, @@ -490,7 +498,7 @@ pub mod pallet { let inclusion::ProcessedCandidates::<::Hash> { core_indices: occupied, candidate_receipt_with_backing_validator_indices, - } = >::process_candidates( + } = >::process_candidates::<_, SKIP_CHECKER_CHECKS>( parent_storage_root, backed_candidates, scheduled, @@ -515,9 +523,9 @@ pub mod pallet { Ok(Some(total_weight).into()) } } -} impl Pallet { + /// Create the `ParachainsInherentData` that gets passed to [`Self::enter`] in [`Self::create_inherent`]. /// This code is pulled out of [`Self::create_inherent`] so it can be unit tested. fn create_inherent_inner(data: &InherentData) -> Option> { @@ -651,15 +659,13 @@ impl Pallet { -> bool { // never include a concluded-invalid candidate concluded_invalid_disputes.contains(&backed_candidate.hash()) || - // assure this only includes valid candidates - // and only code updates that are not overly frequent or too large by themselves - // and hence do not pose a DoS risk. Since we already have logic for that, - // we do this upfront to avoid intentionally submitted code upgrades above the allowed - // frequency to avoid anything other code-upgrade being included. - // This has to happen before the call to `fn apply_weight_limits`. - (backed_candidate.candidate.commitments.new_validation_code.is_some() && checker_ctx + // 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. + checker_ctx .verify_backed_candidate(parent_hash, candidate_idx, backed_candidate) - .is_err()) + .is_err() }, &scheduled[..], ); @@ -781,10 +787,10 @@ fn random_sel Weight>( /// In the common case, the block still has plenty(most) of remaining weight, which is used /// to place as many backed candidates as possible. /// -/// The selection process is random. There is an exception for code upgrades that are prefered +/// The selection process is random. There is an exception for code upgrades that are preferred /// for backed candidates, since with a increasing number of parachains their chances of /// inclusion become slim. The DoS vector opened to a potentially malicious collator -/// is mitigated by checking all backed candidates beforehand in `fn create_inherent`. +/// is mitigated by checking all backed candidates beforehand in `fn create_inherent_inner`. fn apply_weight_limit( candidates: &mut Vec::Hash>>, bitfields: &mut UncheckedSignedAvailabilityBitfields, From a6d022b1bb591568469a447e269e6907e4edb055 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 23 Nov 2021 19:23:37 +0100 Subject: [PATCH 10/17] refactor: make sure backed candidates are sane, even more --- runtime/parachains/src/inclusion.rs | 8 +- runtime/parachains/src/paras_inherent.rs | 366 +++++++++++------------ 2 files changed, 186 insertions(+), 188 deletions(-) diff --git a/runtime/parachains/src/inclusion.rs b/runtime/parachains/src/inclusion.rs index 112b118d280e..ebfc5c732dc8 100644 --- a/runtime/parachains/src/inclusion.rs +++ b/runtime/parachains/src/inclusion.rs @@ -434,7 +434,7 @@ impl Pallet { group_validators: GV, ) -> Result, DispatchError> where - GV: Fn(GroupIndex) -> Option> + GV: Fn(GroupIndex) -> Option>, { ensure!(candidates.len() <= scheduled.len(), Error::::UnscheduledCandidate); @@ -488,7 +488,11 @@ impl Pallet { candidates.iter().enumerate() { if !SKIP_CHECKER_CHECKS { - check_ctx.verify_backed_candidate(parent_hash, candidate_idx, backed_candidate)?; + check_ctx.verify_backed_candidate( + parent_hash, + candidate_idx, + backed_candidate, + )?; } let para_id = backed_candidate.descriptor().para_id; diff --git a/runtime/parachains/src/paras_inherent.rs b/runtime/parachains/src/paras_inherent.rs index 9c902c792a29..3b519ff6b0b2 100644 --- a/runtime/parachains/src/paras_inherent.rs +++ b/runtime/parachains/src/paras_inherent.rs @@ -44,7 +44,7 @@ use primitives::v1::{ UncheckedSignedAvailabilityBitfields, ValidatorId, ValidatorIndex, PARACHAINS_INHERENT_IDENTIFIER, }; -use rand::{SeedableRng, seq::SliceRandom}; +use rand::{seq::SliceRandom, SeedableRng}; use scale_info::TypeInfo; use sp_runtime::traits::{Header as HeaderT, One}; @@ -256,25 +256,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_inner::(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()) { + 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 }) } @@ -339,193 +338,188 @@ pub mod pallet { } impl Pallet { + pub(crate) fn enter_inner( + data: ParachainsInherentData, + ) -> DispatchResultWithPostInfo { + let ParachainsInherentData { + bitfields: mut signed_bitfields, + mut backed_candidates, + parent_header, + mut disputes, + } = data; - pub(crate) fn enter_inner(data: ParachainsInherentData) -> 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() + ); - log::debug!( - target: LOG_TARGET, - "[enter] bitfields.len(): {}, backed_candidates.len(): {}, disputes.len() {}", - signed_bitfields.len(), - backed_candidates.len(), - disputes.len() - ); + // 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, + ); - // 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, - ); + let now = >::block_number(); - let now = >::block_number(); + 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 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 max_block_weight = ::BlockWeights::get().max_block; - let max_block_weight = ::BlockWeights::get().max_block; + // 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; + } - // 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; - } + if disputes_weight > max_block_weight { + // if disputes are by themselves overweight already, trim the disputes. + debug_assert!(candidate_weight == 0 && bitfields_weight == 0); - 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 entropy = compute_entropy::(parent_hash); + let mut rng = rand_chacha::ChaChaRng::from_seed(entropy.into()); - let entropy = compute_entropy::(parent_hash); - let mut rng = rand_chacha::ChaChaRng::from_seed(entropy.into()); + 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) + } + }; - 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) - } - }; + let expected_bits = >::availability_cores().len(); - let expected_bits = >::availability_cores().len(); + // 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()) + } - // Handle disputes logic. - let current_session = >::session_index(); - let disputed_bitfield = { - let new_current_dispute_sets: Vec<_> = disputes + let mut freed_disputed = if !new_current_dispute_sets.is_empty() { + let concluded_invalid_disputes = new_current_dispute_sets .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 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() - }; - - // 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), - ); - - 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); - } + .filter(|(session, candidate)| { + T::DisputesHandler::concluded_invalid(*session, *candidate) + }) + .map(|(_, candidate)| *candidate) + .collect::>(); - disputed_bitfield + 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. - 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()); - - >::clear(); - >::schedule(freed, now); - - 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[..], - ); + disputed_bitfield + }; - // 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::<_, SKIP_CHECKER_CHECKS>( - 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, - }); + // 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, + ); - // Note which of the scheduled cores were actually occupied by a backed candidate. - >::occupied(&occupied); + // Inform the disputes module of all included candidates. + for (_, candidate_hash) in &freed_concluded { + T::DisputesHandler::note_included(current_session, *candidate_hash, 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 freed = collect_all_freed_cores::(freed_concluded.iter().cloned()); - Ok(Some(total_weight).into()) - } + >::clear(); + >::schedule(freed, now); + + 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[..], + ); + + // 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::<_, SKIP_CHECKER_CHECKS>( + 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, + }); + + // 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()) } +} impl Pallet { - /// Create the `ParachainsInherentData` that gets passed to [`Self::enter`] in [`Self::create_inherent`]. /// This code is pulled out of [`Self::create_inherent`] so it can be unit tested. fn create_inherent_inner(data: &InherentData) -> Option> { @@ -655,9 +649,9 @@ impl Pallet { parent_hash, backed_candidates, move |candidate_idx: usize, - backed_candidate: &BackedCandidate<::Hash>| - -> bool { - // never include a concluded-invalid candidate + 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. @@ -678,7 +672,6 @@ impl Pallet { )) }); - let entropy = compute_entropy::(parent_hash); let mut rng = rand_chacha::ChaChaRng::from_seed(entropy.into()); @@ -747,7 +740,6 @@ fn random_sel Weight>( 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.saturating_add(weight_fn(item)); @@ -789,8 +781,8 @@ fn random_sel Weight>( /// /// The selection process is random. There is an exception for code upgrades that are preferred /// for backed candidates, since with a increasing number of parachains their chances of -/// inclusion become slim. The DoS vector opened to a potentially malicious collator -/// is mitigated by checking all backed candidates beforehand in `fn create_inherent_inner`. +/// 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, @@ -2103,7 +2095,8 @@ mod tests { .unwrap(); } - let has_concluded_invalid = |_idx: usize, _backed_candidate: &BackedCandidate| -> bool { false }; + let has_concluded_invalid = + |_idx: usize, _backed_candidate: &BackedCandidate| -> bool { false }; let scheduled = (0_usize..2) .into_iter() @@ -2203,7 +2196,8 @@ mod tests { } set }; - let has_concluded_invalid = |_idx: usize, candidate: &BackedCandidate| set.contains(&candidate.hash()); + let has_concluded_invalid = + |_idx: usize, candidate: &BackedCandidate| set.contains(&candidate.hash()); assert_eq!( sanitize_backed_candidates::( relay_parent, From 25509e90aacb81ba64e8316788b98c460eba0256 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Wed, 24 Nov 2021 08:03:49 +0100 Subject: [PATCH 11/17] doc wording Co-authored-by: Zeke Mostov --- runtime/parachains/src/inclusion.rs | 2 +- runtime/parachains/src/paras_inherent.rs | 7 +++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/runtime/parachains/src/inclusion.rs b/runtime/parachains/src/inclusion.rs index ebfc5c732dc8..bf8271fc3ce6 100644 --- a/runtime/parachains/src/inclusion.rs +++ b/runtime/parachains/src/inclusion.rs @@ -924,7 +924,7 @@ impl CandidateCheckContext { /// Execute verification of the candidate. /// /// Assures: - /// * correct expecte relay parent reference + /// * 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 diff --git a/runtime/parachains/src/paras_inherent.rs b/runtime/parachains/src/paras_inherent.rs index 3b519ff6b0b2..4f7d8584fc03 100644 --- a/runtime/parachains/src/paras_inherent.rs +++ b/runtime/parachains/src/paras_inherent.rs @@ -775,11 +775,10 @@ fn random_sel Weight>( /// If there is sufficient space, all disputes, all bitfields and all candidates /// will be included. /// -/// Otherwise tries to include all bitfields, and fills the remaining space with bitfields. -/// In the common case, the block still has plenty(most) of remaining weight, which is used -/// to place as many backed candidates as possible. +/// Otherwise tries to include all disputes, and then tries to fill the remaining space with bitfields and then candidates. /// -/// The selection process is random. There is an exception for code upgrades that are preferred +/// 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. From 6b041d588efe2521044d7387f06b0dd711523749 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Wed, 24 Nov 2021 10:59:38 +0100 Subject: [PATCH 12/17] refactor: avoid const generics for sake of wasm size `true` -> `FullCheck::Skip`, `false` -> `FullCheck::Yes`. --- runtime/parachains/src/inclusion.rs | 61 +++++++++++++++++------- runtime/parachains/src/paras_inherent.rs | 12 +++-- 2 files changed, 52 insertions(+), 21 deletions(-) diff --git a/runtime/parachains/src/inclusion.rs b/runtime/parachains/src/inclusion.rs index bf8271fc3ce6..a663e4c48c6d 100644 --- a/runtime/parachains/src/inclusion.rs +++ b/runtime/parachains/src/inclusion.rs @@ -56,6 +56,20 @@ 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, TypeInfo)] +#[cfg_attr(test, derive(Debug))] +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))] @@ -427,11 +441,12 @@ 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: GV, + full_check: FullCheck, ) -> Result, DispatchError> where GV: Fn(GroupIndex) -> Option>, @@ -487,7 +502,7 @@ impl Pallet { 'next_backed_candidate: for (candidate_idx, backed_candidate) in candidates.iter().enumerate() { - if !SKIP_CHECKER_CHECKS { + if let FullCheck::Yes = full_check { check_ctx.verify_backed_candidate( parent_hash, candidate_idx, @@ -1960,11 +1975,12 @@ pub(crate) mod tests { )); assert_noop!( - ParaInclusion::process_candidates::<_, false>( + ParaInclusion::process_candidates( Default::default(), vec![backed], vec![chain_b_assignment.clone()], &group_validators, + FullCheck::Yes, ), Error::::UnscheduledCandidate ); @@ -2015,11 +2031,12 @@ pub(crate) mod tests { // out-of-order manifests as unscheduled. assert_noop!( - ParaInclusion::process_candidates::<_, false>( + ParaInclusion::process_candidates( Default::default(), vec![backed_b, backed_a], vec![chain_a_assignment.clone(), chain_b_assignment.clone()], &group_validators, + FullCheck::Yes, ), Error::::UnscheduledCandidate ); @@ -2048,11 +2065,12 @@ pub(crate) mod tests { )); assert_noop!( - ParaInclusion::process_candidates::<_, false>( + ParaInclusion::process_candidates( Default::default(), vec![backed], vec![chain_a_assignment.clone()], &group_validators, + FullCheck::Yes, ), Error::::InsufficientBacking ); @@ -2083,11 +2101,12 @@ pub(crate) mod tests { )); assert_noop!( - ParaInclusion::process_candidates::<_, false>( + ParaInclusion::process_candidates( Default::default(), vec![backed], vec![chain_a_assignment.clone()], &group_validators, + FullCheck::Yes, ), Error::::CandidateNotInParentContext ); @@ -2118,7 +2137,7 @@ pub(crate) mod tests { )); assert_noop!( - ParaInclusion::process_candidates::<_, false>( + ParaInclusion::process_candidates( Default::default(), vec![backed], vec![ @@ -2127,6 +2146,7 @@ pub(crate) mod tests { thread_a_assignment.clone(), ], &group_validators, + FullCheck::Yes, ), Error::::WrongCollator, ); @@ -2160,11 +2180,12 @@ pub(crate) mod tests { )); assert_noop!( - ParaInclusion::process_candidates::<_, false>( + ParaInclusion::process_candidates( Default::default(), vec![backed], vec![thread_a_assignment.clone()], &group_validators, + FullCheck::Yes, ), Error::::NotCollatorSigned ); @@ -2210,11 +2231,12 @@ pub(crate) mod tests { >::insert(&chain_a, candidate.commitments); assert_noop!( - ParaInclusion::process_candidates::<_, false>( + ParaInclusion::process_candidates( Default::default(), vec![backed], vec![chain_a_assignment.clone()], &group_validators, + FullCheck::Yes, ), Error::::CandidateScheduledBeforeParaFree ); @@ -2253,11 +2275,12 @@ pub(crate) mod tests { )); assert_noop!( - ParaInclusion::process_candidates::<_, false>( + ParaInclusion::process_candidates( Default::default(), vec![backed], vec![chain_a_assignment.clone()], &group_validators, + FullCheck::Yes, ), Error::::CandidateScheduledBeforeParaFree ); @@ -2304,11 +2327,12 @@ pub(crate) mod tests { } assert_noop!( - ParaInclusion::process_candidates::<_, false>( + ParaInclusion::process_candidates( Default::default(), vec![backed], vec![chain_a_assignment.clone()], &group_validators, + FullCheck::Yes, ), Error::::PrematureCodeUpgrade ); @@ -2338,11 +2362,12 @@ pub(crate) mod tests { )); assert_eq!( - ParaInclusion::process_candidates::<_, false>( + ParaInclusion::process_candidates( Default::default(), vec![backed], vec![chain_a_assignment.clone()], &group_validators, + FullCheck::Yes, ), Err(Error::::ValidationDataHashMismatch.into()), ); @@ -2373,11 +2398,12 @@ pub(crate) mod tests { )); assert_noop!( - ParaInclusion::process_candidates::<_, false>( + ParaInclusion::process_candidates( Default::default(), vec![backed], vec![chain_a_assignment.clone()], &group_validators, + FullCheck::Yes, ), Error::::InvalidValidationCodeHash ); @@ -2408,11 +2434,12 @@ pub(crate) mod tests { )); assert_noop!( - ParaInclusion::process_candidates::<_, false>( + ParaInclusion::process_candidates( Default::default(), vec![backed], vec![chain_a_assignment.clone()], &group_validators, + FullCheck::Yes, ), Error::::ParaHeadMismatch ); @@ -2573,7 +2600,7 @@ pub(crate) mod tests { let ProcessedCandidates { core_indices: occupied_cores, candidate_receipt_with_backing_validator_indices, - } = ParaInclusion::process_candidates::<_, false>( + } = ParaInclusion::process_candidates( Default::default(), backed_candidates.clone(), vec![ @@ -2582,6 +2609,7 @@ pub(crate) mod tests { thread_a_assignment.clone(), ], &group_validators, + FullCheck::Yes, ) .expect("candidates scheduled, in order, and backed"); @@ -2767,11 +2795,12 @@ pub(crate) mod tests { )); let ProcessedCandidates { core_indices: occupied_cores, .. } = - ParaInclusion::process_candidates::<_, false>( + ParaInclusion::process_candidates( Default::default(), 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 4f7d8584fc03..d07e5eba9a8d 100644 --- a/runtime/parachains/src/paras_inherent.rs +++ b/runtime/parachains/src/paras_inherent.rs @@ -24,7 +24,7 @@ use crate::{ disputes::DisputesHandler, inclusion, - inclusion::CandidateCheckContext, + inclusion::{CandidateCheckContext, FullCheck}, initializer, scheduler::{self, CoreAssignment, FreedReason}, shared, ump, @@ -256,7 +256,7 @@ 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_inner::(inherent_data.clone()) { + let inherent_data = match Self::enter_inner(inherent_data.clone(), FullCheck::Skip) { Ok(_) => inherent_data, Err(err) => { log::error!( @@ -332,14 +332,15 @@ pub mod pallet { ensure!(!Included::::exists(), Error::::TooManyInclusionInherents); Included::::set(Some(())); - Self::enter_inner::(data) + Self::enter_inner(data, FullCheck::Yes) } } } impl Pallet { - pub(crate) fn enter_inner( + pub(crate) fn enter_inner( data: ParachainsInherentData, + full_check: FullCheck, ) -> DispatchResultWithPostInfo { let ParachainsInherentData { bitfields: mut signed_bitfields, @@ -493,11 +494,12 @@ impl Pallet { let inclusion::ProcessedCandidates::<::Hash> { core_indices: occupied, candidate_receipt_with_backing_validator_indices, - } = >::process_candidates::<_, SKIP_CHECKER_CHECKS>( + } = >::process_candidates( parent_storage_root, backed_candidates, scheduled, >::group_validators, + full_check, )?; // The number of disputes included in a block is From bf5c52e841e98f07f8435ff6e9ab0086e0cde6a3 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Wed, 24 Nov 2021 11:01:50 +0100 Subject: [PATCH 13/17] chore: unify `CandidateCheckContext` instance names --- runtime/parachains/src/inclusion.rs | 4 ++-- runtime/parachains/src/paras_inherent.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/runtime/parachains/src/inclusion.rs b/runtime/parachains/src/inclusion.rs index a663e4c48c6d..47cb60586a12 100644 --- a/runtime/parachains/src/inclusion.rs +++ b/runtime/parachains/src/inclusion.rs @@ -688,9 +688,9 @@ impl Pallet { // `relay_parent_number` is equal to `now`. let now = >::block_number(); let relay_parent_number = now; - let checker_ctx = CandidateCheckContext::::new(now, relay_parent_number); + let check_ctx = CandidateCheckContext::::new(now, relay_parent_number); - if let Err(err) = checker_ctx.check_validation_outputs( + if let Err(err) = check_ctx.check_validation_outputs( para_id, &validation_outputs.head_data, &validation_outputs.new_validation_code, diff --git a/runtime/parachains/src/paras_inherent.rs b/runtime/parachains/src/paras_inherent.rs index d07e5eba9a8d..6c777a055e8d 100644 --- a/runtime/parachains/src/paras_inherent.rs +++ b/runtime/parachains/src/paras_inherent.rs @@ -646,7 +646,7 @@ impl Pallet { let relay_parent_number = now - One::one(); - let checker_ctx = CandidateCheckContext::::new(now, relay_parent_number); + let check_ctx = CandidateCheckContext::::new(now, relay_parent_number); let backed_candidates = sanitize_backed_candidates::( parent_hash, backed_candidates, @@ -659,7 +659,7 @@ impl Pallet { // 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. - checker_ctx + check_ctx .verify_backed_candidate(parent_hash, candidate_idx, backed_candidate) .is_err() }, From 43bef25281d6353f04ce68b6528958adbe836aaa Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Wed, 24 Nov 2021 11:19:06 +0100 Subject: [PATCH 14/17] refactor: introduce `IndexedRetain` for `Vec` --- runtime/parachains/src/paras_inherent.rs | 44 +++++++++++++++--------- 1 file changed, 27 insertions(+), 17 deletions(-) diff --git a/runtime/parachains/src/paras_inherent.rs b/runtime/parachains/src/paras_inherent.rs index 6c777a055e8d..7620300c089e 100644 --- a/runtime/parachains/src/paras_inherent.rs +++ b/runtime/parachains/src/paras_inherent.rs @@ -162,6 +162,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)] @@ -826,12 +849,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); @@ -850,12 +868,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 } @@ -986,11 +999,8 @@ fn sanitize_backed_candidates< scheduled: &[CoreAssignment], ) -> Vec> { // Remove any candidates that were concluded invalid. - let mut index = 0; - backed_candidates.retain(move |backed_candidate| { - let ret = !candidate_has_concluded_invalid_dispute_or_is_invalid(index, backed_candidate); - index += 1; - ret + 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. From 9b251141ab0eaa709abc33570d2e3a04e89ad44f Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Wed, 24 Nov 2021 11:51:04 +0100 Subject: [PATCH 15/17] chore: make tests prefix free --- runtime/parachains/src/paras_inherent.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/runtime/parachains/src/paras_inherent.rs b/runtime/parachains/src/paras_inherent.rs index 7620300c089e..190c0999d5be 100644 --- a/runtime/parachains/src/paras_inherent.rs +++ b/runtime/parachains/src/paras_inherent.rs @@ -1695,7 +1695,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(); @@ -1766,7 +1766,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(); From ab394dcd7be3cbd06686f1778cd7dd90f84e4c89 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Wed, 24 Nov 2021 12:40:32 +0100 Subject: [PATCH 16/17] doc: re-introduce removed comment --- runtime/parachains/src/paras_inherent.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/runtime/parachains/src/paras_inherent.rs b/runtime/parachains/src/paras_inherent.rs index 190c0999d5be..0d3ddce8ba8d 100644 --- a/runtime/parachains/src/paras_inherent.rs +++ b/runtime/parachains/src/paras_inherent.rs @@ -601,6 +601,13 @@ impl Pallet { e }); + // 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) @@ -611,8 +618,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()) From 6cec917f885dffeb265a30ccad6b7c2bb6dbab73 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Wed, 24 Nov 2021 12:55:49 +0100 Subject: [PATCH 17/17] refactor: remove another const generic to save some wasm size --- .../src/runtime/inclusion.md | 5 +- runtime/parachains/src/inclusion.rs | 8 +- runtime/parachains/src/paras_inherent.rs | 88 +++++++++++-------- 3 files changed, 57 insertions(+), 44 deletions(-) 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 47cb60586a12..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, }; @@ -58,8 +58,7 @@ pub struct AvailabilityBitfieldRecord { /// 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, TypeInfo)] -#[cfg_attr(test, derive(Debug))] +#[derive(Encode, Decode, PartialEq, Eq, RuntimeDebug, TypeInfo)] pub(crate) enum FullCheck { /// Yes, do a full check, skip nothing. Yes, @@ -417,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>( diff --git a/runtime/parachains/src/paras_inherent.rs b/runtime/parachains/src/paras_inherent.rs index 0d3ddce8ba8d..15abaca24708 100644 --- a/runtime/parachains/src/paras_inherent.rs +++ b/runtime/parachains/src/paras_inherent.rs @@ -59,8 +59,6 @@ use sp_std::{ 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 @@ -646,13 +644,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 = @@ -896,15 +895,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`, +/// `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( +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()); @@ -924,8 +924,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(), ); @@ -970,7 +970,7 @@ pub(crate) fn 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() ); @@ -1947,25 +1949,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 @@ -1974,22 +1978,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()); } @@ -1998,24 +2004,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] ); @@ -2027,24 +2035,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)] ); @@ -2062,24 +2072,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[..] );