From 83e654a10674ee9d73bfe4e5f272352ec8760f6c Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 8 Aug 2019 16:16:05 +0930 Subject: [PATCH] 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 --- CHANGELOG.md | 1 + contrib/pylightning/lightning/lightning.py | 38 +++++++-- doc/lightning-close.7 | 23 +++--- doc/lightning-close.7.txt | 43 +++++----- lightningd/peer_control.c | 95 +++++++++++++++++++--- wallet/test/run-wallet.c | 3 + 6 files changed, 156 insertions(+), 47 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7e6b3e667..100964121 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/contrib/pylightning/lightning/lightning.py b/contrib/pylightning/lightning/lightning.py index 941802f06..8eb99b22e 100644 --- a/contrib/pylightning/lightning/lightning.py +++ b/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) diff --git a/doc/lightning-close.7 b/doc/lightning-close.7 index ce0d489b2..6e1e5663e 100644 --- a/doc/lightning-close.7 +++ b/doc/lightning-close.7 @@ -2,12 +2,12 @@ .\" Title: lightning-close .\" Author: [see the "AUTHOR" section] .\" Generator: DocBook XSL Stylesheets v1.79.1 -.\" 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 is mainly responsible\&. .SH "SEE ALSO" +.sp +lightning\-disconnect(7), lightning\-fundchannel(7) .SH "RESOURCES" .sp Main web site: https://github\&.com/ElementsProject/lightning diff --git a/doc/lightning-close.7.txt b/doc/lightning-close.7.txt index cb84447c8..d3d678ad9 100644 --- a/doc/lightning-close.7.txt +++ b/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 is mainly responsible. SEE ALSO -------- - +lightning-disconnect(7), lightning-fundchannel(7) RESOURCES --------- diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index c33988945..70ce43d10 100644 --- a/lightningd/peer_control.c +++ b/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); diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c index e277f95b9..eaa25322e 100644 --- a/wallet/test/run-wallet.c +++ b/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(); }