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

Gaiad manager #928

Merged
merged 33 commits into from
May 21, 2021
Merged

Gaiad manager #928

merged 33 commits into from
May 21, 2021

Conversation

greg-szabo
Copy link
Member

@greg-szabo greg-szabo commented May 11, 2021

Closes: #902

Description

This PR introduces the gm command described in issue #902. All functionality of the issue is implemented with some extras described in the documentation.

Start with the README!


For contributor use:

  • Updated the Unreleased section of CHANGELOG.md with the issue.
  • If applicable: Unit tests written, added test to CI.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments.
  • Re-reviewed Files changed in the Github PR explorer.

strategy = 'naive'
log_level = 'info'

[[chains]]
id='node1'
rpc_addr='http://localhost:27010'
grpc_addr='https://localhost:27012'
websocket_addr='ws://localhost:27010/websocket'
rpc_timeout='1s'
account_prefix='cosmos'
key_name='wallet'
store_prefix='ibc'
fee_denom='stake'

[chains.trust_threshold]
numerator = '1'
denominator = '3'

[[chains]]
id='node3'
rpc_addr='http://localhost:27040'
grpc_addr='https://localhost:27042'
websocket_addr='ws://localhost:27040/websocket'
rpc_timeout='1s'
account_prefix='cosmos'
key_name='wallet'
store_prefix='ibc'
fee_denom='stake'

[chains.trust_threshold]
numerator = '1'
denominator = '3' command added
@colin-axner
Copy link
Contributor

Ran into:

$ ./gm install
./lib-gm: line 174: GLOBAL_GAIAD_BINARY: unbound variable

Rurunning that fixed the issue

Now I'm running into:

./gm keys
/usr/local/bin/stoml: line 1: syntax error near unexpected token `<'
/usr/local/bin/stoml: line 1: `<html><body>You are being <a href="https://github-releases.githubusercontent.com/170707906/4d0ddd80-aeb5-11eb-87d6-96d19dbbb9ea?X-Amz-Algorithm=AWS4-HMAC-SHA256&amp;X-Amz-Credential=AKIAIWNJYAX4CSVEH53A%2F20210514%2Fus-east-1%2Fs3%2Faws4_request&amp;X-Amz-Date=20210514T094928Z&amp;X-Amz-Expires=300&amp;X-Amz-Signature=ab89db3b452a079d5bd10e65a41d28a4f683113e960cb03d3df4b5180edbd12a&amp;X-Amz-SignedHeaders=host&amp;actor_id=0&amp;key_id=0&amp;repo_id=170707906&amp;response-content-disposition=attachment%3B%20filename%3Dstoml_linux_amd64&amp;response-content-type=application%2Foctet-stream">redirected</a>.</body></html>'
/usr/local/bin/stoml: line 1: syntax error near unexpected token `<'
/usr/local/bin/stoml: line 1: `<html><body>You are being <a href="https://github-releases.githubusercontent.com/170707906/4d0ddd80-aeb5-11eb-87d6-96d19dbbb9ea?X-Amz-Algorithm=AWS4-HMAC-SHA256&amp;X-Amz-Credential=AKIAIWNJYAX4CSVEH53A%2F20210514%2Fus-east-1%2Fs3%2Faws4_request&amp;X-Amz-Date=20210514T094928Z&amp;X-Amz-Expires=300&amp;X-Amz-Signature=ab89db3b452a079d5bd10e65a41d28a4f683113e960cb03d3df4b5180edbd12a&amp;X-Amz-SignedHeaders=host&amp;actor_id=0&amp;key_id=0&amp;repo_id=170707906&amp;response-content-disposition=attachment%3B%20filename%3Dstoml_linux_amd64&amp;response-content-type=application%2Foctet-stream">redirected</a>.</body></html>'

@greg-szabo
Copy link
Member Author

greg-szabo commented May 14, 2021

Hi @colin-axner ! Thank you for finding these bugs and shame on me for not testing them properly. I've updated the documentation with the correct commands to download stoml and sconfig. I've also fixed the error message.

For your convenience, here's the updated documentation part to install the dependencies:


On MacOS:

curl -Lo /usr/local/bin/sconfig https://github.com/freshautomations/sconfig/releases/download/v0.1.0/sconfig_darwin_amd64
curl -Lo /usr/local/bin/stoml https://github.com/freshautomations/stoml/releases/download/v0.6.1/stoml_darwin_amd64
chmod 755 /usr/local/bin/sconfig
chmod 755 /usr/local/bin/stoml

On Linux:

curl -Lo /usr/local/bin/sconfig https://github.com/freshautomations/sconfig/releases/download/v0.1.0/sconfig_linux_amd64
curl -Lo /usr/local/bin/stoml https://github.com/freshautomations/stoml/releases/download/v0.6.1/stoml_linux_amd64
chmod 755 /usr/local/bin/sconfig
chmod 755 /usr/local/bin/stoml

