-
Notifications
You must be signed in to change notification settings - Fork 264
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Detect broken HTLC links at startup #757
Conversation
If we have stopped eclair while it was forwarding HTLCs, it is possible that we are in a state were an incoming HTLC was committed by both sides, but we didn't have time to send and/or sign the corresponding HTLC to the downstream node. In that case, if we do nothing, the incoming HTLC will eventually expire and we won't lose money, but the channel will get closed, which is a major inconvenience. This check will detect this and will allow us to fast-fail HTLCs and thus preserve channels.
.collect { case r: Relayed => r } | ||
.toSet | ||
|
||
val htlcs_broken = htlcs_in.filterNot(htlc_in => relayed_out.exists(r => r.originChannelId == htlc_in.channelId && r.originHtlcId == htlc_in.id)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens for direct payments that we've received ? there is no corresponding Relayed
origin so they will be considered broken ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, good catch
|
||
val brokenHtlcs = checkBrokenHtlcsLink(channels) | ||
val brokenHtlcKiller = context.actorOf(Props[HtlcReaper], name = "htlc-reaper") | ||
brokenHtlcKiller ! brokenHtlcs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since broken HTLCs are known when we create the reaper actor, why not pass them to its constructor ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because they would never be garbage collected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would also be a bit more efficient not to create the reaper actor if the list is empty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it, but I don't really like that kind of branching that early in the code.
Also, the fact that the reaper's state is never cleaned up is not great.
If we have stopped eclair while it was forwarding HTLCs, it is possible
that we are in a state were an incoming HTLC
was committed by both sides, but we didn't have time to send
and/or sign the corresponding HTLC to the downstream node.
In that case, if we do nothing, the incoming HTLC will
eventually expire and we won't lose money, but the channel
will get closed, which is a major inconvenience.
This check will detect this and will allow us
to fast-fail HTLCs and thus preserve channels.