diff --git a/substrate/core/executor/src/wasm_executor.rs b/substrate/core/executor/src/wasm_executor.rs index 42af29e9ba..ce41d86a18 100644 --- a/substrate/core/executor/src/wasm_executor.rs +++ b/substrate/core/executor/src/wasm_executor.rs @@ -25,7 +25,7 @@ use wasmi::{ }; use wasmi::RuntimeValue::{I32, I64, self}; use wasmi::memory_units::{Pages}; -use state_machine::Externalities; +use state_machine::{Externalities, ChildStorageKey}; use crate::error::{Error, ErrorKind, Result}; use crate::wasm_utils::UserError; use primitives::{blake2_256, twox_128, twox_256, ed25519, sr25519, Pair}; @@ -174,6 +174,10 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, HexDisplay::from(&key) ); } + let storage_key = ChildStorageKey::from_vec(storage_key) + .ok_or_else(|| + UserError("ext_set_child_storage: child storage key is invalid") + )?; this.ext.set_child_storage(storage_key, key, value); Ok(()) }, @@ -189,8 +193,13 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, format!("%{}", ::primitives::hexdisplay::ascii_format(&_preimage)) } else { format!(" {}", ::primitives::hexdisplay::ascii_format(&key)) - }, HexDisplay::from(&key)); - this.ext.clear_child_storage(&storage_key, &key); + }, HexDisplay::from(&key) + ); + let storage_key = ChildStorageKey::from_vec(storage_key) + .ok_or_else(|| + UserError("ext_clear_child_storage: child storage key is not valid") + )?; + this.ext.clear_child_storage(storage_key, &key); Ok(()) }, ext_clear_storage(key_data: *const u8, key_len: u32) => { @@ -214,7 +223,11 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, storage_key_len as usize ).map_err(|_| UserError("Invalid attempt to determine storage_key in ext_exists_child_storage"))?; let key = this.memory.get(key_data, key_len as usize).map_err(|_| UserError("Invalid attempt to determine key in ext_exists_child_storage"))?; - Ok(if this.ext.exists_child_storage(&storage_key, &key) { 1 } else { 0 }) + let storage_key = ChildStorageKey::from_vec(storage_key) + .ok_or_else(|| + UserError("ext_exists_child_storage: child storage key is not valid") + )?; + Ok(if this.ext.exists_child_storage(storage_key, &key) { 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(|_| UserError("Invalid attempt to determine prefix in ext_clear_prefix"))?; @@ -226,7 +239,11 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, storage_key_data, storage_key_len as usize ).map_err(|_| UserError("Invalid attempt to determine storage_key in ext_kill_child_storage"))?; - this.ext.kill_child_storage(&storage_key); + let storage_key = ChildStorageKey::from_vec(storage_key) + .ok_or_else(|| + UserError("ext_exists_child_storage: child storage key is not valid") + )?; + this.ext.kill_child_storage(storage_key); Ok(()) }, // return 0 and place u32::max_value() into written_out if no value exists for the key. @@ -273,7 +290,14 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, key_data, key_len as usize ).map_err(|_| UserError("Invalid attempt to determine key in ext_get_allocated_child_storage"))?; - let maybe_value = this.ext.child_storage(&storage_key, &key); + + let maybe_value = { + let storage_key = ChildStorageKey::from_slice(&storage_key) + .ok_or_else(|| + UserError("ext_get_allocated_child_storage: child storage key is not valid") + )?; + this.ext.child_storage(storage_key, &key) + }; debug_trace!(target: "wasm-trace", "*** Getting child storage: {} -> {} == {} [k={}]", ::primitives::hexdisplay::ascii_format(&storage_key), @@ -339,7 +363,14 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, key_data, key_len as usize ).map_err(|_| UserError("Invalid attempt to get key in ext_get_child_storage_into"))?; - let maybe_value = this.ext.child_storage(&storage_key, &key); + + let maybe_value = { + let storage_key = ChildStorageKey::from_slice(&*storage_key) + .ok_or_else(|| + UserError("ext_get_child_storage_into: child storage key is not valid") + )?; + this.ext.child_storage(storage_key, &key) + }; debug_trace!(target: "wasm-trace", "*** Getting storage: {} -> {} == {} [k={}]", ::primitives::hexdisplay::ascii_format(&storage_key), if let Some(_preimage) = this.hash_lookup.get(&key) { @@ -371,18 +402,17 @@ impl_function_executor!(this: FunctionExecutor<'e, E>, }, ext_child_storage_root(storage_key_data: *const u8, storage_key_len: u32, written_out: *mut u32) -> *mut u8 => { let storage_key = this.memory.get(storage_key_data, storage_key_len as usize).map_err(|_| UserError("Invalid attempt to determine storage_key in ext_child_storage_root"))?; - let r = this.ext.child_storage_root(&storage_key); - if let Some(value) = r { - let offset = this.heap.allocate(value.len() as u32)? as u32; - this.memory.set(offset, &value).map_err(|_| UserError("Invalid attempt to set memory in ext_child_storage_root"))?; - this.memory.write_primitive(written_out, value.len() as u32) - .map_err(|_| UserError("Invalid attempt to write written_out in ext_child_storage_root"))?; - Ok(offset) - } else { - this.memory.write_primitive(written_out, u32::max_value()) - .map_err(|_| UserError("Invalid attempt to write failed written_out in ext_child_storage_root"))?; - Ok(0) - } + let storage_key = ChildStorageKey::from_slice(&*storage_key) + .ok_or_else(|| + UserError("ext_child_storage_root: child storage key is not valid") + )?; + let value = this.ext.child_storage_root(storage_key); + + let offset = this.heap.allocate(value.len() as u32)? as u32; + this.memory.set(offset, &value).map_err(|_| UserError("Invalid attempt to set memory in ext_child_storage_root"))?; + this.memory.write_primitive(written_out, value.len() as u32) + .map_err(|_| UserError("Invalid attempt to write written_out in ext_child_storage_root"))?; + Ok(offset) }, ext_storage_changes_root(parent_hash_data: *const u8, parent_hash_len: u32, parent_number: u64, result: *mut u8) -> u32 => { let mut parent_hash = H256::default(); diff --git a/substrate/core/primitives/src/storage.rs b/substrate/core/primitives/src/storage.rs index 79652a8d4c..3203fdb1ba 100644 --- a/substrate/core/primitives/src/storage.rs +++ b/substrate/core/primitives/src/storage.rs @@ -83,8 +83,11 @@ pub mod well_known_keys { pub const CHILD_STORAGE_KEY_PREFIX: &'static [u8] = b":child_storage:"; /// Whether a key is a child storage key. + /// + /// This is convenience function which basically checks if the given `key` starts + /// with `CHILD_STORAGE_KEY_PREFIX` and doesn't do anything apart from that. pub fn is_child_storage_key(key: &[u8]) -> bool { + // Other code might depend on this, so be careful changing this. key.starts_with(CHILD_STORAGE_KEY_PREFIX) } - } diff --git a/substrate/core/sr-io/with_std.rs b/substrate/core/sr-io/with_std.rs index 1f4ce56fc9..bf7147babb 100644 --- a/substrate/core/sr-io/with_std.rs +++ b/substrate/core/sr-io/with_std.rs @@ -24,7 +24,12 @@ pub use primitives::{ pub use tiny_keccak::keccak256 as keccak_256; // Switch to this after PoC-3 // pub use primitives::BlakeHasher; -pub use substrate_state_machine::{Externalities, BasicExternalities, TestExternalities}; +pub use substrate_state_machine::{ + Externalities, + BasicExternalities, + TestExternalities, + ChildStorageKey +}; use environmental::environmental; use primitives::{hexdisplay::HexDisplay, H256}; @@ -41,6 +46,18 @@ pub type StorageOverlay = HashMap, Vec>; /// A set of key value pairs for children storage; pub type ChildrenStorageOverlay = HashMap, StorageOverlay>; +/// Returns a `ChildStorageKey` if the given `storage_key` slice is a valid storage +/// key or panics otherwise. +/// +/// Panicking here is aligned with what the `without_std` environment would do +/// in the case of an invalid child storage key. +fn child_storage_key_or_panic(storage_key: &[u8]) -> ChildStorageKey { + match ChildStorageKey::from_slice(storage_key) { + Some(storage_key) => storage_key, + None => panic!("child storage key is invalid"), + } +} + /// Get `key` from storage and return a `Vec`, empty if there's a problem. pub fn storage(key: &[u8]) -> Option> { ext::with(|ext| ext.storage(key).map(|s| s.to_vec())) @@ -49,8 +66,11 @@ pub fn storage(key: &[u8]) -> Option> { /// Get `key` from child storage and return a `Vec`, empty if there's a problem. pub fn child_storage(storage_key: &[u8], key: &[u8]) -> Option> { - ext::with(|ext| ext.child_storage(storage_key, key).map(|s| s.to_vec())) - .expect("storage cannot be called outside of an Externalities-provided environment.") + ext::with(|ext| { + let storage_key = child_storage_key_or_panic(storage_key); + ext.child_storage(storage_key, key).map(|s| s.to_vec()) + }) + .expect("storage cannot be called outside of an Externalities-provided environment.") } /// Get `key` from storage, placing the value into `value_out` (as much of it as possible) and return @@ -70,13 +90,23 @@ pub fn read_storage(key: &[u8], value_out: &mut [u8], value_offset: usize) -> Op /// the number of bytes that the entry in storage had beyond the offset or None if the storage entry /// doesn't exist at all. Note that if the buffer is smaller than the storage entry length, the returned /// number of bytes is not equal to the number of bytes written to the `value_out`. -pub fn read_child_storage(storage_key: &[u8], key: &[u8], value_out: &mut [u8], value_offset: usize) -> Option { - ext::with(|ext| ext.child_storage(storage_key, key).map(|value| { - let value = &value[value_offset..]; - let written = ::std::cmp::min(value.len(), value_out.len()); - value_out[..written].copy_from_slice(&value[..written]); - value.len() - })).expect("read_storage cannot be called outside of an Externalities-provided environment.") +pub fn read_child_storage( + storage_key: &[u8], + key: &[u8], + value_out: &mut [u8], + value_offset: usize, +) -> Option { + ext::with(|ext| { + let storage_key = child_storage_key_or_panic(storage_key); + ext.child_storage(storage_key, key) + .map(|value| { + let value = &value[value_offset..]; + let written = ::std::cmp::min(value.len(), value_out.len()); + value_out[..written].copy_from_slice(&value[..written]); + value.len() + }) + }) + .expect("read_child_storage cannot be called outside of an Externalities-provided environment.") } /// Set the storage of a key to some value. @@ -88,9 +118,10 @@ pub fn set_storage(key: &[u8], value: &[u8]) { /// Set the child storage of a key to some value. pub fn set_child_storage(storage_key: &[u8], key: &[u8], value: &[u8]) { - ext::with(|ext| - ext.set_child_storage(storage_key.to_vec(), key.to_vec(), value.to_vec()) - ); + ext::with(|ext| { + let storage_key = child_storage_key_or_panic(storage_key); + ext.set_child_storage(storage_key, key.to_vec(), value.to_vec()) + }); } /// Clear the storage of a key. @@ -102,9 +133,10 @@ pub fn clear_storage(key: &[u8]) { /// Clear the storage of a key. pub fn clear_child_storage(storage_key: &[u8], key: &[u8]) { - ext::with(|ext| + ext::with(|ext| { + let storage_key = child_storage_key_or_panic(storage_key); ext.clear_child_storage(storage_key, key) - ); + }); } /// Check whether a given `key` exists in storage. @@ -116,9 +148,10 @@ pub fn exists_storage(key: &[u8]) -> bool { /// Check whether a given `key` exists in storage. pub fn exists_child_storage(storage_key: &[u8], key: &[u8]) -> bool { - ext::with(|ext| + ext::with(|ext| { + let storage_key = child_storage_key_or_panic(storage_key); ext.exists_child_storage(storage_key, key) - ).unwrap_or(false) + }).unwrap_or(false) } /// Clear the storage entries with a key that starts with the given prefix. @@ -130,9 +163,10 @@ pub fn clear_prefix(prefix: &[u8]) { /// Clear an entire child storage. pub fn kill_child_storage(storage_key: &[u8]) { - ext::with(|ext| + ext::with(|ext| { + let storage_key = child_storage_key_or_panic(storage_key); ext.kill_child_storage(storage_key) - ); + }); } /// The current relay chain identifier. @@ -150,10 +184,11 @@ pub fn storage_root() -> H256 { } /// "Commit" all existing operations and compute the resultant child storage root. -pub fn child_storage_root(storage_key: &[u8]) -> Option> { - ext::with(|ext| +pub fn child_storage_root(storage_key: &[u8]) -> Vec { + ext::with(|ext| { + let storage_key = child_storage_key_or_panic(storage_key); ext.child_storage_root(storage_key) - ).unwrap_or(None) + }).expect("child_storage_root cannot be called outside of an Externalities-provided environment.") } /// "Commit" all existing operations and get the resultant storage change root. diff --git a/substrate/core/sr-io/without_std.rs b/substrate/core/sr-io/without_std.rs index 08b1a3c70a..f4b3c91154 100644 --- a/substrate/core/sr-io/without_std.rs +++ b/substrate/core/sr-io/without_std.rs @@ -475,18 +475,18 @@ pub fn storage_root() -> [u8; 32] { } /// "Commit" all existing operations and compute the resultant child storage root. -pub fn child_storage_root(storage_key: &[u8]) -> Option> { +pub fn child_storage_root(storage_key: &[u8]) -> Vec { let mut length: u32 = 0; unsafe { - let ptr = ext_child_storage_root.get()(storage_key.as_ptr(), storage_key.len() as u32, &mut length); - if length == u32::max_value() { - None - } else { - // Invariants required by Vec::from_raw_parts are not formally fulfilled. - // We don't allocate via String/Vec, but use a custom allocator instead. - // See #300 for more details. - Some(>::from_raw_parts(ptr, length as usize, length as usize)) - } + let ptr = ext_child_storage_root.get()( + storage_key.as_ptr(), + storage_key.len() as u32, + &mut length + ); + // Invariants required by Vec::from_raw_parts are not formally fulfilled. + // We don't allocate via String/Vec, but use a custom allocator instead. + // See #300 for more details. + >::from_raw_parts(ptr, length as usize, length as usize) } } diff --git a/substrate/core/state-machine/src/basic.rs b/substrate/core/state-machine/src/basic.rs index 5feab02835..0c95de61cb 100644 --- a/substrate/core/state-machine/src/basic.rs +++ b/substrate/core/state-machine/src/basic.rs @@ -23,7 +23,7 @@ use heapsize::HeapSizeOf; use trie::trie_root; use primitives::storage::well_known_keys::{CHANGES_TRIE_CONFIG, CODE, HEAP_PAGES}; use parity_codec::Encode; -use super::{Externalities, OverlayedChanges}; +use super::{ChildStorageKey, Externalities, OverlayedChanges}; use log::warn; /// Simple HashMap-based Externalities impl. @@ -115,7 +115,7 @@ impl Externalities for BasicExternalities where H::Out: Ord + Heap Externalities::::storage(self, key) } - fn child_storage(&self, _storage_key: &[u8], _key: &[u8]) -> Option> { + fn child_storage(&self, _storage_key: ChildStorageKey, _key: &[u8]) -> Option> { None } @@ -132,11 +132,10 @@ impl Externalities for BasicExternalities where H::Out: Ord + Heap } } - fn place_child_storage(&mut self, _storage_key: Vec, _key: Vec, _value: Option>) -> bool { - false + fn place_child_storage(&mut self, _storage_key: ChildStorageKey, _key: Vec, _value: Option>) { } - fn kill_child_storage(&mut self, _storage_key: &[u8]) { } + fn kill_child_storage(&mut self, _storage_key: ChildStorageKey) { } fn clear_prefix(&mut self, prefix: &[u8]) { self.changes.clear_prefix(prefix); @@ -149,8 +148,8 @@ impl Externalities for BasicExternalities where H::Out: Ord + Heap trie_root::(self.inner.clone()) } - fn child_storage_root(&mut self, _storage_key: &[u8]) -> Option> { - None + fn child_storage_root(&mut self, _storage_key: ChildStorageKey) -> Vec { + vec![42] } fn storage_changes_root(&mut self, _parent: H::Out, _parent_num: u64) -> Option { diff --git a/substrate/core/state-machine/src/ext.rs b/substrate/core/state-machine/src/ext.rs index a9411f26ad..c88798f37f 100644 --- a/substrate/core/state-machine/src/ext.rs +++ b/substrate/core/state-machine/src/ext.rs @@ -20,10 +20,10 @@ use std::{error, fmt, cmp::Ord}; use log::warn; use crate::backend::{Backend, Consolidate}; use crate::changes_trie::{AnchorBlockId, Storage as ChangesTrieStorage, compute_changes_trie_root}; -use crate::{Externalities, OverlayedChanges, OffchainExt}; +use crate::{Externalities, OverlayedChanges, OffchainExt, ChildStorageKey}; use hash_db::Hasher; use primitives::storage::well_known_keys::is_child_storage_key; -use trie::{MemoryDB, TrieDBMut, TrieMut, default_child_trie_root, is_child_trie_key_valid}; +use trie::{MemoryDB, TrieDBMut, TrieMut, default_child_trie_root}; use heapsize::HeapSizeOf; const EXT_NOT_ALLOWED_TO_FAIL: &str = "Externalities not allowed to fail within runtime"; @@ -214,10 +214,10 @@ where self.backend.storage_hash(key).expect(EXT_NOT_ALLOWED_TO_FAIL) } - fn child_storage(&self, storage_key: &[u8], key: &[u8]) -> Option> { + fn child_storage(&self, storage_key: ChildStorageKey, key: &[u8]) -> Option> { let _guard = panic_handler::AbortGuard::new(true); - self.overlay.child_storage(storage_key, key).map(|x| x.map(|x| x.to_vec())).unwrap_or_else(|| - self.backend.child_storage(storage_key, key).expect(EXT_NOT_ALLOWED_TO_FAIL)) + self.overlay.child_storage(storage_key.as_ref(), key).map(|x| x.map(|x| x.to_vec())).unwrap_or_else(|| + self.backend.child_storage(storage_key.as_ref(), key).expect(EXT_NOT_ALLOWED_TO_FAIL)) } fn exists_storage(&self, key: &[u8]) -> bool { @@ -228,11 +228,12 @@ where } } - fn exists_child_storage(&self, storage_key: &[u8], key: &[u8]) -> bool { + fn exists_child_storage(&self, storage_key: ChildStorageKey, key: &[u8]) -> bool { let _guard = panic_handler::AbortGuard::new(true); - match self.overlay.child_storage(storage_key, key) { + + match self.overlay.child_storage(storage_key.as_ref(), key) { Some(x) => x.is_some(), - _ => self.backend.exists_child_storage(storage_key, key).expect(EXT_NOT_ALLOWED_TO_FAIL), + _ => self.backend.exists_child_storage(storage_key.as_ref(), key).expect(EXT_NOT_ALLOWED_TO_FAIL), } } @@ -247,28 +248,20 @@ where self.overlay.set_storage(key, value); } - fn place_child_storage(&mut self, storage_key: Vec, key: Vec, value: Option>) -> bool { + fn place_child_storage(&mut self, storage_key: ChildStorageKey, key: Vec, value: Option>) { let _guard = panic_handler::AbortGuard::new(true); - if !is_child_storage_key(&storage_key) || !is_child_trie_key_valid::(&storage_key) { - return false; - } self.mark_dirty(); - self.overlay.set_child_storage(storage_key, key, value); - - true + self.overlay.set_child_storage(storage_key.into_owned(), key, value); } - fn kill_child_storage(&mut self, storage_key: &[u8]) { + fn kill_child_storage(&mut self, storage_key: ChildStorageKey) { let _guard = panic_handler::AbortGuard::new(true); - if !is_child_storage_key(storage_key) || !is_child_trie_key_valid::(storage_key) { - return; - } self.mark_dirty(); - self.overlay.clear_child_storage(storage_key); - self.backend.for_keys_in_child_storage(storage_key, |key| { - self.overlay.set_child_storage(storage_key.to_vec(), key.to_vec(), None); + self.overlay.clear_child_storage(storage_key.as_ref()); + self.backend.for_keys_in_child_storage(storage_key.as_ref(), |key| { + self.overlay.set_child_storage(storage_key.as_ref().to_vec(), key.to_vec(), None); }); } @@ -315,17 +308,17 @@ where root } - fn child_storage_root(&mut self, storage_key: &[u8]) -> Option> { + fn child_storage_root(&mut self, storage_key: ChildStorageKey) -> Vec { let _guard = panic_handler::AbortGuard::new(true); - if !is_child_storage_key(storage_key) || !is_child_trie_key_valid::(storage_key) { - return None; - } - if self.storage_transaction.is_some() { - return Some(self.storage(storage_key).unwrap_or(default_child_trie_root::(storage_key))); + self + .storage(storage_key.as_ref()) + .unwrap_or( + default_child_trie_root::(storage_key.as_ref()) + ) + } else { + self.child_storage_root_transaction(storage_key.as_ref()).0 } - - Some(self.child_storage_root_transaction(storage_key).0) } fn storage_changes_root(&mut self, parent: H::Out, parent_num: u64) -> Option { diff --git a/substrate/core/state-machine/src/lib.rs b/substrate/core/state-machine/src/lib.rs index edddc37126..bd9d9284e5 100644 --- a/substrate/core/state-machine/src/lib.rs +++ b/substrate/core/state-machine/src/lib.rs @@ -19,6 +19,7 @@ #![warn(missing_docs)] use std::{fmt, panic::UnwindSafe, result, marker::PhantomData}; +use std::borrow::Cow; use log::warn; use hash_db::Hasher; use heapsize::HeapSizeOf; @@ -55,6 +56,58 @@ pub use proving_backend::{create_proof_check_backend, create_proof_check_backend pub use trie_backend_essence::{TrieBackendStorage, Storage}; pub use trie_backend::TrieBackend; +/// A wrapper around a child storage key. +/// +/// This wrapper ensures that the child storage key is correct and properly used. It is +/// impossible to create an instance of this struct without providing a correct `storage_key`. +pub struct ChildStorageKey<'a, H: Hasher> { + storage_key: Cow<'a, [u8]>, + _hasher: PhantomData, +} + +impl<'a, H: Hasher> ChildStorageKey<'a, H> { + fn new(storage_key: Cow<'a, [u8]>) -> Option { + if !trie::is_child_trie_key_valid::(&storage_key) { + return None; + } + + Some(ChildStorageKey { + storage_key, + _hasher: PhantomData, + }) + } + + /// Create a new `ChildStorageKey` from a vector. + /// + /// `storage_key` has should start with `:child_storage:default:` + /// See `is_child_trie_key_valid` for more details. + pub fn from_vec(key: Vec) -> Option { + Self::new(Cow::Owned(key)) + } + + /// Create a new `ChildStorageKey` from a slice. + /// + /// `storage_key` has should start with `:child_storage:default:` + /// See `is_child_trie_key_valid` for more details. + pub fn from_slice(key: &'a [u8]) -> Option { + Self::new(Cow::Borrowed(key)) + } + + /// Get access to the byte representation of the storage key. + /// + /// This key is guaranteed to be correct. + pub fn as_ref(&self) -> &[u8] { + &*self.storage_key + } + + /// Destruct this instance into an owned vector that represents the storage key. + /// + /// This key is guaranteed to be correct. + pub fn into_owned(self) -> Vec { + self.storage_key.into_owned() + } +} + /// State Machine Error bound. /// /// This should reflect WASM error type bound for future compatibility. @@ -105,7 +158,7 @@ pub trait Externalities { } /// Read child runtime storage. - fn child_storage(&self, storage_key: &[u8], key: &[u8]) -> Option>; + fn child_storage(&self, storage_key: ChildStorageKey, key: &[u8]) -> Option>; /// Set storage entry `key` of current contract being called (effective immediately). fn set_storage(&mut self, key: Vec, value: Vec) { @@ -113,7 +166,7 @@ pub trait Externalities { } /// Set child storage entry `key` of current contract being called (effective immediately). - fn set_child_storage(&mut self, storage_key: Vec, key: Vec, value: Vec) -> bool { + fn set_child_storage(&mut self, storage_key: ChildStorageKey, key: Vec, value: Vec) { self.place_child_storage(storage_key, key, Some(value)) } @@ -123,8 +176,8 @@ pub trait Externalities { } /// Clear a child storage entry (`key`) of current contract being called (effective immediately). - fn clear_child_storage(&mut self, storage_key: &[u8], key: &[u8]) -> bool { - self.place_child_storage(storage_key.to_vec(), key.to_vec(), None) + fn clear_child_storage(&mut self, storage_key: ChildStorageKey, key: &[u8]) { + self.place_child_storage(storage_key, key.to_vec(), None) } /// Whether a storage entry exists. @@ -133,12 +186,12 @@ pub trait Externalities { } /// Whether a child storage entry exists. - fn exists_child_storage(&self, storage_key: &[u8], key: &[u8]) -> bool { + fn exists_child_storage(&self, storage_key: ChildStorageKey, key: &[u8]) -> bool { self.child_storage(storage_key, key).is_some() } /// Clear an entire child storage. - fn kill_child_storage(&mut self, storage_key: &[u8]); + fn kill_child_storage(&mut self, storage_key: ChildStorageKey); /// Clear storage entries which keys are start with the given prefix. fn clear_prefix(&mut self, prefix: &[u8]); @@ -147,7 +200,7 @@ pub trait Externalities { fn place_storage(&mut self, key: Vec, value: Option>); /// Set or clear a child storage entry. Return whether the operation succeeds. - fn place_child_storage(&mut self, storage_key: Vec, key: Vec, value: Option>) -> bool; + fn place_child_storage(&mut self, storage_key: ChildStorageKey, key: Vec, value: Option>); /// Get the identity of the chain. fn chain_id(&self) -> u64; @@ -155,10 +208,11 @@ pub trait Externalities { /// 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 storage keys in the top-level storage map. If the storage root equals default hash as defined by trie, the key in top-level storage map will be removed. - /// - /// Returns None if key provided is not a storage key. This can due to not being started with CHILD_STORAGE_KEY_PREFIX, or the trie implementation regards the key as invalid. - fn child_storage_root(&mut self, storage_key: &[u8]) -> Option>; + /// Get the trie root of a child storage map. This will also update the value of the child + /// storage keys in the top-level storage map. + /// If the storage root equals the default hash as defined by the trie, the key in the top-level + /// storage map will be removed. + fn child_storage_root(&mut self, storage_key: ChildStorageKey) -> Vec; /// 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, parent_num: u64) -> Option where H::Out: Ord; @@ -905,12 +959,35 @@ mod tests { let backend = InMemory::::default().try_into_trie_backend().unwrap(); let changes_trie_storage = InMemoryChangesTrieStorage::new(); let mut overlay = OverlayedChanges::default(); - let mut ext = Ext::new(&mut overlay, &backend, Some(&changes_trie_storage), NeverOffchainExt::new()); + let mut ext = Ext::new( + &mut overlay, + &backend, + Some(&changes_trie_storage), + NeverOffchainExt::new() + ); - assert!(ext.set_child_storage(b":child_storage:testchild".to_vec(), b"abc".to_vec(), b"def".to_vec())); - assert_eq!(ext.child_storage(b":child_storage:testchild", b"abc"), Some(b"def".to_vec())); - ext.kill_child_storage(b":child_storage:testchild"); - assert_eq!(ext.child_storage(b":child_storage:testchild", b"abc"), None); + ext.set_child_storage( + ChildStorageKey::from_slice(b":child_storage:default:testchild").unwrap(), + b"abc".to_vec(), + b"def".to_vec() + ); + assert_eq!( + ext.child_storage( + ChildStorageKey::from_slice(b":child_storage:default:testchild").unwrap(), + b"abc" + ), + Some(b"def".to_vec()) + ); + ext.kill_child_storage( + ChildStorageKey::from_slice(b":child_storage:default:testchild").unwrap() + ); + assert_eq!( + ext.child_storage( + ChildStorageKey::from_slice(b":child_storage:default:testchild").unwrap(), + b"abc" + ), + None + ); } #[test] diff --git a/substrate/core/state-machine/src/testing.rs b/substrate/core/state-machine/src/testing.rs index 6bbfc27667..ac096c0c3e 100644 --- a/substrate/core/state-machine/src/testing.rs +++ b/substrate/core/state-machine/src/testing.rs @@ -25,7 +25,7 @@ use crate::backend::InMemory; use crate::changes_trie::{compute_changes_trie_root, InMemoryStorage as ChangesTrieInMemoryStorage, AnchorBlockId}; use primitives::storage::well_known_keys::{CHANGES_TRIE_CONFIG, CODE, HEAP_PAGES}; use parity_codec::Encode; -use super::{Externalities, OverlayedChanges}; +use super::{ChildStorageKey, Externalities, OverlayedChanges}; /// Simple HashMap-based Externalities impl. pub struct TestExternalities where H::Out: HeapSizeOf { @@ -122,8 +122,8 @@ impl Externalities for TestExternalities where H::Out: Ord + He self.storage(key) } - fn child_storage(&self, storage_key: &[u8], key: &[u8]) -> Option> { - self.changes.child_storage(storage_key, key)?.map(Vec::from) + fn child_storage(&self, storage_key: ChildStorageKey, key: &[u8]) -> Option> { + self.changes.child_storage(storage_key.as_ref(), key)?.map(Vec::from) } fn place_storage(&mut self, key: Vec, maybe_value: Option>) { @@ -139,14 +139,12 @@ impl Externalities for TestExternalities where H::Out: Ord + He } } - fn place_child_storage(&mut self, storage_key: Vec, key: Vec, value: Option>) -> bool { - self.changes.set_child_storage(storage_key, key, value); - // TODO place_child_storage and set_child_storage should always be valid (create child on set)? - true + fn place_child_storage(&mut self, storage_key: ChildStorageKey, key: Vec, value: Option>) { + self.changes.set_child_storage(storage_key.into_owned(), key, value); } - fn kill_child_storage(&mut self, storage_key: &[u8]) { - self.changes.clear_child_storage(storage_key); + fn kill_child_storage(&mut self, storage_key: ChildStorageKey) { + self.changes.clear_child_storage(storage_key.as_ref()); } fn clear_prefix(&mut self, prefix: &[u8]) { @@ -160,8 +158,8 @@ impl Externalities for TestExternalities where H::Out: Ord + He trie_root::(self.inner.clone()) } - fn child_storage_root(&mut self, _storage_key: &[u8]) -> Option> { - None + fn child_storage_root(&mut self, _storage_key: ChildStorageKey) -> Vec { + unimplemented!() } fn storage_changes_root(&mut self, parent: H::Out, parent_num: u64) -> Option { diff --git a/substrate/core/trie/Cargo.toml b/substrate/core/trie/Cargo.toml index d535c29701..1fd1b7b3bd 100644 --- a/substrate/core/trie/Cargo.toml +++ b/substrate/core/trie/Cargo.toml @@ -18,9 +18,9 @@ hash-db = { version = "0.12.2", default-features = false } trie-db = { version = "0.12.2", default-features = false } trie-root = { version = "0.12.2", default-features = false } memory-db = { version = "0.12.2", default-features = false } +substrate-primitives = { path = "../primitives", default-features = false } [dev-dependencies] -substrate-primitives = { path = "../primitives" } trie-bench = { version = "0.12" } trie-standardmap = { version = "0.12" } keccak-hasher = { version = "0.12" } @@ -31,9 +31,10 @@ hex-literal = "0.1.0" default = ["std"] std = [ "rstd/std", - "codec/std", + "codec/std", "hash-db/std", "memory-db/std", "trie-db/std", - "trie-root/std" + "trie-root/std", + "substrate-primitives/std", ] diff --git a/substrate/core/trie/src/lib.rs b/substrate/core/trie/src/lib.rs index f40ae81491..a9a9860f94 100644 --- a/substrate/core/trie/src/lib.rs +++ b/substrate/core/trie/src/lib.rs @@ -137,9 +137,23 @@ where ) } -/// Determine whether a child trie key is valid. `child_trie_root` and `child_delta_trie_root` can panic if invalid value is provided to them. -pub fn is_child_trie_key_valid(_storage_key: &[u8]) -> bool { - true +/// Determine whether a child trie key is valid. +/// +/// For now, the only valid child trie key is `:child_storage:default:`. +/// +/// `child_trie_root` and `child_delta_trie_root` can panic if invalid value is provided to them. +pub fn is_child_trie_key_valid(storage_key: &[u8]) -> bool { + use substrate_primitives::storage::well_known_keys; + let has_right_prefix = storage_key.starts_with(b":child_storage:default:"); + if has_right_prefix { + // This is an attempt to catch a change of `is_child_storage_key`, which + // just checks if the key has prefix `:child_storage:` at the moment of writing. + debug_assert!( + well_known_keys::is_child_storage_key(&storage_key), + "`is_child_trie_key_valid` is a subset of `is_child_storage_key`", + ); + } + has_right_prefix } /// Determine the default child trie root. diff --git a/substrate/srml/contract/src/lib.rs b/substrate/srml/contract/src/lib.rs index 42bdf3240c..5b52a31150 100644 --- a/substrate/srml/contract/src/lib.rs +++ b/substrate/srml/contract/src/lib.rs @@ -133,8 +133,13 @@ pub struct AccountInfo { pub trait TrieIdGenerator { /// Get a trie id for an account, using reference to parent account trie id to ensure /// uniqueness of trie id. + /// /// The implementation must ensure every new trie id is unique: two consecutive calls with the /// same parameter needs to return different trie id values. + /// + /// Also, the implementation is responsible for ensuring that `TrieId` starts with + /// `:child_storage:`. + /// TODO: We want to change this, see https://github.com/paritytech/substrate/issues/2325 fn trie_id(account_id: &AccountId) -> TrieId; } @@ -156,7 +161,9 @@ where buf.extend_from_slice(account_id.as_ref()); buf.extend_from_slice(&new_seed.to_le_bytes()[..]); + // TODO: see https://github.com/paritytech/substrate/issues/2325 CHILD_STORAGE_KEY_PREFIX.iter() + .chain(b"default:") .chain(T::Hashing::hash(&buf[..]).as_ref().iter()) .cloned() .collect() diff --git a/substrate/srml/contract/src/tests.rs b/substrate/srml/contract/src/tests.rs index 7207835925..d6fa2677df 100644 --- a/substrate/srml/contract/src/tests.rs +++ b/substrate/srml/contract/src/tests.rs @@ -123,7 +123,13 @@ static KEY_COUNTER: AtomicUsize = AtomicUsize::new(0); pub struct DummyTrieIdGenerator; impl TrieIdGenerator for DummyTrieIdGenerator { fn trie_id(account_id: &u64) -> TrieId { - let mut res = KEY_COUNTER.fetch_add(1, Ordering::Relaxed).to_le_bytes().to_vec(); + use substrate_primitives::storage::well_known_keys; + + // TODO: see https://github.com/paritytech/substrate/issues/2325 + let mut res = vec![]; + res.extend_from_slice(well_known_keys::CHILD_STORAGE_KEY_PREFIX); + res.extend_from_slice(b"default:"); + res.extend_from_slice(&KEY_COUNTER.fetch_add(1, Ordering::Relaxed).to_le_bytes()); res.extend_from_slice(&account_id.to_le_bytes()); res }