diff --git a/BUILD.bazel b/BUILD.bazel index 50fcf375c..e690fa95b 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -120,6 +120,7 @@ nogo( "//tools/analyzers/shadowpredecl:go_tool_library", "//tools/analyzers/nop:go_tool_library", "//tools/analyzers/slicedirect:go_tool_library", + "//tools/analyzers/interfacechecker:go_tool_library", "//tools/analyzers/ineffassign:go_tool_library", "//tools/analyzers/properpermissions:go_tool_library", ] + select({ diff --git a/beacon-chain/blockchain/head.go b/beacon-chain/blockchain/head.go index f50687d60..5b3909ce6 100644 --- a/beacon-chain/blockchain/head.go +++ b/beacon-chain/blockchain/head.go @@ -113,7 +113,7 @@ func (s *Service) saveHead(ctx context.Context, headRoot [32]byte) error { if err != nil { return errors.Wrap(err, "could not retrieve head state in DB") } - if newHeadState == nil { + if newHeadState == nil || newHeadState.IsNil() { return errors.New("cannot save nil head state") } @@ -262,7 +262,7 @@ func (s *Service) cacheJustifiedStateBalances(ctx context.Context, justifiedRoot return err } } - if justifiedState == nil { + if justifiedState == nil || justifiedState.IsNil() { return errors.New("justified state can't be nil") } diff --git a/beacon-chain/blockchain/info.go b/beacon-chain/blockchain/info.go index 6ee41e2d4..53c2f5b95 100644 --- a/beacon-chain/blockchain/info.go +++ b/beacon-chain/blockchain/info.go @@ -39,7 +39,7 @@ func (s *Service) TreeHandler(w http.ResponseWriter, r *http.Request) { log.WithError(err).Error("Could not get head state") return } - if headState == nil { + if headState == nil || headState.IsNil() { if _, err := w.Write([]byte("Unavailable during initial syncing")); err != nil { log.WithError(err).Error("Failed to render p2p info page") } diff --git a/beacon-chain/blockchain/process_attestation_helpers.go b/beacon-chain/blockchain/process_attestation_helpers.go index d00212cc6..eaba729c7 100644 --- a/beacon-chain/blockchain/process_attestation_helpers.go +++ b/beacon-chain/blockchain/process_attestation_helpers.go @@ -29,7 +29,7 @@ func (s *Service) getAttPreState(ctx context.Context, c *ethpb.Checkpoint) (ifac if err != nil { return nil, errors.Wrap(err, "could not get cached checkpoint state") } - if cachedState != nil { + if cachedState != nil && !cachedState.IsNil() { return cachedState, nil } diff --git a/beacon-chain/blockchain/process_block.go b/beacon-chain/blockchain/process_block.go index 7912cb62d..1f4cb78f0 100644 --- a/beacon-chain/blockchain/process_block.go +++ b/beacon-chain/blockchain/process_block.go @@ -203,7 +203,7 @@ func (s *Service) onBlockBatch(ctx context.Context, blks []*ethpb.SignedBeaconBl if err != nil { return nil, nil, err } - if preState == nil { + if preState == nil || preState.IsNil() { return nil, nil, fmt.Errorf("nil pre state for slot %d", b.Slot) } diff --git a/beacon-chain/blockchain/process_block_helpers.go b/beacon-chain/blockchain/process_block_helpers.go index d8bbe86cb..d8dff487f 100644 --- a/beacon-chain/blockchain/process_block_helpers.go +++ b/beacon-chain/blockchain/process_block_helpers.go @@ -39,7 +39,7 @@ func (s *Service) getBlockPreState(ctx context.Context, b *ethpb.BeaconBlock) (i if err != nil { return nil, errors.Wrapf(err, "could not get pre state for slot %d", b.Slot) } - if preState == nil { + if preState == nil || preState.IsNil() { return nil, errors.Wrapf(err, "nil pre state for slot %d", b.Slot) } diff --git a/beacon-chain/blockchain/service.go b/beacon-chain/blockchain/service.go index 001eb6013..c379e1de2 100644 --- a/beacon-chain/blockchain/service.go +++ b/beacon-chain/blockchain/service.go @@ -135,7 +135,7 @@ func (s *Service) Start() { attestationProcessorSubscribed := make(chan struct{}, 1) // If the chain has already been initialized, simply start the block processing routine. - if beaconState != nil { + if beaconState != nil && !beaconState.IsNil() { log.Info("Blockchain data already exists in DB, initializing...") s.genesisTime = time.Unix(int64(beaconState.GenesisTime()), 0) s.cfg.OpsService.SetGenesisTime(beaconState.GenesisTime()) diff --git a/beacon-chain/core/blocks/eth1_data.go b/beacon-chain/core/blocks/eth1_data.go index 09dcc1357..278c981b1 100644 --- a/beacon-chain/core/blocks/eth1_data.go +++ b/beacon-chain/core/blocks/eth1_data.go @@ -21,7 +21,7 @@ import ( // if state.eth1_data_votes.count(body.eth1_data) * 2 > EPOCHS_PER_ETH1_VOTING_PERIOD * SLOTS_PER_EPOCH: // state.eth1_data = body.eth1_data func ProcessEth1DataInBlock(_ context.Context, beaconState iface.BeaconState, eth1Data *ethpb.Eth1Data) (iface.BeaconState, error) { - if beaconState == nil { + if beaconState == nil || beaconState.IsNil() { return nil, errors.New("nil state") } if err := beaconState.AppendEth1DataVotes(eth1Data); err != nil { diff --git a/beacon-chain/core/state/transition.go b/beacon-chain/core/state/transition.go index d6b7be3e7..ec6b1bb74 100644 --- a/beacon-chain/core/state/transition.go +++ b/beacon-chain/core/state/transition.go @@ -179,7 +179,7 @@ func ProcessSlotsUsingNextSlotCache( } // If the next slot state is not nil (i.e. cache hit). // We replace next slot state with parent state. - if nextSlotState != nil { + if nextSlotState != nil && !nextSlotState.IsNil() { parentState = nextSlotState } @@ -208,7 +208,7 @@ func ProcessSlotsUsingNextSlotCache( func ProcessSlots(ctx context.Context, state iface.BeaconState, slot types.Slot) (iface.BeaconState, error) { ctx, span := trace.StartSpan(ctx, "core.state.ProcessSlots") defer span.End() - if state == nil { + if state == nil || state.IsNil() { return nil, errors.New("nil state") } span.AddAttributes(trace.Int64Attribute("slots", int64(slot)-int64(state.Slot()))) @@ -399,7 +399,7 @@ func ProcessEpochPrecompute(ctx context.Context, state iface.BeaconState) (iface defer span.End() span.AddAttributes(trace.Int64Attribute("epoch", int64(helpers.CurrentEpoch(state)))) - if state == nil { + if state == nil || state.IsNil() { return nil, errors.New("nil state") } vp, bp, err := precompute.New(ctx, state) diff --git a/beacon-chain/core/state/transition_no_verify_sig.go b/beacon-chain/core/state/transition_no_verify_sig.go index bbec55d2e..1da528cfb 100644 --- a/beacon-chain/core/state/transition_no_verify_sig.go +++ b/beacon-chain/core/state/transition_no_verify_sig.go @@ -121,7 +121,7 @@ func CalculateStateRoot( traceutil.AnnotateError(span, ctx.Err()) return [32]byte{}, ctx.Err() } - if state == nil { + if state == nil || state.IsNil() { return [32]byte{}, errors.New("nil state") } if signed == nil || signed.Block == nil { diff --git a/beacon-chain/db/kv/genesis.go b/beacon-chain/db/kv/genesis.go index d876f0d73..b7d157570 100644 --- a/beacon-chain/db/kv/genesis.go +++ b/beacon-chain/db/kv/genesis.go @@ -70,7 +70,7 @@ func (s *Store) LoadGenesis(ctx context.Context, r io.Reader) error { } // If some different genesis state existed already, return an error. The same genesis state is // considered a no-op. - if existing != nil { + if existing != nil && !existing.IsNil() { a, err := existing.HashTreeRoot(ctx) if err != nil { return err @@ -108,7 +108,7 @@ func (s *Store) EnsureEmbeddedGenesis(ctx context.Context) error { if err != nil { return err } - if gs != nil { + if gs != nil && !gs.IsNil() { return s.SaveGenesisData(ctx, gs) } return nil diff --git a/beacon-chain/db/kv/state.go b/beacon-chain/db/kv/state.go index c9f63437a..03f4370d8 100644 --- a/beacon-chain/db/kv/state.go +++ b/beacon-chain/db/kv/state.go @@ -294,7 +294,7 @@ func (s *Store) HighestSlotStatesBelow(ctx context.Context, slot types.Slot) ([] return nil, err } } - if st == nil { + if st == nil || st.IsNil() { st, err = s.GenesisState(ctx) if err != nil { return nil, err diff --git a/beacon-chain/p2p/gossip_scoring_params.go b/beacon-chain/p2p/gossip_scoring_params.go index 914ce555d..5c71bebdb 100644 --- a/beacon-chain/p2p/gossip_scoring_params.go +++ b/beacon-chain/p2p/gossip_scoring_params.go @@ -113,7 +113,7 @@ func (s *Service) retrieveActiveValidators() (uint64, error) { if err != nil { return 0, err } - if genState == nil { + if genState == nil || genState.IsNil() { return 0, errors.New("no genesis state exists") } activeVals, err := helpers.ActiveValidatorCount(genState, helpers.CurrentEpoch(genState)) @@ -128,7 +128,7 @@ func (s *Service) retrieveActiveValidators() (uint64, error) { if err != nil { return 0, err } - if bState == nil { + if bState == nil || bState.IsNil() { return 0, errors.Errorf("no state with root %#x exists", rt) } activeVals, err := helpers.ActiveValidatorCount(bState, helpers.CurrentEpoch(bState)) diff --git a/beacon-chain/powchain/deposit.go b/beacon-chain/powchain/deposit.go index fbda20478..3d2228c6e 100644 --- a/beacon-chain/powchain/deposit.go +++ b/beacon-chain/powchain/deposit.go @@ -17,7 +17,7 @@ func (s *Service) processDeposit(ctx context.Context, eth1Data *ethpb.Eth1Data, if err != nil { return errors.Wrap(err, "could not process pre-genesis deposits") } - if beaconState != nil { + if beaconState != nil && !beaconState.IsNil() { s.preGenesisState = beaconState } return nil diff --git a/beacon-chain/powchain/service.go b/beacon-chain/powchain/service.go index dc3a478ed..454fe0e43 100644 --- a/beacon-chain/powchain/service.go +++ b/beacon-chain/powchain/service.go @@ -247,7 +247,7 @@ func (s *Service) Start() { if err != nil { log.Fatal(err) } - if genState == nil { + if genState == nil || genState.IsNil() { log.Fatal("cannot create genesis state: no eth1 http endpoint defined") } } @@ -611,7 +611,7 @@ func (s *Service) initDepositCaches(ctx context.Context, ctrs []*protodb.Deposit if err != nil { return errors.Wrap(err, "could not get finalized state") } - if fState == nil { + if fState == nil || fState.IsNil() { return errors.Errorf("finalized state with root %#x does not exist in the db", rt) } // Set deposit index to the one in the current archived state. @@ -1014,7 +1014,7 @@ func (s *Service) ensureValidPowchainData(ctx context.Context) error { return err } // Exit early if no genesis state is saved. - if genState == nil { + if genState == nil || genState.IsNil() { return nil } eth1Data, err := s.cfg.BeaconDB.PowchainData(ctx) diff --git a/beacon-chain/rpc/beacon/validators_stream.go b/beacon-chain/rpc/beacon/validators_stream.go index bfac01f4e..e26413d32 100644 --- a/beacon-chain/rpc/beacon/validators_stream.go +++ b/beacon-chain/rpc/beacon/validators_stream.go @@ -99,7 +99,7 @@ func (bs *Server) StreamValidatorsInfo(stream ethpb.BeaconChain_StreamValidators if err != nil { return status.Error(codes.Internal, "Could not access head state") } - if headState == nil { + if headState == nil || headState.IsNil() { return status.Error(codes.Internal, "Not ready to serve information") } @@ -260,7 +260,7 @@ func (is *infostream) generateValidatorsInfo(pubKeys [][]byte) ([]*ethpb.Validat if err != nil { return nil, status.Error(codes.Internal, "Could not access head state") } - if headState == nil { + if headState == nil || headState.IsNil() { return nil, status.Error(codes.Internal, "Not ready to serve information") } epoch := types.Epoch(headState.Slot() / params.BeaconConfig().SlotsPerEpoch) @@ -450,7 +450,7 @@ func (is *infostream) handleBlockProcessed() { log.Warn("Could not access head state for infostream") return } - if headState == nil { + if headState == nil || headState.IsNil() { // We aren't ready to serve information return } diff --git a/beacon-chain/rpc/validator/attester.go b/beacon-chain/rpc/validator/attester.go index b53b1c4c2..5b62ab398 100644 --- a/beacon-chain/rpc/validator/attester.go +++ b/beacon-chain/rpc/validator/attester.go @@ -90,7 +90,7 @@ func (vs *Server) GetAttestationData(ctx context.Context, req *ethpb.Attestation return nil, status.Errorf(codes.Internal, "Could not get historical head state: %v", err) } } - if headState == nil { + if headState == nil || headState.IsNil() { return nil, status.Error(codes.Internal, "Could not lookup parent state from head.") } diff --git a/beacon-chain/rpc/validator/server.go b/beacon-chain/rpc/validator/server.go index 14f02dead..17b662920 100644 --- a/beacon-chain/rpc/validator/server.go +++ b/beacon-chain/rpc/validator/server.go @@ -154,7 +154,7 @@ func (vs *Server) WaitForChainStart(_ *emptypb.Empty, stream ethpb.BeaconNodeVal if err != nil { return status.Errorf(codes.Internal, "Could not retrieve head state: %v", err) } - if head != nil { + if head != nil && !head.IsNil() { res := ðpb.ChainStartResponse{ Started: true, GenesisTime: head.GenesisTime(), diff --git a/beacon-chain/rpc/validator/status.go b/beacon-chain/rpc/validator/status.go index 1d057e191..c1c65b2fa 100644 --- a/beacon-chain/rpc/validator/status.go +++ b/beacon-chain/rpc/validator/status.go @@ -219,7 +219,7 @@ func (vs *Server) validatorStatus( } func statusForPubKey(headState iface.ReadOnlyBeaconState, pubKey []byte) (ethpb.ValidatorStatus, types.ValidatorIndex, error) { - if headState == nil { + if headState == nil || headState.IsNil() { return ethpb.ValidatorStatus_UNKNOWN_STATUS, 0, errors.New("head state does not exist") } idx, ok := headState.ValidatorIndexByPubkey(bytesutil.ToBytes48(pubKey)) diff --git a/beacon-chain/state/interface/phase0.go b/beacon-chain/state/interface/phase0.go index 3a872115f..d2337c98d 100644 --- a/beacon-chain/state/interface/phase0.go +++ b/beacon-chain/state/interface/phase0.go @@ -43,6 +43,7 @@ type ReadOnlyBeaconState interface { Slashings() []uint64 FieldReferencesCount() map[string]uint64 MarshalSSZ() ([]byte, error) + IsNil() bool } // WriteOnlyBeaconState defines a struct which only has write access to beacon state methods. diff --git a/beacon-chain/state/stateV0/state_test.go b/beacon-chain/state/stateV0/state_test.go index 0056ca671..ea9859eeb 100644 --- a/beacon-chain/state/stateV0/state_test.go +++ b/beacon-chain/state/stateV0/state_test.go @@ -94,3 +94,14 @@ func TestBeaconState_NoDeadlock(t *testing.T) { // Test will not terminate in the event of a deadlock. wg.Wait() } + +func TestStateTrie_IsNil(t *testing.T) { + var emptyState *BeaconState + assert.Equal(t, true, emptyState.IsNil()) + + emptyProto := &BeaconState{state: nil} + assert.Equal(t, true, emptyProto.IsNil()) + + nonNilState := &BeaconState{state: &pb.BeaconState{}} + assert.Equal(t, false, nonNilState.IsNil()) +} diff --git a/beacon-chain/state/stateV0/state_trie.go b/beacon-chain/state/stateV0/state_trie.go index 36aad4cb4..862c20aaf 100644 --- a/beacon-chain/state/stateV0/state_trie.go +++ b/beacon-chain/state/stateV0/state_trie.go @@ -379,6 +379,12 @@ func (b *BeaconState) FieldReferencesCount() map[string]uint64 { return refMap } +// IsNil checks if the state and the underlying proto +// object are nil. +func (b *BeaconState) IsNil() bool { + return b == nil || b.state == nil +} + func (b *BeaconState) rootSelector(ctx context.Context, field fieldIndex) ([32]byte, error) { ctx, span := trace.StartSpan(ctx, "beaconState.rootSelector") defer span.End() diff --git a/beacon-chain/state/stategen/getter.go b/beacon-chain/state/stategen/getter.go index 31c0c40c2..bce066210 100644 --- a/beacon-chain/state/stategen/getter.go +++ b/beacon-chain/state/stategen/getter.go @@ -78,7 +78,7 @@ func (s *State) StateByRootInitialSync(ctx context.Context, blockRoot [32]byte) if err != nil { return nil, errors.Wrap(err, "could not get ancestor state") } - if startState == nil { + if startState == nil || startState.IsNil() { return nil, errUnknownState } summary, err := s.stateSummary(ctx, blockRoot) @@ -149,7 +149,7 @@ func (s *State) loadStateByRoot(ctx context.Context, blockRoot [32]byte) (iface. // First, it checks if the state exists in hot state cache. cachedState := s.hotStateCache.get(blockRoot) - if cachedState != nil { + if cachedState != nil && !cachedState.IsNil() { return cachedState, nil } @@ -179,7 +179,7 @@ func (s *State) loadStateByRoot(ctx context.Context, blockRoot [32]byte) (iface. if err != nil { return nil, errors.Wrap(err, "could not get ancestor state") } - if startState == nil { + if startState == nil || startState.IsNil() { return nil, errUnknownBoundaryState } diff --git a/beacon-chain/state/stategen/replay.go b/beacon-chain/state/stategen/replay.go index 0b150d1da..840bc4353 100644 --- a/beacon-chain/state/stategen/replay.go +++ b/beacon-chain/state/stategen/replay.go @@ -156,7 +156,7 @@ func executeStateTransitionStateGen( func processSlotsStateGen(ctx context.Context, state iface.BeaconState, slot types.Slot) (iface.BeaconState, error) { ctx, span := trace.StartSpan(ctx, "stategen.ProcessSlotsStateGen") defer span.End() - if state == nil { + if state == nil || state.IsNil() { return nil, errUnknownState } diff --git a/beacon-chain/state/stategen/service.go b/beacon-chain/state/stategen/service.go index 7491a04e1..dc6cd7a50 100644 --- a/beacon-chain/state/stategen/service.go +++ b/beacon-chain/state/stategen/service.go @@ -101,7 +101,7 @@ func (s *State) Resume(ctx context.Context) (iface.BeaconState, error) { if err != nil { return nil, err } - if fState == nil { + if fState == nil || fState.IsNil() { return nil, errors.New("finalized state not found in disk") } diff --git a/nogo_config.json b/nogo_config.json index ad54ca3c9..c80c0e321 100644 --- a/nogo_config.json +++ b/nogo_config.json @@ -84,6 +84,13 @@ ".*_test\\.go": "Only tests" } }, + "interfacechecker": { + "exclude_files": { + "external/.*": "Third party code", + "rules_go_work-.*": "Third party code", + ".*/.*_test\\.go": "Tests are OK to ignore this check for" + } + }, "cryptorand": { "only_files": { "beacon-chain/.*": "", diff --git a/tools/analyzers/interfacechecker/BUILD.bazel b/tools/analyzers/interfacechecker/BUILD.bazel new file mode 100644 index 000000000..6ac94a8a6 --- /dev/null +++ b/tools/analyzers/interfacechecker/BUILD.bazel @@ -0,0 +1,26 @@ +load("@prysm//tools/go:def.bzl", "go_library") +load("@io_bazel_rules_go//go:def.bzl", "go_tool_library") + +go_library( + name = "go_default_library", + srcs = ["analyzer.go"], + importpath = "github.com/prysmaticlabs/prysm/tools/analyzers/interfacechecker", + 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_tool_library( + name = "go_tool_library", + srcs = ["analyzer.go"], + importpath = "interfacechecker", + visibility = ["//visibility:public"], + deps = [ + "@org_golang_x_tools//go/analysis:go_tool_library", + "@org_golang_x_tools//go/analysis/passes/inspect:go_tool_library", + "@org_golang_x_tools//go/ast/inspector:go_tool_library", + ], +) diff --git a/tools/analyzers/interfacechecker/analyzer.go b/tools/analyzers/interfacechecker/analyzer.go new file mode 100644 index 000000000..09337c971 --- /dev/null +++ b/tools/analyzers/interfacechecker/analyzer.go @@ -0,0 +1,97 @@ +// Package interfacechecker implements a static analyzer to prevent incorrect conditional checks on select interfaces. +package interfacechecker + +import ( + "errors" + "go/ast" + "go/token" + "go/types" + "strings" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" +) + +// Doc explaining the tool. +const Doc = "Enforce usage of proper conditional check for interfaces" + +// Analyzer runs static analysis. +var Analyzer = &analysis.Analyzer{ + Name: "interfacechecker", + Doc: Doc, + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: run, +} + +// These are the selected interfaces that we want to parse through and check nilness for. +var selectedInterfaces = []string{ + "interfaces.SignedBeaconBlock", + "interface.BeaconState", + "interface.ReadOnlyBeaconState", + "interface.WriteOnlyBeaconState", +} + +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.IfStmt)(nil), + } + + inspection.Preorder(nodeFilter, func(node ast.Node) { + stmt, ok := node.(*ast.IfStmt) + if !ok { + return + } + exp, ok := stmt.Cond.(*ast.BinaryExpr) + if !ok { + return + } + handleConditionalExpression(exp, pass) + }) + + return nil, nil +} + +func handleConditionalExpression(exp *ast.BinaryExpr, pass *analysis.Pass) { + identX, ok := exp.X.(*ast.Ident) + if !ok { + return + } + identY, ok := exp.Y.(*ast.Ident) + if !ok { + return + } + typeMap := pass.TypesInfo.Types + if _, ok := typeMap[identX].Type.(*types.Slice); ok { + return + } + if _, ok := typeMap[identY].Type.(*types.Slice); ok { + return + } + for _, iface := range selectedInterfaces { + xIsIface := strings.Contains(typeMap[identX].Type.String(), iface) + xIsNil := typeMap[identX].IsNil() + yisIface := strings.Contains(typeMap[identY].Type.String(), iface) + yIsNil := typeMap[identY].IsNil() + // Exit early if neither are of the desired interface + if !xIsIface && !yisIface { + continue + } + if xIsIface && yIsNil { + reportFailure(identX.Pos(), pass) + } + if yisIface && xIsNil { + reportFailure(identY.Pos(), pass) + } + } +} + +func reportFailure(pos token.Pos, pass *analysis.Pass) { + pass.Reportf(pos, "A single nilness check is being performed on an interface"+ + ", this check needs another accompanying nilness check on the underlying object for the interface.") +} diff --git a/tools/interop/export-genesis/main.go b/tools/interop/export-genesis/main.go index 88ada3ae6..207ecc346 100644 --- a/tools/interop/export-genesis/main.go +++ b/tools/interop/export-genesis/main.go @@ -34,7 +34,7 @@ func main() { if err != nil { panic(err) } - if gs == nil { + if gs == nil || gs.IsNil() { panic("nil genesis state") } b, err := gs.MarshalSSZ()