From d24c81afe0ecc4ccfbb7eb543a45a11794f04db4 Mon Sep 17 00:00:00 2001 From: Evan Forbes <42654277+evan-forbes@users.noreply.github.com> Date: Tue, 11 Jul 2023 11:27:05 -0500 Subject: [PATCH] Revert "feat!: account for time already elapsed when waiting after the commit (#965)" (#1033) This reverts commit cc1bc3fb98770a9a4b75451631941be4af18e0f5. --- DOCKER/docker-entrypoint.sh | 2 +- config/config.go | 33 ++++++++++++++------------------ config/config_test.go | 4 ++-- config/toml.go | 8 ++++---- consensus/state.go | 11 +++++++---- docs/core/configuration.md | 22 +++++++++++---------- state/execution.go | 2 +- test/maverick/consensus/state.go | 4 ++-- 8 files changed, 43 insertions(+), 43 deletions(-) diff --git a/DOCKER/docker-entrypoint.sh b/DOCKER/docker-entrypoint.sh index 12a1aeaf84..8d00c79994 100755 --- a/DOCKER/docker-entrypoint.sh +++ b/DOCKER/docker-entrypoint.sh @@ -9,7 +9,7 @@ if [ ! -d "$CMTHOME/config" ]; then -e "s/^proxy_app\s*=.*/proxy_app = \"$PROXY_APP\"/" \ -e "s/^moniker\s*=.*/moniker = \"$MONIKER\"/" \ -e 's/^addr_book_strict\s*=.*/addr_book_strict = false/' \ - -e 's/^target_height_duration\s*=.*/target_height_duration = "1000ms"/' \ + -e 's/^timeout_commit\s*=.*/timeout_commit = "500ms"/' \ -e 's/^index_all_tags\s*=.*/index_all_tags = true/' \ -e 's,^laddr = "tcp://127.0.0.1:26657",laddr = "tcp://0.0.0.0:26657",' \ -e 's/^prometheus\s*=.*/prometheus = true/' \ diff --git a/config/config.go b/config/config.go index 9d528169c3..649ae176f0 100644 --- a/config/config.go +++ b/config/config.go @@ -8,8 +8,6 @@ import ( "os" "path/filepath" "time" - - tmtime "github.com/tendermint/tendermint/types/time" ) const ( @@ -946,10 +944,11 @@ type ConsensusConfig struct { TimeoutPrecommit time.Duration `mapstructure:"timeout_precommit"` // How much the timeout_precommit increases with each round TimeoutPrecommitDelta time.Duration `mapstructure:"timeout_precommit_delta"` - // TargetHeigtDuration is used to determine how long we wait after a - // block is committed. If this time is shorter than the actual time to reach - // consensus for that height, then we do not wait at all. - TargetHeightDuration time.Duration `mapstructure:"target_height_duration"` + // How long we wait after committing a block, before starting on the new + // height (this gives us a chance to receive some more precommits, even + // though we already have +2/3). + // NOTE: when modifying, make sure to update time_iota_ms genesis parameter + TimeoutCommit time.Duration `mapstructure:"timeout_commit"` // Make progress as soon as we have all the precommits (as if TimeoutCommit = 0) SkipTimeoutCommit bool `mapstructure:"skip_timeout_commit"` @@ -969,13 +968,13 @@ type ConsensusConfig struct { func DefaultConsensusConfig() *ConsensusConfig { return &ConsensusConfig{ WalPath: filepath.Join(defaultDataDir, "cs.wal", "wal"), - TimeoutPropose: 2000 * time.Millisecond, + TimeoutPropose: 3000 * time.Millisecond, TimeoutProposeDelta: 500 * time.Millisecond, TimeoutPrevote: 1000 * time.Millisecond, TimeoutPrevoteDelta: 500 * time.Millisecond, TimeoutPrecommit: 1000 * time.Millisecond, TimeoutPrecommitDelta: 500 * time.Millisecond, - TargetHeightDuration: 3000 * time.Millisecond, + TimeoutCommit: 1000 * time.Millisecond, SkipTimeoutCommit: false, CreateEmptyBlocks: true, CreateEmptyBlocksInterval: 0 * time.Second, @@ -995,7 +994,7 @@ func TestConsensusConfig() *ConsensusConfig { cfg.TimeoutPrecommit = 10 * time.Millisecond cfg.TimeoutPrecommitDelta = 1 * time.Millisecond // NOTE: when modifying, make sure to update time_iota_ms (testGenesisFmt) in toml.go - cfg.TargetHeightDuration = 70 * time.Millisecond + cfg.TimeoutCommit = 10 * time.Millisecond cfg.SkipTimeoutCommit = true cfg.PeerGossipSleepDuration = 5 * time.Millisecond cfg.PeerQueryMaj23SleepDuration = 250 * time.Millisecond @@ -1029,14 +1028,10 @@ func (cfg *ConsensusConfig) Precommit(round int32) time.Duration { ) * time.Nanosecond } -// NextStartTime adds the TargetHeightDuration to the provided starting time. -func (cfg *ConsensusConfig) NextStartTime(t time.Time) time.Time { - newStartTime := t.Add(cfg.TargetHeightDuration) - now := tmtime.Now() - if newStartTime.Before(now) { - return now - } - return newStartTime +// Commit returns the amount of time to wait for straggler votes after receiving +2/3 precommits +// for a single block (ie. a commit). +func (cfg *ConsensusConfig) Commit(t time.Time) time.Time { + return t.Add(cfg.TimeoutCommit) } // WalFile returns the full path to the write-ahead log file @@ -1073,8 +1068,8 @@ func (cfg *ConsensusConfig) ValidateBasic() error { if cfg.TimeoutPrecommitDelta < 0 { return errors.New("timeout_precommit_delta can't be negative") } - if cfg.TargetHeightDuration < 0 { - return errors.New("target_height_duration can't be negative") + if cfg.TimeoutCommit < 0 { + return errors.New("timeout_commit can't be negative") } if cfg.CreateEmptyBlocksInterval < 0 { return errors.New("create_empty_blocks_interval can't be negative") diff --git a/config/config_test.go b/config/config_test.go index ab3212b509..43f0a46c49 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -157,8 +157,8 @@ func TestConsensusConfig_ValidateBasic(t *testing.T) { "TimeoutPrecommit negative": {func(c *ConsensusConfig) { c.TimeoutPrecommit = -1 }, true}, "TimeoutPrecommitDelta": {func(c *ConsensusConfig) { c.TimeoutPrecommitDelta = time.Second }, false}, "TimeoutPrecommitDelta negative": {func(c *ConsensusConfig) { c.TimeoutPrecommitDelta = -1 }, true}, - "TargetHeightDuration": {func(c *ConsensusConfig) { c.TargetHeightDuration = time.Second }, false}, - "TargetHeightDuration negative": {func(c *ConsensusConfig) { c.TargetHeightDuration = -1 }, true}, + "TimeoutCommit": {func(c *ConsensusConfig) { c.TimeoutCommit = time.Second }, false}, + "TimeoutCommit negative": {func(c *ConsensusConfig) { c.TimeoutCommit = -1 }, true}, "PeerGossipSleepDuration": {func(c *ConsensusConfig) { c.PeerGossipSleepDuration = time.Second }, false}, "PeerGossipSleepDuration negative": {func(c *ConsensusConfig) { c.PeerGossipSleepDuration = -1 }, true}, "PeerQueryMaj23SleepDuration": {func(c *ConsensusConfig) { c.PeerQueryMaj23SleepDuration = time.Second }, false}, diff --git a/config/toml.go b/config/toml.go index 7c50608903..9f45f0b636 100644 --- a/config/toml.go +++ b/config/toml.go @@ -464,10 +464,10 @@ timeout_prevote_delta = "{{ .Consensus.TimeoutPrevoteDelta }}" timeout_precommit = "{{ .Consensus.TimeoutPrecommit }}" # How much the timeout_precommit increases with each round timeout_precommit_delta = "{{ .Consensus.TimeoutPrecommitDelta }}" -# TargetHeigtDuration is used to determine how long we wait after a -# block is committed. If this time is shorter than the actual time to reach -# consensus for that height, then we do not wait at all. -target_height_duration = "{{ .Consensus.TargetHeightDuration }}" +# How long we wait after committing a block, before starting on the new +# height (this gives us a chance to receive some more precommits, even +# though we already have +2/3). +timeout_commit = "{{ .Consensus.TimeoutCommit }}" # How many blocks to look back to check existence of the node's consensus votes before joining consensus # When non-zero, the node will panic upon restart diff --git a/consensus/state.go b/consensus/state.go index b9e90e2103..4b8fd63a37 100644 --- a/consensus/state.go +++ b/consensus/state.go @@ -664,11 +664,14 @@ func (cs *State) updateToState(state sm.State) { cs.updateRoundStep(0, cstypes.RoundStepNewHeight) if cs.CommitTime.IsZero() { - // If it is the first block, start time is equal to - // states last block time which is the genesis time. - cs.StartTime = state.LastBlockTime + // "Now" makes it easier to sync up dev nodes. + // We add timeoutCommit to allow transactions + // to be gathered for the first block. + // And alternative solution that relies on clocks: + // cs.StartTime = state.LastBlockTime.Add(timeoutCommit) + cs.StartTime = cs.config.Commit(cmttime.Now()) } else { - cs.StartTime = cs.config.NextStartTime(cs.StartTime) + cs.StartTime = cs.config.Commit(cs.CommitTime) } cs.Validators = validators diff --git a/docs/core/configuration.md b/docs/core/configuration.md index 3c2cffdbe1..dab0c97466 100644 --- a/docs/core/configuration.md +++ b/docs/core/configuration.md @@ -414,10 +414,10 @@ timeout_prevote_delta = "500ms" timeout_precommit = "1s" # How much the timeout_precommit increases with each round timeout_precommit_delta = "500ms" -# TargetHeigtDuration is used to determine how long we wait after a -# block is committed. If this time is shorter than the actual time to reach -# consensus for that height, then we do not wait at all. -target_height_duration = "15s" +# How long we wait after committing a block, before starting on the new +# height (this gives us a chance to receive some more precommits, even +# though we already have +2/3). +timeout_commit = "1s" # How many blocks to look back to check existence of the node's consensus votes before joining consensus # When non-zero, the node will panic upon restart @@ -537,9 +537,12 @@ timeout_prevote = "1s" timeout_prevote_delta = "500ms" timeout_precommit = "1s" timeout_precommit_delta = "500ms" -target_height_duration = "1s" +timeout_commit = "1s" ``` +Note that in a successful round, the only timeout that we absolutely wait no +matter what is `timeout_commit`. + Here's a brief summary of the timeouts: - `timeout_propose` = how long we wait for a proposal block before prevoting nil @@ -549,8 +552,7 @@ Here's a brief summary of the timeouts: - `timeout_prevote_delta` = how much the `timeout_prevote` increases with each round - `timeout_precommit` = how long we wait after receiving +2/3 precommits for anything (ie. not a single block or nil) -- `timeout_precommit_delta` = how much the timeout_precommit increases with - each round -- `target_height_duration` = used to determine how long we wait after a - block is committed. If this time is shorter than the actual time to reach - consensus for that height, then we do not wait at all. +- `timeout_precommit_delta` = how much the `timeout_precommit` increases with each round +- `timeout_commit` = how long we wait after committing a block, before starting + on the new height (this gives us a chance to receive some more precommits, + even though we already have +2/3) diff --git a/state/execution.go b/state/execution.go index 4818d4fba8..7b704f8e7f 100644 --- a/state/execution.go +++ b/state/execution.go @@ -407,7 +407,7 @@ func execBlockOnProxyApp( return nil, err } - logger.Info("executed block", "height", block.Height, "num_valid_txs", validTxs, "num_invalid_txs", invalidTxs, "time", block.Time) + logger.Info("executed block", "height", block.Height, "num_valid_txs", validTxs, "num_invalid_txs", invalidTxs) return abciResponses, nil } diff --git a/test/maverick/consensus/state.go b/test/maverick/consensus/state.go index ae9ebeadee..f73e4a14c6 100644 --- a/test/maverick/consensus/state.go +++ b/test/maverick/consensus/state.go @@ -900,9 +900,9 @@ func (cs *State) updateToState(state sm.State) { // to be gathered for the first block. // And alternative solution that relies on clocks: // cs.StartTime = state.LastBlockTime.Add(timeoutCommit) - cs.StartTime = state.LastBlockTime + cs.StartTime = cs.config.Commit(cmttime.Now()) } else { - cs.StartTime = cs.config.NextStartTime(cs.StartTime) + cs.StartTime = cs.config.Commit(cs.CommitTime) } cs.Validators = validators