diff --git a/channeldb/payment_control.go b/channeldb/payment_control.go index 4ea0a9545..7bd00e4d4 100644 --- a/channeldb/payment_control.go +++ b/channeldb/payment_control.go @@ -223,6 +223,19 @@ func (p *PaymentControl) InitPayment(paymentHash lntypes.Hash, return updateErr } +// DeleteFailedAttempts deletes all failed htlcs for a payment if configured +// by the PaymentControl db. +func (p *PaymentControl) DeleteFailedAttempts(hash lntypes.Hash) error { + if !p.db.keepFailedPaymentAttempts { + const failedHtlcsOnly = true + err := p.db.DeletePayment(hash, failedHtlcsOnly) + if err != nil { + return err + } + } + return nil +} + // paymentIndexTypeHash is a payment index type which indicates that we have // created an index of payment sequence number to payment hash. type paymentIndexType uint8 diff --git a/channeldb/payment_control_test.go b/channeldb/payment_control_test.go index c79151cb2..0417238b7 100644 --- a/channeldb/payment_control_test.go +++ b/channeldb/payment_control_test.go @@ -308,7 +308,7 @@ func TestPaymentControlFailsWithoutInFlight(t *testing.T) { // TestPaymentControlDeleteNonInFlight checks that calling DeletePayments only // deletes payments from the database that are not in-flight. -func TestPaymentControlDeleteNonInFligt(t *testing.T) { +func TestPaymentControlDeleteNonInFlight(t *testing.T) { t.Parallel() db, cleanup, err := MakeTestDB() @@ -528,7 +528,7 @@ func TestPaymentControlDeletePayments(t *testing.T) { // Register three payments: // 1. A payment with two failed attempts. - // 2. A Payment with one failed and one settled attempt. + // 2. A payment with one failed and one settled attempt. // 3. A payment with one failed and one in-flight attempt. payments := []*payment{ {status: StatusFailed}, @@ -585,7 +585,7 @@ func TestPaymentControlDeleteSinglePayment(t *testing.T) { // according to its final status. // 1. A payment with two failed attempts. // 2. Another payment with two failed attempts. - // 3. A Payment with one failed and one settled attempt. + // 3. A payment with one failed and one settled attempt. // 4. A payment with one failed and one in-flight attempt. // Initiate payments, which is a slice of payment that is used as @@ -1003,6 +1003,96 @@ func TestPaymentControlMPPRecordValidation(t *testing.T) { } } +// TestDeleteFailedAttempts checks that DeleteFailedAttempts properly removes +// failed HTLCs from finished payments. +func TestDeleteFailedAttempts(t *testing.T) { + t.Parallel() + + t.Run("keep failed payment attempts", func(t *testing.T) { + testDeleteFailedAttempts(t, true) + }) + t.Run("remove failed payment attempts", func(t *testing.T) { + testDeleteFailedAttempts(t, false) + }) +} + +func testDeleteFailedAttempts(t *testing.T, keepFailedPaymentAttempts bool) { + db, cleanup, err := MakeTestDB() + defer cleanup() + + require.NoError(t, err, "unable to init db") + db.keepFailedPaymentAttempts = keepFailedPaymentAttempts + + pControl := NewPaymentControl(db) + + // Register three payments: + // All payments will have one failed HTLC attempt and one HTLC attempt + // according to its final status. + // 1. A payment with two failed attempts. + // 2. A payment with one failed and one in-flight attempt. + // 3. A payment with one failed and one settled attempt. + + // Initiate payments, which is a slice of payment that is used as + // template to create the corresponding test payments in the database. + // + // Note: The payment id and number of htlc attempts of each payment will + // be added to this slice when creating the payments below. + // This allows the slice to be used directly for testing purposes. + payments := []*payment{ + {status: StatusFailed}, + {status: StatusInFlight}, + {status: StatusSucceeded}, + } + + // Use helper function to register the test payments in the data and + // populate the data to the payments slice. + createTestPayments(t, pControl, payments) + + // Check that all payments are there as we added them. + assertPayments(t, db, payments) + + // Calling DeleteFailedAttempts on a failed payment should delete all + // HTLCs. + require.NoError(t, pControl.DeleteFailedAttempts(payments[0].id)) + + // Expect all HTLCs to be deleted if the config is set to delete them. + if !keepFailedPaymentAttempts { + payments[0].htlcs = 0 + } + assertPayments(t, db, payments) + + // Calling DeleteFailedAttempts on an in-flight payment should return + // an error. + if keepFailedPaymentAttempts { + require.NoError(t, pControl.DeleteFailedAttempts(payments[1].id)) + } else { + require.Error(t, pControl.DeleteFailedAttempts(payments[1].id)) + } + + // Since DeleteFailedAttempts returned an error, we should expect the + // payment to be unchanged. + assertPayments(t, db, payments) + + // Cleaning up a successful payment should remove failed htlcs. + require.NoError(t, pControl.DeleteFailedAttempts(payments[2].id)) + // Expect all HTLCs except for the settled one to be deleted if the + // config is set to delete them. + if !keepFailedPaymentAttempts { + payments[2].htlcs = 1 + } + assertPayments(t, db, payments) + + if keepFailedPaymentAttempts { + // DeleteFailedAttempts is ignored, even for non-existent + // payments, if the control tower is configured to keep failed + // HTLCs. + require.NoError(t, pControl.DeleteFailedAttempts(lntypes.ZeroHash)) + } else { + // Attempting to cleanup a non-existent payment returns an error. + require.Error(t, pControl.DeleteFailedAttempts(lntypes.ZeroHash)) + } +} + // assertPaymentStatus retrieves the status of the payment referred to by hash // and compares it with the expected state. func assertPaymentStatus(t *testing.T, p *PaymentControl, diff --git a/routing/control_tower.go b/routing/control_tower.go index 19b5ab504..be6e61b4e 100644 --- a/routing/control_tower.go +++ b/routing/control_tower.go @@ -19,6 +19,11 @@ type ControlTower interface { // hash. InitPayment(lntypes.Hash, *channeldb.PaymentCreationInfo) error + // DeleteFailedAttempts removes all failed HTLCs from the db. It should + // be called for a given payment whenever all inflight htlcs are + // completed, and the payment has reached a final settled state. + DeleteFailedAttempts(lntypes.Hash) error + // RegisterAttempt atomically records the provided HTLCAttemptInfo. RegisterAttempt(lntypes.Hash, *channeldb.HTLCAttemptInfo) error @@ -125,6 +130,12 @@ func (p *controlTower) InitPayment(paymentHash lntypes.Hash, return p.db.InitPayment(paymentHash, info) } +// DeleteFailedAttempts deletes all failed htlcs if the payment was +// successfully settled. +func (p *controlTower) DeleteFailedAttempts(paymentHash lntypes.Hash) error { + return p.db.DeleteFailedAttempts(paymentHash) +} + // RegisterAttempt atomically records the provided HTLCAttemptInfo to the // DB. func (p *controlTower) RegisterAttempt(paymentHash lntypes.Hash, diff --git a/routing/mock_test.go b/routing/mock_test.go index 1bc09a5a9..67f6fa432 100644 --- a/routing/mock_test.go +++ b/routing/mock_test.go @@ -309,6 +309,32 @@ func (m *mockControlTowerOld) InitPayment(phash lntypes.Hash, return nil } +func (m *mockControlTowerOld) DeleteFailedAttempts(phash lntypes.Hash) error { + p, ok := m.payments[phash] + if !ok { + return channeldb.ErrPaymentNotInitiated + } + + var inFlight bool + for _, a := range p.attempts { + if a.Settle != nil { + continue + } + + if a.Failure != nil { + continue + } + + inFlight = true + } + + if inFlight { + return channeldb.ErrPaymentInFlight + } + + return nil +} + func (m *mockControlTowerOld) RegisterAttempt(phash lntypes.Hash, a *channeldb.HTLCAttemptInfo) error { @@ -665,6 +691,11 @@ func (m *mockControlTower) InitPayment(phash lntypes.Hash, return args.Error(0) } +func (m *mockControlTower) DeleteFailedAttempts(phash lntypes.Hash) error { + args := m.Called(phash) + return args.Error(0) +} + func (m *mockControlTower) RegisterAttempt(phash lntypes.Hash, a *channeldb.HTLCAttemptInfo) error { diff --git a/routing/payment_lifecycle.go b/routing/payment_lifecycle.go index b2022cf2f..36c63eb14 100644 --- a/routing/payment_lifecycle.go +++ b/routing/payment_lifecycle.go @@ -186,9 +186,21 @@ lifecycle: // Find the first successful shard and return // the preimage and route. for _, a := range payment.HTLCs { - if a.Settle != nil { - return a.Settle.Preimage, &a.Route, nil + if a.Settle == nil { + continue } + + err := p.router.cfg.Control. + DeleteFailedAttempts( + p.identifier) + if err != nil { + log.Errorf("Error deleting failed "+ + "payment attempts for "+ + "payment %v: %v", p.identifier, + err) + } + + return a.Settle.Preimage, &a.Route, nil } // Payment failed. diff --git a/routing/router_test.go b/routing/router_test.go index 343348a02..4ffee7be7 100644 --- a/routing/router_test.go +++ b/routing/router_test.go @@ -3571,6 +3571,7 @@ func TestSendMPPaymentSucceed(t *testing.T) { } } }) + controlTower.On("DeleteFailedAttempts", identifier).Return(nil) // Call the actual method SendPayment on router. This is place inside a // goroutine so we can set a timeout for the whole test, in case @@ -3782,6 +3783,7 @@ func TestSendMPPaymentSucceedOnExtraShards(t *testing.T) { return } }) + controlTower.On("DeleteFailedAttempts", identifier).Return(nil) // Call the actual method SendPayment on router. This is place inside a // goroutine so we can set a timeout for the whole test, in case