Merge remote-tracking branch 'origin/test2' into 0.9.2

CHANGELOG.md

@@ -4,11 +4,11 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
-## [0.9.2rc1] - 2020-11-10: T.B.D.
+## [0.9.2] - 2020-11-20: Now with 0-of-N Multisig
 
 This release named by Sergi Delgado Segura.
 
-* Requires bitcoind v0.20.1 or above *
+* Note: PSBTs now require bitcoind v0.20.1 or above *
 
 ### Added
 
@@ -43,7 +43,6 @@ This release named by Sergi Delgado Segura.
 
 Note: You should always set `allow-deprecated-apis=false` to test for changes.
 
- - Plugins: hooks should now be specified using objects, not raw names. ([4168](https://github.com/ElementsProject/lightning/pull/4168))
  - cli: scripts should filter out '^# ' or use `-N none`, as commands will start returning notifications soon ([4046](https://github.com/ElementsProject/lightning/pull/4046))
 
 ### Removed
@@ -943,7 +942,7 @@ There predate the BOLT specifications, and are only of vague historic interest:
 6. [0.5.1] - 2016-10-21
 7. [0.5.2] - 2016-11-21: "Bitcoin Savings & Trust Daily Interest II"
 
-[0.9.2rc1]: https://github.com/ElementsProject/lightning/releases/tag/v0.9.2rc1
+[0.9.2]: https://github.com/ElementsProject/lightning/releases/tag/v0.9.2
 [0.9.1]: https://github.com/ElementsProject/lightning/releases/tag/v0.9.1
 [0.9.0]: https://github.com/ElementsProject/lightning/releases/tag/v0.9.0
 [0.8.2]: https://github.com/ElementsProject/lightning/releases/tag/v0.8.2

connectd/connectd.c

@@ -173,6 +173,8 @@ struct connecting {
 
 	struct daemon *daemon;
 
+	struct io_conn *conn;
+
 	/* The ID of the peer (not necessarily unique, in transit!) */
 	struct node_id id;
 
@@ -276,16 +278,26 @@ static void connected_to_peer(struct daemon *daemon,
 			      struct io_conn *conn,
 			      const struct node_id *id)
 {
-	/* Don't call destroy_io_conn */
-	io_set_finish(conn, NULL, NULL);
+	struct connecting *outgoing;
 
 	/* We allocate 'conn' as a child of 'connect': we don't want to free
 	 * it just yet though.  tal_steal() it onto the permanent 'daemon'
 	 * struct. */
 	tal_steal(daemon, conn);
 
-	/* Now free the 'connecting' struct. */
-	tal_free(find_connecting(daemon, id));
+	/* This is either us (if conn is an outgoing connection), or
+	 * NULL or a separate attempt (if we're an incoming): in
+	 * that case, kill the outgoing in favor of our successful
+	 * incoming connection. */
+	outgoing = find_connecting(daemon, id);
+	if (outgoing) {
+		/* Don't call destroy_io_conn, since we're done. */
+		if (outgoing->conn)
+			io_set_finish(outgoing->conn, NULL, NULL);
+
+		/* Now free the 'connecting' struct. */
+		tal_free(outgoing);
+	}
 }
 
 /*~ Every per-peer daemon needs a connection to the gossip daemon; this allows
@@ -669,7 +681,7 @@ void add_errors_to_error_list(struct connecting *connect, const char *error)
 		       "%s. ", error);
 }
 
-/*~ This is the destructor for the (unsuccessful) connection.  We accumulate
+/*~ This is the destructor for the (unsuccessful) outgoing connection.  We accumulate
  * the errors which occurred, so we can report to lightningd properly in case
  * they all fail, and try the next address.
  *
@@ -782,6 +794,9 @@ static void try_connect_one_addr(struct connecting *connect)
 	bool use_proxy = connect->daemon->use_proxy_always;
 	const struct wireaddr_internal *addr = &connect->addrs[connect->addrnum];
 
+	/* In case we fail without a connection, make destroy_io_conn happy */
+	connect->conn = NULL;
+
 	/* Out of addresses? */
 	if (connect->addrnum == tal_count(connect->addrs)) {
 		connect_failed(connect->daemon, &connect->id,
@@ -860,9 +875,9 @@ static void try_connect_one_addr(struct connecting *connect)
 	/* This creates the new connection using our fd, with the initialization
 	 * function one of the above. */
 	if (use_proxy)
-		notleak(io_new_conn(connect, fd, conn_proxy_init, connect));
+		connect->conn = io_new_conn(connect, fd, conn_proxy_init, connect);
 	else
-		notleak(io_new_conn(connect, fd, conn_init, connect));
+		connect->conn = io_new_conn(connect, fd, conn_init, connect);
 }
 
 /*~ connectd is responsible for incoming connections, but it's the process of
@@ -1458,8 +1473,17 @@ static void try_connect_peer(struct daemon *daemon,
 		return;
 
 	/* If we're trying to connect it right now, that's OK. */
-	if (find_connecting(daemon, id))
+	if ((connect = find_connecting(daemon, id))) {
+		/* If we've been passed in new connection details
+		 * for this connection, update our addrhint + add
+		 * to addresses to check */
+		if (addrhint) {
+			connect->addrhint = tal_steal(connect, addrhint);
+			tal_arr_expand(&connect->addrs, *addrhint);
+		}
+
 		return;
+	}
 
 	/* Start an array of addresses to try. */
 	addrs = tal_arr(tmpctx, struct wireaddr_internal, 0);

lightningd/plugin.c

@@ -1108,12 +1108,10 @@ static const char *plugin_hooks_add(struct plugin *plugin, const char *buffer,
 			name = json_strdup(tmpctx, buffer, nametok);
 			beforetok = json_get_member(buffer, t, "before");
 			aftertok = json_get_member(buffer, t, "after");
-		} else if (deprecated_apis) {
+		} else {
+			/* FIXME: deprecate in 3 releases after v0.9.2! */
 			name = json_strdup(tmpctx, plugin->buffer, t);
 			beforetok = aftertok = NULL;
-		} else {
-			return tal_fmt(plugin,
-				    "hooks must be an array of objects");
 		}
 
 		hook = plugin_hook_register(plugin, name);

plugins/libplugin-pay.c

@@ -444,6 +444,21 @@ payment_constraints_update(struct payment_constraints *cons,
 	return true;
 }
 
+static struct channel_hint *payment_chanhints_get(struct payment *p,
+						  struct route_hop *h)
+{
+	struct payment *root = payment_root(p);
+	struct channel_hint *curhint;
+	for (size_t j = 0; j < tal_count(root->channel_hints); j++) {
+		curhint = &root->channel_hints[j];
+		if (short_channel_id_eq(&curhint->scid.scid, &h->channel_id) &&
+		    curhint->scid.dir == h->direction) {
+			return curhint;
+		}
+	}
+	return NULL;
+}
+
 /* Given a route and a couple of channel hints, apply the route to the channel
  * hints, so we have a better estimation of channel's capacity. We apply a
  * route to a channel hint before calling `sendonion` so subsequent `route`
@@ -453,57 +468,89 @@ payment_constraints_update(struct payment_constraints *cons,
  * through, since the balances really changed in that case. The `remove`
  * argument indicates whether we want to apply (`remove=false`), or clear a
  * prior application (`remove=true`). */
-static void payment_chanhints_apply_route(struct payment *p, bool remove)
+static bool payment_chanhints_apply_route(struct payment *p, bool remove)
 {
+	bool apply;
 	struct route_hop *curhop;
 	struct channel_hint *curhint;
 	struct payment *root = payment_root(p);
 	assert(p->route != NULL);
+
+	/* No need to check for applicability if we increase
+	 * capacity and budgets. */
+	if (remove)
+		goto apply_changes;
+
+	/* First round: make sure we can cleanly apply the update. */
 	for (size_t i = 0; i < tal_count(p->route); i++) {
 		curhop = &p->route[i];
-		for (size_t j = 0; j < tal_count(root->channel_hints); j++) {
-			curhint = &root->channel_hints[j];
-			if (short_channel_id_eq(&curhint->scid.scid,
-						&curhop->channel_id) &&
-			    curhint->scid.dir == curhop->direction) {
-
-				/* Update the number of htlcs for any local
-				 * channel in the route */
-				if (curhint->local && remove)
-					curhint->htlc_budget++;
-				else if (curhint->local)
-					curhint->htlc_budget--;
-
-				if (remove && !amount_msat_add(
-						  &curhint->estimated_capacity,
-						  curhint->estimated_capacity,
-						  curhop->amount)) {
-					/* This should never happen, it'd mean
-					 * that we unapply a route that would
-					 * result in a msatoshi
-					 * wrap-around. */
-					abort();
-				} else if (!amount_msat_sub(
-					       &curhint->estimated_capacity,
-					       curhint->estimated_capacity,
-					       curhop->amount)) {
-					/* This can happen in case of multipl
-					 * concurrent getroute calls using the
-					 * same channel_hints, no biggy, it's
-					 * an estimation anyway. */
-					paymod_log(
-					    p, LOG_UNUSUAL,
-					    "Could not update the channel hint "
-					    "for %s. Could be a concurrent "
-					    "`getroute` call.",
-					    type_to_string(
-						tmpctx,
-						struct short_channel_id_dir,
-						&curhint->scid));
-				}
-			}
+		curhint = payment_chanhints_get(root, curhop);
+
+		/* If we don't have a hint we can't fail updating it. */
+		if (!curhint)
+			continue;
+
+		/* For local channels we check that we don't overwhelm
+		 * them with too many HTLCs. */
+		apply = (!curhint->local) || curhint->htlc_budget > 0;
+
+		/* For all channels we check that they have a
+		 * sufficiently large estimated capacity to have some
+		 * chance of succeeding. */
+		apply &= amount_msat_greater(curhint->estimated_capacity,
+					     curhop->amount);
+
+		if (!apply) {
+			/* This can happen in case of multiple
+			 * concurrent getroute calls using the
+			 * same channel_hints, no biggy, it's
+			 * an estimation anyway. */
+			paymod_log(p, LOG_DBG,
+				   "Could not update the channel hint "
+				   "for %s. Could be a concurrent "
+				   "`getroute` call.",
+				   type_to_string(tmpctx,
+						  struct short_channel_id_dir,
+						  &curhint->scid));
+			return false;
 		}
 	}
+
+apply_changes:
+	/* Second round: apply the changes, now that we know they'll succeed. */
+	for (size_t i = 0; i < tal_count(p->route); i++) {
+		curhop = &p->route[i];
+		curhint = payment_chanhints_get(root, curhop);
+		if (!curhint)
+			continue;
+
+		/* Update the number of htlcs for any local
+		 * channel in the route */
+		if (curhint->local && remove)
+			curhint->htlc_budget++;
+		else if (curhint->local)
+			curhint->htlc_budget--;
+
+		if (remove && !amount_msat_add(
+			    &curhint->estimated_capacity,
+			    curhint->estimated_capacity,
+			    curhop->amount)) {
+			/* This should never happen, it'd mean
+			 * that we unapply a route that would
+			 * result in a msatoshi
+			 * wrap-around. */
+			abort();
+		} else if (!amount_msat_sub(
+				   &curhint->estimated_capacity,
+				   curhint->estimated_capacity,
+				   curhop->amount)) {
+			/* Given our preemptive test
+			 * above, this should never
+			 * happen either. */
+			abort();
+		}
+	}
+	return true;
 }
 
 static const struct short_channel_id_dir *
@@ -1364,8 +1411,6 @@ static struct command_result *payment_createonion_success(struct command *cmd,
 	struct route_hop *first = &p->route[0];
 	struct secret *secrets;
 
-	payment_chanhints_apply_route(p, false);
-
 	p->createonion_response = json_to_createonion_response(p, buffer, toks);
 
 	req = jsonrpc_request_start(p->plugin, NULL, "sendonion",
@@ -1486,6 +1531,19 @@ static void payment_compute_onion_payloads(struct payment *p)
 	p->step = PAYMENT_STEP_ONION_PAYLOAD;
 	hopcount = tal_count(p->route);
 
+	/* Now that we are about to fix the route parameters by
+	 * encoding them in an onion is the right time to update the
+	 * channel hints. */
+	if (!payment_chanhints_apply_route(p, false)) {
+		/* We can still end up with a failed channel_hints
+		 * update, either because a plugin changed the route,
+		 * or because a modifier was not synchronous, allowing
+		 * for multiple concurrent routes being built. If that
+		 * is the case, discard this route and retry. */
+		payment_set_step(p, PAYMENT_STEP_RETRY_GETROUTE);
+		return payment_continue(p);
+	}
+
 	/* Now compute the payload we're about to pass to `createonion` */
 	cr = p->createonion_request = tal(p, struct createonion_request);
 	cr->assocdata = tal_arr(cr, u8, 0);
@@ -1832,6 +1890,7 @@ void payment_continue(struct payment *p)
 		p->current_modifier = -1;
 		switch (p->step) {
 		case PAYMENT_STEP_INITIALIZED:
+		case PAYMENT_STEP_RETRY_GETROUTE:
 			payment_getroute(p);
 			return;
 

plugins/libplugin-pay.h

@@ -94,6 +94,11 @@ enum payment_step {
 	 * to amend the route. */
 	PAYMENT_STEP_GOT_ROUTE = 2,
 
+	/* Something went wrong with the route returned by the
+	previous step, so retry, but do not rerun the INITIALIZED
+	modifiers. */
+	PAYMENT_STEP_RETRY_GETROUTE = 3,
+
 	/* We just computed the onion payload, allow modifiers to amend,
 	 * before constructing the onion packet. */
 	PAYMENT_STEP_ONION_PAYLOAD = 4,

tests/plugins/slow_start.py

@@ -0,0 +1,33 @@
+#!/usr/bin/env python3
+"""This plugin is used to check that updated connection hints work properly.
+
+"""
+from pyln.client import Plugin
+
+import socket
+import time
+
+plugin = Plugin()
+
+
+@plugin.async_method('waitconn')
+def wait_connection(request, plugin):
+    sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
+    sock.bind(('localhost', 0))
+    sock.listen(1)
+    print("listening for connections on port {}".format(sock.getsockname()[1]))
+
+    # We are a one and done socket connection!
+    conn, client_addr = sock.accept()
+    try:
+        print("connection from {}".format(client_addr))
+        time.sleep(3)
+
+    finally:
+        conn.close()
+
+    print("closing socket")
+    sock.close()
+
+
+plugin.run()