Skip to content

Commit

Permalink
all: more linters (ethereum#24783)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
holiman authored Jun 13, 2022
1 parent eb94896 commit a907d7e
Show file tree
Hide file tree
Showing 67 changed files with 156 additions and 423 deletions.
51 changes: 36 additions & 15 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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'
4 changes: 1 addition & 3 deletions accounts/abi/abi_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
3 changes: 1 addition & 2 deletions accounts/abi/bind/backends/simulated_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -655,8 +655,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())
}

Expand Down
4 changes: 0 additions & 4 deletions accounts/external/backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion accounts/keystore/account_cache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
2 changes: 1 addition & 1 deletion accounts/keystore/passphrase_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
4 changes: 4 additions & 0 deletions cmd/devp2p/internal/ethtest/snap.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down
8 changes: 3 additions & 5 deletions cmd/devp2p/internal/v5test/framework.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
7 changes: 4 additions & 3 deletions cmd/geth/accountcmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -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:")
Expand Down
35 changes: 0 additions & 35 deletions cmd/geth/les_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
6 changes: 3 additions & 3 deletions cmd/puppeth/ssh.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions cmd/puppeth/wizard.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
Expand Down
1 change: 1 addition & 0 deletions cmd/utils/diskusage.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions core/blockchain_sethead_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
51 changes: 2 additions & 49 deletions core/blockchain_snapshot_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
Expand Down
2 changes: 2 additions & 0 deletions core/blockchain_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion core/rawdb/freezer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand Down
2 changes: 1 addition & 1 deletion core/rawdb/freezer_utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading

0 comments on commit a907d7e

Please sign in to comment.