Add forbid panics lint rule (#1294)

* Add forbid panics rule * Remove panics in code * Fix linters * Remove panic from Txn.Send function * Loop closure variable (parallel unit tests) * Remove panics from native_transfer_test * Make newDummyHost a helper function * Remove space from the beginning of error msg --------- Co-authored-by: Stefan Negovanović <[email protected]>
0xPolygon · Mar 15, 2023 · b2dbc12 · b2dbc12
1 parent 0ff272a
commit b2dbc12
Show file tree

Hide file tree

Showing 67 changed files with 685 additions and 464 deletions.
diff --git a/blockchain/blockchain.go b/blockchain/blockchain.go
@@ -1227,7 +1227,7 @@ func (b *Blockchain) writeHeaderImpl(evnt *Event, header *types.Header) error {
 
 	currentTD, ok := b.readTotalDifficulty(currentHeader.Hash)
 	if !ok {
-		panic("failed to get header difficulty")
+		return errors.New("failed to get header difficulty")
 	}
 
 	// parent total difficulty of incoming header

diff --git a/blockchain/storage/keyvalue.go b/blockchain/storage/keyvalue.go
@@ -261,7 +261,7 @@ func (s *KeyValueStorage) ReadTxLookup(hash types.Hash) (types.Hash, bool) {
 	blockHash, err := v.GetBytes(blockHash[:0], 32)
 
 	if err != nil {
-		panic(err)
+		return types.Hash{}, false
 	}
 
 	return types.BytesToHash(blockHash), true

diff --git a/blockchain/storage/testing.go b/blockchain/storage/testing.go
@@ -264,7 +264,7 @@ func testBody(t *testing.T, m PlaceholderStorage) {
 		ExtraData:  []byte{}, // if not set it will fail
 	}
 	if err := s.WriteHeader(header); err != nil {
-		panic(err)
+		t.Fatal(err)
 	}
 
 	addr1 := types.StringToAddress("11")
@@ -297,9 +297,7 @@ func testBody(t *testing.T, m PlaceholderStorage) {
 	}
 
 	body0 := block.Body()
-	if err := s.WriteBody(header.Hash, body0); err != nil {
-		panic(err)
-	}
+	assert.NoError(t, s.WriteBody(header.Hash, body0))
 
 	body1, err := s.ReadBody(header.Hash)
 	assert.NoError(t, err)

diff --git a/command/polybftsecrets/params.go b/command/polybftsecrets/params.go
@@ -186,7 +186,11 @@ func (ip *initParams) initKeys(secretsManager secrets.SecretsManager) ([]string,
 		)
 
 		if !secretsManager.HasSecret(secrets.ValidatorKey) && !secretsManager.HasSecret(secrets.ValidatorBLSKey) {
-			a = wallet.GenerateAccount()
+			a, err = wallet.GenerateAccount()
+			if err != nil {
+				return generated, fmt.Errorf("error generating account: %w", err)
+			}
+
 			if err = a.Save(secretsManager); err != nil {
 				return generated, fmt.Errorf("error saving account: %w", err)
 			}

diff --git a/command/polybftsecrets/utils.go b/command/polybftsecrets/utils.go
@@ -35,7 +35,9 @@ func GetSecretsManager(dataPath, configPath string, insecureLocalStore bool) (se
 	if configPath != "" {
 		secretsConfig, readErr := secrets.ReadConfig(configPath)
 		if readErr != nil {
-			return nil, fmt.Errorf("%s: %w", ErrInvalidConfig.Error(), readErr)
+			invalidConfigErr := ErrInvalidConfig.Error()
+
+			return nil, fmt.Errorf("%s: %w", invalidConfigErr, readErr)
 		}
 
 		if !secrets.SupportedServiceManager(secretsConfig.Type) {

diff --git a/consensus/ibft/fork/hooks_test.go b/consensus/ibft/fork/hooks_test.go
@@ -17,6 +17,7 @@ import (
 	"github.com/0xPolygon/polygon-edge/validators/store"
 	"github.com/hashicorp/go-hclog"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 type mockHeaderModifierStore struct {
@@ -281,7 +282,9 @@ func newTestTransition(
 		Forks: chain.AllForksEnabled,
 	}, st, hclog.NewNullLogger())
 
-	rootHash := ex.WriteGenesis(nil)
+	rootHash, err := ex.WriteGenesis(nil)
+	require.NoError(t, err)
+
 	ex.GetHash = func(h *types.Header) state.GetHashByNumber {
 		return func(i uint64) types.Hash {
 			return rootHash

diff --git a/consensus/ibft/helper_test.go b/consensus/ibft/helper_test.go
@@ -8,6 +8,7 @@ import (
 	"github.com/0xPolygon/polygon-edge/crypto"
 	"github.com/0xPolygon/polygon-edge/types"
 	"github.com/0xPolygon/polygon-edge/validators"
+	"github.com/stretchr/testify/require"
 )
 
 type testerAccount struct {
@@ -55,9 +56,7 @@ func (ap *testerAccountPool) add(accounts ...string) {
 		}
 
 		priv, err := crypto.GenerateECDSAKey()
-		if err != nil {
-			panic("BUG: Failed to generate crypto key")
-		}
+		require.NoError(ap.t, err)
 
 		ap.accounts = append(ap.accounts, &testerAccount{
 			alias: account,

diff --git a/consensus/polybft/blockchain_wrapper.go b/consensus/polybft/blockchain_wrapper.go
@@ -208,5 +208,5 @@ func (s *stateProvider) Call(addr ethgo.Address, input []byte, opts *contract.Ca
 // Txn is part of the contract.Provider interface to make Ethereum transactions. We disable this function
 // since the system state does not make any transaction
 func (s *stateProvider) Txn(ethgo.Address, ethgo.Key, []byte) (contract.Txn, error) {
-	panic(errSendTxnUnsupported)
+	return nil, errSendTxnUnsupported
 }
diff --git a/consensus/polybft/checkpoint_manager_test.go b/consensus/polybft/checkpoint_manager_test.go
@@ -36,7 +36,7 @@ func TestCheckpointManager_SubmitCheckpoint(t *testing.T) {
 
 	var aliases = []string{"A", "B", "C", "D", "E"}
 
-	validators := newTestValidatorsWithAliases(aliases)
+	validators := newTestValidatorsWithAliases(t, aliases)
 	validatorsMetadata := validators.getPublicIdentities()
 	txRelayerMock := newDummyTxRelayer(t)
 	txRelayerMock.On("Call", mock.Anything, mock.Anything, mock.Anything).
@@ -122,8 +122,8 @@ func TestCheckpointManager_abiEncodeCheckpointBlock(t *testing.T) {
 
 	const epochSize = uint64(10)
 
-	currentValidators := newTestValidatorsWithAliases([]string{"A", "B", "C", "D"})
-	nextValidators := newTestValidatorsWithAliases([]string{"E", "F", "G", "H"})
+	currentValidators := newTestValidatorsWithAliases(t, []string{"A", "B", "C", "D"})
+	nextValidators := newTestValidatorsWithAliases(t, []string{"E", "F", "G", "H"})
 	header := &types.Header{Number: 50}
 	checkpoint := &CheckpointData{
 		BlockRound:  1,
@@ -213,10 +213,12 @@ func TestCheckpointManager_getCurrentCheckpointID(t *testing.T) {
 			txRelayerMock.On("Call", mock.Anything, mock.Anything, mock.Anything).
 				Return(c.checkpointID, c.returnError).
 				Once()
+			acc, err := wallet.GenerateAccount()
+			require.NoError(t, err)
 
 			checkpointMgr := &checkpointManager{
 				rootChainRelayer: txRelayerMock,
-				key:              wallet.GenerateAccount().Ecdsa,
+				key:              acc.Ecdsa,
 				logger:           hclog.NewNullLogger(),
 			}
 			actualCheckpointID, err := checkpointMgr.getLatestCheckpointBlock()

diff --git a/consensus/polybft/consensus_runtime_test.go b/consensus/polybft/consensus_runtime_test.go
@@ -183,7 +183,7 @@ func TestConsensusRuntime_OnBlockInserted_EndOfEpoch(t *testing.T) {
 	)
 
 	currentEpochNumber := getEpochNumber(t, epochSize, epochSize)
-	validatorSet := newTestValidators(validatorsCount).getPublicIdentities()
+	validatorSet := newTestValidators(t, validatorsCount).getPublicIdentities()
 	header, headerMap := createTestBlocks(t, epochSize, epochSize, validatorSet)
 	builtBlock := consensus.BuildBlock(consensus.BuildBlockParams{
 		Header: header,
@@ -289,7 +289,7 @@ func TestConsensusRuntime_OnBlockInserted_MiddleOfEpoch(t *testing.T) {
 func TestConsensusRuntime_FSM_NotInValidatorSet(t *testing.T) {
 	t.Parallel()
 
-	validators := newTestValidatorsWithAliases([]string{"A", "B", "C", "D"})
+	validators := newTestValidatorsWithAliases(t, []string{"A", "B", "C", "D"})
 
 	snapshot := NewProposerSnapshot(1, nil)
 	config := &runtimeConfig{
@@ -324,7 +324,7 @@ func TestConsensusRuntime_FSM_NotEndOfEpoch_NotEndOfSprint(t *testing.T) {
 		ExtraData: append(make([]byte, ExtraVanity), extra.MarshalRLPTo(nil)...),
 	}
 
-	validators := newTestValidators(3)
+	validators := newTestValidators(t, 3)
 	blockchainMock := new(blockchainMock)
 	blockchainMock.On("NewBlockBuilder", mock.Anything).Return(&BlockBuilder{}, nil).Once()
 
@@ -382,7 +382,7 @@ func TestConsensusRuntime_FSM_EndOfEpoch_BuildCommitEpoch(t *testing.T) {
 		toIndex           = uint64(9)
 	)
 
-	validatorAccounts := newTestValidatorsWithAliases([]string{"A", "B", "C", "D", "E", "F"})
+	validatorAccounts := newTestValidatorsWithAliases(t, []string{"A", "B", "C", "D", "E", "F"})
 	validators := validatorAccounts.getPublicIdentities()
 
 	lastBuiltBlock, headerMap := createTestBlocks(t, 9, epochSize, validators)
@@ -450,7 +450,7 @@ func Test_NewConsensusRuntime(t *testing.T) {
 		BlockTime:        2 * time.Second,
 	}
 
-	validators := newTestValidators(3).getPublicIdentities()
+	validators := newTestValidators(t, 3).getPublicIdentities()
 
 	systemStateMock := new(systemStateMock)
 	systemStateMock.On("GetEpoch").Return(uint64(1)).Once()
@@ -497,7 +497,7 @@ func TestConsensusRuntime_restartEpoch_SameEpochNumberAsTheLastOne(t *testing.T)
 	const originalBlockNumber = uint64(5)
 
 	newCurrentHeader := &types.Header{Number: originalBlockNumber + 1}
-	validatorSet := newTestValidators(3).getPublicIdentities()
+	validatorSet := newTestValidators(t, 3).getPublicIdentities()
 
 	systemStateMock := new(systemStateMock)
 	systemStateMock.On("GetEpoch").Return(uint64(1), nil).Once()
@@ -547,7 +547,7 @@ func TestConsensusRuntime_calculateCommitEpochInput_SecondEpoch(t *testing.T) {
 		sprintSize      = 5
 	)
 
-	validators := newTestValidatorsWithAliases([]string{"A", "B", "C", "D", "E"})
+	validators := newTestValidatorsWithAliases(t, []string{"A", "B", "C", "D", "E"})
 	polybftConfig := &PolyBFTConfig{
 		ValidatorSetAddr: contracts.ValidatorSetContract,
 		EpochSize:        epochSize,
@@ -596,7 +596,7 @@ func TestConsensusRuntime_IsValidValidator_BasicCases(t *testing.T) {
 	setupFn := func(t *testing.T) (*consensusRuntime, *testValidators) {
 		t.Helper()
 
-		validatorAccounts := newTestValidatorsWithAliases([]string{"A", "B", "C", "D", "E", "F"})
+		validatorAccounts := newTestValidatorsWithAliases(t, []string{"A", "B", "C", "D", "E", "F"})
 		epoch := &epochMetadata{
 			Validators: validatorAccounts.getPublicIdentities("A", "B", "C", "D"),
 		}
@@ -654,7 +654,7 @@ func TestConsensusRuntime_IsValidValidator_BasicCases(t *testing.T) {
 func TestConsensusRuntime_IsValidValidator_TamperSignature(t *testing.T) {
 	t.Parallel()
 
-	validatorAccounts := newTestValidatorsWithAliases([]string{"A", "B", "C", "D", "E", "F"})
+	validatorAccounts := newTestValidatorsWithAliases(t, []string{"A", "B", "C", "D", "E", "F"})
 	epoch := &epochMetadata{
 		Validators: validatorAccounts.getPublicIdentities("A", "B", "C", "D"),
 	}
@@ -676,7 +676,7 @@ func TestConsensusRuntime_IsValidValidator_TamperSignature(t *testing.T) {
 func TestConsensusRuntime_TamperMessageContent(t *testing.T) {
 	t.Parallel()
 
-	validatorAccounts := newTestValidatorsWithAliases([]string{"A", "B", "C", "D", "E", "F"})
+	validatorAccounts := newTestValidatorsWithAliases(t, []string{"A", "B", "C", "D", "E", "F"})
 	epoch := &epochMetadata{
 		Validators: validatorAccounts.getPublicIdentities("A", "B", "C", "D"),
 	}
@@ -838,7 +838,7 @@ func TestConsensusRuntime_HasQuorum(t *testing.T) {
 
 	const round = 5
 
-	validatorAccounts := newTestValidatorsWithAliases([]string{"A", "B", "C", "D", "E", "F"})
+	validatorAccounts := newTestValidatorsWithAliases(t, []string{"A", "B", "C", "D", "E", "F"})
 
 	extra := &Extra{
 		Checkpoint: &CheckpointData{},

diff --git a/consensus/polybft/contractsapi/artifacts-gen/main.go b/consensus/polybft/contractsapi/artifacts-gen/main.go
@@ -2,6 +2,7 @@ package main
 
 import (
 	"fmt"
+	"log"
 	"os"
 	"path"
 	"runtime"
@@ -87,19 +88,19 @@ func main() {
 	for _, v := range readContracts {
 		artifactBytes, err := artifact.ReadArtifactData(scpath, v.Path, v.Name)
 		if err != nil {
-			panic(err)
+			log.Fatal(err)
 		}
 
 		f.Var().Id(v.Name + "Artifact").String().Op("=").Lit(string(artifactBytes))
 	}
 
 	fl, err := os.Create(currentPath + "/../gen_sc_data.go")
 	if err != nil {
-		panic(err)
+		log.Fatal(err)
 	}
 
 	_, err = fmt.Fprintf(fl, "%#v", f)
 	if err != nil {
-		panic(err)
+		log.Fatal(err)
 	}
 }