multi: send channel update for failed interceptor packets #5230
Conversation
|
I'm not too familiar with the interceptor, so just confirming that we want to fail with the incoming channel's update? Assuming that we do because send the resolution to the incoming link. |
| f.packet.incomingChanID, | ||
| ) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
If from some reason this returns an error (like when channel edge is not found) the user won't be able to fail the intercepted HTLC.
I wonder if in this case it is better to pick a different reason for the UpdateFailHTLC, maybe a reason that doesn't require channel update (lnwire.FailTemporaryNodeFailure for example).
There was a problem hiding this comment.
If the HTLC arrived on the incoming edge, I can't think of a practical reason that this call would fail, we need to fetch this policy in order to validate the normal HTLC forwarding information.
See the prior PR for the rationale re why using a node level failure would result in degraded path finding: #5059 (comment)
There was a problem hiding this comment.
@roeierez would a possible approach for this be to:
- Proceed with this change, so that the issue with eclair is fixed
- Follow up with a PR which will reject the HTLC by default if we experience an unexpected error during interception? Seems like reasonable behaviour to me if we're worried about not being able to intercept.
There was a problem hiding this comment.
@carlaKC yes sounds good.
| @@ -139,7 +139,16 @@ func (f *interceptedForward) Resume() error { | |||
|
|
|||
| // Fail forward a failed packet to the switch. | |||
| func (f *interceptedForward) Fail() error { | |||
| reason, err := f.packet.obfuscator.EncryptFirstHop(lnwire.NewTemporaryChannelFailure(nil)) | |||
| update, err := f.htlcSwitch.cfg.FetchLastChannelUpdate( | |||
| f.packet.incomingChanID, | |||
There was a problem hiding this comment.
Yeah I think this is the proper link to use here, as depending on the use case, an outgoing edge might not even actually exist (hence the need for interception). In theory we could extend the API to allow the caller to apss in their own failure, but that's a larger change outside the scope of this simple bug fix.
There was a problem hiding this comment.
If the sender is an lnd node, then sending back a fresh channel update will trigger the "second chance" logic which'll result in retry attempt. The prior behavior of not sending back a channel update at all would cause some other implementations to halt w/ a terminal failure as they were expecting an update to be included.
There was a problem hiding this comment.
Yeah, this is the best strategy imo. roasbeef is spot on with the reasoning.
carlaKC
force-pushed
the
5059-interceptorchanupdate
branch
from
3cd3c3a
to
5b7b6ab
Compare
This PR updates intercepted payments to send channel updates with
TemporaryChannelFailureas is required by the spec.Replaces #5059.