diff --git a/peer_controller.go b/peer_controller.go index c52490a..184f8a2 100644 --- a/peer_controller.go +++ b/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 { diff --git a/process.go b/process.go index a88ad14..8643a5a 100644 --- a/process.go +++ b/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 } diff --git a/process_test.go b/process_test.go index 3706470..bc9b56b 100644 --- a/process_test.go +++ b/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) {