Proper handling for legacy parachain leases with gaps in coretime migration #426

tdimitrov · 2024-08-12T11:47:05Z

While testing #401 I noticed that there are two parachains with gaps in their leases which are not migrated to coretime correctly. These are para ids 3359 and 3388.

The problem is that the migration obtains the list of parachain ids which should be migrated from paras::Parachains (here) and there are no entries for para ids which are not active at the moment.

Paras 3359 and 3388 are not active in the current lease period (excerpt from slots::leases():

[
      3,359
    ]
    [
      null
      [
        14xQXJdUDC1pzyt8y3z27ANiUBgP7zTSaYutaLELJoyQrdLP
        2,010,000,000,000
      ]
      [
        14xQXJdUDC1pzyt8y3z27ANiUBgP7zTSaYutaLELJoyQrdLP
        2,010,000,000,000
      ]
      <skipped>

and:

[
      3,388
    ]
    [
      null
      [
        1TpMimWf8NvrusAQ36hVdgMpYhhtojg5Nw41CuVxG2zHPDg
        31,000,000,000,000
      ]
      [
        1TpMimWf8NvrusAQ36hVdgMpYhhtojg5Nw41CuVxG2zHPDg
        31,000,000,000,000
      ]
      <skipped>

And they have got no entries in paras::parachains():

[ 1,000 1,001 1,002 1,004 2,000 2,002 2,004 2,006 2,008 2,012 2,013 2,025 2,026 2,030 2,031 2,032 2,034 2,035 2,037 2,040 2,043 2,046 2,048 2,051 2,053 2,056 2,086 2,090 2,091 2,092 2,093 2,094 2,101 2,104 2,106 3,333 3,338 3,340 3,344 3,345 3,346 3,354 3,358 3,366 3,367 3,369 3,370 3,373 3,375 3,377 ]

As a result the migration skips them which is wrong.

A proper fix for this issue will require a new polkadot-sdk release and a bump in the runtimes repo which requires time and effort. To avoid this the PR moves the coretime migration from polkadot-sdk to the fellowship repo.

tdimitrov · 2024-08-12T11:50:10Z

This is a draft because I am not happy with already_migrated function in the PR.

…oper fix can be applied

tdimitrov · 2024-08-14T07:22:41Z

As discussed offline with @eskimor we'll move the whole coretime migration to the fellowship repo so that a proper fix can be made.
The PR is tested with this semi-automated test: https://github.com/tdimitrov/polkadot-fellows-runtimes/
I'll extend the repo with some more asserts but meanwhile the PR is ready for review.

eskimor · 2024-08-14T08:30:55Z

relay/polkadot/src/coretime_migration.rs

+	});
+
+	let reservation_content = message_content.clone().chain(reservations).collect();
+	let pool_content = message_content.clone().chain(pool).collect();


Pool will be empty. Do we send an empty message here?

Hmm I missed that. I guess we send an empty message. I'll fix it.

eskimor · 2024-08-14T10:59:41Z

relay/polkadot/src/coretime_migration.rs

+			Some(mk_coretime_call::<T>(CoretimeCalls::SetLease(p.into(), time_slice)))
+		});
+
+	let core_count: u16 = configuration::ActiveConfig::<T>::get()


Hmm. on another call in a minute, but please check that this core handling is actually correct. This core_count value should read 0 on Polkadot at the time of the migration.

It's not 0. We update it on line 211-214 in the same file:
https://github.com/polkadot-fellows/runtimes/pull/426/files#diff-6a311f9b6c4cd2a09f397e5e0c90ea9193ba103bf277bb88de6bd223148af52eR211-R214

The actual value is 52:

coretime-migration TRACE: Set core count to 52. legacy paras count is 52

The reserved cores are 0 though because legacy_paras_count == core_count. I have got an assert for this in the test:
https://github.com/tdimitrov/polkadot-coretime-launch-test/blob/a8e7aba7d6a741b10bd5330ae3119ef15a018711/coretime-migration-check/main.js#L158-L160

Is this correct?

The reserved cores are 0 though

What you mean by reserved cores? The system chains?

Generally your code is correct.

Yes, the system chains.

No, wait. These are reserved for on demand, if I am not mistaken.
We reserve cores for system chains, then for lease cores and what's left is for on demand. Right @eskimor ?

