From 7b5dda0417679bd9508afc651b713c958ddff603 Mon Sep 17 00:00:00 2001 From: carla Date: Fri, 20 Dec 2019 12:25:07 +0200 Subject: [PATCH] invoices: add resolution result to htlcResolution This commit adds the resolution result obtained while updating an invoice in the registry to htlcResolution. The field can be used by calling functions to determine the outcome of the update and act appropriately. --- contractcourt/htlc_incoming_resolver_test.go | 5 +- htlcswitch/link.go | 15 +++--- invoices/invoiceregistry.go | 55 ++++++++++++++------ invoices/invoiceregistry_test.go | 53 +++++++++++++++++-- invoices/update.go | 8 +++ 5 files changed, 109 insertions(+), 27 deletions(-) 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