Allow configuration of worst case buy execution weight #7944

girazoki · 2025-03-17T12:48:39Z

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<

…-buy-execution-weight

girazoki · 2025-03-17T12:49:56Z

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 · 2025-03-17T12:51:33Z

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 · 2025-03-17T12:56:32Z

CC @RomarQ

acatangiu · 2025-03-17T13:57:01Z

cc @franciscoaguirre @bkontur

girazoki · 2025-03-20T10:50:48Z

Any opinion on this?

polkadot/xcm/pallet-xcm-benchmarks/src/generic/mod.rs

franciscoaguirre · 2025-03-20T11:38:07Z

prdoc/pr_7944.prdoc

+      the case where does not even need to go into the Trader. This PR allows
+      developers to set the worst-case scenario as they wish.
+crates:
+  - name: pallet-xcm-benchmarks


Suggested change

- name: pallet-xcm-benchmarks

- name: pallet-xcm-benchmarks

- bump: major

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

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

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

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

Now that you changed fee_asset, there's no default. Everyone needs to change that function so it's a breaking change.
If we don't need to backport then it's fine.

girazoki · 2025-03-24T13:17:38Z

@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 · 2025-03-24T17:16:12Z

@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 · 2025-03-25T13:05:31Z

@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.

girazoki · 2025-03-28T08:53:03Z

@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.

@franciscoaguirre do you have an opinion on this? should I adapt assetshub for its worst case? or should I leave it for another PR? because otherwise I think this PR is "ready"

franciscoaguirre · 2025-03-28T09:10:35Z

@girazoki We can do it in a separate PR

franciscoaguirre · 2025-03-28T09:14:00Z

prdoc/pr_7944.prdoc

+      the case where does not even need to go into the Trader. This PR allows
+      developers to set the worst-case scenario as they wish.
+crates:
+  - name: pallet-xcm-benchmarks


Now that you changed fee_asset, there's no default. Everyone needs to change that function so it's a breaking change.
If we don't need to backport then it's fine.

prdoc/pr_7944.prdoc

girazoki · 2025-04-22T12:17:20Z

@acatangiu I fixed the errors from the merge queue I think, can you put it back?

girazoki · 2025-04-23T09:43:02Z

