contracts: Deprecate random interface (#13204)

* Deprecate random interface

* Revert change to runtime file

* Fix docs

* Fix tests

* Rename to not_deprecated

* Apply suggestions from code review

Co-authored-by: Sasha Gryaznov <hi@agryaznov.com>

* Deprecate `set_rent_allowance`

Co-authored-by: Sasha Gryaznov <hi@agryaznov.com>

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

@@ -162,6 +162,8 @@ struct HostFn {
 	returns: HostFnReturn,
 	is_stable: bool,
 	alias_to: Option<String>,
+	/// Formulating the predicate inverted makes the expression using it simpler.
+	not_deprecated: bool,
 }
 
 enum HostFnReturn {
@@ -199,13 +201,14 @@ impl HostFn {
 
 		// process attributes
 		let msg =
-			"only #[version(<u8>)], #[unstable] and #[prefixed_alias] attributes are allowed.";
+			"only #[version(<u8>)], #[unstable], #[prefixed_alias] and #[deprecated] attributes are allowed.";
 		let span = item.span();
 		let mut attrs = item.attrs.clone();
 		attrs.retain(|a| !a.path.is_ident("doc"));
 		let mut maybe_module = None;
 		let mut is_stable = true;
 		let mut alias_to = None;
+		let mut not_deprecated = true;
 		while let Some(attr) = attrs.pop() {
 			let ident = attr.path.get_ident().ok_or(err(span, msg))?.to_string();
 			match ident.as_str() {
@@ -230,12 +233,22 @@ impl HostFn {
 						item.sig.ident.span(),
 					);
 				},
+				"deprecated" => {
+					if !not_deprecated {
+						return Err(err(span, "#[deprecated] can only be specified once"))
+					}
+					not_deprecated = false;
+				},
 				_ => return Err(err(span, msg)),
 			}
 		}
 		let name = item.sig.ident.to_string();
 
-		// process arguments: The first and second arg are treated differently (ctx, memory)
+		if !(is_stable || not_deprecated) {
+			return Err(err(span, "#[deprecated] is mutually exclusive with #[unstable]"))
+		}
+
+		// process arguments: The first and second args are treated differently (ctx, memory)
 		// they must exist and be `ctx: _` and `memory: _`.
 		let msg = "Every function must start with two inferred parameters: ctx: _ and memory: _";
 		let special_args = item
@@ -330,6 +343,7 @@ impl HostFn {
 							returns,
 							is_stable,
 							alias_to,
+							not_deprecated,
 						})
 					},
 					_ => Err(err(span, &msg)),
