diff --git a/polkadot/node/collation-generation/src/lib.rs b/polkadot/node/collation-generation/src/lib.rs index b948f3d3c8..2c430c17ef 100644 --- a/polkadot/node/collation-generation/src/lib.rs +++ b/polkadot/node/collation-generation/src/lib.rs @@ -284,7 +284,6 @@ async fn handle_new_activations( 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( 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); diff --git a/polkadot/node/core/backing/src/lib.rs b/polkadot/node/core/backing/src/lib.rs index 11736be9b0..91ffbed9e5 100644 --- a/polkadot/node/core/backing/src/lib.rs +++ b/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 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( + // 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, candidate_hash: CandidateHash, validation_data: polkadot_primitives::v1::PersistedValidationData, - outputs: ValidationOutputs, - with_commitments: impl FnOnce(CandidateCommitments) -> Result, - ) -> Result, Error> { + expected_erasure_root: Hash, + ) -> Result, 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(), diff --git a/polkadot/node/core/candidate-validation/src/lib.rs b/polkadot/node/core/candidate-validation/src/lib.rs index 103e27ecd8..3c64f962c2 100644 --- a/polkadot/node/core/candidate-validation/src/lib.rs +++ b/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( 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, diff --git a/polkadot/node/core/runtime-api/src/lib.rs b/polkadot/node/core/runtime-api/src/lib.rs index f9e40a40ac..5fb8ac73cb 100644 --- a/polkadot/node/core/runtime-api/src/lib.rs +++ b/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(¶_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); diff --git a/polkadot/node/network/availability-distribution/src/lib.rs b/polkadot/node/network/availability-distribution/src/lib.rs index 493d5bfbfc..4ca045a7bc 100644 --- a/polkadot/node/network/availability-distribution/src/lib.rs +++ b/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, diff --git a/polkadot/node/network/availability-distribution/src/tests.rs b/polkadot/node/network/availability-distribution/src/tests.rs index a7309043b0..b55c7a2241 100644 --- a/polkadot/node/network/availability-distribution/src/tests.rs +++ b/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, diff --git a/polkadot/node/primitives/src/lib.rs b/polkadot/node/primitives/src/lib.rs index 0b2262da93..0ea2799daa 100644 --- a/polkadot/node/primitives/src/lib.rs +++ b/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), } diff --git a/polkadot/node/subsystem/src/messages.rs b/polkadot/node/subsystem/src/messages.rs index 979364b13c..eef51fe47a 100644 --- a/polkadot/node/subsystem/src/messages.rs +++ b/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, ), /// Get the session index that a child of the block will have. diff --git a/polkadot/primitives/src/v1.rs b/polkadot/primitives/src/v1.rs index 9644c65ffc..947c595036 100644 --- a/polkadot/primitives/src/v1.rs +++ b/polkadot/primitives/src/v1.rs @@ -142,6 +142,8 @@ pub struct CandidateDescriptor { 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 { 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, - /// The horizontal messages sent by the parachain. - pub horizontal_messages: Vec>, - /// The new validation code submitted by the execution, if any. - pub new_validation_code: Option, - /// 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 { pub upward_messages: Vec, /// Horizontal messages sent by the parachain. pub horizontal_messages: Vec>, - /// The root of a block's erasure encoding Merkle tree. - pub erasure_root: Hash, /// New validation code. pub new_validation_code: Option, /// The head-data produced as a result of execution. @@ -761,7 +743,7 @@ sp_api::decl_runtime_apis! { -> Option>; /// 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. /// diff --git a/polkadot/roadmap/implementers-guide/src/types/candidate.md b/polkadot/roadmap/implementers-guide/src/types/candidate.md index 566fa7f9e7..86c80153f3 100644 --- a/polkadot/roadmap/implementers-guide/src/types/candidate.md +++ b/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, /// Messages destined to be interpreted by the Relay chain itself. upward_messages: Vec, - /// The root of a block's erasure encoding Merkle tree. - erasure_root: Hash, /// New validation code. new_validation_code: Option, /// 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, - /// Upwards messages to the relay chain. - upwards_messages: Vec, - /// The new validation code submitted by the execution, if any. - new_validation_code: Option, - /// 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, -} -``` +``` \ No newline at end of file diff --git a/polkadot/roadmap/implementers-guide/src/types/overseer-protocol.md b/polkadot/roadmap/implementers-guide/src/types/overseer-protocol.md index 942d597533..77a94891f2 100644 --- a/polkadot/roadmap/implementers-guide/src/types/overseer-protocol.md +++ b/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, } diff --git a/polkadot/runtime/kusama/src/lib.rs b/polkadot/runtime/kusama/src/lib.rs index 0aa1a4cadb..14b6af0394 100644 --- a/polkadot/runtime/kusama/src/lib.rs +++ b/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 } diff --git a/polkadot/runtime/parachains/src/inclusion.rs b/polkadot/runtime/parachains/src/inclusion.rs index 8bca04f4f4..39a2cf71ce 100644 --- a/polkadot/runtime/parachains/src/inclusion.rs +++ b/polkadot/runtime/parachains/src/inclusion.rs @@ -562,7 +562,7 @@ impl Module { /// 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::::new().check_validation_outputs( para_id, diff --git a/polkadot/runtime/parachains/src/runtime_api_impl/v1.rs b/polkadot/runtime/parachains/src/runtime_api_impl/v1.rs index abecbbaeb1..8b7fe7331d 100644 --- a/polkadot/runtime/parachains/src/runtime_api_impl/v1.rs +++ b/polkadot/runtime/parachains/src/runtime_api_impl/v1.rs @@ -221,7 +221,7 @@ pub fn persisted_validation_data( /// Implementation for the `check_validation_outputs` function of the runtime API. pub fn check_validation_outputs( para_id: ParaId, - outputs: primitives::v1::ValidationOutputs, + outputs: primitives::v1::CandidateCommitments, ) -> bool { >::check_validation_outputs(para_id, outputs) } diff --git a/polkadot/runtime/polkadot/src/lib.rs b/polkadot/runtime/polkadot/src/lib.rs index 361f5240ff..a91fdbda77 100644 --- a/polkadot/runtime/polkadot/src/lib.rs +++ b/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 } diff --git a/polkadot/runtime/rococo/src/lib.rs b/polkadot/runtime/rococo/src/lib.rs index 2cc0a83398..d008463145 100644 --- a/polkadot/runtime/rococo/src/lib.rs +++ b/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::(para_id, outputs) } diff --git a/polkadot/runtime/test-runtime/src/lib.rs b/polkadot/runtime/test-runtime/src/lib.rs index abfb734ccd..aa20079cc4 100644 --- a/polkadot/runtime/test-runtime/src/lib.rs +++ b/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::(para_id, outputs) } diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index 1b79cc69ed..92151ce62f 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/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 }