lnwallet/channel: set add heights based on the locked in commits #1294
Conversation
34f3e22 to
3d78593
Compare
lnwallet/channel.go
Outdated
| nil, pendingCommitPoint, | ||
| ) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| lc.remoteCommitChain.addCommitment(pendingRemoteCommit) | ||
|
|
||
| // TODO: add htlcs |
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.
| // Now set the remote commit height of all incoming HTLCs found on the | ||
| // remote commitment. | ||
| for _, r := range remoteCommitment.incomingHTLCs { | ||
| incomingRemoteAddHeights[r.HtlcIndex] = remoteCommitment.height |
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).
|
|
||
| // And finally we can do the same for the outgoing HTLCs. | ||
| for _, l := range localCommitment.outgoingHTLCs { | ||
| outgoingLocalAddHeights[l.HtlcIndex] = localCommitment.height |
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
| bobChannel = restoreAndAssertCommitHeights(t, bobChannel, true, 0, 2, 1) | ||
| bobChannel = restoreAndAssertCommitHeights(t, bobChannel, true, 1, 2, 2) | ||
|
|
||
| aliceChannel.Stop() |
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.