feat(abci): move timeout_commit into FinalizeBlockResponse (cometbft#3089)

as `next_block_delay`

ADR-115: cometbft#2966
Closes cometbft#2655

---

- [ ] ~~Tests written/updated~~
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [x] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

---------

Co-authored-by: Sergio Mena <[email protected]>

Author: 2 people

.changelog/unreleased/features/2655-move-timeout-commit-into-finalize-block.md

@@ -0,0 +1,2 @@
+- `[config]` Move `timeout_commit` into the ABCI `FinalizeBlockResponse`
+  ([\#2655](https://github.com/cometbft/cometbft/issues/2655))

abci/types/application.go

@@ -1,6 +1,8 @@
 package types
 
-import "context"
+import (
+	"context"
+)
 
 //go:generate ../../scripts/mockery_generate.sh Application
 

abci/types/types.pb.go

@@ -1,3 +1,4 @@
+version: v1
 build:
   roots:
     - proto

buf.yaml

@@ -158,9 +158,12 @@ func (cfg *Config) ValidateBasic() error {
 	return nil
 }
 
-// CheckDeprecated returns any deprecation warnings. These are printed to the operator on startup
+// CheckDeprecated returns any deprecation warnings. These are printed to the operator on startup.
 func (cfg *Config) CheckDeprecated() []string {
 	var warnings []string
+	if cfg.Consensus.TimeoutCommit != 0 {
+		warnings = append(warnings, "[consensus.timeout_commit] is deprecated. Use `next_block_delay` in the ABCI `FinalizeBlockResponse`.")
+	}
 	return warnings
 }
 
@@ -996,6 +999,7 @@ type ConsensusConfig struct {
 	// 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
+	// Deprecated: use `next_block_delay` in the ABCI application's `FinalizeBlockResponse`.
 	TimeoutCommit time.Duration `mapstructure:"timeout_commit"`
 
 	// Make progress as soon as we have all the precommits (as if TimeoutCommit = 0)
@@ -1078,6 +1082,7 @@ func (cfg *ConsensusConfig) Precommit(round int32) time.Duration {
 
 // Commit returns the amount of time to wait for straggler votes after receiving +2/3 precommits
 // for a single block (ie. a commit).
+// Deprecated: use `next_block_delay` in the ABCI application's `FinalizeBlockResponse`.
 func (cfg *ConsensusConfig) Commit(t time.Time) time.Time {
 	return t.Add(cfg.TimeoutCommit)
 }

config/config.go

@@ -637,9 +637,9 @@ func TestReactorWithTimeoutCommit(t *testing.T) {
 	N := 4
 	css, cleanup := randConsensusNet(t, N, "consensus_reactor_with_timeout_commit_test", newMockTickerFunc(false), newKVStore)
 	defer cleanup()
-	// override default SkipTimeoutCommit == true for tests
+	// override default NextBlockDelay == 0 for tests
 	for i := 0; i < N; i++ {
-		css[i].config.SkipTimeoutCommit = false
+		css[i].state.NextBlockDelay = 1 * time.Second
 	}
 
 	reactors, blocksSubs, eventBuses := startConsensusNet(t, css, N-1)

consensus/reactor_test.go

@@ -718,15 +718,20 @@ func (cs *State) updateToState(state sm.State) {
 	cs.updateHeight(height)
 	cs.updateRoundStep(0, cstypes.RoundStepNewHeight)
 
+	timeoutCommit := state.NextBlockDelay
+	// If the ABCI app didn't set a delay, use the deprecated config value.
+	if timeoutCommit == 0 {
+		timeoutCommit = cs.config.TimeoutCommit //nolint:staticcheck
+	}
 	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())
+		//
+		// We add timeoutCommit to allow transactions to be gathered for
+		// the first block. An alternative solution that relies on clocks:
+		// `cs.StartTime = state.LastBlockTime.Add(timeoutCommit)`
+		cs.StartTime = cmttime.Now().Add(timeoutCommit)
 	} else {
-		cs.StartTime = cs.config.Commit(cs.CommitTime)
+		cs.StartTime = cs.CommitTime.Add(timeoutCommit)
 	}
 
 	cs.Validators = validators
@@ -1042,7 +1047,7 @@ func (cs *State) handleTxsAvailable() {
 
 // Enter: `timeoutNewHeight` by startTime (commitTime+timeoutCommit),
 //
-//	or, if SkipTimeoutCommit==true, after receiving all precommits from (height,round-1)
+//	or, if NextBlockDelay==0, after receiving all precommits from (height,round-1)
 //
 // Enter: `timeoutPrecommits` after any +2/3 precommits from (height,round-1)
 // Enter: +2/3 precommits for nil at (height,round-1)
@@ -2211,7 +2216,8 @@ func (cs *State) addVote(vote *types.Vote, peerID p2p.ID) (added bool, err error
 		cs.evsw.FireEvent(types.EventVote, vote)
 
 		// if we can skip timeoutCommit and have all the votes now,
-		if cs.config.SkipTimeoutCommit && cs.LastCommit.HasAll() {
+		skipTimeoutCommit := cs.state.NextBlockDelay == 0 && cs.config.TimeoutCommit == 0 //nolint:staticcheck
+		if skipTimeoutCommit && cs.LastCommit.HasAll() {
 			// go straight to new round (skip timeout commit)
 			// cs.scheduleTimeout(time.Duration(0), cs.Height, 0, cstypes.RoundStepNewHeight)
 			cs.enterNewRound(cs.Height, 0)
@@ -2369,7 +2375,8 @@ func (cs *State) addVote(vote *types.Vote, peerID p2p.ID) (added bool, err error
 
 			if !blockID.IsNil() {
 				cs.enterCommit(height, vote.Round)
-				if cs.config.SkipTimeoutCommit && precommits.HasAll() {
+				skipTimeoutCommit := cs.state.NextBlockDelay == 0 && cs.config.TimeoutCommit == 0 //nolint:staticcheck
+				if skipTimeoutCommit && precommits.HasAll() {
 					cs.enterNewRound(cs.Height, 0)
 				}
 			} else {

consensus/state.go

@@ -2761,6 +2761,7 @@ func (n *fakeTxNotifier) Notify() {
 func TestStartNextHeightCorrectlyAfterTimeout(t *testing.T) {
 	config.Consensus.SkipTimeoutCommit = false
 	cs1, vss := randState(4)
+	cs1.state.NextBlockDelay = 10 * time.Millisecond
 	cs1.txNotifier = &fakeTxNotifier{ch: make(chan struct{})}
 
 	vs2, vs3, vs4 := vss[1], vss[2], vss[3]
@@ -2825,6 +2826,7 @@ func TestResetTimeoutPrecommitUponNewHeight(t *testing.T) {
 
 	config.Consensus.SkipTimeoutCommit = false
 	cs1, vss := randState(4)
+	cs1.state.NextBlockDelay = 10 * time.Millisecond
 
 	vs2, vs3, vs4 := vss[1], vss[2], vss[3]
 	height, round := cs1.Height, cs1.Round

consensus/state_test.go

@@ -118,7 +118,6 @@ transferBalance200FinalizeBlock12          1
 transferNodeNothingFinalizeBlock12         1
 
 ```
-
 The event number is a local variable kept by the indexer and incremented when a new event is processed.
 It is an `int64` variable and has no other semantics besides being used to associate attributes belonging to the same events within a height.
 This variable is not atomically incremented as event indexing is deterministic. **Should this ever change**, the event id generation
@@ -174,32 +173,30 @@ Example:
 
 ```go
 func (app *Application) FinalizeBlock(_ context.Context, req *types.RequestFinalizeBlock) (*types.ResponseFinalizeBlock, error) {
-
-    //...
-  tx_results[0] := &types.ExecTxResult{
-			Code: CodeTypeOK,
-			// With every transaction we can emit a series of events. To make it simple, we just emit the same events.
-			Events: []types.Event{
-				{
-					Type: "app",
-					Attributes: []types.EventAttribute{
-						{Key: "creator", Value: "Cosmoshi Netowoko", Index: true},
-						{Key: "key", Value: key, Index: true},
-						{Key: "index_key", Value: "index is working", Index: true},
-						{Key: "noindex_key", Value: "index is working", Index: false},
-					},
-				},
-				{
-					Type: "app",
-					Attributes: []types.EventAttribute{
-						{Key: "creator", Value: "Cosmoshi", Index: true},
-						{Key: "key", Value: value, Index: true},
-						{Key: "index_key", Value: "index is working", Index: true},
-						{Key: "noindex_key", Value: "index is working", Index: false},
-					},
-				},
-			},
-		}
+    tx_results[0] := &types.ExecTxResult{
+        Code: CodeTypeOK,
+        // With every transaction we can emit a series of events. To make it simple, we just emit the same events.
+        Events: []types.Event{
+            {
+                Type: "app",
+                Attributes: []types.EventAttribute{
+                    {Key: "creator", Value: "Cosmoshi Netowoko", Index: true},
+                    {Key: "key", Value: key, Index: true},
+                    {Key: "index_key", Value: "index is working", Index: true},
+                    {Key: "noindex_key", Value: "index is working", Index: false},
+                },
+            },
+            {
+                Type: "app",
+                Attributes: []types.EventAttribute{
+                    {Key: "creator", Value: "Cosmoshi", Index: true},
+                    {Key: "key", Value: value, Index: true},
+                    {Key: "index_key", Value: "index is working", Index: true},
+                    {Key: "noindex_key", Value: "index is working", Index: false},
+                },
+            },
+        },
+    }
 
     block_events = []types.Event{
 			{

docs/app-dev/indexing-transactions.md

@@ -375,10 +375,13 @@ func (app *KVStoreApplication) FinalizeBlock(_ context.Context, req *abcitypes.R
 
     return &abcitypes.ResponseFinalizeBlock{
       TxResults:        txs,
+      NextBlockDelay:   1 * time.Second,
     }, nil
 }
 ```
 
+`NextBlockDelay` is a delay between the time when the current block is committed and the next height is started. Normally you don't need to change the default value (1s). Please refer to the [spec](../../spec/abci/abci++_methods.md#finalizeblock) for more information.
+
 Transactions are not guaranteed to be valid when they are delivered to an application, even if they were valid when they were proposed.
 
 This can happen if the application state is used to determine transaction validity.