Add Version Checks on Para Upgrade (#2261)

* add version checks on para ugprade

* Apply suggestions from code review

Co-authored-by: Bastian Köcher <[email protected]>

* remove unneeded imports and errors

* fix test error path

---------

Co-authored-by: Bastian Köcher <[email protected]>

Author: joepetrowski

pallets/parachain-system/src/lib.rs

@@ -16,18 +16,18 @@
 
 #![cfg_attr(not(feature = "std"), no_std)]
 
-//! cumulus-pallet-parachain-system is a base pallet for cumulus-based parachains.
+//! `cumulus-pallet-parachain-system` is a base pallet for Cumulus-based parachains.
 //!
-//! This pallet handles low-level details of being a parachain. It's responsibilities include:
+//! This pallet handles low-level details of being a parachain. Its responsibilities include:
 //!
-//! - ingestion of the parachain validation data
-//! - ingestion of incoming downward and lateral messages and dispatching them
-//! - coordinating upgrades with the relay-chain
-//! - communication of parachain outputs, such as sent messages, signalling an upgrade, etc.
+//! - ingestion of the parachain validation data;
+//! - ingestion and dispatch of incoming downward and lateral messages;
+//! - coordinating upgrades with the Relay Chain; and
+//! - communication of parachain outputs, such as sent messages, signaling an upgrade, etc.
 //!
 //! Users must ensure that they register this pallet as an inherent provider.
 
-use codec::Encode;
+use codec::{Decode, Encode, MaxEncodedLen};
 use cumulus_primitives_core::{
 	relay_chain, AbridgedHostConfiguration, ChannelStatus, CollationInfo, DmpMessageHandler,
 	GetChannelInfo, InboundDownwardMessage, InboundHrmpMessage, MessageSendError,
@@ -45,6 +45,7 @@ use frame_support::{
 };
 use frame_system::{ensure_none, ensure_root};
 use polkadot_parachain::primitives::RelayChainBlockNumber;
+use scale_info::TypeInfo;
 use sp_runtime::{
 	traits::{Block as BlockT, BlockNumberProvider, Hash},
 	transaction_validity::{
@@ -134,6 +135,20 @@ impl CheckAssociatedRelayNumber for AnyRelayNumber {
 	fn check_associated_relay_number(_: RelayChainBlockNumber, _: RelayChainBlockNumber) {}
 }
 
+/// 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<T>
+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 super::*;
@@ -442,17 +457,40 @@ pub mod pallet {
 			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<T>, code_hash: T::Hash) -> DispatchResult {
+		pub fn authorize_upgrade(
+			origin: OriginFor<T>,
+			code_hash: T::Hash,
+			check_version: bool,
+		) -> DispatchResult {
 			ensure_root(origin)?;
-
-			AuthorizedUpgrade::<T>::put(&code_hash);
+			AuthorizedUpgrade::<T>::put(CodeUpgradeAuthorization {
+				code_hash: code_hash.clone(),
+				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(
@@ -487,16 +525,16 @@ pub mod pallet {
 
 	#[pallet::error]
 	pub enum Error<T> {
-		/// Attempt to upgrade validation function while existing upgrade pending
+		/// Attempt to upgrade validation function while existing upgrade pending.
 		OverlappingUpgrades,
-		/// Polkadot currently prohibits this parachain from upgrading its validation function
+		/// Polkadot currently prohibits this parachain from upgrading its validation function.
 		ProhibitedByPolkadot,
 		/// The supplied validation function has compiled into a blob larger than Polkadot is
-		/// willing to run
+		/// willing to run.
 		TooBig,
-		/// The inherent which supplies the validation data did not run this block
+		/// The inherent which supplies the validation data did not run this block.
 		ValidationDataNotAvailable,
-		/// The inherent which supplies the host configuration did not run this block
+		/// The inherent which supplies the host configuration did not run this block.
 		HostConfigurationNotAvailable,
 		/// No validation function upgrade is currently scheduled.
 		NotScheduled,
@@ -645,7 +683,7 @@ pub mod pallet {
 
 	/// The next authorized upgrade, if there is one.
 	#[pallet::storage]
-	pub(super) type AuthorizedUpgrade<T: Config> = StorageValue<_, T::Hash>;
+	pub(super) type AuthorizedUpgrade<T: Config> = StorageValue<_, CodeUpgradeAuthorization<T>>;
 
 	/// A custom head data that should be returned as result of `validate_block`.
 	///
@@ -712,9 +750,17 @@ pub mod pallet {
 
 impl<T: Config> Pallet<T> {
 	fn validate_authorized_upgrade(code: &[u8]) -> Result<T::Hash, DispatchError> {
-		let required_hash = AuthorizedUpgrade::<T>::get().ok_or(Error::<T>::NothingAuthorized)?;
+		let authorization = AuthorizedUpgrade::<T>::get().ok_or(Error::<T>::NothingAuthorized)?;
+
+		// ensure that the actual hash matches the authorized hash
 		let actual_hash = T::Hashing::hash(&code[..]);
-		ensure!(actual_hash == required_hash, Error::<T>::Unauthorized);
+		ensure!(actual_hash == authorization.code_hash, Error::<T>::Unauthorized);
+
+		// check versions if required as part of the authorization
+		if authorization.check_version {
+			frame_system::Pallet::<T>::can_set_code(code)?;
+		}
+
 		Ok(actual_hash)
 	}
 }

pallets/parachain-system/src/tests.rs

@@ -32,10 +32,11 @@ use frame_support::{
 use frame_system::RawOrigin;
 use hex_literal::hex;
 use relay_chain::HrmpChannelId;
-use sp_core::H256;
+use sp_core::{blake2_256, H256};
 use sp_runtime::{
 	testing::Header,
 	traits::{BlakeTwo256, IdentityLookup},
+	DispatchErrorWithPostInfo,
 };
 use sp_version::RuntimeVersion;
 use std::cell::RefCell;
@@ -974,3 +975,37 @@ fn test() {
 		.add(1, || {})
 		.add(2, || {});
 }
+
+#[test]
+fn upgrade_version_checks_should_work() {
+	let test_data = vec![
+		("test", 0, 1, Err(frame_system::Error::<Test>::SpecVersionNeedsToIncrease)),
+		("test", 1, 0, Err(frame_system::Error::<Test>::SpecVersionNeedsToIncrease)),
+		("test", 1, 1, Err(frame_system::Error::<Test>::SpecVersionNeedsToIncrease)),
+		("test", 1, 2, Err(frame_system::Error::<Test>::SpecVersionNeedsToIncrease)),
+		("test2", 1, 1, Err(frame_system::Error::<Test>::InvalidSpecName)),
+	];
+
+	for (spec_name, spec_version, impl_version, expected) in test_data.into_iter() {
+		let version = RuntimeVersion {
+			spec_name: spec_name.into(),
+			spec_version,
+			impl_version,
+			..Default::default()
+		};
+		let read_runtime_version = ReadRuntimeVersion(version.encode());
+
+		let mut ext = new_test_ext();
+		ext.register_extension(sp_core::traits::ReadRuntimeVersionExt::new(read_runtime_version));
+		ext.execute_with(|| {
+			let new_code = vec![1, 2, 3, 4];
+			let new_code_hash = sp_core::H256(blake2_256(&new_code));
+
+			let _authorize =
+				ParachainSystem::authorize_upgrade(RawOrigin::Root.into(), new_code_hash, true);
+			let res = ParachainSystem::enact_authorized_upgrade(RawOrigin::None.into(), new_code);
+
+			assert_eq!(expected.map_err(DispatchErrorWithPostInfo::from), res);
+		});
+	}
+}