Skip to content

Commit

Permalink
Ensure xcm versions over bridge (on sending chains) (#2481)
Browse files Browse the repository at this point in the history
## 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 <>
  • Loading branch information
bkontur authored Dec 12, 2023
1 parent 7b8f275 commit 28ca8cb
Show file tree
Hide file tree
Showing 12 changed files with 186 additions and 27 deletions.
8 changes: 7 additions & 1 deletion pallet-xcm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2591,7 +2591,7 @@ impl<T: Config> WrapVersion for Pallet<T> {
dest: &MultiLocation,
xcm: impl Into<VersionedXcm<RuntimeCall>>,
) -> Result<VersionedXcm<RuntimeCall>, ()> {
SupportedVersion::<T>::get(XCM_VERSION, LatestVersionedMultiLocation(dest))
Self::get_version_for(dest)
.or_else(|| {
Self::note_unknown_version(dest);
SafeXcmVersion::<T>::get()
Expand All @@ -2608,6 +2608,12 @@ impl<T: Config> WrapVersion for Pallet<T> {
}
}

impl<T: Config> GetVersion for Pallet<T> {
fn get_version_for(dest: &MultiLocation) -> Option<XcmVersion> {
SupportedVersion::<T>::get(XCM_VERSION, LatestVersionedMultiLocation(dest))
}
}

impl<T: Config> VersionChangeNotifier for Pallet<T> {
/// Start notifying `location` should the XCM version of this chain change.
///
Expand Down
13 changes: 12 additions & 1 deletion pallet-xcm/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -655,14 +655,25 @@ pub(crate) fn buy_limited_execution<C>(

pub(crate) fn new_test_ext_with_balances(
balances: Vec<(AccountId, Balance)>,
) -> sp_io::TestExternalities {
new_test_ext_with_balances_and_xcm_version(
balances,
// By default set actual latest XCM version
Some(XCM_VERSION),
)
}

pub(crate) fn new_test_ext_with_balances_and_xcm_version(
balances: Vec<(AccountId, Balance)>,
safe_xcm_version: Option<XcmVersion>,
) -> sp_io::TestExternalities {
let mut t = frame_system::GenesisConfig::<Test>::default().build_storage().unwrap();

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

pallet_xcm::GenesisConfig::<Test> { safe_xcm_version: Some(2), ..Default::default() }
pallet_xcm::GenesisConfig::<Test> { safe_xcm_version, ..Default::default() }
.assimilate_storage(&mut t)
.unwrap();

Expand Down
73 changes: 71 additions & 2 deletions pallet-xcm/src/tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -774,12 +774,13 @@ fn subscription_side_upgrades_work_without_notify() {

#[test]
fn subscriber_side_subscription_works() {
new_test_ext_with_balances(vec![]).execute_with(|| {
new_test_ext_with_balances_and_xcm_version(vec![], Some(XCM_VERSION)).execute_with(|| {
let remote: MultiLocation = Parachain(1000).into();
assert_ok!(XcmPallet::force_subscribe_version_notify(
RuntimeOrigin::root(),
Box::new(remote.into()),
));
assert_eq!(XcmPallet::get_version_for(&remote), None);
take_sent_xcm();

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

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

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

Expand Down Expand Up @@ -995,3 +999,68 @@ fn subscription_side_upgrades_work_with_multistage_notify() {
);
});
}

#[test]
fn get_and_wrap_version_works() {
new_test_ext_with_balances_and_xcm_version(vec![], None).execute_with(|| {
let remote_a: MultiLocation = Parachain(1000).into();
let remote_b: MultiLocation = Parachain(1001).into();
let remote_c: MultiLocation = Parachain(1002).into();

// no `safe_xcm_version` version at `GenesisConfig`
assert_eq!(XcmPallet::get_version_for(&remote_a), None);
assert_eq!(XcmPallet::get_version_for(&remote_b), None);
assert_eq!(XcmPallet::get_version_for(&remote_c), None);
assert_eq!(VersionDiscoveryQueue::<Test>::get().into_inner(), vec![]);

// set default XCM version (a.k.a. `safe_xcm_version`)
assert_ok!(XcmPallet::force_default_xcm_version(RuntimeOrigin::root(), Some(1)));
assert_eq!(XcmPallet::get_version_for(&remote_a), None);
assert_eq!(XcmPallet::get_version_for(&remote_b), None);
assert_eq!(XcmPallet::get_version_for(&remote_c), None);
assert_eq!(VersionDiscoveryQueue::<Test>::get().into_inner(), vec![]);

// set XCM version only for `remote_a`
assert_ok!(XcmPallet::force_xcm_version(
RuntimeOrigin::root(),
Box::new(remote_a),
XCM_VERSION
));
assert_eq!(XcmPallet::get_version_for(&remote_a), Some(XCM_VERSION));
assert_eq!(XcmPallet::get_version_for(&remote_b), None);
assert_eq!(XcmPallet::get_version_for(&remote_c), None);
assert_eq!(VersionDiscoveryQueue::<Test>::get().into_inner(), vec![]);

let xcm = Xcm::<()>::default();

// wrap version - works because remote_a has `XCM_VERSION`
assert_eq!(
XcmPallet::wrap_version(&remote_a, xcm.clone()),
Ok(VersionedXcm::from(xcm.clone()))
);
// does not work because remote_b has unknown version and default is set to 1, and
// `XCM_VERSION` cannot be wrapped to the `1`
assert_eq!(XcmPallet::wrap_version(&remote_b, xcm.clone()), Err(()));
assert_eq!(VersionDiscoveryQueue::<Test>::get().into_inner(), vec![(remote_b.into(), 1)]);

// set default to the `XCM_VERSION`
assert_ok!(XcmPallet::force_default_xcm_version(RuntimeOrigin::root(), Some(XCM_VERSION)));
assert_eq!(XcmPallet::get_version_for(&remote_b), None);
assert_eq!(XcmPallet::get_version_for(&remote_c), None);

// now works, because default is `XCM_VERSION`
assert_eq!(
XcmPallet::wrap_version(&remote_b, xcm.clone()),
Ok(VersionedXcm::from(xcm.clone()))
);
assert_eq!(VersionDiscoveryQueue::<Test>::get().into_inner(), vec![(remote_b.into(), 2)]);

// change remote_c to `1`
assert_ok!(XcmPallet::force_xcm_version(RuntimeOrigin::root(), Box::new(remote_c), 1));

// does not work because remote_c has `1` and default is `XCM_VERSION` which cannot be
// wrapped to the `1`
assert_eq!(XcmPallet::wrap_version(&remote_c, xcm.clone()), Err(()));
assert_eq!(VersionDiscoveryQueue::<Test>::get().into_inner(), vec![(remote_b.into(), 2)]);
})
}
20 changes: 18 additions & 2 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,12 @@ pub trait WrapVersion {
) -> Result<VersionedXcm<RuntimeCall>, ()>;
}

/// Check and return the `Version` that should be used for the `Xcm` datum for the destination
/// `MultiLocation`, which will interpret it.
pub trait GetVersion {
fn get_version_for(dest: &latest::MultiLocation) -> Option<Version>;
}

/// `()` implementation does nothing with the XCM, just sending with whatever version it was
/// authored as.
impl WrapVersion for () {
Expand All @@ -395,6 +401,11 @@ impl WrapVersion for AlwaysV2 {
Ok(VersionedXcm::<RuntimeCall>::V2(xcm.into().try_into()?))
}
}
impl GetVersion for AlwaysV2 {
fn get_version_for(_dest: &latest::MultiLocation) -> Option<Version> {
Some(v2::VERSION)
}
}

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

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

pub mod prelude {
pub use super::{
latest::prelude::*, AlwaysLatest, AlwaysLts, AlwaysV2, AlwaysV3, IntoVersion, Unsupported,
Version as XcmVersion, VersionedAssetId, VersionedInteriorMultiLocation,
latest::prelude::*, AlwaysLatest, AlwaysLts, AlwaysV2, AlwaysV3, GetVersion, IntoVersion,
Unsupported, Version as XcmVersion, VersionedAssetId, VersionedInteriorMultiLocation,
VersionedMultiAsset, VersionedMultiAssets, VersionedMultiLocation, VersionedResponse,
VersionedXcm, WrapVersion,
};
Expand Down
9 changes: 7 additions & 2 deletions xcm-builder/src/tests/bridging/local_para_para.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,16 @@ use super::*;
parameter_types! {
pub UniversalLocation: Junctions = X2(GlobalConsensus(Local::get()), Parachain(1));
pub RemoteUniversalLocation: Junctions = X2(GlobalConsensus(Remote::get()), Parachain(1));
pub RemoteNetwork: MultiLocation = AncestorThen(2, GlobalConsensus(Remote::get())).into();
}
type TheBridge =
TestBridge<BridgeBlobDispatcher<TestRemoteIncomingRouter, RemoteUniversalLocation, ()>>;
type Router =
TestTopic<UnpaidLocalExporter<HaulBlobExporter<TheBridge, Remote, Price>, UniversalLocation>>;
type Router = TestTopic<
UnpaidLocalExporter<
HaulBlobExporter<TheBridge, RemoteNetwork, AlwaysLatest, Price>,
UniversalLocation,
>,
>;

/// ```nocompile
/// local | remote
Expand Down
9 changes: 7 additions & 2 deletions xcm-builder/src/tests/bridging/local_relay_relay.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,16 @@ use super::*;
parameter_types! {
pub UniversalLocation: Junctions = X1(GlobalConsensus(Local::get()));
pub RemoteUniversalLocation: Junctions = X1(GlobalConsensus(Remote::get()));
pub RemoteNetwork: MultiLocation = AncestorThen(1, GlobalConsensus(Remote::get())).into();
}
type TheBridge =
TestBridge<BridgeBlobDispatcher<TestRemoteIncomingRouter, RemoteUniversalLocation, ()>>;
type Router =
TestTopic<UnpaidLocalExporter<HaulBlobExporter<TheBridge, Remote, Price>, UniversalLocation>>;
type Router = TestTopic<
UnpaidLocalExporter<
HaulBlobExporter<TheBridge, RemoteNetwork, AlwaysLatest, Price>,
UniversalLocation,
>,
>;

/// ```nocompile
/// local | remote
Expand Down
1 change: 1 addition & 0 deletions xcm-builder/src/tests/bridging/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ use super::mock::*;
use crate::{universal_exports::*, WithTopicSource};
use frame_support::{parameter_types, traits::Get};
use std::{cell::RefCell, marker::PhantomData};
use xcm::AlwaysLatest;
use xcm_executor::{
traits::{export_xcm, validate_export},
XcmExecutor,
Expand Down
3 changes: 2 additions & 1 deletion xcm-builder/src/tests/bridging/paid_remote_relay_relay.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ parameter_types! {
pub UniversalLocation: Junctions = X2(GlobalConsensus(Local::get()), Parachain(100));
pub RelayUniversalLocation: Junctions = X1(GlobalConsensus(Local::get()));
pub RemoteUniversalLocation: Junctions = X1(GlobalConsensus(Remote::get()));
pub RemoteNetwork: MultiLocation = AncestorThen(1, GlobalConsensus(Remote::get())).into();
pub BridgeTable: Vec<NetworkExportTableItem> = vec![
NetworkExportTableItem::new(
Remote::get(),
Expand All @@ -41,7 +42,7 @@ parameter_types! {
}
type TheBridge =
TestBridge<BridgeBlobDispatcher<TestRemoteIncomingRouter, RemoteUniversalLocation, ()>>;
type RelayExporter = HaulBlobExporter<TheBridge, Remote, Price>;
type RelayExporter = HaulBlobExporter<TheBridge, RemoteNetwork, AlwaysLatest, Price>;
type LocalInnerRouter = ExecutingRouter<UniversalLocation, RelayUniversalLocation, RelayExporter>;
type LocalBridgeRouter = SovereignPaidRemoteExporter<
NetworkExportTable<BridgeTable>,
Expand Down
3 changes: 2 additions & 1 deletion xcm-builder/src/tests/bridging/remote_para_para.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ parameter_types! {
pub UniversalLocation: Junctions = X2(GlobalConsensus(Local::get()), Parachain(1000));
pub ParaBridgeUniversalLocation: Junctions = X2(GlobalConsensus(Local::get()), Parachain(1));
pub RemoteParaBridgeUniversalLocation: Junctions = X2(GlobalConsensus(Remote::get()), Parachain(1));
pub RemoteNetwork: MultiLocation = AncestorThen(2, GlobalConsensus(Remote::get())).into();
pub BridgeTable: Vec<NetworkExportTableItem> = vec![
NetworkExportTableItem::new(
Remote::get(),
Expand All @@ -36,7 +37,7 @@ parameter_types! {
type TheBridge = TestBridge<
BridgeBlobDispatcher<TestRemoteIncomingRouter, RemoteParaBridgeUniversalLocation, ()>,
>;
type RelayExporter = HaulBlobExporter<TheBridge, Remote, ()>;
type RelayExporter = HaulBlobExporter<TheBridge, RemoteNetwork, AlwaysLatest, ()>;
type LocalInnerRouter =
UnpaidExecutingRouter<UniversalLocation, ParaBridgeUniversalLocation, RelayExporter>;
type LocalBridgingRouter =
Expand Down
3 changes: 2 additions & 1 deletion xcm-builder/src/tests/bridging/remote_para_para_via_relay.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ parameter_types! {
pub UniversalLocation: Junctions = X1(GlobalConsensus(Local::get()));
pub ParaBridgeUniversalLocation: Junctions = X2(GlobalConsensus(Local::get()), Parachain(1));
pub RemoteParaBridgeUniversalLocation: Junctions = X2(GlobalConsensus(Remote::get()), Parachain(1));
pub RemoteNetwork: MultiLocation = AncestorThen(2, GlobalConsensus(Remote::get())).into();
pub BridgeTable: Vec<NetworkExportTableItem> = vec![
NetworkExportTableItem::new(
Remote::get(),
Expand All @@ -36,7 +37,7 @@ parameter_types! {
type TheBridge = TestBridge<
BridgeBlobDispatcher<TestRemoteIncomingRouter, RemoteParaBridgeUniversalLocation, ()>,
>;
type RelayExporter = HaulBlobExporter<TheBridge, Remote, ()>;
type RelayExporter = HaulBlobExporter<TheBridge, RemoteNetwork, AlwaysLatest, ()>;
type LocalInnerRouter =
UnpaidExecutingRouter<UniversalLocation, ParaBridgeUniversalLocation, RelayExporter>;
type LocalBridgingRouter =
Expand Down
3 changes: 2 additions & 1 deletion xcm-builder/src/tests/bridging/remote_relay_relay.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ parameter_types! {
pub UniversalLocation: Junctions = X2(GlobalConsensus(Local::get()), Parachain(1000));
pub RelayUniversalLocation: Junctions = X1(GlobalConsensus(Local::get()));
pub RemoteUniversalLocation: Junctions = X1(GlobalConsensus(Remote::get()));
pub RemoteNetwork: MultiLocation = AncestorThen(1, GlobalConsensus(Remote::get())).into();
pub BridgeTable: Vec<NetworkExportTableItem> = vec![
NetworkExportTableItem::new(
Remote::get(),
Expand All @@ -35,7 +36,7 @@ parameter_types! {
}
type TheBridge =
TestBridge<BridgeBlobDispatcher<TestRemoteIncomingRouter, RemoteUniversalLocation, ()>>;
type RelayExporter = HaulBlobExporter<TheBridge, Remote, ()>;
type RelayExporter = HaulBlobExporter<TheBridge, RemoteNetwork, AlwaysLatest, ()>;
type LocalInnerRouter =
UnpaidExecutingRouter<UniversalLocation, RelayUniversalLocation, RelayExporter>;
type LocalBridgeRouter =
Expand Down
Loading

0 comments on commit 28ca8cb

Please sign in to comment.