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.

Sign up for GitHub

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

Jump to bottom

Make compatible with Polkadot, Kusama and Rococo #217

Open
wants to merge 80 commits into
base: ilya/parachain-update
Choose a base branch
from
Open

Make compatible with Polkadot, Kusama and Rococo #217

wants to merge 80 commits into from

Conversation

ayushmishra2005
Copy link
Contributor

@ayushmishra2005 ayushmishra2005 commented Aug 9, 2021 •
edited

resolves #216

This PR contains the changes to update parachain code to be compatible for Kusama.

  • 1. - update only codebase to be compatible with Kusama version ... based on confirmation from the community

  • 1.1. - Upgrade to Polkadot 0.9.10

  • 1.2. - Upgrade to Polkadot 0.9.11

  • 1.3. - Upgrade to Polkadot 0.9.12

  • 2.1. - testing has been performed but limited to building the code and executing test-cases including .

  • 2.2. - removed use of rustfmt to be consistent with substrate-parachain-template and DataHighway-DHX/node 'master' branch to maintain ease of developability

  • 2.3. - test against polkadot (not westend) running in --chain <dev|local|staging> using polkadot-launch

  • 2.4. - Test registering parachain locally before attempting to connect to Rococo!

  • 3.1.1 - requested rococo tokens

  • 3.1.2 - register paraId on Rococo testnet

  • 3.1.3 - other relevant testing on Rococo to check that our desired parachain functionality works on a parachain testnet

  • 3.2 - optionally repeat 3.1.1 - 3.1.4 for Westend

  • 4. - repeat 3.1.1 - 3.1.4 for Polkadot

  • 5. request to merge update into 'ilya/parachain'

ltfschoen
ltfschoen reviewed Aug 11, 2021
View reviewed changes
node/src/chain_spec.rs Outdated Show resolved Hide resolved
ltfschoen
ltfschoen reviewed Aug 11, 2021
View reviewed changes
node/src/command.rs Outdated
[RelayChainCli::executable_name().to_string()].iter().chain(cli.relaychain_args.iter()),
);

let id = ParaId::from(cli.run.parachain_id.or(para_id).unwrap_or(200));
let id = ParaId::from(cli.run.parachain_id.or(para_id).unwrap_or(2000));
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need to specifically request this Parachain ID?

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 am not sure. In rococo-v1, it was 200 https://github.com/substrate-developer-hub/substrate-parachain-template/blob/40b858149b212e493da08d80e0fc5b06a6b0b72d/node/src/command.rs#L274

But in polkadot-v0.9.8, they replaced it with 2000.

Please see these 2 commits
substrate-developer-hub/substrate-parachain-template@cdc5e5e#diff-8ce03a5a2b79f3021ff2d6322d13448db8cfd5fb9267517ab4b96d0d299ed45f

substrate-developer-hub/substrate-parachain-template@c115b3a#diff-8ce03a5a2b79f3021ff2d6322d13448db8cfd5fb9267517ab4b96d0d299ed45f

@ltfschoen please let me know if you want me to roll back it to 200.

Copy link
Collaborator

@ltfschoen ltfschoen Aug 16, 2021 •
edited

Choose a reason for hiding this comment

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

well this PR is for our parachain that connects to Kusama, so i'd imagine the default parachain id that it'd fallback to in the cli would be our DataHighway parachain id. so far it displays the live Kusama parachain id's that are being used here https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Fkusama.api.onfinality.io%2Fpublic-ws#/parachains, and it looks like 2000 has already been taken by Karura, so i think we should use expect("Missing ParaId"). i don't know why they switched to a fallback to the Karura parachain id in what's supposed to be a "template".

i'd suggest finding out how to register a parachain id in the Parachain Technical room on Matrix.

Copy link
Collaborator

@ltfschoen ltfschoen Aug 16, 2021 •
edited

Choose a reason for hiding this comment

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

it appears that para id 2000 may be Rococo-specific, according to https://substrate.dev/cumulus-workshop/#/en/3-parachains/2-register, please double check in Parachain Technical room on Matrix if it applies to Kusama too.
Note that in the README it says "When registering to Rococo, ensure you set a ParaId > 1000, below 1000 is meant for system parachains." https://github.com/substrate-developer-hub/substrate-parachain-template/tree/40b858149b212e493da08d80e0fc5b06a6b0b72d#register-on-the-relay-with-sudo, so please find out what rules apply when registering to Kusama

