[pallet-xcm] Adjust benchmarks (teleport_assets/reserve_transfer_assets) not relying on ED (patch for 1.7.0) #3465
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ts) not relying on ED
|
The CI pipeline was cancelled due to failure one of the required jobs. |
Merged
3 tasks
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
acatangiu
approved these changes
Feb 26, 2024
github-merge-queue bot
pushed a commit
that referenced
this pull request
Feb 26, 2024
…ts) not relying on ED (#3464) ## Problem During the bumping of the `polkadot-fellows` repository to `polkadot-sdk@1.6.0`, 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](https://github.com/polkadot-fellows/runtimes/pull/158/files#diff-80668ff8e793b64f36a9a3ec512df5cbca4ad448c157a5d81abda1b15f35f1daR213), and also because of a [missing CI pipeline](polkadot-fellows/runtimes#197) 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 - [ ] patch for 1.6 - #3466 - [ ] patch for 1.7 - #3465 - [ ] patch for 1.8 - TODO: PR --------- Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
9 tasks
bkontur
added a commit
to bkontur/runtimes
that referenced
this pull request
Feb 28, 2024
fellowship-merge-bot bot
pushed a commit
to polkadot-fellows/runtimes
that referenced
this pull request
Mar 5, 2024
Based on bump to [`polkadot-sdk@1.6.0`](#159). Attached result of `cargo upgrade -v --pinned --incompatible` [cargo-upgrade-version-bump.log](https://github.com/polkadot-fellows/runtimes/files/14281072/cargo-upgrade-version-bump.log) ~~## For reviewers~~ ~~This PR is against `polkadot-fellows`'s main to bring it to the fellows repo, but if you want to see a real diff relevant to the `polkadot-sdk@1.7.0` update please check: bkontur/runtimes@bko-bump-to-1.6...bkontur:runtimes:bko-bump-to-1.7.~~ ## TODO - [x] fix compilation - [x] apply patch for AssetHub tests: paritytech/polkadot-sdk#3404 - [x] apply patch for `coretime` paritytech/polkadot-sdk#3497 see [comment](#187 (comment)) - [x] fix integration tests - fix after #185 - [x] fix benchmarks (also try them) - [x] apply patch for `pallet-xcm` benchmarks paritytech/polkadot-sdk#3465 - [x] check ranked collectives: - [x] `ExchangeOrigin` see [comment](#187 (comment)) - [x] `MemberSwappedHandler` / `BenchmarkSetup` see [comment](#187 (comment)) --------- Signed-off-by: Adrian Catangiu <adrian@parity.io> Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com> Co-authored-by: Ross Bulat <ross@parity.io> Co-authored-by: Alain Brenzikofer <alain@integritee.network> Co-authored-by: eskimor <eskimor@no-such-url.com> Co-authored-by: Adrian Catangiu <adrian@parity.io> Co-authored-by: brenzi <brenzi@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For description, please see: #3464
Expected patches for (1.7.0):
0.8.10.8.18.0.18.0.28.0.18.0.1