tapchannel: no passive assets in funding proofs#1943
tapchannel: no passive assets in funding proofs#1943jtobin merged 2 commits intolightninglabs:mainfrom
Conversation
Summary of ChangesHello @jtobin, 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 addresses a bug that led to passive assets being incorrectly incorporated into channel funding proofs. The changes introduce a new utility function to accurately identify funding-related virtual outputs and update the proof generation logic to ensure only these active funding outputs are included, thereby preventing the erroneous inclusion of passive assets. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 addresses a bug where passive assets were incorrectly included in channel funding proofs. The fix introduces a new helper function, isFundingOutput, which correctly identifies outputs destined for the funding transaction by checking that the output type is TypeSimple and the anchor output index is 0. This function is now used in anchorVPackets, making the logic clearer and more robust. The corresponding release notes have also been updated. The changes are correct and improve the codebase.
Pull Request Test Coverage Report for Build 21032103406Details
💛 - Coveralls |
ffranr
left a comment
ffranr
left a comment
There was a problem hiding this comment.
If we have time, i think it would be good to add test coverage for this. Some possibilities:
- Unit-level: add a test around anchorVPackets in tapchannel/aux_funding_controller.go that builds vPackets with:
- one TypeSimple output anchored to index 0 (funding),
- one or more TypeSimple outputs anchored to index >= 1 (passive),
- optional split-root/change output.
Assert: - fundingProofs only contains proofs for AnchorOutputIndex == 0,
- every output still has a ProofSuffix (passive proofs are still created).
- Integration: end-to-end channel funding with passive assets present:
- responder accepts funding proofs,
- passive assets are still recoverable locally after the anchor tx confirms (proofs imported to local archive),
- no change in behavior when there are no passive assets.
| // Only include outputs destined for the funding output | ||
| // (index 0). Passive assets go to separate anchor | ||
| // outputs and should not be included in funding proofs. | ||
| if isFundingOutput(vPkt.Outputs[vOutIdx]) { |
There was a problem hiding this comment.
What was the next effect of this? Invalid proofs?
In my mind this could only happen if we had an exactly sized input (enough to pay for fees, etc) and we did a full value fund (entire asset amount utilized). In other words, we should always have a change output where the passive assets would live.
There was a problem hiding this comment.
No invalid proofs; the effect observed in the wild was the one described in #1932, i.e. that displayed channel capacity was inflated by passive assets that weren't filtered out during funding. It's IMO entirely possible that other strange problems could manifest from this.
I've since made lightninglabs/lightning-terminal#1198, which adds an itest for this. I can confirm that it fails locally without the fix here, and passes when the fix is applied. 👍 |
tapchannel/aux_funding_controller.go
Outdated
| // transfer. | ||
| if vPkt.Outputs[vOutIdx].Type == tappsbt.TypeSimple { | ||
| // Only include outputs destined for the funding output | ||
| // (index 0). Passive assets go to separate anchor |
There was a problem hiding this comment.
IMO better not to hardcode the index 0 in the comment
github-project-automation
bot
moved this from 🆕 New
to 👀 In review
in Taproot-Assets Project Board
Previously, anchorVPackets included all "TypeSimple" outputs in the funding proofs sent to a responder. Passive assets (viz. those located in the same UTXO, but not selected for funding) also have default output type TypeSimple, which caused them to be included incorrectly. The fix is to also check that AnchorOutputIndex == 0, since passive assets are always assigned to anchor outputs at index >= 1.
github-project-automation
bot
moved this from 👀 In review
to ✅ Done
in Taproot-Assets Project Board
tapchannel: no passive assets in funding proofs (cherry picked from commit 66105b3)
5 participants
This is a copy of #1940, which was tombstoned by GitHub for some reason.