Add static analyzer to discourage use of panic()

BUILD.bazel

@@ -197,6 +197,7 @@ nogo(
         "//tools/analyzers/logruswitherror:go_default_library",
         "//tools/analyzers/maligned:go_default_library",
         "//tools/analyzers/nop:go_default_library",
+        "//tools/analyzers/nopanic:go_default_library",
         "//tools/analyzers/properpermissions:go_default_library",
         "//tools/analyzers/recursivelock:go_default_library",
         "//tools/analyzers/shadowpredecl:go_default_library",

api/client/beacon/BUILD.bazel

@@ -7,6 +7,7 @@ go_library(
         "doc.go",
         "health.go",
         "log.go",
+        "template.go",
     ],
     importpath = "github.com/prysmaticlabs/prysm/v5/api/client/beacon",
     visibility = ["//visibility:public"],

api/client/beacon/client.go

@@ -11,7 +11,6 @@ import (
 	"regexp"
 	"sort"
 	"strconv"
-	"text/template"
 
 	"github.com/ethereum/go-ethereum/common/hexutil"
 	"github.com/pkg/errors"
@@ -64,23 +63,6 @@ func IdFromSlot(s primitives.Slot) StateOrBlockId {
 	return StateOrBlockId(strconv.FormatUint(uint64(s), 10))
 }
 
-// idTemplate is used to create template functions that can interpolate StateOrBlockId values.
-func idTemplate(ts string) func(StateOrBlockId) string {
-	t := template.Must(template.New("").Parse(ts))
-	f := func(id StateOrBlockId) string {
-		b := bytes.NewBuffer(nil)
-		err := t.Execute(b, struct{ Id string }{Id: string(id)})
-		if err != nil {
-			panic(fmt.Sprintf("invalid idTemplate: %s", ts))
-		}
-		return b.String()
-	}
-	// run the template to ensure that it is valid
-	// this should happen load time (using package scoped vars) to ensure runtime errors aren't possible
-	_ = f(IdGenesis)
-	return f
-}
-
 // RenderGetBlockPath formats a block id into a path for the GetBlock API endpoint.
 func RenderGetBlockPath(id StateOrBlockId) string {
 	return path.Join(getSignedBlockPath, string(id))
@@ -114,8 +96,6 @@ func (c *Client) GetBlock(ctx context.Context, blockId StateOrBlockId) ([]byte,
 	return b, nil
 }
 
-var getBlockRootTpl = idTemplate(getBlockRootPath)
-
 // GetBlockRoot retrieves the hash_tree_root of the BeaconBlock for the given block id.
 // Block identifier can be one of: "head" (canonical head in node's view), "genesis", "finalized",
 // <slot>, <hex encoded blockRoot with 0x prefix>. Variables of type StateOrBlockId are exported by this package
@@ -138,8 +118,6 @@ func (c *Client) GetBlockRoot(ctx context.Context, blockId StateOrBlockId) ([32]
 	return bytesutil.ToBytes32(rs), nil
 }
 
-var getForkTpl = idTemplate(getForkForStatePath)
-
 // GetFork queries the Beacon Node API for the Fork from the state identified by stateId.
 // Block identifier can be one of: "head" (canonical head in node's view), "genesis", "finalized",
 // <slot>, <hex encoded blockRoot with 0x prefix>. Variables of type StateOrBlockId are exported by this package

api/client/beacon/template.go

@@ -0,0 +1,34 @@
+package beacon
+
+import (
+	"bytes"
+	"fmt"
+	"text/template"
+)
+
+type templateFn func(StateOrBlockId) string
+
+var getBlockRootTpl templateFn
+var getForkTpl templateFn
+
+func init() {
+	// idTemplate is used to create template functions that can interpolate StateOrBlockId values.
+	idTemplate := func(ts string) func(StateOrBlockId) string {
+		t := template.Must(template.New("").Parse(ts))
+		f := func(id StateOrBlockId) string {
+			b := bytes.NewBuffer(nil)
+			err := t.Execute(b, struct{ Id string }{Id: string(id)})
+			if err != nil {
+				panic(fmt.Sprintf("invalid idTemplate: %s", ts))
+			}
+			return b.String()
+		}
+		// run the template to ensure that it is valid
+		// this should happen load time (using package scoped vars) to ensure runtime errors aren't possible
+		_ = f(IdGenesis)
+		return f
+	}
+
+	getBlockRootTpl = idTemplate(getBlockRootPath)
+	getForkTpl = idTemplate(getForkForStatePath)
+}

async/event/subscription.go

@@ -154,7 +154,7 @@ retry:
 				continue retry
 			}
 			if sub == nil {
-				panic("event: ResubscribeFunc returned nil subscription and no error")
+				panic("event: ResubscribeFunc returned nil subscription and no error") // lint:nopanic -- This should never happen.
 			}
 			return sub
 		case <-s.unsub:

beacon-chain/db/kv/archived_point.go

@@ -35,7 +35,7 @@ func (s *Store) LastArchivedRoot(ctx context.Context) [32]byte {
 		_, blockRoot = bkt.Cursor().Last()
 		return nil
 	}); err != nil { // This view never returns an error, but we'll handle anyway for sanity.
-		panic(err)
+		panic(err) // lint:nopanic -- View never returns an error.
 	}
 
 	return bytesutil.ToBytes32(blockRoot)
@@ -53,7 +53,7 @@ func (s *Store) ArchivedPointRoot(ctx context.Context, slot primitives.Slot) [32
 		blockRoot = bucket.Get(bytesutil.SlotToBytesBigEndian(slot))
 		return nil
 	}); err != nil { // This view never returns an error, but we'll handle anyway for sanity.
-		panic(err)
+		panic(err) // lint:nopanic -- View never returns an error.
 	}
 
 	return bytesutil.ToBytes32(blockRoot)
