Skip to content

Use inbound SCID alias for blinded path creation #3902

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

Merged
merged 1 commit into from
Jul 2, 2025

Conversation

TheBlueMatt
Copy link
Collaborator

When we generate a compact blinded path to ourselves, we currently use the real SCID of our channels to tell our peers where to forward to. This is fine for short-lived blinded paths where we expect the channel to still be around, but post-splicing this may spuriously invalidate blinded paths just because we spliced.

Instead, here, we default to using inbound SCID aliases where possible. We also avoid one more reference to the channel's internal FundingContext in channelmanager.rs.

When we generate a compact blinded path to ourselves, we currently
use the real SCID of our channels to tell our peers where to
forward to. This is fine for short-lived blinded paths where we
expect the channel to still be around, but post-splicing this may
spuriously invalidate blinded paths just because we spliced.

Instead, here, we default to using inbound SCID aliases where
possible. We also avoid one more reference to the channel's
internal `FundingContext` in `channelmanager.rs`.
@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Jun 30, 2025

I've assigned @tankyleo as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@ldk-reviews-bot ldk-reviews-bot requested a review from tankyleo June 30, 2025 15:09
Copy link
Contributor

@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.

LGTM, but regarding "We also avoid one more reference to the channel's
internal FundingContext in channelmanager.rs.", mind sharing some context on why this is an improvement?

@TheBlueMatt
Copy link
Collaborator Author

"We also avoid one more reference to the channel's
internal FundingContext in channelmanager.rs.", mind sharing some context on why this is an improvement?

FundingContext is an internal detail to a channel, channelmanager.rs should have no need to care at all that a channel's funding has changed, there's simply no reason for it, and more encapsulation of the channel would make channelmanager.rs way less error-prone.

@tnull
Copy link
Contributor

tnull commented Jul 1, 2025

FundingContext is an internal detail to a channel, channelmanager.rs should have no need to care at all that a channel's funding has changed, there's simply no reason for it, and more encapsulation of the channel would make channelmanager.rs way less error-prone.

Ah, I have to admit I also remembered that context when reviewing #3881 afterwards, as we try to reduce the occurrences of FundingScope there.

Copy link
Contributor

@tankyleo tankyleo left a comment

Choose a reason for hiding this comment

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

LGTM as long as we are positive nodes in the blinded path will not complain that we've given them an alias and not the real scid - I don't really see this explicitly written down in the spec, but maybe it is implied ?

@TheBlueMatt
Copy link
Collaborator Author

I assume its fine (the peer told us its an alias for the channel!), but we should probably update the spec to ensure this. Do you mind tackling that with a quick spec PR?

@tnull tnull merged commit 22c3433 into lightningdevkit:main Jul 2, 2025
27 of 28 checks passed
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.

4 participants