lnwallet: return balance changes rather than modifying references

Here we return the balance deltas from evaluateHTLCView rather than
passing in references to variables that will be modified. It is a
far cleaner and compositional approach which allows readers of this
code to more effectively reason about the code without having to
keep the whole codebase in their head.

lnwallet/channel.go

@@ -2893,10 +2893,9 @@ func fundingTxIn(chanState *channeldb.OpenChannel) wire.TxIn {
 // 1. The new htlcView reflecting the current channel state.
 // 2. A Dual of the updates which have not yet been committed in
 // 'whoseCommitChain's commitment chain.
-func (lc *LightningChannel) evaluateHTLCView(view *HtlcView, ourBalance,
+func (lc *LightningChannel) evaluateHTLCView(view *HtlcView,
-	theirBalance *lnwire.MilliSatoshi, nextHeight uint64,
+	whoseCommitChain lntypes.ChannelParty, nextHeight uint64) (*HtlcView,
-	whoseCommitChain lntypes.ChannelParty) (*HtlcView,
+	lntypes.Dual[[]*paymentDescriptor], lntypes.Dual[int64], error) {
-	lntypes.Dual[[]*paymentDescriptor], error) {
 
 	// We initialize the view's fee rate to the fee rate of the unfiltered
 	// view. If any fee updates are found when evaluating the view, it will
@@ -2929,6 +2928,8 @@ func (lc *LightningChannel) evaluateHTLCView(view *HtlcView, ourBalance,
 		Remote: fn.NewSet[uint64](),
 	}
 
+	balanceDeltas := lntypes.Dual[int64]{}
+
 	parties := [2]lntypes.ChannelParty{lntypes.Local, lntypes.Remote}
 	for _, party := range parties {
 		// First we run through non-add entries in both logs,
@@ -2947,7 +2948,8 @@ func (lc *LightningChannel) evaluateHTLCView(view *HtlcView, ourBalance,
 				entry, whoseCommitChain, party.CounterParty(),
 			)
 			if err != nil {
-				return nil, noUncommitted, err
+				noDeltas := lntypes.Dual[int64]{}
+				return nil, noUncommitted, noDeltas, err
 			}
 
 			skipSet := skip.GetForParty(party.CounterParty())
@@ -2959,41 +2961,30 @@ func (lc *LightningChannel) evaluateHTLCView(view *HtlcView, ourBalance,
 			if rmvHeight == 0 {
 				switch {
 				// If an incoming HTLC is being settled, then
-				// this means that we've received the preimage
+				// this means that the preimage has been
-				// either from another subsystem, or the
+				// received by the settling party Therefore, we
-				// upstream peer in the route. Therefore, we
+				// increase the settling party's balance by the
-				// increase our balance by the HTLC amount.
+				// HTLC amount.
-				case party.CounterParty() == lntypes.Remote &&
+				case entry.EntryType == Settle:
-					entry.EntryType == Settle:
+					delta := int64(entry.Amount)
-
+					balanceDeltas.ModifyForParty(
-					*ourBalance += entry.Amount
+						party,
+						func(acc int64) int64 {
+							return acc + delta
+						},
+					)
 
 				// Otherwise, this HTLC is being failed out,
 				// therefore the value of the HTLC should
-				// return to the remote party.
+				// return to the failing party's counterparty.
-				case party.CounterParty() == lntypes.Remote &&
+				case entry.EntryType != Settle:
-					entry.EntryType != Settle:
+					delta := int64(entry.Amount)
-
+					balanceDeltas.ModifyForParty(
-					*theirBalance += entry.Amount
+						party.CounterParty(),
-
+						func(acc int64) int64 {
-				// If an outgoing HTLC is being settled, then
+							return acc + delta
-				// this means that the downstream party
+						},
-				// resented the preimage or learned of it via a
+					)
-				// downstream peer. In either case, we credit
-				// their settled value with the value of the
-				// HTLC.
-				case party.CounterParty() == lntypes.Local &&
-					entry.EntryType == Settle:
-
-					*theirBalance += entry.Amount
-
-				// Otherwise, one of our outgoing HTLC's has
-				// timed out, so the value of the HTLC should
-				// be returned to our settled balance.
-				case party.CounterParty() == lntypes.Local &&
-					entry.EntryType != Settle:
-
-					*ourBalance += entry.Amount
 				}
 			}
 		}
@@ -3015,19 +3006,19 @@ func (lc *LightningChannel) evaluateHTLCView(view *HtlcView, ourBalance,
 				whoseCommitChain,
 			)
 			if addHeight == 0 {
-				if party == lntypes.Remote {
+				// If this is a new incoming (un-committed)
-					// If this is a new incoming
+				// HTLC, then we need to update their balance
-					// (un-committed) HTLC, then we need
+				// accordingly by subtracting the amount of
-					// to update their balance accordingly
+				// the HTLC that are funds pending.
-					// by subtracting the amount of the
+				// Similarly, we need to debit our balance if
-					// HTLC that are funds pending.
+				// this is an out going HTLC to reflect the
-					*theirBalance -= entry.Amount
+				// pending balance.
-				} else {
+				balanceDeltas.ModifyForParty(
-					// Similarly, we need to debit our
+					party,
-					// balance if this is an out going HTLC
+					func(acc int64) int64 {
-					// to reflect the pending balance.
+						return acc - int64(entry.Amount)
-					*ourBalance -= entry.Amount
+					},
-				}
+				)
 			}
 		}
 
@@ -3068,7 +3059,7 @@ func (lc *LightningChannel) evaluateHTLCView(view *HtlcView, ourBalance,
 		},
 	)
 
-	return newView, uncommittedUpdates, nil
+	return newView, uncommittedUpdates, balanceDeltas, nil
 }
 
 // fetchParent is a helper that looks up update log parent entries in the
@@ -4589,13 +4580,26 @@ func (lc *LightningChannel) computeView(view *HtlcView,
 	// channel constraints to the final commitment state. If any fee
 	// updates are found in the logs, the commitment fee rate should be
 	// changed, so we'll also set the feePerKw to this new value.
-	filteredHTLCView, uncommitted, err := lc.evaluateHTLCView(
+	filteredHTLCView, uncommitted, deltas, err := lc.evaluateHTLCView(
-		view, &ourBalance, &theirBalance, nextHeight, whoseCommitChain,
+		view, whoseCommitChain, nextHeight,
 	)
 	if err != nil {
 		return 0, 0, 0, nil, err
 	}
 
+	// Add the balance deltas to the balances we got from the commitment
+	// state.
+	if deltas.Local >= 0 {
+		ourBalance += lnwire.MilliSatoshi(deltas.Local)
+	} else {
+		ourBalance -= lnwire.MilliSatoshi(-1 * deltas.Local)
+	}
+	if deltas.Remote >= 0 {
+		theirBalance += lnwire.MilliSatoshi(deltas.Remote)
+	} else {
+		theirBalance -= lnwire.MilliSatoshi(-1 * deltas.Remote)
+	}
+
 	if updateState {
 		for _, party := range lntypes.BothParties {
 			for _, u := range uncommitted.GetForParty(party) {
@@ -4619,8 +4623,8 @@ func (lc *LightningChannel) computeView(view *HtlcView,
 	}
 
 	// We need to first check ourBalance and theirBalance to be negative
-	// because MilliSathoshi is a unsigned type and can underflow in
+	// because MilliSathoshi is a unsigned type and can underflow in the
-	// `evaluateHTLCView`. This should never happen for views which do not
+	// code above. This should never happen for views which do not
 	// include new updates (remote or local).
 	if int64(ourBalance) < 0 {
 		err := fmt.Errorf("%w: our balance", ErrBelowChanReserve)

lnwallet/channel_test.go

@@ -8953,19 +8953,12 @@ func TestEvaluateView(t *testing.T) {
 				FeePerKw: feePerKw,
 			}
 
-			var (
-				// Create vars to store balance changes. We do
-				// not check these values in this test because
-				// balance modification happens on the htlc
-				// processing level.
-				ourBalance   lnwire.MilliSatoshi
-				theirBalance lnwire.MilliSatoshi
-			)
-
 			// Evaluate the htlc view, mutate as test expects.
-			result, uncommitted, err := lc.evaluateHTLCView(
+			// We do not check the balance deltas in this test
-				view, &ourBalance, &theirBalance, nextHeight,
+			// because balance modification happens on the htlc
-				test.whoseCommitChain,
+			// processing level.
+			result, uncommitted, _, err := lc.evaluateHTLCView(
+				view, test.whoseCommitChain, nextHeight,
 			)
 
 			if err != nil {