node icon indicating copy to clipboard operation
node copied to clipboard
verified publisher icon DataHighway-DHX

Make compatible with Polkadot, Kusama and Rococo

Open ayushmishra2005 opened this issue 4 years ago • 14 comments

resolves #216

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

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

  • [X] 1.1. - Upgrade to Polkadot 0.9.10

  • [ ] 1.2. - Upgrade to Polkadot 0.9.11

  • [ ] 1.3. - Upgrade to Polkadot 0.9.12

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

  • [X] 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

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

  • [X] 3.1.1 - requested rococo tokens

  • [X] 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'

ayushmishra2005 avatar Aug 09 '21 12:08 ayushmishra2005

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

sheenhx avatar Aug 26 '21 13:08 sheenhx

@ayushmishra2005 i went through all the comments, i think just focus on closing these remaining comments that were raised

  • [x] - https://github.com/DataHighway-DHX/node/pull/217#discussion_r720132916
  • [x] - https://github.com/DataHighway-DHX/node/pull/217#discussion_r720121974
  • [x] - https://github.com/DataHighway-DHX/node/pull/217#discussion_r720121974
  • [x] - https://github.com/DataHighway-DHX/node/pull/217#discussion_r720112631
  • [x] - https://github.com/DataHighway-DHX/node/pull/217#discussion_r720097818
  • [x] - https://github.com/DataHighway-DHX/node/pull/217#discussion_r720094659
  • [x] - https://github.com/DataHighway-DHX/node/pull/217#discussion_r720087217
  • [x] - https://github.com/DataHighway-DHX/node/pull/217#discussion_r720084250
  • [x] - https://github.com/DataHighway-DHX/node/pull/217#discussion_r720074493
  • [x] - https://github.com/DataHighway-DHX/node/pull/217#discussion_r720072156
  • [x] - https://github.com/DataHighway-DHX/node/pull/217#discussion_r720068330
  • [x] - https://github.com/DataHighway-DHX/node/pull/217#discussion_r720043744
  • [x] - https://github.com/DataHighway-DHX/node/pull/217#discussion_r720039009
  • [x] - https://github.com/DataHighway-DHX/node/pull/217#discussion_r721068039
  • [x] - https://github.com/DataHighway-DHX/node/pull/217#discussion_r720029007
  • [x] - https://github.com/DataHighway-DHX/node/pull/217#discussion_r720028830

i moved minor tasks that aren't critical to "make compatible with kusama" scope that haven't yet been done yet into #231 and https://github.com/DataHighway-DHX/node/issues/227 so this task gets closed faster

ltfschoen avatar Oct 05 '21 08:10 ltfschoen

@ayushmishra2005 i went through all the comments, i think just focus on closing these remaining comments that were raised

i moved minor tasks that aren't critical to "make compatible with kusama" scope that haven't yet been done yet into #231 and #227 so this task gets closed faster

Thanks @ltfschoen I will look into it

ayushmishra2005 avatar Oct 05 '21 10:10 ayushmishra2005

@ayushmishra2005 CI is failing, might need to change some of the .into() into .unchecked_into()

ltfschoen avatar Oct 11 '21 09:10 ltfschoen

the changes look good, but just a couple of issues still:

  • [x] in the development and local testnet config functions for Rococo and ChaChaCha, you're passing a vector containing the Aura keys for the initial authorities Alice and Bob correctly, however in the parachain config function for Rococo and ChaChaCha, you're passing in a vector containing the Stash and Controller keys for the initial authories argument, but spreehafen_testnet_genesis is expecting them to be Aura keys.

i.e. at the moment this https://github.com/DataHighway-DHX/node/blob/make_compatible_with_kusama/node/src/chain_spec.rs#L246 has the structure,

