Support `ZeroConf` channel type. #1505

tnull · 2022-05-30T21:10:49Z

This PR implements a ZeroConf channel type feature, which is then considered when we receive inbound open channel requests.

Note that zero confirmation channels are rejected by default, and may only get manually accepted via accept_inbound_channel_from_trusted_peer_0conf.

tnull · 2022-05-30T21:19:27Z

lightning/src/ln/features.rs

@@ -402,7 +402,9 @@ mod sealed {
 	define_feature!(47, SCIDPrivacy, [InitContext, NodeContext, ChannelTypeContext],
 		"Feature flags for only forwarding with SCID aliasing. Called `option_scid_alias` in the BOLTs",
 		set_scid_privacy_optional, set_scid_privacy_required, supports_scid_privacy, requires_scid_privacy);
-
+	define_feature!(51, ZeroConf, [ChannelTypeContext],


I still need to add a comment that points out using this feature will break backwards compatibility with versions prior to 0.0.107. Probably, this comment should be added to accept_inbound_channel_from_trusted_peer_0conf?

Probably on the Event::OpenChannelRequest, there's already a similar comment for the scid privacy flag.

Done with 794a6c6.

TheBlueMatt

LGTM aside from the one issue you already noted.

codecov-commenter · 2022-05-30T21:28:48Z

Codecov Report

Merging #1505 (efad02b) into main (ce7b0b4) will increase coverage by 1.26%.
The diff coverage is 94.73%.

@@            Coverage Diff             @@
##             main    #1505      +/-   ##
==========================================
+ Coverage   90.97%   92.23%   +1.26%     
==========================================
  Files          80       80              
  Lines       43081    51324    +8243     
  Branches    43081    51324    +8243     
==========================================
+ Hits        39194    47340    +8146     
- Misses       3887     3984      +97

Impacted Files	Coverage Δ
lightning/src/util/events.rs	`43.78% <ø> (+10.22%)`	⬆️
lightning/src/ln/priv_short_conf_tests.rs	`96.60% <91.37%> (-0.49%)`	⬇️
lightning/src/ln/channel.rs	`92.42% <96.66%> (+3.82%)`	⬆️
lightning/src/ln/channelmanager.rs	`87.72% <100.00%> (+3.01%)`	⬆️
lightning/src/ln/features.rs	`99.46% <100.00%> (+0.01%)`	⬆️
lightning/src/chain/onchaintx.rs	`93.98% <0.00%> (-0.93%)`	⬇️
lightning/src/ln/payment_tests.rs	`99.18% <0.00%> (-0.07%)`	⬇️
lightning/src/lib.rs	`100.00% <0.00%> (ø)`
lightning/src/ln/reorg_tests.rs	`100.00% <0.00%> (ø)`
lightning/src/ln/monitor_tests.rs	`100.00% <0.00%> (ø)`
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce7b0b4...efad02b. Read the comment docs.

tnull · 2022-05-30T22:08:40Z

Squashed.

valentinewallace · 2022-05-30T22:31:13Z

Test? 😛

TheBlueMatt · 2022-05-30T22:31:19Z

lightning/src/util/events.rs

@@ -466,6 +466,10 @@ pub enum Event {
 		/// the resulting [`ChannelManager`] will not be readable by versions of LDK prior to
 		/// 0.0.106.
 		///
+		/// Also note that if [`ChannelTypeFeatures::supports_zero_conf`] returns true on this type,


Maybe also something about either accept the channel via accept_inbound_channel_from_trusted_peer_0conf or the channel will be force-closed?

valentinewallace · 2022-05-30T23:59:01Z

Thanks for the features test.

In channel.rs, I see:

			let mut allowed_type = ChannelTypeFeatures::only_static_remote_key();
			if *channel_type != allowed_type {
				allowed_type.set_scid_privacy_required();
				if *channel_type != allowed_type {
					return Err(ChannelError::Close("Channel Type was not understood".to_owned()));
				}

I'm not sure if we'll accept channels with the 0-conf feature bit set, based on this? Either way, would be nice to either test or file an issue for a test for this case

tnull · 2022-05-31T00:03:03Z

Thanks for the features test.

In channel.rs, I see:
			let mut allowed_type = ChannelTypeFeatures::only_static_remote_key();
			if *channel_type != allowed_type {
				allowed_type.set_scid_privacy_required();
				if *channel_type != allowed_type {
					return Err(ChannelError::Close("Channel Type was not understood".to_owned()));
				}
I'm not sure if we'll accept channels with the 0-conf feature bit set, based on this? Either way, would be nice to either test or file an issue for a test for this case

Thanks, I'll have a look and add a proper test case.

TheBlueMatt · 2022-05-31T21:24:09Z

lightning/src/ln/channel.rs

-				allowed_type.set_scid_privacy_required();
-				if *channel_type != allowed_type {
+			if *channel_type != ChannelTypeFeatures::only_static_remote_key() {
+				if !channel_type.requires_scid_privacy() && !channel_type.requires_zero_conf() {


Wont this change imply we accept a channel_type that has some other unknown bit set?

Argh, right. Fixed this with 7f8bab5.

tnull · 2022-05-31T22:04:17Z

I'd generally like to add yet another test case in channelmanager.rs to check whether the behavior w.r.t. acceptance is the right one. However, I'm currently not sure what the best way to go about it would be, without making the channel type configurable.

tnull · 2022-06-01T00:06:06Z

I'd generally like to add yet another test case in channelmanager.rs to check whether the behavior w.r.t. acceptance is the right one. However, I'm currently not sure what the best way to go about it would be, without making the channel type configurable.

Done with 9ca9dd0.

TheBlueMatt · 2022-06-01T00:24:54Z

lightning/src/ln/channel.rs

+				return Err(ChannelError::Close("Channel Type field contains unknown bits".to_owned()));
+			}
+
+			// We currently only allow three channel types, so write it all out here - we allow


Technically 4 - you can also do SRC and privacy+0conf

TheBlueMatt · 2022-06-01T00:42:07Z

lightning/src/ln/functional_tests.rs

+	nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), InitFeatures::known(), &open_channel_msg);
+
+	let events = nodes[1].node.get_and_clear_pending_msg_events();
+	println!("EVENTS {:?}", events);


Fixed in cfab760.

tnull · 2022-06-01T00:44:22Z

Squashed.

TheBlueMatt · 2022-06-01T00:50:27Z

lightning/src/util/events.rs

+		/// the resulting [`ChannelManager`] will not be readable by versions of LDK prior to
+		/// 0.0.107. Channels setting this type also need to get manually accepted via
+		/// [`crate::ln::channelmanager::ChannelManager::accept_inbound_channel_from_trusted_peer_0conf`],
+		/// or will be rejected otherwise.


Hmm, so its not technically "rejected otherwise" the accept_inbound_channel call fails, and the channel is still there, which kinda sucks. Maybe we just force-close automatically if you call accept_inbound_channel?

AFAICT, if the user calls the wrong method and the channel keeps 'lingering' in our channel_state, we would basically have the same situation as before he called. I'm not sure how this would be different from the 'normal' operation of lingering incoming channels when we set manually_accept_channels?
If we want to get rid of such stale request, we might generally need some kind of garbage collection for the state?

Also not entirely sure what 'force-close' would mean in this context? We'd probably just send an error over the wire and remove the channel from channel_state?

AFAICT, if the user calls the wrong method and the channel keeps 'lingering' in our channel_state, we would basically have the same situation as before he called. I'm not sure how this would be different from the 'normal' operation of lingering incoming channels when we set manually_accept_channels?

Correct, though its pretty different in that the user handled the OpenChannelRequest event, they don't expect to have to handle it twice. In general, our users aren't going to manually handle an error - if we return an APIError here, the users' code probably just won't do anything else. The counterparty, however, may be waiting for an error to retry the channel open (this is the expected behavior for a channel_type "negotiation").

We can (generally) assume that users will try to handle OpenChannelRequest, my concern her is that they won't handle an error handling it.

If we want to get rid of such stale request, we might generally need some kind of garbage collection for the state?

Agreed - in general we should eventually time-out requests, but such requests may take some time to process (external API calls to third-party providers are not unlikely, I believe), so we have to be kinda conservative here, likely even too conservative for moving the channel_type negotiation forward.

Also not entirely sure what 'force-close' would mean in this context? We'd probably just send an error over the wire and remove the channel from channel_state?

Yep, that. I generally use "force-close" to mean "send error message and close channel", though, indeed, there's no broadcast associated here.

Alright, I now reject the channel in accept_inbound_channel if it is 0conf. Also added a note explaining this behavior.

tnull · 2022-06-01T20:03:40Z

Squashed again.

TheBlueMatt · 2022-06-02T00:05:28Z

Feel free to squash again (or split the commits up a bit).

tnull · 2022-06-02T00:07:06Z

Squashed.

tnull commented May 30, 2022

View reviewed changes

TheBlueMatt added this to the 0.0.107 milestone May 30, 2022

TheBlueMatt reviewed May 30, 2022

View reviewed changes

TheBlueMatt previously approved these changes May 30, 2022

View reviewed changes

tnull dismissed TheBlueMatt’s stale review via 1add379 May 30, 2022 22:08

tnull force-pushed the 2022-05-support-0conf-channeltype branch from 794a6c6 to 1add379 Compare May 30, 2022 22:08

TheBlueMatt reviewed May 30, 2022

View reviewed changes

TheBlueMatt reviewed May 31, 2022

View reviewed changes

tnull force-pushed the 2022-05-support-0conf-channeltype branch from a5d577f to 3c13abe Compare May 31, 2022 22:09

TheBlueMatt reviewed Jun 1, 2022

View reviewed changes

tnull force-pushed the 2022-05-support-0conf-channeltype branch 2 times, most recently from 526422a to cfab760 Compare June 1, 2022 00:43

TheBlueMatt reviewed Jun 1, 2022

View reviewed changes

tnull force-pushed the 2022-05-support-0conf-channeltype branch from cfa656e to 6ec1d0a Compare June 1, 2022 20:03

Implement ZeroConf feature.

efad02b

tnull force-pushed the 2022-05-support-0conf-channeltype branch from a28a5ae to efad02b Compare June 2, 2022 00:05

TheBlueMatt approved these changes Jun 2, 2022

View reviewed changes

TheBlueMatt added the Seeking Code Review label Jun 2, 2022

valentinewallace approved these changes Jun 2, 2022

View reviewed changes

valentinewallace merged commit ab20284 into lightningdevkit:main Jun 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `ZeroConf` channel type. #1505

Support `ZeroConf` channel type. #1505

tnull commented May 30, 2022

tnull May 30, 2022 •

edited

TheBlueMatt May 30, 2022

tnull May 30, 2022

TheBlueMatt left a comment

codecov-commenter commented May 30, 2022 •

edited

tnull commented May 30, 2022

valentinewallace commented May 30, 2022

TheBlueMatt May 30, 2022

valentinewallace commented May 30, 2022

tnull commented May 31, 2022

TheBlueMatt May 31, 2022

tnull May 31, 2022

tnull commented May 31, 2022 •

edited

tnull commented Jun 1, 2022

TheBlueMatt Jun 1, 2022

tnull Jun 1, 2022

TheBlueMatt Jun 1, 2022

tnull Jun 1, 2022

tnull commented Jun 1, 2022

TheBlueMatt Jun 1, 2022

tnull Jun 1, 2022

TheBlueMatt Jun 1, 2022

tnull Jun 1, 2022

tnull commented Jun 1, 2022

TheBlueMatt commented Jun 2, 2022

tnull commented Jun 2, 2022

Support ZeroConf channel type. #1505

Support ZeroConf channel type. #1505

Conversation

tnull commented May 30, 2022

tnull May 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

codecov-commenter commented May 30, 2022 • edited

Codecov Report

tnull commented May 30, 2022

valentinewallace commented May 30, 2022

Choose a reason for hiding this comment

valentinewallace commented May 30, 2022

tnull commented May 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull commented May 31, 2022 • edited

tnull commented Jun 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull commented Jun 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull commented Jun 1, 2022

TheBlueMatt commented Jun 2, 2022

tnull commented Jun 2, 2022

Support `ZeroConf` channel type. #1505

Support `ZeroConf` channel type. #1505

tnull May 30, 2022 •

edited

codecov-commenter commented May 30, 2022 •

edited

tnull commented May 31, 2022 •

edited