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

@@ -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:
 	 *

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;
 }

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;
 }

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:
 	 *

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));
 	}
 }

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 */

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;
 }
 

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