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.

Sign up for GitHub

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

Jump to bottom

"Common good" vs "System" parachain clean up #1406

Merged
merged 17 commits into from
Sep 18, 2023
Merged

"Common good" vs "System" parachain clean up #1406

merged 17 commits into from
Sep 18, 2023

Conversation

bkontur
Copy link
Contributor

@bkontur bkontur commented Sep 5, 2023 •
edited
Loading

Summary

The term "common good parachain" has been abandoned in favor of "system parachain" - e.g. Joe's speech at Decoded2023. This pull request tries to fix and align code with this vision.

Impact

The important change is implementation of trait IsSystem for Id here where we changed condition from < 1000 to <= 1999, which means that all parachain IDs bellow 1999 (included) are considered as "system parachain" IDs. This change has a direct impact on the following components:

ChildSystemParachainAsSuperuser

This origin converter is used for allowing to process XCM Transact from "system parachain" on the relay chain - e.g. see configuration for Kusama.
Only configured for Kusama, Westend, Rococo runtimes.

No need for this feature anymore. See comment.

IsChildSystemParachain

IsChildSystemParachain is used with AllowExplicitUnpaidExecutionFrom barrier for checking XCM programs (they have to start with UnpaidExecution instruction).
Only configured for Kusama, Westend, Rococo runtimes.

Overall the impact is low or mostly ok because it only allows unpaid execution for "system parachains" (e.g. AssetHub, BridgeHub...) on the relay chain.

SiblingSystemParachainAsSuperuser

Not used anywhere in polkadot-sdk repo.

Unresolved Questions

  • constants LOWEST_USER_ID and LOWEST_PUBLIC_ID seem to express the same thing now, do we want to keep them both or deprecated one of them? If so, which one?
  • determine impact for ChildSystemParachainAsSuperuser

TODO

  • when merged here, open PR to the polkadot-fellows

Related Material

https://youtu.be/CSO-ERHK2gY?t=456
https://forum.polkadot.network/t/polkadot-protocol-and-common-good-parachains/866
https://wiki.polkadot.network/docs/learn-system-chains

@bkontur bkontur added T8-polkadot This PR/Issue is related to/affects the Polkadot network. T9-cumulus This PR/Issue is related to cumulus. labels Sep 5, 2023
@bkontur bkontur mentioned this pull request Sep 6, 2023
Merged
@bkontur bkontur marked this pull request as ready for review September 6, 2023 10:23
@paritytech-ci paritytech-ci requested review from a team September 6, 2023 10:24
@bkontur
Copy link
Contributor Author

bkontur commented Sep 6, 2023

Based on discussion with @joepetrowski, Transact from system parachain to relay chain was used for opening HRMP channels, but now we have a direct way to force open them. So we should better remove this ChildSystemParachainAsSuperuser feature until we really need it.

@joepetrowski
Copy link
Contributor

constants LOWEST_USER_ID and LOWEST_PUBLIC_ID seem to express the same thing now, do we want to keep them both or deprecated one of them? If so, which one?

IMO OK to combine, slight preference for LOWEST_PUBLIC_ID.

@joepetrowski joepetrowski mentioned this pull request Sep 13, 2023
Merged
3 tasks
@bkontur
Copy link
Contributor Author

bkontur commented Sep 13, 2023

constants LOWEST_USER_ID and LOWEST_PUBLIC_ID seem to express the same thing now, do we want to keep them both or deprecated one of them? If so, which one?

IMO OK to combine, slight preference for LOWEST_PUBLIC_ID.

@paritytech/polkadot-review Can you please confirm that we can remove LOWEST_USER_ID? It is only re-exported in polkadot/primitives/src/v5/mod.rs. Or what do you suggest?

@joepetrowski
Copy link
Contributor

