From 087ab166b3967b71d5ea3f1373ee2e4d2231376f Mon Sep 17 00:00:00 2001
From: lisa neigut <niftynei@gmail.com>
Date: Wed, 8 Apr 2020 18:18:00 -0500
Subject: [PATCH] coins, fix: don't crash if the to_us amount is greater than
 our_msat

It's possible for our peer to publish a commitment tx that has already
updated our balance for an htlc before we've completed removing it from
our commitment tx (aka before we've updated our balance). This used to
crash, now we just update our balance (and the channel balance logs!)
and keep going.

If they've removed anything from our balance, we'll end up counting it
as chain_fees below. Not ideal but fine... probably.
---
 onchaind/onchaind.c   | 38 +++++++++++++++++++++++++++++++++++---
 tests/test_closing.py |  7 ++++++-
 2 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/onchaind/onchaind.c b/onchaind/onchaind.c
index 35e4b2d7e..2a6384a15 100644
--- a/onchaind/onchaind.c
+++ b/onchaind/onchaind.c
@@ -295,15 +295,47 @@ static void record_chain_fees_unilateral(const struct bitcoin_txid *txid,
 		     type_to_string(tmpctx, struct amount_sat, &their_outs),
 		     type_to_string(tmpctx, struct amount_sat, &our_outs));
 
+	/* It's possible they published a commitment tx that
+	 * paid us an htlc before we updated our balance. It's also
+	 * possible that they fulfilled an htlc, but we'll just write
+	 * that down in the chain fees :/ */
+	if (!amount_msat_greater_eq_sat(our_msat, our_outs)) {
+		struct amount_msat missing;
+		struct chain_coin_mvt *mvt;
+
+		if (!amount_sat_sub_msat(&missing, our_outs, our_msat))
+			status_failed(STATUS_FAIL_INTERNAL_ERROR,
+				      "unable to subtract %s from %s",
+				      type_to_string(tmpctx, struct amount_msat,
+						     &our_msat),
+				      type_to_string(tmpctx, struct amount_sat,
+						     &our_outs));
+
+		/* Log the difference and update our_msat */
+		mvt = new_chain_coin_mvt(NULL, NULL,
+					 txid, NULL, 0, NULL,
+					 blockheight,
+					 JOURNAL, missing,
+					 true, BTC);
+		send_coin_mvt(take(mvt));
+		if (!amount_msat_add(&our_msat, our_msat, missing))
+			status_failed(STATUS_FAIL_INTERNAL_ERROR,
+				      "unable to add %s to %s",
+				      type_to_string(tmpctx, struct amount_msat,
+						     &missing),
+				      type_to_string(tmpctx, struct amount_msat,
+						     &our_msat));
+	}
+
 	/* we need to figure out what we paid in fees, total.
 	 * this encompasses the actual chain fees + any trimmed outputs */
 	if (!amount_msat_sub_sat(&trimmed, our_msat, our_outs))
 		status_failed(STATUS_FAIL_INTERNAL_ERROR,
 			      "unable to subtract %s from %s",
-			      type_to_string(tmpctx, struct amount_msat,
-					     &our_msat),
 			      type_to_string(tmpctx, struct amount_sat,
-					     &our_outs));
+					     &our_outs),
+			      type_to_string(tmpctx, struct amount_msat,
+					     &our_msat));
 
 	status_debug("logging 'chain fees' for unilateral (trimmed) %s",
 		     type_to_string(tmpctx, struct amount_msat, &trimmed));
diff --git a/tests/test_closing.py b/tests/test_closing.py
index 4607e23fb..a9c5c746c 100644
--- a/tests/test_closing.py
+++ b/tests/test_closing.py
@@ -1055,7 +1055,10 @@ def test_onchain_first_commit(node_factory, bitcoind):
 @unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1")
 def test_onchain_unwatch(node_factory, bitcoind):
     """Onchaind should not watch random spends"""
-    l1, l2 = node_factory.line_graph(2)
+    # We track channel balances, to verify that accounting is ok.
+    coin_mvt_plugin = os.path.join(os.getcwd(), 'tests/plugins/coin_movements.py')
+    l1, l2 = node_factory.line_graph(2, opts={'plugin': coin_mvt_plugin})
+    channel_id = first_channel_id(l1, l2)
 
     l1.pay(l2, 200000000)
 
@@ -1097,6 +1100,8 @@ def test_onchain_unwatch(node_factory, bitcoind):
     assert not l1.daemon.is_in_log("but we don't care",
                                    start=l1.daemon.logsearch_start)
 
+    assert account_balance(l1, channel_id) == 0
+    assert account_balance(l2, channel_id) == 0
     # Note: for this test we leave onchaind running, so we can detect
     # any leaks!