chore(lint): add nilnil, nilerr and exportloopref linters (#2072)

.golangci.yml

@@ -82,6 +82,7 @@ linters:
     - bodyclose
     - depguard
     - errcheck
+    - exportloopref
     - goconst
     - gocyclo
     - gofmt
@@ -96,6 +97,8 @@ linters:
     - megacheck
     - megacheck
     - misspell
+    - nilerr
+    - nilnil
     - nolintlint
     - revive
     - staticcheck
@@ -173,7 +176,7 @@ issues:
       linters:
         - lll
 
-    - text: 'G204: Subprocess launched with variable'
+    - text: "G204: Subprocess launched with variable"
       linters:
         - gosec
 

dot/digest/digest_test.go

@@ -176,6 +176,7 @@ func TestHandler_GrandpaPauseAndResume(t *testing.T) {
 	require.NoError(t, err)
 	nextPause, err := handler.grandpaState.(*state.GrandpaState).GetNextPause()
 	require.NoError(t, err)
+	require.NotNil(t, nextPause) // ensure pause was found
 	require.Equal(t, big.NewInt(int64(p.Delay)), nextPause)
 
 	headers, _ := state.AddBlocksToState(t, handler.blockState.(*state.BlockState), 3, false)
@@ -211,6 +212,7 @@ func TestHandler_GrandpaPauseAndResume(t *testing.T) {
 
 	nextResume, err := handler.grandpaState.(*state.GrandpaState).GetNextResume()
 	require.NoError(t, err)
+	require.NotNil(t, nextResume) // ensure resume was found
 	require.Equal(t, big.NewInt(int64(r.Delay)+int64(p.Delay)), nextResume)
 }
 

dot/rpc/modules/state.go

@@ -422,8 +422,9 @@ func (sm *StateModule) QueryStorage(
 			changes = append(changes, []string{key, value})
 		}
 
+		blochHash := block
 		response = append(response, StorageChangeSetResponse{
-			Block:   &block,
+			Block:   &blochHash,
 			Changes: changes,
 		})
 	}

dot/state/grandpa.go

@@ -233,14 +233,15 @@ func (s *GrandpaState) SetNextPause(number *big.Int) error {
 	return s.db.Put(pauseKey, number.Bytes())
 }
 
-// GetNextPause returns the block number of the next grandpa pause, nil if there is no upcoming pause
+// GetNextPause returns the block number of the next grandpa pause.
+// If the key is not found in the database, a nil block number is returned
+// to indicate there is no upcoming Grandpa pause.
+// It returns an error on failure.
 func (s *GrandpaState) GetNextPause() (*big.Int, error) {
 	num, err := s.db.Get(pauseKey)
-	if err == chaindb.ErrKeyNotFound {
-		return nil, nil
-	}
-
-	if err != nil {
+	if errors.Is(err, chaindb.ErrKeyNotFound) {
+		return nil, nil //nolint:nilnil
+	} else if err != nil {
 		return nil, err
 	}
 
@@ -252,13 +253,15 @@ func (s *GrandpaState) SetNextResume(number *big.Int) error {
 	return s.db.Put(resumeKey, number.Bytes())
 }
 
-// GetNextResume returns the block number of the next grandpa resume, nil if there is no upcoming resume
+// GetNextResume returns the block number of the next grandpa resume.
+// If the key is not found in the database, a nil block number is returned
+// to indicate there is no upcoming Grandpa resume.
+// It returns an error on failure.
 func (s *GrandpaState) GetNextResume() (*big.Int, error) {
 	num, err := s.db.Get(resumeKey)
-	if err == chaindb.ErrKeyNotFound {
-		return nil, nil
-	}
-	if err != nil {
+	if errors.Is(err, chaindb.ErrKeyNotFound) {
+		return nil, nil //nolint:nilnil
+	} else if err != nil {
 		return nil, err
 	}
 

dot/sync/bootstrap_syncer.go

@@ -33,7 +33,7 @@ func (s *bootstrapSyncer) handleNewPeerState(ps *peerState) (*worker, error) {
 	}
 
 	if ps.number.Cmp(head.Number) <= 0 {
-		return nil, nil
+		return nil, nil //nolint:nilnil
 	}
 
 	return &worker{
@@ -45,7 +45,9 @@ func (s *bootstrapSyncer) handleNewPeerState(ps *peerState) (*worker, error) {
 	}, nil
 }
 
-func (s *bootstrapSyncer) handleWorkerResult(res *worker) (*worker, error) {
+//nolint:nilnil
+func (s *bootstrapSyncer) handleWorkerResult(res *worker) (
+	workerToRetry *worker, err error) {
 	// if there is an error, potentially retry the worker
 	if res.err == nil {
 		return nil, nil

dot/sync/chain_sync.go

@@ -60,13 +60,13 @@ type peerState struct {
 // and stored pending work (ie. pending blocks set)
 // workHandler should be implemented by `bootstrapSync` and `tipSync`
 type workHandler interface {
-	// handleNewPeerState optionally returns a new worker based on a peerState.
-	// returned worker may be nil, in which case we do nothing
+	// handleNewPeerState returns a new worker based on a peerState.
+	// The worker may be nil in which case we do nothing.
 	handleNewPeerState(*peerState) (*worker, error)
 
 	// handleWorkerResult handles the result of a worker, which may be
-	// nil or error. optionally returns a new worker to be dispatched.
-	handleWorkerResult(*worker) (*worker, error)
+	// nil or error. It optionally returns a new worker to be dispatched.
+	handleWorkerResult(w *worker) (workerToRetry *worker, err error)
 
 	// hasCurrentWorker is called before a worker is to be dispatched to
 	// check whether it is a duplicate. this function returns whether there is
@@ -435,9 +435,7 @@ func (cs *chainSync) sync() {
 			if err != nil {
 				logger.Errorf("failed to handle worker result: %s", err)
 				continue
-			}
-
-			if worker == nil {
+			} else if worker == nil {
 				continue
 			}
 
@@ -564,13 +562,10 @@ func (cs *chainSync) handleWork(ps *peerState) error {
 	worker, err := cs.handler.handleNewPeerState(ps)
 	if err != nil {
 		return err
+	} else if worker != nil {
+		cs.tryDispatchWorker(worker)
 	}
 
-	if worker == nil {
-		return nil
-	}
-
-	cs.tryDispatchWorker(worker)
 	return nil
 }
 

dot/sync/tip_syncer.go

@@ -35,7 +35,7 @@ func (s *tipSyncer) handleNewPeerState(ps *peerState) (*worker, error) {
 	}
 
 	if ps.number.Cmp(fin.Number) <= 0 {
-		return nil, nil
+		return nil, nil //nolint:nilnil
 	}
 
 	return &worker{
@@ -47,7 +47,9 @@ func (s *tipSyncer) handleNewPeerState(ps *peerState) (*worker, error) {
 	}, nil
 }
 
-func (s *tipSyncer) handleWorkerResult(res *worker) (*worker, error) {
+//nolint:nilnil
+func (s *tipSyncer) handleWorkerResult(res *worker) (
+	workerToRetry *worker, err error) {
 	if res.err == nil {
 		return nil, nil
 	}

lib/babe/crypto.go

@@ -27,7 +27,9 @@ func makeTranscript(randomness Randomness, slot, epoch uint64) *merlin.Transcrip
 	return t
 }
 
-// claimPrimarySlot checks if a slot can be claimed. if it can be, then a *VrfOutputAndProof is returned, otherwise nil.
+// claimPrimarySlot checks if a slot can be claimed.
+// If it cannot be claimed, the wrapped error
+// errOverPrimarySlotThreshold is returned.
 // https://github.com/paritytech/substrate/blob/master/client/consensus/babe/src/authorship.rs#L239
 func claimPrimarySlot(randomness Randomness,
 	slot, epoch uint64,
@@ -49,7 +51,8 @@ func claimPrimarySlot(randomness Randomness,
 		return nil, fmt.Errorf("failed to compare with threshold, %w", err)
 	}
 	if !ok {
-		return nil, nil
+		return nil, fmt.Errorf("%w: for slot %d, epoch %d and threshold %s",
+			errOverPrimarySlotThreshold, slot, epoch, threshold)
 	}
 
 	return &VrfOutputAndProof{

lib/babe/crypto_test.go

@@ -24,7 +24,7 @@ func TestRunLottery_False(t *testing.T) {
 	babeService.epochData.threshold = minThreshold
 
 	outAndProof, err := babeService.runLottery(0, testEpochIndex)
-	require.NoError(t, err)
+	require.ErrorIs(t, err, errOverPrimarySlotThreshold)
 	require.Nil(t, outAndProof)
 }
 

lib/babe/epoch.go

@@ -4,6 +4,7 @@
 package babe
 
 import (
+	"errors"
 	"fmt"
 )
 
@@ -91,15 +92,16 @@ func (b *Service) initiateEpoch(epoch uint64) error {
 
 		logger.Debugf("estimated first slot as %d based on building block 1", startSlot)
 		for i := startSlot; i < startSlot+b.epochLength; i++ {
-			proof, err := b.runLottery(i, epoch)
+			_, err := b.runLottery(i, epoch)
 			if err != nil {
+				if errors.Is(err, errOverPrimarySlotThreshold) {
+					continue
+				}
 				return fmt.Errorf("error running slot lottery at slot %d: error %w", i, err)
 			}
 
-			if proof != nil {
-				startSlot = i
-				break
-			}
+			startSlot = i
+			break
 		}
 
 		// we are at genesis, set first slot by checking at which slot we will be able to produce block 1
@@ -118,13 +120,14 @@ func (b *Service) initiateEpoch(epoch uint64) error {
 
 		proof, err := b.runLottery(i, epoch)
 		if err != nil {
+			if errors.Is(err, errOverPrimarySlotThreshold) {
+				continue
+			}
 			return fmt.Errorf("error running slot lottery at slot %d: error %w", i, err)
 		}
 
-		if proof != nil {
-			b.slotToProof[i] = proof
-			logger.Tracef("claimed slot %d, there are now %d slots into epoch", startSlot, i-startSlot)
-		}
+		b.slotToProof[i] = proof
+		logger.Tracef("claimed slot %d, there are now %d slots into epoch", startSlot, i-startSlot)
 	}
 
 	return nil
@@ -133,15 +136,16 @@ func (b *Service) initiateEpoch(epoch uint64) error {
 func (b *Service) getFirstSlot(epoch uint64) (uint64, error) {
 	startSlot := getCurrentSlot(b.slotDuration)
 	for i := startSlot; i < startSlot+b.epochLength; i++ {
-		proof, err := b.runLottery(i, epoch)
+		_, err := b.runLottery(i, epoch)
 		if err != nil {
+			if errors.Is(err, errOverPrimarySlotThreshold) {
+				continue
+			}
 			return 0, fmt.Errorf("error running slot lottery at slot %d: error %w", i, err)
 		}
 
-		if proof != nil {
-			startSlot = i
-			break
-		}
+		startSlot = i
+		break
 	}
 
 	return startSlot, nil
@@ -163,8 +167,11 @@ func (b *Service) incrementEpoch() (uint64, error) {
 	return next, nil
 }
 
-// runLottery runs the lottery for a specific slot number
-// returns an encoded VrfOutput and VrfProof if validator is authorized to produce a block for that slot, nil otherwise
+// runLottery runs the lottery for a specific slot number.
+// It returns an encoded VrfOutputAndProof if the validator is authorised
+// to produce a block for that slot.
+// It returns the wrapped error errOverPrimarySlotThreshold
+// if it is not authorised.
 // output = return[0:32]; proof = return[32:96]
 func (b *Service) runLottery(slot, epoch uint64) (*VrfOutputAndProof, error) {
 	return claimPrimarySlot(

lib/babe/errors.go

@@ -57,15 +57,16 @@ var (
 	// ErrNotAuthority is returned when trying to perform authority functions when not an authority
 	ErrNotAuthority = errors.New("node is not an authority")
 
-	errNilBlockImportHandler = errors.New("cannot have nil BlockImportHandler")
-	errNilBlockState         = errors.New("cannot have nil BlockState")
-	errNilEpochState         = errors.New("cannot have nil EpochState")
-	errNilStorageState       = errors.New("storage state is nil")
-	errNilParentHeader       = errors.New("parent header is nil")
-	errInvalidResult         = errors.New("invalid error value")
-	errNoEpochData           = errors.New("no epoch data found for upcoming epoch")
-	errFirstBlockTimeout     = errors.New("timed out waiting for first block")
-	errChannelClosed         = errors.New("block notifier channel was closed")
+	errNilBlockImportHandler    = errors.New("cannot have nil BlockImportHandler")
+	errNilBlockState            = errors.New("cannot have nil BlockState")
+	errNilEpochState            = errors.New("cannot have nil EpochState")
+	errNilStorageState          = errors.New("storage state is nil")
+	errNilParentHeader          = errors.New("parent header is nil")
+	errInvalidResult            = errors.New("invalid error value")
+	errNoEpochData              = errors.New("no epoch data found for upcoming epoch")
+	errFirstBlockTimeout        = errors.New("timed out waiting for first block")
+	errChannelClosed            = errors.New("block notifier channel was closed")
+	errOverPrimarySlotThreshold = errors.New("cannot claim slot, over primary threshold")
 
 	other         Other
 	invalidCustom InvalidCustom

lib/common/types/result.go

@@ -4,6 +4,7 @@
 package types
 
 import (
+	"errors"
 	"io"
 
 	"github.com/ChainSafe/gossamer/lib/common"
@@ -53,8 +54,10 @@ func (r *Result) Decode(reader io.Reader) (*Result, error) {
 
 	for {
 		b, err := common.ReadByte(reader)
-		if err != nil {
+		if errors.Is(err, io.EOF) {
 			break
+		} else if err != nil {
+			return nil, err
 		}
 
 		r.data = append(r.data, b)

lib/genesis/test_utils.go

@@ -5,6 +5,7 @@ package genesis
 
 import (
 	"encoding/json"
+	"fmt"
 	"math/big"
 	"os"
 	"testing"
@@ -97,12 +98,12 @@ func CreateTestGenesisJSONFile(asRaw bool) (string, error) {
 
 	bz, err := json.Marshal(tGen)
 	if err != nil {
-		return "", nil
+		return "", fmt.Errorf("cannot marshal test genesis: %w", err)
 	}
 	// Write to temp file
 	_, err = file.Write(bz)
 	if err != nil {
-		return "", nil
+		return "", fmt.Errorf("cannot write JSON test genesis: %w", err)
 	}
 
 	return file.Name(), nil

lib/grandpa/message_handler.go

@@ -143,7 +143,7 @@ func (h *MessageHandler) handleCommitMessage(msg *CommitMessage) error {
 
 func (h *MessageHandler) handleCatchUpRequest(msg *CatchUpRequest) (*ConsensusMessage, error) {
 	if !h.grandpa.authority {
-		return nil, nil
+		return nil, nil //nolint:nilnil
 	}
 
 	logger.Debugf("received catch up request for round %d and set id %d",

lib/runtime/life/resolver.go

@@ -429,7 +429,7 @@ func storageAppend(storage runtime.Storage, key, valueToAppend []byte) error {
 		if err != nil {
 			logger.Tracef("[ext_storage_append_version_1] item in storage is not SCALE encoded, overwriting at key 0x%x", key)
 			storage.Set(key, append([]byte{4}, valueToAppend...))
-			return nil
+			return nil //nolint:nilerr
 		}
 
 		lengthBytes, err := scale.Marshal(currLength)

lib/runtime/wasmer/imports.go

@@ -1790,7 +1790,7 @@ func storageAppend(storage runtime.Storage, key, valueToAppend []byte) error {
 			logger.Tracef(
 				"[ext_storage_append_version_1] item in storage is not SCALE encoded, overwriting at key 0x%x", key)
 			storage.Set(key, append([]byte{4}, valueToAppend...))
-			return nil
+			return nil //nolint:nilerr
 		}
 
 		lengthBytes, err := scale.Marshal(currLength)