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

rococo-runtime: RococoGenesisExt removed #1490

Merged
merged 17 commits into from
Sep 28, 2023

Conversation

michalkucharczyk
Copy link
Contributor

@michalkucharczyk michalkucharczyk commented Sep 11, 2023

RococoGenesisExt is removed. It was the hack to allow overwriting EpochDurationInBlocks. Removal of RococGenesisExt prevents from manipulating the state to change the runtime constants.

Changes:

  • Environment variable which controls the time::EpochDurationInBlocks value was added: ROCOCO_EPOCH_DURATION (epoch duration will be set to the value of env),
  • 10,100,600 versions of rococo-runtime are built in CI and put into polkadot-debug docker image.

rococo-runtime building examples:

  • to build runtime for versi_staging_testnet which had EpochDurationInBlocks set to 100:
    ROCOCO_EPOCH_DURATION=100 cargo build --features=fast-runtime -p rococo-runtime
    
  • to build runtime for wococo_development
    ROCOCO_EPOCH_DURATION=10 cargo build --features=fast-runtime -p rococo-runtime
    
  • to build versi-staging chain spec:
    ROCOCO_EPOCH_DURATION=100 cargo run -p polkadot --features=fast-runtime -- build-spec --chain versi-staging --raw 
    
  • to build wococo-dev chain spec:
    ROCOCO_EPOCH_DURATION=10 cargo run -p polkadot  --features=fast-runtime  -- build-spec --chain wococo-dev --raw 
    

Chain-specs can also be built using provided sample script. As the epoch duration is hard-coded into wasm blob, it is enough to replace the code field.

Step towards: #25

@michalkucharczyk
Copy link
Contributor Author

michalkucharczyk commented Sep 11, 2023

It is extracted part of #1256

@michalkucharczyk michalkucharczyk added the T4-runtime_API This PR/Issue is related to runtime APIs. label Sep 11, 2023
@michalkucharczyk michalkucharczyk requested review from bkchr and a team September 11, 2023 11:13
@michalkucharczyk
Copy link
Contributor Author

@paritytech/devrel , @the-right-joyce

I wanted to give you a heads-up about some changes that have just been made in this PR. I've modified the default EpochDurationInBlocks for several ChainSpecs, namely: versi-local, wococo-local, rococo-local, wococo-dev, versi-dev, rococo-dev(where duration was previously set to 10), andversi-staging` (where duration was previously set to 100).

You can find additional details in the PR description.

This change could affect tutorials (and docs maybe?) that rely on timing within these runtimes. Therefore, I request you to please review and update them accordingly, and include information on how to adjust timings based on these changes.

Thank you for your attention.

note: this is copy of comment previously made in polkadot repo.

@ordian
Copy link
Member

ordian commented Sep 12, 2023

cc @Sophia-Gold since you're using session_length_in_blocks in your test

@PierreBesson
Copy link

From the point of view of the Devops team that is generating those chainspec to launch testnets, this doesn't look very
practical to use as it will force us to work from source instead from a binary for just 1 config value tweak.

If we could simply have a --chain rococo-fast-runtime already available in the built node it would be most convenient to do our job and automate our processes for launching custom Rococo-based testnet such as Versi.

@bkchr
Copy link
Member

bkchr commented Sep 12, 2023

From the point of view of the Devops team that is generating those chainspec to launch testnets, this doesn't look very
practical to use as it will force us to work from source instead from a binary for just 1 config value tweak.

Our test nets are running with at least 1 hour session length any way or not? And 1 hour is still the default.

@bkchr bkchr added T0-node This PR/Issue is related to the topic “node”. and removed T4-runtime_API This PR/Issue is related to runtime APIs. labels Sep 12, 2023
@eskimor
Copy link
Member

eskimor commented Sep 12, 2023

From the point of view of the Devops team that is generating those chainspec to launch testnets, this doesn't look very
practical to use as it will force us to work from source instead from a binary for just 1 config value tweak.

Our test nets are running with at least 1 hour session length any way or not? And 1 hour is still the default.

No, Versi has shorter sessions and we would like to keep it at that as session changes are something very crucial to test.

@bkchr
Copy link
Member

bkchr commented Sep 12, 2023

No, Versi has shorter sessions and we would like to keep it at that as session changes are something very crucial to test.

Yeah and it will still be possible. It will just work differently. This entire changing of the session length was a hack from the beginning, that I should never have used :P

@PierreBesson
Copy link

I appreciate the change to make it fully configurable. However, being able to change runtime constants without building from source could be very useful. But I guess, we will have to accept that we won't have a way to build fast-runtime chainspecs from the released binary after this change.

@michalkucharczyk

  • Would it be possible to keep the short default epoch duration for the Versi runtime ?
  • Would it also be possible to offer a configurable epoch duration for the westend runtime as well ?

@michalkucharczyk
Copy link
Contributor Author

michalkucharczyk commented Sep 13, 2023

However, being able to change runtime constants without building from source could be very useful.