Copy link
Collaborator

@ltfschoen ltfschoen Aug 16, 2021 •
edited

Choose a reason for hiding this comment

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

please find out how to reserve a parachain id as specified here https://substrate.dev/cumulus-workshop/#/en/3-parachains/2-register?id=registration-transaction. it has a link to "paraID being reserved" but it returns 404, so i'd suggest asking in Parachain Technical room on Matrix how to do it and then create a PR to fix the broken link in their docs.

it also mentions how to do it here in the README of the 'master' branch https://github.com/substrate-developer-hub/substrate-parachain-template/tree/40b858149b212e493da08d80e0fc5b06a6b0b72d#register-on-the-relay-with-sudo, did you try registering on a relay chain to test it like they mention?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ltfschoen Got some responses from community https://matrix.to/#/!WuksvCDImqYSxvNmua:matrix.parity.io/$14W9j8ksOWo3FekiHjEAu6yZoPzd1jSeZVC24ByapeI?via=matrix.parity.io&via=matrix.org&via=web3.foundation

https://matrix.to/#/!WuksvCDImqYSxvNmua:matrix.parity.io/$BCdTh0mP07KNasF-Njm6PSsnKPK1jBrbt9PwHqAEjWg?via=matrix.parity.io&via=matrix.org&via=web3.foundation

We can take any > 1999 & < max (if no one has already taken it).

Here is parachain guide https://substrate.dev/substrate-how-to-guides/docs/parachains/overview/

Here is guide to reserve paraId https://substrate.dev/substrate-how-to-guides/docs/parachains/c-registration/register-paraid

ltfschoen
ltfschoen reviewed Aug 11, 2021
View reviewed changes
Copy link
Collaborator

@ltfschoen ltfschoen left a comment

Choose a reason for hiding this comment

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

minor comments

ltfschoen
ltfschoen reviewed Aug 16, 2021
View reviewed changes
node/src/command.rs Outdated Show resolved Hide resolved
@sheenhx
Copy link

sheenhx commented Aug 26, 2021

any idea we can allow the users in Supernodes to also participate in governance ?

ltfschoen
ltfschoen reviewed Oct 25, 2021
View reviewed changes
node/src/chain_spec.rs Outdated Show resolved Hide resolved
ltfschoen
ltfschoen reviewed Oct 25, 2021
View reviewed changes
node/src/chain_spec.rs Outdated Show resolved Hide resolved
ayushmishra2005 and others added 2 commits October 25, 2021 18:53
@ayushmishra2005 @ltfschoen
Update node/src/chain_spec.rs
4b4a469 
Co-authored-by: Luke Schoen <ltfschoen@users.noreply.github.com>
@ayushmishra2005 @ltfschoen
Update node/src/chain_spec.rs
7d02084 
Co-authored-by: Luke Schoen <ltfschoen@users.noreply.github.com>
ltfschoen
ltfschoen reviewed Oct 25, 2021
View reviewed changes
node/src/chain_spec.rs Outdated Show resolved Hide resolved
@ayushmishra2005 @ltfschoen
Update node/src/chain_spec.rs
935d4d3 
Co-authored-by: Luke Schoen <ltfschoen@users.noreply.github.com>
ltfschoen
ltfschoen reviewed Oct 25, 2021
View reviewed changes
node/src/chain_spec.rs Outdated Show resolved Hide resolved
ltfschoen
ltfschoen reviewed Oct 25, 2021
View reviewed changes
node/src/chain_spec.rs Outdated Show resolved Hide resolved
ltfschoen
ltfschoen reviewed Oct 25, 2021
View reviewed changes
node/src/chain_spec.rs Outdated Show resolved Hide resolved
ltfschoen
ltfschoen reviewed Oct 25, 2021
View reviewed changes
node/src/chain_spec.rs Outdated Show resolved Hide resolved
ltfschoen
ltfschoen reviewed Oct 25, 2021
View reviewed changes
node/src/chain_spec.rs Outdated Show resolved Hide resolved
ltfschoen
ltfschoen reviewed Oct 25, 2021
View reviewed changes
node/src/chain_spec.rs Outdated Show resolved Hide resolved
ltfschoen
ltfschoen reviewed Oct 25, 2021
View reviewed changes
node/src/chain_spec.rs Outdated Show resolved Hide resolved
ltfschoen
ltfschoen requested changes Oct 25, 2021
View reviewed changes
Copy link
Collaborator

