contractcourt+lnwallet: ensure the chainWatcher properly plays all remote commitments#1158
Merged
Merged
Conversation
Roasbeef
force-pushed
the
play-all-commits
branch
2 times, most recently
from
7ecf105 to
0ccdfee
Compare
Member
Author
|
Pushed up a new version w/ some bug fixes and also the conflict addressed. |
halseth
previously approved these changes
May 2, 2018
Contributor
halseth
left a comment
halseth
left a comment
There was a problem hiding this comment.
LGTM, this takes care of an important edge case within the protocol! Also been missing chainwatcher tests in my life, so nice to see that addition. 💯💯💯💯💯💯💯💯
cfromknecht
suggested changes
May 2, 2018
Contributor
cfromknecht
left a comment
cfromknecht
left a comment
There was a problem hiding this comment.
Nice fixes! Great to clean up all these pending commitment cases in next release ⚡️
Contributor
There was a problem hiding this comment.
maybe add warning in the even that broadcastStateNum > remoteStateNum+1 now that we would take no action?
Roasbeef
force-pushed
the
play-all-commits
branch
3 times, most recently
from
d142ac7 to
2889e0b
Compare
…nelSummary In this commit, we extend the CloseChannelSummary by also storing: the current unrevoked revocation for the remote party, the next pending unused revocation, and also the local channel config. We move to store these as the provide an extra level of defense against bugs as we'll always store information required to derive keys for any current and prior states.
In this commit, we move a set of useful functions for testing channels into a new file. The old createTestChannels has been improved as it will now properly set the height hint on the first created commitments, and also no longer accepts any arguments as the revocation window no longer exists.
…ote commits In this commit, we modify the NewUnilateralCloseSummary to be able to distinguish between a unilateral closure using the lowest+highest commitment the remote party possesses. Before this commit, if the remote party broadcast their highest commitment, when they have a lower unrevoked commitment, then this function would fail to find the proper output, leaving funds on the chain. To fix this, it's now the duty of the caller to pass remotePendingCommit with the proper value. The caller should use the lowest unrevoked commitment, and the height hint of the broadcast commitment to discern if this is a pending commitment or not.
…ries for pending broadcast commitments
…o play all remote commitments
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 extend the
chainWatcherand thelnwire.NewUnilteralCloseSummarymethods to ensure that we're able to properly play all valid commitments the remote party may be holding. A bug that existed before this commit would at times cause us to not create the proper close channel summary meaning we wouldn't detect that we had a spendable output within the commitment that the remote party broadcasted.A test of tests at the channel state machine level as well as at the
chainWatcherlevel have been added to ensure this PR properly fixes the issue at hand.Fixes #1141.
Fixes #927.