tapchannel: address fault tolerance gap for initiator during channel funding #1898
Conversation
Previously, the funding output proofs were only imported during force close handling if we were the channel responder. However, this could lead to issues if the channel initiator failed to properly import the funding proofs after the funding transaction confirmed. This change removes the responder-only check and always attempts to import the funding output proofs during force close handling. This ensures that both parties can properly recognize and spend the funding outputs regardless of any prior import failures.
Now that we always attempt to import funding output proofs during force close handling (not just for responders), we may encounter proofs that have already been imported. This change adds an early check using HasProof to detect already-imported proofs and skip the expensive work of fetching from the courier, decoding, and re-importing. The fundingLocator is now constructed early in the loop and reused for both the existence check and the final ImportProofs call, eliminating redundant locator construction.
Roasbeef
requested review from
a team,
GeorgeTsagk and
darioAnongba
and removed request for
a team
Summary of ChangesHello @Roasbeef, 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 significantly enhances the robustness of the channel funding process by ensuring that critical funding proofs are always imported, even in scenarios where the initiating node might crash before completing the import. By removing a conditional import logic and introducing a check for already existing proofs, the system becomes more resilient to transient failures, preventing potential issues with unrecorded funding outputs and improving overall system reliability. 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
|
![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 correctly addresses a fault tolerance gap for the channel initiator by ensuring funding proofs are always imported in the auxiliary sweeper. The addition of an existence check to prevent duplicate proofs is a necessary and well-implemented safeguard for this change. The code is clean, and the refactoring improves maintainability. I've found one potential issue regarding a panic on an empty slice, which should be addressed.
| err = importOutputProofs( | ||
| req.ShortChanID, maps.Values(fundingInputProofs), | ||
| a.cfg.DefaultCourierAddr, a.cfg.ProofFetcher, | ||
| a.cfg.ChainBridge, vCtx, a.cfg.ProofArchive, | ||
| ) | ||
| if err != nil { | ||
| return fmt.Errorf("unable to import output "+ | ||
| "proofs: %w", err) | ||
| } |
There was a problem hiding this comment.
The importOutputProofs function logs the first element of outputProofs without checking if the slice is empty. If fundingInputProofs is an empty map, maps.Values will produce an empty slice, causing a panic. It's safer to check if there are any proofs to import before calling the function.
outputProofs := maps.Values(fundingInputProofs)
if len(outputProofs) > 0 {
err = importOutputProofs(
req.ShortChanID, outputProofs,
a.cfg.DefaultCourierAddr, a.cfg.ProofFetcher,
a.cfg.ChainBridge, vCtx, a.cfg.ProofArchive,
)
if err != nil {
return fmt.Errorf("unable to import output "+
"proofs: %w", err)
}
}There was a problem hiding this comment.
should be non-empty by assertion right? I personally don't like extra validation for things that should never happen by definition but I'm not sure if fundingInputProofs can be empty.
Pull Request Test Coverage Report for Build 19913768812Details
💛 - Coveralls |
GeorgeTsagk
left a comment
GeorgeTsagk
left a comment
There was a problem hiding this comment.
LGTM
missing release note (or tag to skip)
also the gemini feedback is worth checking
darioAnongba
left a comment
darioAnongba
left a comment
There was a problem hiding this comment.
LGTM, small comment on the Gemini review.
github-project-automation
bot
moved this from 🆕 New
to 👀 In review
in Taproot-Assets Project Board
5 participants
In this PR, we address a fault tolerance gap in the current logic. Today the initaitor will attempt to import the proofs for the funding outputs (called funding inputs at times) after we send our last message to
lnd. However, if we crash right before that, as the funding aux controller doesn't have any persistence of its own (not a proper durable state machine), then we may never import these proofs.In the aux sweeper, we currently skip importing the funding outputs under the assumption that we already imported then during the funding process. However this is a faulty assumption, based on the above.
We fix this by always importing the funding proofs in the aux sweeper. We then add an existence check to make sure we don't end up with duplicate proofs, etc.