From 909cbae0601fd34589a16c9bd926865d050f4c3e Mon Sep 17 00:00:00 2001 From: pgherveou Date: Mon, 24 Apr 2023 16:40:36 +0200 Subject: [PATCH] wip --- Cargo.lock | 1 + frame/contracts/Cargo.toml | 1 + .../fixtures/call_runtime_and_call.wat | 56 +++++++++++ frame/contracts/src/lib.rs | 15 ++- frame/contracts/src/storage/meter.rs | 64 ++++--------- frame/contracts/src/tests.rs | 96 +++++++++++++++++++ 6 files changed, 185 insertions(+), 48 deletions(-) create mode 100644 frame/contracts/fixtures/call_runtime_and_call.wat diff --git a/Cargo.lock b/Cargo.lock index aa7d22daccb80..a14afdeb69359 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..4a6197fc1cb0d --- /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) - storage length + ;; [8..40) - hash code 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) ;; Size of the call without the call_runtime + ) + ) + )) + + ;; 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 fa230a8cade14..4a3b0d20b7453 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -1033,7 +1033,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, result, storage_deposit }, + Err(err) => InternalOutput { + result: Err(err.into()), + gas_meter, + storage_deposit: Default::default(), + }, + } } } @@ -1094,9 +1102,8 @@ impl Invokable for InstantiateInput { &salt, debug_message, ); - storage_deposit = storage_meter - .into_deposit(&origin) - .saturating_add(&StorageDeposit::Charge(extra_deposit)); + + storage_deposit = storage_meter.try_into_deposit(&origin)?; result }; InternalOutput { result: try_exec(), gas_meter, storage_deposit } diff --git a/frame/contracts/src/storage/meter.rs b/frame/contracts/src/storage/meter.rs index 497ee03ac319a..7f0b12d3be6ba 100644 --- a/frame/contracts/src/storage/meter.rs +++ b/frame/contracts/src/storage/meter.rs @@ -82,7 +82,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. @@ -343,14 +343,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) } } @@ -405,7 +405,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. @@ -476,41 +477,14 @@ 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: "runtime::contracts", - "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."); - } - } - }, + Deposit::Charge(amount) => T::Currency::transfer( + origin, + deposit_account, + *amount, + ExistenceRequirement::KeepAlive, + ), // 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. @@ -539,8 +513,9 @@ impl Ext for ReservingExt { panic!("Unable to refund storage deposit. This is a bug."); } } + Ok(()) }, - }; + } } } @@ -613,7 +588,7 @@ mod tests { contract: &DepositAccount, amount: &DepositOf, terminated: bool, - ) { + ) -> Result<(), DispatchError> { TestExtTestValue::mutate(|ext| { ext.charges.push(Charge { origin: origin.clone(), @@ -622,6 +597,7 @@ mod tests { terminated, }) }); + Ok(()) } } @@ -719,7 +695,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); @@ -779,7 +755,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 4e6c468f19c81..ffc9e408a5944 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -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(); @@ -2966,6 +2983,85 @@ fn sr25519_verify() { }) } +#[test] +fn test_failed_charge_should_roll_back_call() { + let (wasm_caller, _) = compile_module::("call_runtime_and_call").unwrap(); + let (wasm_callee, _) = compile_module::("store").unwrap(); + + ExtBuilder::default().existential_deposit(200).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), + vec![], + vec![], + false, + ) + .result + .unwrap() + .account_id; + let addr_callee = Contracts::bare_instantiate( + ALICE, + 0, + GAS_LIMIT, + None, + Code::Upload(wasm_callee), + vec![], + vec![], + false, + ) + .result + .unwrap() + .account_id; + + // Give caller proxy access to caller + assert_ok!(Proxy::add_proxy(RuntimeOrigin::signed(ALICE), addr_caller.clone(), (), 0)); + + // create a Proxy call that will create a transfer onbehalf of Alice + let value = pallet_balances::Pallet::::free_balance(&ALICE) - + ::Currency::minimum_balance(); + + let transfer_call = + Box::new(RuntimeCall::Balances(pallet_balances::Call::transfer_allow_death { + dest: CHARLIE, + value, + })); + + // 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, + }); + + // encode inputs data + let data = ( + 30u32, // storage length + addr_callee.clone(), + transfer_proxy_call, + ); + + DEPOSIT_PER_BYTE.with(|c| *c.borrow_mut() = 1_000); + + // dispatch the call + let result = >::call( + RuntimeOrigin::signed(ALICE), + addr_caller.clone(), + 0, + GAS_LIMIT, + None, //Some(5_000), + data.encode(), + ); + + assert_err_ignore_postinfo!(result, TokenError::FundsUnavailable); + }) +} + #[test] fn upload_code_works() { let (wasm, code_hash) = compile_module::("dummy").unwrap();