From c77095f51119d2eccdc54d2f3518bed0ffbd6d53 Mon Sep 17 00:00:00 2001
From: Cyrill Leutwiler <cyrill@parity.io>
Date: Wed, 25 Sep 2024 18:28:59 +0200
Subject: [PATCH] [pallet-revive] last call return data API (#5779)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This PR introduces 2 new syscalls: `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 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.
---------
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: xermicus <cyrill@parity.io>
Co-authored-by: command-bot <>
Co-authored-by: PG Herveou <pgherveou@gmail.com>
---
prdoc/pr_5779.prdoc | 38 ++
.../fixtures/contracts/return_data_api.rs | 166 +++++++++
substrate/frame/revive/src/exec.rs | 348 +++++++++++++-----
substrate/frame/revive/src/tests.rs | 29 ++
substrate/frame/revive/src/wasm/runtime.rs | 144 +++++---
substrate/frame/revive/uapi/src/host.rs | 14 +
.../frame/revive/uapi/src/host/riscv32.rs | 14 +
7 files changed, 619 insertions(+), 134 deletions(-)
create mode 100644 prdoc/pr_5779.prdoc
create mode 100644 substrate/frame/revive/fixtures/contracts/return_data_api.rs
diff --git a/prdoc/pr_5779.prdoc b/prdoc/pr_5779.prdoc
new file mode 100644
index 00000000000..659a3a19f69
--- /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
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 00000000000..846396b0944
--- /dev/null
+++ b/substrate/frame/revive/fixtures/contracts/return_data_api.rs
@@ -0,0 +1,166 @@
+// 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 `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]
+
+use common::{input, u256_bytes};
+use uapi::{HostFn, HostFnImpl as api};
+
+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();
+}
+
+/// 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);
+
+ 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() {
+ api::call(
+ uapi::CallFlags::ALLOW_REENTRY,
+ &recursion_guard(),
+ 0u64,
+ 0u64,
+ None,
+ &[0u8; 32],
+ &[0u8; 32],
+ None,
+ )
+ .unwrap_err();
+ 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));
+}
+
+/// 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);
+}
+
+#[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],);
+
+ // 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;
+ 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)[..]);
+}
diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs
index 233658696c8..2e48bab2925 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<u8>,
allows_reentry: bool,
read_only: bool,
- ) -> Result<ExecReturnValue, ExecError>;
+ ) -> 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<u8>,
- ) -> Result<ExecReturnValue, ExecError>;
+ fn delegate_call(&mut self, code: H256, input_data: Vec<u8>) -> Result<(), ExecError>;
/// Instantiate a contract from the given code.
///
@@ -213,7 +209,7 @@ pub trait Ext: sealing::Sealed {
value: U256,
input_data: Vec<u8>,
salt: Option<&[u8; 32]>,
- ) -> Result<(H160, ExecReturnValue), ExecError>;
+ ) -> Result<H160, ExecError>;
/// Transfer all funds to `beneficiary` and delete the contract.
///
@@ -427,6 +423,12 @@ pub trait Ext: sealing::Sealed {
/// Check if running in read-only context.
fn is_read_only(&self) -> bool;
+
+ /// Returns an immutable reference to the output of the last executed call frame.
+ fn last_frame_output(&self) -> &ExecReturnValue;
+
+ /// Returns a mutable reference to the output of the last executed call frame.
+ fn last_frame_output_mut(&mut self) -> &mut ExecReturnValue;
}
/// Describes the different functions that can be exported by an [`Executable`].
@@ -547,6 +549,8 @@ struct Frame<T: Config> {
read_only: bool,
/// The caller of the currently executing frame which was spawned by `delegate_call`.
delegate_caller: Option<Origin<T>>,
+ /// The output of the last executed call frame.
+ last_frame_output: ExecReturnValue,
}
/// Used in a delegate call frame arguments in order to override the executable and caller.
@@ -731,7 +735,7 @@ 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)
}
@@ -772,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"))]
@@ -865,7 +871,7 @@ where
{
contract
} else {
- return Ok(None)
+ return Ok(None);
}
};
@@ -911,6 +917,7 @@ where
nested_storage: storage_meter.nested(deposit_limit),
allows_reentry: true,
read_only,
+ last_frame_output: Default::default(),
};
Ok(Some((frame, executable)))
@@ -965,12 +972,24 @@ where
/// Run the current (top) frame.
///
/// This can be either a call or an instantiate.
- fn run(&mut self, executable: E, input_data: Vec<u8>) -> ExecResult {
+ fn run(&mut self, executable: E, input_data: Vec<u8>) -> 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 };
+ // 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 = || {
@@ -1109,7 +1128,9 @@ where
}
self.pop_frame(success);
- output
+ output.map(|output| {
+ self.top_frame_mut().last_frame_output = output;
+ })
}
/// Remove the current (top) frame from the stack.
@@ -1285,7 +1306,7 @@ where
input_data: Vec<u8>,
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.
@@ -1323,7 +1344,8 @@ where
&Origin::from_account_id(self.account_id().clone()),
&dest,
value,
- )
+ )?;
+ Ok(())
}
};
@@ -1336,11 +1358,7 @@ where
result
}
- fn delegate_call(
- &mut self,
- code_hash: H256,
- input_data: Vec<u8>,
- ) -> Result<ExecReturnValue, ExecError> {
+ fn delegate_call(&mut self, code_hash: H256, input_data: Vec<u8>) -> 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();
@@ -1368,7 +1386,7 @@ where
value: U256,
input_data: Vec<u8>,
salt: Option<&[u8; 32]>,
- ) -> Result<(H160, ExecReturnValue), ExecError> {
+ ) -> Result<H160, ExecError> {
let executable = E::from_storage(code_hash, self.gas_meter_mut())?;
let sender = &self.top_frame().account_id;
let executable = self.push_frame(
@@ -1385,7 +1403,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 {
@@ -1690,6 +1708,14 @@ where
fn is_read_only(&self) -> bool {
self.top_frame().read_only
}
+
+ fn last_frame_output(&self) -> &ExecReturnValue {
+ &self.top_frame().last_frame_output
+ }
+
+ fn last_frame_output_mut(&mut self) -> &mut ExecReturnValue {
+ &mut self.top_frame_mut().last_frame_output
+ }
}
mod sealing {
@@ -2353,15 +2379,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(|| {
@@ -2447,15 +2475,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(|| {
@@ -2666,6 +2696,7 @@ mod tests {
vec![],
Some(&[48; 32]),
)
+ .map(|address| (address, ctx.ext.last_frame_output().clone()))
.unwrap();
*instantiated_contract_address.borrow_mut() = Some(address);
@@ -2841,15 +2872,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);
@@ -3095,6 +3128,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());
@@ -3137,15 +3171,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()
}
@@ -3153,15 +3189,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(|| {
@@ -3360,7 +3390,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(),
@@ -3916,15 +3946,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]));
@@ -4020,15 +4052,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]));
@@ -4102,4 +4136,148 @@ 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 = <Test as Config>::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::<Test>::new(GAS_LIMIT),
+ &mut storage_meter,
+ 0,
+ vec![],
+ None,
+ )
+ .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::<Test>::new(GAS_LIMIT),
+ &mut storage_meter,
+ 0,
+ vec![0],
+ None,
+ );
+ assert_matches!(result, Ok(_));
+ });
+ }
}
diff --git a/substrate/frame/revive/src/tests.rs b/substrate/frame/revive/src/tests.rs
index d06cdcfd465..5c5d144f24a 100644
--- a/substrate/frame/revive/src/tests.rs
+++ b/substrate/frame/revive/src/tests.rs
@@ -4325,4 +4325,33 @@ mod run_tests {
assert_eq!(received.result.data, chain_id.encode());
});
}
+
+ #[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 _ = <Test as Config>::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::<Test>(),
+ ));
+
+ // 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());
+ });
+ }
}
diff --git a/substrate/frame/revive/src/wasm/runtime.rs b/substrate/frame/revive/src/wasm/runtime.rs
index ebc407adacd..4b5a9a04eb7 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::{fmt, marker::PhantomData, mem};
use frame_support::{
dispatch::DispatchInfo, ensure, pallet_prelude::DispatchResultWithPostInfo, parameter_types,
traits::Get, weights::Weight,
@@ -237,8 +237,8 @@ parameter_types! {
const XcmExecutionFailed: ReturnErrorCode = ReturnErrorCode::XcmExecutionFailed;
}
-impl From<ExecReturnValue> for ReturnErrorCode {
- fn from(from: ExecReturnValue) -> Self {
+impl From<&ExecReturnValue> for ReturnErrorCode {
+ fn from(from: &ExecReturnValue) -> Self {
if from.flags.contains(ReturnFlags::REVERT) {
Self::CalleeReverted
} else {
@@ -769,20 +769,16 @@ impl<'a, E: Ext, M: ?Sized + Memory<E::T>> Runtime<'a, E, M> {
}
}
- /// Fallible conversion of a `ExecResult` to `ReturnErrorCode`.
- fn exec_into_return_code(from: ExecResult) -> Result<ReturnErrorCode, DispatchError> {
+ /// Fallible conversion of a `ExecError` to `ReturnErrorCode`.
+ fn exec_error_into_return_code(from: ExecError) -> Result<ReturnErrorCode, DispatchError> {
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<Key, TrapReason> {
let res = match key_len {
SENTINEL => {
@@ -1036,28 +1032,32 @@ impl<'a, E: Ext, M: ?Sized + Memory<E::T>> 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 {
+ 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: return_value.flags.bits(),
- data: return_value.data,
+ flags: output.flags.bits(),
+ data: output.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)),
- )?;
+ },
+ Ok(_) => {
+ 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)?),
}
- Ok(Self::exec_into_return_code(call_outcome)?)
}
fn instantiate(
@@ -1086,34 +1086,40 @@ impl<'a, E: Ext, M: ?Sized + Memory<E::T>> 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) => {
+ if !self.ext.last_frame_output().flags.contains(ReturnFlags::REVERT) {
+ self.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(
memory,
- address_ptr,
- &address.as_bytes(),
+ output_ptr,
+ output_len_ptr,
+ &output.data,
true,
- already_charged,
- )?;
- }
- self.write_sandbox_output(
- memory,
- output_ptr,
- output_len_ptr,
- &output.data,
- true,
- |len| Some(RuntimeCosts::CopyToContract(len)),
- )?;
+ |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)?),
}
- Ok(Self::exec_into_return_code(instantiate_outcome.map(|(_, retval)| retval))?)
}
fn terminate(&mut self, memory: &M, beneficiary_ptr: u32) -> Result<(), TrapReason> {
@@ -1993,4 +1999,44 @@ 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().data.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_copy(
+ &mut self,
+ memory: &mut M,
+ out_ptr: u32,
+ 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::<E::T>::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?)
+ }
}
diff --git a/substrate/frame/revive/uapi/src/host.rs b/substrate/frame/revive/uapi/src/host.rs
index 57a03332670..816fdec3aaa 100644
--- a/substrate/frame/revive/uapi/src/host.rs
+++ b/substrate/frame/revive/uapi/src/host.rs
@@ -620,6 +620,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 a60c338e8bd..d5ea94c1a91 100644
--- a/substrate/frame/revive/uapi/src/host/riscv32.rs
+++ b/substrate/frame/revive/uapi/src/host/riscv32.rs
@@ -131,6 +131,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);
}
}
@@ -548,4 +550,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);
+ }
}
--
GitLab