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.

Sign up for GitHub

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

Jump to bottom

XCM Weights/Benchmarks TODOs #1132

Closed
2 tasks
bkontur opened this issue Dec 8, 2022 · 5 comments · Fixed by paritytech/cumulus#2934 or #1726
Closed
2 tasks

XCM Weights/Benchmarks TODOs #1132

bkontur opened this issue Dec 8, 2022 · 5 comments · Fixed by paritytech/cumulus#2934 or #1726

Comments

@bkontur
Copy link
Contributor

bkontur commented Dec 8, 2022 •
edited
Loading

TODO

  • parachains/runtimes/assests/ - check fix fn reserve_asset_deposited( weight correctly, now is hardcoded 1_000_000_000
  • search for // Hardcoded till the XCM pallet is fixed and fix
@bkontur
Copy link
Contributor Author

bkontur commented Dec 8, 2022 •
edited
Loading

@KiChjang
maybe related to #900

@bkontur bkontur changed the title XCM Weights for reserve_asset_deposited XCM Weights/Benchmarks TODOs Dec 8, 2022
@KiChjang
Copy link
Contributor

KiChjang commented Dec 9, 2022

IIRC the first item is hardcoded that way in order to make the benchmarking of reserve_transfer_assets pass in the XCM pallet. That should not be necessary anymore, and so if you simply revert it back to the value of Weight::MAX, it should still just work.

@bkontur
Copy link
Contributor Author

bkontur commented Dec 13, 2022

@KiChjang
I tried, but if I use Weight::MAX then pallet_xcm::reserve_transfer_assets fails:

     Running `target/release/polkadot-parachain benchmark pallet --steps=50 --repeat=20 '--extrinsic=*' --execution=wasm --wasm-execution=compiled --heap-pages=4096 --json-file=/var/lib/gitlab-runner/builds/zyw4fam_/0/parity/mirrors/cumulus/.git/.artifacts/bench.json --pallet=pallet_xcm --chain=statemine-dev --header=./file_header.txt --output=./parachains/runtimes/assets/statemine/src/weights/xcm`
2022-12-13 17:07:10 assembling new collators for new session 0 at #0    
2022-12-13 17:07:10 assembling new collators for new session 1 at #0    
Error: Input("Benchmark pallet_xcm::reserve_transfer_assets failed: UnweighableMessage")
2022-12-13 17:07:11 Starting benchmark: pallet_xcm::send    
2022-12-13 17:07:11 Starting benchmark: pallet_xcm::teleport_assets    
2022-12-13 17:07:11 Starting benchmark: pallet_xcm::reserve_transfer_assets

bkontur referenced this issue in paritytech/cumulus Jul 25, 2023
@bkontur
Fix for reserve_asset_deposited (https://github.com/paritytech/cumulu…
23a5125 
…s/issues/1974)
@bkontur
Copy link
Contributor Author

bkontur commented Jul 25, 2023 •
edited
Loading

first point is finally fixed by paritytech/cumulus#2934

@bkontur
Copy link
Contributor Author

bkontur commented Jul 25, 2023 •
edited
Loading

@franciscoaguirre
in Cumulus there is one old hard-coded hack - check the second point from descirption:

fn deposit_asset(assets: &MultiAssetFilter, _dest: &MultiLocation) -> Weight {
		// Hardcoded till the XCM pallet is fixed
		let hardcoded_weight = Weight::from_parts(1_000_000_000_u64, 0);
		let weight = assets.weigh_multi_assets(XcmFungibleWeight::<Runtime>::deposit_asset());
		hardcoded_weight.min(weight)
	}

maybe we could get rid of that, do you have any idea how?

maybe relates to paritytech/polkadot#7077

@bkontur bkontur mentioned this issue Jul 25, 2023
Merged
paritytech-processbot bot referenced this issue in paritytech/cumulus Jul 26, 2023
@bkontur
[xcm ] Compatibility fix for xcm benchmarks (#2934)
7d51356 
* Compatibility fix for xcm benchmarks (cause of missing companion)

* ".git/.scripts/commands/bench/bench.sh" --subcommand=xcm --runtime=asset-hub-polkadot --runtime_dir=assets --target_dir=cumulus --pallet=pallet_xcm_benchmarks::fungible

* Fix for reserve_asset_deposited (https://github.com/paritytech/cumulus/issues/1974)

* Revert back hard-coded 1_000_000_000_u64 for `reserve_asset_deposited` with TODOs

* ".git/.scripts/commands/bench/bench.sh" --subcommand=xcm --runtime=asset-hub-kusama --runtime_dir=assets --target_dir=cumulus --pallet=pallet_xcm_benchmarks::fungible

---------

Co-authored-by: command-bot <>
@bkontur bkontur reopened this Jul 26, 2023
@EmmanuellNorbertTulbure EmmanuellNorbertTulbure transferred this issue from paritytech/cumulus Aug 25, 2023
@the-right-joyce the-right-joyce transferred this issue from paritytech/polkadot-sdk Aug 25, 2023
@the-right-joyce the-right-joyce transferred this issue from paritytech/parity-bridges-common Aug 25, 2023
@bkontur bkontur mentioned this issue Sep 27, 2023
Merged
5 tasks
bkontur added a commit that referenced this issue Oct 10, 2023
@bkontur
[xcm] Use Weight::MAX for reserve_asset_deposited, `receive_telep…
e3c97e4 
…orted_asset` benchmarks (#1726)

# Description

## Summary

Previously, the `pallet_xcm::do_reserve_transfer_assets` and
`pallet_xcm::do_teleport_assets` functions relied on weight estimation
for remote chain execution, which was based on guesswork derived from
the local chain. This approach led to complications for runtimes that
did not provide or support specific [XCM
configurations](https://github.com/paritytech/polkadot-sdk/blob/7cbe0c76ef8fd2aabf9f07de0156941ce3ed44b0/polkadot/xcm/xcm-executor/src/config.rs#L43-L47)
for `IsReserve` or `IsTeleporter`. Consequently, such runtimes had to
resort to implementing hard-coded weights for XCM instructions like
`reserve_asset_deposited` or `receive_teleported_asset` to support
extrinsics such as `pallet_xcm::reserve_transfer_assets` and
`pallet_xcm::teleport_assets`, which depended on remote weight
estimation.

The issue of remote weight estimation was addressed and resolved by
[Pull Request
#1645](#1645), which
removed the need for remote weight estimation.

## Solution

As a continuation of this improvement, the current PR proposes further
cleanup by removing unnecessary hard-coded values and rectifying
benchmark results with `Weight::MAX` that previously used
`T::BlockWeights::get().max_block` as an override for unsupported XCM
instructions like `ReserveAssetDeposited` and `ReceiveTeleportedAsset`.


## Questions

- [x] Can we remove now also `Hardcoded till the XCM pallet is fixed`
for `deposit_asset`? E.g. for AssetHubKusama
[here](https://github.com/paritytech/polkadot-sdk/blob/7cbe0c76ef8fd2aabf9f07de0156941ce3ed44b0/cumulus/parachains/runtimes/assets/asset-hub-kusama/src/weights/xcm/mod.rs#L129-L134)
- [x] Are comments like
[this](https://github.com/paritytech/polkadot-sdk/blob/7cbe0c76ef8fd2aabf9f07de0156941ce3ed44b0/polkadot/runtime/kusama/src/weights/xcm/mod.rs#L94)
`// Kusama doesn't support ReserveAssetDeposited, so this benchmark has
a default weight` still relevant? Shouldnt be removed/changed?


## TODO

- [x] `bench bot` regenerate xcm weights for all runtimes
- [x] remove hard-coded stuff from system parachain weight files
- [ ] when merged, open `polkadot-fellow/runtimes` PR

## References

Fixes #1132
Closes #1132
Old polkadot repo [PR](paritytech/polkadot#7546)

---------

Co-authored-by: command-bot <>
bkchr pushed a commit that referenced this issue Apr 10, 2024
@oxarbitrage @bkchr
calculate fee using full message payload (#1132)
6b3b651
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@KiChjang @bkontur