tapchannel: add awareness of 1st and 2nd level HTLC sweeps to the AuxSweeper #1154
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
21 tasks
Roasbeef
requested review from
a team,
jharveyb and
GeorgeTsagk
and removed request for
a team
|
Will circle back to clean up the commit structure, and add more unit tests. |
guggero
reviewed
Nov 11, 2024
| initiator bool, htlcOutputs []*cmsg.AssetOutput, htlcAmt btcutil.Amount, | ||
| commitCsvDelay uint32, keys lnwallet.CommitmentKeyRing, | ||
| outputIndex fn.Option[uint32], | ||
| ) ([]*Allocation, error) { |
| ) ([]*Allocation, error) { | ||
|
|
||
| // TODO(roasbeef): thaw height not implemented for taproot chans rn | ||
| // (lease expiry) |
There was a problem hiding this comment.
Should we not leave the code block with HasLeaseExpiration() in, so once we have a channel type that supports the lease expiry with taproot (assets) channels, this would work already? Or would there be more changes needed?
There was a problem hiding this comment.
IIUC the only gap to lease expiration (from the PoV of tapd) is that sometimes the settled commitment output also has a CLTV. So that should just manifest as setting the CLTV in the sweep vPkt, then generating the correct script based on the bool of if it's leased enforced or not.
| ctrlBlockBytes: ctrlBlockBytes, | ||
| }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Looks like something doesn't compile here (probably a rebase artifact).
Roasbeef
force-pushed
the
htlc-resolution
branch
from
7cf893c
to
e2327b7
Compare
Roasbeef
force-pushed
the
htlc-resolution
branch
from
e2327b7
to
c37e8cb
Compare
|
Pushed up a new set of commits that should compile at each point. Next push will revise them to reduce the number, restructure, and consolidate in-PR bug fixes into their OG commit. |
Pull Request Test Coverage Report for Build 11809952970Details
💛 - Coveralls |
GeorgeTsagk
reviewed
Nov 12, 2024
There was a problem hiding this comment.
cACK
would be nice to have some unit coverage as well
will now work with the litd itest to understand at a better depth what exactly is going on
| // output, we're pretty much always able to sweep it. | ||
| // | ||
| // TODO(roasbeef): return the sats equiv budget of the asset | ||
| // amount |
There was a problem hiding this comment.
wdym: the guarantee of this input being swept would scale relative to the assets value?
There was a problem hiding this comment.
So let's say we have a 345 satoshi output, but it actually anchors $10k. We should be willing to spend up to some % of the USD value, expressed in BTC, on fees.
This current logic is a stop gap though, as we'd need to query the oracle to figure out how much the anchored asset is actually worth in BTC.
Roasbeef
force-pushed
the
htlc-resolution
branch
5 times, most recently
from
1435ef3
to
41f691a
Compare
|
Alright I've managed to chop things down from 36 to 20 commits! This is ready for a proper pass now. |
Roasbeef
force-pushed
the
htlc-resolution
branch
from
41f691a
to
6e1ff61
Compare
In this commit, we expand the fields of the `ContractResolution` struct to account for second level sweeps. When we have an HTLC that needs to go to the second level, we'll need to create 2 vPkts: one for a direct spend from the commitment txns with the second level txn, and the other for the spend from the sweeping transaction. We can construct that transaction early, but can only sign for it once we know the sweeping transaction. Therefore, we need to keep some extra data for a given resolution, namely the tapTweak and the control block.
…kets This'll be useful later when we need to generate the vPkt for second level spends.
In this commit, we add two fields to the tapscriptSweepDesc struct related to 2nd level HTLCs. First, we add the second level sig index, which is needed to be able to know where to insert our signature after we generate it. We also add an optional lnwallet.AuxSigDesc, which contains the remote party's signature, and also the sighash needed for it and a sign desc to generate it with.
In this commit, we add sweep desc generation for remote HTLCs sweeps. This is needed when the remote party force closes, and we can spend the HTLC directly from their commitment transaction.
In this commit, we add `blobWithWitnessInfo` which couples the raw resolution TLV blob with some additional information such as the input that carried the blob, and also preimage information. Preimage information is needed to know where to insert the preimage in the witness once we learn about it on chain.
In this commit, we update `createSweepVpackets` to be second level aware. This means that if we detect an auxSigInfo, then we know the vPkt we need to create is actually just the second level vPkt. We also don't need to generate a new script key either. Finally, we'll properly set the absolute delay if the sweep desc indicates as such.
In this commit, we update `signSweepVpackets` to be 2nd level HTLC aware. If we have an auxSigDesc, then that means we're spending a 2nd level HTLC txn. To spend this properly, we'll need to insert the remote party's signature at the proper location, as it's a 2-of-2 multi-sig.
In this commit, we make sure to pass in the correct signDesc when we go to sign for a second level txn. For a 2nd level txn, we'll actually use the signDesc that's needed to generate the 2-of-2 multi-sig, instead of the one that we'd normally use to sweep.
With all the prior commits in place, we can now create the new contract resolution that includes the 1st and 2nd level packets, and the information that we'll need to re-sign the second level packets later.
In this commit, we add some intermediate functions and types needed to properly handle HTLC sweeps. We now use the preimage info added in a prior commit to make sure that once we learn of the preimage (after vPkt creation, but before publish), we'll properly insert it in the correct location. In sweepContracts, when we go to create the new anchor change output, we'll make sure to omit any second level transactions, as they already have a destination location specified. Direct spends are spends directly from the commitment transaction. If we don't have any direct spends, then we don't need to make a change addr, as second level spends are already bound to an anchor output.
In this commit, we update `registerAndBroadcastSweep` to be able to handle HTLC sweeps. First, we only need to set the anchor for first level sweeps. We weren't able to sign the 2nd level sweeps earlier as we didn't know the txid of the transaction that swept them. Now that we're about to broadcast, we know the sweeping transaction so we can set the prevID properly, then sign the sweeping transaction. If the sweeper is broadcasting _just_ a second level txn with an asset, then we won't have an extra change output. This transaction also already has its internal key bound.
If we don't do this here, then we'll sign a second level success transaction that doesn't have the sequence set properly. This will then propagate all the way up to anchor output, leading to an incorrect inclusion proof later.
In this commit, we fix an existing logic gap related to 2nd level HTLCs. Before if we were signing a timeout for the remote party, neither of us would properly apply the lock time to the vPkt. In this commit, we fix it by first gaining a new `whoseCommit` param in `FetchLeavesFromCommit`. We then use that to decide when to use a non zero CLTV timeout.
In this commit, we fix a very subtle issue related to split witnesses and root assets. Before this commit, when we went to sign a second level txn, which is a split, we would create a virtual txn, which will commit to the _entire_ witness of the root asset in the vIn. Note that this bypasses the normal segwit encoding logic, as that currently doesn't apply to the root asset encoded in the split commit proof. In the future, we may want to expand the segwit encoding semantics to root assets after careful consideration. In this commit, we fix a bug that would cause an invalid 2nd level sig by ensuring that before we create the output commitments, we update the root asset witness in each split, to the funding witness script, which is just OP_TRUE.
In this commit, we fix an issue that would cause lnd to not broadcast sweeps of HTLCs due to their small value. Before we didn't add enough budget to actually convince lnd to init the fee function and broadcast the sweep. In the future, we should do a more careful calculation here based on the current BTC value of the asset, to make an economical decision. For now, we just increase the amt based on the amt of inputs we have. This gives lnd a fee budget to use for bumping.
In this commit, we update the logic of `notifyBroadcast` to use the new `outpointToTxIndex` map. We'll use this to make sure that the vOut has the correct anchor output index, as the sweeper applies a sorting routine before broadcast. Otherwise, we'll have invalid inclusion proofs.
If we don't import these script keys, spends will fail later as we didn't credit the balance, so we can't spend them.
Roasbeef
force-pushed
the
htlc-resolution
branch
from
6e1ff61
to
cb4f1f1
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In this PR, we add awareness of 1st and 2nd level HTLC sweeps to the
AuxSweeper.For 1st level sweeps (HTLC on the remote party's commitment), the logic is straight forward. We just make a new sweep desc, and create the vPkts as normal. The only new logic is that if this is an incoming HTLC success, then we'll learn the preimage after the vPkt is created. As a result, we'll need to insert the pre-image into the right location in the witness once it's known.
For 2nd level sweeps (HTLC on the local party's commitment), the logic is a bit more involved. First, we'll need to make vPkts for both the first and the second level sweeps. However we can't sign the vPKts yet (we don't have no_input), since lnd's sweeper can aggregate the HTLC sweeps, so we can only know the true outpoint once things are confirmed. In addition, we also need to insert the remote party's signature into the witness after we sign for it, using a distinct sign desc. Finally, when we go to create the anchor output for the sweeps, we must omit any second level sweeps, as the destination is already locked in by the remote party's signature.