From 4d80008974b3998bae15192b2aa2b9eaecf3a9ee Mon Sep 17 00:00:00 2001 From: PG Herveou Date: Sat, 29 Apr 2023 18:07:55 +0200 Subject: [PATCH] Contracts: runtime_call and storage_deposit (#13990) * wip * add comments * fix comment * comments * comments * PR comment * field orders * Update frame/contracts/src/tests.rs * Update frame/contracts/fixtures/call_runtime_and_call.wat Co-authored-by: Sasha Gryaznov * Apply suggestions from code review Co-authored-by: Sasha Gryaznov * Apply suggestions from code review Co-authored-by: Sasha Gryaznov * Update frame/contracts/src/tests.rs Co-authored-by: Sasha Gryaznov * Validate fees of failed call * Update frame/contracts/src/tests.rs * Update frame/contracts/src/tests.rs * Update frame/contracts/src/tests.rs * bubble up refund error * rename fixture file --------- Co-authored-by: Sasha Gryaznov Co-authored-by: parity-processbot <> --- Cargo.lock | 1 + frame/contracts/Cargo.toml | 1 + .../fixtures/call_runtime_and_call.wat | 56 +++++++++++ frame/contracts/src/lib.rs | 13 ++- frame/contracts/src/storage/meter.rs | 86 +++++----------- frame/contracts/src/tests.rs | 99 ++++++++++++++++++- 6 files changed, 189 insertions(+), 67 deletions(-) create mode 100644 frame/contracts/fixtures/call_runtime_and_call.wat diff --git a/Cargo.lock b/Cargo.lock index 420c9040897b5..a1244f2711beb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5922,6 +5922,7 @@ dependencies = [ "pallet-contracts-primitives", "pallet-contracts-proc-macro", "pallet-insecure-randomness-collective-flip", + "pallet-proxy", "pallet-timestamp", "pallet-utility", "parity-scale-codec", diff --git a/frame/contracts/Cargo.toml b/frame/contracts/Cargo.toml index 6ab60846fd412..edb6d294cfcd5 100644 --- a/frame/contracts/Cargo.toml +++ b/frame/contracts/Cargo.toml @@ -59,6 +59,7 @@ pallet-balances = { version = "4.0.0-dev", path = "../balances" } pallet-timestamp = { version = "4.0.0-dev", path = "../timestamp" } pallet-insecure-randomness-collective-flip = { version = "4.0.0-dev", path = "../insecure-randomness-collective-flip" } pallet-utility = { version = "4.0.0-dev", path = "../utility" } +pallet-proxy = { version = "4.0.0-dev", path = "../proxy" } sp-keystore = { version = "0.13.0", path = "../../primitives/keystore" } [features] diff --git a/frame/contracts/fixtures/call_runtime_and_call.wat b/frame/contracts/fixtures/call_runtime_and_call.wat new file mode 100644 index 0000000000000..3320922d9e2cb --- /dev/null +++ b/frame/contracts/fixtures/call_runtime_and_call.wat @@ -0,0 +1,56 @@ +(module + (import "seal0" "call_runtime" (func $call_runtime (param i32 i32) (result i32))) + (import "seal1" "seal_call" (func $seal_call (param i32 i32 i64 i32 i32 i32 i32 i32) (result i32))) + (import "seal0" "seal_input" (func $seal_input (param i32 i32))) + (import "seal0" "seal_return" (func $seal_return (param i32 i32 i32))) + (import "env" "memory" (memory 1 1)) + + (func $assert (param i32) + (block $ok + (br_if $ok (get_local 0)) + (unreachable) + ) + ) + + (func (export "call") + ;; Store available input size at offset 0. + (i32.store (i32.const 0) (i32.const 512)) + + ;; read input data + (call $seal_input (i32.const 4) (i32.const 0)) + + ;; Input data layout. + ;; [0..4) - size of the call + ;; [4..8) - how many bytes to add to storage + ;; [8..40) - address of the callee + ;; [40..n) - encoded runtime call + + ;; Invoke call_runtime with the encoded call passed to this contract. + (call $assert (i32.eqz + (call $call_runtime + (i32.const 40) ;; Pointer where the call is stored + (i32.sub + (i32.load (i32.const 0)) ;; Size of the call + (i32.const 36) ;; Subtract size of the subcall-related part: 4 bytes for storage length to add + 32 bytes of the callee address + ) + ) + )) + + ;; call passed contract + (call $assert (i32.eqz + (call $seal_call + (i32.const 0) ;; No flags + (i32.const 8) ;; Pointer to "callee" address. + (i64.const 0) ;; How much gas to devote for the execution. 0 = all. + (i32.const 512) ;; Pointer to the buffer with value to transfer + (i32.const 4) ;; Pointer to input data buffer address + (i32.const 4) ;; Length of input data buffer + (i32.const 4294967295) ;; u32 max value is the sentinel value: do not copy output + (i32.const 0) ;; Length is ignored in this case + ) + )) + ) + + (func (export "deploy")) +) + diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 5a82a4a42b4a9..24653423b4f67 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -1044,7 +1044,15 @@ impl Invokable for CallInput { debug_message, *determinism, ); - InternalOutput { gas_meter, storage_deposit: storage_meter.into_deposit(&origin), result } + + match storage_meter.try_into_deposit(&origin) { + Ok(storage_deposit) => InternalOutput { gas_meter, storage_deposit, result }, + Err(err) => InternalOutput { + gas_meter, + storage_deposit: Default::default(), + result: Err(err.into()), + }, + } } } @@ -1105,8 +1113,9 @@ impl Invokable for InstantiateInput { &salt, debug_message, ); + storage_deposit = storage_meter - .into_deposit(&origin) + .try_into_deposit(&origin)? .saturating_add(&StorageDeposit::Charge(extra_deposit)); result }; diff --git a/frame/contracts/src/storage/meter.rs b/frame/contracts/src/storage/meter.rs index b8bbd6dc4178b..73794b59774d4 100644 --- a/frame/contracts/src/storage/meter.rs +++ b/frame/contracts/src/storage/meter.rs @@ -19,7 +19,7 @@ use crate::{ storage::{ContractInfo, DepositAccount}, - BalanceOf, Config, Error, Inspect, Pallet, System, LOG_TARGET, + BalanceOf, Config, Error, Inspect, Pallet, System, }; use codec::Encode; use frame_support::{ @@ -85,7 +85,7 @@ pub trait Ext { deposit_account: &DepositAccount, amount: &DepositOf, terminated: bool, - ); + ) -> Result<(), DispatchError>; } /// This [`Ext`] is used for actual on-chain execution when balance needs to be charged. @@ -366,14 +366,14 @@ where /// /// This drops the root meter in order to make sure it is only called when the whole /// execution did finish. - pub fn into_deposit(self, origin: &T::AccountId) -> DepositOf { + pub fn try_into_deposit(self, origin: &T::AccountId) -> Result, DispatchError> { for charge in self.charges.iter().filter(|c| matches!(c.amount, Deposit::Refund(_))) { - E::charge(origin, &charge.deposit_account, &charge.amount, charge.terminated); + E::charge(origin, &charge.deposit_account, &charge.amount, charge.terminated)?; } for charge in self.charges.iter().filter(|c| matches!(c.amount, Deposit::Charge(_))) { - E::charge(origin, &charge.deposit_account, &charge.amount, charge.terminated); + E::charge(origin, &charge.deposit_account, &charge.amount, charge.terminated)?; } - self.total_deposit + Ok(self.total_deposit) } } @@ -428,7 +428,8 @@ where info.deposit_account(), &deposit.saturating_sub(&Deposit::Charge(ed)), false, - ); + )?; + System::::inc_consumers(info.deposit_account())?; // We also need to make sure that the contract's account itself exists. @@ -514,71 +515,27 @@ impl Ext for ReservingExt { deposit_account: &DepositAccount, amount: &DepositOf, terminated: bool, - ) { - // There is nothing we can do when this fails as this constitutes a bug in the runtime. - // We need to settle for emitting an error log in this case. - // - // # Note - // - // This is infallible because it is called in a part of the execution where we cannot - // simply roll back. It might make sense to do some refactoring to move the deposit - // collection to the fallible part of execution. + ) -> Result<(), DispatchError> { match amount { - Deposit::Charge(amount) => { - // This will never fail because a deposit account is required to exist - // at all times. The pallet enforces this invariant by holding a consumer reference - // on the deposit account as long as the contract exists. - // - // The sender always has enough balance because we checked that it had enough - // balance when instantiating the storage meter. There is no way for the sender - // which is a plain account to send away this balance in the meantime. - let result = T::Currency::transfer( - origin, - deposit_account, - *amount, - ExistenceRequirement::KeepAlive, - ); - if let Err(err) = result { - log::error!( - target: LOG_TARGET, - "Failed to transfer storage deposit {:?} from origin {:?} to deposit account {:?}: {:?}", - amount, origin, deposit_account, err, - ); - if cfg!(debug_assertions) { - panic!("Unable to collect storage deposit. This is a bug."); - } - } - }, - // The receiver always exists because the initial value transfer from the - // origin to the contract has a keep alive existence requirement. When taking a deposit - // we make sure to leave at least the ed in the free balance. - // - // The sender always has enough balance because we track it in the `ContractInfo` and - // never send more back than we have. No one has access to the deposit account. Hence no - // other interaction with this account takes place. + Deposit::Charge(amount) => T::Currency::transfer( + origin, + deposit_account, + *amount, + ExistenceRequirement::KeepAlive, + ), Deposit::Refund(amount) => { if terminated { System::::dec_consumers(&deposit_account); } - let result = T::Currency::transfer( + T::Currency::transfer( deposit_account, origin, *amount, // We can safely use `AllowDeath` because our own consumer prevents an removal. ExistenceRequirement::AllowDeath, - ); - if matches!(result, Err(_)) { - log::error!( - target: LOG_TARGET, - "Failed to refund storage deposit {:?} from deposit account {:?} to origin {:?}: {:?}", - amount, deposit_account, origin, result, - ); - if cfg!(debug_assertions) { - panic!("Unable to refund storage deposit. This is a bug."); - } - } + ) }, - }; + } } } @@ -651,7 +608,7 @@ mod tests { contract: &DepositAccount, amount: &DepositOf, terminated: bool, - ) { + ) -> Result<(), DispatchError> { TestExtTestValue::mutate(|ext| { ext.charges.push(Charge { origin: origin.clone(), @@ -660,6 +617,7 @@ mod tests { terminated, }) }); + Ok(()) } } @@ -757,7 +715,7 @@ mod tests { nested0.enforce_limit(Some(&mut nested0_info)).unwrap(); meter.absorb(nested0, DepositAccount(BOB), Some(&mut nested0_info)); - meter.into_deposit(&ALICE); + meter.try_into_deposit(&ALICE).unwrap(); assert_eq!(nested0_info.extra_deposit(), 112); assert_eq!(nested1_info.extra_deposit(), 110); @@ -817,7 +775,7 @@ mod tests { nested0.absorb(nested1, DepositAccount(CHARLIE), None); meter.absorb(nested0, DepositAccount(BOB), None); - meter.into_deposit(&ALICE); + meter.try_into_deposit(&ALICE).unwrap(); assert_eq!( TestExtTestValue::get(), diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index 697b58f15c609..c202d3796e3ed 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -33,7 +33,7 @@ use crate::{ use assert_matches::assert_matches; use codec::Encode; use frame_support::{ - assert_err, assert_err_ignore_postinfo, assert_noop, assert_ok, + assert_err, assert_err_ignore_postinfo, assert_err_with_weight, assert_noop, assert_ok, dispatch::{DispatchErrorWithPostInfo, PostDispatchInfo}, parameter_types, storage::child, @@ -70,6 +70,7 @@ frame_support::construct_runtime!( Randomness: pallet_insecure_randomness_collective_flip::{Pallet, Storage}, Utility: pallet_utility::{Pallet, Call, Storage, Event}, Contracts: pallet_contracts::{Pallet, Call, Storage, Event}, + Proxy: pallet_proxy::{Pallet, Call, Storage, Event}, } ); @@ -337,6 +338,22 @@ impl pallet_utility::Config for Test { type PalletsOrigin = OriginCaller; type WeightInfo = (); } + +impl pallet_proxy::Config for Test { + type RuntimeEvent = RuntimeEvent; + type RuntimeCall = RuntimeCall; + type Currency = Balances; + type ProxyType = (); + type ProxyDepositBase = ConstU64<1>; + type ProxyDepositFactor = ConstU64<1>; + type MaxProxies = ConstU32<32>; + type WeightInfo = (); + type MaxPending = ConstU32<32>; + type CallHasher = BlakeTwo256; + type AnnouncementDepositBase = ConstU64<1>; + type AnnouncementDepositFactor = ConstU64<1>; +} + parameter_types! { pub MySchedule: Schedule = { let mut schedule = >::default(); @@ -2983,6 +3000,86 @@ fn sr25519_verify() { }) } +#[test] +fn failed_deposit_charge_should_roll_back_call() { + let (wasm_caller, _) = compile_module::("call_runtime_and_call").unwrap(); + let (wasm_callee, _) = compile_module::("store_call").unwrap(); + const ED: u64 = 200; + + let execute = || { + ExtBuilder::default().existential_deposit(ED).build().execute_with(|| { + let _ = Balances::deposit_creating(&ALICE, 1_000_000); + + // Instantiate both contracts. + let addr_caller = Contracts::bare_instantiate( + ALICE, + 0, + GAS_LIMIT, + None, + Code::Upload(wasm_caller.clone()), + vec![], + vec![], + false, + ) + .result + .unwrap() + .account_id; + let addr_callee = Contracts::bare_instantiate( + ALICE, + 0, + GAS_LIMIT, + None, + Code::Upload(wasm_callee.clone()), + vec![], + vec![], + false, + ) + .result + .unwrap() + .account_id; + + // Give caller proxy access to Alice. + assert_ok!(Proxy::add_proxy(RuntimeOrigin::signed(ALICE), addr_caller.clone(), (), 0)); + + // Create a Proxy call that will attempt to transfer away Alice's balance. + let transfer_call = + Box::new(RuntimeCall::Balances(pallet_balances::Call::transfer_allow_death { + dest: CHARLIE, + value: pallet_balances::Pallet::::free_balance(&ALICE) - 2 * ED, + })); + + // Wrap the transfer call in a proxy call. + let transfer_proxy_call = RuntimeCall::Proxy(pallet_proxy::Call::proxy { + real: ALICE, + force_proxy_type: Some(()), + call: transfer_call, + }); + + let data = ( + (ED - DepositPerItem::get()) as u32, // storage length + addr_callee, + transfer_proxy_call, + ); + + >::call( + RuntimeOrigin::signed(ALICE), + addr_caller.clone(), + 0, + GAS_LIMIT, + None, + data.encode(), + ) + }) + }; + + // With a low enough deposit per byte, the call should succeed. + let result = execute().unwrap(); + + // Bump the deposit per byte to a high value to trigger a FundsUnavailable error. + DEPOSIT_PER_BYTE.with(|c| *c.borrow_mut() = ED); + assert_err_with_weight!(execute(), TokenError::FundsUnavailable, result.actual_weight); +} + #[test] fn upload_code_works() { let (wasm, code_hash) = compile_module::("dummy").unwrap();