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

HRMP notifications from relay chain should use WrapVersion instead of latest VersionedXcm #4003

Closed
bkontur opened this issue Apr 5, 2024 · 0 comments · Fixed by #4281
Closed
Assignees

Comments

@bkontur
Copy link
Contributor

bkontur commented Apr 5, 2024

The HRMP channel are handled on the relay chain and the relay chain sends notifications to the child parachains:

The potential problem is using VersionedXcm::from(Xcm which uses xcm::latest::* stuff. E.g. when the relay chain is migrated to the higher XCM version than the parachains, it could lead to the undecodable and ProcessMessageError::Corrupt when processing message on the parachain here.

Everywhere where we send XCM (ChildParachainRouter, SiblingParachainRouter, ...) we use WrapVersion which uses implementation from pallet_xcm (SupportedVersion for destination or safeXcmVersion of runtime) to avoid using latest XCM version.

So the solution here is to add WrapVersion to the HRMP pallet Config and use this wrap_version for all notifications mentioned above. The result will be that we VersionedXcm would be at least safeXcmVersion or the previously stored on in SupportedVersion (if parachain communicated before with the relay chain).


Note: We added HRMP notification handler just recently here: #3696, but there could already exist custom implementations of XcmExecutor, so until this fix comes live, for them the hotfix would be to upgrade their XCM version as soon as possible according to the relay chain version (there is XCMv4 coming to the Kusama in few days).

cc: @JuaniRios

github-merge-queue bot pushed a commit that referenced this issue May 7, 2024
Closes: #4003 (please
see for the problem description)

## TODO
- [x] add more tests covering `WrapVersion` corner cases (e.g. para has
lower version, ...)
- [x] regenerate benchmarks `runtime_parachains::hrmp` (fix for Rococo
is here: #4332)

## Questions / possible improvements
- [ ] A `WrapVersion` implementation for `pallet_xcm` initiates version
discovery with
[note_unknown_version](https://github.com/paritytech/polkadot-sdk/blob/master/polkadot/xcm/pallet-xcm/src/lib.rs#L2527C5-L2527C25),
there is possibility to avoid this overhead in this HRMP case to create
new `WrapVersion` adapter for `pallet_xcm` which would not use
`note_unknown_version`. Is it worth to do it or not?
- [ ] There's a possibility to decouple XCM functionality from the HRMP
pallet, allowing any relay chain to generate its own notifications. This
approach wouldn't restrict notifications solely to the XCM. However,
it's uncertain whether it's worthwhile or desirable to do so? It means
making HRMP pallet more generic. E.g. hiding HRMP notifications behind
some trait:
	```
	trait HrmpNotifications {

		fn on_channel_open_request(
			sender: ParaId,
			proposed_max_capacity: u32,
			proposed_max_message_size: u32) -> primitives::DownwardMessage;

fn on_channel_accepted(recipient: ParaId) ->
primitives::DownwardMessage;

fn on_channel_closing(initiator: ParaId, sender: ParaId, recipient:
ParaId) -> primitives::DownwardMessage;
	}
	```
and then we could have whatever adapter, `impl HrmpNotifications for
VersionedXcmHrmpNotifications {...}`,
	```
	impl parachains_hrmp::Config for Runtime {
	..
		type HrmpNotifications = VersionedXcmHrmpNotifications;
	..
	}
	```

---------

Co-authored-by: command-bot <>
Co-authored-by: Bastian Köcher <git@kchr.de>
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 a pull request may close this issue.

1 participant