invoices: force MPP payload inclusion for non-keysend payments

In this commit, we move to start rejecting any normal payments that
aren't keysend, if they don't also include the MPP invoice payload. With
this change, we require that some sort of e2e secret (either the payment
addr or the keysend pre-image) is present in a payload before we'll
accept the payment.

The second portion of the commit also updates all current tests in the
package. We kept the base `TestSettleInvoice` test in-tact as it still
exercises some useful behavior. However, we've removed all cases that
allow an overpayment, as the new MPP logic doesn't allow overpayment for
various reasons. In addition to this, some of the returned errors are
slightly different, tho the actual behavior is equivalent.

invoices/invoiceregistry_test.go

@@ -1190,3 +1190,178 @@ func TestOldInvoiceRemovalOnStart(t *testing.T) {
 	// registry start.
 	require.Equal(t, expected, response.Invoices)
 }
+
+// TestSettleInvoicePaymentAddrRequired tests that if an incoming payment has
+// an invoice that requires the payment addr bit to be set, and the incoming
+// payment doesn't include an mpp payload, then the payment is rejected.
+func TestSettleInvoicePaymentAddrRequired(t *testing.T) {
+	t.Parallel()
+
+	ctx := newTestContext(t)
+	defer ctx.cleanup()
+
+	allSubscriptions, err := ctx.registry.SubscribeNotifications(0, 0)
+	assert.Nil(t, err)
+	defer allSubscriptions.Cancel()
+
+	// Subscribe to the not yet existing invoice.
+	subscription, err := ctx.registry.SubscribeSingleInvoice(
+		testInvoicePaymentHash,
+	)
+	require.NoError(t, err)
+	defer subscription.Cancel()
+
+	require.Equal(
+		t, subscription.invoiceRef.PayHash(), testInvoicePaymentHash,
+	)
+
+	// Add the invoice, which requires the MPP payload to always be
+	// included due to its set of feature bits.
+	addIdx, err := ctx.registry.AddInvoice(
+		testPayAddrReqInvoice, testInvoicePaymentHash,
+	)
+	require.NoError(t, err)
+	require.Equal(t, int(addIdx), 1)
+
+	// We expect the open state to be sent to the single invoice subscriber.
+	select {
+	case update := <-subscription.Updates:
+		if update.State != channeldb.ContractOpen {
+			t.Fatalf("expected state ContractOpen, but got %v",
+				update.State)
+		}
+	case <-time.After(testTimeout):
+		t.Fatal("no update received")
+	}
+
+	// We expect a new invoice notification to be sent out.
+	select {
+	case newInvoice := <-allSubscriptions.NewInvoices:
+		if newInvoice.State != channeldb.ContractOpen {
+			t.Fatalf("expected state ContractOpen, but got %v",
+				newInvoice.State)
+		}
+	case <-time.After(testTimeout):
+		t.Fatal("no update received")
+	}
+
+	hodlChan := make(chan interface{}, 1)
+
+	// Now try to settle the invoice, the testPayload doesn't have any mpp
+	// information, so it should be forced to the updateLegacy path then
+	// fail as a required feature bit exists.
+	resolution, err := ctx.registry.NotifyExitHopHtlc(
+		testInvoicePaymentHash, testInvoice.Terms.Value,
+		uint32(testCurrentHeight)+testInvoiceCltvDelta-1,
+		testCurrentHeight, getCircuitKey(10), hodlChan, testPayload,
+	)
+	require.NoError(t, err)
+
+	failResolution, ok := resolution.(*HtlcFailResolution)
+	if !ok {
+		t.Fatalf("expected fail resolution, got: %T",
+			resolution)
+	}
+	require.Equal(t, failResolution.AcceptHeight, testCurrentHeight)
+	require.Equal(t, failResolution.Outcome, ResultAddressMismatch)
+}
+
+// TestSettleInvoicePaymentAddrRequiredOptionalGrace tests that if an invoice
+// in the database has an optional payment addr required bit set, then we'll
+// still allow it to be paid by an incoming HTLC that doesn't include the MPP
+// payload. This ensures we don't break payment for any invoices in the wild.
+func TestSettleInvoicePaymentAddrRequiredOptionalGrace(t *testing.T) {
+	t.Parallel()
+
+	ctx := newTestContext(t)
+	defer ctx.cleanup()
+
+	allSubscriptions, err := ctx.registry.SubscribeNotifications(0, 0)
+	assert.Nil(t, err)
+	defer allSubscriptions.Cancel()
+
+	// Subscribe to the not yet existing invoice.
+	subscription, err := ctx.registry.SubscribeSingleInvoice(
+		testInvoicePaymentHash,
+	)
+	require.NoError(t, err)
+	defer subscription.Cancel()
+
+	require.Equal(
+		t, subscription.invoiceRef.PayHash(), testInvoicePaymentHash,
+	)
+
+	// Add the invoice, which requires the MPP payload to always be
+	// included due to its set of feature bits.
+	addIdx, err := ctx.registry.AddInvoice(
+		testPayAddrOptionalInvoice, testInvoicePaymentHash,
+	)
+	require.NoError(t, err)
+	require.Equal(t, int(addIdx), 1)
+
+	// We expect the open state to be sent to the single invoice
+	// subscriber.
+	select {
+	case update := <-subscription.Updates:
+		if update.State != channeldb.ContractOpen {
+			t.Fatalf("expected state ContractOpen, but got %v",
+				update.State)
+		}
+	case <-time.After(testTimeout):
+		t.Fatal("no update received")
+	}
+
+	// We expect a new invoice notification to be sent out.
+	select {
+	case newInvoice := <-allSubscriptions.NewInvoices:
+		if newInvoice.State != channeldb.ContractOpen {
+			t.Fatalf("expected state ContractOpen, but got %v",
+				newInvoice.State)
+		}
+	case <-time.After(testTimeout):
+		t.Fatal("no update received")
+	}
+
+	// We'll now attempt to settle the invoice as normal, this should work
+	// no problem as we should allow these existing invoices to be settled.
+	hodlChan := make(chan interface{}, 1)
+	resolution, err := ctx.registry.NotifyExitHopHtlc(
+		testInvoicePaymentHash, testInvoiceAmt,
+		testHtlcExpiry, testCurrentHeight,
+		getCircuitKey(10), hodlChan, testPayload,
+	)
+	require.NoError(t, err)
+
+	settleResolution, ok := resolution.(*HtlcSettleResolution)
+	if !ok {
+		t.Fatalf("expected settle resolution, got: %T",
+			resolution)
+	}
+	require.Equal(t, settleResolution.Outcome, ResultSettled)
+
+	// We expect the settled state to be sent to the single invoice
+	// subscriber.
+	select {
+	case update := <-subscription.Updates:
+		if update.State != channeldb.ContractSettled {
+			t.Fatalf("expected state ContractOpen, but got %v",
+				update.State)
+		}
+		if update.AmtPaid != testInvoice.Terms.Value {
+			t.Fatal("invoice AmtPaid incorrect")
+		}
+	case <-time.After(testTimeout):
+		t.Fatal("no update received")
+	}
+
+	// We expect a settled notification to be sent out.
+	select {
+	case settledInvoice := <-allSubscriptions.SettledInvoices:
+		if settledInvoice.State != channeldb.ContractSettled {
+			t.Fatalf("expected state ContractOpen, but got %v",
+				settledInvoice.State)
+		}
+	case <-time.After(testTimeout):
+		t.Fatal("no update received")
+	}
+}

