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.

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 v1: report congestion status to sending chain #2318

Merged
merged 14 commits into from
Aug 4, 2023

Conversation

svyatonik
Copy link
Contributor

@svyatonik svyatonik commented Aug 3, 2023

closes #2315

TODO:

What this PR does:

  1. instead of physically suspending the XCM channel Source.AH -> Source.BH, we are sending the "congested" message from Source.BH to Source.AH when the lane has more than OUTBOUND_LANE_CONGESTED_THRESHOLD messages;
  2. upon receiving this message, the Source.AH starts increasing fee for every bridge message;
  3. once oubound lane has more than OUTBOUND_LANE_UNCONGESTED_THRESHOLD messages, it sends the "uncongested" message to the Source.AH;
  4. upon receiving this message, the Source.AH starts relieving the fee factor.

Obviously there's a period when the "congested" message is sent to the Source.AH, but is not yet dispatched. So messages may be sent with a constant (previous) fee. This may be alleviated by e.g. sending a ping message when OUTBOUND_LANE_CONGESTED_THRESHOLD messages are sent and if pong is not received in N blocks, start increasing the fee. This is what has been done in the #2233 initially. IDK if we need to do that, because:

  1. the HRMP cost (which is a part of fee) still has its own exponential fee, which would make messages more expensive;
  2. I've added another condition to start increasing the exponential fee - if inbound HRMP queue from Source.BH to Source.AH is suspended (meaning that Source.AH is failing to process inbound messages in a timely manner), then we trigger our own exponential fee increase mechanism.

So to me it looks like the above is enough for us, because it'll be expensive to fill up both queues (Source.AH -> Source.BH and Source.BH -> Source.AH). Also it needs to be coordinated carefully, so that the Source.BH -> Source.AH won't trigger the channel suspension. But please share your thoughts

UPD: in other words, with current implementation, we accept the fact that if the Source.BH -> Source.AH XCM channel is NOT congested, bridge messages may be sent with constant bridge fee (although HRMP fee may apply its own exponential factor meanwhile) while "congested" message is being delivered. We may also change the is_congested impl a bit and look at the number of unprocessed Source.BH -> Source AH messages and if it is larger than some N, consider it congested. The N may be for example the average number of messages, processed at a single Source.AH block.

