From 954090348d00a23f21594eeb6cc70d93b22a0723 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Mon, 22 Nov 2021 17:16:28 +0100 Subject: [PATCH 1/2] 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 7877ad1fd44e3227336bf52c9ffefaa3b4601e9d Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Mon, 22 Nov 2021 18:49:28 +0100 Subject: [PATCH 2/2] 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.