Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Increase xcm fees when busy #2342

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gilescope
Copy link
Contributor

@gilescope gilescope commented Mar 17, 2023

This is the corollary to paritytech/polkadot#6843 . Xcmp fees upwards and horizontally get exponentially more expensive as the queues increase.

TODO:

  • broadenend out from just statemine

@paritytech-ci paritytech-ci requested review from a team March 17, 2023 09:57
@gilescope gilescope added A3-in_progress Pull request is in progress. No review needed at this stage. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Mar 17, 2023
@@ -1069,7 +1085,7 @@ impl<T: Config> Pallet<T> {
match Self::host_configuration() {
Some(cfg) =>
if message.len() > cfg.max_upward_message_size as usize {
return Err(MessageSendError::TooBig)
Self::increment_ump_fee_factor();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the wrong place to put the increment. The condition here checks whether the individual message size is greater than the limit, and does not check for the queue size.

@@ -1129,6 +1163,9 @@ impl<T: Config> XcmpMessageSource for Pallet<T> {

<OutboundXcmpStatus<T>>::put(statuses);

if !result.is_empty() {
Copy link
Contributor

Choose a reason for hiding this comment

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

It should check whether the number of messages in queue is less than the maximum limit.

@@ -261,6 +264,9 @@ pub mod pallet {

UpwardMessages::<T>::put(&up[..num]);
*up = up.split_off(num);
if num > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should check whether the UMP queue is less than then max limit.

@xlc
Copy link
Contributor

xlc commented Mar 18, 2023

Please make sure there is an easy way to estimate XCM fee paritytech/polkadot-sdk#690

Also please consider the scenario that the XCM is triggered indirectly. For example, from a smart contract. The smart contract will pay for the XCM fee so it will need to know how much to charge from the user for the XCM fee.

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/cumulus/-/jobs/2546897

@gilescope
Copy link
Contributor Author

This just does not translate well from the polkadot dmp PR. The polkadot DMP pallet does not page, but holds a list of messages where as the xcmp-queue holds pages of messages. Establishing how many messages are in a page when it gets sent would require decoding all the page or keeping a tally separately, so it's probably nicer to bump the xcm fees based on the number of pages queued up which is readily available information. I.e. once one page is full then xcm messages get more expensive, and once two pages get full then the channel suspends while it clears down (2 pages is the current suspendThreshold).

@@ -475,7 +477,8 @@ impl cumulus_pallet_xcmp_queue::Config for Runtime {
>;
type ControllerOriginConverter = xcm_config::XcmOriginToTransactDispatchOrigin;
type WeightInfo = weights::cumulus_pallet_xcmp_queue::WeightInfo<Runtime>;
type PriceForSiblingDelivery = ();
type PriceForSiblingDelivery =
SiblingExponentialPrice<FeeAssetId, BaseDeliveryFee, TransactionByteFee, XcmpFeeFactor>;
Copy link
Contributor

Choose a reason for hiding this comment

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

when paritytech/polkadot#7005 and paritytech/polkadot#7585,
I am goging to reuse here lots of stuff from runtime/common e.g. ExponentialPrice / PriceForParachainDelivery ...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A3-in_progress Pull request is in progress. No review needed at this stage. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
Development

Successfully merging this pull request may close these issues.

5 participants