Add static analysis to enforce usage of InitWithReset (#5654)

* Refactor attestation packing slightly to reduce the skip slot / HTR of process slots
* Merge branch 'master' into refactor-attestation-packing
* gofmt
* Merge branch 'refactor-attestation-packing' of github.com:prysmaticlabs/prysm into refactor-attestation-packing
* Merge branch 'master' of github.com:prysmaticlabs/prysm into refactor-attestation-packing
* Add static analysis to enforce usage of InitWithReset
* Add comment / lint
* fix a few usages
* more fixes for featureconfig.Init
* Fix analyzer
* Merge branch 'sa-fc-init' of github.com:prysmaticlabs/prysm into sa-fc-init
* Merge refs/heads/master into sa-fc-init
* Merge refs/heads/master into sa-fc-init

BUILD.bazel

@@ -106,6 +106,7 @@ nogo(
         "//tools/analyzers/maligned:go_tool_library",
         "//tools/analyzers/roughtime:go_tool_library",
         "//tools/analyzers/errcheck:go_tool_library",
+        "//tools/analyzers/featureconfig:go_tool_library",
     ],
 )
 

beacon-chain/db/BUILD.bazel

@@ -13,12 +13,12 @@ go_library(
         "alias.go",
         "http_backup_handler.go",
     ] + select({
-        "//conditions:default": [
-            "db_kafka_wrapped.go",
-        ],
         ":kafka_disabled": [
             "db.go",
         ],
+        "//conditions:default": [
+            "db_kafka_wrapped.go",
+        ],
     }),
     importpath = "github.com/prysmaticlabs/prysm/beacon-chain/db",
     visibility = [

beacon-chain/db/kafka/BUILD.bazel

@@ -7,6 +7,7 @@ go_library(
         "passthrough.go",
     ],
     importpath = "github.com/prysmaticlabs/prysm/beacon-chain/db/kafka",
+    tags = ["manual"],
     visibility = ["//beacon-chain/db:__pkg__"],
     deps = [
         "//beacon-chain/db/filters:go_default_library",

nogo_config.json

@@ -84,5 +84,10 @@
       ".*/.*mock\\.go": "Mocks are OK",
       ".*/testmain\\.go": "Test runner generated code"
     }
+  },
+  "featureconfig": {
+    "only_files": {
+      ".*_test\\.go":  "Only tests"
+    }
   }
 }

shared/featureconfig/config.go

@@ -12,7 +12,8 @@ The process for implementing new features using this package is as follows:
 	cfg := &featureconfig.Flags{
 		VerifyAttestationSigs: true,
 	}
-	featureconfig.Init(cfg)
+	resetCfg := featureconfig.InitWithReset(cfg)
+	defer resetCfg()
 	6. Add the string for the flags that should be running within E2E to E2EValidatorFlags
 	and E2EBeaconChainFlags.
 */

tools/analyzers/featureconfig/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/featureconfig",
+    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 = "featureconfig",
+    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",
+    ],
+)

tools/analyzers/featureconfig/analyzer.go

@@ -0,0 +1,91 @@
+package featureconfig
+
+import (
+	"errors"
+	"go/ast"
+	"go/token"
+	"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 featureconfig.InitWithReset to prevent leaking globals in tests."
+
+// Analyzer runs static analysis.
+var Analyzer = &analysis.Analyzer{
+	Name:     "featureconfig",
+	Doc:      Doc,
+	Requires: []*analysis.Analyzer{inspect.Analyzer},
+	Run:      run,
+}
+
+func run(pass *analysis.Pass) (interface{}, error) {
+	inspect, 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),
+		(*ast.ExprStmt)(nil),
+		(*ast.GoStmt)(nil),
+		(*ast.DeferStmt)(nil),
+		(*ast.AssignStmt)(nil),
+	}
+
+	inspect.Preorder(nodeFilter, func(node ast.Node) {
+		if ce, ok := node.(*ast.CallExpr); ok && isPkgDot(ce.Fun, "featureconfig", "Init") {
+			reportForbiddenUsage(pass, ce.Pos())
+			return
+		}
+		switch stmt := node.(type) {
+		case *ast.ExprStmt:
+			if call, ok := stmt.X.(*ast.CallExpr); ok&& isPkgDot(call.Fun, "featureconfig", "InitWithReset"){
+				reportUnhandledReset(pass, call.Lparen)
+			}
+		case *ast.GoStmt:
+			if isPkgDot(stmt.Call, "featureconfig", "InitWithReset") {
+				reportUnhandledReset(pass, stmt.Call.Lparen)
+			}
+		case *ast.DeferStmt:
+			if isPkgDot(stmt.Call, "featureconfig", "InitWithReset") {
+				reportUnhandledReset(pass, stmt.Call.Lparen)
+			}
+		case *ast.AssignStmt:
+			if ce, ok := stmt.Rhs[0].(*ast.CallExpr); ok && isPkgDot(ce.Fun, "featureconfig", "InitWithReset") {
+				for i := 0; i < len(stmt.Lhs); i++ {
+					if id, ok := stmt.Lhs[i].(*ast.Ident); ok {
+						if id.Name == "_" {
+							reportUnhandledReset(pass, id.NamePos)
+						}
+					}
+				}
+			}
+		default:
+		}
+	})
+
+	return nil, nil
+}
+
+func reportForbiddenUsage(pass *analysis.Pass, pos token.Pos) {
+	pass.Reportf(pos, "Use of featureconfig.Init is forbidden in test code. Please use " +
+		"featureconfig.InitWithReset and call reset in the same test function.")
+}
+
+func reportUnhandledReset(pass *analysis.Pass, pos token.Pos) {
+	pass.Reportf(pos, "Unhandled reset featureconfig not found in test " +
+		"method. Be sure to call the returned reset function from featureconfig.InitWithReset " +
+		"within this test method.")
+}
+
+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
+}

