Merge pull request #99 from carlaKC/97-logoutgoingnotfound

process: allow outgoing channel not found for failed htlcs

peer_controller.go

@@ -88,7 +88,7 @@ type peerInterceptEvent struct {
 
 type peerResolvedEvent struct {
 	resolvedEvent
-	outgoingPeer route.Vertex
+	outgoingPeer *route.Vertex
 }
 
 type peerState struct {
@@ -433,6 +433,13 @@ func (p *peerController) markHtlcComplete(ctx context.Context, key circuitKey,
 		return
 	}
 
+	// If we couldn't look up an outgoing peer for the HTLC, either:
+	// 1. The outgoing channel never existed (since this is not validated on intercept)
+	// 2. The outgoing channel is pending close at time of resolution (edge case)
+	if resolution.outgoingPeer == nil {
+		return
+	}
+
 	// Track available HTLC information and report to handler.
 	htlcInfo := &HtlcInfo{
 		addTime:         inFlight.addedTs,
@@ -443,7 +450,7 @@ func (p *peerController) markHtlcComplete(ctx context.Context, key circuitKey,
 		incomingCircuit: key,
 		outgoingCircuit: resolution.outgoingCircuitKey,
 		incomingPeer:    p.pubKey,
-		outgoingPeer:    resolution.outgoingPeer,
+		outgoingPeer:    *resolution.outgoingPeer,
 	}
 
 	if err := p.htlcCompleted(ctx, htlcInfo); err != nil {

process.go

@@ -337,18 +337,35 @@ func (p *process) eventLoop(ctx context.Context, group *errgroup.Group) error {
 
 			ctrl := p.getPeerController(ctx, chanInfo.peer, group.Go)
 
-			// Lookup the outgoing peer to supplement the
-			// information on the resolved event.
+			// Lookup the outgoing peer to supplement the information on the
+			// resolved event. Here we handle a channel lookup error
+			// differently to the incoming channel, because it's possible
+			// we were forwarded a HTLC with a bogus outgoing channel. If
+			// this is the case, LND would have failed the HTLC back even if
+			// we let it through. We catch and log that error, rather than
+			// exiting like we do with incoming channels (where we reasonably
+			// expect to find the channel). We still enforce channel lookup
+			// for successful HTLCs, because then we know that the channel
+			// does exist and should be found.
+			var outgoingPeer *route.Vertex
 			chanInfo, err = p.getChanInfo(
 				resolvedEvent.outgoingCircuitKey.channel,
 			)
-			if err != nil {
+			switch {
+			case errors.Is(err, errChannelNotFound) && !resolvedEvent.settled:
+				log.Debugf("Channel not found for failed htlc: %v",
+					resolvedEvent.outgoingCircuitKey.channel)
+
+			case err != nil:
 				return err
+
+			default:
+				outgoingPeer = &chanInfo.peer
 			}
 
 			if err := ctrl.resolved(ctx, peerResolvedEvent{
 				resolvedEvent: resolvedEvent,
-				outgoingPeer:  chanInfo.peer,
+				outgoingPeer:  outgoingPeer,
 			}); err != nil {
 				return err
 			}

process_test.go

@@ -408,6 +408,115 @@ func TestChannelNotFound(t *testing.T) {
 	}
 }
 
+// TestOutgoingChannelNotFound tests the case where the outgoing channel for a htlc is
+// not found in two cases:
+// 1. The HTLC was settled: the channel must exist, so we fail if it's not found
+// 2. The HTLC was failed: the outgoing channel could be bogus, so we handle the error
+func TestOutgoingChannelNotFound(t *testing.T) {
+	tests := []struct {
+		name          string
+		settled       bool
+		outgoingFound bool
+		err           error
+	}{
+		{
+			name:          "outgoing found, settled",
+			settled:       true,
+			outgoingFound: true,
+		},
+		{
+			name:          "outgoing found, not settled",
+			settled:       false,
+			outgoingFound: true,
+		},
+		{
+			name:          "outgoing not found, settled",
+			settled:       true,
+			outgoingFound: false,
+			err:           errChannelNotFound,
+		},
+		{
+			name:          "outgoing not found, not settled",
+			settled:       false,
+			outgoingFound: false,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			testLookupOutgoingChannel(
+				t, test.settled, test.outgoingFound, test.err,
+			)
+		})
+	}
+}
+
+func testLookupOutgoingChannel(t *testing.T, settled, outgoingFound bool,
+	exitErr error) {
+
+	client := newLndclientMock(testChannels, nil)
+
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	db, cleanup := setupTestDb(t, defaultFwdHistoryLimit)
+	defer cleanup()
+
+	log := zaptest.NewLogger(t).Sugar()
+
+	cfg := &Limits{}
+
+	p := NewProcess(client, log, cfg, db)
+
+	resolved := make(chan struct{})
+	p.resolvedCallback = func() {
+		close(resolved)
+	}
+
+	exit := make(chan error)
+	go func() {
+		exit <- p.Run(ctx)
+	}()
+
+	// Send a htlc with a known incoming channel.
+	key := circuitKey{
+		channel: 2,
+		htlc:    5,
+	}
+	client.htlcInterceptorRequests <- &interceptedEvent{
+		circuitKey: key,
+	}
+
+	resp := <-client.htlcInterceptorResponses
+	require.True(t, resp.resume)
+
+	// Set the outgoing channel based on whether we want it to be found by our
+	// lookup or not.
+	outgoingKey := outgoingKey
+	if !outgoingFound {
+		outgoingKey.channel = 9999
+	}
+
+	htlcEvent := &resolvedEvent{
+		incomingCircuitKey: key,
+		outgoingCircuitKey: outgoingKey,
+		settled:            settled,
+	}
+
+	client.htlcEvents <- htlcEvent
+
+	// If expected, assert that we exit with an error, otherwise ensure that the htlc
+	// is settled and we exit cleanly.
+	if exitErr != nil {
+		require.ErrorIs(t, <-exit, exitErr)
+	} else {
+		<-resolved
+
+		cancel()
+		require.ErrorIs(t, <-exit, context.Canceled)
+	}
+}
+
 // TestClosedChannelHtlc tests that we can handle intercepted htlcs that are associated
 // with closed channels.
 func TestClosedChannelHtlc(t *testing.T) {