From d593d6ba8accf0eca818d53e997bcafd6aa0b3c1 Mon Sep 17 00:00:00 2001 From: Dev Ojha Date: Mon, 15 May 2023 13:04:09 +0200 Subject: [PATCH 01/12] Driveby PR, to split up the server functions a bit --- server/start.go | 143 +++++++++++++++++++++++++++--------------------- 1 file changed, 81 insertions(+), 62 deletions(-) diff --git a/server/start.go b/server/start.go index 882ec02b105c..a22f6c31b217 100644 --- a/server/start.go +++ b/server/start.go @@ -3,6 +3,7 @@ package server import ( "context" "fmt" + "io" "net" "os" "runtime/pprof" @@ -267,24 +268,10 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types. return err } - traceWriterFile := svrCtx.Viper.GetString(flagTraceStore) - traceWriter, err := openTraceWriter(traceWriterFile) + traceWriter, traceWriterCleanup, err := setupTraceWriter(svrCtx) if err != nil { return err } - - // clean up the traceWriter when the server is shutting down - var traceWriterCleanup func() - - // if flagTraceStore is not used then traceWriter is nil - if traceWriter != nil { - traceWriterCleanup = func() { - if err = traceWriter.Close(); err != nil { - svrCtx.Logger.Error("failed to close trace writer", "err", err) - } - } - } - config, err := serverconfig.GetConfig(svrCtx.Viper) if err != nil { return err @@ -361,8 +348,7 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types. emitServerInfoMetrics() var ( - apiSrv *api.Server - grpcSrv *grpc.Server + apiSrv *api.Server ) ctx, cancelFn := context.WithCancel(context.Background()) @@ -371,51 +357,9 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types. // listen for quit signals so the calling parent process can gracefully exit ListenForQuitSignals(cancelFn, svrCtx.Logger) - if config.GRPC.Enable { - _, port, err := net.SplitHostPort(config.GRPC.Address) - if err != nil { - return err - } - - maxSendMsgSize := config.GRPC.MaxSendMsgSize - if maxSendMsgSize == 0 { - maxSendMsgSize = serverconfig.DefaultGRPCMaxSendMsgSize - } - - maxRecvMsgSize := config.GRPC.MaxRecvMsgSize - if maxRecvMsgSize == 0 { - maxRecvMsgSize = serverconfig.DefaultGRPCMaxRecvMsgSize - } - - grpcAddress := fmt.Sprintf("127.0.0.1:%s", port) - - // if gRPC is enabled, configure gRPC client for gRPC gateway - grpcClient, err := grpc.Dial( - grpcAddress, - grpc.WithTransportCredentials(insecure.NewCredentials()), - grpc.WithDefaultCallOptions( - grpc.ForceCodec(codec.NewProtoCodec(clientCtx.InterfaceRegistry).GRPCCodec()), - grpc.MaxCallRecvMsgSize(maxRecvMsgSize), - grpc.MaxCallSendMsgSize(maxSendMsgSize), - ), - ) - if err != nil { - return err - } - - clientCtx = clientCtx.WithGRPCClient(grpcClient) - svrCtx.Logger.Debug("gRPC client assigned to client context", "target", grpcAddress) - - grpcSrv, err = servergrpc.NewGRPCServer(clientCtx, app, config.GRPC) - if err != nil { - return err - } - - // Start the gRPC server in a goroutine. Note, the provided ctx will ensure - // that the server is gracefully shut down. - g.Go(func() error { - return servergrpc.StartGRPCServer(ctx, svrCtx.Logger.With("module", "grpc-server"), config.GRPC, grpcSrv) - }) + grpcSrv, clientCtx, err := startGrpcServer(g, config.GRPC, ctx, clientCtx, svrCtx, app) + if err != nil { + return err } if config.API.Enable { @@ -468,6 +412,81 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types. return g.Wait() } +func setupTraceWriter(svrCtx *Context) (traceWriter io.WriteCloser, cleanup func(), err error) { + traceWriterFile := svrCtx.Viper.GetString(flagTraceStore) + traceWriter, err = openTraceWriter(traceWriterFile) + if err != nil { + return traceWriter, cleanup, err + } + + // clean up the traceWriter when the server is shutting down + cleanup = func() {} + + // if flagTraceStore is not used then traceWriter is nil + if traceWriter != nil { + cleanup = func() { + if err = traceWriter.Close(); err != nil { + svrCtx.Logger.Error("failed to close trace writer", "err", err) + } + } + } + + return traceWriter, cleanup, nil +} + +func startGrpcServer(g *errgroup.Group, config serverconfig.GRPCConfig, ctx context.Context, clientCtx client.Context, svrCtx *Context, app types.Application) ( + *grpc.Server, client.Context, error) { + if !config.Enable { + // return grpcServer as nil if gRPC is disabled + return nil, clientCtx, nil + } + _, port, err := net.SplitHostPort(config.Address) + if err != nil { + return nil, clientCtx, err + } + + maxSendMsgSize := config.MaxSendMsgSize + if maxSendMsgSize == 0 { + maxSendMsgSize = serverconfig.DefaultGRPCMaxSendMsgSize + } + + maxRecvMsgSize := config.MaxRecvMsgSize + if maxRecvMsgSize == 0 { + maxRecvMsgSize = serverconfig.DefaultGRPCMaxRecvMsgSize + } + + grpcAddress := fmt.Sprintf("127.0.0.1:%s", port) + + // if gRPC is enabled, configure gRPC client for gRPC gateway + grpcClient, err := grpc.Dial( + grpcAddress, + grpc.WithTransportCredentials(insecure.NewCredentials()), + grpc.WithDefaultCallOptions( + grpc.ForceCodec(codec.NewProtoCodec(clientCtx.InterfaceRegistry).GRPCCodec()), + grpc.MaxCallRecvMsgSize(maxRecvMsgSize), + grpc.MaxCallSendMsgSize(maxSendMsgSize), + ), + ) + if err != nil { + return nil, clientCtx, err + } + + clientCtx = clientCtx.WithGRPCClient(grpcClient) + svrCtx.Logger.Debug("gRPC client assigned to client context", "target", grpcAddress) + + grpcSrv, err := servergrpc.NewGRPCServer(clientCtx, app, config) + if err != nil { + return nil, clientCtx, err + } + + // Start the gRPC server in a goroutine. Note, the provided ctx will ensure + // that the server is gracefully shut down. + g.Go(func() error { + return servergrpc.StartGRPCServer(ctx, svrCtx.Logger.With("module", "grpc-server"), config, grpcSrv) + }) + return grpcSrv, clientCtx, nil +} + func startTelemetry(cfg serverconfig.Config) (*telemetry.Metrics, error) { if !cfg.Telemetry.Enabled { return nil, nil From 187d112c696e189136499d388e2b9c691d50532c Mon Sep 17 00:00:00 2001 From: Dev Ojha Date: Mon, 15 May 2023 13:13:25 +0200 Subject: [PATCH 02/12] More cleanup --- server/start.go | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/server/start.go b/server/start.go index a22f6c31b217..c445f203d2b1 100644 --- a/server/start.go +++ b/server/start.go @@ -211,6 +211,8 @@ func startStandAlone(svrCtx *Context, appCreator types.AppCreator) error { return err } + // TODO: Should we be using startTraceServer, and defer closing the traceWriter? + // right now its left unclosed traceWriterFile := svrCtx.Viper.GetString(flagTraceStore) traceWriter, err := openTraceWriter(traceWriterFile) if err != nil { @@ -219,6 +221,7 @@ func startStandAlone(svrCtx *Context, appCreator types.AppCreator) error { app := appCreator(svrCtx.Logger, db, traceWriter, svrCtx.Viper) + // TODO: should config be getting validated here? config, err := serverconfig.GetConfig(svrCtx.Viper) if err != nil { return err @@ -272,15 +275,12 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types. if err != nil { return err } - config, err := serverconfig.GetConfig(svrCtx.Viper) + // TODO: Should this be moved to the very top of the function? + config, err := getAndValidateConfig(svrCtx) if err != nil { return err } - if err := config.ValidateBasic(); err != nil { - return err - } - app := appCreator(svrCtx.Logger, db, traceWriter, svrCtx.Viper) nodeKey, err := p2p.LoadOrGenNodeKey(cfg.NodeKeyFile()) @@ -347,9 +347,7 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types. emitServerInfoMetrics() - var ( - apiSrv *api.Server - ) + var apiSrv *api.Server ctx, cancelFn := context.WithCancel(context.Background()) g, ctx := errgroup.WithContext(ctx) @@ -412,6 +410,18 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types. return g.Wait() } +func getAndValidateConfig(svrCtx *Context) (serverconfig.Config, error) { + config, err := serverconfig.GetConfig(svrCtx.Viper) + if err != nil { + return config, err + } + + if err := config.ValidateBasic(); err != nil { + return config, err + } + return config, nil +} + func setupTraceWriter(svrCtx *Context) (traceWriter io.WriteCloser, cleanup func(), err error) { traceWriterFile := svrCtx.Viper.GetString(flagTraceStore) traceWriter, err = openTraceWriter(traceWriterFile) From 74cb1f7f51e90daa34d99efad1d73e7c4df4df0c Mon Sep 17 00:00:00 2001 From: Dev Ojha Date: Mon, 15 May 2023 15:09:34 +0200 Subject: [PATCH 03/12] Apply Julian's api server suggestion! --- server/start.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server/start.go b/server/start.go index c445f203d2b1..ef370ace52f9 100644 --- a/server/start.go +++ b/server/start.go @@ -347,8 +347,6 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types. emitServerInfoMetrics() - var apiSrv *api.Server - ctx, cancelFn := context.WithCancel(context.Background()) g, ctx := errgroup.WithContext(ctx) @@ -361,6 +359,8 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types. } if config.API.Enable { + // TODO: wtf, why do we reload and unmarshal the entire genesis doc in order to get the chain ID. + // surely theres a better way. genDoc, err := genDocProvider() if err != nil { return err @@ -368,7 +368,7 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types. clientCtx := clientCtx.WithHomeDir(home).WithChainID(genDoc.ChainID) - apiSrv = api.New(clientCtx, svrCtx.Logger.With("module", "api-server"), grpcSrv) + apiSrv := api.New(clientCtx, svrCtx.Logger.With("module", "api-server"), grpcSrv) app.RegisterAPIRoutes(apiSrv, config.API) if config.Telemetry.Enabled { From dfe2822f19433f8f18ce9e140c2616a78f9a7d2a Mon Sep 17 00:00:00 2001 From: Dev Ojha Date: Mon, 15 May 2023 15:12:18 +0200 Subject: [PATCH 04/12] Fix lint, point out slowdown --- server/start.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/server/start.go b/server/start.go index ef370ace52f9..9c94dde21194 100644 --- a/server/start.go +++ b/server/start.go @@ -353,14 +353,14 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types. // listen for quit signals so the calling parent process can gracefully exit ListenForQuitSignals(cancelFn, svrCtx.Logger) - grpcSrv, clientCtx, err := startGrpcServer(g, config.GRPC, ctx, clientCtx, svrCtx, app) + grpcSrv, clientCtx, err := startGrpcServer(ctx, g, config.GRPC, clientCtx, svrCtx, app) if err != nil { return err } if config.API.Enable { - // TODO: wtf, why do we reload and unmarshal the entire genesis doc in order to get the chain ID. - // surely theres a better way. + // TODO: Why do we reload and unmarshal the entire genesis doc in order to get the chain ID. + // surely theres a better way. This is likely a serious node start time overhead. genDoc, err := genDocProvider() if err != nil { return err @@ -444,7 +444,7 @@ func setupTraceWriter(svrCtx *Context) (traceWriter io.WriteCloser, cleanup func return traceWriter, cleanup, nil } -func startGrpcServer(g *errgroup.Group, config serverconfig.GRPCConfig, ctx context.Context, clientCtx client.Context, svrCtx *Context, app types.Application) ( +func startGrpcServer(ctx context.Context, g *errgroup.Group, config serverconfig.GRPCConfig, clientCtx client.Context, svrCtx *Context, app types.Application) ( *grpc.Server, client.Context, error) { if !config.Enable { // return grpcServer as nil if gRPC is disabled From 6597cbea469d758677bfde48b2301f9594c48f4a Mon Sep 17 00:00:00 2001 From: Dev Ojha Date: Mon, 15 May 2023 15:17:12 +0200 Subject: [PATCH 05/12] More notes --- server/start.go | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/server/start.go b/server/start.go index 9c94dde21194..816b1374993e 100644 --- a/server/start.go +++ b/server/start.go @@ -12,6 +12,7 @@ import ( "github.com/armon/go-metrics" "github.com/cometbft/cometbft/abci/server" cmtcmd "github.com/cometbft/cometbft/cmd/cometbft/commands" + cmtcfg "github.com/cometbft/cometbft/config" "github.com/cometbft/cometbft/node" "github.com/cometbft/cometbft/p2p" pvm "github.com/cometbft/cometbft/privval" @@ -283,20 +284,13 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types. app := appCreator(svrCtx.Logger, db, traceWriter, svrCtx.Viper) + // TODO: Move this to only be done if were launching the node. (So not in GRPC-only mode) nodeKey, err := p2p.LoadOrGenNodeKey(cfg.NodeKeyFile()) if err != nil { return err } - genDocProvider := func() (*cmttypes.GenesisDoc, error) { - appGenesis, err := genutiltypes.AppGenesisFromFile(cfg.GenesisFile()) - if err != nil { - return nil, err - } - - return appGenesis.ToGenesisDoc() - } - + genDocProvider := getGenDocProvider(cfg) var ( tmNode *node.Node gRPCOnly = svrCtx.Viper.GetBool(flagGRPCOnly) @@ -422,6 +416,18 @@ func getAndValidateConfig(svrCtx *Context) (serverconfig.Config, error) { return config, nil } +// returns a function which returns the genesis doc from the genesis file. +func getGenDocProvider(cfg *cmtcfg.Config) func() (*cmttypes.GenesisDoc, error) { + return func() (*cmttypes.GenesisDoc, error) { + appGenesis, err := genutiltypes.AppGenesisFromFile(cfg.GenesisFile()) + if err != nil { + return nil, err + } + + return appGenesis.ToGenesisDoc() + } +} + func setupTraceWriter(svrCtx *Context) (traceWriter io.WriteCloser, cleanup func(), err error) { traceWriterFile := svrCtx.Viper.GetString(flagTraceStore) traceWriter, err = openTraceWriter(traceWriterFile) From 4db85b3e01dee2eed0999740613ec7ee74deefcf Mon Sep 17 00:00:00 2001 From: Dev Ojha Date: Mon, 15 May 2023 15:25:30 +0200 Subject: [PATCH 06/12] Make startcmtnode fn --- server/start.go | 40 ++++++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 16 deletions(-) diff --git a/server/start.go b/server/start.go index 816b1374993e..ae77f52fea33 100644 --- a/server/start.go +++ b/server/start.go @@ -290,7 +290,6 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types. return err } - genDocProvider := getGenDocProvider(cfg) var ( tmNode *node.Node gRPCOnly = svrCtx.Viper.GetBool(flagGRPCOnly) @@ -301,24 +300,10 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types. config.GRPC.Enable = true } else { svrCtx.Logger.Info("starting node with ABCI CometBFT in-process") - - tmNode, err = node.NewNode( - cfg, - pvm.LoadOrGenFilePV(cfg.PrivValidatorKeyFile(), cfg.PrivValidatorStateFile()), - nodeKey, - proxy.NewLocalClientCreator(app), - genDocProvider, - node.DefaultDBProvider, - node.DefaultMetricsProvider(cfg.Instrumentation), - servercmtlog.CometLoggerWrapper{Logger: svrCtx.Logger}, - ) + tmNode, err = startCmtNode(cfg, nodeKey, app, svrCtx) if err != nil { return err } - - if err := tmNode.Start(); err != nil { - return err - } } // Add the tx service to the gRPC router. We only need to register this @@ -355,6 +340,7 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types. if config.API.Enable { // TODO: Why do we reload and unmarshal the entire genesis doc in order to get the chain ID. // surely theres a better way. This is likely a serious node start time overhead. + genDocProvider := getGenDocProvider(cfg) genDoc, err := genDocProvider() if err != nil { return err @@ -404,6 +390,28 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types. return g.Wait() } +// TODO: Move nodeKey into being created within the function. +func startCmtNode(cfg *cmtcfg.Config, nodeKey *p2p.NodeKey, app types.Application, svrCtx *Context) (tmNode *node.Node, err error) { + tmNode, err = node.NewNode( + cfg, + pvm.LoadOrGenFilePV(cfg.PrivValidatorKeyFile(), cfg.PrivValidatorStateFile()), + nodeKey, + proxy.NewLocalClientCreator(app), + getGenDocProvider(cfg), + node.DefaultDBProvider, + node.DefaultMetricsProvider(cfg.Instrumentation), + servercmtlog.CometLoggerWrapper{Logger: svrCtx.Logger}, + ) + if err != nil { + return tmNode, err + } + + if err := tmNode.Start(); err != nil { + return tmNode, err + } + return tmNode, nil +} + func getAndValidateConfig(svrCtx *Context) (serverconfig.Config, error) { config, err := serverconfig.GetConfig(svrCtx.Viper) if err != nil { From 41807c9e8ff435b879fb8d366a91992b6751da30 Mon Sep 17 00:00:00 2001 From: Dev Ojha Date: Mon, 15 May 2023 15:29:17 +0200 Subject: [PATCH 07/12] Rename config -> svrCfg for clarity with cmtCfg --- server/start.go | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/server/start.go b/server/start.go index ae77f52fea33..9406e19a6da2 100644 --- a/server/start.go +++ b/server/start.go @@ -277,7 +277,7 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types. return err } // TODO: Should this be moved to the very top of the function? - config, err := getAndValidateConfig(svrCtx) + svrCfg, err := getAndValidateConfig(svrCtx) if err != nil { return err } @@ -297,7 +297,7 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types. if gRPCOnly { svrCtx.Logger.Info("starting node in gRPC only mode; CometBFT is disabled") - config.GRPC.Enable = true + svrCfg.GRPC.Enable = true } else { svrCtx.Logger.Info("starting node with ABCI CometBFT in-process") tmNode, err = startCmtNode(cfg, nodeKey, app, svrCtx) @@ -309,17 +309,17 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types. // Add the tx service to the gRPC router. We only need to register this // service if API or gRPC is enabled, and avoid doing so in the general // case, because it spawns a new local CometBFT RPC client. - if (config.API.Enable || config.GRPC.Enable) && tmNode != nil { + if (svrCfg.API.Enable || svrCfg.GRPC.Enable) && tmNode != nil { // Re-assign for making the client available below do not use := to avoid // shadowing the clientCtx variable. clientCtx = clientCtx.WithClient(local.New(tmNode)) app.RegisterTxService(clientCtx) app.RegisterTendermintService(clientCtx) - app.RegisterNodeService(clientCtx, config) + app.RegisterNodeService(clientCtx, svrCfg) } - metrics, err := startTelemetry(config) + metrics, err := startTelemetry(svrCfg) if err != nil { return err } @@ -332,12 +332,12 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types. // listen for quit signals so the calling parent process can gracefully exit ListenForQuitSignals(cancelFn, svrCtx.Logger) - grpcSrv, clientCtx, err := startGrpcServer(ctx, g, config.GRPC, clientCtx, svrCtx, app) + grpcSrv, clientCtx, err := startGrpcServer(ctx, g, svrCfg.GRPC, clientCtx, svrCtx, app) if err != nil { return err } - if config.API.Enable { + if svrCfg.API.Enable { // TODO: Why do we reload and unmarshal the entire genesis doc in order to get the chain ID. // surely theres a better way. This is likely a serious node start time overhead. genDocProvider := getGenDocProvider(cfg) @@ -349,14 +349,14 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types. clientCtx := clientCtx.WithHomeDir(home).WithChainID(genDoc.ChainID) apiSrv := api.New(clientCtx, svrCtx.Logger.With("module", "api-server"), grpcSrv) - app.RegisterAPIRoutes(apiSrv, config.API) + app.RegisterAPIRoutes(apiSrv, svrCfg.API) - if config.Telemetry.Enabled { + if svrCfg.Telemetry.Enabled { apiSrv.SetTelemetry(metrics) } g.Go(func() error { - return apiSrv.Start(ctx, config) + return apiSrv.Start(ctx, svrCfg) }) } @@ -376,6 +376,8 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types. }) // deferred cleanup function + // TODO: Make a generic cleanup function that takes in several func(), and runs them all. + // then we defer that. defer func() { if tmNode != nil && tmNode.IsRunning() { _ = tmNode.Stop() From 7881dc0e3088c13f147b0f2e4dd16581b3abe275 Mon Sep 17 00:00:00 2001 From: Dev Ojha Date: Mon, 15 May 2023 15:30:25 +0200 Subject: [PATCH 08/12] rename cfg -> cmtCfg --- server/start.go | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/server/start.go b/server/start.go index 9406e19a6da2..51c09cff0b76 100644 --- a/server/start.go +++ b/server/start.go @@ -264,8 +264,8 @@ func startStandAlone(svrCtx *Context, appCreator types.AppCreator) error { } func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types.AppCreator) error { - cfg := svrCtx.Config - home := cfg.RootDir + cmtCfg := svrCtx.Config + home := cmtCfg.RootDir db, err := openDB(home, GetAppDBBackend(svrCtx.Viper)) if err != nil { @@ -285,7 +285,7 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types. app := appCreator(svrCtx.Logger, db, traceWriter, svrCtx.Viper) // TODO: Move this to only be done if were launching the node. (So not in GRPC-only mode) - nodeKey, err := p2p.LoadOrGenNodeKey(cfg.NodeKeyFile()) + nodeKey, err := p2p.LoadOrGenNodeKey(cmtCfg.NodeKeyFile()) if err != nil { return err } @@ -300,23 +300,23 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types. svrCfg.GRPC.Enable = true } else { svrCtx.Logger.Info("starting node with ABCI CometBFT in-process") - tmNode, err = startCmtNode(cfg, nodeKey, app, svrCtx) + tmNode, err = startCmtNode(cmtCfg, nodeKey, app, svrCtx) if err != nil { return err } - } - - // Add the tx service to the gRPC router. We only need to register this - // service if API or gRPC is enabled, and avoid doing so in the general - // case, because it spawns a new local CometBFT RPC client. - if (svrCfg.API.Enable || svrCfg.GRPC.Enable) && tmNode != nil { - // Re-assign for making the client available below do not use := to avoid - // shadowing the clientCtx variable. - clientCtx = clientCtx.WithClient(local.New(tmNode)) - app.RegisterTxService(clientCtx) - app.RegisterTendermintService(clientCtx) - app.RegisterNodeService(clientCtx, svrCfg) + // Add the tx service to the gRPC router. We only need to register this + // service if API or gRPC is enabled, and avoid doing so in the general + // case, because it spawns a new local CometBFT RPC client. + if svrCfg.API.Enable || svrCfg.GRPC.Enable { + // Re-assign for making the client available below do not use := to avoid + // shadowing the clientCtx variable. + clientCtx = clientCtx.WithClient(local.New(tmNode)) + + app.RegisterTxService(clientCtx) + app.RegisterTendermintService(clientCtx) + app.RegisterNodeService(clientCtx, svrCfg) + } } metrics, err := startTelemetry(svrCfg) @@ -340,7 +340,7 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types. if svrCfg.API.Enable { // TODO: Why do we reload and unmarshal the entire genesis doc in order to get the chain ID. // surely theres a better way. This is likely a serious node start time overhead. - genDocProvider := getGenDocProvider(cfg) + genDocProvider := getGenDocProvider(cmtCfg) genDoc, err := genDocProvider() if err != nil { return err From b8e96bfeed33561dc4498988cc6b44010ddedf08 Mon Sep 17 00:00:00 2001 From: Dev Ojha Date: Mon, 15 May 2023 15:46:48 +0200 Subject: [PATCH 09/12] Move api server start to its own fn --- server/start.go | 52 +++++++++++++++++++++++++++++-------------------- 1 file changed, 31 insertions(+), 21 deletions(-) diff --git a/server/start.go b/server/start.go index 51c09cff0b76..7b56413590c0 100644 --- a/server/start.go +++ b/server/start.go @@ -337,27 +337,9 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types. return err } - if svrCfg.API.Enable { - // TODO: Why do we reload and unmarshal the entire genesis doc in order to get the chain ID. - // surely theres a better way. This is likely a serious node start time overhead. - genDocProvider := getGenDocProvider(cmtCfg) - genDoc, err := genDocProvider() - if err != nil { - return err - } - - clientCtx := clientCtx.WithHomeDir(home).WithChainID(genDoc.ChainID) - - apiSrv := api.New(clientCtx, svrCtx.Logger.With("module", "api-server"), grpcSrv) - app.RegisterAPIRoutes(apiSrv, svrCfg.API) - - if svrCfg.Telemetry.Enabled { - apiSrv.SetTelemetry(metrics) - } - - g.Go(func() error { - return apiSrv.Start(ctx, svrCfg) - }) + err = startApiServer(ctx, g, cmtCfg, svrCfg, clientCtx, svrCtx, app, home, grpcSrv, metrics) + if err != nil { + return err } // At this point it is safe to block the process if we're in gRPC-only mode as @@ -513,6 +495,34 @@ func startGrpcServer(ctx context.Context, g *errgroup.Group, config serverconfig return grpcSrv, clientCtx, nil } +func startApiServer(ctx context.Context, g *errgroup.Group, cmtCfg *cmtcfg.Config, svrCfg serverconfig.Config, + clientCtx client.Context, svrCtx *Context, app types.Application, home string, grpcSrv *grpc.Server, metrics *telemetry.Metrics) error { + if !svrCfg.API.Enable { + return nil + } + // TODO: Why do we reload and unmarshal the entire genesis doc in order to get the chain ID. + // surely theres a better way. This is likely a serious node start time overhead. + genDocProvider := getGenDocProvider(cmtCfg) + genDoc, err := genDocProvider() + if err != nil { + return err + } + + clientCtx = clientCtx.WithHomeDir(home).WithChainID(genDoc.ChainID) + + apiSrv := api.New(clientCtx, svrCtx.Logger.With("module", "api-server"), grpcSrv) + app.RegisterAPIRoutes(apiSrv, svrCfg.API) + + if svrCfg.Telemetry.Enabled { + apiSrv.SetTelemetry(metrics) + } + + g.Go(func() error { + return apiSrv.Start(ctx, svrCfg) + }) + return nil +} + func startTelemetry(cfg serverconfig.Config) (*telemetry.Metrics, error) { if !cfg.Telemetry.Enabled { return nil, nil From 7751a3356029162f67c4e94a37a5c3e3789178a1 Mon Sep 17 00:00:00 2001 From: Dev Ojha Date: Mon, 15 May 2023 15:56:34 +0200 Subject: [PATCH 10/12] run gofumpt --- server/start.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/server/start.go b/server/start.go index 7b56413590c0..2119c89c75fb 100644 --- a/server/start.go +++ b/server/start.go @@ -443,7 +443,8 @@ func setupTraceWriter(svrCtx *Context) (traceWriter io.WriteCloser, cleanup func } func startGrpcServer(ctx context.Context, g *errgroup.Group, config serverconfig.GRPCConfig, clientCtx client.Context, svrCtx *Context, app types.Application) ( - *grpc.Server, client.Context, error) { + *grpc.Server, client.Context, error, +) { if !config.Enable { // return grpcServer as nil if gRPC is disabled return nil, clientCtx, nil @@ -496,7 +497,8 @@ func startGrpcServer(ctx context.Context, g *errgroup.Group, config serverconfig } func startApiServer(ctx context.Context, g *errgroup.Group, cmtCfg *cmtcfg.Config, svrCfg serverconfig.Config, - clientCtx client.Context, svrCtx *Context, app types.Application, home string, grpcSrv *grpc.Server, metrics *telemetry.Metrics) error { + clientCtx client.Context, svrCtx *Context, app types.Application, home string, grpcSrv *grpc.Server, metrics *telemetry.Metrics, +) error { if !svrCfg.API.Enable { return nil } From 8f196c02be10a723a6fcb34ce2ac6d76cf41adc4 Mon Sep 17 00:00:00 2001 From: Dev Ojha Date: Mon, 15 May 2023 16:37:39 +0200 Subject: [PATCH 11/12] Fix lint --- server/start.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/start.go b/server/start.go index 2119c89c75fb..40c656fb9479 100644 --- a/server/start.go +++ b/server/start.go @@ -337,7 +337,7 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types. return err } - err = startApiServer(ctx, g, cmtCfg, svrCfg, clientCtx, svrCtx, app, home, grpcSrv, metrics) + err = startAPIServer(ctx, g, cmtCfg, svrCfg, clientCtx, svrCtx, app, home, grpcSrv, metrics) if err != nil { return err } @@ -496,7 +496,7 @@ func startGrpcServer(ctx context.Context, g *errgroup.Group, config serverconfig return grpcSrv, clientCtx, nil } -func startApiServer(ctx context.Context, g *errgroup.Group, cmtCfg *cmtcfg.Config, svrCfg serverconfig.Config, +func startAPIServer(ctx context.Context, g *errgroup.Group, cmtCfg *cmtcfg.Config, svrCfg serverconfig.Config, clientCtx client.Context, svrCtx *Context, app types.Application, home string, grpcSrv *grpc.Server, metrics *telemetry.Metrics, ) error { if !svrCfg.API.Enable { From 5578450f9b2275be4b40f0b45362db2f5bc674f1 Mon Sep 17 00:00:00 2001 From: marbar3778 Date: Wed, 17 May 2023 14:31:19 +0200 Subject: [PATCH 12/12] minor todo cleanup --- server/start.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/server/start.go b/server/start.go index 40c656fb9479..c8c3e51e9322 100644 --- a/server/start.go +++ b/server/start.go @@ -222,12 +222,15 @@ func startStandAlone(svrCtx *Context, appCreator types.AppCreator) error { app := appCreator(svrCtx.Logger, db, traceWriter, svrCtx.Viper) - // TODO: should config be getting validated here? config, err := serverconfig.GetConfig(svrCtx.Viper) if err != nil { return err } + if err := config.ValidateBasic(); err != nil { + return err + } + if _, err := startTelemetry(config); err != nil { return err } @@ -272,12 +275,12 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types. return err } - traceWriter, traceWriterCleanup, err := setupTraceWriter(svrCtx) + svrCfg, err := getAndValidateConfig(svrCtx) if err != nil { return err } - // TODO: Should this be moved to the very top of the function? - svrCfg, err := getAndValidateConfig(svrCtx) + + traceWriter, traceWriterCleanup, err := setupTraceWriter(svrCtx) if err != nil { return err }