Deprecate span map cache flag (#5551)

* deprecate span map cache flag * fix tests * nishant feedback * fix startup * gaz * nishant feedback * gaz * Merge branch 'master' into deprecate_span_cache * fix img * Update slasher/usage_test.go * Merge refs/heads/master into deprecate_span_cache * Merge refs/heads/master into deprecate_span_cache * Merge refs/heads/master into deprecate_span_cache * Merge refs/heads/master into deprecate_span_cache * Merge refs/heads/master into deprecate_span_cache * Merge refs/heads/master into deprecate_span_cache * Merge refs/heads/master into deprecate_span_cache * Merge refs/heads/master into deprecate_span_cache * Merge refs/heads/master into deprecate_span_cache * Merge refs/heads/master into deprecate_span_cache * Merge refs/heads/master into deprecate_span_cache * fix up tests
prysmaticlabs · Apr 22, 2020 · 9c012cc · 9c012cc
1 parent 9e6b6bc
commit 9c012cc
Show file tree

Hide file tree

Showing 14 changed files with 43 additions and 29 deletions.
diff --git a/shared/featureconfig/config.go b/shared/featureconfig/config.go
@@ -246,6 +246,12 @@ func ConfigureBeaconChain(ctx *cli.Context) {
 	Init(cfg)
 }
 
+// ConfigureSlasher sets the global config based
+// on what flags are enabled for the slasher client.
+func ConfigureSlasher(ctx *cli.Context) {
+	complainOnDeprecatedFlags(ctx)
+}
+
 // ConfigureValidator sets the global config based
 // on what flags are enabled for the validator client.
 func ConfigureValidator(ctx *cli.Context) {

diff --git a/shared/featureconfig/flags.go b/shared/featureconfig/flags.go
@@ -309,6 +309,11 @@ var (
 		Usage:  deprecatedUsage,
 		Hidden: true,
 	}
+	deprecatedUseSpanCacheFlag = &cli.BoolFlag{
+		Name:   "span-map-cache",
+		Usage:  deprecatedUsage,
+		Hidden: true,
+	}
 )
 
 var deprecatedFlags = []cli.Flag{
@@ -341,6 +346,7 @@ var deprecatedFlags = []cli.Flag{
 	deprecatedProtectProposerFlag,
 	deprecatedDiscv5Flag,
 	deprecatedEnableSSZCache,
+	deprecatedUseSpanCacheFlag,
 }
 
 // ValidatorFlags contains a list of all the feature flags that apply to the validator client.
@@ -352,6 +358,9 @@ var ValidatorFlags = append(deprecatedFlags, []cli.Flag{
 	waitForSyncedFlag,
 }...)
 
+// SlasherFlags contains a list of all the feature flags that apply to the slasher client.
+var SlasherFlags = append(deprecatedFlags, []cli.Flag{}...)
+
 // E2EValidatorFlags contains a list of the validator feature flags to be tested in E2E.
 var E2EValidatorFlags = []string{
 	"--enable-domain-data-cache",

diff --git a/slasher/BUILD.bazel b/slasher/BUILD.bazel
@@ -14,6 +14,7 @@ go_library(
     deps = [
         "//shared/cmd:go_default_library",
         "//shared/debug:go_default_library",
+        "//shared/featureconfig:go_default_library",
         "//shared/logutil:go_default_library",
         "//shared/version:go_default_library",
         "//slasher/flags:go_default_library",
@@ -30,7 +31,10 @@ go_test(
     size = "small",
     srcs = ["usage_test.go"],
     embed = [":go_default_library"],
-    deps = ["@in_gopkg_urfave_cli_v2//:go_default_library"],
+    deps = [
+        "//shared/featureconfig:go_default_library",
+        "@in_gopkg_urfave_cli_v2//:go_default_library",
+    ],
 )
 
 go_image(
@@ -50,6 +54,7 @@ go_image(
     deps = [
         "//shared/cmd:go_default_library",
         "//shared/debug:go_default_library",
+        "//shared/featureconfig:go_default_library",
         "//shared/logutil:go_default_library",
         "//shared/version:go_default_library",
         "//slasher/flags:go_default_library",

diff --git a/slasher/db/kv/BUILD.bazel b/slasher/db/kv/BUILD.bazel
@@ -55,7 +55,6 @@ go_test(
         "//slasher/db/iface:go_default_library",
         "//slasher/db/types:go_default_library",
         "//slasher/detection/attestations/types:go_default_library",
-        "//slasher/flags:go_default_library",
         "@com_github_gogo_protobuf//proto:go_default_library",
         "@com_github_prysmaticlabs_ethereumapis//eth/v1alpha1:go_default_library",
         "@in_gopkg_d4l3k_messagediff_v1//:go_default_library",

diff --git a/slasher/db/kv/kv.go b/slasher/db/kv/kv.go
@@ -23,9 +23,8 @@ type Store struct {
 
 // Config options for the slasher db.
 type Config struct {
-	// SpanCacheEnabled uses span cache to detect surround slashing.
-	SpanCacheEnabled bool
-	SpanCacheSize    int
+	// SpanCacheSize determines the span map cache size.
+	SpanCacheSize int
 }
 
 // Close closes the underlying boltdb database.
@@ -85,7 +84,8 @@ func NewKVStore(dirPath string, cfg *Config) (*Store, error) {
 		}
 		return nil, err
 	}
-	kv := &Store{db: boltDB, databasePath: datafile, spanCacheEnabled: cfg.SpanCacheEnabled}
+	kv := &Store{db: boltDB, databasePath: datafile}
+	kv.enableSpanCache(true)
 	spanCache, err := cache.NewEpochSpansCache(cfg.SpanCacheSize, persistSpanMapsOnEviction(kv))
 	if err != nil {
 		return nil, errors.Wrap(err, "could not create new cache")

diff --git a/slasher/db/kv/kv_test.go b/slasher/db/kv/kv_test.go
@@ -10,7 +10,6 @@ import (
 
 	"github.com/prysmaticlabs/prysm/shared/testutil"
 	"github.com/prysmaticlabs/prysm/slasher/db/iface"
-	"github.com/prysmaticlabs/prysm/slasher/flags"
 	"gopkg.in/urfave/cli.v2"
 )
 
@@ -23,7 +22,7 @@ func setupDB(t testing.TB, ctx *cli.Context) *Store {
 	if err := os.RemoveAll(p); err != nil {
 		t.Fatalf("Failed to remove directory: %v", err)
 	}
-	cfg := &Config{SpanCacheEnabled: ctx.Bool(flags.UseSpanCacheFlag.Name)}
+	cfg := &Config{}
 	db, err := NewKVStore(p, cfg)
 	if err != nil {
 		t.Fatalf("Failed to instantiate DB: %v", err)
@@ -40,7 +39,7 @@ func setupDBDiffCacheSize(t testing.TB, cacheSize int) *Store {
 	if err := os.RemoveAll(p); err != nil {
 		t.Fatalf("Failed to remove directory: %v", err)
 	}
-	cfg := &Config{SpanCacheEnabled: true, SpanCacheSize: cacheSize}
+	cfg := &Config{SpanCacheSize: cacheSize}
 	newDB, err := NewKVStore(p, cfg)
 	if err != nil {
 		t.Fatalf("Failed to instantiate DB: %v", err)

diff --git a/slasher/db/kv/spanner_test.go b/slasher/db/kv/spanner_test.go
@@ -8,7 +8,6 @@ import (
 	"time"
 
 	"github.com/prysmaticlabs/prysm/slasher/detection/attestations/types"
-	"github.com/prysmaticlabs/prysm/slasher/flags"
 	"gopkg.in/urfave/cli.v2"
 )
 
@@ -98,7 +97,6 @@ func TestStore_SaveSpans(t *testing.T) {
 func TestStore_SaveCachedSpans(t *testing.T) {
 	app := cli.App{}
 	set := flag.NewFlagSet("test", 0)
-	set.Bool(flags.UseSpanCacheFlag.Name, true, "enable span map cache")
 	db := setupDB(t, cli.NewContext(&app, set, nil))
 	defer teardownDB(t, db)
 	ctx := context.Background()
@@ -134,7 +132,7 @@ func TestStore_DeleteEpochSpans(t *testing.T) {
 	db := setupDB(t, cli.NewContext(&app, set, nil))
 	defer teardownDB(t, db)
 	ctx := context.Background()
-
+	db.spanCacheEnabled = false
 	for _, tt := range spanTests {
 		err := db.SaveEpochSpansMap(ctx, tt.epoch, tt.spanMap)
 		if err != nil {
@@ -167,7 +165,6 @@ func TestStore_DeleteEpochSpans(t *testing.T) {
 func TestValidatorSpanMap_DeletesOnCacheSavesToDB(t *testing.T) {
 	app := cli.App{}
 	set := flag.NewFlagSet("test", 0)
-	set.Bool(flags.UseSpanCacheFlag.Name, true, "enable span map cache")
 	db := setupDB(t, cli.NewContext(&app, set, nil))
 	defer teardownDB(t, db)
 	ctx := context.Background()
@@ -242,7 +239,6 @@ func TestValidatorSpanMap_SaveOnEvict(t *testing.T) {
 func TestValidatorSpanMap_SaveCachedSpansMaps(t *testing.T) {
 	app := cli.App{}
 	set := flag.NewFlagSet("test", 0)
-	set.Bool(flags.UseSpanCacheFlag.Name, true, "enable span map cache")
 	db := setupDB(t, cli.NewContext(&app, set, nil))
 	defer teardownDB(t, db)
 	ctx := context.Background()
@@ -276,6 +272,7 @@ func TestStore_ReadWriteEpochsSpanByValidatorsIndices(t *testing.T) {
 	db := setupDB(t, cli.NewContext(&app, set, nil))
 	defer teardownDB(t, db)
 	ctx := context.Background()
+	db.spanCacheEnabled = false
 
 	for _, tt := range spanTests {
 		err := db.SaveEpochSpansMap(ctx, tt.epoch, tt.spanMap)

diff --git a/slasher/db/testing/setup_db.go b/slasher/db/testing/setup_db.go
@@ -23,7 +23,7 @@ func SetupSlasherDB(t testing.TB, spanCacheEnabled bool) *kv.Store {
 	if err := os.RemoveAll(p); err != nil {
 		t.Fatalf("Failed to remove directory: %v", err)
 	}
-	cfg := &kv.Config{SpanCacheEnabled: spanCacheEnabled}
+	cfg := &kv.Config{}
 	db, err := slasherDB.NewDB(p, cfg)
 	if err != nil {
 		t.Fatalf("Failed to instantiate DB: %v", err)
@@ -41,7 +41,7 @@ func SetupSlasherDBDiffCacheSize(t testing.TB, cacheItems int64, maxCacheSize in
 	if err := os.RemoveAll(p); err != nil {
 		t.Fatalf("Failed to remove directory: %v", err)
 	}
-	cfg := &kv.Config{SpanCacheEnabled: true}
+	cfg := &kv.Config{}
 	newDB, err := slasherDB.NewDB(p, cfg)
 	if err != nil {
 		t.Fatalf("Failed to instantiate DB: %v", err)

diff --git a/slasher/flags/flags.go b/slasher/flags/flags.go
@@ -43,9 +43,4 @@ var (
 		Name:  "rebuild-span-maps",
 		Usage: "Rebuild span maps from indexed attestations in db",
 	}
-	// UseSpanCacheFlag enables the slasher to use span cache.
-	UseSpanCacheFlag = &cli.BoolFlag{
-		Name:  "span-map-cache",
-		Usage: "Enable span map cache",
-	}
 )
diff --git a/slasher/main.go b/slasher/main.go
@@ -8,6 +8,7 @@ import (
 	joonix "github.com/joonix/log"
 	"github.com/prysmaticlabs/prysm/shared/cmd"
 	"github.com/prysmaticlabs/prysm/shared/debug"
+	"github.com/prysmaticlabs/prysm/shared/featureconfig"
 	"github.com/prysmaticlabs/prysm/shared/logutil"
 	"github.com/prysmaticlabs/prysm/shared/version"
 	"github.com/prysmaticlabs/prysm/slasher/flags"
@@ -20,6 +21,7 @@ import (
 var log = logrus.WithField("prefix", "main")
 
 func startSlasher(ctx *cli.Context) error {
+	featureconfig.ConfigureSlasher(ctx)
 	verbosity := ctx.String(cmd.VerbosityFlag.Name)
 	level, err := logrus.ParseLevel(verbosity)
 	if err != nil {
@@ -41,7 +43,6 @@ var appFlags = []cli.Flag{
 	cmd.TracingProcessNameFlag,
 	cmd.TracingEndpointFlag,
 	cmd.TraceSampleFractionFlag,
-	cmd.BootstrapNode,
 	flags.MonitoringPortFlag,
 	cmd.LogFileName,
 	cmd.LogFormat,
@@ -55,12 +56,15 @@ var appFlags = []cli.Flag{
 	debug.TraceFlag,
 	flags.RPCPort,
 	flags.KeyFlag,
-	flags.UseSpanCacheFlag,
 	flags.RebuildSpanMapsFlag,
 	flags.BeaconCertFlag,
 	flags.BeaconRPCProviderFlag,
 }
 
+func init() {
+	appFlags = cmd.WrapFlags(append(appFlags, featureconfig.SlasherFlags...))
+}
+
 func main() {
 	app := cli.App{}
 	app.Name = "hash slinging slasher"

diff --git a/slasher/node/node.go b/slasher/node/node.go
@@ -55,6 +55,7 @@ func NewSlasherNode(ctx *cli.Context) (*SlasherNode, error) {
 	); err != nil {
 		return nil, err
 	}
+
 	registry := shared.NewServiceRegistry()
 
 	slasher := &SlasherNode{
@@ -142,7 +143,7 @@ func (s *SlasherNode) startDB(ctx *cli.Context) error {
 	clearDB := ctx.Bool(cmd.ClearDB.Name)
 	forceClearDB := ctx.Bool(cmd.ForceClearDB.Name)
 	dbPath := path.Join(baseDir, slasherDBName)
-	cfg := &kv.Config{SpanCacheEnabled: ctx.Bool(flags.UseSpanCacheFlag.Name)}
+	cfg := &kv.Config{}
 	d, err := db.NewDB(dbPath, cfg)
 	if err != nil {
 		return err

diff --git a/slasher/usage.go b/slasher/usage.go
@@ -49,7 +49,6 @@ var appHelpFlagGroups = []flagGroup{
 			cmd.TracingProcessNameFlag,
 			cmd.TracingEndpointFlag,
 			cmd.TraceSampleFractionFlag,
-			cmd.BootstrapNode,
 			flags.MonitoringPortFlag,
 			cmd.LogFormat,
 			cmd.LogFileName,
@@ -74,7 +73,6 @@ var appHelpFlagGroups = []flagGroup{
 			flags.BeaconCertFlag,
 			flags.KeyFlag,
 			flags.RPCPort,
-			flags.UseSpanCacheFlag,
 			flags.RebuildSpanMapsFlag,
 			flags.BeaconRPCProviderFlag,
 		},

diff --git a/slasher/usage_test.go b/slasher/usage_test.go
@@ -3,6 +3,7 @@ package main
 import (
 	"testing"
 
+	"github.com/prysmaticlabs/prysm/shared/featureconfig"
 	"gopkg.in/urfave/cli.v2"
 )
 
@@ -15,6 +16,8 @@ func TestAllFlagsExistInHelp(t *testing.T) {
 	for _, group := range appHelpFlagGroups {
 		helpFlags = append(helpFlags, group.Flags...)
 	}
+	helpFlags = featureconfig.ActiveFlags(helpFlags)
+	appFlags = featureconfig.ActiveFlags(appFlags)
 
 	for _, flag := range appFlags {
 		if !doesFlagExist(flag, helpFlags) {
@@ -32,10 +35,9 @@ func TestAllFlagsExistInHelp(t *testing.T) {
 
 func doesFlagExist(flag cli.Flag, flags []cli.Flag) bool {
 	for _, f := range flags {
-		if f == flag {
+		if f.String() == flag.String() {
 			return true
 		}
 	}
-
 	return false
 }
diff --git a/validator/usage_test.go b/validator/usage_test.go
@@ -39,6 +39,5 @@ func doesFlagExist(flag cli.Flag, flags []cli.Flag) bool {
 			return true
 		}
 	}
-
 	return false
 }
-Original file line number
+Diff line change
@@ Expand Up / @@ -39,6 +39,5 @@ func doesFlagExist(flag cli.Flag, flags []cli.Flag) bool { @@
     			return true
     		}
     	}
     	return false
     }