I am 99.9% sure this value is never used anywhere except the logic to allow Root privilege from a parachain (which we've removed here anyway).

@bkontur bkontur mentioned this pull request Sep 13, 2023
Merged
ordian
ordian approved these changes Sep 14, 2023
View reviewed changes
polkadot/parachain/src/primitives.rs Outdated
const PUBLIC_INDEX_START: u32 = 2000;

/// The ID of the first user (non-system) parachain.
pub const LOWEST_USER_ID: Id = Id(USER_INDEX_START);
pub const LOWEST_USER_ID: Id = Id(PUBLIC_INDEX_START);
Copy link
Member

Choose a reason for hiding this comment

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

I was not able to find any reference on github other than forks of Polkadot:
https://github.com/search?q=LOWEST_USER_ID+language%3ARust&type=code&ref=advsearch
(let me know if you know how to filter out forks in the search query)

so it should be fine to remove

@bkontur bkontur enabled auto-merge (squash) September 18, 2023 08:28
@bkontur bkontur merged commit d569e72 into master Sep 18, 2023
107 of 109 checks passed
@bkontur bkontur deleted the bko-system-vs-common-good branch September 18, 2023 09:11
@bkontur bkontur mentioned this pull request Sep 18, 2023
Closed
@bkontur bkontur mentioned this pull request Sep 27, 2023
Merged
@kiltbot kiltbot mentioned this pull request Oct 19, 2023
Closed
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-release-analysis-v1-2-0/4451/1

@ahmadkaouk ahmadkaouk mentioned this pull request Nov 16, 2023
Closed
fellowship-merge-bot bot pushed a commit to polkadot-fellows/runtimes that referenced this pull request Dec 8, 2023
@bkontur @bkchr
Various backports and alignments with polkadot-sdk (which does not …
d67b905 
…depend on crates.io versions) (#45)

## Summary

This PR proposes several changes.

- The alignment of the terminology used for "common-good" and "system"
parachains in the Polkadot ecosystem, as discussed in
[PR#1406](paritytech/polkadot-sdk#1406).
- Additionally, it suggests the removal of the
`ChildSystemParachainAsSuperuser` feature from the Kusama runtime, as
mentioned in a comment on the same PR [comment
link](paritytech/polkadot-sdk#1406 (comment)).
- Relax `force_default_xcm_version` in `SafeCallFilter` for system
parachains - based on
paritytech/polkadot-sdk#2385
- Add `Storage` item to `PolkadotXcm` pallet definition for BridgeHubs
(now we cannot see storage items for `pallet_xcm` in polkadot.js) -
based on paritytech/polkadot-sdk#2385

## Explanation

A significant [change of
PR#1406](https://github.com/paritytech/polkadot-sdk/pull/1406/files#diff-0b7b4f5b962a18ce980354592b55ab2a27b5a2e9f6f8089ec803ca73853e8583L202-R224)
is the redefinition of what constitutes a "system" parachain.
Previously, a "system" parachain was identified with `para_id <= 1000`.
However, the
[PR#1406](paritytech/polkadot-sdk#1406) changed
a definition, considering a "system" parachain to have `para_id <=
1999`.

One consequence of this change is the reconsideration of the
[`ChildSystemParachainAsSuperuser`](https://github.com/paritytech/polkadot-sdk/blob/a50e6ba7af50a4be9ae78ebb90e86a61f3dd85e1/polkadot/xcm/xcm-builder/src/origin_conversion.rs#L72-L88)
feature. This feature relies on the
[`is_system`](https://github.com/paritytech/polkadot-sdk/blob/a50e6ba7af50a4be9ae78ebb90e86a61f3dd85e1/polkadot/parachain/src/primitives.rs#L216-L226)
function, which, in turn, depends on the para_id of a parachain to
determine "system" status. However, further examination has revealed
that there is no known scenario in which a system parachain would need
to call the Kusama runtime with `RuntimeOrigin::root()` privileges.
Consequently, maintaining the `ChildSystemParachainAsSuperuser` feature
in the Kusama runtime is deemed unnecessary.

## References

- [GitHub Pull Request
#1406](paritytech/polkadot-sdk#1406)
- [GitHub Pull Request
#2385](paritytech/polkadot-sdk#2385)
- Redo of #32 on main.

---------

Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: Bastian Köcher <info@kchr.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gilescope gilescope gilescope approved these changes

@ordian ordian ordian approved these changes

@franciscoaguirre franciscoaguirre franciscoaguirre approved these changes

@kianenigma kianenigma kianenigma approved these changes

@joepetrowski joepetrowski joepetrowski approved these changes

@acatangiu acatangiu acatangiu approved these changes

Assignees
No one assigned
Labels
T8-polkadot This PR/Issue is related to/affects the Polkadot network. T9-cumulus This PR/Issue is related to cumulus.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@bkontur @joepetrowski @Polkadot-Forum @gilescope @ordian @franciscoaguirre @kianenigma @acatangiu