Support manual signaling of channel readiness #2109

johncantrell97 · 2023-03-15T21:44:03Z

First pass at implementing #2106

A lot of the naming here is likely not great. Please make recommendations for Event::PendingChannelReady, signal_channel_readiness, and manually_signal_channel_ready.

I wasn't sure if it was a great idea to make check_get_channel_ready public but it has all the logic I need to implement signal_channel_readiness.

I ended up adding the logic for conditionally sending the ChannelReady message inside of the send_channel_ready macro which might not be great but it was the simplest place to put it to guarantee it would re-trigger the event should the user have the feature enabled.

Fixes #2106

codecov-commenter · 2023-03-15T23:00:45Z

Codecov Report

Patch coverage: 21.05% and project coverage change: +0.29 🎉

Comparison is base (2d213a4) 91.15% compared to head (c8ae2e7) 91.44%.

❗ Current head c8ae2e7 differs from pull request most recent head d529d7b. Consider uploading reports for the commit d529d7b to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2109      +/-   ##
==========================================
+ Coverage   91.15%   91.44%   +0.29%     
==========================================
  Files         101      101              
  Lines       48866    50865    +1999     
  Branches    48866    50865    +1999     
==========================================
+ Hits        44544    46514    +1970     
- Misses       4322     4351      +29

Impacted Files	Coverage Δ
lightning/src/util/config.rs	`40.62% <0.00%> (-0.65%)`	⬇️
lightning/src/util/events.rs	`29.70% <0.00%> (-1.67%)`	⬇️
lightning/src/ln/channel.rs	`89.36% <100.00%> (ø)`
lightning/src/ln/channelmanager.rs	`92.17% <100.00%> (+3.32%)`	⬆️

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

wpaulino · 2023-03-16T18:47:20Z

lightning/src/ln/channelmanager.rs

