diff --git a/substrate/frame/contracts/src/exec.rs b/substrate/frame/contracts/src/exec.rs index d19408f95c..05eaf52c1b 100644 --- a/substrate/frame/contracts/src/exec.rs +++ b/substrate/frame/contracts/src/exec.rs @@ -24,7 +24,7 @@ use sp_core::crypto::UncheckedFrom; use sp_std::prelude::*; use sp_runtime::traits::{Bounded, Zero, Convert, Saturating}; use frame_support::{ - dispatch::DispatchError, + dispatch::DispatchResult, traits::{ExistenceRequirement, Currency, Time, Randomness}, weights::Weight, ensure, StorageMap, @@ -65,7 +65,7 @@ pub trait Ext { /// Sets the storage entry by the given key to the specified value. If `value` is `None` then /// the storage entry is deleted. - fn set_storage(&mut self, key: StorageKey, value: Option>); + fn set_storage(&mut self, key: StorageKey, value: Option>) -> DispatchResult; /// Instantiate a contract from the given code. /// @@ -85,7 +85,7 @@ pub trait Ext { &mut self, to: &AccountIdOf, value: BalanceOf, - ) -> Result<(), DispatchError>; + ) -> DispatchResult; /// Transfer all funds to `beneficiary` and delete the contract. /// @@ -97,7 +97,7 @@ pub trait Ext { fn terminate( &mut self, beneficiary: &AccountIdOf, - ) -> Result<(), DispatchError>; + ) -> DispatchResult; /// Call (possibly transferring some amount of funds) into the specified account. fn call( @@ -121,7 +121,7 @@ pub trait Ext { code_hash: CodeHash, rent_allowance: BalanceOf, delta: Vec, - ) -> Result<(), DispatchError>; + ) -> DispatchResult; /// Returns a reference to the account id of the caller. fn caller(&self) -> &AccountIdOf; @@ -454,7 +454,7 @@ fn transfer<'a, T: Config, V: Vm, L: Loader>( dest: &T::AccountId, value: BalanceOf, ctx: &mut ExecutionContext<'a, T, V, L>, -) -> Result<(), DispatchError> +) -> DispatchResult where T::AccountId: UncheckedFrom + AsRef<[u8]>, { @@ -520,23 +520,19 @@ where Storage::::read(trie_id, key) } - fn set_storage(&mut self, key: StorageKey, value: Option>) { + fn set_storage(&mut self, key: StorageKey, value: Option>) -> DispatchResult { let trie_id = self.ctx.self_trie_id.as_ref().expect( "`ctx.self_trie_id` points to an alive contract within the `CallContext`;\ it cannot be `None`;\ expect can't fail;\ qed", ); - if let Err(storage::ContractAbsentError) = - Storage::::write(&self.ctx.self_account, trie_id, &key, value) - { - panic!( - "the contract must be in the alive state within the `CallContext`;\ - the contract cannot be absent in storage; - write cannot return `None`; - qed" - ); - } + // write panics if the passed account is not alive. + // the contract must be in the alive state within the `CallContext`;\ + // the contract cannot be absent in storage; + // write cannot return `None`; + // qed + Storage::::write(&self.ctx.self_account, trie_id, &key, value) } fn instantiate( @@ -554,7 +550,7 @@ where &mut self, to: &T::AccountId, value: BalanceOf, - ) -> Result<(), DispatchError> { + ) -> DispatchResult { transfer( TransferCause::Call, TransactorKind::Contract, @@ -568,7 +564,7 @@ where fn terminate( &mut self, beneficiary: &AccountIdOf, - ) -> Result<(), DispatchError> { + ) -> DispatchResult { let self_id = self.ctx.self_account.clone(); let value = T::Currency::free_balance(&self_id); if let Some(caller_ctx) = self.ctx.caller { @@ -612,7 +608,7 @@ where code_hash: CodeHash, rent_allowance: BalanceOf, delta: Vec, - ) -> Result<(), DispatchError> { + ) -> DispatchResult { if let Some(caller_ctx) = self.ctx.caller { if caller_ctx.is_live(&self.ctx.self_account) { return Err(Error::::ReentranceDenied.into()); diff --git a/substrate/frame/contracts/src/lib.rs b/substrate/frame/contracts/src/lib.rs index 2e9b934e4d..1c191bfa04 100644 --- a/substrate/frame/contracts/src/lib.rs +++ b/substrate/frame/contracts/src/lib.rs @@ -421,6 +421,11 @@ decl_error! { /// This can be returned from [`Module::claim_surcharge`] because the target /// contract has enough balance to pay for its rent. ContractNotEvictable, + /// A storage modification exhausted the 32bit type that holds the storage size. + /// + /// This can either happen when the accumulated storage in bytes is too large or + /// when number of storage items is too large. + StorageExhausted, } } diff --git a/substrate/frame/contracts/src/storage.rs b/substrate/frame/contracts/src/storage.rs index b4f3071c1e..0d4393aa96 100644 --- a/substrate/frame/contracts/src/storage.rs +++ b/substrate/frame/contracts/src/storage.rs @@ -49,8 +49,6 @@ pub struct DeletedContract { trie_id: TrieId, } - - pub struct Storage(PhantomData); impl Storage @@ -75,15 +73,19 @@ where /// `read`, this function also requires the `account` ID. /// /// If the contract specified by the id `account` doesn't exist `Err` is returned.` + /// + /// # Panics + /// + /// Panics iff the `account` specified is not alive and in storage. pub fn write( account: &AccountIdOf, trie_id: &TrieId, key: &StorageKey, opt_new_value: Option>, - ) -> Result<(), ContractAbsentError> { + ) -> DispatchResult { let mut new_info = match >::get(account) { Some(ContractInfo::Alive(alive)) => alive, - None | Some(ContractInfo::Tombstone(_)) => return Err(ContractAbsentError), + None | Some(ContractInfo::Tombstone(_)) => panic!("Contract not found"), }; let hashed_key = blake2_256(key); @@ -94,10 +96,12 @@ where // Update the total number of KV pairs and the number of empty pairs. match (&opt_prev_len, &opt_new_value) { (Some(_), None) => { - new_info.pair_count -= 1; + new_info.pair_count = new_info.pair_count.checked_sub(1) + .ok_or_else(|| Error::::StorageExhausted)?; }, (None, Some(_)) => { - new_info.pair_count += 1; + new_info.pair_count = new_info.pair_count.checked_add(1) + .ok_or_else(|| Error::::StorageExhausted)?; }, (Some(_), Some(_)) => {}, (None, None) => {}, @@ -111,8 +115,9 @@ where .unwrap_or(0); new_info.storage_size = new_info .storage_size - .saturating_add(new_value_len) - .saturating_sub(prev_value_len); + .checked_sub(prev_value_len) + .and_then(|val| val.checked_add(new_value_len)) + .ok_or_else(|| Error::::StorageExhausted)?; new_info.last_write = Some(>::block_number()); >::insert(&account, ContractInfo::Alive(new_info)); diff --git a/substrate/frame/contracts/src/wasm/mod.rs b/substrate/frame/contracts/src/wasm/mod.rs index e295febb51..45c927dfaa 100644 --- a/substrate/frame/contracts/src/wasm/mod.rs +++ b/substrate/frame/contracts/src/wasm/mod.rs @@ -18,15 +18,15 @@ //! This module provides a means for executing contracts //! represented in wasm. -use crate::{CodeHash, Schedule, Config}; -use crate::wasm::env_def::FunctionImplProvider; -use crate::exec::Ext; -use crate::gas::GasMeter; - +use crate::{ + CodeHash, Schedule, Config, + wasm::env_def::FunctionImplProvider, + exec::Ext, + gas::GasMeter, +}; use sp_std::prelude::*; use sp_core::crypto::UncheckedFrom; use codec::{Encode, Decode}; -use sp_sandbox; #[macro_use] mod env_def; @@ -172,7 +172,7 @@ mod tests { use sp_core::H256; use hex_literal::hex; use sp_runtime::DispatchError; - use frame_support::weights::Weight; + use frame_support::{dispatch::DispatchResult, weights::Weight}; use assert_matches::assert_matches; use pallet_contracts_primitives::{ExecReturnValue, ReturnFlags, ExecError, ErrorOrigin}; @@ -228,8 +228,9 @@ mod tests { fn get_storage(&self, key: &StorageKey) -> Option> { self.storage.get(key).cloned() } - fn set_storage(&mut self, key: StorageKey, value: Option>) { + fn set_storage(&mut self, key: StorageKey, value: Option>) -> DispatchResult { *self.storage.entry(key).or_insert(Vec::new()) = value.unwrap_or(Vec::new()); + Ok(()) } fn instantiate( &mut self, @@ -362,7 +363,7 @@ mod tests { fn get_storage(&self, key: &[u8; 32]) -> Option> { (**self).get_storage(key) } - fn set_storage(&mut self, key: [u8; 32], value: Option>) { + fn set_storage(&mut self, key: [u8; 32], value: Option>) -> DispatchResult { (**self).set_storage(key, value) } fn instantiate( diff --git a/substrate/frame/contracts/src/wasm/runtime.rs b/substrate/frame/contracts/src/wasm/runtime.rs index 88f51046d9..6b459f0519 100644 --- a/substrate/frame/contracts/src/wasm/runtime.rs +++ b/substrate/frame/contracts/src/wasm/runtime.rs @@ -643,8 +643,7 @@ define_env!(Env, , let mut key: StorageKey = [0; 32]; ctx.read_sandbox_memory_into_buf(key_ptr, &mut key)?; let value = Some(ctx.read_sandbox_memory(value_ptr, value_len)?); - ctx.ext.set_storage(key, value); - Ok(()) + ctx.ext.set_storage(key, value).map_err(Into::into) }, // Clear the value at the given key in the contract storage. @@ -656,8 +655,7 @@ define_env!(Env, , ctx.charge_gas(RuntimeToken::ClearStorage)?; let mut key: StorageKey = [0; 32]; ctx.read_sandbox_memory_into_buf(key_ptr, &mut key)?; - ctx.ext.set_storage(key, None); - Ok(()) + ctx.ext.set_storage(key, None).map_err(Into::into) }, // Retrieve the value under the given key from storage.