lightningd: really do allow two Torv3 addresses.

This surprised me, since the CHANGELOG for [0.8.2] said:

	We now announce multiple addresses of the same type, if given. ([3609](https://github.com/ElementsProject/lightning/pull/3609))

But it lied!

Changelog-Fixed: We really do allow providing multiple addresses of the same type.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

common/test/run-wireaddr.c

@@ -112,39 +112,6 @@ void towire_u8_array(u8 **pptr UNNEEDED, const u8 *arr UNNEEDED, size_t num UNNE
 { fprintf(stderr, "towire_u8_array called!\n"); abort(); }
 /* AUTOGENERATED MOCKS END */
 
-static bool wireaddr_internal_eq(const struct wireaddr_internal *a,
-				 const struct wireaddr_internal *b,
-				 bool cmp_torservice_blob)
-{
-	if (a->itype != b->itype)
-		return false;
-
-	switch (a->itype) {
-	case ADDR_INTERNAL_SOCKNAME:
-		return streq(a->u.sockname, b->u.sockname);
-	case ADDR_INTERNAL_ALLPROTO:
-		return a->u.port == b->u.port;
-	case ADDR_INTERNAL_AUTOTOR:
-	case ADDR_INTERNAL_STATICTOR:
-		if (!wireaddr_eq(&a->u.torservice.address,
-				 &b->u.torservice.address))
-			return false;
-		if (a->u.torservice.port != b->u.torservice.port)
-			return false;
-		if (!cmp_torservice_blob)
-			return true;
-		return memeq(a->u.torservice.blob, sizeof(a->u.torservice.blob),
-			     b->u.torservice.blob, sizeof(b->u.torservice.blob));
-	case ADDR_INTERNAL_FORPROXY:
-		if (!streq(a->u.unresolved.name, b->u.unresolved.name))
-			return false;
-		return a->u.unresolved.port == b->u.unresolved.port;
-	case ADDR_INTERNAL_WIREADDR:
-		return wireaddr_eq(&a->u.wireaddr, &b->u.wireaddr);
-	}
-	abort();
-}
-
 int main(int argc, char *argv[])
 {
 	const char *err;
@@ -155,87 +122,98 @@ int main(int argc, char *argv[])
 	assert(parse_wireaddr_internal("127.0.0.1", &addr, DEFAULT_PORT, false, false, false, false, &err));
 	expect->itype = ADDR_INTERNAL_WIREADDR;
 	assert(parse_wireaddr("127.0.0.1:9735", &expect->u.wireaddr, 0, NULL, &err));
-	assert(wireaddr_internal_eq(&addr, expect, false));
+	assert(wireaddr_internal_eq(&addr, expect));
 
 	/* IPv4 address with port. */
 	assert(parse_wireaddr_internal("127.0.0.1:1", &addr, DEFAULT_PORT, false, false, false, false, &err));
 	expect->itype = ADDR_INTERNAL_WIREADDR;
 	assert(parse_wireaddr("127.0.0.1:1", &expect->u.wireaddr, 0, NULL, &err));
-	assert(wireaddr_internal_eq(&addr, expect, false));
+	assert(wireaddr_internal_eq(&addr, expect));
 
 	/* Simple IPv6 address. */
 	assert(parse_wireaddr_internal("::1", &addr, DEFAULT_PORT, false, false, false, false, &err));
 	expect->itype = ADDR_INTERNAL_WIREADDR;
 	assert(parse_wireaddr("::1", &expect->u.wireaddr, DEFAULT_PORT, NULL, &err));
-	assert(wireaddr_internal_eq(&addr, expect, false));
+	assert(wireaddr_internal_eq(&addr, expect));
 
 	/* IPv6 address with port. */
 	assert(parse_wireaddr_internal("[::1]:1", &addr, DEFAULT_PORT, false, false, false, false, &err));
 	expect->itype = ADDR_INTERNAL_WIREADDR;
 	assert(parse_wireaddr("::1", &expect->u.wireaddr, 1, NULL, &err));
-	assert(wireaddr_internal_eq(&addr, expect, false));
+	assert(wireaddr_internal_eq(&addr, expect));
 
 	/* autotor address */
 	assert(parse_wireaddr_internal("autotor:127.0.0.1", &addr, DEFAULT_PORT, false, false, false, false, &err));
 	expect->itype = ADDR_INTERNAL_AUTOTOR;
 	expect->u.torservice.port = DEFAULT_PORT;
 	assert(parse_wireaddr("127.0.0.1", &expect->u.torservice.address, 9051, NULL, &err));
-	assert(wireaddr_internal_eq(&addr, expect, false));
+	assert(wireaddr_internal_eq(&addr, expect));
 
 	/* autotor address with port */
 	assert(parse_wireaddr_internal("autotor:127.0.0.1:9055", &addr, DEFAULT_PORT, false, false, false, false, &err));
 	expect->itype = ADDR_INTERNAL_AUTOTOR;
 	expect->u.torservice.port = DEFAULT_PORT;
 	assert(parse_wireaddr("127.0.0.1", &expect->u.torservice.address, 9055, NULL, &err));
-	assert(wireaddr_internal_eq(&addr, expect, false));
+	assert(wireaddr_internal_eq(&addr, expect));
 
 	/* autotor address with torport */
 	assert(parse_wireaddr_internal("autotor:127.0.0.1/torport=9055", &addr, DEFAULT_PORT, false, false, false, false, &err));
 	expect->itype = ADDR_INTERNAL_AUTOTOR;
 	expect->u.torservice.port = 9055;
 	assert(parse_wireaddr("127.0.0.1", &expect->u.torservice.address, 9051, NULL, &err));
-	assert(wireaddr_internal_eq(&addr, expect, false));
+	assert(wireaddr_internal_eq(&addr, expect));
 
 	/* autotor address with port and torport */
 	assert(parse_wireaddr_internal("autotor:127.0.0.1:9055/torport=10055", &addr, DEFAULT_PORT, false, false, false, false, &err));
 	expect->itype = ADDR_INTERNAL_AUTOTOR;
 	expect->u.torservice.port = 10055;
 	assert(parse_wireaddr("127.0.0.1", &expect->u.torservice.address, 9055, NULL, &err));
-	assert(wireaddr_internal_eq(&addr, expect, false));
+	assert(wireaddr_internal_eq(&addr, expect));
 
 	/* statictor address */
 	assert(parse_wireaddr_internal("statictor:127.0.0.1", &addr, DEFAULT_PORT, false, false, false, false, &err));
 	expect->itype = ADDR_INTERNAL_STATICTOR;
 	expect->u.torservice.port = DEFAULT_PORT;
+	memset(expect->u.torservice.blob, 0, sizeof(expect->u.torservice.blob));
+	strcpy((char *)expect->u.torservice.blob, STATIC_TOR_MAGIC_STRING);
 	assert(parse_wireaddr("127.0.0.1", &expect->u.torservice.address, 9051, NULL, &err));
-	assert(wireaddr_internal_eq(&addr, expect, false));
+	assert(wireaddr_internal_eq(&addr, expect));
 
 	/* statictor address with port */
 	assert(parse_wireaddr_internal("statictor:127.0.0.1:9055", &addr, DEFAULT_PORT, false, false, false, false, &err));
 	expect->itype = ADDR_INTERNAL_STATICTOR;
 	expect->u.torservice.port = DEFAULT_PORT;
 	assert(parse_wireaddr("127.0.0.1", &expect->u.torservice.address, 9055, NULL, &err));
-	assert(wireaddr_internal_eq(&addr, expect, false));
+	assert(wireaddr_internal_eq(&addr, expect));
 
 	/* statictor address with torport */
 	assert(parse_wireaddr_internal("statictor:127.0.0.1/torport=9055", &addr, DEFAULT_PORT, false, false, false, false, &err));
 	expect->itype = ADDR_INTERNAL_STATICTOR;
 	expect->u.torservice.port = 9055;
 	assert(parse_wireaddr("127.0.0.1", &expect->u.torservice.address, 9051, NULL, &err));
-	assert(wireaddr_internal_eq(&addr, expect, false));
+	assert(wireaddr_internal_eq(&addr, expect));
 
 	/* statictor address with port and torport */
 	assert(parse_wireaddr_internal("statictor:127.0.0.1:9055/torport=10055", &addr, DEFAULT_PORT, false, false, false, false, &err));
 	expect->itype = ADDR_INTERNAL_STATICTOR;
 	expect->u.torservice.port = 10055;
 	assert(parse_wireaddr("127.0.0.1", &expect->u.torservice.address, 9055, NULL, &err));
-	assert(wireaddr_internal_eq(&addr, expect, false));
+	assert(wireaddr_internal_eq(&addr, expect));
+
+	/* statictor address with port and torport and torblob */
+	assert(parse_wireaddr_internal("statictor:127.0.0.1:9055/torport=10055/torblob=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", &addr, DEFAULT_PORT, false, false, false, false, &err));
+	expect->itype = ADDR_INTERNAL_STATICTOR;
+	expect->u.torservice.port = 10055;
+	/* This is actually nul terminated */
+	memset(expect->u.torservice.blob, 'x', sizeof(expect->u.torservice.blob)-1);
+	assert(parse_wireaddr("127.0.0.1", &expect->u.torservice.address, 9055, NULL, &err));
+	assert(wireaddr_internal_eq(&addr, expect));
 
 	/* local socket path */
 	assert(parse_wireaddr_internal("/tmp/foo.sock", &addr, DEFAULT_PORT, false, false, false, false, &err));
 	expect->itype = ADDR_INTERNAL_SOCKNAME;
 	strcpy(expect->u.sockname, "/tmp/foo.sock");
-	assert(wireaddr_internal_eq(&addr, expect, false));
+	assert(wireaddr_internal_eq(&addr, expect));
 
 	/* Unresolved */
 	assert(!parse_wireaddr_internal("ozlabs.org", &addr, DEFAULT_PORT, false, false, false, false, &err));
@@ -244,7 +222,7 @@ int main(int argc, char *argv[])
 	expect->itype = ADDR_INTERNAL_FORPROXY;
 	strcpy(expect->u.unresolved.name, "ozlabs.org");
 	expect->u.unresolved.port = DEFAULT_PORT;
-	assert(wireaddr_internal_eq(&addr, expect, false));
+	assert(wireaddr_internal_eq(&addr, expect));
 
 	/* Unresolved with port */
 	assert(!parse_wireaddr_internal("ozlabs.org:1234", &addr, DEFAULT_PORT, false, false, false, false, &err));
@@ -253,7 +231,7 @@ int main(int argc, char *argv[])
 	expect->itype = ADDR_INTERNAL_FORPROXY;
 	strcpy(expect->u.unresolved.name, "ozlabs.org");
 	expect->u.unresolved.port = 1234;
-	assert(wireaddr_internal_eq(&addr, expect, false));
+	assert(wireaddr_internal_eq(&addr, expect));
 
 	tal_free(expect);
 	common_shutdown();

common/wireaddr.c

@@ -454,6 +454,37 @@ finish:
 	return res;
 }
 
+bool wireaddr_internal_eq(const struct wireaddr_internal *a,
+			  const struct wireaddr_internal *b)
+{
+	if (a->itype != b->itype)
+		return false;
+
+	switch (a->itype) {
+	case ADDR_INTERNAL_SOCKNAME:
+		return streq(a->u.sockname, b->u.sockname);
+	case ADDR_INTERNAL_ALLPROTO:
+		return a->u.port == b->u.port;
+	case ADDR_INTERNAL_STATICTOR:
+		if (!memeq(a->u.torservice.blob, sizeof(a->u.torservice.blob),
+			   b->u.torservice.blob, sizeof(b->u.torservice.blob)))
+			return false;
+		/* fall thru */
+	case ADDR_INTERNAL_AUTOTOR:
+		if (!wireaddr_eq(&a->u.torservice.address,
+				 &b->u.torservice.address))
+			return false;
+		return a->u.torservice.port == b->u.torservice.port;
+	case ADDR_INTERNAL_FORPROXY:
+		if (!streq(a->u.unresolved.name, b->u.unresolved.name))
+			return false;
+		return a->u.unresolved.port == b->u.unresolved.port;
+	case ADDR_INTERNAL_WIREADDR:
+		return wireaddr_eq(&a->u.wireaddr, &b->u.wireaddr);
+	}
+	abort();
+}
+
 bool parse_wireaddr_internal(const char *arg, struct wireaddr_internal *addr,
 			     u16 port, bool wildcard_ok, bool dns_ok,
 			     bool unresolved_ok, bool allow_deprecated,

common/wireaddr.h

@@ -146,6 +146,9 @@ struct wireaddr_internal {
 		char sockname[sizeof(((struct sockaddr_un *)0)->sun_path)];
 	} u;
 };
+
+bool wireaddr_internal_eq(const struct wireaddr_internal *a,
+			  const struct wireaddr_internal *b);
 bool parse_wireaddr_internal(const char *arg, struct wireaddr_internal *addr,
 			     u16 port, bool wildcard_ok, bool dns_ok,
 			     bool unresolved_ok, bool allow_deprecated,

lightningd/options.c

@@ -198,6 +198,18 @@ static char *opt_add_addr_withtype(const char *arg,
 				     deprecated_apis, &err_msg)) {
 		return tal_fmt(NULL, "Unable to parse address '%s': %s", arg, err_msg);
 	}
+
+	/* Sanity check for exact duplicates. */
+	for (size_t i = 0; i < tal_count(ld->proposed_wireaddr); i++) {
+		/* Only compare announce vs announce and bind vs bind */
+		if ((ld->proposed_listen_announce[i] & ala) == 0)
+			continue;
+
+		if (wireaddr_internal_eq(&ld->proposed_wireaddr[i], &wi))
+			return tal_fmt(NULL, "Duplicate %s address %s",
+				       ala & ADDR_ANNOUNCE ? "announce" : "listen",
+				       type_to_string(tmpctx, struct wireaddr_internal, &wi));
+	}
 	tal_arr_expand(&ld->proposed_wireaddr, wi);
 	return NULL;
 
@@ -205,7 +217,6 @@ static char *opt_add_addr_withtype(const char *arg,
 
 static char *opt_add_announce_addr(const char *arg, struct lightningd *ld)
 {
-	const struct wireaddr *wn;
 	size_t n = tal_count(ld->proposed_wireaddr);
 	char *err;
 
@@ -226,24 +237,6 @@ static char *opt_add_announce_addr(const char *arg, struct lightningd *ld)
 		return tal_fmt(NULL, "address '%s' is not announcable",
 			       arg);
 
-	/* gossipd will refuse to announce the second one, sure, but it's
-	 * better to check and fail now if they've explicitly asked for it. */
-	wn = &ld->proposed_wireaddr[n].u.wireaddr;
-	for (size_t i = 0; i < n; i++) {
-		const struct wireaddr *wi;
-
-		if (ld->proposed_listen_announce[i] != ADDR_ANNOUNCE)
-			continue;
-		assert(ld->proposed_wireaddr[i].itype == ADDR_INTERNAL_WIREADDR);
-		wi = &ld->proposed_wireaddr[i].u.wireaddr;
-
-		if (wn->type != wi->type)
-			continue;
-		return tal_fmt(NULL, "Cannot announce address %s;"
-			       " already have %s which is the same type",
-			       type_to_string(tmpctx, struct wireaddr, wn),
-			       type_to_string(tmpctx, struct wireaddr, wi));
-	}
 	return NULL;
 }
 

tests/test_gossip.py

@@ -916,8 +916,11 @@ def test_gossip_addresses(node_factory, bitcoind):
     l1 = node_factory.get_node(options={
         'announce-addr': [
             '[::]:3',
+            '[::]',
             '127.0.0.1:2',
+            '127.0.0.1',
             'vww6ybal4bd7szmgncyruucpgfkqahzddi37ktceo3ah7ngmcopnpyyd.onion',
+            '4acth47i6kxnvkewtm6q7ib2s3ufpo5sqbsnzjpbi7utijcltosqemad.onion:1234'
         ],
     })
     l2 = node_factory.get_node()
@@ -931,8 +934,11 @@ def test_gossip_addresses(node_factory, bitcoind):
     nodes = l2.rpc.listnodes(l1.info['id'])['nodes']
     assert len(nodes) == 1 and nodes[0]['addresses'] == [
         {'type': 'ipv4', 'address': '127.0.0.1', 'port': 2},
+        {'type': 'ipv4', 'address': '127.0.0.1', 'port': 9735},
         {'type': 'ipv6', 'address': '::', 'port': 3},
+        {'type': 'ipv6', 'address': '::', 'port': 9735},
         {'type': 'torv3', 'address': 'vww6ybal4bd7szmgncyruucpgfkqahzddi37ktceo3ah7ngmcopnpyyd.onion', 'port': 9735},
+        {'type': 'torv3', 'address': '4acth47i6kxnvkewtm6q7ib2s3ufpo5sqbsnzjpbi7utijcltosqemad.onion', 'port': 1234},
     ]