search

paritytech / polkadot-sdk

The Parity Polkadot Blockchain SDK
https://polkadot.network/
1.78k stars 643 forks source link

Clean up genesis initialization in benchmarking-cli #5694

Open skunert opened 5 days ago

skunert commented 5 days ago

With the introduction of the frame-omni-bencher binary we have the option to initialize genesis state with multiple methods.

The underlying implementation in benchmarking-cli provides options:

  1. We can provide a runtime wasm blob with --runtime a. If provided without --genesis-builder or with --genesis-builder=runtime, it will use the development preset of the WASM blob. This case breaks when used from polkadot-parachain, because in the CLI there we expect that a chain-spec is provided. b. If provided with --genesis-builder=none, it will initialize with empty genesis c. If provided with --genesis-builder=spec it will crash because no spec will be provided
  2. We can provide a chain-spec with --chain a. If provided without --genesis-builder=runtime, it will use the development preset of the WASM code that is included in the chainspec. This case is currently broken as the state we try to read from is not properly initialized here: https://github.com/paritytech/polkadot-sdk/blob/a34cc8dff09dc3840a304befc2415343244b5fd0/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs#L601-L606 b. If provided with --genesis-builder=none, it will initialize with empty genesis c. If provided with --genesis-builder=spec it will use the storage included in the chain-spec

There is also currently a difference between initialization form chain spec vs runtime preset. For runtime presets, we return OverlayedChanges, for chain-specs, we return Storage here: https://github.com/paritytech/polkadot-sdk/blob/a34cc8dff09dc3840a304befc2415343244b5fd0/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs#L570-L579

The comment there says that this should be refactored to always return OverlayedChanges, but I propose we do the opposite and use Storage always. If we use OverlayedChanges read values will be invisible to the proof recorder, which will change the measured storage proof values. The real-world effect of this should be minimal since the measured values only come into effect when someone provides a custom --default-pov-mode, but I am not sure if anyone does that and what the use-case is. Still always using Storage should be the better solution.

TLDR: 1a and 2a are problematic and should be fixed. For the unification I already have some code prepared.

cc @ggwpez

bkontur commented 5 days ago

Based on my investigations and testing so far here, the OverlayedChanges and BenchmarkingState seem to cause problems. Therefore, I am in favor of not using them. However, there could still be a hidden bug related to OverlayedChanges, BenchmarkingState, or StateMachine.