@@ -69,7 +69,7 @@ func (s *Store) HasArchivedPoint(ctx context.Context, slot primitives.Slot) bool
 		exists = iBucket.Get(bytesutil.SlotToBytesBigEndian(slot)) != nil
 		return nil
 	}); err != nil { // This view never returns an error, but we'll handle anyway for sanity.
-		panic(err)
+		panic(err) // lint:nopanic -- View never returns an error.
 	}
 	return exists
 }

beacon-chain/db/kv/blocks.go

@@ -185,7 +185,7 @@ func (s *Store) HasBlock(ctx context.Context, blockRoot [32]byte) bool {
 		exists = bkt.Get(blockRoot[:]) != nil
 		return nil
 	}); err != nil { // This view never returns an error, but we'll handle anyway for sanity.
-		panic(err)
+		panic(err) // lint:nopanic -- View never returns an error.
 	}
 	return exists
 }

beacon-chain/db/kv/deposit_contract.go

@@ -20,7 +20,7 @@ func (s *Store) DepositContractAddress(ctx context.Context) ([]byte, error) {
 		addr = chainInfo.Get(depositContractAddressKey)
 		return nil
 	}); err != nil { // This view never returns an error, but we'll handle anyway for sanity.
-		panic(err)
+		panic(err) // lint:nopanic -- View never returns an error.
 	}
 	return addr, nil
 }

beacon-chain/db/kv/state.go

@@ -407,7 +407,7 @@ func (s *Store) HasState(ctx context.Context, blockRoot [32]byte) bool {
 		return nil
 	})
 	if err != nil {
-		panic(err)
+		panic(err) // lint:nopanic -- View never returns an error.
 	}
 	return hasState
 }

beacon-chain/execution/block_cache.go

@@ -158,7 +158,7 @@ func trim(queue *cache.FIFO, maxSize uint64) {
 	for s := uint64(len(queue.ListKeys())); s > maxSize; s-- {
 		// #nosec G104 popProcessNoopFunc never returns an error
 		if _, err := queue.Pop(popProcessNoopFunc); err != nil { // This never returns an error, but we'll handle anyway for sanity.
-			panic(err)
+			panic(err) // lint:nopanic -- popProcessNoopFunc never returns an error.
 		}
 	}
 }

beacon-chain/execution/testing/mock_faulty_powchain.go

@@ -57,7 +57,7 @@ func (*FaultyExecutionChain) ChainStartEth1Data() *ethpb.Eth1Data {
 func (*FaultyExecutionChain) PreGenesisState() state.BeaconState {
 	s, err := state_native.InitializeFromProtoUnsafePhase0(&ethpb.BeaconState{})
 	if err != nil {
-		panic("could not initialize state")
+		panic("could not initialize state") // lint:nopanic -- test code.
 	}
 	return s
 }

beacon-chain/node/node.go

@@ -440,7 +440,7 @@ func (b *BeaconNode) Start() {
 				log.WithField("times", i-1).Info("Already shutting down, interrupt more to panic")
 			}
 		}
-		panic("Panic closing the beacon node")
+		panic("Panic closing the beacon node") // lint:nopanic -- Panic is requested by user.
 	}()
 
 	// Wait for stop channel to be closed.
