AcalaNetwork / chopsticks

Create parallel reality of your Substrate network.
Apache License 2.0
138 stars 84 forks source link

Fix `HRMP` messages delivery for unregistered channels #688

Open NachoPal opened 8 months ago

NachoPal commented 8 months ago

This PR aims to fix HRMP messages delivery for unregistered HRMP channels. Previous solution of opening channels on the fly did not really work, as it required at least one of the Parachains with an open EgressChannel.

Now, HRMP channels are expected to be opened in the Relay Chain.

Additionally, there are also a couple of improvements foreseen in upcoming runtime changes:

ermalkaleci commented 8 months ago

Thank you for this PR but unfortunately this will break a few things. I made validation data to inject any para id coming from hrmp without relaychain which mean you can open the channel just by submitting and empty message parachain.submitHorizontalMessages(paraId, []). It's very easy implementation. I will like to drop this PR and address this differently.

NachoPal commented 8 months ago

It's very easy implementation. I will like to drop this PR and address this differently.

Ok, but notice that there are other two changes in this PR that would be still applicable?

Can I revert HRMP changes and keep those two?

Can you share what approach you will follow to address the issue?

ermalkaleci commented 8 months ago
  • Fixing slot calculation for genesis with async backing

can you do another PR for this?

ermalkaleci commented 8 months ago
  • Producing an extra block for DMP messages in case MessageQueue is used

I don't why need to build block

NachoPal commented 8 months ago
  • Producing an extra block for DMP messages in case MessageQueue is used

I don't why need to build block

Message is processed in the next block after being received. Without building an extra block I can not assert the expected events.

ermalkaleci commented 8 months ago

but we already listen to downwardMessageQueues and trigger block building but if you start the chain with block-build-mode: manual then you have to manually build the block

NachoPal commented 8 months ago

but we already listen to downwardMessageQueues and trigger block building but if you start the chain with block-build-mode: manual then you have to manually build the block

Not sure if I understand, I think we are talking about different things. Soon, AssetHub runtimes will be using MessageQueue to handle received dmp messages. With that implementation, messages are actually processed in the next block they are included in the queue. If a new block is not automatically produced, the expected events coming from the XCM execution itself will not be emitted, and I wouldn't like to have to manually build another block. Ideally it should be an automated process.

I am already using for my tests another runtime that implements MessageQueue for dmp, therefore I would like to see this implemented in chopsticks.

ermalkaleci commented 8 months ago

Thanks, I wasn't aware. In that case we'll need to proper upgrade to MessageQueue with fallback to DMP

ermalkaleci commented 8 months ago

if you can upload latest assethub genesis it will help me a lot

ermalkaleci commented 8 months ago

Soon, AssetHub runtimes will be using MessageQueue to handle received dmp messages.

I don't think know if we need any change yet. We don't use DmpQueue so this doesn't affect us. I will check tomorrow if we need to upgrade anything

ermalkaleci commented 8 months ago

@NachoPal do you any example that doesn't work so I can look at?

ermalkaleci commented 8 months ago

I will check tomorrow if we need to upgrade anything

we don't need to change anything. parachain messages are injected into validation data

xlc commented 8 months ago

this is the reason the messageQeueue needs an extra block https://github.com/paritytech/polkadot-sdk/issues/3709 basically messageQueue is broken and we need to deal with the consequences

ermalkaleci commented 3 months ago

This is already fixed and can be closed