From 17f529bfada6ba4cbda92e451481996f11cc0ea6 Mon Sep 17 00:00:00 2001 From: Carla Kirk-Cohen Date: Wed, 12 Jul 2023 16:08:20 -0400 Subject: [PATCH 1/6] invoices/test: rename newInvoice to reflect return value better Test utils currently have two different test invoices - a minimalist one that is used in a variety of test cases, and a customizable one that is used specifically for tests concerning invoice expiry. The latter is renamed and moved closer to the calling code to more clearly indicate its use. --- invoices/test_utils_test.go | 88 +++++++++++++++++++------------------ 1 file changed, 46 insertions(+), 42 deletions(-) diff --git a/invoices/test_utils_test.go b/invoices/test_utils_test.go index e16b3ecc3..8bbcdc3ad 100644 --- a/invoices/test_utils_test.go +++ b/invoices/test_utils_test.go @@ -275,46 +275,6 @@ func getCircuitKey(htlcID uint64) invpkg.CircuitKey { } } -func newTestInvoice(t *testing.T, preimage lntypes.Preimage, - timestamp time.Time, expiry time.Duration) *invpkg.Invoice { - - if expiry == 0 { - expiry = time.Hour - } - - var payAddr [32]byte - if _, err := rand.Read(payAddr[:]); err != nil { - t.Fatalf("unable to generate payment addr: %v", err) - } - - rawInvoice, err := zpay32.NewInvoice( - testNetParams, - preimage.Hash(), - timestamp, - zpay32.Amount(testInvoiceAmount), - zpay32.Description(testInvoiceDescription), - zpay32.Expiry(expiry), - zpay32.PaymentAddr(payAddr), - ) - require.NoError(t, err, "Error while creating new invoice") - - paymentRequest, err := rawInvoice.Encode(testMessageSigner) - - require.NoError(t, err, "Error while encoding payment request") - - return &invpkg.Invoice{ - Terms: invpkg.ContractTerm{ - PaymentPreimage: &preimage, - PaymentAddr: payAddr, - Value: testInvoiceAmount, - Expiry: expiry, - Features: testFeatures, - }, - PaymentRequest: []byte(paymentRequest), - CreationDate: timestamp, - } -} - // timeout implements a test level timeout. func timeout() func() { done := make(chan struct{}) @@ -360,7 +320,7 @@ func generateInvoiceExpiryTestData( var preimage lntypes.Preimage binary.BigEndian.PutUint32(preimage[:4], uint32(offset+i)) expiry := time.Duration((i+offset)%24) * time.Hour - invoice := newTestInvoice( + invoice := newInvoiceExpiryTestInvoice( t, preimage, expiredCreationDate, expiry, ) testData.expiredInvoices[preimage.Hash()] = invoice @@ -370,13 +330,57 @@ func generateInvoiceExpiryTestData( var preimage lntypes.Preimage binary.BigEndian.PutUint32(preimage[4:], uint32(offset+i)) expiry := time.Duration((i+offset)%24) * time.Hour - invoice := newTestInvoice(t, preimage, now, expiry) + invoice := newInvoiceExpiryTestInvoice(t, preimage, now, expiry) testData.pendingInvoices[preimage.Hash()] = invoice } return testData } +// newInvoiceExpiryTestInvoice creates a test invoice with a randomly generated +// payment address and custom preimage and expiry details. It should be used in +// the case where tests require custom invoice expiry and unique payment +// hashes. +func newInvoiceExpiryTestInvoice(t *testing.T, preimage lntypes.Preimage, + timestamp time.Time, expiry time.Duration) *invpkg.Invoice { + + if expiry == 0 { + expiry = time.Hour + } + + var payAddr [32]byte + if _, err := rand.Read(payAddr[:]); err != nil { + t.Fatalf("unable to generate payment addr: %v", err) + } + + rawInvoice, err := zpay32.NewInvoice( + testNetParams, + preimage.Hash(), + timestamp, + zpay32.Amount(testInvoiceAmount), + zpay32.Description(testInvoiceDescription), + zpay32.Expiry(expiry), + zpay32.PaymentAddr(payAddr), + ) + require.NoError(t, err, "Error while creating new invoice") + + paymentRequest, err := rawInvoice.Encode(testMessageSigner) + + require.NoError(t, err, "Error while encoding payment request") + + return &invpkg.Invoice{ + Terms: invpkg.ContractTerm{ + PaymentPreimage: &preimage, + PaymentAddr: payAddr, + Value: testInvoiceAmount, + Expiry: expiry, + Features: testFeatures, + }, + PaymentRequest: []byte(paymentRequest), + CreationDate: timestamp, + } +} + // checkSettleResolution asserts the resolution is a settle with the correct // preimage. If successful, the HtlcSettleResolution is returned in case further // checks are desired. From ceff879f37d6937722675a3408547bd81ba6ae94 Mon Sep 17 00:00:00 2001 From: Carla Kirk-Cohen Date: Wed, 12 Jul 2023 16:35:51 -0400 Subject: [PATCH 2/6] invoices/test: replace global testInvoice with constructor For our relatively "static" test invoice, add a simple constructor using the globals defined, rather than having a global invoice which risks being mutated. --- invoices/invoiceregistry_test.go | 53 +++++++++++++++++--------------- invoices/test_utils_test.go | 50 ++++++++++++++++++------------ 2 files changed, 60 insertions(+), 43 deletions(-) diff --git a/invoices/invoiceregistry_test.go b/invoices/invoiceregistry_test.go index 1edd753cd..55bbd4931 100644 --- a/invoices/invoiceregistry_test.go +++ b/invoices/invoiceregistry_test.go @@ -37,6 +37,7 @@ func TestSettleInvoice(t *testing.T) { require.Equal(t, subscription.PayHash(), &testInvoicePaymentHash) // Add the invoice. + testInvoice := newInvoice(t, false) addIdx, err := ctx.registry.AddInvoice( testInvoice, testInvoicePaymentHash, ) @@ -220,7 +221,7 @@ func testCancelInvoice(t *testing.T, gc bool) { require.Equal(t, subscription.PayHash(), &testInvoicePaymentHash) // Add the invoice. - amt := lnwire.MilliSatoshi(100000) + testInvoice := newInvoice(t, false) _, err = ctx.registry.AddInvoice(testInvoice, testInvoicePaymentHash) if err != nil { t.Fatal(err) @@ -298,8 +299,8 @@ func testCancelInvoice(t *testing.T, gc bool) { // result in a cancel resolution. hodlChan := make(chan interface{}) resolution, err := ctx.registry.NotifyExitHopHtlc( - testInvoicePaymentHash, amt, testHtlcExpiry, testCurrentHeight, - getCircuitKey(0), hodlChan, testPayload, + testInvoicePaymentHash, testInvoiceAmt, testHtlcExpiry, + testCurrentHeight, getCircuitKey(0), hodlChan, testPayload, ) if err != nil { t.Fatal("expected settlement of a canceled invoice to succeed") @@ -379,7 +380,8 @@ func TestSettleHoldInvoice(t *testing.T) { require.Equal(t, subscription.PayHash(), &testInvoicePaymentHash) // Add the invoice. - _, err = registry.AddInvoice(testHodlInvoice, testInvoicePaymentHash) + invoice := newInvoice(t, true) + _, err = registry.AddInvoice(invoice, testInvoicePaymentHash) if err != nil { t.Fatal(err) } @@ -536,7 +538,8 @@ func TestCancelHoldInvoice(t *testing.T) { }) // Add the invoice. - _, err = registry.AddInvoice(testHodlInvoice, testInvoicePaymentHash) + invoice := newInvoice(t, true) + _, err = registry.AddInvoice(invoice, testInvoicePaymentHash) if err != nil { t.Fatal(err) } @@ -840,6 +843,7 @@ func TestMppPayment(t *testing.T) { ctx := newTestContext(t, nil) // Add the invoice. + testInvoice := newInvoice(t, false) _, err := ctx.registry.AddInvoice(testInvoice, testInvoicePaymentHash) if err != nil { t.Fatal(err) @@ -936,9 +940,9 @@ func TestMppPaymentWithOverpayment(t *testing.T) { ctx := newTestContext(t, nil) // Add the invoice. - invoice := *testInvoice + testInvoice := newInvoice(t, false) _, err := ctx.registry.AddInvoice( - &invoice, testInvoicePaymentHash, + testInvoice, testInvoicePaymentHash, ) if err != nil { t.Fatal(err) @@ -954,7 +958,7 @@ func TestMppPaymentWithOverpayment(t *testing.T) { // Send htlc 1. hodlChan1 := make(chan interface{}, 1) resolution, err := ctx.registry.NotifyExitHopHtlc( - testInvoicePaymentHash, invoice.Terms.Value/2, + testInvoicePaymentHash, testInvoice.Terms.Value/2, testHtlcExpiry, testCurrentHeight, getCircuitKey(11), hodlChan1, mppPayload, ) @@ -969,7 +973,7 @@ func TestMppPaymentWithOverpayment(t *testing.T) { hodlChan2 := make(chan interface{}, 1) resolution, err = ctx.registry.NotifyExitHopHtlc( testInvoicePaymentHash, - invoice.Terms.Value/2+overpayment, testHtlcExpiry, + testInvoice.Terms.Value/2+overpayment, testHtlcExpiry, testCurrentHeight, getCircuitKey(12), hodlChan2, mppPayload, ) @@ -997,7 +1001,7 @@ func TestMppPaymentWithOverpayment(t *testing.T) { t.Fatal("expected invoice to be settled") } - return inv.AmtPaid == invoice.Terms.Value+overpayment + return inv.AmtPaid == testInvoice.Terms.Value+overpayment } if err := quick.Check(f, nil); err != nil { t.Fatalf("amount incorrect: %v", err) @@ -1222,11 +1226,11 @@ func testHeightExpiryWithRegistry(t *testing.T, numParts int, settle bool) { // Add a hold invoice, we set a non-nil payment request so that this // invoice is not considered a keysend by the expiry watcher. - invoice := *testInvoice - invoice.HodlInvoice = true - invoice.PaymentRequest = []byte{1, 2, 3} + testInvoice := newInvoice(t, false) + testInvoice.HodlInvoice = true + testInvoice.PaymentRequest = []byte{1, 2, 3} - _, err := ctx.registry.AddInvoice(&invoice, testInvoicePaymentHash) + _, err := ctx.registry.AddInvoice(testInvoice, testInvoicePaymentHash) require.NoError(t, err) payLoad := testPayload @@ -1236,7 +1240,7 @@ func testHeightExpiryWithRegistry(t *testing.T, numParts int, settle bool) { } } - htlcAmt := invoice.Terms.Value / lnwire.MilliSatoshi(numParts) + htlcAmt := testInvoice.Terms.Value / lnwire.MilliSatoshi(numParts) hodlChan := make(chan interface{}, numParts) for i := 0; i < numParts; i++ { // We bump our expiry height for each htlc so that we can test @@ -1262,7 +1266,9 @@ func testHeightExpiryWithRegistry(t *testing.T, numParts int, settle bool) { // Now that we've added our htlc(s), we tick our test clock to our // invoice expiry time. We don't expect the invoice to be canceled // based on its expiry time now that we have active htlcs. - ctx.clock.SetTime(invoice.CreationDate.Add(invoice.Terms.Expiry + 1)) + ctx.clock.SetTime( + testInvoice.CreationDate.Add(testInvoice.Terms.Expiry + 1), + ) // The expiry watcher loop takes some time to process the new clock // time. We mine the block before our expiry height, our mock will block @@ -1326,10 +1332,9 @@ func TestMultipleSetHeightExpiry(t *testing.T) { ctx := newTestContext(t, nil) // Add a hold invoice. - invoice := *testInvoice - invoice.HodlInvoice = true + testInvoice := newInvoice(t, true) - _, err := ctx.registry.AddInvoice(&invoice, testInvoicePaymentHash) + _, err := ctx.registry.AddInvoice(testInvoice, testInvoicePaymentHash) require.NoError(t, err) mppPayload := &mockPayload{ @@ -1339,7 +1344,7 @@ func TestMultipleSetHeightExpiry(t *testing.T) { // Send htlc 1. hodlChan1 := make(chan interface{}, 1) resolution, err := ctx.registry.NotifyExitHopHtlc( - testInvoicePaymentHash, invoice.Terms.Value/2, + testInvoicePaymentHash, testInvoice.Terms.Value/2, testHtlcExpiry, testCurrentHeight, getCircuitKey(10), hodlChan1, mppPayload, ) @@ -1369,7 +1374,7 @@ func TestMultipleSetHeightExpiry(t *testing.T) { // Send htlc 2. hodlChan2 := make(chan interface{}, 1) resolution, err = ctx.registry.NotifyExitHopHtlc( - testInvoicePaymentHash, invoice.Terms.Value/2, expiry, + testInvoicePaymentHash, testInvoice.Terms.Value/2, expiry, testCurrentHeight, getCircuitKey(11), hodlChan2, mppPayload, ) require.NoError(t, err) @@ -1378,7 +1383,7 @@ func TestMultipleSetHeightExpiry(t *testing.T) { // Send htlc 3. hodlChan3 := make(chan interface{}, 1) resolution, err = ctx.registry.NotifyExitHopHtlc( - testInvoicePaymentHash, invoice.Terms.Value/2, expiry, + testInvoicePaymentHash, testInvoice.Terms.Value/2, expiry, testCurrentHeight, getCircuitKey(12), hodlChan3, mppPayload, ) require.NoError(t, err) @@ -1462,7 +1467,7 @@ func TestSettleInvoicePaymentAddrRequired(t *testing.T) { // 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, + testInvoicePaymentHash, testPayAddrReqInvoice.Terms.Value, uint32(testCurrentHeight)+testInvoiceCltvDelta-1, testCurrentHeight, getCircuitKey(10), hodlChan, testPayload, ) @@ -1555,7 +1560,7 @@ func TestSettleInvoicePaymentAddrRequiredOptionalGrace(t *testing.T) { t.Fatalf("expected state ContractOpen, but got %v", update.State) } - if update.AmtPaid != testInvoice.Terms.Value { + if update.AmtPaid != testPayAddrOptionalInvoice.Terms.Value { t.Fatal("invoice AmtPaid incorrect") } case <-time.After(testTimeout): diff --git a/invoices/test_utils_test.go b/invoices/test_utils_test.go index 8bbcdc3ad..53f85d1a5 100644 --- a/invoices/test_utils_test.go +++ b/invoices/test_utils_test.go @@ -133,15 +133,6 @@ var ( var ( testInvoiceAmt = lnwire.MilliSatoshi(100000) - testInvoice = &invpkg.Invoice{ - Terms: invpkg.ContractTerm{ - PaymentPreimage: &testInvoicePreimage, - Value: testInvoiceAmt, - Expiry: time.Hour, - Features: testFeatures, - }, - CreationDate: testInvoiceCreationDate, - } testPayAddrReqInvoice = &invpkg.Invoice{ Terms: invpkg.ContractTerm{ @@ -174,16 +165,6 @@ var ( }, CreationDate: testInvoiceCreationDate, } - - testHodlInvoice = &invpkg.Invoice{ - Terms: invpkg.ContractTerm{ - Value: testInvoiceAmt, - Expiry: time.Hour, - Features: testFeatures, - }, - CreationDate: testInvoiceCreationDate, - HodlInvoice: true, - } ) func newTestChannelDB(t *testing.T, clock clock.Clock) (*channeldb.DB, error) { @@ -275,6 +256,37 @@ func getCircuitKey(htlcID uint64) invpkg.CircuitKey { } } +// newInvoice returns an invoice that can be used for testing, using the +// constant values defined above (deep copied if necessary). +// +// Note that this invoice *does not* have a payment address set. It will +// create a regular invoice with a preimage is hodl is false, and a hodl +// invoice with no preimage otherwise. +func newInvoice(t *testing.T, hodl bool) *invpkg.Invoice { + invoice := &invpkg.Invoice{ + Terms: invpkg.ContractTerm{ + Value: testInvoiceAmt, + Expiry: time.Hour, + Features: testFeatures.Clone(), + }, + CreationDate: testInvoiceCreationDate, + } + + // If creating a hodl invoice, we don't include a preimage. + if hodl { + invoice.HodlInvoice = true + return invoice + } + + preimage, err := lntypes.MakePreimage( + testInvoicePreimage[:], + ) + require.NoError(t, err) + invoice.Terms.PaymentPreimage = &preimage + + return invoice +} + // timeout implements a test level timeout. func timeout() func() { done := make(chan struct{}) From 40f695743ad6f35053611457e694c67f31900c79 Mon Sep 17 00:00:00 2001 From: Carla Kirk-Cohen Date: Wed, 12 Jul 2023 16:57:26 -0400 Subject: [PATCH 3/6] invoice/test: move payment address optional into test using it No need for a global when this is only used once. --- invoices/invoiceregistry_test.go | 22 +++++++++++++++++++--- invoices/test_utils_test.go | 16 ---------------- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/invoices/invoiceregistry_test.go b/invoices/invoiceregistry_test.go index 55bbd4931..aca609386 100644 --- a/invoices/invoiceregistry_test.go +++ b/invoices/invoiceregistry_test.go @@ -1504,10 +1504,26 @@ func TestSettleInvoicePaymentAddrRequiredOptionalGrace(t *testing.T) { require.Equal(t, subscription.PayHash(), &testInvoicePaymentHash) - // Add the invoice, which requires the MPP payload to always be + invoice := &invpkg.Invoice{ + Terms: invpkg.ContractTerm{ + PaymentPreimage: &testInvoicePreimage, + Value: testInvoiceAmt, + Expiry: time.Hour, + Features: lnwire.NewFeatureVector( + lnwire.NewRawFeatureVector( + lnwire.TLVOnionPayloadOptional, + lnwire.PaymentAddrOptional, + ), + lnwire.Features, + ), + }, + CreationDate: testInvoiceCreationDate, + } + + // Add the invoice, which does not require the MPP payload to always be // included due to its set of feature bits. addIdx, err := ctx.registry.AddInvoice( - testPayAddrOptionalInvoice, testInvoicePaymentHash, + invoice, testInvoicePaymentHash, ) require.NoError(t, err) require.Equal(t, int(addIdx), 1) @@ -1560,7 +1576,7 @@ func TestSettleInvoicePaymentAddrRequiredOptionalGrace(t *testing.T) { t.Fatalf("expected state ContractOpen, but got %v", update.State) } - if update.AmtPaid != testPayAddrOptionalInvoice.Terms.Value { + if update.AmtPaid != invoice.Terms.Value { t.Fatal("invoice AmtPaid incorrect") } case <-time.After(testTimeout): diff --git a/invoices/test_utils_test.go b/invoices/test_utils_test.go index 53f85d1a5..9abcf5f19 100644 --- a/invoices/test_utils_test.go +++ b/invoices/test_utils_test.go @@ -149,22 +149,6 @@ var ( }, CreationDate: testInvoiceCreationDate, } - - testPayAddrOptionalInvoice = &invpkg.Invoice{ - Terms: invpkg.ContractTerm{ - PaymentPreimage: &testInvoicePreimage, - Value: testInvoiceAmt, - Expiry: time.Hour, - Features: lnwire.NewFeatureVector( - lnwire.NewRawFeatureVector( - lnwire.TLVOnionPayloadOptional, - lnwire.PaymentAddrOptional, - ), - lnwire.Features, - ), - }, - CreationDate: testInvoiceCreationDate, - } ) func newTestChannelDB(t *testing.T, clock clock.Clock) (*channeldb.DB, error) { From d080d07a224c315190d21d985011c601a38b1028 Mon Sep 17 00:00:00 2001 From: Carla Kirk-Cohen Date: Wed, 12 Jul 2023 17:08:07 -0400 Subject: [PATCH 4/6] invoices/test: remove duplicate test invoice amount global We have testInvoiceAmt and testInvoiceAmount with the same value. --- invoices/invoiceregistry_test.go | 14 +++++++------- invoices/test_utils_test.go | 6 ++---- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/invoices/invoiceregistry_test.go b/invoices/invoiceregistry_test.go index aca609386..ad47647d1 100644 --- a/invoices/invoiceregistry_test.go +++ b/invoices/invoiceregistry_test.go @@ -299,7 +299,7 @@ func testCancelInvoice(t *testing.T, gc bool) { // result in a cancel resolution. hodlChan := make(chan interface{}) resolution, err := ctx.registry.NotifyExitHopHtlc( - testInvoicePaymentHash, testInvoiceAmt, testHtlcExpiry, + testInvoicePaymentHash, testInvoiceAmount, testHtlcExpiry, testCurrentHeight, getCircuitKey(0), hodlChan, testPayload, ) if err != nil { @@ -850,7 +850,7 @@ func TestMppPayment(t *testing.T) { } mppPayload := &mockPayload{ - mpp: record.NewMPP(testInvoiceAmt, [32]byte{}), + mpp: record.NewMPP(testInvoiceAmount, [32]byte{}), } // Send htlc 1. @@ -949,7 +949,7 @@ func TestMppPaymentWithOverpayment(t *testing.T) { } mppPayload := &mockPayload{ - mpp: record.NewMPP(testInvoiceAmt, [32]byte{}), + mpp: record.NewMPP(testInvoiceAmount, [32]byte{}), } // We constrain overpayment amount to be [1,1000]. @@ -1236,7 +1236,7 @@ func testHeightExpiryWithRegistry(t *testing.T, numParts int, settle bool) { payLoad := testPayload if numParts > 1 { payLoad = &mockPayload{ - mpp: record.NewMPP(testInvoiceAmt, [32]byte{}), + mpp: record.NewMPP(testInvoiceAmount, [32]byte{}), } } @@ -1338,7 +1338,7 @@ func TestMultipleSetHeightExpiry(t *testing.T) { require.NoError(t, err) mppPayload := &mockPayload{ - mpp: record.NewMPP(testInvoiceAmt, [32]byte{}), + mpp: record.NewMPP(testInvoiceAmount, [32]byte{}), } // Send htlc 1. @@ -1507,7 +1507,7 @@ func TestSettleInvoicePaymentAddrRequiredOptionalGrace(t *testing.T) { invoice := &invpkg.Invoice{ Terms: invpkg.ContractTerm{ PaymentPreimage: &testInvoicePreimage, - Value: testInvoiceAmt, + Value: testInvoiceAmount, Expiry: time.Hour, Features: lnwire.NewFeatureVector( lnwire.NewRawFeatureVector( @@ -1555,7 +1555,7 @@ func TestSettleInvoicePaymentAddrRequiredOptionalGrace(t *testing.T) { // no problem as we should allow these existing invoices to be settled. hodlChan := make(chan interface{}, 1) resolution, err := ctx.registry.NotifyExitHopHtlc( - testInvoicePaymentHash, testInvoiceAmt, + testInvoicePaymentHash, testInvoiceAmount, testHtlcExpiry, testCurrentHeight, getCircuitKey(10), hodlChan, testPayload, ) diff --git a/invoices/test_utils_test.go b/invoices/test_utils_test.go index 9abcf5f19..854dae92f 100644 --- a/invoices/test_utils_test.go +++ b/invoices/test_utils_test.go @@ -132,12 +132,10 @@ var ( ) var ( - testInvoiceAmt = lnwire.MilliSatoshi(100000) - testPayAddrReqInvoice = &invpkg.Invoice{ Terms: invpkg.ContractTerm{ PaymentPreimage: &testInvoicePreimage, - Value: testInvoiceAmt, + Value: testInvoiceAmount, Expiry: time.Hour, Features: lnwire.NewFeatureVector( lnwire.NewRawFeatureVector( @@ -249,7 +247,7 @@ func getCircuitKey(htlcID uint64) invpkg.CircuitKey { func newInvoice(t *testing.T, hodl bool) *invpkg.Invoice { invoice := &invpkg.Invoice{ Terms: invpkg.ContractTerm{ - Value: testInvoiceAmt, + Value: testInvoiceAmount, Expiry: time.Hour, Features: testFeatures.Clone(), }, From 94e434d2f1d7380fb53e5dc8151c7265e9efd038 Mon Sep 17 00:00:00 2001 From: Carla Kirk-Cohen Date: Wed, 12 Jul 2023 17:09:32 -0400 Subject: [PATCH 5/6] invoices/test: move payment address required global into test using it --- invoices/invoiceregistry_test.go | 19 +++++++++++++++++-- invoices/test_utils_test.go | 18 ------------------ 2 files changed, 17 insertions(+), 20 deletions(-) diff --git a/invoices/invoiceregistry_test.go b/invoices/invoiceregistry_test.go index ad47647d1..36591c0d6 100644 --- a/invoices/invoiceregistry_test.go +++ b/invoices/invoiceregistry_test.go @@ -1431,10 +1431,25 @@ func TestSettleInvoicePaymentAddrRequired(t *testing.T) { require.Equal(t, subscription.PayHash(), &testInvoicePaymentHash) + invoice := &invpkg.Invoice{ + Terms: invpkg.ContractTerm{ + PaymentPreimage: &testInvoicePreimage, + Value: testInvoiceAmount, + Expiry: time.Hour, + Features: lnwire.NewFeatureVector( + lnwire.NewRawFeatureVector( + lnwire.TLVOnionPayloadOptional, + lnwire.PaymentAddrRequired, + ), + lnwire.Features, + ), + }, + CreationDate: testInvoiceCreationDate, + } // 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, + invoice, testInvoicePaymentHash, ) require.NoError(t, err) require.Equal(t, int(addIdx), 1) @@ -1467,7 +1482,7 @@ func TestSettleInvoicePaymentAddrRequired(t *testing.T) { // information, so it should be forced to the updateLegacy path then // fail as a required feature bit exists. resolution, err := ctx.registry.NotifyExitHopHtlc( - testInvoicePaymentHash, testPayAddrReqInvoice.Terms.Value, + testInvoicePaymentHash, invoice.Terms.Value, uint32(testCurrentHeight)+testInvoiceCltvDelta-1, testCurrentHeight, getCircuitKey(10), hodlChan, testPayload, ) diff --git a/invoices/test_utils_test.go b/invoices/test_utils_test.go index 854dae92f..13cd11184 100644 --- a/invoices/test_utils_test.go +++ b/invoices/test_utils_test.go @@ -131,24 +131,6 @@ var ( testInvoiceCreationDate = testTime ) -var ( - testPayAddrReqInvoice = &invpkg.Invoice{ - Terms: invpkg.ContractTerm{ - PaymentPreimage: &testInvoicePreimage, - Value: testInvoiceAmount, - Expiry: time.Hour, - Features: lnwire.NewFeatureVector( - lnwire.NewRawFeatureVector( - lnwire.TLVOnionPayloadOptional, - lnwire.PaymentAddrRequired, - ), - lnwire.Features, - ), - }, - CreationDate: testInvoiceCreationDate, - } -) - func newTestChannelDB(t *testing.T, clock clock.Clock) (*channeldb.DB, error) { t.Helper() From 4645fdfb0af75e1c36b8c26f2528822586e373b6 Mon Sep 17 00:00:00 2001 From: Carla Kirk-Cohen Date: Wed, 12 Jul 2023 16:39:03 -0400 Subject: [PATCH 6/6] invoice/test: make all tests parallel When we have tests using global variables and a mix of parallel and non-parallel tests, odd races can appear when we run them. --- invoices/invoiceregistry_test.go | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/invoices/invoiceregistry_test.go b/invoices/invoiceregistry_test.go index 36591c0d6..1b93b8f24 100644 --- a/invoices/invoiceregistry_test.go +++ b/invoices/invoiceregistry_test.go @@ -19,6 +19,8 @@ import ( // TestSettleInvoice tests settling of an invoice and related notifications. func TestSettleInvoice(t *testing.T) { + t.Parallel() + ctx := newTestContext(t, nil) allSubscriptions, err := ctx.registry.SubscribeNotifications(0, 0) @@ -194,6 +196,8 @@ func TestSettleInvoice(t *testing.T) { } func testCancelInvoice(t *testing.T, gc bool) { + t.Parallel() + cfg := defaultRegistryConfig() // If set to true, then also delete the invoice from the DB after @@ -326,6 +330,8 @@ func testCancelInvoice(t *testing.T, gc bool) { // TestCancelInvoice tests cancellation of an invoice and related notifications. func TestCancelInvoice(t *testing.T) { + t.Parallel() + // Test cancellation both with garbage collection (meaning that canceled // invoice will be deleted) and without (meaning it'll be kept). t.Run("garbage collect", func(t *testing.T) { @@ -340,6 +346,7 @@ func TestCancelInvoice(t *testing.T) { // TestSettleHoldInvoice tests settling of a hold invoice and related // notifications. func TestSettleHoldInvoice(t *testing.T) { + t.Parallel() defer timeout()() idb, err := newTestChannelDB(t, clock.NewTestClock(time.Time{})) @@ -513,6 +520,7 @@ func TestSettleHoldInvoice(t *testing.T) { // TestCancelHoldInvoice tests canceling of a hold invoice and related // notifications. func TestCancelHoldInvoice(t *testing.T) { + t.Parallel() defer timeout()() testClock := clock.NewTestClock(testTime) @@ -593,6 +601,7 @@ func TestCancelHoldInvoice(t *testing.T) { // if we are the exit hop, but in htlcIncomingContestResolver it is called with // forwarded htlc hashes as well. func TestUnknownInvoice(t *testing.T) { + t.Parallel() ctx := newTestContext(t, nil) // Notify arrival of a new htlc paying to this invoice. This should @@ -613,6 +622,8 @@ func TestUnknownInvoice(t *testing.T) { // TestKeySend tests receiving a spontaneous payment with and without keysend // enabled. func TestKeySend(t *testing.T) { + t.Parallel() + t.Run("enabled", func(t *testing.T) { testKeySend(t, true) }) @@ -624,6 +635,7 @@ func TestKeySend(t *testing.T) { // testKeySend is the inner test function that tests keysend for a particular // enabled state on the receiver end. func testKeySend(t *testing.T, keySendEnabled bool) { + t.Parallel() defer timeout()() cfg := defaultRegistryConfig() @@ -738,6 +750,8 @@ func testKeySend(t *testing.T, keySendEnabled bool) { // TestHoldKeysend tests receiving a spontaneous payment that is held. func TestHoldKeysend(t *testing.T) { + t.Parallel() + t.Run("settle", func(t *testing.T) { testHoldKeysend(t, false) }) @@ -748,6 +762,7 @@ func TestHoldKeysend(t *testing.T) { // testHoldKeysend is the inner test function that tests hold-keysend. func testHoldKeysend(t *testing.T, timeoutKeysend bool) { + t.Parallel() defer timeout()() const holdDuration = time.Minute @@ -838,6 +853,7 @@ func testHoldKeysend(t *testing.T, timeoutKeysend bool) { // It covers the case where there is a mpp timeout before the whole invoice is // paid and the case where the invoice is settled in time. func TestMppPayment(t *testing.T) { + t.Parallel() defer timeout()() ctx := newTestContext(t, nil) @@ -1199,6 +1215,8 @@ func TestOldInvoiceRemovalOnStart(t *testing.T) { // invoice is settled before expiry (and thus not canceled), and the case // where the invoice is expired. func TestHeightExpiryWithRegistry(t *testing.T) { + t.Parallel() + t.Run("single shot settled before expiry", func(t *testing.T) { testHeightExpiryWithRegistry(t, 1, true) }) @@ -1613,6 +1631,7 @@ func TestSettleInvoicePaymentAddrRequiredOptionalGrace(t *testing.T) { // TestAMPWithoutMPPPayload asserts that we correctly reject an AMP HTLC that // does not include an MPP record. func TestAMPWithoutMPPPayload(t *testing.T) { + t.Parallel() defer timeout()() cfg := defaultRegistryConfig() @@ -1645,6 +1664,8 @@ func TestAMPWithoutMPPPayload(t *testing.T) { // TestSpontaneousAmpPayment tests receiving a spontaneous AMP payment with both // valid and invalid reconstructions. func TestSpontaneousAmpPayment(t *testing.T) { + t.Parallel() + tests := []struct { name string ampEnabled bool @@ -1698,6 +1719,7 @@ func TestSpontaneousAmpPayment(t *testing.T) { func testSpontaneousAmpPayment( t *testing.T, ampEnabled, failReconstruction bool, numShards int) { + t.Parallel() defer timeout()() cfg := defaultRegistryConfig()