@@ -706,15 +706,15 @@ func (b *BeaconNode) registerP2P(cliCtx *cli.Context) error {
 func (b *BeaconNode) fetchP2P() p2p.P2P {
 	var p *p2p.Service
 	if err := b.services.FetchService(&p); err != nil {
-		panic(err)
+		panic(err) // lint:nopanic -- This could panic application start if the services are misconfigured.
 	}
 	return p
 }
 
 func (b *BeaconNode) fetchBuilderService() *builder.Service {
 	var s *builder.Service
 	if err := b.services.FetchService(&s); err != nil {
-		panic(err)
+		panic(err) // lint:nopanic -- This could panic application start if the services are misconfigured.
 	}
 	return s
 }
@@ -1018,13 +1018,13 @@ func (b *BeaconNode) registerPrometheusService(_ *cli.Context) error {
 	var additionalHandlers []prometheus.Handler
 	var p *p2p.Service
 	if err := b.services.FetchService(&p); err != nil {
-		panic(err)
+		panic(err) // lint:nopanic -- This could panic application start if the services are misconfigured.
 	}
 	additionalHandlers = append(additionalHandlers, prometheus.Handler{Path: "/p2p", Handler: p.InfoHandler})
 
 	var c *blockchain.Service
 	if err := b.services.FetchService(&c); err != nil {
-		panic(err)
+		panic(err) // lint:nopanic -- This could panic application start if the services are misconfigured.
 	}
 
 	service := prometheus.NewService(

beacon-chain/operations/attestations/mock/mock.go

@@ -1,3 +1,4 @@
+// lint:nopanic -- Mock / test code, panic is allowed.
 package mock
 
 import (

beacon-chain/operations/blstoexec/mock/mock.go

@@ -28,10 +28,10 @@ func (m *PoolMock) InsertBLSToExecChange(change *eth.SignedBLSToExecutionChange)
 
 // MarkIncluded --
 func (*PoolMock) MarkIncluded(_ *eth.SignedBLSToExecutionChange) {
-	panic("implement me")
+	panic("implement me") // lint:nopanic -- mock / test code.
 }
 
 // ValidatorExists --
 func (*PoolMock) ValidatorExists(_ primitives.ValidatorIndex) bool {
-	panic("implement me")
+	panic("implement me") // lint:nopanic -- mock / test code.
 }

beacon-chain/operations/slashings/mock/mock.go

@@ -40,10 +40,10 @@ func (*PoolMock) ConvertToElectra() {}
 
 // MarkIncludedAttesterSlashing --
 func (*PoolMock) MarkIncludedAttesterSlashing(_ ethpb.AttSlashing) {
-	panic("implement me")
+	panic("implement me") // lint:nopanic -- Test / mock code.
 }
 
 // MarkIncludedProposerSlashing --
 func (*PoolMock) MarkIncludedProposerSlashing(_ *ethpb.ProposerSlashing) {
-	panic("implement me")
+	panic("implement me") // lint:nopanic -- Test / mock code.
 }

beacon-chain/operations/voluntaryexits/mock/mock.go

@@ -28,5 +28,5 @@ func (m *PoolMock) InsertVoluntaryExit(exit *eth.SignedVoluntaryExit) {
 
 // MarkIncluded --
 func (*PoolMock) MarkIncluded(_ *eth.SignedVoluntaryExit) {
-	panic("implement me")
+	panic("implement me") // lint:nopanic -- Mock / test code.
 }

beacon-chain/rpc/testutil/mock_blocker.go

@@ -37,5 +37,5 @@ func (m *MockBlocker) Block(_ context.Context, b []byte) (interfaces.ReadOnlySig
 
 // Blobs --
 func (m *MockBlocker) Blobs(_ context.Context, _ string, _ []uint64) ([]*blocks.VerifiedROBlob, *core.RpcError) {
-	panic("implement me")
+	panic("implement me") // lint:nopanic -- Test code.
 }

beacon-chain/state/stategen/epoch_boundary_state_cache.go

@@ -178,7 +178,7 @@ func (e *epochBoundaryState) delete(blockRoot [32]byte) error {
 func trim(queue *cache.FIFO, maxSize uint64) {
 	for s := uint64(len(queue.ListKeys())); s > maxSize; s-- {
 		if _, err := queue.Pop(popProcessNoopFunc); err != nil { // This never returns an error, but we'll handle anyway for sanity.
-			panic(err)
+			panic(err) // lint:nopanic -- Never returns an error.
 		}
 	}
 }

beacon-chain/state/stategen/mock/mock.go

@@ -1,3 +1,4 @@
+// lint:nopanic -- Mock code, OK to panic.
 package mock
 
 import (

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

@@ -192,7 +192,7 @@ func (s *Service) Start() {
 		if errors.Is(s.ctx.Err(), context.Canceled) {
 			return
 		}
-		panic(err)
+		panic(err) // lint:nopanic -- Unexpected error. This should probably be surfaced with a returned error.
 	}
 	log.WithField("slot", s.cfg.Chain.HeadSlot()).Info("Synced up to")
 	s.markSynced()

beacon-chain/sync/subscriber.go

@@ -181,12 +181,12 @@ func (s *Service) subscribe(topic string, validator wrappedVal, handle subHandle
 	_, e, err := forks.RetrieveForkDataFromDigest(digest, genRoot[:])
 	if err != nil {
 		// Impossible condition as it would mean digest does not exist.
-		panic(err)
+		panic(err) // lint:nopanic -- Impossible condition.
 	}
 	base := p2p.GossipTopicMappings(topic, e)
 	if base == nil {
 		// Impossible condition as it would mean topic does not exist.
-		panic(fmt.Sprintf("%s is not mapped to any message in GossipTopicMappings", topic))
+		panic(fmt.Sprintf("%s is not mapped to any message in GossipTopicMappings", topic)) // lint:nopanic -- Impossible condition.
 	}
 	return s.subscribeWithBase(s.addDigestToTopic(topic, digest), validator, handle)
 }
@@ -497,13 +497,13 @@ func (s *Service) subscribeWithParameters(
 	// Retrieve the epoch of the fork corresponding to the digest.
 	_, epoch, err := forks.RetrieveForkDataFromDigest(digest, genesisValidatorsRoot[:])
 	if err != nil {
-		panic(err)
+		panic(err) // lint:nopanic -- Impossible condition.
 	}
 
 	// Retrieve the base protobuf message.
 	base := p2p.GossipTopicMappings(topicFormat, epoch)
 	if base == nil {
-		panic(fmt.Sprintf("%s is not mapped to any message in GossipTopicMappings", topicFormat))
+		panic(fmt.Sprintf("%s is not mapped to any message in GossipTopicMappings", topicFormat)) // lint:nopanic -- Impossible condition.
 	}
 
 	// Retrieve the genesis time.

build/bazel/bazel.go

@@ -60,6 +60,7 @@ func NewTmpDir(prefix string) (string, error) {
 // `@go_sdk//:files` in its `data` -- this will make sure the whole toolchain
 // gets pulled into the sandbox as well. Generally, this function should only
 // be called in init().
+// lint:nopanic -- This method is only used for testing.
 func SetGoEnv() {
 	gobin, err := Runfile("bin/go")
 	if err != nil {

cache/lru/lru_wrpr.go

@@ -10,7 +10,7 @@ import (
 func New(size int) *lru.Cache {
 	cache, err := lru.New(size)
 	if err != nil {
-		panic(fmt.Errorf("lru new failed: %w", err))
+		panic(fmt.Errorf("lru new failed: %w", err)) // lint:nopanic -- This should never panic.
 	}
 	return cache
 }
@@ -20,7 +20,7 @@ func New(size int) *lru.Cache {
 func NewWithEvict(size int, onEvicted func(key interface{}, value interface{})) *lru.Cache {
 	cache, err := lru.NewWithEvict(size, onEvicted)
 	if err != nil {
-		panic(fmt.Errorf("lru new with evict failed: %w", err))
+		panic(fmt.Errorf("lru new with evict failed: %w", err)) // lint:nopanic -- This should never panic.
 	}
 	return cache
 }

changelog/pvl_nopanic.md

@@ -0,0 +1,3 @@
+### Added
+
+- Added a static analyzer to discourage use of panic() in Prysm

cmd/beacon-chain/main.go

@@ -180,7 +180,7 @@ func before(ctx *cli.Context) error {
 		f := joonix.NewFormatter()
 
 		if err := joonix.DisableTimestampFormat(f); err != nil {
-			panic(err)
+			panic(err) // lint:nopanic -- This shouldn't happen, but crashing immediately at startup is OK.
 		}
 
 		logrus.SetFormatter(f)
@@ -250,7 +250,7 @@ func main() {
 	defer func() {
 		if x := recover(); x != nil {
 			log.Errorf("Runtime panic: %v\n%v", x, string(runtimeDebug.Stack()))
-			panic(x)
+			panic(x) // lint:nopanic -- This is just resurfacing the original panic.
 		}
 	}()
 

cmd/client-stats/main.go

@@ -69,7 +69,7 @@ func main() {
 		case "fluentd":
 			f := joonix.NewFormatter()
 			if err := joonix.DisableTimestampFormat(f); err != nil {
-				panic(err)
+				panic(err) // lint:nopanic -- This shouldn't happen, but crashing immediately at startup is OK.
 			}
 			logrus.SetFormatter(f)
 		case "json":
@@ -94,7 +94,7 @@ func main() {
 	defer func() {
 		if x := recover(); x != nil {
 			log.Errorf("Runtime panic: %v\n%v", x, string(runtimeDebug.Stack()))
-			panic(x)
+			panic(x) // lint:nopanic -- This is just resurfacing the original panic.
 		}
 	}()