Shutdown while awaiting lockin

[rustyrussell]

No description provided.

[ZmnSCPxj]

Looks OK, mostly trusting Rusty superior knowledge of state machine though.

[ZmnSCPxj]

ACK ed767c8

Does this address #1308? Yes

[cdecker]

The commits should probably be reordered (test needs to move down) otherwise we broke the bisectability of master.

FWIW, I don't think this addresses #1308 since that fails on the transition after channeld_shutting_down:

lightning/lightningd/channel_control.c

Lines 104 to 106 in 26f7014

	/* This sets channel->owner, closes down channeld. */
	peer_start_closingd(channel, &cs, gossip_index, fds[0], fds[1], false);
	channel_set_state(channel, CHANNELD_SHUTTING_DOWN, CLOSINGD_SIGEXCHANGE);

[cdecker]

Might a bit too permissive now? We probably also don't want to accept in state sigexchange.

[rustyrussell]

If we're in channeld (which we are), we have to be in AWAITING_LOCKIN or NORMAL or SHUTTING_DOWN. So this is actually safe.

[ZmnSCPxj]

I think the PR makes the state machine transition to CHANNELD_SHUTTING_DOWN from CHANNELD_AWAITING_LOCKIN, so that the code you quoted becomes valid.

[cdecker]

Oh I see, we're forcing the channel state into shutting down, then tell channeld to die, and then start closingd directly afterwards. That was a level of indirection too many for me 😉

IMHO there still is a tiny probability that there is a channel state transition between receiving the shutdown and channeld dying (channel confirmation exactly at the wrong moment). So I'd prefer widening the condition or refusing to transition to a prior state after shutdown was reached.

[rustyrussell]

It's channeld which tells us that it received shutdown, so it can't die at the same time?

The order is:

1. channeld tells us it got the remote shutdown, we save the scriptpubkey for closing and update state to CHANNELD_SHUTTING_DOWN if not already done.
2. When channeld has cleared all the HTLCs, it tells us shutdown is complete, we update state to CLOSINGD_SIGEXCHANGE and start closingd (killing channeld in the process).

Note that if we reconnect, the peer can re-send shutdown so we need to handle the case where we're already in CHANNELD_SHUTTING_DOWN.

We could eliminate the CHANNELD_SHUTTING_DOWN state and use the existence of the remote closing scriptpubkey as a flag, but as shutdown can be extended, I think having the state is clearer to users.

[rustyrussell]

Oh, and on test order: I've taken to leaving the test-then-fix order in my pushes, using unittest.expectedFail() and removing it in the fix. That proves that I actually tested it before and after :)