polkadot-fellows / RFCs

Proposals for change to standards administered by the Fellowship.
https://polkadot-fellows.github.io/RFCs/
Creative Commons Zero v1.0 Universal
109 stars 47 forks source link

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

Closed xlc closed 2 months ago

xlc commented 4 months ago

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 commented 4 months ago

Such pallet could also fix https://github.com/paritytech/polkadot-sdk/issues/922, which is the root cause of https://github.com/polkadot-fellows/runtimes/issues/190

joepetrowski commented 4 months ago

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 commented 4 months ago

so it can be reused by non system parachains and I want to fix https://github.com/paritytech/polkadot-sdk/issues/922 at the same time

joepetrowski commented 4 months ago

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

xlc commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

Where it already is.

xlc commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

bkontur commented 3 months ago

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 https://github.com/paritytech/polkadot-sdk/issues/3692 for different reasons just a few hours ago, and @acatangiu directed me to this RFC issue. Now, I also see @xlc's comment https://github.com/paritytech/polkadot-sdk/issues/922#issuecomment-1691891582. 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.

xlc commented 2 months ago

https://github.com/paritytech/polkadot-sdk/pull/3721 allow any parachain to have HRMP channel with a system parachain