diff --git a/contractcourt/htlc_incoming_resolver_test.go b/contractcourt/htlc_incoming_resolver_test.go index ef5277f4..cce6343b 100644 --- a/contractcourt/htlc_incoming_resolver_test.go +++ b/contractcourt/htlc_incoming_resolver_test.go @@ -99,6 +99,7 @@ func TestHtlcIncomingResolverExitSettle(t *testing.T) { ctx := newIncomingResolverTestContext(t) ctx.registry.notifyResolution = invoices.NewSettleResolution( testResPreimage, testResCircuitKey, testAcceptHeight, + invoices.ResultReplayToSettled, ) ctx.resolve() @@ -129,6 +130,7 @@ func TestHtlcIncomingResolverExitCancel(t *testing.T) { ctx := newIncomingResolverTestContext(t) ctx.registry.notifyResolution = invoices.NewFailureResolution( testResCircuitKey, testAcceptHeight, + invoices.ResultInvoiceAlreadyCanceled, ) ctx.resolve() @@ -147,6 +149,7 @@ func TestHtlcIncomingResolverExitSettleHodl(t *testing.T) { notifyData := <-ctx.registry.notifyChan notifyData.hodlChan <- *invoices.NewSettleResolution( testResPreimage, testResCircuitKey, testAcceptHeight, + invoices.ResultSettled, ) ctx.waitForResult(true) @@ -174,7 +177,7 @@ func TestHtlcIncomingResolverExitCancelHodl(t *testing.T) { ctx.resolve() notifyData := <-ctx.registry.notifyChan notifyData.hodlChan <- *invoices.NewFailureResolution( - testResCircuitKey, testAcceptHeight, + testResCircuitKey, testAcceptHeight, invoices.ResultCanceled, ) ctx.waitForResult(false) diff --git a/htlcswitch/link.go b/htlcswitch/link.go index a1913381..596b13ee 100644 --- a/htlcswitch/link.go +++ b/htlcswitch/link.go @@ -1195,7 +1195,8 @@ func (l *channelLink) processHtlcResolution(resolution invoices.HtlcResolution, circuitKey := resolution.CircuitKey - // Determine required action for the resolution. + // Determine required action for the resolution. If the event's preimage is + // non-nil, the htlc must be settled. Otherwise, it should be canceled. if resolution.Preimage != nil { l.log.Debugf("received settle resolution for %v", circuitKey) @@ -1205,10 +1206,12 @@ func (l *channelLink) processHtlcResolution(resolution invoices.HtlcResolution, ) } - l.log.Debugf("received cancel resolution for %v", circuitKey) + l.log.Debugf("received cancel resolution for %v with outcome: %v", + circuitKey, resolution.Outcome) - // In case of a cancel, always return - // incorrect_or_unknown_payment_details in order to avoid leaking info. + // The htlc has failed so we cancel it with FailIncorrectDetails. This + // error covers invoice failures and hodl cancels (which return it to avoid + // leaking information). failure := lnwire.NewFailIncorrectDetails( htlc.pd.Amount, uint32(resolution.AcceptHeight), ) @@ -2844,10 +2847,10 @@ func (l *channelLink) processExitHop(pd *lnwallet.PaymentDescriptor, obfuscator: obfuscator, } + // If the event is nil, the invoice is being held, so we save payment + // descriptor for future reference. if event == nil { - // Save payment descriptor for future reference. l.hodlMap[circuitKey] = htlc - return nil } diff --git a/invoices/invoiceregistry.go b/invoices/invoiceregistry.go index a65799c5..0ee0364c 100644 --- a/invoices/invoiceregistry.go +++ b/invoices/invoiceregistry.go @@ -48,27 +48,32 @@ type HtlcResolution struct { // AcceptHeight is the original height at which the htlc was accepted. AcceptHeight int32 + + // Outcome indicates the outcome of the invoice registry update. + Outcome ResolutionResult } // NewFailureResolution returns a htlc failure resolution. func NewFailureResolution(key channeldb.CircuitKey, - acceptHeight int32) *HtlcResolution { + acceptHeight int32, outcome ResolutionResult) *HtlcResolution { return &HtlcResolution{ CircuitKey: key, AcceptHeight: acceptHeight, + Outcome: outcome, } } // NewSettleResolution returns a htlc resolution which is associated with a // settle. func NewSettleResolution(preimage lntypes.Preimage, key channeldb.CircuitKey, - acceptHeight int32) *HtlcResolution { + acceptHeight int32, outcome ResolutionResult) *HtlcResolution { return &HtlcResolution{ Preimage: &preimage, CircuitKey: key, AcceptHeight: acceptHeight, + Outcome: outcome, } } @@ -347,7 +352,7 @@ func (i *InvoiceRegistry) invoiceEventLoop() { case <-nextReleaseTick: event := autoReleaseHeap.Pop().(*htlcReleaseEvent) err := i.cancelSingleHtlc( - event.hash, event.key, + event.hash, event.key, ResultMppTimeout, ) if err != nil { log.Errorf("HTLC timer: %v", err) @@ -596,9 +601,11 @@ func (i *InvoiceRegistry) startHtlcTimer(hash lntypes.Hash, } } -// cancelSingleHtlc cancels a single accepted htlc on an invoice. +// cancelSingleHtlc cancels a single accepted htlc on an invoice. It takes +// a resolution result which will be used to notify subscribed links and +// resolvers of the details of the htlc cancellation. func (i *InvoiceRegistry) cancelSingleHtlc(hash lntypes.Hash, - key channeldb.CircuitKey) error { + key channeldb.CircuitKey, result ResolutionResult) error { i.Lock() defer i.Unlock() @@ -674,9 +681,11 @@ func (i *InvoiceRegistry) cancelSingleHtlc(hash lntypes.Hash, return fmt.Errorf("htlc %v not found", key) } if htlc.State == channeldb.HtlcStateCanceled { - i.notifyHodlSubscribers( - *NewFailureResolution(key, int32(htlc.AcceptHeight)), + resolution := *NewFailureResolution( + key, int32(htlc.AcceptHeight), result, ) + + i.notifyHodlSubscribers(resolution) } return nil } @@ -765,7 +774,9 @@ func (i *InvoiceRegistry) NotifyExitHopHtlc(rHash lntypes.Hash, // If it isn't recorded, cancel htlc. if !ok { - return NewFailureResolution(circuitKey, currentHeight), nil + return NewFailureResolution( + circuitKey, currentHeight, result, + ), nil } // Determine accepted height of this htlc. If the htlc reached the @@ -776,7 +787,9 @@ func (i *InvoiceRegistry) NotifyExitHopHtlc(rHash lntypes.Hash, switch invoiceHtlc.State { case channeldb.HtlcStateCanceled: - return NewFailureResolution(circuitKey, acceptHeight), nil + return NewFailureResolution( + circuitKey, acceptHeight, result, + ), nil case channeldb.HtlcStateSettled: // Also settle any previously accepted htlcs. The invoice state @@ -787,17 +800,24 @@ func (i *InvoiceRegistry) NotifyExitHopHtlc(rHash lntypes.Hash, continue } + // Notify subscribers that the htlcs should be settled + // with our peer. Note that the outcome of the + // resolution is set based on the outcome of the single + // htlc that we just settled, so may not be accurate + // for all htlcs. resolution := *NewSettleResolution( invoice.Terms.PaymentPreimage, key, - acceptHeight, + acceptHeight, result, ) i.notifyHodlSubscribers(resolution) } - return NewSettleResolution( - invoice.Terms.PaymentPreimage, circuitKey, acceptHeight, - ), nil + resolution := NewSettleResolution( + invoice.Terms.PaymentPreimage, circuitKey, + acceptHeight, result, + ) + return resolution, nil case channeldb.HtlcStateAccepted: // (Re)start the htlc timer if the invoice is still open. It can @@ -870,7 +890,7 @@ func (i *InvoiceRegistry) SettleHodlInvoice(preimage lntypes.Preimage) error { } resolution := *NewSettleResolution( - preimage, key, int32(htlc.AcceptHeight), + preimage, key, int32(htlc.AcceptHeight), ResultSettled, ) i.notifyHodlSubscribers(resolution) @@ -888,7 +908,8 @@ func (i *InvoiceRegistry) CancelInvoice(payHash lntypes.Hash) error { // cancelInvoice attempts to cancel the invoice corresponding to the passed // payment hash. Accepted invoices will only be canceled if explicitly -// requested to do so. +// requested to do so. It notifies subscribing links and resolvers that +// the associated htlcs were canceled if they change state. func (i *InvoiceRegistry) cancelInvoiceImpl(payHash lntypes.Hash, cancelAccepted bool) error { @@ -948,7 +969,9 @@ func (i *InvoiceRegistry) cancelInvoiceImpl(payHash lntypes.Hash, } i.notifyHodlSubscribers( - *NewFailureResolution(key, int32(htlc.AcceptHeight)), + *NewFailureResolution( + key, int32(htlc.AcceptHeight), ResultCanceled, + ), ) } i.notifyClients(payHash, invoice, channeldb.ContractCanceled) diff --git a/invoices/invoiceregistry_test.go b/invoices/invoiceregistry_test.go index 07e22823..27a63c25 100644 --- a/invoices/invoiceregistry_test.go +++ b/invoices/invoiceregistry_test.go @@ -81,16 +81,23 @@ func TestSettleInvoice(t *testing.T) { t.Fatalf("expected acceptHeight %v, but got %v", testCurrentHeight, resolution.AcceptHeight) } + if resolution.Outcome != ResultExpiryTooSoon { + t.Fatalf("expected expiry too soon, got: %v", + resolution.Outcome) + } // Settle invoice with a slightly higher amount. amtPaid := lnwire.MilliSatoshi(100500) - _, err = ctx.registry.NotifyExitHopHtlc( + resolution, err = ctx.registry.NotifyExitHopHtlc( testInvoicePaymentHash, amtPaid, testHtlcExpiry, testCurrentHeight, getCircuitKey(0), hodlChan, testPayload, ) if err != nil { t.Fatal(err) } + if resolution.Outcome != ResultSettled { + t.Fatalf("expected settled, got: %v", resolution.Outcome) + } // We expect the settled state to be sent to the single invoice // subscriber. @@ -130,6 +137,10 @@ func TestSettleInvoice(t *testing.T) { if resolution.Preimage == nil { t.Fatal("expected settle resolution") } + if resolution.Outcome != ResultReplayToSettled { + t.Fatalf("expected replay settled, got: %v", + resolution.Outcome) + } // Try to settle again with a new higher-valued htlc. This payment // should also be accepted, to prevent any change in behaviour for a @@ -144,6 +155,10 @@ func TestSettleInvoice(t *testing.T) { if resolution.Preimage == nil { t.Fatal("expected settle resolution") } + if resolution.Outcome != ResultDuplicateToSettled { + t.Fatalf("expected duplicate settled, got: %v", + resolution.Outcome) + } // Try to settle again with a lower amount. This should fail just as it // would have failed if it were the first payment. @@ -157,6 +172,10 @@ func TestSettleInvoice(t *testing.T) { if resolution.Preimage != nil { t.Fatal("expected cancel resolution") } + if resolution.Outcome != ResultAmountTooLow { + t.Fatalf("expected amount too low, got: %v", + resolution.Outcome) + } // Check that settled amount is equal to the sum of values of the htlcs // 0 and 1. @@ -287,6 +306,10 @@ func TestCancelInvoice(t *testing.T) { t.Fatalf("expected acceptHeight %v, but got %v", testCurrentHeight, resolution.AcceptHeight) } + if resolution.Outcome != ResultInvoiceAlreadyCanceled { + t.Fatalf("expected invoice already canceled, got: %v", + resolution.Outcome) + } } // TestSettleHoldInvoice tests settling of a hold invoice and related @@ -402,6 +425,10 @@ func TestSettleHoldInvoice(t *testing.T) { if resolution == nil || resolution.Preimage != nil { t.Fatalf("expected htlc to be canceled") } + if resolution.Outcome != ResultExpiryTooSoon { + t.Fatalf("expected expiry too soon, got: %v", + resolution.Outcome) + } // We expect the accepted state to be sent to the single invoice // subscriber. For all invoice subscribers, we don't expect an update. @@ -429,6 +456,10 @@ func TestSettleHoldInvoice(t *testing.T) { t.Fatalf("expected acceptHeight %v, but got %v", testCurrentHeight, resolution.AcceptHeight) } + if htlcResolution.Outcome != ResultSettled { + t.Fatalf("expected result settled, got: %v", + htlcResolution.Outcome) + } // We expect a settled notification to be sent out for both all and // single invoice subscribers. @@ -535,6 +566,10 @@ func TestCancelHoldInvoice(t *testing.T) { t.Fatalf("expected acceptHeight %v, but got %v", testCurrentHeight, resolution.AcceptHeight) } + if resolution.Outcome != ResultReplayToCanceled { + t.Fatalf("expected replay to canceled, got %v", + resolution.Outcome) + } } // TestUnknownInvoice tests that invoice registry returns an error when the @@ -559,8 +594,10 @@ func TestUnknownInvoice(t *testing.T) { } } -// TestSettleMpp tests settling of an invoice with multiple partial payments. -func TestSettleMpp(t *testing.T) { +// TestMppPayment tests settling of an invoice with multiple partial payments. +// It covers the case where there is a mpp timeout before the whole invoice is +// paid and the case where the invoice is settled in time. +func TestMppPayment(t *testing.T) { defer timeout()() ctx := newTestContext(t) @@ -597,6 +634,10 @@ func TestSettleMpp(t *testing.T) { if htlcResolution.Preimage != nil { t.Fatal("expected cancel resolution") } + if htlcResolution.Outcome != ResultMppTimeout { + t.Fatalf("expected mpp timeout, got: %v", + htlcResolution.Outcome) + } // Send htlc 2. hodlChan2 := make(chan interface{}, 1) @@ -625,9 +666,13 @@ func TestSettleMpp(t *testing.T) { if resolution == nil { t.Fatal("expected a settle resolution") } + if resolution.Outcome != ResultSettled { + t.Fatalf("expected result settled, got: %v", + resolution.Outcome) + } // Check that settled amount is equal to the sum of values of the htlcs - // 0 and 1. + // 2 and 3. inv, err := ctx.registry.LookupInvoice(testInvoicePaymentHash) if err != nil { t.Fatal(err) diff --git a/invoices/update.go b/invoices/update.go index 6cf58d0d..fcf7a8e1 100644 --- a/invoices/update.go +++ b/invoices/update.go @@ -49,6 +49,10 @@ const ( // ResultSettled is returned when we settle an invoice. ResultSettled + // ResultCanceled is returned when we cancel an invoice and its associated + // htlcs. + ResultCanceled + // ResultInvoiceNotOpen is returned when a mpp invoice is not open. ResultInvoiceNotOpen @@ -59,6 +63,10 @@ const ( // ResultMppInProgress is returned when we are busy receiving a mpp payment. ResultMppInProgress + // ResultMppTimeout is returned when an invoice paid with multiple partial + // payments times out before it is fully paid. + ResultMppTimeout + // ResultAddressMismatch is returned when the payment address for a mpp // invoice does not match. ResultAddressMismatch