contracts: Replace cargo feature unstable-interface with config (#12787)

* Replace cargo feature with config

* Update frame/contracts/proc-macro/src/lib.rs

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

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

substrate/bin/node/runtime/Cargo.toml

@@ -312,9 +312,6 @@ try-runtime = [
 	"pallet-vesting/try-runtime",
 	"pallet-whitelist/try-runtime",
 ]
-# Make contract callable functions marked as __unstable__ available. Do not enable
-# on live chains as those are subject to change.
-contracts-unstable-interface = ["pallet-contracts/unstable-interface"]
 # Force `sp-sandbox` to call into the host resident executor. One still need to make sure
 # that `sc-executor` gets the `wasmer-sandbox` feature which happens automatically when
 # specified on the command line.

substrate/bin/node/runtime/src/lib.rs

@@ -32,7 +32,7 @@ use frame_support::{
 	pallet_prelude::Get,
 	parameter_types,
 	traits::{
-		AsEnsureOriginWithArg, ConstU128, ConstU16, ConstU32, Currency, EitherOfDiverse,
+		AsEnsureOriginWithArg, ConstBool, ConstU128, ConstU16, ConstU32, Currency, EitherOfDiverse,
 		EqualPrivilegeOnly, Everything, Imbalance, InstanceFilter, KeyOwnerProofSystem,
 		LockIdentifier, Nothing, OnUnbalanced, U128CurrencyToVote, WithdrawReasons,
 	},
@@ -1191,6 +1191,7 @@ impl pallet_contracts::Config for Runtime {
 	type AddressGenerator = pallet_contracts::DefaultAddressGenerator;
 	type MaxCodeLen = ConstU32<{ 128 * 1024 }>;
 	type MaxStorageKeyLen = ConstU32<128>;
+	type UnsafeUnstableInterface = ConstBool<false>;
 }
 
 impl pallet_sudo::Config for Runtime {

substrate/frame/contracts/Cargo.toml

@@ -84,9 +84,5 @@ runtime-benchmarks = [
 	"frame-benchmarking/runtime-benchmarks",
 	"rand",
 	"rand_pcg",
-	"unstable-interface",
 ]
 try-runtime = ["frame-support/try-runtime"]
-# Make contract callable functions marked as unstable available. Do not enable
-# on live chains as those are subject to change.
-unstable-interface = []

substrate/frame/contracts/README.md

@@ -142,18 +142,11 @@ this pallet contains the concept of an unstable interface. Akin to the rust nigh
 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 marked as `#[unstable]` in `runtime.rs` one need to compile
+In order to access interfaces marked as `#[unstable]` in `runtime.rs` one need to set
-this crate with the `unstable-interface` feature enabled. It should be obvious that any
+`pallet_contracts::Config::UnsafeUnstableInterface` to `ConstU32<true>`. It should be obvious
-live runtime should never be compiled with this feature: In addition to be subject to
+that any production runtime should never be compiled with this feature: In addition to be
-change or removal those interfaces do not have proper weights associated with them and
+subject to change or removal those interfaces might not have proper weights associated with
-are therefore considered unsafe.
+them and are therefore considered unsafe.
-
-The substrate runtime exposes this feature as `contracts-unstable-interface`. Example
-commandline for running the substrate node with unstable contracts interfaces:
-
-```bash
-cargo run --release --features contracts-unstable-interface -- --dev
-```
 
 New interfaces are generally added as unstable and might go through several iterations
 before they are promoted to a stable interface.

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

@@ -157,7 +157,7 @@ struct HostFn {
 	module: String,
 	name: String,
 	returns: HostFnReturn,
-	is_unstable: bool,
+	is_stable: bool,
 }
 
 enum HostFnReturn {
@@ -199,7 +199,7 @@ impl HostFn {
 		attrs.retain(|a| !(a.path.is_ident("doc") || a.path.is_ident("prefixed_alias")));
 		let name = item.sig.ident.to_string();
 		let mut maybe_module = None;
-		let mut is_unstable = false;
+		let mut is_stable = true;
 		while let Some(attr) = attrs.pop() {
 			let ident = attr.path.get_ident().ok_or(err(span, msg))?.to_string();
 			match ident.as_str() {
@@ -212,10 +212,10 @@ impl HostFn {
 					maybe_module = Some(format!("seal{}", ver));
 				},
 				"unstable" => {
-					if is_unstable {
+					if !is_stable {
 						return Err(err(span, "#[unstable] can only be specified once"))
 					}
-					is_unstable = true;
+					is_stable = false;
 				},
 				_ => return Err(err(span, msg)),
 			}
@@ -312,7 +312,7 @@ impl HostFn {
 							module: maybe_module.unwrap_or_else(|| "seal0".to_string()),
 							name,
 							returns,
-							is_unstable,
+							is_stable,
 						})
 					},
 					_ => Err(err(span, &msg)),
@@ -406,7 +406,7 @@ fn expand_impls(def: &mut EnvDef) -> TokenStream2 {
 			<E::T as ::frame_system::Config>::AccountId:
 				::sp_core::crypto::UncheckedFrom<<E::T as ::frame_system::Config>::Hash> + ::core::convert::AsRef<[::core::primitive::u8]>,
 		{
-			fn define(store: &mut ::wasmi::Store<crate::wasm::Runtime<E>>, linker: &mut ::wasmi::Linker<crate::wasm::Runtime<E>>) -> Result<(), ::wasmi::errors::LinkerError> {
+			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> {
 				#impls
 				Ok(())
 			}
@@ -414,7 +414,7 @@ fn expand_impls(def: &mut EnvDef) -> TokenStream2 {
 
 		impl crate::wasm::Environment<()> for Env
 		{
-			fn define(store: &mut ::wasmi::Store<()>, linker: &mut ::wasmi::Linker<()>) -> Result<(), ::wasmi::errors::LinkerError> {
+			fn define(store: &mut ::wasmi::Store<()>, linker: &mut ::wasmi::Linker<()>, allow_unstable: bool) -> Result<(), ::wasmi::errors::LinkerError> {
 				#dummy_impls
 				Ok(())
 			}
@@ -437,10 +437,7 @@ fn expand_functions(
 			f.returns.to_wasm_sig(),
 			&f.item.sig.output
 		);
-		let unstable_feat = match f.is_unstable {
+		let is_stable = f.is_stable;
-			true => quote! { #[cfg(feature = "unstable-interface")] },
-			false => quote! {},
-		};
 
 		// If we don't expand blocks (implementing for `()`) we change a few things:
 		// - We replace any code by unreachable!
@@ -480,16 +477,18 @@ fn expand_functions(
 			quote! { #[allow(unused_variables)] }
 		};
 
-
 		quote! {
-			#unstable_feat
+			// We need to allow unstable functions when runtime benchmarks are performed because
-			#allow_unused
+			// we generate the weights even when those interfaces are not enabled.
-			linker.define(#module, #name, ::wasmi::Func::wrap(&mut*store, |mut __caller__: ::wasmi::Caller<#host_state>, #( #params, )*| -> #wasm_output {
+			if ::core::cfg!(feature = "runtime-benchmarks") || #is_stable || allow_unstable {
-				let mut func = #inner;
+				#allow_unused
-				func()
+				linker.define(#module, #name, ::wasmi::Func::wrap(&mut*store, |mut __caller__: ::wasmi::Caller<#host_state>, #( #params, )*| -> #wasm_output {
-					.map_err(#map_err)
+					let mut func = #inner;
-					.map(::core::convert::Into::into)
+					func()
-			}))?;
+						.map_err(#map_err)
+						.map(::core::convert::Into::into)
+				}))?;
+			}
 		}
 	});
 	quote! {

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

@@ -64,7 +64,7 @@ where
 struct EmptyEnv;
 
 impl Environment<()> for EmptyEnv {
-	fn define(_store: &mut Store<()>, _linker: &mut Linker<()>) -> Result<(), LinkerError> {
+	fn define(_: &mut Store<()>, _: &mut Linker<()>, _: bool) -> Result<(), LinkerError> {
 		Ok(())
 	}
 }

substrate/frame/contracts/src/lib.rs

@@ -326,10 +326,24 @@ pub mod pallet {
 		/// The maximum length of a contract code in bytes. This limit applies to the instrumented
 		/// version of the code. Therefore `instantiate_with_code` can fail even when supplying
 		/// a wasm binary below this maximum size.
+		#[pallet::constant]
 		type MaxCodeLen: Get<u32>;
 
 		/// The maximum allowable length in bytes for storage keys.
+		#[pallet::constant]
 		type MaxStorageKeyLen: Get<u32>;
+
+		/// Make contract callable functions marked as `#[unstable]` available.
+		///
+		/// Contracts that use `#[unstable]` functions won't be able to be uploaded unless
+		/// this is set to `true`. This is only meant for testnets and dev nodes in order to
+		/// experiment with new features.
+		///
+		/// # Warning
+		///
+		/// Do **not** set to `true` on productions chains.
+		#[pallet::constant]
+		type UnsafeUnstableInterface: Get<bool>;
 	}
 
 	#[pallet::hooks]

substrate/frame/contracts/src/tests.rs

@@ -122,6 +122,12 @@ pub mod test_utils {
 	}
 }
 
+impl Test {
+	pub fn set_unstable_interface(unstable_interface: bool) {
+		UNSTABLE_INTERFACE.with(|v| *v.borrow_mut() = unstable_interface);
+	}
+}
+
 parameter_types! {
 	static TestExtensionTestValue: TestExtension = Default::default();
 }
@@ -385,6 +391,7 @@ impl Contains<RuntimeCall> for TestFilter {
 
 parameter_types! {
 	pub const DeletionWeightLimit: Weight = Weight::from_ref_time(500_000_000_000);
+	pub static UnstableInterface: bool = true;
 }
 
 impl Config for Test {
@@ -407,6 +414,7 @@ impl Config for Test {
 	type AddressGenerator = DefaultAddressGenerator;
 	type MaxCodeLen = ConstU32<{ 128 * 1024 }>;
 	type MaxStorageKeyLen = ConstU32<128>;
+	type UnsafeUnstableInterface = UnstableInterface;
 }
 
 pub const ALICE: AccountId32 = AccountId32::new([1u8; 32]);
@@ -2687,7 +2695,6 @@ fn gas_estimation_nested_call_fixed_limit() {
 }
 
 #[test]
-#[cfg(feature = "unstable-interface")]
 fn gas_estimation_call_runtime() {
 	use codec::Decode;
 	let (caller_code, caller_hash) = compile_module::<Test>("call_runtime").unwrap();
@@ -4411,7 +4418,6 @@ fn delegate_call_indeterministic_code() {
 }
 
 #[test]
-#[cfg(feature = "unstable-interface")]
 fn reentrance_count_works_with_call() {
 	let (wasm, code_hash) = compile_module::<Test>("reentrance_count_call").unwrap();
 	let contract_addr = Contracts::contract_address(&ALICE, &code_hash, &[]);
@@ -4448,7 +4454,6 @@ fn reentrance_count_works_with_call() {
 }
 
 #[test]
-#[cfg(feature = "unstable-interface")]
 fn reentrance_count_works_with_delegated_call() {
 	let (wasm, code_hash) = compile_module::<Test>("reentrance_count_delegated_call").unwrap();
 	let contract_addr = Contracts::contract_address(&ALICE, &code_hash, &[]);
@@ -4485,7 +4490,6 @@ fn reentrance_count_works_with_delegated_call() {
 }
 
 #[test]
-#[cfg(feature = "unstable-interface")]
 fn account_reentrance_count_works() {
 	let (wasm, code_hash) = compile_module::<Test>("account_reentrance_count_call").unwrap();
 	let (wasm_reentrance_count, code_hash_reentrance_count) =

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

@@ -36,7 +36,7 @@ use crate::{
 };
 use codec::{Decode, Encode, MaxEncodedLen};
 use frame_support::dispatch::{DispatchError, DispatchResult};
-use sp_core::crypto::UncheckedFrom;
+use sp_core::{crypto::UncheckedFrom, Get};
 use sp_runtime::RuntimeDebug;
 use sp_std::prelude::*;
 #[cfg(test)]
@@ -218,7 +218,7 @@ where
 		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)?;
+		E::define(&mut store, &mut linker, T::UnsafeUnstableInterface::get())?;
 		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",
 		);
@@ -627,10 +627,17 @@ mod tests {
 		}
 	}
 
-	fn execute<E: BorrowMut<MockExt>>(wat: &str, input_data: Vec<u8>, mut ext: E) -> ExecResult {
+	fn execute_internal<E: BorrowMut<MockExt>>(
+		wat: &str,
+		input_data: Vec<u8>,
+		mut ext: E,
+		unstable_interface: 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::<<MockExt as Ext>::T>::from_code(
+		let executable = PrefabWasmModule::<RuntimeConfig>::from_code(
 			wasm,
 			&schedule,
 			ALICE,
@@ -641,6 +648,19 @@ mod tests {
 		executable.execute(ext.borrow_mut(), &ExportedFunction::Call, input_data)
 	}
 
+	fn execute<E: BorrowMut<MockExt>>(wat: &str, input_data: Vec<u8>, ext: E) -> ExecResult {
+		execute_internal(wat, input_data, ext, false)
+	}
+
+	#[cfg(not(feature = "runtime-benchmarks"))]
+	fn execute_with_unstable<E: BorrowMut<MockExt>>(
+		wat: &str,
+		input_data: Vec<u8>,
+		ext: E,
+	) -> ExecResult {
+		execute_internal(wat, input_data, ext, true)
+	}
+
 	const CODE_TRANSFER: &str = r#"
 (module
 	;; seal_transfer(
@@ -741,7 +761,6 @@ mod tests {
 	}
 
 	#[test]
-	#[cfg(feature = "unstable-interface")]
 	fn contract_delegate_call() {
 		const CODE: &str = r#"
 (module
@@ -2274,7 +2293,6 @@ mod tests {
 		);
 	}
 
-	#[cfg(feature = "unstable-interface")]
 	const CODE_CALL_RUNTIME: &str = r#"
 (module
 	(import "seal0" "call_runtime" (func $call_runtime (param i32 i32) (result i32)))
@@ -2311,7 +2329,6 @@ mod tests {
 "#;
 
 	#[test]
-	#[cfg(feature = "unstable-interface")]
 	fn call_runtime_works() {
 		let call =
 			RuntimeCall::System(frame_system::Call::remark { remark: b"Hello World".to_vec() });
@@ -2323,7 +2340,6 @@ mod tests {
 	}
 
 	#[test]
-	#[cfg(feature = "unstable-interface")]
 	fn call_runtime_panics_on_invalid_call() {
 		let mut ext = MockExt::default();
 		let result = execute(CODE_CALL_RUNTIME, vec![0x42], &mut ext);
@@ -2586,7 +2602,6 @@ mod tests {
 	}
 
 	#[test]
-	#[cfg(feature = "unstable-interface")]
 	fn take_storage_works() {
 		const CODE: &str = r#"
 (module
@@ -2822,7 +2837,6 @@ mod tests {
 	}
 
 	#[test]
-	#[cfg(feature = "unstable-interface")]
 	fn caller_is_origin_works() {
 		const CODE_CALLER_IS_ORIGIN: &str = r#"
 ;; This runs `caller_is_origin` check on zero account address
@@ -2894,7 +2908,6 @@ mod tests {
 	}
 
 	#[test]
-	#[cfg(feature = "unstable-interface")]
 	fn reentrance_count_works() {
 		const CODE: &str = r#"
 (module
@@ -2927,7 +2940,6 @@ mod tests {
 	}
 
 	#[test]
-	#[cfg(feature = "unstable-interface")]
 	fn account_reentrance_count_works() {
 		const CODE: &str = r#"
 (module
@@ -2958,4 +2970,24 @@ mod tests {
 		let mut mock_ext = MockExt::default();
 		execute(CODE, vec![], &mut mock_ext).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.
+	#[cfg(not(feature = "runtime-benchmarks"))]
+	#[test]
+	fn cannot_deploy_unstable() {
+		const CANNT_DEPLOY_UNSTABLE: &str = r#"
+(module
+	(import "seal0" "reentrance_count" (func $reentrance_count (result i32)))
+	(func (export "call"))
+	(func (export "deploy"))
+)
+"#;
+		assert_err!(
+			execute(CANNT_DEPLOY_UNSTABLE, vec![], MockExt::default()),
+			<Error<Test>>::CodeRejected,
+		);
+		assert_ok!(execute_with_unstable(CANNT_DEPLOY_UNSTABLE, vec![], MockExt::default()));
+	}
 }

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

@@ -46,6 +46,7 @@ pub trait Environment<HostState> {
 	fn define(
 		store: &mut Store<HostState>,
 		linker: &mut Linker<HostState>,
+		allow_unstable: bool,
 	) -> Result<(), LinkerError>;
 }
 
@@ -105,7 +106,6 @@ pub enum ReturnCode {
 	/// recording was disabled.
 	LoggingDisabled = 9,
 	/// The call dispatched by `seal_call_runtime` was executed but returned an error.
-	#[cfg(feature = "unstable-interface")]
 	CallRuntimeReturnedError = 10,
 	/// ECDSA pubkey recovery failed (most probably wrong recovery id or signature), or
 	/// ECDSA compressed pubkey conversion into Ethereum address failed (most probably
@@ -228,7 +228,6 @@ pub enum RuntimeCosts {
 	/// Weight of calling `seal_get_storage` with the specified size in storage.
 	GetStorage(u32),
 	/// Weight of calling `seal_take_storage` for the given size.
-	#[cfg(feature = "unstable-interface")]
 	TakeStorage(u32),
 	/// Weight of calling `seal_transfer`.
 	Transfer,
@@ -257,17 +256,14 @@ pub enum RuntimeCosts {
 	/// Weight charged by a chain extension through `seal_call_chain_extension`.
 	ChainExtension(u64),
 	/// Weight charged for calling into the runtime.
-	#[cfg(feature = "unstable-interface")]
 	CallRuntime(Weight),
 	/// Weight of calling `seal_set_code_hash`
 	SetCodeHash,
 	/// Weight of calling `ecdsa_to_eth_address`
 	EcdsaToEthAddress,
 	/// Weight of calling `reentrance_count`
-	#[cfg(feature = "unstable-interface")]
 	ReentrantCount,
 	/// Weight of calling `account_reentrance_count`
-	#[cfg(feature = "unstable-interface")]
 	AccountEntranceCount,
 }
 
@@ -316,7 +312,6 @@ impl RuntimeCosts {
 				.saturating_add(s.contains_storage_per_byte.saturating_mul(len.into())),
 			GetStorage(len) =>
 				s.get_storage.saturating_add(s.get_storage_per_byte.saturating_mul(len.into())),
-			#[cfg(feature = "unstable-interface")]
 			TakeStorage(len) => s
 				.take_storage
 				.saturating_add(s.take_storage_per_byte.saturating_mul(len.into())),
@@ -344,13 +339,10 @@ impl RuntimeCosts {
 				.saturating_add(s.hash_blake2_128_per_byte.saturating_mul(len.into())),
 			EcdsaRecovery => s.ecdsa_recover,
 			ChainExtension(amount) => amount,
-			#[cfg(feature = "unstable-interface")]
 			CallRuntime(weight) => weight.ref_time(),
 			SetCodeHash => s.set_code_hash,
 			EcdsaToEthAddress => s.ecdsa_to_eth_address,
-			#[cfg(feature = "unstable-interface")]
 			ReentrantCount => s.reentrance_count,
-			#[cfg(feature = "unstable-interface")]
 			AccountEntranceCount => s.account_reentrance_count,
 		};
 		RuntimeToken {