Aux Closer: Move coop-close aux finalization to chain watcher #10289
Conversation
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.
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.
| // Extract close outputs from the transaction. We need to identify | ||
| // which outputs belong to local vs remote parties. | ||
| var localCloseOutput, remoteCloseOutput fn.Option[CloseOutput] | ||
|
|
||
| // Get the delivery scripts for both parties. | ||
| var localDeliveryScript lnwire.DeliveryAddress | ||
| shutdown.WhenSome(func(s channeldb.ShutdownInfo) { | ||
| localDeliveryScript = s.DeliveryScript.Val | ||
| }) | ||
|
|
||
| // Scan through the close transaction outputs to identify local and | ||
| // remote outputs. | ||
| for _, out := range closeTx.TxOut { | ||
| if len(localDeliveryScript) > 0 && | ||
| slices.Equal(out.PkScript, localDeliveryScript) { | ||
|
|
||
| localCloseOutput = fn.Some(CloseOutput{ | ||
| Amt: btcutil.Amount(out.Value), | ||
| PkScript: out.PkScript, | ||
| DustLimit: chanState.LocalChanCfg.DustLimit, | ||
| }) | ||
| } else { | ||
| // This must be the remote output. | ||
| remoteCloseOutput = fn.Some(CloseOutput{ | ||
| Amt: btcutil.Amount(out.Value), | ||
| PkScript: out.PkScript, | ||
| DustLimit: chanState.RemoteChanCfg.DustLimit, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // Build the AuxCloseDesc. | ||
| desc := AuxCloseDesc{ | ||
| AuxShutdownReq: req, | ||
| LocalCloseOutput: localCloseOutput, | ||
| RemoteCloseOutput: remoteCloseOutput, | ||
| } |
There was a problem hiding this comment.
The logic for identifying local and remote outputs in finalizeCoopClose is incorrect. It assumes that any output that is not the local party's must be the remote party's. This is not safe if the closing transaction has more than two outputs (e.g., from an AuxChanCloser). This could lead to misidentifying outputs and passing incorrect data to the AuxChanCloser.
Additionally, the AuxCloseDesc is missing the CloseFee and CommitFee fields. The ShutdownRecords field on CloseOutput is also not populated for local or remote outputs.
For the localCloseOutput, the custom records can be retrieved from ShutdownInfo. However, for remoteCloseOutput, the custom records from the remote peer's shutdown message are not available in chainWatcher as they are not persisted. This should be addressed to ensure AuxChanCloser implementations have all necessary data.
I suggest refactoring this block to correctly identify both local and remote outputs by their respective delivery scripts and to populate all fields of AuxCloseDesc and its nested structs as completely as possible.
// Extract close outputs from the transaction. We need to identify
// which outputs belong to local vs remote parties.
var localCloseOutput, remoteCloseOutput fn.Option[CloseOutput]
// Get the delivery scripts for both parties.
var localDeliveryScript lnwire.DeliveryAddress
var localShutdownRecords lnwire.CustomRecords
shutdown.WhenSome(func(s channeldb.ShutdownInfo) {
localDeliveryScript = s.DeliveryScript.Val
localShutdownRecords = s.CustomRecords
})
remoteDeliveryScript := chanState.RemoteShutdownScript
// Scan through the close transaction outputs to identify local and
// remote outputs.
var totalOut btcutil.Amount
for _, out := range closeTx.TxOut {
totalOut += btcutil.Amount(out.Value)
switch {
case len(localDeliveryScript) > 0 &&
slices.Equal(out.PkScript, localDeliveryScript):
localCloseOutput = fn.Some(CloseOutput{
Amt: btcutil.Amount(out.Value),
PkScript: out.PkScript,
DustLimit: chanState.LocalChanCfg.DustLimit,
ShutdownRecords: localShutdownRecords,
})
case len(remoteDeliveryScript) > 0 &&
slices.Equal(out.PkScript, remoteDeliveryScript):
// TODO(roasbeef): need to persist remote custom
// records.
remoteCloseOutput = fn.Some(CloseOutput{
Amt: btcutil.Amount(out.Value),
PkScript: out.PkScript,
DustLimit: chanState.RemoteChanCfg.DustLimit,
})
}
}
// Build the AuxCloseDesc.
desc := AuxCloseDesc{
AuxShutdownReq: req,
CloseFee: chanState.Capacity - totalOut,
CommitFee: chanState.LocalCommitment.Fee,
LocalCloseOutput: localCloseOutput,
RemoteCloseOutput: remoteCloseOutput,
}1 participant
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.