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

configuration: drop pvf_checking_enabled parameter #7396

Merged
merged 4 commits into from
Jul 11, 2023
Merged

configuration: drop pvf_checking_enabled parameter #7396

merged 4 commits into from
Jul 11, 2023

Conversation

slumber
Copy link
Contributor

@slumber slumber commented Jun 19, 2023

Follow-up for #7138, simply removes unused value from the config.

Part of #3211

@slumber slumber added A0-please_review Pull request needs code review. 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. E0-runtime_migration PR introduces code that might require downstream chains to run a runtime upgrade. labels Jun 19, 2023
@slumber slumber requested a review from eskimor June 19, 2023 13:40
@paritytech-ci paritytech-ci requested review from a team June 19, 2023 13:40
@slumber slumber mentioned this pull request Jun 20, 2023
Closed
24 tasks
@eskimor
Copy link
Member

eskimor commented Jul 4, 2023 •
edited
Loading

Another config migration - let's merge this with the other ones.

@slumber slumber requested review from bkchr and rphmeier July 10, 2023 11:10
@slumber
Copy link
Contributor Author

slumber commented Jul 10, 2023

bot merge

@paritytech-processbot
Copy link

Error: Statuses failed for fb2c83f

@slumber
Copy link
Contributor Author

slumber commented Jul 11, 2023

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot paritytech-processbot bot merged commit aa5c822 into master Jul 11, 2023
@paritytech-processbot paritytech-processbot bot deleted the slumber-drop-prechecking-switch-from-cfg branch July 11, 2023 12:02
@slumber
Copy link
Contributor Author

slumber commented Jul 11, 2023 •
edited
Loading

CI for migrations actually fails with "corrupted state" error at active config key. However, it shows host config without pvf_checking_enabled value before the migration is applied, as if it was done several times. Looks confusing.

UPD: the reason is empty list of pending_configs. Will file a follow up to silence a warning.

@slumber slumber mentioned this pull request Jul 11, 2023
Merged
@eskimor
Copy link
Member

eskimor commented Jul 11, 2023

Another config migration - let's merge this with the other ones.

Hmm, this comment apparently did not make it through. @antonva @tdimitrov the other migrations now have to be adjusted again (bumped one version). Also @tdimitrov doesn't the storage migration issue we had (which was blocking the other PR) not also affect this one? @slumber

@slumber
Copy link
Contributor Author

slumber commented Jul 11, 2023

Another config migration - let's merge this with the other ones.

Hmm, this comment apparently did not make it through. @antonva @tdimitrov the other migrations now have to be adjusted again (bumped one version). Also @tdimitrov doesn't the storage migration issue we had (which was blocking the other PR) not also affect this one? @slumber

My bad, got sidetracked several times and was busy debugging the CI issue. The summary is here #7489 (comment)

@tdimitrov
Copy link
Contributor

tdimitrov commented Jul 12, 2023 •
edited
Loading

Also @tdimitrov doesn't the storage migration issue we had (which was blocking the other PR) not also affect this one? @slumber

As far as I understand from the conversation in #7489 - it should be safe now because the UMP migration is already released (but not yet applied) for Polkadot.

But generally speaking you are right - UMP migration will fail if it is executed together with host config v7 AND before it.

EDIT: by fail I meant the try-runtime checks. I'm not sure if the actual migration will or will not fail as it just sets values we don't touch.

@tdimitrov
Copy link
Contributor

The comment above is not correct. The UMP migration doesn't fail, the HostConfig V7 one fails. #7265 was supposed to introduce HostConfig V7 so I hadn't got the same problem back then.

@crystalin crystalin mentioned this pull request Oct 20, 2023
Closed
@JesseAbram JesseAbram mentioned this pull request Oct 26, 2023
Merged
@s0me0ne-unkn0wn s0me0ne-unkn0wn mentioned this pull request Jan 26, 2024
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@s0me0ne-unkn0wn s0me0ne-unkn0wn s0me0ne-unkn0wn approved these changes

@rphmeier rphmeier rphmeier approved these changes

@ordian ordian ordian approved these changes

@antonva antonva antonva approved these changes

@eskimor eskimor Awaiting requested review from eskimor

@bkchr bkchr Awaiting requested review from bkchr

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. 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. E0-runtime_migration PR introduces code that might require downstream chains to run a runtime upgrade.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@slumber @eskimor @tdimitrov @antonva @ordian @rphmeier @s0me0ne-unkn0wn