search

ignite / cli

Ignite is a CLI tool and hub designed for constructing Proof of Stake Blockchains rooted in Cosmos-SDK
https://ignite.com
Other
1.26k stars 549 forks source link

Logging from an application causes Ignite to crash #2796

Open fadeev opened 2 years ago

fadeev commented 2 years ago

From Discord:

Brennanjl — Yesterday at 18:50 Hi all, I'm getting this error when trying to run my app: json: cannot unmarshal string into Go value of type chaincmdrunner.Account

I have tried uninstalling and reinstalling, as well as cleared the cache in my HOME/.(chain_name) directory and HOME/.ignite directory. This only recently started happening

Brennanjl — Yesterday at 20:08 Hi all, I seem to have found where the issue is coming from, but don't have a solution. When creating the accounts (on chain reset), Ignite has a buffer which the accounts are written to. This buffer then is unmarshalled into a chaincmdrunner.Account struct. The issue is that, before the unmarshalled account data is added to the buffer, an error message gets added to the buffer.

For me, the error message is an error message from my own Cosmos CLI tool. For example, if your chain was called nameservice, and your CLI command is called namerserviced, it appends an error saying namerserviced shutting down. This gets appended to the buffer, which it then tries to unmarshall into the chaincmdrunner.Account struct. The function where it adds this error can be found here: https://github.com/ignite/cli/blob/f17cab11d7af73ab170dd1259a8bd06dabc42c4f/ignite/pkg/chaincmd/runner/account.go#L63 Brennanjl — Yesterday at 20:52 Apologies for the spam, but I have fully fixed the issue (locally at least). The nameserviced shutting down statement was being logged by our own custom change to the main.go file in our cosmos app. It seems that anything that we log from our own Cosmos app can mess up what Ignite is reading in (in our case, it was a shutdown message).

The solution to avoid other developers reaching this error (or other hellish errors like it) would be to either not log to stdout (not sure what an alternative would be), make it clear to developers that they should under no circumstances log to stdout (probably not ideal), or to use a delimiter / custom prefix for the logs that are necessary for ignite to read in so that they can be separated from other logs created by the application

tbruyelle commented 2 years ago

When accounts are created during ignite chain serve, this uses the chain binary, and its output is serialized into chainrunner.Account. As you said, any change to that output will lead to the mentioned error, because the output is no longer JSON valid.

As an alternative, we could use the cosmosclient library to create the accounts, but since cosmos 0.46 introduced a migration in the keyring [0], this could lead to a compatibility issue. Indeed, this migration changes the keyring encoding from amino to protobuf. As soon as you read a keyring with 0.46, it is automatically migrated to protobuf encoding. Once migrated it's no longer possible to read it with a client compiled with a version <0.46.

So for instance, let's say your chain uses cosmos-sdk <0.45, and you use a Ignite CLI compiled with cosmos-sdk >=0.46 to read your keyring, then your chain app can no longer load its keyring because the encoding has changed.

To mitigate that, we could ensure the ignite binary and the chain use the same cosmos-sdk version, but that's something we need to discuss.

[0] https://github.com/cosmos/cosmos-sdk/pull/9695