From 2988eb1002543215a52f0e623587fe7c8d1e51ab Mon Sep 17 00:00:00 2001
From: Rusty Russell <rusty@rustcorp.com.au>
Date: Mon, 22 Oct 2018 14:45:35 +1030
Subject: [PATCH 1/8] spelling: Check LockTime Verify.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 lightningd/htlc_end.c   | 2 +-
 lightningd/options.c    | 4 ++--
 tests/test_misc.py      | 2 +-
 tools/check-spelling.sh | 5 +++++
 4 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/lightningd/htlc_end.c b/lightningd/htlc_end.c
index de2c138de15f..38a40e7f217f 100644
--- a/lightningd/htlc_end.c
+++ b/lightningd/htlc_end.c
@@ -152,7 +152,7 @@ struct htlc_out *htlc_out_check(const struct htlc_out *hout,
 				       " less than %"PRIu64,
 				       hout->in->msatoshi, hout->msatoshi);
 		if (hout->in->cltv_expiry <= hout->cltv_expiry)
-			return corrupt(abortstr, "Input ctlv_expiry %u"
+			return corrupt(abortstr, "Input cltv_expiry %u"
 				       " less than %u",
 				       hout->in->cltv_expiry, hout->cltv_expiry);
 		if (!sha256_eq(&hout->in->payment_hash, &hout->payment_hash))
diff --git a/lightningd/options.c b/lightningd/options.c
index 6edc57845aa2..e66600a9e943 100644
--- a/lightningd/options.c
+++ b/lightningd/options.c
@@ -316,10 +316,10 @@ static void config_register_opts(struct lightningd *ld)
 			 "Percentage of fee to request for their commitment");
 	opt_register_arg("--cltv-delta", opt_set_u32, opt_show_u32,
 			 &ld->config.cltv_expiry_delta,
-			 "Number of blocks for ctlv_expiry_delta");
+			 "Number of blocks for cltv_expiry_delta");
 	opt_register_arg("--cltv-final", opt_set_u32, opt_show_u32,
 			 &ld->config.cltv_final,
-			 "Number of blocks for final ctlv_expiry");
+			 "Number of blocks for final cltv_expiry");
 	opt_register_arg("--commit-time=<millseconds>",
 			 opt_set_u32, opt_show_u32,
 			 &ld->config.commit_time_ms,
diff --git a/tests/test_misc.py b/tests/test_misc.py
index 627b63daf58c..9c1371b3efb5 100644
--- a/tests/test_misc.py
+++ b/tests/test_misc.py
@@ -291,7 +291,7 @@ def test_htlc_in_timeout(node_factory, bitcoind, executor):
     # l1 will disconnect and not reconnect.
     l1.daemon.wait_for_log('dev_disconnect: -WIRE_REVOKE_AND_ACK')
 
-    # Deadline HTLC expiry minus 1/2 cltv-expiry delta (rounded up) (== cltv - 3).  ctlv is 5+1.
+    # Deadline HTLC expiry minus 1/2 cltv-expiry delta (rounded up) (== cltv - 3).  cltv is 5+1.
     bitcoind.generate_block(2)
     assert not l2.daemon.is_in_log('hit deadline')
     bitcoind.generate_block(1)
diff --git a/tools/check-spelling.sh b/tools/check-spelling.sh
index e32ce573488f..a9f88697eeb0 100755
--- a/tools/check-spelling.sh
+++ b/tools/check-spelling.sh
@@ -5,3 +5,8 @@ if git --no-pager grep -nHiE 'l[ightn]{6}g|l[ightn]{8}g|ilghtning|lgihtning|lihg
     echo "Is this warning incorrect? Please teach tools/check-spelling.sh about the exciting new word."
     exit 1
 fi
+
+if git --no-pager grep -nHiE 'ctlv' -- . ':!tools/check-spelling.sh'; then
+    echo "It's check lock time verify, not check time lock verify!" >&2
+    exit 1
+fi

From 16e4b2bd9ac4bd47cc505512436666a455ce6aa6 Mon Sep 17 00:00:00 2001
From: Rusty Russell <rusty@rustcorp.com.au>
Date: Mon, 22 Oct 2018 14:47:24 +1030
Subject: [PATCH 2/8] channeld: htlcmap is never NULL.

I audited the callers.  So remove the code which tested this.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 channeld/commit_tx.c | 23 +++++++++--------------
 channeld/commit_tx.h |  2 +-
 2 files changed, 10 insertions(+), 15 deletions(-)

diff --git a/channeld/commit_tx.c b/channeld/commit_tx.c
index ad44e9982217..1eddfcbcf73b 100644
--- a/channeld/commit_tx.c
+++ b/channeld/commit_tx.c
@@ -158,9 +158,7 @@ struct bitcoin_tx *commit_tx(const tal_t *ctx,
 	tx = bitcoin_tx(ctx, 1, untrimmed + 2);
 
 	/* We keep track of which outputs have which HTLCs */
-	if (htlcmap)
-		*htlcmap = tal_arr(tx, const struct htlc *,
-				   tal_count(tx->output));
+	*htlcmap = tal_arr(tx, const struct htlc *, tal_count(tx->output));
 
 	/* This could be done in a single loop, but we follow the BOLT
 	 * literally to make comments in test vectors clearer. */
@@ -177,8 +175,8 @@ struct bitcoin_tx *commit_tx(const tal_t *ctx,
 		if (trim(htlcs[i], feerate_per_kw, dust_limit_satoshis, side))
 			continue;
 		add_offered_htlc_out(tx, n, htlcs[i], keyset);
-		if (htlcmap)
-			(*htlcmap)[n++] = htlcs[i];
+		(*htlcmap)[n] = htlcs[i];
+		n++;
 	}
 
 	/* BOLT #3:
@@ -192,8 +190,8 @@ struct bitcoin_tx *commit_tx(const tal_t *ctx,
 		if (trim(htlcs[i], feerate_per_kw, dust_limit_satoshis, side))
 			continue;
 		add_received_htlc_out(tx, n, htlcs[i], keyset);
-		if (htlcmap)
-			(*htlcmap)[n++] = htlcs[i];
+		(*htlcmap)[n] = htlcs[i];
+		n++;
 	}
 
 	/* BOLT #3:
@@ -206,8 +204,7 @@ struct bitcoin_tx *commit_tx(const tal_t *ctx,
 		u8 *wscript = to_self_wscript(tmpctx, to_self_delay,keyset);
 		tx->output[n].amount = self_pay_msat / 1000;
 		tx->output[n].script = scriptpubkey_p2wsh(tx, wscript);
-		if (htlcmap)
-			(*htlcmap)[n] = NULL;
+		(*htlcmap)[n] = NULL;
 		SUPERVERBOSE("# to-local amount %"PRIu64" wscript %s\n",
 			     tx->output[n].amount,
 			     tal_hex(tmpctx, wscript));
@@ -231,8 +228,7 @@ struct bitcoin_tx *commit_tx(const tal_t *ctx,
 		tx->output[n].amount = other_pay_msat / 1000;
 		tx->output[n].script = scriptpubkey_p2wpkh(tx,
 						   &keyset->other_payment_key);
-		if (htlcmap)
-			(*htlcmap)[n] = NULL;
+		(*htlcmap)[n] = NULL;
 		SUPERVERBOSE("# to-remote amount %"PRIu64" P2WPKH(%s)\n",
 			     tx->output[n].amount,
 			     type_to_string(tmpctx, struct pubkey,
@@ -242,8 +238,7 @@ struct bitcoin_tx *commit_tx(const tal_t *ctx,
 
 	assert(n <= tal_count(tx->output));
 	tal_resize(&tx->output, n);
-	if (htlcmap)
-		tal_resize(htlcmap, n);
+	tal_resize(htlcmap, n);
 
 	/* BOLT #3:
 	 *
@@ -251,7 +246,7 @@ struct bitcoin_tx *commit_tx(const tal_t *ctx,
 	 *    order](#transaction-input-and-output-ordering)
 	 */
 	permute_outputs(tx->output, tal_count(tx->output),
-			htlcmap ? (const void **)*htlcmap : NULL);
+			(const void **)*htlcmap);
 
 	/* BOLT #3:
 	 *
diff --git a/channeld/commit_tx.h b/channeld/commit_tx.h
index 2cf0e7ec8c1c..6f8decd6ddd2 100644
--- a/channeld/commit_tx.h
+++ b/channeld/commit_tx.h
@@ -33,7 +33,7 @@ size_t commit_tx_num_untrimmed(const struct htlc **htlcs,
  * @self_pay_msat: amount to pay directly to self
  * @other_pay_msat: amount to pay directly to the other side
  * @htlcs: tal_arr of htlcs committed by transaction (some may be trimmed)
- * @htlc_map: outputed map of outnum->HTLC (NULL for direct outputs), or NULL.
+ * @htlc_map: outputed map of outnum->HTLC (NULL for direct outputs).
  * @obscured_commitment_number: number to encode in commitment transaction
  * @side: side to generate commitment transaction for.
  *

From 9726ffc29e0fbc268b930d394ab542cb98a90d89 Mon Sep 17 00:00:00 2001
From: Rusty Russell <rusty@rustcorp.com.au>
Date: Mon, 22 Oct 2018 14:50:46 +1030
Subject: [PATCH 3/8] pytest: test for HTLCs with identical payment_hash and
 different CLTVs.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 tests/test_pay.py | 54 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/tests/test_pay.py b/tests/test_pay.py
index 4eea3398ea48..e690bad9577f 100644
--- a/tests/test_pay.py
+++ b/tests/test_pay.py
@@ -1047,3 +1047,57 @@ def test_forward_stats(node_factory, bitcoind):
     assert l2.rpc.getinfo()['msatoshi_fees_collected'] == 1 + amount // 100000
     assert l1.rpc.getinfo()['msatoshi_fees_collected'] == 0
     assert l3.rpc.getinfo()['msatoshi_fees_collected'] == 0
+
+
+@pytest.mark.xfail(strict=True)
+@unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1 for dev_ignore_htlcs")
+def test_htlcs_cltv_only_difference(node_factory, bitcoind):
+    # l1 -> l2 -> l3 -> l4
+    # l4 ignores htlcs, so they stay.
+    # l3 will see a reconnect from l4 when l4 restarts.
+    l1, l2, l3, l4 = node_factory.line_graph(4, announce=True, opts=[{}] * 2 + [{'dev-no-reconnect': None, 'may_reconnect': True}] * 2)
+
+    h = l4.rpc.invoice(msatoshi=10**8, label='x', description='desc')['payment_hash']
+    l4.rpc.dev_ignore_htlcs(id=l3.info['id'], ignore=True)
+
+    # L2 tries to pay
+    r = l2.rpc.getroute(l4.info['id'], 10**8, 1)["route"]
+    l2.rpc.sendpay(r, h)
+
+    # Now increment CLTV
+    bitcoind.generate_block(1)
+    sync_blockheight(bitcoind, [l1, l2, l3, l4])
+
+    # L1 tries to pay
+    r = l1.rpc.getroute(l4.info['id'], 10**8, 1)["route"]
+    l1.rpc.sendpay(r, h)
+
+    # Now increment CLTV
+    bitcoind.generate_block(1)
+    sync_blockheight(bitcoind, [l1, l2, l3, l4])
+
+    # L3 tries to pay
+    r = l3.rpc.getroute(l4.info['id'], 10**8, 1)["route"]
+    l3.rpc.sendpay(r, h)
+
+    # Give them time to go through.
+    time.sleep(5)
+
+    # Will all be connected OK.
+    assert only_one(l1.rpc.listpeers(l2.info['id'])['peers'])['connected']
+    assert only_one(l2.rpc.listpeers(l3.info['id'])['peers'])['connected']
+    assert only_one(l3.rpc.listpeers(l4.info['id'])['peers'])['connected']
+
+    # Restarting tail node will stop it ignoring HTLCs (it will actually
+    # fail them immediately).
+    l4.restart()
+    l3.rpc.connect(l4.info['id'], 'localhost', l4.port)
+
+    wait_for(lambda: only_one(l1.rpc.listpayments()['payments'])['status'] == 'failed')
+    wait_for(lambda: only_one(l2.rpc.listpayments()['payments'])['status'] == 'failed')
+    wait_for(lambda: only_one(l3.rpc.listpayments()['payments'])['status'] == 'failed')
+
+    # Should all still be connected.
+    assert only_one(l1.rpc.listpeers(l2.info['id'])['peers'])['connected']
+    assert only_one(l2.rpc.listpeers(l3.info['id'])['peers'])['connected']
+    assert only_one(l3.rpc.listpeers(l4.info['id'])['peers'])['connected']

From 3cf657d304ddc26945523aa7df8ada35fe26ca09 Mon Sep 17 00:00:00 2001
From: Rusty Russell <rusty@rustcorp.com.au>
Date: Mon, 22 Oct 2018 14:51:05 +1030
Subject: [PATCH 4/8] channeld: tiebreak identical HTLC outputs by CLTV.

This was suggested by Pierre-Marie as the solution to the 'same HTLC,
different CLTV' signature mismatch.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 channeld/commit_tx.c       | 14 ++++++++--
 common/close_tx.c          |  2 +-
 common/funding_tx.c        |  4 +--
 common/initial_commit_tx.c |  2 +-
 common/permute_tx.c        | 56 ++++++++++++++++++++++++++++----------
 common/permute_tx.h        | 18 ++++++++----
 common/withdraw_tx.c       |  4 +--
 tests/test_pay.py          |  1 -
 8 files changed, 73 insertions(+), 28 deletions(-)

diff --git a/channeld/commit_tx.c b/channeld/commit_tx.c
index 1eddfcbcf73b..ca41d722053d 100644
--- a/channeld/commit_tx.c
+++ b/channeld/commit_tx.c
@@ -107,6 +107,7 @@ struct bitcoin_tx *commit_tx(const tal_t *ctx,
 	u64 base_fee_msat;
 	struct bitcoin_tx *tx;
 	size_t i, n, untrimmed;
+	u32 *cltvs;
 
 	assert(self_pay_msat + other_pay_msat <= funding_satoshis * 1000);
 
@@ -160,6 +161,10 @@ struct bitcoin_tx *commit_tx(const tal_t *ctx,
 	/* We keep track of which outputs have which HTLCs */
 	*htlcmap = tal_arr(tx, const struct htlc *, tal_count(tx->output));
 
+	/* We keep cltvs for tie-breaking HTLC outputs; we use the same order
+	 * for sending the htlc txs, so it may matter. */
+	cltvs = tal_arr(tmpctx, u32, tal_count(tx->output));
+
 	/* This could be done in a single loop, but we follow the BOLT
 	 * literally to make comments in test vectors clearer. */
 
@@ -176,6 +181,7 @@ struct bitcoin_tx *commit_tx(const tal_t *ctx,
 			continue;
 		add_offered_htlc_out(tx, n, htlcs[i], keyset);
 		(*htlcmap)[n] = htlcs[i];
+		cltvs[n] = abs_locktime_to_blocks(&htlcs[i]->expiry);
 		n++;
 	}
 
@@ -191,6 +197,7 @@ struct bitcoin_tx *commit_tx(const tal_t *ctx,
 			continue;
 		add_received_htlc_out(tx, n, htlcs[i], keyset);
 		(*htlcmap)[n] = htlcs[i];
+		cltvs[n] = abs_locktime_to_blocks(&htlcs[i]->expiry);
 		n++;
 	}
 
@@ -205,6 +212,8 @@ struct bitcoin_tx *commit_tx(const tal_t *ctx,
 		tx->output[n].amount = self_pay_msat / 1000;
 		tx->output[n].script = scriptpubkey_p2wsh(tx, wscript);
 		(*htlcmap)[n] = NULL;
+		/* We don't assign cltvs[n]: if we use it, order doesn't matter.
+		 * However, valgrind will warn us something wierd is happening */
 		SUPERVERBOSE("# to-local amount %"PRIu64" wscript %s\n",
 			     tx->output[n].amount,
 			     tal_hex(tmpctx, wscript));
@@ -229,6 +238,8 @@ struct bitcoin_tx *commit_tx(const tal_t *ctx,
 		tx->output[n].script = scriptpubkey_p2wpkh(tx,
 						   &keyset->other_payment_key);
 		(*htlcmap)[n] = NULL;
+		/* We don't assign cltvs[n]: if we use it, order doesn't matter.
+		 * However, valgrind will warn us something wierd is happening */
 		SUPERVERBOSE("# to-remote amount %"PRIu64" P2WPKH(%s)\n",
 			     tx->output[n].amount,
 			     type_to_string(tmpctx, struct pubkey,
@@ -245,8 +256,7 @@ struct bitcoin_tx *commit_tx(const tal_t *ctx,
 	 * 7. Sort the outputs into [BIP 69
 	 *    order](#transaction-input-and-output-ordering)
 	 */
-	permute_outputs(tx->output, tal_count(tx->output),
-			(const void **)*htlcmap);
+	permute_outputs(tx->output, cltvs, (const void **)*htlcmap);
 
 	/* BOLT #3:
 	 *
diff --git a/common/close_tx.c b/common/close_tx.c
index 9f880da508d9..6525c9b88ef4 100644
--- a/common/close_tx.c
+++ b/common/close_tx.c
@@ -58,6 +58,6 @@ struct bitcoin_tx *create_close_tx(const tal_t *ctx,
 		return tal_free(tx);
 	tal_resize(&tx->output, num_outputs);
 
-	permute_outputs(tx->output, num_outputs, NULL);
+	permute_outputs(tx->output, NULL, NULL);
 	return tx;
 }
diff --git a/common/funding_tx.c b/common/funding_tx.c
index 35ce457ee5ed..28a5376e5fed 100644
--- a/common/funding_tx.c
+++ b/common/funding_tx.c
@@ -52,12 +52,12 @@ struct bitcoin_tx *funding_tx(const tal_t *ctx,
 		map[1] = int2ptr(1);
 		tx->output[1].script = scriptpubkey_p2wpkh(tx, changekey);
 		tx->output[1].amount = change_satoshis;
-		permute_outputs(tx->output, tal_count(tx->output), map);
+		permute_outputs(tx->output, NULL, map);
 		*outnum = (map[0] == int2ptr(0) ? 0 : 1);
 	} else {
 		*outnum = 0;
 	}
 
-	permute_inputs(tx->input, tal_count(tx->input), (const void **)utxomap);
+	permute_inputs(tx->input, (const void **)utxomap);
 	return tx;
 }
diff --git a/common/initial_commit_tx.c b/common/initial_commit_tx.c
index bdbff1232060..a64b044a42c9 100644
--- a/common/initial_commit_tx.c
+++ b/common/initial_commit_tx.c
@@ -194,7 +194,7 @@ struct bitcoin_tx *initial_commit_tx(const tal_t *ctx,
 	 * 7. Sort the outputs into [BIP 69
 	 *    order](#transaction-input-and-output-ordering)
 	 */
-	permute_outputs(tx->output, tal_count(tx->output), NULL);
+	permute_outputs(tx->output, NULL, NULL);
 
 	/* BOLT #3:
 	 *
diff --git a/common/permute_tx.c b/common/permute_tx.c
index 327d337d5740..e2e8138e96a2 100644
--- a/common/permute_tx.c
+++ b/common/permute_tx.c
@@ -54,10 +54,11 @@ static void swap_inputs(struct bitcoin_tx_input *inputs,
 	}
 }
 
-void permute_inputs(struct bitcoin_tx_input *inputs, size_t num_inputs,
+void permute_inputs(struct bitcoin_tx_input *inputs,
 		    const void **map)
 {
 	size_t i;
+	size_t num_inputs = tal_count(inputs);
 
 	/* We can't permute nothing! */
 	if (num_inputs == 0)
@@ -73,10 +74,10 @@ void permute_inputs(struct bitcoin_tx_input *inputs, size_t num_inputs,
 
 static void swap_outputs(struct bitcoin_tx_output *outputs,
 			 const void **map,
+			 u32 *cltvs,
 			 size_t i1, size_t i2)
 {
 	struct bitcoin_tx_output tmpoutput;
-	const void *tmp;
 
 	if (i1 == i2)
 		return;
@@ -86,49 +87,74 @@ static void swap_outputs(struct bitcoin_tx_output *outputs,
 	outputs[i2] = tmpoutput;
 
 	if (map) {
-		tmp = map[i1];
+		const void *tmp = map[i1];
 		map[i1] = map[i2];
 		map[i2] = tmp;
 	}
+
+	if (cltvs) {
+		u32 tmp = cltvs[i1];
+		cltvs[i1] = cltvs[i2];
+		cltvs[i2] = tmp;
+	}
 }
 
 static bool output_better(const struct bitcoin_tx_output *a,
-			  const struct bitcoin_tx_output *b)
+			  u32 cltv_a,
+			  const struct bitcoin_tx_output *b,
+			  u32 cltv_b)
 {
-	size_t len;
+	size_t len, lena, lenb;
 	int ret;
 
 	if (a->amount != b->amount)
 		return a->amount < b->amount;
 
 	/* Lexicographical sort. */
-	if (tal_count(a->script) < tal_count(b->script))
-		len = tal_count(a->script);
+	lena = tal_count(a->script);
+	lenb = tal_count(b->script);
+	if (lena < lenb)
+		len = lena;
 	else
-		len = tal_count(b->script);
+		len = lenb;
 
 	ret = memcmp(a->script, b->script, len);
 	if (ret != 0)
 		return ret < 0;
 
-	return tal_count(a->script) < tal_count(b->script);
+	if (lena != lenb)
+		return lena < lenb;
+
+	return cltv_a < cltv_b;
+}
+
+static u32 cltv_of(const u32 *cltvs, size_t idx)
+{
+	if (!cltvs)
+		return 0;
+	return cltvs[idx];
 }
 
-static size_t find_best_out(struct bitcoin_tx_output *outputs, size_t num)
+static size_t find_best_out(struct bitcoin_tx_output *outputs,
+			    const u32 *cltvs,
+			    size_t num)
 {
 	size_t i, best = 0;
 
 	for (i = 1; i < num; i++) {
-		if (output_better(&outputs[i], &outputs[best]))
+		if (output_better(&outputs[i], cltv_of(cltvs, i),
+				  &outputs[best], cltv_of(cltvs, best)))
 			best = i;
 	}
 	return best;
 }
 
-void permute_outputs(struct bitcoin_tx_output *outputs, size_t num_outputs,
+void permute_outputs(struct bitcoin_tx_output *outputs,
+		     u32 *cltvs,
 		     const void **map)
 {
 	size_t i;
+	size_t num_outputs = tal_count(outputs);
 
 	/* We can't permute nothing! */
 	if (num_outputs == 0)
@@ -137,7 +163,9 @@ void permute_outputs(struct bitcoin_tx_output *outputs, size_t num_outputs,
 	/* Now do a dumb sort (num_outputs is small). */
 	for (i = 0; i < num_outputs-1; i++) {
 		/* Swap best into first place. */
-		swap_outputs(outputs, map,
-			     i, i + find_best_out(outputs + i, num_outputs - i));
+		swap_outputs(outputs, map, cltvs,
+			     i, i + find_best_out(outputs + i,
+						  cltvs ? cltvs + i : NULL,
+						  num_outputs - i));
 	}
 }
diff --git a/common/permute_tx.h b/common/permute_tx.h
index f5bbfad405de..6968c94fa185 100644
--- a/common/permute_tx.h
+++ b/common/permute_tx.h
@@ -5,15 +5,23 @@
 
 struct htlc;
 
-/* Permute the transaction into BIP69 order. */
-void permute_inputs(struct bitcoin_tx_input *inputs, size_t num_inputs,
-		    const void **map);
+/**
+ * permute_inputs: permute the transaction inputs into BIP69 order.
+ * @inputs: usually bitcoin_tx->inputs, must be tal_arr.
+ * @map: if non-NULL, pointers to be permuted the same as the inputs.
+ */
+void permute_inputs(struct bitcoin_tx_input *inputs, const void **map);
 
-/* If @map is non-NULL, it will be permuted the same as the outputs.
+/**
+ * permute_outputs: permute the transaction outputs into BIP69 + cltv order.
+ * @outputs: usually bitcoin_tx->outputs, must be tal_arr.
+ * @cltvs: CLTV delays to use as a tie-breaker, or NULL.
+ * @map: if non-NULL, pointers to be permuted the same as the outputs.
  *
  * So the caller initiates the map with which htlcs are used, it
  * can easily see which htlc (if any) is in output #0 with map[0].
  */
-void permute_outputs(struct bitcoin_tx_output *outputs, size_t num_outputs,
+void permute_outputs(struct bitcoin_tx_output *outputs,
+		     u32 *cltvs,
 		     const void **map);
 #endif /* LIGHTNING_COMMON_PERMUTE_TX_H */
diff --git a/common/withdraw_tx.c b/common/withdraw_tx.c
index 7cb0ac433f1a..05432c426e85 100644
--- a/common/withdraw_tx.c
+++ b/common/withdraw_tx.c
@@ -38,9 +38,9 @@ struct bitcoin_tx *withdraw_tx(const tal_t *ctx,
 		map[1] = int2ptr(1);
 		tx->output[1].script = scriptpubkey_p2wpkh(tx, changekey);
 		tx->output[1].amount = changesat;
-		permute_outputs(tx->output, tal_count(tx->output), map);
+		permute_outputs(tx->output, NULL, map);
 	}
-	permute_inputs(tx->input, tal_count(tx->input), (const void **)utxos);
+	permute_inputs(tx->input, (const void **)utxos);
 	return tx;
 }
 
diff --git a/tests/test_pay.py b/tests/test_pay.py
index e690bad9577f..5dca195864ec 100644
--- a/tests/test_pay.py
+++ b/tests/test_pay.py
@@ -1049,7 +1049,6 @@ def test_forward_stats(node_factory, bitcoind):
     assert l3.rpc.getinfo()['msatoshi_fees_collected'] == 0
 
 
-@pytest.mark.xfail(strict=True)
 @unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1 for dev_ignore_htlcs")
 def test_htlcs_cltv_only_difference(node_factory, bitcoind):
     # l1 -> l2 -> l3 -> l4

From b910923762498fc4d1814fa1ffc16ae85553d972 Mon Sep 17 00:00:00 2001
From: Rusty Russell <rusty@rustcorp.com.au>
Date: Mon, 22 Oct 2018 14:52:05 +1030
Subject: [PATCH 5/8] pytest: simple addition to test_onchain_timeout to
 trigger grind failure.

Create a second HTLC with a different CTLV but same preimage; onchaind
uses the wrong signature and fails to grind it.

Reported-by: molz (#c-lightning)
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 tests/test_closing.py | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/tests/test_closing.py b/tests/test_closing.py
index 1744839cfe6c..e8e566c02956 100644
--- a/tests/test_closing.py
+++ b/tests/test_closing.py
@@ -630,17 +630,15 @@ def test_onchain_dust_out(node_factory, bitcoind, executor):
     assert only_one(l2.rpc.listinvoices('onchain_dust_out')['invoices'])['status'] == 'unpaid'
 
 
+@pytest.mark.xfail(strict=True)
 @unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1")
 def test_onchain_timeout(node_factory, bitcoind, executor):
     """Onchain handling of outgoing failed htlcs"""
     # HTLC 1->2, 1 fails just after it's irrevocably committed
-    disconnects = ['+WIRE_REVOKE_AND_ACK', 'permfail']
+    disconnects = ['+WIRE_REVOKE_AND_ACK*3', 'permfail']
     # Feerates identical so we don't get gratuitous commit to update them
     l1 = node_factory.get_node(disconnect=disconnects, feerates=(7500, 7500, 7500))
     l2 = node_factory.get_node()
-    l2 = node_factory.get_node()
-
-    l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
 
     l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
     l1.fund_channel(l2, 10**6)
@@ -654,6 +652,15 @@ def test_onchain_timeout(node_factory, bitcoind, executor):
         'channel': '1:1:1'
     }
 
+    l1.rpc.sendpay([routestep], rhash)
+    with pytest.raises(RpcError):
+        l1.rpc.waitsendpay(rhash)
+
+    # Make sure CLTVs are different, in case it confuses onchaind.
+    bitcoind.generate_block(1)
+    sync_blockheight(bitcoind, [l1])
+
+    # Second one will cause drop to chain.
     l1.rpc.sendpay([routestep], rhash)
     payfuture = executor.submit(l1.rpc.waitsendpay, rhash)
 

From cff9dd293f489b8111e2a13b90d7b45aeb70a7cf Mon Sep 17 00:00:00 2001
From: Rusty Russell <rusty@rustcorp.com.au>
Date: Mon, 22 Oct 2018 20:00:14 +1030
Subject: [PATCH 6/8] pytest: more thorough tests for HTLC confusion.

We set up HTLCs with the same preimage and both different and same
CLTVs in both directions, then make sure that onchaind is OK and that
the HTLCs are failed without causing downstream failure.

We do this for both our-unilateral and their-unilateral cases.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 tests/test_closing.py | 253 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 253 insertions(+)

diff --git a/tests/test_closing.py b/tests/test_closing.py
index e8e566c02956..e2ca6b1c505d 100644
--- a/tests/test_closing.py
+++ b/tests/test_closing.py
@@ -1025,6 +1025,259 @@ def test_permfail_new_commit(node_factory, bitcoind, executor):
     wait_for(lambda: l2.rpc.listpeers()['peers'] == [])
 
 
+def setup_multihtlc_test(node_factory, bitcoind):
+    # l1 -> l2 -> l3 -> l4 -> l5 -> l6 -> l7
+    # l1 and l7 ignore and HTLCs they're sent.
+    # For each direction, we create these HTLCs with same payment_hash:
+    #   1 failed (CLTV1)
+    #   1 failed (CLTV2)
+    #   2 live (CLTV2)
+    #   1 live (CLTV3)
+    nodes = node_factory.line_graph(7, announce=True,
+                                    opts={'dev-no-reconnect': None,
+                                          'may_reconnect': True})
+
+    # Balance by pushing half the funds.
+    b11 = nodes[-1].rpc.invoice(10**9 // 2, '1', 'balancer')['bolt11']
+    nodes[0].rpc.pay(b11)
+
+    nodes[0].rpc.dev_ignore_htlcs(id=nodes[1].info['id'], ignore=True)
+    nodes[-1].rpc.dev_ignore_htlcs(id=nodes[-2].info['id'], ignore=True)
+
+    preimage = "0" * 64
+    h = nodes[0].rpc.invoice(msatoshi=10**8, label='x', description='desc',
+                             preimage=preimage)['payment_hash']
+    nodes[-1].rpc.invoice(msatoshi=10**8, label='x', description='desc',
+                          preimage=preimage)['payment_hash']
+
+    # First, the failed attempts (paying wrong node).  CLTV1
+    r = nodes[0].rpc.getroute(nodes[-2].info['id'], 10**8, 1)["route"]
+    nodes[0].rpc.sendpay(r, h)
+    with pytest.raises(RpcError, match=r'UNKNOWN_PAYMENT_HASH'):
+        nodes[0].rpc.waitsendpay(h)
+
+    r = nodes[-1].rpc.getroute(nodes[1].info['id'], 10**8, 1)["route"]
+    nodes[-1].rpc.sendpay(r, h)
+    with pytest.raises(RpcError, match=r'UNKNOWN_PAYMENT_HASH'):
+        nodes[-1].rpc.waitsendpay(h)
+
+    # Now increment CLTV -> CLTV2
+    bitcoind.generate_block(1)
+    sync_blockheight(bitcoind, nodes)
+
+    # Now, the live attempts with CLTV2 (blackholed by end nodes)
+    r = nodes[0].rpc.getroute(nodes[-1].info['id'], 10**8, 1)["route"]
+    nodes[0].rpc.sendpay(r, h)
+    r = nodes[-1].rpc.getroute(nodes[0].info['id'], 10**8, 1)["route"]
+    nodes[-1].rpc.sendpay(r, h)
+
+    # We send second HTLC from different node, since they refuse to send
+    # multiple with same hash.
+    r = nodes[1].rpc.getroute(nodes[-1].info['id'], 10**8, 1)["route"]
+    nodes[1].rpc.sendpay(r, h)
+    r = nodes[-2].rpc.getroute(nodes[0].info['id'], 10**8, 1)["route"]
+    nodes[-2].rpc.sendpay(r, h)
+
+    # Now increment CLTV -> CLTV3.
+    bitcoind.generate_block(1)
+    sync_blockheight(bitcoind, nodes)
+
+    r = nodes[2].rpc.getroute(nodes[-1].info['id'], 10**8, 1)["route"]
+    nodes[2].rpc.sendpay(r, h)
+    r = nodes[-3].rpc.getroute(nodes[0].info['id'], 10**8, 1)["route"]
+    nodes[-3].rpc.sendpay(r, h)
+
+    # Make sure HTLCs have reached the end.
+    nodes[0].daemon.wait_for_logs(['peer_in WIRE_UPDATE_ADD_HTLC'] * 3)
+    nodes[-1].daemon.wait_for_logs(['peer_in WIRE_UPDATE_ADD_HTLC'] * 3)
+
+    return h, nodes
+
+
+@pytest.mark.xfail(strict=True)
+@unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1 for dev_ignore_htlcs")
+def test_onchain_multihtlc_our_unilateral(node_factory, bitcoind):
+    """Node pushes a channel onchain with multiple HTLCs with same payment_hash """
+    h, nodes = setup_multihtlc_test(node_factory, bitcoind)
+
+    mid = len(nodes) // 2
+
+    for i in range(len(nodes) - 1):
+        assert only_one(nodes[i].rpc.listpeers(nodes[i + 1].info['id'])['peers'])['connected']
+
+    # Now midnode goes onchain with n+1 channel.
+    nodes[mid].rpc.dev_fail(nodes[mid + 1].info['id'])
+    nodes[mid].wait_for_channel_onchain(nodes[mid + 1].info['id'])
+
+    bitcoind.generate_block(1)
+    nodes[mid].daemon.wait_for_log(' to ONCHAIN')
+    nodes[mid + 1].daemon.wait_for_log(' to ONCHAIN')
+
+    # Now, restart and manually reconnect end nodes (so they don't ignore HTLCs)
+    # In fact, they'll fail them with WIRE_TEMPORARY_NODE_FAILURE.
+    nodes[0].restart()
+    nodes[-1].restart()
+
+    # We disabled auto-reconnect so we'd detect breakage, so manually reconnect.
+    nodes[0].rpc.connect(nodes[1].info['id'], 'localhost', nodes[1].port)
+    nodes[-1].rpc.connect(nodes[-2].info['id'], 'localhost', nodes[-2].port)
+
+    # Wait for HTLCs to stabilize.
+    nodes[0].daemon.wait_for_logs(['peer_out WIRE_UPDATE_FAIL_HTLC'] * 3)
+    nodes[0].daemon.wait_for_log('peer_out WIRE_COMMITMENT_SIGNED')
+    nodes[0].daemon.wait_for_log('peer_out WIRE_REVOKE_AND_ACK')
+    nodes[-1].daemon.wait_for_logs(['peer_out WIRE_UPDATE_FAIL_HTLC'] * 3)
+    nodes[-1].daemon.wait_for_log('peer_out WIRE_COMMITMENT_SIGNED')
+    nodes[-1].daemon.wait_for_log('peer_out WIRE_REVOKE_AND_ACK')
+
+    # After at depth 5, midnode will spend its own to-self output.
+    bitcoind.generate_block(4)
+    nodes[mid].wait_for_onchaind_broadcast('OUR_DELAYED_RETURN_TO_WALLET',
+                                           'OUR_UNILATERAL/DELAYED_OUTPUT_TO_US')
+
+    # The three outgoing HTLCs time out at 21, 21 and 22 blocks.
+    bitcoind.generate_block(16)
+    nodes[mid].wait_for_onchaind_broadcast('OUR_HTLC_TIMEOUT_TX',
+                                           'OUR_UNILATERAL/OUR_HTLC')
+    nodes[mid].wait_for_onchaind_broadcast('OUR_HTLC_TIMEOUT_TX',
+                                           'OUR_UNILATERAL/OUR_HTLC')
+    bitcoind.generate_block(1)
+    nodes[mid].wait_for_onchaind_broadcast('OUR_HTLC_TIMEOUT_TX',
+                                           'OUR_UNILATERAL/OUR_HTLC')
+
+    # And three more for us to consider them all settled.
+    bitcoind.generate_block(3)
+
+    # Now, those nodes should have correctly failed the HTLCs
+    for n in nodes[:mid - 1]:
+        with pytest.raises(RpcError, match=r'WIRE_PERMANENT_CHANNEL_FAILURE'):
+            n.rpc.waitsendpay(h, TIMEOUT)
+
+    # Other timeouts are 27,27,28 blocks.
+    bitcoind.generate_block(2)
+    nodes[mid].daemon.wait_for_logs(['Ignoring output.*: OUR_UNILATERAL/THEIR_HTLC'] * 2)
+    for _ in range(2):
+        nodes[mid + 1].wait_for_onchaind_broadcast('OUR_HTLC_TIMEOUT_TO_US',
+                                                   'THEIR_UNILATERAL/OUR_HTLC')
+    bitcoind.generate_block(1)
+    nodes[mid].daemon.wait_for_log('Ignoring output.*: OUR_UNILATERAL/THEIR_HTLC')
+    nodes[mid + 1].wait_for_onchaind_broadcast('OUR_HTLC_TIMEOUT_TO_US',
+                                               'THEIR_UNILATERAL/OUR_HTLC')
+
+    # Depth 3 to consider it settled.
+    bitcoind.generate_block(3)
+
+    for n in nodes[mid + 1:]:
+        with pytest.raises(RpcError, match=r'WIRE_PERMANENT_CHANNEL_FAILURE'):
+            n.rpc.waitsendpay(h, TIMEOUT)
+
+    # At depth 100 it's all done (we didn't bother waiting for mid+1's
+    # spends, so that might still be going)
+    bitcoind.generate_block(96)
+    nodes[mid].daemon.wait_for_logs(['onchaind complete, forgetting peer'])
+
+    # No other channels should have failed.
+    for i in range(len(nodes) - 1):
+        if i != mid:
+            assert only_one(nodes[i].rpc.listpeers(nodes[i + 1].info['id'])['peers'])['connected']
+
+
+@pytest.mark.xfail(strict=True)
+@unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1 for dev_ignore_htlcs")
+def test_onchain_multihtlc_their_unilateral(node_factory, bitcoind):
+    """Node pushes a channel onchain with multiple HTLCs with same payment_hash """
+    h, nodes = setup_multihtlc_test(node_factory, bitcoind)
+
+    mid = len(nodes) // 2
+
+    for i in range(len(nodes) - 1):
+        assert only_one(nodes[i].rpc.listpeers(nodes[i + 1].info['id'])['peers'])['connected']
+
+    # Now midnode+1 goes onchain with midnode channel.
+    nodes[mid + 1].rpc.dev_fail(nodes[mid].info['id'])
+    nodes[mid + 1].wait_for_channel_onchain(nodes[mid].info['id'])
+
+    bitcoind.generate_block(1)
+    nodes[mid].daemon.wait_for_log(' to ONCHAIN')
+    nodes[mid + 1].daemon.wait_for_log(' to ONCHAIN')
+
+    # Now, restart and manually reconnect end nodes (so they don't ignore HTLCs)
+    # In fact, they'll fail them with WIRE_TEMPORARY_NODE_FAILURE.
+    nodes[0].restart()
+    nodes[-1].restart()
+
+    # We disabled auto-reconnect so we'd detect breakage, so manually reconnect.
+    nodes[0].rpc.connect(nodes[1].info['id'], 'localhost', nodes[1].port)
+    nodes[-1].rpc.connect(nodes[-2].info['id'], 'localhost', nodes[-2].port)
+
+    # Wait for HTLCs to stabilize.
+    nodes[0].daemon.wait_for_logs(['peer_out WIRE_UPDATE_FAIL_HTLC'] * 3)
+    nodes[0].daemon.wait_for_log('peer_out WIRE_COMMITMENT_SIGNED')
+    nodes[0].daemon.wait_for_log('peer_out WIRE_REVOKE_AND_ACK')
+    nodes[-1].daemon.wait_for_logs(['peer_out WIRE_UPDATE_FAIL_HTLC'] * 3)
+    nodes[-1].daemon.wait_for_log('peer_out WIRE_COMMITMENT_SIGNED')
+    nodes[-1].daemon.wait_for_log('peer_out WIRE_REVOKE_AND_ACK')
+
+    # At depth 5, midnode+1 will spend its own to-self output.
+    bitcoind.generate_block(4)
+    nodes[mid + 1].wait_for_onchaind_broadcast('OUR_DELAYED_RETURN_TO_WALLET')
+
+    # The three outgoing HTLCs time out at depth 21, 21 and 22 blocks.
+    bitcoind.generate_block(16)
+    nodes[mid].wait_for_onchaind_broadcast('OUR_HTLC_TIMEOUT_TO_US',
+                                           'THEIR_UNILATERAL/OUR_HTLC')
+    nodes[mid].wait_for_onchaind_broadcast('OUR_HTLC_TIMEOUT_TO_US',
+                                           'THEIR_UNILATERAL/OUR_HTLC')
+    bitcoind.generate_block(1)
+    nodes[mid].wait_for_onchaind_broadcast('OUR_HTLC_TIMEOUT_TO_US',
+                                           'THEIR_UNILATERAL/OUR_HTLC')
+
+    # At depth 3 we consider them all settled.
+    bitcoind.generate_block(3)
+
+    # Now, those nodes should have correctly failed the HTLCs
+    for n in nodes[:mid - 1]:
+        with pytest.raises(RpcError, match=r'WIRE_PERMANENT_CHANNEL_FAILURE'):
+            n.rpc.waitsendpay(h, TIMEOUT)
+
+    # Other timeouts are at depths 27,27,28 blocks.
+    bitcoind.generate_block(2)
+    nodes[mid].daemon.wait_for_logs(['Ignoring output.*: THEIR_UNILATERAL/THEIR_HTLC'] * 2)
+    for _ in range(2):
+        nodes[mid + 1].wait_for_onchaind_broadcast('OUR_HTLC_TIMEOUT_TX',
+                                                   'OUR_UNILATERAL/OUR_HTLC')
+    bitcoind.generate_block(1)
+    nodes[mid].daemon.wait_for_log('Ignoring output.*: THEIR_UNILATERAL/THEIR_HTLC')
+    nodes[mid + 1].wait_for_onchaind_broadcast('OUR_HTLC_TIMEOUT_TX',
+                                               'OUR_UNILATERAL/OUR_HTLC')
+
+    # At depth 3 we consider them all settled.
+    bitcoind.generate_block(3)
+
+    for n in nodes[mid + 1:]:
+        with pytest.raises(RpcError, match=r'WIRE_PERMANENT_CHANNEL_FAILURE'):
+            n.rpc.waitsendpay(h, TIMEOUT)
+
+    # At depth 5, mid+1 can spend HTLC_TIMEOUT_TX output.
+    bitcoind.generate_block(1)
+    for _ in range(2):
+        nodes[mid + 1].wait_for_onchaind_broadcast('OUR_DELAYED_RETURN_TO_WALLET',
+                                                   'OUR_HTLC_TIMEOUT_TX/DELAYED_OUTPUT_TO_US')
+    bitcoind.generate_block(1)
+    nodes[mid + 1].wait_for_onchaind_broadcast('OUR_DELAYED_RETURN_TO_WALLET',
+                                               'OUR_HTLC_TIMEOUT_TX/DELAYED_OUTPUT_TO_US')
+
+    # At depth 100 they're all done.
+    bitcoind.generate_block(100)
+    nodes[mid].daemon.wait_for_logs(['onchaind complete, forgetting peer'])
+    nodes[mid + 1].daemon.wait_for_logs(['onchaind complete, forgetting peer'])
+
+    # No other channels should have failed.
+    for i in range(len(nodes) - 1):
+        if i != mid:
+            assert only_one(nodes[i].rpc.listpeers(nodes[i + 1].info['id'])['peers'])['connected']
+
+
 @unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1")
 def test_permfail_htlc_in(node_factory, bitcoind, executor):
     # Test case where we fail with unsettled incoming HTLC.

From 32f56eab4082536601082abea6aae4ffbabbb0e1 Mon Sep 17 00:00:00 2001
From: Rusty Russell <rusty@rustcorp.com.au>
Date: Mon, 22 Oct 2018 20:00:19 +1030
Subject: [PATCH 7/8] onchaind: include htlc id in htlc_stub so we agree on
 what HTLC we're closing.

If there are two HTLCs with the same preimage, lightningd would always
find the first one.  By including the id in the `struct htlc_stub`
it's both faster (normal HTLC lookup) and allows lightningd to detect
that onchaind wants to fail both of them.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 lightningd/onchain_control.c |  8 ++++++--
 lightningd/peer_htlcs.c      | 32 ++++++--------------------------
 lightningd/peer_htlcs.h      |  2 --
 onchaind/onchain_wire.c      |  2 ++
 onchaind/onchain_wire.h      |  1 +
 wallet/wallet.c              |  5 +++--
 6 files changed, 18 insertions(+), 32 deletions(-)

diff --git a/lightningd/onchain_control.c b/lightningd/onchain_control.c
index d8d8ab553c76..13759b4cbfab 100644
--- a/lightningd/onchain_control.c
+++ b/lightningd/onchain_control.c
@@ -343,8 +343,12 @@ static bool tell_if_missing(const struct channel *channel,
 	/* Keep valgrind happy. */
 	*tell_immediate = false;
 
-	/* Is it a current HTLC? */
-	hout = find_htlc_out_by_ripemd(channel, &stub->ripemd);
+	/* Don't care about incoming HTLCs, just ones we offered. */
+	if (stub->owner == REMOTE)
+		return false;
+
+	/* Might not be a current HTLC. */
+	hout = find_htlc_out(&channel->peer->ld->htlcs_out, channel, stub->id);
 	if (!hout)
 		return false;
 
diff --git a/lightningd/peer_htlcs.c b/lightningd/peer_htlcs.c
index 55d8d9e58f22..8cdcea2414be 100644
--- a/lightningd/peer_htlcs.c
+++ b/lightningd/peer_htlcs.c
@@ -823,36 +823,16 @@ static bool peer_failed_our_htlc(struct channel *channel,
 	return true;
 }
 
-/* FIXME: Crazy slow! */
-struct htlc_out *find_htlc_out_by_ripemd(const struct channel *channel,
-					 const struct ripemd160 *ripemd)
-{
-	struct htlc_out_map_iter outi;
-	struct htlc_out *hout;
-	struct lightningd *ld = channel->peer->ld;
-
-	for (hout = htlc_out_map_first(&ld->htlcs_out, &outi);
-	     hout;
-	     hout = htlc_out_map_next(&ld->htlcs_out, &outi)) {
-		struct ripemd160 hash;
-
-		if (hout->key.channel != channel)
-			continue;
-
-		ripemd160(&hash,
-			  &hout->payment_hash, sizeof(hout->payment_hash));
-		if (ripemd160_eq(&hash, ripemd))
-			return hout;
-	}
-	return NULL;
-}
-
 void onchain_failed_our_htlc(const struct channel *channel,
 			     const struct htlc_stub *htlc,
 			     const char *why)
 {
 	struct lightningd *ld = channel->peer->ld;
-	struct htlc_out *hout = find_htlc_out_by_ripemd(channel, &htlc->ripemd);
+	struct htlc_out *hout;
+
+	hout = find_htlc_out(&ld->htlcs_out, channel, htlc->id);
+	if (!hout)
+		return;
 
 	/* Don't fail twice (or if already succeeded)! */
 	if (hout->failuremsg || hout->failcode || hout->preimage)
@@ -863,7 +843,7 @@ void onchain_failed_our_htlc(const struct channel *channel,
 	/* Force state to something which expects a failure, and save to db */
 	hout->hstate = RCVD_REMOVE_HTLC;
 	htlc_out_check(hout, __func__);
-	wallet_htlc_update(channel->peer->ld->wallet, hout->dbid, hout->hstate,
+	wallet_htlc_update(ld->wallet, hout->dbid, hout->hstate,
 			   hout->preimage, hout->failcode, hout->failuremsg);
 
 	if (hout->am_origin) {
diff --git a/lightningd/peer_htlcs.h b/lightningd/peer_htlcs.h
index a36c01b965b1..6f3ad3646e2b 100644
--- a/lightningd/peer_htlcs.h
+++ b/lightningd/peer_htlcs.h
@@ -50,8 +50,6 @@ enum onion_type send_htlc_out(struct channel *out, u64 amount, u32 cltv,
 			      struct htlc_in *in,
 			      struct htlc_out **houtp);
 
-struct htlc_out *find_htlc_out_by_ripemd(const struct channel *channel,
-					 const struct ripemd160 *ripemd160);
 void onchain_failed_our_htlc(const struct channel *channel,
 			     const struct htlc_stub *htlc,
 			     const char *why);
diff --git a/onchaind/onchain_wire.c b/onchaind/onchain_wire.c
index 18b11c2ce3e4..f6de30d7d62f 100644
--- a/onchaind/onchain_wire.c
+++ b/onchaind/onchain_wire.c
@@ -6,6 +6,7 @@ void towire_htlc_stub(u8 **pptr, const struct htlc_stub *htlc_stub)
 {
 	towire_side(pptr, htlc_stub->owner);
 	towire_u32(pptr, htlc_stub->cltv_expiry);
+	towire_u64(pptr, htlc_stub->id);
 	towire_ripemd160(pptr, &htlc_stub->ripemd);
 }
 
@@ -14,5 +15,6 @@ void fromwire_htlc_stub(const u8 **cursor, size_t *max,
 {
 	htlc_stub->owner = fromwire_side(cursor, max);
 	htlc_stub->cltv_expiry = fromwire_u32(cursor, max);
+	htlc_stub->id = fromwire_u64(cursor, max);
 	fromwire_ripemd160(cursor, max, &htlc_stub->ripemd);
 }
diff --git a/onchaind/onchain_wire.h b/onchaind/onchain_wire.h
index b23a03e4bd1d..aeb0f50d7120 100644
--- a/onchaind/onchain_wire.h
+++ b/onchaind/onchain_wire.h
@@ -9,6 +9,7 @@
 struct htlc_stub {
 	enum side owner;
 	u32 cltv_expiry;
+	u64 id;
 	struct ripemd160 ripemd;
 };
 
diff --git a/wallet/wallet.c b/wallet/wallet.c
index 5245da69a2e8..0048b47660c8 100644
--- a/wallet/wallet.c
+++ b/wallet/wallet.c
@@ -1551,7 +1551,7 @@ struct htlc_stub *wallet_htlc_stubs(const tal_t *ctx, struct wallet *wallet,
 	struct htlc_stub *stubs;
 	struct sha256 payment_hash;
 	sqlite3_stmt *stmt = db_prepare(wallet->db,
-		"SELECT channel_id, direction, cltv_expiry, payment_hash "
+		"SELECT channel_id, direction, cltv_expiry, channel_htlc_id, payment_hash "
 		"FROM channel_htlcs WHERE channel_id = ?;");
 
 	sqlite3_bind_int64(stmt, 1, chan->dbid);
@@ -1566,8 +1566,9 @@ struct htlc_stub *wallet_htlc_stubs(const tal_t *ctx, struct wallet *wallet,
 		/* FIXME: merge these two enums */
 		stub->owner = sqlite3_column_int(stmt, 1)==DIRECTION_INCOMING?REMOTE:LOCAL;
 		stub->cltv_expiry = sqlite3_column_int(stmt, 2);
+		stub->id = sqlite3_column_int(stmt, 3);
 
-		sqlite3_column_sha256(stmt, 3, &payment_hash);
+		sqlite3_column_sha256(stmt, 4, &payment_hash);
 		ripemd160(&stub->ripemd, payment_hash.u.u8, sizeof(payment_hash.u));
 	}
 	db_stmt_done(stmt);

From 4c480e93ad0110ea174a782b610356b05d237880 Mon Sep 17 00:00:00 2001
From: Rusty Russell <rusty@rustcorp.com.au>
Date: Mon, 22 Oct 2018 20:00:19 +1030
Subject: [PATCH 8/8] onchaind: allow multiple candidate HTLCs for output
 match.

When we have multiple HTLCs with the same preimage and the same CLTV,
it doesn't matter what order we treat them (they're literally
identical).  But when we offer HTLCs with the same preimage but
different CLTVs, the commitment tx outputs look identical, but the
HTLC txs are different: if we simply take the first HTLC which matches
(and that's not the right one), the HTLC signature we got from them
won't match.  As we rely on the signature matching to detect the fee
paid, we get:

	onchaind: STATUS_FAIL_INTERNAL_ERROR: grind_fee failed

So we alter match_htlc_output() to return an array of all matching
HTLC indices, which can have more than one entry for offered HTLCs.
If it's our commitment, we loop through until one of the HTLC
signatures matches.  If it's their commitment, we choose the HTLC with
the largest CLTV: we're going to ignore it once that hits anyway, so
this is the most conservative approach.  If it's a penalty, it doesn't
matter since we steal all HTLC outputs the same independent of CLTV.

For accepted HTLCs, the CLTV value is encoded in the witness script,
so this confusion isn't possible.  We nonetheless assert that the
CLTVs all match in that case.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 CHANGELOG.md          |   1 +
 onchaind/onchaind.c   | 269 ++++++++++++++++++++++++++++++------------
 tests/test_closing.py |   3 -
 3 files changed, 193 insertions(+), 80 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 8a871b20e31b..0dbc30980187 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -52,6 +52,7 @@ changes.
 - Protocol: fix occasional deadlock when both peers flood with gossip.
 - Protocol: fix occasional long delay on sending `reply_short_channel_ids_end`.
 - Protocol: re-send `node_announcement` when address/alias/color etc change.
+- Protocol: multiple HTLCs with the same payment_hash are handled correctly.
 - Options: 'autotor' defaults to port 9051 if not specified.
 
 ### Security
diff --git a/onchaind/onchaind.c b/onchaind/onchaind.c
index 835481f14589..26dc39a41438 100644
--- a/onchaind/onchaind.c
+++ b/onchaind/onchaind.c
@@ -144,18 +144,10 @@ static u64 grind_htlc_tx_fee(struct bitcoin_tx *tx,
 
 		return fee;
 	}
-	status_failed(STATUS_FAIL_INTERNAL_ERROR,
-		      "grind_fee failed from %u - %u"
-		      " for tx %s, inputamount %"PRIu64", signature %s, wscript %s, multiplier %"PRIu64,
-		      min_possible_feerate, max_possible_feerate,
-		      type_to_string(tmpctx, struct bitcoin_tx, tx),
-		      input_amount,
-		      type_to_string(tmpctx, secp256k1_ecdsa_signature, remotesig),
-		      tal_hex(tmpctx, wscript),
-		      multiplier);
+	return UINT64_MAX;
 }
 
-static void set_htlc_timeout_fee(struct bitcoin_tx *tx,
+static bool set_htlc_timeout_fee(struct bitcoin_tx *tx,
 				 const secp256k1_ecdsa_signature *remotesig,
 				 const u8 *wscript)
 {
@@ -170,21 +162,12 @@ static void set_htlc_timeout_fee(struct bitcoin_tx *tx,
 	 */
 	if (fee == UINT64_MAX) {
 		fee = grind_htlc_tx_fee(tx, remotesig, wscript, 663);
-		return;
+		return fee != UINT64_MAX;
 	}
 
 	tx->output[0].amount = *tx->input[0].amount - fee;
-	if (check_tx_sig(tx, 0, NULL, wscript,
-			 &keyset->other_htlc_key, remotesig))
-		return;
-
-	status_failed(STATUS_FAIL_INTERNAL_ERROR,
-		      "htlc_timeout_fee %"PRIu64" failed sigcheck "
-		      " for tx %s, signature %s, wscript %s",
-		      fee,
-		      type_to_string(tmpctx, struct bitcoin_tx, tx),
-		      type_to_string(tmpctx, secp256k1_ecdsa_signature, remotesig),
-		      tal_hex(tmpctx, wscript));
+	return check_tx_sig(tx, 0, NULL, wscript,
+			    &keyset->other_htlc_key, remotesig);
 }
 
 static void set_htlc_success_fee(struct bitcoin_tx *tx,
@@ -1305,42 +1288,105 @@ static u8 **derive_htlc_scripts(const struct htlc_stub *htlcs, enum side side)
 	return htlc_scripts;
 }
 
-static void resolve_our_htlc_ourcommit(struct tracked_output *out)
+static size_t resolve_our_htlc_ourcommit(struct tracked_output *out,
+					 const size_t *matches,
+					 const struct htlc_stub *htlcs,
+					 u8 **htlc_scripts)
 {
 	struct bitcoin_tx *tx;
 	secp256k1_ecdsa_signature localsig;
+	size_t i;
 
-	/* BOLT #5:
-	 *
-	 * ## HTLC Output Handling: Local Commitment, Local Offers
-	 * ...
-	 *  - if the commitment transaction HTLC output has *timed out* and
-	 *  hasn't been *resolved*:
-	 *    - MUST *resolve* the output by spending it using the HTLC-timeout
-	 *    transaction.
-	 */
-	tx = htlc_timeout_tx(out, &out->txid, out->outnum, out->satoshi * 1000,
-			     out->htlc->cltv_expiry,
-			     to_self_delay[LOCAL], 0, keyset);
+	assert(tal_count(matches));
 
-	set_htlc_timeout_fee(tx, out->remote_htlc_sig, out->wscript);
+	/* These htlcs are all possibilities, but signature will only match
+	 * one with the correct cltv: check which that is. */
+	for (i = 0; i < tal_count(matches); i++) {
+		/* BOLT #5:
+		 *
+		 * ## HTLC Output Handling: Local Commitment, Local Offers
+		 * ...
+		 *  - if the commitment transaction HTLC output has *timed out*
+		 *  and hasn't been *resolved*:
+		 *    - MUST *resolve* the output by spending it using the
+		 *    HTLC-timeout transaction.
+		 */
+		tx = htlc_timeout_tx(tmpctx, &out->txid, out->outnum,
+				     out->satoshi * 1000,
+				     htlcs[matches[i]].cltv_expiry,
+				     to_self_delay[LOCAL], 0, keyset);
 
-	hsm_sign_local_htlc_tx(tx, out->wscript, &localsig);
+		if (set_htlc_timeout_fee(tx, out->remote_htlc_sig,
+					 htlc_scripts[matches[i]]))
+			break;
+	}
+
+	/* Since there's been trouble with this before, we go to some length
+	 * to give details here! */
+	if (i == tal_count(matches)) {
+		char *cltvs, *wscripts;
+
+		cltvs = tal_fmt(tmpctx, "%u", htlcs[matches[0]].cltv_expiry);
+		wscripts = tal_hex(tmpctx, htlc_scripts[matches[0]]);
+
+		for (i = 1; i < tal_count(matches); i++) {
+			tal_append_fmt(&cltvs, "/%u",
+				       htlcs[matches[i]].cltv_expiry);
+			tal_append_fmt(&wscripts, "/%s",
+				       tal_hex(tmpctx, htlc_scripts[matches[i]]));
+		}
+
+		status_failed(STATUS_FAIL_INTERNAL_ERROR,
+			      "No valid signature found for %zu htlc_timeout_txs"
+			      " feerate %u-%u,"
+			      " last tx %s, inputamount %"PRIu64", signature %s,"
+			      " cltvs %s wscripts %s",
+			      tal_count(matches),
+			      min_possible_feerate, max_possible_feerate,
+			      type_to_string(tmpctx, struct bitcoin_tx, tx),
+			      out->satoshi,
+			      type_to_string(tmpctx, secp256k1_ecdsa_signature,
+					     out->remote_htlc_sig),
+			      cltvs, wscripts);
+	}
+
+	hsm_sign_local_htlc_tx(tx, htlc_scripts[matches[i]], &localsig);
 
 	tx->input[0].witness
 		= bitcoin_witness_htlc_timeout_tx(tx->input,
 						  &localsig,
 						  out->remote_htlc_sig,
-						  out->wscript);
+						  htlc_scripts[matches[i]]);
 
-	propose_resolution_at_block(out, tx, out->htlc->cltv_expiry,
+	/* Steals tx onto out */
+	propose_resolution_at_block(out, tx, htlcs[matches[i]].cltv_expiry,
 				    OUR_HTLC_TIMEOUT_TX);
+
+	return matches[i];
+}
+
+/* wscript for *received* htlcs (ie. our htlcs in their commit tx, or their
+ * htlcs in our commit tx) includes cltv, so they must be the same for all
+ * matching htlcs.  Unless, of course, they've found a sha256 clash. */
+static u32 matches_cltv(const size_t *matches,
+			const struct htlc_stub *htlcs)
+{
+	for (size_t i = 1; i < tal_count(matches); i++) {
+		assert(matches[i] < tal_count(htlcs));
+		assert(htlcs[matches[i]].cltv_expiry
+		       == htlcs[matches[i-1]].cltv_expiry);
+	}
+	return htlcs[matches[0]].cltv_expiry;
 }
 
-static void resolve_our_htlc_theircommit(struct tracked_output *out)
+static size_t resolve_our_htlc_theircommit(struct tracked_output *out,
+					   const size_t *matches,
+					   const struct htlc_stub *htlcs,
+					   u8 **htlc_scripts)
 {
 	struct bitcoin_tx *tx;
 	enum tx_type tx_type = OUR_HTLC_TIMEOUT_TO_US;
+	u32 cltv_expiry = matches_cltv(matches, htlcs);
 
 	/* BOLT #5:
 	 *
@@ -1353,15 +1399,24 @@ static void resolve_our_htlc_theircommit(struct tracked_output *out)
 	 *       address.
 	 */
 	tx = tx_to_us(out, remote_htlc_to_us,
-		      out, 0, out->htlc->cltv_expiry, NULL, 0,
-		      out->wscript,
+		      out, 0, cltv_expiry, NULL, 0,
+		      htlc_scripts[matches[0]],
 		      &tx_type);
 
-	propose_resolution_at_block(out, tx, out->htlc->cltv_expiry, tx_type);
+	propose_resolution_at_block(out, tx, cltv_expiry, tx_type);
+
+	/* They're all equivalent: might as well use first one. */
+	return matches[0];
 }
 
-static void resolve_their_htlc(struct tracked_output *out)
+/* Returns which htlcs it chose to use of matches[] */
+static size_t resolve_their_htlc(struct tracked_output *out,
+				 const size_t *matches,
+				 const struct htlc_stub *htlcs,
+				 u8 **htlc_scripts)
 {
+	size_t which_htlc;
+
 	/* BOLT #5:
 	 *
 	 * ## HTLC Output Handling: Remote Commitment, Remote Offers
@@ -1371,18 +1426,45 @@ static void resolve_their_htlc(struct tracked_output *out)
 	 * If not otherwise resolved, once the HTLC output has expired, it is
 	 * considered *irrevocably resolved*.
 	 */
+
+	/* BOLT #5:
+	 *
+	 * ## HTLC Output Handling: Local Commitment, Remote Offers
+	 *...
+	 * ### Requirements
+	 *...
+	 * If not otherwise resolved, once the HTLC output has expired, it is
+	 * considered *irrevocably resolved*.
+	 */
+
+	/* The two cases are identical as far as default handling goes.
+	 * But in the remote commitment / remote offer (ie. caller is
+	 * handle_their_unilateral), htlcs which match may have different cltvs.
+	 * So wait until the worst case (largest HTLC). */
+	assert(tal_count(matches));
+	which_htlc = matches[0];
+	for (size_t i = 1; i < tal_count(matches); i++) {
+		if (htlcs[matches[i]].cltv_expiry > htlcs[which_htlc].cltv_expiry)
+			which_htlc = matches[i];
+	}
+
 	/* If we hit timeout depth, resolve by ignoring. */
-	propose_resolution_at_block(out, NULL, out->htlc->cltv_expiry,
+	propose_resolution_at_block(out, NULL, htlcs[which_htlc].cltv_expiry,
 				    THEIR_HTLC_TIMEOUT_TO_THEM);
+	return which_htlc;
 }
 
-static int match_htlc_output(const struct bitcoin_tx *tx,
-			     unsigned int outnum,
-			     u8 **htlc_scripts)
+/* Return tal_arr of htlc indexes. */
+static const size_t *match_htlc_output(const tal_t *ctx,
+				 const struct bitcoin_tx *tx,
+				 unsigned int outnum,
+				 u8 **htlc_scripts)
 {
+	size_t *matches = tal_arr(ctx, size_t, 0);
+
 	/* Must be a p2wsh output */
 	if (!is_p2wsh(tx->output[outnum].script, NULL))
-		return -1;
+		return matches;
 
 	for (size_t i = 0; i < tal_count(htlc_scripts); i++) {
 		struct sha256 sha;
@@ -1393,9 +1475,21 @@ static int match_htlc_output(const struct bitcoin_tx *tx,
 		if (memeq(tx->output[outnum].script + 2,
 			  tal_count(tx->output[outnum].script) - 2,
 			  &sha, sizeof(sha)))
-			return i;
+			*tal_arr_expand(&matches) = i;
 	}
-	return -1;
+	return matches;
+}
+
+/* They must all be in the same direction, since the scripts are different for
+ * each dir.  Unless, of course, they've found a sha256 clash. */
+static enum side matches_direction(const size_t *matches,
+				   const struct htlc_stub *htlcs)
+{
+	for (size_t i = 1; i < tal_count(matches); i++) {
+		assert(matches[i] < tal_count(htlcs));
+		assert(htlcs[matches[i]].owner == htlcs[matches[i-1]].owner);
+	}
+	return htlcs[matches[0]].owner;
 }
 
 /* Tell master about any we didn't use, if it wants to know. */
@@ -1508,7 +1602,8 @@ static void handle_our_unilateral(const struct bitcoin_tx *tx,
 
 	for (i = 0; i < tal_count(tx->output); i++) {
 		struct tracked_output *out;
-		int j;
+		const size_t *matches;
+		size_t which_htlc;
 
 		if (script[LOCAL]
 		    && scripteq(tx->output[i].script, script[LOCAL])) {
@@ -1577,14 +1672,14 @@ static void handle_our_unilateral(const struct bitcoin_tx *tx,
 			continue;
 		}
 
+		matches = match_htlc_output(tmpctx, tx, i, htlc_scripts);
 		/* FIXME: limp along when this happens! */
-		j = match_htlc_output(tx, i, htlc_scripts);
-		if (j == -1)
+		if (tal_count(matches) == 0)
 			status_failed(STATUS_FAIL_INTERNAL_ERROR,
 				      "Could not find resolution for output %zu",
 				      i);
 
-		if (htlcs[j].owner == LOCAL) {
+		if (matches_direction(matches, htlcs) == LOCAL) {
 			/* BOLT #5:
 			 *
 			 *     - MUST handle HTLCs offered by itself as specified
@@ -1596,17 +1691,19 @@ static void handle_our_unilateral(const struct bitcoin_tx *tx,
 						 OUR_UNILATERAL, i,
 						 tx->output[i].amount,
 						 OUR_HTLC,
-						 &htlcs[j], htlc_scripts[j],
+						 NULL, NULL,
 						 remote_htlc_sigs);
-			resolve_our_htlc_ourcommit(out);
+			/* Tells us which htlc to use */
+			which_htlc = resolve_our_htlc_ourcommit(out, matches,
+								htlcs,
+								htlc_scripts);
 		} else {
 			out = new_tracked_output(&outs, txid,
 						 tx_blockheight,
 						 OUR_UNILATERAL, i,
 						 tx->output[i].amount,
 						 THEIR_HTLC,
-						 &htlcs[j],
-						 htlc_scripts[j],
+						 NULL, NULL,
 						 remote_htlc_sigs);
 			/* BOLT #5:
 			 *
@@ -1614,13 +1711,17 @@ static void handle_our_unilateral(const struct bitcoin_tx *tx,
 			 *     as specified in [HTLC Output Handling: Local
 			 *     Commitment, Remote Offers]
 			 */
-			resolve_their_htlc(out);
+			/* Tells us which htlc to use */
+			which_htlc = resolve_their_htlc(out, matches, htlcs,
+							htlc_scripts);
 		}
+		out->htlc = &htlcs[which_htlc];
+		out->wscript = htlc_scripts[which_htlc];
 
 		/* Each of these consumes one HTLC signature */
 		remote_htlc_sigs++;
 		/* We've matched this HTLC, can't do again. */
-		htlc_scripts[j] = NULL;
+		htlc_scripts[which_htlc] = NULL;
 
 	}
 
@@ -1809,7 +1910,8 @@ static void handle_their_cheat(const struct bitcoin_tx *tx,
 
 	for (i = 0; i < tal_count(tx->output); i++) {
 		struct tracked_output *out;
-		int j;
+		const size_t *matches;
+		size_t which_htlc;
 
 		if (script[LOCAL]
 		    && scripteq(tx->output[i].script, script[LOCAL])) {
@@ -1854,13 +1956,16 @@ static void handle_their_cheat(const struct bitcoin_tx *tx,
 			continue;
 		}
 
-		j = match_htlc_output(tx, i, htlc_scripts);
-		if (j == -1)
+		matches = match_htlc_output(tmpctx, tx, i, htlc_scripts);
+		if (tal_count(matches) == 0)
 			status_failed(STATUS_FAIL_INTERNAL_ERROR,
 				      "Could not find resolution for output %zu",
 				      i);
 
-		if (htlcs[j].owner == LOCAL) {
+		/* In this case, we don't care which HTLC we choose; so pick
+		   first one */
+		which_htlc = matches[0];
+		if (matches_direction(matches, htlcs) == LOCAL) {
 			/* BOLT #5:
 			 *
 			 *  - MUST *resolve* the _local node's offered HTLCs_
@@ -1877,7 +1982,8 @@ static void handle_their_cheat(const struct bitcoin_tx *tx,
 						 THEIR_REVOKED_UNILATERAL, i,
 						 tx->output[i].amount,
 						 OUR_HTLC,
-						 &htlcs[j], htlc_scripts[j],
+						 &htlcs[which_htlc],
+						 htlc_scripts[which_htlc],
 						 NULL);
 			steal_htlc(out);
 		} else {
@@ -1886,7 +1992,8 @@ static void handle_their_cheat(const struct bitcoin_tx *tx,
 						 THEIR_REVOKED_UNILATERAL, i,
 						 tx->output[i].amount,
 						 THEIR_HTLC,
-						 &htlcs[j], htlc_scripts[j],
+						 &htlcs[which_htlc],
+						 htlc_scripts[which_htlc],
 						 NULL);
 			/* BOLT #5:
 			 *
@@ -1899,7 +2006,7 @@ static void handle_their_cheat(const struct bitcoin_tx *tx,
 			 */
 			steal_htlc(out);
 		}
-		htlc_scripts[j] = NULL;
+		htlc_scripts[which_htlc] = NULL;
 	}
 
 	note_missing_htlcs(htlc_scripts, htlcs,
@@ -2021,7 +2128,8 @@ static void handle_their_unilateral(const struct bitcoin_tx *tx,
 
 	for (i = 0; i < tal_count(tx->output); i++) {
 		struct tracked_output *out;
-		int j;
+		const size_t *matches;
+		size_t which_htlc;
 
 		if (script[LOCAL]
 		    && scripteq(tx->output[i].script, script[LOCAL])) {
@@ -2068,12 +2176,13 @@ static void handle_their_unilateral(const struct bitcoin_tx *tx,
 			continue;
 		}
 
-		j = match_htlc_output(tx, i, htlc_scripts);
-		if (j == -1)
+		matches = match_htlc_output(tmpctx, tx, i, htlc_scripts);
+		if (tal_count(matches) == 0)
 			status_failed(STATUS_FAIL_INTERNAL_ERROR,
 				      "Could not find resolution for output %zu",
 				      i);
-		if (htlcs[j].owner == LOCAL) {
+
+		if (matches_direction(matches, htlcs) == LOCAL) {
 			/* BOLT #5:
 			 *
 			 * - MUST handle HTLCs offered by itself as specified in
@@ -2085,16 +2194,19 @@ static void handle_their_unilateral(const struct bitcoin_tx *tx,
 						 THEIR_UNILATERAL, i,
 						 tx->output[i].amount,
 						 OUR_HTLC,
-						 &htlcs[j], htlc_scripts[j],
+						 NULL, NULL,
 						 NULL);
-			resolve_our_htlc_theircommit(out);
+			which_htlc = resolve_our_htlc_theircommit(out,
+								  matches,
+								  htlcs,
+								  htlc_scripts);
 		} else {
 			out = new_tracked_output(&outs, txid,
 						 tx_blockheight,
 						 THEIR_UNILATERAL, i,
 						 tx->output[i].amount,
 						 THEIR_HTLC,
-						 &htlcs[j], htlc_scripts[j],
+						 NULL, NULL,
 						 NULL);
 			/* BOLT #5:
 			 *
@@ -2102,9 +2214,12 @@ static void handle_their_unilateral(const struct bitcoin_tx *tx,
 			 *   specified in [HTLC Output Handling: Remote
 			 *   Commitment, Remote Offers]
 			 */
-			resolve_their_htlc(out);
+			which_htlc = resolve_their_htlc(out, matches, htlcs,
+							htlc_scripts);
 		}
-		htlc_scripts[j] = NULL;
+		out->htlc = &htlcs[which_htlc];
+		out->wscript = htlc_scripts[which_htlc];
+		htlc_scripts[which_htlc] = NULL;
 	}
 
 	note_missing_htlcs(htlc_scripts, htlcs,
diff --git a/tests/test_closing.py b/tests/test_closing.py
index e2ca6b1c505d..37432aa861d4 100644
--- a/tests/test_closing.py
+++ b/tests/test_closing.py
@@ -630,7 +630,6 @@ def test_onchain_dust_out(node_factory, bitcoind, executor):
     assert only_one(l2.rpc.listinvoices('onchain_dust_out')['invoices'])['status'] == 'unpaid'
 
 
-@pytest.mark.xfail(strict=True)
 @unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1")
 def test_onchain_timeout(node_factory, bitcoind, executor):
     """Onchain handling of outgoing failed htlcs"""
@@ -1094,7 +1093,6 @@ def setup_multihtlc_test(node_factory, bitcoind):
     return h, nodes
 
 
-@pytest.mark.xfail(strict=True)
 @unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1 for dev_ignore_htlcs")
 def test_onchain_multihtlc_our_unilateral(node_factory, bitcoind):
     """Node pushes a channel onchain with multiple HTLCs with same payment_hash """
@@ -1182,7 +1180,6 @@ def test_onchain_multihtlc_our_unilateral(node_factory, bitcoind):
             assert only_one(nodes[i].rpc.listpeers(nodes[i + 1].info['id'])['peers'])['connected']
 
 
-@pytest.mark.xfail(strict=True)
 @unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1 for dev_ignore_htlcs")
 def test_onchain_multihtlc_their_unilateral(node_factory, bitcoind):
     """Node pushes a channel onchain with multiple HTLCs with same payment_hash """