Alright now apparently returns_status_for_pruned_blocks fails, but I dont think I touched anything related to that :(

prdoc/pr_7944.prdoc

* master: (120 commits) [CI] Improve GH build status checking (#8331) [CI/CD] Use original PR name in prdoc check for the backport PR's to the stable branches (#8329) Add new host APIs set_storage_or_clear and get_storage_or_zero (#7857) push to dockerhub (#8322) Snowbridge - V1 - Adds 2 hop transfer to Rococo (#7956) [AHM] Prepare `election-provider-multi-block` for full lazy data deletion (#8304) Check umbrella version (#8250) [AHM] Fully bound staking async (#8303) migrate parachain-templates tests to `gha` (#8226) staking-async: add missing new_session_genesis (#8310) New NFT traits: granular and abstract interface (#5620) Extract create_pool_with_native_on macro to common crate (#8289) XCMP: use batching when enqueuing inbound messages (#8021) Snowbridge - Tests refactor (#8014) Allow configuration of worst case buy execution weight (#7944) Fix faulty pre-upgrade migration check in pallet-session (#8294) [pallet-revive] add get_storage_var_key for variable-sized keys (#8274) add poke_deposit extrinsic to pallet-recovery (#7882) `txpool`: use tracing for structured logging (#8001) [revive] eth-rpc refactoring (#8148) ...

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: https://github.com/paritytech/polkadot-sdk/blob/c01dbebeaa6394691974de46dd2d41a582f6a4c2/polkadot/xcm/xcm-executor/src/lib.rs#L833 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: https://github.com/paritytech/polkadot-sdk/blob/38d2fa859861005157ccb249dca1378f015e0b06/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/xcm_config.rs#L403 --------- Co-authored-by: Branislav Kontur <bkontur@gmail.com> Co-authored-by: Adrian Catangiu <adrian@parity.io>

girazoki added 6 commits January 17, 2024 12:28

Allow to set a worst case buy execution fee asset and weight

ce2d375

Merge remote-tracking branch 'origin/master' into girazoki-worst-case…

e58b767

…-buy-execution-weight

add prdoc

0b426c7

remove default impl

c982b26

Merge remote-tracking branch 'origin/master' into girazoki-worst-case…

04d2623

…-buy-execution-weight

reopen and merging back

b9b9b11

girazoki requested a review from a team as a code owner March 17, 2025 12:48

adapt pr number

bb54f66

acatangiu requested review from franciscoaguirre and bkontur March 20, 2025 11:05

bkontur reviewed Mar 20, 2025

View reviewed changes

polkadot/xcm/pallet-xcm-benchmarks/src/generic/mod.rs Outdated Show resolved Hide resolved

franciscoaguirre reviewed Mar 20, 2025

View reviewed changes

generate worst case for all cases, and include it as part of fee asset

ddafc1e

franciscoaguirre added the T6-XCM This PR/Issue is related to XCM. label Mar 24, 2025

girazoki added 2 commits March 25, 2025 09:28

add limited weight for benchmarks

aebfc30

remove non-used

b8dc917

girazoki added 2 commits March 26, 2025 12:37

sanitize prdoc

d8492ce

fmt

5e63326

franciscoaguirre approved these changes Mar 28, 2025

View reviewed changes

prdoc/pr_7944.prdoc Show resolved Hide resolved

Merge branch 'master' into girazoki-worst-case-buy-execution-weight

bd0bfef

bkontur added this pull request to the merge queue Apr 22, 2025

AKJUS approved these changes Apr 22, 2025

View reviewed changes

auto-merge was automatically disabled April 22, 2025 09:45
Pull Request is not mergeable

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 22, 2025

acatangiu and others added 2 commits April 22, 2025 14:18

Merge branch 'master' into girazoki-worst-case-buy-execution-weight

dd35f0b

modify worst_case_trader

5d34856

girazoki requested a review from a team as a code owner April 22, 2025 12:16

girazoki added 2 commits April 22, 2025 17:32

fix signature

7bddc0e

fix

0d29ca9

acatangiu enabled auto-merge April 22, 2025 16:17

fix again

0312304

auto-merge was automatically disabled April 23, 2025 07:16
Head branch was pushed to by a user without write access

Merge branch 'master' into girazoki-worst-case-buy-execution-weight

b110b36

bkontur enabled auto-merge April 23, 2025 08:50

bkontur reviewed Apr 23, 2025

View reviewed changes

prdoc/pr_7944.prdoc Show resolved Hide resolved

Update prdoc/pr_7944.prdoc

f4a6a22

bkontur added this pull request to the merge queue Apr 23, 2025

Merged via the queue into paritytech:master with commit c800a0d Apr 23, 2025
236 of 243 checks passed

TarekkMA mentioned this pull request Jun 19, 2025

Remove moonbeam-xcm-benchmarks and only use pallet-xcm-benchmarks moonbeam-foundation/moonbeam#3318

Merged

bkontur moved this to To be released (SDK) in fellowship/runtimes integrations queue Jul 2, 2025

bkontur added this to fellowship/runtimes integrations queue Jul 2, 2025

bkontur assigned franciscoaguirre Jul 2, 2025

bkontur moved this from Todo to Done in @bkontur's board Jul 2, 2025

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

Allow configuration of worst case buy execution weight #7944

Allow configuration of worst case buy execution weight #7944

Uh oh!

Conversation

girazoki commented Mar 17, 2025

Uh oh!

girazoki commented Mar 17, 2025

Uh oh!

girazoki commented Mar 17, 2025

Uh oh!

girazoki commented Mar 17, 2025

Uh oh!

acatangiu commented Mar 17, 2025

Uh oh!

girazoki commented Mar 20, 2025

Uh oh!

Uh oh!

franciscoaguirre Mar 20, 2025

Choose a reason for hiding this comment

Uh oh!

acatangiu Mar 20, 2025

Choose a reason for hiding this comment

Uh oh!

acatangiu Mar 20, 2025

Choose a reason for hiding this comment

Uh oh!

girazoki Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

franciscoaguirre Mar 28, 2025

Choose a reason for hiding this comment

Uh oh!

girazoki commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

franciscoaguirre commented Mar 24, 2025

Uh oh!

girazoki commented Mar 25, 2025

Uh oh!

girazoki commented Mar 28, 2025

Uh oh!

franciscoaguirre commented Mar 28, 2025

Uh oh!

franciscoaguirre Mar 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

girazoki commented Apr 22, 2025

Uh oh!

girazoki commented Apr 23, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

girazoki Mar 24, 2025 •

edited

Loading

girazoki commented Mar 24, 2025 •

edited

Loading