lightningd/opening_control.c: `fundchannel_cancel` no longer requires a `channel_id` argument.

Fixes: #3785

Changelog-Changed: `fundchannel_cancel` no longer requires its undocumented `channel_id` argument after `fundchannel_complete`.

common/jsonrpc_errors.h

@@ -49,6 +49,8 @@ static const errcode_t FUNDING_BROADCAST_FAIL = 303;
 static const errcode_t FUNDING_STILL_SYNCING_BITCOIN = 304;
 static const errcode_t FUNDING_PEER_NOT_CONNECTED = 305;
 static const errcode_t FUNDING_UNKNOWN_PEER = 306;
+static const errcode_t FUNDING_NOTHING_TO_CANCEL = 307;
+static const errcode_t FUNDING_CANCEL_NOT_SAFE = 308;
 
 /* `connect` errors */
 static const errcode_t CONNECT_NO_KNOWN_ADDRESS = 400;

doc/lightning-fundchannel_cancel.7

@@ -36,9 +36,13 @@ with \fBcode\fR being one of the following:
 .IP \[bu]
 -32602: If the given parameters are wrong\.
 .IP \[bu]
--1: Catchall nonspecific error\.
-.IP \[bu]
 306: Unknown peer id\.
+.IP \[bu]
+307: No channel currently being funded that can be cancelled\.
+.IP \[bu]
+308: It is unsafe to cancel the channel: the funding transaction
+has been broadcast, or there are HTLCs already in the channel, or
+the peer was the initiator and not us\.
 
 .RE
 .SH AUTHOR

doc/lightning-fundchannel_cancel.7.md

@@ -32,8 +32,11 @@ On error the returned object will contain `code` and `message` properties,
 with `code` being one of the following:
 
 - -32602: If the given parameters are wrong.
-- -1: Catchall nonspecific error.
 - 306: Unknown peer id.
+- 307: No channel currently being funded that can be cancelled.
+- 308: It is unsafe to cancel the channel: the funding transaction
+  has been broadcast, or there are HTLCs already in the channel, or
+  the peer was the initiator and not us.
 
 AUTHOR
 ------

lightningd/channel_control.c

