From 7a5010ecea370bfee81d3f5cd06e6c7dc11c4d84 Mon Sep 17 00:00:00 2001 From: Preston Van Loon Date: Thu, 9 Apr 2020 16:35:42 -0700 Subject: [PATCH] Add roughtime static code analysis and fix all violations (#5370) * Add roughtime static code analysis and fix all violations * Merge branch 'master' into enforce-roughtime --- BUILD.bazel | 1 + .../blockchain/process_attestation.go | 4 +- .../blockchain/receive_attestation.go | 4 +- beacon-chain/p2p/connmgr/BUILD.bazel | 2 + beacon-chain/p2p/connmgr/connmgr.go | 13 ++--- beacon-chain/p2p/connmgr/connmgr_test.go | 11 ++-- beacon-chain/powchain/service.go | 2 +- .../sync/initial-sync-old/round_robin.go | 3 +- .../sync/initial-sync/blocks_fetcher.go | 9 ++-- beacon-chain/sync/initial-sync/fsm.go | 5 +- nogo_config.json | 16 ++++++ shared/keystore/BUILD.bazel | 1 + shared/keystore/utils.go | 3 +- tools/analyzers/roughtime/BUILD.bazel | 25 +++++++++ tools/analyzers/roughtime/analyzer.go | 52 +++++++++++++++++++ 15 files changed, 126 insertions(+), 25 deletions(-) create mode 100644 tools/analyzers/roughtime/BUILD.bazel create mode 100644 tools/analyzers/roughtime/analyzer.go diff --git a/BUILD.bazel b/BUILD.bazel index e98e16e68..e1ba6a168 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -104,6 +104,7 @@ nogo( "@org_golang_x_tools//go/analysis/passes/inspect:go_tool_library", "@org_golang_x_tools//go/analysis/passes/asmdecl:go_tool_library", "//tools/analyzers/maligned:go_tool_library", + "//tools/analyzers/roughtime:go_tool_library", ], ) diff --git a/beacon-chain/blockchain/process_attestation.go b/beacon-chain/blockchain/process_attestation.go index f2e792d90..372498c54 100644 --- a/beacon-chain/blockchain/process_attestation.go +++ b/beacon-chain/blockchain/process_attestation.go @@ -3,7 +3,6 @@ package blockchain import ( "context" "fmt" - "time" "github.com/pkg/errors" ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1" @@ -11,6 +10,7 @@ import ( "github.com/prysmaticlabs/prysm/beacon-chain/flags" stateTrie "github.com/prysmaticlabs/prysm/beacon-chain/state" "github.com/prysmaticlabs/prysm/shared/bytesutil" + "github.com/prysmaticlabs/prysm/shared/roughtime" "go.opencensus.io/trace" ) @@ -93,7 +93,7 @@ func (s *Service) onAttestation(ctx context.Context, a *ethpb.Attestation) ([]ui genesisTime := baseState.GenesisTime() // Verify attestation target is from current epoch or previous epoch. - if err := s.verifyAttTargetEpoch(ctx, genesisTime, uint64(time.Now().Unix()), tgt); err != nil { + if err := s.verifyAttTargetEpoch(ctx, genesisTime, uint64(roughtime.Now().Unix()), tgt); err != nil { return nil, err } diff --git a/beacon-chain/blockchain/receive_attestation.go b/beacon-chain/blockchain/receive_attestation.go index f3a6ea71b..81587c100 100644 --- a/beacon-chain/blockchain/receive_attestation.go +++ b/beacon-chain/blockchain/receive_attestation.go @@ -3,7 +3,6 @@ package blockchain import ( "context" "fmt" - "time" "github.com/pkg/errors" ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1" @@ -13,6 +12,7 @@ import ( "github.com/prysmaticlabs/prysm/shared/bytesutil" "github.com/prysmaticlabs/prysm/shared/featureconfig" "github.com/prysmaticlabs/prysm/shared/params" + "github.com/prysmaticlabs/prysm/shared/roughtime" "github.com/prysmaticlabs/prysm/shared/slotutil" "github.com/sirupsen/logrus" "go.opencensus.io/trace" @@ -127,7 +127,7 @@ func (s *Service) processAttestation(subscribedToStateEvents chan struct{}) { // This verifies the epoch of input checkpoint is within current epoch and previous epoch // with respect to current time. Returns true if it's within, false if it's not. func (s *Service) verifyCheckpointEpoch(c *ethpb.Checkpoint) bool { - now := uint64(time.Now().Unix()) + now := uint64(roughtime.Now().Unix()) genesisTime := uint64(s.genesisTime.Unix()) currentSlot := (now - genesisTime) / params.BeaconConfig().SecondsPerSlot currentEpoch := helpers.SlotToEpoch(currentSlot) diff --git a/beacon-chain/p2p/connmgr/BUILD.bazel b/beacon-chain/p2p/connmgr/BUILD.bazel index 1b24a3a95..c3a3a3a45 100644 --- a/beacon-chain/p2p/connmgr/BUILD.bazel +++ b/beacon-chain/p2p/connmgr/BUILD.bazel @@ -6,6 +6,7 @@ go_library( importpath = "github.com/prysmaticlabs/prysm/beacon-chain/p2p/connmgr", visibility = ["//beacon-chain:__subpackages__"], deps = [ + "//shared/roughtime:go_default_library", "//shared/runutil:go_default_library", "@com_github_ipfs_go_log//:go_default_library", "@com_github_libp2p_go_libp2p_core//connmgr:go_default_library", @@ -20,6 +21,7 @@ go_test( srcs = ["connmgr_test.go"], embed = [":go_default_library"], deps = [ + "//shared/roughtime:go_default_library", "@com_github_ipfs_go_detect_race//:go_default_library", "@com_github_libp2p_go_libp2p_core//network:go_default_library", "@com_github_libp2p_go_libp2p_core//peer:go_default_library", diff --git a/beacon-chain/p2p/connmgr/connmgr.go b/beacon-chain/p2p/connmgr/connmgr.go index d9521991a..027d2d696 100644 --- a/beacon-chain/p2p/connmgr/connmgr.go +++ b/beacon-chain/p2p/connmgr/connmgr.go @@ -35,6 +35,7 @@ import ( "github.com/libp2p/go-libp2p-core/network" "github.com/libp2p/go-libp2p-core/peer" ma "github.com/multiformats/go-multiaddr" + "github.com/prysmaticlabs/prysm/shared/roughtime" "github.com/prysmaticlabs/prysm/shared/runutil" ) @@ -104,7 +105,7 @@ func (s *segment) tagInfoFor(p peer.ID) *peerInfo { // create a temporary peer to buffer early tags before the Connected notification arrives. pi = &peerInfo{ id: p, - firstSeen: time.Now(), // this timestamp will be updated when the first Connected notification arrives. + firstSeen: roughtime.Now(), // this timestamp will be updated when the first Connected notification arrives. temp: true, tags: make(map[string]int), conns: make(map[network.Conn]time.Time), @@ -224,7 +225,7 @@ func (cm *BasicConnMgr) TrimOpenConns(ctx context.Context) { c.Close() } - cm.lastTrim = time.Now() + cm.lastTrim = roughtime.Now() } // getConnsToClose runs the heuristics described in TrimOpenConns and returns the @@ -244,7 +245,7 @@ func (cm *BasicConnMgr) getConnsToClose(ctx context.Context) []network.Conn { npeers := cm.segments.countPeers() candidates := make([]*peerInfo, 0, npeers) ncandidates := 0 - gracePeriodStart := time.Now().Add(-cm.gracePeriod) + gracePeriodStart := roughtime.Now().Add(-cm.gracePeriod) cm.plk.RLock() for _, s := range cm.segments { @@ -446,7 +447,7 @@ func (nn *cmNotifee) Connected(n network.Network, c network.Conn) { if !ok { pinfo = &peerInfo{ id: id, - firstSeen: time.Now(), + firstSeen: roughtime.Now(), tags: make(map[string]int), conns: make(map[network.Conn]time.Time), } @@ -456,7 +457,7 @@ func (nn *cmNotifee) Connected(n network.Network, c network.Conn) { // Connected notification arrived: flip the temporary flag, and update the firstSeen // timestamp to the real one. pinfo.temp = false - pinfo.firstSeen = time.Now() + pinfo.firstSeen = roughtime.Now() } _, ok = pinfo.conns[c] @@ -465,7 +466,7 @@ func (nn *cmNotifee) Connected(n network.Network, c network.Conn) { return } - pinfo.conns[c] = time.Now() + pinfo.conns[c] = roughtime.Now() atomic.AddInt32(&cm.connCount, 1) } diff --git a/beacon-chain/p2p/connmgr/connmgr_test.go b/beacon-chain/p2p/connmgr/connmgr_test.go index a66ebadc1..bc864270f 100644 --- a/beacon-chain/p2p/connmgr/connmgr_test.go +++ b/beacon-chain/p2p/connmgr/connmgr_test.go @@ -6,12 +6,11 @@ import ( "time" detectrace "github.com/ipfs/go-detect-race" - "github.com/libp2p/go-libp2p-core/network" "github.com/libp2p/go-libp2p-core/peer" - tu "github.com/libp2p/go-libp2p-core/test" ma "github.com/multiformats/go-multiaddr" + "github.com/prysmaticlabs/prysm/shared/roughtime" ) type tconn struct { @@ -116,12 +115,12 @@ func TestConnsToClose(t *testing.T) { } func TestGetTagInfo(t *testing.T) { - start := time.Now() + start := roughtime.Now() cm := NewConnManager(1, 1, 10*time.Minute) not := cm.Notifee() conn := randConn(t, nil) not.Connected(nil, conn) - end := time.Now() + end := roughtime.Now() other := tu.RandPeerIDFatal(t) tag := cm.GetTagInfo(other) @@ -227,14 +226,14 @@ func TestUntagPeer(t *testing.T) { } func TestGetInfo(t *testing.T) { - start := time.Now() + start := roughtime.Now() gp := 10 * time.Minute cm := NewConnManager(1, 5, gp) not := cm.Notifee() conn := randConn(t, nil) not.Connected(nil, conn) cm.TrimOpenConns(context.Background()) - end := time.Now() + end := roughtime.Now() info := cm.GetInfo() if info.HighWater != 5 { diff --git a/beacon-chain/powchain/service.go b/beacon-chain/powchain/service.go index 1fe6cedc7..1dd09a93c 100644 --- a/beacon-chain/powchain/service.go +++ b/beacon-chain/powchain/service.go @@ -522,7 +522,7 @@ func (s *Service) handleDelayTicker() { // use a 5 minutes timeout for block time, because the max mining time is 278 sec (block 7208027) // (analyzed the time of the block from 2018-09-01 to 2019-02-13) - fiveMinutesTimeout := time.Now().Add(-5 * time.Minute) + fiveMinutesTimeout := roughtime.Now().Add(-5 * time.Minute) // check that web3 client is syncing if time.Unix(int64(s.latestEth1Data.BlockTime), 0).Before(fiveMinutesTimeout) && roughtime.Now().Second()%15 == 0 { log.Warn("eth1 client is not syncing") diff --git a/beacon-chain/sync/initial-sync-old/round_robin.go b/beacon-chain/sync/initial-sync-old/round_robin.go index ee5125bf3..3d79032fb 100644 --- a/beacon-chain/sync/initial-sync-old/round_robin.go +++ b/beacon-chain/sync/initial-sync-old/round_robin.go @@ -24,6 +24,7 @@ import ( "github.com/prysmaticlabs/prysm/shared/featureconfig" "github.com/prysmaticlabs/prysm/shared/mathutil" "github.com/prysmaticlabs/prysm/shared/params" + "github.com/prysmaticlabs/prysm/shared/roughtime" "github.com/sirupsen/logrus" ) @@ -49,7 +50,7 @@ func (s *Service) roundRobinSync(genesis time.Time) error { defer state.SkipSlotCache.Enable() counter := ratecounter.NewRateCounter(counterSeconds * time.Second) - randGenerator := rand.New(rand.NewSource(time.Now().Unix())) + randGenerator := rand.New(rand.NewSource(roughtime.Now().Unix())) var lastEmptyRequests int highestFinalizedSlot := helpers.StartSlot(s.highestFinalizedEpoch() + 1) // Step 1 - Sync to end of finalized epoch. diff --git a/beacon-chain/sync/initial-sync/blocks_fetcher.go b/beacon-chain/sync/initial-sync/blocks_fetcher.go index 03c2afb35..4ca45bf13 100644 --- a/beacon-chain/sync/initial-sync/blocks_fetcher.go +++ b/beacon-chain/sync/initial-sync/blocks_fetcher.go @@ -23,6 +23,7 @@ import ( p2ppb "github.com/prysmaticlabs/prysm/proto/beacon/p2p/v1" "github.com/prysmaticlabs/prysm/shared/mathutil" "github.com/prysmaticlabs/prysm/shared/params" + "github.com/prysmaticlabs/prysm/shared/roughtime" "github.com/sirupsen/logrus" "go.opencensus.io/trace" ) @@ -83,7 +84,7 @@ func newBlocksFetcher(ctx context.Context, cfg *blocksFetcherConfig) *blocksFetc rateLimiter := leakybucket.NewCollector( allowedBlocksPerSecond, /* rate */ allowedBlocksPerSecond, /* capacity */ - false /* deleteEmptyBuckets */) + false /* deleteEmptyBuckets */) return &blocksFetcher{ ctx: ctx, @@ -412,7 +413,7 @@ func selectFailOverPeer(excludedPID peer.ID, peers []peer.ID) (peer.ID, error) { return "", errNoPeersAvailable } - randGenerator := rand.New(rand.NewSource(time.Now().Unix())) + randGenerator := rand.New(rand.NewSource(roughtime.Now().Unix())) randGenerator.Shuffle(len(peers), func(i, j int) { peers[i], peers[j] = peers[j], peers[i] }) @@ -450,7 +451,7 @@ func (f *blocksFetcher) selectPeers(peers []peer.ID) []peer.ID { // Shuffle peers to prevent a bad peer from // stalling sync with invalid blocks. - randGenerator := rand.New(rand.NewSource(time.Now().Unix())) + randGenerator := rand.New(rand.NewSource(roughtime.Now().Unix())) randGenerator.Shuffle(len(peers), func(i, j int) { peers[i], peers[j] = peers[j], peers[i] }) @@ -474,7 +475,7 @@ func (f *blocksFetcher) nonSkippedSlotAfter(ctx context.Context, slot uint64) (u return 0, errNoPeersAvailable } - randGenerator := rand.New(rand.NewSource(time.Now().Unix())) + randGenerator := rand.New(rand.NewSource(roughtime.Now().Unix())) nextPID := func() peer.ID { randGenerator.Shuffle(len(peers), func(i, j int) { peers[i], peers[j] = peers[j], peers[i] diff --git a/beacon-chain/sync/initial-sync/fsm.go b/beacon-chain/sync/initial-sync/fsm.go index d477a26b7..95fd94a33 100644 --- a/beacon-chain/sync/initial-sync/fsm.go +++ b/beacon-chain/sync/initial-sync/fsm.go @@ -6,6 +6,7 @@ import ( "time" eth "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1" + "github.com/prysmaticlabs/prysm/shared/roughtime" ) const ( @@ -119,7 +120,7 @@ func (sm *stateMachine) addEpochState(epoch uint64) { epoch: epoch, state: stateNew, blocks: []*eth.SignedBeaconBlock{}, - updated: time.Now(), + updated: roughtime.Now(), } sm.epochs = append(sm.epochs, state) } @@ -223,7 +224,7 @@ func (es *epochState) setState(name stateID) { if es.state == name { return } - es.updated = time.Now() + es.updated = roughtime.Now() es.state = name } diff --git a/nogo_config.json b/nogo_config.json index dce9f4c44..cc5151b24 100644 --- a/nogo_config.json +++ b/nogo_config.json @@ -59,5 +59,21 @@ "shared/params/config.go": "This config struct needs to be organized for now", "proto/.*": "Excluding protobuf objects for now" } + }, + "roughtime": { + "only_files": { + "beacon-chain/.*": "", + "shared/.*": "", + "validator/.*": "" + }, + "exclude_files": { + ".*/.*_test\\.go": "Tests are OK to use time.Now()", + "shared/version/version\\.go": "Used for printing build time", + "shared/roughtime/roughtime\\.go": "Required to bootstrap roughtime", + "beacon-chain/blockchain/testing/*": "Test-only package", + "beacon-chain/p2p/sender\\.go": "Libp2p uses time.Now and this file sets a time based deadline in such a way that roughtime cannot be used", + "beacon-chain/sync/deadlines\\.go": "Libp2p uses time.Now and this file sets a time based deadline in such a way that roughtime cannot be used", + "validator/client/grpc_interceptor\\.go": "Uses time.Now() for gRPC duration logging" + } } } diff --git a/shared/keystore/BUILD.bazel b/shared/keystore/BUILD.bazel index 4b3fa28f6..45f60ced7 100644 --- a/shared/keystore/BUILD.bazel +++ b/shared/keystore/BUILD.bazel @@ -15,6 +15,7 @@ go_library( "//shared/bls:go_default_library", "//shared/hashutil:go_default_library", "//shared/params:go_default_library", + "//shared/roughtime:go_default_library", "@com_github_minio_sha256_simd//:go_default_library", "@com_github_pborman_uuid//:go_default_library", "@com_github_prysmaticlabs_ethereumapis//eth/v1alpha1:go_default_library", diff --git a/shared/keystore/utils.go b/shared/keystore/utils.go index c25b8656f..8c3103945 100644 --- a/shared/keystore/utils.go +++ b/shared/keystore/utils.go @@ -26,6 +26,7 @@ import ( "time" "github.com/prysmaticlabs/prysm/shared/bls" + "github.com/prysmaticlabs/prysm/shared/roughtime" ) func aesCTRXOR(key, inText, iv []byte) ([]byte, error) { @@ -51,7 +52,7 @@ func ensureInt(x interface{}) int { // keyFileName implements the naming convention for keyfiles: // UTC--- func keyFileName(pubkey *bls.PublicKey) string { - ts := time.Now().UTC() + ts := roughtime.Now().UTC() return fmt.Sprintf("UTC--%s--%s", toISO8601(ts), hex.EncodeToString(pubkey.Marshal())[:8]) } diff --git a/tools/analyzers/roughtime/BUILD.bazel b/tools/analyzers/roughtime/BUILD.bazel new file mode 100644 index 000000000..5c5f67402 --- /dev/null +++ b/tools/analyzers/roughtime/BUILD.bazel @@ -0,0 +1,25 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_tool_library") + +go_library( + name = "go_default_library", + srcs = ["analyzer.go"], + importpath = "github.com/prysmaticlabs/prysm/tools/analyzers/roughtime", + 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 = "github.com/prysmaticlabs/prysm/tools/analyzers/roughtime", + 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/roughtime/analyzer.go b/tools/analyzers/roughtime/analyzer.go new file mode 100644 index 000000000..df30d5bef --- /dev/null +++ b/tools/analyzers/roughtime/analyzer.go @@ -0,0 +1,52 @@ +package roughtime + +import ( + "go/ast" + + "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 = "Tool to enforce the use of roughtime.Now() rather than time.Now(). This is " + + "critically important to certain ETH2 systems where the client / server must be in sync with " + + "some NTP network." + +// Analyzer runs static analysis. +var Analyzer = &analysis.Analyzer{ + Name: "roughtime", + Doc: Doc, + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: run, +} + +func run(pass *analysis.Pass) (interface{}, error) { + inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + + nodeFilter := []ast.Node{ + (*ast.CallExpr)(nil), + } + + inspect.Preorder(nodeFilter, func(node ast.Node) { + if ce, ok := node.(*ast.CallExpr); ok { + if isPkgDot(ce.Fun, "time", "Now") { + pass.Reportf(node.Pos(), "Do not use time.Now(), use "+ + "github.com/prysmaticlabs/prysm/shared/roughtime.Now() or add an exclusion "+ + "to //:nogo.json.") + } + } + }) + + return nil, nil +} + +func isPkgDot(expr ast.Expr, pkg, name string) bool { + sel, ok := expr.(*ast.SelectorExpr) + return ok && isIdent(sel.X, pkg) && isIdent(sel.Sel, name) +} + +func isIdent(expr ast.Expr, ident string) bool { + id, ok := expr.(*ast.Ident) + return ok && id.Name == ident +}