fixbuild: bump linter version (#3178)

golangci-lint was on older version that did not support go1.22 yet, which causes false alarms.

---

This update introduced a lot of new, deprecated and modified linters. There are a lot of changes in the files and I would advise for easier review to go over the commits, as I've tried to keep them concise, so there are only changes from one linter or if many, the changes are little. Most eye catching changes in the files are:

- `testifylint`: we had quite a lot of assertions inside of go routines, this is a bit of a red flag, because as observed in [this small example](https://go.dev/play/p/WoBGMiKQDEk), a failed assertion might or might not be caught
- The `golangci-lint` used in pre-commit was a default `golangci-lint` from [here](https://github.com/golangci/golangci-lint), using its default config, which made the linter pass locally all the time. This is something I've observed multiple times ([dating back to my very first PR](#2850 (comment)) :)). Now the pre-commit hook is running the locally installed `golangci-lint` tool and it checks if the version is the same as the one in the pipelines.
- Added `max-same-issues=0` and `max-issues-per-linter=0` so there is no limit on what we see in the output. Previously the default of 3 was used, so if there were hundreds of errors with one linter, it would have displayed only 3, which made it quite a hassle to fix.
- Bump the `golangci-lint` action's version to v6. It now points where exactly the issue is, which is great!

category: fixbuild
ticket: #3179

.github/workflows/golangci-lint.yml

@@ -17,9 +17,9 @@ jobs:
       - uses: actions/checkout@v3
       - uses: ./.github/actions/setup-go
       - name: golangci-lint
-        uses: golangci/golangci-lint-action@v3
+        uses: golangci/golangci-lint-action@v6
         with:
-          version: v1.54.2
+          version: v1.59.1
       - name: notify failure
         if: failure() && github.ref == 'refs/heads/main'
         env:

.golangci.yml

@@ -25,7 +25,8 @@ linters-settings:
       - 'fmt\.Print.*(# Avoid debug logging)?'
       - 'fmt\.Errorf.*(# Prefer app/errors.Wrap)?'
   gci:
-    local-prefixes: github.com/obolnetwork/charon
+    sections:
+      - prefix(github.com/obolnetwork/charon)
   gocritic:
     disabled-checks:
       - ifElseChain
@@ -86,6 +87,8 @@ linters-settings:
         disabled: true
       - name: comment-spacings
         disabled: true # Doesn't support latest go spec comments
+      - name: range-val-address
+        disabled: true # It is not an issue for go versions >=1.22
       # Some configured revive rules
       - name: unhandled-error
         arguments:
@@ -99,7 +102,6 @@ linters-settings:
          - "github.com/gogo/protobuf/proto" # Prefer google.golang.org/protobuf
          - "github.com/prometheus/client_golang/prometheus/promauto" # Prefer ./app/promauto
   staticcheck:
-    go: "1.22.5"
     checks:
      - "all"
      - "-SA1019" # Ignoring since github.com/drand/kyber/sign/bls uses Proof Of Possession as does Ethereum.
@@ -109,9 +111,16 @@ linters-settings:
     ignoreSigs:
       - github.com/obolnetwork/charon/
       - github.com/attestantio/go-eth2-client
+  testifylint:
+    disable:
+      - expected-actual
+    go-require:
+      ignore-http-handlers: true
 
 issues:
   fix: true
+  max-same-issues: 0
+  max-issues-per-linter: 0
   exclude-rules:
     - path: '(.+)_test\.go'
       linters:
@@ -139,8 +148,8 @@ linters:
     - containedctx
     - contextcheck
     - cyclop
-    - exhaustivestruct
     - exhaustruct
+    - exportloopref # It is not an issue for go versions >=1.22
     - funlen
     - forcetypeassert
     - gci
@@ -150,27 +159,30 @@ linters:
     - godot
     - godox
     - goerr113
-    - golint
     - gomnd
     - gomoddirectives
-    - ifshort
+    - inamedparam
     - interfacebloat
-    - interfacer
     - ireturn
     - lll # Think about adding this (max line length)
     - maintidx
-    - maligned
+    - mnd
     - musttag
     - nestif
     - nonamedreturns
     - paralleltest
     - prealloc
-    - scopelint
     - tagliatelle
     - varnamelen
     - wsl
     # Deprecated
+    - deadcode
+    - exhaustivestruct
+    - golint
+    - ifshort
+    - interfacer
+    - maligned
     - nosnakecase
     - structcheck
+    - scopelint
     - varcheck
-    - deadcode

.pre-commit-config.yaml

@@ -41,13 +41,13 @@ repos:
 
   # Then run code validators (on the formatted code)
 
-  - repo: https://github.com/golangci/golangci-lint # See .golangci.yml for config
-    rev: v1.56.2
-    hooks:
-      - id: golangci-lint
-
   - repo: local
     hooks:
+      - id: golangci-lint
+        name: golangci-lint
+        language: script
+        entry: .pre-commit/run_linter.sh
+        types: [ file, go ]
       - id: run-go-tests
         name: run-go-tests
         language: script

.pre-commit/run_linter.sh

@@ -0,0 +1,17 @@
+#!/usr/bin/env bash
+
+VERSION="1.59.1"
+
+if ! command -v golangci-lint &> /dev/null
+then
+    echo "golangci-lint could not be found"
+    exit 1
+fi
+
+version_check=$(golangci-lint version)
+if [[ $version_check != *"$VERSION"* ]]; then
+    echo $version_check
+    echo "golangci-lint version is not $VERSION"
+fi
+
+golangci-lint run

app/app.go

@@ -161,7 +161,7 @@ func Run(ctx context.Context, conf Config) (err error) {
 		return err
 	}
 
-	network, err := eth2util.ForkVersionToNetwork(cluster.ForkVersion)
+	network, err := eth2util.ForkVersionToNetwork(cluster.GetForkVersion())
 	if err != nil {
 		network = "unknown"
 	}
@@ -184,7 +184,7 @@ func Run(ctx context.Context, conf Config) (err error) {
 		return err
 	}
 
-	lockHashHex := hex7(cluster.InitialMutationHash)
+	lockHashHex := hex7(cluster.GetInitialMutationHash())
 	tcpNode, err := wireP2P(ctx, life, conf, cluster, p2pKey, lockHashHex)
 	if err != nil {
 		return err
@@ -203,16 +203,16 @@ func Run(ctx context.Context, conf Config) (err error) {
 	log.Info(ctx, "Lock file loaded",
 		z.Str("peer_name", p2p.PeerName(tcpNode.ID())),
 		z.Int("peer_index", nodeIdx.PeerIdx),
-		z.Str("cluster_name", cluster.Name),
+		z.Str("cluster_name", cluster.GetName()),
 		z.Str("cluster_hash", lockHashHex),
 		z.Str("cluster_hash_full", hex.EncodeToString(cluster.GetInitialMutationHash())),
 		z.Str("enr", enrRec.String()),
-		z.Int("peers", len(cluster.Operators)))
+		z.Int("peers", len(cluster.GetOperators())))
 
 	// Metric and logging labels.
 	labels := map[string]string{
 		"cluster_hash":    lockHashHex,
-		"cluster_name":    cluster.Name,
+		"cluster_name":    cluster.GetName(),
 		"cluster_peer":    p2p.PeerName(tcpNode.ID()),
 		"cluster_network": network,
 		"charon_version":  version.Version.String(),
@@ -223,7 +223,7 @@ func Run(ctx context.Context, conf Config) (err error) {
 		return err
 	}
 
-	initStartupMetrics(p2p.PeerName(tcpNode.ID()), int(cluster.Threshold), len(cluster.Operators), len(cluster.Validators), network)
+	initStartupMetrics(p2p.PeerName(tcpNode.ID()), int(cluster.GetThreshold()), len(cluster.GetOperators()), len(cluster.GetValidators()), network)
 
 	eth2Cl, subEth2Cl, err := newETH2Client(ctx, conf, life, cluster, cluster.GetForkVersion(), conf.BeaconNodeTimeout, conf.BeaconNodeSubmitTimeout)
 	if err != nil {
@@ -242,7 +242,7 @@ func Run(ctx context.Context, conf Config) (err error) {
 
 	sender := new(p2p.Sender)
 
-	wirePeerInfo(life, tcpNode, peerIDs, cluster.InitialMutationHash, sender, conf.BuilderAPI)
+	wirePeerInfo(life, tcpNode, peerIDs, cluster.GetInitialMutationHash(), sender, conf.BuilderAPI)
 
 	qbftDebug := newQBFTDebugger()
 
@@ -355,7 +355,7 @@ func wireCoreWorkflow(ctx context.Context, life *lifecycle.Manager, conf Config,
 		allPubSharesByKey            = make(map[core.PubKey]map[int]tbls.PublicKey) // map[pubkey]map[shareIdx]pubshare
 		feeRecipientAddrByCorePubkey = make(map[core.PubKey]string)
 	)
-	for _, val := range cluster.Validators {
+	for _, val := range cluster.GetValidators() {
 		pubkey, err := manifest.ValidatorPublicKey(val)
 		if err != nil {
 			return err
@@ -367,7 +367,7 @@ func wireCoreWorkflow(ctx context.Context, life *lifecycle.Manager, conf Config,
 		}
 
 		allPubShares := make(map[int]tbls.PublicKey)
-		for i, b := range val.PubShares {
+		for i, b := range val.GetPubShares() {
 			pubshare, err := tblsconv.PubkeyFromBytes(b)
 			if err != nil {
 				return err
@@ -390,7 +390,7 @@ func wireCoreWorkflow(ctx context.Context, life *lifecycle.Manager, conf Config,
 		corePubkeys = append(corePubkeys, corePubkey)
 		pubshares = append(pubshares, eth2Share)
 		allPubSharesByKey[corePubkey] = allPubShares
-		feeRecipientAddrByCorePubkey[corePubkey] = val.FeeRecipientAddress
+		feeRecipientAddrByCorePubkey[corePubkey] = val.GetFeeRecipientAddress()
 	}
 
 	peers, err := manifest.ClusterPeers(cluster)
@@ -483,7 +483,7 @@ func wireCoreWorkflow(ctx context.Context, life *lifecycle.Manager, conf Config,
 		return err
 	}
 
-	parSigDB := parsigdb.NewMemDB(int(cluster.Threshold), deadlinerFunc("parsigdb"))
+	parSigDB := parsigdb.NewMemDB(int(cluster.GetThreshold()), deadlinerFunc("parsigdb"))
 
 	var parSigEx core.ParSigEx
 	if conf.TestConfig.ParSigExFunc != nil {
@@ -497,7 +497,7 @@ func wireCoreWorkflow(ctx context.Context, life *lifecycle.Manager, conf Config,
 		parSigEx = parsigex.NewParSigEx(tcpNode, sender.SendAsync, nodeIdx.PeerIdx, peerIDs, verifyFunc, gaterFunc)
 	}
 
-	sigAgg, err := sigagg.New(int(cluster.Threshold), sigagg.NewVerifier(eth2Cl))
+	sigAgg, err := sigagg.New(int(cluster.GetThreshold()), sigagg.NewVerifier(eth2Cl))
 	if err != nil {
 		return err
 	}
@@ -522,13 +522,13 @@ func wireCoreWorkflow(ctx context.Context, life *lifecycle.Manager, conf Config,
 		return err
 	}
 
-	err = wirePrioritise(ctx, conf, life, tcpNode, peerIDs, int(cluster.Threshold),
+	err = wirePrioritise(ctx, conf, life, tcpNode, peerIDs, int(cluster.GetThreshold()),
 		sender.SendReceive, cons, sched, p2pKey, deadlineFunc, mutableConf)
 	if err != nil {
 		return err
 	}
 
-	if err = wireRecaster(ctx, eth2Cl, sched, sigAgg, broadcaster, cluster.Validators,
+	if err = wireRecaster(ctx, eth2Cl, sched, sigAgg, broadcaster, cluster.GetValidators(),
 		conf.BuilderAPI, conf.TestConfig.BroadcastCallback); err != nil {
 		return errors.Wrap(err, "wire recaster")
 	}
@@ -657,16 +657,16 @@ func wireRecaster(ctx context.Context, eth2Cl eth2wrap.Client, sched core.Schedu
 
 	for _, val := range validators {
 		// Check if the current cluster manifest supports pre-generate validator registrations.
-		if len(val.BuilderRegistrationJson) == 0 {
+		if len(val.GetBuilderRegistrationJson()) == 0 {
 			continue
 		}
 
 		reg := new(eth2api.VersionedSignedValidatorRegistration)
-		if err := json.Unmarshal(val.BuilderRegistrationJson, reg); err != nil {
+		if err := json.Unmarshal(val.GetBuilderRegistrationJson(), reg); err != nil {
 			return errors.Wrap(err, "unmarshal validator registration")
 		}
 
-		pubkey, err := core.PubKeyFromBytes(val.PublicKey)
+		pubkey, err := core.PubKeyFromBytes(val.GetPublicKey())
 		if err != nil {
 			return errors.Wrap(err, "core pubkey from bytes")
 		}
@@ -763,7 +763,7 @@ func calculateTrackerDelay(ctx context.Context, cl eth2wrap.Client, now time.Tim
 func eth2PubKeys(cluster *manifestpb.Cluster) ([]eth2p0.BLSPubKey, error) {
 	var pubkeys []eth2p0.BLSPubKey
 
-	for _, val := range cluster.Validators {
+	for _, val := range cluster.GetValidators() {
 		pubkey, err := manifest.ValidatorPublicKey(val)
 		if err != nil {
 			return []eth2p0.BLSPubKey{}, err
@@ -1041,7 +1041,7 @@ func setFeeRecipient(eth2Cl eth2wrap.Client, feeRecipientFunc func(core.PubKey)
 // getDVPubkeys returns DV public keys from given cluster.Lock.
 func getDVPubkeys(cluster *manifestpb.Cluster) ([]core.PubKey, error) {
 	var pubkeys []core.PubKey
-	for _, val := range cluster.Validators {
+	for _, val := range cluster.GetValidators() {
 		pk, err := manifest.ValidatorPublicKey(val)
 		if err != nil {
 			return nil, err

app/disk_internal_test.go

@@ -55,7 +55,7 @@ func TestCalculateTrackerDelay(t *testing.T) {
 
 func TestSetFeeRecipient(t *testing.T) {
 	set := beaconmock.ValidatorSetA
-	for i := 0; i < len(set); i++ {
+	for i := range len(set) {
 		clone, err := set.Clone()
 		require.NoError(t, err)
 

app/errors/errors.go

@@ -5,7 +5,6 @@
 package errors
 
 import (
-	//nolint:revive
 	stderrors "errors"
 	"fmt"
 

app/errors/go113.go

@@ -1,6 +1,6 @@
 // Copyright © 2022-2024 Obol Labs Inc. Licensed under the terms of a Business Source License 1.1
 
-//nolint:wrapcheck,revive
+//nolint:wrapcheck
 package errors
 
 import (

app/eth2wrap/eth2wrap.go

@@ -73,8 +73,6 @@ func WithSyntheticDuties(cl Client) Client {
 func NewMultiHTTP(timeout time.Duration, forkVersion [4]byte, addresses ...string) (Client, error) {
 	var clients []Client
 	for _, address := range addresses {
-		address := address // Capture range variable.
-
 		parameters := []eth2http.Parameter{
 			eth2http.WithLogLevel(zeroLogInfo),
 			eth2http.WithAddress(address),
@@ -138,10 +136,10 @@ func provide[O any](ctx context.Context, clients []Client,
 			}
 
 			return res.Output, nil
-		} else {
-			nokResp = res
-			hasNokResp = true
 		}
+
+		nokResp = res
+		hasNokResp = true
 	}
 
 	if ctx.Err() != nil {

app/eth2wrap/eth2wrap_test.go

@@ -246,7 +246,7 @@ func TestErrors(t *testing.T) {
 }
 
 func TestCtxCancel(t *testing.T) {
-	for i := 0; i < 10; i++ {
+	for range 10 {
 		ctx, cancel := context.WithCancel(context.Background())
 
 		bmock, err := beaconmock.New()
@@ -373,7 +373,7 @@ func TestOnlyTimeout(t *testing.T) {
 		if ctx.Err() != nil {
 			return
 		}
-		require.Fail(t, "Expect this only to return after main ctx cancelled")
+		require.Fail(t, "Expect this only to return after main ctx cancelled") //nolint:testifylint // TODO: find a way to do that outside of go routine
 	}()
 
 	// Allow the above goroutine to block on the .Spec() call.
@@ -392,7 +392,7 @@ func TestOnlyTimeout(t *testing.T) {
 	const n = 10
 	var wg sync.WaitGroup
 	wg.Add(n)
-	for i := 0; i < n; i++ {
+	for range n {
 		go func() {
 			testCtxCancel(t, time.Millisecond*10)
 			wg.Done()
@@ -444,7 +444,7 @@ func TestLazy(t *testing.T) {
 	enabled2.Store(true)
 
 	// Proxy2 is enabled, so this should succeed.
-	for i := 0; i < 5; i++ { // Do multiple request to make Proxy2 the "best".
+	for range 5 { // Do multiple request to make Proxy2 the "best".
 		_, err = eth2Cl.NodeSyncing(ctx, &eth2api.NodeSyncingOpts{})
 		require.NoError(t, err)
 	}

app/eth2wrap/httpwrap.go

@@ -296,6 +296,7 @@ func httpPost(ctx context.Context, base string, endpoint string, body io.Reader,
 		return nil, errors.Wrap(err, "failed to read POST response")
 	}
 
+	//nolint:usestdlibvars // we should not replace 100 with http.StatusContinue, it makes it less readable
 	if res.StatusCode/100 != 2 {
 		return nil, errors.New("post failed", z.Int("status", res.StatusCode), z.Str("body", string(data)))
 	}

app/eth2wrap/synthproposer_test.go

@@ -207,7 +207,7 @@ func TestSynthProposerBlockNotFound(t *testing.T) {
 
 		return nil, &eth2api.Error{
 			Method:     http.MethodGet,
-			Endpoint:   fmt.Sprintf("/eth/v2/beacon/blocks/%s", blockID),
+			Endpoint:   "/eth/v2/beacon/blocks/" + blockID,
 			StatusCode: http.StatusNotFound,
 			Data:       []byte(fmt.Sprintf(`{"code":404,"message":"NOT_FOUND: beacon block at slot %s","stacktraces":[]}`, blockID)),
 		}