@@ -752,7 +752,8 @@ static void process_check_funding_broadcast(struct bitcoind *bitcoind,
 
 	if (txout != NULL) {
 		for (size_t i = 0; i < tal_count(cancel->forgets); i++)
-			was_pending(command_fail(cancel->forgets[i], LIGHTNINGD,
+			was_pending(command_fail(cancel->forgets[i],
+				    FUNDING_CANCEL_NOT_SAFE,
 				    "The funding transaction has been broadcast, "
 				    "please consider `close` or `dev-fail`! "));
 		tal_free(cancel->forgets);
@@ -767,47 +768,41 @@ static void process_check_funding_broadcast(struct bitcoind *bitcoind,
 }
 
 struct command_result *cancel_channel_before_broadcast(struct command *cmd,
-						       const char *buffer,
-						       struct peer *peer,
-						       const jsmntok_t *cidtok)
+						       struct peer *peer)
 {
 	struct channel *cancel_channel;
 	struct channel_to_cancel *cc = tal(cmd, struct channel_to_cancel);
+	struct channel *channel;
 
 	cc->peer = peer->id;
-	if (!cidtok) {
-		struct channel *channel;
-
-		cancel_channel = NULL;
-		list_for_each(&peer->channels, channel, list) {
-			if (cancel_channel) {
-				return command_fail(cmd, LIGHTNINGD,
-						    "Multiple channels:"
-						    " please specify channel_id");
-			}
-			cancel_channel = channel;
-		}
-		if (!cancel_channel)
-			return command_fail(cmd, LIGHTNINGD,
-					    "No channels matching that peer_id");
-		derive_channel_id(&cc->cid,
-				  &cancel_channel->funding_txid,
-				  cancel_channel->funding_outnum);
-	} else {
-		if (!json_tok_channel_id(buffer, cidtok, &cc->cid))
-			return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
-					    "Invalid channel_id parameter.");
-
-		cancel_channel = find_channel_by_id(peer, &cc->cid);
-		if (!cancel_channel)
-			return command_fail(cmd, LIGHTNINGD,
-					    "Channel ID not found: '%.*s'",
-					    cidtok->end - cidtok->start,
-					    buffer + cidtok->start);
+	cancel_channel = NULL;
+	list_for_each(&peer->channels, channel, list) {
+		/* After `fundchannel_complete`, channel is in
+		 * `CHANNELD_AWAITING_LOCKIN` state.
+		 *
+		 * TODO: This assumes only one channel at a time
+		 * can be in this state, which is true at the
+		 * time of this writing, but may change *if* we
+		 * ever implement multiple channels per peer.
+		 */
+		if (channel->state != CHANNELD_AWAITING_LOCKIN)
+			continue;
+		cancel_channel = channel;
+		break;
 	}
+	if (!cancel_channel)
+		return command_fail(cmd, FUNDING_NOTHING_TO_CANCEL,
+				    "No channels being opened or "
+				    "awaiting lock-in for "
+				    "peer_id %s",
+				    type_to_string(tmpctx, struct node_id,
+						   &peer->id));
+	derive_channel_id(&cc->cid,
+			  &cancel_channel->funding_txid,
+			  cancel_channel->funding_outnum);
 
 	if (cancel_channel->opener == REMOTE)
-		return command_fail(cmd, LIGHTNINGD,
+		return command_fail(cmd, FUNDING_CANCEL_NOT_SAFE,
 				    "Cannot cancel channel that was "
 				    "initiated by peer");
 
@@ -817,13 +812,13 @@ struct command_result *cancel_channel_before_broadcast(struct command *cmd,
 	if (wallet_transaction_type(cmd->ld->wallet,
 				   &cancel_channel->funding_txid,
 				   &type))
-		return command_fail(cmd, LIGHTNINGD,
+		return command_fail(cmd, FUNDING_CANCEL_NOT_SAFE,
 				    "Has the funding transaction been broadcast? "
 				    "Please use `close` or `dev-fail` instead.");
 
 	if (channel_has_htlc_out(cancel_channel) ||
 	    channel_has_htlc_in(cancel_channel)) {
-		return command_fail(cmd, LIGHTNINGD,
+		return command_fail(cmd, FUNDING_CANCEL_NOT_SAFE,
 				    "This channel has HTLCs attached and it is "
 				    "not safe to cancel. Has the funding transaction "
 				    "been broadcast? Please use `close` or `dev-fail` "

lightningd/channel_control.h

@@ -26,9 +26,7 @@ void channel_notify_new_block(struct lightningd *ld,
 /* Cancel the channel after `fundchannel_complete` succeeds
  * but before funding broadcasts. */
 struct command_result *cancel_channel_before_broadcast(struct command *cmd,
-						       const char *buffer,
-						       struct peer *peer,
-						       const jsmntok_t *cidtok);
+						       struct peer *peer);
 
 /* Forget a channel. Deletes the channel and handles all
  * associated waiting commands, if present. Notifies peer if available */

lightningd/opening_control.c

@@ -308,29 +308,7 @@ static void
 cancel_after_fundchannel_complete_success(struct command *cmd,
 					  struct channel *channel)
 {
-	struct peer *peer;
-	struct channel_id cid;
-	/* Fake these to adapt to the existing
-	 * cancel_channel_before_broadcast
-	 * interface.
-	 */
-	const char *buffer;
-	jsmntok_t cidtok;
-
-	peer = channel->peer;
-	derive_channel_id(&cid,
-			  &channel->funding_txid, channel->funding_outnum);
-
-	buffer = type_to_string(tmpctx, struct channel_id, &cid);
-	cidtok.type = JSMN_STRING;
-	cidtok.start = 0;
-	cidtok.end = strlen(buffer);
-	cidtok.size = 0;
-
-	was_pending(cancel_channel_before_broadcast(cmd,
-						    buffer,
-						    peer,
-						    &cidtok));
+	was_pending(cancel_channel_before_broadcast(cmd, channel->peer));
 }
 
 static void funding_success(struct channel *channel)
@@ -1150,12 +1128,10 @@ static struct command_result *json_fund_channel_cancel(struct command *cmd,
 
 	struct node_id *id;
 	struct peer *peer;
-	const jsmntok_t *cidtok;
 	u8 *msg;
 
 	if (!param(cmd, buffer, params,
 		   p_req("id", param_node_id, &id),
-		   p_opt("channel_id", param_tok, &cidtok),
 		   NULL))
 		return command_param_failed();
 
@@ -1166,7 +1142,8 @@ static struct command_result *json_fund_channel_cancel(struct command *cmd,
 
 	if (peer->uncommitted_channel) {
 		if (!peer->uncommitted_channel->fc || !peer->uncommitted_channel->fc->inflight)
-			return command_fail(cmd, LIGHTNINGD, "No channel funding in progress.");
+			return command_fail(cmd, FUNDING_NOTHING_TO_CANCEL,
+					    "No channel funding in progress.");
 
 		/* Make sure this gets notified if we succeed or cancel */
 		tal_arr_expand(&peer->uncommitted_channel->fc->cancels, cmd);
@@ -1175,7 +1152,8 @@ static struct command_result *json_fund_channel_cancel(struct command *cmd,
 		return command_still_pending(cmd);
 	}
 
-	return cancel_channel_before_broadcast(cmd, buffer, peer, cidtok);
+	/* Handle `fundchannel_cancel` after `fundchannel_complete`.  */
+	return cancel_channel_before_broadcast(cmd, peer);
 }
 
 /**

tests/test_connection.py

@@ -1162,8 +1162,8 @@ def test_funding_close_upfront(node_factory, bitcoind):
 
 @unittest.skipIf(TEST_NETWORK != 'regtest', "External wallet support doesn't work with elements yet.")
 def test_funding_external_wallet(node_factory, bitcoind):
-    l1 = node_factory.get_node()
-    l2 = node_factory.get_node()
+    l1 = node_factory.get_node(options={'funding-confirms': 2})
+    l2 = node_factory.get_node(options={'funding-confirms': 2})
     l3 = node_factory.get_node()
 
     l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
@@ -1197,20 +1197,23 @@ def test_funding_external_wallet(node_factory, bitcoind):
 
     assert l1.rpc.fundchannel_complete(l2.info['id'], txid, txout)['commitments_secured']
 
-    # Broadcast the transaction manually and confirm that channel locks in
+    # Broadcast the transaction manually
     signed_tx = bitcoind.rpc.signrawtransactionwithwallet(raw_funded_tx)['hex']
     assert txid == bitcoind.rpc.decoderawtransaction(signed_tx)['txid']
 
     bitcoind.rpc.sendrawtransaction(signed_tx)
     bitcoind.generate_block(1)
 
-    l1.daemon.wait_for_log(r'Funding tx {} depth 1 of 1'.format(txid))
+    l1.daemon.wait_for_log(r'Funding tx {} depth 1 of 2'.format(txid))
 
     # Check that tx is broadcast by a third party can be catched.
     # Only when the transaction (broadcast by a third pary) is onchain, we can catch it.
     with pytest.raises(RpcError, match=r'.* been broadcast.*'):
         l1.rpc.fundchannel_cancel(l2.info['id'])
 
+    # Confirm that channel locks in
+    bitcoind.generate_block(1)
+
     for node in [l1, l2]:
         node.daemon.wait_for_log(r'State changed from CHANNELD_AWAITING_LOCKIN to CHANNELD_NORMAL')
         channel = node.rpc.listpeers()['peers'][0]['channels'][0]