lnwallet: eliminate inner-most layer of evil mutateState nonsense

This commit begins the process of moving towards a more principled
means of state tracking. We eliminate the mutateState argument from
processAddEntry and processRemoveEntry and move the responsibility
of mutating said state to the call-sites.

The current call-sites of these functions still have their *own*
mutateState argument which will be eliminated during upcoming commits.
However, following the principle of micro-commits I opted to break
these changes up to make review simpler.

lnwallet/channel.go

@@ -2953,10 +2953,19 @@ func (lc *LightningChannel) evaluateHTLCView(view *HtlcView, ourBalance,
 
 		skipThem[addEntry.HtlcIndex] = struct{}{}
 
-		processRemoveEntry(
+		rmvHeights := &entry.removeCommitHeights
-			entry, ourBalance, theirBalance, nextHeight,
+		rmvHeight := rmvHeights.GetForParty(whoseCommitChain)
-			whoseCommitChain, true, mutateState,
+		if rmvHeight == 0 {
-		)
+			processRemoveEntry(
+				entry, ourBalance, theirBalance, true,
+			)
+
+			if mutateState {
+				rmvHeights.SetForParty(
+					whoseCommitChain, nextHeight,
+				)
+			}
+		}
 	}
 	for _, entry := range view.TheirUpdates {
 		switch entry.EntryType {
@@ -2993,10 +3002,19 @@ func (lc *LightningChannel) evaluateHTLCView(view *HtlcView, ourBalance,
 
 		skipUs[addEntry.HtlcIndex] = struct{}{}
 
-		processRemoveEntry(
+		rmvHeights := &entry.removeCommitHeights
-			entry, ourBalance, theirBalance, nextHeight,
+		rmvHeight := rmvHeights.GetForParty(whoseCommitChain)
-			whoseCommitChain, false, mutateState,
+		if rmvHeight == 0 {
-		)
+			processRemoveEntry(
+				entry, ourBalance, theirBalance, false,
+			)
+
+			if mutateState {
+				rmvHeights.SetForParty(
+					whoseCommitChain, nextHeight,
+				)
+			}
+		}
 	}
 
 	// Next we take a second pass through all the log entries, skipping any
@@ -3008,10 +3026,24 @@ func (lc *LightningChannel) evaluateHTLCView(view *HtlcView, ourBalance,
 			continue
 		}
 
-		processAddEntry(
+		// Skip the entries that have already had their add commit
-			entry, ourBalance, theirBalance, nextHeight,
+		// height set for this commit chain.
-			whoseCommitChain, false, mutateState,
+		addHeights := &entry.addCommitHeights
-		)
+		addHeight := addHeights.GetForParty(whoseCommitChain)
+		if addHeight == 0 {
+			processAddEntry(
+				entry, ourBalance, theirBalance, false,
+			)
+
+			// If we are mutating the state, then set the add
+			// height for the appropriate commitment chain to the
+			// next height.
+			if mutateState {
+				addHeights.SetForParty(
+					whoseCommitChain, nextHeight,
+				)
+			}
+		}
 
 		newView.OurUpdates = append(newView.OurUpdates, entry)
 	}
@@ -3021,10 +3053,24 @@ func (lc *LightningChannel) evaluateHTLCView(view *HtlcView, ourBalance,
 			continue
 		}
 
-		processAddEntry(
+		// Skip the entries that have already had their add commit
-			entry, ourBalance, theirBalance, nextHeight,
+		// height set for this commit chain.
-			whoseCommitChain, true, mutateState,
+		addHeights := &entry.addCommitHeights
-		)
+		addHeight := addHeights.GetForParty(whoseCommitChain)
+		if addHeight == 0 {
+			processAddEntry(
+				entry, ourBalance, theirBalance, true,
+			)
+
+			// If we are mutating the state, then set the add
+			// height for the appropriate commitment chain to the
+			// next height.
+			if mutateState {
+				addHeights.SetForParty(
+					whoseCommitChain, nextHeight,
+				)
+			}
+		}
 
 		newView.TheirUpdates = append(newView.TheirUpdates, entry)
 	}
