Ensure xcm versions over bridge (on sending chains)

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::*;

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

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!(

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 {

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.

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,
+			}
+		}
 	}
 }

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;
 }

cumulus/parachains/integration-tests/emulated/chains/parachains/assets/asset-hub-rococo/src/lib.rs

@@ -21,8 +21,8 @@ use frame_support::traits::OnInitialize;
 // Cumulus
 use emulated_integration_tests_common::{
 	impl_accounts_helpers_for_parachain, impl_assert_events_helpers_for_parachain,
-	impl_assets_helpers_for_parachain, impl_foreign_assets_helpers_for_parachain, impls::Parachain,
-	xcm_emulator::decl_test_parachains,
+	impl_assets_helpers_for_parachain, impl_foreign_assets_helpers_for_parachain,
+	impl_xcm_helpers_for_parachain, impls::Parachain, xcm_emulator::decl_test_parachains,
 };
 use rococo_emulated_chain::Rococo;
 
@@ -55,3 +55,4 @@ impl_accounts_helpers_for_parachain!(AssetHubRococo);
 impl_assert_events_helpers_for_parachain!(AssetHubRococo);
 impl_assets_helpers_for_parachain!(AssetHubRococo, Rococo);
 impl_foreign_assets_helpers_for_parachain!(AssetHubRococo, Rococo);
+impl_xcm_helpers_for_parachain!(AssetHubRococo);