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

Upgrade to polkadot-v0.9.26 #850

Merged
merged 20 commits into from
Nov 24, 2022
Merged

Upgrade to polkadot-v0.9.26 #850

merged 20 commits into from
Nov 24, 2022

Conversation

@Chralt98
Copy link
Member

I am waiting for the full CI checks and merge conflicts.

@vivekvpandya vivekvpandya marked this pull request as ready for review November 1, 2022 06:47
@maltekliemann maltekliemann added the s:review-needed The pull request requires reviews label Nov 2, 2022
Copy link
Member

@Chralt98 Chralt98 left a comment

Choose a reason for hiding this comment

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

I think @sea212 , you have the best experience in upgrading to new polkadot versions. So Vivek, assume, that I might missed some important stuff.

zrml/court/src/lib.rs Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
zrml/prediction-markets/src/mock.rs Outdated Show resolved Hide resolved
runtime/battery-station/src/lib.rs Show resolved Hide resolved
node/Cargo.toml Outdated Show resolved Hide resolved
runtime/common/src/lib.rs Show resolved Hide resolved
runtime/common/src/lib.rs Outdated Show resolved Hide resolved
@sea212 sea212 linked an issue Nov 4, 2022 that may be closed by this pull request
Copy link
Member

@sea212 sea212 left a comment

Choose a reason for hiding this comment

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

Changes look good to me. There are some review suggestions in regards to removing some deprecated code in client and bumping the recursion depth in the runtime. In addition, we have to add two more benchmarks that were made available in some pallets. Other than that the PR can be merged as it is.

  • All dependencies updated and Cargo.lock updated
  • Runtimes builds in all modes
  • Client builds in all modes
  • Cleanly use original repositories instead of forks wherever possible (no duplicate crates [fork/original] in Cargo.lock)
  • Runtime logic appropriately adjusted
  • Benchmarks work
  • Try-runtime upgrade succeeds
  • Ensure all benchmarkable pallets are listed as benchmarks and have a custom weights file.

