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

Fix HRMP messages delivery for unregistered channels #688

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

NachoPal
Copy link

@NachoPal NachoPal commented Mar 9, 2024

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:

  • Fixing slot calculation for genesis with async backing
  • Producing an extra block for DMP messages in case MessageQueue is used

@NachoPal NachoPal changed the title Fix HRMP for unregistered channels Fix HRMP messages delivery for unregistered channels Mar 9, 2024
@ermalkaleci
Copy link
Collaborator

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
Copy link
Author

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?

  • Fixing slot calculation for genesis with async backing
  • Producing an extra block for DMP messages in case MessageQueue is used

Can I revert HRMP changes and keep those two?

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

@ermalkaleci
Copy link
Collaborator

  • Fixing slot calculation for genesis with async backing

can you do another PR for this?

@ermalkaleci
Copy link
Collaborator

  • Producing an extra block for DMP messages in case MessageQueue is used

I don't why need to build block

@NachoPal
Copy link
Author

  • 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
Copy link
Collaborator

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
Copy link
Author

NachoPal commented Mar 11, 2024

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
Copy link
Collaborator

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

@ermalkaleci
Copy link
Collaborator

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

@ermalkaleci
Copy link
Collaborator

ermalkaleci commented Mar 11, 2024

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
Copy link
Collaborator

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

@ermalkaleci
Copy link
Collaborator

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
Copy link
Member

xlc commented Mar 15, 2024

this is the reason the messageQeueue needs an extra block paritytech/polkadot-sdk#3709
basically messageQueue is broken and we need to deal with the consequences

@ermalkaleci
Copy link
Collaborator

This is already fixed and can be closed

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

Successfully merging this pull request may close these issues.

3 participants