From a339065fdcec38be273bf75431757f7de1c1872c Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Tue, 4 Feb 2020 15:15:48 +0100 Subject: [PATCH 1/3] invoices: add hash to update context --- invoices/invoiceregistry.go | 10 +++++----- invoices/update.go | 2 ++ 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/invoices/invoiceregistry.go b/invoices/invoiceregistry.go index 4b3a1b2d..9ab1a65b 100644 --- a/invoices/invoiceregistry.go +++ b/invoices/invoiceregistry.go @@ -693,8 +693,7 @@ func (i *InvoiceRegistry) cancelSingleHtlc(hash lntypes.Hash, // processKeySend just-in-time inserts an invoice if this htlc is a keysend // htlc. -func (i *InvoiceRegistry) processKeySend(ctx invoiceUpdateCtx, - hash lntypes.Hash) error { +func (i *InvoiceRegistry) processKeySend(ctx invoiceUpdateCtx) error { // Retrieve keysend record if present. preimageSlice, ok := ctx.customRecords[record.KeySendType] @@ -704,7 +703,7 @@ func (i *InvoiceRegistry) processKeySend(ctx invoiceUpdateCtx, // Cancel htlc is preimage is invalid. preimage, err := lntypes.MakePreimage(preimageSlice) - if err != nil || preimage.Hash() != hash { + if err != nil || preimage.Hash() != ctx.hash { return errors.New("invalid keysend preimage") } @@ -751,7 +750,7 @@ func (i *InvoiceRegistry) processKeySend(ctx invoiceUpdateCtx, // Insert invoice into database. Ignore duplicates, because this // may be a replay. - _, err = i.AddInvoice(invoice, hash) + _, err = i.AddInvoice(invoice, ctx.hash) if err != nil && err != channeldb.ErrDuplicateInvoice { return err } @@ -789,6 +788,7 @@ func (i *InvoiceRegistry) NotifyExitHopHtlc(rHash lntypes.Hash, // Create the update context containing the relevant details of the // incoming htlc. updateCtx := invoiceUpdateCtx{ + hash: rHash, circuitKey: circuitKey, amtPaid: amtPaid, expiry: expiry, @@ -802,7 +802,7 @@ func (i *InvoiceRegistry) NotifyExitHopHtlc(rHash lntypes.Hash, // AddInvoice obtains its own lock. This is no problem, because the // operation is idempotent. if i.cfg.AcceptKeySend { - err := i.processKeySend(updateCtx, rHash) + err := i.processKeySend(updateCtx) if err != nil { debugLog(fmt.Sprintf("keysend error: %v", err)) diff --git a/invoices/update.go b/invoices/update.go index 57a41de9..2e05b398 100644 --- a/invoices/update.go +++ b/invoices/update.go @@ -4,6 +4,7 @@ import ( "errors" "github.com/lightningnetwork/lnd/channeldb" + "github.com/lightningnetwork/lnd/lntypes" "github.com/lightningnetwork/lnd/lnwire" "github.com/lightningnetwork/lnd/record" ) @@ -169,6 +170,7 @@ func (u ResolutionResult) String() string { // invoiceUpdateCtx is an object that describes the context for the invoice // update to be carried out. type invoiceUpdateCtx struct { + hash lntypes.Hash circuitKey channeldb.CircuitKey amtPaid lnwire.MilliSatoshi expiry uint32 From 51324ac7ae49c84df2ab751bc12c33d7a5a769ff Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Tue, 4 Feb 2020 15:17:54 +0100 Subject: [PATCH 2/3] invoices: move log into update context --- invoices/invoiceregistry.go | 11 +++-------- invoices/update.go | 6 ++++++ 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/invoices/invoiceregistry.go b/invoices/invoiceregistry.go index 9ab1a65b..e83a21b6 100644 --- a/invoices/invoiceregistry.go +++ b/invoices/invoiceregistry.go @@ -780,11 +780,6 @@ func (i *InvoiceRegistry) NotifyExitHopHtlc(rHash lntypes.Hash, mpp := payload.MultiPath() - debugLog := func(s string) { - log.Debugf("Invoice(%x): %v, amt=%v, expiry=%v, circuit=%v, "+ - "mpp=%v", rHash[:], s, amtPaid, expiry, circuitKey, mpp) - } - // Create the update context containing the relevant details of the // incoming htlc. updateCtx := invoiceUpdateCtx{ @@ -804,7 +799,7 @@ func (i *InvoiceRegistry) NotifyExitHopHtlc(rHash lntypes.Hash, if i.cfg.AcceptKeySend { err := i.processKeySend(updateCtx) if err != nil { - debugLog(fmt.Sprintf("keysend error: %v", err)) + updateCtx.log(fmt.Sprintf("keysend error: %v", err)) return NewFailureResolution( circuitKey, currentHeight, ResultKeySendError, @@ -852,11 +847,11 @@ func (i *InvoiceRegistry) NotifyExitHopHtlc(rHash lntypes.Hash, case nil: default: - debugLog(err.Error()) + updateCtx.log(err.Error()) return nil, err } - debugLog(result.String()) + updateCtx.log(result.String()) if updateSubscribers { i.notifyClients(rHash, invoice, invoice.State) diff --git a/invoices/update.go b/invoices/update.go index 2e05b398..e6feaba0 100644 --- a/invoices/update.go +++ b/invoices/update.go @@ -180,6 +180,12 @@ type invoiceUpdateCtx struct { mpp *record.MPP } +// log logs a message specific to this update context. +func (i *invoiceUpdateCtx) log(s string) { + log.Debugf("Invoice(%x): %v, amt=%v, expiry=%v, circuit=%v, mpp=%v", + i.hash[:], s, i.amtPaid, i.expiry, i.circuitKey, i.mpp) +} + // updateInvoice is a callback for DB.UpdateInvoice that contains the invoice // settlement logic. func updateInvoice(ctx *invoiceUpdateCtx, inv *channeldb.Invoice) ( From 0042a1ffeb8812417f467dbb32b14c61117ba062 Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Tue, 4 Feb 2020 10:07:56 +0100 Subject: [PATCH 3/3] invoices: fix htlc timer deadlock --- invoices/invoiceregistry.go | 86 ++++++++++++++++++++++++++++--------- 1 file changed, 65 insertions(+), 21 deletions(-) diff --git a/invoices/invoiceregistry.go b/invoices/invoiceregistry.go index e83a21b6..d7ac13f7 100644 --- a/invoices/invoiceregistry.go +++ b/invoices/invoiceregistry.go @@ -807,8 +807,53 @@ func (i *InvoiceRegistry) NotifyExitHopHtlc(rHash lntypes.Hash, } } + // Execute locked notify exit hop logic. i.Lock() - defer i.Unlock() + resolution, err := i.notifyExitHopHtlcLocked(&updateCtx, hodlChan) + i.Unlock() + if err != nil { + return nil, err + } + + switch r := resolution.(type) { + + // A direct resolution was received for this htlc. + case *HtlcResolution: + return r, nil + + // The htlc is held. Start a timer outside the lock if the htlc should + // be auto-released, because otherwise a deadlock may happen with the + // main event loop. + case *acceptResolution: + if r.autoRelease { + err := i.startHtlcTimer(rHash, circuitKey, r.acceptTime) + if err != nil { + return nil, err + } + } + + return nil, nil + + default: + return nil, errors.New("invalid resolution type") + } +} + +// acceptResolution is returned when the htlc should be held. +type acceptResolution struct { + // autoRelease signals that the htlc should be automatically released + // after a timeout. + autoRelease bool + + // acceptTime is the time at which this htlc was accepted. + acceptTime time.Time +} + +// notifyExitHopHtlcLocked is the internal implementation of NotifyExitHopHtlc +// that should be executed inside the registry lock. +func (i *InvoiceRegistry) notifyExitHopHtlcLocked( + ctx *invoiceUpdateCtx, hodlChan chan<- interface{}) ( + interface{}, error) { // We'll attempt to settle an invoice matching this rHash on disk (if // one exists). The callback will update the invoice state and/or htlcs. @@ -817,11 +862,11 @@ func (i *InvoiceRegistry) NotifyExitHopHtlc(rHash lntypes.Hash, updateSubscribers bool ) invoice, err := i.cdb.UpdateInvoice( - rHash, + ctx.hash, func(inv *channeldb.Invoice) ( *channeldb.InvoiceUpdateDesc, error) { - updateDesc, res, err := updateInvoice(&updateCtx, inv) + updateDesc, res, err := updateInvoice(ctx, inv) if err != nil { return nil, err } @@ -841,29 +886,30 @@ func (i *InvoiceRegistry) NotifyExitHopHtlc(rHash lntypes.Hash, // If the invoice was not found, return a failure resolution // with an invoice not found result. return NewFailureResolution( - circuitKey, currentHeight, ResultInvoiceNotFound, + ctx.circuitKey, ctx.currentHeight, + ResultInvoiceNotFound, ), nil case nil: default: - updateCtx.log(err.Error()) + ctx.log(err.Error()) return nil, err } - updateCtx.log(result.String()) + ctx.log(result.String()) if updateSubscribers { - i.notifyClients(rHash, invoice, invoice.State) + i.notifyClients(ctx.hash, invoice, invoice.State) } // Inspect latest htlc state on the invoice. - invoiceHtlc, ok := invoice.Htlcs[circuitKey] + invoiceHtlc, ok := invoice.Htlcs[ctx.circuitKey] // If it isn't recorded, cancel htlc. if !ok { return NewFailureResolution( - circuitKey, currentHeight, result, + ctx.circuitKey, ctx.currentHeight, result, ), nil } @@ -876,7 +922,7 @@ func (i *InvoiceRegistry) NotifyExitHopHtlc(rHash lntypes.Hash, switch invoiceHtlc.State { case channeldb.HtlcStateCanceled: return NewFailureResolution( - circuitKey, acceptHeight, result, + ctx.circuitKey, acceptHeight, result, ), nil case channeldb.HtlcStateSettled: @@ -902,27 +948,25 @@ func (i *InvoiceRegistry) NotifyExitHopHtlc(rHash lntypes.Hash, } resolution := NewSettleResolution( - invoice.Terms.PaymentPreimage, circuitKey, + invoice.Terms.PaymentPreimage, ctx.circuitKey, acceptHeight, result, ) return resolution, nil case channeldb.HtlcStateAccepted: - // (Re)start the htlc timer if the invoice is still open. It can + var resolution acceptResolution + + // Auto-release the htlc if the invoice is still open. It can // only happen for mpp payments that there are htlcs in state // Accepted while the invoice is Open. if invoice.State == channeldb.ContractOpen { - err := i.startHtlcTimer( - rHash, circuitKey, - invoiceHtlc.AcceptTime, - ) - if err != nil { - return nil, err - } + resolution.acceptTime = invoiceHtlc.AcceptTime + resolution.autoRelease = true + } - i.hodlSubscribe(hodlChan, circuitKey) - return nil, nil + i.hodlSubscribe(hodlChan, ctx.circuitKey) + return &resolution, nil default: panic("unknown action")