-
Notifications
You must be signed in to change notification settings - Fork 506
Assume option_channel_type
#1232
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hell yea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a bunch of "default channel type" logic that we should also remove from the funding_signed
requirements.
Indeed, I missed a lot of places where |
Wouldn't the first step here be for everyone to first start to set the required feature bit? That's the existing mechanism we have in the protocol to change something from optional to mandatory. Compared to this PR, it's explicit and unambiguous. |
Opening this PR is the first step to get the discussion started! So yes, we should definitely start setting it as mandatory. But it's also trivial to look at the network's data and see if there are nodes that don't set it yet: if everyone already sets this, we know we can start assuming it. I'll collect that data today and will report back. |
This feature has become more and more useful as we've introduced various channel commitment formats, and has been introduced more than 3 years ago in lightning#880. It should be safe to assume that every implementation sets this TLV fields.
Add a new context type `T` for features that can be included in the `channel_type` field when opening channels.
1468cdc
to
a3dcc51
Compare
There is only one node with a Since this node has only 1 small channel, I think we can safely ignore this and consider that every active node on the network does support |
I'm happy to set it as compulsory in the CLN 25.05 release. We added this in 0.12.0 (2022-08-23), though there were fixes as late as 24.02. Either way that's well outside our support window. |
However, there are about 2600 nodes which still don't advertize support. Those versions are all well out-of-date though, so those results may be false. Further analysis required. For example, some nodes haven't sent out a channel update in years. Their channels are kept alive by their peers retransmitting (in some cases, with the disabled flag set). CLN has logic to not create refresh channel_update messages for disabled channels, maybe this should be in the spec? That would allow these channels (and thus nodes) to vanish from the graph naturally. Though really, closing uncontactable channels after a year would also be a decent spec update! Consider: It seems like its peers are keeping it alive by refreshing the channel_update. But I'd guess they haven't heard from it in months / years, so they shouldn't? |
What I find surprising is why those channels aren't closed: if your peer has been unavailable for months, you should close channels with them to claim your funds back and reallocate them? If implementations aggressively prompted node operators to do so (or automated that task) that would fix the issue entirely. |
You assume a lot about how actively most nodes are maintained... |
Related to this spec PR: lightning/bolts#1232. To start with, we'll start to set the required feature bit for the `channel_type` feature where applicable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🏷️
This is the first step before we make the change to always assume option_channel_type. see lightning/bolts#1232 Removed tests that depend on a node without the option_channel_type feature set. Updated some tests that must explicitly set the init features to include option_channel_type.
This is the first step before we make the change to always assume option_channel_type. see lightning/bolts#1232 Removed tests that depend on a node without the option_channel_type feature set. Updated some tests that must explicitly set the init features to include option_channel_type.
This is the first step before we make the change to always assume option_channel_type. see lightning/bolts#1232 Removed tests that depend on a node without the option_channel_type feature set. Updated some tests that must explicitly set the init features to include option_channel_type.
Related to this spec PR: lightning/bolts#1232. To start with, we'll start to set the required feature bit for the `channel_type` feature where applicable.
This feature has become more and more useful as we've introduced various channel commitment formats, and has been introduced more than 3 years ago in #880. It should be safe to assume that every implementation sets this TLV fields?
We also add a new context type
T
for features that can be included in thechannel_type
field when opening channels in the second commit.