Skip to content

Commit

Permalink
Change the default consensus.timeout_commit to 1500ms. (#1600) (#1601)
Browse files Browse the repository at this point in the history 
* Change the DefaultConsensusTimeoutCommit to 1500ms.

* Update the pre-upgrade command to change all consensus.timeout_commit settings to 1500ms.

* Update the log warning about the consensus.timeout_commit value.

* Add changelog entry.

* Fix some broken unit tests.

* Make those tests I just fixed more robust. They now just use the provconfig.DefaultConsensusTimeoutCommit value instead of a hard-coded value.

* Add some unit tests on the config warner.
  • Loading branch information
@SpicyLemon
SpicyLemon committed Jun 21, 2023
1 parent 1b377e8 commit cf436c0
Show file tree
Hide file tree
Showing 8 changed files with 302 additions and 51 deletions.
4 changes: 2 additions & 2 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,12 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Features

* Add pre-upgrade command that updates config files to newest format and sets `consensus.timeout_commit` to `5s` [PR 1594](https://github.com/provenance-io/provenance/pull/1594)
* Add pre-upgrade command that updates config files to newest format and sets `consensus.timeout_commit` to `1500ms` [PR 1594](https://github.com/provenance-io/provenance/pull/1594), [PR 1600](https://github.com/provenance-io/provenance/pull/1600).

### Bug Fixes

* Add `NewUpdateAccountAttributeExpirationCmd` to the CLI [#1592](https://github.com/provenance-io/provenance/issues/1592).
* Fix `minimum-gas-prices` from sometimes getting unset in the configs [PR 1594](https://github.com/provenance-io/provenance/pull/1594)
* Fix `minimum-gas-prices` from sometimes getting unset in the configs [PR 1594](https://github.com/provenance-io/provenance/pull/1594).

---

Expand Down
6 changes: 3 additions & 3 deletions cmd/provenanced/cmd/config_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ func (s *ConfigTestSuite) TestConfigCmdGet() {
// Tendermint header and a few entries.
{"tendermint header", regexp.MustCompile(`(?m)^Tendermint Config: .*/config/` + s.BaseFNTM + ` \(or env\)$`)},
{"tendermint fast_sync", regexp.MustCompile(`(?m)^fast_sync=true$`)},
{"tendermint consensus.timeout_commit", regexp.MustCompile(`(?m)^consensus.timeout_commit="5s"$`)},
{"tendermint consensus.timeout_commit", regexp.MustCompile(`(?m)^consensus.timeout_commit=` + fmt.Sprintf("%q", provconfig.DefaultConsensusTimeoutCommit) + `$`)},
{"tendermint mempool.size", regexp.MustCompile(`(?m)^mempool.size=5000$`)},
{"tendermint statesync.trust_period", regexp.MustCompile(`(?m)^statesync.trust_period="168h0m0s"$`)},
{"tendermint p2p.recv_rate", regexp.MustCompile(`(?m)^p2p.recv_rate=5120000$`)},
Expand Down Expand Up @@ -609,7 +609,7 @@ func (s *ConfigTestSuite) TestConfigCmdSet() {
},
{
name: "consensus.timeout_commit",
oldVal: `"5s"`,
oldVal: fmt.Sprintf("%q", provconfig.DefaultConsensusTimeoutCommit),
newVal: `"2s"`,
toMatch: []*regexp.Regexp{reTMConfigUpdated},
},
Expand Down Expand Up @@ -701,7 +701,7 @@ func (s *ConfigTestSuite) TestConfigSetMulti() {
out: s.makeMultiLine(
s.makeTMConfigUpdateLines(),
s.makeKeyUpdatedLine("log_format", `"plain"`, `"json"`),
s.makeKeyUpdatedLine("consensus.timeout_commit", `"5s"`, `"950ms"`),
s.makeKeyUpdatedLine("consensus.timeout_commit", fmt.Sprintf("%q", provconfig.DefaultConsensusTimeoutCommit), `"950ms"`),
""),
},
{
Expand Down
4 changes: 2 additions & 2 deletions cmd/provenanced/cmd/pre_upgrade.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,9 @@ func UpdateConfig(cmd *cobra.Command) error {
}

if clientCfg.ChainID == "pio-mainnet-1" {
// Update the timeout commit if it's too low.
// Update the timeout commit.
timeoutCommit := config.DefaultConsensusTimeoutCommit
if tmCfg.Consensus.TimeoutCommit < timeoutCommit/2 {
if tmCfg.Consensus.TimeoutCommit != timeoutCommit {
cmd.Printf("Updating consensus.timeout_commit config value to %q (from %q)\n",
timeoutCommit, tmCfg.Consensus.TimeoutCommit)
tmCfg.Consensus.TimeoutCommit = timeoutCommit
Expand Down
113 changes: 80 additions & 33 deletions cmd/provenanced/cmd/pre_upgrade_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,9 +249,12 @@ func TestPreUpgradeCmd(t *testing.T) {
appCfgC := config.DefaultAppConfig()
appCfgC.API.Enable = true
appCfgC.API.Swagger = true
tmCfgC := config.DefaultTmConfig()
tmCfgC.Consensus.TimeoutCommit = 1 * time.Second
tmCfgC.LogLevel = "debug"
tmCfgLower := config.DefaultTmConfig()
tmCfgLower.Consensus.TimeoutCommit = tmCfgD.Consensus.TimeoutCommit - 500*time.Millisecond
tmCfgLower.LogLevel = "debug"
tmCfgHigher := config.DefaultTmConfig()
tmCfgHigher.Consensus.TimeoutCommit = tmCfgD.Consensus.TimeoutCommit + 500*time.Millisecond
tmCfgHigher.LogLevel = "debug"
clientCfgC := config.DefaultClientConfig()
clientCfgC.Output = "json"
clientCfgMainnetC := config.DefaultClientConfig()
Expand All @@ -261,9 +264,6 @@ func TestPreUpgradeCmd(t *testing.T) {
tmCfgCFixed := config.DefaultTmConfig()
tmCfgCFixed.LogLevel = "debug"

tmCfgNoChange := config.DefaultTmConfig()
tmCfgNoChange.Consensus.TimeoutCommit = 3 * time.Second

successMsg := "pre-upgrade successful"
updatingMsg := func(old time.Duration) string {
return fmt.Sprintf("Updating consensus.timeout_commit config value to %q (from %q)", config.DefaultConsensusTimeoutCommit, old)
Expand All @@ -276,6 +276,7 @@ func TestPreUpgradeCmd(t *testing.T) {
expExitCode int
expInStdout []string
expInStderr []string
expNot []string
expAppCfg *serverconfig.Config
expTmCfg *tmconfig.Config
expClientCfg *config.ClientConfig
Expand Down Expand Up @@ -325,29 +326,59 @@ func TestPreUpgradeCmd(t *testing.T) {
expClientCfg: clientCfgD,
},
{
name: "mainnet unpacked config with changes",
name: "mainnet unpacked config lower than default",
setup: func(t *testing.T) (string, func(), bool) {
home, success := newHome(t, "mainnet_unpacked_lower", appCfgC, tmCfgLower, clientCfgMainnetC)
return home, nil, success
},
expExitCode: 0,
expInStdout: []string{
updatingMsg(tmCfgLower.Consensus.TimeoutCommit),
successMsg,
},
expAppCfg: appCfgC,
expTmCfg: tmCfgCFixed,
expClientCfg: clientCfgMainnetC,
},
{
name: "mainnet unpacked config higher than default",
setup: func(t *testing.T) (string, func(), bool) {
home, success := newHome(t, "mainnet_unpacked_higher", appCfgC, tmCfgHigher, clientCfgMainnetC)
return home, nil, success
},
expExitCode: 0,
expInStdout: []string{
updatingMsg(tmCfgHigher.Consensus.TimeoutCommit),
successMsg,
},
expAppCfg: appCfgC,
expTmCfg: tmCfgCFixed,
expClientCfg: clientCfgMainnetC,
},
{
name: "mainnet packed config lower than default",
setup: func(t *testing.T) (string, func(), bool) {
home, success := newHome(t, "mainnet_changed_unpacked", appCfgC, tmCfgC, clientCfgMainnetC)
home, success := newHomePacked(t, "mainnet_packed_lower", appCfgC, tmCfgLower, clientCfgMainnetC)
return home, nil, success
},
expExitCode: 0,
expInStdout: []string{
updatingMsg(tmCfgC.Consensus.TimeoutCommit),
updatingMsg(tmCfgLower.Consensus.TimeoutCommit),
successMsg,
},
expAppCfg: appCfgC,
expTmCfg: tmCfgCFixed,
expClientCfg: clientCfgMainnetC,
},
{
name: "mainnet packed config with changes",
name: "mainnet packed config higher than default",
setup: func(t *testing.T) (string, func(), bool) {
home, success := newHomePacked(t, "mainnet_changed_packed", appCfgC, tmCfgC, clientCfgMainnetC)
home, success := newHomePacked(t, "mainnet_packed_higher", appCfgC, tmCfgHigher, clientCfgMainnetC)
return home, nil, success
},
expExitCode: 0,
expInStdout: []string{
updatingMsg(tmCfgC.Consensus.TimeoutCommit),
updatingMsg(tmCfgHigher.Consensus.TimeoutCommit),
successMsg,
},
expAppCfg: appCfgC,
Expand All @@ -357,7 +388,7 @@ func TestPreUpgradeCmd(t *testing.T) {
{
name: "mainnet cannot write new file",
setup: func(t *testing.T) (string, func(), bool) {
home, success := newHomePacked(t, "mainnet_cannot_write", appCfgC, tmCfgC, clientCfgMainnetC)
home, success := newHomePacked(t, "mainnet_cannot_write", appCfgC, tmCfgLower, clientCfgMainnetC)
if !success {
return home, nil, success
}
Expand All @@ -371,53 +402,65 @@ func TestPreUpgradeCmd(t *testing.T) {
return home, deferrable, success
},
expExitCode: 30,
expInStdout: []string{updatingMsg(tmCfgC.Consensus.TimeoutCommit)},
expInStdout: []string{updatingMsg(tmCfgLower.Consensus.TimeoutCommit)},
expInStderr: []string{"could not update config file(s):", "error saving config file(s):", "permission denied"},
},
{
name: "mainnet timeout commit low but not too low",
name: "other net unpacked config lower than default",
setup: func(t *testing.T) (string, func(), bool) {
home, success := newHome(t, "mainnet_different_not_changed", appCfgD, tmCfgNoChange, clientCfgD)
home, success := newHome(t, "other_unpacked_lower", appCfgC, tmCfgLower, clientCfgC)
return home, nil, success
},
expExitCode: 0,
expInStdout: []string{successMsg},
expAppCfg: appCfgD,
expTmCfg: tmCfgNoChange,
expClientCfg: clientCfgD,
expNot: []string{"Updating consensus.timeout_commit config value"},
expAppCfg: appCfgC,
expTmCfg: tmCfgLower,
expClientCfg: clientCfgC,
},
{
name: "other net unpacked config with changes",
name: "other net unpacked config higher than default",
setup: func(t *testing.T) (string, func(), bool) {
home, success := newHome(t, "other_changed_unpacked", appCfgC, tmCfgC, clientCfgC)
home, success := newHome(t, "other_unpacked_higher", appCfgC, tmCfgHigher, clientCfgC)
return home, nil, success
},
expExitCode: 0,
expInStdout: []string{
successMsg,
},
expExitCode: 0,
expInStdout: []string{successMsg},
expNot: []string{"Updating consensus.timeout_commit config value"},
expAppCfg: appCfgC,
expTmCfg: tmCfgC,
expTmCfg: tmCfgHigher,
expClientCfg: clientCfgC,
},
{
name: "other net packed config with changes",
name: "other net packed config lower than default",
setup: func(t *testing.T) (string, func(), bool) {
home, success := newHomePacked(t, "other_changed_packed", appCfgC, tmCfgC, clientCfgC)
home, success := newHomePacked(t, "other_packed_lower", appCfgC, tmCfgLower, clientCfgC)
return home, nil, success
},
expExitCode: 0,
expInStdout: []string{
successMsg,
expExitCode: 0,
expInStdout: []string{successMsg},
expNot: []string{"Updating consensus.timeout_commit config value"},
expAppCfg: appCfgC,
expTmCfg: tmCfgLower,
expClientCfg: clientCfgC,
},
{
name: "other net packed config higher than default",
setup: func(t *testing.T) (string, func(), bool) {
home, success := newHomePacked(t, "other_packed_higher", appCfgC, tmCfgHigher, clientCfgC)
return home, nil, success
},
expExitCode: 0,
expInStdout: []string{successMsg},
expNot: []string{"Updating consensus.timeout_commit config value"},
expAppCfg: appCfgC,
expTmCfg: tmCfgC,
expTmCfg: tmCfgHigher,
expClientCfg: clientCfgC,
},
{
name: "other net cannot write new file",
setup: func(t *testing.T) (string, func(), bool) {
home, success := newHomePacked(t, "other_cannot_write", appCfgC, tmCfgC, clientCfgC)
home, success := newHomePacked(t, "other_cannot_write", appCfgC, tmCfgLower, clientCfgC)
if !success {
return home, nil, success
}
Expand Down Expand Up @@ -456,6 +499,10 @@ func TestPreUpgradeCmd(t *testing.T) {
} else {
assertContainsAll(t, res.Stderr, tc.expInStderr, "stderr")
}
for _, unexp := range tc.expNot {
assert.NotContains(t, res.Stdout, unexp, "stdout")
assert.NotContains(t, res.Stderr, unexp, "stderr")
}

if res.ExitCode != 0 {
return
Expand Down
33 changes: 24 additions & 9 deletions cmd/provenanced/cmd/root.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"io"
"os"
"path/filepath"
"time"

"github.com/rs/zerolog"
"github.com/spf13/cast"
Expand Down Expand Up @@ -235,16 +236,9 @@ func txCommand() *cobra.Command {
}

func newApp(logger log.Logger, db dbm.DB, traceStore io.Writer, appOpts servertypes.AppOptions) servertypes.Application {
var cache sdk.MultiStorePersistentCache
warnAboutSettings(logger, appOpts)

chainID := cast.ToString(appOpts.Get(flags.FlagChainID))
if chainID == "pio-mainnet-1" {
timeoutCommit := cast.ToDuration(appOpts.Get("consensus.timeout_commit"))
if timeoutCommit < config.DefaultConsensusTimeoutCommit/2 {
logger.Error(fmt.Sprintf("Your consensus.timeout_commit config value is too low and should be increased to %q (it is currently %q).",
config.DefaultConsensusTimeoutCommit, timeoutCommit))
}
}
var cache sdk.MultiStorePersistentCache

if cast.ToBool(appOpts.Get(server.FlagInterBlockCache)) {
cache = store.NewCommitKVStoreCacheManager()
Expand Down Expand Up @@ -303,6 +297,27 @@ func newApp(logger log.Logger, db dbm.DB, traceStore io.Writer, appOpts serverty
)
}

// warnAboutSettings logs warnings about any settings that might cause problems.
func warnAboutSettings(logger log.Logger, appOpts servertypes.AppOptions) {
defer func() {
// If this func panics, just move on. It's just supposed to log things anyway.
_ = recover()
}()

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))
}
}
}
}

func createAppAndExport(
logger log.Logger,
db dbm.DB,
Expand Down
Loading

0 comments on commit cf436c0

Please sign in to comment.