diff --git a/cumulus/parachains/runtimes/assets/asset-hub-westend/tests/tests.rs b/cumulus/parachains/runtimes/assets/asset-hub-westend/tests/tests.rs index 23bec5016832..e64a365ca5bd 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-westend/tests/tests.rs +++ b/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 diff --git a/polkadot/xcm/pallet-xcm/src/benchmarking.rs b/polkadot/xcm/pallet-xcm/src/benchmarking.rs index 69d710a35d4b..0e0f50e7cc7d 100644 --- a/polkadot/xcm/pallet-xcm/src/benchmarking.rs +++ b/polkadot/xcm/pallet-xcm/src/benchmarking.rs @@ -276,7 +276,7 @@ benchmarks! { let loc = VersionedLocation::from(Location::from(Parent)); SupportedVersion::::insert(old_version, loc, old_version); }: { - crate::Pallet::::check_xcm_version_change(VersionMigrationStage::MigrateSupportedVersion, Weight::zero()); + crate::Pallet::::lazy_migration(VersionMigrationStage::MigrateSupportedVersion, Weight::zero()); } migrate_version_notifiers { @@ -284,7 +284,7 @@ benchmarks! { let loc = VersionedLocation::from(Location::from(Parent)); VersionNotifiers::::insert(old_version, loc, 0); }: { - crate::Pallet::::check_xcm_version_change(VersionMigrationStage::MigrateVersionNotifiers, Weight::zero()); + crate::Pallet::::lazy_migration(VersionMigrationStage::MigrateVersionNotifiers, Weight::zero()); } already_notified_target { @@ -295,7 +295,7 @@ benchmarks! { let current_version = T::AdvertisedXcmVersion::get(); VersionNotifyTargets::::insert(current_version, loc, (0, Weight::zero(), current_version)); }: { - crate::Pallet::::check_xcm_version_change(VersionMigrationStage::NotifyCurrentTargets(None), Weight::zero()); + crate::Pallet::::lazy_migration(VersionMigrationStage::NotifyCurrentTargets(None), Weight::zero()); } notify_current_targets { @@ -307,7 +307,7 @@ benchmarks! { let old_version = current_version - 1; VersionNotifyTargets::::insert(current_version, loc, (0, Weight::zero(), old_version)); }: { - crate::Pallet::::check_xcm_version_change(VersionMigrationStage::NotifyCurrentTargets(None), Weight::zero()); + crate::Pallet::::lazy_migration(VersionMigrationStage::NotifyCurrentTargets(None), Weight::zero()); } notify_target_migration_fail { @@ -323,7 +323,7 @@ benchmarks! { let current_version = T::AdvertisedXcmVersion::get(); VersionNotifyTargets::::insert(current_version, bad_location, (0, Weight::zero(), current_version)); }: { - crate::Pallet::::check_xcm_version_change(VersionMigrationStage::MigrateAndNotifyOldTargets, Weight::zero()); + crate::Pallet::::lazy_migration(VersionMigrationStage::MigrateAndNotifyOldTargets, Weight::zero()); } migrate_version_notify_targets { @@ -332,7 +332,7 @@ benchmarks! { let loc = VersionedLocation::from(Location::from(Parent)); VersionNotifyTargets::::insert(old_version, loc, (0, Weight::zero(), current_version)); }: { - crate::Pallet::::check_xcm_version_change(VersionMigrationStage::MigrateAndNotifyOldTargets, Weight::zero()); + crate::Pallet::::lazy_migration(VersionMigrationStage::MigrateAndNotifyOldTargets, Weight::zero()); } migrate_and_notify_old_targets { @@ -343,7 +343,7 @@ benchmarks! { let old_version = T::AdvertisedXcmVersion::get() - 1; VersionNotifyTargets::::insert(old_version, loc, (0, Weight::zero(), old_version)); }: { - crate::Pallet::::check_xcm_version_change(VersionMigrationStage::MigrateAndNotifyOldTargets, Weight::zero()); + crate::Pallet::::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::::new(); + let mut existing_aliases = BoundedVec::::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::::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::::new(); + let mut existing_aliases = BoundedVec::::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::::insert(&origin_location, existing_aliases); // now benchmark removing an alias diff --git a/polkadot/xcm/pallet-xcm/src/lib.rs b/polkadot/xcm/pallet-xcm/src/lib.rs index 8b4ba937fec6..167c4e20f1c6 100644 --- a/polkadot/xcm/pallet-xcm/src/lib.rs +++ b/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 From for Error { @@ -649,6 +652,15 @@ pub mod pallet { MigrateVersionNotifiers, NotifyCurrentTargets(Option>), 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, } impl Default for VersionMigrationStage { @@ -811,13 +823,14 @@ pub mod pallet { pub(crate) type RecordedXcm = 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 = StorageMap< _, Blake2_128Concat, VersionedLocation, - BoundedVec, + BoundedVec, ValueQuery, >; @@ -849,7 +862,7 @@ pub mod pallet { if let Some(migration) = CurrentMigration::::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, aliaser: Box, + expires: Option, ) -> DispatchResult { let origin: Location = T::ExecuteXcmOrigin::ensure_origin(origin)?; let aliaser: Location = (*aliaser).try_into().map_err(|()| Error::::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::::BadLocation); + if let Some(expiry) = expires { + ensure!( + expiry > frame_system::Pallet::::block_number().saturated_into::(), + Error::::ExpiresInPast + ); + } let versioned_origin = VersionedLocation::from(origin); let versioned_aliaser = VersionedLocation::from(aliaser); let mut authorized_aliases = AuthorizedAliases::::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::::TooManyAuthorizedAliases)?; - AuthorizedAliases::::insert(&versioned_origin, authorized_aliases); + // todo: hold storage deposit } + AuthorizedAliases::::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::::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::::remove(&versioned_origin); + } else if original_length != authorized_aliases.len() { AuthorizedAliases::::insert(&versioned_origin, authorized_aliases); } Ok(()) @@ -2342,7 +2381,7 @@ impl Pallet { /// 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) { @@ -2495,6 +2534,29 @@ impl Pallet { } } } + stage = RemoveExpiredAliasAuthorizations; + } + if stage == RemoveExpiredAliasAuthorizations { + let block_num = frame_system::Pallet::::block_number().saturated_into::(); + let mut writes = 0; + // need to iterate keys and modify map in separate steps to avoid undefined behavior + let keys: Vec = AuthorizedAliases::::iter_keys().collect(); + weight_used.saturating_add(T::DbWeight::get().reads(keys.len() as u64)); + for key in keys { + let mut aliases = AuthorizedAliases::::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::::remove(&key); + writes = writes + 1; + } else if aliases.len() != old_len { + AuthorizedAliases::::insert(key, aliases); + writes = writes + 1; + } + } + weight_used.saturating_add(T::DbWeight::get().writes(writes)); } (weight_used, None) } @@ -3519,7 +3581,18 @@ impl + Clone, T: Config> ContainsPair 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::::get(&target).contains(&origin) + // return true if the `origin` has been explicitly authorized by `target` as aliaser, and + // the authorization has not expired + AuthorizedAliases::::get(&target) + .iter() + .find(|&aliaser| { + let current_block = + frame_system::Pallet::::block_number().saturated_into::(); + let not_expired = + aliaser.expiry.map(|expiry| expiry < current_block).unwrap_or(true); + aliaser.location == origin && not_expired + }) + .is_some() } } diff --git a/polkadot/xcm/pallet-xcm/src/migration.rs b/polkadot/xcm/pallet-xcm/src/migration.rs index fafef2b23d86..82df2870e117 100644 --- a/polkadot/xcm/pallet-xcm/src/migration.rs +++ b/polkadot/xcm/pallet-xcm/src/migration.rs @@ -171,12 +171,14 @@ pub mod data { } /// Implementation of `NeedsMigration` for `AuthorizedAliases` data. - impl> NeedsMigration for (&VersionedLocation, &BoundedVec) { - type MigratedData = (VersionedLocation, BoundedVec); + impl, T: frame_system::Config> NeedsMigration + for (&VersionedLocation, &BoundedVec, PhantomData) + { + type MigratedData = (VersionedLocation, BoundedVec); 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, ()> { @@ -184,6 +186,7 @@ pub mod data { return Ok(None) } + let block_num = frame_system::Pallet::::block_number().saturated_into::(); 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::::new(); + let mut new_aliases = BoundedVec::::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::::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::).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::::remove(old_id); - AuthorizedAliases::::insert(new_id, new_data); + if !new_data.is_empty() { + AuthorizedAliases::::insert(new_id, new_data); + } count = count + 1; } // two writes per key, one to remove old entry, one to write new entry diff --git a/polkadot/xcm/pallet-xcm/src/tests/mod.rs b/polkadot/xcm/pallet-xcm/src/tests/mod.rs index 350530f7711f..9dd42999774f 100644 --- a/polkadot/xcm/pallet-xcm/src/tests/mod.rs +++ b/polkadot/xcm/pallet-xcm/src/tests/mod.rs @@ -1058,7 +1058,7 @@ fn subscription_side_upgrades_work_with_multistage_notify() { let mut counter = 0; while let Some(migration) = maybe_migration.take() { counter += 1; - let (_, m) = XcmPallet::check_xcm_version_change(migration, Weight::zero()); + let (_, m) = XcmPallet::lazy_migration(migration, Weight::zero()); maybe_migration = m; } assert_eq!(counter, 4); @@ -1211,7 +1211,7 @@ fn multistage_migration_works() { let mut weight_used = Weight::zero(); while let Some(migration) = maybe_migration.take() { counter += 1; - let (w, m) = XcmPallet::check_xcm_version_change(migration, Weight::zero()); + let (w, m) = XcmPallet::lazy_migration(migration, Weight::zero()); maybe_migration = m; weight_used.saturating_accrue(w); }