all: more linters (ethereum#24783)

This enables the following linters

- typecheck
- unused
- staticcheck
- bidichk
- durationcheck
- exportloopref
- gosec

WIth a few exceptions.

- We use a deprecated protobuf in trezor. I didn't want to mess with that, since I cannot meaningfully test any changes there.
- The deprecated TypeMux is used in a few places still, so the warning for it is silenced for now.
- Using string type in context.WithValue is apparently wrong, one should use a custom type, to prevent collisions between different places in the hierarchy of callers. That should be fixed at some point, but may require some attention.
- The warnings for using weak random generator are squashed, since we use a lot of random without need for cryptographic guarantees.

.golangci.yml

@@ -19,32 +19,53 @@ linters:
     - govet
     - ineffassign
     - misspell
-    # - staticcheck
     - unconvert
-    # - unused
     - varcheck
+    - typecheck
+    - unused
+    - staticcheck
+    - bidichk
+    - durationcheck
+    - exportloopref
+    - gosec
+
+    #- structcheck # lots of false positives
+    #- errcheck #lot of false positives
+   # - contextcheck
+   # - errchkjson # lots of false positives
+   # - errorlint # this check crashes
+   # - exhaustive # silly check
+   # - makezero # false positives
+   # - nilerr # several intentional
 
 linters-settings:
   gofmt:
     simplify: true
   goconst:
     min-len: 3 # minimum length of string constant
     min-occurrences: 6 # minimum number of occurrences
+  gosec:
+    excludes:
+     - G404 # Use of weak random number generator - lots of FP
+     - G107 # Potential http request -- those are intentional
+     - G306 # G306: Expect WriteFile permissions to be 0600 or less
 
 issues:
   exclude-rules:
-    - path: crypto/blake2b/
-      linters:
-        - deadcode
-    - path: crypto/bn256/cloudflare
-      linters:
-        - deadcode
-    - path: p2p/discv5/
-      linters:
-        - deadcode
-    - path: core/vm/instructions_test.go
-      linters:
-        - goconst
-    - path: cmd/faucet/
+    - path: crypto/bn256/cloudflare/optate.go
       linters:
         - deadcode
+        - staticcheck
+    - path: internal/build/pgp.go
+      text: 'SA1019: package golang.org/x/crypto/openpgp is deprecated'
+    - path: core/vm/contracts.go
+      text: 'SA1019: package golang.org/x/crypto/ripemd160 is deprecated'
+    - path: accounts/usbwallet/trezor.go
+      text: 'SA1019: package github.com/golang/protobuf/proto is deprecated'
+    - path: accounts/usbwallet/trezor/
+      text: 'SA1019: package github.com/golang/protobuf/proto is deprecated'
+  exclude:
+    - 'SA1019: event.TypeMux is deprecated: use Feed'
+    - 'SA1019: strings.Title is deprecated'
+    - 'SA1029: should not use built-in type string as key for value'
+    - 'G306: Expect WriteFile permissions to be 0600 or less'

accounts/abi/abi_test.go

@@ -1038,9 +1038,7 @@ func TestABI_EventById(t *testing.T) {
 		}
 		if event == nil {
 			t.Errorf("We should find a event for topic %s, test #%d", topicID.Hex(), testnum)
-		}
-
-		if event.ID != topicID {
+		} else if event.ID != topicID {
 			t.Errorf("Event id %s does not match topic %s, test #%d", event.ID.Hex(), topicID.Hex(), testnum)
 		}
 

accounts/abi/bind/backends/simulated_test.go

@@ -658,8 +658,7 @@ func TestHeaderByNumber(t *testing.T) {
 	}
 	if latestBlockHeader == nil {
 		t.Errorf("received a nil block header")
-	}
-	if latestBlockHeader.Number.Uint64() != uint64(0) {
+	} else if latestBlockHeader.Number.Uint64() != uint64(0) {
 		t.Errorf("expected block header number 0, instead got %v", latestBlockHeader.Number.Uint64())
 	}
 

accounts/external/backend.go

@@ -152,10 +152,6 @@ func (api *ExternalSigner) SelfDerive(bases []accounts.DerivationPath, chain eth
 	log.Error("operation SelfDerive not supported on external signers")
 }
 
-func (api *ExternalSigner) signHash(account accounts.Account, hash []byte) ([]byte, error) {
-	return []byte{}, fmt.Errorf("operation not supported on external signers")
-}
-
 // SignData signs keccak256(data). The mimetype parameter describes the type of data being signed
 func (api *ExternalSigner) SignData(account accounts.Account, mimeType string, data []byte) ([]byte, error) {
 	var res hexutil.Bytes

accounts/keystore/account_cache_test.go

@@ -383,7 +383,7 @@ func TestUpdatedKeyfileContents(t *testing.T) {
 	time.Sleep(1000 * time.Millisecond)
 
 	// Now replace file contents with crap
-	if err := os.WriteFile(file, []byte("foo"), 0644); err != nil {
+	if err := os.WriteFile(file, []byte("foo"), 0600); err != nil {
 		t.Fatal(err)
 		return
 	}

accounts/keystore/passphrase_test.go

@@ -52,7 +52,7 @@ func TestKeyEncryptDecrypt(t *testing.T) {
 			t.Errorf("test %d: key address mismatch: have %x, want %x", i, key.Address, address)
 		}
 		// Recrypt with a new password and start over
-		password += "new data appended"
+		password += "new data appended" // nolint: gosec
 		if keyjson, err = EncryptKey(key, password, veryLightScryptN, veryLightScryptP); err != nil {
 			t.Errorf("test %d: failed to recrypt key %v", i, err)
 		}

cmd/devp2p/internal/ethtest/snap.go

@@ -104,6 +104,7 @@ func (s *Suite) TestSnapGetAccountRange(t *utesting.T) {
 		// Max bytes: 0. Expect to deliver one account.
 		{0, root, zero, ffHash, 1, firstKey, firstKey},
 	} {
+		tc := tc
 		if err := s.snapGetAccountRange(t, &tc); err != nil {
 			t.Errorf("test %d \n root: %x\n range: %#x - %#x\n bytes: %d\nfailed: %v", i, tc.root, tc.origin, tc.limit, tc.nBytes, err)
 		}
@@ -194,6 +195,7 @@ func (s *Suite) TestSnapGetStorageRanges(t *utesting.T) {
 			expSlots: 2,
 		},
 	} {
+		tc := tc
 		if err := s.snapGetStorageRanges(t, &tc); err != nil {
 			t.Errorf("test %d \n root: %x\n range: %#x - %#x\n bytes: %d\n #accounts: %d\nfailed: %v",
 				i, tc.root, tc.origin, tc.limit, tc.nBytes, len(tc.accounts), err)
@@ -291,6 +293,7 @@ func (s *Suite) TestSnapGetByteCodes(t *utesting.T) {
 			expHashes: 4,
 		},
 	} {
+		tc := tc
 		if err := s.snapGetByteCodes(t, &tc); err != nil {
 			t.Errorf("test %d \n bytes: %d\n #hashes: %d\nfailed: %v", i, tc.nBytes, len(tc.hashes), err)
 		}
@@ -436,6 +439,7 @@ func (s *Suite) TestSnapTrieNodes(t *utesting.T) {
 			},
 		},
 	} {
+		tc := tc
 		if err := s.snapGetTrieNodes(t, &tc); err != nil {
 			t.Errorf("test %d \n #hashes %x\n root: %#x\n bytes: %d\nfailed: %v", i, len(tc.expHashes), tc.root, tc.nBytes, err)
 		}

cmd/devp2p/internal/v5test/framework.go

@@ -60,11 +60,9 @@ type conn struct {
 	remoteAddr *net.UDPAddr
 	listeners  []net.PacketConn
 
-	log           logger
-	codec         *v5wire.Codec
-	lastRequest   v5wire.Packet
-	lastChallenge *v5wire.Whoareyou
-	idCounter     uint32
+	log       logger
+	codec     *v5wire.Codec
+	idCounter uint32
 }
 
 type logger interface {

cmd/geth/accountcmd.go

@@ -239,14 +239,15 @@ func ambiguousAddrRecovery(ks *keystore.KeyStore, err *keystore.AmbiguousAddrErr
 	}
 	fmt.Println("Testing your password against all of them...")
 	var match *accounts.Account
-	for _, a := range err.Matches {
-		if err := ks.Unlock(a, auth); err == nil {
-			match = &a
+	for i, a := range err.Matches {
+		if e := ks.Unlock(a, auth); e == nil {
+			match = &err.Matches[i]
 			break
 		}
 	}
 	if match == nil {
 		utils.Fatalf("None of the listed files could be unlocked.")
+		return accounts.Account{}
 	}
 	fmt.Printf("Your password unlocked %s\n", match.URL)
 	fmt.Println("In order to avoid this warning, you need to remove the following duplicate key files:")

cmd/geth/les_test.go

@@ -81,41 +81,6 @@ func (g *gethrpc) getNodeInfo() *p2p.NodeInfo {
 	return g.nodeInfo
 }
 
-func (g *gethrpc) waitSynced() {
-	// Check if it's synced now
-	var result interface{}
-	g.callRPC(&result, "eth_syncing")
-	syncing, ok := result.(bool)
-	if ok && !syncing {
-		g.geth.Logf("%v already synced", g.name)
-		return
-	}
-
-	// Actually wait, subscribe to the event
-	ch := make(chan interface{})
-	sub, err := g.rpc.Subscribe(context.Background(), "eth", ch, "syncing")
-	if err != nil {
-		g.geth.Fatalf("%v syncing: %v", g.name, err)
-	}
-	defer sub.Unsubscribe()
-	timeout := time.After(4 * time.Second)
-	select {
-	case ev := <-ch:
-		g.geth.Log("'syncing' event", ev)
-		syncing, ok := ev.(bool)
-		if ok && !syncing {
-			break
-		}
-		g.geth.Log("Other 'syncing' event", ev)
-	case err := <-sub.Err():
-		g.geth.Fatalf("%v notification: %v", g.name, err)
-		break
-	case <-timeout:
-		g.geth.Fatalf("%v timeout syncing", g.name)
-		break
-	}
-}
-
 // ipcEndpoint resolves an IPC endpoint based on a configured value, taking into
 // account the set data folders as well as the designated platform we're currently
 // running on.

cmd/puppeth/ssh.go

@@ -30,7 +30,7 @@ import (
 	"github.com/ethereum/go-ethereum/log"
 	"golang.org/x/crypto/ssh"
 	"golang.org/x/crypto/ssh/agent"
-	"golang.org/x/crypto/ssh/terminal"
+	"golang.org/x/term"
 )
 
 // sshClient is a small wrapper around Go's SSH client with a few utility methods
@@ -101,7 +101,7 @@ func dial(server string, pubkey []byte) (*sshClient, error) {
 			key, err := ssh.ParsePrivateKey(buf)
 			if err != nil {
 				fmt.Printf("What's the decryption password for %s? (won't be echoed)\n>", path)
-				blob, err := terminal.ReadPassword(int(os.Stdin.Fd()))
+				blob, err := term.ReadPassword(int(os.Stdin.Fd()))
 				fmt.Println()
 				if err != nil {
 					log.Warn("Couldn't read password", "err", err)
@@ -118,7 +118,7 @@ func dial(server string, pubkey []byte) (*sshClient, error) {
 		}
 		auths = append(auths, ssh.PasswordCallback(func() (string, error) {
 			fmt.Printf("What's the login password for %s at %s? (won't be echoed)\n> ", username, server)
-			blob, err := terminal.ReadPassword(int(os.Stdin.Fd()))
+			blob, err := term.ReadPassword(int(os.Stdin.Fd()))
 
 			fmt.Println()
 			return string(blob), err

cmd/puppeth/wizard.go

@@ -34,7 +34,7 @@ import (
 	"github.com/ethereum/go-ethereum/core"
 	"github.com/ethereum/go-ethereum/log"
 	"github.com/peterh/liner"
-	"golang.org/x/crypto/ssh/terminal"
+	"golang.org/x/term"
 )
 
 // config contains all the configurations needed by puppeth that should be saved
@@ -228,7 +228,7 @@ func (w *wizard) readDefaultFloat(def float64) float64 {
 // line and returns it. The input will not be echoed.
 func (w *wizard) readPassword() string {
 	fmt.Printf("> ")
-	text, err := terminal.ReadPassword(int(os.Stdin.Fd()))
+	text, err := term.ReadPassword(int(os.Stdin.Fd()))
 	if err != nil {
 		log.Crit("Failed to read password", "err", err)
 	}

cmd/utils/diskusage.go

@@ -33,6 +33,7 @@ func getFreeDiskSpace(path string) (uint64, error) {
 
 	// Available blocks * size per block = available space in bytes
 	var bavail = stat.Bavail
+	// nolint:staticcheck
 	if stat.Bavail < 0 {
 		// FreeBSD can have a negative number of blocks available
 		// because of the grace limit.

core/blockchain_sethead_test.go

@@ -51,6 +51,7 @@ type rewindTest struct {
 	expHeadBlock       uint64 // Block number of the expected head full block
 }
 
+//nolint:unused
 func (tt *rewindTest) dump(crash bool) string {
 	buffer := new(strings.Builder)
 

core/blockchain_snapshot_test.go

@@ -150,6 +150,7 @@ func (basic *snapshotTestBasic) verify(t *testing.T, chain *BlockChain, blocks [
 	}
 }
 
+//nolint:unused
 func (basic *snapshotTestBasic) dump() string {
 	buffer := new(strings.Builder)
 
@@ -341,54 +342,6 @@ func (snaptest *setHeadSnapshotTest) test(t *testing.T) {
 	snaptest.verify(t, newchain, blocks)
 }
 
-// restartCrashSnapshotTest is the test type used to test this scenario:
-// - have a complete snapshot
-// - restart chain
-// - insert more blocks with enabling the snapshot
-// - commit the snapshot
-// - crash
-// - restart again
-type restartCrashSnapshotTest struct {
-	snapshotTestBasic
-	newBlocks int
-}
-
-func (snaptest *restartCrashSnapshotTest) test(t *testing.T) {
-	// It's hard to follow the test case, visualize the input
-	// log.Root().SetHandler(log.LvlFilterHandler(log.LvlTrace, log.StreamHandler(os.Stderr, log.TerminalFormat(true))))
-	// fmt.Println(tt.dump())
-	chain, blocks := snaptest.prepare(t)
-
-	// Firstly, stop the chain properly, with all snapshot journal
-	// and state committed.
-	chain.Stop()
-
-	newchain, err := NewBlockChain(snaptest.db, nil, params.AllEthashProtocolChanges, snaptest.engine, vm.Config{}, nil, nil)
-	if err != nil {
-		t.Fatalf("Failed to recreate chain: %v", err)
-	}
-	newBlocks, _ := GenerateChain(params.TestChainConfig, blocks[len(blocks)-1], snaptest.engine, snaptest.gendb, snaptest.newBlocks, func(i int, b *BlockGen) {})
-	newchain.InsertChain(newBlocks)
-
-	// Commit the entire snapshot into the disk if requested. Note only
-	// (a) snapshot root and (b) snapshot generator will be committed,
-	// the diff journal is not.
-	newchain.Snapshots().Cap(newBlocks[len(newBlocks)-1].Root(), 0)
-
-	// Simulate the blockchain crash
-	// Don't call chain.Stop here, so that no snapshot
-	// journal and latest state will be committed
-
-	// Restart the chain after the crash
-	newchain, err = NewBlockChain(snaptest.db, nil, params.AllEthashProtocolChanges, snaptest.engine, vm.Config{}, nil, nil)
-	if err != nil {
-		t.Fatalf("Failed to recreate chain: %v", err)
-	}
-	defer newchain.Stop()
-
-	snaptest.verify(t, newchain, blocks)
-}
-
 // wipeCrashSnapshotTest is the test type used to test this scenario:
 // - have a complete snapshot
 // - restart, insert more blocks without enabling the snapshot
@@ -431,7 +384,7 @@ func (snaptest *wipeCrashSnapshotTest) test(t *testing.T) {
 		SnapshotLimit:  256,
 		SnapshotWait:   false, // Don't wait rebuild
 	}
-	newchain, err = NewBlockChain(snaptest.db, config, params.AllEthashProtocolChanges, snaptest.engine, vm.Config{}, nil, nil)
+	_, err = NewBlockChain(snaptest.db, config, params.AllEthashProtocolChanges, snaptest.engine, vm.Config{}, nil, nil)
 	if err != nil {
 		t.Fatalf("Failed to recreate chain: %v", err)
 	}

core/blockchain_test.go

@@ -2576,6 +2576,7 @@ func TestTransactionIndices(t *testing.T) {
 			t.Fatalf("failed to create temp freezer db: %v", err)
 		}
 		gspec.MustCommit(ancientDb)
+		l := l
 		chain, err = NewBlockChain(ancientDb, nil, params.TestChainConfig, ethash.NewFaker(), vm.Config{}, nil, &l)
 		if err != nil {
 			t.Fatalf("failed to create tester chain: %v", err)
@@ -2601,6 +2602,7 @@ func TestTransactionIndices(t *testing.T) {
 	limit = []uint64{0, 64 /* drop stale */, 32 /* shorten history */, 64 /* extend history */, 0 /* restore all */}
 	tails := []uint64{0, 67 /* 130 - 64 + 1 */, 100 /* 131 - 32 + 1 */, 69 /* 132 - 64 + 1 */, 0}
 	for i, l := range limit {
+		l := l
 		chain, err = NewBlockChain(ancientDb, nil, params.TestChainConfig, ethash.NewFaker(), vm.Config{}, nil, &l)
 		if err != nil {
 			t.Fatalf("failed to create tester chain: %v", err)

core/rawdb/freezer_test.go

@@ -277,7 +277,7 @@ func TestFreezerReadonlyValidate(t *testing.T) {
 
 	// Re-openening as readonly should fail when validating
 	// table lengths.
-	f, err = NewFreezer(dir, "", true, 2049, tables)
+	_, err = NewFreezer(dir, "", true, 2049, tables)
 	if err == nil {
 		t.Fatal("readonly freezer should fail with differing table lengths")
 	}

core/rawdb/freezer_utils_test.go

@@ -43,7 +43,7 @@ func TestCopyFrom(t *testing.T) {
 		{"foo", "bar", 8, true},
 	}
 	for _, c := range cases {
-		os.WriteFile(c.src, content, 0644)
+		os.WriteFile(c.src, content, 0600)
 
 		if err := copyFrom(c.src, c.dest, c.offset, func(f *os.File) error {
 			if !c.writePrefix {