From 7d140d57b91a45fb8851534c6128a3e344a5a84e Mon Sep 17 00:00:00 2001 From: Carla Kirk-Cohen Date: Mon, 27 Nov 2023 09:53:17 +0000 Subject: [PATCH] process: allow outgoing channel not found for failed htlcs Update error handling for outgoing channel not found to catch the case where an outgoing channel was not found for a failed HTLC. Unlike incoming HTLCs, where the HTLC arrived on the channel so we know it exists, we have not yet performed any existence validation on the outgoing channel (because interception happens before we check that it exists). Since we only need the outgoing channel for record keeping, we just log the case where a HTLC was failed back and we don't know the channel (since this is just a bogus channel). We don't store this HTLC in the DB, as it will be instantly failed back. --- peer_controller.go | 11 ++++- process.go | 25 +++++++++-- process_test.go | 109 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 139 insertions(+), 6 deletions(-) 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) {