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

Remove redundant ChannelContext::channel_type #3678

Merged
merged 3 commits into from
Apr 2, 2025

Conversation

jkczyz
Copy link
Contributor

@jkczyz jkczyz commented Mar 21, 2025

A channel's type is duplicated in ChannelContext::channel_type and FundingScope, via ChannelTranscationParameters::channel_type_features. Remove the redundant ChannelContext::channel_type and use FundingScope::get_channel_type instead. While the type will be the same across all FundingScope's during splicing, this may not be the case in the future when migrating existing channels to taproot.

Based on #3651.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Mar 21, 2025

👋 Thanks for assigning @wpaulino 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.

Copy link
Contributor

@dunxen dunxen left a comment

Choose a reason for hiding this comment

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

LGTM. Makes sense to have it under funding scope only.

@jkczyz jkczyz force-pushed the 2025-03-channel-type branch from 8509d0b to cb31af3 Compare March 25, 2025 18:56
While channel_type_features won't change during splicing, they may
change when migrating existing channels to taproot in the future.
@jkczyz jkczyz force-pushed the 2025-03-channel-type branch from cb31af3 to 6490e39 Compare March 25, 2025 21:00
@jkczyz jkczyz marked this pull request as ready for review March 25, 2025 21:00
@jkczyz
Copy link
Contributor Author

jkczyz commented Mar 25, 2025

Rebased on main to avoid being blocked by #3651.

@wpaulino wpaulino requested review from wpaulino and removed request for valentinewallace March 25, 2025 21:10
Copy link
Contributor

@wpaulino wpaulino 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 CI is sad

FundingScope::get_channel_type can be used instead of
ChannelContext::channel_type, which is redundant.
@jkczyz jkczyz force-pushed the 2025-03-channel-type branch from 6490e39 to b036a94 Compare March 26, 2025 02:38
@jkczyz jkczyz added the weekly goal Someone wants to land this this week label Mar 26, 2025
@jkczyz jkczyz requested review from dunxen and wpaulino March 26, 2025 04:30
@jkczyz
Copy link
Contributor Author

jkczyz commented Mar 26, 2025

Pushed a fix for CI.

dunxen
dunxen previously approved these changes Mar 26, 2025
@wpaulino
Copy link
Contributor

wpaulino commented Apr 1, 2025

CI is sad

@wpaulino wpaulino removed their request for review April 1, 2025 17:29
When reading ChannelTransactionParameters, the channel_type_features
defaults to only_static_remote_key. This is similarly the case for
reading a channel. Instead of overriding the latter with the former,
check that they are consistent. It doesn't appear that overriding was
ever necessary.
@jkczyz jkczyz force-pushed the 2025-03-channel-type branch from 54ebb06 to 1bb3b4d Compare April 1, 2025 19:09
@jkczyz jkczyz requested a review from wpaulino April 1, 2025 19:10
@wpaulino
Copy link
Contributor

wpaulino commented Apr 2, 2025

Merging as CI failure seems unrelated: error: failed to run custom build command for aws-lc-sys v0.28.0

@wpaulino wpaulino merged commit d9c20be into lightningdevkit:main Apr 2, 2025
24 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
weekly goal Someone wants to land this this week
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants