harmony-one / harmony

The core protocol of harmony
https://harmony.one
GNU Lesser General Public License v3.0
1.46k stars 287 forks source link

remove the node.sh wrapper #3110

Closed LeoHChen closed 3 years ago

LeoHChen commented 4 years ago

Summary

node.sh wrapper was originally introduced to wrap around harmony binary because we don't have a static binary build when we launched the network. However, since we have added statically linked binary and enabled it by default, it is time to remove the wrapper.

This is the meta ticket to keep track of all related tasks about this big project.

Current Design

node.sh is a wrapper of harmony binary and do a lot of additional work like,

Problems

node.sh becomes a monster wrapper and it is hard for new validators to use it. Also, it becomes error prune to use.

Proposal

As suggested in this ticket, https://github.com/harmony-one/harmony/issues/3108

And I agree with the points made in the ticket.

chainum commented 4 years ago

From the conversation on Discord / here's how I personally see we can do this:

  1. Start off by rewriting the CLI args parsing to use Cobra
  2. Change node.sh to interact with the new Cobra binary - we need to see that it'll continue to run properly for normal starts & stops + also auto-updates
  3. Port the auto-updating section of node.sh to a separate script (which would be something optional for node runners to use)
  4. Phase out auto-updating from node.sh - run multiple tests to see that everything works as expected
  5. After multiple iterations and testing finally drop node.sh altogether

There's so much stuff tied into using node.sh since it has been around for so long (e.g. deployments etc) so we 1000% have to take a gradual approach for this.

I don't agree on the idea of introducing a json config file - that adds another dependency and potential issue for validators (they might end up messing up the formatting etc). Smart refactoring of the CLI args + binary should be enough.

JackyWYX commented 4 years ago

From the conversation on Discord / here's how I personally see we can do this:

  1. Start off by rewriting the CLI args parsing to use Cobra
  2. Change node.sh to interact with the new Cobra binary - we need to see that it'll continue to run properly for normal starts & stops + also auto-updates
  3. Port the auto-updating section of node.sh to a separate script (which would be something optional for node runners to use)
  4. Phase out auto-updating from node.sh - run multiple tests to see that everything works as expected
  5. After multiple iterations and testing finally drop node.sh altogether

There's so much stuff tied into using node.sh since it has been around for so long (e.g. deployments etc) so we 1000% have to take a gradual approach for this.

I don't agree on the idea of introducing a json config file - that adds another dependency and potential issue for validators (they might end up messing up the formatting etc). Smart refactoring of the CLI args + binary should be enough.

Hey Seb, thanks for you suggestion. Also I cannot agree more on not using json file. Using a json file for testnet like OSTN is acceptable, but for external users, we should better keep interface as clean as we can. I will get to the research phase of this task once I have done my work by hand. And any suggestions will be welcomed at that time :)

lkkkk111 commented 4 years ago

https://forum.nkn.org/t/config-json-complete-reference-for-v1-0-en/766 https://github.com/nknorg/nkn/blob/master/config.mainnet.json

I personally want to be able to do cleanly this, specify:

This can be insane with only arguments to the binary.

Basically I have only one requirement, for it to be clean and easily transferable.

Actually TOML could be even better than JSON. https://en.wikipedia.org/wiki/TOML Much more readable and configuration is its main purpose.

What other projects use?

Avalanche Project (AVA) uses CLI parameters, but hey, they're still at the phase where they don't even have a CLI client and they send all RPCs strictly through curl via json-rpc. It's early.

Bitcoin uses .config file, Ethereum's geth (go-ethereum) implementation uses .toml file (arguably the most readable) Zcash uses .config file, NKN uses .json file.

Even though I proposed JSON, I'm against JSON for reasons of doing mistakes when editing them by validators and running into issues as @SebastianJ pointed out. After doing initial research, toml seems to be as the way to go. (Assuming) you want to go with the configuration file like any other sensible blockchain.

@JackyWYX @SebastianJ @LeoHChen

chainum commented 4 years ago

@lkkkk111 those are feature additions - should be made into a separate ticket and prioritized after the current node.sh dependency removal.

And yeah, if we're going to add a gazillion options, def better to use YAML/TOML instead of JSON.

IMHO, best to not add extra complexities at this point in time and just focus on moving away from node.sh with the currently available options - and when that has been tested / vetted to work properly - then add all the low priority extra/nice things to have (custom seed lists etc).

lkkkk111 commented 4 years ago

Ok, for the time being, at least specifying seed nodes (gateway nodes to enter the network), log level and ports, is already long enough already as a CLI argument and deserves to be in a configuration file (I vote for toml).

It's better to do it properly once and do it right, instead of creating another tech debt. Removal of the node.sh wrapper is tightly associated with the configuration.

gaia commented 4 years ago

toml is what is used in the cosmos SDK and Geth.

Use Viper to read the config from the file and set them

nyetwurk commented 4 years ago

I'd vote for yaml myself, but imo it should ONLY contain "differences" from the "default" (or have a separate default yaml file that end users don't have to change) so endusers do not constantly patch an upstream config if it differs from the default.

nyetwurk commented 4 years ago

That is to say, out of the box, a user shouldn't have to provide a config. It should only be required if they want things that are different from the defaults, and whatever they put in the config is not the whole config, just those differences.

nyetwurk commented 4 years ago

Further down the line, I think both docker and .deb images should be supplied. As above, the user should be able to override defaults with specific changes. In the case of docker, by env. For deb, the "user" config file above, or /etc/default/harmony if the deb uses env vars etc.

lkkkk111 commented 4 years ago

@nyetwurk Seed nodes (gateway nodes to join the network) should not be hardcoded in the node binary, where else would you put them if not into configuration file? I think that's the minimum you need configuration file for + genesis block hash if syncing freshly to make sure you start syncing the right chain and not testnet and maybe shard #, especially for explorer/watch nodes?

Agree that the configuration should include only the changes to the default values.

nyetwurk commented 4 years ago

@lkkkk111 agreed.

Incidentally, using a package manager would solve the static/dynamic library problem as well, since the package manager would install libs to /usr/lib or /usr/share/lib where they belong, and none of that LD_LIBRARY_PATH crap.

rlan35 commented 3 years ago

Fixed already.