Skip to content

HTLC JIT channel interception followup + minor cleanups #1893

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Dec 1, 2022
Merged

HTLC JIT channel interception followup + minor cleanups #1893

merged 5 commits into from
Dec 1, 2022

Conversation

valentinewallace
Copy link
Contributor

Closes #1890.

Because the changes were pretty trivial, I stuck on two minor commits that help prepare for moving payment retries.

@valentinewallace
Clean up HTLC intercept errors
6791d2c 
ChannelUnavailable is a better fit for errors regarding unavailable channels
than APIMisuseError.

Also log bytes in errors as hex instead of decimal.
@valentinewallace
Test for unknown HTLC intercept id error
7858010
@valentinewallace
HTLC intercept test: swap hardcoded value for const
d30122d
@valentinewallace
Fix weird import format in persist
7203c83
@valentinewallace
Rename APIError::RouteError to ::InvalidRoute
e0820ae 
Soon we're going to need to return an error when ChannelManager is unable to
find a route, so we'll need a way to distinguish between that and the user
supplying an invalid route.
@ViktorTigerstrom
Copy link
Contributor

Not sure which follow-ups we'd like to include here, but just leaving another follow-up that I thought of that might be worth considering:

I realized after the PR was merged, that I don't think we communicate well enough to the user that using the Intercept feature will break backwards compatibility.
The only mention we have currently is under the accept_intercept_htlcs config flag:
/// Setting this to true may break backwards compatibility with LDK versions < 0.0.113.

I think it would be good to add some docs to get_intercept_scid that informs the user that they will break backwards compatibility once they generate an intercept scid that's forwarded over with accept_intercept_htlcs set to true.

@TheBlueMatt
Copy link
Collaborator

I realized after the PR was merged, that I don't think we communicate well enough to the user that using the Intercept feature will break backwards compatibility.

Let's add a release note for it. I'm not sure we need to litter it all over our docs.

@ViktorTigerstrom
Copy link
Contributor

I realized after the PR was merged, that I don't think we communicate well enough to the user that using the Intercept feature will break backwards compatibility.

Let's add a release note for it. I'm not sure we need to litter it all over our docs.

Ah yeah, sounds good. Didn't think about that :)!

@TheBlueMatt
Copy link
Collaborator

@valentinewallace can you add pending release notes like @ViktorTigerstrom suggests in another followup? Thanks.

@TheBlueMatt TheBlueMatt merged commit 52edb35 into lightningdevkit:main Dec 1, 2022
@TheBlueMatt TheBlueMatt mentioned this pull request Dec 1, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheBlueMatt TheBlueMatt TheBlueMatt approved these changes

@ViktorTigerstrom ViktorTigerstrom ViktorTigerstrom approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

#1835 followups
3 participants
@valentinewallace @ViktorTigerstrom @TheBlueMatt