frame-omni-bencher fix for --repeat > 1

[bkontur]

Summary

I encountered a problem with the fellows' repository when I tried to regenerate weights for runtime using --steps 50 --repeat 20 and frame-omni-bencher here. I faced a strange issue with several benchmarks, and after some debugging, I discovered that e.g. pallet_xcm::SafeXcmVersion was removed during the second run. To address this, I had to apply a hotfix which sets the version every time, because the SafeXcmVersion from genesis was ignored in subsequent runs, works only for the first run.

Testing

I added several temporary CI pipelines with --repeat=2 setup for frame-omni-bencher.
Locally, see quick-benchmarks-omni-repeat-2-leave-intent and quick-benchmarks-omni-repeat-2-leave-intent how to run and simulate.

From local debugging, I see this strange behavior:

pallet_collator_selection::leave_intent and --steps=2 --repeat=1 works OK:

[2024-07-19T15:48:36Z ERROR bender::leave_intent] eligible_collators: 2
[2024-07-19T15:48:36Z ERROR bender::leave_intent] eligible_collators: 2
[2024-07-19T15:48:36Z ERROR bender::leave_intent] eligible_collators: 2
[2024-07-19T15:48:36Z ERROR bender::leave_intent] eligible_collators: 2
[2024-07-19T15:48:36Z ERROR bender::leave_intent] eligible_collators: 2
[2024-07-19T15:48:36Z ERROR bender::leave_intent] eligible_collators: 2
Pallet: "pallet_collator_selection", Extrinsic: "leave_intent", Lowest values: [], Highest values: [], Steps: 2, Repeat: 1

pallet_collator_selection::leave_intent and --steps=2 --repeat=2 fails:

[2024-07-19T15:45:59Z ERROR bender::leave_intent] eligible_collators: 2
[2024-07-19T15:45:59Z ERROR bender::leave_intent] eligible_collators: 2
[2024-07-19T15:45:59Z ERROR bender::leave_intent] eligible_collators: 0
The following 1 benchmarks failed:
- pallet_collator_selection::leave_intent
Error: Input("1 benchmarks failed")

Solution?

I suspect that genesis state is not re-initialized correctly after for second repeat - see suddenly eligible_collators: 0, but I will continue with debugging.

I think it could be related to the:

let (genesis_storage, genesis_changes) =
			self.genesis_storage::<Hasher, ExtraHostFunctions>(&chain_spec)?;