-		if let Some(real_scid) = $channel.get_short_channel_id() {
-			let scid_insert = short_to_chan_info.insert(real_scid, ($channel.get_counterparty_node_id(), $channel.channel_id()));
-			assert!(scid_insert.is_none() || scid_insert.unwrap() == ($channel.get_counterparty_node_id(), $channel.channel_id()),
+		if $self.default_configuration.manually_signal_channel_ready {


Channels may have a different config from ChannelManager::default_configuration so we should handle that, but it seems like we only track it at the time we create the channel.

Right, I think this maybe belongs in the ChannelHandshakeConfig or so?

Ah ok, yeah that probably makes more sense. This does mean though that I'll have to store the manually_signal_channel_ready value in the Channel itself, right? Seems like the ChannelHandshakeConfig isn't actually saved anywhere and is just used to set some values during Channel creation, right?

Yeah the ChannelHandshakeConfig is ephemeral. We'll definitely need to persist that we need to manually signal since we can restart while the channel still needs confirmations.

Hmm, so the other option would be to only do this for inbound channels, then we can keep looking at the default config?

well at least for our use case we only care about inbound channels. I'm not sure what other use cases there might be for this feature other than waiting to see the funding transaction used by your counterparty.

feels like any potential use case for outbound channels could be implemented during the handling of FundingGenerationReady and delaying giving the tx back to ldk then.

I guess in theory(?) there's some use case where you want to do some check after receiving funding_signed. I don't know, happy to put the config in either place.

I think it doesn't really change the implementation details either way afaict since we will need (at least one) flag in Channel to monitor readiness anyway.

I think it doesn't really change the implementation details either way afaict since we will need (at least one) flag in Channel to monitor readiness anyway.

Yea, I guess absent a good reason not to we should support it both directions and just put it in the handshake and store it.

wpaulino · 2023-03-16T18:53:56Z

lightning/src/ln/channelmanager.rs

+				let best_block_height = self.best_block.read().unwrap().height();
+				let channel = channel.get_mut();
+				match channel.check_get_channel_ready(best_block_height) {
+					Some(channel_ready) => send_channel_ready!(self, pending_msg_events, channel, channel_ready),


Isn't this just going to queue another PendingChannelReady event when manually_signal_channel_ready is set? We need to actually force it through this time.

yeah whoops, my bad. Makes sense to pull the conditional logic out of the macro and then the fn on ChannelManager the user uses can just call the macro to send it.

wpaulino · 2023-03-16T18:54:32Z

lightning/src/util/events.rs

+	/// [`ChannelManager::force_close_without_broadcasting_txn`]: crate::ln::channelmanager::ChannelManager::force_close_without_broadcasting_txn
+	/// [`UserConfig::manually_signal_channel_ready`]: crate::util::config::UserConfig::manually_signal_channel_ready
+	/// [`msgs::ChannelReady`]: crate::ln::msgs::ChannelReady
+	PendingChannelReady {


SignalChannelReady or MarkChannelReady?

That sounds like the event itself marks the channel ready? I kinda like pending, tbh.

wpaulino · 2023-03-16T18:56:19Z

lightning/src/ln/channelmanager.rs

+	/// and the `counterparty_node_id` parameter is the id of the peer the channel is with.
+	///
+	/// [`Event::PendingChannelReady`]: events::Event::PendingChannelReady
+	pub fn signal_channel_readiness(&self, channel_id: &[u8; 32], counterparty_node_id: &PublicKey) -> Result<(), APIError> {


Let's add a test for this.

wpaulino · 2023-03-16T19:09:34Z

lightning/src/ln/channelmanager.rs

-			assert!(scid_insert.is_none() || scid_insert.unwrap() == ($channel.get_counterparty_node_id(), $channel.channel_id()),
+		if $self.default_configuration.manually_signal_channel_ready {
+			let mut pending_events = $self.pending_events.lock().unwrap();
+			pending_events.push(events::Event::PendingChannelReady {


The send_channel_ready macro doesn't seem like the proper place for this. At this point, we're ready to send out ChannelReady to our counterparty, and if we already received theirs, then we'll actually mark the channel as ready internally (see check_get_channel_ready) and even receive a ChannelReady event immediately after. This may result in a HTLC being routed through the channel, even though we haven't signaled it ready ourselves. If we want to continue using check_get_channel_ready for the manual signal, we'll need to make sure it only returns Some(ChannelReady) once signal_channel_readiness is called.

ah, I didn't look carefully enough at check_get_channel_ready and didn't realize it can update channel state.

So the options are to update check_get_channel_ready so that like you said, it only returns the msg if manual is enabled and user has already signaled. This implies adding state to a channel to track whether or not user has signaled already.

Is an alternative to update check_get_channel_ready so that it never returns Some(msg) if manual signaling is enabled and then write new logic in the signal_channel_readiness to make sure it's safe to send and then builds and sends the message?

The latter approach would avoid adding additional state and is the way I'd lean going if you think it's okay?

Is an alternative to update check_get_channel_ready so that it never returns Some(msg) if manual signaling is enabled and then write new logic in the signal_channel_readiness to make sure it's safe to send and then builds and sends the message?

This wouldn't allow the ChannelReady message to be retransmitted to the counterparty upon a reconnection and ChannelReestablish, unless another PendingChannelReady event is queued for the consumer to signal. For your use case, it seems you only care about the first ChannelReady that gets sent, excluding any further ones.

Hm true, I was thinking it would just continue to fire Pending as needed, requiring the user to continually perform whatever checks they need before sending.

I guess if we wanted to support only one approval from user then there's no way around adding state to the Channel so it knows whether or not it's received approval already

I guess we can solve both problems at once and just add a requires_manual_readiness_signal to Channel that gets set to the new config at creation and then when they manually signal we can flip it to false.

Hm true, I was thinking it would just continue to fire Pending as needed, requiring the user to continually perform whatever checks they need before sending.

We could, but I don't immediately see the use case for it. We already do something similar with our ChannelReady event – we only queue it once.

I guess we can solve both problems at once and just add a requires_manual_readiness_signal to Channel that gets set to the new config at creation and then when they manually signal we can flip it to false.

Yeah that should work. Depending on how/when the event is queued, we may need another bit to make sure it's only queued once.

Just realized we also potentially send ChannelReady as part of channel reestablishment and monitor update workflows. Seems like everywhere we might return Some(msg::ChannelReady) we would need to make sure it's been signaled already.

The issue then becomes where do we actually queue the PendingChannelReady event. I guess the normal path is part of do_chain_event workflow? Though maybe the first time can also be triggered after funding_created and a node restart via monitor update and 0-conf channel?

I think I need some help figuring out best place to queue it.

Yea, I think we shouldn't concern ourselves with monitors or peer connectivity for generating the event itself - we should generate the event in the 0conf-signed/block-connection path (which all ends up in check_get_channel_ready I think). In those cases, we will want to not set the OurChannelReady/move to the ChannelReady state until the user tells us we're ready to fly.

We'll want to avoid sending the actual ready message if there's a monitor update pending or the peer is disconnected.

We'll also want to generate a fresh event on restart if we see that we're ready to fly (via check_get_channel_ready) but we're blocked on the user. That may be as simple as...doing nothing (but storing a "we generated the event" flag in memory only), as presumably there will (eventually) be another block connected on restart and we'll generate the event then.

TheBlueMatt

Only some minor nits, but @wpaulino raised some good points in his review.

TheBlueMatt · 2023-03-16T21:07:54Z

lightning/src/util/events.rs

+		/// [`ChannelManager::force_close_without_broadcasting_txn`]: crate::ln::channelmanager::ChannelManager::force_close_without_broadcasting_txn
+		counterparty_node_id: PublicKey,
+		/// The txid of the funding transaction.
+		funding_txid: Txid,


nit; we should expose the funding outpoint, rather than txid.

TheBlueMatt · 2023-03-16T21:10:38Z

lightning/src/util/events.rs

+	/// [`ChannelManager::force_close_without_broadcasting_txn`]: crate::ln::channelmanager::ChannelManager::force_close_without_broadcasting_txn
+	/// [`UserConfig::manually_signal_channel_ready`]: crate::util::config::UserConfig::manually_signal_channel_ready
+	/// [`msgs::ChannelReady`]: crate::ln::msgs::ChannelReady
+	PendingChannelReady {


That sounds like the event itself marks the channel ready? I kinda like pending, tbh.

TheBlueMatt · 2023-03-16T21:11:35Z

lightning/src/ln/channelmanager.rs

-		if let Some(real_scid) = $channel.get_short_channel_id() {
-			let scid_insert = short_to_chan_info.insert(real_scid, ($channel.get_counterparty_node_id(), $channel.channel_id()));
-			assert!(scid_insert.is_none() || scid_insert.unwrap() == ($channel.get_counterparty_node_id(), $channel.channel_id()),
+		if $self.default_configuration.manually_signal_channel_ready {


Right, I think this maybe belongs in the ChannelHandshakeConfig or so?

TheBlueMatt · 2023-03-22T18:56:12Z

Added a note in the description that this Fixes #2106

wpaulino · 2023-03-28T20:26:41Z

Is this ready for another look yet @johncantrell97?

johncantrell97 · 2023-03-28T20:28:15Z

Is this ready for another look yet @johncantrell97?

not yet, sorry. moving to another PR actually as I think this approach was too far off what we want.

johncantrell97 · 2023-03-28T21:05:25Z

Closing this in favor of #2137

johncantrell97 force-pushed the manual-channel-ready branch from 1f68897 to c8ae2e7 Compare March 15, 2023 21:54

wpaulino reviewed Mar 16, 2023

View reviewed changes

TheBlueMatt reviewed Mar 16, 2023

View reviewed changes

TheBlueMatt assigned TheBlueMatt and wpaulino Mar 16, 2023

TheBlueMatt added this to the 0.0.115 milestone Mar 22, 2023

Support manual signaling of channel readiness

d529d7b

johncantrell97 force-pushed the manual-channel-ready branch from c8ae2e7 to d529d7b Compare March 28, 2023 17:29

johncantrell97 closed this Mar 28, 2023

johncantrell97 mentioned this pull request Mar 29, 2023

Support manual signaling of channel readiness #2137

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support manual signaling of channel readiness #2109

Support manual signaling of channel readiness #2109

johncantrell97 commented Mar 15, 2023 •

edited by TheBlueMatt

Loading

codecov-commenter commented Mar 15, 2023 •

edited

Loading

wpaulino Mar 16, 2023

TheBlueMatt Mar 16, 2023

johncantrell97 Mar 17, 2023

wpaulino Mar 17, 2023

TheBlueMatt Mar 17, 2023

johncantrell97 Mar 18, 2023

TheBlueMatt Mar 20, 2023

wpaulino Mar 16, 2023

johncantrell97 Mar 16, 2023

wpaulino Mar 16, 2023

TheBlueMatt Mar 16, 2023

wpaulino Mar 16, 2023

wpaulino Mar 16, 2023

johncantrell97 Mar 17, 2023

wpaulino Mar 17, 2023

johncantrell97 Mar 17, 2023

johncantrell97 Mar 17, 2023

wpaulino Mar 17, 2023

johncantrell97 Mar 18, 2023

TheBlueMatt Mar 20, 2023

TheBlueMatt left a comment

TheBlueMatt Mar 16, 2023

TheBlueMatt Mar 16, 2023

TheBlueMatt Mar 16, 2023

TheBlueMatt commented Mar 22, 2023

wpaulino commented Mar 28, 2023

johncantrell97 commented Mar 28, 2023

johncantrell97 commented Mar 28, 2023

Support manual signaling of channel readiness #2109

Support manual signaling of channel readiness #2109

Conversation

johncantrell97 commented Mar 15, 2023 • edited by TheBlueMatt Loading

codecov-commenter commented Mar 15, 2023 • edited Loading

Codecov Report

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

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

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

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

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

TheBlueMatt left a comment

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

TheBlueMatt commented Mar 22, 2023

wpaulino commented Mar 28, 2023

johncantrell97 commented Mar 28, 2023

johncantrell97 commented Mar 28, 2023

johncantrell97 commented Mar 15, 2023 •

edited by TheBlueMatt

Loading

codecov-commenter commented Mar 15, 2023 •

edited

Loading