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: #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 <>
paritytech · Dec 12, 2023 · 575b8f8 · 575b8f8
1 parent 42a3afb
commit 575b8f8
Show file tree

Hide file tree

Showing 55 changed files with 1,279 additions and 490 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/bridges/bin/runtime-common/src/messages_xcm_extension.rs b/bridges/bin/runtime-common/src/messages_xcm_extension.rs
@@ -309,6 +309,28 @@ impl<H: XcmBlobHauler> LocalXcmQueueManager<H> {
 	}
 }
 
+/// Adapter for the implementation of `GetVersion`, which attempts to find the minimal
+/// configured XCM version between the destination `dest` and the bridge hub location provided as
+/// `Get<Location>`.
+pub struct XcmVersionOfDestAndRemoteBridge<Version, RemoteBridge>(
+	sp_std::marker::PhantomData<(Version, RemoteBridge)>,
+);
+impl<Version: GetVersion, RemoteBridge: Get<MultiLocation>> GetVersion
+	for XcmVersionOfDestAndRemoteBridge<Version, RemoteBridge>
+{
+	fn get_version_for(dest: &MultiLocation) -> Option<XcmVersion> {
+		let dest_version = Version::get_version_for(dest);
+		let bridge_hub_version = Version::get_version_for(&RemoteBridge::get());
+
+		match (dest_version, bridge_hub_version) {
+			(Some(dv), Some(bhv)) => Some(sp_std::cmp::min(dv, bhv)),
+			(Some(dv), None) => Some(dv),
+			(None, Some(bhv)) => Some(bhv),
+			(None, None) => None,
+		}
+	}
+}
+
 #[cfg(test)]
 mod tests {
 	use super::*;

diff --git a/bridges/modules/xcm-bridge-hub-router/src/benchmarking.rs b/bridges/modules/xcm-bridge-hub-router/src/benchmarking.rs
@@ -21,7 +21,7 @@
 use crate::{Bridge, Call};
 
 use bp_xcm_bridge_hub_router::{BridgeState, MINIMAL_DELIVERY_FEE_FACTOR};
-use frame_benchmarking::benchmarks_instance_pallet;
+use frame_benchmarking::{benchmarks_instance_pallet, BenchmarkError};
 use frame_support::traits::{EnsureOrigin, Get, Hooks, UnfilteredDispatchable};
 use sp_runtime::traits::Zero;
 use xcm::prelude::*;
@@ -37,11 +37,11 @@ pub trait Config<I: 'static>: crate::Config<I> {
 	/// Returns destination which is valid for this router instance.
 	/// (Needs to pass `T::Bridges`)
 	/// Make sure that `SendXcm` will pass.
-	fn ensure_bridged_target_destination() -> MultiLocation {
-		MultiLocation::new(
+	fn ensure_bridged_target_destination() -> Result<MultiLocation, BenchmarkError> {
+		Ok(MultiLocation::new(
 			Self::UniversalLocation::get().len() as u8,
 			X1(GlobalConsensus(Self::BridgedNetworkId::get().unwrap())),
-		)
+		))
 	}
 }
 
@@ -61,7 +61,7 @@ benchmarks_instance_pallet! {
 			delivery_fee_factor: MINIMAL_DELIVERY_FEE_FACTOR + MINIMAL_DELIVERY_FEE_FACTOR,
 		});
 
