edit-validator will set moniker with the local hostname

[wukongcheng]

Summary of Bug

If you call the 'edit-validator' without the '--moniker' flag, the 'moniker' of the validator's description will be set with the local hostname.

Steps to Reproduce

• gaiacli query validators
  Description: {silei }
• gaiacli tx edit-validator --from=silei
  Description: {SileiMac.local }
• gaiacli tx edit-validator --moniker=raymond --from=silei
  Description: {raymond }

What's the reason

The flag '--moniker' in 'edit-validator' cli has the same name with the tendermint's 'moniker'. And in tendermint, moniker defaults to the machine's hostname instead of "anonymous". Both the two 'moniker' take effect by the spf13/viper.

If the flag is not specified, viper will look up its kvstore before the default value:

    // K/V store next
val = v.searchMap(v.kvstore, path)
if val != nil {
	return val
}
// Default next
val = v.searchMap(v.defaults, path)
if val != nil {
	return val
}

So the tendermint 'moniker' will be returned.

After I use another flag name instead of 'moniker' in the edit-validator, it works fine.

[alexanderbez]

Thanks for the report @wukongcheng! Indeed you are correct, Viper strikes again.

[rigelrozanski]

omg 🤦 - thanks for the catch, and the investigation into the cause

[alexanderbez]

Not sure what the best course of action is here? Maybe a TM update? I think using a different flag may result in a messy/confusing UX. I suppose we could prefix all the edit flags with new-* for the sake of consistency (if we do change the flag).

@ValarDragon would it be harmful to not default to the hostname in TM?

[ValarDragon]

I think hostname is a sensible default for TM. It would probably make observability on testing tendermint clusters harder to not have it default to hostname, though arguably we could say that in that situation the tester should be editing the moniker themself. Does viper expose a boolean of any sort to indicate if it was manually set or not?

[alexanderbez]

@ValarDragon yes but it doesn't work lol...issue has been open forever iirc.

[jackzampolin]

Sounds like we can move this from issue to intended behavior with a small documentation change? Are there any downsides (security?) to doing this?

[ValarDragon]

I definitely think we should fix this. I imagine validators will get quite upset if their monikers change unexpectedly. (This shouldn't be expected behavior lol)

[alexanderbez]

Yes, this definitely needs to be fixed. I suppose we can prefix all the flags with new-*. This would be the easiest approach without changing existing nomenclature I THINK.

[cwgoes]

@alessio Did we fix this already?

[alessio]

AFAICT not just yet

[alessio]

The GetCmdEditValidator()'s flags defaults that relate to the Validator's Description are set to [do-not-modify] by default:

./x/stake/client/cli/flags.go:	fsDescriptionEdit.String(FlagMoniker, types.DoNotModifyDesc, "validator name")
./x/stake/client/cli/flags.go:	fsDescriptionEdit.String(FlagIdentity, types.DoNotModifyDesc, "optional identity signature (ex. UPort or Keybase)")
./x/stake/client/cli/flags.go:	fsDescriptionEdit.String(FlagWebsite, types.DoNotModifyDesc, "optional website")
./x/stake/client/cli/flags.go:	fsDescriptionEdit.String(FlagDetails, types.DoNotModifyDesc, "optional details")

It should be relatively easy for the command to build a new Description struct accordingly.

[alessio]

I can't reproduce this. If I run gaiacli tx staking edit-validator the new staking.Description looks like the following:

{Moniker:[do-not-modify] Identity:[do-not-modify] Website:[do-not-modify] Details:[do-not-modify]}

Which is an expected behaviour.

[rigelrozanski]

I remember there was a bug to this affect which was previously already fixed... maybe worth rummaging through related issues, this issue may already be resolved

[jackzampolin]

Looks like this issue is resolved. Please reopen if you run into this issue.