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

Add the ability to suspend or resume XCM execution on the XCMP queue #896

Merged
merged 9 commits into from
Jan 31, 2022
Merged

Add the ability to suspend or resume XCM execution on the XCMP queue #896

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5392f09
Add the ability to suspend or resume XCM execution on the XCMP queue
KiChjang Jan 12, 2022
1839ffc
Rename QueueActive to QueueSuspended
KiChjang Jan 13, 2022
363ca09
Add the ability to suspend the DMP queue
KiChjang Jan 13, 2022
f4027e5
Rename XCMP to DMP in comments where appropriate
KiChjang Jan 13, 2022
b230550
Merge remote-tracking branch 'origin/master' into kckyeung/suspend-re…
KiChjang Jan 22, 2022
0ad9324
Add a bypass for XCMP queue suspension
KiChjang Jan 22, 2022
08b1a3f
Revert "Add the ability to suspend the DMP queue"
KiChjang Jan 24, 2022
138169e
Change controller origin to either root or council-issued origin
KiChjang Jan 24, 2022
b60139f
Rename to ControllerOriginConverter
KiChjang Jan 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions pallets/dmp-queue/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,9 @@ pub mod pallet {

/// Origin which is allowed to execute overweight messages.
type ExecuteOverweightOrigin: EnsureOrigin<Self::Origin>;

/// The origin that is allowed to resume or suspend the XCMP queue.
type ControllerOrigin: EnsureOrigin<Self::Origin>;
}

/// The configuration.
Expand All @@ -109,6 +112,10 @@ pub mod pallet {
pub(super) type Overweight<T> =
StorageMap<_, Blake2_128Concat, OverweightIndex, (RelayBlockNumber, Vec<u8>), OptionQuery>;

/// Whether or not the XCMP queue is suspended from executing incoming XCMs or not.
#[pallet::storage]
pub(super) type QueueSuspended<T: Config> = StorageValue<_, bool, ValueQuery>;

#[pallet::error]
pub enum Error<T> {
/// The message index given is unknown.
Expand Down Expand Up @@ -154,6 +161,32 @@ pub mod pallet {
Self::deposit_event(Event::OverweightServiced(index, used));
Ok(Some(used.saturating_add(1_000_000)).into())
}

/// Suspends all XCM executions for the XCMP queue, regardless of the sender's origin.
///
/// - `origin`: Must pass `ControllerOrigin`.
#[pallet::weight(T::DbWeight::get().writes(1))]
pub fn suspend_xcm_execution(origin: OriginFor<T>) -> DispatchResult {
T::ControllerOrigin::ensure_origin(origin)?;

QueueSuspended::<T>::put(true);

Ok(())
}

/// Resumes all XCM executions for the XCMP queue.
///
/// Note that this function doesn't change the status of the in/out bound channels.
///
/// - `origin`: Must pass `ControllerOrigin`.
#[pallet::weight(T::DbWeight::get().writes(1))]
pub fn resume_xcm_execution(origin: OriginFor<T>) -> DispatchResult {
T::ControllerOrigin::ensure_origin(origin)?;

QueueSuspended::<T>::put(false);

Ok(())
}
}

#[pallet::event]
Expand Down Expand Up @@ -184,6 +217,11 @@ pub mod pallet {
///
/// Returns the weight consumed by executing messages in the queue.
fn service_queue(limit: Weight) -> Weight {
let suspended = QueueSuspended::<T>::get();
if suspended {
return 0
}

PageIndex::<T>::mutate(|page_index| Self::do_service_queue(limit, page_index))
}

Expand Down Expand Up @@ -263,6 +301,11 @@ pub mod pallet {
iter: impl Iterator<Item = (RelayBlockNumber, Vec<u8>)>,
limit: Weight,
) -> Weight {
let suspended = QueueSuspended::<T>::get();
if suspended {
return 0
}

let mut page_index = PageIndex::<T>::get();
let config = Configuration::<T>::get();

Expand Down Expand Up @@ -447,6 +490,7 @@ mod tests {
type Event = Event;
type XcmExecutor = MockExec;
type ExecuteOverweightOrigin = frame_system::EnsureRoot<AccountId>;
type ControllerOrigin = frame_system::EnsureRoot<AccountId>;
}

pub(crate) fn new_test_ext() -> sp_io::TestExternalities {
Expand Down Expand Up @@ -786,4 +830,20 @@ mod tests {
assert_eq!(pages_queued(), 1);
});
}

#[test]
fn suspend_xcm_execution_works() {
new_test_ext().execute_with(|| {
QueueSuspended::<Test>::put(true);

let incoming = [msg(1000), msg(1001)];
enqueue(&incoming[..]);

let weight_used = handle_messages(&incoming, 5000);

assert_eq!(weight_used, 0);
assert_eq!(take_trace(), Vec::new());
assert_eq!(pages_queued(), 1);
});
}
}
38 changes: 38 additions & 0 deletions pallets/xcmp-queue/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,9 @@ pub mod pallet {

/// The origin that is allowed to execute overweight messages.
type ExecuteOverweightOrigin: EnsureOrigin<Self::Origin>;

/// The origin that is allowed to resume or suspend the XCMP queue.
type ControllerOrigin: EnsureOrigin<Self::Origin>;
}

#[pallet::hooks]
Expand Down Expand Up @@ -129,6 +132,32 @@ pub mod pallet {
Self::deposit_event(Event::OverweightServiced(index, used));
Ok(Some(used.saturating_add(1_000_000)).into())
}

/// Suspends all XCM executions for the XCMP queue, regardless of the sender's origin.
///
/// - `origin`: Must pass `ControllerOrigin`.
#[pallet::weight(T::DbWeight::get().writes(1))]
pub fn suspend_xcm_execution(origin: OriginFor<T>) -> DispatchResult {
T::ControllerOrigin::ensure_origin(origin)?;

QueueSuspended::<T>::put(true);

Ok(())
}

/// Resumes all XCM executions for the XCMP queue.
///
/// Note that this function doesn't change the status of the in/out bound channels.
///
/// - `origin`: Must pass `ControllerOrigin`.
#[pallet::weight(T::DbWeight::get().writes(1))]
pub fn resume_xcm_execution(origin: OriginFor<T>) -> DispatchResult {
T::ControllerOrigin::ensure_origin(origin)?;

QueueSuspended::<T>::put(false);

Ok(())
}
}

