From c34826f7b484a1733f937be3a959297022066fc8 Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Fri, 2 Dec 2022 04:07:56 +0900 Subject: [PATCH] Create thread_local in XCM executor to limit recursion depth (#6304) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 --- Cargo.lock | 5 +- runtime/test-runtime/src/lib.rs | 37 +----------- runtime/test-runtime/src/xcm_config.rs | 44 ++++++++++++-- xcm/src/v3/traits.rs | 2 + xcm/xcm-executor/Cargo.toml | 1 + xcm/xcm-executor/integration-tests/src/lib.rs | 58 ++++++++++++++++++- xcm/xcm-executor/src/lib.rs | 35 ++++++++++- 7 files changed, 134 insertions(+), 48 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0d0ca953616b..2fa95ad85143 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1737,9 +1737,9 @@ dependencies = [ [[package]] name = "environmental" -version = "1.1.3" +version = "1.1.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "68b91989ae21441195d7d9b9993a2f9295c7e1a8c96255d8b729accddc124797" +checksum = "e48c92028aaa870e83d51c64e5d4e0b6981b360c522198c23959f219a4e1b15b" [[package]] name = "erased-serde" @@ -12702,6 +12702,7 @@ dependencies = [ name = "xcm-executor" version = "0.9.33" dependencies = [ + "environmental", "frame-benchmarking", "frame-support", "impl-trait-for-tuples", diff --git a/runtime/test-runtime/src/lib.rs b/runtime/test-runtime/src/lib.rs index c765490fadb9..5d4625d76b94 100644 --- a/runtime/test-runtime/src/lib.rs +++ b/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 = None; - pub const MaxInstructions: u32 = 100; - pub const UniversalLocation: xcm::latest::InteriorMultiLocation = xcm::latest::Junctions::Here; -} - -pub type LocalOriginToLocation = - xcm_builder::SignedToAccountId32; - -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; - type UniversalLocation = UniversalLocation; - type SendXcmOrigin = xcm_builder::EnsureXcmOrigin; - type Weigher = xcm_builder::FixedWeightBounds; - type XcmRouter = xcm_config::DoNothingRouter; - type XcmExecuteFilter = Everything; - type XcmExecutor = xcm_executor::XcmExecutor; - 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; diff --git a/runtime/test-runtime/src/xcm_config.rs b/runtime/test-runtime/src/xcm_config.rs index 992b116e98ea..2f850cf75976 100644 --- a/runtime/test-runtime/src/xcm_config.rs +++ b/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 = 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, + SignedToAccountId32, ); pub struct DoNothingRouter; @@ -80,17 +84,22 @@ impl WeightTrader for DummyWeightTrader { } } +type OriginConverter = ( + pallet_xcm::XcmPassthrough, + SignedAccountId32AsNative, +); + pub struct XcmConfig; impl xcm_executor::Config for XcmConfig { type RuntimeCall = super::RuntimeCall; type XcmSender = DoNothingRouter; type AssetTransactor = DummyAssetTransactor; - type OriginConverter = pallet_xcm::XcmPassthrough; + type OriginConverter = OriginConverter; type IsReserve = (); type IsTeleporter = (); type UniversalLocation = UniversalLocation; type Barrier = Barrier; - type Weigher = FixedWeightBounds; + type Weigher = FixedWeightBounds; 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; + type UniversalLocation = UniversalLocation; + type SendXcmOrigin = EnsureXcmOrigin; + type Weigher = FixedWeightBounds; + type XcmRouter = DoNothingRouter; + type XcmExecuteFilter = Everything; + type XcmExecutor = xcm_executor::XcmExecutor; + 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; +} diff --git a/xcm/src/v3/traits.rs b/xcm/src/v3/traits.rs index 4325eb272d19..eb5dd37cbc6c 100644 --- a/xcm/src/v3/traits.rs +++ b/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 { diff --git a/xcm/xcm-executor/Cargo.toml b/xcm/xcm-executor/Cargo.toml index b96b5b2f0ec7..a83379a91703 100644 --- a/xcm/xcm-executor/Cargo.toml +++ b/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 } diff --git a/xcm/xcm-executor/integration-tests/src/lib.rs b/xcm/xcm-executor/integration-tests/src/lib.rs index 0f23a375b99f..a3212c798dab 100644 --- a/xcm/xcm-executor/integration-tests/src/lib.rs +++ b/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 = ::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 = ::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::*; diff --git a/xcm/xcm-executor/src/lib.rs b/xcm/xcm-executor/src/lib.rs index 07e6c063c486..5906b2eef9c5 100644 --- a/xcm/xcm-executor/src/lib.rs +++ b/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 { holding: Assets, @@ -302,15 +307,39 @@ impl XcmExecutor { 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)