@ltfschoen ltfschoen left a comment •
edited

Choose a reason for hiding this comment

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

  • - in rococo_development_config function, change name from "Development" to "DataHighway Rococo Development Testnet", and id from "dev" to "datahighway-rococo-dev"

  • - in rococo_local_testnet_config function, change name from "Local Testnet" to "DataHighway Rococo Local Testnet", and id from "local_testnet" to "datahighway-rococo-local" (horizontal dash rather than an underscore)

  • - in chachacha_development_config function, change name from "Development" to "DataHighway ChaChaCha Development Testnet", and id from "dev" to "datahighway-chachacha-dev"

  • - in chachacha_local_testnet_config function, change name from "Development" to "DataHighway ChaChaCha Local Testnet", and id from "local_testnet" to "datahighway-chachacha-local" (horizontal dash rather than an underscore)

  • - in rococo_parachain_config function, change name from "DataHighway Spreehafen Parachain Testnet" to "DataHighway Spreehafen Rococo Parachain Testnet", and id from "datahighway_spreehafen" to "datahighway-spreehafen-rococo-parachain-testnet" (horizontal dash rather than an underscore)

  • - in chachacha_parachain_config function, change name from "DataHighway Spreehafen Parachain Testnet" to "DataHighway Spreehafen ChaChaCha Parachain Testnet", and id from "datahighway_spreehafen" to "datahighway-spreehafen-chachacha-parachain-testnet" (horizontal dash rather than an underscore)

  • - in westend_development_config function, change name from "Development" to "DataHighway Westend Development Testnet", and id from "dev" to "datahighway-westend-dev"

  • - in westend_local_testnet_config function, change name from "Local Testnet" to "DataHighway Westend Local Testnet", and id from "local_testnet" to "datahighway-westend-local" (horizontal dash rather than an underscore)

  • - in polkadot_development_config function, change name from "Development" to "DataHighway Polkadot Development Testnet", and id from "dev" to "datahighway-polkadot-dev"

  • - in polkadot_local_testnet_config function, change name from "Local Testnet" to "DataHighway Polkadot Local Testnet", and id from "local_testnet" to "datahighway-polkadot-local" (horizontal dash rather than an underscore)

  • - in westend_parachain_config function, change name from "DataHighway Baikal Parachain Testnet" to "DataHighway Baikal Westend Parachain Testnet", change id from "datahighway_baikal" to "datahighway-baikal-westend-parachain-testnet" (horizontal dash rather than an underscore)

  • - in westend_parachain_config function, i think we should use a different token other than DHX, since the token on Westend is supposed to be valueless.
    On some parachains they actually have a faucet for their parachain testnet and give users the opportunity to win NFTs if they have over a certain amount of their parachain testnet tokens

    • Update 26 Oct - Luke asked team internally about their thoughts on this
    • Update 27 Oct - Ayush - I took follow up on same thread and suggested token name based on testnet name
    • Update 27 Oct - Ayush - After getting confirmation on discord, we changed it to BKL token

  • - in westend_parachain_config function, the relay chain is not "chachacha", its "westend"

  • - in polkadot_parachain_config function, change name from "DataHighway Tanganika Parachain Testnet" to "DataHighway Tanganika Polkadot Parachain", and id from "datahighway_tanganika" to "datahighway-tanganika-polkadot-parachain" (horizontal dash rather than an underscore)

  • - in polkadot_parachain_config function, the relay chain is not "chachacha", its "polkadot"

  • - "in node/src/command.rs please check that all the CLI commands work"

    • Update 26 Oct:
    • - running the following:
     cargo build --release
 ./target/release/datahighway-collator --alice --collator --tmp --chain dev

    gives error:

     Error: Input("Relay chain argument error: Invalid input: `rococo-dev` only supported with `rococo-native` feature enabled.")

      - Update 1 Nov: Luke - Done based on feedback obtained by Ayush below
    • - running the following:
     cargo build --release
 ./target/release/datahighway-collator --alice --collator --tmp --chain rococo-local

    gives error:

     Error: Input("Relay chain argument error: Invalid input: `rococo-local` only supported with `rococo-native` feature enabled.")

    We can either use preconfigured files from here https://docs.substrate.io/tutorials/v3/cumulus/start-relay/#pre-configured-chain-spec-files or we can build using these steps https://docs.substrate.io/tutorials/v3/cumulus/start-relay/#generate-a-plain-chain-spec for rococo-dev and rococo-local

    I downloaded https://docs.substrate.io/assets/tutorials/cumulus/chain-specs/rococo-custom-2-raw.json and executed this command

     ./target/release/datahighway-collator --alice --collator --tmp -- --execution wasm --chain rococo-custom-2-raw.json

    Its working fine.

    • Update 29th Oct Ayush: @ltfschoen I didn't get answer from the community yet. However, I continued my investigation.

    For rococo-dev, rococo-local and rococo, we have to specify the spec path in chain argument. For rococo-dev and rococo-local, as I mentioned above We can either use preconfigured files from here https://docs.substrate.io/tutorials/v3/cumulus/start-relay/#pre-configured-chain-spec-files or we can build using these steps https://docs.substrate.io/tutorials/v3/cumulus/start-relay/#generate-a-plain-chain-spec for rococo-dev and rococo-local.

    I tried this on local and connected parachain. Parachain is able to produce blocks.

    For rococo, we can use https://raw.githubusercontent.com/paritytech/polkadot/v0.9.10/node/service/res/rococo.json. It is also working fine.

    • - running the following:
     cargo build --release
 ./target/release/datahighway-collator --alice --collator --tmp --chain chachacha-dev

    gives error:

     Error: Input("Relay chain argument error: Invalid input: Error opening spec file: No such file or directory (os error 2)")

      - Update 1 Nov: Luke - Done based on feedback obtained by Ayush below
    • - running the following:
     cargo build --release
 ./target/release/datahighway-collator --alice --collator --tmp --chain chachacha-local

    gives error:

     Error: Input("Relay chain argument error: Invalid input: Error opening spec file: No such file or directory (os error 2)")

      - Update 1 Nov: Luke - Done based on feedback obtained by Ayush below - if ChaChaCha community doesn't know we'll just use rococo-local instead of chachacha-local until they respond
    • - running the following:
     cargo build --release
 ./target/release/datahighway-collator --alice --collator --tmp --chain rococo

    successfully runs a node, but why does it mention Babe when we're using Aura instead? are empty Babe epoch changes necessary?

     Creating empty BABE epoch changes on what appears to be first startup.
    • - running the following:
     cargo build --release
 ./target/release/datahighway-collator --alice --collator --tmp --chain chachacha

    gives error:

     Error: Input("Relay chain argument error: Invalid input: Error opening spec file: No such file or directory (os error 2)")
    • Update 1st Nov Luke: closing on the assumption that we just "have to specify the spec path in chain as argument" as mentioned above by Ayush, "use preconfigured files" or "build ..." using the steps specified. Or use rococo otherwise
    • - running the following:
     cargo build --release
 ./target/release/datahighway-collator --alice --collator --tmp --chain westend-dev

    gives error:

     Error: Input("Relay chain argument error: Invalid input: `westend-dev` only supported with `westend-native` feature enabled.")
    • Update 1 Nov: Luke - Done based on feedback obtained by Ayush
    • - running the following:
     cargo build --release
 ./target/release/datahighway-collator --alice --collator --tmp --chain westend-local

    gives error:

     Error: Input("Relay chain argument error: Invalid input: `westend-local` only supported with `westend-native` feature enabled.")
    • Update 1 Nov: Luke - Done based on feedback obtained by Ayush
    • - running the following:
     cargo build --release
 ./target/release/datahighway-collator --alice --collator --tmp --chain westend

    successfully runs a node, but why does it mention Babe when we're using Aura instead? are empty Babe epoch changes necessary?

     Creating empty BABE epoch changes on what appears to be first startup.
    • - running the following:
     cargo build --release
 ./target/release/datahighway-collator --alice --collator --tmp --chain polkadot-dev

    successfully runs a node, but why does it mention Babe when we're using Aura instead? are empty Babe epoch changes necessary?

     Creating empty BABE epoch changes on what appears to be first startup.
    • - running the following:
     cargo build --release
 ./target/release/datahighway-collator --alice --collator --tmp --chain polkadot-local

    successfully runs a node, but why does it mention Babe when we're using Aura instead? are empty Babe epoch changes necessary?

     Creating empty BABE epoch changes on what appears to be first startup.
    • - running the following:
     cargo build --release
 ./target/release/datahighway-collator --alice --collator --tmp --chain polkadot

    successfully syncs with polkadot relay chain, but why does it mention Babe when we're using Aura instead? are empty Babe epoch changes necessary?

     Creating empty BABE epoch changes on what appears to be first startup.
  • Answer by Ayush: 26th Oct - @ltfschoen I have shared keys again. New keys have already been updated
  • - Added 26 Oct - datahighway_chachacha_parachain_config function hasn't been updated with the new keys in (i.e. authority keys, sudo key, endowed accounts) that you shared for rococo (assuming we're using the same keys for rococo and chachacha)
  • Answer by Ayush: 26th Oct - @ltfschoen I have shared keys again. New keys have already been updated
  • Update 1 Nov Luke: checked all latest keys that were added match those that we have secretly stored

