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

Update to not use --parachain-id flag #160

Closed
wants to merge 20 commits into from
Closed

Update to not use --parachain-id flag #160

wants to merge 20 commits into from

Conversation

jimmychu0807
Copy link

@jimmychu0807 jimmychu0807 commented Dec 13, 2021

Many update mixed in this PR...

  • Parachains now create chainSpec and rawChainSpec, and then the paraId are updated within their rawChainSpec.
  • Added -o output flag to specify the asset output folder, logs will be stored at <output>/logs.
  • Added -v verbose flag that if specify, will display the actual commands being run
  • Updated yarn to yarn3 - it is faster.

AWARE

With this PR, I am able to start a 3-node relay chain and a 2-node parachain network, and the parachain blocks got finalized. But I cannot get:

  • 3-node relay chain and two 1-node parachain networks to connect and have the parachains blocks got finalized. Need your help @shawntabrizi @apopiak
  • Cannot get the simpleParachain (using polkadot adder-collator) to connect to the relaychain. It is always syncing to the main adder-collator network in production.
  • I go thru the code in test, they are really moonbeam specific (refer to the constants.ts). I wonder if we still want to keep them in our repo.

If the code looks good, I will update the README, and also the cumulus tutorial in docs.substrate.io (this is what I originally intend to address: polkadot-developers/substrate-docs#634 😄 )


close #156
close #151
close #148
close #146

@jimmychu0807
Copy link
Author

@joelamouche since you are the main participate in contributing to this code base, want to hear from you if changes made in this PR is also compatible with use cases on your side.

@shawntabrizi I see there are conflicts and I can resolve them. But before putting in more effort into this PR, I would like to get an initial indication from you that you are okay with the approach & direction of the change. I will then resolve any remaining conflicts.

Also need your help on getting the case of launching one relay-chain and two 1-node parachains networks back to work.

@joelamouche
Copy link
Contributor

@jimmychu0807 Thanks I will look at it.
I copied your repo and ran yarn run para-test and I got some errors. Did you make sur the tests pass?

@HCastano
Copy link

@jimmychu0807 this PR should really be split into two or three PRs - at the very least the yarn3 update should be split out

@shawntabrizi
Copy link
Member

shawntabrizi commented Dec 16, 2021

100% agree with Hernando.

Added -o output flag to specify the asset output folder, logs will be stored at /logs.

Seems fine. Default behavior without the flag should stay the same.

Added -v verbose flag that if specify, will display the actual commands being run.

Also seems fine.

Updated yarn to yarn3 - it is faster.

I am not a fan of this change if it requires users to install something which is not the normal yarn.

I don't really see that the speed of yarn affects anyone's quality of life on this project, however installing new packages just for this application will def be annoying.

@jimmychu0807
Copy link
Author

jimmychu0807 commented Dec 21, 2021

I have merged with last change, and updated so yarn para-test run.

@joelamouche: But I realized that it stucks at this line and then the before part of describeParachain timed out (I changed that from 5 mins to 3 mins, btw). I look deeper into it. Eventually it calls providePolkadotApi and just return api connection from that port. Are you guys expecting there is a node running and accepting connection at the port?

Eventually, are para-test suppose to be runnable(and pass) in non-moonbeam specific hosts/environments?

@HCastano @shawntabrizi I have removed yarn3. Will be great if you could take a look at the scenario that having:

3-node relay chain and two 1-node parachain networks

They connect, but the parachains blocks DO NOT finalized. And I couldn't get that to work.

Thank you all!

src/runner.ts Outdated

for (const parachain of paras) {
const { resolvedId, chain } = parachain;
const bin = resolve(config_dir, parachain.bin);
const { isSimple, id, resolvedId, protocolId, chain = "local" } = parachain;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const { isSimple, id, resolvedId, protocolId, chain = "local" } = parachain;
const { isSimple, id, resolvedId, protocolId, chain = "local" } = parachain;

Where are you getting chain = "local" from?

This is breaking the application from running.

Are you testing polkadot-launch after the changes you made?

Copy link
Author

@jimmychu0807 jimmychu0807 Dec 30, 2021

Choose a reason for hiding this comment

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

If within the parachain section of the config file the chain value is not specified, then what should its default value be?

Copy link
Author

Choose a reason for hiding this comment

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

Also, I tried with one relay chain of 3-node and one parachain of 2-node with config file listed here and works.

But as mentioned above, I cannot make it to work with 3-node relay chain and two 1-node parachain networks.

What test config do you use when testing?

Copy link
Member

Choose a reason for hiding this comment

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

Right now I was trying with just the default config, generating a binary from cumulus and polkadot master.

Cumulus polkadot-collator node does not have a local chain. By default, I don't think it needs a chain input.

@jimmychu0807
Copy link
Author

@shawntabrizi updated to remove setting chain type to local when missing.

But using polkadot as relay-chain bin and polkadot-collator as parachain bin, this is the log produced:

The point is the parachain does have the PoV block candidate generated but is never added to the parachain blockchain.

Is there something wrong? If yes, how should I go about debugging it?

@mrq1911
Copy link

mrq1911 commented Jan 6, 2022

@jimmychu0807 i have tried your fork, but it transforms large numbers in chainspec like

        "balances": [
          [
            "5GrwvaEF5zXb26Fz9rcQpDWS57CtERHpNehXCPcNoHGKutQY",
            1,
            1000000000000000000000
          ],

to exponential notation

        "balances": [
          [
            "5GrwvaEF5zXb26Fz9rcQpDWS57CtERHpNehXCPcNoHGKutQY",
            1,
            1e+21
          ],

which then node refuses to parse when trying to use it

also I have noticed that relative path moved one level up when trying to execute it as dependency

you can replicate it by running this https://github.com/galacticcouncil/Basilisk-node/blob/polkadot-v0.9.13/scripts/wait-for-inclusion/package.json#L9

@nuke-web3
Copy link

Sorry this is out of any context, just found this https://github.com/paritytech/cumulus/pull/858/files

Might be useful to this PR? I hope so 🙏

@jimmychu0807
Copy link
Author

@lumir-mrkva thanks for trying my fork. Let me test out what you do next week.

@jimmychu0807
Copy link
Author

@lumir-mrkva

  1. I fixed the way binary filepaths are resolved, so they are resolve with respect to the config file, not where you run the parachain-launch script.

  2. I don't see your issue of converting a balance 1000000000000000000000 to 1e+21. Setting the balance in config file will directly send a transaction at era 0 to the chain of setting alice of that balance amount, not writing it to the chainSpec value. If the balance value is updated to a scientific notation, this is how the build-spec subcommand work and I would rather look into how the particular build-spec logic is written.

@shawntabrizi
When you have time this week or next, could you take a look at this PR if it is good to merge? Thanks.

@CertainLach
Copy link

If the balance value is updated to a scientific notation, this is how the build-spec subcommand work and I would rather look into how the particular build-spec logic is written.

It gets converted here: https://github.com/paritytech/polkadot-launch/pull/160/files#diff-2e46e281a9ce50fb008324ac073f40d11857c82caa1a495145416e31b3cb14a1R236-R247

Due to standard JSON not supporting large integers:

JSON.stringify(JSON.parse('{"balance":1000000000000000000000}'))
// {\"balance\":1e+21}

And then serde_json parser not accepting scientific notation as integers (because it is not lossless)
The solution for this may be using something like https://www.npmjs.com/package/json-bigint

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants