lightningd/opening_control.c: Remove 'Try fundchannel_cancel again' error.

Changelog-Changed: `fundchannel_cancel` will now succeed even when executed while a `fundchannel_complete` is ongoing; in that case, it will be considered as cancelling the funding *after* the `fundchannel_complete` succeeds.

Let me introduce the concept of "Sequential Consistency":
All operations on parallel processes form a single total order agreed upon by all processes.

So for example, suppose we have parallel invocations of `fundchannel_complete` and `fundchannel_cancel`:

                          +--[fundchannel_complete]-->
                          |
    --[fundchannel_start]-+
                          |
                          +--[fundchannel_cancel]---->

What "Sequential Consistency" means is that the above parallel operations can be serialized as a single total order as:

    --[fundchannel_start]--[fundchannel_complete]--[fundchannel_cancel]-->

Or:

    --[fundchannel_start]--[fundchannel_cancel]--[fundchannel_complete]-->

In the first case, `fundchannel_complete` succeeds, and the `fundchannel_cancel` invocation also succeeds, sending an `error` to the peer to make them forget the chanel.

In the second case, `fundchannel_cancel` succeeds, and the succeeding `fundchannel_complete` invocation fails, since the funding is already cancelled and there is nothing to complete.

Note that in both cases, `fundchannel_cancel` **always** succeeds.

Unfortunately, prior to this commit, `fundchannel_cancel` could fail with a `Try fundchannel_cancel again` error if the `fundchannel_complete` is ongoing when the `fundchannel_cancel` is initiated.
This violates Sequential Consistency, as there is no single total order that would have caused `fundchannel_cancel` to fail.

This commit is a minimal patch which just reschedules `fundchannel_cancel` to occur after any `fundchannel_complete` that is ongoing.

lightningd/opening_control.c

@@ -34,6 +34,7 @@
 #include <lightningd/plugin_hook.h>
 #include <lightningd/subd.h>
 #include <openingd/gen_opening_wire.h>
+#include <string.h>
 #include <wire/gen_common_wire.h>
 #include <wire/wire.h>
 #include <wire/wire_sync.h>
@@ -282,17 +283,66 @@ wallet_commit_channel(struct lightningd *ld,
 	return channel;
 }
 
+/** cancel_after_fundchannel_complete_success
+ *
+ * @brief Called to cancel a `fundchannel` after
+ * a `fundchannel_complete` succeeds.
+ *
+ * @desc Specifically, this is called when a
+ * `fundchannel_cancel` is blocked due to a
+ * parallel `fundchannel_complete` still running.
+ * After the `fundchannel_complete` succeeds, we
+ * invoke this function to cancel the funding
+ * after all.
+ *
+ * In effect, this forces the `fundchannel_cancel`
+ * to be invoked after the `fundchannel_complete`
+ * succeeds, leading to a reasonable serial
+ * execution.
+ *
+ * @param cmd - The `fundchannel_cancel` command
+ * that wants to cancel this.
+ * @param channel - The channel being cancelled.
+ */
+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));
+}
+
 static void funding_success(struct channel *channel)
 {
 	struct json_stream *response;
 	struct funding_channel *fc = channel->peer->uncommitted_channel->fc;
 	struct command *cmd = fc->cmd;
 
-	/* Well, those cancels didn't work! */
+	/* Well, those cancels now need to trigger!  */
 	for (size_t i = 0; i < tal_count(fc->cancels); i++)
-		was_pending(command_fail(fc->cancels[i], LIGHTNINGD,
+		cancel_after_fundchannel_complete_success(fc->cancels[i],
-					 "Funding succeeded before cancel. "
+							  channel);
-					 "Try fundchannel_cancel again."));
 
 	response = json_stream_success(cmd);
 	json_add_string(response, "channel_id",

tests/test_connection.py

@@ -1025,7 +1025,7 @@ def test_funding_cancel_race(node_factory, bitcoind, executor):
             else:
                 cancels.append(executor.submit(l1.rpc.fundchannel_cancel, n.info['id']))
 
-        # Only one should succeed.
+        # Only up to one should succeed.
         success = False
         for c in completes:
             try:
@@ -1036,23 +1036,25 @@ def test_funding_cancel_race(node_factory, bitcoind, executor):
             except RpcError:
                 pass
 
-        # These may both succeed, iff the above didn't.
+        # At least one of these must succeed, regardless of whether
+        # the completes succeeded or not.
         cancelled = False
         for c in cancels:
             try:
                 c.result(TIMEOUT)
                 cancelled = True
-                assert not success
             except RpcError:
                 pass
-
+        # cancel always succeeds, as per Sequential Consistency.
-        if cancelled:
+        # Either the cancel occurred before complete, in which
+        # case it prevents complete from succeeding, or it
+        # occurred after complete, in which case it errors the
+        # channel to force the remote to forget it.
+        assert cancelled
         num_cancel += 1
-        else:
-            assert success
 
     print("Cancelled {} complete {}".format(num_cancel, num_complete))
-    assert num_cancel + num_complete == len(nodes)
+    assert num_cancel == len(nodes)
 
     # We should have raced at least once!
     if not VALGRIND: