From 47029209551bec1322228aa6b6e62ce145fd6bd8 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Sat, 9 Dec 2023 08:56:28 +0100 Subject: [PATCH 01/16] initial calls in frame system --- substrate/frame/system/src/lib.rs | 133 ++++++++++++++++++++++++-- substrate/frame/system/src/weights.rs | 14 +++ 2 files changed, 137 insertions(+), 10 deletions(-) diff --git a/substrate/frame/system/src/lib.rs b/substrate/frame/system/src/lib.rs index 6624bb43a80d..603e4e7d05b9 100644 --- a/substrate/frame/system/src/lib.rs +++ b/substrate/frame/system/src/lib.rs @@ -77,6 +77,10 @@ use sp_runtime::{ Hash, Header, Lookup, LookupError, MaybeDisplay, MaybeSerializeDeserialize, Member, One, Saturating, SimpleBitOps, StaticLookup, Zero, }, + transaction_validity::{ + InvalidTransaction, TransactionLongevity, TransactionSource, TransactionValidity, + ValidTransaction, + }, DispatchError, RuntimeDebug, }; #[cfg(any(feature = "std", test))] @@ -90,9 +94,9 @@ use frame_support::traits::BuildGenesisConfig; use frame_support::{ dispatch::{ extract_actual_pays_fee, extract_actual_weight, DispatchClass, DispatchInfo, - DispatchResult, DispatchResultWithPostInfo, PerDispatchClass, + DispatchResult, DispatchResultWithPostInfo, PerDispatchClass, PostDispatchInfo, }, - impl_ensure_origin_with_arg_ignoring_arg, + ensure, impl_ensure_origin_with_arg_ignoring_arg, storage::{self, StorageStreamIter}, traits::{ ConstU32, Contains, EnsureOrigin, EnsureOriginWithArg, Get, HandleLifetime, @@ -198,6 +202,20 @@ impl, MaxOverflow: Get> ConsumerLimits for (MaxNormal, } } +/// Information needed when a new runtime binary is submitted and needs to be authorized before +/// replacing the current runtime. +#[derive(Decode, Encode, Default, PartialEq, Eq, MaxEncodedLen, TypeInfo)] +#[scale_info(skip_type_params(T))] +struct CodeUpgradeAuthorization +where + T: Config, +{ + /// Hash of the new runtime binary. + code_hash: T::Hash, + /// Whether or not to carry out version checks. + check_version: bool, +} + #[frame_support::pallet] pub mod pallet { use crate::{self as frame_system, pallet_prelude::*, *}; @@ -658,29 +676,86 @@ pub mod pallet { // Return success. Ok(().into()) } + + /// Authorize new runtime code. + #[pallet::call_index(9)] + #[pallet::weight((T::SystemWeightInfo::authorize_upgrade(), DispatchClass::Operational))] + pub fn authorize_upgrade( + origin: OriginFor, + code_hash: T::Hash, + check_version: bool, + ) -> DispatchResult { + ensure_root(origin)?; + AuthorizedUpgrade::::put(CodeUpgradeAuthorization { code_hash, check_version }); + Self::deposit_event(Event::UpgradeAuthorized { code_hash, check_version }); + Ok(()) + } + + /// Set new, authorized runtime code. + #[pallet::call_index(10)] + #[pallet::weight((T::SystemWeightInfo::apply_authorized_upgrade(), DispatchClass::Operational))] + pub fn apply_authorized_upgrade( + _: OriginFor, + code: Vec, + ) -> DispatchResultWithPostInfo { + Self::validate_authorized_upgrade(&code[..])?; + T::OnSetCode::set_code(code)?; + AuthorizedUpgrade::::kill(); + let post = PostDispatchInfo { + // consume the rest of the block to prevent further transactions + actual_weight: Some(T::BlockWeights::get().max_block), + // no fee for valid upgrade + pays_fee: Pays::No, + }; + Ok(post) + } } /// Event for the System pallet. #[pallet::event] pub enum Event { /// An extrinsic completed successfully. - ExtrinsicSuccess { dispatch_info: DispatchInfo }, + ExtrinsicSuccess { + dispatch_info: DispatchInfo, + }, /// An extrinsic failed. - ExtrinsicFailed { dispatch_error: DispatchError, dispatch_info: DispatchInfo }, + ExtrinsicFailed { + dispatch_error: DispatchError, + dispatch_info: DispatchInfo, + }, /// `:code` was updated. CodeUpdated, /// A new account was created. - NewAccount { account: T::AccountId }, + NewAccount { + account: T::AccountId, + }, /// An account was reaped. - KilledAccount { account: T::AccountId }, + KilledAccount { + account: T::AccountId, + }, /// On on-chain remark happened. - Remarked { sender: T::AccountId, hash: T::Hash }, + Remarked { + sender: T::AccountId, + hash: T::Hash, + }, /// A [`Task`] has started executing - TaskStarted { task: T::RuntimeTask }, + TaskStarted { + task: T::RuntimeTask, + }, /// A [`Task`] has finished executing. - TaskCompleted { task: T::RuntimeTask }, + TaskCompleted { + task: T::RuntimeTask, + }, /// A [`Task`] failed during execution. - TaskFailed { task: T::RuntimeTask, err: DispatchError }, + TaskFailed { + task: T::RuntimeTask, + err: DispatchError, + }, + // Upgrade authorized. + UpgradeAuthorized { + code_hash: T::Hash, + check_version: bool, + }, } /// Error for the System pallet @@ -706,6 +781,10 @@ pub mod pallet { InvalidTask, /// The specified [`Task`] failed during execution. FailedTask, + /// No upgrade authorized. + NothingAuthorized, + /// The submitted code is not authorized. + Unauthorized, } /// Exposed trait-generic origin type. @@ -821,6 +900,11 @@ pub mod pallet { #[pallet::whitelist_storage] pub(super) type ExecutionPhase = StorageValue<_, Phase>; + /// `Some` if a code upgrade has been authorized. + #[pallet::storage] + pub(super) type AuthorizedUpgrade = + StorageValue<_, CodeUpgradeAuthorization, OptionQuery>; + #[derive(frame_support::DefaultNoBound)] #[pallet::genesis_config] pub struct GenesisConfig { @@ -840,6 +924,25 @@ pub mod pallet { sp_io::storage::set(well_known_keys::EXTRINSIC_INDEX, &0u32.encode()); } } + + #[pallet::validate_unsigned] + impl sp_runtime::traits::ValidateUnsigned for Pallet { + type Call = Call; + fn validate_unsigned(_source: TransactionSource, call: &Self::Call) -> TransactionValidity { + if let Call::apply_authorized_upgrade { ref code } = call { + if let Ok(hash) = Self::validate_authorized_upgrade(&code[..]) { + return Ok(ValidTransaction { + priority: 100, + requires: Vec::new(), + provides: vec![hash.as_ref().to_vec()], + longevity: TransactionLongevity::max_value(), + propagate: true, + }) + } + } + Err(InvalidTransaction::Call.into()) + } + } } pub type Key = Vec; @@ -1864,6 +1967,16 @@ impl Pallet { } } } + + fn validate_authorized_upgrade(code: &[u8]) -> Result { + let authorization = AuthorizedUpgrade::::get().ok_or(Error::::NothingAuthorized)?; + let actual_hash = T::Hashing::hash(code); + ensure!(actual_hash == authorization.code_hash, Error::::Unauthorized); + if authorization.check_version { + Self::can_set_code(code)? + } + Ok(actual_hash) + } } /// Returns a 32 byte datum which is guaranteed to be universally unique. `entropy` is provided diff --git a/substrate/frame/system/src/weights.rs b/substrate/frame/system/src/weights.rs index b79db3654b9f..4d2ff45dd1cf 100644 --- a/substrate/frame/system/src/weights.rs +++ b/substrate/frame/system/src/weights.rs @@ -57,6 +57,8 @@ pub trait WeightInfo { fn set_storage(i: u32, ) -> Weight; fn kill_storage(i: u32, ) -> Weight; fn kill_prefix(p: u32, ) -> Weight; + fn authorize_upgrade() -> Weight; + fn apply_authorized_upgrade() -> Weight; } /// Weights for frame_system using the Substrate node and recommended hardware. @@ -149,6 +151,12 @@ impl WeightInfo for SubstrateWeight { .saturating_add(T::DbWeight::get().writes((1_u64).saturating_mul(p.into()))) .saturating_add(Weight::from_parts(0, 70).saturating_mul(p.into())) } + fn authorize_upgrade() -> Weight { + Weight::zero() + } + fn apply_authorized_upgrade() -> Weight { + Weight::zero() + } } // For backwards compatibility and tests @@ -240,4 +248,10 @@ impl WeightInfo for () { .saturating_add(RocksDbWeight::get().writes((1_u64).saturating_mul(p.into()))) .saturating_add(Weight::from_parts(0, 70).saturating_mul(p.into())) } + fn authorize_upgrade() -> Weight { + Weight::zero() + } + fn apply_authorized_upgrade() -> Weight { + Weight::zero() + } } From 0784f4fd9afa8839bd322f424f44fb23f057f102 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Sat, 9 Dec 2023 09:07:04 +0100 Subject: [PATCH 02/16] remove from parachain-system --- cumulus/pallets/parachain-system/src/lib.rs | 100 +------------------- 1 file changed, 4 insertions(+), 96 deletions(-) diff --git a/cumulus/pallets/parachain-system/src/lib.rs b/cumulus/pallets/parachain-system/src/lib.rs index bac1ee28a7ca..a506ff671abc 100644 --- a/cumulus/pallets/parachain-system/src/lib.rs +++ b/cumulus/pallets/parachain-system/src/lib.rs @@ -27,7 +27,7 @@ //! //! Users must ensure that they register this pallet as an inherent provider. -use codec::{Decode, Encode, MaxEncodedLen}; +use codec::{Decode, Encode}; use cumulus_primitives_core::{ relay_chain, AbridgedHostConfiguration, ChannelInfo, ChannelStatus, CollationInfo, GetChannelInfo, InboundDownwardMessage, InboundHrmpMessage, MessageSendError, @@ -49,11 +49,8 @@ use polkadot_runtime_parachains::FeeTracker; use scale_info::TypeInfo; use sp_runtime::{ traits::{Block as BlockT, BlockNumberProvider, Hash}, - transaction_validity::{ - InvalidTransaction, TransactionLongevity, TransactionSource, TransactionValidity, - ValidTransaction, - }, - BoundedSlice, DispatchError, FixedU128, RuntimeDebug, Saturating, + transaction_validity::{InvalidTransaction, TransactionSource, TransactionValidity}, + BoundedSlice, FixedU128, RuntimeDebug, Saturating, }; use sp_std::{cmp, collections::btree_map::BTreeMap, prelude::*}; use xcm::latest::XcmHash; @@ -169,20 +166,6 @@ impl CheckAssociatedRelayNumber for RelayNumberMonotonicallyIncreases { } } -/// Information needed when a new runtime binary is submitted and needs to be authorized before -/// replacing the current runtime. -#[derive(Decode, Encode, Default, PartialEq, Eq, MaxEncodedLen, TypeInfo)] -#[scale_info(skip_type_params(T))] -struct CodeUpgradeAuthorization -where - T: Config, -{ - /// Hash of the new runtime binary. - code_hash: T::Hash, - /// Whether or not to carry out version checks. - check_version: bool, -} - /// The max length of a DMP message. pub type MaxDmpMessageLenOf = <::DmpQueue as HandleMessage>::MaxMessageLen; @@ -667,49 +650,6 @@ pub mod pallet { let _ = Self::send_upward_message(message); Ok(()) } - - /// Authorize an upgrade to a given `code_hash` for the runtime. The runtime can be supplied - /// later. - /// - /// The `check_version` parameter sets a boolean flag for whether or not the runtime's spec - /// version and name should be verified on upgrade. Since the authorization only has a hash, - /// it cannot actually perform the verification. - /// - /// This call requires Root origin. - #[pallet::call_index(2)] - #[pallet::weight((1_000_000, DispatchClass::Operational))] - pub fn authorize_upgrade( - origin: OriginFor, - code_hash: T::Hash, - check_version: bool, - ) -> DispatchResult { - ensure_root(origin)?; - AuthorizedUpgrade::::put(CodeUpgradeAuthorization { code_hash, check_version }); - - Self::deposit_event(Event::UpgradeAuthorized { code_hash }); - Ok(()) - } - - /// Provide the preimage (runtime binary) `code` for an upgrade that has been authorized. - /// - /// If the authorization required a version check, this call will ensure the spec name - /// remains unchanged and that the spec version has increased. - /// - /// Note that this function will not apply the new `code`, but only attempt to schedule the - /// upgrade with the Relay Chain. - /// - /// All origins are allowed. - #[pallet::call_index(3)] - #[pallet::weight({1_000_000})] - pub fn enact_authorized_upgrade( - _: OriginFor, - code: Vec, - ) -> DispatchResultWithPostInfo { - Self::validate_authorized_upgrade(&code[..])?; - Self::schedule_code_upgrade(code)?; - AuthorizedUpgrade::::kill(); - Ok(Pays::No.into()) - } } #[pallet::event] @@ -721,8 +661,6 @@ pub mod pallet { ValidationFunctionApplied { relay_chain_block_num: RelayChainBlockNumber }, /// The relay-chain aborted the upgrade process. ValidationFunctionDiscarded, - /// An upgrade has been authorized. - UpgradeAuthorized { code_hash: T::Hash }, /// Some downward messages have been received and will be processed. DownwardMessagesReceived { count: u32 }, /// Downward messages were processed using the given weight. @@ -928,10 +866,6 @@ pub mod pallet { #[pallet::storage] pub(super) type ReservedDmpWeightOverride = StorageValue<_, Weight>; - /// The next authorized upgrade, if there is one. - #[pallet::storage] - pub(super) type AuthorizedUpgrade = StorageValue<_, CodeUpgradeAuthorization>; - /// A custom head data that should be returned as result of `validate_block`. /// /// See `Pallet::set_custom_validation_head_data` for more information. @@ -981,17 +915,6 @@ pub mod pallet { type Call = Call; fn validate_unsigned(_source: TransactionSource, call: &Self::Call) -> TransactionValidity { - if let Call::enact_authorized_upgrade { ref code } = call { - if let Ok(hash) = Self::validate_authorized_upgrade(code) { - return Ok(ValidTransaction { - priority: 100, - requires: Vec::new(), - provides: vec![hash.as_ref().to_vec()], - longevity: TransactionLongevity::max_value(), - propagate: true, - }) - } - } if let Call::set_validation_data { .. } = call { return Ok(Default::default()) } @@ -1001,21 +924,6 @@ pub mod pallet { } impl Pallet { - fn validate_authorized_upgrade(code: &[u8]) -> Result { - let authorization = AuthorizedUpgrade::::get().ok_or(Error::::NothingAuthorized)?; - - // ensure that the actual hash matches the authorized hash - let actual_hash = T::Hashing::hash(code); - ensure!(actual_hash == authorization.code_hash, Error::::Unauthorized); - - // check versions if required as part of the authorization - if authorization.check_version { - frame_system::Pallet::::can_set_code(code)?; - } - - Ok(actual_hash) - } - /// Get the unincluded segment size after the given hash. /// /// If the unincluded segment doesn't contain the given hash, this returns the @@ -1563,8 +1471,8 @@ impl Pallet { } } +/// Type that implements `SetCode` pub struct ParachainSetCode(sp_std::marker::PhantomData); - impl frame_system::SetCode for ParachainSetCode { fn set_code(code: Vec) -> DispatchResult { Pallet::::schedule_code_upgrade(code) From 74586727f35e9025a51acbf9381c4a327accdcb6 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Sun, 10 Dec 2023 17:30:15 +0100 Subject: [PATCH 03/16] add benchmarks and a test --- .../frame/system/benchmarking/src/lib.rs | 153 +++++++++++++----- substrate/frame/system/src/lib.rs | 9 +- substrate/frame/system/src/tests.rs | 24 +++ 3 files changed, 141 insertions(+), 45 deletions(-) diff --git a/substrate/frame/system/benchmarking/src/lib.rs b/substrate/frame/system/benchmarking/src/lib.rs index d85b631af018..e430c9940054 100644 --- a/substrate/frame/system/benchmarking/src/lib.rs +++ b/substrate/frame/system/benchmarking/src/lib.rs @@ -21,10 +21,7 @@ #![cfg(feature = "runtime-benchmarks")] use codec::Encode; -use frame_benchmarking::{ - v1::{benchmarks, whitelisted_caller}, - BenchmarkError, -}; +use frame_benchmarking::{impl_benchmark_test_suite, v2::*}; use frame_support::{dispatch::DispatchClass, storage, traits::Get}; use frame_system::{Call, Pallet as System, RawOrigin}; use sp_core::storage::well_known_keys; @@ -55,69 +52,104 @@ pub trait Config: frame_system::Config { } } -benchmarks! { - remark { - let b in 0 .. *T::BlockLength::get().max.get(DispatchClass::Normal) as u32; +#[benchmarks] +mod benchmarks { + use super::*; + + #[benchmark] + fn remark( + b: Linear<0, { *T::BlockLength::get().max.get(DispatchClass::Normal) as u32 }>, + ) -> Result<(), BenchmarkError> { let remark_message = vec![1; b as usize]; let caller = whitelisted_caller(); - }: _(RawOrigin::Signed(caller), remark_message) - remark_with_event { - let b in 0 .. *T::BlockLength::get().max.get(DispatchClass::Normal) as u32; + #[extrinsic_call] + remark(RawOrigin::Signed(caller), remark_message); + + Ok(()) + } + + #[benchmark] + fn remark_with_event( + b: Linear<0, { *T::BlockLength::get().max.get(DispatchClass::Normal) as u32 }>, + ) -> Result<(), BenchmarkError> { let remark_message = vec![1; b as usize]; - let caller = whitelisted_caller(); - }: _(RawOrigin::Signed(caller), remark_message) + let caller: T::AccountId = whitelisted_caller(); + let hash = T::Hashing::hash(&remark_message[..]); - set_heap_pages { - }: _(RawOrigin::Root, Default::default()) + #[extrinsic_call] + remark_with_event(RawOrigin::Signed(caller.clone()), remark_message); - set_code { + System::::assert_last_event( + frame_system::Event::::Remarked { sender: caller, hash }.into(), + ); + Ok(()) + } + + #[benchmark] + fn set_heap_pages() -> Result<(), BenchmarkError> { + #[extrinsic_call] + set_heap_pages(RawOrigin::Root, Default::default()); + + Ok(()) + } + + #[benchmark] + fn set_code() -> Result<(), BenchmarkError> { let runtime_blob = T::prepare_set_code_data(); T::setup_set_code_requirements(&runtime_blob)?; - }: _(RawOrigin::Root, runtime_blob) - verify { - T::verify_set_code() + + #[extrinsic_call] + set_code(RawOrigin::Root, runtime_blob); + + T::verify_set_code(); + Ok(()) } - #[extra] - set_code_without_checks { + #[benchmark(extra)] + fn set_code_without_checks() -> Result<(), BenchmarkError> { // Assume Wasm ~4MB let code = vec![1; 4_000_000 as usize]; T::setup_set_code_requirements(&code)?; - }: _(RawOrigin::Root, code) - verify { - let current_code = storage::unhashed::get_raw(well_known_keys::CODE).ok_or("Code not stored.")?; + + #[block] + { + System::::set_code_without_checks(RawOrigin::Root.into(), code)?; + } + + let current_code = + storage::unhashed::get_raw(well_known_keys::CODE).ok_or("Code not stored.")?; assert_eq!(current_code.len(), 4_000_000 as usize); + Ok(()) } - #[skip_meta] - set_storage { - let i in 0 .. 1000; - + #[benchmark(skip_meta)] + fn set_storage(i: Linear<0, { 1_000 }>) -> Result<(), BenchmarkError> { // Set up i items to add let mut items = Vec::new(); - for j in 0 .. i { + for j in 0..i { let hash = (i, j).using_encoded(T::Hashing::hash).as_ref().to_vec(); items.push((hash.clone(), hash.clone())); } let items_to_verify = items.clone(); - }: _(RawOrigin::Root, items) - verify { + + #[extrinsic_call] + set_storage(RawOrigin::Root, items); + // Verify that they're actually in the storage. for (item, _) in items_to_verify { let value = storage::unhashed::get_raw(&item).ok_or("No value stored")?; assert_eq!(value, *item); } + Ok(()) } - #[skip_meta] - kill_storage { - let i in 0 .. 1000; - + #[benchmark(skip_meta)] + fn kill_storage(i: Linear<0, { 1_000 }>) -> Result<(), BenchmarkError> { // Add i items to storage let mut items = Vec::with_capacity(i as usize); - for j in 0 .. i { + for j in 0..i { let hash = (i, j).using_encoded(T::Hashing::hash).as_ref().to_vec(); storage::unhashed::put_raw(&hash, &hash); items.push(hash); @@ -130,22 +162,23 @@ benchmarks! { } let items_to_verify = items.clone(); - }: _(RawOrigin::Root, items) - verify { + + #[extrinsic_call] + kill_storage(RawOrigin::Root, items); + // Verify that they're not in the storage anymore. for item in items_to_verify { assert!(storage::unhashed::get_raw(&item).is_none()); } + Ok(()) } - #[skip_meta] - kill_prefix { - let p in 0 .. 1000; - + #[benchmark(skip_meta)] + fn kill_prefix(p: Linear<0, { 1_000 }>) -> Result<(), BenchmarkError> { let prefix = p.using_encoded(T::Hashing::hash).as_ref().to_vec(); let mut items = Vec::with_capacity(p as usize); // add p items that share a prefix - for i in 0 .. p { + for i in 0..p { let hash = (p, i).using_encoded(T::Hashing::hash).as_ref().to_vec(); let key = [&prefix[..], &hash[..]].concat(); storage::unhashed::put_raw(&key, &key); @@ -157,12 +190,44 @@ benchmarks! { let value = storage::unhashed::get_raw(item).ok_or("No value stored")?; assert_eq!(value, *item); } - }: _(RawOrigin::Root, prefix, p) - verify { + + #[extrinsic_call] + kill_prefix(RawOrigin::Root, prefix, p); + // Verify that they're not in the storage anymore. for item in items { assert!(storage::unhashed::get_raw(&item).is_none()); } + Ok(()) + } + + #[benchmark] + fn authorize_upgrade() -> Result<(), BenchmarkError> { + let runtime_blob = T::prepare_set_code_data(); + T::setup_set_code_requirements(&runtime_blob)?; + let hash = T::Hashing::hash(&runtime_blob); + + #[extrinsic_call] + authorize_upgrade(RawOrigin::Root, hash, true); + + assert!(System::::authorized_upgrade().is_some()); + Ok(()) + } + + #[benchmark] + fn apply_authorized_upgrade() -> Result<(), BenchmarkError> { + let runtime_blob = T::prepare_set_code_data(); + T::setup_set_code_requirements(&runtime_blob)?; + let hash = T::Hashing::hash(&runtime_blob); + System::::authorize_upgrade(RawOrigin::Root.into(), hash, true)?; + + #[extrinsic_call] + apply_authorized_upgrade(RawOrigin::Root, runtime_blob); + + // Can't check for `CodeUpdated` in parachain upgrades. Just check that the authorization is + // gone. + assert!(System::::authorized_upgrade().is_none()); + Ok(()) } impl_benchmark_test_suite!(Pallet, crate::mock::new_test_ext(), crate::mock::Test); diff --git a/substrate/frame/system/src/lib.rs b/substrate/frame/system/src/lib.rs index 603e4e7d05b9..9b055f204952 100644 --- a/substrate/frame/system/src/lib.rs +++ b/substrate/frame/system/src/lib.rs @@ -206,7 +206,7 @@ impl, MaxOverflow: Get> ConsumerLimits for (MaxNormal, /// replacing the current runtime. #[derive(Decode, Encode, Default, PartialEq, Eq, MaxEncodedLen, TypeInfo)] #[scale_info(skip_type_params(T))] -struct CodeUpgradeAuthorization +pub struct CodeUpgradeAuthorization where T: Config, { @@ -1968,6 +1968,13 @@ impl Pallet { } } + /// Return an upgrade authorization, if it exists. + pub fn authorized_upgrade() -> Option> { + AuthorizedUpgrade::::get() + } + + /// Check that provided `code` can be upgraded to. Namely, check that its hash matches an + /// existing authorization and that it meets the specification requirements of `can_set_code`. fn validate_authorized_upgrade(code: &[u8]) -> Result { let authorization = AuthorizedUpgrade::::get().ok_or(Error::::NothingAuthorized)?; let actual_hash = T::Hashing::hash(code); diff --git a/substrate/frame/system/src/tests.rs b/substrate/frame/system/src/tests.rs index 6fbddaaf2294..b752313f0fd6 100644 --- a/substrate/frame/system/src/tests.rs +++ b/substrate/frame/system/src/tests.rs @@ -675,6 +675,30 @@ fn set_code_with_real_wasm_blob() { }); } +#[test] +fn set_code_via_authorization_works() { + let executor = substrate_test_runtime_client::new_native_or_wasm_executor(); + let mut ext = new_test_ext(); + ext.register_extension(sp_core::traits::ReadRuntimeVersionExt::new(executor)); + ext.execute_with(|| { + System::set_block_number(1); + assert!(System::authorized_upgrade().is_none()); + + let runtime = substrate_test_runtime_client::runtime::wasm_binary_unwrap().to_vec(); + let hash = ::Hashing::hash(&runtime); + assert_ok!(System::authorize_upgrade(RawOrigin::Root.into(), hash, true)); + + System::assert_has_event( + SysEvent::UpgradeAuthorized { code_hash: hash, check_version: true }.into(), + ); + assert!(System::authorized_upgrade().is_some()); + + assert_ok!(System::apply_authorized_upgrade(RawOrigin::None.into(), runtime)); + System::assert_has_event(SysEvent::CodeUpdated.into()); + assert!(System::authorized_upgrade().is_none()); + }); +} + #[test] fn runtime_upgraded_with_set_storage() { let executor = substrate_test_runtime_client::new_native_or_wasm_executor(); From f5c5525dbac642e24dffbdd39f1b797a4c6aa1cc Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Mon, 11 Dec 2023 13:59:27 +0100 Subject: [PATCH 04/16] unbreak parachain-system API --- cumulus/pallets/parachain-system/src/lib.rs | 61 ++++++++++++++++++- cumulus/pallets/parachain-system/src/tests.rs | 5 +- substrate/frame/system/src/lib.rs | 54 +++++++++++----- substrate/frame/system/src/tests.rs | 18 +++++- 4 files changed, 119 insertions(+), 19 deletions(-) diff --git a/cumulus/pallets/parachain-system/src/lib.rs b/cumulus/pallets/parachain-system/src/lib.rs index a506ff671abc..b643e4a79669 100644 --- a/cumulus/pallets/parachain-system/src/lib.rs +++ b/cumulus/pallets/parachain-system/src/lib.rs @@ -49,7 +49,9 @@ use polkadot_runtime_parachains::FeeTracker; use scale_info::TypeInfo; use sp_runtime::{ traits::{Block as BlockT, BlockNumberProvider, Hash}, - transaction_validity::{InvalidTransaction, TransactionSource, TransactionValidity}, + transaction_validity::{ + InvalidTransaction, TransactionSource, TransactionValidity, ValidTransaction, + }, BoundedSlice, FixedU128, RuntimeDebug, Saturating, }; use sp_std::{cmp, collections::btree_map::BTreeMap, prelude::*}; @@ -187,7 +189,7 @@ pub mod ump_constants { pub mod pallet { use super::*; use frame_support::pallet_prelude::*; - use frame_system::pallet_prelude::*; + use frame_system::{pallet_prelude::*, WeightInfo as SystemWeightInfo}; #[pallet::pallet] #[pallet::storage_version(migration::STORAGE_VERSION)] @@ -650,6 +652,49 @@ pub mod pallet { let _ = Self::send_upward_message(message); Ok(()) } + + /// Authorize an upgrade to a given `code_hash` for the runtime. The runtime can be supplied + /// later. + /// + /// The `check_version` parameter sets a boolean flag for whether or not the runtime's spec + /// version and name should be verified on upgrade. Since the authorization only has a hash, + /// it cannot actually perform the verification. + /// + /// This call requires Root origin. + #[pallet::call_index(2)] + #[pallet::weight(::SystemWeightInfo::authorize_upgrade())] + #[allow(deprecated)] + #[deprecated(note = "Migrate to frame_system::authorize_upgrade")] + pub fn authorize_upgrade( + origin: OriginFor, + code_hash: T::Hash, + check_version: bool, + ) -> DispatchResult { + ensure_root(origin)?; + frame_system::Pallet::::do_authorize_upgrade(code_hash, check_version); + Ok(()) + } + + /// Provide the preimage (runtime binary) `code` for an upgrade that has been authorized. + /// + /// If the authorization required a version check, this call will ensure the spec name + /// remains unchanged and that the spec version has increased. + /// + /// Note that this function will not apply the new `code`, but only attempt to schedule the + /// upgrade with the Relay Chain. + /// + /// All origins are allowed. + #[pallet::call_index(3)] + #[pallet::weight(::SystemWeightInfo::apply_authorized_upgrade())] + #[allow(deprecated)] + #[deprecated(note = "Migrate to frame_system::apply_authorized_upgrade")] + pub fn enact_authorized_upgrade( + _: OriginFor, + code: Vec, + ) -> DispatchResultWithPostInfo { + let post = frame_system::Pallet::::do_apply_authorize_upgrade(code)?; + Ok(post) + } } #[pallet::event] @@ -915,6 +960,18 @@ pub mod pallet { type Call = Call; fn validate_unsigned(_source: TransactionSource, call: &Self::Call) -> TransactionValidity { + if let Call::enact_authorized_upgrade { ref code } = call { + if let Ok(hash) = frame_system::Pallet::::validate_authorized_upgrade(&code[..]) + { + return Ok(ValidTransaction { + priority: 100, + requires: Vec::new(), + provides: vec![hash.as_ref().to_vec()], + longevity: TransactionLongevity::max_value(), + propagate: true, + }) + } + } if let Call::set_validation_data { .. } = call { return Ok(Default::default()) } diff --git a/cumulus/pallets/parachain-system/src/tests.rs b/cumulus/pallets/parachain-system/src/tests.rs index ab1775b40a84..7528d3d9fe8d 100755 --- a/cumulus/pallets/parachain-system/src/tests.rs +++ b/cumulus/pallets/parachain-system/src/tests.rs @@ -1127,8 +1127,9 @@ fn upgrade_version_checks_should_work() { let new_code = vec![1, 2, 3, 4]; let new_code_hash = H256(sp_core::blake2_256(&new_code)); - let _authorize = - ParachainSystem::authorize_upgrade(RawOrigin::Root.into(), new_code_hash, true); + #[allow(deprecated)] + let _authorize = ParachainSystem::authorize_upgrade(RawOrigin::Root.into(), new_code_hash, true); + #[allow(deprecated)] let res = ParachainSystem::enact_authorized_upgrade(RawOrigin::None.into(), new_code); assert_eq!(expected.map_err(DispatchErrorWithPostInfo::from), res); diff --git a/substrate/frame/system/src/lib.rs b/substrate/frame/system/src/lib.rs index 9b055f204952..66b3529d2889 100644 --- a/substrate/frame/system/src/lib.rs +++ b/substrate/frame/system/src/lib.rs @@ -97,6 +97,7 @@ use frame_support::{ DispatchResult, DispatchResultWithPostInfo, PerDispatchClass, PostDispatchInfo, }, ensure, impl_ensure_origin_with_arg_ignoring_arg, + pallet_prelude::Pays, storage::{self, StorageStreamIter}, traits::{ ConstU32, Contains, EnsureOrigin, EnsureOriginWithArg, Get, HandleLifetime, @@ -677,7 +678,14 @@ pub mod pallet { Ok(().into()) } - /// Authorize new runtime code. + /// Authorize an upgrade to a given `code_hash` for the runtime. The runtime can be supplied + /// later. + /// + /// The `check_version` parameter sets a boolean flag for whether or not the runtime's spec + /// version and name should be verified on upgrade. Since the authorization only has a hash, + /// it cannot actually perform the verification. + /// + /// This call requires Root origin. #[pallet::call_index(9)] #[pallet::weight((T::SystemWeightInfo::authorize_upgrade(), DispatchClass::Operational))] pub fn authorize_upgrade( @@ -686,27 +694,26 @@ pub mod pallet { check_version: bool, ) -> DispatchResult { ensure_root(origin)?; - AuthorizedUpgrade::::put(CodeUpgradeAuthorization { code_hash, check_version }); - Self::deposit_event(Event::UpgradeAuthorized { code_hash, check_version }); + Self::do_authorize_upgrade(code_hash, check_version); Ok(()) } - /// Set new, authorized runtime code. + /// Provide the preimage (runtime binary) `code` for an upgrade that has been authorized. + /// + /// If the authorization required a version check, this call will ensure the spec name + /// remains unchanged and that the spec version has increased. + /// + /// Depending on the runtime's `OnSetCode` configuration, this function may directly apply + /// the new `code` in the same block or attempt to schedule the upgrade. + /// + /// All origins are allowed. #[pallet::call_index(10)] #[pallet::weight((T::SystemWeightInfo::apply_authorized_upgrade(), DispatchClass::Operational))] pub fn apply_authorized_upgrade( _: OriginFor, code: Vec, ) -> DispatchResultWithPostInfo { - Self::validate_authorized_upgrade(&code[..])?; - T::OnSetCode::set_code(code)?; - AuthorizedUpgrade::::kill(); - let post = PostDispatchInfo { - // consume the rest of the block to prevent further transactions - actual_weight: Some(T::BlockWeights::get().max_block), - // no fee for valid upgrade - pays_fee: Pays::No, - }; + let post = Self::do_apply_authorize_upgrade(code)?; Ok(post) } } @@ -1968,6 +1975,25 @@ impl Pallet { } } + /// x + pub fn do_authorize_upgrade(code_hash: T::Hash, check_version: bool) { + AuthorizedUpgrade::::put(CodeUpgradeAuthorization { code_hash, check_version }); + Self::deposit_event(Event::UpgradeAuthorized { code_hash, check_version }); + } + + pub fn do_apply_authorize_upgrade(code: Vec) -> Result { + Self::validate_authorized_upgrade(&code[..])?; + T::OnSetCode::set_code(code)?; + AuthorizedUpgrade::::kill(); + let post = PostDispatchInfo { + // consume the rest of the block to prevent further transactions + actual_weight: Some(T::BlockWeights::get().max_block), + // no fee for valid upgrade + pays_fee: Pays::No, + }; + Ok(post) + } + /// Return an upgrade authorization, if it exists. pub fn authorized_upgrade() -> Option> { AuthorizedUpgrade::::get() @@ -1975,7 +2001,7 @@ impl Pallet { /// Check that provided `code` can be upgraded to. Namely, check that its hash matches an /// existing authorization and that it meets the specification requirements of `can_set_code`. - fn validate_authorized_upgrade(code: &[u8]) -> Result { + pub fn validate_authorized_upgrade(code: &[u8]) -> Result { let authorization = AuthorizedUpgrade::::get().ok_or(Error::::NothingAuthorized)?; let actual_hash = T::Hashing::hash(code); ensure!(actual_hash == authorization.code_hash, Error::::Unauthorized); diff --git a/substrate/frame/system/src/tests.rs b/substrate/frame/system/src/tests.rs index b752313f0fd6..fdddc2527fe3 100644 --- a/substrate/frame/system/src/tests.rs +++ b/substrate/frame/system/src/tests.rs @@ -686,13 +686,29 @@ fn set_code_via_authorization_works() { let runtime = substrate_test_runtime_client::runtime::wasm_binary_unwrap().to_vec(); let hash = ::Hashing::hash(&runtime); - assert_ok!(System::authorize_upgrade(RawOrigin::Root.into(), hash, true)); + // Can't apply before authorization + assert_noop!( + System::apply_authorized_upgrade(RawOrigin::None.into(), runtime.clone()), + Error::::NothingAuthorized, + ); + + // Can authorize + assert_ok!(System::authorize_upgrade(RawOrigin::Root.into(), hash, true)); System::assert_has_event( SysEvent::UpgradeAuthorized { code_hash: hash, check_version: true }.into(), ); assert!(System::authorized_upgrade().is_some()); + // Can't be sneaky + let mut bad_runtime = substrate_test_runtime_client::runtime::wasm_binary_unwrap().to_vec(); + bad_runtime.extend(b"sneaky"); + assert_noop!( + System::apply_authorized_upgrade(RawOrigin::None.into(), bad_runtime), + Error::::Unauthorized, + ); + + // Can apply correct runtime assert_ok!(System::apply_authorized_upgrade(RawOrigin::None.into(), runtime)); System::assert_has_event(SysEvent::CodeUpdated.into()); assert!(System::authorized_upgrade().is_none()); From b6482c5abaeec2c9bf7971bd0db795995bd09e68 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Mon, 11 Dec 2023 14:23:15 +0100 Subject: [PATCH 05/16] docs and part migration --- .../pallets/parachain-system/src/migration.rs | 36 +++++++++++++++++++ substrate/frame/system/src/lib.rs | 6 +++- 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/cumulus/pallets/parachain-system/src/migration.rs b/cumulus/pallets/parachain-system/src/migration.rs index a92f85b9cd42..6ccbf06210d1 100644 --- a/cumulus/pallets/parachain-system/src/migration.rs +++ b/cumulus/pallets/parachain-system/src/migration.rs @@ -45,10 +45,46 @@ impl OnRuntimeUpgrade for Migration { StorageVersion::new(2).put::>(); } + // if StorageVersion::get::>() == 2 { + // weight = weight + // .saturating_add(v3::migrate::()) + // .saturating_add(T::DbWeight::get().writes(1)); + // StorageVersion::new(3).put::>(); + // } + weight } } +// todo: how to make it aware of a storage item that doesn't exist anymore? do we just need to keep +// this storage item here for another release cycle, or is is there a better way? +// ``` +// error[E0433]: failed to resolve: use of undeclared type `AuthorizedUpgrade` +// --> cumulus/pallets/parachain-system/src/migration.rs:77:37 +// | +// 77 | if let Some(authorized_upgrade) = AuthorizedUpgrade::::take() { +// | ^^^^^^^^^^^^^^^^^ use of undeclared type `AuthorizedUpgrade` + +// For more information about this error, try `rustc --explain E0433`. +// ``` + +// /// V3: Deprecation of parachain-system's upgrade authorization. +// mod v3 { +// use super::*; + +// pub fn migrate() -> Weight { +// let mut weight = T::DbWeight::get().reads(1); +// if let Some(authorized_upgrade) = AuthorizedUpgrade::::take() { +// frame_system::Pallet::::do_authorize_upgrade( +// authorized_upgrade.code_hash, +// authorized_upgrade.check_version, +// ); +// weight.saturating_add(T::DbWeight::get().writes(1)); +// } +// weight +// } +// } + /// V2: Migrate to 2D weights for ReservedXcmpWeightOverride and ReservedDmpWeightOverride. mod v2 { use super::*; diff --git a/substrate/frame/system/src/lib.rs b/substrate/frame/system/src/lib.rs index 66b3529d2889..371e79a62be0 100644 --- a/substrate/frame/system/src/lib.rs +++ b/substrate/frame/system/src/lib.rs @@ -1975,12 +1975,16 @@ impl Pallet { } } - /// x + /// To be called after any origin/privilege checks. Put the code upgrade authorization into + /// storage and emit an event. Infallible. pub fn do_authorize_upgrade(code_hash: T::Hash, check_version: bool) { AuthorizedUpgrade::::put(CodeUpgradeAuthorization { code_hash, check_version }); Self::deposit_event(Event::UpgradeAuthorized { code_hash, check_version }); } + /// Apply an authorized upgrade, performing any validation checks, and remove the authorization. + /// Whether or not the code is set directly depends on the `OnSetCode` configuration of the + /// runtime. pub fn do_apply_authorize_upgrade(code: Vec) -> Result { Self::validate_authorized_upgrade(&code[..])?; T::OnSetCode::set_code(code)?; From e3314821345885065452ebc78d87c493b324b8cf Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Mon, 11 Dec 2023 15:03:38 +0100 Subject: [PATCH 06/16] no migration (storage will be none after upgrade anyway) --- .../pallets/parachain-system/src/migration.rs | 36 ------------------- 1 file changed, 36 deletions(-) diff --git a/cumulus/pallets/parachain-system/src/migration.rs b/cumulus/pallets/parachain-system/src/migration.rs index 6ccbf06210d1..a92f85b9cd42 100644 --- a/cumulus/pallets/parachain-system/src/migration.rs +++ b/cumulus/pallets/parachain-system/src/migration.rs @@ -45,46 +45,10 @@ impl OnRuntimeUpgrade for Migration { StorageVersion::new(2).put::>(); } - // if StorageVersion::get::>() == 2 { - // weight = weight - // .saturating_add(v3::migrate::()) - // .saturating_add(T::DbWeight::get().writes(1)); - // StorageVersion::new(3).put::>(); - // } - weight } } -// todo: how to make it aware of a storage item that doesn't exist anymore? do we just need to keep -// this storage item here for another release cycle, or is is there a better way? -// ``` -// error[E0433]: failed to resolve: use of undeclared type `AuthorizedUpgrade` -// --> cumulus/pallets/parachain-system/src/migration.rs:77:37 -// | -// 77 | if let Some(authorized_upgrade) = AuthorizedUpgrade::::take() { -// | ^^^^^^^^^^^^^^^^^ use of undeclared type `AuthorizedUpgrade` - -// For more information about this error, try `rustc --explain E0433`. -// ``` - -// /// V3: Deprecation of parachain-system's upgrade authorization. -// mod v3 { -// use super::*; - -// pub fn migrate() -> Weight { -// let mut weight = T::DbWeight::get().reads(1); -// if let Some(authorized_upgrade) = AuthorizedUpgrade::::take() { -// frame_system::Pallet::::do_authorize_upgrade( -// authorized_upgrade.code_hash, -// authorized_upgrade.check_version, -// ); -// weight.saturating_add(T::DbWeight::get().writes(1)); -// } -// weight -// } -// } - /// V2: Migrate to 2D weights for ReservedXcmpWeightOverride and ReservedDmpWeightOverride. mod v2 { use super::*; From a9c6b751d9a4b634f7e8339028c2438164c32337 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Mon, 11 Dec 2023 15:10:57 +0100 Subject: [PATCH 07/16] prdoc --- prdoc/pr_2682.prdoc | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 prdoc/pr_2682.prdoc diff --git a/prdoc/pr_2682.prdoc b/prdoc/pr_2682.prdoc new file mode 100644 index 000000000000..3ae773c06b82 --- /dev/null +++ b/prdoc/pr_2682.prdoc @@ -0,0 +1,21 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json + +title: Add Authorize Upgrade Pattern to Frame System + +doc: + - audience: Runtime User + description: | + Adds the `authorize_upgrade` -> `enact_authorized_upgrade` pattern to `frame-system`. This + will be useful for upgrading bridged chains that are under the governance of Polkadot without + passing entire runtime Wasm blobs over a bridge. + + Notes: + + - Changed `enact_authorized_upgrade` to `apply_authorized_upgrade`. Personal opinion, "apply" + more accurately expresses what it's doing. Can change back if outvoted. + - Left calls in `parachain-system` and marked as deprecated to prevent breaking the API. They + just call into the `frame-system` functions. + - Updated `frame-system` benchmarks to v2 syntax. + +crates: [ ] From 2a328010d4aa4a402cc305ac7b91f6e54f2f125e Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Mon, 11 Dec 2023 17:42:41 +0100 Subject: [PATCH 08/16] add benchmarked weights --- .../src/weights/frame_system.rs | 27 ++++++++++ .../src/weights/frame_system.rs | 27 ++++++++++ .../src/weights/frame_system.rs | 27 ++++++++++ .../src/weights/frame_system.rs | 27 ++++++++++ .../src/weights/frame_system.rs | 27 ++++++++++ .../src/weights/frame_system.rs | 27 ++++++++++ .../rococo/src/weights/frame_system.rs | 27 ++++++++++ .../westend/src/weights/frame_system.rs | 27 ++++++++++ substrate/frame/system/src/weights.rs | 50 +++++++++++++++++-- 9 files changed, 262 insertions(+), 4 deletions(-) diff --git a/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weights/frame_system.rs b/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weights/frame_system.rs index 4f993155c19c..b257c3825a7e 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weights/frame_system.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weights/frame_system.rs @@ -152,4 +152,31 @@ impl frame_system::WeightInfo for WeightInfo { .saturating_add(T::DbWeight::get().writes((1_u64).saturating_mul(p.into()))) .saturating_add(Weight::from_parts(0, 70).saturating_mul(p.into())) } + /// Storage: `System::AuthorizedUpgrade` (r:0 w:1) + /// Proof: `System::AuthorizedUpgrade` (`max_values`: Some(1), `max_size`: Some(33), added: 528, mode: `MaxEncodedLen`) + fn authorize_upgrade() -> Weight { + // Proof Size summary in bytes: + // Measured: `0` + // Estimated: `0` + // Minimum execution time: 33_027_000 picoseconds. + Weight::from_parts(33_027_000, 0) + .saturating_add(Weight::from_parts(0, 0)) + .saturating_add(T::DbWeight::get().writes(1)) + } + /// Storage: `System::AuthorizedUpgrade` (r:1 w:1) + /// Proof: `System::AuthorizedUpgrade` (`max_values`: Some(1), `max_size`: Some(33), added: 528, mode: `MaxEncodedLen`) + /// Storage: `System::Digest` (r:1 w:1) + /// Proof: `System::Digest` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) + /// Storage: UNKNOWN KEY `0x3a636f6465` (r:0 w:1) + /// Proof: UNKNOWN KEY `0x3a636f6465` (r:0 w:1) + fn apply_authorized_upgrade() -> Weight { + // Proof Size summary in bytes: + // Measured: `22` + // Estimated: `1518` + // Minimum execution time: 118_101_992_000 picoseconds. + Weight::from_parts(118_101_992_000, 0) + .saturating_add(Weight::from_parts(0, 1518)) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(T::DbWeight::get().writes(3)) + } } diff --git a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/frame_system.rs b/cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/frame_system.rs index 6c741af2a13d..687b87e43915 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/frame_system.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/frame_system.rs @@ -151,4 +151,31 @@ impl frame_system::WeightInfo for WeightInfo { .saturating_add(T::DbWeight::get().writes((1_u64).saturating_mul(p.into()))) .saturating_add(Weight::from_parts(0, 70).saturating_mul(p.into())) } + /// Storage: `System::AuthorizedUpgrade` (r:0 w:1) + /// Proof: `System::AuthorizedUpgrade` (`max_values`: Some(1), `max_size`: Some(33), added: 528, mode: `MaxEncodedLen`) + fn authorize_upgrade() -> Weight { + // Proof Size summary in bytes: + // Measured: `0` + // Estimated: `0` + // Minimum execution time: 33_027_000 picoseconds. + Weight::from_parts(33_027_000, 0) + .saturating_add(Weight::from_parts(0, 0)) + .saturating_add(T::DbWeight::get().writes(1)) + } + /// Storage: `System::AuthorizedUpgrade` (r:1 w:1) + /// Proof: `System::AuthorizedUpgrade` (`max_values`: Some(1), `max_size`: Some(33), added: 528, mode: `MaxEncodedLen`) + /// Storage: `System::Digest` (r:1 w:1) + /// Proof: `System::Digest` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) + /// Storage: UNKNOWN KEY `0x3a636f6465` (r:0 w:1) + /// Proof: UNKNOWN KEY `0x3a636f6465` (r:0 w:1) + fn apply_authorized_upgrade() -> Weight { + // Proof Size summary in bytes: + // Measured: `22` + // Estimated: `1518` + // Minimum execution time: 118_101_992_000 picoseconds. + Weight::from_parts(118_101_992_000, 0) + .saturating_add(Weight::from_parts(0, 1518)) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(T::DbWeight::get().writes(3)) + } } diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/weights/frame_system.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/weights/frame_system.rs index b0f7806be8ee..df440a68a36d 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/weights/frame_system.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/weights/frame_system.rs @@ -151,4 +151,31 @@ impl frame_system::WeightInfo for WeightInfo { .saturating_add(T::DbWeight::get().writes((1_u64).saturating_mul(p.into()))) .saturating_add(Weight::from_parts(0, 70).saturating_mul(p.into())) } + /// Storage: `System::AuthorizedUpgrade` (r:0 w:1) + /// Proof: `System::AuthorizedUpgrade` (`max_values`: Some(1), `max_size`: Some(33), added: 528, mode: `MaxEncodedLen`) + fn authorize_upgrade() -> Weight { + // Proof Size summary in bytes: + // Measured: `0` + // Estimated: `0` + // Minimum execution time: 33_027_000 picoseconds. + Weight::from_parts(33_027_000, 0) + .saturating_add(Weight::from_parts(0, 0)) + .saturating_add(T::DbWeight::get().writes(1)) + } + /// Storage: `System::AuthorizedUpgrade` (r:1 w:1) + /// Proof: `System::AuthorizedUpgrade` (`max_values`: Some(1), `max_size`: Some(33), added: 528, mode: `MaxEncodedLen`) + /// Storage: `System::Digest` (r:1 w:1) + /// Proof: `System::Digest` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) + /// Storage: UNKNOWN KEY `0x3a636f6465` (r:0 w:1) + /// Proof: UNKNOWN KEY `0x3a636f6465` (r:0 w:1) + fn apply_authorized_upgrade() -> Weight { + // Proof Size summary in bytes: + // Measured: `22` + // Estimated: `1518` + // Minimum execution time: 118_101_992_000 picoseconds. + Weight::from_parts(118_101_992_000, 0) + .saturating_add(Weight::from_parts(0, 1518)) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(T::DbWeight::get().writes(3)) + } } diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/weights/frame_system.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/weights/frame_system.rs index 3dec4cc7f182..7db371d6af93 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/weights/frame_system.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/weights/frame_system.rs @@ -152,4 +152,31 @@ impl frame_system::WeightInfo for WeightInfo { .saturating_add(T::DbWeight::get().writes((1_u64).saturating_mul(p.into()))) .saturating_add(Weight::from_parts(0, 70).saturating_mul(p.into())) } + /// Storage: `System::AuthorizedUpgrade` (r:0 w:1) + /// Proof: `System::AuthorizedUpgrade` (`max_values`: Some(1), `max_size`: Some(33), added: 528, mode: `MaxEncodedLen`) + fn authorize_upgrade() -> Weight { + // Proof Size summary in bytes: + // Measured: `0` + // Estimated: `0` + // Minimum execution time: 33_027_000 picoseconds. + Weight::from_parts(33_027_000, 0) + .saturating_add(Weight::from_parts(0, 0)) + .saturating_add(T::DbWeight::get().writes(1)) + } + /// Storage: `System::AuthorizedUpgrade` (r:1 w:1) + /// Proof: `System::AuthorizedUpgrade` (`max_values`: Some(1), `max_size`: Some(33), added: 528, mode: `MaxEncodedLen`) + /// Storage: `System::Digest` (r:1 w:1) + /// Proof: `System::Digest` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) + /// Storage: UNKNOWN KEY `0x3a636f6465` (r:0 w:1) + /// Proof: UNKNOWN KEY `0x3a636f6465` (r:0 w:1) + fn apply_authorized_upgrade() -> Weight { + // Proof Size summary in bytes: + // Measured: `22` + // Estimated: `1518` + // Minimum execution time: 118_101_992_000 picoseconds. + Weight::from_parts(118_101_992_000, 0) + .saturating_add(Weight::from_parts(0, 1518)) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(T::DbWeight::get().writes(3)) + } } diff --git a/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/frame_system.rs b/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/frame_system.rs index b6f1dc8dc080..f43c5e0a40b6 100644 --- a/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/frame_system.rs +++ b/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/frame_system.rs @@ -151,4 +151,31 @@ impl frame_system::WeightInfo for WeightInfo { .saturating_add(T::DbWeight::get().writes((1_u64).saturating_mul(p.into()))) .saturating_add(Weight::from_parts(0, 70).saturating_mul(p.into())) } + /// Storage: `System::AuthorizedUpgrade` (r:0 w:1) + /// Proof: `System::AuthorizedUpgrade` (`max_values`: Some(1), `max_size`: Some(33), added: 528, mode: `MaxEncodedLen`) + fn authorize_upgrade() -> Weight { + // Proof Size summary in bytes: + // Measured: `0` + // Estimated: `0` + // Minimum execution time: 33_027_000 picoseconds. + Weight::from_parts(33_027_000, 0) + .saturating_add(Weight::from_parts(0, 0)) + .saturating_add(T::DbWeight::get().writes(1)) + } + /// Storage: `System::AuthorizedUpgrade` (r:1 w:1) + /// Proof: `System::AuthorizedUpgrade` (`max_values`: Some(1), `max_size`: Some(33), added: 528, mode: `MaxEncodedLen`) + /// Storage: `System::Digest` (r:1 w:1) + /// Proof: `System::Digest` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) + /// Storage: UNKNOWN KEY `0x3a636f6465` (r:0 w:1) + /// Proof: UNKNOWN KEY `0x3a636f6465` (r:0 w:1) + fn apply_authorized_upgrade() -> Weight { + // Proof Size summary in bytes: + // Measured: `22` + // Estimated: `1518` + // Minimum execution time: 118_101_992_000 picoseconds. + Weight::from_parts(118_101_992_000, 0) + .saturating_add(Weight::from_parts(0, 1518)) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(T::DbWeight::get().writes(3)) + } } diff --git a/cumulus/parachains/runtimes/glutton/glutton-westend/src/weights/frame_system.rs b/cumulus/parachains/runtimes/glutton/glutton-westend/src/weights/frame_system.rs index 6f8cf4f39dfd..b68f16c98658 100644 --- a/cumulus/parachains/runtimes/glutton/glutton-westend/src/weights/frame_system.rs +++ b/cumulus/parachains/runtimes/glutton/glutton-westend/src/weights/frame_system.rs @@ -150,4 +150,31 @@ impl frame_system::WeightInfo for WeightInfo { .saturating_add(T::DbWeight::get().writes((1_u64).saturating_mul(p.into()))) .saturating_add(Weight::from_parts(0, 70).saturating_mul(p.into())) } + /// Storage: `System::AuthorizedUpgrade` (r:0 w:1) + /// Proof: `System::AuthorizedUpgrade` (`max_values`: Some(1), `max_size`: Some(33), added: 528, mode: `MaxEncodedLen`) + fn authorize_upgrade() -> Weight { + // Proof Size summary in bytes: + // Measured: `0` + // Estimated: `0` + // Minimum execution time: 33_027_000 picoseconds. + Weight::from_parts(33_027_000, 0) + .saturating_add(Weight::from_parts(0, 0)) + .saturating_add(T::DbWeight::get().writes(1)) + } + /// Storage: `System::AuthorizedUpgrade` (r:1 w:1) + /// Proof: `System::AuthorizedUpgrade` (`max_values`: Some(1), `max_size`: Some(33), added: 528, mode: `MaxEncodedLen`) + /// Storage: `System::Digest` (r:1 w:1) + /// Proof: `System::Digest` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) + /// Storage: UNKNOWN KEY `0x3a636f6465` (r:0 w:1) + /// Proof: UNKNOWN KEY `0x3a636f6465` (r:0 w:1) + fn apply_authorized_upgrade() -> Weight { + // Proof Size summary in bytes: + // Measured: `22` + // Estimated: `1518` + // Minimum execution time: 118_101_992_000 picoseconds. + Weight::from_parts(118_101_992_000, 0) + .saturating_add(Weight::from_parts(0, 1518)) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(T::DbWeight::get().writes(3)) + } } diff --git a/polkadot/runtime/rococo/src/weights/frame_system.rs b/polkadot/runtime/rococo/src/weights/frame_system.rs index 7765d669a577..2e49483dcc62 100644 --- a/polkadot/runtime/rococo/src/weights/frame_system.rs +++ b/polkadot/runtime/rococo/src/weights/frame_system.rs @@ -141,4 +141,31 @@ impl frame_system::WeightInfo for WeightInfo { .saturating_add(T::DbWeight::get().writes((1_u64).saturating_mul(p.into()))) .saturating_add(Weight::from_parts(0, 70).saturating_mul(p.into())) } + /// Storage: `System::AuthorizedUpgrade` (r:0 w:1) + /// Proof: `System::AuthorizedUpgrade` (`max_values`: Some(1), `max_size`: Some(33), added: 528, mode: `MaxEncodedLen`) + fn authorize_upgrade() -> Weight { + // Proof Size summary in bytes: + // Measured: `0` + // Estimated: `0` + // Minimum execution time: 33_027_000 picoseconds. + Weight::from_parts(33_027_000, 0) + .saturating_add(Weight::from_parts(0, 0)) + .saturating_add(T::DbWeight::get().writes(1)) + } + /// Storage: `System::AuthorizedUpgrade` (r:1 w:1) + /// Proof: `System::AuthorizedUpgrade` (`max_values`: Some(1), `max_size`: Some(33), added: 528, mode: `MaxEncodedLen`) + /// Storage: `System::Digest` (r:1 w:1) + /// Proof: `System::Digest` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) + /// Storage: UNKNOWN KEY `0x3a636f6465` (r:0 w:1) + /// Proof: UNKNOWN KEY `0x3a636f6465` (r:0 w:1) + fn apply_authorized_upgrade() -> Weight { + // Proof Size summary in bytes: + // Measured: `22` + // Estimated: `1518` + // Minimum execution time: 118_101_992_000 picoseconds. + Weight::from_parts(118_101_992_000, 0) + .saturating_add(Weight::from_parts(0, 1518)) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(T::DbWeight::get().writes(3)) + } } diff --git a/polkadot/runtime/westend/src/weights/frame_system.rs b/polkadot/runtime/westend/src/weights/frame_system.rs index deef0959363c..f679be517151 100644 --- a/polkadot/runtime/westend/src/weights/frame_system.rs +++ b/polkadot/runtime/westend/src/weights/frame_system.rs @@ -144,4 +144,31 @@ impl frame_system::WeightInfo for WeightInfo { .saturating_add(T::DbWeight::get().writes((1_u64).saturating_mul(p.into()))) .saturating_add(Weight::from_parts(0, 70).saturating_mul(p.into())) } + /// Storage: `System::AuthorizedUpgrade` (r:0 w:1) + /// Proof: `System::AuthorizedUpgrade` (`max_values`: Some(1), `max_size`: Some(33), added: 528, mode: `MaxEncodedLen`) + fn authorize_upgrade() -> Weight { + // Proof Size summary in bytes: + // Measured: `0` + // Estimated: `0` + // Minimum execution time: 33_027_000 picoseconds. + Weight::from_parts(33_027_000, 0) + .saturating_add(Weight::from_parts(0, 0)) + .saturating_add(T::DbWeight::get().writes(1)) + } + /// Storage: `System::AuthorizedUpgrade` (r:1 w:1) + /// Proof: `System::AuthorizedUpgrade` (`max_values`: Some(1), `max_size`: Some(33), added: 528, mode: `MaxEncodedLen`) + /// Storage: `System::Digest` (r:1 w:1) + /// Proof: `System::Digest` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) + /// Storage: UNKNOWN KEY `0x3a636f6465` (r:0 w:1) + /// Proof: UNKNOWN KEY `0x3a636f6465` (r:0 w:1) + fn apply_authorized_upgrade() -> Weight { + // Proof Size summary in bytes: + // Measured: `22` + // Estimated: `1518` + // Minimum execution time: 118_101_992_000 picoseconds. + Weight::from_parts(118_101_992_000, 0) + .saturating_add(Weight::from_parts(0, 1518)) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(T::DbWeight::get().writes(3)) + } } diff --git a/substrate/frame/system/src/weights.rs b/substrate/frame/system/src/weights.rs index 4d2ff45dd1cf..41807dea1c55 100644 --- a/substrate/frame/system/src/weights.rs +++ b/substrate/frame/system/src/weights.rs @@ -151,11 +151,32 @@ impl WeightInfo for SubstrateWeight { .saturating_add(T::DbWeight::get().writes((1_u64).saturating_mul(p.into()))) .saturating_add(Weight::from_parts(0, 70).saturating_mul(p.into())) } + /// Storage: `System::AuthorizedUpgrade` (r:0 w:1) + /// Proof: `System::AuthorizedUpgrade` (`max_values`: Some(1), `max_size`: Some(33), added: 528, mode: `MaxEncodedLen`) fn authorize_upgrade() -> Weight { - Weight::zero() + // Proof Size summary in bytes: + // Measured: `0` + // Estimated: `0` + // Minimum execution time: 33_027_000 picoseconds. + Weight::from_parts(33_027_000, 0) + .saturating_add(Weight::from_parts(0, 0)) + .saturating_add(T::DbWeight::get().writes(1)) } + /// Storage: `System::AuthorizedUpgrade` (r:1 w:1) + /// Proof: `System::AuthorizedUpgrade` (`max_values`: Some(1), `max_size`: Some(33), added: 528, mode: `MaxEncodedLen`) + /// Storage: `System::Digest` (r:1 w:1) + /// Proof: `System::Digest` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) + /// Storage: UNKNOWN KEY `0x3a636f6465` (r:0 w:1) + /// Proof: UNKNOWN KEY `0x3a636f6465` (r:0 w:1) fn apply_authorized_upgrade() -> Weight { - Weight::zero() + // Proof Size summary in bytes: + // Measured: `22` + // Estimated: `1518` + // Minimum execution time: 118_101_992_000 picoseconds. + Weight::from_parts(118_101_992_000, 0) + .saturating_add(Weight::from_parts(0, 1518)) + .saturating_add(T::DbWeight::get().reads(2)) + .saturating_add(T::DbWeight::get().writes(3)) } } @@ -248,10 +269,31 @@ impl WeightInfo for () { .saturating_add(RocksDbWeight::get().writes((1_u64).saturating_mul(p.into()))) .saturating_add(Weight::from_parts(0, 70).saturating_mul(p.into())) } + /// Storage: `System::AuthorizedUpgrade` (r:0 w:1) + /// Proof: `System::AuthorizedUpgrade` (`max_values`: Some(1), `max_size`: Some(33), added: 528, mode: `MaxEncodedLen`) fn authorize_upgrade() -> Weight { - Weight::zero() + // Proof Size summary in bytes: + // Measured: `0` + // Estimated: `0` + // Minimum execution time: 33_027_000 picoseconds. + Weight::from_parts(33_027_000, 0) + .saturating_add(Weight::from_parts(0, 0)) + .saturating_add(RocksDbWeight::get().writes(1)) } + /// Storage: `System::AuthorizedUpgrade` (r:1 w:1) + /// Proof: `System::AuthorizedUpgrade` (`max_values`: Some(1), `max_size`: Some(33), added: 528, mode: `MaxEncodedLen`) + /// Storage: `System::Digest` (r:1 w:1) + /// Proof: `System::Digest` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) + /// Storage: UNKNOWN KEY `0x3a636f6465` (r:0 w:1) + /// Proof: UNKNOWN KEY `0x3a636f6465` (r:0 w:1) fn apply_authorized_upgrade() -> Weight { - Weight::zero() + // Proof Size summary in bytes: + // Measured: `22` + // Estimated: `1518` + // Minimum execution time: 118_101_992_000 picoseconds. + Weight::from_parts(118_101_992_000, 0) + .saturating_add(Weight::from_parts(0, 1518)) + .saturating_add(RocksDbWeight::get().reads(2)) + .saturating_add(RocksDbWeight::get().writes(3)) } } From c3a8202dfc6129cee677cef3c7e906c4c4517400 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Mon, 11 Dec 2023 18:56:07 +0100 Subject: [PATCH 09/16] fix match --- .../frame/support/test/tests/pallet_outer_enums_explicit.rs | 2 ++ .../frame/support/test/tests/pallet_outer_enums_implicit.rs | 2 ++ 2 files changed, 4 insertions(+) diff --git a/substrate/frame/support/test/tests/pallet_outer_enums_explicit.rs b/substrate/frame/support/test/tests/pallet_outer_enums_explicit.rs index 6246ad93d678..faf4752a75d2 100644 --- a/substrate/frame/support/test/tests/pallet_outer_enums_explicit.rs +++ b/substrate/frame/support/test/tests/pallet_outer_enums_explicit.rs @@ -92,6 +92,8 @@ fn module_error_outer_enum_expand_explicit() { frame_system::Error::CallFiltered => (), frame_system::Error::InvalidTask => (), frame_system::Error::FailedTask => (), + frame_system::Error::NothingAuthorized => (), + frame_system::Error::Unauthorized => (), frame_system::Error::__Ignore(_, _) => (), }, diff --git a/substrate/frame/support/test/tests/pallet_outer_enums_implicit.rs b/substrate/frame/support/test/tests/pallet_outer_enums_implicit.rs index 26023dfa7b72..9253c5645b8e 100644 --- a/substrate/frame/support/test/tests/pallet_outer_enums_implicit.rs +++ b/substrate/frame/support/test/tests/pallet_outer_enums_implicit.rs @@ -92,6 +92,8 @@ fn module_error_outer_enum_expand_implicit() { frame_system::Error::CallFiltered => (), frame_system::Error::InvalidTask => (), frame_system::Error::FailedTask => (), + frame_system::Error::NothingAuthorized => (), + frame_system::Error::Unauthorized => (), frame_system::Error::__Ignore(_, _) => (), }, From a674095ed45ffa49bbfd438f6c0553964f1cd854 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Fri, 15 Dec 2023 09:32:50 +0100 Subject: [PATCH 10/16] make checked upgrade the default --- .../frame/system/benchmarking/src/lib.rs | 3 ++- substrate/frame/system/src/lib.rs | 27 +++++++++++++------ substrate/frame/system/src/tests.rs | 2 +- 3 files changed, 22 insertions(+), 10 deletions(-) diff --git a/substrate/frame/system/benchmarking/src/lib.rs b/substrate/frame/system/benchmarking/src/lib.rs index e430c9940054..8318c46847d8 100644 --- a/substrate/frame/system/benchmarking/src/lib.rs +++ b/substrate/frame/system/benchmarking/src/lib.rs @@ -208,7 +208,7 @@ mod benchmarks { let hash = T::Hashing::hash(&runtime_blob); #[extrinsic_call] - authorize_upgrade(RawOrigin::Root, hash, true); + authorize_upgrade(RawOrigin::Root, hash); assert!(System::::authorized_upgrade().is_some()); Ok(()) @@ -219,6 +219,7 @@ mod benchmarks { let runtime_blob = T::prepare_set_code_data(); T::setup_set_code_requirements(&runtime_blob)?; let hash = T::Hashing::hash(&runtime_blob); + // Will be heavier when it needs to do verification. System::::authorize_upgrade(RawOrigin::Root.into(), hash, true)?; #[extrinsic_call] diff --git a/substrate/frame/system/src/lib.rs b/substrate/frame/system/src/lib.rs index 371e79a62be0..e2f54a2ef32c 100644 --- a/substrate/frame/system/src/lib.rs +++ b/substrate/frame/system/src/lib.rs @@ -681,20 +681,31 @@ pub mod pallet { /// Authorize an upgrade to a given `code_hash` for the runtime. The runtime can be supplied /// later. /// - /// The `check_version` parameter sets a boolean flag for whether or not the runtime's spec - /// version and name should be verified on upgrade. Since the authorization only has a hash, - /// it cannot actually perform the verification. - /// /// This call requires Root origin. #[pallet::call_index(9)] #[pallet::weight((T::SystemWeightInfo::authorize_upgrade(), DispatchClass::Operational))] - pub fn authorize_upgrade( + pub fn authorize_upgrade(origin: OriginFor, code_hash: T::Hash) -> DispatchResult { + ensure_root(origin)?; + Self::do_authorize_upgrade(code_hash, true); + Ok(()) + } + + /// Authorize an upgrade to a given `code_hash` for the runtime. The runtime can be supplied + /// later. + /// + /// WARNING: This authorizes an upgrade that will take place without any safety checks, for + /// example that the spec name remains the same and that the version number increases. Not + /// recommended for normal use. Use `authorize_upgrade` instead. + /// + /// This call requires Root origin. + #[pallet::call_index(10)] + #[pallet::weight((T::SystemWeightInfo::authorize_upgrade(), DispatchClass::Operational))] + pub fn authorize_upgrade_without_checks( origin: OriginFor, code_hash: T::Hash, - check_version: bool, ) -> DispatchResult { ensure_root(origin)?; - Self::do_authorize_upgrade(code_hash, check_version); + Self::do_authorize_upgrade(code_hash, false); Ok(()) } @@ -707,7 +718,7 @@ pub mod pallet { /// the new `code` in the same block or attempt to schedule the upgrade. /// /// All origins are allowed. - #[pallet::call_index(10)] + #[pallet::call_index(11)] #[pallet::weight((T::SystemWeightInfo::apply_authorized_upgrade(), DispatchClass::Operational))] pub fn apply_authorized_upgrade( _: OriginFor, diff --git a/substrate/frame/system/src/tests.rs b/substrate/frame/system/src/tests.rs index fdddc2527fe3..053cec24f89c 100644 --- a/substrate/frame/system/src/tests.rs +++ b/substrate/frame/system/src/tests.rs @@ -694,7 +694,7 @@ fn set_code_via_authorization_works() { ); // Can authorize - assert_ok!(System::authorize_upgrade(RawOrigin::Root.into(), hash, true)); + assert_ok!(System::authorize_upgrade(RawOrigin::Root.into(), hash)); System::assert_has_event( SysEvent::UpgradeAuthorized { code_hash: hash, check_version: true }.into(), ); From 71cd77c6e9e6f807578c2e0c80f0db52353c1834 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Fri, 15 Dec 2023 09:37:08 +0100 Subject: [PATCH 11/16] /// --- cumulus/pallets/parachain-system/src/lib.rs | 2 +- substrate/frame/system/src/lib.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cumulus/pallets/parachain-system/src/lib.rs b/cumulus/pallets/parachain-system/src/lib.rs index b643e4a79669..e687496956c5 100644 --- a/cumulus/pallets/parachain-system/src/lib.rs +++ b/cumulus/pallets/parachain-system/src/lib.rs @@ -1528,7 +1528,7 @@ impl Pallet { } } -/// Type that implements `SetCode` +/// Type that implements `SetCode`. pub struct ParachainSetCode(sp_std::marker::PhantomData); impl frame_system::SetCode for ParachainSetCode { fn set_code(code: Vec) -> DispatchResult { diff --git a/substrate/frame/system/src/lib.rs b/substrate/frame/system/src/lib.rs index 462fdb44f9d4..80ef85634012 100644 --- a/substrate/frame/system/src/lib.rs +++ b/substrate/frame/system/src/lib.rs @@ -775,7 +775,7 @@ pub mod pallet { task: T::RuntimeTask, err: DispatchError, }, - // Upgrade authorized. + /// An upgrade was authorized. UpgradeAuthorized { code_hash: T::Hash, check_version: bool, From 03cc5f53792eb17b8ec42cb6b30e253a99cfada1 Mon Sep 17 00:00:00 2001 From: command-bot <> Date: Fri, 15 Dec 2023 08:42:04 +0000 Subject: [PATCH 12/16] ".git/.scripts/commands/fmt/fmt.sh" --- substrate/frame/system/src/lib.rs | 40 +++++++------------------------ 1 file changed, 9 insertions(+), 31 deletions(-) diff --git a/substrate/frame/system/src/lib.rs b/substrate/frame/system/src/lib.rs index 80ef85634012..47d3396051b9 100644 --- a/substrate/frame/system/src/lib.rs +++ b/substrate/frame/system/src/lib.rs @@ -736,50 +736,28 @@ pub mod pallet { #[pallet::event] pub enum Event { /// An extrinsic completed successfully. - ExtrinsicSuccess { - dispatch_info: DispatchInfo, - }, + ExtrinsicSuccess { dispatch_info: DispatchInfo }, /// An extrinsic failed. - ExtrinsicFailed { - dispatch_error: DispatchError, - dispatch_info: DispatchInfo, - }, + ExtrinsicFailed { dispatch_error: DispatchError, dispatch_info: DispatchInfo }, /// `:code` was updated. CodeUpdated, /// A new account was created. - NewAccount { - account: T::AccountId, - }, + NewAccount { account: T::AccountId }, /// An account was reaped. - KilledAccount { - account: T::AccountId, - }, + KilledAccount { account: T::AccountId }, /// On on-chain remark happened. - Remarked { - sender: T::AccountId, - hash: T::Hash, - }, + Remarked { sender: T::AccountId, hash: T::Hash }, #[cfg(feature = "experimental")] /// A [`Task`] has started executing - TaskStarted { - task: T::RuntimeTask, - }, + TaskStarted { task: T::RuntimeTask }, #[cfg(feature = "experimental")] /// A [`Task`] has finished executing. - TaskCompleted { - task: T::RuntimeTask, - }, + TaskCompleted { task: T::RuntimeTask }, #[cfg(feature = "experimental")] /// A [`Task`] failed during execution. - TaskFailed { - task: T::RuntimeTask, - err: DispatchError, - }, + TaskFailed { task: T::RuntimeTask, err: DispatchError }, /// An upgrade was authorized. - UpgradeAuthorized { - code_hash: T::Hash, - check_version: bool, - }, + UpgradeAuthorized { code_hash: T::Hash, check_version: bool }, } /// Error for the System pallet From 4ae401f552397f91a90b58965486cdb5fcf42e20 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Fri, 15 Dec 2023 09:52:44 +0100 Subject: [PATCH 13/16] fix benchmark --- substrate/frame/system/benchmarking/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/frame/system/benchmarking/src/lib.rs b/substrate/frame/system/benchmarking/src/lib.rs index 8318c46847d8..18bfb85f52df 100644 --- a/substrate/frame/system/benchmarking/src/lib.rs +++ b/substrate/frame/system/benchmarking/src/lib.rs @@ -219,8 +219,8 @@ mod benchmarks { let runtime_blob = T::prepare_set_code_data(); T::setup_set_code_requirements(&runtime_blob)?; let hash = T::Hashing::hash(&runtime_blob); - // Will be heavier when it needs to do verification. - System::::authorize_upgrade(RawOrigin::Root.into(), hash, true)?; + // Will be heavier when it needs to do verification (i.e. don't use `...without_checks`). + System::::authorize_upgrade(RawOrigin::Root.into(), hash)?; #[extrinsic_call] apply_authorized_upgrade(RawOrigin::Root, runtime_blob); From 5718a0b9560dac819fe60bff4b9b4709ebcbd738 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Mon, 18 Dec 2023 16:05:59 +0100 Subject: [PATCH 14/16] docs about upgrade paths --- substrate/frame/system/src/lib.rs | 53 +++++++++++++++++++++++++------ 1 file changed, 43 insertions(+), 10 deletions(-) diff --git a/substrate/frame/system/src/lib.rs b/substrate/frame/system/src/lib.rs index 47d3396051b9..f9f6d4054080 100644 --- a/substrate/frame/system/src/lib.rs +++ b/substrate/frame/system/src/lib.rs @@ -17,27 +17,60 @@ //! # System Pallet //! -//! The System pallet provides low-level access to core types and cross-cutting utilities. -//! It acts as the base layer for other pallets to interact with the Substrate framework components. +//! The System pallet provides low-level access to core types and cross-cutting utilities. It acts +//! as the base layer for other pallets to interact with the Substrate framework components. //! //! - [`Config`] //! //! ## Overview //! -//! The System pallet defines the core data types used in a Substrate runtime. -//! It also provides several utility functions (see [`Pallet`]) for other FRAME pallets. +//! The System pallet defines the core data types used in a Substrate runtime. It also provides +//! several utility functions (see [`Pallet`]) for other FRAME pallets. //! -//! In addition, it manages the storage items for extrinsics data, indexes, event records, and -//! digest items, among other things that support the execution of the current block. +//! In addition, it manages the storage items for extrinsic data, indices, event records, and digest +//! items, among other things that support the execution of the current block. //! -//! It also handles low-level tasks like depositing logs, basic set up and take down of -//! temporary storage entries, and access to previous block hashes. +//! It also handles low-level tasks like depositing logs, basic set up and take down of temporary +//! storage entries, and access to previous block hashes. //! //! ## Interface //! //! ### Dispatchable Functions //! -//! The System pallet does not implement any dispatchable functions. +//! The System pallet provides dispatchable functions that, with the exception of `remark`, manage +//! low-level or privileged functionality of a Substrate-based runtime. +//! +//! - `remark`: Make some on-chain remark. +//! - `set_heap_pages`: Set the number of pages in the WebAssembly environment's heap. +//! - `set_code`: Set the new runtime code. +//! - `set_code_without_checks`: Set the new runtime code without any checks. +//! - `set_storage`: Set some items of storage. +//! - `kill_storage`: Kill some items from storage. +//! - `kill_prefix`: Kill all storage items with a key that starts with the given prefix. +//! - `remark_with_event`: Make some on-chain remark and emit an event. +//! - `do_task`: Do some specified task. +//! - `authorize_upgrade`: Authorize new runtime code. +//! - `authorize_upgrade_without_checks`: Authorize new runtime code and an upgrade sans +//! verification. +//! - `apply_authorized_upgrade`: Provide new, already-authorized runtime code. +//! +//! #### A Note on Upgrades +//! +//! The pallet provides two primary means of upgrading the runtime, a single-phase means using +//! `set_code` and a two-phase means using `authorize_upgrade` followed by +//! `apply_authorized_upgrade`. The first will directly attempt to apply the provided `code` +//! (application may have to be scheduled, depending on the context and implementation of the +//! `OnSetCode` trait). +//! +//! The `authorize_upgrade` route allows the authorization of a runtime's code hash. Once +//! authorized, anyone may upload the correct runtime to apply the code. This pattern is useful when +//! providing the runtime ahead of time may be unwieldy, for example when a large preimage (the +//! code) would need to be stored on-chain or sent over a message transport protocol such as a +//! bridge. +//! +//! The `*_without_checks` variants do not perform any version checks, so using them runs the risk +//! of applying a downgrade or entirely other chain specification. They will still validate that the +//! `code` meets the authorized hash. //! //! ### Public Functions //! @@ -59,7 +92,7 @@ //! - [`CheckTxVersion`]: Checks that the transaction version is the same as the one used to sign //! the transaction. //! -//! Lookup the runtime aggregator file (e.g. `node/runtime`) to see the full list of signed +//! Look up the runtime aggregator file (e.g. `node/runtime`) to see the full list of signed //! extensions included in a chain. #![cfg_attr(not(feature = "std"), no_std)] From 630fe656970e60af662458415745b77fe0af91fc Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Wed, 20 Dec 2023 08:39:25 +0100 Subject: [PATCH 15/16] add deprecation date --- cumulus/pallets/parachain-system/src/lib.rs | 8 ++++++-- prdoc/pr_2682.prdoc | 6 +++--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/cumulus/pallets/parachain-system/src/lib.rs b/cumulus/pallets/parachain-system/src/lib.rs index db1a13282170..ba8aff0e369d 100644 --- a/cumulus/pallets/parachain-system/src/lib.rs +++ b/cumulus/pallets/parachain-system/src/lib.rs @@ -664,7 +664,9 @@ pub mod pallet { #[pallet::call_index(2)] #[pallet::weight(::SystemWeightInfo::authorize_upgrade())] #[allow(deprecated)] - #[deprecated(note = "Migrate to frame_system::authorize_upgrade")] + #[deprecated( + note = "To be removed after June 2024. Migrate to `frame_system::authorize_upgrade`." + )] pub fn authorize_upgrade( origin: OriginFor, code_hash: T::Hash, @@ -687,7 +689,9 @@ pub mod pallet { #[pallet::call_index(3)] #[pallet::weight(::SystemWeightInfo::apply_authorized_upgrade())] #[allow(deprecated)] - #[deprecated(note = "Migrate to frame_system::apply_authorized_upgrade")] + #[deprecated( + note = "To be removed after June 2024. Migrate to `frame_system::apply_authorized_upgrade`." + )] pub fn enact_authorized_upgrade( _: OriginFor, code: Vec, diff --git a/prdoc/pr_2682.prdoc b/prdoc/pr_2682.prdoc index 3ae773c06b82..eaa5f5a4a9a6 100644 --- a/prdoc/pr_2682.prdoc +++ b/prdoc/pr_2682.prdoc @@ -1,7 +1,7 @@ # Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 # See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json -title: Add Authorize Upgrade Pattern to Frame System +title: "Add Authorize Upgrade Pattern to Frame System" doc: - audience: Runtime User @@ -12,10 +12,10 @@ doc: Notes: - - Changed `enact_authorized_upgrade` to `apply_authorized_upgrade`. Personal opinion, "apply" - more accurately expresses what it's doing. Can change back if outvoted. + - Changed `enact_authorized_upgrade` to `apply_authorized_upgrade`. - Left calls in `parachain-system` and marked as deprecated to prevent breaking the API. They just call into the `frame-system` functions. + - Deprecated calls will be removed no earlier than June 2024. - Updated `frame-system` benchmarks to v2 syntax. crates: [ ] From 434f125cf119e1d697573cedf17ea70e80bdd981 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Wed, 20 Dec 2023 14:00:19 +0100 Subject: [PATCH 16/16] use getter --- substrate/frame/system/src/lib.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/substrate/frame/system/src/lib.rs b/substrate/frame/system/src/lib.rs index f9f6d4054080..069217bcee46 100644 --- a/substrate/frame/system/src/lib.rs +++ b/substrate/frame/system/src/lib.rs @@ -939,6 +939,7 @@ pub mod pallet { /// `Some` if a code upgrade has been authorized. #[pallet::storage] + #[pallet::getter(fn authorized_upgrade)] pub(super) type AuthorizedUpgrade = StorageValue<_, CodeUpgradeAuthorization, OptionQuery>; @@ -2028,11 +2029,6 @@ impl Pallet { Ok(post) } - /// Return an upgrade authorization, if it exists. - pub fn authorized_upgrade() -> Option> { - AuthorizedUpgrade::::get() - } - /// Check that provided `code` can be upgraded to. Namely, check that its hash matches an /// existing authorization and that it meets the specification requirements of `can_set_code`. pub fn validate_authorized_upgrade(code: &[u8]) -> Result {