Configure MessageQueue for compatibility with Snowbridge by vgeddes · Pull Request #2146 · paritytech/polkadot-sdk

vgeddes · 2023-11-03T10:54:23Z

Changes:

Add Export variant to AggregateMessageOrigin enum, to identify queues owned by the snowbridge-outbound-queue pallet.
Change registration of MessageQueue as the last pallet in BridgeHub runtimes, to ensure correct on_initialize order. The on_initialize handler of snowbridge-outbound-queue needs to run first.

Checklist:

Ensure tests and linters pass

* Add `Export` variant to AggregateMessageOrigin enum * Register MessageQueue as the last pallet in BridgeHub runtimes

ggwpez · 2023-11-05T12:38:16Z

cumulus/parachains/runtimes/bridge-hubs/bridge-hub-kusama/src/lib.rs

 		Utility: pallet_utility::{Pallet, Call, Event} = 40,
 		Multisig: pallet_multisig::{Pallet, Call, Storage, Event<T>} = 41,
+
+		// MessageQueue. Comes last so that its on_initialize handler runs last


Why is this important?

Its necessary as we need to install a pallet OutboundQueue, documented here, with an on_initialize handler that must run before any messages are consumed from MessageQueue.

This on_initialize handler kills storage (obsolete processed messages) from the previous block. For the new block, we only want to fill up storage with new processed messages sourced from MessageQueue.

ggwpez · 2023-11-05T12:42:53Z

cumulus/primitives/core/src/lib.rs

