gossipd: order node_announcement addresses correctly, remove duplicate types.

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

gossipd/gossip.c

@@ -2573,6 +2573,45 @@ static void add_binding(struct wireaddr_internal **binding,
 	(*binding)[n] = *addr;
 }
 
+static int wireaddr_cmp_type(const struct wireaddr *a,
+			     const struct wireaddr *b, void *unused)
+{
+	return (int)a->type - (int)b->type;
+}
+
+static void finalize_announcable(struct daemon *daemon)
+{
+	size_t n = tal_count(daemon->announcable);
+
+	/* BOLT #7:
+	 *
+	 * The origin node:
+	 *...
+	 *   - MUST place non-zero typed address descriptors in ascending order.
+	 *...
+	 *   - MUST NOT include more than one `address descriptor` of the same
+	 *     type.
+	 */
+	asort(daemon->announcable, n, wireaddr_cmp_type, NULL);
+	for (size_t i = 1; i < n; i++) {
+		/* Note we use > instead of !=: catches asort bugs too. */
+		if (daemon->announcable[i].type > daemon->announcable[i-1].type)
+			continue;
+
+		status_unusual("WARNING: Cannot announce address %s,"
+			       " already announcing %s",
+			       type_to_string(tmpctx, struct wireaddr,
+					      &daemon->announcable[i]),
+			       type_to_string(tmpctx, struct wireaddr,
+					      &daemon->announcable[i-1]));
+		memmove(daemon->announcable + i,
+			daemon->announcable + i + 1,
+			(n - i - 1) * sizeof(daemon->announcable[0]));
+		tal_resize(&daemon->announcable, --n);
+		--i;
+	}
+}
+
 /* Initializes daemon->announcable array, returns addresses we bound to. */
 static struct wireaddr_internal *setup_listeners(const tal_t *ctx,
 						 struct daemon *daemon)
@@ -2584,19 +2623,29 @@ static struct wireaddr_internal *setup_listeners(const tal_t *ctx,
 	binding = tal_arr(ctx, struct wireaddr_internal, 0);
 	daemon->announcable = tal_arr(daemon, struct wireaddr, 0);
 
+	/* Add addresses we've explicitly been told to *first*: implicit
+	 * addresses will be discarded then if we have multiple. */
+	for (size_t i = 0; i < tal_count(daemon->proposed_wireaddr); i++) {
+		struct wireaddr_internal wa = daemon->proposed_wireaddr[i];
+
+		if (daemon->proposed_listen_announce[i] & ADDR_LISTEN)
+			continue;
+
+		assert(daemon->proposed_listen_announce[i] & ADDR_ANNOUNCE);
+		/* You can only announce wiretypes! */
+		assert(daemon->proposed_wireaddr[i].itype
+		       == ADDR_INTERNAL_WIREADDR);
+		add_announcable(daemon, &wa.u.wireaddr);
+	}
+
+	/* Now look for listening addresses. */
 	for (size_t i = 0; i < tal_count(daemon->proposed_wireaddr); i++) {
 		struct wireaddr_internal wa = daemon->proposed_wireaddr[i];
 		bool announce = (daemon->proposed_listen_announce[i]
 				 & ADDR_ANNOUNCE);
 
-		if (!(daemon->proposed_listen_announce[i] & ADDR_LISTEN)) {
-			assert(announce);
-			/* You can only announce wiretypes! */
-			assert(daemon->proposed_wireaddr[i].itype
-			       == ADDR_INTERNAL_WIREADDR);
-			add_announcable(daemon, &wa.u.wireaddr);
+		if (!(daemon->proposed_listen_announce[i] & ADDR_LISTEN))
 			continue;
-		}
 
 		switch (wa.itype) {
 		case ADDR_INTERNAL_SOCKNAME:
@@ -2679,6 +2728,9 @@ static struct wireaddr_internal *setup_listeners(const tal_t *ctx,
 						daemon->tor_password,
 						binding));
 	}
+
+	finalize_announcable(daemon);
+
 	return binding;
 }
 

tests/test_gossip.py

@@ -2,6 +2,7 @@ from fixtures import *  # noqa: F401,F403
 from test_lightningd import wait_for
 
 import os
+import subprocess
 import time
 import unittest
 
@@ -97,3 +98,32 @@ def test_gossip_disable_channels(node_factory, bitcoind):
 
     wait_for(lambda: count_active(l1) == 2)
     wait_for(lambda: count_active(l2) == 2)
+
+
+def test_announce_address(node_factory, bitcoind):
+    """Make sure our announcements are well formed."""
+
+    # We do not allow announcement of duplicates.
+    opts = {'announce-addr':
+            ['4acth47i6kxnvkewtm6q7ib2s3ufpo5sqbsnzjpbi7utijcltosqemad.onion',
+             'lldan5gahapx5k7iafb3s4ikijc4ni7gx5iywdflkba5y2ezyg6sjgyd.onion',
+             'silkroad6ownowfk.onion',
+             'silkroad7rn2puhj.onion',
+             '1.2.3.4:1234',
+             '192.168.1.1',
+             '::',
+             '2001:0db8:85a3:0000:0000:8a2e:0370:7334'],
+            'log-level': 'io'}
+    l1, l2 = node_factory.get_nodes(2, opts=[opts, {}])
+    l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
+    scid = l1.fund_channel(l2, 10**6)
+    bitcoind.generate_block(5)
+
+    # Activate IO logging for l1.
+    subprocess.run(['kill', '-USR1', l1.subd_pid('channeld')])
+
+    l1.wait_channel_active(scid)
+    l2.wait_channel_active(scid)
+
+    # We should see it send node announce (257 = 0x0101)
+    l1.daemon.wait_for_log("\[OUT\] 0101.*004d010102030404d202000000000000000000000000000000002607039216a8b803f3acd758aa260704e00533f3e8f2aedaa8969b3d0fa03a96e857bbb28064dca5e147e934244b9ba50230032607'")