Extend Aux Blob to breach scenarios#10583
Extend Aux Blob to breach scenarios#10583GeorgeTsagk wants to merge 12 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
GeorgeTsagk
force-pushed
the
aux-htlc-revocation
branch
from
1bb47e1 to
f41b5c3
Compare
lightninglabs-deploy
added
severity-critical
GeorgeTsagk
force-pushed
the
aux-htlc-revocation
branch
2 times, most recently
from
4cd51dc to
5159a8b
Compare
gijswijs
left a comment
gijswijs
left a comment
There was a problem hiding this comment.
Great PR! The commit structure is clean and each commit builds logically on the previous one. The notifyConfirmedJusticeTx + historicJusticeTxs design is elegant.
A few issues I'd like to see addressed before this can be merged. See inline comments.
GeorgeTsagk
force-pushed
the
aux-htlc-revocation
branch
from
5159a8b to
a4f9b0f
Compare
gijswijs
left a comment
gijswijs
left a comment
There was a problem hiding this comment.
Only nits. Answered your comment here: #10583 (comment)
Otherwise LGTM! 🎉
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 - 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
Second-level HTLC auxiliary leaves need to be computed at runtime from the channel state, key ring, and commitment transaction, rather than being stored in the commitment blob. Extend the FetchLeavesFromRevocation interface method to accept these additional parameters so that implementers can derive the leaves on the fly.
When an HTLC is taken to the second level during a breach, the breach arbiter needs to re-resolve the contract with the actual second-level transaction. Preserve the original ResolutionReq as a template in HtlcRetribution so it can be re-used with an updated witness type. Also add SecondLevelTx and SecondLevelTxBlockHeight fields to ResolutionReq so the resolver can re-anchor proofs to the second-level transaction rather than the commitment transaction. Populate HtlcAmt in the resolution request so the resolver has the HTLC value available.
Support revoking second-level HTLC outputs in custom (asset) channels: - Add findSecondLevelOutputIndex to locate second-level outputs by script match rather than relying on SpenderInputIndex, which can diverge from the output index when the spending tx has additional inputs (e.g., wallet UTXOs for fees). - Re-resolve contract blobs at morph time via AuxContractResolver when an HTLC is taken to the second level, using the preserved ResolutionReq template with an updated witness type and the actual second-level spending transaction. - Handle non-positive BTC sweep amounts when an aux output carries the real value, allowing all input BTC to go toward fees. - Wire AuxResolver into BreachConfig from server.go.
Replace the backwards-incompatible CustomBlob.IsSome() check for choosing SigHashDefault with a feature-bit negotiation approach via a new AuxSigner.HtlcSigHashType hook. Add ResolveHtlcSigHashType helper that queries the aux signer for a channel-specific sighash override based on negotiated features, falling back to the default HtlcSigHashType when no aux signer is present or the feature isn't negotiated. Thread the auxSigner through all HTLC second-level transaction signing and validation call sites. This ensures both channel parties agree on the sighash type through explicit feature negotiation rather than an implicit check that could cause sighash disagreement and forced channel closure when only one party has upgraded.
When the channel uses SigHashDefault for HTLC second-level transactions, the sweeper cannot add wallet inputs to pay fees (that would invalidate the peer's signature). Instead, the fee must be baked into the pre-signed transaction at commitment signing time. Add sigHashDefaultFeeRate (3x FeePerKwFloor = 759 sat/kw) and thread a sigHashDefault bool through HtlcTimeoutFee, HtlcSuccessFee, HtlcIsDust, CommitmentBuilder, and all call sites in channel.go, htlcswitch/link.go, and the HTLC resolution construction paths. This results in: - Timeout tx fee: 759 * 645 / 1000 = 489 sat - Success tx fee: 759 * 705 / 1000 = 535 sat The 3x multiplier ensures the pre-signed tx clears the mempool minimum even when nodes compute fee rate against raw serialized size rather than virtual size. Add an IsSigHashDefault convenience helper and IsChanSigHashDefault / isSigHashDefault methods on LightningChannel. For next-commitment call sites (genRemoteHtlcSigJobs, genHtlcSigValidationJobs), extract the sigHashReq into a local variable so it can be shared between ResolveHtlcSigHashType and IsSigHashDefault. Use chanState.LocalCommitment.CustomBlob as the CommitBlob source (instead of the in-memory commitment view blob) since the persisted commitment already carries the SigHashDefault flag. NewBreachRetribution now takes an AuxSigner option so it can resolve the sighash type when computing dust thresholds for HTLC retribution data.
…ault When the second-level HTLC transaction was signed with SigHashDefault (baked-in fees), the sweeper's normal tx-rebuilding flow would invalidate the peer's signature. Add isSigHashDefault() and publishTimeoutTx() / publishSuccessTx() to both the timeout and success resolvers, which bypass the sweeper and broadcast the pre-signed transaction directly. The Launch() method now checks isSigHashDefault() before falling through to the sweeper-based sweepTimeoutTx / sweepSuccessTx paths.
The breach arbiter rebuilds justice tx variants after each spend detection (e.g. when an HTLC transitions to second-level). The tx that ultimately confirms may have been created in an earlier rebuild cycle and is no longer present in the current justiceTxVariants struct. Add a historicJusticeTxs map that records every justice tx variant ever created (keyed by txid) via recordJusticeTxVariants(). The notifyConfirmedJusticeTx function now falls back to this map when the confirmed spend doesn't match any current variant, ensuring the aux sweeper receives NotifyBroadcast for asset proof generation. Also improve the split-broadcast path: rebuild justice tx variants from the updated breach info before splitting, and re-attempt the spendAll variant first (which may now succeed after second-level spends have been incorporated). Add logging to createJusticeTx for input counts and variant creation.
The ErrTweakOverdose check rejected SignDescriptors with both SingleTweak and DoubleTweak set during deserialization. This was a placeholder guard that no caller ever checked for or handled. With taproot asset channels, the combined tweak path (DoubleTweak for revocation + SingleTweak for HTLC index) is exercised during asset-level signing. While the current code only sets both tweaks on transient copies that are never persisted, removing the check eliminates a latent footgun if a future change were to persist such descriptors.
GeorgeTsagk
force-pushed
the
aux-htlc-revocation
branch
from
a4f9b0f to
5416611
Compare
|
@GeorgeTsagk, remember to re-request review from reviewers when ready |
4 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