+}
+/// The origin of a message
+#[derive(Encode, Decode, MaxEncodedLen, Clone, Eq, PartialEq, TypeInfo, Debug)]
+pub enum MessageOrigin {


What is with the naming here? Just MessageOrigin is not very expressive.

ggwpez · 2023-11-05T12:43:35Z

cumulus/primitives/core/src/lib.rs

+	/// The message came from the relay-chain.
+	Parent,
+	/// The message came from a sibling para-chain.
+	Sibling(ParaId),


Why are these variants the same as in the AggregateMessageOrigin?

Our Ethereum bridge acts as an adaptor over XCM, essentially allowing parachains to send XCM messages to Ethereum.

On BridgeHub, to process these XCM messages, we need to track their origin, with the granularity shown in MessageQueue. And it just so happens that they are the same as AggregateMessageOrigin.

Messages from different Polkadot origins are treated differently, for example in this implementation of QueuePausedQuery: https://github.com/Snowfork/snowbridge/blob/c9196e196f650e1362dfdf7e911dc9b292cf0859/parachain/pallets/outbound-queue/src/queue_paused_query_impl.rs#L17

And that's why its difficult to reuse the old AggregateMessageOrigin, as our implementation of QueuePausedQuery would likely conflict with the Xcmp one.

ggwpez · 2023-11-05T12:44:35Z

cumulus/primitives/core/src/lib.rs

 	/// This is used by the HRMP queue.
 	Sibling(ParaId),
+	/// This is used by the snowbridge-outbound-queue pallet
+	Export(MessageOrigin),


I hope we can make this more general somehow. But just so that i understand, this is the outbound queue for bridged messages?
Will there also be an inbound?

I am not sure if we actually need to change this in the SDK, or if there could be an enum type in the runtime that has this AggregatedMessageOrigin as one variant and then a bunch of other custom variants for your snowbridge pallets.

I hope we can make this more general somehow.

I thought about this, came to the conclusion that a generalized solution wouldn't really help, as we can't predict the requirements (or even a need) for the generalized solution. So we could end designing something that will never be used by any projects other than Snowbridge.

But just so that i understand, this is the outbound queue for bridged messages?

Yes. Essentially it takes inbound XCM messages, processes them, and puts them back on the MessageQueue for eventual final processing and commitment. Full documentation here.

Will there also be an inbound?

Nope.

I am not sure if we actually need to change this in the SDK, or if there could be an enum type in the runtime that has this AggregatedMessageOrigin as one variant and then a bunch of other custom variants for your snowbridge pallets.

I kinda originally tried something like that in the other old draft PR. Though as you said, it would increase the complexity of runtime upgrades.

enum AggregateMessageOrigin { Xcmp(XcmpOrigin) Snowbridge(SnowbridgeOrigin) }

Can I see the snowbridge MR to the Polkadot-SDK please? To me it seems like you change this enum but we will never use it in the SDK.
In that case you can just change it in your snowbridge runtime.

Just to clarify, Snowbridge is a set of pallets which will be installed on these system runtimes:

bridge-hub-rococo

bridge-hub-kusama

bridge-hub-polkadot

So within those runtimes, the XCMP and Snowbridge subsystems will need to agree on a common AggregateMessageOrigin, unless I'm missing something.

For example here we install the pallets in the bridge-hub-rococo runtime: https://github.com/Snowfork/polkadot-sdk/blob/71197605d792abca283c01c5873bfe8e1dba83c8/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/lib.rs#L681-L684

Here we configure the MessageQueue to use our OutboundQueue as a message processer: https://github.com/Snowfork/polkadot-sdk/blob/71197605d792abca283c01c5873bfe8e1dba83c8/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/lib.rs#L493-L495

And here's our interim AggregateMessageOrigin:
https://github.com/Snowfork/snowbridge/blob/c9196e196f650e1362dfdf7e911dc9b292cf0859/parachain/primitives/core/src/outbound.rs#L364

Is hard to give you a clean MR right now, as the base branch for our working branch is a bit of frankenstein at the moment.

But then there should maybe be a uniform BridgeAggregateMessageOrigin across all bridge-hub runtimes. So it should be in a bridge-hub-common crate or something. It does not make sense to force this onto parachain that will never use it (like asset-hubs etc...).
Do you see what i mean or does it not make sense to you?

That makes very good sense, narrowing the change down to the BridgeHub level. Let me try that.

vgeddes · 2023-11-06T11:29:39Z

cumulus/primitives/core/src/lib.rs

 	fn from(origin: AggregateMessageOrigin) -> Self {
+		use AggregateMessageOrigin::*;
 		match origin {
-			AggregateMessageOrigin::Here => MultiLocation::here(),


If we design the AggregateMessageOrigin to look like this:

enum AggregateMessageOrigin { Xcmp(XcmpOrigin) Snowbridge(SnowbridgeOrigin) }

Then this change would be less hacky, as we would only need to implement From<XcmpOrigin> for Multilocation.

vgeddes · 2023-11-06T16:40:23Z

Closing off. Will open a new PR with the suggested changes.

MessageQueue compatibility for upcoming Snowbridge merge

db15e36

* Add `Export` variant to AggregateMessageOrigin enum * Register MessageQueue as the last pallet in BridgeHub runtimes

paritytech-review-bot bot requested review from a team November 3, 2023 10:55

Fix index

05ba898

vgeddes mentioned this pull request Nov 3, 2023

RFC: Reorganise AggregateMessageOrigin to support Snowbridge #2117

Closed

vgeddes added 2 commits November 3, 2023 14:52

Merge branch 'master' into message-queue-origins

1906cbb

rustfmt

ed303d0

ggwpez reviewed Nov 5, 2023

View reviewed changes

vgeddes commented Nov 6, 2023

View reviewed changes

vgeddes closed this Nov 6, 2023

This was referenced Nov 8, 2023

Add commons crate for BridgeHub runtimes [do not merge] Snowfork/polkadot-sdk#22

Closed

[Cumulus] Add GeneralKey variant to AggregateMessageOrigin #2338

Closed

Conversation

vgeddes commented Nov 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ggwpez Nov 5, 2023

Choose a reason for hiding this comment

Uh oh!

vgeddes Nov 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggwpez Nov 5, 2023

Choose a reason for hiding this comment

Uh oh!

ggwpez Nov 5, 2023

Choose a reason for hiding this comment

Uh oh!

vgeddes Nov 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggwpez Nov 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vgeddes Nov 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggwpez Nov 6, 2023

Choose a reason for hiding this comment

Uh oh!

vgeddes Nov 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggwpez Nov 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vgeddes Nov 6, 2023

Choose a reason for hiding this comment

Uh oh!

vgeddes Nov 6, 2023

Choose a reason for hiding this comment

Uh oh!

vgeddes commented Nov 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vgeddes commented Nov 3, 2023 •

edited

Loading

vgeddes Nov 6, 2023 •

edited

Loading

vgeddes Nov 6, 2023 •

edited

Loading

ggwpez Nov 5, 2023 •

edited

Loading

vgeddes Nov 6, 2023 •

edited

Loading

vgeddes Nov 6, 2023 •

edited

Loading

ggwpez Nov 6, 2023 •

edited

Loading