[pallet-revive] write sandbox output according to the provided output buffer length

prdoc/pr_5743.prdoc

@@ -0,0 +1,22 @@
+title: "[pallet-revive] write sandbox output according to the provided output buffer length"
+
+doc:
+  - audience: Runtime Dev
+    description: |
+     Instead of error out if the provided output buffer is smaller than what we want to write,
+     we can just write what fits into the output buffer instead.
+     We already write back the actual bytes written to the in-out pointer,
+     so contracts can check it anyways.
+
+     This in turn introduces the benefit of allowing contracts to implicitly request only a portion
+     of the returned data from calls and incantations.
+     Which is especially beneficial for YUL as the call family opcodes have a return data size
+     argument and this change removes the need to work around it in contract code.
+
+crates:
+  - name: pallet-revive
+    bump: major
+  - name: pallet-revive-fixtures
+    bump: patch
+  - name: pallet-revive-uapi
+    bump: patch

substrate/frame/revive/fixtures/contracts/call_diverging_out_len.rs

@@ -0,0 +1,110 @@
+// 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;
+use uapi::{HostFn, HostFnImpl as api};
+
+const BUF_SIZE: usize = 8;
+static DATA: [u8; BUF_SIZE] = [1, 2, 3, 4, 5, 6, 7, 8];
+
+/// Call `callee_address` with an output buf of size `N`
+/// and expect the call output to match `expected_output`.
+fn assert_call<const N: usize>(callee_address: &[u8; 20], expected_output: [u8; BUF_SIZE]) {
+	let mut output_buf = [0u8; BUF_SIZE];
+	let mut output_buf_capped = &mut &mut output_buf[..N];
+
+	api::call(
+		uapi::CallFlags::ALLOW_REENTRY,
+		callee_address,
+		0u64,
+		0u64,
+		None,
+		&[0u8; 32],
+		&[],
+		Some(output_buf_capped),
+	)
+	.unwrap();
+
+	// The (capped) output buf should get properly resized
+	assert_eq!(output_buf_capped.len(), N);
+	assert_eq!(output_buf, expected_output);
+}
+
+/// Instantiate this contract with an output buf of size `N`
+/// and expect the instantiate output to match `expected_output`.
+fn assert_instantiate<const N: usize>(expected_output: [u8; BUF_SIZE]) {
+	let mut code_hash = [0; 32];
+	api::own_code_hash(&mut code_hash);
+
+	let mut output_buf = [0u8; BUF_SIZE];
+	let mut output_buf_capped = &mut &mut output_buf[..N];
+
+	api::instantiate(
+		&code_hash,
+		0u64,
+		0u64,
+		None,
+		&[0; 32],
+		&[0; 32],
+		None,
+		Some(output_buf_capped),
+		None,
+	)
+	.unwrap();
+
+	// The (capped) output buf should get properly resized
+	assert_eq!(output_buf_capped.len(), N);
+	assert_eq!(output_buf, expected_output);
+}
+
+#[no_mangle]
+#[polkavm_derive::polkavm_export]
+pub extern "C" fn deploy() {
+	api::return_value(uapi::ReturnFlags::empty(), &DATA);
+}
+
+#[no_mangle]
+#[polkavm_derive::polkavm_export]
+pub extern "C" fn call() {
+	let mut caller_address = [0u8; 20];
+	api::caller(&mut caller_address);
+
+	let mut callee_address = [0u8; 20];
+	api::address(&mut callee_address);
+
+	// we already recurse; return data
+	if caller_address == callee_address {
+		api::return_value(uapi::ReturnFlags::empty(), &DATA);
+	}
+
+	assert_call::<0>(&callee_address, [0; 8]);
+	assert_call::<4>(&callee_address, [1, 2, 3, 4, 0, 0, 0, 0]);
+
+	assert_instantiate::<0>([0; 8]);
+	assert_instantiate::<4>([1, 2, 3, 4, 0, 0, 0, 0]);
+}

substrate/frame/revive/fixtures/contracts/create1_with_value.rs

@@ -18,8 +18,8 @@
 #![no_std]
 #![no_main]
 
-use common::{input, u256_bytes};
-use uapi::{HostFn, HostFnImpl as api, ReturnErrorCode};
+use common::input;
+use uapi::{HostFn, HostFnImpl as api};
 
 #[no_mangle]
 #[polkavm_derive::polkavm_export]

substrate/frame/revive/src/lib.rs

@@ -467,8 +467,6 @@ pub mod pallet {
 		InvalidCallFlags,
 		/// The executed contract exhausted its gas limit.
 		OutOfGas,
-		/// The output buffer supplied to a contract API call was too small.
-		OutputBufferTooSmall,
 		/// Performing the requested transfer failed. Probably because there isn't enough
 		/// free balance in the sender's account.
 		TransferFailed,

substrate/frame/revive/src/tests.rs

@@ -4254,4 +4254,22 @@ mod run_tests {
 			assert_eq!(usable_balance, value);
 		});
 	}
+
+	#[test]
+	fn call_diverging_out_len_works() {
+		let (code, _) = compile_module("call_diverging_out_len").unwrap();
+
+		ExtBuilder::default().existential_deposit(100).build().execute_with(|| {
+			let _ = <Test as Config>::Currency::set_balance(&ALICE, 1_000_000);
+
+			// Create the contract: Constructor does nothing
+			let Contract { addr, .. } =
+				builder::bare_instantiate(Code::Upload(code)).build_and_unwrap_contract();
+
+			// Call the contract: It will issue calls and deploys, asserting on
+			// correct output if the supplied output length was smaller than
+			// than what the callee returned.
+			assert_ok!(builder::call(addr).build());
+		});
+	}
 }

substrate/frame/revive/src/wasm/runtime.rs

@@ -651,11 +651,12 @@ impl<'a, E: Ext, M: ?Sized + Memory<E::T>> 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 large enough the actual
-	/// `buf.len()` is written to this location.
+	/// 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
@@ -681,18 +682,14 @@ impl<'a, E: Ext, M: ?Sized + Memory<E::T>> Runtime<'a, E, M> {
 			return Ok(())
 		}
 
-		let buf_len = buf.len() as u32;
 		let len = memory.read_u32(out_len_ptr)?;
-
-		if len < buf_len {
-			return Err(Error::<E::T>::OutputBufferTooSmall.into())
-		}
+		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)?;
+		memory.write(out_ptr, &buf[..buf_len as usize])?;
 		memory.write(out_len_ptr, &buf_len.encode())
 	}