feat(mainnet): include xcm benchmarks by al3mart · Pull Request #479 · r0gue-io/pop-node

al3mart · 2025-02-24T16:06:04Z

Needed for: #478

Adds benchmarks related to XCM in mainnet runtime.

pallet_xcm extrinsics benchmark
pallet_xcm_benchmarks::generic benchmark
pallet_xcm_benchmarks::fungible benchmark
Configure Weigher using WeightInfoBounds
Remove default weights from pallet_xcm and use benchmarks results.

Note that the benchmarks have been run locally, and they need to be re-run in the collator machines. A script will be provided in #478 such that we can do that easily.

codecov-commenter · 2025-02-24T17:48:39Z

Codecov Report

❌ Patch coverage is 5.66845% with 882 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.43%. Comparing base (fd64662) to head (f458b53).

Files with missing lines	Patch %	Lines
...t/src/weights/xcm/pallet_xcm_benchmarks_generic.rs	0.00%	265 Missing ⚠️
runtime/mainnet/src/weights/xcm/mod.rs	0.00%	221 Missing ⚠️
runtime/mainnet/src/weights/pallet_xcm.rs	0.00%	191 Missing ⚠️
runtime/mainnet/src/benchmarks.rs	0.00%	108 Missing ⚠️
.../src/weights/xcm/pallet_xcm_benchmarks_fungible.rs	0.00%	96 Missing ⚠️
runtime/mainnet/src/apis.rs	0.00%	1 Missing ⚠️

@@                           Coverage Diff                           @@
##           al3mart/feat-mainnet-all-benchmarks     #479      +/-   ##
=======================================================================
- Coverage                                68.43%   65.43%   -3.00%     
=======================================================================
  Files                                      113      120       +7     
  Lines                                    21344    22424    +1080     
  Branches                                 21344    22424    +1080     
=======================================================================
+ Hits                                     14606    14673      +67     
- Misses                                    6472     7485    +1013     
  Partials                                   266      266

Files with missing lines	Coverage Δ
runtime/mainnet/src/config/xcm.rs	`100.00% <100.00%> (ø)`
runtime/mainnet/src/lib.rs	`66.23% <ø> (ø)`
runtime/mainnet/src/apis.rs	`0.00% <0.00%> (ø)`
.../src/weights/xcm/pallet_xcm_benchmarks_fungible.rs	`0.00% <0.00%> (ø)`
runtime/mainnet/src/benchmarks.rs	`0.00% <0.00%> (ø)`
runtime/mainnet/src/weights/pallet_xcm.rs	`0.00% <0.00%> (ø)`
runtime/mainnet/src/weights/xcm/mod.rs	`0.00% <0.00%> (ø)`
...t/src/weights/xcm/pallet_xcm_benchmarks_generic.rs	`0.00% <0.00%> (ø)`

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

al3mart · 2025-02-25T08:01:31Z

+					// Magic number 2 has to do with the fact that we could have up to 2 times
+					// MaxAssetsIntoHolding in the worst-case scenario.


Reason for this is:

// worst-case, holding.len becomes 2 * holding_limit.
// this guarantees that if holding.len() == holding_limit and you have more than
// holding_limit items (which has a best case outcome of holding.len() == holding_limit),
// then the operation is guaranteed to succeed.

source

Also, even though I've found this comment: paritytech/cumulus#2102 (comment)
The reason why this only is applied for NonFungible is not that apparent to me. It seems that it would apply to both cases.

Worth pinging a Cisco or someone about?

Clarified in: 0a50386

al3mart · 2025-02-25T11:00:51Z

+		let _ = <Balances as frame_support::traits::Currency<_>>::make_free_balance_be(
+			&ah_on_pop, balance,
+		);


Funding AH sovereign account on pop because the benchmark tries to withdraw from destination as a way to verify the benchmark.

I believe that makes sense for reserve transfers happening within the reserve itself. Not quite that for our case.

