diff --git a/channeldb/invoice_test.go b/channeldb/invoice_test.go index 3ed1f8eb..4b5dda87 100644 --- a/channeldb/invoice_test.go +++ b/channeldb/invoice_test.go @@ -10,26 +10,6 @@ import ( "github.com/lightningnetwork/lnd/lnwire" ) -var ( - testCircuitKey = CircuitKey{ - ChanID: lnwire.ShortChannelID{ - BlockHeight: 1, TxIndex: 2, TxPosition: 3, - }, - HtlcID: 4, - } - - testHtlcs = map[CircuitKey]*InvoiceHTLC{ - testCircuitKey: { - State: HtlcStateCancelled, - AcceptTime: time.Unix(1, 0), - AcceptHeight: 100, - ResolveTime: time.Unix(2, 0), - Amt: 5200, - Expiry: 150, - }, - } -) - func randInvoice(value lnwire.MilliSatoshi) (*Invoice, error) { var pre [32]byte if _, err := rand.Read(pre[:]); err != nil { @@ -44,9 +24,8 @@ func randInvoice(value lnwire.MilliSatoshi) (*Invoice, error) { PaymentPreimage: pre, Value: value, }, - Htlcs: testHtlcs, - FinalCltvDelta: 50, - Expiry: 4000, + Htlcs: map[CircuitKey]*InvoiceHTLC{}, + Expiry: 4000, } i.Memo = []byte("memo") i.Receipt = []byte("receipt") @@ -82,7 +61,7 @@ func TestInvoiceWorkflow(t *testing.T) { // Use single second precision to avoid false positive test // failures due to the monotonic time component. CreationDate: time.Unix(time.Now().Unix(), 0), - Htlcs: testHtlcs, + Htlcs: map[CircuitKey]*InvoiceHTLC{}, } fakeInvoice.Memo = []byte("memo") fakeInvoice.Receipt = []byte("receipt") @@ -383,6 +362,14 @@ func TestDuplicateSettleInvoice(t *testing.T) { invoice.Terms.State = ContractSettled invoice.AmtPaid = amt invoice.SettleDate = dbInvoice.SettleDate + invoice.Htlcs = map[CircuitKey]*InvoiceHTLC{ + {}: { + Amt: amt, + AcceptTime: time.Unix(1, 0), + ResolveTime: time.Unix(1, 0), + State: HtlcStateSettled, + }, + } // We should get back the exact same invoice that we just inserted. if !reflect.DeepEqual(dbInvoice, invoice) { @@ -695,7 +682,11 @@ func getUpdateInvoice(amt lnwire.MilliSatoshi) InvoiceUpdateCallback { update := &InvoiceUpdateDesc{ Preimage: invoice.Terms.PaymentPreimage, State: ContractSettled, - AmtPaid: amt, + Htlcs: map[CircuitKey]*HtlcAcceptDesc{ + {}: { + Amt: amt, + }, + }, } return update, nil diff --git a/channeldb/invoices.go b/channeldb/invoices.go index df218af1..ad09da4f 100644 --- a/channeldb/invoices.go +++ b/channeldb/invoices.go @@ -276,14 +276,28 @@ type InvoiceHTLC struct { State HtlcState } +// HtlcAcceptDesc describes the details of a newly accepted htlc. +type HtlcAcceptDesc struct { + // AcceptHeight is the block height at which this htlc was accepted. + AcceptHeight int32 + + // Amt is the amount that is carried by this htlc. + Amt lnwire.MilliSatoshi + + // Expiry is the expiry height of this htlc. + Expiry uint32 +} + // InvoiceUpdateDesc describes the changes that should be applied to the // invoice. type InvoiceUpdateDesc struct { // State is the new state that this invoice should progress to. State ContractState - // AmtPaid is the updated amount that has been paid to this invoice. - AmtPaid lnwire.MilliSatoshi + // Htlcs describes the changes that need to be made to the invoice htlcs + // in the database. Htlc map entries with their value set should be + // added. If the map value is nil, the htlc should be cancelled. + Htlcs map[CircuitKey]*HtlcAcceptDesc // Preimage must be set to the preimage when state is settled. Preimage lntypes.Preimage @@ -1196,9 +1210,52 @@ func (d *DB) updateInvoice(hash lntypes.Hash, invoices, settleIndex *bbolt.Bucke return &invoice, err } - // Update invoice state and amount. + // Update invoice state. invoice.Terms.State = update.State - invoice.AmtPaid = update.AmtPaid + + now := d.now() + + // Update htlc set. + for key, htlcUpdate := range update.Htlcs { + htlc, ok := invoice.Htlcs[key] + + // No update means the htlc needs to be cancelled. + if htlcUpdate == nil { + if !ok { + return nil, fmt.Errorf("unknown htlc %v", key) + } + if htlc.State == HtlcStateSettled { + return nil, fmt.Errorf("cannot cancel a " + + "settled htlc") + } + + htlc.State = HtlcStateCancelled + htlc.ResolveTime = now + invoice.AmtPaid -= htlc.Amt + + continue + } + + // Add new htlc paying to the invoice. + if ok { + return nil, fmt.Errorf("htlc %v already exists", key) + } + htlc = &InvoiceHTLC{ + Amt: htlcUpdate.Amt, + Expiry: htlcUpdate.Expiry, + AcceptHeight: uint32(htlcUpdate.AcceptHeight), + AcceptTime: now, + } + if preUpdateState == ContractSettled { + htlc.State = HtlcStateSettled + htlc.ResolveTime = now + } else { + htlc.State = HtlcStateAccepted + } + + invoice.Htlcs[key] = htlc + invoice.AmtPaid += htlc.Amt + } // If invoice moved to the settled state, update settle index and settle // time. @@ -1210,9 +1267,17 @@ func (d *DB) updateInvoice(hash lntypes.Hash, invoices, settleIndex *bbolt.Bucke } invoice.Terms.PaymentPreimage = update.Preimage - err := setSettleFields( - settleIndex, invoiceNum, &invoice, d.now(), - ) + // Settle all accepted htlcs. + for _, htlc := range invoice.Htlcs { + if htlc.State != HtlcStateAccepted { + continue + } + + htlc.State = HtlcStateSettled + htlc.ResolveTime = now + } + + err := setSettleFields(settleIndex, invoiceNum, &invoice, now) if err != nil { return nil, err } diff --git a/contractcourt/channel_arbitrator.go b/contractcourt/channel_arbitrator.go index 8fd9f940..b9070768 100644 --- a/contractcourt/channel_arbitrator.go +++ b/contractcourt/channel_arbitrator.go @@ -1745,9 +1745,15 @@ func (c *ChannelArbitrator) prepContractResolutions( continue } + circuitKey := channeldb.CircuitKey{ + HtlcID: htlc.HtlcIndex, + ChanID: c.cfg.ShortChanID, + } + resKit.Quit = make(chan struct{}) resolver := &htlcIncomingContestResolver{ htlcExpiry: htlc.RefundTimeout, + circuitKey: circuitKey, htlcSuccessResolver: htlcSuccessResolver{ htlcResolution: resolution, broadcastHeight: height, diff --git a/invoices/invoiceregistry.go b/invoices/invoiceregistry.go index 4fbd683e..b1dc4b4d 100644 --- a/invoices/invoiceregistry.go +++ b/invoices/invoiceregistry.go @@ -27,6 +27,10 @@ var ( // errNoUpdate is returned when no invoice updated is required. errNoUpdate = errors.New("no update needed") + + // errReplayedHtlc is returned if the htlc is already recorded on the + // invoice. + errReplayedHtlc = errors.New("replayed htlc") ) // HodlEvent describes how an htlc should be resolved. If HodlEvent.Preimage is @@ -435,20 +439,34 @@ func (i *InvoiceRegistry) NotifyExitHopHtlc(rHash lntypes.Hash, rHash[:], s, amtPaid, expiry, circuitKey) } - const ( - actionCancel = iota - actionSettle - actionHold - ) - - // If no action is set, cancel the htlc. - action := actionCancel + // Default is to not update subscribers after the invoice update. + updateSubscribers := false updateInvoice := func(inv *channeldb.Invoice) ( *channeldb.InvoiceUpdateDesc, error) { - // If the invoice is already canceled, there is no - // further checking to do. + // Don't update the invoice when this is a replayed htlc. + htlc, ok := inv.Htlcs[circuitKey] + if ok { + switch htlc.State { + case channeldb.HtlcStateCancelled: + debugLog("replayed htlc to canceled invoice") + + case channeldb.HtlcStateAccepted: + debugLog("replayed htlc to accepted invoice") + + case channeldb.HtlcStateSettled: + debugLog("replayed htlc to settled invoice") + + default: + return nil, errors.New("unexpected htlc state") + } + + return nil, errNoUpdate + } + + // If the invoice is already canceled, there is no further + // checking to do. if inv.Terms.State == channeldb.ContractCanceled { debugLog("invoice already canceled") return nil, errNoUpdate @@ -462,36 +480,6 @@ func (i *InvoiceRegistry) NotifyExitHopHtlc(rHash lntypes.Hash, return nil, errNoUpdate } - // Return early in case the invoice was already accepted or - // settled. We don't want to check the expiry again, because it - // may be that we are just restarting. - // - // NOTE: Though our recovery and forwarding logic is - // predominately batched, settling invoices happens iteratively. - // We may reject one of two payments for the same rhash at - // first, but then restart and reject both after seeing that the - // invoice has been settled. Without any record of which one - // settles first, it is ambiguous as to which one actually - // settled the invoice. Thus, by accepting all payments, we - // eliminate the race condition that can lead to this - // inconsistency. - // - // TODO(conner): track ownership of settlements to properly - // recover from failures? or add batch invoice settlement - switch inv.Terms.State { - case channeldb.ContractAccepted: - debugLog("accepting duplicate payment to accepted invoice") - action = actionHold - return nil, errNoUpdate - - // If invoice is already settled, settle htlc. This means we - // accept more payments to the same invoice hash. - case channeldb.ContractSettled: - debugLog("accepting duplicate payment to settled invoice") - action = actionSettle - return nil, errNoUpdate - } - // The invoice is still open. Check the expiry. if expiry < uint32(currentHeight+i.finalCltvRejectDelta) { debugLog("expiry too soon") @@ -503,8 +491,31 @@ func (i *InvoiceRegistry) NotifyExitHopHtlc(rHash lntypes.Hash, return nil, errNoUpdate } + // Record HTLC in the invoice database. + newHtlcs := map[channeldb.CircuitKey]*channeldb.HtlcAcceptDesc{ + circuitKey: { + Amt: amtPaid, + Expiry: expiry, + AcceptHeight: currentHeight, + }, + } + update := channeldb.InvoiceUpdateDesc{ - AmtPaid: amtPaid, + Htlcs: newHtlcs, + } + + // Don't update invoice state if we are accepting a duplicate + // payment. We do accept or settle the HTLC. + switch inv.Terms.State { + case channeldb.ContractAccepted: + debugLog("accepting duplicate payment to accepted invoice") + update.State = channeldb.ContractAccepted + return &update, nil + + case channeldb.ContractSettled: + debugLog("accepting duplicate payment to settled invoice") + update.State = channeldb.ContractSettled + return &update, nil } // Check to see if we can settle or this is an hold invoice and @@ -512,14 +523,15 @@ func (i *InvoiceRegistry) NotifyExitHopHtlc(rHash lntypes.Hash, holdInvoice := inv.Terms.PaymentPreimage == channeldb.UnknownPreimage if holdInvoice { debugLog("accepted") - action = actionHold update.State = channeldb.ContractAccepted } else { debugLog("settled") - action = actionSettle update.Preimage = inv.Terms.PaymentPreimage update.State = channeldb.ContractSettled } + + updateSubscribers = true + return &update, nil } @@ -527,36 +539,39 @@ func (i *InvoiceRegistry) NotifyExitHopHtlc(rHash lntypes.Hash, // one exists). The callback will set the resolution action that is // returned to the link or contract resolver. invoice, err := i.cdb.UpdateInvoice(rHash, updateInvoice) - switch err { - - // Invoice was updated, notify clients. - case nil: - i.notifyClients(rHash, invoice, invoice.Terms.State) - - // No invoice update in the database was performed, no action required. - case errNoUpdate: - - // Log and return other unexpected errors. - default: + if err != nil && err != errNoUpdate { debugLog(err.Error()) return nil, err } - switch action { + if updateSubscribers { + i.notifyClients(rHash, invoice, invoice.Terms.State) + } - case actionSettle: + // Inspect latest htlc state on the invoice. + invoiceHtlc, ok := invoice.Htlcs[circuitKey] + + // If it isn't recorded, cancel htlc. + if !ok { + return &HodlEvent{ + Hash: rHash, + }, nil + } + + switch invoiceHtlc.State { + case channeldb.HtlcStateCancelled: + return &HodlEvent{ + Hash: rHash, + }, nil + + case channeldb.HtlcStateSettled: return &HodlEvent{ Hash: rHash, Preimage: &invoice.Terms.PaymentPreimage, }, nil - case actionCancel: - return &HodlEvent{ - Hash: rHash, - }, nil - - case actionHold: + case channeldb.HtlcStateAccepted: i.hodlSubscribe(hodlChan, rHash) return nil, nil @@ -583,7 +598,6 @@ func (i *InvoiceRegistry) SettleHodlInvoice(preimage lntypes.Preimage) error { } return &channeldb.InvoiceUpdateDesc{ - AmtPaid: invoice.AmtPaid, State: channeldb.ContractSettled, Preimage: preimage, }, nil @@ -626,10 +640,18 @@ func (i *InvoiceRegistry) CancelInvoice(payHash lntypes.Hash) error { return nil, channeldb.ErrInvoiceAlreadyCanceled } + // Mark individual held htlcs as cancelled. + canceledHtlcs := make( + map[channeldb.CircuitKey]*channeldb.HtlcAcceptDesc, + ) + for key := range invoice.Htlcs { + canceledHtlcs[key] = nil + } + // Move invoice to the canceled state. return &channeldb.InvoiceUpdateDesc{ - AmtPaid: 0, - State: channeldb.ContractCanceled, + Htlcs: canceledHtlcs, + State: channeldb.ContractCanceled, }, nil } diff --git a/invoices/invoiceregistry_test.go b/invoices/invoiceregistry_test.go index c7873d5b..c8ae8e0e 100644 --- a/invoices/invoiceregistry_test.go +++ b/invoices/invoiceregistry_test.go @@ -199,13 +199,14 @@ func TestSettleInvoice(t *testing.T) { t.Fatal("expected cancel event") } - // Check that settled amount remains unchanged. + // Check that settled amount is equal to the sum of values of the htlcs + // 0 and 1. inv, err := registry.LookupInvoice(hash) if err != nil { t.Fatal(err) } - if inv.AmtPaid != amtPaid { - t.Fatal("expected amount to be unchanged") + if inv.AmtPaid != amtPaid+amtPaid+600 { + t.Fatal("amount incorrect") } // Try to cancel. @@ -426,8 +427,7 @@ func TestHoldInvoice(t *testing.T) { } // Test a new htlc coming in that doesn't meet the final cltv delta - // requirement. It should be rejected, but because invoice registry - // doesn't track individual htlcs it is accepted. + // requirement. It should be rejected. event, err = registry.NotifyExitHopHtlc( hash, amtPaid, 1, testCurrentHeight, getCircuitKey(1), hodlChan, nil, @@ -435,8 +435,8 @@ func TestHoldInvoice(t *testing.T) { if err != nil { t.Fatalf("expected settle to succeed but got %v", err) } - if event != nil { - t.Fatalf("expected htlc to be held") + if event == nil || event.Preimage != nil { + t.Fatalf("expected htlc to be cancelled") } // We expect the accepted state to be sent to the single invoice