#[pallet::event]
Expand Down Expand Up @@ -220,6 +249,10 @@ pub mod pallet {
/// available free overweight index.
#[pallet::storage]
pub(super) type OverweightCount<T: Config> = StorageValue<_, OverweightIndex, ValueQuery>;

/// Whether or not the XCMP queue is suspended from executing incoming XCMs or not.
#[pallet::storage]
pub(super) type QueueSuspended<T: Config> = StorageValue<_, bool, ValueQuery>;
}

#[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Encode, Decode, RuntimeDebug, TypeInfo)]
Expand Down Expand Up @@ -619,6 +652,11 @@ impl<T: Config> Pallet<T> {
/// for the second &c. though empirical and or practical factors may give rise to adjusting it
/// further.
fn service_xcmp_queue(max_weight: Weight) -> Weight {
let suspended = QueueSuspended::<T>::get();
if suspended {
return 0
}

let mut status = <InboundXcmpStatus<T>>::get(); // <- sorted.
if status.len() == 0 {
return 0
Expand Down
1 change: 1 addition & 0 deletions pallets/xcmp-queue/src/mock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ impl Config for Test {
type ChannelInfo = ParachainSystem;
type VersionWrapper = ();
type ExecuteOverweightOrigin = EnsureRoot<AccountId>;
type ControllerOrigin = EnsureRoot<AccountId>;
}

pub fn new_test_ext() -> sp_io::TestExternalities {
Expand Down
18 changes: 18 additions & 0 deletions pallets/xcmp-queue/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,3 +82,21 @@ fn service_overweight_bad_xcm_format() {
assert_noop!(XcmpQueue::service_overweight(Origin::root(), 0, 1000), Error::<Test>::BadXcm);
});
}

#[test]
fn suspend_xcm_execution_works() {
new_test_ext().execute_with(|| {
QueueSuspended::<Test>::put(true);

let xcm = Instruction::<()>::ClearOrigin.encode();
let mut message_format = XcmpMessageFormat::ConcatenatedVersionedXcm.encode();
message_format.extend(xcm.clone());
let messages = vec![(Default::default(), 1u32.into(), message_format.as_slice())];

// This shouldn't have executed the incoming XCM
XcmpQueue::handle_xcmp_messages(messages.into_iter(), Weight::max_value());

let queued_xcm = InboundXcmpMessages::<Test>::get(ParaId::default(), 1u32);
assert_eq!(queued_xcm, xcm);
});
}
2 changes: 2 additions & 0 deletions parachain-template/runtime/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,12 +525,14 @@ impl cumulus_pallet_xcmp_queue::Config for Runtime {
type ChannelInfo = ParachainSystem;
type VersionWrapper = ();
type ExecuteOverweightOrigin = EnsureRoot<AccountId>;
type ControllerOrigin = EnsureRoot<AccountId>;
}

impl cumulus_pallet_dmp_queue::Config for Runtime {
type Event = Event;
type XcmExecutor = XcmExecutor<XcmConfig>;
type ExecuteOverweightOrigin = EnsureRoot<AccountId>;
type ControllerOrigin = EnsureRoot<AccountId>;
}

parameter_types! {
Expand Down
4 changes: 3 additions & 1 deletion polkadot-parachains/rococo-parachain/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,12 +454,14 @@ impl cumulus_pallet_xcmp_queue::Config for Runtime {
type ChannelInfo = ParachainSystem;
type VersionWrapper = ();
type ExecuteOverweightOrigin = EnsureRoot<AccountId>;
type ControllerOrigin = EnsureRoot<AccountId>;
}

impl cumulus_pallet_dmp_queue::Config for Runtime {
type Event = Event;
type XcmExecutor = XcmExecutor<XcmConfig>;
type ExecuteOverweightOrigin = frame_system::EnsureRoot<AccountId>;
type ExecuteOverweightOrigin = EnsureRoot<AccountId>;
type ControllerOrigin = EnsureRoot<AccountId>;
}

impl cumulus_ping::Config for Runtime {
Expand Down
2 changes: 2 additions & 0 deletions polkadot-parachains/statemine/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -604,12 +604,14 @@ impl cumulus_pallet_xcmp_queue::Config for Runtime {
type ChannelInfo = ParachainSystem;
type VersionWrapper = PolkadotXcm;
type ExecuteOverweightOrigin = EnsureRoot<AccountId>;
type ControllerOrigin = EnsureRoot<AccountId>;
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem with this is that Root is actually on another chain (Kusama). So in order toresume_xcm_execution, it would need to ... process an XCM.

So I think service_{xcmp,dmp}_queue needs a bypass, where messages with ControllerOrigin can still be executed.

I also think this should be EnsureOneOf<EnsureRoot<AccountId>, EnsureXcm<IsMajorityOfBody<KsmLocation, ExecutiveBody>>> but open to discussion on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

For chains without their own governance body (Statemine/Statemint), this feature is actually a bit dangerous because if XCM is disabled, it will not be able to process the XCM to resume it...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh wow ok, if that's the case then we have a bit of a problem in our hands, because I briefly considered whether it made sense to just suspend ALL inbound channels by looping through them and modifying their state, instead of suspending the entire queue in the way that the PR currently handles it.

With such a requirement, it does look to me that the best way to handle it is to suspend all inbound channels, which could be quite computationally heavy if there are a lot of them. The other thing is, when we try to resume all inbound channels afterwards, we would also accidentally resume a channel that was suspended for other reasons, but I think this can be solved by creating more channel states.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I figured that this is not as important on the XCMP queue, since for our use case, it would be the relay chain that's sending us an XCM, and it would use the DMP queue. The problem here is that we don't really get to know whether the XCM came from an executive body; all we know is that it came from the relay chain. This would mean that without the ability to discern the origin more precisely, we should never suspend the DMP queue as it may contain important root calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A corollary for not being able to know whether or not the XCM came from an executive body is that adding an EnsureXcm<IsMajorityOfBody<...>> isn't that useful, as the origin is always set to the sibling parachain. Currently the bypass works by having the OriginConverter convert all incoming XCMs originating from a specified parachain to be the Root origin.

Note that the root conversion is only used to determine whether or not we bypass the suspended queue; the Transact instruction is unaffected. Perhaps I should rename OriginConverter to ControllerConverter to make the intention clearer.

Copy link
Member

@gavofyork gavofyork Jan 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

While I reckon that this is not a bad feature to have, we could also just do all of this with a blocking barrier: paritytech/polkadot#4813

}

impl cumulus_pallet_dmp_queue::Config for Runtime {
type Event = Event;
type XcmExecutor = XcmExecutor<XcmConfig>;
type ExecuteOverweightOrigin = EnsureRoot<AccountId>;
type ControllerOrigin = EnsureRoot<AccountId>;
}

parameter_types! {
Expand Down
2 changes: 2 additions & 0 deletions polkadot-parachains/statemint/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -616,12 +616,14 @@ impl cumulus_pallet_xcmp_queue::Config for Runtime {
type ChannelInfo = ParachainSystem;
type VersionWrapper = PolkadotXcm;
type ExecuteOverweightOrigin = EnsureRoot<AccountId>;
type ControllerOrigin = EnsureRoot<AccountId>;
}

impl cumulus_pallet_dmp_queue::Config for Runtime {
type Event = Event;
type XcmExecutor = XcmExecutor<XcmConfig>;
type ExecuteOverweightOrigin = EnsureRoot<AccountId>;
type ControllerOrigin = EnsureRoot<AccountId>;
}

parameter_types! {
Expand Down
2 changes: 2 additions & 0 deletions polkadot-parachains/westmint/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,12 +596,14 @@ impl cumulus_pallet_xcmp_queue::Config for Runtime {
type ChannelInfo = ParachainSystem;
type VersionWrapper = PolkadotXcm;
type ExecuteOverweightOrigin = EnsureRoot<AccountId>;
type ControllerOrigin = EnsureRoot<AccountId>;
}

impl cumulus_pallet_dmp_queue::Config for Runtime {
type Event = Event;
type XcmExecutor = XcmExecutor<XcmConfig>;
type ExecuteOverweightOrigin = EnsureRoot<AccountId>;
type ControllerOrigin = EnsureRoot<AccountId>;
}

parameter_types! {
Expand Down