Add validate_xcm_nesting to the ParentAsUmp and ChildParachainRouter (#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](https://github.com/paritytech/polkadot-sdk/pull/4186#discussion_r1571344270)
and/or
[here](https://github.com/paritytech/polkadot-sdk/pull/4186#discussion_r1572076666)
and/or [here]()

## Question/TODO

- [x] To the
[comment](https://github.com/paritytech/polkadot-sdk/pull/4186#discussion_r1572072295)
- 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](https://github.com/paritytech/polkadot-sdk/pull/4236#discussion_r1574605191))
This commit is contained in:
Branislav Kontur
2024-04-23 10:38:20 +02:00
committed by GitHub
parent 157294b0d3
commit 7f1646eb38
12 changed files with 237 additions and 69 deletions
+2 -15
View File
@@ -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;
@@ -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()
}
}
@@ -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()
}
}
+28
View File
@@ -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 {
@@ -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! {
+42 -2
View File
@@ -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))
);
});
}
}
@@ -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)?;
@@ -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,
)
}
}
@@ -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,14 +41,22 @@ 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 {
@@ -57,8 +64,8 @@ benchmarks! {
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]);
@@ -132,9 +132,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,
)
}
}
@@ -50,6 +50,8 @@ pub trait Config: frame_system::Config {
fn valid_destination() -> Result<Location, BenchmarkError>;
/// Worst case scenario for a holding account in this runtime.
/// - `depositable_count` specifies the count of assets we plan to add to the holding on top of
/// those generated by the `worst_case_holding` implementation.
fn worst_case_holding(depositable_count: u32) -> Assets;
}
@@ -64,19 +66,22 @@ pub type AssetTransactorOf<T> = <<T as Config>::XcmConfig as XcmConfig>::AssetTr
/// The call type of executor's config. Should eventually resolve to the same overarching call type.
pub type XcmCallOf<T> = <<T as Config>::XcmConfig as XcmConfig>::RuntimeCall;
pub fn mock_worst_case_holding(depositable_count: u32, max_assets: u32) -> Assets {
pub fn generate_holding_assets(max_assets: u32) -> Assets {
let fungibles_amount: u128 = 100;
let holding_fungibles = max_assets / 2 - depositable_count;
let holding_non_fungibles = holding_fungibles;
let holding_fungibles = max_assets / 2;
let holding_non_fungibles = max_assets - holding_fungibles - 1; // -1 because of adding `Here` asset
// add count of `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
}
.into()
})
// add one more `Here` asset
.chain(core::iter::once(Asset { id: AssetId(Here.into()), fun: Fungible(u128::MAX) }))
// add count of `holding_non_fungibles`
.chain((0..holding_non_fungibles).map(|i| Asset {
id: AssetId(GeneralIndex(i as u128).into()),
fun: NonFungible(asset_instance_from(i)),
+92 -1
View File
@@ -25,7 +25,7 @@
extern crate alloc;
use derivative::Derivative;
use parity_scale_codec::{Decode, Encode, Error as CodecError, Input, MaxEncodedLen};
use parity_scale_codec::{Decode, DecodeLimit, Encode, Error as CodecError, Input, MaxEncodedLen};
use scale_info::TypeInfo;
pub mod v2;
@@ -456,6 +456,23 @@ impl<C> IdentifyVersion for VersionedXcm<C> {
}
}
impl<C> VersionedXcm<C> {
/// Checks that the XCM is decodable with `MAX_XCM_DECODE_DEPTH`. Consequently, it also checks
/// all decode implementations and limits, such as MAX_ITEMS_IN_ASSETS or
/// MAX_INSTRUCTIONS_TO_DECODE.
///
/// Note that this uses the limit of the sender - not the receiver. It is a best effort.
pub fn validate_xcm_nesting(&self) -> Result<(), ()> {
self.using_encoded(|mut enc| {
Self::decode_all_with_depth_limit(MAX_XCM_DECODE_DEPTH, &mut enc).map(|_| ())
})
.map_err(|e| {
log::error!(target: "xcm::validate_xcm_nesting", "Decode error: {e:?} for xcm: {self:?}!");
()
})
}
}
impl<RuntimeCall> From<v2::Xcm<RuntimeCall>> for VersionedXcm<RuntimeCall> {
fn from(x: v2::Xcm<RuntimeCall>) -> Self {
VersionedXcm::V2(x)
@@ -704,3 +721,77 @@ fn size_limits() {
}
assert!(!test_failed);
}
#[test]
fn validate_xcm_nesting_works() {
use crate::latest::{
prelude::{GeneralIndex, ReserveAssetDeposited, SetAppendix},
Assets, Xcm, MAX_INSTRUCTIONS_TO_DECODE, MAX_ITEMS_IN_ASSETS,
};
// closure generates assets of `count`
let assets = |count| {
let mut assets = Assets::new();
for i in 0..count {
assets.push((GeneralIndex(i as u128), 100).into());
}
assets
};
// closer generates `Xcm` with nested instructions of `depth`
let with_instr = |depth| {
let mut xcm = Xcm::<()>(vec![]);
for _ in 0..depth - 1 {
xcm = Xcm::<()>(vec![SetAppendix(xcm)]);
}
xcm
};
// `MAX_INSTRUCTIONS_TO_DECODE` check
assert!(VersionedXcm::<()>::from(Xcm(vec![
ReserveAssetDeposited(assets(1));
(MAX_INSTRUCTIONS_TO_DECODE - 1) as usize
]))
.validate_xcm_nesting()
.is_ok());
assert!(VersionedXcm::<()>::from(Xcm(vec![
ReserveAssetDeposited(assets(1));
MAX_INSTRUCTIONS_TO_DECODE as usize
]))
.validate_xcm_nesting()
.is_ok());
assert!(VersionedXcm::<()>::from(Xcm(vec![
ReserveAssetDeposited(assets(1));
(MAX_INSTRUCTIONS_TO_DECODE + 1) as usize
]))
.validate_xcm_nesting()
.is_err());
// `MAX_XCM_DECODE_DEPTH` check
assert!(VersionedXcm::<()>::from(with_instr(MAX_XCM_DECODE_DEPTH - 1))
.validate_xcm_nesting()
.is_ok());
assert!(VersionedXcm::<()>::from(with_instr(MAX_XCM_DECODE_DEPTH))
.validate_xcm_nesting()
.is_ok());
assert!(VersionedXcm::<()>::from(with_instr(MAX_XCM_DECODE_DEPTH + 1))
.validate_xcm_nesting()
.is_err());
// `MAX_ITEMS_IN_ASSETS` check
assert!(VersionedXcm::<()>::from(Xcm(vec![ReserveAssetDeposited(assets(
MAX_ITEMS_IN_ASSETS
))]))
.validate_xcm_nesting()
.is_ok());
assert!(VersionedXcm::<()>::from(Xcm(vec![ReserveAssetDeposited(assets(
MAX_ITEMS_IN_ASSETS - 1
))]))
.validate_xcm_nesting()
.is_ok());
assert!(VersionedXcm::<()>::from(Xcm(vec![ReserveAssetDeposited(assets(
MAX_ITEMS_IN_ASSETS + 1
))]))
.validate_xcm_nesting()
.is_err());
}