From f373a988f943415dd8a4eeb0c2412b6384ddda43 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Wed, 28 Aug 2019 08:57:20 +0300 Subject: [PATCH 01/13] do not panic during execution proof check --- Cargo.lock | 1 + core/client/Cargo.toml | 1 + core/client/src/client.rs | 2 +- core/client/src/light/call_executor.rs | 84 +++++++- core/executor/src/wasm_executor.rs | 227 ++++++++++++-------- core/panic-handler/src/lib.rs | 51 ++++- core/sr-io/with_std.rs | 2 +- core/state-machine/src/basic.rs | 7 +- core/state-machine/src/ext.rs | 14 +- core/state-machine/src/lib.rs | 55 +++-- core/state-machine/src/overlayed_changes.rs | 2 +- core/state-machine/src/testing.rs | 11 +- node/executor/src/lib.rs | 2 +- 13 files changed, 328 insertions(+), 131 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2967d167da1f7..26cef2340e286 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4508,6 +4508,7 @@ dependencies = [ "substrate-executor 2.0.0", "substrate-inherents 2.0.0", "substrate-keyring 2.0.0", + "substrate-panic-handler 2.0.0", "substrate-primitives 2.0.0", "substrate-state-machine 2.0.0", "substrate-telemetry 2.0.0", diff --git a/core/client/Cargo.toml b/core/client/Cargo.toml index 9edb9b1c85d35..4b2184019f598 100644 --- a/core/client/Cargo.toml +++ b/core/client/Cargo.toml @@ -32,6 +32,7 @@ env_logger = "0.6" tempfile = "3.1" test-client = { package = "substrate-test-runtime-client", path = "../test-runtime/client" } kvdb-memorydb = { git = "https://github.com/paritytech/parity-common", rev="b0317f649ab2c665b7987b8475878fc4d2e1f81d" } +panic-handler = { package = "substrate-panic-handler", path = "../panic-handler" } [features] default = ["std"] diff --git a/core/client/src/client.rs b/core/client/src/client.rs index c6234dc8d1cca..270591b596ff1 100644 --- a/core/client/src/client.rs +++ b/core/client/src/client.rs @@ -1032,7 +1032,7 @@ impl Client where let get_execution_manager = |execution_strategy: ExecutionStrategy| { match execution_strategy { ExecutionStrategy::NativeElseWasm => ExecutionManager::NativeElseWasm, - ExecutionStrategy::AlwaysWasm => ExecutionManager::AlwaysWasm, + ExecutionStrategy::AlwaysWasm => ExecutionManager::AlwaysWasm(true), ExecutionStrategy::NativeWhenPossible => ExecutionManager::NativeWhenPossible, ExecutionStrategy::Both => ExecutionManager::Both(|wasm_result, native_result| { let header = import_headers.post(); diff --git a/core/client/src/light/call_executor.rs b/core/client/src/light/call_executor.rs index 746f36069d69e..9f80ca47dabc6 100644 --- a/core/client/src/light/call_executor.rs +++ b/core/client/src/light/call_executor.rs @@ -459,6 +459,32 @@ pub fn check_execution_proof( E: CodeExecutor, H: Hasher, H::Out: Ord + 'static, +{ + check_execution_proof_with_make_header( + executor, + request, + remote_proof, + |header|
::new( + *header.number() + One::one(), + Default::default(), + Default::default(), + header.hash(), + Default::default(), + ), + ) +} + +fn check_execution_proof_with_make_header Header>( + executor: &E, + request: &RemoteCallRequest
, + remote_proof: Vec>, + make_next_header: MakeNextHeader, +) -> ClientResult> + where + Header: HeaderT, + E: CodeExecutor, + H: Hasher, + H::Out: Ord + 'static, { let local_state_root = request.header.state_root(); let root: H::Out = convert_hash(&local_state_root); @@ -466,19 +492,13 @@ pub fn check_execution_proof( // prepare execution environment + check preparation proof let mut changes = OverlayedChanges::default(); let trie_backend = create_proof_check_backend(root, remote_proof)?; - let next_block =
::new( - *request.header.number() + One::one(), - Default::default(), - Default::default(), - request.header.hash(), - Default::default(), - ); + let next_header = make_next_header(&request.header); execution_proof_check_on_trie_backend::( &trie_backend, &mut changes, executor, "Core_initialize_block", - &next_block.encode(), + &next_header.encode(), None, )?; @@ -531,6 +551,43 @@ mod tests { (remote_result, local_result) } + fn execute_with_proof_failure(remote_client: &TestClient, at: u64, method: &'static str) { + 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, + &[] + ).unwrap(); + + // check remote execution proof locally + let local_executor = NativeExecutor::::new(None); + let execution_result = check_execution_proof_with_make_header( + &local_executor, + &RemoteCallRequest { + block: test_client::runtime::Hash::default(), + header: remote_header, + method: method.into(), + call_data: vec![], + retry_count: None, + }, + remote_execution_proof, + |header|
::new( + at + 1, + Default::default(), + Default::default(), + header.hash(), + header.digest().clone(), // this makes next header wrong + ), + ); + match execution_result { + Err(crate::error::Error::Execution(_)) => (), + _ => panic!("Unexpected execution result: {:?}", execution_result), + } + } + // prepare remote client let remote_client = test_client::new(); for i in 1u32..3u32 { @@ -547,15 +604,24 @@ mod tests { let (remote, local) = execute(&remote_client, 0, "Core_version"); assert_eq!(remote, local); + 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); - // check method that requires environment 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"); + + // check that proof check doesn't panic even if proof is incorrect AND panic handler is set + panic_handler::set("TEST"); + execute_with_proof_failure(&remote_client, 2, "Core_version"); } #[test] diff --git a/core/executor/src/wasm_executor.rs b/core/executor/src/wasm_executor.rs index b22fe3d363af3..9cd70ac5ba61d 100644 --- a/core/executor/src/wasm_executor.rs +++ b/core/executor/src/wasm_executor.rs @@ -24,7 +24,7 @@ use wasmi::{ Module, ModuleInstance, MemoryInstance, MemoryRef, TableRef, ImportsBuilder, ModuleRef, memory_units::Pages, RuntimeValue::{I32, I64, self}, }; -use state_machine::{Externalities, ChildStorageKey}; +use state_machine::{Externalities, StorageExternalities, ChildStorageKey}; use crate::error::{Error, Result}; use codec::Encode; use primitives::{ @@ -50,10 +50,44 @@ struct FunctionExecutor<'e, E: Externalities + 'e> { heap: allocator::FreeingBumpHeapAllocator, memory: MemoryRef, table: Option, - ext: &'e mut E, + ext: ExtHideout<'e, E>, hash_lookup: HashMap, Vec>, } +/// Hides externalities reference so that it is hard to reference externalities directly. +struct ExtHideout<'e, E: Externalities + 'e>(&'e mut E); + +impl<'e, E: Externalities + 'e> ExtHideout<'e, E> { + /// Execute closure that has access to externalities. + /// + /// All panics that happen within closure **are NOT** captured. + pub fn with_ext(&mut self, f: F) -> Result + where + F: FnOnce(&mut dyn Externalities) -> Result + { + f(&mut *self.0) + } + + /// Execute closure that has access to storage externalities part. + /// + /// All panics that happen within closure are captured and transformed into + /// runtime error. This requires special panic handler mode to be enabled + /// during the call (see `panic_handler::AbortGuard::never_abort`). + /// If this mode isn't enabled, then all panics within externalities are + /// leading to process abort. + pub fn with_storage_ext(&mut self, f: F) -> Result + where + F: ::std::panic::UnwindSafe + FnOnce(&mut dyn StorageExternalities) -> Result + { + // it is safe beause basic methods of StorageExternalities are guaranteed to touch only + // its internal state + we should discard it on error + let mut ext = std::panic::AssertUnwindSafe(&mut *self.0); + std::panic::catch_unwind(move || f(&mut **ext)) + .map_err(|_| Error::Runtime) + .and_then(|result| result) + } +} + impl<'e, E: Externalities> FunctionExecutor<'e, E> { fn new(m: MemoryRef, heap_base: u32, t: Option, e: &'e mut E) -> Result { Ok(FunctionExecutor { @@ -61,7 +95,7 @@ impl<'e, E: Externalities> FunctionExecutor<'e, E> { heap: allocator::FreeingBumpHeapAllocator::new(m.clone(), heap_base), memory: m, table: t, - ext: e, + ext: ExtHideout(e), hash_lookup: HashMap::new(), }) } @@ -172,7 +206,7 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, HexDisplay::from(&key), ); } - this.ext.set_storage(key, value); + this.ext.with_storage_ext(move |ext| Ok(ext.set_storage(key, value)))?; Ok(()) }, ext_set_child_storage( @@ -208,7 +242,7 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, } let storage_key = ChildStorageKey::from_vec(storage_key) .ok_or_else(|| "ext_set_child_storage: child storage key is invalid")?; - this.ext.set_child_storage(storage_key, key, value); + this.ext.with_storage_ext(move |ext| Ok(ext.set_child_storage(storage_key, key, value)))?; Ok(()) }, ext_clear_child_storage( @@ -236,7 +270,7 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, let storage_key = ChildStorageKey::from_vec(storage_key) .ok_or_else(|| "ext_clear_child_storage: child storage key is not valid")?; - this.ext.clear_child_storage(storage_key, &key); + this.ext.with_storage_ext(move |ext| Ok(ext.clear_child_storage(storage_key, &key)))?; Ok(()) }, ext_clear_storage(key_data: *const u8, key_len: u32) => { @@ -251,13 +285,14 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, }, HexDisplay::from(&key) ); - this.ext.clear_storage(&key); + this.ext.with_storage_ext(move |ext| Ok(ext.clear_storage(&key)))?; Ok(()) }, ext_exists_storage(key_data: *const u8, key_len: u32) -> u32 => { let key = this.memory.get(key_data, key_len as usize) .map_err(|_| "Invalid attempt to determine key in ext_exists_storage")?; - Ok(if this.ext.exists_storage(&key) { 1 } else { 0 }) + let exists_storage = this.ext.with_storage_ext(move |ext| Ok(ext.exists_storage(&key)))?; + Ok(if exists_storage { 1 } else { 0 }) }, ext_exists_child_storage( storage_key_data: *const u8, @@ -273,12 +308,13 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, .map_err(|_| "Invalid attempt to determine key in ext_exists_child_storage")?; let storage_key = ChildStorageKey::from_vec(storage_key) .ok_or_else(|| "ext_exists_child_storage: child storage key is not valid")?; - Ok(if this.ext.exists_child_storage(storage_key, &key) { 1 } else { 0 }) + let exists_child_storage = this.ext.with_storage_ext(move |ext| Ok(ext.exists_child_storage(storage_key, &key)))?; + Ok(if exists_child_storage { 1 } else { 0 }) }, ext_clear_prefix(prefix_data: *const u8, prefix_len: u32) => { let prefix = this.memory.get(prefix_data, prefix_len as usize) .map_err(|_| "Invalid attempt to determine prefix in ext_clear_prefix")?; - this.ext.clear_prefix(&prefix); + this.ext.with_storage_ext(move |ext| Ok(ext.clear_prefix(&prefix)))?; Ok(()) }, ext_clear_child_prefix( @@ -295,7 +331,7 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, .ok_or_else(|| "ext_clear_child_prefix: child storage key is not valid")?; let prefix = this.memory.get(prefix_data, prefix_len as usize) .map_err(|_| "Invalid attempt to determine prefix in ext_clear_child_prefix")?; - this.ext.clear_child_prefix(storage_key, &prefix); + this.ext.with_storage_ext(move |ext| Ok(ext.clear_child_prefix(storage_key, &prefix)))?; Ok(()) }, ext_kill_child_storage(storage_key_data: *const u8, storage_key_len: u32) => { @@ -305,7 +341,7 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, ).map_err(|_| "Invalid attempt to determine storage_key in ext_kill_child_storage")?; let storage_key = ChildStorageKey::from_vec(storage_key) .ok_or_else(|| "ext_exists_child_storage: child storage key is not valid")?; - this.ext.kill_child_storage(storage_key); + this.ext.with_storage_ext(move |ext| Ok(ext.kill_child_storage(storage_key)))?; Ok(()) }, // return 0 and place u32::max_value() into written_out if no value exists for the key. @@ -314,7 +350,7 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, key_data, key_len as usize ).map_err(|_| "Invalid attempt to determine key in ext_get_allocated_storage")?; - let maybe_value = this.ext.storage(&key); + let maybe_value = this.ext.with_storage_ext(move |ext| Ok(ext.storage(&key)))?; debug_trace!( target: "wasm-trace", "*** Getting storage: {} == {} [k={}]", @@ -364,7 +400,7 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, let maybe_value = { let storage_key = ChildStorageKey::from_slice(&storage_key) .ok_or_else(|| "ext_get_allocated_child_storage: child storage key is not valid")?; - this.ext.child_storage(storage_key, &key) + this.ext.with_storage_ext(move |ext| Ok(ext.child_storage(storage_key, &key)))? }; debug_trace!( @@ -406,7 +442,7 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, ) -> u32 => { let key = this.memory.get(key_data, key_len as usize) .map_err(|_| "Invalid attempt to get key in ext_get_storage_into")?; - let maybe_value = this.ext.storage(&key); + let maybe_value = this.ext.with_storage_ext(move |ext| Ok(ext.storage(&key)))?; debug_trace!( target: "wasm-trace", "*** Getting storage: {} == {} [k={}]", if let Some(_preimage) = this.hash_lookup.get(&key) { @@ -454,7 +490,7 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, let maybe_value = { let storage_key = ChildStorageKey::from_slice(&*storage_key) .ok_or_else(|| "ext_get_child_storage_into: child storage key is not valid")?; - this.ext.child_storage(storage_key, &key) + this.ext.with_storage_ext(move |ext| Ok(ext.child_storage(storage_key, &key)))? }; debug_trace!( target: "wasm-trace", "*** Getting storage: {} -> {} == {} [k={}]", @@ -483,7 +519,7 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, } }, ext_storage_root(result: *mut u8) => { - let r = this.ext.storage_root(); + let r = this.ext.with_storage_ext(move |ext| Ok(ext.storage_root()))?; this.memory.set(result, r.as_ref()) .map_err(|_| "Invalid attempt to set memory in ext_storage_root")?; Ok(()) @@ -497,7 +533,7 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, .map_err(|_| "Invalid attempt to determine storage_key in ext_child_storage_root")?; let storage_key = ChildStorageKey::from_slice(&*storage_key) .ok_or_else(|| "ext_child_storage_root: child storage key is not valid")?; - let value = this.ext.child_storage_root(storage_key); + let value = this.ext.with_storage_ext(move |ext| Ok(ext.child_storage_root(storage_key)))?; let offset = this.heap.allocate(value.len() as u32)? as u32; this.memory.set(offset, &value) @@ -514,8 +550,9 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, let raw_parent_hash = this.memory.get(parent_hash_data, parent_hash_len as usize) .map_err(|_| "Invalid attempt to get parent_hash in ext_storage_changes_root")?; parent_hash.as_mut().copy_from_slice(&raw_parent_hash[..]); - let r = this.ext.storage_changes_root(parent_hash) - .map_err(|_| "Invaid parent_hash passed to ext_storage_changes_root")?; + let r = this.ext.with_storage_ext(move |ext| ext.storage_changes_root(parent_hash) + .map_err(|_| "Invalid parent_hash passed to ext_storage_changes_root".into()) + )?; if let Some(r) = r { this.memory.set(result, &r[..]) .map_err(|_| "Invalid attempt to set memory in ext_storage_changes_root")?; @@ -550,7 +587,7 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, Ok(()) }, ext_chain_id() -> u64 => { - Ok(this.ext.chain_id()) + Ok(this.ext.with_storage_ext(move |ext| Ok(ext.chain_id()))?) }, ext_twox_64(data: *const u8, len: u32, out: *mut u8) => { let result: [u8; 8] = if len == 0 { @@ -663,12 +700,14 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, .map_err(|_| "Invalid attempt to get id in ext_ed25519_public_keys")?; let key_type = KeyTypeId(id); - let keys = this.ext + let keys = this.ext.with_ext(|ext| ext .keystore() - .ok_or("No `keystore` associated for the current context!")? - .read() - .ed25519_public_keys(key_type) - .encode(); + .ok_or_else(|| "No `keystore` associated for the current context!".into()) + .map(|keystore| keystore + .read() + .ed25519_public_keys(key_type) + .encode() + ))?; let len = keys.len() as u32; let offset = this.heap.allocate(len)? as u32; @@ -722,12 +761,14 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, .map_err(|_| "Seed not a valid utf8 string in ext_sr25119_generate") ).transpose()?; - let pubkey = this.ext + let pubkey = this.ext.with_ext(|ext| ext .keystore() - .ok_or("No `keystore` associated for the current context!")? - .write() - .ed25519_generate_new(key_type, seed) - .map_err(|_| "`ed25519` key generation failed")?; + .ok_or_else(|| "No `keystore` associated for the current context!".into()) + .and_then(|keystore| keystore + .write() + .ed25519_generate_new(key_type, seed) + .map_err(|_| "`ed25519` key generation failed".into()) + ))?; this.memory.set(out, pubkey.as_ref()) .map_err(|_| "Invalid attempt to set out in ext_ed25519_generate".into()) @@ -754,12 +795,14 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, let pub_key = ed25519::Public::try_from(pubkey.as_ref()) .map_err(|_| "Invalid `ed25519` public key")?; - let signature = this.ext + let signature = this.ext.with_ext(|ext| ext .keystore() - .ok_or("No `keystore` associated for the current context!")? - .read() - .ed25519_key_pair(key_type, &pub_key) - .map(|k| k.sign(msg.as_ref())); + .ok_or_else(|| "No `keystore` associated for the current context!".into()) + .map(|keystore| keystore + .read() + .ed25519_key_pair(key_type, &pub_key) + .map(|k| k.sign(msg.as_ref())) + ))?; match signature { Some(signature) => { @@ -777,12 +820,14 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, .map_err(|_| "Invalid attempt to get id in ext_sr25519_public_keys")?; let key_type = KeyTypeId(id); - let keys = this.ext + let keys = this.ext.with_ext(|ext| ext .keystore() - .ok_or("No `keystore` associated for the current context!")? - .read() - .sr25519_public_keys(key_type) - .encode(); + .ok_or_else(|| "No `keystore` associated for the current context!".into()) + .map(|keystore| keystore + .read() + .sr25519_public_keys(key_type) + .encode() + ))?; let len = keys.len() as u32; let offset = this.heap.allocate(len)? as u32; @@ -836,12 +881,14 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, ) .transpose()?; - let pubkey = this.ext + let pubkey = this.ext.with_ext(|ext| ext .keystore() - .ok_or("No `keystore` associated for the current context!")? - .write() - .sr25519_generate_new(key_type, seed) - .map_err(|_| "`sr25519` key generation failed")?; + .ok_or_else(|| "No `keystore` associated for the current context!".into()) + .and_then(|keystore| keystore + .write() + .sr25519_generate_new(key_type, seed) + .map_err(|_| "`sr25519` key generation failed".into()) + ))?; this.memory.set(out, pubkey.as_ref()) .map_err(|_| "Invalid attempt to set out in ext_sr25519_generate".into()) @@ -868,12 +915,14 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, let pub_key = sr25519::Public::try_from(pubkey.as_ref()) .map_err(|_| "Invalid `sr25519` public key")?; - let signature = this.ext + let signature = this.ext.with_ext(|ext| ext .keystore() - .ok_or("No `keystore` associated for the current context!")? - .read() - .sr25519_key_pair(key_type, &pub_key) - .map(|k| k.sign(msg.as_ref())); + .ok_or_else(|| "No `keystore` associated for the current context!".into()) + .map(|keystore| keystore + .read() + .sr25519_key_pair(key_type, &pub_key) + .map(|k| k.sign(msg.as_ref())) + ))?; match signature { Some(signature) => { @@ -913,24 +962,24 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, Ok(0) }, ext_is_validator() -> u32 => { - this.ext.offchain() + this.ext.with_ext(|ext| ext.offchain() .map(|o| if o.is_validator() { 1 } else { 0 }) - .ok_or("Calling unavailable API ext_is_validator: wasm".into()) + .ok_or_else(|| "Calling unavailable API ext_is_validator: wasm".into())) }, ext_submit_transaction(msg_data: *const u8, len: u32) -> u32 => { let extrinsic = this.memory.get(msg_data, len as usize) .map_err(|_| "OOB while ext_submit_transaction: wasm")?; - let res = this.ext.offchain() + let res = this.ext.with_ext(|ext| ext.offchain() .map(|api| api.submit_transaction(extrinsic)) - .ok_or_else(|| "Calling unavailable API ext_submit_transaction: wasm")?; + .ok_or_else(|| "Calling unavailable API ext_submit_transaction: wasm".into())); Ok(if res.is_ok() { 0 } else { 1 }) }, ext_network_state(written_out: *mut u32) -> *mut u8 => { - let res = this.ext.offchain() + let res = this.ext.with_ext(|ext| ext.offchain() .map(|api| api.network_state()) - .ok_or_else(|| "Calling unavailable API ext_network_state: wasm")?; + .ok_or_else(|| "Calling unavailable API ext_network_state: wasm".into()))?; let encoded = res.encode(); let len = encoded.len() as u32; @@ -944,22 +993,22 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, Ok(offset) }, ext_timestamp() -> u64 => { - let timestamp = this.ext.offchain() + let timestamp = this.ext.with_ext(|ext| ext.offchain() .map(|api| api.timestamp()) - .ok_or_else(|| "Calling unavailable API ext_timestamp: wasm")?; + .ok_or_else(|| "Calling unavailable API ext_timestamp: wasm".into()))?; Ok(timestamp.unix_millis()) }, ext_sleep_until(deadline: u64) => { - this.ext.offchain() + this.ext.with_ext(|ext| ext.offchain() .map(|api| api.sleep_until(offchain::Timestamp::from_unix_millis(deadline))) - .ok_or_else(|| "Calling unavailable API ext_sleep_until: wasm")?; + .ok_or_else(|| "Calling unavailable API ext_sleep_until: wasm".into()))?; Ok(()) }, ext_random_seed(seed_data: *mut u8) => { // NOTE the runtime as assumptions about seed size. - let seed: [u8; 32] = this.ext.offchain() + let seed: [u8; 32] = this.ext.with_ext(|ext| ext.offchain() .map(|api| api.random_seed()) - .ok_or_else(|| "Calling unavailable API ext_random_seed: wasm")?; + .ok_or_else(|| "Calling unavailable API ext_random_seed: wasm".into()))?; this.memory.set(seed_data, &seed) .map_err(|_| "Invalid attempt to set value in ext_random_seed")?; @@ -973,9 +1022,9 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, let value = this.memory.get(value, value_len as usize) .map_err(|_| "OOB while ext_local_storage_set: wasm")?; - this.ext.offchain() + this.ext.with_ext(|ext| ext.offchain() .map(|api| api.local_storage_set(kind, &key, &value)) - .ok_or_else(|| "Calling unavailable API ext_local_storage_set: wasm")?; + .ok_or_else(|| "Calling unavailable API ext_local_storage_set: wasm".into()))?; Ok(()) }, @@ -985,9 +1034,9 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, let key = this.memory.get(key, key_len as usize) .map_err(|_| "OOB while ext_local_storage_get: wasm")?; - let maybe_value = this.ext.offchain() + let maybe_value = this.ext.with_ext(|ext| ext.offchain() .map(|api| api.local_storage_get(kind, &key)) - .ok_or_else(|| "Calling unavailable API ext_local_storage_get: wasm")?; + .ok_or_else(|| "Calling unavailable API ext_local_storage_get: wasm".into()))?; let (offset, len) = if let Some(value) = maybe_value { let offset = this.heap.allocate(value.len() as u32)? as u32; @@ -1021,15 +1070,15 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, let res = { if old_value_len == u32::max_value() { - this.ext.offchain() + this.ext.with_ext(|ext| ext.offchain() .map(|api| api.local_storage_compare_and_set(kind, &key, None, &new_value)) - .ok_or_else(|| "Calling unavailable API ext_local_storage_compare_and_set: wasm")? + .ok_or_else(|| "Calling unavailable API ext_local_storage_compare_and_set: wasm".into()))? } else { let v = this.memory.get(old_value, old_value_len as usize) .map_err(|_| "OOB while ext_local_storage_compare_and_set: wasm")?; - this.ext.offchain() + this.ext.with_ext(|ext| ext.offchain() .map(|api| api.local_storage_compare_and_set(kind, &key, Some(v.as_slice()), &new_value)) - .ok_or_else(|| "Calling unavailable API ext_local_storage_compare_and_set: wasm")? + .ok_or_else(|| "Calling unavailable API ext_local_storage_compare_and_set: wasm".into()))? } }; @@ -1055,9 +1104,9 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, let url_str = str::from_utf8(&url) .map_err(|_| "invalid str while ext_http_request_start: wasm")?; - let id = this.ext.offchain() + let id = this.ext.with_ext(|ext| ext.offchain() .map(|api| api.http_request_start(method_str, url_str, &*meta)) - .ok_or_else(|| "Calling unavailable API ext_http_request_start: wasm")?; + .ok_or_else(|| "Calling unavailable API ext_http_request_start: wasm".into()))?; if let Ok(id) = id { Ok(id.into()) @@ -1082,13 +1131,13 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, let value_str = str::from_utf8(&value) .map_err(|_| "Invalid str while ext_http_request_add_header: wasm")?; - let res = this.ext.offchain() + let res = this.ext.with_ext(|ext| ext.offchain() .map(|api| api.http_request_add_header( offchain::HttpRequestId(request_id as u16), &name_str, &value_str, )) - .ok_or_else(|| "Calling unavailable API ext_http_request_add_header: wasm")?; + .ok_or_else(|| "Calling unavailable API ext_http_request_add_header: wasm".into()))?; Ok(if res.is_ok() { 0 } else { 1 }) }, @@ -1101,13 +1150,13 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, let chunk = this.memory.get(chunk, chunk_len as usize) .map_err(|_| "OOB while ext_http_request_write_body: wasm")?; - let res = this.ext.offchain() + let res = this.ext.with_ext(|ext| ext.offchain() .map(|api| api.http_request_write_body( offchain::HttpRequestId(request_id as u16), &chunk, deadline_to_timestamp(deadline) )) - .ok_or_else(|| "Calling unavailable API ext_http_request_write_body: wasm")?; + .ok_or_else(|| "Calling unavailable API ext_http_request_write_body: wasm".into()))?; Ok(match res { Ok(()) => 0, @@ -1128,14 +1177,15 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, ) .collect::<::std::result::Result, _>>()?; - let res = this.ext.offchain() + let res = this.ext.with_ext(|ext| ext.offchain() .map(|api| api.http_response_wait(&ids, deadline_to_timestamp(deadline))) - .ok_or_else(|| "Calling unavailable API ext_http_response_wait: wasm")? - .into_iter() - .map(|status| status.into()) - .enumerate() - // make sure to take up to `ids_len` to avoid exceeding the mem. - .take(ids_len as usize); + .ok_or_else(|| "Calling unavailable API ext_http_response_wait: wasm".into()) + .map(|response| response + .into_iter() + .map(|status| status.into()) + .enumerate() + // make sure to take up to `ids_len` to avoid exceeding the mem. + .take(ids_len as usize)))?; for (i, status) in res { this.memory.write_primitive(statuses + i as u32 * 4, status) @@ -1150,9 +1200,9 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, ) -> *mut u8 => { use codec::Encode; - let headers = this.ext.offchain() + let headers = this.ext.with_ext(|ext| ext.offchain() .map(|api| api.http_response_headers(offchain::HttpRequestId(request_id as u16))) - .ok_or_else(|| "Calling unavailable API ext_http_response_headers: wasm")?; + .ok_or_else(|| "Calling unavailable API ext_http_response_headers: wasm".into()))?; let encoded = headers.encode(); let len = encoded.len() as u32; @@ -1173,13 +1223,14 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, let mut internal_buffer = Vec::with_capacity(buffer_len as usize); internal_buffer.resize(buffer_len as usize, 0); - let res = this.ext.offchain() + // it is safe because we return from function on error, before internal_buffer is used + let res = this.ext.with_ext(|ext| ext.offchain() .map(|api| api.http_response_read_body( offchain::HttpRequestId(request_id as u16), &mut internal_buffer, deadline_to_timestamp(deadline), )) - .ok_or_else(|| "Calling unavailable API ext_http_response_read_body: wasm")?; + .ok_or_else(|| "Calling unavailable API ext_http_response_read_body: wasm".into()))?; Ok(match res { Ok(read) => { diff --git a/core/panic-handler/src/lib.rs b/core/panic-handler/src/lib.rs index 287ff72a6aa1c..2c04700e96950 100644 --- a/core/panic-handler/src/lib.rs +++ b/core/panic-handler/src/lib.rs @@ -31,7 +31,18 @@ use std::cell::Cell; use std::thread; thread_local! { - static ABORT: Cell = Cell::new(true); + static ON_PANIC: Cell = Cell::new(OnPanic::Abort); +} + +/// Panic action. +#[derive(Debug, Clone, Copy, PartialEq)] +enum OnPanic { + /// Abort when panic occurs. + Abort, + /// Unwind when panic occurs. + Unwind, + /// Always unwind even if someone changes strategy to Abort afterwards. + NeverAbort, } /// Set the panic hook. @@ -52,10 +63,13 @@ This is a bug. Please report it at: ")} /// Set aborting flag. Returns previous value of the flag. -fn set_abort(enabled: bool) -> bool { - ABORT.with(|flag| { - let prev = flag.get(); - flag.set(enabled); +fn set_abort(on_panic: OnPanic) -> OnPanic { + ON_PANIC.with(|val| { + let prev = val.get(); + match prev { + OnPanic::Abort | OnPanic::Unwind => val.set(on_panic), + OnPanic::NeverAbort => (), + } prev }) } @@ -69,7 +83,7 @@ fn set_abort(enabled: bool) -> bool { /// > the `AbortGuard` on the stack and let it destroy itself naturally. pub struct AbortGuard { /// Value that was in `ABORT` before we created this guard. - previous_val: bool, + previous_val: OnPanic, /// Marker so that `AbortGuard` doesn't implement `Send`. _not_send: PhantomData> } @@ -79,7 +93,7 @@ impl AbortGuard { /// unwind the stack (unless another guard is created afterwards). pub fn force_unwind() -> AbortGuard { AbortGuard { - previous_val: set_abort(false), + previous_val: set_abort(OnPanic::Unwind), _not_send: PhantomData } } @@ -88,7 +102,16 @@ impl AbortGuard { /// abort the process (unless another guard is created afterwards). pub fn force_abort() -> AbortGuard { AbortGuard { - previous_val: set_abort(true), + previous_val: set_abort(OnPanic::Abort), + _not_send: PhantomData + } + } + + /// Create a new guard. While the guard is alive, panics that happen in the current thread will + /// **never** abort the process (even if `AbortGuard::force_abort()` guard will be created afterwards). + pub fn never_abort() -> AbortGuard { + AbortGuard { + previous_val: set_abort(OnPanic::NeverAbort), _not_send: PhantomData } } @@ -133,8 +156,8 @@ fn panic_hook(info: &PanicInfo, report_url: &'static str) { ); let _ = writeln!(stderr, ABOUT_PANIC!(), report_url); - ABORT.with(|flag| { - if flag.get() { + ON_PANIC.with(|val| { + if val.get() == OnPanic::Abort { ::std::process::exit(1); } }) @@ -150,4 +173,12 @@ mod tests { let _guard = AbortGuard::force_unwind(); ::std::panic::catch_unwind(|| panic!()).ok(); } + + #[test] + fn does_not_abort_after_never_abort() { + set("test"); + let _guard = AbortGuard::never_abort(); + let _guard = AbortGuard::force_abort(); + std::panic::catch_unwind(|| panic!()).ok(); + } } diff --git a/core/sr-io/with_std.rs b/core/sr-io/with_std.rs index 8e6a160cf203a..23c2477c3fea1 100644 --- a/core/sr-io/with_std.rs +++ b/core/sr-io/with_std.rs @@ -20,7 +20,7 @@ use primitives::{ // Switch to this after PoC-3 // pub use primitives::BlakeHasher; pub use substrate_state_machine::{ - Externalities, BasicExternalities, TestExternalities, ChildStorageKey, + Externalities, StorageExternalities, BasicExternalities, TestExternalities, ChildStorageKey, }; use environmental::environmental; diff --git a/core/state-machine/src/basic.rs b/core/state-machine/src/basic.rs index 1d36a0ddad51d..bcc03afa59701 100644 --- a/core/state-machine/src/basic.rs +++ b/core/state-machine/src/basic.rs @@ -24,7 +24,7 @@ use trie::{TrieConfiguration, default_child_trie_root}; use trie::trie_types::Layout; use primitives::offchain; use primitives::storage::well_known_keys::is_child_storage_key; -use super::{ChildStorageKey, Externalities}; +use super::{ChildStorageKey, Externalities, StorageExternalities}; use log::warn; /// Simple HashMap-based Externalities impl. @@ -35,7 +35,6 @@ pub struct BasicExternalities { } impl BasicExternalities { - /// Create a new instance of `BasicExternalities` pub fn new( top: HashMap, Vec>, @@ -88,7 +87,7 @@ impl From, Vec>> for BasicExternalities { } } -impl Externalities for BasicExternalities where H::Out: Ord { +impl StorageExternalities for BasicExternalities where H::Out: Ord { fn storage(&self, key: &[u8]) -> Option> { self.top.get(key).cloned() } @@ -189,7 +188,9 @@ impl Externalities for BasicExternalities where H::Out: Ord { fn storage_changes_root(&mut self, _parent: H::Out) -> Result, ()> { Ok(None) } +} +impl Externalities for BasicExternalities where H::Out: Ord { fn offchain(&mut self) -> Option<&mut dyn offchain::Externalities> { warn!("Call to non-existent offchain externalities set."); None diff --git a/core/state-machine/src/ext.rs b/core/state-machine/src/ext.rs index 896b07c6473ff..e7a52cac6a6ea 100644 --- a/core/state-machine/src/ext.rs +++ b/core/state-machine/src/ext.rs @@ -20,7 +20,7 @@ use std::{error, fmt, cmp::Ord}; use log::warn; use crate::backend::Backend; use crate::changes_trie::{Storage as ChangesTrieStorage, build_changes_trie}; -use crate::{Externalities, OverlayedChanges, ChildStorageKey}; +use crate::{Externalities, StorageExternalities, OverlayedChanges, ChildStorageKey}; use hash_db::Hasher; use primitives::{offchain, storage::well_known_keys::is_child_storage_key, traits::BareCryptoStorePtr}; use trie::{MemoryDB, default_child_trie_root}; @@ -167,7 +167,7 @@ where } } -impl<'a, B, T, H, N, O> Externalities for Ext<'a, H, N, B, T, O> +impl<'a, B, T, H, N, O> StorageExternalities for Ext<'a, H, N, B, T, O> where H: Hasher, B: 'a + Backend, @@ -357,7 +357,16 @@ where )?; Ok(self.changes_trie_transaction.as_ref().map(|(_, root)| root.clone())) } +} +impl<'a, B, T, H, N, O> Externalities for Ext<'a, H, N, B, T, O> +where H: Hasher, + B: 'a + Backend, + T: 'a + ChangesTrieStorage, + H::Out: Ord + 'static, + N: crate::changes_trie::BlockNumber, + O: 'a + offchain::Externalities, +{ fn offchain(&mut self) -> Option<&mut dyn offchain::Externalities> { self.offchain_externalities.as_mut().map(|x| &mut **x as _) } @@ -365,6 +374,7 @@ where fn keystore(&self) -> Option { self.keystore.clone() } + } #[cfg(test)] diff --git a/core/state-machine/src/lib.rs b/core/state-machine/src/lib.rs index 8c2046e5917c6..5eb9aa28899b7 100644 --- a/core/state-machine/src/lib.rs +++ b/core/state-machine/src/lib.rs @@ -145,8 +145,14 @@ impl fmt::Display for ExecutionError { type CallResult = Result, E>; -/// Externalities: pinned to specific active address. -pub trait Externalities { +/// Storage externalities API: pinned to specific active address. +/// +/// Implementations of methods of this trait guarantee that there are no side effects, +/// other than modifications to some **internal state** which could easily be +/// discarded by throwing this instance of this Externalities away. +/// +/// Every method may panic if storage access fails. +pub trait StorageExternalities { /// Read runtime storage. fn storage(&self, key: &[u8]) -> Option>; @@ -233,7 +239,9 @@ pub trait Externalities { /// Get the identity of the chain. fn chain_id(&self) -> u64; - /// Get the trie root of the current storage map. This will also update all child storage keys in the top-level storage map. + /// Get the trie root of the current storage map. + /// + /// This will also update all child storage keys in the top-level storage map. fn storage_root(&mut self) -> H::Out where H::Out: Ord; /// Get the trie root of a child storage map. This will also update the value of the child @@ -244,7 +252,13 @@ pub trait Externalities { /// Get the change trie root of the current storage overlay at a block with given parent. fn storage_changes_root(&mut self, parent: H::Out) -> Result, ()> where H::Out: Ord; +} +/// Extra externalities API: pinned to specific active address. +/// +/// Methods of implementations of this trait may have some side-effects, thus it isn't safe +/// to catch unwindings of any panics that occur during these methods call. +pub trait Externalities: StorageExternalities { /// Returns offchain externalities extension if present. fn offchain(&mut self) -> Option<&mut dyn offchain::Externalities>; @@ -367,7 +381,9 @@ pub trait CodeExecutor: Sized + Send + Sync { /// Call a given method in the runtime. Returns a tuple of the result (either the output data /// or an execution error) together with a `bool`, which is true if native execution was used. fn call< - E: Externalities, R: Encode + Decode + PartialEq, NC: FnOnce() -> result::Result + UnwindSafe + E: Externalities, + R: Encode + Decode + PartialEq, + NC: FnOnce() -> result::Result + UnwindSafe >( &self, ext: &mut E, @@ -401,8 +417,11 @@ type DefaultHandler = fn( pub enum ExecutionManager { /// Execute with the native equivalent if it is compatible with the given wasm module; otherwise fall back to the wasm. NativeWhenPossible, - /// Use the given wasm module. - AlwaysWasm, + /// Use the given wasm module. The backend on which code is executed code could be + /// trusted (true) or untrusted (false). + /// If untrusted, all panics that occurs within externalities are treated as runtime execution error. + /// If trusted, they are considered fatal && aren't catched. + AlwaysWasm(bool), /// Run with both the wasm and the native variant (if compatible). Call `F` in the case of any discrepency. Both(F), /// First native, then if that fails or is not possible, wasm. @@ -413,7 +432,7 @@ impl<'a, F> From<&'a ExecutionManager> for ExecutionStrategy { fn from(s: &'a ExecutionManager) -> Self { match *s { ExecutionManager::NativeWhenPossible => ExecutionStrategy::NativeWhenPossible, - ExecutionManager::AlwaysWasm => ExecutionStrategy::AlwaysWasm, + ExecutionManager::AlwaysWasm(_) => ExecutionStrategy::AlwaysWasm, ExecutionManager::NativeElseWasm => ExecutionStrategy::NativeElseWasm, ExecutionManager::Both(_) => ExecutionStrategy::Both, } @@ -424,7 +443,7 @@ impl ExecutionStrategy { /// Gets the corresponding manager for the execution strategy. pub fn get_manager(self) -> ExecutionManager> { match self { - ExecutionStrategy::AlwaysWasm => ExecutionManager::AlwaysWasm, + ExecutionStrategy::AlwaysWasm => ExecutionManager::AlwaysWasm(true), ExecutionStrategy::NativeWhenPossible => ExecutionManager::NativeWhenPossible, ExecutionStrategy::NativeElseWasm => ExecutionManager::NativeElseWasm, ExecutionStrategy::Both => ExecutionManager::Both(|wasm_result, native_result| { @@ -450,9 +469,14 @@ pub fn native_else_wasm() -> ExecutionManager ExecutionManager::NativeElseWasm } -/// Evaluate to ExecutionManager::NativeWhenPossible, without having to figure out the type. +/// Evaluate to ExecutionManager::AlwaysWasm with trusted backend, without having to figure out the type. pub fn always_wasm() -> ExecutionManager> { - ExecutionManager::AlwaysWasm + ExecutionManager::AlwaysWasm(true) +} + +/// Evaluate ExecutionManager::AlwaysWasm with untrusted backend, without having to figure out the type. +fn always_untrusted_wasm() -> ExecutionManager> { + ExecutionManager::AlwaysWasm(false) } /// Creates new substrate state machine. @@ -667,7 +691,12 @@ impl<'a, H, N, B, T, O, Exec> StateMachine<'a, H, N, B, T, O, Exec> where ExecutionManager::NativeElseWasm => { self.execute_call_with_native_else_wasm_strategy(compute_tx, native_call.take(), orig_prospective) }, - ExecutionManager::AlwaysWasm => { + ExecutionManager::AlwaysWasm(trusted) => { + let _abort_guard = if !trusted { + Some(panic_handler::AbortGuard::never_abort()) + } else { + None + }; let (result, _, storage_delta, changes_delta) = self.execute_aux(compute_tx, false, native_call); (result, storage_delta, changes_delta) }, @@ -743,7 +772,7 @@ where _hasher: PhantomData, }; let (result, _, _) = sm.execute_using_consensus_failure_handler::<_, NeverNativeValue, fn() -> _>( - native_else_wasm(), + always_wasm(), false, None, )?; @@ -796,7 +825,7 @@ where _hasher: PhantomData, }; sm.execute_using_consensus_failure_handler::<_, NeverNativeValue, fn() -> _>( - native_else_wasm(), + always_untrusted_wasm(), false, None, ).map(|(result, _, _)| result.into_encoded()) diff --git a/core/state-machine/src/overlayed_changes.rs b/core/state-machine/src/overlayed_changes.rs index 9efafab20f57c..ad08319cc3b0a 100644 --- a/core/state-machine/src/overlayed_changes.rs +++ b/core/state-machine/src/overlayed_changes.rs @@ -338,7 +338,7 @@ mod tests { use crate::backend::InMemory; use crate::changes_trie::InMemoryStorage as InMemoryChangesTrieStorage; use crate::ext::Ext; - use crate::Externalities; + use crate::StorageExternalities; use super::*; fn strip_extrinsic_index(map: &HashMap, OverlayedValue>) -> HashMap, OverlayedValue> { diff --git a/core/state-machine/src/testing.rs b/core/state-machine/src/testing.rs index 00a0208a1f72f..24ccc1f5092ba 100644 --- a/core/state-machine/src/testing.rs +++ b/core/state-machine/src/testing.rs @@ -28,7 +28,7 @@ use primitives::{ storage::well_known_keys::{CHANGES_TRIE_CONFIG, CODE, HEAP_PAGES}, traits::BareCryptoStorePtr, offchain }; use codec::Encode; -use super::{ChildStorageKey, Externalities, OverlayedChanges}; +use super::{ChildStorageKey, Externalities, StorageExternalities, OverlayedChanges}; const EXT_NOT_ALLOWED_TO_FAIL: &str = "Externalities not allowed to fail within runtime"; @@ -141,7 +141,7 @@ impl From for TestExternalit } } -impl Externalities for TestExternalities +impl StorageExternalities for TestExternalities where H: Hasher, N: ChangesTrieBlockNumber, @@ -278,7 +278,14 @@ impl Externalities for TestExternalities parent, )?.map(|(_, root)| root)) } +} +impl Externalities for TestExternalities + where + H: Hasher, + N: ChangesTrieBlockNumber, + H::Out: Ord + 'static +{ fn offchain(&mut self) -> Option<&mut dyn offchain::Externalities> { self.offchain .as_mut() diff --git a/node/executor/src/lib.rs b/node/executor/src/lib.rs index 874c5a1b6db1c..7c4a1fc899f03 100644 --- a/node/executor/src/lib.rs +++ b/node/executor/src/lib.rs @@ -42,7 +42,7 @@ mod tests { use substrate_executor::WasmExecutor; use codec::{Encode, Decode, Joiner}; use runtime_support::{Hashable, StorageValue, StorageMap, assert_eq_error_rate, traits::Currency}; - use state_machine::{CodeExecutor, Externalities, TestExternalities as CoreTestExternalities}; + use state_machine::{CodeExecutor, StorageExternalities, TestExternalities as CoreTestExternalities}; use primitives::{ twox_128, blake2_256, Blake2Hasher, NeverNativeValue, NativeOrEncoded, map }; From bfc78d8410023d5299ff0c2fdcc8fa9b430af216 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Thu, 29 Aug 2019 08:12:15 +0300 Subject: [PATCH 02/13] Update core/state-machine/src/lib.rs Co-Authored-By: DemiMarie-parity <48690212+DemiMarie-parity@users.noreply.github.com> --- core/state-machine/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/state-machine/src/lib.rs b/core/state-machine/src/lib.rs index 5eb9aa28899b7..5b33b8e7a41e8 100644 --- a/core/state-machine/src/lib.rs +++ b/core/state-machine/src/lib.rs @@ -256,7 +256,7 @@ pub trait StorageExternalities { /// Extra externalities API: pinned to specific active address. /// -/// Methods of implementations of this trait may have some side-effects, thus it isn't safe +/// Methods of this trait may have side-effects. Therefore, it isn't safe /// to catch unwindings of any panics that occur during these methods call. pub trait Externalities: StorageExternalities { /// Returns offchain externalities extension if present. From 9d51eeee62e738d77e82890abf134fdf21e3229a Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Thu, 29 Aug 2019 08:12:26 +0300 Subject: [PATCH 03/13] Update core/state-machine/src/lib.rs Co-Authored-By: DemiMarie-parity <48690212+DemiMarie-parity@users.noreply.github.com> --- core/state-machine/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/state-machine/src/lib.rs b/core/state-machine/src/lib.rs index 5b33b8e7a41e8..dac84fd81257f 100644 --- a/core/state-machine/src/lib.rs +++ b/core/state-machine/src/lib.rs @@ -257,7 +257,7 @@ pub trait StorageExternalities { /// Extra externalities API: pinned to specific active address. /// /// Methods of this trait may have side-effects. Therefore, it isn't safe -/// to catch unwindings of any panics that occur during these methods call. +/// to catch unwinding of any panics that occur during calls to them. pub trait Externalities: StorageExternalities { /// Returns offchain externalities extension if present. fn offchain(&mut self) -> Option<&mut dyn offchain::Externalities>; From 25051c019f6007b7c7cda79305bed2d6384d787a Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Thu, 29 Aug 2019 08:12:45 +0300 Subject: [PATCH 04/13] Update core/state-machine/src/lib.rs Co-Authored-By: DemiMarie-parity <48690212+DemiMarie-parity@users.noreply.github.com> --- core/state-machine/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/state-machine/src/lib.rs b/core/state-machine/src/lib.rs index dac84fd81257f..0f8b346db56f8 100644 --- a/core/state-machine/src/lib.rs +++ b/core/state-machine/src/lib.rs @@ -417,7 +417,7 @@ type DefaultHandler = fn( pub enum ExecutionManager { /// Execute with the native equivalent if it is compatible with the given wasm module; otherwise fall back to the wasm. NativeWhenPossible, - /// Use the given wasm module. The backend on which code is executed code could be + /// Use the given wasm module. The backend on which code is executed could be /// trusted (true) or untrusted (false). /// If untrusted, all panics that occurs within externalities are treated as runtime execution error. /// If trusted, they are considered fatal && aren't catched. From 95b64e63e7ea4f1853adc6dfe8f4b4569c4ac36b Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Thu, 29 Aug 2019 08:40:46 +0300 Subject: [PATCH 05/13] BackendTrustLevel enum --- core/client/src/client.rs | 4 ++-- core/state-machine/src/lib.rs | 32 ++++++++++++++++++++------------ 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/core/client/src/client.rs b/core/client/src/client.rs index 270591b596ff1..4a5441493f79f 100644 --- a/core/client/src/client.rs +++ b/core/client/src/client.rs @@ -42,7 +42,7 @@ use sr_primitives::{ use state_machine::{ DBValue, Backend as StateBackend, CodeExecutor, ChangesTrieAnchorBlockId, ExecutionStrategy, ExecutionManager, prove_read, prove_child_read, - ChangesTrieRootsStorage, ChangesTrieStorage, + ChangesTrieRootsStorage, ChangesTrieStorage, BackendTrustLevel, key_changes, key_changes_proof, OverlayedChanges, NeverOffchainExt, }; use executor::{RuntimeVersion, RuntimeInfo}; @@ -1032,7 +1032,7 @@ impl Client where let get_execution_manager = |execution_strategy: ExecutionStrategy| { match execution_strategy { ExecutionStrategy::NativeElseWasm => ExecutionManager::NativeElseWasm, - ExecutionStrategy::AlwaysWasm => ExecutionManager::AlwaysWasm(true), + ExecutionStrategy::AlwaysWasm => ExecutionManager::AlwaysWasm(BackendTrustLevel::Trusted), ExecutionStrategy::NativeWhenPossible => ExecutionManager::NativeWhenPossible, ExecutionStrategy::Both => ExecutionManager::Both(|wasm_result, native_result| { let header = import_headers.post(); diff --git a/core/state-machine/src/lib.rs b/core/state-machine/src/lib.rs index 5eb9aa28899b7..701bdcd7e4c8d 100644 --- a/core/state-machine/src/lib.rs +++ b/core/state-machine/src/lib.rs @@ -412,16 +412,25 @@ type DefaultHandler = fn( CallResult, ) -> CallResult; +/// Storage backend trust level. +#[derive(Debug, Clone)] +pub enum BackendTrustLevel { + /// Panics from trusted backend are justified and never caught. + Trusted, + /// Panics from untrusted backend are caught and interpreted as runtime error. + /// Untrusted backend may miss some parts of trie => panics are not considered + /// fatal. + Untrusted, +} + /// Like `ExecutionStrategy` only it also stores a handler in case of consensus failure. #[derive(Clone)] pub enum ExecutionManager { /// Execute with the native equivalent if it is compatible with the given wasm module; otherwise fall back to the wasm. NativeWhenPossible, /// Use the given wasm module. The backend on which code is executed code could be - /// trusted (true) or untrusted (false). - /// If untrusted, all panics that occurs within externalities are treated as runtime execution error. - /// If trusted, they are considered fatal && aren't catched. - AlwaysWasm(bool), + /// trusted or untrusted. + AlwaysWasm(BackendTrustLevel), /// Run with both the wasm and the native variant (if compatible). Call `F` in the case of any discrepency. Both(F), /// First native, then if that fails or is not possible, wasm. @@ -443,7 +452,7 @@ impl ExecutionStrategy { /// Gets the corresponding manager for the execution strategy. pub fn get_manager(self) -> ExecutionManager> { match self { - ExecutionStrategy::AlwaysWasm => ExecutionManager::AlwaysWasm(true), + ExecutionStrategy::AlwaysWasm => ExecutionManager::AlwaysWasm(BackendTrustLevel::Trusted), ExecutionStrategy::NativeWhenPossible => ExecutionManager::NativeWhenPossible, ExecutionStrategy::NativeElseWasm => ExecutionManager::NativeElseWasm, ExecutionStrategy::Both => ExecutionManager::Both(|wasm_result, native_result| { @@ -471,12 +480,12 @@ pub fn native_else_wasm() -> ExecutionManager /// Evaluate to ExecutionManager::AlwaysWasm with trusted backend, without having to figure out the type. pub fn always_wasm() -> ExecutionManager> { - ExecutionManager::AlwaysWasm(true) + ExecutionManager::AlwaysWasm(BackendTrustLevel::Trusted) } /// Evaluate ExecutionManager::AlwaysWasm with untrusted backend, without having to figure out the type. fn always_untrusted_wasm() -> ExecutionManager> { - ExecutionManager::AlwaysWasm(false) + ExecutionManager::AlwaysWasm(BackendTrustLevel::Untrusted) } /// Creates new substrate state machine. @@ -691,11 +700,10 @@ impl<'a, H, N, B, T, O, Exec> StateMachine<'a, H, N, B, T, O, Exec> where ExecutionManager::NativeElseWasm => { self.execute_call_with_native_else_wasm_strategy(compute_tx, native_call.take(), orig_prospective) }, - ExecutionManager::AlwaysWasm(trusted) => { - let _abort_guard = if !trusted { - Some(panic_handler::AbortGuard::never_abort()) - } else { - None + ExecutionManager::AlwaysWasm(trust_level) => { + let _abort_guard = match trust_level { + BackendTrustLevel::Trusted => None, + BackendTrustLevel::Untrusted => Some(panic_handler::AbortGuard::never_abort()), }; let (result, _, storage_delta, changes_delta) = self.execute_aux(compute_tx, false, native_call); (result, storage_delta, changes_delta) From a5de0cdf63665ab0dcefe0f60d32fbadb9ef925f Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Thu, 29 Aug 2019 08:40:59 +0300 Subject: [PATCH 06/13] up runtime version --- node/runtime/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/runtime/src/lib.rs b/node/runtime/src/lib.rs index 0c5cdefd96f8c..33088260b7d2e 100644 --- a/node/runtime/src/lib.rs +++ b/node/runtime/src/lib.rs @@ -80,7 +80,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { // implementation changes and behavior does not, then leave spec_version as // is and increment impl_version. spec_version: 151, - impl_version: 152, + impl_version: 153, apis: RUNTIME_API_VERSIONS, }; From a16359e5f68029a0b2ddd9926f5942272ede5e50 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Fri, 30 Aug 2019 07:27:05 +0300 Subject: [PATCH 07/13] Update core/state-machine/src/lib.rs Co-Authored-By: DemiMarie-parity <48690212+DemiMarie-parity@users.noreply.github.com> --- core/state-machine/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/state-machine/src/lib.rs b/core/state-machine/src/lib.rs index 8f427b38f7b3a..53271b75d0e51 100644 --- a/core/state-machine/src/lib.rs +++ b/core/state-machine/src/lib.rs @@ -419,7 +419,7 @@ pub enum BackendTrustLevel { /// Panics from trusted backend are justified and never caught. Trusted, /// Panics from untrusted backend are caught and interpreted as runtime error. - /// Untrusted backend may miss some parts of trie => panics are not considered + /// Untrusted backend may be missing some parts of the trie, so panics are not considered /// fatal. Untrusted, } From 36a5b97f57b33da0caeea8941d57fed656711e32 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Fri, 30 Aug 2019 07:27:37 +0300 Subject: [PATCH 08/13] Update core/state-machine/src/lib.rs Co-Authored-By: DemiMarie-parity <48690212+DemiMarie-parity@users.noreply.github.com> --- core/state-machine/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/state-machine/src/lib.rs b/core/state-machine/src/lib.rs index 53271b75d0e51..cd28f2e4247cb 100644 --- a/core/state-machine/src/lib.rs +++ b/core/state-machine/src/lib.rs @@ -416,7 +416,7 @@ type DefaultHandler = fn( /// Storage backend trust level. #[derive(Debug, Clone)] pub enum BackendTrustLevel { - /// Panics from trusted backend are justified and never caught. + /// Panics from trusted backends are considered justified, and never caught. Trusted, /// Panics from untrusted backend are caught and interpreted as runtime error. /// Untrusted backend may be missing some parts of the trie, so panics are not considered From 3d5aaa54fa83a07d64494c52efbfabec9248e539 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Fri, 30 Aug 2019 07:32:09 +0300 Subject: [PATCH 09/13] fixed some grumbles --- core/executor/src/wasm_executor.rs | 8 ++++---- core/state-machine/src/lib.rs | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/core/executor/src/wasm_executor.rs b/core/executor/src/wasm_executor.rs index 9cd70ac5ba61d..e71350e76a2d4 100644 --- a/core/executor/src/wasm_executor.rs +++ b/core/executor/src/wasm_executor.rs @@ -16,7 +16,7 @@ //! Rust implementation of Substrate contracts. -use std::{collections::HashMap, convert::TryFrom, str}; +use std::{collections::HashMap, convert::TryFrom, str, panic}; use tiny_keccak; use secp256k1; @@ -77,12 +77,12 @@ impl<'e, E: Externalities + 'e> ExtHideout<'e, E> { /// leading to process abort. pub fn with_storage_ext(&mut self, f: F) -> Result where - F: ::std::panic::UnwindSafe + FnOnce(&mut dyn StorageExternalities) -> Result + F: panic::UnwindSafe + FnOnce(&mut dyn StorageExternalities) -> Result { // it is safe beause basic methods of StorageExternalities are guaranteed to touch only // its internal state + we should discard it on error - let mut ext = std::panic::AssertUnwindSafe(&mut *self.0); - std::panic::catch_unwind(move || f(&mut **ext)) + let mut ext = panic::AssertUnwindSafe(&mut *self.0); + panic::catch_unwind(move || f(&mut **ext)) .map_err(|_| Error::Runtime) .and_then(|result| result) } diff --git a/core/state-machine/src/lib.rs b/core/state-machine/src/lib.rs index cd28f2e4247cb..65aa25b92f9f6 100644 --- a/core/state-machine/src/lib.rs +++ b/core/state-machine/src/lib.rs @@ -146,7 +146,7 @@ impl fmt::Display for ExecutionError { type CallResult = Result, E>; -/// Storage externalities API: pinned to specific active address. +/// Storage externalities API. /// /// Implementations of methods of this trait guarantee that there are no side effects, /// other than modifications to some **internal state** which could easily be @@ -255,7 +255,7 @@ pub trait StorageExternalities { fn storage_changes_root(&mut self, parent: H::Out) -> Result, ()> where H::Out: Ord; } -/// Extra externalities API: pinned to specific active address. +/// Externalities API. /// /// Methods of this trait may have side-effects. Therefore, it isn't safe /// to catch unwinding of any panics that occur during calls to them. From 36dd630e0e16cfcdc786bd320d57ed9703d64b3c Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Mon, 9 Sep 2019 08:18:58 +0300 Subject: [PATCH 10/13] Update core/state-machine/src/testing.rs Co-Authored-By: Gavin Wood --- core/state-machine/src/testing.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/state-machine/src/testing.rs b/core/state-machine/src/testing.rs index 1d7ccd3be7bd2..20289f0ce5dbb 100644 --- a/core/state-machine/src/testing.rs +++ b/core/state-machine/src/testing.rs @@ -284,7 +284,7 @@ impl Externalities for TestExternalities where H: Hasher, N: ChangesTrieBlockNumber, - H::Out: Ord + 'static + H::Out: Ord + 'static, { fn offchain(&mut self) -> Option<&mut dyn offchain::Externalities> { self.offchain From cbe574affc816548bbb35ec414084972c71e2b2f Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Mon, 9 Sep 2019 08:20:15 +0300 Subject: [PATCH 11/13] Update core/state-machine/src/lib.rs Co-Authored-By: Gavin Wood --- core/state-machine/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/state-machine/src/lib.rs b/core/state-machine/src/lib.rs index 65aa25b92f9f6..a67fc13b0aa97 100644 --- a/core/state-machine/src/lib.rs +++ b/core/state-machine/src/lib.rs @@ -430,7 +430,8 @@ pub enum ExecutionManager { /// Execute with the native equivalent if it is compatible with the given wasm module; otherwise fall back to the wasm. NativeWhenPossible, /// Use the given wasm module. The backend on which code is executed code could be - /// trusted or untrusted. + /// trusted to provide all storage or not (i.e. the light client cannot be trusted to provide + /// for all storage queries since the storage entries it has come from an external node). AlwaysWasm(BackendTrustLevel), /// Run with both the wasm and the native variant (if compatible). Call `F` in the case of any discrepency. Both(F), From 95564c97b793e5104fbda8035c42c39d6dbe37de Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Mon, 9 Sep 2019 08:20:35 +0300 Subject: [PATCH 12/13] mov where --- core/state-machine/src/testing.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/core/state-machine/src/testing.rs b/core/state-machine/src/testing.rs index 20289f0ce5dbb..239e04ac3a0df 100644 --- a/core/state-machine/src/testing.rs +++ b/core/state-machine/src/testing.rs @@ -280,11 +280,10 @@ impl StorageExternalities for TestExternalities } } -impl Externalities for TestExternalities - where - H: Hasher, - N: ChangesTrieBlockNumber, - H::Out: Ord + 'static, +impl Externalities for TestExternalities where + H: Hasher, + N: ChangesTrieBlockNumber, + H::Out: Ord + 'static, { fn offchain(&mut self) -> Option<&mut dyn offchain::Externalities> { self.offchain From e0c0ac718908de580b929f6fad4028727e321cd9 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Wed, 11 Sep 2019 10:04:45 +0300 Subject: [PATCH 13/13] spaces -> tabs (to restart build) --- core/network/src/config.rs | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/core/network/src/config.rs b/core/network/src/config.rs index 46bb8aeff4ddd..7392a4aaf7b4e 100644 --- a/core/network/src/config.rs +++ b/core/network/src/config.rs @@ -205,23 +205,23 @@ pub enum ParseErr { } impl fmt::Display for ParseErr { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - ParseErr::MultiaddrParse(err) => write!(f, "{}", err), - ParseErr::InvalidPeerId => write!(f, "Peer id at the end of the address is invalid"), - ParseErr::PeerIdMissing => write!(f, "Peer id is missing from the address"), - } - } + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + ParseErr::MultiaddrParse(err) => write!(f, "{}", err), + ParseErr::InvalidPeerId => write!(f, "Peer id at the end of the address is invalid"), + ParseErr::PeerIdMissing => write!(f, "Peer id is missing from the address"), + } + } } impl std::error::Error for ParseErr { - fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - match self { - ParseErr::MultiaddrParse(err) => Some(err), - ParseErr::InvalidPeerId => None, - ParseErr::PeerIdMissing => None, - } - } + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match self { + ParseErr::MultiaddrParse(err) => Some(err), + ParseErr::InvalidPeerId => None, + ParseErr::PeerIdMissing => None, + } + } } impl From for ParseErr {