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
Prev Previous commit
Next Next commit
Fix the implementation
  • Loading branch information
bkchr committed Nov 29, 2022
commit d098dfd48da1f857aee04535932062a54f45f04d
14 changes: 10 additions & 4 deletions runtime/test-runtime/src/xcm_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand All @@ -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;
Expand All @@ -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<crate::RuntimeOrigin, crate::AccountId, OurNetwork>,
SignedToAccountId32<crate::RuntimeOrigin, crate::AccountId, ThisNetwork>,
);

pub struct DoNothingRouter;
Expand Down Expand Up @@ -83,12 +84,17 @@ impl WeightTrader for DummyWeightTrader {
}
}

type OriginConverter = (
pallet_xcm::XcmPassthrough<super::RuntimeOrigin>,
SignedAccountId32AsNative<ThisNetwork, 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;
Expand Down
31 changes: 14 additions & 17 deletions xcm/xcm-executor/integration-tests/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -81,34 +81,31 @@ fn transact_recursion_limit_works() {

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 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 },
);
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 = <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 }
);
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");

Expand Down
97 changes: 52 additions & 45 deletions xcm/xcm-executor/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -515,61 +515,68 @@ impl<Config: config::Config> XcmExecutor<Config> {
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<R>(fn_: impl FnOnce() -> R) -> R {
// We should make this better in upstream.
bkchr marked this conversation as resolved.
Show resolved Hide resolved
if recursion_count::with(|_| ()).is_none() {
recursion_count::using(&mut 1, fn_)
} else {
(fn_)()
}
}
bkchr marked this conversation as resolved.
Show resolved Hide resolved

with_recursion(|| {
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")
})?;
// 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)?;
Expand Down