and

        /// Produce a genesis storage and genesis changes.
	///
	/// It would be easier to only return one type, but there is no easy way to convert them.
	// TODO: Re-write `BenchmarkingState` to not be such a clusterfuck and only accept
	// `OverlayedChanges` instead of a mix between `OverlayedChanges` and `State`. But this can only
	// be done once we deprecated and removed the legacy interface :(
	fn genesis_storage<H: Hash, F: HostFunctions>(

[bkontur]

cc: @ggwpez I will investigate later, but do you have any hints or thoughts?

[ggwpez]

Could be that somewhere a line like this is missing where it should reset the state:

polkadot-sdk/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs

Line 347 in 9543d31

let mut changes = genesis_changes.clone();

One way to check this would be to print the storage root hashes in every benchmark and assert that they are the same.
sp_io::storage::root or something can be used for that.

[ggwpez]

It reproduces locally, i will take a look.

[bkontur]

It reproduces locally, i will take a look.

@ggwpez looks like your fix works, e.g. this passed: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6758635 or https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6758774

[bkontur]

@ggwpez testing with fellows:

frame-omni-bencher from actual master with --repeat 2:

./frame-omni-bencher v1 benchmark pallet --runtime  ./runtimes/target/debug/wbuild/bridge-hub-kusama-runtime/bridge_hub_kusama_runtime.wasm --all --steps 2 --repeat 2
./frame-omni-bencher v1 benchmark pallet --runtime  ./runtimes/target/debug/wbuild/bridge-hub-polkadot-runtime/bridge_hub_polkadot_runtime.wasm --all --steps 2 --repeat 2
./frame-omni-bencher v1 benchmark pallet --runtime  ./runtimes/target/debug/wbuild/coretime-kusama-runtime/coretime_kusama_runtime.wasm --all --steps 2 --repeat 2

...

Failed on the first one:


The following 27 benchmarks failed:
- pallet_collator_selection::leave_intent
- pallet_xcm::force_subscribe_version_notify
- pallet_xcm::force_unsubscribe_version_notify
- pallet_xcm::send
- pallet_xcm::teleport_assets
- pallet_xcm::transfer_assets
- pallet_xcm_benchmarks::fungible::deposit_reserve_asset
- pallet_xcm_benchmarks::fungible::initiate_reserve_withdraw
- pallet_xcm_benchmarks::fungible::initiate_teleport
- pallet_xcm_benchmarks::fungible::transfer_reserve_asset
- pallet_xcm_benchmarks::generic::query_pallet
- pallet_xcm_benchmarks::generic::report_error
- pallet_xcm_benchmarks::generic::report_holding
- pallet_xcm_benchmarks::generic::report_transact_status
- pallet_xcm_benchmarks::generic::subscribe_version
- pallet_xcm_benchmarks::generic::unsubscribe_version
- snowbridge_pallet_inbound_queue::submit
- snowbridge_pallet_system::create_agent
- snowbridge_pallet_system::create_channel
- snowbridge_pallet_system::force_transfer_native_from_agent
- snowbridge_pallet_system::force_update_channel
- snowbridge_pallet_system::set_operating_mode
- snowbridge_pallet_system::set_pricing_parameters
- snowbridge_pallet_system::set_token_transfer_fees
- snowbridge_pallet_system::transfer_native_from_agent
- snowbridge_pallet_system::update_channel
- snowbridge_pallet_system::upgrade
Error: Input("27 benchmarks failed")

frame-omni-bencher from actual bko-frame-omni-bencher-fix with --repeat 2:

./frame-omni-bencher v1 benchmark pallet --runtime  ./runtimes/target/debug/wbuild/bridge-hub-kusama-runtime/bridge_hub_kusama_runtime.wasm --all --steps 2 --repeat 2
./frame-omni-bencher v1 benchmark pallet --runtime  ./runtimes/target/debug/wbuild/bridge-hub-polkadot-runtime/bridge_hub_polkadot_runtime.wasm --all --steps 2 --repeat 2
./frame-omni-bencher v1 benchmark pallet --runtime  ./runtimes/target/debug/wbuild/coretime-kusama-runtime/coretime_kusama_runtime.wasm --all --steps 2 --repeat 2


...

Passed all.

[paritytech-cicd-pr]

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 2/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6967866

[bkontur]

@ggwpez I ran into a different issue and I think everything leads to the benchmarking code path that uses OverlayedChanges with connection to the StateMachine<BenchmarkingState e.g. here.

Originally, I found the issue when doing --repeat 2 (see description), when second iteration had different genesis which was missing data from OverlayedChanges.

But now, I have a different issue, when it looks like the storage is at some point flushed or something is removed.
Let's take it step by step:

1. With this PR#5327 I am refactoring testnet runtimes to support get_preset feature, so basically just moving chain-spec genesis generation to the runtime.
   So the only result in this PR is that, polkadot-parachain generates chain spec with genesis default from runtime file genesis_config_presets.rs, so really no functional or whatever change. And the short-benchmarks with chain-spec works for BridgeHubRococo.
   Everything is ok so far and I have correct get_preset in the BridgeHubRococo runtime.
2. So now I enabled new short-benchmarks CI that uses frame-omni-bencher (requires get_preset) in the PR#4405 based on 5327
3. But the short benchmarks with frame-omni-bencher for BridgeHubRococo fails e.g. here see full log here, error:

   2024-08-13T09:01:50.3787830Z assertion failed: T::is_message_successfully_dispatched(setup.last_nonce())
   

4. So I cherry-picked your fix for --repeat 1 from this PR to the PR#4405 based on 5327
5. And now the short benchmarks with frame-omni-bencher passed for BridgeHubRococo here with full log here.

---

Based on what I tried and see, I assume that there is some "bug" somewhere when using StateMachine with BenchmarkingState and OverlayedChanges (maybe, something at some point clears/flushes the storage or something like that).
So even if we go with your working fix - changes_to_storage, there will be still some bug underhood.

I don't have time to investigate it further now, but when I come back from vacation in two weeks, I will continue.
If we want to do a hotfix till that time, these two commits are the actual hotfix:
eb04569
0628716