Some runtime configuration don't whitelist AllPalletsWithSystem::whitelisted_storage_keys().

[gui1117]

Usually in the runtime configuration, in the dispatch_benchmark implementation, we whitelist the keys as defined by pallets with this call:

	let whitelist: Vec<TrackedStorageKey> = AllPalletsWithSystem::whitelisted_storage_keys();

But several runtime in this repo don't add those whitelist. From a quick search they are missing in all those files.

	./cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs
	./cumulus/parachains/runtimes/assets/asset-hub-rococo/src/lib.rs
	./cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/lib.rs
	./cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/lib.rs
	./cumulus/parachains/runtimes/contracts/contracts-rococo/src/lib.rs
	./cumulus/parachains/runtimes/testing/penpal/src/lib.rs
	./cumulus/parachains/runtimes/collectives/collectives-westend/src/lib.rs
	./cumulus/parachains/runtimes/coretime/coretime-rococo/src/lib.rs
	./cumulus/parachains/runtimes/coretime/coretime-westend/src/lib.rs
	./cumulus/parachains/runtimes/people/people-rococo/src/lib.rs
	./cumulus/parachains/runtimes/people/people-westend/src/lib.rs
	./cumulus/polkadot-omni-node/lib/src/fake_runtime_api/utils.rs
	./substrate/frame/benchmarking/src/utils.rs

Instead they whitelist some specific hash like:

let whitelist: Vec<TrackedStorageKey> = vec![
	// Block Number
	hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef702a5c1b19ab7a04f536c519aca4983ac").to_vec().into(),
	// Total Issuance
	hex_literal::hex!("c2261276cc9d1f8598ea4b6a74b15c2f57c875e4cff74148e4628f264b974c80").to_vec().into(),
	// Execution Phase
	hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef7ff553b5a9862a516939d82b3d3d8661a").to_vec().into(),
	// Event Count
	hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef70a98fdbe9ce6c55837576c60c7af3850").to_vec().into(),
	// System Events
	hex_literal::hex!("26aa394eea5630e07c48ae0c9558cef780d41e5e16056765bc8461851072c9d7").to_vec().into(),
];

It would be better to add the whitelisted keys from pallet and remove the duplicate whitelist.

[bkchr]

If we are doing this wrong, I can assume that downstream is doing the same thing.

Would be nice if we could force people to use the AllPalletsWithSystem::whitelisted_storage_keys.

[gui1117]

I agree, I think there is ways to factorize most of the code, so user wouldn't need to think about it.

And also remove the duplicate rename (stuff like use .. as OffenceBanchmark;) that are in both list_benchmark and dispatch_benchmark.

[qiweiii]

I added #7024 to replace those with AllPalletsWithSystem::whitelisted_storage_keys.

But not sure how to

force people to use the AllPalletsWithSystem::whitelisted_storage_keys

I can try to do that in another PR if have some pointers

[gui1117]

But not sure how to

force people to use the AllPalletsWithSystem::whitelisted_storage_keys

I can try to do that in another PR if have some pointers

Personally I don't like:

• We rename some path and some types, 2 times: in benchmark_metadata and in dispatch_benchmark: those lines usually appear 2 times:

    		use pallet_xcm::benchmarking::Pallet as PalletXcmExtrinsicsBenchmark;
    		use pallet_xcm_bridge_hub_router::benchmarking::Pallet as XcmBridgeHubRouterBench;
  
    		type XcmBalances = pallet_xcm_benchmarks::fungible::Pallet::<Runtime>;
    		type XcmGeneric = pallet_xcm_benchmarks::generic::Pallet::<Runtime>;

  Ideally it should be defined in one time, nearby the define_benchmarks macro call.

• the benchmark_metadata could be potentially a single function call.

    		let mut list = Vec::<BenchmarkList>::new();
    		list_benchmarks!(list, extra);
  
    		let storage_info = AllPalletsWithSystem::storage_info();
    		(list, storage_info)

• dispatch_benchmark could also be a single function call.

    		let whitelist: Vec<TrackedStorageKey> = AllPalletsWithSystem::whitelisted_storage_keys();
    		let mut batches = Vec::<BenchmarkBatch>::new();
    		let params = (&config, &whitelist);
    		add_benchmarks!(params, batches);
    		Ok(batches)

But we should keep it flexible if people have their own implementation, keep the possibility to add whitelisted call.

All in all I don't have a clear idea on the exact best API, but it feels not optimal. The best API could be one function call with easy optional argument for majority of cases and allow flexibility for some users with special cases.

[ggwpez]

the benchmark_metadata could be potentially a single function call.

This uses macro callbacks to register all benchmarks with the list_benchmarks!, same below in the add_benchmarks!. The define_benchmarks! exports the tokens that they use.
So not sure if it can be a single call.

But it is not optimal for sure 🫠