From d5d92edcbf7d77986dc914ceee3eb5c2f713f8db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Tue, 10 Jan 2023 17:42:41 +0100 Subject: [PATCH] Refactor `validate_block` (#2069) * Refactor `validate_block` This pull request changes the `validate_block` implementation. One of the key changes are that we free data structures as early as possible. The memory while validating the block is scarce and we need to give as much as possible to the actual execution of the block. Besides that the pr moves the validation of the `validation_data` into the `validate_block` implementation completely instead of using this machinery with putting the data into some global variable that would then be read while executing the block. There are also some new docs to explain the internals of `validate_block`. * No clone wars!! * Integrate more feedback * FMT * Delay the header encoding --- .../parachain-system/proc-macro/src/lib.rs | 17 +- pallets/parachain-system/src/lib.rs | 30 --- .../src/validate_block/implementation.rs | 189 ++++++++++++------ .../src/validate_block/mod.rs | 43 ++-- .../src/validate_block/tests.rs | 32 ++- 5 files changed, 199 insertions(+), 112 deletions(-) diff --git a/pallets/parachain-system/proc-macro/src/lib.rs b/pallets/parachain-system/proc-macro/src/lib.rs index 855d7fed484..490b9c17c44 100644 --- a/pallets/parachain-system/proc-macro/src/lib.rs +++ b/pallets/parachain-system/proc-macro/src/lib.rs @@ -114,11 +114,20 @@ pub fn register_validate_block(input: proc_macro::TokenStream) -> proc_macro::To 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, + unsafe fn validate_block(arguments: *mut u8, arguments_len: usize) -> u64 { + // We convert the `arguments` into a boxed slice and then into `Bytes`. + let args = #crate_::validate_block::sp_std::boxed::Box::from_raw( + #crate_::validate_block::sp_std::slice::from_raw_parts_mut( + arguments, + arguments_len, + ) ); + let args = #crate_::validate_block::bytes::Bytes::from(args); + + // Then we decode from these bytes the `MemoryOptimizedValidationParams`. + let params = #crate_::validate_block::decode_from_bytes::< + #crate_::validate_block::MemoryOptimizedValidationParams + >(args).expect("Invalid arguments to `validate_block`."); let res = #crate_::validate_block::implementation::validate_block::< <#runtime as #crate_::validate_block::GetRuntimeBlockType>::RuntimeBlock, diff --git a/pallets/parachain-system/src/lib.rs b/pallets/parachain-system/src/lib.rs index 743c0c25372..be2a1426691 100644 --- a/pallets/parachain-system/src/lib.rs +++ b/pallets/parachain-system/src/lib.rs @@ -355,8 +355,6 @@ pub mod pallet { horizontal_messages, } = data; - Self::validate_validation_data(&vfp); - // Check that the associated relay chain block number is as expected. T::CheckAssociatedRelayNumber::check_associated_relay_number( vfp.relay_parent_number, @@ -769,34 +767,6 @@ impl GetChannelInfo for Pallet { } impl Pallet { - /// 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 - /// - /// Panics while validating the `PoV` on the relay chain if the [`PersistedValidationData`] - /// passed by the block author was incorrect. - 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 storage 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 diff --git a/pallets/parachain-system/src/validate_block/implementation.rs b/pallets/parachain-system/src/validate_block/implementation.rs index 8c03f012be2..25615ecb94b 100644 --- a/pallets/parachain-system/src/validate_block/implementation.rs +++ b/pallets/parachain-system/src/validate_block/implementation.rs @@ -16,18 +16,24 @@ //! The actual implementation of the validate block functionality. -use frame_support::traits::{ExecuteBlock, ExtrinsicCall, Get, IsSubType}; -use sp_runtime::traits::{Block as BlockT, Extrinsic, HashFor, Header as HeaderT}; - -use sp_io::KillStorageResult; -use sp_std::prelude::*; +use super::MemoryOptimizedValidationParams; +use cumulus_primitives_core::{ + relay_chain::v2::Hash as RHash, ParachainBlockData, PersistedValidationData, +}; +use cumulus_primitives_parachain_inherent::ParachainInherentData; -use polkadot_parachain::primitives::{HeadData, ValidationParams, ValidationResult}; +use polkadot_parachain::primitives::{ + HeadData, RelayChainBlockNumber, ValidationParams, ValidationResult, +}; use codec::{Decode, Encode}; +use frame_support::traits::{ExecuteBlock, ExtrinsicCall, Get, IsSubType}; use sp_core::storage::{ChildInfo, StateVersion}; use sp_externalities::{set_and_run_with_externalities, Externalities}; +use sp_io::KillStorageResult; +use sp_runtime::traits::{Block as BlockT, Extrinsic, HashFor, Header as HeaderT}; +use sp_std::{mem, prelude::*}; use sp_trie::MemoryDB; type TrieBackend = sp_state_machine::TrieBackend>, HashFor>; @@ -38,7 +44,32 @@ fn with_externalities R, R>(f: F) -> R { sp_externalities::with_externalities(f).expect("Environmental externalities not set.") } -/// Validate a given parachain block on a validator. +/// Validate the given parachain block. +/// +/// This function is doing roughly the following: +/// +/// 1. We decode the [`ParachainBlockData`] from the `block_data` in `params`. +/// +/// 2. We are doing some security checks like checking that the `parent_head` in `params` +/// is the parent of the block we are going to check. We also ensure that the `set_validation_data` +/// inherent is present in the block and that the validation data matches the values in `params`. +/// +/// 3. We construct the sparse in-memory database from the storage proof inside the block data and +/// then ensure that the storage root matches the storage root in the `parent_head`. +/// +/// 4. We replace all the storage related host functions with functions inside the wasm blob. +/// This means instead of calling into the host, we will stay inside the wasm execution. This is +/// very important as the relay chain validator hasn't the state required to verify the block. But +/// we have the in-memory database that contains all the values from the state of the parachain +/// that we require to verify the block. +/// +/// 5. We are going to run `check_inherents`. This is important to check stuff like the timestamp +/// matching the real world time. +/// +/// 6. The last step is to execute the entire block in the machinery we just have setup. Executing +/// the blocks include running all transactions in the block against our in-memory database and +/// ensuring that the final storage root matches the storage root in the header of the block. In the +/// end we return back the [`ValidationResult`] with all the required information for the validator. #[doc(hidden)] pub fn validate_block< B: BlockT, @@ -46,35 +77,49 @@ pub fn validate_block< PSC: crate::Config, CI: crate::CheckInherents, >( - params: ValidationParams, + MemoryOptimizedValidationParams { + block_data, + parent_head, + relay_parent_number, + relay_parent_storage_root, + }: MemoryOptimizedValidationParams, ) -> ValidationResult where B::Extrinsic: ExtrinsicCall, ::Call: IsSubType>, { - let block_data = - cumulus_primitives_core::ParachainBlockData::::decode(&mut ¶ms.block_data.0[..]) - .expect("Invalid parachain block data"); + let block_data = codec::decode_from_bytes::>(block_data) + .expect("Invalid parachain block data"); - let parent_head = - B::Header::decode(&mut ¶ms.parent_head.0[..]).expect("Invalid parent head"); + let parent_header = + codec::decode_from_bytes::(parent_head.clone()).expect("Invalid parent head"); let (header, extrinsics, storage_proof) = block_data.deconstruct(); - let head_data = HeadData(header.encode()); - let block = B::new(header, extrinsics); - assert!(parent_head.hash() == *block.header().parent_hash(), "Invalid parent hash",); + assert!(parent_header.hash() == *block.header().parent_hash(), "Invalid parent hash"); + + let inherent_data = extract_parachain_inherent_data(&block); + + validate_validation_data( + &inherent_data.validation_data, + relay_parent_number, + relay_parent_storage_root, + parent_head, + ); // Create the db - let db = match storage_proof.to_memory_db(Some(parent_head.state_root())) { + let db = match storage_proof.to_memory_db(Some(parent_header.state_root())) { Ok((db, _)) => db, Err(_) => panic!("Compact proof decoding failure."), }; sp_std::mem::drop(storage_proof); - let backend = sp_state_machine::TrieBackendBuilder::new(db, *parent_head.state_root()).build(); + // We use the storage root of the `parent_head` to ensure that it is the correct root. + // This is already being done above while creating the in-memory db, but let's be paranoid!! + let backend = + sp_state_machine::TrieBackendBuilder::new(db, *parent_header.state_root()).build(); let _guard = ( // Replace storage calls with our own implementations @@ -115,22 +160,6 @@ where sp_io::offchain_index::host_clear.replace_implementation(host_offchain_index_clear), ); - let inherent_data = block - .extrinsics() - .iter() - // Inherents are at the front of the block and are unsigned. - // - // If `is_signed` is returning `None`, we keep it safe and assume that it is "signed". - // We are searching for unsigned transactions anyway. - .take_while(|e| !e.is_signed().unwrap_or(true)) - .filter_map(|e| e.call().is_sub_type()) - .find_map(|c| match c { - crate::Call::set_validation_data { data: validation_data } => - Some(validation_data.clone()), - _ => None, - }) - .expect("Could not find `set_validation_data` inherent"); - run_with_externalities::(&backend, || { let relay_chain_proof = crate::RelayChainStateProof::new( PSC::SelfParaId::get(), @@ -153,34 +182,76 @@ where }); run_with_externalities::(&backend, || { - super::set_and_run_with_validation_params(params, || { - E::execute_block(block); - - let new_validation_code = crate::NewValidationCode::::get(); - let upward_messages = crate::UpwardMessages::::get(); - let processed_downward_messages = crate::ProcessedDownwardMessages::::get(); - let horizontal_messages = crate::HrmpOutboundMessages::::get(); - let hrmp_watermark = crate::HrmpWatermark::::get(); - - let head_data = - if let Some(custom_head_data) = crate::CustomValidationHeadData::::get() { - HeadData(custom_head_data) - } else { - head_data - }; - - ValidationResult { - head_data, - new_validation_code: new_validation_code.map(Into::into), - upward_messages, - processed_downward_messages, - horizontal_messages, - hrmp_watermark, - } - }) + let head_data = HeadData(block.header().encode()); + + E::execute_block(block); + + let new_validation_code = crate::NewValidationCode::::get(); + let upward_messages = crate::UpwardMessages::::get(); + let processed_downward_messages = crate::ProcessedDownwardMessages::::get(); + let horizontal_messages = crate::HrmpOutboundMessages::::get(); + let hrmp_watermark = crate::HrmpWatermark::::get(); + + let head_data = + if let Some(custom_head_data) = crate::CustomValidationHeadData::::get() { + HeadData(custom_head_data) + } else { + head_data + }; + + ValidationResult { + head_data, + new_validation_code: new_validation_code.map(Into::into), + upward_messages, + processed_downward_messages, + horizontal_messages, + hrmp_watermark, + } }) } +/// Extract the [`ParachainInherentData`]. +fn extract_parachain_inherent_data( + block: &B, +) -> &ParachainInherentData +where + B::Extrinsic: ExtrinsicCall, + ::Call: IsSubType>, +{ + block + .extrinsics() + .iter() + // Inherents are at the front of the block and are unsigned. + // + // If `is_signed` is returning `None`, we keep it safe and assume that it is "signed". + // We are searching for unsigned transactions anyway. + .take_while(|e| !e.is_signed().unwrap_or(true)) + .filter_map(|e| e.call().is_sub_type()) + .find_map(|c| match c { + crate::Call::set_validation_data { data: validation_data } => Some(validation_data), + _ => None, + }) + .expect("Could not find `set_validation_data` inherent") +} + +/// Validate the given [`PersistedValidationData`] against the [`ValidationParams`]. +fn validate_validation_data( + validation_data: &PersistedValidationData, + relay_parent_number: RelayChainBlockNumber, + relay_parent_storage_root: RHash, + parent_head: bytes::Bytes, +) { + assert_eq!(parent_head, validation_data.parent_head.0, "Parent head doesn't match"); + assert_eq!( + relay_parent_number, validation_data.relay_parent_number, + "Relay parent number doesn't match", + ); + assert_eq!( + relay_parent_storage_root, validation_data.relay_parent_storage_root, + "Relay parent storage root doesn't match", + ); +} + /// Run the given closure with the externalities set. fn run_with_externalities R>( backend: &TrieBackend, diff --git a/pallets/parachain-system/src/validate_block/mod.rs b/pallets/parachain-system/src/validate_block/mod.rs index 44886efd4fe..8138f2843b3 100644 --- a/pallets/parachain-system/src/validate_block/mod.rs +++ b/pallets/parachain-system/src/validate_block/mod.rs @@ -16,36 +16,43 @@ //! A module that enables a runtime to work as parachain. -use polkadot_parachain::primitives::ValidationParams; - #[cfg(not(feature = "std"))] #[doc(hidden)] pub mod implementation; #[cfg(test)] mod tests; +#[cfg(not(feature = "std"))] +#[doc(hidden)] +pub use bytes; +#[cfg(not(feature = "std"))] +#[doc(hidden)] +pub use codec::decode_from_bytes; #[cfg(not(feature = "std"))] #[doc(hidden)] pub use polkadot_parachain; #[cfg(not(feature = "std"))] #[doc(hidden)] pub use sp_runtime::traits::GetRuntimeBlockType; +#[cfg(not(feature = "std"))] +#[doc(hidden)] +pub use sp_std; -// 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`]. +/// Basically the same as [`ValidationParams`](polkadot_parachain::primitives::ValidationParams), +/// but a little bit optimized for our use case here. /// -/// 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(f: impl FnOnce(&ValidationParams) -> R) -> Option { - 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(mut params: ValidationParams, f: impl FnOnce() -> R) -> R { - VALIDATION_PARAMS::using(&mut params, f) +/// `block_data` and `head_data` are represented as [`bytes::Bytes`] to make them reuse +/// the memory of the input parameter of the exported `validate_blocks` function. +/// +/// The layout of this type must match exactly the layout of +/// [`ValidationParams`](polkadot_parachain::primitives::ValidationParams) to have the same +/// SCALE encoding. +#[derive(codec::Decode)] +#[cfg_attr(feature = "std", derive(codec::Encode))] +#[doc(hidden)] +pub struct MemoryOptimizedValidationParams { + pub parent_head: bytes::Bytes, + pub block_data: bytes::Bytes, + pub relay_parent_number: cumulus_primitives_core::relay_chain::v2::BlockNumber, + pub relay_parent_storage_root: cumulus_primitives_core::relay_chain::v2::Hash, } diff --git a/pallets/parachain-system/src/validate_block/tests.rs b/pallets/parachain-system/src/validate_block/tests.rs index 116ee86edad..2c39188a085 100644 --- a/pallets/parachain-system/src/validate_block/tests.rs +++ b/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 . -use codec::{Decode, Encode}; +use codec::{Decode, DecodeAll, Encode}; use cumulus_primitives_core::{ParachainBlockData, PersistedValidationData}; use cumulus_test_client::{ generate_extrinsic, @@ -27,6 +27,8 @@ use sp_keyring::AccountKeyring::*; use sp_runtime::traits::Header as HeaderT; use std::{env, process::Command}; +use crate::validate_block::MemoryOptimizedValidationParams; + fn call_validate_block_encoded_header( parent_head: Header, block_data: ParachainBlockData, @@ -289,3 +291,31 @@ fn check_inherents_are_unsigned_and_before_all_other_extrinsics() { .contains("Could not find `set_validation_data` inherent")); } } + +/// Test that ensures that `ValidationParams` and `MemoryOptimizedValidationParams` +/// are encoding/decoding. +#[test] +fn validation_params_and_memory_optimized_validation_params_encode_and_decode() { + const BLOCK_DATA: &[u8] = &[1, 2, 3, 4, 5]; + const PARENT_HEAD: &[u8] = &[1, 3, 4, 5, 6, 7, 9]; + + let validation_params = ValidationParams { + block_data: BlockData(BLOCK_DATA.encode()), + parent_head: HeadData(PARENT_HEAD.encode()), + relay_parent_number: 1, + relay_parent_storage_root: Hash::random(), + }; + + let encoded = validation_params.encode(); + + let decoded = MemoryOptimizedValidationParams::decode_all(&mut &encoded[..]).unwrap(); + assert_eq!(decoded.relay_parent_number, validation_params.relay_parent_number); + assert_eq!(decoded.relay_parent_storage_root, validation_params.relay_parent_storage_root); + assert_eq!(decoded.block_data, validation_params.block_data.0); + assert_eq!(decoded.parent_head, validation_params.parent_head.0); + + let encoded = decoded.encode(); + + let decoded = ValidationParams::decode_all(&mut &encoded[..]).unwrap(); + assert_eq!(decoded, validation_params); +}