search

lightningdevkit / lightning-liquidity

Other
27 stars 17 forks source link

LSPS2: Handle all htlc_intercepted error scenarios #40

Open johncantrell97 opened 10 months ago

johncantrell97 commented 10 months ago

If the scid to peer map is empty for an scid we currently just do nothing and return Ok((). This means the intercepted HTLC will never be failed (or forwarded) until LDK detects it's close to expiry. The library should fail it for the user here.

If the peer is found but there's no peer_state then we return Err to the user but do not automatically fail it for them.

Finally, if we do have peer state but there's no outbound jit channel found, we will end up returning Ok(() and the HTLC will remain stuck yet again.

Need to properly fail and error all of these cases.

ZmnSCPxj-jr commented 10 months ago

You also want to consider composability, i.e. the intercepted-HTLC interface could also be used for non-LSPS services outside of ldk-lsp-client.

My suggestion is to create a composable interface with two primary methods: a "do you know this SCID?" call and a "you said you know this SCID so handle it now" call. Then a driver interfaces between the LDK intercepted-HTLC event and the composable interface, calls "do you know this SCID?" for each given interface and if that fails, definitely fails the interception event, otherwise it calls into "handle it now" of the first given interface instance that said it knows the SCID.

johncantrell97 commented 10 months ago

Yeah that's a good point. Originally I was thinking it would just return an error that differentiates between "IDontKnowThisSCID" and "IKnowThisSCIDAndItFailedForSomeReason" but your solution feels better.

ZmnSCPxj-jr commented 10 months ago

Properly speaking the "do you know this SCID" call should return a Option<dyn FnOnce<whatever>> or something rusty like that (the FnOnce is the function that is called for the "handle it now"), i.e. the "do you know this SCID" should only expose the "okay you said you know it so do it now" interface if it actually claims to know the SCID, otherwise returns None.

I proposed something similar for the. unrecognized message ID composable interface but that was rejected because of not being efficient due to the use of dyn though. On the other hand, correctness > efficiency.