From acde6b40587611e1786a874c27463a92af82a3b9 Mon Sep 17 00:00:00 2001 From: Ermal Kaleci Date: Thu, 17 Oct 2024 17:29:53 +0200 Subject: [PATCH 01/18] update delegate_call to take address and weight --- substrate/frame/revive/src/exec.rs | 80 ++++++++++++++++--- substrate/frame/revive/src/wasm/runtime.rs | 18 +++-- substrate/frame/revive/uapi/src/host.rs | 8 +- .../frame/revive/uapi/src/host/riscv32.rs | 48 ++++++----- 4 files changed, 116 insertions(+), 38 deletions(-) diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index c6d2f205ae22..d43dd02f9922 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -194,7 +194,12 @@ pub trait Ext: sealing::Sealed { /// Execute code in the current frame. /// /// Returns the code size of the called contract. - fn delegate_call(&mut self, code: H256, input_data: Vec) -> Result<(), ExecError>; + fn delegate_call( + &mut self, + gas_limit: Weight, + address: H160, + input_data: Vec, + ) -> Result<(), ExecError>; /// Instantiate a contract from the given code. /// @@ -1377,8 +1382,14 @@ where result } - fn delegate_call(&mut self, code_hash: H256, input_data: Vec) -> Result<(), ExecError> { - let executable = E::from_storage(code_hash, self.gas_meter_mut())?; + fn delegate_call( + &mut self, + gas_limit: Weight, + address: H160, + input_data: Vec, + ) -> Result<(), ExecError> { + let contract_info = ContractInfoOf::::get(&address).ok_or(Error::::CodeNotFound)?; + let executable = E::from_storage(contract_info.code_hash, self.gas_meter_mut())?; let top_frame = self.top_frame_mut(); let contract_info = top_frame.contract_info().clone(); let account_id = top_frame.account_id.clone(); @@ -1390,7 +1401,7 @@ where delegated_call: Some(DelegatedCall { executable, caller: self.caller().clone() }), }, value, - Weight::zero(), + gas_limit, BalanceOf::::zero(), self.is_read_only(), )?; @@ -1804,7 +1815,7 @@ mod tests { AddressMapper, Error, }; use assert_matches::assert_matches; - use frame_support::{assert_err, assert_ok, parameter_types}; + use frame_support::{assert_err, assert_noop, assert_ok, parameter_types}; use frame_system::{EventRecord, Phase}; use pallet_revive_uapi::ReturnFlags; use pretty_assertions::assert_eq; @@ -2026,18 +2037,19 @@ mod tests { let delegate_ch = MockLoader::insert(Call, move |ctx, _| { assert_eq!(ctx.ext.value_transferred(), U256::from(value)); - let _ = ctx.ext.delegate_call(success_ch, Vec::new())?; + let _ = ctx.ext.delegate_call(Weight::zero(), CHARLIE_ADDR, Vec::new())?; Ok(ExecReturnValue { flags: ReturnFlags::empty(), data: Vec::new() }) }); ExtBuilder::default().build().execute_with(|| { place_contract(&BOB, delegate_ch); + place_contract(&CHARLIE, success_ch); set_balance(&ALICE, 100); let balance = get_balance(&BOB_CONTRACT_ID); let origin = Origin::from_account_id(ALICE); let mut storage_meter = storage::meter::Meter::new(&origin, 0, 55).unwrap(); - let _ = MockStack::run_call( + assert_ok!(MockStack::run_call( origin, BOB_ADDR, &mut GasMeter::::new(GAS_LIMIT), @@ -2045,14 +2057,62 @@ mod tests { value, vec![], None, - ) - .unwrap(); + )); assert_eq!(get_balance(&ALICE), 100 - value); assert_eq!(get_balance(&BOB_CONTRACT_ID), balance + value); }); } + #[test] + fn delegate_call_missing_contract() { + let missing_ch = MockLoader::insert(Call, move |_ctx, _| { + Ok(ExecReturnValue { flags: ReturnFlags::empty(), data: Vec::new() }) + }); + + let delegate_ch = MockLoader::insert(Call, move |ctx, _| { + let _ = ctx.ext.delegate_call(Weight::zero(), CHARLIE_ADDR, Vec::new())?; + Ok(ExecReturnValue { flags: ReturnFlags::empty(), data: Vec::new() }) + }); + + ExtBuilder::default().build().execute_with(|| { + place_contract(&BOB, delegate_ch); + set_balance(&ALICE, 100); + + let origin = Origin::from_account_id(ALICE); + let mut storage_meter = storage::meter::Meter::new(&origin, 0, 55).unwrap(); + + // contract code missing + assert_noop!( + MockStack::run_call( + origin.clone(), + BOB_ADDR, + &mut GasMeter::::new(GAS_LIMIT), + &mut storage_meter, + 0, + vec![], + None, + ), + ExecError { + error: Error::::CodeNotFound.into(), + origin: ErrorOrigin::Callee, + } + ); + + // add missing contract code + place_contract(&CHARLIE, missing_ch); + assert_ok!(MockStack::run_call( + origin, + BOB_ADDR, + &mut GasMeter::::new(GAS_LIMIT), + &mut storage_meter, + 0, + vec![], + None, + )); + }); + } + #[test] fn changes_are_reverted_on_failing_call() { // This test verifies that changes are reverted on a call which fails (or equally, returns @@ -4418,7 +4478,7 @@ mod tests { // In a delegate call, we should witness the caller immutable data assert_eq!( - ctx.ext.delegate_call(charlie_ch, Vec::new()).map(|_| ctx + ctx.ext.delegate_call(Weight::zero(), CHARLIE_ADDR, Vec::new()).map(|_| ctx .ext .last_frame_output() .data diff --git a/substrate/frame/revive/src/wasm/runtime.rs b/substrate/frame/revive/src/wasm/runtime.rs index 245c91278a7f..c4a3fbb75544 100644 --- a/substrate/frame/revive/src/wasm/runtime.rs +++ b/substrate/frame/revive/src/wasm/runtime.rs @@ -530,7 +530,7 @@ enum CallType { /// Execute another instantiated contract Call { callee_ptr: u32, value_ptr: u32, deposit_ptr: u32, weight: Weight }, /// Execute deployed code in the context (storage, account ID, value) of the caller contract - DelegateCall { code_hash_ptr: u32 }, + DelegateCall { address_ptr: u32, weight: Weight }, } impl CallType { @@ -1026,13 +1026,14 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { read_only, ) }, - CallType::DelegateCall { code_hash_ptr } => { + CallType::DelegateCall { address_ptr, weight } => { if flags.intersects(CallFlags::ALLOW_REENTRY | CallFlags::READ_ONLY) { return Err(Error::::InvalidCallFlags.into()); } - let code_hash = memory.read_h256(code_hash_ptr)?; - self.ext.delegate_call(code_hash, input_data) + let mut address = H160::zero(); + memory.read_into_buf(address_ptr, address.as_bytes_mut())?; + self.ext.delegate_call(weight, address, input_data) }, }; @@ -1290,7 +1291,9 @@ pub mod env { &mut self, memory: &mut M, flags: u32, - code_hash_ptr: u32, + address_ptr: u32, + ref_time_limit: u64, + proof_size_limit: u64, input_data_ptr: u32, input_data_len: u32, output_ptr: u32, @@ -1299,7 +1302,10 @@ pub mod env { self.call( memory, CallFlags::from_bits(flags).ok_or(Error::::InvalidCallFlags)?, - CallType::DelegateCall { code_hash_ptr }, + CallType::DelegateCall { + address_ptr, + weight: Weight::from_parts(ref_time_limit, proof_size_limit), + }, input_data_ptr, input_data_len, output_ptr, diff --git a/substrate/frame/revive/uapi/src/host.rs b/substrate/frame/revive/uapi/src/host.rs index 2106b8fb49b7..2c3859bb7d73 100644 --- a/substrate/frame/revive/uapi/src/host.rs +++ b/substrate/frame/revive/uapi/src/host.rs @@ -290,7 +290,9 @@ pub trait HostFn: private::Sealed { /// # Parameters /// /// - `flags`: See [`CallFlags`] for a documentation of the supported flags. - /// - `code_hash`: The hash of the code to be executed. + /// - `address`: The address of the code to be executed. Should be decodable as an `T::AccountId`. Traps otherwise. + /// - `ref_time_limit`: how much *ref_time* Weight to devote to the execution. + /// - `proof_size_limit`: how much *proof_size* Weight to devote to the execution. /// - `input`: The input data buffer used to call the contract. /// - `output`: A reference to the output data buffer to write the call output buffer. If `None` /// is provided then the output buffer is not copied. @@ -305,7 +307,9 @@ pub trait HostFn: private::Sealed { /// - [CodeNotFound][`crate::ReturnErrorCode::CodeNotFound] fn delegate_call( flags: CallFlags, - code_hash: &[u8; 32], + address: &[u8; 20], + ref_time_limit: u64, + proof_size_limit: u64, input_data: &[u8], output: Option<&mut &mut [u8]>, ) -> Result; diff --git a/substrate/frame/revive/uapi/src/host/riscv32.rs b/substrate/frame/revive/uapi/src/host/riscv32.rs index 866b0ee8dd17..3e1fc15688fe 100644 --- a/substrate/frame/revive/uapi/src/host/riscv32.rs +++ b/substrate/frame/revive/uapi/src/host/riscv32.rs @@ -60,14 +60,7 @@ mod sys { ) -> ReturnCode; pub fn transfer(address_ptr: *const u8, value_ptr: *const u8) -> ReturnCode; pub fn call(ptr: *const u8) -> ReturnCode; - pub fn delegate_call( - flags: u32, - code_hash_ptr: *const u8, - input_data_ptr: *const u8, - input_data_len: u32, - out_ptr: *mut u8, - out_len_ptr: *mut u32, - ) -> ReturnCode; + pub fn delegate_call(*const u8) -> ReturnCode; pub fn instantiate(ptr: *const u8) -> ReturnCode; pub fn terminate(beneficiary_ptr: *const u8); pub fn input(out_ptr: *mut u8, out_len_ptr: *mut u32); @@ -304,24 +297,39 @@ impl HostFn for HostFnImpl { fn delegate_call( flags: CallFlags, - code_hash: &[u8; 32], + address: &[u8; 20], + ref_time_limit: u64, + proof_size_limit: u64, input: &[u8], mut output: Option<&mut &mut [u8]>, ) -> Result { let (output_ptr, mut output_len) = ptr_len_or_sentinel(&mut output); - let ret_code = { - unsafe { - sys::delegate_call( - flags.bits(), - code_hash.as_ptr(), - input.as_ptr(), - input.len() as u32, - output_ptr, - &mut output_len, - ) - } + + #[repr(packed)] + #[allow(dead_code)] + struct Args { + flags: u32, + address: *const u8, + ref_time_limit: u64, + proof_size_limit: u64, + input: *const u8, + input_len: u32, + output: *mut u8, + output_len: *mut u32, + } + let args = Args { + flags: flags.bits(), + address: address.as_ptr(), + ref_time_limit, + proof_size_limit, + input: input.as_ptr(), + input_len: input.len() as _, + output: output_ptr, + output_len: &mut output_len as *mut _, }; + let ret_code = { unsafe { sys::delegate_call(&args as *const Args as *const _) } }; + if let Some(ref mut output) = output { extract_from_slice(output, output_len as usize); } From 02dace63ec14f77e63f30e09478928ed2b8071ca Mon Sep 17 00:00:00 2001 From: Ermal Kaleci Date: Thu, 17 Oct 2024 22:28:40 +0200 Subject: [PATCH 02/18] fix fixtures --- .../fixtures/contracts/delegate_call.rs | 8 +- .../contracts/delegate_call_simple.rs | 6 +- .../contracts/locking_delegate_dependency.rs | 3 +- substrate/frame/revive/src/tests.rs | 74 ++++++++++++++++--- .../frame/revive/uapi/src/host/riscv32.rs | 2 +- 5 files changed, 77 insertions(+), 16 deletions(-) diff --git a/substrate/frame/revive/fixtures/contracts/delegate_call.rs b/substrate/frame/revive/fixtures/contracts/delegate_call.rs index 9fd155408af3..3abb52bdc86d 100644 --- a/substrate/frame/revive/fixtures/contracts/delegate_call.rs +++ b/substrate/frame/revive/fixtures/contracts/delegate_call.rs @@ -28,7 +28,11 @@ pub extern "C" fn deploy() {} #[no_mangle] #[polkavm_derive::polkavm_export] pub extern "C" fn call() { - input!(code_hash: &[u8; 32],); + input!( + address: &[u8; 20], + ref_time: u64, + proof_size: u64, + ); let mut key = [0u8; 32]; key[0] = 1u8; @@ -42,7 +46,7 @@ pub extern "C" fn call() { assert!(value[0] == 2u8); let input = [0u8; 0]; - api::delegate_call(uapi::CallFlags::empty(), code_hash, &input, None).unwrap(); + api::delegate_call(uapi::CallFlags::empty(), address, ref_time, proof_size, &input, None).unwrap(); api::get_storage(StorageFlags::empty(), &key, value).unwrap(); assert!(value[0] == 1u8); diff --git a/substrate/frame/revive/fixtures/contracts/delegate_call_simple.rs b/substrate/frame/revive/fixtures/contracts/delegate_call_simple.rs index 20f8ec3364ee..74ed71921b8e 100644 --- a/substrate/frame/revive/fixtures/contracts/delegate_call_simple.rs +++ b/substrate/frame/revive/fixtures/contracts/delegate_call_simple.rs @@ -28,9 +28,9 @@ pub extern "C" fn deploy() {} #[no_mangle] #[polkavm_derive::polkavm_export] pub extern "C" fn call() { - input!(code_hash: &[u8; 32],); + input!(address: &[u8; 20],); - // Delegate call into passed code hash. + // Delegate call into passed address. let input = [0u8; 0]; - api::delegate_call(uapi::CallFlags::empty(), code_hash, &input, None).unwrap(); + api::delegate_call(uapi::CallFlags::empty(), address, 0, 0, &input, None).unwrap(); } diff --git a/substrate/frame/revive/fixtures/contracts/locking_delegate_dependency.rs b/substrate/frame/revive/fixtures/contracts/locking_delegate_dependency.rs index 2efacb4e683f..181fdd7a6ff0 100644 --- a/substrate/frame/revive/fixtures/contracts/locking_delegate_dependency.rs +++ b/substrate/frame/revive/fixtures/contracts/locking_delegate_dependency.rs @@ -30,6 +30,7 @@ const ETH_ALICE: [u8; 20] = [1u8; 20]; fn load_input(delegate_call: bool) { input!( action: u32, + address: &[u8; 20], code_hash: &[u8; 32], ); @@ -51,7 +52,7 @@ fn load_input(delegate_call: bool) { } if delegate_call { - api::delegate_call(uapi::CallFlags::empty(), code_hash, &[], None).unwrap(); + api::delegate_call(uapi::CallFlags::empty(), address, 0, 0, &[], None).unwrap(); } } diff --git a/substrate/frame/revive/src/tests.rs b/substrate/frame/revive/src/tests.rs index 4816e65f8f5c..ddffad1605fb 100644 --- a/substrate/frame/revive/src/tests.rs +++ b/substrate/frame/revive/src/tests.rs @@ -1116,7 +1116,7 @@ mod run_tests { #[test] fn delegate_call() { let (caller_wasm, _caller_code_hash) = compile_module("delegate_call").unwrap(); - let (callee_wasm, callee_code_hash) = compile_module("delegate_call_lib").unwrap(); + let (callee_wasm, _callee_code_hash) = compile_module("delegate_call_lib").unwrap(); ExtBuilder::default().existential_deposit(500).build().execute_with(|| { let _ = ::Currency::set_balance(&ALICE, 1_000_000); @@ -1126,12 +1126,53 @@ mod run_tests { builder::bare_instantiate(Code::Upload(caller_wasm)) .value(300_000) .build_and_unwrap_contract(); - // Only upload 'callee' code - assert_ok!(Contracts::upload_code(RuntimeOrigin::signed(ALICE), callee_wasm, 100_000,)); + + // Instantiate the 'callee' + let Contract { addr: callee_addr, .. } = + builder::bare_instantiate(Code::Upload(callee_wasm)) + .value(100_000) + .build_and_unwrap_contract(); assert_ok!(builder::call(caller_addr) .value(1337) - .data(callee_code_hash.as_ref().to_vec()) + .data((callee_addr, 0u64, 0u64).encode()) + .build()); + }); + } + + #[test] + fn delegate_call_with_limits() { + let (caller_wasm, _caller_code_hash) = compile_module("delegate_call").unwrap(); + let (callee_wasm, _callee_code_hash) = compile_module("delegate_call_lib").unwrap(); + + ExtBuilder::default().existential_deposit(500).build().execute_with(|| { + let _ = ::Currency::set_balance(&ALICE, 1_000_000); + + // Instantiate the 'caller' + let Contract { addr: caller_addr, .. } = + builder::bare_instantiate(Code::Upload(caller_wasm)) + .value(300_000) + .build_and_unwrap_contract(); + + // Instantiate the 'callee' + let Contract { addr: callee_addr, .. } = + builder::bare_instantiate(Code::Upload(callee_wasm)) + .value(100_000) + .build_and_unwrap_contract(); + + // fails, not enough weight + assert_err!( + builder::bare_call(caller_addr) + .value(1337) + .data((callee_addr, 100u64, 100u64).encode()) + .build() + .result, + Error::::ContractTrapped, + ); + + assert_ok!(builder::call(caller_addr) + .value(1337) + .data((callee_addr, 500_000_000u64, 100_000u64).encode()) .build()); }); } @@ -3696,6 +3737,12 @@ mod run_tests { .map(|c| sp_core::H256(sp_io::hashing::keccak_256(c))) .collect(); + let hash2addr = |code_hash: &H256| { + let mut addr = H160::zero(); + addr.as_bytes_mut().copy_from_slice(&code_hash.as_ref()[..20]); + addr + }; + // Define inputs with various actions to test locking / unlocking delegate_dependencies. // See the contract for more details. let noop_input = (0u32, callee_hashes[0]); @@ -3705,17 +3752,19 @@ mod run_tests { // Instantiate the caller contract with the given input. let instantiate = |input: &(u32, H256)| { + let (action, code_hash) = input; builder::bare_instantiate(Code::Upload(wasm_caller.clone())) .origin(RuntimeOrigin::signed(ETH_ALICE)) - .data(input.encode()) + .data((action, hash2addr(code_hash), code_hash).encode()) .build() }; // Call contract with the given input. let call = |addr_caller: &H160, input: &(u32, H256)| { + let (action, code_hash) = input; builder::bare_call(*addr_caller) .origin(RuntimeOrigin::signed(ETH_ALICE)) - .data(input.encode()) + .data((action, hash2addr(code_hash), code_hash).encode()) .build() }; const ED: u64 = 2000; @@ -3732,7 +3781,7 @@ mod run_tests { // Upload all the delegated codes (they all have the same size) let mut deposit = Default::default(); for code in callee_codes.iter() { - let CodeUploadReturnValue { deposit: deposit_per_code, .. } = + let CodeUploadReturnValue { deposit: deposit_per_code, code_hash } = Contracts::bare_upload_code( RuntimeOrigin::signed(ETH_ALICE), code.clone(), @@ -3740,6 +3789,9 @@ mod run_tests { ) .unwrap(); deposit = deposit_per_code; + // Mock contract info by using first 20 bytes of code_hash as address. + let addr = hash2addr(&code_hash); + ContractInfoOf::::set(&addr, ContractInfo::new(&addr, 0, code_hash).ok()); } // Instantiate should now work. @@ -3776,7 +3828,11 @@ mod run_tests { // Locking self should fail. assert_err!( - call(&addr_caller, &(1u32, self_code_hash)).result, + builder::bare_call(addr_caller) + .origin(RuntimeOrigin::signed(ETH_ALICE)) + .data((1u32, &addr_caller, self_code_hash).encode()) + .build() + .result, Error::::CannotAddSelfAsDelegateDependency ); @@ -3815,7 +3871,7 @@ mod run_tests { assert_err!( builder::bare_call(addr_caller) .storage_deposit_limit(dependency_deposit - 1) - .data(lock_delegate_dependency_input.encode()) + .data((1u32, hash2addr(&callee_hashes[0]), callee_hashes[0]).encode()) .build() .result, Error::::StorageDepositLimitExhausted diff --git a/substrate/frame/revive/uapi/src/host/riscv32.rs b/substrate/frame/revive/uapi/src/host/riscv32.rs index 3e1fc15688fe..8b8f22e140be 100644 --- a/substrate/frame/revive/uapi/src/host/riscv32.rs +++ b/substrate/frame/revive/uapi/src/host/riscv32.rs @@ -60,7 +60,7 @@ mod sys { ) -> ReturnCode; pub fn transfer(address_ptr: *const u8, value_ptr: *const u8) -> ReturnCode; pub fn call(ptr: *const u8) -> ReturnCode; - pub fn delegate_call(*const u8) -> ReturnCode; + pub fn delegate_call(ptr: *const u8) -> ReturnCode; pub fn instantiate(ptr: *const u8) -> ReturnCode; pub fn terminate(beneficiary_ptr: *const u8); pub fn input(out_ptr: *mut u8, out_len_ptr: *mut u32); From 697422b18f1172a34929101fa5752dedc1d9d0f8 Mon Sep 17 00:00:00 2001 From: Ermal Kaleci Date: Wed, 23 Oct 2024 19:21:56 +0200 Subject: [PATCH 03/18] fix test --- substrate/frame/revive/src/exec.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index c01da3074721..157767256f7f 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -4470,7 +4470,7 @@ mod tests { // An unknown code hash to fail the delegate_call on the first condition. *ctx.ext.last_frame_output_mut() = output_revert(); assert_eq!( - ctx.ext.delegate_call(invalid_code_hash, Default::default()), + ctx.ext.delegate_call(Weight::zero(), H160([0xff; 20]), Default::default()), Err(Error::::CodeNotFound.into()) ); assert_eq!(ctx.ext.last_frame_output(), &Default::default()); From 7f374c0b24c702cf96d1f9aa4d7a52f7f0e5b179 Mon Sep 17 00:00:00 2001 From: Ermal Kaleci Date: Wed, 23 Oct 2024 19:36:37 +0200 Subject: [PATCH 04/18] add prdoc --- prdoc/pr_6111.prdoc | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 prdoc/pr_6111.prdoc diff --git a/prdoc/pr_6111.prdoc b/prdoc/pr_6111.prdoc new file mode 100644 index 000000000000..08f2899b4750 --- /dev/null +++ b/prdoc/pr_6111.prdoc @@ -0,0 +1,17 @@ +title: "[pallet-revive] Update delegate_call to accept address and weight" + +doc: + - audience: Runtime Dev + description: | + Enhance the `delegate_call` function to accept an `address` target parameter instead of a `code_hash`. + This allows direct identification of the target contract using the provided address. + Additionally, introduce parameters for specifying a customizable `ref_time` limit and `proof_size` limit, + thereby improving flexibility and control during contract interactions. + +crates: + - name: pallet-revive + bump: major + - name: pallet-revive-fixtures + bump: major + - name: pallet-revive-uapi + bump: major From 4a29f0a0c6a9375b776a917b861ef653f75361ae Mon Sep 17 00:00:00 2001 From: Ermal Kaleci Date: Wed, 23 Oct 2024 20:04:20 +0200 Subject: [PATCH 05/18] fmt --- substrate/frame/revive/src/benchmarking/mod.rs | 4 +++- substrate/frame/revive/uapi/src/host.rs | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/substrate/frame/revive/src/benchmarking/mod.rs b/substrate/frame/revive/src/benchmarking/mod.rs index ebafb6c7054a..7f515f75d041 100644 --- a/substrate/frame/revive/src/benchmarking/mod.rs +++ b/substrate/frame/revive/src/benchmarking/mod.rs @@ -1501,7 +1501,9 @@ mod benchmarks { result = runtime.bench_delegate_call( memory.as_mut_slice(), 0, // flags - 0, // code_hash_ptr + 0, // address_ptr + 0, // ref_time_limit + 0, // proof_size_limit 0, // input_data_ptr 0, // input_data_len SENTINEL, // output_ptr diff --git a/substrate/frame/revive/uapi/src/host.rs b/substrate/frame/revive/uapi/src/host.rs index 4eea6c8be1a9..38319e7f1815 100644 --- a/substrate/frame/revive/uapi/src/host.rs +++ b/substrate/frame/revive/uapi/src/host.rs @@ -292,7 +292,8 @@ pub trait HostFn: private::Sealed { /// # Parameters /// /// - `flags`: See [`CallFlags`] for a documentation of the supported flags. - /// - `address`: The address of the code to be executed. Should be decodable as an `T::AccountId`. Traps otherwise. + /// - `address`: The address of the code to be executed. Should be decodable as an + /// `T::AccountId`. Traps otherwise. /// - `ref_time_limit`: how much *ref_time* Weight to devote to the execution. /// - `proof_size_limit`: how much *proof_size* Weight to devote to the execution. /// - `input`: The input data buffer used to call the contract. From 78d39c2db94cbda229176a0131b7d99703e8f942 Mon Sep 17 00:00:00 2001 From: Ermal Kaleci Date: Wed, 23 Oct 2024 20:35:13 +0200 Subject: [PATCH 06/18] fmt --- substrate/frame/revive/src/benchmarking/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/frame/revive/src/benchmarking/mod.rs b/substrate/frame/revive/src/benchmarking/mod.rs index 7f515f75d041..bafb2d9e557c 100644 --- a/substrate/frame/revive/src/benchmarking/mod.rs +++ b/substrate/frame/revive/src/benchmarking/mod.rs @@ -1502,8 +1502,8 @@ mod benchmarks { memory.as_mut_slice(), 0, // flags 0, // address_ptr - 0, // ref_time_limit - 0, // proof_size_limit + 0, // ref_time_limit + 0, // proof_size_limit 0, // input_data_ptr 0, // input_data_len SENTINEL, // output_ptr From 171167a05fba45979b5076e7dc3f96a10c85bec5 Mon Sep 17 00:00:00 2001 From: Ermal Kaleci Date: Mon, 28 Oct 2024 12:18:45 +0100 Subject: [PATCH 07/18] update prdoc --- prdoc/pr_6111.prdoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prdoc/pr_6111.prdoc b/prdoc/pr_6111.prdoc index 08f2899b4750..4ada3031c805 100644 --- a/prdoc/pr_6111.prdoc +++ b/prdoc/pr_6111.prdoc @@ -12,6 +12,6 @@ crates: - name: pallet-revive bump: major - name: pallet-revive-fixtures - bump: major + bump: patch - name: pallet-revive-uapi bump: major From f899e9e836de99b8479949ca73367903177e5913 Mon Sep 17 00:00:00 2001 From: Ermal Kaleci Date: Mon, 28 Oct 2024 14:19:42 +0100 Subject: [PATCH 08/18] add option to limit storage deposit --- .../fixtures/contracts/delegate_call.rs | 2 +- .../contracts/delegate_call_simple.rs | 2 +- .../contracts/locking_delegate_dependency.rs | 2 +- .../frame/revive/src/benchmarking/mod.rs | 28 +++++++++++++------ substrate/frame/revive/src/exec.rs | 25 +++++++++++------ substrate/frame/revive/src/wasm/runtime.rs | 13 +++++++-- substrate/frame/revive/uapi/src/host.rs | 4 +++ .../frame/revive/uapi/src/host/riscv32.rs | 5 +++- 8 files changed, 56 insertions(+), 25 deletions(-) diff --git a/substrate/frame/revive/fixtures/contracts/delegate_call.rs b/substrate/frame/revive/fixtures/contracts/delegate_call.rs index 3abb52bdc86d..3cf74acf1321 100644 --- a/substrate/frame/revive/fixtures/contracts/delegate_call.rs +++ b/substrate/frame/revive/fixtures/contracts/delegate_call.rs @@ -46,7 +46,7 @@ pub extern "C" fn call() { assert!(value[0] == 2u8); let input = [0u8; 0]; - api::delegate_call(uapi::CallFlags::empty(), address, ref_time, proof_size, &input, None).unwrap(); + api::delegate_call(uapi::CallFlags::empty(), address, ref_time, proof_size, None, &input, None).unwrap(); api::get_storage(StorageFlags::empty(), &key, value).unwrap(); assert!(value[0] == 1u8); diff --git a/substrate/frame/revive/fixtures/contracts/delegate_call_simple.rs b/substrate/frame/revive/fixtures/contracts/delegate_call_simple.rs index 74ed71921b8e..a8501dad4692 100644 --- a/substrate/frame/revive/fixtures/contracts/delegate_call_simple.rs +++ b/substrate/frame/revive/fixtures/contracts/delegate_call_simple.rs @@ -32,5 +32,5 @@ pub extern "C" fn call() { // Delegate call into passed address. let input = [0u8; 0]; - api::delegate_call(uapi::CallFlags::empty(), address, 0, 0, &input, None).unwrap(); + api::delegate_call(uapi::CallFlags::empty(), address, 0, 0, None, &input, None).unwrap(); } diff --git a/substrate/frame/revive/fixtures/contracts/locking_delegate_dependency.rs b/substrate/frame/revive/fixtures/contracts/locking_delegate_dependency.rs index 3f34a3ebea9d..3d7702c6537a 100644 --- a/substrate/frame/revive/fixtures/contracts/locking_delegate_dependency.rs +++ b/substrate/frame/revive/fixtures/contracts/locking_delegate_dependency.rs @@ -52,7 +52,7 @@ fn load_input(delegate_call: bool) { } if delegate_call { - api::delegate_call(uapi::CallFlags::empty(), address, 0, 0, &[], None).unwrap(); + api::delegate_call(uapi::CallFlags::empty(), address, 0, 0, None, &[], None).unwrap(); } } diff --git a/substrate/frame/revive/src/benchmarking/mod.rs b/substrate/frame/revive/src/benchmarking/mod.rs index 675e9d88e22b..5e88c604c150 100644 --- a/substrate/frame/revive/src/benchmarking/mod.rs +++ b/substrate/frame/revive/src/benchmarking/mod.rs @@ -1522,27 +1522,37 @@ mod benchmarks { #[benchmark(pov_mode = Measured)] fn seal_delegate_call() -> Result<(), BenchmarkError> { - let hash = Contract::::with_index(1, WasmModule::dummy(), vec![])?.info()?.code_hash; + let Contract { account_id: address, .. } = + Contract::::with_index(1, WasmModule::dummy(), vec![]).unwrap(); + + let address_bytes = address.encode(); + let address_len = address_bytes.len() as u32; + + let deposit: BalanceOf = (u32::MAX - 100).into(); + let deposit_bytes = Into::::into(deposit).encode(); + let deposit_len = deposit_bytes.len() as u32; let mut setup = CallSetup::::default(); + setup.set_storage_deposit_limit(deposit); setup.set_origin(Origin::from_account_id(setup.contract().account_id.clone())); let (mut ext, _) = setup.ext(); let mut runtime = crate::wasm::Runtime::<_, [u8]>::new(&mut ext, vec![]); - let mut memory = memory!(hash.encode(),); + let mut memory = memory!(address_bytes, deposit_bytes,); let result; #[block] { result = runtime.bench_delegate_call( memory.as_mut_slice(), - 0, // flags - 0, // address_ptr - 0, // ref_time_limit - 0, // proof_size_limit - 0, // input_data_ptr - 0, // input_data_len - SENTINEL, // output_ptr + 0, // flags + 0, // address_ptr + 0, // ref_time_limit + 0, // proof_size_limit + address_len, // deposit_ptr + 0, // input_data_ptr + 0, // input_data_len + SENTINEL, // output_ptr 0, ); } diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index 0f2471812872..5520d5c3ce7f 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -213,6 +213,7 @@ pub trait Ext: sealing::Sealed { fn delegate_call( &mut self, gas_limit: Weight, + deposit_limit: U256, address: H160, input_data: Vec, ) -> Result<(), ExecError>; @@ -1402,6 +1403,7 @@ where fn delegate_call( &mut self, gas_limit: Weight, + deposit_limit: U256, address: H160, input_data: Vec, ) -> Result<(), ExecError> { @@ -1423,7 +1425,7 @@ where }, value, gas_limit, - BalanceOf::::zero(), + deposit_limit.try_into().map_err(|_| Error::::BalanceConversionFailed)?, self.is_read_only(), )?; self.run(executable.expect(FRAME_ALWAYS_EXISTS_ON_INSTANTIATE), input_data) @@ -2072,7 +2074,8 @@ mod tests { let delegate_ch = MockLoader::insert(Call, move |ctx, _| { assert_eq!(ctx.ext.value_transferred(), U256::from(value)); - let _ = ctx.ext.delegate_call(Weight::zero(), CHARLIE_ADDR, Vec::new())?; + let _ = + ctx.ext.delegate_call(Weight::zero(), U256::zero(), CHARLIE_ADDR, Vec::new())?; Ok(ExecReturnValue { flags: ReturnFlags::empty(), data: Vec::new() }) }); @@ -2106,7 +2109,8 @@ mod tests { }); let delegate_ch = MockLoader::insert(Call, move |ctx, _| { - let _ = ctx.ext.delegate_call(Weight::zero(), CHARLIE_ADDR, Vec::new())?; + let _ = + ctx.ext.delegate_call(Weight::zero(), U256::zero(), CHARLIE_ADDR, Vec::new())?; Ok(ExecReturnValue { flags: ReturnFlags::empty(), data: Vec::new() }) }); @@ -4479,7 +4483,12 @@ mod tests { // An unknown code hash to fail the delegate_call on the first condition. *ctx.ext.last_frame_output_mut() = output_revert(); assert_eq!( - ctx.ext.delegate_call(Weight::zero(), H160([0xff; 20]), Default::default()), + ctx.ext.delegate_call( + Weight::zero(), + U256::zero(), + H160([0xff; 20]), + Default::default() + ), Err(Error::::CodeNotFound.into()) ); assert_eq!(ctx.ext.last_frame_output(), &Default::default()); @@ -4598,11 +4607,9 @@ mod tests { // In a delegate call, we should witness the caller immutable data assert_eq!( - ctx.ext.delegate_call(Weight::zero(), CHARLIE_ADDR, Vec::new()).map(|_| ctx - .ext - .last_frame_output() - .data - .clone()), + ctx.ext + .delegate_call(Weight::zero(), U256::zero(), CHARLIE_ADDR, Vec::new()) + .map(|_| ctx.ext.last_frame_output().data.clone()), Ok(vec![1]) ); diff --git a/substrate/frame/revive/src/wasm/runtime.rs b/substrate/frame/revive/src/wasm/runtime.rs index 618fa562bba1..406fea16200a 100644 --- a/substrate/frame/revive/src/wasm/runtime.rs +++ b/substrate/frame/revive/src/wasm/runtime.rs @@ -525,7 +525,7 @@ enum CallType { /// Execute another instantiated contract Call { callee_ptr: u32, value_ptr: u32, deposit_ptr: u32, weight: Weight }, /// Execute deployed code in the context (storage, account ID, value) of the caller contract - DelegateCall { address_ptr: u32, weight: Weight }, + DelegateCall { address_ptr: u32, deposit_ptr: u32, weight: Weight }, } impl CallType { @@ -1021,14 +1021,19 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { read_only, ) }, - CallType::DelegateCall { address_ptr, weight } => { + CallType::DelegateCall { address_ptr, deposit_ptr, weight } => { if flags.intersects(CallFlags::ALLOW_REENTRY | CallFlags::READ_ONLY) { return Err(Error::::InvalidCallFlags.into()); } let mut address = H160::zero(); memory.read_into_buf(address_ptr, address.as_bytes_mut())?; - self.ext.delegate_call(weight, address, input_data) + let deposit_limit = if deposit_ptr == SENTINEL { + U256::zero() + } else { + memory.read_u256(deposit_ptr)? + }; + self.ext.delegate_call(weight, deposit_limit, address, input_data) }, }; @@ -1289,6 +1294,7 @@ pub mod env { address_ptr: u32, ref_time_limit: u64, proof_size_limit: u64, + deposit_ptr: u32, input_data_ptr: u32, input_data_len: u32, output_ptr: u32, @@ -1299,6 +1305,7 @@ pub mod env { CallFlags::from_bits(flags).ok_or(Error::::InvalidCallFlags)?, CallType::DelegateCall { address_ptr, + deposit_ptr, weight: Weight::from_parts(ref_time_limit, proof_size_limit), }, input_data_ptr, diff --git a/substrate/frame/revive/uapi/src/host.rs b/substrate/frame/revive/uapi/src/host.rs index 38319e7f1815..ff5d0adc72fc 100644 --- a/substrate/frame/revive/uapi/src/host.rs +++ b/substrate/frame/revive/uapi/src/host.rs @@ -296,6 +296,9 @@ pub trait HostFn: private::Sealed { /// `T::AccountId`. Traps otherwise. /// - `ref_time_limit`: how much *ref_time* Weight to devote to the execution. /// - `proof_size_limit`: how much *proof_size* Weight to devote to the execution. + /// - `deposit`: The storage deposit limit for delegate call. Passing `None` means setting no + /// specific limit for the call, which implies storage usage up to the limit of the parent + /// call. /// - `input`: The input data buffer used to call the contract. /// - `output`: A reference to the output data buffer to write the call output buffer. If `None` /// is provided then the output buffer is not copied. @@ -313,6 +316,7 @@ pub trait HostFn: private::Sealed { address: &[u8; 20], ref_time_limit: u64, proof_size_limit: u64, + deposit: Option<&[u8; 32]>, input_data: &[u8], output: Option<&mut &mut [u8]>, ) -> Result; diff --git a/substrate/frame/revive/uapi/src/host/riscv32.rs b/substrate/frame/revive/uapi/src/host/riscv32.rs index 951c7e19e56f..f4e79e230192 100644 --- a/substrate/frame/revive/uapi/src/host/riscv32.rs +++ b/substrate/frame/revive/uapi/src/host/riscv32.rs @@ -300,11 +300,12 @@ impl HostFn for HostFnImpl { address: &[u8; 20], ref_time_limit: u64, proof_size_limit: u64, + deposit_limit: Option<&[u8; 32]>, input: &[u8], mut output: Option<&mut &mut [u8]>, ) -> Result { let (output_ptr, mut output_len) = ptr_len_or_sentinel(&mut output); - + let deposit_limit_ptr = ptr_or_sentinel(&deposit_limit); #[repr(packed)] #[allow(dead_code)] struct Args { @@ -312,6 +313,7 @@ impl HostFn for HostFnImpl { address: *const u8, ref_time_limit: u64, proof_size_limit: u64, + deposit_limit: *const u8, input: *const u8, input_len: u32, output: *mut u8, @@ -322,6 +324,7 @@ impl HostFn for HostFnImpl { address: address.as_ptr(), ref_time_limit, proof_size_limit, + deposit_limit: deposit_limit_ptr, input: input.as_ptr(), input_len: input.len() as _, output: output_ptr, From a5f2f1c8444859b1f69a0e4a050ac5a116f31cb4 Mon Sep 17 00:00:00 2001 From: Ermal Kaleci Date: Mon, 28 Oct 2024 16:45:14 +0100 Subject: [PATCH 09/18] clippy --- substrate/frame/revive/src/benchmarking/mod.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/substrate/frame/revive/src/benchmarking/mod.rs b/substrate/frame/revive/src/benchmarking/mod.rs index 5e88c604c150..d124b064bb56 100644 --- a/substrate/frame/revive/src/benchmarking/mod.rs +++ b/substrate/frame/revive/src/benchmarking/mod.rs @@ -1530,7 +1530,6 @@ mod benchmarks { let deposit: BalanceOf = (u32::MAX - 100).into(); let deposit_bytes = Into::::into(deposit).encode(); - let deposit_len = deposit_bytes.len() as u32; let mut setup = CallSetup::::default(); setup.set_storage_deposit_limit(deposit); From c05bd51aa8a696d53d8380b14e58ea6e89a436e1 Mon Sep 17 00:00:00 2001 From: Ermal Kaleci Date: Thu, 31 Oct 2024 13:31:20 +0100 Subject: [PATCH 10/18] fix immutable read on delegate call --- substrate/frame/revive/src/exec.rs | 136 +++++++++++++++++------------ 1 file changed, 80 insertions(+), 56 deletions(-) diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index 5520d5c3ce7f..2ffc90b65eaa 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -582,6 +582,8 @@ struct Frame { read_only: bool, /// The caller of the currently executing frame which was spawned by `delegate_call`. delegate_caller: Option>, + /// The address of the contract the call was delegated to. + delegate_address: Option, /// The output of the last executed call frame. last_frame_output: ExecReturnValue, } @@ -592,6 +594,8 @@ struct DelegatedCall { executable: E, /// The caller of the contract. caller: Origin, + /// The address of the contract. + address: H160, } /// Parameter passed in when creating a new `Frame`. @@ -896,64 +900,74 @@ where read_only: bool, origin_is_caller: bool, ) -> Result, E)>, ExecError> { - let (account_id, contract_info, executable, delegate_caller, entry_point) = match frame_args - { - FrameArgs::Call { dest, cached_info, delegated_call } => { - let contract = if let Some(contract) = cached_info { - contract - } else { - if let Some(contract) = - >::get(T::AddressMapper::to_address(&dest)) - { + let (account_id, contract_info, executable, delegate_caller, delegate_address, entry_point) = + match frame_args { + FrameArgs::Call { dest, cached_info, delegated_call } => { + let contract = if let Some(contract) = cached_info { contract } else { - return Ok(None); - } - }; - - let (executable, delegate_caller) = - if let Some(DelegatedCall { executable, caller }) = delegated_call { - (executable, Some(caller)) - } else { - (E::from_storage(contract.code_hash, gas_meter)?, None) + if let Some(contract) = + >::get(T::AddressMapper::to_address(&dest)) + { + contract + } else { + return Ok(None); + } }; - (dest, contract, executable, delegate_caller, ExportedFunction::Call) - }, - FrameArgs::Instantiate { sender, executable, salt, input_data } => { - let deployer = T::AddressMapper::to_address(&sender); - let account_nonce = >::account_nonce(&sender); - let address = if let Some(salt) = salt { - address::create2(&deployer, executable.code(), input_data, salt) - } else { - use sp_runtime::Saturating; - address::create1( - &deployer, - // the Nonce from the origin has been incremented pre-dispatch, so we need - // to subtract 1 to get the nonce at the time of the call. - if origin_is_caller { - account_nonce.saturating_sub(1u32.into()).saturated_into() + let (executable, delegate_caller, delegate_address) = + if let Some(DelegatedCall { executable, caller, address }) = delegated_call + { + (executable, Some(caller), Some(address)) } else { - account_nonce.saturated_into() - }, + (E::from_storage(contract.code_hash, gas_meter)?, None, None) + }; + + ( + dest, + contract, + executable, + delegate_caller, + delegate_address, + ExportedFunction::Call, ) - }; - let contract = ContractInfo::new( - &address, - >::account_nonce(&sender), - *executable.code_hash(), - )?; - ( - T::AddressMapper::to_fallback_account_id(&address), - contract, - executable, - None, - ExportedFunction::Constructor, - ) - }, - }; + }, + FrameArgs::Instantiate { sender, executable, salt, input_data } => { + let deployer = T::AddressMapper::to_address(&sender); + let account_nonce = >::account_nonce(&sender); + let address = if let Some(salt) = salt { + address::create2(&deployer, executable.code(), input_data, salt) + } else { + use sp_runtime::Saturating; + address::create1( + &deployer, + // the Nonce from the origin has been incremented pre-dispatch, so we + // need to subtract 1 to get the nonce at the time of the call. + if origin_is_caller { + account_nonce.saturating_sub(1u32.into()).saturated_into() + } else { + account_nonce.saturated_into() + }, + ) + }; + let contract = ContractInfo::new( + &address, + >::account_nonce(&sender), + *executable.code_hash(), + )?; + ( + T::AddressMapper::to_fallback_account_id(&address), + contract, + executable, + None, + None, + ExportedFunction::Constructor, + ) + }, + }; let frame = Frame { + delegate_address, delegate_caller, value_transferred, contract_info: CachedContract::Cached(contract_info), @@ -1411,8 +1425,10 @@ where // This is for example the case for unknown code hashes or creating the frame fails. *self.last_frame_output_mut() = Default::default(); - let contract_info = ContractInfoOf::::get(&address).ok_or(Error::::CodeNotFound)?; - let executable = E::from_storage(contract_info.code_hash, self.gas_meter_mut())?; + let code_hash = ContractInfoOf::::get(&address) + .ok_or(Error::::CodeNotFound) + .map(|c| c.code_hash)?; + let executable = E::from_storage(code_hash, self.gas_meter_mut())?; let top_frame = self.top_frame_mut(); let contract_info = top_frame.contract_info().clone(); let account_id = top_frame.account_id.clone(); @@ -1421,7 +1437,11 @@ where FrameArgs::Call { dest: account_id, cached_info: Some(contract_info), - delegated_call: Some(DelegatedCall { executable, caller: self.caller().clone() }), + delegated_call: Some(DelegatedCall { + executable, + caller: self.caller().clone(), + address, + }), }, value, gas_limit, @@ -1593,7 +1613,11 @@ where return Err(Error::::InvalidImmutableAccess.into()); } - let address = T::AddressMapper::to_address(self.account_id()); + // Immutable is read from contract code being executed + let address = self + .top_frame() + .delegate_address + .unwrap_or(T::AddressMapper::to_address(self.account_id())); Ok(>::get(address).ok_or_else(|| Error::::InvalidImmutableAccess)?) } @@ -4605,12 +4629,12 @@ mod tests { Ok(vec![2]), ); - // In a delegate call, we should witness the caller immutable data + // Also in a delegate call, we should witness the callee immutable data assert_eq!( ctx.ext .delegate_call(Weight::zero(), U256::zero(), CHARLIE_ADDR, Vec::new()) .map(|_| ctx.ext.last_frame_output().data.clone()), - Ok(vec![1]) + Ok(vec![2]) ); exec_success() From 46af335eb50ac5c678b99814401f744610fe9d28 Mon Sep 17 00:00:00 2001 From: Ermal Kaleci Date: Thu, 31 Oct 2024 17:49:57 +0100 Subject: [PATCH 11/18] refactor --- substrate/frame/revive/src/exec.rs | 200 ++++++++++++++--------------- 1 file changed, 100 insertions(+), 100 deletions(-) diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index fa0079d63f85..89e50106a910 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -574,6 +574,10 @@ pub struct Stack<'a, T: Config, E> { /// For each nested contract call or instantiate one frame is created. It holds specific /// information for the said call and caches the in-storage `ContractInfo` data structure. struct Frame { + /// The address who instantiated the call. + caller: Origin, + /// The address of contract code being executed. + callee: H160, /// The address of the executing contract. account_id: T::AccountId, /// The cached in-storage data of the contract. @@ -590,22 +594,22 @@ struct Frame { allows_reentry: bool, /// If `true` subsequent calls cannot modify storage. read_only: bool, - /// The caller of the currently executing frame which was spawned by `delegate_call`. - delegate_caller: Option>, - /// The address of the contract the call was delegated to. - delegate_address: Option, /// The output of the last executed call frame. last_frame_output: ExecReturnValue, } -/// Used in a delegate call frame arguments in order to override the executable and caller. -struct DelegatedCall { +impl Frame { + pub fn is_delegate_call(&self) -> bool { + self.account_id != T::AddressMapper::to_account_id(&self.callee) + } +} + +/// Used in a delegate call frame arguments in order to override the executable and callee. +struct DelegatedCall { /// The executable which is run instead of the contracts own `executable`. executable: E, - /// The caller of the contract. - caller: Origin, /// The address of the contract. - address: H160, + callee: H160, } /// Parameter passed in when creating a new `Frame`. @@ -613,18 +617,20 @@ struct DelegatedCall { /// It determines whether the new frame is for a call or an instantiate. enum FrameArgs<'a, T: Config, E> { Call { + /// The contract or signed origin calling the contract. + caller: Origin, /// The account id of the contract that is to be called. dest: T::AccountId, /// If `None` the contract info needs to be reloaded from storage. cached_info: Option>, /// This frame was created by `seal_delegate_call` and hence uses different code than - /// what is stored at [`Self::Call::dest`]. Its caller ([`DelegatedCall::caller`]) is the - /// account which called the caller contract - delegated_call: Option>, + /// what is stored at [`Self::Call::dest`] and the corresponding address + /// [`Self::Call::callee`]. + delegated_call: Option>, }, Instantiate { /// The contract or signed origin which instantiates the new contract. - sender: T::AccountId, + caller: Origin, /// The executable whose `deploy` function is run. executable: E, /// A salt used in the contract address derivation of the new contract. @@ -776,7 +782,12 @@ where ) -> ExecResult { let dest = T::AddressMapper::to_account_id(&dest); if let Some((mut stack, executable)) = Self::new( - FrameArgs::Call { dest: dest.clone(), cached_info: None, delegated_call: None }, + FrameArgs::Call { + caller: origin.clone(), + dest: dest.clone(), + cached_info: None, + delegated_call: None, + }, origin.clone(), gas_meter, storage_meter, @@ -811,7 +822,7 @@ where ) -> Result<(H160, ExecReturnValue), ExecError> { let (mut stack, executable) = Self::new( FrameArgs::Instantiate { - sender: origin.clone(), + caller: Origin::from_account_id(origin.clone()), executable, salt, input_data: input_data.as_ref(), @@ -840,6 +851,7 @@ where ) -> (Self, E) { Self::new( FrameArgs::Call { + caller: origin.clone(), dest: T::AddressMapper::to_account_id(&dest), cached_info: None, delegated_call: None, @@ -911,75 +923,71 @@ where read_only: bool, origin_is_caller: bool, ) -> Result, E)>, ExecError> { - let (account_id, contract_info, executable, delegate_caller, delegate_address, entry_point) = - match frame_args { - FrameArgs::Call { dest, cached_info, delegated_call } => { - let contract = if let Some(contract) = cached_info { + let (caller, callee, account_id, contract_info, executable, entry_point) = match frame_args + { + FrameArgs::Call { caller, dest, cached_info, delegated_call } => { + let contract = if let Some(contract) = cached_info { + contract + } else { + if let Some(contract) = + >::get(T::AddressMapper::to_address(&dest)) + { contract } else { - if let Some(contract) = - >::get(T::AddressMapper::to_address(&dest)) - { - contract - } else { - return Ok(None); - } - }; - - let (executable, delegate_caller, delegate_address) = - if let Some(DelegatedCall { executable, caller, address }) = delegated_call - { - (executable, Some(caller), Some(address)) - } else { - (E::from_storage(contract.code_hash, gas_meter)?, None, None) - }; + return Ok(None); + } + }; - ( - dest, - contract, - executable, - delegate_caller, - delegate_address, - ExportedFunction::Call, - ) - }, - FrameArgs::Instantiate { sender, executable, salt, input_data } => { - let deployer = T::AddressMapper::to_address(&sender); - let account_nonce = >::account_nonce(&sender); - let address = if let Some(salt) = salt { - address::create2(&deployer, executable.code(), input_data, salt) + let (callee, executable) = + if let Some(DelegatedCall { executable, callee }) = delegated_call { + (callee, executable) } else { - use sp_runtime::Saturating; - address::create1( - &deployer, - // the Nonce from the origin has been incremented pre-dispatch, so we - // need to subtract 1 to get the nonce at the time of the call. - if origin_is_caller { - account_nonce.saturating_sub(1u32.into()).saturated_into() - } else { - account_nonce.saturated_into() - }, + ( + T::AddressMapper::to_address(&dest), + E::from_storage(contract.code_hash, gas_meter)?, ) }; - let contract = ContractInfo::new( - &address, - >::account_nonce(&sender), - *executable.code_hash(), - )?; - ( - T::AddressMapper::to_fallback_account_id(&address), - contract, - executable, - None, - None, - ExportedFunction::Constructor, + + (caller, callee, dest, contract, executable, ExportedFunction::Call) + }, + FrameArgs::Instantiate { caller, executable, salt, input_data } => { + let caller_account_id = caller.account_id()?; + let deployer = T::AddressMapper::to_address(&caller_account_id); + let account_nonce = >::account_nonce(&caller_account_id); + let address = if let Some(salt) = salt { + address::create2(&deployer, executable.code(), input_data, salt) + } else { + use sp_runtime::Saturating; + address::create1( + &deployer, + // the Nonce from the origin has been incremented pre-dispatch, so we + // need to subtract 1 to get the nonce at the time of the call. + if origin_is_caller { + account_nonce.saturating_sub(1u32.into()).saturated_into() + } else { + account_nonce.saturated_into() + }, ) - }, - }; + }; + let contract = ContractInfo::new( + &address, + >::account_nonce(&caller_account_id), + *executable.code_hash(), + )?; + ( + caller, + address, + T::AddressMapper::to_fallback_account_id(&address), + contract, + executable, + ExportedFunction::Constructor, + ) + }, + }; let frame = Frame { - delegate_address, - delegate_caller, + caller, + callee, value_transferred, contract_info: CachedContract::Cached(contract_info), account_id, @@ -1048,7 +1056,7 @@ where let frame = self.top_frame(); let entry_point = frame.entry_point; let delegated_code_hash = - if frame.delegate_caller.is_some() { Some(*executable.code_hash()) } else { None }; + if frame.is_delegate_call() { Some(*executable.code_hash()) } else { None }; // The output of the caller frame will be replaced by the output of this run. // It is also not accessible from nested frames. @@ -1422,7 +1430,12 @@ where _ => None, }); if let Some(executable) = self.push_frame( - FrameArgs::Call { dest: dest.clone(), cached_info, delegated_call: None }, + FrameArgs::Call { + caller: Origin::from_account_id(self.account_id().clone()), + dest: dest.clone(), + cached_info, + delegated_call: None, + }, value, gas_limit, deposit_limit.try_into().map_err(|_| Error::::BalanceConversionFailed)?, @@ -1465,13 +1478,10 @@ where let value = top_frame.value_transferred; let executable = self.push_frame( FrameArgs::Call { + caller: self.caller().clone(), dest: account_id, cached_info: Some(contract_info), - delegated_call: Some(DelegatedCall { - executable, - caller: self.caller().clone(), - address, - }), + delegated_call: Some(DelegatedCall { executable, callee: address }), }, value, gas_limit, @@ -1498,7 +1508,7 @@ where let sender = &self.top_frame().account_id; let executable = self.push_frame( FrameArgs::Instantiate { - sender: sender.clone(), + caller: Origin::from_account_id(sender.clone()), executable, salt, input_data: input_data.as_ref(), @@ -1600,14 +1610,7 @@ where } fn caller(&self) -> Origin { - if let Some(caller) = &self.top_frame().delegate_caller { - caller.clone() - } else { - self.frames() - .nth(1) - .map(|f| Origin::from_account_id(f.account_id.clone())) - .unwrap_or(self.origin.clone()) - } + self.top_frame().caller.clone() } fn origin(&self) -> &Origin { @@ -1654,11 +1657,7 @@ where return Err(Error::::InvalidImmutableAccess.into()); } - // Immutable is read from contract code being executed - let address = self - .top_frame() - .delegate_address - .unwrap_or(T::AddressMapper::to_address(self.account_id())); + let address = self.top_frame().callee; Ok(>::get(address).ok_or_else(|| Error::::InvalidImmutableAccess)?) } @@ -1667,12 +1666,13 @@ where return Err(Error::::InvalidImmutableAccess.into()); } - let account_id = self.account_id().clone(); - let len = data.len() as u32; - let amount = self.top_frame_mut().contract_info().set_immutable_data_len(len)?; + let address = self.top_frame().callee; + let account_id = T::AddressMapper::to_account_id(&address); + let amount = + self.top_frame_mut().contract_info().set_immutable_data_len(data.len() as u32)?; self.top_frame_mut().nested_storage.charge_deposit(account_id.clone(), amount); - >::insert(T::AddressMapper::to_address(&account_id), &data); + >::insert(address, &data); Ok(()) } From 63844ee5bdd9c845d0710c0e5b897a0d32a7e037 Mon Sep 17 00:00:00 2001 From: Ermal Kaleci Date: Thu, 31 Oct 2024 18:20:05 +0100 Subject: [PATCH 12/18] fmt --- substrate/frame/revive/src/tests.rs | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/substrate/frame/revive/src/tests.rs b/substrate/frame/revive/src/tests.rs index 9e95253788ed..40538cf8ca71 100644 --- a/substrate/frame/revive/src/tests.rs +++ b/substrate/frame/revive/src/tests.rs @@ -1170,17 +1170,17 @@ fn delegate_call_with_limits() { // fails, not enough weight assert_err!( - builder::bare_call(caller_addr) - .value(1337) - .data((callee_addr, 100u64, 100u64).encode()) - .build() - .result, - Error::::ContractTrapped, - ); + builder::bare_call(caller_addr) + .value(1337) + .data((callee_addr, 100u64, 100u64).encode()) + .build() + .result, + Error::::ContractTrapped, + ); assert_ok!(builder::call(caller_addr) - .value(1337) - .data((callee_addr, 500_000_000u64, 100_000u64).encode()) + .value(1337) + .data((callee_addr, 500_000_000u64, 100_000u64).encode()) .build()); }); } @@ -3808,10 +3808,10 @@ fn locking_delegate_dependency_works() { // Locking self should fail. assert_err!( builder::bare_call(addr_caller) - .origin(RuntimeOrigin::signed(ALICE_FALLBACK)) - .data((1u32, &addr_caller, self_code_hash).encode()) - .build() - .result, + .origin(RuntimeOrigin::signed(ALICE_FALLBACK)) + .data((1u32, &addr_caller, self_code_hash).encode()) + .build() + .result, Error::::CannotAddSelfAsDelegateDependency ); From bb79cda8a81b87c928a43376528e1e283c3e84fe Mon Sep 17 00:00:00 2001 From: Ermal Kaleci Date: Thu, 31 Oct 2024 19:18:32 +0100 Subject: [PATCH 13/18] Revert "refactor" This reverts commit 46af335eb50ac5c678b99814401f744610fe9d28. --- substrate/frame/revive/src/exec.rs | 200 ++++++++++++++--------------- 1 file changed, 100 insertions(+), 100 deletions(-) diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index 6fa8bf063471..d7077630dfe6 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -574,10 +574,6 @@ pub struct Stack<'a, T: Config, E> { /// For each nested contract call or instantiate one frame is created. It holds specific /// information for the said call and caches the in-storage `ContractInfo` data structure. struct Frame { - /// The address who instantiated the call. - caller: Origin, - /// The address of contract code being executed. - callee: H160, /// The address of the executing contract. account_id: T::AccountId, /// The cached in-storage data of the contract. @@ -594,22 +590,22 @@ struct Frame { allows_reentry: bool, /// If `true` subsequent calls cannot modify storage. read_only: bool, + /// The caller of the currently executing frame which was spawned by `delegate_call`. + delegate_caller: Option>, + /// The address of the contract the call was delegated to. + delegate_address: Option, /// The output of the last executed call frame. last_frame_output: ExecReturnValue, } -impl Frame { - pub fn is_delegate_call(&self) -> bool { - self.account_id != T::AddressMapper::to_account_id(&self.callee) - } -} - -/// Used in a delegate call frame arguments in order to override the executable and callee. -struct DelegatedCall { +/// Used in a delegate call frame arguments in order to override the executable and caller. +struct DelegatedCall { /// The executable which is run instead of the contracts own `executable`. executable: E, + /// The caller of the contract. + caller: Origin, /// The address of the contract. - callee: H160, + address: H160, } /// Parameter passed in when creating a new `Frame`. @@ -617,20 +613,18 @@ struct DelegatedCall { /// It determines whether the new frame is for a call or an instantiate. enum FrameArgs<'a, T: Config, E> { Call { - /// The contract or signed origin calling the contract. - caller: Origin, /// The account id of the contract that is to be called. dest: T::AccountId, /// If `None` the contract info needs to be reloaded from storage. cached_info: Option>, /// This frame was created by `seal_delegate_call` and hence uses different code than - /// what is stored at [`Self::Call::dest`] and the corresponding address - /// [`Self::Call::callee`]. - delegated_call: Option>, + /// what is stored at [`Self::Call::dest`]. Its caller ([`DelegatedCall::caller`]) is the + /// account which called the caller contract + delegated_call: Option>, }, Instantiate { /// The contract or signed origin which instantiates the new contract. - caller: Origin, + sender: T::AccountId, /// The executable whose `deploy` function is run. executable: E, /// A salt used in the contract address derivation of the new contract. @@ -782,12 +776,7 @@ where ) -> ExecResult { let dest = T::AddressMapper::to_account_id(&dest); if let Some((mut stack, executable)) = Self::new( - FrameArgs::Call { - caller: origin.clone(), - dest: dest.clone(), - cached_info: None, - delegated_call: None, - }, + FrameArgs::Call { dest: dest.clone(), cached_info: None, delegated_call: None }, origin.clone(), gas_meter, storage_meter, @@ -822,7 +811,7 @@ where ) -> Result<(H160, ExecReturnValue), ExecError> { let (mut stack, executable) = Self::new( FrameArgs::Instantiate { - caller: Origin::from_account_id(origin.clone()), + sender: origin.clone(), executable, salt, input_data: input_data.as_ref(), @@ -851,7 +840,6 @@ where ) -> (Self, E) { Self::new( FrameArgs::Call { - caller: origin.clone(), dest: T::AddressMapper::to_account_id(&dest), cached_info: None, delegated_call: None, @@ -923,71 +911,75 @@ where read_only: bool, origin_is_caller: bool, ) -> Result, E)>, ExecError> { - let (caller, callee, account_id, contract_info, executable, entry_point) = match frame_args - { - FrameArgs::Call { caller, dest, cached_info, delegated_call } => { - let contract = if let Some(contract) = cached_info { - contract - } else { - if let Some(contract) = - >::get(T::AddressMapper::to_address(&dest)) - { + let (account_id, contract_info, executable, delegate_caller, delegate_address, entry_point) = + match frame_args { + FrameArgs::Call { dest, cached_info, delegated_call } => { + let contract = if let Some(contract) = cached_info { contract } else { - return Ok(None); - } - }; + if let Some(contract) = + >::get(T::AddressMapper::to_address(&dest)) + { + contract + } else { + return Ok(None); + } + }; + + let (executable, delegate_caller, delegate_address) = + if let Some(DelegatedCall { executable, caller, address }) = delegated_call + { + (executable, Some(caller), Some(address)) + } else { + (E::from_storage(contract.code_hash, gas_meter)?, None, None) + }; - let (callee, executable) = - if let Some(DelegatedCall { executable, callee }) = delegated_call { - (callee, executable) + ( + dest, + contract, + executable, + delegate_caller, + delegate_address, + ExportedFunction::Call, + ) + }, + FrameArgs::Instantiate { sender, executable, salt, input_data } => { + let deployer = T::AddressMapper::to_address(&sender); + let account_nonce = >::account_nonce(&sender); + let address = if let Some(salt) = salt { + address::create2(&deployer, executable.code(), input_data, salt) } else { - ( - T::AddressMapper::to_address(&dest), - E::from_storage(contract.code_hash, gas_meter)?, + use sp_runtime::Saturating; + address::create1( + &deployer, + // the Nonce from the origin has been incremented pre-dispatch, so we + // need to subtract 1 to get the nonce at the time of the call. + if origin_is_caller { + account_nonce.saturating_sub(1u32.into()).saturated_into() + } else { + account_nonce.saturated_into() + }, ) }; - - (caller, callee, dest, contract, executable, ExportedFunction::Call) - }, - FrameArgs::Instantiate { caller, executable, salt, input_data } => { - let caller_account_id = caller.account_id()?; - let deployer = T::AddressMapper::to_address(&caller_account_id); - let account_nonce = >::account_nonce(&caller_account_id); - let address = if let Some(salt) = salt { - address::create2(&deployer, executable.code(), input_data, salt) - } else { - use sp_runtime::Saturating; - address::create1( - &deployer, - // the Nonce from the origin has been incremented pre-dispatch, so we - // need to subtract 1 to get the nonce at the time of the call. - if origin_is_caller { - account_nonce.saturating_sub(1u32.into()).saturated_into() - } else { - account_nonce.saturated_into() - }, + let contract = ContractInfo::new( + &address, + >::account_nonce(&sender), + *executable.code_hash(), + )?; + ( + T::AddressMapper::to_fallback_account_id(&address), + contract, + executable, + None, + None, + ExportedFunction::Constructor, ) - }; - let contract = ContractInfo::new( - &address, - >::account_nonce(&caller_account_id), - *executable.code_hash(), - )?; - ( - caller, - address, - T::AddressMapper::to_fallback_account_id(&address), - contract, - executable, - ExportedFunction::Constructor, - ) - }, - }; + }, + }; let frame = Frame { - caller, - callee, + delegate_address, + delegate_caller, value_transferred, contract_info: CachedContract::Cached(contract_info), account_id, @@ -1056,7 +1048,7 @@ where let frame = self.top_frame(); let entry_point = frame.entry_point; let delegated_code_hash = - if frame.is_delegate_call() { Some(*executable.code_hash()) } else { None }; + if frame.delegate_caller.is_some() { Some(*executable.code_hash()) } else { None }; // The output of the caller frame will be replaced by the output of this run. // It is also not accessible from nested frames. @@ -1430,12 +1422,7 @@ where _ => None, }); if let Some(executable) = self.push_frame( - FrameArgs::Call { - caller: Origin::from_account_id(self.account_id().clone()), - dest: dest.clone(), - cached_info, - delegated_call: None, - }, + FrameArgs::Call { dest: dest.clone(), cached_info, delegated_call: None }, value, gas_limit, deposit_limit.try_into().map_err(|_| Error::::BalanceConversionFailed)?, @@ -1478,10 +1465,13 @@ where let value = top_frame.value_transferred; let executable = self.push_frame( FrameArgs::Call { - caller: self.caller().clone(), dest: account_id, cached_info: Some(contract_info), - delegated_call: Some(DelegatedCall { executable, callee: address }), + delegated_call: Some(DelegatedCall { + executable, + caller: self.caller().clone(), + address, + }), }, value, gas_limit, @@ -1508,7 +1498,7 @@ where let sender = &self.top_frame().account_id; let executable = self.push_frame( FrameArgs::Instantiate { - caller: Origin::from_account_id(sender.clone()), + sender: sender.clone(), executable, salt, input_data: input_data.as_ref(), @@ -1610,7 +1600,14 @@ where } fn caller(&self) -> Origin { - self.top_frame().caller.clone() + if let Some(caller) = &self.top_frame().delegate_caller { + caller.clone() + } else { + self.frames() + .nth(1) + .map(|f| Origin::from_account_id(f.account_id.clone())) + .unwrap_or(self.origin.clone()) + } } fn origin(&self) -> &Origin { @@ -1657,7 +1654,11 @@ where return Err(Error::::InvalidImmutableAccess.into()); } - let address = self.top_frame().callee; + // Immutable is read from contract code being executed + let address = self + .top_frame() + .delegate_address + .unwrap_or(T::AddressMapper::to_address(self.account_id())); Ok(>::get(address).ok_or_else(|| Error::::InvalidImmutableAccess)?) } @@ -1666,13 +1667,12 @@ where return Err(Error::::InvalidImmutableAccess.into()); } - let address = self.top_frame().callee; - let account_id = T::AddressMapper::to_account_id(&address); - let amount = - self.top_frame_mut().contract_info().set_immutable_data_len(data.len() as u32)?; + let account_id = self.account_id().clone(); + let len = data.len() as u32; + let amount = self.top_frame_mut().contract_info().set_immutable_data_len(len)?; self.top_frame_mut().nested_storage.charge_deposit(account_id.clone(), amount); - >::insert(address, &data); + >::insert(T::AddressMapper::to_address(&account_id), &data); Ok(()) } From fc6a762c3441c769daecea6838371eebcee170b3 Mon Sep 17 00:00:00 2001 From: Ermal Kaleci Date: Fri, 1 Nov 2024 09:53:07 +0100 Subject: [PATCH 14/18] merge caller and callee into delegate info --- substrate/frame/revive/src/exec.rs | 141 ++++++++++++++--------------- 1 file changed, 70 insertions(+), 71 deletions(-) diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index d7077630dfe6..1859ebedf948 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -590,22 +590,30 @@ struct Frame { allows_reentry: bool, /// If `true` subsequent calls cannot modify storage. read_only: bool, - /// The caller of the currently executing frame which was spawned by `delegate_call`. - delegate_caller: Option>, - /// The address of the contract the call was delegated to. - delegate_address: Option, + /// The delegate call info of the currently executing frame which was spawned by + /// `delegate_call`. + delegate: Option>, /// The output of the last executed call frame. last_frame_output: ExecReturnValue, } +/// This structure is used to represent the arguments in a delegate call frame in order to +/// distinguish who delegated the call and where it was delegated to. +struct DelegateInfo { + /// The caller of the contract. + pub caller: Origin, + /// The address of the contract the call was delegated to. + pub callee: H160, +} + /// Used in a delegate call frame arguments in order to override the executable and caller. struct DelegatedCall { /// The executable which is run instead of the contracts own `executable`. executable: E, /// The caller of the contract. caller: Origin, - /// The address of the contract. - address: H160, + /// The address of the contract the call was delegated to. + callee: H160, } /// Parameter passed in when creating a new `Frame`. @@ -911,75 +919,64 @@ where read_only: bool, origin_is_caller: bool, ) -> Result, E)>, ExecError> { - let (account_id, contract_info, executable, delegate_caller, delegate_address, entry_point) = - match frame_args { - FrameArgs::Call { dest, cached_info, delegated_call } => { - let contract = if let Some(contract) = cached_info { + let (account_id, contract_info, executable, delegate, entry_point) = match frame_args { + FrameArgs::Call { dest, cached_info, delegated_call } => { + let contract = if let Some(contract) = cached_info { + contract + } else { + if let Some(contract) = + >::get(T::AddressMapper::to_address(&dest)) + { contract } else { - if let Some(contract) = - >::get(T::AddressMapper::to_address(&dest)) - { - contract - } else { - return Ok(None); - } - }; - - let (executable, delegate_caller, delegate_address) = - if let Some(DelegatedCall { executable, caller, address }) = delegated_call - { - (executable, Some(caller), Some(address)) - } else { - (E::from_storage(contract.code_hash, gas_meter)?, None, None) - }; + return Ok(None); + } + }; - ( - dest, - contract, - executable, - delegate_caller, - delegate_address, - ExportedFunction::Call, - ) - }, - FrameArgs::Instantiate { sender, executable, salt, input_data } => { - let deployer = T::AddressMapper::to_address(&sender); - let account_nonce = >::account_nonce(&sender); - let address = if let Some(salt) = salt { - address::create2(&deployer, executable.code(), input_data, salt) + let (executable, delegate_caller) = + if let Some(DelegatedCall { executable, caller, callee }) = delegated_call { + (executable, Some(DelegateInfo { caller, callee })) } else { - use sp_runtime::Saturating; - address::create1( - &deployer, - // the Nonce from the origin has been incremented pre-dispatch, so we - // need to subtract 1 to get the nonce at the time of the call. - if origin_is_caller { - account_nonce.saturating_sub(1u32.into()).saturated_into() - } else { - account_nonce.saturated_into() - }, - ) + (E::from_storage(contract.code_hash, gas_meter)?, None) }; - let contract = ContractInfo::new( - &address, - >::account_nonce(&sender), - *executable.code_hash(), - )?; - ( - T::AddressMapper::to_fallback_account_id(&address), - contract, - executable, - None, - None, - ExportedFunction::Constructor, + + (dest, contract, executable, delegate_caller, ExportedFunction::Call) + }, + FrameArgs::Instantiate { sender, executable, salt, input_data } => { + let deployer = T::AddressMapper::to_address(&sender); + let account_nonce = >::account_nonce(&sender); + let address = if let Some(salt) = salt { + address::create2(&deployer, executable.code(), input_data, salt) + } else { + use sp_runtime::Saturating; + address::create1( + &deployer, + // the Nonce from the origin has been incremented pre-dispatch, so we + // need to subtract 1 to get the nonce at the time of the call. + if origin_is_caller { + account_nonce.saturating_sub(1u32.into()).saturated_into() + } else { + account_nonce.saturated_into() + }, ) - }, - }; + }; + let contract = ContractInfo::new( + &address, + >::account_nonce(&sender), + *executable.code_hash(), + )?; + ( + T::AddressMapper::to_fallback_account_id(&address), + contract, + executable, + None, + ExportedFunction::Constructor, + ) + }, + }; let frame = Frame { - delegate_address, - delegate_caller, + delegate, value_transferred, contract_info: CachedContract::Cached(contract_info), account_id, @@ -1048,7 +1045,7 @@ where let frame = self.top_frame(); let entry_point = frame.entry_point; let delegated_code_hash = - if frame.delegate_caller.is_some() { Some(*executable.code_hash()) } else { None }; + if frame.delegate.is_some() { Some(*executable.code_hash()) } else { None }; // The output of the caller frame will be replaced by the output of this run. // It is also not accessible from nested frames. @@ -1470,7 +1467,7 @@ where delegated_call: Some(DelegatedCall { executable, caller: self.caller().clone(), - address, + callee: address, }), }, value, @@ -1600,7 +1597,7 @@ where } fn caller(&self) -> Origin { - if let Some(caller) = &self.top_frame().delegate_caller { + if let Some(DelegateInfo { caller, .. }) = &self.top_frame().delegate { caller.clone() } else { self.frames() @@ -1657,7 +1654,9 @@ where // Immutable is read from contract code being executed let address = self .top_frame() - .delegate_address + .delegate + .as_ref() + .map(|d| d.callee) .unwrap_or(T::AddressMapper::to_address(self.account_id())); Ok(>::get(address).ok_or_else(|| Error::::InvalidImmutableAccess)?) } From 7073ee3e0434dcacd605f256fc0b918cf1a91988 Mon Sep 17 00:00:00 2001 From: Ermal Kaleci Date: Wed, 6 Nov 2024 13:53:03 +0100 Subject: [PATCH 15/18] apply suggestion --- substrate/frame/revive/src/exec.rs | 22 +++------ substrate/frame/revive/src/wasm/runtime.rs | 55 +++++++++------------- substrate/frame/revive/uapi/src/host.rs | 4 +- 3 files changed, 32 insertions(+), 49 deletions(-) diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index 1859ebedf948..8ce8737c0e96 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -1118,7 +1118,13 @@ where frame.nested_storage.enforce_limit(contract)?; } - let frame = self.top_frame(); + let frame = self.top_frame_mut(); + + // If a special limit was set for the sub-call, we enforce it here. + // The sub-call will be rolled back in case the limit is exhausted. + let contract = frame.contract_info.as_contract(); + frame.nested_storage.enforce_subcall_limit(contract)?; + let account_id = T::AddressMapper::to_address(&frame.account_id); match (entry_point, delegated_code_hash) { (ExportedFunction::Constructor, _) => { @@ -1127,15 +1133,7 @@ where return Err(Error::::TerminatedInConstructor.into()); } - // If a special limit was set for the sub-call, we enforce it here. - // This is needed because contract constructor might write to storage. - // The sub-call will be rolled back in case the limit is exhausted. - let frame = self.top_frame_mut(); - let contract = frame.contract_info.as_contract(); - frame.nested_storage.enforce_subcall_limit(contract)?; - let caller = T::AddressMapper::to_address(self.caller().account_id()?); - // Deposit an instantiation event. Contracts::::deposit_event(Event::Instantiated { deployer: caller, @@ -1149,12 +1147,6 @@ where }); }, (ExportedFunction::Call, None) => { - // If a special limit was set for the sub-call, we enforce it here. - // The sub-call will be rolled back in case the limit is exhausted. - let frame = self.top_frame_mut(); - let contract = frame.contract_info.as_contract(); - frame.nested_storage.enforce_subcall_limit(contract)?; - let caller = self.caller(); Contracts::::deposit_event(Event::Called { caller: caller.clone(), diff --git a/substrate/frame/revive/src/wasm/runtime.rs b/substrate/frame/revive/src/wasm/runtime.rs index eeb3b75f45c1..aa262f52db6e 100644 --- a/substrate/frame/revive/src/wasm/runtime.rs +++ b/substrate/frame/revive/src/wasm/runtime.rs @@ -539,16 +539,17 @@ macro_rules! charge_gas { /// The kind of call that should be performed. enum CallType { /// Execute another instantiated contract - Call { callee_ptr: u32, value_ptr: u32, deposit_ptr: u32, weight: Weight }, - /// Execute deployed code in the context (storage, account ID, value) of the caller contract - DelegateCall { address_ptr: u32, deposit_ptr: u32, weight: Weight }, + Call { value_ptr: u32 }, + /// Execute another contract code in the context (storage, account ID, value) of the caller + /// contract + DelegateCall, } impl CallType { fn cost(&self) -> RuntimeCosts { match self { CallType::Call { .. } => RuntimeCosts::CallBase, - CallType::DelegateCall { .. } => RuntimeCosts::DelegateCallBase, + CallType::DelegateCall => RuntimeCosts::DelegateCallBase, } } } @@ -990,6 +991,9 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { memory: &mut M, flags: CallFlags, call_type: CallType, + callee_ptr: u32, + deposit_ptr: u32, + weight: Weight, input_data_ptr: u32, input_data_len: u32, output_ptr: u32, @@ -997,6 +1001,10 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { ) -> Result { self.charge_gas(call_type.cost())?; + let callee = memory.read_h160(callee_ptr)?; + let deposit_limit = + if deposit_ptr == SENTINEL { U256::zero() } else { memory.read_u256(deposit_ptr)? }; + let input_data = if flags.contains(CallFlags::CLONE_INPUT) { let input = self.input_data.as_ref().ok_or(Error::::InputForwarded)?; charge_gas!(self, RuntimeCosts::CallInputCloned(input.len() as u32))?; @@ -1009,13 +1017,7 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { }; let call_outcome = match call_type { - CallType::Call { callee_ptr, value_ptr, deposit_ptr, weight } => { - let callee = memory.read_h160(callee_ptr)?; - let deposit_limit = if deposit_ptr == SENTINEL { - U256::zero() - } else { - memory.read_u256(deposit_ptr)? - }; + CallType::Call { value_ptr } => { let read_only = flags.contains(CallFlags::READ_ONLY); let value = memory.read_u256(value_ptr)?; if value > 0u32.into() { @@ -1036,19 +1038,11 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { read_only, ) }, - CallType::DelegateCall { address_ptr, deposit_ptr, weight } => { + CallType::DelegateCall => { if flags.intersects(CallFlags::ALLOW_REENTRY | CallFlags::READ_ONLY) { return Err(Error::::InvalidCallFlags.into()); } - - let mut address = H160::zero(); - memory.read_into_buf(address_ptr, address.as_bytes_mut())?; - let deposit_limit = if deposit_ptr == SENTINEL { - U256::zero() - } else { - memory.read_u256(deposit_ptr)? - }; - self.ext.delegate_call(weight, deposit_limit, address, input_data) + self.ext.delegate_call(weight, deposit_limit, callee, input_data) }, }; @@ -1284,12 +1278,10 @@ pub mod env { self.call( memory, CallFlags::from_bits(flags).ok_or(Error::::InvalidCallFlags)?, - CallType::Call { - callee_ptr, - value_ptr, - deposit_ptr, - weight: Weight::from_parts(ref_time_limit, proof_size_limit), - }, + CallType::Call { value_ptr }, + callee_ptr, + deposit_ptr, + Weight::from_parts(ref_time_limit, proof_size_limit), input_data_ptr, input_data_len, output_ptr, @@ -1316,11 +1308,10 @@ pub mod env { self.call( memory, CallFlags::from_bits(flags).ok_or(Error::::InvalidCallFlags)?, - CallType::DelegateCall { - address_ptr, - deposit_ptr, - weight: Weight::from_parts(ref_time_limit, proof_size_limit), - }, + CallType::DelegateCall, + address_ptr, + deposit_ptr, + Weight::from_parts(ref_time_limit, proof_size_limit), input_data_ptr, input_data_len, output_ptr, diff --git a/substrate/frame/revive/uapi/src/host.rs b/substrate/frame/revive/uapi/src/host.rs index 65f10abe248c..a9441dc300eb 100644 --- a/substrate/frame/revive/uapi/src/host.rs +++ b/substrate/frame/revive/uapi/src/host.rs @@ -327,7 +327,7 @@ pub trait HostFn: private::Sealed { /// `T::AccountId`. Traps otherwise. /// - `ref_time_limit`: how much *ref_time* Weight to devote to the execution. /// - `proof_size_limit`: how much *proof_size* Weight to devote to the execution. - /// - `deposit`: The storage deposit limit for delegate call. Passing `None` means setting no + /// - `deposit_limit`: The storage deposit limit for delegate call. Passing `None` means setting no /// specific limit for the call, which implies storage usage up to the limit of the parent /// call. /// - `input`: The input data buffer used to call the contract. @@ -347,7 +347,7 @@ pub trait HostFn: private::Sealed { address: &[u8; 20], ref_time_limit: u64, proof_size_limit: u64, - deposit: Option<&[u8; 32]>, + deposit_limit: Option<&[u8; 32]>, input_data: &[u8], output: Option<&mut &mut [u8]>, ) -> Result; From 9f06a0356ac3b6ab0f55c02c823d29cf8b9818bd Mon Sep 17 00:00:00 2001 From: Ermal Kaleci Date: Wed, 6 Nov 2024 17:05:26 +0100 Subject: [PATCH 16/18] add deposit_limit test --- .../contracts/delegate_call_deposit_limit.rs | 46 +++++++++++++++++++ substrate/frame/revive/src/tests.rs | 40 +++++++++++++++- 2 files changed, 85 insertions(+), 1 deletion(-) create mode 100644 substrate/frame/revive/fixtures/contracts/delegate_call_deposit_limit.rs diff --git a/substrate/frame/revive/fixtures/contracts/delegate_call_deposit_limit.rs b/substrate/frame/revive/fixtures/contracts/delegate_call_deposit_limit.rs new file mode 100644 index 000000000000..55203d534c9b --- /dev/null +++ b/substrate/frame/revive/fixtures/contracts/delegate_call_deposit_limit.rs @@ -0,0 +1,46 @@ +// This file is part of Substrate. + +// Copyright (C) Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#![no_std] +#![no_main] + +use common::{input, u256_bytes}; +use uapi::{HostFn, HostFnImpl as api, StorageFlags}; + +#[no_mangle] +#[polkavm_derive::polkavm_export] +pub extern "C" fn deploy() {} + +#[no_mangle] +#[polkavm_derive::polkavm_export] +pub extern "C" fn call() { + input!( + address: &[u8; 20], + deposit_limit: u64, + ); + + let input = [0u8; 0]; + api::delegate_call(uapi::CallFlags::empty(), address, 0, 0, Some(&u256_bytes(deposit_limit)), &input, None).unwrap(); + + let mut key = [0u8; 32]; + key[0] = 1u8; + + let mut value = [0u8; 32]; + + api::get_storage(StorageFlags::empty(), &key, &mut &mut value[..]).unwrap(); + assert!(value[0] == 1u8); +} diff --git a/substrate/frame/revive/src/tests.rs b/substrate/frame/revive/src/tests.rs index 40538cf8ca71..56aedf9d8230 100644 --- a/substrate/frame/revive/src/tests.rs +++ b/substrate/frame/revive/src/tests.rs @@ -1149,7 +1149,7 @@ fn delegate_call() { } #[test] -fn delegate_call_with_limits() { +fn delegate_call_with_weight_limit() { let (caller_wasm, _caller_code_hash) = compile_module("delegate_call").unwrap(); let (callee_wasm, _callee_code_hash) = compile_module("delegate_call_lib").unwrap(); @@ -1185,6 +1185,44 @@ fn delegate_call_with_limits() { }); } +#[test] +fn delegate_call_with_deposit_limit() { + let (caller_pvm, _caller_code_hash) = compile_module("delegate_call_deposit_limit").unwrap(); + let (callee_pvm, _callee_code_hash) = compile_module("delegate_call_lib").unwrap(); + + ExtBuilder::default().existential_deposit(500).build().execute_with(|| { + let _ = ::Currency::set_balance(&ALICE, 1_000_000); + + // Instantiate the 'caller' + let Contract { addr: caller_addr, .. } = + builder::bare_instantiate(Code::Upload(caller_pvm)) + .value(300_000) + .build_and_unwrap_contract(); + + // Instantiate the 'callee' + let Contract { addr: callee_addr, .. } = + builder::bare_instantiate(Code::Upload(callee_pvm)) + .value(100_000) + .build_and_unwrap_contract(); + + // Delegate call will write 1 storage and deposit of 2 (1 item) + 32 (bytes) is required. + // Fails, not enough deposit + assert_err!( + builder::bare_call(caller_addr) + .value(1337) + .data((callee_addr, 33u64).encode()) + .build() + .result, + Error::::StorageDepositLimitExhausted, + ); + + assert_ok!(builder::call(caller_addr) + .value(1337) + .data((callee_addr, 34u64).encode()) + .build()); + }); +} + #[test] fn transfer_expendable_cannot_kill_account() { let (wasm, _code_hash) = compile_module("dummy").unwrap(); From 95d1ffd49136dac07bafa2460d746eec2f5b898a Mon Sep 17 00:00:00 2001 From: Ermal Kaleci Date: Wed, 6 Nov 2024 17:34:12 +0100 Subject: [PATCH 17/18] fmt --- substrate/frame/revive/uapi/src/host.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/frame/revive/uapi/src/host.rs b/substrate/frame/revive/uapi/src/host.rs index a9441dc300eb..377e14766127 100644 --- a/substrate/frame/revive/uapi/src/host.rs +++ b/substrate/frame/revive/uapi/src/host.rs @@ -327,8 +327,8 @@ pub trait HostFn: private::Sealed { /// `T::AccountId`. Traps otherwise. /// - `ref_time_limit`: how much *ref_time* Weight to devote to the execution. /// - `proof_size_limit`: how much *proof_size* Weight to devote to the execution. - /// - `deposit_limit`: The storage deposit limit for delegate call. Passing `None` means setting no - /// specific limit for the call, which implies storage usage up to the limit of the parent + /// - `deposit_limit`: The storage deposit limit for delegate call. Passing `None` means setting + /// no specific limit for the call, which implies storage usage up to the limit of the parent /// call. /// - `input`: The input data buffer used to call the contract. /// - `output`: A reference to the output data buffer to write the call output buffer. If `None` From 5c1f1ed74ef02350e81d43923304ed1c4d8e4695 Mon Sep 17 00:00:00 2001 From: Ermal Kaleci Date: Tue, 19 Nov 2024 08:40:16 +0100 Subject: [PATCH 18/18] fix test --- substrate/frame/revive/src/exec.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index 523267a2410c..49c08166483e 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -2267,7 +2267,7 @@ mod tests { BOB_ADDR, &mut GasMeter::::new(GAS_LIMIT), &mut storage_meter, - 0, + U256::zero(), vec![], None, ), @@ -2284,7 +2284,7 @@ mod tests { BOB_ADDR, &mut GasMeter::::new(GAS_LIMIT), &mut storage_meter, - 0, + U256::zero(), vec![], None, ));