Curious why all the above is necessary for us and not asset hub: https://github.com/polkadot-fellows/runtimes/blob/80ee7dce11ed68a68abf89512bd9886ffcabe192/system-parachains/asset-hubs/asset-hub-polkadot/src/lib.rs#L1155

peterwht

Very clean PR, almost ready to approve.

peterwht · 2025-02-25T19:14:34Z

+		//  Measured:  `0`
+		//  Estimated: `0`
+		// Minimum execution time: 18_446_744_073_709_551_000 picoseconds.
+		Weight::from_parts(18_446_744_073_709_551_000, 0)


For future reviewers: this value is so large because we don't support teleporting.

Thanks!

Indeed, max weight is applied for unsupported actions.

peterwht · 2025-02-25T19:20:48Z

+					// Magic number 2 has to do with the fact that we could have up to 2 times
+					// MaxAssetsIntoHolding in the worst-case scenario.


Worth pinging a Cisco or someone about?

peterwht · 2025-02-25T19:28:41Z

+		//  Measured:  `111`
+		//  Estimated: `1596`
+		// Minimum execution time: 291_401_000 picoseconds.
+		Weight::from_parts(291_401_000, 0)


A little expensive compared to westend: https://github.com/paritytech/polkadot-sdk/blob/3dc3a11cd68762c2e5feb0beba0b61f448c4fc92/polkadot/runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs#L72

Likely just hardware, but double checking regardless.

Should be, I've did these benchmarks in my laptop :)
Recreating weights in our collators will give us a better point to compare.

peterwht · 2025-02-25T19:38:52Z

+
+impl pallet_xcm::benchmarking::Config for Runtime {
+	type DeliveryHelper = (
+		polkadot_runtime_common::xcm_sender::ToParachainDeliveryHelper<


should this also have a ToParentDelivery?

https://github.com/polkadot-fellows/runtimes/blob/main/system-parachains/asset-hubs/asset-hub-polkadot/src/lib.rs#L1128

It will be better with it 👍 375e9d1

peterwht · 2025-02-25T19:42:27Z

+			AssetHubParaId::get(),
+		));
+		// Pop can only reserve transfer DOT.
+		// This test needs to be adapted as the features grow.


could you please add a comment on one of the XCM config unit tests?

Basically that way we have this flow:

we change XCM to support other asset transfers

We now have a failing unit test

We fix unit test and see comment that says: "update set_up_complex_asset_transfer ..."

makes sense, thanks!

The worst_case_holding is also one that must be included in this cycle? Perhaps we should create a doc that we can circle back to? Just an idea not saying it as a requirement

peterwht

Great job. Approving!

peterwht · 2025-02-27T20:07:45Z

do we need cumulus_pallet_weight_reclaim : https://github.com/paritytech/polkadot-sdk/blob/master/cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs#L1486?

Daanvdplas

Nice job!!!

I must say that I didn't expect this complex review and have to finish it tomorrow. Will need some more reference material to give a proper review. Already left some comments you can take a look at.

al3mart · 2025-02-28T08:35:00Z

do we need cumulus_pallet_weight_reclaim : https://github.com/paritytech/polkadot-sdk/blob/master/cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs#L1486?

Once we move from using cumulus_primitives_storage_weight_reclaim::StorageWeightReclaim<Runtime> to using the pallet we need to include it in our benchmarks. The pallet didn't ship benchmarks yet for 2412, so I'd suggest to tackle this in a different PR.

Here the PR where it was introduced.

Have created an issue to track this: #480

Daanvdplas · 2025-02-28T09:58:33Z

+		let _ = <Balances as frame_support::traits::Currency<_>>::make_free_balance_be(
+			&ah_on_pop, balance,
+		);


Curious why all the above is necessary for us and not asset hub: https://github.com/polkadot-fellows/runtimes/blob/80ee7dce11ed68a68abf89512bd9886ffcabe192/system-parachains/asset-hubs/asset-hub-polkadot/src/lib.rs#L1155

