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.

Sign up for GitHub

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

Configure MessageQueue for compatibility with Snowbridge #2146

Closed
wants to merge 4 commits into from
Closed

Configure MessageQueue for compatibility with Snowbridge #2146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
db15e36
MessageQueue compatibility for upcoming Snowbridge merge
vgeddes Nov 1, 2023
05ba898
Fix index
vgeddes Nov 3, 2023
1906cbb
Merge branch 'master' into message-queue-origins
vgeddes Nov 3, 2023
ed303d0
rustfmt
vgeddes Nov 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion cumulus/parachains/runtimes/bridge-hubs/bridge-hub-kusama/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,11 +468,13 @@ construct_runtime!(
PolkadotXcm: pallet_xcm::{Pallet, Call, Event<T>, Origin, Config<T>} = 31,
CumulusXcm: cumulus_pallet_xcm::{Pallet, Event<T>, Origin} = 32,
DmpQueue: cumulus_pallet_dmp_queue::{Pallet, Call, Storage, Event<T>} = 33,
MessageQueue: pallet_message_queue::{Pallet, Call, Storage, Event<T>} = 34,

// Handy utilities.
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
Copy link
Member

Choose a reason for hiding this comment

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

Why is this important?

Copy link
Contributor Author

@vgeddes vgeddes Nov 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

MessageQueue: pallet_message_queue::{Pallet, Call, Storage, Event<T>} = 100,
}
);

Expand Down
4 changes: 3 additions & 1 deletion cumulus/parachains/runtimes/bridge-hubs/bridge-hub-polkadot/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,11 +468,13 @@ construct_runtime!(
PolkadotXcm: pallet_xcm::{Pallet, Call, Event<T>, Origin, Config<T>} = 31,
CumulusXcm: cumulus_pallet_xcm::{Pallet, Event<T>, Origin} = 32,
DmpQueue: cumulus_pallet_dmp_queue::{Pallet, Call, Storage, Event<T>} = 33,
MessageQueue: pallet_message_queue::{Pallet, Call, Storage, Event<T>} = 34,

// Handy utilities.
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
MessageQueue: pallet_message_queue::{Pallet, Call, Storage, Event<T>} = 100,
}
);

Expand Down
3 changes: 2 additions & 1 deletion cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -524,7 +524,6 @@ construct_runtime!(
PolkadotXcm: pallet_xcm::{Pallet, Call, Event<T>, Origin, Config<T>} = 31,
CumulusXcm: cumulus_pallet_xcm::{Pallet, Event<T>, Origin} = 32,
DmpQueue: cumulus_pallet_dmp_queue::{Pallet, Call, Storage, Event<T>} = 33,
MessageQueue: pallet_message_queue::{Pallet, Call, Storage, Event<T>} = 34,

// Handy utilities.
Utility: pallet_utility::{Pallet, Call, Event} = 40,
Expand Down Expand Up @@ -564,6 +563,8 @@ construct_runtime!(
BridgeWestendMessages: pallet_bridge_messages::<Instance3>::{Pallet, Call, Storage, Event<T>, Config<T>} = 51,

BridgeRelayers: pallet_bridge_relayers::{Pallet, Call, Storage, Event<T>} = 47,

MessageQueue: pallet_message_queue::{Pallet, Call, Storage, Event<T>} = 100,
}
);

Expand Down
23 changes: 19 additions & 4 deletions cumulus/primitives/core/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,15 +91,30 @@ pub enum AggregateMessageOrigin {
///
/// This is used by the HRMP queue.
Sibling(ParaId),
/// This is used by the snowbridge-outbound-queue pallet
Export(MessageOrigin),
Copy link
Member

@ggwpez ggwpez Nov 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@vgeddes vgeddes Nov 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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)
}

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@vgeddes vgeddes Nov 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@ggwpez ggwpez Nov 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

/// The message came from the para-chain itself.
Here,
/// The message came from the relay-chain.
Parent,
/// The message came from a sibling para-chain.
Sibling(ParaId),
Copy link
Member

Choose a reason for hiding this comment

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

Why are these variants the same as in the AggregateMessageOrigin?

Copy link
Contributor Author

@vgeddes vgeddes Nov 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

}

impl From<AggregateMessageOrigin> for xcm::v3::MultiLocation {
fn from(origin: AggregateMessageOrigin) -> Self {
use AggregateMessageOrigin::*;
match origin {
AggregateMessageOrigin::Here => MultiLocation::here(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

AggregateMessageOrigin::Parent => MultiLocation::parent(),
AggregateMessageOrigin::Sibling(id) =>
MultiLocation::new(1, Junction::Parachain(id.into())),
Here => MultiLocation::here(),
Parent => MultiLocation::parent(),
Sibling(id) => MultiLocation::new(1, Junction::Parachain(id.into())),
Export(MessageOrigin::Here) => MultiLocation::here(),
Export(MessageOrigin::Parent) => MultiLocation::parent(),
Export(MessageOrigin::Sibling(id)) => MultiLocation::new(1, Junction::Parachain(id.into())),
}
}
}
Expand Down
10 changes: 8 additions & 2 deletions substrate/frame/support/src/traits/messages.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,8 +240,14 @@ pub trait QueuePausedQuery<Origin> {
fn is_paused(origin: &Origin) -> bool;
}

impl<Origin> QueuePausedQuery<Origin> for () {
fn is_paused(_: &Origin) -> bool {
#[impl_trait_for_tuples::impl_for_tuples(30)]
impl<Origin> QueuePausedQuery<Origin> for Tuple {
fn is_paused(origin: &Origin) -> bool {
for_tuples!( #(
if Tuple::is_paused(origin) {
return true;
}
)* );
false
}
}