close: change to a unilateraltimeout argument.

`close` takes two optional arguments: `force` and `timeout`.
`timeout` doesn't timeout the close (there's no way to do that), just
the JSON call.  `force` (default `false`) if set, means we unilaterally
close at the timeout, instead of just failing.

Timing out JSON calls is generally deprecated: that's the job of the
client.  And the semantics of this are confusing, even to me!  A
better API is a timeout which, if non-zero, is the time at which we
give up and unilaterally close.

The transition code is awkward, but we'll manage for the three
releases until we can remove it.

The new defaults are to unilaterally close after 48 hours.

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

CHANGELOG.md

@@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- JSON API: `close` optional arguments have changed: it now defaults to unilateral close after 48 hours.
 - build: now requires `python3-mako` to be installed, i.e. `sudo apt-get install python3-mako`
 - plugins: if the config directory has a `plugins` subdirectory, those are loaded.
 - plugins: a new notification type `invoice_payment` (sent when an invoice is paid) has been added

contrib/pylightning/lightning/lightning.py

@@ -3,6 +3,7 @@ import json
 import logging
 from math import floor, log10
 import socket
+import warnings
 
 __version__ = "0.0.7.3"
 
@@ -309,16 +310,43 @@ class LightningRpc(UnixDomainSocketRpc):
         payload.update({k: v for k, v in kwargs.items()})
         return self.call("check", payload)
 
-    def close(self, peer_id, force=None, timeout=None):
+    def _deprecated_close(self, peer_id, force=None, timeout=None):
+        warnings.warn("close now takes unilateraltimeout arg: expect removal"
+                      " in early 2020",
+                      DeprecationWarning)
+        payload = {
+            "id": peer_id,
+            "force": force,
+            "timeout": timeout
+        }
+        return self.call("close", payload)
+
+    def close(self, peer_id, *args, **kwargs):
         """
         Close the channel with peer {id}, forcing a unilateral
-        close if {force} is True, and timing out with {timeout}
-        seconds.
+        close after {unilateraltimeout} seconds if non-zero.
+
+        Deprecated usage has {force} and {timeout} args.
         """
+        unilateraltimeout = None
+
+        if 'force' in kwargs or 'timeout' in kwargs:
+            return self._deprecated_close(peer_id, *args, **kwargs)
+
+        # Single arg is ambigious.
+        if len(args) == 1:
+            if isinstance(args[0], bool):
+                return self._deprecated_close(peer_id, *args, **kwargs)
+            unilateraltimeout = args[0]
+        elif len(args) > 1:
+            return self._deprecated_close(peer_id, *args, **kwargs)
+
+        if 'unilateraltimeout' in kwargs:
+            unilateraltimeout = kwargs['unilateraltimeout']
+
         payload = {
             "id": peer_id,
-            "force": force,
-            "timeout": timeout
+            "unilateraltimeout": unilateraltimeout
         }
         return self.call("close", payload)
 

doc/lightning-close.7

@@ -2,12 +2,12 @@
 .\"     Title: lightning-close
 .\"    Author: [see the "AUTHOR" section]
 .\" Generator: DocBook XSL Stylesheets v1.79.1 <http://docbook.sf.net/>
-.\"      Date: 04/30/2018
+.\"      Date: 08/08/2019
 .\"    Manual: \ \&
 .\"    Source: \ \&
 .\"  Language: English
 .\"
-.TH "LIGHTNING\-CLOSE" "7" "04/30/2018" "\ \&" "\ \&"
+.TH "LIGHTNING\-CLOSE" "7" "08/08/2019" "\ \&" "\ \&"
 .\" -----------------------------------------------------------------
 .\" * Define some portability stuff
 .\" -----------------------------------------------------------------
@@ -31,29 +31,32 @@
 lightning-close \- Command for closing channels with direct peers
 .SH "SYNOPSIS"
 .sp
-\fBclose\fR \fIid\fR [\fIforce\fR] [\fItimeout\fR]
+\fBclose\fR \fIid\fR [\fIunilateraltimeout\fR]
 .SH "DESCRIPTION"
 .sp
