lint: fix linter errors and update CI to require passing

.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.yml

@@ -7,7 +7,7 @@ linters:
   enable:
     - errcheck
     - gofmt
-    - golint
+    - revive
     - govet
     - ineffassign
     - misspell
@@ -41,6 +41,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/catchpointdump/net.go

@@ -303,15 +303,15 @@ func deleteLedgerFiles(deleteTracker bool) error {
 
 func loadAndDump(addr string, tarFile string, genesisInitState ledgercore.InitState) error {
 	// delete current ledger files.
-	deleteLedgerFiles(true)
+	deleteLedgerFiles(true) //nolint:errcheck
 	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 deleteLedgerFiles(!loadOnly) //nolint:errcheck
 	defer l.Close()
 
 	catchupAccessor := ledger.MakeCatchpointCatchupAccessor(l, logging.Base())

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/transactions/logic/assembler.go

@@ -1230,7 +1230,7 @@ func pseudoImmediatesError(ops *OpStream, name string, specs map[int]OpSpec) {
 		immediateCounts[i] = numImms
 		i++
 	}
-	ops.error(name + " expects " + joinIntsOnOr("immediate argument", immediateCounts...))
+	ops.error(name + " expects " + joinIntsOnOr("immediate argument", immediateCounts...)) //nolint:errcheck
 }
 
 // getSpec finds the OpSpec we need during assembly based on its name, our current version, and the immediates passed in
@@ -1255,7 +1255,7 @@ func getSpec(ops *OpStream, name string, args []string) (OpSpec, string, bool) {
 		}
 		pseudo.Name = name
 		if pseudo.Version > ops.Version {
-			ops.errorf("%s opcode with %s was introduced in v%d", pseudo.Name, joinIntsOnOr("immediate", len(args)), pseudo.Version)
+			ops.errorf("%s opcode with %s was introduced in v%d", pseudo.Name, joinIntsOnOr("immediate", len(args)), pseudo.Version) //nolint:errcheck
 		}
 		if len(args) == 0 {
 			return pseudo, pseudo.Name + " without immediates", true
@@ -1266,9 +1266,9 @@ func getSpec(ops *OpStream, name string, args []string) (OpSpec, string, bool) {
 	if !ok {
 		spec, ok = OpsByName[AssemblerMaxVersion][name]
 		if ok {
-			ops.errorf("%s opcode was introduced in v%d", name, spec.Version)
+			ops.errorf("%s opcode was introduced in v%d", name, spec.Version) //nolint:errcheck
 		} else {
-			ops.errorf("unknown opcode: %s", name)
+			ops.errorf("unknown opcode: %s", name) //nolint:errcheck
 		}
 	}
 	return spec, spec.Name, ok
@@ -1496,7 +1496,7 @@ func (ops *OpStream) trace(format string, args ...interface{}) {
 
 func (ops *OpStream) typeError(err error) {
 	if ops.typeTracking {
-		ops.error(err)
+		ops.error(err) //nolint:errcheck
 	}
 }
 
@@ -1571,10 +1571,10 @@ func (ops *OpStream) assemble(text string) error {
 				directive := first[1:]
 				switch directive {
 				case "pragma":
-					ops.pragma(tokens)
+					ops.pragma(tokens) //nolint:errcheck
 					ops.trace("%3d: #pragma line\n", ops.sourceLine)
 				default:
-					ops.errorf("Unknown directive: %s", directive)
+					ops.errorf("Unknown directive: %s", directive) //nolint:errcheck
 				}
 				continue
 			}
@@ -1618,8 +1618,8 @@ func (ops *OpStream) assemble(text string) error {
 					}
 				}
 				ops.trackStack(args, returns, append([]string{expandedName}, current[1:]...))
-				spec.asm(ops, &spec, current[1:])
-				if spec.deadens() { // An unconditional branch deadens the following code
+				spec.asm(ops, &spec, current[1:]) //nolint:errcheck
+				if spec.deadens() {               // An unconditional branch deadens the following code
 					ops.known.deaden()
 				}
 				if spec.Name == "callsub" {
@@ -1636,7 +1636,7 @@ func (ops *OpStream) assemble(text string) error {
 		if errors.Is(err, bufio.ErrTooLong) {
 			err = errors.New("line too long")
 		}
-		ops.error(err)
+		ops.error(err) //nolint:errcheck
 	}
 
 	// backward compatibility: do not allow jumps behind last instruction in v1
@@ -1741,12 +1741,12 @@ func (ops *OpStream) resolveLabels() {
 		// all branch instructions (currently) are opcode byte and 2 offset bytes, and the destination is relative to the next pc as if the branch was a no-op
 		naturalPc := lr.position + 3
 		if ops.Version < backBranchEnabledVersion && dest < naturalPc {
-			ops.errorf("label %#v is a back reference, back jump support was introduced in v4", lr.label)
+			ops.errorf("label %#v is a back reference, back jump support was introduced in v4", lr.label) //nolint:errcheck
 			continue
 		}
 		jump := dest - naturalPc
 		if jump > 0x7fff {
-			ops.errorf("label %#v is too far away", lr.label)
+			ops.errorf("label %#v is too far away", lr.label) //nolint:errcheck
 			continue
 		}
 		raw[lr.position+1] = uint8(jump >> 8)

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
 	return opLt(cx)
 }
 
 func opLe(cx *EvalContext) error {
-	opGt(cx)
+	opGt(cx) //nolint:errcheck
 	return opNot(cx)
 }
 
 func opGe(cx *EvalContext) error {
-	opLt(cx)
+	opLt(cx) //nolint:errcheck
 	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 {

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()

protocol/codec_test.go

@@ -199,3 +199,24 @@ func TestEncodeJSON(t *testing.T) {
 	require.True(t, reflect.DeepEqual(v, nsv))
 	require.True(t, reflect.DeepEqual(v, sv))
 }
+
+func TestRandomizeObjectWithPtrField(t *testing.T) {
+	partitiontest.PartitionTest(t)
+
+	type testObjA struct {
+		U64 uint64
+	}
+	type testObjB struct {
+		U16  uint16
+		ObjA *testObjA
+	}
+
+	obj, err := RandomizeObject(&testObjB{})
+	require.NoError(t, err)
+	objB, ok := obj.(*testObjB)
+	require.True(t, ok)
+	t.Logf("%+v %+v", objB, objB.ObjA)
+	require.NotZero(t, objB.U16)
+	require.NotZero(t, objB.ObjA)
+	require.NotZero(t, objB.ObjA.U64)
+}

protocol/codec_tester.go

@@ -17,7 +17,6 @@
 package protocol
 
 import (
-	"errors"
 	"fmt"
 	"io/ioutil"
 	"math/rand"
@@ -230,7 +229,12 @@ func randomizeValue(v reflect.Value, datapath string, tag string, remainingChang
 
 	switch v.Kind() {
 	case reflect.Uint, reflect.Uintptr, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64:
-		v.SetUint(rand.Uint64())
+		if strings.HasSuffix(datapath, "/HashType") {
+			// generate random value that will avoid protocol.ErrInvalidObject from HashType.Validate()
+			v.SetUint(rand.Uint64() % 3)
+		} else {
+			v.SetUint(rand.Uint64())
+		}
 		*remainingChanges--
 	case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
 		v.SetInt(int64(rand.Uint64()))
@@ -243,6 +247,12 @@ func randomizeValue(v reflect.Value, datapath string, tag string, remainingChang
 		}
 		v.SetString(string(buf))
 		*remainingChanges--
+	case reflect.Ptr:
+		v.Set(reflect.New(v.Type().Elem()))
+		err := randomizeValue(reflect.Indirect(v), datapath, tag, remainingChanges, seenTypes)
+		if err != nil {
+			return err
+		}
 	case reflect.Struct:
 		st := v.Type()
 		if !seenTypes[st] {
@@ -427,15 +437,7 @@ func RunEncodingTest(t *testing.T, template msgpMarshalUnmarshal) {
 			t.Skip()
 			return
 		}
-		if err == nil {
-			continue
-		}
 
-		// some objects might appen to the original error additional info.
-		// we ensure that invalidObject error is not failing the test.
-		if errors.As(err, &ErrInvalidObject) {
-			continue
-		}
 		require.NoError(t, err)
 	}
 }

test/e2e-go/cli/goal/expect/catchpointCatchupWebProxy/webproxy.go

@@ -58,7 +58,7 @@ func main() {
 		// prevent requests for block #2 to go through.
 		if strings.HasSuffix(request.URL.String(), "/block/2") {
 			response.WriteHeader(http.StatusBadRequest)
-			response.Write([]byte("webProxy prevents block 2 from serving"))
+			response.Write([]byte("webProxy prevents block 2 from serving")) //nolint:errcheck
 			return
 		}
 		if *webProxyLogFile != "" {