asset: fix split witness writeback

[bhandras]

UpdateTxWitness mutates a pointer into a copied root asset when a split commitment is present, so the updated TxWitness never propagates back into SplitCommitment.RootAsset. That leaves the root witness empty and produces invalid split proofs/transactions.

To mitigate this issue we write the updated witness back into the stored root asset when handling split commitments, and add a regression test that demonstrates the previous failure and now passes.

[gemini-code-assist]

Summary of Changes

Hello @bhandras, I'm Gemini Code Assist¹! 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 resolves a critical bug in the asset management system where transaction witness updates for assets involved in split commitments were not correctly persisted. The previous implementation mutated a copy of the root asset, causing the updated witness to be lost. The fix ensures proper propagation of witness changes back to the root asset, thereby preventing the generation of invalid split proofs and transactions. A new regression test has been added to confirm the correct behavior.

Highlights

• Witness Propagation Fix: Corrected an issue where UpdateTxWitness failed to propagate updated transaction witnesses back to the SplitCommitment.RootAsset when a split commitment was present, leading to invalid proofs.
• Explicit Witness Writeback: Implemented an explicit writeback mechanism within UpdateTxWitness to ensure that the modified TxWitness is correctly stored in the SplitCommitment.RootAsset.
• Regression Test Added: Introduced TestUpdateTxWitnessSplitCommitment to validate the fix and prevent future regressions, demonstrating that witnesses are now properly updated in split commitment root assets.

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 👍 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

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. ↩

[gemini-code-assist]

Code Review

This pull request correctly fixes a bug where witness updates were not being propagated to the root asset in a split commitment. The fix ensures the updated witness is written back correctly. The addition of a regression test is a great way to prevent this issue from recurring. I've added one suggestion to simplify the implementation of the fix, making it more direct and efficient.

[bhandras]

@gemini-code-assist pls re-review

[gemini-code-assist]

Code Review

This pull request effectively addresses a critical bug where witness updates were not being correctly propagated for assets with split commitments. The fix in asset.go is direct and correct, preventing the mutation of a copied struct. The addition of a regression test in asset_test.go is excellent, as it clearly demonstrates the previous failure and ensures this issue won't reappear. I've left a couple of minor suggestions: one to improve test readability by adding comments as per the style guide, and another to fix a typo in the release notes.

[bhandras]

@gemini-code-assist pls re-review

[gemini-code-assist]

Code Review

This pull request correctly addresses a bug where witness updates for assets with split commitments were not being persisted due to modifications being made on a copy of the root asset. The fix involves directly updating the witness on the nested RootAsset within the SplitCommitment. A regression test has been added, which effectively validates the fix.

My review includes a suggestion to add a comment for clarity in asset/asset.go and points out a minor typo in the release notes. Overall, the changes are solid and well-tested.

[bhandras]

@gemini-code-assist pls re-review

[gemini-code-assist]

Code Review

This pull request provides a solid fix for a critical bug where witness updates for split commitments were not being persisted correctly. The logic now correctly writes the witness back to the root asset. The addition of a targeted regression test is excellent, ensuring this issue does not reappear. The release notes have also been updated to reflect the change. I have one minor suggestion regarding formatting in the release notes for consistency.

[bhandras]

@gemini-code-assist pls re-review

[gemini-code-assist]

Code Review

This pull request correctly fixes a bug where witness updates were not being persisted for assets with a split commitment. The issue was due to modifying a copy of the root asset instead of the original. The fix directly updates the witness on the original root asset, and the logic is sound. A comprehensive regression test has been added to prevent this issue from recurring. I have one minor suggestion to improve readability.

[coveralls]

Pull Request Test Coverage Report for Build 19904484275

Details

• 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
• 57 unchanged lines in 14 files lost coverage.
• Overall coverage increased (+0.009%) to 56.617%

---

Files with Coverage Reduction	New Missed Lines	%
fn/context_guard.go	1	91.94%
asset/group_key.go	2	72.15%
mssmt/compacted_tree.go	2	77.65%
tapdb/mssmt.go	2	90.45%
itest/assertions.go	3	87.42%
itest/multisig.go	3	97.94%
tapchannel/aux_leaf_signer.go	3	43.53%
rpcserver.go	4	61.49%
tapdb/multiverse.go	4	80.16%
tapdb/universe.go	4	80.81%

Totals
Change from base Build 19823124678:	0.009%
Covered Lines:	64841
Relevant Lines:	114525

---

💛 - Coveralls

[bhandras]

@gemini-code-assist pls re-review

[gemini-code-assist]

Code Review

This pull request effectively resolves a critical bug where witness updates for split commitments were not persisted. The issue stemmed from updating a copy of the root asset instead of the original. The fix correctly modifies the witness by operating on a pointer to the root asset within the split commitment. A comprehensive regression test is included, which validates the fix by ensuring the witness is written back to the correct location. The release notes have also been updated accordingly. The changes are well-implemented and adhere to the project's coding standards.

[jtobin]

LGTM, nice catch. 👍 👍

[ffranr]

Great find, thanks. I’m going to add verification logic to ChainPorter to harden it against this bug. We will also add better lit custom channels itest coverage.