consensus: avoid panics during handshake (#8266)

There's no case where we recieve an error during handshake and don't
just return/continue, and it's at a point during startup where not
much is going on in the process, so having some classes of errors
return errors and some return panics is confusing and doesn't protect
anything.

internal/consensus/replay.go

@@ -220,7 +220,6 @@ func NewHandshaker(
 	eventBus *eventbus.EventBus,
 	genDoc *types.GenesisDoc,
 ) *Handshaker {
-
 	return &Handshaker{
 		stateStore:   stateStore,
 		initialState: state,
@@ -228,7 +227,6 @@ func NewHandshaker(
 		eventBus:     eventBus,
 		genDoc:       genDoc,
 		logger:       logger,
-		nBlocks:      0,
 	}
 }
 
@@ -359,7 +357,9 @@ func (h *Handshaker) ReplayBlocks(
 	// First handle edge cases and constraints on the storeBlockHeight and storeBlockBase.
 	switch {
 	case storeBlockHeight == 0:
-		assertAppHashEqualsOneFromState(appHash, state)
+		if err := checkAppHashEqualsOneFromState(appHash, state); err != nil {
+			return nil, err
+		}
 		return appHash, nil
 
 	case appBlockHeight == 0 && state.InitialHeight < storeBlockBase:
@@ -376,11 +376,11 @@ func (h *Handshaker) ReplayBlocks(
 
 	case storeBlockHeight < stateBlockHeight:
 		// the state should never be ahead of the store (this is under tendermint's control)
-		panic(fmt.Sprintf("StateBlockHeight (%d) > StoreBlockHeight (%d)", stateBlockHeight, storeBlockHeight))
+		return nil, fmt.Errorf("StateBlockHeight (%d) > StoreBlockHeight (%d)", stateBlockHeight, storeBlockHeight)
 
 	case storeBlockHeight > stateBlockHeight+1:
 		// store should be at most one ahead of the state (this is under tendermint's control)
-		panic(fmt.Sprintf("StoreBlockHeight (%d) > StateBlockHeight + 1 (%d)", storeBlockHeight, stateBlockHeight+1))
+		return nil, fmt.Errorf("StoreBlockHeight (%d) > StateBlockHeight + 1 (%d)", storeBlockHeight, stateBlockHeight+1)
 	}
 
 	var err error
@@ -395,7 +395,9 @@ func (h *Handshaker) ReplayBlocks(
 
 		} else if appBlockHeight == storeBlockHeight {
 			// We're good!
-			assertAppHashEqualsOneFromState(appHash, state)
+			if err := checkAppHashEqualsOneFromState(appHash, state); err != nil {
+				return nil, err
+			}
 			return appHash, nil
 		}
 
@@ -415,7 +417,11 @@ func (h *Handshaker) ReplayBlocks(
 			// but we'd have to allow the WAL to replay a block that wrote it's #ENDHEIGHT
 			h.logger.Info("Replay last block using real app")
 			state, err = h.replayBlock(ctx, state, storeBlockHeight, appClient)
-			return state.AppHash, err
+			if err != nil {
+				return nil, err
+
+			}
+			return state.AppHash, nil
 
 		case appBlockHeight == storeBlockHeight:
 			// We ran Commit, but didn't save the state, so replayBlock with mock app.
@@ -437,13 +443,13 @@ func (h *Handshaker) ReplayBlocks(
 				return nil, err
 			}
 
-			return state.AppHash, err
+			return state.AppHash, nil
 		}
 
 	}
 
-	panic(fmt.Sprintf("uncovered case! appHeight: %d, storeHeight: %d, stateHeight: %d",
-		appBlockHeight, storeBlockHeight, stateBlockHeight))
+	return nil, fmt.Errorf("uncovered case! appHeight: %d, storeHeight: %d, stateHeight: %d",
+		appBlockHeight, storeBlockHeight, stateBlockHeight)
 }
 
 func (h *Handshaker) replayBlocks(
@@ -452,7 +458,8 @@ func (h *Handshaker) replayBlocks(
 	appClient abciclient.Client,
 	appBlockHeight,
 	storeBlockHeight int64,
-	mutateState bool) ([]byte, error) {
+	mutateState bool,
+) ([]byte, error) {
 	// App is further behind than it should be, so we need to replay blocks.
 	// We replay all blocks from appBlockHeight+1.
 	//
@@ -478,7 +485,9 @@ func (h *Handshaker) replayBlocks(
 		block := h.store.LoadBlock(i)
 		// Extra check to ensure the app was not changed in a way it shouldn't have.
 		if len(appHash) > 0 {
-			assertAppHashEqualsOneFromBlock(appHash, block)
+			if err := checkAppHashEqualsOneFromBlock(appHash, block); err != nil {
+				return nil, err
+			}
 		}
 
 		if i == finalBlock && !mutateState {
@@ -510,7 +519,9 @@ func (h *Handshaker) replayBlocks(
 		appHash = state.AppHash
 	}
 
-	assertAppHashEqualsOneFromState(appHash, state)
+	if err := checkAppHashEqualsOneFromState(appHash, state); err != nil {
+		return nil, err
+	}
 	return appHash, nil
 }
 
@@ -539,24 +550,25 @@ func (h *Handshaker) replayBlock(
 	return state, nil
 }
 
-func assertAppHashEqualsOneFromBlock(appHash []byte, block *types.Block) {
+func checkAppHashEqualsOneFromBlock(appHash []byte, block *types.Block) error {
 	if !bytes.Equal(appHash, block.AppHash) {
-		panic(fmt.Sprintf(`block.AppHash does not match AppHash after replay. Got %X, expected %X.
+		return fmt.Errorf(`block.AppHash does not match AppHash after replay. Got '%X', expected '%X'.
 
-Block: %v
-`,
-			appHash, block.AppHash, block))
+Block: %v`,
+			appHash, block.AppHash, block)
 	}
+	return nil
 }
 
-func assertAppHashEqualsOneFromState(appHash []byte, state sm.State) {
+func checkAppHashEqualsOneFromState(appHash []byte, state sm.State) error {
 	if !bytes.Equal(appHash, state.AppHash) {
-		panic(fmt.Sprintf(`state.AppHash does not match AppHash after replay. Got
-%X, expected %X.
+		return fmt.Errorf(`state.AppHash does not match AppHash after replay. Got '%X', expected '%X'.
 
 State: %v
 
 Did you reset Tendermint without resetting your application's data?`,
-			appHash, state.AppHash, state))
+			appHash, state.AppHash, state)
 	}
+
+	return nil
 }

internal/consensus/replay_test.go

@@ -944,7 +944,7 @@ func buildTMStateFromChain(
 	return state
 }
 
-func TestHandshakePanicsIfAppReturnsWrongAppHash(t *testing.T) {
+func TestHandshakeErrorsIfAppReturnsWrongAppHash(t *testing.T) {
 	// 1. Initialize tendermint and commit 3 blocks with the following app hashes:
 	//		- 0x01
 	//		- 0x02
@@ -988,12 +988,8 @@ func TestHandshakePanicsIfAppReturnsWrongAppHash(t *testing.T) {
 		require.NoError(t, err)
 		t.Cleanup(func() { cancel(); proxyApp.Wait() })
 
-		assert.Panics(t, func() {
-			h := NewHandshaker(logger, stateStore, state, store, eventBus, genDoc)
-			if err = h.Handshake(ctx, proxyApp); err != nil {
-				t.Log(err)
-			}
-		})
+		h := NewHandshaker(logger, stateStore, state, store, eventBus, genDoc)
+		assert.Error(t, h.Handshake(ctx, proxyApp))
 	}
 
 	// 3. Tendermint must panic if app returns wrong hash for the last block
@@ -1008,12 +1004,8 @@ func TestHandshakePanicsIfAppReturnsWrongAppHash(t *testing.T) {
 		require.NoError(t, err)
 		t.Cleanup(func() { cancel(); proxyApp.Wait() })
 
-		assert.Panics(t, func() {
-			h := NewHandshaker(logger, stateStore, state, store, eventBus, genDoc)
-			if err = h.Handshake(ctx, proxyApp); err != nil {
-				t.Log(err)
-			}
-		})
+		h := NewHandshaker(logger, stateStore, state, store, eventBus, genDoc)
+		require.Error(t, h.Handshake(ctx, proxyApp))
 	}
 }