-
Notifications
You must be signed in to change notification settings - Fork 130
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.
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
Dynamic fees for bridges-v1 #2294
Conversation
…ridge hub and target asset hub is suspended
// the cost will include both cost of: (1) to-sibling bridg hub delivery (returned by | ||
// the `Config::ToBridgeHubSender`) and (2) to-bridged bridge hub delivery (returned by | ||
// `Self::exporter_for`) | ||
ViaBridgeHubExporter::<T, I>::validate(dest, xcm) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another dependency we'll need for that is paritytech/cumulus#2342. So idea is:
- bridge backpressure mechanism involves many chains, so it isn't instant - it may take some to suspend the queue between source AH and source BH. And we can't allow senders to send messages over the bridge for a constant cost while this mechanism activates;
- so we need to charge dynamic fees for sending messages from source AH and source BH even when we don't have any info about other bridge queues. That's where the mentioned PR should help us - the total transport cost that the sender pays is the
BridgeFee + HrmpFee
. SoBridgeFee
will grow when we have a report about overloaded bridge queues andHrmpFee
shall be maintained independently
I've been able finally to test the whole bridge pipeline with backpressure mechanism enabled. Here's how it works (from the practice perspective):
Things to notice:
|
Marked onice because of dependencies |
bot fmt |
bot clean |
* report congestion status: changes at the sending chain * OnMessagesDelivered is back * report congestion status: changes at the bridge hub * moer logging * fix? benchmarks * spelling * tests for XcmBlobHaulerAdapter and LocalXcmQueueManager * tests for messages pallet * fix typo * rustdoc * Update modules/messages/src/lib.rs * apply review suggestions
{ | ||
type DispatchPayload = XcmAsPlainPayload; | ||
type DispatchLevelResult = XcmBlobMessageDispatchResult; | ||
|
||
fn is_active() -> bool { | ||
!Channel::is_congested() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is executed at the target bridge hub. I suggest to use the following code for the Channel
implementation:
pub struct LocalXcmChannelWithSiblingAssetHub;
impl bp_xcm_bridge_hub_router::XcmChannelStatusProvider for LocalXcmChannelWithSiblingAssetHub {
fn is_congested() -> bool {
// let's find the channel with the sibling AH
let sibling_asset_hub_id: cumulus_primitives_core::ParaId = xcm_config::SiblingAssetHubParId::get().into();
let outbound_channels = cumulus_pallet_xcmp_queue::OutboundXcmpStatus::<Runtime>::get();
let channel_with_sibling_asset_hub = outbound_channels.iter()
.filter(|c| c.recipient() == sibling_asset_hub_id)
.next();
// no channel => it is empty, so not congested
let channel_with_sibling_asset_hub = match channel_with_sibling_asset_hub {
Some(channel_with_sibling_asset_hub) => channel_with_sibling_asset_hub,
None => return false,
};
// suspended channel => it is congested
if channel_with_sibling_asset_hub.is_suspended() {
return true;
}
// TODO: the following restriction is arguable, we may live without that, assuming that there
// can't be more than some `N` messages queued at the bridge queue (at the source BH) AND before
// accepting next (or next-after-next) delivery transaction, we'll receive the suspension signal
// from the target AH and stop accepting delivery transactions
// it takes some time for target AH to suspend inbound channel with the target BH and during that
// we will keep accepting new message delivery transactions. Let's also reject new deliveries if
// there are too many "pages" (concatenated XCM messages) in the target BH -> target AH queue.
const MAX_QUEUED_PAGES_BEFORE_DEACTIVATION: u16 = 4;
if channel_with_sibling_asset_hub.queued_pages() > MAX_QUEUED_PAGES_BEFORE_DEACTIVATION {
return true;
}
true
}
}
|
||
// if the channel with sibling/child bridge hub is suspended, we don't change | ||
// anything | ||
if T::WithBridgeHubChannel::is_congested() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is executed at the source (aka sending) chain. In our case it is the source asset hub. I suggest to use the following code as theWithBridgeHubChannel
:
pub struct LocalXcmpChannelAdapter;
impl bp_xcm_bridge_hub_router::XcmChannelStatusProvider for LocalXcmpChannelAdapter {
fn is_congested() -> bool {
// if the outbound channel with recipient is suspended, it means that one of further
// bridge queues (e.g. bridge queue between two bridge hubs) is overloaded, so we shall
// take larger fee for our outbound messages
let sibling_bridge_hub_id: cumulus_primitives_core::ParaId = BridgeHubKusamaParaId::get().into();
let outbound_channels = cumulus_pallet_xcmp_queue::OutboundXcmpStatus::<Runtime>::get();
let outbound_channel = outbound_channels.iter()
.filter(|c| c.recipient() == sibling_bridge_hub_id)
.next();
let is_outbound_channel_suspended = outbound_channel.clone().map(|c| c.is_suspended()).unwrap_or(false);
if is_outbound_channel_suspended {
return true;
}
// if the inbound channel with recipient is suspended, it means that we are unable to receive
// congestion reports from the bridge hub. So we assume the bridge pipeline is congested too
let inbound_suspended = cumulus_pallet_xcmp_queue::InboundXcmpSuspended::<Runtime>::get();
let is_inbound_channel_suspended = inbound_suspended.contains(&sibling_bridge_hub_id);
if is_inbound_channel_suspended {
return true;
}
// TODO: https://github.com/paritytech/cumulus/pull/2342 - once this PR is merged, we may
// remove the following code
//
// if the outbound channel has at least `N` pages enqueued, let's assume it is congested.
// Normally, the chain with a few opened HRMP channels, will "send" pages at every block.
// Having `N` pages means that for last `N` blocks we either have not sent any messages,
// or have sent signals.
const MAX_OUTBOUND_PAGES_BEFORE_CONGESTION: u16 = 4;
let is_outbound_channel_congested = outbound_channel.map(|c| c.queued_pages()).unwrap_or(0);
is_outbound_channel_congested > MAX_OUTBOUND_PAGES_BEFORE_CONGESTION
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The base fee then need to include both bridge fee and HRMP fee. And then instead of the BridgeFee * BridgeFeeFactor + HrmpFee * HrmpFeeFactor
, we'll temporary have the (BridgeFee + HrmpFee) * BridgeFeeFactor
until paritytech/cumulus#2342 is implemented. This is fine, because the exponential factor is present in the formula.
Also keep in mind that the actual HRMP fee is currently set to zero (type PriceForSiblingDelivery = ()
), so this is even better for us.
bot fmt |
@bkontur https://gitlab.parity.io/parity/mirrors/parity-bridges-common/-/jobs/3360002 was started for your command Comment |
@bkontur Command |
…K/P (#2350) * Added `XcmBridgeHubRouterCall::report_bridge_status` encodings for AHK/P * Spellcheck * Added const for `XcmBridgeHubRouterTransactCallMaxWeight` * Cargo.lock * Introduced base delivery fee constants * Congestion messages as Optional to turn on/off `supports_congestion_detection` * Spellcheck * Ability to externalize dest for benchmarks * Ability to externalize dest for benchmarks
bot rebase |
Change XCM routing configuration for bridging from unpaid to paid version. Paid version means that origin (besides extrinsic fees) pays delivery fees at source Asset Hub and also Asset Hub sovereign account pays for execution of `ExportMessage` instruction at local Bridge Hub. Change XCM bridging router from `UnpaidRemoteExporter` to `ToPolkadotXcmRouter` and `ToKusamaXcmRouter` which are pallet instances of new module `pallet-xcm-bridge-hub-router`. The main thing that the pallet `pallet-xcm-bridge-hub-router` offers is the dynamic message fee, that is computed based on the bridge queues state. It starts exponentially increasing if the queue between this chain and the sibling/child bridge hub is congested. More about dynamic fees and back-preasure for v1 can be found [here](paritytech/parity-bridges-common#2294). Co-authored-by: Branislav Kontur <bkontur@gmail.com> Co-authored-by: Adrian Catangiu <adrian@parity.io> Co-authored-by: Svyatoslav Nikolsky <svyatonik@gmail.com> Signed-off-by: Branislav Kontur <bkontur@gmail.com> Signed-off-by: Adrian Catangiu <adrian@parity.io> Signed-off-by: Svyatoslav Nikolsky <svyatonik@gmail.com>
Change XCM routing configuration for bridging from unpaid to paid version. Paid version means that origin (besides extrinsic fees) pays delivery fees at source Asset Hub and also Asset Hub sovereign account pays for execution of `ExportMessage` instruction at local Bridge Hub. Change XCM bridging router from `UnpaidRemoteExporter` to `ToPolkadotXcmRouter` and `ToKusamaXcmRouter` which are pallet instances of new module `pallet-xcm-bridge-hub-router`. The main thing that the pallet `pallet-xcm-bridge-hub-router` offers is the dynamic message fee, that is computed based on the bridge queues state. It starts exponentially increasing if the queue between this chain and the sibling/child bridge hub is congested. More about dynamic fees and back-preasure for v1 can be found [here](paritytech/parity-bridges-common#2294). Signed-off-by: Branislav Kontur <bkontur@gmail.com> Signed-off-by: Adrian Catangiu <adrian@parity.io> Signed-off-by: Svyatoslav Nikolsky <svyatonik@gmail.com> Co-authored-by: Adrian Catangiu <adrian@parity.io> Co-authored-by: Svyatoslav Nikolsky <svyatonik@gmail.com>
Change XCM routing configuration for bridging from unpaid to paid version. Paid version means that origin (besides extrinsic fees) pays delivery fees at source Asset Hub and also Asset Hub sovereign account pays for execution of `ExportMessage` instruction at local Bridge Hub. Change XCM bridging router from `UnpaidRemoteExporter` to `ToPolkadotXcmRouter` and `ToKusamaXcmRouter` which are pallet instances of new module `pallet-xcm-bridge-hub-router`. The main thing that the pallet `pallet-xcm-bridge-hub-router` offers is the dynamic message fee, that is computed based on the bridge queues state. It starts exponentially increasing if the queue between this chain and the sibling/child bridge hub is congested. More about dynamic fees and back-preasure for v1 can be found [here](paritytech/parity-bridges-common#2294). Signed-off-by: Branislav Kontur <bkontur@gmail.com> Signed-off-by: Adrian Catangiu <adrian@parity.io> Signed-off-by: Svyatoslav Nikolsky <svyatonik@gmail.com> Co-authored-by: Adrian Catangiu <adrian@parity.io> Co-authored-by: Svyatoslav Nikolsky <svyatonik@gmail.com>
This pull request has been mentioned on Polkadot Forum. There might be relevant details there: https://forum.polkadot.network/t/polkadot-kusama-bridge/2971/5 |
Change XCM routing configuration for bridging from unpaid to paid version. Paid version means that origin (besides extrinsic fees) pays delivery fees at source Asset Hub and also Asset Hub sovereign account pays for execution of `ExportMessage` instruction at local Bridge Hub. Change XCM bridging router from `UnpaidRemoteExporter` to `ToPolkadotXcmRouter` and `ToKusamaXcmRouter` which are pallet instances of new module `pallet-xcm-bridge-hub-router`. The main thing that the pallet `pallet-xcm-bridge-hub-router` offers is the dynamic message fee, that is computed based on the bridge queues state. It starts exponentially increasing if the queue between this chain and the sibling/child bridge hub is congested. More about dynamic fees and back-preasure for v1 can be found [here](paritytech/parity-bridges-common#2294). Signed-off-by: Branislav Kontur <bkontur@gmail.com> Signed-off-by: Adrian Catangiu <adrian@parity.io> Signed-off-by: Svyatoslav Nikolsky <svyatonik@gmail.com> Co-authored-by: Adrian Catangiu <adrian@parity.io> Co-authored-by: Svyatoslav Nikolsky <svyatonik@gmail.com>
* impl backpressure in the XcmBlobHaulerAdapter * LocalXcmQueueManager + more adapters * OnMessageDelviered callback * forbid mesage delivery transactions when the channel between target bridge hub and target asset hub is suspended * pallet-xcm-bridge-hub-router * removed commented code * improvements and tests for palle-xcm-bridge-router * use LocalXcmChannel in XcmBlobMessageDispatch * new tests for logic changes in messages pallet * tests for LocalXcmQueueSuspender * tests for LocalXcmQueueMessageProcessor * tests for new logic in the XcmBlobHaulerAdapter * fix other tests in the bridge-runtime-common * extension_reject_call_when_dispatcher_is_inactive * benchmarks for pallet-xcm-bridge-hub-router * get rid of redundant storage value * add new pallet to verify-pallets-build.sh * fixing spellcheck, clippy and rustdoc * trigger CI * Revert "trigger CI" This reverts commit 48f1ba0. * change log target for xcm bridge router pallet * Update modules/xcm-bridge-hub-router/src/lib.rs Co-authored-by: Branislav Kontur <bkontur@gmail.com> * use saturated_len where possible * fmt * (Suggestion) Ability to externalize configuration for `ExporterFor` (paritytech#2313) * Ability to externalize configuration for `ExporterFor` (Replaced `BridgedNetworkId/SiblingBridgeHubLocation` with `Bridges: ExporterFor`) * Fix millau * Compile fix * Return back `BridgedNetworkId` but as optional filter * Replaced `BaseFee` with fees from inner `Bridges: ExporterFor` * typo * Clippy * Rename LocalXcmChannel to XcmChannelStatusProvider (paritytech#2319) * Rename LocalXcmChannel to XcmChannelStatusProvider * fmt * added/fixed some docs * Dynamic fees v1: report congestion status to sending chain (paritytech#2318) * report congestion status: changes at the sending chain * OnMessagesDelivered is back * report congestion status: changes at the bridge hub * moer logging * fix? benchmarks * spelling * tests for XcmBlobHaulerAdapter and LocalXcmQueueManager * tests for messages pallet * fix typo * rustdoc * Update modules/messages/src/lib.rs * apply review suggestions * ".git/.scripts/commands/fmt/fmt.sh" * Added `XcmBridgeHubRouterCall::report_bridge_status` encodings for AHK/P (paritytech#2350) * Added `XcmBridgeHubRouterCall::report_bridge_status` encodings for AHK/P * Spellcheck * Added const for `XcmBridgeHubRouterTransactCallMaxWeight` * Cargo.lock * Introduced base delivery fee constants * Congestion messages as Optional to turn on/off `supports_congestion_detection` * Spellcheck * Ability to externalize dest for benchmarks * Ability to externalize dest for benchmarks --------- Co-authored-by: Branislav Kontur <bkontur@gmail.com> Co-authored-by: command-bot <>
* impl backpressure in the XcmBlobHaulerAdapter * LocalXcmQueueManager + more adapters * OnMessageDelviered callback * forbid mesage delivery transactions when the channel between target bridge hub and target asset hub is suspended * pallet-xcm-bridge-hub-router * removed commented code * improvements and tests for palle-xcm-bridge-router * use LocalXcmChannel in XcmBlobMessageDispatch * new tests for logic changes in messages pallet * tests for LocalXcmQueueSuspender * tests for LocalXcmQueueMessageProcessor * tests for new logic in the XcmBlobHaulerAdapter * fix other tests in the bridge-runtime-common * extension_reject_call_when_dispatcher_is_inactive * benchmarks for pallet-xcm-bridge-hub-router * get rid of redundant storage value * add new pallet to verify-pallets-build.sh * fixing spellcheck, clippy and rustdoc * trigger CI * Revert "trigger CI" This reverts commit 48f1ba0. * change log target for xcm bridge router pallet * Update modules/xcm-bridge-hub-router/src/lib.rs Co-authored-by: Branislav Kontur <bkontur@gmail.com> * use saturated_len where possible * fmt * (Suggestion) Ability to externalize configuration for `ExporterFor` (paritytech#2313) * Ability to externalize configuration for `ExporterFor` (Replaced `BridgedNetworkId/SiblingBridgeHubLocation` with `Bridges: ExporterFor`) * Fix millau * Compile fix * Return back `BridgedNetworkId` but as optional filter * Replaced `BaseFee` with fees from inner `Bridges: ExporterFor` * typo * Clippy * Rename LocalXcmChannel to XcmChannelStatusProvider (paritytech#2319) * Rename LocalXcmChannel to XcmChannelStatusProvider * fmt * added/fixed some docs * Dynamic fees v1: report congestion status to sending chain (paritytech#2318) * report congestion status: changes at the sending chain * OnMessagesDelivered is back * report congestion status: changes at the bridge hub * moer logging * fix? benchmarks * spelling * tests for XcmBlobHaulerAdapter and LocalXcmQueueManager * tests for messages pallet * fix typo * rustdoc * Update modules/messages/src/lib.rs * apply review suggestions * ".git/.scripts/commands/fmt/fmt.sh" * Added `XcmBridgeHubRouterCall::report_bridge_status` encodings for AHK/P (paritytech#2350) * Added `XcmBridgeHubRouterCall::report_bridge_status` encodings for AHK/P * Spellcheck * Added const for `XcmBridgeHubRouterTransactCallMaxWeight` * Cargo.lock * Introduced base delivery fee constants * Congestion messages as Optional to turn on/off `supports_congestion_detection` * Spellcheck * Ability to externalize dest for benchmarks * Ability to externalize dest for benchmarks --------- Co-authored-by: Branislav Kontur <bkontur@gmail.com> Co-authored-by: command-bot <>
https://gist.github.com/svyatonik/6d4d9f58b800ac951dcac6c1aa827c67#file-bridgequeues-png
This PR introduces all logic and components required for dynamic fees in v1, that we may build in this repo:
XcmBlobMessageDispatch
and messages pallet;XcmBlobHaulerAdapter
,LocalXcmQueueManager
,LocalXcmQueueMessageProcessor
andLocalXcmQueueSuspender
;pallet-xcm-bridge-hub-router
. Now it exists only to support dynamic fees. In the future, we'll (probably) add a support for dynamic bridges to it. Dynamic fee is based on XCM: Properly set the pricing for the DMP router polkadot#6843. We start increasing fee factor when the channel between sending asset hub and its sibling bridge hub is suspended and/or has many queued messages.TODOs in this PR:
add support of delivery transaction logic changes to our relayI'd like to do it in a separate PR. It isn't actually necessary, because signed extensions will do the job, but desirable to make relayer "polite".But before investing a time in that ^^^, I'd like to check if all my assumptions are correct and test it using local chains. For that, we need:
Message Queue
as DMP and XCMP dispatch queue cumulus#2157 (started work on that: https://github.com/paritytech/cumulus/compare/sv-bridges-and-new-message-queue: this branch contains updated bridges subtree (this PR) +oty-message-queue
+bko-transfer-asset-via-bridge-pallet-xcm
+bridge-hub-kusama-polkadot
);IsChannelActive
for outbound part of XCM queue between target BH and target AH;LocalXcmQueue
for outbound part of XCM queue between source AH and source BH;