ayushmishra2005 and others added 12 commits October 26, 2021 08:22
@ayushmishra2005 @ltfschoen
Update node/src/chain_spec.rs
02dcd23 
Co-authored-by: Luke Schoen <ltfschoen@users.noreply.github.com>
@ayushmishra2005 @ltfschoen
Update node/src/chain_spec.rs
c344f1b 
Co-authored-by: Luke Schoen <ltfschoen@users.noreply.github.com>
@ayushmishra2005 @ltfschoen
Update node/src/chain_spec.rs
58a9839 
Co-authored-by: Luke Schoen <ltfschoen@users.noreply.github.com>
@ayushmishra2005 @ltfschoen
Update node/src/chain_spec.rs
951d9f0 
Co-authored-by: Luke Schoen <ltfschoen@users.noreply.github.com>
@ayushmishra2005 @ltfschoen
Update node/src/chain_spec.rs
5c87acf 
Co-authored-by: Luke Schoen <ltfschoen@users.noreply.github.com>
@ayushmishra2005 @ltfschoen
Update node/src/chain_spec.rs
c421509 
Co-authored-by: Luke Schoen <ltfschoen@users.noreply.github.com>
@ayushmishra2005 @ltfschoen
Update node/src/chain_spec.rs
313ec94 
Co-authored-by: Luke Schoen <ltfschoen@users.noreply.github.com>
Addressed PR comments
446c120
Addressed PR comments
839a958
@ltfschoen
endow the aura authorities. mention what the endowed keys are
e6fb0e3
Endowed Aura accounts for rococo_parachain_config, chachacha_parachai…
9714b13 
…n_config functions
Used different token name in westend_parachain_config other than DHX
7c2be28
@ltfschoen
Copy link
Collaborator

