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

… buffer length (paritytech#5743)

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.

---------

Signed-off-by: xermicus <[email protected]>

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())
 	}