Pre-migration the system chains just have leases as if they're regular parachains. They're included in the 52, except for the coretime chain since it isn't live yet. It will be registered by the time this migration runs, so it'll detect 53 leases (maybe more depending on how the auctions go).
reservations: Asset Hub, Collectives, Bridge Hub, People Chain and Coretime Chain for a total of 5 (you'll see 4 at the minute in your tests)
leases: 48 (plus any new ones from auctions, minus any that end between now and when it runs).

legacy_paras_count isn't the same as leases though, I think the assertion you're expecting is system_chains.len() + leases.len() == core_count, since core_count is equal to legacy_paras_count by construction (since num_cores is zero atm)

relay/polkadot/src/lib.rs

relay/polkadot/Cargo.toml

relay/polkadot/src/coretime_migration.rs

bkchr · 2024-08-14T11:34:19Z

relay/polkadot/src/coretime_migration.rs

+			let valid_until: u32 = match valid_until.try_into() {
+				Ok(val) => val,
+				Err(_) => {
+					log::error!("Converting block number to u32 failed!");
+					return None
+				},
+			};


Suggested change

let valid_until: u32 = match valid_until.try_into() {

Ok(val) => val,

Err(_) => {

log::error!("Converting block number to u32 failed!");

return None

},

};

block number is u32

It doesn't compile due to expected u32 found associated type errors. Let's keep it as it is.

bkchr · 2024-08-14T11:35:52Z

relay/polkadot/src/coretime_migration.rs

+			let round_up = if valid_until % TIME_SLICE_PERIOD > 0 { 1 } else { 0 };
+			let time_slice = valid_until / TIME_SLICE_PERIOD + TIME_SLICE_PERIOD * round_up;


Suggested change

let round_up = if valid_until % TIME_SLICE_PERIOD > 0 { 1 } else { 0 };

let time_slice = valid_until / TIME_SLICE_PERIOD + TIME_SLICE_PERIOD * round_up;

let time_slice = (valid_until + TIME_SLICE_PERID - 1) / TIME_SLICE_PERIOD;

I think we do a different kind of round up here. timeslice is not number of periods but a lifetime in blocks (divided by 80). Your formula works only for the 'number of periods' case.

I'm confused... why we multiply the round up with TIME_SLICE_PERIOD?

Shouldn't it be let time_slice = valid_until / TIME_SLICE_PERIOD + round_up;) which is equivalent to your formula?

round_up is just 0 or 1. It leads to the same result as what I have written there.

It doesn't. Round up is multiplied by TIME_SLICE_PERIOD. Have a look: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=a5ce70639f2ee8a169e5d618e707ed74

Basti's version is right. Integer division is just the floor, adding TIMESLICE_PERIOD rounds up, subtracting 1 makes sure it doesn't round up when v%T == 0.
The original code is adding 80 timeslices to the end of the lease, rather than one.

relay/polkadot/src/coretime_migration.rs

Co-authored-by: Bastian Köcher <[email protected]>

relay/polkadot/src/coretime_migration.rs

seadanda · 2024-08-14T16:32:23Z

relay/polkadot/src/coretime_migration.rs

+			let round_up = if valid_until % TIME_SLICE_PERIOD > 0 { 1 } else { 0 };
+			let time_slice = valid_until / TIME_SLICE_PERIOD + TIME_SLICE_PERIOD * round_up;


Basti's version is right. Integer division is just the floor, adding TIMESLICE_PERIOD rounds up, subtracting 1 makes sure it doesn't round up when v%T == 0.
The original code is adding 80 timeslices to the end of the lease, rather than one.

seadanda · 2024-08-14T16:47:16Z

relay/polkadot/src/coretime_migration.rs

+			Some(mk_coretime_call::<T>(CoretimeCalls::SetLease(p.into(), time_slice)))
+		});
+
+	let core_count: u16 = configuration::ActiveConfig::<T>::get()