-The \fBclose\fR RPC command attempts to close the channel cooperatively with the peer\&. If the given \fIid\fR is a peer ID (66 hex digits as a string), then it applies to the active channel of the direct peer corresponding to the given peer ID\&. If the given \fIid\fR is a channel ID (64 hex digits as a string, or the short channel ID \fIblockheight:txindex:outindex\fR form), then it applies to that channel\&.
+The \fBclose\fR RPC command attempts to close the channel cooperatively with the peer, or unilaterally after \fIunilateraltimeout\fR\&.
 .sp
-The \fBclose\fR command will time out and return with an error when the number of seconds specified in \fItimeout\fR is reached\&. If unspecified, it times out in 30 seconds\&.
+If the given \fIid\fR is a peer ID (66 hex digits as a string), then it applies to the active channel of the direct peer corresponding to the given peer ID\&. If the given \fIid\fR is a channel ID (64 hex digits as a string, or the short channel ID \fIblockheight:txindex:outindex\fR form), then it applies to that channel\&.
 .sp
-The \fIforce\fR argument, if the JSON value \fItrue\fR, will cause the channel to be unilaterally closed when the timeout is reached\&. If so, timeout will not cause an error, but instead cause the channel to be failed and put onchain unilaterally\&. Unilateral closes will lead to your funds getting locked according to the \fIto_self_delay\fR parameter of the peer\&.
+If \fIunilateraltimeout\fR is not zero, the \fBclose\fR command will unilaterally close the channel when that number of seconds is reached\&. If \fIunilateraltimeout\fR is zero, then the \fBclose\fR command will wait indefinitely until the peer is online and can negotiate a mutual close\&. The default is 2 days (172800 seconds)\&.
 .sp
-Normally the peer needs to be live and connected in order to negotiate a mutual close\&. Forcing a unilateral close can be used if you suspect you can no longer contact the peer\&.
+The peer needs to be live and connected in order to negotiate a mutual close\&. The default of unilaterally closing after 48 hours is usually a reasonable indication that you can no longer contact the peer\&.
+.SH "NOTES"
+.sp
+Prior to 0\&.7\&.2, \fBclose\fR took two parameters: \fIforce\fR and \fItimeout\fR\&. \fItimeout\fR was the number of seconds before \fIforce\fR took effect (default, 30), and \fIforce\fR determined whether the result was a unilateral close or an RPC error (default)\&. Even after the timeout, the channel would be closed if the peer reconnected\&.
 .SH "RETURN VALUE"
 .sp
 On success, an object with fields \fItx\fR and \fItxid\fR containing the closing transaction are returned\&. It will also have a field \fItype\fR which is either the JSON string \fImutual\fR or the JSON string \fIunilateral\fR\&. A \fImutual\fR close means that we could negotiate a close with the peer, while a \fIunilateral\fR close means that the \fIforce\fR flag was set and we had to close the channel without waiting for the counterparty\&.
 .sp
-A unilateral close may still occur with \fIforce\fR set to \fIfalse\fR if the peer did not behave correctly during the close negotiation\&.
+A unilateral close may still occur at any time if the peer did not behave correctly during the close negotiation\&.
 .sp
 Unilateral closes will return your funds after a delay\&. The delay will vary based on the peer \fIto_self_delay\fR setting, not your own setting\&.
-.sp
-On failure, if \fBclose\fR failed due to timing out with \fIforce\fR argument \fIfalse\fR, the channel will still eventually close once we have contacted the peer\&.
 .SH "AUTHOR"
 .sp
 ZmnSCPxj <ZmnSCPxj@protonmail\&.com> is mainly responsible\&.
 .SH "SEE ALSO"
+.sp
+lightning\-disconnect(7), lightning\-fundchannel(7)
 .SH "RESOURCES"
 .sp
 Main web site: https://github\&.com/ElementsProject/lightning

doc/lightning-close.7.txt

@@ -8,13 +8,14 @@ lightning-close - Command for closing channels with direct peers
 
 SYNOPSIS
 --------
-*close* 'id' ['force'] ['timeout']
+*close* 'id' ['unilateraltimeout']
 
 DESCRIPTION
 -----------
 
 The *close* RPC command attempts to close the channel cooperatively