(Note the -L in the curl command and the chmods.) After running this, you should be able to run the command, but just in case, re-run the installer too.

If you don't have gaiad in your GOPATH, you'll have to update the path in the config file to your gaiad at $HOME/.gm/gm.toml.

@colin-axner
Copy link
Contributor

colin-axner commented May 14, 2021

Thanks @greg-szabo for the fix! I happened to reinstall sconfig and stoml via go before seeing that comment which worked great.

I attempted to startup a network using the new gaia version:

panic: genesis supply is incorrect, expected 10000network1coin,10000000stake, got 10000network1coin,1000000000network1v0token,510000000stake

As a side note, gm start did not indicate that the start failed.

Installing gaiad v4.2.1 results in:

panic: unknown field "power_reduction" in types.Params

which tells me the config files didn't get regenerated or overwritten (which makes sense). Maybe a gm unsafe-reset-all could be added to remove all the configs?

Removing the configs and using an gaia v4.2.1 works as expected

The gaia v4.2.1 never validated the supply (from my knowledge). There's a gaiad validate-genesis command, it might be useful to run this during gm start

Here is the valid supply section for genesis:

       {   
          "denom": "network1coin",
          "amount": "10000"
        },  
        {   
          "denom": "network1v0token",
          "amount": "1000000000"
        },  
        {   
          "denom": "stake",
          "amount": "510000000"
        }   

