Skip to content

Backport xcm bridging extensions to the bridge repo #1813

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

Merged
bkontur merged 4 commits into from
Mar 13, 2023
Merged

Backport xcm bridging extensions to the bridge repo #1813

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
43a7b2e
Backport xcm bridging extensions to the bridge repo
bkontur Jan 23, 2023
5209c04
Merge remote-tracking branch 'origin/master' into bko-nits
bkontur Mar 13, 2023
cee886f
Fixes
bkontur Mar 13, 2023
8f5c757
Fixes
bkontur Mar 13, 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
3 changes: 3 additions & 0 deletions bin/runtime-common/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ pub mod messages;
pub mod messages_api;
pub mod messages_benchmarking;
pub mod messages_extension;
pub mod messages_xcm_extension;
pub mod parachains_benchmarking;
pub mod refund_relayer_extension;

Expand All @@ -35,6 +36,8 @@ mod mock;
#[cfg(feature = "integrity-test")]
pub mod integrity;

const LOG_TARGET_BRIDGE_DISPATCH: &str = "runtime::bridge-dispatch";

/// A duplication of the `FilterCall` trait.
///
/// We need this trait in order to be able to implement it for the messages pallet,
Expand Down
10 changes: 5 additions & 5 deletions bin/runtime-common/src/messages.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,7 +476,7 @@ pub mod target {
let weight = XcmWeigher::weight(&mut payload.xcm.1);
let weight = weight.unwrap_or_else(|e| {
log::debug!(
target: "runtime::bridge-dispatch",
target: crate::LOG_TARGET_BRIDGE_DISPATCH,
"Failed to compute dispatch weight of incoming XCM message {:?}/{}: {:?}",
message.key.lane_id,
message.key.nonce,
Expand Down Expand Up @@ -504,7 +504,7 @@ pub mod target {
let FromBridgedChainMessagePayload { xcm: (location, xcm), weight: weight_limit } =
message.data.payload?;
log::trace!(
target: "runtime::bridge-dispatch",
target: crate::LOG_TARGET_BRIDGE_DISPATCH,
"Going to execute message {:?} (weight limit: {:?}): {:?} {:?}",
message_id,
weight_limit,
Expand All @@ -530,7 +530,7 @@ pub mod target {
match xcm_outcome {
Ok(outcome) => {
log::trace!(
target: "runtime::bridge-dispatch",
target: crate::LOG_TARGET_BRIDGE_DISPATCH,
"Incoming message {:?} dispatched with result: {:?}",
message_id,
outcome,
Expand All @@ -539,7 +539,7 @@ pub mod target {
Ok(_weight) => (),
Err(e) => {
log::error!(
target: "runtime::bridge-dispatch",
target: crate::LOG_TARGET_BRIDGE_DISPATCH,
"Incoming message {:?} was not dispatched, error: {:?}",
message_id,
e,
Expand All @@ -549,7 +549,7 @@ pub mod target {
},
Err(e) => {
log::error!(
target: "runtime::bridge-dispatch",
target: crate::LOG_TARGET_BRIDGE_DISPATCH,
"Incoming message {:?} was not dispatched, codec error: {:?}",
message_id,
e,
Expand Down
170 changes: 170 additions & 0 deletions bin/runtime-common/src/messages_xcm_extension.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,170 @@
// Copyright 2022 Parity Technologies (UK) Ltd.
// This file is part of Cumulus.

// Cumulus is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.

// Cumulus is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.

// You should have received a copy of the GNU General Public License
// along with Cumulus. If not, see <http://www.gnu.org/licenses/>.

//! Module provides utilities for easier XCM handling, e.g:
//! [`XcmExecutor`] -> [`MessageSender`] -> <outbound message queue>
//! |
//! <relayer>
//! |
//! [`XcmRouter`] <- [`MessageDispatch`] <- <inbound message queue>

use bp_messages::{
source_chain::MessagesBridge,
target_chain::{DispatchMessage, MessageDispatch},
LaneId,
};
use bp_runtime::{messages::MessageDispatchResult, AccountIdOf, Chain};
use codec::{Decode, Encode};
use frame_support::{dispatch::Weight, CloneNoBound, EqNoBound, PartialEqNoBound};
use scale_info::TypeInfo;
use xcm_builder::{DispatchBlob, DispatchBlobError, HaulBlob, HaulBlobError};

/// PLain "XCM" payload, which we transfer through bridge
pub type XcmAsPlainPayload = sp_std::prelude::Vec<u8>;

/// Message dispatch result type for single message
#[derive(CloneNoBound, EqNoBound, PartialEqNoBound, Encode, Decode, Debug, TypeInfo)]
pub enum XcmBlobMessageDispatchResult {
InvalidPayload,
Dispatched,
NotDispatched(#[codec(skip)] &'static str),
}

/// [`XcmBlobMessageDispatch`] is responsible for dispatching received messages
pub struct XcmBlobMessageDispatch<SourceBridgeHubChain, TargetBridgeHubChain, DispatchBlob> {
Copy link
Collaborator

@serban300 serban300 Jan 24, 2023

Choose a reason for hiding this comment

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

Sorry, I'm really not familiar with XCM and exactly how we plan to use it. I'll research more about it. But question. After adding this do we still need FromBridgedChainMessageDispatch for example ? It seems also based on XCM.

Copy link
Contributor Author

@bkontur bkontur Jan 24, 2023

Choose a reason for hiding this comment

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

well, yes, FromBridgedChainMessageDispatch is similar but uses our struct FromBridgedChainMessagePayload,
XcmBlobMessageDispatch plays with xcm blob and does not even decode xcm, but also does not touches dispatch_weight and weights/fees for bridge-hub are other issue (coming soon)

so I would not touch FromBridgedChainMessageDispatch now, unless we set up weight/fees for BridgeHub, then we will see better

Copy link
Contributor Author

@bkontur bkontur Jan 24, 2023

Choose a reason for hiding this comment

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

anyway, I thought that could help to fix those integrity_tests, but it is unrelated. If we could change integrity test that InboundPayload could take FromBridgedChainMessagePayload or Vec<u8>.

all this stuff, it is just simple connection point between XcmExecutor/XcmConfig handling ExportMessage and messages pallet (outbound) queue on one side and between message pallet (inbound) queue triggering dispatch with XcmRouter on the other side, so all this is really just xcm extension, because rialto/millau setup does not use xcm-v3::ExportMessage

we dont need to even backport it now, I can still have it in Cumulus, but it looks like it could fit to bridges repo also, so later when weight/fees will be stable we could move it here

Copy link
Contributor

@svyatonik svyatonik Jan 25, 2023

Choose a reason for hiding this comment

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

I agree that we don't need FromBridgedChainMessageDispatch anymore (dispatch is not a goal of this repo anymore). But we can do it in a separate PR to avoid delays in RBH<>WBH implementation (I can do that myself).

@bkontur This dispatch_weight method is a weight of message dispatch at this chain. So if we are talking about bridge hubs, which are just redirecting messages to other parachains, this should be the weight of this redirection, not the weight of the XCM message itself (which happens at other parachain). So e.g. if redirecting is just appending it to some map/queue, it may be just DbWegith::read_writes(1, 1) or something like that. In any case - you don't need to use XcmExecutor or decode XCM message here (unless I'm totally wrong).

Copy link
Contributor Author

@bkontur bkontur Mar 13, 2023

Choose a reason for hiding this comment

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

@svyatonik
ok, thank you, I think I understand now this weight stuff more and more,

I added here DispatchBlobWeigher: Get<Weight>, so the runtime will provide some values,

e.g. BridgeHub for dispatch uses:

pub type XcmRouter = (
	// Two routers - use UMP to communicate with the relay chain:
	cumulus_primitives_utility::ParentAsUmp<ParachainSystem, PolkadotXcm, ()>,
	// ..and XCMP to communicate with the sibling chains.
	XcmpQueue,
);

XcmpQueue does cca 2 reads and 2 writes, so it will be DbWegith::read_writes(2, 2)

_marker:
sp_std::marker::PhantomData<(SourceBridgeHubChain, TargetBridgeHubChain, DispatchBlob)>,
}

impl<SourceBridgeHubChain: Chain, TargetBridgeHubChain: Chain, BlobDispatcher: DispatchBlob>
MessageDispatch<AccountIdOf<SourceBridgeHubChain>>
for XcmBlobMessageDispatch<SourceBridgeHubChain, TargetBridgeHubChain, BlobDispatcher>
{
type DispatchPayload = XcmAsPlainPayload;
type DispatchLevelResult = XcmBlobMessageDispatchResult;

fn dispatch_weight(_message: &mut DispatchMessage<Self::DispatchPayload>) -> Weight {
log::error!(
target: crate::LOG_TARGET_BRIDGE_DISPATCH,
"[XcmBlobMessageDispatch] TODO: change here to XCMv3 dispatch_weight with XcmExecutor - message: ?...?",
);
// TODO:check-parameter - setup weight?
Copy link
Contributor

@svyatonik svyatonik Jan 25, 2023

Choose a reason for hiding this comment

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

Can you, please, add issue reference to those TODOs?

Copy link
Contributor

@svyatonik svyatonik Jan 25, 2023

Choose a reason for hiding this comment

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

BTW - looking at the DispatchBlob trait, I can see that the dispatch_blob function returns the unexpectable weight consumed by the dispatch, but the return value is Result<(),_>. That's kinda strange. Maybe it is designed to be called from on_idle? But anyway - we need the worst-case weight of this function and ideally the unspent weight.

Copy link
Contributor Author

@bkontur bkontur Jan 25, 2023

Choose a reason for hiding this comment

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

@svyatonik
yes, well, I saw that long time ago, and reported in original xcm-v3 pr several time:
paritytech/polkadot#4097 (comment)

I have this still in my TODOs, so I will probably open a separate issue for that dispatch_blob

Copy link
Contributor Author

@bkontur bkontur Mar 13, 2023

Choose a reason for hiding this comment

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

BTW - looking at the DispatchBlob trait, I can see that the dispatch_blob function returns the unexpectable weight consumed by the dispatch, but the return value is Result<(),_>. That's kinda strange. Maybe it is designed to be called from on_idle? But anyway - we need the worst-case weight of this function and ideally the unspent weight.

@svyatonik
here maybe it should be like:

pub trait DispatchBlob {
	/// Dispatches an incoming blob and returns the unexpectable weight consumed by the dispatch.
	fn dispatch_blob(blob: Vec<u8>) -> Result<Weight, DispatchBlobError>;

        /// Worst-case estimated weight
	fn dispatch_weight() -> Weight;
}

Weight::zero()
}

fn dispatch(
_relayer_account: &AccountIdOf<SourceBridgeHubChain>,
message: DispatchMessage<Self::DispatchPayload>,
) -> MessageDispatchResult<Self::DispatchLevelResult> {
let payload = match message.data.payload {
Ok(payload) => payload,
Err(e) => {
log::error!(
target: crate::LOG_TARGET_BRIDGE_DISPATCH,
"[XcmBlobMessageDispatch] payload error: {:?} - message_nonce: {:?}",
e,
message.key.nonce
);
return MessageDispatchResult {
// TODO:check-parameter - setup uspent_weight?
unspent_weight: Weight::zero(),
dispatch_level_result: XcmBlobMessageDispatchResult::InvalidPayload,
}
},
};
let dispatch_level_result = match BlobDispatcher::dispatch_blob(payload) {
Ok(_) => {
log::debug!(
target: crate::LOG_TARGET_BRIDGE_DISPATCH,
"[XcmBlobMessageDispatch] DispatchBlob::dispatch_blob was ok - message_nonce: {:?}",
message.key.nonce
);
XcmBlobMessageDispatchResult::Dispatched
},
Err(e) => {
let e = match e {
DispatchBlobError::Unbridgable => "DispatchBlobError::Unbridgable",
DispatchBlobError::InvalidEncoding => "DispatchBlobError::InvalidEncoding",
DispatchBlobError::UnsupportedLocationVersion =>
"DispatchBlobError::UnsupportedLocationVersion",
DispatchBlobError::UnsupportedXcmVersion =>
"DispatchBlobError::UnsupportedXcmVersion",
DispatchBlobError::RoutingError => "DispatchBlobError::RoutingError",
DispatchBlobError::NonUniversalDestination =>
"DispatchBlobError::NonUniversalDestination",
DispatchBlobError::WrongGlobal => "DispatchBlobError::WrongGlobal",
};
log::error!(
target: crate::LOG_TARGET_BRIDGE_DISPATCH,
"[XcmBlobMessageDispatch] DispatchBlob::dispatch_blob failed, error: {:?} - message_nonce: {:?}",
e, message.key.nonce
);
XcmBlobMessageDispatchResult::NotDispatched(e)
},
};
MessageDispatchResult {
// TODO:check-parameter - setup uspent_weight?
unspent_weight: Weight::zero(),
dispatch_level_result,
}
}
}

/// [`XcmBlobHauler`] is responsible for sending messages to the bridge "point-to-point link" from
/// one side, where on the other it can be dispatched by [`XcmBlobMessageDispatch`].
pub trait XcmBlobHauler {
/// Runtime message sender adapter.
type MessageSender: MessagesBridge<Self::MessageSenderOrigin, XcmAsPlainPayload>;

/// Runtime message sender origin, which is used by MessageSender.
type MessageSenderOrigin;
/// Our location within the Consensus Universe.
fn message_sender_origin() -> Self::MessageSenderOrigin;

/// Return message lane (as "point-to-point link") used to deliver XCM messages.
fn xcm_lane() -> LaneId;
}

/// XCM bridge adapter which connects [`XcmBlobHauler`] with [`MessageSender`] and makes sure that
/// XCM blob is sent to the [`pallet_bridge_messages`] queue to be relayed.
pub struct XcmBlobHaulerAdapter<XcmBlobHauler>(sp_std::marker::PhantomData<XcmBlobHauler>);
impl<HaulerOrigin, H: XcmBlobHauler<MessageSenderOrigin = HaulerOrigin>> HaulBlob
for XcmBlobHaulerAdapter<H>
{
fn haul_blob(blob: sp_std::prelude::Vec<u8>) -> Result<(), HaulBlobError> {
let lane = H::xcm_lane();
let result = H::MessageSender::send_message(H::message_sender_origin(), lane, blob);
let result = result.map(|artifacts| {
let hash = (lane, artifacts.nonce).using_encoded(sp_io::hashing::blake2_256);
hash
});
match &result {
Ok(result) => log::info!(
target: crate::LOG_TARGET_BRIDGE_DISPATCH,
"haul_blob result - ok: {:?} on lane: {:?}",
result,
lane
),
Err(error) => log::error!(
target: crate::LOG_TARGET_BRIDGE_DISPATCH,
"haul_blob result - error: {:?} on lane: {:?}",
error,
lane
),
};
result.map(|_| ()).map_err(|_| HaulBlobError::Transport("MessageSenderError"))
}
}