Investigate too big change for benchmark teleport_assets / reserve_transfer_assets for 1.2.0 release

[bkontur]

Regenerated weights here: #223.
Runtimes diff here: release-v1.1.2...bkontur:runtimes:bko-weights
AssetHubKusama pallet_xcm.rs weights diff: release-v1.1.2...bkontur:runtimes:bko-weights#diff-d6dc494f1c5058902c12b186a1ab36c07ce90fa3bc8e821b782eb6e106b0e6beR71-R124

subweight compare commits          --path-pattern "./**/weights/**/pallet_xcm.rs"          --format markdown --no-color           --change added changed removed          --method asymptotic --ignore-errors          remotes/polkadot-fellows/release-v1.1.2          origin/bko-weights 

File	Extrinsic	Old	New	Change [%]
relay/kusama/src/weights/pallet_xcm.rs	reserve_transfer_assets	17.28us	532.85us	+2982.93
relay/polkadot/src/weights/pallet_xcm.rs	reserve_transfer_assets	17.26us	530.08us	+2971.12
relay/kusama/src/weights/pallet_xcm.rs	teleport_assets	17.68us	534.45us	+2923.79
relay/polkadot/src/weights/pallet_xcm.rs	teleport_assets	17.82us	534.41us	+2899.79
system-parachains/asset-hubs/asset-hub-polkadot/src/weights/pallet_xcm.rs	reserve_transfer_assets	39.41us	844.56us	+2043.01
system-parachains/asset-hubs/asset-hub-kusama/src/weights/pallet_xcm.rs	reserve_transfer_assets	40.05us	846.49us	+2013.84
system-parachains/asset-hubs/asset-hub-polkadot/src/weights/pallet_xcm.rs	teleport_assets	44.33us	593.01us	+1237.68
system-parachains/asset-hubs/asset-hub-kusama/src/weights/pallet_xcm.rs	teleport_assets	45.36us	596.28us	+1214.63
system-parachains/bridge-hubs/bridge-hub-kusama/src/weights/pallet_xcm.rs	teleport_assets	46.40us	601.88us	+1197.07
system-parachains/bridge-hubs/bridge-hub-polkadot/src/weights/pallet_xcm.rs	teleport_assets	46.51us	479.54us	+931.13
system-parachains/collectives/collectives-polkadot/src/weights/pallet_xcm.rs	teleport_assets	47.26us	480.34us	+916.32

TODO

• Close this issue either with a fix or by approving that it is okay.

[bkontur]

My guess is that it could be kind of ok, if we look closer to the generated weight diff.
Before, there was included only ParachainInfo::ParachainId but after there is much more of it.

[acatangiu]

TODO: check if whitelisted_caller() is correctly used in benchmarks, all those extra DB reads and writes look suspicious to me..

[bkontur]

TODO: check/set force_xcm_version as a part of DeliveryHelper stuff to avoid unnecessary VersionDiscoveryQueue / SaveXcmVersion and other read/writes

[acatangiu]

Yes, actually those read/writes are valid, touching:

• System::Account
• Dmp::DeliveryFeeFactor
• XcmPallet::SupportedVersion
• Dmp::DownwardMessageQueues
• Dmp::DownwardMessageQueueHeads

(for relays - and even more for parachains)

[acatangiu]

Old benchmarks code was wrong - it did not actually "deliver" (forward) any XCM, but failure was hidden because error was not propagated.
This runtime update got a number of fixes, including proper benchmarks that run the extrinsic to completion. New weights look correct, old ones were way off (under-estimated).

[bkontur]

yes, agree,
and also I have a simple solution how to reduce 2reads+1write:

        /// Storage: `PolkadotXcm::VersionDiscoveryQueue` (r:1 w:1)
	/// Proof: `PolkadotXcm::VersionDiscoveryQueue` (`max_values`: Some(1), `max_size`: None, mode: `Measured`)
	/// Storage: `PolkadotXcm::SafeXcmVersion` (r:1 w:0)
	/// Proof: `PolkadotXcm::SafeXcmVersion` (`max_values`: Some(1), `max_size`: None, mode: `Measured`)

from every instruction/extrinsic that uses SendXcm.

I will prepare partial fix here and follow-up issue for polkadot-sdk tomorrow.

This is first comparision:

Before: #223 (WITHOUT_POC_FIX)

File	Extrinsic	Old	New	Change [%]
system-parachains/asset-hubs/asset-hub-kusama/src/weights/pallet_xcm.rs	reserve_transfer_assets	40.05us	846.49us	+2013.84
system-parachains/asset-hubs/asset-hub-kusama/src/weights/pallet_xcm.rs	teleport_assets	45.36us	596.28us	+1214.63

After: #223 (WITH_POC_FIX)

File	Extrinsic	Old	New	Change [%]
system-parachains/asset-hubs/asset-hub-kusama/src/weights/pallet_xcm.rs	reserve_transfer_assets	40.05us	704.41us	+1659.04
system-parachains/asset-hubs/asset-hub-kusama/src/weights/pallet_xcm.rs	teleport_assets	45.36us	447.46us	+886.53