|==============|====================|
| RPC (26657) | ports_start_at + 0 |
| App (1317) | ports_start_at + 1 |
| GRPC (9090 | ports_start_at + 2 |
Copy link
Contributor

Choose a reason for hiding this comment

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

The new gaia version includes a GRPC-Web which defaults to 9091

There's an enable so I can manually set that to false for now

@greg-szabo
Copy link
Member Author

Cool, thanks for the feedback!
unsafe_reset_all makes sense.
The new GRPC-Web raises an interesting issue. The code was written with the config of 4.2.1 in mind, so this kind of new features will need some kind of compatibility check: which version of gaiad/regen/iris/etc you run?

I guess for now, I'll blindly replace that variable if it exists. But I think we'll have to keep the configuration management scope small so the developers can maintain their own config as well as we keep this compatible with the most apps.

@colin-axner
Copy link
Contributor

colin-axner commented May 14, 2021

I have managed to get the new gaia binary running after manually fixing the genesis. The only other step that was a little cumbersome is setting up keys on hermes. It'd be nice if there was an easy way to load the keys from gm into hermes. The work around isn't that bad, you just need to copy/paste the mnemonic

Other than that, gm works great! Clean UX, it will definitely be useful for the set of testing I'll be doing over the next week. There's a fix needed on hermes before I can test the new binary further, so will revisit this after that is fixed

I appreciate the separation from hermes as I can reuse this for testing the ts-relayer

@ancazamfir
Copy link
Collaborator

Here are my gm.toml (note the names ibc-0, fullnode-ibc-0, etc). Also added in the gist the generated hermes config versus what I expected it to look like
https://gist.github.com/ancazamfir/2e35dfa72bcca418f82cdcb1bcac80c4

The generated hermes config picks the fullnode name instead of the network.

Also I get:

$ gm start
Creating ibc-0 config...
WARNING: could not create config for ibc-0: "panic: invalid denom: ibc-0v0token

goroutine 1 [running]:
github.com/cosmos/cosmos-sdk/types.NewCoin(0xc00115cde0, 0xc, 0xc001043b80, 0x1, 0x1, 0xc00115cde0)
	github.com/cosmos/cosmos-sdk@v0.42.4/types/coin.go:23 +0x8a
github.com/cosmos/gaia/v4/cmd/gaiad/cmd.InitTestnet(0x0, 0x0, 0x0, 0x0, 0x0, 0x7ffeefbff4fd, 0x5, 0x5bd6620, 0xc000e92790, 0x5bd9c20, ...)
	github.com/cosmos/gaia/v4/cmd/gaiad/cmd/testnet.go:201 +0xf5f
github.com/cosmos/gaia/v4/cmd/gaiad/cmd.testnetCmd.func1(0xc000ecd180, 0xc0003ae600, 0x0, 0xc, 0x0, 0x0)
	github.com/cosmos/gaia/v4/cmd/gaiad/cmd/testnet.go:76 +0x4cb
github.com/spf13/cobra.(*Command).execute(0xc000ecd180, 0xc0003ae540, 0xc, 0xc, 0xc000ecd180, 0xc0003ae540)
	github.com/spf13/cobra@v1.1.3/command.go:852 +0x47c
github.com/spf13/cobra.(*Command).ExecuteC(0xc0003d9b80, 0x0, 0x0, 0xc000e983c0)
	github.com/spf13/cobra@v1.1.3/command.go:960 +0x375
github.com/spf13/cobra.(*Command).Execute(...)
	github.com/spf13/cobra@v1.1.3/command.go:897
github.com/spf13/cobra.(*Command).ExecuteContext(...)
	github.com/spf13/cobra@v1.1.3/command.go:890
github.com/cosmos/cosmos-sdk/server/cmd.Execute(0xc0003d9b80, 0xc000e983c0, 0x12, 0x5be8be0, 0xc000e92790)
	github.com/cosmos/cosmos-sdk@v0.42.4/server/cmd/execute.go:36 +0x265
main.main()
	github.com/cosmos/gaia/v4/cmd/gaiad/main.go:16 +0x45", skipping...
Creating ibc-1 config...
WARNING: could not create config for ibc-1: "panic: invalid denom: ibc-1v0token

@adizere
Copy link
Member

adizere commented May 18, 2021

Bit'o feedback:

  • add_to_hermes what is the default value? I think we can make it true so that by default we always add to Hermes and we could skip defining this variable. (I can imagine we'll almost always want to do that, and it would be harmless to do so even when the user is testing with say ts-relayer).
  • bash auto-completion is amazing, thanks for that!
  • gm start gives me the following error:
gm start
WARNING: invalid genesis.json for network1. Please check the log and fix manually. Skipping...
WARNING: invalid genesis.json for network2. Please check the log and fix manually. Skipping...
WARNING: invalid genesis.json for network3. Please check the log and fix manually. Skipping...
WARNING: invalid genesis.json for network4. Please check the log and fix manually. Skipping...
WARNING: invalid genesis.json for network5. Please check the log and fix manually. Skipping...
WARNING: invalid genesis.json for node1. Please check the log and fix manually. Skipping...
WARNING: invalid genesis.json for node2. Please check the log and fix manually. Skipping...
WARNING: invalid genesis.json for node3. Please check the log and fix manually. Skipping...
WARNING: invalid genesis.json for node4. Please check the log and fix manually. Skipping...
WARNING: invalid genesis.json for node5. Please check the log and fix manually. Skipping...

Looking at the logs I see:

Error: error validating genesis file /Users/adi/.gm/node1/config/genesis.json: failed to unmarshal staking genesis state: unknown field "power_reduction" in types.Params
Usage:
gaiad validate-genesis [file] [flags]

Consistent with Colin's comment above.
I am using the default gm.toml file from the readme, with gaiad v4.2.1. I tried gm unsafe-reset followed by gm start but got the same outcome.

LE: Switching to v4.2.0 got the same error.

@greg-szabo
Copy link
Member Author

Hi @adizere , thanks for the feedback!
Is it possible that the configuration was created with a different version of gaiad than what you are trying to run with now?
If I recall, the power_reduction field was introduced in some newer-than-4.2.1 version of gaiad, so if you revert back to 4.2.1 after you tried to run it with a newer version, you will end up trying to use the new configuration with an older binary.

I was thinking of adding a gm rm command that would completely remove the configuration for the specific node but until that's done, try removing the folder of the node manually and starting it again. (Which will recreate teh config.)

add_to_hermes is set to false by default because the configuration has to know which node you want to connect to when there are multiple nodes for the same network.

For example, the default configuration is a one-validator, one-full-node network. If add_to_hermes is set to true, hermes will have configuration for both nodes but they are on the same network. As far as I know, hermes should have only one connection to a specific network, so add_to_hermes points out which node will it be.

@adizere
Copy link
Member

adizere commented May 18, 2021

Hi @adizere , thanks for the feedback!
Is it possible that the configuration was created with a different version of gaiad than what you are trying to run with now?
If I recall, the power_reduction field was introduced in some newer-than-4.2.1 version of gaiad, so if you revert back to 4.2.1 after you tried to run it with a newer version, you will end up trying to use the new configuration with an older binary.

I was thinking of adding a gm rm command that would completely remove the configuration for the specific node but until that's done, try removing the folder of the node manually and starting it again. (Which will recreate teh config.)

Indeed, I had a newer version of gaiad originally. Was assuming that unsafe-reset was actually going to do that cleanup.
Removing the folders worked! Still testing...

add_to_hermes is set to false by default because the configuration has to know which node you want to connect to when there are multiple nodes for the same network.

For example, the default configuration is a one-validator, one-full-node network. If add_to_hermes is set to true, hermes will have configuration for both nodes but they are on the same network. As far as I know, hermes should have only one connection to a specific network, so add_to_hermes points out which node will it be.

That makes sense, and it will probably be a very common use-case, thanks for the insight!

Copy link
Member

@adizere adizere left a comment

Choose a reason for hiding this comment

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

Finished the basic steps, I like it!

The biggest improvement I can see is allowing this to work with the end to end tests. For instance, I'd like to create multiple networks and subject two of them to the end to end tests.
This currently breaks b/c e2e.py assumes that network names are ibc-0 and ibc-1, yet gm breaks upon creating the configs:

$ gm start
Creating ibc-0 config...
WARNING: could not create config for ibc-0: "panic: invalid denom: ibc-0v0token

goroutine 1 [running]:
github.com/cosmos/cosmos-sdk/types.NewCoin(0xc001276090, 0xc, 0xc001162f20, 0x1, 0x1, 0xc001276090)
[omitted]

For some reason, gm creates the following directories (note the name ibc-0v0).

$ ll ~/.gm/
Permissions Size User Date Modified Name
drwxr-xr-x     - adi  18 May 16:14  bin
.rw-r--r--   808 adi  18 May 19:09  gm.toml
drwx------     - adi  18 May 19:15  ibc-0
drwxr-xr-x     - adi  18 May 19:15  ibc-0v0
drwxr-xr-x     - adi  18 May 19:15  ibc-1v0
drwxr-xr-x     - adi  18 May 19:15  network3
drwxr-xr-x     - adi  18 May 19:15  network4
drwxr-xr-x     - adi  18 May 19:15  network5
drwxr-xr-x     - adi  18 May 19:15  node1

The config I was using is here

One other improvement I see would be to add some testing recipes, but that's out of the scope of this PR I guess.

scripts/gm/README.md Outdated Show resolved Hide resolved
Co-authored-by: Adi Seredinschi <adi@informal.systems>
@greg-szabo
Copy link
Member Author

Dang, I thought I fixed the denom issue! I'll check it again.

@adizere adizere self-requested a review May 19, 2021 08:16
@colin-axner
Copy link
Contributor

* We can introduce a `wallet_mnemonic` parameter in the config and the code will reuse that to create the wallet addresses. Interesting problem, I never thought that this would be required. (I use different addresses everywhere I can.)

Do you think we could add an hd_path as well? This would allow easier integration into the ts-relayer instead of requiring all keys to use the standard SDK fundraiser path. There's a little discussion on this on the ts-relayer issue linked above. We can set the default to be what it is now which I believe is the fundraiser path

@colin-axner
Copy link
Contributor

colin-axner commented May 19, 2021

Another feature request 🙂

Reverse the log command so the latest logs show first (although this can be unintuitive). I just want to be able to quickly see the latest logs

@colin-axner
Copy link
Contributor

Dang, I thought I fixed the denom issue! I'll check it again.

I think this should be fixed on gaia as well. I don't think it should generate tokens based on the directory name. Just minting "stake" is good.

The biggest improvement I can see is allowing this to work with the end to end tests.

Very much agree! I'd love to be able to put in two different gaia binaries, run gm start, and then run a set of end to end tests. Would be a great start to simulation testing

Great work @greg-szabo! GM has worked wonderfully in some initial testing I've been doing between gaia versions

@greg-szabo
Copy link
Member Author

greg-szabo commented May 20, 2021

Added features:

  • Added rm command to remove configuration
  • Rename unsafe-reset to reset and clarified that it resets the database only
  • Added -r to log command and the full log is now scrolled down to the bottom by default
  • Added wallet_mnemonic parameter to config
  • Added wallet_hdpath parameter to config
  • the denom issue now has a workaround (introduces a dependency on the tr command)
  • GRPC_WEB port support for newer gaiads
  • Temporary workaround for the total_supply iusse (I removed the total supply from the genesis, and because of another bug, that's valid. I hope both issue gets fixed in one go.)

@colin-axner @adizere All mentioned points were fixed, I think. If you could give it another spin, I'd appreciate it.

Copy link
Member

@adizere adizere left a comment

Choose a reason for hiding this comment

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

All mentioned points were fixed, I think. If you could give it another spin, I'd appreciate it.

Many thanks Greg, this is great! The remarks I left are optional, we can add features later, as we go along!

scripts/gm/bin/gm Outdated Show resolved Hide resolved
scripts/gm/bin/lib-gm Outdated Show resolved Hide resolved
greg-szabo and others added 2 commits May 21, 2021 11:29
Co-authored-by: Adi Seredinschi <adi@informal.systems>
@greg-szabo greg-szabo merged commit 1d0935f into master May 21, 2021
@greg-szabo greg-szabo deleted the greg/local-gaiad-manager branch May 21, 2021 15:59
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
GM tool v0.1.0 release

Co-authored-by: Adi Seredinschi <adi@informal.systems>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Developer tooling for easier chain experience
5 participants