diff --git a/config_builder.go b/config_builder.go index 07196b21c..74f8b3afb 100644 --- a/config_builder.go +++ b/config_builder.go @@ -1236,9 +1236,6 @@ func (d *DefaultDatabaseBuilder) BuildDatabase( // will build a SQL payments backend. sqlPaymentsDB, err := d.getPaymentsStore( baseDB, dbs.ChanStateDB.Backend, - paymentsdb.WithKeepFailedPaymentAttempts( - cfg.KeepFailedPaymentAttempts, - ), ) if err != nil { err = fmt.Errorf("unable to get payments store: %w", @@ -1280,9 +1277,6 @@ func (d *DefaultDatabaseBuilder) BuildDatabase( // Create the payments DB. kvPaymentsDB, err := paymentsdb.NewKVStore( dbs.ChanStateDB, - paymentsdb.WithKeepFailedPaymentAttempts( - cfg.KeepFailedPaymentAttempts, - ), ) if err != nil { cleanUp() diff --git a/payments/db/kv_store.go b/payments/db/kv_store.go index 0ce0601e4..6d21048d7 100644 --- a/payments/db/kv_store.go +++ b/payments/db/kv_store.go @@ -127,10 +127,6 @@ type KVStore struct { // db is the underlying database implementation. db kvdb.Backend - - // keepFailedPaymentAttempts is a flag that indicates whether we should - // keep failed payment attempts in the database. - keepFailedPaymentAttempts bool } // A compile-time constraint to ensure KVStore implements DB. @@ -152,8 +148,7 @@ func NewKVStore(db kvdb.Backend, } return &KVStore{ - db: db, - keepFailedPaymentAttempts: opts.KeepFailedPaymentAttempts, + db: db, }, nil } @@ -288,19 +283,14 @@ func (p *KVStore) InitPayment(_ context.Context, paymentHash lntypes.Hash, return updateErr } -// DeleteFailedAttempts deletes all failed htlcs for a payment if configured -// by the KVStore db. +// DeleteFailedAttempts deletes all failed htlcs for a payment. func (p *KVStore) DeleteFailedAttempts(ctx context.Context, hash lntypes.Hash) error { - // TODO(ziggie): Refactor to not mix application logic with database - // logic. This decision should be made in the application layer. - if !p.keepFailedPaymentAttempts { - const failedHtlcsOnly = true - err := p.DeletePayment(ctx, hash, failedHtlcsOnly) - if err != nil { - return err - } + const failedHtlcsOnly = true + err := p.DeletePayment(ctx, hash, failedHtlcsOnly) + if err != nil { + return err } return nil diff --git a/payments/db/options.go b/payments/db/options.go index 9e98aafa3..efceb2f9b 100644 --- a/payments/db/options.go +++ b/payments/db/options.go @@ -4,17 +4,12 @@ package paymentsdb type StoreOptions struct { // NoMigration allows to open the database in readonly mode NoMigration bool - - // KeepFailedPaymentAttempts is a flag that determines whether to keep - // failed payment attempts for a settled payment in the db. - KeepFailedPaymentAttempts bool } // DefaultOptions returns a StoreOptions populated with default values. func DefaultOptions() *StoreOptions { return &StoreOptions{ - KeepFailedPaymentAttempts: false, - NoMigration: false, + NoMigration: false, } } @@ -22,13 +17,6 @@ func DefaultOptions() *StoreOptions { // StoreOptions. type OptionModifier func(*StoreOptions) -// WithKeepFailedPaymentAttempts sets the KeepFailedPaymentAttempts to n. -func WithKeepFailedPaymentAttempts(n bool) OptionModifier { - return func(o *StoreOptions) { - o.KeepFailedPaymentAttempts = n - } -} - // WithNoMigration allows the database to be opened in read only mode by // disabling migrations. func WithNoMigration(b bool) OptionModifier { diff --git a/payments/db/payment_test.go b/payments/db/payment_test.go index 581ac2c19..668ac4c1d 100644 --- a/payments/db/payment_test.go +++ b/payments/db/payment_test.go @@ -460,20 +460,7 @@ func genInfo(t *testing.T) (*PaymentCreationInfo, lntypes.Preimage, error) { 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) - }) -} - -// testDeleteFailedAttempts tests the DeleteFailedAttempts method with the -// given keepFailedPaymentAttempts flag as argument. -func testDeleteFailedAttempts(t *testing.T, keepFailedPaymentAttempts bool) { - paymentDB, _ := NewTestDB( - t, WithKeepFailedPaymentAttempts(keepFailedPaymentAttempts), - ) + paymentDB, _ := NewTestDB(t) // Register three payments: // All payments will have one failed HTLC attempt and one HTLC attempt @@ -507,29 +494,16 @@ func testDeleteFailedAttempts(t *testing.T, keepFailedPaymentAttempts bool) { t.Context(), payments[0].id, )) - // Expect all HTLCs to be deleted if the config is set to delete them. - if !keepFailedPaymentAttempts { - payments[0].htlcs = 0 - } + // Expect all HTLCs to be deleted. + payments[0].htlcs = 0 assertDBPayments(t, paymentDB, payments) // Calling DeleteFailedAttempts on an in-flight payment should return // an error. - // - // NOTE: In case the option keepFailedPaymentAttempts is set no delete - // operation are performed in general therefore we do NOT expect an - // error in this case. - if keepFailedPaymentAttempts { - err := paymentDB.DeleteFailedAttempts( - t.Context(), payments[1].id, - ) - require.NoError(t, err) - } else { - err := paymentDB.DeleteFailedAttempts( - t.Context(), payments[1].id, - ) - require.Error(t, err) - } + err := paymentDB.DeleteFailedAttempts( + t.Context(), payments[1].id, + ) + require.Error(t, err) // Since DeleteFailedAttempts returned an error, we should expect the // payment to be unchanged. @@ -540,34 +514,16 @@ func testDeleteFailedAttempts(t *testing.T, keepFailedPaymentAttempts bool) { t.Context(), 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 - } + // Expect all HTLCs except for the settled one to be deleted. + payments[2].htlcs = 1 assertDBPayments(t, paymentDB, payments) - // NOTE: In case the option keepFailedPaymentAttempts is set no delete - // operation are performed in general therefore we do NOT expect an - // error in this case. - if keepFailedPaymentAttempts { - // DeleteFailedAttempts is ignored, even for non-existent - // payments, if the control tower is configured to keep failed - // HTLCs. - require.NoError( - t, paymentDB.DeleteFailedAttempts( - t.Context(), lntypes.ZeroHash, - ), - ) - } else { - // Attempting to cleanup a non-existent payment returns an - // error. - require.Error( - t, paymentDB.DeleteFailedAttempts( - t.Context(), lntypes.ZeroHash, - ), - ) - } + // Attempting to cleanup a non-existent payment returns an error. + require.Error( + t, paymentDB.DeleteFailedAttempts( + t.Context(), lntypes.ZeroHash, + ), + ) } // TestMPPRecordValidation tests MPP record validation. @@ -1754,6 +1710,11 @@ func TestDeleteNonInFlight(t *testing.T) { paymentDB, _ := NewTestDB(t) + var ( + numSuccess, numInflight int + attemptID uint64 = 0 + ) + // Create payments with different statuses: failed, success, inflight, // and another success. payments := []struct { @@ -1770,8 +1731,6 @@ func TestDeleteNonInFlight(t *testing.T) { {failed: false, success: true}, } - var numSuccess, numInflight int - for _, p := range payments { preimg, err := genPreimage(t) require.NoError(t, err) @@ -1779,10 +1738,15 @@ func TestDeleteNonInFlight(t *testing.T) { rhash := sha256.Sum256(preimg[:]) info := genPaymentCreationInfo(t, rhash) attempt, err := genAttemptWithHash( - t, 0, genSessionKey(t), rhash, + t, attemptID, genSessionKey(t), rhash, ) require.NoError(t, err) + // After generating the attempt, increment the attempt ID to + // have unique attempt IDs for each attempt otherwise the unique + // constraint on the attempt ID will be violated. + attemptID++ + // Init payment which initiates StatusInFlight. err = paymentDB.InitPayment(ctx, info.PaymentIdentifier, info) require.NoError(t, err, "unable to init payment") diff --git a/payments/db/sql_store.go b/payments/db/sql_store.go index d23e80895..1c6e3042a 100644 --- a/payments/db/sql_store.go +++ b/payments/db/sql_store.go @@ -99,10 +99,6 @@ type BatchedSQLQueries interface { type SQLStore struct { cfg *SQLStoreConfig db BatchedSQLQueries - - // keepFailedPaymentAttempts is a flag that indicates whether we should - // keep failed payment attempts in the database. - keepFailedPaymentAttempts bool } // A compile-time constraint to ensure SQLStore implements DB. @@ -130,9 +126,8 @@ func NewSQLStore(cfg *SQLStoreConfig, db BatchedSQLQueries, } return &SQLStore{ - cfg: cfg, - db: db, - keepFailedPaymentAttempts: opts.KeepFailedPaymentAttempts, + cfg: cfg, + db: db, }, nil } @@ -1094,10 +1089,6 @@ func (s *SQLStore) FetchInFlightPayments(ctx context.Context) ([]*MPPayment, // - StatusSucceeded: Can delete failed attempts (payment completed) // - StatusFailed: Can delete failed attempts (payment permanently failed) // -// If the keepFailedPaymentAttempts configuration flag is enabled, this method -// returns immediately without deleting anything, allowing failed attempts to -// be retained for debugging or auditing purposes. -// // This method is idempotent - calling it multiple times on the same payment // has no adverse effects. // @@ -1109,15 +1100,6 @@ func (s *SQLStore) FetchInFlightPayments(ctx context.Context) ([]*MPPayment, func (s *SQLStore) DeleteFailedAttempts(ctx context.Context, paymentHash lntypes.Hash) error { - // In case we are configured to keep failed payment attempts, we exit - // early. - // - // TODO(ziggie): Refactor to not mix application logic with database - // logic. This decision should be made in the application layer. - if s.keepFailedPaymentAttempts { - return nil - } - err := s.db.ExecTx(ctx, sqldb.WriteTxOpt(), func(db SQLQueries) error { dbPayment, err := fetchPaymentByHash(ctx, db, paymentHash) if err != nil { diff --git a/routing/control_tower_test.go b/routing/control_tower_test.go index c9e8f4857..697770ff5 100644 --- a/routing/control_tower_test.go +++ b/routing/control_tower_test.go @@ -50,10 +50,7 @@ func TestControlTowerSubscribeUnknown(t *testing.T) { db := initDB(t) - paymentDB, err := paymentsdb.NewKVStore( - db, - paymentsdb.WithKeepFailedPaymentAttempts(true), - ) + paymentDB, err := paymentsdb.NewKVStore(db) require.NoError(t, err) pControl := NewControlTower(paymentDB) @@ -182,17 +179,11 @@ func TestControlTowerSubscribeSuccess(t *testing.T) { func TestKVStoreSubscribeFail(t *testing.T) { t.Parallel() - t.Run("register attempt, keep failed payments", func(t *testing.T) { - testKVStoreSubscribeFail(t, true, true) + t.Run("register attempt", func(t *testing.T) { + testKVStoreSubscribeFail(t, true) }) - t.Run("register attempt, delete failed payments", func(t *testing.T) { - testKVStoreSubscribeFail(t, true, false) - }) - t.Run("no register attempt, keep failed payments", func(t *testing.T) { - testKVStoreSubscribeFail(t, false, true) - }) - t.Run("no register attempt, delete failed payments", func(t *testing.T) { - testKVStoreSubscribeFail(t, false, false) + t.Run("no register attempt", func(t *testing.T) { + testKVStoreSubscribeFail(t, false) }) } @@ -203,10 +194,7 @@ func TestKVStoreSubscribeAllSuccess(t *testing.T) { db := initDB(t) - paymentDB, err := paymentsdb.NewKVStore( - db, - paymentsdb.WithKeepFailedPaymentAttempts(true), - ) + paymentDB, err := paymentsdb.NewKVStore(db) require.NoError(t, err) pControl := NewControlTower(paymentDB) @@ -334,10 +322,7 @@ func TestKVStoreSubscribeAllImmediate(t *testing.T) { db := initDB(t) - paymentDB, err := paymentsdb.NewKVStore( - db, - paymentsdb.WithKeepFailedPaymentAttempts(true), - ) + paymentDB, err := paymentsdb.NewKVStore(db) require.NoError(t, err) pControl := NewControlTower(paymentDB) @@ -385,10 +370,7 @@ func TestKVStoreUnsubscribeSuccess(t *testing.T) { db := initDB(t) - paymentDB, err := paymentsdb.NewKVStore( - db, - paymentsdb.WithKeepFailedPaymentAttempts(true), - ) + paymentDB, err := paymentsdb.NewKVStore(db) require.NoError(t, err) pControl := NewControlTower(paymentDB) @@ -458,17 +440,10 @@ func TestKVStoreUnsubscribeSuccess(t *testing.T) { require.Len(t, subscription2.Updates(), 0) } -func testKVStoreSubscribeFail(t *testing.T, registerAttempt, - keepFailedPaymentAttempts bool) { - +func testKVStoreSubscribeFail(t *testing.T, registerAttempt bool) { db := initDB(t) - paymentDB, err := paymentsdb.NewKVStore( - db, - paymentsdb.WithKeepFailedPaymentAttempts( - keepFailedPaymentAttempts, - ), - ) + paymentDB, err := paymentsdb.NewKVStore(db) require.NoError(t, err) pControl := NewControlTower(paymentDB) diff --git a/routing/payment_lifecycle.go b/routing/payment_lifecycle.go index 9be86dc1b..488df5b79 100644 --- a/routing/payment_lifecycle.go +++ b/routing/payment_lifecycle.go @@ -338,15 +338,16 @@ lifecycle: // terminal condition. We either return the settled preimage or the // payment's failure reason. // - // Optionally delete the failed attempts from the database. Depends on - // the database options deleting attempts is not allowed so this will - // just be a no-op. - err = p.router.cfg.Control.DeleteFailedAttempts( - cleanupCtx, p.identifier, - ) - if err != nil { - log.Errorf("Error deleting failed htlc attempts for payment "+ - "%v: %v", p.identifier, err) + // Optionally delete the failed attempts from the database. If we are + // configured to keep failed payment attempts, we skip deletion. + if !p.router.cfg.KeepFailedPaymentAttempts { + err = p.router.cfg.Control.DeleteFailedAttempts( + cleanupCtx, p.identifier, + ) + if err != nil { + log.Errorf("Error deleting failed htlc attempts "+ + "for payment %v: %v", p.identifier, err) + } } htlc, failure := payment.TerminalInfo() diff --git a/routing/payment_lifecycle_test.go b/routing/payment_lifecycle_test.go index 82e2f800d..564942d32 100644 --- a/routing/payment_lifecycle_test.go +++ b/routing/payment_lifecycle_test.go @@ -1280,6 +1280,156 @@ func TestResumePaymentSuccess(t *testing.T) { require.Equal(t, 1, m.collectResultsCount) } +// TestKeepFailedPaymentAttempts tests that DeleteFailedAttempts is +// called or skipped based on the KeepFailedPaymentAttempts +// configuration of the router. +func TestKeepFailedPaymentAttempts(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + keepFailedPaymentAttempts bool + expectDeleteCalled bool + }{ + { + name: "keep failed attempts - " + + "delete not called", + keepFailedPaymentAttempts: true, + expectDeleteCalled: false, + }, + { + name: "delete failed attempts - " + + "delete called", + keepFailedPaymentAttempts: false, + expectDeleteCalled: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + // Create a test paymentLifecycle with the initial two + // calls mocked. + p, m := setupTestPaymentLifecycle(t) + + // Set the KeepFailedPaymentAttempts configuration. + p.router.cfg.KeepFailedPaymentAttempts = + tc.keepFailedPaymentAttempts + + // Create a dummy route that will be returned by + // `RequestRoute`. + paymentAmt := lnwire.MilliSatoshi(10000) + rt := createDummyRoute(t, paymentAmt) + + // We now enter the payment lifecycle loop. + // + // 1.1. calls `FetchPayment` and return the payment. + m.control.On("FetchPayment", p.identifier). + Return(m.payment, nil).Once() + + // 1.2. calls `GetState` and return the state. + ps := &paymentsdb.MPPaymentState{ + RemainingAmt: paymentAmt, + } + m.payment.On("GetState").Return(ps).Once() + + // NOTE: GetStatus is only used to populate the logs + // which is not critical so we loosen the checks on how + // many times it's been called. + m.payment.On("GetStatus"). + Return(paymentsdb.StatusInFlight) + + // 1.3. decideNextStep now returns stepProceed. + m.payment.On("AllowMoreAttempts"). + Return(true, nil).Once() + + // 1.4. mock requestRoute to return an route. + m.paySession.On("RequestRoute", + paymentAmt, p.feeLimit, + uint32(ps.NumAttemptsInFlight), + uint32(p.currentHeight), mock.Anything, + ).Return(rt, nil).Once() + + // 1.5. mock `registerAttempt` to return an attempt. + // + // Mock NextPaymentID to always return the attemptID. + attemptID := uint64(1) + p.router.cfg.NextPaymentID = func() (uint64, error) { + return attemptID, nil + } + + // Mock shardTracker to return the mock shard. + m.shardTracker.On("NewShard", + attemptID, true, + ).Return(m.shard, nil).Once() + + // Mock the methods on the shard. + m.shard.On("MPP").Return(&record.MPP{}).Twice(). + On("AMP").Return(nil).Once(). + On("Hash").Return(p.identifier).Once() + + // Mock the time and expect it to be called. + m.clock.On("Now").Return(time.Now()) + + // We now register attempt and return no error. + m.control.On("RegisterAttempt", + p.identifier, mock.Anything, + ).Return(nil).Once() + + // 1.6. mock `sendAttempt` to succeed, which brings us + // into the next iteration of the lifecycle. + m.payer.On("SendHTLC", + mock.Anything, attemptID, mock.Anything, + ).Return(nil).Once() + + // We now enter the second iteration of the lifecycle + // loop. + // + // 2.1. calls `FetchPayment` and return the payment. + m.control.On("FetchPayment", p.identifier). + Return(m.payment, nil).Once() + + // 2.2. calls `GetState` and return the state. + m.payment.On("GetState").Return(ps). + Run(func(args mock.Arguments) { + ps.RemainingAmt = 0 + }).Once() + + // 2.3. decideNextStep now returns stepExit and exits + // the loop. + m.payment.On("AllowMoreAttempts"). + Return(false, nil).Once(). + On("NeedWaitAttempts").Return(false, nil).Once() + + // Conditionally expect DeleteFailedAttempts to be + // called based on the configuration. + if tc.expectDeleteCalled { + m.control.On("DeleteFailedAttempts", + p.identifier).Return(nil).Once() + } + // If expectDeleteCalled is false, we don't set up the + // expectation, which means the mock will fail if it's + // called. + + // Finally, mock the `TerminalInfo` to return the + // settled attempt. Create a SettleAttempt. + testPreimage := lntypes.Preimage{1, 2, 3} + settledAttempt := makeSettledAttempt( + t, int(paymentAmt), testPreimage, + ) + m.payment.On("TerminalInfo"). + Return(settledAttempt, nil).Once() + + // Send the payment and assert the preimage is matched. + sendPaymentAndAssertSucceeded(t, p, testPreimage) + + // Expected collectResultAsync to called. + require.Equal(t, 1, m.collectResultsCount) + }) + } +} + // TestResumePaymentSuccessWithTwoAttempts checks a successful payment flow // with two HTLC attempts. // diff --git a/routing/router.go b/routing/router.go index c17aa4176..37aeef223 100644 --- a/routing/router.go +++ b/routing/router.go @@ -295,6 +295,10 @@ type Config struct { // TrafficShaper is an optional traffic shaper that can be used to // control the outgoing channel of a payment. TrafficShaper fn.Option[htlcswitch.AuxTrafficShaper] + + // KeepFailedPaymentAttempts indicates whether to keep failed payment + // attempts in the database. + KeepFailedPaymentAttempts bool } // EdgeLocator is a struct used to identify a specific edge. diff --git a/server.go b/server.go index 91b662463..eaa5a528c 100644 --- a/server.go +++ b/server.go @@ -1023,20 +1023,21 @@ func newServer(ctx context.Context, cfg *Config, listenAddrs []net.Addr, } s.chanRouter, err = routing.New(routing.Config{ - SelfNode: nodePubKey, - RoutingGraph: dbs.GraphDB, - Chain: cc.ChainIO, - Payer: s.htlcSwitch, - Control: s.controlTower, - MissionControl: s.defaultMC, - SessionSource: paymentSessionSource, - GetLink: s.htlcSwitch.GetLinkByShortID, - NextPaymentID: sequencer.NextID, - PathFindingConfig: pathFindingConfig, - Clock: clock.NewDefaultClock(), - ApplyChannelUpdate: s.graphBuilder.ApplyChannelUpdate, - ClosedSCIDs: s.fetchClosedChannelSCIDs(), - TrafficShaper: implCfg.TrafficShaper, + SelfNode: nodePubKey, + RoutingGraph: dbs.GraphDB, + Chain: cc.ChainIO, + Payer: s.htlcSwitch, + Control: s.controlTower, + MissionControl: s.defaultMC, + SessionSource: paymentSessionSource, + GetLink: s.htlcSwitch.GetLinkByShortID, + NextPaymentID: sequencer.NextID, + PathFindingConfig: pathFindingConfig, + Clock: clock.NewDefaultClock(), + ApplyChannelUpdate: s.graphBuilder.ApplyChannelUpdate, + ClosedSCIDs: s.fetchClosedChannelSCIDs(), + TrafficShaper: implCfg.TrafficShaper, + KeepFailedPaymentAttempts: cfg.KeepFailedPaymentAttempts, }) if err != nil { return nil, fmt.Errorf("can't create router: %w", err)