Move erasure root out of candidate commitments and into descriptor (#2010)

* guide: move erasure-root to candidate descriptor

* primitives: move erasure root to descriptor

* guide: unify candidate commitments and validation outputs

* primitives: unify validation outputs and candidate commitments

* parachains-runtime: fix fallout

* runtimes: fix fallout

* collation generation: fix fallout

* fix stray reference in primitives

* fix fallout in node-primitives

* fix remaining fallout in collation generation

* fix fallout in candidate validation

* fix fallout in runtime API subsystem

* fix fallout in subsystem messages

* fix fallout in candidate backing

* fix fallout in availability distribution

* don't clone

* clone

Co-authored-by: Sergei Shulepov <sergei@parity.io>

polkadot/node/collation-generation/src/lib.rs

@@ -284,7 +284,6 @@ async fn handle_new_activations<Context: SubsystemContext>(
 					horizontal_messages: collation.horizontal_messages,
 					new_validation_code: collation.new_validation_code,
 					head_data: collation.head_data,
-					erasure_root,
 					processed_downward_messages: collation.processed_downward_messages,
 					hrmp_watermark: collation.hrmp_watermark,
 				};
@@ -298,6 +297,7 @@ async fn handle_new_activations<Context: SubsystemContext>(
 						collator: task_config.key.public(),
 						persisted_validation_data_hash,
 						pov_hash,
+						erasure_root,
 					},
 				};
 
@@ -702,6 +702,7 @@ mod tests {
 				collator: config.key.public(),
 				persisted_validation_data_hash: expect_validation_data_hash,
 				pov_hash: expect_pov_hash,
+				erasure_root: Default::default(), // this isn't something we're checking right now
 			};
 
 			assert_eq!(sent_messages.len(), 1);
@@ -728,6 +729,7 @@ mod tests {
 					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);

polkadot/node/core/backing/src/lib.rs

@@ -31,8 +31,7 @@ use polkadot_primitives::v1::{
 	CommittedCandidateReceipt, BackedCandidate, Id as ParaId, ValidatorId,
 	ValidatorIndex, SigningContext, PoV, CandidateHash,
 	CandidateDescriptor, AvailableData, ValidatorSignature, Hash, CandidateReceipt,
-	CandidateCommitments, CoreState, CoreIndex, CollatorId, ValidationOutputs,
-	ValidityAttestation,
+	CoreState, CoreIndex, CollatorId, ValidityAttestation,
 };
 use polkadot_node_primitives::{
 	FromTableMisbehavior, Statement, SignedFullStatement, MisbehaviorReport, ValidationResult,
@@ -229,6 +228,8 @@ impl TryFrom<AllMessages> for FromJob {
 	}
 }
 
