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

[pallet-xcm] Adjust benchmarks (teleport_assets/reserve_transfer_assets) not relying on ED #3464

Merged
merged 7 commits into from
Feb 26, 2024

Conversation

bkontur
Copy link
Contributor

@bkontur bkontur commented Feb 23, 2024

Problem

During the bumping of the polkadot-fellows repository to [email protected], I encountered a situation where the benchmarks teleport_assets and reserve_transfer_assets in AssetHubKusama started to fail. This issue arose due to a decreased ED balance for AssetHubs introduced here, and also because of a missing CI pipeline to check the benchmarks, which went unnoticed.

These benchmarks expect the caller to have enough:

  1. balance to transfer (BTT)
  2. balance for paying delivery (BFPD).

So the initial balance was calculated as ED * 100, which seems reasonable:

const ED_MULTIPLIER: u32 = 100;
let balance = existential_deposit.saturating_mul(ED_MULTIPLIER.into());`

The problem arises when the price for delivery is 100 times higher than the existential deposit. In other words, when ED * 100 does not cover BTT + BFPD.

I check AHR/AHW/AHK/AHP and this problem has only AssetHubKusama

ED: 3333333
calculated price to parent delivery:  1031666634  (from xcm logs from the benchmark)
---

3333333 * 100 - BTT(3333333) - BFPD(1031666634) = −701666667

which results in the error;

2024-02-23 09:19:42 Unable to charge fee with error Module(ModuleError { index: 31, error: [17, 0, 0, 0], message: Some("FeesNotMet") })
Error: Input("Benchmark pallet_xcm::reserve_transfer_assets failed: FeesNotMet")
     

Solution

The benchmarks teleport_assets and reserve_transfer_assets were fixed by removing ED * 100 and replacing it with DeliveryHelper logic, which calculates the (almost real) price for delivery and sets it along with the existential deposit as the initial balance for the account used in the benchmark.

TODO

Copy link
Contributor

@franciscoaguirre franciscoaguirre 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

@acatangiu acatangiu added this pull request to the merge queue Feb 26, 2024
acatangiu pushed a commit that referenced this pull request Feb 26, 2024
…ts) not relying on ED (patch for 1.7.0) (#3465)

For description, please see:
#3464

Expected patches for (1.6.0):
- cumulus-pallet-parachain-system `0.8.1`
- cumulus-primitives-utility `0.8.1`
- polkadot-runtime-common `8.0.1`
- pallet-xcm-benchmarks `8.0.2`
- pallet-xcm `8.0.0`
- staging-xcm-builder `8.0.0`

---------

Co-authored-by: Francisco Aguirre <[email protected]>
Merged via the queue into master with commit 3d9439f Feb 26, 2024
130 of 131 checks passed
@acatangiu acatangiu deleted the bko-adjust-benchmarks-pallet-xcm branch February 26, 2024 08:37
ordian added a commit that referenced this pull request Feb 28, 2024
…head-data

* origin/master: (51 commits)
  Runtime Upgrade ref docs and Single Block Migration example pallet  (#1554)
  Collator overseer builder unification (#3335)
  Introduce storage attr macro `#[disable_try_decode_storage]` and set it on `System::Events` and `ParachainSystem::HostConfiguration` (#3454)
  Add Polkadotters bootnoders per IBP application (#3423)
  Add documentation around FRAME Origin (#3362)
  Bridge zombienet tests: Check amount received at destination (#3490)
  Snowbridge benchmark tests fix (#3424)
  fix(zombienet): increase timeout in download artifacts (#3376)
  Cleanup String::from_utf8 (#3446)
  [prdoc] Validate crate names (#3467)
  Limit max execution time for `test-linux-stable` CI jobs (#3483)
  Introduce Notification block pinning limit (#2935)
  frame-support: Improve error reporting when having too many pallets (#3478)
  add Encointer as trusted teleporter for Westend (#3411)
  [pallet-xcm] Adjust benchmarks (teleport_assets/reserve_transfer_assets) not relying on ED (#3464)
  Add more debug logs to understand if statement-distribution misbehaving (#3419)
  Remove redundant parachains assigner pallet. (#3457)
  Use generic hash for runtime wasm in resolve_state_version_from_wasm (#3447)
  Runtime: allow backing multiple candidates of same parachain on different cores  (#3231)
  Bridge zombienet tests: move all "framework" files under one folder (#3462)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes T6-XCM This PR/Issue is related to XCM.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants