From 1b9791f0f5be440e530ecfe08a17b921df075958 Mon Sep 17 00:00:00 2001
From: Rusty Russell <rusty@rustcorp.com.au>
Date: Fri, 16 Mar 2018 10:39:37 +1030
Subject: [PATCH] pay: delete HTLC when we delete payment.

This fixes the root cause of https://github.com/ElementsProject/lightning/issues/1212
where we deleted the payment because we wanted to retry, then retry failed
so we had an (old) HTLC without a matching payment.  We then fed that
HTLC to onchaind, which tells us it's missing, and we try to fail the
payment and deref a NULL pointer.

Fixes: #1212
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 lightningd/pay.c         | 11 ++++++++++-
 tests/test_lightningd.py |  1 -
 wallet/wallet.c          | 18 ++++++++++++++++++
 wallet/wallet.h          | 11 +++++++++++
 4 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/lightningd/pay.c b/lightningd/pay.c
index 3e7801296..cb7921230 100644
--- a/lightningd/pay.c
+++ b/lightningd/pay.c
@@ -705,7 +705,6 @@ send_payment(const tal_t *ctx,
 			cb(result, cbarg);
 			return false;
 		}
-		wallet_payment_delete(ld->wallet, rhash);
 		log_add(ld->log, "... retrying");
 	}
 
@@ -757,6 +756,16 @@ send_payment(const tal_t *ctx,
 	for (i = 0; i < n_hops; ++i)
 		channels[i] = route[i].channel_id;
 
+	/* If we're retrying, delete all trace of previous one.  We delete
+	 * outgoing HTLC, too, otherwise it gets reported to onchaind as
+	 * a possibility, and we end up in handle_missing_htlc_output->
+	 * onchain_failed_our_htlc->payment_failed with no payment.
+	 */
+	if (payment) {
+		wallet_payment_delete(ld->wallet, rhash);
+		wallet_local_htlc_out_delete(ld->wallet, channel, rhash);
+	}
+
 	/* If hout fails, payment should be freed too. */
 	payment = tal(hout, struct wallet_payment);
 	payment->id = 0;
diff --git a/tests/test_lightningd.py b/tests/test_lightningd.py
index cf056d106..73d0797b1 100644
--- a/tests/test_lightningd.py
+++ b/tests/test_lightningd.py
@@ -1366,7 +1366,6 @@ class LightningDTests(BaseLightningDTests):
         # Note: for this test we leave onchaind running, so we can detect
         # any leaks!
 
-    @unittest.skip("Expected failure")
     @unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1")
     def test_onchain_dust_out(self):
         """Onchain handling of outgoing dust htlcs (they should fail)"""
diff --git a/wallet/wallet.c b/wallet/wallet.c
index 3036acf35..ded5c83bf 100644
--- a/wallet/wallet.c
+++ b/wallet/wallet.c
@@ -1468,6 +1468,24 @@ struct htlc_stub *wallet_htlc_stubs(const tal_t *ctx, struct wallet *wallet,
 	return stubs;
 }
 
+void wallet_local_htlc_out_delete(struct wallet *wallet,
+				  struct channel *chan,
+				  const struct sha256 *payment_hash)
+{
+	sqlite3_stmt *stmt;
+
+	stmt = db_prepare(wallet->db,
+			  "DELETE FROM channel_htlcs"
+			  " WHERE direction = ?"
+			  " AND origin_htlc = ?"
+			  " AND payment_hash = ?");
+	sqlite3_bind_int(stmt, 1, DIRECTION_OUTGOING);
+	sqlite3_bind_int(stmt, 2, 0);
+	sqlite3_bind_sha256(stmt, 3, payment_hash);
+
+	db_exec_prepared(wallet->db, stmt);
+}
+
 static struct wallet_payment *
 find_unstored_payment(struct wallet *wallet, const struct sha256 *payment_hash)
 {
diff --git a/wallet/wallet.h b/wallet/wallet.h
index f0c8efcd8..d7ad9a4c8 100644
--- a/wallet/wallet.h
+++ b/wallet/wallet.h
@@ -650,6 +650,17 @@ void wallet_payment_store(struct wallet *wallet,
 void wallet_payment_delete(struct wallet *wallet,
 			   const struct sha256 *payment_hash);
 
+/**
+ * wallet_local_htlc_out_delete - Remove a local outgoing failed HTLC
+ *
+ * This is not a generic HTLC cleanup!  This is specifically for the
+ * narrow (and simple!) case of removing the HTLC associated with a
+ * local outgoing payment.
+ */
+void wallet_local_htlc_out_delete(struct wallet *wallet,
+				  struct channel *chan,
+				  const struct sha256 *payment_hash);
+
 /**
  * wallet_payment_by_hash - Retrieve a specific payment
  *