From 6488586bc82d961483a4c5705b643a0a3890943d Mon Sep 17 00:00:00 2001 From: Cyrill Leutwiler Date: Wed, 18 Sep 2024 21:11:06 +0200 Subject: [PATCH 01/25] store return data in frame Signed-off-by: Cyrill Leutwiler --- substrate/frame/revive/src/exec.rs | 22 +++++++ substrate/frame/revive/src/wasm/runtime.rs | 73 +++++++++++----------- 2 files changed, 58 insertions(+), 37 deletions(-) diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index 233658696c8f..5c2f07b43ef9 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -427,6 +427,9 @@ pub trait Ext: sealing::Sealed { /// Check if running in read-only context. fn is_read_only(&self) -> bool; + + /// Returns a mutable reference to saved output of the current frame. + fn output_mut(&mut self) -> &mut Vec; } /// Describes the different functions that can be exported by an [`Executable`]. @@ -547,6 +550,8 @@ struct Frame { read_only: bool, /// The caller of the currently executing frame which was spawned by `delegate_call`. delegate_caller: Option>, + /// The output of the last call + output: Vec, } /// Used in a delegate call frame arguments in order to override the executable and caller. @@ -911,6 +916,7 @@ where nested_storage: storage_meter.nested(deposit_limit), allows_reentry: true, read_only, + output: Vec::new(), }; Ok(Some((frame, executable))) @@ -971,6 +977,9 @@ where let delegated_code_hash = if frame.delegate_caller.is_some() { Some(*executable.code_hash()) } else { None }; + if let Some(previous) = self.caller_output_mut() { + *previous = Vec::new(); + } self.transient_storage.start_transaction(); let do_transaction = || { @@ -1265,6 +1274,15 @@ where fn account_balance(&self, who: &T::AccountId) -> U256 { T::Currency::reducible_balance(who, Preservation::Preserve, Fortitude::Polite).into() } + + /// Returns a mutable reference to saved output of the caller frame. + fn caller_output_mut(&mut self) -> Option<&mut Vec> { + match self.frames.len() { + 0 => None, + 1 => Some(&mut self.first_frame.output), + _ => self.frames.get_mut(self.frames.len() - 2).map(|frame| &mut frame.output), + } + } } impl<'a, T, E> Ext for Stack<'a, T, E> @@ -1690,6 +1708,10 @@ where fn is_read_only(&self) -> bool { self.top_frame().read_only } + + fn output_mut(&mut self) -> &mut Vec { + &mut self.top_frame_mut().output + } } mod sealing { diff --git a/substrate/frame/revive/src/wasm/runtime.rs b/substrate/frame/revive/src/wasm/runtime.rs index 34cce533c14a..dfad76dd9cda 100644 --- a/substrate/frame/revive/src/wasm/runtime.rs +++ b/substrate/frame/revive/src/wasm/runtime.rs @@ -237,8 +237,8 @@ parameter_types! { const XcmExecutionFailed: ReturnErrorCode = ReturnErrorCode::XcmExecutionFailed; } -impl From for ReturnErrorCode { - fn from(from: ExecReturnValue) -> Self { +impl<'a> From<&'a ExecReturnValue> for ReturnErrorCode { + fn from(from: &'a ExecReturnValue) -> Self { if from.flags.contains(ReturnFlags::REVERT) { Self::CalleeReverted } else { @@ -763,20 +763,16 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { } } - /// Fallible conversion of a `ExecResult` to `ReturnErrorCode`. - fn exec_into_return_code(from: ExecResult) -> Result { + /// Fallible conversion of a `ExecError` to `ReturnErrorCode`. + fn exec_error_into_return_code(from: ExecError) -> Result { use crate::exec::ErrorOrigin::Callee; - let ExecError { error, origin } = match from { - Ok(retval) => return Ok(retval.into()), - Err(err) => err, - }; - - match (error, origin) { + match (from.error, from.origin) { (_, Callee) => Ok(ReturnErrorCode::CalleeTrapped), (err, _) => Self::err_into_return_code(err), } } + fn decode_key(&self, memory: &M, key_ptr: u32, key_len: u32) -> Result { let res = match key_len { SENTINEL => { @@ -1041,17 +1037,18 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { } } - if let Ok(output) = &call_outcome { - self.write_sandbox_output( - memory, - output_ptr, - output_len_ptr, - &output.data, - true, - |len| Some(RuntimeCosts::CopyToContract(len)), - )?; - } - Ok(Self::exec_into_return_code(call_outcome)?) + let output = match call_outcome { + Ok(outcome) => outcome, + Err(err) => return Ok(Self::exec_error_into_return_code(err)?), + }; + let return_error_code = (&output).into(); + + self.write_sandbox_output(memory, output_ptr, output_len_ptr, &output.data, true, |len| { + Some(RuntimeCosts::CopyToContract(len)) + })?; + *self.ext.output_mut() = output.data; + + Ok(return_error_code) } fn instantiate( @@ -1088,26 +1085,28 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { input_data, salt.as_ref(), ); - if let Ok((address, output)) = &instantiate_outcome { - if !output.flags.contains(ReturnFlags::REVERT) { - self.write_fixed_sandbox_output( - memory, - address_ptr, - &address.as_bytes(), - true, - already_charged, - )?; - } - self.write_sandbox_output( + + let (address, retval) = match instantiate_outcome { + Ok(outcome) => outcome, + Err(err) => return Ok(Self::exec_error_into_return_code(err)?), + }; + let return_error_code = (&retval).into(); + + if !retval.flags.contains(ReturnFlags::REVERT) { + self.write_fixed_sandbox_output( memory, - output_ptr, - output_len_ptr, - &output.data, + address_ptr, + &address.as_bytes(), true, - |len| Some(RuntimeCosts::CopyToContract(len)), + already_charged, )?; } - Ok(Self::exec_into_return_code(instantiate_outcome.map(|(_, retval)| retval))?) + self.write_sandbox_output(memory, output_ptr, output_len_ptr, &retval.data, true, |len| { + Some(RuntimeCosts::CopyToContract(len)) + })?; + *self.ext.output_mut() = retval.data; + + Ok(return_error_code) } fn terminate(&mut self, memory: &M, beneficiary_ptr: u32) -> Result<(), TrapReason> { From 1a7569c26217dd1c68746d3905bb09364f39d21f Mon Sep 17 00:00:00 2001 From: xermicus Date: Thu, 19 Sep 2024 10:18:37 +0200 Subject: [PATCH 02/25] impl runtime apis Signed-off-by: xermicus --- substrate/frame/revive/src/exec.rs | 13 ++++-- substrate/frame/revive/src/wasm/runtime.rs | 47 ++++++++++++++++++++-- 2 files changed, 53 insertions(+), 7 deletions(-) diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index 5c2f07b43ef9..2bd79f033ac0 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -428,8 +428,11 @@ pub trait Ext: sealing::Sealed { /// Check if running in read-only context. fn is_read_only(&self) -> bool; - /// Returns a mutable reference to saved output of the current frame. - fn output_mut(&mut self) -> &mut Vec; + /// Returns an immutable reference to saved output of the last executed call frame. + fn last_frame_output(&self) -> &[u8]; + + /// Returns a mutable reference to saved output of the last executed call frame. + fn last_frame_output_mut(&mut self) -> &mut Vec; } /// Describes the different functions that can be exported by an [`Executable`]. @@ -1709,7 +1712,11 @@ where self.top_frame().read_only } - fn output_mut(&mut self) -> &mut Vec { + fn last_frame_output(&self) -> &[u8] { + &self.top_frame().output + } + + fn last_frame_output_mut(&mut self) -> &mut Vec { &mut self.top_frame_mut().output } } diff --git a/substrate/frame/revive/src/wasm/runtime.rs b/substrate/frame/revive/src/wasm/runtime.rs index dfad76dd9cda..a81ae8479f15 100644 --- a/substrate/frame/revive/src/wasm/runtime.rs +++ b/substrate/frame/revive/src/wasm/runtime.rs @@ -1046,7 +1046,7 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { self.write_sandbox_output(memory, output_ptr, output_len_ptr, &output.data, true, |len| { Some(RuntimeCosts::CopyToContract(len)) })?; - *self.ext.output_mut() = output.data; + *self.ext.last_frame_output_mut() = output.data; Ok(return_error_code) } @@ -1104,7 +1104,7 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { self.write_sandbox_output(memory, output_ptr, output_len_ptr, &retval.data, true, |len| { Some(RuntimeCosts::CopyToContract(len)) })?; - *self.ext.output_mut() = retval.data; + *self.ext.last_frame_output_mut() = retval.data; Ok(return_error_code) } @@ -1592,11 +1592,11 @@ pub mod env { self.charge_gas(RuntimeCosts::DepositEvent { num_topic, len: data_len })?; if num_topic > limits::NUM_EVENT_TOPICS { - return Err(Error::::TooManyTopics.into()); + return Err(Error::::TooManyTopics.into()) } if data_len > self.ext.max_value_size() { - return Err(Error::::ValueTooLarge.into()); + return Err(Error::::ValueTooLarge.into()) } let topics: Vec = match num_topic { @@ -1973,4 +1973,43 @@ pub mod env { self.ext.unlock_delegate_dependency(&code_hash)?; Ok(()) } + + /// Stores the length of the data returned by the last call into the supplied buffer. + /// See [`pallet_revive_uapi::HostFn::return_data_size`]. + #[api_version(0)] + fn return_data_size(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { + Ok(self.write_fixed_sandbox_output( + memory, + out_ptr, + &as_bytes(U256::from(self.ext.last_frame_output().len())), + false, + |len| Some(RuntimeCosts::CopyToContract(len)), + )?) + } + + /// Stores data returned by the last call starting from `offset` into the supplied buffer. + /// See [`pallet_revive_uapi::HostFn::return_data`]. + #[api_version(0)] + fn return_data( + &mut self, + memory: &mut M, + out_ptr: u32, + out_len_ptr: u32, + offset: u32, + ) -> Result<(), TrapReason> { + if offset as usize > self.ext.last_frame_output().len() { + return Err(Error::::OutOfBounds.into()) + } + let buf = core::mem::take(self.ext.last_frame_output_mut()); + let result = self.write_sandbox_output( + memory, + out_ptr, + out_len_ptr, + &buf[offset as usize..], + false, + |len| Some(RuntimeCosts::CopyToContract(len)), + ); + *self.ext.last_frame_output_mut() = buf; + Ok(result?) + } } From d53dcae036e5d933adb17aaad2691a93fe95e1da Mon Sep 17 00:00:00 2001 From: xermicus Date: Thu, 19 Sep 2024 10:32:21 +0200 Subject: [PATCH 03/25] better name for output field Signed-off-by: xermicus --- substrate/frame/revive/src/exec.rs | 14 +++++++------- substrate/frame/revive/src/wasm/runtime.rs | 4 ++-- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index 2bd79f033ac0..0f83f44f3567 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -553,8 +553,8 @@ struct Frame { read_only: bool, /// The caller of the currently executing frame which was spawned by `delegate_call`. delegate_caller: Option>, - /// The output of the last call - output: Vec, + /// The output of the last call frame + last_frame_output: Vec, } /// Used in a delegate call frame arguments in order to override the executable and caller. @@ -919,7 +919,7 @@ where nested_storage: storage_meter.nested(deposit_limit), allows_reentry: true, read_only, - output: Vec::new(), + last_frame_output: Vec::new(), }; Ok(Some((frame, executable))) @@ -1282,8 +1282,8 @@ where fn caller_output_mut(&mut self) -> Option<&mut Vec> { match self.frames.len() { 0 => None, - 1 => Some(&mut self.first_frame.output), - _ => self.frames.get_mut(self.frames.len() - 2).map(|frame| &mut frame.output), + 1 => Some(&mut self.first_frame.last_frame_output), + _ => self.frames.get_mut(self.frames.len() - 2).map(|frame| &mut frame.last_frame_output), } } } @@ -1713,11 +1713,11 @@ where } fn last_frame_output(&self) -> &[u8] { - &self.top_frame().output + &self.top_frame().last_frame_output } fn last_frame_output_mut(&mut self) -> &mut Vec { - &mut self.top_frame_mut().output + &mut self.top_frame_mut().last_frame_output } } diff --git a/substrate/frame/revive/src/wasm/runtime.rs b/substrate/frame/revive/src/wasm/runtime.rs index a81ae8479f15..84c93de7cb6c 100644 --- a/substrate/frame/revive/src/wasm/runtime.rs +++ b/substrate/frame/revive/src/wasm/runtime.rs @@ -1592,11 +1592,11 @@ pub mod env { self.charge_gas(RuntimeCosts::DepositEvent { num_topic, len: data_len })?; if num_topic > limits::NUM_EVENT_TOPICS { - return Err(Error::::TooManyTopics.into()) + return Err(Error::::TooManyTopics.into()); } if data_len > self.ext.max_value_size() { - return Err(Error::::ValueTooLarge.into()) + return Err(Error::::ValueTooLarge.into()); } let topics: Vec = match num_topic { From 5073e454ff32818c6c9c6dc71d1e748c08c7a1e5 Mon Sep 17 00:00:00 2001 From: xermicus Date: Thu, 19 Sep 2024 12:16:22 +0200 Subject: [PATCH 04/25] uapi Signed-off-by: xermicus --- substrate/frame/revive/src/wasm/runtime.rs | 2 +- substrate/frame/revive/uapi/src/host.rs | 14 ++++++++++++++ substrate/frame/revive/uapi/src/host/riscv32.rs | 14 ++++++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/substrate/frame/revive/src/wasm/runtime.rs b/substrate/frame/revive/src/wasm/runtime.rs index 84c93de7cb6c..06ce52e426b2 100644 --- a/substrate/frame/revive/src/wasm/runtime.rs +++ b/substrate/frame/revive/src/wasm/runtime.rs @@ -1990,7 +1990,7 @@ pub mod env { /// Stores data returned by the last call starting from `offset` into the supplied buffer. /// See [`pallet_revive_uapi::HostFn::return_data`]. #[api_version(0)] - fn return_data( + fn return_data_copy( &mut self, memory: &mut M, out_ptr: u32, diff --git a/substrate/frame/revive/uapi/src/host.rs b/substrate/frame/revive/uapi/src/host.rs index 538de7ea251d..cd987f5179fb 100644 --- a/substrate/frame/revive/uapi/src/host.rs +++ b/substrate/frame/revive/uapi/src/host.rs @@ -617,6 +617,20 @@ pub trait HostFn: private::Sealed { /// Returns `ReturnCode::Success` when the message was successfully sent. When the XCM /// execution fails, `ReturnErrorCode::XcmSendFailed` is returned. fn xcm_send(dest: &[u8], msg: &[u8], output: &mut [u8; 32]) -> Result; + + /// Stores the size of the returned data of the last contract call or instantiation. + /// + /// # Parameters + /// + /// - `output`: A reference to the output buffer to write the size. + fn return_data_size(output: &mut [u8; 32]); + + /// Stores the returned data of the last contract call or contract instantiation. + /// + /// # Parameters + /// - `output`: A reference to the output buffer to write the data. + /// - `offset`: Byte offset into the returned data + fn return_data_copy(output: &mut &mut [u8], offset: u32); } mod private { diff --git a/substrate/frame/revive/uapi/src/host/riscv32.rs b/substrate/frame/revive/uapi/src/host/riscv32.rs index 0bb0ede4543b..eeb0cdce3fdb 100644 --- a/substrate/frame/revive/uapi/src/host/riscv32.rs +++ b/substrate/frame/revive/uapi/src/host/riscv32.rs @@ -130,6 +130,8 @@ mod sys { msg_len: u32, out_ptr: *mut u8, ) -> ReturnCode; + pub fn return_data_size(out_ptr: *mut u8); + pub fn return_data_copy(out_ptr: *mut u8, out_len_ptr: *mut u32, offset: u32); } } @@ -547,4 +549,16 @@ impl HostFn for HostFnImpl { }; ret_code.into() } + + fn return_data_size(output: &mut [u8; 32]) { + unsafe { sys::return_data_size(output.as_mut_ptr()) }; + } + + fn return_data_copy(output: &mut &mut [u8], offset: u32) { + let mut output_len = output.len() as u32; + { + unsafe { sys::return_data_copy(output.as_mut_ptr(), &mut output_len, offset) }; + } + extract_from_slice(output, output_len as usize); + } } From 81f77ef5fe42180c35949ecd42f22c945341d52c Mon Sep 17 00:00:00 2001 From: Cyrill Leutwiler Date: Thu, 19 Sep 2024 16:16:49 +0200 Subject: [PATCH 05/25] wip tests Signed-off-by: Cyrill Leutwiler --- .../fixtures/contracts/return_data_api.rs | 137 ++++++++++++++++++ substrate/frame/revive/src/tests.rs | 28 ++++ 2 files changed, 165 insertions(+) create mode 100644 substrate/frame/revive/fixtures/contracts/return_data_api.rs diff --git a/substrate/frame/revive/fixtures/contracts/return_data_api.rs b/substrate/frame/revive/fixtures/contracts/return_data_api.rs new file mode 100644 index 000000000000..98f1aa5cb984 --- /dev/null +++ b/substrate/frame/revive/fixtures/contracts/return_data_api.rs @@ -0,0 +1,137 @@ +// 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. + +//! This tests that the correct output data is written when the provided +//! output buffer length is smaller than what was actually returned during +//! calls and instantiations. +//! +//! To not need an additional callee fixture, we call ourself recursively +//! and also instantiate our own code hash (constructor and recursive calls +//! always return `BUF_SIZE` bytes of data). + +#![no_std] +#![no_main] + +use common::{input, u256_bytes}; +use uapi::{HostFn, HostFnImpl as api}; + +const BUF_SIZE: usize = 128; +static DATA: [u8; BUF_SIZE] = [0xff; BUF_SIZE]; + +fn assert_call(callee: &[u8; 20]) { + let value = &[0u8; 32]; + let mut input = DATA; + + input[0] = 0; + api::call(uapi::CallFlags::empty(), callee, 0u64, 0u64, None, value, &input, None).unwrap(); + + assert_return_data_size_of(BUF_SIZE as u64); +} + +/// Instantiate `code_hash` and expect the return_data_{size,copy} APIs to work correctly. +fn assert_instantiate(code_hash: &[u8; 32]) -> [u8; 20] { + let value = &[0; 32]; + let mut input = DATA; + let mut address_buf = [0; 20]; + + /// The return data API should work for reverted executions too + for exit_flag in [0, 1] { + input[0] = exit_flag; + input[7] = exit_flag; + let _ = api::instantiate( + code_hash, + 0u64, + 0u64, + None, + &value, + &input, + Some(&mut address_buf), + None, + None, + ); + assert_return_data_size_of(BUF_SIZE as u64 - 4); + assert_plain_transfer_does_not_reset(BUF_SIZE as u64 - 4); + + reset_return_data(); + } + + address_buf +} + +fn recursion_guard() -> [u8; 20] { + let mut caller_address = [0u8; 20]; + api::caller(&mut caller_address); + + let mut own_address = [0u8; 20]; + api::address(&mut own_address); + + assert_ne!(caller_address, own_address); + + own_address +} + +/// Call ourselves recursively, which panics the callee and thus resets the return data. +fn reset_return_data() { + let mut output_buf = [0u8; BUF_SIZE]; + + let own_address = recursion_guard(); + + let mut output_buf = [0; BUF_SIZE]; + let return_buf = &mut &mut output_buf[..]; + api::call( + uapi::CallFlags::ALLOW_REENTRY, + &own_address, + 0u64, + 0u64, + None, + &[0u8; 32], + &[], + Some(return_buf), + ) + .unwrap_err(); + //assert_eq!(return_buf.len(), 0); +} + +fn assert_return_data_size_of(expected: u64) { + let mut return_data_size = [0xff; 32]; + api::return_data_size(&mut return_data_size); + assert_eq!(return_data_size, u256_bytes(expected)); +} + +/// A plain transfer should not reset the return data. +fn assert_plain_transfer_does_not_reset(expected: u64) { + api::transfer(&[0; 20], &u256_bytes(128)).unwrap(); + assert_return_data_size_of(expected); +} + +#[no_mangle] +#[polkavm_derive::polkavm_export] +pub extern "C" fn deploy() {} + +#[no_mangle] +#[polkavm_derive::polkavm_export] +pub extern "C" fn call() { + input!(code_hash: &[u8; 32],); + + recursion_guard(); + + // we didn't do any calls yet; return data size should be 0 + assert_return_data_size_of(0); + + let callee = assert_instantiate(code_hash); + //assert_call(&callee); +} diff --git a/substrate/frame/revive/src/tests.rs b/substrate/frame/revive/src/tests.rs index 167e8da201b8..ec8626722e62 100644 --- a/substrate/frame/revive/src/tests.rs +++ b/substrate/frame/revive/src/tests.rs @@ -4310,4 +4310,32 @@ mod run_tests { assert_ok!(builder::call(addr).build()); }); } + + #[test] + fn return_data_api_works() { + let (code_return_data_api, _) = compile_module("return_data_api").unwrap(); + let (code_return_with_data, hash_return_with_data) = compile_module("return_with_data").unwrap(); + + ExtBuilder::default().existential_deposit(100).build().execute_with(|| { + let _ = ::Currency::set_balance(&ALICE, 1_000_000); + + // Upload the io echoing fixture for later use + assert_ok!(Contracts::upload_code( + RuntimeOrigin::signed(ALICE), + code_return_with_data, + deposit_limit::(), + )); + + // Create fixture: Constructor does nothing + let Contract { addr, .. } = + builder::bare_instantiate(Code::Upload(code_return_data_api)) + .build_and_unwrap_contract(); + + // Call the contract: It will issue calls and deploys, asserting on + assert_ok!(builder::call(addr) + .value(10 * 1024) + .data(hash_return_with_data.encode()) + .build()); + }); + } } From c981a75ba15b6a9f32e914717620108972de617b Mon Sep 17 00:00:00 2001 From: xermicus Date: Thu, 19 Sep 2024 18:15:33 +0200 Subject: [PATCH 06/25] test fixture Signed-off-by: xermicus --- .../fixtures/contracts/return_data_api.rs | 149 +++++++++++------- 1 file changed, 89 insertions(+), 60 deletions(-) diff --git a/substrate/frame/revive/fixtures/contracts/return_data_api.rs b/substrate/frame/revive/fixtures/contracts/return_data_api.rs index 98f1aa5cb984..76e4ea4bfa63 100644 --- a/substrate/frame/revive/fixtures/contracts/return_data_api.rs +++ b/substrate/frame/revive/fixtures/contracts/return_data_api.rs @@ -15,13 +15,13 @@ // See the License for the specific language governing permissions and // limitations under the License. -//! This tests that the correct output data is written when the provided -//! output buffer length is smaller than what was actually returned during -//! calls and instantiations. -//! -//! To not need an additional callee fixture, we call ourself recursively -//! and also instantiate our own code hash (constructor and recursive calls -//! always return `BUF_SIZE` bytes of data). +//! This tests that the `return_data_size` and `return_data_copy` APIs work. +//! +//! It does so by calling and instantiating the "return_with_data" fixture, +//! which always echoes back the input[4..] regardless of the call outcome. +//! +//! We also check that the saved return data is properly reset after a trap +//! and unaffected by plain transfers. #![no_std] #![no_main] @@ -29,49 +29,38 @@ use common::{input, u256_bytes}; use uapi::{HostFn, HostFnImpl as api}; -const BUF_SIZE: usize = 128; -static DATA: [u8; BUF_SIZE] = [0xff; BUF_SIZE]; - -fn assert_call(callee: &[u8; 20]) { - let value = &[0u8; 32]; - let mut input = DATA; - - input[0] = 0; - api::call(uapi::CallFlags::empty(), callee, 0u64, 0u64, None, value, &input, None).unwrap(); - - assert_return_data_size_of(BUF_SIZE as u64); +const INPUT_BUF_SIZE: usize = 128; +static INPUT_DATA: [u8; INPUT_BUF_SIZE] = [0xFF; INPUT_BUF_SIZE]; +/// The "return_with_data" fixture echoes back 4 bytes less than the input +const OUTPUT_BUF_SIZE: usize = INPUT_BUF_SIZE - 4; +static OUTPUT_DATA: [u8; OUTPUT_BUF_SIZE] = [0xEE; OUTPUT_BUF_SIZE]; + +fn assert_return_data_after_call(input: &[u8]) { + assert_return_data_size_of(OUTPUT_BUF_SIZE as u64); + assert_plain_transfer_does_not_reset(OUTPUT_BUF_SIZE as u64); + assert_return_data_copy(&input[4..]); + reset_return_data(); } -/// Instantiate `code_hash` and expect the return_data_{size,copy} APIs to work correctly. -fn assert_instantiate(code_hash: &[u8; 32]) -> [u8; 20] { - let value = &[0; 32]; - let mut input = DATA; - let mut address_buf = [0; 20]; - - /// The return data API should work for reverted executions too - for exit_flag in [0, 1] { - input[0] = exit_flag; - input[7] = exit_flag; - let _ = api::instantiate( - code_hash, - 0u64, - 0u64, - None, - &value, - &input, - Some(&mut address_buf), - None, - None, - ); - assert_return_data_size_of(BUF_SIZE as u64 - 4); - assert_plain_transfer_does_not_reset(BUF_SIZE as u64 - 4); - - reset_return_data(); - } - - address_buf +/// Assert that what we get from [api::return_data_copy] matches `whole_return_data`, +/// either fully or partially with an offset and limited size. +fn assert_return_data_copy(whole_return_data: &[u8]) { + // The full return data should match + let mut buf = OUTPUT_DATA; + let mut full = &mut buf[..whole_return_data.len()]; + api::return_data_copy(&mut full, 0); + assert_eq!(whole_return_data, full); + + // Partial return data should match + let mut buf = OUTPUT_DATA; + let offset = 5; // we just pick some offset + let size = 32; // we just pick some size + let mut partial = &mut buf[offset..offset + size]; + api::return_data_copy(&mut partial, offset as u32); + assert_eq!(*partial, whole_return_data[offset..offset + size]); } +/// This function panics in a recursive contract call context. fn recursion_guard() -> [u8; 20] { let mut caller_address = [0u8; 20]; api::caller(&mut caller_address); @@ -86,33 +75,29 @@ fn recursion_guard() -> [u8; 20] { /// Call ourselves recursively, which panics the callee and thus resets the return data. fn reset_return_data() { - let mut output_buf = [0u8; BUF_SIZE]; - - let own_address = recursion_guard(); - - let mut output_buf = [0; BUF_SIZE]; - let return_buf = &mut &mut output_buf[..]; api::call( uapi::CallFlags::ALLOW_REENTRY, - &own_address, + &recursion_guard(), 0u64, 0u64, None, &[0u8; 32], - &[], - Some(return_buf), + &[0u8; 32], + None, ) .unwrap_err(); - //assert_eq!(return_buf.len(), 0); + assert_return_data_size_of(0); } +/// Assert [api::return_data_size] to match the `expected` value. fn assert_return_data_size_of(expected: u64) { let mut return_data_size = [0xff; 32]; api::return_data_size(&mut return_data_size); assert_eq!(return_data_size, u256_bytes(expected)); } -/// A plain transfer should not reset the return data. +/// Assert [api::return_data_size] to match the `expected` value after a plain transfer +/// (plain transfers don't issue a call and so should not reset the return data) fn assert_plain_transfer_does_not_reset(expected: u64) { api::transfer(&[0; 20], &u256_bytes(128)).unwrap(); assert_return_data_size_of(expected); @@ -129,9 +114,53 @@ pub extern "C" fn call() { recursion_guard(); - // we didn't do any calls yet; return data size should be 0 + // we didn't do anything yet; return data size should be 0 assert_return_data_size_of(0); - let callee = assert_instantiate(code_hash); - //assert_call(&callee); + let mut address_buf = [0; 20]; + let construct_input = |exit_flag| { + let mut input = INPUT_DATA; + input[0] = exit_flag; + input[9] = 7; + input[17 / 2] = 127; + input[89 / 2] = 127; + input + }; + let mut instantiate = |exit_flag| { + api::instantiate( + code_hash, + 0u64, + 0u64, + None, + &[0; 32], + &construct_input(exit_flag), + Some(&mut address_buf), + None, + None, + ) + }; + let call = |exit_flag, address_buf| { + api::call( + uapi::CallFlags::empty(), + address_buf, + 0u64, + 0u64, + None, + &[0; 32], + &construct_input(exit_flag), + None, + ) + }; + + instantiate(0).unwrap(); + assert_return_data_after_call(&construct_input(0)[..]); + + instantiate(1).unwrap_err(); + assert_return_data_after_call(&construct_input(1)[..]); + + call(0, &address_buf).unwrap(); + assert_return_data_after_call(&construct_input(0)[..]); + + call(1, &address_buf).unwrap_err(); + assert_return_data_after_call(&construct_input(1)[..]); } From 6f43354f258a397348691830ff7f62e9db4076c4 Mon Sep 17 00:00:00 2001 From: Cyrill Leutwiler Date: Fri, 20 Sep 2024 10:04:10 +0200 Subject: [PATCH 07/25] store the output directly Signed-off-by: Cyrill Leutwiler --- substrate/frame/revive/src/exec.rs | 3 + substrate/frame/revive/src/wasm/runtime.rs | 71 +++++++++++----------- 2 files changed, 40 insertions(+), 34 deletions(-) diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index 0f83f44f3567..531aa40e1af2 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -1121,6 +1121,9 @@ where } self.pop_frame(success); + if let Ok(output) = output.as_ref() { + self.top_frame_mut().last_frame_output = output.data.clone(); + } output } diff --git a/substrate/frame/revive/src/wasm/runtime.rs b/substrate/frame/revive/src/wasm/runtime.rs index 06ce52e426b2..bd18fe6da377 100644 --- a/substrate/frame/revive/src/wasm/runtime.rs +++ b/substrate/frame/revive/src/wasm/runtime.rs @@ -237,8 +237,8 @@ parameter_types! { const XcmExecutionFailed: ReturnErrorCode = ReturnErrorCode::XcmExecutionFailed; } -impl<'a> From<&'a ExecReturnValue> for ReturnErrorCode { - fn from(from: &'a ExecReturnValue) -> Self { +impl From for ReturnErrorCode { + fn from(from: ExecReturnValue) -> Self { if from.flags.contains(ReturnFlags::REVERT) { Self::CalleeReverted } else { @@ -763,11 +763,16 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { } } - /// Fallible conversion of a `ExecError` to `ReturnErrorCode`. - fn exec_error_into_return_code(from: ExecError) -> Result { + /// Fallible conversion of a `ExecResult` to `ReturnErrorCode`. + fn exec_into_return_code(from: ExecResult) -> Result { use crate::exec::ErrorOrigin::Callee; - match (from.error, from.origin) { + let ExecError { error, origin } = match from { + Ok(retval) => return Ok(retval.into()), + Err(err) => err, + }; + + match (error, origin) { (_, Callee) => Ok(ReturnErrorCode::CalleeTrapped), (err, _) => Self::err_into_return_code(err), } @@ -1037,18 +1042,17 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { } } - let output = match call_outcome { - Ok(outcome) => outcome, - Err(err) => return Ok(Self::exec_error_into_return_code(err)?), - }; - let return_error_code = (&output).into(); - - self.write_sandbox_output(memory, output_ptr, output_len_ptr, &output.data, true, |len| { - Some(RuntimeCosts::CopyToContract(len)) - })?; - *self.ext.last_frame_output_mut() = output.data; - - Ok(return_error_code) + if let Ok(output) = &call_outcome { + self.write_sandbox_output( + memory, + output_ptr, + output_len_ptr, + &output.data, + true, + |len| Some(RuntimeCosts::CopyToContract(len)), + )?; + } + Ok(Self::exec_into_return_code(call_outcome)?) } fn instantiate( @@ -1086,27 +1090,26 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { salt.as_ref(), ); - let (address, retval) = match instantiate_outcome { - Ok(outcome) => outcome, - Err(err) => return Ok(Self::exec_error_into_return_code(err)?), - }; - let return_error_code = (&retval).into(); - - if !retval.flags.contains(ReturnFlags::REVERT) { - self.write_fixed_sandbox_output( + if let Ok((address, output)) = &instantiate_outcome { + if !output.flags.contains(ReturnFlags::REVERT) { + self.write_fixed_sandbox_output( + memory, + address_ptr, + &address.as_bytes(), + true, + already_charged, + )?; + } + self.write_sandbox_output( memory, - address_ptr, - &address.as_bytes(), + output_ptr, + output_len_ptr, + &output.data, true, - already_charged, + |len| Some(RuntimeCosts::CopyToContract(len)), )?; } - self.write_sandbox_output(memory, output_ptr, output_len_ptr, &retval.data, true, |len| { - Some(RuntimeCosts::CopyToContract(len)) - })?; - *self.ext.last_frame_output_mut() = retval.data; - - Ok(return_error_code) + Ok(Self::exec_into_return_code(instantiate_outcome.map(|(_, retval)| retval))?) } fn terminate(&mut self, memory: &M, beneficiary_ptr: u32) -> Result<(), TrapReason> { From 0eb213c656c5f23d71b5652556c2a3a51a2094de Mon Sep 17 00:00:00 2001 From: Cyrill Leutwiler Date: Fri, 20 Sep 2024 15:03:21 +0200 Subject: [PATCH 08/25] executable output is owned by the frame Signed-off-by: Cyrill Leutwiler --- .../fixtures/contracts/return_data_api.rs | 4 +- substrate/frame/revive/src/exec.rs | 212 +++++++++--------- substrate/frame/revive/src/wasm/runtime.rs | 116 +++++----- 3 files changed, 166 insertions(+), 166 deletions(-) diff --git a/substrate/frame/revive/fixtures/contracts/return_data_api.rs b/substrate/frame/revive/fixtures/contracts/return_data_api.rs index 76e4ea4bfa63..71a5767fbbf2 100644 --- a/substrate/frame/revive/fixtures/contracts/return_data_api.rs +++ b/substrate/frame/revive/fixtures/contracts/return_data_api.rs @@ -16,10 +16,10 @@ // limitations under the License. //! This tests that the `return_data_size` and `return_data_copy` APIs work. -//! +//! //! It does so by calling and instantiating the "return_with_data" fixture, //! which always echoes back the input[4..] regardless of the call outcome. -//! +//! //! We also check that the saved return data is properly reset after a trap //! and unaffected by plain transfers. diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index 531aa40e1af2..6f529e26275d 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -189,16 +189,12 @@ pub trait Ext: sealing::Sealed { input_data: Vec, allows_reentry: bool, read_only: bool, - ) -> Result; + ) -> Result<(), ExecError>; /// 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; + fn delegate_call(&mut self, code: H256, input_data: Vec) -> Result<(), ExecError>; /// Instantiate a contract from the given code. /// @@ -213,7 +209,7 @@ pub trait Ext: sealing::Sealed { value: U256, input_data: Vec, salt: Option<&[u8; 32]>, - ) -> Result<(H160, ExecReturnValue), ExecError>; + ) -> Result; /// Transfer all funds to `beneficiary` and delete the contract. /// @@ -429,10 +425,10 @@ pub trait Ext: sealing::Sealed { fn is_read_only(&self) -> bool; /// Returns an immutable reference to saved output of the last executed call frame. - fn last_frame_output(&self) -> &[u8]; + fn last_frame_output(&self) -> &ExecReturnValue; /// Returns a mutable reference to saved output of the last executed call frame. - fn last_frame_output_mut(&mut self) -> &mut Vec; + fn last_frame_output_mut(&mut self) -> &mut ExecReturnValue; } /// Describes the different functions that can be exported by an [`Executable`]. @@ -554,7 +550,7 @@ struct Frame { /// The caller of the currently executing frame which was spawned by `delegate_call`. delegate_caller: Option>, /// The output of the last call frame - last_frame_output: Vec, + last_frame_output: ExecReturnValue, } /// Used in a delegate call frame arguments in order to override the executable and caller. @@ -739,9 +735,9 @@ where value, debug_message, )? { - stack.run(executable, input_data) + stack.run(executable, input_data).map(|_| stack.first_frame.last_frame_output) } else { - Self::transfer_no_contract(&origin, &dest, value) + Self::transfer_no_contract(&origin, &dest, value).map(|_| Default::default()) } } @@ -780,7 +776,9 @@ where )? .expect(FRAME_ALWAYS_EXISTS_ON_INSTANTIATE); let address = T::AddressMapper::to_address(&stack.top_frame().account_id); - stack.run(executable, input_data).map(|ret| (address, ret)) + stack + .run(executable, input_data) + .map(|_| (address, stack.first_frame.last_frame_output)) } #[cfg(all(feature = "runtime-benchmarks", feature = "riscv"))] @@ -919,7 +917,7 @@ where nested_storage: storage_meter.nested(deposit_limit), allows_reentry: true, read_only, - last_frame_output: Vec::new(), + last_frame_output: Default::default(), }; Ok(Some((frame, executable))) @@ -974,15 +972,13 @@ where /// Run the current (top) frame. /// /// This can be either a call or an instantiate. - fn run(&mut self, executable: E, input_data: Vec) -> ExecResult { + fn run(&mut self, executable: E, input_data: Vec) -> Result<(), ExecError> { 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 let Some(previous) = self.caller_output_mut() { - *previous = Vec::new(); - } + self.take_caller_output(); self.transient_storage.start_transaction(); let do_transaction = || { @@ -1121,10 +1117,9 @@ where } self.pop_frame(success); - if let Ok(output) = output.as_ref() { - self.top_frame_mut().last_frame_output = output.data.clone(); - } - output + output.map(|output| { + self.top_frame_mut().last_frame_output = output; + }) } /// Remove the current (top) frame from the stack. @@ -1237,10 +1232,8 @@ where from: &Origin, to: &T::AccountId, value: BalanceOf, - ) -> ExecResult { - Self::transfer_from_origin(from, to, value) - .map(|_| ExecReturnValue::default()) - .map_err(Into::into) + ) -> Result<(), ExecError> { + Self::transfer_from_origin(from, to, value).map_err(Into::into) } /// Reference to the current (top) frame. @@ -1281,13 +1274,14 @@ where T::Currency::reducible_balance(who, Preservation::Preserve, Fortitude::Polite).into() } - /// Returns a mutable reference to saved output of the caller frame. - fn caller_output_mut(&mut self) -> Option<&mut Vec> { - match self.frames.len() { - 0 => None, - 1 => Some(&mut self.first_frame.last_frame_output), - _ => self.frames.get_mut(self.frames.len() - 2).map(|frame| &mut frame.last_frame_output), - } + /// Replaces the last frame output with the default value, dropping it. + fn take_caller_output(&mut self) { + let len = self.frames.len(); + core::mem::take(match len { + 0 => return, + 1 => &mut self.first_frame.last_frame_output, + _ => &mut self.frames.get_mut(len - 2).expect("len >= 2; qed").last_frame_output, + }); } } @@ -1309,7 +1303,7 @@ where input_data: Vec, allows_reentry: bool, read_only: bool, - ) -> ExecResult { + ) -> Result<(), ExecError> { // Before pushing the new frame: Protect the caller contract against reentrancy attacks. // It is important to do this before calling `allows_reentry` so that a direct recursion // is caught by it. @@ -1360,11 +1354,7 @@ where result } - fn delegate_call( - &mut self, - code_hash: H256, - input_data: Vec, - ) -> 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())?; let top_frame = self.top_frame_mut(); let contract_info = top_frame.contract_info().clone(); @@ -1392,7 +1382,7 @@ where value: U256, input_data: Vec, salt: Option<&[u8; 32]>, - ) -> Result<(H160, ExecReturnValue), ExecError> { + ) -> Result { let executable = E::from_storage(code_hash, self.gas_meter_mut())?; let sender = &self.top_frame().account_id; let executable = self.push_frame( @@ -1409,7 +1399,7 @@ where )?; let address = T::AddressMapper::to_address(&self.top_frame().account_id); self.run(executable.expect(FRAME_ALWAYS_EXISTS_ON_INSTANTIATE), input_data) - .map(|ret| (address, ret)) + .map(|_| address) } fn terminate(&mut self, beneficiary: &H160) -> DispatchResult { @@ -1715,11 +1705,11 @@ where self.top_frame().read_only } - fn last_frame_output(&self) -> &[u8] { + fn last_frame_output(&self) -> &ExecReturnValue { &self.top_frame().last_frame_output } - fn last_frame_output_mut(&mut self) -> &mut Vec { + fn last_frame_output_mut(&mut self) -> &mut ExecReturnValue { &mut self.top_frame_mut().last_frame_output } } @@ -2385,15 +2375,17 @@ mod tests { // ALICE is the origin of the call stack assert!(ctx.ext.caller_is_origin()); // BOB calls CHARLIE - ctx.ext.call( - Weight::zero(), - U256::zero(), - &CHARLIE_ADDR, - U256::zero(), - vec![], - true, - false, - ) + ctx.ext + .call( + Weight::zero(), + U256::zero(), + &CHARLIE_ADDR, + U256::zero(), + vec![], + true, + false, + ) + .map(|_| ctx.ext.last_frame_output().clone()) }); ExtBuilder::default().build().execute_with(|| { @@ -2479,15 +2471,17 @@ mod tests { // root is the origin of the call stack. assert!(ctx.ext.caller_is_root()); // BOB calls CHARLIE. - ctx.ext.call( - Weight::zero(), - U256::zero(), - &CHARLIE_ADDR, - U256::zero(), - vec![], - true, - false, - ) + ctx.ext + .call( + Weight::zero(), + U256::zero(), + &CHARLIE_ADDR, + U256::zero(), + vec![], + true, + false, + ) + .map(|_| ctx.ext.last_frame_output().clone()) }); ExtBuilder::default().build().execute_with(|| { @@ -2698,6 +2692,7 @@ mod tests { vec![], Some(&[48; 32]), ) + .map(|address| (address, ctx.ext.last_frame_output().clone())) .unwrap(); *instantiated_contract_address.borrow_mut() = Some(address); @@ -2873,15 +2868,17 @@ mod tests { assert_eq!(info.storage_byte_deposit, 0); info.storage_byte_deposit = 42; assert_eq!( - ctx.ext.call( - Weight::zero(), - U256::zero(), - &CHARLIE_ADDR, - U256::zero(), - vec![], - true, - false - ), + ctx.ext + .call( + Weight::zero(), + U256::zero(), + &CHARLIE_ADDR, + U256::zero(), + vec![], + true, + false + ) + .map(|_| ctx.ext.last_frame_output().clone()), exec_trapped() ); assert_eq!(ctx.ext.contract_info().storage_byte_deposit, 42); @@ -3127,6 +3124,7 @@ mod tests { let dest = H160::from_slice(ctx.input_data.as_ref()); ctx.ext .call(Weight::zero(), U256::zero(), &dest, U256::zero(), vec![], false, false) + .map(|_| ctx.ext.last_frame_output().clone()) }); let code_charlie = MockLoader::insert(Call, |_, _| exec_success()); @@ -3169,15 +3167,17 @@ mod tests { fn call_deny_reentry() { let code_bob = MockLoader::insert(Call, |ctx, _| { if ctx.input_data[0] == 0 { - ctx.ext.call( - Weight::zero(), - U256::zero(), - &CHARLIE_ADDR, - U256::zero(), - vec![], - false, - false, - ) + ctx.ext + .call( + Weight::zero(), + U256::zero(), + &CHARLIE_ADDR, + U256::zero(), + vec![], + false, + false, + ) + .map(|_| ctx.ext.last_frame_output().clone()) } else { exec_success() } @@ -3185,15 +3185,9 @@ mod tests { // call BOB with input set to '1' let code_charlie = MockLoader::insert(Call, |ctx, _| { - ctx.ext.call( - Weight::zero(), - U256::zero(), - &BOB_ADDR, - U256::zero(), - vec![1], - true, - false, - ) + ctx.ext + .call(Weight::zero(), U256::zero(), &BOB_ADDR, U256::zero(), vec![1], true, false) + .map(|_| ctx.ext.last_frame_output().clone()) }); ExtBuilder::default().build().execute_with(|| { @@ -3392,7 +3386,7 @@ mod tests { let alice_nonce = System::account_nonce(&ALICE); assert_eq!(System::account_nonce(ctx.ext.account_id()), 0); assert_eq!(ctx.ext.caller().account_id().unwrap(), &ALICE); - let (addr, _) = ctx + let addr = ctx .ext .instantiate( Weight::zero(), @@ -3948,15 +3942,17 @@ mod tests { Ok(WriteOutcome::New) ); assert_eq!( - ctx.ext.call( - Weight::zero(), - U256::zero(), - &CHARLIE_ADDR, - U256::zero(), - vec![], - true, - false, - ), + ctx.ext + .call( + Weight::zero(), + U256::zero(), + &CHARLIE_ADDR, + U256::zero(), + vec![], + true, + false, + ) + .map(|_| ctx.ext.last_frame_output().clone()), exec_success() ); assert_eq!(ctx.ext.get_transient_storage(storage_key_1), Some(vec![3])); @@ -4052,15 +4048,17 @@ mod tests { Ok(WriteOutcome::New) ); assert_eq!( - ctx.ext.call( - Weight::zero(), - U256::zero(), - &CHARLIE_ADDR, - U256::zero(), - vec![], - true, - false - ), + ctx.ext + .call( + Weight::zero(), + U256::zero(), + &CHARLIE_ADDR, + U256::zero(), + vec![], + true, + false + ) + .map(|_| ctx.ext.last_frame_output().clone()), exec_trapped() ); assert_eq!(ctx.ext.get_transient_storage(storage_key), Some(vec![1, 2])); diff --git a/substrate/frame/revive/src/wasm/runtime.rs b/substrate/frame/revive/src/wasm/runtime.rs index bd18fe6da377..37f4d320fde7 100644 --- a/substrate/frame/revive/src/wasm/runtime.rs +++ b/substrate/frame/revive/src/wasm/runtime.rs @@ -237,8 +237,8 @@ parameter_types! { const XcmExecutionFailed: ReturnErrorCode = ReturnErrorCode::XcmExecutionFailed; } -impl From for ReturnErrorCode { - fn from(from: ExecReturnValue) -> Self { +impl<'a> From<&'a ExecReturnValue> for ReturnErrorCode { + fn from(from: &'a ExecReturnValue) -> Self { if from.flags.contains(ReturnFlags::REVERT) { Self::CalleeReverted } else { @@ -763,16 +763,11 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { } } - /// Fallible conversion of a `ExecResult` to `ReturnErrorCode`. - fn exec_into_return_code(from: ExecResult) -> Result { + /// Fallible conversion of a `ExecError` to `ReturnErrorCode`. + fn exec_error_into_return_code(from: ExecError) -> Result { use crate::exec::ErrorOrigin::Callee; - let ExecError { error, origin } = match from { - Ok(retval) => return Ok(retval.into()), - Err(err) => err, - }; - - match (error, origin) { + match (from.error, from.origin) { (_, Callee) => Ok(ReturnErrorCode::CalleeTrapped), (err, _) => Self::err_into_return_code(err), } @@ -1031,28 +1026,30 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { }, }; - // `TAIL_CALL` only matters on an `OK` result. Otherwise the call stack comes to - // a halt anyways without anymore code being executed. - if flags.contains(CallFlags::TAIL_CALL) { - if let Ok(return_value) = call_outcome { + let output = core::mem::take(self.ext.last_frame_output_mut()); + match call_outcome { + // `TAIL_CALL` only matters on an `OK` result. Otherwise the call stack comes to + // a halt anyways without anymore code being executed. + Ok(_) if flags.contains(CallFlags::TAIL_CALL) => { return Err(TrapReason::Return(ReturnData { - flags: return_value.flags.bits(), - data: return_value.data, - })) - } - } - - if let Ok(output) = &call_outcome { - self.write_sandbox_output( - memory, - output_ptr, - output_len_ptr, - &output.data, - true, - |len| Some(RuntimeCosts::CopyToContract(len)), - )?; + flags: output.flags.bits(), + data: output.data, + })); + }, + Ok(_) => { + self.write_sandbox_output( + memory, + output_ptr, + output_len_ptr, + &output.data, + true, + |len| Some(RuntimeCosts::CopyToContract(len)), + )?; + *self.ext.last_frame_output_mut() = output; + Ok(self.ext.last_frame_output().into()) + }, + Err(err) => Ok(Self::exec_error_into_return_code(err)?), } - Ok(Self::exec_into_return_code(call_outcome)?) } fn instantiate( @@ -1081,35 +1078,39 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { let salt: [u8; 32] = memory.read_array(salt_ptr)?; Some(salt) }; - let instantiate_outcome = self.ext.instantiate( + + match self.ext.instantiate( weight, deposit_limit, code_hash, value, input_data, salt.as_ref(), - ); - - if let Ok((address, output)) = &instantiate_outcome { - if !output.flags.contains(ReturnFlags::REVERT) { - self.write_fixed_sandbox_output( + ) { + Ok(address) => { + let output = core::mem::take(self.ext.last_frame_output_mut()); + if !output.flags.contains(ReturnFlags::REVERT) { + self.write_fixed_sandbox_output( + memory, + address_ptr, + &address.as_bytes(), + true, + already_charged, + )?; + } + self.write_sandbox_output( memory, - address_ptr, - &address.as_bytes(), + output_ptr, + output_len_ptr, + &output.data, true, - already_charged, + |len| Some(RuntimeCosts::CopyToContract(len)), )?; - } - self.write_sandbox_output( - memory, - output_ptr, - output_len_ptr, - &output.data, - true, - |len| Some(RuntimeCosts::CopyToContract(len)), - )?; + *self.ext.last_frame_output_mut() = output; + Ok(self.ext.last_frame_output().into()) + }, + Err(err) => Ok(Self::exec_error_into_return_code(err)?), } - Ok(Self::exec_into_return_code(instantiate_outcome.map(|(_, retval)| retval))?) } fn terminate(&mut self, memory: &M, beneficiary_ptr: u32) -> Result<(), TrapReason> { @@ -1721,8 +1722,9 @@ pub mod env { Environment::new(self, memory, id, input_ptr, input_len, output_ptr, output_len_ptr); let ret = match chain_extension.call(env)? { RetVal::Converging(val) => Ok(val), - RetVal::Diverging { flags, data } => - Err(TrapReason::Return(ReturnData { flags: flags.bits(), data })), + RetVal::Diverging { flags, data } => { + Err(TrapReason::Return(ReturnData { flags: flags.bits(), data })) + }, }; self.chain_extension = Some(chain_extension); ret @@ -1984,7 +1986,7 @@ pub mod env { Ok(self.write_fixed_sandbox_output( memory, out_ptr, - &as_bytes(U256::from(self.ext.last_frame_output().len())), + &as_bytes(U256::from(self.ext.last_frame_output().data.len())), false, |len| Some(RuntimeCosts::CopyToContract(len)), )?) @@ -2000,19 +2002,19 @@ pub mod env { out_len_ptr: u32, offset: u32, ) -> Result<(), TrapReason> { - if offset as usize > self.ext.last_frame_output().len() { - return Err(Error::::OutOfBounds.into()) + let output = core::mem::take(self.ext.last_frame_output_mut()); + if offset as usize > output.data.len() { + return Err(Error::::OutOfBounds.into()); } - let buf = core::mem::take(self.ext.last_frame_output_mut()); let result = self.write_sandbox_output( memory, out_ptr, out_len_ptr, - &buf[offset as usize..], + &output.data[offset as usize..], false, |len| Some(RuntimeCosts::CopyToContract(len)), ); - *self.ext.last_frame_output_mut() = buf; + *self.ext.last_frame_output_mut() = output; Ok(result?) } } From 68e989b9bd51147ab8b755649bb21609444c4e40 Mon Sep 17 00:00:00 2001 From: xermicus Date: Fri, 20 Sep 2024 15:27:52 +0200 Subject: [PATCH 09/25] some bikeshedding Signed-off-by: xermicus --- .../frame/revive/fixtures/contracts/return_data_api.rs | 4 ++-- substrate/frame/revive/src/exec.rs | 10 +++++----- substrate/frame/revive/src/wasm/runtime.rs | 10 +++++----- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/substrate/frame/revive/fixtures/contracts/return_data_api.rs b/substrate/frame/revive/fixtures/contracts/return_data_api.rs index 71a5767fbbf2..1afc161b11cd 100644 --- a/substrate/frame/revive/fixtures/contracts/return_data_api.rs +++ b/substrate/frame/revive/fixtures/contracts/return_data_api.rs @@ -112,11 +112,11 @@ pub extern "C" fn deploy() {} pub extern "C" fn call() { input!(code_hash: &[u8; 32],); - recursion_guard(); - // we didn't do anything yet; return data size should be 0 assert_return_data_size_of(0); + recursion_guard(); + let mut address_buf = [0; 20]; let construct_input = |exit_flag| { let mut input = INPUT_DATA; diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index 6f529e26275d..eac98bdddda0 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -424,10 +424,10 @@ pub trait Ext: sealing::Sealed { /// Check if running in read-only context. fn is_read_only(&self) -> bool; - /// Returns an immutable reference to saved output of the last executed call frame. + /// Returns an immutable reference to the output of the last executed call frame. fn last_frame_output(&self) -> &ExecReturnValue; - /// Returns a mutable reference to saved output of the last executed call frame. + /// Returns a mutable reference to the output of the last executed call frame. fn last_frame_output_mut(&mut self) -> &mut ExecReturnValue; } @@ -549,7 +549,7 @@ struct Frame { read_only: bool, /// The caller of the currently executing frame which was spawned by `delegate_call`. delegate_caller: Option>, - /// The output of the last call frame + /// The output of the last executed call frame. last_frame_output: ExecReturnValue, } @@ -1274,10 +1274,10 @@ where T::Currency::reducible_balance(who, Preservation::Preserve, Fortitude::Polite).into() } - /// Replaces the last frame output with the default value, dropping it. + /// Replaces the `last_frame_output` of the **caller** frame with the default value. fn take_caller_output(&mut self) { let len = self.frames.len(); - core::mem::take(match len { + mem::take(match len { 0 => return, 1 => &mut self.first_frame.last_frame_output, _ => &mut self.frames.get_mut(len - 2).expect("len >= 2; qed").last_frame_output, diff --git a/substrate/frame/revive/src/wasm/runtime.rs b/substrate/frame/revive/src/wasm/runtime.rs index 37f4d320fde7..f5ad1fc26cfa 100644 --- a/substrate/frame/revive/src/wasm/runtime.rs +++ b/substrate/frame/revive/src/wasm/runtime.rs @@ -28,7 +28,7 @@ use crate::{ }; use alloc::{boxed::Box, vec, vec::Vec}; use codec::{Decode, DecodeLimit, Encode, MaxEncodedLen}; -use core::{fmt, marker::PhantomData}; +use core::{mem, fmt, marker::PhantomData}; use frame_support::{ dispatch::DispatchInfo, ensure, pallet_prelude::DispatchResultWithPostInfo, parameter_types, traits::Get, weights::Weight, @@ -1026,7 +1026,7 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { }, }; - let output = core::mem::take(self.ext.last_frame_output_mut()); + let output = mem::take(self.ext.last_frame_output_mut()); match call_outcome { // `TAIL_CALL` only matters on an `OK` result. Otherwise the call stack comes to // a halt anyways without anymore code being executed. @@ -1088,7 +1088,7 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { salt.as_ref(), ) { Ok(address) => { - let output = core::mem::take(self.ext.last_frame_output_mut()); + let output = mem::take(self.ext.last_frame_output_mut()); if !output.flags.contains(ReturnFlags::REVERT) { self.write_fixed_sandbox_output( memory, @@ -1992,7 +1992,7 @@ pub mod env { )?) } - /// Stores data returned by the last call starting from `offset` into the supplied buffer. + /// Stores data returned by the last call, starting from `offset`, into the supplied buffer. /// See [`pallet_revive_uapi::HostFn::return_data`]. #[api_version(0)] fn return_data_copy( @@ -2002,7 +2002,7 @@ pub mod env { out_len_ptr: u32, offset: u32, ) -> Result<(), TrapReason> { - let output = core::mem::take(self.ext.last_frame_output_mut()); + let output = mem::take(self.ext.last_frame_output_mut()); if offset as usize > output.data.len() { return Err(Error::::OutOfBounds.into()); } From 039c6b3187ef00d81ca8a36069f2ed0395551398 Mon Sep 17 00:00:00 2001 From: xermicus Date: Fri, 20 Sep 2024 15:32:44 +0200 Subject: [PATCH 10/25] fx Signed-off-by: xermicus --- substrate/frame/revive/src/wasm/runtime.rs | 25 +++++++++++----------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/substrate/frame/revive/src/wasm/runtime.rs b/substrate/frame/revive/src/wasm/runtime.rs index f5ad1fc26cfa..66fe0c20f569 100644 --- a/substrate/frame/revive/src/wasm/runtime.rs +++ b/substrate/frame/revive/src/wasm/runtime.rs @@ -28,7 +28,7 @@ use crate::{ }; use alloc::{boxed::Box, vec, vec::Vec}; use codec::{Decode, DecodeLimit, Encode, MaxEncodedLen}; -use core::{mem, fmt, marker::PhantomData}; +use core::{fmt, marker::PhantomData, mem}; use frame_support::{ dispatch::DispatchInfo, ensure, pallet_prelude::DispatchResultWithPostInfo, parameter_types, traits::Get, weights::Weight, @@ -2003,17 +2003,18 @@ pub mod env { offset: u32, ) -> Result<(), TrapReason> { let output = mem::take(self.ext.last_frame_output_mut()); - if offset as usize > output.data.len() { - return Err(Error::::OutOfBounds.into()); - } - let result = self.write_sandbox_output( - memory, - out_ptr, - out_len_ptr, - &output.data[offset as usize..], - false, - |len| Some(RuntimeCosts::CopyToContract(len)), - ); + let result = if offset as usize > output.data.len() { + Err(Error::::OutOfBounds.into()) + } else { + self.write_sandbox_output( + memory, + out_ptr, + out_len_ptr, + &output.data[offset as usize..], + false, + |len| Some(RuntimeCosts::CopyToContract(len)), + ) + }; *self.ext.last_frame_output_mut() = output; Ok(result?) } From f198cf543e5e1aa4304df9f8d20adf02e15be7ee Mon Sep 17 00:00:00 2001 From: xermicus Date: Fri, 20 Sep 2024 15:44:19 +0200 Subject: [PATCH 11/25] fix Signed-off-by: xermicus --- substrate/frame/revive/src/wasm/runtime.rs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/substrate/frame/revive/src/wasm/runtime.rs b/substrate/frame/revive/src/wasm/runtime.rs index 66fe0c20f569..b50397ca803e 100644 --- a/substrate/frame/revive/src/wasm/runtime.rs +++ b/substrate/frame/revive/src/wasm/runtime.rs @@ -1026,26 +1026,28 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { }, }; - let output = mem::take(self.ext.last_frame_output_mut()); match call_outcome { // `TAIL_CALL` only matters on an `OK` result. Otherwise the call stack comes to // a halt anyways without anymore code being executed. Ok(_) if flags.contains(CallFlags::TAIL_CALL) => { + let output = mem::take(self.ext.last_frame_output_mut()); return Err(TrapReason::Return(ReturnData { flags: output.flags.bits(), data: output.data, })); }, Ok(_) => { - self.write_sandbox_output( + let output = mem::take(self.ext.last_frame_output_mut()); + let write_result = self.write_sandbox_output( memory, output_ptr, output_len_ptr, &output.data, true, |len| Some(RuntimeCosts::CopyToContract(len)), - )?; + ); *self.ext.last_frame_output_mut() = output; + write_result?; Ok(self.ext.last_frame_output().into()) }, Err(err) => Ok(Self::exec_error_into_return_code(err)?), @@ -1088,8 +1090,7 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { salt.as_ref(), ) { Ok(address) => { - let output = mem::take(self.ext.last_frame_output_mut()); - if !output.flags.contains(ReturnFlags::REVERT) { + if !self.ext.last_frame_output().flags.contains(ReturnFlags::REVERT) { self.write_fixed_sandbox_output( memory, address_ptr, @@ -1098,15 +1099,17 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { already_charged, )?; } - self.write_sandbox_output( + let output = mem::take(self.ext.last_frame_output_mut()); + let write_result = self.write_sandbox_output( memory, output_ptr, output_len_ptr, &output.data, true, |len| Some(RuntimeCosts::CopyToContract(len)), - )?; + ); *self.ext.last_frame_output_mut() = output; + write_result?; Ok(self.ext.last_frame_output().into()) }, Err(err) => Ok(Self::exec_error_into_return_code(err)?), From 8e8b2a25f102e93cc80f1e3df68a89767db75ac6 Mon Sep 17 00:00:00 2001 From: xermicus Date: Fri, 20 Sep 2024 17:16:40 +0200 Subject: [PATCH 12/25] test return data api on instantiate in the mock Signed-off-by: xermicus --- substrate/frame/revive/src/exec.rs | 78 +++++++++++++++++++++++++++++- 1 file changed, 77 insertions(+), 1 deletion(-) diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index eac98bdddda0..fea5ef4abb5d 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -549,7 +549,7 @@ struct Frame { read_only: bool, /// The caller of the currently executing frame which was spawned by `delegate_call`. delegate_caller: Option>, - /// The output of the last executed call frame. + /// The output of the last executed call frame. last_frame_output: ExecReturnValue, } @@ -4132,4 +4132,80 @@ mod tests { assert_matches!(result, Ok(_)); }); } + + #[test] + fn last_frame_output_works_on_instantiate() { + let ok_ch = MockLoader::insert(Constructor, move |_, _| { + Ok(ExecReturnValue { flags: ReturnFlags::empty(), data: vec![127] }) + }); + let revert_ch = MockLoader::insert(Constructor, move |_, _| { + Ok(ExecReturnValue { flags: ReturnFlags::REVERT, data: vec![70] }) + }); + let trap_ch = MockLoader::insert(Constructor, |_, _| Err("It's a trap!".into())); + let instantiator_ch = MockLoader::insert(Call, { + move |ctx, _| { + let value = ::Currency::minimum_balance().into(); + + // Successful instantiation should set the output + let address = ctx + .ext + .instantiate(Weight::zero(), U256::zero(), ok_ch, value, vec![], None) + .unwrap(); + assert_eq!( + ctx.ext.last_frame_output(), + &ExecReturnValue { flags: ReturnFlags::empty(), data: vec![127] } + ); + + // Plain transfers should not set the output + ctx.ext.transfer(&address, U256::from(1)).unwrap(); + assert_eq!( + ctx.ext.last_frame_output(), + &ExecReturnValue { flags: ReturnFlags::empty(), data: vec![127] } + ); + + // Reverted instantiation should set the output + ctx.ext + .instantiate(Weight::zero(), U256::zero(), revert_ch, value, vec![], None) + .unwrap(); + assert_eq!( + ctx.ext.last_frame_output(), + &ExecReturnValue { flags: ReturnFlags::REVERT, data: vec![70] } + ); + + // Trapped instantiation should clear the output + ctx.ext + .instantiate(Weight::zero(), U256::zero(), trap_ch, value, vec![], None) + .unwrap_err(); + assert_eq!( + ctx.ext.last_frame_output(), + &ExecReturnValue { flags: ReturnFlags::empty(), data: vec![] } + ); + + exec_success() + } + }); + + ExtBuilder::default() + .with_code_hashes(MockLoader::code_hashes()) + .existential_deposit(15) + .build() + .execute_with(|| { + set_balance(&ALICE, 1000); + set_balance(&BOB_CONTRACT_ID, 100); + place_contract(&BOB, instantiator_ch); + let origin = Origin::from_account_id(ALICE); + let mut storage_meter = storage::meter::Meter::new(&origin, 200, 0).unwrap(); + + MockStack::run_call( + origin, + BOB_ADDR, + &mut GasMeter::::new(GAS_LIMIT), + &mut storage_meter, + 0, + vec![], + None, + ) + .unwrap() + }); + } } From 4caa6a06ee265a942952f468241a52b12287ae83 Mon Sep 17 00:00:00 2001 From: xermicus Date: Fri, 20 Sep 2024 17:45:49 +0200 Subject: [PATCH 13/25] test return data api on nested calls in the mock Signed-off-by: xermicus --- .../fixtures/contracts/return_data_api.rs | 2 +- substrate/frame/revive/src/exec.rs | 68 +++++++++++++++++++ 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/substrate/frame/revive/fixtures/contracts/return_data_api.rs b/substrate/frame/revive/fixtures/contracts/return_data_api.rs index 1afc161b11cd..846396b0944d 100644 --- a/substrate/frame/revive/fixtures/contracts/return_data_api.rs +++ b/substrate/frame/revive/fixtures/contracts/return_data_api.rs @@ -112,7 +112,7 @@ pub extern "C" fn deploy() {} pub extern "C" fn call() { input!(code_hash: &[u8; 32],); - // we didn't do anything yet; return data size should be 0 + // We didn't do anything yet; return data size should be 0 assert_return_data_size_of(0); recursion_guard(); diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index fea5ef4abb5d..19792eac3fdb 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -4208,4 +4208,72 @@ mod tests { .unwrap() }); } + + #[test] + fn last_frame_output_works_on_nested_call() { + // Call stack: BOB -> CHARLIE(revert) -> BOB' (success) + let code_bob = MockLoader::insert(Call, |ctx, _| { + if ctx.input_data.is_empty() { + // We didn't do anything yet + assert_eq!( + ctx.ext.last_frame_output(), + &ExecReturnValue { flags: ReturnFlags::empty(), data: vec![] } + ); + + ctx.ext + .call( + Weight::zero(), + U256::zero(), + &CHARLIE_ADDR, + U256::zero(), + vec![], + true, + false, + ) + .unwrap(); + assert_eq!( + ctx.ext.last_frame_output(), + &ExecReturnValue { flags: ReturnFlags::REVERT, data: vec![70] } + ); + } + + Ok(ExecReturnValue { flags: ReturnFlags::empty(), data: vec![127] }) + }); + let code_charlie = MockLoader::insert(Call, |ctx, _| { + // We didn't do anything yet + assert_eq!( + ctx.ext.last_frame_output(), + &ExecReturnValue { flags: ReturnFlags::empty(), data: vec![] } + ); + + assert!(ctx + .ext + .call(Weight::zero(), U256::zero(), &BOB_ADDR, U256::zero(), vec![99], true, false) + .is_ok()); + assert_eq!( + ctx.ext.last_frame_output(), + &ExecReturnValue { flags: ReturnFlags::empty(), data: vec![127] } + ); + + Ok(ExecReturnValue { flags: ReturnFlags::REVERT, data: vec![70] }) + }); + + ExtBuilder::default().build().execute_with(|| { + place_contract(&BOB, code_bob); + place_contract(&CHARLIE, code_charlie); + let origin = Origin::from_account_id(ALICE); + let mut storage_meter = storage::meter::Meter::new(&origin, 0, 0).unwrap(); + + let result = MockStack::run_call( + origin, + BOB_ADDR, + &mut GasMeter::::new(GAS_LIMIT), + &mut storage_meter, + 0, + vec![0], + None, + ); + assert_matches!(result, Ok(_)); + }); + } } From a04cfb7ef41bf849117bee471cab64f03c68c966 Mon Sep 17 00:00:00 2001 From: xermicus Date: Fri, 20 Sep 2024 18:16:19 +0200 Subject: [PATCH 14/25] prdoc Signed-off-by: xermicus --- prdoc/pr_5779.prdoc | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 prdoc/pr_5779.prdoc diff --git a/prdoc/pr_5779.prdoc b/prdoc/pr_5779.prdoc new file mode 100644 index 000000000000..659a3a19f695 --- /dev/null +++ b/prdoc/pr_5779.prdoc @@ -0,0 +1,38 @@ +title: "[pallet-revive] last call return data API" + +doc: + - audience: Runtime Dev + description: | + This PR introduces 2 new syscall: `return_data_size` and `return_data_copy`, + resembling the semantics of the EVM `RETURNDATASIZE` and `RETURNDATACOPY` opcodes. + + The ownership of `ExecReturnValue` (the return data) has moved to the `Frame`. + This allows implementing the new contract API surface functionality in ext with no additional copies. + Returned data is passed via contract memory, memory is (will be) metered, + hence the amount of returned data can not be statically known, + so we should avoid storing copies of the returned data if we can. + By moving the ownership of the exectuables return value into the `Frame` struct we achieve this. + + A zero-copy implementation of those APIs would be technically possible without that internal change by making + the callsite in the runtime responsible for moving the returned data into the frame after any call. + However, resetting the stored output needs to be handled in ext, since plain transfers will _not_ affect the + stored return data (and we don't want to handle this special call case inside the `runtime` API). + This has drawbacks: + - It can not be tested easily in the mock. + - It introduces an inconsistency where resetting the stored output is handled in ext, + but the runtime API is responsible to store it back correctly after any calls made. + Instead, with ownership of the data in `Frame`, both can be handled in a single place. + Handling both in `fn run()` is more natural and leaves less room for runtime API bugs. + + The returned output is reset each time _before_ running any executable in a nested stack. + This change should not incur any overhead to the overall memory usage as _only_ the returned data from the last + executed frame will be kept around at any time. + +crates: + - name: pallet-revive + bump: major + - name: pallet-revive-fixtures + bump: minor + - name: pallet-revive-uapi + bump: minor + \ No newline at end of file From fcacb897d46dd142014bc8a2df2ef9e74fc7f156 Mon Sep 17 00:00:00 2001 From: command-bot <> Date: Fri, 20 Sep 2024 16:54:07 +0000 Subject: [PATCH 15/25] ".git/.scripts/commands/fmt/fmt.sh" --- substrate/frame/revive/src/tests.rs | 3 ++- substrate/frame/revive/src/wasm/runtime.rs | 5 ++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/substrate/frame/revive/src/tests.rs b/substrate/frame/revive/src/tests.rs index ec8626722e62..0c86b53665a1 100644 --- a/substrate/frame/revive/src/tests.rs +++ b/substrate/frame/revive/src/tests.rs @@ -4314,7 +4314,8 @@ mod run_tests { #[test] fn return_data_api_works() { let (code_return_data_api, _) = compile_module("return_data_api").unwrap(); - let (code_return_with_data, hash_return_with_data) = compile_module("return_with_data").unwrap(); + let (code_return_with_data, hash_return_with_data) = + compile_module("return_with_data").unwrap(); ExtBuilder::default().existential_deposit(100).build().execute_with(|| { let _ = ::Currency::set_balance(&ALICE, 1_000_000); diff --git a/substrate/frame/revive/src/wasm/runtime.rs b/substrate/frame/revive/src/wasm/runtime.rs index b50397ca803e..b2023d0e6d49 100644 --- a/substrate/frame/revive/src/wasm/runtime.rs +++ b/substrate/frame/revive/src/wasm/runtime.rs @@ -1725,9 +1725,8 @@ pub mod env { Environment::new(self, memory, id, input_ptr, input_len, output_ptr, output_len_ptr); let ret = match chain_extension.call(env)? { RetVal::Converging(val) => Ok(val), - RetVal::Diverging { flags, data } => { - Err(TrapReason::Return(ReturnData { flags: flags.bits(), data })) - }, + RetVal::Diverging { flags, data } => + Err(TrapReason::Return(ReturnData { flags: flags.bits(), data })), }; self.chain_extension = Some(chain_extension); ret From b6c37057324099fd98a1e347d35155d5cbb1d9b4 Mon Sep 17 00:00:00 2001 From: xermicus Date: Tue, 24 Sep 2024 14:45:54 +0200 Subject: [PATCH 16/25] work around borrow checker limitations Signed-off-by: xermicus --- substrate/frame/revive/src/chain_extension.rs | 9 +- substrate/frame/revive/src/wasm/mod.rs | 4 +- substrate/frame/revive/src/wasm/runtime.rs | 278 ++++++------------ 3 files changed, 104 insertions(+), 187 deletions(-) diff --git a/substrate/frame/revive/src/chain_extension.rs b/substrate/frame/revive/src/chain_extension.rs index ccea12945054..ce1943cf978f 100644 --- a/substrate/frame/revive/src/chain_extension.rs +++ b/substrate/frame/revive/src/chain_extension.rs @@ -344,15 +344,16 @@ impl<'a, 'b, E: Ext, M: ?Sized + Memory> Environment<'a, 'b, E, M> { allow_skip: bool, weight_per_byte: Option, ) -> Result<()> { - self.runtime.write_sandbox_output( + if let Some(weight_per_byte) = weight_per_byte { + let weight = weight_per_byte.saturating_mul(buffer.len() as u64); + self.runtime.charge_gas(RuntimeCosts::ChainExtension(weight))?; + } + super::wasm::write_sandbox_output::( self.memory, self.output_ptr, self.output_len_ptr, buffer, allow_skip, - |len| { - weight_per_byte.map(|w| RuntimeCosts::ChainExtension(w.saturating_mul(len.into()))) - }, ) } } diff --git a/substrate/frame/revive/src/wasm/mod.rs b/substrate/frame/revive/src/wasm/mod.rs index cd274873975d..17b6c0202e2c 100644 --- a/substrate/frame/revive/src/wasm/mod.rs +++ b/substrate/frame/revive/src/wasm/mod.rs @@ -29,7 +29,7 @@ pub use runtime::HIGHEST_API_VERSION; #[cfg(all(feature = "runtime-benchmarks", feature = "riscv"))] pub use crate::wasm::runtime::{ReturnData, TrapReason}; -pub use crate::wasm::runtime::{ApiVersion, Memory, Runtime, RuntimeCosts}; +pub use crate::wasm::runtime::{write_sandbox_output, ApiVersion, Memory, Runtime, RuntimeCosts}; use crate::{ address::AddressMapper, @@ -268,7 +268,7 @@ where &mut self.instance, self.api_version, ) { - break exec_result + break exec_result; } }; let _ = self.runtime.ext().gas_meter_mut().sync_from_executor(self.instance.gas())?; diff --git a/substrate/frame/revive/src/wasm/runtime.rs b/substrate/frame/revive/src/wasm/runtime.rs index 74cb2c34f0f4..e96fe57477e8 100644 --- a/substrate/frame/revive/src/wasm/runtime.rs +++ b/substrate/frame/revive/src/wasm/runtime.rs @@ -538,13 +538,6 @@ impl CallType { } } -/// This is only appropriate when writing out data of constant size that does not depend on user -/// input. In this case the costs for this copy was already charged as part of the token at -/// the beginning of the API entry point. -fn already_charged(_: u32) -> Option { - None -} - /// Can only be used for one call. pub struct Runtime<'a, E: Ext, M: ?Sized> { ext: &'a mut E, @@ -600,6 +593,58 @@ impl<'a, E: Ext, M: PolkaVmInstance> Runtime<'a, E, M> { } } +/// Write the given buffer and its length to the designated locations in sandbox memory and +/// charge gas according to the token returned by `create_token`. +/// +/// `out_ptr` is the location in sandbox memory where `buf` should be written to. +/// `out_len_ptr` is an in-out location in sandbox memory. It is read to determine the +/// length of the buffer located at `out_ptr`. If that buffer is smaller than the actual +/// `buf.len()`, only what fits into that buffer is written to `out_ptr`. +/// The actual amount of bytes copied to `out_ptr` is written to `out_len_ptr`. +/// +/// If `out_ptr` is set to the sentinel value of `SENTINEL` and `allow_skip` is true the +/// operation is skipped and `Ok` is returned. This is supposed to help callers to make copying +/// output optional. For example to skip copying back the output buffer of an `seal_call` +/// when the caller is not interested in the result. +/// +/// `create_token` can optionally instruct this function to charge the gas meter with the token +/// it returns. `create_token` receives the variable amount of bytes that are about to be copied +/// by this function. +/// +/// In addition to the error conditions of `Memory::write` this functions returns +/// `Err` if the size of the buffer located at `out_ptr` is too small to fit `buf`. +pub fn write_sandbox_output>( + memory: &mut M, + out_ptr: u32, + out_len_ptr: u32, + buf: &[u8], + allow_skip: bool, +) -> Result<(), DispatchError> { + if allow_skip && out_ptr == SENTINEL { + return Ok(()); + } + + let len = memory.read_u32(out_len_ptr)?; + let buf_len = len.min(buf.len() as u32); + + memory.write(out_ptr, &buf[..buf_len as usize])?; + memory.write(out_len_ptr, &buf_len.encode()) +} + +/// Same as `write_sandbox_output` but for static size output. +pub fn write_fixed_sandbox_output>( + memory: &mut M, + out_ptr: u32, + buf: &[u8], + allow_skip: bool, +) -> Result<(), DispatchError> { + if allow_skip && out_ptr == SENTINEL { + return Ok(()); + } + + memory.write(out_ptr, buf) +} + impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { pub fn new(ext: &'a mut E, input_data: Vec) -> Self { Self { @@ -657,71 +702,6 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { } } - /// Write the given buffer and its length to the designated locations in sandbox memory and - /// charge gas according to the token returned by `create_token`. - /// - /// `out_ptr` is the location in sandbox memory where `buf` should be written to. - /// `out_len_ptr` is an in-out location in sandbox memory. It is read to determine the - /// length of the buffer located at `out_ptr`. If that buffer is smaller than the actual - /// `buf.len()`, only what fits into that buffer is written to `out_ptr`. - /// The actual amount of bytes copied to `out_ptr` is written to `out_len_ptr`. - /// - /// If `out_ptr` is set to the sentinel value of `SENTINEL` and `allow_skip` is true the - /// operation is skipped and `Ok` is returned. This is supposed to help callers to make copying - /// output optional. For example to skip copying back the output buffer of an `seal_call` - /// when the caller is not interested in the result. - /// - /// `create_token` can optionally instruct this function to charge the gas meter with the token - /// it returns. `create_token` receives the variable amount of bytes that are about to be copied - /// by this function. - /// - /// In addition to the error conditions of `Memory::write` this functions returns - /// `Err` if the size of the buffer located at `out_ptr` is too small to fit `buf`. - pub fn write_sandbox_output( - &mut self, - memory: &mut M, - out_ptr: u32, - out_len_ptr: u32, - buf: &[u8], - allow_skip: bool, - create_token: impl FnOnce(u32) -> Option, - ) -> Result<(), DispatchError> { - if allow_skip && out_ptr == SENTINEL { - return Ok(()); - } - - let len = memory.read_u32(out_len_ptr)?; - let buf_len = len.min(buf.len() as u32); - - if let Some(costs) = create_token(buf_len) { - self.charge_gas(costs)?; - } - - memory.write(out_ptr, &buf[..buf_len as usize])?; - memory.write(out_len_ptr, &buf_len.encode()) - } - - /// Same as `write_sandbox_output` but for static size output. - pub fn write_fixed_sandbox_output( - &mut self, - memory: &mut M, - out_ptr: u32, - buf: &[u8], - allow_skip: bool, - create_token: impl FnOnce(u32) -> Option, - ) -> Result<(), DispatchError> { - if allow_skip && out_ptr == SENTINEL { - return Ok(()); - } - - let buf_len = buf.len() as u32; - if let Some(costs) = create_token(buf_len) { - self.charge_gas(costs)?; - } - - memory.write(out_ptr, buf) - } - /// Computes the given hash function on the supplied input. /// /// Reads from the sandboxed input buffer into an intermediate buffer. @@ -889,14 +869,7 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { }; if let Some(value) = outcome { self.adjust_gas(charged, costs(value.len() as u32)); - self.write_sandbox_output( - memory, - out_ptr, - out_len_ptr, - &value, - false, - already_charged, - )?; + write_sandbox_output::(memory, out_ptr, out_len_ptr, &value, false)?; Ok(ReturnErrorCode::Success) } else { self.adjust_gas(charged, costs(0)); @@ -957,14 +930,7 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { if let crate::storage::WriteOutcome::Taken(value) = outcome { self.adjust_gas(charged, costs(value.len() as u32)); - self.write_sandbox_output( - memory, - out_ptr, - out_len_ptr, - &value, - false, - already_charged, - )?; + write_sandbox_output::(memory, out_ptr, out_len_ptr, &value, false)?; Ok(ReturnErrorCode::Success) } else { self.adjust_gas(charged, costs(0)); @@ -1045,17 +1011,16 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { })); }, Ok(_) => { - let output = mem::take(self.ext.last_frame_output_mut()); - let write_result = self.write_sandbox_output( + self.charge_gas(RuntimeCosts::CopyToContract( + self.ext.last_frame_output().data.len() as u32, + ))?; + write_sandbox_output::( memory, output_ptr, output_len_ptr, - &output.data, + &self.ext.last_frame_output().data, true, - |len| Some(RuntimeCosts::CopyToContract(len)), - ); - *self.ext.last_frame_output_mut() = output; - write_result?; + )?; Ok(self.ext.last_frame_output().into()) }, Err(err) => Ok(Self::exec_error_into_return_code(err)?), @@ -1099,25 +1064,23 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { ) { Ok(address) => { if !self.ext.last_frame_output().flags.contains(ReturnFlags::REVERT) { - self.write_fixed_sandbox_output( + write_fixed_sandbox_output::( memory, address_ptr, &address.as_bytes(), true, - already_charged, )?; } - let output = mem::take(self.ext.last_frame_output_mut()); - let write_result = self.write_sandbox_output( + self.charge_gas(RuntimeCosts::CopyToContract( + self.ext.last_frame_output().data.len() as u32, + ))?; + write_sandbox_output::( memory, output_ptr, output_len_ptr, - &output.data, + &self.ext.last_frame_output().data, true, - |len| Some(RuntimeCosts::CopyToContract(len)), - ); - *self.ext.last_frame_output_mut() = output; - write_result?; + )?; Ok(self.ext.last_frame_output().into()) }, Err(err) => Ok(Self::exec_error_into_return_code(err)?), @@ -1352,9 +1315,8 @@ pub mod env { #[api_version(0)] fn input(&mut self, memory: &mut M, out_ptr: u32, out_len_ptr: u32) -> Result<(), TrapReason> { if let Some(input) = self.input_data.take() { - self.write_sandbox_output(memory, out_ptr, out_len_ptr, &input, false, |len| { - Some(RuntimeCosts::CopyToContract(len)) - })?; + self.charge_gas(RuntimeCosts::CopyToContract(input.len() as u32))?; + write_sandbox_output::(memory, out_ptr, out_len_ptr, &input, false)?; self.input_data = Some(input); Ok(()) } else { @@ -1382,13 +1344,7 @@ pub mod env { fn caller(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { self.charge_gas(RuntimeCosts::Caller)?; let caller = ::AddressMapper::to_address(self.ext.caller().account_id()?); - Ok(self.write_fixed_sandbox_output( - memory, - out_ptr, - caller.as_bytes(), - false, - already_charged, - )?) + Ok(write_fixed_sandbox_output::(memory, out_ptr, caller.as_bytes(), false)?) } /// Checks whether a specified address belongs to a contract. @@ -1414,13 +1370,7 @@ pub mod env { let mut address = H160::zero(); memory.read_into_buf(addr_ptr, address.as_bytes_mut())?; if let Some(value) = self.ext.code_hash(&address) { - self.write_fixed_sandbox_output( - memory, - out_ptr, - &value.as_bytes(), - false, - already_charged, - )?; + write_fixed_sandbox_output::(memory, out_ptr, &value.as_bytes(), false)?; Ok(ReturnErrorCode::Success) } else { Ok(ReturnErrorCode::KeyNotFound) @@ -1433,13 +1383,7 @@ pub mod env { fn own_code_hash(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { self.charge_gas(RuntimeCosts::OwnCodeHash)?; let code_hash = *self.ext.own_code_hash(); - Ok(self.write_fixed_sandbox_output( - memory, - out_ptr, - code_hash.as_bytes(), - false, - already_charged, - )?) + Ok(write_fixed_sandbox_output::(memory, out_ptr, code_hash.as_bytes(), false)?) } /// Checks whether the caller of the current contract is the origin of the whole call stack. @@ -1464,13 +1408,7 @@ pub mod env { fn address(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { self.charge_gas(RuntimeCosts::Address)?; let address = self.ext.address(); - Ok(self.write_fixed_sandbox_output( - memory, - out_ptr, - address.as_bytes(), - false, - already_charged, - )?) + Ok(write_fixed_sandbox_output::(memory, out_ptr, address.as_bytes(), false)?) } /// Stores the price for the specified amount of weight into the supplied buffer. @@ -1485,12 +1423,11 @@ pub mod env { ) -> Result<(), TrapReason> { let weight = Weight::from_parts(ref_time_limit, proof_size_limit); self.charge_gas(RuntimeCosts::WeightToFee)?; - Ok(self.write_fixed_sandbox_output( + Ok(write_fixed_sandbox_output::( memory, out_ptr, &self.ext.get_weight_price(weight).encode(), false, - already_charged, )?) } @@ -1505,14 +1442,7 @@ pub mod env { ) -> Result<(), TrapReason> { self.charge_gas(RuntimeCosts::GasLeft)?; let gas_left = &self.ext.gas_meter().gas_left().encode(); - Ok(self.write_sandbox_output( - memory, - out_ptr, - out_len_ptr, - gas_left, - false, - already_charged, - )?) + Ok(write_sandbox_output::(memory, out_ptr, out_len_ptr, gas_left, false)?) } /// Stores the *free* balance of the current account into the supplied buffer. @@ -1520,12 +1450,11 @@ pub mod env { #[api_version(0)] fn balance(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { self.charge_gas(RuntimeCosts::Balance)?; - Ok(self.write_fixed_sandbox_output( + Ok(write_fixed_sandbox_output::( memory, out_ptr, &as_bytes(self.ext.balance()), false, - already_charged, )?) } @@ -1541,12 +1470,11 @@ pub mod env { self.charge_gas(RuntimeCosts::BalanceOf)?; let mut address = H160::zero(); memory.read_into_buf(addr_ptr, address.as_bytes_mut())?; - Ok(self.write_fixed_sandbox_output( + Ok(write_fixed_sandbox_output::( memory, out_ptr, &as_bytes(self.ext.balance_of(&address)), false, - already_charged, )?) } @@ -1554,12 +1482,12 @@ pub mod env { /// See [`pallet_revive_uapi::HostFn::chain_id`]. #[api_version(0)] fn chain_id(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { - Ok(self.write_fixed_sandbox_output( + self.charge_gas(RuntimeCosts::CopyToContract(32))?; + Ok(write_fixed_sandbox_output::( memory, out_ptr, &as_bytes(U256::from(::ChainId::get())), false, - |_| Some(RuntimeCosts::CopyToContract(32)), )?) } @@ -1568,12 +1496,11 @@ pub mod env { #[api_version(0)] fn value_transferred(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { self.charge_gas(RuntimeCosts::ValueTransferred)?; - Ok(self.write_fixed_sandbox_output( + Ok(write_fixed_sandbox_output::( memory, out_ptr, &as_bytes(self.ext.value_transferred()), false, - already_charged, )?) } @@ -1582,13 +1509,7 @@ pub mod env { #[api_version(0)] fn now(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { self.charge_gas(RuntimeCosts::Now)?; - Ok(self.write_fixed_sandbox_output( - memory, - out_ptr, - &as_bytes(self.ext.now()), - false, - already_charged, - )?) + Ok(write_fixed_sandbox_output::(memory, out_ptr, &as_bytes(self.ext.now()), false)?) } /// Stores the minimum balance (a.k.a. existential deposit) into the supplied buffer. @@ -1596,12 +1517,11 @@ pub mod env { #[api_version(0)] fn minimum_balance(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { self.charge_gas(RuntimeCosts::MinimumBalance)?; - Ok(self.write_fixed_sandbox_output( + Ok(write_fixed_sandbox_output::( memory, out_ptr, &as_bytes(self.ext.minimum_balance()), false, - already_charged, )?) } @@ -1650,12 +1570,11 @@ pub mod env { #[api_version(0)] fn block_number(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { self.charge_gas(RuntimeCosts::BlockNumber)?; - Ok(self.write_fixed_sandbox_output( + Ok(write_fixed_sandbox_output::( memory, out_ptr, &as_bytes(self.ext.block_number()), false, - already_charged, )?) } @@ -2007,12 +1926,12 @@ pub mod env { /// See [`pallet_revive_uapi::HostFn::return_data_size`]. #[api_version(0)] fn return_data_size(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { - Ok(self.write_fixed_sandbox_output( + self.charge_gas(RuntimeCosts::CopyToContract(32))?; + Ok(write_fixed_sandbox_output::( memory, out_ptr, &as_bytes(U256::from(self.ext.last_frame_output().data.len())), false, - |len| Some(RuntimeCosts::CopyToContract(len)), )?) } @@ -2026,20 +1945,17 @@ pub mod env { out_len_ptr: u32, offset: u32, ) -> Result<(), TrapReason> { - let output = mem::take(self.ext.last_frame_output_mut()); - let result = if offset as usize > output.data.len() { - Err(Error::::OutOfBounds.into()) - } else { - self.write_sandbox_output( - memory, - out_ptr, - out_len_ptr, - &output.data[offset as usize..], - false, - |len| Some(RuntimeCosts::CopyToContract(len)), - ) - }; - *self.ext.last_frame_output_mut() = output; - Ok(result?) + let len = self.ext.last_frame_output().data.len(); + if offset as usize > len { + return Err(Error::::OutOfBounds.into()); + } + self.charge_gas(RuntimeCosts::CopyToContract(len as u32))?; + Ok(write_sandbox_output::( + memory, + out_ptr, + out_len_ptr, + &self.ext.last_frame_output().data[offset as usize..], + false, + )?) } } From 7b69ee793df867631e882f5d6e2f72b3fb86fbe4 Mon Sep 17 00:00:00 2001 From: command-bot <> Date: Tue, 24 Sep 2024 12:50:43 +0000 Subject: [PATCH 17/25] ".git/.scripts/commands/fmt/fmt.sh" --- substrate/frame/revive/src/wasm/runtime.rs | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/substrate/frame/revive/src/wasm/runtime.rs b/substrate/frame/revive/src/wasm/runtime.rs index e96fe57477e8..0f6f006155b1 100644 --- a/substrate/frame/revive/src/wasm/runtime.rs +++ b/substrate/frame/revive/src/wasm/runtime.rs @@ -562,9 +562,8 @@ impl<'a, E: Ext, M: PolkaVmInstance> Runtime<'a, E, M> { log::error!(target: LOG_TARGET, "polkavm execution error: {error}"); Some(Err(Error::::ExecutionFailed.into())) }, - Ok(Finished) => { - Some(Ok(ExecReturnValue { flags: ReturnFlags::empty(), data: Vec::new() })) - }, + Ok(Finished) => + Some(Ok(ExecReturnValue { flags: ReturnFlags::empty(), data: Vec::new() })), Ok(Trap) => Some(Err(Error::::ContractTrapped.into())), Ok(Segfault(_)) => Some(Err(Error::::ExecutionFailed.into())), Ok(NotEnoughGas) => Some(Err(Error::::OutOfGas.into())), @@ -579,12 +578,11 @@ impl<'a, E: Ext, M: PolkaVmInstance> Runtime<'a, E, M> { instance.write_output(return_value); None }, - Err(TrapReason::Return(ReturnData { flags, data })) => { + Err(TrapReason::Return(ReturnData { flags, data })) => match ReturnFlags::from_bits(flags) { None => Some(Err(Error::::InvalidCallFlags.into())), Some(flags) => Some(Ok(ExecReturnValue { flags, data })), - } - }, + }, Err(TrapReason::Termination) => Some(Ok(Default::default())), Err(TrapReason::SupervisorError(error)) => Some(Err(error.into())), } @@ -1665,9 +1663,8 @@ pub mod env { Environment::new(self, memory, id, input_ptr, input_len, output_ptr, output_len_ptr); let ret = match chain_extension.call(env)? { RetVal::Converging(val) => Ok(val), - RetVal::Diverging { flags, data } => { - Err(TrapReason::Return(ReturnData { flags: flags.bits(), data })) - }, + RetVal::Diverging { flags, data } => + Err(TrapReason::Return(ReturnData { flags: flags.bits(), data })), }; self.chain_extension = Some(chain_extension); ret From 1b83cf5fca9280251d5378709830bd0238ab52d3 Mon Sep 17 00:00:00 2001 From: xermicus Date: Tue, 24 Sep 2024 17:36:38 +0200 Subject: [PATCH 18/25] remove take_caller_output helper and provide a comment why we want to drop early Signed-off-by: xermicus --- substrate/frame/revive/src/exec.rs | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index 19792eac3fdb..56e4ebdf225b 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -871,7 +871,7 @@ where { contract } else { - return Ok(None) + return Ok(None); } }; @@ -978,7 +978,18 @@ where let delegated_code_hash = if frame.delegate_caller.is_some() { Some(*executable.code_hash()) } else { None }; - self.take_caller_output(); + // The output of the caller frame will be replaced by the output of this run. + // It is also not accessible from nested frames. + // Hence we drop it early to save the memory. + let frames_len = self.frames.len(); + if let Some(caller_frame) = match frames_len { + 0 => None, + 1 => Some(&mut self.first_frame.last_frame_output), + _ => self.frames.get_mut(frames_len - 2).map(|frame| &mut frame.last_frame_output), + } { + *caller_frame = Default::default(); + } + self.transient_storage.start_transaction(); let do_transaction = || { @@ -1096,8 +1107,9 @@ where with_transaction(|| -> TransactionOutcome> { let output = do_transaction(); match &output { - Ok(result) if !result.did_revert() => - TransactionOutcome::Commit(Ok((true, output))), + Ok(result) if !result.did_revert() => { + TransactionOutcome::Commit(Ok((true, output))) + }, _ => TransactionOutcome::Rollback(Ok((false, output))), } }); @@ -1273,16 +1285,6 @@ where fn account_balance(&self, who: &T::AccountId) -> U256 { T::Currency::reducible_balance(who, Preservation::Preserve, Fortitude::Polite).into() } - - /// Replaces the `last_frame_output` of the **caller** frame with the default value. - fn take_caller_output(&mut self) { - let len = self.frames.len(); - mem::take(match len { - 0 => return, - 1 => &mut self.first_frame.last_frame_output, - _ => &mut self.frames.get_mut(len - 2).expect("len >= 2; qed").last_frame_output, - }); - } } impl<'a, T, E> Ext for Stack<'a, T, E> From 2e1c5fcfa24f57109e45556dd6999a4bcc543c80 Mon Sep 17 00:00:00 2001 From: xermicus Date: Tue, 24 Sep 2024 17:57:49 +0200 Subject: [PATCH 19/25] move sandbox write helpers into Memory trait Signed-off-by: xermicus --- substrate/frame/revive/src/chain_extension.rs | 9 +- substrate/frame/revive/src/wasm/mod.rs | 2 +- substrate/frame/revive/src/wasm/runtime.rs | 185 ++++++++---------- 3 files changed, 85 insertions(+), 111 deletions(-) diff --git a/substrate/frame/revive/src/chain_extension.rs b/substrate/frame/revive/src/chain_extension.rs index ce1943cf978f..6135070708bb 100644 --- a/substrate/frame/revive/src/chain_extension.rs +++ b/substrate/frame/revive/src/chain_extension.rs @@ -348,12 +348,7 @@ impl<'a, 'b, E: Ext, M: ?Sized + Memory> Environment<'a, 'b, E, M> { let weight = weight_per_byte.saturating_mul(buffer.len() as u64); self.runtime.charge_gas(RuntimeCosts::ChainExtension(weight))?; } - super::wasm::write_sandbox_output::( - self.memory, - self.output_ptr, - self.output_len_ptr, - buffer, - allow_skip, - ) + self.memory + .write_sandbox_output(self.output_ptr, self.output_len_ptr, buffer, allow_skip) } } diff --git a/substrate/frame/revive/src/wasm/mod.rs b/substrate/frame/revive/src/wasm/mod.rs index 17b6c0202e2c..383086800aa1 100644 --- a/substrate/frame/revive/src/wasm/mod.rs +++ b/substrate/frame/revive/src/wasm/mod.rs @@ -29,7 +29,7 @@ pub use runtime::HIGHEST_API_VERSION; #[cfg(all(feature = "runtime-benchmarks", feature = "riscv"))] pub use crate::wasm::runtime::{ReturnData, TrapReason}; -pub use crate::wasm::runtime::{write_sandbox_output, ApiVersion, Memory, Runtime, RuntimeCosts}; +pub use crate::wasm::runtime::{ApiVersion, Memory, Runtime, RuntimeCosts}; use crate::{ address::AddressMapper, diff --git a/substrate/frame/revive/src/wasm/runtime.rs b/substrate/frame/revive/src/wasm/runtime.rs index 0f6f006155b1..a05148e0edbf 100644 --- a/substrate/frame/revive/src/wasm/runtime.rs +++ b/substrate/frame/revive/src/wasm/runtime.rs @@ -154,6 +154,58 @@ pub trait Memory { .map_err(|_| DispatchError::from(Error::::DecodingFailed))?; Ok(decoded) } + + /// Write the given buffer and its length to the designated locations in sandbox memory and + /// charge gas according to the token returned by `create_token`. + /// + /// `out_ptr` is the location in sandbox memory where `buf` should be written to. + /// `out_len_ptr` is an in-out location in sandbox memory. It is read to determine the + /// length of the buffer located at `out_ptr`. If that buffer is smaller than the actual + /// `buf.len()`, only what fits into that buffer is written to `out_ptr`. + /// The actual amount of bytes copied to `out_ptr` is written to `out_len_ptr`. + /// + /// If `out_ptr` is set to the sentinel value of `SENTINEL` and `allow_skip` is true the + /// operation is skipped and `Ok` is returned. This is supposed to help callers to make copying + /// output optional. For example to skip copying back the output buffer of an `seal_call` + /// when the caller is not interested in the result. + /// + /// `create_token` can optionally instruct this function to charge the gas meter with the token + /// it returns. `create_token` receives the variable amount of bytes that are about to be copied + /// by this function. + /// + /// In addition to the error conditions of `Memory::write` this functions returns + /// `Err` if the size of the buffer located at `out_ptr` is too small to fit `buf`. + fn write_sandbox_output( + &mut self, + out_ptr: u32, + out_len_ptr: u32, + buf: &[u8], + allow_skip: bool, + ) -> Result<(), DispatchError> { + if allow_skip && out_ptr == SENTINEL { + return Ok(()); + } + + let len = self.read_u32(out_len_ptr)?; + let buf_len = len.min(buf.len() as u32); + + self.write(out_ptr, &buf[..buf_len as usize])?; + self.write(out_len_ptr, &buf_len.encode()) + } + + /// Same as `write_sandbox_output` but for static size output. + fn write_fixed_sandbox_output( + &mut self, + out_ptr: u32, + buf: &[u8], + allow_skip: bool, + ) -> Result<(), DispatchError> { + if allow_skip && out_ptr == SENTINEL { + return Ok(()); + } + + self.write(out_ptr, buf) + } } /// Allows syscalls access to the PolkaVM instance they are executing in. @@ -562,8 +614,9 @@ impl<'a, E: Ext, M: PolkaVmInstance> Runtime<'a, E, M> { log::error!(target: LOG_TARGET, "polkavm execution error: {error}"); Some(Err(Error::::ExecutionFailed.into())) }, - Ok(Finished) => - Some(Ok(ExecReturnValue { flags: ReturnFlags::empty(), data: Vec::new() })), + Ok(Finished) => { + Some(Ok(ExecReturnValue { flags: ReturnFlags::empty(), data: Vec::new() })) + }, Ok(Trap) => Some(Err(Error::::ContractTrapped.into())), Ok(Segfault(_)) => Some(Err(Error::::ExecutionFailed.into())), Ok(NotEnoughGas) => Some(Err(Error::::OutOfGas.into())), @@ -578,11 +631,12 @@ impl<'a, E: Ext, M: PolkaVmInstance> Runtime<'a, E, M> { instance.write_output(return_value); None }, - Err(TrapReason::Return(ReturnData { flags, data })) => + Err(TrapReason::Return(ReturnData { flags, data })) => { match ReturnFlags::from_bits(flags) { None => Some(Err(Error::::InvalidCallFlags.into())), Some(flags) => Some(Ok(ExecReturnValue { flags, data })), - }, + } + }, Err(TrapReason::Termination) => Some(Ok(Default::default())), Err(TrapReason::SupervisorError(error)) => Some(Err(error.into())), } @@ -591,58 +645,6 @@ impl<'a, E: Ext, M: PolkaVmInstance> Runtime<'a, E, M> { } } -/// Write the given buffer and its length to the designated locations in sandbox memory and -/// charge gas according to the token returned by `create_token`. -/// -/// `out_ptr` is the location in sandbox memory where `buf` should be written to. -/// `out_len_ptr` is an in-out location in sandbox memory. It is read to determine the -/// length of the buffer located at `out_ptr`. If that buffer is smaller than the actual -/// `buf.len()`, only what fits into that buffer is written to `out_ptr`. -/// The actual amount of bytes copied to `out_ptr` is written to `out_len_ptr`. -/// -/// If `out_ptr` is set to the sentinel value of `SENTINEL` and `allow_skip` is true the -/// operation is skipped and `Ok` is returned. This is supposed to help callers to make copying -/// output optional. For example to skip copying back the output buffer of an `seal_call` -/// when the caller is not interested in the result. -/// -/// `create_token` can optionally instruct this function to charge the gas meter with the token -/// it returns. `create_token` receives the variable amount of bytes that are about to be copied -/// by this function. -/// -/// In addition to the error conditions of `Memory::write` this functions returns -/// `Err` if the size of the buffer located at `out_ptr` is too small to fit `buf`. -pub fn write_sandbox_output>( - memory: &mut M, - out_ptr: u32, - out_len_ptr: u32, - buf: &[u8], - allow_skip: bool, -) -> Result<(), DispatchError> { - if allow_skip && out_ptr == SENTINEL { - return Ok(()); - } - - let len = memory.read_u32(out_len_ptr)?; - let buf_len = len.min(buf.len() as u32); - - memory.write(out_ptr, &buf[..buf_len as usize])?; - memory.write(out_len_ptr, &buf_len.encode()) -} - -/// Same as `write_sandbox_output` but for static size output. -pub fn write_fixed_sandbox_output>( - memory: &mut M, - out_ptr: u32, - buf: &[u8], - allow_skip: bool, -) -> Result<(), DispatchError> { - if allow_skip && out_ptr == SENTINEL { - return Ok(()); - } - - memory.write(out_ptr, buf) -} - impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { pub fn new(ext: &'a mut E, input_data: Vec) -> Self { Self { @@ -867,7 +869,7 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { }; if let Some(value) = outcome { self.adjust_gas(charged, costs(value.len() as u32)); - write_sandbox_output::(memory, out_ptr, out_len_ptr, &value, false)?; + memory.write_sandbox_output(out_ptr, out_len_ptr, &value, false)?; Ok(ReturnErrorCode::Success) } else { self.adjust_gas(charged, costs(0)); @@ -928,7 +930,7 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { if let crate::storage::WriteOutcome::Taken(value) = outcome { self.adjust_gas(charged, costs(value.len() as u32)); - write_sandbox_output::(memory, out_ptr, out_len_ptr, &value, false)?; + memory.write_sandbox_output(out_ptr, out_len_ptr, &value, false)?; Ok(ReturnErrorCode::Success) } else { self.adjust_gas(charged, costs(0)); @@ -1012,8 +1014,7 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { self.charge_gas(RuntimeCosts::CopyToContract( self.ext.last_frame_output().data.len() as u32, ))?; - write_sandbox_output::( - memory, + memory.write_sandbox_output( output_ptr, output_len_ptr, &self.ext.last_frame_output().data, @@ -1062,18 +1063,12 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { ) { Ok(address) => { if !self.ext.last_frame_output().flags.contains(ReturnFlags::REVERT) { - write_fixed_sandbox_output::( - memory, - address_ptr, - &address.as_bytes(), - true, - )?; + memory.write_fixed_sandbox_output(address_ptr, &address.as_bytes(), true)?; } self.charge_gas(RuntimeCosts::CopyToContract( self.ext.last_frame_output().data.len() as u32, ))?; - write_sandbox_output::( - memory, + memory.write_sandbox_output( output_ptr, output_len_ptr, &self.ext.last_frame_output().data, @@ -1314,7 +1309,7 @@ pub mod env { fn input(&mut self, memory: &mut M, out_ptr: u32, out_len_ptr: u32) -> Result<(), TrapReason> { if let Some(input) = self.input_data.take() { self.charge_gas(RuntimeCosts::CopyToContract(input.len() as u32))?; - write_sandbox_output::(memory, out_ptr, out_len_ptr, &input, false)?; + memory.write_sandbox_output(out_ptr, out_len_ptr, &input, false)?; self.input_data = Some(input); Ok(()) } else { @@ -1342,7 +1337,7 @@ pub mod env { fn caller(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { self.charge_gas(RuntimeCosts::Caller)?; let caller = ::AddressMapper::to_address(self.ext.caller().account_id()?); - Ok(write_fixed_sandbox_output::(memory, out_ptr, caller.as_bytes(), false)?) + Ok(memory.write_fixed_sandbox_output(out_ptr, caller.as_bytes(), false)?) } /// Checks whether a specified address belongs to a contract. @@ -1368,7 +1363,7 @@ pub mod env { let mut address = H160::zero(); memory.read_into_buf(addr_ptr, address.as_bytes_mut())?; if let Some(value) = self.ext.code_hash(&address) { - write_fixed_sandbox_output::(memory, out_ptr, &value.as_bytes(), false)?; + memory.write_fixed_sandbox_output(out_ptr, &value.as_bytes(), false)?; Ok(ReturnErrorCode::Success) } else { Ok(ReturnErrorCode::KeyNotFound) @@ -1381,7 +1376,7 @@ pub mod env { fn own_code_hash(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { self.charge_gas(RuntimeCosts::OwnCodeHash)?; let code_hash = *self.ext.own_code_hash(); - Ok(write_fixed_sandbox_output::(memory, out_ptr, code_hash.as_bytes(), false)?) + Ok(memory.write_fixed_sandbox_output(out_ptr, code_hash.as_bytes(), false)?) } /// Checks whether the caller of the current contract is the origin of the whole call stack. @@ -1406,7 +1401,7 @@ pub mod env { fn address(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { self.charge_gas(RuntimeCosts::Address)?; let address = self.ext.address(); - Ok(write_fixed_sandbox_output::(memory, out_ptr, address.as_bytes(), false)?) + Ok(memory.write_fixed_sandbox_output(out_ptr, address.as_bytes(), false)?) } /// Stores the price for the specified amount of weight into the supplied buffer. @@ -1421,8 +1416,7 @@ pub mod env { ) -> Result<(), TrapReason> { let weight = Weight::from_parts(ref_time_limit, proof_size_limit); self.charge_gas(RuntimeCosts::WeightToFee)?; - Ok(write_fixed_sandbox_output::( - memory, + Ok(memory.write_fixed_sandbox_output( out_ptr, &self.ext.get_weight_price(weight).encode(), false, @@ -1440,7 +1434,7 @@ pub mod env { ) -> Result<(), TrapReason> { self.charge_gas(RuntimeCosts::GasLeft)?; let gas_left = &self.ext.gas_meter().gas_left().encode(); - Ok(write_sandbox_output::(memory, out_ptr, out_len_ptr, gas_left, false)?) + Ok(memory.write_sandbox_output(out_ptr, out_len_ptr, gas_left, false)?) } /// Stores the *free* balance of the current account into the supplied buffer. @@ -1448,12 +1442,7 @@ pub mod env { #[api_version(0)] fn balance(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { self.charge_gas(RuntimeCosts::Balance)?; - Ok(write_fixed_sandbox_output::( - memory, - out_ptr, - &as_bytes(self.ext.balance()), - false, - )?) + Ok(memory.write_fixed_sandbox_output(out_ptr, &as_bytes(self.ext.balance()), false)?) } /// Stores the *free* balance of the supplied address into the supplied buffer. @@ -1468,8 +1457,7 @@ pub mod env { self.charge_gas(RuntimeCosts::BalanceOf)?; let mut address = H160::zero(); memory.read_into_buf(addr_ptr, address.as_bytes_mut())?; - Ok(write_fixed_sandbox_output::( - memory, + Ok(memory.write_fixed_sandbox_output( out_ptr, &as_bytes(self.ext.balance_of(&address)), false, @@ -1481,8 +1469,7 @@ pub mod env { #[api_version(0)] fn chain_id(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { self.charge_gas(RuntimeCosts::CopyToContract(32))?; - Ok(write_fixed_sandbox_output::( - memory, + Ok(memory.write_fixed_sandbox_output( out_ptr, &as_bytes(U256::from(::ChainId::get())), false, @@ -1494,8 +1481,7 @@ pub mod env { #[api_version(0)] fn value_transferred(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { self.charge_gas(RuntimeCosts::ValueTransferred)?; - Ok(write_fixed_sandbox_output::( - memory, + Ok(memory.write_fixed_sandbox_output( out_ptr, &as_bytes(self.ext.value_transferred()), false, @@ -1507,7 +1493,7 @@ pub mod env { #[api_version(0)] fn now(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { self.charge_gas(RuntimeCosts::Now)?; - Ok(write_fixed_sandbox_output::(memory, out_ptr, &as_bytes(self.ext.now()), false)?) + Ok(memory.write_fixed_sandbox_output(out_ptr, &as_bytes(self.ext.now()), false)?) } /// Stores the minimum balance (a.k.a. existential deposit) into the supplied buffer. @@ -1515,8 +1501,7 @@ pub mod env { #[api_version(0)] fn minimum_balance(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { self.charge_gas(RuntimeCosts::MinimumBalance)?; - Ok(write_fixed_sandbox_output::( - memory, + Ok(memory.write_fixed_sandbox_output( out_ptr, &as_bytes(self.ext.minimum_balance()), false, @@ -1568,12 +1553,7 @@ pub mod env { #[api_version(0)] fn block_number(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { self.charge_gas(RuntimeCosts::BlockNumber)?; - Ok(write_fixed_sandbox_output::( - memory, - out_ptr, - &as_bytes(self.ext.block_number()), - false, - )?) + Ok(memory.write_fixed_sandbox_output(out_ptr, &as_bytes(self.ext.block_number()), false)?) } /// Computes the SHA2 256-bit hash on the given input buffer. @@ -1663,8 +1643,9 @@ pub mod env { Environment::new(self, memory, id, input_ptr, input_len, output_ptr, output_len_ptr); let ret = match chain_extension.call(env)? { RetVal::Converging(val) => Ok(val), - RetVal::Diverging { flags, data } => - Err(TrapReason::Return(ReturnData { flags: flags.bits(), data })), + RetVal::Diverging { flags, data } => { + Err(TrapReason::Return(ReturnData { flags: flags.bits(), data })) + }, }; self.chain_extension = Some(chain_extension); ret @@ -1924,8 +1905,7 @@ pub mod env { #[api_version(0)] fn return_data_size(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { self.charge_gas(RuntimeCosts::CopyToContract(32))?; - Ok(write_fixed_sandbox_output::( - memory, + Ok(memory.write_fixed_sandbox_output( out_ptr, &as_bytes(U256::from(self.ext.last_frame_output().data.len())), false, @@ -1947,8 +1927,7 @@ pub mod env { return Err(Error::::OutOfBounds.into()); } self.charge_gas(RuntimeCosts::CopyToContract(len as u32))?; - Ok(write_sandbox_output::( - memory, + Ok(memory.write_sandbox_output( out_ptr, out_len_ptr, &self.ext.last_frame_output().data[offset as usize..], From 8ea20a118910ede33068c832352d1215e4bdda48 Mon Sep 17 00:00:00 2001 From: xermicus Date: Tue, 24 Sep 2024 18:09:33 +0200 Subject: [PATCH 20/25] write actual buf len Signed-off-by: xermicus --- substrate/frame/revive/src/wasm/runtime.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/substrate/frame/revive/src/wasm/runtime.rs b/substrate/frame/revive/src/wasm/runtime.rs index a05148e0edbf..23881d4cd2c4 100644 --- a/substrate/frame/revive/src/wasm/runtime.rs +++ b/substrate/frame/revive/src/wasm/runtime.rs @@ -1904,12 +1904,9 @@ pub mod env { /// See [`pallet_revive_uapi::HostFn::return_data_size`]. #[api_version(0)] fn return_data_size(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { - self.charge_gas(RuntimeCosts::CopyToContract(32))?; - Ok(memory.write_fixed_sandbox_output( - out_ptr, - &as_bytes(U256::from(self.ext.last_frame_output().data.len())), - false, - )?) + let bytes = as_bytes(U256::from(self.ext.last_frame_output().data.len())); + self.charge_gas(RuntimeCosts::CopyToContract(bytes.len() as u32))?; + Ok(memory.write_fixed_sandbox_output(out_ptr, &bytes, false)?) } /// Stores data returned by the last call, starting from `offset`, into the supplied buffer. From a7a9c631bcccc5624256cafe6f5b45a386a572ed Mon Sep 17 00:00:00 2001 From: xermicus Date: Wed, 25 Sep 2024 10:01:19 +0200 Subject: [PATCH 21/25] Revert "work around borrow checker limitations" This reverts commit b6c37057324099fd98a1e347d35155d5cbb1d9b4. --- substrate/frame/revive/src/chain_extension.rs | 16 +- substrate/frame/revive/src/wasm/mod.rs | 2 +- substrate/frame/revive/src/wasm/runtime.rs | 253 ++++++++++++++---- 3 files changed, 219 insertions(+), 52 deletions(-) diff --git a/substrate/frame/revive/src/chain_extension.rs b/substrate/frame/revive/src/chain_extension.rs index 6135070708bb..ccea12945054 100644 --- a/substrate/frame/revive/src/chain_extension.rs +++ b/substrate/frame/revive/src/chain_extension.rs @@ -344,11 +344,15 @@ impl<'a, 'b, E: Ext, M: ?Sized + Memory> Environment<'a, 'b, E, M> { allow_skip: bool, weight_per_byte: Option, ) -> Result<()> { - if let Some(weight_per_byte) = weight_per_byte { - let weight = weight_per_byte.saturating_mul(buffer.len() as u64); - self.runtime.charge_gas(RuntimeCosts::ChainExtension(weight))?; - } - self.memory - .write_sandbox_output(self.output_ptr, self.output_len_ptr, buffer, allow_skip) + self.runtime.write_sandbox_output( + self.memory, + self.output_ptr, + self.output_len_ptr, + buffer, + allow_skip, + |len| { + weight_per_byte.map(|w| RuntimeCosts::ChainExtension(w.saturating_mul(len.into()))) + }, + ) } } diff --git a/substrate/frame/revive/src/wasm/mod.rs b/substrate/frame/revive/src/wasm/mod.rs index 383086800aa1..cd274873975d 100644 --- a/substrate/frame/revive/src/wasm/mod.rs +++ b/substrate/frame/revive/src/wasm/mod.rs @@ -268,7 +268,7 @@ where &mut self.instance, self.api_version, ) { - break exec_result; + break exec_result } }; let _ = self.runtime.ext().gas_meter_mut().sync_from_executor(self.instance.gas())?; diff --git a/substrate/frame/revive/src/wasm/runtime.rs b/substrate/frame/revive/src/wasm/runtime.rs index 23881d4cd2c4..d5d5247e5c6e 100644 --- a/substrate/frame/revive/src/wasm/runtime.rs +++ b/substrate/frame/revive/src/wasm/runtime.rs @@ -590,6 +590,13 @@ impl CallType { } } +/// This is only appropriate when writing out data of constant size that does not depend on user +/// input. In this case the costs for this copy was already charged as part of the token at +/// the beginning of the API entry point. +fn already_charged(_: u32) -> Option { + None +} + /// Can only be used for one call. pub struct Runtime<'a, E: Ext, M: ?Sized> { ext: &'a mut E, @@ -702,6 +709,71 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { } } + /// Write the given buffer and its length to the designated locations in sandbox memory and + /// charge gas according to the token returned by `create_token`. + /// + /// `out_ptr` is the location in sandbox memory where `buf` should be written to. + /// `out_len_ptr` is an in-out location in sandbox memory. It is read to determine the + /// length of the buffer located at `out_ptr`. If that buffer is smaller than the actual + /// `buf.len()`, only what fits into that buffer is written to `out_ptr`. + /// The actual amount of bytes copied to `out_ptr` is written to `out_len_ptr`. + /// + /// If `out_ptr` is set to the sentinel value of `SENTINEL` and `allow_skip` is true the + /// operation is skipped and `Ok` is returned. This is supposed to help callers to make copying + /// output optional. For example to skip copying back the output buffer of an `seal_call` + /// when the caller is not interested in the result. + /// + /// `create_token` can optionally instruct this function to charge the gas meter with the token + /// it returns. `create_token` receives the variable amount of bytes that are about to be copied + /// by this function. + /// + /// In addition to the error conditions of `Memory::write` this functions returns + /// `Err` if the size of the buffer located at `out_ptr` is too small to fit `buf`. + pub fn write_sandbox_output( + &mut self, + memory: &mut M, + out_ptr: u32, + out_len_ptr: u32, + buf: &[u8], + allow_skip: bool, + create_token: impl FnOnce(u32) -> Option, + ) -> Result<(), DispatchError> { + if allow_skip && out_ptr == SENTINEL { + return Ok(()); + } + + let len = memory.read_u32(out_len_ptr)?; + let buf_len = len.min(buf.len() as u32); + + if let Some(costs) = create_token(buf_len) { + self.charge_gas(costs)?; + } + + memory.write(out_ptr, &buf[..buf_len as usize])?; + memory.write(out_len_ptr, &buf_len.encode()) + } + + /// Same as `write_sandbox_output` but for static size output. + pub fn write_fixed_sandbox_output( + &mut self, + memory: &mut M, + out_ptr: u32, + buf: &[u8], + allow_skip: bool, + create_token: impl FnOnce(u32) -> Option, + ) -> Result<(), DispatchError> { + if allow_skip && out_ptr == SENTINEL { + return Ok(()); + } + + let buf_len = buf.len() as u32; + if let Some(costs) = create_token(buf_len) { + self.charge_gas(costs)?; + } + + memory.write(out_ptr, buf) + } + /// Computes the given hash function on the supplied input. /// /// Reads from the sandboxed input buffer into an intermediate buffer. @@ -869,7 +941,14 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { }; if let Some(value) = outcome { self.adjust_gas(charged, costs(value.len() as u32)); - memory.write_sandbox_output(out_ptr, out_len_ptr, &value, false)?; + self.write_sandbox_output( + memory, + out_ptr, + out_len_ptr, + &value, + false, + already_charged, + )?; Ok(ReturnErrorCode::Success) } else { self.adjust_gas(charged, costs(0)); @@ -930,7 +1009,14 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { if let crate::storage::WriteOutcome::Taken(value) = outcome { self.adjust_gas(charged, costs(value.len() as u32)); - memory.write_sandbox_output(out_ptr, out_len_ptr, &value, false)?; + self.write_sandbox_output( + memory, + out_ptr, + out_len_ptr, + &value, + false, + already_charged, + )?; Ok(ReturnErrorCode::Success) } else { self.adjust_gas(charged, costs(0)); @@ -1011,15 +1097,17 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { })); }, Ok(_) => { - self.charge_gas(RuntimeCosts::CopyToContract( - self.ext.last_frame_output().data.len() as u32, - ))?; - memory.write_sandbox_output( + let output = mem::take(self.ext.last_frame_output_mut()); + let write_result = self.write_sandbox_output( + memory, output_ptr, output_len_ptr, - &self.ext.last_frame_output().data, + &output.data, true, - )?; + |len| Some(RuntimeCosts::CopyToContract(len)), + ); + *self.ext.last_frame_output_mut() = output; + write_result?; Ok(self.ext.last_frame_output().into()) }, Err(err) => Ok(Self::exec_error_into_return_code(err)?), @@ -1063,17 +1151,25 @@ impl<'a, E: Ext, M: ?Sized + Memory> Runtime<'a, E, M> { ) { Ok(address) => { if !self.ext.last_frame_output().flags.contains(ReturnFlags::REVERT) { - memory.write_fixed_sandbox_output(address_ptr, &address.as_bytes(), true)?; + self.write_fixed_sandbox_output( + memory, + address_ptr, + &address.as_bytes(), + true, + already_charged, + )?; } - self.charge_gas(RuntimeCosts::CopyToContract( - self.ext.last_frame_output().data.len() as u32, - ))?; - memory.write_sandbox_output( + let output = mem::take(self.ext.last_frame_output_mut()); + let write_result = self.write_sandbox_output( + memory, output_ptr, output_len_ptr, - &self.ext.last_frame_output().data, + &output.data, true, - )?; + |len| Some(RuntimeCosts::CopyToContract(len)), + ); + *self.ext.last_frame_output_mut() = output; + write_result?; Ok(self.ext.last_frame_output().into()) }, Err(err) => Ok(Self::exec_error_into_return_code(err)?), @@ -1308,8 +1404,9 @@ pub mod env { #[api_version(0)] fn input(&mut self, memory: &mut M, out_ptr: u32, out_len_ptr: u32) -> Result<(), TrapReason> { if let Some(input) = self.input_data.take() { - self.charge_gas(RuntimeCosts::CopyToContract(input.len() as u32))?; - memory.write_sandbox_output(out_ptr, out_len_ptr, &input, false)?; + self.write_sandbox_output(memory, out_ptr, out_len_ptr, &input, false, |len| { + Some(RuntimeCosts::CopyToContract(len)) + })?; self.input_data = Some(input); Ok(()) } else { @@ -1337,7 +1434,13 @@ pub mod env { fn caller(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { self.charge_gas(RuntimeCosts::Caller)?; let caller = ::AddressMapper::to_address(self.ext.caller().account_id()?); - Ok(memory.write_fixed_sandbox_output(out_ptr, caller.as_bytes(), false)?) + Ok(self.write_fixed_sandbox_output( + memory, + out_ptr, + caller.as_bytes(), + false, + already_charged, + )?) } /// Checks whether a specified address belongs to a contract. @@ -1363,7 +1466,13 @@ pub mod env { let mut address = H160::zero(); memory.read_into_buf(addr_ptr, address.as_bytes_mut())?; if let Some(value) = self.ext.code_hash(&address) { - memory.write_fixed_sandbox_output(out_ptr, &value.as_bytes(), false)?; + self.write_fixed_sandbox_output( + memory, + out_ptr, + &value.as_bytes(), + false, + already_charged, + )?; Ok(ReturnErrorCode::Success) } else { Ok(ReturnErrorCode::KeyNotFound) @@ -1376,7 +1485,13 @@ pub mod env { fn own_code_hash(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { self.charge_gas(RuntimeCosts::OwnCodeHash)?; let code_hash = *self.ext.own_code_hash(); - Ok(memory.write_fixed_sandbox_output(out_ptr, code_hash.as_bytes(), false)?) + Ok(self.write_fixed_sandbox_output( + memory, + out_ptr, + code_hash.as_bytes(), + false, + already_charged, + )?) } /// Checks whether the caller of the current contract is the origin of the whole call stack. @@ -1401,7 +1516,13 @@ pub mod env { fn address(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { self.charge_gas(RuntimeCosts::Address)?; let address = self.ext.address(); - Ok(memory.write_fixed_sandbox_output(out_ptr, address.as_bytes(), false)?) + Ok(self.write_fixed_sandbox_output( + memory, + out_ptr, + address.as_bytes(), + false, + already_charged, + )?) } /// Stores the price for the specified amount of weight into the supplied buffer. @@ -1416,10 +1537,12 @@ pub mod env { ) -> Result<(), TrapReason> { let weight = Weight::from_parts(ref_time_limit, proof_size_limit); self.charge_gas(RuntimeCosts::WeightToFee)?; - Ok(memory.write_fixed_sandbox_output( + Ok(self.write_fixed_sandbox_output( + memory, out_ptr, &self.ext.get_weight_price(weight).encode(), false, + already_charged, )?) } @@ -1434,7 +1557,14 @@ pub mod env { ) -> Result<(), TrapReason> { self.charge_gas(RuntimeCosts::GasLeft)?; let gas_left = &self.ext.gas_meter().gas_left().encode(); - Ok(memory.write_sandbox_output(out_ptr, out_len_ptr, gas_left, false)?) + Ok(self.write_sandbox_output( + memory, + out_ptr, + out_len_ptr, + gas_left, + false, + already_charged, + )?) } /// Stores the *free* balance of the current account into the supplied buffer. @@ -1442,7 +1572,13 @@ pub mod env { #[api_version(0)] fn balance(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { self.charge_gas(RuntimeCosts::Balance)?; - Ok(memory.write_fixed_sandbox_output(out_ptr, &as_bytes(self.ext.balance()), false)?) + Ok(self.write_fixed_sandbox_output( + memory, + out_ptr, + &as_bytes(self.ext.balance()), + false, + already_charged, + )?) } /// Stores the *free* balance of the supplied address into the supplied buffer. @@ -1457,10 +1593,12 @@ pub mod env { self.charge_gas(RuntimeCosts::BalanceOf)?; let mut address = H160::zero(); memory.read_into_buf(addr_ptr, address.as_bytes_mut())?; - Ok(memory.write_fixed_sandbox_output( + Ok(self.write_fixed_sandbox_output( + memory, out_ptr, &as_bytes(self.ext.balance_of(&address)), false, + already_charged, )?) } @@ -1468,11 +1606,12 @@ pub mod env { /// See [`pallet_revive_uapi::HostFn::chain_id`]. #[api_version(0)] fn chain_id(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { - self.charge_gas(RuntimeCosts::CopyToContract(32))?; - Ok(memory.write_fixed_sandbox_output( + Ok(self.write_fixed_sandbox_output( + memory, out_ptr, &as_bytes(U256::from(::ChainId::get())), false, + |_| Some(RuntimeCosts::CopyToContract(32)), )?) } @@ -1481,10 +1620,12 @@ pub mod env { #[api_version(0)] fn value_transferred(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { self.charge_gas(RuntimeCosts::ValueTransferred)?; - Ok(memory.write_fixed_sandbox_output( + Ok(self.write_fixed_sandbox_output( + memory, out_ptr, &as_bytes(self.ext.value_transferred()), false, + already_charged, )?) } @@ -1493,7 +1634,13 @@ pub mod env { #[api_version(0)] fn now(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { self.charge_gas(RuntimeCosts::Now)?; - Ok(memory.write_fixed_sandbox_output(out_ptr, &as_bytes(self.ext.now()), false)?) + Ok(self.write_fixed_sandbox_output( + memory, + out_ptr, + &as_bytes(self.ext.now()), + false, + already_charged, + )?) } /// Stores the minimum balance (a.k.a. existential deposit) into the supplied buffer. @@ -1501,10 +1648,12 @@ pub mod env { #[api_version(0)] fn minimum_balance(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { self.charge_gas(RuntimeCosts::MinimumBalance)?; - Ok(memory.write_fixed_sandbox_output( + Ok(self.write_fixed_sandbox_output( + memory, out_ptr, &as_bytes(self.ext.minimum_balance()), false, + already_charged, )?) } @@ -1553,7 +1702,13 @@ pub mod env { #[api_version(0)] fn block_number(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { self.charge_gas(RuntimeCosts::BlockNumber)?; - Ok(memory.write_fixed_sandbox_output(out_ptr, &as_bytes(self.ext.block_number()), false)?) + Ok(self.write_fixed_sandbox_output( + memory, + out_ptr, + &as_bytes(self.ext.block_number()), + false, + already_charged, + )?) } /// Computes the SHA2 256-bit hash on the given input buffer. @@ -1904,9 +2059,13 @@ pub mod env { /// See [`pallet_revive_uapi::HostFn::return_data_size`]. #[api_version(0)] fn return_data_size(&mut self, memory: &mut M, out_ptr: u32) -> Result<(), TrapReason> { - let bytes = as_bytes(U256::from(self.ext.last_frame_output().data.len())); - self.charge_gas(RuntimeCosts::CopyToContract(bytes.len() as u32))?; - Ok(memory.write_fixed_sandbox_output(out_ptr, &bytes, false)?) + Ok(self.write_fixed_sandbox_output( + memory, + out_ptr, + &as_bytes(U256::from(self.ext.last_frame_output().data.len())), + false, + |len| Some(RuntimeCosts::CopyToContract(len)), + )?) } /// Stores data returned by the last call, starting from `offset`, into the supplied buffer. @@ -1919,16 +2078,20 @@ pub mod env { out_len_ptr: u32, offset: u32, ) -> Result<(), TrapReason> { - let len = self.ext.last_frame_output().data.len(); - if offset as usize > len { - return Err(Error::::OutOfBounds.into()); - } - self.charge_gas(RuntimeCosts::CopyToContract(len as u32))?; - Ok(memory.write_sandbox_output( - out_ptr, - out_len_ptr, - &self.ext.last_frame_output().data[offset as usize..], - false, - )?) + let output = mem::take(self.ext.last_frame_output_mut()); + let result = if offset as usize > output.data.len() { + Err(Error::::OutOfBounds.into()) + } else { + self.write_sandbox_output( + memory, + out_ptr, + out_len_ptr, + &output.data[offset as usize..], + false, + |len| Some(RuntimeCosts::CopyToContract(len)), + ) + }; + *self.ext.last_frame_output_mut() = output; + Ok(result?) } } From 1e3cddf0f762528d8948026e8e70dc9fac85122c Mon Sep 17 00:00:00 2001 From: xermicus Date: Wed, 25 Sep 2024 10:25:32 +0200 Subject: [PATCH 22/25] rm stale methods Signed-off-by: xermicus --- substrate/frame/revive/src/wasm/runtime.rs | 52 ---------------------- 1 file changed, 52 deletions(-) diff --git a/substrate/frame/revive/src/wasm/runtime.rs b/substrate/frame/revive/src/wasm/runtime.rs index d5d5247e5c6e..74cb2c34f0f4 100644 --- a/substrate/frame/revive/src/wasm/runtime.rs +++ b/substrate/frame/revive/src/wasm/runtime.rs @@ -154,58 +154,6 @@ pub trait Memory { .map_err(|_| DispatchError::from(Error::::DecodingFailed))?; Ok(decoded) } - - /// Write the given buffer and its length to the designated locations in sandbox memory and - /// charge gas according to the token returned by `create_token`. - /// - /// `out_ptr` is the location in sandbox memory where `buf` should be written to. - /// `out_len_ptr` is an in-out location in sandbox memory. It is read to determine the - /// length of the buffer located at `out_ptr`. If that buffer is smaller than the actual - /// `buf.len()`, only what fits into that buffer is written to `out_ptr`. - /// The actual amount of bytes copied to `out_ptr` is written to `out_len_ptr`. - /// - /// If `out_ptr` is set to the sentinel value of `SENTINEL` and `allow_skip` is true the - /// operation is skipped and `Ok` is returned. This is supposed to help callers to make copying - /// output optional. For example to skip copying back the output buffer of an `seal_call` - /// when the caller is not interested in the result. - /// - /// `create_token` can optionally instruct this function to charge the gas meter with the token - /// it returns. `create_token` receives the variable amount of bytes that are about to be copied - /// by this function. - /// - /// In addition to the error conditions of `Memory::write` this functions returns - /// `Err` if the size of the buffer located at `out_ptr` is too small to fit `buf`. - fn write_sandbox_output( - &mut self, - out_ptr: u32, - out_len_ptr: u32, - buf: &[u8], - allow_skip: bool, - ) -> Result<(), DispatchError> { - if allow_skip && out_ptr == SENTINEL { - return Ok(()); - } - - let len = self.read_u32(out_len_ptr)?; - let buf_len = len.min(buf.len() as u32); - - self.write(out_ptr, &buf[..buf_len as usize])?; - self.write(out_len_ptr, &buf_len.encode()) - } - - /// Same as `write_sandbox_output` but for static size output. - fn write_fixed_sandbox_output( - &mut self, - out_ptr: u32, - buf: &[u8], - allow_skip: bool, - ) -> Result<(), DispatchError> { - if allow_skip && out_ptr == SENTINEL { - return Ok(()); - } - - self.write(out_ptr, buf) - } } /// Allows syscalls access to the PolkaVM instance they are executing in. From 06305fe45b0b43ca15053734bce67b0d1ba7fc98 Mon Sep 17 00:00:00 2001 From: command-bot <> Date: Wed, 25 Sep 2024 08:28:20 +0000 Subject: [PATCH 23/25] ".git/.scripts/commands/fmt/fmt.sh" --- substrate/frame/revive/src/exec.rs | 5 ++--- substrate/frame/revive/src/wasm/runtime.rs | 15 ++++++--------- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index 56e4ebdf225b..61256c03dd06 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -1107,9 +1107,8 @@ where with_transaction(|| -> TransactionOutcome> { let output = do_transaction(); match &output { - Ok(result) if !result.did_revert() => { - TransactionOutcome::Commit(Ok((true, output))) - }, + Ok(result) if !result.did_revert() => + TransactionOutcome::Commit(Ok((true, output))), _ => TransactionOutcome::Rollback(Ok((false, output))), } }); diff --git a/substrate/frame/revive/src/wasm/runtime.rs b/substrate/frame/revive/src/wasm/runtime.rs index 74cb2c34f0f4..672056ea14e3 100644 --- a/substrate/frame/revive/src/wasm/runtime.rs +++ b/substrate/frame/revive/src/wasm/runtime.rs @@ -569,9 +569,8 @@ impl<'a, E: Ext, M: PolkaVmInstance> Runtime<'a, E, M> { log::error!(target: LOG_TARGET, "polkavm execution error: {error}"); Some(Err(Error::::ExecutionFailed.into())) }, - Ok(Finished) => { - Some(Ok(ExecReturnValue { flags: ReturnFlags::empty(), data: Vec::new() })) - }, + Ok(Finished) => + Some(Ok(ExecReturnValue { flags: ReturnFlags::empty(), data: Vec::new() })), Ok(Trap) => Some(Err(Error::::ContractTrapped.into())), Ok(Segfault(_)) => Some(Err(Error::::ExecutionFailed.into())), Ok(NotEnoughGas) => Some(Err(Error::::OutOfGas.into())), @@ -586,12 +585,11 @@ impl<'a, E: Ext, M: PolkaVmInstance> Runtime<'a, E, M> { instance.write_output(return_value); None }, - Err(TrapReason::Return(ReturnData { flags, data })) => { + Err(TrapReason::Return(ReturnData { flags, data })) => match ReturnFlags::from_bits(flags) { None => Some(Err(Error::::InvalidCallFlags.into())), Some(flags) => Some(Ok(ExecReturnValue { flags, data })), - } - }, + }, Err(TrapReason::Termination) => Some(Ok(Default::default())), Err(TrapReason::SupervisorError(error)) => Some(Err(error.into())), } @@ -1746,9 +1744,8 @@ pub mod env { Environment::new(self, memory, id, input_ptr, input_len, output_ptr, output_len_ptr); let ret = match chain_extension.call(env)? { RetVal::Converging(val) => Ok(val), - RetVal::Diverging { flags, data } => { - Err(TrapReason::Return(ReturnData { flags: flags.bits(), data })) - }, + RetVal::Diverging { flags, data } => + Err(TrapReason::Return(ReturnData { flags: flags.bits(), data })), }; self.chain_extension = Some(chain_extension); ret From 456697cbb8d73ddcc3e912f874835fa501af9ed4 Mon Sep 17 00:00:00 2001 From: Cyrill Leutwiler Date: Wed, 25 Sep 2024 13:17:21 +0200 Subject: [PATCH 24/25] Update substrate/frame/revive/src/wasm/runtime.rs Co-authored-by: PG Herveou --- substrate/frame/revive/src/wasm/runtime.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/frame/revive/src/wasm/runtime.rs b/substrate/frame/revive/src/wasm/runtime.rs index 672056ea14e3..4b5a9a04eb73 100644 --- a/substrate/frame/revive/src/wasm/runtime.rs +++ b/substrate/frame/revive/src/wasm/runtime.rs @@ -237,8 +237,8 @@ parameter_types! { const XcmExecutionFailed: ReturnErrorCode = ReturnErrorCode::XcmExecutionFailed; } -impl<'a> From<&'a ExecReturnValue> for ReturnErrorCode { - fn from(from: &'a ExecReturnValue) -> Self { +impl From<&ExecReturnValue> for ReturnErrorCode { + fn from(from: &ExecReturnValue) -> Self { if from.flags.contains(ReturnFlags::REVERT) { Self::CalleeReverted } else { From 61f1f1e5568ed72cced4d5f4c354a42e17e71b83 Mon Sep 17 00:00:00 2001 From: xermicus Date: Wed, 25 Sep 2024 13:45:00 +0200 Subject: [PATCH 25/25] fix return types Signed-off-by: xermicus --- substrate/frame/revive/src/exec.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index 61256c03dd06..2e48bab29255 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -737,7 +737,7 @@ where )? { stack.run(executable, input_data).map(|_| stack.first_frame.last_frame_output) } else { - Self::transfer_no_contract(&origin, &dest, value).map(|_| Default::default()) + Self::transfer_no_contract(&origin, &dest, value) } } @@ -1243,8 +1243,10 @@ where from: &Origin, to: &T::AccountId, value: BalanceOf, - ) -> Result<(), ExecError> { - Self::transfer_from_origin(from, to, value).map_err(Into::into) + ) -> ExecResult { + Self::transfer_from_origin(from, to, value) + .map(|_| ExecReturnValue::default()) + .map_err(Into::into) } /// Reference to the current (top) frame. @@ -1342,7 +1344,8 @@ where &Origin::from_account_id(self.account_id().clone()), &dest, value, - ) + )?; + Ok(()) } };