-
Notifications
You must be signed in to change notification settings - Fork 378
Add the ability to suspend or resume XCM execution on the XCMP queue #896
Conversation
There was a problem hiding this 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
probably should be "needs audit" |
maybe dmp-queue also need this feature? would that another PR or just here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copy-pasta fixes
Co-authored-by: Alexander Popiak <[email protected]>
@@ -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>; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
This reverts commit 363ca09.
Fixes #891.