[sql-47] Actions migration prep #1126

ViktorTigerstrom · 2025-08-04T22:40:37Z

This PR prepares litd's startup process in preparation for the actions migration.

In the upcoming actions migration, we need fetch LNDs macaroons prior to initializing the stores, as the macaroons will be required during the migration.

This requires that we setup the connection to LND before we initialize the stores, as we can only fetch the macaroons after the LND connection is established.

This PR refactors the litd startup process, so that the stores are initialized after the LND connection is established. This also requires that we refactor the RPC server setup, and separate the starting of the RPC servers from their creation, as litd's RPC servers prior to this PR require the stores to be initialized prior to being created.

gemini-code-assist

Summary of Changes

Hello @ViktorTigerstrom, I'm Gemini Code Assist¹! 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 refactors the startup process of litd to prepare for an upcoming actions migration. The primary goal is to establish a connection to LND and fetch necessary macaroons before initializing the application's stores. This reordering is crucial because the macaroons are required during the migration, and they can only be fetched once an LND connection is active. Consequently, the setup of litd's RPC servers, which depend on initialized stores, has also been adjusted to occur later in the startup sequence.

Highlights

Startup Process Reordering: I've reordered the litd startup sequence in terminal.go to ensure that the connection to LND is established and basic LND client is set up before the application's stores are initialized and RPC servers are created. This allows for fetching LND macaroons earlier in the process.
Split LND Client Setup: The LND client setup has been split into two distinct phases: setupBasicLNDClient for an early, non-blocking connection to fetch macaroons, and setupFullNDClient (formerly setUpLNDClients) which waits for LND to be fully synced and unlocked, used later in the startup.
RPC Server Initialization and Registration: The initialization of account and session RPC servers, along with their registration to the RPC proxy, now occurs after the stores are initialized and the basic LND client is connected. A new helper function, registerSubDaemonGrpcServersToProxy, has been introduced to manage this registration explicitly.

Using Gemini Code Assist

The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize 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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This PR refactors the startup process of litd to initialize the LND connection before the data stores, which is a prerequisite for an upcoming actions migration. The changes look good and achieve the stated goal. I've found one critical bug related to initialization order that could cause a panic, and a couple of minor typos in comments.

terminal.go

gemini-code-assist · 2025-08-04T22:42:45Z

terminal.go

+	host, network, tlsPath, macPath, macData := g.cfg.lndConnectParams()
+
+	if g.cfg.LndMode == ModeIntegrated {
+		// Ssince we will not require tls when communicating with lnd


Typo: "Ssince" should be "Since".

Suggested change

// Ssince we will not require tls when communicating with lnd

// Since we will not require tls when communicating with lnd

terminal.go

ViktorTigerstrom · 2025-08-07T13:41:03Z

The CI flake in the last push is not related to this PR, see:
#1129

bitromortac

I think conceptually it looks good, though I don't have a lot of context concerning the startup process yet.

accounts/rpcserver.go

bitromortac · 2025-08-07T16:09:39Z

session_rpcserver.go

@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"strings"
 	"sync"
+	"sync/atomic"


nit: I think the commit message doesn't need to be repeated, it could just say that we do a similar separation like in the previous commit

Ok updated to not repeat the message and just mention the previous commit.

The main I see though for repetition, is that I do think it's much better from future perspective where someone needs to git blame a specific row from that commit. It makes more sense if the actual commit message mentions why the change was needed, rather than referring to a previous commit message as that requires someone to look up the exact ordering in the commit tree.

session_rpcserver.go

bitromortac · 2025-08-07T16:18:57Z

terminal.go

-	g.accountRpcServer = accounts.NewRPCServer(
-		g.accountService, superMacBaker,
-	)
+	g.accountRpcServer = accounts.NewRPCServer()


we make this change instead of also shifting it after the lnd connection creation in order to error out on RPC calls, right?

Not exactly:
This isn't intuitive when just looking at the litd code, but when the lnd connection is setup for an integrated lnd instance, lnd will call the RegisterGrpcSubserver in litd during the setup of the lnd connection.
If we kept the registerSubDaemonGrpcServers as is while changing the behaviour so that the g.sessionRpcServer & g.accountRpcServer objects where initialised after the lnd connection was setup, you'd then register nil pointers for those RPC servers. Later when litd has been fully started and request gets passed to those RPC servers, they would then nil pointer panic.

We could circumvent this by refactoring the registerSubDaemonGrpcServers and manually register those servers with the g.rpcProxy.grpcServer after the lnd connection has been setup and we've initialised the RPC servers, but that would require updates to the itests and be harder to reason about.

manually register those servers with the g.rpcProxy.grpcServer after the lnd connection has been setup and we've initialised the RPC servers

I think that isnt possible right? i dont think you can register grpc servers after the server has been started

lnd will call the RegisterGrpcSubserver in litd during the setup of the lnd connection.
If we kept the registerSubDaemonGrpcServers as is while changing the behaviour so that the g.sessionRpcServer & g.accountRpcServer

Ok that makes sense - this would have been helpful in the PR description btw :) i only understood after reading this comment 👍

