df: rework closing logic

Trying to put all the disconnect logic into the same path was a dumb
idea. If you asked to reconnect but passed in an 'unsaved' channel, we
would not call the 'reconnect' code.

Instead, we make a differentiation between "unsaved" channels
(ones that we haven't received commitment tx for) and handle the
disconnect for these separate from where we want to do a reconnect.

lightningd/connect_control.c

@@ -297,11 +297,9 @@ static void peer_please_disconnect(struct lightningd *ld, const u8 *msg)
 		channel_cleanup_commands(c, "Reconnected");
 		channel_fail_reconnect(c, "Reconnected");
 	}
-	else {
-		/* v2 has unsaved channels, not uncommitted_chans */
-		c = unsaved_channel_by_id(ld, &id);
-		if (c)
-			channel_close_conn(c, "Reconnected");
+	else if ((c = unsaved_channel_by_id(ld, &id))) {
+		log_info(c->log, "Killing opening daemon: Reconnected");
+		channel_unsaved_close_conn(c, "Reconnected");
 	}
 }
 

lightningd/dual_open_control.c

@@ -52,63 +52,46 @@ static void channel_disconnect(struct channel *channel,
 
 	notify_disconnect(channel->peer->ld, &channel->peer->id);
 
-	if (channel_unsaved(channel)) {
-		log_debug(channel->log, "%s",
-			  "Unsaved peer failed."
-			  " Disconnecting and deleting channel.");
-		delete_channel(channel);
-		return;
-	}
-
-	if (reconnect)
+	if (!reconnect)
+		channel_set_owner(channel, NULL);
+	else
 		channel_fail_reconnect(channel, "%s: %s",
 				       channel->owner ?
 						channel->owner->name :
 						"dualopend-dead",
 				       desc);
-	else
-		channel_set_owner(channel, NULL);
 }
 
-void channel_close_conn(struct channel *channel, const char *why)
+void channel_unsaved_close_conn(struct channel *channel, const char *why)
 {
-	/* Close dualopend */
-	if (channel->owner) {
-		log_info(channel->log, "Killing dualopend: %s", why);
+	/* Gotta be unsaved */
+	assert(channel_unsaved(channel));
+	log_info(channel->log, "Unsaved peer failed."
+		 " Disconnecting and deleting channel. Reason: %s",
+		 why);
 
-		subd_release_channel(channel->owner, channel);
-		channel->owner = NULL;
-	}
+	notify_disconnect(channel->peer->ld, &channel->peer->id);
+	channel_cleanup_commands(channel, why);
 
-	channel_disconnect(channel, LOG_INFORM, false, why);
+	channel_set_owner(channel, NULL);
+	delete_channel(channel);
 }
 
