From 9a213c000df055b7ff5d03bcc8d33bbb05d120e8 Mon Sep 17 00:00:00 2001 From: Alexander Gryaznov Date: Wed, 22 Feb 2023 11:05:20 +0200 Subject: [PATCH 01/17] save: compiles and tests pass --- frame/contracts/src/lib.rs | 304 ++++++++++++++++++++----------------- 1 file changed, 161 insertions(+), 143 deletions(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index a5f35a9cb5f3b..741c7b9fb7bd1 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -616,16 +616,16 @@ pub mod pallet { let gas_limit: Weight = gas_limit.into(); let origin = ensure_signed(origin)?; let dest = T::Lookup::lookup(dest)?; - let mut output = Self::internal_call( + let input = CallInput { origin, dest, value, gas_limit, - storage_deposit_limit.map(Into::into), + storage_deposit_limit: storage_deposit_limit.map(Into::into), data, - None, - Determinism::Deterministic, - ); + determinism: Determinism::Deterministic, + }; + let mut output = Self::internal_run(input, None); if let Ok(retval) = &output.result { if retval.did_revert() { output.result = Err(>::ContractReverted.into()); @@ -678,16 +678,16 @@ pub mod pallet { let code_len = code.len() as u32; let data_len = data.len() as u32; let salt_len = salt.len() as u32; - let mut output = Self::internal_instantiate( + let input = InstantiateInput { origin, value, gas_limit, - storage_deposit_limit.map(Into::into), - Code::Upload(code), + storage_deposit_limit: storage_deposit_limit.map(Into::into), + code: Code::Upload(code), data, salt, - None, - ); + }; + let mut output = Self::internal_run(input, None); if let Ok(retval) = &output.result { if retval.1.did_revert() { output.result = Err(>::ContractReverted.into()); @@ -720,16 +720,16 @@ pub mod pallet { let origin = ensure_signed(origin)?; let data_len = data.len() as u32; let salt_len = salt.len() as u32; - let mut output = Self::internal_instantiate( + let input = InstantiateInput { origin, value, gas_limit, - storage_deposit_limit.map(Into::into), - Code::Existing(code_hash), + storage_deposit_limit: storage_deposit_limit.map(Into::into), + code: Code::Existing(code_hash), data, salt, - None, - ); + }; + let mut output = Self::internal_run(input, None); if let Ok(retval) = &output.result { if retval.1.did_revert() { output.result = Err(>::ContractReverted.into()); @@ -951,11 +951,27 @@ pub mod pallet { StorageValue<_, BoundedVec, ValueQuery>; } -/// Return type of the private [`Pallet::internal_call`] function. -type InternalCallOutput = InternalOutput; +/// TODO: put into primitives +struct CallInput { + origin: T::AccountId, + dest: T::AccountId, + value: BalanceOf, + gas_limit: Weight, + storage_deposit_limit: Option>, + data: Vec, + determinism: Determinism, +} -/// Return type of the private [`Pallet::internal_instantiate`] function. -type InternalInstantiateOutput = InternalOutput, ExecReturnValue)>; +/// TODO: put into primitives +struct InstantiateInput { + origin: T::AccountId, + value: BalanceOf, + gas_limit: Weight, + storage_deposit_limit: Option>, + code: Code>, + data: Vec, + salt: Vec, +} /// Return type of private helper functions. struct InternalOutput { @@ -967,6 +983,111 @@ struct InternalOutput { result: Result, } +/// TODO: put into primitives +/// Helper trait to wrap contract execution entry points into a signle function [`internal_run`] +trait Invokable { + type Output; + + fn run(&self, debug_message: Option<&mut DebugBufferVec>) -> Self::Output; +} + +impl Invokable for CallInput { + type Output = InternalOutput; + + /// Internal function that does the actual call to contract. + /// + /// Called by dispatchables and public functions. + fn run(&self, debug_message: Option<&mut DebugBufferVec>) -> Self::Output { + let mut gas_meter = GasMeter::new(self.gas_limit); + let mut storage_meter = + match StorageMeter::new(&self.origin, self.storage_deposit_limit, self.value) { + Ok(meter) => meter, + Err(err) => + return InternalOutput { + result: Err(err.into()), + gas_meter, + storage_deposit: Default::default(), + }, + }; + let schedule = T::Schedule::get(); + let CallInput { origin, dest, value, data, determinism, .. } = self; + let result = ExecStack::>::run_call( + origin.clone(), + dest.clone(), + &mut gas_meter, + &mut storage_meter, + &schedule, + *value, + data.clone(), + debug_message, + *determinism, + ); + InternalOutput { gas_meter, storage_deposit: storage_meter.into_deposit(origin), result } + } +} + +impl Invokable for InstantiateInput { + type Output = InternalOutput, ExecReturnValue)>; + + /// Internal function that does the actual contract instantiation. + /// + /// Called by dispatchables and public functions. + fn run(&self, mut debug_message: Option<&mut DebugBufferVec>) -> Self::Output { + let mut storage_deposit = Default::default(); + let mut gas_meter = GasMeter::new(self.gas_limit); + let try_exec = || { + let schedule = T::Schedule::get(); + let (extra_deposit, executable) = match &self.code { + Code::Upload(binary) => { + let executable = PrefabWasmModule::from_code( + binary.clone(), + &schedule, + self.origin.clone(), + Determinism::Deterministic, + TryInstantiate::Skip, + ) + .map_err(|(err, msg)| { + debug_message.as_mut().map(|buffer| buffer.try_extend(&mut msg.bytes())); + err + })?; + // The open deposit will be charged during execution when the + // uploaded module does not already exist. This deposit is not part of the + // storage meter because it is not transferred to the contract but + // reserved on the uploading account. + (executable.open_deposit(), executable) + }, + Code::Existing(hash) => ( + Default::default(), + PrefabWasmModule::from_storage(hash.clone(), &schedule, &mut gas_meter)?, + ), + }; + let mut storage_meter = StorageMeter::new( + &self.origin, + self.storage_deposit_limit, + self.value.saturating_add(extra_deposit), + )?; + + let InstantiateInput { origin, value, data, salt, .. } = self; + let result = ExecStack::>::run_instantiate( + origin.clone(), + executable, + &mut gas_meter, + &mut storage_meter, + &schedule, + value.clone(), + data.clone(), + &salt, + debug_message, + ); + storage_deposit = storage_meter + .into_deposit(&origin) + .saturating_add(&StorageDeposit::Charge(extra_deposit)); + result + }; + InternalOutput { result: try_exec(), gas_meter, storage_deposit } + } +} + impl Pallet { /// Perform a call to a specified contract. /// @@ -980,7 +1101,7 @@ impl Pallet { /// If set to `true` it returns additional human readable debugging information. /// /// It returns the execution result and the amount of used weight. - pub fn bare_call( + pub fn bare_call<'a>( origin: T::AccountId, dest: T::AccountId, value: BalanceOf, @@ -991,16 +1112,9 @@ impl Pallet { determinism: Determinism, ) -> ContractExecResult> { let mut debug_message = if debug { Some(DebugBufferVec::::default()) } else { None }; - let output = Self::internal_call( - origin, - dest, - value, - gas_limit, - storage_deposit_limit, - data, - debug_message.as_mut(), - determinism, - ); + let input = + CallInput { origin, dest, value, gas_limit, storage_deposit_limit, data, determinism }; + let output = Self::internal_run(input, debug_message.as_mut()); ContractExecResult { result: output.result.map_err(|r| r.error), gas_consumed: output.gas_meter.gas_consumed(), @@ -1033,16 +1147,9 @@ impl Pallet { debug: bool, ) -> ContractInstantiateResult> { let mut debug_message = if debug { Some(DebugBufferVec::::default()) } else { None }; - let output = Self::internal_instantiate( - origin, - value, - gas_limit, - storage_deposit_limit, - code, - data, - salt, - debug_message.as_mut(), - ); + let input = + InstantiateInput { origin, value, gas_limit, storage_deposit_limit, code, data, salt }; + let output = Self::internal_run(input, debug_message.as_mut()); ContractInstantiateResult { result: output .result @@ -1132,111 +1239,22 @@ impl Pallet { self::wasm::reinstrument(module, schedule).map(|_| ()) } - /// Internal function that does the actual call. - /// - /// Called by dispatchables and public functions. - fn internal_call( - origin: T::AccountId, - dest: T::AccountId, - value: BalanceOf, - gas_limit: Weight, - storage_deposit_limit: Option>, - data: Vec, + /// Single entry point to contract execution. + /// Downstream execution flow is branched by implementations of [`Ivokable`] trait: + /// - [`InstantiateInput::run`] runs contract instantiation, + /// - [`CallInput::run`] runs contract call. + /// TODO check and polish docs + fn internal_run>( + input: I, debug_message: Option<&mut DebugBufferVec>, - determinism: Determinism, - ) -> InternalCallOutput { - let mut gas_meter = GasMeter::new(gas_limit); - let mut storage_meter = match StorageMeter::new(&origin, storage_deposit_limit, value) { - Ok(meter) => meter, - Err(err) => - return InternalCallOutput { - result: Err(err.into()), - gas_meter, - storage_deposit: Default::default(), - }, - }; - let schedule = T::Schedule::get(); - let result = ExecStack::>::run_call( - origin.clone(), - dest, - &mut gas_meter, - &mut storage_meter, - &schedule, - value, - data, - debug_message, - determinism, - ); - InternalCallOutput { - result, - gas_meter, - storage_deposit: storage_meter.into_deposit(&origin), - } - } + ) -> I::Output { + // check global here + // set global here - /// Internal function that does the actual instantiation. - /// - /// Called by dispatchables and public functions. - fn internal_instantiate( - origin: T::AccountId, - value: BalanceOf, - gas_limit: Weight, - storage_deposit_limit: Option>, - code: Code>, - data: Vec, - salt: Vec, - mut debug_message: Option<&mut DebugBufferVec>, - ) -> InternalInstantiateOutput { - let mut storage_deposit = Default::default(); - let mut gas_meter = GasMeter::new(gas_limit); - let try_exec = || { - let schedule = T::Schedule::get(); - let (extra_deposit, executable) = match code { - Code::Upload(binary) => { - let executable = PrefabWasmModule::from_code( - binary, - &schedule, - origin.clone(), - Determinism::Deterministic, - TryInstantiate::Skip, - ) - .map_err(|(err, msg)| { - debug_message.as_mut().map(|buffer| buffer.try_extend(&mut msg.bytes())); - err - })?; - // The open deposit will be charged during execution when the - // uploaded module does not already exist. This deposit is not part of the - // storage meter because it is not transferred to the contract but - // reserved on the uploading account. - (executable.open_deposit(), executable) - }, - Code::Existing(hash) => ( - Default::default(), - PrefabWasmModule::from_storage(hash, &schedule, &mut gas_meter)?, - ), - }; - let mut storage_meter = StorageMeter::new( - &origin, - storage_deposit_limit, - value.saturating_add(extra_deposit), - )?; - let result = ExecStack::>::run_instantiate( - origin.clone(), - executable, - &mut gas_meter, - &mut storage_meter, - &schedule, - value, - data, - &salt, - debug_message, - ); - storage_deposit = storage_meter - .into_deposit(&origin) - .saturating_add(&StorageDeposit::Charge(extra_deposit)); - result - }; - InternalInstantiateOutput { result: try_exec(), gas_meter, storage_deposit } + // do stuff + input.run(debug_message) + + // set global here } /// Deposit a pallet contracts event. Handles the conversion to the overarching event type. From b2b7538c6eba4b3c48a322a3d08f9f9e2595f39b Mon Sep 17 00:00:00 2001 From: Alexander Gryaznov Date: Wed, 22 Feb 2023 13:29:40 +0200 Subject: [PATCH 02/17] save: added global --- Cargo.lock | 1 + frame/contracts/Cargo.toml | 1 + frame/contracts/src/lib.rs | 28 +++++++++++++++++++++++----- 3 files changed, 25 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index bed09f135de32..41d74b1290776 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5608,6 +5608,7 @@ dependencies = [ "assert_matches", "bitflags", "env_logger 0.9.3", + "environmental", "frame-benchmarking", "frame-support", "frame-system", diff --git a/frame/contracts/Cargo.toml b/frame/contracts/Cargo.toml index cd41ae2e39b55..a31c61c3c863e 100644 --- a/frame/contracts/Cargo.toml +++ b/frame/contracts/Cargo.toml @@ -35,6 +35,7 @@ rand = { version = "0.8", optional = true, default-features = false } rand_pcg = { version = "0.3", optional = true } # Substrate Dependencies +environmental = { version = "1.1.4", default-features = false } frame-benchmarking = { version = "4.0.0-dev", default-features = false, path = "../benchmarking", optional = true } frame-support = { version = "4.0.0-dev", default-features = false, path = "../support" } frame-system = { version = "4.0.0-dev", default-features = false, path = "../system" } diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 741c7b9fb7bd1..f017d728b0591 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -107,6 +107,7 @@ use crate::{ weights::WeightInfo, }; use codec::{Codec, Encode, HasCompact}; +use environmental::*; use frame_support::{ dispatch::{Dispatchable, GetDispatchInfo, Pays, PostDispatchInfo}, ensure, @@ -125,6 +126,7 @@ use pallet_contracts_primitives::{ }; use scale_info::TypeInfo; use smallvec::Array; +use sp_core::defer; use sp_runtime::traits::{Convert, Hash, Saturating, StaticLookup}; use sp_std::{fmt::Debug, marker::PhantomData, prelude::*}; @@ -157,6 +159,10 @@ type DebugBufferVec = BoundedVec::MaxDebugBufferLen>; /// that this value makes sense for a memory location or length. const SENTINEL: u32 = u32::MAX; +// TODO doc +// TODO think on renaming to reentrancy_guard +environmental!(executing_contract: bool); + #[frame_support::pallet] pub mod pallet { use super::*; @@ -1248,13 +1254,25 @@ impl Pallet { input: I, debug_message: Option<&mut DebugBufferVec>, ) -> I::Output { - // check global here - // set global here - + // check global here: fail if trying to re-enter + executing_contract::using(&mut false, || { + executing_contract::with(|f| { + if *f { + return Err::<(), Error>(>::ContractNotFound.into()) // TODO: new Err + } + Ok(()) + }) + .unwrap() + }) + .unwrap(); // TODO: refactor + // we are entering the contract: set global here + executing_contract::using(&mut false, || executing_contract::with(|f| *f = true)); + // set global back when exiting the contract + defer! { + executing_contract::with(|f| *f = false); + }; // do stuff input.run(debug_message) - - // set global here } /// Deposit a pallet contracts event. Handles the conversion to the overarching event type. From e23bc131790016e6768b8683c73ec66a92659532 Mon Sep 17 00:00:00 2001 From: Alexander Gryaznov Date: Wed, 22 Feb 2023 20:51:22 +0200 Subject: [PATCH 03/17] done + test --- frame/contracts/src/lib.rs | 107 ++++++++++++++++++++++++----------- frame/contracts/src/tests.rs | 34 ++++------- 2 files changed, 84 insertions(+), 57 deletions(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index f017d728b0591..9bd42256c9ab5 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -100,7 +100,7 @@ pub mod weights; mod tests; use crate::{ - exec::{AccountIdOf, ExecError, Executable, Stack as ExecStack}, + exec::{AccountIdOf, ErrorOrigin, ExecError, Executable, Stack as ExecStack}, gas::GasMeter, storage::{meter::Meter as StorageMeter, ContractInfo, DeletedContract}, wasm::{OwnerInfo, PrefabWasmModule, TryInstantiate}, @@ -159,9 +159,7 @@ type DebugBufferVec = BoundedVec::MaxDebugBufferLen>; /// that this value makes sense for a memory location or length. const SENTINEL: u32 = u32::MAX; -// TODO doc -// TODO think on renaming to reentrancy_guard -environmental!(executing_contract: bool); +environmental!(reentrancy_guard: bool); #[frame_support::pallet] pub mod pallet { @@ -637,6 +635,7 @@ pub mod pallet { output.result = Err(>::ContractReverted.into()); } } + // println!(": result = {:?}", &output.result); output.gas_meter.into_dispatch_result(output.result, T::WeightInfo::call()) } @@ -878,6 +877,7 @@ pub mod pallet { /// This can be triggered by a call to `seal_terminate`. TerminatedInConstructor, /// A call tried to invoke a contract that is flagged as non-reentrant. + /// Or, a call to runtime tried to re-enter a contract. ReentranceDenied, /// Origin doesn't have enough balance to pay the required storage deposits. StorageDepositNotEnoughFunds, @@ -957,7 +957,7 @@ pub mod pallet { StorageValue<_, BoundedVec, ValueQuery>; } -/// TODO: put into primitives +/// Input of a call into contract. struct CallInput { origin: T::AccountId, dest: T::AccountId, @@ -968,7 +968,7 @@ struct CallInput { determinism: Determinism, } -/// TODO: put into primitives +/// Input of a contract instantiation invocation. struct InstantiateInput { origin: T::AccountId, value: BalanceOf, @@ -989,22 +989,40 @@ struct InternalOutput { result: Result, } -/// TODO: put into primitives -/// Helper trait to wrap contract execution entry points into a signle function [`internal_run`] +/// Helper trait to wrap contract execution entry points into a signle function [`Pallet::internal_run`]. trait Invokable { type Output; - fn run(&self, debug_message: Option<&mut DebugBufferVec>) -> Self::Output; + fn run( + &self, + debug_message: Option<&mut DebugBufferVec>, + gas_meter: GasMeter, + ) -> Self::Output; + + fn fallback(&self, err: ExecError, gas_meter: GasMeter) -> Self::Output; + + fn gas_limit(&self) -> Weight; } impl Invokable for CallInput { type Output = InternalOutput; - /// Internal function that does the actual call to contract. + fn gas_limit(&self) -> Weight { + self.gas_limit + } + + fn fallback(&self, err: ExecError, gas_meter: GasMeter) -> Self::Output { + InternalOutput { gas_meter, storage_deposit: Default::default(), result: Err(err) } + } + + /// Method that does the actual call to contract. /// - /// Called by dispatchables and public functions. - fn run(&self, debug_message: Option<&mut DebugBufferVec>) -> Self::Output { - let mut gas_meter = GasMeter::new(self.gas_limit); + /// Called by dispatchables and public functions through the [`Pallet::internal_run`]. + fn run( + &self, + debug_message: Option<&mut DebugBufferVec>, + mut gas_meter: GasMeter, + ) -> Self::Output { let mut storage_meter = match StorageMeter::new(&self.origin, self.storage_deposit_limit, self.value) { Ok(meter) => meter, @@ -1035,12 +1053,22 @@ impl Invokable for CallInput { impl Invokable for InstantiateInput { type Output = InternalOutput, ExecReturnValue)>; - /// Internal function that does the actual contract instantiation. + fn gas_limit(&self) -> Weight { + self.gas_limit + } + + fn fallback(&self, err: ExecError, gas_meter: GasMeter) -> Self::Output { + InternalOutput { gas_meter, storage_deposit: Default::default(), result: Err(err) } + } + /// Method that does the actual contract instantiation. /// - /// Called by dispatchables and public functions. - fn run(&self, mut debug_message: Option<&mut DebugBufferVec>) -> Self::Output { + /// Called by dispatchables and public functions through the [`Pallet::internal_run`]. + fn run( + &self, + mut debug_message: Option<&mut DebugBufferVec>, + mut gas_meter: GasMeter, + ) -> Self::Output { let mut storage_deposit = Default::default(); - let mut gas_meter = GasMeter::new(self.gas_limit); let try_exec = || { let schedule = T::Schedule::get(); let (extra_deposit, executable) = match &self.code { @@ -1246,33 +1274,46 @@ impl Pallet { } /// Single entry point to contract execution. - /// Downstream execution flow is branched by implementations of [`Ivokable`] trait: + /// Downstream execution flow is branched by implementations of [`Invokable`] trait: + /// /// - [`InstantiateInput::run`] runs contract instantiation, /// - [`CallInput::run`] runs contract call. - /// TODO check and polish docs + /// + /// We enforce a re-entrancy guard here by initializing and checking a boolean flag through a + /// global reference. fn internal_run>( input: I, debug_message: Option<&mut DebugBufferVec>, ) -> I::Output { - // check global here: fail if trying to re-enter - executing_contract::using(&mut false, || { - executing_contract::with(|f| { + let gas_meter = GasMeter::new(input.gas_limit()); + reentrancy_guard::using_once(&mut false, || { + let res = reentrancy_guard::with(|f| { + // Check re-entrancy guard if *f { - return Err::<(), Error>(>::ContractNotFound.into()) // TODO: new Err + return Err(()) } + // Set re-entracy guard + *f = true; Ok(()) }) - .unwrap() + .unwrap_or(Ok(())); + + // Remove re-entrancy guard when dropping this scope + defer! { + reentrancy_guard::with(|f| {println!("NOW F IS {:?}", f); *f = false}); + }; + + if res.is_err() { + let err = ExecError { + error: >::ReentranceDenied.into(), + origin: ErrorOrigin::Caller, + }; + input.fallback(err, gas_meter) + } else { + // Enter the contract call + input.run(debug_message, gas_meter) + } }) - .unwrap(); // TODO: refactor - // we are entering the contract: set global here - executing_contract::using(&mut false, || executing_contract::with(|f| *f = true)); - // set global back when exiting the contract - defer! { - executing_contract::with(|f| *f = false); - }; - // do stuff - input.run(debug_message) } /// Deposit a pallet contracts event. Handles the conversion to the overarching event type. diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index 879268a68dca9..5a1b001e82470 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -2766,8 +2766,7 @@ fn gas_estimation_nested_call_fixed_limit() { } #[test] -fn gas_estimation_call_runtime() { - use codec::Decode; +fn gas_call_runtime_reentrancy_guarded() { let (caller_code, _caller_hash) = compile_module::("call_runtime").unwrap(); let (callee_code, _callee_hash) = compile_module::("dummy").unwrap(); ExtBuilder::default().existential_deposit(50).build().execute_with(|| { @@ -2803,8 +2802,7 @@ fn gas_estimation_call_runtime() { .unwrap() .account_id; - // Call something trivial with a huge gas limit so that we can observe the effects - // of pre-charging. This should create a difference between consumed and required. + // Call pallet_contracts call() dispatchable let call = RuntimeCall::Contracts(crate::Call::call { dest: addr_callee, value: 0, @@ -2812,6 +2810,9 @@ fn gas_estimation_call_runtime() { storage_deposit_limit: None, data: vec![], }); + + // Call to call_runtime contract which calls runtime to re-enter contracts stack by + // calling dummy contract let result = Contracts::bare_call( ALICE, addr_caller.clone(), @@ -2821,26 +2822,11 @@ fn gas_estimation_call_runtime() { call.encode(), false, Determinism::Deterministic, - ); - // contract encodes the result of the dispatch runtime - let outcome = u32::decode(&mut result.result.unwrap().data.as_ref()).unwrap(); - assert_eq!(outcome, 0); - assert!(result.gas_required.ref_time() > result.gas_consumed.ref_time()); - - // Make the same call using the required gas. Should succeed. - assert_ok!( - Contracts::bare_call( - ALICE, - addr_caller, - 0, - result.gas_required, - None, - call.encode(), - false, - Determinism::Deterministic, - ) - .result - ); + ) + .result + .unwrap(); + // Call to runtime should fail because of the re-entrancy guard + assert_return_code!(result, RuntimeReturnCode::CallRuntimeFailed); }); } From 5ce957d7b3e37d7318c9eee6b5046aa952709080 Mon Sep 17 00:00:00 2001 From: Alexander Gryaznov Date: Wed, 22 Feb 2023 21:44:58 +0200 Subject: [PATCH 04/17] cleanup --- frame/contracts/src/lib.rs | 6 +++--- frame/contracts/src/tests.rs | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 8934a8e977ee4..9520d8a06968e 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -635,7 +635,6 @@ pub mod pallet { output.result = Err(>::ContractReverted.into()); } } - // println!(": result = {:?}", &output.result); output.gas_meter.into_dispatch_result(output.result, T::WeightInfo::call()) } @@ -989,7 +988,8 @@ struct InternalOutput { result: Result, } -/// Helper trait to wrap contract execution entry points into a signle function [`Pallet::internal_run`]. +/// Helper trait to wrap contract execution entry points into a signle function +/// [`Pallet::internal_run`]. trait Invokable { type Output; @@ -1300,7 +1300,7 @@ impl Pallet { // Remove re-entrancy guard when dropping this scope defer! { - reentrancy_guard::with(|f| {println!("NOW F IS {:?}", f); *f = false}); + reentrancy_guard::with(|f| *f = false); }; if res.is_err() { diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index efe4d1cdd3d03..757a462e35d24 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -2811,7 +2811,7 @@ fn gas_call_runtime_reentrancy_guarded() { data: vec![], }); - // Call to call_runtime contract which calls runtime to re-enter contracts stack by + // Call runtime to re-enter back to contracts engine by // calling dummy contract let result = Contracts::bare_call( ALICE, From b183f27d3eeee5e0e1031b637034862cd2d576bf Mon Sep 17 00:00:00 2001 From: Alexander Gryaznov Date: Wed, 22 Feb 2023 21:47:46 +0200 Subject: [PATCH 05/17] changelog update --- frame/contracts/CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/frame/contracts/CHANGELOG.md b/frame/contracts/CHANGELOG.md index ab3998e6dc4f7..dcb9d6d4d2b20 100644 --- a/frame/contracts/CHANGELOG.md +++ b/frame/contracts/CHANGELOG.md @@ -20,6 +20,9 @@ In other words: Upgrading this pallet will not break pre-existing contracts. ### Added +- Forbid calling back to contracts after switching to runtime +[#13443](https://github.com/paritytech/substrate/pull/13443) + - Allow contracts to dispatch calls into the runtime (**unstable**) [#9276](https://github.com/paritytech/substrate/pull/9276) From 8e6b01cf31daf65c4f1f63c46a101c3dba7f36c5 Mon Sep 17 00:00:00 2001 From: Alexander Gryaznov Date: Wed, 22 Feb 2023 21:50:11 +0200 Subject: [PATCH 06/17] cleanup --- frame/contracts/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 9520d8a06968e..90ba238ad1b2e 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -1135,7 +1135,7 @@ impl Pallet { /// If set to `true` it returns additional human readable debugging information. /// /// It returns the execution result and the amount of used weight. - pub fn bare_call<'a>( + pub fn bare_call( origin: T::AccountId, dest: T::AccountId, value: BalanceOf, From 39913e76e8199afa755127f230058539a209cc0b Mon Sep 17 00:00:00 2001 From: Alexander Gryaznov Date: Fri, 24 Feb 2023 13:47:49 +0200 Subject: [PATCH 07/17] address feedback, step 1 --- frame/contracts/Cargo.toml | 1 + frame/contracts/src/lib.rs | 22 +++++++++------------- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/frame/contracts/Cargo.toml b/frame/contracts/Cargo.toml index a31c61c3c863e..e19326b785b2b 100644 --- a/frame/contracts/Cargo.toml +++ b/frame/contracts/Cargo.toml @@ -81,6 +81,7 @@ std = [ "log/std", "rand/std", "wasmparser/std", + "environmental/std", ] runtime-benchmarks = [ "frame-benchmarking/runtime-benchmarks", diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 90ba238ad1b2e..f6deff0813ea1 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -126,7 +126,6 @@ use pallet_contracts_primitives::{ }; use scale_info::TypeInfo; use smallvec::Array; -use sp_core::defer; use sp_runtime::traits::{Convert, Hash, Saturating, StaticLookup}; use sp_std::{fmt::Debug, marker::PhantomData, prelude::*}; @@ -158,8 +157,8 @@ type DebugBufferVec = BoundedVec::MaxDebugBufferLen>; /// sentinel because contracts are never allowed to use such a large amount of resources /// that this value makes sense for a memory location or length. const SENTINEL: u32 = u32::MAX; - -environmental!(reentrancy_guard: bool); +// Set up a global reference to the boolean flag used for the re-entrancy guard. +environmental!(executing_contract: bool); #[frame_support::pallet] pub mod pallet { @@ -876,7 +875,9 @@ pub mod pallet { /// This can be triggered by a call to `seal_terminate`. TerminatedInConstructor, /// A call tried to invoke a contract that is flagged as non-reentrant. - /// Or, a call to runtime tried to re-enter a contract. + /// The only other cause is that a call from a contract into the runtime tried to call back + /// into `pallet-contracts`. This would make the whole pallet reentrant with regard to + /// contract code execution which is not supported. ReentranceDenied, /// Origin doesn't have enough balance to pay the required storage deposits. StorageDepositNotEnoughFunds, @@ -1286,23 +1287,18 @@ impl Pallet { debug_message: Option<&mut DebugBufferVec>, ) -> I::Output { let gas_meter = GasMeter::new(input.gas_limit()); - reentrancy_guard::using_once(&mut false, || { - let res = reentrancy_guard::with(|f| { - // Check re-entrancy guard + executing_contract::using_once(&mut false, || { + let res = executing_contract::with(|f| { + // Fail if already entered contract execution if *f { return Err(()) } - // Set re-entracy guard + // We are entering contract execution *f = true; Ok(()) }) .unwrap_or(Ok(())); - // Remove re-entrancy guard when dropping this scope - defer! { - reentrancy_guard::with(|f| *f = false); - }; - if res.is_err() { let err = ExecError { error: >::ReentranceDenied.into(), From 795314934165140bd0819129ec738ccab948285e Mon Sep 17 00:00:00 2001 From: Alexander Gryaznov Date: Fri, 24 Feb 2023 15:05:58 +0200 Subject: [PATCH 08/17] address feedback, step 2 --- frame/contracts/src/lib.rs | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index f6deff0813ea1..72355676b36e1 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -994,6 +994,7 @@ struct InternalOutput { trait Invokable { type Output; + /// Invoke a contract execution. fn run( &self, debug_message: Option<&mut DebugBufferVec>, @@ -1286,9 +1287,8 @@ impl Pallet { input: I, debug_message: Option<&mut DebugBufferVec>, ) -> I::Output { - let gas_meter = GasMeter::new(input.gas_limit()); executing_contract::using_once(&mut false, || { - let res = executing_contract::with(|f| { + executing_contract::with(|f| { // Fail if already entered contract execution if *f { return Err(()) @@ -1297,18 +1297,20 @@ impl Pallet { *f = true; Ok(()) }) - .unwrap_or(Ok(())); - - if res.is_err() { - let err = ExecError { - error: >::ReentranceDenied.into(), - origin: ErrorOrigin::Caller, - }; - input.fallback(err, gas_meter) - } else { - // Enter the contract call - input.run(debug_message, gas_meter) - } + .unwrap_or(Ok(())) // Should always be `Some`, still we make it safe. + .map_or_else( + |_| { + input.fallback( + ExecError { + error: >::ReentranceDenied.into(), + origin: ErrorOrigin::Caller, + }, + GasMeter::new(input.gas_limit()), + ) + }, + // Enter contract call. + |_| input.run(debug_message, GasMeter::new(input.gas_limit())), + ) }) } From 187520483a72e50800c8ffd3568b6edad93daa9f Mon Sep 17 00:00:00 2001 From: Alexander Gryaznov Date: Fri, 24 Feb 2023 15:26:03 +0200 Subject: [PATCH 09/17] address feedback, step 3 --- frame/contracts/src/lib.rs | 232 ++++++++++++++++++------------------- 1 file changed, 110 insertions(+), 122 deletions(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 72355676b36e1..d9c07204d2c56 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -619,16 +619,16 @@ pub mod pallet { let gas_limit: Weight = gas_limit.into(); let origin = ensure_signed(origin)?; let dest = T::Lookup::lookup(dest)?; - let input = CallInput { + let context = ExecContext { origin, - dest, value, gas_limit, storage_deposit_limit: storage_deposit_limit.map(Into::into), data, - determinism: Determinism::Deterministic, + debug_message: None, }; - let mut output = Self::internal_run(input, None); + let mut output = CallInput:: { dest, determinism: Determinism::Deterministic } + .run_guarded(context); if let Ok(retval) = &output.result { if retval.did_revert() { output.result = Err(>::ContractReverted.into()); @@ -681,16 +681,16 @@ pub mod pallet { let code_len = code.len() as u32; let data_len = data.len() as u32; let salt_len = salt.len() as u32; - let input = InstantiateInput { + let context = ExecContext { origin, value, gas_limit, storage_deposit_limit: storage_deposit_limit.map(Into::into), - code: Code::Upload(code), data, - salt, + debug_message: None, }; - let mut output = Self::internal_run(input, None); + let mut output = + InstantiateInput:: { code: Code::Upload(code), salt }.run_guarded(context); if let Ok(retval) = &output.result { if retval.1.did_revert() { output.result = Err(>::ContractReverted.into()); @@ -723,16 +723,16 @@ pub mod pallet { let origin = ensure_signed(origin)?; let data_len = data.len() as u32; let salt_len = salt.len() as u32; - let input = InstantiateInput { + let context = ExecContext { origin, value, gas_limit, storage_deposit_limit: storage_deposit_limit.map(Into::into), - code: Code::Existing(code_hash), data, - salt, + debug_message: None, }; - let mut output = Self::internal_run(input, None); + let mut output = InstantiateInput:: { code: Code::Existing(code_hash), salt } + .run_guarded(context); if let Ok(retval) = &output.result { if retval.1.did_revert() { output.result = Err(>::ContractReverted.into()); @@ -957,25 +957,25 @@ pub mod pallet { StorageValue<_, BoundedVec, ValueQuery>; } -/// Input of a call into contract. -struct CallInput { +/// Context of a contract invocation. +struct ExecContext<'a, T: Config> { origin: T::AccountId, - dest: T::AccountId, value: BalanceOf, gas_limit: Weight, storage_deposit_limit: Option>, data: Vec, + debug_message: Option<&'a mut DebugBufferVec>, +} + +/// Input specific to a call into contract. +struct CallInput { + dest: T::AccountId, determinism: Determinism, } -/// Input of a contract instantiation invocation. +/// Input specific to a contract instantiation invocation. struct InstantiateInput { - origin: T::AccountId, - value: BalanceOf, - gas_limit: Weight, - storage_deposit_limit: Option>, code: Code>, - data: Vec, salt: Vec, } @@ -990,86 +990,97 @@ struct InternalOutput { } /// Helper trait to wrap contract execution entry points into a signle function -/// [`Pallet::internal_run`]. -trait Invokable { - type Output; +/// [`Invokable::run_guarded`]. +trait Invokable { + /// Single entry point to contract execution. + /// Downstream execution flow is branched by implementations of [`Invokable`] trait: + /// + /// - [`InstantiateInput::run`] runs contract instantiation, + /// - [`CallInput::run`] runs contract call. + /// + /// We enforce a re-entrancy guard here by initializing and checking a boolean flag through a + /// global reference. + fn run_guarded(&self, context: ExecContext) -> InternalOutput { + let gas_limit = context.gas_limit; + executing_contract::using_once(&mut false, || { + executing_contract::with(|f| { + // Fail if already entered contract execution + if *f { + return Err(()) + } + // We are entering contract execution + *f = true; + Ok(()) + }) + .unwrap_or(Ok(())) // Should always be `Some`, still we make it safe. + .map_or_else( + |_| InternalOutput { + gas_meter: GasMeter::new(gas_limit.clone()), + storage_deposit: Default::default(), + result: Err(ExecError { + error: >::ReentranceDenied.into(), + origin: ErrorOrigin::Caller, + }), + }, + // Enter contract call. + |_| self.run(context, GasMeter::new(gas_limit)), + ) + }) + } /// Invoke a contract execution. - fn run( - &self, - debug_message: Option<&mut DebugBufferVec>, - gas_meter: GasMeter, - ) -> Self::Output; - - fn fallback(&self, err: ExecError, gas_meter: GasMeter) -> Self::Output; - - fn gas_limit(&self) -> Weight; + fn run(&self, context: ExecContext, gas_meter: GasMeter) -> InternalOutput; } -impl Invokable for CallInput { - type Output = InternalOutput; - - fn gas_limit(&self) -> Weight { - self.gas_limit - } - - fn fallback(&self, err: ExecError, gas_meter: GasMeter) -> Self::Output { - InternalOutput { gas_meter, storage_deposit: Default::default(), result: Err(err) } - } - +impl Invokable for CallInput { /// Method that does the actual call to contract. /// /// Called by dispatchables and public functions through the [`Pallet::internal_run`]. fn run( &self, - debug_message: Option<&mut DebugBufferVec>, + context: ExecContext, mut gas_meter: GasMeter, - ) -> Self::Output { - let mut storage_meter = - match StorageMeter::new(&self.origin, self.storage_deposit_limit, self.value) { - Ok(meter) => meter, - Err(err) => - return InternalOutput { - result: Err(err.into()), - gas_meter, - storage_deposit: Default::default(), - }, - }; + ) -> InternalOutput { + let mut storage_meter = match StorageMeter::new( + &context.origin, + context.storage_deposit_limit, + context.value, + ) { + Ok(meter) => meter, + Err(err) => + return InternalOutput { + result: Err(err.into()), + gas_meter, + storage_deposit: Default::default(), + }, + }; let schedule = T::Schedule::get(); - let CallInput { origin, dest, value, data, determinism, .. } = self; + let CallInput { dest, determinism } = self; + let ExecContext { origin, value, data, debug_message, .. } = context; let result = ExecStack::>::run_call( origin.clone(), dest.clone(), &mut gas_meter, &mut storage_meter, &schedule, - *value, + value, data.clone(), debug_message, *determinism, ); - InternalOutput { gas_meter, storage_deposit: storage_meter.into_deposit(origin), result } + InternalOutput { gas_meter, storage_deposit: storage_meter.into_deposit(&origin), result } } } -impl Invokable for InstantiateInput { - type Output = InternalOutput, ExecReturnValue)>; - - fn gas_limit(&self) -> Weight { - self.gas_limit - } - - fn fallback(&self, err: ExecError, gas_meter: GasMeter) -> Self::Output { - InternalOutput { gas_meter, storage_deposit: Default::default(), result: Err(err) } - } +impl Invokable, ExecReturnValue)> for InstantiateInput { /// Method that does the actual contract instantiation. /// /// Called by dispatchables and public functions through the [`Pallet::internal_run`]. fn run( &self, - mut debug_message: Option<&mut DebugBufferVec>, + mut context: ExecContext, mut gas_meter: GasMeter, - ) -> Self::Output { + ) -> InternalOutput, ExecReturnValue)> { let mut storage_deposit = Default::default(); let try_exec = || { let schedule = T::Schedule::get(); @@ -1078,12 +1089,15 @@ impl Invokable for InstantiateInput { let executable = PrefabWasmModule::from_code( binary.clone(), &schedule, - self.origin.clone(), + context.origin.clone(), Determinism::Deterministic, TryInstantiate::Skip, ) .map_err(|(err, msg)| { - debug_message.as_mut().map(|buffer| buffer.try_extend(&mut msg.bytes())); + context + .debug_message + .as_mut() + .map(|buffer| buffer.try_extend(&mut msg.bytes())); err })?; // The open deposit will be charged during execution when the @@ -1098,12 +1112,13 @@ impl Invokable for InstantiateInput { ), }; let mut storage_meter = StorageMeter::new( - &self.origin, - self.storage_deposit_limit, - self.value.saturating_add(extra_deposit), + &context.origin, + context.storage_deposit_limit, + context.value.saturating_add(extra_deposit), )?; - let InstantiateInput { origin, value, data, salt, .. } = self; + let InstantiateInput { salt, .. } = self; + let ExecContext { origin, value, data, debug_message, .. } = context; let result = ExecStack::>::run_instantiate( origin.clone(), executable, @@ -1148,9 +1163,15 @@ impl Pallet { determinism: Determinism, ) -> ContractExecResult> { let mut debug_message = if debug { Some(DebugBufferVec::::default()) } else { None }; - let input = - CallInput { origin, dest, value, gas_limit, storage_deposit_limit, data, determinism }; - let output = Self::internal_run(input, debug_message.as_mut()); + let context = ExecContext { + origin, + value, + gas_limit, + storage_deposit_limit, + data, + debug_message: debug_message.as_mut(), + }; + let output = CallInput:: { dest, determinism }.run_guarded(context); ContractExecResult { result: output.result.map_err(|r| r.error), gas_consumed: output.gas_meter.gas_consumed(), @@ -1183,9 +1204,15 @@ impl Pallet { debug: bool, ) -> ContractInstantiateResult> { let mut debug_message = if debug { Some(DebugBufferVec::::default()) } else { None }; - let input = - InstantiateInput { origin, value, gas_limit, storage_deposit_limit, code, data, salt }; - let output = Self::internal_run(input, debug_message.as_mut()); + let context = ExecContext { + origin, + value, + gas_limit, + storage_deposit_limit, + data, + debug_message: debug_message.as_mut(), + }; + let output = InstantiateInput:: { code, salt }.run_guarded(context); ContractInstantiateResult { result: output .result @@ -1275,45 +1302,6 @@ impl Pallet { self::wasm::reinstrument(module, schedule).map(|_| ()) } - /// Single entry point to contract execution. - /// Downstream execution flow is branched by implementations of [`Invokable`] trait: - /// - /// - [`InstantiateInput::run`] runs contract instantiation, - /// - [`CallInput::run`] runs contract call. - /// - /// We enforce a re-entrancy guard here by initializing and checking a boolean flag through a - /// global reference. - fn internal_run>( - input: I, - debug_message: Option<&mut DebugBufferVec>, - ) -> I::Output { - executing_contract::using_once(&mut false, || { - executing_contract::with(|f| { - // Fail if already entered contract execution - if *f { - return Err(()) - } - // We are entering contract execution - *f = true; - Ok(()) - }) - .unwrap_or(Ok(())) // Should always be `Some`, still we make it safe. - .map_or_else( - |_| { - input.fallback( - ExecError { - error: >::ReentranceDenied.into(), - origin: ErrorOrigin::Caller, - }, - GasMeter::new(input.gas_limit()), - ) - }, - // Enter contract call. - |_| input.run(debug_message, GasMeter::new(input.gas_limit())), - ) - }) - } - /// Deposit a pallet contracts event. Handles the conversion to the overarching event type. fn deposit_event(topics: Vec, event: Event) { >::deposit_event_indexed( From 0a83f289960d619f2bd7b774df5964d7569ea179 Mon Sep 17 00:00:00 2001 From: Alexander Gryaznov Date: Fri, 24 Feb 2023 18:13:13 +0200 Subject: [PATCH 10/17] returned updated gas_estimation_call_runtime test --- frame/contracts/src/tests.rs | 76 ++++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index 757a462e35d24..7144dcf2b5d8c 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -2765,6 +2765,82 @@ fn gas_estimation_nested_call_fixed_limit() { }); } +#[test] +fn gas_estimation_call_runtime() { + use codec::Decode; + let (caller_code, _caller_hash) = compile_module::("call_runtime").unwrap(); + let (callee_code, _callee_hash) = compile_module::("dummy").unwrap(); + ExtBuilder::default().existential_deposit(50).build().execute_with(|| { + let min_balance = ::Currency::minimum_balance(); + let _ = Balances::deposit_creating(&ALICE, 1000 * min_balance); + let _ = Balances::deposit_creating(&CHARLIE, 1000 * min_balance); + + let addr_caller = Contracts::bare_instantiate( + ALICE, + min_balance * 100, + GAS_LIMIT, + None, + Code::Upload(caller_code), + vec![], + vec![0], + false, + ) + .result + .unwrap() + .account_id; + + let addr_callee = Contracts::bare_instantiate( + ALICE, + min_balance * 100, + GAS_LIMIT, + None, + Code::Upload(callee_code), + vec![], + vec![1], + false, + ) + .result + .unwrap() + .account_id; + + // Call something trivial with a huge gas limit so that we can observe the effects + // of pre-charging. This should create a difference between consumed and required. + let call = RuntimeCall::Balances(pallet_balances::Call::transfer { + dest: addr_callee, + value: min_balance * 10, + }); + let result = Contracts::bare_call( + ALICE, + addr_caller.clone(), + 0, + GAS_LIMIT, + None, + call.encode(), + false, + Determinism::Deterministic, + ); + // contract encodes the result of the dispatch runtime + let outcome = u32::decode(&mut result.result.unwrap().data.as_ref()).unwrap(); + assert_eq!(outcome, 0); + assert!(result.gas_required.ref_time() > result.gas_consumed.ref_time()); + + // Make the same call using the required gas. Should succeed. + assert_ok!( + Contracts::bare_call( + ALICE, + addr_caller, + 0, + result.gas_required, + None, + call.encode(), + false, + Determinism::Deterministic, + ) + .result + ); + }); +} + #[test] fn gas_call_runtime_reentrancy_guarded() { let (caller_code, _caller_hash) = compile_module::("call_runtime").unwrap(); From cf045786adbf732e0f44ddd485578767ac85eb5c Mon Sep 17 00:00:00 2001 From: Alexander Gryaznov Date: Sat, 25 Feb 2023 00:52:09 +0200 Subject: [PATCH 11/17] clippy fix --- frame/contracts/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index d9c07204d2c56..56a19c03688e7 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -1015,7 +1015,7 @@ trait Invokable { .unwrap_or(Ok(())) // Should always be `Some`, still we make it safe. .map_or_else( |_| InternalOutput { - gas_meter: GasMeter::new(gas_limit.clone()), + gas_meter: GasMeter::new(gas_limit), storage_deposit: Default::default(), result: Err(ExecError { error: >::ReentranceDenied.into(), @@ -1108,7 +1108,7 @@ impl Invokable, ExecReturnValue)> for InstantiateI }, Code::Existing(hash) => ( Default::default(), - PrefabWasmModule::from_storage(hash.clone(), &schedule, &mut gas_meter)?, + PrefabWasmModule::from_storage(*hash, &schedule, &mut gas_meter)?, ), }; let mut storage_meter = StorageMeter::new( @@ -1125,7 +1125,7 @@ impl Invokable, ExecReturnValue)> for InstantiateI &mut gas_meter, &mut storage_meter, &schedule, - value.clone(), + value, data.clone(), &salt, debug_message, From fc44bcf55ba2146a9c81b88450dca7ec74d0913b Mon Sep 17 00:00:00 2001 From: Alexander Gryaznov Date: Thu, 2 Mar 2023 14:42:31 +0200 Subject: [PATCH 12/17] address feedback, step 4 --- frame/contracts/src/lib.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 56a19c03688e7..99d23a34d68bb 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -157,8 +157,6 @@ type DebugBufferVec = BoundedVec::MaxDebugBufferLen>; /// sentinel because contracts are never allowed to use such a large amount of resources /// that this value makes sense for a memory location or length. const SENTINEL: u32 = u32::MAX; -// Set up a global reference to the boolean flag used for the re-entrancy guard. -environmental!(executing_contract: bool); #[frame_support::pallet] pub mod pallet { @@ -1001,6 +999,9 @@ trait Invokable { /// We enforce a re-entrancy guard here by initializing and checking a boolean flag through a /// global reference. fn run_guarded(&self, context: ExecContext) -> InternalOutput { + // Set up a global reference to the boolean flag used for the re-entrancy guard. + environmental!(executing_contract: bool); + let gas_limit = context.gas_limit; executing_contract::using_once(&mut false, || { executing_contract::with(|f| { @@ -1012,7 +1013,7 @@ trait Invokable { *f = true; Ok(()) }) - .unwrap_or(Ok(())) // Should always be `Some`, still we make it safe. + .expect("Returns `Ok` if called within `using_once`. It is syntactically obvious that this is the case; qed") .map_or_else( |_| InternalOutput { gas_meter: GasMeter::new(gas_limit), From 3079b0e1c2d51bd94d673d1fbcb48314c0204dd5 Mon Sep 17 00:00:00 2001 From: Alexander Gryaznov Date: Thu, 2 Mar 2023 14:52:17 +0200 Subject: [PATCH 13/17] address feedback, step 5 --- frame/contracts/src/lib.rs | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 99d23a34d68bb..cb14dd691faa2 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -989,7 +989,9 @@ struct InternalOutput { /// Helper trait to wrap contract execution entry points into a signle function /// [`Invokable::run_guarded`]. -trait Invokable { +trait Invokable { + /// What is returned as a result of a succeed invocation. + type Output; /// Single entry point to contract execution. /// Downstream execution flow is branched by implementations of [`Invokable`] trait: /// @@ -998,7 +1000,7 @@ trait Invokable { /// /// We enforce a re-entrancy guard here by initializing and checking a boolean flag through a /// global reference. - fn run_guarded(&self, context: ExecContext) -> InternalOutput { + fn run_guarded(&self, context: ExecContext) -> InternalOutput { // Set up a global reference to the boolean flag used for the re-entrancy guard. environmental!(executing_contract: bool); @@ -1030,10 +1032,15 @@ trait Invokable { } /// Invoke a contract execution. - fn run(&self, context: ExecContext, gas_meter: GasMeter) -> InternalOutput; + fn run( + &self, + context: ExecContext, + gas_meter: GasMeter, + ) -> InternalOutput; } -impl Invokable for CallInput { +impl Invokable for CallInput { + type Output = ExecReturnValue; /// Method that does the actual call to contract. /// /// Called by dispatchables and public functions through the [`Pallet::internal_run`]. @@ -1073,7 +1080,8 @@ impl Invokable for CallInput { } } -impl Invokable, ExecReturnValue)> for InstantiateInput { +impl Invokable for InstantiateInput { + type Output = (AccountIdOf, ExecReturnValue); /// Method that does the actual contract instantiation. /// /// Called by dispatchables and public functions through the [`Pallet::internal_run`]. From aee4538d754a670bbdcc290efec048fe3b50ff48 Mon Sep 17 00:00:00 2001 From: Alexander Gryaznov Date: Thu, 2 Mar 2023 15:20:52 +0200 Subject: [PATCH 14/17] move data from context to inputs --- frame/contracts/src/lib.rs | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index cb14dd691faa2..8aa4deb6b9d4a 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -622,10 +622,9 @@ pub mod pallet { value, gas_limit, storage_deposit_limit: storage_deposit_limit.map(Into::into), - data, debug_message: None, }; - let mut output = CallInput:: { dest, determinism: Determinism::Deterministic } + let mut output = CallInput:: { dest, data, determinism: Determinism::Deterministic } .run_guarded(context); if let Ok(retval) = &output.result { if retval.did_revert() { @@ -684,11 +683,10 @@ pub mod pallet { value, gas_limit, storage_deposit_limit: storage_deposit_limit.map(Into::into), - data, debug_message: None, }; let mut output = - InstantiateInput:: { code: Code::Upload(code), salt }.run_guarded(context); + InstantiateInput:: { code: Code::Upload(code), data, salt }.run_guarded(context); if let Ok(retval) = &output.result { if retval.1.did_revert() { output.result = Err(>::ContractReverted.into()); @@ -726,10 +724,9 @@ pub mod pallet { value, gas_limit, storage_deposit_limit: storage_deposit_limit.map(Into::into), - data, debug_message: None, }; - let mut output = InstantiateInput:: { code: Code::Existing(code_hash), salt } + let mut output = InstantiateInput:: { code: Code::Existing(code_hash), data, salt } .run_guarded(context); if let Ok(retval) = &output.result { if retval.1.did_revert() { @@ -961,19 +958,20 @@ struct ExecContext<'a, T: Config> { value: BalanceOf, gas_limit: Weight, storage_deposit_limit: Option>, - data: Vec, debug_message: Option<&'a mut DebugBufferVec>, } /// Input specific to a call into contract. struct CallInput { dest: T::AccountId, + data: Vec, determinism: Determinism, } /// Input specific to a contract instantiation invocation. struct InstantiateInput { code: Code>, + data: Vec, salt: Vec, } @@ -1063,8 +1061,8 @@ impl Invokable for CallInput { }, }; let schedule = T::Schedule::get(); - let CallInput { dest, determinism } = self; - let ExecContext { origin, value, data, debug_message, .. } = context; + let CallInput { dest, data, determinism } = self; + let ExecContext { origin, value, debug_message, .. } = context; let result = ExecStack::>::run_call( origin.clone(), dest.clone(), @@ -1126,8 +1124,8 @@ impl Invokable for InstantiateInput { context.value.saturating_add(extra_deposit), )?; - let InstantiateInput { salt, .. } = self; - let ExecContext { origin, value, data, debug_message, .. } = context; + let InstantiateInput { salt, data, .. } = self; + let ExecContext { origin, value, debug_message, .. } = context; let result = ExecStack::>::run_instantiate( origin.clone(), executable, @@ -1177,10 +1175,9 @@ impl Pallet { value, gas_limit, storage_deposit_limit, - data, debug_message: debug_message.as_mut(), }; - let output = CallInput:: { dest, determinism }.run_guarded(context); + let output = CallInput:: { dest, data, determinism }.run_guarded(context); ContractExecResult { result: output.result.map_err(|r| r.error), gas_consumed: output.gas_meter.gas_consumed(), @@ -1218,10 +1215,9 @@ impl Pallet { value, gas_limit, storage_deposit_limit, - data, debug_message: debug_message.as_mut(), }; - let output = InstantiateInput:: { code, salt }.run_guarded(context); + let output = InstantiateInput:: { code, data, salt }.run_guarded(context); ContractInstantiateResult { result: output .result From dc92b6b73a7eee0baa664bae56496dde88867435 Mon Sep 17 00:00:00 2001 From: Alexander Gryaznov Date: Thu, 2 Mar 2023 15:32:52 +0200 Subject: [PATCH 15/17] docs fix --- frame/contracts/src/lib.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 8aa4deb6b9d4a..a90ab76f255d5 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -1029,7 +1029,10 @@ trait Invokable { }) } - /// Invoke a contract execution. + /// Method that does the actual call to a contract. It can be either a call to a deployed + /// contract or a instantiation of a new one. + /// + /// Called by dispatchables and public functions through the [`Invokable::run_guarded`]. fn run( &self, context: ExecContext, @@ -1039,9 +1042,7 @@ trait Invokable { impl Invokable for CallInput { type Output = ExecReturnValue; - /// Method that does the actual call to contract. - /// - /// Called by dispatchables and public functions through the [`Pallet::internal_run`]. + fn run( &self, context: ExecContext, @@ -1080,9 +1081,7 @@ impl Invokable for CallInput { impl Invokable for InstantiateInput { type Output = (AccountIdOf, ExecReturnValue); - /// Method that does the actual contract instantiation. - /// - /// Called by dispatchables and public functions through the [`Pallet::internal_run`]. + fn run( &self, mut context: ExecContext, From 972e4a7df493d1d91382ea041c7e676429379b8b Mon Sep 17 00:00:00 2001 From: Sasha Gryaznov Date: Fri, 3 Mar 2023 14:43:09 +0200 Subject: [PATCH 16/17] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Alexander Theißen --- frame/contracts/src/lib.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index a90ab76f255d5..e37b70923632e 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -988,8 +988,9 @@ struct InternalOutput { /// Helper trait to wrap contract execution entry points into a signle function /// [`Invokable::run_guarded`]. trait Invokable { - /// What is returned as a result of a succeed invocation. + /// What is returned as a result of a successful invocation. type Output; + /// Single entry point to contract execution. /// Downstream execution flow is branched by implementations of [`Invokable`] trait: /// @@ -1047,7 +1048,7 @@ impl Invokable for CallInput { &self, context: ExecContext, mut gas_meter: GasMeter, - ) -> InternalOutput { + ) -> InternalOutput { let mut storage_meter = match StorageMeter::new( &context.origin, context.storage_deposit_limit, @@ -1086,7 +1087,7 @@ impl Invokable for InstantiateInput { &self, mut context: ExecContext, mut gas_meter: GasMeter, - ) -> InternalOutput, ExecReturnValue)> { + ) -> InternalOutput { let mut storage_deposit = Default::default(); let try_exec = || { let schedule = T::Schedule::get(); From ddc386d81c923d540428044f2b4ee2696ed3b50d Mon Sep 17 00:00:00 2001 From: Alexander Gryaznov Date: Fri, 3 Mar 2023 14:53:03 +0200 Subject: [PATCH 17/17] address feedback, step 6 --- frame/contracts/src/lib.rs | 87 +++++++++++++++++++------------------- 1 file changed, 44 insertions(+), 43 deletions(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index e37b70923632e..4f5d7ba305fc1 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -617,15 +617,16 @@ pub mod pallet { let gas_limit: Weight = gas_limit.into(); let origin = ensure_signed(origin)?; let dest = T::Lookup::lookup(dest)?; - let context = ExecContext { + let common = CommonInput { origin, value, + data, gas_limit, storage_deposit_limit: storage_deposit_limit.map(Into::into), debug_message: None, }; - let mut output = CallInput:: { dest, data, determinism: Determinism::Deterministic } - .run_guarded(context); + let mut output = CallInput:: { dest, determinism: Determinism::Deterministic } + .run_guarded(common); if let Ok(retval) = &output.result { if retval.did_revert() { output.result = Err(>::ContractReverted.into()); @@ -678,15 +679,16 @@ pub mod pallet { let code_len = code.len() as u32; let data_len = data.len() as u32; let salt_len = salt.len() as u32; - let context = ExecContext { + let common = CommonInput { origin, value, + data, gas_limit, storage_deposit_limit: storage_deposit_limit.map(Into::into), debug_message: None, }; let mut output = - InstantiateInput:: { code: Code::Upload(code), data, salt }.run_guarded(context); + InstantiateInput:: { code: Code::Upload(code), salt }.run_guarded(common); if let Ok(retval) = &output.result { if retval.1.did_revert() { output.result = Err(>::ContractReverted.into()); @@ -719,15 +721,16 @@ pub mod pallet { let origin = ensure_signed(origin)?; let data_len = data.len() as u32; let salt_len = salt.len() as u32; - let context = ExecContext { + let common = CommonInput { origin, value, + data, gas_limit, storage_deposit_limit: storage_deposit_limit.map(Into::into), debug_message: None, }; - let mut output = InstantiateInput:: { code: Code::Existing(code_hash), data, salt } - .run_guarded(context); + let mut output = + InstantiateInput:: { code: Code::Existing(code_hash), salt }.run_guarded(common); if let Ok(retval) = &output.result { if retval.1.did_revert() { output.result = Err(>::ContractReverted.into()); @@ -953,9 +956,10 @@ pub mod pallet { } /// Context of a contract invocation. -struct ExecContext<'a, T: Config> { +struct CommonInput<'a, T: Config> { origin: T::AccountId, value: BalanceOf, + data: Vec, gas_limit: Weight, storage_deposit_limit: Option>, debug_message: Option<&'a mut DebugBufferVec>, @@ -964,14 +968,12 @@ struct ExecContext<'a, T: Config> { /// Input specific to a call into contract. struct CallInput { dest: T::AccountId, - data: Vec, determinism: Determinism, } /// Input specific to a contract instantiation invocation. struct InstantiateInput { code: Code>, - data: Vec, salt: Vec, } @@ -999,11 +1001,11 @@ trait Invokable { /// /// We enforce a re-entrancy guard here by initializing and checking a boolean flag through a /// global reference. - fn run_guarded(&self, context: ExecContext) -> InternalOutput { + fn run_guarded(&self, common: CommonInput) -> InternalOutput { // Set up a global reference to the boolean flag used for the re-entrancy guard. environmental!(executing_contract: bool); - let gas_limit = context.gas_limit; + let gas_limit = common.gas_limit; executing_contract::using_once(&mut false, || { executing_contract::with(|f| { // Fail if already entered contract execution @@ -1025,7 +1027,7 @@ trait Invokable { }), }, // Enter contract call. - |_| self.run(context, GasMeter::new(gas_limit)), + |_| self.run(common, GasMeter::new(gas_limit)), ) }) } @@ -1036,7 +1038,7 @@ trait Invokable { /// Called by dispatchables and public functions through the [`Invokable::run_guarded`]. fn run( &self, - context: ExecContext, + common: CommonInput, gas_meter: GasMeter, ) -> InternalOutput; } @@ -1046,25 +1048,22 @@ impl Invokable for CallInput { fn run( &self, - context: ExecContext, + common: CommonInput, mut gas_meter: GasMeter, ) -> InternalOutput { - let mut storage_meter = match StorageMeter::new( - &context.origin, - context.storage_deposit_limit, - context.value, - ) { - Ok(meter) => meter, - Err(err) => - return InternalOutput { - result: Err(err.into()), - gas_meter, - storage_deposit: Default::default(), - }, - }; + let mut storage_meter = + match StorageMeter::new(&common.origin, common.storage_deposit_limit, common.value) { + Ok(meter) => meter, + Err(err) => + return InternalOutput { + result: Err(err.into()), + gas_meter, + storage_deposit: Default::default(), + }, + }; let schedule = T::Schedule::get(); - let CallInput { dest, data, determinism } = self; - let ExecContext { origin, value, debug_message, .. } = context; + let CallInput { dest, determinism } = self; + let CommonInput { origin, value, data, debug_message, .. } = common; let result = ExecStack::>::run_call( origin.clone(), dest.clone(), @@ -1085,7 +1084,7 @@ impl Invokable for InstantiateInput { fn run( &self, - mut context: ExecContext, + mut common: CommonInput, mut gas_meter: GasMeter, ) -> InternalOutput { let mut storage_deposit = Default::default(); @@ -1096,12 +1095,12 @@ impl Invokable for InstantiateInput { let executable = PrefabWasmModule::from_code( binary.clone(), &schedule, - context.origin.clone(), + common.origin.clone(), Determinism::Deterministic, TryInstantiate::Skip, ) .map_err(|(err, msg)| { - context + common .debug_message .as_mut() .map(|buffer| buffer.try_extend(&mut msg.bytes())); @@ -1119,13 +1118,13 @@ impl Invokable for InstantiateInput { ), }; let mut storage_meter = StorageMeter::new( - &context.origin, - context.storage_deposit_limit, - context.value.saturating_add(extra_deposit), + &common.origin, + common.storage_deposit_limit, + common.value.saturating_add(extra_deposit), )?; - let InstantiateInput { salt, data, .. } = self; - let ExecContext { origin, value, debug_message, .. } = context; + let InstantiateInput { salt, .. } = self; + let CommonInput { origin, value, data, debug_message, .. } = common; let result = ExecStack::>::run_instantiate( origin.clone(), executable, @@ -1170,14 +1169,15 @@ impl Pallet { determinism: Determinism, ) -> ContractExecResult> { let mut debug_message = if debug { Some(DebugBufferVec::::default()) } else { None }; - let context = ExecContext { + let common = CommonInput { origin, value, + data, gas_limit, storage_deposit_limit, debug_message: debug_message.as_mut(), }; - let output = CallInput:: { dest, data, determinism }.run_guarded(context); + let output = CallInput:: { dest, determinism }.run_guarded(common); ContractExecResult { result: output.result.map_err(|r| r.error), gas_consumed: output.gas_meter.gas_consumed(), @@ -1210,14 +1210,15 @@ impl Pallet { debug: bool, ) -> ContractInstantiateResult> { let mut debug_message = if debug { Some(DebugBufferVec::::default()) } else { None }; - let context = ExecContext { + let common = CommonInput { origin, value, + data, gas_limit, storage_deposit_limit, debug_message: debug_message.as_mut(), }; - let output = InstantiateInput:: { code, data, salt }.run_guarded(context); + let output = InstantiateInput:: { code, salt }.run_guarded(common); ContractInstantiateResult { result: output .result