Improve CodeExecutor (paritytech#2358)

Since `sp-state-machine` and `GenesisConfigBuilderRuntimeCaller` always set `use_native` to be false. We should remove this param and make `NativeElseWasmExecutor` behave like its name. It could make the above components use the correct execution strategy. Maybe polkadot do not need about `NativeElseWasmExecutor` anymore. But it is still needed by other chains and it's useful for debugging. --------- Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: command-bot <> Co-authored-by: Michal Kucharczyk <[email protected]>
moonbeam-foundation · Nov 29, 2023 · f2fe6a4 · f2fe6a4
1 parent 63ac247
commit f2fe6a4
Show file tree

Hide file tree

Showing 9 changed files with 56 additions and 81 deletions.
diff --git a/substrate/bin/node/cli/benches/executor.rs b/substrate/bin/node/cli/benches/executor.rs
@@ -103,14 +103,7 @@ fn construct_block<E: Externalities>(
 
 	// execute the block to get the real header.
 	executor
-		.call(
-			ext,
-			&runtime_code,
-			"Core_initialize_block",
-			&header.encode(),
-			true,
-			CallContext::Offchain,
-		)
+		.call(ext, &runtime_code, "Core_initialize_block", &header.encode(), CallContext::Offchain)
 		.0
 		.unwrap();
 
@@ -121,7 +114,6 @@ fn construct_block<E: Externalities>(
 				&runtime_code,
 				"BlockBuilder_apply_extrinsic",
 				&i.encode(),
-				true,
 				CallContext::Offchain,
 			)
 			.0
@@ -135,7 +127,6 @@ fn construct_block<E: Externalities>(
 				&runtime_code,
 				"BlockBuilder_finalize_block",
 				&[0u8; 0],
-				true,
 				CallContext::Offchain,
 			)
 			.0
@@ -200,7 +191,6 @@ fn bench_execute_block(c: &mut Criterion) {
 							&runtime_code,
 							"Core_execute_block",
 							&block.0,
-							false,
 							CallContext::Offchain,
 						)
 						.0

diff --git a/substrate/bin/node/cli/tests/basic.rs b/substrate/bin/node/cli/tests/basic.rs
@@ -193,11 +193,9 @@ fn panic_execution_with_foreign_code_gives_error() {
 	t.insert(<pallet_balances::TotalIssuance<Runtime>>::hashed_key().to_vec(), 69_u128.encode());
 	t.insert(<frame_system::BlockHash<Runtime>>::hashed_key_for(0), vec![0u8; 32]);
 
-	let r =
-		executor_call(&mut t, "Core_initialize_block", &vec![].and(&from_block_number(1u32)), true)
-			.0;
+	let r = executor_call(&mut t, "Core_initialize_block", &vec![].and(&from_block_number(1u32))).0;
 	assert!(r.is_ok());
-	let v = executor_call(&mut t, "BlockBuilder_apply_extrinsic", &vec![].and(&xt()), true)
+	let v = executor_call(&mut t, "BlockBuilder_apply_extrinsic", &vec![].and(&xt()))
 		.0
 		.unwrap();
 	let r = ApplyExtrinsicResult::decode(&mut &v[..]).unwrap();
@@ -219,11 +217,9 @@ fn bad_extrinsic_with_native_equivalent_code_gives_error() {
 	t.insert(<pallet_balances::TotalIssuance<Runtime>>::hashed_key().to_vec(), 69u128.encode());
 	t.insert(<frame_system::BlockHash<Runtime>>::hashed_key_for(0), vec![0u8; 32]);
 
-	let r =
-		executor_call(&mut t, "Core_initialize_block", &vec![].and(&from_block_number(1u32)), true)
-			.0;
+	let r = executor_call(&mut t, "Core_initialize_block", &vec![].and(&from_block_number(1u32))).0;
 	assert!(r.is_ok());
-	let v = executor_call(&mut t, "BlockBuilder_apply_extrinsic", &vec![].and(&xt()), true)
+	let v = executor_call(&mut t, "BlockBuilder_apply_extrinsic", &vec![].and(&xt()))
 		.0
 		.unwrap();
 	let r = ApplyExtrinsicResult::decode(&mut &v[..]).unwrap();
@@ -256,14 +252,12 @@ fn successful_execution_with_native_equivalent_code_gives_ok() {
 	);
 	t.insert(<frame_system::BlockHash<Runtime>>::hashed_key_for(0), vec![0u8; 32]);
 
-	let r =
-		executor_call(&mut t, "Core_initialize_block", &vec![].and(&from_block_number(1u32)), true)
-			.0;
+	let r = executor_call(&mut t, "Core_initialize_block", &vec![].and(&from_block_number(1u32))).0;
 	assert!(r.is_ok());
 
 	let fees = t.execute_with(|| transfer_fee(&xt()));
 
-	let r = executor_call(&mut t, "BlockBuilder_apply_extrinsic", &vec![].and(&xt()), true).0;
+	let r = executor_call(&mut t, "BlockBuilder_apply_extrinsic", &vec![].and(&xt())).0;
 	assert!(r.is_ok());
 
 	t.execute_with(|| {
@@ -298,14 +292,12 @@ fn successful_execution_with_foreign_code_gives_ok() {
 	);
 	t.insert(<frame_system::BlockHash<Runtime>>::hashed_key_for(0), vec![0u8; 32]);
 
-	let r =
-		executor_call(&mut t, "Core_initialize_block", &vec![].and(&from_block_number(1u32)), true)
-			.0;
+	let r = executor_call(&mut t, "Core_initialize_block", &vec![].and(&from_block_number(1u32))).0;
 	assert!(r.is_ok());
 
 	let fees = t.execute_with(|| transfer_fee(&xt()));
 
-	let r = executor_call(&mut t, "BlockBuilder_apply_extrinsic", &vec![].and(&xt()), true).0;
+	let r = executor_call(&mut t, "BlockBuilder_apply_extrinsic", &vec![].and(&xt())).0;
 	assert!(r.is_ok());
 
 	t.execute_with(|| {
@@ -337,7 +329,7 @@ fn full_native_block_import_works() {
 				.base_extrinsic,
 		);
 
-	executor_call(&mut t, "Core_execute_block", &block1.0, true).0.unwrap();
+	executor_call(&mut t, "Core_execute_block", &block1.0).0.unwrap();
 
 	t.execute_with(|| {
 		assert_eq!(Balances::total_balance(&alice()), 42 * DOLLARS - fees);
@@ -412,7 +404,7 @@ fn full_native_block_import_works() {
 	fees = t.execute_with(|| transfer_fee(&xt()));
 	let pot = t.execute_with(|| Treasury::pot());
 
-	executor_call(&mut t, "Core_execute_block", &block2.0, true).0.unwrap();
+	executor_call(&mut t, "Core_execute_block", &block2.0).0.unwrap();
 
 	t.execute_with(|| {
 		assert_eq!(
@@ -554,7 +546,7 @@ fn full_wasm_block_import_works() {
 	let mut alice_last_known_balance: Balance = Default::default();
 	let mut fees = t.execute_with(|| transfer_fee(&xt()));
 
-	executor_call(&mut t, "Core_execute_block", &block1.0, false).0.unwrap();
+	executor_call(&mut t, "Core_execute_block", &block1.0).0.unwrap();
 
 	t.execute_with(|| {
 		assert_eq!(Balances::total_balance(&alice()), 42 * DOLLARS - fees);
@@ -564,7 +556,7 @@ fn full_wasm_block_import_works() {
 
 	fees = t.execute_with(|| transfer_fee(&xt()));
 
-	executor_call(&mut t, "Core_execute_block", &block2.0, false).0.unwrap();
+	executor_call(&mut t, "Core_execute_block", &block2.0).0.unwrap();
 
 	t.execute_with(|| {
 		assert_eq!(
@@ -717,7 +709,7 @@ fn deploying_wasm_contract_should_work() {
 
 	let mut t = new_test_ext(compact_code_unwrap());
 
-	executor_call(&mut t, "Core_execute_block", &b.0, false).0.unwrap();
+	executor_call(&mut t, "Core_execute_block", &b.0).0.unwrap();
 
 	t.execute_with(|| {
 		// Verify that the contract does exist by querying some of its storage items
@@ -732,16 +724,15 @@ fn wasm_big_block_import_fails() {
 
 	set_heap_pages(&mut t.ext(), 4);
 
-	let result =
-		executor_call(&mut t, "Core_execute_block", &block_with_size(42, 0, 120_000).0, false).0;
+	let result = executor_call(&mut t, "Core_execute_block", &block_with_size(42, 0, 120_000).0).0;
 	assert!(result.is_err()); // Err(Wasmi(Trap(Trap { kind: Host(AllocatorOutOfSpace) })))
 }
 
 #[test]
 fn native_big_block_import_succeeds() {
 	let mut t = new_test_ext(compact_code_unwrap());
 
-	executor_call(&mut t, "Core_execute_block", &block_with_size(42, 0, 120_000).0, true)
+	executor_call(&mut t, "Core_execute_block", &block_with_size(42, 0, 120_000).0)
 		.0
 		.unwrap();
 }
@@ -754,11 +745,9 @@ fn native_big_block_import_fails_on_fallback() {
 	// block.
 	set_heap_pages(&mut t.ext(), 8);
 
-	assert!(
-		executor_call(&mut t, "Core_execute_block", &block_with_size(42, 0, 120_000).0, false,)
-			.0
-			.is_err()
-	);
+	assert!(executor_call(&mut t, "Core_execute_block", &block_with_size(42, 0, 120_000).0)
+		.0
+		.is_err());
 }
 
 #[test]
@@ -775,15 +764,9 @@ fn panic_execution_gives_error() {
 	t.insert(<pallet_balances::TotalIssuance<Runtime>>::hashed_key().to_vec(), 0_u128.encode());
 	t.insert(<frame_system::BlockHash<Runtime>>::hashed_key_for(0), vec![0u8; 32]);
 
-	let r = executor_call(
-		&mut t,
-		"Core_initialize_block",
-		&vec![].and(&from_block_number(1u32)),
-		false,
-	)
-	.0;
+	let r = executor_call(&mut t, "Core_initialize_block", &vec![].and(&from_block_number(1u32))).0;
 	assert!(r.is_ok());
-	let r = executor_call(&mut t, "BlockBuilder_apply_extrinsic", &vec![].and(&xt()), false)
+	let r = executor_call(&mut t, "BlockBuilder_apply_extrinsic", &vec![].and(&xt()))
 		.0
 		.unwrap();
 	let r = ApplyExtrinsicResult::decode(&mut &r[..]).unwrap();
@@ -816,21 +799,15 @@ fn successful_execution_gives_ok() {
 	);
 	t.insert(<frame_system::BlockHash<Runtime>>::hashed_key_for(0), vec![0u8; 32]);
 
-	let r = executor_call(
-		&mut t,
-		"Core_initialize_block",
-		&vec![].and(&from_block_number(1u32)),
-		false,
-	)
-	.0;
+	let r = executor_call(&mut t, "Core_initialize_block", &vec![].and(&from_block_number(1u32))).0;
 	assert!(r.is_ok());
 	t.execute_with(|| {
 		assert_eq!(Balances::total_balance(&alice()), 111 * DOLLARS);
 	});
 
 	let fees = t.execute_with(|| transfer_fee(&xt()));
 
-	let r = executor_call(&mut t, "BlockBuilder_apply_extrinsic", &vec![].and(&xt()), false)
+	let r = executor_call(&mut t, "BlockBuilder_apply_extrinsic", &vec![].and(&xt()))
 		.0
 		.unwrap();
 	ApplyExtrinsicResult::decode(&mut &r[..])
@@ -861,7 +838,7 @@ fn should_import_block_with_test_client() {
 #[test]
 fn default_config_as_json_works() {
 	let mut t = new_test_ext(compact_code_unwrap());
-	let r = executor_call(&mut t, "GenesisBuilder_create_default_config", &vec![], false)
+	let r = executor_call(&mut t, "GenesisBuilder_create_default_config", &vec![])
 		.0
 		.unwrap();
 	let r = Vec::<u8>::decode(&mut &r[..]).unwrap();

diff --git a/substrate/bin/node/cli/tests/common.rs b/substrate/bin/node/cli/tests/common.rs
@@ -105,7 +105,6 @@ pub fn executor_call(
 	t: &mut TestExternalities<BlakeTwo256>,
 	method: &str,
 	data: &[u8],
-	use_native: bool,
 ) -> (Result<Vec<u8>>, bool) {
 	let mut t = t.ext();
 
@@ -117,7 +116,7 @@ pub fn executor_call(
 		heap_pages: heap_pages.and_then(|hp| Decode::decode(&mut &hp[..]).ok()),
 	};
 	sp_tracing::try_init_simple();
-	executor().call(&mut t, &runtime_code, method, data, use_native, CallContext::Onchain)
+	executor().call(&mut t, &runtime_code, method, data, CallContext::Onchain)
 }
 
 pub fn new_test_ext(code: &[u8]) -> TestExternalities<BlakeTwo256> {
@@ -168,12 +167,12 @@ pub fn construct_block(
 	};
 
 	// execute the block to get the real header.
-	executor_call(env, "Core_initialize_block", &header.encode(), true).0.unwrap();
+	executor_call(env, "Core_initialize_block", &header.encode()).0.unwrap();
 
 	for extrinsic in extrinsics.iter() {
 		// Try to apply the `extrinsic`. It should be valid, in the sense that it passes
 		// all pre-inclusion checks.
-		let r = executor_call(env, "BlockBuilder_apply_extrinsic", &extrinsic.encode(), true)
+		let r = executor_call(env, "BlockBuilder_apply_extrinsic", &extrinsic.encode())
 			.0
 			.expect("application of an extrinsic failed");
 
@@ -186,7 +185,7 @@ pub fn construct_block(
 	}
 
 	let header = Header::decode(
-		&mut &executor_call(env, "BlockBuilder_finalize_block", &[0u8; 0], true).0.unwrap()[..],
+		&mut &executor_call(env, "BlockBuilder_finalize_block", &[0u8; 0]).0.unwrap()[..],
 	)
 	.unwrap();
 

diff --git a/substrate/bin/node/cli/tests/fees.rs b/substrate/bin/node/cli/tests/fees.rs
@@ -95,7 +95,7 @@ fn fee_multiplier_increases_and_decreases_on_big_weight() {
 	);
 
 	// execute a big block.
-	executor_call(&mut t, "Core_execute_block", &block1.0, true).0.unwrap();
+	executor_call(&mut t, "Core_execute_block", &block1.0).0.unwrap();
 
 	// weight multiplier is increased for next block.
 	t.execute_with(|| {
@@ -106,7 +106,7 @@ fn fee_multiplier_increases_and_decreases_on_big_weight() {
 	});
 
 	// execute a big block.
-	executor_call(&mut t, "Core_execute_block", &block2.0, true).0.unwrap();
+	executor_call(&mut t, "Core_execute_block", &block2.0).0.unwrap();
 
 	// weight multiplier is increased for next block.
 	t.execute_with(|| {
@@ -151,12 +151,10 @@ fn transaction_fee_is_correct() {
 		function: RuntimeCall::Balances(default_transfer_call()),
 	});
 
-	let r =
-		executor_call(&mut t, "Core_initialize_block", &vec![].and(&from_block_number(1u32)), true)
-			.0;
+	let r = executor_call(&mut t, "Core_initialize_block", &vec![].and(&from_block_number(1u32))).0;
 
 	assert!(r.is_ok());
-	let r = executor_call(&mut t, "BlockBuilder_apply_extrinsic", &vec![].and(&xt.clone()), true).0;
+	let r = executor_call(&mut t, "BlockBuilder_apply_extrinsic", &vec![].and(&xt.clone())).0;
 	assert!(r.is_ok());
 
 	t.execute_with(|| {
@@ -247,7 +245,7 @@ fn block_weight_capacity_report() {
 			len / 1024 / 1024,
 		);
 
-		let r = executor_call(&mut t, "Core_execute_block", &block.0, true).0;
+		let r = executor_call(&mut t, "Core_execute_block", &block.0).0;
 
 		println!(" || Result = {:?}", r);
 		assert!(r.is_ok());
@@ -310,7 +308,7 @@ fn block_length_capacity_report() {
 			len / 1024 / 1024,
 		);
 
-		let r = executor_call(&mut t, "Core_execute_block", &block.0, true).0;
+		let r = executor_call(&mut t, "Core_execute_block", &block.0).0;
 
 		println!(" || Result = {:?}", r);
 		assert!(r.is_ok());

diff --git a/substrate/client/chain-spec/src/genesis_config_builder.rs b/substrate/client/chain-spec/src/genesis_config_builder.rs
@@ -76,7 +76,6 @@ where
 				&RuntimeCode { heap_pages: None, code_fetcher: self, hash: self.code_hash.clone() },
 				method,
 				data,
-				false,
 				CallContext::Offchain,
 			)
 			.0

diff --git a/substrate/client/executor/src/executor.rs b/substrate/client/executor/src/executor.rs
@@ -492,7 +492,6 @@ where
 		runtime_code: &RuntimeCode,
 		method: &str,
 		data: &[u8],
-		_use_native: bool,
 		context: CallContext,
 	) -> (Result<Vec<u8>>, bool) {
 		tracing::trace!(
@@ -565,6 +564,8 @@ pub struct NativeElseWasmExecutor<D: NativeExecutionDispatch> {
 	/// Fallback wasm executor.
 	wasm:
 		WasmExecutor<ExtendedHostFunctions<sp_io::SubstrateHostFunctions, D::ExtendHostFunctions>>,
+
+	use_native: bool,
 }
 
 impl<D: NativeExecutionDispatch> NativeElseWasmExecutor<D> {
@@ -601,7 +602,7 @@ impl<D: NativeExecutionDispatch> NativeElseWasmExecutor<D> {
 			.with_runtime_cache_size(runtime_cache_size)
 			.build();
 
-		NativeElseWasmExecutor { native_version: D::native_version(), wasm }
+		NativeElseWasmExecutor { native_version: D::native_version(), wasm, use_native: true }
 	}
 
 	/// Create a new instance using the given [`WasmExecutor`].
@@ -610,7 +611,14 @@ impl<D: NativeExecutionDispatch> NativeElseWasmExecutor<D> {
 			ExtendedHostFunctions<sp_io::SubstrateHostFunctions, D::ExtendHostFunctions>,
 		>,
 	) -> Self {
-		Self { native_version: D::native_version(), wasm: executor }
+		Self { native_version: D::native_version(), wasm: executor, use_native: true }
+	}
+
+	/// Disable to use native runtime when possible just behave like `WasmExecutor`.
+	///
+	/// Default to enabled.
+	pub fn disable_use_native(&mut self) {
+		self.use_native = false;
 	}
 
 	/// Ignore missing function imports if set true.
@@ -645,9 +653,10 @@ impl<D: NativeExecutionDispatch + 'static> CodeExecutor for NativeElseWasmExecut
 		runtime_code: &RuntimeCode,
 		method: &str,
 		data: &[u8],
-		use_native: bool,
 		context: CallContext,
 	) -> (Result<Vec<u8>>, bool) {
+		let use_native = self.use_native;
+
 		tracing::trace!(
 			target: "executor",
 			function = %method,
@@ -711,7 +720,11 @@ impl<D: NativeExecutionDispatch + 'static> CodeExecutor for NativeElseWasmExecut
 
 impl<D: NativeExecutionDispatch> Clone for NativeElseWasmExecutor<D> {
 	fn clone(&self) -> Self {
-		NativeElseWasmExecutor { native_version: D::native_version(), wasm: self.wasm.clone() }
+		NativeElseWasmExecutor {
+			native_version: D::native_version(),
+			wasm: self.wasm.clone(),
+			use_native: self.use_native,
+		}
 	}
 }