From 2981c6a5ce42190e9ad81cb5de2736b4d22b4020 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Tue, 20 Jun 2023 14:06:25 -0700 Subject: [PATCH 01/19] break candidate receipt construction and distribution into own function --- node/collation-generation/src/lib.rs | 257 ++++++++++++++++----------- 1 file changed, 150 insertions(+), 107 deletions(-) diff --git a/node/collation-generation/src/lib.rs b/node/collation-generation/src/lib.rs index 5599a737059f..903234c4a533 100644 --- a/node/collation-generation/src/lib.rs +++ b/node/collation-generation/src/lib.rs @@ -29,9 +29,9 @@ #![deny(missing_docs)] -use futures::{channel::mpsc, future::FutureExt, join, select, sink::SinkExt, stream::StreamExt}; +use futures::{channel::{mpsc, oneshot}, future::FutureExt, join, select, sink::SinkExt, stream::StreamExt}; use parity_scale_codec::Encode; -use polkadot_node_primitives::{AvailableData, CollationGenerationConfig, PoV}; +use polkadot_node_primitives::{AvailableData, Collation, CollationGenerationConfig, CollationSecondedSignal, PoV}; use polkadot_node_subsystem::{ messages::{CollationGenerationMessage, CollatorProtocolMessage}, overseer, ActiveLeavesUpdate, FromOrchestra, OverseerSignal, SpawnedSubsystem, @@ -44,7 +44,7 @@ use polkadot_node_subsystem_util::{ use polkadot_primitives::{ collator_signature_payload, CandidateCommitments, CandidateDescriptor, CandidateReceipt, CoreState, Hash, Id as ParaId, OccupiedCoreAssumption, PersistedValidationData, - ValidationCodeHash, + ValidationCodeHash, CollatorPair, }; use sp_core::crypto::Pair; use std::sync::Arc; @@ -291,14 +291,11 @@ async fn handle_new_activations( }; let task_config = config.clone(); - let mut task_sender = sender.clone(); + let task_sender = sender.clone(); let metrics = metrics.clone(); ctx.spawn( "collation-builder", Box::pin(async move { - let persisted_validation_data_hash = validation_data.hash(); - let parent_head_data_hash = validation_data.parent_head.hash(); - let (collation, result_sender) = match (task_config.collator)(relay_parent, &validation_data).await { Some(collation) => collation.into_inner(), @@ -312,109 +309,21 @@ async fn handle_new_activations( }, }; - // Apply compression to the block data. - let pov = { - let pov = collation.proof_of_validity.into_compressed(); - let encoded_size = pov.encoded_size(); - - // As long as `POV_BOMB_LIMIT` is at least `max_pov_size`, this ensures - // that honest collators never produce a PoV which is uncompressed. - // - // As such, honest collators never produce an uncompressed PoV which starts with - // a compression magic number, which would lead validators to reject the collation. - if encoded_size > validation_data.max_pov_size as usize { - gum::debug!( - target: LOG_TARGET, - para_id = %scheduled_core.para_id, - size = encoded_size, - max_size = validation_data.max_pov_size, - "PoV exceeded maximum size" - ); - - return - } - - pov - }; - - let pov_hash = pov.hash(); - - let signature_payload = collator_signature_payload( - &relay_parent, - &scheduled_core.para_id, - &persisted_validation_data_hash, - &pov_hash, - &validation_code_hash, - ); - - let erasure_root = - match erasure_root(n_validators, validation_data, pov.clone()) { - Ok(erasure_root) => erasure_root, - Err(err) => { - gum::error!( - target: LOG_TARGET, - para_id = %scheduled_core.para_id, - err = ?err, - "failed to calculate erasure root", - ); - return - }, - }; - - let commitments = CandidateCommitments { - upward_messages: collation.upward_messages, - horizontal_messages: collation.horizontal_messages, - new_validation_code: collation.new_validation_code, - head_data: collation.head_data, - processed_downward_messages: collation.processed_downward_messages, - hrmp_watermark: collation.hrmp_watermark, - }; - - let ccr = CandidateReceipt { - commitments_hash: commitments.hash(), - descriptor: CandidateDescriptor { - signature: task_config.key.sign(&signature_payload), - para_id: scheduled_core.para_id, + construct_and_distribute_receipt( + PreparedCollation { + collation, + para_id: task_config.para_id, relay_parent, - collator: task_config.key.public(), - persisted_validation_data_hash, - pov_hash, - erasure_root, - para_head: commitments.head_data.hash(), + validation_data, validation_code_hash, + n_validators, }, - }; - - gum::debug!( - target: LOG_TARGET, - candidate_hash = ?ccr.hash(), - ?pov_hash, - ?relay_parent, - para_id = %scheduled_core.para_id, - "candidate is generated", - ); - metrics.on_collation_generated(); - - if let Err(err) = task_sender - .send( - CollatorProtocolMessage::DistributeCollation( - ccr, - parent_head_data_hash, - pov, - result_sender, - ) - .into(), - ) - .await - { - gum::warn!( - target: LOG_TARGET, - para_id = %scheduled_core.para_id, - err = ?err, - "failed to send collation result", - ); - } - }), + task_config.key.clone(), + task_sender, + result_sender, + &metrics, + ).await; + }) )?; } } @@ -422,6 +331,140 @@ async fn handle_new_activations( Ok(()) } +struct PreparedCollation { + collation: Collation, + para_id: ParaId, + relay_parent: Hash, + validation_data: PersistedValidationData, + validation_code_hash: ValidationCodeHash, + n_validators: usize, +} + +/// Takes a prepared collation, along with its context, and produces a candidate receipt +/// which is distributed to validators. +async fn construct_and_distribute_receipt( + collation: PreparedCollation, + key: CollatorPair, + mut sender: mpsc::Sender, + result_sender: Option>, + metrics: &Metrics, +) { + let PreparedCollation { + collation, + para_id, + relay_parent, + validation_data, + validation_code_hash, + n_validators, + } = collation; + + let persisted_validation_data_hash = validation_data.hash(); + let parent_head_data_hash = validation_data.parent_head.hash(); + + // Apply compression to the block data. + let pov = { + let pov = collation.proof_of_validity.into_compressed(); + let encoded_size = pov.encoded_size(); + + // As long as `POV_BOMB_LIMIT` is at least `max_pov_size`, this ensures + // that honest collators never produce a PoV which is uncompressed. + // + // As such, honest collators never produce an uncompressed PoV which starts with + // a compression magic number, which would lead validators to reject the collation. + if encoded_size > validation_data.max_pov_size as usize { + gum::debug!( + target: LOG_TARGET, + para_id = %para_id, + size = encoded_size, + max_size = validation_data.max_pov_size, + "PoV exceeded maximum size" + ); + + return + } + + pov + }; + + let pov_hash = pov.hash(); + + let signature_payload = collator_signature_payload( + &relay_parent, + ¶_id, + &persisted_validation_data_hash, + &pov_hash, + &validation_code_hash, + ); + + let erasure_root = + match erasure_root(n_validators, validation_data, pov.clone()) { + Ok(erasure_root) => erasure_root, + Err(err) => { + gum::error!( + target: LOG_TARGET, + para_id = %para_id, + err = ?err, + "failed to calculate erasure root", + ); + return + }, + }; + + let commitments = CandidateCommitments { + upward_messages: collation.upward_messages, + horizontal_messages: collation.horizontal_messages, + new_validation_code: collation.new_validation_code, + head_data: collation.head_data, + processed_downward_messages: collation.processed_downward_messages, + hrmp_watermark: collation.hrmp_watermark, + }; + + let ccr = CandidateReceipt { + commitments_hash: commitments.hash(), + descriptor: CandidateDescriptor { + signature: key.sign(&signature_payload), + para_id: para_id, + relay_parent, + collator: key.public(), + persisted_validation_data_hash, + pov_hash, + erasure_root, + para_head: commitments.head_data.hash(), + validation_code_hash, + }, + }; + + gum::debug!( + target: LOG_TARGET, + candidate_hash = ?ccr.hash(), + ?pov_hash, + ?relay_parent, + para_id = %para_id, + "candidate is generated", + ); + metrics.on_collation_generated(); + + if let Err(err) = sender + .send( + CollatorProtocolMessage::DistributeCollation( + ccr, + parent_head_data_hash, + pov, + result_sender, + ) + .into(), + ) + .await + { + gum::warn!( + target: LOG_TARGET, + para_id = %para_id, + err = ?err, + "failed to send collation result", + ); + } +} + async fn obtain_current_validation_code_hash( relay_parent: Hash, para_id: ParaId, From 32fde3634681a720eb0f3b97023dc9d2bae35985 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Tue, 20 Jun 2023 14:42:16 -0700 Subject: [PATCH 02/19] update implementers' guide to include SubmitCollation --- .../src/types/overseer-protocol.md | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/roadmap/implementers-guide/src/types/overseer-protocol.md b/roadmap/implementers-guide/src/types/overseer-protocol.md index 30e6dc848802..995f53d99c2b 100644 --- a/roadmap/implementers-guide/src/types/overseer-protocol.md +++ b/roadmap/implementers-guide/src/types/overseer-protocol.md @@ -403,6 +403,44 @@ enum CollatorProtocolMessage { } ``` +## Collation Generation Message + +Messages received by the [Collation Generation subsystem](../node/collators/collation-generation.md) + +This is the core interface by which collators built on top of a Polkadot node submit collations to validators. As such, these messages are not sent by any subsystem but are instead sent from outside of the overseer. + +```rust +/// A function provided to the subsystem which it uses to pull new collations. +/// +/// This mode of querying collations is obsoleted by `CollationGenerationMessages::SubmitCollation` +/// +/// The response channel, if present, is meant to receive a `Seconded` statement as a +/// form of authentication, for collation mechanisms which rely on this for anti-spam. +type CollatorFn = Fn(Hash, PersistedValidationData) -> Future>)>; + +/// Configuration for the collation generator +struct CollationGenerationConfig { + /// Collator's authentication key, so it can sign things. + key: CollatorPair, + /// Collation function. See [`CollatorFn`] for more details. + collator: CollatorFn, + /// The parachain that this collator collates for + para_id: ParaId, +} + +enum CollationGenerationMessage { + /// Initialize the collation generation subsystem + Initialize(CollationGenerationConfig), + /// Submit a collation to the subsystem. This will package it into a signed + /// [`CommittedCandidateReceipt`] and distribute along the network to validators. + SubmitCollation( + Collation, + Hash, // Relay Parent + HeadData, // Parent Head + ), +} +``` + ## Dispute Coordinator Message Messages received by the [Dispute Coordinator subsystem](../node/disputes/dispute-coordinator.md) From 11decbbf4be45643ebce1f45be5b5d657bf385ff Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Wed, 21 Jun 2023 19:13:27 -0700 Subject: [PATCH 03/19] implement SubmitCollation for collation-generation --- node/collation-generation/src/lib.rs | 182 +++++++++++++++++- node/collation-generation/src/metrics.rs | 19 ++ node/primitives/src/lib.rs | 56 +++++- node/subsystem-types/src/messages.rs | 6 + parachain/src/primitives.rs | 3 +- .../node/collators/collation-generation.md | 14 +- .../src/types/overseer-protocol.md | 38 +++- 7 files changed, 296 insertions(+), 22 deletions(-) diff --git a/node/collation-generation/src/lib.rs b/node/collation-generation/src/lib.rs index 903234c4a533..883fb05de068 100644 --- a/node/collation-generation/src/lib.rs +++ b/node/collation-generation/src/lib.rs @@ -31,20 +31,21 @@ use futures::{channel::{mpsc, oneshot}, future::FutureExt, join, select, sink::SinkExt, stream::StreamExt}; use parity_scale_codec::Encode; -use polkadot_node_primitives::{AvailableData, Collation, CollationGenerationConfig, CollationSecondedSignal, PoV}; +use polkadot_node_primitives::{AvailableData, Collation, CollationGenerationConfig, CollationSecondedSignal, PoV, SubmitCollationParams, ValidationCodeHashHint}; use polkadot_node_subsystem::{ - messages::{CollationGenerationMessage, CollatorProtocolMessage}, + messages::{CollationGenerationMessage, CollatorProtocolMessage, RuntimeApiMessage, RuntimeApiRequest}, overseer, ActiveLeavesUpdate, FromOrchestra, OverseerSignal, SpawnedSubsystem, - SubsystemContext, SubsystemError, SubsystemResult, + SubsystemContext, SubsystemError, SubsystemResult, RuntimeApiError, }; use polkadot_node_subsystem_util::{ request_availability_cores, request_persisted_validation_data, request_validation_code, request_validation_code_hash, request_validators, }; use polkadot_primitives::{ + vstaging::BackingState, collator_signature_payload, CandidateCommitments, CandidateDescriptor, CandidateReceipt, CoreState, Hash, Id as ParaId, OccupiedCoreAssumption, PersistedValidationData, - ValidationCodeHash, CollatorPair, + ValidationCodeHash, CollatorPair, HeadData, }; use sp_core::crypto::Pair; use std::sync::Arc; @@ -152,6 +153,25 @@ impl CollationGenerationSubsystem { } false }, + Ok(FromOrchestra::Communication { + msg: CollationGenerationMessage::SubmitCollation(params), + }) => { + if let Some(config) = &self.config { + if let Err(err) = handle_submit_collation( + params, + config, + ctx, + sender.clone(), + &self.metrics, + ).await { + gum::error!(target: LOG_TARGET, ?err, "Failed to submit collation"); + } + } else { + gum::error!(target: LOG_TARGET, "Collation submitted before initialization"); + } + + false + } Ok(FromOrchestra::Signal(OverseerSignal::BlockFinalized(..))) => false, Err(err) => { gum::error!( @@ -190,6 +210,8 @@ async fn handle_new_activations( // follow the procedure from the guide: // https://paritytech.github.io/polkadot/book/node/collators/collation-generation.html + if config.collator.is_none() { return Ok(()) } + let _overall_timer = metrics.time_new_activations(); for relay_parent in activated { @@ -268,7 +290,7 @@ async fn handle_new_activations( }, }; - let validation_code_hash = match obtain_current_validation_code_hash( + let validation_code_hash = match obtain_validation_code_hash_with_assumption( relay_parent, scheduled_core.para_id, assumption, @@ -296,8 +318,13 @@ async fn handle_new_activations( ctx.spawn( "collation-builder", Box::pin(async move { + let collator_fn = match task_config.collator.as_ref() { + Some(x) => x, + None => return, + }; + let (collation, result_sender) = - match (task_config.collator)(relay_parent, &validation_data).await { + match collator_fn(relay_parent, &validation_data).await { Some(collation) => collation.into_inner(), None => { gum::debug!( @@ -331,6 +358,82 @@ async fn handle_new_activations( Ok(()) } +#[overseer::contextbounds(CollationGeneration, prefix = self::overseer)] +async fn handle_submit_collation( + params: SubmitCollationParams, + config: &CollationGenerationConfig, + ctx: &mut Context, + sender: mpsc::Sender, + metrics: &Metrics, +) -> crate::error::Result<()> { + let _timer = metrics.time_submit_collation(); + + let SubmitCollationParams { + relay_parent, + collation, + parent_head, + validation_code_hash_hint: code_hint, + result_sender, + } = params; + + let validators = request_validators(relay_parent, ctx.sender()).await.await??; + let n_validators = validators.len(); + + // We need to swap the parent-head data, but all other fields here will be correct. + let mut validation_data = match request_persisted_validation_data( + relay_parent, + config.para_id, + OccupiedCoreAssumption::TimedOut, + ctx.sender(), + ) + .await + .await?? + { + Some(v) => v, + None => { + gum::debug!( + target: LOG_TARGET, + relay_parent = ?relay_parent, + our_para = %config.para_id, + "No validation data for para - does it exist at this relay-parent?", + ); + return Ok(()); + }, + }; + + validation_data.parent_head = parent_head; + + let validation_code_hash = match obtain_validation_code_hash_with_hint( + relay_parent, + config.para_id, + code_hint, + &validation_data.parent_head, + ctx.sender(), + ).await? { + None => return Ok(()), + Some(v) => v, + }; + + let collation = PreparedCollation { + collation, + relay_parent, + para_id: config.para_id, + validation_data, + validation_code_hash, + n_validators, + }; + + construct_and_distribute_receipt( + collation, + config.key.clone(), + sender.clone(), + result_sender, + metrics, + ).await; + + Ok(()) +} + struct PreparedCollation { collation: Collation, para_id: ParaId, @@ -465,14 +568,73 @@ async fn construct_and_distribute_receipt( } } -async fn obtain_current_validation_code_hash( +async fn obtain_validation_code_hash_with_hint( relay_parent: Hash, para_id: ParaId, - assumption: OccupiedCoreAssumption, + hint: Option, + parent_head: &HeadData, sender: &mut impl overseer::CollationGenerationSenderTrait, -) -> Result, crate::error::Error> { - use polkadot_node_subsystem::RuntimeApiError; +) -> crate::error::Result> { + let parent_rp_number = match hint { + Some(ValidationCodeHashHint::Provided(hash)) => return Ok(Some(hash)), + Some(ValidationCodeHashHint::ParentBlockRelayParentNumber(n)) => Some(n), + None => None, + }; + let maybe_backing_state = { + let (tx, rx) = oneshot::channel(); + sender.send_message(RuntimeApiMessage::Request( + relay_parent, + RuntimeApiRequest::StagingParaBackingState(para_id, tx), + )) + .await; + + // Failure here means the runtime API doesn't exist. + match rx.await? { + Ok(Some(state)) => Some(state), + Err(RuntimeApiError::NotSupported { .. }) => None, + Ok(None) => return Ok(None), + Err(e) => return Err(e.into()), + } + }; + + if let Some(BackingState { constraints, pending_availability }) = maybe_backing_state { + let (future_trigger, future_code_hash) = match constraints.future_validation_code { + Some(x) => x, + None => return Ok(Some(constraints.validation_code_hash)), + }; + + // If not explicitly provided, search for the parent's relay-parent number in the + // set of candidates pending availability on-chain. + let parent_rp_number = parent_rp_number.or_else(|| { + pending_availability.iter().find(|c| &c.commitments.head_data == parent_head) + .map(|c| c.relay_parent_number) + }); + + if parent_rp_number.map_or(false, |n| n >= future_trigger) { + Ok(Some(future_code_hash)) + } else { + Ok(Some(constraints.validation_code_hash)) + } + } else { + // This branch implies that asynchronous backing has not yet been enabled, + // so in that case cores can only be built on when free anyway. + let maybe_hash = request_validation_code_hash( + relay_parent, + para_id, + OccupiedCoreAssumption::Free, + sender, + ).await.await??; + Ok(maybe_hash) + } +} + +async fn obtain_validation_code_hash_with_assumption( + relay_parent: Hash, + para_id: ParaId, + assumption: OccupiedCoreAssumption, + sender: &mut impl overseer::CollationGenerationSenderTrait, +) -> crate::error::Result> { match request_validation_code_hash(relay_parent, para_id, assumption, sender) .await .await? diff --git a/node/collation-generation/src/metrics.rs b/node/collation-generation/src/metrics.rs index cb9e4a0c8e85..b6d4c1d7c70e 100644 --- a/node/collation-generation/src/metrics.rs +++ b/node/collation-generation/src/metrics.rs @@ -22,6 +22,7 @@ pub(crate) struct MetricsInner { pub(crate) new_activations_overall: prometheus::Histogram, pub(crate) new_activations_per_relay_parent: prometheus::Histogram, pub(crate) new_activations_per_availability_core: prometheus::Histogram, + pub(crate) submit_collation: prometheus::Histogram, } /// `CollationGenerationSubsystem` metrics. @@ -57,6 +58,15 @@ impl Metrics { .as_ref() .map(|metrics| metrics.new_activations_per_availability_core.start_timer()) } + + /// Provide a timer for submitting a collation which updates on drop. + pub fn time_submit_collation( + &self, + ) -> Option { + self.0 + .as_ref() + .map(|metrics| metrics.submit_collation.start_timer()) + } } impl metrics::Metrics for Metrics { @@ -96,6 +106,15 @@ impl metrics::Metrics for Metrics { )?, registry, )?, + submit_collation: prometheus::register( + prometheus::Histogram::with_opts( + prometheus::HistogramOpts::new( + "polkadot_parachain_collation_generation_submit_collation", + "Time spent preparing and submitting a collation to the network protocol", + ) + )?, + registry, + )?, }; Ok(Metrics(Some(metrics))) } diff --git a/node/primitives/src/lib.rs b/node/primitives/src/lib.rs index b9aa449188e9..86e4b528468f 100644 --- a/node/primitives/src/lib.rs +++ b/node/primitives/src/lib.rs @@ -33,7 +33,7 @@ use polkadot_primitives::{ BlakeTwo256, BlockNumber, CandidateCommitments, CandidateHash, CollatorPair, CommittedCandidateReceipt, CompactStatement, EncodeAs, Hash, HashT, HeadData, Id as ParaId, PersistedValidationData, SessionIndex, Signed, UncheckedSigned, ValidationCode, ValidatorIndex, - MAX_CODE_SIZE, MAX_POV_SIZE, + MAX_CODE_SIZE, MAX_POV_SIZE, ValidationCodeHash, }; pub use sp_consensus_babe::{ AllowedSlots as BabeAllowedSlots, BabeEpochConfiguration, Epoch as BabeEpoch, @@ -380,6 +380,18 @@ pub enum MaybeCompressedPoV { Compressed(PoV), } +#[cfg(not(target_os = "unknown"))] +impl std::fmt::Debug for MaybeCompressedPoV { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + let (variant, size) = match self { + MaybeCompressedPoV::Raw(pov) => ("Raw", pov.block_data.0.len()), + MaybeCompressedPoV::Compressed(pov) => ("Compressed", pov.block_data.0.len()), + }; + + write!(f, "{} PoV ({} bytes)", variant, size) + } +} + #[cfg(not(target_os = "unknown"))] impl MaybeCompressedPoV { /// Convert into a compressed [`PoV`]. @@ -399,7 +411,7 @@ impl MaybeCompressedPoV { /// /// - does not contain the erasure root; that's computed at the Polkadot level, not at Cumulus /// - contains a proof of validity. -#[derive(Clone, Encode, Decode)] +#[derive(Debug, Clone, Encode, Decode)] #[cfg(not(target_os = "unknown"))] pub struct Collation { /// Messages destined to be interpreted by the Relay chain itself. @@ -475,7 +487,10 @@ pub struct CollationGenerationConfig { /// Collator's authentication key, so it can sign things. pub key: CollatorPair, /// Collation function. See [`CollatorFn`] for more details. - pub collator: CollatorFn, + /// + /// If this is `None`, it implies that collations are intended to be submitted + /// out-of-band and not pulled out of the function. + pub collator: Option, /// The parachain that this collator collates for pub para_id: ParaId, } @@ -487,6 +502,41 @@ impl std::fmt::Debug for CollationGenerationConfig { } } +/// Hints for the collation-generation subsystem to determine the correct +/// validation code hash to put into the candidate receipt. +/// +/// Providing an incorrect hint or no hint at all could lead the candidate to have the +/// wrong code provided, but only in the few blocks surrounding validation code upgrades. +/// such a candidate receipt would be rejected by validators. +#[derive(Debug, Clone)] +pub enum ValidationCodeHashHint { + /// Contains the number of the relay-chain block used as a relay-parent + /// for the collation's parent block. + ParentBlockRelayParentNumber(BlockNumber), + /// Contains explicitly the validation code hash to use in the candidate receipt. + Provided(ValidationCodeHash), +} + +/// Parameters for [`CollationGenerationMessage::SubmitCollation`]. +#[derive(Debug)] +pub struct SubmitCollationParams { + /// The relay-parent the collation is built against. + pub relay_parent: Hash, + /// The collation itself (PoV and commitments) + pub collation: Collation, + /// The parent block's head-data. + pub parent_head: HeadData, + /// The validation code hash hint. If no hint is provided, a best effort will be made + /// against the relay-parent's state. + pub validation_code_hash_hint: Option, + /// An optional result sender that should be informed about a successfully seconded collation. + /// + /// There is no guarantee that this sender is informed ever about any result, it is completely okay to just drop it. + /// However, if it is called, it should be called with the signed statement of a parachain validator seconding the + /// collation. + pub result_sender: Option>, +} + /// This is the data we keep available for each candidate included in the relay chain. #[derive(Clone, Encode, Decode, PartialEq, Eq, Debug)] pub struct AvailableData { diff --git a/node/subsystem-types/src/messages.rs b/node/subsystem-types/src/messages.rs index 68e953b9dade..2293ce57a36f 100644 --- a/node/subsystem-types/src/messages.rs +++ b/node/subsystem-types/src/messages.rs @@ -37,6 +37,7 @@ use polkadot_node_primitives::{ AvailableData, BabeEpoch, BlockWeight, CandidateVotes, CollationGenerationConfig, CollationSecondedSignal, DisputeMessage, DisputeStatus, ErasureChunk, PoV, SignedDisputeStatement, SignedFullStatement, SignedFullStatementWithPVD, ValidationResult, + SubmitCollationParams, }; use polkadot_primitives::{ vstaging as vstaging_primitives, AuthorityDiscoveryId, BackedCandidate, BlockNumber, @@ -736,6 +737,11 @@ pub enum ProvisionerMessage { pub enum CollationGenerationMessage { /// Initialize the collation generation subsystem Initialize(CollationGenerationConfig), + /// Submit a collation to the subsystem. This will package it into a signed + /// [`CommittedCandidateReceipt`] and distribute along the network to validators. + /// + /// If sent before `Initialize`, this will be ignored. + SubmitCollation(SubmitCollationParams), } /// The result type of [`ApprovalVotingMessage::CheckAndImportAssignment`] request. diff --git a/parachain/src/primitives.rs b/parachain/src/primitives.rs index 224591d6048d..ce9daffb1175 100644 --- a/parachain/src/primitives.rs +++ b/parachain/src/primitives.rs @@ -63,7 +63,8 @@ impl ValidationCode { } } -/// Unit type wrapper around [`type@Hash`] that represents a validation code hash. +/// Unit type wrapper around [`type@Hash`] that represents the blake2-256 hash +/// of validation code in particular. /// /// This type is produced by [`ValidationCode::hash`]. /// diff --git a/roadmap/implementers-guide/src/node/collators/collation-generation.md b/roadmap/implementers-guide/src/node/collators/collation-generation.md index 2f0d4742496d..1fba9080c59d 100644 --- a/roadmap/implementers-guide/src/node/collators/collation-generation.md +++ b/roadmap/implementers-guide/src/node/collators/collation-generation.md @@ -9,7 +9,7 @@ Collation generation for Parachains currently works in the following way: 1. A new relay chain block is imported. 2. The collation generation subsystem checks if the core associated to the parachain is free and if yes, continues. -3. Collation generation calls our collator callback to generate a PoV. +3. Collation generation calls our collator callback, if present, to generate a PoV. If none exists, do nothing. 4. Authoring logic determines if the current node should build a PoV. 5. Build new PoV and give it back to collation generation. @@ -25,6 +25,13 @@ Collation generation for Parachains currently works in the following way: - No more than one initialization message should ever be sent to the collation generation subsystem. - Sent by a collator to initialize this subsystem. +- `CollationGenerationMessage::SubmitCollation` + - If the subsystem isn't initialized or the relay-parent is too old to be relevant, ignore the message. + - Otherwise, use the provided parameters to generate a [`CommittedCandidateReceipt`] + - If no `ValidationCodeHashHint` is given, use the current validation code hash from the relay-chain state, unless the parent-head of the collation equals the block pending availability for the parachain. In that case, use the pending future code if that block would trigger a code upgrade. + - If `ValidationCodeHashHint::Provided` is given, use that. + - If `ValidationCodeHashHint::ParentBlockRelayParentNumber` is given, determine whether the given parent block's relay parent number would trigger a code upgrade. If so, use the future code. If not, use the current code. + - Submit the collation to the collator-protocol with `CollatorProtocolMessage::DistributeCollation`. ### Outgoing @@ -101,7 +108,8 @@ pub struct CollationGenerationConfig { /// Collator's authentication key, so it can sign things. pub key: CollatorPair, /// Collation function. See [`CollatorFn`] for more details. - pub collator: CollatorFn, + /// + pub collator: Option, /// The parachain that this collator collates for pub para_id: ParaId, } @@ -136,7 +144,7 @@ The configuration should be optional, to allow for the case where the node is no - **Collation generation config** - - Contains collator's authentication key, collator function, and + - Contains collator's authentication key, optional collator function, and parachain ID. [CP]: collator-protocol.md diff --git a/roadmap/implementers-guide/src/types/overseer-protocol.md b/roadmap/implementers-guide/src/types/overseer-protocol.md index 995f53d99c2b..a9f1ffb34fc6 100644 --- a/roadmap/implementers-guide/src/types/overseer-protocol.md +++ b/roadmap/implementers-guide/src/types/overseer-protocol.md @@ -428,16 +428,44 @@ struct CollationGenerationConfig { para_id: ParaId, } +/// Hints for the collation-generation subsystem to determine the correct +/// validation code hash to put into the candidate receipt. +/// +/// Providing an incorrect hint or no hint at all could lead the candidate to have the +/// wrong code provided, but only in the few blocks surrounding validation code upgrades. +/// such a candidate receipt would be rejected by validators. +enum ValidationCodeHashHint { + /// Contains the number of the relay-chain block used as a relay-parent + /// for the collation's parent block. + ParentBlockRelayParentNumber(BlockNumber), + /// Contains explicitly the validation code hash to use in the candidate receipt. + Provided(ValidationCodeHash), +} + +/// Parameters for submitting a +struct SubmitCollationParams { + /// The relay-parent the collation is built against. + relay_parent: Hash, + /// The collation itself (PoV and commitments) + collation: Collation, + /// The parent block's head-data. + parent_head: HeadData, + /// The validation code hash hint. If no hint is provided, a best effort will be made + /// against the relay-parent's state. + validation_code_hash: Option, + /// A response channel for receiving a `Seconded` message about the candidate + /// once produced by a validator. This is not guaranteed to provide anything. + result_sender: Option>, +} + enum CollationGenerationMessage { /// Initialize the collation generation subsystem Initialize(CollationGenerationConfig), /// Submit a collation to the subsystem. This will package it into a signed /// [`CommittedCandidateReceipt`] and distribute along the network to validators. - SubmitCollation( - Collation, - Hash, // Relay Parent - HeadData, // Parent Head - ), + /// + /// If sent before `Initialize`, this will be ignored. + SubmitCollation(SubmitCollationParams), } ``` From 91993fd4383f1d2dafaa4677f857cc45acf3c639 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Wed, 21 Jun 2023 19:13:39 -0700 Subject: [PATCH 04/19] fmt --- node/collation-generation/src/lib.rs | 105 +++++++++++++---------- node/collation-generation/src/metrics.rs | 8 +- node/primitives/src/lib.rs | 20 ++--- node/subsystem-types/src/messages.rs | 4 +- 4 files changed, 75 insertions(+), 62 deletions(-) diff --git a/node/collation-generation/src/lib.rs b/node/collation-generation/src/lib.rs index 883fb05de068..8789a0e573be 100644 --- a/node/collation-generation/src/lib.rs +++ b/node/collation-generation/src/lib.rs @@ -29,23 +29,33 @@ #![deny(missing_docs)] -use futures::{channel::{mpsc, oneshot}, future::FutureExt, join, select, sink::SinkExt, stream::StreamExt}; +use futures::{ + channel::{mpsc, oneshot}, + future::FutureExt, + join, select, + sink::SinkExt, + stream::StreamExt, +}; use parity_scale_codec::Encode; -use polkadot_node_primitives::{AvailableData, Collation, CollationGenerationConfig, CollationSecondedSignal, PoV, SubmitCollationParams, ValidationCodeHashHint}; +use polkadot_node_primitives::{ + AvailableData, Collation, CollationGenerationConfig, CollationSecondedSignal, PoV, + SubmitCollationParams, ValidationCodeHashHint, +}; use polkadot_node_subsystem::{ - messages::{CollationGenerationMessage, CollatorProtocolMessage, RuntimeApiMessage, RuntimeApiRequest}, - overseer, ActiveLeavesUpdate, FromOrchestra, OverseerSignal, SpawnedSubsystem, - SubsystemContext, SubsystemError, SubsystemResult, RuntimeApiError, + messages::{ + CollationGenerationMessage, CollatorProtocolMessage, RuntimeApiMessage, RuntimeApiRequest, + }, + overseer, ActiveLeavesUpdate, FromOrchestra, OverseerSignal, RuntimeApiError, SpawnedSubsystem, + SubsystemContext, SubsystemError, SubsystemResult, }; use polkadot_node_subsystem_util::{ request_availability_cores, request_persisted_validation_data, request_validation_code, request_validation_code_hash, request_validators, }; use polkadot_primitives::{ - vstaging::BackingState, - collator_signature_payload, CandidateCommitments, CandidateDescriptor, CandidateReceipt, - CoreState, Hash, Id as ParaId, OccupiedCoreAssumption, PersistedValidationData, - ValidationCodeHash, CollatorPair, HeadData, + collator_signature_payload, vstaging::BackingState, CandidateCommitments, CandidateDescriptor, + CandidateReceipt, CollatorPair, CoreState, Hash, HeadData, Id as ParaId, + OccupiedCoreAssumption, PersistedValidationData, ValidationCodeHash, }; use sp_core::crypto::Pair; use std::sync::Arc; @@ -157,13 +167,10 @@ impl CollationGenerationSubsystem { msg: CollationGenerationMessage::SubmitCollation(params), }) => { if let Some(config) = &self.config { - if let Err(err) = handle_submit_collation( - params, - config, - ctx, - sender.clone(), - &self.metrics, - ).await { + if let Err(err) = + handle_submit_collation(params, config, ctx, sender.clone(), &self.metrics) + .await + { gum::error!(target: LOG_TARGET, ?err, "Failed to submit collation"); } } else { @@ -171,7 +178,7 @@ impl CollationGenerationSubsystem { } false - } + }, Ok(FromOrchestra::Signal(OverseerSignal::BlockFinalized(..))) => false, Err(err) => { gum::error!( @@ -210,7 +217,9 @@ async fn handle_new_activations( // follow the procedure from the guide: // https://paritytech.github.io/polkadot/book/node/collators/collation-generation.html - if config.collator.is_none() { return Ok(()) } + if config.collator.is_none() { + return Ok(()) + } let _overall_timer = metrics.time_new_activations(); @@ -349,8 +358,9 @@ async fn handle_new_activations( task_sender, result_sender, &metrics, - ).await; - }) + ) + .await; + }), )?; } } @@ -397,7 +407,7 @@ async fn handle_submit_collation( our_para = %config.para_id, "No validation data for para - does it exist at this relay-parent?", ); - return Ok(()); + return Ok(()) }, }; @@ -409,7 +419,9 @@ async fn handle_submit_collation( code_hint, &validation_data.parent_head, ctx.sender(), - ).await? { + ) + .await? + { None => return Ok(()), Some(v) => v, }; @@ -429,7 +441,8 @@ async fn handle_submit_collation( sender.clone(), result_sender, metrics, - ).await; + ) + .await; Ok(()) } @@ -499,19 +512,18 @@ async fn construct_and_distribute_receipt( &validation_code_hash, ); - let erasure_root = - match erasure_root(n_validators, validation_data, pov.clone()) { - Ok(erasure_root) => erasure_root, - Err(err) => { - gum::error!( - target: LOG_TARGET, - para_id = %para_id, - err = ?err, - "failed to calculate erasure root", - ); - return - }, - }; + let erasure_root = match erasure_root(n_validators, validation_data, pov.clone()) { + Ok(erasure_root) => erasure_root, + Err(err) => { + gum::error!( + target: LOG_TARGET, + para_id = %para_id, + err = ?err, + "failed to calculate erasure root", + ); + return + }, + }; let commitments = CandidateCommitments { upward_messages: collation.upward_messages, @@ -526,7 +538,7 @@ async fn construct_and_distribute_receipt( commitments_hash: commitments.hash(), descriptor: CandidateDescriptor { signature: key.sign(&signature_payload), - para_id: para_id, + para_id, relay_parent, collator: key.public(), persisted_validation_data_hash, @@ -583,11 +595,12 @@ async fn obtain_validation_code_hash_with_hint( let maybe_backing_state = { let (tx, rx) = oneshot::channel(); - sender.send_message(RuntimeApiMessage::Request( - relay_parent, - RuntimeApiRequest::StagingParaBackingState(para_id, tx), - )) - .await; + sender + .send_message(RuntimeApiMessage::Request( + relay_parent, + RuntimeApiRequest::StagingParaBackingState(para_id, tx), + )) + .await; // Failure here means the runtime API doesn't exist. match rx.await? { @@ -607,7 +620,9 @@ async fn obtain_validation_code_hash_with_hint( // If not explicitly provided, search for the parent's relay-parent number in the // set of candidates pending availability on-chain. let parent_rp_number = parent_rp_number.or_else(|| { - pending_availability.iter().find(|c| &c.commitments.head_data == parent_head) + pending_availability + .iter() + .find(|c| &c.commitments.head_data == parent_head) .map(|c| c.relay_parent_number) }); @@ -624,7 +639,9 @@ async fn obtain_validation_code_hash_with_hint( para_id, OccupiedCoreAssumption::Free, sender, - ).await.await??; + ) + .await + .await??; Ok(maybe_hash) } } diff --git a/node/collation-generation/src/metrics.rs b/node/collation-generation/src/metrics.rs index b6d4c1d7c70e..c7690ec82c4f 100644 --- a/node/collation-generation/src/metrics.rs +++ b/node/collation-generation/src/metrics.rs @@ -60,12 +60,8 @@ impl Metrics { } /// Provide a timer for submitting a collation which updates on drop. - pub fn time_submit_collation( - &self, - ) -> Option { - self.0 - .as_ref() - .map(|metrics| metrics.submit_collation.start_timer()) + pub fn time_submit_collation(&self) -> Option { + self.0.as_ref().map(|metrics| metrics.submit_collation.start_timer()) } } diff --git a/node/primitives/src/lib.rs b/node/primitives/src/lib.rs index 86e4b528468f..d94dace48d2a 100644 --- a/node/primitives/src/lib.rs +++ b/node/primitives/src/lib.rs @@ -32,8 +32,8 @@ use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; use polkadot_primitives::{ BlakeTwo256, BlockNumber, CandidateCommitments, CandidateHash, CollatorPair, CommittedCandidateReceipt, CompactStatement, EncodeAs, Hash, HashT, HeadData, Id as ParaId, - PersistedValidationData, SessionIndex, Signed, UncheckedSigned, ValidationCode, ValidatorIndex, - MAX_CODE_SIZE, MAX_POV_SIZE, ValidationCodeHash, + PersistedValidationData, SessionIndex, Signed, UncheckedSigned, ValidationCode, + ValidationCodeHash, ValidatorIndex, MAX_CODE_SIZE, MAX_POV_SIZE, }; pub use sp_consensus_babe::{ AllowedSlots as BabeAllowedSlots, BabeEpochConfiguration, Epoch as BabeEpoch, @@ -510,11 +510,11 @@ impl std::fmt::Debug for CollationGenerationConfig { /// such a candidate receipt would be rejected by validators. #[derive(Debug, Clone)] pub enum ValidationCodeHashHint { - /// Contains the number of the relay-chain block used as a relay-parent - /// for the collation's parent block. - ParentBlockRelayParentNumber(BlockNumber), - /// Contains explicitly the validation code hash to use in the candidate receipt. - Provided(ValidationCodeHash), + /// Contains the number of the relay-chain block used as a relay-parent + /// for the collation's parent block. + ParentBlockRelayParentNumber(BlockNumber), + /// Contains explicitly the validation code hash to use in the candidate receipt. + Provided(ValidationCodeHash), } /// Parameters for [`CollationGenerationMessage::SubmitCollation`]. @@ -526,9 +526,9 @@ pub struct SubmitCollationParams { pub collation: Collation, /// The parent block's head-data. pub parent_head: HeadData, - /// The validation code hash hint. If no hint is provided, a best effort will be made - /// against the relay-parent's state. - pub validation_code_hash_hint: Option, + /// The validation code hash hint. If no hint is provided, a best effort will be made + /// against the relay-parent's state. + pub validation_code_hash_hint: Option, /// An optional result sender that should be informed about a successfully seconded collation. /// /// There is no guarantee that this sender is informed ever about any result, it is completely okay to just drop it. diff --git a/node/subsystem-types/src/messages.rs b/node/subsystem-types/src/messages.rs index 2293ce57a36f..83c9613aa178 100644 --- a/node/subsystem-types/src/messages.rs +++ b/node/subsystem-types/src/messages.rs @@ -36,8 +36,8 @@ use polkadot_node_primitives::{ approval::{BlockApprovalMeta, IndirectAssignmentCert, IndirectSignedApprovalVote}, AvailableData, BabeEpoch, BlockWeight, CandidateVotes, CollationGenerationConfig, CollationSecondedSignal, DisputeMessage, DisputeStatus, ErasureChunk, PoV, - SignedDisputeStatement, SignedFullStatement, SignedFullStatementWithPVD, ValidationResult, - SubmitCollationParams, + SignedDisputeStatement, SignedFullStatement, SignedFullStatementWithPVD, SubmitCollationParams, + ValidationResult, }; use polkadot_primitives::{ vstaging as vstaging_primitives, AuthorityDiscoveryId, BackedCandidate, BlockNumber, From 664a83878641b5c08ffb9cff0a3b29ba52eeb9ab Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Wed, 21 Jun 2023 19:21:50 -0700 Subject: [PATCH 05/19] fix test compilation & remove unnecessary submodule --- node/collation-generation/src/tests.rs | 847 ++++++++++++------------- 1 file changed, 412 insertions(+), 435 deletions(-) diff --git a/node/collation-generation/src/tests.rs b/node/collation-generation/src/tests.rs index b2534bcf36c1..86adc24a6eb6 100644 --- a/node/collation-generation/src/tests.rs +++ b/node/collation-generation/src/tests.rs @@ -14,472 +14,449 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . -mod handle_new_activations { - use super::super::*; - use ::test_helpers::{dummy_hash, dummy_head_data, dummy_validator}; - use futures::{ - lock::Mutex, - task::{Context as FuturesContext, Poll}, - Future, - }; - use polkadot_node_primitives::{ - BlockData, Collation, CollationResult, MaybeCompressedPoV, PoV, - }; - use polkadot_node_subsystem::{ - errors::RuntimeApiError, - messages::{AllMessages, RuntimeApiMessage, RuntimeApiRequest}, - }; - use polkadot_node_subsystem_test_helpers::{ - subsystem_test_harness, TestSubsystemContextHandle, - }; - use polkadot_primitives::{ - CollatorPair, Id as ParaId, PersistedValidationData, ScheduledCore, ValidationCode, - }; - use std::pin::Pin; - - fn test_collation() -> Collation { - Collation { - upward_messages: Default::default(), - horizontal_messages: Default::default(), - new_validation_code: None, - head_data: dummy_head_data(), - proof_of_validity: MaybeCompressedPoV::Raw(PoV { block_data: BlockData(Vec::new()) }), - processed_downward_messages: 0_u32, - hrmp_watermark: 0_u32.into(), - } +use super::*; +use test_helpers::{dummy_hash, dummy_head_data, dummy_validator}; +use futures::{ + lock::Mutex, + task::{Context as FuturesContext, Poll}, + Future, +}; +use polkadot_node_primitives::{BlockData, Collation, CollationResult, MaybeCompressedPoV, PoV}; +use polkadot_node_subsystem::{ + errors::RuntimeApiError, + messages::{AllMessages, RuntimeApiMessage, RuntimeApiRequest}, +}; +use polkadot_node_subsystem_test_helpers::{subsystem_test_harness, TestSubsystemContextHandle}; +use polkadot_primitives::{ + CollatorPair, Id as ParaId, PersistedValidationData, ScheduledCore, ValidationCode, +}; +use std::pin::Pin; + +fn test_collation() -> Collation { + Collation { + upward_messages: Default::default(), + horizontal_messages: Default::default(), + new_validation_code: None, + head_data: dummy_head_data(), + proof_of_validity: MaybeCompressedPoV::Raw(PoV { block_data: BlockData(Vec::new()) }), + processed_downward_messages: 0_u32, + hrmp_watermark: 0_u32.into(), } +} - fn test_collation_compressed() -> Collation { - let mut collation = test_collation(); - let compressed = collation.proof_of_validity.clone().into_compressed(); - collation.proof_of_validity = MaybeCompressedPoV::Compressed(compressed); - collation - } +fn test_collation_compressed() -> Collation { + let mut collation = test_collation(); + let compressed = collation.proof_of_validity.clone().into_compressed(); + collation.proof_of_validity = MaybeCompressedPoV::Compressed(compressed); + collation +} - fn test_validation_data() -> PersistedValidationData { - let mut persisted_validation_data = PersistedValidationData::default(); - persisted_validation_data.max_pov_size = 1024; - persisted_validation_data - } +fn test_validation_data() -> PersistedValidationData { + let mut persisted_validation_data = PersistedValidationData::default(); + persisted_validation_data.max_pov_size = 1024; + persisted_validation_data +} - // Box + Unpin + Send - struct TestCollator; +// Box + Unpin + Send +struct TestCollator; - impl Future for TestCollator { - type Output = Option; +impl Future for TestCollator { + type Output = Option; - fn poll(self: Pin<&mut Self>, _cx: &mut FuturesContext) -> Poll { - Poll::Ready(Some(CollationResult { collation: test_collation(), result_sender: None })) - } + fn poll(self: Pin<&mut Self>, _cx: &mut FuturesContext) -> Poll { + Poll::Ready(Some(CollationResult { collation: test_collation(), result_sender: None })) } +} - impl Unpin for TestCollator {} +impl Unpin for TestCollator {} - fn test_config>(para_id: Id) -> Arc { - Arc::new(CollationGenerationConfig { - key: CollatorPair::generate().0, - collator: Box::new(|_: Hash, _vd: &PersistedValidationData| TestCollator.boxed()), - para_id: para_id.into(), - }) - } +fn test_config>(para_id: Id) -> Arc { + Arc::new(CollationGenerationConfig { + key: CollatorPair::generate().0, + collator: Some(Box::new(|_: Hash, _vd: &PersistedValidationData| TestCollator.boxed())), + para_id: para_id.into(), + }) +} - fn scheduled_core_for>(para_id: Id) -> ScheduledCore { - ScheduledCore { para_id: para_id.into(), collator: None } - } +fn scheduled_core_for>(para_id: Id) -> ScheduledCore { + ScheduledCore { para_id: para_id.into(), collator: None } +} - #[test] - fn requests_availability_per_relay_parent() { - let activated_hashes: Vec = - vec![[1; 32].into(), [4; 32].into(), [9; 32].into(), [16; 32].into()]; - - let requested_availability_cores = Arc::new(Mutex::new(Vec::new())); - - let overseer_requested_availability_cores = requested_availability_cores.clone(); - let overseer = |mut handle: TestSubsystemContextHandle| async move { - loop { - match handle.try_recv().await { - None => break, - Some(AllMessages::RuntimeApi(RuntimeApiMessage::Request(hash, RuntimeApiRequest::AvailabilityCores(tx)))) => { - overseer_requested_availability_cores.lock().await.push(hash); - tx.send(Ok(vec![])).unwrap(); - } - Some(AllMessages::RuntimeApi(RuntimeApiMessage::Request(_hash, RuntimeApiRequest::Validators(tx)))) => { - tx.send(Ok(vec![dummy_validator(); 3])).unwrap(); - } - Some(msg) => panic!("didn't expect any other overseer requests given no availability cores; got {:?}", msg), +#[test] +fn requests_availability_per_relay_parent() { + let activated_hashes: Vec = + vec![[1; 32].into(), [4; 32].into(), [9; 32].into(), [16; 32].into()]; + + let requested_availability_cores = Arc::new(Mutex::new(Vec::new())); + + let overseer_requested_availability_cores = requested_availability_cores.clone(); + let overseer = |mut handle: TestSubsystemContextHandle| async move { + loop { + match handle.try_recv().await { + None => break, + Some(AllMessages::RuntimeApi(RuntimeApiMessage::Request(hash, RuntimeApiRequest::AvailabilityCores(tx)))) => { + overseer_requested_availability_cores.lock().await.push(hash); + tx.send(Ok(vec![])).unwrap(); } + Some(AllMessages::RuntimeApi(RuntimeApiMessage::Request(_hash, RuntimeApiRequest::Validators(tx)))) => { + tx.send(Ok(vec![dummy_validator(); 3])).unwrap(); + } + Some(msg) => panic!("didn't expect any other overseer requests given no availability cores; got {:?}", msg), + } + } + }; + + let (tx, _rx) = mpsc::channel(0); + + let subsystem_activated_hashes = activated_hashes.clone(); + subsystem_test_harness(overseer, |mut ctx| async move { + handle_new_activations( + test_config(123u32), + subsystem_activated_hashes, + &mut ctx, + Metrics(None), + &tx, + ) + .await + .unwrap(); + }); + + let mut requested_availability_cores = Arc::try_unwrap(requested_availability_cores) + .expect("overseer should have shut down by now") + .into_inner(); + requested_availability_cores.sort(); + + assert_eq!(requested_availability_cores, activated_hashes); +} + +#[test] +fn requests_validation_data_for_scheduled_matches() { + let activated_hashes: Vec = vec![ + Hash::repeat_byte(1), + Hash::repeat_byte(4), + Hash::repeat_byte(9), + Hash::repeat_byte(16), + ]; + + let requested_validation_data = Arc::new(Mutex::new(Vec::new())); + + let overseer_requested_validation_data = requested_validation_data.clone(); + let overseer = |mut handle: TestSubsystemContextHandle| async move { + loop { + match handle.try_recv().await { + None => break, + Some(AllMessages::RuntimeApi(RuntimeApiMessage::Request( + hash, + RuntimeApiRequest::AvailabilityCores(tx), + ))) => { + tx.send(Ok(vec![ + CoreState::Free, + // this is weird, see explanation below + CoreState::Scheduled(scheduled_core_for( + (hash.as_fixed_bytes()[0] * 4) as u32, + )), + CoreState::Scheduled(scheduled_core_for( + (hash.as_fixed_bytes()[0] * 5) as u32, + )), + ])) + .unwrap(); + }, + Some(AllMessages::RuntimeApi(RuntimeApiMessage::Request( + hash, + RuntimeApiRequest::PersistedValidationData( + _para_id, + _occupied_core_assumption, + tx, + ), + ))) => { + overseer_requested_validation_data.lock().await.push(hash); + tx.send(Ok(None)).unwrap(); + }, + Some(AllMessages::RuntimeApi(RuntimeApiMessage::Request( + _hash, + RuntimeApiRequest::Validators(tx), + ))) => { + tx.send(Ok(vec![dummy_validator(); 3])).unwrap(); + }, + Some(msg) => { + panic!("didn't expect any other overseer requests; got {:?}", msg) + }, } - }; - - let (tx, _rx) = mpsc::channel(0); - - let subsystem_activated_hashes = activated_hashes.clone(); - subsystem_test_harness(overseer, |mut ctx| async move { - handle_new_activations( - test_config(123u32), - subsystem_activated_hashes, - &mut ctx, - Metrics(None), - &tx, - ) + } + }; + + let (tx, _rx) = mpsc::channel(0); + + subsystem_test_harness(overseer, |mut ctx| async move { + handle_new_activations(test_config(16), activated_hashes, &mut ctx, Metrics(None), &tx) .await .unwrap(); - }); + }); - let mut requested_availability_cores = Arc::try_unwrap(requested_availability_cores) - .expect("overseer should have shut down by now") - .into_inner(); - requested_availability_cores.sort(); + let requested_validation_data = Arc::try_unwrap(requested_validation_data) + .expect("overseer should have shut down by now") + .into_inner(); - assert_eq!(requested_availability_cores, activated_hashes); - } + // the only activated hash should be from the 4 hash: + // each activated hash generates two scheduled cores: one with its value * 4, one with its value * 5 + // given that the test configuration has a `para_id` of 16, there's only one way to get that value: with the 4 + // hash. + assert_eq!(requested_validation_data, vec![[4; 32].into()]); +} - #[test] - fn requests_validation_data_for_scheduled_matches() { - let activated_hashes: Vec = vec![ - Hash::repeat_byte(1), - Hash::repeat_byte(4), - Hash::repeat_byte(9), - Hash::repeat_byte(16), - ]; - - let requested_validation_data = Arc::new(Mutex::new(Vec::new())); - - let overseer_requested_validation_data = requested_validation_data.clone(); - let overseer = |mut handle: TestSubsystemContextHandle| async move { - loop { - match handle.try_recv().await { - None => break, - Some(AllMessages::RuntimeApi(RuntimeApiMessage::Request( - hash, - RuntimeApiRequest::AvailabilityCores(tx), - ))) => { - tx.send(Ok(vec![ - CoreState::Free, - // this is weird, see explanation below - CoreState::Scheduled(scheduled_core_for( - (hash.as_fixed_bytes()[0] * 4) as u32, - )), - CoreState::Scheduled(scheduled_core_for( - (hash.as_fixed_bytes()[0] * 5) as u32, - )), - ])) - .unwrap(); - }, - Some(AllMessages::RuntimeApi(RuntimeApiMessage::Request( - hash, - RuntimeApiRequest::PersistedValidationData( - _para_id, - _occupied_core_assumption, - tx, - ), - ))) => { - overseer_requested_validation_data.lock().await.push(hash); - tx.send(Ok(None)).unwrap(); - }, - Some(AllMessages::RuntimeApi(RuntimeApiMessage::Request( - _hash, - RuntimeApiRequest::Validators(tx), - ))) => { - tx.send(Ok(vec![dummy_validator(); 3])).unwrap(); - }, - Some(msg) => { - panic!("didn't expect any other overseer requests; got {:?}", msg) - }, - } +#[test] +fn sends_distribute_collation_message() { + let activated_hashes: Vec = vec![ + Hash::repeat_byte(1), + Hash::repeat_byte(4), + Hash::repeat_byte(9), + Hash::repeat_byte(16), + ]; + + let overseer = |mut handle: TestSubsystemContextHandle| async move { + loop { + match handle.try_recv().await { + None => break, + Some(AllMessages::RuntimeApi(RuntimeApiMessage::Request( + hash, + RuntimeApiRequest::AvailabilityCores(tx), + ))) => { + tx.send(Ok(vec![ + CoreState::Free, + // this is weird, see explanation below + CoreState::Scheduled(scheduled_core_for( + (hash.as_fixed_bytes()[0] * 4) as u32, + )), + CoreState::Scheduled(scheduled_core_for( + (hash.as_fixed_bytes()[0] * 5) as u32, + )), + ])) + .unwrap(); + }, + Some(AllMessages::RuntimeApi(RuntimeApiMessage::Request( + _hash, + RuntimeApiRequest::PersistedValidationData( + _para_id, + _occupied_core_assumption, + tx, + ), + ))) => { + tx.send(Ok(Some(test_validation_data()))).unwrap(); + }, + Some(AllMessages::RuntimeApi(RuntimeApiMessage::Request( + _hash, + RuntimeApiRequest::Validators(tx), + ))) => { + tx.send(Ok(vec![dummy_validator(); 3])).unwrap(); + }, + Some(AllMessages::RuntimeApi(RuntimeApiMessage::Request( + _hash, + RuntimeApiRequest::ValidationCodeHash( + _para_id, + OccupiedCoreAssumption::Free, + tx, + ), + ))) => { + tx.send(Ok(Some(ValidationCode(vec![1, 2, 3]).hash()))).unwrap(); + }, + Some(msg) => { + panic!("didn't expect any other overseer requests; got {:?}", msg) + }, } - }; + } + }; - let (tx, _rx) = mpsc::channel(0); + let config = test_config(16); + let subsystem_config = config.clone(); - subsystem_test_harness(overseer, |mut ctx| async move { - handle_new_activations(test_config(16), activated_hashes, &mut ctx, Metrics(None), &tx) - .await - .unwrap(); - }); + let (tx, rx) = mpsc::channel(0); + + // empty vec doesn't allocate on the heap, so it's ok we throw it away + let sent_messages = Arc::new(Mutex::new(Vec::new())); + let subsystem_sent_messages = sent_messages.clone(); + subsystem_test_harness(overseer, |mut ctx| async move { + handle_new_activations(subsystem_config, activated_hashes, &mut ctx, Metrics(None), &tx) + .await + .unwrap(); - let requested_validation_data = Arc::try_unwrap(requested_validation_data) - .expect("overseer should have shut down by now") - .into_inner(); + std::mem::drop(tx); + + // collect all sent messages + *subsystem_sent_messages.lock().await = rx.collect().await; + }); + + let mut sent_messages = Arc::try_unwrap(sent_messages) + .expect("subsystem should have shut down by now") + .into_inner(); + + // we expect a single message to be sent, containing a candidate receipt. + // we don't care too much about the `commitments_hash` right now, but let's ensure that we've calculated the + // correct descriptor + let expect_pov_hash = test_collation_compressed().proof_of_validity.into_compressed().hash(); + let expect_validation_data_hash = test_validation_data().hash(); + let expect_relay_parent = Hash::repeat_byte(4); + let expect_validation_code_hash = ValidationCode(vec![1, 2, 3]).hash(); + let expect_payload = collator_signature_payload( + &expect_relay_parent, + &config.para_id, + &expect_validation_data_hash, + &expect_pov_hash, + &expect_validation_code_hash, + ); + let expect_descriptor = CandidateDescriptor { + signature: config.key.sign(&expect_payload), + para_id: config.para_id, + relay_parent: expect_relay_parent, + collator: config.key.public(), + persisted_validation_data_hash: expect_validation_data_hash, + pov_hash: expect_pov_hash, + erasure_root: dummy_hash(), // this isn't something we're checking right now + para_head: test_collation().head_data.hash(), + validation_code_hash: expect_validation_code_hash, + }; - // the only activated hash should be from the 4 hash: - // each activated hash generates two scheduled cores: one with its value * 4, one with its value * 5 - // given that the test configuration has a `para_id` of 16, there's only one way to get that value: with the 4 - // hash. - assert_eq!(requested_validation_data, vec![[4; 32].into()]); + assert_eq!(sent_messages.len(), 1); + match AllMessages::from(sent_messages.pop().unwrap()) { + AllMessages::CollatorProtocol(CollatorProtocolMessage::DistributeCollation( + CandidateReceipt { descriptor, .. }, + _pov, + .., + )) => { + // signature generation is non-deterministic, so we can't just assert that the + // expected descriptor is correct. What we can do is validate that the produced + // descriptor has a valid signature, then just copy in the generated signature + // and check the rest of the fields for equality. + assert!(CollatorPair::verify( + &descriptor.signature, + &collator_signature_payload( + &descriptor.relay_parent, + &descriptor.para_id, + &descriptor.persisted_validation_data_hash, + &descriptor.pov_hash, + &descriptor.validation_code_hash, + ) + .as_ref(), + &descriptor.collator, + )); + let expect_descriptor = { + let mut expect_descriptor = expect_descriptor; + expect_descriptor.signature = descriptor.signature.clone(); + expect_descriptor.erasure_root = descriptor.erasure_root.clone(); + expect_descriptor + }; + assert_eq!(descriptor, expect_descriptor); + }, + _ => panic!("received wrong message type"), } +} - #[test] - fn sends_distribute_collation_message() { - let activated_hashes: Vec = vec![ - Hash::repeat_byte(1), - Hash::repeat_byte(4), - Hash::repeat_byte(9), - Hash::repeat_byte(16), - ]; - - let overseer = |mut handle: TestSubsystemContextHandle| async move { - loop { - match handle.try_recv().await { - None => break, - Some(AllMessages::RuntimeApi(RuntimeApiMessage::Request( - hash, - RuntimeApiRequest::AvailabilityCores(tx), - ))) => { - tx.send(Ok(vec![ - CoreState::Free, - // this is weird, see explanation below - CoreState::Scheduled(scheduled_core_for( - (hash.as_fixed_bytes()[0] * 4) as u32, - )), - CoreState::Scheduled(scheduled_core_for( - (hash.as_fixed_bytes()[0] * 5) as u32, - )), - ])) - .unwrap(); - }, - Some(AllMessages::RuntimeApi(RuntimeApiMessage::Request( - _hash, - RuntimeApiRequest::PersistedValidationData( - _para_id, - _occupied_core_assumption, - tx, - ), - ))) => { - tx.send(Ok(Some(test_validation_data()))).unwrap(); - }, - Some(AllMessages::RuntimeApi(RuntimeApiMessage::Request( - _hash, - RuntimeApiRequest::Validators(tx), - ))) => { - tx.send(Ok(vec![dummy_validator(); 3])).unwrap(); - }, - Some(AllMessages::RuntimeApi(RuntimeApiMessage::Request( - _hash, - RuntimeApiRequest::ValidationCodeHash( - _para_id, - OccupiedCoreAssumption::Free, - tx, - ), - ))) => { - tx.send(Ok(Some(ValidationCode(vec![1, 2, 3]).hash()))).unwrap(); - }, - Some(msg) => { - panic!("didn't expect any other overseer requests; got {:?}", msg) - }, - } +#[test] +fn fallback_when_no_validation_code_hash_api() { + // This is a variant of the above test, but with the validation code hash API disabled. + + let activated_hashes: Vec = vec![ + Hash::repeat_byte(1), + Hash::repeat_byte(4), + Hash::repeat_byte(9), + Hash::repeat_byte(16), + ]; + + let overseer = |mut handle: TestSubsystemContextHandle| async move { + loop { + match handle.try_recv().await { + None => break, + Some(AllMessages::RuntimeApi(RuntimeApiMessage::Request( + hash, + RuntimeApiRequest::AvailabilityCores(tx), + ))) => { + tx.send(Ok(vec![ + CoreState::Free, + CoreState::Scheduled(scheduled_core_for( + (hash.as_fixed_bytes()[0] * 4) as u32, + )), + CoreState::Scheduled(scheduled_core_for( + (hash.as_fixed_bytes()[0] * 5) as u32, + )), + ])) + .unwrap(); + }, + Some(AllMessages::RuntimeApi(RuntimeApiMessage::Request( + _hash, + RuntimeApiRequest::PersistedValidationData( + _para_id, + _occupied_core_assumption, + tx, + ), + ))) => { + tx.send(Ok(Some(test_validation_data()))).unwrap(); + }, + Some(AllMessages::RuntimeApi(RuntimeApiMessage::Request( + _hash, + RuntimeApiRequest::Validators(tx), + ))) => { + tx.send(Ok(vec![dummy_validator(); 3])).unwrap(); + }, + Some(AllMessages::RuntimeApi(RuntimeApiMessage::Request( + _hash, + RuntimeApiRequest::ValidationCodeHash( + _para_id, + OccupiedCoreAssumption::Free, + tx, + ), + ))) => { + tx.send(Err(RuntimeApiError::NotSupported { + runtime_api_name: "validation_code_hash", + })) + .unwrap(); + }, + Some(AllMessages::RuntimeApi(RuntimeApiMessage::Request( + _hash, + RuntimeApiRequest::ValidationCode(_para_id, OccupiedCoreAssumption::Free, tx), + ))) => { + tx.send(Ok(Some(ValidationCode(vec![1, 2, 3])))).unwrap(); + }, + Some(msg) => { + panic!("didn't expect any other overseer requests; got {:?}", msg) + }, } - }; - - let config = test_config(16); - let subsystem_config = config.clone(); - - let (tx, rx) = mpsc::channel(0); - - // empty vec doesn't allocate on the heap, so it's ok we throw it away - let sent_messages = Arc::new(Mutex::new(Vec::new())); - let subsystem_sent_messages = sent_messages.clone(); - subsystem_test_harness(overseer, |mut ctx| async move { - handle_new_activations( - subsystem_config, - activated_hashes, - &mut ctx, - Metrics(None), - &tx, - ) + } + }; + + let config = test_config(16u32); + let subsystem_config = config.clone(); + + let (tx, rx) = mpsc::channel(0); + + // empty vec doesn't allocate on the heap, so it's ok we throw it away + let sent_messages = Arc::new(Mutex::new(Vec::new())); + let subsystem_sent_messages = sent_messages.clone(); + subsystem_test_harness(overseer, |mut ctx| async move { + handle_new_activations(subsystem_config, activated_hashes, &mut ctx, Metrics(None), &tx) .await .unwrap(); - std::mem::drop(tx); - - // collect all sent messages - *subsystem_sent_messages.lock().await = rx.collect().await; - }); - - let mut sent_messages = Arc::try_unwrap(sent_messages) - .expect("subsystem should have shut down by now") - .into_inner(); - - // we expect a single message to be sent, containing a candidate receipt. - // we don't care too much about the `commitments_hash` right now, but let's ensure that we've calculated the - // correct descriptor - let expect_pov_hash = - test_collation_compressed().proof_of_validity.into_compressed().hash(); - let expect_validation_data_hash = test_validation_data().hash(); - let expect_relay_parent = Hash::repeat_byte(4); - let expect_validation_code_hash = ValidationCode(vec![1, 2, 3]).hash(); - let expect_payload = collator_signature_payload( - &expect_relay_parent, - &config.para_id, - &expect_validation_data_hash, - &expect_pov_hash, - &expect_validation_code_hash, - ); - let expect_descriptor = CandidateDescriptor { - signature: config.key.sign(&expect_payload), - para_id: config.para_id, - relay_parent: expect_relay_parent, - collator: config.key.public(), - persisted_validation_data_hash: expect_validation_data_hash, - pov_hash: expect_pov_hash, - erasure_root: dummy_hash(), // this isn't something we're checking right now - para_head: test_collation().head_data.hash(), - validation_code_hash: expect_validation_code_hash, - }; - - assert_eq!(sent_messages.len(), 1); - match AllMessages::from(sent_messages.pop().unwrap()) { - AllMessages::CollatorProtocol(CollatorProtocolMessage::DistributeCollation( + std::mem::drop(tx); + + *subsystem_sent_messages.lock().await = rx.collect().await; + }); + + let sent_messages = Arc::try_unwrap(sent_messages) + .expect("subsystem should have shut down by now") + .into_inner(); + + let expect_validation_code_hash = ValidationCode(vec![1, 2, 3]).hash(); + + assert_eq!(sent_messages.len(), 1); + match &sent_messages[0] { + overseer::CollationGenerationOutgoingMessages::CollatorProtocolMessage( + CollatorProtocolMessage::DistributeCollation( CandidateReceipt { descriptor, .. }, _pov, .., - )) => { - // signature generation is non-deterministic, so we can't just assert that the - // expected descriptor is correct. What we can do is validate that the produced - // descriptor has a valid signature, then just copy in the generated signature - // and check the rest of the fields for equality. - assert!(CollatorPair::verify( - &descriptor.signature, - &collator_signature_payload( - &descriptor.relay_parent, - &descriptor.para_id, - &descriptor.persisted_validation_data_hash, - &descriptor.pov_hash, - &descriptor.validation_code_hash, - ) - .as_ref(), - &descriptor.collator, - )); - let expect_descriptor = { - let mut expect_descriptor = expect_descriptor; - expect_descriptor.signature = descriptor.signature.clone(); - expect_descriptor.erasure_root = descriptor.erasure_root.clone(); - expect_descriptor - }; - assert_eq!(descriptor, expect_descriptor); - }, - _ => panic!("received wrong message type"), - } - } - - #[test] - fn fallback_when_no_validation_code_hash_api() { - // This is a variant of the above test, but with the validation code hash API disabled. - - let activated_hashes: Vec = vec![ - Hash::repeat_byte(1), - Hash::repeat_byte(4), - Hash::repeat_byte(9), - Hash::repeat_byte(16), - ]; - - let overseer = |mut handle: TestSubsystemContextHandle| async move { - loop { - match handle.try_recv().await { - None => break, - Some(AllMessages::RuntimeApi(RuntimeApiMessage::Request( - hash, - RuntimeApiRequest::AvailabilityCores(tx), - ))) => { - tx.send(Ok(vec![ - CoreState::Free, - CoreState::Scheduled(scheduled_core_for( - (hash.as_fixed_bytes()[0] * 4) as u32, - )), - CoreState::Scheduled(scheduled_core_for( - (hash.as_fixed_bytes()[0] * 5) as u32, - )), - ])) - .unwrap(); - }, - Some(AllMessages::RuntimeApi(RuntimeApiMessage::Request( - _hash, - RuntimeApiRequest::PersistedValidationData( - _para_id, - _occupied_core_assumption, - tx, - ), - ))) => { - tx.send(Ok(Some(test_validation_data()))).unwrap(); - }, - Some(AllMessages::RuntimeApi(RuntimeApiMessage::Request( - _hash, - RuntimeApiRequest::Validators(tx), - ))) => { - tx.send(Ok(vec![dummy_validator(); 3])).unwrap(); - }, - Some(AllMessages::RuntimeApi(RuntimeApiMessage::Request( - _hash, - RuntimeApiRequest::ValidationCodeHash( - _para_id, - OccupiedCoreAssumption::Free, - tx, - ), - ))) => { - tx.send(Err(RuntimeApiError::NotSupported { - runtime_api_name: "validation_code_hash", - })) - .unwrap(); - }, - Some(AllMessages::RuntimeApi(RuntimeApiMessage::Request( - _hash, - RuntimeApiRequest::ValidationCode( - _para_id, - OccupiedCoreAssumption::Free, - tx, - ), - ))) => { - tx.send(Ok(Some(ValidationCode(vec![1, 2, 3])))).unwrap(); - }, - Some(msg) => { - panic!("didn't expect any other overseer requests; got {:?}", msg) - }, - } - } - }; - - let config = test_config(16u32); - let subsystem_config = config.clone(); - - let (tx, rx) = mpsc::channel(0); - - // empty vec doesn't allocate on the heap, so it's ok we throw it away - let sent_messages = Arc::new(Mutex::new(Vec::new())); - let subsystem_sent_messages = sent_messages.clone(); - subsystem_test_harness(overseer, |mut ctx| async move { - handle_new_activations( - subsystem_config, - activated_hashes, - &mut ctx, - Metrics(None), - &tx, - ) - .await - .unwrap(); - - std::mem::drop(tx); - - *subsystem_sent_messages.lock().await = rx.collect().await; - }); - - let sent_messages = Arc::try_unwrap(sent_messages) - .expect("subsystem should have shut down by now") - .into_inner(); - - let expect_validation_code_hash = ValidationCode(vec![1, 2, 3]).hash(); - - assert_eq!(sent_messages.len(), 1); - match &sent_messages[0] { - overseer::CollationGenerationOutgoingMessages::CollatorProtocolMessage( - CollatorProtocolMessage::DistributeCollation( - CandidateReceipt { descriptor, .. }, - _pov, - .., - ), - ) => { - assert_eq!(expect_validation_code_hash, descriptor.validation_code_hash); - }, - _ => panic!("received wrong message type"), - } + ), + ) => { + assert_eq!(expect_validation_code_hash, descriptor.validation_code_hash); + }, + _ => panic!("received wrong message type"), } } From f2aa6a49007e9851078bb808d37cb74bdca7c554 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Wed, 21 Jun 2023 19:24:09 -0700 Subject: [PATCH 06/19] add some TODOs for a test suite. --- node/collation-generation/src/tests.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/node/collation-generation/src/tests.rs b/node/collation-generation/src/tests.rs index 86adc24a6eb6..b19c58bff3e5 100644 --- a/node/collation-generation/src/tests.rs +++ b/node/collation-generation/src/tests.rs @@ -460,3 +460,13 @@ fn fallback_when_no_validation_code_hash_api() { _ => panic!("received wrong message type"), } } + +// TODO [now]: test that `SubmitCollation` is a no-op prior to initialization + +// TODO [now]: test that `SubmitCollation` leads to distribution + +// TODO [now]: test that `ValidationCodeHashHint::Provided` is used + +// TODO [now]: test that missing `ValidationCodeHashHint` works for constraints and without + +// TODO [now]: test that provided `ValidationCodeHashHint::ParentRelayParentNumber` works for constraints and without From 6476b284275b353bab1f36d4778981a3d979e5fa Mon Sep 17 00:00:00 2001 From: asynchronous rob Date: Fri, 23 Jun 2023 14:08:13 +0200 Subject: [PATCH 07/19] Update roadmap/implementers-guide/src/types/overseer-protocol.md Co-authored-by: Andrei Sandu <54316454+sandreim@users.noreply.github.com> --- roadmap/implementers-guide/src/types/overseer-protocol.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/roadmap/implementers-guide/src/types/overseer-protocol.md b/roadmap/implementers-guide/src/types/overseer-protocol.md index a9f1ffb34fc6..ffecd3528e43 100644 --- a/roadmap/implementers-guide/src/types/overseer-protocol.md +++ b/roadmap/implementers-guide/src/types/overseer-protocol.md @@ -442,7 +442,7 @@ enum ValidationCodeHashHint { Provided(ValidationCodeHash), } -/// Parameters for submitting a +/// Parameters for submitting a collation struct SubmitCollationParams { /// The relay-parent the collation is built against. relay_parent: Hash, From a652906460952a45ac70d512c0242c0b2332dff7 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Fri, 23 Jun 2023 14:43:16 +0200 Subject: [PATCH 08/19] add new test harness and first test --- node/collation-generation/src/tests.rs | 48 +++++++++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/node/collation-generation/src/tests.rs b/node/collation-generation/src/tests.rs index b19c58bff3e5..c797fe93a66c 100644 --- a/node/collation-generation/src/tests.rs +++ b/node/collation-generation/src/tests.rs @@ -32,6 +32,37 @@ use polkadot_primitives::{ }; use std::pin::Pin; +type VirtualOverseer = TestSubsystemContextHandle; + +fn test_harness>( + test: impl FnOnce(VirtualOverseer) -> T, +) { + let pool = sp_core::testing::TaskExecutor::new(); + let (context, virtual_overseer) = polkadot_node_subsystem_test_helpers::make_subsystem_context(pool.clone()); + let subsystem = async move { + let subsystem = crate::CollationGenerationSubsystem::new(Metrics::default()); + + subsystem.run(context).await; + }; + + let test_fut = test(virtual_overseer); + + futures::pin_mut!(test_fut); + futures::pin_mut!(subsystem); + futures::executor::block_on(futures::future::join( + async move { + let mut virtual_overseer = test_fut.await; + // Ensure we have handled all responses. + if let Ok(Some(msg)) = virtual_overseer.rx.try_next() { + panic!("Did not handle all responses: {:?}", msg); + } + // Conclude. + virtual_overseer.send(FromOrchestra::Signal(OverseerSignal::Conclude)).await; + }, + subsystem, + )); +} + fn test_collation() -> Collation { Collation { upward_messages: Default::default(), @@ -461,7 +492,22 @@ fn fallback_when_no_validation_code_hash_api() { } } -// TODO [now]: test that `SubmitCollation` is a no-op prior to initialization +#[test] +fn submit_collation_is_no_op_before_initialization() { + test_harness(|mut virtual_overseer| async move { + virtual_overseer.send(FromOrchestra::Communication { + msg: CollationGenerationMessage::SubmitCollation(SubmitCollationParams { + relay_parent: Hash::repeat_byte(0), + collation: test_collation(), + parent_head: vec![1, 2, 3].into(), + validation_code_hash_hint: None, + result_sender: None, + }), + }).await; + + virtual_overseer + }); +} // TODO [now]: test that `SubmitCollation` leads to distribution From e70d7568223a8c875d742556fada926c423e8458 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Fri, 23 Jun 2023 15:25:19 +0200 Subject: [PATCH 09/19] refactor to avoid requiring background sender --- node/collation-generation/src/lib.rs | 48 ++++++++++++---------------- 1 file changed, 21 insertions(+), 27 deletions(-) diff --git a/node/collation-generation/src/lib.rs b/node/collation-generation/src/lib.rs index 8789a0e573be..2ea4d3bffe59 100644 --- a/node/collation-generation/src/lib.rs +++ b/node/collation-generation/src/lib.rs @@ -31,7 +31,7 @@ use futures::{ channel::{mpsc, oneshot}, - future::FutureExt, + future::{Future, FutureExt}, join, select, sink::SinkExt, stream::StreamExt, @@ -168,7 +168,7 @@ impl CollationGenerationSubsystem { }) => { if let Some(config) = &self.config { if let Err(err) = - handle_submit_collation(params, config, ctx, sender.clone(), &self.metrics) + handle_submit_collation(params, config, ctx, &self.metrics) .await { gum::error!(target: LOG_TARGET, ?err, "Failed to submit collation"); @@ -322,7 +322,7 @@ async fn handle_new_activations( }; let task_config = config.clone(); - let task_sender = sender.clone(); + let mut task_sender = sender.clone(); let metrics = metrics.clone(); ctx.spawn( "collation-builder", @@ -355,7 +355,15 @@ async fn handle_new_activations( n_validators, }, task_config.key.clone(), - task_sender, + |msg| async move { + if let Err(err) = task_sender.send(msg.into()).await { + gum::warn!( + target: LOG_TARGET, + ?err, + "failed to send collation result" + ); + } + }, result_sender, &metrics, ) @@ -373,7 +381,6 @@ async fn handle_submit_collation( params: SubmitCollationParams, config: &CollationGenerationConfig, ctx: &mut Context, - sender: mpsc::Sender, metrics: &Metrics, ) -> crate::error::Result<()> { let _timer = metrics.time_submit_collation(); @@ -438,7 +445,7 @@ async fn handle_submit_collation( construct_and_distribute_receipt( collation, config.key.clone(), - sender.clone(), + |msg| ctx.send_message(msg), result_sender, metrics, ) @@ -458,10 +465,10 @@ struct PreparedCollation { /// Takes a prepared collation, along with its context, and produces a candidate receipt /// which is distributed to validators. -async fn construct_and_distribute_receipt( +async fn construct_and_distribute_receipt>( collation: PreparedCollation, key: CollatorPair, - mut sender: mpsc::Sender, + distribute_collation: impl FnOnce(CollatorProtocolMessage) -> F, result_sender: Option>, metrics: &Metrics, ) { @@ -559,25 +566,12 @@ async fn construct_and_distribute_receipt( ); metrics.on_collation_generated(); - if let Err(err) = sender - .send( - CollatorProtocolMessage::DistributeCollation( - ccr, - parent_head_data_hash, - pov, - result_sender, - ) - .into(), - ) - .await - { - gum::warn!( - target: LOG_TARGET, - para_id = %para_id, - err = ?err, - "failed to send collation result", - ); - } + distribute_collation(CollatorProtocolMessage::DistributeCollation( + ccr, + parent_head_data_hash, + pov, + result_sender, + )).await; } async fn obtain_validation_code_hash_with_hint( From a02ba1c83cdeb8ea950d7845ddb7d5b4b99649da Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Fri, 23 Jun 2023 15:26:19 +0200 Subject: [PATCH 10/19] ensure collation gets packaged and distributed --- Cargo.lock | 2 + node/collation-generation/Cargo.toml | 2 + node/collation-generation/src/tests.rs | 106 +++++++++++++++++++++++-- 3 files changed, 102 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 419654736dd3..678a45946f88 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7233,6 +7233,7 @@ dependencies = [ name = "polkadot-node-collation-generation" version = "0.9.41" dependencies = [ + "assert_matches", "futures", "parity-scale-codec 3.4.0", "polkadot-erasure-coding", @@ -7243,6 +7244,7 @@ dependencies = [ "polkadot-primitives", "polkadot-primitives-test-helpers", "sp-core", + "sp-keyring", "sp-maybe-compressed-blob", "thiserror", "tracing-gum", diff --git a/node/collation-generation/Cargo.toml b/node/collation-generation/Cargo.toml index 68410c2cecbe..a2ece2dd5404 100644 --- a/node/collation-generation/Cargo.toml +++ b/node/collation-generation/Cargo.toml @@ -20,3 +20,5 @@ parity-scale-codec = { version = "3.4.0", default-features = false, features = [ [dev-dependencies] polkadot-node-subsystem-test-helpers = { path = "../subsystem-test-helpers" } test-helpers = { package = "polkadot-primitives-test-helpers", path = "../../primitives/test-helpers" } +assert_matches = "1.4.0" +sp-keyring = { git = "https://github.com/paritytech/substrate", branch = "master" } diff --git a/node/collation-generation/src/tests.rs b/node/collation-generation/src/tests.rs index c797fe93a66c..d734324f5a44 100644 --- a/node/collation-generation/src/tests.rs +++ b/node/collation-generation/src/tests.rs @@ -15,6 +15,7 @@ // along with Polkadot. If not, see . use super::*; +use assert_matches::assert_matches; use test_helpers::{dummy_hash, dummy_head_data, dummy_validator}; use futures::{ lock::Mutex, @@ -27,9 +28,11 @@ use polkadot_node_subsystem::{ messages::{AllMessages, RuntimeApiMessage, RuntimeApiRequest}, }; use polkadot_node_subsystem_test_helpers::{subsystem_test_harness, TestSubsystemContextHandle}; +use polkadot_node_subsystem_util::TimeoutExt; use polkadot_primitives::{ CollatorPair, Id as ParaId, PersistedValidationData, ScheduledCore, ValidationCode, }; +use sp_keyring::sr25519::Keyring as Sr25519Keyring; use std::pin::Pin; type VirtualOverseer = TestSubsystemContextHandle; @@ -101,12 +104,29 @@ impl Future for TestCollator { impl Unpin for TestCollator {} -fn test_config>(para_id: Id) -> Arc { - Arc::new(CollationGenerationConfig { +async fn overseer_recv(overseer: &mut VirtualOverseer) -> AllMessages { + const TIMEOUT: std::time::Duration = std::time::Duration::from_millis(2000); + + overseer.recv() + .timeout(TIMEOUT) + .await + .expect(&format!("{:?} is long enough to receive messages", TIMEOUT)) +} + +fn test_config>(para_id: Id) -> CollationGenerationConfig { + CollationGenerationConfig { key: CollatorPair::generate().0, collator: Some(Box::new(|_: Hash, _vd: &PersistedValidationData| TestCollator.boxed())), para_id: para_id.into(), - }) + } +} + +fn test_config_no_collator>(para_id: Id) -> CollationGenerationConfig { + CollationGenerationConfig { + key: CollatorPair::generate().0, + collator: None, + para_id: para_id.into(), + } } fn scheduled_core_for>(para_id: Id) -> ScheduledCore { @@ -142,7 +162,7 @@ fn requests_availability_per_relay_parent() { let subsystem_activated_hashes = activated_hashes.clone(); subsystem_test_harness(overseer, |mut ctx| async move { handle_new_activations( - test_config(123u32), + Arc::new(test_config(123u32)), subsystem_activated_hashes, &mut ctx, Metrics(None), @@ -219,7 +239,7 @@ fn requests_validation_data_for_scheduled_matches() { let (tx, _rx) = mpsc::channel(0); subsystem_test_harness(overseer, |mut ctx| async move { - handle_new_activations(test_config(16), activated_hashes, &mut ctx, Metrics(None), &tx) + handle_new_activations(Arc::new(test_config(16)), activated_hashes, &mut ctx, Metrics(None), &tx) .await .unwrap(); }); @@ -297,7 +317,7 @@ fn sends_distribute_collation_message() { } }; - let config = test_config(16); + let config = Arc::new(test_config(16)); let subsystem_config = config.clone(); let (tx, rx) = mpsc::channel(0); @@ -453,7 +473,7 @@ fn fallback_when_no_validation_code_hash_api() { } }; - let config = test_config(16u32); + let config = Arc::new(test_config(16u32)); let subsystem_config = config.clone(); let (tx, rx) = mpsc::channel(0); @@ -509,7 +529,77 @@ fn submit_collation_is_no_op_before_initialization() { }); } -// TODO [now]: test that `SubmitCollation` leads to distribution +#[test] +fn submit_collation_leads_to_distribution() { + let relay_parent = Hash::repeat_byte(0); + let validation_code_hash = ValidationCodeHash::from(Hash::repeat_byte(42)); + let parent_head = HeadData::from(vec![1, 2, 3]); + let para_id = ParaId::from(5); + let expected_pvd = PersistedValidationData { + parent_head: parent_head.clone(), + relay_parent_number: 10, + relay_parent_storage_root: Hash::repeat_byte(1), + max_pov_size: 1024, + }; + + test_harness(|mut virtual_overseer| async move { + virtual_overseer.send(FromOrchestra::Communication { + msg: CollationGenerationMessage::Initialize(test_config_no_collator(para_id)), + }).await; + + virtual_overseer.send(FromOrchestra::Communication { + msg: CollationGenerationMessage::SubmitCollation(SubmitCollationParams { + relay_parent, + collation: test_collation(), + parent_head: vec![1, 2, 3].into(), + validation_code_hash_hint: Some(ValidationCodeHashHint::Provided(validation_code_hash)), + result_sender: None, + }) + }).await; + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::Validators(tx))) => { + assert_eq!(rp, relay_parent); + let _ = tx.send(Ok(vec![ + Sr25519Keyring::Alice.public().into(), + Sr25519Keyring::Bob.public().into(), + Sr25519Keyring::Charlie.public().into(), + ])); + } + ); + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::PersistedValidationData(id, a, tx))) => { + assert_eq!(rp, relay_parent); + assert_eq!(id, para_id); + assert_eq!(a, OccupiedCoreAssumption::TimedOut); + + // Candidate receipt should be constructed with the real parent head. + let mut pvd = expected_pvd.clone(); + pvd.parent_head = vec![4, 5, 6].into(); + let _ = tx.send(Ok(Some(pvd))); + } + ); + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::CollatorProtocol(CollatorProtocolMessage::DistributeCollation( + ccr, + parent_head_data_hash, + .. + )) => { + assert_eq!(parent_head_data_hash, parent_head.hash()); + assert_eq!(ccr.descriptor().persisted_validation_data_hash, expected_pvd.hash()); + assert_eq!(ccr.descriptor().para_head, dummy_head_data().hash()); + assert_eq!(ccr.descriptor().validation_code_hash, validation_code_hash); + } + ); + + virtual_overseer + }); +} // TODO [now]: test that `ValidationCodeHashHint::Provided` is used From db2a8830a0c6633bd2cca4df8c3a8e4f13f4f033 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Fri, 23 Jun 2023 15:55:31 +0200 Subject: [PATCH 11/19] tests for the fallback case with no hint --- node/collation-generation/src/tests.rs | 346 ++++++++++++++++++++++++- 1 file changed, 343 insertions(+), 3 deletions(-) diff --git a/node/collation-generation/src/tests.rs b/node/collation-generation/src/tests.rs index d734324f5a44..cd554e0136ad 100644 --- a/node/collation-generation/src/tests.rs +++ b/node/collation-generation/src/tests.rs @@ -30,7 +30,8 @@ use polkadot_node_subsystem::{ use polkadot_node_subsystem_test_helpers::{subsystem_test_harness, TestSubsystemContextHandle}; use polkadot_node_subsystem_util::TimeoutExt; use polkadot_primitives::{ - CollatorPair, Id as ParaId, PersistedValidationData, ScheduledCore, ValidationCode, + vstaging::{BackingState, Constraints, CandidatePendingAvailability, InboundHrmpLimitations}, + BlockNumber, CollatorPair, Id as ParaId, PersistedValidationData, ScheduledCore, ValidationCode, CandidateHash, }; use sp_keyring::sr25519::Keyring as Sr25519Keyring; use std::pin::Pin; @@ -91,6 +92,62 @@ fn test_validation_data() -> PersistedValidationData { persisted_validation_data } +fn test_backing_constraints(validation_code_hash: ValidationCodeHash, future_code: Option<(BlockNumber, ValidationCodeHash)>) -> Constraints { + Constraints { + min_relay_parent_number: 0, + max_pov_size: 1024, + max_code_size: 1024, + ump_remaining: 1, + ump_remaining_bytes: 1, + max_ump_num_per_candidate: 1, + dmp_remaining_messages: Vec::new(), + hrmp_inbound: InboundHrmpLimitations { valid_watermarks: Vec::new() }, + hrmp_channels_out: Vec::new(), + max_hrmp_num_per_candidate: 1, + required_parent: vec![1].into(), + validation_code_hash, + upgrade_restriction: None, + future_validation_code: future_code, + } +} + +fn test_candidate_pending_availability( + para_id: ParaId, + relay_parent_number: BlockNumber, + head_data: HeadData, + validation_code_hash: ValidationCodeHash +) -> CandidatePendingAvailability { + let collator_key = CollatorPair::generate().0; + + let descriptor = CandidateDescriptor { + para_id, + relay_parent: Hash::repeat_byte(relay_parent_number as u8), + collator: collator_key.public(), + persisted_validation_data_hash: Hash::repeat_byte(0), + pov_hash: Hash::repeat_byte(0), + erasure_root: Hash::repeat_byte(0), + signature: collator_key.sign(&[0]), + para_head: head_data.hash(), + validation_code_hash, + }; + let commitments = CandidateCommitments { + upward_messages: Default::default(), + horizontal_messages: Default::default(), + new_validation_code: None, + head_data, + processed_downward_messages: 0, + hrmp_watermark: relay_parent_number, + }; + + CandidatePendingAvailability { + candidate_hash: CandidateHash(Hash::repeat_byte(100)), + descriptor, + commitments, + relay_parent_number, + max_pov_size: 1024, + } +} + // Box + Unpin + Send struct TestCollator; @@ -601,8 +658,291 @@ fn submit_collation_leads_to_distribution() { }); } -// TODO [now]: test that `ValidationCodeHashHint::Provided` is used +#[test] +fn missing_code_hash_hint_with_constraints() { + let relay_parent = Hash::repeat_byte(0); + let validation_code_hash = ValidationCodeHash::from(Hash::repeat_byte(42)); + let future_validation_code_hash = ValidationCodeHash::from(Hash::repeat_byte(43)); + let parent_head = HeadData::from(vec![1, 2, 3]); + let para_id = ParaId::from(5); + let expected_pvd = PersistedValidationData { + parent_head: parent_head.clone(), + relay_parent_number: 10, + relay_parent_storage_root: Hash::repeat_byte(1), + max_pov_size: 1024, + }; + + test_harness(|mut virtual_overseer| async move { + virtual_overseer.send(FromOrchestra::Communication { + msg: CollationGenerationMessage::Initialize(test_config_no_collator(para_id)), + }).await; + + virtual_overseer.send(FromOrchestra::Communication { + msg: CollationGenerationMessage::SubmitCollation(SubmitCollationParams { + relay_parent, + collation: test_collation(), + parent_head: vec![1, 2, 3].into(), + validation_code_hash_hint: None, + result_sender: None, + }) + }).await; + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::Validators(tx))) => { + assert_eq!(rp, relay_parent); + let _ = tx.send(Ok(vec![ + Sr25519Keyring::Alice.public().into(), + Sr25519Keyring::Bob.public().into(), + Sr25519Keyring::Charlie.public().into(), + ])); + } + ); + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::PersistedValidationData(id, a, tx))) => { + assert_eq!(rp, relay_parent); + assert_eq!(id, para_id); + assert_eq!(a, OccupiedCoreAssumption::TimedOut); + + // Candidate receipt should be constructed with the real parent head. + let mut pvd = expected_pvd.clone(); + pvd.parent_head = vec![4, 5, 6].into(); + let _ = tx.send(Ok(Some(pvd))); + } + ); + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::StagingParaBackingState(id, tx))) => { + assert_eq!(rp, relay_parent); + assert_eq!(id, para_id); + + // The parent candidate is pending availability and triggered a code upgrade, + // so our collated candidate should use the future code hash. + + let _ = tx.send(Ok(Some(BackingState { + constraints: test_backing_constraints( + validation_code_hash, + Some((9, future_validation_code_hash)), + ), + pending_availability: vec![test_candidate_pending_availability( + para_id, + 9, + parent_head.clone(), + validation_code_hash, + )], + }))); + } + ); + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::CollatorProtocol(CollatorProtocolMessage::DistributeCollation( + ccr, + parent_head_data_hash, + .. + )) => { + assert_eq!(parent_head_data_hash, parent_head.hash()); + assert_eq!(ccr.descriptor().persisted_validation_data_hash, expected_pvd.hash()); + assert_eq!(ccr.descriptor().para_head, dummy_head_data().hash()); + assert_eq!(ccr.descriptor().validation_code_hash, future_validation_code_hash); + } + ); + + virtual_overseer + }); +} + +#[test] +fn missing_code_hash_hint_with_constraints_no_code_upgrade() { + let relay_parent = Hash::repeat_byte(0); + let validation_code_hash = ValidationCodeHash::from(Hash::repeat_byte(42)); + let future_validation_code_hash = ValidationCodeHash::from(Hash::repeat_byte(43)); + let parent_head = HeadData::from(vec![1, 2, 3]); + let para_id = ParaId::from(5); + let expected_pvd = PersistedValidationData { + parent_head: parent_head.clone(), + relay_parent_number: 10, + relay_parent_storage_root: Hash::repeat_byte(1), + max_pov_size: 1024, + }; + + test_harness(|mut virtual_overseer| async move { + virtual_overseer.send(FromOrchestra::Communication { + msg: CollationGenerationMessage::Initialize(test_config_no_collator(para_id)), + }).await; + + virtual_overseer.send(FromOrchestra::Communication { + msg: CollationGenerationMessage::SubmitCollation(SubmitCollationParams { + relay_parent, + collation: test_collation(), + parent_head: vec![1, 2, 3].into(), + validation_code_hash_hint: None, + result_sender: None, + }) + }).await; + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::Validators(tx))) => { + assert_eq!(rp, relay_parent); + let _ = tx.send(Ok(vec![ + Sr25519Keyring::Alice.public().into(), + Sr25519Keyring::Bob.public().into(), + Sr25519Keyring::Charlie.public().into(), + ])); + } + ); + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::PersistedValidationData(id, a, tx))) => { + assert_eq!(rp, relay_parent); + assert_eq!(id, para_id); + assert_eq!(a, OccupiedCoreAssumption::TimedOut); + + // Candidate receipt should be constructed with the real parent head. + let mut pvd = expected_pvd.clone(); + pvd.parent_head = vec![4, 5, 6].into(); + let _ = tx.send(Ok(Some(pvd))); + } + ); + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::StagingParaBackingState(id, tx))) => { + assert_eq!(rp, relay_parent); + assert_eq!(id, para_id); + + // The parent candidate is pending availability but did not trigger a code upgrade, + // so our collated candidate should use the future code hash. + + let _ = tx.send(Ok(Some(BackingState { + constraints: test_backing_constraints( + validation_code_hash, + Some((9, future_validation_code_hash)), + ), + pending_availability: vec![test_candidate_pending_availability( + para_id, + 8, + parent_head.clone(), + validation_code_hash, + )], + }))); + } + ); + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::CollatorProtocol(CollatorProtocolMessage::DistributeCollation( + ccr, + parent_head_data_hash, + .. + )) => { + assert_eq!(parent_head_data_hash, parent_head.hash()); + assert_eq!(ccr.descriptor().persisted_validation_data_hash, expected_pvd.hash()); + assert_eq!(ccr.descriptor().para_head, dummy_head_data().hash()); + assert_eq!(ccr.descriptor().validation_code_hash, validation_code_hash); + } + ); + + virtual_overseer + }); +} + +#[test] +fn missing_code_hash_hint_without_constraints() { + let relay_parent = Hash::repeat_byte(0); + let validation_code_hash = ValidationCodeHash::from(Hash::repeat_byte(42)); + let parent_head = HeadData::from(vec![1, 2, 3]); + let para_id = ParaId::from(5); + let expected_pvd = PersistedValidationData { + parent_head: parent_head.clone(), + relay_parent_number: 10, + relay_parent_storage_root: Hash::repeat_byte(1), + max_pov_size: 1024, + }; + + test_harness(|mut virtual_overseer| async move { + virtual_overseer.send(FromOrchestra::Communication { + msg: CollationGenerationMessage::Initialize(test_config_no_collator(para_id)), + }).await; + + virtual_overseer.send(FromOrchestra::Communication { + msg: CollationGenerationMessage::SubmitCollation(SubmitCollationParams { + relay_parent, + collation: test_collation(), + parent_head: vec![1, 2, 3].into(), + validation_code_hash_hint: None, + result_sender: None, + }) + }).await; + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::Validators(tx))) => { + assert_eq!(rp, relay_parent); + let _ = tx.send(Ok(vec![ + Sr25519Keyring::Alice.public().into(), + Sr25519Keyring::Bob.public().into(), + Sr25519Keyring::Charlie.public().into(), + ])); + } + ); + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::PersistedValidationData(id, a, tx))) => { + assert_eq!(rp, relay_parent); + assert_eq!(id, para_id); + assert_eq!(a, OccupiedCoreAssumption::TimedOut); -// TODO [now]: test that missing `ValidationCodeHashHint` works for constraints and without + // Candidate receipt should be constructed with the real parent head. + let mut pvd = expected_pvd.clone(); + pvd.parent_head = vec![4, 5, 6].into(); + let _ = tx.send(Ok(Some(pvd))); + } + ); + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::StagingParaBackingState(id, tx))) => { + assert_eq!(rp, relay_parent); + assert_eq!(id, para_id); + + let _ = tx.send(Err(RuntimeApiError::NotSupported { runtime_api_name: "not_important" })); + } + ); + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::ValidationCodeHash(id, a, tx))) => { + assert_eq!(rp, relay_parent); + assert_eq!(id, para_id); + assert_eq!(a, OccupiedCoreAssumption::Free); + + let _ = tx.send(Ok(Some(validation_code_hash))); + } + ); + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::CollatorProtocol(CollatorProtocolMessage::DistributeCollation( + ccr, + parent_head_data_hash, + .. + )) => { + assert_eq!(parent_head_data_hash, parent_head.hash()); + assert_eq!(ccr.descriptor().persisted_validation_data_hash, expected_pvd.hash()); + assert_eq!(ccr.descriptor().para_head, dummy_head_data().hash()); + assert_eq!(ccr.descriptor().validation_code_hash, validation_code_hash); + } + ); + + virtual_overseer + }); +} // TODO [now]: test that provided `ValidationCodeHashHint::ParentRelayParentNumber` works for constraints and without From 042c88ab5867ece926fdc65cc0e5fe0c949ca0db Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Fri, 23 Jun 2023 16:44:03 +0200 Subject: [PATCH 12/19] add parent rp-number hint tests --- node/collation-generation/src/tests.rs | 282 ++++++++++++++++++++++++- 1 file changed, 281 insertions(+), 1 deletion(-) diff --git a/node/collation-generation/src/tests.rs b/node/collation-generation/src/tests.rs index cd554e0136ad..2936e7b98587 100644 --- a/node/collation-generation/src/tests.rs +++ b/node/collation-generation/src/tests.rs @@ -945,4 +945,284 @@ fn missing_code_hash_hint_without_constraints() { }); } -// TODO [now]: test that provided `ValidationCodeHashHint::ParentRelayParentNumber` works for constraints and without +#[test] +fn parent_rp_number_code_hash_hint_with_constraints() { + let relay_parent = Hash::repeat_byte(0); + let validation_code_hash = ValidationCodeHash::from(Hash::repeat_byte(42)); + let future_validation_code_hash = ValidationCodeHash::from(Hash::repeat_byte(43)); + let parent_head = HeadData::from(vec![1, 2, 3]); + let para_id = ParaId::from(5); + let expected_pvd = PersistedValidationData { + parent_head: parent_head.clone(), + relay_parent_number: 10, + relay_parent_storage_root: Hash::repeat_byte(1), + max_pov_size: 1024, + }; + let parent_rp_number = 9; + let code_upgrade_at = 9; + + test_harness(|mut virtual_overseer| async move { + virtual_overseer.send(FromOrchestra::Communication { + msg: CollationGenerationMessage::Initialize(test_config_no_collator(para_id)), + }).await; + + virtual_overseer.send(FromOrchestra::Communication { + msg: CollationGenerationMessage::SubmitCollation(SubmitCollationParams { + relay_parent, + collation: test_collation(), + parent_head: vec![1, 2, 3].into(), + validation_code_hash_hint: Some(ValidationCodeHashHint::ParentBlockRelayParentNumber(parent_rp_number)), + result_sender: None, + }) + }).await; + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::Validators(tx))) => { + assert_eq!(rp, relay_parent); + let _ = tx.send(Ok(vec![ + Sr25519Keyring::Alice.public().into(), + Sr25519Keyring::Bob.public().into(), + Sr25519Keyring::Charlie.public().into(), + ])); + } + ); + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::PersistedValidationData(id, a, tx))) => { + assert_eq!(rp, relay_parent); + assert_eq!(id, para_id); + assert_eq!(a, OccupiedCoreAssumption::TimedOut); + + // Candidate receipt should be constructed with the real parent head. + let mut pvd = expected_pvd.clone(); + pvd.parent_head = vec![4, 5, 6].into(); + let _ = tx.send(Ok(Some(pvd))); + } + ); + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::StagingParaBackingState(id, tx))) => { + assert_eq!(rp, relay_parent); + assert_eq!(id, para_id); + + // The parent candidate is not on-chain but will have triggered a code upgrade nonetheless, + // so our collated candidate should use the future code hash. + + let _ = tx.send(Ok(Some(BackingState { + constraints: test_backing_constraints( + validation_code_hash, + Some((code_upgrade_at, future_validation_code_hash)), + ), + pending_availability: vec![], + }))); + } + ); + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::CollatorProtocol(CollatorProtocolMessage::DistributeCollation( + ccr, + parent_head_data_hash, + .. + )) => { + assert_eq!(parent_head_data_hash, parent_head.hash()); + assert_eq!(ccr.descriptor().persisted_validation_data_hash, expected_pvd.hash()); + assert_eq!(ccr.descriptor().para_head, dummy_head_data().hash()); + assert_eq!(ccr.descriptor().validation_code_hash, future_validation_code_hash); + } + ); + + virtual_overseer + }); +} + +#[test] +fn parent_rp_number_code_hash_hint_without_constraints() { + let relay_parent = Hash::repeat_byte(0); + let validation_code_hash = ValidationCodeHash::from(Hash::repeat_byte(42)); + let parent_head = HeadData::from(vec![1, 2, 3]); + let para_id = ParaId::from(5); + let expected_pvd = PersistedValidationData { + parent_head: parent_head.clone(), + relay_parent_number: 10, + relay_parent_storage_root: Hash::repeat_byte(1), + max_pov_size: 1024, + }; + let parent_rp_number = 8; + + test_harness(|mut virtual_overseer| async move { + virtual_overseer.send(FromOrchestra::Communication { + msg: CollationGenerationMessage::Initialize(test_config_no_collator(para_id)), + }).await; + + virtual_overseer.send(FromOrchestra::Communication { + msg: CollationGenerationMessage::SubmitCollation(SubmitCollationParams { + relay_parent, + collation: test_collation(), + parent_head: vec![1, 2, 3].into(), + validation_code_hash_hint: Some(ValidationCodeHashHint::ParentBlockRelayParentNumber(parent_rp_number)), + result_sender: None, + }) + }).await; + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::Validators(tx))) => { + assert_eq!(rp, relay_parent); + let _ = tx.send(Ok(vec![ + Sr25519Keyring::Alice.public().into(), + Sr25519Keyring::Bob.public().into(), + Sr25519Keyring::Charlie.public().into(), + ])); + } + ); + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::PersistedValidationData(id, a, tx))) => { + assert_eq!(rp, relay_parent); + assert_eq!(id, para_id); + assert_eq!(a, OccupiedCoreAssumption::TimedOut); + + // Candidate receipt should be constructed with the real parent head. + let mut pvd = expected_pvd.clone(); + pvd.parent_head = vec![4, 5, 6].into(); + let _ = tx.send(Ok(Some(pvd))); + } + ); + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::StagingParaBackingState(id, tx))) => { + assert_eq!(rp, relay_parent); + assert_eq!(id, para_id); + + let _ = tx.send(Err(RuntimeApiError::NotSupported { runtime_api_name: "not_important" })); + } + ); + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::ValidationCodeHash(id, a, tx))) => { + assert_eq!(rp, relay_parent); + assert_eq!(id, para_id); + assert_eq!(a, OccupiedCoreAssumption::Free); + + let _ = tx.send(Ok(Some(validation_code_hash))); + } + ); + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::CollatorProtocol(CollatorProtocolMessage::DistributeCollation( + ccr, + parent_head_data_hash, + .. + )) => { + assert_eq!(parent_head_data_hash, parent_head.hash()); + assert_eq!(ccr.descriptor().persisted_validation_data_hash, expected_pvd.hash()); + assert_eq!(ccr.descriptor().para_head, dummy_head_data().hash()); + assert_eq!(ccr.descriptor().validation_code_hash, validation_code_hash); + } + ); + + virtual_overseer + }); +} + +#[test] +fn parent_rp_number_code_hash_hint_with_constraints_no_upgrade() { + let relay_parent = Hash::repeat_byte(0); + let validation_code_hash = ValidationCodeHash::from(Hash::repeat_byte(42)); + let future_validation_code_hash = ValidationCodeHash::from(Hash::repeat_byte(43)); + let parent_head = HeadData::from(vec![1, 2, 3]); + let para_id = ParaId::from(5); + let expected_pvd = PersistedValidationData { + parent_head: parent_head.clone(), + relay_parent_number: 10, + relay_parent_storage_root: Hash::repeat_byte(1), + max_pov_size: 1024, + }; + let parent_rp_number = 8; + let code_upgrade_at = 9; + + test_harness(|mut virtual_overseer| async move { + virtual_overseer.send(FromOrchestra::Communication { + msg: CollationGenerationMessage::Initialize(test_config_no_collator(para_id)), + }).await; + + virtual_overseer.send(FromOrchestra::Communication { + msg: CollationGenerationMessage::SubmitCollation(SubmitCollationParams { + relay_parent, + collation: test_collation(), + parent_head: vec![1, 2, 3].into(), + validation_code_hash_hint: Some(ValidationCodeHashHint::ParentBlockRelayParentNumber(parent_rp_number)), + result_sender: None, + }) + }).await; + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::Validators(tx))) => { + assert_eq!(rp, relay_parent); + let _ = tx.send(Ok(vec![ + Sr25519Keyring::Alice.public().into(), + Sr25519Keyring::Bob.public().into(), + Sr25519Keyring::Charlie.public().into(), + ])); + } + ); + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::PersistedValidationData(id, a, tx))) => { + assert_eq!(rp, relay_parent); + assert_eq!(id, para_id); + assert_eq!(a, OccupiedCoreAssumption::TimedOut); + + // Candidate receipt should be constructed with the real parent head. + let mut pvd = expected_pvd.clone(); + pvd.parent_head = vec![4, 5, 6].into(); + let _ = tx.send(Ok(Some(pvd))); + } + ); + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::StagingParaBackingState(id, tx))) => { + assert_eq!(rp, relay_parent); + assert_eq!(id, para_id); + + // The parent candidate is not on-chain and will not have used the code upgrade + // so our collated candidate should use the future code hash. + + let _ = tx.send(Ok(Some(BackingState { + constraints: test_backing_constraints( + validation_code_hash, + Some((code_upgrade_at, future_validation_code_hash)), + ), + pending_availability: vec![], + }))); + } + ); + + assert_matches!( + overseer_recv(&mut virtual_overseer).await, + AllMessages::CollatorProtocol(CollatorProtocolMessage::DistributeCollation( + ccr, + parent_head_data_hash, + .. + )) => { + assert_eq!(parent_head_data_hash, parent_head.hash()); + assert_eq!(ccr.descriptor().persisted_validation_data_hash, expected_pvd.hash()); + assert_eq!(ccr.descriptor().para_head, dummy_head_data().hash()); + assert_eq!(ccr.descriptor().validation_code_hash, validation_code_hash); + } + ); + + virtual_overseer + }); +} From 609316504c4e6fbe1750a7793fd43f54c1de225a Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Fri, 23 Jun 2023 16:44:09 +0200 Subject: [PATCH 13/19] fmt --- node/collation-generation/src/lib.rs | 6 +- node/collation-generation/src/tests.rs | 260 +++++++++++++++---------- 2 files changed, 157 insertions(+), 109 deletions(-) diff --git a/node/collation-generation/src/lib.rs b/node/collation-generation/src/lib.rs index 2ea4d3bffe59..6e74e4c6db2d 100644 --- a/node/collation-generation/src/lib.rs +++ b/node/collation-generation/src/lib.rs @@ -168,8 +168,7 @@ impl CollationGenerationSubsystem { }) => { if let Some(config) = &self.config { if let Err(err) = - handle_submit_collation(params, config, ctx, &self.metrics) - .await + handle_submit_collation(params, config, ctx, &self.metrics).await { gum::error!(target: LOG_TARGET, ?err, "Failed to submit collation"); } @@ -571,7 +570,8 @@ async fn construct_and_distribute_receipt>( parent_head_data_hash, pov, result_sender, - )).await; + )) + .await; } async fn obtain_validation_code_hash_with_hint( diff --git a/node/collation-generation/src/tests.rs b/node/collation-generation/src/tests.rs index 2936e7b98587..fb71e823692e 100644 --- a/node/collation-generation/src/tests.rs +++ b/node/collation-generation/src/tests.rs @@ -16,7 +16,6 @@ use super::*; use assert_matches::assert_matches; -use test_helpers::{dummy_hash, dummy_head_data, dummy_validator}; use futures::{ lock::Mutex, task::{Context as FuturesContext, Poll}, @@ -30,19 +29,20 @@ use polkadot_node_subsystem::{ use polkadot_node_subsystem_test_helpers::{subsystem_test_harness, TestSubsystemContextHandle}; use polkadot_node_subsystem_util::TimeoutExt; use polkadot_primitives::{ - vstaging::{BackingState, Constraints, CandidatePendingAvailability, InboundHrmpLimitations}, - BlockNumber, CollatorPair, Id as ParaId, PersistedValidationData, ScheduledCore, ValidationCode, CandidateHash, + vstaging::{BackingState, CandidatePendingAvailability, Constraints, InboundHrmpLimitations}, + BlockNumber, CandidateHash, CollatorPair, Id as ParaId, PersistedValidationData, ScheduledCore, + ValidationCode, }; use sp_keyring::sr25519::Keyring as Sr25519Keyring; use std::pin::Pin; +use test_helpers::{dummy_hash, dummy_head_data, dummy_validator}; type VirtualOverseer = TestSubsystemContextHandle; -fn test_harness>( - test: impl FnOnce(VirtualOverseer) -> T, -) { +fn test_harness>(test: impl FnOnce(VirtualOverseer) -> T) { let pool = sp_core::testing::TaskExecutor::new(); - let (context, virtual_overseer) = polkadot_node_subsystem_test_helpers::make_subsystem_context(pool.clone()); + let (context, virtual_overseer) = + polkadot_node_subsystem_test_helpers::make_subsystem_context(pool.clone()); let subsystem = async move { let subsystem = crate::CollationGenerationSubsystem::new(Metrics::default()); @@ -92,7 +92,10 @@ fn test_validation_data() -> PersistedValidationData { persisted_validation_data } -fn test_backing_constraints(validation_code_hash: ValidationCodeHash, future_code: Option<(BlockNumber, ValidationCodeHash)>) -> Constraints { +fn test_backing_constraints( + validation_code_hash: ValidationCodeHash, + future_code: Option<(BlockNumber, ValidationCodeHash)>, +) -> Constraints { Constraints { min_relay_parent_number: 0, max_pov_size: 1024, @@ -115,7 +118,7 @@ fn test_candidate_pending_availability( para_id: ParaId, relay_parent_number: BlockNumber, head_data: HeadData, - validation_code_hash: ValidationCodeHash + validation_code_hash: ValidationCodeHash, ) -> CandidatePendingAvailability { let collator_key = CollatorPair::generate().0; @@ -164,7 +167,8 @@ impl Unpin for TestCollator {} async fn overseer_recv(overseer: &mut VirtualOverseer) -> AllMessages { const TIMEOUT: std::time::Duration = std::time::Duration::from_millis(2000); - overseer.recv() + overseer + .recv() .timeout(TIMEOUT) .await .expect(&format!("{:?} is long enough to receive messages", TIMEOUT)) @@ -296,9 +300,15 @@ fn requests_validation_data_for_scheduled_matches() { let (tx, _rx) = mpsc::channel(0); subsystem_test_harness(overseer, |mut ctx| async move { - handle_new_activations(Arc::new(test_config(16)), activated_hashes, &mut ctx, Metrics(None), &tx) - .await - .unwrap(); + handle_new_activations( + Arc::new(test_config(16)), + activated_hashes, + &mut ctx, + Metrics(None), + &tx, + ) + .await + .unwrap(); }); let requested_validation_data = Arc::try_unwrap(requested_validation_data) @@ -572,15 +582,17 @@ fn fallback_when_no_validation_code_hash_api() { #[test] fn submit_collation_is_no_op_before_initialization() { test_harness(|mut virtual_overseer| async move { - virtual_overseer.send(FromOrchestra::Communication { - msg: CollationGenerationMessage::SubmitCollation(SubmitCollationParams { - relay_parent: Hash::repeat_byte(0), - collation: test_collation(), - parent_head: vec![1, 2, 3].into(), - validation_code_hash_hint: None, - result_sender: None, - }), - }).await; + virtual_overseer + .send(FromOrchestra::Communication { + msg: CollationGenerationMessage::SubmitCollation(SubmitCollationParams { + relay_parent: Hash::repeat_byte(0), + collation: test_collation(), + parent_head: vec![1, 2, 3].into(), + validation_code_hash_hint: None, + result_sender: None, + }), + }) + .await; virtual_overseer }); @@ -600,19 +612,25 @@ fn submit_collation_leads_to_distribution() { }; test_harness(|mut virtual_overseer| async move { - virtual_overseer.send(FromOrchestra::Communication { - msg: CollationGenerationMessage::Initialize(test_config_no_collator(para_id)), - }).await; - - virtual_overseer.send(FromOrchestra::Communication { - msg: CollationGenerationMessage::SubmitCollation(SubmitCollationParams { - relay_parent, - collation: test_collation(), - parent_head: vec![1, 2, 3].into(), - validation_code_hash_hint: Some(ValidationCodeHashHint::Provided(validation_code_hash)), - result_sender: None, + virtual_overseer + .send(FromOrchestra::Communication { + msg: CollationGenerationMessage::Initialize(test_config_no_collator(para_id)), + }) + .await; + + virtual_overseer + .send(FromOrchestra::Communication { + msg: CollationGenerationMessage::SubmitCollation(SubmitCollationParams { + relay_parent, + collation: test_collation(), + parent_head: vec![1, 2, 3].into(), + validation_code_hash_hint: Some(ValidationCodeHashHint::Provided( + validation_code_hash, + )), + result_sender: None, + }), }) - }).await; + .await; assert_matches!( overseer_recv(&mut virtual_overseer).await, @@ -673,19 +691,23 @@ fn missing_code_hash_hint_with_constraints() { }; test_harness(|mut virtual_overseer| async move { - virtual_overseer.send(FromOrchestra::Communication { - msg: CollationGenerationMessage::Initialize(test_config_no_collator(para_id)), - }).await; - - virtual_overseer.send(FromOrchestra::Communication { - msg: CollationGenerationMessage::SubmitCollation(SubmitCollationParams { - relay_parent, - collation: test_collation(), - parent_head: vec![1, 2, 3].into(), - validation_code_hash_hint: None, - result_sender: None, + virtual_overseer + .send(FromOrchestra::Communication { + msg: CollationGenerationMessage::Initialize(test_config_no_collator(para_id)), }) - }).await; + .await; + + virtual_overseer + .send(FromOrchestra::Communication { + msg: CollationGenerationMessage::SubmitCollation(SubmitCollationParams { + relay_parent, + collation: test_collation(), + parent_head: vec![1, 2, 3].into(), + validation_code_hash_hint: None, + result_sender: None, + }), + }) + .await; assert_matches!( overseer_recv(&mut virtual_overseer).await, @@ -770,19 +792,23 @@ fn missing_code_hash_hint_with_constraints_no_code_upgrade() { }; test_harness(|mut virtual_overseer| async move { - virtual_overseer.send(FromOrchestra::Communication { - msg: CollationGenerationMessage::Initialize(test_config_no_collator(para_id)), - }).await; - - virtual_overseer.send(FromOrchestra::Communication { - msg: CollationGenerationMessage::SubmitCollation(SubmitCollationParams { - relay_parent, - collation: test_collation(), - parent_head: vec![1, 2, 3].into(), - validation_code_hash_hint: None, - result_sender: None, + virtual_overseer + .send(FromOrchestra::Communication { + msg: CollationGenerationMessage::Initialize(test_config_no_collator(para_id)), }) - }).await; + .await; + + virtual_overseer + .send(FromOrchestra::Communication { + msg: CollationGenerationMessage::SubmitCollation(SubmitCollationParams { + relay_parent, + collation: test_collation(), + parent_head: vec![1, 2, 3].into(), + validation_code_hash_hint: None, + result_sender: None, + }), + }) + .await; assert_matches!( overseer_recv(&mut virtual_overseer).await, @@ -866,19 +892,23 @@ fn missing_code_hash_hint_without_constraints() { }; test_harness(|mut virtual_overseer| async move { - virtual_overseer.send(FromOrchestra::Communication { - msg: CollationGenerationMessage::Initialize(test_config_no_collator(para_id)), - }).await; - - virtual_overseer.send(FromOrchestra::Communication { - msg: CollationGenerationMessage::SubmitCollation(SubmitCollationParams { - relay_parent, - collation: test_collation(), - parent_head: vec![1, 2, 3].into(), - validation_code_hash_hint: None, - result_sender: None, + virtual_overseer + .send(FromOrchestra::Communication { + msg: CollationGenerationMessage::Initialize(test_config_no_collator(para_id)), }) - }).await; + .await; + + virtual_overseer + .send(FromOrchestra::Communication { + msg: CollationGenerationMessage::SubmitCollation(SubmitCollationParams { + relay_parent, + collation: test_collation(), + parent_head: vec![1, 2, 3].into(), + validation_code_hash_hint: None, + result_sender: None, + }), + }) + .await; assert_matches!( overseer_recv(&mut virtual_overseer).await, @@ -962,19 +992,25 @@ fn parent_rp_number_code_hash_hint_with_constraints() { let code_upgrade_at = 9; test_harness(|mut virtual_overseer| async move { - virtual_overseer.send(FromOrchestra::Communication { - msg: CollationGenerationMessage::Initialize(test_config_no_collator(para_id)), - }).await; - - virtual_overseer.send(FromOrchestra::Communication { - msg: CollationGenerationMessage::SubmitCollation(SubmitCollationParams { - relay_parent, - collation: test_collation(), - parent_head: vec![1, 2, 3].into(), - validation_code_hash_hint: Some(ValidationCodeHashHint::ParentBlockRelayParentNumber(parent_rp_number)), - result_sender: None, + virtual_overseer + .send(FromOrchestra::Communication { + msg: CollationGenerationMessage::Initialize(test_config_no_collator(para_id)), + }) + .await; + + virtual_overseer + .send(FromOrchestra::Communication { + msg: CollationGenerationMessage::SubmitCollation(SubmitCollationParams { + relay_parent, + collation: test_collation(), + parent_head: vec![1, 2, 3].into(), + validation_code_hash_hint: Some( + ValidationCodeHashHint::ParentBlockRelayParentNumber(parent_rp_number), + ), + result_sender: None, + }), }) - }).await; + .await; assert_matches!( overseer_recv(&mut virtual_overseer).await, @@ -1054,19 +1090,25 @@ fn parent_rp_number_code_hash_hint_without_constraints() { let parent_rp_number = 8; test_harness(|mut virtual_overseer| async move { - virtual_overseer.send(FromOrchestra::Communication { - msg: CollationGenerationMessage::Initialize(test_config_no_collator(para_id)), - }).await; - - virtual_overseer.send(FromOrchestra::Communication { - msg: CollationGenerationMessage::SubmitCollation(SubmitCollationParams { - relay_parent, - collation: test_collation(), - parent_head: vec![1, 2, 3].into(), - validation_code_hash_hint: Some(ValidationCodeHashHint::ParentBlockRelayParentNumber(parent_rp_number)), - result_sender: None, + virtual_overseer + .send(FromOrchestra::Communication { + msg: CollationGenerationMessage::Initialize(test_config_no_collator(para_id)), + }) + .await; + + virtual_overseer + .send(FromOrchestra::Communication { + msg: CollationGenerationMessage::SubmitCollation(SubmitCollationParams { + relay_parent, + collation: test_collation(), + parent_head: vec![1, 2, 3].into(), + validation_code_hash_hint: Some( + ValidationCodeHashHint::ParentBlockRelayParentNumber(parent_rp_number), + ), + result_sender: None, + }), }) - }).await; + .await; assert_matches!( overseer_recv(&mut virtual_overseer).await, @@ -1150,19 +1192,25 @@ fn parent_rp_number_code_hash_hint_with_constraints_no_upgrade() { let code_upgrade_at = 9; test_harness(|mut virtual_overseer| async move { - virtual_overseer.send(FromOrchestra::Communication { - msg: CollationGenerationMessage::Initialize(test_config_no_collator(para_id)), - }).await; - - virtual_overseer.send(FromOrchestra::Communication { - msg: CollationGenerationMessage::SubmitCollation(SubmitCollationParams { - relay_parent, - collation: test_collation(), - parent_head: vec![1, 2, 3].into(), - validation_code_hash_hint: Some(ValidationCodeHashHint::ParentBlockRelayParentNumber(parent_rp_number)), - result_sender: None, + virtual_overseer + .send(FromOrchestra::Communication { + msg: CollationGenerationMessage::Initialize(test_config_no_collator(para_id)), + }) + .await; + + virtual_overseer + .send(FromOrchestra::Communication { + msg: CollationGenerationMessage::SubmitCollation(SubmitCollationParams { + relay_parent, + collation: test_collation(), + parent_head: vec![1, 2, 3].into(), + validation_code_hash_hint: Some( + ValidationCodeHashHint::ParentBlockRelayParentNumber(parent_rp_number), + ), + result_sender: None, + }), }) - }).await; + .await; assert_matches!( overseer_recv(&mut virtual_overseer).await, From 53286e946039a798cb8ef172cc7d1bc862966d01 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Fri, 23 Jun 2023 17:09:16 +0200 Subject: [PATCH 14/19] update uses of CollationGenerationConfig --- node/test/service/src/lib.rs | 3 ++- parachain/test-parachains/adder/collator/src/main.rs | 5 +++-- parachain/test-parachains/undying/collator/src/main.rs | 5 +++-- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/node/test/service/src/lib.rs b/node/test/service/src/lib.rs index 3e7f66128886..899af010fd4c 100644 --- a/node/test/service/src/lib.rs +++ b/node/test/service/src/lib.rs @@ -357,7 +357,8 @@ impl PolkadotTestNode { para_id: ParaId, collator: CollatorFn, ) { - let config = CollationGenerationConfig { key: collator_key, collator, para_id }; + let config = + CollationGenerationConfig { key: collator_key, collator: Some(collator), para_id }; self.overseer_handle .send_msg(CollationGenerationMessage::Initialize(config), "Collator") diff --git a/parachain/test-parachains/adder/collator/src/main.rs b/parachain/test-parachains/adder/collator/src/main.rs index 699cee202cb8..4b959ced4cf9 100644 --- a/parachain/test-parachains/adder/collator/src/main.rs +++ b/parachain/test-parachains/adder/collator/src/main.rs @@ -87,8 +87,9 @@ fn main() -> Result<()> { let config = CollationGenerationConfig { key: collator.collator_key(), - collator: collator - .create_collation_function(full_node.task_manager.spawn_handle()), + collator: Some( + collator.create_collation_function(full_node.task_manager.spawn_handle()), + ), para_id, }; overseer_handle diff --git a/parachain/test-parachains/undying/collator/src/main.rs b/parachain/test-parachains/undying/collator/src/main.rs index 189674b82a97..df1fec00ee86 100644 --- a/parachain/test-parachains/undying/collator/src/main.rs +++ b/parachain/test-parachains/undying/collator/src/main.rs @@ -87,8 +87,9 @@ fn main() -> Result<()> { let config = CollationGenerationConfig { key: collator.collator_key(), - collator: collator - .create_collation_function(full_node.task_manager.spawn_handle()), + collator: Some( + collator.create_collation_function(full_node.task_manager.spawn_handle()), + ), para_id, }; overseer_handle From d3169a2fe30598ac8f66949332027e0a22dbc618 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Tue, 27 Jun 2023 00:25:14 +0200 Subject: [PATCH 15/19] fix remaining test --- node/overseer/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/overseer/src/tests.rs b/node/overseer/src/tests.rs index a5f473007c82..3d155acc1fac 100644 --- a/node/overseer/src/tests.rs +++ b/node/overseer/src/tests.rs @@ -797,7 +797,7 @@ fn test_chain_api_msg() -> ChainApiMessage { fn test_collator_generation_msg() -> CollationGenerationMessage { CollationGenerationMessage::Initialize(CollationGenerationConfig { key: CollatorPair::generate().0, - collator: Box::new(|_, _| TestCollator.boxed()), + collator: Some(Box::new(|_, _| TestCollator.boxed())), para_id: Default::default(), }) } From a1deebab0c3f1ff2c19e3b92455ad7eeb42c305b Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Wed, 5 Jul 2023 16:25:53 +0200 Subject: [PATCH 16/19] address review comments --- node/collation-generation/src/tests.rs | 3 +-- .../src/node/collators/collation-generation.md | 1 - 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/node/collation-generation/src/tests.rs b/node/collation-generation/src/tests.rs index fb71e823692e..8c3852236820 100644 --- a/node/collation-generation/src/tests.rs +++ b/node/collation-generation/src/tests.rs @@ -42,7 +42,7 @@ type VirtualOverseer = TestSubsystemContextHandle; fn test_harness>(test: impl FnOnce(VirtualOverseer) -> T) { let pool = sp_core::testing::TaskExecutor::new(); let (context, virtual_overseer) = - polkadot_node_subsystem_test_helpers::make_subsystem_context(pool.clone()); + polkadot_node_subsystem_test_helpers::make_subsystem_context(pool); let subsystem = async move { let subsystem = crate::CollationGenerationSubsystem::new(Metrics::default()); @@ -52,7 +52,6 @@ fn test_harness>(test: impl FnOnce(VirtualOv let test_fut = test(virtual_overseer); futures::pin_mut!(test_fut); - futures::pin_mut!(subsystem); futures::executor::block_on(futures::future::join( async move { let mut virtual_overseer = test_fut.await; diff --git a/roadmap/implementers-guide/src/node/collators/collation-generation.md b/roadmap/implementers-guide/src/node/collators/collation-generation.md index 1fba9080c59d..03588c4b316f 100644 --- a/roadmap/implementers-guide/src/node/collators/collation-generation.md +++ b/roadmap/implementers-guide/src/node/collators/collation-generation.md @@ -108,7 +108,6 @@ pub struct CollationGenerationConfig { /// Collator's authentication key, so it can sign things. pub key: CollatorPair, /// Collation function. See [`CollatorFn`] for more details. - /// pub collator: Option, /// The parachain that this collator collates for pub para_id: ParaId, From 725f4857f367c03e45567de76a782a0499e2df90 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Wed, 5 Jul 2023 16:49:23 +0200 Subject: [PATCH 17/19] use subsystemsender for background tasks --- node/collation-generation/src/lib.rs | 47 ++++-------------- node/collation-generation/src/tests.rs | 66 +++++++++++--------------- 2 files changed, 37 insertions(+), 76 deletions(-) diff --git a/node/collation-generation/src/lib.rs b/node/collation-generation/src/lib.rs index 6e74e4c6db2d..caf86716b803 100644 --- a/node/collation-generation/src/lib.rs +++ b/node/collation-generation/src/lib.rs @@ -30,11 +30,9 @@ #![deny(missing_docs)] use futures::{ - channel::{mpsc, oneshot}, - future::{Future, FutureExt}, + channel::oneshot, + future::FutureExt, join, select, - sink::SinkExt, - stream::StreamExt, }; use parity_scale_codec::Encode; use polkadot_node_primitives::{ @@ -95,26 +93,13 @@ impl CollationGenerationSubsystem { /// If `err_tx` is not `None`, errors are forwarded onto that channel as they occur. /// Otherwise, most are logged and then discarded. async fn run(mut self, mut ctx: Context) { - // when we activate new leaves, we spawn a bunch of sub-tasks, each of which is - // expected to generate precisely one message. We don't want to block the main loop - // at any point waiting for them all, so instead, we create a channel on which they can - // send those messages. We can then just monitor the channel and forward messages on it - // to the overseer here, via the context. - let (sender, receiver) = mpsc::channel(0); - - let mut receiver = receiver.fuse(); loop { select! { incoming = ctx.recv().fuse() => { - if self.handle_incoming::(incoming, &mut ctx, &sender).await { + if self.handle_incoming::(incoming, &mut ctx).await { break; } }, - msg = receiver.next() => { - if let Some(msg) = msg { - ctx.send_message(msg).await; - } - }, } } } @@ -127,7 +112,6 @@ impl CollationGenerationSubsystem { &mut self, incoming: SubsystemResult::Message>>, ctx: &mut Context, - sender: &mpsc::Sender, ) -> bool { match incoming { Ok(FromOrchestra::Signal(OverseerSignal::ActiveLeaves(ActiveLeavesUpdate { @@ -142,7 +126,6 @@ impl CollationGenerationSubsystem { activated.into_iter().map(|v| v.hash), ctx, metrics, - sender, ) .await { @@ -211,7 +194,6 @@ async fn handle_new_activations( activated: impl IntoIterator, ctx: &mut Context, metrics: Metrics, - sender: &mpsc::Sender, ) -> crate::error::Result<()> { // follow the procedure from the guide: // https://paritytech.github.io/polkadot/book/node/collators/collation-generation.html @@ -321,8 +303,8 @@ async fn handle_new_activations( }; let task_config = config.clone(); - let mut task_sender = sender.clone(); let metrics = metrics.clone(); + let mut task_sender = ctx.sender().clone(); ctx.spawn( "collation-builder", Box::pin(async move { @@ -354,15 +336,7 @@ async fn handle_new_activations( n_validators, }, task_config.key.clone(), - |msg| async move { - if let Err(err) = task_sender.send(msg.into()).await { - gum::warn!( - target: LOG_TARGET, - ?err, - "failed to send collation result" - ); - } - }, + &mut task_sender, result_sender, &metrics, ) @@ -444,7 +418,7 @@ async fn handle_submit_collation( construct_and_distribute_receipt( collation, config.key.clone(), - |msg| ctx.send_message(msg), + ctx.sender(), result_sender, metrics, ) @@ -464,10 +438,10 @@ struct PreparedCollation { /// Takes a prepared collation, along with its context, and produces a candidate receipt /// which is distributed to validators. -async fn construct_and_distribute_receipt>( +async fn construct_and_distribute_receipt( collation: PreparedCollation, key: CollatorPair, - distribute_collation: impl FnOnce(CollatorProtocolMessage) -> F, + sender: &mut impl overseer::CollationGenerationSenderTrait, result_sender: Option>, metrics: &Metrics, ) { @@ -565,13 +539,12 @@ async fn construct_and_distribute_receipt>( ); metrics.on_collation_generated(); - distribute_collation(CollatorProtocolMessage::DistributeCollation( + sender.send_message(CollatorProtocolMessage::DistributeCollation( ccr, parent_head_data_hash, pov, result_sender, - )) - .await; + )).await; } async fn obtain_validation_code_hash_with_hint( diff --git a/node/collation-generation/src/tests.rs b/node/collation-generation/src/tests.rs index 8c3852236820..762d37a72258 100644 --- a/node/collation-generation/src/tests.rs +++ b/node/collation-generation/src/tests.rs @@ -217,8 +217,6 @@ fn requests_availability_per_relay_parent() { } }; - let (tx, _rx) = mpsc::channel(0); - let subsystem_activated_hashes = activated_hashes.clone(); subsystem_test_harness(overseer, |mut ctx| async move { handle_new_activations( @@ -226,7 +224,6 @@ fn requests_availability_per_relay_parent() { subsystem_activated_hashes, &mut ctx, Metrics(None), - &tx, ) .await .unwrap(); @@ -296,15 +293,12 @@ fn requests_validation_data_for_scheduled_matches() { } }; - let (tx, _rx) = mpsc::channel(0); - subsystem_test_harness(overseer, |mut ctx| async move { handle_new_activations( Arc::new(test_config(16)), activated_hashes, &mut ctx, Metrics(None), - &tx, ) .await .unwrap(); @@ -330,6 +324,10 @@ fn sends_distribute_collation_message() { Hash::repeat_byte(16), ]; + // empty vec doesn't allocate on the heap, so it's ok we throw it away + let to_collator_protocol = Arc::new(Mutex::new(Vec::new())); + let inner_to_collator_protocol = to_collator_protocol.clone(); + let overseer = |mut handle: TestSubsystemContextHandle| async move { loop { match handle.try_recv().await { @@ -376,6 +374,9 @@ fn sends_distribute_collation_message() { ))) => { tx.send(Ok(Some(ValidationCode(vec![1, 2, 3]).hash()))).unwrap(); }, + Some(msg @ AllMessages::CollatorProtocol(_)) => { + inner_to_collator_protocol.lock().await.push(msg); + } Some(msg) => { panic!("didn't expect any other overseer requests; got {:?}", msg) }, @@ -386,23 +387,13 @@ fn sends_distribute_collation_message() { let config = Arc::new(test_config(16)); let subsystem_config = config.clone(); - let (tx, rx) = mpsc::channel(0); - - // empty vec doesn't allocate on the heap, so it's ok we throw it away - let sent_messages = Arc::new(Mutex::new(Vec::new())); - let subsystem_sent_messages = sent_messages.clone(); subsystem_test_harness(overseer, |mut ctx| async move { - handle_new_activations(subsystem_config, activated_hashes, &mut ctx, Metrics(None), &tx) + handle_new_activations(subsystem_config, activated_hashes, &mut ctx, Metrics(None)) .await .unwrap(); - - std::mem::drop(tx); - - // collect all sent messages - *subsystem_sent_messages.lock().await = rx.collect().await; }); - let mut sent_messages = Arc::try_unwrap(sent_messages) + let mut to_collator_protocol = Arc::try_unwrap(to_collator_protocol) .expect("subsystem should have shut down by now") .into_inner(); @@ -432,8 +423,8 @@ fn sends_distribute_collation_message() { validation_code_hash: expect_validation_code_hash, }; - assert_eq!(sent_messages.len(), 1); - match AllMessages::from(sent_messages.pop().unwrap()) { + assert_eq!(to_collator_protocol.len(), 1); + match AllMessages::from(to_collator_protocol.pop().unwrap()) { AllMessages::CollatorProtocol(CollatorProtocolMessage::DistributeCollation( CandidateReceipt { descriptor, .. }, _pov, @@ -478,6 +469,10 @@ fn fallback_when_no_validation_code_hash_api() { Hash::repeat_byte(16), ]; + // empty vec doesn't allocate on the heap, so it's ok we throw it away + let to_collator_protocol = Arc::new(Mutex::new(Vec::new())); + let inner_to_collator_protocol = to_collator_protocol.clone(); + let overseer = |mut handle: TestSubsystemContextHandle| async move { loop { match handle.try_recv().await { @@ -532,6 +527,9 @@ fn fallback_when_no_validation_code_hash_api() { ))) => { tx.send(Ok(Some(ValidationCode(vec![1, 2, 3])))).unwrap(); }, + Some(msg @ AllMessages::CollatorProtocol(_)) => { + inner_to_collator_protocol.lock().await.push(msg); + }, Some(msg) => { panic!("didn't expect any other overseer requests; got {:?}", msg) }, @@ -542,36 +540,26 @@ fn fallback_when_no_validation_code_hash_api() { let config = Arc::new(test_config(16u32)); let subsystem_config = config.clone(); - let (tx, rx) = mpsc::channel(0); - // empty vec doesn't allocate on the heap, so it's ok we throw it away - let sent_messages = Arc::new(Mutex::new(Vec::new())); - let subsystem_sent_messages = sent_messages.clone(); subsystem_test_harness(overseer, |mut ctx| async move { - handle_new_activations(subsystem_config, activated_hashes, &mut ctx, Metrics(None), &tx) + handle_new_activations(subsystem_config, activated_hashes, &mut ctx, Metrics(None)) .await .unwrap(); - - std::mem::drop(tx); - - *subsystem_sent_messages.lock().await = rx.collect().await; }); - let sent_messages = Arc::try_unwrap(sent_messages) + let to_collator_protocol = Arc::try_unwrap(to_collator_protocol) .expect("subsystem should have shut down by now") .into_inner(); let expect_validation_code_hash = ValidationCode(vec![1, 2, 3]).hash(); - assert_eq!(sent_messages.len(), 1); - match &sent_messages[0] { - overseer::CollationGenerationOutgoingMessages::CollatorProtocolMessage( - CollatorProtocolMessage::DistributeCollation( - CandidateReceipt { descriptor, .. }, - _pov, - .., - ), - ) => { + assert_eq!(to_collator_protocol.len(), 1); + match &to_collator_protocol[0] { + AllMessages::CollatorProtocol(CollatorProtocolMessage::DistributeCollation( + CandidateReceipt { descriptor, .. }, + _pov, + .., + )) => { assert_eq!(expect_validation_code_hash, descriptor.validation_code_hash); }, _ => panic!("received wrong message type"), From 7186a592411f2d4ad23ee3d68a49f99a519a7533 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Wed, 5 Jul 2023 16:49:33 +0200 Subject: [PATCH 18/19] fmt --- node/collation-generation/src/lib.rs | 20 +++++++++----------- node/collation-generation/src/tests.rs | 2 +- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/node/collation-generation/src/lib.rs b/node/collation-generation/src/lib.rs index caf86716b803..7c9e969b6191 100644 --- a/node/collation-generation/src/lib.rs +++ b/node/collation-generation/src/lib.rs @@ -29,11 +29,7 @@ #![deny(missing_docs)] -use futures::{ - channel::oneshot, - future::FutureExt, - join, select, -}; +use futures::{channel::oneshot, future::FutureExt, join, select}; use parity_scale_codec::Encode; use polkadot_node_primitives::{ AvailableData, Collation, CollationGenerationConfig, CollationSecondedSignal, PoV, @@ -539,12 +535,14 @@ async fn construct_and_distribute_receipt( ); metrics.on_collation_generated(); - sender.send_message(CollatorProtocolMessage::DistributeCollation( - ccr, - parent_head_data_hash, - pov, - result_sender, - )).await; + sender + .send_message(CollatorProtocolMessage::DistributeCollation( + ccr, + parent_head_data_hash, + pov, + result_sender, + )) + .await; } async fn obtain_validation_code_hash_with_hint( diff --git a/node/collation-generation/src/tests.rs b/node/collation-generation/src/tests.rs index 762d37a72258..b795dafeba21 100644 --- a/node/collation-generation/src/tests.rs +++ b/node/collation-generation/src/tests.rs @@ -376,7 +376,7 @@ fn sends_distribute_collation_message() { }, Some(msg @ AllMessages::CollatorProtocol(_)) => { inner_to_collator_protocol.lock().await.push(msg); - } + }, Some(msg) => { panic!("didn't expect any other overseer requests; got {:?}", msg) }, From 2230d474cc7fdbdc961eeb586d82a0c5bb12bc33 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Mon, 10 Jul 2023 16:25:04 +0200 Subject: [PATCH 19/19] remove ValidationCodeHashHint and related tests --- node/collation-generation/src/lib.rs | 93 +-- node/collation-generation/src/tests.rs | 668 +----------------- node/primitives/src/lib.rs | 20 +- .../node/collators/collation-generation.md | 3 - .../src/types/overseer-protocol.md | 19 +- 5 files changed, 13 insertions(+), 790 deletions(-) diff --git a/node/collation-generation/src/lib.rs b/node/collation-generation/src/lib.rs index 7c9e969b6191..8ee5b897ccc1 100644 --- a/node/collation-generation/src/lib.rs +++ b/node/collation-generation/src/lib.rs @@ -33,12 +33,10 @@ use futures::{channel::oneshot, future::FutureExt, join, select}; use parity_scale_codec::Encode; use polkadot_node_primitives::{ AvailableData, Collation, CollationGenerationConfig, CollationSecondedSignal, PoV, - SubmitCollationParams, ValidationCodeHashHint, + SubmitCollationParams, }; use polkadot_node_subsystem::{ - messages::{ - CollationGenerationMessage, CollatorProtocolMessage, RuntimeApiMessage, RuntimeApiRequest, - }, + messages::{CollationGenerationMessage, CollatorProtocolMessage}, overseer, ActiveLeavesUpdate, FromOrchestra, OverseerSignal, RuntimeApiError, SpawnedSubsystem, SubsystemContext, SubsystemError, SubsystemResult, }; @@ -47,9 +45,9 @@ use polkadot_node_subsystem_util::{ request_validation_code_hash, request_validators, }; use polkadot_primitives::{ - collator_signature_payload, vstaging::BackingState, CandidateCommitments, CandidateDescriptor, - CandidateReceipt, CollatorPair, CoreState, Hash, HeadData, Id as ParaId, - OccupiedCoreAssumption, PersistedValidationData, ValidationCodeHash, + collator_signature_payload, CandidateCommitments, CandidateDescriptor, CandidateReceipt, + CollatorPair, CoreState, Hash, Id as ParaId, OccupiedCoreAssumption, PersistedValidationData, + ValidationCodeHash, }; use sp_core::crypto::Pair; use std::sync::Arc; @@ -358,7 +356,7 @@ async fn handle_submit_collation( relay_parent, collation, parent_head, - validation_code_hash_hint: code_hint, + validation_code_hash, result_sender, } = params; @@ -389,19 +387,6 @@ async fn handle_submit_collation( validation_data.parent_head = parent_head; - let validation_code_hash = match obtain_validation_code_hash_with_hint( - relay_parent, - config.para_id, - code_hint, - &validation_data.parent_head, - ctx.sender(), - ) - .await? - { - None => return Ok(()), - Some(v) => v, - }; - let collation = PreparedCollation { collation, relay_parent, @@ -545,72 +530,6 @@ async fn construct_and_distribute_receipt( .await; } -async fn obtain_validation_code_hash_with_hint( - relay_parent: Hash, - para_id: ParaId, - hint: Option, - parent_head: &HeadData, - sender: &mut impl overseer::CollationGenerationSenderTrait, -) -> crate::error::Result> { - let parent_rp_number = match hint { - Some(ValidationCodeHashHint::Provided(hash)) => return Ok(Some(hash)), - Some(ValidationCodeHashHint::ParentBlockRelayParentNumber(n)) => Some(n), - None => None, - }; - - let maybe_backing_state = { - let (tx, rx) = oneshot::channel(); - sender - .send_message(RuntimeApiMessage::Request( - relay_parent, - RuntimeApiRequest::StagingParaBackingState(para_id, tx), - )) - .await; - - // Failure here means the runtime API doesn't exist. - match rx.await? { - Ok(Some(state)) => Some(state), - Err(RuntimeApiError::NotSupported { .. }) => None, - Ok(None) => return Ok(None), - Err(e) => return Err(e.into()), - } - }; - - if let Some(BackingState { constraints, pending_availability }) = maybe_backing_state { - let (future_trigger, future_code_hash) = match constraints.future_validation_code { - Some(x) => x, - None => return Ok(Some(constraints.validation_code_hash)), - }; - - // If not explicitly provided, search for the parent's relay-parent number in the - // set of candidates pending availability on-chain. - let parent_rp_number = parent_rp_number.or_else(|| { - pending_availability - .iter() - .find(|c| &c.commitments.head_data == parent_head) - .map(|c| c.relay_parent_number) - }); - - if parent_rp_number.map_or(false, |n| n >= future_trigger) { - Ok(Some(future_code_hash)) - } else { - Ok(Some(constraints.validation_code_hash)) - } - } else { - // This branch implies that asynchronous backing has not yet been enabled, - // so in that case cores can only be built on when free anyway. - let maybe_hash = request_validation_code_hash( - relay_parent, - para_id, - OccupiedCoreAssumption::Free, - sender, - ) - .await - .await??; - Ok(maybe_hash) - } -} - async fn obtain_validation_code_hash_with_assumption( relay_parent: Hash, para_id: ParaId, diff --git a/node/collation-generation/src/tests.rs b/node/collation-generation/src/tests.rs index b795dafeba21..b7ff4ec2a576 100644 --- a/node/collation-generation/src/tests.rs +++ b/node/collation-generation/src/tests.rs @@ -29,9 +29,7 @@ use polkadot_node_subsystem::{ use polkadot_node_subsystem_test_helpers::{subsystem_test_harness, TestSubsystemContextHandle}; use polkadot_node_subsystem_util::TimeoutExt; use polkadot_primitives::{ - vstaging::{BackingState, CandidatePendingAvailability, Constraints, InboundHrmpLimitations}, - BlockNumber, CandidateHash, CollatorPair, Id as ParaId, PersistedValidationData, ScheduledCore, - ValidationCode, + CollatorPair, HeadData, Id as ParaId, PersistedValidationData, ScheduledCore, ValidationCode, }; use sp_keyring::sr25519::Keyring as Sr25519Keyring; use std::pin::Pin; @@ -91,65 +89,6 @@ fn test_validation_data() -> PersistedValidationData { persisted_validation_data } -fn test_backing_constraints( - validation_code_hash: ValidationCodeHash, - future_code: Option<(BlockNumber, ValidationCodeHash)>, -) -> Constraints { - Constraints { - min_relay_parent_number: 0, - max_pov_size: 1024, - max_code_size: 1024, - ump_remaining: 1, - ump_remaining_bytes: 1, - max_ump_num_per_candidate: 1, - dmp_remaining_messages: Vec::new(), - hrmp_inbound: InboundHrmpLimitations { valid_watermarks: Vec::new() }, - hrmp_channels_out: Vec::new(), - max_hrmp_num_per_candidate: 1, - required_parent: vec![1].into(), - validation_code_hash, - upgrade_restriction: None, - future_validation_code: future_code, - } -} - -fn test_candidate_pending_availability( - para_id: ParaId, - relay_parent_number: BlockNumber, - head_data: HeadData, - validation_code_hash: ValidationCodeHash, -) -> CandidatePendingAvailability { - let collator_key = CollatorPair::generate().0; - - let descriptor = CandidateDescriptor { - para_id, - relay_parent: Hash::repeat_byte(relay_parent_number as u8), - collator: collator_key.public(), - persisted_validation_data_hash: Hash::repeat_byte(0), - pov_hash: Hash::repeat_byte(0), - erasure_root: Hash::repeat_byte(0), - signature: collator_key.sign(&[0]), - para_head: head_data.hash(), - validation_code_hash, - }; - let commitments = CandidateCommitments { - upward_messages: Default::default(), - horizontal_messages: Default::default(), - new_validation_code: None, - head_data, - processed_downward_messages: 0, - hrmp_watermark: relay_parent_number, - }; - - CandidatePendingAvailability { - candidate_hash: CandidateHash(Hash::repeat_byte(100)), - descriptor, - commitments, - relay_parent_number, - max_pov_size: 1024, - } -} - // Box + Unpin + Send struct TestCollator; @@ -575,7 +514,7 @@ fn submit_collation_is_no_op_before_initialization() { relay_parent: Hash::repeat_byte(0), collation: test_collation(), parent_head: vec![1, 2, 3].into(), - validation_code_hash_hint: None, + validation_code_hash: Hash::repeat_byte(1).into(), result_sender: None, }), }) @@ -611,287 +550,7 @@ fn submit_collation_leads_to_distribution() { relay_parent, collation: test_collation(), parent_head: vec![1, 2, 3].into(), - validation_code_hash_hint: Some(ValidationCodeHashHint::Provided( - validation_code_hash, - )), - result_sender: None, - }), - }) - .await; - - assert_matches!( - overseer_recv(&mut virtual_overseer).await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::Validators(tx))) => { - assert_eq!(rp, relay_parent); - let _ = tx.send(Ok(vec![ - Sr25519Keyring::Alice.public().into(), - Sr25519Keyring::Bob.public().into(), - Sr25519Keyring::Charlie.public().into(), - ])); - } - ); - - assert_matches!( - overseer_recv(&mut virtual_overseer).await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::PersistedValidationData(id, a, tx))) => { - assert_eq!(rp, relay_parent); - assert_eq!(id, para_id); - assert_eq!(a, OccupiedCoreAssumption::TimedOut); - - // Candidate receipt should be constructed with the real parent head. - let mut pvd = expected_pvd.clone(); - pvd.parent_head = vec![4, 5, 6].into(); - let _ = tx.send(Ok(Some(pvd))); - } - ); - - assert_matches!( - overseer_recv(&mut virtual_overseer).await, - AllMessages::CollatorProtocol(CollatorProtocolMessage::DistributeCollation( - ccr, - parent_head_data_hash, - .. - )) => { - assert_eq!(parent_head_data_hash, parent_head.hash()); - assert_eq!(ccr.descriptor().persisted_validation_data_hash, expected_pvd.hash()); - assert_eq!(ccr.descriptor().para_head, dummy_head_data().hash()); - assert_eq!(ccr.descriptor().validation_code_hash, validation_code_hash); - } - ); - - virtual_overseer - }); -} - -#[test] -fn missing_code_hash_hint_with_constraints() { - let relay_parent = Hash::repeat_byte(0); - let validation_code_hash = ValidationCodeHash::from(Hash::repeat_byte(42)); - let future_validation_code_hash = ValidationCodeHash::from(Hash::repeat_byte(43)); - let parent_head = HeadData::from(vec![1, 2, 3]); - let para_id = ParaId::from(5); - let expected_pvd = PersistedValidationData { - parent_head: parent_head.clone(), - relay_parent_number: 10, - relay_parent_storage_root: Hash::repeat_byte(1), - max_pov_size: 1024, - }; - - test_harness(|mut virtual_overseer| async move { - virtual_overseer - .send(FromOrchestra::Communication { - msg: CollationGenerationMessage::Initialize(test_config_no_collator(para_id)), - }) - .await; - - virtual_overseer - .send(FromOrchestra::Communication { - msg: CollationGenerationMessage::SubmitCollation(SubmitCollationParams { - relay_parent, - collation: test_collation(), - parent_head: vec![1, 2, 3].into(), - validation_code_hash_hint: None, - result_sender: None, - }), - }) - .await; - - assert_matches!( - overseer_recv(&mut virtual_overseer).await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::Validators(tx))) => { - assert_eq!(rp, relay_parent); - let _ = tx.send(Ok(vec![ - Sr25519Keyring::Alice.public().into(), - Sr25519Keyring::Bob.public().into(), - Sr25519Keyring::Charlie.public().into(), - ])); - } - ); - - assert_matches!( - overseer_recv(&mut virtual_overseer).await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::PersistedValidationData(id, a, tx))) => { - assert_eq!(rp, relay_parent); - assert_eq!(id, para_id); - assert_eq!(a, OccupiedCoreAssumption::TimedOut); - - // Candidate receipt should be constructed with the real parent head. - let mut pvd = expected_pvd.clone(); - pvd.parent_head = vec![4, 5, 6].into(); - let _ = tx.send(Ok(Some(pvd))); - } - ); - - assert_matches!( - overseer_recv(&mut virtual_overseer).await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::StagingParaBackingState(id, tx))) => { - assert_eq!(rp, relay_parent); - assert_eq!(id, para_id); - - // The parent candidate is pending availability and triggered a code upgrade, - // so our collated candidate should use the future code hash. - - let _ = tx.send(Ok(Some(BackingState { - constraints: test_backing_constraints( - validation_code_hash, - Some((9, future_validation_code_hash)), - ), - pending_availability: vec![test_candidate_pending_availability( - para_id, - 9, - parent_head.clone(), - validation_code_hash, - )], - }))); - } - ); - - assert_matches!( - overseer_recv(&mut virtual_overseer).await, - AllMessages::CollatorProtocol(CollatorProtocolMessage::DistributeCollation( - ccr, - parent_head_data_hash, - .. - )) => { - assert_eq!(parent_head_data_hash, parent_head.hash()); - assert_eq!(ccr.descriptor().persisted_validation_data_hash, expected_pvd.hash()); - assert_eq!(ccr.descriptor().para_head, dummy_head_data().hash()); - assert_eq!(ccr.descriptor().validation_code_hash, future_validation_code_hash); - } - ); - - virtual_overseer - }); -} - -#[test] -fn missing_code_hash_hint_with_constraints_no_code_upgrade() { - let relay_parent = Hash::repeat_byte(0); - let validation_code_hash = ValidationCodeHash::from(Hash::repeat_byte(42)); - let future_validation_code_hash = ValidationCodeHash::from(Hash::repeat_byte(43)); - let parent_head = HeadData::from(vec![1, 2, 3]); - let para_id = ParaId::from(5); - let expected_pvd = PersistedValidationData { - parent_head: parent_head.clone(), - relay_parent_number: 10, - relay_parent_storage_root: Hash::repeat_byte(1), - max_pov_size: 1024, - }; - - test_harness(|mut virtual_overseer| async move { - virtual_overseer - .send(FromOrchestra::Communication { - msg: CollationGenerationMessage::Initialize(test_config_no_collator(para_id)), - }) - .await; - - virtual_overseer - .send(FromOrchestra::Communication { - msg: CollationGenerationMessage::SubmitCollation(SubmitCollationParams { - relay_parent, - collation: test_collation(), - parent_head: vec![1, 2, 3].into(), - validation_code_hash_hint: None, - result_sender: None, - }), - }) - .await; - - assert_matches!( - overseer_recv(&mut virtual_overseer).await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::Validators(tx))) => { - assert_eq!(rp, relay_parent); - let _ = tx.send(Ok(vec![ - Sr25519Keyring::Alice.public().into(), - Sr25519Keyring::Bob.public().into(), - Sr25519Keyring::Charlie.public().into(), - ])); - } - ); - - assert_matches!( - overseer_recv(&mut virtual_overseer).await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::PersistedValidationData(id, a, tx))) => { - assert_eq!(rp, relay_parent); - assert_eq!(id, para_id); - assert_eq!(a, OccupiedCoreAssumption::TimedOut); - - // Candidate receipt should be constructed with the real parent head. - let mut pvd = expected_pvd.clone(); - pvd.parent_head = vec![4, 5, 6].into(); - let _ = tx.send(Ok(Some(pvd))); - } - ); - - assert_matches!( - overseer_recv(&mut virtual_overseer).await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::StagingParaBackingState(id, tx))) => { - assert_eq!(rp, relay_parent); - assert_eq!(id, para_id); - - // The parent candidate is pending availability but did not trigger a code upgrade, - // so our collated candidate should use the future code hash. - - let _ = tx.send(Ok(Some(BackingState { - constraints: test_backing_constraints( - validation_code_hash, - Some((9, future_validation_code_hash)), - ), - pending_availability: vec![test_candidate_pending_availability( - para_id, - 8, - parent_head.clone(), - validation_code_hash, - )], - }))); - } - ); - - assert_matches!( - overseer_recv(&mut virtual_overseer).await, - AllMessages::CollatorProtocol(CollatorProtocolMessage::DistributeCollation( - ccr, - parent_head_data_hash, - .. - )) => { - assert_eq!(parent_head_data_hash, parent_head.hash()); - assert_eq!(ccr.descriptor().persisted_validation_data_hash, expected_pvd.hash()); - assert_eq!(ccr.descriptor().para_head, dummy_head_data().hash()); - assert_eq!(ccr.descriptor().validation_code_hash, validation_code_hash); - } - ); - - virtual_overseer - }); -} - -#[test] -fn missing_code_hash_hint_without_constraints() { - let relay_parent = Hash::repeat_byte(0); - let validation_code_hash = ValidationCodeHash::from(Hash::repeat_byte(42)); - let parent_head = HeadData::from(vec![1, 2, 3]); - let para_id = ParaId::from(5); - let expected_pvd = PersistedValidationData { - parent_head: parent_head.clone(), - relay_parent_number: 10, - relay_parent_storage_root: Hash::repeat_byte(1), - max_pov_size: 1024, - }; - - test_harness(|mut virtual_overseer| async move { - virtual_overseer - .send(FromOrchestra::Communication { - msg: CollationGenerationMessage::Initialize(test_config_no_collator(para_id)), - }) - .await; - - virtual_overseer - .send(FromOrchestra::Communication { - msg: CollationGenerationMessage::SubmitCollation(SubmitCollationParams { - relay_parent, - collation: test_collation(), - parent_head: vec![1, 2, 3].into(), - validation_code_hash_hint: None, + validation_code_hash, result_sender: None, }), }) @@ -923,327 +582,6 @@ fn missing_code_hash_hint_without_constraints() { } ); - assert_matches!( - overseer_recv(&mut virtual_overseer).await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::StagingParaBackingState(id, tx))) => { - assert_eq!(rp, relay_parent); - assert_eq!(id, para_id); - - let _ = tx.send(Err(RuntimeApiError::NotSupported { runtime_api_name: "not_important" })); - } - ); - - assert_matches!( - overseer_recv(&mut virtual_overseer).await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::ValidationCodeHash(id, a, tx))) => { - assert_eq!(rp, relay_parent); - assert_eq!(id, para_id); - assert_eq!(a, OccupiedCoreAssumption::Free); - - let _ = tx.send(Ok(Some(validation_code_hash))); - } - ); - - assert_matches!( - overseer_recv(&mut virtual_overseer).await, - AllMessages::CollatorProtocol(CollatorProtocolMessage::DistributeCollation( - ccr, - parent_head_data_hash, - .. - )) => { - assert_eq!(parent_head_data_hash, parent_head.hash()); - assert_eq!(ccr.descriptor().persisted_validation_data_hash, expected_pvd.hash()); - assert_eq!(ccr.descriptor().para_head, dummy_head_data().hash()); - assert_eq!(ccr.descriptor().validation_code_hash, validation_code_hash); - } - ); - - virtual_overseer - }); -} - -#[test] -fn parent_rp_number_code_hash_hint_with_constraints() { - let relay_parent = Hash::repeat_byte(0); - let validation_code_hash = ValidationCodeHash::from(Hash::repeat_byte(42)); - let future_validation_code_hash = ValidationCodeHash::from(Hash::repeat_byte(43)); - let parent_head = HeadData::from(vec![1, 2, 3]); - let para_id = ParaId::from(5); - let expected_pvd = PersistedValidationData { - parent_head: parent_head.clone(), - relay_parent_number: 10, - relay_parent_storage_root: Hash::repeat_byte(1), - max_pov_size: 1024, - }; - let parent_rp_number = 9; - let code_upgrade_at = 9; - - test_harness(|mut virtual_overseer| async move { - virtual_overseer - .send(FromOrchestra::Communication { - msg: CollationGenerationMessage::Initialize(test_config_no_collator(para_id)), - }) - .await; - - virtual_overseer - .send(FromOrchestra::Communication { - msg: CollationGenerationMessage::SubmitCollation(SubmitCollationParams { - relay_parent, - collation: test_collation(), - parent_head: vec![1, 2, 3].into(), - validation_code_hash_hint: Some( - ValidationCodeHashHint::ParentBlockRelayParentNumber(parent_rp_number), - ), - result_sender: None, - }), - }) - .await; - - assert_matches!( - overseer_recv(&mut virtual_overseer).await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::Validators(tx))) => { - assert_eq!(rp, relay_parent); - let _ = tx.send(Ok(vec![ - Sr25519Keyring::Alice.public().into(), - Sr25519Keyring::Bob.public().into(), - Sr25519Keyring::Charlie.public().into(), - ])); - } - ); - - assert_matches!( - overseer_recv(&mut virtual_overseer).await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::PersistedValidationData(id, a, tx))) => { - assert_eq!(rp, relay_parent); - assert_eq!(id, para_id); - assert_eq!(a, OccupiedCoreAssumption::TimedOut); - - // Candidate receipt should be constructed with the real parent head. - let mut pvd = expected_pvd.clone(); - pvd.parent_head = vec![4, 5, 6].into(); - let _ = tx.send(Ok(Some(pvd))); - } - ); - - assert_matches!( - overseer_recv(&mut virtual_overseer).await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::StagingParaBackingState(id, tx))) => { - assert_eq!(rp, relay_parent); - assert_eq!(id, para_id); - - // The parent candidate is not on-chain but will have triggered a code upgrade nonetheless, - // so our collated candidate should use the future code hash. - - let _ = tx.send(Ok(Some(BackingState { - constraints: test_backing_constraints( - validation_code_hash, - Some((code_upgrade_at, future_validation_code_hash)), - ), - pending_availability: vec![], - }))); - } - ); - - assert_matches!( - overseer_recv(&mut virtual_overseer).await, - AllMessages::CollatorProtocol(CollatorProtocolMessage::DistributeCollation( - ccr, - parent_head_data_hash, - .. - )) => { - assert_eq!(parent_head_data_hash, parent_head.hash()); - assert_eq!(ccr.descriptor().persisted_validation_data_hash, expected_pvd.hash()); - assert_eq!(ccr.descriptor().para_head, dummy_head_data().hash()); - assert_eq!(ccr.descriptor().validation_code_hash, future_validation_code_hash); - } - ); - - virtual_overseer - }); -} - -#[test] -fn parent_rp_number_code_hash_hint_without_constraints() { - let relay_parent = Hash::repeat_byte(0); - let validation_code_hash = ValidationCodeHash::from(Hash::repeat_byte(42)); - let parent_head = HeadData::from(vec![1, 2, 3]); - let para_id = ParaId::from(5); - let expected_pvd = PersistedValidationData { - parent_head: parent_head.clone(), - relay_parent_number: 10, - relay_parent_storage_root: Hash::repeat_byte(1), - max_pov_size: 1024, - }; - let parent_rp_number = 8; - - test_harness(|mut virtual_overseer| async move { - virtual_overseer - .send(FromOrchestra::Communication { - msg: CollationGenerationMessage::Initialize(test_config_no_collator(para_id)), - }) - .await; - - virtual_overseer - .send(FromOrchestra::Communication { - msg: CollationGenerationMessage::SubmitCollation(SubmitCollationParams { - relay_parent, - collation: test_collation(), - parent_head: vec![1, 2, 3].into(), - validation_code_hash_hint: Some( - ValidationCodeHashHint::ParentBlockRelayParentNumber(parent_rp_number), - ), - result_sender: None, - }), - }) - .await; - - assert_matches!( - overseer_recv(&mut virtual_overseer).await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::Validators(tx))) => { - assert_eq!(rp, relay_parent); - let _ = tx.send(Ok(vec![ - Sr25519Keyring::Alice.public().into(), - Sr25519Keyring::Bob.public().into(), - Sr25519Keyring::Charlie.public().into(), - ])); - } - ); - - assert_matches!( - overseer_recv(&mut virtual_overseer).await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::PersistedValidationData(id, a, tx))) => { - assert_eq!(rp, relay_parent); - assert_eq!(id, para_id); - assert_eq!(a, OccupiedCoreAssumption::TimedOut); - - // Candidate receipt should be constructed with the real parent head. - let mut pvd = expected_pvd.clone(); - pvd.parent_head = vec![4, 5, 6].into(); - let _ = tx.send(Ok(Some(pvd))); - } - ); - - assert_matches!( - overseer_recv(&mut virtual_overseer).await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::StagingParaBackingState(id, tx))) => { - assert_eq!(rp, relay_parent); - assert_eq!(id, para_id); - - let _ = tx.send(Err(RuntimeApiError::NotSupported { runtime_api_name: "not_important" })); - } - ); - - assert_matches!( - overseer_recv(&mut virtual_overseer).await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::ValidationCodeHash(id, a, tx))) => { - assert_eq!(rp, relay_parent); - assert_eq!(id, para_id); - assert_eq!(a, OccupiedCoreAssumption::Free); - - let _ = tx.send(Ok(Some(validation_code_hash))); - } - ); - - assert_matches!( - overseer_recv(&mut virtual_overseer).await, - AllMessages::CollatorProtocol(CollatorProtocolMessage::DistributeCollation( - ccr, - parent_head_data_hash, - .. - )) => { - assert_eq!(parent_head_data_hash, parent_head.hash()); - assert_eq!(ccr.descriptor().persisted_validation_data_hash, expected_pvd.hash()); - assert_eq!(ccr.descriptor().para_head, dummy_head_data().hash()); - assert_eq!(ccr.descriptor().validation_code_hash, validation_code_hash); - } - ); - - virtual_overseer - }); -} - -#[test] -fn parent_rp_number_code_hash_hint_with_constraints_no_upgrade() { - let relay_parent = Hash::repeat_byte(0); - let validation_code_hash = ValidationCodeHash::from(Hash::repeat_byte(42)); - let future_validation_code_hash = ValidationCodeHash::from(Hash::repeat_byte(43)); - let parent_head = HeadData::from(vec![1, 2, 3]); - let para_id = ParaId::from(5); - let expected_pvd = PersistedValidationData { - parent_head: parent_head.clone(), - relay_parent_number: 10, - relay_parent_storage_root: Hash::repeat_byte(1), - max_pov_size: 1024, - }; - let parent_rp_number = 8; - let code_upgrade_at = 9; - - test_harness(|mut virtual_overseer| async move { - virtual_overseer - .send(FromOrchestra::Communication { - msg: CollationGenerationMessage::Initialize(test_config_no_collator(para_id)), - }) - .await; - - virtual_overseer - .send(FromOrchestra::Communication { - msg: CollationGenerationMessage::SubmitCollation(SubmitCollationParams { - relay_parent, - collation: test_collation(), - parent_head: vec![1, 2, 3].into(), - validation_code_hash_hint: Some( - ValidationCodeHashHint::ParentBlockRelayParentNumber(parent_rp_number), - ), - result_sender: None, - }), - }) - .await; - - assert_matches!( - overseer_recv(&mut virtual_overseer).await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::Validators(tx))) => { - assert_eq!(rp, relay_parent); - let _ = tx.send(Ok(vec![ - Sr25519Keyring::Alice.public().into(), - Sr25519Keyring::Bob.public().into(), - Sr25519Keyring::Charlie.public().into(), - ])); - } - ); - - assert_matches!( - overseer_recv(&mut virtual_overseer).await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::PersistedValidationData(id, a, tx))) => { - assert_eq!(rp, relay_parent); - assert_eq!(id, para_id); - assert_eq!(a, OccupiedCoreAssumption::TimedOut); - - // Candidate receipt should be constructed with the real parent head. - let mut pvd = expected_pvd.clone(); - pvd.parent_head = vec![4, 5, 6].into(); - let _ = tx.send(Ok(Some(pvd))); - } - ); - - assert_matches!( - overseer_recv(&mut virtual_overseer).await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request(rp, RuntimeApiRequest::StagingParaBackingState(id, tx))) => { - assert_eq!(rp, relay_parent); - assert_eq!(id, para_id); - - // The parent candidate is not on-chain and will not have used the code upgrade - // so our collated candidate should use the future code hash. - - let _ = tx.send(Ok(Some(BackingState { - constraints: test_backing_constraints( - validation_code_hash, - Some((code_upgrade_at, future_validation_code_hash)), - ), - pending_availability: vec![], - }))); - } - ); - assert_matches!( overseer_recv(&mut virtual_overseer).await, AllMessages::CollatorProtocol(CollatorProtocolMessage::DistributeCollation( diff --git a/node/primitives/src/lib.rs b/node/primitives/src/lib.rs index d94dace48d2a..9f031d772b06 100644 --- a/node/primitives/src/lib.rs +++ b/node/primitives/src/lib.rs @@ -502,21 +502,6 @@ impl std::fmt::Debug for CollationGenerationConfig { } } -/// Hints for the collation-generation subsystem to determine the correct -/// validation code hash to put into the candidate receipt. -/// -/// Providing an incorrect hint or no hint at all could lead the candidate to have the -/// wrong code provided, but only in the few blocks surrounding validation code upgrades. -/// such a candidate receipt would be rejected by validators. -#[derive(Debug, Clone)] -pub enum ValidationCodeHashHint { - /// Contains the number of the relay-chain block used as a relay-parent - /// for the collation's parent block. - ParentBlockRelayParentNumber(BlockNumber), - /// Contains explicitly the validation code hash to use in the candidate receipt. - Provided(ValidationCodeHash), -} - /// Parameters for [`CollationGenerationMessage::SubmitCollation`]. #[derive(Debug)] pub struct SubmitCollationParams { @@ -526,9 +511,8 @@ pub struct SubmitCollationParams { pub collation: Collation, /// The parent block's head-data. pub parent_head: HeadData, - /// The validation code hash hint. If no hint is provided, a best effort will be made - /// against the relay-parent's state. - pub validation_code_hash_hint: Option, + /// The hash of the validation code the collation was created against. + pub validation_code_hash: ValidationCodeHash, /// An optional result sender that should be informed about a successfully seconded collation. /// /// There is no guarantee that this sender is informed ever about any result, it is completely okay to just drop it. diff --git a/roadmap/implementers-guide/src/node/collators/collation-generation.md b/roadmap/implementers-guide/src/node/collators/collation-generation.md index 03588c4b316f..9053ea40f89e 100644 --- a/roadmap/implementers-guide/src/node/collators/collation-generation.md +++ b/roadmap/implementers-guide/src/node/collators/collation-generation.md @@ -28,9 +28,6 @@ Collation generation for Parachains currently works in the following way: - `CollationGenerationMessage::SubmitCollation` - If the subsystem isn't initialized or the relay-parent is too old to be relevant, ignore the message. - Otherwise, use the provided parameters to generate a [`CommittedCandidateReceipt`] - - If no `ValidationCodeHashHint` is given, use the current validation code hash from the relay-chain state, unless the parent-head of the collation equals the block pending availability for the parachain. In that case, use the pending future code if that block would trigger a code upgrade. - - If `ValidationCodeHashHint::Provided` is given, use that. - - If `ValidationCodeHashHint::ParentBlockRelayParentNumber` is given, determine whether the given parent block's relay parent number would trigger a code upgrade. If so, use the future code. If not, use the current code. - Submit the collation to the collator-protocol with `CollatorProtocolMessage::DistributeCollation`. ### Outgoing diff --git a/roadmap/implementers-guide/src/types/overseer-protocol.md b/roadmap/implementers-guide/src/types/overseer-protocol.md index ffecd3528e43..1fc0c505a1cc 100644 --- a/roadmap/implementers-guide/src/types/overseer-protocol.md +++ b/roadmap/implementers-guide/src/types/overseer-protocol.md @@ -428,20 +428,6 @@ struct CollationGenerationConfig { para_id: ParaId, } -/// Hints for the collation-generation subsystem to determine the correct -/// validation code hash to put into the candidate receipt. -/// -/// Providing an incorrect hint or no hint at all could lead the candidate to have the -/// wrong code provided, but only in the few blocks surrounding validation code upgrades. -/// such a candidate receipt would be rejected by validators. -enum ValidationCodeHashHint { - /// Contains the number of the relay-chain block used as a relay-parent - /// for the collation's parent block. - ParentBlockRelayParentNumber(BlockNumber), - /// Contains explicitly the validation code hash to use in the candidate receipt. - Provided(ValidationCodeHash), -} - /// Parameters for submitting a collation struct SubmitCollationParams { /// The relay-parent the collation is built against. @@ -450,9 +436,8 @@ struct SubmitCollationParams { collation: Collation, /// The parent block's head-data. parent_head: HeadData, - /// The validation code hash hint. If no hint is provided, a best effort will be made - /// against the relay-parent's state. - validation_code_hash: Option, + /// The hash of the validation code the collation was created against. + validation_code_hash: ValidationCodeHash, /// A response channel for receiving a `Seconded` message about the candidate /// once produced by a validator. This is not guaranteed to provide anything. result_sender: Option>,