Merge pull request #13 from osmosis-labs/adam/no-double-save-finalize…

…BlockResponse

perf(state/store): avoid double-saving ABCI responses

.changelog/unreleased/improvements/2017-state-avoid-double-saving-finalize-block-response.md

@@ -0,0 +1,2 @@
+- `[state]` avoid double-saving `FinalizeBlockResponse` for performance reasons
+  ([\#2017](https://github.com/cometbft/cometbft/pull/2017))

state/store.go

@@ -41,8 +41,10 @@ func calcABCIResponsesKey(height int64) []byte {
 
 //----------------------
 
-var lastABCIResponseKey = []byte("lastABCIResponseKey")
-var offlineStateSyncHeight = []byte("offlineStateSyncHeightKey")
+var (
+	lastABCIResponseKey    = []byte("lastABCIResponseKey") // DEPRECATED
+	offlineStateSyncHeight = []byte("offlineStateSyncHeightKey")
+)
 
 //go:generate ../scripts/mockery_generate.sh Store
 
@@ -399,16 +401,16 @@ func (store dbStore) LoadABCIResponses(height int64) (*cmtstate.ABCIResponses, e
 		return nil, ErrNoABCIResponsesForHeight{height}
 	}
 
-	abciResponses := new(cmtstate.ABCIResponses)
-	err = abciResponses.Unmarshal(buf)
+	resp := new(cmtstate.ABCIResponses)
+	err = resp.Unmarshal(buf)
 	if err != nil {
 		// DATA HAS BEEN CORRUPTED OR THE SPEC HAS CHANGED
 		cmtos.Exit(fmt.Sprintf(`LoadABCIResponses: Data has been corrupted or its spec has
                 changed: %v\n`, err))
 	}
 	// TODO: ensure that buf is completely read.
 
-	return abciResponses, nil
+	return resp, nil
 }
 
 // LoadLastABCIResponses loads the ABCIResponses from the most recent height.
@@ -418,28 +420,36 @@ func (store dbStore) LoadABCIResponses(height int64) (*cmtstate.ABCIResponses, e
 // This method is used for recovering in the case that we called the Commit ABCI
 // method on the application but crashed before persisting the results.
 func (store dbStore) LoadLastABCIResponse(height int64) (*cmtstate.ABCIResponses, error) {
-	bz, err := store.db.Get(lastABCIResponseKey)
+	buf, err := store.db.Get(calcABCIResponsesKey(height))
 	if err != nil {
 		return nil, err
 	}
-
-	if len(bz) == 0 {
-		return nil, errors.New("no last ABCI response has been persisted")
+	if len(buf) == 0 {
+		// DEPRECATED lastABCIResponseKey
+		// It is possible if this is called directly after an upgrade that
+		// `lastABCIResponseKey` contains the last ABCI responses.
+		bz, err := store.db.Get(lastABCIResponseKey)
+		if err == nil && len(bz) > 0 {
+			info := new(cmtstate.ABCIResponsesInfo)
+			err = info.Unmarshal(bz)
+			if err != nil {
+				cmtos.Exit(fmt.Sprintf(`LoadLastABCIResponses: Data has been corrupted or its spec has changed: %v\n`, err))
+			}
+			// Here we validate the result by comparing its height to the expected height.
+			if height != info.GetHeight() {
+				return nil, fmt.Errorf("expected height %d but last stored abci responses was at height %d", height, info.GetHeight())
+			}
+			return info.AbciResponses, nil
+		}
+		// END OF DEPRECATED lastABCIResponseKey
+		return nil, fmt.Errorf("expected last ABCI responses at height %d, but none are found", height)
 	}
-
-	abciResponse := new(cmtstate.ABCIResponsesInfo)
-	err = abciResponse.Unmarshal(bz)
+	resp := new(cmtstate.ABCIResponses)
+	err = resp.Unmarshal(buf)
 	if err != nil {
-		cmtos.Exit(fmt.Sprintf(`LoadLastABCIResponses: Data has been corrupted or its spec has
-			changed: %v\n`, err))
+		cmtos.Exit(fmt.Sprintf(`LoadLastABCIResponses: Data has been corrupted or its spec has changed: %v\n`, err))
 	}
-
-	// Here we validate the result by comparing its height to the expected height.
-	if height != abciResponse.GetHeight() {
-		return nil, errors.New("expected height %d but last stored abci responses was at height %d")
-	}
-
-	return abciResponse.AbciResponses, nil
+	return resp, nil
 }
 
 // SaveABCIResponses persists the ABCIResponses to the database.
@@ -458,30 +468,24 @@ func (store dbStore) SaveABCIResponses(height int64, abciResponses *cmtstate.ABC
 	}
 	abciResponses.DeliverTxs = dtxs
 
-	// If the flag is false then we save the ABCIResponse. This can be used for the /BlockResults
-	// query or to reindex an event using the command line.
-	if !store.DiscardABCIResponses {
-		bz, err := abciResponses.Marshal()
-		if err != nil {
-			return err
-		}
-		if err := store.db.Set(calcABCIResponsesKey(height), bz); err != nil {
-			return err
-		}
+	bz, err := abciResponses.Marshal()
+	if err != nil {
+		return err
 	}
 
+	// Save the ABCI response.
+	//
 	// We always save the last ABCI response for crash recovery.
-	// This overwrites the previous saved ABCI Response.
-	response := &cmtstate.ABCIResponsesInfo{
-		AbciResponses: abciResponses,
-		Height:        height,
+	// If `store.DiscardABCIResponses` is true, then we delete the previous ABCI response.
+	if store.DiscardABCIResponses && height > 1 {
+		if err := store.db.Delete(calcABCIResponsesKey(height - 1)); err != nil {
+			return err
+		}
 	}
-	bz, err := response.Marshal()
-	if err != nil {
+	if err := store.db.SetSync(calcABCIResponsesKey(height), bz); err != nil {
 		return err
 	}
-
-	return store.db.SetSync(lastABCIResponseKey, bz)
+	return nil
 }
 
 //-----------------------------------------------------------------------------