pallet-revive: Statically verify imports on code deployment

prdoc/pr_6759.prdoc

@@ -0,0 +1,16 @@
+title: 'pallet-revive: Statically verify imports on code deployment'
+doc:
+- audience: Runtime Dev
+  description: |-
+    Previously, we failed at runtime if an unknown or unstable host function was called. This requires us to keep track of when a host function was added and when a code was deployed. We used the `api_version` to track at which API version each code was deployed. This made sure that when a new host function was added that old code won't have access to it. This is necessary as otherwise the behavior of a contract that made calls to this previously non existent host function would change from "trap" to "do something".
+
+    In this PR we remove the API version. Instead, we statically verify on upload that no non-existent host function is ever used in the code. This will allow us to add new host function later without needing to keep track when they were added.
+
+    This simplifies the code and also gives an immediate feedback if unknown host functions are used.
+crates:
+- name: pallet-revive-proc-macro
+  bump: major
+- name: pallet-revive
+  bump: major
+- name: pallet-revive-fixtures
+  bump: major

substrate/frame/revive/README.md

@@ -92,7 +92,7 @@ Driven by the desire to have an iterative approach in developing new contract in
 concept of an unstable interface. Akin to the rust nightly compiler it allows us to add new interfaces but mark them as
 unstable so that contract languages can experiment with them and give feedback before we stabilize those.
 
-In order to access interfaces which don't have a stable `#[api_version(x)]` in [`runtime.rs`](src/wasm/runtime.rs)
+In order to access interfaces which don't have a stable `#[stable]` in [`runtime.rs`](src/wasm/runtime.rs)
 one need to set `pallet_revive::Config::UnsafeUnstableInterface` to `ConstU32<true>`.
 **It should be obvious that any production runtime should never be compiled with this feature: In addition to be
 subject to change or removal those interfaces might not have proper weights associated with them and are therefore

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

@@ -0,0 +1,44 @@
+// This file is part of Substrate.
+
+// Copyright (C) Parity Technologies (UK) Ltd.
+// SPDX-License-Identifier: Apache-2.0
+
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// 	http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+#![no_std]
+#![no_main]
+
+extern crate common;
+
+#[polkavm_derive::polkavm_import]
+extern "C" {
+	pub fn __this_syscall_does_not_exist__();
+}
+
+// Export that is never called. We can put code here that should be in the binary
+// but is never supposed to be run.
+#[no_mangle]
+#[polkavm_derive::polkavm_export]
+pub extern "C" fn call_never() {
+	// make sure it is not optimized away
+	unsafe {
+		__this_syscall_does_not_exist__();
+	}
+}
+
+#[no_mangle]
+#[polkavm_derive::polkavm_export]
+pub extern "C" fn deploy() {}
+
+#[no_mangle]
+#[polkavm_derive::polkavm_export]
+pub extern "C" fn call() {}

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

@@ -0,0 +1,44 @@
+// This file is part of Substrate.
+
+// Copyright (C) Parity Technologies (UK) Ltd.
+// SPDX-License-Identifier: Apache-2.0
+
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// 	http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+#![no_std]
+#![no_main]
+
+extern crate common;
+
+#[polkavm_derive::polkavm_import]
+extern "C" {
+	pub fn set_code_hash();
+}
+
+// Export that is never called. We can put code here that should be in the binary
+// but is never supposed to be run.
+#[no_mangle]
+#[polkavm_derive::polkavm_export]
+pub extern "C" fn call_never() {
+	// make sure it is not optimized away
+	unsafe {
+		set_code_hash();
+	}
+}
+
+#[no_mangle]
+#[polkavm_derive::polkavm_export]
+pub extern "C" fn deploy() {}
+
+#[no_mangle]
+#[polkavm_derive::polkavm_export]
+pub extern "C" fn call() {}

substrate/frame/revive/proc-macro/src/lib.rs

