From fc9af92dd93801cdd7545c5246f357e81c03f64f Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Wed, 3 Mar 2021 09:58:33 -0800 Subject: [PATCH] channeldb: only accept/settle non-empty HTLC sets Prior to AMP, there could only be one HTLC set. Now that there can be multiple, we introduce the inherent risk that we might be persisting a settle/accept of the wrong HTLC set. To migitigate this risk, we add a check to assert that an invoice can only be transitioned into an Accepted or Settled state if the specified HTLC set is non-empty, i.e. has accepted HTLCs. To do this check, we unroll the loops in UpdateInvoice to first process any added or canceled HTLCs. This ensures that the set of accepted HTLCs is up-to-date when we got to transition the invoice into a new state, at which point we can accurately check that the HTLC set is non-empty. Previously this sort of check wasn't possible because a newly added HTLC wouldn't be populated on the invoice when going to call updateInvoiceState. Once the invoice-level transition checks have completed, we complete a final pass to move the HTLCs into their final states and recompute the amount paid. With this refactor, it is now possible to make stronger assertions about the invoice and the state transitions being made by the invoiceregistry before those changes are ultimately persisted. --- channeldb/invoice_test.go | 7 ++++ channeldb/invoices.go | 82 ++++++++++++++++++++++++--------------- 2 files changed, 57 insertions(+), 32 deletions(-) diff --git a/channeldb/invoice_test.go b/channeldb/invoice_test.go index ad4d84a21..3b0416d1c 100644 --- a/channeldb/invoice_test.go +++ b/channeldb/invoice_test.go @@ -1421,6 +1421,13 @@ func TestSetIDIndex(t *testing.T) { require.Nil(t, err) require.Equal(t, invoice, &dbInvoiceBySetID) + // Now settle the first htlc set, asserting that the two htlcs with set + // id 2 get canceled as a result. + _, err = db.UpdateInvoice( + ref, getUpdateInvoiceAMPSettle(&[32]byte{}), + ) + require.Equal(t, ErrEmptyHTLCSet, err) + // Now settle the first htlc set, asserting that the two htlcs with set // id 2 get canceled as a result. dbInvoice, err = db.UpdateInvoice(ref, getUpdateInvoiceAMPSettle(setID)) diff --git a/channeldb/invoices.go b/channeldb/invoices.go index 3d1188ee1..5cb0cdfcc 100644 --- a/channeldb/invoices.go +++ b/channeldb/invoices.go @@ -118,6 +118,10 @@ var ( // ErrInvoiceHasHtlcs is returned when attempting to insert an invoice // that already has HTLCs. ErrInvoiceHasHtlcs = errors.New("cannot add invoice with htlcs") + + // ErrEmptyHTLCSet is returned when attempting to accept or settle and + // HTLC set that has no HTLCs. + ErrEmptyHTLCSet = errors.New("cannot settle/accept empty HTLC set") ) // ErrDuplicateSetID is an error returned when attempting to adding an AMP HTLC @@ -1753,31 +1757,17 @@ func (d *DB) updateInvoice(hash lntypes.Hash, invoices, return &invoice, nil } - var setID *[32]byte + var ( + newState = invoice.State + setID *[32]byte + ) if update.State != nil { setID = update.State.SetID + newState = update.State.NewState } now := d.clock.Now() - // Update invoice state if the update descriptor indicates an invoice - // state change. - if update.State != nil { - err := updateInvoiceState(&invoice, hash, *update.State) - if err != nil { - return nil, err - } - - if update.State.NewState == ContractSettled { - err := setSettleMetaFields( - settleIndex, invoiceNum, &invoice, now, - ) - if err != nil { - return nil, err - } - } - } - // Process add actions from update descriptor. for key, htlcUpdate := range update.AddHtlcs { if _, exists := invoice.Htlcs[key]; exists { @@ -1820,11 +1810,8 @@ func (d *DB) updateInvoice(hash lntypes.Hash, invoices, invoice.Htlcs[key] = htlc } - // Align htlc states with invoice state and recalculate amount paid. - var ( - amtPaid lnwire.MilliSatoshi - cancelHtlcs = update.CancelHtlcs - ) + // Process cancel actions from update descriptor. + cancelHtlcs := update.CancelHtlcs for key, htlc := range invoice.Htlcs { // Check whether this htlc needs to be canceled. If it does, // update the htlc state to Canceled. @@ -1837,7 +1824,7 @@ func (d *DB) updateInvoice(hash lntypes.Hash, invoices, key) } - err := cancelSingleHtlc(now, htlc, invoice.State) + err := cancelSingleHtlc(now, htlc, newState) if err != nil { return nil, err } @@ -1845,10 +1832,41 @@ func (d *DB) updateInvoice(hash lntypes.Hash, invoices, // Delete processed cancel action, so that we can check // later that there are no actions left. delete(cancelHtlcs, key) + } + } - continue + // Verify that we didn't get an action for htlcs that are not present on + // the invoice. + if len(cancelHtlcs) > 0 { + return nil, errors.New("cancel action on non-existent htlc(s)") + } + + // At this point, the set of accepted HTLCs should be fully + // populated with added HTLCs or removed of canceled ones. Update + // invoice state if the update descriptor indicates an invoice state + // change, which depends on having an accurate view of the accepted + // HTLCs. + if update.State != nil { + err := updateInvoiceState(&invoice, hash, *update.State) + if err != nil { + return nil, err } + if update.State.NewState == ContractSettled { + err := setSettleMetaFields( + settleIndex, invoiceNum, &invoice, now, + ) + if err != nil { + return nil, err + } + } + } + + // With any invoice level state transitions recorded, we'll now finalize + // the process by updating the state transitions for individual HTLCs + // and recalculate the total amount paid to the invoice. + var amtPaid lnwire.MilliSatoshi + for _, htlc := range invoice.Htlcs { // The invoice state may have changed and this could have // implications for the states of the individual htlcs. Align // the htlc state with the current invoice state. @@ -1868,12 +1886,6 @@ func (d *DB) updateInvoice(hash lntypes.Hash, invoices, } invoice.AmtPaid = amtPaid - // Verify that we didn't get an action for htlcs that are not present on - // the invoice. - if len(cancelHtlcs) > 0 { - return nil, errors.New("cancel action on non-existent htlc(s)") - } - // Reserialize and update invoice. var buf bytes.Buffer if err := serializeInvoice(&buf, &invoice); err != nil { @@ -1918,6 +1930,12 @@ func updateInvoiceState(invoice *Invoice, hash lntypes.Hash, return nil } + // Sanity check that the user isn't trying to settle or accept a + // non-existent HTLC set. + if len(invoice.HTLCSet(update.SetID)) == 0 { + return ErrEmptyHTLCSet + } + // For AMP invoices, there are no invoice-level preimage checks. // However, we still sanity check that we aren't trying to // settle an AMP invoice with a preimage.