Hard-code consensus.timeout_commit to 3.5s for mainnet. (#2196)

* [2121]: Change the default consensus timeout value to 3.5 seconds.

* [2121]: Hard-code the consensus.timeout_commit value.

* [2121]: Fix TestIsTestnetFlagSet to not be affected by existing env vars.

* [2121]: Fix a couple unit tests that broke when I changed the default commit timout.

* [2121]: Only hard-code the timeout commit on non-testnets.

* [2121]: Change the default back to 1.5s for faster default testnets.

* [2121]: Fix the TestPreUpgradeCmd that broke because of the hard-coded timeout commit.

* [2121]: Add some unit tests that make sure the consensus timeout commit value is behaving as expected.

* [2121]: Add changelog entry.

* [2121]: When forcing the timeout_commit to be 3.5 seconds, also force the skip flag to be false.

* [2121]: Update warnAboutSettings: Evaluate the timeout commit and skip-timeout-commit fields separately. Issue a warning if skip-timeout-commit is true. Issue a warning if the timeout commit is not exactly what we want it to be.

.changelog/unreleased/improvements/2121-commit-timeout.md

@@ -0,0 +1 @@
+* Hard code the mainnet `consensus.timeout_commit` config value to 3.5s [#2121](https://github.com/provenance-io/provenance/issues/2121).

cmd/provenanced/cmd/pre_upgrade_test.go

@@ -252,6 +252,7 @@ func TestPreUpgradeCmd(t *testing.T) {
 
 	appCfgD := config.DefaultAppConfig()
 	cmtCfgD := config.DefaultCmtConfig()
+	cmtCfgD.Consensus.TimeoutCommit = 3500 * time.Millisecond
 	clientCfgD := config.DefaultClientConfig()
 
 	appCfgC := config.DefaultAppConfig()
@@ -266,6 +267,9 @@ func TestPreUpgradeCmd(t *testing.T) {
 	clientCfgAsync := config.DefaultClientConfig()
 	clientCfgAsync.BroadcastMode = "async"
 
+	cmtCfgT := config.DefaultCmtConfig()
+	cmtCfgT.Consensus.TimeoutCommit = 777 * time.Second
+
 	successMsg := "pre-upgrade successful"
 	updatingBlocksyncMsg := "Updating the broadcast_mode config value to \"sync\" (from \"block\", which is no longer an option)."
 
@@ -432,6 +436,56 @@ func TestPreUpgradeCmd(t *testing.T) {
 			expCmtCfg:    cmtCfgD,
 			expClientCfg: clientCfgAsync,
 		},
+		{
+			name: "unpacked mainnet timeout commit",
+			setup: func(t *testing.T) (string, func(), bool) {
+				home, success := newHome(t, "unpacked_mainnet_timeout_commit", appCfgD, cmtCfgT, clientCfgD)
+				return home, nil, success
+			},
+			expExitCode:  0,
+			expInStdout:  []string{successMsg},
+			expAppCfg:    appCfgD,
+			expCmtCfg:    cmtCfgD,
+			expClientCfg: clientCfgD,
+		},
+		{
+			name: "packed mainnet timeout commit",
+			setup: func(t *testing.T) (string, func(), bool) {
+				home, success := newHomePacked(t, "packed_mainnet_timeout_commit", appCfgD, cmtCfgT, clientCfgD)
+				return home, nil, success
+			},
+			expExitCode:  0,
+			expInStdout:  []string{successMsg},
+			expAppCfg:    appCfgD,
+			expCmtCfg:    cmtCfgD,
+			expClientCfg: clientCfgD,
+		},
+		{
+			name: "unpacked testnet timeout commit",
+			setup: func(t *testing.T) (string, func(), bool) {
+				home, success := newHome(t, "unpacked_testnet_timeout_commit", appCfgD, cmtCfgT, clientCfgD)
+				return home, nil, success
+			},
+			args:         []string{"--testnet"},
+			expExitCode:  0,
+			expInStdout:  []string{successMsg},
+			expAppCfg:    appCfgD,
+			expCmtCfg:    cmtCfgT,
+			expClientCfg: clientCfgD,
+		},
+		{
+			name: "packed testnet timeout commit",
+			setup: func(t *testing.T) (string, func(), bool) {
+				home, success := newHomePacked(t, "packed_testnet_timeout_commit", appCfgD, cmtCfgT, clientCfgD)
+				return home, nil, success
+			},
+			args:         []string{"--testnet"},
+			expExitCode:  0,
+			expInStdout:  []string{successMsg},
+			expAppCfg:    appCfgD,
+			expCmtCfg:    cmtCfgT,
+			expClientCfg: clientCfgD,
+		},
 	}
 
 	for _, tc := range tests {

cmd/provenanced/cmd/root.go

@@ -7,7 +7,6 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
-	"time"
 
 	"github.com/rs/zerolog"
 	"github.com/spf13/cast"
@@ -332,14 +331,18 @@ func warnAboutSettings(logger log.Logger, appOpts servertypes.AppOptions) {
 
 	chainID := cast.ToString(appOpts.Get(flags.FlagChainID))
 	if chainID == "pio-mainnet-1" {
-		skipTimeoutCommit := cast.ToBool(appOpts.Get("consensus.skip_timeout_commit"))
-		if !skipTimeoutCommit {
-			timeoutCommit := cast.ToDuration(appOpts.Get("consensus.timeout_commit"))
-			upperLimit := config.DefaultConsensusTimeoutCommit + 2*time.Second
-			if timeoutCommit > upperLimit {
-				logger.Error(fmt.Sprintf("Your consensus.timeout_commit config value is too high and should be decreased to at most %q. The recommended value is %q. Your current value is %q.",
-					upperLimit, config.DefaultConsensusTimeoutCommit, timeoutCommit))
-			}
+		skipTimeoutCommit := cast.ToBool(appOpts.Get(config.ConsensusSkipTimeoutCommitKey))
+		expSkipTimeoutCommit := cast.ToBool(config.ConsensusSkipTimeoutCommitValue)
+		if skipTimeoutCommit != expSkipTimeoutCommit {
+			logger.Error(fmt.Sprintf("Your %s config value should be %t, but is %t.",
+				config.ConsensusSkipTimeoutCommitKey, expSkipTimeoutCommit, skipTimeoutCommit))
+		}
+
+		timeoutCommit := cast.ToDuration(appOpts.Get(config.ConsensusTimeoutCommitKey))
+		expTimeoutCommit := cast.ToDuration(config.ConsensusTimeoutCommitValue)
+		if timeoutCommit != expTimeoutCommit {
+			logger.Error(fmt.Sprintf("Your %s config value should be %s, but is %s.",
+				config.ConsensusTimeoutCommitKey, expTimeoutCommit, timeoutCommit))
 		}
 	}
 }

cmd/provenanced/cmd/root_test.go

@@ -26,6 +26,7 @@ import (
 
 	"github.com/provenance-io/provenance/cmd/provenanced/config"
 	"github.com/provenance-io/provenance/internal"
+	"github.com/provenance-io/provenance/testutil"
 )
 
 func TestIAVLConfig(t *testing.T) {
@@ -50,17 +51,19 @@ func (o panicOpts) Get(key string) interface{} {
 
 func TestWarnAboutSettings(t *testing.T) {
 	keyChainID := flags.FlagChainID
-	keySkipTimeoutCommit := "consensus.skip_timeout_commit"
-	keyTimeoutCommit := "consensus.timeout_commit"
+	keySkipTimeoutCommit := config.ConsensusSkipTimeoutCommitKey
+	keyTimeoutCommit := config.ConsensusTimeoutCommitKey
+
+	expTimeoutCommit := cast.ToDuration(config.ConsensusTimeoutCommitValue)
+	lowTimeoutCommit := expTimeoutCommit - 2*time.Millisecond
+	highTimeoutCommit := expTimeoutCommit + 3*time.Millisecond
 
-	upperLimit := config.DefaultConsensusTimeoutCommit + 2*time.Second
-	overUpperLimit := upperLimit + 1*time.Millisecond
 	mainnetChainID := "pio-mainnet-1"
 	notMainnetChainID := "not-" + mainnetChainID
 
-	tooHighMsg := func(timeoutCommit time.Duration) string {
-		return fmt.Sprintf("ERR Your consensus.timeout_commit config value is too high and should be decreased to at most %q. The recommended value is %q. Your current value is %q.",
-			upperLimit, config.DefaultConsensusTimeoutCommit, timeoutCommit)
+	badSkipMsg := fmt.Sprintf("ERR Your %s config value should be false, but is true.", config.ConsensusSkipTimeoutCommitKey)
+	badTimeoutMsg := func(curVal string) string {
+		return fmt.Sprintf("ERR Your %s config value should be 3.5s, but is %s.", config.ConsensusTimeoutCommitKey, curVal)
 	}
 
 	mainnetOpts := func(skipTimeoutCommit bool, timeoutCommit time.Duration) testOpts {
@@ -84,89 +87,104 @@ func TestWarnAboutSettings(t *testing.T) {
 		expLogged []string
 	}{
 		{
-			name:      "mainnet not skipped value over upper limit",
-			appOpts:   mainnetOpts(false, overUpperLimit),
-			expLogged: []string{tooHighMsg(overUpperLimit)},
+			name:    "empty opts",
+			appOpts: testOpts{},
 		},
 		{
-			name:    "mainnet not skipped value at limit",
-			appOpts: mainnetOpts(false, upperLimit),
+			name:      "only chain-id mainnet",
+			appOpts:   testOpts{keyChainID: mainnetChainID},
+			expLogged: []string{badTimeoutMsg("0s")},
 		},
 		{
-			name:    "mainnet not skipped value at default",
-			appOpts: mainnetOpts(false, config.DefaultConsensusTimeoutCommit),
+			name:    "only chain-id not mainnet",
+			appOpts: testOpts{keyChainID: notMainnetChainID},
 		},
 		{
-			name:    "mainnet not skipped value at zero",
-			appOpts: mainnetOpts(false, 0),
+			name:      "mainnet, only skip false",
+			appOpts:   testOpts{keyChainID: mainnetChainID, keySkipTimeoutCommit: false},
+			expLogged: []string{badTimeoutMsg("0s")},
 		},
 		{
-			name:    "mainnet skipped value over upper limit",
-			appOpts: mainnetOpts(true, overUpperLimit),
+			name:      "mainnet, only skip true",
+			appOpts:   testOpts{keyChainID: mainnetChainID, keySkipTimeoutCommit: true},
+			expLogged: []string{badSkipMsg, badTimeoutMsg("0s")},
 		},
 		{
-			name:    "mainnet skipped value at upper limit",
-			appOpts: mainnetOpts(true, upperLimit),
+			name:      "mainnet, only skip invalid",
+			appOpts:   testOpts{keyChainID: mainnetChainID, keySkipTimeoutCommit: "abcd"},
+			expLogged: []string{badTimeoutMsg("0s")},
 		},
 		{
-			name:    "mainnet skipped value at default",
-			appOpts: mainnetOpts(true, config.DefaultConsensusTimeoutCommit),
+			name:      "mainnet, only low timeout",
+			appOpts:   testOpts{keyChainID: mainnetChainID, keyTimeoutCommit: lowTimeoutCommit},
+			expLogged: []string{badTimeoutMsg(lowTimeoutCommit.String())},
 		},
 		{
-			name:    "mainnet skipped value at zero",
-			appOpts: mainnetOpts(true, 0),
+			name:    "mainnet, only right timeout",
+			appOpts: testOpts{keyChainID: mainnetChainID, keyTimeoutCommit: expTimeoutCommit},
 		},
 		{
-			name:    "not mainnet not skipped value over upper limit",
-			appOpts: notMainnetOpts(false, overUpperLimit),
+			name:      "mainnet, only high timeout",
+			appOpts:   testOpts{keyChainID: mainnetChainID, keyTimeoutCommit: highTimeoutCommit},
+			expLogged: []string{badTimeoutMsg(highTimeoutCommit.String())},
 		},
 		{
-			name:    "not mainnet not skipped value at upper limit",
-			appOpts: notMainnetOpts(false, upperLimit),
+			name:      "mainnet, only invalid timeout",
+			appOpts:   testOpts{keyChainID: mainnetChainID, keyTimeoutCommit: "xyz"},
+			expLogged: []string{badTimeoutMsg("0s")},
 		},
 		{
-			name:    "not mainnet not skipped value at default",
-			appOpts: notMainnetOpts(false, config.DefaultConsensusTimeoutCommit),
+			name:      "mainnet, skip false, low timeout",
+			appOpts:   mainnetOpts(false, lowTimeoutCommit),
+			expLogged: []string{badTimeoutMsg(lowTimeoutCommit.String())},
 		},
 		{
-			name:    "not mainnet not skipped value at zero",
-			appOpts: notMainnetOpts(false, 0),
+			name:    "mainnet, skip false, right timeout",
+			appOpts: mainnetOpts(false, expTimeoutCommit),
 		},
 		{
-			name:    "not mainnet skipped value over upper limit",
-			appOpts: notMainnetOpts(true, overUpperLimit),
+			name:      "mainnet, skip false, high timeout",
+			appOpts:   mainnetOpts(false, highTimeoutCommit),
+			expLogged: []string{badTimeoutMsg(highTimeoutCommit.String())},
 		},
 		{
-			name:    "not mainnet skipped value at upper limit",
-			appOpts: notMainnetOpts(true, upperLimit),
+			name:      "mainnet, skip true, low timeout",
+			appOpts:   mainnetOpts(true, lowTimeoutCommit),
+			expLogged: []string{badSkipMsg, badTimeoutMsg(lowTimeoutCommit.String())},
 		},
 		{
-			name:    "not mainnet skipped value at default",
-			appOpts: notMainnetOpts(true, config.DefaultConsensusTimeoutCommit),
+			name:      "mainnet, skip true, right timeout",
+			appOpts:   mainnetOpts(true, expTimeoutCommit),
+			expLogged: []string{badSkipMsg},
 		},
 		{
-			name:    "not mainnet skipped value at zero",
-			appOpts: notMainnetOpts(true, 0),
+			name:      "mainnet, skip true, high timeout",
+			appOpts:   mainnetOpts(true, highTimeoutCommit),
+			expLogged: []string{badSkipMsg, badTimeoutMsg(highTimeoutCommit.String())},
 		},
 		{
-			name:    "timeout commit opt not a duration",
-			appOpts: testOpts{keyChainID: mainnetChainID, keySkipTimeoutCommit: false, keyTimeoutCommit: "nope"},
+			name:    "not mainnet, skip false, low timeout",
+			appOpts: notMainnetOpts(false, lowTimeoutCommit),
 		},
 		{
-			name:    "empty opts",
-			appOpts: testOpts{},
+			name:    "not mainnet, skip false, right timeout",
+			appOpts: notMainnetOpts(false, expTimeoutCommit),
 		},
 		{
-			name:    "only chain-id mainnet",
-			appOpts: testOpts{keyChainID: mainnetChainID},
+			name:    "not mainnet, skip false, high timeout",
+			appOpts: notMainnetOpts(false, highTimeoutCommit),
 		},
 		{
-			name:    "only chain-id not mainnet",
-			appOpts: testOpts{keyChainID: mainnetChainID},
+			name:    "not mainnet, skip true, low timeout",
+			appOpts: notMainnetOpts(true, lowTimeoutCommit),
 		},
 		{
-			name:    "mainnet not skipped no timeout commit opt",
-			appOpts: testOpts{keyChainID: mainnetChainID, keySkipTimeoutCommit: false},
+			name:    "not mainnet, skip true, right timeout",
+			appOpts: notMainnetOpts(true, expTimeoutCommit),
+		},
+		{
+			name:    "not mainnet, skip true, high timeout",
+			appOpts: notMainnetOpts(true, highTimeoutCommit),
 		},
 		{
 			name:    "panic from getter",
@@ -340,6 +358,9 @@ func appendIfNew(slice []string, elems ...string) []string {
 func TestIsTestnetFlagSet(t *testing.T) {
 	envVar := "PIO_TESTNET"
 
+	// Don't let a pre-existing PIO_TESTNET environment variable affect the tests.
+	defer testutil.UnsetTestnetEnvVar()()
+
 	tests := []struct {
 		name    string
 		envArgs map[string]string

cmd/provenanced/config/interceptor.go

@@ -23,6 +23,11 @@ const (
 	EnvTypeFlag = "testnet"
 	// CoinTypeFlag is a flag for indicating coin type.
 	CoinTypeFlag = "coin-type"
+
+	ConsensusTimeoutCommitKey       = "consensus.timeout_commit"
+	ConsensusTimeoutCommitValue     = "3.5s"
+	ConsensusSkipTimeoutCommitKey   = "consensus.skip_timeout_commit"
+	ConsensusSkipTimeoutCommitValue = "false"
 )
 
 // InterceptConfigsPreRunHandler performs a pre-run function for all commands.
@@ -58,6 +63,10 @@ func InterceptConfigsPreRunHandler(cmd *cobra.Command) error {
 	//  2. The config is packed and we're filling in the missing with defaults.
 	if vpr.GetBool(EnvTypeFlag) {
 		DefaultKeyringBackend = "test"
+	} else {
+		// Hard-code the consensus.timeout_commit value for non-testnets.
+		vpr.Set(ConsensusTimeoutCommitKey, ConsensusTimeoutCommitValue)
+		vpr.Set(ConsensusSkipTimeoutCommitKey, ConsensusSkipTimeoutCommitValue)
 	}
 	// Read the configs into viper and the contexts.
 	return LoadConfigFromFiles(cmd)

testutil/testnet.go

@@ -0,0 +1,28 @@
+package testutil
+
+import "os"
+
+const TestnetEnvVar = "PIO_TESTNET"
+
+// UnsetTestnetEnvVar will unset the PIO_TESTNET environment variable and return a deferrable that will put it back.
+//
+// Go runs tests inside an environment that might already have some environment variables defined. E.g. if you
+// have `export PIO_TESTNET=true` in your environment, and run `make test`, then, when a test runs, it will
+// start with a `PIO_TESTNET` value of `true`. But that can mess up some tests that expect to start without a
+// PIO_TESTNET env var set.
+//
+// For individual test cases, you should use t.Setenv for changing environment variables.
+// This exists because t.Setenv can't be used to unset an environment variable.
+//
+// Standard usage: defer testutil.UnsetTestnetEnvVar()()
+func UnsetTestnetEnvVar() func() {
+	if origVal, ok := os.LookupEnv(TestnetEnvVar); ok {
+		os.Unsetenv(TestnetEnvVar)
+		return func() {
+			os.Setenv(TestnetEnvVar, origVal)
+		}
+	}
+	return func() {
+		os.Unsetenv(TestnetEnvVar)
+	}
+}