We want to go away from configuring runtime parameters using state trie.

The only other solution I see, is to - by default - build the rococo runtime binary with smaller epochDuration value and name it versi-runtime. And hide it behind the feature as other runtimes. This will lengthen the build process.
If that is acceptable, I can do it. However I believe we should not aim into adding more pre-configured runtimes into default build. Adding versi seems fair to me.

The following values were used in runtimes, I assume that we are talking only about keeping staging versions?:

dev local staging
rococo 10 10 600
versi 10 10 100
wococo 10 10

Would it also be possible to offer a configurable epoch duration for the westend runtime as well ?

Technically should be doable. The question is if we should build another runtime version by default or is it OK for you to build it on your side? Maybe we should move this request to new ticket?

@bkchr
Copy link
Member

bkchr commented Sep 13, 2023

I appreciate the change to make it fully configurable. However, being able to change runtime constants without building from source could be very useful.

This is one of the only ones we support this. I have done this as a hack in 2020 or somewhere then to make it easy to test stuff locally. Versi for sure doesn't fall into this category and I see no problem for you to just rebuild the runtime. In the future we will also be able to support to only build the runtime to build a chain spec, so you will not need to build the entire node. This will then result in a quite fast turn around (assuming you only change something in the runtime lib.rs).

This said, we probably should take the performance hit and also build a fast runtime for rococo-local for now. Sadly parachains are still not able to onboard at genesis and thus, people would be confused why stuff isn't working as they expect it to work.

@michalkucharczyk
Copy link
Contributor Author

@bkchr I've just realized that it may not be doable for native runtime.

We can't just build the other version of native rococo-runtime crate. We would have to somehow re-use the rococo-runtime sources in rococo-runtime-fast (or whatever the name will be).

Am I missing something?

@bkchr
Copy link
Member

bkchr commented Sep 13, 2023

The native runtime isn't used anymore. So, this is fine.

We can do something like here.

@BulatSaif
Copy link
Contributor

The current versi and wococo network is different from what we have in the code. We use Versi and wococo based on westend runtime, surprise!

The current process to launch a new testnet is as follows:

  1. Run build-spec --chain westend-staging, which generates a plain JSON file.
  2. Override fields such as session keys, sudo, balance, id, name... Almost all fields except for "genesis.runtime.system.code."
  3. Generate a new raw chainspec using: build-spec --chain ./chainspec-plain.json --raw.
    You can find an example here.

I propose instead having:

dev local staging
rococo 10 10 600
versi 10 10 100
wococo 10 10

Let's have:

dev local staging
rococo 10 100 600

I hope this will work and the only difference between the dev, local, and staging runtime is the session time? I know that dev, local have different session keys, but we can override them.

@bkchr
Copy link
Member

bkchr commented Sep 13, 2023

Yeah no, we will not support three different lengths for the epoch duration.

@michalkucharczyk can provide you a script for building the stuff in the correct way.

@BulatSaif given that you already patch the chainspec manually, you could also overwrite the system.code field. Then you would only need to recompile the runtime which is way faster.

@BulatSaif
Copy link
Contributor

@BulatSaif given that you already patch the chainspec manually, you could also overwrite the system.code field. Then you would only need to recompile the runtime which is way faster.

We did it in the past as a result we had a chain that is impossible to update (current wococo). After we apply the runtime update chain will stop producing the blocks Error Corrupted state at.
I assume some migration depends on session length, or runtime which I built was incorrect: paritytech/polkadot#7416

I would like to avoid modifying the substrate code by the DevOps team as we are not familiar with the codebase.

But we need a way to deploy and update chains with small session time. It will speed up the development of the parachain team since they can onboard or off board parachains faster.

@bkchr
Copy link
Member

bkchr commented Sep 13, 2023

I would like to avoid modifying the substrate code by the DevOps team as we are not familiar with the codebase.

Yeah that is reasonable. However, noting down which network uses which session length sounds like a doable task. You will not need to modify any code, you will just need to set an env variable at build time to the appropriate number.

@michalkucharczyk
Copy link
Contributor Author

@michalkucharczyk can provide you a script for building the stuff in the correct way.

f84cd5d

@BulatSaif
Copy link
Contributor

You will not need to modify any code, you will just need to set an env variable at build time to the appropriate number.

This is the good news, can we have similar option ROCOCO_EPOCH_DURATION for westend?

@michalkucharczyk michalkucharczyk marked this pull request as draft September 13, 2023 20:09
@michalkucharczyk
Copy link
Contributor Author

michalkucharczyk commented Sep 13, 2023

I did some experiment, and added pre-built runtimes into polkadot-debug docker image under /polkadot/runtimes directory.

Rutimes are not explicitly built by cargo build ... command. I added some extra commands into CI in order to build 10 and 100 versions of rococo runtime:

- time ROCOCO_EPOCH_DURATION=10 ./polkadot/scripts/build-only-wasm.sh rococo-runtime $(pwd)/runtimes/rococo-runtime-10/
- time ROCOCO_EPOCH_DURATION=100 ./polkadot/scripts/build-only-wasm.sh rococo-runtime $(pwd)/runtimes/rococo-runtime-100/

