From 982f5998c59bd2bd455808345ae1bd2b1767f353 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Wed, 30 Nov 2022 14:19:14 +0000 Subject: [PATCH] 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 Co-authored-by: Sasha Gryaznov --- bin/node/runtime/Cargo.toml | 3 -- bin/node/runtime/src/lib.rs | 3 +- frame/contracts/Cargo.toml | 4 -- frame/contracts/README.md | 17 ++----- frame/contracts/proc-macro/src/lib.rs | 39 +++++++------- frame/contracts/src/benchmarking/sandbox.rs | 2 +- frame/contracts/src/lib.rs | 14 ++++++ frame/contracts/src/tests.rs | 12 +++-- frame/contracts/src/wasm/mod.rs | 56 ++++++++++++++++----- frame/contracts/src/wasm/runtime.rs | 10 +--- 10 files changed, 94 insertions(+), 66 deletions(-) diff --git a/bin/node/runtime/Cargo.toml b/bin/node/runtime/Cargo.toml index dcc59ce750934..f812cbe030c86 100644 --- a/bin/node/runtime/Cargo.toml +++ b/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. diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index f284aaa3a69a8..215b02bcca994 100644 --- a/bin/node/runtime/src/lib.rs +++ b/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; } impl pallet_sudo::Config for Runtime { diff --git a/frame/contracts/Cargo.toml b/frame/contracts/Cargo.toml index fead0a414442f..3906e8589c116 100644 --- a/frame/contracts/Cargo.toml +++ b/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 = [] diff --git a/frame/contracts/README.md b/frame/contracts/README.md index 6b8e62e840b07..4df7d9449682d 100644 --- a/frame/contracts/README.md +++ b/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 -this crate with the `unstable-interface` feature enabled. It should be obvious that any -live runtime should never be compiled with this feature: In addition to be subject to -change or removal those interfaces do not have proper weights associated with 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 -``` +In order to access interfaces marked as `#[unstable]` in `runtime.rs` one need to set +`pallet_contracts::Config::UnsafeUnstableInterface` to `ConstU32`. 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 considered unsafe. New interfaces are generally added as unstable and might go through several iterations before they are promoted to a stable interface. diff --git a/frame/contracts/proc-macro/src/lib.rs b/frame/contracts/proc-macro/src/lib.rs index 5f08b2a9d3081..82b5b728a73ee 100644 --- a/frame/contracts/proc-macro/src/lib.rs +++ b/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 { ::AccountId: ::sp_core::crypto::UncheckedFrom<::Hash> + ::core::convert::AsRef<[::core::primitive::u8]>, { - 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> { #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 { - true => quote! { #[cfg(feature = "unstable-interface")] }, - false => quote! {}, - }; + let is_stable = f.is_stable; // 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 - #allow_unused - linker.define(#module, #name, ::wasmi::Func::wrap(&mut*store, |mut __caller__: ::wasmi::Caller<#host_state>, #( #params, )*| -> #wasm_output { - let mut func = #inner; - func() - .map_err(#map_err) - .map(::core::convert::Into::into) - }))?; + // We need to allow unstable functions when runtime benchmarks are performed because + // we generate the weights even when those interfaces are not enabled. + if ::core::cfg!(feature = "runtime-benchmarks") || #is_stable || allow_unstable { + #allow_unused + linker.define(#module, #name, ::wasmi::Func::wrap(&mut*store, |mut __caller__: ::wasmi::Caller<#host_state>, #( #params, )*| -> #wasm_output { + let mut func = #inner; + func() + .map_err(#map_err) + .map(::core::convert::Into::into) + }))?; + } } }); quote! { diff --git a/frame/contracts/src/benchmarking/sandbox.rs b/frame/contracts/src/benchmarking/sandbox.rs index b0cb9297d5656..a35aad3500109 100644 --- a/frame/contracts/src/benchmarking/sandbox.rs +++ b/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(()) } } diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 00b0655ea4af6..4bbb311313d61 100644 --- a/frame/contracts/src/lib.rs +++ b/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; /// The maximum allowable length in bytes for storage keys. + #[pallet::constant] type MaxStorageKeyLen: Get; + + /// 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; } #[pallet::hooks] diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index e7b27ed38e271..e5395c73d2868 100644 --- a/frame/contracts/src/tests.rs +++ b/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 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::("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::("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::("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::("account_reentrance_count_call").unwrap(); let (wasm_reentrance_count, code_hash_reentrance_count) = diff --git a/frame/contracts/src/wasm/mod.rs b/frame/contracts/src/wasm/mod.rs index 6eaf0d37bef07..ac338007e5dc9 100644 --- a/frame/contracts/src/wasm/mod.rs +++ b/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>(wat: &str, input_data: Vec, mut ext: E) -> ExecResult { + fn execute_internal>( + wat: &str, + input_data: Vec, + mut ext: E, + unstable_interface: bool, + ) -> ExecResult { + type RuntimeConfig = ::T; + RuntimeConfig::set_unstable_interface(unstable_interface); let wasm = wat::parse_str(wat).unwrap(); let schedule = crate::Schedule::default(); - let executable = PrefabWasmModule::<::T>::from_code( + let executable = PrefabWasmModule::::from_code( wasm, &schedule, ALICE, @@ -641,6 +648,19 @@ mod tests { executable.execute(ext.borrow_mut(), &ExportedFunction::Call, input_data) } + fn execute>(wat: &str, input_data: Vec, ext: E) -> ExecResult { + execute_internal(wat, input_data, ext, false) + } + + #[cfg(not(feature = "runtime-benchmarks"))] + fn execute_with_unstable>( + wat: &str, + input_data: Vec, + 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()), + >::CodeRejected, + ); + assert_ok!(execute_with_unstable(CANNT_DEPLOY_UNSTABLE, vec![], MockExt::default())); + } } diff --git a/frame/contracts/src/wasm/runtime.rs b/frame/contracts/src/wasm/runtime.rs index 4c6006d2612fe..988bb224f2a6c 100644 --- a/frame/contracts/src/wasm/runtime.rs +++ b/frame/contracts/src/wasm/runtime.rs @@ -46,6 +46,7 @@ pub trait Environment { fn define( store: &mut Store, linker: &mut Linker, + 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 {