Ensure outbound XCMs are decodable with limits + add EnsureDecodableXcm router (for testing purposes) (#4186)

This PR:
- adds `EnsureDecodableXcm` (testing) router that attempts to *encode*
and *decode* passed XCM `message` to ensure that the receiving side will
be able to decode, at least with the same XCM version.
- fixes `pallet_xcm` / `pallet_xcm_benchmarks` assets data generation

Relates to investigation of
https://substrate.stackexchange.com/questions/11288 and missing fix
#2129 which did not get
into the fellows 1.1.X release.

## Questions/TODOs

- [x] fix XCM benchmarks, which produces undecodable data - new router
catched at least two cases
  - `BoundedVec exceeds its limit`
  - `Fungible asset of zero amount is not allowed`  
- [x] do we need to add `sort` to the `prepend_with` as we did for
reanchor [here](#2129)?
@serban300 (**created separate/follow-up PR**:
#4235)
- [x] We added decoding check to `XcmpQueue` -> `validate_xcm_nesting`,
why not to added to the `ParentAsUmp` or `ChildParachainRouter`?
@franciscoaguirre (**created separate/follow-up PR**:
#4236)
- [ ] `SendController::send_blob` replace `VersionedXcm::<()>::decode(`
with `VersionedXcm::<()>::decode_with_depth_limit(MAX_XCM_DECODE_DEPTH,
data)` ?

---------

Co-authored-by: Adrian Catangiu <[email protected]>

Author: bkontur

Cargo.lock

@@ -11,14 +11,10 @@ license.workspace = true
 workspace = true
 
 [dependencies]
-bitvec = { version = "1.0.0", default-features = false, features = ["alloc"] }
 parity-scale-codec = { version = "3.6.1", default-features = false, features = ["derive"] }
 log = { workspace = true }
-rustc-hex = { version = "2.1.0", default-features = false }
 scale-info = { version = "2.11.1", default-features = false, features = ["derive"] }
 serde = { workspace = true }
-serde_derive = { optional = true, workspace = true }
-smallvec = "1.8.0"
 
 authority-discovery-primitives = { package = "sp-authority-discovery", path = "../../../substrate/primitives/authority-discovery", default-features = false }
 babe-primitives = { package = "sp-consensus-babe", path = "../../../substrate/primitives/consensus/babe", default-features = false }
@@ -63,7 +59,6 @@ pallet-vesting = { path = "../../../substrate/frame/vesting", default-features =
 runtime-common = { package = "polkadot-runtime-common", path = "../common", default-features = false }
 primitives = { package = "polkadot-primitives", path = "../../primitives", default-features = false }
 pallet-xcm = { path = "../../xcm/pallet-xcm", default-features = false }
-polkadot-parachain-primitives = { path = "../../parachain", default-features = false }
 polkadot-runtime-parachains = { path = "../parachains", default-features = false }
 xcm-builder = { package = "staging-xcm-builder", path = "../../xcm/xcm-builder", default-features = false }
 xcm-executor = { package = "staging-xcm-executor", path = "../../xcm/xcm-executor", default-features = false }
@@ -92,7 +87,6 @@ std = [
 	"authority-discovery-primitives/std",
 	"babe-primitives/std",
 	"beefy-primitives/std",
-	"bitvec/std",
 	"block-builder-api/std",
 	"frame-election-provider-support/std",
 	"frame-executive/std",
@@ -118,14 +112,11 @@ std = [
 	"pallet-vesting/std",
 	"pallet-xcm/std",
 	"parity-scale-codec/std",
-	"polkadot-parachain-primitives/std",
 	"polkadot-runtime-parachains/std",
 	"primitives/std",
 	"runtime-common/std",
-	"rustc-hex/std",
 	"scale-info/std",
 	"serde/std",
-	"serde_derive",
 	"sp-api/std",
 	"sp-core/std",
 	"sp-genesis-builder/std",
@@ -157,7 +148,6 @@ runtime-benchmarks = [
 	"pallet-timestamp/runtime-benchmarks",
 	"pallet-vesting/runtime-benchmarks",
 	"pallet-xcm/runtime-benchmarks",
-	"polkadot-parachain-primitives/runtime-benchmarks",
 	"polkadot-runtime-parachains/runtime-benchmarks",
 	"primitives/runtime-benchmarks",
 	"runtime-common/runtime-benchmarks",

polkadot/runtime/test-runtime/Cargo.toml

@@ -14,18 +14,12 @@ smallvec = "1.8.0"
 
 frame-support = { path = "../../../../substrate/frame/support", default-features = false }
 primitives = { package = "polkadot-primitives", path = "../../../primitives", default-features = false }
-runtime-common = { package = "polkadot-runtime-common", path = "../../common", default-features = false }
 sp-runtime = { path = "../../../../substrate/primitives/runtime", default-features = false }
-sp-weights = { path = "../../../../substrate/primitives/weights", default-features = false }
-sp-core = { path = "../../../../substrate/primitives/core", default-features = false }
 
 [features]
 default = ["std"]
 std = [
 	"frame-support/std",
 	"primitives/std",
-	"runtime-common/std",
-	"sp-core/std",
 	"sp-runtime/std",
-	"sp-weights/std",
 ]

polkadot/runtime/test-runtime/constants/Cargo.toml

@@ -23,7 +23,9 @@ use frame_support::{
 	traits::{Everything, Nothing},
 };
 use xcm::latest::prelude::*;
-use xcm_builder::{AllowUnpaidExecutionFrom, FrameTransactionalProcessor, MintLocation};
+use xcm_builder::{
+	AllowUnpaidExecutionFrom, EnsureDecodableXcm, FrameTransactionalProcessor, MintLocation,
+};
 
 type Block = frame_system::mocking::MockBlock<Test>;
 
@@ -91,7 +93,7 @@ parameter_types! {
 pub struct XcmConfig;
 impl xcm_executor::Config for XcmConfig {
 	type RuntimeCall = RuntimeCall;
-	type XcmSender = DevNull;
+	type XcmSender = EnsureDecodableXcm<DevNull>;
 	type AssetTransactor = AssetTransactor;
 	type OriginConverter = ();
 	type IsReserve = TrustedReserves;

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

@@ -28,7 +28,8 @@ use xcm_builder::{
 		AssetsInHolding, TestAssetExchanger, TestAssetLocker, TestAssetTrap,
 		TestSubscriptionService, TestUniversalAliases,
 	},
-	AliasForeignAccountId32, AllowUnpaidExecutionFrom, FrameTransactionalProcessor,
+	AliasForeignAccountId32, AllowUnpaidExecutionFrom, EnsureDecodableXcm,
+	FrameTransactionalProcessor,
 };
 use xcm_executor::traits::ConvertOrigin;
 
@@ -81,7 +82,7 @@ type Aliasers = AliasForeignAccountId32<OnlyParachains>;
 pub struct XcmConfig;
 impl xcm_executor::Config for XcmConfig {
 	type RuntimeCall = RuntimeCall;
-	type XcmSender = DevNull;
+	type XcmSender = EnsureDecodableXcm<DevNull>;
 	type AssetTransactor = NoAssetTransactor;
 	type OriginConverter = AlwaysSignedByDefault<RuntimeOrigin>;
 	type IsReserve = AllAssetLocationsPass;

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

@@ -33,10 +33,11 @@ use xcm::prelude::*;
 use xcm_builder::{
 	AccountId32Aliases, AllowKnownQueryResponses, AllowSubscriptionsFrom,
 	AllowTopLevelPaidExecutionFrom, Case, ChildParachainAsNative, ChildParachainConvertsVia,
-	ChildSystemParachainAsSuperuser, DescribeAllTerminal, FixedRateOfFungible, FixedWeightBounds,
-	FrameTransactionalProcessor, FungibleAdapter, FungiblesAdapter, HashedDescription, IsConcrete,
-	MatchedConvertedConcreteId, NoChecking, SignedAccountId32AsNative, SignedToAccountId32,
-	SovereignSignedViaLocation, TakeWeightCredit, XcmFeeManagerFromComponents, XcmFeeToAccount,
+	ChildSystemParachainAsSuperuser, DescribeAllTerminal, EnsureDecodableXcm, FixedRateOfFungible,
+	FixedWeightBounds, FrameTransactionalProcessor, FungibleAdapter, FungiblesAdapter,
+	HashedDescription, IsConcrete, MatchedConvertedConcreteId, NoChecking,
+	SignedAccountId32AsNative, SignedToAccountId32, SovereignSignedViaLocation, TakeWeightCredit,
+	XcmFeeManagerFromComponents, XcmFeeToAccount,
 };
 use xcm_executor::{
 	traits::{Identity, JustTry},
@@ -488,7 +489,8 @@ pub type Barrier = (
 	AllowSubscriptionsFrom<Everything>,
 );
 
-pub type XcmRouter = (TestPaidForPara3000SendXcm, TestSendXcmErrX8, TestSendXcm);
+pub type XcmRouter =
+	EnsureDecodableXcm<(TestPaidForPara3000SendXcm, TestSendXcmErrX8, TestSendXcm)>;
 
 pub struct XcmConfig;
 impl xcm_executor::Config for XcmConfig {

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

@@ -119,7 +119,7 @@ mod process_xcm_message;
 pub use process_xcm_message::ProcessXcmMessage;
 
 mod routing;
-pub use routing::{EnsureDelivery, WithTopicSource, WithUniqueTopic};
+pub use routing::{EnsureDecodableXcm, EnsureDelivery, WithTopicSource, WithUniqueTopic};
 
 mod transactional;
 pub use transactional::FrameTransactionalProcessor;

polkadot/xcm/xcm-builder/src/lib.rs

@@ -139,3 +139,37 @@ impl EnsureDelivery for Tuple {
 		(None, None)
 	}
 }
+
+/// A wrapper router that attempts to *encode* and *decode* passed XCM `message` to ensure that the
+/// receiving side will be able to decode, at least with the same XCM version.
+///
+/// This is designed to be at the top-level of any routers which do the real delivery. While other
+/// routers can manipulate the `message`, we cannot access the final XCM due to the generic
+/// `Inner::Ticket`. Therefore, this router aims to validate at least the passed `message`.
+///
+/// NOTE: For use in mock runtimes which don't have the DMP/UMP/HRMP XCM validations.
+pub struct EnsureDecodableXcm<Inner>(sp_std::marker::PhantomData<Inner>);
+impl<Inner: SendXcm> SendXcm for EnsureDecodableXcm<Inner> {
+	type Ticket = Inner::Ticket;
+
+	fn validate(
+		destination: &mut Option<Location>,
+		message: &mut Option<Xcm<()>>,
+	) -> SendResult<Self::Ticket> {
+		if let Some(msg) = message {
+			let versioned_xcm = VersionedXcm::<()>::from(msg.clone());
+			if versioned_xcm.validate_xcm_nesting().is_err() {
+				log::error!(
+					target: "xcm::validate_xcm_nesting",
+					"EnsureDecodableXcm validate_xcm_nesting error for \nversioned_xcm: {versioned_xcm:?}\nbased on xcm: {msg:?}"
+				);
+				return Err(SendError::Transport("EnsureDecodableXcm validate_xcm_nesting error"))
+			}
+		}
+		Inner::validate(destination, message)
+	}
+
+	fn deliver(ticket: Self::Ticket) -> Result<XcmHash, SendError> {
+		Inner::deliver(ticket)
+	}
+}

polkadot/xcm/xcm-builder/src/routing.rs

@@ -19,6 +19,7 @@
 use crate::{
 	barriers::{AllowSubscriptionsFrom, RespectSuspension, TrailingSetTopicAsId},
 	test_utils::*,
+	EnsureDecodableXcm,
 };
 pub use crate::{
 	AliasForeignAccountId32, AllowExplicitUnpaidExecutionFrom, AllowKnownQueryResponses,
@@ -165,8 +166,8 @@ pub fn set_exporter_override(
 pub fn clear_exporter_override() {
 	EXPORTER_OVERRIDE.with(|x| x.replace(None));
 }
-pub struct TestMessageSender;
-impl SendXcm for TestMessageSender {
+pub struct TestMessageSenderImpl;
+impl SendXcm for TestMessageSenderImpl {
 	type Ticket = (Location, Xcm<()>, XcmHash);
 	fn validate(
 		dest: &mut Option<Location>,
@@ -183,6 +184,8 @@ impl SendXcm for TestMessageSender {
 		Ok(hash)
 	}
 }
+pub type TestMessageSender = EnsureDecodableXcm<TestMessageSenderImpl>;
+
 pub struct TestMessageExporter;
 impl ExportXcm for TestMessageExporter {
 	type Ticket = (NetworkId, u32, InteriorLocation, InteriorLocation, Xcm<()>, XcmHash);

polkadot/xcm/xcm-builder/src/tests/mock.rs

@@ -35,9 +35,9 @@ use staging_xcm_builder as xcm_builder;
 use xcm_builder::{
 	AccountId32Aliases, AllowTopLevelPaidExecutionFrom, AllowUnpaidExecutionFrom,
 	ChildParachainAsNative, ChildParachainConvertsVia, ChildSystemParachainAsSuperuser,
-	FixedRateOfFungible, FixedWeightBounds, FungibleAdapter, IsChildSystemParachain, IsConcrete,
-	MintLocation, RespectSuspension, SignedAccountId32AsNative, SignedToAccountId32,
-	SovereignSignedViaLocation, TakeWeightCredit,
+	EnsureDecodableXcm, FixedRateOfFungible, FixedWeightBounds, FungibleAdapter,
+	IsChildSystemParachain, IsConcrete, MintLocation, RespectSuspension, SignedAccountId32AsNative,
+	SignedToAccountId32, SovereignSignedViaLocation, TakeWeightCredit,
 };
 
 pub type AccountId = AccountId32;
@@ -68,6 +68,8 @@ impl SendXcm for TestSendXcm {
 	}
 }
 
+pub type TestXcmRouter = EnsureDecodableXcm<TestSendXcm>;
+
 // copied from kusama constants
 pub const UNITS: Balance = 1_000_000_000_000;
 pub const CENTS: Balance = UNITS / 30_000;
@@ -180,7 +182,7 @@ pub type TrustedTeleporters = (xcm_builder::Case<KusamaForAssetHub>,);
 pub struct XcmConfig;
 impl xcm_executor::Config for XcmConfig {
 	type RuntimeCall = RuntimeCall;
-	type XcmSender = TestSendXcm;
+	type XcmSender = TestXcmRouter;
 	type AssetTransactor = LocalAssetTransactor;
 	type OriginConverter = LocalOriginConverter;
 	type IsReserve = ();
@@ -215,7 +217,7 @@ impl pallet_xcm::Config for Runtime {
 	type RuntimeEvent = RuntimeEvent;
 	type UniversalLocation = UniversalLocation;
 	type SendXcmOrigin = xcm_builder::EnsureXcmOrigin<RuntimeOrigin, LocalOriginToLocation>;
-	type XcmRouter = TestSendXcm;
+	type XcmRouter = TestXcmRouter;
 	// Anyone can execute XCM messages locally...
 	type ExecuteXcmOrigin = xcm_builder::EnsureXcmOrigin<RuntimeOrigin, LocalOriginToLocation>;
 	type XcmExecuteFilter = Nothing;

polkadot/xcm/xcm-builder/tests/mock/mod.rs

@@ -40,10 +40,10 @@ use polkadot_parachain_primitives::primitives::{
 use xcm::{latest::prelude::*, VersionedXcm};
 use xcm_builder::{
 	Account32Hash, AccountId32Aliases, AllowUnpaidExecutionFrom, ConvertedConcreteId,
-	EnsureXcmOrigin, FixedRateOfFungible, FixedWeightBounds, FrameTransactionalProcessor,
-	FungibleAdapter, IsConcrete, NativeAsset, NoChecking, NonFungiblesAdapter, ParentIsPreset,
-	SiblingParachainConvertsVia, SignedAccountId32AsNative, SignedToAccountId32,
-	SovereignSignedViaLocation,
+	EnsureDecodableXcm, EnsureXcmOrigin, FixedRateOfFungible, FixedWeightBounds,
+	FrameTransactionalProcessor, FungibleAdapter, IsConcrete, NativeAsset, NoChecking,
+	NonFungiblesAdapter, ParentIsPreset, SiblingParachainConvertsVia, SignedAccountId32AsNative,
+	SignedToAccountId32, SovereignSignedViaLocation,
 };
 use xcm_executor::{
 	traits::{ConvertLocation, JustTry},
@@ -212,7 +212,7 @@ pub type LocalAssetTransactor = (
 	>,
 );
 
-pub type XcmRouter = super::ParachainXcmRouter<MsgQueue>;
+pub type XcmRouter = EnsureDecodableXcm<super::ParachainXcmRouter<MsgQueue>>;
 pub type Barrier = AllowUnpaidExecutionFrom<Everything>;
 
 parameter_types! {

polkadot/xcm/xcm-simulator/example/src/parachain.rs

@@ -36,9 +36,9 @@ use xcm::latest::prelude::*;
 use xcm_builder::{
 	Account32Hash, AccountId32Aliases, AllowUnpaidExecutionFrom, AsPrefixedGeneralIndex,
 	ChildParachainAsNative, ChildParachainConvertsVia, ChildSystemParachainAsSuperuser,
-	ConvertedConcreteId, FixedRateOfFungible, FixedWeightBounds, FrameTransactionalProcessor,
-	FungibleAdapter, IsConcrete, NoChecking, NonFungiblesAdapter, SignedAccountId32AsNative,
-	SignedToAccountId32, SovereignSignedViaLocation,
+	ConvertedConcreteId, EnsureDecodableXcm, FixedRateOfFungible, FixedWeightBounds,
+	FrameTransactionalProcessor, FungibleAdapter, IsConcrete, NoChecking, NonFungiblesAdapter,
+	SignedAccountId32AsNative, SignedToAccountId32, SovereignSignedViaLocation,
 };
 use xcm_executor::{traits::JustTry, Config, XcmExecutor};
 
@@ -168,7 +168,7 @@ parameter_types! {
 	pub const MaxAssetsIntoHolding: u32 = 64;
 }
 
-pub type XcmRouter = super::RelayChainXcmRouter;
+pub type XcmRouter = EnsureDecodableXcm<super::RelayChainXcmRouter>;
 pub type Barrier = AllowUnpaidExecutionFrom<Everything>;
 
 pub struct XcmConfig;