search

paritytech / parity-bridges-common

Collection of Useful Bridge Building Tools 🏗️
GNU General Public License v3.0
270 stars 133 forks source link

Re-visit `pallet_xcm` benchmarks for `teleport_assets` / `reserve_transfer_assets` / `transfer_assets` #2874

Open bkontur opened 6 months ago

bkontur commented 6 months ago

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

bkontur commented 6 months ago

@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 commented 6 months ago

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

Either way, the second commit in this PR https://github.com/paritytech/polkadot-sdk/pull/3695 should also indirectly fix this, yes?

bkontur commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

bkontur commented 6 months ago

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 commented 5 months ago

@acatangiu that double-weights fix was patched to the 1.7.0 and now it is coming to the master here: https://github.com/paritytech/polkadot-sdk/pull/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 commented 5 months ago

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 commented 5 months ago

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