[lnwallet] Only Forward Committed Settles and Fails#1293
Merged
Conversation
de276a4 to
4033b59
Compare
Roasbeef
requested changes
May 28, 2018
Member
Roasbeef
left a comment
There was a problem hiding this comment.
Nice! This is definitely a bug that has been lingering for some time. I've modified this patch locally to remove the initial "fix" commit and confirmed that the added test does indeed fail at the proper location.
Noted one minor aspect: in that we can make our assertion on forwarded fails more accurate to ensure the correct HTLC failure is forwarded, and not just any failure.
lnwallet/channel_test.go
Outdated
Member
There was a problem hiding this comment.
We can strengthen this check a bit by ensuring the _proper failed HTLC is what's being forwarded.
4033b59 to
c285c32
Compare
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.
This PR fixes an issue that would cause us to forward a settle or fail prematurely, i.e. we would forward a settle or fail before it was fully committed by both commitment transactions.
Previously, we had such a check for
Addhtlcs, and ensure that neither theaddCommitHeightRemoteandaddCommitHeightLocalwere both non-zero. Here, we add the same constraints toremoveCommitHeightRemoteandremoveCommitHeightLocal.We also change the logic around when an
FailorSettleshould be forwarded by requiring equality between the remote height andremoveCommitHeightRemote. Currently, we use>=to determine when we should forward an HTLC in order to mimic the logic forAdds.This change is more minor, and shouldn't result in any observed difference in behavior, but should provide an additional protection against forwarding the same settle or fail twice. The difference lies in that
Addupdates can persist in the log across multiple commitments after being added, whileSettles andFails are always removed immediately after being locked in.Fixes #1124.
Fixes #1053.
Fixes #977.