feat(abci): move timeout_commit into FinalizeBlockResponse (comet…

…bft#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]>
dydxprotocol · Jun 19, 2024 · a84d857 · a84d857
1 parent c8beeea
commit a84d857
Show file tree

Hide file tree

Showing 17 changed files with 223 additions and 53 deletions.
diff --git a/.changelog/unreleased/features/2655-move-timeout-commit-into-finalize-block.md b/.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))
diff --git a/abci/types/application.go b/abci/types/application.go
@@ -1,6 +1,8 @@
 package types
 
-import "context"
+import (
+	"context"
+)
 
 //go:generate ../../scripts/mockery_generate.sh Application
 

diff --git a/abci/types/types.pb.go b/abci/types/types.pb.go
diff --git a/config/config.go b/config/config.go
@@ -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)
 }

diff --git a/consensus/reactor_test.go b/consensus/reactor_test.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)

diff --git a/consensus/state.go b/consensus/state.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 {

diff --git a/consensus/state_test.go b/consensus/state_test.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

diff --git a/docs/app-dev/indexing-transactions.md b/docs/app-dev/indexing-transactions.md
@@ -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{
 			{

diff --git a/docs/guides/go-built-in.md b/docs/guides/go-built-in.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.

diff --git a/docs/guides/go.md b/docs/guides/go.md
@@ -379,6 +379,8 @@ func (app *KVStoreApplication) FinalizeBlock(_ context.Context, req *abcitypes.R
 }
 ```
 
+`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. The application state may have changed between the initial execution of `CheckTx` and the transaction delivery in `FinalizeBlock` in a way that rendered the transaction no longer valid.

diff --git a/proto/tendermint/abci/types.proto b/proto/tendermint/abci/types.proto
@@ -11,6 +11,7 @@ import "tendermint/types/params.proto";
 import "tendermint/types/validator.proto";
 import "google/protobuf/timestamp.proto";
 import "gogoproto/gogo.proto";
+import "google/protobuf/duration.proto";
 
 // NOTE: When using custom types, mind the warnings.
 // https://github.com/cosmos/gogoproto/blob/master/custom_types.md#warnings-and-issues
@@ -363,6 +364,12 @@ message ResponseFinalizeBlock {
   tendermint.types.ConsensusParams consensus_param_updates = 4;
   // app_hash is the hash of the applications' state which is used to confirm that execution of the transactions was deterministic. It is up to the application to decide which algorithm to use.
   bytes app_hash = 5;
+  // delay between the time when this block is committed and the next height is started.
+  // previously `timeout_commit` in config.toml
+  google.protobuf.Duration next_block_delay = 6 [
+    (gogoproto.nullable) = false,
+    (gogoproto.stdduration) = true
+  ];
 }
 
 //----------------------------------------