faraday+frdrpcserver: make faraday an active component#234
Conversation
|
/gemini review |
![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 significantly refactors the Faraday daemon by extracting server lifecycle management from frdrpcserver.RPCServer into a new top-level Faraday struct. This change effectively turns RPCServer into a thin handler wrapper, improving separation of concerns and enabling Faraday to operate as a first-class subserver within litd. The architecture diagram and key changes table clearly illustrate the benefits of this refactoring. The changes are well-structured and enhance the modularity and maintainability of the codebase.
ViktorT-11
left a comment
ViktorT-11
left a comment
There was a problem hiding this comment.
I think this mainly looks good, but this PR is quite hard to follow.
I think the reason for that is mainly that the first commit both mixes refactors & new additions in the same commit. The commit message for the commit is also very sparse.
Could potentially structure it like this (even though this might be a bit hard):
- Commit 1: Refactors only and move code from the
frdrpcserverto the &faradaypackage. - Commit 2: Introduction of the new additions that this PR also adds.
That'd be a much cleaner and easier way to follow what is actually changed here and what is just moved between the packages.
bitromortac
force-pushed
the
2603-fwd-prep-5
branch
from
74e4a7c to
61fdc05
Compare
Right, sorry about that, I tried to split it up. I think it should be better now, though I added some other fixes that came up in self-review. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant architectural refactoring by creating a new faraday.Faraday struct. This struct now centralizes the lifecycle management of various components, including the LND client, Bitcoin client, Macaroon service, gRPC server, and REST server. Consequently, the frdrpcserver.RPCServer has been simplified to primarily serve as a handler for RPC requests, with its configuration and lifecycle methods moved to the new Faraday struct. The Main function has been updated to utilize this new Faraday structure for starting and stopping the service. Review comments suggest improvements in error propagation for server.Stop(), refining error messages for server startup, addressing code duplication in macaroon service cleanup, correcting a typo, and considering the robustness of the REST proxy's ReadHeaderTimeout.
Move the gRPC/REST server lifecycle, macaroon service management, and TLS setup from frdrpcserver.RPCServer to a new Faraday struct in the faraday package. This leaves RPCServer as a pure RPC request handler with only business logic methods. The Faraday struct embeds *frdrpcserver.RPCServer and takes ownership of all infrastructure concerns: gRPC/REST server creation, macaroon service setup, TLS configuration, and start/stop lifecycle. The frdrpcserver.Config is slimmed down to just Lnd and BitcoinClient. The macaroons.go file is also moved from frdrpcserver to the faraday package, as the macaroon constants are now used by the Faraday struct directly.
Refine the Faraday struct's lifecycle management: - Use atomic.Bool for cleaner start/stop guards with CompareAndSwap instead of the old atomic.AddInt32 pattern. - Extract initialize() to consolidate macaroon and bitcoin client setup shared between Start() and StartAsSubserver(). - Extract startRPCServer()/stopRPCServer() from the monolithic Start()/Stop() methods for better separation of concerns. - Use the lndOwned flag so Stop() only closes the lnd connection in standalone mode, leaving it open for the parent in subserver mode. - Move lnd connection and bitcoin client creation from Main() into the Faraday struct so callers only need New() and Start(). - Mark the struct as permanently stopped on Start failure to prevent retry with stale internal state. Guard startRPCServer against a nil macaroon service. - Reorder Stop() to wait for in-flight RPC goroutines before tearing down the macaroon service.
bitromortac
force-pushed
the
2603-fwd-prep-5
branch
from
61fdc05 to
1b25767
Compare
ellemouton
left a comment
ellemouton
left a comment
There was a problem hiding this comment.
nice!
just the last commit should be removed :)
bitromortac
force-pushed
the
2603-fwd-prep-5
branch
from
1b25767 to
875ea74
Compare
ViktorT-11
left a comment
ViktorT-11
left a comment
There was a problem hiding this comment.
Thanks for the changes, LGTM 🔥!
bitromortac
merged commit 403c32e
into
lightninglabs:faraday-forwarding-ability
3 participants
This extracts server lifecycle management from
frdrpcserver.RPCServerinto a new top-levelFaradaystruct, turningRPCServerinto a thin handler wrapper. Faraday now owns gRPC/REST servers, TLS, macaroons, and the lnd connection, enabling it to run as a first-class subserver in litd rather than just an RPC handler. This is needed to later watch for channel event updates, as the alternative would have been to let the goroutine run in the RPC subserver, which is a violation of concerns.See architecture diagram for a before/after overview.