Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable Coretime on Polkadot Relay #401

Merged

Conversation

tdimitrov
Copy link
Contributor

@tdimitrov tdimitrov commented Jul 25, 2024

Redo of #354 on top of #381

TODOs:

  • Check migrations with chopsticks
  • Regenerate weights

@tdimitrov tdimitrov force-pushed the tsv-polkadot-coretime branch from f42562b to 8463811 Compare July 25, 2024 14:43
relay/polkadot/src/lib.rs Outdated Show resolved Hide resolved
// TODO: Fix this key for Polkadot!
pallet_custom_origins::Origin::AuctionAdmin.into(),
[
0x5c, 0x68, 0xbf, 0x0c, 0x2d, 0x11, 0x04, 0x91, 0x6b, 0xa5, 0xa4, 0xde, 0xe6,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this key is for scheduler -> lookup, right?

image

How can I understand which is the correct key?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the right key already.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found it by searching for Origins: AuctionAdmin. It is:

            [ 0x87, 0xa8, 0x71, 0xb4, 0xd6, 0x21, 0xf0, 0xb9, 0x73, 0x47, 0x5a, 0xaf, 0xcc,
            0x32, 0x61, 0x0b, 0xd7, 0x68, 0x8f, 0x15, 0x02, 0x33, 0x8a, 0xcd, 0x00, 0xee, 0x48,
            0x8a, 0xc3, 0x62, 0x0f, 0x4c, ],

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll provide a PR with this and other changes against your PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to commit in this PR directly (I've invited you as a collaborator).

CHANGELOG.md Outdated Show resolved Hide resolved
@@ -1677,7 +1677,7 @@ construct_runtime! {
ParaSessionInfo: parachains_session_info = 61,
ParasDisputes: parachains_disputes = 62,
ParasSlashing: parachains_slashing = 63,
OnDemandAssignmentProvider: parachains_assigner_on_demand = 64,
OnDemand: parachains_assigner_on_demand = 64,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tdimitrov Can you look into that please?

@eskimor
Copy link
Contributor

eskimor commented Jul 26, 2024

Collecting things to test/check after that PR is ready: https://github.com/orgs/paritytech/projects/119/views/20?pane=issue&itemId=66200512

Comment on lines 1908 to 1859
move_storage_from_pallet(b"Pallet", b"OnDemandAssignmentProvider", b"OnDemand");
move_storage_from_pallet(b"ParaIdAffinity", b"OnDemandAssignmentProvider", b"OnDemand");
move_storage_from_pallet(b"QueueStatus", b"OnDemandAssignmentProvider", b"OnDemand");
move_storage_from_pallet(b"FreeEntries", b"OnDemandAssignmentProvider", b"OnDemand");
move_storage_from_pallet(
b"AffinityEntries",
b"OnDemandAssignmentProvider",
b"OnDemand",
);
move_storage_from_pallet(b"Revenue", b"OnDemandAssignmentProvider", b"OnDemand");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@s0me0ne-unkn0wn does this look right to you?

As far as I understand I need to call move_storage_from_pallet for each storage item in the pallet?
Are the pallet names correct?

Ideally I should use get_storage_prefix() here but the storage items are not accessible here so I hardcoded them 🥇

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@s0me0ne-unkn0wn does this look right to you?

Well, it looks totally fine to me, but it should be taken into account that I didn't ever do it myself as well tbh 😆

As far as I understand I need to call move_storage_from_pallet for each storage item in the pallet?
Are the pallet names correct?

My understanding is the same, and the naming seems to be fine. I still hope to get some reviews from guys who have closer experience 🙂

@ggwpez ggwpez deleted the branch polkadot-fellows:main July 29, 2024 23:53
@ggwpez ggwpez closed this Jul 29, 2024
@ggwpez ggwpez reopened this Jul 30, 2024
@tdimitrov tdimitrov changed the base branch from oty-update-1-14 to main July 30, 2024 12:57
@tdimitrov tdimitrov changed the base branch from main to oty-update-1-14 July 30, 2024 12:57
@tdimitrov tdimitrov changed the base branch from oty-update-1-14 to main July 30, 2024 12:59
@tdimitrov tdimitrov force-pushed the tsv-polkadot-coretime branch from a6794d1 to a274576 Compare July 30, 2024 14:01
@tdimitrov tdimitrov marked this pull request as ready for review July 30, 2024 14:04

// Migrate storage for pallet rename `OnDemandAssignmentProvider` -> `OnDemand`
pub struct OnDemandRename;
impl OnRuntimeUpgrade for OnDemandRename {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things here:

  1. For Polkadot no migration should be needed as onDemand was newly introduced in this PR.
  2. Do we need any protection here from preventing this migration to be run multiple times?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've extracted the pallet rename on Kusama in a separate PR: #417

Copy link
Contributor

@seadanda seadanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

relay/polkadot/constants/src/lib.rs Outdated Show resolved Hide resolved
parameter_types! {
pub const BrokerId: u32 = system_parachain::BROKER_ID;
pub const BrokerPalletId: PalletId = PalletId(*b"py/broke");
pub MaxXcmTransactWeight: Weight = Weight::from_parts(constants::WEIGHT_REF_TIME_PER_MILLIS, 20 * constants::WEIGHT_PROOF_SIZE_PER_KB);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as here: do we need such a huge buffer? Or maybe the other way round, do we care that we have such a big buffer? If not then I'd increase the Coretime Chain side too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took this from Kusama. If the value is not appropriate we should change it there too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, I probably took the values from master instead of from the release branch. Fixed.

CHANGELOG.md Outdated Show resolved Hide resolved
relay/polkadot/src/lib.rs Outdated Show resolved Hide resolved
@tdimitrov tdimitrov requested a review from eskimor August 6, 2024 06:49
This was referenced Aug 6, 2024
// ./target/production/polkadot
// benchmark
// pallet
// --chain=./kusama-chain-spec.json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as above

// ./target/production/polkadot
// benchmark
// pallet
// --chain=./kusama-chain-spec.json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be generated with --chain=./polkadot-chain-spec.json ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be another pre-release PR that regenerates all the weights. Usually @bkontur does it (although would be good to improve/automate that process)

relay/polkadot/src/lib.rs Outdated Show resolved Hide resolved
@eskimor
Copy link
Contributor

eskimor commented Aug 8, 2024

/merge

@fellowship-merge-bot fellowship-merge-bot bot merged commit b52f7de into polkadot-fellows:main Aug 8, 2024
45 checks passed
@fellowship-merge-bot
Copy link
Contributor

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

fellowship-merge-bot bot added a commit that referenced this pull request Aug 13, 2024
Add the Polkadot Coretime chain in advance of the 1.3.0 release.

This uses the new Price adapter which has been running on Kusama now for
two sales cycles, and includes the mechanism to burn revenue.

TODO:
- [x] Add Transact tests for hardcoded weights after
#401 is merged
- [ ] Rerun benchmarks and check hardcoded weights after merge

The genesis chain-spec is developing on
seadanda#4. This can be used as a merge
target for any community boot nodes who would like to be included at
genesis and will be separately merged to `main`.

---------

Co-authored-by: Bastian Köcher <[email protected]>
Co-authored-by: Branislav Kontur <[email protected]>
Co-authored-by: fellowship-merge-bot[bot] <151052383+fellowship-merge-bot[bot]@users.noreply.github.com>
seadanda added a commit to seadanda/runtimes that referenced this pull request Aug 14, 2024
…chains (#4)

* Add kusama live preset and chain spec to builder

* Add coretime-polkadot chainspec

* Make clippy happy

* Remove unnecessary dep

* Use presets for new chainspecs

* Add live to preset names

* Add stake plus bootnodes to coretime polkadot at release. (#5)

* Add the Polkadot Coretime runtime (polkadot-fellows#410)

Add the Polkadot Coretime chain in advance of the 1.3.0 release.

This uses the new Price adapter which has been running on Kusama now for
two sales cycles, and includes the mechanism to burn revenue.

TODO:
- [x] Add Transact tests for hardcoded weights after
polkadot-fellows#401 is merged
- [ ] Rerun benchmarks and check hardcoded weights after merge

The genesis chain-spec is developing on
#4. This can be used as a merge
target for any community boot nodes who would like to be included at
genesis and will be separately merged to `main`.

---------

Co-authored-by: Bastian Köcher <[email protected]>
Co-authored-by: Branislav Kontur <[email protected]>
Co-authored-by: fellowship-merge-bot[bot] <151052383+fellowship-merge-bot[bot]@users.noreply.github.com>

* Update changelog

* all runtimes: remove already applied migrations (polkadot-fellows#420)

Remove migrations already **applied on-chain**.

- [x] Does not require a CHANGELOG entry

---------

Signed-off-by: Adrian Catangiu <[email protected]>
Co-authored-by: joe petrowski <[email protected]>

---------

Signed-off-by: Adrian Catangiu <[email protected]>
Co-authored-by: Tom <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>
Co-authored-by: Branislav Kontur <[email protected]>
Co-authored-by: fellowship-merge-bot[bot] <151052383+fellowship-merge-bot[bot]@users.noreply.github.com>
Co-authored-by: Adrian Catangiu <[email protected]>
Co-authored-by: joe petrowski <[email protected]>
fellowship-merge-bot bot added a commit that referenced this pull request Aug 15, 2024
…ration (#426)

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](https://github.com/paritytech/polkadot-sdk/blob/1f49358db0033e57a790eac6daccc45beba81863/polkadot/runtime/parachains/src/coretime/migration.rs#L114))
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.

---------

Co-authored-by: Bastian Köcher <[email protected]>
Co-authored-by: Dónal Murray <[email protected]>
Co-authored-by: fellowship-merge-bot[bot] <151052383+fellowship-merge-bot[bot]@users.noreply.github.com>
@tdimitrov tdimitrov deleted the tsv-polkadot-coretime branch August 19, 2024 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants