Miscellaneous code quality improvements (#7414)

* anti-patterns

* performance issues

* handle skipped defer

* gazelle fix

* misc bug risks

* make logging of proposer slashings more robust

* simplify calling span.End()

Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>

beacon-chain/blockchain/receive_block.go

@@ -143,6 +143,8 @@ func (s *Service) ReceiveBlockBatch(ctx context.Context, blocks []*ethpb.SignedB
 	}
 
 	if err := s.VerifyWeakSubjectivityRoot(s.ctx); err != nil {
+		// log.Fatalf will prevent defer from being called
+		span.End()
 		// Exit run time if the node failed to verify weak subjectivity checkpoint.
 		log.Fatalf("Could not verify weak subjectivity checkpoint: %v", err)
 	}

beacon-chain/cache/feature_flag_test.go

@@ -10,5 +10,8 @@ import (
 func TestMain(m *testing.M) {
 	resetCfg := featureconfig.InitWithReset(&featureconfig.Flags{EnableEth1DataVoteCache: true})
 	defer resetCfg()
-	os.Exit(m.Run())
+	code := m.Run()
+	// os.Exit will prevent defer from being called
+	resetCfg()
+	os.Exit(code)
 }

beacon-chain/cache/subnet_ids_test.go

@@ -42,12 +42,10 @@ func TestSubnetIDsCache_RoundTrip(t *testing.T) {
 }
 
 func TestSubnetIDsCache_PersistentCommitteeRoundtrip(t *testing.T) {
-	pubkeySet := [][48]byte{}
 	c := newSubnetIDs()
 
 	for i := 0; i < 20; i++ {
 		pubkey := [48]byte{byte(i)}
-		pubkeySet = append(pubkeySet, pubkey)
 		c.AddPersistentCommittee(pubkey[:], []uint64{uint64(i)}, 0)
 	}
 

beacon-chain/core/blocks/spectest/deposit_test.go

@@ -1,13 +1,11 @@
 package spectest
 
 import (
-	"context"
 	"path"
 	"testing"
 
 	ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1"
 	"github.com/prysmaticlabs/prysm/beacon-chain/core/blocks"
-	"github.com/prysmaticlabs/prysm/beacon-chain/state"
 	"github.com/prysmaticlabs/prysm/shared/params/spectest"
 	"github.com/prysmaticlabs/prysm/shared/testutil"
 	"github.com/prysmaticlabs/prysm/shared/testutil/require"
@@ -26,9 +24,7 @@ func runDepositTest(t *testing.T, config string) {
 			require.NoError(t, deposit.UnmarshalSSZ(depositFile), "Failed to unmarshal")
 
 			body := &ethpb.BeaconBlockBody{Deposits: []*ethpb.Deposit{deposit}}
-			testutil.RunBlockOperationTest(t, folderPath, body, func(ctx context.Context, state *state.BeaconState, b *ethpb.SignedBeaconBlock) (*state.BeaconState, error) {
-				return blocks.ProcessDeposits(ctx, state, b)
-			})
+			testutil.RunBlockOperationTest(t, folderPath, body, blocks.ProcessDeposits)
 		})
 	}
 }

beacon-chain/core/helpers/signing_root.go

@@ -134,10 +134,8 @@ func RetrieveBlockSignatureSet(blk *ethpb.BeaconBlock, pub []byte, signature []b
 	if err != nil {
 		return nil, errors.Wrap(err, "could not convert bytes to public key")
 	}
-	root, err := signingData(func() ([32]byte, error) {
-		// utilize custom block hashing function
-		return blk.HashTreeRoot()
-	}, domain)
+	// utilize custom block hashing function
+	root, err := signingData(blk.HashTreeRoot, domain)
 	if err != nil {
 		return nil, errors.Wrap(err, "could not compute signing root")
 	}
@@ -158,9 +156,7 @@ func VerifyBlockHeaderSigningRoot(blkHdr *ethpb.BeaconBlockHeader, pub []byte, s
 	if err != nil {
 		return errors.Wrap(err, "could not convert bytes to signature")
 	}
-	root, err := signingData(func() ([32]byte, error) {
-		return blkHdr.HashTreeRoot()
-	}, domain)
+	root, err := signingData(blkHdr.HashTreeRoot, domain)
 	if err != nil {
 		return errors.Wrap(err, "could not compute signing root")
 	}

beacon-chain/forkchoice/protoarray/helpers_test.go

@@ -141,8 +141,9 @@ func TestComputeDelta_MoveOutOfTree(t *testing.T) {
 
 	indices[indexToHash(1)] = 0
 
-	votes = append(votes, Vote{indexToHash(1), params.BeaconConfig().ZeroHash, 0})
-	votes = append(votes, Vote{indexToHash(1), [32]byte{'A'}, 0})
+	votes = append(votes,
+		Vote{indexToHash(1), params.BeaconConfig().ZeroHash, 0},
+		Vote{indexToHash(1), [32]byte{'A'}, 0})
 
 	delta, _, err := computeDeltas(context.Background(), indices, votes, oldBalances, newBalances)
 	require.NoError(t, err)
@@ -201,8 +202,9 @@ func TestComputeDelta_ValidatorAppear(t *testing.T) {
 	indices[indexToHash(1)] = 0
 	indices[indexToHash(2)] = 1
 
-	votes = append(votes, Vote{indexToHash(1), indexToHash(2), 0})
-	votes = append(votes, Vote{indexToHash(1), indexToHash(2), 0})
+	votes = append(votes,
+		Vote{indexToHash(1), indexToHash(2), 0},
+		Vote{indexToHash(1), indexToHash(2), 0})
 
 	delta, _, err := computeDeltas(context.Background(), indices, votes, oldBalances, newBalances)
 	require.NoError(t, err)
@@ -225,8 +227,9 @@ func TestComputeDelta_ValidatorDisappears(t *testing.T) {
 	indices[indexToHash(1)] = 0
 	indices[indexToHash(2)] = 1
 
-	votes = append(votes, Vote{indexToHash(1), indexToHash(2), 0})
-	votes = append(votes, Vote{indexToHash(1), indexToHash(2), 0})
+	votes = append(votes,
+		Vote{indexToHash(1), indexToHash(2), 0},
+		Vote{indexToHash(1), indexToHash(2), 0})
 
 	delta, _, err := computeDeltas(context.Background(), indices, votes, oldBalances, newBalances)
 	require.NoError(t, err)

beacon-chain/p2p/peers/peers_test.go

@@ -29,7 +29,11 @@ func TestMain(m *testing.M) {
 	defer func() {
 		flags.Init(resetFlags)
 	}()
-	os.Exit(m.Run())
+	code := m.Run()
+	// os.Exit will prevent defer from being called
+	resetCfg()
+	flags.Init(resetFlags)
+	os.Exit(code)
 }
 
 // roundScore returns score rounded in accordance with the score manager's rounding factor.

beacon-chain/p2p/peers/score_block_providers_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"sort"
+	"strconv"
 	"testing"
 
 	"github.com/libp2p/go-libp2p-core/peer"
@@ -180,7 +181,7 @@ func TestPeerScorer_BlockProvider_WeightSorted(t *testing.T) {
 
 	var pids []peer.ID
 	for i := uint64(0); i < 10; i++ {
-		pid := peer.ID(i)
+		pid := peer.ID(strconv.FormatUint(i, 10))
 		scorer.IncrementProcessedBlocks(pid, i*batchSize)
 		pids = append(pids, pid)
 	}

beacon-chain/p2p/peers/status_test.go

@@ -601,9 +601,9 @@ func TestBestFinalized_returnsMaxValue(t *testing.T) {
 	})
 
 	for i := 0; i <= maxPeers+100; i++ {
-		p.Add(new(enr.Record), peer.ID(i), nil, network.DirOutbound)
-		p.SetConnectionState(peer.ID(i), peers.PeerConnected)
-		p.SetChainState(peer.ID(i), &pb.Status{
+		p.Add(new(enr.Record), peer.ID(rune(i)), nil, network.DirOutbound)
+		p.SetConnectionState(peer.ID(rune(i)), peers.PeerConnected)
+		p.SetChainState(peer.ID(rune(i)), &pb.Status{
 			FinalizedEpoch: 10,
 		})
 	}
@@ -624,9 +624,9 @@ func TestStatus_BestNonFinalized(t *testing.T) {
 
 	peerSlots := []uint64{32, 32, 32, 32, 235, 233, 258, 268, 270}
 	for i, headSlot := range peerSlots {
-		p.Add(new(enr.Record), peer.ID(i), nil, network.DirOutbound)
-		p.SetConnectionState(peer.ID(i), peers.PeerConnected)
-		p.SetChainState(peer.ID(i), &pb.Status{
+		p.Add(new(enr.Record), peer.ID(rune(i)), nil, network.DirOutbound)
+		p.SetConnectionState(peer.ID(rune(i)), peers.PeerConnected)
+		p.SetChainState(peer.ID(rune(i)), &pb.Status{
 			HeadSlot: headSlot,
 		})
 	}

beacon-chain/p2p/service.go

@@ -369,13 +369,16 @@ func (s *Service) pingPeers() {
 func (s *Service) awaitStateInitialized() {
 	stateChannel := make(chan *feed.Event, 1)
 	stateSub := s.stateNotifier.StateFeed().Subscribe(stateChannel)
-	defer stateSub.Unsubscribe()
+	cleanup := stateSub.Unsubscribe
+	defer cleanup()
 	for {
 		select {
 		case event := <-stateChannel:
 			if event.Type == statefeed.Initialized {
 				data, ok := event.Data.(*statefeed.InitializedData)
 				if !ok {
+					// log.Fatalf will prevent defer from being called
+					cleanup()
 					log.Fatalf("Received wrong data over state initialized feed: %v", data)
 				}
 				s.genesisTime = data.StartTime

beacon-chain/state/stateutil/stateutil_test.go

@@ -10,5 +10,8 @@ import (
 func TestMain(m *testing.M) {
 	resetCfg := featureconfig.InitWithReset(&featureconfig.Flags{EnableSSZCache: true})
 	defer resetCfg()
-	os.Exit(m.Run())
+	code := m.Run()
+	// os.Exit will prevent defer from being called
+	resetCfg()
+	os.Exit(code)
 }

beacon-chain/sync/initial-sync/initial_sync_test.go

@@ -68,7 +68,11 @@ func TestMain(m *testing.M) {
 	defer func() {
 		flags.Init(resetFlags)
 	}()
-	os.Exit(m.Run())
+	code := m.Run()
+	// os.Exit will prevent defer from being called
+	resetCfg()
+	flags.Init(resetFlags)
+	os.Exit(code)
 }
 
 func initializeTestServices(t *testing.T, blocks []uint64, peers []*peerData) (*mock.ChainService, *p2pt.TestP2P, db.Database) {

beacon-chain/sync/utils_test.go

@@ -55,14 +55,11 @@ func TestSortedObj_NoDuplicates(t *testing.T) {
 		slot := uint64(randFunc())
 		newBlk := &ethpb.SignedBeaconBlock{Block: &ethpb.BeaconBlock{Slot: slot}}
 		// append twice
-		blks = append(blks, newBlk)
-		blks = append(blks, newBlk)
+		blks = append(blks, newBlk, newBlk)
 
 		// append twice
 		root := bytesutil.ToBytes32(bytesutil.Bytes32(slot))
-		roots = append(roots, root)
-		roots = append(roots, root)
-
+		roots = append(roots, root, root)
 	}
 
 	r := &Service{}

shared/aggregation/attestations/attestations_test.go

@@ -26,7 +26,10 @@ func TestMain(m *testing.M) {
 		AttestationAggregationStrategy: string(MaxCoverAggregation),
 	})
 	defer resetCfg()
-	os.Exit(m.Run())
+	code := m.Run()
+	// os.Exit will prevent defer from being called
+	resetCfg()
+	os.Exit(code)
 }
 
 func TestAggregateAttestations_AggregatePair(t *testing.T) {

shared/messagehandler/messagehandler_test.go

@@ -15,7 +15,7 @@ import (
 func TestSafelyHandleMessage(t *testing.T) {
 	hook := logTest.NewGlobal()
 
-	messagehandler.SafelyHandleMessage(nil, func(_ context.Context, _ proto.Message) error {
+	messagehandler.SafelyHandleMessage(context.Background(), func(_ context.Context, _ proto.Message) error {
 		panic("bad!")
 		return nil
 	}, &ethpb.BeaconBlock{})
@@ -26,7 +26,7 @@ func TestSafelyHandleMessage(t *testing.T) {
 func TestSafelyHandleMessage_NoData(t *testing.T) {
 	hook := logTest.NewGlobal()
 
-	messagehandler.SafelyHandleMessage(nil, func(_ context.Context, _ proto.Message) error {
+	messagehandler.SafelyHandleMessage(context.Background(), func(_ context.Context, _ proto.Message) error {
 		panic("bad!")
 		return nil
 	}, nil)

shared/slotutil/slotticker_test.go

@@ -131,8 +131,6 @@ func TestGetSlotTickerWithOffset_OK(t *testing.T) {
 				t.Fatal("Expected normal ticker to tick first")
 			}
 			firstTicked = 1
-			break
 		}
 	}
-
 }

slasher/db/kv/proposer_slashings_test.go

@@ -138,9 +138,15 @@ func TestStore_SaveProposerSlashing(t *testing.T) {
 		proposerSlashings, err := db.ProposalSlashingsByStatus(ctx, tt.ss)
 		require.NoError(t, err, "Failed to get proposer slashings")
 
+		var diff string
+		if len(proposerSlashings) > 0 {
+			diff, _ = messagediff.PrettyDiff(proposerSlashings[0], tt.ps)
+		} else {
+			diff, _ = messagediff.PrettyDiff(nil, tt.ps)
+		}
+		t.Log(diff)
+
 		if proposerSlashings == nil || !reflect.DeepEqual(proposerSlashings[0], tt.ps) {
-			diff, _ := messagediff.PrettyDiff(proposerSlashings[0], tt.ps)
-			t.Log(diff)
 			t.Fatalf("Proposer slashing: %v should be part of proposer slashings response: %v", tt.ps, proposerSlashings)
 		}
 	}

tools/enr-calculator/BUILD.bazel

@@ -11,7 +11,6 @@ go_library(
     visibility = ["//visibility:private"],
     deps = [
         "//shared/maxprocs:go_default_library",
-        "@com_github_btcsuite_btcd//btcec:go_default_library",
         "@com_github_ethereum_go_ethereum//p2p/enode:go_default_library",
         "@com_github_ethereum_go_ethereum//p2p/enr:go_default_library",
         "@com_github_libp2p_go_libp2p_core//crypto:go_default_library",

tools/enr-calculator/main.go

@@ -9,7 +9,6 @@ import (
 	"io/ioutil"
 	"net"
 
-	"github.com/btcsuite/btcd/btcec"
 	"github.com/ethereum/go-ethereum/p2p/enode"
 	"github.com/ethereum/go-ethereum/p2p/enr"
 	"github.com/libp2p/go-libp2p-core/crypto"
@@ -28,7 +27,7 @@ var (
 func main() {
 	flag.Parse()
 
-	if len(*privateKey) == 0 {
+	if *privateKey == "" {
 		log.Fatal("No private key given")
 	}
 	dst, err := hex.DecodeString(*privateKey)
@@ -39,7 +38,7 @@ func main() {
 	if err != nil {
 		panic(err)
 	}
-	ecdsaPrivKey := (*ecdsa.PrivateKey)((*btcec.PrivateKey)(unmarshalledKey.(*crypto.Secp256k1PrivateKey)))
+	ecdsaPrivKey := (*ecdsa.PrivateKey)(unmarshalledKey.(*crypto.Secp256k1PrivateKey))
 
 	if net.ParseIP(*ipAddr).To4() == nil {
 		log.Fatalf("Invalid ipv4 address given: %v\n", err)

tools/eth1exporter/main.go

@@ -132,10 +132,12 @@ func MetricsHTTP(w http.ResponseWriter, r *http.Request) {
 		total.Add(total, bal)
 		allOut = append(allOut, fmt.Sprintf("%veth_balance{name=\"%v\",address=\"%v\"} %v", *prefix, v.Name, v.Address, v.Balance))
 	}
-	allOut = append(allOut, fmt.Sprintf("%veth_balance_total %0.18f", *prefix, total))
-	allOut = append(allOut, fmt.Sprintf("%veth_load_seconds %0.2f", *prefix, loadSeconds))
-	allOut = append(allOut, fmt.Sprintf("%veth_loaded_addresses %v", *prefix, totalLoaded))
-	allOut = append(allOut, fmt.Sprintf("%veth_total_addresses %v", *prefix, len(allWatching)))
+	allOut = append(allOut,
+		fmt.Sprintf("%veth_balance_total %0.18f", *prefix, total),
+		fmt.Sprintf("%veth_load_seconds %0.2f", *prefix, loadSeconds),
+		fmt.Sprintf("%veth_loaded_addresses %v", *prefix, totalLoaded),
+		fmt.Sprintf("%veth_total_addresses %v", *prefix, len(allWatching)))
+
 	if _, err := fmt.Fprintln(w, strings.Join(allOut, "\n")); err != nil {
 		logrus.WithError(err).Error("Failed to write metrics")
 	}

tools/interop/convert-keys/main.go

@@ -57,12 +57,15 @@ func main() {
 	if err != nil {
 		log.Fatalf("Failed to create file at %s: %v", os.Args[2], err)
 	}
-	defer func() {
+	cleanup := func() {
 		if err := outFile.Close(); err != nil {
 			panic(err)
 		}
-	}()
+	}
+	defer cleanup()
 	if err := keygen.SaveUnencryptedKeysToFile(outFile, out); err != nil {
+		// log.Fatalf will prevent defer from being called
+		cleanup()
 		log.Fatalf("Failed to save %v", err)
 	}
 	log.Printf("Wrote %s\n", os.Args[2])

tools/relaynode/relaynode.go

@@ -41,10 +41,13 @@ func main() {
 
 	ctx := context.Background()
 	log.Start(ctx, "main")
-	defer log.Finish(ctx)
+	cleanup := func() { log.Finish(ctx) }
+	defer cleanup()
 
 	srcMAddr, err := multiaddr.NewMultiaddr(fmt.Sprintf("/ip4/0.0.0.0/tcp/%d", *port))
 	if err != nil {
+		// log.Fatalf will prevent defer from being called
+		cleanup()
 		log.Fatalf("Unable to construct multiaddr %v", err)
 	}
 

tools/unencrypted-keys-gen/main.go

@@ -37,11 +37,12 @@ func main() {
 	if err != nil {
 		log.Fatal(err)
 	}
-	defer func() {
+	cleanup := func() {
 		if err := file.Close(); err != nil {
 			log.Fatal(err)
 		}
-	}()
+	}
+	defer cleanup()
 
 	var ctnr *keygen.UnencryptedKeysContainer
 	if *random {
@@ -50,6 +51,8 @@ func main() {
 		ctnr = generateUnencryptedKeys(*startIndex)
 	}
 	if err := keygen.SaveUnencryptedKeysToFile(file, ctnr); err != nil {
+		// log.Fatal will prevent defer from being called
+		cleanup()
 		log.Fatal(err)
 	}
 }

validator/client/runner.go

@@ -50,8 +50,11 @@ type Validator interface {
 // 5 - Determine role at current slot
 // 6 - Perform assigned role, if any
 func run(ctx context.Context, v Validator) {
-	defer v.Done()
+	cleanup := v.Done
+	defer cleanup()
 	if err := v.WaitForWalletInitialization(ctx); err != nil {
+		// log.Fatalf will prevent defer from being called
+		cleanup()
 		log.Fatalf("Wallet is not ready: %v", err)
 	}
 	if featureconfig.Get().SlasherProtection {

validator/client/service.go

@@ -217,12 +217,15 @@ func (v *ValidatorService) recheckKeys(ctx context.Context) {
 		if v.useWeb {
 			initializedChan := make(chan *wallet.Wallet)
 			sub := v.walletInitializedFeed.Subscribe(initializedChan)
-			defer sub.Unsubscribe()
+			cleanup := sub.Unsubscribe
+			defer cleanup()
 			w := <-initializedChan
 			keyManagerV2, err := w.InitializeKeymanager(
 				ctx, true, /* skipMnemonicConfirm */
 			)
 			if err != nil {
+				// log.Fatalf will prevent defer from being called
+				cleanup()
 				log.Fatalf("Could not read keymanager for wallet: %v", err)
 			}
 			v.keyManagerV2 = keyManagerV2

validator/client/service_test.go

@@ -57,16 +57,20 @@ func keySetup() {
 
 func TestMain(m *testing.M) {
 	dir := testutil.TempDir() + "/keystore1"
-	defer func() {
+	cleanup := func() {
 		if err := os.RemoveAll(dir); err != nil {
 			log.WithError(err).Debug("Cannot remove keystore folder")
 		}
-	}()
+	}
+	defer cleanup()
 	if err := v1.NewValidatorAccount(dir, "1234"); err != nil {
 		log.WithError(err).Debug("Cannot create validator account")
 	}
 	keySetup()
-	os.Exit(m.Run())
+	code := m.Run()
+	// os.Exit will prevent defer from being called
+	cleanup()
+	os.Exit(code)
 }
 
 func TestStop_CancelsContext(t *testing.T) {

validator/rpc/gateway/gateway.go

@@ -81,8 +81,6 @@ func (g *Gateway) Start() {
 			return
 		}
 	}()
-
-	return
 }
 
 // Status of grpc gateway. Returns an error if this service is unhealthy.