From 0d9a1b865621c918820a885007b21da8e4ddcd1b Mon Sep 17 00:00:00 2001
From: "Johan T. Halseth" <johanth@gmail.com>
Date: Wed, 19 Feb 2020 12:27:41 +0100
Subject: [PATCH] lnwallet: check local commitment sanity when adding HTLC

This commit adds an extra validation step when adding HTLCs. Previously
we would only validate the remote commitment resulting from adding an
HTLC, which in most cases is enough. However, there are situations where
the dustlimits are different, which could lead to the resulting remote
commitment from adding the HTLC being valid, but not the local
commitment.

Now we also validate the local commitment. A test to trigger the case is
added.
---
 lnwallet/channel.go      | 18 +++++++++++++++-
 lnwallet/channel_test.go | 45 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/lnwallet/channel.go b/lnwallet/channel.go
index 9519e7fe..8b259365 100644
--- a/lnwallet/channel.go
+++ b/lnwallet/channel.go
@@ -4642,8 +4642,11 @@ func (lc *LightningChannel) AddHTLC(htlc *lnwire.UpdateAddHTLC,
 	}
 
 	// Make sure adding this HTLC won't violate any of the constraints we
-	// must keep on our commitment transaction.
+	// must keep on the commitment transactions.
 	remoteACKedIndex := lc.localCommitChain.tail().theirMessageIndex
+
+	// First we'll check whether this HTLC can be added to the remote
+	// commitment transaction without violation any of the constraints.
 	err := lc.validateCommitmentSanity(
 		remoteACKedIndex, lc.localUpdateLog.logIndex, true, pd, nil,
 	)
@@ -4651,6 +4654,19 @@ func (lc *LightningChannel) AddHTLC(htlc *lnwire.UpdateAddHTLC,
 		return 0, err
 	}
 
+	// We must also check whether it can be added to our own commitment
+	// transaction, or the remote node will refuse to sign. This is not
+	// totally bullet proof, as the remote might be adding updates
+	// concurrently, but if we fail this check there is for sure not
+	// possible for us to add the HTLC.
+	err = lc.validateCommitmentSanity(
+		lc.remoteUpdateLog.logIndex, lc.localUpdateLog.logIndex,
+		false, pd, nil,
+	)
+	if err != nil {
+		return 0, err
+	}
+
 	lc.localUpdateLog.appendHtlc(pd)
 
 	return pd.HtlcIndex, nil
diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go
index 3f97f9f9..e9455b75 100644
--- a/lnwallet/channel_test.go
+++ b/lnwallet/channel_test.go
@@ -6061,6 +6061,51 @@ func TestChanReserveRemoteInitiator(t *testing.T) {
 	}
 }
 
+// TestChanReserveLocalInitiatorDustHtlc tests that fee the initiator must pay
+// when adding HTLCs is accounted for, even though the HTLC is considered dust
+// by the remote bode.
+func TestChanReserveLocalInitiatorDustHtlc(t *testing.T) {
+	t.Parallel()
+
+	aliceChannel, bobChannel, cleanUp, err := CreateTestChannels(
+		true,
+	)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer cleanUp()
+
+	// The amount of the HTLC should not be considered dust according to
+	// Alice's dust limit (200 sat), but be dust according to Bob's dust
+	// limit (1300 sat). It is considered dust if the amount remaining
+	// after paying the HTLC fee is below the dustlimit, so we choose a
+	// size of 500+htlcFee.
+	htlcSat := btcutil.Amount(500) + htlcTimeoutFee(
+		chainfee.SatPerKWeight(
+			aliceChannel.channelState.LocalCommitment.FeePerKw,
+		),
+	)
+
+	// Set Alice's channel reserve to be low enough to carry the value of
+	// the HTLC, but not low enough to allow the extra fee from adding the
+	// HTLC to the commitment.
+	commitFee := aliceChannel.channelState.LocalCommitment.CommitFee
+	aliceMinReserve := 5*btcutil.SatoshiPerBitcoin - commitFee - htlcSat
+
+	aliceChannel.channelState.LocalChanCfg.ChanReserve = aliceMinReserve
+	bobChannel.channelState.RemoteChanCfg.ChanReserve = aliceMinReserve
+
+	htlcDustAmt := lnwire.NewMSatFromSatoshis(htlcSat)
+	htlc, _ := createHTLC(0, htlcDustAmt)
+
+	// Alice should realize that the fee she must pay to add this HTLC to
+	// the local commitment would take her below the channel reserve.
+	_, err = aliceChannel.AddHTLC(htlc, nil)
+	if err != ErrBelowChanReserve {
+		t.Fatalf("expected ErrBelowChanReserve, instead received: %v", err)
+	}
+}
+
 // TestMinHTLC tests that the ErrBelowMinHTLC error is thrown if an HTLC is added
 // that is below the minimm allowed value for HTLCs.
 func TestMinHTLC(t *testing.T) {