The runtime-benchmarks feature was made available for cumulus-pallet-parachain-system and session-keys-primitives. Please add those to the runtime-benchmarks feature in our runtimes, to the add_benchmark! and list_benchmark! marcros and finally configure the pallets to use our custom weight for them in runtime/common/weights (for this you have to copy weight files from the original repositories for now, we'll replace those with our benchmarks before the release).

node/Cargo.toml Outdated Show resolved Hide resolved
@@ -224,7 +224,7 @@ impl sc_cli::SubstrateCli for RelayChainCli {
#[derive(Debug, Parser)]
pub struct ExportGenesisStateCommand {
Copy link
Member

Choose a reason for hiding this comment

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

We should be able to remove custom ExportGenesisStateCommand subcommand struct, because it is included in cumulus now:
substrate-developer-hub/substrate-parachain-template@polkadot-v0.9.23...polkadot-v0.9.26#diff-76397c90c6ee2b6b98efd989d3ae3c11e0bde3db30b5ef46f2762180f56041a8

In node/cli.rs, we can then replace:

    /// Export the genesis state of the parachain.
    #[cfg(feature = "parachain")]
    #[clap(name = "export-genesis-state")]
    ExportGenesisState(cli_parachain::ExportGenesisStateCommand),

    /// Export the genesis wasm of the parachain.
    #[cfg(feature = "parachain")]
    #[clap(name = "export-genesis-wasm")]
    ExportGenesisWasm(cli_parachain::ExportGenesisWasmCommand),

by

    /// Export the genesis state of the parachain.
    #[cfg(feature = "parachain")]
    ExportGenesisState(cumulus_client_cli::ExportGenesisStateCommand),

    /// Export the genesis wasm of the parachain.
    #[cfg(feature = "parachain")]
    ExportGenesisWasm(cumulus_client_cli::ExportGenesisWasmCommand),

See
substrate-developer-hub/substrate-parachain-template@polkadot-v0.9.23...polkadot-v0.9.26#diff-76397c90c6ee2b6b98efd989d3ae3c11e0bde3db30b5ef46f2762180f56041a8L46-L78
and
substrate-developer-hub/substrate-parachain-template@polkadot-v0.9.23...polkadot-v0.9.26#diff-76397c90c6ee2b6b98efd989d3ae3c11e0bde3db30b5ef46f2762180f56041a8R27-R31

Copy link
Member

Choose a reason for hiding this comment

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

Also in line 53 we can parse the command directly using clap instead of parsing functions exposed by polkadot client RunCmd: substrate-developer-hub/substrate-parachain-template@polkadot-v0.9.23...polkadot-v0.9.26#diff-76397c90c6ee2b6b98efd989d3ae3c11e0bde3db30b5ef46f2762180f56041a8R91

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we can remove completely as we are taking parachain_id as parameter and that goes to following code to fill details in Extensions struct.

32 pub fn battery_station_staging_config(                                                                                                                                                                                                                                                                                   
 31     #[cfg(feature = "parachain")] parachain_id: cumulus_primitives_core::ParaId,                                                                                                                                                                                                                                         
 30 ) -> Result<BatteryStationChainSpec, String> {                                                                                                                                                                                                                                                                           
...                                                                                                                                                                                                                                                          
  8         #[cfg(feature = "parachain")]                                                                                                                                                                                                                                                                                    
  7         crate::chain_spec::Extensions {                                                                                                                                                                                                                                                                                  
  6             relay_chain: "rococo".into(),                                                                                                                                                                                                                                                                                
  5             parachain_id: parachain_id.into(),                                                                                                                                                                                                                                                                           
  4         },                                                                                                                                                                                                                                                                                                               
  3         #[cfg(not(feature = "parachain"))]                                                                                                                                                                                                                                                                               
  2         Default::default(),                                                                                                                                                                                                                                                                                              
  1     ))                                                                                                                                                                                                                                                                                                                   
153 }  

For others it is possible as they don't require parameter for parachain_id instead they use constant value
https://github.com/AcalaNetwork/Acala/blob/master/node/service/src/chain_spec/acala.rs#L97

Copy link
Member

Choose a reason for hiding this comment

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

Based on this let us try to find a way to be as close as possible to the parachain template code. Because currently the parachain node doesn't sync, we should try to use the new ExportGenesisState and ExportGenesisWasm method to generate our new genesis configuration for onboarding it on the rococo relaychain.

runtime/battery-station/src/lib.rs Show resolved Hide resolved
runtime/common/src/lib.rs Show resolved Hide resolved
@Chralt98
Copy link
Member

Chralt98 commented Nov 4, 2022

Thanks for sharing your check list and the method you use to find the needed changes. Saved ✅

sea212
sea212 previously approved these changes Nov 8, 2022
@sea212
Copy link
Member

sea212 commented Nov 10, 2022

@Chralt98 Can you please review the changes? Before approving, please also execute the node once locally and ensure it syncs with BS and Zeitgeist mainnet.

Copy link
Member

@Chralt98 Chralt98 left a comment

Choose a reason for hiding this comment

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

Always hard to catch possible flaws in polkadot version updates. I checked a couple of dependencies and looked correct to me. Lucky, that we have the CI.

I executed:

cargo build --bin zeitgeist --features parachain --manifest-path Cargo.toml --profile=production

# main-net sync check
./target/production/zeitgeist --tmp

# test-net sync check
./target/production/zeitgeist --chain=battery_station --tmp

After the upgrade to polkadot 0.9.26 the production parachain node did produce the following output:

main-net sync:

Bildschirmfoto 2022-11-10 um 09 46 38

test-net sync:

Bildschirmfoto 2022-11-10 um 09 44 56

The parachain found no peers in both cases.

Standalone (without feature parachain and without chain argument) does produce blocks in standalone local dev chain mode:

Bildschirmfoto 2022-11-10 um 10 20 48

node/src/chain_spec/mod.rs Outdated Show resolved Hide resolved
node/src/chain_spec/mod.rs Outdated Show resolved Hide resolved
node/src/chain_spec/mod.rs Outdated Show resolved Hide resolved
node/Cargo.toml Outdated Show resolved Hide resolved
node/Cargo.toml Show resolved Hide resolved
@@ -224,7 +224,7 @@ impl sc_cli::SubstrateCli for RelayChainCli {
#[derive(Debug, Parser)]
pub struct ExportGenesisStateCommand {
Copy link
Member

Choose a reason for hiding this comment

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

Based on this let us try to find a way to be as close as possible to the parachain template code. Because currently the parachain node doesn't sync, we should try to use the new ExportGenesisState and ExportGenesisWasm method to generate our new genesis configuration for onboarding it on the rococo relaychain.

@Chralt98
Copy link
Member

@sea212 You might have something against the approach of using a constant parachain id. I am open for a revert on this too.. but love on the other hand the simplicity of a constant ...

sea212
sea212 previously approved these changes Nov 24, 2022
Copy link
Member

@sea212 sea212 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for polishing the codebase, it's shiney again 😎

Can you create an issue in regards to fixing the client (connection issue)? Priority should be critical imo.

@vivekvpandya
Copy link
Contributor Author

LGTM, thanks for polishing the codebase, it's shiney again sunglasses

Can you create an issue in regards to fixing the client (connection issue)? Priority should be critical imo.

Please find new issue #883

Chralt98
Chralt98 previously approved these changes Nov 24, 2022
Copy link
Member

@Chralt98 Chralt98 left a comment

Choose a reason for hiding this comment

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

Let us roll out the non-client runtime upgrade!

@Chralt98 Chralt98 added s:accepted This pull request is ready for merge s:review-needed The pull request requires reviews and removed s:review-needed The pull request requires reviews s:accepted This pull request is ready for merge labels Nov 24, 2022
Copy link
Member

@Chralt98 Chralt98 left a comment

Choose a reason for hiding this comment

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

just CI checks are not fully correct

@Chralt98
Copy link
Member

Can we re-run the workflow?

@Chralt98 Chralt98 self-requested a review November 24, 2022 12:33
@Chralt98 Chralt98 dismissed their stale review November 24, 2022 12:35

workflow run is incorrect

@Chralt98 Chralt98 requested a review from sea212 November 24, 2022 13:51
@sea212 sea212 added s:accepted This pull request is ready for merge and removed s:review-needed The pull request requires reviews labels Nov 24, 2022
@sea212 sea212 added this to the v0.3.7 milestone Nov 24, 2022
@sea212 sea212 merged commit 8de3b3a into main Nov 24, 2022
@sea212 sea212 deleted the upgrade_0926 branch November 24, 2022 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s:accepted This pull request is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade codebase to polkadot-v0.9.26
4 participants