From d53700a6bdaf618cd0ffb5bdc5e6b992cb9156f2 Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Thu, 17 Oct 2024 10:34:22 +0200 Subject: [PATCH 01/11] Missing `MigrateToLatestXcmVersion` for westend # Conflicts: # polkadot/runtime/westend/src/lib.rs --- polkadot/runtime/westend/src/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index b7dae533224c..1016207458c1 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -1805,6 +1805,8 @@ pub mod migrations { MaxAgentsToMigrate, >, parachains_shared::migration::MigrateToV1, + // permanent + pallet_xcm::migration::MigrateToLatestXcmVersion, ); } From 2cf2ad73405d46d2ef8c9952f7c90cce921f16d1 Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Mon, 21 Oct 2024 14:18:08 +0200 Subject: [PATCH 02/11] Align everywhere `type AdvertisedXcmVersion = pallet_xcm::CurrentXcmVersion` --- .../cookbook/relay_token_transactor/parachain/xcm_config.rs | 2 +- .../cookbook/relay_token_transactor/relay_chain/xcm_config.rs | 2 +- .../src/asset_exchange/single_asset_adapter/mock.rs | 2 +- polkadot/xcm/xcm-builder/src/tests/pay/mock.rs | 3 +-- polkadot/xcm/xcm-runtime-apis/tests/mock.rs | 3 +-- 5 files changed, 5 insertions(+), 7 deletions(-) diff --git a/polkadot/xcm/docs/src/cookbook/relay_token_transactor/parachain/xcm_config.rs b/polkadot/xcm/docs/src/cookbook/relay_token_transactor/parachain/xcm_config.rs index 7cb230f6e006..f8a1826b8ab1 100644 --- a/polkadot/xcm/docs/src/cookbook/relay_token_transactor/parachain/xcm_config.rs +++ b/polkadot/xcm/docs/src/cookbook/relay_token_transactor/parachain/xcm_config.rs @@ -168,7 +168,7 @@ impl pallet_xcm::Config for Runtime { type UniversalLocation = UniversalLocation; // No version discovery needed const VERSION_DISCOVERY_QUEUE_SIZE: u32 = 0; - type AdvertisedXcmVersion = frame::traits::ConstU32<3>; + type AdvertisedXcmVersion = pallet_xcm::CurrentXcmVersion; type AdminOrigin = frame_system::EnsureRoot; // No locking type TrustedLockers = (); diff --git a/polkadot/xcm/docs/src/cookbook/relay_token_transactor/relay_chain/xcm_config.rs b/polkadot/xcm/docs/src/cookbook/relay_token_transactor/relay_chain/xcm_config.rs index a31e664d8216..e7b602df7338 100644 --- a/polkadot/xcm/docs/src/cookbook/relay_token_transactor/relay_chain/xcm_config.rs +++ b/polkadot/xcm/docs/src/cookbook/relay_token_transactor/relay_chain/xcm_config.rs @@ -142,7 +142,7 @@ impl pallet_xcm::Config for Runtime { type UniversalLocation = UniversalLocation; // No version discovery needed const VERSION_DISCOVERY_QUEUE_SIZE: u32 = 0; - type AdvertisedXcmVersion = frame::traits::ConstU32<3>; + type AdvertisedXcmVersion = pallet_xcm::CurrentXcmVersion; type AdminOrigin = frame_system::EnsureRoot; // No locking type TrustedLockers = (); diff --git a/polkadot/xcm/xcm-builder/src/asset_exchange/single_asset_adapter/mock.rs b/polkadot/xcm/xcm-builder/src/asset_exchange/single_asset_adapter/mock.rs index 4d9809e84f88..e6fe8e45c265 100644 --- a/polkadot/xcm/xcm-builder/src/asset_exchange/single_asset_adapter/mock.rs +++ b/polkadot/xcm/xcm-builder/src/asset_exchange/single_asset_adapter/mock.rs @@ -312,7 +312,7 @@ impl pallet_xcm::Config for Runtime { type UniversalLocation = UniversalLocation; // No version discovery needed const VERSION_DISCOVERY_QUEUE_SIZE: u32 = 0; - type AdvertisedXcmVersion = frame_support::traits::ConstU32<3>; + type AdvertisedXcmVersion = pallet_xcm::CurrentXcmVersion; type AdminOrigin = frame_system::EnsureRoot; // No locking type TrustedLockers = (); diff --git a/polkadot/xcm/xcm-builder/src/tests/pay/mock.rs b/polkadot/xcm/xcm-builder/src/tests/pay/mock.rs index d76ff21b8597..26ea226313f0 100644 --- a/polkadot/xcm/xcm-builder/src/tests/pay/mock.rs +++ b/polkadot/xcm/xcm-builder/src/tests/pay/mock.rs @@ -126,7 +126,6 @@ parameter_types! { pub const AnyNetwork: Option = None; pub UniversalLocation: InteriorLocation = (ByGenesis([0; 32]), Parachain(42)).into(); pub UnitWeightCost: u64 = 1_000; - pub static AdvertisedXcmVersion: u32 = 3; pub const BaseXcmWeight: Weight = Weight::from_parts(1_000, 1_000); pub CurrencyPerSecondPerByte: (AssetId, u128, u128) = (AssetId(RelayLocation::get()), 1, 1); pub TrustedAssets: (AssetFilter, Location) = (All.into(), Here.into()); @@ -267,7 +266,7 @@ impl pallet_xcm::Config for Test { type RuntimeOrigin = RuntimeOrigin; type RuntimeCall = RuntimeCall; const VERSION_DISCOVERY_QUEUE_SIZE: u32 = 100; - type AdvertisedXcmVersion = AdvertisedXcmVersion; + type AdvertisedXcmVersion = pallet_xcm::CurrentXcmVersion; type TrustedLockers = (); type SovereignAccountOf = SovereignAccountOf; type Currency = Balances; diff --git a/polkadot/xcm/xcm-runtime-apis/tests/mock.rs b/polkadot/xcm/xcm-runtime-apis/tests/mock.rs index b3afa23503e3..4a5de8875007 100644 --- a/polkadot/xcm/xcm-runtime-apis/tests/mock.rs +++ b/polkadot/xcm/xcm-runtime-apis/tests/mock.rs @@ -145,7 +145,6 @@ parameter_types! { pub const MaxInstructions: u32 = 100; pub const NativeTokenPerSecondPerByte: (AssetId, u128, u128) = (AssetId(HereLocation::get()), 1, 1); pub UniversalLocation: InteriorLocation = [GlobalConsensus(NetworkId::Westend), Parachain(2000)].into(); - pub static AdvertisedXcmVersion: XcmVersion = 4; pub const HereLocation: Location = Location::here(); pub const RelayLocation: Location = Location::parent(); pub const MaxAssetsIntoHolding: u32 = 64; @@ -349,7 +348,7 @@ impl pallet_xcm::Config for TestRuntime { type RuntimeOrigin = RuntimeOrigin; type RuntimeCall = RuntimeCall; const VERSION_DISCOVERY_QUEUE_SIZE: u32 = 100; - type AdvertisedXcmVersion = AdvertisedXcmVersion; + type AdvertisedXcmVersion = pallet_xcm::CurrentXcmVersion; type AdminOrigin = EnsureRoot; type TrustedLockers = (); type SovereignAccountOf = (); From c7d9771778f38f45722cc2d3118751e5c4325644 Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Mon, 21 Oct 2024 14:32:16 +0200 Subject: [PATCH 03/11] very nits --- polkadot/xcm/pallet-xcm/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/polkadot/xcm/pallet-xcm/src/lib.rs b/polkadot/xcm/pallet-xcm/src/lib.rs index 254bc2c7b83d..761a45da69e9 100644 --- a/polkadot/xcm/pallet-xcm/src/lib.rs +++ b/polkadot/xcm/pallet-xcm/src/lib.rs @@ -2887,7 +2887,7 @@ impl xcm_executor::traits::Enact for UnlockTicket { let mut maybe_remove_index = None; let mut locked = BalanceOf::::zero(); let mut found = false; - // We could just as well do with with an into_iter, filter_map and collect, however this way + // We could just as well do with an into_iter, filter_map and collect, however this way // avoids making an allocation. for (i, x) in locks.iter_mut().enumerate() { if x.1.try_as::<_>().defensive() == Ok(&self.unlocker) { @@ -3268,7 +3268,7 @@ impl OnResponse for Pallet { }); return Weight::zero() } - return match maybe_notify { + match maybe_notify { Some((pallet_index, call_index)) => { // This is a bit horrible, but we happen to know that the `Call` will // be built by `(pallet_index: u8, call_index: u8, QueryId, Response)`. From 5667031b0098834de448a1b82d89a81cb425e280 Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Mon, 21 Oct 2024 15:24:15 +0200 Subject: [PATCH 04/11] Add `Queries` to the `try-state` --- polkadot/xcm/pallet-xcm/src/lib.rs | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/polkadot/xcm/pallet-xcm/src/lib.rs b/polkadot/xcm/pallet-xcm/src/lib.rs index 761a45da69e9..3b4366a3b933 100644 --- a/polkadot/xcm/pallet-xcm/src/lib.rs +++ b/polkadot/xcm/pallet-xcm/src/lib.rs @@ -2835,6 +2835,34 @@ impl Pallet { ); } + // Take minimal version from `SafeXcmVersion` or `T::AdvertisedXcmVersion` and ensure that the operational data is stored at least at that version, for example, to prevent issues when removing older XCM versions. + let minimal_allowed_xcm_version = if let Some(safe_xcm_version) = SafeXcmVersion::::get() { + T::AdvertisedXcmVersion::get().min(safe_xcm_version) + } else { + T::AdvertisedXcmVersion::get() + }; + tracing::warn!( + target: "runtime", + ?minimal_allowed_xcm_version, + "Checking xcm version for `pallet_xcm`'s operational data", + ); + + // check `Queries` + ensure!( + Queries::::iter_values().all(|query| match query { + QueryStatus::Pending { responder, maybe_match_querier, .. } => + responder.identify_version() >= minimal_allowed_xcm_version + && maybe_match_querier + .map(|v| v.identify_version() >= minimal_allowed_xcm_version) + .unwrap_or(true), + QueryStatus::VersionNotifier { origin, .. } => + origin.identify_version() >= minimal_allowed_xcm_version, + QueryStatus::Ready { response, .. } => + response.identify_version() >= minimal_allowed_xcm_version, + }), + TryRuntimeError::Other("`Queries` data should be migrated to the higher xcm version!") + ); + Ok(()) } } From 6dc9179154a34c7558091b92b2d0562cfb0a65ff Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Tue, 22 Oct 2024 14:10:59 +0200 Subject: [PATCH 05/11] WIP: try_state for Queries and LockedFungibles --- polkadot/xcm/pallet-xcm/src/lib.rs | 88 ++++++++++++++++++++---------- 1 file changed, 60 insertions(+), 28 deletions(-) diff --git a/polkadot/xcm/pallet-xcm/src/lib.rs b/polkadot/xcm/pallet-xcm/src/lib.rs index 3b4366a3b933..49c4311ca2d4 100644 --- a/polkadot/xcm/pallet-xcm/src/lib.rs +++ b/polkadot/xcm/pallet-xcm/src/lib.rs @@ -2807,6 +2807,33 @@ impl Pallet { /// set. #[cfg(any(feature = "try-runtime", test))] pub fn do_try_state() -> Result<(), TryRuntimeError> { + // Take minimal version from `SafeXcmVersion` or `latest - 1` and ensure that the + // operational data is stored at least at that version, for example, to prevent issues when + // removing older XCM versions. + let minimal_allowed_xcm_version = if let Some(safe_xcm_version) = SafeXcmVersion::::get() + { + XCM_VERSION.saturating_sub(1).min(safe_xcm_version) + } else { + XCM_VERSION.saturating_sub(1) + }; + + // check `Queries` + ensure!( + !Queries::::iter_values() + .any(|query| query.needs_migration(minimal_allowed_xcm_version)), + TryRuntimeError::Other("`Queries` data should be migrated to the higher xcm version!") + ); + + // check `LockedFungibles` + ensure!( + !LockedFungibles::::iter_values() + .any(|lockeds| LockedFungiblesWrapper::(lockeds) + .needs_migration(minimal_allowed_xcm_version)), + TryRuntimeError::Other( + "`LockedFungibles` data should be migrated to the higher xcm version!" + ) + ); + // if migration has been already scheduled, everything is ok and data will be eventually // migrated if CurrentMigration::::exists() { @@ -2833,35 +2860,7 @@ impl Pallet { "`VersionNotifyTargets` data should be migrated to the `XCM_VERSION`!`" ) ); - } - - // Take minimal version from `SafeXcmVersion` or `T::AdvertisedXcmVersion` and ensure that the operational data is stored at least at that version, for example, to prevent issues when removing older XCM versions. - let minimal_allowed_xcm_version = if let Some(safe_xcm_version) = SafeXcmVersion::::get() { - T::AdvertisedXcmVersion::get().min(safe_xcm_version) - } else { - T::AdvertisedXcmVersion::get() - }; - tracing::warn!( - target: "runtime", - ?minimal_allowed_xcm_version, - "Checking xcm version for `pallet_xcm`'s operational data", - ); - // check `Queries` - ensure!( - Queries::::iter_values().all(|query| match query { - QueryStatus::Pending { responder, maybe_match_querier, .. } => - responder.identify_version() >= minimal_allowed_xcm_version - && maybe_match_querier - .map(|v| v.identify_version() >= minimal_allowed_xcm_version) - .unwrap_or(true), - QueryStatus::VersionNotifier { origin, .. } => - origin.identify_version() >= minimal_allowed_xcm_version, - QueryStatus::Ready { response, .. } => - response.identify_version() >= minimal_allowed_xcm_version, - }), - TryRuntimeError::Other("`Queries` data should be migrated to the higher xcm version!") - ); Ok(()) } @@ -3516,3 +3515,36 @@ impl> ConvertOrigin } } } +pub trait NeedsMigration { + fn needs_migration(&self, minimal_allowed_xcm_version: XcmVersion) -> bool; +} + +struct LockedFungiblesWrapper( + BoundedVec<(BalanceOf, VersionedLocation), T::MaxLockers>, +); +impl NeedsMigration for LockedFungiblesWrapper { + + fn needs_migration(&self, minimal_allowed_xcm_version: XcmVersion) -> bool { + self.0 + .iter() + .any(|(_, unlocker)| unlocker.identify_version() < minimal_allowed_xcm_version) + } +} + +impl NeedsMigration for QueryStatus { + + fn needs_migration(&self, minimal_allowed_xcm_version: XcmVersion) -> bool { + match &self { + QueryStatus::Pending { responder, maybe_match_querier, .. } => + responder.identify_version() < minimal_allowed_xcm_version || + maybe_match_querier + .as_ref() + .map(|v| v.identify_version() < minimal_allowed_xcm_version) + .unwrap_or(false), + QueryStatus::VersionNotifier { origin, .. } => + origin.identify_version() < minimal_allowed_xcm_version, + QueryStatus::Ready { response, .. } => + response.identify_version() < minimal_allowed_xcm_version, + } + } +} From 64eaf497e4c96d4ea78d32684a6cf2b1cb0a06c0 Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Tue, 22 Oct 2024 14:35:22 +0200 Subject: [PATCH 06/11] typo --- polkadot/xcm/pallet-xcm/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/xcm/pallet-xcm/src/lib.rs b/polkadot/xcm/pallet-xcm/src/lib.rs index 49c4311ca2d4..351406bf9f33 100644 --- a/polkadot/xcm/pallet-xcm/src/lib.rs +++ b/polkadot/xcm/pallet-xcm/src/lib.rs @@ -2860,7 +2860,7 @@ impl Pallet { "`VersionNotifyTargets` data should be migrated to the `XCM_VERSION`!`" ) ); - + } Ok(()) } From 0105b4ac17b80d050fefa633cb1f2e20ed57161a Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Wed, 23 Oct 2024 11:27:03 +0200 Subject: [PATCH 07/11] Adds migration for `pallet_xcm` data: `Queries`, `LockedFungibles`, `RemoteLockedFungibles` --- polkadot/xcm/pallet-xcm/src/lib.rs | 50 +-- polkadot/xcm/pallet-xcm/src/migration.rs | 367 ++++++++++++++++++++++- polkadot/xcm/pallet-xcm/src/tests/mod.rs | 150 ++++++++- 3 files changed, 521 insertions(+), 46 deletions(-) diff --git a/polkadot/xcm/pallet-xcm/src/lib.rs b/polkadot/xcm/pallet-xcm/src/lib.rs index 351406bf9f33..701d987e8830 100644 --- a/polkadot/xcm/pallet-xcm/src/lib.rs +++ b/polkadot/xcm/pallet-xcm/src/lib.rs @@ -2807,6 +2807,8 @@ impl Pallet { /// set. #[cfg(any(feature = "try-runtime", test))] pub fn do_try_state() -> Result<(), TryRuntimeError> { + use migration::data::NeedsMigration; + // Take minimal version from `SafeXcmVersion` or `latest - 1` and ensure that the // operational data is stored at least at that version, for example, to prevent issues when // removing older XCM versions. @@ -2820,20 +2822,29 @@ impl Pallet { // check `Queries` ensure!( !Queries::::iter_values() - .any(|query| query.needs_migration(minimal_allowed_xcm_version)), + .any(|data| data.needs_migration(minimal_allowed_xcm_version)), TryRuntimeError::Other("`Queries` data should be migrated to the higher xcm version!") ); // check `LockedFungibles` ensure!( !LockedFungibles::::iter_values() - .any(|lockeds| LockedFungiblesWrapper::(lockeds) - .needs_migration(minimal_allowed_xcm_version)), + .any(|data| data.needs_migration(minimal_allowed_xcm_version)), TryRuntimeError::Other( "`LockedFungibles` data should be migrated to the higher xcm version!" ) ); + // check `RemoteLockedFungibles` + ensure!( + !RemoteLockedFungibles::::iter() + .any(|(key, data)| key + .needs_migration(minimal_allowed_xcm_version) || data.needs_migration(minimal_allowed_xcm_version)), + TryRuntimeError::Other( + "`RemoteLockedFungibles` data should be migrated to the higher xcm version!" + ) + ); + // if migration has been already scheduled, everything is ok and data will be eventually // migrated if CurrentMigration::::exists() { @@ -3515,36 +3526,3 @@ impl> ConvertOrigin } } } -pub trait NeedsMigration { - fn needs_migration(&self, minimal_allowed_xcm_version: XcmVersion) -> bool; -} - -struct LockedFungiblesWrapper( - BoundedVec<(BalanceOf, VersionedLocation), T::MaxLockers>, -); -impl NeedsMigration for LockedFungiblesWrapper { - - fn needs_migration(&self, minimal_allowed_xcm_version: XcmVersion) -> bool { - self.0 - .iter() - .any(|(_, unlocker)| unlocker.identify_version() < minimal_allowed_xcm_version) - } -} - -impl NeedsMigration for QueryStatus { - - fn needs_migration(&self, minimal_allowed_xcm_version: XcmVersion) -> bool { - match &self { - QueryStatus::Pending { responder, maybe_match_querier, .. } => - responder.identify_version() < minimal_allowed_xcm_version || - maybe_match_querier - .as_ref() - .map(|v| v.identify_version() < minimal_allowed_xcm_version) - .unwrap_or(false), - QueryStatus::VersionNotifier { origin, .. } => - origin.identify_version() < minimal_allowed_xcm_version, - QueryStatus::Ready { response, .. } => - response.identify_version() < minimal_allowed_xcm_version, - } - } -} diff --git a/polkadot/xcm/pallet-xcm/src/migration.rs b/polkadot/xcm/pallet-xcm/src/migration.rs index 2c5b2620f535..105dca8c2a24 100644 --- a/polkadot/xcm/pallet-xcm/src/migration.rs +++ b/polkadot/xcm/pallet-xcm/src/migration.rs @@ -14,9 +14,7 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . -use crate::{ - pallet::CurrentMigration, Config, Pallet, VersionMigrationStage, VersionNotifyTargets, -}; +use crate::{pallet::CurrentMigration, Config, CurrentXcmVersion, Pallet, VersionMigrationStage, VersionNotifyTargets}; use frame_support::{ pallet_prelude::*, traits::{OnRuntimeUpgrade, StorageVersion, UncheckedOnRuntimeUpgrade}, @@ -25,6 +23,295 @@ use frame_support::{ const DEFAULT_PROOF_SIZE: u64 = 64 * 1024; +/// Utilities for handling XCM version migration for the relevant data. +pub mod data { + use crate::*; + + /// A trait for handling XCM versioned data migration for the requested `XcmVersion`. + pub(crate) trait NeedsMigration { + type MigratedData; + + /// Returns true if data does not match `minimal_allowed_xcm_version`. + fn needs_migration(&self, minimal_allowed_xcm_version: XcmVersion) -> bool; + + /// Attempts to migrate data. `Ok(None)` means no migration is needed. `Ok(Some(Self::MigratedData))` should contain the migrated data. + fn try_migrate(self, to_xcm_version: XcmVersion) -> Result, ()>; + } + + /// Implementation of `NeedsMigration` for `LockedFungibles` data. + impl NeedsMigration for BoundedVec<(B, VersionedLocation), M> { + type MigratedData = Self; + + fn needs_migration(&self, minimal_allowed_xcm_version: XcmVersion) -> bool { + self.iter() + .any(|(_, unlocker)| unlocker.identify_version() < minimal_allowed_xcm_version) + } + + fn try_migrate(mut self, to_xcm_version: XcmVersion) -> Result, ()> { + let mut was_modified = false; + for locked in self.iter_mut() { + if locked.1.identify_version() < to_xcm_version { + let Ok(new_unlocker) = locked.1.clone().into_version(to_xcm_version) else { + return Err(()) + }; + locked.1 = new_unlocker; + was_modified = true; + } + } + + if was_modified { + Ok(Some(self)) + } else { + Ok(None) + } + } + } + + /// Implementation of `NeedsMigration` for `Queries` data. + impl NeedsMigration for QueryStatus { + type MigratedData = Self; + + fn needs_migration(&self, minimal_allowed_xcm_version: XcmVersion) -> bool { + match &self { + QueryStatus::Pending { responder, maybe_match_querier, .. } => + responder.identify_version() < minimal_allowed_xcm_version || + maybe_match_querier + .as_ref() + .map(|v| v.identify_version() < minimal_allowed_xcm_version) + .unwrap_or(false), + QueryStatus::VersionNotifier { origin, .. } => + origin.identify_version() < minimal_allowed_xcm_version, + QueryStatus::Ready { response, .. } => + response.identify_version() < minimal_allowed_xcm_version, + } + } + + fn try_migrate(self, to_xcm_version: XcmVersion) -> Result, ()> { + if !self.needs_migration(to_xcm_version) { + return Ok(None) + } + + // do migration + match self { + QueryStatus::Pending { responder, maybe_match_querier, maybe_notify, timeout } => { + let Ok(responder) = responder.into_version(to_xcm_version) else { return Err(()) }; + let Ok(maybe_match_querier) = + maybe_match_querier.map(|mmq| mmq.into_version(to_xcm_version)).transpose() + else { + return Err(()) + }; + Ok(Some(QueryStatus::Pending { + responder, + maybe_match_querier, + maybe_notify, + timeout, + })) + }, + QueryStatus::VersionNotifier { origin, is_active } => origin + .into_version(to_xcm_version) + .map(|origin| Some(QueryStatus::VersionNotifier { origin, is_active })), + QueryStatus::Ready { response, at } => response + .into_version(to_xcm_version) + .map(|response| Some(QueryStatus::Ready { response, at })), + } + } + } + + /// Implementation of `NeedsMigration` for `RemoteLockedFungibles` key type. + impl NeedsMigration for (XcmVersion, A, VersionedAssetId) { + type MigratedData = Self; + + fn needs_migration(&self, minimal_allowed_xcm_version: XcmVersion) -> bool { + self.0 < minimal_allowed_xcm_version || self.2.identify_version() < minimal_allowed_xcm_version + } + + fn try_migrate(self, to_xcm_version: XcmVersion) -> Result, ()> { + if !self.needs_migration(to_xcm_version) { + return Ok(None) + } + + let Ok(asset_id) = self.2.into_version(to_xcm_version) else { return Err(()) }; + Ok(Some((to_xcm_version, self.1, asset_id))) + } + } + + /// Implementation of `NeedsMigration` for `RemoteLockedFungibles` data. + impl> NeedsMigration for RemoteLockedFungibleRecord { + type MigratedData = Self; + + fn needs_migration(&self, minimal_allowed_xcm_version: XcmVersion) -> bool { + self.owner.identify_version() < minimal_allowed_xcm_version || self.locker.identify_version() < minimal_allowed_xcm_version + } + + fn try_migrate(self, to_xcm_version: XcmVersion) -> Result, ()> { + if !self.needs_migration(to_xcm_version) { + return Ok(None) + } + + let RemoteLockedFungibleRecord { + amount, owner, locker, consumers + } = self; + + let Ok(owner) = owner.into_version(to_xcm_version) else { return Err(()) }; + let Ok(locker) = locker.into_version(to_xcm_version) else { return Err(()) }; + + Ok(Some(RemoteLockedFungibleRecord { + amount, + owner, + locker, + consumers, + })) + } + } + + impl Pallet { + + /// Migrates relevant data to the `required_xcm_version`. + pub(crate) fn migrate_data_to_xcm_version( + weight: &mut Weight, + required_xcm_version: XcmVersion, + ) { + const LOG_TARGET: &str = "runtime::xcm::pallet_xcm::migrate_data_to_xcm_version"; + + // check and migrate `Queries` + let queries_to_migrate = Queries::::iter().filter_map(|(id, data)| { + weight.saturating_add(T::DbWeight::get().reads(1)); + match data.try_migrate(required_xcm_version) { + Ok(Some(new_data)) => Some((id, new_data)), + Ok(None) => None, + Err(_) => { + tracing::error!( + target: LOG_TARGET, + ?id, + ?required_xcm_version, + "`Queries` cannot be migrated!" + ); + None + }, + } + }); + for (id, new_data) in queries_to_migrate { + tracing::info!( + target: LOG_TARGET, + query_id = ?id, + ?new_data, + "Migrating `Queries`" + ); + Queries::::insert(id, new_data); + weight.saturating_add(T::DbWeight::get().writes(1)); + } + + // check and migrate `LockedFungibles` + let locked_fungibles_to_migrate = LockedFungibles::::iter().filter_map(|(id, data)| { + weight.saturating_add(T::DbWeight::get().reads(1)); + match data.try_migrate(required_xcm_version) { + Ok(Some(new_data)) => Some((id, new_data)), + Ok(None) => None, + Err(_) => { + tracing::error!( + target: LOG_TARGET, + ?id, + ?required_xcm_version, + "`LockedFungibles` cannot be migrated!" + ); + None + }, + } + }); + for (id, new_data) in locked_fungibles_to_migrate { + tracing::info!( + target: LOG_TARGET, + account_id = ?id, + ?new_data, + "Migrating `LockedFungibles`" + ); + LockedFungibles::::insert(id, new_data); + weight.saturating_add(T::DbWeight::get().writes(1)); + } + + // check and migrate `RemoteLockedFungibles` - 1. step - just data + let remote_locked_fungibles_to_migrate = RemoteLockedFungibles::::iter().filter_map(|(id, data)| { + weight.saturating_add(T::DbWeight::get().reads(1)); + match data.try_migrate(required_xcm_version) { + Ok(Some(new_data)) => Some((id, new_data)), + Ok(None) => None, + Err(_) => { + tracing::error!( + target: LOG_TARGET, + ?id, + ?required_xcm_version, + "`RemoteLockedFungibles` data cannot be migrated!" + ); + None + }, + } + }); + for (id, new_data) in remote_locked_fungibles_to_migrate { + tracing::info!( + target: LOG_TARGET, + key = ?id, + amount = ?new_data.amount, + locker = ?new_data.locker, + owner = ?new_data.owner, + consumers_count = ?new_data.consumers.len(), + "Migrating `RemoteLockedFungibles` data" + ); + RemoteLockedFungibles::::insert(id, new_data); + weight.saturating_add(T::DbWeight::get().writes(1)); + } + + // check and migrate `RemoteLockedFungibles` - 2. step - key + let remote_locked_fungibles_keys_to_migrate = RemoteLockedFungibles::::iter_keys().filter_map(|key| if key.needs_migration(required_xcm_version) { + let old_key = key.clone(); + match key.try_migrate(required_xcm_version) { + Ok(Some(new_key)) => Some((old_key, new_key)), + Ok(None) => None, + Err(_) => { + tracing::error!( + target: LOG_TARGET, + id = ?old_key, + ?required_xcm_version, + "`RemoteLockedFungibles` key cannot be migrated!" + ); + None + }, + } + } else { + None + }); + for (old_key, new_key) in remote_locked_fungibles_keys_to_migrate { + weight.saturating_add(T::DbWeight::get().reads(1)); + // make sure, that we don't override accidentally other data + if RemoteLockedFungibles::::get(&new_key).is_some() { + tracing::error!( + target: LOG_TARGET, + ?old_key, + ?new_key, + "`RemoteLockedFungibles` already contains data for a `new_key`!" + ); + // let's just skip for now, could be potentially caused with missing this migration before (manual clean-up?). + continue; + } + + tracing::info!( + target: LOG_TARGET, + ?old_key, + ?new_key, + "Migrating `RemoteLockedFungibles` key" + ); + + // now we can swap the keys + RemoteLockedFungibles::::swap::<( + NMapKey, + NMapKey, + NMapKey, + ), _, _>(&old_key, &new_key); + weight.saturating_add(T::DbWeight::get().writes(1)); + } + } + } +} + pub mod v1 { use super::*; use crate::{CurrentMigration, VersionMigrationStage}; @@ -84,7 +371,79 @@ pub mod v1 { pub struct MigrateToLatestXcmVersion(core::marker::PhantomData); impl OnRuntimeUpgrade for MigrateToLatestXcmVersion { fn on_runtime_upgrade() -> Weight { + let mut weight = T::DbWeight::get().reads(1); + + // trigger expensive/lazy migration (kind of multi-block) CurrentMigration::::put(VersionMigrationStage::default()); - T::DbWeight::get().writes(1) + weight.saturating_accrue(T::DbWeight::get().writes(1)); + + // migrate other operational data to the latest XCM version in-place + let latest = CurrentXcmVersion::get(); + Pallet::::migrate_data_to_xcm_version(&mut weight, latest); + + weight + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(_: alloc::vec::Vec) -> Result<(), sp_runtime::TryRuntimeError> { + use data::NeedsMigration; + const LOG_TARGET: &str = "runtime::xcm::pallet_xcm::migrate_to_latest"; + + let latest = CurrentXcmVersion::get(); + + let number_of_queries_to_migrate = crate::Queries::::iter().filter(|(id, data)| { + let needs_migration = data.needs_migration(latest); + if needs_migration { + tracing::warn!( + target: LOG_TARGET, + query_id = ?id, + query = ?data, + "Query was not migrated!" + ) + } + needs_migration + }).count(); + + let number_of_locked_fungibles_to_migrate = crate::LockedFungibles::::iter().filter_map(|(id, data)| { + if data.needs_migration(latest) { + tracing::warn!( + target: LOG_TARGET, + ?id, + ?data, + "LockedFungibles item was not migrated!" + ); + Some(true) + } else { + None + } + }).count(); + + let number_of_remote_locked_fungibles_to_migrate = crate::RemoteLockedFungibles::::iter().filter_map(|(key, data)| { + if key.needs_migration(latest) || data.needs_migration(latest) { + tracing::warn!( + target: LOG_TARGET, + ?key, + "RemoteLockedFungibles item was not migrated!" + ); + Some(true) + } else { + None + } + }).count(); + + ensure!( + number_of_queries_to_migrate == 0, + "must migrate all `Queries`." + ); + ensure!( + number_of_locked_fungibles_to_migrate == 0, + "must migrate all `LockedFungibles`." + ); + ensure!( + number_of_remote_locked_fungibles_to_migrate == 0, + "must migrate all `RemoteLockedFungibles`." + ); + + Ok(()) } } diff --git a/polkadot/xcm/pallet-xcm/src/tests/mod.rs b/polkadot/xcm/pallet-xcm/src/tests/mod.rs index c16c1a1ba986..8dff3785c554 100644 --- a/polkadot/xcm/pallet-xcm/src/tests/mod.rs +++ b/polkadot/xcm/pallet-xcm/src/tests/mod.rs @@ -18,12 +18,8 @@ pub(crate) mod assets_transfer; -use crate::{ - mock::*, pallet::SupportedVersion, AssetTraps, Config, CurrentMigration, Error, - ExecuteControllerWeightInfo, LatestVersionedLocation, Pallet, Queries, QueryStatus, - RecordedXcm, ShouldRecordXcm, VersionDiscoveryQueue, VersionMigrationStage, VersionNotifiers, - VersionNotifyTargets, WeightInfo, -}; +use bounded_collections::BoundedVec; +use crate::{mock::*, pallet::SupportedVersion, AssetTraps, Config, CurrentMigration, Error, ExecuteControllerWeightInfo, LatestVersionedLocation, Pallet, Queries, QueryStatus, RecordedXcm, RemoteLockedFungibleRecord, ShouldRecordXcm, VersionDiscoveryQueue, VersionMigrationStage, VersionNotifiers, VersionNotifyTargets, WeightInfo}; use frame_support::{ assert_err_ignore_postinfo, assert_noop, assert_ok, traits::{Currency, Hooks}, @@ -37,6 +33,8 @@ use xcm_executor::{ traits::{Properties, QueryHandler, QueryResponseStatus, ShouldExecute}, XcmExecutor, }; +use crate::migration::data::NeedsMigration; +use crate::pallet::{LockedFungibles, RemoteLockedFungibles}; const ALICE: AccountId = AccountId::new([0u8; 32]); const BOB: AccountId = AccountId::new([1u8; 32]); @@ -1258,6 +1256,146 @@ fn multistage_migration_works() { }) } +#[test] +fn migrate_data_to_xcm_version_works() { + new_test_ext_with_balances(vec![]).execute_with(|| { + // check `try-state` + assert!(Pallet::::do_try_state().is_ok()); + + let latest_version = XCM_VERSION; + let previous_version = XCM_VERSION - 1; + + + // `Queries` migration + { + let origin = VersionedLocation::from(Location::parent()); + let query_id1 = 0; + let query_id2 = 2; + let query_as_latest = QueryStatus::VersionNotifier { origin: origin.clone(), is_active: true }; + let query_as_previous = QueryStatus::VersionNotifier { origin: origin.into_version(previous_version).unwrap(), is_active: true }; + assert_ne!(query_as_latest, query_as_previous); + assert!(!query_as_latest.needs_migration(latest_version)); + assert!(!query_as_latest.needs_migration(previous_version)); + assert!(query_as_previous.needs_migration(latest_version)); + assert!(!query_as_previous.needs_migration(previous_version)); + + // store two queries: migrated and not migrated + Queries::::insert(query_id1, query_as_latest.clone()); + Queries::::insert(query_id2, query_as_previous); + assert!(Pallet::::do_try_state().is_ok()); + + // trigger migration + Pallet::::migrate_data_to_xcm_version(&mut Weight::zero(), latest_version); + + // no change for query_id1 + assert_eq!(Queries::::get(query_id1), Some(query_as_latest.clone())); + // change for query_id2 + assert_eq!(Queries::::get(query_id2), Some(query_as_latest)); + assert!(Pallet::::do_try_state().is_ok()); + } + + // `LockedFungibles` migration + { + let account1 = AccountId::new([13u8; 32]); + let account2 = AccountId::new([58u8; 32]); + let unlocker = VersionedLocation::from(Location::parent()); + let lockeds_as_latest = BoundedVec::truncate_from(vec![(0, unlocker.clone())]); + let lockeds_as_previous = BoundedVec::truncate_from(vec![(0, unlocker.into_version(previous_version).unwrap())]); + assert_ne!(lockeds_as_latest, lockeds_as_previous); + assert!(!lockeds_as_latest.needs_migration(latest_version)); + assert!(!lockeds_as_latest.needs_migration(previous_version)); + assert!(lockeds_as_previous.needs_migration(latest_version)); + assert!(!lockeds_as_previous.needs_migration(previous_version)); + + // store two lockeds: migrated and not migrated + LockedFungibles::::insert(&account1, lockeds_as_latest.clone()); + LockedFungibles::::insert(&account2, lockeds_as_previous); + assert!(Pallet::::do_try_state().is_ok()); + + // trigger migration + Pallet::::migrate_data_to_xcm_version(&mut Weight::zero(), latest_version); + + // no change for account1 + assert_eq!(LockedFungibles::::get(&account1), Some(lockeds_as_latest.clone())); + // change for account2 + assert_eq!(LockedFungibles::::get(&account2), Some(lockeds_as_latest)); + assert!(Pallet::::do_try_state().is_ok()); + } + + // `RemoteLockedFungibles` migration + { + let account1 = AccountId::new([13u8; 32]); + let account2 = AccountId::new([58u8; 32]); + let account3 = AccountId::new([97u8; 32]); + let asset_id = VersionedAssetId::from(AssetId(Location::parent())); + let owner = VersionedLocation::from(Location::parent()); + let locker = VersionedLocation::from(Location::parent()); + let key1_as_latest = (latest_version, account1, asset_id.clone()); + let key2_as_latest = (latest_version, account2, asset_id.clone()); + let key3_as_previous = (previous_version, account3.clone(), asset_id.clone().into_version(previous_version).unwrap()); + let expected_key3_as_latest = (latest_version, account3, asset_id); + let data_as_latest = RemoteLockedFungibleRecord { + amount: Default::default(), + owner: owner.clone(), + locker: locker.clone(), + consumers: Default::default(), + }; + let data_as_previous = RemoteLockedFungibleRecord { + amount: Default::default(), + owner: owner.into_version(previous_version).unwrap(), + locker: locker.into_version(previous_version).unwrap(), + consumers: Default::default(), + }; + assert_ne!(data_as_latest.owner, data_as_previous.owner); + assert_ne!(data_as_latest.locker, data_as_previous.locker); + assert!(!key1_as_latest.needs_migration(latest_version)); + assert!(!key1_as_latest.needs_migration(previous_version)); + assert!(!key2_as_latest.needs_migration(latest_version)); + assert!(!key2_as_latest.needs_migration(previous_version)); + assert!(key3_as_previous.needs_migration(latest_version)); + assert!(!key3_as_previous.needs_migration(previous_version)); + assert!(!expected_key3_as_latest.needs_migration(latest_version)); + assert!(!expected_key3_as_latest.needs_migration(previous_version)); + assert!(!data_as_latest.needs_migration(latest_version)); + assert!(!data_as_latest.needs_migration(previous_version)); + assert!(data_as_previous.needs_migration(latest_version)); + assert!(!data_as_previous.needs_migration(previous_version)); + + // store three lockeds: + // fully migrated + RemoteLockedFungibles::::insert(&key1_as_latest, data_as_latest.clone()); + // only key migrated + RemoteLockedFungibles::::insert(&key2_as_latest, data_as_previous.clone()); + // neither key nor data migrated + RemoteLockedFungibles::::insert(&key3_as_previous, data_as_previous); + assert!(Pallet::::do_try_state().is_ok()); + + // trigger migration + Pallet::::migrate_data_to_xcm_version(&mut Weight::zero(), latest_version); + + let assert_locked_eq = |left: Option>, right: Option>| { + match (left, right) { + (None, Some(_)) | (Some(_), None) => assert!(false, "Received unexpected message"), + (None, None) => (), + (Some(l), Some(r)) => { + assert_eq!(l.owner, r.owner); + assert_eq!(l.locker, r.locker); + } + } + }; + + // no change + assert_locked_eq(RemoteLockedFungibles::::get(&key1_as_latest), Some(data_as_latest.clone())); + // change - data migrated + assert_locked_eq(RemoteLockedFungibles::::get(&key2_as_latest), Some(data_as_latest.clone())); + // fully migrated + assert_locked_eq(RemoteLockedFungibles::::get(&key3_as_previous), None); + assert_locked_eq(RemoteLockedFungibles::::get(&expected_key3_as_latest), Some(data_as_latest.clone())); + assert!(Pallet::::do_try_state().is_ok()); + } + }) +} + #[test] fn record_xcm_works() { let balances = vec![(ALICE, INITIAL_BALANCE)]; From e2b1bd3d5f07250db460f40e4c8e3d39a7499301 Mon Sep 17 00:00:00 2001 From: command-bot <> Date: Wed, 23 Oct 2024 09:39:03 +0000 Subject: [PATCH 08/11] ".git/.scripts/commands/fmt/fmt.sh" --- polkadot/xcm/pallet-xcm/src/lib.rs | 4 +- polkadot/xcm/pallet-xcm/src/migration.rs | 260 ++++++++++++----------- polkadot/xcm/pallet-xcm/src/tests/mod.rs | 68 ++++-- 3 files changed, 188 insertions(+), 144 deletions(-) diff --git a/polkadot/xcm/pallet-xcm/src/lib.rs b/polkadot/xcm/pallet-xcm/src/lib.rs index 701d987e8830..1c5392abfb01 100644 --- a/polkadot/xcm/pallet-xcm/src/lib.rs +++ b/polkadot/xcm/pallet-xcm/src/lib.rs @@ -2838,8 +2838,8 @@ impl Pallet { // check `RemoteLockedFungibles` ensure!( !RemoteLockedFungibles::::iter() - .any(|(key, data)| key - .needs_migration(minimal_allowed_xcm_version) || data.needs_migration(minimal_allowed_xcm_version)), + .any(|(key, data)| key.needs_migration(minimal_allowed_xcm_version) || + data.needs_migration(minimal_allowed_xcm_version)), TryRuntimeError::Other( "`RemoteLockedFungibles` data should be migrated to the higher xcm version!" ) diff --git a/polkadot/xcm/pallet-xcm/src/migration.rs b/polkadot/xcm/pallet-xcm/src/migration.rs index 105dca8c2a24..80154f57ddfb 100644 --- a/polkadot/xcm/pallet-xcm/src/migration.rs +++ b/polkadot/xcm/pallet-xcm/src/migration.rs @@ -14,7 +14,10 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . -use crate::{pallet::CurrentMigration, Config, CurrentXcmVersion, Pallet, VersionMigrationStage, VersionNotifyTargets}; +use crate::{ + pallet::CurrentMigration, Config, CurrentXcmVersion, Pallet, VersionMigrationStage, + VersionNotifyTargets, +}; use frame_support::{ pallet_prelude::*, traits::{OnRuntimeUpgrade, StorageVersion, UncheckedOnRuntimeUpgrade}, @@ -34,7 +37,8 @@ pub mod data { /// Returns true if data does not match `minimal_allowed_xcm_version`. fn needs_migration(&self, minimal_allowed_xcm_version: XcmVersion) -> bool; - /// Attempts to migrate data. `Ok(None)` means no migration is needed. `Ok(Some(Self::MigratedData))` should contain the migrated data. + /// Attempts to migrate data. `Ok(None)` means no migration is needed. + /// `Ok(Some(Self::MigratedData))` should contain the migrated data. fn try_migrate(self, to_xcm_version: XcmVersion) -> Result, ()>; } @@ -47,7 +51,10 @@ pub mod data { .any(|(_, unlocker)| unlocker.identify_version() < minimal_allowed_xcm_version) } - fn try_migrate(mut self, to_xcm_version: XcmVersion) -> Result, ()> { + fn try_migrate( + mut self, + to_xcm_version: XcmVersion, + ) -> Result, ()> { let mut was_modified = false; for locked in self.iter_mut() { if locked.1.identify_version() < to_xcm_version { @@ -94,7 +101,9 @@ pub mod data { // do migration match self { QueryStatus::Pending { responder, maybe_match_querier, maybe_notify, timeout } => { - let Ok(responder) = responder.into_version(to_xcm_version) else { return Err(()) }; + let Ok(responder) = responder.into_version(to_xcm_version) else { + return Err(()) + }; let Ok(maybe_match_querier) = maybe_match_querier.map(|mmq| mmq.into_version(to_xcm_version)).transpose() else { @@ -122,7 +131,8 @@ pub mod data { type MigratedData = Self; fn needs_migration(&self, minimal_allowed_xcm_version: XcmVersion) -> bool { - self.0 < minimal_allowed_xcm_version || self.2.identify_version() < minimal_allowed_xcm_version + self.0 < minimal_allowed_xcm_version || + self.2.identify_version() < minimal_allowed_xcm_version } fn try_migrate(self, to_xcm_version: XcmVersion) -> Result, ()> { @@ -136,11 +146,14 @@ pub mod data { } /// Implementation of `NeedsMigration` for `RemoteLockedFungibles` data. - impl> NeedsMigration for RemoteLockedFungibleRecord { + impl> NeedsMigration + for RemoteLockedFungibleRecord + { type MigratedData = Self; fn needs_migration(&self, minimal_allowed_xcm_version: XcmVersion) -> bool { - self.owner.identify_version() < minimal_allowed_xcm_version || self.locker.identify_version() < minimal_allowed_xcm_version + self.owner.identify_version() < minimal_allowed_xcm_version || + self.locker.identify_version() < minimal_allowed_xcm_version } fn try_migrate(self, to_xcm_version: XcmVersion) -> Result, ()> { @@ -148,24 +161,16 @@ pub mod data { return Ok(None) } - let RemoteLockedFungibleRecord { - amount, owner, locker, consumers - } = self; + let RemoteLockedFungibleRecord { amount, owner, locker, consumers } = self; let Ok(owner) = owner.into_version(to_xcm_version) else { return Err(()) }; let Ok(locker) = locker.into_version(to_xcm_version) else { return Err(()) }; - Ok(Some(RemoteLockedFungibleRecord { - amount, - owner, - locker, - consumers, - })) + Ok(Some(RemoteLockedFungibleRecord { amount, owner, locker, consumers })) } } impl Pallet { - /// Migrates relevant data to the `required_xcm_version`. pub(crate) fn migrate_data_to_xcm_version( weight: &mut Weight, @@ -187,7 +192,7 @@ pub mod data { "`Queries` cannot be migrated!" ); None - }, + }, } }); for (id, new_data) in queries_to_migrate { @@ -202,22 +207,23 @@ pub mod data { } // check and migrate `LockedFungibles` - let locked_fungibles_to_migrate = LockedFungibles::::iter().filter_map(|(id, data)| { - weight.saturating_add(T::DbWeight::get().reads(1)); - match data.try_migrate(required_xcm_version) { - Ok(Some(new_data)) => Some((id, new_data)), - Ok(None) => None, - Err(_) => { - tracing::error!( - target: LOG_TARGET, - ?id, - ?required_xcm_version, - "`LockedFungibles` cannot be migrated!" - ); - None - }, - } - }); + let locked_fungibles_to_migrate = + LockedFungibles::::iter().filter_map(|(id, data)| { + weight.saturating_add(T::DbWeight::get().reads(1)); + match data.try_migrate(required_xcm_version) { + Ok(Some(new_data)) => Some((id, new_data)), + Ok(None) => None, + Err(_) => { + tracing::error!( + target: LOG_TARGET, + ?id, + ?required_xcm_version, + "`LockedFungibles` cannot be migrated!" + ); + None + }, + } + }); for (id, new_data) in locked_fungibles_to_migrate { tracing::info!( target: LOG_TARGET, @@ -230,22 +236,23 @@ pub mod data { } // check and migrate `RemoteLockedFungibles` - 1. step - just data - let remote_locked_fungibles_to_migrate = RemoteLockedFungibles::::iter().filter_map(|(id, data)| { - weight.saturating_add(T::DbWeight::get().reads(1)); - match data.try_migrate(required_xcm_version) { - Ok(Some(new_data)) => Some((id, new_data)), - Ok(None) => None, - Err(_) => { - tracing::error!( - target: LOG_TARGET, - ?id, - ?required_xcm_version, - "`RemoteLockedFungibles` data cannot be migrated!" - ); - None - }, - } - }); + let remote_locked_fungibles_to_migrate = + RemoteLockedFungibles::::iter().filter_map(|(id, data)| { + weight.saturating_add(T::DbWeight::get().reads(1)); + match data.try_migrate(required_xcm_version) { + Ok(Some(new_data)) => Some((id, new_data)), + Ok(None) => None, + Err(_) => { + tracing::error!( + target: LOG_TARGET, + ?id, + ?required_xcm_version, + "`RemoteLockedFungibles` data cannot be migrated!" + ); + None + }, + } + }); for (id, new_data) in remote_locked_fungibles_to_migrate { tracing::info!( target: LOG_TARGET, @@ -261,24 +268,27 @@ pub mod data { } // check and migrate `RemoteLockedFungibles` - 2. step - key - let remote_locked_fungibles_keys_to_migrate = RemoteLockedFungibles::::iter_keys().filter_map(|key| if key.needs_migration(required_xcm_version) { - let old_key = key.clone(); - match key.try_migrate(required_xcm_version) { - Ok(Some(new_key)) => Some((old_key, new_key)), - Ok(None) => None, - Err(_) => { - tracing::error!( - target: LOG_TARGET, - id = ?old_key, - ?required_xcm_version, - "`RemoteLockedFungibles` key cannot be migrated!" - ); + let remote_locked_fungibles_keys_to_migrate = RemoteLockedFungibles::::iter_keys() + .filter_map(|key| { + if key.needs_migration(required_xcm_version) { + let old_key = key.clone(); + match key.try_migrate(required_xcm_version) { + Ok(Some(new_key)) => Some((old_key, new_key)), + Ok(None) => None, + Err(_) => { + tracing::error!( + target: LOG_TARGET, + id = ?old_key, + ?required_xcm_version, + "`RemoteLockedFungibles` key cannot be migrated!" + ); + None + }, + } + } else { None - }, - } - } else { - None - }); + } + }); for (old_key, new_key) in remote_locked_fungibles_keys_to_migrate { weight.saturating_add(T::DbWeight::get().reads(1)); // make sure, that we don't override accidentally other data @@ -289,7 +299,8 @@ pub mod data { ?new_key, "`RemoteLockedFungibles` already contains data for a `new_key`!" ); - // let's just skip for now, could be potentially caused with missing this migration before (manual clean-up?). + // let's just skip for now, could be potentially caused with missing this + // migration before (manual clean-up?). continue; } @@ -301,11 +312,15 @@ pub mod data { ); // now we can swap the keys - RemoteLockedFungibles::::swap::<( - NMapKey, - NMapKey, - NMapKey, - ), _, _>(&old_key, &new_key); + RemoteLockedFungibles::::swap::< + ( + NMapKey, + NMapKey, + NMapKey, + ), + _, + _, + >(&old_key, &new_key); weight.saturating_add(T::DbWeight::get().writes(1)); } } @@ -391,57 +406,58 @@ impl OnRuntimeUpgrade for MigrateToLatestXcmVersion { let latest = CurrentXcmVersion::get(); - let number_of_queries_to_migrate = crate::Queries::::iter().filter(|(id, data)| { - let needs_migration = data.needs_migration(latest); - if needs_migration { - tracing::warn!( - target: LOG_TARGET, - query_id = ?id, - query = ?data, - "Query was not migrated!" - ) - } - needs_migration - }).count(); - - let number_of_locked_fungibles_to_migrate = crate::LockedFungibles::::iter().filter_map(|(id, data)| { - if data.needs_migration(latest) { - tracing::warn!( - target: LOG_TARGET, - ?id, - ?data, - "LockedFungibles item was not migrated!" - ); - Some(true) - } else { - None - } - }).count(); - - let number_of_remote_locked_fungibles_to_migrate = crate::RemoteLockedFungibles::::iter().filter_map(|(key, data)| { - if key.needs_migration(latest) || data.needs_migration(latest) { - tracing::warn!( - target: LOG_TARGET, - ?key, - "RemoteLockedFungibles item was not migrated!" - ); - Some(true) - } else { - None - } - }).count(); + let number_of_queries_to_migrate = crate::Queries::::iter() + .filter(|(id, data)| { + let needs_migration = data.needs_migration(latest); + if needs_migration { + tracing::warn!( + target: LOG_TARGET, + query_id = ?id, + query = ?data, + "Query was not migrated!" + ) + } + needs_migration + }) + .count(); + + let number_of_locked_fungibles_to_migrate = crate::LockedFungibles::::iter() + .filter_map(|(id, data)| { + if data.needs_migration(latest) { + tracing::warn!( + target: LOG_TARGET, + ?id, + ?data, + "LockedFungibles item was not migrated!" + ); + Some(true) + } else { + None + } + }) + .count(); + + let number_of_remote_locked_fungibles_to_migrate = + crate::RemoteLockedFungibles::::iter() + .filter_map(|(key, data)| { + if key.needs_migration(latest) || data.needs_migration(latest) { + tracing::warn!( + target: LOG_TARGET, + ?key, + "RemoteLockedFungibles item was not migrated!" + ); + Some(true) + } else { + None + } + }) + .count(); + ensure!(number_of_queries_to_migrate == 0, "must migrate all `Queries`."); + ensure!(number_of_locked_fungibles_to_migrate == 0, "must migrate all `LockedFungibles`."); ensure!( - number_of_queries_to_migrate == 0, - "must migrate all `Queries`." - ); - ensure!( - number_of_locked_fungibles_to_migrate == 0, - "must migrate all `LockedFungibles`." - ); - ensure!( - number_of_remote_locked_fungibles_to_migrate == 0, - "must migrate all `RemoteLockedFungibles`." + number_of_remote_locked_fungibles_to_migrate == 0, + "must migrate all `RemoteLockedFungibles`." ); Ok(()) diff --git a/polkadot/xcm/pallet-xcm/src/tests/mod.rs b/polkadot/xcm/pallet-xcm/src/tests/mod.rs index 8dff3785c554..e98a8f8d2ce7 100644 --- a/polkadot/xcm/pallet-xcm/src/tests/mod.rs +++ b/polkadot/xcm/pallet-xcm/src/tests/mod.rs @@ -18,8 +18,16 @@ pub(crate) mod assets_transfer; +use crate::{ + migration::data::NeedsMigration, + mock::*, + pallet::{LockedFungibles, RemoteLockedFungibles, SupportedVersion}, + AssetTraps, Config, CurrentMigration, Error, ExecuteControllerWeightInfo, + LatestVersionedLocation, Pallet, Queries, QueryStatus, RecordedXcm, RemoteLockedFungibleRecord, + ShouldRecordXcm, VersionDiscoveryQueue, VersionMigrationStage, VersionNotifiers, + VersionNotifyTargets, WeightInfo, +}; use bounded_collections::BoundedVec; -use crate::{mock::*, pallet::SupportedVersion, AssetTraps, Config, CurrentMigration, Error, ExecuteControllerWeightInfo, LatestVersionedLocation, Pallet, Queries, QueryStatus, RecordedXcm, RemoteLockedFungibleRecord, ShouldRecordXcm, VersionDiscoveryQueue, VersionMigrationStage, VersionNotifiers, VersionNotifyTargets, WeightInfo}; use frame_support::{ assert_err_ignore_postinfo, assert_noop, assert_ok, traits::{Currency, Hooks}, @@ -33,8 +41,6 @@ use xcm_executor::{ traits::{Properties, QueryHandler, QueryResponseStatus, ShouldExecute}, XcmExecutor, }; -use crate::migration::data::NeedsMigration; -use crate::pallet::{LockedFungibles, RemoteLockedFungibles}; const ALICE: AccountId = AccountId::new([0u8; 32]); const BOB: AccountId = AccountId::new([1u8; 32]); @@ -1265,14 +1271,17 @@ fn migrate_data_to_xcm_version_works() { let latest_version = XCM_VERSION; let previous_version = XCM_VERSION - 1; - // `Queries` migration { let origin = VersionedLocation::from(Location::parent()); let query_id1 = 0; let query_id2 = 2; - let query_as_latest = QueryStatus::VersionNotifier { origin: origin.clone(), is_active: true }; - let query_as_previous = QueryStatus::VersionNotifier { origin: origin.into_version(previous_version).unwrap(), is_active: true }; + let query_as_latest = + QueryStatus::VersionNotifier { origin: origin.clone(), is_active: true }; + let query_as_previous = QueryStatus::VersionNotifier { + origin: origin.into_version(previous_version).unwrap(), + is_active: true, + }; assert_ne!(query_as_latest, query_as_previous); assert!(!query_as_latest.needs_migration(latest_version)); assert!(!query_as_latest.needs_migration(previous_version)); @@ -1300,7 +1309,10 @@ fn migrate_data_to_xcm_version_works() { let account2 = AccountId::new([58u8; 32]); let unlocker = VersionedLocation::from(Location::parent()); let lockeds_as_latest = BoundedVec::truncate_from(vec![(0, unlocker.clone())]); - let lockeds_as_previous = BoundedVec::truncate_from(vec![(0, unlocker.into_version(previous_version).unwrap())]); + let lockeds_as_previous = BoundedVec::truncate_from(vec![( + 0, + unlocker.into_version(previous_version).unwrap(), + )]); assert_ne!(lockeds_as_latest, lockeds_as_previous); assert!(!lockeds_as_latest.needs_migration(latest_version)); assert!(!lockeds_as_latest.needs_migration(previous_version)); @@ -1332,7 +1344,11 @@ fn migrate_data_to_xcm_version_works() { let locker = VersionedLocation::from(Location::parent()); let key1_as_latest = (latest_version, account1, asset_id.clone()); let key2_as_latest = (latest_version, account2, asset_id.clone()); - let key3_as_previous = (previous_version, account3.clone(), asset_id.clone().into_version(previous_version).unwrap()); + let key3_as_previous = ( + previous_version, + account3.clone(), + asset_id.clone().into_version(previous_version).unwrap(), + ); let expected_key3_as_latest = (latest_version, account3, asset_id); let data_as_latest = RemoteLockedFungibleRecord { amount: Default::default(), @@ -1373,24 +1389,36 @@ fn migrate_data_to_xcm_version_works() { // trigger migration Pallet::::migrate_data_to_xcm_version(&mut Weight::zero(), latest_version); - let assert_locked_eq = |left: Option>, right: Option>| { - match (left, right) { - (None, Some(_)) | (Some(_), None) => assert!(false, "Received unexpected message"), - (None, None) => (), - (Some(l), Some(r)) => { - assert_eq!(l.owner, r.owner); - assert_eq!(l.locker, r.locker); + let assert_locked_eq = + |left: Option>, + right: Option>| { + match (left, right) { + (None, Some(_)) | (Some(_), None) => + assert!(false, "Received unexpected message"), + (None, None) => (), + (Some(l), Some(r)) => { + assert_eq!(l.owner, r.owner); + assert_eq!(l.locker, r.locker); + }, } - } - }; + }; // no change - assert_locked_eq(RemoteLockedFungibles::::get(&key1_as_latest), Some(data_as_latest.clone())); + assert_locked_eq( + RemoteLockedFungibles::::get(&key1_as_latest), + Some(data_as_latest.clone()), + ); // change - data migrated - assert_locked_eq(RemoteLockedFungibles::::get(&key2_as_latest), Some(data_as_latest.clone())); + assert_locked_eq( + RemoteLockedFungibles::::get(&key2_as_latest), + Some(data_as_latest.clone()), + ); // fully migrated assert_locked_eq(RemoteLockedFungibles::::get(&key3_as_previous), None); - assert_locked_eq(RemoteLockedFungibles::::get(&expected_key3_as_latest), Some(data_as_latest.clone())); + assert_locked_eq( + RemoteLockedFungibles::::get(&expected_key3_as_latest), + Some(data_as_latest.clone()), + ); assert!(Pallet::::do_try_state().is_ok()); } }) From e408d3f5906a5bfb3778490dbb37816b0b688b6e Mon Sep 17 00:00:00 2001 From: GitHub Action Date: Wed, 23 Oct 2024 09:50:34 +0000 Subject: [PATCH 09/11] Update from bkontur running command 'prdoc' --- prdoc/pr_6148.prdoc | 50 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 prdoc/pr_6148.prdoc diff --git a/prdoc/pr_6148.prdoc b/prdoc/pr_6148.prdoc new file mode 100644 index 000000000000..30be8e31295a --- /dev/null +++ b/prdoc/pr_6148.prdoc @@ -0,0 +1,50 @@ +title: Fix migrations for pallet-xcm +doc: +- audience: Todo + description: |- + Relates to: https://github.com/paritytech/polkadot-sdk/pull/4826 + Relates to: https://github.com/paritytech/polkadot-sdk/issues/3214 + + ## Description + + `pallet-xcm` stores some operational data that uses `Versioned*` XCM types. When we add a new XCM version (XV), we deprecate XV-2 and remove XV-3. Without proper migration, this can lead to issues with [undecodable storage](https://github.com/paritytech/polkadot-sdk/actions/runs/11381324568/job/31662577532?pr=6092), as was identified on the XCMv5 branch where XCMv2 was removed. + + This PR extends the existing `MigrateToLatestXcmVersion` to include migration for the `Queries`, `LockedFungibles`, and `RemoteLockedFungibles` storage types. Additionally, more checks were added to `try_state` for these types. + + ## TODO + - [ ] create tracking issue for `polkadot-fellows` + - [x] Add missing `MigrateToLatestXcmVersion` for westend + - [x] fix pallet-xcm `Queries` + - fails for Westend https://github.com/paritytech/polkadot-sdk/actions/runs/11381324568/job/31662577532?pr=6092 + - `V2` was removed from `Versioned*` stuff, but we have a live data with V2 e.g. Queries - e.g. Kusama or Polkadot relay chains + ``` + VersionNotifier: { + origin: { + V2: { + parents: 0 + interior: { + X1: { + Parachain: 2,124 + } + } + } + } + isActive: true + } + ``` + ![image](https://github.com/user-attachments/assets/f59f761b-46a7-4def-8aea-45c4e41c0a00) + - [x] fix also for `RemoteLockedFungibles` + - [x] fix also for `LockedFungibles` + + ## Follow-ups + + https://github.com/paritytech/polkadot-sdk/issues/6188 +crates: +- name: westend-runtime + bump: major +- name: staging-xcm-builder + bump: major +- name: xcm-runtime-apis + bump: major +- name: pallet-xcm + bump: major From 6b8a0ebf85c0ee396437758c0f1de41a3011ac85 Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Wed, 23 Oct 2024 11:57:37 +0200 Subject: [PATCH 10/11] fixed prdoc --- prdoc/pr_6148.prdoc | 49 ++++++++------------------------------------- 1 file changed, 8 insertions(+), 41 deletions(-) diff --git a/prdoc/pr_6148.prdoc b/prdoc/pr_6148.prdoc index 30be8e31295a..430a58dfefbb 100644 --- a/prdoc/pr_6148.prdoc +++ b/prdoc/pr_6148.prdoc @@ -1,50 +1,17 @@ title: Fix migrations for pallet-xcm doc: -- audience: Todo +- audience: Runtime Dev description: |- - Relates to: https://github.com/paritytech/polkadot-sdk/pull/4826 - Relates to: https://github.com/paritytech/polkadot-sdk/issues/3214 + `pallet-xcm` stores some operational data that uses `Versioned*` XCM types. When we add a new XCM version (XV), we deprecate XV-2 and remove XV-3. + This PR extends the existing `MigrateToLatestXcmVersion` to include migration for the `Queries`, `LockedFungibles`, and `RemoteLockedFungibles` storage types. + Additionally, more checks were added to `try_state` for these types. - ## Description - - `pallet-xcm` stores some operational data that uses `Versioned*` XCM types. When we add a new XCM version (XV), we deprecate XV-2 and remove XV-3. Without proper migration, this can lead to issues with [undecodable storage](https://github.com/paritytech/polkadot-sdk/actions/runs/11381324568/job/31662577532?pr=6092), as was identified on the XCMv5 branch where XCMv2 was removed. - - This PR extends the existing `MigrateToLatestXcmVersion` to include migration for the `Queries`, `LockedFungibles`, and `RemoteLockedFungibles` storage types. Additionally, more checks were added to `try_state` for these types. - - ## TODO - - [ ] create tracking issue for `polkadot-fellows` - - [x] Add missing `MigrateToLatestXcmVersion` for westend - - [x] fix pallet-xcm `Queries` - - fails for Westend https://github.com/paritytech/polkadot-sdk/actions/runs/11381324568/job/31662577532?pr=6092 - - `V2` was removed from `Versioned*` stuff, but we have a live data with V2 e.g. Queries - e.g. Kusama or Polkadot relay chains - ``` - VersionNotifier: { - origin: { - V2: { - parents: 0 - interior: { - X1: { - Parachain: 2,124 - } - } - } - } - isActive: true - } - ``` - ![image](https://github.com/user-attachments/assets/f59f761b-46a7-4def-8aea-45c4e41c0a00) - - [x] fix also for `RemoteLockedFungibles` - - [x] fix also for `LockedFungibles` - - ## Follow-ups - - https://github.com/paritytech/polkadot-sdk/issues/6188 crates: - name: westend-runtime - bump: major + bump: patch - name: staging-xcm-builder - bump: major + bump: none - name: xcm-runtime-apis - bump: major + bump: none - name: pallet-xcm - bump: major + bump: patch From a3fc9aff9ebd7e04eac7234f9af1ef0dae14de81 Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Wed, 23 Oct 2024 15:49:07 +0200 Subject: [PATCH 11/11] Update polkadot/xcm/pallet-xcm/src/lib.rs Co-authored-by: Francisco Aguirre --- polkadot/xcm/pallet-xcm/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/xcm/pallet-xcm/src/lib.rs b/polkadot/xcm/pallet-xcm/src/lib.rs index 1c5392abfb01..9b8f735b478f 100644 --- a/polkadot/xcm/pallet-xcm/src/lib.rs +++ b/polkadot/xcm/pallet-xcm/src/lib.rs @@ -2809,7 +2809,7 @@ impl Pallet { pub fn do_try_state() -> Result<(), TryRuntimeError> { use migration::data::NeedsMigration; - // Take minimal version from `SafeXcmVersion` or `latest - 1` and ensure that the + // Take the minimum version between `SafeXcmVersion` and `latest - 1` and ensure that the // operational data is stored at least at that version, for example, to prevent issues when // removing older XCM versions. let minimal_allowed_xcm_version = if let Some(safe_xcm_version) = SafeXcmVersion::::get()