diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index 48f65609537..851cb51eb9b 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -11,13 +11,13 @@ jobs: go_test_short: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v2 + - uses: actions/checkout@v4 - name: Set up Go - uses: actions/setup-go@v2 + uses: actions/setup-go@v5 with: go-version: 1.21.4 - name: Run tests run: | # Upstream flakes are race conditions exacerbated by concurrent tests - FLAKY_REGEX='go-ethereum/(eth|accounts/keystore|eth/downloader|miner)$'; + FLAKY_REGEX='go-ethereum/(eth|accounts/keystore|eth/downloader|miner|ethclient/gethclient|eth/catalyst)$'; go list ./... | grep -P "${FLAKY_REGEX}" | xargs -n 1 go test -short; go test -short $(go list ./... | grep -Pv "${FLAKY_REGEX}"); diff --git a/.github/workflows/golangci-lint.yml b/.github/workflows/golangci-lint.yml new file mode 100644 index 00000000000..be2fc92a6c6 --- /dev/null +++ b/.github/workflows/golangci-lint.yml @@ -0,0 +1,25 @@ +name: golangci-lint + +on: + push: + branches: [ libevm ] + pull_request: + branches: [ libevm ] + workflow_dispatch: + +permissions: + contents: read + +jobs: + golangci: + name: lint + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-go@v5 + with: + go-version: stable + - name: golangci-lint + uses: golangci/golangci-lint-action@v6 + with: + version: v1.60 diff --git a/.golangci.yml b/.golangci.yml index 0343c4b4ebf..50be17eef8f 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -3,58 +3,112 @@ run: timeout: 20m tests: true - # default is true. Enables skipping of directories: - # vendor$, third_party$, testdata$, examples$, Godeps$, builtin$ - skip-dirs-use-default: true - skip-files: - - core/genesis_alloc.go linters: - disable-all: true enable: + # Every available linter at the time of writing was considered (quickly) and + # inclusion was liberal. Linters are good at detecting code smells, but if + # we find that a particular one causes too many false positives then we can + # configure it better or, as a last resort, remove it. + - containedctx + - errcheck + - forcetypeassert + - gci + - gocheckcompilerdirectives + - gofmt - goimports - - gosimple + - gomodguard + - gosec - govet - ineffassign + # TODO(arr4n): investigate ireturn - misspell + - nakedret + - nestif + - nilerr + - nolintlint + - reassign + - revive + - sloglint + - staticcheck + - tagliatelle + - testableexamples + - testifylint + - thelper + - tparallel - unconvert - - typecheck + - usestdlibvars - unused - - staticcheck - - bidichk - - durationcheck - - exportloopref - whitespace - # - 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 + gci: + custom-order: true + sections: + - standard + - default + - localmodule + # The rest of these break developer expections, in increasing order of + # divergence, so are at the end to increase the chance of being seen. + - alias + - dot + - blank + gomodguard: + blocked: + modules: + - github.com/ava-labs/avalanchego: + - github.com/ava-labs/coreth: + - github.com/ava-labs/subnet-evm: + revive: + rules: + - name: unused-parameter + # Method parameters may be equired by interfaces and forcing them to be + # named _ is of questionable benefit. + disabled: true issues: - exclude-rules: - - path: crypto/bn256/cloudflare/optate.go + exclude-dirs-use-default: false + exclude-rules: + - path-except: libevm linters: - - deadcode + # If any issue is flagged in a non-libevm file, add the linter here + # because the problem isn't under our control. + - containedctx + - forcetypeassert + - errcheck + - gci + - gofmt + - gosec + - gosimple + - govet + - nakedret + - nestif + - nilerr + - nolintlint + - revive - staticcheck - - path: internal/build/pgp.go - text: 'SA1019: "golang.org/x/crypto/openpgp" is deprecated: this package is unmaintained except for security fixes.' - - path: core/vm/contracts.go - text: 'SA1019: "golang.org/x/crypto/ripemd160" is deprecated: RIPEMD-160 is a legacy hash and should not be used for new applications.' - - path: accounts/usbwallet/trezor.go - text: 'SA1019: "github.com/golang/protobuf/proto" is deprecated: Use the "google.golang.org/protobuf/proto" package instead.' - - path: accounts/usbwallet/trezor/ - text: 'SA1019: "github.com/golang/protobuf/proto" is deprecated: Use the "google.golang.org/protobuf/proto" package instead.' - exclude: - - 'SA1019: event.TypeMux is deprecated: use Feed' - - 'SA1019: strings.Title is deprecated' - - 'SA1019: strings.Title has been deprecated since Go 1.18 and an alternative has been available since Go 1.0: The rule Title uses for word boundaries does not handle Unicode punctuation properly. Use golang.org/x/text/cases instead.' - - 'SA1029: should not use built-in type string as key for value' + - tagliatelle + - testableexamples + - testifylint + - thelper + - tparallel + - usestdlibvars + - varnamelen + - wastedassign + - whitespace + include: + # Many of the default exclusions are because, verbatim "Annoying issue", + # which defeats the point of a linter. + - EXC0002 + - EXC0004 + - EXC0005 + - EXC0006 + - EXC0007 + - EXC0008 + - EXC0009 + - EXC0010 + - EXC0011 + - EXC0012 + - EXC0013 + - EXC0014 + - EXC0015 diff --git a/core/state_transition.libevm_test.go b/core/state_transition.libevm_test.go index fff4267bcf0..15235b40133 100644 --- a/core/state_transition.libevm_test.go +++ b/core/state_transition.libevm_test.go @@ -4,12 +4,13 @@ import ( "fmt" "testing" + "github.com/stretchr/testify/require" + "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/core" "github.com/ethereum/go-ethereum/libevm" "github.com/ethereum/go-ethereum/libevm/ethtest" "github.com/ethereum/go-ethereum/libevm/hookstest" - "github.com/stretchr/testify/require" ) func TestCanExecuteTransaction(t *testing.T) { diff --git a/core/vm/contracts.libevm.go b/core/vm/contracts.libevm.go index 23057d6f9c8..3aa5bed9d53 100644 --- a/core/vm/contracts.libevm.go +++ b/core/vm/contracts.libevm.go @@ -3,9 +3,10 @@ package vm import ( "fmt" + "github.com/holiman/uint256" + "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/params" - "github.com/holiman/uint256" ) // evmCallArgs mirrors the parameters of the [EVM] methods Call(), CallCode(), diff --git a/core/vm/contracts.libevm_test.go b/core/vm/contracts.libevm_test.go index 585cce03f21..3b2c98a887f 100644 --- a/core/vm/contracts.libevm_test.go +++ b/core/vm/contracts.libevm_test.go @@ -4,6 +4,11 @@ import ( "fmt" "testing" + "github.com/holiman/uint256" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/exp/rand" + "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/core/vm" "github.com/ethereum/go-ethereum/crypto" @@ -11,10 +16,6 @@ import ( "github.com/ethereum/go-ethereum/libevm/ethtest" "github.com/ethereum/go-ethereum/libevm/hookstest" "github.com/ethereum/go-ethereum/params" - "github.com/holiman/uint256" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "golang.org/x/exp/rand" ) type precompileStub struct { diff --git a/libevm/ethtest/evm.go b/libevm/ethtest/evm.go index 069a949ac64..171ff60e030 100644 --- a/libevm/ethtest/evm.go +++ b/libevm/ethtest/evm.go @@ -5,13 +5,14 @@ package ethtest import ( "testing" + "github.com/stretchr/testify/require" + "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/core" "github.com/ethereum/go-ethereum/core/rawdb" "github.com/ethereum/go-ethereum/core/state" "github.com/ethereum/go-ethereum/core/vm" "github.com/ethereum/go-ethereum/params" - "github.com/stretchr/testify/require" ) // NewZeroEVM returns a new EVM backed by a [rawdb.NewMemoryDatabase]; all other diff --git a/libevm/ethtest/rand.go b/libevm/ethtest/rand.go index dfabcfedca1..cacce61f51e 100644 --- a/libevm/ethtest/rand.go +++ b/libevm/ethtest/rand.go @@ -1,8 +1,9 @@ package ethtest import ( - "github.com/ethereum/go-ethereum/common" "golang.org/x/exp/rand" + + "github.com/ethereum/go-ethereum/common" ) // PseudoRand extends [rand.Rand] (*not* crypto/rand). @@ -17,7 +18,7 @@ func NewPseudoRand(seed uint64) *PseudoRand { // Address returns a pseudorandom address. func (r *PseudoRand) Address() (a common.Address) { - r.Read(a[:]) + r.Read(a[:]) //nolint:gosec,errcheck // Guaranteed nil error return a } @@ -29,13 +30,13 @@ func (r *PseudoRand) AddressPtr() *common.Address { // Hash returns a pseudorandom hash. func (r *PseudoRand) Hash() (h common.Hash) { - r.Read(h[:]) + r.Read(h[:]) //nolint:gosec,errcheck // Guaranteed nil error return h } // Bytes returns `n` pseudorandom bytes. func (r *PseudoRand) Bytes(n uint) []byte { b := make([]byte, n) - r.Read(b) + r.Read(b) //nolint:gosec,errcheck // Guaranteed nil error return b } diff --git a/libevm/hookstest/stub.go b/libevm/hookstest/stub.go index 2915d487453..719a16664f9 100644 --- a/libevm/hookstest/stub.go +++ b/libevm/hookstest/stub.go @@ -12,9 +12,10 @@ import ( ) // Register clears any registered [params.Extras] and then registers `extras` -// for the liftime of the current test, clearing them via tb's +// for the lifetime of the current test, clearing them via tb's // [testing.TB.Cleanup]. func Register[C params.ChainConfigHooks, R params.RulesHooks](tb testing.TB, extras params.Extras[C, R]) { + tb.Helper() params.TestOnlyClearRegisteredExtras() tb.Cleanup(params.TestOnlyClearRegisteredExtras) params.RegisterExtras(extras) @@ -32,6 +33,7 @@ type Stub struct { // Register is a convenience wrapper for registering s as both the // [params.ChainConfigHooks] and [params.RulesHooks] via [Register]. func (s *Stub) Register(tb testing.TB) { + tb.Helper() Register(tb, params.Extras[*Stub, *Stub]{ NewRules: func(_ *params.ChainConfig, _ *params.Rules, _ *Stub, blockNum *big.Int, isMerge bool, timestamp uint64) *Stub { return s @@ -39,6 +41,9 @@ func (s *Stub) Register(tb testing.TB) { }) } +// PrecompileOverride uses the s.PrecompileOverrides map, if non-empty, as the +// canonical source of all overrides. If the map is empty then no precompiles +// are overridden. func (s Stub) PrecompileOverride(a common.Address) (libevm.PrecompiledContract, bool) { if len(s.PrecompileOverrides) == 0 { return nil, false @@ -47,6 +52,8 @@ func (s Stub) PrecompileOverride(a common.Address) (libevm.PrecompiledContract, return p, ok } +// CanExecuteTransaction proxies arguments to the s.CanExecuteTransactionFn +// function if non-nil, otherwise it acts as a noop. func (s Stub) CanExecuteTransaction(from common.Address, to *common.Address, sr libevm.StateReader) error { if f := s.CanExecuteTransactionFn; f != nil { return f(from, to, sr) @@ -54,6 +61,8 @@ func (s Stub) CanExecuteTransaction(from common.Address, to *common.Address, sr return nil } +// CanCreateContract proxies arguments to the s.CanCreateContractFn function if +// non-nil, otherwise it acts as a noop. func (s Stub) CanCreateContract(cc *libevm.AddressContext, sr libevm.StateReader) error { if f := s.CanCreateContractFn; f != nil { return f(cc, sr) diff --git a/libevm/libevm.go b/libevm/libevm.go index 8b7d85bd49e..bd11f971fef 100644 --- a/libevm/libevm.go +++ b/libevm/libevm.go @@ -1,8 +1,9 @@ package libevm import ( - "github.com/ethereum/go-ethereum/common" "github.com/holiman/uint256" + + "github.com/ethereum/go-ethereum/common" ) // PrecompiledContract is an exact copy of vm.PrecompiledContract, mirrored here diff --git a/libevm/pseudo/constructor_test.go b/libevm/pseudo/constructor_test.go index a28f3dd42de..7b681c48d63 100644 --- a/libevm/pseudo/constructor_test.go +++ b/libevm/pseudo/constructor_test.go @@ -14,6 +14,7 @@ func TestConstructor(t *testing.T) { testConstructor[struct{ x string }](t) } +//nolint:thelper // This is the test itself so we want local line numbers reported. func testConstructor[T any](t *testing.T) { var zero T t.Run(fmt.Sprintf("%T", zero), func(t *testing.T) { diff --git a/libevm/pseudo/type.go b/libevm/pseudo/type.go index 8d1568638ed..822198f9737 100644 --- a/libevm/pseudo/type.go +++ b/libevm/pseudo/type.go @@ -106,10 +106,10 @@ func MustNewValue[T any](t *Type) *Value[T] { } // Get returns the value. -func (a *Value[T]) Get() T { return a.t.val.get().(T) } +func (v *Value[T]) Get() T { return v.t.val.get().(T) } //nolint:forcetypeassert // invariant // Set sets the value. -func (a *Value[T]) Set(v T) { a.t.val.mustSet(v) } +func (v *Value[T]) Set(val T) { v.t.val.mustSet(val) } // MarshalJSON implements the [json.Marshaler] interface. func (t *Type) MarshalJSON() ([]byte, error) { return t.val.MarshalJSON() } diff --git a/libevm/pseudo/type_test.go b/libevm/pseudo/type_test.go index 0b25c945ce2..8a47fe562b8 100644 --- a/libevm/pseudo/type_test.go +++ b/libevm/pseudo/type_test.go @@ -24,6 +24,7 @@ func TestType(t *testing.T) { testType(t, "nil pointer", Zero[*float64], (*float64)(nil), new(float64), 0) } +//nolint:thelper // This is the test itself so we want local line numbers reported. func testType[T any](t *testing.T, name string, ctor func() *Pseudo[T], init T, setTo T, invalid any) { t.Run(name, func(t *testing.T) { typ, val := ctor().TypeAndValue() @@ -67,6 +68,7 @@ func testType[T any](t *testing.T, name string, ctor func() *Pseudo[T], init T, }) } +//nolint:ineffassign,testableexamples // Although `typ` is overwritten it's to demonstrate different approaches func ExamplePseudo_TypeAndValue() { typ, val := From("hello").TypeAndValue() @@ -98,5 +100,4 @@ func TestPointer(t *testing.T) { ptr.payload = 314159 assert.Equal(t, 314159, val.Get().payload, "after setting via pointer") }) - } diff --git a/params/config.libevm_test.go b/params/config.libevm_test.go index 1e4e309f47e..524117e81b7 100644 --- a/params/config.libevm_test.go +++ b/params/config.libevm_test.go @@ -5,9 +5,10 @@ import ( "math/big" "testing" - "github.com/ethereum/go-ethereum/libevm/pseudo" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/ethereum/go-ethereum/libevm/pseudo" ) type rawJSON struct { @@ -102,18 +103,18 @@ func TestRegisterExtras(t *testing.T) { tt.register() defer TestOnlyClearRegisteredExtras() - in := &ChainConfig{ + input := &ChainConfig{ ChainID: big.NewInt(142857), extra: tt.ccExtra, } - buf, err := json.Marshal(in) + buf, err := json.Marshal(input) require.NoError(t, err) got := new(ChainConfig) require.NoError(t, json.Unmarshal(buf, got)) assert.Equal(t, tt.ccExtra.Interface(), got.extraPayload().Interface()) - assert.Equal(t, in, got) + assert.Equal(t, input, got) gotRules := got.Rules(nil, false, 0) assert.Equal(t, tt.wantRulesExtra, gotRules.extraPayload().Interface()) diff --git a/params/example.libevm_test.go b/params/example.libevm_test.go index b38ea7b9ece..dd264e43c78 100644 --- a/params/example.libevm_test.go +++ b/params/example.libevm_test.go @@ -106,8 +106,8 @@ func (r RulesExtra) PrecompileOverride(addr common.Address) (_ libevm.Precompile // to state allows it to be configured on-chain however this is an optional // implementation detail. func (r RulesExtra) CanCreateContract(*libevm.AddressContext, libevm.StateReader) error { - if time.Unix(int64(r.timestamp), 0).UTC().Day() != int(time.Tuesday) { - return errors.New("uh oh!") + if time.Unix(int64(r.timestamp), 0).UTC().Day() != int(time.Tuesday) { //nolint:gosec // G115 timestamp won't overflow int64 for millions of years so this is someone else's problem + return errors.New("uh oh") } return nil } diff --git a/params/json.libevm_test.go b/params/json.libevm_test.go index 780b8f220e6..aa9f7928fdf 100644 --- a/params/json.libevm_test.go +++ b/params/json.libevm_test.go @@ -6,8 +6,9 @@ import ( "math/big" "testing" - "github.com/ethereum/go-ethereum/libevm/pseudo" "github.com/stretchr/testify/require" + + "github.com/ethereum/go-ethereum/libevm/pseudo" ) type nestedChainConfigExtra struct {