Merge pull request #3998 from halseth/panic-addentry

Fix TestNewBreachRetributionSkipsDustHtlcs incorrect state transtion

lnwallet/channel.go

@@ -2374,9 +2374,12 @@ func (lc *LightningChannel) fetchCommitmentView(remoteChain bool,
 	// these balances will be *before* taking a commitment fee from the
 	// initiator.
 	htlcView := lc.fetchHTLCView(theirLogIndex, ourLogIndex)
-	ourBalance, theirBalance, _, filteredHTLCView := lc.computeView(
+	ourBalance, theirBalance, _, filteredHTLCView, err := lc.computeView(
 		htlcView, remoteChain, true,
 	)
+	if err != nil {
+		return nil, err
+	}
 	feePerKw := filteredHTLCView.feePerKw
 
 	// Actually generate unsigned commitment transaction for this view.
@@ -2444,7 +2447,7 @@ func fundingTxIn(chanState *channeldb.OpenChannel) wire.TxIn {
 // method.
 func (lc *LightningChannel) evaluateHTLCView(view *htlcView, ourBalance,
 	theirBalance *lnwire.MilliSatoshi, nextHeight uint64,
-	remoteChain, mutateState bool) *htlcView {
+	remoteChain, mutateState bool) (*htlcView, 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
@@ -2460,6 +2463,57 @@ func (lc *LightningChannel) evaluateHTLCView(view *htlcView, ourBalance,
 	skipUs := make(map[uint64]struct{})
 	skipThem := make(map[uint64]struct{})
 
+	// fetchParentEntry is a helper method that will fetch the parent of
+	// entry from the corresponding update log.
+	fetchParentEntry := func(entry *PaymentDescriptor,
+		remoteLog bool) (*PaymentDescriptor, error) {
+
+		var (
+			updateLog *updateLog
+			logName   string
+		)
+
+		if remoteLog {
+			updateLog = lc.remoteUpdateLog
+			logName = "remote"
+		} else {
+			updateLog = lc.localUpdateLog
+			logName = "local"
+		}
+
+		addEntry := updateLog.lookupHtlc(entry.ParentIndex)
+
+		switch {
+		// We check if the parent entry is not found at this point.
+		// This could happen for old versions of lnd, and we return an
+		// error to gracefully shut down the state machine if such an
+		// entry is still in the logs.
+		case addEntry == nil:
+			return nil, fmt.Errorf("unable to find parent entry "+
+				"%d in %v update log: %v\nUpdatelog: %v",
+				entry.ParentIndex, logName,
+				newLogClosure(func() string {
+					return spew.Sdump(entry)
+				}), newLogClosure(func() string {
+					return spew.Sdump(updateLog)
+				}),
+			)
+
+		// The parent add height should never be zero at this point. If
+		// that's the case we probably forgot to send a new commitment.
+		case remoteChain && addEntry.addCommitHeightRemote == 0:
+			return nil, fmt.Errorf("parent entry %d for update %d "+
+				"had zero remote add height", entry.ParentIndex,
+				entry.LogIndex)
+		case !remoteChain && addEntry.addCommitHeightLocal == 0:
+			return nil, fmt.Errorf("parent entry %d for update %d "+
+				"had zero local add height", entry.ParentIndex,
+				entry.LogIndex)
+		}
+
+		return addEntry, nil
+	}
+
 	// First we run through non-add entries in both logs, populating the
 	// skip sets and mutating the current chain state (crediting balances,
 	// etc) to reflect the settle/timeout entry encountered.
@@ -2486,21 +2540,9 @@ func (lc *LightningChannel) evaluateHTLCView(view *htlcView, ourBalance,
 			lc.channelState.TotalMSatReceived += entry.Amount
 		}
 
-		addEntry := lc.remoteUpdateLog.lookupHtlc(entry.ParentIndex)
+		addEntry, err := fetchParentEntry(entry, true)
-
+		if err != nil {
-		// We check if the parent entry is not found at this point. We
+			return nil, err
-		// have seen this happening a few times and panic with some
-		// addtitional info to figure out why.
-		// TODO(halseth): remove when bug is fixed.
-		if addEntry == nil {
-			panic(fmt.Sprintf("unable to find parent entry %d "+
-				"in remote update log: %v\nUpdatelog: %v",
-				entry.ParentIndex, newLogClosure(func() string {
-					return spew.Sdump(entry)
-				}), newLogClosure(func() string {
-					return spew.Sdump(lc.remoteUpdateLog)
-				}),
-			))
 		}
 
 		skipThem[addEntry.HtlcIndex] = struct{}{}
@@ -2531,21 +2573,9 @@ func (lc *LightningChannel) evaluateHTLCView(view *htlcView, ourBalance,
 			lc.channelState.TotalMSatSent += entry.Amount
 		}
 
-		addEntry := lc.localUpdateLog.lookupHtlc(entry.ParentIndex)
+		addEntry, err := fetchParentEntry(entry, false)
-
+		if err != nil {
-		// We check if the parent entry is not found at this point. We
+			return nil, err
-		// have seen this happening a few times and panic with some
-		// addtitional info to figure out why.
-		// TODO(halseth): remove when bug is fixed.
-		if addEntry == nil {
-			panic(fmt.Sprintf("unable to find parent entry %d "+
-				"in local update log: %v\nUpdatelog: %v",
-				entry.ParentIndex, newLogClosure(func() string {
-					return spew.Sdump(entry)
-				}), newLogClosure(func() string {
-					return spew.Sdump(lc.localUpdateLog)
-				}),
-			))
 		}
 
 		skipUs[addEntry.HtlcIndex] = struct{}{}
@@ -2577,7 +2607,7 @@ func (lc *LightningChannel) evaluateHTLCView(view *htlcView, ourBalance,
 		newView.theirUpdates = append(newView.theirUpdates, entry)
 	}
 
-	return newView
+	return newView, nil
 }
 
 // processAddEntry evaluates the effect of an add entry within the HTLC log.
@@ -3099,9 +3129,12 @@ func (lc *LightningChannel) validateCommitmentSanity(theirLogCounter,
 	ourInitialBalance := commitChain.tip().ourBalance
 	theirInitialBalance := commitChain.tip().theirBalance
 
-	ourBalance, theirBalance, commitWeight, filteredView := lc.computeView(
+	ourBalance, theirBalance, commitWeight, filteredView, err := lc.computeView(
 		view, remoteChain, false,
 	)
+	if err != nil {
+		return err
+	}
 	feePerKw := filteredView.feePerKw
 
 	// Calculate the commitment fee, and subtract it from the initiator's
@@ -3192,7 +3225,7 @@ func (lc *LightningChannel) validateCommitmentSanity(theirLogCounter,
 
 	// First check that the remote updates won't violate it's channel
 	// constraints.
-	err := validateUpdates(
+	err = validateUpdates(
 		filteredView.theirUpdates, &lc.channelState.RemoteChanCfg,
 	)
 	if err != nil {
@@ -3688,7 +3721,7 @@ func (lc *LightningChannel) ProcessChanSyncMsg(
 // 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) {
+	*htlcView, error) {
 
 	commitChain := lc.localCommitChain
 	dustLimit := lc.channelState.LocalChanCfg.DustLimit
@@ -3727,8 +3760,11 @@ func (lc *LightningChannel) computeView(view *htlcView, remoteChain bool,
 	// 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 := lc.evaluateHTLCView(view, &ourBalance,
+	filteredHTLCView, err := lc.evaluateHTLCView(view, &ourBalance,
 		&theirBalance, nextHeight, remoteChain, updateState)
+	if err != nil {
+		return 0, 0, 0, nil, err
+	}
 	feePerKw := filteredHTLCView.feePerKw
 
 	// Now go through all HTLCs at this stage, to calculate the total
@@ -3752,7 +3788,7 @@ func (lc *LightningChannel) computeView(view *htlcView, remoteChain bool,
 	}
 
 	totalCommitWeight := input.CommitWeight + totalHtlcWeight
-	return ourBalance, theirBalance, totalCommitWeight, filteredHTLCView
+	return ourBalance, theirBalance, totalCommitWeight, filteredHTLCView, nil
 }
 
 // genHtlcSigValidationJobs generates a series of signatures verification jobs
@@ -5957,8 +5993,12 @@ func (lc *LightningChannel) availableBalance() (lnwire.MilliSatoshi, int64) {
 		lc.localUpdateLog.logIndex)
 
 	// Then compute our current balance for that view.
-	ourBalance, _, commitWeight, filteredView :=
+	ourBalance, _, commitWeight, filteredView, err :=
 		lc.computeView(htlcView, false, false)
+	if err != nil {
+		lc.log.Errorf("Unable to fetch available balance: %v", err)
+		return 0, 0
+	}
 
 	// If we are the channel initiator, we must remember to subtract the
 	// commitment fee from our available balance.

lnwallet/channel_test.go

@@ -5922,8 +5922,8 @@ func TestNewBreachRetributionSkipsDustHtlcs(t *testing.T) {
 
 	// With the HTLC's applied to both update logs, we'll initiate a state
 	// transition from Alice.
-	if err := ForceStateTransition(bobChannel, aliceChannel); err != nil {
+	if err := ForceStateTransition(aliceChannel, bobChannel); err != nil {
-		t.Fatalf("unable to complete bob's state transition: %v", err)
+		t.Fatalf("unable to complete alice's state transition: %v", err)
 	}
 
 	// At this point, we'll capture the current state number, as well as