Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only customized fields in preset json blob? #5700

Closed
michalkucharczyk opened this issue Sep 13, 2024 · 2 comments · Fixed by #5813
Closed

Only customized fields in preset json blob? #5700

michalkucharczyk opened this issue Sep 13, 2024 · 2 comments · Fixed by #5813

Comments

@michalkucharczyk
Copy link
Contributor

michalkucharczyk commented Sep 13, 2024

Probably not super important, but wanted to know your opinion on that, so I am leaving it here for your consideration.

This is about how to provide preset's JSON and what is potential outcome.

There are two approaches.

JSON blob, containing only overridden fields
fn preset() -> serde_json::Value {
    serde_json::json!({
        "balances": BalancesConfig {
            balances: endowed_accounts.iter().cloned().map(|k| (k, 1u128 << 60)).collect::<Vec<_>>(),
        },
        "parachainInfo": ParachainInfoConfig {
            parachain_id: id,
            ..Default::default()
        },
        "collatorSelection": CollatorSelectionConfig {
            invulnerables: invulnerables.iter().cloned().map(|(acc, _)| acc).collect(),
            candidacy_bond: BRIDGE_HUB_ROCOCO_ED * 16,
            ..Default::default()
        },
        "session": SessionConfig {
            keys: invulnerables
                .into_iter()
                .map(|(acc, aura)| {
                    (
                        acc.clone(),                         // account id
                        acc,                                 // validator id
                        SessionKeys { aura },            // session keys
                    )
                })
                .collect(),
            ..Default::default()
        },
        "polkadotXcm": PolkadotXcmConfig {
            safe_xcm_version: Some(SAFE_XCM_VERSION),
            ..Default::default()
        },
        "bridgeWestendGrandpa": BridgeWestendGrandpaConfig {
            owner: bridges_pallet_owner.clone(),
            ..Default::default()
        },
        "bridgeWestendMessages": BridgeWestendMessagesConfig {
            owner: bridges_pallet_owner.clone(),
            ..Default::default()
        },
        "ethereumSystem": EthereumSystemConfig {
            para_id: id,
            asset_hub_para_id,
            ..Default::default()
        }
    })
}

and second:

full `RuntimeGenesisConfig` blob, containing defaults and overwritten fields:
fn preset() -> serde_json::Value {
    let config = RuntimeGenesisConfig {
        balances: BalancesConfig {
            balances: endowed_accounts
                .iter()
                .cloned()
                .map(|k| (k, 1u128 << 60))
                .collect::<Vec<_>>(),
        },
        parachain_info: ParachainInfoConfig { parachain_id: id, ..Default::default() },
        collator_selection: CollatorSelectionConfig {
            invulnerables: invulnerables.iter().cloned().map(|(acc, _)| acc).collect(),
            candidacy_bond: BRIDGE_HUB_ROCOCO_ED * 16,
            ..Default::default()
        },
        session: SessionConfig {
            keys: invulnerables
                .into_iter()
                .map(|(acc, aura)| {
                    (
                        acc.clone(),          // account id
                        acc,                  // validator id
                        SessionKeys { aura }, // session keys
                    )
                })
                .collect(),
            ..Default::default()
        },
        polkadot_xcm: PolkadotXcmConfig {
            safe_xcm_version: Some(SAFE_XCM_VERSION),
            ..Default::default()
        },
        bridge_westend_grandpa: BridgeWestendGrandpaConfig {
            owner: bridges_pallet_owner.clone(),
            ..Default::default()
        },
        bridge_westend_messages: BridgeWestendMessagesConfig {
            owner: bridges_pallet_owner.clone(),
            ..Default::default()
        },
        ethereum_system: EthereumSystemConfig {
            para_id: id,
            asset_hub_para_id,
            ..Default::default()
        },
        ..Default::default()
    };

    serde_json::to_value(config).expect("Could not build genesis config.")
}

When we generate two two runtime wasm blobs using two above approaches, and then we use chain-spec-builder display-preset util to get the json preset representation, and we display the diff, we will get following:

