add dev-omni-node

[kianenigma]

Initially, we tried to introduce this as a part of #5027, but it turned out to be a bit needlessly cumbersome to add the CLI parameterization (see #5027 (review)). For now it is simpler to add a dev-omni-node binary separately. This will allow me to revive #3946 as well.

What I would like to achieve here, but I am having a hard time with, is to allow users to optionally circumvent generating a chain-spec, by providing just a .wasm file.

Initially, I tried this with a new --runtime that is mutually exclusive with --chain. This could work, but the downside is that it is a new flag, and that I couldn't yet make clap understand the relationship between --chain and --runtime, because --chain is flattened.

Preferably, I would like to achieve what the code does here: If --chain is given .wasm file, generate a default chain spec for it. If not, parse it as a normal chain spec. But, somewhere else in the code other than load_spec, we are trying to read the spec, and a .wasm file is not parse-able as JSON. THe error should help one figure out where this is happening:

     Running `target/release/dev-omni-node --chain ./target/release/wbuild/minimal-template-runtime/minimal_template_runtime.wasm`
2024-07-23 13:23:25 Polkadot SDK Dev Omni Node    
2024-07-23 13:23:25 ✌️  version 1.0.0-unknown    
2024-07-23 13:23:25 ❤️  by Parity Technologies <[email protected]>, 2017-2024    
2024-07-23 13:23:25 📋 Chain specification: Development    
2024-07-23 13:23:25 🏷  Node name: likeable-rate-9985    
2024-07-23 13:23:25 👤 Role: FULL    
2024-07-23 13:23:25 💾 Database: RocksDb at /home/kian/.local/share/dev-omni-node/chains/dev/db/full   
Error: Service(Client(Storage("Invalid JSON blob: invalid type: null, expected struct RuntimeGenesisConfig at line 1 column 4 for blob:\nnull")))

It is also unfortunate that --dev cannot really be supported in this case, and --temp is not the default. I should explore options to achieve this.

[bkchr]

What I would like to achieve here, but I am having a hard time with, is to allow users to optionally circumvent generating a chain-spec, by providing just a .wasm file.

Good idea.

Initially, I tried this with a new --runtime that is mutually exclusive with --chain. This could work, but the downside is that it is a new flag, and that I couldn't yet make clap understand the relationship between --chain and --runtime, because --chain is flattened.

We should just change the basic RunCmd parameters to support this. I don't see any reason for not supporting this.

[michalkucharczyk]

THe error should help one figure out where this is happening:

Not sure if this is a question, but this error is probably originated here:

polkadot-sdk/substrate/client/service/src/builder.rs

Lines 151 to 156 in 604f56f

	let genesis_block_builder = GenesisBlockBuilder::new(
	config.chain_spec.as_storage_builder(),
	!config.no_genesis(),
	backend.clone(),
	executor.clone(),
	)?;

polkadot-sdk/substrate/client/chain-spec/src/genesis_block.rs

Line 110 in 604f56f

build_genesis_storage.build_storage().map_err(sp_blockchain::Error::Storage)?;

[michalkucharczyk]

We would need some way to say omni-node how to generate the initial genesis state using wasm blob. For sure we can use presets for this. If we don't want to force user to explicitly provide the preset name, we need to agree on some common preset name which will be used as default (e.g. "default").

Or we can simply agree that RuntimeGenesisConfig::default() shall be runnable (which on the other hand may not be preferable as we cannot put good default values for some pallets, also defaults could sneak into production code which would not be desirable).

[kianenigma]

we need to agree on some common preset name which will be used as default (e.g. "default").

Precisely, and I was talking to this with @ggwpez as well. He has used Development in omni-bencher. I'd we shorten this to dev for simplicity, and similarity to --chain dev.

I also see some issues with using Default. I'd say:

• Default should be empty and correct state, so the bare minimum that is run-able, but not great for testing. This also adheres to Rust's definition of Default.
• dev or whatever we decide should be mock data for testing.

How is a pallet meant to express this though? A pallet can only declare impl Default for GenesisConfig, so it is a bit of an issue that the preset names are never exposed to any pallet. I will see what I can do about this today, but lmk if you already have ideas around this @michalkucharczyk, thanks!

Beyond this, I can work on amending the SharedParams, as @bkchr suggested, to accept either:

• --chain
• --runtime and optionally --genesis-preset

[michalkucharczyk]

How is a pallet meant to express this though? A pallet can only declare impl Default for GenesisConfig, so it is a bit of an issue that the preset names are never exposed to any pallet. I will see what I can do about this today, but lmk if you already have ideas around this @michalkucharczyk, thanks!

