search

paritytech / polkadot-launch

Simple CLI tool to launch a local Polkadot test network
MIT License
459 stars 93 forks source link

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

Closed jimmychu0807 closed 2 years ago

jimmychu0807 commented 2 years ago

Many update mixed in this PR...

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:

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: https://github.com/substrate-developer-hub/substrate-docs/issues/634 😄 )

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

jimmychu0807 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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

shawntabrizi commented 2 years ago

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 commented 2 years ago

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!

jimmychu0807 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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 :pray:

jimmychu0807 commented 2 years ago

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

jimmychu0807 commented 2 years ago

@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 commented 2 years ago

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