Validator client - Improve readability - NO FUNCTIONAL CHANGE (#13468)

* Improve `NewServiceRegistry` documentation.

* Improve `README.md`.

* Improve readability of `registerValidatorService`.

* Move `log` in `main.go`.

Since `log` is only used in `main.go`.

* Clean Tos.

* `DefaultDataDir`: Use `switch` instead of `if/elif`.

* `ReadPassword`: Remove unused receiver.

* `validator/main.go`: Clean.

* `WarnIfPlatformNotSupported`: Add Mac OSX ARM64.

* `runner.go`: Use idiomatic `err` handling.

* `waitForChainStart`: Avoid `chainStartResponse`mutation.

* `WaitForChainStart`: Reduce cognitive complexity.

* Logs: `powchain` ==> `execution`.

beacon-chain/execution/log.go

@@ -2,4 +2,4 @@ package execution
 
 import "github.com/sirupsen/logrus"
 
-var log = logrus.WithField("prefix", "powchain")
+var log = logrus.WithField("prefix", "execution")

cmd/defaults.go

@@ -31,11 +31,12 @@ func DefaultDataDir() string {
 	// Try to place the data folder in the user's home dir
 	home := file.HomeDir()
 	if home != "" {
-		if runtime.GOOS == "darwin" {
+		switch runtime.GOOS {
+		case "darwin":
 			return filepath.Join(home, "Library", "Eth2")
-		} else if runtime.GOOS == "windows" {
+		case "windows":
 			return filepath.Join(home, "AppData", "Local", "Eth2")
-		} else {
+		default:
 			return filepath.Join(home, ".eth2")
 		}
 	}

cmd/password_reader.go

@@ -16,7 +16,7 @@ type StdInPasswordReader struct {
 }
 
 // ReadPassword reads a password from stdin.
-func (_ StdInPasswordReader) ReadPassword() (string, error) {
+func (StdInPasswordReader) ReadPassword() (string, error) {
 	pwd, err := terminal.ReadPassword(int(os.Stdin.Fd()))
 	return string(pwd), err
 }

cmd/validator/BUILD.bazel

@@ -5,7 +5,6 @@ load("//tools:prysm_image.bzl", "prysm_image_upload")
 go_library(
     name = "go_default_library",
     srcs = [
-        "log.go",
         "main.go",
         "usage.go",
     ],
@@ -30,6 +29,7 @@ go_library(
         "//runtime/version:go_default_library",
         "//validator/node:go_default_library",
         "@com_github_joonix_log//:go_default_library",
+        "@com_github_pkg_errors//:go_default_library",
         "@com_github_sirupsen_logrus//:go_default_library",
         "@com_github_urfave_cli_v2//:go_default_library",
     ],

cmd/validator/log.go

@@ -1,5 +0,0 @@
-package main
-
-import "github.com/sirupsen/logrus"
-
-var log = logrus.WithField("prefix", "main")

cmd/validator/main.go

@@ -10,6 +10,7 @@ import (
 	runtimeDebug "runtime/debug"
 
 	joonix "github.com/joonix/log"
+	"github.com/pkg/errors"
 	"github.com/prysmaticlabs/prysm/v4/cmd"
 	accountcommands "github.com/prysmaticlabs/prysm/v4/cmd/validator/accounts"
 	dbcommands "github.com/prysmaticlabs/prysm/v4/cmd/validator/db"
@@ -31,8 +32,10 @@ import (
 	"github.com/urfave/cli/v2"
 )
 
+var log = logrus.WithField("prefix", "main")
+
 func startNode(ctx *cli.Context) error {
-	// verify if ToS accepted
+	// Verify if ToS is accepted.
 	if err := tos.VerifyTosAcceptedOrPrompt(ctx); err != nil {
 		return err
 	}
@@ -141,6 +144,8 @@ func main() {
 				return err
 			}
 
+			logFileName := ctx.String(cmd.LogFileName.Name)
+
 			format := ctx.String(cmd.LogFormat.Name)
 			switch format {
 			case "text":
@@ -148,8 +153,8 @@ func main() {
 				formatter.TimestampFormat = "2006-01-02 15:04:05"
 				formatter.FullTimestamp = true
 				// If persistent log files are written - we disable the log messages coloring because
-				// the colors are ANSI codes and seen as Gibberish in the log files.
-				formatter.DisableColors = ctx.String(cmd.LogFileName.Name) != ""
+				// the colors are ANSI codes and seen as gibberish in the log files.
+				formatter.DisableColors = logFileName != ""
 				logrus.SetFormatter(formatter)
 			case "fluentd":
 				f := joonix.NewFormatter()
@@ -167,7 +172,6 @@ func main() {
 				return fmt.Errorf("unknown log format %s", format)
 			}
 
-			logFileName := ctx.String(cmd.LogFileName.Name)
 			if logFileName != "" {
 				if err := logs.ConfigurePersistentLogging(logFileName); err != nil {
 					log.WithError(err).Error("Failed to configuring logging to disk.")
@@ -182,8 +186,9 @@ func main() {
 			}
 
 			if err := debug.Setup(ctx); err != nil {
-				return err
+				return errors.Wrap(err, "failed to setup debug")
 			}
+
 			return cmd.ValidateNoArgs(ctx)
 		},
 		After: func(ctx *cli.Context) error {

config/features/README.md

@@ -29,7 +29,7 @@ Examples of when not to use a feature flag:
 Once it has been decided that you should use a feature flag. Follow these steps to safely
 releasing your feature. In general, try to create a single PR for each step of this process.
 
-1. Add your feature flag to shared/featureconfig/flags.go, use the flag to toggle a boolean in the
+1. Add your feature flag to `shared/featureconfig/flags.go`, use the flag to toggle a boolean in the
 feature config in shared/featureconfig/config.go. It is a good idea to use the `enable` prefix for
 your flag since you're going to invert the flag in a later step. i.e you will use `disable` prefix
 later. For example, `--enable-my-feature`. Additionally, [create a feature flag tracking issue](https://github.com/prysmaticlabs/prysm/issues/new?template=feature_flag.md) 
@@ -48,7 +48,7 @@ func someExistingMethod(ctx context.Context) error {
 3. Add the flag to the end to end tests. This set of flags can also be found in shared/featureconfig/flags.go. 
 4. Test the functionality locally and safely in production. Once you have enough confidence that
 your new function works and is safe to release then move onto the next step.
-5. Move your existing flag to the deprecated section of shared/featureconfig/flags.go. It is
+5. Move your existing flag to the deprecated section of `shared/featureconfig/flags.go`. It is
 important NOT to delete your existing flag outright. Deleting a flag can be extremely frustrating
 to users as it may break their existing workflow! Marking a flag as deprecated gives users time to
 adjust their start scripts and workflow. Add another feature flag to represent the inverse of your

runtime/prereqs/prereq.go

@@ -103,7 +103,12 @@ func WarnIfPlatformNotSupported(ctx context.Context) {
 		return
 	}
 	if !supported {
-		log.Warn("This platform is not supported. The following platforms are supported: Linux/AMD64," +
-			" Linux/ARM64, Mac OS X/AMD64 (10.14+ only), and Windows/AMD64")
+		log.Warn(`This platform is not supported. The following platforms are supported:
+		- Linux/AMD64
+		- Linux/ARM64
+		- Mac OS X/AMD64 (from 10.14+)
+		- Mac OS X/ARM64 (from 12.5+)
+		- Windows/AMD64`,
+		)
 	}
 }

runtime/service_registry.go

@@ -31,7 +31,7 @@ type ServiceRegistry struct {
 	serviceTypes []reflect.Type           // keep an ordered slice of registered service types.
 }
 
-// NewServiceRegistry starts a registry instance for convenience
+// NewServiceRegistry starts a registry instance for convenience.
 func NewServiceRegistry() *ServiceRegistry {
 	return &ServiceRegistry{
 		services: make(map[reflect.Type]Service),

runtime/tos/tos.go

@@ -35,11 +35,13 @@ var (
 	log = logrus.WithField("prefix", "tos")
 )
 
-// VerifyTosAcceptedOrPrompt check if Tos was accepted before or asks to accept.
+// VerifyTosAcceptedOrPrompt checks if Tos was accepted before or asks to accept.
 func VerifyTosAcceptedOrPrompt(ctx *cli.Context) error {
-	if file.Exists(filepath.Join(ctx.String(cmd.DataDirFlag.Name), acceptTosFilename)) {
+	tosFilePath := filepath.Join(ctx.String(cmd.DataDirFlag.Name), acceptTosFilename)
+	if file.Exists(tosFilePath) {
 		return nil
 	}
+
 	if ctx.Bool(cmd.AcceptTosFlag.Name) {
 		saveTosAccepted(ctx)
 		return nil
@@ -49,6 +51,7 @@ func VerifyTosAcceptedOrPrompt(ctx *cli.Context) error {
 	if err != nil {
 		return errors.New(acceptTosPromptErrText)
 	}
+
 	if !strings.EqualFold(input, "accept") {
 		return errors.New("you have to accept Terms and Conditions in order to continue")
 	}

validator/client/BUILD.bazel

@@ -32,6 +32,7 @@ go_library(
         "//beacon-chain/core/altair:go_default_library",
         "//beacon-chain/core/signing:go_default_library",
         "//cache/lru:go_default_library",
+        "//cmd:go_default_library",
         "//config/features:go_default_library",
         "//config/fieldparams:go_default_library",
         "//config/params:go_default_library",

validator/client/beacon-api/genesis.go

@@ -45,10 +45,6 @@ func (c beaconApiValidatorClient) waitForChainStart(ctx context.Context) (*ethpb
 		return nil, errors.Wrapf(err, "failed to parse genesis time: %s", genesis.GenesisTime)
 	}
 
-	chainStartResponse := &ethpb.ChainStartResponse{}
-	chainStartResponse.Started = true
-	chainStartResponse.GenesisTime = genesisTime
-
 	if !validRoot(genesis.GenesisValidatorsRoot) {
 		return nil, errors.Errorf("invalid genesis validators root: %s", genesis.GenesisValidatorsRoot)
 	}
@@ -57,7 +53,12 @@ func (c beaconApiValidatorClient) waitForChainStart(ctx context.Context) (*ethpb
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to decode genesis validators root")
 	}
-	chainStartResponse.GenesisValidatorsRoot = genesisValidatorRoot
+
+	chainStartResponse := &ethpb.ChainStartResponse{
+		Started:               true,
+		GenesisTime:           genesisTime,
+		GenesisValidatorsRoot: genesisValidatorRoot,
+	}
 
 	return chainStartResponse, nil
 }

validator/client/runner.go

@@ -18,7 +18,7 @@ import (
 	"google.golang.org/grpc/status"
 )
 
-// time to wait before trying to reconnect with beacon node.
+// Time to wait before trying to reconnect with beacon node.
 var backOffPeriod = 10 * time.Second
 
 // Run the main validator routine. This routine exits if the context is
@@ -149,8 +149,13 @@ func initializeValidatorAndGetHeadSlot(ctx context.Context, v iface.Validator) (
 	ticker := time.NewTicker(backOffPeriod)
 	defer ticker.Stop()
 
-	var headSlot primitives.Slot
 	firstTime := true
+
+	var (
+		headSlot primitives.Slot
+		err      error
+	)
+
 	for {
 		if !firstTime {
 			if ctx.Err() != nil {
@@ -158,35 +163,35 @@ func initializeValidatorAndGetHeadSlot(ctx context.Context, v iface.Validator) (
 				return headSlot, errors.New("context canceled")
 			}
 			<-ticker.C
-		} else {
-			firstTime = false
 		}
-		err := v.WaitForChainStart(ctx)
-		if isConnectionError(err) {
-			log.WithError(err).Warn("Could not determine if beacon chain started")
-			continue
-		}
-		if err != nil {
+
+		firstTime = false
+
+		if err := v.WaitForChainStart(ctx); err != nil {
+			if isConnectionError(err) {
+				log.WithError(err).Warn("Could not determine if beacon chain started")
+				continue
+			}
+
 			log.WithError(err).Fatal("Could not determine if beacon chain started")
 		}
 
-		err = v.WaitForKeymanagerInitialization(ctx)
-		if err != nil {
+		if err := v.WaitForKeymanagerInitialization(ctx); err != nil {
 			// log.Fatal will prevent defer from being called
 			v.Done()
 			log.WithError(err).Fatal("Wallet is not ready")
 		}
 
-		err = v.WaitForSync(ctx)
-		if isConnectionError(err) {
-			log.WithError(err).Warn("Could not determine if beacon chain started")
-			continue
-		}
-		if err != nil {
+		if err := v.WaitForSync(ctx); err != nil {
+			if isConnectionError(err) {
+				log.WithError(err).Warn("Could not determine if beacon chain started")
+				continue
+			}
+
 			log.WithError(err).Fatal("Could not determine if beacon node synced")
 		}
-		err = v.WaitForActivation(ctx, nil /* accountsChangedChan */)
-		if err != nil {
+
+		if err := v.WaitForActivation(ctx, nil /* accountsChangedChan */); err != nil {
 			log.WithError(err).Fatal("Could not wait for validator activation")
 		}
 
@@ -195,15 +200,17 @@ func initializeValidatorAndGetHeadSlot(ctx context.Context, v iface.Validator) (
 			log.WithError(err).Warn("Could not get current canonical head slot")
 			continue
 		}
+
 		if err != nil {
 			log.WithError(err).Fatal("Could not get current canonical head slot")
 		}
-		err = v.CheckDoppelGanger(ctx)
-		if isConnectionError(err) {
-			log.WithError(err).Warn("Could not wait for checking doppelganger")
-			continue
-		}
-		if err != nil {
+
+		if err := v.CheckDoppelGanger(ctx); err != nil {
+			if isConnectionError(err) {
+				log.WithError(err).Warn("Could not wait for checking doppelganger")
+				continue
+			}
+
 			log.WithError(err).Fatal("Could not succeed with doppelganger check")
 		}
 		break

validator/client/validator.go

@@ -22,6 +22,7 @@ import (
 	"github.com/pkg/errors"
 	"github.com/prysmaticlabs/prysm/v4/async/event"
 	"github.com/prysmaticlabs/prysm/v4/beacon-chain/core/altair"
+	"github.com/prysmaticlabs/prysm/v4/cmd"
 	"github.com/prysmaticlabs/prysm/v4/config/features"
 	fieldparams "github.com/prysmaticlabs/prysm/v4/config/fieldparams"
 	"github.com/prysmaticlabs/prysm/v4/config/params"
@@ -238,50 +239,65 @@ func recheckValidatingKeysBucket(ctx context.Context, valDB vdb.Database, km key
 func (v *validator) WaitForChainStart(ctx context.Context) error {
 	ctx, span := trace.StartSpan(ctx, "validator.WaitForChainStart")
 	defer span.End()
+
 	// First, check if the beacon chain has started.
 	log.Info("Syncing with beacon node to align on chain genesis info")
+
 	chainStartRes, err := v.validatorClient.WaitForChainStart(ctx, &emptypb.Empty{})
-	if err != io.EOF {
-		if ctx.Err() == context.Canceled {
-			return errors.Wrap(ctx.Err(), "context has been canceled so shutting down the loop")
-		}
-		if err != nil {
-			return errors.Wrap(
-				iface.ErrConnectionIssue,
-				errors.Wrap(err, "could not receive ChainStart from stream").Error(),
-			)
-		}
-		v.genesisTime = chainStartRes.GenesisTime
-		curGenValRoot, err := v.db.GenesisValidatorsRoot(ctx)
-		if err != nil {
-			return errors.Wrap(err, "could not get current genesis validators root")
-		}
-		if len(curGenValRoot) == 0 {
-			if err := v.db.SaveGenesisValidatorsRoot(ctx, chainStartRes.GenesisValidatorsRoot); err != nil {
-				return errors.Wrap(err, "could not save genesis validators root")
-			}
-		} else {
-			if !bytes.Equal(curGenValRoot, chainStartRes.GenesisValidatorsRoot) {
-				log.Errorf("The genesis validators root received from the beacon node does not match what is in " +
-					"your validator database. This could indicate that this is a database meant for another network. If " +
-					"you were previously running this validator database on another network, please run --clear-db to " +
-					"clear the database. If not, please file an issue at https://github.com/prysmaticlabs/prysm/issues")
-				return fmt.Errorf(
-					"genesis validators root from beacon node (%#x) does not match root saved in validator db (%#x)",
-					chainStartRes.GenesisValidatorsRoot,
-					curGenValRoot,
-				)
-			}
-		}
-	} else {
+	if err == io.EOF {
 		return iface.ErrConnectionIssue
 	}
 
+	if ctx.Err() == context.Canceled {
+		return errors.Wrap(ctx.Err(), "context has been canceled so shutting down the loop")
+	}
+
+	if err != nil {
+		return errors.Wrap(
+			iface.ErrConnectionIssue,
+			errors.Wrap(err, "could not receive ChainStart from stream").Error(),
+		)
+	}
+
+	v.genesisTime = chainStartRes.GenesisTime
+
+	curGenValRoot, err := v.db.GenesisValidatorsRoot(ctx)
+	if err != nil {
+		return errors.Wrap(err, "could not get current genesis validators root")
+	}
+
+	if len(curGenValRoot) == 0 {
+		if err := v.db.SaveGenesisValidatorsRoot(ctx, chainStartRes.GenesisValidatorsRoot); err != nil {
+			return errors.Wrap(err, "could not save genesis validators root")
+		}
+
+		v.setTicker()
+		return nil
+	}
+
+	if !bytes.Equal(curGenValRoot, chainStartRes.GenesisValidatorsRoot) {
+		log.Errorf(`The genesis validators root received from the beacon node does not match what is in
+			your validator database. This could indicate that this is a database meant for another network. If
+			you were previously running this validator database on another network, please run --%s to
+			clear the database. If not, please file an issue at https://github.com/prysmaticlabs/prysm/issues`,
+			cmd.ClearDB.Name,
+		)
+		return fmt.Errorf(
+			"genesis validators root from beacon node (%#x) does not match root saved in validator db (%#x)",
+			chainStartRes.GenesisValidatorsRoot,
+			curGenValRoot,
+		)
+	}
+
+	v.setTicker()
+	return nil
+}
+
+func (v *validator) setTicker() {
 	// Once the ChainStart log is received, we update the genesis time of the validator client
 	// and begin a slot ticker used to track the current slot the beacon node is in.
 	v.ticker = slots.NewSlotTicker(time.Unix(int64(v.genesisTime), 0), params.BeaconConfig().SecondsPerSlot)
 	log.WithField("genesisTime", time.Unix(int64(v.genesisTime), 0)).Info("Beacon chain started")
-	return nil
 }
 
 // WaitForSync checks whether the beacon node has sync to the latest head.

validator/node/node.go

@@ -423,16 +423,22 @@ func (c *ValidatorClient) registerPrometheusService(cliCtx *cli.Context) error {
 }
 
 func (c *ValidatorClient) registerValidatorService(cliCtx *cli.Context) error {
-	endpoint := c.cliCtx.String(flags.BeaconRPCProviderFlag.Name)
-	dataDir := c.cliCtx.String(cmd.DataDirFlag.Name)
-	logValidatorBalances := !c.cliCtx.Bool(flags.DisablePenaltyRewardLogFlag.Name)
-	emitAccountMetrics := !c.cliCtx.Bool(flags.DisableAccountMetricsFlag.Name)
-	cert := c.cliCtx.String(flags.CertFlag.Name)
-	graffiti := c.cliCtx.String(flags.GraffitiFlag.Name)
-	maxCallRecvMsgSize := c.cliCtx.Int(cmd.GrpcMaxCallRecvMsgSizeFlag.Name)
-	grpcRetries := c.cliCtx.Uint(flags.GrpcRetriesFlag.Name)
-	grpcRetryDelay := c.cliCtx.Duration(flags.GrpcRetryDelayFlag.Name)
-	var interopKeysConfig *local.InteropKeymanagerConfig
+	var (
+		endpoint             string        = c.cliCtx.String(flags.BeaconRPCProviderFlag.Name)
+		dataDir              string        = c.cliCtx.String(cmd.DataDirFlag.Name)
+		logValidatorBalances bool          = !c.cliCtx.Bool(flags.DisablePenaltyRewardLogFlag.Name)
+		emitAccountMetrics   bool          = !c.cliCtx.Bool(flags.DisableAccountMetricsFlag.Name)
+		cert                 string        = c.cliCtx.String(flags.CertFlag.Name)
+		graffiti             string        = c.cliCtx.String(flags.GraffitiFlag.Name)
+		maxCallRecvMsgSize   int           = c.cliCtx.Int(cmd.GrpcMaxCallRecvMsgSizeFlag.Name)
+		grpcRetries          uint          = c.cliCtx.Uint(flags.GrpcRetriesFlag.Name)
+		grpcRetryDelay       time.Duration = c.cliCtx.Duration(flags.GrpcRetryDelayFlag.Name)
+
+		interopKeysConfig *local.InteropKeymanagerConfig
+		err               error
+	)
+
+	// Configure interop.
 	if c.cliCtx.IsSet(flags.InteropNumValidators.Name) {
 		interopKeysConfig = &local.InteropKeymanagerConfig{
 			Offset:           cliCtx.Uint64(flags.InteropStartIndex.Name),
@@ -440,27 +446,28 @@ func (c *ValidatorClient) registerValidatorService(cliCtx *cli.Context) error {
 		}
 	}
 
-	gStruct := &g.Graffiti{}
-	var err error
+	// Configure graffiti.
+	graffitiStruct := &g.Graffiti{}
 	if c.cliCtx.IsSet(flags.GraffitiFileFlag.Name) {
-		n := c.cliCtx.String(flags.GraffitiFileFlag.Name)
-		gStruct, err = g.ParseGraffitiFile(n)
+		graffitiFilePath := c.cliCtx.String(flags.GraffitiFileFlag.Name)
+
+		graffitiStruct, err = g.ParseGraffitiFile(graffitiFilePath)
 		if err != nil {
 			log.WithError(err).Warn("Could not parse graffiti file")
 		}
 	}
 
-	wsc, err := Web3SignerConfig(c.cliCtx)
+	web3signerConfig, err := Web3SignerConfig(c.cliCtx)
 	if err != nil {
 		return err
 	}
 
-	bpc, err := proposerSettings(c.cliCtx, c.db)
+	proposerSettings, err := proposerSettings(c.cliCtx, c.db)
 	if err != nil {
 		return err
 	}
 
-	v, err := client.NewValidatorService(c.cliCtx.Context, &client.Config{
+	validatorService, err := client.NewValidatorService(c.cliCtx.Context, &client.Config{
 		Endpoint:                   endpoint,
 		DataDir:                    dataDir,
 		LogValidatorBalances:       logValidatorBalances,
@@ -476,9 +483,9 @@ func (c *ValidatorClient) registerValidatorService(cliCtx *cli.Context) error {
 		InteropKeysConfig:          interopKeysConfig,
 		Wallet:                     c.wallet,
 		WalletInitializedFeed:      c.walletInitialized,
-		GraffitiStruct:             gStruct,
-		Web3SignerConfig:           wsc,
-		ProposerSettings:           bpc,
+		GraffitiStruct:             graffitiStruct,
+		Web3SignerConfig:           web3signerConfig,
+		ProposerSettings:           proposerSettings,
 		BeaconApiTimeout:           time.Second * 30,
 		BeaconApiEndpoint:          c.cliCtx.String(flags.BeaconRESTApiProviderFlag.Name),
 		ValidatorsRegBatchSize:     c.cliCtx.Int(flags.ValidatorsRegistrationBatchSizeFlag.Name),
@@ -487,7 +494,7 @@ func (c *ValidatorClient) registerValidatorService(cliCtx *cli.Context) error {
 		return errors.Wrap(err, "could not initialize validator service")
 	}
 
-	return c.services.RegisterService(v)
+	return c.services.RegisterService(validatorService)
 }
 
 func Web3SignerConfig(cliCtx *cli.Context) (*remoteweb3signer.SetupConfig, error) {