Migrate parachain swaps to Coretime by tdimitrov · Pull Request #3714 · paritytech/polkadot-sdk

tdimitrov · 2024-03-15T14:46:40Z

This PR notifies broker pallet for any parachain slot swaps performed on the relay chain. This is achieved by registering an OnSwap for the the coretime pallet. The hook sends XCM message to the broker chain and invokes a new extrinsic swap_leases which updates Leases storage item (which keeps the legacy parachain leases).

I made two assumptions in this PR:

Leases in broker pallet and Leases in slots pallet are in sync.
swap_leases extrinsic from broker pallet can be triggered only by root or by the XCM message from the relay chain. If not - the extrinsic will generate an error and do nothing.

As a side effect from the changes OnSwap trait is moved from runtime/common/traits.rs to runtime/parachains. Otherwise it is not accessible from broker pallet.

Closes #3552

TODOs:

Weights
Tests

eskimor

Looks good!

tdimitrov · 2024-03-20T07:49:02Z

bot bench polkadot-pallet --runtime=coretime-rococo-dev --pallet=pallet_broker
bot bench polkadot-pallet --runtime=dev --pallet=pallet_broker

tdimitrov · 2024-03-20T07:52:10Z

bot help

tdimitrov · 2024-03-20T07:53:10Z

bot bench substrate-pallet --pallet=pallet_broker

tdimitrov · 2024-03-20T07:55:05Z

bot bench cumulus-coretime --pallet=pallet_broker

…nto HEAD

…=dev --target_dir=substrate --pallet=pallet_broker

…=coretime-rococo --runtime_dir=coretime --target_dir=cumulus --pallet=pallet_broker

tdimitrov · 2024-03-20T09:06:14Z

bot bench cumulus-coretime --runtime coretime-westend --pallet=pallet_broker

…=coretime-westend --runtime_dir=coretime --target_dir=cumulus --pallet=pallet_broker

tdimitrov · 2024-03-20T11:00:37Z

bot clean

tdimitrov · 2024-03-21T09:30:14Z

To get this tested we need to ensure that the interaction between the relay and broker chain works correctly. So unit test or integration test (as in runtime integration tests) is not enough.

We can test this with zombienet but this will mean to port this test to zombienet which won't be trivial.

For these reasons the plan is to test this PR directly on Rococo.

eskimor · 2024-03-21T09:48:37Z

PR doc missing. Worth mentioning to users that this is taking care of their swaps.

antonva

Looks good to me so far.

antonva · 2024-03-21T10:30:02Z

