From 2aa52fb56aa58f02ceb8137ada7d1ec13f4d08cc Mon Sep 17 00:00:00 2001 From: Preston Van Loon Date: Wed, 19 Mar 2025 13:04:15 -0500 Subject: [PATCH] Add static analyzer to discourage use of panic() (#15075) * Implement static analysis to prevent panics * Add nopanic to nogo * Fix violations and add exclusions Fix violations and add exclusions for all * Changelog fragment * Use pass.Report instead of pass.Reportf * Remove strings.ToLower for checking init method name * Add exclusion for herumi init * Move api/client/beacon template function to init and its own file * Fix nopanic testcase --- BUILD.bazel | 1 + api/client/beacon/BUILD.bazel | 1 + api/client/beacon/client.go | 22 ----- api/client/beacon/template.go | 34 ++++++++ async/event/subscription.go | 2 +- beacon-chain/db/kv/archived_point.go | 6 +- beacon-chain/db/kv/blocks.go | 2 +- beacon-chain/db/kv/deposit_contract.go | 2 +- beacon-chain/db/kv/state.go | 2 +- beacon-chain/execution/block_cache.go | 2 +- .../execution/testing/mock_faulty_powchain.go | 2 +- beacon-chain/node/node.go | 10 +-- .../operations/attestations/mock/mock.go | 1 + .../operations/blstoexec/mock/mock.go | 4 +- .../operations/slashings/mock/mock.go | 4 +- .../operations/voluntaryexits/mock/mock.go | 2 +- beacon-chain/rpc/testutil/mock_blocker.go | 2 +- .../stategen/epoch_boundary_state_cache.go | 2 +- beacon-chain/state/stategen/mock/mock.go | 1 + beacon-chain/sync/initial-sync/service.go | 2 +- beacon-chain/sync/subscriber.go | 8 +- build/bazel/bazel.go | 1 + cache/lru/lru_wrpr.go | 4 +- changelog/pvl_nopanic.md | 3 + cmd/beacon-chain/main.go | 4 +- cmd/client-stats/main.go | 4 +- cmd/prysmctl/p2p/client.go | 4 +- cmd/prysmctl/p2p/peers.go | 2 +- cmd/validator/main.go | 4 +- cmd/wrap_flags.go | 4 +- config/params/config_utils_develop.go | 2 +- config/params/config_utils_prod.go | 2 +- config/params/configset.go | 2 +- consensus-types/blocks/getters.go | 4 +- consensus-types/mock/block.go | 2 + consensus-types/primitives/epoch.go | 10 +-- consensus-types/primitives/slot.go | 10 +-- consensus-types/primitives/validator.go | 6 +- container/leaky-bucket/heap.go | 3 +- crypto/bls/herumi/init.go | 1 + crypto/hash/htr/hashtree.go | 6 +- crypto/keystore/keystore.go | 2 +- crypto/rand/rand.go | 2 +- encoding/ssz/detect/fieldspec.go | 2 +- encoding/ssz/merkleize.go | 3 +- math/math_helper.go | 3 +- network/external_ip.go | 2 +- nogo_config.json | 8 ++ runtime/interop/genesis.go | 2 +- testing/benchmark/pregen.go | 2 +- .../endtoend/components/eth1/transactions.go | 4 +- .../endtoend/components/web3remotesigner.go | 1 + testing/endtoend/types/fork.go | 2 +- testing/slasher/simulator/simulator.go | 1 + .../validator-mock/prysm_chain_client_mock.go | 3 +- time/slots/slotticker.go | 6 ++ time/slots/slottime.go | 2 +- tools/analyzers/nopanic/BUILD.bazel | 26 ++++++ tools/analyzers/nopanic/analyzer.go | 86 +++++++++++++++++++ tools/analyzers/nopanic/analyzer_test.go | 20 +++++ tools/analyzers/nopanic/testdata/code.go | 26 ++++++ tools/analyzers/nopanic/testdata/init.go | 7 ++ tools/analyzers/nopanic/testdata/pkg.go | 8 ++ tools/bootnode/bootnode.go | 1 + tools/enr-calculator/main.go | 2 + tools/eth1voting/votes.go | 2 +- tools/gocovmerge/main.go | 1 + validator/accounts/testing/mock.go | 3 + .../beacon_api_beacon_chain_client.go | 2 +- .../beacon-api/beacon_api_node_client.go | 2 +- validator/client/service.go | 2 +- .../db/filesystem/attester_protection.go | 2 +- .../db/filesystem/proposer_protection.go | 6 +- validator/node/node.go | 2 +- 74 files changed, 329 insertions(+), 104 deletions(-) create mode 100644 api/client/beacon/template.go create mode 100644 changelog/pvl_nopanic.md create mode 100644 tools/analyzers/nopanic/BUILD.bazel create mode 100644 tools/analyzers/nopanic/analyzer.go create mode 100644 tools/analyzers/nopanic/analyzer_test.go create mode 100644 tools/analyzers/nopanic/testdata/code.go create mode 100644 tools/analyzers/nopanic/testdata/init.go create mode 100644 tools/analyzers/nopanic/testdata/pkg.go diff --git a/BUILD.bazel b/BUILD.bazel index 7c5ecdcd6c..a1eb46badb 100644 --- a/BUILD.bazel +++ b/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", diff --git a/api/client/beacon/BUILD.bazel b/api/client/beacon/BUILD.bazel index 5990c8be33..35c52211d8 100644 --- a/api/client/beacon/BUILD.bazel +++ b/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"], diff --git a/api/client/beacon/client.go b/api/client/beacon/client.go index 8af0b38bf4..8f05cc3ede 100644 --- a/api/client/beacon/client.go +++ b/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", // , . 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", // , . Variables of type StateOrBlockId are exported by this package diff --git a/api/client/beacon/template.go b/api/client/beacon/template.go new file mode 100644 index 0000000000..815b77a43f --- /dev/null +++ b/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) +} diff --git a/async/event/subscription.go b/async/event/subscription.go index 9ed0dfe1e8..10a4061e8d 100644 --- a/async/event/subscription.go +++ b/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: diff --git a/beacon-chain/db/kv/archived_point.go b/beacon-chain/db/kv/archived_point.go index 64a0aaa351..98b0bbb750 100644 --- a/beacon-chain/db/kv/archived_point.go +++ b/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 } diff --git a/beacon-chain/db/kv/blocks.go b/beacon-chain/db/kv/blocks.go index 2024cd4d4c..cd81e1ea7e 100644 --- a/beacon-chain/db/kv/blocks.go +++ b/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 } diff --git a/beacon-chain/db/kv/deposit_contract.go b/beacon-chain/db/kv/deposit_contract.go index e747b149c5..227449f0be 100644 --- a/beacon-chain/db/kv/deposit_contract.go +++ b/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 } diff --git a/beacon-chain/db/kv/state.go b/beacon-chain/db/kv/state.go index eee40b66a4..178196cb75 100644 --- a/beacon-chain/db/kv/state.go +++ b/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 } diff --git a/beacon-chain/execution/block_cache.go b/beacon-chain/execution/block_cache.go index 4866ee75d1..a1a5de364a 100644 --- a/beacon-chain/execution/block_cache.go +++ b/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. } } } diff --git a/beacon-chain/execution/testing/mock_faulty_powchain.go b/beacon-chain/execution/testing/mock_faulty_powchain.go index a6997b8ce7..f7ba772f00 100644 --- a/beacon-chain/execution/testing/mock_faulty_powchain.go +++ b/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(ðpb.BeaconState{}) if err != nil { - panic("could not initialize state") + panic("could not initialize state") // lint:nopanic -- test code. } return s } diff --git a/beacon-chain/node/node.go b/beacon-chain/node/node.go index 9e3908253d..a834c5af13 100644 --- a/beacon-chain/node/node.go +++ b/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,7 +706,7 @@ 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 } @@ -714,7 +714,7 @@ func (b *BeaconNode) fetchP2P() p2p.P2P { 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( diff --git a/beacon-chain/operations/attestations/mock/mock.go b/beacon-chain/operations/attestations/mock/mock.go index 376ee5681b..9cef512498 100644 --- a/beacon-chain/operations/attestations/mock/mock.go +++ b/beacon-chain/operations/attestations/mock/mock.go @@ -1,3 +1,4 @@ +// lint:nopanic -- Mock / test code, panic is allowed. package mock import ( diff --git a/beacon-chain/operations/blstoexec/mock/mock.go b/beacon-chain/operations/blstoexec/mock/mock.go index 5bef0b1337..68b1e48357 100644 --- a/beacon-chain/operations/blstoexec/mock/mock.go +++ b/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. } diff --git a/beacon-chain/operations/slashings/mock/mock.go b/beacon-chain/operations/slashings/mock/mock.go index 85e4a910cd..847dc5c7f5 100644 --- a/beacon-chain/operations/slashings/mock/mock.go +++ b/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. } diff --git a/beacon-chain/operations/voluntaryexits/mock/mock.go b/beacon-chain/operations/voluntaryexits/mock/mock.go index 732817f022..32c5ea2a04 100644 --- a/beacon-chain/operations/voluntaryexits/mock/mock.go +++ b/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. } diff --git a/beacon-chain/rpc/testutil/mock_blocker.go b/beacon-chain/rpc/testutil/mock_blocker.go index 420d7d83fa..66c9b57e4a 100644 --- a/beacon-chain/rpc/testutil/mock_blocker.go +++ b/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. } diff --git a/beacon-chain/state/stategen/epoch_boundary_state_cache.go b/beacon-chain/state/stategen/epoch_boundary_state_cache.go index 3ed39791d2..0604f048e6 100644 --- a/beacon-chain/state/stategen/epoch_boundary_state_cache.go +++ b/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. } } } diff --git a/beacon-chain/state/stategen/mock/mock.go b/beacon-chain/state/stategen/mock/mock.go index 6be0fe0033..c7103e1012 100644 --- a/beacon-chain/state/stategen/mock/mock.go +++ b/beacon-chain/state/stategen/mock/mock.go @@ -1,3 +1,4 @@ +// lint:nopanic -- Mock code, OK to panic. package mock import ( diff --git a/beacon-chain/sync/initial-sync/service.go b/beacon-chain/sync/initial-sync/service.go index 0fa2bf6f33..f13f1f071f 100644 --- a/beacon-chain/sync/initial-sync/service.go +++ b/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() diff --git a/beacon-chain/sync/subscriber.go b/beacon-chain/sync/subscriber.go index 69afe97eb0..307ffe0812 100644 --- a/beacon-chain/sync/subscriber.go +++ b/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. diff --git a/build/bazel/bazel.go b/build/bazel/bazel.go index 50b957dd35..c40a7a654d 100644 --- a/build/bazel/bazel.go +++ b/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 { diff --git a/cache/lru/lru_wrpr.go b/cache/lru/lru_wrpr.go index eff4d13293..5f261a2384 100644 --- a/cache/lru/lru_wrpr.go +++ b/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 } diff --git a/changelog/pvl_nopanic.md b/changelog/pvl_nopanic.md new file mode 100644 index 0000000000..f301ecdf26 --- /dev/null +++ b/changelog/pvl_nopanic.md @@ -0,0 +1,3 @@ +### Added + +- Added a static analyzer to discourage use of panic() in Prysm diff --git a/cmd/beacon-chain/main.go b/cmd/beacon-chain/main.go index 618b717c3e..f85c7de29d 100644 --- a/cmd/beacon-chain/main.go +++ b/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. } }() diff --git a/cmd/client-stats/main.go b/cmd/client-stats/main.go index fd56b7e5f2..2741ab59a9 100644 --- a/cmd/client-stats/main.go +++ b/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. } }() diff --git a/cmd/prysmctl/p2p/client.go b/cmd/prysmctl/p2p/client.go index c6fe6e39b8..3585355607 100644 --- a/cmd/prysmctl/p2p/client.go +++ b/cmd/prysmctl/p2p/client.go @@ -90,7 +90,7 @@ func newClient(beaconEndpoints []string, tcpPort, quicPort uint) (*client, error func (c *client) Close() { if err := c.host.Close(); err != nil { - panic(err) + panic(err) // lint:nopanic -- The client is closing anyway... } } @@ -193,7 +193,7 @@ func (c *client) initializeMockChainService(ctx context.Context) (*mockChain, er func ipAddr() net.IP { ip, err := network.ExternalIP() if err != nil { - panic(err) + panic(err) // lint:nopanic -- Only returns an error when network interfaces are not available. This is a requirement for the application anyway. } return net.ParseIP(ip) } diff --git a/cmd/prysmctl/p2p/peers.go b/cmd/prysmctl/p2p/peers.go index 687e2fbf31..4a13a9c24f 100644 --- a/cmd/prysmctl/p2p/peers.go +++ b/cmd/prysmctl/p2p/peers.go @@ -14,7 +14,7 @@ func (c *client) connectToPeers(ctx context.Context, peerMultiaddrs ...string) e } addrInfos, err := peer.AddrInfosFromP2pAddrs(peers...) if err != nil { - panic(err) + return err } for _, info := range addrInfos { if info.ID == c.host.ID() { diff --git a/cmd/validator/main.go b/cmd/validator/main.go index a8c4697514..05f7b913e6 100644 --- a/cmd/validator/main.go +++ b/cmd/validator/main.go @@ -163,7 +163,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": @@ -208,7 +208,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. } }() diff --git a/cmd/wrap_flags.go b/cmd/wrap_flags.go index cbddd29250..985f76f3b5 100644 --- a/cmd/wrap_flags.go +++ b/cmd/wrap_flags.go @@ -34,11 +34,11 @@ func WrapFlags(flags []cli.Flag) []cli.Flag { f = altsrc.NewPathFlag(t) case *cli.Int64Flag: // Int64Flag does not work. See https://github.com/prysmaticlabs/prysm/issues/6478 - panic(fmt.Sprintf("unsupported flag type %T", f)) + panic(fmt.Sprintf("unsupported flag type %T", f)) // lint:nopanic -- This is evaluated at application start. case *cli.IntSliceFlag: f = altsrc.NewIntSliceFlag(t) default: - panic(fmt.Sprintf("cannot convert type %T", f)) + panic(fmt.Sprintf("cannot convert type %T", f)) // lint:nopanic -- This is evaluated at application start. } wrapped = append(wrapped, f) } diff --git a/config/params/config_utils_develop.go b/config/params/config_utils_develop.go index ffb3ee0d3d..ca3c1bddd6 100644 --- a/config/params/config_utils_develop.go +++ b/config/params/config_utils_develop.go @@ -33,7 +33,7 @@ func (b *BeaconChainConfig) Copy() *BeaconChainConfig { defer cfgrw.RUnlock() config, ok := deepcopy.Copy(*b).(BeaconChainConfig) if !ok { - panic("somehow deepcopy produced a BeaconChainConfig that is not of the same type as the original") + panic("somehow deepcopy produced a BeaconChainConfig that is not of the same type as the original") // lint:nopanic -- Impossible scenario. } return &config } diff --git a/config/params/config_utils_prod.go b/config/params/config_utils_prod.go index e5e8ce7c70..59ab06bff3 100644 --- a/config/params/config_utils_prod.go +++ b/config/params/config_utils_prod.go @@ -23,7 +23,7 @@ func OverrideBeaconConfig(c *BeaconChainConfig) { func (b *BeaconChainConfig) Copy() *BeaconChainConfig { config, ok := deepcopy.Copy(*b).(BeaconChainConfig) if !ok { - panic("somehow deepcopy produced a BeaconChainConfig that is not of the same type as the original") + panic("somehow deepcopy produced a BeaconChainConfig that is not of the same type as the original") // lint:nopanic -- This would only panic with an application misconfiguration and it should fail right away. } return &config } diff --git a/config/params/configset.go b/config/params/configset.go index a259e22b74..b4701bbc92 100644 --- a/config/params/configset.go +++ b/config/params/configset.go @@ -52,7 +52,7 @@ func newConfigset(configs ...*BeaconChainConfig) *configset { } for _, c := range configs { if err := r.add(c); err != nil { - panic(err) + panic(err) // lint:nopanic -- This would only panic with an application misconfiguration and it should fail right away. } } return r diff --git a/consensus-types/blocks/getters.go b/consensus-types/blocks/getters.go index 6d4a728c09..281737b2ef 100644 --- a/consensus-types/blocks/getters.go +++ b/consensus-types/blocks/getters.go @@ -490,6 +490,7 @@ func (b *SignedBeaconBlock) MarshalSSZTo(dst []byte) ([]byte, error) { // of fastssz's SizeSSZ() interface function to avoid panicking. // Changing the signature causes very problematic issues with wealdtech deps. // For the time being panicking is preferable. +// lint:nopanic -- Panic warning is communicated in godoc commentary. func (b *SignedBeaconBlock) SizeSSZ() int { pb, err := b.Proto() if err != nil { @@ -530,7 +531,7 @@ func (b *SignedBeaconBlock) SizeSSZ() int { } } -// UnmarshalSSZ unmarshals the signed beacon block from its relevant ssz form. +// UnmarshalSSZ unmarshals the sitime/slots/slottime.gogned beacon block from its relevant ssz form. // nolint:gocognit func (b *SignedBeaconBlock) UnmarshalSSZ(buf []byte) error { var newBlock *SignedBeaconBlock @@ -884,6 +885,7 @@ func (b *BeaconBlock) MarshalSSZTo(dst []byte) ([]byte, error) { // of fastssz's SizeSSZ() interface function to avoid panicking. // Changing the signature causes very problematic issues with wealdtech deps. // For the time being panicking is preferable. +// lint:nopanic -- Panic is communicated in godoc. func (b *BeaconBlock) SizeSSZ() int { pb, err := b.Proto() if err != nil { diff --git a/consensus-types/mock/block.go b/consensus-types/mock/block.go index a0ad5fb8dc..3ebe7e1591 100644 --- a/consensus-types/mock/block.go +++ b/consensus-types/mock/block.go @@ -1,3 +1,5 @@ +// package mock +// lint:nopanic -- This is test / mock code, allowed to panic. package mock import ( diff --git a/consensus-types/primitives/epoch.go b/consensus-types/primitives/epoch.go index 1690e27de3..b2ca07d061 100644 --- a/consensus-types/primitives/epoch.go +++ b/consensus-types/primitives/epoch.go @@ -27,7 +27,7 @@ func MaxEpoch(a, b Epoch) Epoch { func (e Epoch) Mul(x uint64) Epoch { res, err := e.SafeMul(x) if err != nil { - panic(err.Error()) + panic(err.Error()) // lint:nopanic -- Panic is communicated in the godoc commentary. } return res } @@ -44,7 +44,7 @@ func (e Epoch) SafeMul(x uint64) (Epoch, error) { func (e Epoch) Div(x uint64) Epoch { res, err := e.SafeDiv(x) if err != nil { - panic(err.Error()) + panic(err.Error()) // lint:nopanic -- Panic is communicated in the godoc commentary. } return res } @@ -61,7 +61,7 @@ func (e Epoch) SafeDiv(x uint64) (Epoch, error) { func (e Epoch) Add(x uint64) Epoch { res, err := e.SafeAdd(x) if err != nil { - panic(err.Error()) + panic(err.Error()) // lint:nopanic -- Panic is communicated in the godoc commentary. } return res } @@ -90,7 +90,7 @@ func (e Epoch) SafeAddEpoch(x Epoch) (Epoch, error) { func (e Epoch) Sub(x uint64) Epoch { res, err := e.SafeSub(x) if err != nil { - panic(err.Error()) + panic(err.Error()) // lint:nopanic -- Panic is communicated in the godoc commentary. } return res } @@ -107,7 +107,7 @@ func (e Epoch) SafeSub(x uint64) (Epoch, error) { func (e Epoch) Mod(x uint64) Epoch { res, err := e.SafeMod(x) if err != nil { - panic(err.Error()) + panic(err.Error()) // lint:nopanic -- Panic is communicated in the godoc commentary. } return res } diff --git a/consensus-types/primitives/slot.go b/consensus-types/primitives/slot.go index 1f9490f883..7637cfa4cf 100644 --- a/consensus-types/primitives/slot.go +++ b/consensus-types/primitives/slot.go @@ -19,7 +19,7 @@ type Slot uint64 func (s Slot) Mul(x uint64) Slot { res, err := s.SafeMul(x) if err != nil { - panic(err.Error()) + panic(err.Error()) // lint:nopanic -- Panic is communicated in the godoc commentary. } return res } @@ -48,7 +48,7 @@ func (s Slot) SafeMulSlot(x Slot) (Slot, error) { func (s Slot) Div(x uint64) Slot { res, err := s.SafeDiv(x) if err != nil { - panic(err.Error()) + panic(err.Error()) // lint:nopanic -- Panic is communicated in the godoc commentary. } return res } @@ -77,7 +77,7 @@ func (s Slot) SafeDivSlot(x Slot) (Slot, error) { func (s Slot) Add(x uint64) Slot { res, err := s.SafeAdd(x) if err != nil { - panic(err.Error()) + panic(err.Error()) // lint:nopanic -- Panic is communicated in the godoc commentary. } return res } @@ -106,7 +106,7 @@ func (s Slot) SafeAddSlot(x Slot) (Slot, error) { func (s Slot) Sub(x uint64) Slot { res, err := s.SafeSub(x) if err != nil { - panic(err.Error()) + panic(err.Error()) // lint:nopanic -- Panic is communicated in the godoc commentary. } return res } @@ -143,7 +143,7 @@ func (s Slot) SafeSubSlot(x Slot) (Slot, error) { func (s Slot) Mod(x uint64) Slot { res, err := s.SafeMod(x) if err != nil { - panic(err.Error()) + panic(err.Error()) // lint:nopanic -- Panic is communicated in the godoc commentary. } return res } diff --git a/consensus-types/primitives/validator.go b/consensus-types/primitives/validator.go index 73fc6e68ea..5e0cb726c5 100644 --- a/consensus-types/primitives/validator.go +++ b/consensus-types/primitives/validator.go @@ -14,9 +14,10 @@ var _ fssz.Unmarshaler = (*ValidatorIndex)(nil) type ValidatorIndex uint64 // Div divides validator index by x. +// This method panics if dividing by zero! func (v ValidatorIndex) Div(x uint64) ValidatorIndex { if x == 0 { - panic("divbyzero") + panic("divbyzero") // lint:nopanic -- Panic is communicated in the godoc commentary. } return ValidatorIndex(uint64(v) / x) } @@ -27,9 +28,10 @@ func (v ValidatorIndex) Add(x uint64) ValidatorIndex { } // Sub subtracts x from the validator index. +// This method panics if causing an underflow! func (v ValidatorIndex) Sub(x uint64) ValidatorIndex { if uint64(v) < x { - panic("underflow") + panic("underflow") // lint:nopanic -- Panic is communicated in the godoc commentary. } return ValidatorIndex(uint64(v) - x) } diff --git a/container/leaky-bucket/heap.go b/container/leaky-bucket/heap.go index cd3a64485e..7f36db23f4 100644 --- a/container/leaky-bucket/heap.go +++ b/container/leaky-bucket/heap.go @@ -27,11 +27,12 @@ func (pq priorityQueue) Swap(i, j int) { pq[j].index = j } +// Push a LeakyBucket to priorityQueue func (pq *priorityQueue) Push(x interface{}) { n := len(*pq) b, ok := x.(*LeakyBucket) if !ok { - panic(fmt.Sprintf("%T", x)) + panic(fmt.Sprintf("%T", x)) // lint:nopanic -- This method should be improved. High risk for misuse! } b.index = n *pq = append(*pq, b) diff --git a/crypto/bls/herumi/init.go b/crypto/bls/herumi/init.go index f236d45973..8bbe3d30f9 100644 --- a/crypto/bls/herumi/init.go +++ b/crypto/bls/herumi/init.go @@ -3,6 +3,7 @@ package herumi import "github.com/herumi/bls-eth-go-binary/bls" // Init allows the required curve orders and appropriate sub-groups to be initialized. +// lint:nopanic -- This method is called at init time only. func Init() { if err := bls.Init(bls.BLS12_381); err != nil { panic(err) diff --git a/crypto/hash/htr/hashtree.go b/crypto/hash/htr/hashtree.go index 1ffb68b038..6a052226e1 100644 --- a/crypto/hash/htr/hashtree.go +++ b/crypto/hash/htr/hashtree.go @@ -13,7 +13,7 @@ func hashParallel(inputList [][32]byte, outputList [][32]byte, wg *sync.WaitGrou defer wg.Done() err := gohashtree.Hash(outputList, inputList) if err != nil { - panic(err) + panic(err) // lint:nopanic -- This should never panic. } } @@ -27,7 +27,7 @@ func VectorizedSha256(inputList [][32]byte) [][32]byte { if len(inputList) < minSliceSizeToParallelize { err := gohashtree.Hash(outputList, inputList) if err != nil { - panic(err) + panic(err) // lint:nopanic -- This should never panic. } return outputList } @@ -40,7 +40,7 @@ func VectorizedSha256(inputList [][32]byte) [][32]byte { } err := gohashtree.Hash(outputList[n*groupSize:], inputList[n*2*groupSize:]) if err != nil { - panic(err) + panic(err) // lint:nopanic -- This should never panic. } wg.Wait() return outputList diff --git a/crypto/keystore/keystore.go b/crypto/keystore/keystore.go index 95477b3e99..4289ae5f08 100644 --- a/crypto/keystore/keystore.go +++ b/crypto/keystore/keystore.go @@ -130,7 +130,7 @@ func EncryptKey(key *Key, password string, scryptN, scryptP int) ([]byte, error) authArray := []byte(password) salt := make([]byte, 32) if _, err := io.ReadFull(rand.Reader, salt); err != nil { - panic("reading from crypto/rand failed: " + err.Error()) + panic("reading from crypto/rand failed: " + err.Error()) // lint:nopanic -- This should never happen. } derivedKey, err := scrypt.Key(authArray, salt, scryptN, scryptR, scryptP, scryptDKLen) diff --git a/crypto/rand/rand.go b/crypto/rand/rand.go index 31cb4556a8..0da38130b4 100644 --- a/crypto/rand/rand.go +++ b/crypto/rand/rand.go @@ -57,7 +57,7 @@ func (_ *source) Uint64() (val uint64) { lock.RLock() defer lock.RUnlock() if err := binary.Read(rand.Reader, binary.BigEndian, &val); err != nil { - panic(err) + panic(err) // lint:nopanic -- Panic risk is communicated in the godoc commentary. } return } diff --git a/encoding/ssz/detect/fieldspec.go b/encoding/ssz/detect/fieldspec.go index 2cdfce6557..2d3569f57a 100644 --- a/encoding/ssz/detect/fieldspec.go +++ b/encoding/ssz/detect/fieldspec.go @@ -35,7 +35,7 @@ func (f fieldType) Size() int { case typeBytes4: return 4 default: - panic("can't determine size for unrecognizedtype ") + panic("can't determine size for unrecognizedtype ") // lint:nopanic -- Impossible field type. } } diff --git a/encoding/ssz/merkleize.go b/encoding/ssz/merkleize.go index 704298b0ee..f55be66bac 100644 --- a/encoding/ssz/merkleize.go +++ b/encoding/ssz/merkleize.go @@ -68,9 +68,10 @@ func Depth(v uint64) (out uint8) { } // Merkleize with log(N) space allocation +// This method will panic when count > limit. func Merkleize(hasher Hasher, count, limit uint64, leaf func(i uint64) []byte) (out [32]byte) { if count > limit { - panic("merkleizing list that is too large, over limit") + panic("merkleizing list that is too large, over limit") // lint:nopanic -- Panic is communicated in godoc commentary. } if limit == 0 { return diff --git a/math/math_helper.go b/math/math_helper.go index e4a59e21d6..aa55e8ca77 100644 --- a/math/math_helper.go +++ b/math/math_helper.go @@ -109,9 +109,10 @@ func IsPowerOf2(n uint64) bool { // PowerOf2 returns an integer that is the provided // exponent of 2. Can only return powers of 2 till 63, // after that it overflows +// This method will panic if `n` is greater than 63. func PowerOf2(n uint64) uint64 { if n >= 64 { - panic("integer overflow") + panic("integer overflow") // lint:nopanic -- Panic is communicated in the godoc commentary. } return 1 << n } diff --git a/network/external_ip.go b/network/external_ip.go index dfc52c3c9a..c1b42b3d4f 100644 --- a/network/external_ip.go +++ b/network/external_ip.go @@ -10,7 +10,7 @@ import ( func IPAddr() net.IP { ip, err := ExternalIP() if err != nil { - panic(err) + panic(err) // lint:nopanic -- Only panics if a network interface is not available. This is a requirement to run the application anyway. } return net.ParseIP(ip) } diff --git a/nogo_config.json b/nogo_config.json index c2f3034b2a..daa989b606 100644 --- a/nogo_config.json +++ b/nogo_config.json @@ -214,5 +214,13 @@ "external/com_github_ethereum_go_ethereum/.*": "Unsafe third party code", "rules_go_work-.*": "Third party code" } + }, + "nopanic": { + "exclude_files": { + "validator/web/site_data.go": "generated code", + ".*/.*_test\\.go": "Tests are OK", + ".*/main\\.go": "main methods are OK", + "external/.*": "Third party code" + } } } diff --git a/runtime/interop/genesis.go b/runtime/interop/genesis.go index 61c2531fd4..71cbd0084b 100644 --- a/runtime/interop/genesis.go +++ b/runtime/interop/genesis.go @@ -212,7 +212,7 @@ func defaultDepositContractAllocation(contractAddress string) depositAllocation } codeBytes, err := hexutil.Decode(DepositContractCode) if err != nil { - panic(err) + panic(err) // lint:nopanic -- The deposit contract code is hardcoded and checked in tests. } return depositAllocation{ Address: common.HexToAddress(contractAddress), diff --git a/testing/benchmark/pregen.go b/testing/benchmark/pregen.go index 2e52f7e85f..0947e4f4cc 100644 --- a/testing/benchmark/pregen.go +++ b/testing/benchmark/pregen.go @@ -104,7 +104,7 @@ func SetBenchmarkConfig() (func(), error) { undo, err := params.SetActiveWithUndo(c) return func() { if err := undo(); err != nil { - panic(err) + panic(err) // lint:nopanic -- Test code / impossible scenario. } }, err } diff --git a/testing/endtoend/components/eth1/transactions.go b/testing/endtoend/components/eth1/transactions.go index 18963147cc..eb461d7f62 100644 --- a/testing/endtoend/components/eth1/transactions.go +++ b/testing/endtoend/components/eth1/transactions.go @@ -321,7 +321,7 @@ func RandomBlobTx(rpc *rpc.Client, f *filler.Filler, sender common.Address, nonc func New4844Tx(nonce uint64, to *common.Address, gasLimit uint64, chainID, tip, feeCap, value *big.Int, code []byte, blobFeeCap *big.Int, blobData []byte, al types.AccessList) *types.Transaction { blobs, comms, proofs, versionedHashes, err := EncodeBlobs(blobData) if err != nil { - panic(err) + panic(err) // lint:nopanic -- Test code. } tx := types.NewTx(&types.BlobTx{ ChainID: uint256.MustFromBig(chainID), @@ -427,7 +427,7 @@ func randomAddress() common.Address { b := make([]byte, 20) _, err := mathRand.Read(b) // #nosec G404 if err != nil { - panic(err) + panic(err) // lint:nopanic -- Test code. } return common.BytesToAddress(b) case 3: diff --git a/testing/endtoend/components/web3remotesigner.go b/testing/endtoend/components/web3remotesigner.go index 6484ec6c20..c3aa34c4a4 100644 --- a/testing/endtoend/components/web3remotesigner.go +++ b/testing/endtoend/components/web3remotesigner.go @@ -1,3 +1,4 @@ +// lint:nopanic -- Test tooling / code. package components import ( diff --git a/testing/endtoend/types/fork.go b/testing/endtoend/types/fork.go index 775194f8e1..0005af6c4f 100644 --- a/testing/endtoend/types/fork.go +++ b/testing/endtoend/types/fork.go @@ -11,7 +11,7 @@ import ( func InitForkCfg(start, end int, c *params.BeaconChainConfig) *params.BeaconChainConfig { c = c.Copy() if end < start { - panic("end fork is less than the start fork") + panic("end fork is less than the start fork") // lint:nopanic -- test code. } if start >= version.Altair { c.AltairForkEpoch = 0 diff --git a/testing/slasher/simulator/simulator.go b/testing/slasher/simulator/simulator.go index f53b039567..3f2fe128ee 100644 --- a/testing/slasher/simulator/simulator.go +++ b/testing/slasher/simulator/simulator.go @@ -1,3 +1,4 @@ +// lint:nopanic -- Test tooling / code. package simulator import ( diff --git a/testing/validator-mock/prysm_chain_client_mock.go b/testing/validator-mock/prysm_chain_client_mock.go index 9a4d07af7c..640b5abcd5 100644 --- a/testing/validator-mock/prysm_chain_client_mock.go +++ b/testing/validator-mock/prysm_chain_client_mock.go @@ -56,6 +56,7 @@ func (mr *MockPrysmChainClientMockRecorder) ValidatorCount(arg0, arg1, arg2 any) mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ValidatorCount", reflect.TypeOf((*MockPrysmChainClient)(nil).ValidatorCount), arg0, arg1, arg2) } + // ValidatorPerformance mocks base method. func (m *MockPrysmChainClient) ValidatorPerformance(arg0 context.Context, arg1 *ethpb.ValidatorPerformanceRequest) (*ethpb.ValidatorPerformanceResponse, error) { m.ctrl.T.Helper() @@ -70,5 +71,3 @@ func (mr *MockPrysmChainClientMockRecorder) ValidatorPerformance(arg0, arg1 any) mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ValidatorCount", reflect.TypeOf((*MockPrysmChainClient)(nil).ValidatorPerformance), arg0, arg1) } - - diff --git a/time/slots/slotticker.go b/time/slots/slotticker.go index 2655657f32..4678efde46 100644 --- a/time/slots/slotticker.go +++ b/time/slots/slotticker.go @@ -75,6 +75,8 @@ func (s *SlotIntervalTicker) Done() { } // NewSlotTicker starts and returns a new SlotTicker instance. +// This method panics if genesis time is zero. +// lint:nopanic -- Communicated panic in godoc commentary. func NewSlotTicker(genesisTime time.Time, secondsPerSlot uint64) *SlotTicker { if genesisTime.IsZero() { panic("zero genesis time") @@ -89,6 +91,8 @@ func NewSlotTicker(genesisTime time.Time, secondsPerSlot uint64) *SlotTicker { // NewSlotTickerWithOffset starts and returns a SlotTicker instance that allows a offset of time from genesis, // entering a offset greater than secondsPerSlot is not allowed. +// This method will panic if genesis time is zero or the offset is less than seconds per slot. +// lint:nopanic -- Communicated panic in godoc commentary. func NewSlotTickerWithOffset(genesisTime time.Time, offset time.Duration, secondsPerSlot uint64) *SlotTicker { if genesisTime.Unix() == 0 { panic("zero genesis time") @@ -176,6 +180,8 @@ func (s *SlotIntervalTicker) startWithIntervals( // several offsets of time from genesis, // Caller is responsible to input the intervals in increasing order and none bigger or equal than // SecondsPerSlot +// This method will panic if genesis time is zero, intervals is 0 length, or offsets are invalid. +// lint:nopanic -- Communicated panic in godoc commentary. func NewSlotTickerWithIntervals(genesisTime time.Time, intervals []time.Duration) *SlotIntervalTicker { if genesisTime.Unix() == 0 { panic("zero genesis time") diff --git a/time/slots/slottime.go b/time/slots/slottime.go index 412ad33d36..4923f01efa 100644 --- a/time/slots/slottime.go +++ b/time/slots/slottime.go @@ -126,7 +126,7 @@ func EpochStart(epoch primitives.Epoch) (primitives.Slot, error) { func UnsafeEpochStart(epoch primitives.Epoch) primitives.Slot { es, err := EpochStart(epoch) if err != nil { - panic(err) + panic(err) // lint:nopanic -- Unsafe is implied and communicated in the godoc commentary. } return es } diff --git a/tools/analyzers/nopanic/BUILD.bazel b/tools/analyzers/nopanic/BUILD.bazel new file mode 100644 index 0000000000..6da115715d --- /dev/null +++ b/tools/analyzers/nopanic/BUILD.bazel @@ -0,0 +1,26 @@ +load("@prysm//tools/go:def.bzl", "go_library", "go_test") + +go_library( + name = "go_default_library", + srcs = ["analyzer.go"], + importpath = "github.com/prysmaticlabs/prysm/v5/tools/analyzers/nopanic", + visibility = ["//visibility:public"], + deps = [ + "@org_golang_x_tools//go/analysis:go_default_library", + "@org_golang_x_tools//go/analysis/passes/inspect:go_default_library", + "@org_golang_x_tools//go/ast/inspector:go_default_library", + ], +) + +go_test( + name = "go_default_test", + srcs = ["analyzer_test.go"], + data = glob(["testdata/**"]) + [ + "@go_sdk//:files", + ], + embed = [":go_default_library"], + deps = [ + "//build/bazel:go_default_library", + "@org_golang_x_tools//go/analysis/analysistest:go_default_library", + ], +) diff --git a/tools/analyzers/nopanic/analyzer.go b/tools/analyzers/nopanic/analyzer.go new file mode 100644 index 0000000000..8c636c9825 --- /dev/null +++ b/tools/analyzers/nopanic/analyzer.go @@ -0,0 +1,86 @@ +package nopanic + +import ( + "errors" + "go/ast" + "strings" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" +) + +const Doc = "Tool to discourage the use of panic(), except in init functions" + +var errNoPanic = errors.New("panic() should not be used, except in rare situations or init functions") + +// Analyzer runs static analysis. +var Analyzer = &analysis.Analyzer{ + Name: "nopanic", + Doc: Doc, + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: run, +} + +func run(pass *analysis.Pass) (interface{}, error) { + inspection, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + if !ok { + return nil, errors.New("analyzer is not type *inspector.Inspector") + } + + nodeFilter := []ast.Node{ + (*ast.CallExpr)(nil), + } + + inspection.WithStack(nodeFilter, func(n ast.Node, push bool, stack []ast.Node) bool { + switch stmt := n.(type) { + case *ast.CallExpr: + if isPanic(stmt) && !hasExclusion(pass, stack) { + pass.Report(analysis.Diagnostic{ + Pos: n.Pos(), + End: n.End(), + Message: errNoPanic.Error(), + }) + return false + } + } + + return true + }) + + return nil, nil +} + +// isPanic returns true if the method name is exactly "panic", case insensitive. +func isPanic(call *ast.CallExpr) bool { + i, ok := call.Fun.(*ast.Ident) + return ok && strings.ToLower(i.Name) == "panic" +} + +// hasExclusion looks at the ast stack and if any node in the stack has the magic words "lint:nopanic" +// then this node is considered excluded. This allows exclusions to be placed at the function or package level. +// This method also excludes init functions. +func hasExclusion(pass *analysis.Pass, stack []ast.Node) bool { + if len(stack) < 2 { + return false + } + // The first value in the stack is always the file, then the second value would be a package level function. + // Init functions are always package level. + if fd, ok := stack[1].(*ast.FuncDecl); ok && fd.Name.Name == "init" { + return true + } + + // Build a comment map and scan the comments of this node stack. + cm := ast.NewCommentMap(pass.Fset, stack[0], stack[0].(*ast.File).Comments) + for _, n := range stack { + for _, cmt := range cm[n] { + for _, l := range cmt.List { + if strings.Contains(l.Text, "lint:nopanic") { + return true + } + } + } + } + + return false +} diff --git a/tools/analyzers/nopanic/analyzer_test.go b/tools/analyzers/nopanic/analyzer_test.go new file mode 100644 index 0000000000..dc0a28e3f1 --- /dev/null +++ b/tools/analyzers/nopanic/analyzer_test.go @@ -0,0 +1,20 @@ +package nopanic + +import ( + "testing" + + "github.com/prysmaticlabs/prysm/v5/build/bazel" + "golang.org/x/tools/go/analysis/analysistest" +) + +func init() { + if bazel.BuiltWithBazel() { + bazel.SetGoEnv() + } +} + +func TestAnalyzer(t *testing.T) { + testdata := bazel.TestDataPath(t) + analysistest.TestData = func() string { return testdata } + analysistest.Run(t, testdata, Analyzer) +} diff --git a/tools/analyzers/nopanic/testdata/code.go b/tools/analyzers/nopanic/testdata/code.go new file mode 100644 index 0000000000..a4ba744b9d --- /dev/null +++ b/tools/analyzers/nopanic/testdata/code.go @@ -0,0 +1,26 @@ +package testdata + +func A() error { + panic("feeling cute, let's panic!") // want "panic\\(\\) should not be used, except in rare situations or init functions" +} + +func B(foo interface{}) error { + if foo == nil { + panic("impossible condition: foo is nil") //lint:nopanic -- This is validated by the caller. + } + + if _, ok := foo.(string); !ok { + panic("foo should not be a string!!") // want "panic\\(\\) should not be used, except in rare situations or init functions" + } + + return nil +} + +//lint:nopanic -- This is method is really safe ;) +func C(foo interface{}) error { + if foo == nil { + panic("impossible condition: foo is nil") + } + + return nil +} diff --git a/tools/analyzers/nopanic/testdata/init.go b/tools/analyzers/nopanic/testdata/init.go new file mode 100644 index 0000000000..d7aecdbd21 --- /dev/null +++ b/tools/analyzers/nopanic/testdata/init.go @@ -0,0 +1,7 @@ +package testdata + +func init() { + if false { + panic("this should never happen") + } +} diff --git a/tools/analyzers/nopanic/testdata/pkg.go b/tools/analyzers/nopanic/testdata/pkg.go new file mode 100644 index 0000000000..96a5102ca9 --- /dev/null +++ b/tools/analyzers/nopanic/testdata/pkg.go @@ -0,0 +1,8 @@ +// package testdata +// +// lint:nopanic -- This package is so safe that you can trust it... +package testdata + +func dive() { + panic("BELLY FLOP!!!!") +} diff --git a/tools/bootnode/bootnode.go b/tools/bootnode/bootnode.go index 2a41f1755e..b41a107e18 100644 --- a/tools/bootnode/bootnode.go +++ b/tools/bootnode/bootnode.go @@ -7,6 +7,7 @@ * * Usage: Run bootnode --help for flag options. */ +// lint:nopanic -- This tool is OK to panic. package main import ( diff --git a/tools/enr-calculator/main.go b/tools/enr-calculator/main.go index 475df32128..7b030d0a5c 100644 --- a/tools/enr-calculator/main.go +++ b/tools/enr-calculator/main.go @@ -1,5 +1,7 @@ // This binary is a simple rest API endpoint to calculate // the ENR value of a node given its private key,ip address and port. +// +// lint:nopanic -- Tooling allowed to panic. package main import ( diff --git a/tools/eth1voting/votes.go b/tools/eth1voting/votes.go index c1afd1d235..4bd40651b6 100644 --- a/tools/eth1voting/votes.go +++ b/tools/eth1voting/votes.go @@ -37,7 +37,7 @@ func (v *votes) Insert(blk interfaces.ReadOnlyBeaconBlock) { e1d := blk.Body().Eth1Data() htr, err := e1d.HashTreeRoot() if err != nil { - panic(err) + panic(err) // lint:nopanic -- Panic is OK for this tool. } v.hashes[bytesutil.ToBytes32(e1d.BlockHash)]++ v.roots[bytesutil.ToBytes32(e1d.DepositRoot)]++ diff --git a/tools/gocovmerge/main.go b/tools/gocovmerge/main.go index c4fe415604..d276812b37 100644 --- a/tools/gocovmerge/main.go +++ b/tools/gocovmerge/main.go @@ -2,6 +2,7 @@ // merges them into one profile // // Copied, with minor changes, from https://github.com/wadey/gocovmerge under BSD-2-Clause License +// lint:nopanic -- Tooling that is allowed to panic. package main import ( diff --git a/validator/accounts/testing/mock.go b/validator/accounts/testing/mock.go index a95d5b77e6..fcaf9a1f6e 100644 --- a/validator/accounts/testing/mock.go +++ b/validator/accounts/testing/mock.go @@ -1,3 +1,6 @@ +// package mock +// +// lint:nopanic -- Test / mock code, allowed to panic. package mock import ( diff --git a/validator/client/beacon-api/beacon_api_beacon_chain_client.go b/validator/client/beacon-api/beacon_api_beacon_chain_client.go index 3779e526cf..0da60408c4 100644 --- a/validator/client/beacon-api/beacon_api_beacon_chain_client.go +++ b/validator/client/beacon-api/beacon_api_beacon_chain_client.go @@ -148,7 +148,7 @@ func (c beaconApiChainClient) ValidatorBalances(ctx context.Context, in *ethpb.L } // TODO: Implement me - panic("beaconApiChainClient.ValidatorBalances is not implemented. To use a fallback client, pass a fallback client as the last argument of NewBeaconApiChainClientWithFallback.") + return nil, errors.New("beaconApiChainClient.ValidatorBalances is not implemented. To use a fallback client, pass a fallback client as the last argument of NewBeaconApiChainClientWithFallback.") } func (c beaconApiChainClient) Validators(ctx context.Context, in *ethpb.ListValidatorsRequest) (*ethpb.Validators, error) { diff --git a/validator/client/beacon-api/beacon_api_node_client.go b/validator/client/beacon-api/beacon_api_node_client.go index 5b094a2e7b..bdbbc3ccf4 100644 --- a/validator/client/beacon-api/beacon_api_node_client.go +++ b/validator/client/beacon-api/beacon_api_node_client.go @@ -100,7 +100,7 @@ func (c *beaconApiNodeClient) Peers(ctx context.Context, in *empty.Empty) (*ethp } // TODO: Implement me - panic("beaconApiNodeClient.Peers is not implemented. To use a fallback client, pass a fallback client as the last argument of NewBeaconApiNodeClientWithFallback.") + return nil, errors.New("beaconApiNodeClient.Peers is not implemented. To use a fallback client, pass a fallback client as the last argument of NewBeaconApiNodeClientWithFallback.") } func (c *beaconApiNodeClient) IsHealthy(ctx context.Context) bool { diff --git a/validator/client/service.go b/validator/client/service.go index 63ff04227a..552b002931 100644 --- a/validator/client/service.go +++ b/validator/client/service.go @@ -146,7 +146,7 @@ func (v *ValidatorService) Start() { BufferItems: 64, // number of keys per Get buffer. }) if err != nil { - panic(err) + panic(err) // lint:nopanic -- Only errors on misconfiguration of config values. } aggregatedSlotCommitteeIDCache := lruwrpr.New(int(params.BeaconConfig().MaxCommitteesPerSlot)) diff --git a/validator/db/filesystem/attester_protection.go b/validator/db/filesystem/attester_protection.go index 5a48e7f78f..afce30f01b 100644 --- a/validator/db/filesystem/attester_protection.go +++ b/validator/db/filesystem/attester_protection.go @@ -27,7 +27,7 @@ func (*Store) SaveEIPImportBlacklistedPublicKeys(_ context.Context, _ [][fieldpa // SigningRootAtTargetEpoch is implemented only to satisfy the interface. func (*Store) SigningRootAtTargetEpoch(_ context.Context, _ [fieldparams.BLSPubkeyLength]byte, _ primitives.Epoch) ([]byte, error) { - panic("not implemented") + return nil, errors.New("not implemented") } // LowestSignedTargetEpoch returns the lowest signed target epoch for a public key, a boolean indicating if it exists and an error. diff --git a/validator/db/filesystem/proposer_protection.go b/validator/db/filesystem/proposer_protection.go index 086c9527a7..5b7b6d42c5 100644 --- a/validator/db/filesystem/proposer_protection.go +++ b/validator/db/filesystem/proposer_protection.go @@ -14,12 +14,12 @@ import ( // HighestSignedProposal is implemented only to satisfy the interface. func (*Store) HighestSignedProposal(_ context.Context, _ [fieldparams.BLSPubkeyLength]byte) (primitives.Slot, bool, error) { - panic("not implemented") + return 0, false, errors.New("not implemented") } // LowestSignedProposal is implemented only to satisfy the interface. func (*Store) LowestSignedProposal(_ context.Context, _ [fieldparams.BLSPubkeyLength]byte) (primitives.Slot, bool, error) { - panic("not implemented") + return 0, false, errors.New("not implemented") } // ProposalHistoryForPubKey returns the proposal history for a given public key. @@ -45,7 +45,7 @@ func (s *Store) ProposalHistoryForPubKey(_ context.Context, publicKey [fieldpara // ProposalHistoryForSlot is implemented only to satisfy the interface. func (*Store) ProposalHistoryForSlot(_ context.Context, _ [fieldparams.BLSPubkeyLength]byte, _ primitives.Slot) ([fieldparams.RootLength]byte, bool, bool, error) { - panic("not implemented") + return [32]byte{}, false, false, errors.New("not implemented") } // SaveProposalHistoryForSlot checks if the incoming proposal is valid regarding EIP-3076 minimal slashing protection. diff --git a/validator/node/node.go b/validator/node/node.go index 383688dba7..61e915a456 100644 --- a/validator/node/node.go +++ b/validator/node/node.go @@ -162,7 +162,7 @@ func (c *ValidatorClient) Start() { log.WithField("times", i-1).Info("Already shutting down, interrupt more to panic.") } } - panic("Panic closing the validator client") + panic("Panic closing the validator client") // lint:nopanic -- Panic is requested by user. }() // Wait for stop channel to be closed.