Daanvdplas · 2025-02-28T10:26:23Z

Don't forget to rebase before next review :)

fmt

…arks::fungible

…eric::unsubscribe_version

visibility

Daanvdplas

A few last comments

Daanvdplas · 2025-02-28T16:07:35Z

+		ParachainSystem::open_outbound_hrmp_channel_for_benchmarks_or_tests(ParaId::from(
+			AssetHubParaId::get(),


Suggested change

ParachainSystem::open_outbound_hrmp_channel_for_benchmarks_or_tests(ParaId::from(

AssetHubParaId::get(),

ParachainSystem::open_outbound_hrmp_channel_for_benchmarks_or_tests(

AssetHubParaId::get(),

peterwht

Approving again.

The code to generate the benchmarks are solid
The outputted weight files discrepancies make sense to me as well due to hardware, number of steps, and different XCM configs. So, until we run on production collators this will be sufficient.

Daanvdplas

Well done on this PR, very big and complex undertaking and you helped me understand everything very well!

* feat(benchmark): add pallet_xcm extrinsic benchmarks * refactor(benchmkars): amend xcm transfer_assets benchmark fmt * style(benchmarks): pretty comments. * refactor(benchmarks): fund SA of AH on pop in reserve_transfer benchmark * feat(mainnet): add generic & fungible xcm benchmarks * refactor(benchmarks): don't define CheckAccount for pallet_xcm_benchmarks::fungible * refactor(benchmarks): amend benchmarks for pallet_xcm_benchmarks::generic::unsubscribe_version * feat(mainnet): configure xcm Weigher as WeightInfoBounds * docs(xcm): missing doc * refactor(weights): MAX_ASSETS is MaxAssetsIntoHolding * refactor(mainnet): remove default weights from queue pallets * docs(xcm): credit source for WeighAssets impl * docs(xcm): reminders for updating benchmarks * refactor(mainnet): DeliveryHelper considers delivery to parent * docs(benchmarks): clarify xcm NonFungible AllOf weight comments * refactor(api): reorg imports * style(benchmark): refactor import syntax * docs(bencmark): better docs for XcmBalances * refactor(benchmark): define locations via xcm config definitions * test(xcm): ensure xcm encoding size * refactor(benchmark): improve worst_case_response * chore(benchmark): update results for query_response bench * style(benchmarks): remove RelayAsset * docs(benchmarks): explain ExistentialDepositAsset * docs(xcm): note worst_case_holding needs updating * style(benchmark): better naming * chore(benchmarks): update xcm benchmarks visibility * test(xcm): amend router_uses_ump_for_relay_and_xcmp_for_para * test(xcm): amend router_uses_ump_for_relay_and_xcmp_for_para * refactor(benchmarks): unnecessary ParaId::from * refactor(xcm): unnecessary u64 cast * test(xcm): msg_length factor is > 1 in price_for_parent_delivery~

* refactor(benchmakrs): benchmarks mod in its own file (#477) refactor(benchmark): reorg files * feat(benchmarks): add every pallet benchmark to mainnet without XCM * refactor(benchmarks): rename Assets to TrustBackedAssets * fix(motion): map to Weightless Error one origin check failure * fix(monetary): Treausyr BenchmarkHelper to use correct ExistentialDeposit * refactor(mainnet): remove default weights for assets * refactor(mainnet): remove default weights for pallet_session * refactor(mainnet): remove default weights from governance modules * refactor(mainnet): remove default weights from monetary module * refactor(mainnet): remove default weights from proxy * refactor(mainnet): remove default weights from revive * refactor(mainnet): remove default weights from system modules * refactor(mainnet): remove default weights from utiliy * feat(mainnet): introduce benchmarks-ci.sh * refactor(mainnet): remove default weights for pallet_collator_selection * style(collation): more correct comment. * refactor(collation): amend candidates_disabled test * docs(governance): provide docs for CouncilCollective * docs(benchmakrs): allow missing docs * chore(benchmarks): update weights results * refactor(benchmark): rename TrustBackedAssets to Assets * feat(mainnet): include xcm benchmarks (#479) * feat(benchmark): add pallet_xcm extrinsic benchmarks * refactor(benchmkars): amend xcm transfer_assets benchmark fmt * style(benchmarks): pretty comments. * refactor(benchmarks): fund SA of AH on pop in reserve_transfer benchmark * feat(mainnet): add generic & fungible xcm benchmarks * refactor(benchmarks): don't define CheckAccount for pallet_xcm_benchmarks::fungible * refactor(benchmarks): amend benchmarks for pallet_xcm_benchmarks::generic::unsubscribe_version * feat(mainnet): configure xcm Weigher as WeightInfoBounds * docs(xcm): missing doc * refactor(weights): MAX_ASSETS is MaxAssetsIntoHolding * refactor(mainnet): remove default weights from queue pallets * docs(xcm): credit source for WeighAssets impl * docs(xcm): reminders for updating benchmarks * refactor(mainnet): DeliveryHelper considers delivery to parent * docs(benchmarks): clarify xcm NonFungible AllOf weight comments * refactor(api): reorg imports * style(benchmark): refactor import syntax * docs(bencmark): better docs for XcmBalances * refactor(benchmark): define locations via xcm config definitions * test(xcm): ensure xcm encoding size * refactor(benchmark): improve worst_case_response * chore(benchmark): update results for query_response bench * style(benchmarks): remove RelayAsset * docs(benchmarks): explain ExistentialDepositAsset * docs(xcm): note worst_case_holding needs updating * style(benchmark): better naming * chore(benchmarks): update xcm benchmarks visibility * test(xcm): amend router_uses_ump_for_relay_and_xcmp_for_para * test(xcm): amend router_uses_ump_for_relay_and_xcmp_for_para * refactor(benchmarks): unnecessary ParaId::from * refactor(xcm): unnecessary u64 cast * test(xcm): msg_length factor is > 1 in price_for_parent_delivery~ * refactor(motion): improve pallet_motion benchmarking * test(governance): amend simple_majority_is_never_origin * refactor(weights): move weights into pop_runtime_common * syle: fmt * chore(benchmark): improve benchmarking script * fix(ci): add and properly propagate pop-runtime-common try-runtime feat

al3mart changed the title ~~feat(benchmarks): xcm benchmarks~~ feat(benchmarks): xcm benchmarks for mainnet runtime Feb 24, 2025

al3mart mentioned this pull request Feb 24, 2025

feat(mainnet): include all benchmarks #478

Merged

4 tasks

al3mart changed the title ~~feat(benchmarks): xcm benchmarks for mainnet runtime~~ feat(mainnet): include xcm benchmarks Feb 24, 2025

al3mart force-pushed the al3mart/feat-mainnet-all-benchmarks branch from ec2ffde to 1c65204 Compare February 24, 2025 23:01

al3mart force-pushed the al3mart/feat-mainnet-xcm-benchmarks branch from a9e6d11 to 9b39ef8 Compare February 24, 2025 23:27

al3mart marked this pull request as ready for review February 25, 2025 00:52

al3mart requested a review from a team February 25, 2025 00:53

al3mart commented Feb 25, 2025

View reviewed changes

Comment thread runtime/mainnet/src/weights/xcm/mod.rs

al3mart added the ready-for-final-review label Feb 25, 2025

al3mart commented Feb 25, 2025

View reviewed changes

peterwht requested changes Feb 25, 2025

View reviewed changes

al3mart requested a review from peterwht February 26, 2025 14:21

peterwht approved these changes Feb 27, 2025

View reviewed changes

Daanvdplas self-requested a review February 27, 2025 21:58

Daanvdplas reviewed Feb 27, 2025

View reviewed changes

Daanvdplas reviewed Feb 28, 2025

View reviewed changes

al3mart added 9 commits February 28, 2025 16:23

feat(benchmark): add pallet_xcm extrinsic benchmarks

7cbcf93

refactor(benchmkars): amend xcm transfer_assets benchmark

d01b5fd

fmt

style(benchmarks): pretty comments.

bdbcf25

refactor(benchmarks): fund SA of AH on pop in reserve_transfer benchmark

8cc0ab1

feat(mainnet): add generic & fungible xcm benchmarks

2b0ec99

refactor(benchmarks): don't define CheckAccount for pallet_xcm_benchm…

8974a5a

…arks::fungible

refactor(benchmarks): amend benchmarks for pallet_xcm_benchmarks::gen…

e92d2a0

…eric::unsubscribe_version

feat(mainnet): configure xcm Weigher as WeightInfoBounds

08ac3ee

docs(xcm): missing doc

09b6582

al3mart added 12 commits February 28, 2025 16:25

docs(benchmarks): clarify xcm NonFungible AllOf weight comments

18a2cee

refactor(api): reorg imports

4b04f4a

style(benchmark): refactor import syntax

081ca54

docs(bencmark): better docs for XcmBalances

31f9e63

refactor(benchmark): define locations via xcm config definitions

1f3c852

test(xcm): ensure xcm encoding size

8906204

refactor(benchmark): improve worst_case_response

4863071

chore(benchmark): update results for query_response bench

7269599

style(benchmarks): remove RelayAsset

0b83a86

docs(benchmarks): explain ExistentialDepositAsset

8640b6b

docs(xcm): note worst_case_holding needs updating

66e3802

style(benchmark): better naming

ac7e849

al3mart force-pushed the al3mart/feat-mainnet-xcm-benchmarks branch from 7a74982 to 95d1187 Compare February 28, 2025 15:40

chore(benchmarks): update xcm benchmarks

f6f8af0

visibility

al3mart force-pushed the al3mart/feat-mainnet-xcm-benchmarks branch from 95d1187 to f6f8af0 Compare February 28, 2025 15:41

al3mart requested review from Daanvdplas and peterwht February 28, 2025 15:44

Daanvdplas reviewed Feb 28, 2025

View reviewed changes

peterwht approved these changes Feb 28, 2025

View reviewed changes

al3mart added 5 commits March 1, 2025 00:36

test(xcm): amend router_uses_ump_for_relay_and_xcmp_for_para

9755095

test(xcm): amend router_uses_ump_for_relay_and_xcmp_for_para

b4824d2

refactor(benchmarks): unnecessary ParaId::from

7cb4c2a

refactor(xcm): unnecessary u64 cast

39e0ae5

test(xcm): msg_length factor is > 1 in price_for_parent_delivery~

f458b53

al3mart requested a review from Daanvdplas March 4, 2025 11:16

Daanvdplas approved these changes Mar 4, 2025

View reviewed changes

al3mart merged commit 0c41112 into al3mart/feat-mainnet-all-benchmarks Mar 4, 2025

al3mart deleted the al3mart/feat-mainnet-xcm-benchmarks branch March 4, 2025 16:00

		// Magic number 2 has to do with the fact that we could have up to 2 times
		// MaxAssetsIntoHolding in the worst-case scenario.

		ParachainSystem::open_outbound_hrmp_channel_for_benchmarks_or_tests(ParaId::from(
		AssetHubParaId::get(),

Conversation

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

Uh oh!

Uh oh!

codecov-commenter commented Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterwht left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterwht left a comment

Choose a reason for hiding this comment

Uh oh!

peterwht commented Feb 27, 2025

Uh oh!

Daanvdplas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

al3mart commented Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Daanvdplas commented Feb 28, 2025

Uh oh!

Daanvdplas left a comment

al3mart commented Feb 24, 2025 •

edited

Loading

codecov-commenter commented Feb 24, 2025 •

edited

Loading

al3mart commented Feb 28, 2025 •

edited

Loading