channeld: order htlcs by id before retransmission.

We had one report of this, and then Eugene and Roasbeef of Lightning
Labs confirmed it; they saw misordered HTLCs on reconnection too.

Since we didn't enforce this when we receive HTLCs, we never noticed :(

Fixes: #3920
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Fixed: Protocol: fixed retransmission order of multiple new HTLCs (causing channel close with LND)

channeld/channeld.c

@@ -15,6 +15,7 @@
 #include <bitcoin/psbt.h>
 #include <bitcoin/script.h>
 #include <ccan/array_size/array_size.h>
+#include <ccan/asort/asort.h>
 #include <ccan/cast/cast.h>
 #include <ccan/container_of/container_of.h>
 #include <ccan/crypto/hkdf_sha256/hkdf_sha256.h>
@@ -2190,7 +2191,20 @@ static void send_fail_or_fulfill(struct peer *peer, const struct htlc *h)
 	sync_crypto_write(peer->pps, take(msg));
 }
 
-static void resend_commitment(struct peer *peer, const struct changed_htlc *last)
+static int cmp_changed_htlc_id(const struct changed_htlc *a,
+			       const struct changed_htlc *b,
+			       void *unused)
+{
+	/* ids can be the same (sender and receiver are indep) but in
+	 * that case we don't care about order. */
+	if (a->id > b->id)
+		return 1;
+	else if (a->id < b->id)
+		return -1;
+	return 0;
+}
+
+static void resend_commitment(struct peer *peer, struct changed_htlc *last)
 {
 	size_t i;
 	struct bitcoin_signature commit_sig, *htlc_sigs;
@@ -2204,6 +2218,12 @@ static void resend_commitment(struct peer *peer, const struct changed_htlc *last
 		     channel_feerate(peer->channel, LOCAL),
 		     channel_feerate(peer->channel, REMOTE));
 
+	/* Note that HTLCs must be *added* in order.  Simplest thing to do
+	 * is to sort them all into ascending ID order here (we could do
+	 * this when we save them in channel_sending_commit, but older versions
+	 * won't have them sorted in the db, so doing it here is better). */
+	asort(last, tal_count(last), cmp_changed_htlc_id, NULL);
+
 	/* BOLT #2:
 	 *
 	 *   - if `next_commitment_number` is equal to the commitment

tests/test_connection.py

@@ -2193,7 +2193,6 @@ def test_dataloss_protection(node_factory, bitcoind):
     assert (closetxid, "confirmed") in set([(o['txid'], o['status']) for o in l2.rpc.listfunds()['outputs']])
 
 
-@unittest.skip("Broken")
 @unittest.skipIf(not DEVELOPER, "needs dev_disconnect")
 def test_restart_multi_htlc_rexmit(node_factory, bitcoind, executor):
     # l1 disables commit timer once we send first htlc, dies on commit
@@ -2663,7 +2662,6 @@ def test_connection_timeout(node_factory):
     l1.daemon.wait_for_log('conn timed out')
 
 
-@unittest.skip("Broken")
 @unittest.skipIf(not DEVELOPER, "needs --dev-disconnect")
 def test_htlc_retransmit_order(node_factory, executor):
     NUM_HTLCS = 10