My feeling is that initial config (preset) is property of runtime, not pallet. The pallet never knows in which configuration/context it will be embedded into runtime.

Editied - second thought: maybe trait like this would make sense for pallet for the sake of devex improvements? Sth like:

trait PresetProvider { fn preset(name: PresetId) -> json::Value }

or more opinionated

trait DevPresetProvider { fn dev_preset() -> json::Value }

This could be combined into full RuntimeGenesisConfig preset in runtime macros.

[kianenigma]

What you say makes sense. But, at the same time, it kinda sucks as well that when testing pallet-balances, there would be no singular way to define a set of seed accounts for testing. In that sense, it would have been nice for pallet-balances to simply provide this testing data itself..

[michalkucharczyk]

What you say makes sense. But, at the same time, it kinda sucks as well that when testing pallet-balances, there would be no singular way to define a set of seed accounts for testing. In that sense, it would have been nice for pallet-balances to simply provide this testing data itself..

I feel it. (I also re-edited my previous comment).

[kianenigma]

Initially, I tried this with a new --runtime that is mutually exclusive with --chain. This could work, but the downside is that it is a new flag, and that I couldn't yet make clap understand the relationship between --chain and --runtime, because --chain is flattened.

We should just change the basic RunCmd parameters to support this. I don't see any reason for not supporting this.

I've experimented with this in 3e55a98

There's still the same error as above for genesis build code path, but it is solve-able.

[bkchr]

The runtime should expose some sensible genesis when the preset_id == None. We should not start trying to push presets to pallets. That makes no sense. This would also require to have them standardized across all chains etc.

[michalkucharczyk]

The runtime should expose some sensible genesis when the preset_id == None. We should not start trying to push presets to pallets. That makes no sense. This would also require to have them standardized across all chains etc.

Just a kind reminder about the assumptions we made to build presets (discussion about api):

1. None is just RuntimeGenesisConfig::default() which is an empty base for applying patches,
2. preset is a json::patch which shall be applied on the base,

I am just wondering: if None no longer means RuntimeGenesisConfig::default() then is it still good base to apply patches? Probably not - some dev values may sneak into the genesis.

One solution is to get rid of patches, and provide only full genesis configs - but I personally think patches are nice for development (e.g. in zombienet).

My opinion is that we should have standardized "dev" preset.

[kianenigma]

The runtime should expose some sensible genesis when the preset_id == None. We should not start trying to push presets to pallets. That makes no sense. This would also require to have them standardized across all chains etc.

I agree to "We should not start trying to push presets to pallets" part, as most often sensible genesis is dependent on multiple pallets to begin with.

But I am hesitant about "The runtime should expose some sensible genesis when the preset_id == None". What is sensible? it is too vague. What is sensible on the day you want to test is often not sensible for real life.

For example, in testing, you want your balances filled with the 6 dev accounts. In production you never want that. I am worried that if we make the dev accounts be preset_id = None, people will mistakenly call this code path in production, whereas if it is behind a strict dev, it is guarded.

I am still arguing for:

• preset_id = None: Empty and correct genesis, as per Rust's definition of Default.
• preset_id = 'dev': fake data for testing.

[bkchr]

• Empty and correct genesis, as per Rust's definition of Default

For a correct genesis you will need the correct authorities. You will not have the authorities until you start preparing your launch. I see where you are coming from. However, "a correct genesis" is a little bit more complex and not just exists.

I mean we can go for passing "dev" and then live with "None" giving no correct value until the runtime is ready for launch.

[kianenigma]

For a correct genesis you will need the correct authorities. You will not have the authorities until you start preparing your launch. I see where you are coming from. However, "a correct genesis" is a little bit more complex and not just exists.

This notion of authority is also a bit dated, isn't it? It all goes back to launching a local chain with aura/babe/grandpa etc. which this node does not care about at all.

I am not decided on this yet tbh. Let me explore a bit more and come back here with more insights.

[bkchr]

This notion of authority is also a bit dated, isn't it? It all goes back to launching a local chain with aura/babe/grandpa etc. which this node does not care about at all.

Please do not try again to come up with a new node. Not sure why this node doesn't care. I mean I see where you are coming from. But then you will need more code written to build blocks without any kind of consensus logic.

[paritytech-cicd-pr]

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6860899

[kianenigma]

Closing this as it we decided to give it another shot to integrate it properly into a new subcommand of polkadot-parachain.