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

Revert #2958#3026

Closed
gilescope wants to merge 1 commit intomasterfrom
giles-revert-2958
Closed

Revert #2958#3026
gilescope wants to merge 1 commit intomasterfrom
giles-revert-2958

Conversation

@gilescope
Copy link
Copy Markdown
Contributor

@gilescope gilescope commented Aug 17, 2023 •
edited
Loading

#2958 is causing some trouble with a few other PRs. There's some suggestion that we should revert it. This PR does this. We will re-add it as a series of PRs that should be less disruptive to other devs.

@gilescope gilescope added A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Aug 17, 2023
@bkontur
Copy link
Copy Markdown
Contributor

bkontur commented Aug 17, 2023 •
edited
Loading

causing some trouble with a few other PRs

Just out of curiosity, what troubles? Could you please link those PRs here?

@gilescope gilescope changed the title Potential: Revert #2958 Revert #2958 Aug 17, 2023
@gilescope
Copy link
Copy Markdown
Contributor Author

gilescope commented Aug 17, 2023

Sure, #3023 (holding up async backing being merged) and #2157 - unifying the xcm message queues. Both are things we want to land before monorepo.

@bkontur
Copy link
Copy Markdown
Contributor

bkontur commented Aug 17, 2023

#2157 ran one week ago with no failure: https://gitlab.parity.io/parity/mirrors/cumulus/-/jobs/3355109
#2157 should be rebased on actual master and we will see if there is any failure,
or am I missing something here?

#3023 ok, has some errors: https://gitlab.parity.io/parity/mirrors/cumulus/-/jobs/3403396, but why revert instead of fixing for async backing?

@gilescope
Copy link
Copy Markdown
Contributor Author

gilescope commented Aug 17, 2023

Rebasing/merge master of #2157 does lead to a lot of conflicts (understandably).

@gilescope
Copy link
Copy Markdown
Contributor Author

gilescope commented Aug 17, 2023

But I think that you're right - #3023 is going to still be a problem even if we revert this changeset.

@rphmeier
Copy link
Copy Markdown
Contributor

rphmeier commented Aug 17, 2023

instead of fixing for async backing?

Please do it if possible. I left a comment in #3023 about why this test harness is flaky & unrealistic, so it needs refactoring regardless. Unfortunately, I had to fix some of that unrealistic behavior in order to ensure the invariants of the ParachainSystem pallet in async-backing were upheld, and this caused downstream test failures.

@rphmeier rphmeier mentioned this pull request Aug 17, 2023
Open
@gilescope
Copy link
Copy Markdown
Contributor Author

gilescope commented Aug 17, 2023

Given that people have found workarounds for now we will keep this in, and improve on what's there in upcoming PRs.

@gilescope gilescope closed this Aug 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gilescope @bkontur @rphmeier