From 4bb4b5d381ec97d3c2627177ce97b60331bf8f87 Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Thu, 17 Nov 2022 17:06:56 +0900 Subject: [PATCH 01/11] Create thread_local in XCM executor to limit recursion depth --- xcm/src/v3/traits.rs | 2 ++ xcm/xcm-executor/src/lib.rs | 17 ++++++++++++++++- 2 files changed, 18 insertions(+), 1 deletion(-) 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/src/lib.rs b/xcm/xcm-executor/src/lib.rs index 07e6c063c486..2bd32713ef2e 100644 --- a/xcm/xcm-executor/src/lib.rs +++ b/xcm/xcm-executor/src/lib.rs @@ -23,7 +23,7 @@ use frame_support::{ }; use parity_scale_codec::{Decode, Encode}; use sp_io::hashing::blake2_128; -use sp_std::{marker::PhantomData, prelude::*}; +use sp_std::{cell::Cell, marker::PhantomData, prelude::*}; use sp_weights::Weight; use xcm::latest::prelude::*; @@ -44,6 +44,12 @@ pub struct FeesMode { pub jit_withdraw: bool, } +const RECURSION_LIMIT: u8 = 10; + +thread_local! { + static RECURSION_COUNT: Cell = Cell::new(0); +} + /// The XCM executor. pub struct XcmExecutor { holding: Assets, @@ -510,6 +516,15 @@ impl XcmExecutor { Ok(()) }, Transact { origin_kind, require_weight_at_most, mut call } => { + // Check whether we've exhausted the recursion limit + RECURSION_COUNT.with(|count| { + if count.get() > RECURSION_LIMIT { + return Err(XcmError::ExceedsStackLimit) + } + count.set(count.get().saturating_add(1)); + Ok(()) + })?; + // We assume that the Relay-chain is allowed to use transact on this parachain. let origin = self.origin_ref().ok_or(XcmError::BadOrigin)?.clone(); From 9d5b815d1467afe9cfea0ce17635f2490d3c261b Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Mon, 21 Nov 2022 16:42:31 +0900 Subject: [PATCH 02/11] Add unit test for recursion limit --- Cargo.lock | 1 + runtime/test-runtime/src/lib.rs | 37 +---------- runtime/test-runtime/src/xcm_config.rs | 32 +++++++++- xcm/xcm-executor/Cargo.toml | 1 + xcm/xcm-executor/integration-tests/src/lib.rs | 61 ++++++++++++++++++- xcm/xcm-executor/src/lib.rs | 5 +- 6 files changed, 95 insertions(+), 42 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1e5fdf3e1ecb..117c872a5b44 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -12684,6 +12684,7 @@ dependencies = [ name = "xcm-executor" version = "0.9.31" 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 1b33d24ced19..e78dc0e05ce7 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..8d0961335391 100644 --- a/runtime/test-runtime/src/xcm_config.rs +++ b/runtime/test-runtime/src/xcm_config.rs @@ -20,13 +20,16 @@ use frame_support::{ weights::Weight, }; use xcm::latest::prelude::*; -use xcm_builder::{AllowUnpaidExecutionFrom, FixedWeightBounds, SignedToAccountId32}; +use xcm_builder::{ + AllowUnpaidExecutionFrom, EnsureXcmOrigin, FixedWeightBounds, SignedToAccountId32, +}; use xcm_executor::{ traits::{TransactAsset, WeightTrader}, Assets, }; parameter_types! { + pub const BaseXcmWeight: xcm::latest::Weight = Weight::from_parts(1_000, 1_000); pub const OurNetwork: NetworkId = NetworkId::Polkadot; pub const MaxInstructions: u32 = 100; pub const MaxAssetsIntoHolding: u32 = 16; @@ -90,7 +93,7 @@ impl xcm_executor::Config for XcmConfig { 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 +108,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/xcm-executor/Cargo.toml b/xcm/xcm-executor/Cargo.toml index 6dd6aded96ae..2fde306e6bd7 100644 --- a/xcm/xcm-executor/Cargo.toml +++ b/xcm/xcm-executor/Cargo.toml @@ -7,6 +7,7 @@ version = "0.9.31" [dependencies] impl-trait-for-tuples = "0.2.2" +environmental = { version = "1.1.3", 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..68e9369d90c8 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, weights::Weight, dispatch::GetDispatchInfo}; 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,62 @@ 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 mut 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..10 { + msg = Xcm(vec![ + WithdrawAsset((Parent, 1_000_000_000).into()), + BuyExecution { fees: (Parent, 1_000_000_000).into(), weight_limit: Unlimited }, + Transact { + origin_kind: OriginKind::Xcm, + require_weight_at_most: call.get_dispatch_info().weight, + call: call.encode().into(), + }, + ]); + 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 2bd32713ef2e..259b79941369 100644 --- a/xcm/xcm-executor/src/lib.rs +++ b/xcm/xcm-executor/src/lib.rs @@ -46,8 +46,8 @@ pub struct FeesMode { const RECURSION_LIMIT: u8 = 10; -thread_local! { - static RECURSION_COUNT: Cell = Cell::new(0); +environmental::thread_local_impl! { + static RECURSION_COUNT: Cell = Cell::new(0) } /// The XCM executor. @@ -556,6 +556,7 @@ impl XcmExecutor { // weight consumed correctly (potentially allowing them to do more operations in a // block than they otherwise would). self.total_surplus.saturating_accrue(surplus); + RECURSION_COUNT.with(|count| count.set(count.get().saturating_sub(1))); Ok(()) }, QueryResponse { query_id, response, max_weight, querier } => { From 395a0997e2eafee75b501bcb3d0a9ce4b4d4ef91 Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Sat, 26 Nov 2022 20:13:03 +0900 Subject: [PATCH 03/11] Fix statefulness in tests --- xcm/xcm-executor/integration-tests/src/lib.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/xcm/xcm-executor/integration-tests/src/lib.rs b/xcm/xcm-executor/integration-tests/src/lib.rs index 68e9369d90c8..495549d280d2 100644 --- a/xcm/xcm-executor/integration-tests/src/lib.rs +++ b/xcm/xcm-executor/integration-tests/src/lib.rs @@ -80,13 +80,13 @@ fn transact_recursion_limit_works() { .build(); let mut msg = Xcm(vec![ClearOrigin]); - let mut max_weight = ::Weigher::weight(&mut msg).unwrap(); + 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..10 { - msg = Xcm(vec![ + let mut msg = Xcm(vec![ WithdrawAsset((Parent, 1_000_000_000).into()), BuyExecution { fees: (Parent, 1_000_000_000).into(), weight_limit: Unlimited }, Transact { @@ -95,7 +95,7 @@ fn transact_recursion_limit_works() { call: call.encode().into(), }, ]); - max_weight = ::Weigher::weight(&mut msg).unwrap(); + 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 } ); @@ -119,6 +119,7 @@ fn transact_recursion_limit_works() { .expect("imports the block"); client.state_at(block_hash).expect("state should exist").inspect_state(|| { + panic!("{:?}", polkadot_test_runtime::System::events()); assert!(polkadot_test_runtime::System::events().iter().any(|r| matches!( r.event, polkadot_test_runtime::RuntimeEvent::Xcm(pallet_xcm::Event::Attempted( From 85fe5c26219dd2b5d829f52d8df7d88684f33124 Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Sat, 26 Nov 2022 20:26:59 +0900 Subject: [PATCH 04/11] Remove panic --- xcm/xcm-executor/integration-tests/src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/xcm/xcm-executor/integration-tests/src/lib.rs b/xcm/xcm-executor/integration-tests/src/lib.rs index 495549d280d2..54a1ee6de77d 100644 --- a/xcm/xcm-executor/integration-tests/src/lib.rs +++ b/xcm/xcm-executor/integration-tests/src/lib.rs @@ -85,7 +85,7 @@ fn transact_recursion_limit_works() { pallet_xcm::Call::execute { message: Box::new(VersionedXcm::from(msg)), max_weight }, ); - for _ in 0..10 { + for _ in 0..11 { let mut msg = Xcm(vec![ WithdrawAsset((Parent, 1_000_000_000).into()), BuyExecution { fees: (Parent, 1_000_000_000).into(), weight_limit: Unlimited }, @@ -119,7 +119,6 @@ fn transact_recursion_limit_works() { .expect("imports the block"); client.state_at(block_hash).expect("state should exist").inspect_state(|| { - panic!("{:?}", polkadot_test_runtime::System::events()); assert!(polkadot_test_runtime::System::events().iter().any(|r| matches!( r.event, polkadot_test_runtime::RuntimeEvent::Xcm(pallet_xcm::Event::Attempted( From 661c0e1efd1519888b46cb0647ee186b168358ec Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Sun, 27 Nov 2022 11:18:42 +0900 Subject: [PATCH 05/11] Use defer and environmental macro --- xcm/xcm-executor/integration-tests/src/lib.rs | 2 +- xcm/xcm-executor/src/lib.rs | 34 +++++++++++++------ 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/xcm/xcm-executor/integration-tests/src/lib.rs b/xcm/xcm-executor/integration-tests/src/lib.rs index 54a1ee6de77d..c211cc3b96ef 100644 --- a/xcm/xcm-executor/integration-tests/src/lib.rs +++ b/xcm/xcm-executor/integration-tests/src/lib.rs @@ -85,7 +85,7 @@ fn transact_recursion_limit_works() { pallet_xcm::Call::execute { message: Box::new(VersionedXcm::from(msg)), max_weight }, ); - for _ in 0..11 { + for _ in 0..10 { let mut msg = Xcm(vec![ WithdrawAsset((Parent, 1_000_000_000).into()), BuyExecution { fees: (Parent, 1_000_000_000).into(), weight_limit: Unlimited }, diff --git a/xcm/xcm-executor/src/lib.rs b/xcm/xcm-executor/src/lib.rs index 259b79941369..9544377c9ea6 100644 --- a/xcm/xcm-executor/src/lib.rs +++ b/xcm/xcm-executor/src/lib.rs @@ -22,8 +22,9 @@ 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::{cell::Cell, marker::PhantomData, prelude::*}; +use sp_std::{marker::PhantomData, prelude::*}; use sp_weights::Weight; use xcm::latest::prelude::*; @@ -46,9 +47,7 @@ pub struct FeesMode { const RECURSION_LIMIT: u8 = 10; -environmental::thread_local_impl! { - static RECURSION_COUNT: Cell = Cell::new(0) -} +environmental::environmental!(recursion_count: u8); /// The XCM executor. pub struct XcmExecutor { @@ -517,14 +516,28 @@ impl XcmExecutor { }, Transact { origin_kind, require_weight_at_most, mut call } => { // Check whether we've exhausted the recursion limit - RECURSION_COUNT.with(|count| { - if count.get() > RECURSION_LIMIT { - return Err(XcmError::ExceedsStackLimit) - } - count.set(count.get().saturating_add(1)); - Ok(()) + recursion_count::using(&mut 0, || { + recursion_count::with(|count| { + if *count > RECURSION_LIMIT { + return Err(XcmError::ExceedsStackLimit) + } + *count = count.saturating_add(1); + Ok(()) + }) + .expect("`with` is being used within a `using` block; qed") })?; + // Ensure that we always decrement the counter whenever we finish executing + // `Transact` + defer! { + recursion_count::using(&mut 0, || { + recursion_count::with(|count| { + *count = count.saturating_sub(1); + }) + .expect("`with` is being used within a `using` block; qed"); + }); + } + // We assume that the Relay-chain is allowed to use transact on this parachain. let origin = self.origin_ref().ok_or(XcmError::BadOrigin)?.clone(); @@ -556,7 +569,6 @@ impl XcmExecutor { // weight consumed correctly (potentially allowing them to do more operations in a // block than they otherwise would). self.total_surplus.saturating_accrue(surplus); - RECURSION_COUNT.with(|count| count.set(count.get().saturating_sub(1))); Ok(()) }, QueryResponse { query_id, response, max_weight, querier } => { From d098dfd48da1f857aee04535932062a54f45f04d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Tue, 29 Nov 2022 16:48:45 +0100 Subject: [PATCH 06/11] Fix the implementation --- runtime/test-runtime/src/xcm_config.rs | 14 ++- xcm/xcm-executor/integration-tests/src/lib.rs | 31 +++--- xcm/xcm-executor/src/lib.rs | 97 ++++++++++--------- 3 files changed, 76 insertions(+), 66 deletions(-) diff --git a/runtime/test-runtime/src/xcm_config.rs b/runtime/test-runtime/src/xcm_config.rs index 8d0961335391..c8396f86b312 100644 --- a/runtime/test-runtime/src/xcm_config.rs +++ b/runtime/test-runtime/src/xcm_config.rs @@ -21,7 +21,8 @@ use frame_support::{ }; use xcm::latest::prelude::*; use xcm_builder::{ - AllowUnpaidExecutionFrom, EnsureXcmOrigin, FixedWeightBounds, SignedToAccountId32, + AllowUnpaidExecutionFrom, EnsureXcmOrigin, FixedWeightBounds, SignedAccountId32AsNative, + SignedToAccountId32, }; use xcm_executor::{ traits::{TransactAsset, WeightTrader}, @@ -30,7 +31,7 @@ use xcm_executor::{ parameter_types! { pub const BaseXcmWeight: xcm::latest::Weight = Weight::from_parts(1_000, 1_000); - pub const OurNetwork: NetworkId = NetworkId::Polkadot; + pub const ThisNetwork: NetworkId = NetworkId::Polkadot; pub const MaxInstructions: u32 = 100; pub const MaxAssetsIntoHolding: u32 = 16; pub const UniversalLocation: xcm::latest::InteriorMultiLocation = xcm::latest::Junctions::Here; @@ -40,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; @@ -83,12 +84,17 @@ 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; diff --git a/xcm/xcm-executor/integration-tests/src/lib.rs b/xcm/xcm-executor/integration-tests/src/lib.rs index c211cc3b96ef..a3212c798dab 100644 --- a/xcm/xcm-executor/integration-tests/src/lib.rs +++ b/xcm/xcm-executor/integration-tests/src/lib.rs @@ -17,7 +17,7 @@ #![cfg_attr(not(feature = "std"), no_std)] #![cfg(test)] -use frame_support::{codec::Encode, weights::Weight, dispatch::GetDispatchInfo}; +use frame_support::{codec::Encode, dispatch::GetDispatchInfo, weights::Weight}; use polkadot_test_client::{ BlockBuilderExt, ClientBlockImportExt, DefaultTestClientBuilderExt, ExecutionStrategy, InitPolkadotBlockBuilder, TestClientBuilder, TestClientBuilderExt, @@ -81,34 +81,31 @@ fn transact_recursion_limit_works() { 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 }, - ); + let mut call = polkadot_test_runtime::RuntimeCall::Xcm(pallet_xcm::Call::execute { + message: Box::new(VersionedXcm::from(msg)), + max_weight, + }); - for _ in 0..10 { + for _ in 0..11 { let mut msg = Xcm(vec![ - WithdrawAsset((Parent, 1_000_000_000).into()), - BuyExecution { fees: (Parent, 1_000_000_000).into(), weight_limit: Unlimited }, + WithdrawAsset((Parent, 1_000).into()), + BuyExecution { fees: (Parent, 1).into(), weight_limit: Unlimited }, Transact { - origin_kind: OriginKind::Xcm, + 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 } - ); + 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, - ); + let execute = construct_extrinsic(&client, call, sp_keyring::Sr25519Keyring::Alice, 0); block_builder.push_polkadot_extrinsic(execute).expect("pushes extrinsic"); diff --git a/xcm/xcm-executor/src/lib.rs b/xcm/xcm-executor/src/lib.rs index 9544377c9ea6..7176e5ab99ba 100644 --- a/xcm/xcm-executor/src/lib.rs +++ b/xcm/xcm-executor/src/lib.rs @@ -515,8 +515,16 @@ impl XcmExecutor { Ok(()) }, Transact { origin_kind, require_weight_at_most, mut call } => { - // Check whether we've exhausted the recursion limit - recursion_count::using(&mut 0, || { + fn with_recursion(fn_: impl FnOnce() -> R) -> R { + // We should make this better in upstream. + if recursion_count::with(|_| ()).is_none() { + recursion_count::using(&mut 1, fn_) + } else { + (fn_)() + } + } + + with_recursion(|| { recursion_count::with(|count| { if *count > RECURSION_LIMIT { return Err(XcmError::ExceedsStackLimit) @@ -524,52 +532,51 @@ impl XcmExecutor { *count = count.saturating_add(1); Ok(()) }) - .expect("`with` is being used within a `using` block; qed") - })?; + // This should always return `Some`, but let's play it safe. + .unwrap_or(Ok(()))?; - // Ensure that we always decrement the counter whenever we finish executing - // `Transact` - defer! { - recursion_count::using(&mut 0, || { + // Ensure that we always decrement the counter whenever we finish executing + // `Transact` + defer! { recursion_count::with(|count| { *count = count.saturating_sub(1); - }) - .expect("`with` is being used within a `using` block; qed"); - }); - } - - // We assume that the Relay-chain is allowed to use transact on this parachain. - let origin = self.origin_ref().ok_or(XcmError::BadOrigin)?.clone(); - - // TODO: #2841 #TRANSACTFILTER allow the trait to issue filters for the relay-chain - let message_call = call.take_decoded().map_err(|_| XcmError::FailedToDecode)?; - let dispatch_origin = Config::OriginConverter::convert_origin(origin, origin_kind) - .map_err(|_| XcmError::BadOrigin)?; - let weight = message_call.get_dispatch_info().weight; - ensure!(weight.all_lte(require_weight_at_most), XcmError::MaxWeightInvalid); - let maybe_actual_weight = - match Config::CallDispatcher::dispatch(message_call, dispatch_origin) { - Ok(post_info) => { - self.transact_status = MaybeErrorCode::Success; - post_info.actual_weight - }, - Err(error_and_info) => { - self.transact_status = error_and_info.error.encode().into(); - error_and_info.post_info.actual_weight - }, - }; - let actual_weight = maybe_actual_weight.unwrap_or(weight); - let surplus = weight.saturating_sub(actual_weight); - // We assume that the `Config::Weigher` will counts the `require_weight_at_most` - // for the estimate of how much weight this instruction will take. Now that we know - // that it's less, we credit it. - // - // We make the adjustment for the total surplus, which is used eventually - // reported back to the caller and this ensures that they account for the total - // weight consumed correctly (potentially allowing them to do more operations in a - // block than they otherwise would). - self.total_surplus.saturating_accrue(surplus); - Ok(()) + }); + } + + // We assume that the Relay-chain is allowed to use transact on this parachain. + let origin = self.origin_ref().ok_or(XcmError::BadOrigin)?.clone(); + + // TODO: #2841 #TRANSACTFILTER allow the trait to issue filters for the relay-chain + let message_call = call.take_decoded().map_err(|_| XcmError::FailedToDecode)?; + let dispatch_origin = + Config::OriginConverter::convert_origin(origin, origin_kind) + .map_err(|_| XcmError::BadOrigin)?; + let weight = message_call.get_dispatch_info().weight; + ensure!(weight.all_lte(require_weight_at_most), XcmError::MaxWeightInvalid); + let maybe_actual_weight = + match Config::CallDispatcher::dispatch(message_call, dispatch_origin) { + Ok(post_info) => { + self.transact_status = MaybeErrorCode::Success; + post_info.actual_weight + }, + Err(error_and_info) => { + self.transact_status = error_and_info.error.encode().into(); + error_and_info.post_info.actual_weight + }, + }; + let actual_weight = maybe_actual_weight.unwrap_or(weight); + let surplus = weight.saturating_sub(actual_weight); + // We assume that the `Config::Weigher` will counts the `require_weight_at_most` + // for the estimate of how much weight this instruction will take. Now that we know + // that it's less, we credit it. + // + // We make the adjustment for the total surplus, which is used eventually + // reported back to the caller and this ensures that they account for the total + // weight consumed correctly (potentially allowing them to do more operations in a + // block than they otherwise would). + self.total_surplus.saturating_accrue(surplus); + Ok(()) + }) }, QueryResponse { query_id, response, max_weight, querier } => { let origin = self.origin_ref().ok_or(XcmError::BadOrigin)?; From 52de9c2dfbe3c057575ab7366d96c9756a58dd42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Wed, 30 Nov 2022 16:01:01 +0100 Subject: [PATCH 07/11] Use nicer interface --- Cargo.lock | 4 ++-- xcm/xcm-executor/Cargo.toml | 2 +- xcm/xcm-executor/src/lib.rs | 13 +++---------- 3 files changed, 6 insertions(+), 13 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 117c872a5b44..c7ab0f9c70da 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" diff --git a/xcm/xcm-executor/Cargo.toml b/xcm/xcm-executor/Cargo.toml index 2fde306e6bd7..4deee8ca0876 100644 --- a/xcm/xcm-executor/Cargo.toml +++ b/xcm/xcm-executor/Cargo.toml @@ -7,7 +7,7 @@ version = "0.9.31" [dependencies] impl-trait-for-tuples = "0.2.2" -environmental = { version = "1.1.3", default-features = false } +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/src/lib.rs b/xcm/xcm-executor/src/lib.rs index 7176e5ab99ba..771e3ffa9f89 100644 --- a/xcm/xcm-executor/src/lib.rs +++ b/xcm/xcm-executor/src/lib.rs @@ -515,16 +515,9 @@ impl XcmExecutor { Ok(()) }, Transact { origin_kind, require_weight_at_most, mut call } => { - fn with_recursion(fn_: impl FnOnce() -> R) -> R { - // We should make this better in upstream. - if recursion_count::with(|_| ()).is_none() { - recursion_count::using(&mut 1, fn_) - } else { - (fn_)() - } - } - - with_recursion(|| { + // Initialize the recursion count only the first time we hit this code in our + // potential recursive execution. + recursion_count::using_once(&mut 1, || { recursion_count::with(|count| { if *count > RECURSION_LIMIT { return Err(XcmError::ExceedsStackLimit) From ee0b2ac5213ccdbc41e63a07d5bba7502a8800b4 Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Fri, 2 Dec 2022 03:24:03 +0900 Subject: [PATCH 08/11] Change ThisNetwork to AnyNetwork --- runtime/test-runtime/src/xcm_config.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/runtime/test-runtime/src/xcm_config.rs b/runtime/test-runtime/src/xcm_config.rs index c8396f86b312..2f850cf75976 100644 --- a/runtime/test-runtime/src/xcm_config.rs +++ b/runtime/test-runtime/src/xcm_config.rs @@ -31,7 +31,7 @@ use xcm_executor::{ parameter_types! { pub const BaseXcmWeight: xcm::latest::Weight = Weight::from_parts(1_000, 1_000); - pub const ThisNetwork: NetworkId = NetworkId::Polkadot; + 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; @@ -41,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; @@ -86,7 +86,7 @@ impl WeightTrader for DummyWeightTrader { type OriginConverter = ( pallet_xcm::XcmPassthrough, - SignedAccountId32AsNative, + SignedAccountId32AsNative, ); pub struct XcmConfig; From 989a1263d8d5b6601245a3b9c8de3024077d5b18 Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Fri, 2 Dec 2022 03:34:46 +0900 Subject: [PATCH 09/11] Move recursion check up to top level --- xcm/xcm-executor/src/lib.rs | 116 ++++++++++++++++++------------------ 1 file changed, 59 insertions(+), 57 deletions(-) diff --git a/xcm/xcm-executor/src/lib.rs b/xcm/xcm-executor/src/lib.rs index 771e3ffa9f89..9c25b475f371 100644 --- a/xcm/xcm-executor/src/lib.rs +++ b/xcm/xcm-executor/src/lib.rs @@ -307,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 executing + // `Transact` + 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) @@ -515,61 +539,39 @@ impl XcmExecutor { Ok(()) }, Transact { origin_kind, require_weight_at_most, mut call } => { - // Initialize the recursion count only the first time we hit this code in our - // potential recursive execution. - 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 executing - // `Transact` - defer! { - recursion_count::with(|count| { - *count = count.saturating_sub(1); - }); - } + // We assume that the Relay-chain is allowed to use transact on this parachain. + let origin = self.origin_ref().ok_or(XcmError::BadOrigin)?.clone(); - // We assume that the Relay-chain is allowed to use transact on this parachain. - let origin = self.origin_ref().ok_or(XcmError::BadOrigin)?.clone(); - - // TODO: #2841 #TRANSACTFILTER allow the trait to issue filters for the relay-chain - let message_call = call.take_decoded().map_err(|_| XcmError::FailedToDecode)?; - let dispatch_origin = - Config::OriginConverter::convert_origin(origin, origin_kind) - .map_err(|_| XcmError::BadOrigin)?; - let weight = message_call.get_dispatch_info().weight; - ensure!(weight.all_lte(require_weight_at_most), XcmError::MaxWeightInvalid); - let maybe_actual_weight = - match Config::CallDispatcher::dispatch(message_call, dispatch_origin) { - Ok(post_info) => { - self.transact_status = MaybeErrorCode::Success; - post_info.actual_weight - }, - Err(error_and_info) => { - self.transact_status = error_and_info.error.encode().into(); - error_and_info.post_info.actual_weight - }, - }; - let actual_weight = maybe_actual_weight.unwrap_or(weight); - let surplus = weight.saturating_sub(actual_weight); - // We assume that the `Config::Weigher` will counts the `require_weight_at_most` - // for the estimate of how much weight this instruction will take. Now that we know - // that it's less, we credit it. - // - // We make the adjustment for the total surplus, which is used eventually - // reported back to the caller and this ensures that they account for the total - // weight consumed correctly (potentially allowing them to do more operations in a - // block than they otherwise would). - self.total_surplus.saturating_accrue(surplus); - Ok(()) - }) + // TODO: #2841 #TRANSACTFILTER allow the trait to issue filters for the relay-chain + let message_call = call.take_decoded().map_err(|_| XcmError::FailedToDecode)?; + let dispatch_origin = + Config::OriginConverter::convert_origin(origin, origin_kind) + .map_err(|_| XcmError::BadOrigin)?; + let weight = message_call.get_dispatch_info().weight; + ensure!(weight.all_lte(require_weight_at_most), XcmError::MaxWeightInvalid); + let maybe_actual_weight = + match Config::CallDispatcher::dispatch(message_call, dispatch_origin) { + Ok(post_info) => { + self.transact_status = MaybeErrorCode::Success; + post_info.actual_weight + }, + Err(error_and_info) => { + self.transact_status = error_and_info.error.encode().into(); + error_and_info.post_info.actual_weight + }, + }; + let actual_weight = maybe_actual_weight.unwrap_or(weight); + let surplus = weight.saturating_sub(actual_weight); + // We assume that the `Config::Weigher` will counts the `require_weight_at_most` + // for the estimate of how much weight this instruction will take. Now that we know + // that it's less, we credit it. + // + // We make the adjustment for the total surplus, which is used eventually + // reported back to the caller and this ensures that they account for the total + // weight consumed correctly (potentially allowing them to do more operations in a + // block than they otherwise would). + self.total_surplus.saturating_accrue(surplus); + Ok(()) }, QueryResponse { query_id, response, max_weight, querier } => { let origin = self.origin_ref().ok_or(XcmError::BadOrigin)?; From 48973bad4d8c65926b09a53af51de97a3af4c332 Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Fri, 2 Dec 2022 03:36:00 +0900 Subject: [PATCH 10/11] cargo fmt --- rpc/src/lib.rs | 6 +----- xcm/xcm-executor/src/lib.rs | 9 ++++----- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/rpc/src/lib.rs b/rpc/src/lib.rs index 0f54840f2ca5..43efefcae15b 100644 --- a/rpc/src/lib.rs +++ b/rpc/src/lib.rs @@ -108,11 +108,7 @@ where + Sync + 'static, C::Api: frame_rpc_system::AccountNonceApi, - C::Api: mmr_rpc::MmrRuntimeApi< - Block, - ::Hash, - BlockNumber, - >, + C::Api: mmr_rpc::MmrRuntimeApi::Hash, BlockNumber>, C::Api: pallet_transaction_payment_rpc::TransactionPaymentRuntimeApi, C::Api: BabeApi, C::Api: BlockBuilder, diff --git a/xcm/xcm-executor/src/lib.rs b/xcm/xcm-executor/src/lib.rs index 9c25b475f371..158a26256193 100644 --- a/xcm/xcm-executor/src/lib.rs +++ b/xcm/xcm-executor/src/lib.rs @@ -320,7 +320,7 @@ impl XcmExecutor { }) // This should always return `Some`, but let's play it safe. .unwrap_or(Ok(()))?; - + // Ensure that we always decrement the counter whenever we finish executing // `Transact` defer! { @@ -339,7 +339,7 @@ impl XcmExecutor { weight: Weight::zero(), }); } - } + }, Err(ref mut error) => if let Ok(x) = Config::Weigher::instr_weight(&instr) { error.weight.saturating_accrue(x) @@ -544,9 +544,8 @@ impl XcmExecutor { // TODO: #2841 #TRANSACTFILTER allow the trait to issue filters for the relay-chain let message_call = call.take_decoded().map_err(|_| XcmError::FailedToDecode)?; - let dispatch_origin = - Config::OriginConverter::convert_origin(origin, origin_kind) - .map_err(|_| XcmError::BadOrigin)?; + let dispatch_origin = Config::OriginConverter::convert_origin(origin, origin_kind) + .map_err(|_| XcmError::BadOrigin)?; let weight = message_call.get_dispatch_info().weight; ensure!(weight.all_lte(require_weight_at_most), XcmError::MaxWeightInvalid); let maybe_actual_weight = From 8e1718b7c65a1eeadacffa77cea53384cfe9c4a9 Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Fri, 2 Dec 2022 04:00:41 +0900 Subject: [PATCH 11/11] Update comment --- xcm/xcm-executor/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/xcm/xcm-executor/src/lib.rs b/xcm/xcm-executor/src/lib.rs index 158a26256193..5906b2eef9c5 100644 --- a/xcm/xcm-executor/src/lib.rs +++ b/xcm/xcm-executor/src/lib.rs @@ -321,8 +321,8 @@ impl XcmExecutor { // This should always return `Some`, but let's play it safe. .unwrap_or(Ok(()))?; - // Ensure that we always decrement the counter whenever we finish executing - // `Transact` + // Ensure that we always decrement the counter whenever we finish processing + // the instruction. defer! { recursion_count::with(|count| { *count = count.saturating_sub(1);