Fix TestNewBreachRetributionSkipsDustHtlcs incorrect state transtion #3998
Conversation
The unit test TestNewBreachRetributionSkipsDustHtlcs triggered a state transition from Bob, even though it was Alice that had added the HTLCs. This is wrong since it will lead to Bob still owing Alice a commitment, which is not accounted for in the unit tests. We add a sanity check that the add heights has been set for all entries found in the logs, and return an error otherwise. This won't happen during normal operation, but it does reveal the mistake in the unit test, which is fixed by making Alice trigger the transition. In addition we resolve a long standing TODO by removing a (purposeful) panic in the channel state machine. Old version of lnd had a bug that could lead to the parent entries being lost during channel restore. A panic was added to get to the bottom of if. This is now fixed, so new nodes shouldn't encounter it. However, to be on the safe side, instead of panicking we return an error back to gracefully exit the channel state machine.
halseth
requested review from
cfromknecht and
joostjager
and removed request for
Roasbeef
| ) | ||
|
|
||
| // The parent add height should never be zero at this point. If | ||
| // that's the case we probably forgot to send a new commitment. |
There was a problem hiding this comment.
It should never be zero, because commit height 0 is the opening commitment tx that doesn't contain any htlcs?
There was a problem hiding this comment.
Yes. If it is zero it means it wasn't added to a commitment (yet), so a CommitSig that covers it must be sent. This is what was missing in the test case, and hence we wasn't actually testing settling the HTLCs, as they weren't added at that point.
There was a problem hiding this comment.
I mean just using the magic value of zero to signal that it wasn't added, while commit height zero is also a valid commitment. But I guess it is ok
It shouldn't happen, since this is not really a situation that happens in practice. And if that happens I guess it is better to pipeline it to the switch so you can claim your money there then not to :) |
And there are no other side effects of the pipelining that we need to be aware of? Closing circuits, updating fwd log? I don't have a particular concern, but just checking |
|
@joostjager pipelining of settles only after processing the settle successfully so we should be fine in terms of circuits, fwding log, etc |
The unit test TestNewBreachRetributionSkipsDustHtlcs triggered a state
transition from Bob, even though it was Alice that had added the HTLCs.
This is wrong since it will lead to Bob still owing Alice a commitment,
which is not accounted for in the unit tests.
We add a sanity check that the add heights has been set for all entries
found in the logs, and return an error otherwise. This won't happen
during normal operation, but it does reveal the mistake in the unit
test, which is fixed by making Alice trigger the transition.
In addition we resolve a long standing TODO by removing a (purposeful)
panic in the channel state machine. Old version of lnd had a bug that
could lead to the parent entries being lost during channel restore. A
panic was added to get to the bottom of if.
This is now fixed, so new nodes shouldn't encounter it. However, to be
on the safe side, instead of panicking we return an error back to
gracefully exit the channel state machine.