There is also another small script to update the code in chain-spec (2eddd98):

./polkadot/scripts/update-chain-spec-with-fast-rococo.sh paritypr/polkadot-debug:1490-657ae27f 100

If that is acceptable, we could keep this as final solution.
If not, we could merge it, and keep it until devops team finds out how to deal with change introduced by this PR.

@bkchr, @BulatSaif wdyt?

@BulatSaif
Copy link
Contributor

If that is acceptable, we could keep this as final solution.
If not, we could merge it, and keep it until devops team finds out how to deal with change introduced by this PR.

The proposed alternative solution to build chainspec with shorter epoch duration looks good. I did not run the scripts but it looks clear and we can use it.

@michalkucharczyk michalkucharczyk marked this pull request as ready for review September 14, 2023 16:10
@michalkucharczyk michalkucharczyk requested review from bkchr and a team September 22, 2023 07:00
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Mainly nitpicks. For bash scripts IDK. They look very complicated and not sure what they are doing. I would have thought this is just some ROCOCO_EPOCH_DURATION=100 build-wasm-only.sh rococo-runtime script. However, I have no strong views around this, as long as someone finds it useful.

polkadot/runtime/rococo/build.rs Show resolved Hide resolved
polkadot/runtime/rococo/build.rs Show resolved Hide resolved
polkadot/scripts/update-chain-spec-with-fast-rococo.sh Outdated Show resolved Hide resolved
polkadot/runtime/rococo/constants/src/lib.rs Outdated Show resolved Hide resolved
@paritytech-ci paritytech-ci requested a review from a team September 26, 2023 11:48
@michalkucharczyk michalkucharczyk requested a review from a team September 28, 2023 13:28
@michalkucharczyk michalkucharczyk merged commit 50242a6 into master Sep 28, 2023
8 of 9 checks passed
@michalkucharczyk michalkucharczyk deleted the mku-rococo-runtime-genesis-ext-removed branch September 28, 2023 16:29
ordian added a commit that referenced this pull request Oct 3, 2023
* master: (24 commits)
  Init System Parachain storage versions and add migration check jobs to CI (#1344)
  no-bound derives: Use absolute path for `core` (#1763)
  migrate alliance, fast-unstake and bags list to use derive-impl (#1636)
  Tvl pool staking (#1322)
  improve service error (#1734)
  frame-support: `RuntimeDebug\Eq\PartialEq` impls for `Imbalance` (#1717)
  Point documentation links to monorepo (#1741)
  [NPoS] Fix for Reward Deficit in the pool (#1255)
  Move import queue from `ChainSync` to `SyncingEngine` (#1736)
  Enable mocking contracts (#1331)
  Revert "fix(review-bot): pull secrets from `master` environment" (#1748)
  Remove kusama and polkadot runtime crates (#1731)
  Use `Extensions` to register offchain worker custom extensions (#1719)
  [RPC-Spec-V2] chainHead: use integer for block index and adjust RuntimeVersion JSON format (#1666)
  fix(review-bot): pull secrets from `master` environment (#1745)
  Fix `subkey inspect` output text padding (#1744)
  archive: Implement height, hashByHeight and call (#1582)
  rpc/client: Propagate `rpc_methods` method to reported methods (#1713)
  rococo-runtime: `RococoGenesisExt` removed (#1490)
  PVF: more filesystem sandboxing (#1373)
  ...
ordian added a commit that referenced this pull request Oct 10, 2023
* tsv-disabling-node-side: (24 commits)
  Init System Parachain storage versions and add migration check jobs to CI (#1344)
  no-bound derives: Use absolute path for `core` (#1763)
  migrate alliance, fast-unstake and bags list to use derive-impl (#1636)
  Tvl pool staking (#1322)
  improve service error (#1734)
  frame-support: `RuntimeDebug\Eq\PartialEq` impls for `Imbalance` (#1717)
  Point documentation links to monorepo (#1741)
  [NPoS] Fix for Reward Deficit in the pool (#1255)
  Move import queue from `ChainSync` to `SyncingEngine` (#1736)
  Enable mocking contracts (#1331)
  Revert "fix(review-bot): pull secrets from `master` environment" (#1748)
  Remove kusama and polkadot runtime crates (#1731)
  Use `Extensions` to register offchain worker custom extensions (#1719)
  [RPC-Spec-V2] chainHead: use integer for block index and adjust RuntimeVersion JSON format (#1666)
  fix(review-bot): pull secrets from `master` environment (#1745)
  Fix `subkey inspect` output text padding (#1744)
  archive: Implement height, hashByHeight and call (#1582)
  rpc/client: Propagate `rpc_methods` method to reported methods (#1713)
  rococo-runtime: `RococoGenesisExt` removed (#1490)
  PVF: more filesystem sandboxing (#1373)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants