configuration: optionally read pending configs in migration

[slumber]

Follows up #7396

CI was failing because of warning caused in migration. The warning is coming from defensive_proof call on optional value of PendingConfigs, which is actually allowed to be None. My bad for blindly copying code from #6271.

[slumber]

Despite the fact that the warning is gone, there's still Corrupted state ....

[slumber]

Reasoning behind failing CI:

https://github.com/paritytech/polkadot/blob/master/runtime/parachains/src/configuration/migration_ump.rs
This migration schedules config upgrade for a set of values, without altering the struct.

Note that this migration does it based on current code of host config, the one we're supposed to have after applying new (v7) migration.

It adds another item to PendingConfigs, of version 7, when trying to upgrade it from v6 to v7 we fail with "Corrupted state".

Why is this harmless?

We can't skip this migration based on pallet storage version, it's supposed to be applied for v6, the same way my config migration is supposed to be applied for v6.
UMP migration is already part of release, and it's not guarded against being re-executed. However, since we fail to read pending config upgrades, ump migration will be discarded with execution of v7 (we failed to read it).
Assuming it was previously applied, this is ok.

Solution?

Linked migration was supposed to bump storage version (unfortunately it didn't). Then we could guard against re-executing it, thus mitigating read-error. However, altering migration that was already applied sounds bad to me, even though it could work in theory.

So unless V0943 migrations get dropped, only option is to ignore this warning.

[ordian]

Assuming it was previously applied, this is ok.

UMP migration was not applied on Polkadot yet:

polkadot/runtime/polkadot/src/lib.rs

Lines 1560 to 1563 in aa5c822

	pub const MAX_UPWARD_QUEUE_SIZE: u32 = 1 * 1024 * 1024;
	pub const MAX_UPWARD_QUEUE_COUNT: u32 = 174762;
	pub const MAX_UPWARD_MESSAGE_SIZE: u32 = (1 << 16) - 5; // Checked in test `max_upward_message_size`.
	pub const MAX_UPWARD_MESSAGE_NUM_PER_CANDIDATE: u32 = 16;

But it probably will be by the time v7 hits the runtime.

[ggwpez]

Polkadot should upgrade to .43 in 16 days or so?
So you can probably already remove the .43 migrations (and older) to fix this. I have to admit that this UmpLimits "migration" was very hacky, but the limits needed to be bumped synchronously with the runtime upgrade for security reasons...

[ordian]

Why are runtime migration checks are failing btw? Is that expected? I don't see them failing on other PRs.

[slumber]

Polkadot should upgrade to .43 in 16 days or so? So you can probably already remove the .43 migrations (and older) to fix this. I have to admit that this UmpLimits "migration" was very hacky, but the limits needed to be bumped synchronously with the runtime upgrade for security reasons...

We can do this in a follow-up to address the warning.

[slumber]

Why are runtime migration checks are failing btw? Is that expected? I don't see them failing on other PRs.

#7489 (comment) is exact explanation. I've merged config migration very recently so there're no PRs built on top of it

[ggwpez]

We can do this in a follow-up to address the warning.

Then we have to ensure that this does not get into a release. I am not sure if the runtime 1.0.0 release is already branched off.

[slumber]

We can do this in a follow-up to address the warning.

Then we have to ensure that this does not get into a release. I am not sure if the runtime 1.0.0 release is already branched off.

It's safe to be released as long as we drop earlier migrations? I meant that this PR addresses unrelated thing and don't want to include these changes here.

[ordian]

bot merge

[ordian]

Polkadot should upgrade to .43 in 16 days or so? So you can probably already remove the .43 migrations (and older) to fix this. I have to admit that this UmpLimits "migration" was very hacky, but the limits needed to be bumped synchronously with the runtime upgrade for security reasons...

We can do this in a follow-up to address the warning.

Please do.

Why are runtime migration checks are failing btw? Is that expected? I don't see them failing on other PRs.

#7489 (comment) is exact explanation. I've merged config migration very recently so there're no PRs built on top of it

Yeah, sorry, I misinterpreted the explanation to be for the previous PR.