Add validate_xcm_nesting to the ParentAsUmp and `ChildParachainRo…

…uter` (paritytech#4236)

This PR:
- moves `validate_xcm_nesting` from `XcmpQueue` into the `VersionedXcm`
- adds `validate_xcm_nesting` to the `ParentAsUmp`
- adds `validate_xcm_nesting` to the `ChildParachainRouter`


Based on discussion
[here](paritytech#4186 (comment))
and/or
[here](paritytech#4186 (comment))
and/or [here]()

## Question/TODO

- [x] To the
[comment](paritytech#4186 (comment))
- Why was `validate_xcm_nesting` added just to the `XcmpQueue` router
and nowhere else? What kind of problem `MAX_XCM_DECODE_DEPTH` is
solving? (see
[comment](paritytech#4236 (comment)))

cumulus/pallets/xcmp-queue/src/lib.rs

@@ -916,7 +916,8 @@ impl<T: Config> SendXcm for Pallet<T> {
 				let price = T::PriceForSiblingDelivery::price_for_delivery(id, &xcm);
 				let versioned_xcm = T::VersionWrapper::wrap_version(&d, xcm)
 					.map_err(|()| SendError::DestinationUnsupported)?;
-				validate_xcm_nesting(&versioned_xcm)
+				versioned_xcm
+					.validate_xcm_nesting()
 					.map_err(|()| SendError::ExceedsMaxMessageSize)?;
 
 				Ok(((id, versioned_xcm), price))
@@ -932,10 +933,6 @@ impl<T: Config> SendXcm for Pallet<T> {
 
 	fn deliver((id, xcm): (ParaId, VersionedXcm<()>)) -> Result<XcmHash, SendError> {
 		let hash = xcm.using_encoded(sp_io::hashing::blake2_256);
-		defensive_assert!(
-			validate_xcm_nesting(&xcm).is_ok(),
-			"Tickets are valid prior to delivery by trait XCM; qed"
-		);
 
 		match Self::send_fragment(id, XcmpMessageFormat::ConcatenatedVersionedXcm, xcm) {
 			Ok(_) => {
@@ -950,16 +947,6 @@ impl<T: Config> SendXcm for Pallet<T> {
 	}
 }
 
-/// Checks that the XCM is decodable with `MAX_XCM_DECODE_DEPTH`.
-///
-/// Note that this uses the limit of the sender - not the receiver. It it best effort.
-pub(crate) fn validate_xcm_nesting(xcm: &VersionedXcm<()>) -> Result<(), ()> {
-	xcm.using_encoded(|mut enc| {
-		VersionedXcm::<()>::decode_all_with_depth_limit(MAX_XCM_DECODE_DEPTH, &mut enc).map(|_| ())
-	})
-	.map_err(|_| ())
-}
-
 impl<T: Config> FeeTracker for Pallet<T> {
 	type Id = ParaId;
 

cumulus/parachains/runtimes/assets/asset-hub-rococo/src/lib.rs

@@ -1519,27 +1519,23 @@ impl_runtime_apis! {
 				fn worst_case_holding(depositable_count: u32) -> xcm::v4::Assets {
 					// A mix of fungible, non-fungible, and concrete assets.
 					let holding_non_fungibles = MaxAssetsIntoHolding::get() / 2 - depositable_count;
-					let holding_fungibles = holding_non_fungibles.saturating_sub(1);
+					let holding_fungibles = holding_non_fungibles.saturating_sub(2);  // -2 for two `iter::once` bellow
 					let fungibles_amount: u128 = 100;
-					let mut assets = (0..holding_fungibles)
+					(0..holding_fungibles)
 						.map(|i| {
 							Asset {
 								id: GeneralIndex(i as u128).into(),
-								fun: Fungible(fungibles_amount * i as u128),
+								fun: Fungible(fungibles_amount * (i + 1) as u128), // non-zero amount
 							}
 						})
 						.chain(core::iter::once(Asset { id: Here.into(), fun: Fungible(u128::MAX) }))
+						.chain(core::iter::once(Asset { id: AssetId(TokenLocation::get()), fun: Fungible(1_000_000 * UNITS) }))
 						.chain((0..holding_non_fungibles).map(|i| Asset {
 							id: GeneralIndex(i as u128).into(),
 							fun: NonFungible(asset_instance_from(i)),
 						}))
-						.collect::<Vec<_>>();
-
-					assets.push(Asset {
-						id: AssetId(TokenLocation::get()),
-						fun: Fungible(1_000_000 * UNITS),
-					});
-					assets.into()
+						.collect::<Vec<_>>()
+						.into()
 				}
 			}
 

cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs

@@ -1610,27 +1610,23 @@ impl_runtime_apis! {
 				fn worst_case_holding(depositable_count: u32) -> xcm::v4::Assets {
 					// A mix of fungible, non-fungible, and concrete assets.
 					let holding_non_fungibles = MaxAssetsIntoHolding::get() / 2 - depositable_count;
-					let holding_fungibles = holding_non_fungibles - 1;
+					let holding_fungibles = holding_non_fungibles - 2; // -2 for two `iter::once` bellow
 					let fungibles_amount: u128 = 100;
-					let mut assets = (0..holding_fungibles)
+					(0..holding_fungibles)
 						.map(|i| {
 							Asset {
 								id: AssetId(GeneralIndex(i as u128).into()),
-								fun: Fungible(fungibles_amount * i as u128),
+								fun: Fungible(fungibles_amount * (i + 1) as u128), // non-zero amount
 							}
 						})
 						.chain(core::iter::once(Asset { id: AssetId(Here.into()), fun: Fungible(u128::MAX) }))
+						.chain(core::iter::once(Asset { id: AssetId(WestendLocation::get()), fun: Fungible(1_000_000 * UNITS) }))
 						.chain((0..holding_non_fungibles).map(|i| Asset {
 							id: AssetId(GeneralIndex(i as u128).into()),
 							fun: NonFungible(asset_instance_from(i)),
 						}))
-						.collect::<Vec<_>>();
-
-					assets.push(Asset {
-						id: AssetId(WestendLocation::get()),
-						fun: Fungible(1_000_000 * UNITS),
-					});
-					assets.into()
+						.collect::<Vec<_>>()
+						.into()
 				}
 			}
 

cumulus/primitives/utility/src/lib.rs

@@ -69,6 +69,9 @@ where
 			let price = P::price_for_delivery((), &xcm);
 			let versioned_xcm =
 				W::wrap_version(&d, xcm).map_err(|()| SendError::DestinationUnsupported)?;
+			versioned_xcm
+				.validate_xcm_nesting()
+				.map_err(|()| SendError::ExceedsMaxMessageSize)?;
 			let data = versioned_xcm.encode();
 
 			Ok((data, price))
@@ -526,6 +529,8 @@ impl<
 mod test_xcm_router {
 	use super::*;
 	use cumulus_primitives_core::UpwardMessage;
+	use frame_support::assert_ok;
+	use xcm::MAX_XCM_DECODE_DEPTH;
 
 	/// Validates [`validate`] for required Some(destination) and Some(message)
 	struct OkFixedXcmHashWithAssertingRequiredInputsSender;
@@ -621,6 +626,29 @@ mod test_xcm_router {
 			)>(dest.into(), message)
 		);
 	}
+
+	#[test]
+	fn parent_as_ump_validate_nested_xcm_works() {
+		let dest = Parent;
+
+		type Router = ParentAsUmp<(), (), ()>;
+
+		// Message that is not too deeply nested:
+		let mut good = Xcm(vec![ClearOrigin]);
+		for _ in 0..MAX_XCM_DECODE_DEPTH - 1 {
+			good = Xcm(vec![SetAppendix(good)]);
+		}
+
+		// Check that the good message is validated:
+		assert_ok!(<Router as SendXcm>::validate(&mut Some(dest.into()), &mut Some(good.clone())));
+
+		// Nesting the message one more time should reject it:
+		let bad = Xcm(vec![SetAppendix(good)]);
+		assert_eq!(
+			Err(SendError::ExceedsMaxMessageSize),
+			<Router as SendXcm>::validate(&mut Some(dest.into()), &mut Some(bad))
+		);
+	}
 }
 #[cfg(test)]
 mod test_trader {

polkadot/runtime/common/src/integration_tests.rs

@@ -39,7 +39,7 @@ use primitives::{
 	MAX_CODE_SIZE,
 };
 use runtime_parachains::{
-	configuration, origin, paras, shared, Origin as ParaOrigin, ParaLifecycle,
+	configuration, dmp, origin, paras, shared, Origin as ParaOrigin, ParaLifecycle,
 };
 use sp_core::H256;
 use sp_io::TestExternalities;
@@ -84,6 +84,7 @@ frame_support::construct_runtime!(
 		Paras: paras,
 		ParasShared: shared,
 		ParachainsOrigin: origin,
+		Dmp: dmp,
 
 		// Para Onboarding Pallets
 		Registrar: paras_registrar,
@@ -201,6 +202,8 @@ impl shared::Config for Test {
 	type DisabledValidators = ();
 }
 
+impl dmp::Config for Test {}
+
 impl origin::Config for Test {}
 
 parameter_types! {

polkadot/runtime/common/src/xcm_sender.rs

@@ -119,7 +119,9 @@ where
 		let config = configuration::ActiveConfig::<T>::get();
 		let para = id.into();
 		let price = P::price_for_delivery(para, &xcm);
-		let blob = W::wrap_version(&d, xcm).map_err(|()| DestinationUnsupported)?.encode();
+		let versioned_xcm = W::wrap_version(&d, xcm).map_err(|()| DestinationUnsupported)?;
+		versioned_xcm.validate_xcm_nesting().map_err(|()| ExceedsMaxMessageSize)?;
+		let blob = versioned_xcm.encode();
 		dmp::Pallet::<T>::can_queue_downward_message(&config, &para, &blob)
 			.map_err(Into::<SendError>::into)?;
 
@@ -236,9 +238,11 @@ impl EnsureForParachain for () {
 #[cfg(test)]
 mod tests {
 	use super::*;
-	use frame_support::parameter_types;
+	use crate::integration_tests::new_test_ext;
+	use frame_support::{assert_ok, parameter_types};
 	use runtime_parachains::FeeTracker;
 	use sp_runtime::FixedU128;
+	use xcm::MAX_XCM_DECODE_DEPTH;
 
 	parameter_types! {
 		pub const BaseDeliveryFee: u128 = 300_000_000;
@@ -297,4 +301,40 @@ mod tests {
 			(FeeAssetId::get(), result).into()
 		);
 	}
+
+	#[test]
+	fn child_parachain_router_validate_nested_xcm_works() {
+		let dest = Parachain(5555);
+
+		type Router = ChildParachainRouter<
+			crate::integration_tests::Test,
+			(),
+			NoPriceForMessageDelivery<ParaId>,
+		>;
+
+		// Message that is not too deeply nested:
+		let mut good = Xcm(vec![ClearOrigin]);
+		for _ in 0..MAX_XCM_DECODE_DEPTH - 1 {
+			good = Xcm(vec![SetAppendix(good)]);
+		}
+
+		new_test_ext().execute_with(|| {
+			configuration::ActiveConfig::<crate::integration_tests::Test>::mutate(|c| {
+				c.max_downward_message_size = u32::MAX;
+			});
+
+			// Check that the good message is validated:
+			assert_ok!(<Router as SendXcm>::validate(
+				&mut Some(dest.into()),
+				&mut Some(good.clone())
+			));
+
+			// Nesting the message one more time should reject it:
+			let bad = Xcm(vec![SetAppendix(good)]);
+			assert_eq!(
+				Err(ExceedsMaxMessageSize),
+				<Router as SendXcm>::validate(&mut Some(dest.into()), &mut Some(bad))
+			);
+		});
+	}
 }

polkadot/xcm/pallet-xcm-benchmarks/src/fungible/benchmarking.rs

@@ -146,8 +146,6 @@ benchmarks_instance_pallet! {
 
 	initiate_reserve_withdraw {
 		let (sender_account, sender_location) = account_and_location::<T>(1);
-		let holding = T::worst_case_holding(1);
-		let assets_filter = AssetFilter::Definite(holding.clone().into_inner().into_iter().take(MAX_ITEMS_IN_ASSETS).collect::<Vec<_>>().into());
 		let reserve = T::valid_destination().map_err(|_| BenchmarkError::Skip)?;
 
 		let (expected_fees_mode, expected_assets_in_holding) = T::DeliveryHelper::ensure_successful_delivery(
@@ -157,15 +155,29 @@ benchmarks_instance_pallet! {
 		);
 		let sender_account_balance_before = T::TransactAsset::balance(&sender_account);
 
+		// generate holding and add possible required fees
+		let holding = if let Some(expected_assets_in_holding) = expected_assets_in_holding {
+			let mut holding = T::worst_case_holding(1 + expected_assets_in_holding.len() as u32);
+			for a in expected_assets_in_holding.into_inner() {
+				holding.push(a);
+			}
+			holding
+		} else {
+			T::worst_case_holding(1)
+		};
+
 		let mut executor = new_executor::<T>(sender_location);
-		executor.set_holding(holding.into());
+		executor.set_holding(holding.clone().into());
 		if let Some(expected_fees_mode) = expected_fees_mode {
 			executor.set_fees_mode(expected_fees_mode);
 		}
-		if let Some(expected_assets_in_holding) = expected_assets_in_holding {
-			executor.set_holding(expected_assets_in_holding.into());
-		}
-		let instruction = Instruction::InitiateReserveWithdraw { assets: assets_filter, reserve, xcm: Xcm(vec![]) };
+
+		let instruction = Instruction::InitiateReserveWithdraw {
+			// Worst case is looking through all holdings for every asset explicitly - respecting the limit `MAX_ITEMS_IN_ASSETS`.
+			assets: Definite(holding.into_inner().into_iter().take(MAX_ITEMS_IN_ASSETS).collect::<Vec<_>>().into()),
+			reserve,
+			xcm: Xcm(vec![])
+		};
 		let xcm = Xcm(vec![instruction]);
 	}: {
 		executor.bench_process(xcm)?;

polkadot/xcm/pallet-xcm-benchmarks/src/fungible/mock.rs

@@ -16,7 +16,7 @@
 
 //! A mock runtime for XCM benchmarking.
 
-use crate::{fungible as xcm_balances_benchmark, mock::*};
+use crate::{fungible as xcm_balances_benchmark, generate_holding_assets, mock::*};
 use frame_benchmarking::BenchmarkError;
 use frame_support::{
 	derive_impl, parameter_types,
@@ -130,9 +130,8 @@ impl crate::Config for Test {
 		Ok(valid_destination)
 	}
 	fn worst_case_holding(depositable_count: u32) -> Assets {
-		crate::mock_worst_case_holding(
-			depositable_count,
-			<XcmConfig as xcm_executor::Config>::MaxAssetsIntoHolding::get(),
+		generate_holding_assets(
+			<XcmConfig as xcm_executor::Config>::MaxAssetsIntoHolding::get() - depositable_count,
 		)
 	}
 }

polkadot/xcm/pallet-xcm-benchmarks/src/generic/benchmarking.rs

@@ -19,9 +19,9 @@ use crate::{account_and_location, new_executor, EnsureDelivery, XcmCallOf};
 use codec::Encode;
 use frame_benchmarking::{benchmarks, BenchmarkError};
 use frame_support::{dispatch::GetDispatchInfo, traits::fungible::Inspect};
-use sp_std::vec;
+use sp_std::{prelude::*, vec};
 use xcm::{
-	latest::{prelude::*, MaxDispatchErrorLen, MaybeErrorCode, Weight},
+	latest::{prelude::*, MaxDispatchErrorLen, MaybeErrorCode, Weight, MAX_ITEMS_IN_ASSETS},
 	DoubleEncoded,
 };
 use xcm_executor::{
@@ -32,7 +32,6 @@ use xcm_executor::{
 benchmarks! {
 	report_holding {
 		let (sender_account, sender_location) = account_and_location::<T>(1);
-		let holding = T::worst_case_holding(0);
 		let destination = T::valid_destination().map_err(|_| BenchmarkError::Skip)?;
 
 		let (expected_fees_mode, expected_assets_in_holding) = T::DeliveryHelper::ensure_successful_delivery(
@@ -42,23 +41,31 @@ benchmarks! {
 		);
 		let sender_account_balance_before = T::TransactAsset::balance(&sender_account);
 
+		// generate holding and add possible required fees
+		let holding = if let Some(expected_assets_in_holding) = expected_assets_in_holding {
+			let mut holding = T::worst_case_holding(expected_assets_in_holding.len() as u32);
+			for a in expected_assets_in_holding.into_inner() {
+				holding.push(a);
+			}
+			holding
+		} else {
+			T::worst_case_holding(0)
+		};
+
 		let mut executor = new_executor::<T>(sender_location);
 		executor.set_holding(holding.clone().into());
 		if let Some(expected_fees_mode) = expected_fees_mode {
 			executor.set_fees_mode(expected_fees_mode);
 		}
-		if let Some(expected_assets_in_holding) = expected_assets_in_holding {
-			executor.set_holding(expected_assets_in_holding.into());
-		}
 
 		let instruction = Instruction::<XcmCallOf<T>>::ReportHolding {
 			response_info: QueryResponseInfo {
 				destination,
 				query_id: Default::default(),
 				max_weight: Weight::MAX,
 			},
-			// Worst case is looking through all holdings for every asset explicitly.
-			assets: Definite(holding),
+			// Worst case is looking through all holdings for every asset explicitly - respecting the limit `MAX_ITEMS_IN_ASSETS`.
+			assets: Definite(holding.into_inner().into_iter().take(MAX_ITEMS_IN_ASSETS).collect::<Vec<_>>().into()),
 		};
 
 		let xcm = Xcm(vec![instruction]);
@@ -612,14 +619,19 @@ benchmarks! {
 		let sender_account = T::AccountIdConverter::convert_location(&owner).unwrap();
 		let sender_account_balance_before = T::TransactAsset::balance(&sender_account);
 
+		// generate holding and add possible required fees
+		let mut holding: Assets = asset.clone().into();
+		if let Some(expected_assets_in_holding) = expected_assets_in_holding {
+			for a in expected_assets_in_holding.into_inner() {
+				holding.push(a);
+			}
+		};
+
 		let mut executor = new_executor::<T>(owner);
-		executor.set_holding(asset.clone().into());
+		executor.set_holding(holding.into());
 		if let Some(expected_fees_mode) = expected_fees_mode {
 			executor.set_fees_mode(expected_fees_mode);
 		}
-		if let Some(expected_assets_in_holding) = expected_assets_in_holding {
-			executor.set_holding(expected_assets_in_holding.into());
-		}
 
 		let instruction = Instruction::LockAsset { asset, unlocker };
 		let xcm = Xcm(vec![instruction]);