Aux Closer: Move coop-close aux finalization to chain watcher #10289
Conversation
Summary of ChangesHello @GeorgeTsagk, 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 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
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 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 counter productive. 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. 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. Footnotes
|
GeorgeTsagk
changed the title
![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 refactors the cooperative close finalization by moving the AuxChanCloser hook from ReceiveClosingSigned to the chainWatcher. This is a good improvement as it makes the finalization process more reliable by depending on on-chain events rather than peer messages.
The implementation of the adapter pattern to resolve the circular dependency is clean. However, I've found a critical issue in the new finalizeCoopClose function where the logic to identify local and remote outputs is incorrect and could lead to problems if there are more than two outputs in the closing transaction. Additionally, some fields in AuxCloseDesc were not being populated. I've provided a detailed comment and a code suggestion to address this.
There was a problem hiding this comment.
I did a first pass on this PR and left some comments.
There's also a typo in the second commit: countractcourt+lnwallet should be contractcourt+lnwallet
I have a few general questions as well:
If node restarts after close tx confirms but before finalization, what happens? Do we re-detect the spend on restart?
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.
| } | ||
| c.closingTx = closeTx | ||
|
|
||
| // If there's an aux chan closer, then we'll finalize with it |
There was a problem hiding this comment.
question: is there value in leaving this code?
This PR introduces more robust handling of the aux closer finalization by moving the hook call site from ReceiveClosingSigned to the chainwatcher. That also introduces a waiting time of ~10 minutes before the finalization is initiated. Couldn't we keep doing both?
In case our peer stays online this would lead to a faster finalization which in turn maybe is a better user experience.
|
Replying to @gijswijs's comment:
I believe that this is the case, since this is code that is called for any channel in LND (with the
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. |
|
@Roasbeef: review reminder |
contractcourt/chain_watcher.go
Outdated
| } | ||
|
|
||
| // AuxCloseDesc is used to describe the channel close that is being performed. | ||
| type AuxCloseDesc struct { |
There was a problem hiding this comment.
These types are duplicated from lnwallet/chancloser/aux_closer.go. Why do they need to be moved here?
There was a problem hiding this comment.
Following @gijswijs comments I'll extract them into a separate package and use that one in both places (to avoid circular imports)
contractcourt/chain_watcher.go
Outdated
| // TODO(roasbeef): Extract the internal key if this is a taproot | ||
| // channel. For now, we leave it as None since we don't persist it | ||
| // separately from the delivery script. | ||
| var internalKey fn.Option[btcec.PublicKey] |
There was a problem hiding this comment.
In the existing call site, this is always properly set.
There was a problem hiding this comment.
Yes that's true.
This turned out to not be needed though as it's only used in the hook ShutdownBlob:
- LND callsite: https://github.com/lightningnetwork/lnd/blob/master/lnwallet/chancloser/chancloser.go#L381
- Tapd usage of internal key: https://github.com/lightninglabs/taproot-assets/blob/main/tapchannel/aux_closer.go#L557
The hook FinalizeClose does not actually use internalKey at all:
https://github.com/lightninglabs/taproot-assets/blob/main/tapchannel/aux_closer.go#L685
server.go
Outdated
| } | ||
|
|
||
| // FinalizeClose adapts the chancloser types to contractcourt types. | ||
| func (a auxCloserAdapter) FinalizeClose(desc contractcourt.AuxCloseDesc, |
There was a problem hiding this comment.
Why do we need this adapter at all? Why can't we pass the existing interface as defined in the chanclose package into the chain watcher?
There was a problem hiding this comment.
Was needed to mitigate the import cycle caused by not having this #10289 (comment)
Now that it's added (in lnwallet/types) the import cycle is avoided and we actually don't need any adapter.
The aux close types will soon be used by a different package that would otherwise cause an import cycle if used directly from lnwallet/chancloser. We now create a new sub-package lnwallet/types that will be improrted from all users of these types.
Due to a circular dependency issue, we cannot directly import these definitions from lnwallet/chancloser. So we'll redefine the types we need and define a slim interface around the aux closer, which contains exactly the functionality that we'll want to use here.
We now execute the aux chan closer finalization within the chain watcher. This is better as we don't need to rely on the remote party being online and sending us a message. Instead we do the finalization once the on-chain transaction has been confirmed.
The final step is to provide the aux closer to the corresponding configs from the server. Here we use an adapter pattern which returns an aux chan closer as defined in the contractcourt package.
GeorgeTsagk
force-pushed
the
move-aux-closer
branch
from
2b077d5 to
3f5f7ed
Compare
|
I will re-request review although 2 main things remain to figure out:
|
4 participants
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/chancloserand now also incontractcourt(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.