Support manual signaling of channel readiness #2137

johncantrell97 · 2023-03-28T21:04:43Z

Second pass at implementing #2106

Refactors the unnamed tuple that gets returned as part of do_chain_work flow with a ChainActionUpdates struct.
Adds a manually_signal_channel_ready config option to ChannelHandshakeConfig for users to configure a channel with this option. Defaults to false.
Saves this option to Channel using requires_manual_readiness_signal and this same flag is updated when user manually signals readiness.
Tracks emission of PendingChannelReady event using new flag on Channel pending_channel_ready_event_emitted similar to how the ChannelReady event works.
Adds a new ChainActionUpdates field to signal to ChannelManager when it's time to generate the PendingChannelReady event.

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

@wpaulino would love another round of review on this approach when you get a chance

codecov-commenter · 2023-03-28T23:46:33Z

Codecov Report

Patch coverage: 61.33% and project coverage change: +0.07 🎉

Comparison is base (31e78ff) 91.40% compared to head (5c88c17) 91.48%.

📣 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    #2137      +/-   ##
==========================================
+ Coverage   91.40%   91.48%   +0.07%     
==========================================
  Files         102      102              
  Lines       49563    50430     +867     
  Branches    49563    50430     +867     
==========================================
+ Hits        45304    46136     +832     
- Misses       4259     4294      +35

Impacted Files	Coverage Δ
lightning/src/events/mod.rs	`30.91% <0.00%> (-1.21%)`	⬇️
lightning/src/util/config.rs	`40.00% <0.00%> (-0.63%)`	⬇️
lightning/src/ln/channel.rs	`89.89% <74.50%> (-0.16%)`	⬇️
lightning/src/ln/channelmanager.rs	`89.17% <88.88%> (+0.02%)`	⬆️

... and 5 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.

lightning/src/ln/channel.rs

wpaulino · 2023-03-28T23:05:52Z

lightning/src/ln/channel.rs

