lnwallet+test: properly generate the sender HTLC script in a contract breach scenario#1025
Merged
Roasbeef merged 5 commits intoApr 6, 2018
Conversation
cfromknecht
suggested changes
Apr 5, 2018
Contributor
cfromknecht
left a comment
cfromknecht
left a comment
There was a problem hiding this comment.
Woohoo! Nice to get our breach tests tightened up a bit further. Changes LGTM, mostly just minor doc fixes
Contributor
There was a problem hiding this comment.
I think the following section (~40 lines) can be replaced with net.SendCoins, no?
Contributor
There was a problem hiding this comment.
she persists her -> he persists his
Contributor
There was a problem hiding this comment.
she should -> he should
Roasbeef
force-pushed
the
incoming-htlc-breach-retribution
branch
from
9df1267 to
f8f8786
Compare
…oth directions In this commit, we extend the testRevokedCloseRetributionRemoteHodl so that the final broadcast revoked transaction has incoming *and* outgoing HTLC's. As is, this test fails as there's a lingering bug in the way we generate htlc resolutions. A follow up commit will remedy this issue.
…ing a breach In this commit, we fix an existing within lnd. Before this commit, within NewBreachRetribution the order of the keys when generating the sender HTLC script was incorrect. As in this case, the remote party is the sender, their key should be first. However, the order was swapped, meaning that at breach time, our transaction would be rejected as it had the incorrect witness script. The fix is simple: swap the ordering of the keys. After this commit, the test extension added in the prior commit now passes.
Roasbeef
force-pushed
the
incoming-htlc-breach-retribution
branch
from
f8f8786 to
c393475
Compare
halseth
previously approved these changes
Apr 5, 2018
Contributor
halseth
left a comment
halseth
left a comment
There was a problem hiding this comment.
Would prefer an added unit test that shows how the htlc dust output was not properly trimmed before, other than that LGTM.
|
|
||
| // If the HTLC is dust, then we'll skip it as it doesn't have | ||
| // an output on the commitment transaction. | ||
| if htlcIsDust( |
Contributor
There was a problem hiding this comment.
Should add a unit test that checks that this is properly fixed.
Member
Author
There was a problem hiding this comment.
Excellent call! Just push out the tests that led to another bug fix.
ghost
mentioned this pull request
…o capacity, not length In this commit, we fix an existing bug in the NewBreachRetribution method. Rather than creating the slice to the proper length, we instead now create it to the proper _capacity_. As we'll now properly filter out any dust HTLCs, before this commit, even if no HTLCs were added, then the slice would still have a full length, meaning callers could actually interact with _blank_ HtlcRetribution structs. The fix is simple: create the slice with the proper capacity, and append to the end of it.
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
3 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.
In this commit, we fix an existing bug w.r.t to the way we handle certain classes of contract breaches. Before PR this commit, within NewBreachRetribution the order of the keys when generating the sender HTLC script was incorrect. As in this case, the remote party is the sender, their key should be first. However, the order was swapped, meaning that at breach time, our transaction would be rejected as it had the incorrect witness script.
We first include an extension to one of the revocation integration tests to trigger a failure. We do this by sending a new set of HTLCs from Carol to Dave (in addition to the ones from Dave to Carol), such that, the final breached commitment has HTLCs in both directions.
We then including a commit with the fix: swap the ordering of the keys. After this PR, the test extension added in the prior commit now passes.
Fixes #970.