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

Permisionless way to create HRMP channels between system parachains and other parachains #82

Closed
xlc opened this issue Mar 10, 2024 · 19 comments

Comments

@xlc
Copy link
Contributor

xlc commented Mar 10, 2024

Almost all parachains will want to connect to one or more system parachains for various reasons and system parachains are created to offer functionalities to relaychain and other parachains.

However, a HRMP channel is required before a non-system parachain to be able to communicate with system parachain and it requires a governance proposal for that to happen. This is slow, high overhead, and unnecessary.

We should develop a pallet for system parachains to allow other parachains to permisionlessly create bidirectional HRMP channels with it. There maybe some deposit requirements etc to ensure security.

@xlc
Copy link
Contributor Author

xlc commented Mar 10, 2024

Such pallet could also fix paritytech/polkadot-sdk#922, which is the root cause of polkadot-fellows/runtimes#190

@joepetrowski
Copy link
Contributor

Why do we need a whole new pallet for this? Couldn't we add one new call, similar to establish_system_channel, that a parachain would call?

So:

pub fn establish_channel_with_system(
	origin: OriginFor<T>,
	interlocutor: ParaId,
) -> DispatchResult {
	let origin = ensure_parachain(<T as Config>::RuntimeOrigin::from(origin))?;
	ensure!(interlocutor.is_system(), Error::<T>::ChannelCreationNotAuthorized);
	// ...
	// establish bidirectional channel
	// ...
}

@xlc
Copy link
Contributor Author

xlc commented Mar 10, 2024

so it can be reused by non system parachains and I want to fix paritytech/polkadot-sdk#922 at the same time

@joepetrowski
Copy link
Contributor

But like you said in that issue, that just requires implementation of the Hrmp* instructions.

@xlc
Copy link
Contributor Author

xlc commented Mar 10, 2024

yeah but the implementation have to be lived somewhere? also for security reasons we may want to config things like max channel count or deposit requirements etc. what is your suggestion?

@joepetrowski
Copy link
Contributor

yeah but the implementation have to be lived somewhere?

The same place that implements the other instructions... It's just an implementation of the ExecuteXcm trait.

also for security reasons we may want to config things like max channel count or deposit requirements

We already have a Configuration pallet that contains these.

@xlc
Copy link
Contributor Author

xlc commented Mar 10, 2024

I don’t really get what are you suggesting. Yeah maybe not a new pallet but where do you suggest to put those code?
And maybe it doesn’t need to be a new pallet but why it can’t be a new pallet?

@joepetrowski
Copy link
Contributor

Where it already is.

@xlc
Copy link
Contributor Author

xlc commented Mar 10, 2024

maybe we need to touch this file but I still think we need a new pallet. eg to allow configure the parameters via governance proposal

and forcing more non optional parameters to already super complicated xcm configurations just sounds bad

anyway, those are technical details and not exactly something we need to discuss before a draft is created.

my high level questions are:

should we do this? sounds like a yes from Joe
do we need to have some limits and what are the limits for the system parachains?
what are the hrmp channel parameters that should be used or do we want to not hardcoded it?
any security risks or other concerns?
to confirm we need a rfc for this

@bkchr
Copy link
Contributor

bkchr commented Mar 10, 2024

We should develop a pallet for system parachains to allow other parachains to permisionlessly create bidirectional HRMP channels with it

With on-demand parachains this isn't such a great idea. I mean with the current implementation of XCMP it isn't a problem. However, if we ever go to offchain-xcmp, this will lead to problems.

@xlc
Copy link
Contributor Author

xlc commented Mar 10, 2024

that’s exactly the reason why I am asking do we need to have some limits and what are the limits for the system parachains?
permisionless does not mean no requirement at all. we can still ask for deposit, time delay, etc.

I think we absolutely need the system parachain to be able to communicate with every long live parachain / whatever coretime software that wants to purchase coretime directly via xcm. Otherwise we have a problem to fix and in that case we identified a problem and we need to then find out a solution to it.

I don’t exactly know all the details and limitations about offchain-xcmp but if it’s feature set does not cover everything we have currently, we need either alter the design to make it more capable, or in the case that’s not feasible, keep the hrmp mechanism.

@JuaniRios
Copy link

We had to fork the executor to have an implementation of the HRMP instructions at Polimec.

All we did is create a config type on the executor so we can pass whatever logic we want to handle the HRMP instructions in the construct_runtime.

