Fix: panic may cause inconsistent heights between app and dymint store (

#232)

* Fixed bug where if the applyBlock would crash in the middle it could cause height incosistencies between app and state/store.

* Removed redundant tests.

* Updated applyBlock function comment.

block/manager.go

@@ -16,6 +16,7 @@ import (
 	tmcrypto "github.com/tendermint/tendermint/crypto"
 	"github.com/tendermint/tendermint/crypto/merkle"
 	"github.com/tendermint/tendermint/libs/pubsub"
+	tmstate "github.com/tendermint/tendermint/proto/tendermint/state"
 	"github.com/tendermint/tendermint/proxy"
 	tmtypes "github.com/tendermint/tendermint/types"
 
@@ -385,53 +386,57 @@ func (m *Manager) ApplyBlockLoop(ctx context.Context) {
 	}
 }
 
+// applyBlock applies the block to the store and the abci app.
+// steps: save block -> execute block with app -> update state -> commit block to app -> update store height and state hash.
+// As the entire process can't be atomic we need to make sure the following condition apply before
+// we're applying the block in the happy path: block height - 1 == abci app last block height.
+// In case the following doesn't hold true, it means we crashed after the commit and before updating the store height.
+// In that case we'll want to align the store with the app state and continue to the next block.
 func (m *Manager) applyBlock(ctx context.Context, block *types.Block, commit *types.Commit, blockMetaData blockMetaData) error {
 	if block.Header.Height == m.store.Height()+1 {
 		m.logger.Info("Applying block", "height", block.Header.Height, "source", blockMetaData.source)
 
-		_, err := m.store.SaveBlock(block, commit, nil)
+		// Check if alignment is needed due to incosistencies between the store and the app.
+		isAlignRequired, err := m.alignStoreWithApp(ctx, block)
 		if err != nil {
-			m.logger.Error("failed to save block", "error", err)
+			return err
+		}
+		if isAlignRequired {
+			m.logger.Info("Aligned with app state required. Skipping to next block", "height", block.Header.Height)
+			return nil
+		}
+		// Start applying the block assuming no inconsistency was found.
+		_, err = m.store.SaveBlock(block, commit, nil)
+		if err != nil {
+			m.logger.Error("Failed to save block", "error", err)
 			return err
 		}
 
-		// Currently we're assuming proposer is never nil as it's a pre-condition for
-		// dymint to start
-		proposer := m.settlementClient.GetProposer()
-		// Apply the block but DONT commit
-		newState, responses, err := m.executor.ApplyBlock(ctx, m.lastState, block, commit, proposer)
+		responses, err := m.executeBlock(ctx, block, commit)
 		if err != nil {
-			m.logger.Error("failed to ApplyBlock", "error", err)
+			m.logger.Error("Failed to execute block", "error", err)
 			return err
 		}
 
-		// Commit the new state and block which writes to disk on the proxy app
-		err = m.executor.Commit(ctx, &newState, block, responses)
+		newState, err := m.executor.UpdateStateFromResponses(responses, m.lastState, block)
 		if err != nil {
-			m.logger.Error("failed to Commit to the block", "error", err)
 			return err
 		}
 
 		batch := m.store.NewBatch()
 
-		// SaveBlockResponses commits the DB tx
 		batch, err = m.store.SaveBlockResponses(block.Header.Height, responses, batch)
 		if err != nil {
 			batch.Discard()
 			return err
 		}
 
-		// After this call m.lastState is the NEW state returned from ApplyBlock
 		m.lastState = newState
-
-		// UpdateState commits the DB tx
 		batch, err = m.store.UpdateState(m.lastState, batch)
 		if err != nil {
 			batch.Discard()
 			return err
 		}
-
-		// SaveValidators commits the DB tx
 		batch, err = m.store.SaveValidators(block.Header.Height, m.lastState.Validators, batch)
 		if err != nil {
 			batch.Discard()
@@ -440,17 +445,73 @@ func (m *Manager) applyBlock(ctx context.Context, block *types.Block, commit *ty
 
 		err = batch.Commit()
 		if err != nil {
-			m.logger.Error("failed to persist batch to disk", "error", err)
+			m.logger.Error("Failed to persist batch to disk", "error", err)
 			return err
 		}
 
-		// Only update the stored height after successfully committing to the DB
+		// Commit block to app
+		err = m.executor.Commit(ctx, &newState, block, responses)
+		if err != nil {
+			m.logger.Error("Failed to commit to the block", "error", err)
+			return err
+		}
+
+		// Update the state with the new app hash from the commit.
+		_, err = m.store.UpdateState(newState, nil)
+		if err != nil {
+			m.logger.Error("Failed to update state", "error", err)
+			return err
+		}
+		m.lastState = newState
+
 		m.store.SetHeight(block.Header.Height)
 
 	}
 	return nil
 }
 
+// alignStoreWithApp is responsible for aligning the state of the store and the abci app if necessary.
+func (m *Manager) alignStoreWithApp(ctx context.Context, block *types.Block) (bool, error) {
+	isRequired := false
+	// Validate incosistency in height wasn't caused by a crash and if so handle it.
+	proxyAppInfo, err := m.executor.GetAppInfo()
+	if err != nil {
+		m.logger.Error("Failed to get app info", "error", err)
+		return isRequired, err
+	}
+	if uint64(proxyAppInfo.LastBlockHeight) == block.Header.Height {
+		isRequired = true
+		m.logger.Info("Skipping block application and only updating store height and state hash", "height", block.Header.Height)
+		// update the state with the hash
+		m.lastState.AppHash = *(*[32]byte)(proxyAppInfo.LastBlockAppHash)
+		_, err := m.store.UpdateState(m.lastState, nil)
+		if err != nil {
+			m.logger.Error("Failed to update state", "error", err)
+			return isRequired, err
+		}
+		m.store.SetHeight(block.Header.Height)
+		return isRequired, nil
+	}
+	return isRequired, nil
+}
+
+func (m *Manager) executeBlock(ctx context.Context, block *types.Block, commit *types.Commit) (*tmstate.ABCIResponses, error) {
+	// Currently we're assuming proposer is never nil as it's a pre-condition for
+	// dymint to start
+	proposer := m.settlementClient.GetProposer()
+
+	if err := m.executor.Validate(m.lastState, block, commit, proposer); err != nil {
+		return &tmstate.ABCIResponses{}, err
+	}
+
+	responses, err := m.executor.Execute(ctx, m.lastState, block)
+	if err != nil {
+		return &tmstate.ABCIResponses{}, err
+	}
+
+	return responses, nil
+}
+
 func (m *Manager) gossipBlock(ctx context.Context, block types.Block, commit types.Commit) error {
 	gossipedBlock := p2p.GossipedBlock{Block: block, Commit: commit}
 	gossipedBlockBytes, err := gossipedBlock.MarshalBinary()