pallet-xcm: add expiry to authorized aliases

cumulus/parachains/runtimes/assets/asset-hub-westend/tests/tests.rs

@@ -987,7 +987,8 @@ fn authorized_aliases_work() {
 			// Alice explicitly authorizes `alice_on_sibling_para` to alias her local account
 			assert_ok!(PolkadotXcm::add_authorized_alias(
 				RuntimeHelper::origin_of(alice.clone()),
-				Box::new(alice_on_sibling_para.clone().into())
+				Box::new(alice_on_sibling_para.clone().into()),
+				None
 			));
 
 			// `alice_on_sibling_para` now explicitly allowed to alias into `local_alice`
@@ -1006,7 +1007,8 @@ fn authorized_aliases_work() {
 			// Alice explicitly authorizes `alice_on_relay` to alias her local account
 			assert_ok!(PolkadotXcm::add_authorized_alias(
 				RuntimeHelper::origin_of(alice.clone()),
-				Box::new(alice_on_relay.clone().into())
+				Box::new(alice_on_relay.clone().into()),
+				None
 			));
 			// Now both `alice_on_relay` and `alice_on_sibling_para` can alias into her local
 			// account

polkadot/xcm/pallet-xcm/src/benchmarking.rs

@@ -276,15 +276,15 @@ benchmarks! {
 		let loc = VersionedLocation::from(Location::from(Parent));
 		SupportedVersion::<T>::insert(old_version, loc, old_version);
 	}: {
-		crate::Pallet::<T>::check_xcm_version_change(VersionMigrationStage::MigrateSupportedVersion, Weight::zero());
+		crate::Pallet::<T>::lazy_migration(VersionMigrationStage::MigrateSupportedVersion, Weight::zero());
 	}
 
 	migrate_version_notifiers {
 		let old_version = XCM_VERSION - 1;
 		let loc = VersionedLocation::from(Location::from(Parent));
 		VersionNotifiers::<T>::insert(old_version, loc, 0);
 	}: {
-		crate::Pallet::<T>::check_xcm_version_change(VersionMigrationStage::MigrateVersionNotifiers, Weight::zero());
+		crate::Pallet::<T>::lazy_migration(VersionMigrationStage::MigrateVersionNotifiers, Weight::zero());
 	}
 
 	already_notified_target {
@@ -295,7 +295,7 @@ benchmarks! {
 		let current_version = T::AdvertisedXcmVersion::get();
 		VersionNotifyTargets::<T>::insert(current_version, loc, (0, Weight::zero(), current_version));
 	}: {
-		crate::Pallet::<T>::check_xcm_version_change(VersionMigrationStage::NotifyCurrentTargets(None), Weight::zero());
+		crate::Pallet::<T>::lazy_migration(VersionMigrationStage::NotifyCurrentTargets(None), Weight::zero());
 	}
 
 	notify_current_targets {
@@ -307,7 +307,7 @@ benchmarks! {
 		let old_version = current_version - 1;
 		VersionNotifyTargets::<T>::insert(current_version, loc, (0, Weight::zero(), old_version));
 	}: {
-		crate::Pallet::<T>::check_xcm_version_change(VersionMigrationStage::NotifyCurrentTargets(None), Weight::zero());
+		crate::Pallet::<T>::lazy_migration(VersionMigrationStage::NotifyCurrentTargets(None), Weight::zero());
 	}
 
 	notify_target_migration_fail {
@@ -323,7 +323,7 @@ benchmarks! {
 		let current_version = T::AdvertisedXcmVersion::get();
 		VersionNotifyTargets::<T>::insert(current_version, bad_location, (0, Weight::zero(), current_version));
 	}: {
-		crate::Pallet::<T>::check_xcm_version_change(VersionMigrationStage::MigrateAndNotifyOldTargets, Weight::zero());
+		crate::Pallet::<T>::lazy_migration(VersionMigrationStage::MigrateAndNotifyOldTargets, Weight::zero());
 	}
 
 	migrate_version_notify_targets {
@@ -332,7 +332,7 @@ benchmarks! {
 		let loc = VersionedLocation::from(Location::from(Parent));
 		VersionNotifyTargets::<T>::insert(old_version, loc, (0, Weight::zero(), current_version));
 	}: {
-		crate::Pallet::<T>::check_xcm_version_change(VersionMigrationStage::MigrateAndNotifyOldTargets, Weight::zero());
+		crate::Pallet::<T>::lazy_migration(VersionMigrationStage::MigrateAndNotifyOldTargets, Weight::zero());
 	}
 
 	migrate_and_notify_old_targets {
@@ -343,7 +343,7 @@ benchmarks! {
 		let old_version = T::AdvertisedXcmVersion::get() - 1;
 		VersionNotifyTargets::<T>::insert(old_version, loc, (0, Weight::zero(), old_version));
 	}: {
-		crate::Pallet::<T>::check_xcm_version_change(VersionMigrationStage::MigrateAndNotifyOldTargets, Weight::zero());
+		crate::Pallet::<T>::lazy_migration(VersionMigrationStage::MigrateAndNotifyOldTargets, Weight::zero());
 	}
 
 	new_query {
@@ -389,27 +389,29 @@ benchmarks! {
 		let origin = RawOrigin::Root;
 		let origin_location: VersionedLocation = T::ExecuteXcmOrigin::try_origin(origin.clone().into())
 			.map_err(|_| BenchmarkError::Override(BenchmarkResult::from_weight(Weight::MAX)))?.into();
-		let mut existing_aliases = BoundedVec::<VersionedLocation, MaxAuthorizedAliases>::new();
+		let mut existing_aliases = BoundedVec::<OriginAliaser, MaxAuthorizedAliases>::new();
 		// prepopulate list with `max-1` aliases to benchmark worst case
 		for i in 1..MaxAuthorizedAliases::get() {
 			let alias = Location::new(1, [Parachain(i), AccountId32 { network: None, id: [42_u8; 32] }]).into();
-			existing_aliases.try_push(alias).unwrap()
+			let aliaser = OriginAliaser { location: alias, expiry: None };
+			existing_aliases.try_push(aliaser).unwrap()
 		}
 		AuthorizedAliases::<T>::insert(&origin_location, existing_aliases);
 		// now benchmark adding new alias
 		let aliaser: VersionedLocation =
 			Location::new(1, [Parachain(1234), AccountId32 { network: None, id: [42_u8; 32] }]).into();
-	}: _(origin, Box::new(aliaser))
+	}: _(origin, Box::new(aliaser), None)
 
 	remove_authorized_alias {
 		let origin = RawOrigin::Root;
 		let origin_location: VersionedLocation = T::ExecuteXcmOrigin::try_origin(origin.clone().into())
 			.map_err(|_| BenchmarkError::Override(BenchmarkResult::from_weight(Weight::MAX)))?.into();
-		let mut existing_aliases = BoundedVec::<VersionedLocation, MaxAuthorizedAliases>::new();
+		let mut existing_aliases = BoundedVec::<OriginAliaser, MaxAuthorizedAliases>::new();
 		// prepopulate list with `max` aliases to benchmark worst case
 		for i in 1..MaxAuthorizedAliases::get()+1 {
 			let alias = Location::new(1, [Parachain(i), AccountId32 { network: None, id: [42_u8; 32] }]).into();
-			existing_aliases.try_push(alias).unwrap()
+			let aliaser = OriginAliaser { location: alias, expiry: None };
+			existing_aliases.try_push(aliaser).unwrap()
 		}
 		AuthorizedAliases::<T>::insert(&origin_location, existing_aliases);
 		// now benchmark removing an alias

polkadot/xcm/pallet-xcm/src/lib.rs

@@ -51,7 +51,7 @@ use sp_runtime::{
 		AccountIdConversion, BadOrigin, BlakeTwo256, BlockNumberProvider, Dispatchable, Hash,
 		Saturating, Zero,
 	},
-	Either, RuntimeDebug,
+	Either, RuntimeDebug, SaturatedConversion,
 };
 use xcm::{latest::QueryResponseInfo, prelude::*};
 use xcm_builder::{
@@ -591,6 +591,9 @@ pub mod pallet {
 		/// Too many locations authorized to alias origin.
 		#[codec(index = 25)]
 		TooManyAuthorizedAliases,
+		/// Expiry block number is in the past.
+		#[codec(index = 26)]
+		ExpiresInPast,
 	}
 
 	impl<T: Config> From<SendError> for Error<T> {
@@ -649,6 +652,15 @@ pub mod pallet {
 		MigrateVersionNotifiers,
 		NotifyCurrentTargets(Option<Vec<u8>>),
 		MigrateAndNotifyOldTargets,
+		RemoveExpiredAliasAuthorizations,
+	}
+
+	/// Entry of an authorized aliaser for a local origin. The aliaser `location` is only authorized
+	/// until its inner `expiry` block number.
+	#[derive(Clone, Encode, Decode, Debug, Eq, PartialEq, Ord, PartialOrd, TypeInfo)]
+	pub(crate) struct OriginAliaser {
+		pub(crate) location: VersionedLocation,
+		pub(crate) expiry: Option<u64>,
 	}
 
 	impl Default for VersionMigrationStage {
@@ -811,13 +823,14 @@ pub mod pallet {
 	pub(crate) type RecordedXcm<T: Config> = StorageValue<_, Xcm<()>>;
 
 	/// Map of authorized aliasers of local origins. Each local location can authorize a list of
-	/// other locations to alias into it.
+	/// other locations to alias into it. Each aliaser is only valid until its inner `expiry`
+	/// block number.
 	#[pallet::storage]
 	pub(super) type AuthorizedAliases<T: Config> = StorageMap<
 		_,
 		Blake2_128Concat,
 		VersionedLocation,
-		BoundedVec<VersionedLocation, MaxAuthorizedAliases>,
+		BoundedVec<OriginAliaser, MaxAuthorizedAliases>,
 		ValueQuery,
 	>;
 
@@ -849,7 +862,7 @@ pub mod pallet {
 			if let Some(migration) = CurrentMigration::<T>::get() {
 				// Consume 10% of block at most
 				let max_weight = T::BlockWeights::get().max_block / 10;
-				let (w, maybe_migration) = Self::check_xcm_version_change(migration, max_weight);
+				let (w, maybe_migration) = Self::lazy_migration(migration, max_weight);
 				if maybe_migration.is_none() {
 					Self::deposit_event(Event::VersionMigrationFinished { version: XCM_VERSION });
 				}
@@ -1455,6 +1468,7 @@ pub mod pallet {
 		}
 
 		/// Authorize another `aliaser` location to alias into the local `origin` making this call.
+		/// The `aliaser` is only authorized until the provided `expiry` block number.
 		///
 		/// Usually useful to allow your local account to be aliased into from a remote location
 		/// also under your control (like your account on another chain).
@@ -1466,20 +1480,42 @@ pub mod pallet {
 		pub fn add_authorized_alias(
 			origin: OriginFor<T>,
 			aliaser: Box<VersionedLocation>,
+			expires: Option<u64>,
 		) -> DispatchResult {
 			let origin: Location = T::ExecuteXcmOrigin::ensure_origin(origin)?;
 			let aliaser: Location = (*aliaser).try_into().map_err(|()| Error::<T>::BadVersion)?;
-			tracing::debug!(target: "xcm::pallet_xcm::add_authorized_alias", ?origin, ?aliaser);
+			tracing::debug!(target: "xcm::pallet_xcm::add_authorized_alias", ?origin, ?aliaser, ?expires);
 			ensure!(origin != aliaser, Error::<T>::BadLocation);
+			if let Some(expiry) = expires {
+				ensure!(
+					expiry > frame_system::Pallet::<T>::block_number().saturated_into::<u64>(),
+					Error::<T>::ExpiresInPast
+				);
+			}
 			let versioned_origin = VersionedLocation::from(origin);
 			let versioned_aliaser = VersionedLocation::from(aliaser);
 			let mut authorized_aliases = AuthorizedAliases::<T>::get(&versioned_origin);
-			if !authorized_aliases.contains(&versioned_aliaser) {
+			if authorized_aliases
+				.iter_mut()
+				.find_map(|aliaser| {
+					// if the aliaser already exists, just update its expiry block
+					if aliaser.location == versioned_aliaser {
+						aliaser.expiry = expires;
+						Some(())
+					} else {
+						None
+					}
+				})
+				.is_none()
+			{
+				// if the aliaser does not yet exist, add it to the list
+				let aliaser = OriginAliaser { location: versioned_aliaser, expiry: expires };
 				authorized_aliases
-					.try_push(versioned_aliaser)
+					.try_push(aliaser)
 					.map_err(|_| Error::<T>::TooManyAuthorizedAliases)?;
-				AuthorizedAliases::<T>::insert(&versioned_origin, authorized_aliases);
+				// todo: hold storage deposit
 			}
+			AuthorizedAliases::<T>::insert(&versioned_origin, authorized_aliases);
 			Ok(())
 		}
 
@@ -1498,8 +1534,11 @@ pub mod pallet {
 			let versioned_to_remove = VersionedLocation::from(to_remove);
 			let mut authorized_aliases = AuthorizedAliases::<T>::get(&versioned_origin);
 			let original_length = authorized_aliases.len();
-			authorized_aliases.retain(|alias| versioned_to_remove.ne(alias));
-			if original_length != authorized_aliases.len() {
+			authorized_aliases.retain(|alias| versioned_to_remove.ne(&alias.location));
+			if authorized_aliases.is_empty() {
+				// todo: return storage deposit
+				AuthorizedAliases::<T>::remove(&versioned_origin);
+			} else if original_length != authorized_aliases.len() {
 				AuthorizedAliases::<T>::insert(&versioned_origin, authorized_aliases);
 			}
 			Ok(())
@@ -2342,7 +2381,7 @@ impl<T: Config> Pallet<T> {
 
 	/// Will always make progress, and will do its best not to use much more than `weight_cutoff`
 	/// in doing so.
-	pub(crate) fn check_xcm_version_change(
+	pub(crate) fn lazy_migration(
 		mut stage: VersionMigrationStage,
 		weight_cutoff: Weight,
 	) -> (Weight, Option<VersionMigrationStage>) {
@@ -2495,6 +2534,29 @@ impl<T: Config> Pallet<T> {
 					}
 				}
 			}
+			stage = RemoveExpiredAliasAuthorizations;
+		}
+		if stage == RemoveExpiredAliasAuthorizations {
+			let block_num = frame_system::Pallet::<T>::block_number().saturated_into::<u64>();
+			let mut writes = 0;
+			// need to iterate keys and modify map in separate steps to avoid undefined behavior
+			let keys: Vec<VersionedLocation> = AuthorizedAliases::<T>::iter_keys().collect();
+			weight_used.saturating_add(T::DbWeight::get().reads(keys.len() as u64));
+			for key in keys {
+				let mut aliases = AuthorizedAliases::<T>::get(&key);
+				let old_len = aliases.len();
+				aliases.retain(|aliaser| {
+					aliaser.expiry.map(|expiry| expiry > block_num).unwrap_or(true)
+				});
+				if aliases.is_empty() {
+					AuthorizedAliases::<T>::remove(&key);
+					writes = writes + 1;
+				} else if aliases.len() != old_len {
+					AuthorizedAliases::<T>::insert(key, aliases);
+					writes = writes + 1;
+				}
+			}
+			weight_used.saturating_add(T::DbWeight::get().writes(writes));
 		}
 		(weight_used, None)
 	}
@@ -3519,7 +3581,18 @@ impl<L: Into<VersionedLocation> + Clone, T: Config> ContainsPair<L, L> for Autho
 		let origin: VersionedLocation = origin.clone().into();
 		let target: VersionedLocation = target.clone().into();
 		tracing::trace!(target: "xcm::pallet_xcm::AuthorizedAliasers::contains", ?origin, ?target);
-		AuthorizedAliases::<T>::get(&target).contains(&origin)
+		// return true if the `origin` has been explicitly authorized by `target` as aliaser, and
+		// the authorization has not expired
+		AuthorizedAliases::<T>::get(&target)
+			.iter()
+			.find(|&aliaser| {
+				let current_block =
+					frame_system::Pallet::<T>::block_number().saturated_into::<u64>();
+				let not_expired =
+					aliaser.expiry.map(|expiry| expiry < current_block).unwrap_or(true);
+				aliaser.location == origin && not_expired
+			})
+			.is_some()
 	}
 }
 

polkadot/xcm/pallet-xcm/src/migration.rs

@@ -171,19 +171,22 @@ pub mod data {
 	}
 
 	/// Implementation of `NeedsMigration` for `AuthorizedAliases` data.
-	impl<M: Get<u32>> NeedsMigration for (&VersionedLocation, &BoundedVec<VersionedLocation, M>) {
-		type MigratedData = (VersionedLocation, BoundedVec<VersionedLocation, M>);
+	impl<M: Get<u32>, T: frame_system::Config> NeedsMigration
+		for (&VersionedLocation, &BoundedVec<OriginAliaser, M>, PhantomData<T>)
+	{
+		type MigratedData = (VersionedLocation, BoundedVec<OriginAliaser, M>);
 
 		fn needs_migration(&self, _: XcmVersion) -> bool {
 			self.0.identify_version() < XCM_VERSION ||
-				self.1.iter().any(|alias| alias.identify_version() < XCM_VERSION)
+				self.1.iter().any(|alias| alias.location.identify_version() < XCM_VERSION)
 		}
 
 		fn try_migrate(self, _: XcmVersion) -> Result<Option<Self::MigratedData>, ()> {
 			if !self.needs_migration(XCM_VERSION) {
 				return Ok(None)
 			}
 
+			let block_num = frame_system::Pallet::<T>::block_number().saturated_into::<u64>();
 			let key = if self.0.identify_version() < XCM_VERSION {
 				let Ok(converted_key) = self.0.clone().into_version(XCM_VERSION) else {
 					return Err(())
@@ -192,24 +195,19 @@ pub mod data {
 			} else {
 				self.0.clone()
 			};
-			let mut aliases = BoundedVec::<VersionedLocation, M>::new();
+			let mut new_aliases = BoundedVec::<OriginAliaser, M>::new();
 			for alias in self.1 {
-				if aliases
-					.try_push(if alias.identify_version() < XCM_VERSION {
-						let Ok(new_alias) = alias.clone().into_version(XCM_VERSION) else {
-							return Err(())
-						};
-						new_alias
-					} else {
-						alias.clone()
-					})
-					.is_err()
-				{
-					return Err(())
-				};
+				// skip expired aliases
+				if alias.expiry.map(|expiry| expiry <= block_num).unwrap_or(false) {
+					continue
+				}
+				let OriginAliaser { mut location, expiry } = alias.clone();
+				if location.identify_version() < XCM_VERSION {
+					location = location.into_version(XCM_VERSION)?;
+				}
+				new_aliases.try_push(OriginAliaser { location, expiry }).map_err(|_| ())?;
 			}
-
-			Ok(Some((key, aliases)))
+			Ok(Some((key, new_aliases)))
 		}
 	}
 
@@ -370,7 +368,7 @@ pub mod data {
 			// check and migrate `AuthorizedAliases`
 			let aliases_to_migrate = AuthorizedAliases::<T>::iter().filter_map(|(id, data)| {
 				weight.saturating_add(T::DbWeight::get().reads(1));
-				match (&id, &data).try_migrate(required_xcm_version) {
+				match (&id, &data, PhantomData::<T>).try_migrate(required_xcm_version) {
 					Ok(Some((new_id, new_data))) => Some((id, new_id, new_data)),
 					Ok(None) => None,
 					Err(_) => {
@@ -394,7 +392,9 @@ pub mod data {
 					"Migrating `AuthorizedAliases`"
 				);
 				AuthorizedAliases::<T>::remove(old_id);
-				AuthorizedAliases::<T>::insert(new_id, new_data);
+				if !new_data.is_empty() {
+					AuthorizedAliases::<T>::insert(new_id, new_data);
+				}
 				count = count + 1;
 			}
 			// two writes per key, one to remove old entry, one to write new entry