Skip to content

Commit

Permalink
Improve CodeExecutor (#2358)
Browse files Browse the repository at this point in the history 
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 <git@kchr.de>
Co-authored-by: command-bot <>
Co-authored-by: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com>
  • Loading branch information
@yjhmelody @bkchr @michalkucharczyk
3 people authored Nov 29, 2023
1 parent 63ac247 commit f2fe6a4
Show file tree
Hide file tree
Showing 9 changed files with 56 additions and 81 deletions.
12 changes: 1 addition & 11 deletions substrate/bin/node/cli/benches/executor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand All @@ -121,7 +114,6 @@ fn construct_block<E: Externalities>(
&runtime_code,
"BlockBuilder_apply_extrinsic",
&i.encode(),
true,
CallContext::Offchain,
)
.0
Expand All @@ -135,7 +127,6 @@ fn construct_block<E: Externalities>(
&runtime_code,
"BlockBuilder_finalize_block",
&[0u8; 0],
true,
CallContext::Offchain,
)
.0
Expand Down Expand Up @@ -200,7 +191,6 @@ fn bench_execute_block(c: &mut Criterion) {
&runtime_code,
"Core_execute_block",
&block.0,
false,
CallContext::Offchain,
)
.0
Expand Down
69 changes: 23 additions & 46 deletions substrate/bin/node/cli/tests/basic.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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();
Expand Down Expand Up @@ -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(|| {
Expand Down Expand Up @@ -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(|| {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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!(
Expand Down Expand Up @@ -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);
Expand All @@ -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!(
Expand Down Expand Up @@ -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
Expand All @@ -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();
}
Expand All @@ -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]
Expand All @@ -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();
Expand Down Expand Up @@ -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[..])
Expand Down Expand Up @@ -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();
Expand Down
9 changes: 4 additions & 5 deletions substrate/bin/node/cli/tests/common.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand All @@ -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> {
Expand Down Expand Up @@ -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");

Expand All @@ -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();

Expand Down
14 changes: 6 additions & 8 deletions substrate/bin/node/cli/tests/fees.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(|| {
Expand All @@ -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(|| {
Expand Down Expand Up @@ -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(|| {
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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());
Expand Down
1 change: 0 additions & 1 deletion substrate/client/chain-spec/src/genesis_config_builder.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@ where
&RuntimeCode { heap_pages: None, code_fetcher: self, hash: self.code_hash.clone() },
method,
data,
false,
CallContext::Offchain,
)
.0
Expand Down
23 changes: 18 additions & 5 deletions substrate/client/executor/src/executor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -492,7 +492,6 @@ where
runtime_code: &RuntimeCode,
method: &str,
data: &[u8],
_use_native: bool,
context: CallContext,
) -> (Result<Vec<u8>>, bool) {
tracing::trace!(
Expand Down Expand Up @@ -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> {
Expand Down Expand Up @@ -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`].
Expand All @@ -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.
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
}
}
}

Expand Down
Loading

0 comments on commit f2fe6a4

Please sign in to comment.