multi: move failed attempt cfg option to the router subsytem

Previously we had db and application logic mixed on the db level.
We now move the config option KeepFailedPaymentAttempts to the
ChannelRouter level and move it out of the db level.

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()

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

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 {

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")

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 {

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)

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()

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.
 //

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.

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)