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

chain-spec: support for json config/patch (and GenesisBuilder API) #14562

Open
wants to merge 53 commits into
base: master
Choose a base branch
from

Conversation

michalkucharczyk
Copy link
Contributor

@michalkucharczyk michalkucharczyk commented Jul 12, 2023

This PR:

  • adds support for:
    • JSON based GenesisConfig to ChainSpec allowing interaction with runtime GenesisBuilder API.
    • interacting with arbitrary runtime wasm blob to chain-spec-builder command line util,
    • GenesisBuilder API in kitchensink runtime
  • removes code from system_pallet
  • adds code to the ChainSpec
  • deprecates ChainSpec::from_genesis, but also changes the signature of this method extending it with code argument. ChainSpec::builder() should be used instead.
  • implements GenesisBuilder API for node-template-runtime and kitchensink-runtime.

code moved into ChainSpec

Having the explicit code field in ChainSpec would allow for duplication of the wasm code within the raw version of the ChainSpec (as the raw storage may contain wasm code under genesis::top::raw::0x3a636f6465 key).

To avoid this redundancy/ambiguity following measure was implemented. When re-exporting the raw version of ChainSpec the code field will be converted into genesis::top::raw::0x3a636f6465. It will be removed from the ChainSpec. If the raw ChainSpec already contains genesis::top::raw::0x3a636f6465 entry, it will be overwritten with the value of the code field (if present).

Similarly, when building genesis state (actually calling ChainSpec::assimilate_storage) the existing 0x3a636f6465 will also be overwritten with code field (if present).

Example:

{
  "name": "TestName",
  "id": "test_id",
  "chainType": "Local",
   ...
  "genesis": {
    "raw": {
      "top": {
        "0x3a636f6465": "0x010101"
      },
      "childrenDefault": {}
    }
  },
  "code": "0x060708"
}

is equivalent of:

{
  "name": "TestName",
  "id": "test_id",
  "chainType": "Local",
   ...
  "genesis": {
    "raw": {
      "top": {
        "0x3a636f6465": "0x060708"
      },
      "childrenDefault": {}
    }
  }
}

JSON based GenesisConfig in ChainSpec:

Patch

