Merge pull request #6438 from tvolk131/delete_failed_payments

Delete failed payment attempts for successfully settled payments

channeldb/db.go

@@ -247,10 +247,11 @@ type DB struct {
 	// channelStateDB separates all DB operations on channel state.
 	channelStateDB *ChannelStateDB
 
-	dbPath string
+	dbPath                    string
-	graph  *ChannelGraph
+	graph                     *ChannelGraph
-	clock  clock.Clock
+	clock                     clock.Clock
-	dryRun bool
+	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,
+		clock:                     opts.clock,
-		dryRun: opts.dryRun,
+		dryRun:                    opts.dryRun,
+		keepFailedPaymentAttempts: opts.keepFailedPaymentAttempts,
 	}
 
 	// Set the parent pointer (only used in tests).

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
+	}
+}

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

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,

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,
+		MaxOutgoingCltvExpiry:     htlcswitch.DefaultMaxOutgoingCltvExpiry,
-		MaxChannelFeeAllocation: htlcswitch.DefaultMaxLinkFeeAllocation,
+		MaxChannelFeeAllocation:   htlcswitch.DefaultMaxLinkFeeAllocation,
-		MaxCommitFeeRateAnchors: lnwallet.DefaultAnchorsCommitMaxFeeRateSatPerVByte,
+		MaxCommitFeeRateAnchors:   lnwallet.DefaultAnchorsCommitMaxFeeRateSatPerVByte,
-		DustThreshold:           uint64(htlcswitch.DefaultDustThreshold.ToSatoshis()),
+		DustThreshold:             uint64(htlcswitch.DefaultDustThreshold.ToSatoshis()),
-		LogWriter:               build.NewRotatingLogWriter(),
+		LogWriter:                 build.NewRotatingLogWriter(),
-		DB:                      lncfg.DefaultDB(),
+		DB:                        lncfg.DefaultDB(),
-		Cluster:                 lncfg.DefaultCluster(),
+		Cluster:                   lncfg.DefaultCluster(),
-		RPCMiddleware:           lncfg.DefaultRPCMiddleware(),
+		RPCMiddleware:             lncfg.DefaultRPCMiddleware(),
-		registeredChains:        chainreg.NewChainRegistry(),
+		registeredChains:          chainreg.NewChainRegistry(),
-		ActiveNetParams:         chainreg.BitcoinTestNetParams,
+		ActiveNetParams:           chainreg.BitcoinTestNetParams,
-		ChannelCommitInterval:   defaultChannelCommitInterval,
+		ChannelCommitInterval:     defaultChannelCommitInterval,
-		PendingCommitInterval:   defaultPendingCommitInterval,
+		PendingCommitInterval:     defaultPendingCommitInterval,
-		ChannelCommitBatchSize:  defaultChannelCommitBatchSize,
+		ChannelCommitBatchSize:    defaultChannelCommitBatchSize,
-		CoinSelectionStrategy:   defaultCoinSelectionStrategy,
+		CoinSelectionStrategy:     defaultCoinSelectionStrategy,
+		KeepFailedPaymentAttempts: defaultKeepFailedPaymentAttempts,
 		RemoteSigner: &lncfg.RemoteSigner{
 			Timeout: lncfg.DefaultRemoteSignerRPCTimeout,
 		},

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

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

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,

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) {
+	t.Run("register attempt, keep failed payments", func(t *testing.T) {
-		testPaymentControlSubscribeFail(t, true)
+		testPaymentControlSubscribeFail(t, true, true)
 	})
-	t.Run("no register attempt", func(t *testing.T) {
+	t.Run("register attempt, delete failed payments", func(t *testing.T) {
-		testPaymentControlSubscribeFail(t, false)
+		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) {
+func testPaymentControlSubscribeFail(t *testing.T, registerAttempt,
-	db, err := initDB()
+	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
+	// We expect both subscribers to now report the final outcome followed
-	// no other events.
+	// 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
 	}

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 {
 

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 {
+				if a.Settle == nil {
-					return a.Settle.Preimage, &a.Route, 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
 

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

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.