Skip to content
This repository has been 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

Conversation

KiChjang
Copy link
Contributor

Fixes #891.

Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

looks super straight forward

@shawntabrizi
Copy link
Member

probably should be "needs audit"

@zqhxuyuan
Copy link
Contributor

maybe dmp-queue also need this feature? would that another PR or just here?

pallets/xcmp-queue/src/lib.rs Outdated Show resolved Hide resolved
@KiChjang KiChjang changed the title Add the ability to suspend or resume XCM execution on the XCMP queue Add the ability to suspend or resume XCM execution on the XCMP and DMP queues Jan 13, 2022
Copy link
Contributor

@apopiak apopiak left a comment

Choose a reason for hiding this comment

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

copy-pasta fixes

pallets/dmp-queue/src/lib.rs Outdated Show resolved Hide resolved
pallets/dmp-queue/src/lib.rs Outdated Show resolved Hide resolved
pallets/dmp-queue/src/lib.rs Outdated Show resolved Hide resolved
Co-authored-by: Alexander Popiak <alexander.popiak@parity.io>
@@ -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

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

@shawntabrizi shawntabrizi self-requested a review January 15, 2022 01:41
@KiChjang KiChjang changed the title Add the ability to suspend or resume XCM execution on the XCMP and DMP queues Add the ability to suspend or resume XCM execution on the XCMP queues Jan 29, 2022
@KiChjang KiChjang changed the title Add the ability to suspend or resume XCM execution on the XCMP queues Add the ability to suspend or resume XCM execution on the XCMP queue Jan 29, 2022
@gavofyork gavofyork merged commit 7749187 into master Jan 31, 2022
@gavofyork gavofyork deleted the kckyeung/suspend-resume-xcm-exec branch January 31, 2022 12:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suspend xcmp/dmp queue
7 participants