feat!: account for time already elapsed when waiting after the commit (…

…#965)

* feat!: consider time already elapsed when waiting after the commit

* chore: minor doc changes

* chore: try a different default config time

* chore: try increasing the config again

* fix: use appropriate default time

* fix: docs

* chore: simplify addition and rename

* chore: revert pointless go mod tidy change

* chore: consistent config

* chore: replace config comments

* chore: fix a few remaining round -> height name changes

* fix: lingering compile errors from merge

* fix: silly bug

* fix: use correct next start time

* dynamic block time modifications (#983)

* chore: remove event collector

* Update test/maverick/consensus/state.go

Co-authored-by: Callum Waters <[email protected]>

* Update test/maverick/consensus/state.go

Co-authored-by: Callum Waters <[email protected]>

* chore: formatting

---------

Co-authored-by: Callum Waters <[email protected]>

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/^timeout_commit\s*=.*/timeout_commit = "500ms"/' \
+		-e 's/^target_height_duration\s*=.*/target_height_duration = "1000ms"/' \
 		-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/' \

config/config.go

@@ -961,11 +961,10 @@ 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"`
-	// 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"`
+	// 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"`
 
 	// Make progress as soon as we have all the precommits (as if TimeoutCommit = 0)
 	SkipTimeoutCommit bool `mapstructure:"skip_timeout_commit"`
@@ -986,13 +985,13 @@ func DefaultConsensusConfig() *ConsensusConfig {
 	return &ConsensusConfig{
 		OnlyInternalWal:             true,
 		WalPath:                     filepath.Join(defaultDataDir, "cs.wal", "wal"),
-		TimeoutPropose:              3000 * time.Millisecond,
+		TimeoutPropose:              2000 * time.Millisecond,
 		TimeoutProposeDelta:         500 * time.Millisecond,
 		TimeoutPrevote:              1000 * time.Millisecond,
 		TimeoutPrevoteDelta:         500 * time.Millisecond,
 		TimeoutPrecommit:            1000 * time.Millisecond,
 		TimeoutPrecommitDelta:       500 * time.Millisecond,
-		TimeoutCommit:               1000 * time.Millisecond,
+		TargetHeightDuration:        3000 * time.Millisecond,
 		SkipTimeoutCommit:           false,
 		CreateEmptyBlocks:           true,
 		CreateEmptyBlocksInterval:   0 * time.Second,
@@ -1012,7 +1011,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.TimeoutCommit = 10 * time.Millisecond
+	cfg.TargetHeightDuration = 70 * time.Millisecond
 	cfg.SkipTimeoutCommit = true
 	cfg.PeerGossipSleepDuration = 5 * time.Millisecond
 	cfg.PeerQueryMaj23SleepDuration = 250 * time.Millisecond
@@ -1046,10 +1045,14 @@ func (cfg *ConsensusConfig) Precommit(round int32) time.Duration {
 	) * time.Nanosecond
 }
 
-// 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)
+// NextStartTime adds the TargetHeightDuration to the provided starting time.
+func (cfg *ConsensusConfig) NextStartTime(t time.Time) time.Time {
+	newStartTime := t.Add(cfg.TargetHeightDuration)
+	now := time.Now()
+	if newStartTime.Before(now) {
+		return now
+	}
+	return newStartTime
 }
 
 // WalFile returns the full path to the write-ahead log file
@@ -1086,8 +1089,8 @@ func (cfg *ConsensusConfig) ValidateBasic() error {
 	if cfg.TimeoutPrecommitDelta < 0 {
 		return errors.New("timeout_precommit_delta can't be negative")
 	}
-	if cfg.TimeoutCommit < 0 {
-		return errors.New("timeout_commit can't be negative")
+	if cfg.TargetHeightDuration < 0 {
+		return errors.New("target_height_duration can't be negative")
 	}
 	if cfg.CreateEmptyBlocksInterval < 0 {
 		return errors.New("create_empty_blocks_interval can't be negative")

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},
-		"TimeoutCommit":                        {func(c *ConsensusConfig) { c.TimeoutCommit = time.Second }, false},
-		"TimeoutCommit negative":               {func(c *ConsensusConfig) { c.TimeoutCommit = -1 }, true},
+		"TargetHeightDuration":                 {func(c *ConsensusConfig) { c.TargetHeightDuration = time.Second }, false},
+		"TargetHeightDuration negative":        {func(c *ConsensusConfig) { c.TargetHeightDuration = -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},

config/toml.go

@@ -475,10 +475,10 @@ timeout_prevote_delta = "{{ .Consensus.TimeoutPrevoteDelta }}"
 timeout_precommit = "{{ .Consensus.TimeoutPrecommit }}"
 # How much the timeout_precommit increases with each round
 timeout_precommit_delta = "{{ .Consensus.TimeoutPrecommitDelta }}"
-# 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 }}"
+# 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 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

consensus/state.go

@@ -667,14 +667,11 @@ func (cs *State) updateToState(state sm.State) {
 	cs.updateRoundStep(0, cstypes.RoundStepNewHeight)
 
 	if cs.CommitTime.IsZero() {
-		// "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())
+		// If it is the first block, start time is equal to
+		// states last block time which is the genesis time.
+		cs.StartTime = state.LastBlockTime
 	} else {
-		cs.StartTime = cs.config.Commit(cs.CommitTime)
+		cs.StartTime = cs.config.NextStartTime(cs.StartTime)
 	}
 
 	cs.Validators = validators

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"
-# 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"
+# 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 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,12 +537,9 @@ timeout_prevote = "1s"
 timeout_prevote_delta = "500ms"
 timeout_precommit = "1s"
 timeout_precommit_delta = "500ms"
-timeout_commit = "1s"
+target_height_duration = "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
@@ -552,7 +549,8 @@ 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
-- `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)
+- `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.

state/execution.go

@@ -416,7 +416,7 @@ func execBlockOnProxyApp(
 		return nil, err
 	}
 
-	logger.Info("executed block", "height", block.Height, "num_valid_txs", validTxs, "num_invalid_txs", invalidTxs)
+	logger.Info("executed block", "height", block.Height, "num_valid_txs", validTxs, "num_invalid_txs", invalidTxs, "time", block.Time)
 	return abciResponses, nil
 }
 

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 = cs.config.Commit(cmttime.Now())
+		cs.StartTime = state.LastBlockTime
 	} else {
-		cs.StartTime = cs.config.Commit(cs.CommitTime)
+		cs.StartTime = cs.config.NextStartTime(cs.StartTime)
 	}
 
 	cs.Validators = validators