Skip to content

Commit 4f1babb

Browse files
ziggie1984ziggie1984
committed
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.
1 parent b70343f commit 4f1babb

File tree

10 files changed

+213
-181
lines changed

10 files changed

+213
-181
lines changed

‎config_builder.go‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1236,9 +1236,6 @@ func (d *DefaultDatabaseBuilder) BuildDatabase(
12361236
// will build a SQL payments backend.
12371237
sqlPaymentsDB, err := d.getPaymentsStore(
12381238
baseDB, dbs.ChanStateDB.Backend,
1239-
paymentsdb.WithKeepFailedPaymentAttempts(
1240-
cfg.KeepFailedPaymentAttempts,
1241-
),
12421239
)
12431240
if err != nil {
12441241
err = fmt.Errorf("unable to get payments store: %w",
@@ -1280,9 +1277,6 @@ func (d *DefaultDatabaseBuilder) BuildDatabase(
12801277
// Create the payments DB.
12811278
kvPaymentsDB, err := paymentsdb.NewKVStore(
12821279
dbs.ChanStateDB,
1283-
paymentsdb.WithKeepFailedPaymentAttempts(
1284-
cfg.KeepFailedPaymentAttempts,
1285-
),
12861280
)
12871281
if err != nil {
12881282
cleanUp()

‎payments/db/kv_store.go‎

Lines changed: 6 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -127,10 +127,6 @@ type KVStore struct {
127127

128128
// db is the underlying database implementation.
129129
db kvdb.Backend
130-
131-
// keepFailedPaymentAttempts is a flag that indicates whether we should
132-
// keep failed payment attempts in the database.
133-
keepFailedPaymentAttempts bool
134130
}
135131

136132
// A compile-time constraint to ensure KVStore implements DB.
@@ -152,8 +148,7 @@ func NewKVStore(db kvdb.Backend,
152148
}
153149

154150
return &KVStore{
155-
db: db,
156-
keepFailedPaymentAttempts: opts.KeepFailedPaymentAttempts,
151+
db: db,
157152
}, nil
158153
}
159154

@@ -288,19 +283,14 @@ func (p *KVStore) InitPayment(_ context.Context, paymentHash lntypes.Hash,
288283
return updateErr
289284
}
290285

291-
// DeleteFailedAttempts deletes all failed htlcs for a payment if configured
292-
// by the KVStore db.
286+
// DeleteFailedAttempts deletes all failed htlcs for a payment.
293287
func (p *KVStore) DeleteFailedAttempts(ctx context.Context,
294288
hash lntypes.Hash) error {
295289

296-
// TODO(ziggie): Refactor to not mix application logic with database
297-
// logic. This decision should be made in the application layer.
298-
if !p.keepFailedPaymentAttempts {
299-
const failedHtlcsOnly = true
300-
err := p.DeletePayment(ctx, hash, failedHtlcsOnly)
301-
if err != nil {
302-
return err
303-
}
290+
const failedHtlcsOnly = true
291+
err := p.DeletePayment(ctx, hash, failedHtlcsOnly)
292+
if err != nil {
293+
return err
304294
}
305295

306296
return nil

‎payments/db/options.go‎

Lines changed: 1 addition & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -4,31 +4,19 @@ package paymentsdb
44
type StoreOptions struct {
55
// NoMigration allows to open the database in readonly mode
66
NoMigration bool
7-
8-
// KeepFailedPaymentAttempts is a flag that determines whether to keep
9-
// failed payment attempts for a settled payment in the db.
10-
KeepFailedPaymentAttempts bool
117
}
128

139
// DefaultOptions returns a StoreOptions populated with default values.
1410
func DefaultOptions() *StoreOptions {
1511
return &StoreOptions{
16-
KeepFailedPaymentAttempts: false,
17-
NoMigration: false,
12+
NoMigration: false,
1813
}
1914
}
2015

2116
// OptionModifier is a function signature for modifying the default
2217
// StoreOptions.
2318
type OptionModifier func(*StoreOptions)
2419

25-
// WithKeepFailedPaymentAttempts sets the KeepFailedPaymentAttempts to n.
26-
func WithKeepFailedPaymentAttempts(n bool) OptionModifier {
27-
return func(o *StoreOptions) {
28-
o.KeepFailedPaymentAttempts = n
29-
}
30-
}
31-
3220
// WithNoMigration allows the database to be opened in read only mode by
3321
// disabling migrations.
3422
func WithNoMigration(b bool) OptionModifier {

‎payments/db/payment_test.go‎

Lines changed: 15 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -460,20 +460,7 @@ func genInfo(t *testing.T) (*PaymentCreationInfo, lntypes.Preimage, error) {
460460
func TestDeleteFailedAttempts(t *testing.T) {
461461
t.Parallel()
462462

463-
t.Run("keep failed payment attempts", func(t *testing.T) {
464-
testDeleteFailedAttempts(t, true)
465-
})
466-
t.Run("remove failed payment attempts", func(t *testing.T) {
467-
testDeleteFailedAttempts(t, false)
468-
})
469-
}
470-
471-
// testDeleteFailedAttempts tests the DeleteFailedAttempts method with the
472-
// given keepFailedPaymentAttempts flag as argument.
473-
func testDeleteFailedAttempts(t *testing.T, keepFailedPaymentAttempts bool) {
474-
paymentDB, _ := NewTestDB(
475-
t, WithKeepFailedPaymentAttempts(keepFailedPaymentAttempts),
476-
)
463+
paymentDB, _ := NewTestDB(t)
477464

478465
// Register three payments:
479466
// All payments will have one failed HTLC attempt and one HTLC attempt
@@ -507,29 +494,16 @@ func testDeleteFailedAttempts(t *testing.T, keepFailedPaymentAttempts bool) {
507494
t.Context(), payments[0].id,
508495
))
509496

510-
// Expect all HTLCs to be deleted if the config is set to delete them.
511-
if !keepFailedPaymentAttempts {
512-
payments[0].htlcs = 0
513-
}
497+
// Expect all HTLCs to be deleted.
498+
payments[0].htlcs = 0
514499
assertDBPayments(t, paymentDB, payments)
515500

516501
// Calling DeleteFailedAttempts on an in-flight payment should return
517502
// an error.
518-
//
519-
// NOTE: In case the option keepFailedPaymentAttempts is set no delete
520-
// operation are performed in general therefore we do NOT expect an
521-
// error in this case.
522-
if keepFailedPaymentAttempts {
523-
err := paymentDB.DeleteFailedAttempts(
524-
t.Context(), payments[1].id,
525-
)
526-
require.NoError(t, err)
527-
} else {
528-
err := paymentDB.DeleteFailedAttempts(
529-
t.Context(), payments[1].id,
530-
)
531-
require.Error(t, err)
532-
}
503+
err := paymentDB.DeleteFailedAttempts(
504+
t.Context(), payments[1].id,
505+
)
506+
require.Error(t, err)
533507

534508
// Since DeleteFailedAttempts returned an error, we should expect the
535509
// payment to be unchanged.
@@ -540,34 +514,16 @@ func testDeleteFailedAttempts(t *testing.T, keepFailedPaymentAttempts bool) {
540514
t.Context(), payments[2].id,
541515
))
542516

543-
// Expect all HTLCs except for the settled one to be deleted if the
544-
// config is set to delete them.
545-
if !keepFailedPaymentAttempts {
546-
payments[2].htlcs = 1
547-
}
517+
// Expect all HTLCs except for the settled one to be deleted.
518+
payments[2].htlcs = 1
548519
assertDBPayments(t, paymentDB, payments)
549520

550-
// NOTE: In case the option keepFailedPaymentAttempts is set no delete
551-
// operation are performed in general therefore we do NOT expect an
552-
// error in this case.
553-
if keepFailedPaymentAttempts {
554-
// DeleteFailedAttempts is ignored, even for non-existent
555-
// payments, if the control tower is configured to keep failed
556-
// HTLCs.
557-
require.NoError(
558-
t, paymentDB.DeleteFailedAttempts(
559-
t.Context(), lntypes.ZeroHash,
560-
),
561-
)
562-
} else {
563-
// Attempting to cleanup a non-existent payment returns an
564-
// error.
565-
require.Error(
566-
t, paymentDB.DeleteFailedAttempts(
567-
t.Context(), lntypes.ZeroHash,
568-
),
569-
)
570-
}
521+
// Attempting to cleanup a non-existent payment returns an error.
522+
require.Error(
523+
t, paymentDB.DeleteFailedAttempts(
524+
t.Context(), lntypes.ZeroHash,
525+
),
526+
)
571527
}
572528

573529
// TestMPPRecordValidation tests MPP record validation.

‎payments/db/sql_store.go‎

Lines changed: 2 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -98,10 +98,6 @@ type BatchedSQLQueries interface {
9898
type SQLStore struct {
9999
cfg *SQLStoreConfig
100100
db BatchedSQLQueries
101-
102-
// keepFailedPaymentAttempts is a flag that indicates whether we should
103-
// keep failed payment attempts in the database.
104-
keepFailedPaymentAttempts bool
105101
}
106102

107103
// A compile-time constraint to ensure SQLStore implements DB.
@@ -129,9 +125,8 @@ func NewSQLStore(cfg *SQLStoreConfig, db BatchedSQLQueries,
129125
}
130126

131127
return &SQLStore{
132-
cfg: cfg,
133-
db: db,
134-
keepFailedPaymentAttempts: opts.KeepFailedPaymentAttempts,
128+
cfg: cfg,
129+
db: db,
135130
}, nil
136131
}
137132

@@ -1000,10 +995,6 @@ func (s *SQLStore) FetchInFlightPayments(ctx context.Context) ([]*MPPayment,
1000995
// - StatusSucceeded: Can delete failed attempts (payment completed)
1001996
// - StatusFailed: Can delete failed attempts (payment permanently failed)
1002997
//
1003-
// If the keepFailedPaymentAttempts configuration flag is enabled, this method
1004-
// returns immediately without deleting anything, allowing failed attempts to
1005-
// be retained for debugging or auditing purposes.
1006-
//
1007998
// This method is idempotent - calling it multiple times on the same payment
1008999
// has no adverse effects.
10091000
//
@@ -1015,15 +1006,6 @@ func (s *SQLStore) FetchInFlightPayments(ctx context.Context) ([]*MPPayment,
10151006
func (s *SQLStore) DeleteFailedAttempts(ctx context.Context,
10161007
paymentHash lntypes.Hash) error {
10171008

1018-
// In case we are configured to keep failed payment attempts, we exit
1019-
// early.
1020-
//
1021-
// TODO(ziggie): Refactor to not mix application logic with database
1022-
// logic. This decision should be made in the application layer.
1023-
if s.keepFailedPaymentAttempts {
1024-
return nil
1025-
}
1026-
10271009
err := s.db.ExecTx(ctx, sqldb.WriteTxOpt(), func(db SQLQueries) error {
10281010
dbPayment, err := fetchPaymentByHash(ctx, db, paymentHash)
10291011
if err != nil {
@@ -1109,15 +1091,6 @@ func (s *SQLStore) computePaymentStatusFromDB(ctx context.Context,
11091091
func (s *SQLStore) DeletePayment(ctx context.Context, paymentHash lntypes.Hash,
11101092
failedHtlcsOnly bool) error {
11111093

1112-
// In case we are configured to keep failed payment attempts, we exit
1113-
// early.
1114-
//
1115-
// TODO(ziggie): Refactor to not mix application logic with database
1116-
// logic. This decision should be made in the application layer.
1117-
if s.keepFailedPaymentAttempts {
1118-
return nil
1119-
}
1120-
11211094
err := s.db.ExecTx(ctx, sqldb.WriteTxOpt(), func(db SQLQueries) error {
11221095
dbPayment, err := fetchPaymentByHash(ctx, db, paymentHash)
11231096
if err != nil {

‎routing/control_tower_test.go‎

Lines changed: 10 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -50,10 +50,7 @@ func TestControlTowerSubscribeUnknown(t *testing.T) {
5050

5151
db := initDB(t)
5252

53-
paymentDB, err := paymentsdb.NewKVStore(
54-
db,
55-
paymentsdb.WithKeepFailedPaymentAttempts(true),
56-
)
53+
paymentDB, err := paymentsdb.NewKVStore(db)
5754
require.NoError(t, err)
5855

5956
pControl := NewControlTower(paymentDB)
@@ -182,17 +179,11 @@ func TestControlTowerSubscribeSuccess(t *testing.T) {
182179
func TestKVStoreSubscribeFail(t *testing.T) {
183180
t.Parallel()
184181

185-
t.Run("register attempt, keep failed payments", func(t *testing.T) {
186-
testKVStoreSubscribeFail(t, true, true)
187-
})
188-
t.Run("register attempt, delete failed payments", func(t *testing.T) {
189-
testKVStoreSubscribeFail(t, true, false)
190-
})
191-
t.Run("no register attempt, keep failed payments", func(t *testing.T) {
192-
testKVStoreSubscribeFail(t, false, true)
182+
t.Run("register attempt", func(t *testing.T) {
183+
testKVStoreSubscribeFail(t, true)
193184
})
194-
t.Run("no register attempt, delete failed payments", func(t *testing.T) {
195-
testKVStoreSubscribeFail(t, false, false)
185+
t.Run("no register attempt", func(t *testing.T) {
186+
testKVStoreSubscribeFail(t, false)
196187
})
197188
}
198189

@@ -203,10 +194,7 @@ func TestKVStoreSubscribeAllSuccess(t *testing.T) {
203194

204195
db := initDB(t)
205196

206-
paymentDB, err := paymentsdb.NewKVStore(
207-
db,
208-
paymentsdb.WithKeepFailedPaymentAttempts(true),
209-
)
197+
paymentDB, err := paymentsdb.NewKVStore(db)
210198
require.NoError(t, err)
211199

212200
pControl := NewControlTower(paymentDB)
@@ -334,10 +322,7 @@ func TestKVStoreSubscribeAllImmediate(t *testing.T) {
334322

335323
db := initDB(t)
336324

337-
paymentDB, err := paymentsdb.NewKVStore(
338-
db,
339-
paymentsdb.WithKeepFailedPaymentAttempts(true),
340-
)
325+
paymentDB, err := paymentsdb.NewKVStore(db)
341326
require.NoError(t, err)
342327

343328
pControl := NewControlTower(paymentDB)
@@ -385,10 +370,7 @@ func TestKVStoreUnsubscribeSuccess(t *testing.T) {
385370

386371
db := initDB(t)
387372

388-
paymentDB, err := paymentsdb.NewKVStore(
389-
db,
390-
paymentsdb.WithKeepFailedPaymentAttempts(true),
391-
)
373+
paymentDB, err := paymentsdb.NewKVStore(db)
392374
require.NoError(t, err)
393375

394376
pControl := NewControlTower(paymentDB)
@@ -458,17 +440,10 @@ func TestKVStoreUnsubscribeSuccess(t *testing.T) {
458440
require.Len(t, subscription2.Updates(), 0)
459441
}
460442

461-
func testKVStoreSubscribeFail(t *testing.T, registerAttempt,
462-
keepFailedPaymentAttempts bool) {
463-
443+
func testKVStoreSubscribeFail(t *testing.T, registerAttempt bool) {
464444
db := initDB(t)
465445

466-
paymentDB, err := paymentsdb.NewKVStore(
467-
db,
468-
paymentsdb.WithKeepFailedPaymentAttempts(
469-
keepFailedPaymentAttempts,
470-
),
471-
)
446+
paymentDB, err := paymentsdb.NewKVStore(db)
472447
require.NoError(t, err)
473448

474449
pControl := NewControlTower(paymentDB)

‎routing/payment_lifecycle.go‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -338,15 +338,16 @@ lifecycle:
338338
// terminal condition. We either return the settled preimage or the
339339
// payment's failure reason.
340340
//
341-
// Optionally delete the failed attempts from the database. Depends on
342-
// the database options deleting attempts is not allowed so this will
343-
// just be a no-op.
344-
err = p.router.cfg.Control.DeleteFailedAttempts(
345-
cleanupCtx, p.identifier,
346-
)
347-
if err != nil {
348-
log.Errorf("Error deleting failed htlc attempts for payment "+
349-
"%v: %v", p.identifier, err)
341+
// Optionally delete the failed attempts from the database. If we are
342+
// configured to keep failed payment attempts, we skip deletion.
343+
if !p.router.cfg.KeepFailedPaymentAttempts {
344+
err = p.router.cfg.Control.DeleteFailedAttempts(
345+
cleanupCtx, p.identifier,
346+
)
347+
if err != nil {
348+
log.Errorf("Error deleting failed htlc attempts "+
349+
"for payment %v: %v", p.identifier, err)
350+
}
350351
}
351352

352353
htlc, failure := payment.TerminalInfo()

0 commit comments

Comments
 (0)