Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

[xcm ] Compatibility fix for xcm benchmarks #2934

Merged

Conversation

bkontur
Copy link
Contributor

@bkontur bkontur commented Jul 25, 2023

There was a change on polkadot repo pallet_xcm benchmarks: https://github.com/paritytech/polkadot/pull/7077/files#diff-a4111c7c81a2c886c9cb40df80da8016448148fc24ba109150da006ebad5fcc6R134-R165
which moved initiate_reserve_withdraw/reserve_asset_deposited from pallet_xcm_benchmarks_generic.rs to file pallet_xcm_benchmarks_fungible.rs

and because CI for companions does not regenerate weights, there was no companion for that, so this PR fixes that

now after weights regeneration there is a compilation error: https://gitlab.parity.io/parity/mirrors/cumulus/-/jobs/3250422

Partially also fixes: paritytech/polkadot-sdk#1132

@bkontur bkontur added B0-silent Changes should not be mentioned in any release notes A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. labels Jul 25, 2023
@bkontur
Copy link
Contributor Author

bkontur commented Jul 25, 2023

bot help

@bkontur bkontur changed the title Compatibility fix for xcm benchmarks (cause of missing companion) [xcm ]Compatibility fix for xcm benchmarks Jul 25, 2023
@bkontur bkontur changed the title [xcm ]Compatibility fix for xcm benchmarks [xcm ] Compatibility fix for xcm benchmarks Jul 25, 2023
@bkontur
Copy link
Contributor Author

bkontur commented Jul 25, 2023

bot bench cumulus-assets --xcm=pallet_xcm_benchmarks::fungible
bot bench cumulus-assets --xcm=pallet_xcm_benchmarks::generic

@bkontur
Copy link
Contributor Author

bkontur commented Jul 25, 2023

bot bench $ xcm cumulus-assets --pallet=pallet_xcm_benchmarks::fungible

@bkontur
Copy link
Contributor Author

bkontur commented Jul 25, 2023

bot bench cumulus-assets --subcommand=xcm --pallet=pallet_xcm_benchmarks::fungible
bot bench cumulus-assets --subcommand=xcm --pallet=pallet_xcm_benchmarks::generic

@bkontur
Copy link
Contributor Author

bkontur commented Jul 25, 2023

bot bench cumulus-bridge-hubs --subcommand=xcm --pallet=pallet_xcm_benchmarks::fungible
bot bench cumulus-bridge-hubs --subcommand=xcm --pallet=pallet_xcm_benchmarks::generic

@bkontur
Copy link
Contributor Author

bkontur commented Jul 25, 2023

bot bench cumulus-assets --subcommand=xcm --pallet=pallet_xcm_benchmarks::fungible --runtime=asset-hub-kusama
bot bench cumulus-assets --subcommand=xcm --pallet=pallet_xcm_benchmarks::generic --runtime=asset-hub-kusama
bot bench cumulus-assets --subcommand=xcm --pallet=pallet_xcm_benchmarks::fungible --runtime=asset-hub-westend
bot bench cumulus-assets --subcommand=xcm --pallet=pallet_xcm_benchmarks::generic --runtime=asset-hub-westend
bot bench cumulus-bridge-hubs --subcommand=xcm --pallet=pallet_xcm_benchmarks::fungible --runtime=bridge-hub-kusama
bot bench cumulus-bridge-hubs --subcommand=xcm --pallet=pallet_xcm_benchmarks::generic --runtime=bridge-hub-kusama
bot bench cumulus-bridge-hubs --subcommand=xcm --pallet=pallet_xcm_benchmarks::fungible --runtime=bridge-hub-rococo
bot bench cumulus-bridge-hubs --subcommand=xcm --pallet=pallet_xcm_benchmarks::generic --runtime=bridge-hub-rococo

// Measured: `0`
// Estimated: `0`
// Minimum execution time: 500_000_000_000 picoseconds.
Weight::from_parts(500_000_000_000, 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not going to fit in a block at that size. If we want to prohibit it then we should be using max weight.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably thats the point, not to allow process reserve_asset_deposited by default,
this number was generated by:

let (trusted_reserve, transferable_reserve_asset) = T::TrustedReserve::get()
			.ok_or(BenchmarkError::Override(
				BenchmarkResult::from_weight(T::BlockWeights::get().max_block)
			))?;

and for AssetHubs where we allow IsReserve for transfer over bridge, this will have correct value in this branch: #2762

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@franciscoaguirre wdyt? you added reserve_asset_deposited benchmark :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

before all this,
we had there hard-coded:

// TODO: hardcoded - fix https://github.com/paritytech/cumulus/issues/1974
		Weight::from_parts(1_000_000_000_u64, 0)

which is also not good

Copy link
Contributor Author

@bkontur bkontur Jul 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gilescope @franciscoaguirre
so the question is,
if it is better to change:

BenchmarkResult::from_weight(T::BlockWeights::get().max_block)

to:

BenchmarkResult::from_weight(Weight::MAX)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather than T::BlockWeights::get().max_block it's traditional to use Weight::MAX as that's the value we use everywhere else to mean not allowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I will fix that for pallet_xcm with Weight::MAX and use this PR as Companion

Copy link
Contributor Author

@bkontur bkontur Jul 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, we cannot go with Weight:MAX now because: paritytech/polkadot#7546 (comment) because it breaks pallet_xcm::reserve_transfer_assets,
so I reverted back that hard-coded 1_000_000_000_u64,
anyway the result of this PR is that regeneration of weights will work in master again

but as a follow-up we need to fix and finish remote weight estimation (maybe with "standard xcm weights") plus:
paritytech/polkadot#7424
paritytech/polkadot#7546

@bkontur
Copy link
Contributor Author

bkontur commented Jul 25, 2023

bot clean

@bkontur
Copy link
Contributor Author

bkontur commented Jul 26, 2023

bot bench cumulus-assets --subcommand=xcm --pallet=pallet_xcm_benchmarks::fungible --runtime=asset-hub-kusama

@command-bot
Copy link

command-bot bot commented Jul 26, 2023

@bkontur https://gitlab.parity.io/parity/mirrors/cumulus/-/jobs/3264556 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=xcm --runtime=asset-hub-kusama --runtime_dir=assets --target_dir=cumulus --pallet=pallet_xcm_benchmarks::fungible. Check out https://gitlab.parity.io/parity/mirrors/cumulus/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 21-bfe53269-df56-4b37-82e7-5191d76ee860 to cancel this command or bot cancel to cancel all commands in this pull request.

…set-hub-kusama --runtime_dir=assets --target_dir=cumulus --pallet=pallet_xcm_benchmarks::fungible
@command-bot
Copy link

command-bot bot commented Jul 26, 2023

@bkontur Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=xcm --runtime=asset-hub-kusama --runtime_dir=assets --target_dir=cumulus --pallet=pallet_xcm_benchmarks::fungible has finished. Result: https://gitlab.parity.io/parity/mirrors/cumulus/-/jobs/3264556 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/cumulus/-/jobs/3264556/artifacts/download.

@bkontur
Copy link
Contributor Author

bkontur commented Jul 26, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 7d51356 into master Jul 26, 2023
3 checks passed
@paritytech-processbot paritytech-processbot bot deleted the bko-compatibility-fix-for-xcm-benchmarks branch July 26, 2023 12:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

XCM Weights/Benchmarks TODOs
3 participants