cli should throw error if --chain-id not provided

[ebuchman]

Txs require --chain-id to be provided so it can be signed.

Note the chainid is not included in the tx itself, but it is part of the bytes that get signed to prevent cross-chain replay attacks.

Unfortunately, it means the error message just comes up as "signature verification failed" and we can't really detect why.

Thus, if the --chain-id is not provided, we should throw an error and tell the user it must be provided.

[cwgoes]

Can we load the chain ID automatically from the genesis file, if present?

[cwgoes]

Will the light client keep a genesis file with chain ID? democli init isn't implemented, not sure if that's intended to write genesis information or not.

[ebuchman]

Note we have to implement the basecli init functionality, which should set a chain id and other stuff like the node id and the initial validator set (to be socially authenticated).

See #822

[Onefox]

This problem is there again with gaiacli tx sign. No error is giving and a signature is produces that can not be broadcasted.

[cwgoes]

This problem is there again with gaiacli tx sign. No error is giving and a signature is produces that can not be broadcasted.

Yes, you're right, looks like a regression here.

cc @alessio Thoughts?

[alexanderbez]

There is a function we can already use to get the default chain ID. If the user does not provide one, it seems like a reasonable UX to fall back on the default. Alternatively, we could simply require the chain ID.

[cwgoes]

There is a function we can already use to get the default chain ID. If the user does not provide one, it seems like a reasonable UX to fall back on the default. Alternatively, we could simply require the chain ID.

I think this is a bit dangerous - chain ID is part of the signature; if you sign over a message with a chain ID you didn't intend it could have deleterious consequences (e.g. it might be a valid tx on a different blockchain). Better to require chain ID, if it isn't specified in the CLI config.

[alexanderbez]

Indeed. Seems like we have an actionable item here to require the chain ID flag.

[alessio]

When --chain-id is not specified on the command line, it shoud be automatically set:

func NewTxBuilderFromCLI() TxBuilder {
	// if chain ID is not specified manually, read default chain ID
	chainID := viper.GetString(client.FlagChainID)
	if chainID == "" {
		defaultChainID, err := sdk.DefaultChainID()
		if err != nil {
			chainID = defaultChainID
		}
	}

[snip]

Well, sdk.DefaultChainID() returns "":

// DefaultChainID returns the chain ID from the genesis file if present. An
// error is returned if the file cannot be read or parsed.
//
// TODO: This should be removed and the chainID should always be provided by
// the end user.
func DefaultChainID() (string, error) {
	cfg, err := tcmd.ParseConfig()
	if err != nil {
		return "", err
	}

	doc, err := tmtypes.GenesisDocFromFile(cfg.GenesisFile())
	if err != nil {
		return "", err
	}

	return doc.ChainID, nil
}

IMO we should get rid of DefaultChainID once and for all, the user needs to provide --chain-id

[aaronc]

I had previously setup my client to pull the chain-id from a config file in the cli settings dir. For our project I had even setup a cli init command. PR #3127 broke this with the addition of c.MarkFlagRequired(FlagChainID) in client/flags.go. At first I thought something had changed with the viper config, but eventually I tracked it down to this.

Is the intention here to always require cli users to specify the chain-id even if they have a config file setup?

[jackzampolin]

I'm with @aaronc on this one. This also broke the gaiad config file for chain ID too...

[alessio]

@aaronc Thank you for taking the time to report this bug and helping to make Cosmos SDK better.

Is the intention here to always require cli users to specify the chain-id even if they have a config file setup?

No, that was not the intention. I'll look into it ASAP.

[alessio]

@aaronc can you paste your config file please?

[aaronc]

@alessio Here's a config file I use to connect to our testnet:

node = "tcp://xrn-us-east-1.regen.network:26657"
chain-id = "xrn-test-2"

[alessio]

@aaronc this is done now, please test

[jackzampolin]

My CLI is giving an error when chainid not provided.

[alessio]

@jackzampolin please run gaiacli config