Re-visit pallet_xcm benchmarks for teleport_assets / reserve_transfer_assets / transfer_assets

[bkontur]

Relates to the: https://github.com/polkadot-fellows/runtimes/issues/231#issue-2178704646

reserve_transfer_assets - https://github.com/paritytech/polkadot-sdk/blob/master/polkadot/xcm/pallet-xcm/src/lib.rs#L997-L1003
teleport_assets - https://github.com/paritytech/polkadot-sdk/blob/master/polkadot/xcm/pallet-xcm/src/lib.rs#L940-L945
transfer_assets - https://github.com/paritytech/polkadot-sdk/blob/master/polkadot/xcm/pallet-xcm/src/lib.rs#L1299-L1308

Is this adding ok? And does not double the actual weights?

T::WeightInfo::reserve_transfer_assets().saturating_add(w)
T::WeightInfo::teleport_assets().saturating_add(w)
T::WeightInfo::transfer_assets().saturating_add(w)

For example for AssetHubRococo for pallet_xcm::reserve_transfer_assets, it adds:

1. benchamark for transfer_reserve_asset which is configured to transfer to Parent, which covers reserve, send+delivery
   https://github.com/paritytech/polkadot-sdk/blob/master/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weights/xcm/pallet_xcm_benchmarks_fungible.rs#L73-L97

2. benchmark for pallet_xcm::reserve_transfer_assets, which is configured for delvering to sibling parachain, which also covers reserve+fee handling, send+delivery
   https://github.com/paritytech/polkadot-sdk/blob/master/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weights/pallet_xcm.rs#L99-L117

Why we adds "reserve+delivery-to-parent" + "reserve+delivery-to-sibling"?

I think we should remove maybe those: .saturating_add(w)

TODO

• backport for 1.7 pallet-xcm: remove extrinsics guessed weight and rely on runtime benchmarks instead polkadot-sdk#3730
• backport for 1.8 - TODO:PR?
• backport for 1.9 - TODO:PR?
• make sure it lends to the master before 1.10 (03.04.2024) (as a part of pallet-xcm: add new extrinsic for asset transfers using explicit XCM transfer types polkadot-sdk#3695 or cherry-picked) merged here: pallet-xcm: fix weights for all XTs and deprecate unlimited weight ones  polkadot-sdk#3927

[bkontur]

@acatangiu @franciscoaguirre guys, I need your eyes and fresh minds :), maybe I am missing something, I don't understand why we need to do those .saturating_add(w)

[acatangiu]

I thought either one weight or the other is used, not adding both (doubling)..

Either way, the second commit in this PR paritytech/polkadot-sdk#3695 should also indirectly fix this, yes?

[bkontur]

I thought either one weight or the other is used, not adding both (doubling)..

Either way, the second commit in this PR paritytech/polkadot-sdk#3695 should also indirectly fix this, yes?

wow, I think this commit exactly fixes this, I would even backport that commit to 1.7 / (1.8?) / 1.9 :)

[franciscoaguirre]

I was confused by this at first, but then I saw it gives you credit to execute the message, which is this part.
I think the idea came from that, you have to pay for the extrinsic but you also pay for the execution so you don't need to do BuyExecution in the message itself.
But I guess that only makes sense if prepare_and_execute is somehow mocked during the extrinsic benchmarks. If it's being accounted for then you're accounting for it twice indeed 😅

[franciscoaguirre]

Just being able to use WeightInfo like in that commit would be great. It probably is what we should do

[bkontur]

Just being able to use WeightInfo like in that commit would be great. It probably is what we should do

yes, exactly, also I just talked to the Adrian and we will patch his commit to the 1.7.0

[bkontur]

@acatangiu that double-weights fix was patched to the 1.7.0 and now it is coming to the master here: paritytech/polkadot-sdk#3927 so 1.10.0 release will be ok, but what about 1.8.0 / 1.9.0? Do we want/need to backport it?

[acatangiu]

I'm not sure if/how that PR fixes double weight or how they happened in the first place, for example benchmarking asset-hub-westend in said PR didn't result in relevant changes to pallet_xcm::weights.

[bkontur]

I'm not sure if/how that PR fixes double weight or how they happened in the first place, for example benchmarking asset-hub-westend in said PR didn't result in relevant changes to pallet_xcm::weights.

this double-weights problem, you won't see it in the weights, but you would see it on the extrinsic fees: https://github.com/paritytech/polkadot-sdk/pull/3927/files#diff-742026ed87c0710683d2ef4263b463a3e6f0acfae3c8fad80f2f3d291088997fL991