@bkontur bkontur mentioned this pull request Aug 3, 2023
9 tasks
return T::WeightInfo::on_initialize_when_congested()
}
Bridge::<T, I>::mutate(|bridge| {
// TODO: make sure that `WithBridgeHubChannel::is_congested` returns true if either
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Relevant commit: paritytech/cumulus@c0e3225. I'll remove this TODO before merging

@svyatonik svyatonik marked this pull request as ready for review August 4, 2023 05:26
@bkontur
Copy link
Contributor

bkontur commented Aug 4, 2023

@svyatonik
just nit, in your Cumulus branch, I have never seen CumulusXcm used anywhere, I guess this module is really not needed and maybe will be removed,
and for matching XCM origin (e.g. from sibling parachain for example for ForeignCreatetors) we used something like this:

                let origin_location = EnsureXcm::<Everything>::try_origin(origin.clone())?;
		if origin_location.eq(BridgeHubKusama::get()) {
  ....
                }

@svyatonik
Copy link
Contributor Author

CumulusXcm

@bkontur Which code exactly are you talking about? Could you, please, share a link?

@bkontur
Copy link
Contributor

bkontur commented Aug 4, 2023

CumulusXcm

@bkontur Which code exactly are you talking about? Could you, please, share a link?

paritytech/cumulus@bridge-hub-kusama-polkadot...sv-bridges-and-new-message-queue#diff-9a51956f7781a349d207a9d5f54ea0061666a856fb108d1a1190bd5178eaac50R647-R657

hmm, I am sure that I saw there CumulusXcm somewhere :D :D , nevermind :D

modules/messages/src/lib.rs Outdated Show resolved Hide resolved
modules/messages/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@serban300 serban300 left a comment

Choose a reason for hiding this comment

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

Looks good. Just left a few small comments

modules/xcm-bridge-hub-router/src/lib.rs Show resolved Hide resolved
modules/xcm-bridge-hub-router/src/benchmarking.rs Outdated Show resolved Hide resolved
bin/runtime-common/src/messages_xcm_extension.rs Outdated Show resolved Hide resolved
bin/runtime-common/src/messages_xcm_extension.rs Outdated Show resolved Hide resolved
enqueued_messages,
);

if let Err(e) = Self::send_congested_signal(sender_and_lane) {
Copy link
Contributor

@bkontur bkontur Aug 4, 2023

Choose a reason for hiding this comment

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

maybe also to trigger some event here, I know that inner send_xcm creates one,
but maybe could be useful for later analyse if something happens - https://github.com/paritytech/parity-bridges-common/issues/2323

Copy link
Contributor

@bkontur bkontur left a comment

Choose a reason for hiding this comment

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

lgmt

@svyatonik svyatonik merged commit f822ebc into dynamic-fees-v1 Aug 4, 2023
15 checks passed
@svyatonik svyatonik deleted the report-bridge-congestion-to-sending-chain branch August 4, 2023 13:14
bkontur added a commit that referenced this pull request Aug 16, 2023
* 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` (#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 (#2319)

* Rename LocalXcmChannel to XcmChannelStatusProvider

* fmt

* added/fixed some docs

* Dynamic fees v1: report congestion status to sending chain (#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 (#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 <>
bkontur added a commit to paritytech/polkadot-sdk that referenced this pull request Sep 5, 2023
…upport

Add congestion detection to the Bridge Hub runtimes and report congestion status to the sending chain.

Bridge Hub's `ExportMessage` handling is extended with check if outbound message queue is congested,
if so then `CongestedMessage` signal is sent to the sending chain's relevant `pallet-xcm-bridge-hub-router` pallet instance,
where dynamic fees factor is processed.
When then same Bridge Hub receives message delivery confirmation, there is a another check is outbound queue is still congested,
if not then `UncongestedMessage` signal is sent to the sending chain's relevant `pallet-xcm-bridge-hub-router` pallet instance.

`pallet-bridge-messages`'s `receive_messages_proof` does another check for congestion or back-preassure
with checking status of underlaying XCMP queue (`cumulus_pallet_xcmp_queue::bridging::OutboundXcmpChannelCongestionStatusProvider`).
If we cannot deliver a message to the target, then `receive_messages_proof` returns error and Bridge Hub does not allow to receive new bridged messages.

More about congestion detection [here](paritytech/parity-bridges-common#2318).

Co-authored-by: Branislav Kontur <bkontur@gmail.com>
Co-authored-by: Adrian Catangiu <adrian@parity.io>
Co-authored-by: Svyatoslav Nikolsky <svyatonik@gmail.com>
Co-authored-by: Serban Iorga <serban@parity.io>

Signed-off-by: Branislav Kontur <bkontur@gmail.com>
Signed-off-by: Adrian Catangiu <adrian@parity.io>
Signed-off-by: Svyatoslav Nikolsky <svyatonik@gmail.com>
Signed-off-by: Serban Iorga <serban@parity.io>
bkontur added a commit to paritytech/polkadot-sdk that referenced this pull request Sep 5, 2023
…upport

Add congestion detection to the Bridge Hub runtimes and report congestion status to the sending chain.

Bridge Hub's `ExportMessage` handling is extended with check if outbound message queue is congested,
if so then `CongestedMessage` signal is sent to the sending chain's relevant `pallet-xcm-bridge-hub-router` pallet instance,
where dynamic fees factor is processed.
When then same Bridge Hub receives message delivery confirmation, there is a another check is outbound queue is still congested,
if not then `UncongestedMessage` signal is sent to the sending chain's relevant `pallet-xcm-bridge-hub-router` pallet instance.

`pallet-bridge-messages`'s `receive_messages_proof` does another check for congestion or back-preassure
with checking status of underlaying XCMP queue (`cumulus_pallet_xcmp_queue::bridging::OutboundXcmpChannelCongestionStatusProvider`).
If we cannot deliver a message to the target, then `receive_messages_proof` returns error and Bridge Hub does not allow to receive new bridged messages.

More about congestion detection [here](paritytech/parity-bridges-common#2318).

Signed-off-by: Branislav Kontur <bkontur@gmail.com>
Signed-off-by: Adrian Catangiu <adrian@parity.io>
Signed-off-by: Svyatoslav Nikolsky <svyatonik@gmail.com>
Signed-off-by: Serban Iorga <serban@parity.io>

Co-authored-by: Adrian Catangiu <adrian@parity.io>
Co-authored-by: Svyatoslav Nikolsky <svyatonik@gmail.com>
Co-authored-by: Serban Iorga <serban@parity.io>
bkontur added a commit to paritytech/polkadot-sdk that referenced this pull request Sep 5, 2023
…upport

Add congestion detection to the Bridge Hub runtimes and report congestion status to the sending chain.

Bridge Hub's `ExportMessage` handling is extended with check if outbound message queue is congested,
if so then `CongestedMessage` signal is sent to the sending chain's relevant `pallet-xcm-bridge-hub-router` pallet instance,
where dynamic fees factor is processed.
When then same Bridge Hub receives message delivery confirmation, there is a another check is outbound queue is still congested,
if not then `UncongestedMessage` signal is sent to the sending chain's relevant `pallet-xcm-bridge-hub-router` pallet instance.

`pallet-bridge-messages`'s `receive_messages_proof` does another check for congestion or back-preassure
with checking status of underlaying XCMP queue (`cumulus_pallet_xcmp_queue::bridging::OutboundXcmpChannelCongestionStatusProvider`).
If we cannot deliver a message to the target, then `receive_messages_proof` returns error and Bridge Hub does not allow to receive new bridged messages.

More about congestion detection [here](paritytech/parity-bridges-common#2318).

Signed-off-by: Branislav Kontur <bkontur@gmail.com>
Signed-off-by: Adrian Catangiu <adrian@parity.io>
Signed-off-by: Svyatoslav Nikolsky <svyatonik@gmail.com>
Signed-off-by: Serban Iorga <serban@parity.io>

Co-authored-by: Adrian Catangiu <adrian@parity.io>
Co-authored-by: Svyatoslav Nikolsky <svyatonik@gmail.com>
Co-authored-by: Serban Iorga <serban@parity.io>
claravanstaden pushed a commit to Snowfork/polkadot-sdk that referenced this pull request Sep 19, 2023
…upport

Add congestion detection to the Bridge Hub runtimes and report congestion status to the sending chain.

Bridge Hub's `ExportMessage` handling is extended with check if outbound message queue is congested,
if so then `CongestedMessage` signal is sent to the sending chain's relevant `pallet-xcm-bridge-hub-router` pallet instance,
where dynamic fees factor is processed.
When then same Bridge Hub receives message delivery confirmation, there is a another check is outbound queue is still congested,
if not then `UncongestedMessage` signal is sent to the sending chain's relevant `pallet-xcm-bridge-hub-router` pallet instance.

`pallet-bridge-messages`'s `receive_messages_proof` does another check for congestion or back-preassure
with checking status of underlaying XCMP queue (`cumulus_pallet_xcmp_queue::bridging::OutboundXcmpChannelCongestionStatusProvider`).
If we cannot deliver a message to the target, then `receive_messages_proof` returns error and Bridge Hub does not allow to receive new bridged messages.

More about congestion detection [here](paritytech/parity-bridges-common#2318).

Signed-off-by: Branislav Kontur <bkontur@gmail.com>
Signed-off-by: Adrian Catangiu <adrian@parity.io>
Signed-off-by: Svyatoslav Nikolsky <svyatonik@gmail.com>
Signed-off-by: Serban Iorga <serban@parity.io>

Co-authored-by: Adrian Catangiu <adrian@parity.io>
Co-authored-by: Svyatoslav Nikolsky <svyatonik@gmail.com>
Co-authored-by: Serban Iorga <serban@parity.io>
serban300 pushed a commit to serban300/parity-bridges-common that referenced this pull request Mar 27, 2024
* 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 <>
serban300 pushed a commit to serban300/parity-bridges-common that referenced this pull request Apr 8, 2024
* 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 <>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants