From c879d1d5827f1fa4f72621ce185fa08ac3ea8d31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Mon, 11 Sep 2023 22:45:18 +0200 Subject: [PATCH] contracts: Run start function (#1367) Fixes #116 Start function wasn't allowed in a contract. Now it is allowed and is being run. It was disallowed because it is not used by Rust and supporting it made the code more complex. However, not running the start function violates the wasm standard. This makes life harder for some languages (see linked ticket). --- .../fixtures/run_out_of_gas_start_fn.wat | 10 + .../contracts/src/benchmarking/sandbox.rs | 8 +- substrate/frame/contracts/src/exec.rs | 111 ++++----- substrate/frame/contracts/src/lib.rs | 8 +- substrate/frame/contracts/src/tests.rs | 21 ++ substrate/frame/contracts/src/wasm/mod.rs | 210 +++++++++--------- 6 files changed, 197 insertions(+), 171 deletions(-) create mode 100644 substrate/frame/contracts/fixtures/run_out_of_gas_start_fn.wat diff --git a/substrate/frame/contracts/fixtures/run_out_of_gas_start_fn.wat b/substrate/frame/contracts/fixtures/run_out_of_gas_start_fn.wat new file mode 100644 index 0000000000..6591d7ede7 --- /dev/null +++ b/substrate/frame/contracts/fixtures/run_out_of_gas_start_fn.wat @@ -0,0 +1,10 @@ +(module + (import "env" "memory" (memory 1 1)) + (start $start) + (func $start + (loop $inf (br $inf)) ;; just run out of gas + (unreachable) + ) + (func (export "call")) + (func (export "deploy")) +) diff --git a/substrate/frame/contracts/src/benchmarking/sandbox.rs b/substrate/frame/contracts/src/benchmarking/sandbox.rs index 34974b02ea..c3abbcad5f 100644 --- a/substrate/frame/contracts/src/benchmarking/sandbox.rs +++ b/substrate/frame/contracts/src/benchmarking/sandbox.rs @@ -58,7 +58,13 @@ impl From<&WasmModule> for Sandbox { .add_fuel(u64::MAX) .expect("We've set up engine to fuel consuming mode; qed"); - let entry_point = instance.get_export(&store, "call").unwrap().into_func().unwrap(); + let entry_point = instance + .start(&mut store) + .unwrap() + .get_export(&store, "call") + .unwrap() + .into_func() + .unwrap(); Self { entry_point, store } } } diff --git a/substrate/frame/contracts/src/exec.rs b/substrate/frame/contracts/src/exec.rs index fdb30310ef..f93e7a2b21 100644 --- a/substrate/frame/contracts/src/exec.rs +++ b/substrate/frame/contracts/src/exec.rs @@ -21,7 +21,7 @@ use crate::{ storage::{self, meter::Diff, WriteOutcome}, BalanceOf, CodeHash, CodeInfo, CodeInfoOf, Config, ContractInfo, ContractInfoOf, DebugBufferVec, Determinism, Error, Event, Nonce, Origin, Pallet as Contracts, Schedule, - WasmBlob, LOG_TARGET, + LOG_TARGET, }; use frame_support::{ crypto::ecdsa::ECDSAExt, @@ -318,6 +318,22 @@ pub trait Ext: sealing::Sealed { /// Returns a nonce that is incremented for every instantiated contract. fn nonce(&mut self) -> u64; + /// Increment the reference count of a of a stored code by one. + /// + /// # Errors + /// + /// [`Error::CodeNotFound`] is returned if no stored code found having the specified + /// `code_hash`. + fn increment_refcount(code_hash: CodeHash) -> Result<(), DispatchError>; + + /// Decrement the reference count of a stored code by one. + /// + /// # Note + /// + /// A contract whose reference count dropped to zero isn't automatically removed. A + /// `remove_code` transaction must be submitted by the original uploader to do so. + fn decrement_refcount(code_hash: CodeHash); + /// Adds a delegate dependency to [`ContractInfo`]'s `delegate_dependencies` field. /// /// This ensures that the delegated contract is not removed while it is still in use. It @@ -381,22 +397,6 @@ pub trait Executable: Sized { gas_meter: &mut GasMeter, ) -> Result; - /// Increment the reference count of a of a stored code by one. - /// - /// # Errors - /// - /// [`Error::CodeNotFound`] is returned if no stored code found having the specified - /// `code_hash`. - fn increment_refcount(code_hash: CodeHash) -> Result<(), DispatchError>; - - /// Decrement the reference count of a stored code by one. - /// - /// # Note - /// - /// A contract whose reference count dropped to zero isn't automatically removed. A - /// `remove_code` transaction must be submitted by the original uploader to do so. - fn decrement_refcount(code_hash: CodeHash); - /// Execute the specified exported function and return the result. /// /// When the specified function is `Constructor` the executable is stored and its @@ -1285,10 +1285,10 @@ where info.queue_trie_for_deletion(); ContractInfoOf::::remove(&frame.account_id); - E::decrement_refcount(info.code_hash); + Self::decrement_refcount(info.code_hash); for (code_hash, deposit) in info.delegate_dependencies() { - E::decrement_refcount(*code_hash); + Self::decrement_refcount(*code_hash); frame .nested_storage .charge_deposit(frame.account_id.clone(), StorageDeposit::Refund(*deposit)); @@ -1491,8 +1491,8 @@ where frame.nested_storage.charge_deposit(frame.account_id.clone(), deposit); - E::increment_refcount(hash)?; - E::decrement_refcount(prev_hash); + Self::increment_refcount(hash)?; + Self::decrement_refcount(prev_hash); Contracts::::deposit_event( vec![T::Hashing::hash_of(&frame.account_id), hash, prev_hash], Event::ContractCodeUpdated { @@ -1525,6 +1525,25 @@ where } } + fn increment_refcount(code_hash: CodeHash) -> Result<(), DispatchError> { + >::mutate(code_hash, |existing| -> Result<(), DispatchError> { + if let Some(info) = existing { + *info.refcount_mut() = info.refcount().saturating_add(1); + Ok(()) + } else { + Err(Error::::CodeNotFound.into()) + } + }) + } + + fn decrement_refcount(code_hash: CodeHash) { + >::mutate(code_hash, |existing| { + if let Some(info) = existing { + *info.refcount_mut() = info.refcount().saturating_sub(1); + } + }); + } + fn add_delegate_dependency( &mut self, code_hash: CodeHash, @@ -1537,7 +1556,7 @@ where let deposit = T::CodeHashLockupDepositPercent::get().mul_ceil(code_info.deposit()); info.add_delegate_dependency(code_hash, deposit)?; - >::increment_refcount(code_hash)?; + Self::increment_refcount(code_hash)?; frame .nested_storage .charge_deposit(frame.account_id.clone(), StorageDeposit::Charge(deposit)); @@ -1552,8 +1571,7 @@ where let info = frame.contract_info.get(&frame.account_id); let deposit = info.remove_delegate_dependency(code_hash)?; - >::decrement_refcount(*code_hash); - + Self::decrement_refcount(*code_hash); frame .nested_storage .charge_deposit(frame.account_id.clone(), StorageDeposit::Refund(deposit)); @@ -1600,11 +1618,7 @@ mod tests { use pallet_contracts_primitives::ReturnFlags; use pretty_assertions::assert_eq; use sp_runtime::{traits::Hash, DispatchError}; - use std::{ - cell::RefCell, - collections::hash_map::{Entry, HashMap}, - rc::Rc, - }; + use std::{cell::RefCell, collections::hash_map::HashMap, rc::Rc}; type System = frame_system::Pallet; @@ -1635,7 +1649,6 @@ mod tests { func_type: ExportedFunction, code_hash: CodeHash, code_info: CodeInfo, - refcount: u64, } #[derive(Default, Clone)] @@ -1664,37 +1677,11 @@ mod tests { func_type, code_hash: hash, code_info: CodeInfo::::new(ALICE), - refcount: 1, }, ); hash }) } - - fn increment_refcount(code_hash: CodeHash) -> Result<(), DispatchError> { - Loader::mutate(|loader| { - match loader.map.entry(code_hash) { - Entry::Vacant(_) => Err(>::CodeNotFound)?, - Entry::Occupied(mut entry) => entry.get_mut().refcount += 1, - } - Ok(()) - }) - } - - fn decrement_refcount(code_hash: CodeHash) { - use std::collections::hash_map::Entry::Occupied; - Loader::mutate(|loader| { - let mut entry = match loader.map.entry(code_hash) { - Occupied(e) => e, - _ => panic!("code_hash does not exist"), - }; - let refcount = &mut entry.get_mut().refcount; - *refcount -= 1; - if *refcount == 0 { - entry.remove(); - } - }); - } } impl Executable for MockExecutable { @@ -1707,14 +1694,6 @@ mod tests { }) } - fn increment_refcount(code_hash: CodeHash) -> Result<(), DispatchError> { - MockLoader::increment_refcount(code_hash) - } - - fn decrement_refcount(code_hash: CodeHash) { - MockLoader::decrement_refcount(code_hash); - } - fn execute>( self, ext: &mut E, @@ -1722,7 +1701,7 @@ mod tests { input_data: Vec, ) -> ExecResult { if let &Constructor = function { - Self::increment_refcount(self.code_hash).unwrap(); + E::increment_refcount(self.code_hash).unwrap(); } // # Safety // @@ -1733,7 +1712,7 @@ mod tests { // The transmute is necessary because `execute` has to be generic over all // `E: Ext`. However, `MockExecutable` can't be generic over `E` as it would // constitute a cycle. - let ext = unsafe { std::mem::transmute(ext) }; + let ext = unsafe { mem::transmute(ext) }; if function == &self.func_type { (self.func)(MockCtx { ext, input_data }, &self) } else { diff --git a/substrate/frame/contracts/src/lib.rs b/substrate/frame/contracts/src/lib.rs index e22e4a3f9f..7d516fbe24 100644 --- a/substrate/frame/contracts/src/lib.rs +++ b/substrate/frame/contracts/src/lib.rs @@ -103,7 +103,9 @@ pub mod weights; #[cfg(test)] mod tests; use crate::{ - exec::{AccountIdOf, ErrorOrigin, ExecError, Executable, Key, MomentOf, Stack as ExecStack}, + exec::{ + AccountIdOf, ErrorOrigin, ExecError, Executable, Ext, Key, MomentOf, Stack as ExecStack, + }, gas::GasMeter, storage::{meter::Meter as StorageMeter, ContractInfo, DeletionQueueManager}, wasm::{CodeInfo, WasmBlob}, @@ -658,8 +660,8 @@ pub mod pallet { } else { return Err(>::ContractNotFound.into()) }; - >::increment_refcount(code_hash)?; - >::decrement_refcount(contract.code_hash); + >>::increment_refcount(code_hash)?; + >>::decrement_refcount(contract.code_hash); Self::deposit_event( vec![T::Hashing::hash_of(&dest), code_hash, contract.code_hash], Event::ContractCodeUpdated { diff --git a/substrate/frame/contracts/src/tests.rs b/substrate/frame/contracts/src/tests.rs index 8d6c5c5ac7..0fea2b1559 100644 --- a/substrate/frame/contracts/src/tests.rs +++ b/substrate/frame/contracts/src/tests.rs @@ -862,6 +862,27 @@ fn deposit_event_max_value_limit() { }); } +// Fail out of fuel (ref_time weight) inside the start function. +#[test] +fn run_out_of_fuel_start_fun() { + let (wasm, _code_hash) = compile_module::("run_out_of_gas_start_fn").unwrap(); + ExtBuilder::default().existential_deposit(50).build().execute_with(|| { + let _ = ::Currency::set_balance(&ALICE, 1_000_000); + assert_err_ignore_postinfo!( + Contracts::instantiate_with_code( + RuntimeOrigin::signed(ALICE), + 0, + Weight::from_parts(1_000_000_000_000, u64::MAX), + None, + wasm, + vec![], + vec![], + ), + Error::::OutOfGas, + ); + }); +} + // Fail out of fuel (ref_time weight) in the engine. #[test] fn run_out_of_fuel_engine() { diff --git a/substrate/frame/contracts/src/wasm/mod.rs b/substrate/frame/contracts/src/wasm/mod.rs index 5fc65e314a..77e94b1677 100644 --- a/substrate/frame/contracts/src/wasm/mod.rs +++ b/substrate/frame/contracts/src/wasm/mod.rs @@ -49,7 +49,7 @@ use frame_support::{ use sp_core::Get; use sp_runtime::{DispatchError, RuntimeDebug}; use sp_std::prelude::*; -use wasmi::{Instance, Linker, Memory, MemoryType, StackLimits, Store}; +use wasmi::{InstancePre, Linker, Memory, MemoryType, StackLimits, Store}; const BYTES_PER_PAGE: usize = 64 * 1024; @@ -164,7 +164,30 @@ impl WasmBlob { /// /// Applies all necessary checks before removing the code. pub fn remove(origin: &T::AccountId, code_hash: CodeHash) -> DispatchResult { - Self::try_remove_code(origin, code_hash) + >::try_mutate_exists(&code_hash, |existing| { + if let Some(code_info) = existing { + ensure!(code_info.refcount == 0, >::CodeInUse); + ensure!(&code_info.owner == origin, BadOrigin); + let _ = T::Currency::release( + &HoldReason::CodeUploadDepositReserve.into(), + &code_info.owner, + code_info.deposit, + BestEffort, + ); + let deposit_released = code_info.deposit; + let remover = code_info.owner.clone(); + + *existing = None; + >::remove(&code_hash); + >::deposit_event( + vec![code_hash], + Event::CodeRemoved { code_hash, deposit_released, remover }, + ); + Ok(()) + } else { + Err(>::CodeNotFound.into()) + } + }) } /// Creates and returns an instance of the supplied code. @@ -179,7 +202,7 @@ impl WasmBlob { determinism: Determinism, stack_limits: StackLimits, allow_deprecated: AllowDeprecatedInterface, - ) -> Result<(Store, Memory, Instance), &'static str> + ) -> Result<(Store, Memory, InstancePre), &'static str> where E: Environment, { @@ -217,9 +240,7 @@ impl WasmBlob { let instance = linker .instantiate(&mut store, &contract.module) - .map_err(|_| "can't instantiate module with provided definitions")? - .ensure_no_start(&mut store) - .map_err(|_| "start function is forbidden but found in the module")?; + .map_err(|_| "can't instantiate module with provided definitions")?; Ok((store, memory, instance)) } @@ -261,45 +282,6 @@ impl WasmBlob { }) } - /// Try to remove code together with all associated information. - fn try_remove_code(origin: &T::AccountId, code_hash: CodeHash) -> DispatchResult { - >::try_mutate_exists(&code_hash, |existing| { - if let Some(code_info) = existing { - ensure!(code_info.refcount == 0, >::CodeInUse); - ensure!(&code_info.owner == origin, BadOrigin); - let _ = T::Currency::release( - &HoldReason::CodeUploadDepositReserve.into(), - &code_info.owner, - code_info.deposit, - BestEffort, - ); - let deposit_released = code_info.deposit; - let remover = code_info.owner.clone(); - - *existing = None; - >::remove(&code_hash); - >::deposit_event( - vec![code_hash], - Event::CodeRemoved { code_hash, deposit_released, remover }, - ); - Ok(()) - } else { - Err(>::CodeNotFound.into()) - } - }) - } - - /// Load code with the given code hash. - fn load_code( - code_hash: CodeHash, - gas_meter: &mut GasMeter, - ) -> Result<(CodeVec, CodeInfo), DispatchError> { - let code_info = >::get(code_hash).ok_or(Error::::CodeNotFound)?; - gas_meter.charge(CodeLoadToken(code_info.code_len))?; - let code = >::get(code_hash).ok_or(Error::::CodeNotFound)?; - Ok((code, code_info)) - } - /// Create the module without checking the passed code. /// /// # Note @@ -318,12 +300,6 @@ impl WasmBlob { } impl CodeInfo { - /// Return the refcount of the module. - #[cfg(test)] - pub fn refcount(&self) -> u64 { - self.refcount - } - #[cfg(test)] pub fn new(owner: T::AccountId) -> Self { CodeInfo { @@ -335,6 +311,16 @@ impl CodeInfo { } } + /// Returns reference count of the module. + pub fn refcount(&self) -> u64 { + self.refcount + } + + /// Return mutable reference to the refcount of the module. + pub fn refcount_mut(&mut self) -> &mut u64 { + &mut self.refcount + } + /// Returns the deposit of the module. pub fn deposit(&self) -> BalanceOf { self.deposit @@ -346,29 +332,12 @@ impl Executable for WasmBlob { code_hash: CodeHash, gas_meter: &mut GasMeter, ) -> Result { - let (code, code_info) = Self::load_code(code_hash, gas_meter)?; + let code_info = >::get(code_hash).ok_or(Error::::CodeNotFound)?; + gas_meter.charge(CodeLoadToken(code_info.code_len))?; + let code = >::get(code_hash).ok_or(Error::::CodeNotFound)?; Ok(Self { code, code_info, code_hash }) } - fn increment_refcount(code_hash: CodeHash) -> Result<(), DispatchError> { - >::mutate(code_hash, |existing| -> Result<(), DispatchError> { - if let Some(info) = existing { - info.refcount = info.refcount.saturating_add(1); - Ok(()) - } else { - Err(Error::::CodeNotFound.into()) - } - }) - } - - fn decrement_refcount(code_hash: CodeHash) { - >::mutate(code_hash, |existing| { - if let Some(info) = existing { - info.refcount = info.refcount.saturating_sub(1); - } - }); - } - fn execute>( self, ext: &mut E, @@ -410,25 +379,38 @@ impl Executable for WasmBlob { .add_fuel(fuel_limit) .expect("We've set up engine to fuel consuming mode; qed"); - let exported_func = instance - .get_export(&store, function.identifier()) - .and_then(|export| export.into_func()) - .ok_or_else(|| { - log::error!(target: LOG_TARGET, "failed to find entry point"); - Error::::CodeRejected - })?; + // Sync this frame's gas meter with the engine's one. + let process_result = |mut store: Store>, result| { + let engine_consumed_total = + store.fuel_consumed().expect("Fuel metering is enabled; qed"); + let gas_meter = store.data_mut().ext().gas_meter_mut(); + gas_meter.charge_fuel(engine_consumed_total)?; + store.into_data().to_execution_result(result) + }; + // Start function should already see the correct refcount in case it will be ever inspected. if let &ExportedFunction::Constructor = function { - WasmBlob::::increment_refcount(self.code_hash)?; + E::increment_refcount(self.code_hash)?; } - let result = exported_func.call(&mut store, &[], &mut []); - let engine_consumed_total = store.fuel_consumed().expect("Fuel metering is enabled; qed"); - // Sync this frame's gas meter with the engine's one. - let gas_meter = store.data_mut().ext().gas_meter_mut(); - gas_meter.charge_fuel(engine_consumed_total)?; + // Any abort in start function (includes `return` + `terminate`) will make us skip the + // call into the subsequent exported function. This means that calling `return` returns data + // from the whole contract execution. + match instance.start(&mut store) { + Ok(instance) => { + let exported_func = instance + .get_export(&store, function.identifier()) + .and_then(|export| export.into_func()) + .ok_or_else(|| { + log::error!(target: LOG_TARGET, "failed to find entry point"); + Error::::CodeRejected + })?; - store.into_data().to_execution_result(result) + let result = exported_func.call(&mut store, &[], &mut []); + process_result(store, result) + }, + Err(err) => process_result(store, Err(err)), + } } fn code_hash(&self) -> &CodeHash { @@ -740,7 +722,10 @@ mod tests { fn nonce(&mut self) -> u64 { 995 } - + fn increment_refcount(_code_hash: CodeHash) -> Result<(), DispatchError> { + Ok(()) + } + fn decrement_refcount(_code_hash: CodeHash) {} fn add_delegate_dependency( &mut self, code: CodeHash, @@ -748,7 +733,6 @@ mod tests { self.delegate_dependencies.borrow_mut().insert(code); Ok(()) } - fn remove_delegate_dependency( &mut self, code: &CodeHash, @@ -790,11 +774,20 @@ mod tests { executable.execute(ext.borrow_mut(), entry_point, input_data) } - /// Execute the supplied code. + /// Execute the `call` function within the supplied code. fn execute>(wat: &str, input_data: Vec, ext: E) -> ExecResult { execute_internal(wat, input_data, ext, &ExportedFunction::Call, true, false) } + /// Execute the `deploy` function within the supplied code. + fn execute_instantiate>( + wat: &str, + input_data: Vec, + ext: E, + ) -> ExecResult { + execute_internal(wat, input_data, ext, &ExportedFunction::Constructor, true, false) + } + /// Execute the supplied code with disabled unstable functions. /// /// In our test config unstable functions are disabled so that we can test them. @@ -1878,32 +1871,47 @@ mod tests { assert_ok!(execute(CODE_VALUE_TRANSFERRED, vec![], MockExt::default())); } - const START_FN_ILLEGAL: &str = r#" + const START_FN_DOES_RUN: &str = r#" (module - (import "seal0" "seal_return" (func $seal_return (param i32 i32 i32))) + (import "seal0" "seal_deposit_event" (func $seal_deposit_event (param i32 i32 i32 i32))) (import "env" "memory" (memory 1 1)) (start $start) (func $start - (unreachable) + (call $seal_deposit_event + (i32.const 0) ;; Pointer to the start of topics buffer + (i32.const 0) ;; The length of the topics buffer. + (i32.const 0) ;; Pointer to the start of the data buffer + (i32.const 13) ;; Length of the buffer + ) ) - (func (export "call") - (unreachable) - ) + (func (export "call")) - (func (export "deploy") - (unreachable) - ) + (func (export "deploy")) - (data (i32.const 8) "\01\02\03\04") + (data (i32.const 0) "\00\01\2A\00\00\00\00\00\00\00\E5\14\00") ) "#; #[test] - fn start_fn_illegal() { - let output = execute(START_FN_ILLEGAL, vec![], MockExt::default()); - assert_err!(output, >::CodeRejected,); + fn start_fn_does_run_on_call() { + let mut ext = MockExt::default(); + execute(START_FN_DOES_RUN, vec![], &mut ext).unwrap(); + assert_eq!( + ext.events[0].1, + [0x00_u8, 0x01, 0x2a, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xe5, 0x14, 0x00] + ); + } + + #[test] + fn start_fn_does_run_on_deploy() { + let mut ext = MockExt::default(); + execute_instantiate(START_FN_DOES_RUN, vec![], &mut ext).unwrap(); + assert_eq!( + ext.events[0].1, + [0x00_u8, 0x01, 0x2a, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xe5, 0x14, 0x00] + ); } const CODE_TIMESTAMP_NOW: &str = r#"