Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Create thread_local in XCM executor to limit recursion depth #6304

Merged
merged 14 commits into from
Dec 1, 2022
Merged
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

37 changes: 1 addition & 36 deletions runtime/test-runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 = ();
Expand Down Expand Up @@ -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;
Expand Down
32 changes: 30 additions & 2 deletions runtime/test-runtime/src/xcm_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -90,7 +93,7 @@ impl xcm_executor::Config for XcmConfig {
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;
Expand All @@ -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<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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this really be a const rather than a const-type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hope is that we'd eventually be able to use associated consts in the bounds, but the last time we tried, we ran into an unsupported Rust feature: paritytech/substrate#9865

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;
}
1 change: 1 addition & 0 deletions xcm/xcm-executor/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
61 changes: 59 additions & 2 deletions xcm/xcm-executor/integration-tests/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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]);
KiChjang marked this conversation as resolved.
Show resolved Hide resolved
KiChjang marked this conversation as resolved.
Show resolved Hide resolved
let mut 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..10 {
msg = Xcm(vec![
KiChjang marked this conversation as resolved.
Show resolved Hide resolved
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 = <XcmConfig as xcm_executor::Config>::Weigher::weight(&mut msg).unwrap();
KiChjang marked this conversation as resolved.
Show resolved Hide resolved
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(
KiChjang marked this conversation as resolved.
Show resolved Hide resolved
Outcome::Incomplete(_, XcmError::ExceedsStackLimit)
)),
)));
});
}

#[test]
fn query_response_fires() {
use pallet_test_notifier::Event::*;
Expand Down
5 changes: 3 additions & 2 deletions xcm/xcm-executor/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ pub struct FeesMode {

const RECURSION_LIMIT: u8 = 10;

thread_local! {
static RECURSION_COUNT: Cell<u8> = Cell::new(0);
environmental::thread_local_impl! {
KiChjang marked this conversation as resolved.
Show resolved Hide resolved
static RECURSION_COUNT: Cell<u8> = Cell::new(0)
KiChjang marked this conversation as resolved.
Show resolved Hide resolved
}

/// The XCM executor.
Expand Down Expand Up @@ -556,6 +556,7 @@ impl<Config: config::Config> XcmExecutor<Config> {
// 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)));
KiChjang marked this conversation as resolved.
Show resolved Hide resolved
Ok(())
},
QueryResponse { query_id, response, max_weight, querier } => {
Expand Down