From c273d0b4b0ba9198aea39f6f4096691336089e3d Mon Sep 17 00:00:00 2001 From: Robin Freyler Date: Mon, 2 Mar 2020 14:12:32 +0100 Subject: [PATCH 1/6] split out ext_clear_storage from ext_set_storage contracts API --- frame/contracts/src/wasm/runtime.rs | 55 +++++++++++++++++++---------- 1 file changed, 37 insertions(+), 18 deletions(-) diff --git a/frame/contracts/src/wasm/runtime.rs b/frame/contracts/src/wasm/runtime.rs index a84556d884ce9..a8a516af84d4d 100644 --- a/frame/contracts/src/wasm/runtime.rs +++ b/frame/contracts/src/wasm/runtime.rs @@ -348,30 +348,49 @@ define_env!(Env, , Ok(()) }, - // Change the value at the given key in the storage or remove the entry. - // The value length must not exceed the maximum defined by the Contracts module parameters. - // - // - key_ptr: pointer into the linear - // memory where the location of the requested value is placed. - // - value_non_null: if set to 0, then the entry - // at the given location will be removed. - // - value_ptr: pointer into the linear memory - // where the value to set is placed. If `value_non_null` is set to 0, then this parameter is ignored. - // - value_len: the length of the value. If `value_non_null` is set to 0, then this parameter is ignored. - ext_set_storage(ctx, key_ptr: u32, value_non_null: u32, value_ptr: u32, value_len: u32) => { - if value_non_null != 0 && ctx.ext.max_value_size() < value_len { + // Set the value at the given key in the contract storage. + // + // The value length must not exceed the maximum defined by the contracts module parameters. + // Storing an empty value is disallowed. + // + // # Parameters + // + // - `key_ptr`: pointer into the linear memory where the location to store the value is placed. + // - `value_ptr`: pointer into the linear memory where the value to set is placed. + // - `value_len`: the length of the value in bytes. + // + // # Errors + // + // - If value length exceeds the configured maximum value length of a storage entry. + // - Upon trying to set an empty storage entry (value length is 0). + ext_set_storage(ctx, key_ptr: u32, value_ptr: u32, value_len: u32) => { + if value_len > ctx.ext.max_value_size() { + // Bail out if value length exceeds the set maximum value size. + return Err(sp_sandbox::HostError); + } + if value_len == 0 { + // Bail out on setting storage to an emptry entry. + // + // We might remove this constraint later if there are actual + // use cases that require setting empty contract storage entries. return Err(sp_sandbox::HostError); } let mut key: StorageKey = [0; 32]; read_sandbox_memory_into_buf(ctx, key_ptr, &mut key)?; - let value = - if value_non_null != 0 { - Some(read_sandbox_memory(ctx, value_ptr, value_len)?) - } else { - None - }; + let value = Some(read_sandbox_memory(ctx, value_ptr, value_len)?); ctx.ext.set_storage(key, value).map_err(|_| sp_sandbox::HostError)?; + Ok(()) + }, + // Clear the value at the given key in the contract storage. + // + // # Parameters + // + // - `key_ptr`: pointer into the linear memory where the location to clear the value is placed. + ext_clear_storage(ctx, key_ptr: u32) => { + let mut key: StorageKey = [0; 32]; + read_sandbox_memory_into_buf(ctx, key_ptr, &mut key)?; + ctx.ext.set_storage(key, None).map_err(|_| sp_sandbox::HostError)?; Ok(()) }, From db0674606b34c23474d690383efb2d1718a15ff7 Mon Sep 17 00:00:00 2001 From: Robin Freyler Date: Mon, 2 Mar 2020 14:13:01 +0100 Subject: [PATCH 2/6] update tests to adjust for the ext_set_storage changes --- frame/contracts/src/tests.rs | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index 51da3e8d1ff97..1b85e32e7ed2c 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -770,7 +770,8 @@ fn run_out_of_gas() { const CODE_SET_RENT: &str = r#" (module (import "env" "ext_dispatch_call" (func $ext_dispatch_call (param i32 i32))) - (import "env" "ext_set_storage" (func $ext_set_storage (param i32 i32 i32 i32))) + (import "env" "ext_set_storage" (func $ext_set_storage (param i32 i32 i32))) + (import "env" "ext_clear_storage" (func $ext_clear_storage (param i32))) (import "env" "ext_set_rent_allowance" (func $ext_set_rent_allowance (param i32 i32))) (import "env" "ext_scratch_size" (func $ext_scratch_size (result i32))) (import "env" "ext_scratch_read" (func $ext_scratch_read (param i32 i32 i32))) @@ -779,7 +780,6 @@ const CODE_SET_RENT: &str = r#" ;; insert a value of 4 bytes into storage (func $call_0 (call $ext_set_storage - (i32.const 1) (i32.const 1) (i32.const 0) (i32.const 4) @@ -788,11 +788,8 @@ const CODE_SET_RENT: &str = r#" ;; remove the value inserted by call_1 (func $call_1 - (call $ext_set_storage + (call $ext_clear_storage (i32.const 1) - (i32.const 0) - (i32.const 0) - (i32.const 0) ) ) @@ -852,7 +849,6 @@ const CODE_SET_RENT: &str = r#" ) (call $ext_set_storage (i32.const 0) - (i32.const 1) (i32.const 0) (i32.const 4) ) @@ -1327,7 +1323,7 @@ fn default_rent_allowance_on_instantiate() { const CODE_RESTORATION: &str = r#" (module - (import "env" "ext_set_storage" (func $ext_set_storage (param i32 i32 i32 i32))) + (import "env" "ext_set_storage" (func $ext_set_storage (param i32 i32 i32))) (import "env" "ext_restore_to" (func $ext_restore_to (param i32 i32 i32 i32 i32 i32 i32 i32))) (import "env" "memory" (memory 1 1)) @@ -1352,7 +1348,6 @@ const CODE_RESTORATION: &str = r#" ;; Data to restore (call $ext_set_storage (i32.const 0) - (i32.const 1) (i32.const 0) (i32.const 4) ) @@ -1360,7 +1355,6 @@ const CODE_RESTORATION: &str = r#" ;; ACL (call $ext_set_storage (i32.const 100) - (i32.const 1) (i32.const 0) (i32.const 4) ) @@ -1377,8 +1371,8 @@ const CODE_RESTORATION: &str = r#" ;; Code hash of SET_RENT (data (i32.const 264) - "\14\eb\65\3c\86\98\d6\b2\3d\8d\3c\4a\54\c6\c4\71" - "\b9\fc\19\36\df\ca\a0\a1\f2\dc\ad\9d\e5\36\0b\25" + "\c2\1c\41\10\a5\22\d8\59\1c\4c\77\35\dd\2d\bf\a1" + "\13\0b\50\93\76\9b\92\31\97\b7\c5\74\26\aa\38\2a" ) ;; Rent allowance @@ -1624,7 +1618,7 @@ fn restoration(test_different_storage: bool, test_restore_to_with_dirty_storage: const CODE_STORAGE_SIZE: &str = r#" (module (import "env" "ext_get_storage" (func $ext_get_storage (param i32) (result i32))) - (import "env" "ext_set_storage" (func $ext_set_storage (param i32 i32 i32 i32))) + (import "env" "ext_set_storage" (func $ext_set_storage (param i32 i32 i32))) (import "env" "ext_scratch_size" (func $ext_scratch_size (result i32))) (import "env" "ext_scratch_read" (func $ext_scratch_read (param i32 i32 i32))) (import "env" "memory" (memory 16 16)) @@ -1657,7 +1651,6 @@ const CODE_STORAGE_SIZE: &str = r#" ;; place a garbage value in storage, the size of which is specified by the call input. (call $ext_set_storage (i32.const 0) ;; Pointer to storage key - (i32.const 1) ;; Value is not null (i32.const 0) ;; Pointer to value (i32.load (i32.const 32)) ;; Size of value ) @@ -2278,7 +2271,7 @@ const CODE_DESTROY_AND_TRANSFER: &str = r#" (import "env" "ext_scratch_size" (func $ext_scratch_size (result i32))) (import "env" "ext_scratch_read" (func $ext_scratch_read (param i32 i32 i32))) (import "env" "ext_get_storage" (func $ext_get_storage (param i32) (result i32))) - (import "env" "ext_set_storage" (func $ext_set_storage (param i32 i32 i32 i32))) + (import "env" "ext_set_storage" (func $ext_set_storage (param i32 i32 i32))) (import "env" "ext_call" (func $ext_call (param i32 i32 i64 i32 i32 i32 i32) (result i32))) (import "env" "ext_instantiate" (func $ext_instantiate (param i32 i32 i64 i32 i32 i32 i32) (result i32))) (import "env" "memory" (memory 1 1)) @@ -2340,7 +2333,6 @@ const CODE_DESTROY_AND_TRANSFER: &str = r#" ;; Store the return address. (call $ext_set_storage (i32.const 16) ;; Pointer to the key - (i32.const 1) ;; Value is not null (i32.const 80) ;; Pointer to the value (i32.const 8) ;; Length of the value ) From 226062140ea31635cba44de6d92dfe54a0692d7d Mon Sep 17 00:00:00 2001 From: Robin Freyler Date: Mon, 2 Mar 2020 14:13:16 +0100 Subject: [PATCH 3/6] adjust COMPLEXITY for the ext_set_storage API changes --- frame/contracts/COMPLEXITY.md | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/frame/contracts/COMPLEXITY.md b/frame/contracts/COMPLEXITY.md index e44d3006c8ed3..0619e6c67a434 100644 --- a/frame/contracts/COMPLEXITY.md +++ b/frame/contracts/COMPLEXITY.md @@ -261,10 +261,21 @@ Each external function invoked from a contract can involve some overhead. This function receives a `key` and `value` as arguments. It consists of the following steps: 1. Reading the sandbox memory for `key` and `value` (see sandboxing memory get). -2. Setting the storage by the given `key` with the given `value` (see `set_storage`). +2. Setting the storage at the given `key` to the given `value` (see `set_storage`). **complexity**: Complexity is proportional to the size of the `value`. This function induces a DB write of size proportional to the `value` size (if flushed to the storage), so should be priced accordingly. +## ext_clear_storage + +This function receives a `key` as argument. It consists of the following steps: + +1. Reading the sandbox memory for `key` (see sandboxing memory get). +2. Clearing the storage at the given `key` (see `set_storage`). + +**complexity**: Complexity is constant. This function induces a DB write to clear the storage entry +(upon being flushed to the storage) and should be priced accordingly. The price should generally be +low enough to motivate clearing of unused contract storage. + ## ext_get_storage This function receives a `key` as an argument. It consists of the following steps: From 0d3619f3d3d365ed83954dedc6ccb178d2e66896 Mon Sep 17 00:00:00 2001 From: Robin Freyler Date: Tue, 3 Mar 2020 13:53:05 +0100 Subject: [PATCH 4/6] remove value_len == 0 constraint for ext_set_storage --- frame/contracts/src/wasm/runtime.rs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/frame/contracts/src/wasm/runtime.rs b/frame/contracts/src/wasm/runtime.rs index a8a516af84d4d..82232dde39a1b 100644 --- a/frame/contracts/src/wasm/runtime.rs +++ b/frame/contracts/src/wasm/runtime.rs @@ -368,13 +368,6 @@ define_env!(Env, , // Bail out if value length exceeds the set maximum value size. return Err(sp_sandbox::HostError); } - if value_len == 0 { - // Bail out on setting storage to an emptry entry. - // - // We might remove this constraint later if there are actual - // use cases that require setting empty contract storage entries. - return Err(sp_sandbox::HostError); - } let mut key: StorageKey = [0; 32]; read_sandbox_memory_into_buf(ctx, key_ptr, &mut key)?; let value = Some(read_sandbox_memory(ctx, value_ptr, value_len)?); From cd7d0def482fa555b88b79d5b234aee57b2530e8 Mon Sep 17 00:00:00 2001 From: Robin Freyler Date: Tue, 10 Mar 2020 17:00:01 +0100 Subject: [PATCH 5/6] bump spec_version --- bin/node/runtime/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 34cb8eb749e24..44d23cf7722ac 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -82,7 +82,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { // and set impl_version to 0. If only runtime // implementation changes and behavior does not, then leave spec_version as // is and increment impl_version. - spec_version: 234, + spec_version: 235, impl_version: 0, apis: RUNTIME_API_VERSIONS, }; From 0f959124d9f7afd2d0eec0759ded80972ce4a35e Mon Sep 17 00:00:00 2001 From: Robin Freyler Date: Tue, 10 Mar 2020 17:00:42 +0100 Subject: [PATCH 6/6] remove guarantee from COMPLEXITY of ext_clear_storage --- frame/contracts/COMPLEXITY.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frame/contracts/COMPLEXITY.md b/frame/contracts/COMPLEXITY.md index 0619e6c67a434..ac0b2535f556e 100644 --- a/frame/contracts/COMPLEXITY.md +++ b/frame/contracts/COMPLEXITY.md @@ -273,8 +273,7 @@ This function receives a `key` as argument. It consists of the following steps: 2. Clearing the storage at the given `key` (see `set_storage`). **complexity**: Complexity is constant. This function induces a DB write to clear the storage entry -(upon being flushed to the storage) and should be priced accordingly. The price should generally be -low enough to motivate clearing of unused contract storage. +(upon being flushed to the storage) and should be priced accordingly. ## ext_get_storage