@@ -119,7 +119,7 @@ struct EnvDef {
 /// Parsed host function definition.
 struct HostFn {
 	item: syn::ItemFn,
-	api_version: Option<u16>,
+	is_stable: bool,
 	name: String,
 	returns: HostFnReturn,
 	cfg: Option<syn::Attribute>,
@@ -183,22 +183,21 @@ impl HostFn {
 		};
 
 		// process attributes
-		let msg = "Only #[api_version(<u16>)], #[cfg] and #[mutating] attributes are allowed.";
+		let msg = "Only #[stable], #[cfg] and #[mutating] attributes are allowed.";
 		let span = item.span();
 		let mut attrs = item.attrs.clone();
 		attrs.retain(|a| !a.path().is_ident("doc"));
-		let mut api_version = None;
+		let mut is_stable = false;
 		let mut mutating = false;
 		let mut cfg = None;
 		while let Some(attr) = attrs.pop() {
 			let ident = attr.path().get_ident().ok_or(err(span, msg))?.to_string();
 			match ident.as_str() {
-				"api_version" => {
-					if api_version.is_some() {
-						return Err(err(span, "#[api_version] can only be specified once"))
+				"stable" => {
+					if is_stable {
+						return Err(err(span, "#[stable] can only be specified once"))
 					}
-					api_version =
-						Some(attr.parse_args::<syn::LitInt>().and_then(|lit| lit.base10_parse())?);
+					is_stable = true;
 				},
 				"mutating" => {
 					if mutating {
@@ -313,7 +312,7 @@ impl HostFn {
 							_ => Err(err(arg1.span(), &msg)),
 						}?;
 
-						Ok(Self { item, api_version, name, returns, cfg })
+						Ok(Self { item, is_stable, name, returns, cfg })
 					},
 					_ => Err(err(span, &msg)),
 				}
@@ -411,19 +410,23 @@ fn expand_env(def: &EnvDef) -> TokenStream2 {
 	let impls = expand_functions(def);
 	let bench_impls = expand_bench_functions(def);
 	let docs = expand_func_doc(def);
-	let highest_api_version =
-		def.host_funcs.iter().filter_map(|f| f.api_version).max().unwrap_or_default();
+	let stable_syscalls = expand_func_list(def, false);
+	let all_syscalls = expand_func_list(def, true);
 
 	quote! {
-		#[cfg(test)]
-		pub const HIGHEST_API_VERSION: u16 = #highest_api_version;
+		pub fn list_syscalls(include_unstable: bool) -> &'static [&'static [u8]] {
+			if include_unstable {
+				#all_syscalls
+			} else {
+				#stable_syscalls
+			}
+		}
 
 		impl<'a, E: Ext, M: PolkaVmInstance<E::T>> Runtime<'a, E, M> {
 			fn handle_ecall(
 				&mut self,
 				memory: &mut M,
 				__syscall_symbol__: &[u8],
-				__available_api_version__: ApiVersion,
 			) -> Result<Option<u64>, TrapReason>
 			{
 				#impls
@@ -474,10 +477,6 @@ fn expand_functions(def: &EnvDef) -> TokenStream2 {
 		let body = &f.item.block;
 		let map_output = f.returns.map_output();
 		let output = &f.item.sig.output;
-		let api_version = match f.api_version {
-			Some(version) => quote! { Some(#version) },
-			None => quote! { None },
-		};
 
 		// wrapped host function body call with host function traces
 		// see https://github.com/paritytech/polkadot-sdk/tree/master/substrate/frame/contracts#host-function-tracing
@@ -513,7 +512,7 @@ fn expand_functions(def: &EnvDef) -> TokenStream2 {
 
 		quote! {
 			#cfg
-			#syscall_symbol if __is_available__(#api_version) => {
+			#syscall_symbol => {
 				// closure is needed so that "?" can infere the correct type
 				(|| #output {
 					#arg_decoder
@@ -534,18 +533,6 @@ fn expand_functions(def: &EnvDef) -> TokenStream2 {
 		// This is the overhead to call an empty syscall that always needs to be charged.
 		self.charge_gas(crate::wasm::RuntimeCosts::HostFn).map_err(TrapReason::from)?;
 
-		// Not all APIs are available depending on configuration or when the code was deployed.
-		// This closure will be used by syscall specific code to perform this check.
-		let __is_available__ = |syscall_version: Option<u16>| {
-			match __available_api_version__ {
-				ApiVersion::UnsafeNewest => true,
-				ApiVersion::Versioned(max_available_version) =>
-					syscall_version
-						.map(|required_version| max_available_version >= required_version)
-						.unwrap_or(false),
-			}
-		};
-
 		// They will be mapped to variable names by the syscall specific code.
 		let (__a0__, __a1__, __a2__, __a3__, __a4__, __a5__) = memory.read_input_regs();
 
@@ -607,10 +594,8 @@ fn expand_func_doc(def: &EnvDef) -> TokenStream2 {
 				});
 				quote! { #( #docs )* }
 			};
-			let availability = if let Some(version) = func.api_version {
-				let info = format!(
-					"\n# Required API version\nThis API was added in version **{version}**.",
-				);
+			let availability = if func.is_stable {
+				let info = "\n# Stable API\nThis API is stable and will never change.";
 				quote! { #[doc = #info] }
 			} else {
 				let info =
@@ -632,3 +617,20 @@ fn expand_func_doc(def: &EnvDef) -> TokenStream2 {
 		#( #docs )*
 	}
 }
+
+fn expand_func_list(def: &EnvDef, include_unstable: bool) -> TokenStream2 {
+	let docs = def.host_funcs.iter().filter(|f| include_unstable || f.is_stable).map(|f| {
+		let name = Literal::byte_string(f.name.as_bytes());
+		quote! {
+			#name.as_slice()
+		}
+	});
+	let len = docs.clone().count();
+
+	quote! {
+		{
+			static FUNCS: [&[u8]; #len] = [#(#docs),*];
+			FUNCS.as_slice()
+		}
+	}
+}

substrate/frame/revive/src/benchmarking/call_builder.rs

@@ -21,7 +21,7 @@ use crate::{
 	exec::{ExportedFunction, Ext, Key, Stack},
 	storage::meter::Meter,
 	transient_storage::MeterEntry,
-	wasm::{ApiVersion, PreparedCall, Runtime},
+	wasm::{PreparedCall, Runtime},
 	BalanceOf, Config, DebugBuffer, Error, GasMeter, MomentOf, Origin, WasmBlob, Weight,
 };
 use alloc::{vec, vec::Vec};
@@ -164,13 +164,7 @@ where
 		module: WasmBlob<T>,
 		input: Vec<u8>,
 	) -> PreparedCall<'a, StackExt<'a, T>> {
-		module
-			.prepare_call(
-				Runtime::new(ext, input),
-				ExportedFunction::Call,
-				ApiVersion::UnsafeNewest,
-			)
-			.unwrap()
+		module.prepare_call(Runtime::new(ext, input), ExportedFunction::Call).unwrap()
 	}
 
 	/// Add transient_storage

substrate/frame/revive/src/lib.rs

@@ -115,19 +115,6 @@ const SENTINEL: u32 = u32::MAX;
 /// Example: `RUST_LOG=runtime::revive=debug my_code --dev`
 const LOG_TARGET: &str = "runtime::revive";
 
-/// This version determines which syscalls are available to contracts.
-///
-/// Needs to be bumped every time a versioned syscall is added.
-const API_VERSION: u16 = 0;
-
-#[test]
-fn api_version_up_to_date() {
-	assert!(
-		API_VERSION == crate::wasm::HIGHEST_API_VERSION,
-		"A new versioned API has been added. The `API_VERSION` needs to be bumped."
-	);
-}
-
 #[frame_support::pallet]
 pub mod pallet {
 	use super::*;
@@ -623,14 +610,6 @@ pub mod pallet {
 	#[pallet::storage]
 	pub(crate) type AddressSuffix<T: Config> = StorageMap<_, Identity, H160, [u8; 12]>;
 
-	#[pallet::extra_constants]
-	impl<T: Config> Pallet<T> {
-		#[pallet::constant_name(ApiVersion)]
-		fn api_version() -> u16 {
-			API_VERSION
-		}
-	}
-
 	#[pallet::hooks]
 	impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
 		fn on_idle(_block: BlockNumberFor<T>, limit: Weight) -> Weight {

substrate/frame/revive/src/limits.rs

@@ -116,7 +116,10 @@ pub mod code {
 	const BASIC_BLOCK_SIZE: u32 = 1000;
 
 	/// Make sure that the various program parts are within the defined limits.
-	pub fn enforce<T: Config>(blob: Vec<u8>) -> Result<CodeVec, DispatchError> {
+	pub fn enforce<T: Config>(
+		blob: Vec<u8>,
+		available_syscalls: &[&[u8]],
+	) -> Result<CodeVec, DispatchError> {
 		fn round_page(n: u32) -> u64 {
 			// performing the rounding in u64 in order to prevent overflow
 			u64::from(n).next_multiple_of(PAGE_SIZE.into())
@@ -134,6 +137,26 @@ pub mod code {
 			Err(Error::<T>::CodeRejected)?;
 		}
 
+		// Need to check that no non-existent syscalls are used. This allows us to add
+		// new syscalls later without affecting already deployed code.
+		for (idx, import) in program.imports().iter().enumerate() {
+			// We are being defensive in case an attacker is able to somehow include
+			// a lot of imports. This is important because we search the array of host
+			// functions for every import.
+			if idx == available_syscalls.len() {
+				log::debug!(target: LOG_TARGET, "Program contains too many imports.");
+				Err(Error::<T>::CodeRejected)?;
+			}
+			let Some(import) = import else {
+				log::debug!(target: LOG_TARGET, "Program contains malformed import.");
+				return Err(Error::<T>::CodeRejected.into());
+			};
+			if !available_syscalls.contains(&import.as_bytes()) {
+				log::debug!(target: LOG_TARGET, "Program references unknown syscall: {}", import);
+				Err(Error::<T>::CodeRejected)?;
+			}
+		}
+
 		// This scans the whole program but we only do it once on code deployment.
 		// It is safe to do unchecked math in u32 because the size of the program
 		// was already checked above.

substrate/frame/revive/src/tests.rs

@@ -4666,3 +4666,35 @@ fn mapped_address_works() {
 		assert_eq!(<Test as Config>::Currency::total_balance(&EVE), 1_100);
 	});
 }
+
+#[test]
+fn unknown_syscall_rejected() {
+	let (code, _) = compile_module("unknown_syscall").unwrap();
+
+	ExtBuilder::default().existential_deposit(100).build().execute_with(|| {
+		<Test as Config>::Currency::set_balance(&ALICE, 1_000_000);
+
+		assert_err!(
+			builder::bare_instantiate(Code::Upload(code)).build().result,
+			<Error<Test>>::CodeRejected,
+		)
+	});
+}
+
+#[test]
+fn unstable_interface_rejected() {
+	let (code, _) = compile_module("unstable_interface").unwrap();
+
+	ExtBuilder::default().existential_deposit(100).build().execute_with(|| {
+		<Test as Config>::Currency::set_balance(&ALICE, 1_000_000);
+
+		Test::set_unstable_interface(false);
+		assert_err!(
+			builder::bare_instantiate(Code::Upload(code.clone())).build().result,
+			<Error<Test>>::CodeRejected,
+		);
+
+		Test::set_unstable_interface(true);
+		assert_ok!(builder::bare_instantiate(Code::Upload(code)).build().result);
+	});
+}