lint: fix linter errors and update CI to require passing (#4241)

.github/workflows/reviewdog.yml

@@ -20,10 +20,11 @@ jobs:
         with:
           golangci_lint_version: "v1.41.1"
           golangci_lint_flags: "-c .golangci.yml --allow-parallel-runners"
-          reporter: "github-pr-review"
+          reporter: "github-pr-check"
           tool_name: "Lint Errors"
           level: "error"
           fail_on_error: true
+          filter_mode: "nofilter"
   # Non-Blocking Warnings Section
   reviewdog-warnings:
     runs-on: ubuntu-latest

.golangci-warnings.yml

@@ -5,14 +5,12 @@ run:
 linters:
   disable-all: true
   enable:
-    - staticcheck
+    - deadcode
+    - partitiontest
     - structcheck
     - typecheck
     - varcheck
-    - deadcode
-    - gosimple
     - unused
-    - partitiontest
 
 
 linters-settings:

.golangci.yml

@@ -3,18 +3,37 @@ run:
   tests: false
 
 linters:
+  # default: deadcode, errcheck, gosimple, govet, ineffassign, staticcheck, typecheck, unused, varcheck
   disable-all: true
   enable:
     - errcheck
     - gofmt
-    - golint
+    - gosimple
     - govet
     - ineffassign
     - misspell
+    - nolintlint
+    - revive
+    - staticcheck
+    - typecheck
 
 severity:
   default-severity: error
 
+linters-settings:
+  nolintlint:
+    # require naming a specific linter X using //nolint:X
+    require-specific: true
+    # require comments like "//nolint:errcheck // Explanation of why we are ignoring linter here..."
+    require-explanation: true
+  errcheck:
+    exclude-functions:
+      # data/transactions/logic/assembler.go uses ops.error, warn, to append log messages: OK to ignore for this case
+    - (*github.com/algorand/go-algorand/data/transactions/logic.OpStream).errorf
+    - (*github.com/algorand/go-algorand/data/transactions/logic.OpStream).error
+    - (*github.com/algorand/go-algorand/data/transactions/logic.OpStream).warnf
+    - (*github.com/algorand/go-algorand/data/transactions/logic.OpStream).warn
+
 issues:
   # use these new lint checks on code since #2574
   new-from-rev: eb019291beed556ec6ac1ceb4a15114ce4df0c57
@@ -41,6 +60,8 @@ issues:
     - Error return value of .((os\.)?std(out|err)\..*|.*Close|.*Flush|os\.Remove(All)?|.*print(f|ln)?|os\.(Un)?Setenv). is not checked
     # "EXC0005 staticcheck: Developers tend to write in C-style with an explicit 'break' in a 'switch', so it's ok to ignore"
     - ineffective break statement. Did you mean to break out of the outer loop
+    # revive: irrelevant error about naming
+    - "var-naming: don't use leading k in Go names"
 
   exclude-rules:
     # Add all linters here -- Comment this block out for testing linters

cmd/algokey/keyreg.go

@@ -75,10 +75,14 @@ func init() {
 
 	keyregCmd.Flags().Uint64Var(&params.fee, "fee", minFee, "transaction fee")
 	keyregCmd.Flags().Uint64Var(&params.firstValid, "firstvalid", 0, "first round where the transaction may be committed to the ledger")
-	keyregCmd.MarkFlagRequired("firstvalid") // nolint:errcheck
+	if err := keyregCmd.MarkFlagRequired("firstvalid"); err != nil {
+		panic(err)
+	}
 	keyregCmd.Flags().Uint64Var(&params.lastValid, "lastvalid", 0, fmt.Sprintf("last round where the generated transaction may be committed to the ledger, defaults to firstvalid + %d", txnLife))
 	keyregCmd.Flags().StringVar(&params.network, "network", "mainnet", "the network where the provided keys will be registered, one of mainnet/testnet/betanet")
-	keyregCmd.MarkFlagRequired("network") // nolint:errcheck
+	if err := keyregCmd.MarkFlagRequired("network"); err != nil {
+		panic(err)
+	}
 	keyregCmd.Flags().BoolVar(&params.offline, "offline", false, "set to bring an account offline")
 	keyregCmd.Flags().StringVarP(&params.txFile, "outputFile", "o", "", fmt.Sprintf("write signed transaction to this file, or '%s' to write to stdout", stdoutFilenameValue))
 	keyregCmd.Flags().StringVar(&params.partkeyFile, "keyfile", "", "participation keys to register, file is opened to fetch metadata for the transaction; only specify when bringing an account online to vote in Algorand consensus")

cmd/catchpointdump/net.go

@@ -303,15 +303,21 @@ func deleteLedgerFiles(deleteTracker bool) error {
 
 func loadAndDump(addr string, tarFile string, genesisInitState ledgercore.InitState) error {
 	// delete current ledger files.
-	deleteLedgerFiles(true)
+	if err := deleteLedgerFiles(true); err != nil {
+		reportWarnf("Error deleting ledger files: %v", err)
+	}
 	cfg := config.GetDefaultLocal()
 	l, err := ledger.OpenLedger(logging.Base(), "./ledger", false, genesisInitState, cfg)
 	if err != nil {
 		reportErrorf("Unable to open ledger : %v", err)
 		return err
 	}
 
-	defer deleteLedgerFiles(!loadOnly)
+	defer func() {
+		if err := deleteLedgerFiles(!loadOnly); err != nil {
+			reportWarnf("Error deleting ledger files: %v", err)
+		}
+	}()
 	defer l.Close()
 
 	catchupAccessor := ledger.MakeCatchpointCatchupAccessor(l, logging.Base())

cmd/goal/account.go

@@ -540,9 +540,7 @@ func printAccountInfo(client libgoal.Client, address string, onlyShowAssetIds bo
 	var createdAssets []generatedV2.Asset
 	if account.CreatedAssets != nil {
 		createdAssets = make([]generatedV2.Asset, len(*account.CreatedAssets))
-		for i, asset := range *account.CreatedAssets {
-			createdAssets[i] = asset
-		}
+		copy(createdAssets, *account.CreatedAssets)
 		sort.Slice(createdAssets, func(i, j int) bool {
 			return createdAssets[i].Index < createdAssets[j].Index
 		})
@@ -551,9 +549,7 @@ func printAccountInfo(client libgoal.Client, address string, onlyShowAssetIds bo
 	var heldAssets []generatedV2.AssetHolding
 	if account.Assets != nil {
 		heldAssets = make([]generatedV2.AssetHolding, len(*account.Assets))
-		for i, assetHolding := range *account.Assets {
-			heldAssets[i] = assetHolding
-		}
+		copy(heldAssets, *account.Assets)
 		sort.Slice(heldAssets, func(i, j int) bool {
 			return heldAssets[i].AssetId < heldAssets[j].AssetId
 		})
@@ -562,9 +558,7 @@ func printAccountInfo(client libgoal.Client, address string, onlyShowAssetIds bo
 	var createdApps []generatedV2.Application
 	if account.CreatedApps != nil {
 		createdApps = make([]generatedV2.Application, len(*account.CreatedApps))
-		for i, app := range *account.CreatedApps {
-			createdApps[i] = app
-		}
+		copy(createdApps, *account.CreatedApps)
 		sort.Slice(createdApps, func(i, j int) bool {
 			return createdApps[i].Id < createdApps[j].Id
 		})
@@ -573,9 +567,7 @@ func printAccountInfo(client libgoal.Client, address string, onlyShowAssetIds bo
 	var optedInApps []generatedV2.ApplicationLocalState
 	if account.AppsLocalState != nil {
 		optedInApps = make([]generatedV2.ApplicationLocalState, len(*account.AppsLocalState))
-		for i, appLocalState := range *account.AppsLocalState {
-			optedInApps[i] = appLocalState
-		}
+		copy(optedInApps, *account.AppsLocalState)
 		sort.Slice(optedInApps, func(i, j int) bool {
 			return optedInApps[i].Id < optedInApps[j].Id
 		})

cmd/goal/application.go

@@ -189,9 +189,15 @@ func init() {
 
 	infoAppCmd.MarkFlagRequired("app-id")
 
-	methodAppCmd.MarkFlagRequired("method")    // nolint:errcheck // follow previous required flag format
-	methodAppCmd.MarkFlagRequired("from")      // nolint:errcheck
-	methodAppCmd.Flags().MarkHidden("app-arg") // nolint:errcheck
+	panicIfErr(methodAppCmd.MarkFlagRequired("method"))
+	panicIfErr(methodAppCmd.MarkFlagRequired("from"))
+	panicIfErr(appCmd.PersistentFlags().MarkHidden("app-arg"))
+}
+
+func panicIfErr(err error) {
+	if err != nil {
+		panic(err)
+	}
 }
 
 type appCallArg struct {

cmd/loadgenerator/main.go

@@ -66,9 +66,9 @@ func loadMnemonic(mnemonic string) crypto.Seed {
 // Like shared/pingpong/accounts.go
 func findRootKeys(algodDir string) []*crypto.SignatureSecrets {
 	keylist := make([]*crypto.SignatureSecrets, 0, 5)
-	err := filepath.Walk(algodDir, func(path string, info fs.FileInfo, err error) error {
+	err := filepath.Walk(algodDir, func(path string, info fs.FileInfo, _ error) error {
 		var handle db.Accessor
-		handle, err = db.MakeErasableAccessor(path)
+		handle, err := db.MakeErasableAccessor(path)
 		if err != nil {
 			return nil // don't care, move on
 		}
@@ -241,7 +241,7 @@ func generateTransactions(restClient client.RestClient, cfg config, privateKeys
 		sendSize = transactionBlockSize
 	}
 	// create sendSize transaction to send.
-	txns := make([]transactions.SignedTxn, sendSize, sendSize)
+	txns := make([]transactions.SignedTxn, sendSize)
 	for i := range txns {
 		tx := transactions.Transaction{
 			Header: transactions.Header{
@@ -289,7 +289,7 @@ func generateTransactions(restClient client.RestClient, cfg config, privateKeys
 	for i := 0; i < nroutines; i++ {
 		totalSent += sent[i]
 	}
-	dt := time.Now().Sub(start)
+	dt := time.Since(start)
 	fmt.Fprintf(os.Stdout, "sent %d/%d in %s (%.1f/s)\n", totalSent, sendSize, dt.String(), float64(totalSent)/dt.Seconds())
 	if cfg.TxnsToSend != 0 {
 		// We attempted what we were asked. We're done.

crypto/merklesignature/const.go

@@ -18,6 +18,7 @@ package merklesignature
 
 import (
 	"fmt"
+
 	"github.com/algorand/go-algorand/crypto"
 )
 
@@ -40,7 +41,7 @@ const (
 var NoKeysCommitment = Commitment{}
 
 func init() {
-	// no keys generated, inner tree of merkle siganture scheme is empty.
+	// no keys generated, inner tree of merkle signature scheme is empty.
 	o, err := New(KeyLifetimeDefault+1, KeyLifetimeDefault+2, KeyLifetimeDefault)
 	if err != nil {
 		panic(fmt.Errorf("initializing empty merkle signature scheme failed, err: %w", err))

crypto/stateproof/coinGenerator.go

@@ -18,9 +18,10 @@ package stateproof
 
 import (
 	"encoding/binary"
-	"golang.org/x/crypto/sha3"
 	"math/big"
 
+	"golang.org/x/crypto/sha3"
+
 	"github.com/algorand/go-algorand/crypto"
 	"github.com/algorand/go-algorand/protocol"
 )
@@ -75,7 +76,7 @@ func makeCoinGenerator(choice *coinChoiceSeed) coinGenerator {
 	choice.version = VersionForCoinGenerator
 	rep := crypto.HashRep(choice)
 	shk := sha3.NewShake256()
-	shk.Write(rep)
+	shk.Write(rep) //nolint:errcheck // ShakeHash.Write may panic, but does not return error
 
 	threshold := prepareRejectionSamplingThreshold(choice.signedWeight)
 	return coinGenerator{shkContext: shk, signedWeight: choice.signedWeight, threshold: threshold}
@@ -111,7 +112,7 @@ func (cg *coinGenerator) getNextCoin() uint64 {
 	var randNumFromXof uint64
 	for {
 		var shakeDigest [8]byte
-		cg.shkContext.Read(shakeDigest[:])
+		cg.shkContext.Read(shakeDigest[:]) //nolint:errcheck // ShakeHash.Read never returns error
 		randNumFromXof = binary.LittleEndian.Uint64(shakeDigest[:])
 
 		z := &big.Int{}

data/ledger.go

@@ -184,7 +184,7 @@ func (l *Ledger) Circulation(r basics.Round) (basics.MicroAlgos, error) {
 		}
 	}
 
-	totals, err := l.OnlineTotals(r) //nolint:typecheck
+	totals, err := l.OnlineTotals(r)
 	if err != nil {
 		return basics.MicroAlgos{}, err
 	}

data/transactions/logic/assembler.go

@@ -723,7 +723,7 @@ func asmMethod(ops *OpStream, spec *OpSpec, args []string) error {
 		if err != nil {
 			// Warn if an invalid signature is used. Don't return an error, since the ABI is not
 			// governed by the core protocol, so there may be changes to it that we don't know about
-			ops.warnf("Invalid ARC-4 ABI method signature for method op: %s", err.Error()) // nolint:errcheck
+			ops.warnf("Invalid ARC-4 ABI method signature for method op: %s", err.Error())
 		}
 		hash := sha512.Sum512_256(methodSig)
 		ops.ByteLiteral(hash[0:4])
@@ -1571,7 +1571,7 @@ func (ops *OpStream) assemble(text string) error {
 				directive := first[1:]
 				switch directive {
 				case "pragma":
-					ops.pragma(tokens)
+					ops.pragma(tokens) //nolint:errcheck // report bad pragma line error, but continue assembling
 					ops.trace("%3d: #pragma line\n", ops.sourceLine)
 				default:
 					ops.errorf("Unknown directive: %s", directive)
@@ -1618,7 +1618,8 @@ func (ops *OpStream) assemble(text string) error {
 					}
 				}
 				ops.trackStack(args, returns, append([]string{expandedName}, current[1:]...))
-				spec.asm(ops, &spec, current[1:])
+				spec.asm(ops, &spec, current[1:]) //nolint:errcheck // ignore error and continue, to collect more errors
+
 				if spec.deadens() { // An unconditional branch deadens the following code
 					ops.known.deaden()
 				}

data/transactions/logic/eval.go

@@ -1341,17 +1341,17 @@ func opLt(cx *EvalContext) error {
 // opSwap, opLt, and opNot always succeed (return nil). So error checking elided in Gt,Le,Ge
 
 func opGt(cx *EvalContext) error {
-	opSwap(cx)
+	opSwap(cx) //nolint:errcheck // opSwap always succeeds
 	return opLt(cx)
 }
 
 func opLe(cx *EvalContext) error {
-	opGt(cx)
+	opGt(cx) //nolint:errcheck // opGt always succeeds
 	return opNot(cx)
 }
 
 func opGe(cx *EvalContext) error {
-	opLt(cx)
+	opLt(cx) //nolint:errcheck // opLt always succeeds
 	return opNot(cx)
 }
 

ledger/accountdb.go

@@ -302,7 +302,7 @@ type compactAccountDeltas struct {
 }
 
 // onlineAccountDelta track all changes of account state within a range,
-// used in conjunction wih compactOnlineAccountDeltas to group and represent per-account changes.
+// used in conjunction with compactOnlineAccountDeltas to group and represent per-account changes.
 // oldAcct represents the "old" state of the account in the DB, and compared against newAcct[0]
 // to determine if the acct became online or went offline.
 type onlineAccountDelta struct {
@@ -1593,7 +1593,7 @@ func (bo *baseOnlineAccountData) SetCoreAccountData(ad *ledgercore.AccountData)
 type resourceFlags uint8
 
 const (
-	resourceFlagsHolding    resourceFlags = 0 //nolint:deadcode,varcheck
+	resourceFlagsHolding    resourceFlags = 0
 	resourceFlagsNotHolding resourceFlags = 1
 	resourceFlagsOwnership  resourceFlags = 2
 	resourceFlagsEmptyAsset resourceFlags = 4

ledger/acctonline.go

@@ -634,7 +634,6 @@ func (ao *onlineAccounts) lookupOnlineAccountData(rnd basics.Round, addr basics.
 			}
 			// the round number cannot be found in deltas, it is in history
 			inHistory = true
-			err = nil
 		}
 		paramsOffset, err = ao.roundParamsOffset(rnd)
 		if err != nil {
@@ -764,7 +763,6 @@ func (ao *onlineAccounts) TopOnlineAccounts(rnd basics.Round, voteRnd basics.Rou
 			}
 			// the round number cannot be found in deltas, it is in history
 			inMemory = false
-			err = nil
 		}
 
 		modifiedAccounts := make(map[basics.Address]*ledgercore.OnlineAccount)

network/wsNetwork.go

@@ -201,7 +201,7 @@ type GossipNode interface {
 	// this node to send corresponding MsgOfInterest notifications to any
 	// newly connecting peers.  This should be called before the network
 	// is started.
-	RegisterMessageInterest(protocol.Tag) error
+	RegisterMessageInterest(protocol.Tag)
 
 	// SubstituteGenesisID substitutes the "{genesisID}" with their network-specific genesisID.
 	SubstituteGenesisID(rawURL string) string
@@ -2308,7 +2308,7 @@ func SetUserAgentHeader(header http.Header) {
 // this node to send corresponding MsgOfInterest notifications to any
 // newly connecting peers.  This should be called before the network
 // is started.
-func (wn *WebsocketNetwork) RegisterMessageInterest(t protocol.Tag) error {
+func (wn *WebsocketNetwork) RegisterMessageInterest(t protocol.Tag) {
 	wn.messagesOfInterestMu.Lock()
 	defer wn.messagesOfInterestMu.Unlock()
 
@@ -2321,11 +2321,10 @@ func (wn *WebsocketNetwork) RegisterMessageInterest(t protocol.Tag) error {
 
 	wn.messagesOfInterest[t] = true
 	wn.updateMessagesOfInterestEnc()
-	return nil
 }
 
 // DeregisterMessageInterest will tell peers to no longer send us traffic with a protocol Tag
-func (wn *WebsocketNetwork) DeregisterMessageInterest(t protocol.Tag) error {
+func (wn *WebsocketNetwork) DeregisterMessageInterest(t protocol.Tag) {
 	wn.messagesOfInterestMu.Lock()
 	defer wn.messagesOfInterestMu.Unlock()
 
@@ -2338,7 +2337,6 @@ func (wn *WebsocketNetwork) DeregisterMessageInterest(t protocol.Tag) error {
 
 	delete(wn.messagesOfInterest, t)
 	wn.updateMessagesOfInterestEnc()
-	return nil
 }
 
 func (wn *WebsocketNetwork) updateMessagesOfInterestEnc() {

network/wsNetwork_test.go

@@ -1838,7 +1838,7 @@ func TestWebsocketNetworkMessageOfInterest(t *testing.T) {
 	waitReady(t, netB, readyTimeout.C)
 
 	// have netB asking netA to send it only AgreementVoteTag and ProposalPayloadTag
-	require.NoError(t, netB.RegisterMessageInterest(ft2))
+	netB.RegisterMessageInterest(ft2)
 	// send another message which we can track, so that we'll know that the first message was delivered.
 	netB.Broadcast(context.Background(), protocol.VoteBundleTag, []byte{0, 1, 2, 3, 4}, true, nil)
 	messageFilterArriveWg.Wait()