[Part 7|*] Addressing TODOs left from the SQL payments implementation

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,31 +4,19 @@ 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,
 	}
 }
 
 // OptionModifier is a function signature for modifying the default
 // 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,19 +1731,22 @@ 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)
 
 		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, 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("register attempt", func(t *testing.T) {
+		testKVStoreSubscribeFail(t, 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)