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

@@ -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))

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.
+	// Process cancel actions from update descriptor.
-	var (
+	cancelHtlcs := update.CancelHtlcs
-		amtPaid     lnwire.MilliSatoshi
-		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.