-		let _ = T::ensure_bridged_target_destination();
+		let _ = T::ensure_bridged_target_destination()?;
 		T::make_congested();
 	}: {
 		crate::Pallet::<T, I>::on_initialize(Zero::zero())
@@ -81,7 +81,7 @@ benchmarks_instance_pallet! {
 	}
 
 	send_message {
-		let dest = T::ensure_bridged_target_destination();
+		let dest = T::ensure_bridged_target_destination()?;
 		let xcm = sp_std::vec![].into();
 
 		// make local queue congested, because it means additional db write

diff --git a/bridges/modules/xcm-bridge-hub-router/src/lib.rs b/bridges/modules/xcm-bridge-hub-router/src/lib.rs
@@ -89,6 +89,8 @@ pub mod pallet {
 		/// **possible fee**. Allows to externalize better control over allowed **bridged
 		/// networks/locations**.
 		type Bridges: ExporterFor;
+		/// Checks the XCM version for the destination.
+		type DestinationVersion: GetVersion;
 
 		/// Origin of the sibling bridge hub that is allowed to report bridge status.
 		type BridgeHubOrigin: EnsureOrigin<Self::RuntimeOrigin>;
@@ -319,19 +321,32 @@ impl<T: Config<I>, I: 'static> SendXcm for Pallet<T, I> {
 		dest: &mut Option<MultiLocation>,
 		xcm: &mut Option<Xcm<()>>,
 	) -> SendResult<Self::Ticket> {
-		// we won't have an access to `dest` and `xcm` in the `delvier` method, so precompute
+		// `dest` and `xcm` are required here
+		let dest_ref = dest.as_ref().ok_or(SendError::MissingArgument)?;
+		let xcm_ref = xcm.as_ref().ok_or(SendError::MissingArgument)?;
+
+		// we won't have an access to `dest` and `xcm` in the `deliver` method, so precompute
 		// everything required here
-		let message_size = xcm
-			.as_ref()
-			.map(|xcm| xcm.encoded_size() as _)
-			.ok_or(SendError::MissingArgument)?;
+		let message_size = xcm_ref.encoded_size() as _;
 
 		// bridge doesn't support oversized/overweight messages now. So it is better to drop such
 		// messages here than at the bridge hub. Let's check the message size.
 		if message_size > HARD_MESSAGE_SIZE_LIMIT {
 			return Err(SendError::ExceedsMaxMessageSize)
 		}
 
+		// We need to ensure that the known `dest`'s XCM version can comprehend the current `xcm`
+		// program. This may seem like an additional, unnecessary check, but it is not. A similar
+		// check is probably performed by the `ViaBridgeHubExporter`, which attempts to send a
+		// versioned message to the sibling bridge hub. However, the local bridge hub may have a
+		// higher XCM version than the remote `dest`. Once again, it is better to discard such
+		// messages here than at the bridge hub (e.g., to avoid losing funds).
+		let destination_version = T::DestinationVersion::get_version_for(dest_ref)
+			.ok_or(SendError::DestinationUnsupported)?;
+		let _ = VersionedXcm::from(xcm_ref.clone())
+			.into_version(destination_version)
+			.map_err(|()| SendError::DestinationUnsupported)?;
+
 		// just use exporter to validate destination and insert instructions to pay message fee
 		// at the sibling/child bridge hub
 		//
@@ -358,6 +373,7 @@ impl<T: Config<I>, I: 'static> SendXcm for Pallet<T, I> {
 #[cfg(test)]
 mod tests {
 	use super::*;
+	use frame_support::assert_ok;
 	use mock::*;
 
 	use frame_support::traits::Hooks;
@@ -451,6 +467,19 @@ mod tests {
 		});
 	}
 
+	#[test]
+	fn destination_unsupported_if_wrap_version_fails() {
+		run_test(|| {
+			assert_eq!(
+				send_xcm::<XcmBridgeHubRouter>(
+					UnknownXcmVersionLocation::get(),
+					vec![ClearOrigin].into(),
+				),
+				Err(SendError::DestinationUnsupported),
+			);
+		});
+	}
+
 	#[test]
 	fn returns_proper_delivery_price() {
 		run_test(|| {
@@ -488,17 +517,14 @@ mod tests {
 	fn sent_message_doesnt_increase_factor_if_xcm_channel_is_uncongested() {
 		run_test(|| {
 			let old_bridge = XcmBridgeHubRouter::bridge();
-			assert_eq!(
-				send_xcm::<XcmBridgeHubRouter>(
-					MultiLocation::new(
-						2,
-						X2(GlobalConsensus(BridgedNetworkId::get()), Parachain(1000))
-					),
-					vec![ClearOrigin].into(),
-				)
-				.map(drop),
-				Ok(()),
-			);
+			assert_ok!(send_xcm::<XcmBridgeHubRouter>(
+				MultiLocation::new(
+					2,
+					X2(GlobalConsensus(BridgedNetworkId::get()), Parachain(1000))
+				),
+				vec![ClearOrigin].into(),
+			)
+			.map(drop));
 
 			assert!(TestToBridgeHubSender::is_message_sent());
 			assert_eq!(old_bridge, XcmBridgeHubRouter::bridge());
@@ -511,17 +537,14 @@ mod tests {
 			TestWithBridgeHubChannel::make_congested();
 
 			let old_bridge = XcmBridgeHubRouter::bridge();
-			assert_eq!(
-				send_xcm::<XcmBridgeHubRouter>(
-					MultiLocation::new(
-						2,
-						X2(GlobalConsensus(BridgedNetworkId::get()), Parachain(1000))
-					),
-					vec![ClearOrigin].into(),
-				)
-				.map(drop),
-				Ok(()),
-			);
+			assert_ok!(send_xcm::<XcmBridgeHubRouter>(
+				MultiLocation::new(
+					2,
+					X2(GlobalConsensus(BridgedNetworkId::get()), Parachain(1000))
+				),
+				vec![ClearOrigin].into(),
+			)
+			.map(drop));
 
 			assert!(TestToBridgeHubSender::is_message_sent());
 			assert!(
@@ -536,17 +559,14 @@ mod tests {
 			Bridge::<TestRuntime, ()>::put(congested_bridge(MINIMAL_DELIVERY_FEE_FACTOR));
 
 			let old_bridge = XcmBridgeHubRouter::bridge();
-			assert_eq!(
-				send_xcm::<XcmBridgeHubRouter>(
-					MultiLocation::new(
-						2,
-						X2(GlobalConsensus(BridgedNetworkId::get()), Parachain(1000))
-					),
-					vec![ClearOrigin].into(),
-				)
-				.map(drop),
-				Ok(()),
-			);
+			assert_ok!(send_xcm::<XcmBridgeHubRouter>(
+				MultiLocation::new(
+					2,
+					X2(GlobalConsensus(BridgedNetworkId::get()), Parachain(1000))
+				),
+				vec![ClearOrigin].into(),
+			)
+			.map(drop));
 
 			assert!(TestToBridgeHubSender::is_message_sent());
 			assert!(

diff --git a/bridges/modules/xcm-bridge-hub-router/src/mock.rs b/bridges/modules/xcm-bridge-hub-router/src/mock.rs
@@ -19,7 +19,10 @@
 use crate as pallet_xcm_bridge_hub_router;
 
 use bp_xcm_bridge_hub_router::XcmChannelStatusProvider;
-use frame_support::{construct_runtime, derive_impl, parameter_types};
+use frame_support::{
+	construct_runtime, derive_impl, parameter_types,
+	traits::{Contains, Equals},
+};
 use frame_system::EnsureRoot;
 use sp_runtime::{traits::ConstU128, BuildStorage};
 use xcm::prelude::*;
@@ -58,6 +61,7 @@ parameter_types! {
 				Some((BridgeFeeAsset::get(), BASE_FEE).into())
 			)
 		];
+	pub UnknownXcmVersionLocation: MultiLocation = MultiLocation::new(2, X2(GlobalConsensus(BridgedNetworkId::get()), Parachain(9999)));
 }
 
 #[derive_impl(frame_system::config_preludes::TestDefaultConfig as frame_system::DefaultConfig)]
@@ -71,6 +75,8 @@ impl pallet_xcm_bridge_hub_router::Config<()> for TestRuntime {
 	type UniversalLocation = UniversalLocation;
 	type BridgedNetworkId = BridgedNetworkId;
 	type Bridges = NetworkExportTable<BridgeTable>;
+	type DestinationVersion =
+		LatestOrNoneForLocationVersionChecker<Equals<UnknownXcmVersionLocation>>;
 
 	type BridgeHubOrigin = EnsureRoot<AccountId>;
 	type ToBridgeHubSender = TestToBridgeHubSender;
@@ -80,6 +86,18 @@ impl pallet_xcm_bridge_hub_router::Config<()> for TestRuntime {
 	type FeeAsset = BridgeFeeAsset;
 }
 
+pub struct LatestOrNoneForLocationVersionChecker<Location>(sp_std::marker::PhantomData<Location>);
+impl<Location: Contains<MultiLocation>> GetVersion
+	for LatestOrNoneForLocationVersionChecker<Location>
+{
+	fn get_version_for(dest: &MultiLocation) -> Option<XcmVersion> {
+		if Location::contains(dest) {
+			return None
+		}
+		Some(XCM_VERSION)
+	}
+}
+
 pub struct TestToBridgeHubSender;
 
 impl TestToBridgeHubSender {

diff --git a/bridges/modules/xcm-bridge-hub/src/exporter.rs b/bridges/modules/xcm-bridge-hub/src/exporter.rs
@@ -33,7 +33,8 @@ use xcm_executor::traits::ExportXcm;
 /// An easy way to access `HaulBlobExporter`.
 pub type PalletAsHaulBlobExporter<T, I> = HaulBlobExporter<
 	DummyHaulBlob,
-	<T as Config<I>>::BridgedNetworkId,
+	<T as Config<I>>::BridgedNetwork,
+	<T as Config<I>>::DestinationVersion,
 	<T as Config<I>>::MessageExportPrice,
 >;
 /// An easy way to access associated messages pallet.

diff --git a/bridges/modules/xcm-bridge-hub/src/lib.rs b/bridges/modules/xcm-bridge-hub/src/lib.rs
@@ -37,6 +37,7 @@ pub mod pallet {
 	use super::*;
 	use bridge_runtime_common::messages_xcm_extension::SenderAndLane;
 	use frame_support::pallet_prelude::*;
+	use frame_system::pallet_prelude::BlockNumberFor;
 
 	#[pallet::config]
 	#[pallet::disable_frame_system_supertrait_check]
@@ -48,15 +49,17 @@ pub mod pallet {
 		// TODO: https://github.com/paritytech/parity-bridges-common/issues/1666 remove `ChainId` and
 		// replace it with the `NetworkId` - then we'll be able to use
 		// `T as pallet_bridge_messages::Config<T::BridgeMessagesPalletInstance>::BridgedChain::NetworkId`
-		/// Bridged network id.
+		/// Bridged network as relative location of bridged `GlobalConsensus`.
 		#[pallet::constant]
-		type BridgedNetworkId: Get<NetworkId>;
+		type BridgedNetwork: Get<MultiLocation>;
 		/// Associated messages pallet instance that bridges us with the
 		/// `BridgedNetworkId` consensus.
 		type BridgeMessagesPalletInstance: 'static;
 
 		/// Price of single message export to the bridged consensus (`Self::BridgedNetworkId`).
 		type MessageExportPrice: Get<MultiAssets>;
+		/// Checks the XCM version for the destination.
+		type DestinationVersion: GetVersion;
 
 		/// Get point-to-point links with bridged consensus (`Self::BridgedNetworkId`).
 		/// (this will be replaced with dynamic on-chain bridges - `Bridges V2`)
@@ -69,6 +72,17 @@ pub mod pallet {
 	#[pallet::pallet]
 	pub struct Pallet<T, I = ()>(PhantomData<(T, I)>);
 
+	#[pallet::hooks]
+	impl<T: Config<I>, I: 'static> Hooks<BlockNumberFor<T>> for Pallet<T, I> {
+		fn integrity_test() {
+			assert!(
+				Self::bridged_network_id().is_some(),
+				"Configured `T::BridgedNetwork`: {:?} does not contain `GlobalConsensus` junction with `NetworkId`",
+				T::BridgedNetwork::get()
+			)
+		}
+	}
+
 	impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		/// Returns dedicated/configured lane identifier.
 		pub(crate) fn lane_for(
@@ -83,7 +97,7 @@ pub mod pallet {
 				.find_map(|(lane_source, (lane_dest_network, lane_dest))| {
 					if lane_source.location == source &&
 						&lane_dest_network == dest.0 &&
-						&T::BridgedNetworkId::get() == dest.0 &&
+						Self::bridged_network_id().as_ref() == Some(dest.0) &&
 						&lane_dest == dest.1
 					{
 						Some(lane_source)
@@ -92,5 +106,13 @@ pub mod pallet {
 					}
 				})
 		}
+
+		/// Returns some `NetworkId` if contains `GlobalConsensus` junction.
+		fn bridged_network_id() -> Option<NetworkId> {
+			match T::BridgedNetwork::get().take_first_interior() {
+				Some(GlobalConsensus(network)) => Some(network),
+				_ => None,
+			}
+		}
 	}
 }
diff --git a/bridges/modules/xcm-bridge-hub/src/mock.rs b/bridges/modules/xcm-bridge-hub/src/mock.rs
@@ -170,6 +170,10 @@ impl pallet_bridge_messages::WeightInfoExt for TestMessagesWeights {
 parameter_types! {
 	pub const RelayNetwork: NetworkId = NetworkId::Kusama;
 	pub const BridgedRelayNetwork: NetworkId = NetworkId::Polkadot;
+	pub const BridgedRelayNetworkLocation: MultiLocation = MultiLocation {
+		parents: 1,
+		interior: X1(GlobalConsensus(BridgedRelayNetwork::get()))
+	};
 	pub const NonBridgedRelayNetwork: NetworkId = NetworkId::Rococo;
 	pub const BridgeReserve: Balance = 100_000;
 	pub UniversalLocation: InteriorMultiLocation = X2(
@@ -181,10 +185,12 @@ parameter_types! {
 
 impl pallet_xcm_bridge_hub::Config for TestRuntime {
 	type UniversalLocation = UniversalLocation;
-	type BridgedNetworkId = BridgedRelayNetwork;
+	type BridgedNetwork = BridgedRelayNetworkLocation;
 	type BridgeMessagesPalletInstance = ();
 
 	type MessageExportPrice = ();
+	type DestinationVersion = AlwaysLatest;
+
 	type Lanes = TestLanes;
 	type LanesSupport = TestXcmBlobHauler;
 }