Allow configuration of worst case buy execution weight by girazoki · Pull Request #7944 · paritytech/polkadot-sdk

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

* 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 <[email protected]> Co-authored-by: Adrian Catangiu <[email protected]>

@kianenigma

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 <[email protected]> Co-authored-by: Adrian Catangiu <[email protected]>

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 bkontur and franciscoaguirre 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

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

acatangiu mentioned this pull request Jul 17, 2025

Upgrade to Polkadot-SDK stable2506 polkadot-fellows/runtimes#817

Merged

21 tasks

karolk91 mentioned this pull request Jul 30, 2025

Update XCM benchmarks delivery to sibling parachains not parent/relay chain #7321

Merged

bkontur mentioned this pull request Aug 14, 2025

Fresh weights for upcoming runtimes 1.7.0 release polkadot-fellows/runtimes#853

Merged

9 tasks

github-actions bot mentioned this pull request Oct 24, 2025

Update polkadot-sdk from stable2503 to stable2506 moondance-labs/tanssi#1340

Closed

cl0w5 mentioned this pull request Feb 13, 2026

feat: Upgrade to polkadot-stable2506 galacticcouncil/hydration-node#1364

Merged

6 tasks

cl0w5 mentioned this pull request Feb 24, 2026

feat: Upgrade to polkadot-stable2506 & Migrations galacticcouncil/Basilisk-node#686

Merged

6 tasks

pgherveou added this to [preview] release tracker Mar 20, 2026

github-project-automation bot moved this to Todo in [preview] release tracker Mar 20, 2026

github-project-automation bot moved this from Todo to Done in [preview] release tracker Mar 20, 2026

pgherveou removed this from [preview] release tracker Mar 20, 2026

pgherveou added this to [preview] release tracker Mar 23, 2026

github-project-automation bot moved this to Todo in [preview] release tracker Mar 23, 2026

github-project-automation bot moved this from Todo to Done in [preview] release tracker Mar 23, 2026

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

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

girazoki Mar 24, 2025 •

edited

Loading

girazoki commented Mar 24, 2025 •

edited

Loading