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

Add support for versioned notification for HRMP pallet #4281

Merged
merged 25 commits into from
May 7, 2024

Conversation

bkontur
Copy link
Contributor

@bkontur bkontur commented Apr 25, 2024

Closes: #4003 (please see for the problem description)

TODO

Questions / possible improvements

  • A WrapVersion implementation for pallet_xcm initiates version discovery with note_unknown_version, 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;
     ..
     }
    

@bkontur bkontur added the T6-XCM This PR/Issue is related to XCM. label Apr 25, 2024
@bkontur bkontur self-assigned this Apr 25, 2024
@bkontur bkontur marked this pull request as ready for review April 25, 2024 14:42
@bkontur bkontur requested a review from a team as a code owner April 29, 2024 08:01
@bkontur
Copy link
Contributor Author

bkontur commented Apr 29, 2024

bot help

@bkontur
Copy link
Contributor Author

bkontur commented Apr 29, 2024

bot bench polkadot-pallet --runtime=rococo --pallet=runtime_parachains::hrmp
bot bench polkadot-pallet --runtime=westend --pallet=runtime_parachains::hrmp

bkontur and others added 3 commits April 29, 2024 10:13
…=westend --target_dir=polkadot --pallet=runtime_parachains::hrmp
@bkontur
Copy link
Contributor Author

bkontur commented Apr 29, 2024

bot bench polkadot-pallet --runtime=rococo --pallet=runtime_parachains::hrmp

@bkontur
Copy link
Contributor Author

bkontur commented Apr 29, 2024

bot clean

@bkontur
Copy link
Contributor Author

bkontur commented May 1, 2024

bot bench polkadot-pallet --runtime=rococo --pallet=runtime_parachains::hrmp
bot bench polkadot-pallet --runtime=westend --pallet=runtime_parachains::hrmp

@command-bot
Copy link

command-bot bot commented May 1, 2024

@bkontur https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6106217 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=rococo --target_dir=polkadot --pallet=runtime_parachains::hrmp. Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 5-6e0df268-bf72-4b9f-b749-942009368634 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented May 1, 2024

@bkontur https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6106218 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=westend --target_dir=polkadot --pallet=runtime_parachains::hrmp. Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 6-9b096c89-e1f6-4a2c-8a68-ff505215facb to cancel this command or bot cancel to cancel all commands in this pull request.

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 2/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6106085

…=rococo --target_dir=polkadot --pallet=runtime_parachains::hrmp
@command-bot
Copy link

command-bot bot commented May 1, 2024

@bkontur Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=rococo --target_dir=polkadot --pallet=runtime_parachains::hrmp has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6106217 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6106217/artifacts/download.

…=westend --target_dir=polkadot --pallet=runtime_parachains::hrmp
@command-bot
Copy link

command-bot bot commented May 1, 2024

@bkontur Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=westend --target_dir=polkadot --pallet=runtime_parachains::hrmp has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6106218 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6106218/artifacts/download.

polkadot/runtime/parachains/src/hrmp.rs Outdated Show resolved Hide resolved
polkadot/runtime/parachains/src/hrmp.rs Outdated Show resolved Hide resolved
polkadot/runtime/parachains/src/hrmp.rs Outdated Show resolved Hide resolved
polkadot/runtime/parachains/src/hrmp.rs Outdated Show resolved Hide resolved
@bkchr bkchr requested a review from franciscoaguirre May 2, 2024 20:35
prdoc/pr_4281.prdoc Outdated Show resolved Hide resolved
which controls the encoding of XCM notifications related to the opening/closing of HRMP channels.
If your runtime does not concern itself with the XCM version used for notifications,
you can set it as `type VersionWrapper = ()` to always use the latest XCM.
If your runtime does care about the XCM version when sending to child parachains,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if your runtime is a parachain you just put (), right?
What does it mean to concern oneself about versions? Is it allowing older versions of XCM?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if your runtime is a parachain you just put (), right?

The HRMP pallet is dedicated to relay chain runtimes, I don't think that any parachain runtime uses this HRMP pallet.

What does it mean to concern oneself about versions? Is it allowing older versions of XCM?

The problem could arise from a higher version on the relay chain than on the receiving parachain. For example, imagine that we update Polkadot with xcm::v5, but Polimec still uses xcm::v4. Without this fix, they could lose messages because they would not be able to decode xcm::v5.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it could be possible to replace most of the functionality related to the WrapVersion with just the SendXcm trait. In this scenario, the HRMP pallet would use type XcmSender = ChildParachainRouter, potentially simplifying things. However, this approach would introduce another layer of complexity, particularly regarding handling transport fees. Nevertheless, I think these HRMP notifications are treated as system notifications, which is why they are sent directly by dmp::Pallet::<T>::queue_downward_message.

@bkontur bkontur enabled auto-merge May 6, 2024 07:30
@bkontur bkontur added this pull request to the merge queue May 7, 2024
Merged via the queue into master with commit b709dcc May 7, 2024
144 of 147 checks passed
@bkontur bkontur deleted the bko-hrmp-fix-xcm-version-4003 branch May 7, 2024 08:55
hitchhooker pushed a commit to ibp-network/polkadot-sdk that referenced this pull request 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 <[email protected]>
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request 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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T6-XCM This PR/Issue is related to XCM.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

HRMP notifications from relay chain should use WrapVersion instead of latest VersionedXcm
5 participants