From bf5b82578cc48ed5b4c5bdcb33d61c8d05fdee0e Mon Sep 17 00:00:00 2001 From: Daniel Wedul <github@wedul.com> Date: Thu, 24 Oct 2024 14:32:49 -0600 Subject: [PATCH] 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. --- .../improvements/2121-commit-timeout.md | 1 + cmd/provenanced/cmd/pre_upgrade_test.go | 54 ++++++++ cmd/provenanced/cmd/root.go | 21 +-- cmd/provenanced/cmd/root_test.go | 121 ++++++++++-------- cmd/provenanced/config/interceptor.go | 9 ++ testutil/testnet.go | 28 ++++ 6 files changed, 175 insertions(+), 59 deletions(-) create mode 100644 .changelog/unreleased/improvements/2121-commit-timeout.md create mode 100644 testutil/testnet.go diff --git a/.changelog/unreleased/improvements/2121-commit-timeout.md b/.changelog/unreleased/improvements/2121-commit-timeout.md new file mode 100644 index 000000000..e0f0c9f67 --- /dev/null +++ b/.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). diff --git a/cmd/provenanced/cmd/pre_upgrade_test.go b/cmd/provenanced/cmd/pre_upgrade_test.go index 723e0df15..69a809aa1 100644 --- a/cmd/provenanced/cmd/pre_upgrade_test.go +++ b/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 { diff --git a/cmd/provenanced/cmd/root.go b/cmd/provenanced/cmd/root.go index c6ba7160b..d151d4d01 100644 --- a/cmd/provenanced/cmd/root.go +++ b/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)) } } } diff --git a/cmd/provenanced/cmd/root_test.go b/cmd/provenanced/cmd/root_test.go index 10210bf02..11c46a8d0 100644 --- a/cmd/provenanced/cmd/root_test.go +++ b/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 diff --git a/cmd/provenanced/config/interceptor.go b/cmd/provenanced/config/interceptor.go index e854bdc62..f7e06fc26 100644 --- a/cmd/provenanced/config/interceptor.go +++ b/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) diff --git a/testutil/testnet.go b/testutil/testnet.go new file mode 100644 index 000000000..caa508782 --- /dev/null +++ b/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) + } +}