lnwallet: within validateCommitmentSanity check for balance underflow

In this commit, we add an additional check within
validateCommitmentSanity due to the recent change to unsigned integers
for peer balances in the channel state machine. If after evaluation
(just applying HTLC updates), the balances are negative, then we’ll
return ErrBelowChanReserve.

lnwallet/channel.go

@@ -3051,14 +3051,14 @@ func (lc *LightningChannel) ChanSyncMsg() (*lnwire.ChannelReestablish, error) {
 	}, nil
 }
 
-// computeView takes the given htlcView, and calculates the balances,
+// computeView takes the given htlcView, and calculates the balances, filtered
-// filtered view (settling unsettled HTLCs), commitment weight and
+// view (settling unsettled HTLCs), commitment weight and feePerKw, after
-// feePerKw, after applying the HTLCs to the latest commitment. The
+// applying the HTLCs to the latest commitment. The returned balanced are the
-// returned balanced are the balances *before* subtracting the
+// balances *before* subtracting the commitment fee from the initiator's
-// commitment fee from the initiator's balance.
+// balance.
 //
-// If the updateState boolean is set true, the add and remove heights
+// If the updateState boolean is set true, the add and remove heights of the
-// of the HTLCs will be set to the next commitment height.
+// HTLCs will be set to the next commitment height.
 func (lc *LightningChannel) computeView(view *htlcView, remoteChain bool,
 	updateState bool) (lnwire.MilliSatoshi, lnwire.MilliSatoshi, int64,
 	*htlcView, SatPerKWeight) {
@@ -3070,16 +3070,16 @@ func (lc *LightningChannel) computeView(view *htlcView, remoteChain bool,
 		dustLimit = lc.remoteChanCfg.DustLimit
 	}
 
-	// Since the fetched htlc view will include all updates added
+	// Since the fetched htlc view will include all updates added after the
-	// after the last committed state, we start with the balances
+	// last committed state, we start with the balances reflecting that
-	// reflecting that state.
+	// state.
 	ourBalance := commitChain.tip().ourBalance
 	theirBalance := commitChain.tip().theirBalance
 
 	// Add the fee from the previous commitment state back to the
 	// initiator's balance, so that the fee can be recalculated and
-	// re-applied in case fee estimation parameters have changed or
+	// re-applied in case fee estimation parameters have changed or the
-	// the number of outstanding HTLCs has changed.
+	// number of outstanding HTLCs has changed.
 	if lc.channelState.IsInitiator {
 		ourBalance += lnwire.NewMSatFromSatoshis(
 			commitChain.tip().fee)
@@ -3089,11 +3089,10 @@ func (lc *LightningChannel) computeView(view *htlcView, remoteChain bool,
 	}
 	nextHeight := commitChain.tip().height + 1
 
-	// We evaluate the view at this stage, meaning settled and
+	// We evaluate the view at this stage, meaning settled and failed HTLCs
-	// failed HTLCs will remove their corresponding added HTLCs.
+	// will remove their corresponding added HTLCs.  The resulting filtered
-	// The resulting filtered view will only have Add entries left,
+	// view will only have Add entries left, making it easy to compare the
-	// making it easy to compare the channel constraints to the
+	// channel constraints to the final commitment state.
-	// final commitment state.
 	filteredHTLCView := lc.evaluateHTLCView(view, &ourBalance,
 		&theirBalance, nextHeight, remoteChain, updateState)
 
@@ -3164,6 +3163,7 @@ func (lc *LightningChannel) computeView(view *htlcView, remoteChain bool,
 func (lc *LightningChannel) validateCommitmentSanity(theirLogCounter,
 	ourLogCounter uint64, remoteChain bool,
 	predictAdded *PaymentDescriptor) error {
+
 	// Fetch all updates not committed.
 	view := lc.fetchHTLCView(theirLogCounter, ourLogCounter)
 
@@ -3171,8 +3171,8 @@ func (lc *LightningChannel) validateCommitmentSanity(theirLogCounter,
 	// update log, in order to validate the sanity of the commitment
 	// resulting from _actually adding_ this HTLC to the state.
 	if predictAdded != nil {
-		// If we are adding an HTLC, this will be an Add to the
+		// If we are adding an HTLC, this will be an Add to the local
-		// local update log.
+		// update log.
 		view.ourUpdates = append(view.ourUpdates, predictAdded)
 	}
 
@@ -3183,21 +3183,33 @@ func (lc *LightningChannel) validateCommitmentSanity(theirLogCounter,
 	ourInitialBalance := commitChain.tip().ourBalance
 	theirInitialBalance := commitChain.tip().theirBalance
 
-	ourBalance, theirBalance, commitWeight, filteredView, feePerKw :=
+	ourBalance, theirBalance, commitWeight, filteredView, feePerKw := lc.computeView(
-		lc.computeView(view, remoteChain, false)
+		view, remoteChain, false,
+	)
 
-	// Calculate the commitment fee, and subtract it from the
+	// Calculate the commitment fee, and subtract it from the initiator's
-	// initiator's balance.
+	// balance.
 	commitFee := feePerKw.FeeForWeight(commitWeight)
+	commitFeeMsat := lnwire.NewMSatFromSatoshis(commitFee)
 	if lc.channelState.IsInitiator {
-		ourBalance -= lnwire.NewMSatFromSatoshis(commitFee)
+		ourBalance -= commitFeeMsat
 	} else {
-		theirBalance -= lnwire.NewMSatFromSatoshis(commitFee)
+		theirBalance -= commitFeeMsat
 	}
 
-	// If the added HTLCs will decrease the balance, make sure
+	// As a quick sanity check, we'll ensure that if we interpret the
-	// they won't dip the local and remote balances below the
+	// balances as signed integers, they haven't dipped down below zero. If
-	// channel reserves.
+	// they have, then this indicates that a party doesn't have sufficient
+	// balance to satisfy the final evaluated HTLC's.
+	switch {
+	case int64(ourBalance) < 0:
+		return ErrBelowChanReserve
+	case int64(theirBalance) < 0:
+		return ErrBelowChanReserve
+	}
+
+	// If the added HTLCs will decrease the balance, make sure they won't
+	// dip the local and remote balances below the channel reserves.
 	if ourBalance < ourInitialBalance &&
 		ourBalance < lnwire.NewMSatFromSatoshis(
 			lc.localChanCfg.ChanReserve) {
@@ -3210,8 +3222,8 @@ func (lc *LightningChannel) validateCommitmentSanity(theirLogCounter,
 		return ErrBelowChanReserve
 	}
 
-	// validateUpdates take a set of updates, and validates them
+	// validateUpdates take a set of updates, and validates them against
-	// against the passed channel constraints.
+	// the passed channel constraints.
 	validateUpdates := func(updates []*PaymentDescriptor,
 		constraints *channeldb.ChannelConfig) error {
 

lnwallet/channel_test.go

@@ -2462,8 +2462,8 @@ func TestAddHTLCNegativeBalance(t *testing.T) {
 	}
 	defer cleanUp()
 
-	// We set the channel reserve to 0, such that we can add HTLCs
+	// We set the channel reserve to 0, such that we can add HTLCs all the
-	// all the way to a negative balance.
+	// way to a negative balance.
 	aliceChannel.localChanCfg.ChanReserve = 0
 
 	// First, we'll add 3 HTLCs of 1 BTC each to Alice's commitment.
@@ -2476,14 +2476,15 @@ func TestAddHTLCNegativeBalance(t *testing.T) {
 		}
 	}
 
-	// Alice now has an available balance of 2 BTC. We'll add a new HTLC
+	// Alice now has an available balance of 2 BTC. We'll add a new HTLC of
-	// of value 2 BTC, which should make Alice's balance negative (since
+	// value 2 BTC, which should make Alice's balance negative (since (she
-	// (she has to pay a commitment fee).
+	// has to pay a commitment fee).
 	htlcAmt = lnwire.NewMSatFromSatoshis(2 * btcutil.SatoshiPerBitcoin)
 	htlc, _ := createHTLC(numHTLCs+1, htlcAmt)
 	_, err = aliceChannel.AddHTLC(htlc)
 	if err != ErrBelowChanReserve {
-		t.Fatalf("expected balance below channel reserve, instead got: %v", err)
+		t.Fatalf("expected balance below channel reserve, instead "+
+			"got: %v", err)
 	}
 }
 
@@ -4375,23 +4376,22 @@ func TestDesyncHTLCs(t *testing.T) {
 		t.Fatalf("unable to recv htlc cancel: %v", err)
 	}
 
-	// Alice now has gotten all here original balance (5 BTC) back,
+	// Alice now has gotten all her original balance (5 BTC) back, however,
-	// however, adding a new HTLC at this point SHOULD fail, since
+	// adding a new HTLC at this point SHOULD fail, since if she adds the
-	// if she add the HTLC and sign the next state, Bob cannot assume
+	// HTLC and sign the next state, Bob cannot assume she received the
-	// she received the FailHTLC, and must assume she doesn't have
+	// FailHTLC, and must assume she doesn't have the necessary balance
-	// the necessary balance available.
+	// available.
 	//
-	// We try adding an HTLC of value 1 BTC, which should fail
+	// We try adding an HTLC of value 1 BTC, which should fail because the
-	// because the balance is unavailable.
+	// balance is unavailable.
 	htlcAmt = lnwire.NewMSatFromSatoshis(1 * btcutil.SatoshiPerBitcoin)
 	htlc, _ = createHTLC(1, htlcAmt)
 	if _, err = aliceChannel.AddHTLC(htlc); err != ErrBelowChanReserve {
-		t.Fatalf("expected ErrInsufficientBalance, instead received: %v",
+		t.Fatalf("expected ErrBelowChanReserve, instead received: %v", err)
-			err)
 	}
 
-	// Now do a state transition, which will ACK the FailHTLC, making
+	// Now do a state transition, which will ACK the FailHTLC, making Alice
-	// Alice able to add the new HTLC.
+	// able to add the new HTLC.
 	if err := forceStateTransition(aliceChannel, bobChannel); err != nil {
 		t.Fatalf("unable to complete state update: %v", err)
 	}
@@ -4403,7 +4403,8 @@ func TestDesyncHTLCs(t *testing.T) {
 // TODO(roasbeef): testing.Quick test case for retrans!!!
 
 // TestMaxAcceptedHTLCs tests that the correct error message (ErrMaxHTLCNumber)
-// is thrown when a node tries to accept more than MaxAcceptedHTLCs in a channel.
+// is thrown when a node tries to accept more than MaxAcceptedHTLCs in a
+// channel.
 func TestMaxAcceptedHTLCs(t *testing.T) {
 	t.Parallel()