lnwallet/channel: set add heights based on the locked in commits#1294
lnwallet/channel: set add heights based on the locked in commits#1294Roasbeef merged 2 commits intolightningnetwork:masterfrom
Conversation
34f3e22 to
3d78593
Compare
Roasbeef
left a comment
There was a problem hiding this comment.
Nice! I think between this and the other PR, we've nearly cornered those existing state machine bugs which have been holding back the release of 0.4.2.
One minor comment on a TODO, but it isn't a block at all. May merge this in within the next 12 hrs or so.
lnwallet/channel.go
Outdated
There was a problem hiding this comment.
They're already logged in the contract court FWIW.
There was a problem hiding this comment.
woops, stray TODO. Removed.
lnwallet/channel.go
Outdated
There was a problem hiding this comment.
What if the htlcs were locked in before the latest remote commitment?
We could walk backwards along the remote commitment until finding the earliest occurrence of each htlc, not sure that's the best way to approach it though
There was a problem hiding this comment.
There's no material diff if they were say locked in at height 3 (waay before), but then we restore at height 20, as long as it's done symmetrically on both commitments (which it is atm).
lnwallet/channel.go
Outdated
There was a problem hiding this comment.
Same question here wrt. HTLCs being locked in by a prior commitment? This might be more difficult to restore than the remote case since we discard our old local states
lnwallet/channel_test.go
Outdated
There was a problem hiding this comment.
Maybe defer aliceChannel.Stop() after each restore?
There was a problem hiding this comment.
Chose to stop the channel inside restoreAndAssertCommitHeights to not clutter the test flow too much :)
There was a problem hiding this comment.
Hadn't noticed that :) won't catch in cases where we fail, but probably okay
3d78593 to
1f715f5
Compare
This commit fixes a bug which would cause the add heights of the HTLCs in the update log to be set wrongly. At times, an add height could be incorrecly set, leading to the HTLCs not being accounted for correctly during evaluating the HTLC views. This was caused by the assumption that if the HTLC was not on the pending remote commit, then it was locked in on both the local and the remote commit, which is not always true. Instead of making this assumption, we instead now inspect the three commits: the local, remote and pending remote; and set the add heights accordingly. This should ensure that HTLCs are subtracted from the balances only when they are first added.
This commit adds a test which will restore a channel from an OpenChannel struct at various stages of the state transation cycle, ensuring the HTLC local and remote add heights are restored properly.
1f715f5 to
d470064
Compare
This commit fixes a bug which would cause the add heights of the HTLCs
in the update log to be set wrongly. At times, an add height could be
incorrecly set, leading to the HTLCs not being accounted for correctly
during evaluating the HTLC views. This was caused by the assumption that
if the HTLC was not on the pending remote commit, then it was locked in
on both the local and the remote commit, which is not always true.
Instead of making this assumption, we instead now inspect the three
commits: the local, remote and pending remote; and set the add heights
accordingly. This should ensure that HTLCs are subtracted from the
balances only when they are first added.
Fixes #1293.
Fixes #1267.
Fixes #1261.
Fixes #1292.