-
Notifications
You must be signed in to change notification settings - Fork 379
[xcm ] Compatibility fix for xcm benchmarks #2934
[xcm ] Compatibility fix for xcm benchmarks #2934
Conversation
bot help |
bot bench cumulus-assets --xcm=pallet_xcm_benchmarks::fungible |
bot bench $ xcm cumulus-assets --pallet=pallet_xcm_benchmarks::fungible |
bot bench cumulus-assets --subcommand=xcm --pallet=pallet_xcm_benchmarks::fungible |
bot bench cumulus-bridge-hubs --subcommand=xcm --pallet=pallet_xcm_benchmarks::fungible |
bot bench cumulus-assets --subcommand=xcm --pallet=pallet_xcm_benchmarks::fungible --runtime=asset-hub-kusama |
…set-hub-polkadot --runtime_dir=assets --target_dir=cumulus --pallet=pallet_xcm_benchmarks::fungible
…ix-for-xcm-benchmarks
// Measured: `0` | ||
// Estimated: `0` | ||
// Minimum execution time: 500_000_000_000 picoseconds. | ||
Weight::from_parts(500_000_000_000, 0) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
parachains/runtimes/assets/asset-hub-polkadot/src/weights/xcm/pallet_xcm_benchmarks_fungible.rs
Show resolved
Hide resolved
bot clean |
…ix-for-xcm-benchmarks
bot bench cumulus-assets --subcommand=xcm --pallet=pallet_xcm_benchmarks::fungible --runtime=asset-hub-kusama |
@bkontur https://gitlab.parity.io/parity/mirrors/cumulus/-/jobs/3264556 was started for your command Comment |
…set-hub-kusama --runtime_dir=assets --target_dir=cumulus --pallet=pallet_xcm_benchmarks::fungible
@bkontur Command |
bot merge |
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
frompallet_xcm_benchmarks_generic.rs
to filepallet_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