Repository: lightningnetwork/lnd (go)

Issue:
Disk Space Reduction: Remove Failed Payments
Breaking down the [master issue](https://github.com/lightningnetwork/lnd/issues/5388) into independent issues for tracking

Only keep around failed payment attempts if specified by a new flag. This is a major culprit of ballooning database state as due to things like payment splitting a successful attempt may generate several failures.




Pull request description:
Delete failed payment attempts for successfully settled payments
Fixes #6318

Failed payment attempts are deleted from channel.db for successfully settled payments, unless the `keep-failed-payment-attempts` flag is specified in configuration.

Summarizing part of the discussion below:
We only want to delete failed attempts for _settled_ payments. If we were to delete failed attempts for failed payments, that would leave all failed payments as empty shells, so we might as well delete the whole payment. This might be a nice optional feature to have, but should be handled in a separate PR (if at all). And we don't want to delete failed attempts for inflight payments since inflight payments could end up failing.

Patch:
diff --git a/channeldb/db.go b/channeldb/db.go
index 8a6935941..ac0ba9642 100644
--- a/channeldb/db.go
+++ b/channeldb/db.go
@@ -247,10 +247,11 @@ type DB struct {
 	// channelStateDB separates all DB operations on channel state.
 	channelStateDB *ChannelStateDB
 
-	dbPath string
-	graph  *ChannelGraph
-	clock  clock.Clock
-	dryRun bool
+	dbPath                    string
+	graph                     *ChannelGraph
+	clock                     clock.Clock
+	dryRun                    bool
+	keepFailedPaymentAttempts bool
 }
 
 // Open opens or creates channeldb. Any necessary schemas migrations due
@@ -303,8 +304,9 @@ func CreateWithBackend(backend kvdb.Backend, modifiers ...OptionModifier) (*DB,
 			},
 			backend: backend,
 		},
-		clock:  opts.clock,
-		dryRun: opts.dryRun,
+		clock:                     opts.clock,
+		dryRun:                    opts.dryRun,
+		keepFailedPaymentAttempts: opts.keepFailedPaymentAttempts,
 	}
 
 	// Set the parent pointer (only used in tests).
diff --git a/channeldb/options.go b/channeldb/options.go
index 068a7fa01..7e121d254 100644
--- a/channeldb/options.go
+++ b/channeldb/options.go
@@ -61,6 +61,10 @@ type Options struct {
 	// dryRun will fail to commit a successful migration when opening the
 	// database if set to true.
 	dryRun bool
+
+	// keepFailedPaymentAttempts determines whether failed htlc attempts
+	// are kept on disk or removed to save space.
+	keepFailedPaymentAttempts bool
 }
 
 // DefaultOptions returns an Options populated with default values.
@@ -164,3 +168,11 @@ func OptionDryRunMigration(dryRun bool) OptionModifier {
 		o.dryRun = dryRun
 	}
 }