-void channel_close_reconn(struct channel *channel, const char *why)
-{
-	/* Close the daemon */
-	if (channel->owner) {
-		log_info(channel->log, "Killing %s: %s",
-			 channel->owner->name, why);
-
-		subd_release_channel(channel->owner, channel);
-		channel->owner = NULL;
-	}
-
-	channel_disconnect(channel, LOG_INFORM, true, why);
-}
-
-static void channel_err_broken_reconn(struct channel *channel,
-				      const char *fmt, ...)
+static void channel_saved_err_broken_reconn(struct channel *channel,
+					    const char *fmt, ...)
 {
 	va_list ap;
 	const char *errmsg;
 
+	/* We only reconnect to 'saved' channel peers */
+	assert(!channel_unsaved(channel));
+
 	va_start(ap, fmt);
 	errmsg = tal_vfmt(tmpctx, fmt, ap);
 	va_end(ap);
 
 	log_broken(channel->log, "%s", errmsg);
-	channel_close_reconn(channel, errmsg);
+	channel_disconnect(channel, LOG_INFORM, true, errmsg);
 }
 
 static void channel_err_broken(struct channel *channel,
@@ -121,8 +104,11 @@ static void channel_err_broken(struct channel *channel,
 	errmsg = tal_vfmt(tmpctx, fmt, ap);
 	va_end(ap);
 
-	log_broken(channel->log, "%s", errmsg);
-	channel_close_conn(channel, errmsg);
+	if (channel_unsaved(channel)) {
+		log_broken(channel->log, "%s", errmsg);
+		channel_unsaved_close_conn(channel, errmsg);
+	} else
+		channel_disconnect(channel, LOG_BROKEN, false, errmsg);
 }
 
 void json_add_unsaved_channel(struct json_stream *response,
@@ -520,14 +506,8 @@ static void rbf_channel_hook_cb(struct rbf_channel_payload *payload STEALS)
 
 	tal_steal(tmpctx, payload);
 
-	if (!dualopend) {
-		channel_close_conn(channel, tal_fmt(tmpctx,
-				   "Lost conn to node %s"
-				   " awaiting rbf_channel callback",
-				   type_to_string(tmpctx, struct node_id,
-						  &channel->peer->id)));
+	if (!dualopend)
 		return;
-	}
 
 	tal_del_destructor2(dualopend, rbf_channel_remove_dualopend, payload);
 
@@ -663,16 +643,9 @@ openchannel2_hook_cb(struct openchannel2_payload *payload STEALS)
 	struct channel *channel = payload->channel;
 	u8 *msg;
 
-	/* Our daemon died, we fail and try to reconnect */
-	if (!dualopend) {
-		channel_close_conn(channel,
-				   tal_fmt(tmpctx, "Lost conn to node %s"
-				           " awaiting callback openchannel2",
-					   type_to_string(tmpctx,
-							  struct node_id,
-							  &channel->peer->id)));
+	/* Our daemon died! */
+	if (!dualopend)
 		return;
-	}
 
 	/* Free payload regardless of what happens next */
 	tal_steal(tmpctx, payload);
@@ -969,9 +942,9 @@ openchannel2_sign_hook_cb(struct openchannel2_psbt_payload *payload STEALS)
 send_msg:
 	/* Peer's gone away, let's try reconnecting */
 	if (!payload->dualopend) {
-		channel_err_broken_reconn(channel, "%s: dualopend daemon died"
-					  " before signed PSBT returned",
-					  channel->owner->name);
+		channel_saved_err_broken_reconn(channel,
+						"dualopend daemon died"
+						" before signed PSBT returned");
 		return;
 	}
 	tal_del_destructor2(payload->dualopend,
@@ -1499,15 +1472,17 @@ static void handle_peer_locked(struct subd *dualopend, const u8 *msg)
 	struct pubkey remote_per_commit;
 	struct channel *channel = dualopend->channel;
 
-	if (!fromwire_dualopend_peer_locked(msg, &remote_per_commit))
+	if (!fromwire_dualopend_peer_locked(msg, &remote_per_commit)) {
 		channel_internal_error(channel,
 				       "Bad WIRE_DUALOPEND_PEER_LOCKED: %s",
 				       tal_hex(msg, msg));
+		return;
+	}
 
-	/* Updates channel with the next per-commit point etc */
+	/* Updates channel with the next per-commit point etc, calls
+	 * channel_internal_error on failure */
 	if (!channel_on_funding_locked(channel, &remote_per_commit))
-		channel_internal_error(channel,
-				       "Got funding_locked twice");
+		return;
 
 	/* Remember that we got the lock-in */
 	wallet_channel_save(dualopend->ld->wallet, channel);
@@ -2572,7 +2547,7 @@ static void handle_commit_received(struct subd *dualopend,
 
 	if (channel->state == DUALOPEND_OPEN_INIT) {
 		if (peer_active_channel(channel->peer)) {
-			channel_err_broken_reconn(channel,
+			channel_saved_err_broken_reconn(channel,
 						  "Already have active"
 						  " channel with %s",
 						  type_to_string(tmpctx,
@@ -2627,13 +2602,12 @@ static void handle_commit_received(struct subd *dualopend,
 						       funding_ours,
 						       feerate_funding,
 						       psbt))) {
-			channel_err_broken_reconn(channel,
-						  "wallet_update_channel failed"
-						  " (chan %s)",
-						  type_to_string(
-							  tmpctx,
-							  struct channel_id,
-							  &channel->cid));
+			channel_internal_error(channel,
+					       "wallet_update_channel failed"
+					       " (chan %s)",
+					       type_to_string(tmpctx,
+							      struct channel_id,
+							      &channel->cid));
 			channel->open_attempt
 				= tal_free(channel->open_attempt);
 			return;
@@ -2852,9 +2826,9 @@ static void start_fresh_dualopend(struct peer *peer,
 					  take(&hsmfd), NULL);
 
 	if (!channel->owner) {
-		channel_err_broken_reconn(channel,
-					  "Running lightning_dualopend: %s",
-					  strerror(errno));
+		channel_internal_error(channel,
+				       "Running lightningd_dualopend: %s",
+				       strerror(errno));
 		return;
 	}
 

lightningd/dual_open_control.h

@@ -20,11 +20,8 @@ void dualopen_tell_depth(struct subd *dualopend,
 			 const struct bitcoin_txid *txid,
 			 u32 depth);
 
-void channel_close_conn(struct channel *channel,
-			const char *why);
-
-void channel_close_reconn(struct channel *channel,
-			  const char *why);
+/* Close connection to an unsaved channel */
+void channel_unsaved_close_conn(struct channel *channel, const char *why);
 
 void json_add_unsaved_channel(struct json_stream *response,
 			      const struct channel *channel);

lightningd/peer_control.c

@@ -1562,7 +1562,8 @@ static struct command_result *json_close(struct command *cmd,
 			return command_success(cmd, json_stream_success(cmd));
 		}
 		if ((channel = peer_unsaved_channel(peer))) {
-			channel_close_conn(channel, "close command called");
+			channel_unsaved_close_conn(channel,
+						   "close command called");
 			return command_success(cmd, json_stream_success(cmd));
 		}
 		return command_fail(cmd, LIGHTNINGD,
@@ -1867,7 +1868,7 @@ static struct command_result *json_disconnect(struct command *cmd,
 	}
 	channel = peer_unsaved_channel(peer);
 	if (channel) {
-		channel_close_conn(channel, "disconnect command");
+		channel_unsaved_close_conn(channel, "disconnect command");
 		return command_success(cmd, json_stream_success(cmd));
 	}
 	if (!peer->uncommitted_channel) {

lightningd/test/run-invoice-select-inchan.c

@@ -59,10 +59,6 @@ const char *channel_change_state_reason_str(enum state_change reason UNNEEDED)
 /* Generated stub for channel_cleanup_commands */
 void channel_cleanup_commands(struct channel *channel UNNEEDED, const char *why UNNEEDED)
 { fprintf(stderr, "channel_cleanup_commands called!\n"); abort(); }
-/* Generated stub for channel_close_conn */
-void channel_close_conn(struct channel *channel UNNEEDED,
-			const char *why UNNEEDED)
-{ fprintf(stderr, "channel_close_conn called!\n"); abort(); }
 /* Generated stub for channel_fail_forget */
 void channel_fail_forget(struct channel *channel UNNEEDED, const char *fmt UNNEEDED, ...)
 { fprintf(stderr, "channel_fail_forget called!\n"); abort(); }
@@ -115,6 +111,9 @@ bool channel_tell_depth(struct lightningd *ld UNNEEDED,
 				 const struct bitcoin_txid *txid UNNEEDED,
 				 u32 depth UNNEEDED)
 { fprintf(stderr, "channel_tell_depth called!\n"); abort(); }
+/* Generated stub for channel_unsaved_close_conn */
+void channel_unsaved_close_conn(struct channel *channel UNNEEDED, const char *why UNNEEDED)
+{ fprintf(stderr, "channel_unsaved_close_conn called!\n"); abort(); }
 /* Generated stub for command_fail */
 struct command_result *command_fail(struct command *cmd UNNEEDED, errcode_t code UNNEEDED,
 				    const char *fmt UNNEEDED, ...)

tests/test_connection.py

@@ -481,10 +481,7 @@ def test_reconnect_openingd(node_factory):
     l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
 
     # We should get a message about reconnecting.
-    if l2.config('experimental-dual-fund'):
-        l2.daemon.wait_for_log('Killing dualopend: Reconnected')
-    else:
-        l2.daemon.wait_for_log('Killing opening daemon: Reconnected')
+    l2.daemon.wait_for_log('Killing opening daemon: Reconnected')
     l2.daemon.wait_for_log('Handed peer, entering loop')
 
     # Should work fine.

wallet/db_postgres_sqlgen.c

@@ -1888,4 +1888,4 @@ struct db_query db_postgres_queries[] = {
 
 #endif /* LIGHTNINGD_WALLET_GEN_DB_POSTGRES */
 
-// SHA256STAMP:4404a89dadab8225901d024fd293d4b47b57ed4c6b5e7f00cf1fc9df0c345d57
+// SHA256STAMP:74b99da984e5e1872a7b3de32d3fc00efd7538e95e0509c6a839097522ea8a94

wallet/db_sqlite3_sqlgen.c

@@ -1888,4 +1888,4 @@ struct db_query db_sqlite3_queries[] = {
 
 #endif /* LIGHTNINGD_WALLET_GEN_DB_SQLITE3 */
 
-// SHA256STAMP:4404a89dadab8225901d024fd293d4b47b57ed4c6b5e7f00cf1fc9df0c345d57
+// SHA256STAMP:74b99da984e5e1872a7b3de32d3fc00efd7538e95e0509c6a839097522ea8a94

wallet/statements_gettextgen.po

@@ -1238,11 +1238,11 @@ msgstr ""
 msgid "not a valid SQL statement"
 msgstr ""
 
-#: wallet/test/run-wallet.c:1445
+#: wallet/test/run-wallet.c:1444
 msgid "SELECT COUNT(1) FROM channel_funding_inflights WHERE channel_id = ?;"
 msgstr ""
 
-#: wallet/test/run-wallet.c:1643
+#: wallet/test/run-wallet.c:1642
 msgid "INSERT INTO channels (id) VALUES (1);"
 msgstr ""
-#  SHA256STAMP:be8710aae042274c3b689ec333ee2d50d4116c3ea4d6b97338eaf88050137967
+#  SHA256STAMP:6f39707798a473b25ddf2706f514421533231682d47b0141df500193e9c27fd2

wallet/test/run-wallet.c

@@ -60,16 +60,15 @@ void broadcast_tx(struct chain_topology *topo UNNEEDED,
 				 bool success UNNEEDED,
 				 const char *err))
 { fprintf(stderr, "broadcast_tx called!\n"); abort(); }
-/* Generated stub for channel_close_conn */
-void channel_close_conn(struct channel *channel UNNEEDED,
-			const char *why UNNEEDED)
-{ fprintf(stderr, "channel_close_conn called!\n"); abort(); }
 /* Generated stub for channel_tell_depth */
 bool channel_tell_depth(struct lightningd *ld UNNEEDED,
 				 struct channel *channel UNNEEDED,
 				 const struct bitcoin_txid *txid UNNEEDED,
 				 u32 depth UNNEEDED)
 { fprintf(stderr, "channel_tell_depth called!\n"); abort(); }
+/* Generated stub for channel_unsaved_close_conn */
+void channel_unsaved_close_conn(struct channel *channel UNNEEDED, const char *why UNNEEDED)
+{ fprintf(stderr, "channel_unsaved_close_conn called!\n"); abort(); }
 /* Generated stub for command_fail */
 struct command_result *command_fail(struct command *cmd UNNEEDED, errcode_t code UNNEEDED,
 				    const char *fmt UNNEEDED, ...)