@@ -510,7 +524,12 @@ fn expand_impls(def: &mut EnvDef) -> TokenStream2 {
 	quote! {
 		impl<'a, E: Ext> crate::wasm::Environment<crate::wasm::runtime::Runtime<'a, E>> for Env
 		{
-			fn define(store: &mut ::wasmi::Store<crate::wasm::Runtime<E>>, linker: &mut ::wasmi::Linker<crate::wasm::Runtime<E>>, allow_unstable: bool) -> Result<(), ::wasmi::errors::LinkerError> {
+			fn define(
+				store: &mut ::wasmi::Store<crate::wasm::Runtime<E>>,
+				linker: &mut ::wasmi::Linker<crate::wasm::Runtime<E>>,
+				allow_unstable: AllowUnstableInterface,
+				allow_deprecated: AllowDeprecatedInterface,
+			) -> Result<(),::wasmi::errors::LinkerError> {
 				#impls
 				Ok(())
 			}
@@ -518,7 +537,12 @@ fn expand_impls(def: &mut EnvDef) -> TokenStream2 {
 
 		impl crate::wasm::Environment<()> for Env
 		{
-			fn define(store: &mut ::wasmi::Store<()>, linker: &mut ::wasmi::Linker<()>, allow_unstable: bool) -> Result<(), ::wasmi::errors::LinkerError> {
+			fn define(
+				store: &mut ::wasmi::Store<()>,
+				linker: &mut ::wasmi::Linker<()>,
+				allow_unstable: AllowUnstableInterface,
+				allow_deprecated: AllowDeprecatedInterface,
+			) -> Result<(), ::wasmi::errors::LinkerError> {
 				#dummy_impls
 				Ok(())
 			}
@@ -542,6 +566,7 @@ fn expand_functions(
 			&f.item.sig.output
 		);
 		let is_stable = f.is_stable;
+		let not_deprecated = f.not_deprecated;
 
 		// If we don't expand blocks (implementing for `()`) we change a few things:
 		// - We replace any code by unreachable!
@@ -582,9 +607,13 @@ fn expand_functions(
 		};
 
 		quote! {
-			// We need to allow unstable functions when runtime benchmarks are performed because
+			// We need to allow all interfaces when runtime benchmarks are performed because
-			// we generate the weights even when those interfaces are not enabled.
+			// we generate the weights even when those interfaces are not enabled. This
-			if ::core::cfg!(feature = "runtime-benchmarks") || #is_stable || allow_unstable {
+			// is necessary as the decision whether we allow unstable or deprecated functions
+			// is a decision made at runtime. Generation of the weights happens statically.
+			if ::core::cfg!(feature = "runtime-benchmarks") ||
+				((#is_stable || __allow_unstable__) && (#not_deprecated || __allow_deprecated__))
+			{
 				#allow_unused
 				linker.define(#module, #name, ::wasmi::Func::wrap(&mut*store, |mut __caller__: ::wasmi::Caller<#host_state>, #( #params, )*| -> #wasm_output {
 					let mut func = #inner;
@@ -596,6 +625,8 @@ fn expand_functions(
 		}
 	});
 	quote! {
+		let __allow_unstable__ = matches!(allow_unstable, AllowUnstableInterface::Yes);
+		let __allow_deprecated__ = matches!(allow_deprecated, AllowDeprecatedInterface::Yes);
 		#( #impls )*
 	}
 }
@@ -691,6 +722,16 @@ fn expand_functions(
 /// `...` modules each having its `Api` trait containing functions holding documentation for every
 /// host function defined by the macro.
 ///
+/// # Deprecated Interfaces
+///
+/// An interface can be annotated with `#[deprecated]`. It is mutually exclusive with `#[unstable]`.
+/// Deprecated interfaces have the following properties:
+/// 	- New contract codes utilizing those interfaces cannot be uploaded.
+/// 	- New contracts from existing codes utilizing those interfaces cannot be instantiated.
+/// - Existing contracts containing those interfaces still work.
+///
+/// Those interfaces will eventually be removed.
+///
 /// To build up these docs, run:
 ///
 /// ```nocompile

substrate/frame/contracts/src/benchmarking/sandbox.rs

@@ -19,7 +19,9 @@
 /// ! sandbox to execute the wasm code. This is because we do not need the full
 /// ! environment that provides the seal interface as imported functions.
 use super::{code::WasmModule, Config};
-use crate::wasm::{Environment, PrefabWasmModule};
+use crate::wasm::{
+	AllowDeprecatedInterface, AllowUnstableInterface, Environment, PrefabWasmModule,
+};
 use wasmi::{errors::LinkerError, Func, Linker, StackLimits, Store};
 
 /// Minimal execution environment without any imported functions.
@@ -49,6 +51,8 @@ impl<T: Config> From<&WasmModule<T>> for Sandbox {
 			(),
 			memory,
 			StackLimits::default(),
+			// We are testing with an empty environment anyways
+			AllowDeprecatedInterface::No,
 		)
 		.expect("Failed to create benchmarking Sandbox instance");
 		let entry_point = instance.get_export(&store, "call").unwrap().into_func().unwrap();
@@ -59,7 +63,12 @@ impl<T: Config> From<&WasmModule<T>> for Sandbox {
 struct EmptyEnv;
 
 impl Environment<()> for EmptyEnv {
-	fn define(_: &mut Store<()>, _: &mut Linker<()>, _: bool) -> Result<(), LinkerError> {
+	fn define(
+		_: &mut Store<()>,
+		_: &mut Linker<()>,
+		_: AllowUnstableInterface,
+		_: AllowDeprecatedInterface,
+	) -> Result<(), LinkerError> {
 		Ok(())
 	}
 }

substrate/frame/contracts/src/exec.rs

@@ -3440,4 +3440,35 @@ mod tests {
 			));
 		});
 	}
+
+	/// This works even though random interface is deprecated, as the check to ban deprecated
+	/// functions happens in the wasm stack which is mocked for exec tests.
+	#[test]
+	fn randomness_works() {
+		let subject = b"nice subject".as_ref();
+		let code_hash = MockLoader::insert(Call, move |ctx, _| {
+			let rand = <Test as Config>::Randomness::random(subject);
+			assert_eq!(rand, ctx.ext.random(subject));
+			exec_success()
+		});
+
+		ExtBuilder::default().build().execute_with(|| {
+			let schedule = <Test as Config>::Schedule::get();
+			place_contract(&BOB, code_hash);
+
+			let mut storage_meter = storage::meter::Meter::new(&ALICE, Some(0), 0).unwrap();
+			let result = MockStack::run_call(
+				ALICE,
+				BOB,
+				&mut GasMeter::<Test>::new(GAS_LIMIT),
+				&mut storage_meter,
+				&schedule,
+				0,
+				vec![],
+				None,
+				Determinism::Deterministic,
+			);
+			assert_matches!(result, Ok(_));
+		});
+	}
 }

substrate/frame/contracts/src/lib.rs

@@ -216,7 +216,14 @@ pub mod pallet {
 		/// The time implementation used to supply timestamps to contracts through `seal_now`.
 		type Time: Time;
 
-		/// The generator used to supply randomness to contracts through `seal_random`
+		/// The generator used to supply randomness to contracts through `seal_random`.
+		///
+		/// # Deprecated
+		///
+		/// Codes using the randomness functionality cannot be uploaded. Neither can contracts
+		/// be instantiated from existing codes that use this deprecated functionality. It will
+		/// be removed eventually. Hence for new `pallet-contracts` deployments it is okay
+		/// to supply a dummy implementation for this type (because it is never used).
 		type Randomness: Randomness<Self::Hash, Self::BlockNumber>;
 
 		/// The currency in which fees are paid and contract balances are held.

substrate/frame/contracts/src/wasm/mod.rs

@@ -24,13 +24,15 @@ mod runtime;
 
 #[cfg(feature = "runtime-benchmarks")]
 pub use crate::wasm::code_cache::reinstrument;
-pub use crate::wasm::{
-	prepare::TryInstantiate,
-	runtime::{CallFlags, Environment, ReturnCode, Runtime, RuntimeCosts},
-};
-
 #[cfg(doc)]
 pub use crate::wasm::runtime::api_doc;
+pub use crate::wasm::{
+	prepare::TryInstantiate,
+	runtime::{
+		AllowDeprecatedInterface, AllowUnstableInterface, CallFlags, Environment, ReturnCode,
+		Runtime, RuntimeCosts,
+	},
+};
 
 use crate::{
 	exec::{ExecResult, Executable, ExportedFunction, Ext},
@@ -205,6 +207,7 @@ impl<T: Config> PrefabWasmModule<T> {
 		host_state: H,
 		memory: (u32, u32),
 		stack_limits: StackLimits,
+		allow_deprecated: AllowDeprecatedInterface,
 	) -> Result<(Store<H>, Memory, Instance), wasmi::Error>
 	where
 		E: Environment<H>,
@@ -220,7 +223,16 @@ impl<T: Config> PrefabWasmModule<T> {
 		let module = Module::new(&engine, code)?;
 		let mut store = Store::new(&engine, host_state);
 		let mut linker = Linker::new();
-		E::define(&mut store, &mut linker, T::UnsafeUnstableInterface::get())?;
+		E::define(
+			&mut store,
+			&mut linker,
+			if T::UnsafeUnstableInterface::get() {
+				AllowUnstableInterface::Yes
+			} else {
+				AllowUnstableInterface::No
+			},
+			allow_deprecated,
+		)?;
 		let memory = Memory::new(&mut store, MemoryType::new(memory.0, Some(memory.1))?).expect(
 			"The limits defined in our `Schedule` limit the amount of memory well below u32::MAX; qed",
 		);
@@ -233,20 +245,14 @@ impl<T: Config> PrefabWasmModule<T> {
 		Ok((store, memory, instance))
 	}
 
-	/// Create and store the module without checking nor instrumenting the passed code.
+	/// See [`Self::from_code_unchecked`].
-	///
-	/// # Note
-	///
-	/// This is useful for benchmarking where we don't want instrumentation to skew
-	/// our results. This also does not collect any deposit from the `owner`.
 	#[cfg(feature = "runtime-benchmarks")]
 	pub fn store_code_unchecked(
 		original_code: Vec<u8>,
 		schedule: &Schedule<T>,
 		owner: T::AccountId,
 	) -> DispatchResult {
-		let executable = prepare::benchmarking::prepare(original_code, schedule, owner)
+		let executable = Self::from_code_unchecked(original_code, schedule, owner)?;
-			.map_err::<DispatchError, _>(Into::into)?;
 		code_cache::store(executable, false)
 	}
 
@@ -255,6 +261,23 @@ impl<T: Config> PrefabWasmModule<T> {
 	pub fn decrement_version(&mut self) {
 		self.instruction_weights_version = self.instruction_weights_version.checked_sub(1).unwrap();
 	}
+
+	/// Create the module without checking nor instrumenting the passed code.
+	///
+	/// # Note
+	///
+	/// This is useful for benchmarking where we don't want instrumentation to skew
+	/// our results. This also does not collect any deposit from the `owner`. Also useful
+	/// during testing when we want to deploy codes that do not pass the instantiation checks.
+	#[cfg(any(test, feature = "runtime-benchmarks"))]
+	fn from_code_unchecked(
+		original_code: Vec<u8>,
+		schedule: &Schedule<T>,
+		owner: T::AccountId,
+	) -> Result<Self, DispatchError> {
+		prepare::benchmarking::prepare(original_code, schedule, owner)
+			.map_err::<DispatchError, _>(Into::into)
+	}
 }
 
 impl<T: Config> OwnerInfo<T> {
@@ -294,6 +317,10 @@ impl<T: Config> Executable<T> for PrefabWasmModule<T> {
 			runtime,
 			(self.initial, self.maximum),
 			StackLimits::default(),
+			match function {
+				ExportedFunction::Constructor => AllowDeprecatedInterface::No,
+				ExportedFunction::Call => AllowDeprecatedInterface::Yes,
+			},
 		)
 		.map_err(|msg| {
 			log::debug!(target: "runtime::contracts", "failed to instantiate code: {}", msg);
@@ -629,38 +656,77 @@ mod tests {
 		}
 	}
 
+	/// Execute the supplied code.
+	///
+	/// Not used directly but through the wrapper functions defined below.
 	fn execute_internal<E: BorrowMut<MockExt>>(
 		wat: &str,
 		input_data: Vec<u8>,
 		mut ext: E,
+		entry_point: &ExportedFunction,
 		unstable_interface: bool,
+		skip_checks: bool,
 	) -> ExecResult {
 		type RuntimeConfig = <MockExt as Ext>::T;
 		RuntimeConfig::set_unstable_interface(unstable_interface);
 		let wasm = wat::parse_str(wat).unwrap();
 		let schedule = crate::Schedule::default();
-		let executable = PrefabWasmModule::<RuntimeConfig>::from_code(
+		let executable = if skip_checks {
-			wasm,
+			PrefabWasmModule::<RuntimeConfig>::from_code_unchecked(wasm, &schedule, ALICE)?
-			&schedule,
+		} else {
-			ALICE,
+			PrefabWasmModule::<RuntimeConfig>::from_code(
-			Determinism::Deterministic,
+				wasm,
-			TryInstantiate::Skip,
+				&schedule,
-		)
+				ALICE,
-		.map_err(|err| err.0)?;
+				Determinism::Deterministic,
-		executable.execute(ext.borrow_mut(), &ExportedFunction::Call, input_data)
+				TryInstantiate::Instantiate,
+			)
+			.map_err(|err| err.0)?
+		};
+		executable.execute(ext.borrow_mut(), entry_point, input_data)
 	}
 
+	/// Execute the suppplied code.
 	fn execute<E: BorrowMut<MockExt>>(wat: &str, input_data: Vec<u8>, ext: E) -> ExecResult {
-		execute_internal(wat, input_data, ext, true)
+		execute_internal(wat, input_data, ext, &ExportedFunction::Call, true, false)
 	}
 
+	/// Execute the supplied code with disabled unstable functions.
+	///
+	/// In our test config unstable functions are disabled so that we can test them.
+	/// In order to test that code using them is properly rejected we temporarily disable
+	/// them when this test is run.
 	#[cfg(not(feature = "runtime-benchmarks"))]
 	fn execute_no_unstable<E: BorrowMut<MockExt>>(
 		wat: &str,
 		input_data: Vec<u8>,
 		ext: E,
 	) -> ExecResult {
-		execute_internal(wat, input_data, ext, false)
+		execute_internal(wat, input_data, ext, &ExportedFunction::Call, false, false)
+	}
+
+	/// Execute code without validating it first.
+	///
+	/// This is mainly useful in order to test code which uses deprecated functions. Those
+	/// would fail when validating the code.
+	fn execute_unvalidated<E: BorrowMut<MockExt>>(
+		wat: &str,
+		input_data: Vec<u8>,
+		ext: E,
+	) -> ExecResult {
+		execute_internal(wat, input_data, ext, &ExportedFunction::Call, false, true)
+	}
+
+	/// Execute instantiation entry point of code without validating it first.
+	///
+	/// Same as `execute_unvalidated` except that the `deploy` entry point is ran.
+	#[cfg(not(feature = "runtime-benchmarks"))]
+	fn execute_instantiate_unvalidated<E: BorrowMut<MockExt>>(
+		wat: &str,
+		input_data: Vec<u8>,
+		ext: E,
+	) -> ExecResult {
+		execute_internal(wat, input_data, ext, &ExportedFunction::Constructor, false, true)
 	}
 
 	const CODE_TRANSFER: &str = r#"
@@ -1861,7 +1927,7 @@ mod tests {
 
 	#[test]
 	fn random() {
-		let output = execute(CODE_RANDOM, vec![], MockExt::default()).unwrap();
+		let output = execute_unvalidated(CODE_RANDOM, vec![], MockExt::default()).unwrap();
 
 		// The mock ext just returns the same data that was passed as the subject.
 		assert_eq!(
@@ -1931,7 +1997,7 @@ mod tests {
 
 	#[test]
 	fn random_v1() {
-		let output = execute(CODE_RANDOM_V1, vec![], MockExt::default()).unwrap();
+		let output = execute_unvalidated(CODE_RANDOM_V1, vec![], MockExt::default()).unwrap();
 
 		// The mock ext just returns the same data that was passed as the subject.
 		assert_eq!(
@@ -3007,6 +3073,29 @@ mod tests {
 		execute(CODE, vec![], &mut mock_ext).unwrap();
 	}
 
+	/// Code with deprecated functions cannot be uploaded or instantiated. However, we
+	/// need to make sure that it still can be re-instrumented.
+	#[test]
+	fn can_reinstrument_deprecated() {
+		const CODE_RANDOM: &str = r#"
+(module
+	(import "seal0" "random" (func $seal_random (param i32 i32 i32 i32)))
+	(func (export "call"))
+	(func (export "deploy"))
+)
+	"#;
+		let wasm = wat::parse_str(CODE_RANDOM).unwrap();
+		let schedule = crate::Schedule::<Test>::default();
+		#[cfg(not(feature = "runtime-benchmarks"))]
+		assert_err!(execute(CODE_RANDOM, vec![], MockExt::default()), <Error<Test>>::CodeRejected);
+		self::prepare::reinstrument::<runtime::Env, Test>(
+			&wasm,
+			&schedule,
+			Determinism::Deterministic,
+		)
+		.unwrap();
+	}
+
 	/// This test check that an unstable interface cannot be deployed. In case of runtime
 	/// benchmarks we always allow unstable interfaces. This is why this test does not
 	/// work when this feature is enabled.
@@ -3026,4 +3115,80 @@ mod tests {
 		);
 		assert_ok!(execute(CANNOT_DEPLOY_UNSTABLE, vec![], MockExt::default()));
 	}
+
+	/// The random interface is deprecated and hence new contracts using it should not be deployed.
+	/// In case of runtime benchmarks we always allow deprecated interfaces. This is why this
+	/// test doesn't work if this feature is enabled.
+	#[cfg(not(feature = "runtime-benchmarks"))]
+	#[test]
+	fn cannot_deploy_deprecated() {
+		const CODE_RANDOM_0: &str = r#"
+(module
+	(import "seal0" "seal_random" (func $seal_random (param i32 i32 i32 i32)))
+	(func (export "call"))
+	(func (export "deploy"))
+)
+	"#;
+		const CODE_RANDOM_1: &str = r#"
+(module
+	(import "seal1" "seal_random" (func $seal_random (param i32 i32 i32 i32)))
+	(func (export "call"))
+	(func (export "deploy"))
+)
+	"#;
+		const CODE_RANDOM_2: &str = r#"
+(module
+	(import "seal0" "random" (func $seal_random (param i32 i32 i32 i32)))
+	(func (export "call"))
+	(func (export "deploy"))
+)
+	"#;
+		const CODE_RANDOM_3: &str = r#"
+(module
+	(import "seal1" "random" (func $seal_random (param i32 i32 i32 i32)))
+	(func (export "call"))
+	(func (export "deploy"))
+)
+	"#;
+
+		assert_ok!(execute_unvalidated(CODE_RANDOM_0, vec![], MockExt::default()));
+		assert_err!(
+			execute_instantiate_unvalidated(CODE_RANDOM_0, vec![], MockExt::default()),
+			<Error<Test>>::CodeRejected,
+		);
+		assert_err!(
+			execute(CODE_RANDOM_0, vec![], MockExt::default()),
+			<Error<Test>>::CodeRejected,
+		);
+
+		assert_ok!(execute_unvalidated(CODE_RANDOM_1, vec![], MockExt::default()));
+		assert_err!(
+			execute_instantiate_unvalidated(CODE_RANDOM_1, vec![], MockExt::default()),
+			<Error<Test>>::CodeRejected,
+		);
+		assert_err!(
+			execute(CODE_RANDOM_1, vec![], MockExt::default()),
+			<Error<Test>>::CodeRejected,
+		);
+
+		assert_ok!(execute_unvalidated(CODE_RANDOM_2, vec![], MockExt::default()));
+		assert_err!(
+			execute_instantiate_unvalidated(CODE_RANDOM_2, vec![], MockExt::default()),
+			<Error<Test>>::CodeRejected,
+		);
+		assert_err!(
+			execute(CODE_RANDOM_2, vec![], MockExt::default()),
+			<Error<Test>>::CodeRejected,
+		);
+
+		assert_ok!(execute_unvalidated(CODE_RANDOM_3, vec![], MockExt::default()));
+		assert_err!(
+			execute_instantiate_unvalidated(CODE_RANDOM_3, vec![], MockExt::default()),
+			<Error<Test>>::CodeRejected,
+		);
+		assert_err!(
+			execute(CODE_RANDOM_3, vec![], MockExt::default()),
+			<Error<Test>>::CodeRejected,
+		);
+	}
 }

substrate/frame/contracts/src/wasm/prepare.rs

@@ -22,7 +22,9 @@
 use crate::{
 	chain_extension::ChainExtension,
 	storage::meter::Diff,
-	wasm::{Determinism, Environment, OwnerInfo, PrefabWasmModule},
+	wasm::{
+		runtime::AllowDeprecatedInterface, Determinism, Environment, OwnerInfo, PrefabWasmModule,
+	},
 	AccountIdOf, CodeVec, Config, Error, Schedule,
 };
 use codec::{Encode, MaxEncodedLen};
@@ -54,6 +56,14 @@ pub enum TryInstantiate {
 	Skip,
 }
 
+/// The reason why a contract is instrumented.
+enum InstrumentReason {
+	/// A new code is uploaded.
+	New,
+	/// Existing code is re-instrumented.
+	Reinstrument,
+}
+
 struct ContractModule<'a, T: Config> {
 	/// A deserialized module. The module is valid (this is Guaranteed by `new` method).
 	module: elements::Module,
@@ -381,6 +391,7 @@ fn instrument<E, T>(
 	schedule: &Schedule<T>,
 	determinism: Determinism,
 	try_instantiate: TryInstantiate,
+	reason: InstrumentReason,
 ) -> Result<(Vec<u8>, (u32, u32)), (DispatchError, &'static str)>
 where
 	E: Environment<()>,
@@ -454,11 +465,20 @@ where
 		// We don't actually ever run any code so we can get away with a minimal stack which
 		// reduces the amount of memory that needs to be zeroed.
 		let stack_limits = StackLimits::new(1, 1, 0).expect("initial <= max; qed");
-		PrefabWasmModule::<T>::instantiate::<E, _>(&code, (), (initial, maximum), stack_limits)
+		PrefabWasmModule::<T>::instantiate::<E, _>(
-			.map_err(|err| {
+			&code,
-				log::debug!(target: "runtime::contracts", "{}", err);
+			(),
-				(Error::<T>::CodeRejected.into(), "new code rejected after instrumentation")
+			(initial, maximum),
-			})?;
+			stack_limits,
+			match reason {
+				InstrumentReason::New => AllowDeprecatedInterface::No,
+				InstrumentReason::Reinstrument => AllowDeprecatedInterface::Yes,
+			},
+		)
+		.map_err(|err| {
+			log::debug!(target: "runtime::contracts", "{}", err);
+			(Error::<T>::CodeRejected.into(), "new code rejected after instrumentation")
+		})?;
 	}
 
 	Ok((code, (initial, maximum)))
@@ -486,8 +506,13 @@ where
 	E: Environment<()>,
 	T: Config,
 {
-	let (code, (initial, maximum)) =
+	let (code, (initial, maximum)) = instrument::<E, T>(
-		instrument::<E, T>(original_code.as_ref(), schedule, determinism, try_instantiate)?;
+		original_code.as_ref(),
+		schedule,
+		determinism,
+		try_instantiate,
+		InstrumentReason::New,
+	)?;
 
 	let original_code_len = original_code.len();
 
@@ -532,21 +557,30 @@ where
 	E: Environment<()>,
 	T: Config,
 {
-	instrument::<E, T>(original_code, schedule, determinism, TryInstantiate::Skip)
+	instrument::<E, T>(
-		.map_err(|(err, msg)| {
+		original_code,
-			log::error!(target: "runtime::contracts", "CodeRejected during reinstrument: {}", msg);
+		schedule,
-			err
+		determinism,
-		})
+		// This function was triggered by an interaction with an existing contract code
-		.map(|(code, _)| code)
+		// that will try to instantiate anyways. Failing here would not help
+		// as the contract is already on chain.
+		TryInstantiate::Skip,
+		InstrumentReason::Reinstrument,
+	)
+	.map_err(|(err, msg)| {
+		log::error!(target: "runtime::contracts", "CodeRejected during reinstrument: {}", msg);
+		err
+	})
+	.map(|(code, _)| code)
 }
 
-/// Alternate (possibly unsafe) preparation functions used only for benchmarking.
+/// Alternate (possibly unsafe) preparation functions used only for benchmarking and testing.
 ///
 /// For benchmarking we need to construct special contracts that might not pass our
 /// sanity checks or need to skip instrumentation for correct results. We hide functions
-/// allowing this behind a feature that is only set during benchmarking to prevent usage
+/// allowing this behind a feature that is only set during benchmarking or testing to
-/// in production code.
+/// prevent usage in production code.
-#[cfg(feature = "runtime-benchmarks")]
+#[cfg(any(test, feature = "runtime-benchmarks"))]
 pub mod benchmarking {
 	use super::*;
 
@@ -600,7 +634,7 @@ mod tests {
 	#[allow(unreachable_code)]
 	mod env {
 		use super::*;
-		use crate::wasm::runtime::TrapReason;
+		use crate::wasm::runtime::{AllowDeprecatedInterface, AllowUnstableInterface, TrapReason};
 
 		// Define test environment for tests. We need ImportSatisfyCheck
 		// implementation from it. So actual implementations doesn't matter.

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

@@ -37,6 +37,22 @@ use wasmi::{core::HostError, errors::LinkerError, Linker, Memory, Store};
 /// The maximum nesting depth a contract can use when encoding types.
 const MAX_DECODE_NESTING: u32 = 256;
 
+/// Passed to [`Environment`] to determine whether it should expose deprecated interfaces.
+pub enum AllowDeprecatedInterface {
+	/// No deprecated interfaces are exposed.
+	No,
+	/// Deprecated interfaces are exposed.
+	Yes,
+}
+
+/// Passed to [`Environment`] to determine whether it should expose unstable interfaces.
+pub enum AllowUnstableInterface {
+	/// No unstable interfaces are exposed.
+	No,
+	/// Unstable interfaces are exposed.
+	Yes,
+}
+
 /// Trait implemented by the [`define_env`](pallet_contracts_proc_macro::define_env) macro for the
 /// emitted `Env` struct.
 pub trait Environment<HostState> {
@@ -45,14 +61,15 @@ pub trait Environment<HostState> {
 	fn define(
 		store: &mut Store<HostState>,
 		linker: &mut Linker<HostState>,
-		allow_unstable: bool,
+		allow_unstable: AllowUnstableInterface,
+		allow_deprecated: AllowDeprecatedInterface,
 	) -> Result<(), LinkerError>;
 }
 
 /// Type of a storage key.
 #[allow(dead_code)]
 enum KeyType {
-	/// Deprecated fix sized key `[u8;32]`.
+	/// Legacy fix sized key `[u8;32]`.
 	Fix,
 	/// Variable sized key used in transparent hashing,
 	/// cannot be larger than MaxStorageKeyLen.
@@ -91,12 +108,8 @@ pub enum ReturnCode {
 	CalleeReverted = 2,
 	/// The passed key does not exist in storage.
 	KeyNotFound = 3,
-	/// Deprecated and no longer returned: There is only the minimum balance.
-	_BelowSubsistenceThreshold = 4,
 	/// See [`Error::TransferFailed`].
 	TransferFailed = 5,
-	/// Deprecated and no longer returned: Endowment is no longer required.
-	_EndowmentTooLow = 6,
 	/// No code could be found at the supplied code hash.
 	CodeNotFound = 7,
 	/// The contract that was called is no contract (a plain account).
@@ -1280,7 +1293,7 @@ pub mod env {
 
 	/// Make a call to another contract.
 	///
-	/// # Deprecation
+	/// # New version available
 	///
 	/// This is equivalent to calling the newer version of this function with
 	/// `flags` set to `ALLOW_REENTRY`. See the newer version for documentation.
@@ -1418,7 +1431,7 @@ pub mod env {
 
 	/// Instantiate a contract with the specified code hash.
 	///
-	/// # Deprecation
+	/// # New version available
 	///
 	/// This is equivalent to calling the newer version of this function. The newer version
 	/// drops the now unnecessary length fields.
@@ -1538,7 +1551,7 @@ pub mod env {
 
 	/// Remove the calling account and transfer remaining balance.
 	///
-	/// # Deprecation
+	/// # New version available
 	///
 	/// This is equivalent to calling the newer version of this function. The newer version
 	/// drops the now unnecessary length fields.
@@ -1879,12 +1892,8 @@ pub mod env {
 	/// space at `out_ptr` is less than the size of the value a trap is triggered.
 	///
 	/// The data is encoded as `T::Hash`.
-	///
-	/// # Deprecation
-	///
-	/// This function is deprecated. Users should migrate to the [`super::seal1::Api::random()`]
-	/// version.
 	#[prefixed_alias]
+	#[deprecated]
 	fn random(
 		ctx: _,
 		memory: _,
@@ -1931,6 +1940,7 @@ pub mod env {
 	/// commitment.
 	#[version(1)]
 	#[prefixed_alias]
+	#[deprecated]
 	fn random(
 		ctx: _,
 		memory: _,
@@ -2001,10 +2011,11 @@ pub mod env {
 	/// `out_ptr`. This call overwrites it with the size of the value. If the available
 	/// space at `out_ptr` is less than the size of the value a trap is triggered.
 	///
-	/// # Deprecation
+	/// # Note
 	///
 	/// There is no longer a tombstone deposit. This function always returns `0`.
 	#[prefixed_alias]
+	#[deprecated]
 	fn tombstone_deposit(
 		ctx: _,
 		memory: _,
@@ -2030,6 +2041,7 @@ pub mod env {
 	/// The state rent functionality was removed. This is stub only exists for
 	/// backwards compatiblity
 	#[prefixed_alias]
+	#[deprecated]
 	fn restore_to(
 		ctx: _,
 		memory: _,
@@ -2054,6 +2066,7 @@ pub mod env {
 	/// backwards compatiblity
 	#[version(1)]
 	#[prefixed_alias]
+	#[deprecated]
 	fn restore_to(
 		ctx: _,
 		memory: _,
@@ -2067,6 +2080,59 @@ pub mod env {
 		Ok(())
 	}
 
+	/// Was used to set rent allowance of the contract.
+	///
+	/// # Note
+	///
+	/// The state rent functionality was removed. This is stub only exists for
+	/// backwards compatiblity.
+	#[prefixed_alias]
+	#[deprecated]
+	fn set_rent_allowance(
+		ctx: _,
+		memory: _,
+		_value_ptr: u32,
+		_value_len: u32,
+	) -> Result<(), TrapReason> {
+		ctx.charge_gas(RuntimeCosts::DebugMessage)?;
+		Ok(())
+	}
+
+	/// Was used to set rent allowance of the contract.
+	///
+	/// # Note
+	///
+	/// The state rent functionality was removed. This is stub only exists for
+	/// backwards compatiblity.
+	#[version(1)]
+	#[prefixed_alias]
+	#[deprecated]
+	fn set_rent_allowance(ctx: _, _memory: _, _value_ptr: u32) -> Result<(), TrapReason> {
+		ctx.charge_gas(RuntimeCosts::DebugMessage)?;
+		Ok(())
+	}
+
+	/// Was used to store the rent allowance into the supplied buffer.
+	///
+	/// # Note
+	///
+	/// The state rent functionality was removed. This is stub only exists for
+	/// backwards compatiblity.
+	#[prefixed_alias]
+	#[deprecated]
+	fn rent_allowance(ctx: _, memory: _, out_ptr: u32, out_len_ptr: u32) -> Result<(), TrapReason> {
+		ctx.charge_gas(RuntimeCosts::Balance)?;
+		let rent_allowance = <BalanceOf<E::T>>::max_value().encode();
+		Ok(ctx.write_sandbox_output(
+			memory,
+			out_ptr,
+			out_len_ptr,
+			&rent_allowance,
+			false,
+			already_charged,
+		)?)
+	}
+
 	/// Deposit a contract event with the data buffer and optional list of topics. There is a limit
 	/// on the maximum number of topics specified by `event_topics`.
 	///
@@ -2110,56 +2176,6 @@ pub mod env {
 		Ok(())
 	}
 
-	/// Was used to set rent allowance of the contract.
-	///
-	/// # Note
-	///
-	/// The state rent functionality was removed. This is stub only exists for
-	/// backwards compatiblity.
-	#[prefixed_alias]
-	fn set_rent_allowance(
-		ctx: _,
-		memory: _,
-		_value_ptr: u32,
-		_value_len: u32,
-	) -> Result<(), TrapReason> {
-		ctx.charge_gas(RuntimeCosts::DebugMessage)?;
-		Ok(())
-	}
-
-	/// Was used to set rent allowance of the contract.
-	///
-	/// # Note
-	///
-	/// The state rent functionality was removed. This is stub only exists for
-	/// backwards compatiblity.
-	#[version(1)]
-	#[prefixed_alias]
-	fn set_rent_allowance(ctx: _, _memory: _, _value_ptr: u32) -> Result<(), TrapReason> {
-		ctx.charge_gas(RuntimeCosts::DebugMessage)?;
-		Ok(())
-	}
-
-	/// Was used to store the rent allowance into the supplied buffer.
-	///
-	/// # Note
-	///
-	/// The state rent functionality was removed. This is stub only exists for
-	/// backwards compatiblity.
-	#[prefixed_alias]
-	fn rent_allowance(ctx: _, memory: _, out_ptr: u32, out_len_ptr: u32) -> Result<(), TrapReason> {
-		ctx.charge_gas(RuntimeCosts::Balance)?;
-		let rent_allowance = <BalanceOf<E::T>>::max_value().encode();
-		Ok(ctx.write_sandbox_output(
-			memory,
-			out_ptr,
-			out_len_ptr,
-			&rent_allowance,
-			false,
-			already_charged,
-		)?)
-	}
-
 	/// Stores the current block number of the current contract into the supplied buffer.
 	///
 	/// The value is stored to linear memory at the address pointed to by `out_ptr`.