From b21d76db6edd4540ee9bd5ed2cbaef2b395a21f8 Mon Sep 17 00:00:00 2001 From: Sergey Pepyakin Date: Tue, 10 Sep 2019 21:31:46 +0200 Subject: [PATCH 1/9] Add a failing test case --- core/test-runtime/src/lib.rs | 41 ++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/core/test-runtime/src/lib.rs b/core/test-runtime/src/lib.rs index aa0b57ec9285f..dd547addea34f 100644 --- a/core/test-runtime/src/lib.rs +++ b/core/test-runtime/src/lib.rs @@ -280,6 +280,8 @@ cfg_if! { /// /// Returns the signature generated for the message `sr25519`. fn test_sr25519_crypto() -> (sr25519::AppSignature, sr25519::AppPublic); + /// Run various tests against storage. + fn test_storage(); } } } else { @@ -322,6 +324,8 @@ cfg_if! { /// /// Returns the signature generated for the message `sr25519`. fn test_sr25519_crypto() -> (sr25519::AppSignature, sr25519::AppPublic); + /// Run various tests against storage. + fn test_storage(); } } } @@ -590,6 +594,10 @@ cfg_if! { fn test_sr25519_crypto() -> (sr25519::AppSignature, sr25519::AppPublic) { test_sr25519_crypto() } + + fn test_storage() { + test_storage(); + } } impl aura_primitives::AuraApi for Runtime { @@ -805,6 +813,10 @@ cfg_if! { fn test_sr25519_crypto() -> (sr25519::AppSignature, sr25519::AppPublic) { test_sr25519_crypto() } + + fn test_storage() { + test_storage(); + } } impl aura_primitives::AuraApi for Runtime { @@ -887,6 +899,26 @@ fn test_sr25519_crypto() -> (sr25519::AppSignature, sr25519::AppPublic) { (signature, public0) } +fn test_storage() { + const KEY: &[u8] = b":test_storage"; + + runtime_io::set_storage(KEY, b"test"); + + let mut v = [0u8; 4]; + let r = runtime_io::read_storage( + KEY, + &mut v, + 0 + ); + assert_eq!(r, Some(4)); + assert_eq!(&v, b"test"); + + let mut v = [0u8; 4]; + let r = runtime_io::read_storage(KEY, &mut v, 8); + assert_eq!(r, Some(4)); + assert_eq!(&v, &[0, 0, 0, 0]); +} + #[cfg(test)] mod tests { use substrate_test_runtime_client::{ @@ -981,4 +1013,13 @@ mod tests { let ret = runtime_api.vec_with_capacity(&new_block_id, 1048576); assert!(ret.is_ok()); } + + #[test] + fn read_storage() { + let client = TestClientBuilder::new().build(); + let runtime_api = client.runtime_api(); + let block_id = BlockId::Number(client.info().chain.best_number); + + runtime_api.test_storage(&block_id).unwrap(); + } } From cb25909c8fc95e81dd7eb7fc2cc631cbec44da93 Mon Sep 17 00:00:00 2001 From: Sergey Pepyakin Date: Wed, 11 Sep 2019 12:22:52 +0200 Subject: [PATCH 2/9] Actual fix --- core/sr-io/with_std.rs | 16 ++++++++++------ core/test-runtime/src/lib.rs | 32 +++++++++++++++++++++++++++----- 2 files changed, 37 insertions(+), 11 deletions(-) diff --git a/core/sr-io/with_std.rs b/core/sr-io/with_std.rs index ca2a2554c3ca0..ff377b3358df0 100644 --- a/core/sr-io/with_std.rs +++ b/core/sr-io/with_std.rs @@ -54,9 +54,11 @@ impl StorageApi for () { fn read_storage(key: &[u8], value_out: &mut [u8], value_offset: usize) -> Option { ext::with(|ext| ext.storage(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]); + if value_offset < value.len() { + 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.") } @@ -85,9 +87,11 @@ impl StorageApi for () { 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]); + if value_offset < value.len() { + 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() }) }) diff --git a/core/test-runtime/src/lib.rs b/core/test-runtime/src/lib.rs index dd547addea34f..b3452f55f9eec 100644 --- a/core/test-runtime/src/lib.rs +++ b/core/test-runtime/src/lib.rs @@ -596,7 +596,8 @@ cfg_if! { } fn test_storage() { - test_storage(); + test_read_storage(); + test_read_child_storage(); } } @@ -815,7 +816,8 @@ cfg_if! { } fn test_storage() { - test_storage(); + test_read_storage(); + test_read_child_storage(); } } @@ -899,9 +901,8 @@ fn test_sr25519_crypto() -> (sr25519::AppSignature, sr25519::AppPublic) { (signature, public0) } -fn test_storage() { +fn test_read_storage() { const KEY: &[u8] = b":test_storage"; - runtime_io::set_storage(KEY, b"test"); let mut v = [0u8; 4]; @@ -919,6 +920,27 @@ fn test_storage() { assert_eq!(&v, &[0, 0, 0, 0]); } +fn test_read_child_storage() { + const CHILD_KEY: &[u8] = b":default:mock"; + const KEY: &[u8] = b":test_storage"; + runtime_io::set_child_storage(CHILD_KEY, KEY, b"test"); + + let mut v = [0u8; 4]; + let r = runtime_io::read_child_storage( + CHILD_KEY, + KEY, + &mut v, + 0 + ); + assert_eq!(r, Some(4)); + assert_eq!(&v, b"test"); + + let mut v = [0u8; 4]; + let r = runtime_io::read_child_storage(CHILD_KEY, KEY, &mut v, 8); + assert_eq!(r, Some(4)); + assert_eq!(&v, &[0, 0, 0, 0]); +} + #[cfg(test)] mod tests { use substrate_test_runtime_client::{ @@ -1015,7 +1037,7 @@ mod tests { } #[test] - fn read_storage() { + fn test_storage() { let client = TestClientBuilder::new().build(); let runtime_api = client.runtime_api(); let block_id = BlockId::Number(client.info().chain.best_number); From 7682ed562b6b0c07f7a26d8bb4d37e1c3e231e9c Mon Sep 17 00:00:00 2001 From: Sergey Pepyakin Date: Wed, 11 Sep 2019 13:23:50 +0200 Subject: [PATCH 3/9] read_child_storage, fix wasm side --- core/executor/src/wasm_executor.rs | 24 ++++++++++++++---------- core/test-runtime/src/lib.rs | 10 ++++++---- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/core/executor/src/wasm_executor.rs b/core/executor/src/wasm_executor.rs index 4baa547ab8723..47d9610799c07 100644 --- a/core/executor/src/wasm_executor.rs +++ b/core/executor/src/wasm_executor.rs @@ -31,8 +31,8 @@ use crate::error::{Error, Result}; use codec::{Encode, Decode}; use primitives::{ blake2_128, blake2_256, twox_64, twox_128, twox_256, ed25519, sr25519, Pair, crypto::KeyTypeId, - offchain, hexdisplay::HexDisplay, sandbox as sandbox_primitives, H256, Blake2Hasher, - traits::Externalities, child_storage_key::ChildStorageKey, + offchain, hexdisplay::HexDisplay, sandbox as sandbox_primitives, Blake2Hasher, + traits::Externalities, }; use trie::{TrieConfiguration, trie_types::Layout}; use crate::sandbox; @@ -605,10 +605,12 @@ impl_wasm_host_interface! { let maybe_value = runtime_io::storage(&key); if let Some(value) = maybe_value { - let value = &value[value_offset as usize..]; - let written = std::cmp::min(value_len as usize, value.len()); - context.write_memory(value_data, &value[..written]) - .map_err(|_| "Invalid attempt to set value in ext_get_storage_into")?; + if (value_offset as usize) < value.len() { + let value = &value[value_offset as usize..]; + let written = std::cmp::min(value_len as usize, value.len()); + context.write_memory(value_data, &value[..written]) + .map_err(|_| "Invalid attempt to set value in ext_get_storage_into")?; + } Ok(value.len() as u32) } else { Ok(u32::max_value()) @@ -632,10 +634,12 @@ impl_wasm_host_interface! { let maybe_value = runtime_io::child_storage(&storage_key, &key); if let Some(value) = maybe_value { - let value = &value[value_offset as usize..]; - let written = std::cmp::min(value_len as usize, value.len()); - context.write_memory(value_data, &value[..written]) - .map_err(|_| "Invalid attempt to set value in ext_get_child_storage_into")?; + if (value_offset as usize) < value.len() { + let value = &value[value_offset as usize..]; + let written = std::cmp::min(value_len as usize, value.len()); + context.write_memory(value_data, &value[..written]) + .map_err(|_| "Invalid attempt to set value in ext_get_child_storage_into")?; + } Ok(value.len() as u32) } else { Ok(u32::max_value()) diff --git a/core/test-runtime/src/lib.rs b/core/test-runtime/src/lib.rs index b3452f55f9eec..88a246b094741 100644 --- a/core/test-runtime/src/lib.rs +++ b/core/test-runtime/src/lib.rs @@ -902,7 +902,7 @@ fn test_sr25519_crypto() -> (sr25519::AppSignature, sr25519::AppPublic) { } fn test_read_storage() { - const KEY: &[u8] = b":test_storage"; + const KEY: &[u8] = b":read_storage"; runtime_io::set_storage(KEY, b"test"); let mut v = [0u8; 4]; @@ -921,8 +921,8 @@ fn test_read_storage() { } fn test_read_child_storage() { - const CHILD_KEY: &[u8] = b":default:mock"; - const KEY: &[u8] = b":test_storage"; + const CHILD_KEY: &[u8] = b":child_storage:default:read_child_storage"; + const KEY: &[u8] = b":read_child_storage"; runtime_io::set_child_storage(CHILD_KEY, KEY, b"test"); let mut v = [0u8; 4]; @@ -1038,7 +1038,9 @@ mod tests { #[test] fn test_storage() { - let client = TestClientBuilder::new().build(); + let client = TestClientBuilder::new() + .set_execution_strategy(ExecutionStrategy::Both) + .build(); let runtime_api = client.runtime_api(); let block_id = BlockId::Number(client.info().chain.best_number); From e2df78789d1f41d41a7766377d2fa6a05a875119 Mon Sep 17 00:00:00 2001 From: Sergey Pepyakin Date: Wed, 11 Sep 2019 13:38:35 +0200 Subject: [PATCH 4/9] Bump the impl 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 9d7ae5d9df85d..16a4b551cb886 100644 --- a/node/runtime/src/lib.rs +++ b/node/runtime/src/lib.rs @@ -83,7 +83,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { // implementation changes and behavior does not, then leave spec_version as // is and increment impl_version. spec_version: 155, - impl_version: 155, + impl_version: 156, apis: RUNTIME_API_VERSIONS, }; From ddaff89f24c1c5af8754301f8c983ca3e6b82446 Mon Sep 17 00:00:00 2001 From: Gavin Wood Date: Wed, 11 Sep 2019 15:53:52 +0200 Subject: [PATCH 5/9] Alternative (#3597) * Update with_std.rs * Update with_std.rs --- core/sr-io/with_std.rs | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/core/sr-io/with_std.rs b/core/sr-io/with_std.rs index ff377b3358df0..45c0c9f824465 100644 --- a/core/sr-io/with_std.rs +++ b/core/sr-io/with_std.rs @@ -54,11 +54,9 @@ impl StorageApi for () { fn read_storage(key: &[u8], value_out: &mut [u8], value_offset: usize) -> Option { ext::with(|ext| ext.storage(key).map(|value| { - if value_offset < value.len() { - let value = &value[value_offset..]; - let written = std::cmp::min(value.len(), value_out.len()); - value_out[..written].copy_from_slice(&value[..written]); - } + let value = &value[value_offset.min(value.len())..]; + 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.") } @@ -87,11 +85,9 @@ impl StorageApi for () { let storage_key = child_storage_key_or_panic(storage_key); ext.child_storage(storage_key, key) .map(|value| { - if value_offset < value.len() { - let value = &value[value_offset..]; - let written = std::cmp::min(value.len(), value_out.len()); - value_out[..written].copy_from_slice(&value[..written]); - } + let value = &value[value_offset.min(value.len())..]; + let written = std::cmp::min(value.len(), value_out.len()); + value_out[..written].copy_from_slice(&value[..written]); value.len() }) }) From baa2e3b93f18fc19ff260e2ec65c2e630385b281 Mon Sep 17 00:00:00 2001 From: Gavin Wood Date: Wed, 11 Sep 2019 15:55:38 +0200 Subject: [PATCH 6/9] Update wasm_executor.rs --- core/executor/src/wasm_executor.rs | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/core/executor/src/wasm_executor.rs b/core/executor/src/wasm_executor.rs index 47d9610799c07..e61f23b284a2f 100644 --- a/core/executor/src/wasm_executor.rs +++ b/core/executor/src/wasm_executor.rs @@ -605,12 +605,10 @@ impl_wasm_host_interface! { let maybe_value = runtime_io::storage(&key); if let Some(value) = maybe_value { - if (value_offset as usize) < value.len() { - let value = &value[value_offset as usize..]; - let written = std::cmp::min(value_len as usize, value.len()); - context.write_memory(value_data, &value[..written]) - .map_err(|_| "Invalid attempt to set value in ext_get_storage_into")?; - } + let value = &value[value.len().min(value_offset as usize)..]; + let written = std::cmp::min(value_len as usize, value.len()); + context.write_memory(value_data, &value[..written]) + .map_err(|_| "Invalid attempt to set value in ext_get_storage_into")?; Ok(value.len() as u32) } else { Ok(u32::max_value()) @@ -634,12 +632,10 @@ impl_wasm_host_interface! { let maybe_value = runtime_io::child_storage(&storage_key, &key); if let Some(value) = maybe_value { - if (value_offset as usize) < value.len() { - let value = &value[value_offset as usize..]; - let written = std::cmp::min(value_len as usize, value.len()); - context.write_memory(value_data, &value[..written]) - .map_err(|_| "Invalid attempt to set value in ext_get_child_storage_into")?; - } + let value = &value[value.len().min(value_offset as usize)..]; + let written = std::cmp::min(value_len as usize, value.len()); + context.write_memory(value_data, &value[..written]) + .map_err(|_| "Invalid attempt to set value in ext_get_child_storage_into")?; Ok(value.len() as u32) } else { Ok(u32::max_value()) From a5abf0afbea90af78ff521000f3e8b99319d6e86 Mon Sep 17 00:00:00 2001 From: Gavin Wood Date: Wed, 11 Sep 2019 17:38:34 +0200 Subject: [PATCH 7/9] Update wasm_executor.rs --- core/executor/src/wasm_executor.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/core/executor/src/wasm_executor.rs b/core/executor/src/wasm_executor.rs index 317bbbe0a134a..4d83db11646ea 100644 --- a/core/executor/src/wasm_executor.rs +++ b/core/executor/src/wasm_executor.rs @@ -629,9 +629,9 @@ impl_wasm_host_interface! { )?; if let Some(value) = maybe_value { - let value = &value[value.len().min(value_offset as usize)..]; - let written = std::cmp::min(value_len as usize, value.len()); - context.write_memory(value_data, &value[..written]) + let data = &value[value.len().min(value_offset as usize)..]; + let written = std::cmp::min(value_len as usize, data.len()); + context.write_memory(value_data, &data[..written]) .map_err(|_| "Invalid attempt to set value in ext_get_storage_into")?; Ok(value.len() as u32) } else { @@ -658,11 +658,11 @@ impl_wasm_host_interface! { )?; if let Some(value) = maybe_value { - let value = &value[value.len().min(value_offset as usize)..]; - let written = std::cmp::min(value_len as usize, value.len()); - context.write_memory(value_data, &value[..written]) - .map_err(|_| "Invalid attempt to set value in ext_get_child_storage_into")?; - Ok(value.len() as u32) + let data = &value[value.len().min(value_offset as usize)..]; + let written = std::cmp::min(value_len as usize, data.len()); + context.write_memory(value_data, &data[..written]) + .map_err(|_| "Invalid attempt to get value in ext_get_child_storage_into")?; + Ok(data.len() as u32) } else { Ok(u32::max_value()) } From c18832ba1e760095acb50d0a9cfcc097609bc534 Mon Sep 17 00:00:00 2001 From: Gavin Wood Date: Wed, 11 Sep 2019 17:39:36 +0200 Subject: [PATCH 8/9] Update with_std.rs --- core/sr-io/with_std.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/core/sr-io/with_std.rs b/core/sr-io/with_std.rs index 45c0c9f824465..41baf295328c6 100644 --- a/core/sr-io/with_std.rs +++ b/core/sr-io/with_std.rs @@ -54,9 +54,9 @@ impl StorageApi for () { fn read_storage(key: &[u8], value_out: &mut [u8], value_offset: usize) -> Option { ext::with(|ext| ext.storage(key).map(|value| { - let value = &value[value_offset.min(value.len())..]; - let written = std::cmp::min(value.len(), value_out.len()); - value_out[..written].copy_from_slice(&value[..written]); + let data = &value[value_offset.min(value.len())..]; + let written = std::cmp::min(data.len(), value_out.len()); + value_out[..written].copy_from_slice(&data[..written]); value.len() })).expect("read_storage cannot be called outside of an Externalities-provided environment.") } @@ -85,9 +85,9 @@ impl StorageApi for () { let storage_key = child_storage_key_or_panic(storage_key); ext.child_storage(storage_key, key) .map(|value| { - let value = &value[value_offset.min(value.len())..]; - let written = std::cmp::min(value.len(), value_out.len()); - value_out[..written].copy_from_slice(&value[..written]); + let data = &value[value_offset.min(value.len())..]; + let written = std::cmp::min(data.len(), value_out.len()); + value_out[..written].copy_from_slice(&data[..written]); value.len() }) }) From f4962db8edc4bf37ca8315a926a6fc2e4c892abe Mon Sep 17 00:00:00 2001 From: Gavin Wood Date: Wed, 11 Sep 2019 17:40:14 +0200 Subject: [PATCH 9/9] Update wasm_executor.rs --- core/executor/src/wasm_executor.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/executor/src/wasm_executor.rs b/core/executor/src/wasm_executor.rs index 4d83db11646ea..38b8d8eff17e3 100644 --- a/core/executor/src/wasm_executor.rs +++ b/core/executor/src/wasm_executor.rs @@ -662,7 +662,7 @@ impl_wasm_host_interface! { let written = std::cmp::min(value_len as usize, data.len()); context.write_memory(value_data, &data[..written]) .map_err(|_| "Invalid attempt to get value in ext_get_child_storage_into")?; - Ok(data.len() as u32) + Ok(value.len() as u32) } else { Ok(u32::max_value()) }