-with the peer.
+with the peer, or unilaterally after 'unilateraltimeout'.
+
 If the given 'id' is a peer ID (66 hex digits as a string), then
 it applies to the active channel of the direct peer corresponding to
 the given peer ID.
@@ -22,22 +23,26 @@ If the given 'id' is a channel ID (64 hex digits as a string, or
 the short channel ID 'blockheight:txindex:outindex' form), then it
 applies to that channel.
 
-The *close* command will time out and return with an error when the
-number of seconds specified in 'timeout' is reached.
-If unspecified, it times out in 30 seconds.
-
-The 'force' argument, if the JSON value 'true', will cause the
-channel to be unilaterally closed when the timeout is reached.
-If so, timeout will not cause an error, but instead cause the
-channel to be failed and put onchain unilaterally.
-Unilateral closes will lead to your funds getting locked according
-to the 'to_self_delay' parameter of the peer.
+If 'unilateraltimeout' is not zero, the *close* command will
+unilaterally close the channel when that number of seconds is reached.
+If 'unilateraltimeout' is zero, then the *close* command will wait
+indefinitely until the peer is online and can negotiate a mutual
+close.  The default is 2 days (172800 seconds).
 
-Normally the peer needs to be live and connected in order to negotiate
-a mutual close.
-Forcing a unilateral close can be used if you suspect you can no longer
+The peer needs to be live and connected in order to negotiate
+a mutual close.  The default of unilaterally closing after 48 hours
+is usually a reasonable indication that you can no longer
 contact the peer.
 
+NOTES
+-----
+
+Prior to 0.7.2, *close* took two parameters: 'force' and 'timeout'.
+'timeout' was the number of seconds before 'force' took effect
+(default, 30), and 'force' determined whether the result was a
+unilateral close or an RPC error (default).  Even after
+the timeout, the channel would be closed if the peer reconnected.
+
 RETURN VALUE
 ------------
 
@@ -50,24 +55,20 @@ peer, while a 'unilateral' close means that the 'force' flag was
 set and we had to close the channel without waiting for the
 counterparty.
 
-A unilateral close may still occur with 'force' set to 'false' if
+A unilateral close may still occur at any time if
 the peer did not behave correctly during the close negotiation.
 
 Unilateral closes will return your funds after a delay.
 The delay will vary based on the peer 'to_self_delay' setting, not
 your own setting.
 
-On failure, if *close* failed due to timing out with 'force'
-argument 'false', the channel will still eventually close once
-we have contacted the peer.
-
 AUTHOR
 ------
 ZmnSCPxj <ZmnSCPxj@protonmail.com> is mainly responsible.
 
 SEE ALSO
 --------
-
+lightning-disconnect(7), lightning-fundchannel(7)
 
 RESOURCES
 ---------

lightningd/peer_control.c

