Extend Aux Blob to breach scenarios#10583
Extend Aux Blob to breach scenarios#10583GeorgeTsagk wants to merge 6 commits intolightningnetwork:masterfrom
Conversation
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 significantly improves the handling of channel breaches within the Lightning Network by extending the auxiliary channel contract framework. It introduces mechanisms to correctly process HTLC outputs during breach scenarios, ensuring that all funds are properly swept. Key changes include populating resolution blobs for HTLCs, passing crucial breach commitment height information to external resolvers, and refining the aux sweeper notification process to only trigger upon on-chain confirmation of justice transactions. This ensures more robust and reliable breach recovery, particularly for custom channel types. 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. Changelog
Activity
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
|
lightninglabs-deploy
added
the
severity-critical
GeorgeTsagk
force-pushed
the
aux-htlc-revocation
branch
from
21df3f7 to
1bb47e1
Compare
![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 extends the auxiliary channel contract framework to handle HTLC outputs in breach scenarios. The changes include populating resolution blobs for HTLC retribution, passing breach commitment height to HTLC resolution requests, and deferring auxiliary sweeper notifications until justice transactions are confirmed. The logic appears sound, and the changes are well-tested. I've made a few minor suggestions to improve code clarity and correctness, particularly around handling combined tweaks and clarifying test comments.
|
|
||
| // Should not panic and should not mark as notified since there's no |
There was a problem hiding this comment.
This comment is misleading. The implementation of notifyConfirmedJusticeTx does mark the transaction as notified even if there is no aux sweeper, and the subsequent comment and test assertion confirm this is the desired behavior. To avoid confusion, I suggest removing this comment block.
| // maybeTweakPrivKey examines the single and double tweak parameters on the | ||
| // passed sign descriptor and may perform a mapping on the passed private key | ||
| // in order to utilize the tweaks, if populated. | ||
| // in order to utilize the tweaks, if populated. If both tweak parameters are | ||
| // set, then both are applied in the following order: | ||
| // | ||
| // a) double tweak | ||
| // b) single tweak |
There was a problem hiding this comment.
The function comment has been updated to describe the case where both SingleTweak and DoubleTweak are present. According to the new logic, the double tweak is applied first, then the single tweak. It would be clearer to explicitly state this order in the comment for better maintainability.
// maybeTweakPrivKey examines the single and double tweak parameters on the
// passed sign descriptor and may perform a mapping on the passed private key
// in order to utilize the tweaks, if populated. If both tweak parameters are
// set, then they are applied in the order of the double tweak, then the
// single tweak.
//
// a) double tweak
// b) single tweak
| spendingTxHash := *s.detail.SpenderTxHash | ||
|
|
||
| // Skip if we've already notified about this transaction. | ||
| if notifiedTxs[spendingTxHash] { |
There was a problem hiding this comment.
Where will this state be stored/maintained?
There was a problem hiding this comment.
The map is in-memory only and won't survive restarts. On restart, exactRetribution re-enters the spend detection loop and could call NotifyBroadcast again for the same tx.
Will address this on the tapd side by making sure NotifyBroadcast is idempotent / reentrancy-safe
| // so we pass skipBroadcast=true. | ||
| return aux.NotifyBroadcast( | ||
| &bumpReq, s.detail.SpendingTx, | ||
| justiceCtx.fee, nil, true, |
There was a problem hiding this comment.
All values should be populated here.
There was a problem hiding this comment.
outpointToTxIndex is only populated for inputs with a non-nil RequiredTxOut() (seem to be pre-signed SINGLE|ANYONECANPAY second-level HTLC txs)
For breach sweeps breachedOutput.RequiredTxOut() always returns nil since we sign fresh with the revocation key
am I missing something?
There was a problem hiding this comment.
or were you referring to the bumpReq fields?
|
|
||
| // Track which justice transactions we've already notified the aux | ||
| // sweeper about, to avoid duplicate NotifyBroadcast calls. | ||
| notifiedJusticeTxs := make(map[chainhash.Hash]bool) |
There was a problem hiding this comment.
How can we have duplicate notifications if only one version can confirm? You have some re-org scenario in mind?
There was a problem hiding this comment.
The dedup map is needed because waitForSpendEvent registers a separate RegisterSpendNtfn for each breached output. When a single justice tx (e.g. spendAll) confirms and spends all N outputs, the chain notifier delivers a SpendDetail to each subscription independently.
You can verify by running the itest revoked_uncooperative_close_retribution_remote_hodl and grep'ing for "Detected spend on".
Without it we'd be calling NotifyBroadcast multiple times
Regarding re-orgs:
with the recent N-block confirmation delay changes in chain_watcher, spends reaching this code path are already N-blocks deep, so re-org-triggered duplicates shouldn't be a practical concern here.
|
@gijswijs: review reminder |
Previously we'd define either a single or a double tweak for the sign descriptor. We introduce the option to apply both consecutively (double tweak first, single tweak second) if both tweak parameters are set. For callers who define only one of the two parameters we maintain the old behavior.
Pass the breach height through the call chain from NewBreachRetribution to createHtlcRetribution, and include it in the ResolutionReq for HTLC outputs via CommitTxBlockHeight. This is required for taproot-assets to properly reanchor asset proofs when sweeping revoked HTLCs. The reanchorAssetOutputs function needs the block height to fetch the block containing the breach commitment transaction and construct valid proofs for the sweep. Without this, the CommitTxBlockHeight defaults to 0, causing tapd to look for the commitment tx in the genesis block and fail with: "commit tx not found in block". See: lightninglabs/taproot-assets@815021fd (tapchannel: add reanchorAssetOutputs for proof reanchoring)
Add unit tests for the notifyConfirmedJusticeTx function to verify correct behavior when detecting confirmed justice transactions and notifying the aux sweeper. Test cases cover: - Detection of each justice tx variant (spendAll, spendCommitOuts, spendHTLCs) - Skipping already-notified transactions via notifiedTxs map - Skipping unconfirmed spends (SpendingHeight == 0) - No notification for unrelated transactions - Multiple spends with mixed matching/non-matching - Graceful handling of nil justice tx contexts - Operation without an aux sweeper configured
GeorgeTsagk
force-pushed
the
aux-htlc-revocation
branch
from
1bb47e1 to
f41b5c3
Compare
lightninglabs-deploy
added
severity-critical
3 participants
Description
This PR extends the auxiliary channel contract framework to properly handle HTLC outputs during breach scenarios.
Currently, the aux sweeper infrastructure handles commitment output revocations but lacks the plumbing needed for HTLC outputs. This fills in those gaps by:
Based on #10377