lnwallet: apply BIP69+CLTV tie-break to HTLC signature order#4121
Merged
Conversation
cfromknecht
force-pushed
the
sort-htlc-sigs
branch
4 times, most recently
from
9484d72 to
143f32d
Compare
wpaulino
approved these changes
Mar 27, 2020
halseth
suggested changes
Mar 30, 2020
cfromknecht
force-pushed
the
sort-htlc-sigs
branch
from
143f32d to
ab65bed
Compare
cfromknecht
force-pushed
the
sort-htlc-sigs
branch
from
ab65bed to
ee93cc6
Compare
Contributor
Author
Fixed |
Roasbeef
requested changes
Apr 1, 2020
Member
Roasbeef
left a comment
Roasbeef
left a comment
There was a problem hiding this comment.
The diff looks solid, other than some questions I have on the first commitment, it isn't yet clear to me that some of these changes are actually required. I may very well be missing something though.
We currently write each HTLCs OutputIndex to disk, but we don't use it when restoring. The restoration is modified to use these directly, since we will have lost access to the sorting of CLTVs after the initial signing process.
This commit fixes lightningnetwork#4118 by properly sorting the HTLC signatures sent over the wire to match the BOLT3 BIP69+CLTV sorting of the commitment outputs. To do so, we expose the slice of cltv deltas for HTLCs on the unsigned commitment after applying the commitment sorting. This will be used to locate the proper output index, as the CLTV serves as a tie breaker between HTLCs that otherwise have the same payment hash and amount. Note that lightningnetwork#3412 fixed the issue partially by ensuring the commitment was constructed properly (and the second-level prev outpoint's txid was correct), but failed to address that the HTLC signatures were still sent out in the incorrect order. With this, we pass the test case introduce in the next commit.
This commit adds a test to exercise that HTLC signatures are sent in the correct order, i.e. they match the sorting of the HTLC outputs on the commitment after applying BOLT 3's BIP69+CLTV sort.
cfromknecht
force-pushed
the
sort-htlc-sigs
branch
from
ee93cc6 to
e8fc5b5
Compare
This commit adds an additional santity check that rejects zero-value HTLCs, preventing them from being added to the channel state even if the channel config's minhtlc value is zero.
cfromknecht
force-pushed
the
sort-htlc-sigs
branch
from
e8fc5b5 to
3f4dc0d
Compare
Roasbeef
approved these changes
Apr 4, 2020
Member
Roasbeef
left a comment
Roasbeef
left a comment
There was a problem hiding this comment.
LGTM 🍺
thanks for investigating the restart case!
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Fixes #4118
This PR fixes an issue in which HTLC signatures sent to the remote peer could be improperly ordered. This can occur when multiple HTLCs share the identical payment hashes and amounts, but have differing CLTV values. In particular, if these HTLCs are added in such a way that subsequence has descending CLTVs, the HTLC signatures would not reflect the HTLC tie-breaker used to sort the commitment.
#3412 partially fixed the issue, by ensuring that we properly sort the actual commitment and second-level transaction refer to the proper txid in their previous outpoint, however it failed to apply this sorting when generating the signing jobs and hence the final ordering that is sent on the wire.