perhaps it would be useful to add a comment before the LND set-up just to make this super clear since it is quite hard to see by just reading the lit code & has always been a bit confusing imo

Added a commit 604c109 that adds some docs for that :)! I also updated 3d85944 & 4a6f627 to clarify that during the init of the accounts and session RPC server variables

Thanks for the explanation, I think I understand that better now. Something that's not clear to me is why we are registering the sub-services with the lnd-created grpc server and not only with lit's proxy server?

terminal.go

ViktorTigerstrom · 2025-08-08T10:32:16Z

Thanks for the review @bitromortac 🙏! Addressed your feedback with the latest push :)

accounts/rpcserver.go

ellemouton · 2025-08-12T10:26:45Z

accounts/rpcserver.go

+	if !s.started() {
+		return nil, ErrServerNotActive
+	}


i think if this is a pattern we use everywhere, then use a grpc interceptor pattern instead. Else we have to remember to check this each time.

but actually also: isnt this already checked by the rpc proxy's interceptor (checkSubSystemStarted)?

but actually also: isnt this already checked by the rpc proxy's interceptor (checkSubSystemStarted)?

Yes, this is already checked with the rpcProxy.checkSubSystemStarted function. I had added a comment for this in the commit message:
3d85944

I have now also added docs for the actual active variables to make that very clear.

My reasoning for this is that I did it for a defensive programming approach to ensure that we don't end up adding future updates which breaks this rpcProxy interceptor logic and leads to nil pointer panics. The main scenarios I see for that would be if we start white-listening some account or session RPCs, or if we add some custom status to the lit sub-server that makes it possible to send some accountorsessionRPC requests prior to thelit` sub-server being marked as running.

This may just be over-engineering though, so I'd be fine to just remove these active booleans if you think we shouldn't try to optimize for that already :)!

i think for now it is over-engineering imo. i just done love that we are basically doing the job of an interceptor & doing so by repeating the same check everywhere - imo that makes it easy to forget to add that check to any new method.

So id then rather have some more generic way of doing it: like this for example. And then a comment saying that any Registrar of the service must plug in the interceptor.

Also, just re nil pointers - it is very standard to assume that a subserver's methods are not ready unless Start has actually been called.

Thanks a lot for the suggestion @ellemouton! After discussing this offline, we decided that we're just going to drop the checks of if the account & session servers are started. Instead we'll keep the logic as is, and let the rpcProxy checkSubSystemStarted function that checks if the lit sub-server is started or not which will ensure that no requests are passed to the account & session servers before they've been started.

ellemouton · 2025-08-12T10:40:09Z

session_rpcserver.go

+	if !s.started() {
+		return nil, ErrServerNotActive
+	}


similar to previous comment: can probs just add a subservers.SESSIONS registration & then the rpc proxy should already check this via it's interceptor yeah?

See #1126 (comment)

ellemouton · 2025-08-12T10:41:09Z

terminal.go

-	g.accountRpcServer = accounts.NewRPCServer(
-		g.accountService, superMacBaker,
-	)
+	g.accountRpcServer = accounts.NewRPCServer()


perhaps it would be useful to add a comment before the LND set-up just to make this super clear since it is quite hard to see by just reading the lit code & has always been a bit confusing imo

ellemouton

looking good! lemme know what you think about the startableService idea.

ellemouton · 2025-08-14T10:54:53Z

accounts/rpcserver.go

+	if !s.started() {
+		return nil, ErrServerNotActive
+	}


i think for now it is over-engineering imo. i just done love that we are basically doing the job of an interceptor & doing so by repeating the same check everywhere - imo that makes it easy to forget to add that check to any new method.

So id then rather have some more generic way of doing it: like this for example. And then a comment saying that any Registrar of the service must plug in the interceptor.

Also, just re nil pointers - it is very standard to assume that a subserver's methods are not ready unless Start has actually been called.

terminal.go

bitromortac

LGTM, nice investigation making it work to establish the LND connection first 🎉

accounts/rpcserver.go

session_rpcserver.go

bitromortac · 2025-08-20T08:49:32Z

terminal.go

-	g.accountRpcServer = accounts.NewRPCServer(
-		g.accountService, superMacBaker,
-	)
+	g.accountRpcServer = accounts.NewRPCServer()


Thanks for the explanation, I think I understand that better now. Something that's not clear to me is why we are registering the sub-services with the lnd-created grpc server and not only with lit's proxy server?

