Do not call initialize_block before any runtime api (#8953)

* Do not call `initialize_block` before any runtime api

Before this change we always called `initialize_block` before calling
into the runtime. There was already support with `skip_initialize` to skip
the initialization. Almost no runtime_api requires that
`initialize_block` is called before. Actually this only leads to higher
execution times most of the time, because all runtime modules are
initialized and this is especially expensive when the block contained a
runtime upgrade.

TLDR: Do not call `initialize_block` before calling a runtime api.

* Change `validate_transaction` interface

* Fix rpc test

* Fixes and comments

* Some docs

substrate/client/api/src/call_executor.rs

@@ -30,7 +30,7 @@ use sc_executor::{RuntimeVersion, NativeVersion};
 use sp_externalities::Extensions;
 use sp_core::NativeOrEncoded;
 
-use sp_api::{ProofRecorder, InitializeBlock, StorageTransactionCache};
+use sp_api::{ProofRecorder, StorageTransactionCache};
 use crate::execution_extensions::ExecutionExtensions;
 
 /// Executor Provider
@@ -71,8 +71,6 @@ pub trait CallExecutor<B: BlockT> {
 	/// Before executing the method, passed header is installed as the current header
 	/// of the execution context.
 	fn contextual_call<
-		'a,
-		IB: Fn() -> sp_blockchain::Result<()>,
 		EM: Fn(
 			Result<NativeOrEncoded<R>, Self::Error>,
 			Result<NativeOrEncoded<R>, Self::Error>
@@ -81,7 +79,6 @@ pub trait CallExecutor<B: BlockT> {
 		NC: FnOnce() -> result::Result<R, sp_api::ApiError> + UnwindSafe,
 	>(
 		&self,
-		initialize_block_fn: IB,
 		at: &BlockId<B>,
 		method: &str,
 		call_data: &[u8],
@@ -89,7 +86,6 @@ pub trait CallExecutor<B: BlockT> {
 		storage_transaction_cache: Option<&RefCell<
 			StorageTransactionCache<B, <Self::Backend as crate::backend::Backend<B>>::State>,
 		>>,
-		initialize_block: InitializeBlock<'a, B>,
 		execution_manager: ExecutionManager<EM>,
 		native_call: Option<NC>,
 		proof_recorder: &Option<ProofRecorder<B>>,

substrate/client/light/src/call_executor.rs

@@ -27,7 +27,7 @@ use sp_core::{
 	convert_hash, NativeOrEncoded, traits::{CodeExecutor, SpawnNamed},
 };
 use sp_runtime::{
-	generic::BlockId, traits::{One, Block as BlockT, Header as HeaderT, HashFor},
+	generic::BlockId, traits::{Block as BlockT, Header as HeaderT, HashFor},
 };
 use sp_externalities::Extensions;
 use sp_state_machine::{
@@ -36,7 +36,7 @@ use sp_state_machine::{
 };
 use hash_db::Hasher;
 
-use sp_api::{ProofRecorder, InitializeBlock, StorageTransactionCache};
+use sp_api::{ProofRecorder, StorageTransactionCache};
 
 use sp_blockchain::{Error as ClientError, Result as ClientResult};
 
@@ -97,8 +97,6 @@ impl<Block, B, Local> CallExecutor<Block> for
 	}
 
 	fn contextual_call<
-		'a,
-		IB: Fn() -> ClientResult<()>,
 		EM: Fn(
 			Result<NativeOrEncoded<R>, Self::Error>,
 			Result<NativeOrEncoded<R>, Self::Error>
@@ -107,13 +105,11 @@ impl<Block, B, Local> CallExecutor<Block> for
 		NC: FnOnce() -> result::Result<R, sp_api::ApiError> + UnwindSafe,
 	>(
 		&self,
-		initialize_block_fn: IB,
 		at: &BlockId<Block>,
 		method: &str,
 		call_data: &[u8],
 		changes: &RefCell<OverlayedChanges>,
 		_: Option<&RefCell<StorageTransactionCache<Block, B::State>>>,
-		initialize_block: InitializeBlock<'a, Block>,
 		_manager: ExecutionManager<EM>,
 		native_call: Option<NC>,
 		recorder: &Option<ProofRecorder<Block>>,
@@ -124,7 +120,6 @@ impl<Block, B, Local> CallExecutor<Block> for
 
 		match self.backend.is_local_state_available(at) {
 			true => CallExecutor::contextual_call::<
-				_,
 				fn(
 					Result<NativeOrEncoded<R>, Local::Error>,
 					Result<NativeOrEncoded<R>, Local::Error>,
@@ -133,13 +128,11 @@ impl<Block, B, Local> CallExecutor<Block> for
 				NC
 			>(
 				&self.local,
-				initialize_block_fn,
 				at,
 				method,
 				call_data,
 				changes,
 				None,
-				initialize_block,
 				ExecutionManager::NativeWhenPossible,
 				native_call,
 				recorder,
@@ -177,7 +170,6 @@ impl<Block, B, Local> CallExecutor<Block> for
 /// Proof includes both environment preparation proof and method execution proof.
 pub fn prove_execution<Block, S, E>(
 	mut state: S,
-	header: Block::Header,
 	executor: &E,
 	method: &str,
 	call_data: &[u8],
@@ -193,31 +185,20 @@ pub fn prove_execution<Block, S, E>(
 				Box<dyn sp_state_machine::Error>
 		)?;
 
-	// prepare execution environment + record preparation proof
-	let mut changes = Default::default();
-	let (_, init_proof) = executor.prove_at_trie_state(
-		trie_state,
-		&mut changes,
-		"Core_initialize_block",
-		&header.encode(),
-	)?;
-
 	// execute method + record execution proof
 	let (result, exec_proof) = executor.prove_at_trie_state(
 		&trie_state,
-		&mut changes,
+		&mut Default::default(),
 		method,
 		call_data,
 	)?;
-	let total_proof = StorageProof::merge(vec![init_proof, exec_proof]);
 
-	Ok((result, total_proof))
+	Ok((result, exec_proof))
 }
 
 /// Check remote contextual execution proof using given backend.
 ///
-/// Method is executed using passed header as environment' current block.
-/// Proof should include both environment preparation proof and method execution proof.
+/// Proof should include the method execution proof.
 pub fn check_execution_proof<Header, E, H>(
 	executor: &E,
 	spawn_handle: Box<dyn SpawnNamed>,
@@ -229,63 +210,19 @@ pub fn check_execution_proof<Header, E, H>(
 		E: CodeExecutor + Clone + 'static,
 		H: Hasher,
 		H::Out: Ord + codec::Codec + 'static,
-{
-	check_execution_proof_with_make_header::<Header, E, H, _>(
-		executor,
-		spawn_handle,
-		request,
-		remote_proof,
-		|header| <Header as HeaderT>::new(
-			*header.number() + One::one(),
-			Default::default(),
-			Default::default(),
-			header.hash(),
-			Default::default(),
-		),
-	)
-}
-
-/// Check remote contextual execution proof using given backend and header factory.
-///
-/// Method is executed using passed header as environment' current block.
-/// Proof should include both environment preparation proof and method execution proof.
-pub fn check_execution_proof_with_make_header<Header, E, H, MakeNextHeader>(
-	executor: &E,
-	spawn_handle: Box<dyn SpawnNamed>,
-	request: &RemoteCallRequest<Header>,
-	remote_proof: StorageProof,
-	make_next_header: MakeNextHeader,
-) -> ClientResult<Vec<u8>>
-	where
-		E: CodeExecutor + Clone + 'static,
-		H: Hasher,
-		Header: HeaderT,
-		H::Out: Ord + codec::Codec + 'static,
-		MakeNextHeader: Fn(&Header) -> Header,
 {
 	let local_state_root = request.header.state_root();
 	let root: H::Out = convert_hash(&local_state_root);
 
-	// prepare execution environment + check preparation proof
+	// prepare execution environment
 	let mut changes = OverlayedChanges::default();
 	let trie_backend = create_proof_check_backend(root, remote_proof)?;
-	let next_header = make_next_header(&request.header);
 
 	// TODO: Remove when solved: https://github.com/paritytech/substrate/issues/5047
 	let backend_runtime_code = sp_state_machine::backend::BackendRuntimeCode::new(&trie_backend);
 	let runtime_code = backend_runtime_code.runtime_code()
 		.map_err(|_e| ClientError::RuntimeCodeMissing)?;
 
-	execution_proof_check_on_trie_backend::<H, Header::Number, _, _>(
-		&trie_backend,
-		&mut changes,
-		executor,
-		spawn_handle.clone(),
-		"Core_initialize_block",
-		&next_header.encode(),
-		&runtime_code,
-	)?;
-
 	// execute method
 	execution_proof_check_on_trie_backend::<H, Header::Number, _, _>(
 		&trie_backend,

substrate/client/rpc/src/state/tests.rs

@@ -474,7 +474,7 @@ fn should_return_runtime_version() {
 
 	let result = "{\"specName\":\"test\",\"implName\":\"parity-test\",\"authoringVersion\":1,\
 		\"specVersion\":2,\"implVersion\":2,\"apis\":[[\"0xdf6acb689907609b\",3],\
-		[\"0x37e397fc7c91f5e4\",1],[\"0xd2bc9897eed08f15\",2],[\"0x40fe3ad401f8959a\",5],\
+		[\"0x37e397fc7c91f5e4\",1],[\"0xd2bc9897eed08f15\",3],[\"0x40fe3ad401f8959a\",5],\
 		[\"0xc6e9a76309f39b09\",1],[\"0xdd718d5cc53262d4\",1],[\"0xcbca25e39f142387\",2],\
 		[\"0xf78b278be53f454c\",2],[\"0xab3c0572291feb8b\",1],[\"0xbc9d89904f5b923f\",1]],\
 		\"transactionVersion\":1}";

substrate/client/service/src/client/call_executor.rs

@@ -30,7 +30,7 @@ use sp_externalities::Extensions;
 use sp_core::{
 	NativeOrEncoded, NeverNativeValue, traits::{CodeExecutor, SpawnNamed, RuntimeCode},
 };
-use sp_api::{ProofRecorder, InitializeBlock, StorageTransactionCache};
+use sp_api::{ProofRecorder, StorageTransactionCache};
 use sc_client_api::{backend, call_executor::CallExecutor};
 use super::{client::ClientConfig, wasm_override::WasmOverride, wasm_substitutes::WasmSubstitutes};
 
@@ -173,8 +173,6 @@ where
 	}
 
 	fn contextual_call<
-		'a,
-		IB: Fn() -> sp_blockchain::Result<()>,
 		EM: Fn(
 			Result<NativeOrEncoded<R>, Self::Error>,
 			Result<NativeOrEncoded<R>, Self::Error>
@@ -183,7 +181,6 @@ where
 		NC: FnOnce() -> result::Result<R, sp_api::ApiError> + UnwindSafe,
 	>(
 		&self,
-		initialize_block_fn: IB,
 		at: &BlockId<Block>,
 		method: &str,
 		call_data: &[u8],
@@ -191,21 +188,11 @@ where
 		storage_transaction_cache: Option<&RefCell<
 			StorageTransactionCache<Block, B::State>
 		>>,
-		initialize_block: InitializeBlock<'a, Block>,
 		execution_manager: ExecutionManager<EM>,
 		native_call: Option<NC>,
 		recorder: &Option<ProofRecorder<Block>>,
 		extensions: Option<Extensions>,
 	) -> Result<NativeOrEncoded<R>, sp_blockchain::Error> where ExecutionManager<EM>: Clone {
-		match initialize_block {
-			InitializeBlock::Do(ref init_block)
-				if init_block.borrow().as_ref().map(|id| id != at).unwrap_or(true) => {
-				initialize_block_fn()?;
-			},
-			// We don't need to initialize the runtime at a block.
-			_ => {},
-		}
-
 		let changes_trie_state = backend::changes_tries_state_at_block(at, self.backend.changes_trie_storage())?;
 		let mut storage_transaction_cache = storage_transaction_cache.map(|c| c.borrow_mut());
 

substrate/client/service/src/client/client.rs

@@ -1243,18 +1243,6 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
 		trace!("Collected {} uncles", uncles.len());
 		Ok(uncles)
 	}
-
-	/// Prepare in-memory header that is used in execution environment.
-	fn prepare_environment_block(&self, parent: &BlockId<Block>) -> sp_blockchain::Result<Block::Header> {
-		let parent_hash = self.backend.blockchain().expect_block_hash_from_id(parent)?;
-		Ok(<<Block as BlockT>::Header as HeaderT>::new(
-			self.backend.blockchain().expect_block_number_from_id(parent)? + One::one(),
-			Default::default(),
-			Default::default(),
-			parent_hash,
-			Default::default(),
-		))
-	}
 }
 
 impl<B, E, Block, RA> UsageProvider<Block> for Client<B, E, Block, RA> where
@@ -1313,10 +1301,8 @@ impl<B, E, Block, RA> ProofProvider<Block> for Client<B, E, Block, RA> where
 		)?;
 
 		let state = self.state_at(id)?;
-		let header = self.prepare_environment_block(id)?;
 		prove_execution(
 			state,
-			header,
 			&self.executor,
 			method,
 			call_data,
@@ -1782,12 +1768,10 @@ impl<B, E, Block, RA> CallApiAt<Block> for Client<B, E, Block, RA> where
 		'a,
 		R: Encode + Decode + PartialEq,
 		NC: FnOnce() -> result::Result<R, sp_api::ApiError> + UnwindSafe,
-		C: CoreApi<Block>,
 	>(
 		&self,
-		params: CallApiAtParams<'a, Block, C, NC, B::State>,
+		params: CallApiAtParams<'a, Block, NC, B::State>,
 	) -> Result<NativeOrEncoded<R>, sp_api::ApiError> {
-		let core_api = params.core_api;
 		let at = params.at;
 
 		let (manager, extensions) = self.execution_extensions.manager_and_extensions(
@@ -1795,16 +1779,12 @@ impl<B, E, Block, RA> CallApiAt<Block> for Client<B, E, Block, RA> where
 			params.context,
 		);
 
-		self.executor.contextual_call::<_, fn(_,_) -> _,_,_>(
-			|| core_api
-				.initialize_block(at, &self.prepare_environment_block(at)?)
-				.map_err(Error::RuntimeApiError),
+		self.executor.contextual_call::<fn(_,_) -> _, _, _>(
 			at,
 			params.function,
 			&params.arguments,
 			params.overlayed_changes,
 			Some(params.storage_transaction_cache),
-			params.initialize_block,
 			manager,
 			params.native_call,
 			params.recorder,

substrate/client/service/test/src/client/light.rs

@@ -20,7 +20,6 @@ use sc_light::{
 	call_executor::{
 		GenesisCallExecutor,
 		check_execution_proof,
-		check_execution_proof_with_make_header,
 	},
 	fetcher::LightDataChecker,
 	blockchain::{BlockchainCache, Blockchain},
@@ -37,7 +36,7 @@ use parking_lot::Mutex;
 use substrate_test_runtime_client::{
 	runtime::{Hash, Block, Header}, TestClient, ClientBlockImportExt,
 };
-use sp_api::{InitializeBlock, StorageTransactionCache, ProofRecorder};
+use sp_api::{StorageTransactionCache, ProofRecorder};
 use sp_consensus::BlockOrigin;
 use sc_executor::{NativeExecutor, WasmExecutionMethod, RuntimeVersion, NativeVersion};
 use sp_core::{H256, NativeOrEncoded, testing::TaskExecutor};
@@ -209,8 +208,6 @@ impl CallExecutor<Block> for DummyCallExecutor {
 	}
 
 	fn contextual_call<
-		'a,
-		IB: Fn() -> ClientResult<()>,
 		EM: Fn(
 			Result<NativeOrEncoded<R>, Self::Error>,
 			Result<NativeOrEncoded<R>, Self::Error>
@@ -219,7 +216,6 @@ impl CallExecutor<Block> for DummyCallExecutor {
 		NC: FnOnce() -> Result<R, sp_api::ApiError> + UnwindSafe,
 	>(
 		&self,
-		_initialize_block_fn: IB,
 		_at: &BlockId<Block>,
 		_method: &str,
 		_call_data: &[u8],
@@ -230,7 +226,6 @@ impl CallExecutor<Block> for DummyCallExecutor {
 				<Self::Backend as sc_client_api::backend::Backend<Block>>::State,
 			>
 		>>,
-		_initialize_block: InitializeBlock<'a, Block>,
 		_execution_manager: ExecutionManager<EM>,
 		_native_call: Option<NC>,
 		_proof_recorder: &Option<ProofRecorder<Block>>,
@@ -333,36 +328,41 @@ fn execution_proof_is_generated_and_checked() {
 		(remote_result, local_result)
 	}
 
-	fn execute_with_proof_failure(remote_client: &TestClient, at: u64, method: &'static str) {
+	fn execute_with_proof_failure(remote_client: &TestClient, at: u64) {
 		let remote_block_id = BlockId::Number(at);
 		let remote_header = remote_client.header(&remote_block_id).unwrap().unwrap();
 
 		// 'fetch' execution proof from remote node
 		let (_, remote_execution_proof) = remote_client.execution_proof(
 			&remote_block_id,
-			method,
-			&[]
+			"Core_initialize_block",
+			&Header::new(
+				at,
+				Default::default(),
+				Default::default(),
+				Default::default(),
+				Default::default(),
+			).encode(),
 		).unwrap();
 
 		// check remote execution proof locally
-		let execution_result = check_execution_proof_with_make_header::<_, _, BlakeTwo256, _>(
+		let execution_result = check_execution_proof::<_, _, BlakeTwo256>(
 			&local_executor(),
 			Box::new(TaskExecutor::new()),
 			&RemoteCallRequest {
 				block: substrate_test_runtime_client::runtime::Hash::default(),
-				header: remote_header,
-				method: method.into(),
-				call_data: vec![],
+				header: remote_header.clone(),
+				method: "Core_initialize_block".into(),
+				call_data: Header::new(
+					at + 1,
+					Default::default(),
+					Default::default(),
+					remote_header.hash(),
+					remote_header.digest().clone(), // this makes next header wrong
+				).encode(),
 				retry_count: None,
 			},
 			remote_execution_proof,
-			|header| <Header as HeaderT>::new(
-				at + 1,
-				Default::default(),
-				Default::default(),
-				header.hash(),
-				header.digest().clone(), // this makes next header wrong
-			),
 		);
 		match execution_result {
 			Err(sp_blockchain::Error::Execution(_)) => (),
@@ -389,21 +389,12 @@ fn execution_proof_is_generated_and_checked() {
 	let (remote, local) = execute(&remote_client, 2, "Core_version");
 	assert_eq!(remote, local);
 
-	// check method that requires environment
-	let (_, block) = execute(&remote_client, 0, "BlockBuilder_finalize_block");
-	let local_block: Header = Decode::decode(&mut &block[..]).unwrap();
-	assert_eq!(local_block.number, 1);
-
-	let (_, block) = execute(&remote_client, 2, "BlockBuilder_finalize_block");
-	let local_block: Header = Decode::decode(&mut &block[..]).unwrap();
-	assert_eq!(local_block.number, 3);
-
 	// check that proof check doesn't panic even if proof is incorrect AND no panic handler is set
-	execute_with_proof_failure(&remote_client, 2, "Core_version");
+	execute_with_proof_failure(&remote_client, 2);
 
 	// check that proof check doesn't panic even if proof is incorrect AND panic handler is set
 	sp_panic_handler::set("TEST", "1.2.3");
-	execute_with_proof_failure(&remote_client, 2, "Core_version");
+	execute_with_proof_failure(&remote_client, 2);
 }
 
 #[test]

substrate/client/transaction-pool/src/api.rs

@@ -203,24 +203,54 @@ where
 	sp_tracing::within_span!(sp_tracing::Level::TRACE, "validate_transaction";
 	{
 		let runtime_api = client.runtime_api();
-		let has_v2 = sp_tracing::within_span! { sp_tracing::Level::TRACE, "check_version";
+		let api_version = sp_tracing::within_span! { sp_tracing::Level::TRACE, "check_version";
 			runtime_api
-				.has_api_with::<dyn TaggedTransactionQueue<Block>, _>(&at, |v| v >= 2)
-				.unwrap_or_default()
-		};
+				.api_version::<dyn TaggedTransactionQueue<Block>>(&at)
+				.map_err(|e| Error::RuntimeApi(e.to_string()))?
+				.ok_or_else(|| Error::RuntimeApi(
+					format!("Could not find `TaggedTransactionQueue` api for block `{:?}`.", at)
+				))
+		}?;
 
-		let res = sp_tracing::within_span!(
+		let block_hash = client.to_hash(at)
+			.map_err(|e| Error::RuntimeApi(format!("{:?}", e)))?
+			.ok_or_else(|| Error::RuntimeApi(format!("Could not get hash for block `{:?}`.", at)))?;
+
+		use sp_api::Core;
+
+		sp_tracing::within_span!(
 			sp_tracing::Level::TRACE, "runtime::validate_transaction";
 		{
-			if has_v2 {
-				runtime_api.validate_transaction(&at, source, uxt)
+			if api_version >= 3 {
+				runtime_api.validate_transaction(&at, source, uxt, block_hash)
+					.map_err(|e| Error::RuntimeApi(e.to_string()))
 			} else {
-				#[allow(deprecated)] // old validate_transaction
-				runtime_api.validate_transaction_before_version_2(&at, uxt)
-			}
-		});
+				let block_number = client.to_number(at)
+					.map_err(|e| Error::RuntimeApi(format!("{:?}", e)))?
+					.ok_or_else(||
+						Error::RuntimeApi(format!("Could not get number for block `{:?}`.", at))
+					)?;
 
-		res.map_err(|e| Error::RuntimeApi(e.to_string()))
+				// The old versions require us to call `initialize_block` before.
+				runtime_api.initialize_block(at, &sp_runtime::traits::Header::new(
+					block_number + sp_runtime::traits::One::one(),
+					Default::default(),
+					Default::default(),
+					block_hash,
+					Default::default()),
+				).map_err(|e| Error::RuntimeApi(e.to_string()))?;
+
+				if api_version == 2 {
+					#[allow(deprecated)] // old validate_transaction
+					runtime_api.validate_transaction_before_version_3(&at, source, uxt)
+						.map_err(|e| Error::RuntimeApi(e.to_string()))
+				} else {
+					#[allow(deprecated)] // old validate_transaction
+					runtime_api.validate_transaction_before_version_2(&at, uxt)
+						.map_err(|e| Error::RuntimeApi(e.to_string()))
+				}
+			}
+		})
 	})
 }