From d6d9ec6aa542e64573fcaca7b3b2bed2ae3d9dc0 Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Fri, 9 Aug 2019 15:09:57 +0200 Subject: [PATCH] invoices: replay awareness Previously the invoice registry wasn't aware of replayed htlcs. This was dealt with by keeping the invoice accept/settle logic idempotent, so that a replay wouldn't have an effect. This mechanism has two limitations: 1. No accurate tracking of the total amount paid to an invoice. The total amount couldn't just be increased with every htlc received, because it could be a replay which would lead to counting the htlc amount multiple times. Therefore the total amount was set to the amount of the first htlc that was received, even though there may have been multiple htlcs paying to the invoice. 2. Impossible to check htlc expiry consistently for hodl invoices. When an htlc is new, its expiry needs to be checked against the invoice cltv delta. But for a replay, that check must be skipped. The htlc was accepted in time, the invoice was moved to the accepted state and a replay some blocks later shouldn't lead to that htlc being cancelled. Because the invoice registry couldn't recognize replays, it stopped checking htlc expiry heights when the invoice reached the accepted state. This prevents hold htlcs from being cancelled after a restart. But unfortunately this also caused additional htlcs to be accepted on an already accepted invoice without their expiry being checked. In this commit, the invoice registry starts to persistently track htlcs so that replays can be recognized. For replays, an htlc resolution action is returned early. This fixes both limitations mentioned above. --- channeldb/invoice_test.go | 41 +++----- channeldb/invoices.go | 79 +++++++++++++-- contractcourt/channel_arbitrator.go | 6 ++ invoices/invoiceregistry.go | 152 ++++++++++++++++------------ invoices/invoiceregistry_test.go | 14 +-- 5 files changed, 188 insertions(+), 104 deletions(-) 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