Skip to content
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

error on htlc_intercepted for unknown scid #138

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

johncantrell97
Copy link
Contributor

Currently we return Ok(()) if the intercept_scid is unknown which means upstream users won't know that we didn't know about this SCID and the intercepted htlc will be stuck until expiry.

We should signal Error for unknown scid's so upstream users can fail back if there's no other reason for the intercepted htlc.

Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, wasn't this kind of expected behavior as we wouldn't exactly fail if the user decided not to hand us every single intercept SCID?

See the comment:

	/// Will do nothing if the intercept scid does not match any of the ones we gave out.

@johncantrell97
Copy link
Contributor Author

Hm, wasn't this kind of expected behavior as we wouldn't exactly fail if the user decided not to hand us every single intercept SCID?

See the comment:

	/// Will do nothing if the intercept scid does not match any of the ones we gave out.

I think we need to error or maybe we return some kind of enum/bool. The LSP needs some way to know whether or not that this handler of intercepted HTLCs was interested in the HTLC but failed to process or does not care about that htlc.

In theory the lsp could have a chain of handlers for intercepted htlcs that it tries in succession until it finds the one that cares about it.

More importantly, if we intercept an HTLC and the liquidity library returns Ok(()) then our code assumes it was or will be handled by liquidity crate but in this case nothing will ever happen and the HTLC will be held until expiry.

We need some way to know that we should fail the htlc backwards if our handlers don't know about it or do but fail to process it.

@tnull
Copy link
Collaborator

tnull commented May 31, 2024

Hm, wasn't this kind of expected behavior as we wouldn't exactly fail if the user decided not to hand us every single intercept SCID?
See the comment:

	/// Will do nothing if the intercept scid does not match any of the ones we gave out.

I think we need to error or maybe we return some kind of enum/bool. The LSP needs some way to know whether or not that this handler of intercepted HTLCs was interested in the HTLC but failed to process or does not care about that htlc.

In theory the lsp could have a chain of handlers for intercepted htlcs that it tries in succession until it finds the one that cares about it.

More importantly, if we intercept an HTLC and the liquidity library returns Ok(()) then our code assumes it was or will be handled by liquidity crate but in this case nothing will ever happen and the HTLC will be held until expiry.

We need some way to know that we should fail the htlc backwards if our handlers don't know about it or do but fail to process it.

Alright, mind adjusting said comment to reflect the new behavior then?

@tnull
Copy link
Collaborator

tnull commented Jul 1, 2024

Gentle ping @johncantrell97

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants