Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

{suspend, resume} xcm execution when {enter, exit}ing maintenance-mode #1359

Merged
merged 15 commits into from
Mar 25, 2022

Conversation

4meta5
Copy link
Contributor

@4meta5 4meta5 commented Mar 22, 2022

MOON-1483

Suspends xcm execution of messages in xcmp_queue::on_idle when entering maintenance-mode

Resumes xcm execution of messages in xcmp_queue::on_idle when exiting maintenance-mode

Removes the XcmpHandler associated types from pallet maintenance mode

Relevant:

@4meta5 4meta5 added A0-pleasereview Pull request needs code review. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D3-trivial PR contains trivial changes in a runtime directory that do not require an audit labels Mar 22, 2022
@4meta5 4meta5 requested a review from girazoki March 23, 2022 16:21
@@ -88,6 +103,9 @@ pub mod pallet {
/// able to return to normal mode. For example, if your MaintenanceOrigin is a council, make
/// sure that your councilors can still cast votes.
type MaintenanceOrigin: EnsureOrigin<Self::Origin>;
/// Handler to suspend and resume XCM execution
#[cfg(feature = "xcm-support")]
type XcmExecutionManager: PauseXcmExecution;
/// The DMP handler to be used in normal operating mode
#[cfg(feature = "xcm-support")]
type NormalDmpHandler: DmpMessageHandler;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need the Handlers at all now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm I think we do, the new code just stops executing XCM in xcmp_queue::on_idle. I don't think it changes how messages are otherwise handled...

@girazoki
Copy link
Collaborator

Let's remove the dmp & xcmp handlers from the config and verify all tests pass. I think thats the ultimate check that we need to ensure this behaves as we expect

@4meta5
Copy link
Contributor Author

4meta5 commented Mar 23, 2022

Let's remove the dmp & xcmp handlers from the config and verify all tests pass. I think thats the ultimate check that we need to ensure this behaves as we expect

Sounds good, I'll try it. I don't think they do the same thing though...

@4meta5
Copy link
Contributor Author

4meta5 commented Mar 23, 2022

@girazoki the CI says the TS test fails without the handlers:

  1) Pallet Maintenance Mode - dmp messages should queue in maintenance mode
       should queue xcm with maintenance mode and execute when off:

      AssertionError: expected false to equal true
      + expected - actual

      -false
      +true
      
      at Context.<anonymous> (tests/test-pallet-maintenance-mode.ts:509:38)
      at runMicrotasks (<anonymous>)
      at processTicksAndRejections (internal/process/task_queues.js:95:5)

So I will revert removing the handlers

@girazoki
Copy link
Collaborator

Oh I see whats going on here. I think initially the cumulus PR was set to pause the XCMP and DMP queues, but it finally made in with only the XCMP queue. So what we can remove is the XCMP handler, not the DMP one. I think the xcmp handler test works fine

@girazoki
Copy link
Collaborator

girazoki commented Mar 24, 2022

More generally, I think we might be able to do this for both queues with https://github.com/paritytech/polkadot/pull/5035/files But for now I think we can remove the XCMP queue. Can you mention this PR as a TODO in the DMP handler so that we dont forget to remove it in the future?

@4meta5 4meta5 merged commit c582e2f into master Mar 25, 2022
@4meta5 4meta5 deleted the amar-use-cumulus-xcm-pause branch March 25, 2022 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0-pleasereview Pull request needs code review. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D3-trivial PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants