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

@@ -295,9 +295,15 @@ 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));
 
-	/* we need to figure out what we paid in fees, total.
+	/* It's possible they published a commitment tx that
-	 * this encompasses the actual chain fees + any trimmed outputs */
+	 * paid us an htlc before we updated our balance. It's also
-	if (!amount_msat_sub_sat(&trimmed, our_msat, our_outs))
+	 * 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,
@@ -305,6 +311,32 @@ static void record_chain_fees_unilateral(const struct bitcoin_txid *txid,
 				      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_sat,
+					     &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));
 	update_ledger_chain_fees_msat(txid, blockheight, trimmed);

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!