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

Add channel type feature bit #2073

Merged
merged 3 commits into from
Dec 2, 2021
Merged

Add channel type feature bit #2073

merged 3 commits into from
Dec 2, 2021

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented Nov 23, 2021

We already support channel types, but we make it explicit with a feature bit as required by lightning/bolts#906

@t-bast t-bast requested a review from pm47 November 23, 2021 10:58
Copy link
Member

@pm47 pm47 left a comment

Choose a reason for hiding this comment

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

Do I understand correctly that we don't use the feature bit in our code because we will always set the tlv? But then:

  • shouldn't we add a check at startup that forces the feature bit to be enabled?
  • shouldn't we check the remote feature bit and reject if the feature bit is set and there is no corresponding tlv?

@t-bast
Copy link
Member Author

t-bast commented Dec 2, 2021

Do I understand correctly that we don't use the feature bit in our code because we will always set the tlv?

Yes, that is correct, because the spec allows it and it's an odd tlv.
It's less code and less branches to test to always include it rather than check for the feature bit.
LND insisted to add this feature bit to the spec to eventually be able to only connect to nodes that support the feature.

shouldn't we add a check at startup that forces the feature bit to be enabled?

We could indeed, I'll add that.

shouldn't we check the remote feature bit and reject if the feature bit is set and there is no corresponding tlv?

We could. I thought it would better to be lenient here because we automatically fallback to the most probable channel type based on feature bits, which will very likely work, but I can change that.

We already support channel types, but we make it explicit with a feature
bit as required by lightning/bolts#906
@t-bast
Copy link
Member Author

t-bast commented Dec 2, 2021

Done in 65f96b8 and 74baf90

@t-bast t-bast requested a review from pm47 December 2, 2021 16:29
When the feature bit is set, they must provide a channel type.
Copy link
Member

@pm47 pm47 left a comment

Choose a reason for hiding this comment

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

LGTM

@t-bast t-bast merged commit bacb31c into master Dec 2, 2021
@t-bast t-bast deleted the channel-type-feature-bit branch December 2, 2021 17:22
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