As it's quite confusing by just looking at `litd` codebase that LND will execute `litd`'s `RegisterGrpcSubserver` function during the setup of the LND connection, we add a comment to clarify this.

In the upcoming actions migration, we need fetch LNDs macaroons prior to initializing the stores, as the macaroons will be required during the migration. This requires that we setup the connection to LND before we initialize the stores, as we can only fetch the macaroons after the LND connection is established. In preparation of doing so, we cannot reference the stores when initializing/creating the accounts RPC server object, as we do so prior to setting up the LND connection. This commit therefore refactors the accounts RPC server so that we separate the initializing from the starting of the RPC server, and only require the store reference during the actual startup of the RPC server. Note that we still keep the init of the accounts RPC server reference prior to setting up the LND connection, as not doing so would require that we'd refactor the registering of `GrpcSubserver`s in a more complex and in a less elegant way.

Similar to how the previous commit separated the initialization and starting of the account RPC server, this commit separates the initialization and starting of the sessions RPC server.

In the upcoming actions migration, we need fetch LNDs macaroons prior to initializing the stores, as the macaroons will be required during the migration. This requires that we setup the connection to LND before we initialize the stores, as we can only fetch the macaroons after the LND connection is established. This commit refactors the litd startup process, so that the stores are initialized after the LND connection is established.

In the upcoming actions migration, we will need to fetch LND's macaroons prior creating litd's stores, and therefore we need to connect to LND prior to creating the stores. To avoid having to wait for LND to fully sync before creating the stores (and, by extension, Litd's RPC servers), we separate the basic LND client setup from the full LND client setup. Only the full setup requires a fully synced LND.

ViktorTigerstrom added the no-changelog This PR is does not require a release notes entry label Aug 4, 2025

gemini-code-assist bot reviewed Aug 4, 2025

View reviewed changes

ViktorTigerstrom force-pushed the 2025-08-actions-migration-prep branch 3 times, most recently from 0d9f96f to 8a4e962 Compare August 6, 2025 13:07

ViktorTigerstrom marked this pull request as ready for review August 6, 2025 13:10

ViktorTigerstrom self-assigned this Aug 6, 2025

ViktorTigerstrom requested review from ellemouton and bitromortac August 6, 2025 13:19

ViktorTigerstrom mentioned this pull request Aug 7, 2025

[sql-48] session: sort MacaroonRecipe.caveats in migration #1129

Merged

bitromortac reviewed Aug 7, 2025

View reviewed changes

ViktorTigerstrom force-pushed the 2025-08-actions-migration-prep branch from 8a4e962 to 1cfe9e7 Compare August 8, 2025 10:19

ViktorTigerstrom requested a review from bitromortac August 8, 2025 10:31

ellemouton reviewed Aug 12, 2025

View reviewed changes

ViktorTigerstrom force-pushed the 2025-08-actions-migration-prep branch from 1cfe9e7 to 604c109 Compare August 13, 2025 14:32

ViktorTigerstrom requested a review from ellemouton August 13, 2025 14:50

ellemouton reviewed Aug 14, 2025

View reviewed changes

ViktorTigerstrom force-pushed the 2025-08-actions-migration-prep branch from 604c109 to 84adc05 Compare August 15, 2025 12:32

ViktorTigerstrom requested a review from ellemouton August 15, 2025 12:37

ViktorTigerstrom force-pushed the 2025-08-actions-migration-prep branch from 84adc05 to 49f6900 Compare August 15, 2025 12:39

bitromortac approved these changes Aug 20, 2025

View reviewed changes

ViktorTigerstrom added 5 commits August 21, 2025 16:33

terminal: add clarifying comment for LND startup

a79187f

As it's quite confusing by just looking at `litd` codebase that LND will execute `litd`'s `RegisterGrpcSubserver` function during the setup of the LND connection, we add a comment to clarify this.

terminal: separate session RPC server init & start

7dafaed

Similar to how the previous commit separated the initialization and starting of the account RPC server, this commit separates the initialization and starting of the sessions RPC server.

ViktorTigerstrom force-pushed the 2025-08-actions-migration-prep branch from 49f6900 to d163247 Compare August 21, 2025 14:34

	// Ssince we will not require tls when communicating with lnd
	// Since we will not require tls when communicating with lnd

[sql-47] Actions migration prep #1126

Are you sure you want to change the base?

[sql-47] Actions migration prep #1126

Uh oh!

Conversation

ViktorTigerstrom commented Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

gemini-code-assist bot Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ViktorTigerstrom commented Aug 7, 2025

Uh oh!

bitromortac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorTigerstrom Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ViktorTigerstrom commented Aug 8, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bitromortac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ViktorTigerstrom commented Aug 4, 2025 •

edited

Loading

ViktorTigerstrom Aug 8, 2025 •

edited

Loading