tools/extractor/main.go

@@ -19,13 +19,9 @@ var (
 	state = flag.Uint("state", 0, "Extract state at this slot.")
 )
 
-func init() {
-	fc := featureconfig.Get()
-	fc.WriteSSZStateTransitions = true
-	featureconfig.Init(fc)
-}
-
 func main() {
+	resetCfg := featureconfig.InitWithReset(&featureconfig.Flags{WriteSSZStateTransitions: true})
+	defer resetCfg()
 	flag.Parse()
 	fmt.Println("Starting process...")
 	d, err := db.NewDB(*datadir, cache.NewStateSummaryCache())

validator/client/runner_test.go

@@ -37,7 +37,8 @@ func TestCancelledContext_WaitsForSynced(t *testing.T) {
 	cfg := &featureconfig.Flags{
 		WaitForSynced: true,
 	}
-	featureconfig.Init(cfg)
+	reset := featureconfig.InitWithReset(cfg)
+	defer reset()
 	v := &fakeValidator{}
 	run(cancelledContext(), v)
 	if !v.WaitForSyncedCalled {

validator/client/validator_attest_test.go

@@ -132,7 +132,8 @@ func TestAttestToBlockHead_BlocksDoubleAtt(t *testing.T) {
 	config := &featureconfig.Flags{
 		ProtectAttester: true,
 	}
-	featureconfig.Init(config)
+	reset := featureconfig.InitWithReset(config)
+	defer reset()
 	hook := logTest.NewGlobal()
 	validator, m, finish := setup(t)
 	defer finish()
@@ -173,7 +174,8 @@ func TestAttestToBlockHead_BlocksSurroundAtt(t *testing.T) {
 	config := &featureconfig.Flags{
 		ProtectAttester: true,
 	}
-	featureconfig.Init(config)
+	reset := featureconfig.InitWithReset(config)
+	defer reset()
 	hook := logTest.NewGlobal()
 	validator, m, finish := setup(t)
 	defer finish()
@@ -224,7 +226,8 @@ func TestAttestToBlockHead_BlocksSurroundedAtt(t *testing.T) {
 	config := &featureconfig.Flags{
 		ProtectAttester: true,
 	}
-	featureconfig.Init(config)
+	reset := featureconfig.InitWithReset(config)
+	defer reset()
 	hook := logTest.NewGlobal()
 	validator, m, finish := setup(t)
 	defer finish()

validator/client/validator_propose_test.go

@@ -33,11 +33,11 @@ func setup(t *testing.T) (*validator, *mocks, func()) {
 	}
 
 	validator := &validator{
-		db:                             valDB,
+		db:              valDB,
-		validatorClient:                m.validatorClient,
+		validatorClient: m.validatorClient,
-		keyManager:                     testKeyManager,
+		keyManager:      testKeyManager,
-		graffiti:                       []byte{},
+		graffiti:        []byte{},
-		attLogs:                        make(map[[32]byte]*attSubmitted),
+		attLogs:         make(map[[32]byte]*attSubmitted),
 		aggregatedSlotCommitteeIDCache: aggregatedSlotCommitteeIDCache,
 	}
 
@@ -119,7 +119,8 @@ func TestProposeBlock_BlocksDoubleProposal(t *testing.T) {
 	cfg := &featureconfig.Flags{
 		ProtectProposer: true,
 	}
-	featureconfig.Init(cfg)
+	reset := featureconfig.InitWithReset(cfg)
+	defer reset()
 	hook := logTest.NewGlobal()
 	validator, m, finish := setup(t)
 	defer finish()
@@ -156,7 +157,8 @@ func TestProposeBlock_BlocksDoubleProposal_After54KEpochs(t *testing.T) {
 	cfg := &featureconfig.Flags{
 		ProtectProposer: true,
 	}
-	featureconfig.Init(cfg)
+	reset := featureconfig.InitWithReset(cfg)
+	defer reset()
 	hook := logTest.NewGlobal()
 	validator, m, finish := setup(t)
 	defer finish()
@@ -194,7 +196,8 @@ func TestProposeBlock_AllowsPastProposals(t *testing.T) {
 	cfg := &featureconfig.Flags{
 		ProtectProposer: true,
 	}
-	featureconfig.Init(cfg)
+	reset := featureconfig.InitWithReset(cfg)
+	defer reset()
 	hook := logTest.NewGlobal()
 	validator, m, finish := setup(t)
 	defer finish()
@@ -233,7 +236,8 @@ func TestProposeBlock_AllowsSameEpoch(t *testing.T) {
 	cfg := &featureconfig.Flags{
 		ProtectProposer: true,
 	}
-	featureconfig.Init(cfg)
+	reset := featureconfig.InitWithReset(cfg)
+	defer reset()
 	hook := logTest.NewGlobal()
 	validator, m, finish := setup(t)
 	defer finish()