ltfschoen commented Nov 1, 2021 •
edited

  • Rococo

@ayushmishra2005

  • please update the title to mention that you have made this compatible with other relay chains like Polkadot, Rococo, etc
  • please update this initial checklist Make compatible with Polkadot, Kusama and Rococo #217 (comment) once you have finished 3.2, 4 (add an additional step for testing on Polkadot), then we can merge per dot point 5 into 'ilya/parachain'

@ayushmishra2005 ayushmishra2005 changed the title Make compatible with kusama Make compatible with Polkadot, Kusama and Rococo Nov 1, 2021
@ayushmishra2005
Copy link
Contributor Author

  • Rococo

@ayushmishra2005

  • please update the title to mention that you have made this compatible with other relay chains like Polkadot, Rococo, etc
  • please update this initial checklist Make compatible with kusama #217 (comment) once you have finished 3.2, 4 (add an additional step for testing on Polkadot), then we can merge per dot point 5 into 'ilya/parachain'

@ltfschoen I have updated the checklist based on recent discussion with community.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ltfschoen ltfschoen ltfschoen approved these changes

@cgroeschel cgroeschel Awaiting requested review from cgroeschel

@festelo festelo Awaiting requested review from festelo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ayushmishra2005 @sheenhx @ltfschoen @cgroeschel