Create thread_local in XCM executor to limit recursion depth (#6304)

* Create thread_local in XCM executor to limit recursion depth

* Add unit test for recursion limit

* Fix statefulness in tests

* Remove panic

* Use defer and environmental macro

* Fix the implementation

* Use nicer interface

* Change ThisNetwork to AnyNetwork

* Move recursion check up to top level

* cargo fmt

* Update comment

Co-authored-by: Bastian Köcher <info@kchr.de>

runtime/test-runtime/src/lib.rs

@@ -131,7 +131,7 @@ parameter_types! {
 }
 
 impl frame_system::Config for Runtime {
-	type BaseCallFilter = frame_support::traits::Everything;
+	type BaseCallFilter = Everything;
 	type BlockWeights = BlockWeights;
 	type BlockLength = BlockLength;
 	type DbWeight = ();
@@ -538,41 +538,6 @@ impl parachains_ump::Config for Runtime {
 	type WeightInfo = parachains_ump::TestWeightInfo;
 }
 
-parameter_types! {
-	pub const BaseXcmWeight: xcm::latest::Weight = Weight::from_parts(1_000, 1_000);
-	pub const AnyNetwork: Option<xcm::latest::NetworkId> = None;
-	pub const MaxInstructions: u32 = 100;
-	pub const UniversalLocation: xcm::latest::InteriorMultiLocation = xcm::latest::Junctions::Here;
-}
-
-pub type LocalOriginToLocation =
-	xcm_builder::SignedToAccountId32<RuntimeOrigin, AccountId, AnyNetwork>;
-
-impl pallet_xcm::Config for Runtime {
-	// The config types here are entirely configurable, since the only one that is sorely needed
-	// is `XcmExecutor`, which will be used in unit tests located in xcm-executor.
-	type RuntimeEvent = RuntimeEvent;
-	type ExecuteXcmOrigin = xcm_builder::EnsureXcmOrigin<RuntimeOrigin, LocalOriginToLocation>;
-	type UniversalLocation = UniversalLocation;
-	type SendXcmOrigin = xcm_builder::EnsureXcmOrigin<RuntimeOrigin, LocalOriginToLocation>;
-	type Weigher = xcm_builder::FixedWeightBounds<BaseXcmWeight, RuntimeCall, MaxInstructions>;
-	type XcmRouter = xcm_config::DoNothingRouter;
-	type XcmExecuteFilter = Everything;
-	type XcmExecutor = xcm_executor::XcmExecutor<xcm_config::XcmConfig>;
-	type XcmTeleportFilter = Everything;
-	type XcmReserveTransferFilter = Everything;
-	type RuntimeOrigin = RuntimeOrigin;
-	type RuntimeCall = RuntimeCall;
-	const VERSION_DISCOVERY_QUEUE_SIZE: u32 = 100;
-	type AdvertisedXcmVersion = pallet_xcm::CurrentXcmVersion;
-	type Currency = Balances;
-	type CurrencyMatcher = ();
-	type TrustedLockers = ();
-	type SovereignAccountOf = ();
-	type MaxLockers = frame_support::traits::ConstU32<8>;
-	type WeightInfo = pallet_xcm::TestWeightInfo;
-}
-
 impl parachains_hrmp::Config for Runtime {
 	type RuntimeOrigin = RuntimeOrigin;
 	type RuntimeEvent = RuntimeEvent;

runtime/test-runtime/src/xcm_config.rs

@@ -20,14 +20,18 @@ use frame_support::{
 	weights::Weight,
 };
 use xcm::latest::prelude::*;
-use xcm_builder::{AllowUnpaidExecutionFrom, FixedWeightBounds, SignedToAccountId32};
+use xcm_builder::{
+	AllowUnpaidExecutionFrom, EnsureXcmOrigin, FixedWeightBounds, SignedAccountId32AsNative,
+	SignedToAccountId32,
+};
 use xcm_executor::{
 	traits::{TransactAsset, WeightTrader},
 	Assets,
 };
 
 parameter_types! {
-	pub const OurNetwork: NetworkId = NetworkId::Polkadot;
+	pub const BaseXcmWeight: xcm::latest::Weight = Weight::from_parts(1_000, 1_000);
+	pub const AnyNetwork: Option<NetworkId> = None;
 	pub const MaxInstructions: u32 = 100;
 	pub const MaxAssetsIntoHolding: u32 = 16;
 	pub const UniversalLocation: xcm::latest::InteriorMultiLocation = xcm::latest::Junctions::Here;
@@ -37,7 +41,7 @@ parameter_types! {
 /// of this chain.
 pub type LocalOriginToLocation = (
 	// And a usual Signed origin to be used in XCM as a corresponding AccountId32
-	SignedToAccountId32<crate::RuntimeOrigin, crate::AccountId, OurNetwork>,
+	SignedToAccountId32<crate::RuntimeOrigin, crate::AccountId, AnyNetwork>,
 );
 
 pub struct DoNothingRouter;
@@ -80,17 +84,22 @@ impl WeightTrader for DummyWeightTrader {
 	}
 }
 
+type OriginConverter = (
+	pallet_xcm::XcmPassthrough<super::RuntimeOrigin>,
+	SignedAccountId32AsNative<AnyNetwork, super::RuntimeOrigin>,
+);
+
 pub struct XcmConfig;
 impl xcm_executor::Config for XcmConfig {
 	type RuntimeCall = super::RuntimeCall;
 	type XcmSender = DoNothingRouter;
 	type AssetTransactor = DummyAssetTransactor;
-	type OriginConverter = pallet_xcm::XcmPassthrough<super::RuntimeOrigin>;
+	type OriginConverter = OriginConverter;
 	type IsReserve = ();
 	type IsTeleporter = ();
 	type UniversalLocation = UniversalLocation;
 	type Barrier = Barrier;
-	type Weigher = FixedWeightBounds<super::BaseXcmWeight, super::RuntimeCall, MaxInstructions>;
+	type Weigher = FixedWeightBounds<BaseXcmWeight, super::RuntimeCall, MaxInstructions>;
 	type Trader = DummyWeightTrader;
 	type ResponseHandler = super::Xcm;
 	type AssetTrap = super::Xcm;
@@ -105,3 +114,28 @@ impl xcm_executor::Config for XcmConfig {
 	type UniversalAliases = Nothing;
 	type CallDispatcher = super::RuntimeCall;
 }
+
+impl pallet_xcm::Config for crate::Runtime {
+	// The config types here are entirely configurable, since the only one that is sorely needed
+	// is `XcmExecutor`, which will be used in unit tests located in xcm-executor.
+	type RuntimeEvent = crate::RuntimeEvent;
+	type ExecuteXcmOrigin = EnsureXcmOrigin<crate::RuntimeOrigin, LocalOriginToLocation>;
+	type UniversalLocation = UniversalLocation;
+	type SendXcmOrigin = EnsureXcmOrigin<crate::RuntimeOrigin, LocalOriginToLocation>;
+	type Weigher = FixedWeightBounds<BaseXcmWeight, crate::RuntimeCall, MaxInstructions>;
+	type XcmRouter = DoNothingRouter;
+	type XcmExecuteFilter = Everything;
+	type XcmExecutor = xcm_executor::XcmExecutor<XcmConfig>;
+	type XcmTeleportFilter = Everything;
+	type XcmReserveTransferFilter = Everything;
+	type RuntimeOrigin = crate::RuntimeOrigin;
+	type RuntimeCall = crate::RuntimeCall;
+	const VERSION_DISCOVERY_QUEUE_SIZE: u32 = 100;
+	type AdvertisedXcmVersion = pallet_xcm::CurrentXcmVersion;
+	type Currency = crate::Balances;
+	type CurrencyMatcher = ();
+	type TrustedLockers = ();
+	type SovereignAccountOf = ();
+	type MaxLockers = frame_support::traits::ConstU32<8>;
+	type WeightInfo = pallet_xcm::TestWeightInfo;
+}

xcm/src/v3/traits.rs

@@ -150,6 +150,8 @@ pub enum Error {
 	Barrier,
 	/// The weight of an XCM message is not computable ahead of execution.
 	WeightNotComputable,
+	/// Recursion stack limit reached
+	ExceedsStackLimit,
 }
 
 impl MaxEncodedLen for Error {

xcm/xcm-executor/Cargo.toml

@@ -7,6 +7,7 @@ version = "0.9.33"
 
 [dependencies]
 impl-trait-for-tuples = "0.2.2"
+environmental = { version = "1.1.4", default-features = false }
 parity-scale-codec = { version = "3.1.5", default-features = false, features = ["derive"] }
 xcm = { path = "..", default-features = false }
 sp-std = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }

xcm/xcm-executor/integration-tests/src/lib.rs

@@ -17,16 +17,17 @@
 #![cfg_attr(not(feature = "std"), no_std)]
 #![cfg(test)]
 
-use frame_support::weights::Weight;
+use frame_support::{codec::Encode, dispatch::GetDispatchInfo, weights::Weight};
 use polkadot_test_client::{
 	BlockBuilderExt, ClientBlockImportExt, DefaultTestClientBuilderExt, ExecutionStrategy,
 	InitPolkadotBlockBuilder, TestClientBuilder, TestClientBuilderExt,
 };
-use polkadot_test_runtime::pallet_test_notifier;
+use polkadot_test_runtime::{pallet_test_notifier, xcm_config::XcmConfig};
 use polkadot_test_service::construct_extrinsic;
 use sp_runtime::traits::Block;
 use sp_state_machine::InspectState;
 use xcm::{latest::prelude::*, VersionedResponse, VersionedXcm};
+use xcm_executor::traits::WeightBounds;
 
 #[test]
 fn basic_buy_fees_message_executes() {
@@ -71,6 +72,59 @@ fn basic_buy_fees_message_executes() {
 	});
 }
 
+#[test]
+fn transact_recursion_limit_works() {
+	sp_tracing::try_init_simple();
+	let mut client = TestClientBuilder::new()
+		.set_execution_strategy(ExecutionStrategy::AlwaysWasm)
+		.build();
+
+	let mut msg = Xcm(vec![ClearOrigin]);
+	let max_weight = <XcmConfig as xcm_executor::Config>::Weigher::weight(&mut msg).unwrap();
+	let mut call = polkadot_test_runtime::RuntimeCall::Xcm(pallet_xcm::Call::execute {
+		message: Box::new(VersionedXcm::from(msg)),
+		max_weight,
+	});
+
+	for _ in 0..11 {
+		let mut msg = Xcm(vec![
+			WithdrawAsset((Parent, 1_000).into()),
+			BuyExecution { fees: (Parent, 1).into(), weight_limit: Unlimited },
+			Transact {
+				origin_kind: OriginKind::Native,
+				require_weight_at_most: call.get_dispatch_info().weight,
+				call: call.encode().into(),
+			},
+		]);
+		let max_weight = <XcmConfig as xcm_executor::Config>::Weigher::weight(&mut msg).unwrap();
+		call = polkadot_test_runtime::RuntimeCall::Xcm(pallet_xcm::Call::execute {
+			message: Box::new(VersionedXcm::from(msg)),
+			max_weight,
+		});
+	}
+
+	let mut block_builder = client.init_polkadot_block_builder();
+
+	let execute = construct_extrinsic(&client, call, sp_keyring::Sr25519Keyring::Alice, 0);
+
+	block_builder.push_polkadot_extrinsic(execute).expect("pushes extrinsic");
+
+	let block = block_builder.build().expect("Finalizes the block").block;
+	let block_hash = block.hash();
+
+	futures::executor::block_on(client.import(sp_consensus::BlockOrigin::Own, block))
+		.expect("imports the block");
+
+	client.state_at(block_hash).expect("state should exist").inspect_state(|| {
+		assert!(polkadot_test_runtime::System::events().iter().any(|r| matches!(
+			r.event,
+			polkadot_test_runtime::RuntimeEvent::Xcm(pallet_xcm::Event::Attempted(
+				Outcome::Incomplete(_, XcmError::ExceedsStackLimit)
+			)),
+		)));
+	});
+}
+
 #[test]
 fn query_response_fires() {
 	use pallet_test_notifier::Event::*;

xcm/xcm-executor/src/lib.rs

@@ -22,6 +22,7 @@ use frame_support::{
 	traits::{Contains, ContainsPair, Get, PalletsInfoAccess},
 };
 use parity_scale_codec::{Decode, Encode};
+use sp_core::defer;
 use sp_io::hashing::blake2_128;
 use sp_std::{marker::PhantomData, prelude::*};
 use sp_weights::Weight;
@@ -44,6 +45,10 @@ pub struct FeesMode {
 	pub jit_withdraw: bool,
 }
 
+const RECURSION_LIMIT: u8 = 10;
+
+environmental::environmental!(recursion_count: u8);
+
 /// The XCM executor.
 pub struct XcmExecutor<Config: config::Config> {
 	holding: Assets,
@@ -302,15 +307,39 @@ impl<Config: config::Config> XcmExecutor<Config> {
 		let mut result = Ok(());
 		for (i, instr) in xcm.0.into_iter().enumerate() {
 			match &mut result {
-				r @ Ok(()) =>
-					if let Err(e) = self.process_instruction(instr) {
+				r @ Ok(()) => {
+					// Initialize the recursion count only the first time we hit this code in our
+					// potential recursive execution.
+					let inst_res = recursion_count::using_once(&mut 1, || {
+						recursion_count::with(|count| {
+							if *count > RECURSION_LIMIT {
+								return Err(XcmError::ExceedsStackLimit)
+							}
+							*count = count.saturating_add(1);
+							Ok(())
+						})
+						// This should always return `Some`, but let's play it safe.
+						.unwrap_or(Ok(()))?;
+
+						// Ensure that we always decrement the counter whenever we finish processing
+						// the instruction.
+						defer! {
+							recursion_count::with(|count| {
+								*count = count.saturating_sub(1);
+							});
+						}
+
+						self.process_instruction(instr)
+					});
+					if let Err(e) = inst_res {
 						log::trace!(target: "xcm::execute", "!!! ERROR: {:?}", e);
 						*r = Err(ExecutorError {
 							index: i as u32,
 							xcm_error: e,
 							weight: Weight::zero(),
 						});
-					},
+					}
+				},
 				Err(ref mut error) =>
 					if let Ok(x) = Config::Weigher::instr_weight(&instr) {
 						error.weight.saturating_accrue(x)