Pre-migration the system chains just have leases as if they're regular parachains. They're included in the 52, except for the coretime chain since it isn't live yet. It will be registered by the time this migration runs, so it'll detect 53 leases (maybe more depending on how the auctions go).
reservations: Asset Hub, Collectives, Bridge Hub, People Chain and Coretime Chain for a total of 5 (you'll see 4 at the minute in your tests)
leases: 48 (plus any new ones from auctions, minus any that end between now and when it runs).

legacy_paras_count isn't the same as leases though, I think the assertion you're expecting is system_chains.len() + leases.len() == core_count, since core_count is equal to legacy_paras_count by construction (since num_cores is zero atm)

seadanda · 2024-08-14T16:55:21Z

relay/polkadot/src/coretime_migration.rs

+	let pool_content = message_content.clone().chain(pool).collect();
+	let leases_content_1 = message_content
+		.clone()
+		.chain(leases.by_ref().take(legacy_paras_count / 2)) // split in two messages to avoid overweighted XCM


Suggested change

.chain(leases.by_ref().take(legacy_paras_count / 2)) // split in two messages to avoid overweighted XCM

.chain(leases.by_ref().take(leases.len() / 2)) // split in two messages to avoid overweighted XCM

legacy_paras_count includes system parachains
leases does not

Yes, but leases is an iterator and it's a bit involved to get its length. Let's keep it that way. It's not a big deal that the messages aren't equally distributed as long as they are sent in two passes (it's just one ParaId which causes the overweight).

seadanda · 2024-08-14T17:13:48Z

relay/polkadot/src/coretime_migration.rs

+
+	let config = configuration::ActiveConfig::<T>::get();
+	// num_cores was on_demand_cores until now:
+	for on_demand in 0..config.scheduler_params.num_cores {


On Polkadot this is not the path of this config entry, checking the chain state it's at config.coretime_cores. Is there a migration ahead of this one to change the configuration in storage?

parachains_configuration::migration::v12::MigrateToV12<Runtime> does this change:
https://github.com/paritytech/polkadot-sdk/blob/ebf4f8d2d590f41817d5d38b2d9b5812a46f2342/polkadot/runtime/parachains/src/configuration/migration/v12.rs#L146-L158

It gets executed before the coretime migration.

Still the comment is actually not correct for Polkadot. It used to be coretime_cores not on_demand cores and there was no on-demand core before, thus nothing to migrate.

(This loop will do nothing, which is fine)

I've removed the comment.

Co-authored-by: Dónal Murray <[email protected]>

seadanda

Looks good

relay/polkadot/src/coretime_migration.rs

Co-authored-by: Dónal Murray <[email protected]>

eskimor · 2024-08-15T07:57:42Z

relay/polkadot/src/coretime_migration.rs

+
+	let config = configuration::ActiveConfig::<T>::get();
+	// num_cores was on_demand_cores until now:
+	for on_demand in 0..config.scheduler_params.num_cores {


Still the comment is actually not correct for Polkadot. It used to be coretime_cores not on_demand cores and there was no on-demand core before, thus nothing to migrate.

eskimor · 2024-08-15T07:58:31Z

relay/polkadot/src/coretime_migration.rs

+
+	let config = configuration::ActiveConfig::<T>::get();
+	// num_cores was on_demand_cores until now:
+	for on_demand in 0..config.scheduler_params.num_cores {


(This loop will do nothing, which is fine)

seadanda

Re-approving to trigger a CI run with the broken runner disabled

tdimitrov · 2024-08-15T19:28:03Z

There was a try-runtime check verifying if the relay chain is sending XMP messages to the broker chain. It was using dmq_contents which is not accessible from the runtime crate. Since I have tested the migration with chopsticks I think it's safe to drop it.

Please see this commit for details: 4675183

seadanda · 2024-08-15T20:57:55Z

LGTM, zombienet is failing in CI but seems like it's unrelated

seadanda · 2024-08-15T20:58:17Z

/merge

fellowship-merge-bot · 2024-08-15T21:04:34Z

Enabled auto-merge in Pull Request

Available commands

/merge: Enables auto-merge for Pull Request
/merge cancel: Cancels auto-merge for Pull Request
/merge help: Shows this menu

For more information see the documentation

#5380) Backport fixes from polkadot-fellows/runtimes#426

paritytech#5380) Backport fixes from polkadot-fellows/runtimes#426

tdimitrov added 2 commits August 12, 2024 14:21

Handle parachains with gaps when migrating to agile coretime

f10c395

Reorder migrations to make already_migrated

863ed89

Move the whole coretime migration to the fellowship repo so that a pr…

7590924

…oper fix can be applied

tdimitrov marked this pull request as ready for review August 14, 2024 07:22

eskimor approved these changes Aug 14, 2024

View reviewed changes

clippy

a38eb7a

eskimor reviewed Aug 14, 2024

View reviewed changes

bkchr approved these changes Aug 14, 2024

View reviewed changes

tdimitrov and others added 4 commits August 14, 2024 16:23

Remove unneeded sort

f7116ba

Apply suggestions from code review

ae4dd55

Co-authored-by: Bastian Köcher <[email protected]>

Undo some sugestions

4279c2a

Fix feature propagation

677f5be

seadanda reviewed Aug 14, 2024

View reviewed changes

tdimitrov and others added 4 commits August 15, 2024 09:08

Fix timeslice calculation

d475837

Apply suggestions from code review

fd636f1

Co-authored-by: Dónal Murray <[email protected]>

Use TIMESLICE_PERIOD from polkadot_runtime_constants

c4c1a18

Undo code review suggestion

e97b31d

tdimitrov requested a review from seadanda August 15, 2024 06:36

If pool_content is empty don't send a blank XCM message

2f73317

seadanda approved these changes Aug 15, 2024

View reviewed changes

relay/polkadot/src/coretime_migration.rs Outdated Show resolved Hide resolved

Update relay/polkadot/src/coretime_migration.rs

74a930c

Co-authored-by: Dónal Murray <[email protected]>

eskimor approved these changes Aug 15, 2024

View reviewed changes

tdimitrov added 4 commits August 15, 2024 11:32

Remove a comment

d3ac70b

Fix feature propagation

bd27d40

Update CHANGELOG

bd0bf5f

Merge branch 'main' into tsv-coretime-null-slots

6f28106

seadanda mentioned this pull request Aug 15, 2024

1.3.0 Release #186

Closed

9 tasks

seadanda approved these changes Aug 15, 2024

View reviewed changes

tdimitrov added 2 commits August 15, 2024 16:16

Restart CI

6cef2a5

Fix try-runtime

4675183

Merge branch 'main' into tsv-coretime-null-slots

4036d15

fellowship-merge-bot bot enabled auto-merge (squash) August 15, 2024 21:04

fellowship-merge-bot bot merged commit ffe6a36 into polkadot-fellows:main Aug 15, 2024
47 checks passed

tdimitrov mentioned this pull request Aug 16, 2024

Fix leases with gaps and time slice calculation in MigrateToCoretime paritytech/polkadot-sdk#5380

Merged

github-merge-queue bot pushed a commit to paritytech/polkadot-sdk that referenced this pull request Aug 20, 2024

Fix leases with gaps and time slice calculation in MigrateToCoretime (

37c2027

#5380) Backport fixes from polkadot-fellows/runtimes#426

nazar-pc pushed a commit to nazar-pc/polkadot-sdk that referenced this pull request Aug 21, 2024

Fix leases with gaps and time slice calculation in MigrateToCoretime (

f935e00

paritytech#5380) Backport fixes from polkadot-fellows/runtimes#426

		let round_up = if valid_until % TIME_SLICE_PERIOD > 0 { 1 } else { 0 };
		let time_slice = valid_until / TIME_SLICE_PERIOD + TIME_SLICE_PERIOD * round_up;

	let round_up = if valid_until % TIME_SLICE_PERIOD > 0 { 1 } else { 0 };
	let time_slice = valid_until / TIME_SLICE_PERIOD + TIME_SLICE_PERIOD * round_up;
	let time_slice = (valid_until + TIME_SLICE_PERID - 1) / TIME_SLICE_PERIOD;

	.chain(leases.by_ref().take(legacy_paras_count / 2)) // split in two messages to avoid overweighted XCM
	.chain(leases.by_ref().take(leases.len() / 2)) // split in two messages to avoid overweighted XCM

Proper handling for legacy parachain leases with gaps in coretime migration #426

Proper handling for legacy parachain leases with gaps in coretime migration #426

Conversation

tdimitrov commented Aug 12, 2024 • edited Loading

tdimitrov commented Aug 12, 2024

tdimitrov commented Aug 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdimitrov Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seadanda Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seadanda Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdimitrov Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seadanda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seadanda left a comment

Choose a reason for hiding this comment

tdimitrov commented Aug 15, 2024

seadanda commented Aug 15, 2024

seadanda commented Aug 15, 2024

fellowship-merge-bot bot commented Aug 15, 2024

tdimitrov commented Aug 12, 2024 •

edited

Loading

tdimitrov Aug 14, 2024 •

edited

Loading

seadanda Aug 14, 2024 •

edited

Loading

seadanda Aug 14, 2024 •

edited

Loading

tdimitrov Aug 15, 2024 •

edited

Loading