Aux Closer: Move coop-close aux finalization to chain watcher
Description
Moves the aux closer finalization hook call site from ReceiveClosingSigned, which relies on our peer staying online and sending us the message, to the chain watcher where we instead wait for the coop close tx to be detected onchain.
This implies that the finalization will be completed after some delay, but this also comes coupled with the guarantee of not having to rely on our peer behaving correctly.
Circular dependency
The aux closer is used in lnwallet/chancloser and now also in contractcourt (by chain watcher). This caused a circular dependency which disallowed us from importing some definitions directly from the former package. The direct solution was to replicate some of the types and use an adapter on the server for our new slim interface. Not an ideal solution, will keep searching for alternatives -- although this is non blocking for the purpose of this PR.
Summary of Changes
Hello @GeorgeTsagk, I'm Gemini Code Assist[^1]! 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 cooperative channel closing mechanism to enhance its reliability. It shifts the responsibility of finalizing auxiliary channel closures from relying on a peer's message to monitoring the blockchain for the confirmed cooperative close transaction. This change ensures that finalization is guaranteed, albeit with a slight delay, by removing the dependency on the peer's online status or correct behavior. The implementation also addresses a resulting circular dependency by introducing new types and an adapter.
Highlights
- Aux Closer Relocation: The finalization hook call for the aux closer has been moved from
ReceiveClosingSigned(which relied on peer messages) to thechain watcher. This means finalization now occurs when the cooperative close transaction is detected on-chain, ensuring robustness even if the peer is offline or misbehaves. - Circular Dependency Resolution: A circular dependency between
lnwallet/chancloserandcontractcourtwas introduced due to the aux closer's new usage. This was resolved by replicating necessary types incontractcourtand implementing an adapter inserver.goto bridge the interfaces. - New Interface and Types: New types (
CloseOutput,AuxShutdownReq,AuxCloseDesc) and an interface (AuxChanCloser) have been defined withincontractcourtto support the new on-chain finalization logic and manage the circular dependency.
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 by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.
| 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 pull request 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 :thumbsup: and :thumbsdown: 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.
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.
[^1]: 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.
Replying to @gijswijs's comment:
If node restarts after close tx confirms but before finalization, what happens? Do we re-detect the spend on restart?
I believe that this is the case, since this is code that is called for any channel in LND (with the finalizeCoopClose being optionally called).
I'm missing an unit test for finalizeCoopClose., although the original ReceiveClosingSigned func also doesn't have any testing iiiuc. But this might be a good time to increase that coverage.
Yeah there's not much to test as this is an external hook. We can make a basic-level assertion on a mock implementation of it -- i.e just make sure that it is being called with non-nil parameters.
I will re-request review although 2 main things remain to figure out:
- Do we keep both call-sites for better UX? (https://github.com/lightningnetwork/lnd/pull/10289#discussion_r2439129067)
- Add test coverage (probably in LiT) to expose the behavior of a peer dropping the connection before sending the closing signed message.
Rebased
@gijswijs: review reminder @georgetsagk, remember to re-request review from reviewers when ready