itest: stop litd nodes via litd's StopDaemon#1356
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request updates the itest harness to ensure reliable node shutdown by prioritizing litd's StopDaemon RPC. By standardizing the connection to litd and implementing an in-memory macaroon baking process for stateless nodes, the harness can now correctly manage the lifecycle of litd-backed nodes regardless of their initialization method. This change is essential for future updates where litd and lnd lifecycles are fully decoupled. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the integration test harness to support stateless node initialization by baking a super macaroon from the in-memory admin macaroon and refactoring the node startup, client initialization, and shutdown sequences to consistently use Lit's own RPCs. The review feedback highlights three key improvement opportunities: checking for a nil gRPC response before accessing its fields to prevent panics, restricting the file permissions of the written macaroon to 0600 for better security, and utilizing a context with a timeout when establishing the Lit RPC connection to avoid potential indefinite blocking.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // BakeSuperMacaroon hex encodes the returned macaroon. | ||
| superMacBytes, err := hex.DecodeString(resp.Macaroon) |
There was a problem hiding this comment.
It is a good defensive programming practice to check if the gRPC response resp is nil before accessing its fields to prevent potential nil pointer dereference panics.
| if err != nil { | |
| return err | |
| } | |
| // BakeSuperMacaroon hex encodes the returned macaroon. | |
| superMacBytes, err := hex.DecodeString(resp.Macaroon) | |
| if err != nil { | |
| return err | |
| } | |
| if resp == nil { | |
| return errors.New("received empty response from BakeSuperMacaroon") | |
| } | |
| // BakeSuperMacaroon hex encodes the returned macaroon. | |
| superMacBytes, err := hex.DecodeString(resp.Macaroon) |
| if err := os.WriteFile(macPath, superMacBytes, 0644); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Macaroons are sensitive credentials and should be written with restrictive file permissions (e.g., 0600 instead of 0644) to prevent unauthorized local users from reading them.
| if err := os.WriteFile(macPath, superMacBytes, 0644); err != nil { | |
| return err | |
| } | |
| if err := os.WriteFile(macPath, superMacBytes, 0600); err != nil { | |
| return err | |
| } |
| litConn, err := connectLitRPC( | ||
| context.Background(), hn.Cfg.LitAddr(), hn.Cfg.LitTLSCertPath, | ||
| macPath, | ||
| ) | ||
| if err != nil { | ||
| return fmt.Errorf("could not connect to Lit RPC: %w", err) | ||
| } | ||
| hn.litConn = litConn | ||
|
|
||
| ctxt, cancel := context.WithTimeout( | ||
| context.Background(), lntest.DefaultTimeout, | ||
| ) | ||
| defer cancel() | ||
| err = wait.NoError(func() error { | ||
| litClient := litrpc.NewProxyClient(hn.litConn) | ||
|
|
||
| _, err := litClient.GetInfo(ctxt, &litrpc.GetInfoRequest{}) | ||
| return err | ||
| }, lntest.DefaultTimeout) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Calling connectLitRPC with context.Background() can cause the test harness to block indefinitely if there are connection issues, because connectLitRPC uses grpc.WithBlock(). We should reuse the context with timeout (ctxt) to ensure the connection attempt is bounded.
ctxt, cancel := context.WithTimeout(
context.Background(), lntest.DefaultTimeout,
)
defer cancel()
litConn, err := connectLitRPC(
ctxt, hn.Cfg.LitAddr(), hn.Cfg.LitTLSCertPath,
macPath,
)
if err != nil {
return fmt.Errorf("could not connect to Lit RPC: %w", err)
}
hn.litConn = litConn
err = wait.NoError(func() error {
litClient := litrpc.NewProxyClient(hn.litConn)
_, err := litClient.GetInfo(ctxt, &litrpc.GetInfoRequest{})
return err
}, lntest.DefaultTimeout)
if err != nil {
return err
}
71061fd to
523fae5
Compare
1 participant
This is split out of #1329 at @ViktorT-11's request, so it can be reviewed on its own. #1329 will be rebased on top of this.
What this does
The itest harness stops a node through litd's own
StopDaemonwhen it has a lit connection, and falls back to lnd'sStopDaemonotherwise. The lit connection (litConn) was only ever set up on the no-seed path inStart, so seed based nodes (stateless init, seed restarts) always fell back to lnd.Two changes:
litConnsetup moves intoinitLightningClient, so every node gets one, seed or not.Init/InitChangePassword, writes it to the node's directory, and uses that forlitConn.This mirrors what a real stateless setup does:
Why it matters
On master this is a no-op in behaviour, since stopping lnd also brings litd down either way. It becomes necessary with #1329, where litd deliberately keeps running after lnd stops. Without it, seed based nodes stop lnd only, litd stays up by design, and the harness times out waiting for the process to exit.
Testing
terminal stateless init modepasses on this branch on top of master.