@@ -6484,6 +6500,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Writeable for Channel<Signer> {
 			(23, channel_ready_event_emitted, option),
 			(25, user_id_high_opt, option),
 			(27, self.channel_keys_id, required),
+			(29, requires_manual_readiness_signal, option)


Hmm, maybe we should make this even with an option that's only Some if the flag is set? If a user downgrades while the channel is still waiting for the remote's ChannelReady, then they won't get the signal they explicitly requested.

Interesting. So to make sure I understand. It's a user who had enabled this feature and got partially through the channel open process. They then downgrade LDK to a version before this feature exists. It means this TLV is persisted and the old version will just ignore it because it doesn't know about it.

In the downgraded ldk it will automatically signal channel ready without asking them.

I don't really think that's a problem? They explicitly downgraded to a version of LDK that no longer supports manual signaling?

If we make this even then it means when they try to start up the downgraded LDK it will panic when it tries to deserialize this type it doesn't know about?

Basically forcing them to upgrade or wait for a bugfix for whatever reason they downgraded in the first place?

If all of this is correct then I guess I'm not so sure what is the better experience. I guess it really just depends on why they want to manually signal and partially why they are downgrading.

Generally, we don't support downgrading if you opt in to a new feature previously not supported, but will leave it up to @TheBlueMatt.

FWIW, for your use case of this feature, it seems like you wouldn't want a channel to enter ChannelReady on its own if you downgrade.

Agreed, though we currently are okay with it happening on the node today. But yeah, I guess in theory we would not want it to happen.

Yea, I don't have a strong feeling here either. I assume there's no case where we could downgrade then forget to send the channelready, because we rebroadcast it on startup if we haven't exchanged any HTLCs...I'm fine with leaving it as-is.

lightning/src/events/mod.rs

wpaulino · 2023-03-28T23:42:00Z

lightning/src/ln/channelmanager.rs

+				let (channel_ready_opt, _) = channel.check_get_channel_ready(best_block_height);
+				match channel_ready_opt {
+					Some(channel_ready) => {
+						channel.unset_requires_manual_readiness_signal();


This doesn't seem to achieve anything? We decide not to further emit once we've done it once.

I think we may even be able to just store one bool per channel, requires_manual_readiness_signal, and flip it to false once we queue the PendingChannelReady event.

Hm yeah, that's how I originally had it but this comment #2109 (comment) made it seem like we wanted to re-generate the PendingChannelReady event on restart if they hadn't signaled yet.

So the flow is that a chain event happens, we generate PendingChannelReady, flip requires_manual_readiness_signal to false, then LDK terminates.

On next startup, assuming the flag update was persisted, we will go ahead and automatically send ChannelReady when a block is connected without the user manually signaling it's okay.

So if we only have one flag and don't flip it until they signal then every time a block is found we would keep generating the event.

So one flag tracks whether or not we already emitted the event to prevent duplicates on subsequent chain events and the other is supposed to track whether this channel still requires manual signal from user.

I guess the way I have it currently will also persist the "we emitted" flag and would prevent a re-emission on startup which is why Matt mentioned in-memory only?

Thoughts? I'm kind of confusing myself again around this logic.

Sorry for the confusion, the change is somewhat subtle but looks correct. I still don't think unset_requires_manual_readiness_signal does anything though since we also check if we've already emitted an event in should_emit_pending_channel_ready_event?

I guess the way I have it currently will also persist the "we emitted" flag and would prevent a re-emission on startup which is why Matt mentioned in-memory only?

I'm not seeing the immediate use case for regenerating it on startup if we need the user to signal. As long as we've already notified them once through the event (which is persisted), we just have to wait for them to handle it.

lightning/src/ln/channel.rs

wpaulino · 2023-03-29T17:45:20Z

lightning/src/ln/channel.rs

@@ -4968,12 +5036,12 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
 		self.channel_update_status = status;
 	}

-	fn check_get_channel_ready(&mut self, height: u32) -> Option<msgs::ChannelReady> {
+	pub fn check_get_channel_ready(&mut self, height: u32) -> ChannelReadyStatus {


Just needs to be pub(crate)?

wpaulino · 2023-03-29T17:49:24Z

lightning/src/ln/channelmanager.rs

+			hash_map::Entry::Occupied(mut channel) => {
+				let best_block_height = self.best_block.read().unwrap().height();
+				let channel = channel.get_mut();
+				match channel.check_get_channel_ready(best_block_height) {


Let's first check that the channel must be in the appropriate state to do this (i.e., TheirChannelReady or FundingSent) and that it requires a manual signal before actually signaling.

wpaulino · 2023-03-29T17:51:26Z

lightning/src/ln/channel.rs

@@ -75,6 +97,24 @@ pub struct AvailableBalances {
 	pub next_outbound_htlc_limit_msat: u64,
 }

+pub(crate) struct ChainActionUpdates {
+	pub(crate) channel_ready_msg: Option<msgs::ChannelReady>, 


Is it any nicer if we reuse the ChannelReadyStatus enum here as well?

wpaulino · 2023-03-29T18:00:27Z

lightning/src/ln/channel.rs

@@ -6484,6 +6500,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Writeable for Channel<Signer> {
 			(23, channel_ready_event_emitted, option),
 			(25, user_id_high_opt, option),
 			(27, self.channel_keys_id, required),
+			(29, requires_manual_readiness_signal, option)


Generally, we don't support downgrading if you opt in to a new feature previously not supported, but will leave it up to @TheBlueMatt.

FWIW, for your use case of this feature, it seems like you wouldn't want a channel to enter ChannelReady on its own if you downgrade.

wpaulino · 2023-03-29T18:00:31Z

lightning/src/ln/channelmanager.rs

+				let (channel_ready_opt, _) = channel.check_get_channel_ready(best_block_height);
+				match channel_ready_opt {
+					Some(channel_ready) => {
+						channel.unset_requires_manual_readiness_signal();


Sorry for the confusion, the change is somewhat subtle but looks correct. I still don't think unset_requires_manual_readiness_signal does anything though since we also check if we've already emitted an event in should_emit_pending_channel_ready_event?

I guess the way I have it currently will also persist the "we emitted" flag and would prevent a re-emission on startup which is why Matt mentioned in-memory only?

I'm not seeing the immediate use case for regenerating it on startup if we need the user to signal. As long as we've already notified them once through the event (which is persisted), we just have to wait for them to handle it.

wpaulino · 2023-03-29T18:02:12Z

We should definitely have test coverage for this on both zero conf and conf channels.

TheBlueMatt · 2023-03-29T20:53:53Z

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> {


Would this API be easier to use if we allow it to be called before the PendingChannelReady event? Wouldn't be much code difference, but would let users also use this as a method to "turn on" a channel which hasn't yet fully confirmed, as long as their peer does as well. You could even opt into 0conf after-the-fact with it (which I think is how CLN does 0conf).

We could also do that later but in the mean time at least allow this method to be called and unset the wait-for-event bit on the channel even if we're not ready to send the channel_ready.

TheBlueMatt · 2023-03-29T20:58:10Z

lightning/src/ln/channelmanager.rs

@@ -1504,6 +1504,23 @@ macro_rules! emit_channel_ready_event {
 	}
 }

+macro_rules! emit_pending_channel_ready_event {


This macro is only used once? Can we inline the code?

TheBlueMatt · 2023-03-29T21:00:48Z

lightning/src/ln/channel.rs

+							});
+						},
+						ChannelReadyStatus::PendingChannelReady => {
+							return Ok(ChainActionUpdates {


Won't we generate a new event on every block? We should probably just do it once.

TheBlueMatt · 2023-03-29T21:58:23Z

Actually, does it make sense to drop the event entirely from this PR and rely on #2098 ?

wpaulino · 2023-03-29T22:04:11Z

Great idea, would simplify this PR a good bit.

TheBlueMatt · 2023-03-29T22:08:49Z

Yea, we will need to port the logic around "if we restart and a channel is (a) set to manual-ready and (b) not yet set to allow-ready regenerate the event" to the event from that PR, but that seems fine?

wpaulino · 2023-03-29T22:12:31Z

Why would we need to regenerate it? The user should manually signal upon processing the event the first time.

TheBlueMatt · 2023-03-29T22:18:43Z

Hmmmm, I think there's a case where we (a) broadcast funding, (b) the funding confirmes, (c) we restart without persisting the channelmanager. On restart I think the event will be gone and we won't get it back - #2098 (comment)

wpaulino · 2023-03-29T22:59:14Z

Interesting, I'd assume the channel would get closed immediately upon restarting. Do we have any assurances that the channel will be in a functional state after the restart? The blocks would be replayed and the funding confirmation would be picked up, but what if we're using async monitor persistence? Would a channel_monitor_updated call need to be replayed on the channel to clear the MonitorUpdateInProgress bit (it was cleared when the monitor update completed, but we never got to persist the new state), and if so, is that documented somewhere?

TheBlueMatt · 2023-03-29T23:33:06Z

Interesting, I'd assume the channel would get closed immediately upon restarting.

I don't see why? We're past funding signed exchange, so its "locked in", and hopefully we don't have an HTLC exchange before we shut down.

Do we have any assurances that the channel will be in a functional state after the restart? The blocks would be replayed and the funding confirmation would be picked up, but what if we're using async monitor persistence? Would a channel_monitor_updated call need to be replayed on the channel to clear the MonitorUpdateInProgress bit (it was cleared when the monitor update completed, but we never got to persist the new state), and if so, is that documented somewhere?

Heh, I wasn't referring to anything this subtle lol. In #2098 the event is generated literally by the act of broadcasting the funding tx, if we don't have to broadcast the funding tx on restart because its been confirmed we wont regenerate the event, even though if this PR is based on it we'll be stuck waiting for the event to ready the channel.

johncantrell97 · 2023-03-30T22:05:09Z

Ok so is the latest thinking to:

remove the PendingChannelReady event entirely
make signal_channel_readiness used to trigger ChannelReady message and state regardless of config/set up (assuming funding_signed has already been sent) so that people can "opt in" to any-conf they want?
and then the manual signal flag just stops the automatic sending of ChannelReady but the end-user should use signal_channel_readiness whenever they want after receiving the ChannelPending event?

i guess for inbound channels (what we're concerned with here) we wont necessarily have the funding tx in our bitcoind when we receive ChannelPending where as with this PendingChannelReady we will have received confirmations already assuming we aren't using 0-conf inbound (which we wont).

it means our workflow would have to be something like maintain a queue of channel_ids / funding_txo that we push to when we receive a ChannelPending and then add another object that implements Listen and when we finally see it in a block we can trigger readiness if it passes our screen?

Or is there a simpler workflow we can use? or am I misunderstanding the suggestion?

TheBlueMatt · 2023-03-30T22:35:00Z

remove the PendingChannelReady event entirely

I think so.

make signal_channel_readiness used to trigger ChannelReady message and state regardless of config/set up (assuming funding_signed has already been sent) so that people can "opt in" to any-conf they want?

I think this is going to be a large overhaul and require a few new tests. I'm fine if you dont want to do this now, but I think it should be done long-term.

and then the manual signal flag just stops the automatic sending of ChannelReady but the end-user should use signal_channel_readiness whenever they want after receiving the ChannelPending event?

Yea.

it means our workflow would have to be something like maintain a queue of channel_ids / funding_txo that we push to when we receive a ChannelPending and then add another object that implements Listen and when we finally see it in a block we can trigger readiness if it passes our screen?

Yea, that's correct. If that's substantial additional complexity on your end we can do the event still or make the event appear on 1-conf irrespective of the channel config. We could also just make the event repeat, but I think that's a bit weird.

johncantrell97 · 2023-03-31T11:54:46Z

Yea, that's correct. If that's substantial additional complexity on your end we can do the event still or make the event appear on 1-conf irrespective of the channel config. We could also just make the event repeat, but I think that's a bit weird.

Ok, that's not too bad. It's much better than one option I had floated to avoid changing ldk all which was to basically filter and screen every single tx before we feed it to ldk. This allows us to only screen relevant txs.

Actually, couldn't we basically do this without any of these changes by just querying our channels on each block connected to see if there are any channels that are (funding_txo.is_some() && !is_outbound && confirmations < confirmations_required).

for each channel in that state we can check if the funding tx is in the block, if it is, screen the inputs and close the channel id needed.

as long as we don't accept 0 (or 1) conf inbound channels this should be fine?

TheBlueMatt · 2023-03-31T18:38:37Z

Right, as long as you dont accept 0/1 confs that would totally work.

wpaulino · 2023-04-03T16:28:34Z

Can this be closed now @johncantrell97? I think 1 conf should also work since you're checking transactions before providing them to LDK.

johncantrell97 · 2023-04-03T17:21:39Z

Ah, well i'm not actually doing it before providing to LDK. I guess I could change that but currently it's just adding a third Listen implementation to the SpvClient (ChannelManager, ChainMonitor, and this new ChannelScreener).

I guess maybe it's better to have a single Listener that can pipe them wherever/whenever it needs.

As for this PR, I think it's probably not so important for us anymore. Though having manual signaling would be much more explicit then this new approach.

Do you think it's worth implementing this feature without the event?

TheBlueMatt · 2023-04-03T17:44:55Z

I think manual signaling is kinda useful/nice to have, its not all that critical but no reason not to have it. Further having the ability to make a channel ready before confirmation is quite nice to have, and worth doing, but also more complicated.

TheBlueMatt · 2023-07-08T20:11:31Z

Any desire to keep working on this (no pressure, we can also close)?

wpaulino · 2023-10-31T16:36:52Z

@johncantrell97 closing for now, feel free to re-open if this would still be useful to you.

ZmnSCPxj-jr · 2024-01-18T04:24:20Z

@johncantrell97 Looks like this may be useful for a 0-conf channel open flow from a swap-in-potentiam address, what can be done to revive this PR?

ZmnSCPxj-jr · 2024-01-29T12:58:37Z

In addition, I need to get the scriptPubKey of the funding outpoint.

replace do_chain_event tuple with ChainActionUpdates struct

ebcaaea

johncantrell97 mentioned this pull request Mar 28, 2023

Support manual signaling of channel readiness #2109

Closed

johncantrell97 force-pushed the pending-channel-ready branch from e55264c to c345478 Compare March 28, 2023 21:10

wpaulino reviewed Mar 29, 2023

View reviewed changes

johncantrell97 force-pushed the pending-channel-ready branch from c345478 to 0137376 Compare March 29, 2023 13:52

johncantrell97 added 2 commits March 29, 2023 10:12

add manual signaling of channel readiness

7fcbd34

use ChannelReadyStatus enum instead of tuple

5c88c17

johncantrell97 force-pushed the pending-channel-ready branch from 0137376 to 5c88c17 Compare March 29, 2023 14:12

wpaulino reviewed Mar 29, 2023

View reviewed changes

TheBlueMatt reviewed Mar 29, 2023

View reviewed changes

wpaulino mentioned this pull request Mar 30, 2023

Add ChannelPending event emitted upon funding_signed #2098

Merged

wpaulino closed this Oct 31, 2023

ZmnSCPxj-jr mentioned this pull request Jan 29, 2024

Return the funding outpoint scriptPubKey in accept_inbound_channel #2857

Open

Support manual signaling of channel readiness #2137

Support manual signaling of channel readiness #2137

Conversation

johncantrell97 commented Mar 28, 2023

johncantrell97 commented Mar 28, 2023

codecov-commenter commented Mar 28, 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

wpaulino commented Mar 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Mar 29, 2023

wpaulino commented Mar 29, 2023

TheBlueMatt commented Mar 29, 2023

wpaulino commented Mar 29, 2023

TheBlueMatt commented Mar 29, 2023

wpaulino commented Mar 29, 2023

TheBlueMatt commented Mar 29, 2023

johncantrell97 commented Mar 30, 2023

TheBlueMatt commented Mar 30, 2023 • edited Loading

johncantrell97 commented Mar 31, 2023

TheBlueMatt commented Mar 31, 2023 • edited Loading

wpaulino commented Apr 3, 2023

johncantrell97 commented Apr 3, 2023

TheBlueMatt commented Apr 3, 2023

TheBlueMatt commented Jul 8, 2023 • edited Loading

wpaulino commented Oct 31, 2023

ZmnSCPxj-jr commented Jan 18, 2024

ZmnSCPxj-jr commented Jan 29, 2024

codecov-commenter commented Mar 28, 2023 •

edited

Loading

TheBlueMatt commented Mar 30, 2023 •

edited

Loading

TheBlueMatt commented Mar 31, 2023 •

edited

Loading

TheBlueMatt commented Jul 8, 2023 •

edited

Loading