@@ -3081,23 +3127,7 @@ func (lc *LightningChannel) fetchParent(entry *paymentDescriptor,
 // was committed is updated. Keeping track of this inclusion height allows us to
 // later compact the log once the change is fully committed in both chains.
 func processAddEntry(htlc *paymentDescriptor, ourBalance,
-	theirBalance *lnwire.MilliSatoshi, nextHeight uint64,
+	theirBalance *lnwire.MilliSatoshi, isIncoming bool) {
-	whoseCommitChain lntypes.ChannelParty, isIncoming, mutateState bool) {
-
-	// If we're evaluating this entry for the remote chain (to create/view
-	// a new commitment), then we'll may be updating the height this entry
-	// was added to the chain. Otherwise, we may be updating the entry's
-	// height w.r.t the local chain.
-	var addHeight *uint64
-	if whoseCommitChain.IsRemote() {
-		addHeight = &htlc.addCommitHeights.Remote
-	} else {
-		addHeight = &htlc.addCommitHeights.Local
-	}
-
-	if *addHeight != 0 {
-		return
-	}
 
 	if isIncoming {
 		// If this is a new incoming (un-committed) HTLC, then we need
@@ -3109,30 +3139,13 @@ func processAddEntry(htlc *paymentDescriptor, ourBalance,
 		// going HTLC to reflect the pending balance.
 		*ourBalance -= htlc.Amount
 	}
-
-	if mutateState {
-		*addHeight = nextHeight
-	}
 }
 
 // processRemoveEntry processes a log entry which settles or times out a
 // previously added HTLC. If the removal entry has already been processed, it
 // is skipped.
 func processRemoveEntry(htlc *paymentDescriptor, ourBalance,
-	theirBalance *lnwire.MilliSatoshi, nextHeight uint64,
+	theirBalance *lnwire.MilliSatoshi, isIncoming bool) {
-	whoseCommitChain lntypes.ChannelParty, isIncoming, mutateState bool) {
-
-	var removeHeight *uint64
-	if whoseCommitChain.IsRemote() {
-		removeHeight = &htlc.removeCommitHeights.Remote
-	} else {
-		removeHeight = &htlc.removeCommitHeights.Local
-	}
-
-	// Ignore any removal entries which have already been processed.
-	if *removeHeight != 0 {
-		return
-	}
 
 	switch {
 	// If an incoming HTLC is being settled, then this means that we've
@@ -3159,10 +3172,6 @@ func processRemoveEntry(htlc *paymentDescriptor, ourBalance,
 	case !isIncoming && (htlc.EntryType == Fail || htlc.EntryType == MalformedFail):
 		*ourBalance += htlc.Amount
 	}
-
-	if mutateState {
-		*removeHeight = nextHeight
-	}
 }
 
 // processFeeUpdate processes a log update that updates the current commitment

lnwallet/channel_test.go

@@ -9613,15 +9613,25 @@ func TestProcessAddRemoveEntry(t *testing.T) {
 			// update type. Process remove is used for settles,
 			// fails and malformed htlcs.
 			process := processRemoveEntry
+			heightDual := &update.removeCommitHeights
 			if test.updateType == Add {
 				process = processAddEntry
+				heightDual = &update.addCommitHeights
 			}
 
-			process(
+			if heightDual.GetForParty(test.whoseCommitChain) == 0 {
-				update, &ourBalance, &theirBalance, nextHeight,
+				process(
-				test.whoseCommitChain, test.isIncoming,
+					update, &ourBalance, &theirBalance,
-				test.mutateState,
+					test.isIncoming,
-			)
+				)
+
+				if test.mutateState {
+					heightDual.SetForParty(
+						test.whoseCommitChain,
+						nextHeight,
+					)
+				}
+			}
 
 			// Check that balances were updated as expected.
 			if ourBalance != test.ourExpectedBalance {