Implement additional require primitives for dynamic fees directly for pallet-xcm-bridge-hub #2261

svyatonik · 2023-07-13T09:26:32Z

related to #2434
alternative to #2233

The idea is to:

add backoff to inbound bridge queue (aka outbound XCMP queue between target BH and target chain) - if it has too many enqueued messages, we assume that the target chain is unable to process messages in a timely fashion and stop accepting message delivery transactions: e19c82a
add backoff mechanism to outbound bridge queue - if it has too many enqueued messages, we assume that the inbound bridge queue (at the bridged, target BH) contains too many messages (or if there's no relayer running) and stop accepting new messages to this queue by pausing inbound "end" of XCMP channel between sending chain and its sibling BH. It requires some parts of XCM bridge hub pallet (will be deployed at bridge hubs) #2213 and A way to dynamically (programmatically) suspend and resume inbound XCM channels at bridge hub #2406
XCMP queue has its own backoff mechanism and because of that, messages will eventually start piling up at the sending chain outbound XCMP queue "end". So it can estimate the number of messages in the whole bridge pipeline and start applying exponential fee or something like that

This PR requires corresponding changes to the relayer. It'll be done in a follow up/parallel PR.

===

Update: this ^^^ information is a bit obsolete, but overall it stays the same.Main things to notice in this PR:

I've introduced a separate BridgeId struct, so there's now both BridgeId and LaneId. Both are H256 and are matching now. The reason is that we shall use versioned XCM structs in BridgeId AND version may change at some point. Don't know what we'll do when it happens, but we must be prepared for that;
messages_xcm_extension.rs file is removed and required implementations are moved to the pallet-xcm-bridge-hub (HaulBlobExporter, MessageDispatch and so on);
there's now LocalXcmChannelManager, which is able to report channel status + suspend/resume the channel. I've removed bp-xcm-bridge-hub-router crate because of that.

svyatonik · 2023-07-14T12:16:20Z

bin/runtime-common/src/messages_xcm_extension.rs

+// TODO: use following structures in the `pallet-message-queue` configuration
+
+// TODO: it must be a part of `pallet-xcm-bridge-hub`
+pub struct LocalInboundXcmChannelSuspender<Origin, Inner>(PhantomData<(Origin, Inner)>);


Open question: in paritytech/cumulus#2157, the QueueChangeHandler of pallet_message_queue is not set (???) to the cumulus-pallet-xcmp-queue. Given that, our backpressure won't actually work - we are pausing dispatch here, but the transport channel will stay opened.

This: https://github.com/paritytech/cumulus/pull/2157/files#diff-3d3febd8cfcd42ba5bb949d63fdafb9a71da3db1feb35a45601404b64fcd70c4R575 and e.g. this https://github.com/paritytech/cumulus/pull/2157/files#diff-447906ba563849a582953cc1753f58164c23d4902539e37c5dde387113a3aa5bR411

* add version checks on para ugprade * Apply suggestions from code review Co-authored-by: Bastian Köcher <git@kchr.de> * remove unneeded imports and errors * fix test error path --------- Co-authored-by: Bastian Köcher <git@kchr.de>

…orage format will likely change to be able to resume bridges from the on_iniitalize/on_idle" This reverts commit bdd7ae1.

svyatonik · 2023-08-01T08:57:21Z

renamed, because most of the code required for dynamic fees is now in a separate PR: #2312

svyatonik · 2023-08-01T10:44:44Z

modules/xcm-bridge-hub/src/backpressure.rs

@@ -0,0 +1,131 @@
+// Copyright 2019-2021 Parity Technologies (UK) Ltd.


This file should be the copy of the similar primitives (LocalXcmQueueMessageProcessor and LocalXcmQueueSuspender) from #2294. The only difference is how we "compute" whether the inbound XCM queue should be suspended or not. In v1 PR it looks at the number of queued messages at the outbound lane. Here it should take into account all outbound lanes, opened by the same origin (a bit WIP)

…here

- BridgeId now uses versioned universal locations - added missing stuff to exporter.rs

modules/xcm-bridge-hub/src/exporter.rs

modules/xcm-bridge-hub/src/lib.rs

serban300 · 2023-08-23T06:20:49Z

primitives/xcm-bridge-hub/src/lib.rs

+pub struct BridgeId(LaneId);
+
+impl BridgeId {
+	/// Create bridge identifier from two universal locations.
 	///
-	/// - no more incoming XCM messages from the `owner` will be processed until further
-	///  `resume_inbound_channel` call;
+	/// The fact that we are using versioned locations here means that XCM version upgrades must
+	/// be coordinated at all involved chains (at source and target chains + at bridge hubs).
+	/// Otherwise messages may simply be dropped anywhere on its path to the target chain.
+	pub fn new(
+		universal_location1: &VersionedInteriorMultiLocation,
+		universal_location2: &VersionedInteriorMultiLocation,
+	) -> Self {
+		// a tricky helper struct that adds required `Ord` support for
+		// `VersionedInteriorMultiLocation`
+		#[derive(Eq, PartialEq, Ord, PartialOrd)]
+		struct EncodedVersionedInteriorMultiLocation(sp_std::vec::Vec<u8>);
+
+		impl Encode for EncodedVersionedInteriorMultiLocation {
+			fn encode(&self) -> sp_std::vec::Vec<u8> {
+				self.0.clone()
+			}
+		}
+
+		Self(LaneId::new(
+			EncodedVersionedInteriorMultiLocation(universal_location1.encode()),
+			EncodedVersionedInteriorMultiLocation(universal_location2.encode()),
+		))
+	}


I've introduced a separate BridgeId struct, so there's now both BridgeId and LaneId. Both are H256 and are matching now. The reason is that we shall use versioned XCM structs in BridgeId AND version may change at some point. Don't know what we'll do when it happens, but we must be prepared for that;

Sounds like a complicated issue. But I'm not sure if introducing the BridgeId will help.

Yep - that's a very complicated issue. Like XCM version is upgraded for the whole chain. And since bridge hub is used by many other chains (including from other consensus) - we should coordinate that with all those chains. Or else we need e.g. to use different XCM version with different chains. I've seen there's a subscription for XCM version - i.e. you receive notification when XCM is upgraded on some other chain. Maybe we could use it here. But I do not know yet.

Why I'm adding BridgeId - because we e.g. may introduce storage map from BridgeId to LaneId. So even if BridgeId is changed, we may keep using the same LaneId. Also - those types are (will be) used in different communications - BridgeId will be used when communicating with sibling/parent chain and LaneId is used when communicating with the bridged bridge hub. So imo it makes sense to keep this difference at least because of that.

As for the XCM version support - I'll file an issue (probably for v3 :) ). It definitely needs some more thinking.

Also - those types are (will be) used in different communications - BridgeId will be used when communicating with sibling/parent chain and LaneId is used when communicating with the bridged bridge hub. So imo it makes sense to keep this difference at least because of that.

Makes sense

As for the XCM version support - I'll file an issue (probably for v3 :) ). It definitely needs some more thinking.

Perfect. Better to track this in an issue

filed #2500

* backport latest relevant dynamic fees changes from v1 to v2 * fix comment

… pallet-xcm-bridge-hub (#2261) * added backoff mechanism to inbound bridge queue * impl backpressure in the XcmBlobHaulerAdapter * leave TODOs * BridgeMessageProcessor prototype * another TODO * Revert "also temporary (?) remove BridgesByLocalOrigin because the storage format will likely change to be able to resume bridges from the on_iniitalize/on_idle" This reverts commit bdd7ae1. * prototype for QueuePausedQuery * implement ExportXcm and MessageDispatch for pallet-xcm-bridge-hub * spelling * flush * small comments to myself * more backports from dynamic-fees-v1 * use new pallet as exporter and dispatcher in Millau * use new pallet as exporter and dispatcher in Rialto * use new pallet as exporter and dispatcher in RialtoParachain * flush * fix remaining compilation issues * warnings + fmt * fix tests * LocalXcmChannelManager * change lane ids * it works! * remove bp-xcm-bridge-hub-router and use LocalXcmChannelManager everywhere * removed commented code * cleaning up * cleaning up * cleaning up * - separated BridgeId and LaneId - BridgeId now uses versioned universal locations - added missing stuff to exporter.rs * OnMessagesDelivered is back * start using bp-xcm-bridge-hub as OnMessagesDelivered * cleaning up * spelling * fix stupid issues * Backport latest relevant dynamic fees changes from v1 to v2 (#2372) * backport latest relevant dynamic fees changes from v1 to v2 * fix comment

added backoff mechanism to inbound bridge queue

e19c82a

svyatonik added P-Runtime PR-audit-needed A PR has to be audited before going live. labels Jul 13, 2023

svyatonik added this to the Kusama <> Polkadot Parachain Bridge V2 milestone Jul 13, 2023

svyatonik mentioned this pull request Jul 13, 2023

Introduce partial bp-xcm-bridge-hub - just LocalXcmChannelManager #2265

Merged

Merge branch 'master' into xcm-dynamic-fees-using-backoff-mechanism

30cb294

svyatonik mentioned this pull request Jul 14, 2023

XCM bridge hub pallet (will be deployed at bridge hubs) #2213

Merged

impl backpressure in the XcmBlobHaulerAdapter

6714576

svyatonik mentioned this pull request Jul 14, 2023

Misbehavior in the xcm-bridge-hub pallet #2268

Closed

svyatonik added 2 commits July 14, 2023 14:13

leave TODOs

519de78

BridgeMessageProcessor prototype

921d21e

svyatonik commented Jul 14, 2023

View reviewed changes

svyatonik added 2 commits July 14, 2023 15:42

another TODO

6aaae89

Merge branch 'master' into xcm-dynamic-fees-using-backoff-mechanism

321b9ae

svyatonik mentioned this pull request Jul 17, 2023

Update dependecies #2277

Merged

svyatonik added 2 commits July 20, 2023 08:19

Merge branch 'master' into xcm-dynamic-fees-using-backoff-mechanism

e89f119

Revert "also temporary (?) remove BridgesByLocalOrigin because the st…

5efb5e0

…orage format will likely change to be able to resume bridges from the on_iniitalize/on_idle" This reverts commit bdd7ae1.

svyatonik mentioned this pull request Jul 20, 2023

Implement ExportXcm and MessageDispatch for pallet-xcm-bridge-hub #2285

Merged

svyatonik added 4 commits July 20, 2023 14:24

prototype for QueuePausedQuery

a007337

implement ExportXcm and MessageDispatch for pallet-xcm-bridge-hub

dd65e9b

spelling

ffbd4b2

flush

dd7eeea

svyatonik changed the title ~~Dynamic fees implementation that adds backoff for all intermediate bridge queues (main option)~~ Implement additional require primitives for dynamic fees directly for pallet-xcm-bridge-hub Aug 1, 2023

svyatonik added 3 commits August 1, 2023 13:20

Merge branch 'master' into xcm-dynamic-fees-using-backoff-mechanism

5c88d28

Merge branch 'master' into xcm-dynamic-fees-using-backoff-mechanism

7e93841

small comments to myself

a3324ff

svyatonik commented Aug 1, 2023

View reviewed changes

more backports from dynamic-fees-v1

3202065

svyatonik added 3 commits August 2, 2023 12:43

use new pallet as exporter and dispatcher in Rialto

b836c41

use new pallet as exporter and dispatcher in RialtoParachain

694f133

flush

66ec0f4

bkontur mentioned this pull request Aug 3, 2023

Dynamic fees for bridges-v1 #2294

Merged

9 tasks

svyatonik added 17 commits August 16, 2023 12:52

fix remaining compilation issues

ba3ad38

warnings + fmt

5a60dad

fix tests

b843f13

LocalXcmChannelManager

d88a43e

change lane ids

5cb5dee

it works!

9f757ab

remove bp-xcm-bridge-hub-router and use LocalXcmChannelManager everyw…

3516fcb

…here

removed commented code

41321b6

cleaning up

0d46366

cleaning up

985a721

cleaning up

ebe5be7

- separated BridgeId and LaneId

d38fbca

- BridgeId now uses versioned universal locations - added missing stuff to exporter.rs

OnMessagesDelivered is back

fe5f4b1

start using bp-xcm-bridge-hub as OnMessagesDelivered

8753e37

cleaning up

e087a4e

Merge branch 'master' into xcm-dynamic-fees-using-backoff-mechanism

ac5d8ac

spelling

7577a43

svyatonik marked this pull request as ready for review August 22, 2023 09:34

serban300 approved these changes Aug 23, 2023

View reviewed changes

svyatonik added 2 commits August 23, 2023 16:31

fix stupid issues

6e174a0

Backport latest relevant dynamic fees changes from v1 to v2 (#2372)

d091824

* backport latest relevant dynamic fees changes from v1 to v2 * fix comment

svyatonik mentioned this pull request Aug 25, 2023

Added remaining unit tests for pallet-xcm-bridge-hub #2499

Merged

svyatonik merged commit bd87488 into master Aug 25, 2023

svyatonik deleted the xcm-dynamic-fees-using-backoff-mechanism branch August 25, 2023 08:14

svyatonik mentioned this pull request Aug 25, 2023

Investigate how XCM version upgrade will affect the bridge #2500

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement additional require primitives for dynamic fees directly for pallet-xcm-bridge-hub #2261

Implement additional require primitives for dynamic fees directly for pallet-xcm-bridge-hub #2261

svyatonik commented Jul 13, 2023 •

edited

Loading

svyatonik Jul 14, 2023

svyatonik Jul 14, 2023

svyatonik commented Aug 1, 2023

svyatonik Aug 1, 2023

serban300 Aug 23, 2023

svyatonik Aug 23, 2023

serban300 Aug 23, 2023

svyatonik Aug 25, 2023

		@@ -0,0 +1,131 @@
		// Copyright 2019-2021 Parity Technologies (UK) Ltd.

Implement additional require primitives for dynamic fees directly for pallet-xcm-bridge-hub #2261

Implement additional require primitives for dynamic fees directly for pallet-xcm-bridge-hub #2261

Conversation

svyatonik commented Jul 13, 2023 • edited Loading

svyatonik Jul 14, 2023

Choose a reason for hiding this comment

svyatonik Jul 14, 2023

Choose a reason for hiding this comment

svyatonik commented Aug 1, 2023

svyatonik Aug 1, 2023

Choose a reason for hiding this comment

serban300 Aug 23, 2023

Choose a reason for hiding this comment

svyatonik Aug 23, 2023

Choose a reason for hiding this comment

serban300 Aug 23, 2023

Choose a reason for hiding this comment

svyatonik Aug 25, 2023

Choose a reason for hiding this comment

svyatonik commented Jul 13, 2023 •

edited

Loading