Comparision between WITHOUT_POC_FIX and WITH_POC_FIX

File	Extrinsic	Old	New	Change [%]
system-parachains/asset-hubs/asset-hub-kusama/src/weights/xcm/pallet_xcm_benchmarks_generic.rs	expect_error	1.87us	1.77us	-5.25
system-parachains/asset-hubs/asset-hub-kusama/src/weights/xcm/pallet_xcm_benchmarks_generic.rs	set_topic	1.91us	1.79us	-6.13
system-parachains/asset-hubs/asset-hub-kusama/src/weights/xcm/pallet_xcm_benchmarks_generic.rs	clear_origin	1.94us	1.81us	-6.95
system-parachains/asset-hubs/asset-hub-kusama/src/weights/xcm/pallet_xcm_benchmarks_generic.rs	clear_error	1.96us	1.81us	-7.57
system-parachains/asset-hubs/asset-hub-kusama/src/weights/pallet_xcm.rs	reserve_transfer_assets	846.49us	704.41us	-16.78
system-parachains/asset-hubs/asset-hub-kusama/src/weights/xcm/pallet_xcm_benchmarks_fungible.rs	transfer_reserve_asset	845.89us	700.32us	-17.21
system-parachains/asset-hubs/asset-hub-kusama/src/weights/xcm/pallet_xcm_benchmarks_generic.rs	report_holding	762.42us	614.37us	-19.42
system-parachains/asset-hubs/asset-hub-kusama/src/weights/xcm/pallet_xcm_benchmarks_fungible.rs	initiate_reserve_withdraw	719.77us	570.96us	-20.68
system-parachains/asset-hubs/asset-hub-kusama/src/weights/xcm/pallet_xcm_benchmarks_generic.rs	query_pallet	689.40us	541.07us	-21.52
system-parachains/asset-hubs/asset-hub-kusama/src/weights/xcm/pallet_xcm_benchmarks_generic.rs	report_transact_status	683.94us	536.32us	-21.58
system-parachains/asset-hubs/asset-hub-kusama/src/weights/xcm/pallet_xcm_benchmarks_generic.rs	report_error	684.31us	536.29us	-21.63
system-parachains/asset-hubs/asset-hub-kusama/src/weights/pallet_xcm.rs	teleport_assets	596.28us	447.46us	-24.96

[bkontur]

closing and moving further investigation and optimizations to the issue paritytech/polkadot-sdk#3692

[bkontur]

TLDR: So the result looks like, comparing to the latest release 1.1.2, that we lowered user fee for pallet_xcm::reserve_transfer_assets from 410615635 to 273882302 (-33%).

Note: Hope, I did not make any mistake with calculations on the way :)

The significant increase of reserve_transfer_assets from 40.05us to 846.49us is ok, as @acatangiu mentioned above. When compared with the weight for the XCM instruction in pallet_xcm_benchmarks_fungible.rs, which is 845.89us, it is almost the same.

The consequence for pallet_xcm::reserve_transfer_assets is that both weights, reserve_transfer_assets and transfer_reserve_asset, were added together, and based on that result, we estimated the fee for the extrinsic.

We applied the patch for pallet_xcm benchmarks, and upon randomly checking the fee estimation for pallet_xcm::reserve_transfer_assets for AssetHubKusama, the results are as follows:

// fee before patch:	567182302 
// fee after patch:	273882302
// fee release 1.1.2:	410615635

used test:

#[test]
fn estimate_fee_for_reserve_asset_transfer() {
	use asset_hub_kusama_runtime::TransactionPayment;
	use frame_support::dispatch::GetDispatchInfo;
	use xcm::prelude::{VersionedAssets, VersionedLocation};

	let call = pallet_xcm::Call::<Runtime>::reserve_transfer_assets {
		assets: Box::new(VersionedAssets::V4((Here, 1200).into())),
		dest: Box::new(VersionedLocation::V4(
			AccountId32 { network: None, id: ALICE.clone().into() }.into()
		)),
		fee_asset_item: 0,
		beneficiary: Box::new(VersionedLocation::V4(
			AccountId32 { network: None, id: ALICE.clone().into() }.into()
		)),
	};
	let batch_info = call.get_dispatch_info();

	ExtBuilder::<Runtime>::default()
		.with_collators(vec![AccountId::from(ALICE)])
		.with_session_keys(vec![(
			AccountId::from(ALICE),
			AccountId::from(ALICE),
			SessionKeys { aura: AuraId::from(sp_core::sr25519::Public::from_raw(ALICE)) },
		)])
		.build()
		.execute_with(|| {
			let xt = construct_extrinsic(sp_keyring::AccountKeyring::Alice, RuntimeCall::PolkadotXcm(call));
			let estimated_tx_fee = TransactionPayment::compute_fee(xt.encoded_size() as _, &batch_info, 0);
			println!("{estimated_tx_fee}");
		});
}