lightningd: peer state cleanup.

1. We explicitly assert what state we're coming from, to make transitions
   clearer.
2. Every transition has a state, even between owners while waiting for HSM.
3. Explictly step though getting the HSM signature on the funding tx
   before starting channeld, rather than doing it in parallel: makes
   states clearer.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

lightningd/channel/channel.c

@@ -267,8 +267,6 @@ static void handle_peer_funding_locked(struct peer *peer, const u8 *msg)
 			      "Funding locked twice");
 
 	peer->funding_locked[REMOTE] = true;
-	daemon_conn_send(&peer->master,
-			 take(towire_channel_received_funding_locked(peer)));
 
 	if (peer->funding_locked[LOCAL]) {
 		daemon_conn_send(&peer->master,
@@ -1322,7 +1320,6 @@ static struct io_plan *req_in(struct io_conn *conn, struct daemon_conn *master)
 		case WIRE_CHANNEL_INTERNAL_ERROR:
 		case WIRE_CHANNEL_PEER_WRITE_FAILED:
 		case WIRE_CHANNEL_PEER_READ_FAILED:
-		case WIRE_CHANNEL_RECEIVED_FUNDING_LOCKED:
 		case WIRE_CHANNEL_NORMAL_OPERATION:
 		case WIRE_CHANNEL_INIT:
 		case WIRE_CHANNEL_OFFER_HTLC_REPLY:

lightningd/channel/channel_wire.csv

@@ -10,8 +10,6 @@ channel_peer_write_failed,0x8010
 channel_peer_read_failed,0x8011
 channel_peer_bad_message,0x8012
 
-# Received funding_locked
-channel_received_funding_locked,1000
 # Received and sent funding_locked
 channel_normal_operation,1001
 

lightningd/peer_control.c

@@ -68,10 +68,15 @@ void peer_fail(struct peer *peer, const char *fmt, ...)
 		peer_reconnect(peer);
 }
 
-void peer_set_condition(struct peer *peer, enum peer_state state)
+void peer_set_condition(struct peer *peer, enum peer_state old_state,
+			enum peer_state state)
 {
 	log_info(peer->log, "state: %s -> %s",
 		 peer_state_name(peer->state), peer_state_name(state));
+	if (peer->state != old_state)
+		fatal("peer state %s should be %s",
+		      peer_state_name(peer->state), peer_state_name(old_state));
+
 	peer->state = state;
 }
 
@@ -92,7 +97,7 @@ void add_peer(struct lightningd *ld, u64 unique_id,
 	peer->funding_txid = NULL;
 	peer->seed = NULL;
 	peer->balance = NULL;
-	peer->state = INITIALIZING;
+	peer->state = UNINITIALIZED;
 
 	idname = type_to_string(peer, struct pubkey, id);
 
@@ -118,7 +123,7 @@ void add_peer(struct lightningd *ld, u64 unique_id,
 	/* Let gossip handle it from here. */
 	peer->owner = peer->ld->gossip;
 	tal_steal(peer->owner, peer);
-	peer_set_condition(peer, GOSSIPING);
+	peer_set_condition(peer, UNINITIALIZED, GOSSIPD);
 
 	msg = towire_gossipctl_new_peer(peer, peer->unique_id, cs);
 	subd_send_msg(peer->ld->gossip, take(msg));
@@ -427,6 +432,9 @@ struct funding_channel {
 
 	struct pubkey local_fundingkey, remote_fundingkey;
 	struct bitcoin_tx *funding_tx;
+
+	/* We prepare this when channeld exits, and hold until HSM replies. */
+	u8 *channel_init_msg;
 };
 
 static void fail_fundchannel_command(struct funding_channel *fc)
@@ -476,7 +484,6 @@ static enum watch_result funding_depth_cb(struct peer *peer,
 		peer->scid->outnum = peer->funding_outnum;
 		tal_free(loc);
 
-		peer_set_condition(peer, OPENING_SENT_LOCKED);
 		subd_send_msg(peer->owner,
 			      take(towire_channel_funding_locked(peer, peer->scid)));
 	}
@@ -490,6 +497,9 @@ static enum watch_result funding_depth_cb(struct peer *peer,
 	return DELETE_WATCH;
 }
 
+/* FIXME: Reshuffle. */
+static void peer_start_channeld(struct peer *peer, const u8 *initmsg);
+
 static bool opening_got_hsm_funding_sig(struct subd *hsm, const u8 *resp,
 					const int *fds,
 					struct funding_channel *fc)
@@ -506,8 +516,6 @@ static bool opening_got_hsm_funding_sig(struct subd *hsm, const u8 *resp,
 		fatal("HSM gave %zu sigs, needed %zu",
 		      tal_count(sigs), tal_count(tx->input));
 
-	peer_set_condition(fc->peer, OPENING_AWAITING_LOCKIN);
-
 	/* Create input parts from signatures. */
 	for (i = 0; i < tal_count(tx->input); i++) {
 		struct pubkey key;
@@ -522,6 +530,9 @@ static bool opening_got_hsm_funding_sig(struct subd *hsm, const u8 *resp,
 			= bitcoin_witness_p2wpkh(tx, &sigs[i], &key);
 	}
 
+	peer_set_condition(fc->peer,
+			   GETTING_SIG_FROM_HSM, OPENINGD_AWAITING_LOCKIN);
+
 	/* Send it out and watch for confirms. */
 	broadcast_tx(hsm->ld->topology, fc->peer, tx, funding_broadcast_failed);
 	watch_tx(fc->peer, fc->peer->ld->topology, fc->peer, tx,
@@ -531,6 +542,10 @@ static bool opening_got_hsm_funding_sig(struct subd *hsm, const u8 *resp,
 	 * harder. */
 	tal_del_destructor(fc, fail_fundchannel_command);
 	command_success(fc->cmd, null_response(fc->cmd));
+
+	/* Start normal channel daemon. */
+	peer_start_channeld(fc->peer, fc->channel_init_msg);
+
 	tal_free(fc);
 	return true;
 }
@@ -1229,11 +1244,9 @@ static int channel_msg(struct subd *sd, const u8 *msg, const int *unused)
 	enum channel_wire_type t = fromwire_peektype(msg);
 
 	switch (t) {
-	case WIRE_CHANNEL_RECEIVED_FUNDING_LOCKED:
-		peer_set_condition(sd->peer, OPENING_RCVD_LOCKED);
-		break;
 	case WIRE_CHANNEL_NORMAL_OPERATION:
-		peer_set_condition(sd->peer, NORMAL);
+		peer_set_condition(sd->peer,
+				   CHANNELD_AWAITING_LOCKIN, CHANNELD_NORMAL);
 		break;
 	case WIRE_CHANNEL_ACCEPTED_HTLC:
 		return peer_accepted_htlc(sd->peer, msg);
@@ -1298,23 +1311,21 @@ static bool peer_start_channeld_hsmfd(struct subd *hsm, const u8 *resp,
 	cds->peer->fd = -1;
 
 	log_debug(cds->peer->log, "Waiting for funding confirmations");
+	peer_set_condition(cds->peer, GETTING_HSMFD, CHANNELD_AWAITING_LOCKIN);
+
 	/* We don't expect a response: we are triggered by funding_depth_cb. */
 	subd_send_msg(cds->peer->owner, take(cds->initmsg));
 	tal_free(cds);
 	return true;
 }
 
-/* opening is done, start lightningd_channel for peer. */
-static void peer_start_channeld(struct peer *peer,
-				const struct channel_config *their_config,
-				const struct crypto_state *crypto_state,
-				const secp256k1_ecdsa_signature *commit_sig,
-				const struct pubkey *remote_fundingkey,
-				const struct basepoints *theirbase,
-				const struct pubkey *their_per_commit_point)
+/* opening is done, start lightningd_channel for peer.
+ * Steals initmsg: caller prepares it because it has the information to
+ * construct it.
+ */
+static void peer_start_channeld(struct peer *peer, const u8 *initmsg)
 {
 	struct channeld_start *cds = tal(peer, struct channeld_start);
-	struct config *cfg = &peer->ld->dstate.config;
 	/* Unowned: back to being owned by main daemon. */
 	peer->owner = NULL;
 	tal_steal(peer->ld, peer);
@@ -1327,30 +1338,8 @@ static void peer_start_channeld(struct peer *peer,
 	peer->balance[!peer->funder] = peer->push_msat;
 
 	cds->peer = peer;
-	/* Prepare init message now while we have access to all the data. */
-	cds->initmsg = towire_channel_init(cds,
-					   peer->funding_txid,
-					   peer->funding_outnum,
-					   &peer->our_config,
-					   their_config,
-					   commit_sig,
-					   crypto_state,
-					   remote_fundingkey,
-					   &theirbase->revocation,
-					   &theirbase->payment,
-					   &theirbase->delayed_payment,
-					   their_per_commit_point,
-					   peer->funder == LOCAL,
-					   cfg->fee_base,
-					   cfg->fee_per_satoshi,
-					   peer->funding_satoshi,
-					   peer->push_msat,
-					   peer->seed,
-					   &peer->ld->dstate.id,
-					   peer->id,
-					   time_to_msec(cfg->commit_time),
-					   cfg->deadline_blocks
-		);
+	cds->initmsg = tal_steal(cds, initmsg);
+	peer_set_condition(peer, OPENINGD_AWAITING_LOCKIN, GETTING_HSMFD);
 
 	/* Get fd from hsm. */
 	subd_req(peer, peer->ld->hsm,
@@ -1369,6 +1358,7 @@ static bool opening_release_tx(struct subd *opening, const u8 *resp,
 	secp256k1_ecdsa_signature commit_sig;
 	struct pubkey their_per_commit_point;
 	struct basepoints theirbase;
+	struct config *cfg = &fc->peer->ld->dstate.config;
 	/* FIXME: marshal code wants array, not array of pointers. */
 	struct utxo *utxos = tal_arr(fc, struct utxo, tal_count(fc->utxomap));
 
@@ -1401,15 +1391,36 @@ static bool opening_release_tx(struct subd *opening, const u8 *resp,
 					 &fc->remote_fundingkey,
 					 utxos);
 	tal_free(utxos);
+
+	/* Prepare this while we have the information. */
+	fc->channel_init_msg = towire_channel_init(fc,
+						   fc->peer->funding_txid,
+						   fc->peer->funding_outnum,
+						   &fc->peer->our_config,
+						   &their_config,
+						   &commit_sig,
+						   &crypto_state,
+						   &fc->remote_fundingkey,
+						   &theirbase.revocation,
+						   &theirbase.payment,
+						   &theirbase.delayed_payment,
+						   &their_per_commit_point,
+						   fc->peer->funder == LOCAL,
+						   cfg->fee_base,
+						   cfg->fee_per_satoshi,
+						   fc->peer->funding_satoshi,
+						   fc->peer->push_msat,
+						   fc->peer->seed,
+						   &fc->peer->ld->dstate.id,
+						   fc->peer->id,
+						   time_to_msec(cfg->commit_time),
+						   cfg->deadline_blocks);
+
+	fc->peer->owner = NULL;
+	peer_set_condition(fc->peer, OPENINGD, GETTING_SIG_FROM_HSM);
 	subd_req(fc, fc->peer->ld->hsm, take(msg), -1, 0,
 		 opening_got_hsm_funding_sig, fc);
 
-	/* Start normal channel daemon. */
-	peer_start_channeld(fc->peer,
-			    &their_config, &crypto_state, &commit_sig,
-			    &fc->remote_fundingkey, &theirbase,
-			    &their_per_commit_point);
-
 	/* Tell opening daemon to exit. */
 	return false;
 }
@@ -1460,6 +1471,8 @@ static bool opening_accept_finish_response(struct subd *opening,
 	struct crypto_state crypto_state;
 	struct basepoints theirbase;
 	struct pubkey remote_fundingkey, their_per_commit_point;
+	struct config *cfg = &peer->ld->dstate.config;
+	u8 *initmsg;
 
 	log_debug(peer->log, "Got opening_accept_finish_response");
 	assert(tal_count(fds) == 1);
@@ -1482,12 +1495,32 @@ static bool opening_accept_finish_response(struct subd *opening,
 		return false;
 	}
 
-	peer_set_condition(peer, OPENING_AWAITING_LOCKIN);
+	initmsg = towire_channel_init(peer,
+				      peer->funding_txid,
+				      peer->funding_outnum,
+				      &peer->our_config,
+				      &their_config,
+				      &first_commit_sig,
+				      &crypto_state,
+				      &remote_fundingkey,
+				      &theirbase.revocation,
+				      &theirbase.payment,
+				      &theirbase.delayed_payment,
+				      &their_per_commit_point,
+				      peer->funder == LOCAL,
+				      cfg->fee_base,
+				      cfg->fee_per_satoshi,
+				      peer->funding_satoshi,
+				      peer->push_msat,
+				      peer->seed,
+				      &peer->ld->dstate.id,
+				      peer->id,
+				      time_to_msec(cfg->commit_time),
+				      cfg->deadline_blocks);
 
 	/* On to normal operation! */
-	peer_start_channeld(peer, &their_config, &crypto_state,
-			    &first_commit_sig, &remote_fundingkey, &theirbase,
-			    &their_per_commit_point);
+	peer->owner = NULL;
+	peer_start_channeld(peer, initmsg);
 
 	/* Tell opening daemon to exit. */
 	return false;
@@ -1510,6 +1543,9 @@ static bool opening_accept_reply(struct subd *opening, const u8 *reply,
 	watch_txid(peer, peer->ld->topology, peer, peer->funding_txid,
 		   funding_depth_cb, NULL);
 
+	/* It's about to send out funding_signed, so set this now. */
+	peer_set_condition(peer, OPENINGD, OPENINGD_AWAITING_LOCKIN);
+
 	/* Tell it we're watching. */
 	subd_req(peer, opening, towire_opening_accept_finish(reply),
 		 -1, 1,
@@ -1579,7 +1615,7 @@ void peer_accept_open(struct peer *peer,
 		return;
 	}
 
-	peer_set_condition(peer, OPENING_NOT_LOCKED);
+	peer_set_condition(peer, GOSSIPD, OPENINGD);
 	peer->owner = new_subd(ld, ld, "lightningd_opening", peer,
 			       opening_wire_type_name,
 			       NULL, peer_owner_finished,
@@ -1651,7 +1687,7 @@ static bool gossip_peer_released(struct subd *gossip,
 		fatal("Gossup daemon release gave %"PRIu64" not %"PRIu64,
 		      id, fc->peer->unique_id);
 
-	peer_set_condition(fc->peer, OPENING_NOT_LOCKED);
+	peer_set_condition(fc->peer, GOSSIPD, OPENINGD);
 	opening = new_subd(fc->peer->ld, ld,
 			   "lightningd_opening", fc->peer,
 			   opening_wire_type_name,

lightningd/peer_control.h

@@ -71,30 +71,39 @@ struct peer {
 
 static inline bool peer_can_add_htlc(const struct peer *peer)
 {
-	return peer->state == NORMAL;
+	return peer->state == CHANNELD_NORMAL;
 }
 
 static inline bool peer_can_remove_htlc(const struct peer *peer)
 {
-	return peer->state == NORMAL
-		|| peer->state == SHUTDOWN_SENT
-		|| peer->state == SHUTDOWN_RCVD
-		|| peer->state == ONCHAIN_THEIR_UNILATERAL
-		|| peer->state == ONCHAIN_OUR_UNILATERAL;
+	return peer->state == CHANNELD_NORMAL
+		|| peer->state == SHUTDOWND_SENT
+		|| peer->state == SHUTDOWND_RCVD
+		|| peer->state == ONCHAIND_THEIR_UNILATERAL
+		|| peer->state == ONCHAIND_OUR_UNILATERAL;
 }
 
 static inline bool peer_on_chain(const struct peer *peer)
 {
-	return peer->state == ONCHAIN_CHEATED
-		|| peer->state == ONCHAIN_THEIR_UNILATERAL
-		|| peer->state == ONCHAIN_OUR_UNILATERAL
-		|| peer->state == ONCHAIN_MUTUAL;
+	return peer->state == ONCHAIND_CHEATED
+		|| peer->state == ONCHAIND_THEIR_UNILATERAL
+		|| peer->state == ONCHAIND_OUR_UNILATERAL
+		|| peer->state == ONCHAIND_MUTUAL;
 }
 
-/* Do we need to remember anything about this peer? */
+/* BOLT #2:
+ *
+ * On disconnection, the funder MUST remember the channel for
+ * reconnection if it has broadcast the funding transaction, otherwise it
+ * MUST NOT.
+ *
+ * On disconnection, the non-funding node MUST remember the channel for
+ * reconnection if it has sent the `funding_signed` message, otherwise
+ * it MUST NOT.
+ */
 static inline bool peer_persists(const struct peer *peer)
 {
-	return peer->state > OPENING_NOT_LOCKED;
+	return peer->state >= CHANNELD_AWAITING_LOCKIN;
 }
 
 struct peer *peer_by_unique_id(struct lightningd *ld, u64 unique_id);
@@ -113,6 +122,7 @@ void add_peer(struct lightningd *ld, u64 unique_id,
 PRINTF_FMT(2,3) void peer_fail(struct peer *peer, const char *fmt, ...);
 
 const char *peer_state_name(enum peer_state state);
-void peer_set_condition(struct peer *peer, enum peer_state state);
+void peer_set_condition(struct peer *peer, enum peer_state oldstate,
+			enum peer_state state);
 void setup_listeners(struct lightningd *ld);
 #endif /* LIGHTNING_LIGHTNINGD_PEER_CONTROL_H */

lightningd/peer_state.h

@@ -3,35 +3,43 @@
 #include "config.h"
 
 enum peer_state {
-	/* Not important: we can forget about peers in these states. */
-	INITIALIZING,
-	GOSSIPING,
-
-	/* Negotiating channel opening */
-	OPENING_NOT_LOCKED,
-	/* Waiting for funding tx to lock in. */
-	OPENING_AWAITING_LOCKIN,
-	/* Opening, have received funding_locked (not sent). */
-	OPENING_RCVD_LOCKED,
-	/* Opening, have sent funding_locked (not received). */
-	OPENING_SENT_LOCKED,
+	UNINITIALIZED,
+
+	/* In gossip daemon. */
+	GOSSIPD,
+
+	/* Negotiating channel opening: in opening daemon */
+	OPENINGD,
+
+	/* Getting signature from HSM for funding tx (funder only). */
+	GETTING_SIG_FROM_HSM,
+
+	/* Waiting for funding tx to lock in: either have broadcast, or
+	 * have sent `funding_signed`. */
+	OPENINGD_AWAITING_LOCKIN,
+
+	/* Getting HSM fd for channeld. */
+	GETTING_HSMFD,
+
+	/* In channeld, still waiting for lockin. */
+	CHANNELD_AWAITING_LOCKIN,
 
 	/* Normal operating state. */
-	NORMAL,
+	CHANNELD_NORMAL,
 
 	/* We are closing, pending HTLC resolution. */
-	SHUTDOWN_SENT,
+	SHUTDOWND_SENT,
 	/* Both are closing, pending HTLC resolution. */
-	SHUTDOWN_RCVD,
+	SHUTDOWND_RCVD,
 
 	/* Exchanging signatures on closing tx. */
-	CLOSING_SIGEXCHANGE,
+	CLOSINGD_SIGEXCHANGE,
 
 	/* Various onchain states. */
-	ONCHAIN_CHEATED,
-	ONCHAIN_THEIR_UNILATERAL,
-	ONCHAIN_OUR_UNILATERAL,
-	ONCHAIN_MUTUAL
+	ONCHAIND_CHEATED,
+	ONCHAIND_THEIR_UNILATERAL,
+	ONCHAIND_OUR_UNILATERAL,
+	ONCHAIND_MUTUAL
 };
 
 #endif /* LIGHTNING_LIGHTNINGD_PEER_STATE_H */

tests/test_lightningd.py

@@ -180,8 +180,8 @@ class LightningDTests(BaseLightningDTests):
 
         l1.bitcoin.rpc.generate(6)
 
-        l1.daemon.wait_for_log('-> NORMAL')
-        l2.daemon.wait_for_log('-> NORMAL')
+        l1.daemon.wait_for_log('-> CHANNELD_NORMAL')
+        l2.daemon.wait_for_log('-> CHANNELD_NORMAL')
 
     def test_connect(self):
         l1,l2 = self.connect()
@@ -189,11 +189,11 @@ class LightningDTests(BaseLightningDTests):
         p1 = l1.rpc.getpeer(l2.info['id'], 'info')
         p2 = l2.rpc.getpeer(l1.info['id'], 'info')
 
-        assert p1['state'] == 'GOSSIPING'
-        assert p2['state'] == 'GOSSIPING'
+        assert p1['state'] == 'GOSSIPD'
+        assert p2['state'] == 'GOSSIPD'
 
         # It should have gone through these steps
-        assert 'state: INITIALIZING -> GOSSIPING' in p1['log']
+        assert 'state: UNINITIALIZED -> GOSSIPD' in p1['log']
 
         # Both should still be owned by gossip
         assert p1['owner'] == 'lightningd_gossip'
@@ -203,8 +203,8 @@ class LightningDTests(BaseLightningDTests):
         l1,l2 = self.connect()
 
         self.fund_channel(l1, l2, 10**6)
-        l1.daemon.wait_for_log('-> NORMAL')
-        l2.daemon.wait_for_log('-> NORMAL')
+        l1.daemon.wait_for_log('-> CHANNELD_NORMAL')
+        l2.daemon.wait_for_log('-> CHANNELD_NORMAL')
 
         secret = '1de08917a61cb2b62ed5937d38577f6a7bfe59c176781c6d8128018e8b5ccdfd'
         rhash = l1.rpc.dev_rhash(secret)['rhash']

tests/utils.py

@@ -265,8 +265,8 @@ class LightningNode(object):
             #fut.result(timeout=5)
 
             # Now wait for confirmation
-            self.daemon.wait_for_log("STATE_NORMAL")
-            remote_node.daemon.wait_for_log("STATE_NORMAL")
+            self.daemon.wait_for_log("-> CHANNELD_NORMAL|STATE_NORMAL")
+            remote_node.daemon.wait_for_log("-> CHANNELD_NORMAL|STATE_NORMAL")
 
         if async:
             return self.executor.submit(wait_connected)
@@ -282,5 +282,5 @@ class LightningNode(object):
         self.daemon.wait_for_log('sendrawtx exit 0, gave')
         time.sleep(1)
         self.bitcoin.rpc.generate(6)
-        self.daemon.wait_for_log('-> NORMAL')
+        self.daemon.wait_for_log('-> CHANNELD_NORMAL|STATE_NORMAL')