Skip to content

Commit 28ca8cb

Browse files
authored
Ensure xcm versions over bridge (on sending chains) (#2481)
## Summary This pull request proposes a solution for improved control of the versioned XCM flow over the bridge (across different consensus chains) and resolves the situation where the sending chain/consensus has already migrated to a higher XCM version than the receiving chain/consensus. ## Problem/Motivation The current flow over the bridge involves a transfer from AssetHubRococo (AHR) to BridgeHubRococo (BHR) to BridgeHubWestend (BHW) and finally to AssetHubWestend (AHW), beginning with a reserve-backed transfer on AHR. In this process: 1. AHR sends XCM `ExportMessage` through `XcmpQueue`, incorporating XCM version checks using the `WrapVersion` feature, influenced by `pallet_xcm::SupportedVersion` (managed by `pallet_xcm::force_xcm_version` or version discovery). 2. BHR handles the `ExportMessage` instruction, utilizing the latest XCM version. The `HaulBlobExporter` converts the inner XCM to [`VersionedXcm::from`](https://github.com/paritytech/polkadot-sdk/blob/63ac2471aa0210f0ac9903bdd7d8f9351f9a635f/polkadot/xcm/xcm-builder/src/universal_exports.rs#L465-L467), also using the latest XCM version. However, challenges arise: - Incompatibility when BHW uses a different version than BHR. For instance, if BHR migrates to **XCMv4** while BHW remains on **XCMv3**, BHR's `VersionedXcm::from` uses `VersionedXcm::V4` variant, causing encoding issues for BHW. ``` /// Just a simulation of possible error, which could happen on BHW /// (this code is based on actual master without XCMv4) let encoded = hex_literal::hex!("0400"); println!("{:?}", VersionedXcm::<()>::decode(&mut &encoded[..])); Err(Error { cause: None, desc: "Could not decode `VersionedXcm`, variant doesn't exist" }) ``` - Similar compatibility issues exist between AHR and AHW. ## Solution This pull request introduces the following solutions: 1. **New trait `CheckVersion`** - added to the `xcm` module and exposing `pallet_xcm::SupportedVersion`. This enhancement allows checking the actual XCM version for desired destinations outside of the `pallet_xcm` module. 2. **Version Check in `HaulBlobExporter`** uses `CheckVersion` to check known/configured destination versions, ensuring compatibility. For example, in the scenario mentioned, BHR can store the version `3` for BHW. If BHR is on XCMv4, it will attempt to downgrade the message to version `3` instead of using the latest version `4`. 3. **Version Check in `pallet-xcm-bridge-hub-router`** - this check ensures compatibility with the real destination's XCM version, preventing the unnecessary sending of messages to the local bridge hub if versions are incompatible. These additions aim to improve the control and compatibility of XCM flows over the bridge and addressing issues related to version mismatches. ## Possible alternative solution _(More investigation is needed, and at the very least, it should extend to XCMv4/5. If this proves to be a viable option, I can open an RFC for XCM.)._ Add the `XcmVersion` attribute to the `ExportMessage` so that the sending chain can determine, based on what is stored in `pallet_xcm::SupportedVersion`, the version the destination is using. This way, we may not need to handle the version in `HaulBlobExporter`. ``` ExportMessage { network: NetworkId, destination: InteriorMultiLocation, xcm: Xcm<()> destination_xcm_version: Version, // <- new attritbute }, ``` ``` pub trait ExportXcm { fn validate( network: NetworkId, channel: u32, universal_source: &mut Option<InteriorMultiLocation>, destination: &mut Option<InteriorMultiLocation>, message: &mut Option<Xcm<()>>, destination_xcm_version: Version, , // <- new attritbute ) -> SendResult<Self::Ticket>; ``` ## Future Directions This PR does not fix version discovery over bridge, further investigation will be conducted here: paritytech/polkadot-sdk#2417. ## TODO - [x] `pallet_xcm` mock for tests uses hard-coded XCM version `2` - change to 3 or lastest? - [x] fix `pallet-xcm-bridge-hub-router` - [x] fix HaulBlobExporter with version determination [here](https://github.com/paritytech/polkadot-sdk/blob/2183669d05f9b510f979a0cc3c7847707bacba2e/polkadot/xcm/xcm-builder/src/universal_exports.rs#L465) - [x] add unit-tests to the runtimes - [x] run benchmarks for `ExportMessage` - [x] extend local run scripts about `force_xcm_version(dest, version)` - [ ] when merged, prepare governance calls for Rococo/Westend - [ ] add PRDoc Part of: paritytech/parity-bridges-common#2719 --------- Co-authored-by: command-bot <>
1 parent 7b8f275 commit 28ca8cb

File tree

12 files changed

+186
-27
lines changed

12 files changed

+186
-27
lines changed

pallet-xcm/src/lib.rs

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2591,7 +2591,7 @@ impl<T: Config> WrapVersion for Pallet<T> {
25912591
dest: &MultiLocation,
25922592
xcm: impl Into<VersionedXcm<RuntimeCall>>,
25932593
) -> Result<VersionedXcm<RuntimeCall>, ()> {
2594-
SupportedVersion::<T>::get(XCM_VERSION, LatestVersionedMultiLocation(dest))
2594+
Self::get_version_for(dest)
25952595
.or_else(|| {
25962596
Self::note_unknown_version(dest);
25972597
SafeXcmVersion::<T>::get()
@@ -2608,6 +2608,12 @@ impl<T: Config> WrapVersion for Pallet<T> {
26082608
}
26092609
}
26102610

2611+
impl<T: Config> GetVersion for Pallet<T> {
2612+
fn get_version_for(dest: &MultiLocation) -> Option<XcmVersion> {
2613+
SupportedVersion::<T>::get(XCM_VERSION, LatestVersionedMultiLocation(dest))
2614+
}
2615+
}
2616+
26112617
impl<T: Config> VersionChangeNotifier for Pallet<T> {
26122618
/// Start notifying `location` should the XCM version of this chain change.
26132619
///

pallet-xcm/src/mock.rs

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -655,14 +655,25 @@ pub(crate) fn buy_limited_execution<C>(
655655

656656
pub(crate) fn new_test_ext_with_balances(
657657
balances: Vec<(AccountId, Balance)>,
658+
) -> sp_io::TestExternalities {
659+
new_test_ext_with_balances_and_xcm_version(
660+
balances,
661+
// By default set actual latest XCM version
662+
Some(XCM_VERSION),
663+
)
664+
}
665+
666+
pub(crate) fn new_test_ext_with_balances_and_xcm_version(
667+
balances: Vec<(AccountId, Balance)>,
668+
safe_xcm_version: Option<XcmVersion>,
658669
) -> sp_io::TestExternalities {
659670
let mut t = frame_system::GenesisConfig::<Test>::default().build_storage().unwrap();
660671

661672
pallet_balances::GenesisConfig::<Test> { balances }
662673
.assimilate_storage(&mut t)
663674
.unwrap();
664675

665-
pallet_xcm::GenesisConfig::<Test> { safe_xcm_version: Some(2), ..Default::default() }
676+
pallet_xcm::GenesisConfig::<Test> { safe_xcm_version, ..Default::default() }
666677
.assimilate_storage(&mut t)
667678
.unwrap();
668679

pallet-xcm/src/tests/mod.rs

Lines changed: 71 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -774,12 +774,13 @@ fn subscription_side_upgrades_work_without_notify() {
774774

775775
#[test]
776776
fn subscriber_side_subscription_works() {
777-
new_test_ext_with_balances(vec![]).execute_with(|| {
777+
new_test_ext_with_balances_and_xcm_version(vec![], Some(XCM_VERSION)).execute_with(|| {
778778
let remote: MultiLocation = Parachain(1000).into();
779779
assert_ok!(XcmPallet::force_subscribe_version_notify(
780780
RuntimeOrigin::root(),
781781
Box::new(remote.into()),
782782
));
783+
assert_eq!(XcmPallet::get_version_for(&remote), None);
783784
take_sent_xcm();
784785

785786
// Assume subscription target is working ok.
@@ -798,6 +799,7 @@ fn subscriber_side_subscription_works() {
798799
let r = XcmExecutor::<XcmConfig>::execute_xcm(remote, message, hash, weight);
799800
assert_eq!(r, Outcome::Complete(weight));
800801
assert_eq!(take_sent_xcm(), vec![]);
802+
assert_eq!(XcmPallet::get_version_for(&remote), Some(1));
801803

802804
// This message cannot be sent to a v2 remote.
803805
let v2_msg = xcm::v2::Xcm::<()>(vec![xcm::v2::Instruction::Trap(0)]);
@@ -815,6 +817,8 @@ fn subscriber_side_subscription_works() {
815817
let hash = fake_message_hash(&message);
816818
let r = XcmExecutor::<XcmConfig>::execute_xcm(remote, message, hash, weight);
817819
assert_eq!(r, Outcome::Complete(weight));
820+
assert_eq!(take_sent_xcm(), vec![]);
821+
assert_eq!(XcmPallet::get_version_for(&remote), Some(2));
818822

819823
// This message can now be sent to remote as it's v2.
820824
assert_eq!(
@@ -827,7 +831,7 @@ fn subscriber_side_subscription_works() {
827831
/// We should auto-subscribe when we don't know the remote's version.
828832
#[test]
829833
fn auto_subscription_works() {
830-
new_test_ext_with_balances(vec![]).execute_with(|| {
834+
new_test_ext_with_balances_and_xcm_version(vec![], None).execute_with(|| {
831835
let remote_v2: MultiLocation = Parachain(1000).into();
832836
let remote_v3: MultiLocation = Parachain(1001).into();
833837

@@ -995,3 +999,68 @@ fn subscription_side_upgrades_work_with_multistage_notify() {
995999
);
9961000
});
9971001
}
1002+
1003+
#[test]
1004+
fn get_and_wrap_version_works() {
1005+
new_test_ext_with_balances_and_xcm_version(vec![], None).execute_with(|| {
1006+
let remote_a: MultiLocation = Parachain(1000).into();
1007+
let remote_b: MultiLocation = Parachain(1001).into();
1008+
let remote_c: MultiLocation = Parachain(1002).into();
1009+
1010+
// no `safe_xcm_version` version at `GenesisConfig`
1011+
assert_eq!(XcmPallet::get_version_for(&remote_a), None);
1012+
assert_eq!(XcmPallet::get_version_for(&remote_b), None);
1013+
assert_eq!(XcmPallet::get_version_for(&remote_c), None);
1014+
assert_eq!(VersionDiscoveryQueue::<Test>::get().into_inner(), vec![]);
1015+
1016+
// set default XCM version (a.k.a. `safe_xcm_version`)
1017+
assert_ok!(XcmPallet::force_default_xcm_version(RuntimeOrigin::root(), Some(1)));
1018+
assert_eq!(XcmPallet::get_version_for(&remote_a), None);
1019+
assert_eq!(XcmPallet::get_version_for(&remote_b), None);
1020+
assert_eq!(XcmPallet::get_version_for(&remote_c), None);
1021+
assert_eq!(VersionDiscoveryQueue::<Test>::get().into_inner(), vec![]);
1022+
1023+
// set XCM version only for `remote_a`
1024+
assert_ok!(XcmPallet::force_xcm_version(
1025+
RuntimeOrigin::root(),
1026+
Box::new(remote_a),
1027+
XCM_VERSION
1028+
));
1029+
assert_eq!(XcmPallet::get_version_for(&remote_a), Some(XCM_VERSION));
1030+
assert_eq!(XcmPallet::get_version_for(&remote_b), None);
1031+
assert_eq!(XcmPallet::get_version_for(&remote_c), None);
1032+
assert_eq!(VersionDiscoveryQueue::<Test>::get().into_inner(), vec![]);
1033+
1034+
let xcm = Xcm::<()>::default();
1035+
1036+
// wrap version - works because remote_a has `XCM_VERSION`
1037+
assert_eq!(
1038+
XcmPallet::wrap_version(&remote_a, xcm.clone()),
1039+
Ok(VersionedXcm::from(xcm.clone()))
1040+
);
1041+
// does not work because remote_b has unknown version and default is set to 1, and
1042+
// `XCM_VERSION` cannot be wrapped to the `1`
1043+
assert_eq!(XcmPallet::wrap_version(&remote_b, xcm.clone()), Err(()));
1044+
assert_eq!(VersionDiscoveryQueue::<Test>::get().into_inner(), vec![(remote_b.into(), 1)]);
1045+
1046+
// set default to the `XCM_VERSION`
1047+
assert_ok!(XcmPallet::force_default_xcm_version(RuntimeOrigin::root(), Some(XCM_VERSION)));
1048+
assert_eq!(XcmPallet::get_version_for(&remote_b), None);
1049+
assert_eq!(XcmPallet::get_version_for(&remote_c), None);
1050+
1051+
// now works, because default is `XCM_VERSION`
1052+
assert_eq!(
1053+
XcmPallet::wrap_version(&remote_b, xcm.clone()),
1054+
Ok(VersionedXcm::from(xcm.clone()))
1055+
);
1056+
assert_eq!(VersionDiscoveryQueue::<Test>::get().into_inner(), vec![(remote_b.into(), 2)]);
1057+
1058+
// change remote_c to `1`
1059+
assert_ok!(XcmPallet::force_xcm_version(RuntimeOrigin::root(), Box::new(remote_c), 1));
1060+
1061+
// does not work because remote_c has `1` and default is `XCM_VERSION` which cannot be
1062+
// wrapped to the `1`
1063+
assert_eq!(XcmPallet::wrap_version(&remote_c, xcm.clone()), Err(()));
1064+
assert_eq!(VersionDiscoveryQueue::<Test>::get().into_inner(), vec![(remote_b.into(), 2)]);
1065+
})
1066+
}

src/lib.rs

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -373,6 +373,12 @@ pub trait WrapVersion {
373373
) -> Result<VersionedXcm<RuntimeCall>, ()>;
374374
}
375375

376+
/// Check and return the `Version` that should be used for the `Xcm` datum for the destination
377+
/// `MultiLocation`, which will interpret it.
378+
pub trait GetVersion {
379+
fn get_version_for(dest: &latest::MultiLocation) -> Option<Version>;
380+
}
381+
376382
/// `()` implementation does nothing with the XCM, just sending with whatever version it was
377383
/// authored as.
378384
impl WrapVersion for () {
@@ -395,6 +401,11 @@ impl WrapVersion for AlwaysV2 {
395401
Ok(VersionedXcm::<RuntimeCall>::V2(xcm.into().try_into()?))
396402
}
397403
}
404+
impl GetVersion for AlwaysV2 {
405+
fn get_version_for(_dest: &latest::MultiLocation) -> Option<Version> {
406+
Some(v2::VERSION)
407+
}
408+
}
398409

399410
/// `WrapVersion` implementation which attempts to always convert the XCM to version 3 before
400411
/// wrapping it.
@@ -407,6 +418,11 @@ impl WrapVersion for AlwaysV3 {
407418
Ok(VersionedXcm::<Call>::V3(xcm.into().try_into()?))
408419
}
409420
}
421+
impl GetVersion for AlwaysV3 {
422+
fn get_version_for(_dest: &latest::MultiLocation) -> Option<Version> {
423+
Some(v3::VERSION)
424+
}
425+
}
410426

411427
/// `WrapVersion` implementation which attempts to always convert the XCM to the latest version
412428
/// before wrapping it.
@@ -418,8 +434,8 @@ pub type AlwaysLts = AlwaysV3;
418434

419435
pub mod prelude {
420436
pub use super::{
421-
latest::prelude::*, AlwaysLatest, AlwaysLts, AlwaysV2, AlwaysV3, IntoVersion, Unsupported,
422-
Version as XcmVersion, VersionedAssetId, VersionedInteriorMultiLocation,
437+
latest::prelude::*, AlwaysLatest, AlwaysLts, AlwaysV2, AlwaysV3, GetVersion, IntoVersion,
438+
Unsupported, Version as XcmVersion, VersionedAssetId, VersionedInteriorMultiLocation,
423439
VersionedMultiAsset, VersionedMultiAssets, VersionedMultiLocation, VersionedResponse,
424440
VersionedXcm, WrapVersion,
425441
};

xcm-builder/src/tests/bridging/local_para_para.rs

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,16 @@ use super::*;
2323
parameter_types! {
2424
pub UniversalLocation: Junctions = X2(GlobalConsensus(Local::get()), Parachain(1));
2525
pub RemoteUniversalLocation: Junctions = X2(GlobalConsensus(Remote::get()), Parachain(1));
26+
pub RemoteNetwork: MultiLocation = AncestorThen(2, GlobalConsensus(Remote::get())).into();
2627
}
2728
type TheBridge =
2829
TestBridge<BridgeBlobDispatcher<TestRemoteIncomingRouter, RemoteUniversalLocation, ()>>;
29-
type Router =
30-
TestTopic<UnpaidLocalExporter<HaulBlobExporter<TheBridge, Remote, Price>, UniversalLocation>>;
30+
type Router = TestTopic<
31+
UnpaidLocalExporter<
32+
HaulBlobExporter<TheBridge, RemoteNetwork, AlwaysLatest, Price>,
33+
UniversalLocation,
34+
>,
35+
>;
3136

3237
/// ```nocompile
3338
/// local | remote

xcm-builder/src/tests/bridging/local_relay_relay.rs

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,16 @@ use super::*;
2323
parameter_types! {
2424
pub UniversalLocation: Junctions = X1(GlobalConsensus(Local::get()));
2525
pub RemoteUniversalLocation: Junctions = X1(GlobalConsensus(Remote::get()));
26+
pub RemoteNetwork: MultiLocation = AncestorThen(1, GlobalConsensus(Remote::get())).into();
2627
}
2728
type TheBridge =
2829
TestBridge<BridgeBlobDispatcher<TestRemoteIncomingRouter, RemoteUniversalLocation, ()>>;
29-
type Router =
30-
TestTopic<UnpaidLocalExporter<HaulBlobExporter<TheBridge, Remote, Price>, UniversalLocation>>;
30+
type Router = TestTopic<
31+
UnpaidLocalExporter<
32+
HaulBlobExporter<TheBridge, RemoteNetwork, AlwaysLatest, Price>,
33+
UniversalLocation,
34+
>,
35+
>;
3136

3237
/// ```nocompile
3338
/// local | remote

xcm-builder/src/tests/bridging/mod.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ use super::mock::*;
2020
use crate::{universal_exports::*, WithTopicSource};
2121
use frame_support::{parameter_types, traits::Get};
2222
use std::{cell::RefCell, marker::PhantomData};
23+
use xcm::AlwaysLatest;
2324
use xcm_executor::{
2425
traits::{export_xcm, validate_export},
2526
XcmExecutor,

xcm-builder/src/tests/bridging/paid_remote_relay_relay.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ parameter_types! {
3030
pub UniversalLocation: Junctions = X2(GlobalConsensus(Local::get()), Parachain(100));
3131
pub RelayUniversalLocation: Junctions = X1(GlobalConsensus(Local::get()));
3232
pub RemoteUniversalLocation: Junctions = X1(GlobalConsensus(Remote::get()));
33+
pub RemoteNetwork: MultiLocation = AncestorThen(1, GlobalConsensus(Remote::get())).into();
3334
pub BridgeTable: Vec<NetworkExportTableItem> = vec![
3435
NetworkExportTableItem::new(
3536
Remote::get(),
@@ -41,7 +42,7 @@ parameter_types! {
4142
}
4243
type TheBridge =
4344
TestBridge<BridgeBlobDispatcher<TestRemoteIncomingRouter, RemoteUniversalLocation, ()>>;
44-
type RelayExporter = HaulBlobExporter<TheBridge, Remote, Price>;
45+
type RelayExporter = HaulBlobExporter<TheBridge, RemoteNetwork, AlwaysLatest, Price>;
4546
type LocalInnerRouter = ExecutingRouter<UniversalLocation, RelayUniversalLocation, RelayExporter>;
4647
type LocalBridgeRouter = SovereignPaidRemoteExporter<
4748
NetworkExportTable<BridgeTable>,

xcm-builder/src/tests/bridging/remote_para_para.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ parameter_types! {
2424
pub UniversalLocation: Junctions = X2(GlobalConsensus(Local::get()), Parachain(1000));
2525
pub ParaBridgeUniversalLocation: Junctions = X2(GlobalConsensus(Local::get()), Parachain(1));
2626
pub RemoteParaBridgeUniversalLocation: Junctions = X2(GlobalConsensus(Remote::get()), Parachain(1));
27+
pub RemoteNetwork: MultiLocation = AncestorThen(2, GlobalConsensus(Remote::get())).into();
2728
pub BridgeTable: Vec<NetworkExportTableItem> = vec![
2829
NetworkExportTableItem::new(
2930
Remote::get(),
@@ -36,7 +37,7 @@ parameter_types! {
3637
type TheBridge = TestBridge<
3738
BridgeBlobDispatcher<TestRemoteIncomingRouter, RemoteParaBridgeUniversalLocation, ()>,
3839
>;
39-
type RelayExporter = HaulBlobExporter<TheBridge, Remote, ()>;
40+
type RelayExporter = HaulBlobExporter<TheBridge, RemoteNetwork, AlwaysLatest, ()>;
4041
type LocalInnerRouter =
4142
UnpaidExecutingRouter<UniversalLocation, ParaBridgeUniversalLocation, RelayExporter>;
4243
type LocalBridgingRouter =

xcm-builder/src/tests/bridging/remote_para_para_via_relay.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ parameter_types! {
2424
pub UniversalLocation: Junctions = X1(GlobalConsensus(Local::get()));
2525
pub ParaBridgeUniversalLocation: Junctions = X2(GlobalConsensus(Local::get()), Parachain(1));
2626
pub RemoteParaBridgeUniversalLocation: Junctions = X2(GlobalConsensus(Remote::get()), Parachain(1));
27+
pub RemoteNetwork: MultiLocation = AncestorThen(2, GlobalConsensus(Remote::get())).into();
2728
pub BridgeTable: Vec<NetworkExportTableItem> = vec![
2829
NetworkExportTableItem::new(
2930
Remote::get(),
@@ -36,7 +37,7 @@ parameter_types! {
3637
type TheBridge = TestBridge<
3738
BridgeBlobDispatcher<TestRemoteIncomingRouter, RemoteParaBridgeUniversalLocation, ()>,
3839
>;
39-
type RelayExporter = HaulBlobExporter<TheBridge, Remote, ()>;
40+
type RelayExporter = HaulBlobExporter<TheBridge, RemoteNetwork, AlwaysLatest, ()>;
4041
type LocalInnerRouter =
4142
UnpaidExecutingRouter<UniversalLocation, ParaBridgeUniversalLocation, RelayExporter>;
4243
type LocalBridgingRouter =

0 commit comments

Comments
 (0)