@joepetrowski, why can't the original xcm executor have something like this instead of returning an error? This way we don't have to keep an up-to-date fork of the executor for only two lines of code. It's generic enough to not cause any complications, and if any parachain wants to avoid using those instructions, they can simply pass an empty implementation.

@joepetrowski
Copy link
Contributor

@joepetrowski, why can't the original xcm executor have something like this instead of returning an error? This way we don't have to keep an up-to-date fork of the executor for only two lines of code. It's generic enough to not cause any complications, and if any parachain wants to avoid using those instructions, they can simply pass an empty implementation.

Yeah this is what I'm arguing for in this thread...

@acatangiu
Copy link
Contributor

We had to fork the executor to have an implementation of the HRMP instructions at Polimec.

All we did is create a config type on the executor so we can pass whatever logic we want to handle the HRMP instructions in the construct_runtime.

@joepetrowski, why can't the original xcm executor have something like this instead of returning an error? This way we don't have to keep an up-to-date fork of the executor for only two lines of code. It's generic enough to not cause any complications, and if any parachain wants to avoid using those instructions, they can simply pass an empty implementation.

Why not try to upstream it then?

I believe it was always the plan to have an implementation for that instruction, but there's always more things to do than time to do them. I would welcome a PR that gets it done 🚀

@bkchr
Copy link
Contributor

bkchr commented Mar 11, 2024

After writing this comment, I thought more about this. Actually my comment isn't that correct. As long as the system chains are building every 24 hours a block, it should be quite easy for them to get hold of the messages from someone else. So, we can probably ignore what I said.

This brings me back to some other thought I had, based on this comment from you:

are created to offer functionalities to relaychain and other parachains.

Maybe we should open these channels automatically between all chains and the system chains. Maybe at the beginning using some low capacity, which could be increased by providing some deposit.

@joepetrowski
Copy link
Contributor

Maybe we should open these channels automatically between all chains and the system chains. Maybe at the beginning using some low capacity, which could be increased by providing some deposit.

Yeah, so we could make a call like this that would let any para open a channel with any system chain, but with a default low-capacity configuration. Then if a chain wants a higher capacity channel, they can use the force_open_hrmp_channel call that goes through governance.

@xlc
Copy link
Contributor Author

xlc commented Mar 11, 2024

ok. that will work.
next question: do we need rfc for this? I should have everything I needed to start implement this.

@bkontur
Copy link

bkontur commented Mar 14, 2024

We had to fork the executor to have an implementation of the HRMP instructions at Polimec.

All we did is create a config type on the executor so we can pass whatever logic we want to handle the HRMP instructions in the construct_runtime.

@joepetrowski, why can't the original xcm executor have something like this instead of returning an error? This way we don't have to keep an up-to-date fork of the executor for only two lines of code. It's generic enough to not cause any complications, and if any parachain wants to avoid using those instructions, they can simply pass an empty implementation.

I initiated this issue paritytech/polkadot-sdk#3692 for different reasons just a few hours ago, and @acatangiu directed me to this RFC issue. Now, I also see @xlc's comment paritytech/polkadot-sdk#922 (comment). It seems like everything revolves around the same issue - adding a callback handler for HrmpNewChannelOpenRequest / HrmpChannelAccepted / HrmpChannelClosing XCM instructions to the XcmExecutor.

@JuaniRios, as Adrian mentioned, you could open a PR to the polkadot-sdk repo, and I could assist with its completion. This way, you can eliminate your fork. I checked your use-case, and it appears to be different from mine and @xlc's regarding the VersionDiscoveryQueue. However, with tuple implementation for HrmpHandler, we can support both and more use cases.

github-merge-queue bot pushed a commit to paritytech/polkadot-sdk that referenced this issue Apr 12, 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]>
github-merge-queue bot pushed a commit to paritytech/polkadot-sdk that referenced this issue Apr 12, 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]>
github-merge-queue bot pushed a commit to paritytech/polkadot-sdk that referenced this issue Apr 12, 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]>
github-merge-queue bot pushed a commit to paritytech/polkadot-sdk that referenced this issue Apr 12, 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]>
github-merge-queue bot pushed a commit to paritytech/polkadot-sdk that referenced this issue Apr 12, 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]>
@xlc
Copy link
Contributor Author

xlc commented Apr 14, 2024

paritytech/polkadot-sdk#3721 allow any parachain to have HRMP channel with a system parachain

@xlc xlc closed this as completed Apr 14, 2024
EgorPopelyaev pushed a commit to paritytech/polkadot-sdk that referenced this issue 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 issue 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
None yet
Projects
None yet
Development

No branches or pull requests

6 participants