+
+// OptionKeepFailedPaymentAttempts controls whether failed payment attempts are
+// kept on disk after a payment settles.
+func OptionKeepFailedPaymentAttempts(keepFailedPaymentAttempts bool) OptionModifier {
+	return func(o *Options) {
+		o.keepFailedPaymentAttempts = keepFailedPaymentAttempts
+	}
+}
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/config.go b/config.go
index d313033a6..71044e898 100644
--- a/config.go
+++ b/config.go
@@ -199,6 +199,10 @@ const (
 	// defaultCoinSelectionStrategy is the coin selection strategy that is
 	// used by default to fund transactions.
 	defaultCoinSelectionStrategy = "largest"
+
+	// defaultKeepFailedPaymentAttempts is the default setting for whether
+	// to keep failed payments in the database.
+	defaultKeepFailedPaymentAttempts = false
 )
 
 var (
@@ -362,6 +366,8 @@ type Config struct {
 
 	ChannelCommitBatchSize uint32 `long:"channel-commit-batch-size" description:"The maximum number of channel state updates that is accumulated before signing a new commitment."`
 
+	KeepFailedPaymentAttempts bool `long:"keep-failed-payment-attempts" description:"Keeps persistent record of all failed payment attempts for successfully settled payments."`
+
 	DefaultRemoteMaxHtlcs uint16 `long:"default-remote-max-htlcs" description:"The default max_htlc applied when opening or accepting channels. This value limits the number of concurrent HTLCs that the remote party can add to the commitment. The maximum possible value is 483."`
 
 	NumGraphSyncPeers      int           `long:"numgraphsyncpeers" description:"The number of peers that we should receive new graph updates from. This option can be tuned to save bandwidth for light clients or routing nodes."`
@@ -611,20 +617,21 @@ func DefaultConfig() Config {
 		Invoices: &lncfg.Invoices{
 			HoldExpiryDelta: lncfg.DefaultHoldInvoiceExpiryDelta,
 		},
-		MaxOutgoingCltvExpiry:   htlcswitch.DefaultMaxOutgoingCltvExpiry,
-		MaxChannelFeeAllocation: htlcswitch.DefaultMaxLinkFeeAllocation,
-		MaxCommitFeeRateAnchors: lnwallet.DefaultAnchorsCommitMaxFeeRateSatPerVByte,
-		DustThreshold:           uint64(htlcswitch.DefaultDustThreshold.ToSatoshis()),
-		LogWriter:               build.NewRotatingLogWriter(),
-		DB:                      lncfg.DefaultDB(),
-		Cluster:                 lncfg.DefaultCluster(),
-		RPCMiddleware:           lncfg.DefaultRPCMiddleware(),
-		registeredChains:        chainreg.NewChainRegistry(),
-		ActiveNetParams:         chainreg.BitcoinTestNetParams,
-		ChannelCommitInterval:   defaultChannelCommitInterval,
-		PendingCommitInterval:   defaultPendingCommitInterval,
-		ChannelCommitBatchSize:  defaultChannelCommitBatchSize,
-		CoinSelectionStrategy:   defaultCoinSelectionStrategy,
+		MaxOutgoingCltvExpiry:     htlcswitch.DefaultMaxOutgoingCltvExpiry,
+		MaxChannelFeeAllocation:   htlcswitch.DefaultMaxLinkFeeAllocation,
+		MaxCommitFeeRateAnchors:   lnwallet.DefaultAnchorsCommitMaxFeeRateSatPerVByte,
+		DustThreshold:             uint64(htlcswitch.DefaultDustThreshold.ToSatoshis()),
+		LogWriter:                 build.NewRotatingLogWriter(),
+		DB:                        lncfg.DefaultDB(),
+		Cluster:                   lncfg.DefaultCluster(),
+		RPCMiddleware:             lncfg.DefaultRPCMiddleware(),
+		registeredChains:          chainreg.NewChainRegistry(),
+		ActiveNetParams:           chainreg.BitcoinTestNetParams,
+		ChannelCommitInterval:     defaultChannelCommitInterval,
+		PendingCommitInterval:     defaultPendingCommitInterval,
+		ChannelCommitBatchSize:    defaultChannelCommitBatchSize,
+		CoinSelectionStrategy:     defaultCoinSelectionStrategy,
+		KeepFailedPaymentAttempts: defaultKeepFailedPaymentAttempts,
 		RemoteSigner: &lncfg.RemoteSigner{
 			Timeout: lncfg.DefaultRemoteSignerRPCTimeout,
 		},
diff --git a/config_builder.go b/config_builder.go
index 18e9a3f6f..05f8ab168 100644
--- a/config_builder.go
+++ b/config_builder.go
@@ -852,6 +852,7 @@ func (d *DefaultDatabaseBuilder) BuildDatabase(
 		channeldb.OptionSetBatchCommitInterval(cfg.DB.BatchCommitInterval),
 		channeldb.OptionDryRunMigration(cfg.DryRunMigration),
 		channeldb.OptionSetUseGraphCache(!cfg.DB.NoGraphCache),
+		channeldb.OptionKeepFailedPaymentAttempts(cfg.KeepFailedPaymentAttempts),
 	}
 
 	// We want to pre-allocate the channel graph cache according to what we
diff --git a/docs/release-notes/release-notes-0.15.1.md b/docs/release-notes/release-notes-0.15.1.md
index 4c3608d51..b97848fd9 100644
--- a/docs/release-notes/release-notes-0.15.1.md
+++ b/docs/release-notes/release-notes-0.15.1.md
@@ -16,6 +16,11 @@
   addholdinvoice`](https://github.com/lightningnetwork/lnd/pull/6577). Users now
   need to explicitly specify the `--private` flag.
 
+## Database
+
+* [Delete failed payment attempts](https://github.com/lightningnetwork/lnd/pull/6438)
+  once payments are settled, unless specified with `keep-failed-payment-attempts` flag.
+
 ## Documentation
 
 * [Add minor comment](https://github.com/lightningnetwork/lnd/pull/6559) on
@@ -57,4 +62,5 @@
 * Eugene Siegel
 * Oliver Gugger
 * Priyansh Rastogi
+* Tommy Volk
 * Yong Yu
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/payment_lifecycle.go b/routing/payment_lifecycle.go
index f8b3cd558..36c63eb14 100644
--- a/routing/payment_lifecycle.go
+++ b/routing/payment_lifecycle.go
@@ -55,7 +55,7 @@ func (ps paymentState) terminated() bool {
 }
 
 // needWaitForShards returns a bool to specify whether we need to wait for the
-// outcome of the shanrdHandler.
+// outcome of the shardHandler.
 func (ps paymentState) needWaitForShards() bool {
 	// If we have in flight shards and the payment is in final stage, we
 	// need to wait for the outcomes from the shards. Or if we have no more
@@ -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.
@@ -279,7 +291,7 @@ lifecycle:
 			continue lifecycle
 		}
 
-		// If this route will consume the last remeining amount to send
+		// If this route will consume the last remaining amount to send
 		// to the receiver, this will be our last shard (for now).
 		lastShard := rt.ReceiverAmt() == currentState.remainingAmt
 
diff --git a/sample-lnd.conf b/sample-lnd.conf
index 114ad5c19..5685f8275 100644
--- a/sample-lnd.conf
+++ b/sample-lnd.conf
@@ -311,6 +311,10 @@
 ; a new commitment.
 ; channel-commit-batch-size=10
 
+; Keeps persistent record of all failed payment attempts for successfully
+; settled payments.
+; keep-failed-payment-attempts=false
+
 ; The default max_htlc applied when opening or accepting channels. This value
 ; limits the number of concurrent HTLCs that the remote party can add to the
 ; commitment. The maximum possible value is 483.


Test patch:
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_test.go b/routing/control_tower_test.go
index e24870c87..267e60505 100644
--- a/routing/control_tower_test.go
+++ b/routing/control_tower_test.go
@@ -48,7 +48,7 @@ var (
 func TestControlTowerSubscribeUnknown(t *testing.T) {
 	t.Parallel()
 
-	db, err := initDB()
+	db, err := initDB(false)
 	require.NoError(t, err, "unable to init db")
 
 	pControl := NewControlTower(channeldb.NewPaymentControl(db))
@@ -65,7 +65,7 @@ func TestControlTowerSubscribeUnknown(t *testing.T) {
 func TestControlTowerSubscribeSuccess(t *testing.T) {
 	t.Parallel()
 
-	db, err := initDB()
+	db, err := initDB(false)
 	require.NoError(t, err, "unable to init db")
 
 	pControl := NewControlTower(channeldb.NewPaymentControl(db))
@@ -165,16 +165,24 @@ func TestControlTowerSubscribeSuccess(t *testing.T) {
 func TestPaymentControlSubscribeFail(t *testing.T) {
 	t.Parallel()
 
-	t.Run("register attempt", func(t *testing.T) {
-		testPaymentControlSubscribeFail(t, true)
+	t.Run("register attempt, keep failed payments", func(t *testing.T) {
+		testPaymentControlSubscribeFail(t, true, true)
 	})
-	t.Run("no register attempt", func(t *testing.T) {
-		testPaymentControlSubscribeFail(t, false)
+	t.Run("register attempt, delete failed payments", func(t *testing.T) {
+		testPaymentControlSubscribeFail(t, true, false)
+	})
+	t.Run("no register attempt, keep failed payments", func(t *testing.T) {
+		testPaymentControlSubscribeFail(t, false, true)
+	})
+	t.Run("no register attempt, delete failed payments", func(t *testing.T) {
+		testPaymentControlSubscribeFail(t, false, false)
 	})
 }
 
-func testPaymentControlSubscribeFail(t *testing.T, registerAttempt bool) {
-	db, err := initDB()
+func testPaymentControlSubscribeFail(t *testing.T, registerAttempt,
+	keepFailedPaymentAttempts bool) {
+
+	db, err := initDB(keepFailedPaymentAttempts)
 	require.NoError(t, err, "unable to init db")
 
 	pControl := NewControlTower(channeldb.NewPaymentControl(db))
@@ -228,8 +236,8 @@ func testPaymentControlSubscribeFail(t *testing.T, registerAttempt bool) {
 	subscriber2, err := pControl.SubscribePayment(info.PaymentIdentifier)
 	require.NoError(t, err, "expected subscribe to succeed, but got")
 
-	// We expect all subscribers to now report the final outcome followed by
-	// no other events.
+	// We expect both subscribers to now report the final outcome followed
+	// by no other events.
 	subscribers := []*ControlTowerSubscriber{
 		subscriber1, subscriber2,
 	}
@@ -286,13 +294,17 @@ func testPaymentControlSubscribeFail(t *testing.T, registerAttempt bool) {
 	}
 }
 
-func initDB() (*channeldb.DB, error) {
+func initDB(keepFailedPaymentAttempts bool) (*channeldb.DB, error) {
 	tempPath, err := ioutil.TempDir("", "routingdb")
 	if err != nil {
 		return nil, err
 	}
 
-	db, err := channeldb.Open(tempPath)
+	db, err := channeldb.Open(
+		tempPath, channeldb.OptionKeepFailedPaymentAttempts(
+			keepFailedPaymentAttempts,
+		),
+	)
 	if err != nil {
 		return nil, err
 	}
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/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