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

establish_channel_with_system #3721

Merged
merged 29 commits into from
Apr 12, 2024
Merged

Conversation

xlc
Copy link
Contributor

@xlc xlc commented Mar 17, 2024

Implements polkadot-fellows/RFCs#82
Allow any parachain to have bidirectional channel with any system parachains

Looking for initial feedbacks before continue finish it

TODOs:

  • docs
  • benchmarks
  • tests

polkadot/runtime/parachains/src/hrmp.rs Outdated Show resolved Hide resolved
polkadot/runtime/parachains/src/hrmp.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@joepetrowski joepetrowski left a comment

Choose a reason for hiding this comment

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

I think the approach is OK, just a few naming comments.

Comment on lines 273 to 275
/// The default channel size and capacity to use when opening a channel to a system
/// parachain.
type DefaultChannelSizeAndCapacityToSystem: Get<(u32, u32)>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add an integrity_test that these are below the maximum allowed by Configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

config is from storages so it is not really possible to assert it at build time.
if the value is above the max defined in config, the call will fail, and I think that's fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah right 👍

Copy link
Member

Choose a reason for hiding this comment

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

We do run integrity_test in externalities, so at least the genesis values can be checked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but why check with genesis values?

Copy link
Member

Choose a reason for hiding this comment

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

Because they should also be correct? but its not a must, just an idea.

polkadot/runtime/parachains/src/hrmp.rs Outdated Show resolved Hide resolved
polkadot/runtime/parachains/src/hrmp.rs Outdated Show resolved Hide resolved
polkadot/runtime/parachains/src/hrmp.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

approach looks good, will approve once PR is final (todos, tests, benchmarks, etc)

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

LGTM

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5654892

// Measured: `417`
// Estimated: `6357`
// Minimum execution time: 629_674_000 picoseconds.
Weight::from_parts(640_174_000, 0)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

copied from establish_system_channel

@joepetrowski joepetrowski added T6-XCM This PR/Issue is related to XCM. T14-system_parachains This PR/Issue is related to system parachains. labels Mar 27, 2024
@xlc
Copy link
Contributor Author

xlc commented Apr 3, 2024

not sure why PRDoc check failed?

@acatangiu
Copy link
Contributor

not sure why PRDoc check failed?

You need to specify what kind of bump should the affected crates get:

bump: minor

prdoc/pr_3721.prdoc Outdated Show resolved Hide resolved
@xlc xlc requested a review from a team as a code owner April 12, 2024 05:22
substrate/frame/support/src/traits/randomness.rs Outdated Show resolved Hide resolved
polkadot/runtime/parachains/src/hrmp/benchmarking.rs Outdated Show resolved Hide resolved
polkadot/runtime/parachains/src/hrmp/benchmarking.rs Outdated Show resolved Hide resolved
polkadot/runtime/parachains/src/hrmp/benchmarking.rs Outdated Show resolved Hide resolved
polkadot/runtime/parachains/src/hrmp/benchmarking.rs Outdated Show resolved Hide resolved
@joepetrowski joepetrowski enabled auto-merge April 12, 2024 05:40
@joepetrowski joepetrowski added this pull request to the merge queue Apr 12, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Apr 12, 2024
@acatangiu acatangiu added this pull request to the merge queue Apr 12, 2024
Merged via the queue into paritytech:master with commit b1db5f3 Apr 12, 2024
129 of 136 checks passed
@xlc xlc deleted the hrml_with_system branch April 14, 2024 01:57
EgorPopelyaev pushed a commit that referenced this pull request May 24, 2024
Implements polkadot-fellows/RFCs#82
Allow any parachain to have bidirectional channel with any system
parachains

Looking for initial feedbacks before continue finish it

TODOs:
- [x] docs
- [x] benchmarks
- [x] tests

---------

Co-authored-by: joe petrowski <[email protected]>
Co-authored-by: Adrian Catangiu <[email protected]>
fellowship-merge-bot bot pushed a commit to polkadot-fellows/runtimes that referenced this pull request May 29, 2024
Allows any parachain to have bidirectional channel with any system
parachains.

Relates to: polkadot-fellows/RFCs#82
Relates to: paritytech/polkadot-sdk#3721
Relates to: paritytech/polkadot-sdk#4332

<!-- Remember that you can run `/merge` to enable auto-merge in the PR
-->

<!-- Remember to modify the changelog. If you don't need to modify it,
you can check the following box.
Instead, if you have already modified it, simply delete the following
line. -->

- [x] regenerate weights for `runtime_parachains::hrmp`
- [ ] Does not require a CHANGELOG entry