+			.into_iter()
+			.map(|mut lease| {
+				if lease.task == id {
+					lease.task = other;


nit: I don't think TaskId uniqueness is enforced anywhere in Leases, so this effectively swaps all leases sharing a TaskId. This shouldn't really matter as there should only be one task for both id and other.

Good point though!

I thought there could be multiple leases and we do want to swap all of them.

If this is the case, we'd better add another ensure! for it?

Like this -> f5f6dce

The error code I use is not entirely correct but I don't see a point adding a new one just for a temp call.

No Basti has a point. The "correctness" of the swap has already been verified on the relay chain, we should be very lenient on the Coretime chain. In particular, there could be a race and the lease of one para expired by the time the message arrives at the coretime chain (admittedly super unlikely edge case, but still), we should still do the swap, although one of the leases does not exist.

I mean that race in particular is really super niche, if a user updates that close he could miss the deadline himself already.

Nevertheless, I would err on as little ensures as possible: The swap is confirmed, it actually has to be good.

Yes, I came to the same conclusion and removed everything. Even if someone decides to use the extrinsic directly it can be called again and 'undo' the result.

bkchr · 2024-03-22T08:11:39Z

+		ensure!(id_leases_count > 0 && other_leases_count > 0, Error::<T>::UnknownReservation);
+		// And ensure there are just to leases to be swapped
+		ensure!(id_leases_count == 1 && other_leases_count == 1, Error::<T>::TooManyLeases);


This is wrong. When one of the leases for example already ended, you should still be able to swap it with a "working one"

I can imagine the source lease (id in this case) might not exist and is not correct to enforce its existence because of what you wrote.

But why would one want to swap something with a non-existent lease? The way I understand it we should have:

ensure!(other_leases_count > 0, Error::<T>::UnknownXXX);

Never mind, it's unnecessary complication. Even if someone does it - he/she can revert it back anytime.

Co-authored-by: Bastian Köcher <git@kchr.de>

paritytech-cicd-pr · 2024-03-22T08:52:29Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 2/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5613814

tdimitrov · 2024-03-26T13:52:32Z

Tested locally with zombienet - no issues found.

This PR notifies broker pallet for any parachain slot swaps performed on the relay chain. This is achieved by registering an `OnSwap` for the the `coretime` pallet. The hook sends XCM message to the broker chain and invokes a new extrinsic `swap_leases` which updates `Leases` storage item (which keeps the legacy parachain leases). I made two assumptions in this PR: 1. [`Leases`](https://github.com/paritytech/polkadot-sdk/blob/4987d7982461e2e5ffe219cdf71ec697284cea7c/substrate/frame/broker/src/lib.rs#L120) in `broker` pallet and [`Leases`](https://github.com/paritytech/polkadot-sdk/blob/4987d7982461e2e5ffe219cdf71ec697284cea7c/polkadot/runtime/common/src/slots/mod.rs#L118) in `slots` pallet are in sync. 2. `swap_leases` extrinsic from `broker` pallet can be triggered only by root or by the XCM message from the relay chain. If not - the extrinsic will generate an error and do nothing. As a side effect from the changes `OnSwap` trait is moved from runtime/common/traits.rs to runtime/parachains. Otherwise it is not accessible from `broker` pallet. Closes paritytech#3552 TODOs: - [x] Weights - [x] Tests --------- Co-authored-by: command-bot <> Co-authored-by: eskimor <eskimor@users.noreply.github.com> Co-authored-by: Bastian Köcher <git@kchr.de>

tdimitrov added 4 commits March 14, 2024 09:51

Move trait OnSwap from runtime_common to parachains

36a6368

Benchmark and weights

37962d0

swap_leases implementation

d60b31e

Fix compilation errors

03aaad2

eskimor reviewed Mar 15, 2024

View reviewed changes

Comment thread polkadot/runtime/common/src/traits.rs

Comment thread polkadot/runtime/parachains/src/coretime/mod.rs

Fix benchmark

bd32142

tdimitrov added the T8-polkadot This PR/Issue is related to/affects the Polkadot network. label Mar 18, 2024

tdimitrov added 4 commits March 18, 2024 16:24

Undo trait move and fix OnSwap registration

4bd69ab

Change call id of swap_leases to 99

518a072

Fix a compilation error

296521a

clippy

e0a1424

command-bot added 3 commits March 20, 2024 07:55

Merge branch 'master' of https://github.com/paritytech/polkadot-sdk i…

ff0d3d7

…nto HEAD

".git/.scripts/commands/bench/bench.sh" --subcommand=pallet --runtime…

2f72d26

…=dev --target_dir=substrate --pallet=pallet_broker

".git/.scripts/commands/bench/bench.sh" --subcommand=pallet --runtime…

b2c5ddd

…=coretime-rococo --runtime_dir=coretime --target_dir=cumulus --pallet=pallet_broker

".git/.scripts/commands/bench/bench.sh" --subcommand=pallet --runtime…

1953bc2

…=coretime-westend --runtime_dir=coretime --target_dir=cumulus --pallet=pallet_broker

Merge branch 'master' into tsv-ct-parachain-swaps

e419bb7

tdimitrov marked this pull request as ready for review March 21, 2024 09:27

tdimitrov requested a review from a team as a code owner March 21, 2024 09:27

tdimitrov requested a review from antonva March 21, 2024 09:30

Merge branch 'master' into tsv-ct-parachain-swaps

8986843

eskimor approved these changes Mar 21, 2024

View reviewed changes

Comment thread polkadot/runtime/rococo/src/lib.rs Outdated

antonva reviewed Mar 21, 2024

View reviewed changes

antonva approved these changes Mar 21, 2024

View reviewed changes

tdimitrov added 5 commits March 21, 2024 14:03

Add a prdoc

7f630c8

Implement OnSwap for a struct instead of for Runtime directly 😅

cc2efb7

Merge branch 'master' into tsv-ct-parachain-swaps

a8f17f7

Swap no more than two leases

f5f6dce

Merge branch 'master' into tsv-ct-parachain-swaps

113bbcf

bkchr reviewed Mar 22, 2024

View reviewed changes

Apply suggestions from code review

89ef634

Co-authored-by: Bastian Köcher <git@kchr.de>

paritytech-review-bot Bot requested a review from a team March 22, 2024 08:48

Fix compilation errors and apply more code review suggestions

fce11f9

franciscoaguirre approved these changes Mar 22, 2024

View reviewed changes

tdimitrov added 2 commits March 26, 2024 11:46

Merge branch 'master' into tsv-ct-parachain-swaps

7351ea6

Fix taplo errors

0bbcfd1

tdimitrov added this pull request to the merge queue Mar 26, 2024

Merged via the queue into master with commit 9023454 Mar 26, 2024

tdimitrov deleted the tsv-ct-parachain-swaps branch March 26, 2024 17:00

eskimor mentioned this pull request Mar 27, 2024

1.3.0 Release polkadot-fellows/runtimes#186

Closed

9 tasks

Conversation

tdimitrov commented Mar 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eskimor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tdimitrov commented Mar 20, 2024

Uh oh!

tdimitrov commented Mar 20, 2024

Uh oh!

tdimitrov commented Mar 20, 2024

Uh oh!

tdimitrov commented Mar 20, 2024

Uh oh!

tdimitrov commented Mar 20, 2024

Uh oh!

tdimitrov commented Mar 20, 2024

Uh oh!

tdimitrov commented Mar 21, 2024

Uh oh!

eskimor commented Mar 21, 2024

Uh oh!

Uh oh!

antonva left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tdimitrov Mar 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

paritytech-cicd-pr commented Mar 22, 2024

Uh oh!

tdimitrov commented Mar 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tdimitrov commented Mar 15, 2024 •

edited

Loading

tdimitrov Mar 21, 2024 •

edited

Loading