Setup HrmpChannelAccepted handler with pallet_xcm for initiating XCM version discovery when openning HRMP channel

[bkontur]

Closes: #922
Implements: #3692 (see: Proposed solution part 2 - initiate VersionDiscoveryQueue when HRMP is accepted).

When we utilize a parachain's pallet_xcm extrinsics or other functionality that employs XcmpQueue as an XcmRouter, there is a possibility of encountering a situation where it is the first message to the desired destination, and we do not know a supported XCM version of the destination. In such cases, pallet_xcm includes a feature called "version discovery," which is initiated. This is a one-time operation. Therefore, this fix aims to initiate "version discovery" as soon as possible when an HRMP channel is created, with best effort.

In other words, when parachains establish an HRMP channel, we immediately initiate XCM version discovery and don't wait for the very first user request.

This PR:

• introduces AllowHrmpNotificationsFromRelayChain barrier for allowing HRMP notifications just from the relay chain (to fulfill safety assumptions - see)
• adds the implementation of HandleHrmpChannelAccepted for pallet_xcm, so pallet_xcm instance can be used for xcm_executor::Config::HrmpChannelAcceptedHandler(https://github.com/paritytech/polkadot-sdk/blob/master/polkadot/xcm/xcm-executor/src/config.rs#L121-L122)
• sets HandleHrmpChannelAccepted for all cumulus/testnets

[svyatonik]

Just curious - I though that UniversalLocation is assumed to be started with a GlobalConsensus. Then why would we need that match here? A defensive programming or something else?

[bkontur]

Just curious - I though that UniversalLocation is assumed to be started with a GlobalConsensus. Then why would we need that match here? A defensive programming or something else?

Yes, good point, exactly - defensive and generic.

Originally, I wanted to use just let dest = ParentThen(Parachain(recipient).into()).into();, but yeah there is a Parent which is also not that generic.

Yes, there is that thing with GlobalConsensus you mentioned, which I guess it's kind of unwritten rule or assumption that runtime's UniversalLocation starts with GlobalConsensus - I don't remember exactly but without GlobalConsensus something fails in XcmExecutor (maybe just ExportMessage or something with WithComputedOrigin...)

The Hrmp notifications have just paraId: u32 and we need to convert it to xcm::Location, so I was thinking this way:
*Parachain(recipient)*.relative_to(&T::UniversalLocation::get());.

        #[test]
	fn test() {
		// E.g. we are on AssetHub
		frame_support::parameter_types! {
			pub UniversalLocation: InteriorLocation = [GlobalConsensus(Rococo), Parachain(1000)].into();
		}

		// We opened channel with 1003
		let recipient = 1003;

		// convert 1003 from the point of view of `UniversalLocation`
		let dest = match UniversalLocation::get().global_consensus() {
			Ok(global_consensus) =>
				Junctions::from([GlobalConsensus(global_consensus), Parachain(recipient)]),
			Err(_) => Junctions::from([Parachain(recipient)]),
		}.relative_to(&UniversalLocation::get());

		// expected result, how AssetHub sees sibling parachain
		assert_eq!(dest, Location { parents: 1, interior: Junctions::X1([Parachain(1003)].into()) });
	}

And when I was checking other runtimes, I saw that we have some minimal template without GlobalConsensus: https://github.com/paritytech/polkadot-sdk/blob/master/templates/parachain/runtime/src/configs/xcm_config.rs#L29, so thats why I ended up with that match.

@svyatonik Do you have any other idea how to handle a conversion from paraId: u32 to the Location with T::UniversalLocation? I was thinking about some adapter with which would wrap Convert<u32, Location>, Self::get_version_for with Self::note_unknown_version, but it could just complicate things and not sure if it is worth.

[acatangiu]

I though that UniversalLocation is assumed to be started with a GlobalConsensus.

Yes, this is ideally where we should get, but not all parachains have done this yet..

Everything except bridging works fine without it too, so there is also no big incentive to do it.

[acatangiu]

And when I was checking other runtimes, I saw that we have some minimal template without GlobalConsensus: https://github.com/paritytech/polkadot-sdk/blob/master/templates/parachain/runtime/src/configs/xcm_config.rs#L29

We should sanitize whole repo and make sure all our runtimes test, mock, etc have the GlobalConsensus too so we create a consistent best-practice.

[svyatonik]

Originally, I wanted to use just let dest = ParentThen(Parachain(recipient).into()).into();, but yeah there is a Parent which is also not that generic.

But OTOH we know that this notification comes from the relay chain (because it notifies us about new HRMP channels), right? So maybe Parent would be enough here :) Not sure if it'll be changed

And when I was checking other runtimes, I saw that we have some minimal template without GlobalConsensus: https://github.com/paritytech/polkadot-sdk/blob/master/templates/parachain/runtime/src/configs/xcm_config.rs#L29, so thats why I ended up with that match.

Maybe we should fix that in this case. E.g. some bridging adapters (that's where I got this assumption - e.g. UnpaidRemoteExporter) are assuming that UniversalLocation starts with GlobalConsensus. But that's not related to this PR ofc.

Thank you for the explanation - I've just been curios about that && I wasn't planning to request changes or smth like that :)

[bkontur]

And when I was checking other runtimes, I saw that we have some minimal template without GlobalConsensus: https://github.com/paritytech/polkadot-sdk/blob/master/templates/parachain/runtime/src/configs/xcm_config.rs#L29

We should sanitize whole repo and make sure all our runtimes test, mock, etc have the GlobalConsensus too so we create a consistent best-practice.

Good point, attempt to sanitize (where possible): 9c5a918

[acatangiu]

@bkontur is this good to merge?

[bkontur]

@bkontur is this good to merge?

not yet, I want to check comments and add also closing part, where we clean storage for unneded stuff

[paritytech-cicd-pr]

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5926429

[bkontur]

• investigating one more test if this is enough and aligned with process_hrmp_open_channel_requests / initializer_on_new_session

[bkontur]

Changing to the draft, because HrmpNotificationAccepted won't work and need to find another solution: #922 (comment)

Opened another PR: #4156 which contains still relevant changes but with reverted HrmpNotificationAccepted handler stuff.