From 48b1952f18e5d88233b42fb5e19c3373993608c6 Mon Sep 17 00:00:00 2001
From: Sam Kleinman <garen@tychoish.com>
Date: Tue, 15 Mar 2022 18:27:23 -0400
Subject: [PATCH] state: avoid panics for marshaling errors (#8125)

---
 internal/rpc/core/blocks.go    |  4 +---
 internal/rpc/core/consensus.go |  8 +++-----
 internal/state/state.go        | 25 ++++++++++++++++---------
 internal/state/state_test.go   | 13 ++++++++++---
 internal/state/store.go        | 14 ++++++++++++--
 5 files changed, 42 insertions(+), 22 deletions(-)

diff --git a/internal/rpc/core/blocks.go b/internal/rpc/core/blocks.go
index 427e691da..26044aef7 100644
--- a/internal/rpc/core/blocks.go
+++ b/internal/rpc/core/blocks.go
@@ -23,9 +23,7 @@ import (
 // order (highest first).
 //
 // More: https://docs.tendermint.com/master/rpc/#/Info/blockchain
-func (env *Environment) BlockchainInfo(
-	ctx context.Context,
-	minHeight, maxHeight int64) (*coretypes.ResultBlockchainInfo, error) {
+func (env *Environment) BlockchainInfo(ctx context.Context, minHeight, maxHeight int64) (*coretypes.ResultBlockchainInfo, error) {
 
 	const limit int64 = 20
 
diff --git a/internal/rpc/core/consensus.go b/internal/rpc/core/consensus.go
index 6acdcc333..b30209b38 100644
--- a/internal/rpc/core/consensus.go
+++ b/internal/rpc/core/consensus.go
@@ -14,10 +14,7 @@ import (
 // for the validators in the set as used in computing their Merkle root.
 //
 // More: https://docs.tendermint.com/master/rpc/#/Info/validators
-func (env *Environment) Validators(
-	ctx context.Context,
-	heightPtr *int64,
-	pagePtr, perPagePtr *int) (*coretypes.ResultValidators, error) {
+func (env *Environment) Validators(ctx context.Context, heightPtr *int64, pagePtr, perPagePtr *int) (*coretypes.ResultValidators, error) {
 
 	// The latest validator that we know is the NextValidator of the last block.
 	height, err := env.getHeight(env.latestUncommittedHeight(), heightPtr)
@@ -86,7 +83,8 @@ func (env *Environment) DumpConsensusState(ctx context.Context) (*coretypes.Resu
 	}
 	return &coretypes.ResultDumpConsensusState{
 		RoundState: roundState,
-		Peers:      peerStates}, nil
+		Peers:      peerStates,
+	}, nil
 }
 
 // ConsensusState returns a concise summary of the consensus state.
diff --git a/internal/state/state.go b/internal/state/state.go
index 727adcef7..a31d8baad 100644
--- a/internal/state/state.go
+++ b/internal/state/state.go
@@ -129,23 +129,30 @@ func (state State) Copy() State {
 }
 
 // Equals returns true if the States are identical.
-func (state State) Equals(state2 State) bool {
-	sbz, s2bz := state.Bytes(), state2.Bytes()
-	return bytes.Equal(sbz, s2bz)
+func (state State) Equals(state2 State) (bool, error) {
+	sbz, err := state.Bytes()
+	if err != nil {
+		return false, err
+	}
+	s2bz, err := state2.Bytes()
+	if err != nil {
+		return false, err
+	}
+	return bytes.Equal(sbz, s2bz), nil
 }
 
-// Bytes serializes the State using protobuf.
-// It panics if either casting to protobuf or serialization fails.
-func (state State) Bytes() []byte {
+// Bytes serializes the State using protobuf, propagating marshaling
+// errors
+func (state State) Bytes() ([]byte, error) {
 	sm, err := state.ToProto()
 	if err != nil {
-		panic(err)
+		return nil, err
 	}
 	bz, err := proto.Marshal(sm)
 	if err != nil {
-		panic(err)
+		return nil, err
 	}
-	return bz
+	return bz, nil
 }
 
 // IsEmpty returns true if the State is equal to the empty State.
diff --git a/internal/state/state_test.go b/internal/state/state_test.go
index 18c419e4f..f38d37ea4 100644
--- a/internal/state/state_test.go
+++ b/internal/state/state_test.go
@@ -55,13 +55,18 @@ func TestStateCopy(t *testing.T) {
 
 	stateCopy := state.Copy()
 
-	assert.True(t, state.Equals(stateCopy),
+	seq, err := state.Equals(stateCopy)
+	require.NoError(t, err)
+	assert.True(t, seq,
 		"expected state and its copy to be identical.\ngot: %v\nexpected: %v",
 		stateCopy, state)
 
 	stateCopy.LastBlockHeight++
 	stateCopy.LastValidators = state.Validators
-	assert.False(t, state.Equals(stateCopy), "expected states to be different. got same %v", state)
+
+	seq, err = state.Equals(stateCopy)
+	require.NoError(t, err)
+	assert.False(t, seq, "expected states to be different. got same %v", state)
 }
 
 // TestMakeGenesisStateNilValidators tests state's consistency when genesis file's validators field is nil.
@@ -90,7 +95,9 @@ func TestStateSaveLoad(t *testing.T) {
 
 	loadedState, err := stateStore.Load()
 	require.NoError(t, err)
-	assert.True(t, state.Equals(loadedState),
+	seq, err := state.Equals(loadedState)
+	require.NoError(t, err)
+	assert.True(t, seq,
 		"expected state and its copy to be identical.\ngot: %v\nexpected: %v",
 		loadedState, state)
 }
diff --git a/internal/state/store.go b/internal/state/store.go
index d9a591f6a..93bd3eb2b 100644
--- a/internal/state/store.go
+++ b/internal/state/store.go
@@ -170,7 +170,12 @@ func (store dbStore) save(state State, key []byte) error {
 		return err
 	}
 
-	if err := batch.Set(key, state.Bytes()); err != nil {
+	stateBz, err := state.Bytes()
+	if err != nil {
+		return err
+	}
+
+	if err := batch.Set(key, stateBz); err != nil {
 		return err
 	}
 
@@ -206,7 +211,12 @@ func (store dbStore) Bootstrap(state State) error {
 		return err
 	}
 
-	if err := batch.Set(stateKey, state.Bytes()); err != nil {
+	stateBz, err := state.Bytes()
+	if err != nil {
+		return err
+	}
+
+	if err := batch.Set(stateKey, stateBz); err != nil {
 		return err
 	}