Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Serialisable genesis config #229

Merged
merged 11 commits into from
Jul 3, 2018
Merged

Serialisable genesis config #229

merged 11 commits into from
Jul 3, 2018

Conversation

arkpar
Copy link
Member

@arkpar arkpar commented Jun 21, 2018

Chain spec now contains bootnodes and genesis sections. Genesis can be either raw storage, or serialised GenesisConfig.

@arkpar arkpar added the A0-please_review Pull request needs code review. label Jun 21, 2018
.unwrap_or_else(|f| (Box::new(move ||
read_storage_json(&f)
.map(|s| { info!("{} storage items read from {}", s.len(), f); s })
.unwrap_or_else(|| panic!("Bad genesis state file: {}", f))
), vec![]));

if matches.is_present("build-genesis") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can it still output the raw genesis? this is crucial.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added this as polkatod build-spec --raw

@gavofyork
Copy link
Member

gavofyork commented Jun 22, 2018

OK - so the idea is that it can be used in two main ways:

  1. You can use whatever the current runtime is (via the confusingly named GenesisConfig) as well as a runtime-dependent JSON Genesis Config file to build a runtime-independent JSON Raw Genesis Map. The Genesis Config file should state what runtime ID/version it is for to avoid later runtimes accidentally being configured with earlier files. The output data file (Raw Genesis Map) actually includes the WASM runtime as well as the initial values it requires to get to validate block 1 and beyond. If the Genesis Config includes bootnode descriptions, then the Raw Genesis Map should also include the same boot nodes (can just be an extra key "_bootnodes").

  2. You can take the Raw Genesis Map (as above) and use it to configure and sync to a blockchain. This includes using it as a file passed on the CLI and as a file embedded into the executable and exposed as a predefined network. All non-local network definitions (i.e. everything except dev and local) should be defined in this way (currently poc-2 is defined via GenesisConfig, which is wrong).

A third, less important use case is:

  1. You can use whatever the current runtime is (via the confusingly named GenesisConfig) as well as a JSON Genesis Config file to configure and sync to a blockchain.

@gavofyork
Copy link
Member

Is the above what this PR achieves?

@arkpar
Copy link
Member Author

arkpar commented Jun 24, 2018

@gavofyork Yes, all three scenarios are supported with the following notes:

  1. There is no runtime version information in the runtime-dependent JSON. It will just fail to parse if it does not match the native runtime.

  2. poc-2 config is still defined in the code. Putting it into JSON would require some way to automatically update it with the lates runtime code on the CI. I'll leave it to the next PR.

@gavofyork
Copy link
Member

gavofyork commented Jun 24, 2018

Yeah we should probably just rename it to staging and keep it as-is. When we finalise poc-2 then we can export staging and define poc-2 as the runtime-independent file.

Copy link
Member

@gavofyork gavofyork left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to have local, dev and poc-2 (aka staging) just be JSON files.

info!("Chain specification: {}", chain_spec);

config.chain_name = chain_spec.clone().into();
let spec = load_spec(&matches);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is expensive, isn't it? If it loads a potentially huge raw genesis map or genesis config and builds genesis from it, then it's needless most of the time. Stuff should only be loaded if genesis block is needed to be built as it was before.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before it was loaded unconditionally as well, wasn't? genesis_storage is required in config currently. I'll add an issue to optimise this later, as it requires partial json parsing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no - genesis_storage was a function that only bothered doing the work if the genesis needed to be computed.

Copy link
Member

@gavofyork gavofyork Jun 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would prefer to have the load-on-demand not phased-out in this pr, but if it's really going to add significant complexity, then i guess it could wait for a second pr.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented

@gavofyork gavofyork added A5-grumble and removed A0-please_review Pull request needs code review. labels Jun 28, 2018
@arkpar arkpar added A0-please_review Pull request needs code review. and removed A5-grumble labels Jul 2, 2018
@arkpar arkpar mentioned this pull request Jul 2, 2018
Copy link
Member

@gavofyork gavofyork left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good. Couple of minor nits.

ChainSpec::LocalTestnet => service::ChainSpec::local_testnet_config(),
ChainSpec::PoC2Testnet => service::ChainSpec::poc_2_testnet_config(),
ChainSpec::Custom(f) => {
service::ChainSpec::from_json_file(PathBuf::from(f))?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

superfluous (and incorrectly indented) code block

fn default() -> Configuration {
Configuration {
impl Configuration {
/// Create default condif for given chain spec.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"config"?

@gavofyork gavofyork added A6-mustntgrumble and removed A0-please_review Pull request needs code review. labels Jul 2, 2018
@arkpar arkpar added A0-please_review Pull request needs code review. and removed A6-mustntgrumble labels Jul 2, 2018
@gavofyork gavofyork added A8-looksgood and removed A0-please_review Pull request needs code review. labels Jul 3, 2018
@gavofyork gavofyork merged commit 106c112 into master Jul 3, 2018
@gavofyork gavofyork deleted the gen-file branch July 3, 2018 13:56
JoshOrndorff added a commit to moonbeam-foundation/substrate that referenced this pull request Apr 21, 2021
liuchengxu pushed a commit to chainx-org/substrate that referenced this pull request Aug 23, 2021
* Support Indices

* Support Indices in staking module

* Update substrate

* Format code

* Make 2s gossip

* Merge develop
liuchengxu pushed a commit to chainx-org/substrate that referenced this pull request Aug 23, 2021
* update orml to newest version

* add wasmtime support
liuchengxu pushed a commit to autonomys/substrate that referenced this pull request Jun 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants