Fix handling of CLTVs: in three ways!

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

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);
 
@@ -158,9 +159,11 @@ 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));
+
+	/* 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. */
@@ -177,8 +180,9 @@ 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];
+		cltvs[n] = abs_locktime_to_blocks(&htlcs[i]->expiry);
+		n++;
 	}
 
 	/* BOLT #3:
@@ -192,8 +196,9 @@ 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];
+		cltvs[n] = abs_locktime_to_blocks(&htlcs[i]->expiry);
+		n++;
 	}
 
 	/* BOLT #3:
@@ -206,8 +211,9 @@ 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;
+		/* 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));
@@ -231,8 +237,9 @@ 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;
+		/* 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,
@@ -242,16 +249,14 @@ 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:
 	 *
 	 * 7. Sort the outputs into [BIP 69
 	 *    order](#transaction-input-and-output-ordering)
 	 */
-	permute_outputs(tx->output, tal_count(tx->output),
-			htlcmap ? (const void **)*htlcmap : NULL);
+	permute_outputs(tx->output, cltvs, (const void **)*htlcmap);
 
 	/* BOLT #3:
 	 *

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

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

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

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;
 

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,

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) {

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