$ diff f.json j.json
2,5d1
<   "aura": {
<     "authorities": []
<   },
<   "auraExt": {},
58,68d53
<   "bridgePolkadotBulletinGrandpa": {
<     "initData": null,
<     "owner": null
<   },
<   "bridgePolkadotBulletinMessages": {
<     "openedLanes": [],
<     "operatingMode": {
<       "Basic": "Normal"
<     },
<     "owner": null
<   },
80,83d64
<   "bridgeWestendParachains": {
<     "operatingMode": "Normal",
<     "owner": null
<   },
99d79
<   "parachainSystem": {},
121,130d100
<   },
<   "system": {},
<   "transactionPayment": {
<     "multiplier": "1000000000000000000"
<   },
<   "xcmOverBridgeHubWestend": {
<     "openedBridges": []
<   },
<   "xcmOverPolkadotBulletin": {
<     "openedBridges": []

So the preset constructed from the full RuntimeGenesisConfig struct contains some fields that are just defaults and were not actually customized.

In practice, in most cases that should not matter, but it potentially may lead to some strange behavior - e.g. imagine fetching the preset to the file, then using it to against different runtime version (e.g. newer version with transactionPayment::multiplier updated) where defaults are changed may lead to some strange situation.

On the other hand - using JSON approach may lead to invalid JSON (and we would addition coverage in tests).

tagging @kianenigma , @bkchr.

Originally posted by @michalkucharczyk in #5327 (comment)

@michalkucharczyk
Copy link
Contributor Author

some proposal from #5327 discussion: #5327 (comment)

@bkontur bkontur added A4-needs-backport Pull request must be backported to all maintained releases. and removed A4-needs-backport Pull request must be backported to all maintained releases. labels Sep 13, 2024
@bkontur
Copy link
Contributor

bkontur commented Sep 18, 2024

  • add tests for presets to verify correctness - see comment

github-merge-queue bot pushed a commit that referenced this issue Sep 22, 2024
It is a first step for switching to the `frame-omni-bencher` for CI.

This PR includes several changes related to generating chain specs plus:

- [x] pallet `assigned_slots` fix missing `#[serde(skip)]` for phantom
- [x] pallet `paras_inherent` benchmark fix - cherry-picked from
#5688
- [x] migrates `get_preset` to the relevant runtimes
- [x] fixes Rococo genesis presets - does not work
https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7317249
- [x] fixes Rococo benchmarks for CI 
- [x] migrate westend genesis
- [x] remove wococo stuff

Closes: #5680

## Follow-ups
- Fix for frame-omni-bencher
#5655
- Enable new short-benchmarking CI -
#5706
- Remove gitlab pipelines for short benchmarking
- refactor all Cumulus runtimes to use `get_preset` -
#5704
- #5705
- #5700
- [ ] Backport to the stable

---------

Co-authored-by: command-bot <>
Co-authored-by: ordian <noreply@reusable.software>
bkontur added a commit that referenced this issue Sep 22, 2024
It is a first step for switching to the `frame-omni-bencher` for CI.

This PR includes several changes related to generating chain specs plus:

- [x] pallet `assigned_slots` fix missing `#[serde(skip)]` for phantom
- [x] pallet `paras_inherent` benchmark fix - cherry-picked from
#5688
- [x] migrates `get_preset` to the relevant runtimes
- [x] fixes Rococo genesis presets - does not work
https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7317249
- [x] fixes Rococo benchmarks for CI
- [x] migrate westend genesis
- [x] remove wococo stuff

Closes: #5680

- Fix for frame-omni-bencher
#5655
- Enable new short-benchmarking CI -
#5706
- Remove gitlab pipelines for short benchmarking
- refactor all Cumulus runtimes to use `get_preset` -
#5704
- #5705
- #5700
- [ ] Backport to the stable

---------

Co-authored-by: command-bot <>
Co-authored-by: ordian <noreply@reusable.software>
(cherry picked from commit 8735c66)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants