diff --git a/invoices/invoiceregistry_test.go b/invoices/invoiceregistry_test.go index cb916aea..3d12ef2b 100644 --- a/invoices/invoiceregistry_test.go +++ b/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") + } +} diff --git a/invoices/test_utils_test.go b/invoices/test_utils_test.go index d7846aec..63cf8d98 100644 --- a/invoices/test_utils_test.go +++ b/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, diff --git a/invoices/update.go b/invoices/update.go index ff420b85..8e2fd194 100644 --- a/invoices/update.go +++ b/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