spreehafen_testnet_genesis(
    vec![
        hex!["<CONTROLLER_1_PUBLIC_KEY>"].unchecked_into(),
        hex!["<CONTROLLER_2_PUBLIC_KEY>"].unchecked_into(),
        hex!["<CONTROLLER_3_PUBLIC_KEY>"].unchecked_into(),
        hex!["<CONTROLLER_4_PUBLIC_KEY>"].unchecked_into(),
        hex!["<STASH_1_PUBLIC_KEY>"].unchecked_into(),
        hex!["<STASH_2_PUBLIC_KEY>"].unchecked_into(),
        hex!["<STASH_3_PUBLIC_KEY>"].unchecked_into(),
        hex!["<STASH_4_PUBLIC_KEY>"].unchecked_into(),
    ],
    ...

...

fn spreehafen_testnet_genesis(
    initial_authorities: Vec<AuraId>,
    ...

but i believe it should be in this format like in the standalone chain code https://github.com/DataHighway-DHX/node/blob/master/node/src/chain_spec.rs#L298, and comments help to know what the keys refer to

spreehafen_testnet_genesis(
    vec![
    	// authority #1
    	(
    		// aura
        	hex!["<AURA_1_PUBLIC_KEY>"].unchecked_into(),
        	// do we need to add any other keys for a parachain??
        ),
        // authority #2
        (
        	// aura
        	hex!["<AURA_2_PUBLIC_KEY>"].unchecked_into(),
        	// do we need to add any other keys for a parachain??
        ),
    ],

please check if we also need to add any of Stash, Controller, Grandpa, ImOnline, Authority Discovery in the keys of a parachain (even though they are not included in the parachain template) i.e. should the initial_authorities: Vec<AuraId>, part of the following code include other keys like we need for a standalone chain https://github.com/DataHighway-DHX/node/blob/master/node/src/chain_spec.rs#L1243 like initial_authorities: Vec<(AccountId, AccountId, GrandpaId, AuraId, ImOnlineId, AuthorityDiscoveryId)>,, or are some of those keys used by validators on relay chain and not on collator?

fn spreehafen_testnet_genesis(
    initial_authorities: Vec<AuraId>,

...

fn testnet_genesis(
    initial_authorities: Vec<AuraId>,

we would need to apply these changes to both functions rococo_parachain_config and chachacha_parachain_config

Note that in the Rococo keys file that you shared with me, it only has Stash and Controller keys generated for a root account and 4x authorities, along with Babe (which we're not using anymore as we're using Aura instead) and Grandpa keys, but there is no Aura key (which should be sr25519). Please fix the formatting add all the correct keys and in the correct order

  • [x] we still haven't added Sudo to the endowed accounts for the parachain configurations, we need this so the Sudo account is endowed with funds at genesis to cover transaction fees.

we've already added the root Sudo account to the endowed account list in the development rococo_development_config https://github.com/DataHighway-DHX/node/blob/make_compatible_with_kusama/node/src/chain_spec.rs#L95, and rococo_local_testnet_config https://github.com/DataHighway-DHX/node/blob/make_compatible_with_kusama/node/src/chain_spec.rs#L130, and chachacha_development_config https://github.com/DataHighway-DHX/node/blob/make_compatible_with_kusama/node/src/chain_spec.rs#L1732, and chachacha_local_testnet_config https://github.com/DataHighway-DHX/node/blob/make_compatible_with_kusama/node/src/chain_spec.rs#L208

but we haven't yet added the root Sudo account public key (i.e. 3c917f65753cd375582a6d7a1612c8f01df8805f5c8940a66e9bda3040f88f5d) to the vector of endowed accounts for rococo_parachain_config in the vector here https://github.com/DataHighway-DHX/node/blob/make_compatible_with_kusama/node/src/chain_spec.rs#L258 and in the chachacha_parachain_config vector here https://github.com/DataHighway-DHX/node/blob/make_compatible_with_kusama/node/src/chain_spec.rs#L309

i.e. we need to add the folllowing to the top of each of those lists

// Endow the root Sudo account to cover transaction fees
hex!["3c917f65753cd375582a6d7a1612c8f01df8805f5c8940a66e9bda3040f88f5d"].into(),

ltfschoen avatar Oct 12 '21 10:10 ltfschoen

initial_authorities: Vec<(AccountId, AccountId, GrandpaId, AuraId, ImOnlineId, AuthorityDiscoveryId)>,

@ltfschoen Could you please guide me how should I check whether we should use this format initial_authorities: Vec<(AccountId, AccountId, GrandpaId, AuraId, ImOnlineId, AuthorityDiscoveryId)>, or only Vec<AuraId>

ayushmishra2005 avatar Oct 13 '21 07:10 ayushmishra2005

initial_authorities: Vec<(AccountId, AccountId, GrandpaId, AuraId, ImOnlineId, AuthorityDiscoveryId)>,

@ltfschoen Could you please guide me how should I check whether we should use this format initial_authorities: Vec<(AccountId, AccountId, GrandpaId, AuraId, ImOnlineId, AuthorityDiscoveryId)>, or only Vec<AuraId>

i don't know the answer either, the parachain template shows only AuraId being used. i'd suggest having a look at the parachain code of some parachain's that are live to see what's in their chain_spec.rs file. perhaps creating a question in subport and asking the question in the parachain technical room on Element. if no luck there, raise the question at the next Substrate Builders call

ltfschoen avatar Oct 13 '21 08:10 ltfschoen

initial_authorities: Vec<(AccountId, AccountId, GrandpaId, AuraId, ImOnlineId, AuthorityDiscoveryId)>,

@ltfschoen Could you please guide me how should I check whether we should use this format initial_authorities: Vec<(AccountId, AccountId, GrandpaId, AuraId, ImOnlineId, AuthorityDiscoveryId)>, or only Vec<AuraId>

i don't know the answer either, the parachain template shows only AuraId being used. i'd suggest having a look at the parachain code of some parachain's that are live to see what's in their chain_spec.rs file. perhaps creating a question in subport and asking the question in the parachain technical room on Element. if no luck there, raise the question at the next Substrate Builders call

Asked here https://github.com/paritytech/subport/issues/250

ayushmishra2005 avatar Oct 13 '21 10:10 ayushmishra2005

ready to merge after:

  • a new set of keys have been generated and added for Aura (as the current keys have been compromised, secret seed and mnemonic phrase should not be shared in chat programs like Discord). a new root key and root mnemonic phrase is also required. please archive but do not delete the old keys. please generate each key using the option --network datahighway so the SS58 address starts with 4 instead of 5 (so it uses our DataHighway prefix of 33 i.e. pub const SS58Prefix: u16 = 33;, instead of Substrate's default)
  • receive response from https://github.com/paritytech/subport/issues/250 that confirms how we should proceed, and update this code based on response

ltfschoen avatar Oct 13 '21 16:10 ltfschoen

also, FYI, this might be a cleaner way of endowing the accounts than the way we've done it https://github.com/AcalaNetwork/Acala/blob/master/node/service/src/chain_spec/karura.rs#L101

ltfschoen avatar Oct 13 '21 17:10 ltfschoen

also, FYI, this might be a cleaner way of endowing the accounts than the way we've done it https://github.com/AcalaNetwork/Acala/blob/master/node/service/src/chain_spec/karura.rs#L101

This would be done as a part of #237

ayushmishra2005 avatar Oct 16 '21 06:10 ayushmishra2005

ready to merge after:

  • a new set of keys have been generated and added for Aura (as the current keys have been compromised, secret seed and mnemonic phrase should not be shared in chat programs like Discord). a new root key and root mnemonic phrase is also required. please archive but do not delete the old keys. please generate each key using the option --network datahighway so the SS58 address starts with 4 instead of 5 (so it uses our DataHighway prefix of 33 i.e. pub const SS58Prefix: u16 = 33;, instead of Substrate's default)

@ltfschoen This is done.

Community will get back on this after sub0 Event.

ayushmishra2005 avatar Oct 16 '21 06:10 ayushmishra2005

  • 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 https://github.com/DataHighway-DHX/node/pull/217#issue-706470842 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 avatar Nov 01 '21 11:11 ltfschoen

  • 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.

ayushmishra2005 avatar Nov 01 '21 11:11 ayushmishra2005