invoices/test_utils_test.go

@@ -100,6 +100,32 @@ var (
 		CreationDate: testInvoiceCreationDate,
 	}
 
+	testPayAddrReqInvoice = &channeldb.Invoice{
+		Terms: channeldb.ContractTerm{
+			PaymentPreimage: &testInvoicePreimage,
+			Value:           testInvoiceAmt,
+			Expiry:          time.Hour,
+			Features: lnwire.NewFeatureVector(
+				lnwire.NewRawFeatureVector(lnwire.PaymentAddrRequired),
+				lnwire.Features,
+			),
+		},
+		CreationDate: testInvoiceCreationDate,
+	}
+
+	testPayAddrOptionalInvoice = &channeldb.Invoice{
+		Terms: channeldb.ContractTerm{
+			PaymentPreimage: &testInvoicePreimage,
+			Value:           testInvoiceAmt,
+			Expiry:          time.Hour,
+			Features: lnwire.NewFeatureVector(
+				lnwire.NewRawFeatureVector(lnwire.PaymentAddrOptional),
+				lnwire.Features,
+			),
+		},
+		CreationDate: testInvoiceCreationDate,
+	}
+
 	testHodlInvoice = &channeldb.Invoice{
 		Terms: channeldb.ContractTerm{
 			Value:    testInvoiceAmt,

invoices/update.go

@@ -90,6 +90,9 @@ func updateInvoice(ctx *invoiceUpdateCtx, inv *channeldb.Invoice) (
 		}
 	}
 
+	// If no MPP payload was provided, then we expect this to be a keysend,
+	// or a payment to an invoice created before we started to require the
+	// MPP payload.
 	if ctx.mpp == nil {
 		return updateLegacy(ctx, inv)
 	}
@@ -206,6 +209,10 @@ func updateMpp(ctx *invoiceUpdateCtx,
 
 // updateLegacy is a callback for DB.UpdateInvoice that contains the invoice
 // settlement logic for legacy payments.
+//
+// NOTE: This function is only kept in place in order to be able to handle key
+// send payments and any invoices we created in the past that are valid and
+// still had the optional mpp bit set.
 func updateLegacy(ctx *invoiceUpdateCtx,
 	inv *channeldb.Invoice) (*channeldb.InvoiceUpdateDesc, HtlcResolution, error) {
 
@@ -223,8 +230,20 @@ func updateLegacy(ctx *invoiceUpdateCtx,
 		return nil, ctx.failRes(ResultAmountTooLow), nil
 	}
 
-	// TODO(joostjager): Check invoice mpp required feature
-	// bit when feature becomes mandatory.
+	// If the invoice had the required feature bit set at this point, then
+	// if we're in this method it means that the remote party didn't supply
+	// the expected payload. However if this is a keysend payment, then
+	// we'll permit it to pass.
+	_, isKeySend := ctx.customRecords[record.KeySendType]
+	invoiceFeatures := inv.Terms.Features
+	paymentAddrRequired := invoiceFeatures.RequiresFeature(
+		lnwire.PaymentAddrRequired,
+	)
+	if !isKeySend && paymentAddrRequired {
+		log.Warnf("Payment to pay_hash=%v doesn't include MPP "+
+			"payload, rejecting", ctx.hash)
+		return nil, ctx.failRes(ResultAddressMismatch), nil
+	}
 
 	// Don't allow settling the invoice with an old style
 	// htlc if we are already in the process of gathering an