From bfff7038dec4290513a6e38d37de4700cd51b790 Mon Sep 17 00:00:00 2001 From: corver Date: Thu, 26 Jan 2023 17:14:11 +0200 Subject: [PATCH] *: fix multiple lints (#1693) Fixes lints raised by upgraded golangci-lint, also improves lint config. category: fixbuild ticket: none --- .golangci.yml | 8 +- app/app.go | 2 +- app/errors/errors.go | 2 +- app/eth2wrap/httpwrap.go | 2 +- app/eth2wrap/synthproposer.go | 2 +- cluster/definition.go | 18 ++--- cmd/relay/p2p.go | 2 +- core/deadline.go | 2 +- core/infosync/infosync.go | 2 +- core/qbft/qbft.go | 2 +- core/tracker/tracker.go | 6 +- core/tracker/tracker_internal_test.go | 4 +- dkg/dkg.go | 2 +- eth2util/keymanager/keymanager.go | 4 +- p2p/p2p.go | 87 +++++++++++++-------- testutil/genchangelog/main_internal_test.go | 2 +- testutil/validatormock/validatormock.go | 2 +- 17 files changed, 87 insertions(+), 62 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 3d165b484..9eb1800f4 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -13,6 +13,7 @@ linters-settings: forbidigo: forbid: - 'fmt\.Print.*(# Avoid debug logging)?' + - 'fmt\.Errorf.*(# Prefer app/errors.Wrap)?' gci: local-prefixes: github.com/obolnetwork/charon gocritic: @@ -50,8 +51,11 @@ linters-settings: disabled: true - name: cognitive-complexity disabled: true - # Some configured revive rules + - name: unhandled-error + arguments: + - 'fmt.Printf' + - 'fmt.Println' - name: imports-blacklist arguments: - "errors" # Prefer ./app/errors @@ -94,6 +98,7 @@ linters: disable: # Keep disabled - containedctx + - contextcheck - cyclop - exhaustivestruct - exhaustruct @@ -108,6 +113,7 @@ linters: - gomnd - gomoddirectives - ifshort + - interfacebloat - interfacer - ireturn - lll # Think about adding this (max line length) diff --git a/app/app.go b/app/app.go index 289525019..e736f1b02 100644 --- a/app/app.go +++ b/app/app.go @@ -294,7 +294,7 @@ func wireP2P(ctx context.Context, life *lifecycle.Manager, conf Config, conf.TestConfig.TCPNodeCallback(tcpNode) } - p2p.RegisterConnectionLogger(tcpNode, peerIDs) + p2p.RegisterConnectionLogger(ctx, tcpNode, peerIDs) life.RegisterStop(lifecycle.StopP2PTCPNode, lifecycle.HookFuncErr(tcpNode.Close)) diff --git a/app/errors/errors.go b/app/errors/errors.go index ae47a360d..56a8c22aa 100644 --- a/app/errors/errors.go +++ b/app/errors/errors.go @@ -86,7 +86,7 @@ func SkipWrap(err error, msg string, skip int, fields ...z.Field) error { } return structured{ - err: fmt.Errorf("%s: %w", msg, err), // Wrap error message. + err: fmt.Errorf("%s: %w", msg, err), //nolint: forbidigo // Wrap error message using stdlib. fields: fields, stack: stack, } diff --git a/app/eth2wrap/httpwrap.go b/app/eth2wrap/httpwrap.go index 9a4f7f235..fe0daf3ed 100644 --- a/app/eth2wrap/httpwrap.go +++ b/app/eth2wrap/httpwrap.go @@ -50,7 +50,7 @@ type NodePeerCountProvider interface { } // NewHTTPAdapterForT returns a http adapter for testing non-eth2service methods as it is nil. -func NewHTTPAdapterForT(_ *testing.T, address string, timeout time.Duration) *httpAdapter { +func NewHTTPAdapterForT(_ *testing.T, address string, timeout time.Duration) Client { return newHTTPAdapter(nil, address, timeout) } diff --git a/app/eth2wrap/synthproposer.go b/app/eth2wrap/synthproposer.go index 73d6b0c32..b2d788acd 100644 --- a/app/eth2wrap/synthproposer.go +++ b/app/eth2wrap/synthproposer.go @@ -405,7 +405,7 @@ func getStandardHashFn() shuffle.HashFn { hash := sha256.New() hashFn := func(in []byte) []byte { hash.Reset() - hash.Write(in) + _, _ = hash.Write(in) return hash.Sum(nil) } diff --git a/cluster/definition.go b/cluster/definition.go index 6e09ebc0d..95dab7579 100644 --- a/cluster/definition.go +++ b/cluster/definition.go @@ -344,21 +344,21 @@ func (d Definition) SetDefinitionHashes() (Definition, error) { } func (d Definition) MarshalJSON() ([]byte, error) { - d, err := d.SetDefinitionHashes() + d2, err := d.SetDefinitionHashes() if err != nil { return nil, err } switch { - case isV1x0(d.Version) || isV1x1(d.Version): - return marshalDefinitionV1x0or1(d) - case isV1x2(d.Version) || isV1x3(d.Version): + case isV1x0(d2.Version) || isV1x1(d2.Version): + return marshalDefinitionV1x0or1(d2) + case isV1x2(d2.Version) || isV1x3(d2.Version): // v1.2 and v1.3 has the same json format. - return marshalDefinitionV1x2or3(d) - case isV1x4(d.Version): - return marshalDefinitionV1x4(d) - case isV1x5(d.Version): - return marshalDefinitionV1x5(d) + return marshalDefinitionV1x2or3(d2) + case isV1x4(d2.Version): + return marshalDefinitionV1x4(d2) + case isV1x5(d2.Version): + return marshalDefinitionV1x5(d2) default: return nil, errors.New("unsupported version") } diff --git a/cmd/relay/p2p.go b/cmd/relay/p2p.go index 160fec490..2447a2023 100644 --- a/cmd/relay/p2p.go +++ b/cmd/relay/p2p.go @@ -67,7 +67,7 @@ func startP2P(ctx context.Context, config Config, key *k1.PrivateKey, reporter m return nil, errors.Wrap(err, "new tcp node") } - p2p.RegisterConnectionLogger(tcpNode, nil) + p2p.RegisterConnectionLogger(ctx, tcpNode, nil) // Reservations are valid for 30min (github.com/libp2p/go-libp2p/p2p/protocol/circuitv2/relay/constraints.go:14) relayResources := relay.DefaultResources() diff --git a/core/deadline.go b/core/deadline.go index 2a2c704e3..71a4ca61f 100644 --- a/core/deadline.go +++ b/core/deadline.go @@ -32,7 +32,7 @@ import ( const lateFactor = 5 // lateMin defines the minimum absolute value of the lateFactor. -const lateMin = time.Second * 30 +const lateMin = time.Second * 30 //nolint:revive // Min suffix is minimum not minute. // Deadliner provides duty Deadline functionality. The C method isn’t thread safe and // may only be used by a single goroutine. So, multiple instances are required diff --git a/core/infosync/infosync.go b/core/infosync/infosync.go index 0f87fbbce..9b5dc1e30 100644 --- a/core/infosync/infosync.go +++ b/core/infosync/infosync.go @@ -40,7 +40,7 @@ const ( // New returns a new infosync component. func New(prioritiser *priority.Component, versions []string, protocols []protocol.ID) *Component { - // Add a mock alpha protocol if alpha features enabled in order to to test infosync in prod. + // Add a mock alpha protocol if alpha features enabled in order to test infosync in prod. // TODO(corver): Remove this once we have an actual use case. if featureset.Enabled(featureset.MockAlpha) { protocols = append(protocols, "/charon/mock_alpha/1.0.0") diff --git a/core/qbft/qbft.go b/core/qbft/qbft.go index 34cf49a31..28f1383eb 100644 --- a/core/qbft/qbft.go +++ b/core/qbft/qbft.go @@ -181,7 +181,7 @@ func Run[I any, V comparable](ctx context.Context, d Definition[I, V], t Transpo if !strings.Contains(fmt.Sprint(r), "bug") { panic(r) // Only catch internal sanity checks. } - err = fmt.Errorf("qbft sanity check: %v", r) + err = fmt.Errorf("qbft sanity check: %v", r) //nolint: forbidigo // Wrapping a panic, not error. } }() diff --git a/core/tracker/tracker.go b/core/tracker/tracker.go index 2b2adfdfa..a4c44ab0e 100644 --- a/core/tracker/tracker.go +++ b/core/tracker/tracker.go @@ -252,7 +252,7 @@ func (t *Tracker) Run(ctx context.Context) error { case duty := <-t.analyser.C(): ctx := log.WithCtx(ctx, z.Any("duty", duty)) - parsigs := analyseParSigs(t.events[duty]) + parsigs := analyseParSigs(ctx, t.events[duty]) t.parSigReporter(ctx, duty, parsigs) // Analyse failed duties @@ -339,7 +339,7 @@ func analyseDutyFailed(duty core.Duty, allEvents map[core.Duty][]event, msgRootC } // analyseParSigs returns a mapping of partial signed data messages by peers (share index) by validator (pubkey). -func analyseParSigs(events []event) parsigsByMsg { +func analyseParSigs(ctx context.Context, events []event) parsigsByMsg { type dedupKey struct { Pubkey core.PubKey ShareIdx int @@ -361,7 +361,7 @@ func analyseParSigs(events []event) parsigsByMsg { root, err := e.parSig.MessageRoot() if err != nil { - log.Warn(context.Background(), "Parsig message root", err) + log.Warn(ctx, "Parsig message root", err) continue // Just log and ignore as this is highly unlikely and non-critical code. } diff --git a/core/tracker/tracker_internal_test.go b/core/tracker/tracker_internal_test.go index c2bbd7988..96710517a 100644 --- a/core/tracker/tracker_internal_test.go +++ b/core/tracker/tracker_internal_test.go @@ -943,7 +943,7 @@ func TestIsParSigEventExpected(t *testing.T) { } func TestAnalyseParSigs(t *testing.T) { - require.Empty(t, analyseParSigs(nil)) + require.Empty(t, analyseParSigs(context.Background(), nil)) var events []event @@ -972,7 +972,7 @@ func TestAnalyseParSigs(t *testing.T) { makeEvents(n, pubkey) } - allParSigMsgs := analyseParSigs(events) + allParSigMsgs := analyseParSigs(context.Background(), events) lengths := make(map[int]string) for pubkey, parSigMsgs := range allParSigMsgs { diff --git a/dkg/dkg.go b/dkg/dkg.go index ea73fb9ec..874531230 100644 --- a/dkg/dkg.go +++ b/dkg/dkg.go @@ -261,7 +261,7 @@ func setupP2P(ctx context.Context, key *k1.PrivateKey, p2pConf p2p.Config, peers return nil, nil, err } - p2p.RegisterConnectionLogger(tcpNode, peerIDs) + p2p.RegisterConnectionLogger(ctx, tcpNode, peerIDs) for _, relay := range relays { go func(relay *p2p.MutablePeer) { diff --git a/eth2util/keymanager/keymanager.go b/eth2util/keymanager/keymanager.go index c7d2d3b16..ad10b4003 100644 --- a/eth2util/keymanager/keymanager.go +++ b/eth2util/keymanager/keymanager.go @@ -84,7 +84,7 @@ func (c Client) VerifyConnection(ctx context.Context) error { if err != nil { return errors.Wrap(err, "cannot ping address", z.Str("addr", c.baseURL)) } - conn.Close() + _ = conn.Close() return nil } @@ -121,7 +121,7 @@ func postKeys(ctx context.Context, addr string, reqBody keymanagerReq) error { if err != nil { return errors.Wrap(err, "read response") } - resp.Body.Close() + _ = resp.Body.Close() if resp.StatusCode/100 != 2 { return errors.New("failed posting keys", z.Int("status", resp.StatusCode), z.Str("body", string(data))) diff --git a/p2p/p2p.go b/p2p/p2p.go index 22af0dcdd..e1252ae12 100644 --- a/p2p/p2p.go +++ b/p2p/p2p.go @@ -191,7 +191,11 @@ func (f peerRoutingFunc) FindPeer(ctx context.Context, p peer.ID) (peer.AddrInfo // RegisterConnectionLogger registers a connection logger with the host. // This is pretty weird and hacky, but that is because libp2p uses the network.Notifiee interface as a map key, // so the implementation can only contain fields that are hashable. So we use a channel and do the logic externally. :(. -func RegisterConnectionLogger(tcpNode host.Host, peerIDs []peer.ID) { +func RegisterConnectionLogger(ctx context.Context, tcpNode host.Host, peerIDs []peer.ID) { + ctx = log.WithTopic(ctx, "p2p") + quit := make(chan struct{}) + defer close(quit) + var ( peers = make(map[peer.ID]bool) events = make(chan logEvent) @@ -203,42 +207,47 @@ func RegisterConnectionLogger(tcpNode host.Host, peerIDs []peer.ID) { tcpNode.Network().Notify(connLogger{ events: events, + quit: quit, }) go func() { - ctx := log.WithTopic(context.Background(), "p2p") - for e := range events { - addr := NamedAddr(e.Addr) - name := PeerName(e.Peer) - typ := addrType(e.Addr) - - if e.Listen { - log.Debug(ctx, "Libp2p listening on address", z.Str("address", addr)) - continue - } else if e.Connected { - log.Debug(ctx, "Libp2p new connection", - z.Str("peer", name), - z.Any("peer_address", addr), - z.Any("direction", e.Direction), - z.Str("type", typ), - ) - } + for { + select { + case <-ctx.Done(): + return + case e := <-events: + addr := NamedAddr(e.Addr) + name := PeerName(e.Peer) + typ := addrType(e.Addr) + + if e.Listen { + log.Debug(ctx, "Libp2p listening on address", z.Str("address", addr)) + continue + } else if e.Connected { + log.Debug(ctx, "Libp2p new connection", + z.Str("peer", name), + z.Any("peer_address", addr), + z.Any("direction", e.Direction), + z.Str("type", typ), + ) + } - if !peers[e.Peer] { - // Do not instrument relays. - continue - } + if !peers[e.Peer] { + // Do not instrument relays. + continue + } - if e.Connected { - peerConnGauge.WithLabelValues(name, addrType(e.Addr)).Inc() - peerConnCounter.WithLabelValues(name).Inc() - } else if e.Disconnect { - peerConnGauge.WithLabelValues(name, addrType(e.Addr)).Dec() - } + if e.Connected { + peerConnGauge.WithLabelValues(name, addrType(e.Addr)).Inc() + peerConnCounter.WithLabelValues(name).Inc() + } else if e.Disconnect { + peerConnGauge.WithLabelValues(name, addrType(e.Addr)).Dec() + } - // Ensure both connection type metrics are initiated - peerConnGauge.WithLabelValues(name, addrTypeDirect).Add(0) - peerConnGauge.WithLabelValues(name, addrTypeRelay).Add(0) + // Ensure both connection type metrics are initiated + peerConnGauge.WithLabelValues(name, addrTypeDirect).Add(0) + peerConnGauge.WithLabelValues(name, addrTypeRelay).Add(0) + } } }() } @@ -256,34 +265,44 @@ type logEvent struct { // connLogger implements network.Notifee and only sends logEvents on a channel since // it is used as a map key internally in libp2p, it cannot contain complex types. type connLogger struct { + quit chan struct{} events chan logEvent } func (l connLogger) Listen(_ network.Network, addr ma.Multiaddr) { - l.events <- logEvent{ + select { + case <-l.quit: + case l.events <- logEvent{ Addr: addr, Listen: true, + }: } } func (connLogger) ListenClose(network.Network, ma.Multiaddr) {} func (l connLogger) Connected(_ network.Network, conn network.Conn) { - l.events <- logEvent{ + select { + case <-l.quit: + case l.events <- logEvent{ Peer: conn.RemotePeer(), Addr: conn.RemoteMultiaddr(), Direction: conn.Stat().Direction, Connected: true, ConnID: conn.ID(), + }: } } func (l connLogger) Disconnected(_ network.Network, conn network.Conn) { - l.events <- logEvent{ + select { + case <-l.quit: + case l.events <- logEvent{ Peer: conn.RemotePeer(), Addr: conn.RemoteMultiaddr(), Disconnect: true, ConnID: conn.ID(), + }: } } diff --git a/testutil/genchangelog/main_internal_test.go b/testutil/genchangelog/main_internal_test.go index c856b6ae8..6c289982e 100644 --- a/testutil/genchangelog/main_internal_test.go +++ b/testutil/genchangelog/main_internal_test.go @@ -76,7 +76,7 @@ func TestPRFromLog(t *testing.T) { }, { in: log{ - Subject: "build(deps): blah blah", + Subject: "build(deps): foo bar", }, }, } diff --git a/testutil/validatormock/validatormock.go b/testutil/validatormock/validatormock.go index 39ff12d0f..bea992f69 100644 --- a/testutil/validatormock/validatormock.go +++ b/testutil/validatormock/validatormock.go @@ -503,7 +503,7 @@ func httpGet(ctx context.Context, base string, endpoint string) ([]byte, error) } defer res.Body.Close() - if res.StatusCode == 404 { + if res.StatusCode == http.StatusNotFound { // Nothing found. This is not an error, so we return nil on both counts. return nil, nil }