tapchannel: support HTLC revocation sweeps #1994
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 enhances the 'tapchannel' functionality by introducing robust support for sweeping revoked HTLC outputs in Taproot Asset channels. It refines the auxiliary sweeper's resolution capabilities to manage various types of revoked HTLCs and adapts the signing path to correctly handle key spend operations required for breach recovery. Furthermore, it optimizes transaction broadcasting during breach events by allowing the system to skip broadcasting for already confirmed transactions, which is crucial for pinning mitigation. 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
|
![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 support for sweeping revoked HTLC outputs in Taproot Asset channels, a crucial feature for channel security. The changes are extensive, primarily affecting the tapchannel package. Key modifications include extending the auxiliary sweeper with resolution logic for various HTLC revocation scenarios and updating the signing path to handle breach scenarios via key-path spends. A skipBroadcast flag has also been added to the NotifyBroadcast interface to handle transactions that are already on-chain, such as justice transactions. The code is generally well-structured and includes detailed comments explaining the complex logic, especially around cryptographic tweaks. I've identified a couple of areas for improvement: one for enhancing code maintainability by reducing duplication, and another to make the HTLC lookup logic more robust. Overall, this is a solid contribution that adds significant functionality.
| // If not found in outgoing, try incoming (accepted HTLCs). | ||
| if len(assetOutputs) == 0 { | ||
| htlcOutputs = commitState.IncomingHtlcAssets.Val | ||
| assetOutputs = htlcOutputs.FilterByHtlcIndex(htlcID) | ||
| } |
There was a problem hiding this comment.
If the HTLC is not found in OutgoingHtlcAssets, the code proceeds to check IncomingHtlcAssets. However, if it's not found there either, assetOutputs will be empty, and the function will proceed without error, likely resulting in an empty resolution blob later on. It would be more robust to return an error if the HTLC cannot be found in either list, to make debugging easier in case of an unexpected state.
if len(assetOutputs) == 0 {
htlcOutputs = commitState.IncomingHtlcAssets.Val
assetOutputs = htlcOutputs.FilterByHtlcIndex(htlcID)
if len(assetOutputs) == 0 {
return lfn.Errf[returnType](
"htlc with id %d not found", htlcID,
)
}
}
There was a problem hiding this comment.
At the very least should log here. Invariants further up the stack should prevent this though.
Pull Request Test Coverage Report for Build 22064145876Details
💛 - Coveralls |
| } | ||
| if signDesc.DoubleTweak != nil { | ||
| if isBreach { | ||
| // Breach scenario: Apply DoubleTweak first, then SingleTweak. |
There was a problem hiding this comment.
Why does the ordering matter here? The operation should be commutative.
There was a problem hiding this comment.
Ah you're right. The append order of PSBT unknowns is indeed irrelevant since the signer identifies them by key type, not position.
|
|
||
| // Revoked second-level HTLC transaction. We sweep this using the | ||
| // revocation path. | ||
| case input.TaprootHtlcSecondLevelRevoke: |
There was a problem hiding this comment.
There aren't two diff enum types for accepted vs offered here?
There was a problem hiding this comment.
Right, LND's TaprootHtlcSecondLevelRevoke doesn't distinguish offered vs accepted (unlike first-level which has separate TaprootHtlcOfferedRevoke/TaprootHtlcAcceptedRevoke). We handle this by trying outgoing assets first, then falling back to incoming. HTLC IDs are unique within a commitment so at most one lookup succeeds. If neither has assets for that HTLC ID, it's a non-asset HTLC and doesn't need asset-level resolution.
| // If not found in outgoing, try incoming (accepted HTLCs). | ||
| if len(assetOutputs) == 0 { | ||
| htlcOutputs = commitState.IncomingHtlcAssets.Val | ||
| assetOutputs = htlcOutputs.FilterByHtlcIndex(htlcID) | ||
| } |
There was a problem hiding this comment.
At the very least should log here. Invariants further up the stack should prevent this though.
|
|
||
| // Now that we have the script tree, we'll make the control block | ||
| // needed to spend it using the revocation path. | ||
| ctrlBlock, err := tweakedScriptTree.CtrlBlockForPath( |
There was a problem hiding this comment.
We should add tests here.
AFIACT, this would fail in prod, as it's using an enum, but that value isn't handled by this func: https://github.com/lightningnetwork/lnd/blob/0b00c662318c4960a0f8f814e16e43155029ca35/input/script_utils.go#L1754-L1771
There was a problem hiding this comment.
Unless we've changed something here, this should also just be a keyspend: https://github.com/lightningnetwork/lnd/blob/0b00c662318c4960a0f8f814e16e43155029ca35/input/script_utils.go#L1782-L1805
There was a problem hiding this comment.
Yeah we should add unit level tests here to ensure that this func is able to sign+verify using the same routines used to create the txns+scripts in the first place.
github-project-automation
Bot
moved this from 🆕 New
to 👀 In review
in Taproot-Assets Project Board
|
Added a gist explaining the 2nd level situation, focused around the sighash of the pre-signed 2nd level HTLC transaction: https://gist.github.com/GeorgeTsagk/9775947b6b9d8cc07cd23e038a246719 Will soon push a version using the |
GeorgeTsagk
force-pushed
the
htlc-revocation
branch
from
2dd5006 to
58bd73f
Compare
|
Breach itest has now been moved here, no need for a Lit PR |
GeorgeTsagk
force-pushed
the
htlc-revocation
branch
from
58bd73f to
732281c
Compare
GeorgeTsagk
force-pushed
the
htlc-revocation
branch
from
732281c to
0016ec2
Compare
GeorgeTsagk
force-pushed
the
htlc-revocation
branch
2 times, most recently
from
cfe3665 to
d5a4ef1
Compare
|
Had to update some of the test assertions Now that we bump |
|
Realized that some of the custom channels itests failures on CI aren't flakes: Some actual test assertions need to change since the HTLC anchor amount was bumped. Will address them asap, shouldn't be a blocker for reviewers. |
gijswijs
left a comment
gijswijs
left a comment
There was a problem hiding this comment.
So I did a thorough review of this PR.
The core cryptographic operations are sound but the surrounding infrastructure needs rework.
I've pointed out all issues with inline comments. I also have some nits wrt the commits (commitnit), which you'll find below:
Typos in commit message for commit d728fae:
The tricky part for taproot assets related to the call sequence of "NotifyBroadcast".
Shouldn't that read "is related"?
we need to signal to the NotifyBroadcast call that we do not wish to broadcast, as that is prone to fail.
prone to fail is an understatement. It will fail and makes no sense whatsoever.
| ctx := context.Background() | ||
| secondLevelTxHash := secondLevelTx.TxHash() | ||
|
|
||
| // Check if already imported. |
There was a problem hiding this comment.
Nice dedup guard here. The same pattern is needed in registerAndBroadcastSweep — it also goes through shipChannelTxn → LogPendingParcel → InsertAssetTransfer (plain INSERT), so duplicate NotifyBroadcast calls after restart will insert duplicate transfer rows. See the existing TODO at line 3048.
| signDesc, vIn, &a.cfg.ChainParams, tapTweak, | ||
| ) | ||
|
|
||
| // In this case, the witness isn't special, so we'll set |
There was a problem hiding this comment.
nit:
I don't like this comment. "special" does a lot of heavy lifting without clarifying what ismeant by it.
// This is a normal scriptspend (not a breach keyspend), so the witness follows the standard structure.
// Set the control block on the leaf script that applySignDescToVIn prepared
| }, | ||
| SuccessTapLeaf: tree.SuccessTapLeaf, | ||
| TimeoutTapLeaf: tree.TimeoutTapLeaf, | ||
| AuxLeaf: tree.AuxLeaf, |
There was a problem hiding this comment.
nit:
Why are we carrying the AuxLeaf value here. It has no actual usage here, it's only there because we are reusing the lnd stuct.
| // IMPORTANT: We must match the creation flow exactly: | ||
| // 1. Create script tree with UNTWEAKED keyring | ||
| // 2. Then apply HTLC index tweak to the tree's internal key | ||
| // | ||
| // During creation, GenTaprootHtlcScript is called with the untweaked | ||
| // keyring, then TweakHtlcTree applies the index tweak. We must do | ||
| // the same here. | ||
| // | ||
| // For TaprootHtlcAcceptedRevoke (htlc.Incoming=true in remote's log), | ||
| // this means incoming to us (they're sending to us). | ||
| // On remote's commitment with them sending, GenTaprootHtlcScript uses: | ||
| // isIncoming && whoseCommit.IsRemote() → SenderHTLCScriptTaproot | ||
| // with parameters: RemoteHtlcKey, LocalHtlcKey (in that order!) | ||
| // where RemoteHtlcKey = sender (them), LocalHtlcKey = receiver (us) |
There was a problem hiding this comment.
ultranit: This comment is way longer than the comment at the same step in htlcOfferedRevokeSweepDesc. Shouldn't both comments be similar?
| // WITHOUT the aux leaf (createSecondLevelHtlcAllocations passes | ||
| // None). The aux leaf only affects the BTC-level on-chain output, | ||
| // not the asset-level script key derivation. | ||
| _ = auxLeaf // Used at BTC level, not needed for asset signing. |
There was a problem hiding this comment.
Why not remove the argument altogether? This does validate my earlier point about carrying the auxLeaf value. At the ASSET-level auxLeaf has no meaning.
| // This is used for importing confirmed second-level HTLC transactions in | ||
| // breach scenarios where exclusion proofs for counterparty wallet outputs | ||
| // cannot be constructed. | ||
| func WithSkipExclusionProofVerification() ProofVerificationOption { |
There was a problem hiding this comment.
Similar comment as with AssumeVerifiedAnnotatedProofs. This is a public function with no enforcement that it's only used for confirmed channel transactions. Could allow importing proofs where the same asset appears in multiple outputs.
Consider requiring a confirmation check parameter, or (in this case possible, not with AssumeVerifiedAnnotatedProofs) consider making this private.
| // the HTLC script keys needed for a valid asset witness. The | ||
| // BTC-level transaction is already confirmed on-chain, which | ||
| // serves as proof of validity. | ||
| return shipChannelTxn( |
There was a problem hiding this comment.
Nothing prevents these proofs from being later re-verified, exported, or served to peers where they would fail verification, right?
| // This mirrors what RawTxInTaprootSignature does | ||
| // internally. | ||
| tapTweak := desc.scriptTree.TapTweak() | ||
| taprootPriv := txscript.TweakTaprootPrivKey( |
There was a problem hiding this comment.
Instead of manually deriving the private key, construct a full virtual packet with both tweak unknowns set via applySignDescToVIn, then call SignVirtualPacket (the actual production signer), and verify the resulting signature against the expected public key. That closes the loop — you'd be testing that the PSBT signer interprets the unknowns the same way applySignDescToVIn intends.
|
|
||
| log.Debugf("signing vPacket for input=%v", | ||
| limitSpewer.Sdump(vIn.PrevID)) | ||
| log.Infof("signing vPacket[%d]: isBreach=%v, "+ |
There was a problem hiding this comment.
Consider logging singleTweak and signingKey at Trace level instead of Info.
| } | ||
| } | ||
| } | ||
| t.Logf("Found %d second-level txns total", len(secondLevelTxns)) |
There was a problem hiding this comment.
secondLevelTxns populated and logged but never asserted. Add require.NotEmpty.
GeorgeTsagk
force-pushed
the
htlc-revocation
branch
from
d5a4ef1 to
50a5441
Compare
GeorgeTsagk
force-pushed
the
htlc-revocation
branch
from
50a5441 to
509d773
Compare
|
Rebased on |
GeorgeTsagk
force-pushed
the
htlc-revocation
branch
from
509d773 to
d2827c6
Compare
|
@Roasbeef: review reminder |
|
Great Job |
GeorgeTsagk
force-pushed
the
htlc-revocation
branch
from
d2827c6 to
f81ab5a
Compare
|
Rebased on latest |
We add the missing cases for resolving contracts that correspond to revoked offered/accepted HTLCs, and revoked HTLCs that have been taken to the second level. For signing, we distinguish between normal and breach scenarios: normal force close sweeps use scriptspend (control block present), while breach sweeps use keyspend (no control block). The breach path is auto-detected by checking if both SingleTweak and DoubleTweak are present in the sign descriptor. We also supply the HTLC index as a single tweak, to be applied by the signer later. This is crucial for matching the taproot internal key that was placed in the commitment during creation.
A short intro: For sweeping revoked states the breach arbitrator of LND crafts 3 different sweep transactions: a) spendAll: spends commitment outputs (local, remote) and all HTLCs b) spendCommitOuts: spends only the commitment outputs (local, remote) c) spendHTLCs: spends only the HTLCs Initially LND broadcasts version a) of the sweep. If that is not confirmed within a few blocks(4) then it will broadcast b) and c). This serves as a pinning attack mitigation. The tricky part for taproot assets related to the call sequence of "NotifyBroadcast". That call finalizes the proofs for the transfer given the txid of the transaction that contains it. Now, we have multiple competing sweeps fighting to get in a block, so we are uncertain about which one is going to make it on-chain. By changing LND into calling NotifyBroadcast after confirming any sweep transaction, we have a way of certainly telling which proofs need to be crafted. Given that the transaction is already confirmed, we need to signal to the NotifyBroadcast call that we do not wish to broadcast, as that is prone to fail. The new skipBroadcast flag serves as that signal, and is set by the caller (LND).
Add a unit test that performs a full sign+verify round-trip for all three revocation sweep descriptor types (offered, accepted, and second-level HTLC). The test generates real key material, derives the revocation signing key using the same routines used in production (DeriveRevocationPrivKey + TweakPrivKey), and verifies the resulting schnorr signature against the taproot output key from the sweep descriptor. This validates that the sweep descriptor functions produce script trees whose taproot output keys are consistent with the private key derivation path, ensuring that breach sweeps will produce valid signatures at runtime.
…mitment caching Introduce the DeterministicHTLCs feature bit (bits 4/5) which gates deterministic second-level HTLC transactions. When negotiated, second-level HTLCs use SigHashDefault instead of SigHashSingle|AnyOneCanPay, and the revoking party includes dual-path AuxSigs in RevokeAndAck for breach proof reconstruction. Also add SigHashType caching in the commitment blob so the sighash choice is recorded alongside the commitment state and can be recovered at breach time without re-querying the feature store.
…HTLCs flag Implement Server.HtlcSigHashType (lnwallet.AuxSigner interface) which checks live feature negotiation state first, then falls back to decoding the commitment blob's cached SigHashDefault flag. Includes a nil guard for the startup race when tapd config is not yet initialized. Thread the DeterministicHTLCs feature flag through the commitment and leaf creation paths: - aux_funding_controller: store flag from negotiated features in pendingAssetFunding and pass through to initial commitment blobs. - aux_leaf_creator: query feature store to determine sighash type for each new commitment, and pass to allocation helpers. - commitment.go: accept sigHashDefault parameter in GenerateCommitmentAllocations and CreateSecondLevelHtlcPackets.
Teach the revoked HTLC recovery flow to sweep second-level outputs back through the aux channel machinery. The sweep/import path can report an already-broadcast pre-anchored transfer more than once, and it can also confirm a successor spend before the exact predecessor asset row has been materialized locally. Without additional DB-side handling those cases strand recovery transfers in pending state even though the BTC spend is already confirmed. Plumb second-level revoked HTLC sweeps through the aux sweeper and make the asset-store side of the porter tolerant of repeated notifications and missing exact predecessor rows by treating pending transfer insertion as idempotent by anchor txid and falling back to a same-asset template row when finalizing a confirmed recovery spend.
Update NotifyBroadcast callers and implementation to use the new AuxNotifyOpts struct from lnd, replacing the bare skipBroadcast bool. SkipBroadcast and SkipProofVerify are now evaluated independently.
Replace placeholder witnesses in importSecondLevelHtlcTx with real asset-level signatures. When AuxSigDesc is available (breach case), sign with our local HTLC key via SignVirtualPacket and insert the remote party's pre-stored signature to produce a full 2-of-2 witness. Try both primary and alternate AuxSig candidates when constructing the witness, since the virtual transaction's spending path may not match the BTC on-chain path. Use the imported proof suffix (with correct inclusion proof, block header, and merkle proof) for the justice sweep descriptor instead of hand-built stubs. When AuxSigDesc is not available, fall back to placeholder witnesses with proof verification skipped.
The default 30-second RPC timeout in lndclient is too short when the integrated daemon restarts after many blocks have been mined while it was offline. The chain sync during GetInfo can easily exceed 30 seconds when catching up on 100+ blocks. Increase to 2 minutes to match production configurations and prevent spurious restart failures during integration tests.
GeorgeTsagk
force-pushed
the
htlc-revocation
branch
2 times, most recently
from
6ca3f29 to
ac03a97
Compare
Add a custom-channel breach integration test that exercises revoked HTLCs which advance to second level before the honest party comes back online and sweeps them. Keep the default scenario small for normal suite runtime, but structure the itest so it can be scaled up to stress the same recovery path. Higher-count variants need additional setup discipline: the pre-breach rebalance must scale with the requested number of in-flight HTLCs, the post-settlement keysend must wait until HTLC cleanup has actually completed, and justice mining must accept the variable set of justice transactions published once more HTLCs reach second level. Make the breach test configurable through TAPD_BREACH_HODL_INVOICES_PER_SIDE while keeping the default at two HTLCs per side.
GeorgeTsagk
force-pushed
the
htlc-revocation
branch
from
ac03a97 to
c556567
Compare
7 participants
Description
This PR adds support for sweeping revoked HTLC outputs in Taproot Asset channels.
The aux sweeper is extended with resolution logic for revoked offered/accepted HTLCs and second-level HTLC revocations. The signing path is updated to handle breach scenarios, where inputs are spent via key spend rather than script spend, with the HTLC index applied as a tweak to match the commitment's taproot internal key.
Additionally, the NotifyBroadcast interface gains a skipBroadcast flag. During breach resolution, LND crafts multiple competing justice transactions as a pinning mitigation. Rather than notifying tapd at time, LND now notifies after confirmation — the flag signals that the tx is already on-chain and proof finalization can proceed without re-broadcasting.
Depends on https://github.com/lightningnetwork/lnd/pull/10583/commits