+struct InvalidErasureRoot;
+
 // It looks like it's not possible to do an `impl From` given the current state of
 // the code. So this does the necessary conversion.
 fn primitive_statement_to_table(s: &SignedFullStatement) -> TableSignedStatement {
@@ -347,36 +348,38 @@ impl CandidateBackingJob {
 		let candidate_hash = candidate.hash();
 
 		let statement = match valid {
-			ValidationResult::Valid(outputs, validation_data) => {
+			ValidationResult::Valid(commitments, validation_data) => {
 				// make PoV available for later distribution. Send data to the availability
 				// store to keep. Sign and dispatch `valid` statement to network if we
 				// have not seconded the given candidate.
 				//
 				// If the commitments hash produced by validation is not the same as given by
 				// the collator, do not make available and report the collator.
-				let commitments_check = self.make_pov_available(
-					pov,
-					candidate_hash,
-					validation_data,
-					outputs,
-					|commitments| if commitments.hash() == candidate.commitments_hash {
-						Ok(CommittedCandidateReceipt {
-							descriptor: candidate.descriptor().clone(),
-							commitments,
-						})
-					} else {
-						Err(())
-					},
-				).await?;
+				if candidate.commitments_hash != commitments.hash() {
+					self.issue_candidate_invalid_message(candidate.clone()).await?;
+					None
+				} else {
+					let erasure_valid = self.make_pov_available(
+						pov,
+						candidate_hash,
+						validation_data,
+						candidate.descriptor.erasure_root,
+					).await?;
 
-				match commitments_check {
-					Ok(candidate) => {
-						self.issued_statements.insert(candidate_hash);
-						Some(Statement::Seconded(candidate))
-					}
-					Err(()) => {
-						self.issue_candidate_invalid_message(candidate.clone()).await?;
-						None
+					match erasure_valid {
+						Ok(()) => {
+							let candidate = CommittedCandidateReceipt {
+								descriptor: candidate.descriptor().clone(),
+								commitments,
+							};
+
+							self.issued_statements.insert(candidate_hash);
+							Some(Statement::Seconded(candidate))
+						}
+						Err(InvalidErasureRoot) => {
+							self.issue_candidate_invalid_message(candidate.clone()).await?;
+							None
+						}
 					}
 				}
 			}
@@ -566,6 +569,7 @@ impl CandidateBackingJob {
 		// and not just those things that the function uses.
 		let candidate = self.table.get_candidate(&candidate_hash).ok_or(Error::CandidateNotFound)?;
 		let expected_commitments = candidate.commitments.clone();
+		let expected_erasure_root = candidate.descriptor.erasure_root;
 
 		let descriptor = candidate.descriptor().clone();
 
@@ -585,23 +589,22 @@ impl CandidateBackingJob {
 		let v = self.request_candidate_validation(descriptor, pov.clone()).await?;
 
 		let statement = match v {
-			ValidationResult::Valid(outputs, validation_data) => {
+			ValidationResult::Valid(commitments, validation_data) => {
 				// If validation produces a new set of commitments, we vote the candidate as invalid.
-				let commitments_check = self.make_pov_available(
-					pov,
-					candidate_hash,
-					validation_data,
-					outputs,
-					|commitments| if commitments == expected_commitments {
-						Ok(())
-					} else {
-						Err(())
-					}
-				).await?;
+				if commitments != expected_commitments {
+					Statement::Invalid(candidate_hash)
+				} else {
+					let erasure_valid = self.make_pov_available(
+						pov,
+						candidate_hash,
+						validation_data,
+						expected_erasure_root,
+					).await?;
 
-				match commitments_check {
-					Ok(()) => Statement::Valid(candidate_hash),
-					Err(()) => Statement::Invalid(candidate_hash),
+					match erasure_valid {
+						Ok(()) => Statement::Valid(candidate_hash),
+						Err(InvalidErasureRoot) => Statement::Invalid(candidate_hash),
+					}
 				}
 			}
 			ValidationResult::Invalid(_reason) => {
@@ -733,18 +736,16 @@ impl CandidateBackingJob {
 
 	// Make a `PoV` available.
 	//
-	// This calls an inspection function before making the PoV available for any last checks
-	// that need to be done. If the inspection function returns an error, this function returns
-	// early without making the PoV available.
-	#[tracing::instrument(level = "trace", skip(self, pov, with_commitments), fields(subsystem = LOG_TARGET))]
-	async fn make_pov_available<T, E>(
+	// This will compute the erasure root internally and compare it to the expected erasure root.
+	// This returns `Err()` iff there is an internal error. Otherwise, it returns either `Ok(Ok(()))` or `Ok(Err(_))`.
+	#[tracing::instrument(level = "trace", skip(self, pov), fields(subsystem = LOG_TARGET))]
+	async fn make_pov_available(
 		&mut self,
 		pov: Arc<PoV>,
 		candidate_hash: CandidateHash,
 		validation_data: polkadot_primitives::v1::PersistedValidationData,
-		outputs: ValidationOutputs,
-		with_commitments: impl FnOnce(CandidateCommitments) -> Result<T, E>,
-	) -> Result<Result<T, E>, Error> {
+		expected_erasure_root: Hash,
+	) -> Result<Result<(), InvalidErasureRoot>, Error> {
 		let available_data = AvailableData {
 			pov,
 			validation_data,
@@ -758,20 +759,9 @@ impl CandidateBackingJob {
 		let branches = erasure_coding::branches(chunks.as_ref());
 		let erasure_root = branches.root();
 
-		let commitments = CandidateCommitments {
-			upward_messages: outputs.upward_messages,
-			horizontal_messages: outputs.horizontal_messages,
-			erasure_root,
-			new_validation_code: outputs.new_validation_code,
-			head_data: outputs.head_data,
-			processed_downward_messages: outputs.processed_downward_messages,
-			hrmp_watermark: outputs.hrmp_watermark,
-		};
-
-		let res = match with_commitments(commitments) {
-			Ok(x) => x,
-			Err(e) => return Ok(Err(e)),
-		};
+		if erasure_root != expected_erasure_root {
+			return Ok(Err(InvalidErasureRoot));
+		}
 
 		self.store_available_data(
 			self.table_context.validator.as_ref().map(|v| v.index()),
@@ -780,7 +770,7 @@ impl CandidateBackingJob {
 			available_data,
 		).await?;
 
-		Ok(Ok(res))
+		Ok(Ok(()))
 	}
 
 	async fn distribute_signed_statement(&mut self, s: SignedFullStatement) -> Result<(), Error> {
@@ -1183,11 +1173,11 @@ mod tests {
 					para_id: self.para_id,
 					pov_hash: self.pov_hash,
 					relay_parent: self.relay_parent,
+					erasure_root: self.erasure_root,
 					..Default::default()
 				},
 				commitments: CandidateCommitments {
 					head_data: self.head_data,
-					erasure_root: self.erasure_root,
 					..Default::default()
 				},
 			}
@@ -1290,7 +1280,7 @@ mod tests {
 					)
 				) if pov == pov && &c == candidate.descriptor() => {
 					tx.send(Ok(
-						ValidationResult::Valid(ValidationOutputs {
+						ValidationResult::Valid(CandidateCommitments {
 							head_data: expected_head_data.clone(),
 							horizontal_messages: Vec::new(),
 							upward_messages: Vec::new(),
@@ -1428,7 +1418,7 @@ mod tests {
 					)
 				) if pov == pov && &c == candidate_a.descriptor() => {
 					tx.send(Ok(
-						ValidationResult::Valid(ValidationOutputs {
+						ValidationResult::Valid(CandidateCommitments {
 							head_data: expected_head_data.clone(),
 							upward_messages: Vec::new(),
 							horizontal_messages: Vec::new(),
@@ -1579,7 +1569,7 @@ mod tests {
 					)
 				) if pov == pov && &c == candidate_a.descriptor() => {
 					tx.send(Ok(
-						ValidationResult::Valid(ValidationOutputs {
+						ValidationResult::Valid(CandidateCommitments {
 							head_data: expected_head_data.clone(),
 							upward_messages: Vec::new(),
 							horizontal_messages: Vec::new(),
@@ -1764,7 +1754,7 @@ mod tests {
 					)
 				) if pov == pov && &c == candidate_b.descriptor() => {
 					tx.send(Ok(
-						ValidationResult::Valid(ValidationOutputs {
+						ValidationResult::Valid(CandidateCommitments {
 							head_data: expected_head_data.clone(),
 							upward_messages: Vec::new(),
 							horizontal_messages: Vec::new(),

polkadot/node/core/candidate-validation/src/lib.rs

@@ -36,7 +36,7 @@ use polkadot_subsystem::errors::RuntimeApiError;
 use polkadot_node_primitives::{ValidationResult, InvalidCandidate};
 use polkadot_primitives::v1::{
 	ValidationCode, PoV, CandidateDescriptor, PersistedValidationData,
-	OccupiedCoreAssumption, Hash, ValidationOutputs,
+	OccupiedCoreAssumption, Hash, CandidateCommitments,
 };
 use polkadot_parachain::wasm_executor::{
 	self, IsolationStrategy, ValidationError, InvalidCandidate as WasmInvalidCandidate
@@ -458,7 +458,7 @@ fn validate_candidate_exhaustive<B: ValidationBackend, S: SpawnNamed + 'static>(
 			Ok(ValidationResult::Invalid(InvalidCandidate::ExecutionError(e.to_string()))),
 		Err(ValidationError::Internal(e)) => Err(ValidationFailed(e.to_string())),
 		Ok(res) => {
-			let outputs = ValidationOutputs {
+			let outputs = CandidateCommitments {
 				head_data: res.head_data,
 				upward_messages: res.upward_messages,
 				horizontal_messages: res.horizontal_messages,

polkadot/node/core/runtime-api/src/lib.rs

@@ -276,7 +276,7 @@ mod tests {
 			fn check_validation_outputs(
 				&self,
 				para_id: ParaId,
-				_commitments: polkadot_primitives::v1::ValidationOutputs,
+				_commitments: polkadot_primitives::v1::CandidateCommitments,
 			) -> bool {
 				self.validation_outputs_results
 					.get(&para_id)
@@ -498,7 +498,7 @@ mod tests {
 		let relay_parent = [1; 32].into();
 		let para_a = 5.into();
 		let para_b = 6.into();
-		let commitments = polkadot_primitives::v1::ValidationOutputs::default();
+		let commitments = polkadot_primitives::v1::CandidateCommitments::default();
 
 		runtime_api.validation_outputs_results.insert(para_a, false);
 		runtime_api.validation_outputs_results.insert(para_b, true);

polkadot/node/network/availability-distribution/src/lib.rs

@@ -627,7 +627,7 @@ where
 	};
 
 	// check the merkle proof
-	let root = &live_candidate.commitments.erasure_root;
+	let root = &live_candidate.descriptor.erasure_root;
 	let anticipated_hash = if let Ok(hash) = branch_hash(
 		root,
 		&message.erasure_chunk.proof,

polkadot/node/network/availability-distribution/src/tests.rs

@@ -290,11 +290,11 @@ impl TestCandidateBuilder {
 				para_id: self.para_id,
 				pov_hash: self.pov_hash,
 				relay_parent: self.relay_parent,
+				erasure_root: self.erasure_root,
 				..Default::default()
 			},
 			commitments: CandidateCommitments {
 				head_data: self.head_data,
-				erasure_root: self.erasure_root,
 				..Default::default()
 			},
 		}
@@ -323,7 +323,7 @@ fn helper_integrity() {
 	let message =
 		make_valid_availability_gossip(&test_state, candidate.hash(), 2, pov_block.clone());
 
-	let root = dbg!(&candidate.commitments.erasure_root);
+	let root = dbg!(&candidate.descriptor.erasure_root);
 
 	let anticipated_hash = branch_hash(
 		root,

polkadot/node/primitives/src/lib.rs

@@ -28,7 +28,7 @@ use polkadot_primitives::v1::{
 	Hash, CommittedCandidateReceipt, CandidateReceipt, CompactStatement,
 	EncodeAs, Signed, SigningContext, ValidatorIndex, ValidatorId,
 	UpwardMessage, ValidationCode, PersistedValidationData, ValidationData,
-	HeadData, PoV, CollatorPair, Id as ParaId, OutboundHrmpMessage, ValidationOutputs, CandidateHash,
+	HeadData, PoV, CollatorPair, Id as ParaId, OutboundHrmpMessage, CandidateCommitments, CandidateHash,
 };
 use polkadot_statement_table::{
 	generic::{
@@ -144,7 +144,7 @@ pub enum InvalidCandidate {
 pub enum ValidationResult {
 	/// Candidate is valid. The validation process yields these outputs and the persisted validation
 	/// data used to form inputs.
-	Valid(ValidationOutputs, PersistedValidationData),
+	Valid(CandidateCommitments, PersistedValidationData),
 	/// Candidate is invalid.
 	Invalid(InvalidCandidate),
 }

polkadot/node/subsystem/src/messages.rs

@@ -406,7 +406,7 @@ pub enum RuntimeApiRequest {
 	/// Sends back `true` if the validation outputs pass all acceptance criteria checks.
 	CheckValidationOutputs(
 		ParaId,
-		polkadot_primitives::v1::ValidationOutputs,
+		polkadot_primitives::v1::CandidateCommitments,
 		RuntimeApiSender<bool>,
 	),
 	/// Get the session index that a child of the block will have.

polkadot/primitives/src/v1.rs

@@ -142,6 +142,8 @@ pub struct CandidateDescriptor<H = Hash> {
 	pub persisted_validation_data_hash: Hash,
 	/// The blake2-256 hash of the pov.
 	pub pov_hash: Hash,
+	/// The root of a block's erasure encoding Merkle tree.
+	pub erasure_root: Hash,
 	/// Signature on blake2-256 of components of this receipt:
 	/// The parachain index, the relay parent, the validation data hash, and the pov_hash.
 	pub signature: CollatorSignature,
@@ -341,24 +343,6 @@ pub struct TransientValidationData<N = BlockNumber> {
 	pub dmq_length: u32,
 }
 
-/// Outputs of validating a candidate.
-#[derive(Encode, Decode)]
-#[cfg_attr(feature = "std", derive(Clone, Debug, Default))]
-pub struct ValidationOutputs {
-	/// The head-data produced by validation.
-	pub head_data: HeadData,
-	/// Upward messages to the relay chain.
-	pub upward_messages: Vec<UpwardMessage>,
-	/// The horizontal messages sent by the parachain.
-	pub horizontal_messages: Vec<OutboundHrmpMessage<Id>>,
-	/// The new validation code submitted by the execution, if any.
-	pub new_validation_code: Option<ValidationCode>,
-	/// The number of messages processed from the DMQ.
-	pub processed_downward_messages: u32,
-	/// The mark which specifies the block number up to which all inbound HRMP messages are processed.
-	pub hrmp_watermark: BlockNumber,
-}
-
 /// Commitments made in a `CandidateReceipt`. Many of these are outputs of validation.
 #[derive(PartialEq, Eq, Clone, Encode, Decode)]
 #[cfg_attr(feature = "std", derive(Debug, Default, Hash))]
@@ -367,8 +351,6 @@ pub struct CandidateCommitments<N = BlockNumber> {
 	pub upward_messages: Vec<UpwardMessage>,
 	/// Horizontal messages sent by the parachain.
 	pub horizontal_messages: Vec<OutboundHrmpMessage<Id>>,
-	/// The root of a block's erasure encoding Merkle tree.
-	pub erasure_root: Hash,
 	/// New validation code.
 	pub new_validation_code: Option<ValidationCode>,
 	/// The head-data produced as a result of execution.
@@ -761,7 +743,7 @@ sp_api::decl_runtime_apis! {
 			-> Option<PersistedValidationData<N>>;
 
 		/// Checks if the given validation outputs pass the acceptance criteria.
-		fn check_validation_outputs(para_id: Id, outputs: ValidationOutputs) -> bool;
+		fn check_validation_outputs(para_id: Id, outputs: CandidateCommitments) -> bool;
 
 		/// Returns the session index expected at a child of the block.
 		///

polkadot/roadmap/implementers-guide/src/types/candidate.md

@@ -80,6 +80,8 @@ struct CandidateDescriptor {
 	persisted_validation_data_hash: Hash,
 	/// The blake2-256 hash of the pov-block.
 	pov_hash: Hash,
+	/// The root of a block's erasure encoding Merkle tree.
+	erasure_root: Hash,
 	/// Signature on blake2-256 of components of this receipt:
 	/// The parachain index, the relay parent, the validation data hash, and the pov_hash.
 	signature: CollatorSignature,
@@ -251,8 +253,6 @@ struct CandidateCommitments {
 	horizontal_messages: Vec<OutboundHrmpMessage>,
 	/// Messages destined to be interpreted by the Relay chain itself.
 	upward_messages: Vec<UpwardMessage>,
-	/// The root of a block's erasure encoding Merkle tree.
-	erasure_root: Hash,
 	/// New validation code.
 	new_validation_code: Option<ValidationCode>,
 	/// The head-data produced as a result of execution.
@@ -275,27 +275,4 @@ struct SigningContext {
 	/// The session index this signature is in the context of.
 	session_index: SessionIndex,
 }
-```
-
-## Validation Outputs
-
-This struct encapsulates the outputs of candidate validation.
-
-```rust
-struct ValidationOutputs {
-	/// The head-data produced by validation.
-	head_data: HeadData,
-	/// The validation data, persisted.
-	validation_data: PersistedValidationData,
-	/// Messages directed to other paras routed via the relay chain.
-	horizontal_messages: Vec<OutboundHrmpMessage>,
-	/// Upwards messages to the relay chain.
-	upwards_messages: Vec<UpwardsMessage>,
-	/// The new validation code submitted by the execution, if any.
-	new_validation_code: Option<ValidationCode>,
-	/// The number of messages processed from the DMQ.
-	processed_downward_messages: u32,
-	/// The mark which specifies the block number up to which all inbound HRMP messages are processed.
-	hrmp_watermark: BlockNumber,
-}
-```
+```

polkadot/roadmap/implementers-guide/src/types/overseer-protocol.md

@@ -503,7 +503,7 @@ Various modules request that the [Candidate Validation subsystem](../node/utilit
 enum ValidationResult {
 	/// Candidate is valid, and here are the outputs and the validation data used to form inputs.
 	/// In practice, this should be a shared type so that validation caching can be done.
-	Valid(ValidationOutputs, PersistedValidationData),
+	Valid(CandidateCommitments, PersistedValidationData),
 	/// Candidate is invalid.
 	Invalid,
 }

polkadot/runtime/kusama/src/lib.rs

@@ -1083,7 +1083,7 @@ sp_api::impl_runtime_apis! {
 		}
 		fn check_validation_outputs(
 			_: Id,
-			_: primitives::v1::ValidationOutputs,
+			_: primitives::v1::CandidateCommitments,
 		) -> bool {
 			false
 		}

polkadot/runtime/parachains/src/inclusion.rs

@@ -562,7 +562,7 @@ impl<T: Trait> Module<T> {
 	/// Run the acceptance criteria checks on the given candidate commitments.
 	pub(crate) fn check_validation_outputs(
 		para_id: ParaId,
-		validation_outputs: primitives::v1::ValidationOutputs,
+		validation_outputs: primitives::v1::CandidateCommitments,
 	) -> bool {
 		if let Err(err) = CandidateCheckContext::<T>::new().check_validation_outputs(
 			para_id,

polkadot/runtime/parachains/src/runtime_api_impl/v1.rs

@@ -221,7 +221,7 @@ pub fn persisted_validation_data<T: initializer::Trait>(
 /// Implementation for the `check_validation_outputs` function of the runtime API.
 pub fn check_validation_outputs<T: initializer::Trait>(
 	para_id: ParaId,
-	outputs: primitives::v1::ValidationOutputs,
+	outputs: primitives::v1::CandidateCommitments,
 ) -> bool {
 	<inclusion::Module<T>>::check_validation_outputs(para_id, outputs)
 }

polkadot/runtime/polkadot/src/lib.rs

@@ -1078,7 +1078,7 @@ sp_api::impl_runtime_apis! {
 			None
 		}
 
-		fn check_validation_outputs(_: Id, _: primitives::v1::ValidationOutputs) -> bool {
+		fn check_validation_outputs(_: Id, _: primitives::v1::CandidateCommitments) -> bool {
 			false
 		}
 

polkadot/runtime/rococo/src/lib.rs

@@ -657,7 +657,7 @@ sp_api::impl_runtime_apis! {
 
 		fn check_validation_outputs(
 			para_id: Id,
-			outputs: primitives::v1::ValidationOutputs,
+			outputs: primitives::v1::CandidateCommitments,
 		) -> bool {
 			runtime_api_impl::check_validation_outputs::<Runtime>(para_id, outputs)
 		}

polkadot/runtime/test-runtime/src/lib.rs

@@ -650,7 +650,7 @@ sp_api::impl_runtime_apis! {
 
 		fn check_validation_outputs(
 			para_id: ParaId,
-			outputs: primitives::v1::ValidationOutputs,
+			outputs: primitives::v1::CandidateCommitments,
 		) -> bool {
 			runtime_impl::check_validation_outputs::<Runtime>(para_id, outputs)
 		}

polkadot/runtime/westend/src/lib.rs

@@ -831,7 +831,7 @@ sp_api::impl_runtime_apis! {
 
 		fn check_validation_outputs(
 			_: Id,
-			_: primitives::v1::ValidationOutputs
+			_: primitives::v1::CandidateCommitments
 		) -> bool {
 			false
 		}