try-runtime - specifying runtime & cutting off unnecessary configuration burden

[pmikolajczyk41]

I'm trying to change the way how local runtime is passed. Currently, we must pass path to a chainspec, build genesis storage and retrieve :CODE: from there. Not only it is the second time we build the storage (the first time is in create_runner function in the client code), but we also do it completetly unnecessary.

I would like to propose two things:

1. we add a new shared parameter for all try-runtime subcommands: enum Runtime, which has three variants: Remote | Native | Local(Pathbuf) with natural meaning:
   - Remote: we use Wasm execution with the runtime included in the scraped storage (and we do not have to overwrite anything)
   - Native: we use Native execution with the code from the binary that is actually running (in case of spec-* mismatch we can panic)
   - Local(Pathbuf): we use Wasm execution with the runtime read from local file at Pathbuf (which requires overwriting :CODE: in the scraped storage)

Of course, not every variant makes sense with every subcommand, but having a common way of specifying runtime + variant checks seems to be more friendly and straightforward than implicit, non-consistent approach.

2. we completely get rid of using sc_service::Configuration and sc_cli::SharedParams in try-runtime - if I understand correctly, the only essential information that we use from it are: default_heap_pages, max_runtime_instances and runtime_cache_size. We could 'move' these three arguments to SharedParams and stop using both concepts at all. However, the problem is not gone: TryRuntimeCmd will be still launched using sc::cli::Runner, which requires sc_service::Configuration object. If I'm not mistaken, try-runtime is a command that needs nothing more than an async environment (with some logging configured etc), and the whole overhead with sc_service::Configuration is unneeded and possibly misleading.

Unfortunately, I don't see any other way than providing some additional simplified/generalized methods / structs to the client like:

fn create_simple_runner<T: ...>(&self, command: &T) -> error::Result<SimpleRunner<Self>> {
	let tokio_runtime = build_runtime()?;
	command.generalized_init(&Self::support_url(), &Self::impl_version())?;
	SimpleRunner::new(tokio_runtime)
}

so that commands like TryRuntime can be launched like:

#[cfg(feature = "try-runtime")]
Some(Subcommand::TryRuntime(cmd)) => {
	let runner = cli.create_simple_runner(cmd)?;
	runner.async_run(|| {
		let tokio_handle = ...;
		let task_manager =
			sc_service::TaskManager::new(tokio_handle, no_prometheus_registry)
				.map_err(|e| sc_cli::Error::Service(...))?;
		Ok((cmd.run::<Block, service::ExecutorDispatch>(), task_manager))
	})
}

As long as the former point seems to be quite feasible, I have many doubts with the latter one. In particular, I don't know exactly how much effort it requires.

@kianenigma please take a look

[kianenigma]

I overall love the idea. I must confess that I am not sure if this approach works though, because it does touch a bit of the service code that I am unfamiliar with.

currently, one of the main points of confusion around try-runtime is exactly this: Which runtime is actually being used?

This is specially complicated if you use --execution Native, because in that case if the spec versions don't match we silently fallback to wasm, which could actually a different runtime, or the same one as the wasm one.

I think this is definitely worth prototyping and exploring, and would be a great bonus of we can deliver it before Sub0.

[bkchr]

1. we add a new shared parameter for all try-runtime subcommands: enum Runtime, which has three variants: Remote | Native | Local(Pathbuf) with natural meaning:
   - Remote: we use Wasm execution with the runtime included in the scraped storage (and we do not have to overwrite anything)
   - Native: we use Native execution with the code from the binary that is actually running (in case of spec-* mismatch we can panic)
   - Local(Pathbuf): we use Wasm execution with the runtime read from local file at Pathbuf (which requires overwriting :CODE: in the scraped storage)

I would drop Native. We want to get rid off it any way, so we should not start adding support here. For overriding the wasm files, you can have a look at:

substrate/client/cli/src/params/import_params.rs

Line 91 in ed12e60

pub wasm_runtime_overrides: Option<PathBuf>,

2. we completely get rid of using sc_service::Configuration and sc_cli::SharedParams in try-runtime - if I understand correctly, the only essential information that we use from it are: default_heap_pages, max_runtime_instances and runtime_cache_size. We could 'move' these three arguments to SharedParams and stop using both concepts at all. However, the problem is not gone: TryRuntimeCmd will be still launched using sc::cli::Runner, which requires sc_service::Configuration object. If I'm not mistaken, try-runtime is a command that needs nothing more than an async environment (with some logging configured etc), and the whole overhead with sc_service::Configuration is unneeded and possibly misleading.

Currently that is not really possible. You at least need SharedParams. That is a little bit shitty, but would probably be some big refactor to make it better.

[pmikolajczyk41]

@kianenigma @bkchr

please check out my PoC: #12193