@@ -320,7 +320,7 @@ static void
 register_close_command(struct lightningd *ld,
 		       struct command *cmd,
 		       struct channel *channel,
-		       unsigned int timeout,
+		       unsigned int *timeout,
 		       bool force)
 {
 	struct close_command *cc;
@@ -335,8 +335,11 @@ register_close_command(struct lightningd *ld,
 	tal_add_destructor2(channel,
 			    &destroy_close_command_on_channel_destroy,
 			    cc);
-	new_reltimer(ld->timers, cc, time_from_sec(timeout),
-		     &close_command_timeout, cc);
+	log_debug(ld->log, "close_command: force = %u, timeout = %i",
+		  force, timeout ? *timeout : -1);
+	if (timeout)
+		new_reltimer(ld->timers, cc, time_from_sec(*timeout),
+			     &close_command_timeout, cc);
 }
 
 static bool invalid_last_tx(const struct bitcoin_tx *tx)
@@ -1225,14 +1228,83 @@ static struct command_result *json_close(struct command *cmd,
 	struct peer *peer;
 	struct channel *channel COMPILER_WANTS_INIT("gcc 7.3.0 fails, 8.3 OK");
 	unsigned int *timeout;
-	bool *force;
+	bool force = true;
+	bool do_timeout;
+
+	/* For generating help, give new-style. */
+	if (!params || !deprecated_apis) {
+		if (!param(cmd, buffer, params,
+			   p_req("id", param_tok, &idtok),
+			   p_opt_def("unilateraltimeout", param_number,
+				     &timeout, 48 * 3600),
+			   NULL))
+			return command_param_failed();
+		do_timeout = (*timeout != 0);
+	} else if (params->type == JSMN_ARRAY) {
+		const jsmntok_t *tok;
+
+		/* Could be new or old style; get as tok. */
+		if (!param(cmd, buffer, params,
+			   p_req("id", param_tok, &idtok),
+			   p_opt("unilateraltimeout_or_force", param_tok, &tok),
+			   p_opt("timeout", param_number, &timeout),
+			   NULL))
+			return command_param_failed();
+
+		if (tok) {
+			/* old-style force bool? */
+			if (json_to_bool(buffer, tok, &force)) {
+				/* Old default timeout */
+				if (!timeout) {
+					timeout = tal(cmd, unsigned int);
+					*timeout = 30;
+				}
+			/* New-style timeout */
+			} else {
+				timeout = tal(cmd, unsigned int);
+				if (!json_to_number(buffer, tok, timeout)) {
+					return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
+							    "Expected unilerataltimeout to be a number");
+				}
+			}
+		}
 
-	if (!param(cmd, buffer, params,
-		   p_req("id", param_tok, &idtok),
-		   p_opt_def("force", param_bool, &force, false),
-		   p_opt_def("timeout", param_number, &timeout, 30),
-		   NULL))
-		return command_param_failed();
+		/* If they didn't specify timeout, it's the (new) default */
+		if (!timeout) {
+			timeout = tal(cmd, unsigned int);
+			*timeout = 48 * 3600;
+		}
+		do_timeout = true;
+	} else {
+		unsigned int *old_timeout;
+		bool *old_force;
+
+		/* Named parameters are easy to distinguish */
+		if (!param(cmd, buffer, params,
+			   p_req("id", param_tok, &idtok),
+			   p_opt_def("unilateraltimeout", param_number,
+				     &timeout, 48 * 3600),
+			   p_opt("force", param_bool, &old_force),
+			   p_opt("timeout", param_number, &old_timeout),
+			   NULL))
+			return command_param_failed();
+		/* Old style. */
+		if (old_timeout) {
+			*timeout = *old_timeout;
+		}
+		if (old_force) {
+			/* Use old default */
+			if (!old_timeout)
+				*timeout = 30;
+			force = *old_force;
+		}
+
+		/* New style: do_timeout unless it's 0 */
+		if (!old_timeout && !old_force)
+			do_timeout = (*timeout != 0);
+		else
+			do_timeout = true;
+	}
 
 	peer = peer_from_json(cmd->ld, buffer, idtok);
 	if (peer)
@@ -1283,7 +1355,8 @@ static struct command_result *json_close(struct command *cmd,
 	}
 
 	/* Register this command for later handling. */
-	register_close_command(cmd->ld, cmd, channel, *timeout, *force);
+	register_close_command(cmd->ld, cmd, channel,
+			       do_timeout ? timeout : NULL, force);
 
 	/* Wait until close drops down to chain. */
 	return command_still_pending(cmd);

wallet/test/run-wallet.c

@@ -325,6 +325,9 @@ char *json_strdup(const tal_t *ctx UNNEEDED, const char *buffer UNNEEDED, const
 /* Generated stub for json_stream_success */
 struct json_stream *json_stream_success(struct command *cmd UNNEEDED)
 { fprintf(stderr, "json_stream_success called!\n"); abort(); }
+/* Generated stub for json_to_bool */
+bool json_to_bool(const char *buffer UNNEEDED, const jsmntok_t *tok UNNEEDED, bool *b UNNEEDED)
+{ fprintf(stderr, "json_to_bool called!\n"); abort(); }
 /* Generated stub for json_tok_bin_from_hex */
 u8 *json_tok_bin_from_hex(const tal_t *ctx UNNEEDED, const char *buffer UNNEEDED, const jsmntok_t *tok UNNEEDED)
 { fprintf(stderr, "json_tok_bin_from_hex called!\n"); abort(); }