tapfreighter: add SendStateVerifyPreBroadcast and supporting proof reanchor helpers
#1884
Conversation
Summary of ChangesHello @ffranr, 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 improves the robustness of asset transfers by implementing an early validation step for input proofs within the 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 introduces a valuable pre-broadcast validation step for asset sends, enhancing the robustness of the ChainPorter by verifying input proofs before the anchor transaction is broadcast. The refactoring of proof-handling logic into new helpers like reanchorAssetOutputs and updateProofsFromShortChanID improves code structure and maintainability. My review includes a critical comment regarding a temporary dependency in go.mod that should be removed, along with suggestions to address code duplication and a misleading comment for better clarity and maintainability.
ffranr
force-pushed
the
wip/add-chainporter-validation/add-proof-validation-state
branch
from
21585ab to
e966327
Compare
Introduce a new state in the ChainPorter state machine to perform pre-broadcast validation on the send package. Currently, it validates only input proofs. Additional validation steps will be added in the future.
Pre-anchored send packages now begin in the SendStateVerifyPreBroadcast state instead of SendStateStorePreBroadcast. This ensures that pre-broadcast validation is performed before storing the package or anchor transaction broadcast.
- Add updateProofsFromShortChanID to populate transition proofs using a channel's funding outpoint (block hash, txid, tx index). - Share block lookup logic via ChainBridge to avoid redundant code in each caller. - Use the helper in aux sweeper and closer to populate funding proofs consistently.
Add TaprootOutputScript method to the Proof type to derive the Taproot output script and taproot key anchoring the proof using its inclusion path.
ffranr
force-pushed
the
wip/add-chainporter-validation/add-proof-validation-state
branch
from
e966327 to
70e3645
Compare
Pull Request Test Coverage Report for Build 19735582954Details
💛 - Coveralls |
|
LiT CI job is failing because litd will also need LND bump to lightningnetwork/lnd#10391 |
ffranr
force-pushed
the
wip/add-chainporter-validation/add-proof-validation-state
branch
from
70e3645 to
c4ddbf6
Compare
jtobin
left a comment
jtobin
left a comment
There was a problem hiding this comment.
Another great add. 🙏 LGTM modulo the requisite LND bump and matching replace.. uh, replacement.
I also wound up putting together an itest case while reviewing this that might be worth pulling in (see the linked PR).
- Update lnd dependency to a version that includes the new CommitTxBlockHeight field in the ResolutionReq struct.
Introduce reanchorAssetOutputs to centralize the logic for anchoring proofs to the actual commitment transaction. This function sets the correct output index in the inclusion proof by matching the asset's Taproot output in the transaction. Used to keep proof reanchoring consistent across aux_sweeper and aux_closer by reusing a shared anchoring path.
ffranr
force-pushed
the
wip/add-chainporter-validation/add-proof-validation-state
branch
from
c4ddbf6 to
815021f
Compare
jtobin
left a comment
jtobin
left a comment
There was a problem hiding this comment.
LGTM. 👍
(I clocked the associated itest as taking 23s to run on my M4 MacBook Air, so probably longer in CI -- probably not worth pulling in purely as a regression defence.)
This PR adds a new pre-broadcast validation step to the
ChainPorterso that input proofs on pre-anchored send packages are validated before the anchor transaction is broadcast. Broadcasting is a non-reversible action, so we now fail early when a package carries invalid input proofs.SendStateVerifyPreBroadcastto run input-proof validation before broadcasting the anchor transaction.SendStateStorePreBroadcast.reanchorAssetOutputs) to anchor proofs to the actual commitment transaction.updateProofsFromShortChanID,Proof.TaprootOutputScript) to populate and derive the data required for correct input-proof construction.ChainBridge.GetBlockByHeightpath to avoid per-caller duplication.Notes
CommitTxBlockHeighttoResolutionReqlightningnetwork/lnd#10391