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

Xcm bridge hub router v2 (backport to master branch) #2312

Merged
merged 5 commits into from
Aug 1, 2023

Conversation

svyatonik
Copy link
Contributor

related to #2434

There are 3 commits in this PR. First two just copy the pallet and associated changes (mostly benchmark-related) from #2294. So the pallet is copied 1:1 and is already reviewed in context of v1.

The last commit is adding a (detailed) comment on why the pallet is copied without any changes, even though we are going to support multiple bridges in v2. So my initial idea is that we'll have a separate fee factor for every bridge, opened by this chain (see for example https://github.com/paritytech/parity-bridges-common/pull/2233/files#diff-195506c72a3b56acee36eb21626cda8dc593091644ecd920f9eb9bf57c8d20bdR78), but it was designed assuming queues-state-report dynamic fees mechanism in mind. So we were able to estimate number of messages in every bridge pipeline separately. Now, however, all bridges are simply activating backpressure on the XCM chanel between this (sending) chain and sibling/child bridge hub when one of queues is congested. So at this chain we are unable to distinguish between different bridges. So we are using the same fee factor for all bridges.

From user (senders) perspective it isn't ideal, but it is a tradeoff between complicated queues-state-report and a simple backpressure-based dynamic fees implementation. There can be an alternative implementation of this pallet if sending chain is going to support multiple bridges at once and want to make them cheaper. I'm thinking of having something like per-bridge local messages scheduling and sending those messages in a round robin fashion. This will guarantee that the local XCM channel won't be congested because of multiple active bridges (given that all other queues are working fine) and the pallet will be able to compute dynamic fee on per-bridge basis. However, I'm not going to implement that until some strong demand.

@svyatonik svyatonik added P-Runtime PR-audit-needed A PR has to be audited before going live. labels Aug 1, 2023
@svyatonik svyatonik requested a review from a team as a code owner August 1, 2023 08:26
@svyatonik svyatonik removed the request for review from a team August 1, 2023 08:26
message_size,
);

Some((T::SiblingBridgeHubLocation::get(), Some((T::FeeAsset::get(), fee).into())))
Copy link
Collaborator

Choose a reason for hiding this comment

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

@svyatonik

Sorry, late question. I was looking on this while working on #2554.

If I understand correctly, the fee that we set here will be used through the ViaBridgeHubExporter -> SovereignPaidRemoteExporter here:

				WithdrawAsset(fees.clone().into()),
				BuyExecution { fees, weight_limit: Unlimited },
				export_instruction,
				RefundSurplus,
				DepositAsset { assets: All.into(), beneficiary: local_from_bridge },

But what will happen to it exactly ? BuyExecution, doesn't seem to do anything with it (here) so I suppose that it will be deposited to the sovereign account of the asset hub on the bridge hub when DepositAsset { assets: All.into(), beneficiary: local_from_bridge } is executed ?

Did I understand correctly ? And is this the desired behavior ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@serban300 IIRC Unlimited is changed to actual weight of the XCM program by AllowTopLevelPaidExecutionFrom barrier. So idea behind that is following:

  1. here, at asset hub, user pays fee that covers both XCM execution at sibling BH (the weight we're talking here) + some extra;
  2. when message is dispatched at the sibling BH, xcm executor computes the actual weight of the message and "sends" it to the AllowTopLevelPaidExecutionFrom;
  3. then the AllowTopLevelPaidExecutionFrom changes Unlimited to actual weight, so we'll be charging the SA of AH at BH in the end. But without that extra that was paid by the user at the AH.

Please correct me here, if you see any mistake - I was checking it long time ago, so could lie (accidentally) here :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh you're right. Sorry, I forgot about AllowTopLevelPaidExecutionFrom. So the weight of the message is indeed charged.

But what about the extra amount ? Will it be deposited to the SA of AH at BH ? Especially if the bridge queue is congested and we apply a high fee factor at the AH, the extra amount can be significant.

@svyatonik
Copy link
Contributor Author

But what about the extra amount ? Will it be deposited to the SA of AH at BH ? Especially if the bridge queue is congested and we apply a high fee factor at the AH, the extra amount can be significant.

It won't be deposited to the SA of AH at BH. It is paid and deposited to where configured at the AH itself. So message sender pays this whole fee to the AH at AH. Then we wrap XCM program into WithdrawAsset(fees), BuyExecution(fees), ..., RefundSurplus, DepositAsset construction. So what would happen at the BH is that the total fees will be withdrawn form SA of AH to the holding register and then we'll only spend basic (weight) fee component of those fees and deposit remaining fees (extra) back to the SA of AH at BH.

So in the end what happens with local transport fees:

  1. message sender pays base + extra to the AH at AH. So message sender account balance is decreased, and AH account (treasury) is increased by the same amount;
  2. SA of AH at BH is decreased by base for every message sent through the bridge. That's why we need to replenish it preiodically.

In addition, there are also accounts, used by the relayers pallet at AH, but let's not talk about them now

@serban300
Copy link
Collaborator

Oh yes, you're right. Thanks for the details !

bkontur pushed a commit that referenced this pull request May 7, 2024
* copy new pallet (palle-xcm-bridge-hub-router) from dynamic-fees-v1 branch

* added remaining traces of pallet-xcm-bridge-hub-router

* added comment about sharing delivery fee factor between all bridges, opened by this chain

* spelling

* clippy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-Runtime PR-audit-needed A PR has to be audited before going live.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants