Allow configuration of worst case buy execution weight

[girazoki]

Adds worst_case_buy_execution to the Config trait of pallet-xcm-benchmarks with a default implementation that mimics the code that existed previous to this PR.

Rationale: not allowing to set the WeightLimit and the FeeAsset might mean that we dont benchmark the worst case, as with WeightLimit::Unlimited the Trader does not even execute:

polkadot-sdk/polkadot/xcm/xcm-executor/src/lib.rs

Line 833 in c01dbeb

if let Some(weight) = Option::<Weight>::from(weight_limit) {

The new configurable function allows projects to customize the parameters with which the benchmark is run to make sure they account for the worst-case scenario

This is very likely the case of the assethub system chain, with several traders being analyzed and possibly several reads being made:

polkadot-sdk/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/xcm_config.rs

Line 403 in 38d2fa8

cumulus_primitives_utility::TakeFirstAssetTrader<

[girazoki]

CC I am re-opening this #2962. I think it is still valid as right now the buy_execution instruction is being benchmarked with the (Here) asset, which might not include the worst case.

CC @bkchr sorry for not answering before on the closed PR, I somehow missed it

[girazoki]

I remember back in that PR there were a couple of things that needed to be analyzed:

• do we want a default impl for the new helper?
• what is the worst case for assethub? (in which case, I can add it)

[girazoki]

CC @RomarQ

[acatangiu]

cc @franciscoaguirre @bkontur

[girazoki]

Any opinion on this?

[bkontur]

I would not add here default, we want runtimes to set the worst case:

Suggested change

	/// By default returns ((AssetId(Here.into()), 100_000_000u128), Unlimited)
	fn worst_case_buy_execution() -> Result<(Asset, WeightLimit), BenchmarkError> {
	Ok(((AssetId(Junctions::Here.into()), 100_000_000u128).into(), WeightLimit::Unlimited))
	}
	fn worst_case_buy_execution() -> Result<(Asset, WeightLimit), BenchmarkError>;

Just thinking about fn fee_asset() which is supposed to do the same for PayFees,
maybe we could have something like fn worst_case_for_trader() and use it for PayFees and BuyExecution benchmarks.

[girazoki]

indeed yes

[girazoki]

is there any hint that I can take for the worst case scenario of each runtime? or should I try to guess?

[girazoki]

actually never mind, I can get it through fee_asset

[franciscoaguirre]

Suggested change

	- name: pallet-xcm-benchmarks
	- name: pallet-xcm-benchmarks
	- bump: major

It's major since you're adding a function with no default.

[acatangiu]

well.. shit! how are we gonna backport then?

I guess we'll have to limit "backport" to just 2503 which hasn't been published yet

[acatangiu]

ah, nevermind, that's fine, we don't really need to backport this too far back

[girazoki]

I am actually adding a default value, unless you want me to change it. If I dont add a default value I need to put a worst case scenario in all runtimes, and I dont really know what is the worst case scenario in each of them. I can try to guess, but a few hints would be good

[girazoki]

@franciscoaguirre I actually applied @bkontur suggestion and converted fee_asset into worst_case_trader, which now returns both the asset and the weight limit. For now I left everyting to 'Unlimited' because it was the case before. However I think the buyExecution benchmark should be benchmarked for a given weight value, otherwise it does not do anything:

polkadot-sdk/polkadot/xcm/xcm-executor/src/lib.rs

Line 1358 in ec32daa

let Some(weight) = Option::<Weight>::from(weight_limit) else { return Ok(()) };

If you want as part of this PR I can also add a weight value so that the benchmarking of the buyExecution instruction corresponds to worst case

[franciscoaguirre]

@girazoki I think a Limited weight value makes sense for this PR. Changing fee_asset makes sense to reuse it both in BuyExecution and PayFees.

[girazoki]

@girazoki I think a Limited weight value makes sense for this PR. Changing fee_asset makes sense to reuse it both in BuyExecution and PayFees.

This is done know. Now I think the only thing "left", but maybe that is for a separate PR, is to adjust assethub to its worst case scenario, which I believe is when you try to buy weight with an asset different than the parent.

I have an idea in mind for this, let me know guys if you want to make it as part of this PR.