Implement channel closing for block_disconnected on ChainListener + test #47

ariard · 2018-07-17T02:25:43Z

I implemented taking care of potential deadlock between channel_state and call to close_channel. Test is really basic, creating a channel, disconnecting blocks and checking it going to LocalShutdownSent state.

ariard · 2018-07-17T02:29:33Z

src/ln/channelmanager.rs

+		for channel in closing_channel {
+			match self.close_channel(&channel) {
+				Ok(_msg) => {}
+				Err(_e) => {} //TODO: if error is stir up by pending HTLC, after timer, try to call close_channel again ?


I'm thinking we have 2 way to handle errors (like pending HTLCs) on channel close: 1/ we return a vec of tuple (channel_id, false) to caller of block_disconnected, 2/ we push a timer on event with a new call to block_disconnected. Former is cumbersome for the caller, latter for dealing internally with timer events.

I think we need to be more aggressive. Generally, when dealing with a confirmation-tx-unconf error, we'd really prefer to force-close and initiate the race instead of letting our counterparty participate in the shutdown, as its more of a last-ditch thing. Should be able to just do a force_shutdown() and remove the channel from our channels maps, I think.

TheBlueMatt · 2018-07-18T20:02:58Z

src/ln/channel.rs

@@ -1844,6 +1844,15 @@ impl Channel {
 		self.channel_update_count
 	}

+	pub fn get_channel_id(&self) -> Uint256 {


This is redundant with channel_id(&self), a few lines up.

TheBlueMatt · 2018-07-18T20:09:12Z

src/ln/channel.rs

+	}
+
+	#[test]
+	pub fn get_channel_state(&self) -> u32 {


Hmm, I'd prefer this not be exposed, especially given we may want to change the enum values at some point without having to recall that the tests use it. It should be easy to instead test for an outbound Shutdown message having been generated to see the close occurring instead of getting the channel state explicitly.

TheBlueMatt · 2018-07-18T20:48:33Z

src/ln/channelmanager.rs

+		for channel in closing_channel {
+			match self.close_channel(&channel) {
+				Ok(_msg) => {}
+				Err(_e) => {} //TODO: if error is stir up by pending HTLC, after timer, try to call close_channel again ?


I think we need to be more aggressive. Generally, when dealing with a confirmation-tx-unconf error, we'd really prefer to force-close and initiate the race instead of letting our counterparty participate in the shutdown, as its more of a last-ditch thing. Should be able to just do a force_shutdown() and remove the channel from our channels maps, I think.

ariard · 2018-07-19T01:43:14Z

src/ln/channel.rs

@@ -231,7 +231,7 @@ const BOTH_SIDES_SHUTDOWN_MASK: u32 = (ChannelState::LocalShutdownSent as u32 |

 // TODO: We should refactor this to be an Inbound/OutboundChannel until initial setup handshaking
 // has been completed, and then turn into a Channel to get compiler-time enforcement of things like
-// calling get_channel_id() before we're set up or things like get_outbound_funding_signed on an
+// calling channel_id() before we're set up or things like get_outbound_funding_signed on an


The reason at first why I added get_channel_id(), but I think that channel_id() is that.

TheBlueMatt

Cool, looking good!

TheBlueMatt · 2018-07-19T17:49:51Z

src/ln/channelmanager.rs

 		for channel in channel_state.by_id.values_mut() {
 			if channel.block_disconnected(header) {
-				//TODO Close channel here
+				channel.force_shutdown();


You need to broadcast the transactions returned here.

TheBlueMatt · 2018-07-19T17:51:10Z

src/ln/channelmanager.rs

-				//TODO Close channel here
+				channel.force_shutdown();
+				if let Some(chan_id) = channel.get_short_channel_id() {
+					closed_channel.push(chan_id.clone());


You should be able to remove this by changing the loop to a retain().

ariard · 2018-07-20T00:48:46Z

Updated, spec was unclear on what doing in this case, what theoretically prevent us to just "freeze" the channel and put it again alive if it gets again enough confirmations ? (

TheBlueMatt · 2018-07-20T01:09:37Z

src/ln/channelmanager.rs

 	fn block_disconnected(&self, header: &BlockHeader) {
+		let mut closed_channel = Vec::new();


I think you can skip this, too, if you switch back to the borrow_parts() (since borrow_parts() is used to let you get around the borrow checker for stuff like this :))

Sorry are you sure it works with closure? I get a E500 or E0499 each time, I'm still a beginner in magic spells against borrowck

This diff works for me (though no guarantees about older rustcs, lets see what travis says): https://0bin.net/paste/at3mwfdpXIeugIQ4#X62nrP-X/OYnkLEJ75jrbpR9FRY8DxcOudBzraNDW98

ariard · 2018-07-20T02:39:59Z

Updated, thanks, get it works on 1.22 and my 1.28-nightly!

ariard commented Jul 17, 2018

View reviewed changes

TheBlueMatt reviewed Jul 18, 2018

View reviewed changes

ariard force-pushed the block_disconnected_close_chan branch from 1e5c0cb to 286cc3a Compare July 19, 2018 01:41

ariard commented Jul 19, 2018

View reviewed changes

TheBlueMatt reviewed Jul 19, 2018

View reviewed changes

ariard force-pushed the block_disconnected_close_chan branch from 286cc3a to 11adae4 Compare July 20, 2018 00:45

TheBlueMatt reviewed Jul 20, 2018

View reviewed changes

Implement channel closing for block_disconnected on ChainListener + test

1b8544f

ariard force-pushed the block_disconnected_close_chan branch from 11adae4 to 1b8544f Compare July 20, 2018 02:38

TheBlueMatt merged commit de523c4 into lightningdevkit:master Jul 20, 2018

ariard deleted the block_disconnected_close_chan branch September 7, 2018 22:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Implement channel closing for block_disconnected on ChainListener + test #47

Implement channel closing for block_disconnected on ChainListener + test #47

Uh oh!

ariard commented Jul 17, 2018

Uh oh!

ariard Jul 17, 2018

Uh oh!

TheBlueMatt Jul 18, 2018

Uh oh!

TheBlueMatt Jul 18, 2018

Uh oh!

TheBlueMatt Jul 18, 2018

Uh oh!

TheBlueMatt Jul 18, 2018

Uh oh!

ariard Jul 19, 2018

Uh oh!

TheBlueMatt left a comment

Uh oh!

TheBlueMatt Jul 19, 2018

Uh oh!

TheBlueMatt Jul 19, 2018

Uh oh!

ariard commented Jul 20, 2018

Uh oh!

TheBlueMatt Jul 20, 2018

Uh oh!

ariard Jul 20, 2018 •

edited

Loading

Uh oh!

TheBlueMatt Jul 20, 2018

Uh oh!

ariard commented Jul 20, 2018

Uh oh!

Uh oh!

		fn block_disconnected(&self, header: &BlockHeader) {
		let mut closed_channel = Vec::new();

Implement channel closing for block_disconnected on ChainListener + test #47

Implement channel closing for block_disconnected on ChainListener + test #47

Uh oh!

Conversation

ariard commented Jul 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard commented Jul 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard Jul 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard commented Jul 20, 2018

Uh oh!

Uh oh!

ariard Jul 20, 2018 •

edited

Loading