fix all bits broken by viper API changes (#5982)

github.com/spf13/viper's recent releases introduced a semantic
change in some public API such as viper.IsSet(), which have
broken some of our flags checks. Instead of checking whether
users have changed a flag's default value we should rely on such
defaults and adjust runtime behaviour accordingly. In order to do
so, it's important that we pick sane defaults for all our flags.

The --pruning flag and configuration option now allow for a
fake custom strategy. When users elect custom, then the
pruning-{keep,snapshot}-every options are interpreted and
parsed; else they're ignored.
Zero is pruning-{keep,snapshot}-every default value. When
users choose to set a custom pruning strategy they are
signalling that they want more fine-grainted control, therefore
it's legitimate to expect them to know what they are doing and
enter valid values for both options.

Ref #5964

CHANGELOG.md

@@ -52,6 +52,7 @@ that parse log messages.
 older clients.
 * (x/auth) [\#5844](https://github.com/cosmos/cosmos-sdk/pull/5844) `tx sign` command now returns an error when signing is attempted with offline/multisig keys.
 * (client/keys) [\#5889](https://github.com/cosmos/cosmos-sdk/pull/5889) Remove `keys update` command.
+* (server) [\#5982](https://github.com/cosmos/cosmos-sdk/pull/5982) `--pruning` now must be set to `custom` if you want to customise the granular options.
 
 ### API Breaking Changes
 
@@ -114,6 +115,7 @@ invalid or incomplete requests.
 * (x/genutil) [\#5938](https://github.com/cosmos/cosmos-sdk/pull/5938) Fix `InitializeNodeValidatorFiles` error handling.
 * (x/staking) [\#5949](https://github.com/cosmos/cosmos-sdk/pull/5949) Skip staking `HistoricalInfoKey` in simulations as headers are not exported.
 * (x/auth) [\#5950](https://github.com/cosmos/cosmos-sdk/pull/5950) Fix `IncrementSequenceDecorator` to use is `IsReCheckTx` instead of `IsCheckTx` to allow account sequence incrementing.
+* (client) [\#5964](https://github.com/cosmos/cosmos-sdk/issues/5964) `--trust-node` is now false by default - for real. Users must ensure it is set to true if they don't want to enable the verifier.
 
 ### State Machine Breaking
 

client/context/context.go

@@ -88,6 +88,7 @@ func NewCLIContextWithInputAndFrom(input io.Reader, from string) CLIContext {
 		}
 	}
 
+	trustNode := viper.GetBool(flags.FlagTrustNode)
 	ctx := CLIContext{
 		Client:        rpc,
 		ChainID:       viper.GetString(flags.FlagChainID),
@@ -99,7 +100,7 @@ func NewCLIContextWithInputAndFrom(input io.Reader, from string) CLIContext {
 		OutputFormat:  viper.GetString(cli.OutputFlag),
 		Height:        viper.GetInt64(flags.FlagHeight),
 		HomeDir:       homedir,
-		TrustNode:     viper.GetBool(flags.FlagTrustNode),
+		TrustNode:     trustNode,
 		UseLedger:     viper.GetBool(flags.FlagUseLedger),
 		BroadcastMode: viper.GetString(flags.FlagBroadcastMode),
 		Simulate:      viper.GetBool(flags.FlagDryRun),
@@ -111,9 +112,13 @@ func NewCLIContextWithInputAndFrom(input io.Reader, from string) CLIContext {
 		SkipConfirm:   viper.GetBool(flags.FlagSkipConfirmation),
 	}
 
+	if offline {
+		return ctx
+	}
+
 	// create a verifier for the specific chain ID and RPC client
 	verifier, err := CreateVerifier(ctx, DefaultVerifierCacheSize)
-	if err != nil && viper.IsSet(flags.FlagTrustNode) {
+	if err != nil && !trustNode {
 		fmt.Printf("failed to create verifier: %s\n", err)
 		os.Exit(1)
 	}

client/context/context_test.go

@@ -21,15 +21,6 @@ func TestCLIContext_WithOffline(t *testing.T) {
 	ctx := context.NewCLIContext()
 	require.True(t, ctx.Offline)
 	require.Nil(t, ctx.Client)
-
-	viper.Reset()
-
-	viper.Set(flags.FlagOffline, false)
-	viper.Set(flags.FlagNode, "tcp://localhost:26657")
-
-	ctx = context.NewCLIContext()
-	require.False(t, ctx.Offline)
-	require.NotNil(t, ctx.Client)
 }
 
 func TestCLIContext_WithGenOnly(t *testing.T) {

client/keys/add.go

@@ -190,23 +190,16 @@ func RunAddCmd(cmd *cobra.Command, args []string, kb keyring.Keyring, inBuf *buf
 	coinType := uint32(viper.GetInt(flagCoinType))
 	account := uint32(viper.GetInt(flagAccount))
 	index := uint32(viper.GetInt(flagIndex))
+	hdPath := viper.GetString(flagHDPath)
 
-	useBIP44 := !viper.IsSet(flagHDPath)
-	var hdPath string
-
-	if useBIP44 {
+	if len(hdPath) == 0 {
 		hdPath = hd.CreateHDPath(coinType, account, index).String()
-	} else {
-		hdPath = viper.GetString(flagHDPath)
+	} else if viper.GetBool(flags.FlagUseLedger) {
+		return errors.New("cannot set custom bip32 path with ledger")
 	}
 
 	// If we're using ledger, only thing we need is the path and the bech32 prefix.
 	if viper.GetBool(flags.FlagUseLedger) {
-
-		if !useBIP44 {
-			return errors.New("cannot set custom bip32 path with ledger")
-		}
-
 		bech32PrefixAccAddr := sdk.GetConfig().GetBech32AccountAddrPrefix()
 		info, err := kb.SaveLedgerKey(name, hd.Secp256k1, bech32PrefixAccAddr, coinType, account, index)
 		if err != nil {

server/config/config.go

@@ -35,7 +35,9 @@ type BaseConfig struct {
 	// InterBlockCache enables inter-block caching.
 	InterBlockCache bool `mapstructure:"inter-block-cache"`
 
-	Pruning string `mapstructure:"pruning"`
+	Pruning              string `mapstructure:"pruning"`
+	PruningKeepEvery     string `mapstructure:"pruning-keep-every"`
+	PruningSnapshotEvery string `mapstructure:"pruning-snapshot-every"`
 }
 
 // Config defines the server's top level configuration
@@ -74,9 +76,11 @@ func (c *Config) GetMinGasPrices() sdk.DecCoins {
 func DefaultConfig() *Config {
 	return &Config{
 		BaseConfig{
-			MinGasPrices:    defaultMinGasPrices,
-			InterBlockCache: true,
-			Pruning:         store.PruningStrategySyncable,
+			MinGasPrices:         defaultMinGasPrices,
+			InterBlockCache:      true,
+			Pruning:              store.PruningStrategySyncable,
+			PruningKeepEvery:     "0",
+			PruningSnapshotEvery: "0",
 		},
 	}
 }

server/config/toml.go

@@ -34,11 +34,16 @@ halt-time = {{ .BaseConfig.HaltTime }}
 # InterBlockCache enables inter-block caching.
 inter-block-cache = {{ .BaseConfig.InterBlockCache }}
 
-# Pruning sets the pruning strategy: syncable, nothing, everything
+# Pruning sets the pruning strategy: syncable, nothing, everything, custom
 # syncable: only those states not needed for state syncing will be deleted (keeps last 100 + every 10000th)
 # nothing: all historic states will be saved, nothing will be deleted (i.e. archiving node)
 # everything: all saved states will be deleted, storing only the current state
+# custom: allows fine-grained control through the pruning-keep-every and pruning-snapshot-every options.
 pruning = "{{ .BaseConfig.Pruning }}"
+
+# These are applied if and only if the pruning strategy is custom.
+pruning-keep-every = "{{ .BaseConfig.PruningKeepEvery }}"
+pruning-snapshot-every = "{{ .BaseConfig.PruningSnapshotEvery }}"
 `
 
 var configTemplate *template.Template

server/pruning.go

@@ -1,6 +1,8 @@
 package server
 
 import (
+	"fmt"
+
 	"github.com/spf13/viper"
 
 	"github.com/cosmos/cosmos-sdk/store"
@@ -9,17 +11,23 @@ import (
 // GetPruningOptionsFromFlags parses start command flags and returns the correct PruningOptions.
 // flagPruning prevails over flagPruningKeepEvery and flagPruningSnapshotEvery.
 // Default option is PruneSyncable.
-func GetPruningOptionsFromFlags() store.PruningOptions {
-	if viper.IsSet(flagPruning) {
-		return store.NewPruningOptionsFromString(viper.GetString(flagPruning))
-	}
+func GetPruningOptionsFromFlags() (store.PruningOptions, error) {
+	strategy := viper.GetString(flagPruning)
+	switch strategy {
+	case "syncable", "nothing", "everything":
+		return store.NewPruningOptionsFromString(viper.GetString(flagPruning)), nil
 
-	if viper.IsSet(flagPruningKeepEvery) && viper.IsSet(flagPruningSnapshotEvery) {
-		return store.PruningOptions{
+	case "custom":
+		opts := store.PruningOptions{
 			KeepEvery:     viper.GetInt64(flagPruningKeepEvery),
 			SnapshotEvery: viper.GetInt64(flagPruningSnapshotEvery),
 		}
-	}
+		if !opts.IsValid() {
+			return opts, fmt.Errorf("invalid granular options")
+		}
+		return opts, nil
 
-	return store.PruneSyncable
+	default:
+		return store.PruningOptions{}, fmt.Errorf("unknown pruning strategy %s", strategy)
+	}
 }

server/pruning_test.go

@@ -14,6 +14,7 @@ func TestGetPruningOptionsFromFlags(t *testing.T) {
 		name            string
 		initParams      func()
 		expectedOptions store.PruningOptions
+		wantErr         bool
 	}{
 		{
 			name: "pruning",
@@ -25,6 +26,7 @@ func TestGetPruningOptionsFromFlags(t *testing.T) {
 		{
 			name: "granular pruning",
 			initParams: func() {
+				viper.Set(flagPruning, "custom")
 				viper.Set(flagPruningSnapshotEvery, 1234)
 				viper.Set(flagPruningKeepEvery, 4321)
 			},
@@ -44,8 +46,14 @@ func TestGetPruningOptionsFromFlags(t *testing.T) {
 		tt := tt
 		t.Run(tt.name, func(j *testing.T) {
 			viper.Reset()
+			viper.SetDefault(flagPruning, "syncable")
 			tt.initParams()
-			require.Equal(t, tt.expectedOptions, GetPruningOptionsFromFlags())
+			opts, err := GetPruningOptionsFromFlags()
+			if tt.wantErr {
+				require.Error(t, err)
+				return
+			}
+			require.Equal(t, tt.expectedOptions, opts)
 		})
 	}
 }

server/start.go

@@ -72,7 +72,8 @@ For profiling and benchmarking purposes, CPU profiling can be enabled via the '-
 which accepts a path for the resulting pprof file.
 `,
 		PreRunE: func(cmd *cobra.Command, args []string) error {
-			return checkPruningParams()
+			_, err := GetPruningOptionsFromFlags()
+			return err
 		},
 		RunE: func(cmd *cobra.Command, args []string) error {
 			if !viper.GetBool(flagWithTendermint) {
@@ -91,9 +92,9 @@ which accepts a path for the resulting pprof file.
 	cmd.Flags().Bool(flagWithTendermint, true, "Run abci app embedded in-process with tendermint")
 	cmd.Flags().String(flagAddress, "tcp://0.0.0.0:26658", "Listen address")
 	cmd.Flags().String(flagTraceStore, "", "Enable KVStore tracing to an output file")
-	cmd.Flags().String(flagPruning, "syncable", "Pruning strategy: syncable, nothing, everything")
-	cmd.Flags().Int64(flagPruningKeepEvery, 0, "Define the state number that will be kept")
-	cmd.Flags().Int64(flagPruningSnapshotEvery, 0, "Defines the state that will be snapshot for pruning")
+	cmd.Flags().String(flagPruning, "syncable", "Pruning strategy: syncable, nothing, everything, custom")
+	cmd.Flags().Int64(flagPruningKeepEvery, 0, "Define the state number that will be kept. Ignored if pruning is not custom.")
+	cmd.Flags().Int64(flagPruningSnapshotEvery, 0, "Defines the state that will be snapshot for pruning. Ignored if pruning is not custom.")
 	cmd.Flags().String(
 		FlagMinGasPrices, "",
 		"Minimum gas prices to accept for transactions; Any fee in a tx must meet this minimum (e.g. 0.01photino;0.0001stake)",
@@ -104,32 +105,15 @@ which accepts a path for the resulting pprof file.
 	cmd.Flags().Bool(FlagInterBlockCache, true, "Enable inter-block caching")
 	cmd.Flags().String(flagCPUProfile, "", "Enable CPU profiling and write to the provided file")
 
+	viper.BindPFlag(flagPruning, cmd.Flags().Lookup(flagPruning))
+	viper.BindPFlag(flagPruningKeepEvery, cmd.Flags().Lookup(flagPruningKeepEvery))
+	viper.BindPFlag(flagPruningSnapshotEvery, cmd.Flags().Lookup(flagPruningSnapshotEvery))
+
 	// add support for all Tendermint-specific command line options
 	tcmd.AddNodeFlags(cmd)
 	return cmd
 }
 
-// checkPruningParams checks that the provided pruning params are correct
-func checkPruningParams() error {
-	if !viper.IsSet(flagPruning) && !viper.IsSet(flagPruningKeepEvery) && !viper.IsSet(flagPruningSnapshotEvery) {
-		return nil
-	}
-
-	if viper.IsSet(flagPruning) {
-		if viper.IsSet(flagPruningKeepEvery) || viper.IsSet(flagPruningSnapshotEvery) {
-			return errPruningWithGranularOptions
-		}
-
-		return nil
-	}
-
-	if !(viper.IsSet(flagPruningKeepEvery) && viper.IsSet(flagPruningSnapshotEvery)) {
-		return errPruningGranularOptions
-	}
-
-	return nil
-}
-
 func startStandAlone(ctx *Context, appCreator AppCreator) error {
 	addr := viper.GetString(flagAddress)
 	home := viper.GetString("home")

server/start_test.go

@@ -1,84 +1,64 @@
 package server
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/spf13/viper"
 	"github.com/stretchr/testify/require"
 )
 
 func TestPruningOptions(t *testing.T) {
-	startCommand := StartCmd(nil, nil)
-
 	tests := []struct {
 		name        string
 		paramInit   func()
 		returnsErr  bool
 		expectedErr error
 	}{
 		{
-			name:        "none set, returns nil and will use default from flags",
+			name:        "default",
 			paramInit:   func() {},
 			returnsErr:  false,
 			expectedErr: nil,
 		},
+		{
+			name:        "unknown strategy",
+			paramInit:   func() { viper.Set(flagPruning, "unknown") },
+			returnsErr:  true,
+			expectedErr: fmt.Errorf("unknown pruning strategy unknown"),
+		},
 		{
 			name: "only keep-every provided",
 			paramInit: func() {
+				viper.Set(flagPruning, "custom")
 				viper.Set(flagPruningKeepEvery, 12345)
 			},
-			returnsErr:  true,
-			expectedErr: errPruningGranularOptions,
+			returnsErr:  false,
+			expectedErr: nil,
 		},
 		{
 			name: "only snapshot-every provided",
 			paramInit: func() {
+				viper.Set(flagPruning, "custom")
 				viper.Set(flagPruningSnapshotEvery, 12345)
 			},
 			returnsErr:  true,
-			expectedErr: errPruningGranularOptions,
-		},
-		{
-			name: "pruning flag with other granular options 1",
-			paramInit: func() {
-				viper.Set(flagPruning, "set")
-				viper.Set(flagPruningSnapshotEvery, 1234)
-			},
-			returnsErr:  true,
-			expectedErr: errPruningWithGranularOptions,
-		},
-		{
-			name: "pruning flag with other granular options 2",
-			paramInit: func() {
-				viper.Set(flagPruning, "set")
-				viper.Set(flagPruningKeepEvery, 1234)
-			},
-			returnsErr:  true,
-			expectedErr: errPruningWithGranularOptions,
+			expectedErr: fmt.Errorf("invalid granular options"),
 		},
 		{
 			name: "pruning flag with other granular options 3",
 			paramInit: func() {
-				viper.Set(flagPruning, "set")
+				viper.Set(flagPruning, "custom")
 				viper.Set(flagPruningKeepEvery, 1234)
 				viper.Set(flagPruningSnapshotEvery, 1234)
 			},
-			returnsErr:  true,
-			expectedErr: errPruningWithGranularOptions,
-		},
-		{
-			name: "only prunning set",
-			paramInit: func() {
-				viper.Set(flagPruning, "set")
-			},
 			returnsErr:  false,
 			expectedErr: nil,
 		},
 		{
-			name: "only granular set",
+			name: "nothing strategy",
 			paramInit: func() {
-				viper.Set(flagPruningSnapshotEvery, 12345)
-				viper.Set(flagPruningKeepEvery, 12345)
+				viper.Set(flagPruning, "nothing")
 			},
 			returnsErr:  false,
 			expectedErr: nil,
@@ -90,9 +70,10 @@ func TestPruningOptions(t *testing.T) {
 
 		t.Run(tt.name, func(t *testing.T) {
 			viper.Reset()
+			viper.SetDefault(flagPruning, "syncable")
+			startCommand := StartCmd(nil, nil)
 			tt.paramInit()
-
-			err := startCommand.PreRunE(nil, nil)
+			err := startCommand.PreRunE(startCommand, nil)
 
 			if tt.returnsErr {
 				require.EqualError(t, err, tt.expectedErr.Error())