Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[xcm/pallet_xcm] New XCM version change improvements #3214

Open
2 of 10 tasks
bkontur opened this issue Feb 5, 2024 · 0 comments
Open
2 of 10 tasks

[xcm/pallet_xcm] New XCM version change improvements #3214

bkontur opened this issue Feb 5, 2024 · 0 comments
Assignees
Labels
T6-XCM This PR/Issue is related to XCM.

Comments

@bkontur
Copy link
Contributor

bkontur commented Feb 5, 2024

Summary

All the xcm-builder and xcm-executor modules are implemented with xcm::latest::prelude::* or xcm::latest::*. This means that when we add a new XCM version to the xcm module and change latest to point to the new XCM version, it affects everything.

One of the affected aspects is the XCM version handling in the pallet_xcm, where we store operational data with XcmVersion (e.g., SupportedVersion). Currently, all data use xcm::latest::XCM_VERSION (e.g., destination XCM versions). The only way to migrate these data is to set CurrentMigration::<T>::put(VersionMigrationStage::default());, but this is not used anywhere, only in the old migration, which is not used.

One consequence is that the "notify subscribers when the XCM version changes" feature does not work without proper migration. Yes, we have force_xcm_version or fn wrap_version with fn note_unknown_version (we have one scenario for bridges where we don't use wrap_version + note_unknown_version, because SubscribeVersion does not work over bridge - issue), which could help and/or trigger negotiation from scratch, but then the "version notify" and SubscribeVersion become somewhat useless.

We didn't encounter this issue with XCMv3 because a migration was added to the runtimes here.

Example of problem/issue

We encountered this issue on BridgeHubRococo. However, when we upgrade all testnet runtimes to version 1.7.0, the problem will persist across all instances. In our case, we stored some XCM versions in SupportedVersion with XcmVersion=3. The BridgeHubRococo was subsequently upgraded with runtime versions featuring XCMv4, without any associated migration process. As a result, pallet_xcm now uses XCM_VERSION=4 as a key to operational data, but no data were migrated to XcmVersion=4. This has led to the malfunctioning of certain functionalities. Unfortunately, there is currently no mechanism to trigger the migration; it can only occur with another runtime upgrade that includes the necessary migration steps.

TODO/Improvements (minimal)

  • Implement the fn try_state function in pallet_xcm to verify that all stored operational data has been migrated to the current XCM version, ensuring migration steps are not overlooked.
  • Add reusable MigrateTo(XcmVersion) to the pallet_xcm - whenever the XCM version is updated, include the corresponding migration step MigrateTo(XcmVersion::4) in the runtime. After the upgrade, these migration steps can be removed.

Improvements (other possibilities)

(Maybe over-complicated and improvements (minimal) above are enough)

  • Avoid direct usage of XCM_VERSION in pallet_xcm. Instead, create a new storage item or reuse pub const CurrentXcmVersion as pub storage CurrentXcmVersion. This approach ensures that any changes to XCM_VERSION won't require automatic migration by default, preventing potential disruptions. Migrations will only be necessary if a runtime chooses to upgrade to a higher version, at which point they can add the corresponding migration step or trigger an extrinsic. Note: This approach may result in additional storage reads.
    • Introduce an extrinsic that allows manual triggering to initiate CurrentMigration.
  • Explore other sophisticated solutions for adding new XCM version to the runtime. (Maybe without need to add/remove MigrateTo(XcmVersion))
  • Change storage version when upgrading to new XCM version? (That could force us to add migration)
  • automatic update of SafeXcmVersion? Which can be done just by force_default_xcm_version?
  • simplify version handling of pallet_xcm, now we use XCM_VERSION and LatestVersionedLocation, maybe we should not use Versioned* struct for storage and use just xcm::latest and do migrations, it could pretty simplify stuff
  • AdvertisedXcmVersion vs CurrentXcmVersion vs SafeXcmVersion
  • RemoteLockedFungibles remove XcmVersion [XCM] pallet_xcm internal data changes proposal #6188
github-merge-queue bot pushed a commit that referenced this issue Feb 6, 2024
…`pallet_xcm` (#3228)

Relates to: #3214

## TODO

- [ ] backport to the `1.7.0` release
EgorPopelyaev pushed a commit that referenced this issue Feb 7, 2024
…`pallet_xcm` (#3228)

Relates to: #3214

## TODO

- [ ] backport to the `1.7.0` release
EgorPopelyaev pushed a commit that referenced this issue Feb 7, 2024
…`pallet_xcm` (#3228)

Relates to: #3214

## TODO

- [ ] backport to the `1.7.0` release
@acatangiu acatangiu added the T6-XCM This PR/Issue is related to XCM. label Mar 1, 2024
@acatangiu acatangiu moved this from Draft to Next to pick up in Parity Roadmap Mar 28, 2024
@bkontur bkontur moved this to Todo in Bridges + XCM Oct 21, 2024
github-merge-queue bot pushed a commit that referenced this issue Oct 25, 2024
Relates to: #4826
Relates to: #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
- [x] create tracking issue for `polkadot-fellows`
polkadot-fellows/runtimes#492
- [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

- [ ] deploy on Westend chains before XCMv5
- [ ] #6188

---------

Co-authored-by: command-bot <>
Co-authored-by: GitHub Action <action@github.com>
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
bkontur added a commit that referenced this issue Oct 28, 2024
Relates to: #4826
Relates to: #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. 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.

- [x] create tracking issue for `polkadot-fellows`
polkadot-fellows/runtimes#492
- [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`

- [ ] deploy on Westend chains before XCMv5
- [ ] #6188

---------

Co-authored-by: command-bot <>
Co-authored-by: GitHub Action <action@github.com>
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
(cherry picked from commit efd6603)
bkontur added a commit that referenced this issue Oct 28, 2024
Relates to: #4826
Relates to: #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. 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.

- [x] create tracking issue for `polkadot-fellows`
polkadot-fellows/runtimes#492
- [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`

- [ ] deploy on Westend chains before XCMv5
- [ ] #6188

---------

Co-authored-by: command-bot <>
Co-authored-by: GitHub Action <action@github.com>
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
(cherry picked from commit efd6603)
mordamax pushed a commit to paritytech-stg/polkadot-sdk that referenced this issue Oct 29, 2024
Relates to: paritytech#4826
Relates to: paritytech#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
- [x] create tracking issue for `polkadot-fellows`
polkadot-fellows/runtimes#492
- [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

- [ ] deploy on Westend chains before XCMv5
- [ ] paritytech#6188

---------

Co-authored-by: command-bot <>
Co-authored-by: GitHub Action <action@github.com>
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
mordamax pushed a commit to paritytech-stg/polkadot-sdk that referenced this issue Oct 29, 2024
Relates to: paritytech#4826
Relates to: paritytech#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
- [x] create tracking issue for `polkadot-fellows`
polkadot-fellows/runtimes#492
- [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

- [ ] deploy on Westend chains before XCMv5
- [ ] paritytech#6188

---------

Co-authored-by: command-bot <>
Co-authored-by: GitHub Action <action@github.com>
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T6-XCM This PR/Issue is related to XCM.
Projects
Status: Todo
Status: Next to pick up
Development

No branches or pull requests

2 participants