The ChainSpec can now be built using genesis config JSON patch (which contains some key-value pairs meant to override runtime's genesis config default values). This can be achieved with with_genesis_patch method of the builder:

let chain_spec = ChainSpec::builder()
    .with_code(substrate_test_runtime::wasm_binary_unwrap())
    .with_genesis_config_patch(json!({
        "babe": {
            "epochConfig": {
                "c": [
                    7,
                    10
                ],
                "allowed_slots": "PrimaryAndSecondaryPlainSlots"
            }
        }
    }))
    .build()

Resulting ChainSpec instance can be converted to raw version of chain spec JSON file. This was not changed and can be done with chain_spec.as_json(true) method. Sample output is here. The runtime's GenesisBuilder::build_config API is called during this conversion.

The ChainSpec instance can also be written to chain spec JSON file in human readable form. The resulting chain spec file will contain the genesis config patch (partial genesis config). Sample output is here

Full Config

It is also possible to build ChainSpec using full genesis config JSON (containing all the genesis config key-value pairs). No defaults will be used in this approach. The sample code is as follows:

let chain_spec = ChainSpec::builder()
    .with_code(substrate_test_runtime::wasm_binary_unwrap())
    .with_genesis_config(json!({
        "babe": {
            "authorities": [
                [ "5GrwvaEF5zXb26Fz9rcQpDWS57CtERHpNehXCPcNoHGKutQY", 1 ],
                ...
            ],
            "epochConfig": {
                "allowed_slots": "PrimaryAndSecondaryPlainSlots",
                "c": [ 3, 10 ]
            }
        },
        "balances": {
            "balances": [
                [ "5D34dL5prEUaGNQtPPZ3yN5Y6BnkfXunKXXz6fo7ZJbLwRRH", 100000000000000000 ],
            ],
            ...
        },
        "substrateTest": {
            "authorities": [
                "5GrwvaEF5zXb26Fz9rcQpDWS57CtERHpNehXCPcNoHGKutQY",
                ...
            ]
        },
        "system": {}
    }))
    .build()

Again, resulting ChainSpec instance can be converted to the raw version of chain spec JSON file (which involves calling GenesisBuilder::build_config runtime method) .
It can be also stored in human readable version, sample output here.

chain-spec-builder util

New commands allowing to interact with arbitrary WASM runtime blob were added. Use cases are as follows:

Get default config from runtime

Queries the default genesis config from the provided runtime.wasm and uses it in the chain spec. Can also store runtime's default genesis config in given file (-d):

chain-spec-builder runtime -r runtime.wasm default -d /dev/stdout

Note: GenesisBuilder::create_default_config runtime function is called.

Generate raw storage chain spec using genesis config patch

Patches the runtime's default genesis config with provided patch.json and generates raw storage (-s) version of chain spec:

chain-spec-builder runtime -s -r runtime.wasm patch -p patch.json

Note: GenesisBuilder::build_config runtime function is called.

Generate raw storage chain spec using full genesis config

Build the chain spec using provided full genesis config json file. No defaults will be used:

chain-spec-builder runtime -s -r runtime.wasm full -c full-genesis-config.json

Note: GenesisBuilder::build_config runtime function is called.

Generate human readable chain spec using genesis config patch

chain-spec-builder runtime -r runtime.wasm patch -p patch.json

Note: No runtime API is called.

Generate human readable chain spec using full genesis config

chain-spec-builder runtime -r runtime.wasm full -c full-genesis-config.json

Note: No runtime API is called.

Some extra utils:

  • verify: allows to verify if human readable chain spec is valid (precisely: all required fields in genesis config are initialized),
  • edit, allows to:
    • update the code in given chain spec,
    • convert given chain spec to the raw chain spec,

Some open questions/issues:

  • naming .with_no_genesis_defaults + in chain spec json keys: JsonPatch / JsonFull,
  • GenesisSource source for patch and full config.
  • support for New/Generate commands in `chain-spec-builder? (IMO we can remove them).

Step towards: paritytech/polkadot-sdk#25

polkadot companion: paritytech/polkadot#7508
cumulus companion: paritytech/cumulus#2936

@michalkucharczyk michalkucharczyk force-pushed the mku-chain-spec-support-for-genesis-builder-api branch from 18330f8 to fc55a8d Compare July 12, 2023 13:03
@michalkucharczyk michalkucharczyk added A3-in_progress Pull request is in progress. No review needed at this stage. C1-low PR touches the given topic and has a low impact on builders. B1-note_worthy Changes should be noted in the release notes T2-API This PR/Issue is related to APIs. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Jul 12, 2023
@michalkucharczyk michalkucharczyk marked this pull request as ready for review August 1, 2023 12:15
@michalkucharczyk michalkucharczyk requested review from a team August 1, 2023 12:15
@michalkucharczyk michalkucharczyk added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Aug 1, 2023
@michalkucharczyk
Copy link
Contributor Author

bot clean

@command-bot command-bot bot deleted a comment from paritytech-processbot bot Aug 1, 2023
@command-bot command-bot bot deleted a comment from paritytech-processbot bot Aug 1, 2023
/// accounts. Only works for kitchen-sink runtime
#[derive(Parser, Debug)]
#[command(rename_all = "kebab-case")]
pub struct NewCmd {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO we could remove it as it works only with kitchensink runtime.

Copy link
Member

Choose a reason for hiding this comment

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

I think that can be removed.
If I understood correctly the same result can be accomplished by:

  1. using the generate command to build the patch
  2. then using the runtime to patch the kitchensink wasm (passed from the command line)

Copy link
Member

Choose a reason for hiding this comment

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

But I also noted that the GenerateCmd also uses generate_chain_spec. And thus also uses the kitchensink runtime.

What abut NOT using the kitchensink runtime at all here? I.e. use this Generate command just to build a patch without the code?

But this also requires the ChainSpecBuilder::with_code to be optional (optional code field for patches)

test-utils/runtime/src/lib.rs Outdated Show resolved Hide resolved
bin/node/cli/src/chain_spec.rs Outdated Show resolved Hide resolved
client/chain-spec/src/chain_spec.rs Outdated Show resolved Hide resolved
client/chain-spec/src/chain_spec.rs Outdated Show resolved Hide resolved
client/chain-spec/src/genesis_config_builder.rs Outdated Show resolved Hide resolved
client/chain-spec/src/chain_spec.rs Outdated Show resolved Hide resolved
client/chain-spec/src/chain_spec.rs Show resolved Hide resolved
bin/node/runtime/src/lib.rs Outdated Show resolved Hide resolved
client/chain-spec/src/chain_spec.rs Show resolved Hide resolved
michalkucharczyk and others added 2 commits August 2, 2023 18:07
Co-authored-by: Sebastian Kunert <skunert49@gmail.com>
@michalkucharczyk
Copy link
Contributor Author

bot fmt

@michalkucharczyk
Copy link
Contributor Author

bot rebase

@michalkucharczyk
Copy link
Contributor Author

bot clean

@command-bot command-bot bot deleted a comment from paritytech-processbot bot Aug 2, 2023
@michalkucharczyk michalkucharczyk requested a review from a team August 3, 2023 12:06
},
balances: BalancesConfig {
) -> serde_json::Value {
serde_json::json!({
Copy link
Member

Choose a reason for hiding this comment

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

By using serde_json::Value we are loosing the static type checking advantages we can have when using the types defined by the native runtime (i.e. RuntimeGenesisConfig).

(Maybe I'm going to say something stupid, not 100% confident with metadata) but isn't possible to have some sanity check using the runtime metadata?
Maybe for this we have to investigate (scale_info::meta_type::<Runtime>())

@@ -114,3 +117,6 @@ try-runtime = [
"pallet-transaction-payment/try-runtime",
"sp-runtime/try-runtime"
]

#Enabling this flag will disable GenesisBuilder API implementation in runtime.
disable-genesis-builder = []
Copy link
Member

Choose a reason for hiding this comment

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

Just an idea to open a discussion (not saying this is better).

What about instead using a feature called genesis-builder that is part of the default = ["std", "genesis-builder"]?

Then define genesis-builder feature to also enable serde_json and sp-genesis-builder which may be set as optional dependencies.

client/chain-spec/src/chain_spec.rs Show resolved Hide resolved
bin/utils/chain-spec-builder/bin/main.rs Show resolved Hide resolved
/// accounts. Only works for kitchen-sink runtime
#[derive(Parser, Debug)]
#[command(rename_all = "kebab-case")]
pub struct NewCmd {
Copy link
Member

Choose a reason for hiding this comment

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

I think that can be removed.
If I understood correctly the same result can be accomplished by:

  1. using the generate command to build the patch
  2. then using the runtime to patch the kitchensink wasm (passed from the command line)

/// into `RawGenesis` as `genesis::top::raw::0x3a636f6465` (which is
/// [`sp_core::storage::well_known_keys::CODE`]). If the spec is already in `raw` format, and
/// contains `genesis::top::raw::0x3a636f6465` field it will be updated with content of `code`
/// field (if present).
Copy link
Member

Choose a reason for hiding this comment

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

Dumb question. Do we care somewhere about preserving field order in the json? Looks not... but I just wanted to open the discussion

@@ -13,15 +13,30 @@ readme = "README.md"
targets = ["x86_64-unknown-linux-gnu"]

[dependencies]
codec = { package = "parity-scale-codec", version = "3.6.1", default-features = false, features = ["derive"] }
json-patch = { version = "1.0.0", default-features = false }
Copy link
Member

Choose a reason for hiding this comment

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

I'd just add this few lines function in chain-spec/lib.rs and remove the dependency.
IMO adding a dependency to use a trivial function is not good (again, this is my opinion).

client/chain-spec/src/lib.rs Show resolved Hide resolved
@@ -671,7 +669,6 @@ pub mod pallet {
<UpgradedToU32RefCount<T>>::put(true);
<UpgradedToTripleRefCount<T>>::put(true);

sp_io::storage::set(well_known_keys::CODE, &self.code);
sp_io::storage::set(well_known_keys::EXTRINSIC_INDEX, &0u32.encode());
Copy link
Member

@davxy davxy Aug 4, 2023

Choose a reason for hiding this comment

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

Looking at this now I wonder what is the point of setting to 0 the well known EXTRINSIC_INDEX during build.

  • maybe we can modify this with an unwrap_or_default() (thus not returning an Option).
  • On block initialization this is always set to 0

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T2-API This PR/Issue is related to APIs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants