Change the way we store, handle and validate the validation data (#342)

Currently validation data is shared by using a well known key between
the parachain system pallet and the validate block implementation. This
pr changes this by passing the parachain system directly to the validate
block implementation to make use of it. Besides that, we also store the
validation params in some thread local variable to make it inspectable
by parachain system. This moves the validation of validation data and
validation params to the parachain system pallet directly, instead of
having this hidden inside the validate block implementation.

Fixes: https://github.com/paritytech/cumulus/issues/217

Cargo.lock

@@ -1238,6 +1238,7 @@ dependencies = [
  "cumulus-test-client",
  "cumulus-test-relay-sproof-builder",
  "env_logger 0.7.1",
+ "environmental",
  "frame-executive",
  "frame-support",
  "frame-system",

pallets/parachain-system/Cargo.toml

@@ -34,6 +34,7 @@ serde = { version = "1.0.101", optional = true, features = ["derive"] }
 hash-db = { version = "0.15.2", default-features = false }
 memory-db = { version = "0.26.0", default-features = false }
 trie-db = { version = "0.22.0", default-features = false }
+environmental = { version = "1.1.2", default-features = false }
 
 [dev-dependencies]
 substrate-test-runtime-client = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" }
@@ -70,4 +71,5 @@ std = [
 	"frame-executive/std",
 	"cumulus-primitives-core/std",
 	"cumulus-primitives-parachain-inherent/std",
+	"environmental/std",
 ]

pallets/parachain-system/src/lib.rs

@@ -29,7 +29,7 @@
 
 use cumulus_primitives_core::{
 	relay_chain,
-	well_known_keys::{self, NEW_VALIDATION_CODE, VALIDATION_DATA},
+	well_known_keys::{self, NEW_VALIDATION_CODE},
 	AbridgedHostConfiguration, DownwardMessageHandler, HrmpMessageHandler, HrmpMessageSender,
 	InboundDownwardMessage, InboundHrmpMessage, OnValidationData, OutboundHrmpMessage, ParaId,
 	PersistedValidationData, UpwardMessage, UpwardMessageSender,
@@ -82,8 +82,8 @@ decl_storage! {
 		PendingValidationFunction get(fn new_validation_function):
 			Option<(RelayChainBlockNumber, Vec<u8>)>;
 
-		/// Were the [`ValidationData`] updated in this block?
-		DidUpdateValidationData: bool;
+		/// The [`PersistedValidationData`] set for this block.
+		ValidationData get(fn validation_data): Option<PersistedValidationData>;
 
 		/// Were the validation data set to notify the relay chain?
 		DidSetValidationCode: bool;
@@ -168,7 +168,10 @@ decl_module! {
 		#[weight = (0, DispatchClass::Mandatory)]
 		fn set_validation_data(origin, data: ParachainInherentData) -> DispatchResult {
 			ensure_none(origin)?;
-			assert!(!DidUpdateValidationData::exists(), "ValidationData must be updated only once in a block");
+			assert!(
+				!ValidationData::exists(),
+				"ValidationData must be updated only once in a block",
+			);
 
 			let ParachainInherentData {
 				validation_data: vfp,
@@ -177,6 +180,8 @@ decl_module! {
 				horizontal_messages,
 			} = data;
 
+			Self::validate_validation_data(&vfp);
+
 			// initialization logic: we know that this runs exactly once every block,
 			// which means we can put the initialization logic here to remove the
 			// sequencing problem.
@@ -200,8 +205,7 @@ decl_module! {
 					Error::<T>::InvalidRelayChainMerkleProof
 				})?;
 
-			storage::unhashed::put(VALIDATION_DATA, &vfp);
-			DidUpdateValidationData::put(true);
+			ValidationData::put(&vfp);
 			RelevantMessagingState::put(relevant_messaging_state.clone());
 			HostConfiguration::put(host_config);
 
@@ -232,7 +236,6 @@ decl_module! {
 		}
 
 		fn on_finalize() {
-			assert!(DidUpdateValidationData::take(), "VFPs must be updated once per block");
 			DidSetValidationCode::take();
 
 			let host_config = Self::host_configuration()
@@ -396,7 +399,8 @@ decl_module! {
 				storage::unhashed::kill(NEW_VALIDATION_CODE);
 			}
 
-			storage::unhashed::kill(VALIDATION_DATA);
+			// Remove the validation from the old block.
+			ValidationData::kill();
 
 			let mut weight = T::DbWeight::get().writes(3);
 			storage::unhashed::kill(well_known_keys::HRMP_WATERMARK);
@@ -440,6 +444,30 @@ decl_module! {
 }
 
 impl<T: Config> Module<T> {
+	/// Validate the given [`PersistedValidationData`] against the
+	/// [`ValidationParams`](polkadot_parachain::primitives::ValidationParams).
+	///
+	/// This check will only be executed when the block is currently being executed in the context
+	/// of [`validate_block`]. If this is being executed in the context of block building or block
+	/// import, this is a no-op.
+	///
+	/// # Panics
+	fn validate_validation_data(validation_data: &PersistedValidationData) {
+		validate_block::with_validation_params(|params| {
+			assert_eq!(params.parent_head, validation_data.parent_head, "Parent head doesn't match");
+			assert_eq!(
+				params.relay_parent_number,
+				validation_data.relay_parent_number,
+				"Relay parent number doesn't match",
+			);
+			assert_eq!(
+				params.relay_parent_storage_root,
+				validation_data.relay_parent_storage_root,
+				"Relay parent stoarage root doesn't match",
+			);
+		});
+	}
+
 	/// Process all inbound downward messages relayed by the collator.
 	///
 	/// Checks if the sequence of the messages is valid, dispatches them and communicates the number
@@ -561,13 +589,6 @@ impl<T: Config> Module<T> {
 		Ok(())
 	}
 
-	/// Get validation data.
-	///
-	/// Returns `Some(_)` after the inherent set the data for the current block.
-	pub fn validation_data() -> Option<PersistedValidationData> {
-		storage::unhashed::get(VALIDATION_DATA)
-	}
-
 	/// Put a new validation function into a particular location where polkadot
 	/// monitors for updates. Calling this function notifies polkadot that a new
 	/// upgrade has been scheduled.
@@ -1129,7 +1150,7 @@ mod tests {
 						hook(self, *n as RelayChainBlockNumber, &mut vfp);
 					}
 
-					storage::unhashed::put(VALIDATION_DATA, &vfp);
+					ValidationData::put(&vfp);
 					storage::unhashed::kill(NEW_VALIDATION_CODE);
 
 					// It is insufficient to push the validation function params

pallets/parachain-system/src/validate_block/implementation.rs

@@ -19,19 +19,21 @@
 use frame_executive::ExecuteBlock;
 use sp_runtime::traits::{Block as BlockT, HashFor, Header as HeaderT, NumberFor};
 
-use sp_std::{boxed::Box, vec::Vec};
 use sp_io::KillChildStorageResult;
+use sp_std::{boxed::Box, vec::Vec};
 
 use hash_db::{HashDB, EMPTY_PREFIX};
 
-use polkadot_parachain::primitives::{HeadData, ValidationCode, ValidationParams, ValidationResult};
+use polkadot_parachain::primitives::{
+	HeadData, ValidationCode, ValidationParams, ValidationResult,
+};
 
 use codec::{Decode, Encode};
 
 use cumulus_primitives_core::{
 	well_known_keys::{
 		HRMP_OUTBOUND_MESSAGES, HRMP_WATERMARK, NEW_VALIDATION_CODE, PROCESSED_DOWNWARD_MESSAGES,
-		UPWARD_MESSAGES, VALIDATION_DATA,
+		UPWARD_MESSAGES,
 	},
 	OutboundHrmpMessage, PersistedValidationData, UpwardMessage,
 };
@@ -42,9 +44,6 @@ use sp_externalities::{
 use sp_std::any::{Any, TypeId};
 use sp_trie::MemoryDB;
 
-type StorageValue = Vec<u8>;
-type StorageKey = Vec<u8>;
-
 type Ext<'a, B> = sp_state_machine::Ext<
 	'a,
 	HashFor<B>,
@@ -56,18 +55,13 @@ fn with_externalities<F: FnOnce(&mut dyn Externalities) -> R, R>(f: F) -> R {
 	sp_externalities::with_externalities(f).expect("Environmental externalities not set.")
 }
 
-/// Implement `Encode` by forwarding the stored raw vec.
-struct EncodeOpaqueValue(Vec<u8>);
-
-impl Encode for EncodeOpaqueValue {
-	fn using_encoded<R, F: FnOnce(&[u8]) -> R>(&self, f: F) -> R {
-		f(&self.0)
-	}
-}
+type ParachainSystem<PSC> = crate::Module::<PSC>;
 
 /// Validate a given parachain block on a validator.
 #[doc(hidden)]
-pub fn validate_block<B: BlockT, E: ExecuteBlock<B>>(params: ValidationParams) -> ValidationResult {
+pub fn validate_block<B: BlockT, E: ExecuteBlock<B>, PSC: crate::Config>(
+	params: ValidationParams,
+) -> ValidationResult {
 	let block_data =
 		cumulus_primitives_core::ParachainBlockData::<B>::decode(&mut &params.block_data.0[..])
 			.expect("Invalid parachain block data");
@@ -93,10 +87,7 @@ pub fn validate_block<B: BlockT, E: ExecuteBlock<B>>(params: ValidationParams) -
 	let backend = sp_state_machine::TrieBackend::new(db, root);
 	let mut overlay = sp_state_machine::OverlayedChanges::default();
 	let mut cache = Default::default();
-	let mut ext = WitnessExt::<B> {
-		inner: Ext::<B>::new(&mut overlay, &mut cache, &backend),
-		params: &params,
-	};
+	let mut ext = Ext::<B>::new(&mut overlay, &mut cache, &backend);
 
 	let _guard = (
 		// Replace storage calls with our own implementations
@@ -138,8 +129,13 @@ pub fn validate_block<B: BlockT, E: ExecuteBlock<B>>(params: ValidationParams) -
 		sp_io::offchain_index::host_clear.replace_implementation(host_offchain_index_clear),
 	);
 
-	set_and_run_with_externalities(&mut ext, || {
+	let validation_data = set_and_run_with_externalities(&mut ext, || {
+		super::set_and_run_with_validation_params(params, || {
 			E::execute_block(block);
+
+			ParachainSystem::<PSC>::validation_data()
+				.expect("`PersistedValidationData` should be set in every block!")
+		})
 	});
 
 	// If in the course of block execution new validation code was set, insert
@@ -166,12 +162,6 @@ pub fn validate_block<B: BlockT, E: ExecuteBlock<B>>(params: ValidationParams) -
 		})
 		.unwrap_or_default();
 
-	let validation_data: PersistedValidationData = overlay
-		.storage(VALIDATION_DATA)
-		.flatten()
-		.and_then(|v| Decode::decode(&mut &v[..]).ok())
-		.expect("`PersistedValidationData` is required to be placed into the storage!");
-
 	let horizontal_messages = match overlay.storage(HRMP_OUTBOUND_MESSAGES).flatten() {
 		Some(encoded) => Vec::<OutboundHrmpMessage>::decode(&mut &encoded[..])
 			.expect("Outbound HRMP messages vec is not correctly encoded in the storage!"),
@@ -194,174 +184,6 @@ pub fn validate_block<B: BlockT, E: ExecuteBlock<B>>(params: ValidationParams) -
 	}
 }
 
-/// The storage implementation used when validating a block that is using the
-/// witness data as source.
-struct WitnessExt<'a, B: BlockT> {
-	inner: Ext<'a, B>,
-	params: &'a ValidationParams,
-}
-
-impl<'a, B: BlockT> WitnessExt<'a, B> {
-	/// Checks that the encoded `PersistedValidationData` in `data` is correct.
-	///
-	/// Should be removed with: https://github.com/paritytech/cumulus/issues/217
-	/// When removed `WitnessExt` could also be removed.
-	fn check_validation_data(&self, mut data: &[u8]) {
-		let validation_data =
-			PersistedValidationData::decode(&mut data).expect("Invalid `PersistedValidationData`");
-
-		assert_eq!(self.params.parent_head, validation_data.parent_head,);
-		assert_eq!(
-			self.params.relay_parent_number,
-			validation_data.relay_parent_number,
-		);
-		assert_eq!(
-			self.params.relay_parent_storage_root,
-			validation_data.relay_parent_storage_root,
-		);
-	}
-}
-
-impl<'a, B: BlockT> Externalities for WitnessExt<'a, B> {
-	fn storage(&self, key: &[u8]) -> Option<StorageValue> {
-		self.inner.storage(key)
-	}
-
-	fn set_offchain_storage(&mut self, key: &[u8], value: Option<&[u8]>) {
-		self.inner.set_offchain_storage(key, value)
-	}
-
-	fn storage_hash(&self, key: &[u8]) -> Option<Vec<u8>> {
-		self.inner.storage_hash(key)
-	}
-
-	fn child_storage(&self, child_info: &ChildInfo, key: &[u8]) -> Option<StorageValue> {
-		self.inner.child_storage(child_info, key)
-	}
-
-	fn child_storage_hash(&self, child_info: &ChildInfo, key: &[u8]) -> Option<Vec<u8>> {
-		self.inner.child_storage_hash(child_info, key)
-	}
-
-	fn exists_storage(&self, key: &[u8]) -> bool {
-		self.inner.exists_storage(key)
-	}
-
-	fn exists_child_storage(&self, child_info: &ChildInfo, key: &[u8]) -> bool {
-		self.inner.exists_child_storage(child_info, key)
-	}
-
-	fn next_storage_key(&self, key: &[u8]) -> Option<StorageKey> {
-		self.inner.next_storage_key(key)
-	}
-
-	fn next_child_storage_key(&self, child_info: &ChildInfo, key: &[u8]) -> Option<StorageKey> {
-		self.inner.next_child_storage_key(child_info, key)
-	}
-
-	fn place_storage(&mut self, key: StorageKey, value: Option<StorageValue>) {
-		if let Some(value) = value.as_ref() {
-			if key == VALIDATION_DATA {
-				self.check_validation_data(value);
-			}
-		}
-
-		self.inner.place_storage(key, value)
-	}
-
-	fn place_child_storage(
-		&mut self,
-		child_info: &ChildInfo,
-		key: StorageKey,
-		value: Option<StorageValue>,
-	) {
-		self.inner.place_child_storage(child_info, key, value)
-	}
-
-	fn kill_child_storage(&mut self, child_info: &ChildInfo, limit: Option<u32>) -> (bool, u32) {
-		self.inner.kill_child_storage(child_info, limit)
-	}
-
-	fn clear_prefix(&mut self, prefix: &[u8]) {
-		self.inner.clear_prefix(prefix)
-	}
-
-	fn clear_child_prefix(&mut self, child_info: &ChildInfo, prefix: &[u8]) {
-		self.inner.clear_child_prefix(child_info, prefix)
-	}
-
-	fn storage_append(&mut self, key: Vec<u8>, value: Vec<u8>) {
-		self.inner.storage_append(key, value)
-	}
-
-	fn storage_root(&mut self) -> Vec<u8> {
-		self.inner.storage_root()
-	}
-
-	fn child_storage_root(&mut self, child_info: &ChildInfo) -> Vec<u8> {
-		self.inner.child_storage_root(child_info)
-	}
-
-	fn storage_changes_root(&mut self, parent_hash: &[u8]) -> Result<Option<Vec<u8>>, ()> {
-		self.inner.storage_changes_root(parent_hash)
-	}
-
-	fn storage_start_transaction(&mut self) {
-		self.inner.storage_start_transaction()
-	}
-
-	fn storage_rollback_transaction(&mut self) -> Result<(), ()> {
-		self.inner.storage_rollback_transaction()
-	}
-
-	fn storage_commit_transaction(&mut self) -> Result<(), ()> {
-		self.inner.storage_commit_transaction()
-	}
-
-	fn wipe(&mut self) {
-		self.inner.wipe()
-	}
-
-	fn commit(&mut self) {
-		self.inner.commit()
-	}
-
-	fn read_write_count(&self) -> (u32, u32, u32, u32) {
-		self.inner.read_write_count()
-	}
-
-	fn reset_read_write_count(&mut self) {
-		self.inner.reset_read_write_count()
-	}
-
-	fn get_whitelist(&self) -> Vec<TrackedStorageKey> {
-		self.inner.get_whitelist()
-	}
-
-	fn set_whitelist(&mut self, new: Vec<TrackedStorageKey>) {
-		self.inner.set_whitelist(new)
-	}
-}
-
-impl<'a, B: BlockT> ExtensionStore for WitnessExt<'a, B> {
-	fn extension_by_type_id(&mut self, type_id: TypeId) -> Option<&mut dyn Any> {
-		self.inner.extension_by_type_id(type_id)
-	}
-
-	fn register_extension_with_type_id(
-		&mut self,
-		type_id: TypeId,
-		extension: Box<dyn Extension>,
-	) -> Result<(), Error> {
-		self.inner
-			.register_extension_with_type_id(type_id, extension)
-	}
-
-	fn deregister_extension_by_type_id(&mut self, type_id: TypeId) -> Result<(), Error> {
-		self.inner.deregister_extension_by_type_id(type_id)
-	}
-}
-
 fn host_storage_read(key: &[u8], value_out: &mut [u8], value_offset: u32) -> Option<u32> {
 	match with_externalities(|ext| ext.storage(key)) {
 		Some(value) => {

pallets/parachain-system/src/validate_block/mod.rs

@@ -16,6 +16,8 @@
 
 //! A module that enables a runtime to work as parachain.
 
+use polkadot_parachain::primitives::ValidationParams;
+
 #[cfg(not(feature = "std"))]
 #[doc(hidden)]
 pub mod implementation;
@@ -25,28 +27,50 @@ mod tests;
 #[cfg(not(feature = "std"))]
 #[doc(hidden)]
 pub use polkadot_parachain;
+#[cfg(not(feature = "std"))]
+#[doc(hidden)]
+pub use sp_runtime::traits::GetRuntimeBlockType;
+
+// Stores the [`ValidationParams`] that are being passed to `validate_block`.
+//
+// This value will only be set when a parachain validator validates a given `PoV`.
+environmental::environmental!(VALIDATION_PARAMS: ValidationParams);
+
+/// Execute the given closure with the [`ValidationParams`].
+///
+/// Returns `None` if the [`ValidationParams`] are not set, because the code is currently not being
+/// executed in the context of `validate_block`.
+pub(crate) fn with_validation_params<R>(f: impl FnOnce(&ValidationParams) -> R) -> Option<R> {
+	VALIDATION_PARAMS::with(|v| f(v))
+}
+
+/// Set the [`ValidationParams`] for the local context and execute the given closure in this context.
+#[cfg(not(feature = "std"))]
+fn set_and_run_with_validation_params<R>(mut params: ValidationParams, f: impl FnOnce() -> R) -> R {
+	VALIDATION_PARAMS::using(&mut params, f)
+}
 
 /// Register the `validate_block` function that is used by parachains to validate blocks on a
 /// validator.
 ///
 /// Does *nothing* when `std` feature is enabled.
 ///
-/// Expects as parameters the block and the block executor.
+/// Expects as parameters the runtime and a block executor.
 ///
 /// # Example
 ///
 /// ```
-///     struct Block;
 ///     struct BlockExecutor;
+///     struct Runtime;
 ///
-///     cumulus_pallet_parachain_system::register_validate_block!(Block, BlockExecutor);
+///     cumulus_pallet_parachain_system::register_validate_block!(Runtime, BlockExecutor);
 ///
 /// # fn main() {}
 /// ```
 #[macro_export]
 macro_rules! register_validate_block {
-	($block:ty, $block_executor:ty) => {
-		$crate::register_validate_block_impl!($block, $block_executor);
+	($runtime:ty, $block_executor:ty) => {
+		$crate::register_validate_block_impl!($runtime, $block_executor);
 	};
 }
 
@@ -55,19 +79,22 @@ macro_rules! register_validate_block {
 #[doc(hidden)]
 #[macro_export]
 macro_rules! register_validate_block_impl {
-	($block:ty, $block_executor:ty) => {
+	($runtime:ty, $block_executor:ty) => {
 		#[doc(hidden)]
 		mod parachain_validate_block {
 			use super::*;
 
 			#[no_mangle]
 			unsafe fn validate_block(arguments: *const u8, arguments_len: usize) -> u64 {
-				let params =
-					$crate::validate_block::polkadot_parachain::load_params(arguments, arguments_len);
+				let params = $crate::validate_block::polkadot_parachain::load_params(
+					arguments,
+					arguments_len,
+				);
 
 				let res = $crate::validate_block::implementation::validate_block::<
-					$block,
+					<$runtime as $crate::validate_block::GetRuntimeBlockType>::RuntimeBlock,
 					$block_executor,
+					$runtime,
 				>(params);
 
 				$crate::validate_block::polkadot_parachain::write_result(&res)
@@ -81,5 +108,5 @@ macro_rules! register_validate_block_impl {
 #[doc(hidden)]
 #[macro_export]
 macro_rules! register_validate_block_impl {
-	($block:ty, $block_executor:ty) => {};
+	($runtime:ty, $block_executor:ty) => {};
 }

pallets/parachain-system/src/validate_block/tests.rs

@@ -14,7 +14,7 @@
 // You should have received a copy of the GNU General Public License
 // along with Cumulus.  If not, see <http://www.gnu.org/licenses/>.
 
-use cumulus_primitives_core::{PersistedValidationData, ParachainBlockData};
+use cumulus_primitives_core::{ParachainBlockData, PersistedValidationData};
 use cumulus_test_client::{
 	runtime::{Block, Hash, Header, UncheckedExtrinsic, WASM_BINARY},
 	transfer, Client, DefaultTestClientBuilderExt, InitBlockBuilder, LongestChain,
@@ -84,7 +84,7 @@ fn create_test_client() -> (Client, LongestChain) {
 struct TestBlockData {
 	block: Block,
 	witness: sp_trie::StorageProof,
-	relay_parent_storage_root: Hash,
+	validation_data: PersistedValidationData,
 }
 
 fn build_block_with_witness(
@@ -95,15 +95,15 @@ fn build_block_with_witness(
 	let sproof_builder = RelayStateSproofBuilder::default();
 	let (relay_parent_storage_root, _) = sproof_builder.clone().into_state_root_and_proof();
 	let block_id = BlockId::Hash(client.info().best_hash);
-	let mut builder = client.init_block_builder_at(
-		&block_id,
-		Some(PersistedValidationData {
+	let mut validation_data = PersistedValidationData {
 		relay_parent_number: 1,
 		parent_head: parent_head.encode().into(),
 		..Default::default()
-		}),
-		sproof_builder,
-	);
+	};
+	let mut builder =
+		client.init_block_builder_at(&block_id, Some(validation_data.clone()), sproof_builder);
+
+	validation_data.relay_parent_storage_root = relay_parent_storage_root;
 
 	extra_extrinsics
 		.into_iter()
@@ -116,7 +116,7 @@ fn build_block_with_witness(
 		witness: built_block
 			.proof
 			.expect("We enabled proof recording before."),
-		relay_parent_storage_root,
+		validation_data,
 	}
 }
 
@@ -129,13 +129,17 @@ fn validate_block_no_extra_extrinsics() {
 	let TestBlockData {
 		block,
 		witness,
-		relay_parent_storage_root,
+		validation_data,
 	} = build_block_with_witness(&client, vec![], parent_head.clone());
 	let (header, extrinsics) = block.deconstruct();
 
 	let block_data = ParachainBlockData::new(header.clone(), extrinsics, witness);
 
-	let res_header = call_validate_block(parent_head, block_data, relay_parent_storage_root)
+	let res_header = call_validate_block(
+		parent_head,
+		block_data,
+		validation_data.relay_parent_storage_root,
+	)
 	.expect("Calls `validate_block`");
 	assert_eq!(header, res_header);
 }
@@ -155,13 +159,17 @@ fn validate_block_with_extra_extrinsics() {
 	let TestBlockData {
 		block,
 		witness,
-		relay_parent_storage_root,
+		validation_data,
 	} = build_block_with_witness(&client, extra_extrinsics, parent_head.clone());
 	let (header, extrinsics) = block.deconstruct();
 
 	let block_data = ParachainBlockData::new(header.clone(), extrinsics, witness);
 
-	let res_header = call_validate_block(parent_head, block_data, relay_parent_storage_root)
+	let res_header = call_validate_block(
+		parent_head,
+		block_data,
+		validation_data.relay_parent_storage_root,
+	)
 	.expect("Calls `validate_block`");
 	assert_eq!(header, res_header);
 }
@@ -176,12 +184,39 @@ fn validate_block_invalid_parent_hash() {
 	let TestBlockData {
 		block,
 		witness,
-		relay_parent_storage_root,
+		validation_data,
 	} = build_block_with_witness(&client, vec![], parent_head.clone());
 	let (mut header, extrinsics) = block.deconstruct();
 	header.set_parent_hash(Hash::from_low_u64_be(1));
 
 	let block_data = ParachainBlockData::new(header, extrinsics, witness);
-	call_validate_block(parent_head, block_data, relay_parent_storage_root)
+	call_validate_block(
+		parent_head,
+		block_data,
+		validation_data.relay_parent_storage_root,
+	)
+	.expect("Calls `validate_block`");
+}
+
+#[test]
+#[should_panic(expected = "Calls `validate_block`: Other(\"Trap: Trap { kind: Unreachable }\")")]
+fn validate_block_fails_on_invalid_validation_data() {
+	let _ = env_logger::try_init();
+
+	let (client, longest_chain) = create_test_client();
+	let parent_head = longest_chain.best_chain().expect("Best block exists");
+	let TestBlockData {
+		block,
+		witness,
+		..
+	} = build_block_with_witness(&client, vec![], parent_head.clone());
+	let (header, extrinsics) = block.deconstruct();
+
+	let block_data = ParachainBlockData::new(header, extrinsics, witness);
+	call_validate_block(
+		parent_head,
+		block_data,
+		Hash::random(),
+	)
 	.expect("Calls `validate_block`");
 }

primitives/core/src/lib.rs

@@ -46,9 +46,6 @@ pub mod well_known_keys {
 	/// The upward messages are stored as SCALE encoded `Vec<UpwardMessage>`.
 	pub const UPWARD_MESSAGES: &'static [u8] = b":cumulus_upward_messages:";
 
-	/// Current validation data.
-	pub const VALIDATION_DATA: &'static [u8] = b":cumulus_validation_data:";
-
 	/// Code upgarde (set as appropriate by a pallet).
 	pub const NEW_VALIDATION_CODE: &'static [u8] = b":cumulus_new_validation_code:";
 

rococo-parachains/runtime/src/lib.rs

@@ -415,4 +415,4 @@ impl_runtime_apis! {
 	}
 }
 
-cumulus_pallet_parachain_system::register_validate_block!(Block, Executive);
+cumulus_pallet_parachain_system::register_validate_block!(Runtime, Executive);

test/runtime/src/lib.rs

@@ -371,4 +371,4 @@ impl_runtime_apis! {
 	}
 }
 
-cumulus_pallet_parachain_system::register_validate_block!(Block, Executive);
+cumulus_pallet_parachain_system::register_validate_block!(Runtime, Executive);