onchaind: fail htlcs which are not included in commitment tx.

As per update 149cf020d6

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

lightningd/peer_control.c

@@ -1261,6 +1261,27 @@ static int handle_extracted_preimage(struct peer *peer, const u8 *msg)
 	return 0;
 }
 
+static int handle_missing_htlc_output(struct peer *peer, const u8 *msg)
+{
+	struct htlc_stub htlc;
+
+	if (!fromwire_onchain_missing_htlc_output(msg, NULL, &htlc)) {
+		log_broken(peer->log, "Invalid missing_htlc_output");
+		return -1;
+	}
+
+	/* BOLT #5:
+	 *
+	 * For any committed HTLC which does not have an output in this
+	 * commitment transaction, the node MUST fail the corresponding
+	 * incoming HTLC (if any) once the commitment transaction has reached
+	 * reasonable depth, and MAY fail it sooner if no valid commitment
+	 * transaction contains an output corresponding to the HTLC.
+	 */
+	onchain_failed_our_htlc(peer, &htlc, "missing in commitment tx");
+	return 0;
+}
+
 static int onchain_msg(struct subd *sd, const u8 *msg, const int *fds)
 {
 	enum onchain_wire_type t = fromwire_peektype(msg);
@@ -1278,6 +1299,9 @@ static int onchain_msg(struct subd *sd, const u8 *msg, const int *fds)
  	case WIRE_ONCHAIN_EXTRACTED_PREIMAGE:
 		return handle_extracted_preimage(sd->peer, msg);
 
+	case WIRE_ONCHAIN_MISSING_HTLC_OUTPUT:
+		return handle_missing_htlc_output(sd->peer, msg);
+
 	/* We send these, not receive them */
 	case WIRE_ONCHAIN_INIT:
 	case WIRE_ONCHAIN_SPENT:
@@ -1300,6 +1324,37 @@ static u8 *p2wpkh_for_keyidx(const tal_t *ctx, struct lightningd *ld, u64 keyidx
 	return scriptpubkey_p2wpkh(ctx, &shutdownkey);
 }
 
+/* If we want to know if this HTLC is missing, return depth. */
+static bool tell_if_missing(const struct peer *peer, size_t n,
+			    bool *tell_immediate)
+{
+	struct htlc_out *hout;
+
+	/* Keep valgrind happy. */
+	*tell_immediate = false;
+
+	/* Is it a current HTLC? */
+	hout = find_htlc_out_by_ripemd(peer, &peer->htlcs[n].ripemd);
+	if (!hout)
+		return false;
+
+	/* BOLT #5:
+	 *
+	 * For any committed HTLC which does not have an output in this
+	 * commitment transaction, the node MUST fail the corresponding
+	 * incoming HTLC (if any) once the commitment transaction has reached
+	 * reasonable depth, and MAY fail it sooner if no valid commitment
+	 * transaction contains an output corresponding to the HTLC.
+	 */
+	if (hout->hstate >= RCVD_ADD_REVOCATION
+	    && hout->hstate < SENT_REMOVE_REVOCATION)
+		*tell_immediate = true;
+
+	log_debug(peer->log, "We want to know if htlc %"PRIu64" is missing (%s)",
+		  hout->key.id, *tell_immediate ? "immediate" : "later");
+	return true;
+}
+
 /* With a reorg, this can get called multiple times; each time we'll kill
  * onchaind (like any other owner), and restart */
 static enum watch_result funding_spent(struct peer *peer,
@@ -1374,17 +1429,21 @@ static enum watch_result funding_spent(struct peer *peer,
 				  &peer->channel_info->theirbase.delayed_payment,
 				  tx,
 				  block->height,
+				  /* FIXME: config for 'reasonable depth' */
+				  3,
 				  peer->last_htlc_sigs,
 				  tal_count(peer->htlcs));
 	subd_send_msg(peer->owner, take(msg));
 
 	/* FIXME: Don't queue all at once, use an empty cb... */
 	for (size_t i = 0; i < tal_count(peer->htlcs); i++) {
-		msg = towire_onchain_htlc(peer, peer->htlcs+i);
+		bool tell_immediate;
+		bool tell = tell_if_missing(peer, i, &tell_immediate);
+		msg = towire_onchain_htlc(peer, peer->htlcs+i,
+					  tell, tell_immediate);
 		subd_send_msg(peer->owner, take(msg));
 	}
 
-	/* FIXME: Send any known HTLC preimage for live HTLCs! */
 	watch_tx_and_outputs(peer, tx);
 
 	/* We keep watching until peer finally deleted, for reorgs. */

lightningd/peer_htlcs.c

@@ -791,6 +791,53 @@ static bool peer_failed_our_htlc(struct peer *peer,
 	return true;
 }
 
+/* FIXME: Crazy slow! */
+struct htlc_out *find_htlc_out_by_ripemd(const struct peer *peer,
+					 const struct ripemd160 *ripemd)
+{
+	struct htlc_out_map_iter outi;
+	struct htlc_out *hout;
+
+	for (hout = htlc_out_map_first(&peer->ld->htlcs_out, &outi);
+	     hout;
+	     hout = htlc_out_map_next(&peer->ld->htlcs_out, &outi)) {
+		struct ripemd160 hash;
+
+		if (hout->key.peer != peer)
+			continue;
+
+		ripemd160(&hash,
+			  &hout->payment_hash, sizeof(hout->payment_hash));
+		if (structeq(&hash, ripemd))
+			return hout;
+	}
+	return NULL;
+}
+
+void onchain_failed_our_htlc(const struct peer *peer,
+			     const struct htlc_stub *htlc,
+			     const char *why)
+{
+	struct htlc_out *hout = find_htlc_out_by_ripemd(peer, &htlc->ripemd);
+
+	/* Don't fail twice! */
+	if (hout->failuremsg)
+		return;
+
+	hout->failuremsg = make_failmsg(hout, hout->msatoshi,
+					WIRE_PERMANENT_CHANNEL_FAILURE,
+					NULL);
+
+	if (!hout->in) {
+		char *localfail = tal_fmt(peer, "%s: %s",
+					  onion_type_name(WIRE_PERMANENT_CHANNEL_FAILURE),
+					  why);
+		payment_failed(hout->key.peer->ld, hout, localfail);
+		tal_free(localfail);
+	} else
+		local_fail_htlc(hout->in, WIRE_PERMANENT_CHANNEL_FAILURE);
+}
+
 static void remove_htlc_in(struct peer *peer, struct htlc_in *hin)
 {
 	htlc_in_check(hin, __func__);

lightningd/peer_htlcs.h

@@ -41,5 +41,10 @@ enum onion_type send_htlc_out(struct peer *out, u64 amount, u32 cltv,
 			      struct pay_command *pc,
 			      struct htlc_out **houtp);
 
+struct htlc_out *find_htlc_out_by_ripemd(const struct peer *peer,
+					 const struct ripemd160 *ripemd160);
+void onchain_failed_our_htlc(const struct peer *peer,
+			     const struct htlc_stub *htlc,
+			     const char *why);
 void onchain_fulfilled_htlc(struct peer *peer, const struct preimage *preimage);
 #endif /* LIGHTNING_LIGHTNINGD_PEER_HTLCS_H */

onchaind/onchain.c

@@ -51,6 +51,12 @@ static struct privkey *revocation_privkey;
 /* one value is useful for a few witness scripts */
 static const u8 ONE = 0x1;
 
+/* When to tell master about HTLCs which aren't in commitment tx */
+static u32 htlc_missing_depth;
+
+/* The messages to send at that depth. */
+static u8 **missing_htlc_msgs;
+
 /* If we broadcast a tx, or need a delay to resolve the output. */
 struct proposed_resolution {
 	/* This can be NULL if our proposal is to simply ignore it after depth */
@@ -685,6 +691,7 @@ static void output_spent(struct tracked_output ***outs,
 			    || out->resolved->tx_type == OUR_HTLC_TIMEOUT_TX)
 				resolve_htlc_tx(outs, i, tx, &txid,
 						tx_blockheight);
+			/* FIXME: Fail timed out htlc after reasonable depth. */
 			return;
 		}
 
@@ -747,6 +754,18 @@ static void tx_new_depth(struct tracked_output **outs,
 {
 	size_t i;
 
+	/* Special handling for commitment tx reaching depth */
+	if (structeq(&outs[0]->resolved->txid, txid)
+	    && depth >= htlc_missing_depth
+	    && missing_htlc_msgs) {
+		status_trace("Sending %zu missing htlc messages",
+			     tal_count(missing_htlc_msgs));
+		for (i = 0; i < tal_count(missing_htlc_msgs); i++)
+			wire_sync_write(REQ_FD, missing_htlc_msgs[i]);
+		/* Don't do it again. */
+		missing_htlc_msgs = tal_free(missing_htlc_msgs);
+	}
+
 	for (i = 0; i < tal_count(outs); i++) {
 		/* Is this tx resolving an output? */
 		if (outs[i]->resolved) {
@@ -1049,6 +1068,35 @@ static int match_htlc_output(const struct bitcoin_tx *tx,
 	return -1;
 }
 
+/* Tell master about any we didn't use, if it wants to know. */
+static void note_missing_htlcs(u8 **htlc_scripts,
+			       const struct htlc_stub *htlcs,
+			       const bool *tell_if_missing,
+			       const bool *tell_immediately)
+{
+	for (size_t i = 0; i < tal_count(htlcs); i++) {
+		u8 *msg;
+
+		/* Used. */
+		if (!htlc_scripts[i])
+			continue;
+
+		/* Doesn't care. */
+		if (!tell_if_missing[i])
+			continue;
+
+		msg = towire_onchain_missing_htlc_output(missing_htlc_msgs,
+							 &htlcs[i]);
+		if (tell_immediately[i])
+			wire_sync_write(REQ_FD, take(msg));
+		else {
+			size_t n = tal_count(missing_htlc_msgs);
+			tal_resize(&missing_htlc_msgs, n+1);
+			missing_htlc_msgs[n] = msg;
+		}
+	}
+}
+
 static void handle_our_unilateral(const struct bitcoin_tx *tx,
 				  u32 tx_blockheight,
 				  const struct sha256_double *txid,
@@ -1060,6 +1108,8 @@ static void handle_our_unilateral(const struct bitcoin_tx *tx,
 				  const struct pubkey *local_delayed_payment_basepoint,
 				  u64 commit_num,
 				  const struct htlc_stub *htlcs,
+				  const bool *tell_if_missing,
+				  const bool *tell_immediately,
 				  const secp256k1_ecdsa_signature *remote_htlc_sigs,
 				  struct tracked_output **outs)
 {
@@ -1258,12 +1308,16 @@ static void handle_our_unilateral(const struct bitcoin_tx *tx,
 			 *    Output Handling: Their Offers" below. */
 			resolve_their_htlc(out);
 		}
+
 		/* Each of these consumes one HTLC signature */
 		remote_htlc_sigs++;
 		/* We've matched this HTLC, can't do again. */
 		htlc_scripts[j] = NULL;
+
 	}
 
+	note_missing_htlcs(htlc_scripts, htlcs,
+			   tell_if_missing, tell_immediately);
 	wait_for_resolved(outs);
 	tal_free(tmpctx);
 }
@@ -1336,6 +1390,8 @@ static void handle_their_cheat(const struct bitcoin_tx *tx,
 			       const struct pubkey *remote_delayed_payment_basepoint,
 			       u64 commit_num,
 			       const struct htlc_stub *htlcs,
+			       const bool *tell_if_missing,
+			       const bool *tell_immediately,
 			       struct tracked_output **outs)
 {
 	const tal_t *tmpctx = tal_tmpctx(NULL);
@@ -1564,6 +1620,8 @@ static void handle_their_cheat(const struct bitcoin_tx *tx,
 		htlc_scripts[j] = NULL;
 	}
 
+	note_missing_htlcs(htlc_scripts, htlcs,
+			   tell_if_missing, tell_immediately);
 	wait_for_resolved(outs);
 	tal_free(tmpctx);
 }
@@ -1580,6 +1638,8 @@ static void handle_their_unilateral(const struct bitcoin_tx *tx,
 				    const struct pubkey *remote_delayed_payment_basepoint,
 				    u64 commit_num,
 				    const struct htlc_stub *htlcs,
+				    const bool *tell_if_missing,
+				    const bool *tell_immediately,
 				    struct tracked_output **outs)
 {
 	const tal_t *tmpctx = tal_tmpctx(NULL);
@@ -1759,6 +1819,8 @@ static void handle_their_unilateral(const struct bitcoin_tx *tx,
 		htlc_scripts[j] = NULL;
 	}
 
+	note_missing_htlcs(htlc_scripts, htlcs,
+			   tell_if_missing, tell_immediately);
 	wait_for_resolved(outs);
 	tal_free(tmpctx);
 }
@@ -1783,6 +1845,7 @@ int main(int argc, char *argv[])
 	u64 funding_amount_satoshi, num_htlcs;
 	u8 *scriptpubkey[NUM_SIDES];
 	struct htlc_stub *htlcs;
+	bool *tell_if_missing, *tell_immediately;
 	u32 tx_blockheight;
 
 	if (argc == 2 && streq(argv[1], "--version")) {
@@ -1798,6 +1861,8 @@ int main(int argc, char *argv[])
 						 | SECP256K1_CONTEXT_SIGN);
 	status_setup_sync(REQ_FD);
 
+	missing_htlc_msgs = tal_arr(ctx, u8 *, 0);
+
 	msg = wire_sync_read(ctx, REQ_FD);
 	tx = tal(ctx, struct bitcoin_tx);
 	if (!fromwire_onchain_init(ctx, msg, NULL,
@@ -1819,6 +1884,7 @@ int main(int argc, char *argv[])
 				   &remote_delayed_payment_basepoint,
 				   tx,
 				   &tx_blockheight,
+				   &htlc_missing_depth,
 				   &remote_htlc_sigs,
 				   &num_htlcs)) {
 		master_badmsg(WIRE_ONCHAIN_INIT, msg);
@@ -1828,13 +1894,17 @@ int main(int argc, char *argv[])
 
 	/* FIXME: Filter as we go, don't load them all into mem! */
 	htlcs = tal_arr(ctx, struct htlc_stub, num_htlcs);
-	if (!htlcs)
+	tell_if_missing = tal_arr(ctx, bool, num_htlcs);
+	tell_immediately = tal_arr(ctx, bool, num_htlcs);
+	if (!htlcs || !tell_if_missing || !tell_immediately)
 		status_failed(STATUS_FAIL_INTERNAL_ERROR,
 			      "Can't allocate %"PRIu64" htlcs", num_htlcs);
 
 	for (u64 i = 0; i < num_htlcs; i++) {
 		msg = wire_sync_read(ctx, REQ_FD);
-		if (!fromwire_onchain_htlc(msg, NULL, &htlcs[i]))
+		if (!fromwire_onchain_htlc(msg, NULL, &htlcs[i],
+					   &tell_if_missing[i],
+					   &tell_immediately[i]))
 			master_badmsg(WIRE_ONCHAIN_HTLC, msg);
 	}
 
@@ -1892,6 +1962,7 @@ int main(int argc, char *argv[])
 					      &basepoints.delayed_payment,
 					      commit_num,
 					      htlcs,
+					      tell_if_missing, tell_immediately,
 					      remote_htlc_sigs,
 					      outs);
 		/* BOLT #5:
@@ -1913,7 +1984,9 @@ int main(int argc, char *argv[])
 					   &remote_payment_basepoint,
 					   &remote_delayed_payment_basepoint,
 					   commit_num,
-					   htlcs, outs);
+					   htlcs,
+					   tell_if_missing, tell_immediately,
+					   outs);
 		/* BOLT #5:
 		 *
 		 * Note that there can be more than one valid,
@@ -1933,7 +2006,10 @@ int main(int argc, char *argv[])
 						&remote_payment_basepoint,
 						&remote_delayed_payment_basepoint,
 						commit_num,
-						htlcs, outs);
+						htlcs,
+						tell_if_missing,
+						tell_immediately,
+						outs);
 		} else if (commit_num == revocations_received(&shachain) + 1) {
 			status_trace("Their unilateral tx, new commit point");
 			handle_their_unilateral(tx, tx_blockheight,
@@ -1944,7 +2020,10 @@ int main(int argc, char *argv[])
 						&remote_payment_basepoint,
 						&remote_delayed_payment_basepoint,
 						commit_num,
-						htlcs, outs);
+						htlcs,
+						tell_if_missing,
+						tell_immediately,
+						outs);
 		} else
 			status_failed(STATUS_FAIL_INTERNAL_ERROR,
 				      "Unknown commitment index %"PRIu64

onchaind/onchain_wire.csv

@@ -26,6 +26,7 @@ onchain_init,,remote_payment_basepoint,struct pubkey
 onchain_init,,remote_delayed_payment_basepoint,struct pubkey
 onchain_init,,tx,struct bitcoin_tx
 onchain_init,,tx_blockheight,u32
+onchain_init,,htlc_missing_depth,u32
 onchain_init,,num_htlc_sigs,u16
 onchain_init,,htlc_signature,num_htlc_sigs*secp256k1_ecdsa_signature
 onchain_init,,num_htlcs,u64
@@ -34,6 +35,9 @@ onchain_init,,num_htlcs,u64
 # This is all the HTLCs: one per message
 onchain_htlc,2
 onchain_htlc,,htlc,struct htlc_stub
+# If it's not in the commitment tx, tell us (immediately or htlc_missing_depth)
+onchain_htlc,,tell_if_missing,bool
+onchain_htlc,,tell_immediately,bool
 
 # This sets what the state is, depending on tx.
 onchain_init_reply,101
@@ -67,3 +71,6 @@ onchain_known_preimage,,preimage,struct preimage
 onchain_extracted_preimage,8
 onchain_extracted_preimage,,preimage,struct preimage
 
+# onchaind->master: this HTLC was missing from commit tx.
+onchain_missing_htlc_output,9
+onchain_missing_htlc_output,,htlc,struct htlc_stub

tests/test_lightningd.py

@@ -447,6 +447,72 @@ class LightningDTests(BaseLightningDTests):
         bitcoind.rpc.generate(6)
         l2.daemon.wait_for_log('onchaind complete, forgetting peer')
 
+    def test_onchain_dust_out(self):
+        """Onchain handling of outgoing dust htlcs (they should fail)"""
+        # HTLC 1->2, 1 fails after it's irrevocably committed
+        disconnects = ['@WIRE_REVOKE_AND_ACK', 'permfail']
+        l1 = self.node_factory.get_node(disconnect=disconnects)
+        l2 = self.node_factory.get_node()
+
+        l1.rpc.connect('localhost', l2.info['port'], l2.info['id'])
+        self.fund_channel(l1, l2, 10**6)
+
+        # Must be dust!
+        rhash = l2.rpc.invoice(1, 'onchain_dust_out')['rhash']
+        routestep = {
+            'msatoshi' : 1,
+            'id' : l2.info['id'],
+            'delay' : 5,
+            'channel': '1:1:1'
+        }
+
+        q = queue.Queue()
+
+        def try_pay():
+            try:
+                l1.rpc.sendpay(to_json([routestep]), rhash, async=False)
+                q.put(None)
+            except Exception as err:
+                q.put(err)
+
+        t = threading.Thread(target=try_pay)
+        t.daemon = True
+        t.start()
+
+        # l1 will drop to chain.
+        l1.daemon.wait_for_log('permfail')
+        l1.daemon.wait_for_log('sendrawtx exit 0')
+        l1.bitcoin.rpc.generate(1)
+        l1.daemon.wait_for_log('-> ONCHAIND_OUR_UNILATERAL')
+        l2.daemon.wait_for_log('-> ONCHAIND_THEIR_UNILATERAL')
+
+        # We use 3 blocks for "reasonable depth"
+        bitcoind.rpc.generate(3)
+
+        # It should fail.
+        err = q.get(timeout = 5)
+        assert type(err) is ValueError
+        t.join(timeout=1)
+        assert not t.isAlive()
+
+        l1.daemon.wait_for_log('WIRE_PERMANENT_CHANNEL_FAILURE: missing in commitment tx')
+
+        # 6 later, l1 should collect its to-self payment.
+        bitcoind.rpc.generate(6)
+        l1.daemon.wait_for_log('Broadcasting OUR_DELAYED_RETURN_TO_WALLET .* to resolve OUR_UNILATERAL/DELAYED_OUTPUT_TO_US')
+        l1.daemon.wait_for_log('sendrawtx exit 0')
+
+        # 94 later, l2 is done.
+        bitcoind.rpc.generate(94)
+        l2.daemon.wait_for_log('onchaind complete, forgetting peer')
+
+        # Now, 100 blocks and l1 should be done.
+        bitcoind.rpc.generate(6)
+        l1.daemon.wait_for_log('onchaind complete, forgetting peer')
+
+        # Payment failed, BTW
+        assert l2.rpc.listinvoice('onchain_dust_out')[0]['complete'] == False
+
     def test_onchain_middleman(self):
         # HTLC 1->2->3, 1->2 goes down after 2 gets preimage from 3.
         disconnects = ['-WIRE_UPDATE_FULFILL_HTLC', 'permfail']