openingd: disconnect from peer when an error occurs.

openingd currently holds the connection to idle peers, but we're about
to change that: it will only look after peers which are actively
opening a connection.  We can start this process by disconnecting
whenever we have a negotiation failure.

We could stay connected if we wanted to, but that would be up to
connectd to decide.  Right now it's easier if we disconnect from any
idle peer once it's been active.

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

lightningd/opening_control.c

@@ -561,22 +561,25 @@ opening_funder_failed_cancel_commands(struct uncommitted_channel *uc,
 	 */
 	uc->fc = tal_free(uc->fc);
 }
-static void opening_funder_failed(struct subd *openingd, const u8 *msg,
-				  struct uncommitted_channel *uc)
+
+static void openingd_failed(struct subd *openingd, const u8 *msg,
+			    struct uncommitted_channel *uc)
 {
 	char *desc;
 
-	if (!fromwire_openingd_funder_failed(msg, msg, &desc)) {
+	if (!fromwire_openingd_failed(msg, msg, &desc)) {
 		log_broken(uc->log,
-			   "bad OPENING_FUNDER_FAILED %s",
+			   "bad OPENINGD_FAILED %s",
 			   tal_hex(tmpctx, msg));
-		was_pending(command_fail(uc->fc->cmd, LIGHTNINGD,
-					 "bad OPENING_FUNDER_FAILED %s",
-					 tal_hex(uc->fc->cmd, msg)));
+		if (uc->fc)
+			was_pending(command_fail(uc->fc->cmd, LIGHTNINGD,
+						 "bad OPENINGD_FAILED %s",
+						 tal_hex(uc->fc->cmd, msg)));
 		tal_free(uc);
 		return;
 	}
 
+	/* Noop if we're not funder. */
 	opening_funder_failed_cancel_commands(uc, desc);
 }
 
@@ -853,14 +856,8 @@ static unsigned int openingd_msg(struct subd *openingd,
 		}
 		opening_funder_start_replied(openingd, msg, fds, uc->fc);
 		return 0;
-	case WIRE_OPENINGD_FUNDER_FAILED:
-		if (!uc->fc) {
-			log_unusual(openingd->log, "Unexpected FUNDER_FAILED %s",
-				   tal_hex(tmpctx, msg));
-			tal_free(openingd);
-			return 0;
-		}
-		opening_funder_failed(openingd, msg, uc);
+	case WIRE_OPENINGD_FAILED:
+		openingd_failed(openingd, msg, uc);
 		return 0;
 
 	case WIRE_OPENINGD_FUNDEE:

openingd/openingd.c

@@ -102,10 +102,9 @@ struct state {
 	struct feature_set *our_features;
 };
 
-/*~ If we can't agree on parameters, we fail to open the channel.  If we're
- * the opener, we need to tell lightningd, otherwise it never really notices. */
-static void negotiation_aborted(struct state *state, bool am_opener,
-				const char *why)
+/*~ If we can't agree on parameters, we fail to open the channel.
+ *  Tell lightningd why. */
+static void NORETURN negotiation_aborted(struct state *state, const char *why)
 {
 	status_debug("aborted opening negotiation: %s", why);
 	/*~ The "billboard" (exposed as "status" in the JSON listpeers RPC
@@ -116,29 +115,14 @@ static void negotiation_aborted(struct state *state, bool am_opener,
 	 * status. */
 	peer_billboard(true, why);
 
-	/* If necessary, tell master that funding failed. */
-	if (am_opener) {
-		u8 *msg = towire_openingd_funder_failed(NULL, why);
-		wire_sync_write(REQ_FD, take(msg));
-	}
-
-	/* Default is no shutdown_scriptpubkey: free any leftover ones. */
-	state->upfront_shutdown_script[LOCAL]
-		= tal_free(state->upfront_shutdown_script[LOCAL]);
-	state->upfront_shutdown_script[REMOTE]
-		= tal_free(state->upfront_shutdown_script[REMOTE]);
-
-	/*~ Reset state.  We keep gossipping with them, even though this open
-	* failed. */
-	memset(&state->channel_id, 0, sizeof(state->channel_id));
-	state->channel = tal_free(state->channel);
-
-	state->channel_type = tal_free(state->channel_type);
+	/* Tell master that funding failed. */
+	wire_sync_write(REQ_FD, take(towire_openingd_failed(NULL, why)));
+	exit(0);
 }
 
 /*~ For negotiation failures: we tell them the parameter we didn't like. */
-static void negotiation_failed(struct state *state, bool am_opener,
-			       const char *fmt, ...)
+static void NORETURN negotiation_failed(struct state *state,
+					const char *fmt, ...)
 {
 	va_list ap;
 	const char *errmsg;
@@ -152,7 +136,7 @@ static void negotiation_failed(struct state *state, bool am_opener,
 			      "You gave bad parameters: %s", errmsg);
 	peer_write(state->pps, take(msg));
 
-	negotiation_aborted(state, am_opener, errmsg);
+	negotiation_aborted(state, errmsg);
 }
 
 /* We always set channel_reserve_satoshis to 1%, rounded down. */
@@ -177,7 +161,6 @@ static void set_reserve(struct state *state, const struct amount_sat dust_limit)
 /*~ Handle random messages we might get during opening negotiation, (eg. gossip)
  * returning the first non-handled one, or NULL if we aborted negotiation. */
 static u8 *opening_negotiate_msg(const tal_t *ctx, struct state *state,
-				 bool am_opener,
 				 const struct channel_id *alternate)
 {
 	/* This is an event loop of its own.  That's generally considered poor
@@ -219,7 +202,7 @@ static u8 *opening_negotiate_msg(const tal_t *ctx, struct state *state,
 				tal_free(msg);
 				continue;
 			}
-			negotiation_aborted(state, am_opener,
+			negotiation_aborted(state,
 					    tal_fmt(tmpctx, "They sent %s",
 						    err));
 			/* Return NULL so caller knows to stop negotiating. */
@@ -384,7 +367,7 @@ static u8 *funder_channel_start(struct state *state, u8 channel_flags)
 		       "Funding channel start: offered, now waiting for accept_channel");
 
 	/* ... since their reply should be immediate. */
-	msg = opening_negotiate_msg(tmpctx, state, true, NULL);
+	msg = opening_negotiate_msg(tmpctx, state, NULL);
 	if (!msg)
 		return NULL;
 
@@ -426,7 +409,7 @@ static u8 *funder_channel_start(struct state *state, u8 channel_flags)
 	if (accept_tlvs->channel_type
 	    && !featurebits_eq(accept_tlvs->channel_type,
 			       state->channel_type->features)) {
-		negotiation_failed(state, true,
+		negotiation_failed(state,
 				   "Return unoffered channel_type: %s",
 				   fmt_featurebits(tmpctx,
 						   accept_tlvs->channel_type));
@@ -447,7 +430,7 @@ static u8 *funder_channel_start(struct state *state, u8 channel_flags)
 
 	if (amount_sat_greater(state->remoteconf.dust_limit,
 			       state->localconf.channel_reserve)) {
-		negotiation_failed(state, true,
+		negotiation_failed(state,
 				   "dust limit %s"
 				   " would be above our reserve %s",
 				   type_to_string(tmpctx, struct amount_sat,
@@ -468,7 +451,7 @@ static u8 *funder_channel_start(struct state *state, u8 channel_flags)
 						    state->their_features,
 						    OPT_ANCHOR_OUTPUTS),
 				 &err_reason)) {
-		negotiation_failed(state, true, "%s", err_reason);
+		negotiation_failed(state, "%s", err_reason);
 		return NULL;
 	}
 
@@ -579,7 +562,7 @@ static bool funder_finalize_channel_setup(struct state *state,
 	if (!*tx) {
 		/* This should not happen: we should never create channels we
 		 * can't afford the fees for after reserve. */
-		negotiation_failed(state, true,
+		negotiation_failed(state,
 				   "Could not meet their fees and reserve: %s", err_reason);
 		goto fail;
 	}
@@ -644,7 +627,7 @@ static bool funder_finalize_channel_setup(struct state *state,
 	 * transaction.  Note that errors may refer to the temporary channel
 	 * id (state->channel_id), but success should refer to the new
 	 * "cid" */
-	msg = opening_negotiate_msg(tmpctx, state, true, &cid);
+	msg = opening_negotiate_msg(tmpctx, state, &cid);
 	if (!msg)
 		goto fail;
 
@@ -696,7 +679,7 @@ static bool funder_finalize_channel_setup(struct state *state,
 				 &state->first_per_commitment_point[LOCAL],
 				 LOCAL, direct_outputs, &err_reason);
 	if (!*tx) {
-		negotiation_failed(state, true,
+		negotiation_failed(state,
 				   "Could not meet our fees and reserve: %s", err_reason);
 		goto fail;
 	}
@@ -842,7 +825,7 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg)
 					    state->our_features,
 					    state->their_features);
 		if (!state->channel_type) {
-			negotiation_failed(state, false,
+			negotiation_failed(state,
 					   "Did not support channel_type %s",
 					   fmt_featurebits(tmpctx,
 							   open_tlvs->channel_type));
@@ -861,7 +844,7 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg)
 	 *  that is unknown to the receiver.
 	 */
 	if (!bitcoin_blkid_eq(&chain_hash, &chainparams->genesis_blockhash)) {
-		negotiation_failed(state, false,
+		negotiation_failed(state,
 				   "Unknown chain-hash %s",
 				   type_to_string(tmpctx,
 						  struct bitcoin_blkid,
@@ -879,7 +862,7 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg)
 	if (!feature_negotiated(state->our_features, state->their_features,
 				OPT_LARGE_CHANNELS)
 	    && amount_sat_greater(state->funding_sats, chainparams->max_funding)) {
-		negotiation_failed(state, false,
+		negotiation_failed(state,
 				   "funding_satoshis %s too large",
 				   type_to_string(tmpctx, struct amount_sat,
 						  &state->funding_sats));
@@ -911,14 +894,14 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg)
 	 *    unreasonably large.
 	 */
 	if (state->feerate_per_kw < state->min_feerate) {
-		negotiation_failed(state, false,
+		negotiation_failed(state,
 				   "feerate_per_kw %u below minimum %u",
 				   state->feerate_per_kw, state->min_feerate);
 		return NULL;
 	}
 
 	if (state->feerate_per_kw > state->max_feerate) {
-		negotiation_failed(state, false,
+		negotiation_failed(state,
 				   "feerate_per_kw %u above maximum %u",
 				   state->feerate_per_kw, state->max_feerate);
 		return NULL;
@@ -938,7 +921,7 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg)
 	 */
 	if (amount_sat_greater(state->remoteconf.dust_limit,
 			       state->localconf.channel_reserve)) {
-		negotiation_failed(state, false,
+		negotiation_failed(state,
 				   "Our channel reserve %s"
 				   " would be below their dust %s",
 				   type_to_string(tmpctx, struct amount_sat,
@@ -949,7 +932,7 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg)
 	}
 	if (amount_sat_greater(state->localconf.dust_limit,
 			       state->remoteconf.channel_reserve)) {
-		negotiation_failed(state, false,
+		negotiation_failed(state,
 				   "Our dust limit %s"
 				   " would be above their reserve %s",
 				   type_to_string(tmpctx, struct amount_sat,
@@ -971,7 +954,7 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg)
 						    state->their_features,
 						    OPT_ANCHOR_OUTPUTS),
 				 &err_reason)) {
-		negotiation_failed(state, false, "%s", err_reason);
+		negotiation_failed(state, "%s", err_reason);
 		return NULL;
 	}
 
@@ -1001,7 +984,7 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg)
 
 	/* If they give us a reason to reject, do so. */
 	if (err_reason) {
-		negotiation_failed(state, false, "%s", err_reason);
+		negotiation_failed(state, "%s", err_reason);
 		tal_free(err_reason);
 		return NULL;
 	}
@@ -1044,7 +1027,7 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg)
 		       "Incoming channel: accepted, now waiting for them to create funding tx");
 
 	/* This is a loop which handles gossip until we get a non-gossip msg */
-	msg = opening_negotiate_msg(tmpctx, state, false, NULL);
+	msg = opening_negotiate_msg(tmpctx, state, NULL);
 	if (!msg)
 		return NULL;
 
@@ -1130,7 +1113,7 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg)
 					  LOCAL, NULL, &err_reason);
 	/* This shouldn't happen either, AFAICT. */
 	if (!local_commit) {
-		negotiation_failed(state, false,
+		negotiation_failed(state,
 				   "Could not meet our fees and reserve: %s", err_reason);
 		return NULL;
 	}
@@ -1189,7 +1172,7 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg)
 					   &state->first_per_commitment_point[REMOTE],
 					   REMOTE, direct_outputs, &err_reason);
 	if (!remote_commit) {
-		negotiation_failed(state, false,
+		negotiation_failed(state,
 				   "Could not meet their fees and reserve: %s", err_reason);
 		return NULL;
 	}
@@ -1353,7 +1336,7 @@ static u8 *handle_master_in(struct state *state)
 
 		msg = towire_errorfmt(NULL, &state->channel_id, "Channel open canceled by us");
 		peer_write(state->pps, take(msg));
-		negotiation_aborted(state, true, "Channel open canceled by RPC");
+		negotiation_aborted(state, "Channel open canceled by RPC");
 		return NULL;
 	case WIRE_OPENINGD_DEV_MEMLEAK:
 #if DEVELOPER
@@ -1365,7 +1348,7 @@ static u8 *handle_master_in(struct state *state)
 	case WIRE_OPENINGD_FUNDER_REPLY:
 	case WIRE_OPENINGD_FUNDER_START_REPLY:
 	case WIRE_OPENINGD_FUNDEE:
-	case WIRE_OPENINGD_FUNDER_FAILED:
+	case WIRE_OPENINGD_FAILED:
 	case WIRE_OPENINGD_GOT_OFFER:
 	case WIRE_OPENINGD_GOT_OFFER_REPLY:
 	case WIRE_OPENINGD_GOT_REESTABLISH:

openingd/openingd_wire.csv

@@ -104,8 +104,8 @@ msgdata,openingd_funder_complete,channel_type,channel_type,
 msgtype,openingd_funder_cancel,6013
 
 # Openingd->master: we failed to negotiation channel
-msgtype,openingd_funder_failed,6004
-msgdata,openingd_funder_failed,reason,wirestring,
+msgtype,openingd_failed,6004
+msgdata,openingd_failed,reason,wirestring,
 
 # Openingd->master: they offered channel.
 # This gives their txid and info, means we can send funding_signed: we're done.

tests/test_connection.py

@@ -998,13 +998,9 @@ def test_funding_fail(node_factory, bitcoind):
     with pytest.raises(RpcError, match=r'to_self_delay \d+ larger than \d+'):
         l1.rpc.fundchannel(l2.info['id'], int(funds / 10))
 
-    # dual-funded channels disconnect on failure
-    if not l1.config('experimental-dual-fund'):
-        assert only_one(l1.rpc.listpeers()['peers'])['connected']
-        assert only_one(l2.rpc.listpeers()['peers'])['connected']
-    else:
-        assert len(l1.rpc.listpeers()['peers']) == 0
-        assert len(l2.rpc.listpeers()['peers']) == 0
+    # channels disconnect on failure
+    assert len(l1.rpc.listpeers()['peers']) == 0
+    assert len(l2.rpc.listpeers()['peers']) == 0
 
     # Restart l2 without ridiculous locktime.
     del l2.daemon.opts['watchtime-blocks']
@@ -1217,7 +1213,9 @@ def test_funding_external_wallet_corners(node_factory, bitcoind):
     l1.rpc.txdiscard(wrongaddr['txid'])
 
     l1.rpc.fundchannel_cancel(l2.info['id'])
-    # Should be able to 'restart' after canceling
+
+    # Cancelling causes disconnection.
+    l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
     amount2 = 1000000
     funding_addr = l1.rpc.fundchannel_start(l2.info['id'], amount2)['funding_address']