## Polkadot weights
(the threshold moved from the default 5 to 0.1 to see some differences)
```
subweight compare commits          --path-pattern "./relay/polkadot/**/weights/**/*.rs"          --format markdown --no-color           --change added changed          --method asymptotic --ignore-errors --threshold 0.1          remotes/polkadot-fellows/main          origin/bko-hrmp-patch
```
| File | Extrinsic | Old | New | Change [%] |

|-------------------------------------------------------|-------------------------------|----------|----------|------------|
| relay/polkadot/src/weights/runtime_parachains_hrmp.rs |
poke_channel_deposits | 137.20us | 140.03us | +2.06 |
| relay/polkadot/src/weights/runtime_parachains_hrmp.rs |
hrmp_accept_open_channel | 555.89us | 563.03us | +1.28 |
| relay/polkadot/src/weights/runtime_parachains_hrmp.rs |
hrmp_close_channel | 556.97us | 563.79us | +1.22 |
| relay/polkadot/src/weights/runtime_parachains_hrmp.rs |
hrmp_init_open_channel | 732.03us | 740.10us | +1.10 |
| relay/polkadot/src/weights/runtime_parachains_hrmp.rs |
force_open_hrmp_channel | 1.16ms | 1.17ms | +0.98 |
| relay/polkadot/src/weights/runtime_parachains_hrmp.rs |
establish_system_channel | 1.15ms | 1.16ms | +0.69 |
| relay/polkadot/src/weights/runtime_parachains_hrmp.rs |
force_process_hrmp_open | 102.05ms | 102.47ms | +0.42 |
| relay/polkadot/src/weights/runtime_parachains_hrmp.rs |
force_clean_hrmp | 91.52ms | 91.86ms | +0.37 |
| relay/polkadot/src/weights/runtime_parachains_hrmp.rs |
clean_open_channel_requests | 16.56ms | 16.61ms | +0.35 |
| relay/polkadot/src/weights/runtime_parachains_hrmp.rs |
force_process_hrmp_close | 75.41ms | 75.65ms | +0.33 |
| relay/polkadot/src/weights/runtime_parachains_hrmp.rs |
hrmp_cancel_open_request | 422.12us | 415.03us | -1.68 |
| relay/polkadot/src/weights/runtime_parachains_hrmp.rs |
establish_channel_with_system | | 1.67ms | Added |

## Kusama weights
(the threshold moved from the default 5 to 0.1 to see some differences)
```
subweight compare commits          --path-pattern "./relay/kusama/**/weights/**/*.rs"          --format markdown --no-color           --change added changed          --method asymptotic --ignore-errors --threshold 0.1          remotes/polkadot-fellows/main          origin/bko-hrmp-patch
```
| File | Extrinsic | Old | New | Change [%] |

|-----------------------------------------------------|-------------------------------|----------|----------|------------|
| relay/kusama/src/weights/runtime_parachains_hrmp.rs |
poke_channel_deposits | 137.23us | 140.25us | +2.20 |
| relay/kusama/src/weights/runtime_parachains_hrmp.rs |
hrmp_close_channel | 557.39us | 564.18us | +1.22 |
| relay/kusama/src/weights/runtime_parachains_hrmp.rs |
hrmp_accept_open_channel | 556.49us | 563.23us | +1.21 |
| relay/kusama/src/weights/runtime_parachains_hrmp.rs |
hrmp_init_open_channel | 735.26us | 743.59us | +1.13 |
| relay/kusama/src/weights/runtime_parachains_hrmp.rs |
force_open_hrmp_channel | 1.16ms | 1.17ms | +0.97 |
| relay/kusama/src/weights/runtime_parachains_hrmp.rs |
establish_system_channel | 1.15ms | 1.16ms | +0.84 |
| relay/kusama/src/weights/runtime_parachains_hrmp.rs |
hrmp_cancel_open_request | 413.79us | 416.03us | +0.54 |
| relay/kusama/src/weights/runtime_parachains_hrmp.rs |
force_process_hrmp_open | 101.96ms | 102.43ms | +0.46 |
| relay/kusama/src/weights/runtime_parachains_hrmp.rs |
clean_open_channel_requests | 16.54ms | 16.61ms | +0.45 |
| relay/kusama/src/weights/runtime_parachains_hrmp.rs | force_clean_hrmp
| 91.43ms | 91.82ms | +0.43 |
| relay/kusama/src/weights/runtime_parachains_hrmp.rs |
force_process_hrmp_close | 75.34ms | 75.63ms | +0.40 |
| relay/kusama/src/weights/runtime_parachains_hrmp.rs |
establish_channel_with_system | | 1.67ms | Added |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T6-XCM This PR/Issue is related to XCM. T14-system_parachains This PR/Issue is related to system parachains.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants