Add a test for 'fundchannel_start' crash on deconnection

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

channeld/full_channel.c

@@ -367,6 +367,7 @@ static enum channel_add_err add_htlc(struct channel *channel,
 	enum side sender = htlc_state_owner(state), recipient = !sender;
 	const struct htlc **committed, **adding, **removing;
 	const struct channel_view *view;
+	u32 min_concurrent_htlcs;
 
 	htlc = tal(tmpctx, struct htlc);
 
@@ -443,8 +444,16 @@ static enum channel_add_err add_htlc(struct channel *channel,
 	 *     HTLCs to its local commitment transaction...
 	 *     - SHOULD fail the channel.
 	 */
+	/* Also we should not add more htlc's than sender or recipient
+	 * configured.  This mitigates attacks in which a peer can force the
+	 * funder of the channel to pay unnecessary onchain fees during a fee
+	 * spike with large commitment transactions.
+	 */
+	min_concurrent_htlcs = channel->config[recipient].max_accepted_htlcs;
+	if (min_concurrent_htlcs > channel->config[sender].max_accepted_htlcs)
+		min_concurrent_htlcs = channel->config[sender].max_accepted_htlcs;
 	if (tal_count(committed) - tal_count(removing) + tal_count(adding)
-	    > channel->config[recipient].max_accepted_htlcs) {
+	    > min_concurrent_htlcs) {
 		return CHANNEL_ERR_TOO_MANY_HTLCS;
 	}
 

contrib/pylightning/lightning/lightning.py

@@ -3,6 +3,7 @@
 import logging
 from math import floor, log10
 import socket
+import warnings
 
 __version__ = "0.0.7.3"
 
@@ -309,16 +310,43 @@ def check(self, command_to_check, **kwargs):
         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,36 +8,41 @@ 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.
 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
 ---------

doc/lightningd-config.5

@@ -2,12 +2,12 @@
 .\"     Title: lightningd-config
 .\"    Author: [see the "AUTHOR" section]
 .\" Generator: DocBook XSL Stylesheets v1.79.1 <http://docbook.sf.net/>
-.\"      Date: 08/04/2019
+.\"      Date: 08/09/2019
 .\"    Manual: \ \&
 .\"    Source: \ \&
 .\"  Language: English
 .\"
-.TH "LIGHTNINGD\-CONFIG" "5" "08/04/2019" "\ \&" "\ \&"
+.TH "LIGHTNINGD\-CONFIG" "5" "08/09/2019" "\ \&" "\ \&"
 .\" -----------------------------------------------------------------
 .\" * Define some portability stuff
 .\" -----------------------------------------------------------------
@@ -257,6 +257,11 @@ Limits on what onchain fee range we\(cqll allow when a node opens a channel with
 can (should!) be greater than 100\&.
 .RE
 .PP
+\fBmax\-concurrent\-htlcs\fR=\fIINTEGER\fR
+.RS 4
+Number of HTLCs one channel can handle concurrently in each direction\&. Should be between 1 and 483 (default 30)\&.
+.RE
+.PP
 \fBcltv\-delta\fR=\fIBLOCKS\fR
 .RS 4
 The number of blocks between incoming payments and outgoing payments: this needs to be enough to make sure that if we have to, we can close the outgoing payment before the incoming, or redeem the incoming once the outgoing is redeemed\&.

doc/lightningd-config.5.txt

@@ -205,6 +205,10 @@ Lightning channel and HTLC options
     they're outside this range, we abort their opening attempt.  Note
     that *commit-fee-max* can (should!) be greater than 100.
 
+*max-concurrent-htlcs*='INTEGER'::
+    Number of HTLCs one channel can handle concurrently in each direction.
+    Should be between 1 and 483 (default 30).
+
 *cltv-delta*='BLOCKS'::
     The number of blocks between incoming payments and outgoing payments:
     this needs to be enough to make sure that if we have to, we can close

gossipd/gossip_store.c

@@ -544,8 +544,13 @@ const u8 *gossip_store_get(const tal_t *ctx,
 			      offset, gs->len, strerror(errno));
 	}
 
-	/* FIXME: We should skip over these deleted entries! */
-	msglen = be32_to_cpu(hdr.len) & ~GOSSIP_STORE_LEN_DELETED_BIT;
+	if (be32_to_cpu(hdr.len) & GOSSIP_STORE_LEN_DELETED_BIT)
+		status_failed(STATUS_FAIL_INTERNAL_ERROR,
+			      "gossip_store: get delete entry offset %"PRIu64
+			      "/%"PRIu64"",
+			      offset, gs->len);
+
+	msglen = be32_to_cpu(hdr.len);
 	checksum = be32_to_cpu(hdr.crc);
 	msg = tal_arr(ctx, u8, msglen);
 	if (pread(gs->fd, msg, msglen, offset + sizeof(hdr)) != msglen)

lightningd/lightningd.h

@@ -42,6 +42,9 @@ struct config {
 	u32 fee_base;
 	u32 fee_per_satoshi;
 
+	/* htlcs per channel */
+	u32 max_concurrent_htlcs;
+
 	/* How long between changing commit and sending COMMIT message. */
 	u32 commit_time_ms;
 

lightningd/opening_control.c

@@ -99,7 +99,7 @@ static void uncommitted_channel_disconnect(struct uncommitted_channel *uc,
 	u8 *msg = towire_connectctl_peer_disconnected(tmpctx, &uc->peer->id);
 	log_info(uc->log, "%s", desc);
 	subd_send_msg(uc->peer->ld->connectd, msg);
-	if (uc->fc)
+	if (uc->fc && uc->fc->cmd)
 		was_pending(command_fail(uc->fc->cmd, LIGHTNINGD, "%s", desc));
 	notify_disconnect(uc->peer->ld, &uc->peer->id);
 }
@@ -835,13 +835,7 @@ static void channel_config(struct lightningd *ld,
 	 */
 	 ours->to_self_delay = ld->config.locktime_blocks;
 
-	 /* BOLT #2:
-	  *
-	  * The receiving node MUST fail the channel if:
-	  *...
-	  *   - `max_accepted_htlcs` is greater than 483.
-	  */
-	 ours->max_accepted_htlcs = 483;
+	 ours->max_accepted_htlcs = ld->config.max_concurrent_htlcs;
 
 	 /* This is filled in by lightning_openingd, for consistency. */
 	 ours->channel_reserve = AMOUNT_SAT(UINT64_MAX);

lightningd/options.c

@@ -457,6 +457,9 @@ static const struct config testnet_config = {
 	/* We offer to pay 5 times 2-block fee */
 	.commitment_fee_percent = 500,
 
+	/* Testnet blockspace is free. */
+	.max_concurrent_htlcs = 483,
+
 	/* Be aggressive on testnet. */
 	.cltv_expiry_delta = 6,
 	.cltv_final = 10,
@@ -513,6 +516,9 @@ static const struct config mainnet_config = {
 	/* We offer to pay 5 times 2-block fee */
 	.commitment_fee_percent = 500,
 
+	/* While up to 483 htlcs are possible we do 30 by default (as eclair does) to save blockspace */
+	.max_concurrent_htlcs = 30,
+
 	/* BOLT #2:
 	 *
 	 * 1. the `cltv_expiry_delta` for channels, `3R+2G+2S`: if in doubt, a
@@ -569,7 +575,15 @@ static void check_config(struct lightningd *ld)
 		fatal("Commitment fee invalid min-max %u-%u",
 		      ld->config.commitment_fee_min_percent,
 		      ld->config.commitment_fee_max_percent);
-
+	/* BOLT #2:
+	 *
+	 * The receiving node MUST fail the channel if:
+	 *...
+	 *   - `max_accepted_htlcs` is greater than 483.
+	 */
+	if (ld->config.max_concurrent_htlcs < 1 || ld->config.max_concurrent_htlcs > 483)
+		fatal("--max-concurrent-htlcs value must be between 1 and 483 it is: %u",
+		      ld->config.max_concurrent_htlcs);
 	if (ld->config.anchor_confirms == 0)
 		fatal("anchor-confirms must be greater than zero");
 
@@ -928,6 +942,9 @@ static void register_opts(struct lightningd *ld)
 	opt_register_arg("--fee-per-satoshi", opt_set_u32, opt_show_u32,
 			 &ld->config.fee_per_satoshi,
 			 "Microsatoshi fee for every satoshi in HTLC");
+	opt_register_arg("--max-concurrent-htlcs", opt_set_u32, opt_show_u32,
+			 &ld->config.max_concurrent_htlcs,
+			 "Number of HTLCs one channel can handle concurrently. Should be between 1 and 483");
 	opt_register_arg("--min-capacity-sat", opt_set_u64, opt_show_u64,
 			 &ld->config.min_capacity_sat,
 			 "Minimum capacity in satoshis for accepting channels");