HRMP notifications from relay chain should use WrapVersion instead of latest VersionedXcm
#4003
Assignees
Comments
4 tasks
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>
hitchhooker
pushed a commit
to ibp-network/polkadot-sdk
that referenced
this issue
Jun 5, 2024
Closes: paritytech#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: paritytech#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>
TarekkMA
pushed a commit
to moonbeam-foundation/polkadot-sdk
that referenced
this issue
Aug 2, 2024
Closes: paritytech#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: paritytech#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
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(Xcmwhich usesxcm::latest::*stuff. E.g. when the relay chain is migrated to the higher XCM version than the parachains, it could lead to the undecodable andProcessMessageError::Corruptwhen processing message on the parachain here.Everywhere where we send XCM (ChildParachainRouter, SiblingParachainRouter, ...) we use
WrapVersionwhich uses implementation frompallet_xcm(SupportedVersionfor destination orsafeXcmVersionof runtime) to avoid using latest XCM version.So the solution here is to add
WrapVersionto the HRMP pallet Config and use thiswrap_versionfor all notifications mentioned above. The result will be that weVersionedXcmwould be at leastsafeXcmVersionor the previously stored on inSupportedVersion(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
The text was updated successfully, but these errors were encountered: