feat: fix gRPC server #55
Conversation
WalkthroughThe changes refactor the startup sequence within the Changes
Sequence Diagram(s)sequenceDiagram
participant Main as startInProcess
participant GRPC as startGrpcServer
participant Rollkit as startNode
participant API as startAPIServer
participant Hook as PostSetupHook
Main->>GRPC: Start gRPC server (if enabled and not gRPC-only)
alt gRPC error
GRPC-->>Main: Return error
end
alt not gRPC-only
Main->>Rollkit: Start Rollkit node
Rollkit-->>Main: Node started (defer cleanup)
Main->>Main: Register services if API/gRPC enabled and Rollkit RPC available
else gRPC-only
Main->>Main: Skip Rollkit node and service registration
end
alt gRPC enabled but not started
Main->>GRPC: Start gRPC server
end
Main->>API: Start API server (if enabled)
alt API error
API-->>Main: Return error
end
Main->>Hook: Run PostSetup hook (if provided)
Main->>Main: Wait for termination signals and errgroup completion
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
RollkitBot
requested review from
a team,
tuxcanfly,
yarikbratashchuk and
Manav-Aggarwal
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
server/start.go (1)
254-265: 🛠️ Refactor suggestion
⚠️ Potential issue
grpc.NewClientdoes not exist – compilation will failThe canonical way to obtain a client connection is
grpc.DialContext(orgrpc.Dial).grpc.NewClientis not part ofgoogle.golang.org/grpc.- grpcClient, err := grpc.NewClient( - config.Address, + grpcClient, err := grpc.DialContext( + ctx, + config.Address, grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithDefaultCallOptions( grpc.ForceCodec(codec.NewProtoCodec(clientCtx.InterfaceRegistry).GRPCCodec()), grpc.MaxCallRecvMsgSize(maxRecvMsgSize), grpc.MaxCallSendMsgSize(maxSendMsgSize), ), )This returns
*grpc.ClientConn, which satisfiesgrpc.ClientConnInterface, soclientCtx.WithGRPCClientcontinues to work.
🧹 Nitpick comments (2)
server/start.go (2)
256-264: Consider replacing deprecatedgrpc.ForceCodec
grpc.ForceCodecis deprecated; modern gRPC prefersgrpc.ForceServerCodec/grpc.ForceClientCodec(or interceptors). Deprecations don’t break compilation today but create tech‑debt.
361-370: Variable shadows imported packagesigner, harming readabilityDeclaring
var signer signer.Signerovershadows the importedsignerpackage for the entire function scope.
While it compiles, it surprises readers and prevents accidental future use of the package inside the function.- var signer signer.Signer + var seqSigner signer.SignerRename the variable (and subsequent references) to avoid shadowing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/start.go(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
server/start.go (1)
rpc/rpc.go (1)
RPCServer(57-65)
🔇 Additional comments (2)
server/start.go (2)
168-175:svrCfgmutation only local – verify expectationsSetting
svrCfg.GRPC.Enable = truein gRPC‑only mode alters only the local copy (the struct is passed by value). That is fine here, but double‑check that no pointer to the original config is shared elsewhere and relied upon later (e.g. in hooks).
450-451: Nil‑safety forappGenesis.AppState
appGenesis.AppStateis passed intogenesis.NewGenesis; ensure the value is nevernil, otherwise Rollkit genesis may marshal to an empty state and nodes started from the same genesis file could disagree.
server/start.go
Outdated
| // 1. Start gRPC server FIRST (if enabled and not gRPC-only mode initially) | ||
| // Uses the main errgroup 'g' and context 'ctx' | ||
| if !gRPCOnly && svrCfg.GRPC.Enable { | ||
| svrCtx.Logger.Info("Attempting to start gRPC server before Rollkit node...") | ||
| grpcSrv, clientCtx, err = startGrpcServer(ctx, g, svrCfg.GRPC, clientCtx, svrCtx, app) | ||
| if err != nil { | ||
| return err | ||
| svrCtx.Logger.Error("Failed to start gRPC server early", "error", err) | ||
| // Decide whether to proceed or return the error | ||
| return fmt.Errorf("failed to start gRPC server: %w", err) // Return error if gRPC is essential | ||
| } | ||
| defer cleanupFn() | ||
|
|
||
| // 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(rpcServer) | ||
| svrCtx.Logger.Info("Successfully started gRPC server early.") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Early‑started gRPC server is left running when a later step fails
startGrpcServer is spawned before the Rollkit node and API server.
If startGrpcServer succeeds but startNode (or any step that follows) returns an error, startInProcess bails out immediately (e.g. line 141). The context that the gRPC goroutine is listening to (ctx) is never cancelled and g.Wait() is never reached, leaving the gRPC listener alive and leaking the port and goroutine.
A minimal fix is to ensure the context is cancelled on every early‑return path after the server could have been spawned.
- grpcSrv, clientCtx, err = startGrpcServer(ctx, g, svrCfg.GRPC, clientCtx, svrCtx, app)
+ grpcSrv, clientCtx, err = startGrpcServer(ctx, g, svrCfg.GRPC, clientCtx, svrCtx, app)
if err != nil {
svrCtx.Logger.Error("Failed to start gRPC server early", "error", err)
- return fmt.Errorf("failed to start gRPC server: %w", err) // Return error if gRPC is essential
+ cancel() // new ‑‑ ensure all sub‑goroutines terminate
+ return fmt.Errorf("failed to start gRPC server: %w", err)
}You need a cancel handle; the simplest way is to capture it from getCtx (return (g, ctx, cancel) or similar).
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Can we keep this logic as close as possible to the SDK server/start.go? And remove the bunch of added info logs, that's a bit too verbose otherwise.
Overview
gRPC server started after executing the node, so it never was executed.
Summary by CodeRabbit